Skip to content

Keyboard workspace cycling mirrors the sidebar: collapsed groups are one stop - #9801

Open
krandder wants to merge 2 commits into
manaflow-ai:mainfrom
krandder:keyboard-nav-skip-collapsed
Open

krandder wants to merge 2 commits into
manaflow-ai:mainfrom
krandder:keyboard-nav-skip-collapsed

Conversation

@krandder

@krandder krandder commented Aug 7, 2026 •

Copy link
Copy Markdown

Keyboard workspace cycling visits every workspace in tabs order, including members hidden inside collapsed sidebar groups. Crossing a closed folder costs one keystroke per hidden member, and selecting a hidden member auto-expands the group — cycling past a folder destroys its collapse state.

Now selectNextTab/selectPreviousTab derive their stops from SidebarWorkspaceRenderItem.renderItems — what the sidebar draws — so cycling mirrors the visible sidebar: a collapsed group is one stop (its header), hidden members are skipped, expanded groups traverse normally, wrap-around is unchanged. Expand/collapse stays on the existing toggle shortcut, which cycling can now reach for closed folders.

Notes:

  • The socket/CLI workspace next/previous commands share this path and inherit the semantics.
  • Same-target selection (single visible stop) still runs, keeping the notification-dismissal side effects.
  • Happy to gate this behind a setting if you'd rather keep the current traversal as default.

Red/green commits per the regression-test policy: WorkspaceGroupKeyboardCycleTests, same shape as WorkspaceGroupTests, pbxproj-wired (lint-pbxproj-test-wiring clean).

🤖 Generated with Claude Code


View with [code]smith Autofix with [code]smith
Need help on this PR? Tag @codesmith-bot with what you need. Autofix is disabled.

Summary by CodeRabbit

  • Bug Fixes

    • Improved keyboard navigation between workspaces and groups.
    • Collapsed groups now act as a single navigation stop, while expanded groups allow navigation through each workspace.
    • Hidden workspace selections now navigate relative to their group.
    • Preserved selection when only one workspace is visible.
    • Maintained existing selection clearing and notification behavior during navigation.
  • Tests

    • Added coverage for forward and backward navigation across grouped workspaces, including collapsed and hidden selections.

@coderabbitai

coderabbitai Bot commented Aug 7, 2026 •

Copy link
Copy Markdown

Review Change Stack

Important

Review skipped

No new commits to review since the last review.

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: fbc2cc0a-8af3-40d6-98de-7bc4d93c34ca

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

Workspace next and previous selection now follow visible sidebar rows. Collapsed groups act as single stops, expanded groups expose members, and hidden selected members resolve through their group anchor. Shared navigation and tests cover these cases.

Changes

Workspace group keyboard cycling

Layer / File(s) Summary
Adjacent selection contract
Packages/macOS/CmuxWorkspaces/Sources/CmuxWorkspaces/Navigation/WorkspaceAdjacentSelection.swift, Packages/macOS/CmuxWorkspaces/Tests/CmuxWorkspacesTests/Navigation/WorkspaceAdjacentSelectionTests.swift
Adds wrapping navigation across visible workspace stops, fallback handling for hidden selections, and tests for boundary and nil cases.
Sidebar-row navigation behavior
Sources/TabManager.swift
Next and previous commands use visible sidebar rows, collapsed-group anchors, hidden-member skipping, wrapping, and preserved selection side effects.
Grouped navigation test coverage
cmuxTests/WorkspaceGroupKeyboardCycleTests.swift, cmux.xcodeproj/project.pbxproj
Tests cover collapsed and expanded groups, reverse navigation, hidden selected members, collapse preservation, and single-workspace retention. The test file is registered in the Xcode project.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Possibly related PRs

Suggested reviewers: austinywang, lawrencecchen


Important

Pre-merge checks failed

Please resolve all errors before merging. Addressing warnings is optional.

❌ Failed checks (3 errors, 2 warnings)

