Skip to content

Conversation

aibrahim-oai
Copy link
Collaborator

Esc should have other functionalities when it's not used in a backtracking situation. i.e. to cancel pop up menu when selecting model/approvals or to interrupt an active turn.

Copy link
Contributor

@chatgpt-codex-connector chatgpt-codex-connector bot left a 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 suggestions.

Reply with @codex fix comments to fix any unresolved comments.

About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you open a pull request for review, mark a draft as ready, or comment "@codex review". If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex fix this CI failure" or "@codex address that feedback".

Comment on lines 318 to +326
popup.move_down();
(InputResult::None, true)
}
KeyEvent {
code: KeyCode::Esc, ..
} => {
// Dismiss the slash popup; keep the current input untouched.
self.active_popup = ActivePopup::None;
(InputResult::None, true)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P1] Esc dismissal of slash popup reopened immediately

In handle_key_event_with_slash_popup the Esc branch clears active_popup (lines 321‑327) to hide the slash command menu. But handle_key_event then unconditionally calls sync_command_popup (lines 280‑290), which recreates the popup whenever the text still starts with '/'. Thus Esc cannot actually close the popup, contradicting the intended behaviour.

Useful? React with 👍 / 👎.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we want to esc hide pop up on the slash or not?
@edward-bayes @easong-openai

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ideally yes - sometimes you just want a /

@aibrahim-oai aibrahim-oai merged commit 907afc9 into main Aug 25, 2025
25 of 27 checks passed
@aibrahim-oai aibrahim-oai deleted the esc-replace branch August 25, 2025 22:38
@github-actions github-actions bot locked and limited conversation to collaborators Aug 25, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants