1670: [TUI] Show executor's details in a popup#27
1670: [TUI] Show executor's details in a popup#27martin-augment wants to merge 3 commits intomainfrom
Conversation
![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 an executor details popup to the Ballista CLI TUI, allowing users to view detailed information about a selected executor, including OS info and metrics. Key changes include new domain models for executor details, API client support for fetching these details, and the UI implementation for the popup with scrollable content and updated key bindings. Feedback focuses on optimizing performance by reducing unnecessary string allocations and clones, and addressing a missing upper bound check in the popup's scroll logic.
| let executor_id = executor.id.clone(); | ||
| if let Err(e) = load_executor_details_popup(self, &executor_id).await { |
There was a problem hiding this comment.
The executor_id is cloned unnecessarily here. Since load_executor_details_popup accepts a &str, you can pass a reference to the ID instead to avoid an allocation.
| let executor_id = executor.id.clone(); | |
| if let Err(e) = load_executor_details_popup(self, &executor_id).await { | |
| let executor_id = &executor.id; | |
| if let Err(e) = load_executor_details_popup(self, executor_id).await { |
There was a problem hiding this comment.
value:good-to-have; category:bug; feedback: The Gemini AI reviewer is correct! There is no need to clone the String. Using a reference of it is good enough too. Prevents wasting some stack memory.
| pub fn scroll_down(&mut self) { | ||
| self.scroll_position = self.scroll_position.saturating_add(1); | ||
| } |
There was a problem hiding this comment.
The scroll_down method increments the scroll position without an upper bound. This can lead to the UI rendering an empty popup if the user scrolls past the actual number of lines generated in the view. Consider passing the maximum number of lines or handling the bounds check in the UI layer.
There was a problem hiding this comment.
value:good-to-have; category:bug; feedback: The Gemini AI reviewer is correct! The scroll_position should not grow more than ~20 lines, otherwise it will scroll down too much and no info will be shown on the popup window. Prevents rendering useless popup content.
| ]), | ||
| Line::from(vec
WalkthroughThis PR introduces two independent changes. First, it adds configuration rules to four agent files ( ✨ 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 |
🤖 Augment PR SummarySummary: This PR enhances the Ballista CLI TUI executors view by adding an “executor details” popup that fetches and displays extended executor information. Changes:
Technical Notes: The popup renders metrics and OS information and uses DataFusion’s human-readable formatters for durations/sizes. 🤖 Was this summary useful? React with 👍 or 👎 |
![augmentcode[bot]](https://avatars.githubusercontent.com/in/1027498?s=60&v=4){kind=small}
|
|
||
| #[derive(Deserialize, Clone, Debug)] | ||
| pub struct Specification { | ||
| pub task_slots: u16, |
There was a problem hiding this comment.
ballista-cli/src/tui/domain/executors.rs:47: The scheduler REST API uses wider/optional types for executor fields (e.g., task_slots/physical_cores/num_disks are u32, open_files_limit is u64, and last_seen can be null when no heartbeat exists), but the CLI structs use narrower non-optional integers (u16/u32/i64). This can lead to JSON deserialization failures (or truncation) when values exceed these ranges or when last_seen is missing.
Severity: medium
Other Locations
ballista-cli/src/tui/domain/executors.rs:54ballista-cli/src/tui/domain/executors.rs:55ballista-cli/src/tui/domain/executors.rs:58
🤖 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 types of the data transferred with REST should match to avoid mismatches and data loss after transfer from the scheduler to the TUI application.
| format!(" {:<20} ", metric_name(&metric.typ)), | ||
| label_style, | ||
| ), | ||
| Span::raw(app.format_size(metric.value as usize)), |
There was a problem hiding this comment.
ballista-cli/src/tui/ui/main/executors/executor_details_popup.rs:87: Casting u64 byte counts (metric.value, total_disk_space, total_available_disk_space) to usize can truncate on 32-bit targets and for extremely large values, leading to incorrect sizes being displayed. This is especially relevant since these numbers are naturally u64 in the scheduler API.
Severity: low
Other Locations
ballista-cli/src/tui/ui/main/executors/executor_details_popup.rs:121ballista-cli/src/tui/ui/main/executors/executor_details_popup.rs:125
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
There was a problem hiding this comment.
value:good-but-wont-fix; category:bug; feedback: The Augment AI reviewer is correct! The data coming from the Scheduler REST API uses u64 type but the helper methods coming from DataFusion use usize and this leads to the castings. The TUI application is not expected to be used on a 32-bit system, since DataFusion itself does not support it, so there is no need to worry here.
![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 0c506cd. Configure here.
| format!(" {:<20} ", metric_name(&metric.typ)), | ||
| label_style, | ||
| ), | ||
| Span::raw(app.format_size(metric.value as usize)), |
There was a problem hiding this comment.
Lossy u64 as usize cast truncates large values
Low Severity
metric.value, os.total_disk_space, and os.total_available_disk_space are u64 but are cast to usize via as usize before passing to format_size. On 32-bit targets, usize is 32 bits, so any value above ~4 GB is silently truncated, producing incorrect display output. Disk space values routinely exceed 4 GB, making this a realistic truncation scenario on those platforms.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 0c506cd. Configure here.
There was a problem hiding this comment.
value:good-but-wont-fix; category:bug; feedback: The Bugbot AI reviewer is correct! The data coming from the Scheduler REST API uses u64 type but the helper methods coming from DataFusion use usize and this leads to the castings. The TUI application is not expected to be used on a 32-bit system, since DataFusion itself does not support it, so there is no need to worry here.
Code Review: TUI – Show Executor Details in a PopupOverviewThis PR adds an executor-details popup to the Ballista TUI. Pressing Enter on a selected executor fires an HTTP request, streams the response through the existing event channel, and renders an ExecutorDetailsPopup with OS info, metrics, and specifications. The implementation follows the existing job-stages popup pattern faithfully. Positives
Issues and Suggestions1. No-op serde attribute on OsInfo
2. Narrowing cast u64 to usize in format_size
3. Unused _popup parameter in build_lines The function signature carries 4. No upper bound on scroll position
5. Import split in event.rs
6. Minor: format!(...).as_str() pattern in http_client.rs
Test CoverageDomain logic and app helpers have good coverage. The rendering path ( SummaryThe feature is well-structured and follows established patterns. The three items to address before merging are: the no-op |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/executors.rs`:
- Around line 28-29: Update the TUI deserialization types to match the API:
change the executor struct's last_seen field to an optional integer (e.g.,
Option<u64> or Option<i64>) and/or add #[serde(default)] so nulls deserialize
without error, and change Specification::task_slots from u16 to u32 to match the
API; alternatively ensure the API's ExecutorSpecification type used by the TUI
derives Deserialize (add Deserialize to ExecutorSpecification) so the
Specification type used in this file can be deserialized correctly.
🪄 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: 7c3cb993-ef6b-4ac8-9049-efbf47aa3b44
📒 Files selected for processing (14)
.cursor/rules.md.gemini/rules.mdAGENTS.mdCLAUDE.mdballista-cli/src/tui/app.rsballista-cli/src/tui/domain/executors.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/main/executors/executor_details_popup.rsballista-cli/src/tui/ui/main/executors/mod.rsballista-cli/src/tui/ui/main/mod.rsballista-cli/src/tui/ui/mod.rs
| pub specification: Specification, | ||
| pub metrics: Vec<Metric>, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check if the API documentation or response types include specification and metrics
# Search for API response struct definitions or documentation
rg -nP --type=rust -C5 'struct.*Executor.*Response|pub.*struct.*Executor[^a-zA-Z]' -g '!**/tui/**'
# Search for any API endpoint handlers that return executor data
rg -nP --type=rust -C5 'executor.*api|get.*executor' -g '**/server/**' -g '**/scheduler/**'Repository: martin-augment/datafusion-ballista
Length of output: 49775
🏁 Script executed:
#!/bin/bash
# Examine the TUI Executor struct and its deserialization
cat -n ballista-cli/src/tui/domain/executors.rs | head -60
# Search for any deserialization logic or serde attributes
rg -n 'serde|Deserialize|from_' ballista-cli/src/tui/domain/executors.rs | head -20Repository: martin-augment/datafusion-ballista
Length of output: 2603
🏁 Script executed:
#!/bin/bash
# Find ExecutorSpecification and ExecutorMetricResponse definitions
rg -n 'struct ExecutorSpecification|struct ExecutorMetricResponse' -A 5 ballista/scheduler/src/api/
# Also check for any custom deserializers
rg -n 'impl.*Deserialize|#\[serde' ballista-cli/src/tui/domain/executors.rsRepository: martin-augment/datafusion-ballista
Length of output: 143
🏁 Script executed:
#!/bin/bash
# Broader search for type definitions
rg -n 'ExecutorSpecification|ExecutorMetricResponse' --type=rust | head -30
# Also check the handlers file more carefully for the full context
sed -n '60,80p' ballista/scheduler/src/api/handlers.rsRepository: martin-augment/datafusion-ballista
Length of output: 3923
🏁 Script executed:
#!/bin/bash
# Get the full ExecutorMetricResponse and ExecutorSpecification definitions
sed -n '88,110p' ballista/scheduler/src/api/handlers.rs
# Also check for ExecutorSpecification source (likely protobuf generated)
rg -n 'struct ExecutorSpecification|pub struct ExecutorSpecification' ballista-core/src/serde/
# Check imports in handlers.rs
head -35 ballista/scheduler/src/api/handlers.rs | tail -15Repository: martin-augment/datafusion-ballista
Length of output: 1640
🏁 Script executed:
#!/bin/bash
# Find ExecutorSpecification definition (in ballista_core/src)
fd -t f 'scheduler' ballista-core/src/serde/ 2>/dev/null | head -10
# Search more broadly
rg -n 'pub struct ExecutorSpecification' ballista-core/
# Also check what the API actually serializes
sed -n '254,275p' ballista/scheduler/src/api/handlers.rsRepository: martin-augment/datafusion-ballista
Length of output: 803
🏁 Script executed:
#!/bin/bash
# Search for ExecutorSpecification in ballista directory
find ballista -name "*.rs" -type f -exec grep -l "pub struct ExecutorSpecification" {} \;
# Also check what type it is
rg -B2 -A5 'pub struct ExecutorSpecification' ballista/Repository: martin-augment/datafusion-ballista
Length of output: 1457
🏁 Script executed:
#!/bin/bash
# Check the full ExecutorSpecification implementation in scheduler/mod.rs
sed -n '/pub struct ExecutorSpecification/,/^}/p' ballista/core/src/serde/scheduler/mod.rs | head -30
# Check if there are any #[serde] attributes on ExecutorSpecification
rg -B5 -A10 'pub struct ExecutorSpecification' ballista/core/src/serde/scheduler/mod.rsRepository: martin-augment/datafusion-ballista
Length of output: 643
🏁 Script executed:
#!/bin/bash
# Check if ExecutorSpecification derives Deserialize
rg -B10 'pub struct ExecutorSpecification' ballista/core/src/serde/scheduler/mod.rs | grep -E '#\[derive|Deserialize'
# Check the actual JSON serialization of ExecutorMetricResponse
rg -B5 -A20 'impl.*ExecutorMetricResponse' ballista/scheduler/src/api/handlers.rs | head -40
# Look at what serde does with ExecutorMetricResponse enum
rg -B5 'pub enum ExecutorMetricResponse' ballista/scheduler/src/api/handlers.rs | head -20Repository: martin-augment/datafusion-ballista
Length of output: 1231
🏁 Script executed:
#!/bin/bash
# Verify how ExecutorMetricResponse serializes to JSON by checking serde tag usage
rg -B3 -A10 '#\[serde.*tag.*type.*content' ballista/scheduler/src/api/handlers.rs
# Check if there are any integration tests showing the actual JSON structure
rg -n 'ExecutorBriefResponse|ExecutorMetricResponse' ballista/scheduler/src/api/handlers.rs | grep -i test
# Search for tests that show the JSON structure
fd -t f '*.rs' ballista/scheduler/src/api | xargs grep -l 'json\|json!'Repository: martin-augment/datafusion-ballista
Length of output: 745
🏁 Script executed:
#!/bin/bash
# Check the serde tag configuration for ExecutorMetricResponse
sed -n '88,110p' ballista/scheduler/src/api/handlers.rs
# Look for integration tests or examples showing actual API responses
find ballista/scheduler/src -name "*.rs" -type f | xargs grep -l 'ExecutorBriefResponse' | head -5
# Check if there are any test files
ls -la ballista/scheduler/src/api/Repository: martin-augment/datafusion-ballista
Length of output: 1277
🏁 Script executed:
#!/bin/bash
# Let me understand the exact JSON structure that will be generated
# Check if ExecutorMetricResponse has Deserialize derive
rg -B10 'pub enum ExecutorMetricResponse' ballista/scheduler/src/api/handlers.rs | grep -E 'Serialize|Deserialize'
# Create a simple Python script to show what the JSON will look like
python3 << 'EOF'
# Show what the serde enum serialization produces
enum_config = {
'tag': 'type',
'content': 'value',
'rename_all': 'snake_case'
}
variants = [
'AvailableMemory(64)',
'TotalMemory(256)',
'UsedMemory(128)'
]
print("With #[serde(tag=\"type\", content=\"value\", rename_all=\"snake_case\")]:")
print("AvailableMemory(64) serializes to:")
print('{"type": "available_memory", "value": 64}')
print()
print("TUI Metric struct expects:")
print('{"type": "...", "value": ...}')
print("So deserialization should work!")
EOF
# The real issue is last_seen - check the type mismatch
echo "=== API last_seen type ==="
sed -n '70,77p' ballista/scheduler/src/api/handlers.rs
echo "=== TUI last_seen type ==="
sed -n '22,30p' ballista-cli/src/tui/domain/executors.rsRepository: martin-augment/datafusion-ballista
Length of output: 875
🏁 Script executed:
#!/bin/bash
# Check if there are any custom serde attributes on the TUI fields
cat -n ballista-cli/src/tui/domain/executors.rs | sed -n '20,35p'
# Check if there are any custom deserializers in the file
rg -n 'deserialize' ballista-cli/src/tui/domain/executors.rs
# Verify the actual type mismatch - check if Specification matches
sed -n '45,48p' ballista-cli/src/tui/domain/executors.rs
echo "=== API Specification ==="
sed -n '500,550p' ballista/core/src/serde/scheduler/mod.rs | grep -A 10 'pub struct ExecutorSpecification'
# Check how the API actually handles null/None values in JSON
rg -B5 -A5 'last_seen.*Option' ballista/scheduler/src/api/handlers.rsRepository: martin-augment/datafusion-ballista
Length of output: 965
🏁 Script executed:
#!/bin/bash
# Verify the ExecutorSpecification serialization attributes
rg -B5 -A10 'pub struct ExecutorSpecification' ballista/core/src/serde/scheduler/mod.rs
# Check the full serde configuration
sed -n '175,230p' ballista/core/src/serde/scheduler/mod.rs | head -50
# Final verification: does ExecutorSpecification derive Serialize/Deserialize?
rg '#\[derive' ballista/core/src/serde/scheduler/mod.rs | grep -B1 -A1 'ExecutorSpecification'Repository: martin-augment/datafusion-ballista
Length of output: 2081
🏁 Script executed:
#!/bin/bash
# Critical check: Does ExecutorSpecification derive Deserialize?
rg -B10 'pub struct ExecutorSpecification' ballista/core/src/serde/scheduler/mod.rs | head -15
# And verify if it's used in any deserialization context in the TUI
rg -n 'from_str|deserialize' ballista-cli/src/tui/ | grep -i executor
# Final check: How does the TUI client actually fetch executor data?
rg -n 'get_executors\|ExecutorBriefResponse\|Executor' ballista-cli/src/ | grep -E '\.rs:.*http|\.rs:.*client|\.rs:.*request' | head -10Repository: martin-augment/datafusion-ballista
Length of output: 520
🏁 Script executed:
#!/bin/bash
# Check exactly what ExecutorSpecification derives - get the full line
sed -n '170,185p' ballista/core/src/serde/scheduler/mod.rs
# Check if there are any serde attributes on ExecutorSpecification
rg -B3 'pub struct ExecutorSpecification' ballista/core/src/serde/scheduler/mod.rs | head -10
# Verify Specification type in TUI - does it have u32 or u16 for task_slots?
sed -n '45,50p' ballista-cli/src/tui/domain/executors.rsRepository: martin-augment/datafusion-ballista
Length of output: 972
🏁 Script executed:
#!/bin/bash
# Check if ExecutorSpecification has a custom Deserialize implementation
rg -n 'impl.*Deserialize.*ExecutorSpecification|impl.*From.*ExecutorSpecification' ballista/core/src/serde/scheduler/mod.rs
# Check if ExecutorMetricResponse has Deserialize
rg -B20 'pub enum ExecutorMetricResponse' ballista/scheduler/src/api/handlers.rs | grep -E 'Deserialize|derive'
# Check if there's a custom impl for deserializing ExecutorMetricResponse
rg -n 'impl.*Deserialize.*ExecutorMetricResponse' ballista/scheduler/src/api/handlers.rs
# Look for FromStr or From trait implementations
rg -n 'impl.*From.*for.*Executor|impl.*From.*for.*Specification' ballista-cli/src/tui/Repository: martin-augment/datafusion-ballista
Length of output: 183
Fix type mismatches between API responses and TUI deserialization structs.
The API does provide specification and metrics fields, but deserialization will fail due to type incompatibilities:
-
last_seentype mismatch: API sendsOption<u128>(can be null), but TUI expectsi64(non-nullable). When the API returnsnull, deserialization will fail. -
ExecutorSpecificationnot deserializable: The API'sExecutorSpecificationderives onlySerialize, notDeserialize. Additionally, it hastask_slots: u32while the TUI expectstask_slots: u16.
Changes needed:
- Change TUI's
last_seenfield fromi64toOption<u64>orOption<i64>with#[serde(default)] - Change TUI's
Specification::task_slotsfromu16tou32to match the API, or ensure ExecutorSpecification implements Deserialize
🤖 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/executors.rs` around lines 28 - 29, Update the
TUI deserialization types to match the API: change the executor struct's
last_seen field to an optional integer (e.g., Option<u64> or Option<i64>) and/or
add #[serde(default)] so nulls deserialize without error, and change
Specification::task_slots from u16 to u32 to match the API; alternatively ensure
the API's ExecutorSpecification type used by the TUI derives Deserialize (add
Deserialize to ExecutorSpecification) so the Specification type used in this
file can be deserialized correctly.
There was a problem hiding this comment.
value:useful; category:bug; feedback: The CodeRabbit AI reviewer is correct! The type of last_seen should be an Option of an integer. There is no need to read it as u128 though. No one expects to support dates in the far future and the helper methods to format timestamps as string work with i64. annoying: There is no ExecutorSpecification struct in the application code base
value:useful; category:bug; feedback: The Claude AI reviewer is correct! Indeed the Serde attribute is misplaced. It should be on a struct field instead of the struct itself, where it is ignored. |
value:good-to-have; category:bug; feedback: The CodeRabbit AI reviewer is correct! The scroll_position should not grow more than ~20 lines, otherwise it will scroll down too much and no info will be shown on the popup window. Prevents rendering useless popup content. |
value:good-but-wont-fix; category:bug; feedback: The Claude AI reviewer is correct! The data coming from the Scheduler REST API uses u64 type but the helper methods coming from DataFusion use usize and this leads to the castings. The TUI application is not expected to be used on a 32-bit system, since DataFusion itself does not support it, so there is no need to worry here. |
2 participants
1670: To review by AI