Conversation
WalkthroughAdds a new Changes
Sequence Diagram(s)sequenceDiagram
participant User as User/Terminal
participant TUI as Basilica TUI (App)
participant Events as EventHandler
participant SDK as Basilica SDK / API
participant Auth as Auth System
User->>TUI: launch (args, tick_rate)
TUI->>Auth: initialize / check token
alt no valid token
TUI->>Auth: DeviceFlow::start_flow()
Auth-->>TUI: DeviceAuthInstructions + DeviceFlowPending
TUI->>User: show verification URI & code
User->>Auth: complete approval (external)
Auth->>Auth: pending.wait_for_completion() (poll)
Auth-->>TUI: TokenSet
else token present
TUI->>SDK: create client with token
end
loop main loop
Events->>TUI: Tick / Key / Mouse
alt Tick → refresh
TUI->>SDK: refresh_data()
SDK-->>TUI: user/miner updates
else Key → action
TUI->>SDK: perform API action (rent/ssh/logs)
SDK-->>TUI: result
end
TUI->>TUI: render_app() -> screen.render_with_ctx()
TUI->>User: draw frame
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 18
🧹 Nitpick comments (32)
crates/basilica-tui/Cargo.toml (1)
37-39: Unused dependencies declared for future use.
tokio-tungsteniteandeventsource-clientare declared but not currently used in the provided code. Thestreams.rsfile has TODOs indicating these will be used for SSE streaming. Consider adding a comment in the Cargo.toml to clarify these are for upcoming features, or defer adding them until implementation.crates/basilica-tui/src/events/tick.rs (1)
22-29: Minor timing drift in check().Setting
last_tick = Instant::now()on each successful check can cause drift if there's processing delay between the elapsed check and the reset. For more precise intervals, consider adding the interval duration tolast_tickinstead.🔎 Proposed fix to prevent drift
pub fn check(&mut self) -> bool { if self.last_tick.elapsed() >= self.interval { - self.last_tick = Instant::now(); + self.last_tick += self.interval; true } else { false } }crates/basilica-tui/src/events/input.rs (1)
26-27: RemoveNode action has no key binding.
KeyAction::RemoveNodeis declared but not mapped inparse_key_action. If this is intentional scaffolding, consider adding a comment. Otherwise, add the missing binding (e.g.,'x'key).crates/basilica-tui/src/config.rs (1)
110-117: Consider aligning config directory approach with basilica-cli.The CLI crate uses
etcetera::choose_base_strategy(seecrates/basilica-cli/src/config/mod.rs:293-298) while this TUI usesdirectories::ProjectDirs. This could result in different config locations for the same user. Consider using the same approach for consistency across Basilica tools.crates/basilica-tui/src/data/streams.rs (3)
53-64: Consider implementingFromStrtrait instead of custom method.
LogLevel::from_strshadows the standardFromStrtrait method name. Implementing the trait would make the type more idiomatic and enable.parse()usage.🔎 Proposed implementation
+use std::str::FromStr; +impl FromStr for LogLevel { + type Err = (); + + fn from_str(s: &str) -> Result<Self, Self::Err> { + match s.to_uppercase().as_str() { + "DEBUG" | "DBG" => Ok(Self::Debug), + "INFO" | "INF" => Ok(Self::Info), + "WARN" | "WRN" | "WARNING" => Ok(Self::Warn), + "ERROR" | "ERR" => Ok(Self::Error), + _ => Err(()), + } + } +} -impl LogLevel { - pub fn from_str(s: &str) -> Option<Self> { - match s.to_uppercase().as_str() { - "DEBUG" | "DBG" => Some(Self::Debug), - "INFO" | "INF" => Some(Self::Info), - "WARN" | "WRN" | "WARNING" => Some(Self::Warn), - "ERROR" | "ERR" => Some(Self::Error), - _ => None, - } - } -}
248-253: O(n) removal in EventStream::push.Using
remove(0)on a Vec is O(n) as it shifts all remaining elements. For a frequently-updated event buffer, consider usingVecDequefor O(1) push/pop operations at both ends.🔎 Proposed fix using VecDeque
+use std::collections::VecDeque; pub struct EventStream { - events: Vec<AppEvent>, + events: VecDeque<AppEvent>, max_events: usize, } impl EventStream { pub fn new(max_events: usize) -> Self { Self { - events: Vec::with_capacity(max_events), + events: VecDeque::with_capacity(max_events), max_events, } } pub fn push(&mut self, event: AppEvent) { if self.events.len() >= self.max_events { - self.events.remove(0); + self.events.pop_front(); } - self.events.push(event); + self.events.push_back(event); } pub fn recent(&self, count: usize) -> Vec<&AppEvent> { let start = self.events.len().saturating_sub(count); - &self.events[start..] + self.events.iter().skip(start).collect() }
68-76: Unbounded channel could grow without limit.The unbounded channel for log events could accumulate indefinitely if consumers don't keep up. Consider using a bounded channel with a reasonable buffer size to apply backpressure and prevent memory exhaustion.
crates/basilica-tui/src/data/miner.rs (1)
44-74: Consider VecDeque for better performance in sliding window operations.The
push_*methods useVec::remove(0)which is O(n) since it shifts all elements. For a fixed-size sliding window,VecDequewithpop_front()would be O(1).🔎 Proposed refactor using VecDeque
+use std::collections::VecDeque; + /// Historical metrics data for visualization #[derive(Debug, Default, Clone)] pub struct MetricsHistory { /// GPU utilization history (last N samples) - pub gpu_utilization: Vec<f64>, + pub gpu_utilization: VecDeque<f64>, /// Memory utilization history - pub memory_utilization: Vec<f64>, + pub memory_utilization: VecDeque<f64>, /// Revenue history (daily) - pub daily_revenue: Vec<f64>, + pub daily_revenue: VecDeque<f64>, /// Request rate history - pub request_rate: Vec<f64>, + pub request_rate: VecDeque<f64>, /// Max samples to keep pub max_samples: usize, } impl MetricsHistory { pub fn new(max_samples: usize) -> Self { Self { - gpu_utilization: Vec::with_capacity(max_samples), - memory_utilization: Vec::with_capacity(max_samples), - daily_revenue: Vec::with_capacity(max_samples), - request_rate: Vec::with_capacity(max_samples), + gpu_utilization: VecDeque::with_capacity(max_samples), + memory_utilization: VecDeque::with_capacity(max_samples), + daily_revenue: VecDeque::with_capacity(max_samples), + request_rate: VecDeque::with_capacity(max_samples), max_samples, } } pub fn push_gpu_util(&mut self, value: f64) { if self.gpu_utilization.len() >= self.max_samples { - self.gpu_utilization.remove(0); + self.gpu_utilization.pop_front(); } - self.gpu_utilization.push(value); + self.gpu_utilization.push_back(value); } pub fn push_memory_util(&mut self, value: f64) { if self.memory_utilization.len() >= self.max_samples { - self.memory_utilization.remove(0); + self.memory_utilization.pop_front(); } - self.memory_utilization.push(value); + self.memory_utilization.push_back(value); } pub fn push_revenue(&mut self, value: f64) { if self.daily_revenue.len() >= self.max_samples { - self.daily_revenue.remove(0); + self.daily_revenue.pop_front(); } - self.daily_revenue.push(value); + self.daily_revenue.push_back(value); } }crates/basilica-tui/src/ui/screens/miner/earnings.rs (1)
15-259: Significant code duplication between render paths.The file implements two nearly identical rendering paths (
renderandrender_with_ctx), where each helper function is duplicated with minimal differences. This creates maintenance burden and increases the risk of inconsistencies.🔎 Suggested refactor to eliminate duplication
Consider extracting the rendering logic into shared helper functions that accept the theme and data parameters directly:
// Shared helper that both paths can call fn render_earnings_summary_shared(frame: &mut Frame, theme: &crate::ui::Theme, area: Rect) { // Implementation here } // Then simplify the two public render functions: pub fn render(frame: &mut Frame, app: &App) { let theme = &app.theme; // ... layout setup ... render_earnings_summary_shared(frame, theme, left_chunks[0]); // etc. } pub fn render_with_ctx(frame: &mut Frame, ctx: &RenderContext) { let theme = ctx.theme; // ... layout setup ... render_earnings_summary_shared(frame, theme, left_chunks[0]); // etc. }This would eliminate the duplicate
_ctxvariants and centralize the rendering logic.crates/basilica-tui/src/ui/screens/rentals.rs (1)
59-64: Consider extracting status matching to a dedicated helper.The status string matching logic is repeated across screens. Consider creating a helper function or using an enum to centralize status-to-style mapping for consistency and maintainability.
crates/basilica-tui/src/ui/mod.rs (1)
65-69: Silently ignoring terminal cleanup errors in Drop.The
Dropimplementation discards any errors fromexit(), which could leave the terminal in an inconsistent state (raw mode enabled, alternate screen active, hidden cursor). Consider logging the error before discarding it.🔎 Proposed enhancement
impl Drop for Tui { fn drop(&mut self) { - let _ = self.exit(); + if let Err(e) = self.exit() { + eprintln!("Failed to cleanup terminal: {}", e); + } } }crates/basilica-tui/src/ui/widgets/gauge.rs (1)
13-13: Redundant clamping operation.The
.min(100)call is redundant afterclamp(0.0, 100.0), which already ensures the value is at most 100.0.🔎 Proposed simplification
-let percent_u16 = (percent.clamp(0.0, 100.0) as u16).min(100); +let percent_u16 = percent.clamp(0.0, 100.0) as u16;Also applies to: 41-41
crates/basilica-tui/src/ui/screens/marketplace.rs (1)
44-47: Consider importingHashMapat the module level for consistency.The inline
std::collections::HashMapimport works but is unconventional. Other files in this crate import types at the top of the file.Suggested change
Add to imports at line 3:
use std::collections::HashMap;Then simplify line 44:
- let mut gpu_types: std::collections::HashMap<&str, usize> = std::collections::HashMap::new(); + let mut gpu_types: HashMap<&str, usize> = HashMap::new();crates/basilica-tui/src/ui/screens/miner/nodes.rs (3)
52-115: Significant code duplication betweenrender_node_listandrender_node_list_ctx.The sample nodes array is duplicated verbatim in both functions (lines 52-115 and 279-342). This makes maintenance error-prone—any change must be applied in two places.
Extract shared node data to a constant or helper
const SAMPLE_NODES: [(&str, &str, &str, &str, &str, &str); 8] = [ ("node-001", "192.168.1.10", "22", "root", "H100 x 4", "Healthy"), ("node-002", "192.168.1.11", "22", "root", "H100 x 4", "Healthy"), // ... rest of nodes ];Then both
render_node_listandrender_node_list_ctxcan referenceSAMPLE_NODES.Also applies to: 279-342
172-242: Node details panel is static and ignores selection.The
render_node_detailsfunction always displays hardcoded "node-001" data regardless of which node is selected viaapp.selected_index. This appears to be scaffolding code.Consider adding a TODO comment to clarify this is placeholder behavior:
// TODO: Replace with actual selected node data from miner_data.nodes[selected_index] let details = vec![
269-270: Inconsistent parameter signature forrender_node_details_ctx.This function takes
theme: &crate::ui::Themedirectly instead ofctx: &RenderContextlike other_ctxvariants. While functional, it breaks the pattern established byrender_node_list_ctxand other context-based helpers.-fn render_node_details_ctx(frame: &mut Frame, theme: &crate::ui::Theme, area: Rect) { +fn render_node_details_ctx(frame: &mut Frame, ctx: &RenderContext, area: Rect) { + let theme = ctx.theme;crates/basilica-tui/src/ui/components/notifications.rs (1)
13-66: Consider extracting shared rendering logic to reduce duplication.
render_notificationsandrender_notifications_ctxshare nearly identical implementations (~50 lines each). The only difference is the source ofthemeandnotifications.Suggested refactor
Extract a shared helper:
fn render_notification_toasts( frame: &mut Frame, notifications: &[Notification], theme: &Theme, ) { // ... shared implementation } pub fn render_notifications(frame: &mut Frame, app: &App) { render_notification_toasts(frame, &app.notifications, &app.theme); } pub fn render_notifications_ctx(frame: &mut Frame, ctx: &RenderContext) { render_notification_toasts(frame, ctx.notifications, ctx.theme); }Also applies to: 69-117
crates/basilica-tui/src/ui/components/help.rs (2)
92-101:help_lineuses hardcoded style instead of theme.The key styling on line 96 uses
ratatui::style::Style::default().add_modifier(Modifier::BOLD)instead of a theme method liketheme.text_bold(). This could cause visual inconsistency if the theme's text color differs from the terminal default.Consider accepting
themeas a parameter:fn help_line<'a>(key: &'a str, desc: &'a str, theme: &Theme) -> Line<'a> { Line::from(vec![ Span::styled(format!("{:>12}", key), theme.text_bold()), Span::raw(" "), Span::raw(desc), ]) }
14-89: Duplication betweenrender_help_overlayandrender_help_overlay_ctx.Both functions share ~70 lines of identical content-building logic. This is the same pattern seen in
notifications.rs.A shared helper could build the
linesvector givenmodeandtheme, with thin wrappers for each entry point. This would reduce duplication from ~140 lines to ~50.Also applies to: 114-185
crates/basilica-tui/src/ui/screens/miner/fleet.rs (5)
16-17: Unused variabletheme.The
themevariable on line 17 is never used withinrender()since it's re-extracted within each helper function.Suggested fix
pub fn render(frame: &mut Frame, app: &App) { - let theme = &app.theme; let area = frame.area();
48-117: Significant code duplication betweenrender_stats_barandrender_stats_bar_ctx.These two functions are nearly identical (~70 lines each), differing only in how they obtain the theme. Consider extracting a shared internal function that accepts the theme directly, then have both public variants delegate to it.
Proposed refactor pattern
fn render_stats_bar_inner(frame: &mut Frame, theme: &Theme, area: Rect) { // Shared implementation } fn render_stats_bar(frame: &mut Frame, app: &App, area: Rect) { render_stats_bar_inner(frame, &app.theme, area); } fn render_stats_bar_ctx(frame: &mut Frame, theme: &Theme, area: Rect) { render_stats_bar_inner(frame, theme, area); }Also applies to: 329-386
161-232: Hardcoded sample node data duplicated across two functions.The identical 8-node sample data array is defined twice—once in
render_node_gridand again inrender_node_grid_ctx. Extract this to a module-level constant or, better yet, source it fromRenderContext.miner_datafor real data integration.Suggested approach
// At module level: const SAMPLE_NODES: &[(&str, &str, &str, &str, f64, f64, &str)] = &[ ("node-001", "192.168.1.10", "H100 x 4", "Healthy", 85.0, 72.0, "3/4 assigned"), // ... rest of nodes ]; // Then in both functions: let nodes = SAMPLE_NODES;Or integrate with actual data:
let nodes = ctx.miner_data.nodes.iter()...Also applies to: 427-498
119-155:render_stat_boxandrender_stat_box_ctxare duplicates.Both functions have identical logic, differing only in how
themeis obtained. Apply the same consolidation pattern as suggested for the stats bar.Also applies to: 388-422
234-300:render_node_gridandrender_node_grid_ctxshare identical table-rendering logic.Beyond the duplicated sample data, the row-building, styling, and table construction logic is repeated. Consider a shared helper that accepts
theme,selected_index, and the node data.Also applies to: 500-565
crates/basilica-tui/src/ui/screens/miner/validators.rs (2)
14-42: Duplicated rendering paths betweenrenderandrender_with_ctx.Same pattern as
fleet.rs—the two public entry points duplicate nearly all logic. Consider consolidating via a shared internal function or convertingrender()to build a temporaryRenderContextand delegate.Also applies to: 239-264
146-171: Hardcoded validator data duplicated.The sample validator array appears twice. Extract to a constant or, when ready, source from
ctx.miner_data.Also applies to: 364-389
crates/basilica-tui/src/app.rs (2)
688-699: ConsiderVecDequefor notification queue.Using
Vec::remove(0)is O(n). While acceptable for a 5-item cap,VecDeque::pop_front()would be O(1) and more idiomatic for queue semantics.Suggested change
- pub notifications: Vec<Notification>, + pub notifications: VecDeque<Notification>, // In add_notification: - if self.notifications.len() > 5 { - self.notifications.remove(0); - } + if self.notifications.len() > 5 { + self.notifications.pop_front(); + }
294-306: Health check during client creation lacks explicit timeout.If the API server is unresponsive,
client.health_check().awaitcould hang indefinitely during startup. Consider wrapping with a timeout.Suggested fix
async fn create_client(api_url: &str) -> Result<BasilicaClient> { let client = ClientBuilder::default() .base_url(api_url) .with_file_auth() .build()?; - client.health_check().await?; + tokio::time::timeout( + std::time::Duration::from_secs(5), + client.health_check() + ).await + .map_err(|_| anyhow::anyhow!("Health check timed out"))? + ?; Ok(client) }crates/basilica-tui/src/data/user.rs (4)
157-205: Duplicate logic betweenfetch_rentalsmethod andfetch_rentals_datafunction.The rental parsing logic (uptime calculation, GPU extraction) is duplicated. The method and standalone function should share implementation.
Suggested refactor
// Keep only the standalone function async fn fetch_rentals_data(client: &Arc<BasilicaClient>) -> Result<Vec<RentalInfo>> { ... } // Method delegates to standalone async fn fetch_rentals(&mut self, client: &Arc<BasilicaClient>) -> Result<()> { self.loading.rentals = true; match fetch_rentals_data(client).await { Ok(rentals) => { self.rentals = rentals; self.loading.rentals = false; Ok(()) } Err(e) => { self.loading.rentals = false; Err(e) } } }Also applies to: 359-397
207-254: Same duplication pattern forfetch_offerings/fetch_offerings_data.Apply the same consolidation as suggested for rentals.
Also applies to: 399-437
256-286: Same duplication pattern forfetch_deployments/fetch_deployments_data.Apply the same consolidation as suggested for rentals.
Also applies to: 439-459
288-317: Same duplication pattern forfetch_balance/fetch_balance_data.Apply the same consolidation as suggested for rentals.
Also applies to: 461-481
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (37)
Cargo.tomlcrates/basilica-tui/Cargo.tomlcrates/basilica-tui/src/actions.rscrates/basilica-tui/src/app.rscrates/basilica-tui/src/config.rscrates/basilica-tui/src/data/miner.rscrates/basilica-tui/src/data/mod.rscrates/basilica-tui/src/data/streams.rscrates/basilica-tui/src/data/user.rscrates/basilica-tui/src/events/input.rscrates/basilica-tui/src/events/mod.rscrates/basilica-tui/src/events/tick.rscrates/basilica-tui/src/main.rscrates/basilica-tui/src/ui/components/footer.rscrates/basilica-tui/src/ui/components/header.rscrates/basilica-tui/src/ui/components/help.rscrates/basilica-tui/src/ui/components/mod.rscrates/basilica-tui/src/ui/components/notifications.rscrates/basilica-tui/src/ui/mod.rscrates/basilica-tui/src/ui/screens/billing.rscrates/basilica-tui/src/ui/screens/dashboard.rscrates/basilica-tui/src/ui/screens/deployments.rscrates/basilica-tui/src/ui/screens/marketplace.rscrates/basilica-tui/src/ui/screens/miner/earnings.rscrates/basilica-tui/src/ui/screens/miner/fleet.rscrates/basilica-tui/src/ui/screens/miner/logs.rscrates/basilica-tui/src/ui/screens/miner/mod.rscrates/basilica-tui/src/ui/screens/miner/nodes.rscrates/basilica-tui/src/ui/screens/miner/validators.rscrates/basilica-tui/src/ui/screens/mod.rscrates/basilica-tui/src/ui/screens/rentals.rscrates/basilica-tui/src/ui/theme.rscrates/basilica-tui/src/ui/widgets/gauge.rscrates/basilica-tui/src/ui/widgets/logs.rscrates/basilica-tui/src/ui/widgets/mod.rscrates/basilica-tui/src/ui/widgets/sparkline.rscrates/basilica-tui/src/ui/widgets/table.rs
🧰 Additional context used
🧬 Code graph analysis (18)
crates/basilica-tui/src/ui/screens/miner/mod.rs (1)
crates/basilica-tui/src/data/miner.rs (2)
nodes(243-243)nodes(251-251)
crates/basilica-tui/src/ui/screens/miner/fleet.rs (4)
crates/basilica-tui/src/ui/theme.rs (5)
text(75-77)header(142-147)default(194-196)text_muted(80-82)text_accent(93-95)crates/basilica-tui/src/ui/widgets/gauge.rs (1)
mini_bar(60-65)crates/basilica-tui/src/ui/components/header.rs (1)
header_height(149-151)crates/basilica-tui/src/ui/components/footer.rs (1)
footer_height(130-132)
crates/basilica-tui/src/ui/widgets/logs.rs (2)
crates/basilica-tui/src/ui/theme.rs (3)
text(75-77)text_muted(80-82)default(194-196)crates/basilica-tui/src/ui/screens/miner/logs.rs (1)
render_logs(77-192)
crates/basilica-tui/src/ui/mod.rs (1)
crates/basilica-tui/src/app.rs (1)
new(242-292)
crates/basilica-tui/src/ui/theme.rs (3)
crates/basilica-tui/src/ui/widgets/logs.rs (1)
style(30-37)crates/basilica-tui/src/config.rs (2)
default(69-75)default(88-95)crates/basilica-tui/src/data/streams.rs (2)
default(204-206)default(266-268)
crates/basilica-tui/src/ui/components/footer.rs (2)
crates/basilica-tui/src/ui/theme.rs (1)
text(75-77)crates/basilica-tui/src/app.rs (1)
new(242-292)
crates/basilica-tui/src/main.rs (6)
crates/basilica-common/src/logging/mod.rs (1)
init_logging(32-63)crates/basilica-tui/src/config.rs (1)
load(100-120)crates/basilica-tui/src/app.rs (1)
new(242-292)crates/basilica-tui/src/events/mod.rs (1)
new(36-73)crates/basilica-tui/src/events/tick.rs (1)
new(14-19)crates/basilica-tui/src/ui/mod.rs (1)
new(25-29)
crates/basilica-tui/src/events/tick.rs (2)
crates/basilica-tui/src/app.rs (1)
new(242-292)crates/basilica-tui/src/events/mod.rs (1)
new(36-73)
crates/basilica-tui/src/ui/screens/miner/logs.rs (3)
crates/basilica-tui/src/ui/components/header.rs (3)
header_height(149-151)render_header(14-80)render_header_ctx(83-146)crates/basilica-tui/src/ui/components/footer.rs (3)
footer_height(130-132)render_footer(13-67)render_footer_ctx(75-127)crates/basilica-tui/src/ui/widgets/logs.rs (1)
render_logs(50-86)
crates/basilica-tui/src/data/miner.rs (1)
crates/basilica-tui/src/app.rs (1)
new(242-292)
crates/basilica-tui/src/ui/components/mod.rs (1)
crates/basilica-tui/src/ui/theme.rs (1)
header(142-147)
crates/basilica-tui/src/ui/components/notifications.rs (2)
crates/basilica-tui/src/ui/theme.rs (2)
text(75-77)default(194-196)crates/basilica-tui/src/app.rs (1)
new(242-292)
crates/basilica-tui/src/events/mod.rs (2)
crates/basilica-tui/src/app.rs (1)
new(242-292)crates/basilica-tui/src/events/tick.rs (1)
new(14-19)
crates/basilica-tui/src/config.rs (3)
crates/basilica-tui/src/data/streams.rs (2)
default(204-206)default(266-268)crates/basilica-tui/src/ui/theme.rs (1)
default(194-196)crates/basilica-cli/src/config/mod.rs (1)
config_dir(294-299)
crates/basilica-tui/src/ui/components/help.rs (1)
crates/basilica-tui/src/ui/theme.rs (3)
text(75-77)text_bold(85-90)text_muted(80-82)
crates/basilica-tui/src/ui/screens/rentals.rs (4)
crates/basilica-tui/src/ui/theme.rs (4)
text(75-77)header(142-147)default(194-196)text_muted(80-82)crates/basilica-tui/src/ui/components/header.rs (1)
header_height(149-151)crates/basilica-tui/src/ui/components/footer.rs (1)
footer_height(130-132)crates/basilica-tui/src/data/user.rs (1)
new(93-95)
crates/basilica-tui/src/ui/screens/marketplace.rs (3)
crates/basilica-tui/src/ui/theme.rs (7)
text(75-77)header(142-147)default(194-196)text_bold(85-90)text_accent(93-95)text_muted(80-82)text_warning(103-105)crates/basilica-tui/src/ui/components/header.rs (2)
header_height(149-151)render_header_ctx(83-146)crates/basilica-tui/src/ui/components/footer.rs (1)
render_footer_ctx(75-127)
crates/basilica-tui/src/data/streams.rs (1)
crates/basilica-tui/src/app.rs (1)
new(242-292)
🔇 Additional comments (31)
Cargo.toml (1)
14-14: LGTM!The new
basilica-tuicrate is properly added to the workspace members list.crates/basilica-tui/Cargo.toml (1)
1-46: Manifest structure looks good.The package metadata, binary target, and dependency organization are well-structured. The mix of workspace and explicit dependencies is appropriate for a TUI crate with specific version requirements.
crates/basilica-tui/src/events/tick.rs (1)
1-35: LGTM overall.The Timer utility is a straightforward implementation for tracking periodic events. It's appropriately marked as scaffolding with
#![allow(dead_code)].crates/basilica-tui/src/events/input.rs (1)
50-52: Verify GoToTab index for digit '0'.Pressing '0' maps to
GoToTab(0). Confirm this is the intended behavior - some TUI applications use '1'-'9' for tabs 0-8, while '0' goes to tab 9 or is unused.crates/basilica-tui/src/ui/theme.rs (1)
1-197: LGTM!The Theme struct provides a comprehensive and well-organized theming system. The style helper methods are consistently implemented and the color palette choices (Catppuccin-inspired dark theme) are visually coherent. Good use of the Default trait delegating to
dark().crates/basilica-tui/src/data/streams.rs (1)
1-269: Streaming infrastructure is well-structured.Good use of oneshot channels for cancellation, proper Drop implementation to clean up streams, and clear separation of concerns between log streaming and event buffering. The simulated SSE with TODOs is appropriate scaffolding.
crates/basilica-tui/src/data/miner.rs (1)
174-253: LGTM! Well-structured data model with clear scaffolding.The MinerData implementation provides a solid foundation with:
- Clear TODO markers for future integration
- Proper async scaffolding with tokio
- Safe aggregation methods with empty collection handling
- Simulated metrics for development/testing
crates/basilica-tui/src/ui/components/mod.rs (1)
1-8: LGTM! Clean module organization.The component module structure is well-organized and clearly exposes the shared UI components.
crates/basilica-tui/src/ui/screens/miner/mod.rs (1)
1-10: LGTM! Clear miner screen module structure.The module organization aligns well with the miner mode functionality and is appropriately documented as scaffolding.
crates/basilica-tui/src/main.rs (1)
19-61: LGTM! Well-structured CLI entry point.The main function follows best practices:
- Clean CLI argument parsing with Clap
- Proper async runtime setup with Tokio
- Centralized logging initialization
- Clear error propagation with anyhow
crates/basilica-tui/src/ui/widgets/mod.rs (1)
1-10: LGTM! Clean widget module structure.The widget organization is straightforward and provides reusable UI components for the TUI.
crates/basilica-tui/src/data/mod.rs (1)
1-8: LGTM! Clean data module with convenient re-exports.The module provides a clear public API surface for data management, making MinerData and UserData easily accessible.
crates/basilica-tui/src/ui/screens/mod.rs (1)
1-9: LGTM! Comprehensive screen module organization.The screen structure covers all major TUI sections for both user and miner modes.
crates/basilica-tui/src/ui/screens/miner/earnings.rs (1)
59-259: LGTM! Well-structured earnings screen with clear layout.The earnings screen provides a comprehensive view with:
- Clean two-column layout (summary/charts on left, history on right)
- Proper theme integration throughout
- Selection highlighting for payment history
- Sparkline visualization for revenue trends
The hardcoded sample data is appropriate for scaffolding, as indicated by the module-level documentation.
crates/basilica-tui/src/ui/screens/billing.rs (1)
109-110: TODOs for API integration are clearly marked.The placeholder data for spending and deposit address is well-documented with TODO comments. These can be addressed when the API endpoints are ready.
Also applies to: 144-144
crates/basilica-tui/src/ui/widgets/sparkline.rs (1)
37-59: Well-implemented text sparkline with proper edge case handling.The function correctly handles empty input, computes min/max, handles zero-range data, and clamps the index to prevent out-of-bounds access. The normalization logic is sound.
crates/basilica-tui/src/ui/widgets/gauge.rs (1)
60-65: Well-implemented mini bar generator.The function correctly handles the percentage-to-width conversion with proper saturation arithmetic to prevent underflow.
crates/basilica-tui/src/ui/screens/marketplace.rs (1)
161-166: Selection state management is well-implemented.The bounds checking with
.min(user_data.offerings.len().saturating_sub(1))correctly handles edge cases whereselected_indexmight exceed the offerings count after data refresh.crates/basilica-tui/src/ui/screens/dashboard.rs (1)
217-230: Well-structured uptime formatting helper.The
format_uptimefunction correctly handles the minute→hour→day transitions with clear boundary conditions.crates/basilica-tui/src/events/mod.rs (2)
34-73: Event handler implementation looks solid.The pattern of storing
_txto keep the channel alive is correct. Thetokio::select!properly handles both tick and terminal events, and the loop breaks cleanly when the receiver is dropped.One consideration: if event processing is slow,
tick_interval.tick()may burst multiple ticks. This is typically acceptable for TUI tick rates, but be aware if you see unexpected tick clustering.
3-4: No action needed. Theinputandtickmodules are intentionally private and serve as internal implementation details for the events module. External code only imports the publicEventandEventHandlertypes, which provide the complete public interface. There is no evidence these internal modules are intended for external use.Likely an incorrect or invalid review comment.
crates/basilica-tui/src/ui/widgets/logs.rs (2)
49-86: Clean log viewer implementation with auto-scroll support.The
render_logsfunction correctly builds styled log entries and handles auto-scroll by selecting the last item in theListState. The theming integration is consistent with the rest of the codebase.
21-27: LogLevel is duplicated across three files, not one, but without actual conversion overhead.
LogLevelis defined inlogs.rs,miner.rs, andstreams.rswith identical variants (Debug, Info, Warn, Error). However, the original concern about conversion overhead is incorrect—these types are not imported or converted across module boundaries. Each definition is isolated to its layer: the UI widget (logs.rs) maintains its ownLogLevelwith styling helpers, while the data layer (miner.rs and streams.rs) maintain separate versions with serialization and parsing. The duplication is architectural rather than problematic in practice, though consolidating these types to a shared module could reduce maintenance burden if the enum ever needs to change.Likely an incorrect or invalid review comment.
crates/basilica-tui/src/ui/widgets/table.rs (2)
11-51: Well-designed reusable table helper.The
styled_tablefunction provides a clean abstraction for creating themed tables. The separation of concerns is good—it handles styling and structure while leaving state management to callers.One note: the returned
TableStateis always default (no selection). Callers will need to set the selection themselves before rendering.
53-64: Robust status cell with case-insensitive matching.The
status_cellhelper provides consistent status styling across the application with good coverage of common status terms and a sensible default fallback.crates/basilica-tui/src/ui/components/help.rs (1)
103-112: Clean centered popup positioning.The
centered_recthelper usingLayoutwithFlex::Centeris a clean solution for centering modals. This pattern could be reused for other overlays if needed.crates/basilica-tui/src/ui/screens/deployments.rs (2)
119-127: Good defensive bounds checking on selection index.The
.min(user_data.deployments.len().saturating_sub(1))pattern prevents out-of-bounds selection when the deployments list shrinks. This is a good defensive pattern.
44-91: Well-structured deployments table rendering.The table handles the empty state gracefully with a helpful placeholder message, applies appropriate status-based styling, and correctly formats GPU info with the CPU fallback.
crates/basilica-tui/src/app.rs (2)
536-543: Periodic refresh logic is commented out and has redundant checks.The auto-refresh is disabled (
self.refresh_data().awaitis commented out). Additionally, the logic checks both tick count and elapsed time, which is redundant since elapsed time is what matters. Consider simplifying when re-enabling.Is the auto-refresh intentionally disabled for this PR, or should it be enabled?
240-292: App initialization and run loop are well-structured.The constructor properly handles offline mode fallback, initializes all state, and the run loop cleanly separates rendering from event handling. The clone-before-borrow pattern for RenderContext avoids lock contention issues.
crates/basilica-tui/src/data/user.rs (1)
97-155: Parallel data fetching withtokio::join!is well implemented.Good use of parallel fetching for independent API calls. The clone of the client Arc is appropriate for concurrent access.
| pub async fn ssh_exec(host: &str, port: u16, user: &str, command: &str) -> Result<String> { | ||
| let output = Command::new("ssh") | ||
| .args([ | ||
| "-p", | ||
| &port.to_string(), | ||
| &format!("{}@{}", user, host), | ||
| command, | ||
| ]) | ||
| .output()?; | ||
|
|
||
| if output.status.success() { | ||
| Ok(String::from_utf8_lossy(&output.stdout).to_string()) | ||
| } else { | ||
| Err(anyhow::anyhow!( | ||
| "SSH command failed: {}", | ||
| String::from_utf8_lossy(&output.stderr) | ||
| )) | ||
| } | ||
| } |
There was a problem hiding this comment.
Blocking I/O in async function.
Command::output() is a blocking call that will block the async runtime thread. Use tokio::process::Command instead for async-safe subprocess execution.
🔎 Proposed fix using tokio::process
+use tokio::process::Command;
-use std::process::Command;
pub async fn ssh_exec(host: &str, port: u16, user: &str, command: &str) -> Result<String> {
let output = Command::new("ssh")
.args([
"-p",
&port.to_string(),
&format!("{}@{}", user, host),
command,
])
- .output()?;
+ .output()
+ .await?;
if output.status.success() {
Ok(String::from_utf8_lossy(&output.stdout).to_string())
} else {
Err(anyhow::anyhow!(
"SSH command failed: {}",
String::from_utf8_lossy(&output.stderr)
))
}
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In crates/basilica-tui/src/actions.rs around lines 26 to 44, the function uses
std::process::Command::output() which blocks the async runtime; replace it with
tokio::process::Command so the subprocess call is awaited asynchronously.
Specifically, import tokio::process::Command, build the command the same way,
call .output().await, propagate io errors into anyhow as before, and keep the
success/error handling converting stdout/stderr from bytes to UTF-8; ensure the
function signature and callers remain async-compatible.
| let output = Command::new("scp") | ||
| .args(["-P", &port.to_string(), "-r", &src, &dst]) | ||
| .output()?; |
There was a problem hiding this comment.
Same blocking I/O issue in scp_copy.
Apply the same fix using tokio::process::Command with .await for the output() call.
🤖 Prompt for AI Agents
In crates/basilica-tui/src/actions.rs around lines 67 to 69, the scp_copy
implementation is using blocking std::process::Command::output() which performs
blocking I/O; replace it with tokio::process::Command, construct the same
command and args, call .output().await (and propagate/handle the Result) so the
child is driven asynchronously by tokio; ensure you import
tokio::process::Command and adjust the function to be async if not already and
update error handling to await the output instead of using the blocking call.
| /// Copy text to clipboard | ||
| pub fn copy_to_clipboard(text: &str) -> Result<()> { | ||
| #[cfg(target_os = "macos")] | ||
| { | ||
| let mut child = Command::new("pbcopy") | ||
| .stdin(std::process::Stdio::piped()) | ||
| .spawn()?; | ||
| if let Some(stdin) = child.stdin.as_mut() { | ||
| use std::io::Write; | ||
| stdin.write_all(text.as_bytes())?; | ||
| } | ||
| child.wait()?; | ||
| } | ||
| #[cfg(target_os = "linux")] | ||
| { | ||
| let mut child = Command::new("xclip") | ||
| .args(["-selection", "clipboard"]) | ||
| .stdin(std::process::Stdio::piped()) | ||
| .spawn()?; | ||
| if let Some(stdin) = child.stdin.as_mut() { | ||
| use std::io::Write; | ||
| stdin.write_all(text.as_bytes())?; | ||
| } | ||
| child.wait()?; | ||
| } | ||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
Windows clipboard implementation is missing.
The Windows #[cfg] block is empty, causing copy_to_clipboard to silently succeed without actually copying anything on Windows. This could confuse users.
🔎 Proposed fix for Windows
+ #[cfg(target_os = "windows")]
+ {
+ let mut child = Command::new("clip")
+ .stdin(std::process::Stdio::piped())
+ .spawn()?;
+ if let Some(stdin) = child.stdin.as_mut() {
+ use std::io::Write;
+ stdin.write_all(text.as_bytes())?;
+ }
+ child.wait()?;
+ }
- #[cfg(target_os = "windows")]
- {
- // Not implemented
- }Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In crates/basilica-tui/src/actions.rs around lines 98 to 124, the Windows #[cfg]
block is empty so copy_to_clipboard silently does nothing on Windows; add a
#[cfg(target_os = "windows")] branch that spawns the standard Windows "clip"
command with stdin piped, write the provided text bytes into the child stdin,
and wait for the process to finish, mirroring the macOS/Linux implementations;
ensure you handle stdin via child.stdin.as_mut(), import std::io::Write where
needed, propagate errors via the existing Result, and keep the function
returning Ok(()) on success.
crates/basilica-tui/src/app.rs
Outdated
| fn get_current_list_len(&self) -> usize { | ||
| // TODO: Return actual list lengths based on current screen and data | ||
| match self.mode { | ||
| AppMode::User => match self.user_screen { | ||
| UserScreen::Rentals => 10, // placeholder | ||
| UserScreen::Marketplace => 20, // placeholder | ||
| UserScreen::Deployments => 5, // placeholder | ||
| _ => 0, | ||
| }, | ||
| AppMode::Miner => match self.miner_screen { | ||
| MinerScreen::Fleet => 8, // placeholder | ||
| MinerScreen::Nodes => 4, // placeholder | ||
| _ => 0, | ||
| }, | ||
| } | ||
| } |
There was a problem hiding this comment.
Placeholder list lengths will cause incorrect selection behavior.
get_current_list_len returns hardcoded values (10, 20, 5, etc.) instead of actual data lengths. This causes select_next/select_prev to wrap incorrectly when actual data differs from placeholders. For example, if there are only 2 rentals but this returns 10, users can select non-existent items.
Suggested fix pattern
fn get_current_list_len(&self) -> usize {
// This requires async access to user_data/miner_data
// Consider caching lengths or making this method async
match self.mode {
AppMode::User => match self.user_screen {
UserScreen::Rentals => {
// Would need cached length or blocking read
// For now, return a safe default
0
}
// ...
},
// ...
}
}A better approach would be to cache list lengths when data is refreshed, storing them in ScreenStates.
🤖 Prompt for AI Agents
crates/basilica-tui/src/app.rs lines 621-636: get_current_list_len currently
returns hardcoded placeholder lengths causing selection wrap bugs; replace these
constants by returning the actual list lengths (or safe cached lengths) for the
active screen — e.g., read the corresponding Vec/collection length from
self.user_data/self.miner_data or a ScreenStates cached_lengths map and return
that; ensure those cached lengths are updated whenever data is refreshed (on
fetch/update) and keep this method synchronous by relying on already-populated
fields so select_next/select_prev operate on real lengths.
| /// Save to default config location | ||
| pub fn save_default(&self) -> Result<()> { | ||
| if let Some(config_dir) = directories::ProjectDirs::from("ai", "basilica", "basilica-tui") { | ||
| let config_path = config_dir.config_dir().join("config.toml"); | ||
| self.save(&config_path)?; | ||
| } | ||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
Silent success when ProjectDirs returns None.
save_default() returns Ok(()) when ProjectDirs::from() returns None, silently not saving the config. Consider returning an error or logging a warning so users know their config wasn't persisted.
🔎 Proposed fix
pub fn save_default(&self) -> Result<()> {
- if let Some(config_dir) = directories::ProjectDirs::from("ai", "basilica", "basilica-tui") {
- let config_path = config_dir.config_dir().join("config.toml");
- self.save(&config_path)?;
- }
- Ok(())
+ let config_dir = directories::ProjectDirs::from("ai", "basilica", "basilica-tui")
+ .ok_or_else(|| anyhow::anyhow!("Could not determine config directory"))?;
+ let config_path = config_dir.config_dir().join("config.toml");
+ self.save(&config_path)
}📝 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.
| /// Save to default config location | |
| pub fn save_default(&self) -> Result<()> { | |
| if let Some(config_dir) = directories::ProjectDirs::from("ai", "basilica", "basilica-tui") { | |
| let config_path = config_dir.config_dir().join("config.toml"); | |
| self.save(&config_path)?; | |
| } | |
| Ok(()) | |
| } | |
| /// Save to default config location | |
| pub fn save_default(&self) -> Result<()> { | |
| let config_dir = directories::ProjectDirs::from("ai", "basilica", "basilica-tui") | |
| .ok_or_else(|| anyhow::anyhow!("Could not determine config directory"))?; | |
| let config_path = config_dir.config_dir().join("config.toml"); | |
| self.save(&config_path) | |
| } |
🤖 Prompt for AI Agents
In crates/basilica-tui/src/config.rs around lines 132 to 139, save_default()
currently silently returns Ok(()) when directories::ProjectDirs::from(...)
yields None, causing configs to not be saved without feedback; change the
function to return an Err when ProjectDirs::from(...) is None (or at minimum log
a warning before returning), e.g. map the None case into a meaningful error
using the crate's Result error type with a clear message like "could not
determine project config directory, config not saved" so callers are aware the
save failed.
| let logs = vec![ | ||
| ("14:32:15", "INFO", "node-001", "Health check passed"), | ||
| ("14:32:14", "INFO", "node-002", "Health check passed"), | ||
| ( | ||
| "14:32:10", | ||
| "INFO", | ||
| "validator", | ||
| "Validator discovery completed, found 3 validators", | ||
| ), | ||
| ("14:32:05", "INFO", "node-003", "Health check passed"), | ||
| ( | ||
| "14:32:00", | ||
| "WARN", | ||
| "node-004", | ||
| "High memory utilization: 95%", | ||
| ), | ||
| ("14:31:55", "INFO", "node-005", "Health check passed"), | ||
| ( | ||
| "14:31:50", | ||
| "ERROR", | ||
| "node-006", | ||
| "SSH connection failed: Connection refused", | ||
| ), | ||
| ("14:31:45", "INFO", "node-007", "Health check passed"), | ||
| ("14:31:40", "INFO", "node-008", "Health check passed"), | ||
| ( | ||
| "14:31:30", | ||
| "INFO", | ||
| "validator", | ||
| "Received assignment request from Val-001", | ||
| ), | ||
| ( | ||
| "14:31:25", | ||
| "INFO", | ||
| "node-001", | ||
| "GPU 0-3 assigned to Val-001", | ||
| ), | ||
| ( | ||
| "14:31:20", | ||
| "INFO", | ||
| "metrics", | ||
| "Metrics collection completed", | ||
| ), | ||
| ( | ||
| "14:31:15", | ||
| "INFO", | ||
| "node-002", | ||
| "Container started for rental abc123", | ||
| ), | ||
| ( | ||
| "14:31:10", | ||
| "INFO", | ||
| "validator", | ||
| "Node assignment successful", | ||
| ), | ||
| ( | ||
| "14:31:05", | ||
| "INFO", | ||
| "bittensor", | ||
| "Chain registration verified", | ||
| ), | ||
| ("14:31:00", "INFO", "main", "Miner tick completed"), | ||
| ( | ||
| "14:30:55", | ||
| "WARN", | ||
| "node-004", | ||
| "GPU 2 temperature high: 82°C", | ||
| ), | ||
| ("14:30:50", "INFO", "node-003", "Container logs rotated"), | ||
| ("14:30:45", "INFO", "validator", "Heartbeat sent to Val-002"), | ||
| ("14:30:40", "INFO", "metrics", "Prometheus scrape completed"), | ||
| ]; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Hardcoded log data duplicated across functions.
The sample log entries vector (72 lines) is duplicated verbatim in both render_logs and render_logs_ctx. Extract this to a module-level constant or helper function.
🔎 Proposed refactor
+const SAMPLE_LOGS: &[(&str, &str, &str, &str)] = &[
+ ("14:32:15", "INFO", "node-001", "Health check passed"),
+ ("14:32:14", "INFO", "node-002", "Health check passed"),
+ // ... rest of the log entries
+];
+
fn render_logs(frame: &mut Frame, app: &App, area: Rect) {
let theme = &app.theme;
-
- // Sample log entries
- let logs = vec![
- ("14:32:15", "INFO", "node-001", "Health check passed"),
- // ...
- ];
+ let logs = SAMPLE_LOGS;Apply the same change to render_logs_ctx.
Also applies to: 252-323
🤖 Prompt for AI Agents
In crates/basilica-tui/src/ui/screens/miner/logs.rs around lines 81-152 (and the
duplicate at 252-323), the same sample log Vec is duplicated in render_logs and
render_logs_ctx; extract that data into a single module-level definition (either
a static slice/array of (&'static str, &'static str, &'static str, &'static str)
or a helper fn like fn sample_logs() -> Vec<(...)>) and replace the inline
literals in both functions with a reference/call to the shared
constant/function; ensure visibility is crate-private, keep types consistent
with existing code, and update both locations so they use the single source of
truth.
| let mut state = TableState::default(); | ||
| state.select(Some(app.selected_index)); | ||
|
|
||
| frame.render_stateful_widget(table, area, &mut state); |
There was a problem hiding this comment.
Missing bounds check on selected_index for validators table.
If app.selected_index exceeds the hardcoded validators array length (5), the selection will be invalid. Unlike deployments.rs which clamps the index, this could cause visual glitches or incorrect highlighting.
Suggested fix
let mut state = TableState::default();
- state.select(Some(app.selected_index));
+ state.select(Some(app.selected_index.min(validators.len().saturating_sub(1))));Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In crates/basilica-tui/src/ui/screens/miner/validators.rs around lines 233 to
236, the code sets TableState::select(Some(app.selected_index)) without
validating that app.selected_index is within the validators list length; clamp
app.selected_index to the valid range before selecting (e.g., if
validators.is_empty() set selection to None, otherwise clamp to
validators.len()-1 using min), so the TableState.select call always receives a
valid index.
| let mut state = TableState::default(); | ||
| state.select(Some(ctx.selected_index)); | ||
|
|
||
| frame.render_stateful_widget(table, area, &mut state); |
There was a problem hiding this comment.
Same bounds check issue in render_validator_list_ctx.
Apply the same fix as the non-ctx variant.
Suggested fix
let mut state = TableState::default();
- state.select(Some(ctx.selected_index));
+ state.select(Some(ctx.selected_index.min(validators.len().saturating_sub(1))));📝 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 mut state = TableState::default(); | |
| state.select(Some(ctx.selected_index)); | |
| frame.render_stateful_widget(table, area, &mut state); | |
| let mut state = TableState::default(); | |
| state.select(Some(ctx.selected_index.min(validators.len().saturating_sub(1)))); | |
| frame.render_stateful_widget(table, area, &mut state); |
🤖 Prompt for AI Agents
In crates/basilica-tui/src/ui/screens/miner/validators.rs around lines 451 to
454, the code sets TableState::select(Some(ctx.selected_index)) without
validating that selected_index is within the current list bounds; replicate the
same bounds-check fix used in the non-ctx variant by checking the number of
rows/items first and only calling state.select(Some(idx)) when idx < items_len
(or set state.select(None) when out of range), ensuring you fetch the correct
length/rows variable for the ctx renderer and handle empty lists appropriately.
| let id_short = if rental.id.len() > 8 { | ||
| &rental.id[..8] | ||
| } else { | ||
| &rental.id | ||
| }; |
There was a problem hiding this comment.
Potential panic on non-ASCII rental IDs.
Slicing rental.id by byte index without checking UTF-8 character boundaries can panic if the ID contains multi-byte characters. Use .chars().take(8).collect::<String>() or a dedicated truncation helper that respects character boundaries.
🔎 Proposed fix
-let id_short = if rental.id.len() > 8 {
- &rental.id[..8]
-} else {
- &rental.id
-};
+let id_short = if rental.id.chars().count() > 8 {
+ rental.id.chars().take(8).collect::<String>()
+} else {
+ rental.id.clone()
+};Then use id_short directly instead of converting to string:
-Cell::from(id_short.to_string()),
+Cell::from(id_short),Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In crates/basilica-tui/src/ui/screens/rentals.rs around lines 79 to 83, the code
slices rental.id by byte index which can panic on multi-byte (non-ASCII)
characters; replace the byte-slice logic with a UTF-8-safe truncation such as
collecting the first 8 chars via rental.id.chars().take(8).collect::<String>()
(or a shared helper that truncates by character count), then use that resulting
String (or &str from it) for display instead of slicing by bytes.
| let image = if rental.container_image.len() > 25 { | ||
| format!("{}...", &rental.container_image[..22]) | ||
| } else { | ||
| rental.container_image.clone() |
There was a problem hiding this comment.
Potential panic on non-ASCII container images.
Similar to the ID truncation, slicing container_image by byte index can panic on multi-byte UTF-8 sequences.
🔎 Proposed fix
-let image = if rental.container_image.len() > 25 {
- format!("{}...", &rental.container_image[..22])
-} else {
- rental.container_image.clone()
-};
+let image = if rental.container_image.chars().count() > 25 {
+ let truncated: String = rental.container_image.chars().take(22).collect();
+ format!("{}...", truncated)
+} else {
+ rental.container_image.clone()
+};📝 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 image = if rental.container_image.len() > 25 { | |
| format!("{}...", &rental.container_image[..22]) | |
| } else { | |
| rental.container_image.clone() | |
| let image = if rental.container_image.chars().count() > 25 { | |
| let truncated: String = rental.container_image.chars().take(22).collect(); | |
| format!("{}...", truncated) | |
| } else { | |
| rental.container_image.clone() | |
| }; |
🤖 Prompt for AI Agents
In crates/basilica-tui/src/ui/screens/rentals.rs around lines 86 to 89, the code
slices rental.container_image by byte indices which can panic on multi-byte
UTF-8 characters; change the logic to truncate by characters (e.g., use
rental.container_image.chars().take(22).collect::<String>() and append "..."
when original contains more than 25 chars) or otherwise use a safe
character-aware truncation method, and keep the existing branch that clones the
full string when it is short enough.
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (8)
crates/basilica-tui/src/ui/screens/rentals.rs (2)
79-83: Potential panic on non-ASCII rental IDs.Slicing
rental.idby byte index without checking UTF-8 character boundaries can panic if the ID contains multi-byte characters. Use.chars().take(8).collect::<String>()or a dedicated truncation helper that respects character boundaries.🔎 Proposed fix
-let id_short = if rental.id.len() > 8 { - &rental.id[..8] -} else { - &rental.id -}; +let id_short = if rental.id.chars().count() > 8 { + rental.id.chars().take(8).collect::<String>() +} else { + rental.id.clone() +};Then use
id_shortdirectly:-Cell::from(id_short.to_string()), +Cell::from(id_short),
86-89: Potential panic on non-ASCII container images.Similar to the ID truncation, slicing
container_imageby byte index can panic on multi-byte UTF-8 sequences.🔎 Proposed fix
-let image = if rental.container_image.len() > 25 { - format!("{}...", &rental.container_image[..22]) -} else { - rental.container_image.clone() -}; +let image = if rental.container_image.chars().count() > 25 { + let truncated: String = rental.container_image.chars().take(22).collect(); + format!("{}...", truncated) +} else { + rental.container_image.clone() +};crates/basilica-tui/src/ui/screens/dashboard.rs (1)
97-116: Table highlighting won't work without TableState.The table is configured with
.row_highlight_style()and.highlight_symbol()(lines 113-114), butrender_widgetis used instead ofrender_stateful_widgetwith aTableState. Without aTableState, the row highlighting and selection symbol will not render even thoughrow_styleis set per-row.Suggested fix
+ let mut state = TableState::default(); + if !user_data.rentals.is_empty() { + state.select(Some(ctx.selected_index.min(user_data.rentals.len().saturating_sub(1)))); + } + - frame.render_widget(table, area); + frame.render_stateful_widget(table, area, &mut state);Also add
TableStateto imports at line 7:- widgets::{Block, Borders, Cell, List, ListItem, Paragraph, Row, Table}, + widgets::{Block, Borders, Cell, List, ListItem, Paragraph, Row, Table, TableState},crates/basilica-tui/src/ui/screens/marketplace.rs (1)
62-65: GPU type filter order is non-deterministic.
HashMapiteration order is arbitrary and may change between renders. This could cause the filter list to visually jump around, degrading user experience.Suggested fix: sort GPU types for consistent ordering
+ let mut sorted_gpu_types: Vec<_> = gpu_types.iter().collect(); + sorted_gpu_types.sort_by_key(|(gpu_type, _)| *gpu_type); + // Add GPU type options - for (gpu_type, count) in gpu_types.iter() { + for (gpu_type, count) in sorted_gpu_types { content.push(Line::from(format!(" ○ {} ({})", gpu_type, count))); }crates/basilica-tui/src/ui/screens/miner/validators.rs (2)
235-236: Missing bounds check on selected_index for validators table.If
app.selected_indexexceeds the hardcoded validators array length (5), the selection will be invalid. Clamp the index to the valid range.Suggested fix
let mut state = TableState::default(); - state.select(Some(app.selected_index)); + state.select(Some(app.selected_index.min(validators.len().saturating_sub(1))));
455-456: Same bounds check issue in context variant.Apply the same bounds checking fix as the non-ctx variant.
Suggested fix
let mut state = TableState::default(); - state.select(Some(ctx.selected_index)); + state.select(Some(ctx.selected_index.min(validators.len().saturating_sub(1))));crates/basilica-tui/src/app.rs (1)
625-641: Placeholder list lengths will cause incorrect selection behavior.This issue was flagged in a previous review. The hardcoded values (10, 20, 5, etc.) will cause
select_next/select_prevto wrap incorrectly when actual data differs from placeholders.crates/basilica-tui/src/data/user.rs (1)
119-147: Error handling overwrites previous errors.This issue was flagged in a previous review. When multiple parallel fetches fail, only the last error is preserved in
last_error. Consider accumulating errors or using aVec<String>.
🧹 Nitpick comments (12)
.github/workflows/ci.yml (1)
479-517: LGTM! Build job is well-structured and consistent.The
build-tuijob follows the established pattern for CLI tools in this workflow. The setup, build, test, and verification steps are appropriate, and the conditional logic correctly triggers on basilica-tui or workspace changes.Optional: Configure actionlint for custom runner labels
The static analysis hint flags
blacksmith-32vcpu-ubuntu-2404as an unknown label. This is a pre-existing issue affecting all jobs in this workflow. To suppress these warnings, consider adding anactionlint.yamlconfiguration file:self-hosted-runner: labels: - blacksmith-32vcpu-ubuntu-2404crates/basilica-tui/src/ui/screens/miner/fleet.rs (4)
161-234: Hardcoded sample data should use actual MinerData.The
nodesarray is duplicated verbatim in bothrender_node_grid(lines 161-234) andrender_node_grid_ctx(lines 429-502). This data should be pulled fromapp.miner_dataorctx.miner_datato display real node information.The
MinerDatastruct incrates/basilica-tui/src/data/miner.rsalready definesnodes: Vec<NodeInfo>with fields matching this sample data structure.🔎 Suggested approach
-fn render_node_grid(frame: &mut Frame, app: &App, area: Rect) { +fn render_node_grid(frame: &mut Frame, app: &App, miner_data: &crate::data::MinerData, area: Rect) { let theme = &app.theme; - // Sample node data - let nodes = [ - ( - "node-001", - ... - ), - ... - ]; + // Use actual node data from MinerData + let nodes = &miner_data.nodes; + if nodes.is_empty() { + // Render empty state + return; + } let rows: Vec<Row> = nodes .iter() .enumerate() - .map(|(i, (name, ip, gpu, status, gpu_util, mem_util, assigned))| { + .map(|(i, node)| { + let status_str = match node.status { + NodeStatus::Healthy => "Healthy", + NodeStatus::Warning => "Warning", + NodeStatus::Offline => "Offline", + _ => "Unknown", + }; let status_style = match node.status { - "Healthy" => theme.status_running(), + NodeStatus::Healthy => theme.status_running(), // ... };Also applies to: 429-502
119-155: Significant code duplication between rendering paths.
render_stat_boxandrender_stat_box_ctxare nearly identical, differing only in how the theme is obtained. The same applies to the stats bar and node grid functions. Consider extracting the core rendering logic to reduce duplication.🔎 Suggested consolidation
// Extract core logic that takes theme directly fn render_stat_box_impl( frame: &mut Frame, theme: &crate::ui::Theme, area: Rect, title: &str, value: &str, subtitle: Option<&str>, value_style: ratatui::style::Style, ) { // ... shared implementation } // Thin wrappers fn render_stat_box(frame: &mut Frame, app: &App, area: Rect, ...) { render_stat_box_impl(frame, &app.theme, area, ...) } fn render_stat_box_ctx(frame: &mut Frame, theme: &crate::ui::Theme, area: Rect, ...) { render_stat_box_impl(frame, theme, area, ...) }Also applies to: 390-424
119-128: Unusedappparameter — onlythemeis needed.The
appparameter is only used to extractapp.theme. Consider accepting&Themedirectly to match the signature ofrender_stat_box_ctxand reduce coupling.
63-116: Stats bar values are hardcoded.The stat values ("8", "24", "$12.50/hr", etc.) are static placeholders. These should be derived from
MinerData, which providestotal_gpus(),healthy_nodes(), andearningsdata.🔎 Example integration
fn render_stats_bar(frame: &mut Frame, app: &App, miner_data: &MinerData, area: Rect) { // ... render_stat_box( frame, app, chunks[0], "Nodes", &miner_data.nodes.len().to_string(), Some(&format!("{} healthy", miner_data.healthy_nodes())), theme.text_success(), ); // ... }crates/basilica-tui/src/app.rs (3)
700-703: ConsiderVecDequefor the notification queue.Using
Vec::remove(0)is O(n). Since notifications behave as a FIFO queue with a fixed max size,VecDequewould provide O(1) operations for both push and pop.
320-351: Cloning data on every render frame may become expensive.The loop clones
user_data,miner_data,theme,screens, andnotificationson every iteration. This is reasonable for keeping locks short, but if data grows large (many rentals/nodes), consider caching or usingArc<T>for immutable data to reduce allocations.
540-548:is_multiple_ofis stable and widely available—the dual refresh triggers are a matter of code clarity.
u64::is_multiple_ofis stable since Rust 1.32.0 and does not require any nightly features. However, the refresh logic combines a tick-based check with an elapsed-time check, which is somewhat redundant. Using the elapsed-time trigger alone would be simpler and more reliable:- let refresh_interval_ticks = (self.config.refresh.balance * 1000) / 250; - if self.tick_count.is_multiple_of(refresh_interval_ticks) - && self.last_refresh.elapsed().as_secs() >= self.config.refresh.balance - { + if self.last_refresh.elapsed().as_secs() >= self.config.refresh.balance { self.last_refresh = std::time::Instant::now(); // Auto-refresh data in background // self.refresh_data().await; }crates/basilica-tui/src/data/streams.rs (2)
92-120: Spawned streaming tasks are detached — consider trackingJoinHandle.The
tokio::spawnresult (aJoinHandle) is dropped, making it impossible to await task completion or detect panics. If graceful shutdown or error propagation is needed later, store the handle.🔎 Suggested improvement
struct LogStreamHandle { id: String, cancel_tx: tokio::sync::oneshot::Sender<()>, task_handle: tokio::task::JoinHandle<()>, // Track the spawned task }
248-253:EventStream::pushuses O(n) removal — preferVecDeque.Same issue as the notification queue:
remove(0)on aVecis O(n).VecDeque::push_back+pop_frontwould be O(1).crates/basilica-tui/src/data/miner.rs (1)
54-73: MetricsHistory uses O(n) removal — preferVecDeque.
push_gpu_util,push_memory_util, andpush_revenueall useremove(0)to maintain a sliding window. WithVecDeque, this becomes O(1).🔎 Example refactor
+use std::collections::VecDeque; pub struct MetricsHistory { - pub gpu_utilization: Vec<f64>, - pub memory_utilization: Vec<f64>, - pub daily_revenue: Vec<f64>, - pub request_rate: Vec<f64>, + pub gpu_utilization: VecDeque<f64>, + pub memory_utilization: VecDeque<f64>, + pub daily_revenue: VecDeque<f64>, + pub request_rate: VecDeque<f64>, pub max_samples: usize, } impl MetricsHistory { pub fn push_gpu_util(&mut self, value: f64) { if self.gpu_utilization.len() >= self.max_samples { - self.gpu_utilization.remove(0); + self.gpu_utilization.pop_front(); } - self.gpu_utilization.push(value); + self.gpu_utilization.push_back(value); } // similar for other push_* methods }crates/basilica-tui/src/data/user.rs (1)
294-300: Balance string parsing is fragile.The parsing assumes
response.balanceis formatted as"12.5 TAO"and extracts the first token. If the API returns just a number or a different format, this silently defaults to 0.0. Consider logging unexpected formats for debugging.🔎 Suggested improvement
let balance_tao: f64 = response .balance .split_whitespace() .next() - .and_then(|s| s.parse().ok()) - .unwrap_or(0.0); + .and_then(|s| s.parse().ok()) + .unwrap_or_else(|| { + tracing::warn!("Unexpected balance format: {:?}", response.balance); + 0.0 + });
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
.github/workflows/ci.ymlcrates/basilica-tui/src/app.rscrates/basilica-tui/src/data/miner.rscrates/basilica-tui/src/data/streams.rscrates/basilica-tui/src/data/user.rscrates/basilica-tui/src/events/mod.rscrates/basilica-tui/src/ui/components/mod.rscrates/basilica-tui/src/ui/screens/billing.rscrates/basilica-tui/src/ui/screens/dashboard.rscrates/basilica-tui/src/ui/screens/marketplace.rscrates/basilica-tui/src/ui/screens/miner/earnings.rscrates/basilica-tui/src/ui/screens/miner/fleet.rscrates/basilica-tui/src/ui/screens/miner/mod.rscrates/basilica-tui/src/ui/screens/miner/nodes.rscrates/basilica-tui/src/ui/screens/miner/validators.rscrates/basilica-tui/src/ui/screens/rentals.rscrates/basilica-tui/src/ui/widgets/mod.rs
🚧 Files skipped from review as they are similar to previous changes (3)
- crates/basilica-tui/src/ui/screens/billing.rs
- crates/basilica-tui/src/ui/screens/miner/mod.rs
- crates/basilica-tui/src/ui/widgets/mod.rs
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-13T15:34:46.544Z
Learnt from: epappas
Repo: one-covenant/basilica PR: 184
File: crates/basilica-validator/src/rental/billing.rs:129-130
Timestamp: 2025-10-13T15:34:46.544Z
Learning: In the basilica-validator crate, rental_id is a String type (not UUID) throughout the system - in RentalInfo struct, database rentals.id column, and all rental operations. The persistence layer methods should accept &str for rental_id parameters, not &Uuid, since they immediately convert to String for SQL binding anyway.
Applied to files:
crates/basilica-tui/src/ui/screens/rentals.rscrates/basilica-tui/src/data/user.rs
🧬 Code graph analysis (3)
crates/basilica-tui/src/ui/screens/miner/fleet.rs (6)
crates/basilica-tui/src/ui/theme.rs (5)
text(75-77)header(142-147)default(194-196)text_muted(80-82)text_accent(93-95)crates/basilica-tui/src/ui/widgets/gauge.rs (1)
mini_bar(60-65)crates/basilica-tui/src/ui/screens/miner/nodes.rs (1)
render(15-46)crates/basilica-tui/src/ui/components/header.rs (2)
header_height(149-151)render_header(14-80)crates/basilica-tui/src/ui/components/footer.rs (2)
footer_height(130-132)render_footer(13-67)crates/basilica-tui/src/app.rs (1)
new(247-297)
crates/basilica-tui/src/ui/screens/miner/nodes.rs (2)
crates/basilica-tui/src/ui/theme.rs (6)
text(75-77)header(142-147)default(194-196)text_accent(93-95)text_bold(85-90)status_running(163-165)crates/basilica-tui/src/ui/components/header.rs (2)
render_header(14-80)render_header_ctx(83-146)
crates/basilica-tui/src/ui/components/mod.rs (1)
crates/basilica-tui/src/ui/theme.rs (1)
header(142-147)
🪛 actionlint (1.7.9)
.github/workflows/ci.yml
481-481: label "blacksmith-32vcpu-ubuntu-2404" is unknown. available labels are "windows-latest", "windows-latest-8-cores", "windows-2025", "windows-2022", "windows-11-arm", "ubuntu-slim", "ubuntu-latest", "ubuntu-latest-4-cores", "ubuntu-latest-8-cores", "ubuntu-latest-16-cores", "ubuntu-24.04", "ubuntu-24.04-arm", "ubuntu-22.04", "ubuntu-22.04-arm", "macos-latest", "macos-latest-xl", "macos-latest-xlarge", "macos-latest-large", "macos-26-xlarge", "macos-26", "macos-15-intel", "macos-15-xlarge", "macos-15-large", "macos-15", "macos-14-xl", "macos-14-xlarge", "macos-14-large", "macos-14", "macos-13-xl", "macos-13-xlarge", "macos-13-large", "macos-13", "self-hosted", "x64", "arm", "arm64", "linux", "macos", "windows". if it is a custom label for self-hosted runner, set list of labels in actionlint.yaml config file
(runner-label)
🔇 Additional comments (14)
.github/workflows/ci.yml (3)
44-44: LGTM! Changes detection properly configured.The output declaration and path filters for basilica-tui follow the established pattern. Including
basilica-sdkin the path filter (line 90) appropriately captures the dependency relationship.Also applies to: 88-93
195-196: LGTM! Clippy integration is consistent.The clippy configuration for basilica-tui matches the pattern used for other packages, properly enforcing warnings as errors.
818-818: LGTM! CI success checks properly updated.The
build-tuijob is correctly integrated into theci-successfinal status check, ensuring the workflow properly tracks success/failure of the TUI build alongside other components.Also applies to: 833-833
crates/basilica-tui/src/ui/components/mod.rs (1)
1-7: LGTM! Clean module organization.The module structure is well-organized with clear submodule declarations for shared UI components. The
#![allow(dead_code)]attribute is appropriate for new code under development.crates/basilica-tui/src/events/mod.rs (1)
33-81: LGTM! Well-structured event handling system.The event handler design is solid:
- Properly uses
tokio::select!to multiplex periodic ticks and terminal events- The
_txfield correctly keeps the channel alive while the task runs- Clean error handling with appropriate channel closure detection
- Event translation from Crossterm to the application's Event enum is straightforward
crates/basilica-tui/src/ui/screens/rentals.rs (1)
135-141: LGTM! Proper bounds checking for table selection.The TableState selection correctly clamps the index to valid bounds using
saturating_sub(1)and handles the empty rentals case.crates/basilica-tui/src/ui/screens/marketplace.rs (1)
171-177: LGTM! Proper TableState usage with bounds checking.The table correctly uses
render_stateful_widgetwith a properly initializedTableState, and the selection index is clamped to valid bounds.crates/basilica-tui/src/app.rs (1)
706-730: Action stubs are well-documented.The stub implementations for
ssh_into_rental,stop_rental,start_rental, anddelete_deploymentappropriately notify users that functionality is not yet implemented. Good placeholder pattern.crates/basilica-tui/src/data/streams.rs (2)
66-76: LGTM — LogStreamManager initialization is clean.The unbounded channel setup and struct initialization are straightforward.
203-207: Default implementation hardcodes production API URL.
Default::default()uses"https://api.basilica.ai". Ensure callers usenew(api_url)with the configured URL instead of relying onDefaultif a custom endpoint is needed.crates/basilica-tui/src/data/miner.rs (2)
179-214: Refresh and poll methods are stubs — acceptable for scaffolding.The
refresh_all,refresh_nodes, andpoll_metricsmethods are clearly marked with TODO comments and simulate behavior. The simulated metrics inpoll_metricsare appropriate for UI development.
234-248: Aggregation methods handle empty collections correctly.
avg_gpu_utilizationandavg_memory_utilizationproperly guard against division by zero withis_empty()checks.crates/basilica-tui/src/data/user.rs (2)
339-345: Status filtering uses string matching.
active_rentals_countchecks if status contains "Running" or "Active". Sincestatusis populated viaformat!("{:?}", r.state)(Debug formatting of an enum), this could break if enum variant names change. Consider matching on a proper enum if available.
99-117: Parallel data fetching pattern is well-implemented.Using
tokio::join!with cloned client references for concurrent API calls is a good pattern for reducing latency during refresh.
| /// Fetch rentals from API | ||
| async fn fetch_rentals(&mut self, client: &Arc<BasilicaClient>) -> Result<()> { | ||
| self.loading.rentals = true; | ||
|
|
||
| match client.list_rentals(None).await { | ||
| Ok(response) => { | ||
| self.rentals = response | ||
| .rentals | ||
| .into_iter() | ||
| .map(|r| { | ||
| // Parse created_at to compute uptime | ||
| let uptime_minutes = chrono::DateTime::parse_from_rfc3339(&r.created_at) | ||
| .map(|start| { | ||
| let now = chrono::Utc::now(); | ||
| let duration = now.signed_duration_since(start); | ||
| duration.num_minutes().max(0) as u64 | ||
| }) | ||
| .unwrap_or(0); | ||
|
|
||
| // Extract GPU info from gpu_specs | ||
| let (gpu_type, gpu_count) = if let Some(spec) = r.gpu_specs.first() { | ||
| (spec.name.clone(), r.gpu_specs.len() as u32) | ||
| } else { | ||
| ("Unknown".to_string(), 0) | ||
| }; | ||
|
|
||
| RentalInfo { | ||
| id: r.rental_id, | ||
| gpu_type, | ||
| gpu_count, | ||
| status: format!("{:?}", r.state), | ||
| uptime_minutes, | ||
| cost: 0.0, // TODO: Get from usage API | ||
| container_image: r.container_image, | ||
| ssh_host: None, // Need to fetch from rental details | ||
| ssh_port: None, | ||
| ssh_user: None, | ||
| } | ||
| }) | ||
| .collect(); | ||
| self.loading.rentals = false; | ||
| Ok(()) | ||
| } | ||
| Err(e) => { | ||
| self.loading.rentals = false; | ||
| Err(anyhow::anyhow!("Failed to list rentals: {}", e)) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Significant code duplication between instance methods and standalone functions.
fetch_rentals and fetch_rentals_data contain nearly identical logic. The same applies to offerings, deployments, and balance. The instance methods could delegate to the standalone functions to eliminate duplication.
🔎 Suggested consolidation
async fn fetch_rentals(&mut self, client: &Arc<BasilicaClient>) -> Result<()> {
self.loading.rentals = true;
- match client.list_rentals(None).await {
- Ok(response) => {
- self.rentals = response
- .rentals
- .into_iter()
- .map(|r| {
- // ... duplicate mapping logic
- })
- .collect();
- self.loading.rentals = false;
- Ok(())
- }
- Err(e) => {
- self.loading.rentals = false;
- Err(anyhow::anyhow!("Failed to list rentals: {}", e))
- }
- }
+ let result = fetch_rentals_data(client).await;
+ self.loading.rentals = false;
+ match result {
+ Ok(rentals) => {
+ self.rentals = rentals;
+ Ok(())
+ }
+ Err(e) => Err(e),
+ }
}Also applies to: 359-395
| fn render_payment_history(frame: &mut Frame, app: &App, area: Rect) { | ||
| let theme = &app.theme; | ||
|
|
||
| // Sample payment data | ||
| let payments = [ | ||
| ( | ||
| "2024-01-15", | ||
| "Val-001", | ||
| "24h rental", | ||
| "+$280.00", | ||
| "Completed", | ||
| ), | ||
| ( | ||
| "2024-01-14", | ||
| "Val-002", | ||
| "12h rental", | ||
| "+$135.00", | ||
| "Completed", | ||
| ), | ||
| ("2024-01-14", "Val-001", "8h rental", "+$95.00", "Completed"), | ||
| ("2024-01-13", "Val-003", "4h rental", "+$45.00", "Completed"), | ||
| ( | ||
| "2024-01-13", | ||
| "Val-001", | ||
| "16h rental", | ||
| "+$190.00", | ||
| "Completed", | ||
| ), | ||
| ( | ||
| "2024-01-12", | ||
| "Val-002", | ||
| "24h rental", | ||
| "+$310.00", | ||
| "Completed", | ||
| ), | ||
| ( | ||
| "2024-01-11", | ||
| "Val-001", | ||
| "12h rental", | ||
| "+$140.00", | ||
| "Completed", | ||
| ), | ||
| ("2024-01-10", "Val-003", "8h rental", "+$92.00", "Completed"), | ||
| ]; | ||
|
|
||
| let rows: Vec<Row> = payments | ||
| .iter() | ||
| .enumerate() | ||
| .map(|(i, (date, validator, desc, amount, status))| { | ||
| let row_style = if i == app.selected_index { | ||
| theme.selected_row() | ||
| } else { | ||
| theme.text() | ||
| }; | ||
|
|
||
| Row::new(vec![ | ||
| Cell::from(*date).style(theme.text_muted()), | ||
| Cell::from(*validator).style(theme.text_accent()), | ||
| Cell::from(*desc), | ||
| Cell::from(*amount).style(theme.text_success()), | ||
| Cell::from(*status).style(theme.text_muted()), | ||
| ]) | ||
| .style(row_style) | ||
| }) | ||
| .collect(); | ||
|
|
||
| let header = Row::new(vec!["Date", "Validator", "Description", "Amount", "Status"]) | ||
| .style(theme.header()); | ||
|
|
||
| let table = Table::new( | ||
| rows, | ||
| [ | ||
| Constraint::Length(12), | ||
| Constraint::Length(10), | ||
| Constraint::Min(15), | ||
| Constraint::Length(12), | ||
| Constraint::Length(12), | ||
| ], | ||
| ) | ||
| .header(header) | ||
| .block( | ||
| Block::default() | ||
| .borders(Borders::ALL) | ||
| .border_style(theme.border()) | ||
| .title(Span::styled(" Payment History ", theme.block_title())), | ||
| ) | ||
| .row_highlight_style(theme.selected_row()); | ||
|
|
||
| frame.render_widget(table, area); | ||
| } |
There was a problem hiding this comment.
Table highlighting won't work without TableState.
The payment history table is configured with .row_highlight_style() at line 258, but uses render_widget instead of render_stateful_widget. Without a TableState, the selection highlighting will not be rendered.
Suggested fix
+ let mut state = TableState::default();
+ state.select(Some(app.selected_index.min(payments.len().saturating_sub(1))));
+
- frame.render_widget(table, area);
+ frame.render_stateful_widget(table, area, &mut state);Also add TableState to imports at line 7:
- widgets::{Block, Borders, Cell, Paragraph, Row, Table},
+ widgets::{Block, Borders, Cell, Paragraph, Row, Table, TableState},📝 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.
| fn render_payment_history(frame: &mut Frame, app: &App, area: Rect) { | |
| let theme = &app.theme; | |
| // Sample payment data | |
| let payments = [ | |
| ( | |
| "2024-01-15", | |
| "Val-001", | |
| "24h rental", | |
| "+$280.00", | |
| "Completed", | |
| ), | |
| ( | |
| "2024-01-14", | |
| "Val-002", | |
| "12h rental", | |
| "+$135.00", | |
| "Completed", | |
| ), | |
| ("2024-01-14", "Val-001", "8h rental", "+$95.00", "Completed"), | |
| ("2024-01-13", "Val-003", "4h rental", "+$45.00", "Completed"), | |
| ( | |
| "2024-01-13", | |
| "Val-001", | |
| "16h rental", | |
| "+$190.00", | |
| "Completed", | |
| ), | |
| ( | |
| "2024-01-12", | |
| "Val-002", | |
| "24h rental", | |
| "+$310.00", | |
| "Completed", | |
| ), | |
| ( | |
| "2024-01-11", | |
| "Val-001", | |
| "12h rental", | |
| "+$140.00", | |
| "Completed", | |
| ), | |
| ("2024-01-10", "Val-003", "8h rental", "+$92.00", "Completed"), | |
| ]; | |
| let rows: Vec<Row> = payments | |
| .iter() | |
| .enumerate() | |
| .map(|(i, (date, validator, desc, amount, status))| { | |
| let row_style = if i == app.selected_index { | |
| theme.selected_row() | |
| } else { | |
| theme.text() | |
| }; | |
| Row::new(vec![ | |
| Cell::from(*date).style(theme.text_muted()), | |
| Cell::from(*validator).style(theme.text_accent()), | |
| Cell::from(*desc), | |
| Cell::from(*amount).style(theme.text_success()), | |
| Cell::from(*status).style(theme.text_muted()), | |
| ]) | |
| .style(row_style) | |
| }) | |
| .collect(); | |
| let header = Row::new(vec!["Date", "Validator", "Description", "Amount", "Status"]) | |
| .style(theme.header()); | |
| let table = Table::new( | |
| rows, | |
| [ | |
| Constraint::Length(12), | |
| Constraint::Length(10), | |
| Constraint::Min(15), | |
| Constraint::Length(12), | |
| Constraint::Length(12), | |
| ], | |
| ) | |
| .header(header) | |
| .block( | |
| Block::default() | |
| .borders(Borders::ALL) | |
| .border_style(theme.border()) | |
| .title(Span::styled(" Payment History ", theme.block_title())), | |
| ) | |
| .row_highlight_style(theme.selected_row()); | |
| frame.render_widget(table, area); | |
| } | |
| fn render_payment_history(frame: &mut Frame, app: &App, area: Rect) { | |
| let theme = &app.theme; | |
| // Sample payment data | |
| let payments = [ | |
| ( | |
| "2024-01-15", | |
| "Val-001", | |
| "24h rental", | |
| "+$280.00", | |
| "Completed", | |
| ), | |
| ( | |
| "2024-01-14", | |
| "Val-002", | |
| "12h rental", | |
| "+$135.00", | |
| "Completed", | |
| ), | |
| ("2024-01-14", "Val-001", "8h rental", "+$95.00", "Completed"), | |
| ("2024-01-13", "Val-003", "4h rental", "+$45.00", "Completed"), | |
| ( | |
| "2024-01-13", | |
| "Val-001", | |
| "16h rental", | |
| "+$190.00", | |
| "Completed", | |
| ), | |
| ( | |
| "2024-01-12", | |
| "Val-002", | |
| "24h rental", | |
| "+$310.00", | |
| "Completed", | |
| ), | |
| ( | |
| "2024-01-11", | |
| "Val-001", | |
| "12h rental", | |
| "+$140.00", | |
| "Completed", | |
| ), | |
| ("2024-01-10", "Val-003", "8h rental", "+$92.00", "Completed"), | |
| ]; | |
| let rows: Vec<Row> = payments | |
| .iter() | |
| .enumerate() | |
| .map(|(i, (date, validator, desc, amount, status))| { | |
| let row_style = if i == app.selected_index { | |
| theme.selected_row() | |
| } else { | |
| theme.text() | |
| }; | |
| Row::new(vec![ | |
| Cell::from(*date).style(theme.text_muted()), | |
| Cell::from(*validator).style(theme.text_accent()), | |
| Cell::from(*desc), | |
| Cell::from(*amount).style(theme.text_success()), | |
| Cell::from(*status).style(theme.text_muted()), | |
| ]) | |
| .style(row_style) | |
| }) | |
| .collect(); | |
| let header = Row::new(vec!["Date", "Validator", "Description", "Amount", "Status"]) | |
| .style(theme.header()); | |
| let table = Table::new( | |
| rows, | |
| [ | |
| Constraint::Length(12), | |
| Constraint::Length(10), | |
| Constraint::Min(15), | |
| Constraint::Length(12), | |
| Constraint::Length(12), | |
| ], | |
| ) | |
| .header(header) | |
| .block( | |
| Block::default() | |
| .borders(Borders::ALL) | |
| .border_style(theme.border()) | |
| .title(Span::styled(" Payment History ", theme.block_title())), | |
| ) | |
| .row_highlight_style(theme.selected_row()); | |
| let mut state = TableState::default(); | |
| state.select(Some(app.selected_index.min(payments.len().saturating_sub(1)))); | |
| frame.render_stateful_widget(table, area, &mut state); | |
| } |
🤖 Prompt for AI Agents
In crates/basilica-tui/src/ui/screens/miner/earnings.rs around lines 172 to 261,
the table uses .row_highlight_style() but is rendered with frame.render_widget
so selection highlighting never shows; add TableState to the imports, create a
mutable TableState instance before building the Table, set its selection from
app.selected_index (e.g. state.select(Some(app.selected_index)) or None when out
of range), and replace frame.render_widget(table, area) with
frame.render_stateful_widget(table, area, &mut state) so the row_highlight_style
can take effect.
| fn render_payment_history_ctx(frame: &mut Frame, ctx: &RenderContext, area: Rect) { | ||
| let theme = ctx.theme; | ||
|
|
||
| let payments = [ | ||
| ( | ||
| "2024-01-15", | ||
| "Val-001", | ||
| "24h rental", | ||
| "+$280.00", | ||
| "Completed", | ||
| ), | ||
| ( | ||
| "2024-01-14", | ||
| "Val-002", | ||
| "12h rental", | ||
| "+$135.00", | ||
| "Completed", | ||
| ), | ||
| ("2024-01-14", "Val-001", "8h rental", "+$95.00", "Completed"), | ||
| ("2024-01-13", "Val-003", "4h rental", "+$45.00", "Completed"), | ||
| ( | ||
| "2024-01-13", | ||
| "Val-001", | ||
| "16h rental", | ||
| "+$190.00", | ||
| "Completed", | ||
| ), | ||
| ( | ||
| "2024-01-12", | ||
| "Val-002", | ||
| "24h rental", | ||
| "+$310.00", | ||
| "Completed", | ||
| ), | ||
| ( | ||
| "2024-01-11", | ||
| "Val-001", | ||
| "12h rental", | ||
| "+$140.00", | ||
| "Completed", | ||
| ), | ||
| ("2024-01-10", "Val-003", "8h rental", "+$92.00", "Completed"), | ||
| ]; | ||
|
|
||
| let rows: Vec<Row> = payments | ||
| .iter() | ||
| .enumerate() | ||
| .map(|(i, (date, validator, desc, amount, status))| { | ||
| let row_style = if i == ctx.selected_index { | ||
| theme.selected_row() | ||
| } else { | ||
| theme.text() | ||
| }; | ||
|
|
||
| Row::new(vec![ | ||
| Cell::from(*date).style(theme.text_muted()), | ||
| Cell::from(*validator).style(theme.text_accent()), | ||
| Cell::from(*desc), | ||
| Cell::from(*amount).style(theme.text_success()), | ||
| Cell::from(*status).style(theme.text_muted()), | ||
| ]) | ||
| .style(row_style) | ||
| }) | ||
| .collect(); | ||
|
|
||
| let header_row = Row::new(vec!["Date", "Validator", "Description", "Amount", "Status"]) | ||
| .style(theme.header()); | ||
|
|
||
| let table = Table::new( | ||
| rows, | ||
| [ | ||
| Constraint::Length(12), | ||
| Constraint::Length(10), | ||
| Constraint::Min(15), | ||
| Constraint::Length(12), | ||
| Constraint::Length(12), | ||
| ], | ||
| ) | ||
| .header(header_row) | ||
| .block( | ||
| Block::default() | ||
| .borders(Borders::ALL) | ||
| .border_style(theme.border()) | ||
| .title(Span::styled(" Payment History ", theme.block_title())), | ||
| ) | ||
| .row_highlight_style(theme.selected_row()); | ||
|
|
||
| frame.render_widget(table, area); | ||
| } |
There was a problem hiding this comment.
Same TableState issue in context-aware variant.
The context-aware payment history function has the same issue: it configures row highlighting but doesn't use render_stateful_widget.
Suggested fix
+ let mut state = TableState::default();
+ state.select(Some(ctx.selected_index.min(payments.len().saturating_sub(1))));
+
- frame.render_widget(table, area);
+ frame.render_stateful_widget(table, area, &mut state);📝 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.
| fn render_payment_history_ctx(frame: &mut Frame, ctx: &RenderContext, area: Rect) { | |
| let theme = ctx.theme; | |
| let payments = [ | |
| ( | |
| "2024-01-15", | |
| "Val-001", | |
| "24h rental", | |
| "+$280.00", | |
| "Completed", | |
| ), | |
| ( | |
| "2024-01-14", | |
| "Val-002", | |
| "12h rental", | |
| "+$135.00", | |
| "Completed", | |
| ), | |
| ("2024-01-14", "Val-001", "8h rental", "+$95.00", "Completed"), | |
| ("2024-01-13", "Val-003", "4h rental", "+$45.00", "Completed"), | |
| ( | |
| "2024-01-13", | |
| "Val-001", | |
| "16h rental", | |
| "+$190.00", | |
| "Completed", | |
| ), | |
| ( | |
| "2024-01-12", | |
| "Val-002", | |
| "24h rental", | |
| "+$310.00", | |
| "Completed", | |
| ), | |
| ( | |
| "2024-01-11", | |
| "Val-001", | |
| "12h rental", | |
| "+$140.00", | |
| "Completed", | |
| ), | |
| ("2024-01-10", "Val-003", "8h rental", "+$92.00", "Completed"), | |
| ]; | |
| let rows: Vec<Row> = payments | |
| .iter() | |
| .enumerate() | |
| .map(|(i, (date, validator, desc, amount, status))| { | |
| let row_style = if i == ctx.selected_index { | |
| theme.selected_row() | |
| } else { | |
| theme.text() | |
| }; | |
| Row::new(vec![ | |
| Cell::from(*date).style(theme.text_muted()), | |
| Cell::from(*validator).style(theme.text_accent()), | |
| Cell::from(*desc), | |
| Cell::from(*amount).style(theme.text_success()), | |
| Cell::from(*status).style(theme.text_muted()), | |
| ]) | |
| .style(row_style) | |
| }) | |
| .collect(); | |
| let header_row = Row::new(vec!["Date", "Validator", "Description", "Amount", "Status"]) | |
| .style(theme.header()); | |
| let table = Table::new( | |
| rows, | |
| [ | |
| Constraint::Length(12), | |
| Constraint::Length(10), | |
| Constraint::Min(15), | |
| Constraint::Length(12), | |
| Constraint::Length(12), | |
| ], | |
| ) | |
| .header(header_row) | |
| .block( | |
| Block::default() | |
| .borders(Borders::ALL) | |
| .border_style(theme.border()) | |
| .title(Span::styled(" Payment History ", theme.block_title())), | |
| ) | |
| .row_highlight_style(theme.selected_row()); | |
| frame.render_widget(table, area); | |
| } | |
| fn render_payment_history_ctx(frame: &mut Frame, ctx: &RenderContext, area: Rect) { | |
| let theme = ctx.theme; | |
| let payments = [ | |
| ( | |
| "2024-01-15", | |
| "Val-001", | |
| "24h rental", | |
| "+$280.00", | |
| "Completed", | |
| ), | |
| ( | |
| "2024-01-14", | |
| "Val-002", | |
| "12h rental", | |
| "+$135.00", | |
| "Completed", | |
| ), | |
| ("2024-01-14", "Val-001", "8h rental", "+$95.00", "Completed"), | |
| ("2024-01-13", "Val-003", "4h rental", "+$45.00", "Completed"), | |
| ( | |
| "2024-01-13", | |
| "Val-001", | |
| "16h rental", | |
| "+$190.00", | |
| "Completed", | |
| ), | |
| ( | |
| "2024-01-12", | |
| "Val-002", | |
| "24h rental", | |
| "+$310.00", | |
| "Completed", | |
| ), | |
| ( | |
| "2024-01-11", | |
| "Val-001", | |
| "12h rental", | |
| "+$140.00", | |
| "Completed", | |
| ), | |
| ("2024-01-10", "Val-003", "8h rental", "+$92.00", "Completed"), | |
| ]; | |
| let rows: Vec<Row> = payments | |
| .iter() | |
| .enumerate() | |
| .map(|(i, (date, validator, desc, amount, status))| { | |
| let row_style = if i == ctx.selected_index { | |
| theme.selected_row() | |
| } else { | |
| theme.text() | |
| }; | |
| Row::new(vec![ | |
| Cell::from(*date).style(theme.text_muted()), | |
| Cell::from(*validator).style(theme.text_accent()), | |
| Cell::from(*desc), | |
| Cell::from(*amount).style(theme.text_success()), | |
| Cell::from(*status).style(theme.text_muted()), | |
| ]) | |
| .style(row_style) | |
| }) | |
| .collect(); | |
| let header_row = Row::new(vec!["Date", "Validator", "Description", "Amount", "Status"]) | |
| .style(theme.header()); | |
| let table = Table::new( | |
| rows, | |
| [ | |
| Constraint::Length(12), | |
| Constraint::Length(10), | |
| Constraint::Min(15), | |
| Constraint::Length(12), | |
| Constraint::Length(12), | |
| ], | |
| ) | |
| .header(header_row) | |
| .block( | |
| Block::default() | |
| .borders(Borders::ALL) | |
| .border_style(theme.border()) | |
| .title(Span::styled(" Payment History ", theme.block_title())), | |
| ) | |
| .row_highlight_style(theme.selected_row()); | |
| let mut state = TableState::default(); | |
| state.select(Some(ctx.selected_index.min(payments.len().saturating_sub(1)))); | |
| frame.render_stateful_widget(table, area, &mut state); | |
| } |
🤖 Prompt for AI Agents
In crates/basilica-tui/src/ui/screens/miner/earnings.rs around lines 410 to 498,
the context-aware payment history table configures row highlighting but calls
frame.render_widget instead of frame.render_stateful_widget so the
selection/highlight doesn't work; fix by creating a mutable
tui::widgets::TableState, set its selected index to Some(ctx.selected_index) (or
None when out of range), and call frame.render_stateful_widget(table, area, &mut
table_state) instead of frame.render_widget(table, area) so the table uses the
stateful selection highlighting.
| let mut state = TableState::default(); | ||
| state.select(Some(app.selected_index)); |
There was a problem hiding this comment.
Missing bounds check on selected_index.
If app.selected_index exceeds the hardcoded nodes array length (8), the selection could be invalid. Clamp the index to the valid range.
Suggested fix
let mut state = TableState::default();
- state.select(Some(app.selected_index));
+ state.select(Some(app.selected_index.min(nodes.len().saturating_sub(1))));📝 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 mut state = TableState::default(); | |
| state.select(Some(app.selected_index)); | |
| let mut state = TableState::default(); | |
| state.select(Some(app.selected_index.min(nodes.len().saturating_sub(1)))); |
🤖 Prompt for AI Agents
In crates/basilica-tui/src/ui/screens/miner/nodes.rs around lines 168 to 169,
app.selected_index is used directly to set TableState selection but may exceed
the nodes array length (8); clamp it to the valid range before calling
state.select. Add a check: if nodes.is_empty() do not call select; otherwise
compute a clamped_index = app.selected_index.min(nodes.len() - 1) (ensuring
types match) and call state.select(Some(clamped_index)). Ensure you handle the
case where selected_index is negative or a different integer type by
converting/validating to usize first.
| let mut state = TableState::default(); | ||
| state.select(Some(ctx.selected_index)); |
There was a problem hiding this comment.
Same bounds check issue in context variant.
Apply the same bounds checking fix as the non-ctx variant.
Suggested fix
let mut state = TableState::default();
- state.select(Some(ctx.selected_index));
+ state.select(Some(ctx.selected_index.min(nodes.len().saturating_sub(1))));📝 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 mut state = TableState::default(); | |
| state.select(Some(ctx.selected_index)); | |
| let mut state = TableState::default(); | |
| state.select(Some(ctx.selected_index.min(nodes.len().saturating_sub(1)))); |
🤖 Prompt for AI Agents
In crates/basilica-tui/src/ui/screens/miner/nodes.rs around lines 397 to 398,
the TableState.select call uses ctx.selected_index without bounds checking;
mirror the non-ctx variant by first computing the number of rows (rows.len()),
then if ctx.selected_index as usize < rows.len() call
state.select(Some(ctx.selected_index as usize)) else call state.select(None);
ensure any necessary cast to usize is applied consistently and keep the same
logic/path used in the non-ctx variant.
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (5)
crates/basilica-tui/src/ui/screens/marketplace.rs (1)
95-98: GPU type filter order is non-deterministic.
HashMapiteration order is arbitrary and may change between renders, causing the filter list to visually jump around. Sort the GPU types before rendering for consistent ordering.crates/basilica-tui/src/ui/components/footer.rs (1)
13-137: Code duplication between render_footer and render_footer_ctx.The two functions contain nearly identical logic (~80 lines duplicated), differing only in the source of mode/screen state. This was previously flagged and should be addressed by extracting the hint-building logic into a shared helper function.
crates/basilica-tui/src/actions.rs (3)
82-101: Blocking I/O in async function.This issue was identified in a previous review.
Command::output()is a blocking call that will block the async runtime thread. Usetokio::process::Commandinstead for async-safe subprocess execution.
166-168: Same blocking I/O issue in scp_copy.This issue was identified in a previous review. Apply the same fix using
tokio::process::Commandwith.awaitfor theoutput()call.
197-223: Windows clipboard implementation and return path concern.This issue was partially identified in a previous review regarding Windows. Additionally, note that on non-macOS/Linux/Windows platforms, the function returns
Ok(())without doing anything, which may silently fail to copy.
🧹 Nitpick comments (10)
crates/basilica-tui/README.md (2)
52-62: Add blank lines before tables per markdown formatting standards.Markdown tables should be surrounded by blank lines. Add a blank line before each table header to comply with MD058 and improve readability.
🔎 Proposed fix
### Global + | Key | Action |Apply the same pattern before each table (lines 52, 64, 76, 84, 94):
### Rentals Screen + | Key | Action |### Marketplace Screen + | Key | Action |### Deployments Screen + | Key | Action |### Settings Screen + | Key | Action |Also applies to: 64-74, 76-82, 84-92, 94-101
137-137: Specify a language identifier for the code block.The architecture diagram code fence at line 137 is missing a language identifier. Add
textorplaintextto comply with MD040.🔎 Proposed fix
-``` +```text src/crates/basilica-tui/src/ui/components/mod.rs (1)
1-8: Module structure looks good.The public submodule declarations appropriately expose the UI components. The
#![allow(dead_code)]attribute is acceptable for a new module under active development, but consider removing it once the API stabilizes to catch unused code.crates/basilica-tui/src/main.rs (1)
27-46: Consider removingpubfromArgsstruct.For a binary crate, the
Argsstruct doesn't need to be public since it's not exported for external use. Usingstruct Args(withoutpub) is sufficient.Suggested change
-pub struct Args { +struct Args {crates/basilica-tui/src/ui/mod.rs (1)
68-90: Consider improving error handling in suspend_and_run.The method has several error handling considerations:
- If
self.exit()at line 73 fails, the closuref()still executes, which may cause unexpected behavior.- Line 84 ignores the
Resultfromstdin().read_line(), which could silently fail.- If
self.enter()at line 87 fails, the TUI may be left in an inconsistent state.Consider adding early returns or propagating errors appropriately to ensure consistent terminal state management.
crates/basilica-tui/src/ui/screens/settings.rs (1)
373-410: Theme selection UI correctly shows availability status.The theme section appropriately indicates which themes are available vs. coming soon. Consider extracting the theme list to a constant or configuration if you plan to add more themes.
crates/basilica-tui/src/ui/theme.rs (1)
162-165: Consider removingtab_selectedalias or documenting purpose.
tab_selected()simply delegates totab_active(). If they're semantically identical, consider removing one to reduce API surface, or document why both exist if there's intent to differentiate later.crates/basilica-tui/src/actions.rs (1)
43-51: Async wrapper is a no-op placeholder.The
ssh_connectasync function logs the command but doesn't execute anything. The comment explains this is intentional (caller must use sync version), but consider either removing this function or making the design intent clearer in the signature/docs.crates/basilica-tui/src/ui/components/dialog.rs (1)
319-376: Cursor position parameter is unused in render_input_dialog.The
cursor_posparameter is passed but prefixed with_and ignored. The cursor is displayed as a fixed_appended to the value. Consider implementing proper cursor positioning for better text editing UX, or remove the parameter if not needed.🔎 Suggested implementation for cursor rendering
If you want to show the cursor at the correct position:
fn render_input_dialog( frame: &mut Frame, area: Rect, title: &str, prompt: &str, value: &str, - _cursor_pos: usize, + cursor_pos: usize, placeholder: Option<&str>, theme: &Theme, ) { // ... - let input = Paragraph::new(format!("{}_", display_value)) + let display_with_cursor = if value.is_empty() { + format!("_{}", placeholder.unwrap_or("")) + } else { + let (before, after) = value.split_at(cursor_pos.min(value.len())); + format!("{}_{}", before, after) + }; + let input = Paragraph::new(display_with_cursor)crates/basilica-tui/src/app/core.rs (1)
1313-1325: Notification queue management uses O(n) removal.Using
remove(0)on aVecis O(n). For a queue of max 5 items this is negligible, but considerVecDequeif the limit increases.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (25)
crates/basilica-cli/src/auth/mod.rscrates/basilica-cli/src/auth/types.rscrates/basilica-cli/src/cli/handlers/auth.rscrates/basilica-cli/src/config/mod.rscrates/basilica-sdk/Cargo.tomlcrates/basilica-sdk/src/auth/callback_server.rscrates/basilica-sdk/src/auth/device_flow.rscrates/basilica-sdk/src/auth/mod.rscrates/basilica-sdk/src/auth/oauth_flow.rscrates/basilica-tui/README.mdcrates/basilica-tui/src/actions.rscrates/basilica-tui/src/app/core.rscrates/basilica-tui/src/app/mod.rscrates/basilica-tui/src/main.rscrates/basilica-tui/src/ui/components/dialog.rscrates/basilica-tui/src/ui/components/footer.rscrates/basilica-tui/src/ui/components/mod.rscrates/basilica-tui/src/ui/mod.rscrates/basilica-tui/src/ui/screens/billing.rscrates/basilica-tui/src/ui/screens/deployments.rscrates/basilica-tui/src/ui/screens/marketplace.rscrates/basilica-tui/src/ui/screens/mod.rscrates/basilica-tui/src/ui/screens/rentals.rscrates/basilica-tui/src/ui/screens/settings.rscrates/basilica-tui/src/ui/theme.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- crates/basilica-tui/src/ui/screens/billing.rs
- crates/basilica-tui/src/ui/screens/rentals.rs
🧰 Additional context used
🧬 Code graph analysis (11)
crates/basilica-cli/src/config/mod.rs (1)
crates/basilica-sdk/src/auth/mod.rs (1)
create_auth_config_with_port(64-83)
crates/basilica-tui/src/ui/components/footer.rs (1)
crates/basilica-tui/src/ui/theme.rs (1)
text(75-77)
crates/basilica-cli/src/cli/handlers/auth.rs (2)
crates/basilica-cli/src/progress/mod.rs (2)
complete_spinner_and_clear(137-139)complete_spinner_error(130-134)crates/basilica-cli/src/output/mod.rs (1)
print_info(30-32)
crates/basilica-tui/src/ui/components/dialog.rs (1)
crates/basilica-tui/src/ui/theme.rs (5)
text(75-77)default(207-209)text_accent(93-95)text_muted(80-82)keybind(168-173)
crates/basilica-tui/src/ui/components/mod.rs (1)
crates/basilica-tui/src/ui/theme.rs (1)
header(142-147)
crates/basilica-sdk/src/auth/mod.rs (5)
crates/basilica-sdk/src/auth/oauth_flow.rs (2)
refresh_access_token(248-255)new(68-79)crates/basilica-sdk/src/auth/refresh.rs (1)
refresh_access_token(25-73)crates/basilica-sdk/src/auth/types.rs (2)
get_sdk_data_dir(214-221)new(47-52)crates/basilica-sdk/src/auth/token_store.rs (1)
new(20-28)crates/basilica-common/src/auth_constants.rs (2)
auth0_client_id(24-29)auth0_issuer(40-45)
crates/basilica-tui/src/ui/mod.rs (1)
crates/basilica-tui/src/app/core.rs (1)
new(277-385)
crates/basilica-tui/src/app/core.rs (9)
crates/basilica-tui/src/ui/screens/settings.rs (3)
all(25-32)label(34-41)render_with_ctx(96-113)crates/basilica-autoscaler/src/warm_pool.rs (1)
mem(363-363)crates/basilica-tui/src/ui/components/dialog.rs (6)
handle_dialog_key(602-789)input(143-155)form(184-194)confirm(110-121)confirm_custom(124-140)render_dialog(221-276)crates/basilica-tui/src/actions.rs (1)
ssh_connect_sync(19-41)crates/basilica-sdk/src/auth/mod.rs (1)
create_auth_config_with_port(64-83)crates/basilica-tui/src/ui/screens/deployments.rs (1)
render_with_ctx(14-44)crates/basilica-tui/src/ui/screens/marketplace.rs (1)
render_with_ctx(14-39)crates/basilica-tui/src/ui/screens/rentals.rs (1)
render_with_ctx(14-44)crates/basilica-tui/src/ui/components/notifications.rs (1)
render_notifications_ctx(69-118)
crates/basilica-sdk/src/auth/device_flow.rs (1)
crates/basilica-sdk/src/auth/types.rs (1)
new(47-52)
crates/basilica-cli/src/auth/mod.rs (1)
crates/basilica-sdk/src/auth/mod.rs (4)
is_container_runtime(42-45)is_ssh_session(37-39)is_wsl_environment(30-34)should_use_device_flow(54-56)
crates/basilica-tui/src/ui/screens/marketplace.rs (4)
crates/basilica-tui/src/ui/theme.rs (8)
text(75-77)header(142-147)default(207-209)text_muted(80-82)keybind(168-173)text_bold(85-90)text_accent(93-95)text_warning(103-105)crates/basilica-tui/src/ui/components/header.rs (1)
render_header_ctx(83-146)crates/basilica-tui/src/ui/components/footer.rs (2)
footer_height(140-142)render_footer_ctx(80-137)crates/basilica-tui/src/data/user.rs (1)
new(95-97)
🪛 markdownlint-cli2 (0.18.1)
crates/basilica-tui/README.md
53-53: Tables should be surrounded by blank lines
(MD058, blanks-around-tables)
65-65: Tables should be surrounded by blank lines
(MD058, blanks-around-tables)
77-77: Tables should be surrounded by blank lines
(MD058, blanks-around-tables)
85-85: Tables should be surrounded by blank lines
(MD058, blanks-around-tables)
95-95: Tables should be surrounded by blank lines
(MD058, blanks-around-tables)
137-137: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🔇 Additional comments (42)
crates/basilica-tui/README.md (1)
1-178: Comprehensive README with clear structure and documentation.The README effectively documents the TUI's features, usage, keybindings, configuration, authentication workflow, architecture, and development setup. Coverage is thorough and well-organized for both end-users and developers. The only issues are minor markdown formatting conventions flagged by linters above.
crates/basilica-sdk/src/auth/callback_server.rs (1)
144-144: Branding updates align with SDK generalization.The HTML page content changes appropriately generalize the messaging from CLI-specific to application-agnostic, supporting both CLI and TUI consumers of the SDK.
Also applies to: 206-207, 224-224, 300-300
crates/basilica-sdk/src/auth/device_flow.rs (2)
166-180: Clean separation of concerns in device flow API.The refactored
start_flowreturning(DeviceAuthInstructions, DeviceFlowPending)instead of completing inline is a good design choice. It allows callers (CLI, TUI) to display instructions in their preferred format before waiting for completion.
237-251:AuthConfigcorrectly derivesClone. Verified atcrates/basilica-sdk/src/auth/types.rs:16— the struct derives bothDebugandClone, confirming theclone()call inwait_for_completion()is safe.crates/basilica-sdk/Cargo.toml (1)
32-37: Dependencies appropriately support auth functionality.The added workspace dependencies (
webbrowser,rand,sha2,url,tokio-util,axum) align with OAuth flow requirements for PKCE, browser launching, and callback server handling.crates/basilica-cli/src/auth/types.rs (1)
1-6: Good consolidation of auth types to SDK.Re-exporting
AuthConfig,AuthError, andAuthResultfrombasilica_sdk::autheliminates duplication and ensures type consistency between CLI and SDK.crates/basilica-tui/src/app/mod.rs (1)
5-8: Module facade pattern is appropriate here.The blanket re-export from
coreprovides a clean public API. The submodule namecoreis fine since it's accessed asself::coreinternally and won't conflict withstd::core.crates/basilica-tui/src/main.rs (1)
48-64: Clean application bootstrap sequence.The main function follows a sensible flow: argument parsing, logging setup, config loading, and app execution. Error propagation with
?andanyhow::Resultis appropriate for a binary entry point.crates/basilica-cli/src/config/mod.rs (1)
117-145: Good layered configuration approach.Starting with the SDK's base config and extending with CLI-specific scopes is a clean pattern. The port validation warning (lines 120-128) appropriately alerts about misconfiguration without breaking device flow (port 0).
crates/basilica-tui/src/ui/screens/mod.rs (1)
1-10: LGTM!The module structure is clean and properly organized. The
#![allow(dead_code)]attribute is reasonable for modules under active development.crates/basilica-cli/src/cli/handlers/auth.rs (1)
61-107: LGTM!The device flow refactoring properly handles the new SDK return type. The conditional terminal line clearing based on
verification_uri_completepresence is appropriate.crates/basilica-sdk/src/auth/oauth_flow.rs (2)
157-164: LGTM!The new public accessor methods
get_auth_urlandget_expected_stateprovide a clean API for external callers to retrieve the authorization URL and state parameter.
201-245: LGTM!Making
exchange_code_for_tokenpublic is appropriate and enables external callers to perform manual code exchange when needed.crates/basilica-sdk/src/auth/mod.rs (3)
30-34: LGTM!The WSL detection logic correctly checks
/proc/versionfor Microsoft/WSL identifiers, which is the standard approach.
37-56: LGTM!The environment detection utilities correctly identify SSH sessions, container runtimes, and appropriately determine when device flow should be used. The logic is sound and covers the relevant scenarios.
64-83: LGTM!The
create_auth_config_with_portfunction properly centralizes auth configuration with correct endpoints, standard OAuth scopes, and parameterized redirect URI port.crates/basilica-tui/src/ui/screens/marketplace.rs (2)
41-70: LGTM!The action bar correctly applies conditional styling based on selection and filter state, providing good visual feedback to users.
128-213: LGTM!The GPU list rendering comprehensively handles empty states with appropriate messages and applies contextual styling based on source type and selection state. The table structure is well-organized.
crates/basilica-tui/src/ui/screens/deployments.rs (3)
14-44: LGTM!The conditional layout properly handles the split view when logs are enabled, providing a clear separation between deployments table and logs panel.
73-157: LGTM!The deployments table rendering correctly handles empty states, applies appropriate status-based styling, and properly formats GPU information (showing "CPU" for non-GPU deployments).
159-192: LGTM!The deployment logs panel provides appropriate context and clear placeholder messaging for the not-yet-implemented log streaming feature.
crates/basilica-tui/src/ui/mod.rs (1)
23-62: LGTM!The TUI lifecycle methods (
new,enter,exit,draw) are correctly implemented with proper terminal setup/teardown and symmetric operations. TheDropimplementation ensures cleanup.crates/basilica-cli/src/auth/mod.rs (1)
1-17: Clean centralization of auth types from SDK.The refactoring appropriately consolidates authentication logic into the SDK while maintaining CLI-specific types locally. The re-export organization with separate sections for SDK and CLI types improves maintainability.
crates/basilica-tui/src/ui/screens/settings.rs (3)
14-60: Well-structured section navigation with cyclic helpers.The
SettingsSectionenum withnext()/prev()helpers provides clean cyclic navigation. Theall()method returning a static slice is memory-efficient.
62-93: State and data structs are appropriately defined.The structs provide necessary fields for settings management with correct derives. The separation of
AuthStatus,TokenInfo, andSshKeyInfokeeps concerns well-organized.
95-113: Clean layout composition with context-based rendering.The render entry point properly decomposes the screen into header, tabs, content, and footer sections. The delegation pattern to section-specific renderers keeps the code maintainable.
crates/basilica-tui/src/ui/theme.rs (3)
1-33: Comprehensive theme structure with semantic color fields.The
Themestruct provides a well-organized set of color fields covering all major UI concerns. The field names are self-documenting and the derives are appropriate.
35-70: Well-crafted color palettes for dark and light themes.The dark theme appears Catppuccin-inspired with good contrast ratios. The light theme provides appropriate counterparts. Both palettes maintain visual consistency.
206-210: Default implementation correctly delegates to dark theme.Clean implementation following Rust idioms.
crates/basilica-tui/src/actions.rs (3)
10-14: SpawnResult struct appropriately captures execution outcome.Clean structure for conveying success status and messages from subprocess execution.
16-41: Correct use of inherited stdio for interactive SSH.The sync SSH connection properly inherits stdin/stdout/stderr for interactive terminal usage.
180-195: Platform-specific URL opener is well-structured.Clean use of
#[cfg]attributes for platform-specific code. The Windows implementation usescmd /c startwhich is correct.crates/basilica-tui/src/ui/components/dialog.rs (4)
14-49: Well-designed DialogKind enum with comprehensive variants.The enum covers common dialog patterns (confirm, input, select, form, alert) with appropriate data for each variant. The structure allows clean pattern matching in both rendering and key handling.
90-106: DialogState and DialogResult provide clean state management.The separation of dialog state from result allows proper lifecycle management. The
take_result()method follows Rust idioms for consuming optional values.
108-218: Factory methods provide ergonomic dialog creation.The constructors using
impl Into<String>are flexible. Theconfirm_result()andcancel_result()methods properly close the dialog after setting the result.
600-789: Comprehensive keyboard handling with dual-purpose 'j'/'k' keys.The key handler correctly differentiates context: 'j'/'k' navigate in Select dialogs but insert characters in Input/Form dialogs. The Tab/BackTab handling for form navigation is correct with proper wraparound.
crates/basilica-tui/src/app/core.rs (6)
16-134: Well-structured screen enums with consistent navigation helpers.The
UserScreenandMinerScreenenums provide cleanall(),label(),index(), andfrom_index()methods enabling flexible navigation. Thefrom_indexfallback to default screen is a safe choice.
152-211: Comprehensive App state structure.The App struct aggregates all necessary state with clear field organization. The use of
Arc<RwLock<_>>for shared data enables safe concurrent access during async operations.
275-399: App initialization with proper dev mode and offline fallback.The
new()method handles dev mode with mock data and gracefully falls back to offline mode if client creation fails. The notification to guide users when not logged in is helpful.
2116-2153: Clean render dispatch with overlay composition.The
render_appfunction properly composes screen rendering with help overlay, dialog overlay, and notifications in correct z-order. The use ofRenderContextprovides a clean interface for passing state to renderers.
2155-2170: RenderContext provides comprehensive state access for renderers.The lifetime-bound context struct cleanly aggregates all necessary state for rendering without requiring the renderers to access the App directly.
1156-1159: No action needed. Theis_multiple_ofmethod was stabilized in Rust 1.87 (February 2025) and no longer requires theunsigned_is_multiple_offeature flag. The code is correct as written.Likely an incorrect or invalid review comment.
| // Print info before opening browser | ||
| crate::output::print_info("Opening browser for sign in..."); | ||
| crate::output::print_info("Browser didn't open? Use the URL below to sign in:"); | ||
| let auth_url = oauth_flow.get_auth_url().map_err(|e| eyre!(e))?; | ||
| println!("{}", console::style(&auth_url).dim()); | ||
| crate::output::print_info("Waiting for authentication..."); | ||
|
|
||
| match oauth_flow.start_flow().await { | ||
| Ok(tokens) => tokens, | ||
| Ok(tokens) => { | ||
| // Clear the browser instructions | ||
| let term = console::Term::stdout(); | ||
| let _ = term.clear_last_lines(6); | ||
| tokens | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Count the actual number of lines printed by print_info and println statements
# Check how print_info formats output
echo "=== Checking print_info implementation ==="
rg -n "pub fn print_info" crates/basilica-cli/src/output/mod.rs -A 5
# Examine println usage in the auth handler
echo -e "\n=== Examining output statements in auth.rs ==="
rg -n "print_info\|println!" crates/basilica-cli/src/cli/handlers/auth.rs -B 1 -A 1Repository: one-covenant/basilica
Length of output: 338
🏁 Script executed:
#!/bin/bash
# Verify the file exists and check the actual content
echo "=== Checking if file exists ==="
ls -la crates/basilica-cli/src/cli/handlers/auth.rs
echo -e "\n=== Content at lines 110-125 ==="
sed -n '110,125p' crates/basilica-cli/src/cli/handlers/auth.rs | cat -n
echo -e "\n=== Full context around the OAuth flow ==="
sed -n '105,130p' crates/basilica-cli/src/cli/handlers/auth.rs | cat -nRepository: one-covenant/basilica
Length of output: 2145
Verify terminal line clearing count.
The code prints 4 lines of output (lines 112, 113, 115, 116: three print_info calls and one println!), but clears 6 lines at line 122. This discrepancy may clear unintended content or leave visual gaps. Confirm whether clearing 6 lines is intentional.
🤖 Prompt for AI Agents
In crates/basilica-cli/src/cli/handlers/auth.rs around lines 111 to 124, the
code prints four lines (three crate::output::print_info calls and one println!)
but calls term.clear_last_lines(6), which over-clears; change the count to match
the actual number of printed lines (use clear_last_lines(4)) or compute the
number dynamically based on the exact messages shown (e.g., track how many lines
were printed and pass that value) so only the intended lines are cleared.
| /// Get the length of the current list being displayed | ||
| fn get_current_list_len(&self) -> usize { | ||
| // TODO: Return actual list lengths based on current screen and data | ||
| match self.mode { | ||
| AppMode::User => match self.user_screen { | ||
| UserScreen::Rentals => 10, // placeholder | ||
| UserScreen::Marketplace => 20, // placeholder | ||
| UserScreen::Deployments => 5, // placeholder | ||
| _ => 0, | ||
| }, | ||
| AppMode::Miner => match self.miner_screen { | ||
| MinerScreen::Fleet => 8, // placeholder | ||
| MinerScreen::Nodes => 4, // placeholder | ||
| _ => 0, | ||
| }, | ||
| } | ||
| } |
There was a problem hiding this comment.
Placeholder list lengths should use actual data.
The get_current_list_len method returns hardcoded placeholder values. This will cause incorrect list bounds when navigating with actual data. The TODOs indicate awareness but this should be prioritized.
🔎 Proposed fix using actual data
fn get_current_list_len(&self) -> usize {
- // TODO: Return actual list lengths based on current screen and data
match self.mode {
AppMode::User => match self.user_screen {
- UserScreen::Rentals => 10, // placeholder
- UserScreen::Marketplace => 20, // placeholder
- UserScreen::Deployments => 5, // placeholder
+ UserScreen::Rentals => {
+ // Access requires async, so we'd need to cache this or use try_read
+ // For now, this is a known limitation
+ 10 // TODO: Wire to actual data length
+ }
+ UserScreen::Marketplace => 20, // TODO: Wire to actual data length
+ UserScreen::Deployments => 5, // TODO: Wire to actual data length
_ => 0,
},Since user_data is behind RwLock, consider caching list lengths in ScreenStates during data refresh, or using try_read() for non-blocking access.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In crates/basilica-tui/src/app/core.rs around lines 1240 to 1256, the
get_current_list_len function currently returns hardcoded placeholder values;
replace those placeholders with actual lengths derived from the underlying data
structures for each screen. For User screens, acquire a read handle to the
user_data (use try_read() to avoid blocking or propagate a cached length from
ScreenStates if try_read fails) and return the appropriate collection.len() for
Rentals, Marketplace, Deployments; for Miner screens, similarly read the miner
data or cached lengths for Fleet and Nodes. Ensure you handle lock contention by
returning 0 or the cached length when try_read() fails and keep the match arms
symmetrical so navigation bounds reflect real data sizes.
| async fn handle_login(&mut self) { | ||
| use basilica_sdk::auth::{ | ||
| create_auth_config_with_port, get_sdk_data_dir, should_use_device_flow, | ||
| CallbackServer, DeviceFlow, OAuthFlow, TokenStore, | ||
| }; | ||
|
|
||
| self.add_notification("Starting authentication...", NotificationLevel::Info); | ||
|
|
||
| // Check if we should use device flow (headless environments) | ||
| let use_device_flow = should_use_device_flow(); | ||
|
|
||
| // Find available port for callback server | ||
| let auth_config = if use_device_flow { | ||
| create_auth_config_with_port(0) | ||
| } else { | ||
| match CallbackServer::find_available_port() { | ||
| Ok(port) => create_auth_config_with_port(port), | ||
| Err(e) => { | ||
| self.add_notification( | ||
| &format!("Auth setup failed: {}", e), | ||
| NotificationLevel::Error, | ||
| ); | ||
| return; | ||
| } | ||
| } | ||
| }; | ||
|
|
||
| // Initialize token store | ||
| let data_dir = match get_sdk_data_dir() { | ||
| Ok(dir) => dir, | ||
| Err(e) => { | ||
| self.add_notification( | ||
| &format!("Data dir error: {}", e), | ||
| NotificationLevel::Error, | ||
| ); | ||
| return; | ||
| } | ||
| }; | ||
|
|
||
| let token_store = match TokenStore::new(data_dir) { | ||
| Ok(store) => store, | ||
| Err(e) => { | ||
| self.add_notification( | ||
| &format!("Token store error: {}", e), | ||
| NotificationLevel::Error, | ||
| ); | ||
| return; | ||
| } | ||
| }; | ||
|
|
||
| // Perform the OAuth flow | ||
| let token_result = if use_device_flow { | ||
| let device_flow = DeviceFlow::new(auth_config); | ||
| match device_flow.start_flow().await { | ||
| Ok((instructions, pending)) => { | ||
| // Show device code to user | ||
| self.add_notification( | ||
| &format!( | ||
| "Visit {} and enter code: {}", | ||
| instructions.verification_uri, instructions.user_code | ||
| ), | ||
| NotificationLevel::Info, | ||
| ); | ||
|
|
||
| // Wait for completion | ||
| pending.wait_for_completion().await | ||
| } | ||
| Err(e) => Err(e), | ||
| } | ||
| } else { | ||
| // Browser-based OAuth - suspend TUI temporarily | ||
| let mut oauth_flow = OAuthFlow::new(auth_config); | ||
|
|
||
| // Get the auth URL first | ||
| let auth_url = match oauth_flow.get_auth_url() { | ||
| Ok(url) => url, | ||
| Err(e) => { | ||
| self.add_notification( | ||
| &format!("Failed to build auth URL: {}", e), | ||
| NotificationLevel::Error, | ||
| ); | ||
| return; | ||
| } | ||
| }; | ||
|
|
||
| self.add_notification("Opening browser for login...", NotificationLevel::Info); | ||
|
|
||
| // Suspend TUI and run OAuth flow | ||
| let flow_result = self.tui.suspend_and_run(|| -> anyhow::Result<()> { | ||
| println!("\n⛪ Basilica - Sacred Compute ⛪\n"); | ||
| println!("Opening browser for sign in..."); | ||
| println!("Browser didn't open? Use the URL below:"); | ||
| println!("{}\n", auth_url); | ||
| println!("Waiting for authentication..."); | ||
| Ok(()) | ||
| }); | ||
|
|
||
| if let Err(e) = flow_result { | ||
| self.add_notification(&format!("TUI error: {}", e), NotificationLevel::Error); | ||
| return; | ||
| } | ||
|
|
||
| // Run the OAuth flow (browser will open) | ||
| oauth_flow.start_flow().await | ||
| }; | ||
|
|
||
| match token_result { | ||
| Ok(tokens) => { | ||
| // Store tokens | ||
| if let Err(e) = token_store.store_tokens(&tokens).await { | ||
| self.add_notification( | ||
| &format!("Failed to store tokens: {}", e), | ||
| NotificationLevel::Error, | ||
| ); | ||
| return; | ||
| } | ||
|
|
||
| // Update auth status | ||
| self.screens.settings.auth_status.logged_in = true; | ||
| // TODO: Extract email from JWT token | ||
| self.screens.settings.auth_status.user_email = Some("user@example.com".to_string()); | ||
| if let Some(expiry) = tokens.time_until_expiry() { | ||
| let expiry_time = | ||
| chrono::Utc::now() + chrono::Duration::seconds(expiry.as_secs() as i64); | ||
| self.screens.settings.auth_status.token_expiry = | ||
| Some(expiry_time.format("%Y-%m-%d %H:%M UTC").to_string()); | ||
| } | ||
|
|
||
| // Recreate the API client with the new tokens | ||
| let api_url = self.config.api_url.clone(); | ||
| match Self::create_client(&api_url).await { | ||
| Ok(client) => { | ||
| self.client = Some(Arc::new(client)); | ||
| } | ||
| Err(e) => { | ||
| self.add_notification( | ||
| &format!("Failed to create client: {}", e), | ||
| NotificationLevel::Warning, | ||
| ); | ||
| } | ||
| } | ||
|
|
||
| self.add_notification("⛪ Login successful!", NotificationLevel::Success); | ||
| } | ||
| Err(e) => { | ||
| self.add_notification(&format!("Login failed: {}", e), NotificationLevel::Error); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Login flow doesn't open browser for OAuth path.
In the browser-based OAuth flow (lines 1832-1866), the TUI is suspended and the auth URL is printed, but open_url() is never called to actually open the browser. The user has to manually copy the URL.
🔎 Proposed fix to open browser
self.add_notification("Opening browser for login...", NotificationLevel::Info);
// Suspend TUI and run OAuth flow
let flow_result = self.tui.suspend_and_run(|| -> anyhow::Result<()> {
println!("\n⛪ Basilica - Sacred Compute ⛪\n");
println!("Opening browser for sign in...");
println!("Browser didn't open? Use the URL below:");
println!("{}\n", auth_url);
println!("Waiting for authentication...");
+ // Actually open the browser
+ let _ = crate::actions::open_url(&auth_url);
Ok(())
});Note: The closure captures auth_url so this should work. Alternatively, call open_url before suspending the TUI.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
crates/basilica-tui/src/app/core.rs around lines 1763 to 1911: the browser-based
OAuth path prints the auth URL but never opens the browser; call the SDK helper
to open the URL (e.g., basilica_sdk::auth::open_url(&auth_url)) either just
before calling self.tui.suspend_and_run(...) or inside the suspend closure
(cloning auth_url into the closure), handle the Result and add a notification on
failure, and add the necessary import for open_url at the top of the function.
| KeyCode::Backspace => { | ||
| match kind { | ||
| DialogKind::Input { value, cursor_pos, .. } => { | ||
| if *cursor_pos > 0 { | ||
| *cursor_pos -= 1; | ||
| value.remove(*cursor_pos); | ||
| } | ||
| true | ||
| } | ||
| DialogKind::Form { fields, selected_field, .. } => { | ||
| if let Some(field) = fields.get_mut(*selected_field) { | ||
| field.value.pop(); | ||
| } | ||
| true | ||
| } | ||
| _ => false, | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Read the dialog.rs file to understand the full context
cd crates/basilica-tui/src/ui/components && wc -l dialog.rsRepository: one-covenant/basilica
Length of output: 78
🏁 Script executed:
# Read the section around lines 675-691 with more context
cd crates/basilica-tui/src/ui/components && sed -n '650,710p' dialog.rsRepository: one-covenant/basilica
Length of output: 2227
🏁 Script executed:
# Search for how cursor_pos is used and tracked throughout this file
cd crates/basilica-tui/src/ui/components && rg -n "cursor_pos" dialog.rs -A 2 -B 2 | head -100Repository: one-covenant/basilica
Length of output: 2057
🌐 Web query:
Rust String remove panic UTF-8 char boundary
💡 Result:
Rust's String methods that take byte indices (remove/insert/drain/split_at) will panic if the index isn't on a UTF‑8 char boundary. To avoid the panic, either check boundaries with str::is_char_boundary or operate by chars/char_indices and convert to a valid byte range before mutating.
Short examples:
- Remove at a byte index if it's a valid char boundary:
if s.is_char_boundary(i) {
// remove the next char starting at byte i
let ch_len = s[i..].chars().next().unwrap().len_utf8();
s.drain(i..i+ch_len);
}- Remove the Nth character (by character index) safely:
if let Some((byte_i, ch)) = s.char_indices().nth(n) {
s.drain(byte_i..byte_i + ch.len_utf8());
}- Or check before slicing/splitting:
if s.is_char_boundary(mid) {
let (a, b) = s.split_at(mid); // won't panic if checked
}References: String API (panic behavior) and str::is_char_boundary documentation. [1][2][3]
Sources
- Rust String docs (methods panic on non-char boundary). [1]
- str::is_char_boundary docs. [2]
- String::drain docs (requires char boundaries). [3]
🏁 Script executed:
# Look for test cases in the code that might show cursor handling with multi-byte chars
rg -n "cursor" crates/basilica-tui/src/ui/components/dialog.rs -A 3 -B 1 | grep -E "(test|mod tests)" -A 50Repository: one-covenant/basilica
Length of output: 47
🏁 Script executed:
# Check if there are any tests in the crate related to input handling
find crates/basilica-tui -name "*.rs" -type f -exec grep -l "cursor_pos\|backspace\|Backspace" {} \;Repository: one-covenant/basilica
Length of output: 112
🏁 Script executed:
# Check for other cursor_pos handling patterns in the codebase
rg -n "cursor_pos\s*[\+\-]" crates/basilica-tui/src --type rustRepository: one-covenant/basilica
Length of output: 428
🏁 Script executed:
# Look for string mutation patterns with insert/remove in the codebase
rg "\.insert\(.*cursor" crates/basilica-tui/src --type rust -A 2Repository: one-covenant/basilica
Length of output: 845
Cursor position tracking doesn't handle multi-byte UTF-8 characters and will panic.
All character insertions increment cursor_pos by 1 (lines 707, 733, 749), and backspace decrements by 1 (line 679). With multi-byte characters (emoji, CJK, accented text), cursor_pos becomes misaligned with byte boundaries. For example, inserting '😀' (4 bytes) and then incrementing cursor_pos by 1 places the next operation mid-character, causing String::insert() or String::remove() to panic.
Use char_indices() to track character boundaries instead of byte offsets, or validate with is_char_boundary() before mutations.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (9)
crates/basilica-tui/src/ui/screens/marketplace.rs (1)
105-108: GPU type filter order is non-deterministic.As flagged previously, iterating over the
gpu_typesHashMap produces an arbitrary order that may change between renders, causing the filter list to visually jump and degrading user experience. Sort the entries before rendering for consistency.crates/basilica-tui/src/actions.rs (3)
79-97: Blocking I/O in async function.As flagged in previous reviews,
ssh_execusesstd::process::Command::output()which blocks the async runtime. Usetokio::process::Commandwith.awaitfor non-blocking subprocess execution.
142-174: Same blocking I/O issue in scp_copy.The
scp_copyfunction also uses blockingstd::process::Command::output(). Apply the same fix asssh_execby usingtokio::process::Commandwith.await.
193-219: Windows clipboard implementation missing.The Windows platform support is incomplete—
copy_to_clipboardreturnsOk(())without actually copying to the clipboard on Windows. Users on Windows will experience silent failures.crates/basilica-tui/src/ui/screens/rentals.rs (2)
150-154: UTF-8 string slicing will panic on non-ASCII rental IDs.The code slices
rental.idby byte index (&rental.id[..8]), which panics if the ID contains multi-byte UTF-8 characters (emoji, CJK, accented characters). Use character-based truncation instead.🔎 Proposed fix
-let id_short = if rental.id.len() > 8 { - &rental.id[..8] -} else { - &rental.id -}; +let id_short: String = rental.id.chars().take(8).collect();Then use
id_shortdirectly:-Cell::from(id_short.to_string()), +Cell::from(id_short),
157-160: UTF-8 string slicing will panic on non-ASCII container images.Similar to the ID truncation, slicing
container_imageby byte index can panic on multi-byte UTF-8 sequences.🔎 Proposed fix
-let image = if rental.container_image.len() > 25 { - format!("{}...", &rental.container_image[..22]) -} else { - rental.container_image.clone() -}; +let image = if rental.container_image.chars().count() > 25 { + let truncated: String = rental.container_image.chars().take(22).collect(); + format!("{}...", truncated) +} else { + rental.container_image.clone() +};crates/basilica-tui/src/ui/components/dialog.rs (1)
683-691: Cursor position tracking uses byte indices, will panic on multi-byte UTF-8 characters.All character insertions increment
cursor_posby 1 (lines 723, 732, 757, 778), and backspace decrements by 1 (line 688). With multi-byte UTF-8 characters (emoji, CJK, accented text),cursor_posbecomes misaligned with character boundaries, causingString::insert()orString::remove()to panic.🔎 Recommended fix
Track cursor position by character index, not byte index. Before any
insertorremoveoperation, convert the character index to a byte index:// Helper to convert char index to byte index fn char_idx_to_byte_idx(s: &str, char_idx: usize) -> usize { s.char_indices() .nth(char_idx) .map(|(byte_idx, _)| byte_idx) .unwrap_or(s.len()) } // Example for Backspace: DialogKind::Input { value, cursor_pos, .. } => { if *cursor_pos > 0 { *cursor_pos -= 1; let byte_pos = char_idx_to_byte_idx(value, *cursor_pos); if let Some(ch) = value[byte_pos..].chars().next() { value.drain(byte_pos..byte_pos + ch.len_utf8()); } } true } // Example for Char insertion: DialogKind::Input { value, cursor_pos, .. } => { let byte_pos = char_idx_to_byte_idx(value, *cursor_pos); value.insert(byte_pos, c); *cursor_pos += 1; true }Apply similar fixes to Form field editing (lines 693-702, 726-735, 760-769, 781-790).
Also applies to: 719-792
crates/basilica-tui/src/app/core.rs (2)
1359-1375: Placeholder list lengths should use actual data.The
get_current_list_lenmethod returns hardcoded placeholder values (10, 20, 5, 8, 4). This causes incorrect list bounds when navigating with actual data, leading to potential out-of-bounds selection or inability to select all items.🔎 Recommended fix
Since
user_dataandminer_dataare behindRwLock, usetry_read()for non-blocking access during navigation:fn get_current_list_len(&self) -> usize { match self.mode { AppMode::User => match self.user_screen { - UserScreen::Rentals => 10, // placeholder - UserScreen::Marketplace => 20, // placeholder - UserScreen::Deployments => 5, // placeholder + UserScreen::Rentals => { + self.user_data.try_read() + .map(|data| data.rentals.len()) + .unwrap_or(0) + } + UserScreen::Marketplace => { + self.user_data.try_read() + .map(|data| data.offerings.len()) + .unwrap_or(0) + } + UserScreen::Deployments => { + self.user_data.try_read() + .map(|data| data.deployments.len()) + .unwrap_or(0) + } _ => 0, }, AppMode::Miner => match self.miner_screen { - MinerScreen::Fleet => 8, // placeholder - MinerScreen::Nodes => 4, // placeholder + MinerScreen::Nodes => { + self.miner_data.try_read() + .map(|data| data.nodes.len()) + .unwrap_or(0) + } + MinerScreen::Validators => { + self.miner_data.try_read() + .map(|data| data.validators.len()) + .unwrap_or(0) + } _ => 0, }, } }
1949-1982: OAuth flow prints URL but doesn't open browser.In the browser-based OAuth path (lines 1949-1982), the TUI is suspended and the auth URL is printed, but the browser is never opened automatically. Users must manually copy the URL.
🔎 Proposed fix
Open the browser before or during the suspend:
self.add_notification("Opening browser for login...", NotificationLevel::Info); + // Try to open browser + let auth_url_clone = auth_url.clone(); + if let Err(e) = crate::actions::open_url(&auth_url_clone) { + tracing::warn!("Failed to open browser: {}", e); + } + // Suspend TUI and run OAuth flow let flow_result = self.tui.suspend_and_run(|| -> anyhow::Result<()> { println!("\n⛪ Basilica - Sacred Compute ⛪\n"); println!("Opening browser for sign in..."); - println!("Browser didn't open? Use the URL below:"); + println!("If browser didn't open, use the URL below:"); println!("{}\n", auth_url); println!("Waiting for authentication..."); Ok(()) });
🧹 Nitpick comments (9)
crates/basilica-tui/README.md (1)
61-113: Optional: Add blank lines around tables for Markdown consistency.The Markdown linter suggests adding blank lines before and after each table for better formatting consistency. Similarly, the architecture diagram code block at line 155 should specify a language identifier (even if just as a placeholder or "text").
crates/basilica-tui/src/ui/mod.rs (1)
64-90: Consider error handling for stdin read in suspend_and_run.Line 84 ignores errors from stdin reading with
let _ = std::io::stdin().read_line(...). If reading fails, the user might not see the prompt to resume, leading to confusion.🔎 Suggested improvement
// Wait for user to press Enter before resuming println!("\n\nPress Enter to return to TUI..."); -let _ = std::io::stdin().read_line(&mut String::new()); +if let Err(e) = std::io::stdin().read_line(&mut String::new()) { + eprintln!("Failed to read input: {}. Resuming anyway...", e); +}crates/basilica-tui/src/ui/components/dialog.rs (1)
609-825: Consider extracting dialog key handling into per-type methods.The
handle_dialog_keyfunction is 216 lines with deeply nested match statements. Extracting per-dialog-type handlers would improve maintainability and testability.Example refactor
// Add methods to DialogKind impl DialogKind { fn handle_key(&mut self, key: KeyEvent) -> bool { match self { DialogKind::Input { .. } => self.handle_input_key(key), DialogKind::Select { .. } => self.handle_select_key(key), DialogKind::Form { .. } => self.handle_form_key(key), _ => false, } } fn handle_input_key(&mut self, key: KeyEvent) -> bool { // Input-specific handling } // ... other handlers }Then
handle_dialog_keybecomes simpler:pub fn handle_dialog_key(dialog: &mut DialogState, key: KeyEvent) -> bool { if !dialog.active { return false; } match key.code { KeyCode::Esc => { dialog.cancel_result(); true } KeyCode::Enter => { /* handle submission */ true } _ => dialog.kind.as_mut().map_or(false, |k| k.handle_key(key)) } }crates/basilica-tui/src/app/core.rs (1)
291-418: Consider splitting App::new into smaller initialization functions.The
App::newconstructor is 127 lines long and handles multiple concerns: config setup, mock data, client creation, auth checking, phase determination. Extracting helpers likeinit_data,init_auth_status,determine_initial_phasewould improve readability and testability.Example refactor
impl App { pub async fn new(...) -> Result<Self> { let theme = Theme::dark(); let event_handler = EventHandler::new(tick_rate); let tui = Tui::new()?; let (user_data, miner_data) = Self::init_data(dev_mode); let client = Self::init_client(dev_mode, &config.api_url).await; let mut screens = Self::init_screens(dev_mode); let notifications = Self::init_notifications(dev_mode, &client, &mut screens); let (phase, startup_selection) = Self::determine_phase(miner_mode); Ok(Self { /* ... */ }) } fn init_data(dev_mode: bool) -> (Arc<RwLock<UserData>>, Arc<RwLock<MinerData>>) { // Extract lines 304-314 } async fn init_client(dev_mode: bool, api_url: &str) -> Option<Arc<BasilicaClient>> { // Extract lines 318-337 } // ... other helpers }crates/basilica-sdk/src/auth/oauth_flow.rs (1)
157-160: Consider the necessity of theget_auth_urlwrapper.This method is a thin wrapper around
build_auth_urlwith no additional logic. Unless you plan to add behavior here in the future, consider makingbuild_auth_urlpublic directly or documenting why the indirection is valuable.crates/basilica-tui/src/ui/screens/settings.rs (2)
115-143: Consider indexing tabs by position rather than enum cast.Line 138 casts
section as usizeto select the tab, which assumes enum discriminants match the order inall(). While this works currently, it's fragile if the enum variants are reordered or given explicit discriminants.🔎 Safer alternative using position lookup
fn render_section_tabs(frame: &mut Frame, ctx: &RenderContext, area: Rect) { let theme = ctx.theme; let section = ctx.settings_state.section; + + let selected_idx = SettingsSection::all() + .iter() + .position(|s| *s == section) + .unwrap_or(0); let titles: Vec<Line> = SettingsSection::all() .iter() .map(|s| { let style = if *s == section { theme.tab_selected() } else { theme.tab_inactive() }; Line::from(Span::styled(s.label(), style)) }) .collect(); let tabs = Tabs::new(titles) .block( Block::default() .borders(Borders::ALL) .border_style(theme.border()) .title(Span::styled(" Settings ", theme.block_title())), ) - .select(section as usize) + .select(selected_idx) .style(theme.text()) .highlight_style(theme.tab_selected()); frame.render_widget(tabs, area); }
156-228: LGTM! Auth section handles both logged in/out states well.The conditional rendering clearly distinguishes between authenticated and unauthenticated states with appropriate prompts.
Optional: Avoid cloning Option<String> values
Lines 174-176 and 184-186 clone
Option<String>values unnecessarily. Consider:Line::from(vec![ Span::styled("Email: ", theme.text_muted()), Span::styled( - auth.user_email - .clone() - .unwrap_or_else(|| "Unknown".to_string()), + auth.user_email.as_deref().unwrap_or("Unknown"), theme.text(), ), ]),Apply similar change to
token_expiryon lines 184-186.crates/basilica-sdk/src/auth/device_flow.rs (2)
231-253: Consider adding serialization support.The new public API structs are well-designed with appropriate encapsulation (
DeviceFlowPendingfields are private). However, if there's a need to persist the device flow state (e.g., for resuming interrupted flows), consider addingSerialize/Deserializederives to both structs.🔎 Optional serialization support
+use serde::{Deserialize, Serialize}; + /// Instructions for user to complete device authorization -#[derive(Debug, Clone)] +#[derive(Debug, Clone, Serialize, Deserialize)] pub struct DeviceAuthInstructions { pub verification_uri: String, pub user_code: String, pub verification_uri_complete: Option<String>, } /// Pending device flow that can be polled for completion +#[derive(Debug, Clone, Serialize, Deserialize)] pub struct DeviceFlowPending { device_code: String, poll_interval: Duration, config: AuthConfig, }Note: This requires
AuthConfigto also implementSerialize/Deserialize.
166-180: Breaking change is already integrated into calling code.The
start_flowsignature change fromAuthResult<TokenSet>toAuthResult<(DeviceAuthInstructions, DeviceFlowPending)>has been correctly implemented in bothbasilica-cli(handlers/auth.rs) andbasilica-tui(app/core.rs), which now pattern match the returned tuple and callpending.wait_for_completion().await. The architectural shift enables non-blocking patterns for flexible UI integration.Optional: Consider using
Arc<AuthConfig>to avoid redundant clones.Line 177 clones
AuthConfig, andDeviceFlowPending::wait_for_completion()(line 249) clones it again when creating a newDeviceFlow. While the currentCloneimplementation is reasonable forAuthConfig(String, Vec, HashMap components), wrapping config inArc<AuthConfig>would eliminate these redundant clones. This is a minor optimization and optional since the codebase already usesArcfor similar multi-ownership patterns (e.g.,Arc<Mutex<AuthMethod>>in simple_manager.rs).
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (21)
crates/basilica-sdk/src/auth/callback_server.rscrates/basilica-sdk/src/auth/device_flow.rscrates/basilica-sdk/src/auth/mod.rscrates/basilica-sdk/src/auth/oauth_flow.rscrates/basilica-tui/Cargo.tomlcrates/basilica-tui/README.mdcrates/basilica-tui/src/actions.rscrates/basilica-tui/src/app/core.rscrates/basilica-tui/src/config.rscrates/basilica-tui/src/ui/components/dialog.rscrates/basilica-tui/src/ui/mod.rscrates/basilica-tui/src/ui/screens/billing.rscrates/basilica-tui/src/ui/screens/deployments.rscrates/basilica-tui/src/ui/screens/marketplace.rscrates/basilica-tui/src/ui/screens/mod.rscrates/basilica-tui/src/ui/screens/rentals.rscrates/basilica-tui/src/ui/screens/settings.rscrates/basilica-tui/src/ui/screens/startup.rsjustfilescripts/billing/Dockerfilescripts/payments/Dockerfile
🚧 Files skipped from review as they are similar to previous changes (4)
- crates/basilica-tui/src/config.rs
- crates/basilica-sdk/src/auth/mod.rs
- crates/basilica-tui/src/ui/screens/mod.rs
- crates/basilica-sdk/src/auth/callback_server.rs
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-13T15:34:46.544Z
Learnt from: epappas
Repo: one-covenant/basilica PR: 184
File: crates/basilica-validator/src/rental/billing.rs:129-130
Timestamp: 2025-10-13T15:34:46.544Z
Learning: In the basilica-validator crate, rental_id is a String type (not UUID) throughout the system - in RentalInfo struct, database rentals.id column, and all rental operations. The persistence layer methods should accept &str for rental_id parameters, not &Uuid, since they immediately convert to String for SQL binding anyway.
Applied to files:
crates/basilica-tui/src/ui/screens/rentals.rs
🧬 Code graph analysis (7)
crates/basilica-tui/src/ui/screens/billing.rs (5)
crates/basilica-tui/src/ui/theme.rs (6)
text(75-77)header(142-147)default(207-209)text_muted(80-82)text_accent(93-95)text_warning(103-105)crates/basilica-tui/src/ui/widgets/sparkline.rs (1)
text_sparkline(37-59)crates/basilica-tui/src/ui/components/header.rs (1)
header_height(149-151)crates/basilica-tui/src/ui/components/footer.rs (1)
footer_height(140-142)crates/basilica-tui/src/app/core.rs (1)
new(293-418)
crates/basilica-sdk/src/auth/oauth_flow.rs (2)
crates/basilica-sdk/src/auth/callback_server.rs (1)
new(53-55)crates/basilica-sdk/src/auth/types.rs (1)
new(47-52)
crates/basilica-tui/src/ui/screens/marketplace.rs (3)
crates/basilica-tui/src/ui/theme.rs (8)
text(75-77)header(142-147)default(207-209)text_muted(80-82)keybind(168-173)text_bold(85-90)text_accent(93-95)text_warning(103-105)crates/basilica-tui/src/ui/components/header.rs (1)
header_height(149-151)crates/basilica-tui/src/ui/components/footer.rs (1)
render_footer_ctx(80-137)
crates/basilica-tui/src/ui/screens/deployments.rs (4)
crates/basilica-tui/src/ui/theme.rs (4)
text(75-77)header(142-147)default(207-209)text_accent(93-95)crates/basilica-tui/src/ui/components/header.rs (1)
header_height(149-151)crates/basilica-tui/src/ui/components/footer.rs (1)
footer_height(140-142)crates/basilica-tui/src/data/user.rs (1)
new(95-97)
crates/basilica-tui/src/ui/screens/rentals.rs (3)
crates/basilica-tui/src/ui/theme.rs (5)
text(75-77)header(142-147)default(207-209)text_muted(80-82)keybind(168-173)crates/basilica-tui/src/ui/components/header.rs (1)
header_height(149-151)crates/basilica-tui/src/ui/components/footer.rs (1)
footer_height(140-142)
crates/basilica-tui/src/ui/mod.rs (2)
crates/basilica-tui/src/app/core.rs (1)
new(293-418)crates/basilica-tui/src/events/tick.rs (1)
new(14-19)
crates/basilica-tui/src/ui/components/dialog.rs (2)
crates/basilica-tui/src/ui/theme.rs (5)
text(75-77)default(207-209)text_accent(93-95)text_muted(80-82)keybind(168-173)crates/basilica-tui/src/ui/mod.rs (1)
new(25-29)
🪛 markdownlint-cli2 (0.18.1)
crates/basilica-tui/README.md
61-61: Tables should be surrounded by blank lines
(MD058, blanks-around-tables)
71-71: Tables should be surrounded by blank lines
(MD058, blanks-around-tables)
83-83: Tables should be surrounded by blank lines
(MD058, blanks-around-tables)
95-95: Tables should be surrounded by blank lines
(MD058, blanks-around-tables)
103-103: Tables should be surrounded by blank lines
(MD058, blanks-around-tables)
113-113: Tables should be surrounded by blank lines
(MD058, blanks-around-tables)
155-155: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🔇 Additional comments (27)
scripts/billing/Dockerfile (1)
30-30: LGTM! Correctly excludes the new TUI crate from the billing build.The addition of the
basilica-tuiexclusion aligns with the existing pattern and ensures the billing Docker build remains isolated from workspace members it doesn't depend on.scripts/payments/Dockerfile (1)
30-30: LGTM!The addition correctly excludes the new
basilica-tuiworkspace member from the payments service build, mirroring the pattern used for other unrelated crates.justfile (1)
22-48: LGTM! Good improvements to the build commands.The addition of
set -efor strict error handling and the newfix-verifytarget that runs clippy twice (fix then verify) are solid improvements that will catch more issues during development.crates/basilica-tui/Cargo.toml (1)
1-49: LGTM! Well-structured crate manifest.The dependencies are appropriate for a TUI application. Good use of workspace dependencies for consistency and local path dependencies for internal crates.
crates/basilica-tui/src/ui/screens/billing.rs (1)
16-273: LGTM! Well-structured billing screen with appropriate placeholder data.The implementation correctly handles various states (loading, not connected, empty data) and uses theme styling consistently. The TODO comments at lines 113 and 148 appropriately document that spending data and deposit address will come from the API in the future.
crates/basilica-tui/src/ui/screens/startup.rs (1)
14-117: LGTM! Clean startup screen implementation.The mode selection UI is well-structured with proper centering, theme integration, and clear visual feedback for the selected option. The
togglemethod is straightforward and correct.crates/basilica-tui/src/ui/screens/rentals.rs (1)
256-269: LGTM! Clean uptime formatting logic.The
format_uptimefunction correctly converts minutes to human-readable format with proper threshold handling.crates/basilica-tui/src/ui/screens/deployments.rs (1)
1-216: LGTM! Deployments screen implementation is clean.The rendering logic correctly handles empty states, applies status-based styling, and manages table state. The structure mirrors the rentals screen effectively. No string slicing by byte index or other critical issues detected.
crates/basilica-tui/src/ui/mod.rs (1)
93-97: LGTM! Proper cleanup in Drop implementation.The Drop implementation correctly calls
exit()to restore the terminal state, ignoring errors since we're in a destructor. This prevents leaving the terminal in a broken state if the app panics.crates/basilica-tui/src/ui/components/dialog.rs (1)
827-1250: LGTM! Comprehensive test coverage for dialog functionality.The test suite thoroughly validates dialog creation, state transitions, keyboard navigation, input handling, and result generation across all dialog types. Well-structured and maintainable tests.
crates/basilica-tui/src/app/core.rs (1)
1447-1512: SSH action implementation is well-structured.The
ssh_into_rentalmethod correctly validates rental state, extracts SSH connection details, and suspends the TUI to run the SSH session. Good error handling and user feedback.crates/basilica-sdk/src/auth/oauth_flow.rs (2)
250-257: The sharedrefresh_access_tokenfunction exists incrates/basilica-sdk/src/auth/refresh.rswith the correct signature, acceptingrefresh_token: &str,client_id: Option<&str>, andtoken_endpoint: Option<&str>. The function is properly exported in the module and the method call correctly passes the required parameters. The refactoring to delegate to the shared function is valid and improves code reuse.
202-247: Makingexchange_code_for_tokenpublic is a safe design choice for future extensibility.The method is currently only called internally via
start_flow(), which properly initializes PKCE throughbuild_auth_url(). While the method is now public to support custom OAuth flows (e.g., for TUI callback handling), there are no external callers that bypass the PKCE initialization sequence. The defensive error handling at lines 205-208 will catch any misuse where a caller forgets to initialize the code verifier, making this a well-architected public API.crates/basilica-tui/src/ui/screens/settings.rs (9)
1-12: LGTM! Clean module structure.The imports are well-organized and the module documentation clearly describes the screen's purpose.
14-60: LGTM! Well-structured enum with navigation helpers.The cyclic navigation pattern (wrapping from Theme back to Auth and vice versa) provides good UX for tab switching.
62-93: LGTM! Appropriate state structures.The data structures clearly represent the settings screen state. The absence of
DefaultonTokenInfoandSshKeyInfois appropriate since these are always constructed with meaningful data.
95-113: LGTM! Clean layout structure.The four-section vertical layout (header, tabs, content, footer) is well-organized and uses appropriate constraints.
145-154: LGTM! Clean dispatcher pattern.The exhaustive match ensures all settings sections have a corresponding render function.
317-393: LGTM! SSH keys section follows consistent pattern.The implementation mirrors the tokens section structure with appropriate SSH key-specific data (name, fingerprint, added date) and actions.
395-432: LGTM! Theme section appropriately scoped for WIP.The hardcoded theme list with only "Dark" available and others marked "coming soon" is a reasonable approach for a work-in-progress feature. The selection indicators provide clear visual feedback.
434-455: LGTM! Clear keybinding documentation.The footer provides essential navigation hints with appropriate styling. The top-only border cleanly separates it from the content area.
230-315: The tokens table is well-structured with appropriate empty state handling and action hints. The API usage at lines 275-282 correctly implements Table::new for ratatui 0.29.0, which accepts rows and an iterable of width constraints as parameters. No changes needed.crates/basilica-sdk/src/auth/device_flow.rs (5)
6-6: LGTM! Import organization improved.Consolidating TokenSet import and adding tracing support improves code organization and observability.
Also applies to: 9-9
46-47: LGTM! Appropriate handling of unused protocol field.The
#[allow(dead_code)]annotation correctly silences warnings for thescopefield that's part of the OAuth response but not actively used.
101-108: LGTM! Clean API separation.The
get_user_instructionsmethod provides a clean abstraction that separates the internal OAuth response from the user-facing instruction data.
154-157: LGTM! Improved observability.Replacing
println!with structureddebug!logging is appropriate for library code and enables better log management.
183-228: LGTM! Improved error handling and observability.The changes to
handle_poll_responseimprove the code:
- Returning
Ok(None)for pending states is cleaner- Error descriptions are handled gracefully with
unwrap_or_default()- Success logging provides better observability
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
crates/basilica-tui/src/ui/screens/marketplace.rs (1)
105-108: GPU type filter order is non-deterministic.This issue was already flagged in a previous review.
HashMapiteration order is arbitrary and may cause the filter list to visually jump around between renders.crates/basilica-tui/src/config.rs (1)
133-140: Silent failure when config directory is unavailable.The
save_default()method returnsOk(())whenProjectDirs::from()returnsNone, causing the config to not be saved without any indication to the caller. This matches a previous review comment and should be addressed.🔎 Proposed fix (as suggested in previous review)
pub fn save_default(&self) -> Result<()> { - if let Some(config_dir) = directories::ProjectDirs::from("ai", "basilica", "basilica-tui") { - let config_path = config_dir.config_dir().join("config.toml"); - self.save(&config_path)?; - } - Ok(()) + let config_dir = directories::ProjectDirs::from("ai", "basilica", "basilica-tui") + .ok_or_else(|| anyhow::anyhow!("Could not determine config directory"))?; + let config_path = config_dir.config_dir().join("config.toml"); + self.save(&config_path) }
🧹 Nitpick comments (8)
crates/basilica-tui/src/ui/screens/billing.rs (2)
230-234: Potential index-out-of-bounds when applying row style.
ctx.selected_indexis compared directly without clamping to the transactions range. Ifselected_indexis set from another screen (e.g., marketplace) and exceeds the transaction count, no row will be highlighted—which may be acceptable but could also lead to confusion. Consider aligning with the pattern used inmarketplace.rs(lines 234-237) which clamps the index.Suggested consistency improvement
- let row_style = if i == ctx.selected_index { + let effective_index = ctx.selected_index.min(user_data.transactions.len().saturating_sub(1)); + let row_style = if i == effective_index { theme.selected_row() } else { theme.text() };
257-275: Consider usingrender_stateful_widgetfor proper table selection.Unlike
marketplace.rs, this table usesrender_widgetwithoutTableState. This meansrow_highlight_styleon line 273 won't take effect since ratatui requires stateful rendering for built-in highlight behavior. Either remove the unused.row_highlight_style()call or switch to stateful rendering for consistency.Option 1: Use stateful rendering (consistent with marketplace.rs)
+ let mut state = TableState::default(); + if !user_data.transactions.is_empty() { + state.select(Some( + ctx.selected_index + .min(user_data.transactions.len().saturating_sub(1)), + )); + } + - frame.render_widget(table, area); + frame.render_stateful_widget(table, area, &mut state);You'll also need to add
TableStateto the imports on line 7:use ratatui::widgets::{Block, Borders, Cell, Paragraph, Row, Table, TableState};Option 2: Remove unused row_highlight_style
.block( Block::default() .borders(Borders::ALL) .border_style(theme.border()) .title(Span::styled(title, theme.block_title())), - ) - .row_highlight_style(theme.selected_row()); + );scripts/tui-test/start.sh (1)
25-41: Consider checking HTTP status code in the health check.The current health check only verifies that curl can connect, but doesn't validate the HTTP response code. The API could be returning 500 errors and the script would still report success.
🔎 Suggested improvement to verify HTTP 200 status
- if curl -s http://localhost:8000/health > /dev/null 2>&1; then + if curl -s -f -o /dev/null http://localhost:8000/health 2>&1; then echo "✅ API is ready!" break fiThe
-fflag makes curl fail on HTTP error codes (4xx, 5xx), ensuring the health check only passes when the API returns a successful response.crates/basilica-tui/README.md (2)
63-123: Consider adding blank lines around tables for better Markdown formatting.The keybinding tables are well-organized, but markdownlint suggests adding blank lines before and after tables for better compliance with Markdown best practices.
As per static analysis hints, tables at lines 64, 74, 86, 98, 106, and 116 should be surrounded by blank lines (MD058).
Example fix for the first table
### Startup Screen + | Key | Action | |-----|--------| ... + ### Global
161-178: Consider adding a language specifier to the architecture diagram.The fenced code block showing the architecture would benefit from a language specifier for better syntax highlighting and Markdown compliance.
As per static analysis hints (MD040), consider adding
textorplaintextas the language:Suggested fix
-``` +```text src/ ├── app/ ...crates/basilica-tui/tests/integration.rs (2)
14-14: Reduce code duplication: Extract URL retrieval into a helper.The pattern
std::env::var("BASILICA_API_URL").unwrap_or_else(|_| "http://localhost:8000".into())is repeated in every test function. Consider extracting this into a helper function.🔎 Proposed refactor
Add a helper function after
api_available():+fn get_api_url() -> String { + std::env::var("BASILICA_API_URL").unwrap_or_else(|_| "http://localhost:8000".into()) +} + /// Skip test if API is not available macro_rules! require_api {Then use it in tests:
#[tokio::test] async fn test_api_health() { require_api!(); - let url = std::env::var("BASILICA_API_URL").unwrap_or_else(|_| "http://localhost:8000".into()); + let url = get_api_url(); let client = reqwest::Client::new();Also applies to: 43-43, 59-59, 80-80, 101-101, 122-122
39-53: Consider marking tests with #[ignore] for optional execution.These integration tests require an external API service. Consider marking them with
#[ignore]so they don't run by default in CI/CD but can be explicitly run when needed withcargo test -- --ignored.🔎 Proposed change
#[tokio::test] +#[ignore = "requires external API service"] async fn test_api_health() { require_api!();Apply similar changes to other test functions.
Also applies to: 55-74, 76-95, 97-116, 118-137
crates/basilica-tui/src/config.rs (1)
2-2: Module-wide dead code allowance may hide incomplete implementation.The
#![allow(dead_code)]attribute at the module level suppresses warnings for all unused code. Consider removing this once the TUI implementation is complete, or apply it more selectively to specific items that are intentionally unused.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (16)
crates/basilica-api/README.mdcrates/basilica-api/src/config/mod.rscrates/basilica-api/src/lib.rscrates/basilica-api/src/main.rscrates/basilica-api/src/mock.rscrates/basilica-api/src/server.rscrates/basilica-tui/Cargo.tomlcrates/basilica-tui/README.mdcrates/basilica-tui/src/config.rscrates/basilica-tui/src/ui/screens/billing.rscrates/basilica-tui/src/ui/screens/marketplace.rscrates/basilica-tui/tests/integration.rsscripts/README.mdscripts/tui-test/README.mdscripts/tui-test/docker-compose.ymlscripts/tui-test/start.sh
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/basilica-tui/Cargo.toml
🧰 Additional context used
🧬 Code graph analysis (3)
crates/basilica-api/src/main.rs (1)
crates/basilica-api/src/config/mod.rs (1)
load(262-286)
crates/basilica-tui/src/ui/screens/marketplace.rs (2)
crates/basilica-tui/src/ui/components/header.rs (2)
header_height(149-151)render_header_ctx(83-146)crates/basilica-tui/src/ui/components/footer.rs (2)
footer_height(140-142)render_footer_ctx(80-137)
crates/basilica-tui/src/config.rs (2)
crates/basilica-tui/src/ui/theme.rs (1)
default(207-209)crates/basilica-tui/src/data/streams.rs (4)
default(204-206)default(266-268)new(68-76)new(241-246)
🪛 Checkov (3.2.334)
scripts/tui-test/docker-compose.yml
[medium] 41-42: Basic Auth Credentials
(CKV_SECRET_4)
🪛 markdownlint-cli2 (0.18.1)
crates/basilica-tui/README.md
64-64: Tables should be surrounded by blank lines
(MD058, blanks-around-tables)
74-74: Tables should be surrounded by blank lines
(MD058, blanks-around-tables)
86-86: Tables should be surrounded by blank lines
(MD058, blanks-around-tables)
98-98: Tables should be surrounded by blank lines
(MD058, blanks-around-tables)
106-106: Tables should be surrounded by blank lines
(MD058, blanks-around-tables)
116-116: Tables should be surrounded by blank lines
(MD058, blanks-around-tables)
161-161: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🔇 Additional comments (26)
crates/basilica-tui/src/ui/screens/billing.rs (4)
16-50: LGTM!The layout structure is well-organized with proper use of header/footer height functions and a sensible two-column split for the billing content.
52-108: LGTM!The balance rendering correctly handles the three states (has balance, loading, not connected) with appropriate styling differentiation.
110-143: Static spending data acknowledged.The TODO comment on line 113 indicates this is placeholder data. The implementation is correct for now.
145-204: LGTM!The deposit info rendering handles connected/disconnected states appropriately, and the address truncation logic is correct.
crates/basilica-tui/src/ui/screens/marketplace.rs (3)
14-39: LGTM!The layout structure with header, action bar, content area (filters + GPU list), and footer is well-organized and follows the established pattern.
41-80: LGTM!The action bar cleanly implements conditional styling based on selection and filter state, providing good visual feedback for available actions.
138-241: LGTM!The GPU list rendering correctly handles all three empty states (loading, not connected, no GPUs), implements proper stateful table rendering with index clamping (lines 234-237), and provides good visual differentiation for source types.
crates/basilica-api/README.md (1)
18-42: LGTM! Clear and helpful documentation.The Dev Mode section effectively documents the three ways to enable dev mode and clearly explains its behavior and use cases. This will help developers get started quickly without needing external dependencies.
scripts/README.md (2)
54-56: Good addition to the infrastructure section.The tui-test entry is appropriately placed alongside localtest, making it easy for developers to find testing environments.
91-104: Clear and actionable TUI testing documentation.The testing workflow is well-documented with practical examples for both API-connected and dev-mode testing.
crates/basilica-api/src/lib.rs (1)
26-26: LGTM! Appropriate module exposure for dev mode.The mock module export enables development and testing workflows without requiring external Bittensor/Validator connections.
scripts/tui-test/README.md (1)
1-80: Excellent testing documentation.The README provides comprehensive guidance for TUI testing with clear options for different scenarios (dev mode, local API, integration tests) and helpful troubleshooting tips.
scripts/tui-test/start.sh (1)
43-59: Clear and helpful usage instructions.The output provides all the necessary information for developers to start using the TUI against the local API.
crates/basilica-tui/README.md (2)
1-62: Excellent and comprehensive TUI documentation.The README provides thorough documentation of features, installation, usage, and the startup screen with clear examples and explanations.
180-249: Clear documentation for dependencies, development, and testing.The sections on dependencies, development workflow, and integration testing provide all the necessary information for contributors.
crates/basilica-api/src/config/mod.rs (1)
208-211: LGTM! Clean dev mode configuration field.The
dev_modefield is appropriately added withserde(default)to allow backward compatibility with existing config files, and the documentation clearly explains its purpose.crates/basilica-api/src/main.rs (2)
21-24: Well-documented CLI flag for dev mode.The
--devflag is clearly documented and aligns with the README description.
83-92: Clean dev mode override logic with proper precedence.The implementation correctly establishes precedence (config file < environment variable < CLI flag) and provides clear feedback when dev mode is active. The environment variable support is automatically handled by figment's
Env::prefixed("BASILICA_API_")in the config loading logic.crates/basilica-tui/tests/integration.rs (1)
139-160: LGTM: Unit tests provide good coverage.The unit test module appropriately validates TOML-based config parsing with direct deserialization tests, ensuring the config structure works as expected.
scripts/tui-test/docker-compose.yml (1)
1-64: LGTM: Docker Compose configuration is appropriate for testing.The configuration properly orchestrates Postgres and API services for TUI integration testing. The dev-mode credentials and disabled features are appropriate for a local test environment.
Note: The static analysis hint about "Basic Auth Credentials" is a false positive—the database connection string is expected and appropriate for test environments.
crates/basilica-api/src/server.rs (4)
570-576: LGTM: Clean separation of dev and production initialization.The branching logic cleanly separates dev mode (mock validator) from production mode (Bittensor discovery), improving maintainability and testability.
1232-1264: LGTM: Dev mode initialization properly bypasses external dependencies.The dev mode initialization correctly uses mock constants and avoids Bittensor network calls, enabling local development and testing without external services.
1266-1338: LGTM: Production mode includes proper validator discovery and validation.The production initialization properly:
- Validates configuration
- Discovers validators via Bittensor metagraph
- Verifies validator permits
- Constructs proper endpoints
824-907: LGTM: Health checks appropriately skipped in dev mode.The conditional health check startup correctly:
- Spawns health check tasks only in production mode
- Logs dev mode bypass clearly
- Maintains proper task lifecycle
crates/basilica-tui/src/config.rs (2)
88-97: LGTM: Well-structured configuration with sensible defaults.The configuration implementation provides:
- Clear structure with serde support
- Environment variable override for API URL
- Reasonable default values
- Proper file I/O with directory creation
Also applies to: 99-147
149-260: LGTM: Comprehensive test coverage.The test suite validates:
- Default configuration values
- Serialization/deserialization
- File-based loading
- Save/load round trips
- Theme variants
- Missing file fallback behavior
| //! the Bittensor metagraph discovery at startup. | ||
|
|
||
| /// Mock validator hotkey for dev mode | ||
| pub const DEV_VALIDATOR_HOTKEY: &str = "5DevMockValidatorHotkey000000000000000000000000000"; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify if there are any SS58 address validation functions in the codebase
rg -nP --type=rust -C3 'ss58|SS58|substrate.*address|verify.*hotkey'Repository: one-covenant/basilica
Length of output: 50379
🏁 Script executed:
# Also check the specific file and surrounding context
cat -n crates/basilica-api/src/mock.rs | head -30Repository: one-covenant/basilica
Length of output: 969
🏁 Script executed:
# Search for how this constant is used
rg -nP 'DEV_VALIDATOR_HOTKEY' --type=rust -B2 -A2Repository: one-covenant/basilica
Length of output: 1400
🏁 Script executed:
# Check for any hotkey validation or SS58 address patterns in the codebase
rg -nP '5[A-Za-z0-9]{47}|hotkey.*valid|address.*format' --type=rust | head -20Repository: one-covenant/basilica
Length of output: 2763
Replace mock hotkey with a valid SS58 test address.
The current mock hotkey "5DevMockValidatorHotkey000000000000000000000000000" (50 characters) fails SS58 validation requirements (47-48 characters, valid checksum). Use an existing test address like "5GrwvaEF5zXb26Fz9rcQpDWS57CtERHpNehXCPcNoHGKutQY" instead, which is already used throughout the test suite and will pass validation if the string is ever passed through Hotkey::new().
🤖 Prompt for AI Agents
In crates/basilica-api/src/mock.rs around line 14, the constant
DEV_VALIDATOR_HOTKEY is set to an invalid 50‑character mock string that fails
SS58 validation; replace it with a known valid SS58 test address (for example
"5GrwvaEF5zXb26Fz9rcQpDWS57CtERHpNehXCPcNoHGKutQY") so Hotkey::new() will accept
it — update the constant value to that valid test address and run tests that
exercise Hotkey::new() to confirm validation passes.
1 participant
Summary
Brief description of what this PR does.
Related Issues
Closes #(issue number)
Type of Change
Changes Made
List the main changes in this PR:
Testing
How Has This Been Tested?
Describe the tests you ran to verify your changes.
cargo test)Test Configuration
Checklist
cargo fmtto format my codecargo clippyand addressed all warningsAdditional Context
Add any other context about the PR here.
Summary by CodeRabbit
New Features
Documentation
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.