Skip to content

fix(frontend): preserve unsaved card drafts across archive recovery and completion - #3015

Merged
Chris0Jeky merged 3 commits into
mainfrom
issue-2969/draft-safe-archive-recovery
Sep 11, 2026
Merged

Chris0Jeky merged 3 commits into
mainfrom
issue-2969/draft-safe-archive-recovery

Conversation

@Chris0Jeky

Copy link
Copy Markdown
Owner

What

Two close paths in the Legacy card editor emitted close without asking the dirty-close guard anything, so an unsaved draft could disappear without a confirmation:

  • Archive recovery. refreshArchiveState (the refresh handler behind "Refresh card state" on a failed archive) emitted close and refetched the board. Edit the title after an archive fails, press it, and the draft is gone.
  • Archive completion. @changed="emit('updated'); emit('close')" closed unconditionally, so a draft typed while an archive request was in flight was discarded the moment that request succeeded.

Both now funnel through the guards PR #2977 established:

  • refreshArchiveState calls handleClose. A clean editor closes and refetches exactly as before; a dirty one gets the save/discard confirmation; an unanswered assignment PUT gets Add multiple card assignments and explicit import mapping #2977's truthful "already sent to the server" notice instead of a close. The board refetch it owes is carried with the close (pendingArchiveRefresh) and runs only if the close actually happens — cancel the confirmation and neither the draft nor the failed archive state moves.
  • handleArchiveChanged keeps the editor open when the draft is dirty: it records the state the completed request produced, refetches the board, and renders a role="status" notice saying what happened and that the unsaved changes are still there. A clean editor behaves as today (updated then the normal close funnel). A request that settles after the editor already closed emits nothing, so one user action can never produce two closes.

Two supporting details, both deliberate:

  • A kept draft does not emit updated. That event is not a neutral "something changed" signal in this codebase — ColumnLane maps it straight to its close handler and PaperBoardView resolves a held navigation with it — so emitting it would close the editor through the host and reintroduce the same loss. The committed archive still reaches the board through the store mutation plus the refetch.
  • Both hosts keep the open card in a snapshot ref, so props.card never learns the card is now archived while the editor stays open. cardIsArchived prefers the state the settled request produced, and the Save gate uses it, so the editor stops offering a save that could only fail against an archived card.

CardArchiveAction.vue is untouched: its confirmation flow, in-dialog recovery and expected-version/fingerprint checks are exactly as #2995 left them, and no prop or emit signature changed.

Closes #2969
Closes #2997

