-
Notifications
You must be signed in to change notification settings - Fork 6.6k
fix: Prevent slash command popup from activating on invalid inputs #7704
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
…ke "/ test" - Updated `sync_command_popup` logic to ensure that the slash command popup only activates when: - The input starts with `/` and has no non-whitespace content after `/` (e.g., `/`). - The input starts with `/` and the command name is a valid prefix of a built-in command (e.g., `/re` for `/review`). - Prevents recalled history inputs like `/ test` from incorrectly triggering the command popup. - Ensures that `/` alone still activates the popup to display all available commands.
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.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| @@ -0,0 +1,40 @@ | |||
| # Recall Prompt History Issue | |||
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.
Please remove this file from the PR.
| assert_eq!(composer.textarea.text(), "z".repeat(count)); | ||
| assert!(composer.pending_pastes.is_empty()); | ||
| } | ||
|
|
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 think a test is needed in this case because there's little chance of a regression. We're trying to keep our CI test times from exploding, so we want to add tests only when they matter. Please remove.
|
Thanks for the contribution. I'm not that familiar with the code you're changing here, so I will ask a colleague to review the change. Before I pass it along for review, please address the comments I added. |
Slash Command popup issue
#7659
When recalling history, the composer(
codex_tui::bottom_pane::chat_composer) restores the previous prompt text (which may start with/) and then callssync_command_popup. The logic insync_command_popuptreats any first line that starts with/and has the caret inside the initial/nametoken as an active slash command name:This detection does not distinguish between an actual interactive slash command being typed and a normal historical prompt that happens to begin with
/. As a result, after history recall, the restored prompt like/ testis interpreted as an "editing command name" context and the slash-command popup is (re)activated. Onceactive_popupisActivePopup::Command, subsequentUpkey presses are handled byhandle_key_event_with_slash_popupinstead ofhandle_key_event_without_popup, so they no longer triggerhistory.navigate_up(...)and the session prompt history cannot be scrolled.