Skip to content

Invalidate canceled hierarchy delete-preview requests - #2992

Merged
Chris0Jeky merged 1 commit into
mainfrom
issue-2968/delete-preview-ownership
Sep 11, 2026
Merged

Invalidate canceled hierarchy delete-preview requests#2992
Chris0Jeky merged 1 commit into
mainfrom
issue-2968/delete-preview-ownership

Conversation

@Chris0Jeky

Copy link
Copy Markdown
Owner

Closes #2968

What

Give every hierarchy delete-preview attempt its own ownership token in useCardModal.ts, and reject every late effect from a superseded attempt.

handleDeleteClick previously guarded its success/failure/finally effects with isCurrentCardSession(cardId, session) plus showDeleteConfirm.value. Both are identical across a cancel-then-reopen on the same card, so a response from the canceled attempt could populate the reopened dialog, clear its loading state, or replace a newer preview that had already landed.

Why this shape

A per-attempt generation counter is the idiom already in use in this codebase for exactly this problem — components/board/BoardCardArchive.vue, components/board/CardParentField.vue, components/board/BoardProposalPreview.vue, components/chat/ChatContextPicker.vue. Reused rather than introducing an AbortController, so cardsApi.previewDetach keeps its signature and no other file changes.

  • deletePreviewGeneration is bumped by invalidateDeletePreview(), which resets detachPreview, deletePreviewError and deletePreviewLoading together with the token, so a bump can never leave the dialog spinning on an abandoned request.
  • Invalidation points: starting a new attempt, handleDeleteCancel, a card-identity switch, the card modal closing, and unmount.
  • ownsDeletePreview(generation, cardId, session) gates all three effect sites (success, failure, finally) — the finally and catch arms were previously weaker than the success arm.
  • handleDeleteConfirm is unchanged: it still reads detachPreview.value, i.e. the fingerprint/version the user is actually shown, so explicit confirmation stays wired to the displayed child list. A test now pins that a stale reply landing after a fresh one cannot change what gets confirmed.

Deliberately not changed: the card watcher's unconditional detachPreview = null / deletePreviewError = null for a same-card object refresh does not bump the generation. A refresh of the same card is not a card change, and #2965's post-commit refresh repair relies on the in-flight preview for that card still being allowed to land. Bumping there would strand the dialog with no preview and no way to retry short of cancel/reopen.

Also out of scope: handleDeleteConfirm's own failure path still writes deletePreviewError without an ownership check. That is the delete mutation, not the preview request, and it is guarded server-side by the fingerprint; flagging rather than expanding the slice.

Lane / file-ownership note

PR #2977 (issue-2240/card-assignments) is concurrently editing CardModal.vue, CardAssignmentField.vue and PaperBoardCard.vue. This PR touches neither — no template change was needed, CardModal.vue already renders detachPreview / deletePreviewLoading / deletePreviewError unchanged. The only source edit is useCardModal.ts, confined to the delete region (delete state declarations, handleDeleteClick / handleDeleteCancel, and one added line each in the two watchers and onBeforeUnmount). Textual conflict with #2977 is possible, semantic overlap is not. Claimed on #2968 with [Claude lane claim v2] before writing.

Tests

Seven new cases in src/tests/composables/useCardModal.spec.ts under delete preview ownership, driven by a defer() helper so two attempts overlap deterministically:

  1. old response after new — stale reply does not populate the reopened dialog and does not clear its loading state
  2. new response then old — stale reply does not replace the newer preview
  3. a failure from the stale attempt surfaces no error in the new dialog
  4. cancel clears the displayed child list, error and loading state
  5. card change mid-flight discards the in-flight preview (and does not leave it spinning)
  6. card modal close discards the in-flight preview
  7. confirmation goes to the server with the fingerprint the user is currently shown, not the stale one

All seven were confirmed red against the pre-fix useCardModal.ts (source reverted, tests run, 7 failed) and green with the fix.

Checks run

