Skip to content

fix(tabs): a tab strip the user scrolled is not pulled back to the open tab when a typeface arrives - #479

Merged
devswha merged 3 commits into
mainfrom
fix/tab-strip-face-scroll
Oct 5, 2026
Merged

devswha merged 3 commits into
mainfrom
fix/tab-strip-face-scroll

Conversation

@devswha

@devswha devswha commented Oct 5, 2026

Copy link
Copy Markdown
Owner

Follow-up to #473 (unreleased), from its review: two Low findings.

What was wrong

1. The tab strip jumped back to the open tab on every face arrival. Since #473 TabStrip follows useFacesArrived() and brings the open tab into view again each time a face loads, because the names are redrawn at another width. That is right when nobody touched the strip, and wrong after the user scrolled it: on a phone with more tabs than fit, open the last tab, drag the strip back to look at the first ones, and then the first Korean text appears on the page. Its Pretendard chunk loads (this can be long after page load) and the strip jumps back to the last tab.

2. scripts/font-swap-demo-regression.ts did not test what it said. Its end-following cases opened an 844px-tall window in which the demo conversation does not overflow (measured here: 437 in 437), so "still at the end" was true of anything, and the height change was only logged.

What changed

  • src/lib/tabStripScroll.ts (new, pure): the rule. The strip keeps where it is known to be (at) and whether the user scrolled it (moved).
  • TabStrip.tsx: one onScroll on the strip. The effect is split in two: selection, tab count and strip visibility place the open tab as before; a face arrival places it only while moved is false. A change of the open tab clears moved.

How a user scroll is told from the strip's own. No input event is listened for. Wheel, touch drag and its momentum, a key, and a focus that scrolls a tab into view all end in a scroll event, and so does the strip's own scrollLeft assignment, so they are told apart by position:

  • when the strip places itself it reads scrollLeft back (the value the browser kept after clamping) and stores it; a scroll event that finds the strip exactly there is its own;
  • a scroll event that finds it lower, at the new end of a row that got shorter (a face that drew names narrower, a tab renamed or closed: the browser clamps), is nobody's scroll and only updates the known position;
  • anything else is the user's.

