[WEB-462] refactor: editor props structure#7233
Conversation
WalkthroughThis change introduces a new Changes
Sequence Diagram(s)sequenceDiagram
participant Parent as Parent Component
participant Flagging as useEditorFlagging
participant Editor as Editor Component
participant Hook as useEditor/useReadOnlyEditor
participant Extensions as CoreEditorExtensions
Parent->>Flagging: useEditorFlagging()
Flagging-->>Parent: {disabled, flagged} arrays for editor type
Parent->>Editor: Pass disabledExtensions, flaggedExtensions
Editor->>Hook: Pass disabledExtensions, flaggedExtensions
Hook->>Extensions: Pass disabledExtensions, flaggedExtensions
Extensions-->>Hook: Compose extensions using both arrays
Hook-->>Editor: Editor instance with configured extensions
Possibly related PRs
Suggested labels
Suggested reviewers
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:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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: 6
🔭 Outside diff range comments (6)
packages/editor/src/ce/extensions/rich-text/read-only-extensions.tsx (1)
13-18:isEnabledshould acceptflaggedExtensionsas wellThe new extension-flagging feature is ignored here—
isEnabledonly receivesdisabledExtensions. As a result, a “flagged” extension might still be enabled in read-only mode.- isEnabled: (disabledExtensions: TExtensions[]) => boolean; + isEnabled: ( + disabledExtensions: TExtensions[], + flaggedExtensions: TExtensions[] + ) => boolean;You’ll also need to update every registry entry and the filter call at lines 25-28:
- .filter((config) => config.isEnabled(disabledExtensions)) + .filter((config) => config.isEnabled(disabledExtensions, props.flaggedExtensions ?? []))packages/editor/src/core/extensions/extensions.ts (1)
192-196: Guard.includesagainst emptydisabledExtensionsAfter applying the previous change, this is safe. If you choose not to default, wrap the check:
-if (!disabledExtensions.includes("image")) { +if (!disabledExtensions?.includes("image")) {packages/editor/src/core/hooks/use-editor.ts (1)
46-81: Stale closure risk: dependency array only containseditable.
useTiptapEditorwill not recreate the editor whenflaggedExtensions,disabledExtensions,placeholder,fileHandler, etc. change at runtime.
If any of those props are expected to change (e.g. after a feature-flag fetch) the editor will keep the old values, leading to inconsistent UI.- const editor = useTiptapEditor( + const editor = useTiptapEditor( { /* config */ }, - [editable] + [ + editable, + disabledExtensions?.join(), + flaggedExtensions?.join(), + placeholder, + enableHistory, + fileHandler, // handlers are objects – consider memoising before passing + ] );web/core/components/editor/rich-text-editor/rich-text-editor.tsx (1)
28-38:flaggedExtensionsis forwarded twice – later spread overrides the calculated value.Because
flaggedExtensionsis not plucked out ofprops, it remains inside...rest.
The explicit prop you pass on line 54 will be silently overwritten by whatever the caller supplied.- uploadFile, - disabledExtensions: additionalDisabledExtensions, + uploadFile, + disabledExtensions: additionalDisabledExtensions, + // hoist and discard to keep it out of `...rest` + flaggedExtensions: _ignoredFlaggedExtensions,This guarantees that the editor always receives the union computed from the flagging hook.
Duplicate removal also prevents hard-to-trace bugs when the two arrays diverge.web/core/components/editor/rich-text-editor/rich-text-read-only-editor.tsx (1)
28-34: Same double-prop collision as in the writable editor.
flaggedExtensionssurvives inside...propsand will overwrite the computed value below.
Apply the same extraction pattern:- ({ workspaceId, workspaceSlug, projectId, disabledExtensions: additionalDisabledExtensions, ...props }, ref) => { + ( + { + workspaceId, + workspaceSlug, + projectId, + disabledExtensions: additionalDisabledExtensions, + flaggedExtensions: _ignoredFlaggedExtensions, + ...props + }, + ref, + ) => {web/core/components/editor/lite-text-editor/lite-text-editor.tsx (1)
96-115: DuplicateflaggedExtensionsprop passed – remove oneYou explicitly pass
flaggedExtensionsand then spread...rest, which (see comment above) may contain anotherflaggedExtensions.
Removing it fromrestor re-ordering the spread avoids accidental overrides and React warnings about duplicate props.
♻️ Duplicate comments (1)
packages/editor/src/ce/extensions/document-extensions.tsx (1)
19-27: SameisEnabledparameter mismatch as in rich-text registryAlign the function signature with the declared type to avoid type-checking failures.
- isEnabled: (disabledExtensions) => !disabledExtensions.includes("slash-commands"), + isEnabled: (disabledExtensions, _flaggedExtensions) => + !disabledExtensions.includes("slash-commands"),
🧹 Nitpick comments (24)
packages/editor/src/core/types/config.ts (1)
37-47: Prefer marking DTO-like types asreadonlyand consider optionalqueryParams.
TUserDetailsandTRealtimeConfiglook fine, but these value objects are typically treated as immutable once created. Addingreadonlyto each property communicates that intent at the type level and prevents accidental mutation.-export type TUserDetails = { - color: string; - id: string; - name: string; - cookie?: string; -}; +export type TUserDetails = { + readonly color: string; + readonly id: string; + readonly name: string; + readonly cookie?: string; +}; -export type TRealtimeConfig = { - url: string; - queryParams: TWebhookConnectionQueryParams; -}; +export type TRealtimeConfig = { + readonly url: string; + /** optional: some consumers may not need extra params */ + readonly queryParams?: TWebhookConnectionQueryParams; +};A tiny improvement, but promotes safer code.
packages/editor/src/core/extensions/slash-commands/command-items-list.tsx (1)
292-294: Pass a default empty array to avoidundefinedpropagation.
coreEditorAdditionalSlashCommandOptionslikely expects arrays. Passingundefinedforces the callee to null-check each time.- disabledExtensions, - flaggedExtensions, + disabledExtensions: disabledExtensions ?? [], + flaggedExtensions: flaggedExtensions ?? [],A minor guard that reduces branching down-stream.
web/core/components/pages/version/editor.tsx (1)
35-36: Variable naming avoids shadowing but considerdocumentExtensions.Destructuring as
{ document: documentEditorExtensions }is clear, yet the intermediate name still contains “Editor”. To stay concise and avoid redundancy you could rename todocumentExtensions.Not blocking, just a readability tweak.
packages/editor/src/core/components/editors/document/page-renderer.tsx (1)
8-16: Consider in-place destructuring to remove double handling ofpropsThe component currently declares a
Propstype and then immediately re-destructures inside the body. In small renderers this extra indirection is noise and slightly harms readability.-export const PageRenderer = (props: Props) => { - const { aiHandler, bubbleMenuEnabled, displayConfig, editor, editorContainerClassName, id, tabIndex } = props; +export const PageRenderer = ({ + aiHandler, + bubbleMenuEnabled, + displayConfig, + editor, + editorContainerClassName, + id, + tabIndex, +}: Props) => {This removes one object allocation and makes the parameter list self-documenting. No functional change.
packages/editor/src/core/components/editors/read-only-editor-wrapper.tsx (1)
20-33: Provide a default forflaggedExtensionsto avoid extra undefined checks downstream
flaggedExtensionsis optional here but many extension filters expect an array. Supplying[]by default keeps call-sites simpler and eliminates the need for repeated?? []guards in hooks.- flaggedExtensions, + flaggedExtensions = [],Same default can be passed into
useReadOnlyEditor.packages/editor/src/core/components/editors/editor-wrapper.tsx (1)
29-49: Align the newflaggedExtensionsprop with the existing defaulting patternElsewhere props such as
displayConfigandeditorClassNamecarry sensible defaults. For consistency, defaultflaggedExtensionsto an empty array before forwarding touseEditor; that hook can then rely on an array type.- fileHandler, - flaggedExtensions, + fileHandler, + flaggedExtensions = [],This avoids potential
undefinedchecks inside the hook and extension factories.packages/editor/src/ce/extensions/core/read-only-extensions.ts (1)
5-8: Unused parameter indicates dead code path
CoreReadOnlyEditorAdditionalExtensionsreceivesdisabledExtensionsandflaggedExtensionsbut immediately discards them:const {} = props; return [];Either remove the parameter or implement the filtering logic; leaving it empty is misleading and may confuse future readers.
-export const CoreReadOnlyEditorAdditionalExtensions = (props: Props): Extensions => { - const {} = props; +export const CoreReadOnlyEditorAdditionalExtensions = (_: Props): Extensions => {or flesh out the extension filtering if that is still planned.
packages/editor/src/ce/extensions/slash-commands.tsx (1)
8-11: Props are accepted but never used – consider trimming API surface or implementing the logic
disabledExtensions/flaggedExtensionsare passed in, yet we immediately discard them and always return an empty array. If this is intentional, drop the parameter (or prefix it with_) and inline-return[]to avoid dead code; otherwise wire the flags into the filtering logic.-export const coreEditorAdditionalSlashCommandOptions = (props: Props): TSlashCommandAdditionalOption[] => { - const {} = props; - const options: TSlashCommandAdditionalOption[] = []; - return options; -}; +export const coreEditorAdditionalSlashCommandOptions = (): TSlashCommandAdditionalOption[] => { + return []; +};packages/editor/src/ce/extensions/core/extensions.ts (1)
7-9: Same pattern: unusedpropsargument hides missing implementation
fileHandler,disabledExtensions, and nowflaggedExtensionsare collected but ignored. Either (1) implement the extension composition here, or (2) drop the argument to keep the public contract honest.-export const CoreEditorAdditionalExtensions = (props: Props): Extensions => { - const {} = props; - return []; -}; +export const CoreEditorAdditionalExtensions = (): Extensions => { + return []; +};web/core/components/pages/editor/editor-body.tsx (1)
216-218: Ensure arrays are always defined
documentEditorExtensions.disabled/.flaggedshould default to[]to avoid passingundefinedinto the editor which may expect an array.-disabledExtensions={documentEditorExtensions.disabled} -flaggedExtensions={documentEditorExtensions.flagged} +disabledExtensions={documentEditorExtensions.disabled ?? []} +flaggedExtensions={documentEditorExtensions.flagged ?? []}packages/editor/src/core/components/editors/rich-text/read-only-editor.tsx (1)
12-20:getExtensions()creates a new array on every render – memoise to prevent unnecessary re-initialisationPassing a freshly created
extensionsarray each render can cause the underlying editor to think its extension set changed, triggering costly reconfiguration. Cache the result withuseMemo.-const getExtensions = useCallback(() => { - const extensions = RichTextReadOnlyEditorAdditionalExtensions({ - disabledExtensions, - fileHandler, - flaggedExtensions, - }); - return extensions; -}, [disabledExtensions, fileHandler, flaggedExtensions]); +const extensions = useMemo( + () => + RichTextReadOnlyEditorAdditionalExtensions({ + disabledExtensions, + fileHandler, + flaggedExtensions, + }), + [disabledExtensions, fileHandler, flaggedExtensions], +);Then pass
extensions={extensions}toReadOnlyEditorWrapper.packages/editor/src/core/hooks/use-collaborative-editor.ts (1)
19-25: DefaultflaggedExtensionsto an empty array for safety
flaggedExtensionsis optional, yet downstream helpers may call.includeson it. Defaulting it to[]in the parameter destructuring keeps the contract consistent withdisabledExtensionsand prevents accidentalTypeError: cannot read properties of undefined.- flaggedExtensions, + flaggedExtensions = [],packages/editor/src/core/hooks/use-read-only-editor.ts (1)
19-25: Provide a default forflaggedExtensionsSame rationale as other hooks—avoid undefined dereference.
- flaggedExtensions, + flaggedExtensions = [],packages/editor/src/core/components/editors/rich-text/editor.tsx (1)
22-38:useMemois a better fit thanuseCallbackhere
getExtensionsreturns a value, not a function reference consumers call later. Recompute it only when its dependencies change:- const getExtensions = useCallback(() => { + const extensions = useMemo(() => { const extensions = [ ... ]; - return extensions; - }, [dragDropEnabled, disabledExtensions, externalExtensions, fileHandler, flaggedExtensions]); + return extensions; + }, [dragDropEnabled, disabledExtensions, externalExtensions, fileHandler, flaggedExtensions]);Then pass
extensionsdirectly toEditorWrapper. This avoids an unnecessary function allocation every render.packages/editor/src/core/components/editors/lite-text/editor.tsx (2)
12-20: ConsiderflaggedExtensionswhen generating the localextensionsarray.
flaggedExtensionsis part ofILiteTextEditorProps, yet the memo completely ignores it and may therefore re-add an extension that upstream code only wants to flag (e.g. highlight, warn, etc.).
If the semantics of “flagged” require the extension still to be mounted then this is fine, otherwise you may want to honour that list the same way you do fordisabledExtensions.
12-20:externalExtensionsmutability caveat
useMemoonly re-runs when the reference ofexternalExtensionschanges. If callers mutate the array in-place (not uncommon when extensions are accumulated imperatively) the memo will stay stale.
A defensive copy in the dependency array solves this cheaply:- }, [externalExtensions, disabledExtensions, onEnterKeyPress]); + }, [externalExtensions?.length, disabledExtensions, onEnterKeyPress]);web/core/components/editor/sticky-editor/editor.tsx (1)
68-70: Potential duplicate / conflicting extension flags
"enter-key"is forcibly appended todisabledExtensions, while the same id could also be present inliteTextEditorExtensions.flagged.
Down-stream logic should clarify the precedence (disabled vs flagged). To avoid accidental duplication you can dedupe before passing:- disabledExtensions={[...liteTextEditorExtensions.disabled, "enter-key"]} + disabledExtensions={[...new Set([...liteTextEditorExtensions.disabled, "enter-key"])]}packages/editor/src/core/hooks/use-editor.ts (1)
108-111: Effect dependency on nested property can thrash renders
useEffectdepends onfileHandler.assetsUploadStatus. Because this is a non-stable primitive nested inside an object, every parent re-render that recreatesfileHandlerwill trigger the effect even ifassetsUploadStatusis unchanged.Consider lifting
assetsUploadStatusout into its own prop or memoisingfileHandler.web/ce/hooks/use-editor-flagging.ts (1)
22-34:workspaceSlugparameter is unusedThe hook signature suggests workspace-specific flagging, but the argument is ignored. Either:
- Remove the parameter from both caller & definition, or
- Implement the logic that actually varies the returned flags per workspace.
This avoids misleading future maintainers.
web/core/components/editor/lite-text-editor/lite-text-read-only-editor.tsx (1)
38-40: Guard against duplicate ids in combineddisabledExtensionsWhen merging
liteTextEditorExtensions.disabledwithadditionalDisabledExtensions, duplicates can slip in, increasing array length and uselessly re-running extension disabling logic.- disabledExtensions={[...liteTextEditorExtensions.disabled, ...(additionalDisabledExtensions ?? [])]} + disabledExtensions={[...new Set([...liteTextEditorExtensions.disabled, ...(additionalDisabledExtensions ?? [])])]}web/core/components/editor/rich-text-editor/rich-text-read-only-editor.tsx (1)
40-42: De-duplicate disabled extensions before forwarding.Mirror the improvement suggested for the writable editor to keep look-ups O(n).
- disabledExtensions={[...richTextEditorExtensions.disabled, ...(additionalDisabledExtensions ?? [])]} + disabledExtensions={[ + ...new Set([ + ...richTextEditorExtensions.disabled, + ...(additionalDisabledExtensions ?? []), + ]), + ]}packages/editor/src/core/extensions/read-only-extensions.ts (1)
38-41: Guard against undefinedflaggedExtensionsto simplify downstream checks.- const { disabledExtensions, fileHandler, flaggedExtensions, mentionHandler } = props; + const { + disabledExtensions = [], + flaggedExtensions = [], + fileHandler, + mentionHandler, + } = props;This ensures
CoreReadOnlyEditorAdditionalExtensionsnever receivesundefined.packages/editor/src/core/components/editors/document/read-only-editor.tsx (1)
25-28: DefaultflaggedExtensionsto an empty array.- flaggedExtensions, + flaggedExtensions = [],Keeps behaviour consistent with other editors and avoids extra nil-checks in
useReadOnlyEditor.packages/editor/src/core/types/hook.ts (1)
26-31: Redundantvalue/initialValueduo – clarify intent
TEditorHookPropsexposes bothvalue(picked) andinitialValue(added manually).
Having two sources of truth for the same concept often leads to stale data or divergent behaviours. Consider keeping only one of them or documenting the difference explicitly.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (37)
packages/editor/src/ce/extensions/core/extensions.ts(1 hunks)packages/editor/src/ce/extensions/core/read-only-extensions.ts(1 hunks)packages/editor/src/ce/extensions/document-extensions.tsx(1 hunks)packages/editor/src/ce/extensions/rich-text/extensions.tsx(1 hunks)packages/editor/src/ce/extensions/rich-text/read-only-extensions.tsx(1 hunks)packages/editor/src/ce/extensions/slash-commands.tsx(1 hunks)packages/editor/src/core/components/editors/document/collaborative-editor.tsx(4 hunks)packages/editor/src/core/components/editors/document/page-renderer.tsx(2 hunks)packages/editor/src/core/components/editors/document/read-only-editor.tsx(4 hunks)packages/editor/src/core/components/editors/editor-wrapper.tsx(2 hunks)packages/editor/src/core/components/editors/lite-text/editor.tsx(1 hunks)packages/editor/src/core/components/editors/lite-text/read-only-editor.tsx(1 hunks)packages/editor/src/core/components/editors/read-only-editor-wrapper.tsx(2 hunks)packages/editor/src/core/components/editors/rich-text/editor.tsx(3 hunks)packages/editor/src/core/components/editors/rich-text/read-only-editor.tsx(1 hunks)packages/editor/src/core/extensions/extensions.ts(2 hunks)packages/editor/src/core/extensions/read-only-extensions.ts(2 hunks)packages/editor/src/core/extensions/slash-commands/command-items-list.tsx(2 hunks)packages/editor/src/core/extensions/slash-commands/root.tsx(2 hunks)packages/editor/src/core/extensions/utility.ts(2 hunks)packages/editor/src/core/hooks/use-collaborative-editor.ts(2 hunks)packages/editor/src/core/hooks/use-editor.ts(4 hunks)packages/editor/src/core/hooks/use-read-only-editor.ts(3 hunks)packages/editor/src/core/types/collaboration.ts(0 hunks)packages/editor/src/core/types/config.ts(2 hunks)packages/editor/src/core/types/editor.ts(3 hunks)packages/editor/src/core/types/hook.ts(1 hunks)packages/editor/src/core/types/index.ts(1 hunks)packages/editor/src/index.ts(0 hunks)web/ce/hooks/use-editor-flagging.ts(1 hunks)web/core/components/editor/lite-text-editor/lite-text-editor.tsx(4 hunks)web/core/components/editor/lite-text-editor/lite-text-read-only-editor.tsx(3 hunks)web/core/components/editor/rich-text-editor/rich-text-editor.tsx(4 hunks)web/core/components/editor/rich-text-editor/rich-text-read-only-editor.tsx(3 hunks)web/core/components/editor/sticky-editor/editor.tsx(4 hunks)web/core/components/pages/editor/editor-body.tsx(2 hunks)web/core/components/pages/version/editor.tsx(2 hunks)
💤 Files with no reviewable changes (2)
- packages/editor/src/core/types/collaboration.ts
- packages/editor/src/index.ts
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Analyze (javascript)
🔇 Additional comments (9)
packages/editor/src/core/types/index.ts (1)
7-7: Barrel export looks good.Re-exporting
./hookhere keeps the public surface consistent with the new typing modules. No issues spotted.packages/editor/src/core/components/editors/lite-text/read-only-editor.tsx (1)
5-8: Prop type update looks correct.Switching to
ILiteTextReadOnlyEditorPropsaligns with the new consolidated prop hierarchy. Implementation and ref-forwarding remain intact.web/core/components/pages/version/editor.tsx (1)
102-104: Ensure component supports empty arrays to prevent uncontrolled behaviour.
disabledExtensions/flaggedExtensionsmay beundefinedon first render. Verify thatDocumentReadOnlyEditorWithReftreatsundefinedthe same as[]to avoid runtime checks inside the editor wrapper. Initialising with an empty array here would be safer:- disabledExtensions={documentEditorExtensions.disabled} - flaggedExtensions={documentEditorExtensions.flagged} + disabledExtensions={documentEditorExtensions.disabled ?? []} + flaggedExtensions={documentEditorExtensions.flagged ?? []}packages/editor/src/core/extensions/slash-commands/root.tsx (1)
109-111:TExtensionPropsnow mirrors editor props – good consolidationSwitching to
Pick<IEditorProps, "disabledExtensions" | "flaggedExtensions">eliminates duplication and centralises the contract. ✔️web/core/components/pages/editor/editor-body.tsx (1)
84-85: Potential undefined-destructuring when hook returnsnull | undefined
IfuseEditorFlaggingever returnsundefined(e.g. before data is loaded), destructuring will throw. Consider safe-defaulting.-const { document: documentEditorExtensions } = useEditorFlagging(workspaceSlug); +const { document: documentEditorExtensions } = + useEditorFlagging(workspaceSlug) ?? { document: { disabled: [], flagged: [] } };packages/editor/src/core/extensions/utility.ts (1)
11-28: Type refactor looks good
LeveragingPick<IEditorProps, "disabledExtensions">keeps the prop contract consistent with the new base-types approach without altering run-time behaviour.web/core/components/editor/sticky-editor/editor.tsx (1)
51-52: UnusedworkspaceSlugparameter insideuseEditorFlagging
useEditorFlaggingis invoked withworkspaceSlug?.toString()but, judging from its implementation, the hook ignores the argument altogether.
Either remove the parameter here (and from the hook signature) or implement workspace-specific flagging inside the hook.packages/editor/src/core/components/editors/document/collaborative-editor.tsx (1)
30-31: Provide a safe default forflaggedExtensions.
flaggedExtensionsis optional in the type, but no default ([]) is applied.
If a downstream consumer assumes an array, anundefinedmay leak and trigger runtime checks or spread-operator failures.- flaggedExtensions, + flaggedExtensions = [],(Apply the same default in the
useCollaborativeEditorargument list if it expects a non-nullable array.)web/core/components/editor/lite-text-editor/lite-text-editor.tsx (1)
67-68: Sanity-checkworkspaceSlugbefore coercion
useEditorFlagging(workspaceSlug?.toString())will pass the literal string"undefined"whenworkspaceSlugisundefined, which is easy to overlook in downstream logic.
Defensively bail out or default to""instead.- const { liteText: liteTextEditorExtensions } = useEditorFlagging(workspaceSlug?.toString()); + const { liteText: liteTextEditorExtensions } = useEditorFlagging( + workspaceSlug ? workspaceSlug.toString() : "" + );
packages/editor/src/core/extensions/slash-commands/command-items-list.tsx
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
space/core/components/editor/rich-text-read-only-editor.tsx (1)
22-30: Avoid recreating default arrays on every render
disabledExtensions={disabledExtensions ?? []}andflaggedExtensions={flaggedExtensions ?? []}allocate a brand-new empty array on every render when the prop isundefined.
If the underlying editor implementation relies on referential equality (e.g.,useEffectdeps), this can trigger needless re-runs / re-renders.-export const RichTextReadOnlyEditor = React.forwardRef<EditorReadOnlyRefApi, RichTextReadOnlyEditorWrapperProps>( - ({ anchor, workspaceId, disabledExtensions, flaggedExtensions, ...props }, ref) => { +const EMPTY_ARR: readonly [] = []; + +export const RichTextReadOnlyEditor = React.forwardRef< + EditorReadOnlyRefApi, + RichTextReadOnlyEditorWrapperProps +>(({ anchor, workspaceId, disabledExtensions, flaggedExtensions, ...props }, ref) => { ... - disabledExtensions={disabledExtensions ?? []} + disabledExtensions={disabledExtensions ?? EMPTY_ARR} ... - flaggedExtensions={flaggedExtensions ?? []} + flaggedExtensions={flaggedExtensions ?? EMPTY_ARR}space/core/components/editor/rich-text-editor.tsx (1)
23-41: Reuse a stable empty array for defaulted propsSame comment as for the read-only editor: defaulting to
[]inline causes a new reference each render. Prefer a frozen constant to avoid unnecessary downstream effects.- const { anchor, containerClassName, uploadFile, workspaceId, disabledExtensions, flaggedExtensions, ...rest } = props; + const { + anchor, + containerClassName, + uploadFile, + workspaceId, + disabledExtensions, + flaggedExtensions, + ...rest + } = props; + + const EMPTY_ARR: readonly [] = []; ... - disabledExtensions={disabledExtensions ?? []} + disabledExtensions={disabledExtensions ?? EMPTY_ARR} ... - flaggedExtensions={flaggedExtensions ?? []} + flaggedExtensions={flaggedExtensions ?? EMPTY_ARR}space/core/components/editor/lite-text-read-only-editor.tsx (1)
22-30: Avoid creating a new empty array on every render
disabledExtensions={disabledExtensions ?? []}and likewise forflaggedExtensionsproduce a fresh array each render when the prop isundefined, defeating referential equality and triggering unnecessary downstream re-renders.- disabledExtensions={disabledExtensions ?? []} - flaggedExtensions={flaggedExtensions ?? []} + disabledExtensions={disabledExtensions ?? EMPTY_EXT_ARR} + flaggedExtensions={flaggedExtensions ?? EMPTY_EXT_ARR}Add once, outside the component:
const EMPTY_EXT_ARR: never[] = [];space/core/components/editor/lite-text-editor.tsx (1)
47-49: Reuse a stable empty array constantSame concern as in the read-only wrapper: passing a brand-new
[]each render hampers memoisation. Consider:- disabledExtensions={disabledExtensions ?? []} - flaggedExtensions={flaggedExtensions ?? []} + disabledExtensions={disabledExtensions ?? EMPTY_EXT_ARR} + flaggedExtensions={flaggedExtensions ?? EMPTY_EXT_ARR}and declare
const EMPTY_EXT_ARR: never[] = [];once per file (or share across wrappers).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
packages/editor/src/ce/extensions/core/extensions.ts(1 hunks)packages/editor/src/ce/extensions/core/read-only-extensions.ts(1 hunks)packages/editor/src/core/components/editors/document/collaborative-editor.tsx(4 hunks)packages/editor/src/core/types/editor.ts(3 hunks)packages/editor/src/core/types/hook.ts(1 hunks)space/core/components/editor/lite-text-editor.tsx(4 hunks)space/core/components/editor/lite-text-read-only-editor.tsx(2 hunks)space/core/components/editor/rich-text-editor.tsx(3 hunks)space/core/components/editor/rich-text-read-only-editor.tsx(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- packages/editor/src/core/types/hook.ts
- packages/editor/src/core/components/editors/document/collaborative-editor.tsx
- packages/editor/src/ce/extensions/core/read-only-extensions.ts
- packages/editor/src/ce/extensions/core/extensions.ts
- packages/editor/src/core/types/editor.ts
🔇 Additional comments (7)
space/core/components/editor/rich-text-read-only-editor.tsx (1)
13-16: Type-level change looks correctSwitching to
IRichTextReadOnlyEditorPropsand making bothdisabledExtensions&flaggedExtensionsoptional viaMakeOptionalis consistent with the wider refactor. No issues spotted.space/core/components/editor/rich-text-editor.tsx (1)
12-16: Wrapper prop typing aligns with base editor propsThe updated
RichTextEditorWrapperPropscorrectly exposes the newflaggedExtensionswhile still omitting handler props. Looks good.space/core/components/editor/lite-text-read-only-editor.tsx (2)
3-3: Correct prop-type importSwitching to
ILiteTextReadOnlyEditorPropskeeps the wrapper in sync with the upstream refactor.
13-16: ```shell
#!/bin/bashShow where ILiteTextReadOnlyEditorProps is imported in the wrapper file
rg -n 'ILiteTextReadOnlyEditorProps' -C2 space/core/components/editor/lite-text-read-only-editor.tsx
Locate the definition of ILiteTextReadOnlyEditorProps to inspect if disabledExtensions/flaggedExtensions are already optional
rg -n 'interface ILiteTextReadOnlyEditorProps' -C6 .
</details> <details> <summary>space/core/components/editor/lite-text-editor.tsx (3)</summary> `3-3`: **Import update looks good** Aligns the wrapper with the new prop type introduced upstream. --- `13-16`: **Re-assess `MakeOptional` wrapper** As with the read-only variant, double-optionalising props may now be superfluous and can mask future type changes. --- `32-34`: **Destructuring change correctly surfaces `flaggedExtensions`** Prop now bubbles through the wrapper – good catch. </details> </blockquote></details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
* refactor: editor props structure * chore: add missing prop * fix: space app build * chore: export ce types
3 participants
Description
This PR refactors the types structure of the editor package-
Type of Change
Summary by CodeRabbit
New Features
Refactor
Chores