Check Result
npx vitest --run --maxWorkers=2 src/tests/composables/useCardModal.spec.ts 75 passed (68 pre-existing + 7 new)
npx vitest --run --maxWorkers=2 src/tests/components/CardModal.spec.ts src/tests/guards/nativeBrowserDialogs.spec.ts 44 passed
npm run typecheck (vue-tsc -b) clean
npm run build built, only the pre-existing INEFFECTIVE_DYNAMIC_IMPORT warning for toastStore.ts
npx eslint src/composables/useCardModal.ts src/tests/composables/useCardModal.spec.ts clean
Red-before-green on the 7 new cases confirmed

NOT verified

  • No browser or Playwright reproduction — the race is only exercised through the composable's unit tests, matching the issue's "no separate browser reproduction is claimed".
  • Full frontend vitest suite not run (OOMs on this box); only the specs above.
  • No backend run — frontend-only change, no API contract touched.
  • Not proven: behaviour when the delete mutation itself fails after an invalidation (see the out-of-scope note above).
  • No canonical doc change: no product doc describes the delete-preview request-ownership contract, so docs/STATUS.md / masterplan / ADR are untouched.

Opening Delete, cancelling, and reopening before the first preview resolved reused the same card-session identity, so the stale response could populate the reopened dialog, clear its loading state, or replace a newer preview. Track a per-attempt generation - the idiom already used by BoardCardArchive, CardParentField and BoardProposalPreview - and bump it on cancel, reopen, card switch, modal close and unmount. Success, failure and finally effects now run only while their attempt still owns the open dialog, so confirmation keeps applying to the child list the user is actually shown.
@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review Completed 2026-09-11T17:03:57.029797Z 0ce72e0 PR opened
ℹ️ 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" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@Chris0Jeky

Copy link
Copy Markdown
Owner Author

Fresh-context independent review at exact head 0ce72e04e9f8a7ba5576001311031e75bc8cce29 against base 401fd648c289edad45c62878c3bbcdcb429779f3: CLEAN, no CRITICAL/HIGH findings.

Verified by the reviewer from source (read-only):

  • Token discipline: the generation is bumped before previewDetach is issued, captured in a local, and all three arms (then/catch/finally) are gated on it; no ungated post-await write to detachPreview, deletePreviewError, the loading flag or showDeleteConfirm.
  • Loading: a stale attempt's finally cannot clear a newer attempt's flag; every writer of showDeleteConfirm = false (cancel, card-switch watcher, close, unmount) is paired with invalidation, and the confirm success path is unreachable while an attempt is unsettled.
  • Cancel-then-reopen resets the preview/error together with the bump, so no stale child list renders while the new attempt loads; the card-identity watcher invalidates and nulls the preview, so a preview for card X cannot be confirmed against card Y.
  • Tests use deferred promises resolved in the stated order, and the confirm test resolves the stale attempt last, proving the stale fingerprint cannot reach deleteCard.
  • Side benefit: closing the card editor now also closes the delete confirmation, which previously stayed rendered outside the modal's v-if.

Non-blocking notes, recorded here rather than as commits (law 2c):

  • LOW: handleDeleteConfirm's own failure path remains ungated (declared exclusion). Escape is not disabled mid-delete, so a rejection can write an error into a reopened dialog; the outcome is conservative (blocks a delete, never enables a wrong one) and the server fingerprint bounds safety. Tracked as the author's stated next slice.
  • LOW: the design relies on every future showDeleteConfirm = false being paired with invalidateDeletePreview(); a one-line comment on the declaration would pin it.

Checks at this head: hosted rollup green (0 failed, 0 pending). Merging with a merge commit.

@Chris0Jeky
Chris0Jeky merged commit e84ccfe into main Sep 11, 2026
36 checks passed
@Chris0Jeky
Chris0Jeky deleted the issue-2968/delete-preview-ownership branch September 11, 2026 17:28
@github-project-automation github-project-automation Bot moved this from Pending to Done in Taskdeck Execution Sep 11, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

Invalidate canceled hierarchy delete-preview requests

1 participant