Skip to content

Fix command-hold shortcut hints and prevent sidebar truncation - #2767

Merged
lawrencecchen merged 1 commit into
mainfrom
feat-cmd-hold-shows-all-hints
Apr 10, 2026
Merged

lawrencecchen merged 1 commit into
mainfrom
feat-cmd-hold-shows-all-hints

Conversation

@lawrencecchen

@lawrencecchen lawrencecchen commented Apr 10, 2026 •

Copy link
Copy Markdown
Contributor

Summary

  • make command-hold hint reveal independent of workspace-number shortcut shape (including chorded remaps)
  • keep control-hold behavior scoped to control-relevant pane hints
  • stop reserving extra sidebar row width for shortcut hint pills so command-hold no longer forces title ellipsis
  • add regression coverage for the chorded-remap command-hold policy
  • update bonsplit submodule for pane-hint command-hold reveal policy and tests

Linked submodule PR


Summary by cubic

Fixes Command-hold shortcut hints so they always show, even when workspace-by-number uses a chorded remap. Also prevents sidebar title truncation by removing extra hint-pill width, while keeping Control-hold scoped to control-only hints.

  • Bug Fixes

    • Show hints on Command-hold regardless of workspace-number shortcut shape (including chords).
    • Limit Control-hold to control-relevant pane hints.
    • Stop reserving sidebar width for hint pills to avoid title ellipsis on Command-hold.
    • Add regression test for the chorded-remap Command-hold policy.
  • Dependencies

    • Update vendor/bonsplit to include the new hint-reveal policy and tests.

Written for commit f8dfdef. Summary will update on new commits.

Summary by CodeRabbit

  • Bug Fixes

    • Improved keyboard shortcut hint display logic to ensure consistent behavior across different modifier key configurations, particularly for workspace shortcuts with chord modifiers.
  • Tests

    • Added test coverage for keyboard shortcut hint behavior when using chord modifier combinations.
  • Chores

    • Updated internal dependencies.

@vercel

vercel Bot commented Apr 10, 2026 •

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
cmux Ready Ready Preview, Comment Apr 10, 2026 0:44am

@coderabbitai

coderabbitai Bot commented Apr 10, 2026 •

Copy link
Copy Markdown
📝 Walkthrough

Walkthrough

This PR simplifies shortcut hint visibility logic by removing the dependency on keyboard shortcut chord detection. The ShortcutHintModifierPolicy now determines hint visibility based solely on normalized modifier flags and debug settings, while SidebarTrailingAccessoryWidthPolicy no longer needs to handle shortcut hint-related properties.

Changes

Cohort / File(s) Summary
Shortcut Hint Logic Simplification
Sources/ContentView.swift
Removed chord detection guard from ShortcutHintModifierPolicy.shouldShowHints(for:defaults:) to simplify hint visibility logic. Redesigned SidebarTrailingAccessoryWidthPolicy.width(...) to accept only canCloseWorkspace: Bool, eliminating all workspace shortcut hint measurement and offset logic. Updated TabItemView.trailingAccessoryWidth call site accordingly.
Test Coverage
cmuxTests/ShortcutAndCommandPaletteTests.swift
Added test testShortcutHintStaysCommandOnlyWhenWorkspaceShortcutUsesChord to verify that hint visibility correctly returns true for [.command] and false for [.control] modifier flags even when workspace shortcuts use chord configuration.
Dependency Updates
vendor/bonsplit
Updated git submodule commit reference to newer version.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

Possibly related PRs

Poem

🐰 With chords now gone, the hints stay clean,
Command-key logic, sharp and keen!
Width is simpler, hints align,
A rabbit's refactor, perfectly fine! 🎀

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately summarizes the main changes: fixing command-hold shortcut hints and preventing sidebar truncation, which directly align with the primary objectives.
Description check ✅ Passed The description covers the Summary and Testing sections adequately. However, it is missing the Demo Video section, and the Checklist is incomplete with all items unchecked.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch feat-cmd-hold-shows-all-hints

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 and usage tips.

@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 the current code and only fix it if needed.

