Skip to content

feat(chat): Quote / Ask on selected text in ChatPane (Members + split) - #8947

Open
CrysisDeu wants to merge 1 commit into
mainfrom
feat/chatpane-selection-quote-ask
Open

feat(chat): Quote / Ask on selected text in ChatPane (Members + split)#8947
CrysisDeu wants to merge 1 commit into
mainfrom
feat/chatpane-selection-quote-ask

Conversation

@CrysisDeu

@CrysisDeu CrysisDeu commented Sep 6, 2026

Copy link
Copy Markdown
Collaborator

Summary

Selecting text in a Crew Members thread (and in a split-view pane) offered Copy only. AssistantMessage draws the selection actions its host hands it (useSelectionActions(onQuote, onAsk), AssistantMessage.tsx:212), and ChatPage was the only host passing onQuote/onAsk — Quote's FlyingQuote-into-composer and Ask's open-/side-and-seed were ChatPage-local.

This PR extracts both into one chat-core seam and wires ChatPane to 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.tsuseSelectionQuoteAsk({ 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 (openSideChat absent = no Ask; no slot = no Ask). chat-core imports nothing from pages/.
  • quoteDraft.tsquoteIntoDraft(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) via useSyncExternalStore). 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 a seedTick), 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 (after switchSlot, a tab open and a drawer transition) still finds the selection where an event would have fired unheard. SideChat keeps only the focus nudge (caret after the quote) keyed on seedTick.

Wiring

