1828: feat(TUI): enable various plan rendering formats - #68
1828: feat(TUI): enable various plan rendering formats#68martin-augment wants to merge 7 commits into
Conversation
…h metrics specified)
…WASM support. UI fix
WalkthroughThis PR implements multi-format plan selection for the Ballista TUI job details interface. 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 |
![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 enhances the Ballista CLI TUI by adding support for dynamic switching and rendering of different physical plan formats (such as tree format) and stage plans (default, tree, and metrics). It removes the static configuration setting in favor of interactive key bindings. The review feedback suggests two key improvements: mapping the default stage plan tab to None instead of Some("") to avoid sending empty query parameters (?plan_format=) to the server, and caching the fetched tree physical plan in self.job_details to prevent redundant network requests when the plan popup is closed and reopened.
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.
| let fmt = match tab { | ||
| StagePlanTab::Default => Some(""), | ||
| StagePlanTab::Tree => Some("tree"), | ||
| StagePlanTab::Metrics => Some("metrics"), | ||
| }; |
There was a problem hiding this comment.
Mapping StagePlanTab::Default to Some("") causes the HTTP client to append an empty query parameter ?plan_format= to the request URL. Mapping it to None instead will cleanly omit the query parameter, matching the default behavior of the endpoint.
| 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"), | |
| }; |
There was a problem hiding this comment.
value:useful; category:bug; feedback: The Gemini AI reviewer is correct! Instead of passing an Option<&str> it would be better to pass &StagePlanTab and add implementations to it to represent itself as a query parameter and as String, i.e. a Display impl. This way the APIs will be typed and adding a new enum variant will be done in one place - StagePlanTab and its impls, not all over the codebase.
| let fmt = match tab { | ||
| StagePlanTab::Default => Some(""), | ||
| StagePlanTab::Tree => Some("tree"), | ||
| StagePlanTab::Metrics => Some("metrics"), | ||
| }; |
There was a problem hiding this comment.
Mapping StagePlanTab::Default to Some("") causes the HTTP client to append an empty query parameter ?plan_format= to the request URL. Mapping it to None instead will cleanly omit the query parameter, matching the default behavior of the endpoint.
let fmt = match tab {
StagePlanTab::Default => None,
StagePlanTab::Tree => Some("tree"),
StagePlanTab::Metrics => Some("metrics"),
};There was a problem hiding this comment.
value:useful; category:bug; feedback: The Gemini AI reviewer is correct! Instead of passing an Option<&str> it would be better to pass &StagePlanTab and add implementations to it to represent itself as a query parameter and as String, i.e. a Display impl. This way the APIs will be typed and adding a new enum variant will be done in one place - StagePlanTab and its impls, not all over the codebase.
| 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); | ||
| } |
There was a problem hiding this comment.
When the tree physical plan is loaded, it is only updated in self.job_plan_popup. If the user closes and reopens the plan popup, the tree plan will have to be fetched again. Caching it in self.job_details as well avoids redundant network requests.
| 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); | |
| } | |
| 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.clone(); | |
| } | |
| if let Some(job_details) = &mut self.job_details { | |
| if job_details.job_id == details.job_id { | |
| job_details.physical_plan_tree = details.physical_plan_tree; | |
| } | |
| } | |
| } else { | |
| self.job_details = Some(details); | |
| } |
| Ok(details) => { | ||
| if let Some(p) = &mut self.job_plan_popup { | ||
| p.details.physical_plan_tree = details.physical_plan; | ||
| } | ||
| } |
There was a problem hiding this comment.
When the tree physical plan is fetched, it is only updated in self.job_plan_popup. Caching it in self.job_details as well ensures that if the user closes and reopens the plan popup, the tree plan does not need to be fetched again.
Ok(details) => {
if let Some(p) = &mut self.job_plan_popup {
p.details.physical_plan_tree = details.physical_plan.clone();
}
if let Some(job_details) = &mut self.job_details {
if job_details.job_id == job_id {
job_details.physical_plan_tree = details.physical_plan;
}
}
}
Code ReviewOverviewThis PR adds dynamic plan rendering format support to the TUI: users can now switch between Default, Tree, and Metrics formats for stage plans (via Bugs / Correctness1. In both StagePlanTab::Default => Some(""),This produces 2.
details.physical_plan_tree = details.physical_plan.take();The field rename happens outside the struct's natural construction site. If Code Quality3. Broad lint suppressions
4. Dead-code markers on items that are actually used
5. Duplicated key-handling block in The 6. pub fn set_physical_format(&mut self, fmt: PhysicalFormat) -> Option<()> { ... }
// before
.map(|_| plans_popup.details.job_id.clone())
// after (with bool)
if plans_popup.set_physical_format(PhysicalFormat::Tree) { Some(plans_popup.details.job_id.clone()) } else { None }Either is fine, but the current return type warrants the doc comment it already has — consider whether Performance / Design7. Each format caches a complete 8. Inline } else if popup.is_plan_view() {
use crate::tui::domain::jobs::stages::StagePlanTab;This Test CoverageNo new unit tests are added for:
The existing test helper in Minor / Nits
Positives
|
![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 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 5c57311. Configure here.
| None | ||
| } else { | ||
| Some(tab) | ||
| } |
There was a problem hiding this comment.
Stale stage plan after tab
Medium Severity
When switching the stage plan tab to tree or metrics without a cached response, set_tab updates the active format but leaves stages unchanged until the HTTP call finishes. The plan popup keeps rendering the previous format’s stage_plan text, and a failed fetch leaves that mismatch with no loading or error state.
Reviewed by Cursor Bugbot for commit 5c57311. Configure here.
🤖 Augment PR SummarySummary: This PR extends the Ballista CLI TUI to support multiple plan rendering formats and to fetch/cycle them interactively. Changes:
Technical Notes: Tree/metrics variants are loaded on-demand (only when selected) and then cached in the popup state to reduce repeated HTTP calls. 🤖 Was this summary useful? React with 👍 or 👎 |
![augmentcode[bot]](https://avatars.githubusercontent.com/in/1027498?s=60&v=4){kind=small}
| WebKeyAsyncAction::LoadStagePlan(job_id, tab) => { | ||
| use crate::tui::domain::jobs::stages::StagePlanTab; | ||
| let fmt = match tab { | ||
| StagePlanTab::Default => Some(""), |
There was a problem hiding this comment.
ballista-cli/src/tui/mod.rs:361: StagePlanTab::Default => Some("") will generate URLs like ...?plan_format= rather than omitting the parameter, which can behave differently on the server (e.g., treated as invalid/unknown format). Consider mapping the default case to None so the default request doesn’t send an empty query value.
Other locations where this applies: ballista-cli/src/tui/ui/main/jobs/mod.rs:116
Severity: medium
Other Locations
ballista-cli/src/tui/ui/main/jobs/mod.rs:116
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
There was a problem hiding this comment.
value:useful; category:bug; feedback: The Augment AI reviewer is correct! Instead of passing an Option<&str> it would be better to pass &StagePlanTab and add implementations to it to represent itself as a query parameter and as String, i.e. a Display impl. This way the APIs will be typed and adding a new enum variant will be done in one place - StagePlanTab and its impls, not all over the codebase.
| current_view_key_bindings.push(Span::from("[↑↓] Scroll up/down, ")); | ||
| current_view_key_bindings.push(Span::from("[↑↓←→] Scroll, ")); | ||
| current_view_key_bindings.push(Span::from("[s] Stage plan, ")); | ||
| current_view_key_bindings.push(Span::from("[p] Physical plan, ")); |
There was a problem hiding this comment.
ballista-cli/src/tui/ui/footer.rs:46: The job plan popup footer lists [s]/[p]/[l] but doesn’t mention the new physical plan format toggles (t/d), so users may miss that the format can be switched from the popup.
Severity: low
🤖 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: 3
🧹 Nitpick comments (3)
ballista-cli/src/tui/app.rs (1)
857-864: ⚖️ Poor tradeoffConsider explicit event variants for job details routing.
The current logic routes
UiData::JobDetailsbased on whetherphysical_plan_treeis present, using field presence as an implicit signal. While functional, this coupling makes the routing logic less explicit. Consider using separateUiDatavariants (e.g.,JobDetailsTreevsJobDetailsDefault) to make the intent clearer and reduce future maintenance burden.🤖 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 857 - 864, The code currently multiplexes UiData::JobDetails based on the presence of details.physical_plan_tree, which couples routing to field content; change to explicit variants (e.g., UiData::JobDetailsTree and UiData::JobDetailsDefault) and update the match arm currently handling UiData::JobDetails to handle the new variants: have UiData::JobDetailsTree carry the details intended for job_plan_popup and assign popup.details.physical_plan_tree = details.physical_plan_tree in that arm (using self.job_plan_popup), and have UiData::JobDetailsDefault set self.job_details = Some(details); also update any producers/emitters that create UiData::JobDetails to emit the correct new variant so consumers like the match in app.rs no longer inspect physical_plan_tree for routing.ballista-cli/src/tui/http_client.rs (1)
118-125: 💤 Low valueConsider using proper URL query parameter construction.
The current implementation concatenates query parameters using string formatting. While functional, this approach is less robust than using a URL builder library method (e.g.,
Url::parsewithquery_pairs_mut). This would also handle URL encoding of parameter values automatically if needed in the future.🤖 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 118 - 125, The code builds the job URL by string-concatenating the query string; instead use a URL builder so query parameters are added safely. Replace the manual formatting in the call to self.url(...) for job/{...} with constructing a Url (e.g., Url::parse or Url::options().base_url(...)) for the base path with the encoded job_id (keep using self.url_encode(job_id) if you need the path encoded), then call url.query_pairs_mut().append_pair("plan_format", fmt) when plan_format is Some(fmt) and pass the resulting url.to_string() into self.url; this will avoid manual string concatenation and ensure proper encoding of query values.ballista-cli/src/tui/domain/jobs/stages.rs (1)
17-17: 💤 Low valueClarify the purpose of file-level lint suppression.
The
#![allow(unfulfilled_lint_expectations)]attribute suppresses warnings about lint expectations that were not triggered, but the rationale for adding this is unclear. If specific lints were expected but not fulfilled, it would be better to address the underlying issue or document why the suppression is necessary.🤖 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` at line 17, The file contains a blanket crate-level attribute `#![allow(unfulfilled_lint_expectations)]` with no explanation; remove this broad suppression and either delete it entirely or replace it with targeted lint allowances for specific cases (or keep it but add a short comment explaining exactly which expected_lint annotations were intentionally left unfulfilled and why). Locate the attribute at the top of the file (the `#![allow(unfulfilled_lint_expectations)]` line) and either remove it, narrow it to specific lint names used in this module, or add a one-line comment above it documenting the rationale and linking to the related lints or TODO for resolving the underlying issues.
🤖 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/domain/jobs/stages.rs`:
- Around line 188-200: The #[allow(dead_code)] on the method set_tab should be
removed (or replaced with a justification) because set_tab is actually used;
edit the StagePlanTab::set_tab implementation to delete the attribute and run a
build/test to confirm no dead-code warning remains, or if this method is only
used under a feature flag, add the appropriate #[cfg(...)] gate or a comment
explaining the conditional usage and use #[allow(dead_code)] only within that
cfg block to justify it; reference the set_tab function and
StageDetailsView::Plan/ cached_response symbols when making the change.
In `@ballista-cli/src/tui/mod.rs`:
- Around line 358-376: In WebKeyAsyncAction::LoadStagePlan the
StagePlanTab::Default arm sets fmt to Some(""), which yields a request query
plan_format= and fails server-side deserialization; change the mapping so
Default yields None (or "default") instead of Some(""), i.e. update the match
over StagePlanTab in LoadStagePlan so fmt is None for Default before calling
get_job_stages(&job_id, fmt), keeping other arms (Tree/Metrics) as before.
In `@ballista-cli/src/tui/ui/main/jobs/mod.rs`:
- Around line 115-119: The mapping for the format parameter is inconsistent:
StagePlanTab::Default is set to Some("") while load_job_stages_popup expects
None for the no-format case; change the fmt assignment so StagePlanTab::Default
yields None (not Some("")) to match load_job_stages_popup's behavior and ensure
the HTTP client/backend receives a missing parameter rather than an empty string
(update the match that sets fmt where StagePlanTab is handled).
---
Nitpick comments:
In `@ballista-cli/src/tui/app.rs`:
- Around line 857-864: The code currently multiplexes UiData::JobDetails based
on the presence of details.physical_plan_tree, which couples routing to field
content; change to explicit variants (e.g., UiData::JobDetailsTree and
UiData::JobDetailsDefault) and update the match arm currently handling
UiData::JobDetails to handle the new variants: have UiData::JobDetailsTree carry
the details intended for job_plan_popup and assign
popup.details.physical_plan_tree = details.physical_plan_tree in that arm (using
self.job_plan_popup), and have UiData::JobDetailsDefault set self.job_details =
Some(details); also update any producers/emitters that create UiData::JobDetails
to emit the correct new variant so consumers like the match in app.rs no longer
inspect physical_plan_tree for routing.
In `@ballista-cli/src/tui/domain/jobs/stages.rs`:
- Line 17: The file contains a blanket crate-level attribute
`#![allow(unfulfilled_lint_expectations)]` with no explanation; remove this
broad suppression and either delete it entirely or replace it with targeted lint
allowances for specific cases (or keep it but add a short comment explaining
exactly which expected_lint annotations were intentionally left unfulfilled and
why). Locate the attribute at the top of the file (the
`#![allow(unfulfilled_lint_expectations)]` line) and either remove it, narrow it
to specific lint names used in this module, or add a one-line comment above it
documenting the rationale and linking to the related lints or TODO for resolving
the underlying issues.
In `@ballista-cli/src/tui/http_client.rs`:
- Around line 118-125: The code builds the job URL by string-concatenating the
query string; instead use a URL builder so query parameters are added safely.
Replace the manual formatting in the call to self.url(...) for job/{...} with
constructing a Url (e.g., Url::parse or Url::options().base_url(...)) for the
base path with the encoded job_id (keep using self.url_encode(job_id) if you
need the path encoded), then call
url.query_pairs_mut().append_pair("plan_format", fmt) when plan_format is
Some(fmt) and pass the resulting url.to_string() into self.url; this will avoid
manual string concatenation and ensure proper encoding of query values.
🪄 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: ba785880-9ee0-4bdb-987f-6cd1b070e247
📒 Files selected for processing (16)
.cursor/rules.md.gemini/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
| #[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) | ||
| } | ||
| } |
There was a problem hiding this comment.
Remove or justify the #[allow(dead_code)] attribute.
The set_tab method is marked with #[allow(dead_code)], but it appears to be called in app.rs (around line 232 with popup.set_tab(StagePlanTab::Default)). If the method is truly unused in certain build configurations, consider adding a feature-gate comment to clarify; otherwise, remove the attribute.
🤖 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 188 - 200, The
#[allow(dead_code)] on the method set_tab should be removed (or replaced with a
justification) because set_tab is actually used; edit the StagePlanTab::set_tab
implementation to delete the attribute and run a build/test to confirm no
dead-code warning remains, or if this method is only used under a feature flag,
add the appropriate #[cfg(...)] gate or a comment explaining the conditional
usage and use #[allow(dead_code)] only within that cfg block to justify it;
reference the set_tab function and StageDetailsView::Plan/ cached_response
symbols when making the change.
| WebKeyAsyncAction::LoadStagePlan(job_id, tab) => { | ||
| use crate::tui::domain::jobs::stages::StagePlanTab; | ||
| let fmt = match tab { | ||
| StagePlanTab::Default => Some(""), | ||
| StagePlanTab::Tree => Some("tree"), | ||
| StagePlanTab::Metrics => Some("metrics"), | ||
| }; | ||
| match http_client.get_job_stages(&job_id, fmt).await { | ||
| Ok(mut stages) => { | ||
| stages | ||
| .stages | ||
| .sort_by_key(|s| s.id.parse::<u64>().unwrap_or(u64::MAX)); | ||
| send_data(UiData::JobStagesPlanData(tab, stages), tx).await; | ||
| } | ||
| Err(e) => { | ||
| tracing::error!("Failed to load stage plan for '{job_id}': {e:?}") | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Search for server-side handling of plan_format parameter to verify empty string behavior
# Search for plan_format parameter handling in server code
rg -n --type rust "plan_format" -A 5 -B 5 -g '!ballista-cli/**'Repository: martin-augment/datafusion-ballista
Length of output: 5955
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# 1) Find the client implementation of get_job_stages
rg -n --type rust "fn get_job_stages|get_job_stages\\(" ballista-cli/src -S
# 2) Find the server endpoint/handler for "job stages plan" and its query params
rg -n --type rust "job.*stages|stages.*plan|get_job_stages|JobStages" ballista/scheduler/src/api -S
# 3) Search for the exact query parameter name used by that endpoint
rg -n --type rust "plan_format|format.*plan|stage.*plan.*format" ballista/scheduler/src/api -SRepository: martin-augment/datafusion-ballista
Length of output: 3322
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show client request construction for get_job_stages
sed -n '120,190p' ballista-cli/src/tui/http_client.rs
# Show server route + handler around the Job stages endpoint
sed -n '1,120p' ballista/scheduler/src/api/routes.rs
sed -n '300,470p' ballista/scheduler/src/api/handlers.rsRepository: martin-augment/datafusion-ballista
Length of output: 11935
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Client: how get_job_stages turns fmt into query params (especially when fmt == Some(""))
sed -n '120,190p' ballista-cli/src/tui/http_client.rs
# Server: handler for "/api/job/{job_id}/stages" (how query.plan_format is deserialized/handled)
sed -n '1,120p' ballista/scheduler/src/api/routes.rs
sed -n '300,470p' ballista/scheduler/src/api/handlers.rsRepository: martin-augment/datafusion-ballista
Length of output: 11935
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the handler for /api/job/{job_id}/stages and inspect surrounding code
rg -n --type rust "get_query_stages|get_job_stages|JobStages|stages.*plan_format|plan_format.*stages" ballista/scheduler/src/api/handlers.rs -S
# Print the relevant sections around get_query_stages and PlanFormat/JobQueryParams definitions
rg -n --type rust "pub async fn get_query_stages" ballista/scheduler/src/api/handlers.rs -n
sed -n '150,260p' ballista/scheduler/src/api/handlers.rs
sed -n '260,420p' ballista/scheduler/src/api/handlers.rs
sed -n '420,650p' ballista/scheduler/src/api/handlers.rsRepository: martin-augment/datafusion-ballista
Length of output: 19635
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '60,150p' ballista-cli/src/tui/ui/main/jobs/mod.rs
sed -n '250,430p' ballista-cli/src/tui/mod.rsRepository: martin-augment/datafusion-ballista
Length of output: 8593
Fix default plan_format handling ("" -> invalid query value)
In LoadStagePlan, StagePlanTab::Default maps to Some(""), which get_job_stages turns into the request ...?plan_format=. The scheduler’s /api/job/{job_id}/stages handler deserializes plan_format as Option<PlanFormat> where valid values are default/tree/metrics; an empty string won’t match any variant, so the request fails instead of falling back via unwrap_or_default().
Change default to pass None (matching other call sites) or use "default" explicitly (e.g., StagePlanTab::Default => None).
🤖 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/mod.rs` around lines 358 - 376, In
WebKeyAsyncAction::LoadStagePlan the StagePlanTab::Default arm sets fmt to
Some(""), which yields a request query plan_format= and fails server-side
deserialization; change the mapping so Default yields None (or "default")
instead of Some(""), i.e. update the match over StagePlanTab in LoadStagePlan so
fmt is None for Default before calling get_job_stages(&job_id, fmt), keeping
other arms (Tree/Metrics) as before.
There was a problem hiding this comment.
value:useful; category:bug; feedback: The CodeRabbit AI reviewer is correct! Instead of passing an Option<&str> it would be better to pass &StagePlanTab and add implementations to it to represent itself as a query parameter and as String, i.e. a Display impl. This way the APIs will be typed and adding a new enum variant will be done in one place - StagePlanTab and its impls, not all over the codebase.
| let fmt = match tab { | ||
| StagePlanTab::Default => Some(""), | ||
| StagePlanTab::Tree => Some("tree"), | ||
| StagePlanTab::Metrics => Some("metrics"), | ||
| }; |
There was a problem hiding this comment.
Inconsistent format mapping for default case.
StagePlanTab::Default maps to Some(""), but load_job_stages_popup (line 89) passes None for the default/no-format case. If the HTTP client or backend treats an empty string differently from a missing parameter, this could produce inconsistent results.
🔧 Proposed fix for consistency
let fmt = match tab {
- StagePlanTab::Default => Some(""),
+ StagePlanTab::Default => None,
StagePlanTab::Tree => Some("tree"),
StagePlanTab::Metrics => Some("metrics"),
};📝 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.
| 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"), | |
| }; |
🤖 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 mapping
for the format parameter is inconsistent: StagePlanTab::Default is set to
Some("") while load_job_stages_popup expects None for the no-format case; change
the fmt assignment so StagePlanTab::Default yields None (not Some("")) to match
load_job_stages_popup's behavior and ensure the HTTP client/backend receives a
missing parameter rather than an empty string (update the match that sets fmt
where StagePlanTab is handled).
There was a problem hiding this comment.
value:useful; category:bug; feedback: The CodeRabbit AI reviewer is correct! Instead of passing an Option<&str> it would be better to pass &StagePlanTab and add implementations to it to represent itself as a query parameter and as String, i.e. a Display impl. This way the APIs will be typed and adding a new enum variant will be done in one place - StagePlanTab and its impls, not all over the codebase.
value:useful; category:bug; feedback: The Claude AI reviewer is correct! Instead of passing an Option<&str> it would be better to pass &StagePlanTab and add implementations to it to represent itself as a query parameter and as String, i.e. a Display impl. This way the APIs will be typed and adding a new enum variant will be done in one place - StagePlanTab and its impls, not all over the codebase. |
3 participants
1828: To review by AI