Inline comments:
In `@vendor/bonsplit`:
- Line 1: The parent repo's gitlink for submodule vendor/bonsplit points at an
unreachable commit (7cd3b5e038e28264c6efc9492d17126e29c8765d); resolve by either
pushing that missing commit to the vendor/bonsplit remote main branch so the
pointer is reachable, or update the parent repo's submodule pointer to a
reachable commit (for example the submodule's current origin/main HEAD 9155d09)
by updating the submodule (git submodule update --init; cd vendor/bonsplit; git
fetch && git checkout <reachable-commit-or-main>), committing the changed
gitlink in the parent repo, and pushing the parent branch.
🪄 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: a5efef71-e770-47dc-8f8b-a81258f49922

📥 Commits

Reviewing files that changed from the base of the PR and between 9155d09 and f8dfdef.

📒 Files selected for processing (3)
  • Sources/ContentView.swift
  • cmuxTests/ShortcutAndCommandPaletteTests.swift
  • vendor/bonsplit

Comment thread vendor/bonsplit
@greptile-apps

greptile-apps Bot commented Apr 10, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR makes two independent fixes: it removes a chord-guard in ShortcutHintModifierPolicy.shouldShowHints so command-hold hint reveal is no longer blocked when the workspace-by-number shortcut is a chorded remap, and it strips the hint-pill width reservation from SidebarTrailingAccessoryWidthPolicy.width() so the trailing ZStack no longer pre-allocates layout space for shortcut hints (preventing title ellipsis). A regression test for the chorded-remap case and an updated bonsplit submodule (pane-hint command-hold policy) round out the change.

Confidence Score: 5/5

Safe to merge — targeted, well-tested fixes with no regressions identified.

Both changes are narrowly scoped: removing a two-line chord guard and stripping unused parameters from a layout helper. The existing Equatable conformance, showsWorkspaceShortcutHint rendering, and settings snapshot are all still intact. The new regression test follows the established pattern (isolated UserDefaults suite + defer restore) and directly validates the fixed behavior. No P0/P1 issues found.

No files require special attention.

Important Files Changed

Filename Overview
Sources/ContentView.swift Two targeted edits: drop chord guard from shouldShowHints and simplify SidebarTrailingAccessoryWidthPolicy.width to stop pre-reserving layout space for hint pills; showsWorkspaceShortcutHint/workspaceShortcutLabel are still used in the view body for visual rendering.
cmuxTests/ShortcutAndCommandPaletteTests.swift Adds testShortcutHintStaysCommandOnlyWhenWorkspaceShortcutUsesChord, covering the chorded-remap regression; follows existing test pattern with isolated UserDefaults suite and defer-based shortcut restoration.
vendor/bonsplit Submodule pointer bumped from 098d9fa to 7cd3b5e for pane-hint command-hold reveal policy and associated tests in bonsplit.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[User holds Command key] --> B[ShortcutHintModifierPolicy.shouldShowHints]
    B --> C{normalized == command only?}
    C -- No --> D[return false — no hints]
    C -- Yes --> E{showHintsOnCommandHold enabled?}
    E -- No --> D
    E -- Yes --> F[return true — show hints]

    F --> G[TabItemView renders hints]
    G --> H{showsWorkspaceShortcutHint?}
    H -- Yes --> I[Render hint pill in ZStack\noverflows layout — no title truncation]
    H -- No --> J{showCloseButton?}
    J -- Yes --> K[Render close button\nwidth = 16px]
    J -- No --> L[trailingAccessoryWidth = 0]

    style D fill:#f66,color:#fff
    style F fill:#6a6,color:#fff
    style I fill:#69f,color:#fff
Loading

Reviews (1): Last reviewed commit: "Fix command-hold shortcut hints and keep..." | Re-trigger Greptile

@cubic-dev-ai cubic-dev-ai 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.

No issues found across 3 files

@lawrencecchen
lawrencecchen merged commit fae33ca into main Apr 10, 2026
24 checks passed
@lawrencecchen
lawrencecchen deleted the feat-cmd-hold-shows-all-hints branch April 10, 2026 01:23
rodchristiansen pushed a commit to rodchristiansen/cmux that referenced this pull request Sep 2, 2026
…ows-all-hints

Fix command-hold shortcut hints and prevent sidebar truncation

This branch was successfully deployed

1 active deployment
Preview — f8dfdefa Deployed Apr 10, 2026 by vercel[bot]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant