Repository navigation
Stop a pending draft autosave from undoing a note save - #702
Merged
Merged
Conversation
Saving a note clears its server draft, but the composer only cancelled its debounced draft write once the save's response came back. An edit made within the 1s debounce before Save fired that write mid-save, and if it landed after the clear the saved text came back as a draft on the next load. This flaked the note-compose E2E test, where reloading a draft re-emits it once on mount. Save now drops the pending write, stops a mid-save tab hide from beaconing it, and waits for a write already in flight before creating the note. The E2E test edits right before Save and holds the save's response past the debounce, so it fails every time without the fix. Amp-Thread-ID: https://ampcode.com/threads/T-01a1dc3f-f20d-736a-80f8-8b811bc505b5
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
There was a problem hiding this comment.
All reported issues were addressed across 2 files
Reply with feedback, questions, or to request a fix.
View guided diff | Turn on auto-fix | Re-trigger cubic
…s mid-save Review follow-ups: - Overlapping draft autosaves are all awaited, not just the latest. - The wait is capped at 3s so a stalled draft write can't block the save. - The composer is read-only while saving; the save sends the click-time text and then empties the composer, so an edit made meanwhile was lost (and could schedule a draft write after the clear). Amp-Thread-ID: https://ampcode.com/threads/T-01a1dc3f-f20d-736a-80f8-8b811bc505b5
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.
What & why
Saving a note from the composer right after typing could bring the saved text back as a draft on the next load.
Saving a note clears the server draft in the same request. But the composer only cancelled its debounced (1s) draft autosave once the save's response came back. An edit made within that 1s before Save fired the autosave mid-save. If that write reached the server after the clear, the draft came back. A tab hidden mid-save could do the same via the
sendBeaconflush, since the composer was still marked dirty.This is what flaked
note-compose.spec.ts› "auto-saves the note as a server-side draft, repopulated on load" in CI. In that test, the fresh tab's editor re-emits a loaded draft once on mount (it adds two trailing newlines), which schedules an autosave. The failing CI value was exactly that re-emitted string, not the first tab's text. The first tab'scontext.close()sends no beacon at all: checked directly, Playwright's context close fires neitherpagehidenorvisibilitychange.Fix, in
note-composer.tsx, before creating the note:If the save fails, the text is still in the composer, so autosave resumes.
Checklist
.env.example, in-app help,CONTRIBUTING.md, onboarding checklist), or N/A: N/A, the documented behavior (saving clears the draft) is unchanged; this makes it holdcaptureServerException/ the error boundary, or N/A: N/A, no new error pathsmainit fails every time with the CI symptom; with the fix it passes.record-pr-demoskill); refactor through a user flow → parity run, or N/A: before and after belowlearnskill, or N/A: the regression test above is the guardrail; this isn't a pattern repeated elsewhereDemo
Both takes simulate a slow connection inside the page, holding the save's response for 2s, so the race shows reliably. In production it needs Save to respond more than ~1s after the last keystroke.
❌ Before
pr-draft-race-before.mp4
✅ After
pr-draft-race-after.mp4
Notes
Verified locally:
bun run check:fixis clean.bun run test:e2e e2e/note-compose.spec.tspasses 3/3 full runs with the fix, and again after the review follow-ups. The updated test fails onmainon every run.Not verified: the save-failure path (autosave resuming) isn't exercised by a test.
Left for follow-ups, out of scope here:
pagehideandvisibilitychange).