Skip to content

Conversation

@zemaj
Copy link

@zemaj zemaj commented Oct 30, 2025

Summary

  • promote /exit to a first-class slash command (aliasing /quit) and keep dispatch wiring intact
  • normalize the parser so /exit preserves its spelling while still accepting /quit
  • add parser + ChatWidget harness coverage and document /exit in docs/slash-commands.md

Testing

  • ./build-fast.sh
  • cargo test -p code-tui slash_exit_and_quit_dispatch_exit_command (fails: local cargo registry copy of cc 1.2.41 is missing generated modules; clear/update the crate and rerun)

Closes openai#5932.

Copy link

@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 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".

Comment on lines +205 to 207
pub fn drain_events(&self) -> Vec<AppEvent> {
let mut out = Vec::new();
while let Ok(ev) = self.events.try_recv() {

Choose a reason for hiding this comment

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

P0 Badge 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 👍 / 👎.

Copy link
Author

@zemaj zemaj left a comment

Choose a reason for hiding this comment

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

✅ Reviewed locally in an isolated worktree.

Key notes:

  • Tightened process_slash_command_message so /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::Exit inside submit_user_message so 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 /exit to the session history; added harness tests covering both suppressed and normal /exit flows 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

"/exit" support, I'm sick of opening "/feedback"

2 participants