fix: O(N) session switch and intermittent message overlap in web UI - #73
Merged
Conversation
Two bugs, both rooted in unnecessary O(N) work during session switches.
**O(N) session switch slowness**
`replaceScrollback` ran a `for...of messages` loop inside a SolidJS
`produce()` callback to bump a version counter for every replayed
message. For a 200-message session this created 200 fine-grained store
mutations per switch — path-tracked by Solid's reactive internals even
inside a batch. `versionOf` is not used in any render path (only in
tests), so this work had no downstream effect.
Fix: remove the per-message version loop. The session epoch bump that
already happens is the only invalidation signal consumers need.
Additionally, `hasMessage` (called on every streaming delta to gate
`applyDelta`) used `buf.some(m => m.messageId === messageId)` — an O(N)
linear scan per delta. Fix: add a plain `Map<sessionId, Set<messageId>>`
outside the Solid store. `hasMessage` becomes O(1), `applyMessage` and
`replaceScrollback` maintain the index. The Map lives outside the store
so inserts don't create reactive overhead.
**Intermittent message overlap**
On every session switch, `virtualizer.measure()` was called to notify
the virtualizer. `measure()` wipes the entire `itemSizeCache` (keyed by
stable `messageId`). With no cached sizes, every visible row falls back
to `estimateSize = 96px`. Rows taller than 96px get wrong `translateY`
offsets until their `ResizeObserver` fires — that transient mismatch is
the "overlapping messages that clear on scroll" the user sees.
`@tanstack/solid-virtual` 3.x already calls `notify()` internally when
`setOptions({ count })` changes the count, so `measure()` was redundant
as a notification mechanism. Fix: remove the `needsRemeasure` flag and
the `queueMicrotask(() => virtualizer.measure())` call entirely. The
stable `messageId`-keyed size cache now survives session switches:
revisiting a session renders at correct sizes immediately; first visits
to a new session use `estimateSize` as before (brief, resolves quickly
via ResizeObserver).
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis PR adds a per-session message existence index, clears session message state when a session is removed, and updates transcript virtualizer identity and count handling. It also memoizes the reasoning line count shown in message rows. ChangesMessage state and session cleanup
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install failed. For unrecoverable errors, disable the tool in CodeRabbit configuration. Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #73 +/- ##
=======================================
Coverage 81.07% 81.07%
=======================================
Files 55 55
Lines 7597 7597
=======================================
Hits 6159 6159
Misses 1438 1438
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@web/src/components/transcript/Transcript.tsx`:
- Around line 184-205: The virtualizer in Transcript.tsx is reusing item
measurements across different sessions because the cache key is based on
messageId alone. Update the Transcript virtualizer setup so getItemKey
incorporates the active session identity (for example focusedSessionId() or a
session epoch), or explicitly clear the measurement cache when the session
changes, while keeping the count update in the existing createEffect. This will
ensure setOptions() does not carry stale row sizes between sessions with the
same message count.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 4e818283-59c3-433f-b53d-4083df348095
📒 Files selected for processing (2)
web/src/components/transcript/Transcript.tsxweb/src/state/messages.ts
… destroy Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
web/src/components/transcript/Transcript.tsx (1)
185-207: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winInclude the focused session in the virtualizer update.
countalone won’t invalidate the measurement cache on same-length session switches, so the list can keep stale row positions until another event forces a rebuild.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/src/components/transcript/Transcript.tsx` around lines 185 - 207, The virtualizer update in Transcript should also react to the focused session, not just messages().length, because same-length session switches can leave stale row measurements and positions. Update the createEffect around virtualizer.setOptions so it depends on the focused session state as well as the row count, and ensure the virtualizer is rebuilt/invalidated when the session changes even if count stays the same. Keep using the existing virtualizer and messages logic, but make the session identifier part of the update trigger so cached measurements don’t carry over across sessions.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@web/src/state/sessions.ts`:
- Line 13: The session removal flow currently calls clearSessionMessages before
the existing batch in the sessions state module, which causes the messages
update and the byId removal to commit separately. Move clearSessionMessages(id)
inside the same batch used by the relevant sessions update path in sessions.ts
so both updates are batched together; keep the surrounding logic in the
functions that manage session deletion/removal and preserve the existing
setFocusedSessionAccessor behavior.
---
Outside diff comments:
In `@web/src/components/transcript/Transcript.tsx`:
- Around line 185-207: The virtualizer update in Transcript should also react to
the focused session, not just messages().length, because same-length session
switches can leave stale row measurements and positions. Update the createEffect
around virtualizer.setOptions so it depends on the focused session state as well
as the row count, and ensure the virtualizer is rebuilt/invalidated when the
session changes even if count stays the same. Keep using the existing
virtualizer and messages logic, but make the session identifier part of the
update trigger so cached measurements don’t carry over across sessions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: d4076f2b-96ea-4851-b61f-8652c37fe222
📒 Files selected for processing (4)
web/src/components/transcript/MessageRow.tsxweb/src/components/transcript/Transcript.tsxweb/src/state/messages.tsweb/src/state/sessions.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- web/src/state/messages.ts
…destroy Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
This was referenced Jul 2, 2026
saucam
added a commit
that referenced
this pull request
Jul 2, 2026
… via upsert (#74) * fix: streamed messages pushed into scrollback twice — commit finalize via upsert, not a second push The #50 fix covered only #artificiallyStreamText. The main streaming path still called #persistAndBuffer at BOTH stream start (text_delta creates the message) and finalize (text_done), and the same double-push lived in #flushActiveAssistant (interrupt/turn-boundary flush) and #finalizeActiveThinking (every thinking block). Consequences: - scrollback.replay carried two entries per streamed messageId; clients rendered the message twice and the web virtualizer's messageId-keyed caches collided (the residual cause of the 'intermittent message overlap' that #73 partially fixed) - the memory chunker received the stream-start push with empty content, emitting a prompt-only user_turn episode and then a promptless assistant_turn — every plain turn fragmented into two half-episodes - byte accounting drifted negative (push #1 accounted the empty size, eviction subtracted the grown size twice), permanently disabling the 20MB scrollback cap Fixes: - ScrollbackBuffer now records the accounted size per entry and upserts by messageId: re-pushing a buffered id re-accounts the existing entry in place (keeping its replay position) instead of appending a duplicate. Eviction subtracts exactly what was added — negative drift is structurally impossible. updateMessage is O(1) via the id index (was a front-to-back scan). - Session stream-start sites push to scrollback only; the new #commitStreamed emits the durable transcript row and the chunker event exactly once, at finalize, with final content. #artificiallyStreamText drops its bespoke reset-and-updateMessage dance for the same helper. - #seq now seeds past the persisted transcript tail on resume instead of restarting at 0, making seq usable as a monotonic replay cursor. Tests: session-stream-commit.test.ts pins one-scrollback-entry-per- messageId across all four finalize paths (text_done, thinking_done, batch-reply artificial streaming, turn-boundary flush), buffer upsert + byte-cap accounting under by-reference growth, chunker episode pairing (including a test documenting the pre-fix fragmentation), and seq continuation after resume. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix: account scrollback bytes as UTF-8, add live-session chunker regression test CodeRabbit review follow-ups on #74: - String.length counts UTF-16 code units; use Buffer.byteLength so the 20MB cap holds for non-ASCII payloads - end-to-end test that a real Session + MemoryEngine ingests exactly one combined user+assistant episode per streamed turn (a stray stream-start chunker feed would fail it) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix: StubEmbedder missing close() from the Embedder interface Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
saucam
added a commit
that referenced
this pull request
Jul 2, 2026
…ngest, analytics scoping (#75) * fix: message store cleanups — versions leak on destroy, scrollback dupes, session cache pruner registry - clearSessionMessages deleted bySession/epochBySession but left every per-messageId entry in the versions map behind — they accumulated forever across destroyed sessions. Iterate the session's id set and delete them before dropping the set. - replaceScrollback now dedupes replayed entries by messageId (the daemon can replay duplicates for the same messageId — known daemon bug being fixed separately). Mirrors applyMessage's upsert semantics: first occurrence keeps its position, last occurrence's content wins. - New registerSessionCachePruner(): components holding per-message caches keyed by `${sid}:${msgId}` (the Transcript virtualizer's itemSizeCache) get notified when a session is DESTROYED so its keys can be evicted — deliberately not fired on mere focus switches, so caches still survive revisits (#73). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix: transcript virtualizer never synced on count/session change; detached-row DOM leak setOptions() in the pinned @tanstack/virtual-core 3.14.0 ONLY assigns options — it never notifies. The previous comment claimed solid-virtual 3.x notifies internally on count change; that is false, and the solid adapter's onChange (the only path that updates the store getVirtualItems reads) then only fired from scroll/resize observers. Consequences: (a) stale transcript layout after a session switch whenever scrollTop couldn't move; (b) the per-element ResizeObserver writing the NEW session's row heights under the OLD session's `${sid}:${msgId}` keys — poisoning the size cache and re-introducing the overlap bug #73 fixed; (c) scrollback replay after mount (count 0→N) rendering blank until an unrelated event. Fix, verified against the installed virtual-core source: - On count/session change, call the unconditional notify(false) after setOptions. maybeNotify() is memoized on the visible range indexes and skips when they're identical across sessions; measure() would wipe itemSizeCache (the #73 regression). notify(false) does neither. - On session SWITCH additionally pass a fresh getItemKey function identity: getMeasurementOptions is memoized on [count, …, getItemKey], so with an equal count the stale measurementsCache (old keys) would survive and resizeItem would poison itemSizeCache. The fresh identity forces getMeasurements to rebuild all measurements under the NEW keys straight from itemSizeCache — cache hits for revisited sessions, no cache clearing. The effect runs in the same synchronous batch as the content swap; ResizeObserver callbacks fire at the next frame boundary, so keys are correct before the first measurement lands. - Solid never calls refs with null on unmount, so elementsCache retained every unmounted row's DOM subtree forever (keys unique per message). Row refs now register an onCleanup that schedules a coalesced measureElement(null) sweep on a microtask (after the node detaches), which is virtual-core's documented prune path for disconnected nodes. - On session DESTROY (via the messages-store pruner registry), evict the dead session's `${sid}:` keys from itemSizeCache in place (no Map reference change → no relayout of the live session) and sweep elementsCache. Manual verification (no DOM harness in web tests; vitest runs in node): 1. Two sessions both pinned at bottom, similar message counts: switch between them — transcript content, offsets, and total height update immediately; no rows overlap after several round-trip switches. 2. Attach to a session with existing scrollback from a fresh load — messages render as soon as the replay lands (no blank/fallback until another event). 3. Stream a long turn, scroll a 1000+ message session, switch away and back — revisited rows keep their measured heights (no estimate-size flash), memory profile shows detached row nodes collected after GC. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix: session.list.result double-ingest and per-id session list merge - The ws client forwards every frame to onMessage handlers even after resolving the pending request, so session.list.result was ingested twice per refresh: once by refreshSessions (request path) and once by routeBroadcast. The daemon only ever emits session.list.result as a reply to session.list (session-manager returns it with requestId; no unsolicited push path exists), so the broadcast case is dropped — the request path owns it. - ingestSessionList now merges per-id instead of wholesale-replacing byId: unchanged sessions keep their store object identity, changed fields update in place, fields the daemon stopped sending are dropped, new sessions are added and missing ones deleted. Solid's fine-grained reactivity therefore stops tearing down and re-creating every SessionRow on each periodic refresh. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix: stop O(N)-per-delta pending-approval scans over the whole transcript ApprovalBar's memo and the desktop-notification effect both re-ran on every streaming delta and scanned the ENTIRE message array from index 0 — O(N) per delta on a 5000-message session. Both now share lib/approvals.findPendingApproval, which: - gates on the session status: when idle/error nothing can pend, skip entirely. The gate deliberately accepts ALL active statuses rather than strictly waiting_approval: with parallel pending approvals the daemon flips status to tool_running/thinking as soon as the FIRST approval resolves while a second still pends (session.ts sets status per canUseTool resolution), and a strict gate would hide that second approval bar forever. - scans backward from the tail and stops at the current turn boundary (the last user message) — pending approvals can only live in the current turn since earlier turns' tools were finalized before the next user message. Within the window the OLDEST pending match wins, preserving the previous forward-scan pick. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix: usage analytics tenancy leak on empty scope and SQLite variable-limit overflow - An identity owning zero sessions passed an empty ownedSessionIds array to dailyUsage/lifetimeTotals, which treated [] as "no filter" — so a zero-session identity saw EVERYONE's usage. The store now enforces a strict contract: undefined = unscoped (internal callers only), an array — including empty — is a strict ownership filter returning zeros/no buckets. usage.daily additionally early-returns zeros before querying (belt and suspenders around the tenancy boundary). - `session_id IN (?,?,...)` bound one variable per owned session and blew SQLite's bound-variable limit at ~1000 sessions, permanently breaking the analytics panel. The filter is now a single JSON-array bind param via json_each (the sessions table lives in a different DB file than turn_usage, so a JOIN isn't available). Covered by tests with 2500 ids. - web analytics.ts no longer swallows the request error silently — a permanently blank panel with zero console output made the daemon-side failure undiagnosable. Panel still degrades gracefully. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix: analytics panel day bucketing mixed local time with UTC The daemon buckets usage by sqlite date(created_at/1000,'unixepoch') — UTC. The panel stepped days with local-time setDate()/getDate() and then derived keys via toISOString() (UTC), and computed "today" and weekday labels in local time: for any non-UTC user the bars mismatched the daemon's buckets and could skip/duplicate a day around DST. Extracted padDays/utcDayKey into lib/usage-days (pure, unit-tested, injectable nowMs) doing everything in UTC — fixed 86400000ms stepping is exact since UTC has no DST — and the weekday label/today highlight now use timeZone:"UTC" to match. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix: track session epoch in ApprovalBar memo, guard session merge against proto keys CodeRabbit review follow-ups on #75: - ApprovalBar's pending memo now reads epochOf(focusedSessionId()): tool-state deltas mutate messages in place, so a second parallel approval flipping to waiting_confirmation mid-turn (no status change) would otherwise never recompute the memo. Added the motivating parallel-approvals scenario as a unit test. - ingestSessionList's field-merge skips __proto__/constructor/prototype keys (network-sourced payload; JSON.parse yields __proto__ as an own property) and uses Object.hasOwn for the stale-field sweep. Covered by a pollution regression test. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix: skip reassigning deep-equal object fields in session list merge Object-valued fields (usage, subagents, pinnedFiles) arrive as fresh references on every list refresh; reassigning them when deep-equal would notify their subscribers for nothing. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix: cover object-field identity preservation in session list merge Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix: address CodeRabbit review — approval-bar epoch tracking, prototype-pollution guard, deep-equal session merge - ApprovalBar memo reads epochOf(focusedSessionId()) so in-place tool-state patches recompute the pending scan even when no status change accompanies them (second parallel approval flipping mid-turn); scan is turn-bounded so the per-delta cost stays O(current turn) - ingestSessionList skips __proto__/constructor/prototype keys (JSON.parse yields __proto__ as a plain own property) and uses Object.hasOwn for the stale-field delete loop - object-valued fields (pinnedFiles, subagents, usage) keep store identity when deep-equal across refreshes instead of notifying subscribers on every poll; parallel-approvals + pollution + identity tests added - extract #emptyUsageResponse to deduplicate the zeroed usage payload Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
saucam
added a commit
that referenced
this pull request
Jul 3, 2026
applyMessage and applyDelta scanned the session buffer with findIndex from index 0 on every event. The #73/#75 fix made the EXISTENCE check O(1), but the positional lookup still walked all N store-proxied entries inside produce() — O(N) per streaming delta, O(N²) over a session, so long transcripts got progressively laggier. The per-session existence Set is now a messageId → index Map, kept in sync on insert (buffer is append-only) and rebuilt wholesale by replaceScrollback from the dedupe pass it already runs. Both reducers use it for the positional lookup. 2000 tail deltas on a 5000-message session: 46 µs/delta → 1 µs/delta (and the old cost kept growing with transcript length). Fixes #90 Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Two bugs found on main, both rooted in unnecessary O(N) work during session switches.
O(N) session switch slowness
replaceScrollbackran afor...of messagesloop inside a SolidJSproduce()callback bumping a version counter per replayed message. For a 200-message session, this was 200 fine-grained store mutations per switch — path-tracked by Solid's reactive internals even inside abatch().versionOfis not used in any render path (only tests), so this work had no downstream effect.Fix: Remove the per-message version loop from
replaceScrollback. The session epoch bump is the only invalidation signal consumers need.Additionally,
hasMessage(called on every streaming delta to gateapplyDelta) usedbuf.some(m => m.messageId === messageId)— O(N) per delta. For a 200-message session receiving 20 deltas/second, that's 4000 array comparisons/second.Fix: Add a plain
Map<sessionId, Set<messageId>>outside the Solid store.hasMessageis now O(1). The Map lives outside the store so inserts have no reactive overhead.Intermittent message overlap
On every session switch,
virtualizer.measure()wiped the entireitemSizeCache(keyed by stablemessageId). With no cached sizes, every visible row fell back toestimateSize = 96px. Rows taller than 96px got wrongtranslateYoffsets until theirResizeObserverfired — that transient state is the "overlapping messages that clear on scroll" bug.@tanstack/solid-virtual3.x callsnotify()internally whensetOptions({ count })changes the count, makingmeasure()redundant as a notification trigger.Fix: Remove the
needsRemeasureflag andqueueMicrotask(() => virtualizer.measure())entirely. The stable messageId-keyed size cache now survives session switches: revisiting a session renders at correct sizes immediately; first visits useestimateSizeas before.Test plan
bun test ./web/src/state/messages.test.ts)bun run typecheck)🤖 Generated with Claude Code
Summary by CodeRabbit