[WIKI-345] regression: update block menu options#7647
[WIKI-345] regression: update block menu options#7647sriramveeraghanta merged 2 commits intopreviewfrom
Conversation
WalkthroughAdds embedHandler to core extension props type. Makes PageRenderer require flaggedExtensions and disabledExtensions. Adjusts Document editor to pass props accordingly (order change only). No runtime logic changes. Changes
Sequence Diagram(s)Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
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:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
iam-vipin
changed the title
There was a problem hiding this comment.
Pull Request Overview
This PR addresses a regression in the block menu functionality by making the flaggedExtensions and disabledExtensions props required instead of optional. The changes ensure that these props are properly passed to the BlockMenu component.
- Updated prop types to make
flaggedExtensionsanddisabledExtensionsrequired in PageRenderer - Reordered props in DocumentEditor component call
- Added
embedHandlerto the core extensions type definition
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| page-renderer.tsx | Makes flaggedExtensions and disabledExtensions required props for PageRenderer |
| editor.tsx | Reorders props in PageRenderer call (flaggedExtensions and disabledExtensions before isTouchDevice) |
| extensions.ts | Adds embedHandler to TCoreAdditionalExtensionsProps type definition |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
![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 (4)
packages/editor/src/ce/extensions/core/extensions.ts (2)
10-13: Remove empty destructuring to appease linters and reduce noise.
const {} = props;can trigger eslint’s no-empty-pattern and adds noise. It’s safe to drop.export const CoreEditorAdditionalExtensions = (props: TCoreAdditionalExtensionsProps): Extensions => { - const {} = props; return []; };
5-8: Unify embedHandler/embedConfig namingIt looks like you’ve successfully surfaced
embedHandlerin the core props, but the document extensions API still usesembedConfig. This dual naming shows up in several places and could lead to confusion down the road. Consider renaming one to match the other across the codebase:• packages/editor/src/ce/extensions/document-extensions.tsx:
•embedConfig: TEmbedConfig | undefined
• packages/editor/src/core/hooks/use-collaborative-editor.ts:
•DocumentEditorAdditionalExtensions({ …, embedConfig: embedHandler, … })
• packages/editor/src/core/components/editors/document/editor.tsx:
•DocumentEditorAdditionalExtensions({ …, embedConfig: embedHandler, … })You might either:
- Rename all
embedConfigparameters toembedHandlerin the DocumentEditorAdditionalExtensions API- Or switch the core prop back to
embedConfigfor full alignmentEither approach will reduce cognitive load and prevent mix-ups in future refactors.
packages/editor/src/core/components/editors/document/editor.tsx (2)
40-69: useMemo has an empty dependency array but derives from props; consider adding deps or document “init-only” intent.The extensions array depends on several props (embedHandler, disabledExtensions, flaggedExtensions, editable, fileHandler, user). With
[], updates to those won’t recompute extensions, which could lead to stale behavior if parent props change post-mount. If intentional (init-only), add a brief comment. Otherwise, add dependencies.- const extensions: Extensions = useMemo(() => { + const extensions: Extensions = useMemo(() => { const additionalExtensions: Extensions = []; if (embedHandler?.issue) { additionalExtensions.push( WorkItemEmbedExtension({ widgetCallback: embedHandler.issue.widgetCallback, }) ); } additionalExtensions.push( SideMenuExtension({ - aiEnabled: !disabledExtensions?.includes("ai"), + aiEnabled: !disabledExtensions?.includes("ai"), }), HeadingListExtension, ...DocumentEditorAdditionalExtensions({ disabledExtensions, - embedConfig: embedHandler, + embedConfig: embedHandler, flaggedExtensions, isEditable: editable, fileHandler, userDetails: user ?? { id: "", name: "", color: "", }, }) ); return additionalExtensions; - }, []); + }, [embedHandler, disabledExtensions, flaggedExtensions, editable, fileHandler, user?.id, user?.name, user?.color]);If the intent is “construct once,” add:
// Intentional: build extensions once on mount.
49-67: Minor naming mismatch: embedConfig vs embedHandler.Here we pass
embedConfig: embedHandler. This works, but the dual terminology across modules is confusing. Consider standardizing on “embedHandler” throughout (or “embedConfig” if that’s the chosen term) in a follow-up to improve coherence.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
packages/editor/src/ce/extensions/core/extensions.ts(1 hunks)packages/editor/src/core/components/editors/document/editor.tsx(1 hunks)packages/editor/src/core/components/editors/document/page-renderer.tsx(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
packages/editor/src/core/components/editors/document/page-renderer.tsx (1)
packages/editor/src/core/types/editor.ts (1)
IEditorProps(133-160)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build and lint web apps
🔇 Additional comments (1)
packages/editor/src/core/components/editors/document/editor.tsx (1)
103-104: Boolean cast for isTouchDevice is fine.The double-bang ensures a strict boolean; no issues here.
Description
this pr update the requirement of flaggedExtensions and disabledExtensions for block menu
Type of Change
Screenshots and Media (if applicable)
Test Scenarios
References
Summary by CodeRabbit
Refactor
Chores