Repository navigation
Conversation
The left titlebar cluster has a sidebar toggle, but once the right sidebar is closed nothing in the window chrome reopens it — the only way back is the keyboard shortcut. - New RightSidebarToggleAccessoryViewController attached at layoutAttribute .right, reusing TitlebarControlButton and the titlebar controls style (the button follows the titlebarControlsStyle setting and resizes on change). - Toggles via the same AppDelegate.toggleRightSidebarInActiveMainWindow path as the Cmd+Opt+B shortcut; tooltip shows the bound shortcut. - Hidden in minimal mode and fullscreen alongside the existing controls cluster, and removed through the same accessory lifecycle. - Localized accessibility label + tooltip added for all 19 supported locales in Localizable.xcstrings. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
@lidge-jun is attempting to deploy a commit to the Manaflow Team on Vercel. A member of the Team first needs to authorize it. |
|
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:
📝 WalkthroughWalkthroughAdds a localized right-sidebar toggle button to the titlebar: a SwiftUI-hosted accessory that toggles the right sidebar, plus wiring to attach, hide/show, and remove the accessory in the titlebar accessory lifecycle. ChangesRight Sidebar Toggle Accessory
Sequence DiagramsequenceDiagram
participant TitlebarAccessoryController
participant RightSidebarToggleAccessoryViewController
participant AppDelegate
participant Window
TitlebarAccessoryController->>RightSidebarToggleAccessoryViewController: attach accessory to Window
RightSidebarToggleAccessoryViewController->>Window: present hosted SwiftUI button
RightSidebarToggleAccessoryViewController->>AppDelegate: toggleRightSidebarInActiveMainWindow(preferredWindow:)
TitlebarAccessoryController->>RightSidebarToggleAccessoryViewController: hide/show based on minimal/fullscreen state
TitlebarAccessoryController->>RightSidebarToggleAccessoryViewController: remove accessory when filtering identifiers
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (4 errors, 1 warning)
✅ Passed checks (16 passed)
✨ 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. Comment |
Contributor
Greptile SummaryAdds a trailing-edge titlebar button that toggles the right sidebar via the same
Confidence Score: 4/5The new button and its visibility/hide logic are sound; one issue in the session-restore path leaves the toggle button incorrectly visible until polling resolves. The session-restore binding uses a 40-iteration asyncAfter(0.25 s) polling loop as its only synchronization mechanism. When the right sidebar is already open at launch, setToggleHidden(true) is delayed for as long as it takes the fileExplorerState context to register — visibly incorrect titlebar state for up to 10 seconds on a slow restore path. All other mechanics (KVO-only resize, click-passthrough container, fullscreen/minimal hiding, localization) look correct. Sources/Update/UpdateTitlebarAccessory.swift — specifically scheduleBindRetryIfNeeded and the bindSidebarVisibilityIfNeeded retry path. Important Files Changed
Sequence DiagramsequenceDiagram
participant W as NSWindow
participant C as UpdateTitlebarAccessoryController
participant VC as RightSidebarToggleAccessoryViewController
participant AD as AppDelegate
participant FE as FileExplorerState
W->>C: attach(to:) / viewDidAppear
C->>VC: "addTitlebarAccessoryViewController (layoutAttribute = .right)"
VC->>VC: applyLayout()
VC->>VC: bindSidebarVisibilityIfNeeded()
alt context ready
VC->>AD: contextForMainTerminalWindow(window)
AD-->>VC: fileExplorerState
VC->>FE: subscribe $isVisible
FE-->>VC: "sidebarVisible -> setToggleHidden(sidebarVisible)"
else context not ready (session restore)
VC->>VC: scheduleBindRetryIfNeeded() asyncAfter 0.25s x 40
note over VC: button may be incorrectly visible for up to 10 s
VC-->>VC: retry bindSidebarVisibilityIfNeeded()
end
W->>C: applyAccessoryVisibility (minimal/fullscreen)
C->>VC: "isHidden = true"
W->>AD: user clicks button
AD->>AD: toggleRightSidebarInActiveMainWindow(preferredWindow:)
Reviews (6): Last reviewed commit: "Rebind sidebar-visibility when the windo..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+3258
to
+3259
| @MainActor | ||
| final class RightSidebarToggleAccessoryViewController: NSTitlebarAccessoryViewController { |
Contributor
There was a problem hiding this comment.
RightSidebarToggleAccessoryViewController is declared internal (no access modifier), making it visible to the whole module despite being a file-private implementation detail. RightSidebarToggleTitlebarView correctly uses private; the view controller should too.
Suggested change
| @MainActor | |
| final class RightSidebarToggleAccessoryViewController: NSTitlebarAccessoryViewController { | |
| @MainActor | |
| private final class RightSidebarToggleAccessoryViewController: NSTitlebarAccessoryViewController { |
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!
Contributor
Author
There was a problem hiding this comment.
Fixed in 3c3f0b8 — the view controller is now private, matching the view.
Comment on lines
+3290
to
+3298
| userDefaultsObserver = NotificationCenter.default.addObserver( | ||
| forName: UserDefaults.didChangeNotification, | ||
| object: nil, | ||
| queue: .main | ||
| ) { [weak self] _ in | ||
| Task { @MainActor [weak self] in | ||
| self?.applyFittingSize() | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
UserDefaults.didChangeNotification observer
Every UserDefaults write across the entire app — workspace settings, history entries, search state, etc. — fires this observer and enqueues a new Task { @MainActor } for applyFittingSize(). The guard in applyFittingSize() prevents actual frame changes when the size hasn't changed, but the task allocation and main-actor dispatch still happen for every write. The SwiftUI view already has @AppStorage("titlebarControlsStyle") which triggers a body re-render (and updates fittingSize) on exactly the right key. An onChange modifier on that view feeding back through the onToggle-style closure pattern, or KVO on the specific defaults key, would limit the resize work to the one key that actually changes the button's geometry.
Contributor
Author
There was a problem hiding this comment.
Fixed in 3c3f0b8 — replaced the broad UserDefaults.didChangeNotification observer with KVO on the titlebarControlsStyle key, so it only fires when the style actually changes.
…only its style key Review follow-up: the accessory class needs no module-wide visibility, and the broad UserDefaults.didChangeNotification observer ran fittingSize on every defaults write app-wide. KVO on the titlebarControlsStyle key fires only when the style actually changes. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Sources/Update/UpdateTitlebarAccessory.swift (1)
3259-3332: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winConsider modernizing the KVO implementation.
The old-style KVO pattern (lines 3292–3298, 3311–3324) requires manual observer cleanup and boilerplate. The modern block-based KVO API is safer, more concise, and eliminates the need for
isObservingStyleDefaultand manualremoveObservercalls.♻️ Proposed refactor using block-based KVO
Replace the manual KVO setup/teardown with a single observation that auto-cleans up:
`@MainActor` private final class RightSidebarToggleAccessoryViewController: NSTitlebarAccessoryViewController { private static let styleDefaultsKey = "titlebarControlsStyle" private let hostingView: NonDraggableHostingView<RightSidebarToggleTitlebarView> private let containerView: NSView - private var isObservingStyleDefault = false + private var styleObservation: NSKeyValueObservation? init() { let containerView = NSView() self.containerView = containerView let toggle = { [weak containerView] in `#if` DEBUG cmuxDebugLog("titlebar.toggleRightSidebar") `#endif` _ = AppDelegate.shared?.toggleRightSidebarInActiveMainWindow( preferredWindow: containerView?.window ) } hostingView = NonDraggableHostingView( rootView: RightSidebarToggleTitlebarView(onToggle: toggle) ) super.init(nibName: nil, bundle: nil) view = containerView containerView.translatesAutoresizingMaskIntoConstraints = true hostingView.translatesAutoresizingMaskIntoConstraints = true hostingView.autoresizingMask = [] containerView.addSubview(hostingView) applyFittingSize() - // The button size follows the titlebar controls style; resize when - // that specific default changes (KVO on the key, not the broad - // UserDefaults.didChangeNotification). - UserDefaults.standard.addObserver( - self, - forKeyPath: Self.styleDefaultsKey, - options: [], - context: nil - ) - isObservingStyleDefault = true + // The button size follows the titlebar controls style; resize when + // that specific default changes (KVO on the key, not the broad + // UserDefaults.didChangeNotification). + styleObservation = UserDefaults.standard.observe( + \.titlebarControlsStyle, + options: [] + ) { [weak self] _, _ in + Task { `@MainActor` [weak self] in + self?.applyFittingSize() + } + } } required init?(coder: NSCoder) { fatalError("init(coder:) has not been implemented") } - - deinit { - if isObservingStyleDefault { - UserDefaults.standard.removeObserver(self, forKeyPath: Self.styleDefaultsKey) - } - } - - override func observeValue( - forKeyPath keyPath: String?, - of object: Any?, - change: [NSKeyValueChangeKey: Any]?, - context: UnsafeMutableRawPointer? - ) { - guard keyPath == Self.styleDefaultsKey else { - super.observeValue(forKeyPath: keyPath, of: object, change: change, context: context) - return - } - Task { `@MainActor` [weak self] in - self?.applyFittingSize() - } - } + // deinit not needed - NSKeyValueObservation auto-cleans up private func applyFittingSize() { let size = hostingView.fittingSizeNote: You'll need to extend
UserDefaultswith a keyPath fortitlebarControlsStyle:extension UserDefaults { `@objc` dynamic var titlebarControlsStyle: Int { return integer(forKey: "titlebarControlsStyle") } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Sources/Update/UpdateTitlebarAccessory.swift` around lines 3259 - 3332, The class RightSidebarToggleAccessoryViewController uses legacy KVO via UserDefaults.standard.addObserver(forKeyPath:options:context:) plus isObservingStyleDefault, observeValue(_:of:change:context:) and manual removeObserver in deinit; replace this with the modern block-based KVO API (NSKeyValueObservation) to auto-manage the token: add a stored NSKeyValueObservation? property (e.g. styleObservation), use UserDefaults.standard.observe(\.titlebarControlsStyle, options: []) to assign the observation and run applyFittingSize on change (dispatch to MainActor if needed), remove the isObservingStyleDefault flag and the override observeValue implementation, and drop the manual removeObserver call in deinit so cleanup is automatic.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@Sources/Update/UpdateTitlebarAccessory.swift`:
- Around line 3259-3332: The class RightSidebarToggleAccessoryViewController
uses legacy KVO via
UserDefaults.standard.addObserver(forKeyPath:options:context:) plus
isObservingStyleDefault, observeValue(_:of:change:context:) and manual
removeObserver in deinit; replace this with the modern block-based KVO API
(NSKeyValueObservation) to auto-manage the token: add a stored
NSKeyValueObservation? property (e.g. styleObservation), use
UserDefaults.standard.observe(\.titlebarControlsStyle, options: []) to assign
the observation and run applyFittingSize on change (dispatch to MainActor if
needed), remove the isObservingStyleDefault flag and the override observeValue
implementation, and drop the manual removeObserver call in deinit so cleanup is
automatic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: e830cebf-2367-4e0d-bc4b-c8098db5c143
📒 Files selected for processing (1)
Sources/Update/UpdateTitlebarAccessory.swift
… open sidebar The accessory previously sized its container to the bare button, so the system placed it below the titlebar controls row (visibly lower than the leading cluster). It now mirrors the cluster's layout strategy: container spans the titlebar height and the button centers on the traffic lights' midY. The toggle also hides while the right sidebar is open. manaflow-ai#3757 moved the right-sidebar control into the sidebar chrome as an X button at the same trailing position; showing both stacked them. With the titlebar toggle visible only when the sidebar is closed, the control reads as a single button that moves into the sidebar chrome when it opens — matching how the left sidebar toggle reads as part of the open left sidebar — and the testRightSidebarCloseButtonLivesInsideSidebarChrome expectation (no global titlebar toggle while the sidebar is open) still holds. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Contributor
Author
|
Design update in 343d0f4, after digging into #3757 (which deliberately moved the right-sidebar control out of the global titlebar and into the sidebar chrome as the X button):
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Sources/Update/UpdateTitlebarAccessory.swift`:
- Around line 3395-3399: setToggleHidden(_:) currently uses the incoming hidden
value directly which can re-show the accessory even when fullscreen/minimal
suppression should keep it hidden; change the method to compute a finalHidden by
OR-ing the passed hidden with the current suppression state (e.g. let
finalHidden = hidden || isTitlebarAccessorySuppressed() or check
window/fullscreen/minimal flags), then use guard isHidden != finalHidden else {
return } and set isHidden, view.isHidden and view.alphaValue based on
finalHidden; add or call a helper like isTitlebarAccessorySuppressed() to
encapsulate the fullscreen/minimal check.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: b62afb8a-da93-4e11-98a0-d84b5e5f0a1c
📒 Files selected for processing (1)
Sources/Update/UpdateTitlebarAccessory.swift
Comment on lines
+3395
to
+3399
| private func setToggleHidden(_ hidden: Bool) { | ||
| guard isHidden != hidden else { return } | ||
| isHidden = hidden | ||
| view.isHidden = hidden | ||
| view.alphaValue = hidden ? 0 : 1 |
There was a problem hiding this comment.
Preserve fullscreen/minimal suppression when sidebar state changes.
setToggleHidden(_:) currently maps sidebar visibility directly to controller visibility. That can re-show the accessory after a sidebar close even while fullscreen/minimal mode should still suppress all titlebar accessories.
Suggested fix
+ private func shouldSuppressForWindowMode() -> Bool {
+ WorkspacePresentationModeSettings.mode() == .minimal
+ || (view.window?.styleMask.contains(.fullScreen) ?? false)
+ }
+
- private func setToggleHidden(_ hidden: Bool) {
- guard isHidden != hidden else { return }
- isHidden = hidden
- view.isHidden = hidden
- view.alphaValue = hidden ? 0 : 1
+ private func setToggleHidden(_ hiddenBySidebar: Bool) {
+ let hidden = hiddenBySidebar || shouldSuppressForWindowMode()
+ guard isHidden != hidden || view.isHidden != hidden else { return }
+ isHidden = hidden
+ view.isHidden = hidden
+ view.alphaValue = hidden ? 0 : 1
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Sources/Update/UpdateTitlebarAccessory.swift` around lines 3395 - 3399,
setToggleHidden(_:) currently uses the incoming hidden value directly which can
re-show the accessory even when fullscreen/minimal suppression should keep it
hidden; change the method to compute a finalHidden by OR-ing the passed hidden
with the current suppression state (e.g. let finalHidden = hidden ||
isTitlebarAccessorySuppressed() or check window/fullscreen/minimal flags), then
use guard isHidden != finalHidden else { return } and set isHidden,
view.isHidden and view.alphaValue based on finalHidden; add or call a helper
like isTitlebarAccessorySuppressed() to encapsulate the fullscreen/minimal
check.
During session restore with the right sidebar already open, the accessory's first layout passes run before the main-window context is registered, so the visibility binding never attached and the toggle stacked on the sidebar's X button until the first interaction. Retry the lookup briefly (matching the accessory attach retry pattern) and let the @published subscription apply the restored state immediately. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The accessory container spans the full titlebar height for layout, so an opaque container swallowed clicks that landed inside the accessory frame but outside the button — notably on the sidebar chrome's X button while the toggle briefly overlapped it before the visibility binding attached. Hit-test now claims only actual content. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
♻️ Duplicate comments (1)
Sources/Update/UpdateTitlebarAccessory.swift (1)
3418-3422: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winPreserve fullscreen/minimal suppression when sidebar visibility updates.
Line 3418 rewrites the controller’s hidden state from
FileExplorerState.isVisiblealone. IfapplyAccessoryVisibility(for:)has already hidden this accessory for minimal mode or fullscreen, the nextsidebarVisible == falseemission flipsisHiddenback tofalseand resurrects the button in a mode where this PR says it should stay suppressed.Suggested fix
+ private func shouldSuppressForWindowMode() -> Bool { + WorkspacePresentationModeSettings.mode() == .minimal + || (view.window?.styleMask.contains(.fullScreen) ?? false) + } + - private func setToggleHidden(_ hidden: Bool) { - guard isHidden != hidden else { return } - isHidden = hidden - view.isHidden = hidden - view.alphaValue = hidden ? 0 : 1 + private func setToggleHidden(_ hiddenBySidebar: Bool) { + let hidden = hiddenBySidebar || shouldSuppressForWindowMode() + guard isHidden != hidden || view.isHidden != hidden else { return } + isHidden = hidden + view.isHidden = hidden + view.alphaValue = hidden ? 0 : 1 }Based on PR objectives, this accessory is supposed to stay hidden in minimal mode and fullscreen.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Sources/Update/UpdateTitlebarAccessory.swift` around lines 3418 - 3422, The current setToggleHidden(_:) unconditionally overwrites isHidden from sidebar visibility and can re-show the accessory despite fullscreen/minimal suppression; add a suppression tracking flag or enum (e.g., isSuppressedByMode or suppressionReasons set) that applyAccessoryVisibility(for:) sets when hiding for fullscreen/minimal, and change setToggleHidden(_:) to early-return when hidden == false but suppression flag indicates it should remain suppressed; alternatively accept a reason parameter so applyAccessoryVisibility(for:) sets suppression and only the matching reason can clear it (keeps FileExplorerState-driven visibility updates but preserves mode-based suppression).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@Sources/Update/UpdateTitlebarAccessory.swift`:
- Around line 3418-3422: The current setToggleHidden(_:) unconditionally
overwrites isHidden from sidebar visibility and can re-show the accessory
despite fullscreen/minimal suppression; add a suppression tracking flag or enum
(e.g., isSuppressedByMode or suppressionReasons set) that
applyAccessoryVisibility(for:) sets when hiding for fullscreen/minimal, and
change setToggleHidden(_:) to early-return when hidden == false but suppression
flag indicates it should remain suppressed; alternatively accept a reason
parameter so applyAccessoryVisibility(for:) sets suppression and only the
matching reason can clear it (keeps FileExplorerState-driven visibility updates
but preserves mode-based suppression).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 32c41472-3f1c-4979-863c-1523ce688cb3
📒 Files selected for processing (1)
Sources/Update/UpdateTitlebarAccessory.swift
There was a problem hiding this comment.
♻️ Duplicate comments (1)
Sources/Update/UpdateTitlebarAccessory.swift (1)
3394-3434: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winUnify the accessory's final hidden state.
Line 3403 only schedules a retry when the window context is missing, so a restored window can show this toggle before the first
FileExplorerStatevalue arrives. Then Lines 3429-3433 overwrite the controller-level minimal/fullscreen hide state with the raw sidebar bit, so closing the sidebar can re-show the button even though the titlebar accessory should still stay suppressed.Suggested fix
private final class RightSidebarToggleAccessoryViewController: NSTitlebarAccessoryViewController { + private var sidebarVisibilityResolved = false + private var sidebarVisible = true + + private func shouldSuppressForWindowMode() -> Bool { + WorkspacePresentationModeSettings.mode() == .minimal + || (view.window?.styleMask.contains(.fullScreen) ?? false) + } + + private func applyFinalVisibility() { + let hidden = !sidebarVisibilityResolved || sidebarVisible || shouldSuppressForWindowMode() + guard isHidden != hidden || view.isHidden != hidden else { return } + isHidden = hidden + view.isHidden = hidden + view.alphaValue = hidden ? 0 : 1 + } + init() { let containerView = ClickPassThroughAccessoryContainerView() self.containerView = containerView ... containerView.addSubview(hostingView) applyLayout() + applyFinalVisibility() ... } private func bindSidebarVisibilityIfNeeded() { guard observedFileExplorerState == nil else { return } guard let window = view.window, let state = AppDelegate.shared?.contextForMainTerminalWindow(window)?.fileExplorerState else { + applyFinalVisibility() scheduleBindRetryIfNeeded() return } - bindRetriesRemaining = 0 + bindRetriesRemaining = 0 + sidebarVisibilityResolved = true observedFileExplorerState = state sidebarVisibilityCancellable = state.$isVisible .removeDuplicates() .receive(on: RunLoop.main) .sink { [weak self] sidebarVisible in - self?.setToggleHidden(sidebarVisible) + self?.sidebarVisible = sidebarVisible + self?.applyFinalVisibility() } } - private func setToggleHidden(_ hidden: Bool) { - guard isHidden != hidden else { return } - isHidden = hidden - view.isHidden = hidden - view.alphaValue = hidden ? 0 : 1 - } + private func setToggleHidden(_ hiddenBySidebar: Bool) { + sidebarVisible = hiddenBySidebar + applyFinalVisibility() + } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Sources/Update/UpdateTitlebarAccessory.swift` around lines 3394 - 3434, The accessory currently applies the sidebar's visibility directly in bindSidebarVisibilityIfNeeded -> sink and setToggleHidden, which can re-show the toggle even when the controller intends it suppressed (e.g., minimal/fullscreen state); update the logic so the final hidden state is the OR of the controller-level suppression flag and the sidebar visibility. Concretely, add or use a single source-of-truth (e.g., a computed property or method like desiredAccessoryHidden()) that combines the controller suppression state and the sidebarVisible boolean, call that from the sink installed in bindSidebarVisibilityIfNeeded, and have setToggleHidden accept/apply that combined value (ensure scheduleBindRetryIfNeeded remains unchanged but does not short-circuit the unified hide logic).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@Sources/Update/UpdateTitlebarAccessory.swift`:
- Around line 3394-3434: The accessory currently applies the sidebar's
visibility directly in bindSidebarVisibilityIfNeeded -> sink and
setToggleHidden, which can re-show the toggle even when the controller intends
it suppressed (e.g., minimal/fullscreen state); update the logic so the final
hidden state is the OR of the controller-level suppression flag and the sidebar
visibility. Concretely, add or use a single source-of-truth (e.g., a computed
property or method like desiredAccessoryHidden()) that combines the controller
suppression state and the sidebarVisible boolean, call that from the sink
installed in bindSidebarVisibilityIfNeeded, and have setToggleHidden
accept/apply that combined value (ensure scheduleBindRetryIfNeeded remains
unchanged but does not short-circuit the unified hide logic).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 3023f03f-a913-4f4f-abae-d099bb103abf
📒 Files selected for processing (1)
Sources/Update/UpdateTitlebarAccessory.swift
Session restore can swap the context's FileExplorerState after an early bind; the bind-once guard left the subscription watching the dead object, so a sidebar restored open still showed the titlebar toggle stacked on the chrome X button. Rebind whenever the resolved state instance changes (layout passes drive the re-check). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This was referenced Sep 29, 2026
teamleaderleo
added
area: layout
Collaborator
|
The titlebar toggle remains a distinct UI change; leaving it open with the fullscreen suppression and observer findings for follow-up. |
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
The left titlebar cluster has a sidebar toggle button, but the right sidebar (Files / Find / Sessions / Feed / Dock) has no chrome affordance once it's closed — its header close button disappears with it, so the only way to reopen it is the keyboard shortcut. Mouse-first users lose discoverability of the whole Files panel.
Change
A single trailing-edge titlebar accessory with a
sidebar.righttoggle button:RightSidebarToggleAccessoryViewControllerattached atlayoutAttribute = .right, living inUpdateTitlebarAccessory.swiftnext to the existing controls cluster (reusesTitlebarControlButton,TitlebarControlIconStyle,NonDraggableHostingView— all file-internal).titlebarControlsStylesetting (classic/compact/roomy/pill/soft) via@AppStorageand resizes when the style changes.AppDelegate.toggleRightSidebarInActiveMainWindow(preferredWindow:), the same path as thetoggleRightSidebarkeyboard shortcut (Cmd+Opt+B by default); the tooltip renders the currently bound shortcut viaKeyboardShortcutSettings.Action.toggleRightSidebar.tooltip.applyAccessoryVisibility/removeAccessoryIfPresent).Localization audit
Two new user-facing strings (
titlebar.rightSidebar.accessibilityLabel,titlebar.rightSidebar.tooltip) added toResources/Localizable.xcstringswith translations for all 19 supported locales, phrased consistently with the existingtitlebar.sidebar.*entries. The xcstrings edit is insertion-only (238 added lines, no reformat).Verification
accessibilityIdentifier:titlebarControl.toggleRightSidebar(matches thetitlebarControl.*convention for UI tests).🤖 Generated with Claude Code
Need help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.Summary by cubic
Adds a right-sidebar toggle button on the title bar’s trailing edge to restore mouse discoverability for Files/Find/Sessions/Feed/Dock. It mirrors
toggleRightSidebar(Cmd+Opt+B), aligns with the controls row, and hides in minimal, fullscreen, and while the right sidebar is open.Shares the
toggleRightSidebaraction path; tooltip shows the bound shortcut. FollowstitlebarControlsStyleand resizes on change via KVO. Centers on the traffic lights row. Accessory is file‑private. Container passes clicks through outside the button. Binds to sidebar visibility with retries during session restore, and rebinds when the window context replacesFileExplorerState, so the toggle yields to the sidebar’s X without stacking. Localized accessibility label + tooltip for 19 locales. UI test id:titlebarControl.toggleRightSidebar.Written for commit 873d992. Summary will update on new commits.
Summary by CodeRabbit
New Features
Localization