perf(tabs): scroll the active tab into view once per activation, not three times - #1277
Merged
Conversation
|
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! |
h4yfans
marked this pull request as ready for review
August 12, 2026 14:22
This was referenced Aug 12, 2026
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.
Closes #1105. Part of epic #987 (memory/CPU audit 2026-08).
What was wrong
apps/desktop/src/renderer/src/components/tabs/tab-bar-with-drag.tsx:111-116(pre-fix) — one tab activation fired three smoothscrollIntoViewanimations:canScrollToStart/canScrollToEndare gutter flags, and each one that flips adds aps-7/pe-7gutter to the strip. The activation scroll is what makes them flip, so the effect re-entered twice more and re-issued a smooth scroll each time, restarting the animation mid-flight. Measured on a strip that overflows: 3scrollIntoViewcalls for a single activation (1 on activation, +1 whencanScrollToEndflips in the same commit, +1 whencanScrollToStartflips on the first scroll event).The gutter re-runs are not pointless — a widening gutter really can push the tab back out of view — so they could not simply be dropped from the deps.
What changed
scrolledTabIdRef). A run whose target changed always animates; a repeat run for the same tab (i.e. a gutter-driven one) re-scrolls only if the tab is no longer fully inside the strip.isTabFullyVisiblecompares the rects and subtracts the strip's inline padding, so a tab hidden under a chevron still counts as out of view — the correction the old deps existed for is preserved, minus the redundant animations.apps/docs/src/user-guide/tabs-split-view.md— one line on the single scroll animation, one line stating tabs are never auto-closed.Deliberately NOT done: the tab cap
The issue also asks for a
maxTabssoft cap with LRU-close. I did not add one, on purpose:recentlyClosedonly retains 25 entries, in memory, so it is not a safety net across a restart). Refusing to open past a limit means clicking a note in the sidebar silently does nothing. Both are worse than the thing they fix. This is a live-user build; tab state is persisted.split-view/tab-pane.tsx:57renders<TabContent>for the active tab only — so an idle tab costs one record plus itsviewState. The other per-tab accumulators are already capped: activation history at 50 (reducers/history-helpers.ts:3), recently-closed at 25 (reducers/tab-crud-reducer.ts:29), and the persistence serialize +JSON.stringifynow runs once per debounce window rather than per state change (perf(tabs): serialize the tab tree once per debounce window #1174, which closed [HIGH][tabs] Tab persistence serializes + JSON.stringifies the whole tab tree on every state change (debounce only covers the write) #1056 — re-verified inpersistence/hooks.ts:62-88).The only
maxTabsreference in the tree is a stray field in a test fixture (persistence/persistence.test.tsx:112) that is cast away and read by nothing; left untouched as pre-existing dead data. Since no cap is introduced, a persisted session of any size still restores in full.How it was proven
New
apps/desktop/src/renderer/src/components/tabs/tab-bar-scroll.test.tsxdrives the real component with stubbed strip geometry (scrollWidth/clientWidth/scrollLeft), so the gutter flags flip exactly as they do in the app, and countsscrollIntoViewcalls.origin/mainsource, same test)git checkout origin/main -- …/tab-bar-with-drag.tsx→Test Files 1 failed (1) / Tests 2 failed (2)—expected "vi.fn()" to be called 1 times, but got 2 times.Test Files 1 passed (1) / Tests 2 passed (2).Verification
Backward compatibility
Render-time only: no schema, contract, settings, persisted-tab-state, or sync-shape change, and no cap that could drop tabs from an existing session.