Skip to content

fix(web): prevent composer label measurement update loops - #13106

Closed
jorda0mega wants to merge 1 commit into
pingdotgg:mainfrom
jorda0mega:t3code/fix-fable-scroll-loop
Closed

jorda0mega wants to merge 1 commit into
pingdotgg:mainfrom
jorda0mega:t3code/fix-fable-scroll-loop

Conversation

@jorda0mega

@jorda0mega jorda0mega commented Sep 22, 2026 •

Copy link
Copy Markdown

Why the change

Scrolling can collapse the composer and trigger React error #185 because split branch names are measured differently when their labels collapse, so this keeps their full measured width stable.

Special things to note

  • Reproduced the original crash in the local web client with a synthetic eight-turn thread and the page constrained to 960px; the same layout and scrolling remain stable with this fix.
  • 115 focused tests pass, including expanded, intermediate, and collapsed label widths; web typecheck, targeted lint, and formatting pass.
  • Shared web/desktop toolbar logic only; no provider, mobile, or wire-contract changes.

Change outline

 BranchToolbar → useLabelsOverflow → measure label text
-  maximum descendant scrollWidth
+  MiddleTruncate: sum head and tail scrollWidth
+  other descendants: keep existing maximum
   compare full width with available space
   keep compact / expanded labels stable
Before: composer collapse crashes After: compact composer stays stable
Before After

Before recording:

before.mp4

After recording:

after.mp4

Summary by CodeRabbit

  • Bug Fixes

    • Improved branch toolbar width calculations for long or middle-truncated branch names.
    • Branch labels now account for the full rendered text width, helping prevent incorrect clipping and overflow behavior.
  • Tests

    • Added coverage for branch labels with varying available widths, including narrow and fully hidden text areas.

@github-actions github-actions Bot added vouch:unvouched PR author is not yet trusted in the VOUCHED list. size:S 10-29 changed lines (additions + deletions). labels Sep 22, 2026
@jorda0mega
jorda0mega marked this pull request as ready for review September 22, 2026 18:21
@coderabbitai

coderabbitai Bot commented Sep 22, 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: pingdotgg/t3code/.coderabbit.yaml

Review profile: CHILL

Plan: Advanced

Run ID: d5f7f7ce-0529-44df-8e13-716d35cb82c5

📥 Commits

Reviewing files that changed from the base of the PR and between f25a8e4 and 83d2fe8.

📒 Files selected for processing (4)
  • apps/web/src/components/BranchToolbar.logic.test.ts
  • apps/web/src/components/BranchToolbar.logic.ts
  • apps/web/src/components/BranchToolbar.tsx
  • apps/web/src/components/ui/middle-truncate.tsx

Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review.


📝 Walkthrough

Walkthrough

The change extracts context strip label-width measurement into measureContextStripLabelWidth. It handles middle-truncated content by summing child widths. MiddleTruncate adds a identifying data attribute, and the branch toolbar uses the helper.

Changes

Context strip width measurement

Layer / File(s) Summary
Width measurement helper and validation
apps/web/src/components/BranchToolbar.logic.ts, apps/web/src/components/ui/middle-truncate.tsx, apps/web/src/components/BranchToolbar.logic.test.ts
The new helper measures maximum rendered widths and sums child widths for middle-truncated elements. Tests cover label widths of 200, 100, and 0, with an expected result of 200. MiddleTruncate exposes the data-slot="middle-truncate" attribute.
Branch toolbar integration
apps/web/src/components/BranchToolbar.tsx
useLabelsOverflow now calls measureContextStripLabelWidth instead of scanning the label subtree inline.

Priority: ➖ Normal

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Bug fix

Suggested reviewers: juliusmarminge, maria-rcks

Merge Risk: ⚪ Minimal · up to 83d2f

The reported scrolling measurement loop is addressed, with focused coverage and no identified regression.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 25.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 4 functions across 4 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
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.
Title check ✅ Passed The title clearly identifies the main fix: preventing composer label measurement update loops. It matches the pull request objectives and changes.
Description check ✅ Passed The description explains the problem, the measurement approach, validation results, and UI behavior. It includes screenshots and recordings for the UI change. It does not use the template's exact "Wha…
  • Fix all pre-merge checks with AI
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create a new PR

Comment @coderabbitai help to get the list of available commands.

@juliusmarminge

Copy link
Copy Markdown
Member

Superseded by #13555 (merged) — same composer/BranchToolbar label measurement loop fix (sum MiddleTruncate text widths instead of max scrollWidth).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size:S 10-29 changed lines (additions + deletions). vouch:unvouched PR author is not yet trusted in the VOUCHED list.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants