feat(tui): add read-only operator shell - #23
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. |
WalkthroughThis PR adds a Ratatui/Crossterm read-only terminal operator shell ("mailroom tui") with a new CLI subcommand and TuiArgs (--search), handler wiring (handle_tui_command), lib dispatch and metadata, and a full TUI implementation (run entrypoint, terminal guard, Snapshot loading, Dashboard/Search/Workflows/Automation/Help views, key handling, rendering, and tests). It exposes/readjusts read-only service entrypoints (search_read_only, list_workflows_read_only, rollout_read_only), refactors rollout validation, updates exports for AutomationRolloutReport, adds Ratatui/Crossterm dependencies, and includes decision/ops/architecture/README/roadmap documentation defining the TUI safety contract and ownership constraints. 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: bc99c13684
ℹ️ 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
Adds a new mailroom tui subcommand that provides a read-only Ratatui/Crossterm operator shell over existing Mailroom services/reports, and documents the intended “thin UI layer” boundary across the repo.
Changes:
- Introduces a new TUI implementation (
src/tui.rs) with Dashboard/Search/Workflows/Automation/Help views backed by existing services. - Wires the new
tuisubcommand through CLI parsing, handlers, and command metadata. - Adds dependencies (
ratatui,crossterm) and updates architecture/runbook/ADR/roadmap documentation to define the read-only safety contract.
Reviewed changes
Copilot reviewed 15 out of 16 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/tui.rs | New Ratatui-based read-only TUI shell and view rendering over existing reports/search. |
| src/lib.rs | Adds tui module and routes Commands::Tui to the new handler; extends roadmap output. |
| src/handlers/tui.rs | New handler that resolves config and launches the TUI. |
| src/handlers/mod.rs | Registers and re-exports the new TUI handler. |
| src/cli.rs | Adds tui subcommand and TuiArgs (--search). |
| src/automation/mod.rs | Re-exports AutomationRolloutReport for in-crate TUI consumption. |
| README.md | Documents the read-only TUI as an available operator surface and adds usage examples. |
| docs/roadmap/v1-search-triage-draft-queue.md | Updates milestone narrative to include the read-only TUI foundation. |
| docs/README.md | Adds links to the new ADR and TUI operator runbook. |
| docs/operations/tui-operator-shell.md | New runbook for using mailroom tui, including key bindings and safety contract. |
| docs/decisions/0008-read-only-tui-foundation.md | New ADR defining scope/boundaries for the read-only TUI foundation. |
| docs/architecture/system-overview.md | Documents the TUI layer as a thin, read-only shell over existing services. |
| docs/architecture/repository-layout.md | Adds src/tui.rs to the layout and reiterates “no duplicate ownership” guidance. |
| Cargo.toml | Adds Ratatui and Crossterm dependencies (Ratatui with default features disabled). |
| Cargo.lock | Locks new transitive dependencies introduced by Ratatui/Crossterm. |
| AGENTS.md | Adds explicit contributor guidance to keep TUI work thin and non-owning. |
💡 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: 2
🤖 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/handlers/tui.rs`:
- Around line 9-10: Replace the inline blocking call to config::resolve with a
spawn_blocking task: run config::resolve(paths) inside
tokio::task::spawn_blocking(move || config::resolve(paths)), await the join
handle, and propagate the inner Result so errors still use the original ?-style
flow; do not make the spawn_blocking work abortable (don’t wrap it in a
cancellable select), then pass the resolved config_report to tui::run(paths,
config_report, args.search).await as before.
In `@src/tui.rs`:
- Around line 727-765: Add unit tests that exercise the new failure branches
introduced in this module: create Snapshot instances with each task (doctor,
verification, workflows, automation) set to Err variants representing task
failures and assert TuiApp renders the appropriate error/report view or sets
fields (e.g., verify that TuiApp::new(...).view or rendered error text contains
the expected failure mapping); also add tests for search/report rendering
failures by seeding TuiApp with a failing search state (use TuiApp, View, and
any search/report rendering helpers) and assert search-related error output and
search_editing flags behave as expected. Ensure each test targets a single
failure class and checks the contract (view selection, flags, or error text) for
that branch.
🪄 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: 973c6df3-0982-4a6b-816d-ed02c6629eee
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (15)
AGENTS.mdCargo.tomlREADME.mddocs/README.mddocs/architecture/repository-layout.mddocs/architecture/system-overview.mddocs/decisions/0008-read-only-tui-foundation.mddocs/operations/tui-operator-shell.mddocs/roadmap/v1-search-triage-draft-queue.mdsrc/automation/mod.rssrc/cli.rssrc/handlers/mod.rssrc/handlers/tui.rssrc/lib.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 (4)
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
Keep the Rust surface modular: command parsing, workspace paths, storage, Gmail adapters, and TUI should stay separate
Use
anyhowfor command dispatch and top-level context insrc/lib.rs, and prefer typedthiserrorerrors in Gmail, workflow, and store layersKeep error enums local to the layer that owns the failure semantics; do not introduce a repo-wide catch-all error enum
For new CLI JSON contracts, normalize success and failure to one top-level shape: success as
{ "success": true, "data": ... }and failure as{ "success": false, "error": { "code": ..., "message": ..., "kind": ..., "operation": ..., "causes": [...] } }Do not include debug or backtrace payloads in JSON error contracts; use stderr and Rust backtrace env vars for deep diagnostics
Keep
error.codestable and operator-oriented, keeperror.kindfor deeper subsystem detail, and keeperror.causesto an ordered message chain onlyKeep blocking SQLite and filesystem work behind
tokio::task::spawn_blocking; do not treat runningspawn_blockingwork as abortableAdd focused error-path tests for every new failure class, and add contract tests for new CLI JSON or exit-code behavior changes
Files:
src/automation/mod.rssrc/handlers/mod.rssrc/cli.rssrc/handlers/tui.rssrc/lib.rssrc/tui.rs
**/*.{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/architecture/repository-layout.mddocs/architecture/system-overview.mddocs/operations/tui-operator-shell.mddocs/README.mddocs/roadmap/v1-search-triage-draft-queue.mdAGENTS.mddocs/decisions/0008-read-only-tui-foundation.mdREADME.md
docs/{operations,workflows}/**
📄 CodeRabbit inference engine (AGENTS.md)
Put operator procedures in
docs/operations/ordocs/workflows/
Files:
docs/operations/tui-operator-shell.md
docs/decisions/**
📄 CodeRabbit inference engine (AGENTS.md)
Put durable architecture choices in
docs/decisions/
Files:
docs/decisions/0008-read-only-tui-foundation.md
🧠 Learnings (1)
📓 Common learnings
Learnt from: CR
Repo: BjornMelin/mailroom
Timestamp: 2026-05-07T19:44:52.368Z
Learning: Never commit live mailbox caches, OAuth credentials, tokens, or exported attachments by default
Learnt from: CR
Repo: BjornMelin/mailroom
Timestamp: 2026-05-07T19:44:52.368Z
Learning: Keep all runtime state under `.mailroom/` unless there is a deliberate reason to introduce a new ignored path
Learnt from: CR
Repo: BjornMelin/mailroom
Timestamp: 2026-05-07T19:44:52.368Z
Learning: Prefer one canonical representation for local mail metadata rather than duplicating state across multiple stores
Learnt from: CR
Repo: BjornMelin/mailroom
Timestamp: 2026-05-07T19:44:52.368Z
Learning: Treat delete/archive/label/send actions as deliberate operator actions; keep analysis and mutations clearly separated in the code and docs
Learnt from: CR
Repo: BjornMelin/mailroom
Timestamp: 2026-05-07T19:44:52.368Z
Learning: Prefer repo-local paths over hidden global machine state when reasonable
Learnt from: CR
Repo: BjornMelin/mailroom
Timestamp: 2026-05-07T19:44:52.368Z
Learning: Add new dependencies only when they earn their keep for the current milestone
Learnt from: CR
Repo: BjornMelin/mailroom
Timestamp: 2026-05-07T19:44:52.368Z
Learning: Preserve structured output for agent and shell workflows
Learnt from: CR
Repo: BjornMelin/mailroom
Timestamp: 2026-05-07T19:44:52.368Z
Learning: Prefer read-only audit and verification commands before broad mailbox mutations or high-volume automation apply runs
Learnt from: CR
Repo: BjornMelin/mailroom
Timestamp: 2026-05-07T19:44:52.368Z
Learning: Keep TUI work as a thin shell over existing services and reports; do not duplicate Gmail, workflow, automation, or store ownership in UI code
Learnt from: CR
Repo: BjornMelin/mailroom
Timestamp: 2026-05-07T19:44:52.368Z
Learning: If a plugin-assisted Codex workflow exists for an operation, document it alongside native commands rather than pretending the repo already implements it
Learnt from: CR
Repo: BjornMelin/mailroom
Timestamp: 2026-05-07T19:44:52.368Z
Learning: Preserve existing human-facing error text unless it is misleading, ambiguous, or missing required operator action
Learnt from: CR
Repo: BjornMelin/mailroom
Timestamp: 2026-05-07T19:44:52.368Z
Learning: When adding or changing CLI failures, keep exit codes in a small stable bucket set rather than creating one-off codes per variant
Learnt from: CR
Repo: BjornMelin/mailroom
Timestamp: 2026-05-07T19:44:52.368Z
Learning: Update docs when architecture, storage boundaries, or command surfaces change
Learnt from: CR
Repo: BjornMelin/mailroom
Timestamp: 2026-05-07T19:44:52.368Z
Learning: Keep docs concrete and aligned with the current binary surface
Learnt from: CR
Repo: BjornMelin/mailroom
Timestamp: 2026-05-07T19:44:52.368Z
Learning: Update operator-facing docs in the same change whenever CLI error contracts, JSON envelopes, or exit-code behavior change
🪛 LanguageTool
docs/decisions/0008-read-only-tui-foundation.md
[style] ~74-~74: This word has been used in one of the immediately preceding sentences. Using a synonym could make your text more interesting to read, unless the repetition is intentional.
Context: ...ure set Rejected because Mailroom only needs core widgets and the Crossterm backend ...
(EN_REPEATEDWORDS_NEED)
README.md
[style] ~172-~172: This sentence may be long and difficult for your reader to follow. Consider inserting a period and starting a new sentence here.
Context: ...](docs/operations/tui-operator-shell.md), with the durable design captured in [`docs/decisions/0008-read-only-tui-...
(WITH_THE_SENTENCE)
[style] ~234-~234: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...tch archive/label runs land cleanly. 4. Expand unsubscribe assistance only after the d...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
🔍 Remote MCP Firecrawl
Perfect! Now I have comprehensive information. Let me compile my findings.
Summary of Relevant Context for TUI Implementation Review
Based on comprehensive research using Context7 (Ratatui/Crossterm documentation), Firecrawl (web documentation and best practices), and direct inspection of the PR code, here are the key findings:
Event Handling Implementation
According to official Ratatui documentation, the simplest event handling approach involves "centralized catching, message passing" where events are polled in one place and handled accordingly. The PR's implementation in src/tui.rs uses event::poll(EVENT_POLL_INTERVAL)? with a 200ms fixed interval, which is a standard pattern. However, best practices for crossterm+Ratatui note that message passing to separate event handling threads is more responsive than fixed polling intervals, and the main thread should not directly listen for keystrokes.
Terminal Initialization [::Context7_query-docs::]
According to Ratatui's official documentation, the highest-level entry point for Ratatui applications is ratatui::run, which initializes the terminal, executes a provided closure, and automatically calls restore() upon completion or panic. The PR uses a manual approach instead: ratatui::try_init() with a custom TerminalGuard struct that implements Drop to call ratatui::restore(). This is a valid alternative pattern but adds slightly more code complexity.
Event Polling Pattern
Ratatui's official recipes show a Terminal and EventHandler pattern where event::poll is called with a timeout and event::read() is then called only if poll returns true. The PR correctly implements this pattern: if event::poll(EVENT_POLL_INTERVAL)? { ... event::read()? ... }.
Fixed Interval Polling Considerations
Official Ratatui best practices discussions note that fixed polling intervals like 100ms can introduce a slight latency in responsiveness, though the PR uses 200ms which is slightly more conservative. For a read-only dashboard UI, this is acceptable and provides a balance between responsiveness and CPU efficiency.
Cargo.toml Dependency Configuration [::GitHub_Copilot_get_file_contents::]
The PR correctly adds ratatui (unspecified version—needs verification) and crossterm dependencies. However, I notice the Cargo.toml shows the ratatui and crossterm versions are not explicitly pinned in the shown output, which should be validated for consistency with the codebase's version requirements.
Architecture Patterns
Ratatui development best practices emphasize the MVC (Model-View-Controller) pattern, where state is tracked separately from rendering. The PR's TuiApp struct correctly implements this separation: state (search_input, view, snapshot) is kept separate from rendering functions.
Snapshot Loading with Blocking Tasks
The PR uses tokio::task::spawn_blocking() to run blocking operations (doctor inspection and verification) concurrently while fetching workflows asynchronously. This pattern is appropriate for keeping the event loop responsive, though concurrent execution of potentially long-running diagnostics could benefit from documented timeouts.
Search Integration
The TUI correctly integrates with existing mailbox search (using local SQLite FTS) rather than duplicating Gmail query logic, which aligns with the PR's stated boundary of remaining "a thin shell over existing services."
🔇 Additional comments (14)
src/automation/mod.rs (1)
8-8: Scoped re-export looks good.
AutomationRolloutReportis exposed crate-wide without expanding the public API surface.Cargo.toml (1)
34-35: TUI dependency wiring is appropriately minimal.The added crates and ratatui feature flags match the new read-only shell scope.
AGENTS.md (1)
39-39: Good architecture guardrail.This keeps TUI ownership boundaries explicit and prevents service duplication drift.
src/handlers/mod.rs (1)
12-12: Handler registration is clean and consistent.Module declaration and re-export follow the existing dispatch structure.
Also applies to: 27-27
src/cli.rs (1)
54-55: CLI surface addition is well-scoped.
tuiplus optional--searchkeeps the command contract simple and composable.Also applies to: 211-216
src/lib.rs (2)
113-113: Dispatch + operation metadata mapping for TUI is correct.The new command path is integrated cleanly into runtime execution and error metadata.
Also applies to: 199-202
368-373: Roadmap output update is aligned with the new surface.Including the TUI operations doc in
roadmapoutput improves operator discoverability.docs/architecture/repository-layout.md (1)
32-32: Architecture ownership guidance is strong and clear.This keeps TUI as a shell layer and guards against duplicated domain ownership.
Also applies to: 53-55
docs/README.md (1)
20-31: Docs index update is well aligned with the new TUI surface.The added decision and operations links keep the docs entrypoint consistent with the new read-only operator shell scope.
docs/architecture/system-overview.md (1)
39-50: TUI boundary is clearly and correctly documented.The thin-shell wording and cross-links to decision and operations docs make the ownership contract explicit and enforceable.
Also applies to: 124-126
docs/roadmap/v1-search-triage-draft-queue.md (1)
76-88: Roadmap sequencing and TUI exclusions are precise.This keeps the milestone focused and avoids reopening core ownership areas in the TUI slice.
Also applies to: 105-107
docs/decisions/0008-read-only-tui-foundation.md (1)
21-34: ADR scope and exclusions are strong and durable.The decision clearly constrains first-slice behavior and prevents TUI ownership drift.
Also applies to: 45-75
docs/operations/tui-operator-shell.md (1)
40-53: Safety contract and troubleshooting coverage are solid.The runbook clearly documents read-only limits and gives actionable recovery commands.
Also applies to: 63-87
README.md (1)
70-72: README command surface and cross-links are consistent with the new TUI slice.The new
tuiexamples and decision/runbook references are clear and aligned with the read-only boundary.Also applies to: 171-174, 217-217, 225-225
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
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 `@README.md`:
- Around line 232-234: The three consecutive list items in README.md (items
starting with "Expand" for the TUI, automation ergonomics, and unsubscribe
assistance) are repetitive; reword items 2–4 to vary sentence openings and
rhythm—e.g., change item 2 to "Turn the TUI from read-only inspection into
explicit workflow..." or "Add explicit workflow, draft, cleanup, and automation
action flows to the TUI with confirmation screens," change item 3 to "Delay
broader automation ergonomics until a few low-surprise micro-batch archive/label
runs complete cleanly," and change item 4 to "Defer unsubscribe assistance until
the deeper sync validates list-header coverage in the local cache." Use any of
these alternate openings to avoid repeating "Expand."
In `@src/mailbox/search.rs`:
- Around line 8-11: The call to store::init inside async fn search blocks the
async runtime; change it to run on a blocking executor (e.g., use
tokio::task::spawn_blocking or reuse the existing init_store_task helper) so
initialization doesn't block the async thread: perform store::init via
spawn_blocking (or call init_store_task) before awaiting
search_read_only(config_report, request), and propagate errors the same way so
search retains its Result signature.
In `@src/tui.rs`:
- Around line 61-67: The code defines TerminalGuard with a Drop impl to call
ratatui::restore() and calls ratatui::try_init(), but try_init() doesn't install
a panic hook so Drop may not run on panic; replace the try_init() call with
ratatui::init() (e.g., change the initialization to let mut terminal =
ratatui::init()?;) so ratatui installs its panic hook and removes the need for
the TerminalGuard; then delete the TerminalGuard struct and its Drop impl
(symbols: TerminalGuard, Drop::drop, ratatui::try_init, ratatui::init,
ratatui::restore, terminal).
🪄 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: 916cbcd1-fbd1-475c-9364-a7d852dcdc69
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (21)
AGENTS.mdCargo.tomlREADME.mddocs/README.mddocs/architecture/repository-layout.mddocs/architecture/system-overview.mddocs/decisions/0008-read-only-tui-foundation.mddocs/operations/tui-operator-shell.mddocs/roadmap/v1-search-triage-draft-queue.mdsrc/automation/mod.rssrc/automation/service.rssrc/cli.rssrc/handlers/mod.rssrc/handlers/tui.rssrc/lib.rssrc/mailbox.rssrc/mailbox/search.rssrc/tui.rssrc/workflows/mod.rssrc/workflows/service/mod.rssrc/workflows/service/queries.rs
📜 Review details
🧰 Additional context used
📓 Path-based instructions (3)
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
Write Rust code; native center of gravity for the mailroom project
Run
cargo fmt --checkas part of verification before submitting codeRun
cargo clippy --all-targets --all-features -- -D warningsas part of verificationRun
cargo testas part of verification before submitting code
Files:
src/handlers/mod.rssrc/mailbox.rssrc/cli.rssrc/workflows/mod.rssrc/workflows/service/mod.rssrc/handlers/tui.rssrc/lib.rssrc/mailbox/search.rssrc/workflows/service/queries.rssrc/automation/mod.rssrc/automation/service.rssrc/tui.rs
**/*.{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:
AGENTS.mddocs/architecture/repository-layout.mddocs/operations/tui-operator-shell.mddocs/architecture/system-overview.mddocs/roadmap/v1-search-triage-draft-queue.mddocs/README.mddocs/decisions/0008-read-only-tui-foundation.mdREADME.md
src/lib.rs
📄 CodeRabbit inference engine (AGENTS.md)
Use
anyhowfor command dispatch and top-level context insrc/lib.rsas the application boundary
Files:
src/lib.rs
🧠 Learnings (1)
📓 Common learnings
Learnt from: CR
Repo: BjornMelin/mailroom
Timestamp: 2026-05-07T23:09:40.403Z
Learning: Keep the Rust surface modular: command parsing, workspace paths, storage, Gmail adapters, and TUI should stay separate
Learnt from: CR
Repo: BjornMelin/mailroom
Timestamp: 2026-05-07T23:09:40.403Z
Learning: Prefer typed `thiserror` errors in Gmail, workflow, and store layers rather than repo-wide catch-all error enums
Learnt from: CR
Repo: BjornMelin/mailroom
Timestamp: 2026-05-07T23:09:40.403Z
Learning: For new CLI JSON contracts, normalize success to `{ "success": true, "data": ... }` and failure to `{ "success": false, "error": { "code": ..., "message": ..., "kind": ..., "operation": ..., "causes": [...] } }`
Learnt from: CR
Repo: BjornMelin/mailroom
Timestamp: 2026-05-07T23:09:40.403Z
Learning: Do not include debug or backtrace payloads in JSON error contracts; use stderr and Rust backtrace env vars for deep diagnostics
Learnt from: CR
Repo: BjornMelin/mailroom
Timestamp: 2026-05-07T23:09:40.403Z
Learning: Keep blocking SQLite and filesystem work behind `tokio::task::spawn_blocking`; do not treat running `spawn_blocking` work as abortable
Learnt from: CR
Repo: BjornMelin/mailroom
Timestamp: 2026-05-07T23:09:40.403Z
Learning: Add focused error-path tests for every new failure class; if CLI JSON or exit-code behavior changes, add contract tests in the same pass
Learnt from: CR
Repo: BjornMelin/mailroom
Timestamp: 2026-05-07T23:09:40.403Z
Learning: Never commit live mailbox caches, OAuth credentials, tokens, or exported attachments by default
Learnt from: CR
Repo: BjornMelin/mailroom
Timestamp: 2026-05-07T23:09:40.403Z
Learning: Keep all runtime state under `.mailroom/` unless there is a deliberate reason to introduce a new ignored path
Learnt from: CR
Repo: BjornMelin/mailroom
Timestamp: 2026-05-07T23:09:40.403Z
Learning: Prefer one canonical representation for local mail metadata rather than duplicating state across multiple stores
Learnt from: CR
Repo: BjornMelin/mailroom
Timestamp: 2026-05-07T23:09:40.403Z
Learning: Treat delete/archive/label/send actions as deliberate operator actions; keep analysis and mutations clearly separated in the code and docs
Learnt from: CR
Repo: BjornMelin/mailroom
Timestamp: 2026-05-07T23:09:40.403Z
Learning: Prefer repo-local paths over hidden global machine state when reasonable
Learnt from: CR
Repo: BjornMelin/mailroom
Timestamp: 2026-05-07T23:09:40.403Z
Learning: Add new dependencies only when they earn their keep for the current milestone
Learnt from: CR
Repo: BjornMelin/mailroom
Timestamp: 2026-05-07T23:09:40.403Z
Learning: Preserve structured output for agent and shell workflows
Learnt from: CR
Repo: BjornMelin/mailroom
Timestamp: 2026-05-07T23:09:40.403Z
Learning: Prefer read-only audit and verification commands before broad mailbox mutations or high-volume automation apply runs
Learnt from: CR
Repo: BjornMelin/mailroom
Timestamp: 2026-05-07T23:09:40.403Z
Learning: Keep TUI work as a thin shell over existing services and reports; do not duplicate Gmail, workflow, automation, or store ownership in UI code
Learnt from: CR
Repo: BjornMelin/mailroom
Timestamp: 2026-05-07T23:09:40.403Z
Learning: If a plugin-assisted Codex workflow exists for an operation, document it alongside native commands rather than pretending the repo already implements it
Learnt from: CR
Repo: BjornMelin/mailroom
Timestamp: 2026-05-07T23:09:40.403Z
Learning: Keep error enums local to the layer that owns the failure semantics; do not introduce a repo-wide catch-all error enum unless it clearly reduces total code and cognitive load
Learnt from: CR
Repo: BjornMelin/mailroom
Timestamp: 2026-05-07T23:09:40.403Z
Learning: Keep `error.code` stable and operator-oriented, keep `error.kind` for deeper subsystem detail, and keep `error.causes` to an ordered message chain only
Learnt from: CR
Repo: BjornMelin/mailroom
Timestamp: 2026-05-07T23:09:40.403Z
Learning: Preserve existing human-facing error text unless it is misleading, ambiguous, or missing required operator action
Learnt from: CR
Repo: BjornMelin/mailroom
Timestamp: 2026-05-07T23:09:40.403Z
Learning: When adding or changing CLI failures, keep exit codes in a small stable bucket set rather than creating one-off codes per variant
Learnt from: CR
Repo: BjornMelin/mailroom
Timestamp: 2026-05-07T23:09:40.403Z
Learning: Update docs when architecture, storage boundaries, or command surfaces change
Learnt from: CR
Repo: BjornMelin/mailroom
Timestamp: 2026-05-07T23:09:40.403Z
Learning: Put durable architecture choices in `docs/decisions/`
Learnt from: CR
Repo: BjornMelin/mailroom
Timestamp: 2026-05-07T23:09:40.403Z
Learning: Put operator procedures in `docs/operations/` or `docs/workflows/`
Learnt from: CR
Repo: BjornMelin/mailroom
Timestamp: 2026-05-07T23:09:40.403Z
Learning: Keep docs concrete and aligned with the current binary surface
Learnt from: CR
Repo: BjornMelin/mailroom
Timestamp: 2026-05-07T23:09:40.403Z
Learning: Update operator-facing docs in the same change whenever CLI error contracts, JSON envelopes, or exit-code behavior change
🪛 LanguageTool
docs/decisions/0008-read-only-tui-foundation.md
[style] ~75-~75: This word has been used in one of the immediately preceding sentences. Using a synonym could make your text more interesting to read, unless the repetition is intentional.
Context: ...ure set Rejected because Mailroom only needs core widgets and the Crossterm backend ...
(EN_REPEATEDWORDS_NEED)
README.md
[style] ~172-~172: This sentence may be long and difficult for your reader to follow. Consider inserting a period and starting a new sentence here.
Context: ...](docs/operations/tui-operator-shell.md), with the durable design captured in [`docs/decisions/0008-read-only-tui-...
(WITH_THE_SENTENCE)
[style] ~234-~234: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...tch archive/label runs land cleanly. 4. Expand unsubscribe assistance only after the d...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
🔍 Remote MCP Firecrawl
Summary of additional facts relevant to this PR review
-
Ratatui: disabling default features is normal for no-std / backend control; v0.30+ adds ratatui::run()/init()/restore() helper that calls init before and restore after a closure (recommended convenience for ensuring terminal restore). Also disabling default-features can turn off layout-cache (may affect perf unless layout-cache explicitly enabled). Sources: Ratatui docs / highlights pages.,
-
Ratatui / Crossterm coupling: Ratatui has historically re-exported/backed multiple crossterm versions and there are feature/compat concerns; mixed crossterm versions may appear to work but can fail due to global state (raw-mode state, event buffers). Best practice is to ensure the application depends on the same crossterm version used by ratatui or use ratatui’s documented backend feature selection. Source: ratatui GitHub discussion/issue.
-
Crossterm raw-mode semantics and restore: crossterm provides enable_raw_mode() / disable_raw_mode(); programs must disable raw mode on exit (including on panic). Typical patterns: call disable_raw_mode() in a Drop guard or use a central panic hook that disables raw mode before printing backtraces. There are platform differences (raw-mode semantics on Windows vs UNIX) and prior crossterm issues reference these concerns. Sources: crossterm docs and discussion.,, and practical writeups on restoring raw-mode in panic hooks.
Implications for this PR (concise, actionable)
- Using ratatui::run() would simplify terminal init/restore and panic-safety; if the PR implements manual init/restore via a TerminalGuard Drop type that reliably calls restore(), that's acceptable but verify it handles panics and mixed-version crossterm globals. (See ratatui::run() for comparison.)
- Ensure the repo's Cargo.toml pins or otherwise aligns the crossterm version used by the app with the version ratatui expects (avoid mixed crossterm globals that can break raw-mode/event buffering).
- Confirm raw-mode restore on panic/early-exit: the TUI should restore terminal state on panic (Drop guard, panic hook, or ratatui::run()). Also consider try_lock / weak-ref patterns if using panic hooks as described in community writeups.,
Tools/sources used
- Firecrawl firecrawl_search (ratatui docs, ratatui GitHub issue, crossterm docs, raw-mode blog post)
🔇 Additional comments (29)
src/handlers/mod.rs (1)
12-12: Handler module wiring is clean and consistent.This follows the established handler registration pattern and keeps TUI dispatch isolated in the handlers layer.
Also applies to: 27-27
AGENTS.md (1)
39-39: Strong boundary guidance for TUI ownership.This rule is precise and helps prevent service duplication in UI code.
src/automation/mod.rs (1)
8-8: Automation module exports look well-scoped.The added re-exports support read-only rollout/report consumers without blurring ownership boundaries.
Also applies to: 18-19
src/cli.rs (1)
54-55: TUI command surface is clean and focused.The new subcommand and seed argument are concise and consistent with the existing CLI structure.
Also applies to: 211-216
docs/architecture/repository-layout.md (1)
32-32: Repository ownership guidance is clear and enforceable.These additions make TUI boundaries explicit and reduce the risk of cross-layer ownership drift.
Also applies to: 53-55
docs/README.md (1)
20-31: Docs index update is consistent and helpful.The added ADR and renumbered reading path keep the onboarding sequence aligned with the new TUI surface.
src/mailbox.rs (1)
25-25: Mailbox read-only search export is a good addition.This keeps the API explicit for read-only consumers while preserving the mailbox module boundary.
Cargo.toml (1)
34-35: Use explicitcrossterm_0_29feature for clarity.While
ratatui 0.30.0andcrossterm 0.29.0are compatible and the direct dependency on line 35 ensures single-version resolution, the generic"crossterm"feature on line 34 is implicit. Replace with the explicit"crossterm_0_29"feature to document the intended version compatibility:ratatui = { version = "0.30.0", default-features = false, features = ["crossterm_0_29"] }src/workflows/mod.rs (1)
9-13: LGTM.Re-export additions for
list_workflows_read_onlyandpromote_workfloware correctly surfaced at the module boundary alongside existing workflow service items.src/workflows/service/mod.rs (1)
16-19: LGTM.
list_workflows_read_onlyandpromote_workfloware correctly promoted to the service public API.src/lib.rs (1)
113-113: LGTM.
Commands::Tuidispatch correctly takes&paths(matching the async handler signature) without an unnecessary clone, consistent with other non-spawn_blocking-wrapped async handlers likeSearchandGmail.docs/roadmap/v1-search-triage-draft-queue.md (1)
76-107: LGTM.The TUI foundation section accurately describes the scope (Dashboard/Search/Workflows/Automation/Help, local SQLite, no write operations) and correctly positions the next slice as production action flows. Aligned with the implementation and the broader ADR.
docs/decisions/0008-read-only-tui-foundation.md (1)
1-77: LGTM.Well-structured ADR with clear Context/Decision/Why/Consequences/Rejected-alternatives sections. The explicit enumeration of excluded operations (Gmail mutations, draft send, cleanup, attachment export, automation apply) and the Ratatui feature-set rationale are good decision record hygiene. Based on learnings: "Put durable architecture choices in
docs/decisions/."docs/operations/tui-operator-shell.md (1)
1-89: LGTM.Concrete runbook covering invocation, views, key bindings, safety contract, and troubleshooting with accurate CLI commands. Based on learnings: "Put operator procedures in
docs/operations/" and "Keep docs concrete and aligned with the current binary surface."src/workflows/service/queries.rs (1)
15-53: LGTM.The delegation refactor (
list_workflows→store::init+list_workflows_read_only) cleanly separates the store-initializing CLI path from the migration-free TUI path. Theresolve_workflow_account_id(config_report, None)call inlist_workflows_read_onlywill surfaceWorkflowServiceError::NoActiveAccountwhen the database has no synced account, which the TUI correctly treats as a reportable condition rather than an initialization trigger — consistent with the documented safety contract.src/handlers/tui.rs (1)
12-12: ⚡ Quick winBoth terminal integration concerns are properly addressed.
Verification confirms the implementation handles both potential issues from the Ratatui/Crossterm integration:
Panic safety —
src/tui.rsimplements aTerminalGuardstruct with aDropimpl that callsratatui::restore()(line 40, 61-65), ensuring the terminal is restored even if a panic occurs after enabling raw mode.Crossterm version alignment —
Cargo.tomlexplicitly pinscrossterm = "0.29.0", which aligns with ratatui 0.30.0's expected version range. Both versions are compatible with no mismatch risk.docs/architecture/system-overview.md (2)
34-50: LGTM!The TUI layer documentation clearly establishes the read-only boundary and thin-shell constraint, aligning with the project's architectural principle that the TUI must not create a second rules engine or storage model. The enumeration of rendered views (Dashboard, Search, Workflows, Automation) and the explicit mutation gating requirement are well-documented.
124-127: LGTM!Cross-references to the new decision record and operations doc are correctly placed alongside other ownership references.
src/mailbox/search.rs (1)
13-68: LGTM!The
search_read_onlyfunction correctly validates input (non-empty terms, positive limit), resolves the account ID, and usesspawn_blockingfor the SQLite FTS query. This aligns with the project's pattern of keeping blocking database work off the async runtime and supports the TUI's read-only surface without initializing state.src/tui.rs (5)
27-59: LGTM!The
runfunction correctly:
- Loads a snapshot of read-only data before entering the render loop
- Seeds the Search view from
initial_searchif provided- Uses a guard pattern for terminal cleanup
- Handles key events with proper filtering for
KeyEventKind::PressThe structure maintains the TUI as a thin shell over existing services without duplicating business logic.
189-226: LGTM!The
load_snapshotfunction correctly usesspawn_blockingfor the synchronousDoctorReport::inspectandaudit::verificationcalls, while the async service calls (list_workflows_read_only,rollout_read_only) are awaited directly since they handle their own blocking work internally. Error handling preserves context viaerror_chain.
711-726: LGTM!The
truncatehelper correctly handles edge cases: empty max, small max (≤3), and normal truncation with ellipsis. The boundary conditions are well-tested in the test module.
736-893: LGTM!Comprehensive test coverage for:
- Truncation helpers (lines 749-757)
- View initialization and navigation (lines 759-773)
- Diagnostic failure context preservation (lines 775-787)
- Error rendering for all views (lines 789-831)
- Read-only invariant: snapshot load and search do not create runtime state (lines 833-870)
The use of
TestBackendfor rendering tests is appropriate for TUI modules.
39-40: The code is correct:ratatui::try_init()andratatui::restore()are available in ratatui 0.30.0 with the crossterm feature enabled.The project specifies
ratatui = { version = "0.30.0", ... features = ["crossterm"] }, which provides these convenience API functions. The implementation is sound:try_init()initializes the terminal and returns aResult, andTerminalGuardcorrectly callsrestore()on drop to clean up.README.md (2)
70-71: LGTM!The TUI command examples are correctly placed in the native commands section and demonstrate both basic invocation and search-seeded startup.
171-175: LGTM!Cross-references to the new TUI documentation follow the established pattern for other subsystem docs.
src/automation/service.rs (3)
287-292: LGTM!The validation helper uses the typed
AutomationServiceError::InvalidRolloutLimiterror, consistent with the project's preference for typedthiserrorerrors over catch-all enums.
196-204: LGTM!The refactored
rolloutfunction correctly validates the request, ensures runtime directories exist, initializes the store, then delegates to the sharedbuild_rollout_report. This maintains the write-path invariants while sharing report-building logic with the read-only variant.
206-212:rollout_read_onlygracefully handles uninitialized store via NoActiveAccount blocker.Unlike
rollout, this function skipsensure_runtime_dirs_taskandinit_store_task. When the store doesn't exist (e.g., fresh checkout without priorstore init),resolve_automation_account_idreturns aNoActiveAccounterror, which is explicitly handled as a rollout blocker byis_rollout_blocker(line 617). The function returnsOk(AutomationRolloutReport)with blockers populated rather than propagating an error. This is the intentional design for read-only operations and is consistent with thesearch_read_onlypattern verified in the test suite—no runtime state is created, and graceful degradation is applied.
There was a problem hiding this comment.
💡 Codex Review
mailroom/src/mailbox/search.rs
Line 40 in 825cbf9
Offload the read-only account lookup
When the new search_read_only path is used from the TUI, this call still performs SQLite work synchronously via store::accounts::get_active / store::mailbox::inspect_mailbox before the actual search is moved to spawn_blocking. On a busy or slow local store, submitting a TUI search can block the async runtime/UI thread, and it also violates the repo’s AGENTS.md guidance to keep blocking SQLite work behind tokio::task::spawn_blocking; move this lookup into the existing blocking section or a separate blocking task.
ℹ️ 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.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/mailbox/search.rs (1)
40-40:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
resolve_search_account_idblocks the async thread with SQLite I/O.
resolve_search_account_idcallsstore::accounts::get_active()andstore::mailbox::inspect_mailbox()synchronously. Both are blocking SQLite operations and must not run on a Tokio worker thread. This affects both the CLI and the new TUI path that callssearch_read_onlydirectly.As per coding guidelines: "Keep blocking SQLite and filesystem work behind
tokio::task::spawn_blocking."🔧 Proposed fix
- let account_id = resolve_search_account_id(config_report)?; + let config_for_account = config_report.clone(); + let account_id = + spawn_blocking(move || resolve_search_account_id(&config_for_account)).await??;🤖 Prompt for 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. In `@src/mailbox/search.rs` at line 40, The call to resolve_search_account_id performs synchronous SQLite I/O (store::accounts::get_active and store::mailbox::inspect_mailbox) on a Tokio worker thread; change the flow so these blocking operations run inside tokio::task::spawn_blocking and are awaited instead of executing on the async runtime. Either make resolve_search_account_id async and inside it wrap get_active()/inspect_mailbox() calls in spawn_blocking (returning the resolved account_id), or keep it synchronous and call tokio::task::spawn_blocking from the caller (where resolve_search_account_id is invoked) to execute the function off the async thread; ensure the returned Result/AccountId is awaited and propagated correctly.
🤖 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.
Outside diff comments:
In `@src/mailbox/search.rs`:
- Line 40: The call to resolve_search_account_id performs synchronous SQLite I/O
(store::accounts::get_active and store::mailbox::inspect_mailbox) on a Tokio
worker thread; change the flow so these blocking operations run inside
tokio::task::spawn_blocking and are awaited instead of executing on the async
runtime. Either make resolve_search_account_id async and inside it wrap
get_active()/inspect_mailbox() calls in spawn_blocking (returning the resolved
account_id), or keep it synchronous and call tokio::task::spawn_blocking from
the caller (where resolve_search_account_id is invoked) to execute the function
off the async thread; ensure the returned Result/AccountId is awaited and
propagated correctly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 81fb49d6-66a2-41b0-8173-b8faf081f10c
📒 Files selected for processing (3)
Cargo.tomlREADME.mdsrc/mailbox/search.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
📓 Path-based instructions (2)
src/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
Keep the Rust surface modular: command parsing, workspace paths, storage, Gmail adapters, and TUI should stay separate
Use
anyhowfor command dispatch and top-level context insrc/lib.rs, and prefer typedthiserrorerrors in Gmail, workflow, and store layersKeep error enums local to the layer that owns the failure semantics; do not introduce a repo-wide catch-all error enum unless it clearly reduces total code and cognitive load
For new CLI JSON contracts, normalize success to
{ "success": true, "data": ... }and failure to{ "success": false, "error": { "code": ..., "message": ..., "kind": ..., "operation": ..., "causes": [...] } }Do not include debug or backtrace payloads in the JSON error contract; use stderr and Rust backtrace env vars for deep diagnostics instead
Keep
error.codestable and operator-oriented, keeperror.kindfor deeper subsystem detail, and keeperror.causesto an ordered message chain onlyWhen adding or changing CLI failures, keep exit codes in a small stable bucket set rather than creating one-off codes per variant
Keep blocking SQLite and filesystem work behind
tokio::task::spawn_blocking; do not treat runningspawn_blockingwork as abortableAdd focused error-path tests for every new failure class. If CLI JSON or exit-code behavior changes, add contract tests for the new output and exit mapping in the same pass
Preserve structured output for agent and shell workflows
Run
cargo fmt --checkto verify code formattingRun
cargo clippy --all-targets --all-features -- -D warningsto verify lintingRun
cargo testto verify all tests pass
Files:
src/mailbox/search.rs
**/*.{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:
README.md
🧠 Learnings (2)
📓 Common learnings
Learnt from: CR
Repo: BjornMelin/mailroom
Timestamp: 2026-05-07T23:34:26.464Z
Learning: Never commit live mailbox caches, OAuth credentials, tokens, or exported attachments by default
Learnt from: CR
Repo: BjornMelin/mailroom
Timestamp: 2026-05-07T23:34:26.464Z
Learning: Keep all runtime state under `.mailroom/` unless there is a deliberate reason to introduce a new ignored path
Learnt from: CR
Repo: BjornMelin/mailroom
Timestamp: 2026-05-07T23:34:26.464Z
Learning: Prefer one canonical representation for local mail metadata rather than duplicating state across multiple stores
Learnt from: CR
Repo: BjornMelin/mailroom
Timestamp: 2026-05-07T23:34:26.464Z
Learning: Treat delete/archive/label/send actions as deliberate operator actions; keep analysis and mutations clearly separated in the code and docs
Learnt from: CR
Repo: BjornMelin/mailroom
Timestamp: 2026-05-07T23:34:26.464Z
Learning: Prefer repo-local paths over hidden global machine state when reasonable
Learnt from: CR
Repo: BjornMelin/mailroom
Timestamp: 2026-05-07T23:34:26.464Z
Learning: Add new dependencies only when they earn their keep for the current milestone
Learnt from: CR
Repo: BjornMelin/mailroom
Timestamp: 2026-05-07T23:34:26.464Z
Learning: Prefer read-only audit and verification commands before broad mailbox mutations or high-volume automation apply runs
Learnt from: CR
Repo: BjornMelin/mailroom
Timestamp: 2026-05-07T23:34:26.464Z
Learning: Keep TUI work as a thin shell over existing services and reports; do not duplicate Gmail, workflow, automation, or store ownership in UI code
Learnt from: CR
Repo: BjornMelin/mailroom
Timestamp: 2026-05-07T23:34:26.464Z
Learning: If a plugin-assisted Codex workflow exists for an operation, document it alongside native commands rather than pretending the repo already implements it
📚 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/mailbox/search.rs
🪛 LanguageTool
README.md
[style] ~172-~172: This sentence may be long and difficult for your reader to follow. Consider inserting a period and starting a new sentence here.
Context: ...](docs/operations/tui-operator-shell.md), with the durable design captured in [`docs/decisions/0008-read-only-tui-...
(WITH_THE_SENTENCE)
🔍 Remote MCP Firecrawl
Summary — additional facts useful for reviewing PR #23 (read-only Ratatui TUI)
- Ratatui offers convenience init/run/restore helpers and recommends using ratatui::run() to ensure terminal is initialized/restored (helps panic-safety); docs and examples show both run/init/restore and manual Terminal usage.()
- Ratatui 0.30+ supports selecting Crossterm major versions via feature flags (crossterm_0_28, crossterm_0_29). Mixed Crossterm majors can cause: separate event queues, raw-mode globals not restored correctly, and type incompatibilities — so the repo should ensure the same Crossterm major is used by ratatui and application code. (Cargo.toml in PR already sets ratatui default-features = false + feature crossterm_0_29 and adds crossterm.)
- ratatui-crossterm re-exports the selected Crossterm as its backend; using that re-export (or aligning versions) avoids conflicts in downstream code.
- Crossterm raw-mode APIs: enable_raw_mode()/disable_raw_mode() are the recommended top-level calls; programs must disable raw mode on exit (including on panic). Using these simplifies panic cleanup compared to Termion’s RawTerminal semantics.
- Panic-hook guidance: Ratatui docs include explicit patterns for panic hooks for each backend. Termion requires extra care (RawTerminal stores previous state; docs show creating a RawTerminal, suspending raw mode for the hook, and notes pitfalls). Termion panic handling is more complex and can deadlock if not designed carefully. Review should ensure PR’s TUI uses ratatui/crossterm patterns (or implements robust panic/restore handling).
- Community advisory and issue history: Ratatui maintainers warn about Crossterm version incompatibility (pinned/pinned-feature guidance, examples of projects switching to ratatui::crossterm re-export to avoid mismatches). This is an established, frequent source of runtime and subtle bugs (raw-mode persistence, lost events). Confirm the repository’s Cargo graph avoids multiple Crossterm majors.
- Practical implication for this PR: because Cargo.toml adds ratatui with default-features = false and feature crossterm_0_29 plus a direct crossterm dependency, verify:
- The ratatui crate version used by src/tui.rs is compatible with the chosen ratatui features (init/run/restore availability).
- No other dependency pulls a different Crossterm major (use cargo tree -p crossterm to confirm). If other crates need Crossterm, prefer using ratatui’s re-export or align feature flags.
- Ensure panic/early-exit paths call restore/disable_raw_mode (or use ratatui::run()) so terminal state is restored on panics or test failures.
Sources / tools used
- Ratatui docs (Backends, Feature Flags, Panic Hooks) via Firecrawl firecrawl_search.,,
- Ratatui / crates metadata (Terminal API, run/init/restore guidance) via Firecrawl firecrawl_search.
- Crossterm raw-mode docs via Firecrawl firecrawl_search.
- Ratatui issue discussing Crossterm incompatibility (practical community guidance) via Firecrawl firecrawl_search.
🔇 Additional comments (8)
src/mailbox/search.rs (1)
8-12:store::initcorrectly moved tospawn_blocking.The previous concern about blocking the async runtime on
store::inithas been addressed. The??correctly unwraps theJoinErrorand the innerResult.README.md (5)
8-8: Clear documentation of TUI posture.The consistent emphasis on "read-only" and the explicit priority ordering (CLI first, TUI second) effectively communicate the TUI's role as a thin inspection shell over existing services.
Also applies to: 10-10, 48-48
70-71: TUI command examples follow established patterns.The placement between search and attachment commands is logical, and the syntax is consistent with other command examples. The
--searchflag example clearly demonstrates the feature.
217-217: Docs map entries are consistent.The documentation map entries follow the established format and correctly reference the same TUI documentation files mentioned in the operations section.
Also applies to: 225-225
232-234: Roadmap items now have varied structure.The previous review concern about repetitive "Expand" openings has been addressed. The current wording clearly distinguishes each item while maintaining logical sequencing and dependencies.
171-174: The referenced documentation files are present in the repository:
docs/operations/tui-operator-shell.mddocs/decisions/0008-read-only-tui-foundation.mdNo issues with this section.
Cargo.toml (2)
34-35: LGTM! Dependencies justified by TUI milestone.The ratatui and crossterm additions are appropriate for the read-only TUI feature. The configuration follows best practices with explicit feature flag selection and proper version alignment.
Based on learnings, these dependencies earn their keep for the current milestone.
34-35: ⚡ Quick winDependency configuration is correct; verify locally that no transitive dependencies pull a different Crossterm major version.
The ratatui and crossterm dependency configuration is properly aligned:
- ratatui's
crossterm_0_29feature flag matches the direct crossterm0.29.0dependency- Using
default-features = falsefor ratatui is appropriateCrossterm version conflicts are a common source of subtle runtime bugs. Verify during local testing or in CI that no other dependency in the tree pulls a different Crossterm major version (e.g., via
cargo tree -p crossterm).
|
[codex] Addressed the latest Codex review in 4d0d552. |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/mailbox/search.rs (1)
32-38: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winAdd error-path tests for the three new failure classes.
Per coding guidelines, every new failure class requires a focused error-path test. These three paths lack coverage:
- Empty
terms→"search terms cannot be empty"(line 32–33)limit == 0→"search limit must be greater than zero"(line 36–37)- No active account →
"no active Gmail account found; runmailroom auth loginfirst"(line 85–87)The existing
search_request_default_limit_is_nonzerotest validates only that the default limit is nonzero, not error handling when zero is explicitly passed.🤖 Prompt for 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. In `@src/mailbox/search.rs` around lines 32 - 38, Add focused unit/integration tests in src/mailbox/search.rs covering the three error paths: (1) call the search entry point with an empty terms vector and assert the returned error contains "search terms cannot be empty" (use the same function that reads `terms`), (2) call it with request.limit == 0 and assert the error contains "search limit must be greater than zero" (exercise the branch that reads `request.limit`), and (3) simulate/no-op the account lookup to produce the "no active Gmail account found; run `mailroom auth login` first" error and assert that exact message is returned (exercise the code path that checks for an active account around the `no active Gmail account found` check). Ensure each test is named clearly (e.g., search_empty_terms_errors, search_zero_limit_errors, search_no_active_account_errors) and asserts the specific error string.
🤖 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.
Outside diff comments:
In `@src/mailbox/search.rs`:
- Around line 32-38: Add focused unit/integration tests in src/mailbox/search.rs
covering the three error paths: (1) call the search entry point with an empty
terms vector and assert the returned error contains "search terms cannot be
empty" (use the same function that reads `terms`), (2) call it with
request.limit == 0 and assert the error contains "search limit must be greater
than zero" (exercise the branch that reads `request.limit`), and (3)
simulate/no-op the account lookup to produce the "no active Gmail account found;
run `mailroom auth login` first" error and assert that exact message is returned
(exercise the code path that checks for an active account around the `no active
Gmail account found` check). Ensure each test is named clearly (e.g.,
search_empty_terms_errors, search_zero_limit_errors,
search_no_active_account_errors) and asserts the specific error string.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 5b50af71-503f-458a-bd70-769df952fc63
📒 Files selected for processing (1)
src/mailbox/search.rs
📜 Review details
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
For new CLI JSON contracts, normalize success and failure to the standard top-level shape:
{ "success": true, "data": ... }for success and{ "success": false, "error": { "code": ..., "message": ..., "kind": ..., "operation": ..., "causes": [...] } }for failureKeep
error.codestable and operator-oriented, keeperror.kindfor deeper subsystem detail, and keeperror.causesto an ordered message chain onlyDo not include debug or backtrace payloads in the JSON error contract; use stderr and Rust backtrace env vars for deep diagnostics instead
Keep blocking SQLite and filesystem work behind
tokio::task::spawn_blocking; do not treat runningspawn_blockingwork as abortableAdd focused error-path tests for every new failure class. If CLI JSON or exit-code behavior changes, add contract tests for the new output and exit mapping in the same pass
When adding or changing CLI failures, keep exit codes in a small stable bucket set rather than creating one-off codes per variant
Files:
src/mailbox/search.rs
🧠 Learnings (2)
📓 Common learnings
Learnt from: CR
Repo: BjornMelin/mailroom
Timestamp: 2026-05-07T23:46:55.128Z
Learning: Native center of gravity is Rust; all new code should be written in Rust unless there is a deliberate reason to use another language
Learnt from: CR
Repo: BjornMelin/mailroom
Timestamp: 2026-05-07T23:46:55.128Z
Learning: Keep the Rust surface modular: command parsing, workspace paths, storage, Gmail adapters, and TUI should stay separate
Learnt from: CR
Repo: BjornMelin/mailroom
Timestamp: 2026-05-07T23:46:55.128Z
Learning: Use `anyhow` for command dispatch and top-level context in `src/lib.rs`, and prefer typed `thiserror` errors in Gmail, workflow, and store layers
Learnt from: CR
Repo: BjornMelin/mailroom
Timestamp: 2026-05-07T23:46:55.128Z
Learning: Keep error enums local to the layer that owns the failure semantics; do not introduce a repo-wide catch-all error enum unless it clearly reduces total code and cognitive load
Learnt from: CR
Repo: BjornMelin/mailroom
Timestamp: 2026-05-07T23:46:55.128Z
Learning: Never commit live mailbox caches, OAuth credentials, tokens, or exported attachments to git by default
Learnt from: CR
Repo: BjornMelin/mailroom
Timestamp: 2026-05-07T23:46:55.128Z
Learning: Keep all runtime state under `.mailroom/` unless there is a deliberate reason to introduce a new ignored path
Learnt from: CR
Repo: BjornMelin/mailroom
Timestamp: 2026-05-07T23:46:55.128Z
Learning: Prefer one canonical representation for local mail metadata rather than duplicating state across multiple stores
Learnt from: CR
Repo: BjornMelin/mailroom
Timestamp: 2026-05-07T23:46:55.128Z
Learning: Treat delete/archive/label/send actions as deliberate operator actions; keep analysis and mutations clearly separated in the code and docs
Learnt from: CR
Repo: BjornMelin/mailroom
Timestamp: 2026-05-07T23:46:55.128Z
Learning: Prefer repo-local paths over hidden global machine state when reasonable
Learnt from: CR
Repo: BjornMelin/mailroom
Timestamp: 2026-05-07T23:46:55.128Z
Learning: Add new dependencies only when they earn their keep for the current milestone
Learnt from: CR
Repo: BjornMelin/mailroom
Timestamp: 2026-05-07T23:46:55.128Z
Learning: Preserve structured output for agent and shell workflows in CLI output
Learnt from: CR
Repo: BjornMelin/mailroom
Timestamp: 2026-05-07T23:46:55.128Z
Learning: Prefer read-only audit and verification commands before broad mailbox mutations or high-volume automation apply runs
Learnt from: CR
Repo: BjornMelin/mailroom
Timestamp: 2026-05-07T23:46:55.128Z
Learning: Keep TUI work as a thin shell over existing services and reports; do not duplicate Gmail, workflow, automation, or store ownership in UI code
Learnt from: CR
Repo: BjornMelin/mailroom
Timestamp: 2026-05-07T23:46:55.128Z
Learning: If a plugin-assisted Codex workflow exists for an operation, document it alongside native commands rather than pretending the repo already implements it
Learnt from: CR
Repo: BjornMelin/mailroom
Timestamp: 2026-05-07T23:46:55.128Z
Learning: Update operator-facing docs in the same change whenever CLI error contracts, JSON envelopes, or exit-code behavior change
Learnt from: CR
Repo: BjornMelin/mailroom
Timestamp: 2026-05-07T23:46:55.128Z
Learning: Keep docs concrete and aligned with the current binary surface
Learnt from: CR
Repo: BjornMelin/mailroom
Timestamp: 2026-05-07T23:46:55.128Z
Learning: Update docs when architecture, storage boundaries, or command surfaces change
Learnt from: CR
Repo: BjornMelin/mailroom
Timestamp: 2026-05-07T23:46:55.128Z
Learning: Primary interfaces are CLI first, TUI second; design commands with this priority in mind
Learnt from: CR
Repo: BjornMelin/mailroom
Timestamp: 2026-05-07T23:46:55.128Z
Learning: Use SQLite FTS5 as the single operational store for mail metadata
Learnt from: CR
Repo: BjornMelin/mailroom
Timestamp: 2026-05-07T23:46:55.128Z
Learning: Scope is single user, single mailbox first; do not introduce multi-user or multi-mailbox complexity until there is a clear operational need
Learnt from: CR
Repo: BjornMelin/mailroom
Timestamp: 2026-05-07T23:46:55.128Z
Learning: Runtime workspace is `.mailroom/`; use this as the canonical location for all local state
Learnt from: CR
Repo: BjornMelin/mailroom
Timestamp: 2026-05-07T23:46:55.128Z
Learning: Sensitive state must remain out of git; ensure credentials, tokens, and live mailbox data are in .gitignore
Learnt from: CR
Repo: BjornMelin/mailroom
Timestamp: 2026-05-07T23:46:55.128Z
Learning: Preserve existing human-facing error text unless it is misleading, ambiguous, or missing required operator action
📚 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/mailbox/search.rs
🔍 Remote MCP Firecrawl
Summary — additional, review-relevant facts found (concise)
-
PR page: confirmed PR
#23description, verification steps, CI/agent comments, requested changes, and that author addressed a search-blocking fix in commit 4d0d552 (account ID resolution moved into spawn_blocking).#23 -
Ratatui (docs.rs / v0.30.x): ratatui provides convenience init()/try_init()/run()/restore() helpers; run() installs panic hooks and restores terminal automatically. Manual init/restore + a Drop guard is a supported alternative. Ratatui supports backend selection via features (crossterm, crossterm_0_28, crossterm_0_29, termion, termwiz) and re-exports ratatui_crossterm::crossterm. Recommend matching backend feature flags to the direct crossterm dependency to avoid mixed-major issues.https://docs.rs/ratatui/latest/ratatui/
-
Crossterm (docs.rs / v0.29.0): crossterm exposes enable_raw_mode()/disable_raw_mode(), event APIs, and optional async/event-stream features. Feature flags include event-stream and backend-related options. Crossterm 0.29 is the expected major for ratatui's crossterm_0_29 feature. Ensure no other transitive dependency pulls a different Crossterm major.https://docs.rs/crossterm/latest/crossterm/
Actionable reviewer checks to run (minimal, concrete)
-
Verify Cargo dependency alignment locally/CI: run cargo tree -p crossterm and ensure only crossterm 0.29.x is present (or that all ratatui/crossterm users resolve to the same major). [Rationale: mixed Crossterm majors cause raw-mode/event/global-state bugs]https://docs.rs/ratatui/latest/ratatui/ , https://docs.rs/crossterm/latest/crossterm/
-
Confirm terminal restore/panic-safety: check src/tui.rs uses either ratatui::run()/init()/try_init() + restore() or a Drop guard that reliably calls restore() (and that tests/execution paths exercise panic/early-return cleanup). PR uses a TerminalGuard + try_init() pattern — acceptable per docs but verify Drop runs on panic in the runtime context.https://docs.rs/ratatui/latest/ratatui/
-
Confirm blocking I/O is offloaded: verify search/account-resolution and other SQLite work run inside tokio::task::spawn_blocking (author notes they moved resolve into the same closure). Re-run mailbox search tests and TUI interactive verification from PR verification steps. [PR evidence + docs]#23 , https://docs.rs/ratatui/latest/ratatui/
Citations
- PR page and review thread (PR
#23) — Firecrawl scrape of #23 - Ratatui docs (init/run/restore, backend feature flags, re-exports) — Firecrawl scrape of https://docs.rs/ratatui/latest/ratatui/
- Crossterm docs (raw-mode APIs, feature flags) — Firecrawl scrape of https://docs.rs/crossterm/latest/crossterm/
🔇 Additional comments (2)
src/mailbox/search.rs (2)
8-12: LGTM —store::initcorrectly offloaded tospawn_blocking.The clone +
spawn_blocking(move || store::init(&init_config)).await??pattern is consistent with the rest of the service layer. The past blocking-on-async-runtime concern is fully resolved here.
79-83: ⚡ Quick winNo issue. The crate correctly declares
edition = "2024"inCargo.toml, which supports thelet_chainssyntax used in lines 79-83.> Likely an incorrect or invalid review comment.
Summary
mailroom tui, a Ratatui/Crossterm read-only operator shell over existing Mailroom services.Safety
.mailroom/automation.toml.Verification
cargo fmt --checkcargo clippy --all-targets --all-features -- -D warningscargo testcargo run -- paths --jsoncargo run -- doctor --jsoncargo run -- tui --helpcargo run -- roadmapcargo run -- tuiin a PTY, thenqcargo run -- tui --search definitely-no-such-mailroom-test-tokenin a PTY, thenqFollow-up Issues