Skip to content

1828: feat(TUI): enable various plan rendering formats #66

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
martin-augment wants to merge 7 commits into
base: main
Choose a base branch
from
Open

1828: feat(TUI): enable various plan rendering formats #66

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
d52edf2
Added support for stages' plan formatting (default, tree-like and wit…
sandugood Jun 7, 2026
7d7999c
Formatting fixed
sandugood Jun 7, 2026
2689c3d
Fixed clippy warnings
sandugood Jun 7, 2026
862c39d
Addressing all of the comments. Refactoring and adding logic for the …
sandugood Jun 8, 2026
eab60d0
Merge remote-tracking branch 'upstream/main' into feat/tui-plan-format
sandugood Jun 8, 2026
85a0783
Merge branch 'main' into feat/tui-plan-format
martin-g Jun 9, 2026
d1e0caa
Add CLAUDE.md & AGENTS.md
martin-g Jun 9, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .cursor/rules.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
The AI agents should never link to any issue or a pull request
in any GitHub repository in the code reviews!

The AI agents should not review AI agents' config files like CLAUDE.md or AGENTS.md!

5 changes: 5 additions & 0 deletions AGENTS.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
The AI agents should never link to any issue or a pull request
in any GitHub repository in the code reviews!

The AI agents should not review AI agents' config files like CLAUDE.md or AGENTS.md!

5 changes: 5 additions & 0 deletions CLAUDE.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
The AI agents should never link to any issue or a pull request
in any GitHub repository in the code reviews!

The AI agents should not review AI agents' config files like CLAUDE.md or AGENTS.md!

224 changes: 177 additions & 47 deletions ballista-cli/src/tui/app.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,8 @@
#[cfg(not(feature = "web"))]
use crate::tui::TuiError;
use crate::tui::TuiResult;
#[cfg(feature = "web")]
use crate::tui::domain::jobs::stages::StagePlanTab;
use crate::tui::event::Event;
#[cfg(feature = "web")]
use crate::tui::event::web::Sender;
Expand All @@ -28,8 +30,8 @@ use crate::tui::{
ExecutorDetailsPopup, ExecutorsData, SortColumn as ExecutorsSortColumn,
},
jobs::{
CancelJobResult, JobDetails, JobPlansPopup, JobsData, PlanTab,
SortColumn as JobsSortColumn,
CancelJobResult, JobDetails, JobPlansPopup, JobsData, PhysicalFormat,
PlanTab, SortColumn as JobsSortColumn,
stages::{JobStagesPopup, StagesGraph},
},
metrics::MetricsData,
Expand All @@ -52,7 +54,7 @@ use crate::tui::http_client::HttpClient;
#[cfg(not(feature = "web"))]
use crate::tui::ui::{
load_executor_details_popup, load_executors_data, load_job_details, load_job_dot,
load_job_stages_popup, load_jobs_data, load_metrics_data,
load_job_stages_popup, load_jobs_data, load_metrics_data, load_stage_plan,
};

