From 8cbf902be41cb6d40e9f6583495543fd25ed6011 Mon Sep 17 00:00:00 2001 From: Eric Curtin Date: Thu, 4 Jun 2026 11:56:10 +0100 Subject: [PATCH] refactor: deduplicate shared driver and TUI helpers Move supervisor_image_tag/supervisor_image_should_refresh from openshell-driver-docker and openshell-driver-podman into openshell-core::driver_utils, which already hosts shared driver utilities. Both drivers now import the single canonical copy. Replace the private unix_epoch_millis() function in openshell-server::ssh_sessions with openshell_core::time::now_ms, which exists precisely to avoid local reimplementations of this pattern. Extract a draw_empty_message() helper into openshell-tui::ui::mod and replace the six copy-pasted Rect+Paragraph blocks across dashboard, sandboxes, providers (x2), global_settings, and sandbox_settings with calls to the shared helper. --- crates/openshell-core/src/driver_utils.rs | 35 +++++++++++++++++++ crates/openshell-driver-docker/src/lib.rs | 19 +--------- crates/openshell-driver-podman/src/driver.rs | 18 +--------- crates/openshell-server/src/ssh_sessions.rs | 17 ++------- crates/openshell-tui/src/ui/dashboard.rs | 12 ++----- .../openshell-tui/src/ui/global_settings.rs | 11 ++---- crates/openshell-tui/src/ui/mod.rs | 16 +++++++++ crates/openshell-tui/src/ui/providers.rs | 20 ++--------- .../openshell-tui/src/ui/sandbox_settings.rs | 10 ++---- crates/openshell-tui/src/ui/sandboxes.rs | 11 ++---- 10 files changed, 68 insertions(+), 101 deletions(-) diff --git a/crates/openshell-core/src/driver_utils.rs b/crates/openshell-core/src/driver_utils.rs index a03e8a691..09fb82dce 100644 --- a/crates/openshell-core/src/driver_utils.rs +++ b/crates/openshell-core/src/driver_utils.rs @@ -89,3 +89,38 @@ pub fn sandbox_log_level(sandbox: &DriverSandbox, default_level: &str) -> String .unwrap_or(default_level) .to_string() } + +// --------------------------------------------------------------------------- +// Supervisor image helpers (shared by Docker and Podman drivers) +// --------------------------------------------------------------------------- + +/// Return the tag portion of a supervisor image reference, or `None` if the +/// reference uses a digest (`@sha256:...`). +/// +/// Examples: +/// - `"ghcr.io/org/image:1.2.3"` → `Some("1.2.3")` +/// - `"ghcr.io/org/image:latest"` → `Some("latest")` +/// - `"ghcr.io/org/image"` → `Some("latest")` (implied tag) +/// - `"ghcr.io/org/image@sha256:abc"` → `None` (pinned by digest) +/// - `"ghcr.io/org/image:"` → `None` (empty tag) +pub fn supervisor_image_tag(image: &str) -> Option<&str> { + if image.contains('@') { + return None; + } + + let image_name = image.rsplit('/').next().unwrap_or(image); + image_name + .rsplit_once(':') + .map_or(Some("latest"), |(_, tag)| { + if tag.is_empty() { None } else { Some(tag) } + }) +} + +/// Return `true` if the supervisor image should be refreshed before each use. +/// +/// Mutable tags (`dev`, `latest`) are always re-pulled so that the running +/// container tracks the latest pushed version. Digest-pinned references and +/// all other versioned tags are treated as immutable and pulled at most once. +pub fn supervisor_image_should_refresh(image: &str) -> bool { + matches!(supervisor_image_tag(image), Some("dev" | "latest")) +} diff --git a/crates/openshell-driver-docker/src/lib.rs b/crates/openshell-driver-docker/src/lib.rs index e30ee7754..864d91f22 100644 --- a/crates/openshell-driver-docker/src/lib.rs +++ b/crates/openshell-driver-docker/src/lib.rs @@ -23,7 +23,7 @@ use openshell_core::config::{ }; use openshell_core::driver_utils::{ LABEL_MANAGED_BY, LABEL_MANAGED_BY_VALUE, LABEL_SANDBOX_ID, LABEL_SANDBOX_NAME, - LABEL_SANDBOX_NAMESPACE, SUPERVISOR_IMAGE_BINARY_PATH, + LABEL_SANDBOX_NAMESPACE, SUPERVISOR_IMAGE_BINARY_PATH, supervisor_image_should_refresh, }; use openshell_core::gpu::cdi_gpu_device_ids; use openshell_core::progress::{ @@ -2572,23 +2572,6 @@ async fn extract_supervisor_bin_from_image(docker: &Docker, image: &str) -> Core Ok(cache_path) } -fn supervisor_image_should_refresh(image: &str) -> bool { - matches!(supervisor_image_tag(image), Some("dev" | "latest")) -} - -fn supervisor_image_tag(image: &str) -> Option<&str> { - if image.contains('@') { - return None; - } - - let image_name = image.rsplit('/').next().unwrap_or(image); - image_name - .rsplit_once(':') - .map_or(Some("latest"), |(_, tag)| { - if tag.is_empty() { None } else { Some(tag) } - }) -} - async fn pull_supervisor_image(docker: &Docker, image: &str) -> CoreResult<()> { let mut stream = docker.create_image( Some(CreateImageOptions { diff --git a/crates/openshell-driver-podman/src/driver.rs b/crates/openshell-driver-podman/src/driver.rs index e2deb1c63..1358d8945 100644 --- a/crates/openshell-driver-podman/src/driver.rs +++ b/crates/openshell-driver-podman/src/driver.rs @@ -10,6 +10,7 @@ use crate::watcher::{ self, WatchStream, driver_sandbox_from_inspect, driver_sandbox_from_list_entry, }; use openshell_core::ComputeDriverError; +use openshell_core::driver_utils::supervisor_image_should_refresh; use openshell_core::proto::compute::v1::{DriverSandbox, GetCapabilitiesResponse}; use std::path::PathBuf; use std::time::Duration; @@ -592,23 +593,6 @@ fn supervisor_image_pull_policy(image: &str) -> &'static str { } } -fn supervisor_image_should_refresh(image: &str) -> bool { - matches!(supervisor_image_tag(image), Some("dev" | "latest")) -} - -fn supervisor_image_tag(image: &str) -> Option<&str> { - if image.contains('@') { - return None; - } - - let image_name = image.rsplit('/').next().unwrap_or(image); - image_name - .rsplit_once(':') - .map_or(Some("latest"), |(_, tag)| { - if tag.is_empty() { None } else { Some(tag) } - }) -} - /// Check whether the current user has subuid/subgid ranges configured. /// /// Rootless Podman requires entries in `/etc/subuid` and `/etc/subgid` for diff --git a/crates/openshell-server/src/ssh_sessions.rs b/crates/openshell-server/src/ssh_sessions.rs index e55c1b6af..752fee1c0 100644 --- a/crates/openshell-server/src/ssh_sessions.rs +++ b/crates/openshell-server/src/ssh_sessions.rs @@ -5,6 +5,7 @@ use openshell_core::ObjectId; use openshell_core::proto::SshSession; +use openshell_core::time::now_ms; use prost::Message; use std::sync::Arc; use std::time::Duration; @@ -33,7 +34,7 @@ pub fn spawn_session_reaper(store: Arc, interval: Duration) { } async fn reap_expired_sessions(store: &Store) -> Result<(), String> { - let now_ms = unix_epoch_millis(); + let now_ms = now_ms(); let records = store .list(SshSession::object_type(), 1000, 0) @@ -68,16 +69,6 @@ async fn reap_expired_sessions(store: &Store) -> Result<(), String> { Ok(()) } -fn unix_epoch_millis() -> i64 { - i64::try_from( - std::time::SystemTime::now() - .duration_since(std::time::UNIX_EPOCH) - .unwrap_or_default() - .as_millis(), - ) - .unwrap_or(i64::MAX) -} - #[cfg(test)] mod tests { use super::*; @@ -103,10 +94,6 @@ mod tests { } } - fn now_ms() -> i64 { - unix_epoch_millis() - } - #[tokio::test] async fn reaper_deletes_expired_sessions() { let store = test_store().await; diff --git a/crates/openshell-tui/src/ui/dashboard.rs b/crates/openshell-tui/src/ui/dashboard.rs index 43ae6a937..1de2578fb 100644 --- a/crates/openshell-tui/src/ui/dashboard.rs +++ b/crates/openshell-tui/src/ui/dashboard.rs @@ -4,8 +4,9 @@ use ratatui::Frame; use ratatui::layout::{Constraint, Direction, Layout, Rect}; use ratatui::text::{Line, Span}; -use ratatui::widgets::{Block, Borders, Cell, Padding, Paragraph, Row, Table}; +use ratatui::widgets::{Block, Borders, Cell, Padding, Row, Table}; +use super::draw_empty_message; use crate::app::{App, Focus, MiddlePaneTab}; pub fn draw(frame: &mut Frame<'_>, app: &App, area: Rect) { @@ -118,13 +119,6 @@ fn draw_gateway_list(frame: &mut Frame<'_>, app: &App, area: Rect) { frame.render_widget(table, area); if app.gateways.is_empty() { - let inner = Rect { - x: area.x + 2, - y: area.y + 2, - width: area.width.saturating_sub(4), - height: area.height.saturating_sub(3), - }; - let msg = Paragraph::new(Span::styled(" No gateways found.", t.muted)); - frame.render_widget(msg, inner); + draw_empty_message(frame, area, " No gateways found.", t.muted); } } diff --git a/crates/openshell-tui/src/ui/global_settings.rs b/crates/openshell-tui/src/ui/global_settings.rs index 9e47fd8d6..9e2b927e9 100644 --- a/crates/openshell-tui/src/ui/global_settings.rs +++ b/crates/openshell-tui/src/ui/global_settings.rs @@ -6,6 +6,8 @@ use ratatui::layout::{Constraint, Rect}; use ratatui::text::{Line, Span}; use ratatui::widgets::{Block, Borders, Cell, Clear, Padding, Paragraph, Row, Table}; +use super::draw_empty_message; + use crate::app::{App, MiddlePaneTab}; pub fn draw(frame: &mut Frame<'_>, app: &App, area: Rect, focused: bool) { @@ -72,14 +74,7 @@ pub fn draw(frame: &mut Frame<'_>, app: &App, area: Rect, focused: bool) { frame.render_widget(table, area); if app.global_settings.is_empty() { - let inner = Rect { - x: area.x + 2, - y: area.y + 2, - width: area.width.saturating_sub(4), - height: area.height.saturating_sub(3), - }; - let msg = Paragraph::new(Span::styled(" No settings available.", t.muted)); - frame.render_widget(msg, inner); + draw_empty_message(frame, area, " No settings available.", t.muted); } // Draw edit overlay if active. diff --git a/crates/openshell-tui/src/ui/mod.rs b/crates/openshell-tui/src/ui/mod.rs index b0d0037ef..17d862840 100644 --- a/crates/openshell-tui/src/ui/mod.rs +++ b/crates/openshell-tui/src/ui/mod.rs @@ -16,6 +16,7 @@ mod splash; use ratatui::Frame; use ratatui::layout::{Constraint, Direction, Layout, Rect}; +use ratatui::style::Style; use ratatui::text::{Line, Span}; use ratatui::widgets::{Block, Borders, Paragraph}; @@ -474,6 +475,21 @@ fn draw_command_bar(frame: &mut Frame<'_>, app: &App, area: Rect) { frame.render_widget(bar, area); } +/// Render an empty-state message inside a bordered panel. +/// +/// Calculates an inset [`Rect`] that clears the panel border and renders +/// `text` with the given `style`. Use this whenever a table or list has no +/// rows to display. +pub fn draw_empty_message(frame: &mut Frame<'_>, area: Rect, text: &str, style: Style) { + let inner = Rect { + x: area.x + 2, + y: area.y + 2, + width: area.width.saturating_sub(4), + height: area.height.saturating_sub(3), + }; + frame.render_widget(Paragraph::new(Span::styled(text, style)), inner); +} + /// Center a popup rectangle within `area` using absolute width and height (in /// terminal columns/rows). pub fn centered_rect(width: u16, height: u16, area: Rect) -> Rect { diff --git a/crates/openshell-tui/src/ui/providers.rs b/crates/openshell-tui/src/ui/providers.rs index a7bfb7d1e..06a092460 100644 --- a/crates/openshell-tui/src/ui/providers.rs +++ b/crates/openshell-tui/src/ui/providers.rs @@ -4,7 +4,7 @@ use ratatui::Frame; use ratatui::layout::{Constraint, Rect}; use ratatui::text::{Line, Span}; -use ratatui::widgets::{Block, Borders, Cell, Padding, Paragraph, Row, Table}; +use ratatui::widgets::{Block, Borders, Cell, Padding, Row, Table}; use crate::app::App; @@ -83,14 +83,7 @@ pub fn draw(frame: &mut Frame<'_>, app: &App, area: Rect, focused: bool) { frame.render_widget(table, area); if app.provider_count == 0 { - let inner = Rect { - x: area.x + 2, - y: area.y + 2, - width: area.width.saturating_sub(4), - height: area.height.saturating_sub(3), - }; - let msg = Paragraph::new(Span::styled(" No providers. Press [c] to create.", t.muted)); - frame.render_widget(msg, inner); + super::draw_empty_message(frame, area, " No providers. Press [c] to create.", t.muted); } } @@ -172,13 +165,6 @@ fn draw_v2(frame: &mut Frame<'_>, app: &App, area: Rect, focused: bool) { frame.render_widget(table, area); if app.provider_count == 0 { - let inner = Rect { - x: area.x + 2, - y: area.y + 2, - width: area.width.saturating_sub(4), - height: area.height.saturating_sub(3), - }; - let msg = Paragraph::new(Span::styled(" No providers found.", t.muted)); - frame.render_widget(msg, inner); + super::draw_empty_message(frame, area, " No providers found.", t.muted); } } diff --git a/crates/openshell-tui/src/ui/sandbox_settings.rs b/crates/openshell-tui/src/ui/sandbox_settings.rs index 8ac5eb687..beb483b11 100644 --- a/crates/openshell-tui/src/ui/sandbox_settings.rs +++ b/crates/openshell-tui/src/ui/sandbox_settings.rs @@ -6,6 +6,7 @@ use ratatui::layout::{Constraint, Rect}; use ratatui::text::{Line, Span}; use ratatui::widgets::{Block, Borders, Cell, Clear, Padding, Paragraph, Row, Table}; +use super::draw_empty_message; use crate::app::{App, SandboxPolicyTab, SettingScope}; pub fn draw(frame: &mut Frame<'_>, app: &App, area: Rect) { @@ -83,14 +84,7 @@ pub fn draw(frame: &mut Frame<'_>, app: &App, area: Rect) { frame.render_widget(table, area); if app.sandbox_settings.is_empty() { - let inner = Rect { - x: area.x + 2, - y: area.y + 2, - width: area.width.saturating_sub(4), - height: area.height.saturating_sub(3), - }; - let msg = Paragraph::new(Span::styled(" No settings available.", t.muted)); - frame.render_widget(msg, inner); + draw_empty_message(frame, area, " No settings available.", t.muted); } // Overlays. diff --git a/crates/openshell-tui/src/ui/sandboxes.rs b/crates/openshell-tui/src/ui/sandboxes.rs index 3fb8e3a3b..6ace580b8 100644 --- a/crates/openshell-tui/src/ui/sandboxes.rs +++ b/crates/openshell-tui/src/ui/sandboxes.rs @@ -4,7 +4,7 @@ use ratatui::Frame; use ratatui::layout::{Constraint, Rect}; use ratatui::text::{Line, Span}; -use ratatui::widgets::{Block, Borders, Cell, Padding, Paragraph, Row, Table}; +use ratatui::widgets::{Block, Borders, Cell, Padding, Row, Table}; use crate::app::App; @@ -102,13 +102,6 @@ pub fn draw(frame: &mut Frame<'_>, app: &App, area: Rect, focused: bool) { frame.render_widget(table, area); if app.sandbox_count == 0 { - let inner = Rect { - x: area.x + 2, - y: area.y + 2, - width: area.width.saturating_sub(4), - height: area.height.saturating_sub(3), - }; - let msg = Paragraph::new(Span::styled(" No sandboxes. Press [c] to create.", t.muted)); - frame.render_widget(msg, inner); + super::draw_empty_message(frame, area, " No sandboxes. Press [c] to create.", t.muted); } }