Checks run (worktree C:/wt/td-2969, Node 24, head 38e672b)

  • npx vitest --run --maxWorkers=2 src/tests/components/CardModal.spec.ts src/tests/composables/useCardModal.spec.ts src/tests/components/CardModalAssignmentSave.spec.ts src/tests/components/CardArchiveAction.spec.ts src/tests/components/BoardCardArchive.spec.ts — 144 passed (135 before this change).
  • npm run typecheck — clean. npm run build — clean. npx eslint on the three touched files — clean.
  • Red-first: with CardModal.vue reverted to base 4c479a7ff and the new specs in place, 3 of the 7 new CardModal.spec.ts cases fail (the two draft-loss paths) and the [Frontend][Assignments] Refresh-card-state closes the editor behind an in-flight assignment save (from PR #2977 round-2 review) #2997 case in CardModalAssignmentSave.spec.ts fails; all pass with the fix. The other four new cases pin behaviour that is unchanged on both sides (clean recovery close, clean completion, rejected request then clean close, late success after a clean close, ordinary untouched close).

NOT verified

  • No browser or screen-reader run: no Playwright/E2E execution and no real backend. The status notice's announcement was not observed with a screen reader, only asserted as role="status" in the DOM.
  • Paper board (presentation: 'inspector') was exercised only through the existing CardModalAssignmentSave.spec.ts matrix; the new archive cases run in the modal presentation. PaperBoardView.spec.ts and the wider frontend suite were not run — the suite needs a live backend on this box.
  • Not verified against the real API that a completed archive followed by a kept-open editor leaves the board consistent; the store mutation path (setCardArchived removing the card from currentBoardCards) is exercised only through mocks here.
  • An archived card with a kept draft still cannot be saved until it is restored. That is stated in the notice rather than solved; a "restore and save" affordance is out of scope for this slice.

Refresh card state and a completed archive/restore were the two close paths
in the card editor that emitted close unconditionally, so an unsaved draft
typed after an archive failure or during an in-flight archive was discarded
without a confirmation.

Refresh card state now funnels through handleClose, so it inherits both the
dirty-close confirmation and the in-flight assignment-save refusal (#2997);
the board refetch it owes is carried with the close and runs only when the
editor actually closes. A completed archive over a dirty editor keeps the
editor open with its draft, records the state the request produced, refetches
the board and says so, instead of emitting updated/close - both hosts treat
updated as "this editor is finished".

Refs #2969, #2997
Seven specs in CardModal.spec.ts cover the guarded close funnels: archive
error then edit then Refresh card state (confirmation shown, no close, no
refetch), cancelling that confirmation (draft and failed state kept, the
refresh dropped), a clean recovery close (one close plus the refetch), an
archive that succeeds over a newer draft (no close, no updated, draft kept,
state notice, refetch, save refused), a clean completion (updated + close),
a rejected request followed by a clean close, a late success after a clean
close, and an ordinary untouched close.

One spec in CardModalAssignmentSave.spec.ts pins #2997: archive recovery now
refuses while an assignment PUT is unanswered.

Verified red against the pre-fix component: the two draft-loss specs and the
#2997 spec fail there and pass with the fix; the rest pin unchanged behaviour.

Refs #2969, #2997
@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Sep 11, 2026

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-11T19:21:08.082915Z 478b8c5 New commits
ℹ️ 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.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 38e672b3e4

ℹ️ 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".

<p v-else-if="assignmentDirty" class="text-sm">Save or cancel assignment changes before saving other card fields.</p>
<CardModalActions
:is-form-valid="isFormValid && !card.isArchived && !isSaving && !assignmentDirty"
:is-form-valid="isFormValid && !cardIsArchived && !isSaving && !assignmentDirty"

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Apply the settled archive state to every control

When an archive finishes over a draft, cardIsArchived reflects the server state but only this Save gate consumes it. If the user dismisses the busy archive confirmation, edits the card, and the archive then succeeds, the assignment field and archive action still read the snapshot's card.isArchived === false; assignments remain submittable even though CardAssignmentService.StageReplaceAsync rejects archived cards, and reverting the draft re-enables a stale “Archive card” action instead of Restore. Propagate the settled state to every archive-sensitive control.

AGENTS.md reference: frontend/AGENTS.md:L5-L5

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in round 2 (478b8c5): every archive-state gate now reads the settled state.

if (hasUnsavedChanges.value) {
archiveStateAfterChange.value = !cardIsArchived.value
archiveCompletedWithDraft.value = true
void boardStore.fetchBoard(props.card.boardId)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Reload comments after the kept-open refresh

On this kept-draft path, a successful fetchBoard clears cardCommentsByCardId in boardCrudStore, but the modal remains open and useCardModal reloads comments only when the editor opens or switches cards. Thus, if the user dismisses an in-flight archive confirmation and starts editing an existing comment before the archive settles, this refresh removes the comment row and makes its unsaved edit inaccessible until the editor is closed, defeating the draft-preservation behavior. Preserve the comment cache or explicitly reload this card's comments after the refresh.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Triaged once: real, narrow, non-blocking (round ceiling reached; no committed data lost). Tracked as #3023.

@Chris0Jeky

Copy link
Copy Markdown
Owner Author

Fresh-context independent review (round 1) at exact head 38e672b3e43443f0a659eb49e7cd71cc274fe970 against base 4c479a7ff311912ffb96da027f6e525521bb70df: no CRITICAL/HIGH, two MEDIUMs taken into the fix round, LOWs recorded. A fix round is in progress on this branch.

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

  • No dangling Paper navigation: pendingNavigation only exists while the editor is dirty, and in the kept-draft case the Paper dialog still owns it (confirm/cancel, closeCard, handleCardUpdated, unmount all settle it).
  • No draft reset by the board refetch: both hosts hold the open card as a snapshot ref, fetchBoard never replaces props.card, and the card watcher bails on a dirty draft anyway.
  • No permanently disabled Save for a legitimate writer: the server rejects updates to archived cards, so cardIsArchived gating Save is truthful, and the settled state is cleared on card change, open transition and close.
  • The clean branch of handleArchiveChanged cannot bypass the in-flight assignment-save guard, because the assignment field stays dirty for the whole PUT.
  • The kept-draft state is reachable through the real route (Escape/backdrop dismisses the archive confirmation while the request is in flight, then the form can be dirtied).

Fix-now (MEDIUM, both inside the new editor-open-over-an-archived-card state):

  • Half-migrated gates: the form fieldset, CardParentField, CardAssignmentField :read-only and can-edit-type still read the stale host snapshot, so the assignment field stays writable and a save is rejected server-side with a misleading explanation. The fix round gates them on the settled archived state.
  • Contradictory notice: "restore it to save them" while the archive control is disabled for unsaved changes and still labelled "Archive card" from the stale prop. The fix round rewords the notice to what is possible and makes the control read the settled state.

Non-blocking (law 2c):

  • LOW, folded into the fix round as one-liners: pendingArchiveRefresh survives a refusal by the in-flight-save guard and is consumed by a later unrelated close (a redundant GET); the suppressed-host no-op of "Refresh card state" has no spec.
  • LOW, recorded: the kept-draft notice stays after the draft is reverted; the restore variant of the notice is probably unreachable; the new specs enter the state by typing while the confirmation is open rather than the real Escape route.

Round 2 review, two MEDIUMs in the "editor kept open over an archived card"
state this PR introduces.

1. Only the save gate had moved to the settled state; the form fieldset, the
   parent field, the assignment field's read-only gate and the type selector
   still read the host's snapshot, whose isArchived stays permanently false
   there. The assignment field therefore stayed writable and a Save assignments
   PUT would have been rejected server-side with a misleading explanation. All
   of them now read cardIsArchived, so the editor is consistently read-only
   over an archived card while the typed draft stays visible.

2. The notice claimed the card could be restored to save the draft, but the
   archive control is disabled while the editor is dirty, and its label and
   operation came from the stale snapshot ("Archive card", archive=true with a
   pre-archive updatedAt). The notice now states what is actually possible, and
   CardArchiveAction takes an optional `archived` override so its label and
   operation follow the settled state. Archive/restore stays refused while
   dirty: the explicit lifecycle confirmation contract is unchanged, and the
   override defaults to `undefined` (not Vue's Boolean cast to false) so every
   existing caller keeps reading its own card.

Also: a close refused by the in-flight assignment-save guard now drops the
board refetch "Refresh card state" had armed, so it cannot fire behind a later
unrelated close.

Specs: the kept-draft case asserts the new notice text, the disabled form
fieldset, the read-only assignment field and the "Restore card" label; a new
case pins the deliberate no-op when the host suppresses the discard prompt;
the #2997 case now also proves no stale refetch after the refusal.

Refs #2969, #2997

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 478b8c5fce

ℹ️ 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 (hasUnsavedChanges.value) {
archiveStateAfterChange.value = !cardIsArchived.value
archiveCompletedWithDraft.value = true
void boardStore.fetchBoard(props.card.boardId)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Handle rejected board refreshes

When the post-archive refresh encounters a current network/5xx failure, fetchBoard uses the default explicit intent and rethrows after updating the store (boardCrudStore.ts:488-498), but this detached promise has no rejection handler. The same pattern appears in the recovery close path at line 286, so either flow produces a global unhandledrejection and erroneous Sentry event instead of treating the refresh failure as an expected UI error; await/catch it or use the non-rejecting background refresh contract.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Triaged once: real, cosmetic in effect (unhandled rejection on a failed refresh). Tracked as #3023.

@Chris0Jeky

Copy link
Copy Markdown
Owner Author

Fix round 2 (last round) pushed: 478b8c5fceb6e78d7abd205704c58db43af988de — one commit, fix(frontend): make the whole editor read the settled archive state, on top of the reviewed 38e672b3e. Both MEDIUMs are fixed; the LOWs are recorded below rather than fixed.

MEDIUM 1 — half-migrated gates. Fixed. Every gate that turns on the archive state now reads cardIsArchived (the settled state), not the host's snapshot whose isArchived stays permanently false in this state: the form <fieldset :disabled>, CardParentField :disabled, CardAssignmentField :read-only, can-edit-type, and the save gate that had already moved. The editor is therefore consistently read-only over an archived card while the typed draft stays visible and selectable — so no control offers a write the server can only reject with a confusing explanation (the assignment PUT was the concrete one).

MEDIUM 2 — contradictory notice. Fixed, minimally. The notice now reads:

Your unsaved changes are still here. This card is now archived, so they cannot be saved; close the editor to discard them, or restore the card from the board and reopen it.

and CardArchiveAction takes an optional archived override, so in this state its label is "Restore card" and its operation would be archive=false — it can no longer offer "Archive card" over an archived card, nor send archive=true with a pre-archive updatedAt. Archive/restore stays refused while the editor is dirty: the explicit lifecycle confirmation contract is untouched, and that refusal is now asserted in the spec.

One detail worth flagging for the next reader: the override prop is declared withDefaults(..., { archived: undefined }). Vue casts an absent Boolean prop to false unless a default is declared, so without that line every existing caller (BoardCardArchive, and the component's own specs) would have been told its card is active. That mistake was made and caught — three specs in CardArchiveAction.spec.ts / BoardCardArchive.spec.ts went red on it before the default was added.

One-liners, both done. A close refused by the in-flight assignment-save guard now clears pendingArchiveRefresh, so a refetch armed by "Refresh card state" cannot fire behind a later unrelated close (refuseWhileAssignmentSaving); the #2997 spec now closes the editor after the save settles and asserts fetchBoard was never called. And a new spec mounts with suppressDiscardPrompt: true, clicks "Refresh card state", and pins the deliberate no-op: no close, no refetch, no confirmation, and nothing armed for the next close.

LOWs — recorded, not fixed (law 2c; none of them is a merge blocker under 2a):

  1. Stale notice when the draft is reverted. If the user undoes every edit after the notice appears, the notice stays until the editor is closed or reopened. It remains factually true (the card is archived), and the gates it explains are still in force, so it misleads no one about what is possible. Tying it to hasUnsavedChanges would also make it flicker while typing. Declined for this PR.
  2. The restore variant of the notice is probably unreachable. Reaching it needs a restore request in flight over a dirty editor, and the archive control is disabled whenever the editor is dirty — so the only window is "clean at press time, dirty before the response". That window is the same one the archive variant uses, so the branch is not dead by construction, but I have no evidence it is reachable for restore specifically. Kept as the honest symmetric message rather than deleted.
  3. Spec fidelity. The kept-draft specs reach the state by typing while the archive confirmation is still open (the dialog's Cancel is :disabled="busy", so the real user route out of it during the request is Escape or the backdrop). The code path under test — handleArchiveChanged with a dirty editor — is identical either way, so this is spec realism, not coverage. Worth a follow-up if anyone touches these specs.

Checks at 478b8c5fc (fresh worktree, npm ci, Node 24): the five-spec archive/editor set — CardModal, useCardModal, CardModalAssignmentSave, CardArchiveAction, BoardCardArchive145 passed (144 at the previous head; one new case). npm run typecheck clean. npx eslint on the four touched files clean. npm run build clean (exit 0, built in 7.16s; it ran behind a RAM guard because the box was under 1 GB free when the other checks finished).

Red-checked against the reviewed head 38e672b3e: with only the round-2 source reverted, the kept-draft case (gates + notice + label) and the #2997 case fail; the suppressDiscardPrompt case passes on both, as it pins behaviour this round did not change.

Still NOT verified, unchanged from the PR body: no browser, E2E or screen-reader run; no live backend; the new archive cases run in the modal presentation only; real-API board consistency after a kept-open completed archive is exercised through mocks.

@Chris0Jeky

Copy link
Copy Markdown
Owner Author

Round 2 (fix round) review at exact head 478b8c5fceb6e78d7abd205704c58db43af988de, fix range 38e672b3e..478b8c5fc: no merge blocker. Round ceiling reached (law 2d); shipping once the last hosted check at this head is green and the aging floor is met.

Scoped fresh-context verification of the fix, with the two candidate HIGHs refuted from source and the installed Vue 3.5 runtime:

  • The archived override on CardArchiveAction is declared with an explicit default: undefined, so Vue's absent-Boolean cast does not fire; BoardCardArchive, which omits the prop, still falls back to card.isArchived (its existing spec would fail loudly otherwise).
  • A stale expectedUpdatedAt cannot be silently accepted: the server requires it and returns 409 on mismatch; the fix actually removes a wrong operation the round-1 head could send (archive on an already-archived card).
  • The read-only flip on the assignment field goes false to true, which does not trigger its load(), so the [Frontend][Assignments] A load() during an in-flight assignment save can latch saving=true and leave the editor un-closable (from PR #3011 review, unverified) #3017 shape is not reachable here; the in-flight PUT still settles.
  • Clearing pendingArchiveRefresh on refusal is recoverable in one click (the Refresh control stays rendered).
  • Both extended specs and the new suppressDiscardPrompt spec are real regression guards (the Restore-label assertion fails on the round-1 head).

Triage of everything raised since round 1 (law 2c), all non-blocking and tracked as #3023:

  • MEDIUM: if the only draft was an assignment save that settles after the archive completed, the kept-draft notice outlives the draft and the Restore control re-enables with the frozen updatedAt, producing an explicit 409 with a recovery control.
  • Codex 3992475647: the kept-open fetchBoard clears the comment cache, so a comment edit in progress becomes inaccessible until the editor is closed. Real, narrow, no committed data lost.
  • Codex 3992639326: the detached fetchBoard promise has no rejection handler on the kept-open and recovery-close paths (unhandled rejection on a network/5xx failure). Real, cosmetic in effect.
  • Codex 3992475636: fixed in round 2 (all gates read the settled archive state).
  • LOWs: fieldset disabling mid-keystroke blurs focus to body (a11y); the restore variant of the notice looks unreachable; no component-level spec for the archived prop; specs enter the state by typing while the confirmation is open.

Author-run proof at this head: 145 passed across the five specs, typecheck, build (exit 0), eslint; red-checked against the round-1 head. The hosted rollup at this head is the executable proof.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

1 participant