Adding talk brainstorming section #84
Conversation
|
Warning Rate limit exceeded@jpchauvel has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 11 minutes and 30 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (2)
WalkthroughA new Sphinx extension was introduced to embed Notion pages via an iframe using a custom directive. The Sphinx configuration was updated to enable this extension. Additionally, a new markdown file for brainstorming talks was added, utilizing the new Notion embedding directive. Minor documentation updates include the addition of a hidden toctree for navigation and a correction of a typographical error in a toctree caption. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Sphinx
participant NotionDirective
participant Notion
User->>Sphinx: Builds documentation
Sphinx->>NotionDirective: Encounters {notion} directive with Notion URL
NotionDirective->>Notion: Generates iframe HTML with Notion URL
Notion-->>Sphinx: Returns raw HTML node
Sphinx-->>User: Renders documentation with embedded Notion page
Poem
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
ext/notion.py (3)
8-8: Unused logger.The logger is initialized but never used in the code. Consider either removing it or using it to log relevant events.
-logger = logging.getLogger(__name__)
26-33: Add proper docstring for setup function.The setup function should include a docstring explaining its purpose and parameters.
def setup(app: Sphinx): + """ + Setup function for the Notion directive Sphinx extension. + + Args: + app: Sphinx application instance + + Returns: + Extension metadata dictionary + """ app.add_directive("notion",Notion) return { "version": "0.1", "parallel_read_safe": True, "parallel_write_safe": True, }
27-27: Missing space after comma.There should be a space after the comma in the
add_directivecall for better readability.- app.add_directive("notion",Notion) + app.add_directive("notion", Notion)talk-brainstorming.md (1)
3-4: Consider adding context for the embedded Notion page.While the embedding works, it would be helpful to add a brief description of what users can expect to find in this Notion page.
# Brainstorming de Charlas +<!-- Esta página contiene una lista colaborativa de ideas para charlas y talleres propuestos por la comunidad. --> + ```{notion} https://forested-roadrunner-0a7.notion.site/ebd/1e114afe2a67809c9ff0eb9429b1d3c7?v=1e114afe2a6780d88e41000c2debb2b5</blockquote></details> <details> <summary>conf.py (1)</summary><blockquote> `215-219`: **Consider grouping custom vs third-party extensions.** For clarity, you could separate locally maintained extensions (`"embed"`, `"notion"`) from third-party ones (e.g., `"myst_parser"`, `"sphinx_design"`). A blank line or a comment header before `"embed"`/`"notion"` will make future maintenance easier. </blockquote></details> </blockquote></details> <details> <summary>📜 Review details</summary> **Configuration used: CodeRabbit UI** **Review profile: CHILL** **Plan: Pro** <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between 4f93d75db5eae70a1cf562e197a62b65cbe57f41 and 6c19da667f4e8018727b38263235322bfefbcc4d. </details> <details> <summary>📒 Files selected for processing (5)</summary> * `conf.py` (2 hunks) * `events.md` (1 hunks) * `ext/notion.py` (1 hunks) * `index.md` (1 hunks) * `talk-brainstorming.md` (1 hunks) </details> <details> <summary>⏰ Context from checks skipped due to timeout of 90000ms (1)</summary> * GitHub Check: deploy </details> <details> <summary>🔇 Additional comments (4)</summary><blockquote> <details> <summary>events.md (1)</summary> `13-18`: **LGTM! The toctree addition looks good.** The toctree directive is properly added to include the new talk-brainstorming.md file and is appropriately hidden. </details> <details> <summary>index.md (1)</summary> `82-82`: **LGTM! Typo correction.** Good catch on fixing the typo from "Secciónes" to "Secciones". </details> <details> <summary>talk-brainstorming.md (1)</summary> `1-5`: <details> <summary>✅ Verification successful</summary> **Verify that the Notion page is publicly accessible.** Make sure that the Notion page is set with the correct sharing permissions so all users can access it without requiring a login. --- 🏁 Script executed: ```shell #!/bin/bash # Check if the Notion URL returns a successful response curl -s -I "https://forested-roadrunner-0a7.notion.site/ebd/1e114afe2a67809c9ff0eb9429b1d3c7?v=1e114afe2a6780d88e41000c2debb2b5" | head -n 1Length of output: 154
Notion embed is publicly accessible
Confirmed via HTTP/2 200 response—no further action required.
conf.py (1)
217-219:
Details
✅ Verification successful
Verify the custom Notion extension registration.
Ensure that ext/notion.py exists and that the earlier sys.path.append(str(Path("./ext").resolve())) correctly exposes the notion module so Sphinx can load it without import errors.
Run:
🏁 Script executed:
#!/bin/bash
# Check for the presence of the custom extension file
if [[ -f ext/notion.py ]]; then
echo "✔ ext/notion.py found"
else
echo "✖ ext/notion.py is missing"
fiLength of output: 77
🏁 Script executed:
#!/bin/bash
# Verify sys.path is extended to include the ext directory in conf.py
rg -n "sys\.path\.append.*ext" conf.pyLength of output: 89
Custom Notion extension registration verified
Both conditions are satisfied:
- ✔
ext/notion.pyexists - ✔
sys.path.append(str(Path("./ext").resolve()))is present inconf.py
No further action required.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
ext/notion.py (3)
37-37: Add space after comma for better readability.There should be a space after the comma in the
add_directivemethod call.- app.add_directive("notion",Notion) + app.add_directive("notion", Notion)
10-13: Consider making iframe dimensions configurable.The iframe has a fixed height of 1200 pixels, which may not be optimal for all screen sizes or content lengths. Consider making the dimensions configurable through directive options.
class Notion(Directive): has_content = True final_argument_whitespace = False + option_spec = { + 'height': directives.length_or_unitless, + 'width': directives.length_or_percentage_or_unitless, + } def run(self): # ... validation code ... + height = self.options.get('height', '1200') + width = self.options.get('width', '100%') + para = nodes.raw( - "", TEMPLATE.format(url=url), format="html" + "", TEMPLATE.format(url=url, height=height, width=width), format="html" ) return [para]And update the template:
-TEMPLATE: str = """ -<iframe src="{url}" width="100%" height="1200" frameborder="0" allowfullscreen> -</iframe> -""" +TEMPLATE: str = """ +<iframe src="{url}" width="{width}" height="{height}" frameborder="0" allowfullscreen> +</iframe> +"""
3-6: Missing import for potential enhancements.If implementing the suggested configurable dimensions, you'll need to import the
directivesmodule.from docutils import nodes from docutils.parsers.rst import Directive +from docutils.parsers.rst import directives from sphinx.application import Sphinx from sphinx.util import logging
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
ext/notion.py(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: deploy
ext/notion.py
Outdated
| if not url.startswith(("https://notion.site", "https://www.notion.so")) and "notion" not in url: | ||
| raise self.error("URL does not appear to be a valid Notion URL") |
There was a problem hiding this comment.
🛠️ Refactor suggestion
URL validation could be more robust.
The current validation allows any URL containing "notion" which could be too permissive. Consider using a more restrictive pattern or providing a warning when falling back to this looser check.
- if not url.startswith(("https://notion.site", "https://www.notion.so")) and "notion" not in url:
+ if url.startswith(("https://notion.site", "https://www.notion.so")):
+ pass # Standard Notion URLs are fine
+ elif "notion" in url:
+ logger.warning(f"URL {url} doesn't use standard Notion domains but contains 'notion'. Proceeding with caution.")
+ else:
raise self.error("URL does not appear to be a valid Notion URL")Committable suggestion skipped: line range outside the PR's diff.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Summary by CodeRabbit
New Features
Documentation