Support Ctrl-N/Ctrl-P in cmux themes#4404
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ 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 advances the pinned Ghostty fork submodule to
Confidence Score: 5/5Safe to merge — no production Swift or runtime code is touched; changes are limited to a submodule bump, its matching checksum, documentation, and new PTY test scenarios. The submodule advance, checksum update, and docs are consistent with each other and follow the established pattern in this repo. The new PTY test scenarios correctly exercise the Ctrl-N/Ctrl-P paths, use isolated XDG_CONFIG_HOME to avoid real-user-config contamination, and assert the exact theme name written to the config file. No production code paths are modified. No files require special attention. Important Files Changed
Sequence DiagramsequenceDiagram
participant Shell as test script (bash)
participant Py as Python harness
participant Helper as ghostty helper binary
Shell->>Py: launch python3 heredoc
Py->>Helper: +list-themes --plain (isolated XDG_CONFIG_HOME)
Helper-->>Py: sorted theme list stdout
Py->>Py: parse theme_names[0], theme_names[1]
Note over Py,Helper: normal mode scenario
Py->>Helper: pty.fork() + execve(+list-themes)
Helper-->>Py: renders picker ("Enter apply" visible)
Py->>Helper: write(b"\r")
Helper-->>Py: exits 0, writes config_path
Note over Py,Helper: Ctrl-N scenario
Py->>Helper: pty.fork() + execve(+list-themes)
Helper-->>Py: renders picker ("Enter apply" visible)
Py->>Helper: write(b"\x0e\r") [Ctrl-N, Enter]
Helper-->>Py: exits 0, writes ctrl_n_config_path
Py->>Py: assert_theme_written(second_theme)
Note over Py,Helper: Ctrl-P scenario
Py->>Helper: pty.fork() + execve(+list-themes)
Helper-->>Py: renders picker ("Enter apply" visible)
Py->>Helper: write(b"\x0e\x10\r") [Ctrl-N, Ctrl-P, Enter]
Helper-->>Py: exits 0, writes ctrl_p_config_path
Py->>Py: assert_theme_written(first_theme)
Py->>Shell: write results_path with all 4 config paths
Shell->>Shell: grep-validate markers + theme line in each config
Reviews (2): Last reviewed commit: "fix: support ctrl navigation in cmux the..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| theme_names = sorted( | ||
| ( | ||
| entry.name | ||
| for entry in os.scandir(theme_dir) | ||
| if entry.is_file() or entry.is_symlink() | ||
| ), | ||
| key=lambda name: name.lower(), | ||
| ) | ||
| if len(theme_names) < 2: | ||
| sys.stderr.write(f"FAIL: expected at least two bundled themes in {theme_dir}\n") | ||
| sys.exit(1) |
There was a problem hiding this comment.
os.scandir returns entry.name including any file extension. If the bundled themes directory ever contains files named ThemeName.ghostty instead of plain ThemeName, theme_names[0] and theme_names[1] would carry the .ghostty suffix, causing assert_theme_written to compare against theme = light:ThemeName.ghostty,dark:ThemeName.ghostty — a line the picker would never write — and the Ctrl-N/Ctrl-P assertions would always fail. The existing Ghostty bundled themes use bare names so this is currently safe, but an explicit os.path.splitext(entry.name)[0] (or entry.name.removesuffix(".ghostty")) would make the intent explicit and guard against a future themes-directory restructure.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
lawrencecchen
force-pushed
the
followup-themes-ctrl-np
branch
from
5827c1e to
f283c43
Compare
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Superseded by the main-targeted follow-up PR: #4474. |
1 participant
Summary
cmux themesCtrl-N and Ctrl-P navigation.Why
cmux themes setreloads and interactive picker reload behavior.Testing
./tests/test_ci_ghosttykit_checksum_present.shGHOSTTY_SHA=176bd550f6fedd29e85cd92470e5dfadf295ebf7 GHOSTTYKIT_OUTPUT_DIR=/tmp/cmux-ghosttykit-followup-check ./scripts/download-prebuilt-ghosttykit.sh./tests/test_ci_ghosttykit_checksum_verification.sh./scripts/reload.sh --tag ctrlnpCMUX_APP_PATH="$HOME/Library/Developer/Xcode/DerivedData/cmux-ctrlnp/Build/Products/Debug/cmux DEV ctrlnp.app" tests/test_bundled_ghostty_theme_picker_helper.shRegression Structure
Dogfood
ctrlnp.cmux themes, press Ctrl-N, then Enter. Expected: selection moves down and applies that highlighted theme.cmux themesagain, press Ctrl-N then Ctrl-P, then Enter. Expected: selection returns up and applies that highlighted theme.Need help on this PR? Tag
@codesmithwith what you need.Note
Medium Risk
Moderate risk because it updates the pinned
ghosttyfork artifact/checksum and relies on PTY-driven integration behavior, which can be brittle across environments.Overview
Adds PTY-based regression coverage to ensure the bundled Ghostty theme picker helper supports Ctrl-N/Ctrl-P navigation and still writes/applies the highlighted theme on Enter.
Updates the pinned Ghostty fork reference/documentation and the
GhosttyKitprebuilt checksum/release tag to the new fork head (176bd550f) that includes the Ctrl keybinding fix.Reviewed by Cursor Bugbot for commit f283c43. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by cubic
Adds Ctrl-N/Ctrl-P navigation to the
cmux themespicker and verifies it end-to-end with PTY tests that assert the highlighted theme is applied. Updates theghosttyfork and pins the matchingGhosttyKitrelease and checksum, with refreshed fork docs.Bug Fixes
cmux themespicker; Enter applies the highlighted theme.XDG_CONFIG_HOME, and assert the expected theme is written.Dependencies
ghosttyand pin the matchingGhosttyKitbuild/checksum; update fork docs with the new release tag.Written for commit f283c43. Summary will update on new commits. Review in cubic