Skip to content

feat: add runtime theme system - #86

Merged
Petyok merged 62 commits into
Petyok:developmentfrom
michabbb:feature/theme-system
Aug 11, 2026
Merged

feat: add runtime theme system#86
Petyok merged 62 commits into
Petyok:developmentfrom
michabbb:feature/theme-system

Conversation

@michabbb

Copy link
Copy Markdown
Contributor

Summary

This PR adds a complete runtime theme system to SSHub. Colours and styles are
no longer fixed renderer constants: every SSHub surface reads from a validated
TOML theme that can be inherited, previewed live, checked headlessly and
changed without rebuilding the application.

The system covers the dashboard, shared chrome, popups, forms, session views,
tunnels, SFTP, broadcast, audit, identities, settings, help, keybindings and
the startup animation. The embedded remote PTY is deliberately excluded from
theme painting, so a theme can never recolour or overwrite remote terminal
content.

Built-in theme preview

SSHub built-in theme preview

The video above previews the themes shipped in the binary. SSHub includes five
built-ins which work without any external files:

  • default preserves SSHub's original dark appearance and acts as the
    inheritance root.
  • summer is a bright, warm daylight theme and demonstrates light
    surfaces and subtle gradients.
  • aqua uses deep-water blues, cyan accents and a perimeter-lit focus
    frame.
  • fire combines charcoal surfaces with red, orange and gold while
    keeping semantic status colours distinct.
  • high-contrast uses white on pure black, saturated statuses and no
    decorative gradients.

All built-ins are embedded, pass through the same parser and validator as user
themes, and can be exported as editable TOML with sshub theme show.

What users can customise

Layered TOML themes

A theme can be as small as:

schema_version = 1
name = "My Theme"

Everything omitted is inherited from default. From there, authors can work
at four increasingly precise layers:

  1. Palette — define reusable named colours under [palette].
  2. Semantic core — override 23 stable meanings such as accent,
    background, surface, text, selection_bg, success, warning,
    error, connecting and unknown.
  3. Static gradients — define reusable multi-stop gradients under
    [gradients].
  4. Component roles — override any of the 234 published roles under
    [components] when a specific cell or surface needs to differ from its
    semantic fallback.

This makes broad changes cheap: changing semantic.accent recolours every
component that inherits the accent, while a single component can still be
fine-tuned independently.

Colour expressions and terminal-aware values

Theme values support:

  • #RRGGBB literals;
  • explicit { rgb = [r, g, b] } values;
  • qualified palette.* and semantic.* references;
  • brightness transforms from -1.0 to 1.0;
  • deterministic simulated opacity over an explicit RGB ground;
  • terminal to preserve the terminal emulator's own default colour;
  • auto to reset an inherited component override to its catalogue fallback;
  • native for tint roles such as distro logos, preserving their original
    asset colours.

Style roles independently control foreground, background and the modifiers
bold, dim, italic, underlined, reversed and crossed_out.

Inheritance and reset behaviour

User themes can extend any available theme by ID. Definitions are deep-merged
first and colour references are resolved afterwards, so a child can replace a
semantic colour and automatically update component styles inherited from its
parent. Cycles, missing parents and chains deeper than 16 are diagnosed.

auto provides a deliberate escape hatch: it removes an inherited component
customisation and restores that role's built-in semantic recipe. Individual
style fields and modifier lists can also be reset without duplicating the rest
of the parent definition.

Static gradients

Named gradients support multiple ordered stops and five directions:

  • horizontal
  • vertical
  • diagonal_down
  • diagonal_up
  • perimeter

Gradients can paint backgrounds, separators and frames. Perimeter gradients
follow the ring of a closed rectangular frame; validation rejects them on
roles that do not own a closed perimeter. Rendering is static, allocation-free
per cell, clipped to the intended surface and never applied to the remote PTY.

Runtime theme picker

The Settings overlay now contains a dedicated theme picker:

  • open it with Ctrl+H → Theme… → Enter;
  • moving through the list previews each theme on the entire live TUI;
  • an additional two-box sampler exposes important colours, gradients and
    focused/unfocused states;
  • Esc restores the theme that was active before opening the picker;
  • r reloads the themes directory;
  • only Enter persists appearance.active_theme to config.toml;
  • invalid themes remain visible with their diagnostics instead of silently
    disappearing;
  • themes containing unknown newer component roles remain usable in compatible
    runtime mode and show warnings;
  • selection and preview survive reloads, repairs and deleted files safely.

