Configurable surface tab bar font size#2645
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
This review could not be run because your cubic account has exceeded the monthly review limit. If you need help restoring access, please contact contact@cubic.dev. |
📝 WalkthroughWalkthroughAdds a new Changes
Sequence Diagram(s)sequenceDiagram
participant Config as GhosttyConfig
participant Workspace as Workspace
participant Bonsplit as Bonsplit/Renderer
Config->>Workspace: load() -> surfaceTabBarFontSize
Workspace->>Workspace: bonsplitAppearance(from:, tabTitleFontSize:)
Workspace->>Bonsplit: compute next background hex + tabTitleFontSize
Bonsplit-->>Workspace: current appearance
Workspace->>Workspace: compare current vs next (background, fontSize)
alt changes needed
Workspace->>Bonsplit: apply updated chrome (background, tab font size)
Bonsplit-->>Workspace: confirmation
else no-op
Workspace-->>Config: no-op (early return)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 SummaryAdds a Confidence Score: 5/5Safe to merge; all changes are additive and the config-reload path is correct. The parsing, defaults, and snapshot-refresh flow are all implemented correctly and consistently with existing patterns. No P0/P1 issues found — only a minor naming suggestion on the No files require special attention. Important Files Changed
Sequence DiagramsequenceDiagram
participant U as Config file
participant G as GhosttyConfig
participant NC as NotificationCenter
participant S as SidebarTabItemSettingsStore
participant SS as SidebarTabItemSettingsSnapshot
participant FM as SidebarWorkspaceFontMetrics
U->>G: sidebar-font-size = N (parsed on disk load)
G->>NC: post .ghosttyConfigDidReload
NC->>S: observer fires (MainActor Task)
S->>G: invalidateLoadCache()
S->>SS: SidebarTabItemSettingsSnapshot(defaults:)
SS->>G: GhosttyConfig.load() [cache miss → loadFromDisk]
G-->>SS: sidebarFontSize: CGFloat
SS->>FM: SidebarWorkspaceFontMetrics(sidebarFontSize:)
FM-->>SS: primary / description / secondary / tertiary
SS-->>S: new snapshot (fontMetrics updated)
S->>S: guard nextSnapshot != snapshot → publish
Reviews (1): Last reviewed commit: "feat: configurable sidebar tab font size..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Sources/ContentView.swift
Outdated
|
|
||
| private struct SidebarWorkspaceFontMetrics: Equatable { | ||
| let primary: CGFloat | ||
| let description: CGFloat |
There was a problem hiding this comment.
description property name shadows Swift convention
Naming a stored CGFloat property description can confuse readers because description is the canonical CustomStringConvertible / NSObject debug property. Any call-site reading fontMetrics.description looks like a debug-string access rather than a font size. A name like descriptionLabel or subtitle would be clearer.
| let description: CGFloat | |
| let subtitleLabel: CGFloat |
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!
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
Sources/GhosttyConfig.swift (1)
246-249: Parsing implementation is correct and consistent.The
sidebar-font-sizeconfig key parsing follows the exact same pattern asfont-size(lines 242-245), which is good for consistency.Optional: Consider adding bounds validation.
While not required for this implementation, you might consider adding bounds checking similar to
sidebar-tint-opacity(line 310) to prevent extreme values. For example:case "sidebar-font-size": if let size = Double(value) { sidebarFontSize = CGFloat(min(max(size, 6.0), 72.0)) }This is purely optional since the existing
font-sizealso lacks validation and config files are user-controlled.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/GhosttyConfig.swift` around lines 246 - 249, The current parsing for the "sidebar-font-size" config key correctly sets sidebarFontSize but lacks bounds validation; update the switch case handling "sidebar-font-size" in GhosttyConfig to clamp the parsed Double to a safe range (e.g., min 6.0, max 72.0) before assigning to sidebarFontSize (similar pattern as sidebar-tint-opacity), so extreme values are prevented while keeping the existing parsing behavior.Sources/ContentView.swift (1)
9894-9901: Avoid another sharedGhosttyConfigcache flush on reload.
Sources/WorkspaceContentView.swiftalready invalidatesGhosttyConfigon.ghosttyConfigDidReload. Clearing it again here means one reload can fan out into repeated cache clears/loads across windows and sidebars. Refreshing from the already-reloaded config, or centralizing invalidation once at the source, would avoid that extra churn.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/ContentView.swift` around lines 9894 - 9901, The handler registered in ghosttyConfigObserver is redundantly calling GhosttyConfig.invalidateLoadCache() on .ghosttyConfigDidReload, causing duplicate cache clears; remove the GhosttyConfig.invalidateLoadCache() call from the NotificationCenter.default.addObserver closure (keep the Task `@MainActor` and self?.refreshSnapshot()) so this view only refreshes from the already-reloaded config and lets WorkspaceContentView or the centralized reload producer perform the single cache invalidation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@Sources/ContentView.swift`:
- Around line 9894-9901: The handler registered in ghosttyConfigObserver is
redundantly calling GhosttyConfig.invalidateLoadCache() on
.ghosttyConfigDidReload, causing duplicate cache clears; remove the
GhosttyConfig.invalidateLoadCache() call from the
NotificationCenter.default.addObserver closure (keep the Task `@MainActor` and
self?.refreshSnapshot()) so this view only refreshes from the already-reloaded
config and lets WorkspaceContentView or the centralized reload producer perform
the single cache invalidation.
In `@Sources/GhosttyConfig.swift`:
- Around line 246-249: The current parsing for the "sidebar-font-size" config
key correctly sets sidebarFontSize but lacks bounds validation; update the
switch case handling "sidebar-font-size" in GhosttyConfig to clamp the parsed
Double to a safe range (e.g., min 6.0, max 72.0) before assigning to
sidebarFontSize (similar pattern as sidebar-tint-opacity), so extreme values are
prevented while keeping the existing parsing behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e74bfb9e-afa6-4f62-81f8-5d3d0ccb29e3
📒 Files selected for processing (2)
Sources/ContentView.swiftSources/GhosttyConfig.swift
austinywang
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Sources/Workspace.swift`:
- Around line 6778-6785: The code passes surfaceTabBarFontSize directly into
BonsplitConfiguration.Appearance via bonsplitAppearance, allowing zero or
negative values from config to make labels disappear; sanitize by normalizing
surfaceTabBarFontSize to a positive fallback before creating
BonsplitConfiguration.Appearance (e.g., clamp or use max(1,
surfaceTabBarFontSize) or a defined minimum) and use that sanitized value
wherever surfaceTabBarFontSize is read/stored (including the other appearance
construction block around lines 6796-6825) so Bonsplit always receives a valid
positive font size.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0d7f07a3-7169-493f-b419-dc2182b08e6f
📒 Files selected for processing (3)
Sources/GhosttyConfig.swiftSources/Workspace.swiftvendor/bonsplit
✅ Files skipped from review due to trivial changes (2)
- vendor/bonsplit
- Sources/GhosttyConfig.swift
| private static func bonsplitAppearance( | ||
| from backgroundColor: NSColor, | ||
| backgroundOpacity: Double | ||
| backgroundOpacity: Double, | ||
| tabTitleFontSize: CGFloat = 11 | ||
| ) -> BonsplitConfiguration.Appearance { | ||
| BonsplitConfiguration.Appearance( | ||
| tabTitleFontSize: tabTitleFontSize, | ||
| splitButtonTooltips: Self.currentSplitButtonTooltips(), |
There was a problem hiding this comment.
Sanitize surfaceTabBarFontSize before it reaches Bonsplit.
This value is now written straight into BonsplitConfiguration.Appearance. Since Sources/GhosttyConfig.swift parses numeric values directly, a config like surface-tab-bar-font-size = 0 or -1 will make the tab labels unreadable/disappear on first paint and on reload. Please normalize to a positive fallback before comparing or storing it.
🩹 Suggested guardrail
+ private static func sanitizedSurfaceTabBarFontSize(_ size: CGFloat) -> CGFloat {
+ guard size > 0 else { return 11 }
+ return size
+ }
+
private static func bonsplitAppearance(
from backgroundColor: NSColor,
backgroundOpacity: Double,
tabTitleFontSize: CGFloat = 11
) -> BonsplitConfiguration.Appearance {
+ let tabTitleFontSize = Self.sanitizedSurfaceTabBarFontSize(tabTitleFontSize)
BonsplitConfiguration.Appearance(
tabTitleFontSize: tabTitleFontSize,
splitButtonTooltips: Self.currentSplitButtonTooltips(),
enableAnimations: false,
chromeColors: .init(
@@
- let nextTabTitleFontSize = config.surfaceTabBarFontSize
+ let nextTabTitleFontSize = Self.sanitizedSurfaceTabBarFontSize(config.surfaceTabBarFontSize)Also applies to: 6796-6825
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Sources/Workspace.swift` around lines 6778 - 6785, The code passes
surfaceTabBarFontSize directly into BonsplitConfiguration.Appearance via
bonsplitAppearance, allowing zero or negative values from config to make labels
disappear; sanitize by normalizing surfaceTabBarFontSize to a positive fallback
before creating BonsplitConfiguration.Appearance (e.g., clamp or use max(1,
surfaceTabBarFontSize) or a defined minimum) and use that sanitized value
wherever surfaceTabBarFontSize is read/stored (including the other appearance
construction block around lines 6796-6825) so Bonsplit always receives a valid
positive font size.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix is kicking off a free cloud agent to fix this issue. This run is complimentary, but you can enable autofix for all future PRs in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 9494732. Configure here.
| let initialSurfaceTabBarFontSize = GhosttyConfig.load().surfaceTabBarFontSize | ||
| let appearance = Self.bonsplitAppearance( | ||
| from: GhosttyApp.shared.defaultBackgroundColor, | ||
| backgroundOpacity: GhosttyApp.shared.defaultBackgroundOpacity |
There was a problem hiding this comment.
Refactoring left behind unused method overload
Low Severity
The applyGhosttyChrome(backgroundColor:backgroundOpacity:reason:) overload is now dead code. Its previous caller, applyGhosttyChrome(from config:), was refactored to inline logic for tabTitleFontSize updates, leaving this overload unused and missing font size handling.
Reviewed by Cursor Bugbot for commit 9494732. Configure here.
…nt-size # Conflicts: # vendor/bonsplit
Lost-update race: persistWindowGeometry and saveSessionSnapshot both wrote the displayConfigurations map with their own copies, racing on UserDefaults. Both writers now go through sessionPersistenceQueue and do read-merge-encode-write inside the queue (writePersistedWindowGeometry). Per-display visibility: hasSufficientVisibleFrame previously unioned intersections into a bbox, which falsely passed two disjoint slivers across two displays. It now checks each display individually so a window is only "reachable" if at least one display has a usable chunk. LRU cap: bound the per-display geometry map at 8 entries so users cycling through many docks/displays don't accumulate UserDefaults indefinitely. The just-written fingerprint is always preserved. Gate post-reconcile persist: handleScreenParametersDidChange now only persists the primary window's frame after the new display set if it actually reconciled the primary (skipping miniaturized/fullscreen windows), so a stale off-screen frame can't get baked into the new fingerprint. Restore tab title font size plumbing: the prior commit removed the surfaceTabBarFontSize wiring in Workspace.swift, silently reverting PR #2645. The bonsplit API still exposes tabTitleFontSize, so the plumbing is restored. Add regression tests for the per-display visibility check and the LRU eviction so the contracts are pinned. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
1 participant
Related to #2534
This PR switches the branch from the earlier sidebar-only work to the actual requested feature: configurable font sizing for the horizontal surface tab bar.
What changed:
surface-tab-bar-font-size = N11Notes:
Note
Low Risk
Low risk UI/appearance change that threads a new config value into Bonsplit; potential issues are limited to tab-strip sizing updates and workspace chrome refresh behavior.
Overview
Adds a new Ghostty config key,
surface-tab-bar-font-size(default11), and parses it intoGhosttyConfig.surfaceTabBarFontSize.Threads this value into Bonsplit appearance so surface tab titles use the configured font size, seeds new workspaces from the cached config, and updates
Workspace.applyGhosttyChrome(from:)to no-op (and optionally log) when neither the chrome background nor tab-title font size changed.Reviewed by Cursor Bugbot for commit 24adfd0. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by CodeRabbit
New Features
Bug Fixes