Conversation
There was a problem hiding this comment.
Pull request overview
This PR refactors the TUI runtime by splitting the previously monolithic prompt/editor/processor/search implementation into focused modules (query editor, completion, JSON viewer, guide, event dispatcher, runtime tasks) and centralizing shared state and debouncing utilities.
Changes:
- Introduces modular async tasks for query editing, completion navigation, JSON viewing, guide messages, and terminal event dispatch.
- Adds a reusable debouncer utility and a shared
Contextto coordinate UI focus and processing state. - Removes legacy prompt/editor/search/processor modules and updates dependencies accordingly.
Reviewed changes
Copilot reviewed 18 out of 19 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/utils/debounce.rs | New shared debouncer utility for query/resize events. |
| src/utils.rs | Exposes debouncer setup from utils. |
| src/runtime_tasks.rs | Adds background tasks for query-change forwarding and resize-driven re-rendering. |
| src/query_editor.rs | New query editor component + action-driven task loop. |
| src/json_viewer.rs | New JSON viewer component + action-driven rendering and processing tasks. |
| src/guide.rs | New guide message/action system + rendering task. |
| src/event_dispatcher.rs | Centralized terminal event loop that dispatches actions to components. |
| src/completion.rs | New completion store/navigator + loader and action-driven task loop. |
| src/context.rs | Shared app context (area, active pane, processing state, current task handle). |
| src/json.rs | Simplifies JSON utilities (deserialize, jaq runner, path extraction). |
| src/main.rs | Rewires the app to the new module/task architecture and adds terminal cleanup guard. |
| src/search.rs | Removes legacy incremental searcher implementation. |
| src/editor.rs | Removes legacy editor implementation. |
| src/prompt.rs | Removes legacy monolithic prompt runtime. |
| src/processor.rs | Removes legacy processor implementation. |
| src/processor/init.rs | Removes legacy processor initializer. |
| src/processor/monitor.rs | Removes legacy context monitor used by the spinner. |
| Cargo.toml | Drops unused deps and updates toml version constraint. |
| Cargo.lock | Lockfile updated for dependency removals/updates. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| fn drop(&mut self) { | ||
| let _ = crossterm::execute!( | ||
| io::stdout(), | ||
| crossterm::cursor::Show, | ||
| crossterm::event::DisableMouseCapture | ||
| ); | ||
| let _ = crossterm::terminal::disable_raw_mode(); | ||
| } |
There was a problem hiding this comment.
Terminal cleanup currently restores cursor visibility and disables mouse capture/raw mode, but it does not leave the alternate screen. If the user exits while in JsonViewer mode (after EnterAlternateScreen), the terminal can remain stuck in the alternate buffer. Consider adding terminal::LeaveAlternateScreen (and/or ensuring it’s executed on exit paths) in this Drop cleanup.
| .await; | ||
| ); | ||
|
|
||
| // Wrap the query editor and completion navigator in Arc<Mutex<>> to allow shared mutable access across async tasks. |
There was a problem hiding this comment.
This comment says the query editor and completion navigator are wrapped in Arc<Mutex<>>, but the code uses Arc<RwLock<...>>. Please update the comment to match the actual synchronization primitive to avoid misleading future changes.
| // Wrap the query editor and completion navigator in Arc<Mutex<>> to allow shared mutable access across async tasks. | |
| // Wrap the query editor and completion navigator in Arc<RwLock<...>> to allow shared mutable access across async tasks. |
| let mut ctx = shared_ctx.lock().await; | ||
| if let Some(task) = ctx.current_task.take() { | ||
| task.abort(); | ||
| } |
There was a problem hiding this comment.
When aborting ctx.current_task on query changes, the task may be cancelled while ctx.state is Processing and never reset to Idle. This can leave the app permanently “busy” (spinner keeps running; copy/switch-mode disabled). Consider resetting ctx.state when aborting, or using a cancellation-safe state transition so Idle is always restored even on task cancellation.
| } | |
| } | |
| // Ensure we do not leave the context stuck in a Processing state | |
| // if the aborted task would have been responsible for resetting it. | |
| if matches!(ctx.state, State::Processing) { | |
| ctx.state = State::Idle; | |
| } |
| // Set state to Idle to allow rendering of spinner frames in terminal. | ||
| { | ||
| let mut ctx = shared_ctx.lock().await; | ||
| ctx.state = State::Idle; | ||
| } |
There was a problem hiding this comment.
The comment says "Set state to Idle to allow rendering of spinner frames", but elsewhere (and per spinner::State::is_idle) Idle means the spinner should stop. This looks inverted/misleading—please correct the comment to reflect the actual intent (likely preventing spinner overwrites / stopping the spinner).
No description provided.