Repository navigation
Tune Bonsplit tab bar action lane - #3406
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughA new DEBUG-only "Bonsplit Tab Bar Debug" window is introduced to tune split button backdrop visual parameters via UserDefaults, alongside refresh methods in Workspace and TabManager that propagate configuration changes across all main-window tab managers. Changes
Sequence DiagramsequenceDiagram
participant User
participant DebugWindow as BonsplitTabBarDebugWindowController
participant Defaults as UserDefaults
participant AppDelegate
participant TabMgr as TabManager
participant Workspace
User->>DebugWindow: Change debug setting (slider/stepper)
DebugWindow->>Defaults: Write updated value
DebugWindow->>AppDelegate: allMainWindowTabManagersForDebug()
AppDelegate-->>DebugWindow: [TabManager, TabManager, ...]
loop For each TabManager
DebugWindow->>TabMgr: refreshSplitButtonBackdropEffect()
TabMgr->>Workspace: refreshSplitButtonBackdropEffect()
Workspace->>Workspace: bonsplitSplitButtonBackdropEffect(defaults)
Workspace->>Defaults: Read current tuning values
Defaults-->>Workspace: separator/content/solid values
Workspace->>Workspace: Update bonsplitController.appearance.splitButtonBackdropEffect
end
DebugWindow->>DebugWindow: Render updated preview with new effect
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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 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. Review rate limit: 0/8 reviews remaining, refill in 3 minutes and 47 seconds.Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 96f1934f56
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| var renderIdentity: String { | ||
| "\(id)-\(chromeHex)-\(tabBarHex)-\(splitButtonBackdropHex)-\(paneHex)-\(borderHex)-\(String(format: "%.3f", opacity))-\(String(format: "%.1f", effect.fadeWidth))-\(String(format: "%.1f", effect.contentFadeWidth))-\(String(format: "%.1f", effect.solidWidth))-\(String(format: "%.2f", effect.fadeRampStartFraction))-\(String(format: "%.2f", effect.leadingOpacity))-\(String(format: "%.2f", effect.trailingOpacity))-\(String(format: "%.2f", effect.contentOcclusionFraction))-\(effect.masksTabContent ? 1 : 0)" | ||
| "\(id)-\(chromeHex)-\(tabBarHex)-\(splitButtonBackdropHex)-\(paneHex)-\(borderHex)-\(String(format: "%.3f", opacity))-\(String(format: "%.1f", effect.fadeWidth))-\(String(format: "%.1f", effect.contentFadeWidth))-\(String(format: "%.1f", effect.solidWidth))-\(String(format: "%.1f", effect.solidSurfaceWidthAdjustment))-\(String(format: "%.2f", effect.fadeRampStartFraction))-\(String(format: "%.2f", effect.leadingOpacity))-\(String(format: "%.2f", effect.trailingOpacity))-\(String(format: "%.2f", effect.contentOcclusionFraction))-\(effect.masksTabContent ? 1 : 0)" |
There was a problem hiding this comment.
When separatorFadeWidth changes (via the new Bonsplit Tab Bar debug controls), TabBarBackdropLabSample does not refresh because its update hooks depend on variant.renderIdentity, and that identity string still omits effect.separatorFadeWidth. In this case the BonsplitController keeps the old configuration, so the lab preview can show stale visuals even though the setting changed.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 58bec3c by including separatorFadeWidth in the lab render identity.
— Claude Code
Greptile SummaryThis PR bumps Confidence Score: 4/5Safe to merge; only P2 style findings present. No logic or correctness issues were found. The two findings are both P2: debug menu entries are out of alphabetical order per CLAUDE.md policy, and the new debug window uses bare string literals where the adjacent TabBarBackdropLab window uses String(localized:). Neither affects runtime behavior. Sources/cmuxApp.swift — debug menu ordering and localization of new window/view strings. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[BonsplitTabBarDebugView\nSlider / Stepper] -->|setSeparatorFadeWidth\nsetContentFadeWidth\nsetSolidSurfaceWidthAdjustment| B[AppStorage / UserDefaults]
B --> C[refreshLiveWorkspaces]
C --> D[TabManager\nrefreshSplitButtonBackdropEffect]
D --> E[Workspace\nrefreshSplitButtonBackdropEffect]
E --> F[Workspace.bonsplitSplitButtonBackdropEffect\nreads UserDefaults via BonsplitTabBarDebugSettings]
F --> G[BonsplitConfiguration\n.appearance.splitButtonBackdropEffect updated]
G --> H[bonsplitController.configuration =]
B --> I[BonsplitTabBarDebugView body\ncurrentTuningDescription re-rendered]
Reviews (1): Last reviewed commit: "Update Bonsplit action lane fixes" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| Button("Bonsplit Tab Bar Debug…") { | ||
| BonsplitTabBarDebugWindowController.shared.show() | ||
| } |
There was a problem hiding this comment.
Per CLAUDE.md, "Debug > Debug Windows" entries must be alphabetical with no dividers. "Bonsplit Tab Bar Debug…" (B) is inserted after "Split Button Layout Debug…" (S), but B < S so it should appear before the Split Button entry. The same transposition appears in DebugWindowControlsView around line 1755, where it's inserted after PDFPreviewChromeDebugWindowController (P) but before TabBarBackdropLab (T) — again 'B' belongs earlier in that list.
Context Used: CLAUDE.md (source)
There was a problem hiding this comment.
Fixed in 58bec3c by moving the Bonsplit debug window entry up with the other B entries.
— Claude Code
| defer: false | ||
| ) | ||
| window.title = "Bonsplit Tab Bar Debug" | ||
| window.titleVisibility = .visible |
There was a problem hiding this comment.
CLAUDE.md requires all user-facing strings to be localized with String(localized:defaultValue:). The window title here is a bare literal, while the adjacent TabBarBackdropLabWindowController (line 3552) uses String(localized: "debug.tabBarBackdropLab.title", defaultValue: "Tab Bar Backdrop Lab"). BonsplitTabBarDebugView likewise has bare literals for every Text, GroupBox, Button, and Stepper label ("Bonsplit Tab Bar", "Action Lane Geometry", "Reset", "Fine tune", etc.).
Context Used: CLAUDE.md (source)
There was a problem hiding this comment.
Fixed in 58bec3c by wrapping the new debug-window labels with String(localized:defaultValue:) and adding catalog keys.
— Claude Code
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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/cmuxApp.swift`:
- Around line 3486-3495: refreshLiveWorkspaces() currently only refreshes two
TabManager instances (activeTabManagerForCommands() and tabManager), missing
secondary main windows; change it to iterate AppDelegate's per-window registry
(mainWindowContexts) and collect each context's TabManager (the ones created via
createMainWindow), deduplicate by ObjectIdentifier like the existing seen set,
and call manager.refreshSplitButtonBackdropEffect() for every manager found;
replace the current managers array construction with enumerating
AppDelegate.shared?.mainWindowContexts (or equivalent) to gather all live
TabManager instances before the refresh loop.
- Around line 386-388: Replace all raw English UI strings introduced for the
Bonsplit tab-bar debug UI (e.g., the Button label "Bonsplit Tab Bar Debug…", the
window title set in BonsplitTabBarDebugWindowController, group titles, slider
labels, and action button titles used in that controller and related views) with
localized variants using String(localized: "key.name", defaultValue: "English
text"); keep the current English text as the defaultValue and pick meaningful
localization keys (e.g., bonsplit.debug.button.title,
bonsplit.debug.window.title, bonsplit.debug.group.general,
bonsplit.debug.slider.opacity, bonsplit.debug.action.reset) so they will be
extracted by xcstrings. Apply the same replacement pattern for the other raw
strings flagged in the ranges around lines 1758-1760 and 3345-3538.
In `@Sources/Workspace.swift`:
- Around line 88-93: The currentValue(defaults:) function and similar
UserDefaults reads (e.g., the use of defaults.double(forKey: key) in resolved
paths) let debug tuning overrides affect release builds; change the logic so
that outside of DEBUG the function always returns the baked defaultValue (or
call site uses the baked value), e.g. gate the UserDefaults read with `#if` DEBUG
inside currentValue(defaults:) (or remove the override read and only allow it
when compiled with DEBUG) so non-DEBUG builds never read debugBonsplitTabBar…
keys and instead return defaultValue/resolved(defaultValue).
- Around line 163-176: currentTuningDescription(defaults:) currently builds its
effect from UserDefaults.standard causing inconsistency with the injected
defaults; change it to use the defaults parameter by calling
bonsplitSplitButtonBackdropEffect(defaults: defaults) (instead of
bonsplitSplitButtonBackdropEffect()) and add a new nonisolated overload of
bonsplitSplitButtonBackdropEffect(defaults: UserDefaults = .standard) ->
BonsplitConfiguration.Appearance.SplitButtonBackdropEffect (placed outside the
shown range) that constructs the effect using the passed-in defaults for
contentFadeWidth, solidSurfaceWidthAdjustment, and separatorFadeWidth (and the
same literal values for the other fields) so tests and alternate suites observe
the injected defaults.
In `@vendor/bonsplit`:
- Line 1: The vendor/bonsplit gitlink points to commit
cc0f548608843462b131f5e69856515181e3497b that is not present on the Bonsplit
remote main; before merging update the remote by pushing that commit to
https://github.com/manaflow-ai/bonsplit (or fast-forward main there) so the
submodule commit becomes reachable, or else reset the submodule pointer in the
parent repo to a commit that already exists on the remote; ensure the commit
hash cc0f5486 is present on the remote main branch and verify by cloning the
repo and confirming the gitlink resolves.
🪄 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: 5c1b6925-dc1d-49d5-82e5-5c091267c1a0
📒 Files selected for processing (6)
Sources/TabManager.swiftSources/Workspace.swiftSources/cmuxApp.swiftdogfood/directory-actions/README.mddogfood/directory-actions/many-tab-actions/.cmux/cmux.jsonvendor/bonsplit
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/AppDelegate.swift`:
- Line 5190: The method allMainWindowTabManagersForDebug iterates
mainWindowContexts.values and calls resolvedWindow(for:) which can mutate
mainWindowContexts; to avoid mutation-during-enumeration, first take a snapshot
like let contexts = Array(mainWindowContexts.values) and iterate contexts
instead, then call resolvedWindow(for:) and return the tabManager for each
resolved context (use the existing resolvedWindow(for:) and tabManager symbols).
In `@Sources/BonsplitTabBarDebug.swift`:
- Around line 271-278: Add unified debug logging calls using cmuxDebugLog(...)
for all user actions that change tab/split tuning: inside the Reset Button
action add cmuxDebugLog indicating reset to defaults (include values from
BonsplitTabBarDebugSettings.defaultSeparatorFadeWidth, defaultContentFadeWidth,
defaultSolidSurfaceWidthAdjustment), inside the Copy Config Button action add
cmuxDebugLog that a copy-to-pasteboard occurred (use
BonsplitTabBarDebugSettings.copyCurrentTuningToPasteboard() as context), and
also add cmuxDebugLog calls in the tuning-value change handlers referenced
around functions/setters setSeparatorFadeWidth, setContentFadeWidth, and
setSolidSurfaceWidthAdjustment (and the code block around lines 291-304) to log
new values; ensure messages are concise and only compiled in DEBUG builds per
guideline.
- Line 338: Replace the hard-coded label Text("\(setting.format(resolvedValue))
px") with a localized string that includes the formatted value and the localized
unit; construct the UI string using String(localized: "bonsplit.slider.value",
defaultValue: "%@ px") (or similar key) and inject setting.format(resolvedValue)
into that localized format so the full displayed text is localized; update the
Text call to use that localized string and add the localization key/English
default to your strings file.
🪄 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: a5beaa1b-2cf9-4b82-93c7-411378a30956
📒 Files selected for processing (7)
GhosttyTabs.xcodeproj/project.pbxprojResources/Localizable.xcstringsSources/AppDelegate.swiftSources/BonsplitTabBarDebug.swiftSources/Workspace.swiftSources/cmuxApp.swiftvendor/bonsplit
✅ Files skipped from review due to trivial changes (3)
- Resources/Localizable.xcstrings
- vendor/bonsplit
- GhosttyTabs.xcodeproj/project.pbxproj
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
This branch was successfully deployed
1 participant
Updates vendor/bonsplit to the action-lane layout fixes from manaflow-ai/bonsplit#113 and applies the final tab bar tuning.
Changes:
Verification:
Summary by CodeRabbit
New Features
Documentation