Refactor session handling and simplify app key management#1
Conversation
There was a problem hiding this comment.
Code Review
This pull request modernizes the codebase by tightening module boundaries, removing wildcard re-exports, and introducing owner types like CacheStore, SessionParser, and IndexWorker. Key handling in the TUI has been refactored for better maintainability, and the test suite was reorganized into domain-specific modules. Feedback focuses on further code cleanups, including removing redundant match arms, avoiding unidiomatic string allocations, and improving encapsulation by making internal methods private.
| } | ||
| KeyCode::Char(_) => {} | ||
| _ => {} | ||
| KeyCode::Char(_) | KeyCode::Null => false, |
There was a problem hiding this comment.
| let payload = value.get("payload").unwrap_or(&Value::Null); | ||
| if let Some(turn_cwd) = payload.get("cwd").and_then(Value::as_str) { | ||
| cwd = Some(turn_cwd.to_string()); | ||
| } |
There was a problem hiding this comment.
Calling .to_string() on a String is unidiomatic as it performs an unnecessary allocation/copy when a simple .clone() (or moving the value if it's the last use) would suffice. Since turn_model is moved into model here and not used further in this block, you can move it directly.
model = Some(turn_model);| cache_dir.display() | ||
| ))); | ||
| impl IndexWorker { | ||
| pub(crate) fn new(root: PathBuf, cache_dir: PathBuf, tx: Sender<LoadMessage>) -> Self { |
There was a problem hiding this comment.
The new associated function is only used internally within the run method of the same struct. To improve encapsulation and clarify the module's internal API, it should be private (remove pub(crate)).
| pub(crate) fn new(root: PathBuf, cache_dir: PathBuf, tx: Sender<LoadMessage>) -> Self { | |
| fn new(root: PathBuf, cache_dir: PathBuf, tx: Sender<LoadMessage>) -> Self { |
There was a problem hiding this comment.
Pull request overview
This PR performs an internal Rust refactor to tighten module boundaries and encapsulate key subsystems (cache, parsing, worker, terminal lifecycle), while preserving existing CLI/TUI behavior and keeping codex_cost::run() as the only public API surface.
Changes:
- Introduces owner types (
CacheStore,SessionParser,IndexWorker,TerminalSession) and updates call sites to use module-owned imports instead of crate-wide re-exports. - Refactors worker and TUI setup/teardown logic for clearer lifecycle management (including RAII-style terminal restoration).
- Splits the previously monolithic test module into domain-focused test files under
src/tests/and adds a refactor invariants doc.
Reviewed changes
Copilot reviewed 21 out of 21 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
src/app.rs |
Updates imports and refactors key-handling into focused helpers; switches index worker launch to IndexWorker. |
src/cache.rs |
Adds CacheStore wrapper and routes reconciliation/loading through it; updates parsing integration to SessionParser. |
src/cli.rs |
Replaces crate-root imports with module imports; computes cache dir via CacheStore. |
src/lib.rs |
Removes crate-wide wildcard re-exports, leaving pub use cli::run; as the public entry. |
src/parser.rs |
Introduces SessionParser owner type and moves parsing entrypoints/helpers behind it. |
src/pricing.rs |
Updates Session import path to crate::models::Session. |
src/search.rs |
Updates Session import path to crate::models::Session. |
src/ui.rs |
Adds TerminalSession guard-like wrapper and simplifies run_tui terminal lifecycle handling. |
src/util.rs |
Updates model type imports to come from crate::models. |
src/worker.rs |
Refactors index worker logic into IndexWorker struct using CacheStore. |
src/tests.rs |
Removes the monolithic test file (replaced by src/tests/ module). |
src/tests/mod.rs |
New test module entrypoint with shared fixtures/helpers and submodule wiring. |
src/tests/app_tests.rs |
New tests for app key handling and sorting behavior. |
src/tests/cache_tests.rs |
New tests covering cache reconciliation, merkle behavior, and compatibility error messaging. |
src/tests/parser_tests.rs |
New tests for session parsing and fingerprint behavior. |
src/tests/pricing_tests.rs |
New tests for pricing estimates across models/context sizes. |
src/tests/search_tests.rs |
New tests for search indexing and progress reporting. |
src/tests/ui_tests.rs |
New tests for UI helper behavior and error surfacing. |
src/tests/worker_tests.rs |
New tests for index locking and worker behavior on incompatible cache. |
docs/refactor-invariants.md |
Documents behavioral invariants to preserve during modernization refactors. |
docs/superpowers/plans/2026-05-23-modernize-refactor.md |
Adds an implementation plan for the refactor sequence (needs minor updates to reflect the test split performed in this PR). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| - `src/search.rs`: `SearchIndex` methods and search token helpers owned by `SearchIndex` where practical. | ||
| - `src/pricing.rs`: `Pricing`, `CostEstimate`, and cost calculation. | ||
| - `src/util.rs`: generic filesystem/hash/time/atomic-write helpers only. | ||
| - `src/tests.rs`: compatibility test module for the first pass. Split in a later pass after production boundaries settle. |
| - Create: `docs/refactor-invariants.md` | ||
| - Read: `src/tests.rs` | ||
|
|
This pull request introduces a significant internal refactor focused on modularizing and encapsulating cache, parser, and worker logic, as well as improving the structure and maintainability of the TUI event handling code. It also adds a new documentation file outlining refactor invariants. The changes are largely internal and do not affect the public API or user-facing behaviors, as documented in the new invariants file.
Key changes include:
Documentation and Invariants
docs/refactor-invariants.mdto specify behaviors and interfaces that must remain stable during modernization refactors, ensuring backward compatibility and predictable behavior.Cache and Parser Refactoring
CacheStorestruct incache.rsto encapsulate cache directory logic, replacing scattered free functions and improving testability and clarity. [1] [2]SessionParserstruct with associated methods, replacing free functions and making the parser logic more modular. [1] [2]app.rs,cli.rs, etc.) to use the newCacheStoreandSessionParserinterfaces. [1] [2] [3] [4] [5] [6] [7]TUI Event Handling Improvements
app.rsby splitting it into smaller, focused methods (handle_search_key,handle_browse_key, etc.), improving readability and maintainability.Crate API and Internal Structure
pub(crate)re-exports fromlib.rs, clarifying the crate's public API and reducing internal coupling.These changes lay the groundwork for future improvements while ensuring that all critical behaviors and interfaces remain stable for users.