Check name Status Explanation Resolution
Cmux Swift Actor Isolation ❌ Error The new production WorkspaceAdjacentSelection is a pure value-only helper but has no explicit nonisolated boundary, introducing avoidable MainActor coupling under Swift 6 isolation. Declare WorkspaceAdjacentSelection explicitly nonisolated (and its static helper if required), or document why MainActor isolation is intentional.
Cmux Algorithmic Complexity ❌ Error Sources/TabManager.swift:3635-3640 rebuilds all sidebar stops and rescans tabs/stops on every keyboard or socket step: O(W+G) per event, with no cache or bound for ~1000 workspaces. Cache the sidebar stop snapshot and invalidate it when tab order, membership, or group collapse changes, or pass the existing sidebar snapshot into navigation; benchmark at ~1000 workspaces.
Cmux No Ambient Global State ❌ Error WorkspaceAdjacentSelection.swift:5-26 adds a caseless public enum whose only API is the static target function, matching the rule's static-only namespace failure. Replace the static-only enum with a constructable WorkspaceAdjacentSelector instance type and inject it at the TabManager seam, or keep a pure helper private/fileprivate.
Docstring Coverage ⚠️ Warning Docstring coverage is 21.43% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
Description check ⚠️ Warning The description explains the behavior change and mentions regression testing, but it omits the required Testing, Demo Video, review trigger, and checklist sections. Add the template headings and complete the Testing, Demo Video, Review Trigger, and Checklist sections, including manual verification and review status.
✅ Passed checks (20 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Cmux Swift Blocking Runtime ✅ Passed The production diff adds only deterministic sidebar-stop selection. It adds no blocking or timing primitive; existing Task.sleep and dispatch delays are unchanged, and test files use no such primit...
Cmux Browser Automation Off-Main ✅ Passed The PR changes workspace navigation, tests, and project wiring only; it does not modify browser socket commands, worker routing, WebKit waits, or policy tests.
Cmux Expensive Synchronous Load ✅ Passed The production diff only adds in-memory sidebar stop computation and UUID navigation; it adds no agent-history loader, file read, JSON parse, directory scan, or transcript access on the main actor.
Cmux Cache Substitution Correctness ✅ Passed The diff changes only live sidebar navigation: render stops derive from current tabs and workspaceGroups, and the helper is pure UUID stepping. No persistence, history, undo, or snapshot read is re...
Cmux No Hacky Sleeps ✅ Passed The PR changes only Swift source/tests and four pbxproj wiring lines; no timing primitives were added, and the rule excludes Swift runtime changes.
Cmux Swift Concurrency ✅ Passed The diff adds only synchronous navigation logic and a pure UUID helper; no new Dispatch, Task, Combine, completion-handler, or fire-and-forget async pattern appears in added Swift lines.
Cmux Swift @Concurrent ✅ Passed The diff adds only synchronous navigation code: TabManager remains @MainActor, and WorkspaceAdjacentSelection.target is a pure synchronous helper with no async or @concurrent changes.
Cmux Swift Package Boundaries ✅ Passed The new Foundation-only WorkspaceAdjacentSelection API is isolated in CmuxWorkspaces and has package unit tests; TabManager retains only AppKit/app-state composition and sidebar render glue.
Cmux Swiftpm Lockfiles ✅ Passed Packages/macOS/CmuxWorkspaces adds only a source file; no Package.swift, dependency pin, Xcode package-reference, .gitignore, or workflow change requires a lockfile diff.
Cmux Swift Logging ✅ Passed The diff only changes existing workspace-switch tracing inside #if DEBUG; it adds no print, debugPrint, dump, NSLog, ad hoc logging, Logger, or sensitive-data logs.
Cmux User-Facing Error Privacy ✅ Passed The production diff only changes workspace navigation and adds a UUID selection helper; it adds no user-facing errors, alerts, command output, raw messages, or sensitive implementation details.
Cmux Full Internationalization ✅ Passed The PR adds no user-facing text; “next”/“prev” only feed debug tracing under #if DEBUG, and no localization catalogs or locale message files change.
Cmux Swiftui State Layout ✅ Passed The diff adds a pure UUID navigation helper and changes existing TabManager methods; it adds no SwiftUI state, geometry measurement, lazy-row store references, or render-time mutation.
Cmux Architecture Rethink ✅ Passed The aggregate diff adds a pure UUID helper and one shared selectAdjacentTab path; it adds no timing, blocking, observer, mutable-state, or split-lifecycle repair path.
Cmux Swift Auxiliary Window Close Shortcuts ✅ Passed The complete PR diff changes workspace navigation and tests only; it adds no NSWindow, NSPanel, controller, Window, or WindowGroup. scripts/lint_auxiliary_window_close_shortcuts.py passed.
Cmux Source Artifacts ✅ Passed The diff changes only intentional Swift source files: Sources/TabManager.swift and the new WorkspaceAdjacentSelection.swift; no logs, caches, build output, scratch directories, or copied artifacts...
Cmux No Test Or Debug Seam In Production Source ✅ Passed The added production API is WorkspaceAdjacentSelection.target, called by TabManager.selectAdjacentTab for live navigation; no test/debug-named seam or test-only guarded accessor was added.
Title check ✅ Passed The title clearly summarizes the main change: workspace cycling now treats collapsed groups as one sidebar navigation stop.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@krandder
krandder force-pushed the keyboard-nav-skip-collapsed branch from 1affdda to 89218dc Compare August 7, 2026 07:54

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 `@cmuxTests/WorkspaceGroupKeyboardCycleTests.swift`:
- Around line 77-90: Extend previousDirectionMirrorsNextAcrossACollapsedGroup
using a fixture with at least three visible navigation stops, and assert
previous traversal reaches distinct stops in reverse order before wrapping to
the start. Keep the collapsed-group assertion, ensuring the test distinguishes
correct previous direction from an implementation that incorrectly uses the
next-step behavior.
🪄 Autofix

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 Plus

Run ID: d1bdd183-c182-4da5-aa79-3f087215cd5b

📥 Commits

Reviewing files that changed from the base of the PR and between 3faf795 and 1affdda.

📒 Files selected for processing (3)
  • Sources/TabManager.swift
  • cmux.xcodeproj/project.pbxproj
  • cmuxTests/WorkspaceGroupKeyboardCycleTests.swift

Comment thread cmuxTests/WorkspaceGroupKeyboardCycleTests.swift Outdated
@coderabbitai

coderabbitai Bot commented Aug 7, 2026

Copy link
Copy Markdown

Note

GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer.

Cycling visits members hidden inside collapsed sidebar groups: crossing a
closed folder costs one keystroke per hidden member and auto-expands it.
App-target tests pin the sidebar-mirroring behavior; package tests cover
the stop-resolution logic. They fail until the next commit.
Stop resolution lives in CmuxWorkspaces as WorkspaceAdjacentSelection;
TabManager builds the stops from SidebarWorkspaceRenderItem and keeps the
selection side effects. A collapsed group is one stop (its header); hidden
members are skipped; wrap-around is unchanged. Expand and collapse stay on
the existing toggle shortcut.

The socket workspace next/previous commands share this path. Same-target
selection still runs, keeping the notification-dismissal side effects.
@krandder
krandder force-pushed the keyboard-nav-skip-collapsed branch from 89218dc to 1e9a0fb Compare August 7, 2026 08:08
@coderabbitai

coderabbitai Bot commented Aug 7, 2026

Copy link
Copy Markdown

Note

GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer.

@krandder

krandder commented Aug 7, 2026

Copy link
Copy Markdown
Author

@coderabbitai review

@cubic-dev-ai review

@coderabbitai

coderabbitai Bot commented Aug 7, 2026 •

Copy link
Copy Markdown

@krandder I will review the changes in #9801.

⚠️ Action not completed

Already reviewed.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@cubic-dev-ai

cubic-dev-ai Bot commented Aug 7, 2026

Copy link
Copy Markdown

@coderabbitai review

@cubic-dev-ai review

@krandder I can't start this review because your workspace has reached its free monthly review limit. cubic has reviewed 245,275 of the 240,000 allowed lines of code this month. Reviews resume on 1 September 2026 (in 25 days). Paid plans include much higher monthly review limits. Upgrade now to resume reviews.

To help optimise your usage, you can tune cubic to get the most out of your usage limits:

Learn more →

@teamleaderleo

Copy link
Copy Markdown
Collaborator

Thank you for this, @krandder! Skipping hidden members of collapsed groups when cycling is exactly right. Since you opened it, #9352 moved workspace cycling into WorkspacesModel.cycleDestination, so the fix would need to live there now (for the window scope). If you're up for porting it, we'd love to land it, and putting it behind a setting like you offered sounds great :)

@teamleaderleo teamleaderleo added bug Something isn't working S3: minor Wrong behavior with a workaround area: sidebar The workspace sidebar: list, groups, status, reordering area: workspaces Workspaces, sessions, restore after relaunch, worktrees labels Sep 30, 2026

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area: sidebar The workspace sidebar: list, groups, status, reordering area: workspaces Workspaces, sessions, restore after relaunch, worktrees bug Something isn't working S3: minor Wrong behavior with a workaround

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants