fix(tui): accept the documented dark/light theme aliases - #523
Conversation
docs/configuration/themes.mdx lists `dark` and `light` as aliases for the default dark and light palettes. `Theme::from_name` resolves both, so the themes work — but `all_names()`, which `/color` uses to validate input, omitted them. `/color dark` was rejected as unknown while naming a theme that exists. Both are now in the registry, so `/color` accepts them and the picker offers them. Adds two guards against the pattern this repo keeps hitting — a theme picker that was unreachable, `/vim` setting a key nothing read, keybindings listed as active that never fired: every_documented_theme_name_is_accepted checks each identifier the theme docs advertise against what /color will take. every_documented_command_exists parses the command reference's table rows and asserts each one is a real command. It reads table rows only, so prose mentioning a slash command in passing is not treated as a claim, and it refuses to run if extraction finds implausibly few commands — otherwise a table reformat would silently make it vacuous.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5243bf122a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| "dark".to_string(), | ||
| "Dark (default dark palette)".to_string(), |
There was a problem hiding this comment.
When a user has a valid custom theme named dark.toml (or light.toml), this new “default” option does not select the default palette: from_name calls lookup_palette before handling aliases, so the custom palette shadows the alias, and the later user-theme loop also adds a second picker row with the same ID. Thus both rows select the custom theme while one is misleadingly labeled as the default. Either resolve these reserved aliases before user palettes or exclude/deduplicate colliding custom IDs.
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bb9c612982
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| "dark".to_string(), | ||
| "Dark (default dark palette)".to_string(), | ||
| ), | ||
| ( | ||
| "light".to_string(), | ||
| "Light (default light palette)".to_string(), |
There was a problem hiding this comment.
When a user has a valid custom dark.toml or light.toml theme, these alias rows are followed by a user-palette row with the same identifier. Since Theme::from_name resolves an exact user palette before handling aliases, selecting “Dark (default dark palette)” actually previews and applies the custom palette, while both picker entries resolve identically and /color repeats the name. Deduplicate these identifiers or omit the alias row when a user palette shadows it.
Useful? React with 👍 / 👎.
| let missing: Vec<&String> = claimed | ||
| .iter() | ||
| .filter(|c| !is_builtin_command(c) && !is_interactive_slash(&format!("/{c}"))) | ||
| .collect(); |
There was a problem hiding this comment.
If an interactive command's COMMANDS entry is removed or misspelled while its hard-coded name remains in is_interactive_slash (for example theme, model, or scroll), this test still considers it implemented. However, execute() dispatches only entries from COMMANDS, and the modern TUI also gates slash execution through is_builtin_command, so the documented command would instead become a prompt while this guard passes. Check registration directly rather than accepting routing metadata as proof of implementation.
Useful? React with 👍 / 👎.
| let accepted = super::Theme::all_names(); | ||
| let documented = [ | ||
| "auto", |
There was a problem hiding this comment.
This test never reads docs/configuration/themes.mdx; it checks a manually copied list instead. If the documentation later adds or renames an advertised identifier without updating this array, the test continues to pass even when /color rejects that identifier, defeating the stated documentation-regression guard. Extract the identifiers from the documentation, as the command test does.
Useful? React with 👍 / 👎.
Three review follow-ups, all cases where a check could pass while the thing it guards had drifted: - all_names/all_options listed the dark/light aliases even when a user palette owns those ids. from_name resolves the user palette first, so the alias row could never win — the picker offered the same id twice and one entry was a lie. Alias rows are now dropped when shadowed. - every_documented_theme_name_is_accepted restated the documented ids in an array, so adding or renaming a theme in the docs left the test passing. It now parses the theme table out of themes.mdx (scoped to that table — the page also documents colour keys in backticked tables) and asserts the parse is non-trivial so a format change cannot make it vacuous. Verified by renaming a documented theme: the test fails, naming it. - the documented-commands test accepted is_interactive_slash as proof of implementation, but that helper carries its own hard-coded list; a command whose COMMANDS entry was dropped or misspelled stayed 'implemented' while execute could no longer dispatch it. Registration in COMMANDS is now the requirement, plus a check that every interactive-classified name is registered too.
|
@codex review — all three addressed: alias rows are dropped when a user palette shadows Separately: the Coverage failure on this PR is the known |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 86ff3ce4b3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let mut seen = std::collections::HashSet::new(); | ||
| names.retain(|n| seen.insert(n.clone())); |
There was a problem hiding this comment.
When a user has a valid dark.toml or light.toml, this retains the leading alias in all_names, while all_options removes that alias and keeps the custom palette at the end. Consequently theme_picker.rs::every_registry_theme_is_offered fails its exact-vector assertion based solely on the user's real config directory, so the CLI test suite is no longer hermetic for these supported custom-theme IDs; derive both catalogs using the same shadowing and ordering rule.
AGENTS.md reference: AGENTS.md:L63-L67
Useful? React with 👍 / 👎.
| let is_alias = (id == "dark" || id == "light") | ||
| && (label.contains("default dark palette") | ||
| || label.contains("default light palette")); |
There was a problem hiding this comment.
When the user supplies auto.toml, from_name("auto") resolves that exact user palette before running automatic detection, but this predicate only recognizes dark and light as aliases. The subsequent deduplication therefore keeps the misleading Auto (match terminal) row and discards the user's labeled row, so the picker hides the custom palette's identity while applying it; auto needs the same shadow handling.
Useful? React with 👍 / 👎.
| // The interactive list names commands too; every one of those must | ||
| // be registered as well, or `execute` will never reach it. | ||
| for name in ["theme", "model", "scroll", "editor", "open"] { |
There was a problem hiding this comment.
This list does not cover every name classified by is_interactive_slash: it omits at least session, powerup, uninstall, plugin, and team-remember. If any of those entries is removed or misspelled in COMMANDS, resolution falls back to the raw name and the existing interactive-classification assertions still pass, while execute can no longer dispatch the command, so the new registration guard misses precisely that regression for the omitted names.
Useful? React with 👍 / 👎.
Follow-ups on my own previous fix, which traded three hardcodings for two more: - all_names and all_options each built their own list, so a shadowed alias was dropped from one and kept in the other — the catalogs disagreed about which ids exist and where they sit. all_options is now the only builder and all_names derives from it. - alias shadowing covered dark/light but not auto, though from_name resolves a user auto.toml ahead of automatic detection just the same. All three aliases are now dropped when a user palette owns the id. - the registration test restated a five-name subset of the interactive commands, omitting session, powerup, uninstall and any name added later. The classifier's list is now a public const that both is_interactive_slash and the test read, so the test cannot fall behind it. Verified by adding an unregistered name to the const: the test fails and names it.
|
@codex review — all three addressed at the root rather than patched:
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a60e53c0c9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| ("light", "Light (default light palette)"), | ||
| ] | ||
| .into_iter() | ||
| .filter(|(id, _)| !shadowed.contains(id)) |
There was a problem hiding this comment.
auto to the user palette
When <config>/themes/auto.toml exists, this filter removes the built-in alias row and leaves the user-labeled row as the sole auto option, but the runtime facade still intercepts that ID: theme_runtime.rs::Theme::from_name and resolve_theme convert auto to one-dark or solarized-light before the legacy palette lookup, and /color and the theme pickers use those paths. Consequently, selecting the advertised custom row silently applies the detected built-in theme instead of the user's palette; only shadow the alias after runtime resolution can preserve a custom auto ID.
Useful? React with 👍 / 👎.
The catalog now lists a user <config>/themes/auto.toml as the sole 'auto' row, but the runtime facade intercepted the id before any palette lookup: from_name and resolve_theme both converted 'auto' to one-dark or solarized-light, so selecting the advertised row silently applied a built-in the user never chose. Both sites now defer when a user palette owns the id, matching how dark/light already shadow. The rule is a pure auto_detection_applies so it is testable without mutating the process environment — writing a themes dir would have needed XDG_CONFIG_HOME, and process-global env in tests is exactly the pattern behind the flake fixed in #528.
|
@codex review — fixed: both runtime intercepts ( Note on this PR's red check: |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0427473d09
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // A user palette named `auto` owns the id: the catalog lists it | ||
| // as the only `auto` row, so intercepting it here would apply a | ||
| // detected built-in instead of the palette the user picked. | ||
| if auto_detection_applies(name, legacy::user_palette_exists("auto")) { |
There was a problem hiding this comment.
When name is any value other than auto, Rust still evaluates legacy::user_palette_exists("auto") before auto_detection_applies can reject it. That helper synchronously reads and parses every user theme file, and the onboarding picker calls Theme::from_name on every redraw, so navigating standard themes now repeatedly performs blocking filesystem I/O and re-emits warnings for malformed palettes. Short-circuit on name == "auto" before querying the user catalog; the same eager evaluation also occurs in resolve_theme.
AGENTS.md reference: AGENTS.md:L124-L126
Useful? React with 👍 / 👎.
auto_detection_applies took the ownership flag by value, so every theme resolution evaluated it — and that helper reads and parses every file in the user themes directory. The onboarding picker resolves a theme on each redraw, so arrowing through the standard palettes scanned the disk once per keystroke. The check is a closure now, run only for the literal 'auto'. A test asserts it is not invoked for any other name.
|
@codex review — fixed: the ownership check is now a closure, so |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: eada28be5c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| pub fn resolve_theme(configured: &str) -> String { | ||
| if configured == "auto" { | ||
| // Same rule as `Theme::from_name`: a user `auto.toml` owns the id. | ||
| if auto_detection_applies(configured, || legacy::user_palette_exists("auto")) { |
There was a problem hiding this comment.
auto palette as explicit
When a valid themes/auto.toml exists and [ui].inherit_fg = true, this condition returns false and resolve_theme preserves the literal auto; callers then pass configured_name == "auto" to init_with_options, which marks the theme as automatic and causes current() to replace the custom palette's text color with the terminal foreground. Thus the newly supported user palette does not fully shadow the alias as intended, unlike custom dark/light palettes; propagate whether detection actually applied rather than inferring Auto mode solely from the configured string.
Useful? React with 👍 / 👎.
With themes/auto.toml present, resolve_theme now preserves the literal 'auto', so callers still passed configured_name == "auto" to init_with_options and ActiveOptions::auto came out true. Under [ui].inherit_fg that rewrites the text slot — of a palette the user wrote and picked deliberately. The flag now uses the same auto_detection_applies predicate as the resolution path, so the two cannot disagree about what 'auto' meant.
|
@codex review — fixed: |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4d7c8c4094
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| ("light", "Light (default light palette)"), | ||
| ] | ||
| .into_iter() | ||
| .filter(|(id, _)| !shadowed.contains(id)) |
There was a problem hiding this comment.
auto as the first option when shadowed
When a valid user themes/auto.toml exists, this filter removes the leading auto row and the user row is appended after every standard palette, so Theme::all_names().first() becomes dark. That makes the existing all_names_starts_with_auto_and_lists_standard_ids unit test depend on the developer's real config directory and fail for the newly supported custom-auto scenario; replace the leading row with the user's label or isolate the test/config lookup so the default test path remains hermetic.
AGENTS.md reference: AGENTS.md:L63-L67
Useful? React with 👍 / 👎.
Dropping the alias row and appending the user palette moved the id to the end of the catalog, so all_names().first() became 'dark' whenever a themes/auto.toml existed — and the existing ordering test then passed or failed depending on the developer's own config directory. The alias row now carries the user's label in place, so ids and their order are identical whether or not a palette shadows one. The rule is a pure catalog(user) so it is testable without a themes directory; a test covers the shadowed and unshadowed shapes.
|
@codex review — fixed by keeping the alias row in place with the user's label rather than dropping it and appending the palette. That preserves |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3dfc58a87f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| /// accepted, or the docs are promising a theme `/color` will reject. | ||
| #[test] | ||
| fn every_documented_theme_name_is_accepted() { | ||
| let accepted = super::Theme::all_names(); |
There was a problem hiding this comment.
When XDG_CONFIG_HOME or the platform config directory contains user themes, this call traverses and parses those external *.toml files through all_names(); stable ordering does not make the test hermetic, and a special or blocking file such as a .toml FIFO can even hang the test. Exercise the new pure catalog with explicit inputs, or inject an isolated themes directory, instead of reading the developer's real config.
AGENTS.md reference: AGENTS.md:L63-L65
Useful? React with 👍 / 👎.
Five tests asserted on built-in ids while calling all_names(), which traverses and parses the user's themes directory. That makes them depend on the developer's machine — a stray .toml changes the answer, a malformed one fails an unrelated assertion, and a special file such as a FIFO can block the read outright. They now use a pure catalog_ids(Vec::new()): the shipped palettes are what each of them is actually about. all_names() keeps its disk read for the real callers.
|
@codex review — fixed, and widened to the class: the docs test now compares against a pure |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 78f7af11ba
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| pub fn resolve_theme(configured: &str) -> String { | ||
| if configured == "auto" { | ||
| // Same rule as `Theme::from_name`: a user `auto.toml` owns the id. | ||
| if auto_detection_applies(configured, || legacy::user_palette_exists("auto")) { |
There was a problem hiding this comment.
When the default [ui].theme = "auto" is used and the themes directory contains any .toml FIFO or symlink to one, the modern startup path calls resolve_theme, and this new predicate invokes user_palette_exists, which loads every palette and blocks in read_to_string before terminal detection. Previously auto went directly to detect_system_theme; check only a regular auto.toml candidate rather than parsing unrelated directory entries.
Useful? React with 👍 / 👎.
user_palette_exists loaded and parsed every file in the user's themes directory, and the default theme = "auto" takes that path during startup: an unrelated malformed file was parsed for nothing, and a .toml FIFO blocked before terminal detection. A palette id is its file stem, so open only that candidate, and stat it first so a non-regular file is rejected without a blocking read.
|
@codex review Fixed the P2 on
Behaviour I deliberately kept: a malformed Three tests added, all pure over a temp dir: only-own-candidate (a sibling malformed file and a Gate: fmt, clippy |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 150b37c8e2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // A user palette named `auto` owns the id: the catalog lists it | ||
| // as the only `auto` row, so intercepting it here would apply a | ||
| // detected built-in instead of the palette the user picked. | ||
| if auto_detection_applies(name, || legacy::user_palette_exists("auto")) { |
There was a problem hiding this comment.
When auto.toml is valid, this predicate returns false and falls through to legacy::Theme::from_name("auto"); its lookup_palette calls user_palettes(), which reads every .toml entry in the directory. Consequently, startup with a custom auto.toml still parses all sibling themes and can block indefinitely on an unrelated .toml FIFO, despite the new candidate-specific stat guard. Load and return the validated candidate directly, or otherwise ensure this fallthrough does not rescan the directory.
Useful? React with 👍 / 👎.
The previous commit stopped the scan in the existence check, but a
valid auto.toml made that predicate fall through to from_name("auto"),
whose lookup_palette called user_palettes() and read the directory
anyway — so startup with a custom auto theme still paid for it.
Route both by-id paths through one targeted loader. load_palettes_dir
stays for the pickers, which do want every palette.
|
@codex review You were right and my previous fix was incomplete — pushed I stopped the directory scan in Both by-id paths now go through one targeted loader ( New test Gate: fmt, clippy |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fff4927b47
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| fn user_palette_in(dir: &Path, id: &str) -> Option<Palette> { | ||
| // `id` reaches the filesystem, so it has to name a file *in* `dir` | ||
| // and not a path that walks out of it. | ||
| if Path::new(id).components().count() != 1 || id.starts_with('.') { |
There was a problem hiding this comment.
When a valid custom theme is named .foo.toml, load_palettes_dir advertises it in the picker with the ID .foo, but resolving that selection or the equivalent config value now returns None here and silently falls back to the default theme. A leading dot is a safe single-component file stem, so reject traversal components without rejecting every dot-prefixed ID.
Useful? React with 👍 / 👎.
The traversal guard rejected every id starting with a dot, but ids come from the file stem, so .foo.toml is listed in the picker as .foo — and choosing it fell back to the default theme instead. Reject the traversal components themselves: only a single Normal component that round-trips to the id is accepted, which still refuses . and .. and anything with a separator.
|
@codex review Fixed at The guard now rejects the traversal components rather than a leading character: the id must be a single Test Gate: fmt, clippy |
|
Codex Review: Didn't find any major issues. Breezy! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
1 participant
Summary
docs/configuration/themes.mdxlists:Theme::from_nameresolves both:…but
all_names()— which/coloruses to validate input — omitted them. So/color darkprinted "Unknown theme" while naming a theme that resolves perfectly. Both are now in the registry, so/coloraccepts them and the picker offers them.The more useful half: making the docs checkable
This repo keeps shipping advertised-but-absent features. Just this week: a theme picker that was unreachable (#505),
/vimsetting a config key nothing read (#522), keybindings listed as active that never fired (#511),/resumewhose description promised a picker that didn't exist (#518). Every one was found by hand.Two guards so the next one is found by CI:
every_documented_theme_name_is_accepted— checks each identifier the theme docs advertise against what/colorwill take. This is what caughtdark/light.every_documented_command_exists— parses the command reference's table rows and asserts each is a real command.Two details that keep the second one honest:
|/foo| … |), so prose like "Type these at the start of a prompt (not as/commands)" is not mistaken for a claim. That line is why a naive grep reports a false positive.Mutation-checked: injecting
|/teleport| Beam the repo to Mars |into the docs fails the test withdocumented but not implemented: ["teleport"]; removing it passes.Two register rows were wrong, and are corrected rather than "fixed"
escinRESERVED_CHORDSwith a test that a hostile binding cannot steal it.docs/tui/README.md"still claimsstart_paused" as though it were false. It is true: there are 4start_pausedtests in the tree. No change needed.Verification
646 bin tests pass;
clippy --all-targets -- -D warningsandfmt --checkclean.