Repository navigation
Fix cmux theme picker Enter from search - #3378
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughUpdates the pinned Ghostty fork revision and cmux theme picker search-mode Enter behavior documentation. Adjusts GhosttyKit checksum mapping. Refactors theme picker test harness to validate both normal and search mode scenarios with additional configuration paths. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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. Review rate limit: 5/8 reviews remaining, refill in 21 minutes and 10 seconds.Comment |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Greptile SummaryThis PR fixes the cmux theme picker failing to apply a theme when Enter is pressed from search mode. It bumps the Ghostty submodule to the new fork commit Confidence Score: 4/5Safe to merge; changes are a targeted fork bump with accompanying test coverage and documentation — no logic regressions expected. All four changed files are consistent with each other: the submodule SHA, checksum file, documentation, and test all agree on the new Ghostty commit. The refactored test correctly exercises both normal-mode and search-mode Enter paths. Only minor P2 observations (hardcoded search term, shared trigger condition) prevent a perfect score. tests/test_bundled_ghostty_theme_picker_helper.sh — the search-mode scenario relies on 'tokyo' matching at least one bundled theme; worth a quick sanity-check if themes are ever pruned. Important Files Changed
Sequence DiagramsequenceDiagram
participant Shell as test script (bash)
participant PY as Python PTY harness
participant PTY as PTY master fd
participant GH as ghostty +list-themes
Shell->>PY: run_picker("normal mode", config_path, CR)
PY->>GH: pty.fork() + execve
GH-->>PTY: renders picker UI ("Enter apply" visible)
PTY-->>PY: output accumulates
PY->>PTY: write(CR) on "Enter apply" trigger
GH-->>PTY: writes config_path, exits 0
PY-->>Shell: returns (success)
Shell->>PY: run_picker("search mode", search_config_path, "/tokyo\r")
PY->>GH: pty.fork() + execve
GH-->>PTY: renders picker UI ("Enter apply" visible)
PTY-->>PY: output accumulates
PY->>PTY: write("/tokyo\r") on "Enter apply" trigger
Note over PTY,GH: "/" enters search mode, "tokyo" filters list, CR applies selection
GH-->>PTY: writes search_config_path, exits 0
PY-->>Shell: returns (success)
Shell->>Shell: write results_path listing both config paths
loop for each path in results_path
Shell->>Shell: assert file exists
Shell->>Shell: grep cmux themes start/end markers
Shell->>Shell: grep theme = light:...,dark:...
end
Shell->>Shell: echo PASS
Reviews (1): Last reviewed commit: "Apply cmux theme from picker search" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
|
||
|
|
||
| run_picker("normal mode", config_path, b"\r") | ||
| run_picker("search mode", search_config_path, b"/tokyo\r") |
There was a problem hiding this comment.
b"/tokyo\r" assumes at least one bundled theme name contains "tokyo". If the theme list is ever reorganised and no match exists, Enter on an empty filtered list may not write the override file — the test would then fail on the existence check, which is the right outcome, but there is no signal about why it failed. A comment explaining the chosen term (e.g. "tokyo matches 'Tokyo Night' and variants, confirmed present in bundled themes") would make future regressions easier to diagnose.
| if not sent_input and b"Enter apply" in output: | ||
| os.write(master_fd, scripted_input) | ||
| sent_input = True |
There was a problem hiding this comment.
Both run_picker calls use the same trigger: b"Enter apply" in output. For the search-mode scenario this is fine because "Enter apply" appears in the initial footer before / is sent. However, if a future Ghostty version changes the footer text in search mode (e.g. to "Enter apply search result"), the trigger would still fire on the initial normal-mode text — making the test structurally valid but potentially masking a regression. Accepting a trigger parameter alongside scripted_input would make each scenario's intent explicit.
This branch was successfully deployed
1 participant
Summary
cmux themesapplying from picker search mode with Enter.Testing
bash -n tests/test_bundled_ghostty_theme_picker_helper.sh./scripts/reload.sh --tag thmenterGHOSTTY_SHA=4265d34282ce2023c27da851c454dabe6cdc76ce GHOSTTYKIT_OUTPUT_DIR=<tmp>/GhosttyKit.xcframework ./scripts/download-prebuilt-ghosttykit.shIssues
Summary by CodeRabbit
Documentation
Chores
Tests