Support Ctrl-N/Ctrl-P in cmux themes on main#4474
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughRepoints Ghostty to a new pinned commit adding Ctrl-N/Ctrl-P navigation to the cmux theme picker, updates the xcframework checksum and fork documentation/changelog, and extends the bundled picker test to exercise and validate Ctrl-N/Ctrl-P scenarios. ChangesGhostty fork update with keyboard navigation support
🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs:
🚥 Pre-merge checks | ✅ 17✅ Passed checks (17 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 SummaryThis PR adds Ctrl-N (down) and Ctrl-P (up) one-row navigation to the cmux theme picker by advancing the pinned
Confidence Score: 5/5Safe to merge — changes are confined to the submodule pointer, its pinned checksum, documentation, and test scaffolding with no production Swift or runtime code altered. The diff touches only the Ghostty submodule pointer, the corresponding prebuilt checksum, documentation, and a test-only shell/Python script. No production Swift, no runtime logic, and no app behaviour is changed directly. The test additions are well-structured and the XDG isolation is a clear improvement over the previous approach. No files require special attention beyond confirming the new GhosttyKit checksum matches the published artifact. Important Files Changed
Sequence DiagramsequenceDiagram
participant Test as test_bundled_ghostty_theme_picker_helper.sh
participant PY as Python inline script
participant Helper as ghostty helper (+list-themes)
participant PTY as PTY subprocess
Test->>PY: launch python3 heredoc
PY->>Helper: +list-themes --plain (subprocess.run)
Helper-->>PY: theme names list
PY->>PY: parse first_theme, second_theme
Note over PY,PTY: Ctrl-N navigation
PY->>PTY: pty.fork → +list-themes
PTY-->>PY: render (waits for Enter apply)
PY->>PTY: send Ctrl-N + Enter
PTY-->>PY: exit 0
PY->>PY: assert_theme_written(second_theme)
Note over PY,PTY: Ctrl-P navigation
PY->>PTY: pty.fork → +list-themes
PTY-->>PY: render (waits for Enter apply)
PY->>PTY: send Ctrl-N + Ctrl-P + Enter
PTY-->>PY: exit 0
PY->>PY: assert_theme_written(first_theme)
PY->>Test: write results_path
Test->>Test: verify config file structure
Reviews (3): Last reviewed commit: "test: time out theme list discovery" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 `@tests/test_bundled_ghostty_theme_picker_helper.sh`:
- Around line 117-124: The subprocess.run call that sets plain_result (invoking
[helper_path, "+list-themes", "--plain"] with
env=helper_environment(config_path)) can block and hang CI; add a timeout
parameter to that subprocess.run invocation (e.g., timeout=<reasonable_seconds>)
and handle subprocess.TimeoutExpired by failing the test with a clear message or
raising the exception so the test suite does not hang indefinitely.
🪄 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: d373d83a-01f7-4697-9846-df89ccdd9f1b
📒 Files selected for processing (4)
docs/ghostty-fork.mdghosttyscripts/ghosttykit-checksums.txttests/test_bundled_ghostty_theme_picker_helper.sh
lawrencecchen
force-pushed
the
followup-themes-main-full
branch
from
f283c43 to
1d9b4b1
Compare
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 4 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="ghostty">
<violation number="1" location="ghostty:1">
P2: Submodule commit `176bd550f6fedd29e85cd92470e5dfadf295ebf7` is not on the fork's `main` branch, violating the repository convention that submodule commits must be pushed to main before updating the parent pointer. The commit is only on feature branch `issue-themes-broken-ctrl-np`, which could become unreachable if that branch is deleted. Push the commit to the fork's `main` branch (or merge it) before updating the parent submodule pointer.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| @@ -1 +1 @@ | |||
| Subproject commit ff6e1260d2e7767de55b8d9307b328e4060545b7 | |||
| Subproject commit 176bd550f6fedd29e85cd92470e5dfadf295ebf7 | |||
There was a problem hiding this comment.
P2: Submodule commit 176bd550f6fedd29e85cd92470e5dfadf295ebf7 is not on the fork's main branch, violating the repository convention that submodule commits must be pushed to main before updating the parent pointer. The commit is only on feature branch issue-themes-broken-ctrl-np, which could become unreachable if that branch is deleted. Push the commit to the fork's main branch (or merge it) before updating the parent submodule pointer.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At ghostty, line 1:
<comment>Submodule commit `176bd550f6fedd29e85cd92470e5dfadf295ebf7` is not on the fork's `main` branch, violating the repository convention that submodule commits must be pushed to main before updating the parent pointer. The commit is only on feature branch `issue-themes-broken-ctrl-np`, which could become unreachable if that branch is deleted. Push the commit to the fork's `main` branch (or merge it) before updating the parent submodule pointer.</comment>
<file context>
@@ -1 +1 @@
-Subproject commit ff6e1260d2e7767de55b8d9307b328e4060545b7
+Subproject commit 176bd550f6fedd29e85cd92470e5dfadf295ebf7
</file context>
There was a problem hiding this comment.
Valid dependency ordering issue. I retargeted manaflow-ai/ghostty#60 to main; this parent PR should merge only after that Ghostty PR is merged following dogfood approval.
— Claude Code
There was a problem hiding this comment.
Thanks for confirming and handling the merge order.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
tests/test_bundled_ghostty_theme_picker_helper.sh (1)
117-124:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd a timeout to plain-mode theme discovery.
At Line 117,
subprocess.runcan block indefinitely if+list-themes --plainhangs, which can stall CI before the PTY timeout path is reached.Suggested patch
-plain_result = subprocess.run( - [helper_path, "+list-themes", "--plain"], - check=True, - env=helper_environment(config_path), - stdout=subprocess.PIPE, - stderr=subprocess.PIPE, - text=True, -) +try: + plain_result = subprocess.run( + [helper_path, "+list-themes", "--plain"], + check=True, + env=helper_environment(config_path), + stdout=subprocess.PIPE, + stderr=subprocess.PIPE, + text=True, + timeout=10, + ) +except subprocess.TimeoutExpired: + sys.stderr.write("FAIL: timed out while listing bundled themes in plain mode\n") + sys.exit(1)🤖 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 `@tests/test_bundled_ghostty_theme_picker_helper.sh` around lines 117 - 124, The subprocess.run call that captures plain-mode theme discovery (plain_result) should include a timeout to avoid hanging CI; add a timeout argument (e.g., timeout=30) to the subprocess.run invocation that uses helper_path with "+list-themes" and helper_environment(config_path), and also catch subprocess.TimeoutExpired around that call to fail the test with a clear message/diagnostic rather than blocking indefinitely.
🤖 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.
Duplicate comments:
In `@tests/test_bundled_ghostty_theme_picker_helper.sh`:
- Around line 117-124: The subprocess.run call that captures plain-mode theme
discovery (plain_result) should include a timeout to avoid hanging CI; add a
timeout argument (e.g., timeout=30) to the subprocess.run invocation that uses
helper_path with "+list-themes" and helper_environment(config_path), and also
catch subprocess.TimeoutExpired around that call to fail the test with a clear
message/diagnostic rather than blocking indefinitely.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: d6552920-9b32-4f77-99db-d2a5492d106f
📒 Files selected for processing (4)
docs/ghostty-fork.mdghosttyscripts/ghosttykit-checksums.txttests/test_bundled_ghostty_theme_picker_helper.sh
1 participant
Summary
cmux themespicker breakage.docs/ghostty-fork.md.Testing
./tests/test_ci_ghosttykit_checksum_present.shGHOSTTY_SHA=176bd550f6fedd29e85cd92470e5dfadf295ebf7 GHOSTTYKIT_OUTPUT_DIR=/tmp/cmux-ghosttykit-npmain-check ./scripts/download-prebuilt-ghosttykit.sh./tests/test_ci_ghosttykit_checksum_verification.sh./scripts/reload.sh --tag npmainCMUX_APP_PATH="$HOME/Library/Developer/Xcode/DerivedData/cmux-npmain/Build/Products/Debug/cmux DEV npmain.app" tests/test_bundled_ghostty_theme_picker_helper.shRegression Structure
Related
Need help on this PR? Tag
@codesmithwith what you need. Autofix is disabled.Note
Medium Risk
Moderate risk because it bumps the bundled GhosttyKit/ghostty pin that affects runtime theme-picker behavior and distribution artifacts, though changes are scoped and covered by new PTY tests.
Overview
Advances the pinned Ghostty fork/GhosttyKit release to a new head that adds Ctrl-N/Ctrl-P one-row navigation in cmux-managed theme picker sessions, and updates the fork documentation accordingly.
Extends
test_bundled_ghostty_theme_picker_helper.shto run the picker in a PTY with an isolatedXDG_CONFIG_HOME, dynamically discover bundled themes, and assert that Ctrl-N selects the next theme and Ctrl-N then Ctrl-P returns to the original theme before Enter writes the expected override config.Reviewed by Cursor Bugbot for commit 2665899. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by cubic
Adds Ctrl‑N/Ctrl‑P one-line navigation to the cmux theme picker on
mainby updating theghosttyfork and pinning the matchingGhosttyKit. Strengthens PTY tests with dynamic theme discovery, a timeout for theme listing, and isolated config to verify the correct theme is written.New Features
+list-themes --plain, a discovery timeout,XDG_CONFIG_HOMEisolation, and asserts for normal, search, Ctrl‑N, and Ctrl‑P flows.Dependencies
ghosttysubmodule to the fork head that includes Ctrl‑N/Ctrl‑P.GhosttyKitrelease and checksum; updatedocs/ghostty-fork.mdwith the new tag and branch.Written for commit 2665899. Summary will update on new commits. Review in cubic
Summary by CodeRabbit
New Features
Documentation
Tests
Chores