Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
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
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
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!

55 changes: 47 additions & 8 deletions ballista-cli/src/tui/app.rs
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,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 +221,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 @@ -797,6 +831,11 @@ impl App {
UiData::JobStagesData(job_id, stages) => {
self.job_stages_popup = Some(JobStagesPopup::new(job_id, stages));
}
UiData::JobStagesPlanData(_job_id, tab, stages) => {

@augmentcode augmentcode Bot Jun 7, 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.

ballista-cli/src/tui/app.rs:834: In UiData::JobStagesPlanData(_job_id, ...) the job id is ignored; if a request returns after the user opens a different job’s stages popup, this can cache/update the wrong popup. Consider verifying _job_id matches popup.job_id before calling cache_plan_response.

Severity: medium

Fix This in Augment

🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.

if let Some(popup) = &mut self.job_stages_popup {
popup.cache_plan_response(tab, stages);
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Stale plan data wrong job

High Severity

JobStagesPlanData ignores the event job_id and always calls cache_plan_response on whichever job_stages_popup is open. A late async plan response from job A can populate the cache for job B after the user switches jobs or reloads stages.

Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit 881a59a. Configure here.

}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Web plan format keys missing

Medium Severity

The footer advertises [d], [t], and [m] in the stage plan popup, but the WASM on_key_sync plan branch only handles Esc and scrolling. load_stage_plan and WebKeyAsyncAction were not extended, so web users cannot switch formats.

Additional Locations (1)
Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit 881a59a. Configure here.

Comment on lines +834 to +838

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 | ⚡ Quick win

Use job_id to reject stale plan responses before mutating popup state.

At Line 834, _job_id is dropped. If responses arrive out of order, a plan payload for job A can be cached into an open popup for job B.

✅ Suggested guard in apply_ui_data
-            UiData::JobStagesPlanData(_job_id, 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/app.rs` around lines 834 - 838,
UiData::JobStagesPlanData currently ignores the job_id and always calls
job_stages_popup.cache_plan_response, which can allow stale responses to
overwrite a popup for another job; update apply_ui_data to capture the job_id
from UiData::JobStagesPlanData and compare it against the currently-open popup's
job id (via self.job_stages_popup.as_ref().map(|p| p.job_id()) or similar) and
only call cache_plan_response(tab, stages) when they match, otherwise drop the
response; reference UiData::JobStagesPlanData, self.job_stages_popup, and
cache_plan_response when making this guard.

UiData::ExecutorDetails(executor) => {
self.executor_details_popup = Some(ExecutorDetailsPopup::new(executor));
}
Expand Down
72 changes: 68 additions & 4 deletions ballista-cli/src/tui/domain/jobs/stages.rs
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@
// KIND, either express or implied. See the License for the
// specific language governing permissions and limitations
// under the License.

#![allow(unfulfilled_lint_expectations)]
use ratatui::widgets::{ScrollbarState, TableState};
use serde::Deserialize;

Expand Down Expand Up @@ -67,17 +67,33 @@ pub struct TaskPercentiles {
pub p75: u64,
}

#[derive(Debug, Default)]
pub struct PlanCache {
pub default: Option<JobStagesResponse>,
pub tree: Option<JobStagesResponse>,
pub metrics: Option<JobStagesResponse>,
}

#[derive(Debug, PartialEq)]
pub enum StageDetailsView {
None,
Tasks,
Plan,
Plan(StagePlanTab),
}

#[allow(dead_code)]
#[derive(Debug, Clone, PartialEq)]
pub enum StagePlanTab {
Default,
Tree,
Metrics,
}

#[derive(Debug)]
pub struct JobStagesPopup {
pub job_id: String,
pub stages: JobStagesResponse,
pub plan_cache: PlanCache,
pub table_state: TableState,
pub scrollbar_state: ScrollbarState,
pub tasks_table_state: TableState,
Expand All @@ -92,6 +108,10 @@ impl JobStagesPopup {
Self {
job_id,
scrollbar_state: ScrollbarState::new(stages.stages.len()),
plan_cache: PlanCache {
default: Some(stages.clone()),
..Default::default()
},

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Default tab never refetches plan

Medium Severity

The popup seeds plan_cache.default from the initial stages load and set_tab(Default) always treats that as a cache hit, so [d] Default format never calls load_stage_plan. With job.stage.plan.tree enabled, that cached data is tree-shaped, not the API default indent format.

Additional Locations (1)
Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit 881a59a. Configure here.

stages,
table_state: TableState::default(),
tasks_table_state: TableState::default(),
Expand All @@ -102,6 +122,36 @@ impl JobStagesPopup {
}
}

pub fn cache_plan_response(&mut self, fmt: StagePlanTab, resp: JobStagesResponse) {
match fmt {
StagePlanTab::Default => self.plan_cache.default = Some(resp.clone()),
StagePlanTab::Tree => self.plan_cache.tree = Some(resp.clone()),
StagePlanTab::Metrics => self.plan_cache.metrics = Some(resp.clone()),
}
// If we're currently on that tab, update the live stages too so the
// selection / scroll state is preserved.
let active_fmt = self.active_plan_format();
if active_fmt == Some(fmt) {
self.stages = resp;
}
}
Comment on lines +125 to +137

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

In cache_plan_response, resp.clone() is called unconditionally for every match arm, even if the tab being cached is not the active one. Since JobStagesResponse can be quite large (containing all stages and tasks), this unnecessary cloning can be expensive.

We can optimize this by checking if the tab is active first, cloning only if necessary, and then moving resp into the cache.

    pub fn cache_plan_response(&mut self, fmt: StagePlanTab, resp: JobStagesResponse) {
        let active_fmt = self.active_plan_format();
        if active_fmt.as_ref() == Some(&fmt) {
            self.stages = resp.clone();
        }
        match fmt {
            StagePlanTab::Default => self.plan_cache.default = Some(resp),
            StagePlanTab::Tree => self.plan_cache.tree = Some(resp),
            StagePlanTab::Metrics => self.plan_cache.metrics = Some(resp),
        }
    }


#[allow(dead_code)]
pub fn cached_response(&self, tab: &StagePlanTab) -> Option<JobStagesResponse> {
match tab {
StagePlanTab::Default => self.plan_cache.default.clone(),
StagePlanTab::Tree => self.plan_cache.tree.clone(),
StagePlanTab::Metrics => self.plan_cache.metrics.clone(),
}
}
Comment on lines +139 to +146

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

cached_response currently returns an owned Option<JobStagesResponse>, which forces a clone of the cached response. Since this is a helper method, returning a reference Option<&JobStagesResponse> is more idiomatic and avoids unnecessary cloning when callers only need to inspect the cached data.

Suggested change
#[allow(dead_code)]
pub fn cached_response(&self, tab: &StagePlanTab) -> Option<JobStagesResponse> {
match tab {
StagePlanTab::Default => self.plan_cache.default.clone(),
StagePlanTab::Tree => self.plan_cache.tree.clone(),
StagePlanTab::Metrics => self.plan_cache.metrics.clone(),
}
}
#[allow(dead_code)]
pub fn cached_response(&self, tab: &StagePlanTab) -> Option<&JobStagesResponse> {
match tab {
StagePlanTab::Default => self.plan_cache.default.as_ref(),
StagePlanTab::Tree => self.plan_cache.tree.as_ref(),
StagePlanTab::Metrics => self.plan_cache.metrics.as_ref(),
}
}


pub fn active_plan_format(&self) -> Option<StagePlanTab> {
match &self.details_view {
StageDetailsView::Plan(tab) => Some(tab.clone()),
_ => None,
}
}

pub fn plan_vertical_scroll_position(&self) -> u16 {
self.plan_vertical_scroll_position
}
Expand All @@ -117,7 +167,7 @@ impl JobStagesPopup {
}

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;
}
Comment on lines 169 to 173

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 | ⚡ Quick win

Restore default-tab data when entering plan view.

Line 170 switches to Default, but Lines 171-172 only reset scroll state. If a non-default tab was previously active, self.stages can remain on old tab data and render the wrong plan for the active tab.

Proposed fix
 pub fn set_plan_view(&mut self) {
     self.details_view = StageDetailsView::Plan(StagePlanTab::Default);
     self.plan_vertical_scroll_position = 0;
     self.plan_horizontal_scroll_position = 0;
+    if let Some(cached) = self.cached_response(&StagePlanTab::Default) {
+        self.stages = cached;
+    }
 }
🤖 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 169 - 173,
set_plan_view currently sets details_view to
StageDetailsView::Plan(StagePlanTab::Default) and resets scroll positions but
leaves self.stages pointing at whatever tab data was previously active; update
set_plan_view so after switching to StagePlanTab::Default it also restores or
replaces self.stages with the default-tab plan data (e.g., pull the plan from
your tab-indexed storage or call the existing loader/initializer for the default
tab), ensuring you reference the same symbols: set_plan_view,
StageDetailsView::Plan(StagePlanTab::Default), and self.stages (or the tab
storage container) so the rendered plan matches the newly selected Default tab.

Expand All @@ -135,7 +185,21 @@ impl JobStagesPopup {
}

pub fn is_plan_view(&self) -> bool {
self.details_view == StageDetailsView::Plan
matches!(self.details_view, StageDetailsView::Plan(_))
}

#[allow(dead_code)]
pub fn set_tab(&mut self, tab: StagePlanTab) -> Option<StagePlanTab> {
self.details_view = StageDetailsView::Plan(tab.clone());
self.plan_vertical_scroll_position = 0;
self.plan_horizontal_scroll_position = 0;

if let Some(cached) = self.cached_response(&tab) {
self.stages = cached;
None
} else {
Some(tab)
}
}
Comment on lines +191 to 203

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

If cached_response is updated to return a reference Option<&JobStagesResponse> to avoid unnecessary cloning, we should clone the reference here when assigning it to self.stages.

Suggested change
#[allow(dead_code)]
pub fn set_tab(&mut self, tab: StagePlanTab) -> Option<StagePlanTab> {
self.details_view = StageDetailsView::Plan(tab.clone());
self.plan_vertical_scroll_position = 0;
self.plan_horizontal_scroll_position = 0;
if let Some(cached) = self.cached_response(&tab) {
self.stages = cached;
None
} else {
Some(tab)
}
}
#[allow(dead_code)]
pub fn set_tab(&mut self, tab: StagePlanTab) -> Option<StagePlanTab> {
self.details_view = StageDetailsView::Plan(tab.clone());
self.plan_vertical_scroll_position = 0;
self.plan_horizontal_scroll_position = 0;
if let Some(cached) = self.cached_response(&tab) {
self.stages = cached.clone();
None
} else {
Some(tab)
}
}


pub fn scroll_down(&mut self) {
Expand Down
4 changes: 3 additions & 1 deletion ballista-cli/src/tui/event.rs
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ use crate::tui::domain::{
executors::Executor,
jobs::{
CancelJobResult, Job, JobDetails,
stages::{JobStagesResponse, StagesGraph},
stages::{JobStagesResponse, StagePlanTab, StagesGraph},
},
metrics::Metric,
};
Expand All @@ -38,6 +38,8 @@ pub enum UiData {
JobDetails(JobDetails),
JobStagesGraph(StagesGraph),
JobStagesData(String, JobStagesResponse),
#[allow(dead_code)]
JobStagesPlanData(String, StagePlanTab, JobStagesResponse),
ExecutorDetails(Executor),
CancelJobResult(CancelJobResult),
}
Expand Down
20 changes: 16 additions & 4 deletions ballista-cli/src/tui/http_client.rs
Original file line number Diff line number Diff line change
Expand Up @@ -126,14 +126,26 @@ impl HttpClient {
self.text(&url).await
}

pub async fn get_job_stages(&self, job_id: &str) -> TuiResult<JobStagesResponse> {
pub async fn get_job_stages(
&self,
job_id: &str,
plan_format: Option<&str>,
) -> TuiResult<JobStagesResponse> {
let fmt = plan_format.unwrap_or({
if self.config.job.stage.plan.tree {
"tree"
} else {
""
}
});
Comment on lines +134 to +140

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 | ⚡ Quick win

None currently conflates “use config default” and “explicit default tab”.

At Line 134, None falls back to self.config.job.stage.plan.tree. But the stage-tab loader maps StagePlanTab::Default to None, so selecting the Default tab can still fetch tree when tree mode is enabled in config.

✅ Minimal fix (caller-side) to preserve tab semantics
--- a/ballista-cli/src/tui/ui/main/jobs/mod.rs
+++ b/ballista-cli/src/tui/ui/main/jobs/mod.rs
@@
-    let fmt = match tab {
-        StagePlanTab::Default => None,
+    let fmt = match tab {
+        StagePlanTab::Default => Some("default"),
         StagePlanTab::Tree => Some("tree"),
         StagePlanTab::Metrics => Some("metrics"),
     };
🤖 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/http_client.rs` around lines 134 - 140, The current use
of plan_format: Option<&str> conflates “use config default” with “explicit
default tab” because StagePlanTab::Default is mapped to None; change the mapping
so StagePlanTab::Default does NOT become None (e.g., map Default to Some("") or
Some("default")) or change the caller (stage-tab loader) to pass an explicit
sentinel rather than None; then in this function (the plan_format unwrap_or
block that checks self.config.job.stage.plan.tree) keep None meaning “use config
default” and treat the sentinel Some("") as the explicit default tab so
selecting Default tab won’t accidentally pick tree mode from
self.config.job.stage.plan.tree. Ensure references: plan_format,
self.config.job.stage.plan.tree, and StagePlanTab::Default are updated
accordingly.


let url = self.url(&format!(
"job/{}/stages{}",
self.url_encode(job_id),
if self.config.job.stage.plan.tree {
"?plan_format=tree"
if fmt.is_empty() {
String::new()
} else {
""
format!("?plan_format={fmt}")
}
));
self.json::<JobStagesResponse>(&url).await
Expand Down
2 changes: 1 addition & 1 deletion ballista-cli/src/tui/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -291,7 +291,7 @@ pub(crate) mod web {

match action {
WebKeyAsyncAction::LoadJobStages(id) => {
match http_client.get_job_stages(&id).await {
match http_client.get_job_stages(&id, None).await {
Ok(mut stages) => {
stages
.stages
Expand Down
6 changes: 6 additions & 0 deletions ballista-cli/src/tui/ui/footer.rs
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,12 @@ pub(super) fn render_footer(f: &mut Frame, area: Rect, app: &App) {
} else if app.is_job_stage_plan_popup_open() {
current_view_key_bindings
.push(Span::from("[↑↓] Scroll up/down, "));
Comment on lines 80 to 82

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

The footer currently only lists [↑↓] Scroll up/down for the stage plan view. However, the plan view also supports horizontal scrolling via the left and right arrow keys. Updating the key binding hint to include horizontal scrolling (e.g., [↑↓←→] Scroll) would improve usability and make this feature discoverable.

Suggested change
} else if app.is_job_stage_plan_popup_open() {
current_view_key_bindings
.push(Span::from("[↑↓] Scroll up/down, "));
} else if app.is_job_stage_plan_popup_open() {
current_view_key_bindings
.push(Span::from("[↑↓←→] Scroll, "));

current_view_key_bindings
.push(Span::from("[d] Default format, "));
current_view_key_bindings
.push(Span::from("[t] Tree format, "));
current_view_key_bindings
.push(Span::from("[m] Show metrics, "));
current_view_key_bindings
Comment on lines +83 to 89

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 | 🟡 Minor | ⚡ Quick win

Plan-format shortcuts are shown unconditionally, but web plan view doesn’t handle them.

These hints appear in all builds, while web key handling currently supports only arrow/Esc in plan popup. Please gate these spans for non-web (or implement web d/t/m handling) to avoid misleading controls.

✅ Low-effort mitigation in footer
                         } else if app.is_job_stage_plan_popup_open() {
                             current_view_key_bindings
                                 .push(Span::from("[↑↓] Scroll up/down, "));
+                            #[cfg(not(feature = "web"))]
+                            {
                             current_view_key_bindings
                                 .push(Span::from("[d] Default format, "));
                             current_view_key_bindings
                                 .push(Span::from("[t] Tree format, "));
                             current_view_key_bindings
                                 .push(Span::from("[m] Show metrics, "));
+                            }
                             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 83 - 89, The footer shows
plan-format shortcut spans unconditionally but the web plan view doesn't support
them; update the code that pushes to current_view_key_bindings (the
Span::from("[d] Default format, "), Span::from("[t] Tree format, "),
Span::from("[m] Show metrics, ")) to only add those spans when building for
non-web (e.g., guard with cfg not wasm32 or a runtime check like
!cfg!(target_arch = "wasm32")), so web builds won't display these misleading key
hints; locate the pushes to current_view_key_bindings in footer.rs and wrap them
with that conditional.

.push(Span::from("[Esc] Close popup, "));
} else if app.is_job_stage_tasks_popup_open() {
Expand Down
37 changes: 36 additions & 1 deletion ballista-cli/src/tui/ui/main/jobs/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -81,11 +81,12 @@ pub async fn load_job_dot(app: &App, job_id: &str) -> TuiResult<()> {
}
}

/// Loading whole job's stages to render the popup window
#[cfg(not(feature = "web"))]
pub async fn load_job_stages_popup(app: &App, job_id: &str) -> TuiResult<()> {
let mut stages = app
.http_client
.get_job_stages(job_id)
.get_job_stages(job_id, None)
.await
.inspect(|stages| tracing::trace!("Loaded stages for job '{job_id}': {stages:?}"))
.inspect_err(|e| {
Expand All @@ -102,6 +103,40 @@ pub async fn load_job_stages_popup(app: &App, job_id: &str) -> TuiResult<()> {
.await
}

/// Loading stage's plan to render the popup window
#[cfg(not(feature = "web"))]
pub async fn load_stage_plan(
app: &App,
job_id: &str,
tab: crate::tui::domain::jobs::stages::StagePlanTab,
) -> TuiResult<()> {
use crate::tui::domain::jobs::stages::StagePlanTab;

let fmt = match tab {
StagePlanTab::Default => None,
StagePlanTab::Tree => Some("tree"),
StagePlanTab::Metrics => Some("metrics"),
};
Comment on lines +115 to +119

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

When the user explicitly requests the default plan format (by pressing d), load_stage_plan maps StagePlanTab::Default to None. This causes get_job_stages to fall back to the default configuration (self.config.job.stage.plan.tree). If the user has tree = true in their configuration, pressing d will incorrectly request and display the tree format instead of the default format.

To fix this and bypass the configuration fallback when explicitly switching tabs, map StagePlanTab::Default to Some("") instead of None.

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

Comment on lines +115 to +119

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 | ⚡ Quick win

Make Default tab bypass config fallback explicitly.

Line 116 maps StagePlanTab::Default to None. With the updated HTTP-client contract, None can inherit configured plan_format, so pressing d may still request tree/metrics. That breaks tab semantics and can poison cache entries under the wrong tab key.

🤖 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, The match
arm mapping StagePlanTab::Default currently returns None which allows the HTTP
client to inherit the configured plan_format; change the Default arm to return
an explicit format string (e.g., Some("default")) so it cannot fall back to user
config — update the match that assigns fmt (the mapping of StagePlanTab ->
Option<&str>) so StagePlanTab::Default => Some("default") (or another explicit
token your server recognizes) to guarantee the Default tab bypasses config
fallback and preserves correct cache keys.


let mut stages = app
.http_client
.get_job_stages(job_id, fmt)
.await
.inspect(|s| tracing::trace!("Loaded {fmt:?} plan for job '{job_id}': {s:?}"))
.inspect_err(|e| {
tracing::error!("Failed to load {fmt:?} plan for job '{job_id}': {e:?}")
})?;

stages
.stages
.sort_by_key(|s| s.id.parse::<u64>().unwrap_or(u64::MAX));

app.send_event(Event::DataLoaded {
data: UiData::JobStagesPlanData(job_id.to_owned(), tab, stages),
})
.await
}

#[cfg(not(feature = "web"))]
pub async fn load_job_details(app: &App, job_id: &str) -> TuiResult<()> {
let details = match app.http_client.get_job_details(job_id).await {
Expand Down
5 changes: 4 additions & 1 deletion ballista-cli/src/tui/ui/main/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,10 @@ pub use executors::{executor_details_popup, render_executors};
#[cfg(not(feature = "web"))]
pub use executors::{load_executor_details_popup, load_executors_data};
#[cfg(not(feature = "web"))]
pub use jobs::{load_job_details, load_job_dot, load_job_stages_popup, load_jobs_data};
pub use jobs::{
load_job_details, load_job_dot, load_job_stages_popup, load_jobs_data,
load_stage_plan,
};

#[cfg(feature = "web")]
pub(crate) use jobs::dot_parser;
Expand Down
2 changes: 1 addition & 1 deletion ballista-cli/src/tui/ui/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ pub use main::{
#[cfg(not(feature = "web"))]
pub use main::{
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,
};

use ratatui::{
Expand Down
Loading