-
Notifications
You must be signed in to change notification settings - Fork 5.5k
Add "? for shortcuts" #4316
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
Add "? for shortcuts" #4316
Conversation
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
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
- 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
pub(crate) use super::shortcut_bindings::FooterMode; | ||
pub(crate) use super::shortcut_bindings::FooterProps; | ||
pub(crate) use super::shortcut_bindings::ShortcutAction; | ||
pub(crate) use super::shortcut_bindings::ShortcutContext; | ||
pub(crate) use super::shortcut_bindings::classify_key; | ||
pub(crate) use super::shortcut_bindings::effective_mode; | ||
pub(crate) use super::shortcut_bindings::esc_hint_mode; | ||
pub(crate) use super::shortcut_bindings::footer_height; | ||
pub(crate) use super::shortcut_bindings::prompt_mode; | ||
pub(crate) use super::shortcut_bindings::render_footer; | ||
pub(crate) use super::shortcut_bindings::reset_mode_after_activity; | ||
pub(crate) use super::shortcut_bindings::toggle_shortcut_mode; |
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.
Introduced footer module references without implementation
The rewritten footer now re‑exports FooterProps
, toggle_shortcut_mode
, render_footer
, etc. from super::shortcut_bindings
, and mod shortcut_bindings;
was added to the module tree. However there is no shortcut_bindings
module anywhere in bottom_pane
, so these symbols are unresolved. The crate will fail to compile with could not find module 'shortcut_bindings'
and missing type definitions.
Useful? React with 👍 / 👎.
77ed699
to
7fd738d
Compare
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.
Pull Request Overview
Adds a "? for shortcuts" help interface to the terminal UI, replacing the previous static shortcut display with a toggleable system that shows minimal prompts by default and detailed shortcuts on demand.
- Replaces static footer shortcuts with a toggleable "? for shortcuts" prompt system
- Removes unused key hint utilities and token usage display from footer
- Implements state management for different footer modes (prompt, overlay, escape hints, ctrl+c reminders)
Reviewed Changes
Copilot reviewed 25 out of 25 changed files in this pull request and generated 2 comments.
Show a summary per file
File | Description |
---|---|
codex-rs/tui/src/key_hint.rs |
Removes unused key hint helper functions for ctrl, shift, and plain text |
codex-rs/tui/src/bottom_pane/footer.rs |
Major refactor to implement toggleable shortcut system with multiple footer modes |
codex-rs/tui/src/bottom_pane/chat_composer.rs |
Integrates footer mode state management and "?" key handling |
codex-rs/tui/src/bottom_pane/mod.rs |
Removes token usage tracking functionality |
codex-rs/tui/src/chatwidget.rs |
Removes token usage display calls |
Multiple snapshot files | Updates test snapshots to reflect new footer display format |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
let pad_width = target_width + 2; | ||
line.push_str(&format!("{entry:<pad_width$}")); |
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.
The magic number 2
for padding width is not explained. Consider adding a comment or constant to clarify this represents the spacing between columns.
Copilot uses AI. Check for mistakes.
for y in 0..area.height { | ||
let row = row_to_string(y); | ||
if row.contains(" send") { | ||
if row.contains("? for shortcuts") { |
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.
This test assertion relies on a hard-coded string that should match the actual footer text. Consider extracting this string to a constant or using a more robust matching approach to prevent test brittleness.
Copilot uses AI. Check for mistakes.
@codex review this |
Codex Review: Didn't find any major issues. You're on a roll. ℹ️ 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 |
Screen.Recording.2025-09-26.at.5.39.45.PM.mov