perf: prune per-thread client state and evict idle VCS status cache

[RusiruSadathana]

Problem

Long-lived sessions accumulated state that was never released: deleting a thread left entries behind in the right-panel, diff-panel, mini-player, and ui-state stores (the zustand removeThread helpers existed with no callers, and ui-state had no removal at all — these persist to localStorage, so the growth survives restarts), previewStateAtom used Atom.keepAlive so one registry node per thread ever visited lived for the tab's lifetime, a released browser surface left a tombstone in byTabId per preview tab ever opened, and the server's VcsStatusBroadcaster kept its per-cwd status cache for the process lifetime after the last poller subscriber released. Re-ports the still-applicable parts of #4176, which was closed when #2829 deleted its V1 base.

Change

• Web, thread deletion (useThreadActions.ts): both delete paths (live and archived) now prune every per-thread store — preview state, right panel, diff panel, preview mini player, and ui state — and the archived path additionally releases composer draft uploads before clearing the draft, matching the live path's order.
• Web, preview atoms (previewStateStore.ts): previewStateAtom moves from Atom.keepAlive to a 5-minute idle TTL. Threads with live preview sessions stay resident because the observed activePreviewSessionsAtom reads them (so the desktop webview host and automation hosts are unaffected); idle threads are collected and re-seed from EMPTY, repopulated by the next server snapshot via usePreviewSession's preview.list reconcile. Adds removePreviewThread for explicit cleanup on deletion.
• Web, browser surfaces (browserSurfaceStore.ts): release deletes the entry instead of writing a { visible: false, owner: null } tombstone. All readers already treat a missing entry as not-visible/not-present, so this is behavior-preserving, and a stale present() from a released lease can no longer resurrect the entry.
• Server, VCS cache (VcsStatusBroadcaster.ts): when the last poller subscriber releases, the cwd's cached status is dropped inside the same modifyEffect critical section that removes the poller, so a concurrent retainRemotePoller cannot have its freshly seeded entry wiped. A later subscriber re-loads and re-seeds the cache (its poller starts with an immediate refresh).

The orchestration read-model half of #4176 is intentionally not ported: orchestration V2 loads per-thread state from indexed SQLite inside each command's dispatch (per-thread keyed lock, one commit transaction) and holds no process-wide in-memory read model, so the O(N) scans and unbounded in-memory growth that change removed no longer exist.

No visible UI changes.

Scope and approval

Triaged bug issue: #4178. Maintainer approval of direction and scope: juliusmarminge on #4176 — "Once #2829 merges, please rebase onto main, port the change to the V2 equivalent, and reopen (or open a fresh PR). Ping me and I'll prioritise the review. This is not a judgement on the change itself. Several of these are real gaps we still want fixed." These changes are the port of the parts whose underlying code still exists; the web and server changes are one fix because they are the client and server halves of the same unbounded-retention problem from that issue.

Verification

• apps/server/src/vcs/VcsStatusBroadcaster.test.ts — extended the existing last-subscriber-disconnects test: after the final poller releases, a new subscription reloads local status (localStatusCalls 1 → 2). Verified against unmodified main that this assertion fails (cache retained, counter stays 1) and passes with the change.
• apps/web/src/previewStateStore.test.ts — new test: removePreviewThread resets a deleted thread's state to EMPTY, leaves other threads intact, and a later event for the removed thread still lands.
• apps/web/src/uiStateStore.test.ts — new test: removeThreadUiState drops both per-thread records and is a reference-preserving no-op for unknown threads.
• apps/web/src/browser/browserSurfaceStore.test.ts — release now asserts the entry is gone (and a stale present() cannot resurrect it); the fitted-presentation release test updated to match.
• Focused runs: 159/159 passing across the six touched test files; tsc --noEmit clean for @t3tools/web and t3; targeted vp lint clean (one pre-existing warning on main, untouched lines).
• Not checked: no integrated pass in a running client — there are no visible UI changes to capture, and store semantics are covered by the unit tests above.

Closes #4178.

---

Done with GLM-5.3 via the Claude Code harness in T3 Code.

🤖 Generated with Claude Code

[coderabbitai]

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

🧰 Additional context used

📚 Code guidelines (1)

docs/internals/effect-services.md — configured

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

• Configuration used: Repository: pingdotgg/t3code/.coderabbit.yaml
• Review profile: CHILL
• Plan: Advanced
• Run ID: 8dd7f264-f7b6-4970-a393-3b59d03c14f1

📥 Commits

Reviewing files that changed from the base of the PR and between 71dbaf1 and 0e1ffe4.

📒 Files selected for processing (9)

• apps/server/src/vcs/VcsStatusBroadcaster.test.ts
• apps/server/src/vcs/VcsStatusBroadcaster.ts
• apps/web/src/browser/browserSurfaceStore.test.ts
• apps/web/src/browser/browserSurfaceStore.ts
• apps/web/src/hooks/useThreadActions.ts
• apps/web/src/previewStateStore.test.ts
• apps/web/src/previewStateStore.ts
• apps/web/src/uiStateStore.test.ts
• apps/web/src/uiStateStore.ts

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 9 remain after this review.

