perf(tabs): serialize the tab tree once per debounce window - #1174
Merged
Conversation
The auto-save effect ran serializeTabState() plus JSON.stringify() on every tab-state change and only debounced the write, so a burst of changes walked the whole tab tree and JSON-encoded the payload once per change while producing a single save. Dragging a split divider dispatches RESIZE_SPLIT on every mousemove, so this ran at pointer-event frequency. Move both calls inside the debounced callback. The effect still re-runs on every state change, so only the last timer of a burst survives and it closes over the newest state - the persisted payload is unchanged. Also register the beforeunload listener once instead of tearing it down and re-adding it on every state change; it now reads the current state from the ref the flush registry already keeps in sync.
|
React Doctor found no new issues. 🎉 Reviewed by React Doctor for commit |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
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
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.
Problem
useTabPersistencedebounced only the write. The effect body itself ranserializeTabState(state)andJSON.stringify(serialized)synchronously onevery tab-state change, before the debounce ever applied. A burst of N state
changes performed N full walks of the tab tree and N JSON encodes to produce
exactly one save.
The
beforeunloadlistener was also torn down and re-added on every state change(
[state, enabled]deps), because the handler closed overstate.Corrected trigger. The issue guessed "per keystroke title update"; no
per-keystroke title dispatch exists. The real high-frequency driver is
split-pane.tsx→onResize→RESIZE_SPLIT, dispatched on everymousemovewhile dragging a split divider (no rAF throttle). Every discretetab action (open/close/activate/pin/reorder/rename,
SAVE_TAB_STATEview-statewrites) paid the same cost once each.
Measured cost of one serialize + stringify (30 tabs, each with a realistic
viewStateblob, 18,571-byte payload, 2000 iterations after warm-up):0.0320 ms/op. At pointer-event frequency that is ~2 MB/s of throwaway stringsand object graphs on the renderer thread, for a single write.
Root cause
apps/desktop/src/renderer/src/contexts/tabs/persistence/hooks.ts— theserialize + stringify pair lived in the effect body instead of in the
setTimeoutcallback.Fix
Move
serializeTabState+JSON.stringify(and thelastSavedRefdedupe thatdepends on them) inside the debounced callback. The effect still re-runs on every
state change and still clears/reschedules the timer, so only the last timer of
a burst survives and it closes over the newest
state— the payload written isidentical to before.
Register the
beforeunloadlistener once ([enabled]deps) and readstateRef.current, the ref the flush registry already keeps in sync via ano-dep effect declared above it (so it is committed before any later effect or
any async browser event can read it).
Measured before / after
beforeunloadregistrationsTest evidence
New tests in
persistence.test.tsxcount real calls intoserializeTabState(module mocked with
importOriginalpassthrough, so the counter wraps the realimplementation) — assertions are on observable counts, never on timing.
RED, before the fix:
GREEN, after the fix:
Tests 7 passed (7).Both tests pin the exact persisted payload (every tab field, layout,
settings, and
savedAtunder a pinned system clock), not just its shape.Mutation verification — 4 mutants, 4 killed
const serialized = serializeTabState(state)back out of thesetTimeoutexpected 21 to be +0}, [enabled])→}, [state, enabled])on the unload effectexpected … to have a length of 1 but got 21serializeTabState(stateRef.current)→serializeTabState(state)in the unload handler"title": "Roadmap"instead of"Roadmap 20"statefrom the debounce effect deps (a cheaper "schedule once, never reschedule" implementation: serialize count stays 1)"title": "Roadmap"instead of"Roadmap 20"M4 is the important one: it proves the exact-payload assertion, not just the
count assertion, is load-bearing, so a cheaper implementation that serializes
once but writes stale state cannot pass.
Risk & backward compatibility
Persisted payload — unchanged.
serializeTabStateis untouched; the samePersistedTabState(version: 2) is written to the samememry_tab_statelocalStorage key with the same fields. A tree written by an older app version
still restores here, and a tree written here still restores in an older version.
Nothing about what is written changed — only where in the timeline the
identical computation happens.
The final pending write is never dropped:
app:request-flush→useFlushOnQuit→flushAllPendingSaves()→ thetab-stateregistry entry serializesstateRef.currentandsaveSyncs it synchronously to localStorage. Thepending debounced write is superseded by a full write of the newest state.
Untouched by this PR, and it already read through the ref.
app:request-flushhandshake (per-window, per-requestafter fix(shutdown): scope the window flush handshake per window and request #1150), same registry callback, same synchronous write.
beforeunload: still writes synchronously viasaveSync. It now readsstateRef.currentinstead of a closed-overstate— equally fresh (the ref issynced by a no-dep effect declared above it, so it is current at commit time).
switchVaultdoes not reload or unmount the renderer;TabPersistenceManagerstays mounted and the pending timer keeps running andfires normally. Tab state is a single global key, not per-vault. Unchanged
before/after.
No memoization, no cached serialized string. The dedupe key (
lastSavedRef)is computed fresh from the live state each time the timer fires — there is no
cache that could go stale, so there is no invalidation matrix to get wrong. The
lastSavedRefcomparison keeps exactly its previous semantics (it is effectivelyinert either way because
serializeTabStatestampssavedAt: Date.now();deliberately left alone as out of scope).
Behavioural delta, stated precisely: previously, when the new state
serialized identically to the last saved one, the effect returned early and left
an older pending timer alive; now the timer is always rescheduled and the write
is skipped inside the callback. Either way the newest state is what ends up
persisted.
Verification
pnpm typecheck— 16/16 tasks successfulpnpm lint— 0 errors, 15 pre-existing warnings, none in the changed files(confirmed with a targeted
eslint --no-cache --max-warnings=0onhooks.ts:clean)
pnpm exec vitest run --config config/vitest.config.ts --project renderer src/renderer/src/contexts/tabs src/renderer/src/App.test.tsx src/renderer/src/components/split-view— 136 passed, 0 failedgit diff --check— cleannpx -y react-doctor@latest .— run twice, byte-identical output (1855 files,score 64/100), no findings in the changed files
Docs
pnpm docs:impact --base origin/main --strictreportsmissing-docsbecause thediff touches
apps/desktop/**. I reviewed the docs and deliberately did notupdate them: this is an internal timing change with no user-visible effect.
user-guide/tabs-split-view.mdanduser-guide/settings.mddescribe whatRestore Session brings back; the payload, the debounce interval, and the restore
semantics are all unchanged, so those pages remain accurate.
Relation to other open work
use-tab-actions-stable-identity) — splits the tabs context intoseparate actions/state halves so action-only consumers stop re-rendering. It
touches
contexts/tabs/context.tsxandcontext.test.tsx. This PR touchesonly
persistence/hooks.tsandpersistence/persistence.test.tsx— no fileoverlap.
useTabPersistenceis a genuine state consumer (it must see everystate change to reschedule), so it stays on
useTabs(). fix(tabs): scope useTabActions to a stable actions context #1137 reduces howmany components re-render; this PR reduces what each state change costs in
the one component that legitimately must observe them. Complementary, no
conflict.
that handshake through the existing
save-registryentry(
FLUSH_REGISTRY_KEY = 'tab-state'). No second mechanism invented, and fix(shutdown): scope the window flush handshake per window and request #1150'sbranch is untouched.
Out-of-scope findings (recorded, not fixed)
split-pane.tsx:76dispatchesRESIZE_SPLITon everymousemovewith norAF/throttle, so a divider drag pushes 60–120 reducer passes per second
through the whole tab tree. This PR removes the serialization cost from that
path, but the reducer churn and the resulting re-render of every tab pane
remain.
serializeTabStatestampssavedAt: Date.now(), so thelastSavedRefdedupein
useTabPersistencecan never match and is dead in practice — everydebounce window writes, even when nothing durable changed.
SET_TAB_MODIFIED/UPDATE_TAB_TITLEintab-modify-reducer.tsallocate afresh state object even when the value is unchanged, so idempotent calls still
trigger a full re-render of every tab-state consumer.
Closes #1056