fix(crew-editor): extend discard guard to all dirty panes

[bolichen97]

Problem / Motivation

Open a crew in the Crew editor, change its Triggers (or its template, workspace,
memory store, model, reasoning effort, session color, or picture), then press
Escape — or click the footer Cancel, or click outside the sheet. The editor
closes and the edit is gone. Nothing asks first.

The editor already knew the edit was there: dirtyPanes tracks all nine fields,
the rail shows an unsaved dot for the pane, and the footer says "Unsaved
changes". #5539 added a discard confirm to exactly these close paths, but keyed
it on the inline schedule draft alone. So the one pane whose work was protected
was the schedule draft, and the other eight were destroyed silently by the same
click.

Why it matters

This is the cheapest kind of data loss to hit and the most annoying: the user
did the work, the UI showed them it was pending, and one Escape threw it away
with no warning and no undo. A confirm already existed a few lines away — it
just did not cover them.

What changed (motivation → approach → change)

requestClose asked if (schedDraft). It now asks if (dirtyPanes.size > 0),
which is the set the rail dot and the footer note already read. There is still
ONE confirm dialog on this page — the nested one #5539 built — and it now
carries whichever of three questions is true instead of always naming a
schedule.

The schedule draft is still tested first, because its leg is not
interchangeable: only that confirm locks its Discard button while the draft's
create POST is in flight, and unlocks it after a grace period. What it did need
is a wider claim. discardTakesSheet spots the case where that dialog is about
to close the whole editor while other panes are dirty, and adds a second line
naming them — asking only about "the schedule you typed" while Discard also
throws away a Model edit is the same silent loss, just behind a dialog.

Two dismissals stay unprompted on purpose. A committing PUT is already gone and
no answer here can recall it, so offering "Discard changes" would promise a
rollback the server will not honor. A rail pane switch destroys nothing but the
draft, since every other field lives in page state and survives the switch. A
save that is only STAGING a picture is the opposite case — it has committed
nothing, so it prompts, and saveEdit waits for the answer instead of firing
the PUT through the open question. Backing out releases that save rather than
killing it.

Tests

New website/src/test/KiroCrewAgentsPage.discardGuard.test.tsx, 12 tests
against the real page:

• footer Cancel while dirty prompts; Keep editing keeps the sheet and the edit
• Discard closes the sheet and never calls the update endpoint
• Escape while dirty prompts, editor still mounted behind the confirm
• a clean sheet closes with no prompt
• a rail pane switch while dirty stays unprompted (the scope boundary)
• a dismissal while the committing PUT is away closes with no prompt
• a dismissal while the avatar upload is only STAGING still prompts
• Discard answered while the staged upload lands still abandons the save
• backing out of that confirm lets the staged save finish and promote its token
• the schedule confirm names the crew edits it also destroys
• a schedule draft alone keeps the narrow schedule question
• a successful save closes with no prompt

Each guard term was mutation-checked — reverting it turns the suite red:
requestClose ignoring dirtyPanes, committing widened to sheetBusy,
saveEdit not waiting for the answer, discardTakesSheet forced false, and
askSchedOnly forced false. All five were caught.

Main's existing KiroCrewAgentsPage.schedDraftGuard.test.tsx (13 tests) is
unchanged and still green: the schedule-draft behavior it pins is preserved.

Manual verification

Captured from the real built SPA through the committed harness
website/scripts/capture-crew-discard-guard.mjs (fixture-stubbed /api/**, no
gateway), in both themes. Verified: the confirm appears over the still-open
editor, Keep editing leaves the edit intact, and the widened schedule dialog
carries both losses.

Gates run locally: tsc -b clean, eslint clean, npm run i18n:check clean,
npm run lint:i18n clean, full frontend suite 29482 passed, and the backend
cross-surface guard set selected by scripts/local-gate.py.

Two pre-existing failures are NOT from this diff and are left alone:

• src/i18n/deadKeys.test.ts reports 30 dead keys against BASELINE 29. This
  reproduces on a pristine origin/main checkout with a clean tree. The 30th
  key is pages.chatPage.dismiss_upload_error, orphaned by fix(components): route error states through ErrorNotice (batch components-1) #8859 and already
  tracked in main is red: deadKeys.test.ts at 30 vs BASELINE 29 (pages.chatPage.dismiss_upload_error orphaned by #8859) #8959. Ratcheting the baseline from here would collide with that
  issue's own fix, so this PR does not touch it.
• 72 backend failures in this host's filesystem-ownership and symlink-trust
  tests (test_service.py, test_host_isolation_floor.py,
  test_source_providers.py, and friends). This diff leaves src/, test/ and
  docs/ byte-identical to origin/main, and the failure set is identical
  across two runs, so none of them can be attributable to it.

Screenshots / video

The confirm now raised for a Triggers edit, over the still-open editor:

The schedule draft's own confirm, widened to name the crew edits it also
destroys:

Both themes, all four states

Dirty routing pane, before the dismissal:

The confirm, light:

After Keep editing — sheet still open, edit intact, unsaved dot still on
Triggers:

The widened schedule confirm, light:

Related Issues

Closes #8284

Pattern harvest

Rule candidate: review-prompt

Pattern: a guard whose condition names ONE member of a set the same component
already computes for display. dirtyPanes drove the rail's unsaved dot and the
footer's "Unsaved changes" note, while the confirm that protects that same work
keyed on schedDraft — one element of it. The checkable shape is a dirty/pending
set that a render path consumes in full (.size, a spread, an iteration) while a
destructive path tests a single flag that also feeds that set: the two readings
disagree, and the narrower one is the one that loses data.

Also worth flagging as a shape rather than a rule: the earlier attempt at this
fix added a SECOND confirm mechanism (a useConfirm body-portal Modal) beside
the existing nested Radix one, which is what forced a modal={!confirmOpen}
focus-scope release to stop the editor trapping focus behind it. Two independent
confirm mechanisms reachable from one page's close paths is greppable —
useConfirm plus a nested <Dialog> in the same component — and the
focus-scope workaround is the tell that the second one is fighting the first.

Checklist

• At most two commits (one is the norm), with a Conventional Commits title
• Existing tests pass and new tests added for new functionality
• Self-review completed; code follows project style guidelines
• Documentation updated (if applicable) — N/A, no documented behavior changes shape
• No secrets, credentials, or internal references in the diff

[github-actions]

Design Review (Fable 5) — 🟡 CONCERNS

Design-level review of bb58d9b295469ea78c6b63722e8034fd25b97410 — updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

Design-Verdict: CONCERNS

One sheet-destroying path — "Chat with this crew" — still keys on schedDraft alone, the exact narrow-guard shape this PR exists to remove.

Watch

• requestChat (line 1261) still reads if (schedDraft) { setDiscardAsk('chat'); return }, yet chatWith ends in closeSheet() plus navigation. Cause: the guard widening was applied to requestClose but not to the other close path. Mechanism: a dirty Model/Triggers/avatar edit with no open schedule draft, then "Chat with this crew" → sheet closes and the page navigates away, unprompted. Consequence: the same silent loss the PR fixes, on the most destructive dismissal (it also leaves the page). The PR itself treats 'chat' as sheet-destroying — discardTakesSheet includes discardAsk === 'chat' — so the entry guard is now inconsistent with the dialog copy it feeds, and the description's "two dismissals stay unprompted on purpose" list (committing PUT, pane switch) does not account for it.

Suggestions

• Widen requestChat to the same dirtyPanes.size > 0 test; it already routes through the shared dialog via 'chat', so this is the identical one-line shape plus one test, in scope for this PR.

[DESIGN-REVIEWED] bb58d9b

[github-actions]

UX Review (Fable 5) — 🟡 CONCERNS

UX-level review of bb58d9b295469ea78c6b63722e8034fd25b97410 — updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

UX-Verdict: CONCERNS

The guard works and both confirms read correctly cold; the one snag is the new copy calling the thing "crew" while the surface it guards says "agent".

Watch

• New strings discard_unsaved_body ("This crew has edits that were never saved…") and discard_also_crew_edits say "crew" while the header above the dialog says "Chat with this agent" and the rail says "Agent Template" — the blind reader flagged it verbatim: "the window header says 'agent' … while the warning and body text say 'crew' … They seem to be the same thing, but I can't be sure." Referential doubt inside a destructive confirm, hit on every guarded dismissal, persistent. Fix: use one noun in the two new strings (match whichever term this editor standardizes on).

Suggestions

• discard_also_crew_edits says "other unsaved edits" without saying which; the rail already knows (dirtyPanes) — the blind reader "could not say confidently which one loses how much." Interpolate the dirty pane names ("Your Model and Triggers edits are thrown away with it").

[UX-REVIEWED] bb58d9b

[github-actions]

First Principles Review (Fable 5) — 🟡 CONCERNS

Premise-level review of bb58d9b295469ea78c6b63722e8034fd25b97410 — why this exists and whether the shipped surface is the smallest honest version. Updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

I have everything I need. All the repo-convention checks pass (committed capture scripts: 100+ siblings in website/scripts/; committed screenshot dirs: 300+ siblings in temp-screenshots/), no shared confirm mechanism exists to reuse (sibling pages use ad-hoc window.confirm), and I found one counted unfixed sibling of the PR's own named root cause: requestChat at KiroCrewAgentsPage.tsx:1262 still keys on schedDraft alone, so "Chat with this crew" closes the sheet over dirty panes with no prompt.

First-Principles-Verdict: CONCERNS

The fix is real and cause-level, but requestChat still keys on schedDraft alone — one counted unfixed sibling of the exact pattern this PR names.

What this change ships

Intent: stop the crew editor from silently destroying unsaved edits when the user dismisses it — a FIX.

1. Dismissing the editor (Cancel, Escape, overlay) with any unsaved edit now asks first — justified (Crew editor: extend the dismissal discard-guard from the schedule draft to all dirty panes #8284)
2. Dismissal during a committing save closes with no prompt — justified, non-cancellable request, declared
3. The schedule confirm now also names the crew edits it destroys — justified, declared
4. A save staging a picture now waits for the discard answer — justified, closes a race the new prompt creates
5. The confirm's title, body and button relabel by what's at stake — justified, declared
6. Four new strings in 13 locale files — justified, i18n convention
7. Committed screenshot harness script — justified, 100+ capture-*.mjs siblings in website/scripts/
8. Eight PNGs under temp-screenshots/crew-discard-guard/ — justified, 300+ sibling per-PR dirs, repo convention

Watch

• Unfixed sibling, count 1: requestChat (website/src/pages/KiroCrewAgentsPage.tsx:1262) still asks if (schedDraft), so "Chat with this crew" over a dirty Triggers or Model edit runs chatWith → closeSheet unprompted — the exact "guard names ONE member of a set" pattern the PR's own harvest describes, and discardTakesSheet already handles discardAsk === 'chat' for when the question is raised. The description's "two dismissals stay unprompted on purpose" does not list this one, so the gap is undeclared, not deliberate. Smallest fix: key requestChat on the same dirtyPanes set requestClose now reads. All other destruction paths check out: requestPane and requestCancelDraft keep the sheet, so their narrow guard is correct.

[FIRST-PRINCIPLES-REVIEWED] bb58d9b

[github-actions]

Opus 4.8 Review — ✅ no blocking findings

Reviewed bb58d9b295469ea78c6b63722e8034fd25b97410 — this comment is updated in place on each push.

Review details

No findings.

[OPUS-REVIEWED] bb58d9b

Verdict parsed from the review's SHA-scoped output markers for commit bb58d9b295469ea78c6b63722e8034fd25b97410.

False positive or not applicable? A repository writer can comment:
/ai-review override fable bb58d9b295469ea78c6b63722e8034fd25b97410: <one-sentence reason>

[github-actions]

GPT 5.6 Review — ✅ no blocking findings

GPT 5.6 completed its review of bb58d9b295469ea78c6b63722e8034fd25b97410 and found no blocking issues.

This comment is updated in place on each push.

Review details

No findings.
[GPT-REVIEWED] bb58d9b

False positive or not applicable? A repository writer can comment:
/ai-review override gpt bb58d9b295469ea78c6b63722e8034fd25b97410: <one-sentence reason>

[bolichen97]

• span=6e26220b5826 — outer focus trap active behind the confirm — fixed in 831595052.

The finding holds: the discard confirm is a body-portal Modal rendered as a sibling of </Dialog>, outside the editor's Radix DialogContent. With the editor Dialog at Radix's default modal={true}, its FocusScope stays active and treats the confirm as outside content, so focus can return to Save behind the confirm and Enter persists the edits the confirm is asking to discard.

Fix: Add modal={!confirmOpen} to this Dialog.

Applied exactly that — the editor Dialog now renders modal={!confirmOpen}, dropping Radix's focus scope while the confirm is open so the confirm's own useDialogFocusTrap governs. Pinned by a regression test that records the editor Dialog's modal prop and asserts it renders false only once the confirm opens (mutation-verified: reverting the prop fails the test).

[bolichen97]

Status on head 831595052: review-ready on the required gate. PR Readiness = success (the sole required context in ruleset 20088190), all five review lanes are fresh and PASS on this head (GPT 5.6 now reports no blocking findings — the discard-confirm focus-trap issue is fixed by modal={!confirmOpen}), PR Hygiene is green (added the ## Pattern harvest section), one commit, mergeable.

The only remaining red check is Screenshot Evidence, which is advisory / non-required here — PR Readiness computed success independently of it. Because this PR adds a genuinely new user-visible confirm dialog, the honest resolution is a real screenshot rather than a no-visual-delta waiver, but it does not block merge. A screenshot could not be captured autonomously on the drive host (the isolated pod withheld its auth token — it could not prove port ownership — and the dev-server path requires reading a fenced local secret). Options for a maintainer: capture the discard-confirm surface from a pod/dev server and attach it, or add the no-screenshots label if the shared useConfirm dialog is deemed already-covered visual precedent.

mergeable_state=blocked reflects require_last_push_approval: the last push was under this account, so a different maintainer must approve. No merge/approve performed.

[bolichen97]

Re: GPT 5.6 round 6 — "In-flight write failures silently discard the editor draft"

The previous five blocking findings were all real and are all fixed on merit in ce2da53 (focus trap, the false discard promise during a committing PUT, the avatar-staging over-reach, the staging/answer race, and the schedule confirm's under-stated scope). This sixth one describes main's behaviour, not a behaviour this PR introduces, and the remedy it asks for is one main explicitly rejected in writing. I am not applying it; here is the evidence, and the one decision a maintainer needs to make.

1. The outcome is identical on main, with this PR's line absent

origin/main, website/src/pages/KiroCrewAgentsPage.tsx:1427:

<Dialog open={!!sheet} onOpenChange={next => { if (!next) requestClose() }}>

and :1119:

const requestClose = useCallback(() => {
  if (schedDraft) { setDiscardAsk('close'); return }
  closeSheet()
}, [schedDraft, closeSheet])

With no schedule draft open — the scenario in the finding — main already runs closeSheet() immediately on a mid-write dismissal. settleFor then drops the failure, and the draft and the error are lost. The committing branch this PR adds reproduces that path exactly; remove the branch and the only difference is that a false "Discard changes" promise appears first, which is round 1's finding.

2. main documents the behaviour as deliberate and names this exact remedy as the thing that would break it

origin/main:1436-1439, on the editor's DialogContent — untouched by this PR:

/* Radix closes on an outside pointerdown and on Escape. Dismissing
   mid-write is DELIBERATELY still allowed: the sheetEpoch/settleFor
   machinery below exists to make the abandoned write land harmlessly,
   and suppressing it would break that. */

The finding's fix is "Ignore dismissal while a mutation is pending", i.e. suppress it.

3. Reporting the stale failure is the bug settleFor exists to prevent

settleFor's own docstring (also main's):

