Conversation
There was a problem hiding this comment.
💡 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 "@codex address that feedback".
| pub fn drain_events(&self) -> Vec<AppEvent> { | ||
| let mut out = Vec::new(); | ||
| while let Ok(ev) = self.events.try_recv() { |
There was a problem hiding this comment.
Make drain_events public without exporting AppEvent
Changing ChatWidgetHarness::drain_events to pub exposes a return type of Vec<AppEvent> while AppEvent itself remains pub(crate). When the crate is built with the test-helpers feature (required for the newly added integration test), ChatWidgetHarness is re‑exported and this method becomes part of the public API, causing error[E0446]: private type AppEvent in public interface and the crate no longer compiles. Either keep the method crate‑visible or publicly re‑export AppEvent so the public signature uses a public type.
Useful? React with 👍 / 👎.
zemaj
left a comment
There was a problem hiding this comment.
✅ Reviewed locally in an isolated worktree.
Key notes:
- Tightened
process_slash_command_messageso/exit(and/quit) only trigger with a leading slash, reject extra args, and normalise casing—bare words now fall back to regular chat text. - Guarded
SlashCommand::Exitinsidesubmit_user_messageso synthetic/suppressed submissions (Auto Drive, CLI reroutes) can no longer exit the app, and skipped history persistence/dispatch in that path. - Updated the App dispatcher to avoid adding
/exitto the session history; added harness tests covering both suppressed and normal/exitflows plus parser edge cases. - Build + targeted unit tests:
cargo test -p code-tui --lib slash_exit_and_quit_dispatch_exit_command,cargo test -p code-tui --lib exit_command_ignored_for_suppressed_message,cargo test -p code-tui --lib exit_command_dispatches_without_history_persistence, and./build-fast.sh.
Follow-up: full app-level integration coverage of the exit path is still missing; if UX ever needs confirmation/cleanup semantics, it should be added alongside an integration test.
Summary
/exitto a first-class slash command (aliasing/quit) and keep dispatch wiring intact/exitpreserves its spelling while still accepting/quit/exitindocs/slash-commands.mdTesting
cc1.2.41 is missing generated modules; clear/update the crate and rerun)Closes openai#5932.