Repository navigation
Add beta feature toggles for Feed and Dock - #3537
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Caution Review failedPull request was closed or merged during review 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 (5)
📝 WalkthroughWalkthroughAdds opt-in beta-feature gating for right-sidebar Feed and Dock: new UserDefaults keys and defaults, Settings UI and localization, availability checks for modes/shortcuts/command-palette, FileExplorerState enforcing/clamping stored modes, focus guards, UI adjustments, tests, and project wiring. ChangesBeta Feature Gating for Right Sidebar Feed & Dock
sequenceDiagram
participant User
participant SettingsView
participant UD as UserDefaults
participant Mode as RightSidebarMode
participant Panel as RightSidebarPanel
participant State as FileExplorerState
participant Focus as MainWindowFocusController
User->>SettingsView: toggle Feed/Dock
SettingsView->>UD: write rightSidebar.beta.*.enabled
Panel->>Mode: request availableModes()
Mode->>UD: read feed/dock flags
Mode-->>Panel: filtered modes
Panel->>State: refreshModeAvailability()
State->>Mode: storedMode.isAvailable()
Mode-->>State: availability result
State-->>Panel: mode clamped/persisted if needed
User->>Panel: request focus/select mode
Panel->>Focus: focusRightSidebar(mode)
Focus->>Mode: mode.isAvailable()
Mode-->>Focus: allowed? (true/false)
alt allowed
Focus-->>Panel: perform focus
else not allowed
Focus-->>Panel: return false / fallback to .files
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (3 errors, 1 warning, 1 inconclusive)
✅ Passed checks (8 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the 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 Beta Features settings section with Feed and Dock toggles, defaulting both to disabled. Availability is enforced at the model setter level (
Confidence Score: 5/5Safe to merge. The availability invariant is now enforced at the model layer, with consistent guards in the command palette, keyboard shortcuts, and focus controller. All changed paths enforce availability through a single chokepoint in FileExplorerState.setMode. The command palette, shortcuts, and MainWindowFocusController all independently guard against activating a disabled mode. Tests cover the clamping logic at init and at the setter. The only finding is a style preference about adopting @observable, which does not affect runtime correctness. No files require special attention. FileExplorerState.swift is the most structurally significant change and is well-tested by the new FileExplorerStateModePersistenceTests. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
S[Settings toggle Feed/Dock] -->|writes UserDefaults| UD[(UserDefaults)]
UD --> RPV[RightSidebarPanelView AppStorage feedEnabled/dockEnabled]
RPV -->|onChange| RAM[FileExplorerState refreshModeAvailability]
RAM -->|setMode clamps to available| FES[FileExplorerState storedMode]
FES -->|objectWillChange| MBAR[modeBar ForEach availableModes only]
CP[Command Palette open] -->|availableModes filter| CONTRIB[Contributions - available modes only]
CONTRIB -->|user activates| HCPR[handleCommandPaletteRightSidebarMode guard isAvailable]
KS[Keyboard Shortcut] -->|isAvailable guard| FES
MWF[MainWindowFocusController focusRightSidebar] -->|isAvailable guard fallback to files| FES
Reviews (4): Last reviewed commit: "Trigger CI rerun" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| @@ -192,23 +237,29 @@ struct RightSidebarPanelView: View { | |||
| .onAppear { | |||
| modeShortcutHintMonitor.start() | |||
| focusShortcutHintMonitor.start() | |||
| ensureSelectedModeIsAvailable() | |||
| } | |||
| .onDisappear { | |||
| modeShortcutHintMonitor.stop() | |||
| focusShortcutHintMonitor.stop() | |||
| } | |||
| .onChange(of: fileExplorerState.mode) { _, mode in if mode != .dock { dockStore.deactivate() } } | |||
| .onChange(of: fileExplorerState.mode) { _, mode in | |||
| ensureSelectedModeIsAvailable() | |||
| if mode != .dock { dockStore.deactivate() } | |||
| } | |||
| .onChange(of: fileExplorerState.isVisible) { _, visible in if !visible { dockStore.deactivate() } } | |||
| .onChange(of: feedEnabled) { _, _ in ensureSelectedModeIsAvailable() } | |||
| .onChange(of: dockEnabled) { _, _ in ensureSelectedModeIsAvailable() } | |||
There was a problem hiding this comment.
The PR adds effectiveMode as a silent view-local override whenever fileExplorerState.mode holds a disabled mode, and ensureSelectedModeIsAvailable() attempts to repair the model from three onChange sites. But FileExplorerState.mode's didSet has no availability guard, so the model can hold an invalid mode from the moment a feature is disabled until the view's next change-handler fires. During that window, fileExplorerState.mode and effectiveMode disagree — and any non-view code path that reads fileExplorerState.mode directly (e.g. MainWindowFocusController, command-palette registration, deep links) sees the stale unavailable value rather than the corrected one.
The invariant "mode must be available" is now enforced in three separate places: FileExplorerState.init, the @Published var mode setter (not at all), and the view layer. The cleanest single enforcement point would be the mode setter on FileExplorerState itself — validate against the current beta-feature defaults before writing to UserDefaults. That would let effectiveMode and ensureSelectedModeIsAvailable() be removed entirely.
Rule Used: Flag Swift fixes that patch symptoms while leaving... (source)
There was a problem hiding this comment.
Fixed by centralizing unavailable-mode clamping in FileExplorerState and removing the view-local effectiveMode override.
— Claude Code
| .onChange(of: fileExplorerState.mode) { _, mode in | ||
| ensureSelectedModeIsAvailable() | ||
| if mode != .dock { dockStore.deactivate() } | ||
| } | ||
| .onChange(of: fileExplorerState.isVisible) { _, visible in if !visible { dockStore.deactivate() } } | ||
| .onChange(of: feedEnabled) { _, _ in ensureSelectedModeIsAvailable() } | ||
| .onChange(of: dockEnabled) { _, _ in ensureSelectedModeIsAvailable() } |
There was a problem hiding this comment.
dockStore.deactivate() fires twice on an unavailable-mode transition
When fileExplorerState.mode is set to an unavailable mode (e.g. .feed while Feed is disabled), the onChange(of: fileExplorerState.mode) handler fires, calls ensureSelectedModeIsAvailable(), which immediately sets fileExplorerState.mode = .files. That write triggers onChange a second time with mode = .files, and dockStore.deactivate() executes again (files != dock). deactivate() is called once for the unavailable mode and once more for the .files correction. This double-fire is harmless only if deactivate() is idempotent; if it ever grows side effects the second invocation could cause observable misbehaviour.
| enum RightSidebarBetaFeatureSettings { | ||
| static let feedEnabledKey = "rightSidebar.beta.feed.enabled" | ||
| static let dockEnabledKey = "rightSidebar.beta.dock.enabled" | ||
|
|
||
| static let defaultFeedEnabled = false | ||
| static let defaultDockEnabled = false | ||
|
|
||
| static func isFeedEnabled(defaults: UserDefaults = .standard) -> Bool { | ||
| guard defaults.object(forKey: feedEnabledKey) != nil else { return defaultFeedEnabled } | ||
| return defaults.bool(forKey: feedEnabledKey) | ||
| } | ||
|
|
||
| static func isDockEnabled(defaults: UserDefaults = .standard) -> Bool { | ||
| guard defaults.object(forKey: dockEnabledKey) != nil else { return defaultDockEnabled } | ||
| return defaults.bool(forKey: dockEnabledKey) | ||
| } | ||
| } |
There was a problem hiding this comment.
RightSidebarBetaFeatureSettings static helpers should be nonisolated
The new static methods isFeedEnabled(defaults:) and isDockEnabled(defaults:) read from UserDefaults — a pure, thread-safe value operation with no UI coupling. Without an explicit nonisolated annotation, they inherit the module's default actor isolation (likely @MainActor in a Swift 6 target), unnecessarily coupling them to the main actor and preventing calls from non-isolated async contexts. Per the actor-isolation rule, static utility helpers that do not interact with UI state should be marked nonisolated.
Rule Used: Flag new or materially worsened Swift 6 actor isol... (source)
There was a problem hiding this comment.
Fixed by marking the pure RightSidebarBetaFeatureSettings lookup helpers nonisolated.
— 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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
Sources/SettingsNavigation.swift (1)
372-432:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd settings-path anchor mappings for new beta toggles.
settingsPathAnchorIDswas not extended for the new Beta Features settings, so path-based navigation/highlight likely won’t resolve for those keys even though search entries exist.Suggested patch
private static let settingsPathAnchorIDs: [String: String] = [ + "rightSidebar.beta.feed.enabled": settingID(for: .betaFeatures, idSuffix: "feed"), + "rightSidebar.beta.dock.enabled": settingID(for: .betaFeatures, idSuffix: "dock"), "app.language": settingID(for: .app, idSuffix: "language"), "app.appearance": settingID(for: .app, idSuffix: "appearance"),Based on learnings, when adding a new Settings section, matching SettingsSearchIndex wiring should be kept consistent so navigation and jump-to behavior remain intact.
🤖 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/SettingsNavigation.swift` around lines 372 - 432, The settingsPathAnchorIDs map is missing entries for the new Beta Features settings, so path-based navigation/highlight won't resolve for those keys; update the settingsPathAnchorIDs dictionary to include mappings for each new beta toggle using settingID(for: .<section>, idSuffix: "<suffix>") (reference the existing pattern in settingsPathAnchorIDs and the settingID(...) helper) — add keys matching the SettingsSearchIndex/new beta setting keys and point them to the appropriate anchor IDs so jump-to and highlight behavior works consistently with other sections.Sources/MainWindowFocusController.swift (1)
412-429:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winFile-length CI budget exceeded — extract before merging.
The new availability guards are correct (recursion to
.filesis well-bounded, and the private overload's defensive return is consistent with the public entry). However CI now fails:File length exceeds budget: actual=784, budget=778.
You're 6 lines over. Move some self-contained logic to a sibling file under
Sources/(e.g.,MainWindowFocusController+ResponderQueries.swift) — good extraction candidates areterminalFocusRequest(for:),selectedFocusedPanelRequest(owning:),selectedFocusedBrowserPanelRequest(), and the privateFocusedPanelRequest/TerminalFocusRequeststructs. They have no SwiftUI/AppKit-state coupling beyondtabManager/windowand are pure responder/workspace lookups, so they extract cleanly.As per coding guidelines (file-length budget enforced for tracked Swift files; prefer a dedicated
Sources/file rather than bloating existing files; do not extract app-internal helpers into a SwiftPM package without a dedicated package).🤖 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/MainWindowFocusController.swift` around lines 412 - 429, Move the pure responder/workspace lookup logic out of MainWindowFocusController into a new sibling file (e.g., MainWindowFocusController+ResponderQueries.swift): extract the functions terminalFocusRequest(for:), selectedFocusedPanelRequest(owning:), selectedFocusedBrowserPanelRequest(), and the private structs FocusedPanelRequest and TerminalFocusRequest into that file as an extension on MainWindowFocusController so they can still reference tabManager and window; preserve their current access control (private/internal) and signatures so callers need no changes; add any necessary imports at the top of the new file and run the build to ensure visibility is correct and no references remain in the original file.
🤖 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/cmuxApp.swift`:
- Around line 6486-6530: The file is over the length budget because the
beta-features UI (the SettingsSectionHeader + SettingsCard block and the
SettingsWarningNote component) is in cmuxApp.swift; extract that UI into a new
SwiftUI view file (e.g., BetaFeaturesSettingsView.swift) that defines a struct
BetaFeaturesSettingsView: View which contains the SettingsSectionHeader,
SettingsCard, the SettingsWarningNote implementation (move its definition if
it's newly introduced here), and the SettingsCardRow/Toggles; expose any
bindings used here (e.g., rightSidebarFeedEnabled, rightSidebarDockEnabled,
rightSidebarFeedSubtitle/rightSidebarDockSubtitle) as `@Binding` or init
parameters so the parent can pass them in, keep the same
accessibilityIdentifiers/labels and SettingsSearchIndex calls, import SwiftUI,
and then replace the moved block in cmuxApp.swift with a single
BetaFeaturesSettingsView(...) instantiation. Ensure no logic changes and update
any references to SettingsWarningNote to the moved implementation.
- Around line 5273-5276: resetAllSettings() currently doesn't clear the new
AppStorage keys rightSidebarFeedEnabled and rightSidebarDockEnabled, so "Reset
All Settings" leaves stale Feed/Dock beta states; update resetAllSettings() to
set rightSidebarFeedEnabled = RightSidebarBetaFeatureSettings.defaultFeedEnabled
and rightSidebarDockEnabled = RightSidebarBetaFeatureSettings.defaultDockEnabled
(or call whatever existing helper exists to reset
RightSidebarBetaFeatureSettings) so those persisted toggles are returned to
their defaults.
In `@Sources/RightSidebarPanelView.swift`:
- Around line 49-77: The RightSidebarMode availability helpers (the extension
methods availableModes(...) and isAvailable(...), which reference
RightSidebarBetaFeatureSettings) should be extracted out of
RightSidebarPanelView into a new standalone Swift source file as a plain
extension on RightSidebarMode; create a file containing the same extension,
include import Foundation, keep the existing static func
availableModes(defaults:), static func availableModes(feedEnabled:dockEnabled:),
func isAvailable(defaults:), and func isAvailable(feedEnabled:dockEnabled:)
signatures and logic unchanged so existing call sites in FileExplorerStore and
MainWindowFocusController continue to compile without changes.
---
Outside diff comments:
In `@Sources/MainWindowFocusController.swift`:
- Around line 412-429: Move the pure responder/workspace lookup logic out of
MainWindowFocusController into a new sibling file (e.g.,
MainWindowFocusController+ResponderQueries.swift): extract the functions
terminalFocusRequest(for:), selectedFocusedPanelRequest(owning:),
selectedFocusedBrowserPanelRequest(), and the private structs
FocusedPanelRequest and TerminalFocusRequest into that file as an extension on
MainWindowFocusController so they can still reference tabManager and window;
preserve their current access control (private/internal) and signatures so
callers need no changes; add any necessary imports at the top of the new file
and run the build to ensure visibility is correct and no references remain in
the original file.
In `@Sources/SettingsNavigation.swift`:
- Around line 372-432: The settingsPathAnchorIDs map is missing entries for the
new Beta Features settings, so path-based navigation/highlight won't resolve for
those keys; update the settingsPathAnchorIDs dictionary to include mappings for
each new beta toggle using settingID(for: .<section>, idSuffix: "<suffix>")
(reference the existing pattern in settingsPathAnchorIDs and the settingID(...)
helper) — add keys matching the SettingsSearchIndex/new beta setting keys and
point them to the appropriate anchor IDs so jump-to and highlight behavior works
consistently with other sections.
🪄 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: f4c8e61e-b5fa-41c9-af2c-7b1dd4ab31ac
📒 Files selected for processing (13)
Resources/Localizable.xcstringsSources/App/WorkspaceRuntimeSettings.swiftSources/ContentView+RightSidebarCommandPalette.swiftSources/ContentView.swiftSources/Feed/FeedPanelView.swiftSources/FileExplorerStore.swiftSources/MainWindowFocusController.swiftSources/RightSidebarPanelView.swiftSources/SettingsNavigation.swiftSources/SettingsSearchAliases.swiftSources/cmuxApp.swiftcmuxUITests/FeedSidebarUITests.swiftcmuxUITests/RightSidebarChromeHeightUITests.swift
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/RightSidebarPanelView.swift`:
- Around line 215-216: Replace the simple calls to
fileExplorerState.refreshModeAvailability() in the .onChange handlers for
feedEnabled and dockEnabled with a helper that refreshes availability and
retargets keyboard focus when the active right-sidebar mode changed; implement
refreshModeAvailabilityAndFocusIfNeeded() to capture previousMode =
fileExplorerState.mode, call fileExplorerState.refreshModeAvailability(), and if
previousMode != fileExplorerState.mode and fileExplorerState.isVisible and there
is an NSApp keyWindow/mainWindow, call
AppDelegate.shared?.focusRightSidebarInActiveMainWindow(mode:
fileExplorerState.mode, focusFirstItem: false, preferredWindow: window) so
MainWindowFocusController’s intent is updated immediately.
🪄 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: 261bd78e-b283-46e1-b361-a30fe0abb114
📒 Files selected for processing (15)
GhosttyTabs.xcodeproj/project.pbxprojSources/App/WorkspaceRuntimeSettings.swiftSources/BetaFeaturesSettingsView.swiftSources/FileExplorerState.swiftSources/FileExplorerStore.swiftSources/MainWindowFocusController.swiftSources/MainWindowFocusTypes.swiftSources/RightSidebarMode+Availability.swiftSources/RightSidebarPanelView.swiftSources/SettingsCardNote.swiftSources/SettingsNavigation.swiftSources/cmuxApp.swiftcmuxTests/FileExplorerStateModePersistenceTests.swiftcmuxTests/RightSidebarCommandPaletteTests.swiftcmuxTests/ShortcutAndCommandPaletteTests.swift
💤 Files with no reviewable changes (2)
- cmuxTests/ShortcutAndCommandPaletteTests.swift
- Sources/FileExplorerStore.swift
Adds default-disabled beta feature toggles for Feed and Dock, hides Feed Activity, and enforces unavailable right-sidebar modes across settings, command palette, shortcuts, and focus routing. PR: manaflow-ai/cmux#3537
This branch was successfully deployed
1 participant
Summary
Testing
jq empty Resources/Localizable.xcstringsgit diff --check./scripts/reload.sh --tag betaftRightSidebarChromeHeightUITests: https://github.com/manaflow-ai/cmux/actions/runs/25362995486FeedSidebarUITests: https://github.com/manaflow-ai/cmux/actions/runs/25362995440Issues
Summary by CodeRabbit
New Features
Improvements
Tests