---

📝 Walkthrough

Walkthrough

The changes remove cached VCS status when the final remote poller releases, clear per-thread client state after successful thread deletion, and remove browser surface entries when their owner releases them.

Changes

VCS status cache

Layer / File(s)	Summary
Evict status when the final poller releases apps/server/src/vcs/VcsStatusBroadcaster.ts, apps/server/src/vcs/VcsStatusBroadcaster.test.ts	The final subscriber release removes the poller and its cached CWD status. The test checks that a later subscription reads local status again.

Per-thread client state

Layer / File(s)	Summary
Remove preview and UI state apps/web/src/previewStateStore.ts, apps/web/src/previewStateStore.test.ts, apps/web/src/uiStateStore.ts, apps/web/src/uiStateStore.test.ts	Preview state now uses a five-minute idle TTL and has a removal function that clears its active-session indexes. UI state removal clears a thread’s visit timestamp and changed-files expansion state. Tests cover removal and preservation of other threads’ state.
Clear client state after thread deletion apps/web/src/hooks/useThreadActions.ts	Both successful deletion paths clear per-thread preview, panel, mini-player, and UI state.

Browser surface release

Layer / File(s)	Summary
Delete the surface entry on release apps/web/src/browser/browserSurfaceStore.ts, apps/web/src/browser/browserSurfaceStore.test.ts	An owner’s release removes the tab entry. Tests check that the entry stays absent after a stale presentation and after fitted-surface release.

Priority: ⬆️ High

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

Change: Bug fix · Severity of issue fixed: High

Suggested reviewers: maria-rcks, juliusmarminge

Merge Risk: ⚪ Minimal · up to 0e1ff

The change prunes per-thread client state and evicts idle VCS status cache, and no concrete merge-blocking risk was identified. The author did not run an integrated check in a live client, but that is normal residual uncertainty.

Security Architecture Review

Security architecture risk: 🔵 Low · up to 0e1ff

The cleanup generally reduces retained state and preserves thread isolation. One lifecycle question remains: discarded preview state also contains safeguards against stale updates, which could allow a previously closed preview to reappear. No new unauthorized access or privilege gain was established.

Retained concerns

• Low · architecture · inferred: The new idle eviction discards ordering and suppression guards together with disposable preview data. A subsequent stale result can reseed sessions that desktop browser hosts consume; whether authoritative refresh always prevents reopening a previously closed browser remains unresolved. This is a bounded lifecycle-control concern, not a verified authorization bypass.

Security review details

Security Blast Radius

• inferred — The identified lifecycle concern is bounded to preview sessions already represented in a connected desktop client. Rehydrated snapshots supply the browser URL and profile selection, but no cross-environment reach or new attacker authority was demonstrated.

Security Findings and Attack Paths

• inferred — The supported conditional path is reset, acceptance of stale preview data, active-session indexing and browser-host mounting. Evidence does not establish attacker-controlled stale delivery, unauthorized automation execution or increased exposure relative to deletion's previous retained-session behavior.

Trust Boundaries and Controls

• observed — Normal server close removes sessions and advances revision within a synchronized transition; list reads return authoritative sessions and revision. Client event handling filters thread identity, and automation requests use an environment-bound subscription configured for immediate idle disposal.

Hardening Proposals

• proposed — If authoritative reseeding cannot exclude stale results, separate lightweight lifecycle guards from evictable presentation state or require a fresh authoritative list before recreating a browser host.

🚥 Pre-merge checks | ✅ 3 | ❌ 1 | ❓ 1

❌ Failed checks (1 warning, 1 inconclusive)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 44.44% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 9 functions across 9 files.	Write docstrings for the functions missing them to satisfy the coverage threshold.
Linked Issues check	❓ Inconclusive	For [#4178], the diff wires cleanup into live and archived thread deletion, adds idle TTL and explicit preview cleanup, removes released browser-surface entries, and evicts VCS status when the last po…	Provide source-level evidence or a focused test showing how V2 loads thread state and that command processing does not use a process-wide read model whose cost grows with threads created.

✅ Passed checks (3 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly summarizes the main changes: pruning per-thread client state and evicting idle VCS status cache.
Description check	✅ Passed	The description covers the problem, changes, scope and approval, and focused verification results. It also explains why the orchestration read-model change is out of scope.
Out of Scope Changes check	✅ Passed	The changes address [#4178]'s retained client and VCS state. The browser-surface tombstone cleanup and archived draft-upload cleanup support the same thread and preview lifecycle cleanup. No unrelated…

Full details: Linked Issues check

Explanation

For [#4178], the diff wires cleanup into live and archived thread deletion, adds idle TTL and explicit preview cleanup, removes released browser-surface entries, and evicts VCS status when the last poller releases. The diff also adds focused tests for these behaviors. The PR says the orchestration V1 read-model problem no longer exists in V2, but the inspected evidence does not establish that V2 has no equivalent process-wide read model or lifetime-thread-count scaling.

• Fix all pre-merge checks with AI

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create a new PR

• Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

---

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