Repository navigation
fix: Stabilize block document renderer props to reduce rerenders - #761
Merged
Merged
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughThree files in the documents package are updated to improve React rendering stability. ChangesRender stability: memoized boundaries and stable callbacks
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
ilyabo
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/documents/src/BlockDocumentEditor/node-views/BlockDocumentStatefulBlockNodeView.tsx (1)
130-134:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPreserve the read-only guard in the new stateful callbacks.
The chart callbacks block writes when
readOnlyis true, but these new title/caption handlers always callupdateAttributesRef.current. A renderer that invokes the callback despitereadOnlycan still mutate the document.🛡️ Proposed read-only guard
const wrapperRef = useRef<HTMLDivElement | null>(null); const updateAttributesRef = useRef(updateAttributes); const hideScrollHintTimeoutRef = useRef<number | undefined>(undefined); const scrollHintTargetRef = useRef<HTMLElement | null>(null); const resizeCleanupRef = useRef<(() => void) | null>(null); const {documentId, readOnly} = useBlockDocumentEditorContext(); + const readOnlyRef = useRef(readOnly);useEffect(() => { updateAttributesRef.current = updateAttributes; - }, [updateAttributes]); + readOnlyRef.current = readOnly; + }, [readOnly, updateAttributes]);const handleTitleChange = useCallback((nextTitle: string | undefined) => { + if (readOnlyRef.current) return; updateAttributesRef.current({title: nextTitle || undefined}); }, []); const handleCaptionChange = useCallback((nextCaption: string | undefined) => { + if (readOnlyRef.current) return; updateAttributesRef.current({caption: nextCaption}); }, []);Also applies to: 168-170, 329-335
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/documents/src/BlockDocumentEditor/node-views/BlockDocumentStatefulBlockNodeView.tsx` around lines 130 - 134, The new title and caption callbacks are missing a read-only guard that prevents mutations when the document is read-only. Add a check at the beginning of these callbacks (which call updateAttributesRef.current) to ensure they only execute when readOnly is false, using the readOnly variable already destructured from useBlockDocumentEditorContext. Apply this same guard pattern to all callback handlers mentioned in the comment at lines 168-170 and 329-335 to ensure consistent protection across all mutation operations.
🧹 Nitpick comments (4)
packages/documents/src/BlockDocumentEditor/node-views/BlockDocumentChartNodeView.tsx (2)
36-38: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winAdd TSDoc for the exported node view component.
This exported component is part of the touched TSX surface and should have a short API-level description.
♻️ Proposed documentation addition
+/** Renders a chart block as a Tiptap node view inside the block document editor. */ export const BlockDocumentChartNodeView: FC< BlockDocumentChartNodeViewProps > = ({node, selected, updateAttributes}) => {As per coding guidelines,
**/*.{ts,tsx}: “Add tsdoc to exported types and functions.”🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/documents/src/BlockDocumentEditor/node-views/BlockDocumentChartNodeView.tsx` around lines 36 - 38, Add a TSDoc comment block above the BlockDocumentChartNodeView component definition that provides a short, API-level description of what this node view component does. The TSDoc comment should start with /** and include a brief summary of the component's purpose and its role in the document editing context. This aligns with the coding guidelines requiring TSDoc for all exported types and functions in TypeScript files.Source: Coding guidelines
25-32: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winHoist the empty config fallback so the memo boundary can stay effective.
Line 48 creates a new
{}wheneverattrs.configis absent, so Line 86 passes a newconfigidentity intoChartRendererBoundaryon every parent render. That defeats the new memoized boundary for unconfigured chart blocks.♻️ Proposed stable fallback
type ChartRendererBoundaryProps = BlockDocumentChartRendererProps & { Renderer: BlockDocumentChartRenderer; }; +const EMPTY_CHART_CONFIG: Record<string, never> = {}; + const ChartRendererBoundary = memo( ({Renderer, ...props}: ChartRendererBoundaryProps) => createElement(Renderer, props), );- const config = attrs.config ?? {}; + const config = attrs.config ?? EMPTY_CHART_CONFIG;Verify with a render-count/profiler case for a chart block without stored
config; unrelated parent rerenders should not rerender the boundary after this change.Also applies to: 48-48, 81-87
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/documents/src/BlockDocumentEditor/node-views/BlockDocumentChartNodeView.tsx` around lines 25 - 32, The memo optimization of ChartRendererBoundary is ineffective because a new empty object is created on line 48 whenever attrs.config is absent, causing the config prop to have a new identity on each parent render. Extract the empty object fallback to a module-level constant so that the same reference is reused across renders when config is missing. This stable reference will allow the memo boundary to properly prevent unnecessary rerenders of ChartRendererBoundary when the parent rerenders but the config value hasn't actually changed.packages/documents/src/BlockDocumentChartRendererContext.tsx (1)
31-38: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winAdd TSDoc for the touched exported provider surface.
BlockDocumentChartRendererProviderPropsandBlockDocumentChartRendererProviderare exported, and this change updates provider behavior.♻️ Proposed documentation addition
+/** Props for {`@link` BlockDocumentChartRendererProvider}. */ export type BlockDocumentChartRendererProviderProps = PropsWithChildren<{ renderer?: BlockDocumentChartRenderer; }>; +/** Provides the chart renderer used by block document chart node views. */ export const BlockDocumentChartRendererProvider: FC< BlockDocumentChartRendererProviderProps > = ({renderer, children}) => {As per coding guidelines,
**/*.{ts,tsx}: “Add tsdoc to exported types and functions.”🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/documents/src/BlockDocumentChartRendererContext.tsx` around lines 31 - 38, Add TSDoc documentation comments to the exported type and function in this file. Add a TSDoc comment block above the BlockDocumentChartRendererProviderProps type definition to describe its purpose and properties, and add another TSDoc comment block above the BlockDocumentChartRendererProvider function to document its purpose, parameters, and what it provides via context. Follow the coding guidelines that require tsdoc to be added to exported types and functions in TypeScript files.Source: Coding guidelines
packages/documents/src/BlockDocumentEditor/node-views/BlockDocumentStatefulBlockNodeView.tsx (1)
126-128: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winAdd TSDoc for the exported stateful node view component.
This exported component is part of the touched TSX surface and should have a short API-level description.
♻️ Proposed documentation addition
+/** Renders a stateful block as a Tiptap node view inside the block document editor. */ export const BlockDocumentStatefulBlockNodeView: FC< BlockDocumentStatefulBlockNodeViewProps > = ({node, selected, updateAttributes}) => {As per coding guidelines,
**/*.{ts,tsx}: “Add tsdoc to exported types and functions.”🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/documents/src/BlockDocumentEditor/node-views/BlockDocumentStatefulBlockNodeView.tsx` around lines 126 - 128, The exported component BlockDocumentStatefulBlockNodeView is missing TSDoc documentation at the component declaration level. Add a TSDoc comment block directly above the component declaration that provides a brief, API-level description of what this stateful block node view component does and its purpose. The documentation should follow the coding guideline that requires tsdoc for all exported types and functions in typescript files.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In
`@packages/documents/src/BlockDocumentEditor/node-views/BlockDocumentStatefulBlockNodeView.tsx`:
- Around line 130-134: The new title and caption callbacks are missing a
read-only guard that prevents mutations when the document is read-only. Add a
check at the beginning of these callbacks (which call
updateAttributesRef.current) to ensure they only execute when readOnly is false,
using the readOnly variable already destructured from
useBlockDocumentEditorContext. Apply this same guard pattern to all callback
handlers mentioned in the comment at lines 168-170 and 329-335 to ensure
consistent protection across all mutation operations.
---
Nitpick comments:
In `@packages/documents/src/BlockDocumentChartRendererContext.tsx`:
- Around line 31-38: Add TSDoc documentation comments to the exported type and
function in this file. Add a TSDoc comment block above the
BlockDocumentChartRendererProviderProps type definition to describe its purpose
and properties, and add another TSDoc comment block above the
BlockDocumentChartRendererProvider function to document its purpose, parameters,
and what it provides via context. Follow the coding guidelines that require
tsdoc to be added to exported types and functions in TypeScript files.
In
`@packages/documents/src/BlockDocumentEditor/node-views/BlockDocumentChartNodeView.tsx`:
- Around line 36-38: Add a TSDoc comment block above the
BlockDocumentChartNodeView component definition that provides a short, API-level
description of what this node view component does. The TSDoc comment should
start with /** and include a brief summary of the component's purpose and its
role in the document editing context. This aligns with the coding guidelines
requiring TSDoc for all exported types and functions in TypeScript files.
- Around line 25-32: The memo optimization of ChartRendererBoundary is
ineffective because a new empty object is created on line 48 whenever
attrs.config is absent, causing the config prop to have a new identity on each
parent render. Extract the empty object fallback to a module-level constant so
that the same reference is reused across renders when config is missing. This
stable reference will allow the memo boundary to properly prevent unnecessary
rerenders of ChartRendererBoundary when the parent rerenders but the config
value hasn't actually changed.
In
`@packages/documents/src/BlockDocumentEditor/node-views/BlockDocumentStatefulBlockNodeView.tsx`:
- Around line 126-128: The exported component BlockDocumentStatefulBlockNodeView
is missing TSDoc documentation at the component declaration level. Add a TSDoc
comment block directly above the component declaration that provides a brief,
API-level description of what this stateful block node view component does and
its purpose. The documentation should follow the coding guideline that requires
tsdoc for all exported types and functions in typescript files.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 625d3165-8a5c-446e-aa40-f023c86ccda4
📒 Files selected for processing (3)
packages/documents/src/BlockDocumentChartRendererContext.tsxpackages/documents/src/BlockDocumentEditor/node-views/BlockDocumentChartNodeView.tsxpackages/documents/src/BlockDocumentEditor/node-views/BlockDocumentStatefulBlockNodeView.tsx
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Testing
Summary by CodeRabbit