Repository navigation
Normalize browser navigation icon sizing - #6208
lawrencecchen wants to merge 1 commit into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughIntroduces a ChangesNavigation Icon Raster Sizing Refactor
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Poem
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error)
✅ Passed checks (20 passed)
✨ 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 normalizes browser navigation icon sizing by introducing a
Confidence Score: 4/5Safe to merge — the change is a well-scoped visual normalization with 9 passing unit tests and no behavioural changes to navigation logic. The implementation is clean and the test coverage is thorough. The only items worth addressing are a computed static var that would be better as a static let, and a missing doc comment on a method whose parameter is intentionally unused — neither affects correctness or runtime behaviour. Sources/Panels/BrowserChromeMetrics.swift — the two style suggestions above both apply here. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[tabBarFontSize] --> B[BrowserChromeMetrics.scale]
D[BrowserNavigationToolbarIcon.allCases] --> E[measuredReferenceMaxEdge per case]
E --> F["max() → 15pt from reload height"]
F --> G[navigationIconReferenceRasterSize]
G --> C[navigationIconRasterSize = scaled base]
B --> C
C --> H["navigationIconRasterSize(for: .back)"]
C --> I["navigationIconRasterSize(for: .forward)"]
C --> J["navigationIconRasterSize(for: reloadOrStop)"]
H --> K[back button Image]
I --> L[forward button Image]
J --> M[reload / stop button Image]
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
flowchart TD
A[tabBarFontSize] --> B[BrowserChromeMetrics.scale]
D[BrowserNavigationToolbarIcon.allCases] --> E[measuredReferenceMaxEdge per case]
E --> F["max() → 15pt from reload height"]
F --> G[navigationIconReferenceRasterSize]
G --> C[navigationIconRasterSize = scaled base]
B --> C
C --> H["navigationIconRasterSize(for: .back)"]
C --> I["navigationIconRasterSize(for: .forward)"]
C --> J["navigationIconRasterSize(for: reloadOrStop)"]
H --> K[back button Image]
I --> L[forward button Image]
J --> M[reload / stop button Image]
Reviews (1): Last reviewed commit: "Normalize browser navigation icon sizing" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| private static var navigationIconReferenceRasterSize: CGFloat { | ||
| BrowserNavigationToolbarIcon.allCases | ||
| .map(\.measuredReferenceMaxEdge) | ||
| .max() ?? 12 | ||
| } |
There was a problem hiding this comment.
measuredReferenceSize values are hardcoded constants, so navigationIconReferenceRasterSize always returns 15 and never changes. A computed static var re-traverses allCases, allocates an intermediate array, and re-finds the max on every access to navigationIconRasterSize. Using static let computes the value once (lazily on first use) and is clearer about the intent.
| private static var navigationIconReferenceRasterSize: CGFloat { | |
| BrowserNavigationToolbarIcon.allCases | |
| .map(\.measuredReferenceMaxEdge) | |
| .max() ?? 12 | |
| } | |
| private static let navigationIconReferenceRasterSize: CGFloat = | |
| BrowserNavigationToolbarIcon.allCases | |
| .map(\.measuredReferenceMaxEdge) | |
| .max() ?? 12 |
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| func navigationIconRasterSize(for _: BrowserNavigationToolbarIcon) -> CGFloat { | ||
| return navigationIconRasterSize | ||
| } |
There was a problem hiding this comment.
for: parameter is silently discarded — every call site returns navigationIconRasterSize regardless of which icon is passed. A brief doc comment clarifying this is intentional (all icons share one box so the reload↔stop swap doesn't resize) would prevent future readers from assuming per-icon sizing is already in effect and that changes to the enum will automatically produce different sizes.
| func navigationIconRasterSize(for _: BrowserNavigationToolbarIcon) -> CGFloat { | |
| return navigationIconRasterSize | |
| } | |
| /// Returns the shared raster size for every navigation toolbar icon. | |
| /// All variants deliberately share one box so the reload↔stop swap does | |
| /// not cause the button to resize. If per-icon sizing is added in the | |
| /// future, differentiate here. | |
| func navigationIconRasterSize(for _: BrowserNavigationToolbarIcon) -> CGFloat { | |
| return navigationIconRasterSize | |
| } |
lawrencecchen
added
the
stale-revisit
This branch was successfully deployed
2 participants
Summary
Testing
xcodebuild -project cmux.xcodeproj -scheme cmux-unit -configuration Debug -destination 'platform=macOS' -derivedDataPath /tmp/cmux-btnsz test -only-testing:cmuxTests/BrowserChromeMetricsTestspassed, 9 Swift Testing tests.\n-./scripts/reload-cloud.sh --tag btnszpassed.\n- Launched tagged app, opened browser surface, verified back/forward/reload toolbar.\n- Navigated to a deliberately hanging local URL and verified reload flipped to the stop xmark toolbar variant.Need help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.Note
Low Risk
Localized browser chrome layout and metrics only; minor default-scale visual tweak (nav raster 12→15) with no auth, data, or networking impact.
Overview
Browser back, forward, reload, and stop controls now share one scaled raster size derived from measured SF Symbol bounds, so the reload/stop toggle and chevrons no longer shift layout.
A new
BrowserNavigationToolbarIcontype centralizes symbol names, per-variant reference measurements, andreloadOrStop(isLoading:).BrowserChromeMetricsdropsnavigationIconFontSizein favor ofnavigationIconRasterSize(reference base 15, the max intrinsic edge across the four glyphs) plusnavigationIconRasterSize(for:). The address bar button bar routes all three nav buttons through that path withcmuxSymbolRasterSize.Tests rename the legacy fixture to reference sizes, expect 15 for navigation raster at scale 1, and add coverage for measured bounds and a single raster size across variants.
Reviewed by Cursor Bugbot for commit ffa5bf0. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by cubic
Normalized browser navigation icon sizing so back, forward, reload, and stop share one consistent raster size. Prevents size jumps when reload switches to stop and keeps the toolbar scale aligned with the tab bar font size.
Written for commit ffa5bf0. Summary will update on new commits.
Summary by CodeRabbit
Refactor
Tests