perf(renderer): bail out of identity-equal terminal layout and cache-timer writes - #12420
Conversation
…timer writes setCacheTimerStartedAt and setTabLayout spread a fresh object and returned it unconditionally, so every redundant call published a new AppState and ran every zustand subscriber's selector across all mounted panes. Both have a real redundant cadence: parked-terminal-byte-watcher writes a null cache timer on each agent working/exit/stale-title transition, and TerminalPane re-persists an identical layout on pane-title churn. Extract the existing terminalLayoutEqual comparator out of web-session-tabs-sync into a shared module and use it to gate the layout write, and dedupe the remote-runtime layout IPC against the last snapshot pushed per tab.
📝 WalkthroughWalkthroughThe change centralizes terminal layout equality in a shared utility. Store updates avoid redundant writes, remove stale timer sentinels, normalize PTY ownership, and handle layout deletion. Remote pane layout updates use a resettable pusher that deduplicates layouts per worktree and tab, sends changed geometry and titles, retries failed updates, and re-pushes after remounts. Tests cover equality, identity bailouts, cache behavior, PTY swaps, isolation, failures, remounts, and reconnects. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 7ccc1cb0-255d-4561-8fbe-4aa2a36f5a59
📒 Files selected for processing (7)
src/renderer/src/components/terminal-pane/TerminalPane.tsxsrc/renderer/src/components/terminal-pane/remote-pane-layout-push.test.tssrc/renderer/src/components/terminal-pane/remote-pane-layout-push.tssrc/renderer/src/lib/terminal-layout-equality.tssrc/renderer/src/runtime/web-session-tabs-sync.tssrc/renderer/src/store/slices/terminal-write-identity-bailouts.test.tssrc/renderer/src/store/slices/terminals.ts
| if (lastPushed?.tabId === tabId && terminalLayoutEqual(lastPushed.snapshot, layout)) { | ||
| return |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | ⚡ Quick win
Compare the remote delivery state instead of the full persisted snapshot.
Line 19 compares activeLeafId, buffersByLeafId, and scrollbackRefsByLeafId. The runtime RPC does not receive these fields. A change only in one of these fields sends an identical remote layout update.
Keep PTY mappings in the comparison because a PTY swap changes the host session. Compare only PTY mappings plus root, expandedLeafId, and titlesByLeafId. Add tests for active-leaf and scrollback-only changes.
| return ( | ||
| b.type === 'split' && | ||
| a.direction === b.direction && | ||
| a.ratio === b.ratio && | ||
| terminalLayoutNodeEqual(a.first, b.first) && | ||
| terminalLayoutNodeEqual(a.second, b.second) |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟡 Minor | ⚡ Quick win
Normalize the default split ratio before comparison.
Line 35 treats undefined and 0.5 as different values. The shared type defines an absent ratio as 0.5. A host snapshot that omits the default ratio will bypass store and IPC identity bailouts when the renderer writes 0.5.
Proposed fix
- a.ratio === b.ratio &&
+ (a.ratio ?? 0.5) === (b.ratio ?? 0.5) &&Add coverage for an omitted ratio versus 0.5.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| return ( | |
| b.type === 'split' && | |
| a.direction === b.direction && | |
| a.ratio === b.ratio && | |
| terminalLayoutNodeEqual(a.first, b.first) && | |
| terminalLayoutNodeEqual(a.second, b.second) | |
| return ( | |
| b.type === 'split' && | |
| a.direction === b.direction && | |
| (a.ratio ?? 0.5) === (b.ratio ?? 0.5) && | |
| terminalLayoutNodeEqual(a.first, b.first) && | |
| terminalLayoutNodeEqual(a.second, b.second) |
Until-clean review and Electron QAThe same-model review returned Live Electron validation on the PR worktree:
Verification on pushed |
|
Added paired Electron reconnect coverage in 10664b2. The test now proves:
Validation:
Existing Electron screenshot evidence remains attached here: #12420 (comment) |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
tests/e2e/paired-remote-pane-layout-retry.spec.ts (2)
202-209: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winMake the cleanup sequence fault tolerant.
observer?.dispose()andclient?.dispose()are awaited without error handling. If either rejects, the remaining cleanup steps do not run. The test then leaks an Electron app, a daemon, and a temp user-data directory, which can destabilize later tests in the same run.♻️ Proposed change
} finally { - await observer?.dispose() + await observer?.dispose().catch(() => undefined) if (terminal) { await host.client.call('terminal.closeTab', { terminal }).catch(() => undefined) } - await client?.dispose() + await client?.dispose().catch(() => undefined) await host.dispose() }
185-186: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe local assertion after the retry is weak; consider asserting the push explicitly.
setPaneTitlesets the sametitleagain. TheEdit pane title: ${title}button from Line 105 is already visible from the first, failed attempt, so that step cannot fail. Line 186 then only proves the local layout is unchanged. The host poll on Lines 187-195 carries the whole proof of the retry. Add a console-event wait for a successful push, or assert a remote IPC call count, so a regression that stops re-arming the pusher fails on the client side too.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: ee8517e5-72f7-4414-8856-3052057383d7
📒 Files selected for processing (1)
tests/e2e/paired-remote-pane-layout-retry.spec.ts
1 participant
Summary
Two terminal-store writes published a new
AppStateon every call even when nothing changed, and one remote IPC fired on every layout persist regardless of whether the host-visible layout had moved. Because the renderer's per-pane selectors are O(mounted panes), each redundant publish ran every subscriber's selector across the whole app.setCacheTimerStartedAtspread{ ...s.cacheTimerByKey, [key]: ts }and returned unconditionally, while every sibling action in the slice guards. The cadence is real:parked-terminal-byte-watcherwritesnullon everyonAgentBecameWorking, everyonAgentExited, and every stale-working-title clear — i.e. repeated null-over-null writes per agent transition, per parked pane. Now bails when the stored value is already identical (strict equality, so it covers bothnumberandnull). The:seedsentinel clear is preserved: a real pane write with an unchanged value still proceeds when a stale${tabId}:seedentry exists, or the bailout would strand a phantom timer.setTabLayoutspread{ ...s.terminalLayoutsByTabId }and returned unconditionally even when the normalized snapshot was structurally identical to the stored one. Its hot caller isTerminalPane.persistLayoutSnapshot, invoked from ~10 sites including an effect whose deps includepaneTitles— so pane-title churn drove layout persistence. Now bails whenterminalLayoutEqual(existing, normalized.snapshot). Normalization still runs first, and pty-ownership transfers are resolved before the bailout so that side effect is unchanged for any layout that normalization actually rewrites.setTabLayout, if any pty was remote,persistLayoutSnapshotfiredupdateWebRuntimePaneLayoutwith no comparison against what was last pushed — one remote round trip per title change per tab. Extracted intocreateRemotePaneLayoutPusher, which dedupes in-flight and successful pushes by worktree, tab, and full snapshot; failed pushes re-arm the layout and remounts start with a fresh pusher.The comparator that gates the layout write already existed as
terminalLayoutEqualinweb-session-tabs-sync.ts, module-private. It is nowsrc/renderer/src/lib/terminal-layout-equality.tsand imported back intoweb-session-tabs-sync.tsunchanged — the moved implementation is character-identical, so the mirroring path's behavior is untouched.No user-visible behavior changes: the only things eliminated are writes and IPCs that carried no new information.
Screenshots
No visual change.
Testing
pnpm lint—oxlint,audit:code-quality:native,audit:code-quality:type-aware,check:reliability-gates,check:max-lines-ratchetall cleanpnpm typecheck—config/tsconfig.tc.web.jsoncleanpnpm test—src/renderer+src/shared: 23,181 passed, 19 skipped. The only 2 failures are insrc/shared/setup-agent-sequencing.test.ts, which fails identically on the parent commit (9a97e737f5) in this environment — a pre-existing local red (Node v26 vs the repo's pinned Node 24), unrelated to this diff.pnpm build— not run; typecheck covers the compile surface and the diff is renderer-only TypeScript with no build-config or native-module changes.Measurements
Two new test files count the actual effect, and both are red on the parent commit:
Store-level subscriber wakeups (
terminal-write-identity-bailouts.test.ts) — a subscriber counter attached to a real zustand store (createTestStore), 1,000 repeat writes each:setCacheTimerStartedAtsetTabLayoutsnapshotsVerified red on parent by reverting
terminals.tstoHEAD:expected 1000 to be +0on all three, 6 of 10 tests failing.Remote-push count (
remote-pane-layout-push.test.ts) — IPC boundary mocked, 100 persist calls with an unchanged layout:updateWebRuntimePaneLayoutinvocationsVerified red by deleting the dedupe guard:
expected "vi.fn()" to be called 1 times, but got 100 times.Comparator micro-bench — realistic 8-leaf split snapshot (balanced split tree, UUID leaf ids, all four leaf-keyed records populated), 1M iterations × 5 runs after a 200k warm-up, Node v26.5.0 / Apple M5 Pro:
~0.58 µs against the 5 µs bar, i.e. the check costs roughly 1/8600th of a 5 ms frame budget, versus the O(panes) selector sweep it avoids. The bench was a throwaway; no timing assertion is committed, since wall-clock thresholds are flaky in CI.
Behavior coverage beyond the counters: publishes still happen for every tracked field (
rootshape,direction,ratio,activeLeafId,expandedLeafId,ptyIdsByLeafId,buffersByLeafId,scrollbackRefsByLeafId,titlesByLeafId); the first write for a key is recorded even when the value isnull; a stale:seedsentinel is still cleared on an otherwise-unchanged pane write; the clearing call (setTabLayout(tabId, null)) still deletes and then bails when already absent; and a bailed-out identical layout is asserted to produce no pane-ownership transfers rather than that being assumed. The pusher tests also pin that the payload is byte-for-byte what the un-deduped path sent, thattitlesByLeafIdis still omitted when absent, and that the cache is not carried across tabs.AI Review Report
Self-review of the diff, focused on the ways an identity bailout can silently change behavior:
setTabLayoutis thatnormalizeTerminalLayoutPtyOwnershipcan rewrite a layout carrying a duplicate pty id, andresolveTerminalLayoutPtyOwnershipTransfersthen moves pane-keyed store state. A naive bailout placed before that resolution would skip the transfer whenever the normalized result happened to match what was already stored. Transfers are therefore resolved before the equality check, sotransferNormalizedTerminalLayoutPtyOwnershipsees exactly what it saw before; only the state publish is skipped. Covered by a test that feeds a duplicate-pty layout, then replays the normalized snapshot 1,000 times and asserts both the stored reference and the pane-keyed maps are untouched.return svsreturn {}. zustand 5 short-circuits onObject.is(nextState, state), so returning the state object itself is what actually skips the listener sweep — returning{}would stillObject.assigna fresh state and wake everyone. Both bailouts returns, matching the existing idiom in this slice (markTerminalTabUnread,markTerminalPaneUnread,markAgentCompletionPaneUnread).:seedcarve-out. Bailing purely on value equality would have stranded the${tabId}:seedsentinel thatseedCacheTimersForIdleTabsplants, leaving a phantom countdown. The guard only fires when there is no stale sentinel to clear.undefined === nullis false, so a key's firstnullwrite still lands and is still recorded; only the second identical write bails. Pinned by a test.web-session-tabs-sync.tsimports it and its mirroring call site is unchanged. Its whole test surface passes (285 files / 3,558 tests acrossterminal-pane+runtime).tabId; failed pushes re-arm only the current attempt, and the comparator includesptyIdsByLeafId, so a remote reconnect that mints new pty ids re-pushes. The residual case is a host that loses its layout while re-adopting byte-identical pty ids; see Notes.isRemoteRuntimePtyIdstill gates the push exactly as before, and nothing here assumes a git worktree. No Git commands are added or changed, so the Git-version baseline is not in play.react-doctorreports one warning inTerminalPane.tsxat line 1649 (no-adjust-state-on-prop-change); it is present identically on the parent commit and is outside every line and effect this PR touches.The same-model until-clean review returned clean after scoped fix and coverage passes, and
$electronevidence is attached in the PR conversation.Security Audit
No new attack surface. The diff adds no input parsing, no command execution, no filesystem or path handling, no auth or secret handling, and no dependencies. The one IPC touched (
updateWebRuntimePaneLayout) is unchanged in payload, sender, and validation — it is simply invoked less often, and the dedupe decision is made entirely from renderer-local state that the renderer already owned. The extracted comparator usesObject.prototype.hasOwnProperty.callfor record comparison, preserving the original's prototype-pollution-safe key check. No follow-up needed.Notes
createRemotePaneLayoutPusherskips a push when the tab's full layout snapshot matches the last one pushed. It is keyed by tab and worktree, failed pushes re-arm the current attempt, and the comparator includesptyIdsByLeafId— so a remote runtime that reconnects with fresh pty ids re-pushes. The one path that could theoretically skip a needed push is a host that loses its stored pane geometry while the client re-adopts byte-identical pty ids for the same leaves; in that case the host would also be replaying a stale layout back throughweb-session-tabs-sync, which changes the client snapshot and re-arms the push. Flagging it as the one behavioral edge worth watching.root/expandedLeafId/titlesByLeafId, but the dedupe compares the whole snapshot. That is deliberate and conservative: equality on the full snapshot implies equality on the payload, so the dedupe can never skip a push whose payload actually differs.