fix - not saving editor state to convex when closing/ moving to other… - #22
fix - not saving editor state to convex when closing/ moving to other…#22tarunislucky wants to merge 3 commits into
Conversation
|
@tarunislucky is attempting to deploy a commit to the Jason's projects Team on Vercel. A member of the Team first needs to authorize it. |
📝 WalkthroughWalkthroughEditorView now tracks the latest unsaved editor content in ChangesContent Persistence Enhancement
Sequence DiagramsequenceDiagram
participant User
participant CodeEditor
participant EditorView
participant Persistence
User->>CodeEditor: Type / update text
CodeEditor->>EditorView: onChange(newContent)
EditorView-->>EditorView: pendingContentRef = newContent\nclear existing timeout\nset debounce timeout
par Debounce fires later
EditorView->>Persistence: updateFile(pendingContentRef)
EditorView-->>EditorView: pendingContentRef = null
end
alt Unmount or active tab change before debounce
EditorView->>Persistence: updateFile(pendingContentRef) (immediate)
EditorView-->>EditorView: pendingContentRef = null
end
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 unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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)
src/features/editor/components/editor-view.tsx (1)
26-36:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftCleanup effect has dependency and error-handling concerns.
Two issues with this cleanup:
activeFilein dependency array may defeat debouncing: WhenupdateFilesucceeds, Convex will push an updated document, causinguseFileto return a new object reference. This triggers the effect cleanup, which could flush pending content prematurely if the user is still typing. Consider using onlyactiveTabId(which is stable) and storing the ID in a ref for the cleanup.Fire-and-forget mutation risks silent data loss: The
updateFilecall has no error handling. Per the relevant snippet, this mutation can throw on auth failures or network issues. During unmount or navigation, there's no opportunity to retry or inform the user.Proposed fix
const timeoutRef = useRef<NodeJS.Timeout | null>(null); const pendingContentRef = useRef<string | null>(null); +const activeFileIdRef = useRef<Id<"files"> | null>(null); +// Keep activeFileIdRef in sync +useEffect(() => { + activeFileIdRef.current = activeFile?._id ?? null; +}, [activeFile?._id]); // Cleanup pending debounced updates on unmount or file change useEffect(() => { return () => { if (timeoutRef.current) { clearTimeout(timeoutRef.current); } - if (pendingContentRef.current && activeFile) { - updateFile({ id: activeFile._id, content: pendingContentRef.current }); + if (pendingContentRef.current && activeFileIdRef.current) { + updateFile({ id: activeFileIdRef.current, content: pendingContentRef.current }) + .catch((err) => console.error("Failed to save pending changes:", err)); pendingContentRef.current = null; } }; -}, [activeTabId, activeFile, updateFile]); +}, [activeTabId, updateFile]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/features/editor/components/editor-view.tsx` around lines 26 - 36, The cleanup effect should stop depending on the changing activeFile object and handle updateFile failures: remove activeFile from the dependency array (leave activeTabId and updateFile), capture the current file id into a stable ref (e.g., activeFileIdRef.current = activeFile?._id inside the effect when activeFile changes), and in the cleanup use that ref (not the activeFile object) to flush pendingContentRef; call updateFile({ id: activeFileIdRef.current, content: pendingContentRef.current }) but handle errors by attaching .catch(...) to the promise (log or surface an error/toast and/or enqueue a retry) rather than fire-and-forget, and continue to clear pendingContentRef and timeoutRef as before.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/features/editor/components/editor-view.tsx`:
- Around line 26-36: The cleanup effect should stop depending on the changing
activeFile object and handle updateFile failures: remove activeFile from the
dependency array (leave activeTabId and updateFile), capture the current file id
into a stable ref (e.g., activeFileIdRef.current = activeFile?._id inside the
effect when activeFile changes), and in the cleanup use that ref (not the
activeFile object) to flush pendingContentRef; call updateFile({ id:
activeFileIdRef.current, content: pendingContentRef.current }) but handle errors
by attaching .catch(...) to the promise (log or surface an error/toast and/or
enqueue a retry) rather than fire-and-forget, and continue to clear
pendingContentRef and timeoutRef as before.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 72a4e20f-0244-409e-857b-43e34a3eadf7
📒 Files selected for processing (1)
src/features/editor/components/editor-view.tsx
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.
Inline comments:
In `@src/features/editor/components/editor-view.tsx`:
- Around line 31-34: The updateFile call is fire-and-forget, which loses
pendingContentRef.current on non-retryable failures; wrap the Promise returned
by updateFile(id: activeTabId, content: pendingContentRef.current) with
.then/.catch so you only set pendingContentRef.current = null after the Promise
resolves successfully and handle errors in .catch (e.g., log or surface via the
app's notification/error handler) to allow retry or inform the user; apply the
same pattern to the debounce save handler (the debounce callback that calls
updateFile around line 71) so both spots attach .catch and only clear
pendingContentRef on success.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e653698a-6fe4-4406-9e28-7d03da66c873
📒 Files selected for processing (1)
src/features/editor/components/editor-view.tsx
| if (pendingContentRef.current !== null && activeTabId) { | ||
| updateFile({ id: activeTabId, content: pendingContentRef.current }); | ||
| pendingContentRef.current = null; | ||
| } |
There was a problem hiding this comment.
Unhandled Promise in cleanup — silent data loss on non-retryable failure
updateFile(...) returns a Promise that is never handled. Convex React automatically retries mutations until they are confirmed to have been written to the database, and the backend ensures each mutation call executes only once, so transient network hiccups are covered. However, non-retryable failures (e.g., auth expiry, server-side validation errors thrown by the mutation) will be silently swallowed because:
- No
.catch()is attached. pendingContentRef.current = null(line 33) clears the pending content synchronously before the Promise settles — meaning on a permanent failure, the content is gone with no retry path and no error surfaced to the user.
The same fire-and-forget pattern exists in the debounce callback (line 71), though that code predates this PR.
🛡️ Proposed fix — attach a .catch() to surface failures
- if (pendingContentRef.current !== null && activeTabId) {
- updateFile({ id: activeTabId, content: pendingContentRef.current });
- pendingContentRef.current = null;
- }
+ if (pendingContentRef.current !== null && activeTabId) {
+ updateFile({ id: activeTabId, content: pendingContentRef.current }).catch(
+ (err) => console.error("[EditorView] Failed to flush pending editor content:", err)
+ );
+ pendingContentRef.current = null;
+ }For the debounce handler (line 71), the same .catch() pattern applies:
timeoutRef.current = setTimeout(() => {
- updateFile({ id: activeFile._id, content });
+ updateFile({ id: activeFile._id, content }).catch(
+ (err) => console.error("[EditorView] Debounced update failed:", err)
+ );
pendingContentRef.current = null;
}, DEBOUNCE_MS);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (pendingContentRef.current !== null && activeTabId) { | |
| updateFile({ id: activeTabId, content: pendingContentRef.current }); | |
| pendingContentRef.current = null; | |
| } | |
| if (pendingContentRef.current !== null && activeTabId) { | |
| updateFile({ id: activeTabId, content: pendingContentRef.current }).catch( | |
| (err) => console.error("[EditorView] Failed to flush pending editor content:", err) | |
| ); | |
| pendingContentRef.current = null; | |
| } |
🤖 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 `@src/features/editor/components/editor-view.tsx` around lines 31 - 34, The
updateFile call is fire-and-forget, which loses pendingContentRef.current on
non-retryable failures; wrap the Promise returned by updateFile(id: activeTabId,
content: pendingContentRef.current) with .then/.catch so you only set
pendingContentRef.current = null after the Promise resolves successfully and
handle errors in .catch (e.g., log or surface via the app's notification/error
handler) to allow retry or inform the user; apply the same pattern to the
debounce save handler (the debounce callback that calls updateFile around line
71) so both spots attach .catch and only clear pendingContentRef on success.
1 participant
Issue
When a user edits a file and switches to another file before the debounce timeout (1500ms) completes, the pending changes are lost without being saved to Convex.
Root Cause
The useEffect cleanup function was clearing the debounce timer when activeTabId changed, but it wasn't flushing the pending content before clearing the timeout. This resulted in unsaved edits being discarded.
Steps to Reproduce
Solution
Changes
Summary by CodeRabbit