const INVALID_DATE: &str = "Invalid date";
Expand Down Expand Up @@ -221,13 +223,47 @@ impl App {
_ => {}
}
} else if popup.is_plan_view() {
match key.code {
KeyCode::Esc => popup.set_no_details_view(),
KeyCode::Up => popup.scroll_up(),
KeyCode::Down => popup.scroll_down(),
KeyCode::Left => popup.scroll_left(),
KeyCode::Right => popup.scroll_right(),
_ => {}
use crate::tui::domain::jobs::stages::StagePlanTab;

// Collect job_id + tab to fetch while popup is still borrowed,
// then drop the borrow before the async call.
let fetch = match key.code {
KeyCode::Char('d') => popup
.set_tab(StagePlanTab::Default)
.map(|tab| (popup.job_id.clone(), tab)),
KeyCode::Char('t') => popup
.set_tab(StagePlanTab::Tree)
.map(|tab| (popup.job_id.clone(), tab)),
KeyCode::Char('m') => popup
.set_tab(StagePlanTab::Metrics)
.map(|tab| (popup.job_id.clone(), tab)),
KeyCode::Esc => {
popup.set_no_details_view();
None
}
KeyCode::Up => {
popup.scroll_up();
None
}
KeyCode::Down => {
popup.scroll_down();
None
}
KeyCode::Left => {
popup.scroll_left();
None
}
KeyCode::Right => {
popup.scroll_right();
None
}
_ => None,
};

if let Some((job_id, tab)) = fetch
&& let Err(e) = load_stage_plan(self, &job_id, tab).await
{
tracing::error!("Failed to load stage plan: {e:?}");
}
} else if popup.is_no_details_view() {
match key.code {
Expand Down Expand Up @@ -265,25 +301,55 @@ impl App {
}

if let Some(ref mut plans_popup) = self.job_plan_popup {
match key.code {
KeyCode::Up => plans_popup.scroll_up(),
KeyCode::Down => plans_popup.scroll_down(),
KeyCode::Left => plans_popup.scroll_left(),
KeyCode::Right => plans_popup.scroll_right(),
KeyCode::Char('s') => {
plans_popup.set_tab(PlanTab::Stage);
}
KeyCode::Char('p') => {
plans_popup.set_tab(PlanTab::Physical);
}
KeyCode::Char('l') => {
plans_popup.set_tab(PlanTab::Logical);
let fetch_tree = if key.code == KeyCode::Char('t')
&& plans_popup.get_tab() == &PlanTab::Physical
{
plans_popup
.set_physical_format(PhysicalFormat::Tree)
.map(|_| plans_popup.details.job_id.clone())
} else {
match key.code {
KeyCode::Up => {
plans_popup.scroll_up();
}
KeyCode::Down => {
plans_popup.scroll_down();
}
KeyCode::Left => {
plans_popup.scroll_left();
}
KeyCode::Right => {
plans_popup.scroll_right();
}
KeyCode::Char('s') => plans_popup.set_tab(PlanTab::Stage),
KeyCode::Char('p') => plans_popup.set_tab(PlanTab::Physical),
KeyCode::Char('l') => plans_popup.set_tab(PlanTab::Logical),
KeyCode::Char('d') if plans_popup.get_tab() == &PlanTab::Physical => {
plans_popup.set_physical_format(PhysicalFormat::Default);
}
KeyCode::Esc => {
self.job_plan_popup = None;
}
_ => {}
}
KeyCode::Esc => {
self.job_plan_popup = None;
None
};

if let Some(job_id) = fetch_tree {
match self
.http_client
.get_job_details(&job_id, Some("tree"))
.await
{
Ok(details) => {
if let Some(p) = &mut self.job_plan_popup {
p.details.physical_plan_tree = details.physical_plan;
}
Comment on lines +345 to +347

@gemini-code-assist gemini-code-assist Bot Jun 9, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

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;
                            }
                        }

}
Err(e) => tracing::error!("Failed to load tree physical plan: {e:?}"),
}
_ => {}
}

return Ok(());
}

Expand Down Expand Up @@ -789,14 +855,25 @@ impl App {
};
}
UiData::JobDetails(details) => {
self.job_details = Some(details);
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;
}
Comment on lines +859 to +861

@gemini-code-assist gemini-code-assist Bot Jun 9, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

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.

Suggested change
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;
}
}

} 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);
}
Comment on lines +872 to +875

@gemini-code-assist gemini-code-assist Bot Jun 9, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

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);
                    }
                }
            }

Comment on lines +858 to +875

