feat(slash): /theme picker with live preview - #67
Merged
Conversation
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
Bare `/theme` opens a list picker whose Up / Down repaints the full TUI in the candidate theme; Esc snaps back to the original, Enter commits for the rest of the session. `/theme <name>` swaps directly. Session-only — restart returns to `[tui.theme] base` in the user config. Adds `ModalKey::Preview(ModalAction)` so live-preview modals can emit actions without popping; threads a `theme_name` field through `Config` / `ConfigSnapshot` so the picker marks the active row; and adds `set_theme` to `ChatView` / `StatusBar` / `InputArea` for mid-session repaint.
…fill - Slash-synthesized non-`SubmitPrompt` actions now route through `dispatch_user_action` instead of forwarding direct to the agent. The agent silently dropped TUI-only actions like `SwapTheme`, so `/theme latte` did nothing. Synthesized prompts (e.g. `/init`) preserve the existing direct-forward path to avoid re-parsing the leading `/`. - Popup now scrolls with a centered cursor (Claude Code convention) instead of trimming with `... (N more)`. Once selection moves past the top half of the window the cursor anchors at the visual middle; near the end of the list it anchors at the bottom. Footer dropped. - `App::draw_frame` paints the full frame area with `theme.surface()` before sub-widgets render so light themes (e.g. `latte`) cover gaps between blocks, modal bodies, and the popup band. Belt-and-braces: the popup also paints surface bg explicitly. - New tests: `swap_theme_with_unknown_name_pushes_error_and_keeps_active_theme` and `slash_typed_swap_theme_routes_through_local_handler` pin the fixed dispatch path; `theme_row_picker_item_methods_return_curated_values` closes the codecov gap on `ThemeRow`'s trait impl; `scroll_offset_*` quartet pins the centered-scroll formula.
Prior `load_builtin` collapsed unknown-name and parse-error into a single `None`, so a regression in any vendored palette would surface to users as a misleading "unknown theme" error instead of a startup failure. Vendored TOML is build-time content; expect it to parse.
`StreamingAssistant` caches rendered lines up to the most recent `\n\n` boundary; styles bake in at cache time. Bare `/theme` is classified `ReadOnly` so the picker can open mid-stream, but the cached prefix used to keep painting under the previous theme until `commit_streaming` rebuilt it. `ChatView::set_theme` now drops the prefix cache so the next frame re-parses with the active palette. Adds `StreamingAssistant::invalidate_cache` and threads `invalidate_cache_for_width` through it to share the reset logic.
The SubmitPrompt arm carried 60+ lines of slash-command dispatch logic inline. Splitting it into a dedicated method keeps `apply_action_locally` under the `clippy::too_many_lines` threshold and makes room for upcoming fixes that grow the variant arms (PreviewTheme drift warn, etc.). No behavior change.
`SwapTheme` only cleared `preview_theme_snapshot` inside the success arm of `load_builtin`. If the defensive `else` arm ever fired (today unreachable via the slash form, but a future caller could bypass validation), the snapshot would survive and roll back the next modal's cancel. Hoist the reset above the guard so the snapshot's lifetime ends when SwapTheme commits, regardless of result. `PreviewTheme` now logs `tracing::warn!` when `load_builtin` returns `None` — the picker roster (`LISTED_THEMES`) and the loader's lookup table must agree, so a `None` here means drift between the two. User-facing silence stays appropriate (preview repaints on every keystroke), but a log line surfaces the bug to developers.
`char::from_digit(_, 10)` already returns `None` for digits >= 10, making the explicit `(1..=9).contains` filter redundant. Collapse the `u32::try_from / checked_add / range-contains / branch` cascade to a single expression.
Sweep across the PR-touched files and adjacent code to align with the project comment guidelines: drop multi-line WHAT-only docstrings where the code conveys the same, and rephrase the `do X, not Y` antithesis tic into positive form. - `slash/theme.rs`: module doc, `LISTED_THEMES`, `resolve_theme_arg`, and the `ThemePicker::height` arithmetic comment. - `tui/app.rs`: module-level dirty-flag note, `apply_theme` doc, surface-bg fill comment, two test docstrings, and the dispatch-fix comment that was on the wrong arm — now lives next to the `else` branch it actually motivates. - `tui/components/input/popup.rs`: module doc and `height` doc. - `config.rs` and two test docstrings in `tui/app.rs`: rephrase `must X, not Y` constructions into positive form.
- `draw_frame_surface_fill_overwrites_unpainted_cells_with_surface_bg` pre-stains the buffer with a sentinel colour, renders the frame, and asserts no sentinel survives. Pins the buffer-wide invariant that any future widget leaving cells unpainted gets caught. - `slash_typed_model_routes_synthesized_swap_config_through_dispatch` mirrors the typed-`/theme` regression on the agent-bound side: `/model haiku` synthesizes a `SwapConfig` that must reach the agent via dispatch. Existing `dispatch_swap_config_*` covered direct emission; this exercises the synthesized-via-slash path. - Three new `scroll_offset_*` cases pin (a) the centering invariant directly via `visible_row == pad`, (b) the `total == MAX_VISIBLE_ROWS` boundary that the `<=` early-return guards, and (c) the symmetric `select_prev` wrap-from-top where the offset clamps to the bottom.
Targeted gap-fillers identified from the codecov patch report: - `up_and_k_emit_preview_for_prev_row_without_popping` and `j_alias_for_down_emits_preview` exercise the `Up | k` and `Down | j` arms in `ThemePicker::handle_key`. Existing tests covered Down only. - `preview_theme_with_unknown_name_does_nothing_and_keeps_active_theme` hits the `tracing::warn!` arm in `App::apply_action_locally` when a preview-roster drift would surface. Pins user-visible no-op state. - `set_theme_invalidates_streaming_cache_so_in_flight_tokens_repaint` and `set_theme_without_active_streaming_is_a_noop_on_cache_state` cover both arms of the new `if let Some(streaming)` guard in `ChatView::set_theme`, ensuring future refactors don't invert it.
Project convention requires test sections to mirror the production function order in the same file. Several recent additions landed in sections that no longer matched the prod layout — sweep them now. - `tui/components/chat.rs`: `set_theme` tests had no section header and lived inside the `update_layout` block. Move them into a fresh `// ── set_theme ──` section between `Fixtures` and `load_history`, matching the prod method's position at line 62. - `tui/app.rs`: `draw_frame_surface_fill_*` belongs in the `// ── draw_frame ──` section, not the theme section. Reorder the apply_action_locally / theme tests so each preview / swap variant groups with its happy-path neighbour (preview happy → preview cancel → preview unknown; swap happy → swap unknown). - `slash/theme.rs`: section order now mirrors prod — ThemeRow → numeric_hint → ThemePicker::new → ThemePicker::handle_key → ThemePicker::render → ThemeCmd metadata → ThemeCmd::execute. Combine the asymmetric `down_arrow_emits_preview` plus `j_alias_for_down_emits_preview` pair into a single `down_and_j_emit_preview_for_next_row_without_popping`, matching the existing `up_and_k_emit_preview_for_prev_row...`. - `tui/components/input/popup.rs`: move `selected` ahead of `select_next / select_prev` and `scroll_offset` after `render`, matching the prod method declarations at lines 56 / 60 / 84 / 103. Reorder the eight `scroll_offset_*` cases so each test groups with its thematic partner (no-scroll cases → top → mid pinning → bottom → wrap pair). - `tui/components/chat/blocks/streaming.rs`: have `take_buffer` delegate to `invalidate_cache` instead of duplicating its three resets — the helper now exists for `set_theme` to call.
Mocha / macchiato / frappe already inherit the terminal bg via
`surface = { bg = "reset" }`. Light and Material were the outliers
forcing an explicit fill — apply the same convention so every
built-in theme adapts to the user's terminal palette.
The surface-fill regression test no longer needs to swap to latte;
default theme is now sufficient since all built-ins use Reset.Modals own focus and intercept every key, so the input row + slash popup are unreachable while one is on the stack. Collapse both layout slots so the chat reclaims that vertical space — parallels Claude Code's modal UX. Adds a layout-level test using a `FakeModal` so future modals inherit the behavior automatically without per-modal opt-in.
…den tests Drop the `SwapTheme` chat-error path (`tui/app.rs`): the slash form already validates names against `LISTED_THEMES`, so the only path where `load_builtin` returns None is roster drift between slash and builtin tables — a developer bug, not user error. Mirror `PreviewTheme`'s `tracing::warn!` and rename the test to `swap_theme_with_unknown_name_is_silent_noop`. Move the `apply_action_locally / theme` test section to sit between `dispatch_user_action` and `handle_agent_event` so the section header order mirrors the production-function order (CLAUDE.md testing convention). Trim multi-line comments flagged in review: - `apply_modal_action` rollback comment. - `apply_theme` doc. - `handle_submit_prompt` synthesized-action arm. - Queued-prompt forward gate. - `resolve_theme_arg` doc + a few test preambles. Harden tests: - `draw_frame_surface_fill` asserts `cell.bg == surface_bg` (mutation that paints any non-magenta color used to pass). - `numeric_jump_past_roster_is_a_consumed_noop` derives the digit from `LISTED_THEMES.len()` so it survives roster growth. - New `modal_cancel_without_snapshot_is_noop` pins the rollback arm's no-snapshot path. - `assert!(... == "latte")` → `assert_eq!` for better failure output.
…t section
Add `submit_slash_theme_pushes_picker_onto_modal_stack` covering the
`handle_submit_prompt` modal-push branch (`tui/app.rs:339`), and extend
`draw_frame_hides_input_and_popup_while_modal_active` with a key send so
`FakeModal::handle_key` (the test fixture) is exercised.
Move the two `slash_typed_*` regression tests out of `apply_action_locally
/ theme` into a new `// ── handle_submit_prompt ──` section. Production
order is `apply_action_locally` (278) → `handle_submit_prompt` (329) →
`handle_agent_event` (391); test sections now mirror that.
Patch coverage rises to 99.21% — the remaining 6 missing lines in
`slash/theme.rs` are line-attribution artifacts (defensive `None` arms
shared with `slash/picker.rs`, plus `match { other => panic!(...) }`
test-failure idioms).Uh oh!
There was an error while loading. Please reload this page.
5 tasks
hakula139 added a commit
that referenced
this pull request
May 9, 2026
<!-- markdownlint-disable-next-line first-line-heading --> ## Summary A doc-only sweep of `docs/` that fixes stale claims drifted from source, replaces rotting inline file links with backticks or concept-level cross-doc pointers, normalizes Sources sections, and reworks the prose tics that had crept into design and research docs. 19 files touched, `pnpm spellcheck` and `pnpm lint` clean. The sweep was deferred during the `/resume` PR after rewriting `slash/resume.md` exposed several stale claims of the same shape lurking in `commands.md` and `modals.md`. Folding the cross-doc audit into one PR reads cleaner than piecemeal edits attached to feature work. ## Design decisions - **Sources sections keep full `crates/oxide-code/src/...` paths, sorted alphabetically.** Full paths are clickable in editors and unambiguous about what level of the tree we're naming. Line numbers stay out so the entries don't rot under refactors. Annotations stay one short clause. - **Inline body references switch to backticks or concept-level cross-doc links.** `[X](../../../crates/.../x.rs)` in prose rotted on every file split. Backticked names like `` `SearchableList` `` and `[modals.md](modals.md)` survive renames. - **Trait-shape claims described semantically rather than as literal Rust.** The `modals.md` `ModalKey` block was already stale (the `Preview` variant added in PR #67 was missing). Rewriting it as four named outcomes with prose stays correct across future variant additions. - **Research docs keep their implementation depth.** Research docs document external systems (Claude Code, Codex, opencode, Anthropic API) and earn their detail. The sweep there is wording consistency only — antithesis trims, em-dash chain splits, stale counts. - **"X, not Y" rewritten in the body, sometimes preserved in titles.** Decision titles where the contrast IS the load-bearing rationale (`/resume`'s `roll_into` vs. process replacement; the `cch` body field vs. beta header) keep the antithesis. Body prose loses it: `xxh64, not SHA-256` becomes `xxh64 for change detection`, etc. - **Prose connectors over em-dashes and period fragmentation.** Em-dash is reserved for true parenthetical asides, not for stitching two independent clauses together. When trimming em-dash / semicolon overuse, transitions like `since`, `because`, `while`, and `where` carry the rewrite — defaulting to a period creates staccato fragmentation that reads worse than the original. ## Changes | File | Description | | ---- | ----------- | | `docs/design/slash/commands.md` | "Nine built-ins" → eleven; `/rename` and `/resume` added to the inline list and Per-Command notes; antithesis decision titles softened; Sources alphabetized with full paths. | | `docs/design/slash/modals.md` | `ModalKey` rewritten semantically (now-stale 3-of-4 Rust block dropped); Per-Modal notes added for `/rename` editor and `/resume` picker; Decisions 1, 7, 8 rewritten without antithesis; em-dash chains turned into transition-word sentences; Sources alphabetized. | | `docs/design/slash/resume.md` | `SearchableList` / `SessionRow` descriptions updated for the multi-line render and current field set (`message_count`, `git_branch`, `project`); decision titles softened; full repo paths in body links replaced with concept-level pointers; Sources alphabetized. | | `docs/design/session/file-tracking.md` | "xxh64, not SHA-256" rewritten in positive form; Sources alphabetized. | | `docs/design/session/persistence.md` | Em-dash chains in actor-batching and resume-sanitization paragraphs converted to conjunction-joined sentences; `WriterStatus::Pending` updated to mention the deferred-title field added in PR #72. | | `docs/design/tools/truncation.md` | `TRUNCATION_OVERHEAD` constant `50` → `80` (matches `tool.rs`); "Head-tail, not tail-only" rewritten in positive form; Sources alphabetized. | | `docs/design/tui/cancellation.md` | Status hints `Streaming . Esc` / `Running {tool} . Esc` → middot `·` matching `status.rs`; Decision 6's antithesis rewritten; em-dash-as-connector cases swapped for `since` / `because` clauses; Sources alphabetized. | | `docs/design/tui/overview.md` | Dropped fictional `trait Component` pseudocode (no such trait exists); "11 named color slots" replaced with a slot-family description (the actual count is 30+ accessors); the staccato streaming-markdown paragraph combined into one cohesive sentence. | | `docs/design/tui/welcome.md` | "9 entries" → 11 (full registry size); "8-entry STARTER_POOL / TIP_POOL" → 9-entry; antithesis decision titles softened; em-dash chain in the live-feeds Out-of-Scope item turned into a conjunction; Sources alphabetized. | | `docs/guide/configuration.md` | Default `model` cell `claude-opus-4-7` → `claude-opus-4-7[1m]` (matches `DEFAULT_MODEL`) in both the `[client]` table and the env-var table; "opt-in rather than automatic" rewritten as "you have to opt in explicitly"; OAuth-paragraph "matches Claude Code" implementation leak trimmed. | | `docs/guide/instructions.md` | "More specific locations override broader ones" softened to "files closer to your working directory appear later in the prompt and conventionally take precedence", which matches the actual concatenate-in-walk-order behaviour. | | `docs/guide/sessions.md` | Mid-session resume description loses the implementation-internal "load + sanitize pipeline" phrase; `/rename` interaction with the AI title generator added to the Titles section. | | `docs/guide/slash-commands.md` | Theme bullet's comma-spliced enumeration restructured; `/resume` description's em-dash run split; persistence-stance em-dash dropped. | | `docs/guide/theming.md` | `Color::Reset` (internal Rust type) → user-facing `reset`; relative-paths antithesis sentence rephrased; "same routing applies in both modes" empty restatement dropped. | | `docs/research/api/anthropic.md` | Per-model beta-set body trimmed of one staircase-narration sentence; em-dash chain on first-party-vs-3P fingerprint paragraph rewritten with conjunctions; "billing plumbing, not a security boundary" reworded; `prompt-caching-scope` paragraph reflows. | | `docs/research/api/extended-thinking.md` | `signature_delta` line restored to a clean parenthetical; credential-rotation em-dash chain split. | | `docs/research/api/system-prompt.md` | "absent → default (org-scoped) ephemeral cache. Universally accepted" tightened; org-default rationale's three-clause stack rewritten as a colon-introduced list. | | `docs/research/slash/commands.md` | Comparison table `oxide-code` `Variants: 9` → `11`. | | `docs/roadmap.md` | "and bash output" dropped from rich-tool-views (bash uses the fallback view); silent-merge "Rejects Claude Code's …" parenthetical inlined; status-bar follow-up sentence flow tightened. | ## Test plan - [x] `pnpm spellcheck` — clean - [x] `pnpm lint` — clean - [x] All Sources entries verified to exist under `crates/oxide-code/src/` - [x] All count-bearing claims verified against source (`BUILT_INS = 11`, `STARTER_POOL.len() = 9`, `TIP_POOL.len() = 9`, `TRUNCATION_OVERHEAD = 80`, `DEFAULT_MODEL = "claude-opus-4-7[1m]"`) - [x] All `[link](path)` cross-doc references resolve
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
/themeopens a list picker that live-previews each candidate as the cursor moves; Esc snaps back, Enter commits for the rest of the session./theme <name>swaps directly to a curated built-in. Restart returns to[tui.theme] basein user config.Design decisions
ModalKey::Preview(action). Live-preview modals need to emit aUserActionon each cursor move without closing. Adds the missing third path alongsideConsumed/Cancelled/Submitted— composes with PR feat(slash): /effort slider + 1M-context display fix + DRY modal cancel #66's stack-level Esc / Ctrl+C cancel so individual modals don't reimplement the gesture.apply_action_locally(set-once-if-empty) avoids couplingAppto per-picker logic at modal-open time. Any future live-preview modal participates with no further wiring./theme <name>. Custom file-path themes still work via~/.config/ox/config.toml, but the slash form rejects paths since the picker can't list them.SubmitPromptactions go throughdispatch_user_action. The original wiring forwarded all slash-synthesized actions straight to the agent, which silently dropped TUI-only ones likeSwapTheme(so/theme lattewas a no-op). Routing through the local handler first lets TUI-only actions land while still forwarding agent-bound ones (Clear,SwapConfig).ChatView::set_themealso drops the streaming-prefix cache so an in-flight assistant turn re-renders under the new palette./thememade the registry exceedMAX_VISIBLE_ROWS; the previous... (N more)truncation hid/themefrom discovery. Footer dropped; the cursor anchors at the visual middle once it leaves the top half (Claude Code's typeahead convention) and at the bottom near the list end.surface = { bg = "reset" }so the chosen palette layers over the user's terminal bg — light themes adapt to pale terminals and Material composes over any dark backdrop. The frame still paintstheme.surface()over its full area before sub-widgets render so unpainted gaps don't leak the previous frame's contents.draw_framecollapses both layout slots so the chat reclaims the vertical space — applied at the layout level so future modals inherit it without per-modal opt-in.Changes
slash/theme.rsThemeCmd+ThemePicker, curated roster, live-preview key handling.slash.rs,slash/registry.rsThemeCmdintoBUILT_INS.tui/modal.rsModalKey::Preview(action)— emits an action without popping.tui/theme.rsload_builtin(name) -> Option<Theme>;expecton vendored TOML parse so a broken built-in surfaces at startup.tui/app.rspreview_theme_snapshot,apply_themehelper,PreviewTheme/SwapThemearms; route synthesized non-SubmitPromptactions throughdispatch_user_action; full-frame surface fill; collapse input + popup while a modal is on the stack.tui/components/input/popup.rs... (N more)footer; centered-cursor scroll via statelessscroll_offset; explicit surface bg.tui/components/{chat,status,input}.rsset_themefor mid-session repaint; chat invalidates the streaming cache.agent/event.rsUserAction::{PreviewTheme, SwapTheme}(TUI-only, agent-bypass).config.rsConfig.theme_name+ConfigSnapshot.theme_name,DEFAULT_THEME = "mocha".agent.rs,main.rs,client/anthropic/testing.rsUserActionvariants; theme-name fixture for test config.themes/{mocha,macchiato,frappe,latte,material}.tomlsurface = { bg = "reset" }so the theme adapts to the terminal palette.docs/guide/slash-commands.md,docs/roadmap.md,CLAUDE.md/theme; move from "current focus" to "working today"; add to crate tree.Test plan
cargo fmt --all --checkcargo clippy --package oxide-code --all-targets -- -D warningscargo test --package oxide-code— 1670 passedpnpm lint,pnpm spellcheck— cleanox,/theme, navigate Up / Down, confirm full repaint each row, Esc snaps back, Enter commits/theme latte,/theme mocha; confirm chat showsTheme set to ...for each/theme solarized; confirm error lists the curated built-ins/thememid-stream → picker opens (ReadOnly); typed/theme <name>mid-stream → refuses (Mutating gate)/themeswap mid-stream — confirm in-flight assistant tokens repaint under the new palette on the next frame/theme,/model,/effort,/status— confirm input row collapses while modal is open and reappears on Esc