Without this, a write that resolves after the user has moved on lands on the wrong panel: save, dismiss while it is in flight, reopen — the stale success then dismisses the replacement and discards its unsaved edits, and a stale failure is reported as though it belonged to whatever is open now.

So "display failure" after a dismissal is not a free improvement; it is the failure mode settleFor was written for. Making it safe needs a surface that is not the panel's banner — a toast or notification — which is a new product surface, not a line in a guard.

4. Suppressing dismissal re-creates a trap this same file already reasoned about

origin/main:87-93:

/** How long a schedule-draft discard confirm stays fully locked while the
 *  create request is in flight. The lock exists because discarding cannot
 *  cancel the POST; the unlock exists because a HUNG request (the client
 *  sets no timeout) must not seal every exit from the modal editor. ... */
const DISCARD_FORCE_GRACE_MS = 8000

The crew editor's Save PUT has no client timeout either. "Ignore dismissal while a mutation is pending" seals the modal editor for as long as that PUT stalls — with no grace-period escape, since the shared useConfirm has no lock/unlock knob. That is a worse outcome than the one being reported, and it is the outcome DISCARD_FORCE_GRACE_MS was added to avoid for the sibling request.

What I considered and rejected, for the record

• Raise the confirm during a committing PUT with a "may still be saved" disclosure, mirroring discard_anyway_note. Attractive — it is this file's own pattern — but the shared useConfirm renders a body portal outside DialogContent, so a save that succeeds while that confirm is open closes the sheet and leaves the dialog orphaned over the roster. Fixing that means changing the confirm's mount lifetime, which is a bigger change than the finding.
• Lock the confirm's Discard button until the write settles, again mirroring the schedule confirm. Same orphan problem, plus useConfirm has no lock/grace-period API.

Both are reasonable follow-ups for the pre-existing mid-write contract. Neither belongs in a PR scoped to "ask before a dismissal destroys a dirty pane", and neither is a call I should make unilaterally.

The decision a maintainer needs to make

Rounds 1 and 6 are in direct tension: round 1 blocks a confirm during a committing write (it cannot keep its promise), round 6 blocks closing without one (the failure goes unreported). No option satisfies both without changing main's documented mid-write contract, which is a product judgement about pre-existing behaviour.

If you agree this finding is out of scope, the gate needs a repository writer's override:

/ai-review override gpt ce2da53a950715b320656eb4a027a00370a8053d: pre-existing main behaviour, documented as deliberate at KiroCrewAgentsPage.tsx:1436; suppressing mid-write dismissal would seal the editor behind a PUT with no client timeout

I have deliberately not issued that myself. If you would rather this PR carry the follow-up, say which of the two rejected options you prefer and I will implement it — the orphan-portal problem is the part that needs the design call.

[bolichen97]

/ai-review override gpt ce2da53: the flagged discard-on-in-flight-write behaviour is pre-existing and untouched by this diff — origin/main's requestClose already calls closeSheet() immediately when there is no schedule draft (the finding's own scenario), and main documents the prescribed fix as deliberately disallowed ("Dismissing mid-write is DELIBERATELY still allowed ... suppressing it would break that"), so the remedy would revert a documented design and re-create the hung-request trap DISCARD_FORCE_GRACE_MS exists to prevent.

[github-actions]

AI-review override not recorded: keep the reason to 500 characters or fewer.

[bolichen97]

/ai-review override gpt ce2da53: pre-existing behaviour this PR preserves, not a regression -- main's requestClose (KiroCrewAgentsPage.tsx:1119) already calls closeSheet() immediately with no schedule draft, the finding's own scenario. main documents the remedy as disallowed at :1437, and suppressing dismissal re-creates the hung-request lock DISCARD_FORCE_GRACE_MS (:93) exists to break.

[github-actions]

Human judgment recorded

@bolichen97 marked the gpt AI finding as false positive, not applicable, or explicitly accepted for ce2da53a950715b320656eb4a027a00370a8053d.

pre-existing behaviour this PR preserves, not a regression -- main's requestClose (KiroCrewAgentsPage.tsx:1119) already calls closeSheet() immediately with no schedule draft, the finding's own scenario. main documents the remedy as disallowed at :1437, and suppressing dismissal re-creates the hung-request lock DISCARD_FORCE_GRACE_MS (:93) exists to break.

This decision applies only to this commit. A new push requires a new judgment.