Repository navigation
Fix tab indicator selection chrome - #125
Conversation
…65-tab-indicator-glitch-merge
📝 WalkthroughWalkthroughRefactored tab-bar frame tracking from preference-key aggregation to on-demand computation from a persisted frames dictionary, centralized animation suppression for hover and split-button state changes using transaction-wrapped updates, and added unit test coverage for selected tab frame selection. ChangesTab bar frame and animation refactoring
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Greptile SummaryThis PR fixes a tab indicator chrome selection glitch by replacing a
Confidence Score: 5/5Safe to merge — the change is a targeted, well-understood fix with direct test coverage and no altered external API surface. The logic change is small and correct: replacing a state variable populated by a lagging preference-key observer with a computed property that reads directly from the live tab-frames dictionary eliminates the root cause of the glitch. Animation suppression is applied surgically via the new helper and by wrapping state mutations in no-animation transactions. The new test covers the frame-lookup logic path including the nil-selection edge case. No files require special attention. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[pane.selectedTabId changes] --> B[selectedTabFrameInBar computed property]
C[Layout pass: GeometryReader emits frames] --> D[TabFramePreferenceKey collects all tab frames]
D --> E[onPreferenceChange withTransaction animation=nil tabFramesInBar = frames]
E --> B
B --> F{Lookup tabFramesInBar selectedTabId}
F -->|found| G[CGRect for selected tab]
F -->|nil or missing| H[nil: no indicator]
G --> I[selectedIndicatorFrame renders maskedSelectedTabIndicatorChrome]
Reviews (1): Last reviewed commit: "fix: narrow tab bar animation transactio..." | Re-trigger Greptile |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Sources/Bonsplit/Internal/Views/TabItemView.swift (1)
140-145:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUse the macOS 14
onChangeclosure signature to clear deprecation warnings.Lines 139, 140, and 144 use the deprecated single-parameter closure form. Update them to use the two-parameter closure signature.
♻️ Proposed fix
- .onChange(of: tab.isLoading) { _ in updateGlobeFallback() } - .onChange(of: tab.iconImageData) { _ in + .onChange(of: tab.isLoading) { _, _ in updateGlobeFallback() } + .onChange(of: tab.iconImageData) { _, _ in updateRenderedFaviconImage() updateGlobeFallback() } - .onChange(of: tab.icon) { _ in updateGlobeFallback() } + .onChange(of: tab.icon) { _, _ in updateGlobeFallback() }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Sources/Bonsplit/Internal/Views/TabItemView.swift` around lines 140 - 145, Replace the deprecated single-parameter onChange closures for tab.isLoading, tab.iconImageData, and tab.icon with the macOS 14 two-parameter signature; update the three calls (.onChange(of: tab.isLoading), .onChange(of: tab.iconImageData), .onChange(of: tab.icon)) to use the (oldValue, newValue) closure form and call updateGlobeFallback() and updateRenderedFaviconImage() inside that closure (you may ignore oldValue/newValue by using underscores if not needed), ensuring the closures reference the same updateGlobeFallback() and updateRenderedFaviconImage() functions.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@Sources/Bonsplit/Internal/Views/TabItemView.swift`:
- Around line 140-145: Replace the deprecated single-parameter onChange closures
for tab.isLoading, tab.iconImageData, and tab.icon with the macOS 14
two-parameter signature; update the three calls (.onChange(of: tab.isLoading),
.onChange(of: tab.iconImageData), .onChange(of: tab.icon)) to use the (oldValue,
newValue) closure form and call updateGlobeFallback() and
updateRenderedFaviconImage() inside that closure (you may ignore
oldValue/newValue by using underscores if not needed), ensuring the closures
reference the same updateGlobeFallback() and updateRenderedFaviconImage()
functions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: eedd938e-bc16-4a09-b2b7-e4595172aa4e
📒 Files selected for processing (3)
Sources/Bonsplit/Internal/Views/TabBarView.swiftSources/Bonsplit/Internal/Views/TabItemView.swiftTests/BonsplitTests/BonsplitTests.swift
2 participants
Summary
Testing
Supports manaflow-ai/cmux#3969.
Summary by cubic
Fixes the tab indicator so it always follows the current selection and snaps instantly during rapid switches. Disables tab-bar button animations and hard-contains the split action lane to prevent flicker, bleed, and startup layout jumps.
pane.selectedTabIdand measured tab frames viaTabBarStyling.selectedTabFrame(...), removingSelectedTabFramePreferenceKey. Added a unit test confirming it tracks the current selection and returns nil when none is selected..tabBarButtonAnimationsDisabled()and use no-animation transactions for hover tracking and tab-frame preference updates (including split buttons).Written for commit e093031. Summary will update on new commits. Review in cubic
Summary by CodeRabbit
Refactor
Tests