fix(buddy): edit the flag-to-PM question before confirming (#235)

[daniilperkin]

Show — and edit — the exact message before a Buddy escalation is confirmed

Closes #235 · Stacked on #267 (feature/311-buddy-onboarding-tutor) — this diff is the delta on top of that branch.

What this changes

flag_to_pm gains the one thing the issue asked for: the composed question is shown in an editable field above the confirm, and the confirm sends exactly what that field holds.

• BuddyActionProposals becomes a thin list; each action is its own BuddyProposalCard, so an offer has an identity of its own across every status.
• handleConfirm is the single send path — the card's confirm button, and the same button again after a refusal — sending { ...action, question: editedQuestion.trim() }.
• The confirm is disabled while the field is empty or whitespace-only; the field is frozen while a confirm is in flight.
• The hire's wording lives in the session (actionDrafts, src/features/buddy/actionDrafts.ts), not in the card: closing the dock, handing the conversation over to /buddy, or any re-render keeps it. It is let go of once the flag actually went out — or once the hire declined it — because a card re-rendered later must not offer wording that already left the product.
• ui/Field + ui/Textarea, not the legacy AutoResizeTextarea: the shared control already carries the label binding, the hint's aria-describedby and the disabled treatment.
• No change to the confirm payload in useBuddyConversation: it already sends action.question, so an edited action flows through untouched.

After review round 1 (2026-09-28)

A refused hire offer now keeps its card — reason above it, field and buttons intact — instead of collapsing to a compact outcome line with a separate "Try again" button underneath:

• the hire can see and correct what would go out before retrying (before, the field was unmounted, so the wording existed only in memory);
• pressing the button again no longer swaps the whole card in and out (the layout jump);
• the separate "Try again" button is gone: the card's own confirm is the retry, and it is disabled rather than silently inert when the field is empty;
• the reason stays on screen while the retry is in flight, so pressing the button does not blank the sentence being acted on.

Acceptance criteria (#235)

• A flag_to_pm proposal displays the complete proposed question.
• The user can edit the question before sending it — on the first attempt and on a refusal's retry.
• Confirming the action submits exactly the displayed text.
• Cancelling the action does not create a knowledge request (a hire offer dismisses locally — useBuddyConversation.ts, unchanged).
• Works in both surfaces: the full page and the dock render the same BuddyThread → BuddyActionProposals, and because the wording is held by the session, switching between them (or closing the dock) keeps it.

Verification

• npm run try (install → format:check → build → lint → unit → a11y) on Node 22: green — unit 302 files / 2929 tests passing, a11y 52 files / 65 tests passing, build + lint + format clean.
• tests/unit/features/buddy/BuddyActionProposals.test.tsx: 42 tests, 9 of them the flag card; tests/unit/features/buddy/useBuddy.test.tsx pins the session draft (kept on a refusal, dropped once sent); tests/unit/a11y/BuddyActionProposals.a11y.test.tsx covers the editable field and the refused card.
• Red-checked: with the offer-keeping behaviour reverted, four of the new tests fail (retries a refusal with the text the hire last edited, cannot retry a refusal with an empty field, keeps the reason and offers it again under it, keeps the hire's wording for a refused offer and lets go of it once it went through).
• Gates green, new/updated unit tests red-checked
• Manually verified end-to-end in the running app (not done in this PR)

Merge note

Retarget to dev after #267 merges — GitHub does not retarget an open PR while the base branch still exists; it is one command (gh pr edit <N> --base dev).

Second review round — both "worth a look" points and both nits, each confirmed against the code and fixed

• actionDrafts is cleared wherever the conversation is reset. startFreshVisit and the team-mode switch effect both reset messages, opener, error and composer draft; the flag wording now goes with them. It was never a correctness bug (the keys are per-message UUIDs, so a leftover could not collide), but a long-lived tab kept every wording the hire ever typed, on exactly the two transitions that reset everything else.
• The emptied field says why it cannot be sent. It passes Field's error ("Write a question before sending.") — announced, aria-invalid, and described by it — instead of leaving a disabled button with no reason. On the paths the backend produces it appears in response to the hire's own clearing — the buddy refuses to compose a flag with no question; a proposal that somehow arrived without one would show the error from the first render, which is right, because a disabled confirm still has to say why.
• The field is handed back what it sent. handleConfirm writes the trimmed question into the session draft, so a retry after a refusal shows the copy the PM would have read, not the padded typing.
• One continuous draft-lifecycle test (refused → kept in the session → retried → sent, in one session), plus a test per reset path (fresh visit, switch) and an a11y case for the emptied field.

Red-checked: with src/ reverted and the tests kept, exactly these four tests fail.

Third round — the deep review's findings, fact-checked before they were fixed

Its mechanisms held up. Two of the findings turned out to be inherited from #311 rather than brought in here, and one was a claim about the code rather than a defect in it.

• A retry that failed on the wire no longer tells two stories. The outcome line stands down for status: "error" (it still holds while the retry is in flight, which is why it lives above the card), so only "Couldn't reach the server — try again." remains. New test, red-checked.
• The refusal stopped wearing a checkmark. AlertCircle in the warning tone instead, so the mark and the sentence agree — the shape carries the meaning, per the palette's colour-blind rule.
• The field is required — asterisk plus aria-required on the textarea, now that Field propagates it (dev's rework); no native validation attribute is set, so nothing blocks typing.
• The claim about when the error appears is corrected above. It is true for every proposal the backend produces — it refuses to compose a flag with no question; one that somehow arrived without a question would show the error from the first render, which is right: a disabled confirm still has to say why.
• BuddyDock.test.tsx's duplicate React keys are gone (inherited): its message helpers take an optional id, and the one test rendering two assistants passes its own.
• Left alone on purpose: the keystroke re-render observation (Finding 5). EMPTY_ACTION_DRAFTS is what keeps the memoised rows stable while a flag is typed into; a BuddyDraftProvider-style context is the follow-up if flag editing ever grows.

[daniilperkin]

Why this PR shows "This stack has conflicts that must be resolved"

That banner is about the layer below, not about this branch — #272 only shows it because a stack can't merge while any layer can't.

feature/311-buddy-onboarding-tutor (#267) is ~99 commits behind dev, and both sides changed the same file, src/features/buddy/components/BuddyComposer.tsx (dev: the caret hand-off/return around submit; #267: the typedRef focus for drafts arriving from outside). So the bottom layer can't merge into dev, and that blocks everything above it. #267 reports CONFLICTING/DIRTY. This PR itself is clean against its own base: one commit, no conflicts, checks green.

What we're doing about it

Waiting for David to resolve #267 on his own branch. The conflict is his code against dev's, so the call is his — we're not merging into or force-pushing someone else's branch.

Please don't press Rebase stack on this stack in the meantime: it rewrites and force-pushes every branch in the stack (this one included) to replay #267's 23 commits onto dev.

For whoever does it, the cheap path — dry-ran locally end to end, verified:

1. Merge origin/dev into feature/311-buddy-onboarding-tutor: one content conflict, BuddyComposer.tsx, both sides additive — keep dev's caret hand-off/refocus and the typedRef external-draft focus, one below the other.
2. tests/unit/features/buddy/BuddyComposer.test.tsx: the handleSubmit stub must return true now that dev's contract returns a boolean. Git merges that file cleanly and only tsc sees the mismatch.
3. That push is a plain fast-forward from the branch tip — no force, no history rewrite — and Make the buddy the tutor along the onboarding path #267's approval survives it (dismiss_stale_reviews_on_push: false on the ruleset); only CI re-runs.

Dry-run result: tsc -b and prettier clean, 23 buddy test files / 157 tests green. Ask me and I'll hand the resolved diff over.

After #267 merges

• GitHub retargets this PR's base to dev automatically. Since Make the buddy the tutor along the onboarding path #267 will land as a merge commit (the repo's convention — every first-parent commit on dev), 311's tip stays an ancestor of dev, so the diff here stays our one commit.
• If GitHub still reports a conflict at that point, we back-merge dev into this branch — the pattern the repo already uses (466a1b20 Merge remote-tracking branch 'origin/dev' into feature/KB-updates-final) — then merge.

Do not squash-merge #267: squashing rewrites the SHAs the layers above are based on, which breaks the stack and would make this PR show #267's changes as its own.

[kiranfin]

Review

I did not find anything that should block landing this on top of #267; the two points below are worth a look, not blockers.

🟡 Worth a look

1. actionDrafts is never cleared when the conversation is reset — src/features/buddy/hooks/useBuddyConversation.ts:437-459 (startFreshVisit) and :863-895 (the team-mode switch effect)
   Both places reset messages, openerAction, openError, draft and more, but not actionDrafts. Keys are messageId:actionId from freshly generated UUIDs, so a leftover entry can never collide with a later card — this isn't a correctness bug — but every flag the hire ever edits across a long-lived tab stays in memory forever, since the buddy session never unmounts. Given how deliberately this file resets every other piece of session state on these two transitions, clearing actionDrafts alongside them would be the consistent thing to do.

2. An empty field disables the confirm silently, with no visible reason and no aria-describedby — src/features/buddy/components/BuddyActionProposals.tsx:309-323
ui/Field has an error prop built exactly for this ("its presence is the error state: it colors the border, sets aria-invalid, and is announced," per its own doc comment), but the card renders Field here without one. A keyboard or screen-reader user who clears the field and tabs to the disabled "Flag this to your PM" button gets nothing telling them why it won't activate. Passing something like error="Write a question before sending." when editedQuestion.trim() is empty would close that gap and match how the rest of the app treats invalid fields.

🟢 Nits / cleanup

• handleConfirm sends editedQuestion.trim() but never writes the trimmed value back into actionDrafts (BuddyActionProposals.tsx:120-132), so a refused retry's field keeps whatever leading/trailing whitespace the hire originally typed. Harmless — invisible in a textarea — but slightly at odds with "what is confirmed is what the field held."
• The new test "keeps the hire's wording for a refused offer and lets go of it once it went through" (tests/unit/features/buddy/useBuddy.test.tsx:480-493) actually renders two independent hook instances via two separate confirmOnce calls, rather than following one offer from refusal through to acceptance in one session. It proves both halves of the claim, just not as the one continuous story the name implies.

Overall this is a tight, well-tested PR. Nothing here needs to hold up the stack.