fix(mobile): make every session tab reachable in the tab strip - #267
Merged
Conversation
With five tabs open on a phone, the right-hand tabs were effectively unreachable. Selecting a tab only toggled the .active class, so the strip never moved, and every full rebuild (a task badge appearing, a session created elsewhere) replaced the strip's innerHTML, which resets scrollLeft to 0 and yanked a mid-swipe strip back to the first tab. Three changes, which only work together: * computeTabScrollLeft() (pure, constants.js) decides the scroll target from measured rects, and _scrollActiveTabIntoView() applies it on selection. Rect math on the strip's own scrollLeft rather than scrollIntoView(), which also scrolls ancestors: on a phone that is the document, under a fixed header and possibly an open keyboard. * _fullRenderSessionTabs() saves and restores scrollLeft across the rebuild, and re-reveals the active tab only when it actually changed (_lastRenderedActiveTabId), so a background render never undoes a manual swipe. * Mobile no longer hoists the active session to the front of the strip. That reordering ran on full renders only, so tab order flipped depending on which render path fired, and it renumbered the Alt+N badges. Scrolling the active tab into view replaces it. Also sets overscroll-behavior-x: contain on the strip so a swipe that runs past the last tab stays in the strip instead of becoming the browser's back gesture. Tests: scroll-target math in test/tab-overflow.test.ts (runs in CI), plus five browser regressions in test/mobile/tabs.test.ts covering reveal-on- select in both directions, scroll preservation across an ambient rebuild, sessionOrder rendering on phones, and a real touch drag reaching the last tab. Closes#257 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Uh oh!
There was an error while loading. Please reload this page.
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 freeto 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.
Fixes#257.
The problem
With five tabs open on a phone, the tabs on the right could not be reached. Two separate things caused it, and fixing either alone is not enough:
selectSession()goes through_updateActiveTabImmediate(), which toggles the.activeclass and nothing else. Nothing ever touchedscrollLeft._fullRenderSessionTabs()replaces the strip'sinnerHTML, which dropsscrollLeftback to 0. That rebuild fires on ambient events (a task badge appearing or disappearing, a session created or deleted elsewhere, a tab-order broadcast), so a strip the user had just swiped rightward snapped back to the first tab a moment later.There was also a mobile-only reordering that hoisted the active session to the front of the strip, dating from when only one tab fit on a phone. It ran on full renders only, so with five tabs the order flipped depending on which render path happened to fire, the tab you just tapped teleported to position 0, and the Alt+N badge numbers were reshuffled.
The fix
computeTabScrollLeft()inconstants.jsis a pure policy: given the strip's measurements and a tab's box it returns thescrollLeftthat puts that tab in view (leaving a 16px sliver of the neighbour showing, so the strip still reads as scrollable), clamped to the scrollable range, and returns the current position when the tab is already visible._scrollActiveTabIntoView()feeds it measured rects and applies the result. It writes the strip's ownscrollLeftrather than callingscrollIntoView(), which also scrolls ancestors: on a phone that means the document, under a fixed header and possibly an open keyboard. It honoursprefers-reduced-motion._updateActiveTabImmediate()calls it, so every selection path (tap, swipe, Alt+N, command palette, notification) reveals its tab._fullRenderSessionTabs()saves and restoresscrollLeftacross the rebuild, and re-reveals the active tab only when it actually changed (_lastRenderedActiveTabId). Restoring unconditionally and revealing conditionally is what lets someone browse the far end of the strip while a background render fires, without a newly selected tab ever being stranded off-screen.sessionOrder, like every other device.overscroll-behavior-x: containon the strip, so a swipe running past the last tab stays in the strip instead of chaining into the browser's back gesture.Desktop is untouched: it wraps to a second row,
scrollWidth === clientWidth, and the policy no-ops.Tests
test/tab-overflow.test.ts(runs in CI): six cases over the scroll-target math, covering both edges, the already-visible no-op, clamping at both ends, a tab wider than the window, and missing measurements.test/mobile/tabs.test.ts: five browser regressions on an iPhone 14 Pro profile with five real tabs rendered through_fullRenderSessionTabs(): reveal-on-select rightward and leftward, scroll preserved across an ambient rebuild,sessionOrderrendering on phones, and a real CDP touch drag reaching the last tab.npm run test:cipasses locally (233 files, 4756 tests).Note:
test/mobile/tabs.test.tshas one pre-existing failure on master ("switching tabs with the keyboard closed...": it expectspreserveKeyboard: false, whilehandleSessionTabClickpasses no such option). That suite is excluded from CI so it drifted; this PR leaves it alone.