Skip to content

1828: feat(TUI): enable various plan rendering formats for stages - #64

Open
martin-augment wants to merge 4 commits into
mainfrom
pr-1828-2026-06-07-19-28-37
Open

1828: feat(TUI): enable various plan rendering formats for stages#64
martin-augment wants to merge 4 commits into
mainfrom
pr-1828-2026-06-07-19-28-37

Conversation

@martin-augment

Copy link
Copy Markdown
Owner

1828: To review by AI

@coderabbitai

coderabbitai Bot commented Jun 7, 2026

Copy link
Copy Markdown

Review Change Stack

Walkthrough

This PR adds multi-format job stage plan viewing to the Ballista TUI. Users can now switch between Default, Tree, and Metrics plan displays via keyboard shortcuts (d/t/m) with cached responses per format. The changes include a new plan-tab domain model with per-format caching, event data extensions, HTTP client API updates to support plan format parameters, plan-loading functions, and UI integration including key binding display in the footer. Configuration files are updated with AI agent review guidelines.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch pr-1828-2026-06-07-19-28-37

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Cursor Bugbot has reviewed your changes and found 3 potential issues.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

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

UiData::JobStagesPlanData(_job_id, tab, stages) => {
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.

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.

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.

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.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Code Review

This pull request introduces a multi-tab stage plan view in the Ballista CLI TUI, allowing users to switch between Default, Tree, and Metrics formats using the d, t, and m keys. It also implements a caching mechanism (PlanCache) to store and reuse loaded stage plans. The review feedback highlights a bug where switching to the default format can incorrectly fall back to the configuration's default (such as tree format), and suggests mapping StagePlanTab::Default to Some("") to resolve this. Additionally, several optimizations are recommended to avoid unnecessary cloning of JobStagesResponse in the caching and retrieval methods, alongside a usability suggestion to update the footer hint to reflect horizontal scrolling support.

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.

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

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 +125 to +137
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;
}
}

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

Comment on lines +139 to +146
#[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(),
}
}

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(),
}
}

Comment on lines +191 to 203
#[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)
}
}

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

