Themes: Accept a dark/light theme table - #985
Open
masonmcelvain wants to merge 9 commits into
Open
Conversation
Contributor
|
PR author is not in the allowed authors list. |
|
@masonmcelvain is attempting to deploy a commit to the Modem Team on Vercel. A member of the Team first needs to authorize it. |
Member
|
Really like the selection vs. resolved theme split here, and the overall behavior makes sense. I found a few things I think we should address before merging:
I'd strongly suggest validating the generated TOML before replacing the config file, and avoiding rewriting The feature and core model look good, but I don't think the config-writing path is safe to merge as-is. |
`theme = "auto"` only ever chose between github-light-default and github-dark-default, so a reader who wanted one-light and one-dark-pro had no way to follow their terminal. Accept a `[theme]` table naming both sides instead, with an optional `fallback` for terminals that never answer the background probe, following Helix's config shape. The committed preference now stays whatever config asked for until someone picks a theme in the selector, so quitting without touching themes no longer rewrites `auto` — or a pair — into the one id it happened to resolve to. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013Y3vNR6iJEH4epjQo8JARe
The theme writer only recognized the `theme = ` and `[theme]` spellings, while the reader takes whatever Bun.TOML.parse produced. A config written as `theme.dark = "nord"` read fine, then gained a second `theme = "..."` assignment on the next preference save, after which Hunk refused to start with `BuildMessage: Cannot redefine key 'theme'` — an error naming neither the file nor the key. A quoted key inside `[theme]` failed the same way. Match every spelling TOML accepts for a key, and drop the extra lines a dotted key spreads a value over. Saves also deleted comments. The `[theme]` range ran to the next section header, so collapsing the table took the blank lines and comments that introduce whatever follows, and every rewritten key lost its trailing comment. Since one save rewrites all nine preferences, toggling `wrap_lines` stripped comments out of an untouched `[theme]` table. Comments now stay attached to the key or section they introduce, which is also why a collapsed table is written over its own header rather than appended — but only while `[theme]` is the first table, since a top-level key at any later position would scope into the table above it. Theme errors named `theme` whichever section or file held the bad table. That matters more now that a checked-in `.hunk/config.toml` can hard-fail startup for everyone in the repo, so they name both. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BLvy5LioGNhwJHzDPb7xxy
Saving view preferences could leave a config file slightly messier than it found it. Collapsing a [theme] table that sat between two other tables left the blank line from each side, and a file with no tables at all had new keys inserted above its trailing comment, because the backtrack that keeps a comment attached to the table it documents could not tell a real table index from the clamp used when no table exists. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NJVQRku6dLXkxiACCy177U
An in-session refresh rebuilt its reload input from the theme id the selection currently resolved to, so an adaptive pair arrived at the reload as whichever side the terminal happened to be on. Nothing collapses today because every refresh path keeps the mounted App, but the descriptor is the wrong thing to freeze. Carry the committed selection instead, and leave the resolved id to extension events, which want a concrete theme. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NJVQRku6dLXkxiACCy177U
masonmcelvain
force-pushed
the
feat/adaptive-theme-config
branch
from
September 7, 2026 14:28
c66827f to
904a4d4
Compare
Keep the selection beside the input in the history surface, resolve it per terminal in the interactive surface, and let static output fall through to the pair's fallback side since it never probes the terminal. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017mPP65ngDNSnKqBG3LU1K6
The line-based writer matched `[theme]` and `theme =` by regex, so it missed quoted headers like `["theme"]` and added a second root key, mistook a `[theme]` example inside a multiline string for the real table, and dropped comments on the header and inside the table when a single id replaced a pair. It also rewrote every preference on every save, so a mode-only change could still corrupt an untouched theme. Replace the regex matching with a small line scanner that tracks multiline strings and bracketed values and compares headers and keys by parsed path, hoist a collapsed table's comments onto the key, and only rewrite keys that differ from the loaded baseline. Before replacing the file, parse the candidate and require it to equal the original document with just the edited keys changed; otherwise leave the file alone and say what to set by hand. Writes go through a sibling temp file so a crash cannot truncate the config. Saves also learn the command and pager tables that shaped the session, so a key those tables define is written back there instead of to a root key the next run would override. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017mPP65ngDNSnKqBG3LU1K6
Carry the resolved command and pager scope from config resolution into the review and history bootstraps and on to the quit prompt, so a theme picked under a `[diff.theme]` or `[pager]` override lands in that table rather than in a root key the override would shadow on the next run. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017mPP65ngDNSnKqBG3LU1K6
Startup asked the terminal for its background right after the first config resolution, but an extension VCS backend can settle a repo root the bundled catalog could not, and that repo's config is only read in the second resolution. A `[theme]` pair introduced there never triggered the probe, so a light terminal drew the dark or fallback side. Move the probe after extension-backed resolution so it sees the final selection. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017mPP65ngDNSnKqBG3LU1K6
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017mPP65ngDNSnKqBG3LU1K6
masonmcelvain
force-pushed
the
feat/adaptive-theme-config
branch
from
September 7, 2026 16:32
904a4d4 to
d2a509b
Compare
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 free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Support a
[theme]table inconfig.tomlto set dark/light themes:fallbackcovers sessions where terminal background is unknown, defaulting todark.Writing the pair back exposed that the config writer was rewriting more of
config.tomlthan it should, so that is fixed here too.Changes
Theme selection
src/core/theme/selection.ts: aThemeSelectionis either a theme id or a{ dark, light, fallback? }pair, and the module owns reading, comparing, and resolving one against a terminal background.autodoes.--theme <id>overrides a configured pair for one run.theme_changedevents keep receiving a concrete id.Config writing
themein place instead of appending a second definition, including quoted ("theme" = …) and dotted (theme.dark = …) key forms, and drop duplicate definitions rather than leaving them behind.[theme]table; keep a comment attached to the table it documents.[theme]table to a top-leveltheme = "…"when a single id replaces the pair, leaving one blank line between the neighbours it separated.[theme]section.QA
Themes used below are interchangeable; any two built-in ids work.
[theme]table in~/.config/hunk/config.tomlwith distinguishabledark/lightids, then runhunk diffin a dark terminal and again in a light one. Each should draw its own side.fallbackand run through a host that does not answer the background query (ascript-allocated pty, or LazyGit as a pager). The fallback theme should render; removefallbackanddarkshould.hunk diff --theme github-dark-defaultwith the table still in config should ignore the pair.t, choose a theme, thenq. The prompt should show- theme = { dark = "…", light = "…" }/+ theme = "…", and saving should leave a top-leveltheme = "…"with no[theme]table.[custom_theme]table, and an[extensions]table around the[theme]table. After saving, they should be untouched, with no doubled blank lines, and the file should still parse.r, thent. One side of the pair should still be active, and quitting should not offer to save a theme change.light, or with a typo'd key, should exit 1 naming the file and the problem.