fix(chat): remove only the complete generated suffix on a ChatPane chip un-toggle

[leonlaiyc]

Problem / Motivation

In a grid pane, un-toggling a still-lit follow-up chip can delete text the user
typed themselves.

ChatPane's toggle removed the last occurrence of ", " + option anywhere
in the composer. That is correct only while the text the handler appended is
still present. Once the user has deleted that appended tail by hand — a normal
thing to do, and the chip stays lit because nothing tells it otherwise — the
search falls back onto the next-most-recent match, which is the user's own
draft:

step	composer
user types	Discuss, Alpha home
clicks the Alpha chip	Discuss, Alpha home, Alpha
user deletes the appended , Alpha by hand	Discuss, Alpha home
clicks Alpha again to un-toggle	Discuss home ← their , Alpha is gone

ChatPage had exactly this defect and was rewritten for it; the review of #6092
counted ChatPane as the unfixed sibling, still on the lastIndexOf splice.

Why it matters

It is silent, unprompted destruction of unsent user input, in the one widget
whose entire contract is "if the user edited the text so it no longer matches,
leave the text alone" — a promise both files' own comments make. Nothing warns,
nothing undoes it, and the deleted span is exactly the kind of short phrase that
is easy not to notice before pressing Enter.

It is also a divergence between two panes that are meant to behave identically:
the same click on the same draft produces different text in the main chat and in
a grid pane.

What changed (motivation → approach → change)

Symptom: un-toggling a chip removes a matching substring of the user's draft.

Root cause: the removal was defined by pattern (lastIndexOf(', ' + o)),
not by provenance. A substring search cannot distinguish text this handler
generated from identical text the user typed, so once the generated copy is gone
it happily takes theirs.

Change: adopt ChatPage's removal, which is defined by provenance instead.
The picked options are appended as one ordered suffix, so the only text the
handler may take back is that whole structure, still intact at the end:

• prev === pickedSuffix → the composer is nothing but the suffix; replace it
  with the remaining options.
• prev.endsWith(', ' + pickedSuffix) → strip exactly that tail and re-append
  what is left picked.
• otherwise → the user edited it; return prev untouched (the chip still
  un-highlights, as before).

website/src/components/ChatPane.tsx only — a 14-line block swapped for the
12-line one ChatPage.tsx:8331-8344 already runs. No new helper, no shared
extraction: the two blocks are now textually parallel, which is what makes the
next drift visible in review.

Behavioural delta beyond the bug: a draft the user has appended to after the
generated suffix (Alpha, Beta and hurry) now keeps its text on un-toggle
instead of being spliced. That is the stated contract, and it is what
ChatPage already does.

Tests

website/src/test/ChatPane.followUpOptions.test.tsx — one added case,
leaves earlier draft text alone when the user already deleted the appended option, mirroring the ChatPage test of the same name. It drives the table
above through the real component and asserts the composer still reads
Discuss, Alpha home.

Red-before on this branch's parent (3a5824d20), with the fix reverted:

AssertionError: expected 'Discuss home' to be 'Discuss, Alpha home'
Expected: "Discuss, Alpha home"
Received: "Discuss home"
 Tests  1 failed | 32 passed (33)

After the fix, and including the sibling suite that pins the behaviour being
ported from:

npx vitest run src/test/ChatPane.followUpOptions.test.tsx src/test/ChatPage.followUpToggle.test.tsx
 Test Files  2 passed (2)
      Tests  47 passed (47)

The pre-existing unselecting an option splices its own appended text, never a matching substring of the draft case (draft Please, Alphabet) and the
multi-option accumulate/remove cases stay green — they are the regression
control for the case this change does not alter.

npx tsc --noEmit clean; npx eslint clean on both touched files.

Manual verification

N/A — unit coverage sufficient: the added test drives the real ChatPane
through React Testing Library and asserts on the live composer value, so it
exercises the same code path a click does.

Related Issues

Residual named by the review of #6092 (ChatPane.tsx counted as the unfixed
sibling of the ChatPage.tsx rewrite). No separate issue.

Pattern harvest

Rule candidate: review-prompt

Pattern: "text removal identified by substring match rather than by provenance"
— when a handler appends generated text into a field a user also edits, the
undo must be defined by the exact structure it generated (anchored at a known
end, matched whole), never by searching for a pattern that the user's own input
can also satisfy. indexOf → lastIndexOf looks like the fix and is only a
narrower guess; the generated span has to be identified, not searched for.

Checklist

• At most two commits (one is the norm), with a Conventional Commits title (feat|fix|docs|refactor|perf|test|chore|ci|build|revert: ...)
• Existing tests pass and new tests added for new functionality
• Self-review completed; code follows project style guidelines
• Documentation updated (if applicable)
• No secrets, credentials, or internal references in the diff

Contribution License Agreement

[bolichen97]

Open PR relationship audit

This is a consolidated, point-in-time code-level audit note. It compares complete merge-base diffs and current/merged code; it does not treat a shared topic as duplication or partial coverage as completion.

Relationship findings

• This PR is OVERLAPPING with PR #5895. The goals differ or the implementations can complement each other; this is not a duplicate claim. Recommended action for PR #7616: KEEP. The merged PR introduced the defect rather than fixing it; no later merged commit has touched the block. Files: website/src/components/ChatPane.tsx.
• This PR is OVERLAPPING with PR #6092. The goals differ or the implementations can complement each other; this is not a duplicate claim. Recommended action for PR #7616: KEEP. 6092 is the source of the ported logic, not coverage of it. The grid pane is still defective on current main, so this PR closes the divergence 6092 left behind. Files: website/src/pages/ChatPage.tsx.
• This PR is OVERLAPPING with PR #6823. The goals differ or the implementations can complement each other; this is not a duplicate claim. Recommended action for PR #7616: KEEP. Complementary changes to the same wiring block; no duplication of behavior. Files: website/src/components/ChatPane.tsx.
• This PR is OVERLAPPING with PR #7255. The goals differ or the implementations can complement each other; this is not a duplicate claim. Recommended action for PR #7616: KEEP. Independent files and independent goals; the interaction is a stale comment reference at worst. Files: website/src/pages/chat/useChatPageComposerController.tsx.

No PR, Issue, label, branch, or review state was changed by the relationship-note portion of this audit.

[bolichen97]

Rebased onto current main (41dcadf2c) by a maintainer as part of the 2026-09-08 open-PR audit. New head: 3e08d45cd (was b46adbaf5).

Clean rebase, no conflicts. The un-toggle block on main was byte-identical to this PR's pre-image (only shifted), so both hunks replayed unchanged and the PR's behaviour is untouched.

Gates run locally on the rebased tree:

• npx tsc --noEmit -p website/tsconfig.json - clean
• npx vitest run src/test/ChatPane.followUpOptions.test.tsx - 33 passed

Please review the rebased head. Note that a maintainer push makes the maintainer the last pusher, so under this repo's last-push rule a second approver is needed. Reply here if anything looks wrong.

[github-actions]

UX Review (Fable 5, fork) — ✅ PASS

UX-level review of 3e08d45cdfab9fe4312df6caf7b8970c06370cbe via the fork AI-review pipeline — updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

UX-Verdict: PASS

Invisible-surface bug fix: the un-toggle now stops silently deleting user drafts and matches ChatPage's exact behavior — no new control, string, or state to evidence.

[UX-REVIEWED] 3e08d45

[github-actions]

Design Review (Fable 5, fork) — ✅ PASS

Design-level review of 3e08d45cdfab9fe4312df6caf7b8970c06370cbe via the fork AI-review pipeline — updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

Design-Verdict: PASS

Root-cause fix — removal keyed to provenance (the intact generated suffix) instead of substring search — ported faithfully from the ChatPage sibling, with a red-before test.

Suggestions

• This is the second time these twin toggle blocks drifted (fix(chat): remove follow-up options only from generated suffix #6092 fixed ChatPage, ChatPane lagged until now); a shared pure helper removePicked(prev, picked, o) tested once would end the drift class structurally rather than relying on reviewers noticing textual parallelism — worth a follow-up, not this PR.

[DESIGN-REVIEWED] 3e08d45

[github-actions]

First Principles Review (Fable 5, fork) — ✅ PASS

Premise-level review of 3e08d45cdfab9fe4312df6caf7b8970c06370cbe via the fork AI-review pipeline — 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.

All verification done. The base ChatPane.tsx:875-882 still carries the lastIndexOf splice; reading it against the description's table confirms the defect ("Discuss, Alpha home".lastIndexOf(", Alpha") = 7 → "Discuss home"), ChatPage.tsx:7769-7783 already runs the ported removal, its sibling test exists at ChatPage.followUpToggle.test.tsx:240, and a grep for lastIndexOf( across website/src (70 hits) shows every other use is a path/newline utility — no remaining follow-up-splice sibling. The deleted comment's protected case ("Please, Google" mid-word splice) keeps its own test, untouched by the diff.

First-Principles-Verdict: PASS

Verify npm run test passed in full — its pretest jscpd gate can fail on the now-identical 12-line blocks, and the description only reports targeted vitest, tsc, and eslint runs.

What this change ships

Intent: stop a chip un-toggle in a grid pane from silently deleting text the user typed — a FIX.

Inventory (3 items)

1. Un-toggling a still-lit chip no longer deletes the user's own matching draft text — justified
2. Text typed after the appended options now survives un-toggle instead of being spliced — justified
3. A new test pins the un-toggle-after-manual-delete case — justified

The defect is derived, not asserted: it reproduces by reading base ChatPane.tsx:876 against the description's table, and the added test fails on base. The fix sits at cause level (removal defined by generated structure, not substring search). The symmetry framing is not the justification — the harm stands without the twin. Grep count: lastIndexOf( has 70 hits in website/src; the only follow-up-option splice was ChatPane.tsx:876, fixed here, so zero unfixed siblings remain.

[FIRST-PRINCIPLES-REVIEWED] 3e08d45

[github-actions]

Opus 4.8 Review (fork) — ✅ no blocking findings

Reviewed 3e08d45cdfab9fe4312df6caf7b8970c06370cbe via the fork AI-review pipeline; updated in place on each push.

Review details

No findings.

[OPUS-REVIEWED] 3e08d45

[github-actions]

GPT 5.6 Review (fork) — 🔴 changes requested (blocking)

Reviewed 3e08d45cdfab9fe4312df6caf7b8970c06370cbe via the fork AI-review pipeline; updated in place on each push.

1 of 1 blocking finding(s) are security-class and were withheld from adjudication, so the blocking verdict stands.

BLOCKING -- website/src/components/ChatPane.tsx:880 -- Exact-suffix drafts still lose user text
if (!prev.endsWith(delimitedSuffix)) return prev
Draft Discuss, Alpha -> select Alpha -> manually delete appended tail -> un-toggle -> composer becomes Discuss.
Anchor: residual/crash-data-loss-corruption
Fix: Invalidate the picked state on manual composer edits, or track generated-text provenance before removing it.
[BLOCK-MERGE] 3e08d45
[GPT-REVIEWED] 3e08d45

Adjudication (Opus 4.8) — is blocking on each finding proportionate?

I've read the prompt, the findings, the diff, and the base code. There are 0 adjudicable findings and 1 fenced finding (F1).

F1 — ChatPane.tsx:880, exact-suffix draft loss (FENCED)

Tracing the new code against the base tree:

• The picked chip must be lit — confirmed at ChatPane.tsx:865 (if (followUpPickedRef.current.has(o))).
• The whole-suffix short-circuit must miss — patch line 35 (if (prev === pickedSuffix) return remainingSuffix).
• The draft must end with ", " + pickedSuffix, and for this to be data loss that trailing text must be the user's own (they manually deleted the generated append) — patch lines 36-38 (if (!prev.endsWith(delimitedSuffix)) return prev then prev.slice(0, -delimitedSuffix.length)).

The defect is real: draft Discuss, Alpha → pick Alpha (append → Discuss, Alpha, Alpha) → delete the appended tail by hand (Discuss, Alpha) → un-toggle → the endsWith(", Alpha") branch strips the user's own trailing , Alpha, yielding Discuss.

Harm rung: LOW. The lost text is unsent composer draft in local state, immediately visible in the composer and trivially re-typed — not persisted data, not corruption despite the crash-data-loss-corruption anchor.

Recovery: no handler auto-repairs it, but it is visible on the spot and self-corrects on the user's next keystroke.

Rarity: requires an exact string coincidence (the user's trailing draft equals ", " + join(picked options)) together with the specific pick → manual-tail-delete → unpick sequence. This is the same residual ChatPage carries by design (#6092), which this PR is aligning ChatPane to. A human would plausibly accept this residual.

Evidence record complete; harm genuinely bounded UI draft loss → FLAG.

[ADJUDICATION] 3e08d45 total=0 uphold=0 downgrade=0
[GPT-ADJUDICATED] 3e08d45

[ADJUDICATION-FENCED] 3e08d45 fenced=1 flagged=1
FLAG F1 website/src/components/ChatPane.tsx:880 -- Loss is a handful of unsent, immediately-visible composer draft characters, trivially re-typed, triggered only by an exact-string coincidence plus a pick→manual-delete→unpick sequence — the same residual ChatPage ships by design (#6092).
[GPT-ADJUDICATED-FENCED] 3e08d45

🏷️ Fenced finding(s) machine-flagged as likely edge case

The security fence keeps these findings blocking regardless of adjudication; the only clearance path is a human override recorded by a repository writer, who must independently verify a rationale before recording it — it is machine-authored, and a wrong override on a security-class finding ships exactly the class the fence exists to stop. (This lane's comment deliberately carries no override command.)

• F1 website/src/components/ChatPane.tsx:880 — Loss is a handful of unsent, immediately-visible composer draft characters, trivially re-typed, triggered only by an exact-string coincidence plus a pick→manual-delete→unpick sequence — the same residual ChatPage ships by design (fix(chat): remove follow-up options only from generated suffix #6092).