Restore sidebar workspace icon size#4973
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughCentralizes browser-stack sidebar sizing into ExtensionBrowserStackSidebarMetrics and updates tile/row views to use it; aligns header/icon/frame sizing with shared HeaderChromeControlMetrics, adjusts titlebar style presets, removes custom sidebar glyph, and updates tests to assert the new metrics. ChangesBrowser Stack Sidebar Metrics Refactoring
Header & Titlebar Metrics Update
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 14❌ Failed checks (1 warning, 13 inconclusive)
✅ 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 restores the v0.64.10 window-chrome sizing: sidebar workspace icons return to 28 pt, the classic titlebar controls go back to 15 pt icons in 24 pt lanes with semibold weight, the sidebar-toggle glyph switches back to the
Confidence Score: 5/5Safe to merge — all changes are layout constants, static metric enums, and hit-region plumbing with no auth, data, or concurrency impact. The change is scoped entirely to visual chrome metrics and hit-region sizing. Restored constants are validated by deterministic unit tests locking in exact widths, padding values, and button counts. The one fragility flagged (barVerticalPadding derivation without a guard) is not a current bug given the locked constants, and the tests would catch a regression before it shipped. Sources/WindowChromeMetrics.swift — the derived barVerticalPadding formula has no guard against the padding going negative if buttonSize is later bumped past secondaryBarHeight. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
HCM["HeaderChromeControlMetrics\nbuttonSize=24 iconSize=15\ncornerRadius=8 weight=semibold"]
RSC["RightSidebarChromeMetrics\nbarVerticalPadding=(28-24)/2=2\nheaderIconSize=15 headerControlSize=24"]
EBSM["ExtensionBrowserStackSidebarMetrics\niconSize=28 tileHeight=54\nrowHeight(compact)=34/38"]
TCS_classic["TitlebarControlsStyle.classic\nspacing=10 buttonSize=24 iconSize=15"]
FC["fittingConfig()\nscales down spacing/button/icon\nto fit hostWidth=124"]
SIDEBAR_CFG["titlebarControlsSidebarChromeConfig()\nfor style → fitted config"]
TCV["TitlebarControlsView\nactionSlots=[sidebar,notif,newTab]\nstyleConfigOverride=fittedConfig"]
HIT["TitlebarControlsHitRegions\nbuttonCount=3\nbuttonXRanges(config, buttonCount)"]
HCM --> RSC
HCM --> TCS_classic
TCS_classic --> FC
FC --> SIDEBAR_CFG
SIDEBAR_CFG --> TCV
SIDEBAR_CFG --> HIT
EBSM --> ContentView["ContentView\nextensionBrowserStackRow"]
Reviews (11): Last reviewed commit: "fix: avoid crashing on sidebar icon padd..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
|
+1 for configurable sidebar icon size — same need on the icon side. The workspace icons are hard to distinguish at the current fixed size when a lot of tabs are open. Would love to see this land alongside sidebar font sizing. |
|
@lugfug I kept this PR scoped to the visual regression: the default workspace sidebar icon size is restored to the v0.64.10-era 28pt size, and the sizing now comes from a shared metrics path so a future preference can drive it cleanly. I did not add a new user-facing icon-size setting here because that changes the settings/config surface and needs row-height/density behavior designed separately from this regression fix. |
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 8dc7bbd. Configure here.
2 participants
Summary
sidebar.left) instead of the newer custom drawn glyphRepro / Root Cause
Used local source diffing against
v0.64.10, plus the user-provided screenshots. The shippedv0.64.10titlebar controls usediconSize: 15,buttonSize: 24,spacing: 10, semibold symbols, and SF Symbolsidebar.leftfor the sidebar toggle. The later focus-history/titlebar chrome refactor shrank classic to compact shared values (12pticons in20ptlanes, regular weight) and replaced the sidebar toggle with a smaller custom glyph.This fix restores the release-era titlebar icon/lane scale at the shared chrome metric root and restores the original SF Symbol path for the sidebar toggle, so the main titlebar controls and right-sidebar header controls stay in sync without a one-off frame override.
No cloud-mac video was recorded.
Testing
Launch
Required command after approval:
CMUX_SKIP_ZIG_BUILD=1 ./scripts/reload.sh --tag fix-sidebar-icons-restore-size --launchSummary by CodeRabbit
Note
Low Risk
Mostly visual layout and hit-region metrics with regression tests; no auth, data, or security paths touched.
Overview
Restores pre-refactor chrome sizing for extension sidebar workspace tiles and shared header/titlebar controls, and aligns minimal-mode sidebar titlebar behavior with a three-button chrome row.
Extension sidebar: Introduces
ExtensionBrowserStackSidebarMetrics(28pt icons, fixed tile/row heights, padding and corner radius helpers with vertical padding derived from icon height) so tile and list rows no longer use scattered magic numbers; list rows always use the 28pt icon size instead of smaller compact icons.Shared chrome:
HeaderChromeControlMetricsmoves to 24×15pt buttons/icons with 8pt corners;HeaderChromeIconStyleuses semibold weight and drops the custom sidebar glyph stroke. Right-sidebar header metrics follow the shared titlebar sizes.Titlebar: Style presets get larger spacing and icon/button sizes; sidebar toggle returns to SF Symbol
sidebar.left.TitlebarControlsViewsupports configurableactionSlotsandstyleConfigOverride; minimal/hidden sidebar chrome uses only toggle sidebar, notifications, new tab viasidebarChromeSlots, withfittingConfigshrinking controls to fit the 124pt host width and updated hit-test/drag regions for three buttons. Focus-history controls remain on the full titlebar path where applicable.Tests: Assertions updated for new layout widths, metrics, and three-button hit regions.
Reviewed by Cursor Bugbot for commit b7ceab1. Bugbot is set up for automated code reviews on this repo. Configure here.