Host Quote lands in Ask opens
ChatPage (unchanged behaviour) main composer + FlyingQuote activity panel side tab (openActivityToTab('side'))
ChatPane in split view (SessionGridView) the pane's own composer + FlyingQuote ChatPage's opener: switchSlot(paneSlot) if it is not the active slot (the panel — and its SideChat — is bound to the active slot), then the side tab. Split mode is not left.
ChatPane on the Members page the thread composer + FlyingQuote the detail drawer, switched to a Side Chat view bound to the member's slot (<SideChat slot={memberSlot}/>), with a "Details" header action back and close resetting the view
  • app-sdk: ChatMessageList / MessageRenderContext gain onQuote / onAsk; the default assistant row passes them through (same shape as onFileOpen). ChatPane therefore needs no renderer override.
  • SideChat: runs its composer in controlled mode against sideChatDrafts and subscribes to it; no event listener, no mount poll, no DOM probe. data-side-chat-slot on the composer wrapper remains as a test / capture-harness hook only.
  • ChatPage.tsx: limited to consuming the hook (logic-equivalent; the deleted handleQuote/handleAsk bodies are the hook, byte-for-byte in behaviour) plus threading the one-prop opener into SessionGridView.
  • No new user-visible copy: the drawer reuses pages.chat.sidePanel.menu_side ("Side Chat") and pages.membersPage.details; the toolbar labels already existed. No catalog regeneration needed.
  • Deliberate global restyle — the Side Chat composer band is tinted on EVERY Side Chat (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.
  • Members drawer navigation: from the details view a Side Chat header action appears whenever the member's Side Chat holds an unsent draft, so checking details or closing the drawer mid-question is not a one-way door (re-selecting text would append a second quote). Ask swaps the drawer's content (details → Side Chat) rather than opening a second panel — the page has exactly one aside, and a member thread is a narrow three-column layout with no room for a fourth. The swap is a 120 ms crossfade keyed on the view (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.
  • Side Chat drafts survive the panel unmounting (chat-core/composer/sideChatDrafts.ts, in-memory, per slot, subscribable): every host unmounts SideChat through 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. send clears 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.ts resets 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 in MembersPage.tsx (both add one prop) and the ChatPane.tsx import block / <ChatInput> neighbourhood. None of that PR's steer/composer/busyMode logic 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 ChatPage passes to AssistantMessage that the SDK's default assistant row (what ChatPane, and so Members + split view, renders) does not:

Capability Props ChatPage passes, ChatPane lacks
Open chips: folder / artifact / session links onFolderOpen, onArtifactOpen, onSessionOpen, sessions, activeSession
File-change chips → diff viewer onOpenDiff, fileChipStyle, artifactPaths
Regenerate + variant navigation onRegenerate, onSwitchVariant, isRegenerating (variants/variantIdx ARE passed, so the arrows render inert)
Fork / plan-from-here onFork, onPlanFromHere, forkIndex, forkMessageId
Load earlier messages inline onLoadEarlier, loadingOlder, earlierRemaining (the pane uses its own "earlier messages → open session" row instead)
Speak (TTS) onSpeak
Apply plan onApplyPlan, planTaskId
Pin / unpin message pinned, onTogglePin
Share card + link previews shareEnabled, prevUserText, linkPreviews
Identity for footer actions messageTs, slotKey, slotTitle, mode

Also outside AssistantMessage: the pane's user row is the SDK default (no onEditResend / edit-and-resend; the parallel PR is adding its own user override for steer chrome), and the pane has no mcp_oauth/plan orchestrator 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.tsxquoteIntoDraft shaping/stacking, sideSeedTargets, seedSideChat waits 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 hands AssistantMessage onQuote always / onAsk only with openSideChat; Quote lands in the pane composer with the flight; stacking; Ask calls openSideChat(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 with data-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 pagewebsite/scripts/capture-members-selection-quote-ask.mjs

01 — toolbar over a member reply: Quote / Ask in Side Chat / Copy toolbar
02 — Ask: drawer became the member's Side Chat (tinted composer band), seeded with the selection; thread composer empty ask
03 — Quote: selection in the thread composer as a blockquote; drawer stays on details quote
04 — recording: the drawer's details → Side Chat crossfade on Ask, and Details back 04-drawer-details-to-side-chat.webm

Split view (selection made in the NON-active pane, pane-b) — website/scripts/capture-chatpane-selection-quote-ask.mjs, on the shared prepareSplitChatPage fixture

1 — toolbar in the non-active pane: Quote / Ask in Side Chat / Copy split toolbar
2 — Ask from pane-b: the page re-bound to pane-b (sidebar highlight moved to Release checklist), Side tab opened, pane-b's Side Chat seeded; split mode kept; both pane composers empty split ask
3 — Quote from pane-b lands in pane-b's own composer; pane-a untouched split quote

Round 1 (head 75bf525a9, rebased onto origin/main @ #8951)

  • GPT F1 (security, fixed): the Members drawer's Side Chat now mounts only on the endpoint-confirmed activeSlot (never the roster slot_key), and openMember resets 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).
  • GPT F3 (fixed): docs/feature-map/README.md Side chat + Crew Members rows carry the new reach paths.
  • GPT F2 (rebutted, disposition posted): max-two-buttons-per-row on the selection toolbar — the diff adds no markup to that row; useSelectionActions already renders Quote / Ask / Copy for ChatPage on main (grandfathered). ChatPane merely stops withholding the two callbacks the shared component takes.
  • UX Watch 1 (implemented): Side Chat composer band is now tinted (border-accent/30 bg-accent/5) so the off-record composer reads differently from the live thread composer at a glance.
  • UX Watch 2 (verified perceivable): split-view Ask's switchSlot is 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 own onCollapse already uses the same switch.
  • UX Evidence gap (closed): split-view frames above.
  • UX Suggestion ("MCPs" wording): pre-existing shared string on the main chat's Side Chat; deferred with a tracked issue.
  • Automated Rule Check: test cleanup uses replaceChildren(), not .innerHTML.
  • Frontend Tests (3) / Coverage cascade: deadKeys ratchet 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 onto origin/main @ #8927)

  • First Principles Watch 1 (declared): the composer tint is a deliberate global restyle — see the Wiring bullet above.
  • First Principles Watch 2 / Subtraction (fixed): the slot-less seed compatibility branch is gone — SideSeedDetail.slot is required, sideSeedTargets / sideChatComposerFor have 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).
  • UX Watch (fixed): the drawer's details ↔ Side Chat swap is a crossfade keyed on the view; rationale in the Wiring bullet above.
  • UX Evidence gap (closed): recording 04 above, produced by the Members harness.
  • UX Suggestion (fixed): both harnesses wait out the toolbar's mount animation before the frame; all six stills re-shot.
  • Readiness red on 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)

  • GPT F1 (data-loss, fixed): leaving the Side Chat view (Details / close / member switch) unmounted SideChat and discarded its uncontrolled draft. Fixed at the component: drafts are kept per slot in sideChatDrafts.ts and SideChat runs 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 new SelectToAsk.test.tsx case (unmount → other slot sees nothing → same slot gets the draft back).
  • Frontend Tests (3): the slug-collision test asserted the Side Chat's absence synchronously while the previous member's view was still crossfading out — assertion now waits for the exit.

Round 4 (head 166204b35, rebased onto origin/main @ #8958)

  • GPT F1 (data-loss, fixed): a failed side submit handed the text back through 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. Now restoreDraftTo(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 new SelectToAsk.test.tsx case (submit on A, re-bind the same instance to B, reject → A's store has the question, B's draft untouched).
  • Frontend Tests (2): the new draft-survival test seeded a hand-picked chat state that lacked the per-slot maps the composer's selectors read for a non-active slot; it now spreads the reducer's initial state.
  • Backend Tests (3.12, 4): test_snapshot.py::TestNotificationCopyWhenNoLiveFileExists timing 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)

  • GPT F1 (data-loss, fixed — same file as round 4, second hit on that span): the per-slot draft cache in state could shadow the store: submit on A clears the cached {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 truthSideChat reads readSideChatDraft(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.
  • Frontend Tests (2): the failed-submit test could not observe the submit — the file's api mock minted a fresh vi.fn per property access, so mockImplementation and the call assertion targeted different fns; sideTurn / sideOpen are now stable mocks (the SideChat.oversizeQuestion pattern).

Round 6 (head 8a039f385)

  • GPT F1 (data-loss, fixed): the third sibling of the same class — editQueued.onError (a failed queued-question edit) still merged the text into the draft of whatever slot the panel showed. Now restoreDraftTo(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 by sideSeedTargets; queue release → read from slotSide[slot], waits in the store for a hidden slot; failed submit and failed edit → restoreDraftTo(vars.slot)), with the rule that a mergeIntoDraft inside 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 — the mergeIntoDraft is in SideChat.tsx's own editQueued.onError, so the finding was real and is fixed.) New test in SideChat.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.
  • Backend Tests (3.12, 4): 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)

  • GPT: ✅ no blocking findings on 8a039f385.
  • Design CONCERNS (fixed, both suggestions taken): the seed no longer travels over the side-seed event + 20-frame rAF poll + data-side-chat-slot DOM probe — all three are deleted. sideChatDrafts moved 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. revealComposer is a host callback (selectionActions.ts no longer imports from pages/). Tests: store contract (selectionActions.test.tsx, incl. the seed-before-mount case), SelectToAsk.test.tsx seed cases rewritten against the store, host tests assert the store instead of listening for an event.
  • UX Watch (label pair is a first-use guess) → needs a maintainer call, asked in the disposition; the labels ("Quote" / "Ask in Side Chat") pre-exist on main, this PR does not change them.
  • UX Watch (split-pane placeholder wraps + clips) → accepted-and-deferred to ChatInput placeholder wraps and clips in narrow split-view panes #9016: pre-existing ChatInput behaviour on every split pane on main; this PR does not touch ChatInput.
  • UX Suggestion ("MCPs" wording) → already tracked in i18n: reword Side Chat footer 'Tools and MCPs are unavailable here' into plain language #8964.

Round 8 (head c2b4fd126, rebased onto origin/main @ #8998) — readiness passed on 8226e607e; two advisories fixed

  • Opus advisory (fixed): seedTick was 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.
  • UX Watch (one-way door into the Members Side Chat, fixed): the details view now offers a Side Chat header action while useSideChatDraft(activeSlot) holds text; new MembersPage.sideChat.test.tsx case (absent without a draft, present with one, gone when it clears).
  • UX Watch ("Ask in Side Chat" is a first-use guess): maintainer decision still open (posted as needs-a-decision in round 7); labels pre-exist on main, default is to ship as-is.

Round 9 (head f936f404a)

  • GPT F1 (data-loss, fixed): split-view Ask on a non-active pane dispatched switchSlot unguarded while every other switchSlot in ChatPage (tab strip, sidebar row, ?sid deep link, tab close) is gated on connected — 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 handed openSideChat only while connected, so panes' toolbars offer Copy / Quote only offline (capability by omission, no Ask into the void and no orphan seed); and openSideChatForPane itself checks connectedRef before 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 new ChatPageMoreCoverage.test.tsx cases through a stubbed SessionGridView: connected Ask re-binds to the pane's slot and opens the side tab; after sseDisconnected the capability is gone and a slipped-through call moves nothing.
  • Frontend Tests (4): MochiChatPanel.coverage.test.tsx › replaces the streamed text with the committed message — a findByText('partial') timeout (1034 ms) in the Mochi app's streaming footer; this PR touches nothing under apps/mochi and 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.
  • Backend Tests (3.12, 4) + Coverage Gate: the flaky: TestNotificationCopyWhenNoLiveFileExists ordering asserts race on loaded CI shards #8893 test_snapshot flake again — also red on main's own runs 34035151321 / 34033073642 at the same time.

Round 10 (head af095d91e) — round 9's new connected-path test, fixed

  • Frontend Tests (3), own test: re-binds the activity panel to a split pane's slot… asserted activeSlot === 'chat-2' but the harness knew only chat-1, and ChatPage undoes a switch to a slot its list does not hold (the mode-guard clears activeSlot, the auto-select falls back to the first known slot) — the test was switching to a stranger. renderChatPage now takes a slots option; the split tests render with chat-1 + chat-2. Production code unchanged from round 9.
  • Every other check green on f936f404a (GPT ✅ no blocking, Opus ✅, all advisory lanes settled); Coverage Merge / Coverage Gate were the cascade.

Round 11 (head 38a1bf806, rebased onto origin/main @ #9037) — CONFLICTING cleared

Round 12 (head f3aeec2f0, rebased onto origin/main @ #8694) — picks up the #8893 fix

Local gates

tsc -b ✓ · eslint on every touched file ✓ · check-i18n-strings.mjs with I18N_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.

@CrysisDeu
CrysisDeu requested a review from a team September 6, 2026 08:22
@CrysisDeu
CrysisDeu requested a review from a team as a code owner September 6, 2026 08:22
@github-actions github-actions Bot added the readiness: checking Automated validation is still running label Sep 6, 2026
@github-actions

github-actions Bot commented Sep 6, 2026

Copy link
Copy Markdown
Contributor

Design Review (Fable 5) — ✅ PASS

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

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

  • Members drawer: the "Side Chat" return action is gated on an unsent draft only (memberSideDraft.trim()), but send clears the draft — a user who taps Details while the answer streams loses the way back and must re-select text, appending a spurious quote. Gate it on draft or a non-empty slotSide[activeSlot] conversation.

[DESIGN-REVIEWED] f3aeec2

@github-actions

github-actions Bot commented Sep 6, 2026

Copy link
Copy Markdown
Contributor

First Principles Review (Fable 5) — ✅ PASS

Premise-level review of f3aeec2f05271dbc49da0ddcd336ab907b728a66 — 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 is done. The description's claims held up against the repo: the seed race fix is cause-level (the rAF poll and side-seed event are fully deleted, only a stale test comment remains), the new store is not a second spelling of createSlotDraftStore (that factory is Web-Storage persistence with no subscription or in-memory mode), every new surface has ≥1 real consumer, and the capture scripts/screenshots follow an existing convention of 424 scripts and 1,100+ committed frames.

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 ships

Intent: let a reader quote or ask about selected assistant text on every transcript surface, not only the main chat — an ADDITION.

  1. Members thread selection now offers Quote / Ask (was Copy only) — justified
  2. Split-view pane selection offers the same — justified
  3. Members drawer swaps to a Side Chat view on Ask, Details as the way back — justified (page has no activity panel)
  4. Details view offers "Side Chat" while an unsent draft exists — justified (return without double-quoting)
  5. Side Chat drafts survive the panel unmounting, every surface — justified, declared behavior change
  6. Ask's seed handoff: rAF-poll + CustomEvent replaced by a per-slot store write — cause-level; deletes ChatPage's poll
  7. Ask from a non-active split pane re-binds the active slot first; withheld offline — justified, derived
  8. Every Side Chat composer band is tinted — declared global restyle, named hazard (mis-send steers a live run)
  9. app-sdk rows gain onQuote/onAsk passthrough — 1 consumer (ChatPane), same shape as onFileOpen
  10. Capture scripts + screenshots + feature-map rows — convention (424 existing capture scripts, 1,100+ frames counted)

Checks that came back clean: sideChatDrafts is not a duplicate of createSlotDraftStore (utils/slotDraftStore.ts is Web-Storage-only, no subscribe — a reactive in-memory store is meaningfully different); the quote shaping's 3 prior copies are collapsed and no composer-facing sibling remains (IssuePanel/PullRequestPanel blockquotes serve a different job); side-seed has 0 production references left.

[FIRST-PRINCIPLES-REVIEWED] f3aeec2

@github-actions github-actions Bot added readiness: action required A blocking check or review needs attention and removed readiness: checking Automated validation is still running labels Sep 6, 2026
@github-actions

github-actions Bot commented Sep 6, 2026

Copy link
Copy Markdown
Contributor

Opus 4.8 Review — ✅ no blocking findings

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

Review details

I'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 revealComposerqueryComposer falls to step 2 ([data-chat-pane="focused"]), which the candidate assumes is a different pane than the one selected in.

That assumption is false. ChatPane.tsx:638 wraps every pane in onMouseDownCapture={onFocus}, so the triple-click that makes the selection in pane-b fires onFocus and marks pane-b as the grid-focused pane (data-chat-pane="focused" at ChatPane.tsx:654). By the time the portal toolbar's "Quote" button is clicked, pane-b is already the focused pane, so queryComposer's step-2 fallback (composerFocus.ts:46) resolves to pane-b's own composer — the originating pane. The wrong-pane focus outcome does not occur; (c) fails.

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 f3aeec2f05271dbc49da0ddcd336ab907b728a66.

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

@github-actions

github-actions Bot commented Sep 6, 2026

Copy link
Copy Markdown
Contributor

UX Review (Fable 5) — 🟡 CONCERNS

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

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

  • Both primary toolbar actions were only guesses cold: "Quote: a guess; Ask in Side Chat: a guess (I don't know what a 'Side Chat' is yet)" (shot-01). Guesses matched the shipped behavior and the panel self-explains on open, so impact is one-time mild uncertainty; no change required, but a human should confirm the label carries enough on first sight.
  • Drawer's "Side Chat" header action (MembersPage.tsx headerActions, draft-gated) appears only while an unsent draft exists → a user who closed the drawer mid-question must notice a button that wasn't there before, with no cue it holds their text; low frequency, recoverable, but worth a human look at discoverability.

Evidence gaps

  • The details-view "Side Chat" return button (data-testid="member-drawer-side-chat") is in no committed screenshot; a still of the details drawer with a pending Side Chat draft (or a frame from 04-drawer-details-to-side-chat.webm showing the Details-back state) would close it.

[UX-REVIEWED] f3aeec2

@github-actions

github-actions Bot commented Sep 6, 2026

Copy link
Copy Markdown
Contributor

GPT 5.6 Review — ✅ no blocking findings (all downgraded on adjudication)

GPT 5.6 flagged blocking issues on f3aeec2f05271dbc49da0ddcd336ab907b728a66; Opus 4.8 adjudication downgraded every one of them to advisory. Adjudication downgraded all 1 blocking finding(s) to advisory: the remedy each one requires is disproportionate to the harm it prevents. Read them as advice, not as merge conditions.

This comment is updated in place on each push.

Review details

BLOCKING -- website/src/pages/ChatPage.tsx:6600 -- Failed pane switch silently blanks the transcript
dispatch(switchSlot(slot))
Ask in a non-active pane + rejected slot fetch -> rejection clears cached messages -> blank transcript with no failure notice.
Anchor: errors-use-error-notice
Fix: Unwrap the switch, restore the prior slot on rejection, show an ErrorNotice, and open Side Chat only after success.
[BLOCK-MERGE-DOWNGRADED] f3aeec2
[GPT-REVIEWED] f3aeec2

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

F1 — openSideChatForPane (ChatPage.tsx:6597-6603) dispatches switchSlot(slot) fire-and-forget after guarding the offline case (if (!connectedRef.current) return, line 6599). A 404 rejection does not blank anything — the reducer restores the prior slot's activeSlot, cached page, and cursor (chatSlice.switchSlotRejection.test.ts:124-142). Only a transient (non-404) failure clears the pane, and that path is deliberately designed to "keep the target selected with a cleared pane, so a retry can succeed" (test:166-173) — recoverable by re-select / WS slots update / reconnect.

The anchored blocking rule errors-use-error-notice (AUTOSDE.yaml:526, blocking: true) governs how surfaced errors are rendered and its blocking triggers are a hand-written error surface or a touched ErrorNotice missing the askAgent decision (AUTOSDE.yaml:571-575). This hunk introduces neither — it is a silent path, not a hand-rolled surface — so the rule's flag does not authoritatively bind this finding; it is an ordinary error-handling proportionality question I may weigh.

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, ?sid deep link — comment ChatPage.tsx:6589-6596); the proposed fix (unwrap, restore prior slot, ErrorNotice, sequence the tab open) forks a bespoke mechanism for this one caller, diverging from the shared, deliberately-designed, extensively-tested seam and carrying permanent maintenance/inconsistency load that clearly exceeds the LOW recoverable harm.

[ADJUDICATION] f3aeec2f05271dbc49da0ddcd336ab907b728a66 total=1 uphold=0 downgrade=1
DOWNGRADE F1 website/src/pages/ChatPage.tsx:6600 reason=disproportionate-remedy
[GPT-ADJUDICATED] f3aeec2f05271dbc49da0ddcd336ab907b728a66

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

@CrysisDeu
CrysisDeu force-pushed the feat/chatpane-selection-quote-ask branch from 148ebb4 to 75bf525 Compare September 6, 2026 08:53
@github-actions github-actions Bot added readiness: checking Automated validation is still running and removed readiness: action required A blocking check or review needs attention labels Sep 6, 2026
@CrysisDeu

Copy link
Copy Markdown
Collaborator Author
  • F1 — Side Chat mounts an unverified roster slot (website/src/pages/members/MembersPage.tsx) — span=e7ac5689c151 — fixed in 75bf525

The drawer's Side Chat now mounts only on activeSlot, the key filled by POST /api/members/{slug}/thread, never on activeMemberKey's roster fallback; and openMember resets drawerView to 'details' on every member switch, so a side view left open on one member cannot re-mount on the next member's rejected key. Both halves are pinned by new tests in MembersPage.sideChat.test.tsx ("switching members returns the drawer to details", "never mounts a Side Chat on a thread key the opener rejected (slug collision)").

@CrysisDeu

Copy link
Copy Markdown
Collaborator Author
  • F3 — New Side Chat entry point absent from the feature map (website/src/pages/members/MembersPage.tsx) — span=e7ac5689c151 — fixed in 75bf525

docs/feature-map/README.md: the Side chat row now lists the three reach paths (right panel Side tab, /side, and Ask in Side Chat on selected text in main chat / split panes / Members threads) plus MembersPage.tsx as a page; the Crew Members row names the drawer's Side Chat surface, the selection-toolbar reach path, handlers/side.py and the side/* endpoints.

@CrysisDeu

Copy link
Copy Markdown
Collaborator Author
  • F2 — Selection toolbar grows to three peer buttons (website/src/components/ChatPane.tsx) — span=42cfccb1d7fb — rebutted (grandfathered row; the diff adds no markup to it)

max-two-buttons-per-row scopes to what the diff ADDS. The row in question is SelectionToolbar's action strip, rendered by useSelectionActions(onQuote, onAsk) — unchanged by this PR. On main, ChatPage.tsx already passes both callbacks, so the identical Quote / Ask in Side Chat / Copy row ships today in the main chat. This PR only stops ChatPane from withholding the two callbacks the shared component already takes (ChatMessageListctx.onQuote/onAskAssistantMessage), so the same component renders the same row in a second host. Moving Copy into an overflow menu would have to change the main chat's shipped toolbar — out of this PR's scope and the UX lane's call; UX Review returned CONCERNS on other points and did not flag the row. Same precedent as #5895 (FollowUpBar mounted in ChatPane) and #5892 (QueueStack onEdit in ChatPane), both overridden on the grandfather clause.

@CrysisDeu

Copy link
Copy Markdown
Collaborator Author
  • UX Watch — wrong-composer risk: two identical composers on one screen (Members thread + Side Chat)fixed in 75bf525

Took the smallest fix named: the Side Chat composer band in SideChat.tsx is now tinted (border-t border-accent/30 bg-accent/5) so the off-record composer reads differently from the live thread composer at a glance, not only from the panel header. Applies to the main chat's Side Chat too, so the two surfaces stay one family. Re-shot frame 02 (Members) shows the pair side by side with the tinted band.

@CrysisDeu

Copy link
Copy Markdown
Collaborator Author
  • UX Watch — split-view Ask silently switches the active slot (openSideChatForPaneswitchSlot)rebutted on the "verify the switch is perceivable" branch, with evidence

The switch is perceivable: in split mode the active slot's only visible manifestations are the sessions-sidebar highlight and the activity panel's binding, and both move to the pane's session on Ask — see the new split-view frame 2 (temp-screenshots/chatpane-selection-quote-ask/2-split-ask-side-chat-seeded.png): the sidebar highlight sits on Release checklist (pane-b), the Side tab is open and seeded for pane-b, and the grid stays in split. Nothing else re-renders (the main transcript is not mounted in split mode). Restoring the prior slot on side-chat close was considered and rejected: the grid's own onCollapse already switches the active slot the same way, and an automatic switch-back would fight whatever the user selected in between. "Without interrupting the active run" stays true — no turn is interrupted; only which session the panel is about changes, which is exactly what Ask-from-this-pane asks for.

@CrysisDeu

Copy link
Copy Markdown
Collaborator Author
  • UX Evidence gap — the split-view pane's Quote / Ask appears in no committed screenshotfixed in 75bf525

Added website/scripts/capture-chatpane-selection-quote-ask.mjs on the shared prepareSplitChatPage fixture, selecting in the NON-active pane (pane-b), with three state-asserted frames under temp-screenshots/chatpane-selection-quote-ask/: the toolbar in the non-active pane; Ask re-binding the page to pane-b, opening the Side tab and seeding pane-b's Side Chat while split mode is kept and both pane composers stay empty; Quote landing in pane-b's own composer with pane-a untouched. Embedded in the PR body.

@CrysisDeu

Copy link
Copy Markdown
Collaborator Author

Agreed on the wording. The string (pages.chat.sideChat.context_only_tools_unavailable) is pre-existing and shared with the main chat's Side Chat panel — this PR only exposes it on a second surface — so the reword is a 13-catalog copy change (en + 12 locales via i18n-translate.mjs, en-XA regenerated) that belongs in its own PR rather than inside the selection-actions wiring. Tracked in #8964 (deferred-finding, assigned, Due 2026-09-20) with the proposed replacement "Tools and integrations are unavailable here."

@CrysisDeu

Copy link
Copy Markdown
Collaborator Author

/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).

@github-actions

github-actions Bot commented Sep 6, 2026

Copy link
Copy Markdown
Contributor

Human judgment recorded

@CrysisDeu marked the gpt AI finding as false positive, not applicable, or explicitly accepted for 75bf525a99dd210cff29801c785e76469da98864.

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).

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

@github-actions github-actions Bot added readiness: action required A blocking check or review needs attention readiness: checking Automated validation is still running and removed readiness: checking Automated validation is still running readiness: action required A blocking check or review needs attention labels Sep 6, 2026
@CrysisDeu
CrysisDeu force-pushed the feat/chatpane-selection-quote-ask branch from 75bf525 to c0ebaf8 Compare September 6, 2026 09:20
@github-actions github-actions Bot added readiness: checking Automated validation is still running readiness: action required A blocking check or review needs attention and removed readiness: action required A blocking check or review needs attention readiness: checking Automated validation is still running labels Sep 6, 2026
@CrysisDeu
CrysisDeu force-pushed the feat/chatpane-selection-quote-ask branch from f936f40 to af095d9 Compare September 6, 2026 15:02
@github-actions github-actions Bot added readiness: checking Automated validation is still running merge conflict Branch has merge conflicts with its base — author must resolve before merge readiness: action required A blocking check or review needs attention and removed readiness: action required A blocking check or review needs attention readiness: checking Automated validation is still running labels Sep 6, 2026
@CrysisDeu
CrysisDeu force-pushed the feat/chatpane-selection-quote-ask branch from af095d9 to 38a1bf8 Compare September 6, 2026 15:47
@github-actions github-actions Bot added readiness: checking Automated validation is still running readiness: action required A blocking check or review needs attention and removed readiness: action required A blocking check or review needs attention merge conflict Branch has merge conflicts with its base — author must resolve before merge readiness: checking Automated validation is still running labels Sep 6, 2026
… 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/.
@CrysisDeu
CrysisDeu force-pushed the feat/chatpane-selection-quote-ask branch from 38a1bf8 to f3aeec2 Compare September 6, 2026 18:00
@github-actions github-actions Bot added readiness: checking Automated validation is still running readiness: passed Eligible automated validation passed for the current revision merge conflict Branch has merge conflicts with its base — author must resolve before merge and removed readiness: action required A blocking check or review needs attention readiness: checking Automated validation is still running labels Sep 6, 2026
@chenmingwei23

Copy link
Copy Markdown
Contributor

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. closingIssuesReferences on this PR is empty, so merging it as it stands leaves #8930 open. A Closes #8930 line on its own at the bottom of the body fixes that; nothing else reports it after the merge.

Conflicting against main. mergeStateStatus is DIRTY against main at 7dd090fb6, with no push since 2026-09-06T20:08Z, so CI is not saying anything useful about the current diff until it is rebased. #8852 also still rewrites ChatPane.tsx (+286/-29) and is DIRTY too, so whichever of the two rebases second absorbs the other's changes to that file.

One note in your favour on the issue itself: I checked whether #8930 is really a regression and it is not. git log -S'onQuote' -- website/src/components/ChatPane.tsx is empty across the whole history, while the same probe on ChatPage.tsx returns the initial dashboard commit 737ab0c45. onQuote/onAsk were never wired into ChatPane, so nothing removed them and this is a never-wired surface rather than a break in 0.6.0rc1. Recorded on the issue as well, since the channel: insider release-blocker reading rests on it.

Coverage evidence I read: ChatPane.tsx +25/-1 wiring both props, plus website/src/test/ChatPane.selectionActions.test.tsx and website/src/pages/members/MembersPage.sideChat.test.tsx -- which is exactly the regression test the earlier triage note on #8930 asked for.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

merge conflict Branch has merge conflicts with its base — author must resolve before merge readiness: passed Eligible automated validation passed for the current revision

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants