web(drafts): keep the losing writer's text after a stale save - #67
TusanHomichi wants to merge 1 commit into
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0c955ce220
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| destroy(): void { | ||
| this.#destroyed = true; | ||
| if (this.#timer !== null) { | ||
| clearTimeout(this.#timer); | ||
| this.#timer = null; |
There was a problem hiding this comment.
When a user navigates away within the 600 ms debounce window, destroy() clears the pending timer and marks the controller destroyed, so the queued autosave never reaches the server and the edit is silently lost despite the UI having shown “Saving…”. Flush the pending snapshot during navigation or warn/block navigation rather than canceling the only save attempt.
Useful? React with 👍 / 👎.
| function received(result: SaveResult): void { | ||
| if (result.status === 'stale') { | ||
| void recoverFromRefusal(result); | ||
| return; | ||
| } |
There was a problem hiding this comment.
When the writer follows the displayed recovery instructions by copying the refused text into the reloaded fields and waiting for autosave to succeed, the saved result is ignored and refusalUnresolved remains true. Submit and finalize therefore remain blocked until the user additionally presses the destructive-sounding discard button, contrary to the offered “copy ... and save, or discard” alternatives; successful saves that incorporate the recovered content need to resolve the guard.
Useful? React with 👍 / 👎.
| return () => { | ||
| editor.destroy(); | ||
| // A different draft must not inherit this one's working copy or | ||
| // its refused text. | ||
| editor = new DraftEditorController( |
There was a problem hiding this comment.
When SvelteKit reuses this [id] route component for another draft after a refusal, the cleanup replaces the editor and removes its recovery panel but leaves the page-level refusalUnresolved flag set. Submit or finalize on the new draft is then blocked with a claim that refused text is still present, while there is no panel or discard action capable of clearing the flag.
Useful? React with 👍 / 👎.
| const buffer = divergentBuffer( | ||
| refused, | ||
| replaced ?? editor.snapshot(), | ||
| reloaded ? view : null | ||
| ); | ||
| editor.keepRefused(buffer); |
There was a problem hiding this comment.
If the route changes to another draft while the refusal-triggered load() is pending, that load correctly returns null, but this continuation then uses the newly installed editor and calls keepRefused on it. Consequently the old draft's narrative can appear in the new draft's recovery panel and set its workflow guard; capture the originating controller/draft and stop the recovery continuation when either changes.
Useful? React with 👍 / 👎.
TusanHomichi
left a comment
TusanHomichi
left a comment
There was a problem hiding this comment.
Merge review at 0c955ce: hold this PR. I inspected the controller/route lifecycle and agree with the four existing, unresolved Codex findings; this summary does not create duplicate findings. pr-gate run 34661983530 is green, but it does not cover these boundaries.
Required repairs and regression evidence:
DraftEditorController::destroy()cancels the only pending autosave inside the 600 ms debounce window. Preserve/flush legitimate working-copy edits against their original draft identity before allowing navigation, or explicitly warn/block navigation. Do not mistake the requirement to discard refused recovery buffers on navigation for permission to silently discard unsaved ordinary edits. Add a real navigation-before-debounce regression.received()ignores successful saves, so following the offered copy-back-and-save recovery path leavesrefusalUnresolvedset. Make successful resolution clear the matching guard without requiring an additional discard action, and without allowing unrelated/failed saves or a first attempted workflow action to clear other unresolved text. Test copy-back -> successful save -> submit/finalize without discard.- The identity-change cleanup replaces the controller but leaves the page-level refusal guard set. Keep all refusal/guard state in the same draft lifetime, and prove that client-side navigation to another
[id]does not leave it blocked with no recovery panel capable of resolving it. recoverFromRefusal()resumes afterawait load()using the currenteditor, even whenload()returned null because the route changed. Capture the originating controller and generation/draft identity and invalidate the complete continuation before it updates buffers, errors, or guards. Distinguish obsolete navigation results from actual reload failures. Hold the refusal-triggered GET, navigate to another draft using SPA navigation (not a full page reload), then release it; assert no old content, error, or guard appears in the destination. A different-form destination and an A -> B -> A navigation are useful checks against field-ID coincidences and identity-only guards.
Keep the existing recovery text memory-only and read-only; no automatic merging/resubmission. Retain the existing failed-reload, late-typing, and repeated-refusal coverage. Revalidate against current main, which now includes #68, and rerun the full repository/browser gates. #59 remains open.
Verification limitation: this review used source inspection and GitHub CI/review evidence; no independent local Rust/browser run was performed.
A refused save reloaded the winning copy and took the writer's own text with it, turning an honest concurrency refusal into lost work (#34). The draft working copy, its autosave chain, and the recovery buffers now have one owner: `web/src/lib/drafts/`, which the route page reads instead of holding a second copy of every editable value (#59, partial). The refusal keeps every save the contract refused, newest first, with anything typed while the request was in flight, shown read-only, copyable, and field-labelled. Nothing is merged, resubmitted, persisted, or sent anywhere, and the buffers die with the draft identity. Recovery state and its guard share the controller's lifetime, which is what the review's four lifecycle findings needed: - a queued ordinary edit is saved when the route goes away instead of being cancelled with its debounce timer; refused recovery text is not resubmitted, and a refusal the page has not answered yet is skipped; - the workflow-act guard is per refusal, and a save the writer makes after seeing a refusal resolves exactly the buffers that save carries — copying the text back and saving unlocks submission and finalization without an extra discard, while another refusal's uncopied text keeps holding the act; - the guard lives with the buffers in the controller, so a draft identity change takes it away instead of leaving a page blocked with nothing to dismiss; - the recovery continuation captures the controller and the route it started on, so a refusal whose reload lands after the page moved on touches no buffer, no error, and no guard of the draft now on screen. Browser proof in `web/e2e/draft-recovery.spec.ts` now runs eleven scenarios over two real writer contexts, synchronized on held requests: the loser copies their sentence; text typed during a pending request and text typed while the winner was reloading both stay recoverable and never reach the server; a failed reload keeps the buffer; a second refusal adds to the first; a workflow act makes no request until the writer acknowledges, and then does; copy-back and save lifts the guard without a discard; an edit made inside the debounce window survives navigation; a refusal dies with its draft while another draft's acts work; and a refusal that lands after the page moved on changes nothing. Each fix was falsified against the behaviour it replaces before it was kept.
0c955ce to
afed873
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: afed87374b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (this.#stale(run)) { | ||
| return; | ||
| } | ||
| this.revision = saved.revision; |
There was a problem hiding this comment.
When navigation occurs after edit B is made while save A is in flight, destroy() sees the dirty edit and starts #saveOnTeardown, but first marks the controller destroyed. This check then discards A's successful response before this.revision is updated, so the teardown save sends B using A's now-stale revision; the server refuses it and the teardown catch suppresses that result, silently losing B. The new teardown path needs to retain A's returned revision, or otherwise serialize the final save without treating A's response as obsolete.
Useful? React with 👍 / 👎.
| if (text === '' || text === (winning_narratives.get(id) ?? '')) { | ||
| continue; |
There was a problem hiding this comment.
When the winning copy still contains a narrative that the losing writer intentionally cleared, current is empty and this condition drops the edit before comparing it with the winner. If that was the only divergence, no recovery buffer is retained and unresolved becomes false, so the reload restores the old narrative and submission or finalization can proceed without the writer acknowledging that their deletion was refused. Represent an empty-but-divergent narrative explicitly rather than filtering every empty value.
Useful? React with 👍 / 👎.
Repairs at
|
|
1 participant
Primary Issue
Closes #34
Problem And Outcome
A refused save (
stale_save) reloaded the winning copy and took the losing writer's own text with it, turning an honest concurrency refusal into lost work. After this change the loser's text — what the refused save carried, plus anything typed while that request or the winner's reload was still in flight — stays on the page, read-only and copyable, until they discard it or navigate away. Nothing is persisted, merged, resubmitted, or sent anywhere.Changes
web/src/lib/drafts/editor.svelte.ts— one owner for the draft working copy, the revision every save carries, the debounced autosave chain, the metadata refresh, and the refused-save recovery buffers (the refactor(web): split large workflow pages and domain API contracts #59 ownership boundary, for the draft editor only).web/src/lib/drafts/RefusedTextPanel.svelte— one read-only, copyable block per refused save, newest first, labelled per field, distinct from the saved working copy.web/src/routes/drafts/[id]/+page.svelte— reads the controller; reloads the winner on a refusal; keeps the recovery panel outside the block a failed reload empties;load()returns the working copyadoptreplaced and refuses to publish an answer for a draft the route has left.web/e2e/draft-recovery.spec.ts— seven two-context browser scenarios synchronized on held HTTP requests rather than sleeps.docs/development.md— the/drafts/[id]row and theweb/src/lib/drafts/ownership.Workflow acts wait while the writer's own text is unresolved — a failed save, or a refusal they have not dealt with — and say why; the guard lifts when they copy the text in and save, or discard it.
Scope
Verification
Each behavioural fix was falsified against the behaviour it replaces before it was kept: the "typed while the winner was reloading" scenario shows the older sentence without the fix, the two-refusal scenario finds one panel instead of two with a single-slot buffer, and the workflow-act scenario lets the second act reach the server when the guard is consumed by the first.
Review And Merge Notes