@coderabbitai coderabbitai Bot Jun 9, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift

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.

}
UiData::ExecutorDetails(executor) => {
self.executor_details_popup = Some(ExecutorDetailsPopup::new(executor));
}
Expand Down Expand Up @@ -841,14 +918,43 @@ impl App {
_ => {}
}
} else if popup.is_plan_view() {
match key.code {
KeyCode::Esc => popup.set_no_details_view(),
KeyCode::Up => popup.scroll_up(),
KeyCode::Down => popup.scroll_down(),
KeyCode::Left => popup.scroll_left(),
KeyCode::Right => popup.scroll_right(),
_ => {}
}
use crate::tui::domain::jobs::stages::StagePlanTab;

let fetch = match key.code {
KeyCode::Char('d') => popup
.set_tab(StagePlanTab::Default)
.map(|tab| (popup.job_id.clone(), tab)),
KeyCode::Char('t') => popup
.set_tab(StagePlanTab::Tree)
.map(|tab| (popup.job_id.clone(), tab)),
KeyCode::Char('m') => popup
.set_tab(StagePlanTab::Metrics)
.map(|tab| (popup.job_id.clone(), tab)),
KeyCode::Esc => {
popup.set_no_details_view();
None
}
KeyCode::Up => {
popup.scroll_up();
None
}
KeyCode::Down => {
popup.scroll_down();
None
}
KeyCode::Left => {
popup.scroll_left();
None
}
KeyCode::Right => {
popup.scroll_right();
None
}
_ => None,
};

return fetch
.map(|(job_id, tab)| WebKeyAsyncAction::LoadStagePlan(job_id, tab));
} else if popup.is_no_details_view() {
match key.code {
KeyCode::Up => popup.scroll_up(),
Expand Down Expand Up @@ -879,18 +985,39 @@ impl App {
}

if let Some(ref mut plans_popup) = self.job_plan_popup {
match key.code {
KeyCode::Up => plans_popup.scroll_up(),
KeyCode::Down => plans_popup.scroll_down(),
KeyCode::Left => plans_popup.scroll_left(),
KeyCode::Right => plans_popup.scroll_right(),
KeyCode::Char('s') => plans_popup.set_tab(PlanTab::Stage),
KeyCode::Char('p') => plans_popup.set_tab(PlanTab::Physical),
KeyCode::Char('l') => plans_popup.set_tab(PlanTab::Logical),
KeyCode::Esc => self.job_plan_popup = None,
_ => {}
}
return None;
let fetch_tree = if key.code == KeyCode::Char('t')
&& plans_popup.get_tab() == &PlanTab::Physical
{
plans_popup
.set_physical_format(PhysicalFormat::Tree)
.map(|_| plans_popup.details.job_id.clone())
} else {
match key.code {
KeyCode::Up => {
plans_popup.scroll_up();
}
KeyCode::Down => {
plans_popup.scroll_down();
}
KeyCode::Left => {
plans_popup.scroll_left();
}
KeyCode::Right => {
plans_popup.scroll_right();
}
KeyCode::Char('s') => plans_popup.set_tab(PlanTab::Stage),
KeyCode::Char('p') => plans_popup.set_tab(PlanTab::Physical),
KeyCode::Char('l') => plans_popup.set_tab(PlanTab::Logical),
KeyCode::Char('d') if plans_popup.get_tab() == &PlanTab::Physical => {
plans_popup.set_physical_format(PhysicalFormat::Default);
}
KeyCode::Esc => self.job_plan_popup = None,
_ => {}
}
None
};

return fetch_tree.map(WebKeyAsyncAction::LoadJobPlanTree);
}

if let Some(ref mut executor_popup) = self.executor_details_popup {
Expand Down Expand Up @@ -1084,6 +1211,8 @@ pub enum WebKeyAsyncAction {
CancelJob(String),
UpdateJobDetails(Option<String>),
ReloadView,
LoadJobPlanTree(String),
LoadStagePlan(String, StagePlanTab),
}

#[cfg(test)]
Expand Down Expand Up @@ -1199,6 +1328,7 @@ mod tests {
job_id: job_id.to_string(),
logical_plan: Some("logical".to_string()),
physical_plan: Some("physical".to_string()),
physical_plan_tree: Some("tree".to_string()),
stage_plan: Some("stage".to_string()),
}
}
Expand Down
28 changes: 28 additions & 0 deletions ballista-cli/src/tui/domain/jobs.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -219,11 +219,18 @@ pub enum CancelJobResult {
Failure { job_id: String, error: String },
}

#[derive(Debug, Clone, PartialEq)]
pub enum PhysicalFormat {
Default,
Tree,
}

#[derive(Clone, Debug)]
pub struct JobDetails {
pub job_id: String,
pub logical_plan: Option<String>,
pub physical_plan: Option<String>,
pub physical_plan_tree: Option<String>,
pub stage_plan: Option<String>,
}

Expand All @@ -238,6 +245,7 @@ pub(crate) enum PlanTab {
pub struct JobPlansPopup {
pub details: JobDetails,
tab: PlanTab,
physical_format: PhysicalFormat,
vertical_scroll_position: u16,
horizontal_scroll_position: u16,
}
Expand All @@ -247,6 +255,7 @@ impl JobPlansPopup {
Self {
details,
tab,
physical_format: PhysicalFormat::Default,
vertical_scroll_position: 0,
horizontal_scroll_position: 0,
}
Expand All @@ -256,6 +265,24 @@ impl JobPlansPopup {
&self.tab
}

pub fn get_physical_format(&self) -> &PhysicalFormat {
&self.physical_format
}

/// Returns Some(()) if a fetch is needed (tree not cached yet), None if already available.
pub fn set_physical_format(&mut self, fmt: PhysicalFormat) -> Option<()> {
self.physical_format = fmt;
self.vertical_scroll_position = 0;
self.horizontal_scroll_position = 0;
if self.physical_format == PhysicalFormat::Tree
&& self.details.physical_plan_tree.is_none()
{
Some(())
} else {
None
}
}

pub fn set_tab(&mut self, tab: PlanTab) {
self.tab = tab;
self.vertical_scroll_position = 0;
Expand Down Expand Up @@ -696,6 +723,7 @@ mod tests {
fn make_job_details(id: &str) -> JobDetails {
JobDetails {
job_id: id.to_string(),
physical_plan_tree: None,
logical_plan: None,
physical_plan: None,
stage_plan: None,
Expand Down
Loading
Loading