1828: feat(TUI): enable various plan rendering formats - #66
1828: feat(TUI): enable various plan rendering formats#66martin-augment wants to merge 7 commits into
Conversation
…h metrics specified)
…WASM support. UI fix
WalkthroughThis PR extends the ballista-cli TUI with multi-format physical plan display and tab-based stage plan navigation. It introduces ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Code Review: feat(TUI): enable various plan rendering formatsOverviewThis PR adds interactive tab switching for plan rendering formats in the Ballista TUI — users can now toggle between Default, Tree, and Metrics views for both job-level physical plans and stage-level plans. It also introduces a caching layer for fetched responses and removes the previous static The approach is sound and the feature is a meaningful UX improvement. Several issues need attention before merge. IssuesBug:
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces support for rendering physical plan trees and stage plans (Default, Tree, and Metrics formats) asynchronously within the Ballista CLI TUI. The changes include updates to the HTTP client, event handling, domain models, and UI rendering to fetch and cache these plans. The review feedback highlights several potential race conditions where asynchronous responses could be applied to the wrong popup if a user switches views while a request is in flight, suggesting that the job_id be passed and verified. Additionally, the reviewer recommends mapping the default plan tab to None instead of an empty string to avoid unnecessary empty query parameters in API requests.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| if let Some(p) = &mut self.job_plan_popup { | ||
| p.details.physical_plan_tree = details.physical_plan; | ||
| } |
There was a problem hiding this comment.
There is a potential race condition here. If the user closes the popup or opens a popup for a different job while the async get_job_details request is in flight, the returned tree plan will be applied to the wrong popup. We should verify that the popup's job_id matches the job_id of the fetched plan.
if let Some(p) = &mut self.job_plan_popup {
if p.details.job_id == job_id {
p.details.physical_plan_tree = details.physical_plan;
}
}| if let Some(popup) = &mut self.job_plan_popup { | ||
| popup.details.physical_plan_tree = details.physical_plan_tree; | ||
| } |
There was a problem hiding this comment.
Similar to the non-web path, there is a potential race condition where the returned tree plan is applied to the wrong popup if the user switched popups while the async request was in flight. We should verify that the popup's job_id matches the job_id of the fetched plan.
| if let Some(popup) = &mut self.job_plan_popup { | |
| popup.details.physical_plan_tree = details.physical_plan_tree; | |
| } | |
| if let Some(popup) = &mut self.job_plan_popup { | |
| if popup.details.job_id == details.job_id { | |
| popup.details.physical_plan_tree = details.physical_plan_tree; | |
| } | |
| } |
| UiData::JobStagesPlanData(tab, stages) => { | ||
| if let Some(popup) = &mut self.job_stages_popup { | ||
| popup.cache_plan_response(tab, stages); | ||
| } |
There was a problem hiding this comment.
There is a potential race condition where the returned stage plan is cached in the wrong popup if the user switched popups while the async request was in flight. We should include the job_id in UiData::JobStagesPlanData and verify that the popup's job_id matches before caching.
UiData::JobStagesPlanData(job_id, tab, stages) => {
if let Some(popup) = &mut self.job_stages_popup {
if popup.job_id == job_id {
popup.cache_plan_response(tab, stages);
}
}
}| #[allow(dead_code)] | ||
| JobStagesPlanData(StagePlanTab, JobStagesResponse), |
There was a problem hiding this comment.
To prevent race conditions when caching stage plans, we should include the job_id in the JobStagesPlanData variant so that the receiver can verify it belongs to the currently active popup.
| #[allow(dead_code)] | |
| JobStagesPlanData(StagePlanTab, JobStagesResponse), | |
| #[allow(dead_code)] | |
| JobStagesPlanData(String, StagePlanTab, JobStagesResponse), |
| app.send_event(Event::DataLoaded { | ||
| data: UiData::JobStagesPlanData(tab, stages), | ||
| }) |
There was a problem hiding this comment.
Pass the job_id along with the JobStagesPlanData event to allow the receiver to verify and prevent race conditions.
| app.send_event(Event::DataLoaded { | |
| data: UiData::JobStagesPlanData(tab, stages), | |
| }) | |
| app.send_event(Event::DataLoaded { | |
| data: UiData::JobStagesPlanData(job_id.to_string(), tab, stages), | |
| }) |
| stages | ||
| .stages | ||
| .sort_by_key(|s| s.id.parse::<u64>().unwrap_or(u64::MAX)); | ||
| send_data(UiData::JobStagesPlanData(tab, stages), tx).await; |
There was a problem hiding this comment.
Pass the job_id along with the JobStagesPlanData event in the web path to prevent race conditions.
send_data(UiData::JobStagesPlanData(job_id, tab, stages), tx).await;| let fmt = match tab { | ||
| StagePlanTab::Default => Some(""), | ||
| StagePlanTab::Tree => Some("tree"), | ||
| StagePlanTab::Metrics => Some("metrics"), | ||
| }; |
There was a problem hiding this comment.
Instead of mapping StagePlanTab::Default to Some("") which appends an empty query parameter ?plan_format=, we can map it to None so that no query parameter is appended at all, which is cleaner and more standard.
| let fmt = match tab { | |
| StagePlanTab::Default => Some(""), | |
| StagePlanTab::Tree => Some("tree"), | |
| StagePlanTab::Metrics => Some("metrics"), | |
| }; | |
| let fmt = match tab { | |
| StagePlanTab::Default => None, | |
| StagePlanTab::Tree => Some("tree"), | |
| StagePlanTab::Metrics => Some("metrics"), | |
| }; |
| let fmt = match tab { | ||
| StagePlanTab::Default => Some(""), | ||
| StagePlanTab::Tree => Some("tree"), | ||
| StagePlanTab::Metrics => Some("metrics"), | ||
| }; |
There was a problem hiding this comment.
Map StagePlanTab::Default to None in the web path as well to avoid appending an empty query parameter.
let fmt = match tab {
StagePlanTab::Default => None,
StagePlanTab::Tree => Some("tree"),
StagePlanTab::Metrics => Some("metrics"),
};![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit d1e0caa. Configure here.
|
|
||
| pub fn set_plan_view(&mut self) { | ||
| self.details_view = StageDetailsView::Plan; | ||
| self.details_view = StageDetailsView::Plan(StagePlanTab::Default); |
There was a problem hiding this comment.
Stale plan after reopening view
Medium Severity
After switching stage plans to tree or metrics, leaving plan view and opening it again with p sets the active tab to default but leaves stages on the last fetched format, so the UI can show the wrong plan text until the user presses d, t, or m.
Reviewed by Cursor Bugbot for commit d1e0caa. Configure here.
| StagePlanTab::Default => Some(""), | ||
| StagePlanTab::Tree => Some("tree"), | ||
| StagePlanTab::Metrics => Some("metrics"), | ||
| }; |
There was a problem hiding this comment.
Empty default plan_format query
Low Severity
Default stage plan reloads pass Some("") into get_job_stages, which builds ?plan_format= instead of omitting the parameter or using default. The scheduler expects default, tree, or metrics, so a default-tab fetch can fail when the default cache is missing.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit d1e0caa. Configure here.
🤖 Augment PR SummarySummary: This PR extends the Ballista CLI TUI/web UI to support additional plan rendering formats. Changes:
Technical Notes: Plan-format selection is implemented via the scheduler REST API’s 🤖 Was this summary useful? React with 👍 or 👎 |
![augmentcode[bot]](https://avatars.githubusercontent.com/in/1027498?s=60&v=4){kind=small}
| @@ -57,8 +39,6 @@ pub struct Settings { | |||
| pub data_reload_interval_ms: u64, | |||
| /// How often to refresh the UI. In millis. | |||
| pub repaint_interval_ms: u64, | |||
There was a problem hiding this comment.
In ballista-cli/src/tui/infrastructure/config.rs, Settings no longer has job, but DEFAULT_CONFIG (lines 44-58) and the web query-string config builder still emit a job: section, so try_deserialize() will likely fail with an unknown-field error at startup. Consider updating/removing those config sources to match the new Settings schema.
Severity: high
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
| use crate::tui::domain::jobs::stages::StagePlanTab; | ||
|
|
||
| let fmt = match tab { | ||
| StagePlanTab::Default => Some(""), |
There was a problem hiding this comment.
StagePlanTab::Default => Some("") will construct a ?plan_format= query if it ever triggers a fetch, but the scheduler deserializes plan_format into an enum and an empty value is likely rejected as an unknown variant. Consider representing default by omitting the query parameter (or using default) to avoid 400s on stage-plan loads.
Severity: medium
Other Locations
ballista-cli/src/tui/mod.rs:361
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
ballista-cli/src/tui/ui/main/jobs/mod.rs (1)
115-119: ⚡ Quick winUse
Nonefor default plan format instead ofSome("").On Line 116,
Some("")sends?plan_format=while other default stage loads useNone(Line 89). Using one representation avoids inconsistent server/cache behavior.Suggested fix
let fmt = match tab { - StagePlanTab::Default => Some(""), + StagePlanTab::Default => None, StagePlanTab::Tree => Some("tree"), StagePlanTab::Metrics => Some("metrics"), };Please apply the same mapping change in
ballista-cli/src/tui/mod.rs(Line 361).🤖 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 `@ballista-cli/src/tui/ui/main/jobs/mod.rs` around lines 115 - 119, Change the default plan format mapping to use None instead of Some(""): in the match that assigns fmt for StagePlanTab (the arm StagePlanTab::Default currently returns Some(""), change it to None), and apply the identical change to the other occurrence where fmt is set for the same StagePlanTab variants in the TUI module (the other match that maps StagePlanTab::Default to Some("") should be changed to None as well) so both places consistently omit the plan_format parameter.
🤖 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 `@ballista-cli/src/tui/app.rs`:
- Around line 858-875: Async responses must be correlated by job_id before
mutating popups: when handling UiData::JobDetails ensure you only set
popup.details.physical_plan_tree if self.job_plan_popup exists and its job_id
matches the JobDetails' job_id (otherwise set self.job_details); similarly,
change UiData::JobStagesPlanData to carry job_id and in the match check that if
let Some(popup) = &mut self.job_stages_popup { popup.job_id == job_id &&
popup.cache_plan_response(tab, stages) } so you only cache for the matching
JobStagesPopup; update all emission sites of UiData::JobStagesPlanData to
include the job_id.
In `@ballista-cli/src/tui/domain/jobs/stages.rs`:
- Around line 166-170: set_plan_view currently sets details_view to
StagePlanTab::Default and resets scroll positions but doesn't clear or reset the
plan payload, so a previously-loaded Tree/Metrics payload can be shown under the
Default tab; update the set_plan_view method to also reset the stored plan data
(e.g. clear or set self.stages / the plan payload field to the default/empty
payload for the Default tab) so reopening the plan view cannot display stale
non-default content, keeping the existing resets of
plan_vertical_scroll_position and plan_horizontal_scroll_position.
In `@ballista-cli/src/tui/event.rs`:
- Around line 41-42: The variant JobStagesPlanData currently holds only
(StagePlanTab, JobStagesResponse) which allows a delayed response to overwrite
state for a different job; change the enum variant to include the originating
job identifier (e.g., JobStagesPlanData(JobId, StagePlanTab,
JobStagesResponse)), update all creators/emitters to supply the job_id, and
update all pattern matches/consumers (places that destructure JobStagesPlanData
and any response-matching logic) to compare the incoming job_id against the
currently active job before applying the payload so stale responses are ignored.
In `@ballista-cli/src/tui/ui/footer.rs`:
- Around line 41-42: Update the footer hint to match the actual key handling for
the job-dot popup: locate the two pushes that build current_view_key_bindings
(the Span::from lines) and change the horizontal-arrows hint to only show
vertical scroll keys (e.g., replace "[↑↓←→] Scroll, " with "[↑↓] Scroll, " or
similar) so the displayed keys reflect the implemented up/down-only scrolling
for the dot popup.
---
Nitpick comments:
In `@ballista-cli/src/tui/ui/main/jobs/mod.rs`:
- Around line 115-119: Change the default plan format mapping to use None
instead of Some(""): in the match that assigns fmt for StagePlanTab (the arm
StagePlanTab::Default currently returns Some(""), change it to None), and apply
the identical change to the other occurrence where fmt is set for the same
StagePlanTab variants in the TUI module (the other match that maps
StagePlanTab::Default to Some("") should be changed to None as well) so both
places consistently omit the plan_format parameter.
🪄 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: CHILL
Plan: Pro
Run ID: eb0cf81d-d04b-471b-a956-836db99f5b41
📒 Files selected for processing (15)
.cursor/rules.mdAGENTS.mdCLAUDE.mdballista-cli/src/tui/app.rsballista-cli/src/tui/domain/jobs.rsballista-cli/src/tui/domain/jobs/stages.rsballista-cli/src/tui/event.rsballista-cli/src/tui/http_client.rsballista-cli/src/tui/infrastructure/config.rsballista-cli/src/tui/mod.rsballista-cli/src/tui/ui/footer.rsballista-cli/src/tui/ui/main/jobs/job_plan_popup.rsballista-cli/src/tui/ui/main/jobs/mod.rsballista-cli/src/tui/ui/main/mod.rsballista-cli/src/tui/ui/mod.rs
💤 Files with no reviewable changes (1)
- ballista-cli/src/tui/infrastructure/config.rs
| if details.physical_plan_tree.is_some() { | ||
| if let Some(popup) = &mut self.job_plan_popup { | ||
| popup.details.physical_plan_tree = details.physical_plan_tree; | ||
| } | ||
| } else { | ||
| self.job_details = Some(details); | ||
| } | ||
| } | ||
| UiData::JobStagesGraph(graph) => { | ||
| self.job_dot_popup = Some(graph); | ||
| } | ||
| UiData::JobStagesData(job_id, stages) => { | ||
| self.job_stages_popup = Some(JobStagesPopup::new(job_id, stages)); | ||
| } | ||
| UiData::JobStagesPlanData(tab, stages) => { | ||
| if let Some(popup) = &mut self.job_stages_popup { | ||
| popup.cache_plan_response(tab, stages); | ||
| } |
There was a problem hiding this comment.
Guard async popup updates with job_id correlation.
Out-of-order async responses can update the wrong popup state. JobDetails tree updates and JobStagesPlanData cache writes currently apply to whichever popup is open, without verifying job identity.
Proposed fix
UiData::JobDetails(details) => {
if details.physical_plan_tree.is_some() {
- if let Some(popup) = &mut self.job_plan_popup {
+ if let Some(popup) = &mut self.job_plan_popup
+ && popup.details.job_id == details.job_id
+ {
popup.details.physical_plan_tree = details.physical_plan_tree;
}
} else {
self.job_details = Some(details);
}
}
-UiData::JobStagesPlanData(tab, stages) => {
- if let Some(popup) = &mut self.job_stages_popup {
+UiData::JobStagesPlanData(job_id, tab, stages) => {
+ if let Some(popup) = &mut self.job_stages_popup
+ && popup.job_id == job_id
+ {
popup.cache_plan_response(tab, stages);
}
}This also requires extending the UiData::JobStagesPlanData payload at emission sites to include job_id.
🤖 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 `@ballista-cli/src/tui/app.rs` around lines 858 - 875, Async responses must be
correlated by job_id before mutating popups: when handling UiData::JobDetails
ensure you only set popup.details.physical_plan_tree if self.job_plan_popup
exists and its job_id matches the JobDetails' job_id (otherwise set
self.job_details); similarly, change UiData::JobStagesPlanData to carry job_id
and in the match check that if let Some(popup) = &mut self.job_stages_popup {
popup.job_id == job_id && popup.cache_plan_response(tab, stages) } so you only
cache for the matching JobStagesPopup; update all emission sites of
UiData::JobStagesPlanData to include the job_id.
| pub fn set_plan_view(&mut self) { | ||
| self.details_view = StageDetailsView::Plan; | ||
| self.details_view = StageDetailsView::Plan(StagePlanTab::Default); | ||
| self.plan_vertical_scroll_position = 0; | ||
| self.plan_horizontal_scroll_position = 0; | ||
| } |
There was a problem hiding this comment.
Default plan tab can display stale non-default payload.
On Line 167, set_plan_view switches to StagePlanTab::Default but leaves self.stages untouched. If Tree/Metrics was previously loaded, reopening plan view can show the wrong content under the default tab.
Suggested fix
pub fn set_plan_view(&mut self) {
self.details_view = StageDetailsView::Plan(StagePlanTab::Default);
+ if let Some(default_resp) = self.plan_cache.default.as_ref() {
+ self.stages = default_resp.clone();
+ }
self.plan_vertical_scroll_position = 0;
self.plan_horizontal_scroll_position = 0;
}🤖 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 `@ballista-cli/src/tui/domain/jobs/stages.rs` around lines 166 - 170,
set_plan_view currently sets details_view to StagePlanTab::Default and resets
scroll positions but doesn't clear or reset the plan payload, so a
previously-loaded Tree/Metrics payload can be shown under the Default tab;
update the set_plan_view method to also reset the stored plan data (e.g. clear
or set self.stages / the plan payload field to the default/empty payload for the
Default tab) so reopening the plan view cannot display stale non-default
content, keeping the existing resets of plan_vertical_scroll_position and
plan_horizontal_scroll_position.
| #[allow(dead_code)] | ||
| JobStagesPlanData(StagePlanTab, JobStagesResponse), |
There was a problem hiding this comment.
JobStagesPlanData is missing job_id, enabling cross-job stale-response overwrite.
Line 42 carries only tab + payload. If a delayed async response arrives after the user switches jobs, the active popup can cache stages for the wrong job.
Contract fix
- JobStagesPlanData(StagePlanTab, JobStagesResponse),
+ JobStagesPlanData(String, StagePlanTab, JobStagesResponse),- data: UiData::JobStagesPlanData(tab, stages),
+ data: UiData::JobStagesPlanData(job_id.to_owned(), tab, stages),- UiData::JobStagesPlanData(tab, stages) => {
- if let Some(popup) = &mut self.job_stages_popup {
- popup.cache_plan_response(tab, stages);
- }
- }
+ UiData::JobStagesPlanData(job_id, tab, stages) => {
+ if let Some(popup) = &mut self.job_stages_popup
+ && popup.job_id == job_id
+ {
+ popup.cache_plan_response(tab, stages);
+ }
+ }🤖 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 `@ballista-cli/src/tui/event.rs` around lines 41 - 42, The variant
JobStagesPlanData currently holds only (StagePlanTab, JobStagesResponse) which
allows a delayed response to overwrite state for a different job; change the
enum variant to include the originating job identifier (e.g.,
JobStagesPlanData(JobId, StagePlanTab, JobStagesResponse)), update all
creators/emitters to supply the job_id, and update all pattern matches/consumers
(places that destructure JobStagesPlanData and any response-matching logic) to
compare the incoming job_id against the currently active job before applying the
payload so stale responses are ignored.
| current_view_key_bindings.push(Span::from("[↑↓←→] Scroll, ")); | ||
| current_view_key_bindings.push(Span::from("[Esc] Close popup, ")); |
There was a problem hiding this comment.
Fix job-dot footer hint to match actual key handling.
The footer shows horizontal scroll keys for the dot popup, but the handler only supports up/down scrolling.
Proposed fix
- current_view_key_bindings.push(Span::from("[↑↓←→] Scroll, "));
+ current_view_key_bindings.push(Span::from("[↑↓] Scroll, "));📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| current_view_key_bindings.push(Span::from("[↑↓←→] Scroll, ")); | |
| current_view_key_bindings.push(Span::from("[Esc] Close popup, ")); | |
| current_view_key_bindings.push(Span::from("[↑↓] Scroll, ")); | |
| current_view_key_bindings.push(Span::from("[Esc] Close popup, ")); |
🤖 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 `@ballista-cli/src/tui/ui/footer.rs` around lines 41 - 42, Update the footer
hint to match the actual key handling for the job-dot popup: locate the two
pushes that build current_view_key_bindings (the Span::from lines) and change
the horizontal-arrows hint to only show vertical scroll keys (e.g., replace
"[↑↓←→] Scroll, " with "[↑↓] Scroll, " or similar) so the displayed keys reflect
the implemented up/down-only scrolling for the dot popup.
3 participants
1828: To review by AI