From 6ea71700169094f5b9704f6ed7c044735566def2 Mon Sep 17 00:00:00 2001 From: Daniil Perkin Date: Sun, 27 Sep 2026 22:51:11 +0200 Subject: [PATCH 1/4] fix(buddy): edit the flag-to-PM question before confirming (#235) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A flag to the PM leaves the product in the hire's name, and the buddy composes the question — but the confirm showed only the button's label plus a read-only line under it, so the words that actually land in somebody's inbox could not be checked, let alone corrected, before they went out. The card now shows the composed question in an editable field above the confirm, and sends exactly what that field holds: handleConfirm is the single send path (the offer's button and a refusal's retry both come through it), it trims the text, and it refuses to send nothing — a blank flag is not a question, and the backend's blank-question refusal is a card with no field left to fix it in. Every action is now its own BuddyProposalCard rather than an inline branch of the map, because the draft is per-action state and it has to survive the switch from the offer to its refusal: the retry is precisely where the hire's own wording matters again. The field is frozen while the confirm is in flight, so what was sent is what is still on screen when the outcome lands. Field and control are ui/Field + ui/Textarea, not the legacy AutoResizeTextarea: the primitives already carry the label binding, the hint's aria-describedby and the disabled treatment, and ui/Textarea is what the standards ask for at every text field (the buddy composer is the documented composite exception; this is not one). The skip request's reason stays a static line under the buttons — it is a sentence to acknowledge, not one to compose — and the question field's doc in types.ts now says where it is shown. Tests: the static-preview assertion became a nine-test suite over the flag card (editable prefill, edited text sent, untouched text sent, trimming, empty and whitespace guards, freeze while confirming, decline, retry carrying the edited text). Red-checked: with the component reverted to the previous commit, seven of them fail. --- .../buddy/components/BuddyActionProposals.tsx | 434 ++++++++++-------- src/features/buddy/types.ts | 5 +- .../a11y/BuddyActionProposals.a11y.test.tsx | 32 ++ .../buddy/BuddyActionProposals.test.tsx | 233 +++++++++- 4 files changed, 494 insertions(+), 210 deletions(-) create mode 100644 tests/unit/a11y/BuddyActionProposals.a11y.test.tsx diff --git a/src/features/buddy/components/BuddyActionProposals.tsx b/src/features/buddy/components/BuddyActionProposals.tsx index 84e6d8ce..ff332a05 100644 --- a/src/features/buddy/components/BuddyActionProposals.tsx +++ b/src/features/buddy/components/BuddyActionProposals.tsx @@ -1,4 +1,7 @@ +import { useState } from "react"; import { Check, Info, Loader2, RotateCcw, TriangleAlert, Users, X } from "lucide-react"; +import { Field } from "../../../components/ui/Field"; +import { Textarea } from "../../../components/ui/Textarea"; import type { ProposedAction, ProposalRisk } from "../types"; import { BUDDY_ACTION_AMEND_CHECKLIST, @@ -17,6 +20,13 @@ type BuddyActionProposalsProps = { onDismiss: (messageId: string, actionId: string) => void; }; +type BuddyProposalCardProps = { + messageId: string; + action: ProposedAction; + onConfirm: (messageId: string, action: ProposedAction) => void; + onDismiss: (messageId: string, actionId: string) => void; +}; + /** * The confirm affordance for actions the buddy proposed. Nothing here has changed anything yet — * the buddy offered, and only a click on "Confirm" mutates. Each action shows its own state: the @@ -33,6 +43,12 @@ type BuddyActionProposalsProps = { * the only control for it taken away, and the mentor, which is never told what became of a * proposal, went on asking them to click a button that was no longer on screen. So the outcome * stays, and the offer comes back under it. + * + * Each one is its own {@link BuddyProposalCard} rather than an inline branch of the map below, + * because a flag to the PM carries the hire's own words and those are edited **before** confirming: + * the offer's draft is per-action state, and state cannot live inside a `map` callback — nor + * survive the switch from the offer to its refusal, which is precisely where a retry has to send + * the text the hire last saw. */ export function BuddyActionProposals({ messageId, @@ -44,111 +60,146 @@ export function BuddyActionProposals({ return (
- {actions.map((action) => { - if (action.status === "dismissed") { - return ( -

- Dismissed — nothing changed. -

- ); - } + {actions.map((action) => ( + + ))} +
+ ); +} - if (action.status === "resolved") { - return ( -
-

-

- {/* Under the refusal, not instead of it. The outcome is why it did not work and is +/** + * One proposed action, from offer to settled — and the only place the hire's own words can be + * edited before they leave the product. + * + * A `flag_to_pm` is the one proposal whose payload is a message a person reads: the buddy composes + * the question, but it arrives in a PM's inbox in the hire's name, so the whole question is shown + * in a field above the button and is theirs to correct. What is confirmed is therefore exactly what + * the field held (see `handleConfirm`), never the original proposal. Every other action echoes a + * target back verbatim and a stored team proposal confirms by id — nothing there is editable, so + * nothing there grows a field. + */ +function BuddyProposalCard({ messageId, action, onConfirm, onDismiss }: BuddyProposalCardProps) { + const isStored = "proposalId" in action; + const isFlagToPm = !isStored && action.action === BUDDY_ACTION_FLAG_TO_PM; + const isConfirming = action.status === "confirming"; + // Seeded once, on mount, and never re-seeded from the prop: a retry after a refusal has to send + // the text the hire last read, and an edit that silently reverted would be worse than no field. + const [editedQuestion, setEditedQuestion] = useState(() => + !isStored && action.action === BUDDY_ACTION_FLAG_TO_PM ? (action.question ?? "") : "", + ); + /** A flag with nothing in it is not a question — the confirm stays out of reach rather than + * letting the backend answer with a refusal the hire cannot act on. */ + const canConfirm = !isConfirming && (!isFlagToPm || editedQuestion.trim().length > 0); + + /** The offer's own button and a refusal's retry both come through here, so neither can send a + * question other than the one on screen — or an empty one. */ + const handleConfirm = () => { + if (!isStored && action.action === BUDDY_ACTION_FLAG_TO_PM) { + const question = editedQuestion.trim(); + if (!question) return; + onConfirm(messageId, { ...action, question }); + return; + } + + onConfirm(messageId, action); + }; + + if (action.status === "dismissed") { + return

Dismissed — nothing changed.

; + } + + if (action.status === "resolved") { + return ( +
+

+

+ {/* Under the refusal, not instead of it. The outcome is why it did not work and is the thing worth reading; this is only the way to try it once the reason has been dealt with. Never offered on success — running a confirmed action twice is how somebody claims the same task twice. Hire actions only: a stored team proposal is settled server-side once decided, so confirming it again could only be refused. */} - {!action.ok && "action" in action && ( - - )} - {/* Opening orientation is the one hire action whose result is content, not + {!action.ok && "action" in action && ( + + )} + {/* Opening orientation is the one hire action whose result is content, not just an outcome line: the packet renders right here in the thread instead of navigating to a page. Stored proposals have no `action` name to match — their payoff is always the outcome line. */} - {"action" in action && - action.action === BUDDY_ACTION_OPEN_ORIENTATION && - action.ok && } -
- ); - } + {"action" in action && action.action === BUDDY_ACTION_OPEN_ORIENTATION && action.ok && ( + + )} +
+ ); + } - const isConfirming = action.status === "confirming"; - const isStored = "proposalId" in action; - // Fail closed: a proposal whose risk did not survive the stream (or that arrived - // without its description) is not confirmable — an approval card for a project - // mutation must never guess how loudly to warn, nor confirm what it cannot show. - const isUnsupported = isStored && (action.risk === null || !action.preview); - const previewId = `buddy-proposal-preview-${action.id}`; + // Fail closed: a proposal whose risk did not survive the stream (or that arrived + // without its description) is not confirmable — an approval card for a project + // mutation must never guess how loudly to warn, nor confirm what it cannot show. + const isUnsupported = isStored && (action.risk === null || !action.preview); + const previewId = `buddy-proposal-preview-${action.id}`; - return ( -
- {/* Shown before the press, not after it, and only where the payload is *content*. + return ( +
+ {/* Shown before the press, not after it, and only where the payload is *content*. `place_checklist` is the one action whose confirm writes the mentor's own sentences onto a surface the hire owns — the same reason the assessment proposal names the skill and the level rather than saying "Save this". The lines are in the reply above as well; having them here is what makes the two comparable, so a list that does not match what was written is visible before it is kept, not after. */} - {!isStored && action.checklistItems && action.checklistItems.length > 0 && ( -
- {action.checklistTitle && ( -

- {action.checklistTitle} -

- )} - {/* Named as an addition when it is one. The card keeps everything it already has, + {!isStored && action.checklistItems && action.checklistItems.length > 0 && ( +
+ {action.checklistTitle && ( +

{action.checklistTitle}

+ )} + {/* Named as an addition when it is one. The card keeps everything it already has, and these lines go after it — saying so is the difference between agreeing to three new steps and agreeing to whatever the list becomes. */} - {action.action === BUDDY_ACTION_AMEND_CHECKLIST && ( -

- Added to the end of that list — nothing on it changes: -

- )} - {action.action === BUDDY_ACTION_TICK_CHECKLIST && ( -

- Ticked off on that list — nothing else on it changes: -

- )} -
    - {action.checklistItems.map((item, index) => ( -
  • - · {item} -
  • - ))} -
-
- )} + {action.action === BUDDY_ACTION_AMEND_CHECKLIST && ( +

+ Added to the end of that list — nothing on it changes: +

+ )} + {action.action === BUDDY_ACTION_TICK_CHECKLIST && ( +

+ Ticked off on that list — nothing else on it changes: +

+ )} +
    + {action.checklistItems.map((item, index) => ( +
  • + · {item} +
  • + ))} +
+
+ )} - {/* The one offer that *replaces* something already on a card, so it shows both: the + {/* The one offer that *replaces* something already on a card, so it shows both: the line as it reads now and as it would read. Only the new wording would be asking them to agree to a change they would have to go and diff for themselves. @@ -159,118 +210,129 @@ export function BuddyActionProposals({ Keyed off the action, like the amend and tick branches above, so a future action that happens to carry both fields does not render as a rewording. */} - {!isStored && - action.action === BUDDY_ACTION_REWORD_CHECKLIST && - action.lineBefore && - action.lineAfter && ( -
- - Currently: - {action.lineBefore} - - - Would become: - {action.lineAfter} - -
- )} + {!isStored && + action.action === BUDDY_ACTION_REWORD_CHECKLIST && + action.lineBefore && + action.lineAfter && ( +
+ + Currently: + {action.lineBefore} + + + Would become: + {action.lineAfter} + +
+ )} - {/* Whitespace kept: the note's first line becomes the card's heading, so a preview that + {/* Whitespace kept: the note's first line becomes the card's heading, so a preview that reflowed it would not be showing what would be kept. */} - {!isStored && action.noteText && ( -

- {action.noteText} -

- )} - {/* What the manager is agreeing to comes FIRST, in the buddy's own words, and the + {!isStored && action.noteText && ( +

+ {action.noteText} +

+ )} + {/* What the manager is agreeing to comes FIRST, in the buddy's own words, and the confirm button describes it (aria-describedby): the target is the stored id, so this text is the offer's full description — never a summary the client recomposed, and never read after the button that acts on it. */} - {isStored && action.preview && ( -

- {action.preview} -

- )} + {isStored && action.preview && ( +

+ {action.preview} +

+ )} - {/* A stored proposal warns about itself before it is even clicked: how much it would + {/* A stored proposal warns about itself before it is even clicked: how much it would change decides how loudly the card speaks, before any confirm happens. */} - {isStored && action.risk !== null && } + {isStored && action.risk !== null && } - {isUnsupported && ( - <> -

- This proposal arrived without its details, so it cannot be confirmed here. Ask the - buddy to propose it again. -

-
- -
- - )} - {!isUnsupported && ( -
- {/* `action.label` is written by the model, so its length is not ours to + {isUnsupported && ( + <> +

+ This proposal arrived without its details, so it cannot be confirmed here. Ask the buddy + to propose it again. +

+
+ +
+ + )} + {/* The one proposal whose payload is a *message*: the buddy composes the question, + the hire sends it under their own name, and a person reads it — so it is shown in + full, editable, above the button that sends it. Two things follow from that, and + both are enforced: what goes out is the field's own text (`handleConfirm`), and an + empty field cannot be confirmed at all. */} + {isFlagToPm && ( + +