Repository navigation
Group tab context menu actions - #8208
lawrencecchen wants to merge 1 commit into
Conversation
|
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 (2)
📝 WalkthroughWalkthrough
ChangesTab context routing
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 25✅ Passed checks (25 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
Greptile SummaryGroups the tab context menu into intent-based sections (current tab, create, arrange, contextual, view/state, developer), adds a "Close Tab" item beside "Rename Tab", and routes context-menu closes through the shared
Confidence Score: 4/5The close-tab routing and shortcut wiring changes are tight and coherent; the main risk to verify before merging is the The new The Important Files Changed
Sequence Diagram%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
participant User
participant Bonsplit as Bonsplit (context menu)
participant Delegate as Workspace (BonsplitDelegate)
participant CloseReq as onTabCloseRequest
User->>Bonsplit: right-click → Close Tab
Bonsplit->>Delegate: didRequestTabContextAction(.close, tab)
Delegate-->>Bonsplit: break (no-op — bonsplit owns close)
Bonsplit->>CloseReq: "source = .contextMenu"
CloseReq->>Delegate: markTabStripMiddleClickClose(surfaceId)
Note over Delegate: tabStripCloseButtonByTabId[id] = false<br/>Pinned-tab / close-warning<br/>checks applied by bonsplit
User->>Bonsplit: right-click → Disconnect Remote
Bonsplit->>Delegate: didRequestTabContextAction(.disconnectRemote, tab)
Delegate->>Delegate: disconnectRemoteConnection(clearConfiguration: false)
Note over Delegate: Workspace-level disconnect,<br/>tab.id not used to guard
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
participant User
participant Bonsplit as Bonsplit (context menu)
participant Delegate as Workspace (BonsplitDelegate)
participant CloseReq as onTabCloseRequest
User->>Bonsplit: right-click → Close Tab
Bonsplit->>Delegate: didRequestTabContextAction(.close, tab)
Delegate-->>Bonsplit: break (no-op — bonsplit owns close)
Bonsplit->>CloseReq: "source = .contextMenu"
CloseReq->>Delegate: markTabStripMiddleClickClose(surfaceId)
Note over Delegate: tabStripCloseButtonByTabId[id] = false<br/>Pinned-tab / close-warning<br/>checks applied by bonsplit
User->>Bonsplit: right-click → Disconnect Remote
Bonsplit->>Delegate: didRequestTabContextAction(.disconnectRemote, tab)
Delegate->>Delegate: disconnectRemoteConnection(clearConfiguration: false)
Note over Delegate: Workspace-level disconnect,<br/>tab.id not used to guard
Reviews (1): Last reviewed commit: "Group tab context menu actions" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| case .disconnectRemote: | ||
| disconnectRemoteConnection(clearConfiguration: false) |
There was a problem hiding this comment.
disconnectRemoteConnection ignores tab.id
The handler disconnects the workspace-level SSH connection without verifying that the right-clicked tab.id corresponds to the panel that owns the remote session. For a workspace where the context menu is shown on a local (non-SSH) tab while a remote connection exists in a sibling pane, this silently disconnects the connection the user did not intend to affect. Other SSH-related guard patterns in this switch (e.g. .reload, .toggleAudioMute) validate via panelIdFromSurfaceId before acting. At minimum, consider gating with guard panelIdFromSurfaceId(tab.id) != nil or checking remoteConfiguration ownership before dispatching.
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!
| @@ -1 +1 @@ | |||
| Subproject commit 10563e2fda6fc18c47adf1864d55e0e25087a864 | |||
| Subproject commit da56587cd6f1a618f3cf1b96011f6765eb95f377 | |||
There was a problem hiding this comment.
The PR verification confirms plutil -lint for English and Japanese bonsplit string catalogs, but the cmux app ships strings across 20 locales (ar, bs, da, de, es, fr, it, km, ko, nb, pl, pt-BR, ru, th, tr, uk, zh-Hans, zh-Hant in addition to en and ja). Any new user-facing text in the bonsplit context menu (e.g. "Close Tab", grouping separators, "Disconnect") that is defined in bonsplit's own string catalog needs translated entries for every locale the catalog already supports. If bonsplit's catalog covers more than two locales, the missing translations will fall back to English at runtime for those locales, violating the full-internationalization rule.
Rule Used: Flag production user-facing text that is not fully... (source)
|
Superseded by the Rust cmux TUI implementation: #8225. This PR targeted the macOS tab strip by mistake. |
1 participant
Summary
Depends on manaflow-ai/bonsplit#186.
Verification
swift testinvendor/bonsplit: 203 tests passed.plutil -lint.cmux DEV tabgrp.Need help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.Summary by cubic
Reorganized the tab context menu by intent and routed context‑menu closes through the shared close path to keep pinned‑tab and close‑warning behavior consistent. Also adds Close Tab next to Rename Tab.
New Features
.closeto the.closeTabshortcut.Dependencies
vendor/bonsplit; depends onbonsplitPR Add native floating status bar + summarized voice notification system (Requesting Permission to Create PR) #186.Written for commit 4c82096. Summary will update on new commits.
Summary by CodeRabbit
New Features
Bug Fixes