Skip to content

fix: web transcript virtualizer sync, DOM leak, session-list double-ingest, analytics scoping - #75

Merged
saucam merged 10 commits into
mainfrom
fix/web-virtualizer-sync
Jul 2, 2026
Merged

fix: web transcript virtualizer sync, DOM leak, session-list double-ingest, analytics scoping#75
saucam merged 10 commits into
mainfrom
fix/web-virtualizer-sync

Conversation

@saucam

@saucam saucam commented Jul 2, 2026

Copy link
Copy Markdown
Collaborator

Fixes (from the session/rendering audit)

Transcript virtualizer never synced on count/session change (P1). setOptions() in the pinned virtual-core 3.14.0 only assigns options — it never notifies (the previous comment's claim that solid-virtual notifies internally was false). Stale layouts after session switches whenever scrollTop couldn't move, and the per-row ResizeObserver poisoned itemSizeCache by writing the new session's heights under the old session's keys — re-introducing the overlap bug #73 fixed. Now: unconditional notify(false) after count/session changes (cache-preserving; maybeNotify is range-memoized and skips the switch case, measure() wipes the size cache), plus a fresh getItemKey identity on session switch so measurementsCache rebuilds under the new keys before the first resize lands. Mechanism verified line-by-line against the installed virtual-core source and documented in place.

Detached-DOM leak (P1). Solid never calls refs with null on unmount, so every scrolled-past row's DOM subtree stayed referenced in elementsCache forever. Row refs now register a coalesced microtask measureElement(null) sweep (virtual-core's prune path for disconnected nodes — verified). Session destroy additionally evicts the dead session's ${sid}: keys from itemSizeCache in place via a new pruner registry in the messages store.

Message store cleanups (P2). clearSessionMessages now clears per-message versions entries; replaceScrollback defensively dedupes by messageId (daemon-side duplicate-replay fix is #74).

session.list double-ingest (P2). The result was ingested by both the request path and routeBroadcast (daemon never pushes it unsolicited — verified), and each ingest wholesale-replaced byId, tearing down every sidebar row. Now single-ingest + per-id merge preserving object identity for unchanged sessions.

O(N)-per-delta approval scans (P2). Notification watcher and ApprovalBar re-scanned the whole transcript on every streaming delta. Shared findPendingApproval helper: active-status gate + backward scan bounded at the current turn boundary. (Deliberately NOT gated strictly on waiting_approval: with parallel approvals the status flips to tool_running when the first resolves while another still pends.)

Analytics scoping (P2, daemon). Empty owned-session list meant "no filter" — a zero-session identity saw everyone's usage. Now empty-array = strict filter returning zeros. The unbounded IN (?,...) bind list (SQLite variable-limit blowup at ~1000 sessions → permanently blank panel via a swallowed catch) is replaced with a single json_each bind param; the client now logs fetch errors.

UTC day bucketing (P3). Panel mixed local-time bucketing with the daemon's UTC buckets; extracted an all-UTC usage-days helper.

Tests

  • Daemon: analytics scoping (empty-scope zeros, cross-identity exclusion, 2500-id no-throw) — 661 pass
  • Web: scrollback dedupe, versions/ids cleanup, pruner registry, per-id session merge (object identity), approval scan gate/boundary/ordering, UTC bucketing incl. DST window — 139 pass
  • tsc clean, biome clean, vite build green. Virtualizer DOM behavior isn't reachable from the node-only harness; manual verification steps documented in the commit message.

🤖 Generated with Claude Code

Summary by CodeRabbit

  • New Features
    • Added UTC-safe analytics day bucketing and consistent chart labeling.
    • Improved pending-approval detection with a shared helper powering desktop notifications.
  • Bug Fixes
    • Empty session filters now strictly return zero/empty usage instead of unscoped totals.
    • Usage analytics returns empty results (and stable totals) when there are no owned sessions.
    • Fixed transcript virtualizer caching/measurement when switching sessions.
    • Improved session syncing to avoid stale/missing fields and ignore unsafe payload keys.
  • Other
    • Added broader test coverage for analytics, approvals, usage-days, and session/message stores.

saucam and others added 6 commits July 3, 2026 00:09
…pes, 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>
…ached-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>
- 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>
…ript

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>
…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>
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>
@coderabbitai

coderabbitai Bot commented Jul 2, 2026

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

This PR tightens usage aggregation scoping for empty session-id lists, adds UTC-safe day bucketing for analytics, centralizes pending-approval lookup, changes session-list ingestion to in-place per-id merging, and adds scrollback deduplication plus session-scoped transcript cache pruning.

Changes

Usage scoping and analytics

Layer / File(s) Summary
Store scoping and JSON-array filtering
src/daemon/memory/store.ts, src/tests/memory.test.ts
dailyUsage/lifetimeTotals return empty or zeroed results for an explicit empty sessionIds array, and SQL filtering switches from dynamic IN (?,...) placeholders to a single JSON-array bind via json_each; tests updated to verify strict scoping and oversized-id handling.
Session-manager short-circuit
src/daemon/session-manager.ts
#usageDaily returns zeroed usage immediately when the caller owns no sessions instead of querying the store.
UTC day-bucketing and panel adoption
web/src/lib/usage-days.ts, web/src/lib/usage-days.test.ts, web/src/components/AnalyticsPanel.tsx, web/src/state/analytics.ts
New utcDayKey/padDays helpers replace local date logic in AnalyticsPanel for UTC-consistent bucketing and weekday labeling; analytics fetch now logs errors instead of swallowing them.

Estimated code review effort: 3 (Moderate) | ~25 minutes

Shared pending-approval lookup

Layer / File(s) Summary
findPendingApproval helper and tests
web/src/lib/approvals.ts, web/src/lib/approvals.test.ts
New status-gated, turn-scoped backward scan finds the oldest pending tool call awaiting confirmation within the current turn.
ApprovalBar and desktop-notifications adoption
web/src/components/transcript/ApprovalBar.tsx, web/src/state/desktop-notifications.ts
Both components replace inline scanning loops with calls to findPendingApproval, incorporating session status into reactive dependencies.

Estimated code review effort: 2 (Simple) | ~12 minutes

Session-list merge and message store cache pruning

Layer / File(s) Summary
Incremental per-id session store merge
web/src/state/sessions.ts, web/src/state/sessions.test.ts, web/src/state/connection.ts
ingestSessionList updates fields in place, removes stale fields/sessions, and preserves object identity; routeBroadcast no longer re-ingests session.list.result to prevent double-application.
Scrollback dedup and pluggable session cache pruners
web/src/state/messages.ts, web/src/state/messages.test.ts
replaceScrollback dedupes by messageId; registerSessionCachePruner lets consumers register cleanup callbacks invoked by clearSessionMessages, which also clears stale version entries.
Transcript virtualizer session-scoped keys and cache pruning
web/src/components/transcript/Transcript.tsx
Adds session-namespaced itemKeyFor, a session/count-driven effect reconfiguring the virtualizer, a session-destruction cache pruner, and microtask-coalesced row-unmount pruning.

Estimated code review effort: 4 (Complex) | ~45 minutes

Sequence Diagram(s)

sequenceDiagram
  participant ApprovalBar
  participant DesktopNotifications
  participant findPendingApproval
  ApprovalBar->>findPendingApproval: focusedSessionMessages(), focusedSession().status
  findPendingApproval-->>ApprovalBar: pending message or null
  DesktopNotifications->>findPendingApproval: focusedSessionMessages(), session.status
  findPendingApproval-->>DesktopNotifications: pending message or null
Loading
sequenceDiagram
  participant Transcript
  participant Virtualizer
  participant MessagesStore
  Transcript->>MessagesStore: registerSessionCachePruner(pruneFn)
  MessagesStore-->>Transcript: unregister()
  Transcript->>Virtualizer: getItemKey via itemKeyFor(index)
  MessagesStore->>Transcript: clearSessionMessages(sessionId)
  Transcript->>Virtualizer: delete itemSizeCache entries prefixed sessionId, measureElement(null)
Loading

Possibly related PRs

  • saucam/codeoid#48: Introduced the same SqliteEpisodeStore.dailyUsage/lifetimeTotals methods and AnalyticsPanel that this PR directly refines.
  • saucam/codeoid#50: Addressed duplicate scrollback rendering by messageId, related to the replaceScrollback dedup logic added here.
  • saucam/codeoid#73: Prior fixes to session-switch overlap/performance in Transcript.tsx and messages.ts overlap with the virtualizer cache pruning and dedup changes here.
🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately reflects the main fixes: transcript syncing, DOM leak cleanup, session-list double ingest, and analytics scoping.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/web-virtualizer-sync

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

If the error stems from missing dependencies, add them to the package.json file. For unrecoverable errors (e.g., due to private dependencies), disable the tool in the CodeRabbit configuration.

ESLint install failed. For unrecoverable errors, disable the tool in CodeRabbit configuration.


Comment @coderabbitai help to get the list of available commands.

@codecov

codecov Bot commented Jul 2, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 80.74%. Comparing base (9233d9a) to head (4f805cf).
✅ All tests successful. No failed tests found.

Additional details and impacted files
@@            Coverage Diff             @@
##             main      #75      +/-   ##
==========================================
+ Coverage   80.72%   80.74%   +0.02%     
==========================================
  Files          56       56              
  Lines        7895     7905      +10     
==========================================
+ Hits         6373     6383      +10     
  Misses       1522     1522              
Flag Coverage Δ
daemon 80.74% <100.00%> (+0.02%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
src/daemon/memory/store.ts 87.71% <100.00%> (+0.30%) ⬆️
🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Actionable comments posted: 2

🧹 Nitpick comments (4)
web/src/lib/approvals.test.ts (1)

46-52: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Consider adding a test for the parallel-pending-approvals scenario.

The doc comment in approvals.ts (lines 10-17) specifically motivates the "any active status" gate with a scenario where two approvals are pending and the daemon flips status once the first resolves. That scenario (two waiting_confirmation tool calls in the same turn, second one returned while status is tool_running/thinking) isn't directly exercised by a test.

🤖 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/lib/approvals.test.ts` around lines 46 - 52, Add a test in
approvals.test.ts that covers the parallel-pending-approvals case described by
approvals.ts: create two pending approvals from the same turn (e.g. two
waiting_confirmation tool calls) and assert that findPendingApproval still
returns the later pending message when the session status is one of the active
states like tool_running or thinking. Use the existing findPendingApproval,
waiting, and SessionStatus symbols so the new test exercises the “any active
status” behavior and the daemon flip scenario.
src/daemon/session-manager.ts (1)

1306-1356: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Extract the duplicated zero-response literal.

The no-memory branch (Lines 1310-1325) and the no-owned-sessions branch (Lines 1334-1348) build byte-identical zeroed daily/lifetime payloads. Consider a small private helper (e.g. #emptyUsageResponse(requestId)) to avoid the two literals drifting if the shape changes later.

♻️ Proposed refactor
+  `#emptyUsageResponse`(requestId: string): DaemonMessage {
+    return {
+      type: "response.ok",
+      requestId,
+      data: {
+        daily: [] as DailyUsageBucket[],
+        lifetime: {
+          costUsd: 0,
+          inputTokens: 0,
+          outputTokens: 0,
+          numTurns: 0,
+          numSessions: 0,
+        } as LifetimeUsageTotals,
+      },
+    };
+  }
+
   `#usageDaily`(
     msg: Extract<ClientMessage, { type: "usage.daily" }>,
     auth: AuthContext,
   ): DaemonMessage {
     if (!this.#memory) {
-      return {
-        type: "response.ok",
-        requestId: msg.id,
-        data: {
-          daily: [] as DailyUsageBucket[],
-          lifetime: {
-            costUsd: 0,
-            inputTokens: 0,
-            outputTokens: 0,
-            numTurns: 0,
-            numSessions: 0,
-          } as LifetimeUsageTotals,
-        },
-      };
+      return this.#emptyUsageResponse(msg.id);
     }
     const days = typeof msg.days === "number" && msg.days > 0 ? Math.min(msg.days, 365) : 30;
     const ownedSessionIds = this.#store
       .listSessions(auth.accountId, auth.projectId)
       .map((s) => s.id);
     if (ownedSessionIds.length === 0) {
-      return {
-        type: "response.ok",
-        requestId: msg.id,
-        data: {
-          daily: [] as DailyUsageBucket[],
-          lifetime: {
-            costUsd: 0,
-            inputTokens: 0,
-            outputTokens: 0,
-            numTurns: 0,
-            numSessions: 0,
-          } as LifetimeUsageTotals,
-        },
-      };
+      return this.#emptyUsageResponse(msg.id);
     }
🤖 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 `@src/daemon/session-manager.ts` around lines 1306 - 1356, The zeroed response
payload is duplicated in both the no-memory and no-owned-sessions branches of
`#usageDaily`, so extract that byte-identical `daily`/`lifetime` object into a
small private helper such as `#emptyUsageResponse(requestId)`. Update both
branches to call the helper and return the same `response.ok` shape from
`#usageDaily`, keeping the response structure centralized so future changes only
need to be made once.
web/src/state/sessions.ts (1)

64-91: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win

Object-valued fields will always look "changed" on every poll.

The !== comparison at Line 80 is a reference check, so fields like rotation, usage, subagents, and pinnedFiles will be treated as "changed" on every ingest even when their content is identical, since the daemon sends a fresh object/array each time. That's a smaller-scoped win than a full identity swap, but it partially works against the stated goal ("only fields that actually changed trigger updates") for consumers that read those specific nested fields on a busy polling cadence.

Worth a follow-up (not blocking) to add a cheap structural-equality check for known object/array-valued fields, or leave as-is if downstream consumers of those fields are rare/cheap to re-render.

🤖 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/state/sessions.ts` around lines 64 - 91, The change in
ingestSessionList only uses reference equality when updating fields, so nested
object/array fields like rotation, usage, subagents, and pinnedFiles will appear
changed on every poll even when their contents are unchanged. Update the
comparison logic in ingestSessionList to treat known object/array-valued fields
with a cheap structural-equality check before writing into s.byId, while keeping
the existing in-place update flow for scalar fields and the cleanup of removed
keys.
web/src/components/transcript/Transcript.tsx (1)

194-274: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win

Undocumented internal fields (itemSizeCache, notify) are used via type-erasing casts.

The reasoning documented here checks out: getMeasurements(): Dependencies are count, paddingStart, scrollMargin, getItemKey, enabled, itemSizeCache, which matches the claim about getItemKey identity forcing a measurement rebuild without wiping itemSizeCache. The public measureElement(null) sweep is also real — Without this option, the ResizeObserver fires with size 0 for all items when hidden, resetting all measurements is a related but separate cache-invalidation path; the actual published source confirms measureElement(null) iterates elementsCache and evicts disconnected nodes, as reflected in the comments here.

That said, itemSizeCache and notify aren't part of the documented public API surface — hence the as unknown as {...} casts bypassing TypeScript's own types. A future patch/minor release of @tanstack/virtual-core (note also that the library recently reworked its internal size-cache implementation for perf, per the maintainers' own changelog) could rename/restructure these fields silently, with no compile-time signal here. Consider pinning an exact @tanstack/solid-virtual version (not a caret/range) and/or adding a small smoke test asserting these internal shapes still hold, so an upgrade fails loudly instead of silently poisoning the size cache.

🤖 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 194 - 274, The
transcript virtualizer logic relies on internal, type-erased access to
non-public fields (`itemSizeCache`) and methods (`notify`) in `Transcript.tsx`,
so a future `@tanstack/virtual-core` or `@tanstack/solid-virtual` update could
break it silently. Keep the existing `createEffect` and `onMount` behavior, but
make the dependency on these internals explicit by pinning the virtualizer
package version and/or adding a smoke test around the `virtualizer` shape used
by the `notify(false)` call and the `itemSizeCache` pruning path, so upgrades
fail loudly if those symbols change.
🤖 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/ApprovalBar.tsx`:
- Around line 75-80: The approval bar memo only depends on
focusedSessionMessages() and the session status, so in-place delta patches can
be missed when a tool transitions to waiting_confirmation. Update the memo in
ApprovalBar to also read the focused session’s epoch via epochOf(...) alongside
focusedSessionMessages() and focusedSession()?.status, ensuring createMemo
re-fires when the focused session epoch changes and the pending approval state
is recomputed.

In `@web/src/state/sessions.ts`:
- Around line 76-86: The field-merge loop in the session update logic can copy
dangerous prototype-polluting keys from the daemon payload into the store draft.
In the merge code inside the session update path, add a guard in the
Object.keys(source) and Object.keys(target) loops to skip keys like "__proto__",
"constructor", and "prototype" before reading, writing, or deleting them. Keep
the existing in-place update behavior, but ensure the merge only touches safe
own data fields on existing session records.

---

Nitpick comments:
In `@src/daemon/session-manager.ts`:
- Around line 1306-1356: The zeroed response payload is duplicated in both the
no-memory and no-owned-sessions branches of `#usageDaily`, so extract that
byte-identical `daily`/`lifetime` object into a small private helper such as
`#emptyUsageResponse(requestId)`. Update both branches to call the helper and
return the same `response.ok` shape from `#usageDaily`, keeping the response
structure centralized so future changes only need to be made once.

In `@web/src/components/transcript/Transcript.tsx`:
- Around line 194-274: The transcript virtualizer logic relies on internal,
type-erased access to non-public fields (`itemSizeCache`) and methods (`notify`)
in `Transcript.tsx`, so a future `@tanstack/virtual-core` or
`@tanstack/solid-virtual` update could break it silently. Keep the existing
`createEffect` and `onMount` behavior, but make the dependency on these
internals explicit by pinning the virtualizer package version and/or adding a
smoke test around the `virtualizer` shape used by the `notify(false)` call and
the `itemSizeCache` pruning path, so upgrades fail loudly if those symbols
change.

In `@web/src/lib/approvals.test.ts`:
- Around line 46-52: Add a test in approvals.test.ts that covers the
parallel-pending-approvals case described by approvals.ts: create two pending
approvals from the same turn (e.g. two waiting_confirmation tool calls) and
assert that findPendingApproval still returns the later pending message when the
session status is one of the active states like tool_running or thinking. Use
the existing findPendingApproval, waiting, and SessionStatus symbols so the new
test exercises the “any active status” behavior and the daemon flip scenario.

In `@web/src/state/sessions.ts`:
- Around line 64-91: The change in ingestSessionList only uses reference
equality when updating fields, so nested object/array fields like rotation,
usage, subagents, and pinnedFiles will appear changed on every poll even when
their contents are unchanged. Update the comparison logic in ingestSessionList
to treat known object/array-valued fields with a cheap structural-equality check
before writing into s.byId, while keeping the existing in-place update flow for
scalar fields and the cleanup of removed keys.
🪄 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: ab134687-4a72-47f2-a94d-bb24c7a7cb9f

📥 Commits

Reviewing files that changed from the base of the PR and between 9233d9a and 6cec406.

📒 Files selected for processing (17)
  • src/daemon/memory/store.ts
  • src/daemon/session-manager.ts
  • src/tests/memory.test.ts
  • web/src/components/AnalyticsPanel.tsx
  • web/src/components/transcript/ApprovalBar.tsx
  • web/src/components/transcript/Transcript.tsx
  • web/src/lib/approvals.test.ts
  • web/src/lib/approvals.ts
  • web/src/lib/usage-days.test.ts
  • web/src/lib/usage-days.ts
  • web/src/state/analytics.ts
  • web/src/state/connection.ts
  • web/src/state/desktop-notifications.ts
  • web/src/state/messages.test.ts
  • web/src/state/messages.ts
  • web/src/state/sessions.test.ts
  • web/src/state/sessions.ts

Comment thread web/src/components/transcript/ApprovalBar.tsx Outdated
Comment thread web/src/state/sessions.ts Outdated
saucam and others added 2 commits July 3, 2026 00:31
…inst 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>
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>
@saucam

saucam commented Jul 2, 2026

Copy link
Copy Markdown
Collaborator Author

Addressed the review in 34d1847 + 63e3592:

  • ApprovalBar epoch tracking (Major): applied — the pending memo now reads epochOf(focusedSessionId()), so a second parallel approval flipping to waiting_confirmation mid-turn (in-place mutation, no status change) recomputes it. Added the motivating parallel-approvals scenario as a unit test (the nitpick's ask).
  • Prototype-pollution guard (Minor): applied — the field-merge loop skips __proto__/constructor/prototype and the stale-field sweep uses Object.hasOwn; covered by a JSON.parse-sourced pollution regression test.
  • Also hardened the merge to skip reassigning deep-equal object-valued fields (usage/subagents), so their subscribers aren't notified on every refresh.

141 web tests green, tsc/biome clean.

saucam and others added 2 commits July 3, 2026 00:33
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…pe-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>
@saucam

saucam commented Jul 2, 2026

Copy link
Copy Markdown
Collaborator Author

Review addressed through 4f805cf:

  • ApprovalBar epoch (Major): applied — the memo now reads epochOf(focusedSessionId()). Note the scan's store-proxy path reads were already fine-grained reactive for the flagged case, but the approval bar is too critical to leave riding on proxy-path subtleties a future refactor could silently break; the epoch read makes recomputation unconditional and the turn-bounded scan keeps it cheap.
  • __proto__ guard (Minor): applied, plus Object.hasOwn in the stale-field delete loop (stronger than k in source — inherited keys can't mask a delete), with a JSON.parse-sourced pollution regression test.
  • Parallel-approvals test (nitpick): added — first approval resolves in place, status flips to tool_running, second approval still surfaces.
  • Zero-response helper (nitpick): extracted #emptyUsageResponse.
  • Object-valued fields always look changed (nitpick): applied — deep-equal skip via jsonEqual so pinnedFiles/subagents/usage keep store identity across refreshes, with an identity-preservation test.

Suites: 661 daemon + 142 web, tsc/biome clean.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🧹 Nitpick comments (1)
web/src/state/sessions.test.ts (1)

140-151: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Consider covering constructor/prototype keys too.

The implementation's skip-list in ingestSessionList (web/src/state/sessions.ts) also special-cases "constructor" and "prototype", but only __proto__ is tested here. Extending this test (or adding a parallel case) would guard against regressions if the skip-list is ever refactored.

🤖 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/state/sessions.test.ts` around lines 140 - 151, The
prototype-pollution test for ingestSessionList only covers "__proto__", but the
skip-list in ingestSessionList also blocks "constructor" and "prototype". Extend
the existing test in sessions.test.ts, or add a sibling case, to feed network
payloads containing those keys and assert they are ignored while legitimate
fields still merge through ingestSessionList and getSession.
🤖 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.

Nitpick comments:
In `@web/src/state/sessions.test.ts`:
- Around line 140-151: The prototype-pollution test for ingestSessionList only
covers "__proto__", but the skip-list in ingestSessionList also blocks
"constructor" and "prototype". Extend the existing test in sessions.test.ts, or
add a sibling case, to feed network payloads containing those keys and assert
they are ignored while legitimate fields still merge through ingestSessionList
and getSession.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 8a9130c2-7bb7-4d4d-820a-eda322cb5804

📥 Commits

Reviewing files that changed from the base of the PR and between 6cec406 and 4f805cf.

📒 Files selected for processing (5)
  • src/daemon/session-manager.ts
  • web/src/components/transcript/ApprovalBar.tsx
  • web/src/lib/approvals.test.ts
  • web/src/state/sessions.test.ts
  • web/src/state/sessions.ts
🚧 Files skipped from review as they are similar to previous changes (4)
  • web/src/lib/approvals.test.ts
  • web/src/state/sessions.ts
  • src/daemon/session-manager.ts
  • web/src/components/transcript/ApprovalBar.tsx

@saucam
saucam merged commit 6e56b9b into main Jul 2, 2026
4 of 5 checks passed
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant