Fix: DocumentEditor open delay by deferring content load#477
Fix: DocumentEditor open delay by deferring content load#477
Conversation
Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
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 (1)
📝 WalkthroughWalkthroughInitial editor content was removed from the Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
Greptile SummaryThis PR optimises Confidence Score: 4/5Safe to merge with minor style/robustness improvements No P0/P1 bugs found. Two P2 findings: autofocus being unnecessarily included in the dependency array (causes spurious applyContent calls after initial load) and the hasLoadedInitialContentRef not being reset when the editor instance is recreated. Core animation-deferral logic and rAF cancellation are correct. src/components/editor/DocumentEditor.tsx — review the effect dependency array and ref reset behaviour Important Files Changed
Reviews (1): Last reviewed commit: "Defer DocumentEditor content load to fix..." | Re-trigger Greptile |
| if (rafA != null) cancelAnimationFrame(rafA); | ||
| if (rafB != null) cancelAnimationFrame(rafB); | ||
| }; | ||
| }, [editor, content, contentType, autofocus]); |
There was a problem hiding this comment.
autofocus in deps triggers unnecessary applyContent after initial load
autofocus is only consumed in the initial-load rAF branch. Once hasLoadedInitialContentRef.current is true, any change to autofocus re-runs the effect and calls applyContent() — which, for large documents, runs a full JSON.stringify comparison — but never actually re-focuses the editor. Consider reading autofocus through a ref so it can be omitted from the dependency array:
const autofocusRef = useRef(autofocus);
useEffect(() => { autofocusRef.current = autofocus; }, [autofocus]);
// then in the rAF callback:
if (autofocusRef.current) { editor.commands.focus("end"); }This eliminates spurious applyContent calls while keeping the focus behaviour correct.
| if (hasLoadedInitialContentRef.current) { | ||
| // External update (e.g., ZeroDB sync from another tab). Apply immediately. | ||
| applyContent(); | ||
| return; | ||
| } |
There was a problem hiding this comment.
hasLoadedInitialContentRef not reset on editor recreation
hasLoadedInitialContentRef is a component-scoped useRef and persists for the lifetime of the component instance. If TipTap's useEditor ever recreates the editor object (e.g. because an extension config prop changes), the effect runs with hasLoadedInitialContentRef.current === true and applies content synchronously on the fresh editor. In most cases this is harmless, but it also means the deferred-paint optimisation is silently bypassed for any editor rebuild. Adding a reset when editor changes would make the behaviour explicit:
useEffect(() => {
hasLoadedInitialContentRef.current = false;
}, [editor]);Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
0 issues found across 1 file (changes from recent commits).
Requires human review: Modifies the initialization and synchronization timing of a core UI component (DocumentEditor), which requires manual verification to ensure no UI flickering or race conditions.
Defer initial content load in DocumentEditor to avoid blocking the modal's open animation with synchronous markdown parsing and KaTeX rendering.
content/contentTypefromuseEditorinitial options so the editor mounts empty and paints immediately.requestAnimationFramefor the first content load to ensure it runs after the modal's open paint.autofocusafter deferred initial load to focus at end of content.Summary by CodeRabbit
Release Notes