Comment on lines 80 to 82
} else if app.is_job_stage_plan_popup_open() {
current_view_key_bindings
.push(Span::from("[↑↓] Scroll up/down, "));

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

@augmentcode

augmentcode Bot commented Jun 7, 2026

Copy link
Copy Markdown
🤖 Augment PR Summary

Summary: This PR enhances the Ballista CLI TUI stage-plan popup to support switching between multiple plan rendering formats.

Changes:

  • Introduced StagePlanTab and extended the stage plan details view to track the active plan format
  • Added a per-format plan response cache (PlanCache) to avoid refetching when toggling between tabs
  • Extended HttpClient::get_job_stages to accept an optional plan_format query parameter
  • Added a new async loader (load_stage_plan) to fetch a specific plan format on demand
  • Updated the app event pipeline with a new UiData::JobStagesPlanData variant to deliver plan responses back to the popup
  • Updated footer help text and key handling to support d/t/m shortcuts for Default/Tree/Metrics formats

Technical Notes: The popup now swaps its displayed JobStagesResponse based on the active tab, while preserving selection/scroll state across server responses via caching.

🤖 Was this summary useful? React with 👍 or 👎

@augmentcode augmentcode Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Review completed. 1 suggestion posted.

Fix All in Augment

Comment augment review to trigger a new review at any time.

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.

@claude

claude Bot commented Jun 7, 2026

Copy link
Copy Markdown

Code Review: feat(TUI): enable various plan rendering formats for stages

Overview

This PR adds multi-format plan rendering to the Ballista TUI's stage plan popup. Users can now switch between Default, Tree, and Metrics plan formats via keyboard shortcuts (d/t/m), with a caching layer to avoid redundant HTTP requests for previously-fetched formats. The architecture is solid overall, but there are several issues worth addressing.


Bugs / Correctness

1. tab used after move in load_stage_plan

In ballista-cli/src/tui/ui/main/jobs/mod.rs, tab is moved by the match expression but then used again:

let fmt = match tab {               // tab is moved here
    StagePlanTab::Default => None,
    StagePlanTab::Tree => Some("tree"),
    StagePlanTab::Metrics => Some("metrics"),
};
// ...
data: UiData::JobStagesPlanData(job_id.to_owned(), tab, stages),  // ERROR: tab already moved

Since StagePlanTab only derives Clone (not Copy), this is a use-after-move. Fix: derive Copy on StagePlanTab (all variants are unit types, so it's free), or match on &tab and clone where needed.

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

This would also eliminate most of the explicit .clone() calls scattered through the caching code.


Incorrect #[allow(dead_code)] Annotations

Several items are annotated dead_code but are actively used:

  • set_tab - called in app.rs for each key binding (popup.set_tab(StagePlanTab::Default), etc.)
  • cached_response - called by set_tab itself
  • UiData::JobStagesPlanData - matched in app.rs and constructed in load_stage_plan

These annotations should be removed. They mask real dead-code warnings and mislead the reader about what is actually live.


Nightly-only Lint Attribute

ballista-cli/src/tui/domain/jobs/stages.rs opens with:

#![allow(unfulfilled_lint_expectations)]

unfulfilled_lint_expectations is a nightly-only lint related to #[expect(...)] attributes. It should not appear in stable Rust code, and there are no corresponding #[expect(...)] attributes in this file to justify it. Remove the attribute.


Code Quality

Empty-string sentinel in get_job_stages

let fmt = plan_format.unwrap_or({
    if self.config.job.stage.plan.tree { "tree" } else { "" }
});
// later:
if fmt.is_empty() { String::new() } else { format!("?plan_format={fmt}") }

Using "" as a sentinel for "no format" is a code smell. The caller already has an Option<&str>; the config fallback should also produce one:

let effective_fmt = plan_format.or_else(|| self.config.job.stage.plan.tree.then_some("tree"));
let query = effective_fmt.map_or_else(String::new, |f| format!("?plan_format={f}"));

Opaque return type from set_tab

set_tab returns Option<StagePlanTab> where None means "cache hit, no fetch needed" and Some(tab) means "cache miss, please fetch". This contract is non-obvious from the type alone. Either add a doc comment, or define a small enum:

enum TabSwitchResult { Cached, NeedsFetch(StagePlanTab) }

Unnecessary clone in cache_plan_response

match fmt {
    StagePlanTab::Default => self.plan_cache.default = Some(resp.clone()),  // cloned
    ...
}
if active_fmt == Some(fmt) {
    self.stages = resp;  // original moved
}

The clone is needed when the tab is active (cache gets the copy, stages gets the original), but it is wasteful when the tab is inactive (the original is dropped). With Copy on StagePlanTab the match becomes reference-based and this can be restructured to avoid the unconditional clone.

Redundant use inside load_stage_plan

The function parameter already names the full path crate::tui::domain::jobs::stages::StagePlanTab, then the body re-imports it with a use statement. The use is redundant.


Test Coverage

No tests are added for:

  • PlanCache hit/miss behavior
  • set_tab returning None (cached) vs Some (uncached)
  • cache_plan_response updating self.stages only when the tab is active

Unit tests for the cache logic in stages.rs would prevent regressions here.


Summary

Severity Issue
Bug tab used after move in load_stage_plan
Correctness Three incorrect #[allow(dead_code)] annotations
Stability #![allow(unfulfilled_lint_expectations)] is nightly-only
Style Empty-string sentinel in get_job_stages
Style Opaque return type from set_tab
Performance Unnecessary clone in cache_plan_response
Coverage No tests for cache logic

The overall design (caching per tab, async fetch on miss, scroll-state preservation) is sound. The keyboard navigation additions and footer hints are clean. The move bug and spurious allow attributes are the most important fixes before merging.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Actionable comments posted: 5

🤖 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 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.

In `@ballista-cli/src/tui/domain/jobs/stages.rs`:
- Around line 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.

In `@ballista-cli/src/tui/http_client.rs`:
- Around line 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.

In `@ballista-cli/src/tui/ui/footer.rs`:
- Around line 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.

In `@ballista-cli/src/tui/ui/main/jobs/mod.rs`:
- Around line 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.
🪄 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: 38db3539-b63c-4643-b470-20e81a6477bc

📥 Commits

Reviewing files that changed from the base of the PR and between 7deca5c and 881a59a.

📒 Files selected for processing (12)
  • .cursor/rules.md
  • AGENTS.md
  • CLAUDE.md
  • ballista-cli/src/tui/app.rs
  • ballista-cli/src/tui/domain/jobs/stages.rs
  • ballista-cli/src/tui/event.rs
  • ballista-cli/src/tui/http_client.rs
  • ballista-cli/src/tui/mod.rs
  • ballista-cli/src/tui/ui/footer.rs
  • ballista-cli/src/tui/ui/main/jobs/mod.rs
  • ballista-cli/src/tui/ui/main/mod.rs
  • ballista-cli/src/tui/ui/mod.rs

Comment on lines +834 to +838
UiData::JobStagesPlanData(_job_id, tab, stages) => {
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.

⚠️ 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.

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

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.

Comment on lines +134 to +140
let fmt = plan_format.unwrap_or({
if self.config.job.stage.plan.tree {
"tree"
} else {
""
}
});

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.

Comment on lines +83 to 89
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

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.

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

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants