Conversation
|
Thanks - can take a closer look once CI is not failing anymore. |
There was a problem hiding this comment.
Pull request overview
This PR addresses issue #241 by adding a configurable option to change the ESC key behavior from exiting the application to navigating to the parent directory. Users can now opt-in to this behavior by creating a configuration file at ~/.config/dua-cli/config.toml (respecting XDG_CONFIG_HOME) with [keys] esc_navigates_back = true. By default (when the config is absent or set to false), ESC retains its original quit behavior.
Changes:
- Added configuration file support with TOML parsing for key binding customization
- Modified ESC key behavior in main view to navigate to parent directory when configured
- Updated help text and exit prompts to reflect the conditional key bindings
Reviewed changes
Copilot reviewed 10 out of 11 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| src/config.rs | New module for loading and parsing TOML configuration from ~/.config/dua-cli/config.toml with support for XDG_CONFIG_HOME |
| src/main.rs | Added config module import and config loading before initializing the terminal app |
| src/interactive/app/state.rs | Added esc_navigates_back field to AppState and updated constructor to accept the configuration value |
| src/interactive/app/terminal.rs | Added esc_navigates_back parameter to initialize method and passed it to AppState |
| src/interactive/app/eventloop.rs | Modified ESC key handling to navigate back to parent directory when in main view and config is enabled, otherwise preserves original quit behavior |
| src/interactive/app/tests/utils.rs | Updated test utility to pass false for esc_navigates_back to maintain existing test behavior |
| src/interactive/widgets/help.rs | Updated help pane to display conditional key binding descriptions based on configuration |
| src/interactive/widgets/footer.rs | Updated exit prompt message to show appropriate key combinations based on configuration |
| src/interactive/widgets/main.rs | Passed esc_navigates_back state to help and footer widgets for conditional rendering |
| Cargo.toml | Added toml = "0.8" and dirs = "6" dependencies for configuration parsing and path resolution |
| Cargo.lock | Added dependency trees for toml, dirs, and their transitive dependencies |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7c58e0a6d5
ℹ️ 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 "@codex address that feedback".
The comment states "standard for CLI tools on all platforms" but this is inaccurate. On Windows, the standard location for configuration files is in the AppData folder (e.g., %APPDATA% or %LOCALAPPDATA%), not ~/.config. While this code will work on Windows if users manually create the .config directory in their home folder, it doesn't follow Windows conventions. Consider using the dirs crate's config_dir() function instead of manually constructing ~/.config, which will use the appropriate platform-specific directory (e.g., %APPDATA% on Windows, ~/.config on Linux, ~/Library/Application Support on macOS). The check !glob_focussed is redundant here because self.focussed == Main already ensures we're not in Glob mode (since glob_focussed = self.focussed == Glob on line 250). While this doesn't cause incorrect behavior and may serve as defensive programming, consider removing it for clarity since the condition self.focussed == Main already guarantees !glob_focussed. Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|
Thanks a lot! I am completing the PR and will merge. Regarding the configuration location, instead of altering the defaults, I am adding |
Byron
force-pushed
the
feat/disable-esc
branch
from
16080d2 to
30c472c
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 30c472c8c1
ℹ️ 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 "@codex address that feedback".
Byron
force-pushed
the
feat/disable-esc
branch
from
30c472c to
f275703
Compare
2 participants
We disable esc as the exit key. User can still exit with q/C-c
We also add config file to
~/.config/dua-cli/config.toml, where user can set#241