-
Notifications
You must be signed in to change notification settings - Fork 7.4k
tui: stabilize shortcut overlay snapshots on WSL #9359
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
Gate WSL detection behind cfg(not(test)) so shortcut-overlay snapshots stay deterministic across environments. Add a focused unit test that still asserts the WSL-specific paste-image binding.
|
@codex review |
|
Codex Review: Didn't find any major issues. 🎉 ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
Fixes #9058 ## Summary When the transcript backtrack preview is armed (press `Esc`), allow navigating to newer user messages with the `→` arrow, in addition to navigating backwards with `Esc`/`←`, before confirming with `Enter`. ## Changes - Backtrack preview navigation: `Esc`/`←` steps to older user messages, `→` steps to newer ones, `Enter` edits the selected message (clamped at bounds, no wrap-around). - Transcript overlay footer hints updated to advertise `esc/←`, `→`, and `enter` when a message is highlighted. ## Related - WSL shortcut-overlay snapshot determinism: #9359 ## Testing - `just fmt` - `just fix -p codex-tui` - `just fix -p codex-tui2` - `cargo test -p codex-tui app_backtrack::` - `cargo test -p codex-tui pager_overlay::` - `cargo test -p codex-tui2 app_backtrack::` - `cargo test -p codex-tui2 pager_overlay::` --------- Co-authored-by: Josh McKinney <joshka@openai.com>
| //! hint. The owning widgets schedule redraws so time-based hints can expire even if the UI is | ||
| //! otherwise idle. | ||
| #[cfg(target_os = "linux")] | ||
| #[cfg(all(target_os = "linux", not(test)))] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't like having test related cfg checks in non-test code like this. It makes it difficult to reason about how the behavior works as different behavior under tests is usually not something that most people will anticipate. Likely this instead should be separate test behavior that triggers based on the test environment.
This probably means that we need to investing in actually setting up our CI to have a WSL specific test in the matrix.
Fixes #9361
Context
Split out from #9059 per review: #9059 (comment)
Summary
The shortcut overlay renders different paste-image bindings on WSL (Ctrl+Alt+V) vs non-WSL (Ctrl+V), which makes snapshot tests non-deterministic when run under WSL.
Changes
cfg(not(test))so snapshot tests are deterministic across environments.Testing
just fmtjust fix -p codex-tuijust fix -p codex-tui2cargo test -p codex-tuicargo test -p codex-tui2