Repository navigation
Fix tab indicator animation drift - #134
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughTabBarView now wraps its selected tab indicator with an explicit SwiftUI transaction that disables animations, ensuring the indicator's layout updates are not animated. Two new regression tests validate this behavior by asserting the indicator disappears when scrolled out of view and jumps immediately (not animated) to newly selected tabs. Supporting image-analysis utilities extract the indicator's pixel position for test verification. ChangesTab Indicator Animation Fix
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
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 |
Greptile SummaryThis PR fixes the active tab indicator drifting during animated selection transitions by adding
Confidence Score: 4/5Safe to merge — the production change is a minimal, targeted opt-out of inherited animations that mirrors an established pattern in the codebase, and the new tests directly validate the before/after behavior. The fix itself is a single Tests/BonsplitTests/BonsplitTests.swift — the new Important Files Changed
Sequence DiagramsequenceDiagram
participant Caller
participant SwiftUI
participant TabBarView
participant selectedTabIndicator
Caller->>SwiftUI: "withAnimation(.linear(duration: 10)) { selectedTabId = second }"
SwiftUI->>TabBarView: state update with animation transaction
TabBarView->>selectedTabIndicator: "recompute frame (minX = second tab origin)"
Note over selectedTabIndicator: .transaction strips inherited animation
selectedTabIndicator-->>SwiftUI: snaps to new position immediately
SwiftUI-->>Caller: indicator at second tab frame after first layout pass
Reviews (1): Last reviewed commit: "test: fail missing scroll view setup" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| @MainActor | ||
| private func highSaturationRange(in view: NSView, sampleRect: NSRect) -> ClosedRange<CGFloat>? { | ||
| let integralBounds = view.bounds.integral | ||
| guard let bitmap = view.bitmapImageRepForCachingDisplay(in: integralBounds) else { return nil } | ||
| bitmap.size = integralBounds.size | ||
| view.cacheDisplay(in: integralBounds, to: bitmap) | ||
|
|
||
| let scaleX = CGFloat(bitmap.pixelsWide) / max(1, integralBounds.width) | ||
| let scaleY = CGFloat(bitmap.pixelsHigh) / max(1, integralBounds.height) | ||
| let minX = max(0, Int(floor(sampleRect.minX * scaleX))) | ||
| let maxX = min(bitmap.pixelsWide, Int(ceil(sampleRect.maxX * scaleX))) | ||
| let minY = max(0, Int(floor(sampleRect.minY * scaleY))) | ||
| let maxY = min(bitmap.pixelsHigh, Int(ceil(sampleRect.maxY * scaleY))) | ||
| var firstActiveX: Int? | ||
| var lastActiveX: Int? | ||
|
|
||
| for x in minX..<maxX { | ||
| var hasIndicatorPixel = false | ||
| for y in minY..<maxY { | ||
| guard let color = bitmap.colorAt(x: x, y: y), | ||
| let rgb = color.usingColorSpace(.sRGB), | ||
| rgb.alphaComponent > 0.05 else { continue } | ||
| let alpha = min(max(rgb.alphaComponent, 0), 1) | ||
| let red = rgb.redComponent * alpha | ||
| let green = rgb.greenComponent * alpha | ||
| let blue = rgb.blueComponent * alpha | ||
| let high = max(red, green, blue) | ||
| guard high > 0.01 else { continue } | ||
| let low = min(red, green, blue) | ||
| if (high - low) / high > 0.4 { | ||
| hasIndicatorPixel = true | ||
| break | ||
| } | ||
| } | ||
| guard hasIndicatorPixel else { continue } | ||
| if firstActiveX == nil { | ||
| firstActiveX = x | ||
| } | ||
| lastActiveX = x | ||
| } | ||
|
|
||
| guard let firstActiveX, let lastActiveX else { return nil } | ||
| return (CGFloat(firstActiveX) / scaleX)...(CGFloat(lastActiveX + 1) / scaleX) | ||
| } |
There was a problem hiding this comment.
highSaturationRange is a near-verbatim copy of the same loop in the existing highSaturationWidth — same bitmap setup, same scale computation, same per-column saturation test. If the saturation threshold or alpha guard ever needs adjusting, both functions will need to be updated in sync. Consider extracting the shared predicate into a helper that both callers reuse.
| @MainActor | |
| private func highSaturationRange(in view: NSView, sampleRect: NSRect) -> ClosedRange<CGFloat>? { | |
| let integralBounds = view.bounds.integral | |
| guard let bitmap = view.bitmapImageRepForCachingDisplay(in: integralBounds) else { return nil } | |
| bitmap.size = integralBounds.size | |
| view.cacheDisplay(in: integralBounds, to: bitmap) | |
| let scaleX = CGFloat(bitmap.pixelsWide) / max(1, integralBounds.width) | |
| let scaleY = CGFloat(bitmap.pixelsHigh) / max(1, integralBounds.height) | |
| let minX = max(0, Int(floor(sampleRect.minX * scaleX))) | |
| let maxX = min(bitmap.pixelsWide, Int(ceil(sampleRect.maxX * scaleX))) | |
| let minY = max(0, Int(floor(sampleRect.minY * scaleY))) | |
| let maxY = min(bitmap.pixelsHigh, Int(ceil(sampleRect.maxY * scaleY))) | |
| var firstActiveX: Int? | |
| var lastActiveX: Int? | |
| for x in minX..<maxX { | |
| var hasIndicatorPixel = false | |
| for y in minY..<maxY { | |
| guard let color = bitmap.colorAt(x: x, y: y), | |
| let rgb = color.usingColorSpace(.sRGB), | |
| rgb.alphaComponent > 0.05 else { continue } | |
| let alpha = min(max(rgb.alphaComponent, 0), 1) | |
| let red = rgb.redComponent * alpha | |
| let green = rgb.greenComponent * alpha | |
| let blue = rgb.blueComponent * alpha | |
| let high = max(red, green, blue) | |
| guard high > 0.01 else { continue } | |
| let low = min(red, green, blue) | |
| if (high - low) / high > 0.4 { | |
| hasIndicatorPixel = true | |
| break | |
| } | |
| } | |
| guard hasIndicatorPixel else { continue } | |
| if firstActiveX == nil { | |
| firstActiveX = x | |
| } | |
| lastActiveX = x | |
| } | |
| guard let firstActiveX, let lastActiveX else { return nil } | |
| return (CGFloat(firstActiveX) / scaleX)...(CGFloat(lastActiveX + 1) / scaleX) | |
| } | |
| @MainActor | |
| private func highSaturationRange(in view: NSView, sampleRect: NSRect) -> ClosedRange<CGFloat>? { | |
| let integralBounds = view.bounds.integral | |
| guard let bitmap = view.bitmapImageRepForCachingDisplay(in: integralBounds) else { return nil } | |
| bitmap.size = integralBounds.size | |
| view.cacheDisplay(in: integralBounds, to: bitmap) | |
| let scaleX = CGFloat(bitmap.pixelsWide) / max(1, integralBounds.width) | |
| let scaleY = CGFloat(bitmap.pixelsHigh) / max(1, integralBounds.height) | |
| let minX = max(0, Int(floor(sampleRect.minX * scaleX))) | |
| let maxX = min(bitmap.pixelsWide, Int(ceil(sampleRect.maxX * scaleX))) | |
| let minY = max(0, Int(floor(sampleRect.minY * scaleY))) | |
| let maxY = min(bitmap.pixelsHigh, Int(ceil(sampleRect.maxY * scaleY))) | |
| var firstActiveX: Int? | |
| var lastActiveX: Int? | |
| for x in minX..<maxX { | |
| guard hasSaturatedPixel(in: bitmap, x: x, minY: minY, maxY: maxY) else { continue } | |
| if firstActiveX == nil { firstActiveX = x } | |
| lastActiveX = x | |
| } | |
| guard let firstActiveX, let lastActiveX else { return nil } | |
| return (CGFloat(firstActiveX) / scaleX)...(CGFloat(lastActiveX + 1) / scaleX) | |
| } | |
| private func hasSaturatedPixel(in bitmap: NSBitmapImageRep, x: Int, minY: Int, maxY: Int) -> Bool { | |
| for y in minY..<maxY { | |
| guard let color = bitmap.colorAt(x: x, y: y), | |
| let rgb = color.usingColorSpace(.sRGB), | |
| rgb.alphaComponent > 0.05 else { continue } | |
| let alpha = min(max(rgb.alphaComponent, 0), 1) | |
| let red = rgb.redComponent * alpha | |
| let green = rgb.greenComponent * alpha | |
| let blue = rgb.blueComponent * alpha | |
| let high = max(red, green, blue) | |
| guard high > 0.01 else { continue } | |
| let low = min(red, green, blue) | |
| if (high - low) / high > 0.4 { return true } | |
| } | |
| return false | |
| } |
1 participant
Summary
Validation
testActiveTabIndicatorIgnoresAnimatedSelectionTransactionsfailed with indicator lowerBound11.0, expected>44.0.swift test --package-path vendor/bonsplit --filter BonsplitTests.testActiveTabIndicatorpassed on the cmux cloud Mac.Parent PR
main.Need help on this PR? Tag
@codesmithwith what you need. Autofix is disabled.Summary by cubic
Fixes the active tab indicator drifting between tabs. The indicator now ignores inherited animations and snaps to the selected tab, and it scrolls out with its tab during horizontal scroll.
TabBarViewusing a local transaction to prevent drift during animated selection changes.Written for commit 0699ba0. Summary will update on new commits. Review in cubic
Summary by CodeRabbit
Bug Fixes
Tests