[PE-239] chore: add strictNullCheck flag to the editor package#6439
[PE-239] chore: add strictNullCheck flag to the editor package#6439sriramveeraghanta merged 4 commits intopreviewfrom
strictNullCheck flag to the editor package#6439Conversation
|
Warning Rate limit exceeded@aaryan610 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 12 minutes and 25 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)
WalkthroughThis pull request introduces several changes across various files in the editor package. Key modifications include the addition of the Changes
Suggested labels
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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: 0
🧹 Nitpick comments (1)
packages/editor/tsconfig.json (1)
15-15: Consider enabling full strict mode.While
strictNullChecksis a good start, consider enabling the fullstrictflag to catch more potential issues. This would enable additional checks likestrictFunctionTypes,strictBindCallApply, etc.You could enhance type safety further with:
{ "compilerOptions": { + "strict": true, - "strictNullChecks": true, ... } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/editor/tsconfig.json(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Analyze (javascript)
- GitHub Check: Analyze (python)
🔇 Additional comments (1)
packages/editor/tsconfig.json (1)
15-15: LGTM! But let's verify the impact.Adding
strictNullChecksis a good step towards better type safety. However, this change might surface existing null-handling issues in the codebase.Let's check for potential null-handling issues that might need fixing:
✅ Verification successful
Enabling
strictNullChecksis safe to proceed with.The codebase already follows strict null-checking patterns with extensive use of optional chaining, explicit null guards, and proper null-state handling throughout the editor components.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Look for patterns that might need attention with strictNullChecks # Search for non-null assertions that might need proper null checks echo "Checking for non-null assertions (!.) that might need proper null checks..." rg "!." packages/editor/src # Search for potential null access without checks echo "Checking for potential null access patterns..." ast-grep --pattern 'const $x = $y.$z' packages/editor/src # Look for optional chaining that might indicate existing null-awareness echo "Checking existing null-aware code..." rg "\?\." packages/editor/srcLength of output: 53415
There was a problem hiding this comment.
Actionable comments posted: 3
🔭 Outside diff range comments (4)
packages/editor/src/core/components/editors/document/read-only-editor.tsx (1)
23-23: 🛠️ Refactor suggestionType
embedHandlerproperly instead of usingany.With
strictNullChecksenabled, usinganytype bypasses type checking. Consider creating a proper interface forembedHandler.- embedHandler: any; + interface IEmbedHandler { + issue?: { + widgetCallback: (props: any) => JSX.Element; // Replace 'any' with proper props type + }; + } + embedHandler: IEmbedHandler;packages/editor/src/core/components/editors/document/collaborative-editor.tsx (1)
25-25: 🛠️ Refactor suggestionMaintain consistent typing with
DocumentReadOnlyEditor.For consistency and type safety, use the same interface for
embedHandleras suggested inDocumentReadOnlyEditor.- embedHandler: any; + interface IEmbedHandler { + issue?: { + widgetCallback: (props: any) => JSX.Element; // Replace 'any' with proper props type + }; + } + embedHandler: IEmbedHandler;packages/editor/src/core/extensions/custom-color.ts (1)
96-108:⚠️ Potential issueFix incorrect
getAttrslogic and address type error.
- The
getAttrsfunctions currently returnnullwhen attributes exist, which seems incorrect. They should return an object with attributes instead.- The
@ts-expect-errorcomment suggests underlying type issues that should be addressed properly.- // @ts-expect-error types are incorrect parseHTML() { return [ { tag: "span", - getAttrs: (node) => node.getAttribute("data-text-color") && null, + getAttrs: (node) => { + const color = node.getAttribute("data-text-color"); + return color ? { color } : false; + }, }, { tag: "span", - getAttrs: (node) => node.getAttribute("data-background-color") && null, + getAttrs: (node) => { + const backgroundColor = node.getAttribute("data-background-color"); + return backgroundColor ? { backgroundColor } : false; + }, }, ]; },packages/editor/src/core/components/menus/ai-menu.tsx (1)
26-38: 💡 Verification agent🛠️ Refactor suggestion
🧩 Analysis chain
Improve null safety and type handling for Tippy initialization.
Given that this PR aims to add
strictNullCheck, there are several areas that need improvement:
- The
@ts-expect-errorsuppression should be avoided. Instead, properly type the Tippy instance.- Add null checks for the
.frame-rendererselector andmenuRef.currentaccess.Consider this safer implementation:
- // @ts-expect-error - Tippy types are incorrect + const frameRenderer = document.querySelector(".frame-renderer"); + if (!frameRenderer) { + console.warn("Frame renderer element not found"); + return; + } + popup.current = tippy(document.body, { getReferenceClientRect: null, content: menuRef.current, - appendTo: () => document.querySelector(".frame-renderer"), + appendTo: () => frameRenderer, trigger: "manual", interactive: true, arrow: false, placement: "bottom-start", animation: "shift-away", hideOnClick: true, - onShown: () => menuRef.current?.focus(), + onShown: () => { + if (menuRef.current) { + menuRef.current.focus(); + } + }, });Let's verify if there's a known issue with Tippy.js types:
🌐 Web query:
Are there known type definition issues with Tippy.js in TypeScript?💡 Result:
Yes, there have been several reported TypeScript definition issues with Tippy.js, primarily related to prop typing completeness and dependency conflicts. Here are the key known issues:
1. Missing Prop Definitions
- The
offsetprop caused TypeScript errors in headless Tippy components due to incomplete type definitions[2]. Users temporarily resolved this with@ts-ignorecomments before identifying the root cause as outdated TypeScript versions[2].- Solution: Update TypeScript to v3.8+ and ensure
@tippyjs/react/tippy.jspackages are synchronized[2].2. Dependency Version Conflicts
- Issues arise when using newer TypeScript versions (4.9+) with older
@popperjs/coredependencies[8]. Errors like "This type parameter might need an extends constraint" appear due to incompatible type definitions in dependencies[8].- Solution: Update both
tippy.jsand@popperjs/coreto their latest versions.3. Plugin Typing Issues
- Adding plugins or native props sometimes breaks TypeScript compatibility if plugin types aren't properly declared[9]. For example:
tippy(element, { // Throws "pluginName does not exist in type Props" pluginName: {} });- Workaround: Use module augmentation or cast props with
as unknown[9].4. React Wrapper Limitations
While not shown in these results, the official React wrapper (
@tippyjs/react) generally provides better TS support than manual implementations[3][5]. However, version mismatches between coretippy.jsand wrapper packages can still cause typing issues[2].For developers encountering these issues:
- Verify all package versions match (TypeScript ≥4.x,
tippy.js@6.x,@popperjs/core@2.x)- Use the React wrapper for declarative components
- Check open GitHub issues for workarounds on specific props[2][8][9]
These issues highlight the importance of keeping dependencies synchronized when using Tippy.js in TypeScript projects[2][8][9].
Citations:
- 1: https://atomiks.github.io/tippyjs/v6/getting-started/
- 2: atomiks/tippyjs-react#206
- 3: https://atomiks.github.io/tippyjs/
- 4: http://twinery.org/questions/4943/using-tippy-js-in-harlowe
- 5: https://www.npmjs.com/package/tippy.js/v/6.3.7
- 6: https://atomiks.github.io/tippyjs/v5/faq/
- 7: atomiks/tippyjs-react#11
- 8: atomiks/tippyjs-react#374
- 9: atomiks/tippyjs#631
- 10: http://js.cytoscape.org
Improve Tippy.js initialization: enforce null checks and remove error suppression
Since this PR is enabling
strictNullCheck, the Tippy.js initialization must be updated to safely handle possible null values and avoid relying on error suppression. Note that while Tippy.js has known TypeScript definition issues, it’s best to update dependency versions rather than bypass type safety.
- Remove the
@ts-expect-errorsuppression and properly type the Tippy instance.- Retrieve the
.frame-rendererelement with a null check and warn/exit if not available.- Add a null check for
menuRef.currentin theonShowncallback to prevent runtime errors.- // @ts-expect-error - Tippy types are incorrect + const frameRenderer = document.querySelector(".frame-renderer"); + if (!frameRenderer) { + console.warn("Frame renderer element not found"); + return; + } + popup.current = tippy(document.body, { getReferenceClientRect: null, content: menuRef.current, - appendTo: () => document.querySelector(".frame-renderer"), + appendTo: () => frameRenderer, trigger: "manual", interactive: true, arrow: false, placement: "bottom-start", animation: "shift-away", hideOnClick: true, - onShown: () => menuRef.current?.focus(), + onShown: () => { + if (menuRef.current) { + menuRef.current.focus(); + } + }, });Note: Known issues with Tippy.js’ TypeScript definitions (as confirmed by community reports) highlight the importance of synchronizing package versions (e.g., updating
tippy.jsand@popperjs/core) to improve type safety in the long term.
🧹 Nitpick comments (8)
packages/editor/src/core/components/menus/menu-items.ts (1)
243-247: LGTM! Consider enhancing with type guard.The null-safety implementation is solid and consistent with other menu items. Consider enhancing type safety further with a type guard:
- command: (props) => { - if (!props) return; - setTextAlign(props.alignment, editor); - }, + command: (props) => { + if (!props?.alignment) return; + setTextAlign(props.alignment, editor); + },This would ensure
alignmentis defined before callingsetTextAlign.packages/editor/src/core/extensions/custom-image/components/image-uploader.tsx (1)
41-41: Consider handling cleanup before early return.The early return when
imageEntityIdis null could skip important cleanup operations. Consider settingisUploadedto false before returning to maintain consistent state.- if (!imageEntityId) return; + if (!imageEntityId) { + setIsUploaded(false); + return; + }packages/editor/src/core/extensions/custom-image/components/toolbar/full-screen.tsx (2)
36-58: Improve null handling in setImageRef callback.The callback could be simplified and made more robust by using early returns and optional chaining.
const setImageRef = useCallback( (node: HTMLImageElement | null) => { - if (!node || !isFullScreenEnabled) return; + if (!isFullScreenEnabled) return; + if (!node) { + imgRef.current = null; + return; + } imgRef.current = node;
122-137: Optimize handleMouseMove for performance.Consider memoizing the calculations in
handleMouseMoveto prevent unnecessary recalculations during rapid mouse movements.const handleMouseMove = useCallback( (e: MouseEvent) => { if (!isDragging || !imgRef.current) return; + const scale = 1 / magnification; const dx = e.clientX - dragStart.current.x; const dy = e.clientY - dragStart.current.y; - const scaledDx = dx / magnification; - const scaledDy = dy / magnification; + const scaledDx = dx * scale; + const scaledDy = dy * scale; imgRef.current.style.left = `${dragOffset.current.x + scaledDx}px`; imgRef.current.style.top = `${dragOffset.current.y + scaledDy}px`; }, [isDragging, magnification] );packages/editor/src/core/extensions/custom-image/components/image-block.tsx (1)
282-288: Improve aspectRatio handling in ImageToolbarRoot.Using a fallback of 1 for null aspectRatio could lead to unexpected image distortions. Consider calculating the actual aspect ratio or using a more appropriate fallback.
image={{ src: resolvedImageSrc, - aspectRatio: size.aspectRatio === null ? 1 : size.aspectRatio, + aspectRatio: size.aspectRatio ?? (imageRef.current + ? imageRef.current.naturalWidth / imageRef.current.naturalHeight + : 1), height: size.height, width: size.width, }}packages/editor/src/core/hooks/use-editor.ts (3)
133-134: Add complete null checks for the state and selection chain.While the optional chaining on
editoris good, consider adding complete null checks for the entire chain to be fully compliant withstrictNullChecks.-const resolvedPos = pos ?? editor?.state.selection.from; +const resolvedPos = pos ?? editor?.state?.selection?.from;
137-137: Add complete null checks for cursor position access.Similar to the previous case, add complete null checks for the entire chain.
-getCurrentCursorPosition: () => editor?.state.selection.from, +getCurrentCursorPosition: () => editor?.state?.selection?.from,
203-206: Ensure consistent null handling across the method.While the optional chaining for
getJSON()is good, consider applying the same pattern to other similar calls in the method for consistency.-const documentBinary = provider?.document ? Y.encodeStateAsUpdate(provider?.document) : null; -const documentHTML = editor?.getHTML() ?? "<p></p>"; -const documentJSON = editor?.getJSON() ?? null; +const documentBinary = provider?.document ? Y.encodeStateAsUpdate(provider?.document) : null; +const documentHTML = editor?.getHTML?.() ?? "<p></p>"; +const documentJSON = editor?.getJSON?.() ?? null;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
packages/editor/src/core/components/editors/document/collaborative-editor.tsx(2 hunks)packages/editor/src/core/components/editors/document/read-only-editor.tsx(2 hunks)packages/editor/src/core/components/menus/ai-menu.tsx(1 hunks)packages/editor/src/core/components/menus/block-menu.tsx(1 hunks)packages/editor/src/core/components/menus/menu-items.ts(1 hunks)packages/editor/src/core/extensions/custom-color.ts(1 hunks)packages/editor/src/core/extensions/custom-image/components/image-block.tsx(6 hunks)packages/editor/src/core/extensions/custom-image/components/image-uploader.tsx(4 hunks)packages/editor/src/core/extensions/custom-image/components/toolbar/full-screen.tsx(1 hunks)packages/editor/src/core/extensions/extensions.tsx(2 hunks)packages/editor/src/core/extensions/slash-commands/root.tsx(1 hunks)packages/editor/src/core/extensions/typography/index.ts(2 hunks)packages/editor/src/core/hooks/use-editor.ts(2 hunks)packages/editor/src/core/hooks/use-read-only-editor.ts(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- packages/editor/src/core/extensions/slash-commands/root.tsx
- packages/editor/src/core/components/menus/block-menu.tsx
🔇 Additional comments (10)
packages/editor/src/core/extensions/extensions.tsx (2)
37-38: LGTM! Proper type imports added.Moving the type imports to a proper import statement improves type safety and aligns with the
strictNullCheckflag implementation.
151-151: LGTM! Explicit string return added for type safety.The change from implicit
undefinedto explicit empty string return improves type safety and maintains consistency with other return statements in the placeholder configuration.packages/editor/src/core/components/editors/document/read-only-editor.tsx (1)
45-45: LGTM! Type safety improvement.The explicit typing of
extensionsasExtensionsenhances type safety.packages/editor/src/core/components/editors/document/collaborative-editor.tsx (1)
38-38: LGTM! Type safety improvement.The explicit typing of
extensionsasExtensionsenhances type safety.packages/editor/src/core/extensions/typography/index.ts (1)
29-29: LGTM! Type safety improvement.The explicit typing of
rulesasInputRule[]ensures type safety when adding input rules.packages/editor/src/core/components/menus/menu-items.ts (2)
221-225: LGTM! Robust null-safety implementation.The changes effectively handle potential undefined values using optional chaining and null checks, aligning with the new
strictNullChecksrequirements.
232-236: LGTM! Consistent null-safety pattern.The changes mirror the robust null-safety implementation in TextColorItem, maintaining consistency across similar components.
packages/editor/src/core/hooks/use-editor.ts (3)
131-131: LGTM! Proper null check added.The optional chaining on
editor?.commands.blur()correctly handles cases where the editor instance might be null.
145-148: LGTM! Proper null checks for editor and selection.The optional chaining correctly verifies both the editor instance and selection existence before proceeding.
1-290:Details
❓ Verification inconclusive
Consider reviewing similar patterns throughout the file.
While the current changes add necessary null checks, there are several other similar patterns in the file that might need the same treatment for full
strictNullCheckscompliance. Consider reviewing:
- Editor method calls in callbacks (e.g.,
onUpdate,onTransaction)- Storage access patterns (e.g.,
editor.storage.imageComponent)- Chain method calls (e.g.,
editor.chain())This would ensure consistent null handling throughout the codebase.
🏁 Script executed:
#!/bin/bash # Search for potential spots that might need null checks rg -A 2 "editor\." packages/editor/src/core/hooks/use-editor.tsLength of output: 1592
Action: Review and verify consistent null safety in editor method usages.
The recent changes add appropriate null checks in several spots, and our grep search confirms that most editor accesses (e.g., storage access, chain calls, and callbacks in onUpdate/onTransaction) include checks or use optional chaining. However, similar patterns elsewhere in the file—such as in the onUpdate callback (e.g., using the destructured editor), in storage access (
editor.storage.imageComponent), and in direct chain calls likeeditor.chain()—could benefit from a uniform review to ensure fullstrictNullCheckscompliance.Recommendations:
- Callbacks: Verify that callbacks (like onUpdate and onTransaction) consistently handle cases where the editor might be null or already destroyed.
- Storage Access: Ensure all accesses to properties such as
editor.storage.imageComponentare safeguarded, potentially using optional chaining.- Chain Calls: Confirm that every call to
editor.chain()is preceded by an appropriate null check, even if the surrounding code appears to assume an available editor.Please perform a manual review of these similar patterns to confirm that the null-checking strategy is applied consistently throughout the file.
packages/editor/src/core/extensions/custom-image/components/image-uploader.tsx
Show resolved
Hide resolved
packages/editor/src/core/extensions/custom-image/components/image-block.tsx
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
packages/editor/src/core/extensions/custom-color.ts (1)
109-121:⚠️ Potential issueFix incorrect attribute parsing logic and add proper typing.
The
getAttrsfunctions in theparseHTMLmethod have incorrect logic:
- They return
nullwhen attributes exist, which would prevent the attributes from being parsed- The implementation lacks proper TypeScript typing for the
nodeparameterApply this diff to fix the issues:
- // @ts-expect-error types are incorrect parseHTML() { return [ { tag: "span", - getAttrs: (node) => node.getAttribute("data-text-color") && null, + getAttrs: (node: HTMLElement) => { + const color = node.getAttribute("data-text-color"); + return color ? { color } : false; + }, }, { tag: "span", - getAttrs: (node) => node.getAttribute("data-background-color") && null, + getAttrs: (node: HTMLElement) => { + const backgroundColor = node.getAttribute("data-background-color"); + return backgroundColor ? { backgroundColor } : false; + }, }, ]; },
🧹 Nitpick comments (1)
packages/editor/src/core/extensions/custom-color.ts (1)
45-94: Enhance type safety for attributes.With
strictNullChecksenabled, we should be more explicit about our types:Apply these type improvements:
addAttributes() { return { color: { - default: null, + default: null as string | null, parseHTML: (element: HTMLElement) => element.getAttribute("data-text-color"), - renderHTML: (attributes: { color: string }) => { + renderHTML: (attributes: { color: string | null }) => { const { color } = attributes; // ... rest of the implementation }, }, backgroundColor: { - default: null, + default: null as string | null, parseHTML: (element: HTMLElement) => element.getAttribute("data-background-color"), - renderHTML: (attributes: { backgroundColor: string }) => { + renderHTML: (attributes: { backgroundColor: string | null }) => { const { backgroundColor } = attributes; // ... rest of the implementation }, }, }; },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
packages/editor/src/core/components/editors/document/collaborative-editor.tsx(2 hunks)packages/editor/src/core/components/editors/document/read-only-editor.tsx(2 hunks)packages/editor/src/core/components/menus/menu-items.ts(1 hunks)packages/editor/src/core/extensions/custom-color.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/editor/src/core/components/editors/document/collaborative-editor.tsx
- packages/editor/src/core/components/editors/document/read-only-editor.tsx
- packages/editor/src/core/components/menus/menu-items.ts
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Analyze (javascript)
* chore: add strictNullCheck flag * fix: types and errors * chore: update error handling
3 participants
Description
This PR adds the
strictNullCheckflag to the editor packagetsconfigfor better error handling.Type of Change
Summary by CodeRabbit
New Features
Bug Fixes
Chores
getDocumentInfomethod for better readability.