Notion markdown followup #16214
Notion markdown followup #16214
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 3 Skipped Deployments
|
|
Caution Review failedThe pull request is closed. WalkthroughThis update refactors the retrieval logic for Notion blocks. In the Changes
Possibly related PRs
Suggested reviewers
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
components/notion/actions/append-block/append-block.mjsOops! Something went wrong! :( ESLint: 8.57.1 Error [ERR_MODULE_NOT_FOUND]: Cannot find package 'jsonc-eslint-parser' imported from /eslint.config.mjs components/notion/actions/duplicate-page/duplicate-page.mjsOops! Something went wrong! :( ESLint: 8.57.1 Error [ERR_MODULE_NOT_FOUND]: Cannot find package 'jsonc-eslint-parser' imported from /eslint.config.mjs 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
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 (
|
| }); | ||
| }, | ||
| async retrieveBlockChildren(block, params = {}) { | ||
| async retrieveBlockChildren(block, subpagesOnly = false) { |
There was a problem hiding this comment.
For the record, I've checked that all instances of this method being called did not include a params argument, so it was always initialized as an empty object - hence I changed it to a variable declaration instead.
|
I'm also skipping the mass-version-bumping that is usually required for the check to pass, and opting to update only the versions of the components that are affected by the change (double-checked). |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
components/notion/notion.app.mjs (1)
339-339: Consider simplifying the filter expressionThe filtering logic is functional but a bit complex. Consider splitting it into a more readable form for better maintainability.
- children.push(...(results.filter((child) => !subpagesOnly || child.type === "child_page"))); + const filteredResults = subpagesOnly + ? results.filter((child) => child.type === "child_page") + : results; + children.push(...filteredResults);components/notion/actions/retrieve-block/retrieve-block.mjs (3)
36-39: Consider renaming for consistency with response formatFor better consistency with the documented response format, consider renaming
markdownContenttomarkdown.- let markdownContent; + let markdown; if (this.retrieveMarkdown) { - markdownContent = await this.notion.getPageAsMarkdown(this.blockId); + markdown = await this.notion.getPageAsMarkdown(this.blockId); }
53-53: Unnecessary null coalescing operatorSince
block.childrenis already defined on line 50, the null coalescing operator (??) is unnecessary here.- $.export("$summary", `Successfully retrieved block${retrieveChildren - ? ` with ${block.children.length ?? 0} ${subpagesOnly + $.export("$summary", `Successfully retrieved block${retrieveChildren + ? ` with ${block.children.length} ${subpagesOnly
57-62: Consider consistent naming in response formatFor consistency with the documented response format mentioned in the PR objectives, consider renaming
markdownContenttomarkdownin the returned object.- return markdownContent + return markdown ? { - markdownContent, + markdown, block, } : block;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (3)
components/notion/actions/retrieve-block/retrieve-block.mjs(2 hunks)components/notion/notion.app.mjs(2 hunks)components/notion/package.json(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Verify TypeScript components
- GitHub Check: pnpm publish
- GitHub Check: Publish TypeScript components
🔇 Additional comments (7)
components/notion/package.json (1)
3-3: Version update is appropriate for the new featuresThe increment from "0.5.0" to "0.6.0" correctly follows semantic versioning principles for backward-compatible functionality additions. This aligns with the changes introduced in this PR.
components/notion/notion.app.mjs (2)
328-329: Good change to method signatureChanging from a generic
paramsobject to a specificsubpagesOnlyboolean parameter improves the method's clarity and makes its purpose more explicit. The initialization of an emptyparamsobject inside the method is also appropriate.
343-343: Correctly propagates filtering optionThe recursive call properly passes the
subpagesOnlyparameter, ensuring consistent filtering throughout the recursion chain.components/notion/actions/retrieve-block/retrieve-block.mjs (4)
7-7: Version increment is appropriateThe bump from "0.1.0" to "0.2.0" is correct for a non-breaking feature enhancement.
18-26: Good enhancement to retrieveChildren propertyConverting from a boolean to a string type with meaningful options improves the API's flexibility. The options are well-labeled and the description clearly explains the functionality.
41-43: Clear setup for child retrieval optionsGood extraction of the
retrieveChildrenproperty and setting up thesubpagesOnlyflag based on the selected option.
45-51: Backward compatibility is well maintainedExcellent implementation of backward compatibility by including
trueas a valid option for child retrieval. This ensures existing integrations continue to work.
|
We shipped this one due to its urgency, but code review and QA would be helpful nonetheless - we can follow up with any adjustments needed |
michelle0927
left a comment
michelle0927
left a comment
There was a problem hiding this comment.
I went ahead and reviewed this because it didn't have a reviewer assigned. Found one small issue.
| } | ||
| $.export("$summary", `Successfully retrieved block${this.retrieveChildren | ||
| ? ` with ${block.children.length ?? 0} children` | ||
| $.export("$summary", `Successfully retrieved block${retrieveChildren |
There was a problem hiding this comment.
This line gets an error when retrieveChildren is set to None. Should add && retrieveChildren !== "None":
$.export("$summary", Successfully retrieved block${retrieveChildren && retrieveChildren !== "None"`
michelle0927
moved this from Ready for PR Review
to Changes Required
in Component (Source and Action) Backlog
michelle0927
moved this from Changes Required
to Done
in Component (Source and Action) Backlog
The
Retrieve Contentprop was renamed back toRetrieve Childrenand changed to a three-option prop instead of a boolean (still optional).I've special-cased this to check for a boolean value in the code and keep the same behavior (true = 'all children', false = 'none') in case users update the action and don't change the prop's value, so it'll function exactly as it did before.
Finally, the markdown is now returned as well as the blocks, such that enabling the 'Retrieve as Markdown' prop returns this:
while not enabling it keeps the same behavior as before it existed.
Summary by CodeRabbit