feat(tui): add automation apply and rules editing#30
Conversation
ⓘ You've reached your Qodo monthly free-tier limit. Reviews pause until next month — upgrade your plan to continue now, or link your paid account if you already have one. |
|
ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
📜 Recent review details🧰 Additional context used🧠 Learnings (1)📚 Learning: 2026-05-07T23:34:50.823ZApplied to files:
🔍 Remote MCP FirecrawlSummary — additional review-relevant facts (concise)
Relevant reviewer action items (derived from above)
Sources (Firecrawl searches used)
🔇 Additional comments (6)
WalkthroughThis PR adds a TUI automation pane with rules validation and suggestion review, editor-backed rules editing (seeded from config), persisted preview run creation and SQLite-backed run loading/inspection, and guarded apply that only executes on a loaded persisted run after typing exact "APPLY". The TUI event loop supports a pending terminal-action queue to restore the terminal, run a blocking editor subprocess, reinitialize the UI, and then refresh automation reports. Documentation (ADR 0011, README, ops, roadmap), automation re-exports, and the TUI handler wiring were updated to match the operator contract. Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 8 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (8 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
✨ Simplify code
Comment |
![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: e2a7b0862d
ℹ️ 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".
There was a problem hiding this comment.
Pull request overview
This PR extends the existing Ratatui/Crossterm operator shell (mailroom tui) to support a review-first automation workflow: validating/editing rules, creating and inspecting persisted preview runs, and applying only a loaded saved run behind explicit confirmation. It also updates the operator runbooks/ADR set to document the new TUI automation contract.
Changes:
- Add TUI Automation pane actions/modals for rules validate/suggest, persisted run creation/load, candidate inspection, and guarded apply.
- Add an editor-backed flow for
.mailroom/automation.toml(seed fromconfig/automation.example.tomlwhen missing, then validate on exit). - Update README, runbooks, roadmap, and add ADR 0011 to document the automation TUI safety model and operator key bindings.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/tui.rs | Implements Automation pane actions, modals, candidate inspection UI, apply confirmation gate, and $EDITOR rules editing integration. |
| src/automation/mod.rs | Expands crate-internal re-exports for automation reports used by the TUI. |
| README.md | Updates project posture and ADR references to include TUI automation actions. |
| docs/roadmap/v1-search-triage-draft-queue.md | Updates roadmap slice notes to reflect shipped automation TUI flows. |
| docs/README.md | Adds ADR 0011 to the “Start here” index. |
| docs/operations/tui-operator-shell.md | Updates TUI contract and key bindings to include automation actions and safety gates. |
| docs/operations/automation-rules-and-bulk-actions.md | Documents the TUI automation operator flow and its review-first constraints. |
| docs/decisions/0011-tui-automation-apply-rules.md | Adds ADR capturing the rationale and safety boundaries for automation apply + rules editing in the TUI. |
| docs/decisions/0008-read-only-tui-foundation.md | Clarifies that later ADRs add confirmed actions beyond the original read-only slice. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/tui.rs`:
- Around line 1244-1270: The pattern matches in push_automation_modal_text and
pop_automation_modal_text alias different fields (run_id_text, confirm_text) to
the misleading name limit_text; update the match arms to use a neutral name like
text (or match each variant explicitly) so each variant binds its actual field
with a clear identifier: change the bindings in AutomationModal::RunPreview,
::ShowRun (run_id_text), and ::ApplyRun (confirm_text) to use text, and update
the subsequent push/pop usage to reference text.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 6acd04b2-f236-464b-a242-97b62cad943f
📒 Files selected for processing (9)
README.mddocs/README.mddocs/decisions/0008-read-only-tui-foundation.mddocs/decisions/0011-tui-automation-apply-rules.mddocs/operations/automation-rules-and-bulk-actions.mddocs/operations/tui-operator-shell.mddocs/roadmap/v1-search-triage-draft-queue.mdsrc/automation/mod.rssrc/tui.rs
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Agent
- GitHub Check: rust
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{js,jsx,ts,tsx,md,txt,json}
📄 CodeRabbit inference engine (Custom checks)
Reject Unicode EM DASH (code point U+2014) in comments and string literals. Use
--instead. Hard FAIL if any newly introduced content includes U+2014.
Files:
docs/README.mddocs/decisions/0011-tui-automation-apply-rules.mddocs/operations/automation-rules-and-bulk-actions.mddocs/roadmap/v1-search-triage-draft-queue.mdREADME.mddocs/decisions/0008-read-only-tui-foundation.mddocs/operations/tui-operator-shell.md
docs/decisions/**
📄 CodeRabbit inference engine (AGENTS.md)
Update docs in
docs/decisions/when architecture, storage boundaries, or command surfaces change
Files:
docs/decisions/0011-tui-automation-apply-rules.mddocs/decisions/0008-read-only-tui-foundation.md
docs/{operations,workflows}/**
📄 CodeRabbit inference engine (AGENTS.md)
Put operator procedures in
docs/operations/ordocs/workflows/and keep docs concrete and aligned with the current binary surface
Files:
docs/operations/automation-rules-and-bulk-actions.mddocs/operations/tui-operator-shell.md
🧠 Learnings (1)
📚 Learning: 2026-05-07T23:34:50.823Z
Learnt from: BjornMelin
Repo: BjornMelin/mailroom PR: 23
File: src/tui.rs:61-67
Timestamp: 2026-05-07T23:34:50.823Z
Learning: When reviewing Rust TUI code using ratatui 0.30.0, do not flag `ratatui::try_init()` as missing a panic hook. In this version, `try_init()` installs a panic hook that restores the terminal (similar to `ratatui::init()`), but returns `Result` so callers can handle failures without panicking. In a CLI/TUI handler context (e.g., files like `src/tui.rs`), the correct pattern is to use `try_init()` and ensure terminal restoration via `TerminalGuard`’s `Drop` implementation rather than expecting `init()`-style panic behavior.
Applied to files:
src/automation/mod.rssrc/tui.rs
🔍 Remote MCP Context7
Perfect! Now I have enough context. Let me compile a comprehensive summary of my findings:
Summary of Additional Context for PR #30 Review,, [::GitHub_Copilot_get_file_contents::], [::Firecrawl_firecrawl_search::], [::Firecrawl_firecrawl_search::] Based on my investigation of dependencies, library patterns, service layer, and implementation details:
Key Dependencies and Versions
The codebase uses Ratatui v0.30.0 with Crossterm v0.29.0 for TUI rendering and event handling. [::GitHub_Copilot_get_file_contents::] The TOML crate is at v0.9.8 for rules file parsing and serialization. [::GitHub_Copilot_get_file_contents::]
Ratatui Modal and Popup Patterns
Ratatui provides stateless and stateful widget rendering via Frame::render_widget and Frame::render_stateful_widget, which allows modals to overlay on a cleared area. [::Context7_query-docs::] The implementation uses a centered rect approach with Clear widget to render modals, visible in the new render_automation_modal function in the PR.
Crossterm Event Handling
Crossterm supports event handling with event::read() for blocking reads and poll(duration) for non-blocking checks with timeout, including support for key modifiers (ALT, SHIFT, CONTROL). [::Context7_query-docs::] The PR's automation modal input handling uses KeyCode::Char(value) with backspace support and text input validation, matching Crossterm's documented patterns.
Terminal Restoration Pattern
The PR introduces a pending_terminal_action mechanism to safely restore the terminal, launch an external editor, then re-initialize Ratatui. This is critical because: the TUI runs in raw mode and alternate screen mode via Ratatui's backend initialization, so external commands (like $EDITOR) require disabling these modes, then restoring after the editor exits. [::GitHub_Copilot_pull_request_read::] The implementation calls ratatui::restore() before spawning the editor command and terminal = ratatui::try_init()? after to resume rendering.
Automation Service Layer Integration
The automation service provides distinct operations: automation rules validate, automation rules suggest, automation run for persisted preview snapshots, automation apply <run-id>, and automation show <run-id>. [::GitHub_Copilot_get_file_contents::] The PR re-exports these report types (AutomationApplyReport, AutomationRulesValidateReport, AutomationRulesSuggestReport, AutomationRunPreviewReport, AutomationShowReport) in src/automation/mod.rs to expose them to the TUI layer.
Gmail API Quota Considerations
The Gmail API is subject to 250 quota units per user per second (moving average) and 1,000,000,000 quota units per day. [::Firecrawl_firecrawl_search::] Archive, label, and trash operations each consume quota; the PR's apply concurrency limit of 4 (defined in service.rs) should help avoid quota exhaustion during batch operations.
TOML Handling
The toml crate provides toml::from_str for deserialization and toml::to_string for serialization, supporting serde traits. [::Context7_query-docs::] The PR's editor flow reads .mailroom/automation.toml, invokes $VISUAL/$EDITOR, then validates the edited file by re-calling automation::validate_rules. If the file is missing, it seeds from config/automation.example.toml.
Confirmation Modal Safety Pattern
The PR implements high-friction confirmation by:
- Showing run ID, candidate count, action mix, blocked rules, and Gmail mutation warning in the modal
- Requiring exact
APPLYtext input (not just any confirmation) - Blocking apply unless a persisted run is loaded (never applying rollout preview directly)
- Using
automation apply <run-id> --executeto target only the saved snapshot, not live rollout output
Prior TUI Work Context
PR #23 introduced the read-only TUI operator shell without draft send, cleanup execution, attachment export, automation snapshots, or apply. [::GitHub_Copilot_pull_request_read::] This PR (#30) builds on that foundation, adding the automation workflow interactions while preserving the review-first model.
Test Coverage
The PR includes unit tests covering:
- Modal input validation and blocking invalid limits/run IDs
- Apply confirmation gating (requires
APPLYtext, rejects mismatches) - Prevention of apply without a loaded run
- Candidate detail rendering for loaded automation runs
- Terminal action queueing for editor launch
🔇 Additional comments (15)
src/automation/mod.rs (1)
8-11: Internal report re-export expansion is coherent.The added
modelreport re-exports cleanly expose the automation report types needed by the new TUI automation flows while keeping visibility scoped topub(crate).docs/decisions/0008-read-only-tui-foundation.md (1)
31-34: Read-only boundary clarification is strong and consistent.This update clearly codifies what the first TUI slice excludes while leaving a well-defined path for future confirmed actions.
docs/README.md (1)
23-34: Docs index expansion is well-structured.The added ADR and operations links keep the onboarding path current with the new automation/TUI contract and maintain clear sequencing.
README.md (1)
8-9: README posture and sequencing updates are aligned.These edits consistently reflect the reviewed-snapshot automation flow, include ADR 0011 in the right places, and keep the near-term plan concrete.
Also applies to: 175-178, 223-224, 239-245
docs/roadmap/v1-search-triage-draft-queue.md (1)
91-101: Roadmap scope update is precise and safety-preserving.The added automation capabilities and explicit non-goals are clearly bounded and consistent with the review-first model.
docs/operations/automation-rules-and-bulk-actions.md (1)
172-191: TUI automation runbook flow is concrete and well-guarded.The key-driven steps, persisted-run requirement, and exact
APPLYconfirmation make the operator contract explicit and actionable.docs/operations/tui-operator-shell.md (1)
64-74: Automation keys and mutation guardrails are documented clearly.The key mappings, confirmation inputs, persisted-run restriction, and post-action refresh contract are all explicit and operationally solid.
Also applies to: 95-103, 132-155
docs/decisions/0011-tui-automation-apply-rules.md (1)
1-73: ADR 0011 is complete and decision-grade.This decision document cleanly defines boundaries, confirms persisted-run-only apply, and captures the editor-backed rules workflow without introducing a second automation engine.
src/tui.rs (7)
1-6: LGTM!The new automation imports are correctly structured and include all necessary types from the automation module for the TUI integration.
64-70: LGTM!The pending terminal action handling correctly implements the pattern for external editor invocation: restore terminal, run action, re-initialize. This follows ratatui best practices for spawning external processes while maintaining proper terminal state management. Based on learnings,
try_init()installs a panic hook and returnsResult, which is the correct pattern here.
738-748: LGTM!The apply modal correctly gates on having a loaded persisted run before allowing the user to proceed. Combined with the APPLY confirmation requirement in
confirm_automation_modal, this enforces the review-first safety model where apply targets only a loaded persisted run snapshot.
755-764: LGTM!The APPLY confirmation gate is correctly implemented - it requires an exact match of the string "APPLY" before proceeding with the automation apply operation. This provides the high-friction confirmation specified in the PR objectives.
1300-1334: LGTM!The editor launching implementation follows established patterns:
- Uses
$VISUAL→$EDITOR→vifallback chain (standard convention)- Uses login shell (
-lc) to ensure user's editor configuration is loaded- Properly quotes the path for shell interpretation using single-quote escaping
- Checks exit status and reports errors with path context
1367-1370: LGTM!The shell quoting uses correct POSIX single-quote escaping (
'\''). This safely handles paths containing spaces, special characters, and single quotes when passed to a shell.
3638-3763: LGTM!The new automation tests provide comprehensive coverage for:
- Modal input validation (positive limits/run IDs)
- Apply gate requiring loaded persisted run
- APPLY confirmation requirement
- Editor key queuing terminal action
This aligns with the PR objective for "tests for confirmation gates, blocked apply state, and prevention of direct-rollout-apply behavior."
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/tui.rs`:
- Around line 684-699: The refresh_automation snapshot is only called on some
success branches (e.g., in validate_automation_rules) but not on error branches
or in suggest_automation_rules, so update each automation action path to always
refresh the rollout snapshot: ensure validate_automation_rules calls
self.refresh_automation(config_report).await in both Ok and Err branches (after
setting automation_action_report/status), and add the same unconditional refresh
call to suggest_automation_rules and the run/apply handlers (the other blocks
referenced around lines 701-719, 783-803, 831-853, 864-883) so every success and
failure path invokes self.refresh_automation(config_report).await to keep the
readiness panel consistent.
- Around line 876-882: The current Err arm sets self.automation_action_report
and self.status to claim "invalid TOML" even though validate_rules() may fail
for semantic reasons; update the messages to avoid saying "invalid TOML" and
accurately reflect a validation failure: change the status set on error (in the
Err(error) branch) from "automation rules edit returned invalid TOML" to
something like "automation rules validation failed" and adjust the
automation_action_report text to say the edited rules file "did not validate"
including the error details from error; optionally, if you want finer-grained
messaging, detect a TOML parse error (e.g. a toml::de::Error) and only then use
"invalid TOML", otherwise use the generic validation-failed wording, but at
minimum remove the misleading "invalid TOML" wording in the Err(error) branch
for validate_rules().
- Around line 738-748: open_automation_apply_modal currently only verifies a
persisted run is loaded (via loaded_automation_detail()) but must also verify
the run is still preview-only; update open_automation_apply_modal to inspect the
loaded report's run status (e.g., report.detail.run.status or an
is_preview()/is_pending_preview flag) and only set self.automation_modal =
Some(AutomationModal::ApplyRun { ... }) when that status indicates a
preview-only run; otherwise do not create the ApplyRun modal and set self.status
to the blocked message (e.g., "automation apply unavailable: run not in preview
state") so the destructive APPLY flow is prevented for already-applied or
non-preview runs.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 581d9f68-a3b1-40dd-9917-c6aa5dbc8441
📒 Files selected for processing (1)
src/tui.rs
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: rust
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2026-05-07T23:34:50.823Z
Learnt from: BjornMelin
Repo: BjornMelin/mailroom PR: 23
File: src/tui.rs:61-67
Timestamp: 2026-05-07T23:34:50.823Z
Learning: When reviewing Rust TUI code using ratatui 0.30.0, do not flag `ratatui::try_init()` as missing a panic hook. In this version, `try_init()` installs a panic hook that restores the terminal (similar to `ratatui::init()`), but returns `Result` so callers can handle failures without panicking. In a CLI/TUI handler context (e.g., files like `src/tui.rs`), the correct pattern is to use `try_init()` and ensure terminal restoration via `TerminalGuard`’s `Drop` implementation rather than expecting `init()`-style panic behavior.
Applied to files:
src/tui.rs
🔍 Remote MCP Context7, Firecrawl
Relevant factual points to aid review (concise):
-
Ratatui: modal/popups
- Use Frame::render_widget / render_stateful_widget; common popup pattern is to compute a centered Rect and call frame.render_widget(Clear, area) before rendering the popup Block/Paragraph,.
- Clear only clears the specified buffer area and cannot be relied on to “clear the terminal on the first render” (terminal.clear() needed for first render) — consider that when restoring/initializing after launching $EDITOR,.
-
Crossterm: event handling and editor/terminal restore considerations
- event::read() blocks; poll(timeout) allows non-blocking checks. Do not call read/poll from multiple threads — use a single reader thread and dispatch events to others (cross-thread read is unsupported).
- Key input uses KeyCode::Char / Backspace / Enter variants; Backspace semantics vary by platform (Delete on macOS) — verify TUI modal input mapping and backspace handling match crossterm semantics.
-
TOML (toml crate + serde)
- Use toml::from_str / toml::to_string (or to_string_pretty) for parsing/serializing rules file; seeding a missing file by copying config/automation.example.toml then validating via automation::validate_rules is consistent with typical workflow. Ensure derived Serialize/Deserialize on types used for rules,.
-
Gmail API quotas (safety / apply implications)
- Gmail API enforces quota units and per-minute/per-user/per-project limits; per-minute-per-project and per-minute-per-user-per-project limits (example page lists 1,200,000/min project and 6,000/min-user in table) and per-method quota unit costs (e.g., messages.modify, threads.modify consume quota units) — important for planning apply concurrency and rate-limiting when TUI triggers automation apply that issues Gmail mutations.
- Docs recommend exponential backoff for time-based quota errors; confirm TUI/apply re-tries/backoff and concurrency (PR mentions concurrency limit of 4) align with these guidelines.
-
Practical gotchas & guidance observed in community sources
- Ratatui Clear/popups: forum posts note Clear helps but rendering diffs and external stdout writes can cause artifacts; initializing/restoring terminal state before spawning $EDITOR and re-initializing Ratatui (as PR does) is correct to avoid terminal buffer corruption and flicker,.
- Crossterm: multiple-thread reads cause “stealing” of input; use single-threaded event loop or central reader + dispatch, matching PR's queued terminal-action approach for editor launch is appropriate,.
Sources/tools used:
- Ratatui docs and recipes (widgets, popup/centered layout, Clear) — Firecrawl firecrawl_search scraped pages above,,.
- Crossterm event docs and issue thread — Firecrawl firecrawl_search,,.
- toml crate docs (serde usage, to_string/from_str) — Firecrawl firecrawl_search,.
- Gmail API quota page (quotas, per-method costs, backoff guidance) — Firecrawl firecrawl_search.
- Context7 library resolution for ratatui (library id / docs availability) — Context7_resolve-library-id returned /ratatui/ratatui.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
All actionable review threads have been addressed in pushed commits and resolved. Latest head is @coderabbitai review |
|
Tip For best results, initiate chat on the files or code changes.
[review] |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/tui.rs`:
- Around line 1279-1291: The function run_terminal_action currently ignores the
passed _paths and re-resolves workspace paths via
configured_paths(config_report) when handling
TerminalAction::EditAutomationRules; replace the redundant resolution by using
the provided paths parameter (rename _paths to paths if needed) and pass that to
edit_automation_rules_blocking, or if re-resolution is intentional add a comment
explaining why and keep the variable name; update references to
TerminalAction::EditAutomationRules, configured_paths,
edit_automation_rules_blocking and the function signature run_terminal_action
accordingly so the unused-parameter warning is resolved.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: cea18c9b-31de-45e9-9156-d1e79014db7f
📒 Files selected for processing (1)
src/tui.rs
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: rust
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2026-05-07T23:34:50.823Z
Learnt from: BjornMelin
Repo: BjornMelin/mailroom PR: 23
File: src/tui.rs:61-67
Timestamp: 2026-05-07T23:34:50.823Z
Learning: When reviewing Rust TUI code using ratatui 0.30.0, do not flag `ratatui::try_init()` as missing a panic hook. In this version, `try_init()` installs a panic hook that restores the terminal (similar to `ratatui::init()`), but returns `Result` so callers can handle failures without panicking. In a CLI/TUI handler context (e.g., files like `src/tui.rs`), the correct pattern is to use `try_init()` and ensure terminal restoration via `TerminalGuard`’s `Drop` implementation rather than expecting `init()`-style panic behavior.
Applied to files:
src/tui.rs
🔍 Remote MCP Firecrawl
Summary of additional, review-relevant facts
-
Ratatui: popup/clear behavior
- Use Frame::render_widget(Clear, area) to erase an area before drawing a popup; Clear only clears a buffer area and "cannot be used to clear the terminal on the first render" — use Terminal::clear for initial full-screen clears. Example popup pattern available in ratatui examples.,
-
Ratatui rendering gotchas
- External stdout/stderr writes or other threads can cause buffer diffs/artifacts; Clear only affects ratatui's buffer and won't fix terminal-printed output. Community guidance recommends restoring/initializing terminal state before spawning external editors to avoid artifacts.
-
Crossterm event handling
- crossterm::event::read() blocks until an Event is available; combine poll(Duration) + read() for non-blocking behavior. Avoid reading events from multiple threads — prefer a single reader or an async EventStream. (Docs + community guidance on EventStream vs read()).,
-
TOML (toml crate) serializing / seeding rules file
- toml::from_str / toml::to_string or toml::to_string_pretty are the standard APIs for parsing/serializing with serde; to_string_pretty yields human-friendly formatting (available when serde feature enabled). Seeding a missing file by copying an example and then validating via the rules validator is consistent with common practice.
-
Gmail API quotas & safety guidance (relevant to apply behavior that issues Gmail mutations)
- Quotas: per-minute-per-project = 1,200,000 quota units; per-minute-per-user-per-project = 6,000 quota units. Per-method quota unit costs are listed (e.g., messages.modify 5, messages.batchModify 50, messages.send 100, threads.modify 10). Apply work that issues many mutations must respect these limits and per-method costs.
- For time-based quota errors Google recommends (truncated) exponential backoff and provides an example algorithm; consider backoff and rate-limiting when TUI apply triggers automation apply operations.
Sources consulted
- Ratatui popup and Clear docs and examples; forum discussion on Clear/terminal artifacts.,,
- Crossterm read/poll and EventStream docs/discussion.,
- toml crate docs (to_string / to_string_pretty).
- Gmail API quotas (limits, per-method costs, backoff guidance).
🔇 Additional comments (12)
src/tui.rs (12)
684-699: Refresh not called on error paths.This issue was identified in a previous review and appears to still be present. The
refresh_automation()call on line 691 only runs on the success path. Per the PR objective to "refresh automation data after validate/suggest/run/apply," the rollout snapshot should be refreshed on both success and failure paths to ensure the readiness panel stays consistent.
877-883: Error message still says "invalid TOML" for all validation failures.This issue was identified in a previous review and appears to still be present. Line 882 sets status to "automation rules edit returned invalid TOML" but
validate_rules()can fail for semantic reasons after successful TOML parsing. Consider using "automation rules edit returned invalid rules" or similar generic wording.
739-749: Block APPLY unless the loaded run is still in preview status.This issue was identified in a previous review and appears to still be present. The function only checks that a persisted run is loaded but doesn't verify that
report.detail.run.status == AutomationRunStatus::Previewed. If an operator loads an already-applied run, the TUI would still present the APPLY confirmation flow rather than blocking it.
64-70: LGTM!The terminal action handling correctly restores the terminal before spawning the editor, then reinitializes ratatui after the editor exits. Using
take()ensures the action is only executed once. This follows the recommended pattern for external process spawning in TUI applications.
98-106: LGTM!The new automation state fields follow the established pattern from workflow state management, providing consistent UX for candidate selection and scrolling.
1326-1349: LGTM!Good editor resolution pattern: check
VISUALfirst, thenEDITOR, with sensible platform-specific defaults (vion Unix,notepadon Windows). The let-chain syntax for checking both the env var result and non-empty value is clean.
1373-1420: LGTM!The shell-like command parsing correctly handles single and double quotes, backslash escapes (including escaped quotes within quoted strings), and whitespace tokenization. The unterminated quote detection provides a helpful error message.
1245-1265: LGTM!The push/pop functions now match each variant explicitly with its own field name (
limit_text,run_id_text,confirm_text), which is clearer than the previous aliased binding approach noted in past reviews.
1632-1668: LGTM!Automation view key bindings follow the established pattern and provide intuitive shortcuts:
j/kfor navigation (matching vim conventions),vfor validate,gfor suggest (mnemonic: "generate"),nfor new run,ofor open/show,afor apply, andefor edit.
2439-2468: LGTM!The ApplyRun modal provides comprehensive context for the destructive operation: run ID, candidate count, action mix, blocked rules, and a clear Gmail mutation warning. This aligns with the PR objective to show "run id, candidate count, action mix, blocked rules, and Gmail mutation warning" before apply.
3781-3826: LGTM - but consider adding a test for blocking already-applied runs.This test correctly verifies that apply requires a loaded run and APPLY confirmation. However, it doesn't cover the scenario where the loaded run has status
Appliedinstead ofPreviewed. Consider adding a test case that loads an already-applied run and verifies the modal doesn't open (once the status check mentioned in the past review is implemented).
3868-3893: LGTM!Good test verifying that the seeding function respects custom workspace configuration and creates the rules file in the configured runtime root rather than the default location.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/tui.rs`:
- Around line 64-70: After reinitializing the Ratatui terminal (the line with
terminal = ratatui::try_init()?) add a call to clear the terminal buffer
(terminal.clear()) before proceeding to app.handle_terminal_action_result so
that leftover content from the external editor doesn't flash; update the block
where app.pending_terminal_action is taken, run_terminal_action is called, and
the terminal is reinitialized (terminal = ratatui::try_init()) to invoke
terminal.clear() immediately after reinit and before awaiting
app.handle_terminal_action_result.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: a5143751-f6fd-4855-b002-99911643fc97
📒 Files selected for processing (2)
src/handlers/tui.rssrc/tui.rs
📜 Review details
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2026-05-07T23:34:50.823Z
Learnt from: BjornMelin
Repo: BjornMelin/mailroom PR: 23
File: src/tui.rs:61-67
Timestamp: 2026-05-07T23:34:50.823Z
Learning: When reviewing Rust TUI code using ratatui 0.30.0, do not flag `ratatui::try_init()` as missing a panic hook. In this version, `try_init()` installs a panic hook that restores the terminal (similar to `ratatui::init()`), but returns `Result` so callers can handle failures without panicking. In a CLI/TUI handler context (e.g., files like `src/tui.rs`), the correct pattern is to use `try_init()` and ensure terminal restoration via `TerminalGuard`’s `Drop` implementation rather than expecting `init()`-style panic behavior.
Applied to files:
src/handlers/tui.rssrc/tui.rs
🔍 Remote MCP Firecrawl
Summary of additional, review-relevant facts (from Firecrawl searches)
-
Ratatui provides an explicit recipe for spawning an external editor from a TUI: leave alternate screen, disable raw mode, run child Command (blocking), re-enter alternate screen, enable raw mode, then call terminal.clear() to avoid redraw artifacts. Example code and rationale are in the Ratatui "Spawn External Editor (Vim)" recipe.
-
Ratatui docs + recipes emphasize using panic hooks / cleanup to restore terminal state (disable raw mode + leave alternate screen) so the terminal isn't left corrupted after panics; common pattern shown in Ratatui "Setup Panic Hooks".
-
Crossterm (ratatui's typical backend) documents the alternate screen and raw-mode APIs and provides enable/disable raw mode and Enter/LeaveAlternateScreen primitives that match the TUI→editor flow; there are platform-specific raw-mode gotchas discussed in Crossterm issues (Windows vs UNIX differences). Reviewers should confirm cross-platform behavior in tests.,
-
For seeding/writing TOML rules files: the official toml crate exposes toml::to_string_pretty (requires serde feature) to serialize config structs to a human-friendly TOML string—useful for seeding .mailroom/automation.toml then writing to disk. Community guidance also shows common patterns to write/serialize TOML files.,
-
Gmail API specifics relevant to "apply" safety and scale:
- users.messages.batchModify supports up to 1000 message IDs per request (server-side limit).
- Gmail per-method quota table lists quota units per call; messages.batchModify is shown as costing 50 quota units per request (monitor per-minute-per-user and per-minute-per-project quotas). Reviewers should note rate/quotas and recommend instrumentation or backoff for large applies.
- Community/StackOverflow threads warn that batchModify behavior and quota accounting have nuances; monitor quotas in CI/ops.,
Relevant implications for this PR review (concise)
-
Editor launch approach in src/tui.rs (restore terminal → spawn editor → reinitialize terminal → terminal.clear()) matches Ratatui recipe and panic-hook guidance — verify implementation calls terminal.clear() and installs/uses panic-hooks or equivalent cleanup to avoid terminal corruption.,
-
Cross-platform raw-mode/alternate-screen differences exist; ensure tests exercise editor flow on supported platforms or that code handles platform-specific edge cases documented in Crossterm issues.,
-
TOML seeding/serialization: prefer toml::to_string_pretty (serde feature) when seeding .mailroom/automation.toml so the rules file is readable; confirm code enables serde feature and handles file I/O errors.
-
Gmail apply safety/scale: applying saved runs that map to many Gmail mutations must consider:
- the 1000-id-per-request batchModify limit;
- messages.batchModify per-request quota unit cost (50 units) and per-minute quotas (e.g., 6,000 per-minute-per-user per-project); and
- need for exponential backoff/monitoring to avoid quota exhaustion during large applies. Recommend CI/ops tests or instrumentation to observe quota impact for typical runs.,
Tools/sources used
- Firecrawl (firecrawl_search) — ratatui recipes (spawn-vim, panic-hooks), Crossterm docs & issue, toml docs and examples, Gmail API batchModify and quota docs, relevant community/StackOverflow threads.
🔇 Additional comments (5)
src/handlers/tui.rs (1)
10-13: LGTM!The handler correctly derives workspace paths from the resolved config, ensuring the TUI operates with configured paths (e.g., custom
runtime_root) rather than falling back to defaults. This aligns with the PR objective of honoring configured paths for terminal actions.src/tui.rs (4)
741-758: LGTM!The apply modal correctly enforces the review-first safety model: it requires both a loaded persisted run and verifies the run is still in
Previewedstatus before presenting the destructiveAPPLYflow. This addresses the PR objective to block apply for already-applied or non-preview runs.
1302-1326: LGTM!The editor launch flow correctly: seeds the rules file from the tracked example when missing, resolves the editor command from
$VISUAL/$EDITORwith platform-appropriate fallbacks, and validates the editor exit status before reporting success. Error messages include helpful context (editor program, rules path, and underlying error).
2447-2476: Apply modal shows comprehensive safety context.The confirmation modal for
AutomationModal::ApplyRuncorrectly displays the run ID, candidate count, action mix, any blocked rules from the rollout snapshot, and prominent Gmail mutation warnings (red text). This satisfies the PR objective to require explicit confirmation showing sufficient context before destructive operations.
3789-3856: Good test coverage for apply safety gates.The tests verify both required safety conditions:
automation_apply_requires_loaded_run_and_apply_confirmationchecks that apply is blocked without a loaded run and requires exactAPPLYconfirmation.automation_apply_blocks_non_preview_runsconfirms that already-applied runs cannot be re-applied.This fulfills the PR objective to add tests for confirmation gates, blocked-apply state, and prevention of direct-rollout-apply.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Summary
APPLYconfirmation with run/candidate/action context.Safety
.mailroom/automation.tomlfrom the tracked example when missing, then validates after editor exit.Verification
rtk err cargo fmt --checkrtk err cargo clippy --all-targets --all-features -- -D warningsrtk test cargo testcargo run -- paths --jsoncargo run -- doctor --jsonscript -qefc 'timeout 2s cargo run -- tui' /tmp/mailroom-tui-automation-smoke.txtgit diff --checkCloses #26