The strip has no scrollbar (scrollbar-width: none), so there is no separate scrollbar path; a scrollbar drag would arrive as the same event anyway. scroll-behavior is not smooth here, so one assignment is one final position. One frame of ambiguity remains by nature: a user scroll whose event has not fired yet when the strip places itself in the same frame is overwritten, as before.

  • scripts/font-swap-demo-regression.ts: the two end-following cases use a 480px window and now require an overflowing transcript and a height that changed with the face before asserting the reader is at the end. Two strip cases added on the demo transport (tabs made through the demo's /api/tab/create).
  • scripts/mobile-tabs-regression.ts: feat(type): the app ships Pretendard and JetBrains Mono, and the chat reads a step larger #473's held-fonts case runs twice, the second time with a user scroll before the faces come.

Tested

  • bun run typecheck: clean. bun run test:unit: 1322 pass, 4 skip, 0 fail (6 new in src/lib/tabStripScroll.test.ts). bun run build: ok.
  • CHROME_PATH=/usr/bin/google-chrome bun scripts/font-swap-demo-regression.ts: 5 PASS (transcript height 430 -> 455 on arrival; strip follows its open tab, scrollLeft 154 -> 157, after a scroll that ended in opening that tab; a scrolled strip stays at 0).
  • The same script against main's TabStrip.tsx: the new scrolled-strip case fails (157 !== 0), the others pass.

Not tested

  • scripts/mobile-tabs-regression.ts needs a live herdr session and was not run locally: it was only bundle-checked (bun build, together with ui-regression.ts). CI's "Integration and browser" is its first run.
  • font-swap-demo-regression.ts is not part of CI (as before this PR), so its assertions are checked only on a developer's machine; "height changed" depends on the machine's fallback font differing from Pretendard in width.
  • The scripts scroll the strip by setting scrollLeft from the page. The app cannot tell that from a wheel or a finger (it sees only the scroll event), but no real touch drag with momentum was driven, and no real phone was used.
  • RTL is not considered (the strip is LTR in all four languages).

🤖 Generated with Claude Code

devswha and others added 2 commits October 6, 2026 01:07
…en tab when a typeface arrives

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented Oct 5, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

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

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Repository: devswha/herdr-web-ui/.coderabbit.yaml
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 3b4d09dc-b8d4-45eb-a996-a300f58b24f3
📥 Commits

Reviewing files that changed from the base of the PR and between d33f266 and 4e3fffd.

📒 Files selected for processing (1)
  • CHANGELOG.md
🚧 Files skipped from review as they are similar to previous changes (1)
  • CHANGELOG.md

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


📝 Summary

Summary by CodeRabbit

  • Bug Fixes
    • When fonts load late and change tab-label widths, the tab strip now keeps the open tab in view if you haven’t scrolled the strip yourself. If you have scrolled it, the strip stays at your chosen position until you open another tab, which is then brought into view.
    • Improved handling of delayed font loading in short-viewport layouts.

Walkthrough

The tab strip now tracks user scrolling. Late font loading brings the active tab into view unless the user has scrolled the strip. Regression checks cover both scroll states and verify that opening a tab brings it into view.

Changes

Late font loading and tab scrolling

Layer / File(s) Summary
Track tab-strip scroll state
src/lib/tabStripScroll.ts, src/lib/tabStripScroll.test.ts
New helpers track the strip position and whether the user moved it. Tests cover placement, scrolling, row shortening, and tab selection.
Apply scroll tracking in TabStrip
src/components/TabStrip.tsx
TabStrip tracks scroll events and brings the active tab into view after selection, tab-count, or visibility changes, and after font changes unless the user has scrolled.
Verify late-font tab behavior
scripts/font-swap-demo-regression.ts, scripts/mobile-tabs-regression.ts, docs/development.md, CHANGELOG.md
Regression checks cover scrolled and unscrolled strips during font swaps. The demo checks also verify viewport-relative conversation sizing. The documentation and changelog describe the regression coverage and behavior.

Priority: ⬇️ Low

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

Change: Bug fix

Merge Risk: ⚪ Minimal · up to 4e3ff

No actionable merge-blocking risk is identified; the change is mergeable after normal checks.

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and specifically describes the main change: a user-scrolled tab strip stays in place when a typeface arrives.
Description check ✅ Passed The description explains the problem, the behavior change, and the validation results, including tests that were not run and their limitations. It does not include the screenshot or recording requeste…
Docstring Coverage ✅ Passed Docstring coverage is 80.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 5 functions across 5 files. (1 skipped: 1 u…
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Commit to this branch
  • 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.

@devswha
devswha merged commit 9f9ef7f into main Oct 5, 2026
4 checks passed
@devswha
devswha deleted the fix/tab-strip-face-scroll branch October 5, 2026 16:31
devswha added a commit that referenced this pull request Oct 6, 2026
…ntry

Its branch had appended the tab-strip sentence to the fonts entry under
[0.3.50], which was released before #479 merged. The sentence is its own
entry under [0.3.51] Fixed, and the 0.3.50 entry links #473 alone again.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
devswha added a commit that referenced this pull request Oct 6, 2026
Its branch had appended the tab-strip sentence to the fonts entry under
[0.3.50], which was released before #479 merged. The sentence is its own
entry under [0.3.51] Fixed, and the 0.3.50 entry links #473 alone again.

Bumps package.json and herdr-plugin.toml to 0.3.51 and moves the
Unreleased notes under [0.3.51] - 2026-10-06, with the compare link. Raises the
remote PC bundle to 18: #368 adds the `take-over` capability the bridge
advertises, and #477, #483, #484, #485 and #486 change what the bridge reads
(transcripts, prompts, process start times, a pane's Codex store), so a
remote PC on bundle 17 keeps the old behaviour until its bridge is updated.
Push the `remote-v18` tag and let the bundle workflow publish before
running release.yml.

Carries #368 #477 #479 #481 #482 #483 #484 #485 #486 #487

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant