Repository navigation
Add cmux Help menu resources - #3402
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
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 Help command group to the app with localized help/documentation links, feedback and update actions, and a Keyboard Shortcuts action; adds extensive English/Japanese localization entries; introduces a UI test for the Help menu; adjusts app activation policy when running under XCTest; adds skills installer script and web docs/pages for "Skills". Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant App as cmuxApp
participant Delegate as AppDelegate
participant System as System
User->>App: Open Help menu
App->>User: Display Help items (Docs, GitHub, Discord, Feedback, Updates, Shortcuts)
alt Open external doc/link
User->>App: Select doc/link
App->>System: openURL(resource.url)
else Send Feedback
User->>App: Select "Send Feedback"
App->>Delegate: routeToFeedbackComposer()
Delegate->>System: present feedback composer (ensure window)
else Keyboard Shortcuts
User->>App: Select "Keyboard Shortcuts"
App->>Delegate: openKeyboardShortcutsPrefs()
Delegate->>System: open Shortcuts preferences pane
else Check for Updates
User->>App: Select "Check for Updates"
App->>System: trigger update-check flow
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
Greptile SummaryThis PR adds a macOS Help menu with cmux documentation links, reusing existing Send Feedback, Check for Updates, and Keyboard Shortcuts actions modeled after the Codex app, backed by a new
Confidence Score: 3/5Not safe to merge as-is — the Help menu Send Feedback action silently fails when no window is active. One P1 logic bug (feedback silently dropped in the no-window edge case) pulls the score below the P1 ceiling of 4; combined with a P2 localization ambiguity the score lands at 3. Sources/cmuxApp.swift — presentFeedbackFromHelpMenu race condition Important Files Changed
Sequence DiagramsequenceDiagram
participant User
participant HelpMenu
participant cmuxApp
participant AppDelegate
participant FeedbackBridge
participant ContentView
User->>HelpMenu: Click "Send Feedback"
HelpMenu->>cmuxApp: presentFeedbackFromHelpMenu()
alt keyWindow or mainWindow exists
cmuxApp->>FeedbackBridge: openComposer(in: window)
FeedbackBridge->>ContentView: post feedbackComposerRequested(object: window)
ContentView->>ContentView: presentFeedbackComposer()
else No active window (race condition path)
cmuxApp->>AppDelegate: showMainWindowFromMenuBar()
Note over AppDelegate: bringToFront(window) async AppKit
cmuxApp->>cmuxApp: Task @MainActor fires before window is key
cmuxApp->>FeedbackBridge: openComposer(in: nil)
FeedbackBridge->>ContentView: post feedbackComposerRequested(object: nil)
ContentView->>ContentView: shouldHandleCommandPaletteRequest returns false
Note over ContentView: Silent failure — presentFeedbackComposer() never called
end
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| private func presentFeedbackFromHelpMenu() { | ||
| if let targetWindow = NSApp.keyWindow ?? NSApp.mainWindow { | ||
| FeedbackComposerBridge.openComposer(in: targetWindow) | ||
| return | ||
| } | ||
|
|
||
| AppDelegate.shared?.showMainWindowFromMenuBar() | ||
| Task { @MainActor in | ||
| FeedbackComposerBridge.openComposer(in: NSApp.keyWindow ?? NSApp.mainWindow) | ||
| } | ||
| } |
There was a problem hiding this comment.
When NSApp.keyWindow ?? NSApp.mainWindow is nil, showMainWindowFromMenuBar() calls bringToFront(_:) synchronously, but AppKit window-key state isn't updated until the next run-loop cycle — after the Task { @MainActor in } has already fired. So NSApp.keyWindow ?? NSApp.mainWindow is still nil inside the task, openComposer(in: nil) posts the notification with object: nil, and shouldHandleCommandPaletteRequest(…requestedWindow: nil, keyWindow: nil, mainWindow: nil) returns false for every observed window — silently swallowing the request. The pattern used in TerminalController.swift (lines 7937–7944) calls makeKeyAndOrderFront + activate before openComposer, which avoids this race. At minimum, openComposer should be called without an explicit argument here so its default NSApp.keyWindow ?? NSApp.mainWindow evaluation happens lazily on notification receipt (after more run-loop cycles), or the call should be deferred until windowDidBecomeKey fires.
There was a problem hiding this comment.
Fixed by having showMainWindowFromMenuBar return the concrete main window and passing that window to FeedbackComposerBridge, so the feedback request is routed before key-window state matters.
— Claude Code
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
cmuxUITests/SidebarHelpMenuUITests.swift (1)
68-73: ⚡ Quick winAdd an explicit assertion for “Check for Updates” in this new main Help menu test.
Since this PR also wires/reuses the Check for Updates action in the main Help menu, asserting its presence here would lock that contract down in the same test path.
✅ Suggested small test addition
XCTAssertTrue(app.menuItems["cmux Documentation"].waitForExistence(timeout: 2.0)) XCTAssertTrue(app.menuItems["What's New"].waitForExistence(timeout: 2.0)) XCTAssertTrue(app.menuItems["Codex Integration"].waitForExistence(timeout: 2.0)) XCTAssertTrue(app.menuItems["Automation & API"].waitForExistence(timeout: 2.0)) XCTAssertTrue(app.menuItems["Send Feedback"].waitForExistence(timeout: 2.0)) + XCTAssertTrue(app.menuItems["Check for Updates"].waitForExistence(timeout: 2.0))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmuxUITests/SidebarHelpMenuUITests.swift` around lines 68 - 73, The Help menu UI test in SidebarHelpMenuUITests is missing an assertion for the "Check for Updates" item; add a line mirroring the other checks that calls XCTAssertTrue(app.menuItems["Check for Updates"].waitForExistence(timeout: 2.0)) inside the same test method so the main Help menu wiring for Check for Updates is validated alongside "cmux Documentation", "What's New", "Codex Integration", "Automation & API", and "Send Feedback".
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@cmuxUITests/SidebarHelpMenuUITests.swift`:
- Around line 68-73: The Help menu UI test in SidebarHelpMenuUITests is missing
an assertion for the "Check for Updates" item; add a line mirroring the other
checks that calls XCTAssertTrue(app.menuItems["Check for
Updates"].waitForExistence(timeout: 2.0)) inside the same test method so the
main Help menu wiring for Check for Updates is validated alongside "cmux
Documentation", "What's New", "Codex Integration", "Automation & API", and "Send
Feedback".
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9d6b5d60-f487-496a-82b1-b4c32a2cd623
📒 Files selected for processing (3)
Resources/Localizable.xcstringsSources/cmuxApp.swiftcmuxUITests/SidebarHelpMenuUITests.swift
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1a4728ed8a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| XCTAssertTrue(app.menuItems["cmux Documentation"].waitForExistence(timeout: 2.0)) | ||
| XCTAssertTrue(app.menuItems["What's New"].waitForExistence(timeout: 2.0)) | ||
| XCTAssertTrue(app.menuItems["Codex Integration"].waitForExistence(timeout: 2.0)) | ||
| XCTAssertTrue(app.menuItems["Automation & API"].waitForExistence(timeout: 2.0)) | ||
| XCTAssertTrue(app.menuItems["Send Feedback"].waitForExistence(timeout: 2.0)) |
There was a problem hiding this comment.
The new UI test hard-codes English Help menu labels ("cmux Documentation", "What's New", etc.) but does not pin -AppleLanguages/-AppleLocale at launch, so it will fail on non-English runners even when the feature works. This file already uses locale pinning for other string-sensitive checks (for example in testCmdShiftPCheckQueryPrefersCheckForUpdatesBeforeAttemptUpdate), so this test should do the same or compare against localized expectations.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Pinned the Help menu UI test to en_US so English menu assertions are deterministic across localized runners.
— Claude Code
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmuxUITests/SidebarHelpMenuUITests.swift`:
- Around line 62-99: The test uses hard-coded English menu titles in
testMainHelpMenuShowsCmuxResourcesAndOpensKeyboardShortcuts which will fail on
localized runners; before launching the app (e.g., right after creating
XCUIApplication() and before launchAndActivate) set a stable locale by adding
app.launchArguments += ["-AppleLanguages", "(en)", "-AppleLocale", "en_US"] or
switch the menu lookups to use accessibility identifiers (set identifiers for
"Help", "cmux Documentation", "Keyboard Shortcuts", etc., in the app and replace
app.menuBars.menuBarItems[...] / app.menuItems[...] string lookups with those
identifiers) so the test is deterministic across locales; update requireElement
calls to use the chosen identifiers or keep the added AppleLanguages/AppleLocale
args if you prefer pinning to English.
- Around line 258-290: The final readiness check in launchAndActivate allows a
backgrounded app if any window exists, which can make menuBar access fail;
change the last sidebarHelpPollUntil predicate to require app.state ==
.runningForeground (remove the alternative app.windows.firstMatch.exists) so the
method only returns once the app is actually foreground-activated; update any
related timeout/assert message if needed to reflect that the app must be
runningForeground and keep using sidebarHelpPollUntil and app.activate() logic
as-is.
🪄 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: 9eaac204-6673-4e18-8095-33bdc0eb40a9
📒 Files selected for processing (1)
cmuxUITests/SidebarHelpMenuUITests.swift
There was a problem hiding this comment.
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 `@skills.sh`:
- Around line 153-157: The code uses "$dest_dir/$skill_name" in rm and mv which
can act on "/" if dest_dir is empty; guard against an empty dest_dir by
enforcing a non-empty value before using it (e.g., use the parameter expansion
form to fail on empty) so tmp_target, dest_dir and skill_name operations cannot
target the root; add the check at the start of the block that creates tmp_target
(or immediately before rm -rf "$dest_dir/$skill_name" and mv "$tmp_target"
"$dest_dir/$skill_name") to validate dest_dir is non-empty and bail with a clear
error if it is.
🪄 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: d2406229-f2fe-4a6b-bd0f-2e907df89a5f
📒 Files selected for processing (9)
Resources/Localizable.xcstringsSources/cmuxApp.swiftcmuxUITests/SidebarHelpMenuUITests.swiftskills.shweb/app/[locale]/components/docs-nav-items.tsweb/app/[locale]/docs/skills/page.tsxweb/app/sitemap.tsweb/messages/en.jsonweb/messages/ja.json
✅ Files skipped from review due to trivial changes (4)
- web/app/[locale]/components/docs-nav-items.ts
- web/messages/ja.json
- Resources/Localizable.xcstrings
- web/app/sitemap.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- Sources/cmuxApp.swift
e49df5a to
4f7b584
Compare
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
4f7b584 to
c7a50d0
Compare
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fdff976a0a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| script_dir="$(cd -- "$(dirname -- "${BASH_SOURCE[0]}")" >/dev/null 2>&1 && pwd || true)" | ||
| if [[ -z "$source_dir" && -n "$script_dir" && -d "$script_dir/skills" ]]; then | ||
| source_dir="$script_dir/skills" |
There was a problem hiding this comment.
When this script is executed via curl ... | bash, BASH_SOURCE[0] is empty, so dirname resolves to . and script_dir becomes the caller’s current directory. If that directory happens to contain a skills/ folder, the installer silently uses local files instead of downloading from GitHub, so --ref no longer controls what gets installed. This makes piped installs nondeterministic and can install stale or unrelated skills depending on where the command is run.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed by only using local source discovery when BASH_SOURCE[0] points to an actual script file. Piped installs now fall through to the GitHub archive unless --source is explicit.
— Claude Code
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
This branch was successfully deployed
1 participant
Summary
/docs/skillswith the checked-in skills andskills.shinstall flow.skills.shfor local or GitHub-based skill installation.Testing
SKIP_ENV_VALIDATION=1 bun run build./skills.sh --source . --dest /tmp/cmux-skills-install-test --skill cmux --skill cmux-browserpython3 scripts/swift_file_length_budget.py --budget .github/swift-file-length-budget.tsv./scripts/reload.sh --tag helpmenuHelpMenuUITests/testMainHelpMenuShowsCmuxResourcesAndOpensKeyboardShortcuts: https://github.com/manaflow-ai/cmux/actions/runs/25235283663