feat(chat): Quote / Ask on selected text in ChatPane (Members + split) - #8947
feat(chat): Quote / Ask on selected text in ChatPane (Members + split)#8947CrysisDeu wants to merge 1 commit into
Conversation
Design Review (Fable 5) — ✅ PASSDesign-level review of Design-Verdict: PASS One shared chat-core seam replaces three would-be copies and a fragile event/poll bridge; each host injects only what it owns — sound, root-cause shape. Suggestions
[DESIGN-REVIEWED] f3aeec2 |
First Principles Review (Fable 5) — ✅ PASSPremise-level review of All verification is done. The description's claims held up against the repo: the seed race fix is cause-level (the rAF poll and First-Principles-Verdict: PASS One capability (Quote/Ask), one seam, wired to two hosts that lacked it; the old event-race workaround is deleted, not wrapped. What this change shipsIntent: let a reader quote or ask about selected assistant text on every transcript surface, not only the main chat — an ADDITION.
Checks that came back clean: [FIRST-PRINCIPLES-REVIEWED] f3aeec2 |
Opus 4.8 Review — ✅ no blocking findingsReviewed Review detailsI've verified the single candidate against the actual code. The candidate claims that quoting from a non-focused split pane lands keyboard focus in the wrong pane's composer, because That assumption is false. The candidate's own confidence line flagged exactly this unverified premise ("if selection focus always marks the origin pane, the focus lands correctly"), and it does. No findings. [OPUS-REVIEWED] f3aeec2 Verdict parsed from the review's SHA-scoped output markers for commit False positive or not applicable? A repository writer can comment: |
UX Review (Fable 5) — 🟡 CONCERNSUX-level review of I have everything needed: the blind read, the full diff of user-facing surfaces, the PR intent, and the rendered screenshots. Reconciliation: the Quote/Ask toolbar, the seeded Side Chat (both hosts), the quoted composers, and the drawer's "Details" back-action are all screenshotted and were read correctly by the blind reader (Quote and Ask rated "a guess", but the guesses match the implemented behavior); the drawer swap has a stated rationale plus a committed recording; the one control shown nowhere is the conditional "Side Chat" return button in the details drawer header. UX-Verdict: CONCERNS Blind reader used every primary control correctly, but the draft-pending "Side Chat" return button in the details drawer appears in no screenshot. Watch
Evidence gaps
[UX-REVIEWED] f3aeec2 |
GPT 5.6 Review — ✅ no blocking findings (all downgraded on adjudication)GPT 5.6 flagged blocking issues on This comment is updated in place on each push. Review detailsBLOCKING -- website/src/pages/ChatPage.tsx:6600 -- Failed pane switch silently blanks the transcript Adjudication (Opus 4.8) — is blocking on each finding proportionate?F1 — The anchored blocking rule Harm rung: LOW — a rare, visible, self-correcting blank pane requiring split view + Ask-in-non-active-pane + a transient (non-404) slot fetch rejection while connected. Recovery is the tested retry path. The switchSlot fire-and-forget seam is used identically by every other navigation caller in this file (tab strip, sidebar row, False positive or not applicable? A repository writer can comment: |
148ebb4 to
75bf525
Compare
|
|
|
|
|
|
|
|
/ai-review override gpt 75bf525: max-two-buttons-per-row on the selection toolbar is a grandfathered row — SelectionToolbar already renders Quote / Ask in Side Chat / Copy for ChatPage on main via useSelectionActions; this diff adds no markup to that row, it only stops ChatPane withholding the two callbacks the shared component takes (same clause as #5895 FollowUpBar and #5892 QueueStack onEdit). |
Human judgment recorded@CrysisDeu marked the gpt AI finding as false positive, not applicable, or explicitly accepted for
This decision applies only to this commit. A new push requires a new judgment. |
75bf525 to
c0ebaf8
Compare
f936f40 to
af095d9
Compare
af095d9 to
38a1bf8
Compare
… view) Selecting text in a Crew Members thread (or a split-view pane) offered Copy only: AssistantMessage's selection toolbar draws the actions its host hands it, and ChatPage was the only host passing onQuote/onAsk — quote's FlyingQuote-into-composer and ask's open-/side-and-seed were ChatPage-local. Extract both into one chat-core seam, chat-core/composer/selectionActions (useSelectionQuoteAsk + quoteIntoDraft + seedSideChat), and make ChatPage and ChatPane both consume it — no second implementation. Hosts differ only in what they own: the composer draft, and how a Side Chat surface for a slot is brought on screen (openSideChat). Quote is always offered; Ask exactly when the host provides an opener (capability by omission, like onOpenFull). - app-sdk: ChatMessageList/MessageRenderContext gain onQuote/onAsk; the default assistant row passes them through. - ChatPane: wires the seam to its own composer (FlyingQuote lands in the pane) and takes an openSideChat prop. - Split view: SessionGridView threads ChatPage's opener, which re-binds the activity panel to the pane's slot (switchSlot) before opening the side tab. - Members page: the detail drawer gains a Side Chat view bound to the member's slot (reuses existing "Side Chat"/"Details" strings — no new copy). - SideChat: the side-seed event now names its slot; a SideChat bound to another slot ignores it, and the seed poll waits for the named slot's composer (data-side-chat-slot) rather than any Side Chat's. - ChatPage: only change is consuming the hook (behaviour unchanged) plus the one-prop grid wiring. Tests (mutation-oriented pins): selectionActions unit tests, ChatPane selection-actions test, MembersPage Side Chat drawer test, SideChat slot-aware seed, ChatPage Ask seed regression. Capture harness website/scripts/capture-members-selection-quote-ask.mjs with three asserted frames under temp-screenshots/members-selection-quote-ask/.
38a1bf8 to
f3aeec2
Compare
|
Kiro Crew Auto-Pipeline [operator: chenmingwei23]: not a review -- issue #8930 was routed to this pipeline and this PR already covers it, so I am standing down rather than opening a parallel branch. Two things I found while checking coverage that are yours to act on. No closing keyword for the issue. Conflicting against One note in your favour on the issue itself: I checked whether #8930 is really a regression and it is not. Coverage evidence I read: |
Summary
Selecting text in a Crew Members thread (and in a split-view pane) offered Copy only.
AssistantMessagedraws the selection actions its host hands it (useSelectionActions(onQuote, onAsk),AssistantMessage.tsx:212), andChatPagewas the only host passingonQuote/onAsk— Quote'sFlyingQuote-into-composer and Ask's open-/side-and-seed were ChatPage-local.This PR extracts both into one chat-core seam and wires
ChatPaneto it, so the Members page and split view get Quote / Ask in Side Chat / Copy with the same behaviour as the main chat. No second implementation is left behind.The seam —
website/src/chat-core/composer/selectionActions.ts—useSelectionQuoteAsk({ slot, setInput, revealComposer, openSideChat })→{ onQuote, onAsk, quoteFlight, endQuoteFlight }. Hosts hand it only what they own: the composer draft, how to bring the composer into view after a quote, and how to bring a Side Chat surface on screen (openSideChatabsent = no Ask; no slot = no Ask). chat-core imports nothing frompages/.quoteDraft.ts—quoteIntoDraft(prev, text), the one blockquote shaping for the main composer, a pane's composer and the Side Chat seed (three copies collapsed).sideChatDrafts.ts— the per-slot Side Chat draft store, in-memory and subscribable (useSideChatDraft(slot)viauseSyncExternalStore). It is the one place a Side Chat's unsent text lives: typing, a failed request handing its text back, and Ask's seed all write it. The seed is a store write (seedSideChatDraft(slot, selection)appends the blockquote and bumps aseedTick), not an event fired at the panel — a store entry waits for the panel to mount and read it, so a Side Chat that comes up a frame late (afterswitchSlot, a tab open and a drawer transition) still finds the selection where an event would have fired unheard.SideChatkeeps only the focus nudge (caret after the quote) keyed onseedTick.Wiring
ChatPage(unchanged behaviour)FlyingQuotesidetab (openActivityToTab('side'))ChatPanein split view (SessionGridView)FlyingQuoteswitchSlot(paneSlot)if it is not the active slot (the panel — and its SideChat — is bound to the active slot), then thesidetab. Split mode is not left.ChatPaneon the Members pageFlyingQuote<SideChat slot={memberSlot}/>), with a "Details" header action back and close resetting the viewChatMessageList/MessageRenderContextgainonQuote/onAsk; the default assistant row passes them through (same shape asonFileOpen).ChatPanetherefore needs no renderer override.sideChatDraftsand subscribes to it; no event listener, no mount poll, no DOM probe.data-side-chat-sloton the composer wrapper remains as a test / capture-harness hook only.handleQuote/handleAskbodies are the hook, byte-for-byte in behaviour) plus threading the one-prop opener intoSessionGridView.pages.chat.sidePanel.menu_side("Side Chat") andpages.membersPage.details; the toolbar labels already existed. No catalog regeneration needed.SideChat.tsx:border-t border-accent/30 bg-accent/5), including the main chat's right-panel Side Chat, not only the Members drawer. The hazard it answers — an off-record composer next to a live one, same send arrow — exists on the main chat too (side panel open beside the main composer, or beside a split pane), and Side Chat is one component on every surface; scoping the tint to one host would make the same panel read differently depending on where it is docked.AnimatePresence mode="wait",initial={false}so the drawer's own mount animation is not doubled), the header title changes to "Side Chat", a Details header action is the way back, closing the drawer forgets the view, and switching members returns to details. Recording below.chat-core/composer/sideChatDrafts.ts, in-memory, per slot, subscribable): every host unmountsSideChatthrough a control beside its composer — another activity tab, the Members drawer's Details, closing it, switching members — and the draft used to die with each.sendclears it through the same path; a failed submit or failed queued edit restores into the slot the request was FOR (restoreDraftTo(vars.slot)), whichever slot the panel shows by then. In-memory on purpose: a Side Chat is never persisted, so its draft should not outlive the page either.integration/setup.tsresets the store per test (same rule as the Pierre staging reset).Relationship to
feat/members-chat-steer-only(parallel PR)Branched from
origin/main, not stacked. Overlap is limited to the<ChatPane …/>mount line inMembersPage.tsx(both add one prop) and theChatPane.tsximport block /<ChatInput>neighbourhood. None of that PR's steer/composer/busyModelogic is touched. Whichever lands second keeps both props on the mount line.ChatPane vs ChatPage — remaining interaction gaps (list only, not fixed here → P3 / P5 backlog)
Props
ChatPagepasses toAssistantMessagethat the SDK's default assistant row (whatChatPane, and so Members + split view, renders) does not:onFolderOpen,onArtifactOpen,onSessionOpen,sessions,activeSessiononOpenDiff,fileChipStyle,artifactPathsonRegenerate,onSwitchVariant,isRegenerating(variants/variantIdxARE passed, so the arrows render inert)onFork,onPlanFromHere,forkIndex,forkMessageIdonLoadEarlier,loadingOlder,earlierRemaining(the pane uses its own "earlier messages → open session" row instead)onSpeakonApplyPlan,planTaskIdpinned,onTogglePinshareEnabled,prevUserText,linkPreviewsmessageTs,slotKey,slotTitle,modeAlso outside
AssistantMessage: the pane'suserrow is the SDK default (noonEditResend/ edit-and-resend; the parallel PR is adding its ownuseroverride for steer chrome), and the pane has nomcp_oauth/planorchestrator dispatch beyond what the registry carries (#5893). These are the P3 "composer/row capability" seams still to thread; this PR threads exactly one (selection actions) and deliberately nothing else.Tests (written, not run locally per task rules — CI runs them)
src/test/selectionActions.test.tsx—quoteIntoDraftshaping/stacking,sideSeedTargets,seedSideChatwaits for the named slot's composer (old-slot composer present ≠ satisfied), mount-wait exhaustion still dispatches, hook offers Ask only with an opener, Quote appends + one flight, Ask opens for THIS slot + seeds by slot + leaves the draft alone.src/test/ChatPane.selectionActions.test.tsx— the pane handsAssistantMessageonQuotealways /onAskonly withopenSideChat; Quote lands in the pane composer with the flight; stacking; Ask callsopenSideChat(paneSlot)and the seed names the pane slot, draft untouched.src/pages/members/MembersPage.sideChat.test.tsx— the page hands the pane an opener; Ask swaps the drawer to the member-slot Side Chat titled "Side Chat"; Details returns; close forgets the view.src/test/SelectToAsk.test.tsx(+1) — SideChat takes a seed naming its slot, ignores one naming another, marks its composer withdata-side-chat-slot.src/test/ChatPageMoreCoverage.test.tsx— existing Quote/Ask regression pins unchanged; the Ask pin additionally asserts the seed names the active slot.Screenshot evidence
Real components in the capture entries, genuine triple-click selections, every frame state-asserted by the harness scripts (all frames OK):
Members page —
website/scripts/capture-members-selection-quote-ask.mjsSplit view (selection made in the NON-active pane,
pane-b) —website/scripts/capture-chatpane-selection-quote-ask.mjs, on the sharedprepareSplitChatPagefixturepane-b: the page re-bound topane-b(sidebar highlight moved to Release checklist), Side tab opened,pane-b's Side Chat seeded; split mode kept; both pane composers emptypane-blands inpane-b's own composer;pane-auntouchedRound 1 (head
75bf525a9, rebased ontoorigin/main@ #8951)activeSlot(never the rosterslot_key), andopenMemberresets the drawer view to details on member switch — a slug-collided member can no longer inherit a Side Chat bound to a rejected key. Two new tests pin both halves (MembersPage.sideChat.test.tsx).docs/feature-map/README.mdSide chat + Crew Members rows carry the new reach paths.max-two-buttons-per-rowon the selection toolbar — the diff adds no markup to that row;useSelectionActionsalready renders Quote / Ask / Copy for ChatPage on main (grandfathered). ChatPane merely stops withholding the two callbacks the shared component takes.border-accent/30 bg-accent/5) so the off-record composer reads differently from the live thread composer at a glance.switchSlotis visible as the sidebar highlight moving to the pane's session + the panel header (frame 2 above); split mode is kept. Restoring the prior slot on close was rejected — it would fight a later explicit selection and the grid's ownonCollapsealready uses the same switch.replaceChildren(), not.innerHTML.deadKeysratchet red was main-inherited (fixed on main by fix(i18n): drop the dead pages.chatPage.dismiss_upload_error key #8951; this head is rebased onto it).Round 2 (head
c0ebaf8d7, rebased ontoorigin/main@ #8927)SideSeedDetail.slotis required,sideSeedTargets/sideChatComposerForhave no "any Side Chat" arm, and the hook offers no Ask without a slot. Tests updated (selectionActions.test.tsx,SelectToAsk.test.tsx: a seed naming no slot is ignored).75bf525a9: a PR-body edit after the GPT lane had posted re-triggered the lane and its newest run was cancelled by concurrency, which readiness reads as a failure — re-ran that run (green). This head's body edit landed before the push so the push supersedes it.Round 3 (head
c7f3a6d8e)SideChatand discarded its uncontrolled draft. Fixed at the component: drafts are kept per slot insideChatDrafts.tsandSideChatruns its composer in controlled mode against it — which also closes the same pre-existing loss on the main chat's activity panel when switching tabs. Pinned by a newSelectToAsk.test.tsxcase (unmount → other slot sees nothing → same slot gets the draft back).Round 4 (head
166204b35, rebased ontoorigin/main@ #8958)mergeIntoDraft, i.e. into whatever slot the panel showed at rejection time — after a re-bind (split-view Ask, member switch) A's question landed in B's draft. NowrestoreDraftTo(vars.slot, vars.q): the visible draft when that slot is still the one shown, otherwise straight into that slot's store entry. Pinned by a newSelectToAsk.test.tsxcase (submit on A, re-bind the same instance to B, reject → A's store has the question, B's draft untouched).test_snapshot.py::TestNotificationCopyWhenNoLiveFileExiststiming failure in untouched Python (also seen in round 1); this PR changes no backend code. Re-run only that job if it recurs on this head.Round 5 (head
48436bb9e){A, ''}, re-bind to B, A fails and restores into the store only, re-bind to A → the stale cached empty string was shown and the next keystroke overwrote the restored question. Invariant now: the store is the single source of truth —SideChatreadsreadSideChatDraft(slot)on every render and keeps state only as a render tick, so nothing can shadow a restore. The failed-submit test now also drives the return to A and asserts the restored question is shown and appended to, not overwritten.vi.fnper property access, somockImplementationand the call assertion targeted different fns;sideTurn/sideOpenare now stable mocks (theSideChat.oversizeQuestionpattern).Round 6 (head
8a039f385)editQueued.onError(a failed queued-question edit) still merged the text into the draft of whatever slot the panel showed. NowrestoreDraftTo(vars.slot, vars.content). Rather than another point fix,restoreDraftTo's doc now carries the table of every draft writer and why each lands on the right slot (typing / chips / send-clear → the shown slot by definition; seed → gated bysideSeedTargets; queue release → read fromslotSide[slot], waits in the store for a hidden slot; failed submit and failed edit →restoreDraftTo(vars.slot)), with the rule that amergeIntoDraftinside a mutation callback is the wrong-slot bug again. (The Opus adjudicator flagged the finding as naming non-existent code; it was reading the ChatPane queue hook — themergeIntoDraftis inSideChat.tsx's owneditQueued.onError, so the finding was real and is fixed.) New test inSideChat.steerQueue.test.tsx: edit on A, re-bind to B and type, A's edit fails → A's store draft holds the wording, B's draft untouched.test_snapshot.py::TestNotificationCopyWhenNoLiveFileExists— the known flake tracked in flaky: TestNotificationCopyWhenNoLiveFileExists ordering asserts race on loaded CI shards #8893 (open); no backend code in this PR.Round 7 (head
8226e607e)8a039f385.side-seedevent + 20-frame rAF poll +data-side-chat-slotDOM probe — all three are deleted.sideChatDraftsmoved into chat-core and became subscribable (useSyncExternalStore); Ask writes the quote into the slot's store entry (seedSideChatDraft) and the panel renders it whenever it mounts, so a late-mounting panel cannot miss the selection. Draft state now lives in exactly one place.revealComposeris a host callback (selectionActions.tsno longer imports frompages/). Tests: store contract (selectionActions.test.tsx, incl. the seed-before-mount case),SelectToAsk.test.tsxseed cases rewritten against the store, host tests assert the store instead of listening for an event.main, this PR does not change them.ChatInputbehaviour on every split pane onmain; this PR does not touchChatInput.Round 8 (head
c2b4fd126, rebased ontoorigin/main@ #8998) — readiness passed on8226e607e; two advisories fixedseedTickwas never reset, so every later remount of a once-seeded slot (reopening the Side tab, a member switch and back) re-ran the focus nudge into an empty composer. The nudge now consumes the seed (consumeSideChatSeed), so a non-zero tick means "a seed is waiting for the caret", never "was seeded once". Store contract test added.useSideChatDraft(activeSlot)holds text; newMembersPage.sideChat.test.tsxcase (absent without a draft, present with one, gone when it clears).needs-a-decisionin round 7); labels pre-exist onmain, default is to ship as-is.Round 9 (head
f936f404a)switchSlotunguarded while every otherswitchSlotinChatPage(tab strip, sidebar row,?siddeep link, tab close) is gated onconnected— offline, the rejected switch clears the active pane's messages and the transcript the reader just selected from disappears until reconnect. Two-part fix: the grid is handedopenSideChatonly while connected, so panes' toolbars offer Copy / Quote only offline (capability by omission, no Ask into the void and no orphan seed); andopenSideChatForPaneitself checksconnectedRefbefore the switch, covering the frame between the drop and the re-render — it does nothing rather than open a Side Chat bound to some other slot. Two newChatPageMoreCoverage.test.tsxcases through a stubbedSessionGridView: connected Ask re-binds to the pane's slot and opens thesidetab; aftersseDisconnectedthe capability is gone and a slipped-through call moves nothing.MochiChatPanel.coverage.test.tsx › replaces the streamed text with the committed message— afindByText('partial')timeout (1034 ms) in the Mochi app's streaming footer; this PR touches nothing underapps/mochiand the file has a prior stabilisation for the same streaming assertion (test: stabilize flaky widget-tag streaming assertion (#3314) #3378). Not this PR's; Coverage Merge is its cascade.test_snapshotflake again — also red onmain's own runs34035151321/34033073642at the same time.Round 10 (head
af095d91e) — round 9's new connected-path test, fixedre-binds the activity panel to a split pane's slot…assertedactiveSlot === 'chat-2'but the harness knew onlychat-1, and ChatPage undoes a switch to a slot its list does not hold (the mode-guard clearsactiveSlot, the auto-select falls back to the first known slot) — the test was switching to a stranger.renderChatPagenow takes aslotsoption; the split tests render withchat-1+chat-2. Production code unchanged from round 9.f936f404a(GPT ✅ no blocking, Opus ✅, all advisory lanes settled); Coverage Merge / Coverage Gate were the cascade.Round 11 (head
38a1bf806, rebased ontoorigin/main@ #9037) — CONFLICTING clearedaf095d91ewas green on every frontend check and every AI lane; the two backend reds were inherited (shard 4 = flaky: TestNotificationCopyWhenNoLiveFileExists ordering asserts race on loaded CI shards #8893test_snapshot; shard 2 = threeTimeout >120sintest_dashboard_state_ws/test_history_locking_remediation— no backend code here). Main moved under the PR (28 commits), so it was rebased instead of rerun.docs/feature-map/README.mdCrew Members row: feat(members): show each member's auto-patrol (monitor loop) status #8936's auto-patrol wording and this PR's Side Chat wording merged as a union.MembersPage.tsxauto-merged with feat(members): show each member's auto-patrol (monitor loop) status #8936;tsc -b --force+ eslint clean. No code change.Round 12 (head
f3aeec2f0, rebased ontoorigin/main@ #8694) — picks up the #8893 fix38a1bf806everything was green except Backend shard 4 (test_snapshot … survives_the_READER, flaky: TestNotificationCopyWhenNoLiveFileExists ordering asserts race on loaded CI shards #8893) and its Coverage Gate cascade; shard 1's rerun passed. The shard-4 red was a stale merge ref, not a flake to rerun: fix(test): read the copy ordering before the worker is released #9047 fixed that test onmainat 16:03 UTC, but the run was built at 15:47 andgh run rerunreuses the same merge SHA, so it kept hitting the pre-fix assertion. Rebased so the merge ref contains fix(test): read the copy ordering before the worker is released #9047. No code change;tsc -b --forceclean.Local gates
tsc -b✓ ·eslinton every touched file ✓ ·check-i18n-strings.mjswithI18N_BASE_REF=origin/main✓ (0 added untranslated) ·check-theme-colors✓ ·check-phantom-classes✓ ·jscpd .✓ (0 clones). vitest/pytest deliberately not run locally.no linked issue: dispatched from a crew work item (session task), not a GitHub issue; #8964 is a follow-up filed by this PR, #5893 / #5895 / #5892 are referenced as precedent only.