User themes live in ~/.config/sshub/themes/*.toml, or beneath
$SSHUB_CONFIG_DIR/themes/ when that environment variable is set. The file
name is the technical theme ID; built-in IDs are reserved.

Headless CLI

The PR adds a database-free, TUI-free sshub theme command family:

Command Behaviour
sshub theme list [--format plain|json] Lists built-in and user themes with source and validation state.
sshub theme show <id> [--resolved] [--format toml|json] Prints the documented source or a fully resolved standalone export.
sshub theme check <file> [--format plain|json] Runs strict validation using the same parser and resolver as the application.

Diagnostics collect independent problems in one run, include file, line and
column where available, and suggest close matches for misspelled keys and
sentinels. The CLI has stable exit-code behaviour for success, invalid input
and command misuse, and shell completions plus the man page include the new
commands.

Validation, compatibility and safety

The implementation introduces a versioned V1 schema with:

  • span-aware TOML parsing;
  • strict type checks for colour, style, paint and tint roles;
  • unknown-key and near-match diagnostics;
  • inheritance, reference and gradient depth limits;
  • file count, file size and identifier limits;
  • reserved built-in IDs;
  • strict mode for theme check and compatible mode for runtime loading;
  • transactional activation: a broken reload never replaces the active theme;
  • startup notices that preserve other degradation warnings;
  • an explicit, compiler-typed inventory proving that every published role has
    a productive renderer;
  • parity tests ensuring default reproduces the legacy appearance, with each
    intentional exception documented and tested.

Implementation overview

The new src/theme/ domain is split into focused stages:

  • catalog.rs defines semantic slots, role types, fallback recipes and the
    public component contract;
  • parse.rs builds span-aware definitions from TOML;
  • validate.rs checks schema rules and produces actionable diagnostics;
  • resolve.rs merges inheritance and resolves references, transforms,
    opacity, gradients and sentinels;
  • registry.rs discovers built-in and user themes without allowing user files
    to replace reserved IDs;
  • manager.rs owns the active and preview themes;
  • gradient.rs samples resolved static gradients;
  • builtins.rs embeds and verifies the five reference themes.

The TUI renderers now consume typed ColorRole, StyleRole, PaintRole and
TintRole values. Shared panel bundles keep titles, badges, borders and
backgrounds from the same role family, while buffer post-processing provides
gradient fills and transitions without leaking into protected PTY regions.

Documentation

The feature includes a dedicated, user-facing manual:

The README, changelog, man page and shell completions also document the public
workflow and commands.

Validation performed

The final reviewed branch passed:

  • git diff --check
  • cargo fmt --check
  • cargo clippy --all-targets -- -D warnings
  • just test

Test results:

  • 973 unit tests passed, 0 failed, 2 ignored
  • 81 E2E tests passed, 0 failed
  • 62 smoke tests passed, 0 failed
  • 1 config-load test passed, 0 failed

The final changes also completed an independent adversarial review loop with
no remaining PR-blocking findings.

michabbb added 30 commits July 30, 2026 11:11
`theme check <file>` now builds its registry from the file's own directory,
as the V1 spec requires, so a portable package whose child extends a sibling
validates as the package it is. The neighbourhood is read with the loader's
own limits and lexicographic order, the explicit target can never collide
with a second read of itself, and only the target may stand in for a built-in
of the same id. A parent that actually came from a sibling is reported, since
installing the child alone would leave it unresolvable.

`{ auto = false }` is no longer accepted: `auto` is the whole-style reset and
`false` is a line the resolver ignores, so it is now a span-accurate error in
both validation modes.

`ResolvedTheme` carries its invariants in the type rather than by convention.
Its fields, those of `ResolvedSemantic` and `ResolvedGradient`, `GradientId`'s
constructor and `ResolvedComponents::new` are crate-private; reading accessors
cover everything a CLI export or a renderer needs. A paint role pointing past
the gradient table now trips a debug assertion instead of only discolouring a
frame, while release builds keep the defensive fallback.

Also stops `paint_color_at` from panicking on a rect whose right or bottom
edge saturates, which no layout produces but a public method must survive.
Petyok

This comment was marked as outdated.

Petyok

This comment was marked as outdated.

Petyok

This comment was marked as outdated.

Petyok

This comment was marked as outdated.

Petyok

This comment was marked as outdated.

Petyok

This comment was marked as outdated.

Petyok

This comment was marked as outdated.

@Petyok Petyok left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review: FIX FIRST — not merge-ready yet

Thanks @michabbb — this is a serious theme-system contribution (schema, inheritance, picker, CLI, PTY exclusion work, broad tests/docs).

CI: Lint ✅ · Ubuntu ✅ · macOS ❌ (tui::tests::default_dashboard_matches_frozen_legacy_buffer)

Blockers

  1. Golden dashboard hermeticityrender_agent_panel calls detect_agent() (reads SSH_AUTH_SOCK / ssh-add) every frame; macOS runners have an agent socket, so the frozen fixture fails deterministically. Verified locally: unset sock → pass; set sock → fail.
  2. Tab-slide PTY protection gapFrameComposition::capture only protects when the active (incoming) session shows_remote_pty. During Ctrl+arrow onto a Connecting tab, outgoing Running PTY cells are blitted then theme-painted. Same class of bug already fixed for the exit slide.

Also please (lower priority)

  • Bound theme file reads (metadata then unbounded read_to_string TOCTOU / FIFO hang).
  • theme check should reject non-regular files.
  • theme show should not dump toml_source for unparseable symlink targets outside the themes dir.

Parse/validate/resolve/manager look solid under review (cycles, depth, compatible discard, transactional reload, reserved IDs). Happy to re-review once the two blockers are green on macOS.

Written by Cursor Grok 4.5 (Cursor) on behalf of the maintainer.

@@ -401,12 +434,18 @@ pub(crate) fn content_fade(at: Option<std::time::Instant>, motion: bool) -> f32
}

pub(crate) fn render_agent_panel(buf: &mut Buffer, area: Rect, app: &App) {

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

blocker: this panel still calls detect_agent() (around L454) every frame — SSH_AUTH_SOCK + ssh-add -l. The new frozen golden expects socket not found, so macOS CI (and any contributor with an agent) fails deterministically.

keys.rs already reads app.agent_info. Paint from that cache here too, and set a fixed AgentInfo in the golden setup. Do not use process-global env::remove_var in the test (races the parallel harness).

Bonus: removes a subprocess from the 50 ms render loop.

Written by Cursor Grok 4.5 (Cursor) on behalf of the maintainer.

Comment thread src/tui/mod.rs
// but are SSHub's own chrome, and a theme is allowed to back them.
if shows_session_view(app) {
if let Some(session) = app.active_session() {
if crate::session::render::shows_remote_pty(session) {

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

blocker: protection is keyed only on the active session. During session_tab_switch, active is the incoming tab. If that tab is Connecting / disconnected Exited, protected stays empty while render_session_tab_slide still blits the outgoing Running PTY into remote_pty_rect, then apply_app_background paints theme colour into those cells.

Please protect remote_pty_rect whenever session_tab_switch is armed (over-protect like the exit slide), and add a regression test for Running → Connecting mid-slide.

Written by Cursor Grok 4.5 (Cursor) on behalf of the maintainer.

Comment thread src/tui/mod.rs
// fixed reference while its highlight slides between tabs (#35).
let area = crate::session::render::body_rect(frame.area());
// fixed reference while its highlight slides between tabs (#35). The rect
// is the viewport's own, so the region cleared and blitted here is exactly

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

risk: render_session_tab_slide still takes its own Instant::now() for progress (just above this blit). FrameComposition already captured a frame clock — same skew the exit-slide comment warns about. Please drive tab-slide progress from the composition clock too.

Written by Cursor Grok 4.5 (Cursor) on behalf of the maintainer.

Comment thread src/tui/mod.rs Outdated
assert!(buffer_contains(&buffer, "web-prod"));
fn default_dashboard_matches_frozen_legacy_buffer() {
let buffer = render_default_theme_golden_surface(&test_app_with_hosts(), 132, 38);
assert_eq!(

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: assert_eq! on ~5k-line signatures dumps an unusable diff in CI (macOS log stops at left == right failed). Prefer reporting the first differing cell (coord + expected/actual).

Written by Cursor Grok 4.5 (Cursor) on behalf of the maintainer.

Comment thread src/theme/registry.rs Outdated
length,
});
}
let text = fs::read_to_string(file).map_err(|source| ThemeRegistryError::Io {

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

risk: size check is metadata then unbounded read_to_string. Concurrent grow / FIFO after a zero-length metadata can bypass the 1 MiB bound or hang the CLI. Prefer open once + .take(MAX+1), and require a regular file (this path currently skips the is_file() guard used by the directory loader).

Written by Cursor Grok 4.5 (Cursor) on behalf of the maintainer.

Comment thread src/theme/registry.rs Outdated
continue;
}

let text = match fs::read_to_string(&path) {

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

risk: same metadata-then-unbounded-read pattern on the directory loader. Open the file handle once and read through .take(MAX_THEME_FILE_BYTES + 1).

Written by Cursor Grok 4.5 (Cursor) on behalf of the maintainer.

Comment thread src/cli/theme.rs
println!(
"# copied from theme '{id}'; change `name` before installing under a new filename"
);
print!("{}", record.toml_source);

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

risk: theme show prints toml_source with no validity gate. A symlink planted in themes/ that points at a private file (e.g. ~/.ssh/id_ed25519) fails TOML parse, stays listed as invalid, and theme show <id> dumps the target contents under the user’s privileges (≤1 MiB).

Keep symlink follow for discovery if you want shared packs, but either reject canonicalize-outside-themes-dir, or suppress toml_source for records that failed to parse.

Written by Cursor Grok 4.5 (Cursor) on behalf of the maintainer.

Comment thread src/tui/mod.rs Outdated
}
}

if app.config.appearance.opaque_background {

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit / docs: intentional carve-out — opaque_background paints theme.semantic().canvas into PTY Reset cells. Fine as designed, but PR/docs “themes never recolour remote PTY” overstates this. Worth aligning the wording with what the tests already say.

Written by Cursor Grok 4.5 (Cursor) on behalf of the maintainer.

Comment thread src/theme/resolve.rs Outdated
self.diagnostics.push(ThemeDiagnostic::error(
origin,
span,
format!("unknown parent theme `{parent}`"),

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: when the parent file exists but is invalid, it is absent from definitions, so the child gets unknown parent theme. Prefer distinguishing “missing” vs “installed but failed to load — fix parent first”.

Written by Cursor Grok 4.5 (Cursor) on behalf of the maintainer.

Comment thread src/theme/resolve.rs Outdated
site.origin.clone(),
Some(site.span.clone()),
format!(
"`{}` is not a closed frame and cannot use the `perimeter` \

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: when the child redefines a gradient as perimeter while inheriting a non-closed paint site, the message can say inherited from {child} (last writer of the gradient). Detection is correct; naming both halves (gradient origin vs paint site) would avoid sending the author to the wrong file.

Written by Cursor Grok 4.5 (Cursor) on behalf of the maintainer.

@Petyok

Petyok commented Aug 1, 2026

Copy link
Copy Markdown
Owner

Findings

High

  • src/tui/screens/push_key_pickers.rs:41-59,95-100,135-146,178-184 and src/tui/screens/broadcast.rs:587-642,716-821: tiny terminal layouts can panic. Popup height clamps to zero or one row, but renderers write fixed rows directly; broadcast preview also computes popup.y + popup.height - 1. Opening these overlays at 1x1 can terminate TUI. Guard every row write and zero-height arithmetic; add render coverage for all affected modes at 1x1, 3x2, 8x4, and 20x6.

Medium

  • src/cli/theme.rs:291-295, src/theme/parse.rs:228-238, src/theme/validate.rs:290-300: plain theme list prints user-controlled theme names without control-character sanitization. A theme name containing \u001b[2J emits raw terminal controls. Reject C0/DEL characters in metadata or escape them in plain output, including diagnostics.

  • src/cli/theme.rs:70-74, src/config.rs:361-368,421-450: read-only theme list and theme show invoke legacy config migration. This creates ~/.config/sshub and copies legacy files; fs::copy follows symlinked files, so migration can copy data from outside the legacy tree. Use a side-effect-free config path resolver for theme CLI and reject symlinks during migration.

  • src/tui/screens/keygen.rs:8-95, caller src/tui/mod.rs:1564-1568: key-generation form still hardcodes Yellow/Cyan/DarkGray/White styling and an unstyled title. It receives no ResolvedTheme, so custom themes leave this SSHub-owned popup with fixed colors. Add keygen roles, pass the active theme, and add parity/marker coverage.

  • src/tui/mod.rs:31-43, src/tui/screens/keys.rs:64-94,230-258, src/tui/screens/session_picker.rs:77-91, src/tui/screens/palette.rs:58-82: several gradient-capable paint roles are reduced to one sampled color instead of receiving a gradient post-pass. assets/themes/aqua.toml:92-93 assigns a perimeter gradient to components.popup.border, but popup and identity-card borders render flat. Either apply gradient ring/line passes or reject gradients for these roles.

  • src/tui/screens/settings.rs:67-97, src/tui/screens/keybind_editor.rs:103-125, src/tui/screens/tunnel_reconnect.rs:90-105: selected-row backgrounds are painted first, then selected checkbox/value cells are written with foreground-only styles. set_string resets the background to Color::Reset, leaving holes in selection bars. Preserve the selected background in all selected control styles and add buffer assertions.

  • src/app/mod.rs:686-705, src/tui/mod.rs:127-130,1186-1203: theme invalidation clears popup/session/SFTP snapshots but not dashboard_snapshot. A queued theme preview/commit followed by session entry can reuse dashboard cells captured under the previous theme or with stale overlay chrome. Clear the dashboard snapshot and guard session-entry animation when no fresh snapshot exists.

  • src/cli/theme.rs:839-845,888-925, src/theme/parse.rs:403-416: theme show --resolved accepts quoted gradient names such as odd name but emits [gradients.odd name] and raw references without TOML quoting. Valid input therefore produces invalid output. Serialize TOML keys/references properly or restrict gradient names to bare keys; add non-bare-name round-trip tests.

  • src/theme/validate.rs:597-625, src/theme/resolve.rs:746-748: gradient stop ordering is validated as f64 but stored as f32. Stops at 0.5 and 0.50000001 pass validation and collapse to equal positions. Preserve f64 or reject positions that are not strictly ordered after conversion.

  • src/theme/model.rs:439-456,795-805,839-849: public GradientId contains only a table index. ResolvedTheme::gradient documents that foreign-theme IDs return None, but implementation indexes the receiving theme directly. An ID from one theme can resolve to another theme’s gradient at the same index. Add theme ownership/generation to IDs or make lookup theme-scoped, with a cross-theme API test.

Low

  • docs/theme-system.md:15-16,135-139,440, CHANGELOG.md:82, assets/themes/default.toml:28, and src/theme/role_matrix.snapshot: implementation has 228 role rows while documentation repeatedly claims 234 roles and 233/234 semantic fallbacks. Generate counts from ROLE_SPECS or reconcile all published text.

Written by openai/gpt-5.6-luna (OpenCode) on behalf of the maintainer.

Tiny-terminal safety: fit_popup clamps the push-key pickers and the
broadcast wizard popups, with explicit guards before every set_string,
so 1x1 up to 40x2 frames render instead of panicking.

CLI hygiene: plain theme output escapes control characters in theme
names and diagnostics, gradient keys and string values are serialised
through toml_edit, and the read-only theme commands no longer create
the config directory. Legacy migration copies with symlink_metadata
and rejects symlinked roots and entries.

Theme chrome: six components.keygen.* roles enter the catalogue
(234/233 roles), the keygen form drops its hardcoded ANSI styles, and
paint_border gradients popup frames without repainting their titles.
The session slide only draws on a dashboard snapshot matching the
current area.

Gradients: positions are f64 end to end, and GradientId carries the
resolve generation so an id from another resolve run is rejected
rather than silently indexing a foreign table.

@Petyok Petyok left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-review of the fix commits (b56fe1a8, 12860200, f852a636) against both earlier reviews. Every claim below was verified by reading the current code and running the suite — not from the commit messages.

Verification performed

  • Checked out the PR head (f852a63) in a worktree; just test green locally (all four targets), CI green including macOS.
  • All findings from the 2026-07-31 review and the 2026-08-01 findings comment traced to their current code, tests located and read; the one remaining crash was reproduced by execution, not just static analysis.

What's verifiably fixed — 13 of 14 items

From the 2026-07-31 review: golden-dashboard hermeticity (agent detection moved out of the renderer entirely — render_agent_panel reads app.agent_info, detection lives in the event loop behind a 30 s throttle with an injectable detector; cleaner than the original env-var plumbing), the tab-slide PTY gap (session_tab_slide_regions protects both sides of the slide, with Running→Connecting tests), and all three file-handling asks (bounded take() reads with O_NONBLOCK and a post-open regularity re-check, non-regular files rejected uniformly, no toml_source dump for invalid records).

From the 2026-08-01 findings: control-char sanitization funnels through one sanitize_plain/render() pair with unit + subprocess tests (no unescaped sibling path found); read-only theme CLI uses the side-effect-free config_dir_path() and migration rejects symlinked roots/entries via symlink_metadata; keygen is fully role-driven (six new components.keygen.* roles, zero Color:: literals left, marker + parity + legacy-inventory coverage); selection-bar holes closed in all three screens via a shared inherit_background, with buffer tests asserting fg and bg; dashboard_snapshot is genuinely cleared in invalidate_theme_visual_state (the commit message undersold this — both mutation seams route through it) plus an area guard on session entry; resolved-TOML export goes through toml_edit with a round-trip test on odd ."\ name; gradient stop positions are f64 end to end with the exact 0.5/0.50000001 pair tested; GradientId carries a per-resolve generation minted from a non-wrapping process-global counter, checked before every lookup, with the demanded cross-theme API test; role counts reconciled at 234/233 everywhere and pinned to ROLE_SPECS by test.

Gradient coverage in general is real ring/line painting now (paint_border + paint_popup_border paired at every popup call site I checked, identity cards included), not a sampled flat colour.

Why REQUEST_CHANGES: one blocker survives

The tiny-terminal fix covers the three wizard popups but not the docked live panel in the same file — docked_rect still clamp-then-grows past a zero-height body and the panel renderers guard their input rect, not the buffer. Reproduced by execution: running broadcast + small terminal = panic in ratatui's index_of. Full trace, failing test (built on this PR's own TINY matrix and fixtures) and fix shape in the file comment on src/tui/screens/broadcast.rs.

Small items

  • CHANGELOG entries sit inside the already-released [0.11.0] instead of [Unreleased], and the credit line is missing (inline comment).
  • The tunnel host picker's separator is the one SeparatorSecondary site that still flattens gradients (inline comment).
  • Noted, not asked: copy_dir_recursive is check-then-act (symlink_metadatafs::copy), so a same-privilege racer can still swap in a symlink between check and copy. Much narrower than the original finding and unreachable via the theme CLI; fine to leave for a follow-up.

With the docked-panel panic fixed (keep the test) and the CHANGELOG moved, this is merge-ready from my side — the fix work across both review rounds is thorough and honestly tested.

Written by Fable 5 (Claude Code) on behalf of the maintainer.

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Blocking: the docked live panel still panics on a tiny terminal — same bug class as the wizard-popup finding, one renderer over. (File-level comment because docked_rect itself is pre-existing code outside this diff.)

The fit_popup fix covers the three wizard popups, but the docked panel path doesn't go through it: docked_rect (line ~420) still does clamp-then-grow (.min(...).max(20) / .max(6) — exactly the pattern fit_popup was introduced to kill), and render_broadcast_panel / render_broadcast_zoomed guard the rect they are handed, not the buffer. At a 20x6 frame dashboard_layout_zoomed(...).body is (0, 6, 20, 0), so docked_rect returns (0, 6, 20, 6) — entirely below a 6-row buffer — and src/tui/mod.rs:603-614 renders the panel with it whenever app.broadcast is Some. ratatui 0.30's set_string panics on an out-of-range row (index_of), so: running broadcast + terminal shrunk small = the TUI dies.

Reproduced, not just traced — this test (built on your own TINY matrix and fixtures) fails on every size from 1x1 to 20x6:

#[test]
fn the_docked_panel_survives_a_tiny_terminal() {
    for zoomed in [false, true] {
        let mut app = themed_app(resolved_default());
        app.broadcast = Some(four_states());
        for (w, h) in TINY {
            let area = Rect::new(0, 0, *w, *h);
            let body =
                crate::tui::dashboard_layout::dashboard_layout_zoomed(area, 0).body;
            let buf = frame_at(area, |f| {
                if zoomed {
                    render_broadcast_zoomed(f, body, &app);
                } else {
                    render_broadcast_panel(f, docked_rect(body), &app, false);
                }
            });
            assert_eq!(buf.area, area, "docked at {w}x{h} zoomed={zoomed}");
        }
    }
}
panicked at ratatui-core-0.1.2/src/buffer/buffer.rs:251:
index outside of buffer: the area is Rect { x: 0, y: 0, width: 1, height: 1 } but index is (0, 6)

Fix shape: route docked_rect through the same clamp discipline as fit_popup (never grow past the body, and treat a zero-height body as "don't render"), and keep the test.

Comment thread CHANGELOG.md Outdated
optional dependencies, so there is no build step and nothing is fetched from
outside the registry. Prebuilt for Linux x64, macOS arm64 and macOS x64; every
other platform keeps using `cargo install sshub`.
- **Runtime theme system** - SSHub's colours are TOML files now, not constants.

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These entries are inside ## [0.11.0] - 2026-07-30 — a release that already shipped (tagged, on main and crates.io) — while ## [Unreleased] above sits empty. As written, the changelog claims the theme system was part of 0.11.0. Please move the four entries up under [Unreleased] (per CONTRIBUTING §5) and add the credit to the first one: - **Runtime theme system** (PR #86 by @michabbb) - … — the credit line is required by CONTRIBUTING § Credit (you did both correctly in #88).

Comment on lines 615 to +624
let sep: String = std::iter::repeat_n('\u{2500}', inner_w).collect();
buf.set_string(row_x, popup.y + 2, &sep, theme::dim());
buf.set_string(
row_x,
popup.y + 2,
&sep,
Style::default().fg(crate::tui::blit::line_color(
theme,
PaintRole::SeparatorSecondary,
Rect::new(row_x, popup.y + 2, inner_w as u16, 1),
)),

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor, non-blocking: this is now the only SeparatorSecondary site that samples line_color without a follow-up paint_line pass, so a gradient assigned to that role renders flat here while every sibling separator sweeps. The other pickers (session_picker.rs, push_key_pickers.rs) deliberately use StyleRole::PopupHint with a comment saying why — this one uses the gradient-capable paint role but skips the pass. Either add the paint_line call or switch to the non-gradient role with the same comment as the siblings.

@michabbb

michabbb commented Aug 3, 2026

Copy link
Copy Markdown
Contributor Author

@Petyok give me some time, i currently tokenmaxxing and running out of resets 😂

The docked broadcast panel could panic on a tiny terminal: `docked_rect`
clamped then grew back past the body (`.max(20)` / `.max(6)`), so a
zero-height body produced a rect entirely below the buffer, and both panel
renderers guarded the rect they were handed rather than the buffer. Route
the size through `fit_popup` so it never outgrows the body, let a zero
extent stay zero, and intersect the incoming rect with the frame in
`render_broadcast_panel` and `render_broadcast_zoomed`.

A zero-sized `docked_rect` is a new precondition for its other consumers,
so `the_docked_panel_survives_a_tiny_terminal` covers `spawn_rect` and the
toast stack as well — the latter in both anchor branches, since the
panel-gone branch is the one that computes `dock.y + dock.height`.

The tunnel host picker was the one `SeparatorSecondary` site that sampled
`line_color` without the matching `paint_line` pass, so a gradient on that
role rendered flat there while every sibling separator swept.

Finally, the CHANGELOG entries claimed the theme system shipped in 0.11.0,
a release that is already tagged and on crates.io; all six move up under
[Unreleased], with the credit line CONTRIBUTING asks for.
`copy_dir_recursive` rejected symlinks, but check-then-act: it read
`symlink_metadata` and then handed the path to `fs::copy`, which resolves
symlinks. Anything that replaced a regular file with a link in that window
had its target copied into the new config directory — `~/.ssh/id_rsa`, for
instance. Reproduced by simulating the swap: the old branch returned
`Ok(())` with the key material in the destination.

The file branch now opens the source once with `O_NOFOLLOW`, confirms it is
regular on that handle rather than on the path again, and copies out of the
handle, so a swapped link fails with `ELOOP` instead of being followed.

`io::copy` does not carry permissions the way `fs::copy` does, so the
destination is created with the source's mode and set to it again after the
content is written — a migrated `0600` secret must not land as `0644`.

The guard lives in `copy_regular_file` so a test can reach it directly:
every existing test stops at the pre-check and would stay green if this
second line of defence were removed.

Directories are still traversed by path, which leaves the same race for a
directory swapped against a symlink. Closing that needs an fd-relative
`openat` traversal; it is noted in a comment and left for a follow-up.
Four findings, two of them reproduced before being acted on.

The symlink guard in `copy_regular_file` covered the source and left the
destination open: `create(true).truncate(true)` follows a link at the
destination path, so a link planted in the staging tree had its target
truncated and overwritten — the source-side bug read foreign data, this one
destroyed it, and the call still returned `Ok(())`. The staging tree is
freshly created and discarded on every failed attempt, so the destination
must never already exist: open it with `create_new` and `O_NOFOLLOW`.

Clipping the docked panel against the frame stopped the panic but fed the
clipped rect back in as the layout area, so a panel sliding out of view was
re-laid out instead of cropped — narrower box, re-ellipsised title, right
border pinned to the terminal edge. That is every exit animation, not just
tiny terminals. Layout now always uses the original rect; only when it
overhangs does the panel render into a scratch buffer and get blitted onto
the visible part, so a fully visible panel allocates no scratch buffer (the
renderer still allocates its title, badge and text fragments as before).

Both halves of the tiny-terminal fix were only held by one test that needed
both to be removed before it failed, so either half could regress unnoticed.
`docked_rect`'s geometry and each renderer's clipping are now pinned
separately.

The bounded handle read for theme files was held by nothing: the oversize
tests all trip the path-based `metadata.len()` pre-check first, so removing
`take(MAX + 1)` left all 30 registry tests green — while that second read is
exactly what defends against a file growing after the pre-check.
Both came out of resolving the upstream merge in `save_config_at`, and
`.github/workflows/ci.yml` runs clippy with `-D warnings`, so they would
have failed the pipeline rather than sat there as warnings.
@michabbb

michabbb commented Aug 7, 2026

Copy link
Copy Markdown
Contributor Author

Thanks — all three items from the 2026-08-01 review are addressed, and a couple of things turned up along the way that are worth reporting honestly.

The three items

The docked panel panic. docked_rect now goes through fit_popup, so it never grows back past the body, and a zero extent stays zero. Both renderers additionally clip against the frame rather than trusting the rect they are handed. Your test is in, and it fails on the unfixed code with exactly the trace you quoted.

CHANGELOG. Moved — but there were six entries, not four: sshub theme CLI and Theme documentation were also added by this PR and were still sitting in the released 0.11.0. All six are now under [Unreleased], with the credit line on the first.

The tunnel separator. paint_line added with the same rect line_color samples, matching tunnels.rs:94, audit.rs:192-194 and broadcast.rs:304-310.

copy_dir_recursive — fixed, not deferred

You flagged the check-then-act as "noted, not asked". We fixed it anyway, and it turned out to be worth doing: the file branch now opens the source with O_NOFOLLOW, confirms it is regular on the handle rather than on the path again, and copies out of that handle. Simulating the swap on the old code returned Ok(()) with the target's contents in the destination, so the gap was real rather than theoretical.

io::copy does not carry permissions the way fs::copy does, so the mode is transferred explicitly — a migrated 0600 secret must not land as 0644.

Directories are still traversed by path. Closing that needs an fd-relative openat traversal; it is marked in a comment and left as a follow-up, as you suggested.

What an independent audit found afterwards

The fixes above were audited by a second pass, which found four real defects in the fix work itself. All four are fixed and re-verified; the reproductions below fail on the old code and pass on the current head:

  1. The symlink guard covered the source and left the destination open — create(true).truncate(true) follows a link at the destination path, so a link planted in the staging tree had its target truncated. The source-side bug read foreign data; this one destroyed it, and the call still returned Ok(()). Now create_new + O_NOFOLLOW.
  2. Clipping the panel stopped the panic but fed the clipped rect back in as the layout area, so a panel sliding out of view was re-laid out instead of cropped — narrower box, re-ellipsised title. That is every exit animation, not just tiny terminals. Layout now always uses the original rect; only an overhanging panel renders into a scratch buffer and gets blitted onto the visible part.
  3. Both halves of the tiny-terminal fix were held by a single test that only failed when both were removed, so either half could have regressed unnoticed. Geometry and clipping are now pinned separately.
  4. The bounded handle read for theme files was held by nothing — the oversize tests all trip the path-based metadata.len() pre-check first, so removing take(MAX + 1) left all 30 registry tests green. That second read is what defends against a file growing after the pre-check, so it now has a test that fails without it.

Caught up with current development

The branch had drifted 28 commits behind, including v0.13.0, so development is merged in (not rebased — it would have discarded the review threads here). Two things came out of that:

The new profile/picker.rs and screens/known_hosts.rs used the pre-theme tui::theme::{text, bright, …} API this PR removes, which made the merged tree uncompilable. They are on runtime roles now, reusing existing generic roles — so the role count stays at 234/233 and nothing pinned to ROLE_SPECS moved. Both also got tiny-terminal coverage, since that failure mode has already cost this PR two blockers.

CI is green on all three jobs, and the branch is mergeable.

Written by Opus 5 (Claude Code) on behalf of the maintainer.

@Petyok

Petyok commented Aug 8, 2026

Copy link
Copy Markdown
Owner

Verified manually! Found only 2 bugs:

  1. PTY does not follow theme when opaque background is disabled. Looks bad 🥲
  2. "Summer" theme rippes eyes at PTY when "Opaque background" is enabled.

Bugs:

  1. Summer (my eyes 😭 ):
image
  1. Opaque

Opaque enabled:
image

Opaque disabled:
image

Maybe "Enable Opaue" should modify alpha channel at some constant, so everything would seem somewhat transparent but still follow theme? Or go even further and make it as a scroller for alpha

Design record for the two defects @Petyok found in the PR Petyok#86 manual
review, and for the switch rework that followed from testing the first
fix.
Two defects from @Petyok's manual review of PR Petyok#86, and the switch
rework that testing the first fix made unavoidable.

The remote grid kept the emulator's foreground while taking our
background, so `summer` drew near-white on cream. `fill_reset_background`
wrote one channel; both are now written as a pair, per channel, which
also keeps reverse video correct. Two semantic slots join the core
(23 -> 25): `pty_background` and `pty_foreground`, declared in `default`
as references to `background` and `text`, so a theme that paints its own
ground paints it under the grid too without any theme file changing.

`opaque_background` is gone. It asked to fill what a theme left empty,
which is nothing at all under a theme that paints everything - measured
as zero changed cells under four of the five built-ins. Asking to
release is the question every theme can answer, so SSHub is opaque out
of the box and two independent switches hand a surface back to the
terminal: `transparent_sshub_background` and
`transparent_session_background`. The retired key is dropped from
config.toml on the next save.

Releasing works by role, not by colour. `with_ground_released` resolves
the ground slots and every role inheriting from them to the terminal's
own background before anything is drawn, so panel bodies painted through
`semantic.surface` come free while selection bars and status colours
stay - a theme may legitimately give its selection and its surface the
same value, and no colour comparison over the finished frame could tell
them apart.

Three defects in this fix work itself, all found by review while the
suite was green: the enter slide claimed the resting viewport as
ownership and painted grid ground over 220 cells of dashboard; releasing
SSHub's surfaces also released the grid's fallback, coupling two
switches meant to be independent, with a test that accepted it because
it compared against the same broken value; and a theme setting
`pty_foreground = "terminal"` re-opened the original bug, which
`PtyGround::of` now answers with no ground at all rather than half of
one.

No alpha slider: ANSI has no per-cell alpha, and only the emulator can
blend, behind cells left at the default background.
@michabbb

Copy link
Copy Markdown
Contributor Author

Both confirmed and fixed. The second one turned out not to be a summer problem, and the first one turned into a larger change than expected — details below, including what a follow-up review found in my own fixes.

1. summer shredding your eyes

Not the theme. fill_reset_background wrote only the background channel, so the remote's default foreground stayed Color::Reset and your emulator painted its own near-white into our cream ground. White on cream.

Nothing about that is specific to summer — every light theme hits it, and anyone running a light emulator profile hits the mirror image under a dark theme. It only looked fine on dark themes because emulators happen to default to a light foreground.

Background and foreground are now written as a pair, tested per channel. That also repairs reverse video: a REVERSED cell with both channels unset used to swap our ground against a foreground that was never defined.

2. The PTY not following the theme

Two new slots join the semantic core (23 → 25): pty_background and pty_foreground. In default they are declared as references to background and text, so the rule reads: a theme that paints its own ground paints it under the remote grid too, and inherits a foreground that matches. No other theme file needed changing, and derived user themes inherit the rule.

On the alpha channel

It cannot be built, and the reason is worth stating: ANSI has no per-cell alpha — a cell holds a colour or "default", nothing in between — and the application cannot see what is behind its window to blend against. Blending is exclusively your emulator's job (kitty background_opacity, WezTerm window_background_opacity), and it applies only to cells left at the default background. The moment we write a colour, that cell is opaque and the emulator's setting no longer reaches it.

The usable core of your idea survived, though, and it changed the design.

opaque_background is gone

Testing the first fix surfaced something the switch could never do: under fire it changed exactly zero cells. It asked "fill what the theme left empty", and a theme that paints everything leaves nothing to fill. Which direction is open depends on the theme — so a switch that can only fill is inert half the time.

Asking to release is the question every theme can answer. So SSHub is now opaque out of the box under every theme, and two independent Settings toggles hand a surface back to your terminal:

Settings with both transparency toggles on

Releasing works by role, not by colour: the ground slots and every component role inheriting from them resolve to the terminal's own background before anything is drawn. That reaches the panel bodies a theme paints through semantic.surface, while selection bars, status colours and borders stay — a see-through dashboard still has to show which row is selected, even under a theme that gives its selection and its surface the same colour.

Your old opaque_background = true is dropped from config.toml on the next save; it described the state that is now the default.

What a follow-up review found in the fixes

Worth reporting honestly — three of these were defects in the fix work itself, and the test suite was green through all of them:

  1. The enter slide carries the session in from the right while restoring the dashboard on its left, but ownership still claimed the resting viewport — so the new grid pass painted remote ground over 220 cells of dashboard.
  2. Releasing SSHub's surfaces also released the grid's fallback colour, coupling two switches that are meant to be independent. The regression test accepted it, because it compared against the same value that was already broken.
  3. A theme setting pty_foreground = "terminal" re-opened bug 1 above. The pair now resolves to nothing at all rather than half of it.
  4. The invariant test written to guard the role-based release never called the function it was guarding.

Not in this PR

Colour output from the remote — ls, git, a coloured prompt — still comes from your emulator's 16-colour palette, which on summer's cream can be low-contrast. Remapping those to the theme needs a [terminal] section and a pass through the vt100 render path; it gets its own issue rather than being smuggled in here.

Thanks for the manual pass, @Petyok — the second bug was ours and invisible to every test we had.

Written by Claude Opus 5 (Claude Code) on behalf of the maintainer.

@Petyok

Petyok commented Aug 11, 2026

Copy link
Copy Markdown
Owner

lgtm!

@Petyok
Petyok merged commit daefe23 into Petyok:development Aug 11, 2026
3 checks passed
Petyok pushed a commit that referenced this pull request Aug 11, 2026
`[Unreleased] → Added` had grown to nine bullets, seven of them PR #86:
the theme system, the picker, the built-ins, gradients, the PTY ground,
the CLI and the docs each claimed a top-level entry, so the section read
as nine features when it is two. Every other release in this file gives
one bullet per feature — the known hosts manager covers its overlay,
filter, delete and refresh in a single entry.

Same content, restructured: the theme system is one entry with the
picker, built-ins, gradients, PTY ground and CLI nested under it, and the
doc links folded into the parts they document. The `opaque_background`
replacement moves to `Changed`, where a removed config key belongs.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants