feat(chat-core): P5-e — one virtualized transcript for every chat surface - #10146
Conversation
Design Review (Fable 5) — ✅ PASSDesign-level review of Design-Verdict: PASS A disclosed, RFC-staged extraction: real freeze fixed at the render layer, SDK surface stays backward-compatible, every deferred trade-off is a scheduled P5-f row. [DESIGN-REVIEWED] 5de855d |
UX Review (Fable 5) — 🟡 CONCERNSUX-level review of All evidence reviewed: the blind read ran, the three screenshots match the diff's three hosts, and no recordings were supplied (none are required — the diff adds no in-place transform of a persistent element; row prepending and empty/error/list states are async lifecycle). The one primary new control, the embed's "Load earlier messages" bar, was read correctly and confidently by the blind reader. What keeps this from PASS is the PR's own admitted evidence gaps and the silent 500-row ceiling. UX-Verdict: CONCERNS Virtualization is invisible when it works — but three new states ship unscreenshotted, and the history bar dead-ends silently at the 500-row ceiling. Watch
Evidence gaps
[UX-REVIEWED] 5de855d |
First Principles Review (Fable 5) — ✅ PASSPremise-level review of All verification complete. Every premise the change rests on checks out in the repo: the backend slot handler defaults First-Principles-Verdict: PASS The author ran only static gates locally ("Tests are written for CI") — confirm the CI test lanes are green before merge. What this change shipsInventory (10 items) — 9 justifiedIntent: Stop long chat threads from freezing the tab in the DM pane, side panel, and app embed by rendering only the visible window and bounding the embed's per-second history fetch — a FIX (#10005, PR 2 of 2), delivered as the RFC's planned P5-e extraction.
Deleted-pin audit: [FIRST-PRINCIPLES-REVIEWED] 5de855d |
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/app-sdk/ChatEmbed.tsx:145 -- Background poll failures are silently ignored FINDING -- website/src/app-sdk/ChatEmbed.tsx:209 -- Initial history triggers Adjudication (Opus 4.8) — is blocking on each finding proportionate?I've confirmed the mechanism, the rule, and the recovery path. Let me record my ruling. F1 — The behavior is real: after a successful load, a failed background refetch leaves The anchor
False positive or not applicable? A repository writer can comment: |
Opus 4.8 Review — ✅ no blocking findingsReviewed Verdict parsed from the review's SHA-scoped output markers for commit False positive or not applicable? A repository writer can comment: |
1905fc4 to
9ef15fd
Compare
|
Round 2 (
|
9ef15fd to
ccc29f9
Compare
|
Round 3 (
|
ccc29f9 to
38a14ce
Compare
|
Round 4 (
Static gates: |
|
Round 5 ( Static gates: |
38a14ce to
d19dec5
Compare
…face (#10005) Extract ChatPage's private virtualized transcript into a reusable unit under website/src/chat-core/transcript/ and mount it from ChatMessageList, so the split/DM pane, the side panel and the app-SDK embed (and the spec-builder column through it) render only their viewport window and gain the earlier-history bar. - chat-core/transcript/VirtualTranscript: useVirtualChat over grouped DisplayItems + the shared TranscriptScrollShell + EarlierMessagesBar, with an imperative handle (scrollToBottom / scrollToIndex / mountIndex / getFollow / scroller) and slots for host content above and below the rows. - chat-core/transcript/rowKeys: the page's row-identity builders (virtualKeyFor, uniqueRowKeys, stableAnchorIdFor, anchorAltIdFor, ...) moved here; ChatPageMessageContent re-exports them so the page, the store and the tests keep their import path. useStableMessageKey hoists the page's clientTs → ts → minted-id rule. - ChatMessageList: new `transcript` prop (TranscriptMount) makes the list own its scroller through VirtualTranscript; without it the list stays the bare fragment the SDK shipped. forwardRef exposes the handle. - ChatPane, SideChat, ChatEmbed: drop their own overflow-y-auto div and useChatScrollFollow; empty states, hydrate errors, the earlier row and the footer move into the aboveRows / belowRows slots. ChatPane's pinned prompt now waits for mounted rows at the hand-off line, as the page does. - ChatEmbed: the poll is bounded (?limit=200, the handler's page) and the bar widens it by a page per press up to the handler's 500 ceiling; keepPreviousData keeps the rows on screen while a wider page loads. A new slot starts back at one page. - TranscriptScrollShell: `headerSpacer` opt-out for hosts without the page's overlay header (default unchanged; recipe tokens intact). - RFC: §4 P5-e note, §4.2 P5-e / P5-f rows, §2.1 embed transport note. ChatPage keeps its inline wiring (P5-f switches it). Incremental (since-cursor) polling for the embed and a before-cursor pager for the pane are recorded follow-ups.
|
Round 6 ( Static gates: |
d19dec5 to
5de855d
Compare
bolichen97
left a comment
There was a problem hiding this comment.
Maintainer review (agent-assisted, owner delegated), pinned to 5de855d. Combined review passes cover the full diff and changed tests. VirtualTranscript preserves the ChatPage virtualizer recipe, row visibility predicate, tail-role streaming index and running follow semantics. Content moves into aboveRows/belowRows unchanged; per-host scopes and stable side-message keys preserve identity, and the pane uses requiresMountedHandoff. No remaining imports of the removed scroll hook. Accepted accessibility follow-up: multiple embedded host scrollers now expose polite live regions in split views. Source review in isolated worktrees and current-head CI success; no local suites run this wave.
Summary
chat-core P5-e: ChatPage's private virtualized transcript becomes a reusable unit,
website/src/chat-core/transcript/, andChatMessageListmounts it behind a newtranscriptprop — so the Crew Members DM / split panes (ChatPane), the side panel (SideChat), the app-SDK embed (ChatEmbed) and the spec-builder column (throughChatEmbed) render only their viewport window and gain the earlier-history bar.ChatEmbed's poll is bounded to one page (?limit=200) and the bar widens it up to the handler's 500-row ceiling. Second PR of #10005; independent of the stopgap in #10026 (both branch frommain).Closes nothing on its own; #10005 closes when both PRs land.
Why now
#10005: a running Crew Members DM over a ~3000-row / 10 MB thread froze the tab. #10026 stops the fetch from being unbounded; this PR stops the render from being unbounded on every non-page host — a thread's DOM cost is now its viewport (overscan + window), not its history — and closes the same fetch hole in the embed, whose 1 s poll re-read the whole slot while a turn ran.
What changed
chat-core/transcript/VirtualTranscript.tsx(new):useVirtualChatover groupedDisplayItems + the sharedTranscriptScrollShell+EarlierMessagesBar, with an imperative handle (scrollToBottom/scrollToIndex/mountIndex/getFollow/scroller),aboveRows/belowRowsslots, an optional externalscrollerRef(forusePinnedPrompt),onAtBottomChange, andisRowHidden. Same tuning as the page (estimated 100 px, overscan 6, eager first measure, width-bucketed height scope). Height/anchor caches are partitioned per host (pane:/side:/embed:prefixes) so a split pane never restores the main page's anchor for the same slot.chat-core/transcript/rowKeys.ts:msgIdentityKey,turnLeadKey,virtualKeyFor,uniqueRowKeys,stableAnchorIdFor,anchorAltIdForMOVED here byte-for-byte;ChatPageMessageContentre-exports them, so ChatPage, the store and the tests keep their import path.useStableMessageKeyhoists the page's clientTs → ts → minted-id rule.app-sdk/ChatMessageList.tsx: newtranscript?: TranscriptMountprop. With it the list OWNS its scroller throughVirtualTranscript; without it, the bare fragment the SDK shipped (unchanged, pinned by the existing tests).forwardRefexposes the handle.streamingIndexis derived from the last row's tail ROLE (streaming), not the run flag.ChatPane,SideChat,ChatEmbeddrop theiroverflow-y-autodiv anduseChatScrollFollow; empty states, the hydrate error + retry, the "earlier messages → open session" row,ChatFooterand the thinking dots move into the slots.ChatPanepassesrunning || paneSlot.running(a remotely driven turn still follows, like its footer already assumed) and its pinned prompt waits for mounted rows at the hand-off line (requiresMountedHandoff), as the page does. The jump pill / edge fades are unchanged.ChatEmbed:GET /api/chat/slots/{slot}?limit=200(the handler's own page size);has_moredrives the bar; each press widens by 200 to 500; the widening is remembered per slot so a new slot's FIRST read is one page; a same-slot widen keeps the rows on screen as placeholder (never across slots). A REJECTED widen keeps the last settled page on screen (the transcript never blanks on a failed history read), the bar shows the failure, and Retry re-reads the limit that failed; the bar'sloading/failedare the widen's, not the ambient poll's. The top-anchored mode's "smooth-scroll on new reply" is keyed on the tail row's identity, so a prepended page does not yank the reader.startAtBottom→followOutput+initialPlacement: 'bottom'; otherwise top.TranscriptScrollShell:headerSpaceropt-out (defaulttrue; the page passes nothing; the recipe tokens are intact andChatPage.scrollShell.recipe.test.tsxis untouched).docs/request-for-change/rfc-chat-core-extraction.md: §4 P5-e note, §4.2 P5-e (this PR) + P5-f (ChatPage switch,before-cursor pager for the pane, MeasureFarm for hosts, incremental embed poll) rows, §2.1 embed note.website/scripts/capture-chat-core-p5e.mjs(+openMembersDmharness helper) — real-pod screenshots that also assert the row counts; the images are PR attachments (gh pr edit --attach, per chore: upload PR evidence with gh --attach instead of committing it #10010), not committed files.app-sdk/useChatScrollFollow.tsand its test — this PR removed its last consumers;FollowControllerstays (the virtualizer uses it).Screenshots (real pod,
kirocrew pod up wt-p5e-pr2 --seed minimal; attached, not committed)Load earlier messagesat the topCapture output:
dm: mounted 14 (indices 36–49 of 50), spacers [3456, 0], scrollHeight 4826 / clientHeight 763;embed: 17 rows mounted of the 200-row page; after one press the top spacer is 7808 px (400 rows loaded).What was tested
Static gates only, locally:
npx tsc -bandnpx eslinton every changed file — both clean. Tests are written for CI:test/VirtualTranscript.test.tsx— short list mounts every row in order; a 40-row list mounts a bounded, contiguous window with two spacers;initialPlacement: 'top'; the shell contract (.chat-container, overflow axes, host padding, noh-16band); the earlier bar only whilehasMoreand its press reaches the host; above/below slot order;isRowHidden→visibility: hidden; the handle; the externalscrollerRef+onAtBottomChange.test/ChatMessageList.transcriptMount.test.tsx— fragment mode unchanged;transcriptmode owns the scroller, indexes rows, hides the pinned row by identity, still reportsonDisplayItems.test/ChatEmbed.boundedPoll.test.tsx—?limit=200and never the bare slot URL; bar only whilehas_more; 200 → 400 → 500 per press, then the bar stops at the ceiling; a new slot starts at 200 on its first read; a rejected widen keeps the settled page and Retry re-reads that limit;startAtBottomvs top-anchored mount wiring.test/chatCoreTranscript.contract.test.ts— source guards: every host mountstranscript={{, none importsuseChatScrollFollowor keeps a transcript scroll div; per-host cache prefixes; the embed's bounded poll;VirtualTranscriptcomposes the page virtualizer + shell +rowKeys; the page's re-export;ChatMessageList's two modes.ChatEmbed.test.tsxURL assertion gains?limit=200.ChatPane.scrollChrome.test.tsx's jump-pill case now flushes the virtualizer's frame-deferred pin and dispatches the resulting scroll event (the virtualizer applies its pin on the next animation frame so the tail rows it just mounted have real heights, and reads the position back through the scroll event — as the page's pill already works); the assertions themselves are unchanged. No other test was edited;ChatPage.scrollShell.recipe.test.tsxandvirtualizerHeightOwner.test.tsare untouched.9ef15fd6b6): GPT F1 — a failed FIRST read rendered the empty-session copy; the embed now showsErrorNotice+ Retry (chat-embed-load-error), retry re-reads the page. GPT F2 — the history bar's ask-the-agent hand-off navigates to the main chat and would discard the embed's unsaved draft;EarlierMessagesBargainshandOff(default on, the page's contract) and the embed turns it off.ChatPane.pinnedPrompt.test.tsx's layout gains a physically contiguous next-prompt row: withrequiresMountedHandoffa gap under the fold is unmounted spacer and the hook correctly drops the banner, so the synthetic 260px gap the old layout used was itself the failure. My two newboundedPollcases awaitedfindByinsideact(a deadlock) and read data before React Query had settled it — fixed.1905fc4576): GPT F1 (a rejected widen blanked the transcript) fixed as above;VirtualTranscriptHandleshrinks toscrollToBottom(its only caller today; the page's navigation methods join in P5-f);useChatScrollFollowdeleted with its last consumers (and the source guard on it inFollowController.test.ts); the three PNGs left the branch for--attach; three test defects of mine fixed (theinitialPlacement: 'top'case now uses the top-anchored embed's own wiring,followOutput: false; the ChatEmbed mock ofChatMessageListrenders thetranscript.aboveRowsslot the empty state moved into; the new-slot case renders insideact).paneSlot.running, the over-broadstreamingIndex, andfailedtracking the ambient poll were all found there and fixed before the first push.UX CONCERNS
space-y-2wrapper; the virtualized rows have no inter-row gap of their own (same as ChatPane and the page). Visible in screenshot 2's empty state only; a populated side thread reads slightly tighter than before.limit + 200rows (the same stopgap shape as perf(chat): keep the pane hydrate and warm bounded while a slot runs #10026); abefore-cursor pager is P5-f. At the 500 ceiling the bar stops even if the server has more — the handler clamps wider asks, and the embed then shows no cue that older rows exist (UX lane's concern; a static "can't be shown here" line needs a new string across 12 locales, so it rides with the P5-f pager rather than landing as an untranslated placeholder here).ChatEmbed.boundedPoll.test.tsxrather than screenshotted.MeasureFarmis not mounted here, so the scrollbar of a pane/embed uses estimated heights for rows it has not seen yet (settles as the reader scrolls). Recorded as P5-f.memo(ChatMessageList)no longer short-circuits for hosts (they pass a freshtranscriptobject each render); the expensive memos inside are keyed on their own deps and are unaffected.Follow-ups (recorded in the RFC §4.2 as P5-f)
ChatPage mounts
VirtualTranscript(deleting its inline wiring;TranscriptScrollShellmoves into chat-core); ChatPane's load-earlier as abefore-cursor pager;MeasureFarmfor hosts; ChatEmbed incremental (since-cursor) polling instead of a per-poll page re-read.