improvement(files): remove Save UI in favor of silent autosave with local-first draft recovery#5549
Conversation
…ocal-first draft recovery Files editor no longer shows a Save/Saving/Save failed button - autosave already ran in the background, the button was vestigial. Cmd+S still works. Adds local-first draft persistence to the shared useAutosave hook (opt-in via draftKey): edits mirror into IndexedDB on a 400ms debounce, independent of the 1.5s network save, and flush best-effort on visibilitychange/pagehide. On reopen, a newer local draft is silently recovered and resynced. This replaces the beforeunload "leave site?" warning, which only blocked navigation - it never actually saved anything. A toast (with a Retry action) surfaces a save failure, since there's no more persistent status indicator to show it.
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
PR SummaryMedium Risk Overview
Save failures show a toast with Retry (replacing the old status button). Rich markdown now resyncs the ProseMirror doc when content changes outside streaming (e.g. restored drafts). Extensive Reviewed by Cursor Bugbot for commit 3ea8703. Configure here. |
Greptile SummaryThis PR moves the Files editor to silent autosave with local draft recovery. The main changes are:
Confidence Score: 4/5This is close, but the discard correction race should be fixed before merging.
apps/sim/hooks/use-autosave.ts Important Files Changed
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
…itor resync Structural cleanup after the local-draft feature: draftKey is now ANDed with enabled inside useAutosave itself (rather than trusting every caller to replicate that gating), the combined dirty-transition/debounce effect is split into two single-purpose effects, redundant back-to-back IndexedDB writes on visibilitychange+pagehide are deduped, a dead identity wrapper around setDraftContent is removed, and the three near-identical "resync editor body if changed" blocks in rich-markdown-editor.tsx collapse into one local helper.
Two real bugs from Greptile's first review pass: - Unmounting before the 400ms local-draft debounce fired cancelled the pending timer without ever writing the draft, so if the network flush also failed on the way out, the edit had no backup anywhere. The unmount cleanup now calls persistLocalDraft() synchronously before attempting the network flush. - The save-failure toast's Retry action read saveRef.current lazily at click time, so navigating to a different file before clicking Retry would retry-save the wrong file. It now captures the failing file's save function at the moment the toast is created.
|
@cursor review |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Discard previously only cleared the parent's mirrored isDirty/saveStatus state; the editor's own content was never reset to match the server baseline. On unmount, useAutosave's flush logic saw content still diverged and (a) re-saved the "discarded" edit to the server, and (b) after this PR's local-draft addition, also persisted it to IndexedDB — so even a future fix to (a) would still have the draft resurrect the discarded text on next open. Adds a discardRef bridge (mirrors the existing saveRef pattern) so Discard resets the editor's draft content back to savedContent before navigating away, closing both paths at the root.
|
@cursor review |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit c0bc3a4. Configure here.
…iming The previous discard fix (setDraftContent(savedContent) before navigating) relied on that dispatch landing before the FileViewer unmounts. If unmount raced ahead of it, the autosave cleanup would still see stale dirty content and could resurrect the discarded edit via the local draft. useAutosave now exposes discard(): it flags the instance as discarded, cancels any pending timers, and clears the local draft immediately. Every write path (persistLocalDraft, save, the unmount flush) checks that flag first, so nothing written after discard() can bring the edit back, regardless of whether the content-reset render has committed yet.
|
@cursor review |
…/delete ordering Two more real races from round 4 of review: - discard() couldn't stop a save that had already started (discardedRef only blocks saves not yet begun). Once that in-flight save lands, it now schedules a corrective save to push the reverted content, rather than leaving the discarded edit on the server permanently. Only fires when a save was genuinely in flight at discard time. - persistLocalDraft's set() and clearLocalDraft's del() were independent promises with no ordering guarantee. A slow write starting before discard could resolve after discard's delete and resurrect the draft. Both now go through a single serialized queue per hook instance, so a delete queued after a write always runs after it completes.
|
@cursor review |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 1a70d43. Configure here.
The corrective save (from the previous fix) relied on the caller's setDraftContent(savedContent) having landed by the time it ran — a real race, not a guarantee: React commits that render on its own schedule, and if the correction's continuation runs first, onSave still reads the ambient (still-dirty) content ref and re-persists the discarded edit. onSave now accepts an optional override content; discard() captures savedContentRef.current as an explicit target at the moment it's called and passes it through, so the correction always pushes the true reverted baseline regardless of render timing. Widened the shared onSave type is backward compatible — the other useAutosave caller (chunk-editor) ignores the extra optional param.
inFlightRef.current was never reset after a save resolved or rejected — it stayed pointing at the (now-fulfilled) promise indefinitely. discard() reads it to decide whether a save is genuinely in flight; since a resolved promise is still truthy, discard() treated any prior completed save as still pending, captured savedContentRef.current as the "corrective" target, and could push a stale baseline if that capture happened before the save's own dispatch had updated it. Now cleared to null as soon as the save settles, so discard()'s in-flight check reflects reality regardless of how long ago the last save finished.
|
@cursor review |
…r discard if editing continues Two more from round 8: - If discard()'s corrective save failed, it was only logged — the server could permanently keep the discarded edit with zero user-facing signal. Now surfaced via a dedicated onDiscardCorrectionFailed callback, which closes over the specific file's own name rather than routing through the shared onSaveStatusChange path (that path reads whichever file is currently selected, which by the time this fires is already the file the user navigated to, not the discarded one). - discardedRef never cleared once set, so if the editor stayed mounted briefly after discard (before navigation completes) and the user typed again, every save path silently no-op'd forever for that new edit too. It now clears itself as soon as a genuinely new edit (content diverging from savedContent again) is observed.
|
@cursor review |
…hin one idbQueueRef was a per-instance ref, so it only ordered IndexedDB ops issued by the same hook instance. A slow write queued by an unmount's flush lived on as a bare promise after that instance was gone, with nothing sequencing it against a freshly-mounted instance for the same file — its del() or recovery read could run first, and the late write would land afterward and resurrect a draft that was supposed to be gone. Replaced the per-instance ref with a module-level queue keyed by draft key, shared by every useAutosave instance (past or present) touching that key, so ordering holds across a fast unmount+remount of the same file.
|
@cursor review |
…mental fixes Cosmetic-only pass, no behavior change: - Hoisted MIN_SAVING_DISPLAY_MS to module scope alongside LOCAL_DRAFT_DELAY_MS (was declared inside the hook body, re-allocated every render, inconsistent with its sibling constant). - Grouped the ~15 refs by concern (save/network, content mirrors, draft-key + callbacks, local-draft persistence, discard) instead of the chronological order they were added across nine review rounds. - Removed a provably-dead re-check in the unmount cleanup: content/savedContent can't change between the outer guard and the inner one (no renders happen post-unmount), so only the discardedRef half of the inner check was live. - Removed an unnecessary useCallback around onDiscardCorrectionFailed — its reference is never observed by anything (useAutosave copies it into a ref every render regardless of identity), unlike handleSaveStatusChange in files.tsx, which is correctly memoized because it flows through React.memo-wrapped TextEditor/RichMarkdownEditor. Validated against external research: the two-tier debounce (network + local IndexedDB draft) matches how Tiptap/Notion describe their own local-first persistence; the discardedRef+corrective-save approach over AbortController is a deliberate, justified choice (onSave has no signal parameter, and an abort can't undo a write that's already landed server-side); the module-level per-key promise queue is a recognized idiomatic pattern. Splitting this hook into three smaller ones (useDebouncedSave/useLocalDraft/useDiscard) is a legitimate future refactor, deliberately deferred given the risk of touching this heavily-interdependent, already-hardened state this late in review.
… local drafts only once per mount Two more real races, both interactions between earlier fixes: - discard()'s corrective save and a genuinely new edit made right after could race independently: if the user typed again before the correction fired, and that correction landed after the new edit's own save, the server would end up with the discarded baseline instead of the user's latest content. The correction now shares the same inFlightRef/savingRef mutual exclusion normal saves use, and skips entirely once content has moved on to something that's neither the discarded baseline nor what it was at the moment discard() was called. - The local-draft recovery effect re-ran every time draftKey toggled through enabled (e.g. autosave turning off during agent streaming and back on once it settles), re-scanning IndexedDB as if freshly mounted. If the settled content coincidentally matched the stale draft's stored baseline, a pre-stream local edit could silently overwrite the agent's work. Recovery now attempts exactly once per mount.
|
@cursor review |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 3ea8703. Configure here.
|
@cursor review |
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 3ea8703. Configure here.
| return () => { | ||
| cancelled = true | ||
| } | ||
| }, [effectiveDraftKey, clearLocalDraft]) |
There was a problem hiding this comment.
Cancelled recovery never retries
Medium Severity
IndexedDB draft recovery sets recoveryAttemptedRef before the async read finishes. If effectiveDraftKey drops when agent streaming disables autosave, the effect cleanup marks the attempt cancelled, but the ref stays true. When autosave returns, recovery is skipped for the rest of the mount, so a valid local draft may never apply until a full remount.
Reviewed by Cursor Bugbot for commit 3ea8703. Configure here.
1 participant
Summary
beforeunload"leave site?" warning - it only blocked navigation, it never actually saved anything.useAutosavehook (opt-in viadraftKey): edits mirror into IndexedDB on a 400ms debounce, independent of the 1.5s network save, and flush best-effort onvisibilitychange/pagehide. On reopen, a newer local draft is silently recovered and resynced.Type of Change
Testing
useAutosave's local-draft persistence, recovery, and race-condition guards (apps/sim/hooks/use-autosave.test.tsx)bun run type-checkandbiome checkclean on all touched filesChecklist