Skip to content

perf(chat): keep the pane hydrate and warm bounded while a slot runs - #10026

Open
CrysisDeu wants to merge 1 commit into
mainfrom
fix/chatpane-bounded-hydrate-running
Open

perf(chat): keep the pane hydrate and warm bounded while a slot runs#10026
CrysisDeu wants to merge 1 commit into
mainfrom
fix/chatpane-bounded-hydrate-running

Conversation

@CrysisDeu

@CrysisDeu CrysisDeu commented Sep 11, 2026

Copy link
Copy Markdown
Collaborator

Part 1 of 2 for #10005 (chat-core P5-e). Stops the bleeding: the pane's history reads stay bounded. Rebased onto main after PR-2 (#10146, the virtualized transcript) merged, so the Members DM's earlier-history bar now rides VirtualTranscript's earlier slot.

Summary

Two hydrate paths pulled a running slot's whole transcript, and neither reason still holds:

  • ChatPane hydrate (components/ChatPane.tsx:470-476 on main) lifted PANE_HYDRATE_LIMIT to undefined and latched it the moment the slot was running. The justification from perf(chat): bound the per-pane history hydrate in the session grid #3240 ("a limit slices raw chunk rows mid-stream") was falsified by 1afc7dc — the handler collapses chunk runs before it computes total and slices — but the lift stayed. The bound now holds whether or not the slot runs; a turn's rows reach the pane over the WS routing, not this fetch.
  • warmSlotCache (store/chatSlice.ts:2846 on main) fires on every background chat_done and went unbounded while streaming or whenever the pane already held rows (fix(chat): stop a bounded switch refetch shrinking a painted transcript #6839). perf(chat): reachable archived history and stable phone scrolling #7916 retired exactly those exemptions for switchSlot and left the warm as the one branch it did not revisit. The warm now sizes its window on the durable rows the pane holds (slotSwitchFetchLimit, floor PANE_HYDRATE_LIMIT, cap 500) and accepts the page when it can be stitched onto the cache — the fulfilled reducer's actual need (it keeps any head above the page and rescues any tail below it), not slotCoverageShortfall's multiset, which would read a row appended during the await or a cache past 500 rows as a hole and send every warm of a busy pane unbounded.

Also fixes three stale "Virtuoso" comments (ChatPage.tsx:5108, :6105, ChatMessageList.tsx header): the virtualizer is our own hooks/virtualizer/useVirtualChat, and ChatPage does not use ChatMessageList.

Why now

The Crew Members page's member DM gets slower the longer it is used. One real thread is 10 MB / ~3000 persisted rows; a member thread is almost always running (monitor loops, patrols), so opening it took the unbounded read and rendered ~3000 rows, and every completed turn re-pulled the full 10 MB through the warm. Fixing only the hydrate would have lasted until the member's first turn finished.

What changed

website/src/components/ChatPane.tsx

  • hydrateLimit starts at PANE_HYDRATE_LIMIT; the running latch and its useRef pair are gone, so no second fetch is issued when a turn starts mid-hydrate.
  • A bounded page is not the end of the road — every older row stays reachable. A host that can open the full session keeps its "earlier messages" row; a host that cannot (the Crew Members DM) gets the transcript's earlier-history bar, which widens the bounded window one page per press (hydrateSlotMessages accepts a wider bounded page over the held one) up to the handler's 500 cap — and at the cap the bar stays: the next press is the explicit full-history read (no limit), which the reducer accepts as the final, widest page. So a thread longer than 500 rows reaches row 0 in ten presses, and the only unbounded read is the one the reader asked for (the maintainer's floor for landing the bound). Cursor paging with next_before can replace the last step later without changing what the reader sees.

website/src/store/chatSlice.ts

  • warmSlotCache: bounded window sized on durable rows → pageStitchesOntoView decides 'complete' | 'overlaps' | 'spans' | null; 'spans' alone is accepted only when no mid-less durable row sits above the cache's oldest identified row (unidentifiedDurableHead, legacy history). Anything else takes the unbounded retry, which carries the bounded page's collapsed total (comparableTotal) — the retry's own count is raw. Net effect for a ~3000-row pane: one 500-row read per chat_done, head kept by the reducer.
  • pageStitchesOntoView / identifiedServerRows are extracted from refreshSlot's inline post-fetch check so the two share one predicate; refreshSlot behaviour is unchanged (truthy test over the same three conditions, same requireTs anchors).
  • warmSlotCache.fulfilled: total is the bounded count (own or carried) for the shrink/rewrite comparisons and retention alike; a raw count is never a baseline.
  • retainServerTotal retains only a count in settled units (settledTotalOf): an unbounded read's raw total (a done row per finished turn even when settled) is never used — a complete unbounded read derives its settled count from its prepared durable rows instead, so an empty-view refresh still leaves a baseline; a bounded running count is normalised by settledBoundedTotal: the bounded corpus keeps pending permission cards in its count and drops answered ones, so a mid-turn total exceeds the settled one by exactly the cards in the page — subtract them and the result is a floor the settled count can only grow from. Retained raw, the next settled warm read the cards' resolution as a truncation and dropped its rescued tail; refused outright, a slot that streams most of its life never got a baseline and a remote rewind of it was never recognised. switchSlot, refreshSlot, the hydrate and the warm all go through the one policy.
  • warmSlotCache re-validates after each await against a per-key generation (slotClearSeq: moved by clearSlotCache, clearMessages and evictSlotState, and deliberately not evicted with the slot so a delete-and-recreate under the same key reads as a move), plus an emptied-cache check and the slot's presence in the loaded authoritative list; any of them makes the thunk discard the page instead of writing a transcript that no longer exists back.
  • hydrateSlotMessages first path drops live frames the page already carries (tailNotInPage): a duplicated mid cannot anchor and would have sent the first warm unbounded. Docblock updated (the unbounded-upgrade branch is kept for callers/tests but no host takes it).
  • Two stale comments in fetchSlotDetail and switchSlot that still described the warm as unbounded-while-streaming.

Screenshots (real pod, kirocrew pod up wt-p5e-pr1 --seed minimal, a 200-row member-default thread)

Crew Members DM opens on the newest bounded page (50 display rows of 200) with the load-earlier bar; one press widens the window to 100 rows. Captured by website/scripts/capture-members-dm-load-earlier.mjs against the running pod (asserts the row counts).

newest page + load-earlier bar (50 rows) after one press (100 rows)
Members DM opens on the newest 50 rows with a Load earlier messages bar After one press the window holds 100 rows, starting at turn 51

What was tested

Only the static gates ran locally: npx tsc -b (exit 0) and npx eslint on the seven changed files (exit 0). No local test run — CI runs the suites.

Tests added/changed (CI-run):

  • ChatPane.hydrateBound.test.tsx: running slot hydrates with PANE_HYDRATE_LIMIT; a turn starting mid-hydrate issues no unbounded refetch (absence asserted after settle, not waitFor); the running page reports bounded and retains the running bounded count in settled units; the load-earlier bar appears only when the host passes no onOpenFull, widens by one page per press; >500-row regression: a 600-row thread widens 50 → 500 over nine presses, the bar stays at the cap, the tenth press issues the full read (limit undefined) and row-0 is held with the bounded marker cleared — and no unbounded read happened before the reader asked; a wider bounded page supersedes the held one and keeps the live tail.
  • chatSlice.warmSlotCacheBound.test.ts: streaming warm is bounded; window ≥ durable rows held, sized on durable rows only; a 3000-row pane stays intact on one 500-row read; no retry over server growth or a live row appended in flight; retry only on a page that anchors nowhere or only spans a mid-less head; a tail error bubble does not pin the slot unbounded; no retry at start of history; the retry carries the bounded count; a running bounded count is retained in settled units (pending cards subtracted), on the page path and through the retry.
  • useWebSocketReconnect.splitPanes.test.ts: streaming member of a split warms bounded.
  • store/chatSlice.boundedRefetchShrink.test.ts: the background-warm pin now asserts the property (first read sized ≥ what the pane holds; a window disjoint by construction takes the unbounded retry; nothing shrinks) rather than the old [undefined] mechanism.
  • Round 11 (GPT lane + own test): a RUNNING complete unbounded read yields no derived count (settledTotalOf returns undefined; it would be short by the in-flight row and hide a later one-row rewind), so the bounded page's normalised count carries through the retry; a cold slot deleted and recreated under the same key is detected through the recreated record's created (the eviction cannot move the generation of a slot with no residue), alongside the generation for slots with residue. Tests for both.
  • Round 10 (GPT lane): slotClearSeq becomes a per-key generation that survives eviction and is bumped by evictSlotState, so a slot deleted and recreated under the same key during a cold warm no longer receives the deleted transcript's page; test with real dashboardReducer (evict, recreate, resolve).
  • Round 9 (GPT lane): the wider-page upgrade branch (now live via load-earlier) reconciles its tail with liveTailNotInPage like the first hydrate, so a repeated sendId cannot drop a distinct confirmed live row; reducer test added.
  • Round 8 (GPT lane): cold-cache slot deletion during a warm now detected via the loaded dashboard.slots list (test with a real dashboard reducer); a complete unbounded read derives its settled count from prepared durable rows (settledTotalOf) so an empty-view refresh leaves a baseline (tests re-pinned: 120 durable rows against a raw 900; hydrate → 2 against a raw 7).
  • Round 7 (GPT lane): /clear or delete landing during a warm's fetch or retry → page discarded (slotClearSeq + emptied-cache check), two tests. Also chatSlice.warmSlotCacheBound.test.ts re-pinned where an unbounded hydrate used to move the count (raw units are refused now).
  • Round 6 (GPT lane): an unbounded settled refreshSlot no longer seeds the baseline (raw done-inclusive units vs. the bounded warm's collapsed units); retainServerTotal keeps only bounded normalised counts or a carried comparableTotal. serverTotalBaseline.test.ts pins the unbounded refusal; the refresh positive-control test now paints the view so its refresh is the bounded read.
  • Round 5 (GPT lane): running bounded counts are retained again, normalised by settledBoundedTotal (pending permission rows in the page subtracted) on every path, so neither the inflated-baseline drop (round 3) nor the absent-baseline rewind miss (round 5) applies. serverTotalBaseline.test.ts re-pinned (running bounded switch → 900; two pending cards → 900 from a raw 902; unbounded retry still refused); warm/hydrate tests pin the same normalisation.
  • Round 4 (GPT lane): liveTailNotInPage matches a confirmed row through idAnchorsOneRow (the file's one identity invariant: the mid names exactly one row on each side and the two rows do not contradict on ts) rather than a bare mid set lookup, since meta.mid is minted only when absent and a caller can post one twice. Two reducer tests pin the declines (ts contradiction; mid carried twice by the page).
  • Round 3 (GPT lane): retainServerTotal now refuses a running count on every path; chatSlice.serverTotalBaseline.test.ts re-pinned to the unified policy (its earlier premise — that the switch's coverage check reads the baseline — has been stale since perf(chat): reachable archived history and stable phone scrolling #7916).
  • Round 2 (GPT lane): the hydrate's live-tail dedupe matches a confirmed row on its mid only (liveTailNotInPage); sendId is used solely for rows the server has not yet named, so a repeated sendId across two valid sends cannot drop the newer row. Three reducer tests pin it.

Four rounds of blind adversarial review (Logic / Concurrency lenses) on the source diff; every Required/Critical finding was fixed and re-reviewed. Three Low findings are left deliberately and listed below.

UX CONCERNS

  • Members DM opens on the newest 50 rows, with a Load earlier messages bar at the top (screenshots above). Each press widens the window by a page; a very long thread reaches the handler's 500-row cap after nine presses, and the tenth press loads the full history — so every older row a user could reach before this PR is still reachable, at the cost of one explicit unbounded read. Before this PR the pane loaded all ~3000 rows on every open and froze.
  • Scroll follow / pinned banner / stick-to-bottom: unchanged from main (the pane is virtualized since feat(chat-core): P5-e — one virtualized transcript for every chat surface #10146); only how much history it fetches changes.
  • A pane that was already holding a full transcript (opened before this fix, or cached from switchSlot.pending) keeps it: the bounded warm anchors and the reducer keeps the head. Render cost for such a pane is the virtualizer's (feat(chat-core): P5-e — one virtualized transcript for every chat surface #10146); the 10 MB re-fetch per turn is gone.

Known Low residuals (documented, not fixed here): a client-minted error/notice row that is the pane's first durable row keeps that pane on the unbounded warm (unlikely layout; safe direction); under 'spans' a page shorter than the cache is discarded by the reducer's keptPrior rather than merged (no data loss; reconciled by a later warm).

Reviewer questions (bolichen97)

  1. Does settledTotalOf's client-side count equal the bounded handler's collapsed total for persisted error / mcp_oauth / tool rows? Yes. The bounded total is len(_collapse_wire_rows(all_msgs)) (chat_handlers.py bounded branch), and an unbounded read's messages come through _prepare_messages, whose loop iterates _collapse_wire_rows(messages) as well (chat_utils.py): both sides start from the same collapsed row set — every persisted row, error/mcp_oauth/tool included, done dropped, a chunk run folded to one row. isDurableRow excludes only queued/streaming/thinking/permission, so the two counts can differ by exactly (a) the pending permission cards, which settledBoundedTotal subtracts on the bounded side and isDurableRow excludes on the unbounded side, and (b) the one collapsed in-flight row while a turn runs, which the bounded count includes and the unbounded derivation does not — an under-count of at most 1 on the unbounded side, which is the safe direction (a baseline below the settled count can never read as a shrink).
  2. Do WS-delivered ts strings match the slot-detail handler's format for the same row? Both are the server's own stamp for that row (monotonic_transcript_ts at append; the WS frame carries the persisted row's ts, and the handler returns the persisted row), so they agree for assistant/tool rows. The one row that can differ is the dashboard's own optimistic user bubble, whose ts is the client ISO stamp and which has no server echo — its identity is the mid the send receipt supplies (confirmOptimisticSend). idAnchorsOneRow in the stitch check anchors on mid and treats ts as a non-contradiction test, so an optimistic user row is never the anchor row that decides the page (the oldest identified row / the page's oldest row are server rows). Where a ts mismatch could bite is perf-only: an unnecessary unbounded retry, never a drop — and the earlier rounds' tests (does not retry over a live row appended while the window was in flight) cover the appended-row case.

Follow-ups

  • PR-2 (P5-e proper) — merged as feat(chat-core): P5-e — one virtualized transcript for every chat surface #10146.
  • Cursor paging (next_before) for the pane's earlier-history bar beyond the handler cap, replacing the explicit full-history read as the last step.
  • P5-f: switch ChatPage onto the shared unit.
  • Have the handler report a settled total (pending permission rows excluded) so the client-side settledBoundedTotal normalisation can go.

Closes nothing; part of #10005.

@CrysisDeu
CrysisDeu requested a review from a team September 11, 2026 00:48
@CrysisDeu CrysisDeu added the area: dashboard Dashboard UI and its backend handlers label Sep 11, 2026
@CrysisDeu
CrysisDeu requested a review from a team as a code owner September 11, 2026 00:48
@CrysisDeu
CrysisDeu requested a review from dwu96 September 11, 2026 00:48
@github-actions github-actions Bot added the readiness: checking Automated validation is still running label Sep 11, 2026
@github-actions

github-actions Bot commented Sep 11, 2026

Copy link
Copy Markdown
Contributor

Design Review (Fable 5) — 🟡 CONCERNS

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

Design-Verdict: CONCERNS

Sound bounded-fetch design, but its correctness now rests on client code re-deriving the server's row-counting semantics with nothing pinning the two sides together.

Watch

  • settledTotalOf/settledBoundedTotal hard-code the backend's counting behavior (_collapse_wire_rows order, _is_answered_permission window rules, _prepare_messages drops) on the client. The PR verifies the equivalence by hand (reviewer question 1) and every new test is frontend-only, so a backend change to collapse or permission-window rules silently skews the "settled units" math — and the failure mode is the quiet class this PR spent eleven rounds on (a unit skew read as truncation drops a rescued tail; a missed rewind restores a superseded reply). The listed follow-up (handler reports a settled total) is the real fix; until then the contract is unguarded.
    Clears when: a cross-boundary test pins client settled-unit derivation against the handler's actual bounded/unbounded counts, or the handler ships the settled total and the client normalisation is deleted.

  • Two ~416 KB PNGs are force-added under temp-screenshots/, which this repo's own .gitignore designates as "uploaded as a GitHub attachment … never committed" — the raw-URL embeds in the description only work because the rule was bypassed, and the blobs stay in history permanently after any tip-side removal.
    Clears when: the PNGs are dropped from the branch and attached via gh pr edit --attach (or a PR comment) instead.

[DESIGN-REVIEWED] 6d57406

@github-actions

github-actions Bot commented Sep 11, 2026

Copy link
Copy Markdown
Contributor

UX Review (Fable 5) — 🟡 CONCERNS

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

UX-Verdict: CONCERNS

The load-earlier bar read cleanly blind, but a failed widen stacks two contradictory error notices and its "Retry" secretly widens instead of retrying.

Watch

  • ChatPane feeds the bar the pane-wide flag (failed: slotDetailFailed) and keeps the pre-existing aboveRows error on the same flag, so one failed widen renders the bar's "Couldn't load earlier messages [Retry]" stacked on "Couldn't load this session's messages. [Retry]" — the second contradicts the transcript visibly loaded below it. And the bar's Retry calls widenHydrate, which steps the limit again (at the 500 cap it escalates a failed retry into the full unbounded read of a 10 MB thread). Rare (fetch failure) × friction/misleading copy × every failure. Fix: mirror the sibling ChatEmbed.tsx:145,330 — derive a widenFailed (error while hydrateLimit > PANE_HYDRATE_LIMIT), feed only it to the bar with onLoad: widenFailed ? refetchSlotDetail : widenHydrate, and gate the aboveRows notice to the initial hydrate.

Evidence gaps

  • The bar's loading and failed states in this surface (including the double-notice stack above) appear in no screenshot; a shot of a failed widen would close it.

[UX-REVIEWED] 6d57406

@github-actions

github-actions Bot commented Sep 11, 2026

Copy link
Copy Markdown
Contributor

First Principles Review (Fable 5) — ✅ PASS

Premise-level review of 6d57406cea19a384d8c4b9ff64a45827725cfbaf — 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.

First-Principles-Verdict: PASS

Verify the at-cap full-history press on a real >500-row thread — the pod evidence covers only the 200-row widen path, vitest covers the rest.

Not justified as shipped

  1. Three stale "Virtuoso" comments corrected — rides along: declared, harm-free comment fixes in a perf PR.

What this change ships

Inventory (9 items) — 8 justified

Intent: stop a long-lived running thread (Members DM, 10 MB/~3000 rows) from being re-read whole on every open and every completed turn — a FIX (#10005, measured, with tests that fail on base).

  1. Opening a running slot's pane loads the newest page, not the whole transcript — justified
  2. Each background turn-completion warm reads one bounded page (≤500), unbounded only when the page can't stitch — justified
  3. Members DM gains an "earlier messages" bar: one page wider per press; at the 500 cap the next press reads full history — justified
  4. A wider bounded page now replaces the held one (was: only the unbounded refetch, once) — justified
  5. Server counts retained only in settled units; a running bounded read now seeds a baseline, pending cards subtracted — justified
  6. A warm landing after a /clear, delete, or delete-and-recreate discards its page instead of resurrecting the transcript — justified
  7. A live WS frame the hydrate page already carries is no longer held twice — justified
  8. Three stale "Virtuoso" comments corrected — rides along
  9. New pod capture script + two committed screenshots — justified (repo evidence convention: ux-review.yml consumes temp-screenshots/**; ~60 sibling capture-*.mjs scripts)

Deleted pins ("unbounded while streaming is deliberate", the [undefined] warm-limit assertion, "a bounded count is already in settled units") are each overturned with checkable evidence — commit 1afc7dc, the #7916 precedent, and new tests that fail on base — not recast as gaps. Backend premises verified in-repo: _is_answered_permission/_UNOWED_WINDOW_ROLES (chat_handlers.py:1648) keep pending cards in the bounded count; slot frames carry created (slot_projection.py:306). Remaining unbounded reads (explicit full press, stitch retry, empty-view refresh) are enumerated and deliberate; cursor paging is the recorded part 2.

[FIRST-PRINCIPLES-REVIEWED] 6d57406

@github-actions

github-actions Bot commented Sep 11, 2026

Copy link
Copy Markdown
Contributor

GPT 5.6 Review — ✅ no blocking findings

GPT 5.6 completed its review of 6d57406cea19a384d8c4b9ff64a45827725cfbaf and found no blocking issues.

This comment is updated in place on each push.

Review details

FINDING -- website/src/components/ChatPane.tsx:489 -- passive "useEffect" resets after rebinding, so the new slot first fetches with the prior slot’s widened or unbounded limit -> Fix: key the limit state by slot and derive the default synchronously on mismatch.
FINDING -- website/src/components/ChatPane.tsx:1279 -- "onLoad: widenHydrate" advances after a failed request, so Retry skips the failed limit, becomes unbounded at 500, and no-ops after an unbounded failure -> Fix: use refetchSlotDetail when slotDetailFailed.
[GPT-REVIEWED] 6d57406

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

@github-actions

github-actions Bot commented Sep 11, 2026

Copy link
Copy Markdown
Contributor

Opus 4.8 Review — ✅ no blocking findings

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

Review details

No findings that block.

FINDING — website/src/components/ChatPane.tsx:489 — useEffect(() => { setHydrateLimit(PANE_HYDRATE_LIMIT) }, [slotKey]) resets the limit in a post-commit effect, so on a Members-DM rebind (<ChatPane slotKey={activeSlot}> with no key) from member A — whose window was widened via the load-earlier bar — to member B, the render where slotKey becomes B still carries A's hydrateLimit; react-query's setOptions effect (registered after this reset effect) then dispatches queryKey ['slot-messages', B, hydrateLimit ?? 'all']api.chatSlotDetail(B, staleLimit), a wasted oversized/unbounded fetch of member B's transcript (a full-transcript read + transfer + JSON parse when A was widened to undefined) before the reset re-render issues the bounded [B, 50] fetch that supersedes it → Fix: reset during render instead of in an effect — track the previous slotKey in a ref and if (prev.current !== slotKey) { prev.current = slotKey; setHydrateLimit(PANE_HYDRATE_LIMIT) }, so the query never renders for a new slot under the prior slot's limit.

[OPUS-REVIEWED] 6d57406

Verdict parsed from the review's SHA-scoped output markers for commit 6d57406cea19a384d8c4b9ff64a45827725cfbaf.

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

@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 11, 2026
@CrysisDeu
CrysisDeu force-pushed the fix/chatpane-bounded-hydrate-running branch from 6195ec4 to 2ee1875 Compare September 11, 2026 01:06
@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 11, 2026
@CrysisDeu

Copy link
Copy Markdown
Collaborator Author

Round 2 (2ee1875d50):

  • GPT F1 (sendId collision drops a distinct live row) — fixed. The hydrate's live-tail dedupe now goes through liveTailNotInPage: a row that already carries the server's mid is matched on that alone; sendId is consulted only for a row the server has not yet named. Pinned by three reducer tests in ChatPane.hydrateBound.test.tsx (mid match drops the raced copy; repeated sendId across two confirmed rows keeps both; an unconfirmed row still matches by sendId).
  • Frontend Tests (4)store/chatSlice.boundedRefetchShrink.test.ts pinned the old mechanism (limit === undefined on a cached warm). Its fixture caches the oldest 111 rows while the window is the newest 111, so the two are disjoint by construction and the retry is the correct answer; the test now asserts the property (first read sized ≥ what the pane holds, unbounded retry, no shrink), in the file's own "property, not the mechanism" style.
  • Design / UX CONCERNS (Members DM history access until PR-2) — acknowledged as a deliberate interim, stated in the body. PR-2 (chat-core P5-e, chat-core P5-e: virtualized transcript for every shared chat surface; ChatPane hydrates unbounded while running #10005) brings in-place load-earlier paging to ChatPane and the other three ChatMessageList hosts; wiring onOpenFull from Members would send the user out of the Members surface into a full session for a slot that surface owns, which is the wrong door. The running-count three-way divergence is tracked in chat-core P5-e: virtualized transcript for every shared chat surface; ChatPane hydrates unbounded while running #10005 as a follow-up.
  • Screenshot Evidence — fetch-bound change with no rendered delta; the body now carries the <!-- no-visual-delta --> marker with its justification line.

@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 11, 2026
@CrysisDeu
CrysisDeu force-pushed the fix/chatpane-bounded-hydrate-running branch from 2ee1875 to 9175dd6 Compare September 11, 2026 01:27
@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 11, 2026
@CrysisDeu

Copy link
Copy Markdown
Collaborator Author

Round 3 (9175dd668f):

  • GPT F1 (running switch total seeds a permission-inflated baseline → later settled warm reads a shrink → tail dropped) — fixed at the policy, not the symptom. retainServerTotal now refuses a running count on every path and its boundedRead arm is gone; switchSlot.fulfilled, refreshSlot.fulfilled, hydrateSlotMessages and warmSlotCache.fulfilled all call the one rule. This also closes the three-way divergence the Design lane flagged. The premise that once justified accepting a bounded running count — the switch's coverage check needing a baseline to prove overlap — has been stale since perf(chat): reachable archived history and stable phone scrolling #7916 made coverage row-based (slotCoverageShortfall reads no count), so refusing costs only rewind detection for a slot that never settles, the "decline, not guess" direction the warm already takes. chatSlice.serverTotalBaseline.test.ts is re-pinned accordingly (running bounded switch → no baseline; the observed-hole retry still happens and still retains nothing).

@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 11, 2026
@CrysisDeu
CrysisDeu force-pushed the fix/chatpane-bounded-hydrate-running branch from 9175dd6 to e61a064 Compare September 11, 2026 01:38
@CrysisDeu

Copy link
Copy Markdown
Collaborator Author

Round 4 (e61a064b92):

  • GPT F1 (repeated caller-supplied mid drops a distinct live row) — fixed. I am not taking the pre-drafted override: its premise that meta.mid is never caller-supplied is wrong — _ChatSlot.append (state.py:4380) mints a mid only when the row carries none, and this file already treats a repeated mid as a real hazard (idAnchorsOneRow's docblock). liveTailNotInPage now matches a confirmed row through that same invariant: the mid must name exactly one row on each side and the two rows must not contradict on ts; otherwise both are kept (a transient double the next warm reconciles, which is what an undeduped hydrate did before this PR). sendId remains consulted only for a row the server has not yet named. Two new reducer tests in ChatPane.hydrateBound.test.tsx pin the declines.

@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 11, 2026
@github-actions github-actions Bot added the readiness: checking Automated validation is still running label Sep 11, 2026
@CrysisDeu

Copy link
Copy Markdown
Collaborator Author

@bolichen97 — addressed in bad3ad7463 (single commit, rebased content only):

  • Members DM history access. Took shape 1, the minimal load-earlier: when the host passes no onOpenFull (the Crew Members DM), ChatPane renders EarlierMessagesBar, and each press widens the bounded window by one page (hydrateSlotMessages now accepts a wider bounded page over the held one — the previously dead bounded→unbounded upgrade branch, generalised to "wider", so it has a live caller again). Capped at the handler's 500 rows; P5-e's cursor pager replaces it. Split panes keep the "earlier messages → open session" row unchanged. Reducer + component tests added.
  • Screenshots. Real pod (kirocrew pod up wt-p5e-pr1 --seed minimal, a 200-row member-default thread): DM opens on 50 display rows with the bar; one press → 100 rows. Captured by the committed website/scripts/capture-members-dm-load-earlier.mjs, which asserts the row counts. The no-visual-delta waiver is gone from the body.
  • Contradictory test bullets reworded to "retains the running bounded count in settled units".
  • Two questions answered in the body under "Reviewer questions": both count paths start from the same _collapse_wire_rows output (_prepare_messages iterates it), so persisted error/mcp_oauth/tool rows are counted identically and the only deltas are pending cards (subtracted / excluded on each side) and the one in-flight row (an under-count of ≤1 on the unbounded side, the safe direction); WS and slot-detail ts are the same server stamp for server rows, the optimistic user bubble is identified by the receipt's mid, and the stitch anchors never fall on it.

@CrysisDeu
CrysisDeu force-pushed the fix/chatpane-bounded-hydrate-running branch from bad3ad7 to 3bae0f1 Compare September 11, 2026 07:34
@CrysisDeu

Copy link
Copy Markdown
Collaborator Author

Head is now 3bae0f16a1: same content as bad3ad7463 plus a lint fix — jscpd flagged the new capture script's Members boot block as a clone of record-members-activity-days.mjs, so it moved into scripts/lib/crew-pod-harness.mjs as openMembersDm and the capture script calls it. Screenshot links in the body re-pinned to the new SHA.

@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 11, 2026
@CrysisDeu
CrysisDeu force-pushed the fix/chatpane-bounded-hydrate-running branch from 3bae0f1 to ebef572 Compare September 11, 2026 07:45
@CrysisDeu

Copy link
Copy Markdown
Collaborator Author

Round 9 (ebef5724ba): GPT F1 — the wider-page upgrade branch, now a live path through the load-earlier bar, reconciled its tail with the bare tailNotInPage; it now uses liveTailNotInPage like the first hydrate (confirmed rows match on mid through the anchor invariant, sendId only names an unconfirmed row). Reducer test: a repeated sendId across two confirmed rows keeps both. Screenshot links re-pinned.

@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 11, 2026
@CrysisDeu
CrysisDeu force-pushed the fix/chatpane-bounded-hydrate-running branch from ebef572 to 61b13a7 Compare September 11, 2026 07:58
@CrysisDeu

Copy link
Copy Markdown
Collaborator Author

Round 10 (61b13a7c58): GPT F1 — fixed as suggested: slotClearSeq is now a per-key generation that is bumped by evictSlotState too and is deliberately kept out of slotKeyedMaps, so a cold warm whose slot is deleted and recreated under the same key mid-flight sees the generation move and discards its page. Test with the real dashboardReducer: evict via sseSlots, recreate, resolve → nothing written. Screenshot links re-pinned.

@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 11, 2026
@CrysisDeu
CrysisDeu force-pushed the fix/chatpane-bounded-hydrate-running branch from 61b13a7 to 065e987 Compare September 11, 2026 08:12
@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 11, 2026
@CrysisDeu

Copy link
Copy Markdown
Collaborator Author

Round 11 (065e987a81): GPT F1 — fixed as suggested: settledTotalOf returns undefined for a RUNNING complete unbounded read (short by the in-flight row), so the bounded page's normalised count carries through the retry as the baseline; test pins 400 retained from the bounded page rather than the retry's own 5. Also fixed my own round-10 test: a cold slot leaves no residue for the eviction to move the generation on, so the warm now also compares the slot record's created before/after each await and discards on a recreated record; the generation still covers slots with residue (both tested with the real dashboardReducer). Screenshot links re-pinned.

@github-actions github-actions Bot added readiness: passed Eligible automated validation passed for the current revision and removed readiness: checking Automated validation is still running labels Sep 11, 2026

@bolichen97 bolichen97 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maintainer re-review (agent-assisted), pinned to 065e987. Requesting changes: the Members DM has no full-session fallback, and Load earlier only expands to 500 then disappears. The body now explicitly acknowledges that older rows in a longer thread are inaccessible until P5-e lands. This removes access users had before; a future pager is not a present recovery path.

Owner-delegated decision: bounded initial hydration and incremental reads are welcome, but retain a working route to all older history now (cursor paging or an explicit safe full-history fallback) before landing. Add a >500-row regression proving the earliest rows remain reachable. The warm-cache stitching work can remain; the blocker is the DM access cap, not the bounded initial page. Source/test review and current-head CI evidence only; local suites not rerun in this wave.

…10005)

A Crew Members DM thread got slower the longer it was used: one real thread
is 10 MB / ~3000 persisted rows, and opening it -- plus every turn the member
finished -- pulled and rendered the whole transcript.

Two paths went unbounded for a running slot, and neither reason still holds.

ChatPane's hydrate lifted PANE_HYDRATE_LIMIT to undefined and latched it as
soon as the slot was running (#3240), on the theory that a limit would slice
the in-flight response's raw chunk rows. 1afc7dc corrected that: the handler
collapses chunk runs before it computes total and slices, so the hazard never
existed -- but the lift stayed. A member thread is almost always running, so
the pane always took the unbounded read. The bound now holds regardless; the
turn's own rows reach the pane over the WS routing, not this fetch. The first
hydrate also drops live frames the page already carries (tailNotInPage): a
duplicated mid cannot anchor, so it would have sent the first warm unbounded.

warmSlotCache fires on every background chat_done and went unbounded both
while streaming and whenever the pane already held rows (#6839). #7916
retired exactly those two exemptions for switchSlot -- verify after the
response instead of pre-purchasing a wider read -- and left the warm as the
one branch it did not revisit. The warm now sizes its window on the durable
rows the pane holds (slotSwitchFetchLimit, floor PANE_HYDRATE_LIMIT, the
handler's 500 as cap) and accepts the page when it can be STITCHED onto the
cache: it reaches the start of history, its own oldest row is in the cache
(the head-keeping cut), or it contains the cache's oldest identified row and
no mid-less durable row sits above that (pageStitchesOntoView +
unidentifiedDurableHead; the stitch predicate is extracted from refreshSlot's
post-fetch check so the two share one). That is the fulfilled reducer's
actual need -- it keeps any head above the page and rescues any tail below it
-- and deliberately not slotCoverageShortfall's multiset, which would read a
row appended during the await, or a cache past 500 rows, as a hole and send
every warm of a busy pane to the unbounded read. A ~3000-row pane therefore
reaches a steady state of one 500-row read per chat_done.

Counts stay in one unit. The unbounded retry's own total is raw (a `done` per
finished turn), so the retry carries the bounded page's collapsed count and
the reducer's shrink/rewrite comparisons and retention all read that one; a
raw count is never a baseline: only a count in SETTLED units is
(retainServerTotal via settledTotalOf). A bounded read's count is the
collapsed corpus less the pending permission cards it keeps (they are in the
page), a floor the settled count can only grow from -- raw, the next settled
warm read that fall as a truncation and dropped its rescued tail. An
unbounded read's `total` counts a `done` per finished turn and is never used;
a COMPLETE unbounded read derives its settled count from its prepared rows
instead, so an empty-view refresh still leaves the baseline a later remote
regeneration is recognised against. One policy on switchSlot, refreshSlot,
the hydrate and the warm.

The warm re-validates after EACH await: a /clear (slotClearSeq, bumped by
clearSlotCache / clearMessages), an emptied cache, or the slot's absence from
the loaded authoritative slot list (a delete evicts every per-slot trace, so a
cold cache alone cannot show it) discards the page, which describes a
transcript that no longer exists; a reducer guarding only on activeSlot would
write it back.

A bounded page must not be the end of the road. A host that can open the
full session keeps its "earlier messages" row; the Crew Members DM (no full
session to open) gets a load-earlier bar that WIDENS the bounded window a page
at a time (hydrateSlotMessages now accepts a wider bounded page over the held
one), up to the handler's cap -- the cursor-based pager of P5-e replaces it.
Real-pod capture: temp-screenshots/members-dm-load-earlier/ (50 display rows
on open, 100 after one press; scripts/capture-members-dm-load-earlier.mjs).

Also corrects three stale "Virtuoso" comments (ChatPage x2, ChatMessageList
header): the virtualizer is hooks/virtualizer/useVirtualChat, and ChatPage
does not use ChatMessageList.

Tests (CI-run): ChatPane stays bounded while running and issues no unbounded
refetch when a turn starts mid-hydrate; warm stays bounded while streaming,
sizes on durable rows, keeps a 3000-row pane on one 500-row read, does not
retry over server growth or a live row appended in flight, retries only on a
page that anchors nowhere or only spans a mid-less head, skips the retry at
the start of history, carries the bounded count through a retry, and never
retains a running count.
@CrysisDeu

Copy link
Copy Markdown
Collaborator Author

Round 11 (6d57406cea) — addressing the re-review (owner-delegated decision) and the merge of #10146:

  • Rebased onto main (P5-e landed first). Three conflicts resolved: the Members DM's earlier-history bar now rides VirtualTranscript's earlier slot instead of a bar mounted above the list; ChatMessageList's header keeps main's P5-e text; the switchSlot retry keeps main's emitSlotRead guard and this PR's settledBoundedTotal.
  • DM access cap removed — a working route to all older history, now. The bar no longer disappears at the handler's 500-row cap: it stays, and the next press is the explicit full-history read (no limit), which hydrateSlotMessages already accepts as the final, widest page over a bounded one. A >500-row thread reaches row 0 in ten presses; the only unbounded read is the one the reader asked for, so the bounded initial hydration and the warm-cache stitching are untouched. Cursor paging over next_before can replace that last step later without changing what the reader sees (listed under follow-ups).
  • >500-row regression (ChatPane.hydrateBound.test.tsx): a 600-row thread widens 50 → 500 over nine presses, the bar stays at the cap, the tenth press issues the full read and row-0 is held with the bounded marker cleared; the test also asserts no unbounded read happened before that press.
  • Body updated: the "inaccessible until P5-e lands" acknowledgement is gone (it is no longer true), UX section and follow-ups reflect feat(chat-core): P5-e — one virtualized transcript for every chat surface #10146 having merged.

Static gates: npx tsc -b exit 0, eslint clean on the changed files. Tests run in CI.

@CrysisDeu

CrysisDeu commented Sep 12, 2026

Copy link
Copy Markdown
Collaborator Author

Round 11 CI note: the one red on 6d57406cea is Backend Tests (Windows) (3)test_remove_slot_for_history_key.py::…::test_malformed_persisted_owner_does_not_poison_unrelated_removal (assert '' == ['not', 'a', 'string']). This PR touches no Python (frontend + tests + one capture script), so it is not this diff's.

Update: the rerun failed on the same single test (1 failed / 22287 passed), and main's latest completed run of this workflow (run 34673414316 on cb7fe76506) fails the identical test on the same Windows shard — inherited from main, not introduced here. Everything else, including the new >500-row regression, is green.

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

Labels

area: dashboard Dashboard UI and its backend handlers readiness: action required A blocking check or review needs attention

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants