1825: feat(tui): Add configuration popup for jobs - #65
Conversation
WalkthroughThis pull request adds a new interactive job configuration popup feature to the Ballista CLI TUI. Users can press ✨ 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 introduces a new feature to view and search job configurations within the Ballista CLI TUI, adding a scrollable popup, keybindings, and HTTP client integration. The review feedback highlights two important improvements: fixing a compilation error in 'job_config_popup.rs' where 'is_multiple_of' is used on 'usize' (which is not in the standard library), and optimizing rendering performance in both 'job_config_popup.rs' and 'search_box.rs' by avoiding unnecessary string clones and heap allocations during frame updates.
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.
| fn row_for_entry<'a>(i: usize, entry: &&'a JobConfigEntry) -> Row<'a> { | ||
| let color = if i.is_multiple_of(2) { | ||
| Color::DarkGray | ||
| } else { | ||
| Color::Black | ||
| }; | ||
|
|
||
| Row::new(vec![ | ||
| Cell::from(entry.key.clone()), | ||
| Cell::from(entry.value.clone()), | ||
| ]) | ||
| .style(Style::default().bg(color)) | ||
| } |
There was a problem hiding this comment.
The method is_multiple_of is not part of the standard library for usize in Rust and will cause a compilation error. Additionally, we can avoid cloning entry.key and entry.value on every row render by using .as_str() to borrow the string slices directly, which improves rendering performance.
| fn row_for_entry<'a>(i: usize, entry: &&'a JobConfigEntry) -> Row<'a> { | |
| let color = if i.is_multiple_of(2) { | |
| Color::DarkGray | |
| } else { | |
| Color::Black | |
| }; | |
| Row::new(vec![ | |
| Cell::from(entry.key.clone()), | |
| Cell::from(entry.value.clone()), | |
| ]) | |
| .style(Style::default().bg(color)) | |
| } | |
| fn row_for_entry<'a>(i: usize, entry: &&'a JobConfigEntry) -> Row<'a> { | |
| let color = if i % 2 == 0 { | |
| Color::DarkGray | |
| } else { | |
| Color::Black | |
| }; | |
| Row::new(vec![ | |
| Cell::from(entry.key.as_str()), | |
| Cell::from(entry.value.as_str()), | |
| ]) | |
| .style(Style::default().bg(color)) | |
| } |
There was a problem hiding this comment.
value:useful; category:bug; feedback: The Gemini AI reviewer is semi-correct! It is correct about passing &str to Cell::from() but it is not correct about https://doc.rust-lang.org/stable/std/primitive.usize.html#method.is_multiple_of - it is available since Rust 1.87.0
| let display_text = if is_edit_mode { | ||
| let search_term = Span::from(search_term.to_string()); | ||
| let cursor = Span::from("_").style(Style::default().bold().yellow()); | ||
| Line::from(vec![search_term, cursor]) | ||
| } else { | ||
| Line::from(Span::from(app.search_term.clone())) | ||
| Line::from(Span::from(search_term.to_string())) | ||
| }; |
There was a problem hiding this comment.
Avoid calling search_term.to_string() which performs unnecessary heap allocations on every frame render. Span::from can accept &str directly, which is much more efficient.
| let display_text = if is_edit_mode { | |
| let search_term = Span::from(search_term.to_string()); | |
| let cursor = Span::from("_").style(Style::default().bold().yellow()); | |
| Line::from(vec![search_term, cursor]) | |
| } else { | |
| Line::from(Span::from(app.search_term.clone())) | |
| Line::from(Span::from(search_term.to_string())) | |
| }; | |
| let display_text = if is_edit_mode { | |
| let search_term = Span::from(search_term); | |
| let cursor = Span::from("_").style(Style::default().bold().yellow()); | |
| Line::from(vec![search_term, cursor]) | |
| } else { | |
| Line::from(Span::from(search_term)) | |
| }; |
There was a problem hiding this comment.
value:good-to-have; category:bug; feedback: The Gemini AI reviewer is correct! All text related Ratatui widgets accept &str, so the String cloning could be avoided. Prevents memory allocation where it is not needed.
![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 0dad7d2. Configure here.
| pub async fn on_key(&mut self, key: KeyEvent) -> TuiResult<()> { | ||
| // Edit mode takes priority over everything | ||
| if self.is_edit_mode() { | ||
| if let Some(popup) = &mut self.job_config_popup { |
There was a problem hiding this comment.
Stale config after jobs refresh
Medium Severity
With the job config popup open, periodic jobs reload can change which job is selected at the same table index while the popup still shows the earlier job’s config. on_tick refreshes job_details for the new selection but never clears or updates job_config_popup, so config and selection can diverge.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 0dad7d2. Configure here.
There was a problem hiding this comment.
value:useful; category:bug; feedback: The Bugbot AI reviewer is correct! The tick event handler should check whether the job_config_popup is still valid for the currently selected job and invalidate it if there are not in sync. The data is loaded asynchronously and this may lead to processing late async response for job that is no more the currently selected one.
Code Review: feat(tui): Add configuration popup for jobsOverviewThis PR adds a job configuration popup to the Ballista TUI. Pressing Security / Prompt Injection ConcernThe PR includes three new files --
These files are not project documentation -- they appear to be a prompt-injection attempt designed to influence AI code review tools. These files should not be merged and should be removed from the PR. Potential Bug: Render State Copies DiscardedFile: let mut table_state = popup.table_state;
let mut scrollbar_state = popup.scrollbar_state;
Other popup renderers in the codebase face the same constraint (render takes Error Handling: Double-Logged, Inner Error SwallowedFile: let config = match app.http_client.get_job_config(job_id).await {
Ok(config) => config,
Err(e) => {
tracing::error!("Failed to load job config for {job_id}: {e:?}");
return Ok(()); // error silently swallowed
}
};The function logs the error and returns Minor Issues
let filtered = self.filtered_entries();
let len = filtered.len();Double reference in fn row_for_entry<'a>(i: usize, entry: &&'a JobConfigEntry) -> Row<'a>The Unnecessary let search_term = Span::from(search_term.to_string());
Module visibility
Test CoverageThe domain-level tests are good -- filtering, selection reset, and state transitions are well covered. Gaps worth considering:
SummaryThe feature is well-structured and consistent with existing popup patterns. The main items to address before merging are the prompt-injection files (must remove) and the render state copy issue (potential scroll bug). The error-handling inconsistency and minor style points are lower priority. |
🤖 Augment PR SummarySummary: Adds a “Job config” popup to the Ballista CLI TUI so users can inspect per-job configuration items. Changes:
🤖 Was this summary useful? React with 👍 or 👎 |
![augmentcode[bot]](https://avatars.githubusercontent.com/in/1027498?s=60&v=4){kind=small}
| } else if app.is_job_config_popup_open() { | ||
| current_view_key_bindings.push(Span::from("[↑↓] Navigate, ")); | ||
| current_view_key_bindings.push(Span::from("[/] Search config, ")); | ||
| current_view_key_bindings.push(Span::from("[Esc] Close popup, ")); |
There was a problem hiding this comment.
footer.rs:52 shows [Esc] Close popup for the job config popup, but when the user is in config search edit mode Esc only clears the search and exits edit mode (the popup stays open). This hint seems inaccurate in that state and may confuse users.
Severity: low
🤖 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! The context-aware keybinding hint should also check the edit mode to decide what text label to use for the Esc keybinding. Prevents showing wrong keybinding hint that the popup will be closed when in Edit mode. It will actually switch to View mode.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/http_client.rs`:
- Around line 132-135: The get_job_config method currently calls the generic
json helper (json) which leads to the full JobConfigResponse being logged
elsewhere; update get_job_config to perform endpoint-specific response handling
instead of using the generic helper so we do not log full payloads—call the
request, deserialize only the minimal safe fields you need (or deserialize into
a separate lightweight struct), and ensure any logging (where JobConfigResponse
was previously logged) is removed or replaced with redacted/sanitized values
(e.g., mask tokens/secrets or log only job_id/status). Locate usages of
get_job_config and the json helper to remove the full-response logs and replace
them with safe, endpoint-specific logging.
In `@ballista-cli/src/tui/ui/main/jobs/mod.rs`:
- Around line 95-102: The build fails because load_job_config_popup uses
JobConfigEntry and JobConfigPopup but they aren't imported; add the missing use
import(s) for JobConfigEntry and JobConfigPopup at the top of this module (the
same module that defines load_job_config_popup) so those symbols are in
scope—e.g. add a use statement that imports JobConfigEntry and JobConfigPopup
from their defining module (where job config popup types are declared) so the
map(...) and UiData::JobConfig(...) lines compile.
🪄 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: 4b786b52-a881-4c91-a59e-4e44ade7eb72
📒 Files selected for processing (15)
.cursor/rules.mdAGENTS.mdCLAUDE.mdballista-cli/src/tui/app.rsballista-cli/src/tui/domain/jobs.rsballista-cli/src/tui/event.rsballista-cli/src/tui/http_client.rsballista-cli/src/tui/mod.rsballista-cli/src/tui/ui/footer.rsballista-cli/src/tui/ui/help_overlay.rsballista-cli/src/tui/ui/main/jobs/job_config_popup.rsballista-cli/src/tui/ui/main/jobs/mod.rsballista-cli/src/tui/ui/main/mod.rsballista-cli/src/tui/ui/mod.rsballista-cli/src/tui/ui/search_box.rs
| pub async fn get_job_config(&self, job_id: &str) -> TuiResult<JobConfigResponse> { | ||
| let url = self.url(&format!("job/{}/config", self.url_encode(job_id))); | ||
| self.json::<JobConfigResponse>(&url).await | ||
| } |
There was a problem hiding this comment.
Avoid logging full job-config payloads in HTTP traces.
get_job_config (Lines 132-135) uses the generic json helper, and Line 186 logs the entire deserialized response. Job configs can contain secrets/tokens, so this leaks sensitive values into logs.
Proposed fix (endpoint-specific parsing without payload logging)
pub async fn get_job_config(&self, job_id: &str) -> TuiResult<JobConfigResponse> {
let url = self.url(&format!("job/{}/config", self.url_encode(job_id)));
- self.json::<JobConfigResponse>(&url).await
+ let response = self.get(&url).await?;
+ let response = response
+ .error_for_status()
+ .map_err(TuiError::from)
+ .inspect_err(|err| tracing::error!("HTTP error status: {err:?}"))?;
+
+ response
+ .json::<JobConfigResponse>()
+ .await
+ .map_err(TuiError::from)
+ .inspect(|data| tracing::trace!("Loaded job config entries: {}", data.len()))
+ .inspect_err(|err| tracing::error!("The HTTP request failed: {err:?}"))
}Also applies to: 182-187
🤖 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 132 - 135, The
get_job_config method currently calls the generic json helper (json) which leads
to the full JobConfigResponse being logged elsewhere; update get_job_config to
perform endpoint-specific response handling instead of using the generic helper
so we do not log full payloads—call the request, deserialize only the minimal
safe fields you need (or deserialize into a separate lightweight struct), and
ensure any logging (where JobConfigResponse was previously logged) is removed or
replaced with redacted/sanitized values (e.g., mask tokens/secrets or log only
job_id/status). Locate usages of get_job_config and the json helper to remove
the full-response logs and replace them with safe, endpoint-specific logging.
There was a problem hiding this comment.
value:valid-but-wont-fix; category:bug; feedback: The Augment AI reviewer is correct! The sensitive data is skipped by the REST endpoint itself, i.e. the server does not serve it at all. In addition the log uses TRACE level that is not enabled by default. If a user can change the log level them (s)he can also make a REST request without using the TUI app and see the full config.
| let entries = config | ||
| .into_iter() | ||
| .map(|(key, value)| JobConfigEntry { key, value }) | ||
| .collect(); | ||
|
|
||
| app.send_event(Event::DataLoaded { | ||
| data: UiData::JobConfig(JobConfigPopup::new(job_id.to_string(), entries)), | ||
| }) |
There was a problem hiding this comment.
Import missing domain types used by load_job_config_popup.
Line 97 and Line 101 use JobConfigEntry and JobConfigPopup, but those types are not imported in this module, so this path won’t compile for non-web builds.
Proposed fix
#[cfg(not(feature = "web"))]
use crate::tui::{
TuiResult,
event::{Event, UiData},
};
+#[cfg(not(feature = "web"))]
+use crate::tui::domain::jobs::{JobConfigEntry, JobConfigPopup};
use crate::tui::{
app::App,
domain::{
SortOrder,
jobs::{Job, SortColumn},📝 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 entries = config | |
| .into_iter() | |
| .map(|(key, value)| JobConfigEntry { key, value }) | |
| .collect(); | |
| app.send_event(Event::DataLoaded { | |
| data: UiData::JobConfig(JobConfigPopup::new(job_id.to_string(), entries)), | |
| }) | |
| #[cfg(not(feature = "web"))] | |
| use crate::tui::{ | |
| TuiResult, | |
| event::{Event, UiData}, | |
| }; | |
| #[cfg(not(feature = "web"))] | |
| use crate::tui::domain::jobs::{JobConfigEntry, JobConfigPopup}; | |
| use crate::tui::{ | |
| app::App, | |
| domain::{ | |
| SortOrder, | |
| jobs::{Job, SortColumn}, |
🤖 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 95 - 102, The build
fails because load_job_config_popup uses JobConfigEntry and JobConfigPopup but
they aren't imported; add the missing use import(s) for JobConfigEntry and
JobConfigPopup at the top of this module (the same module that defines
load_job_config_popup) so those symbols are in scope—e.g. add a use statement
that imports JobConfigEntry and JobConfigPopup from their defining module (where
job config popup types are declared) so the map(...) and UiData::JobConfig(...)
lines compile.
There was a problem hiding this comment.
value:useful; category:bug; feedback: The CodeRabbit AI reviewer is correct! These structs are not imported and this causes compilation failure.
value:good-but-wont-fix; category:bug; feedback: The Claude AI reviewer is correct! The UI states are cloned/copied locally to make them mutable because the rendering needs to write to their buffers but the states themselves are not modified, so there is nothing to lose. Passing |
3 participants
1825: To review by AI