Repository navigation
Add a custom theme, a background and a primary colour of the contributor's own - #650
Merged
Merged
Conversation
…tor's own The prototype's fourth theme (#560). Under Custom, the General tab asks for two colours, typed as hex or picked with the system's picker behind the swatch, and the design system builds every other colour from them, as it builds the dark theme from its one seed. The prototype calls the second colour the accent; the design system's name for it, primary, is used. A custom theme is a scheme too: main gives Electron light or dark by whether the background reads better with black text or with white, so the native form controls and the window's chrome match the page, and makes and paints the window in the background chosen. The window reads the settings above the design system's provider now, since the theme is one of them, and the terminal re-reads its colours whenever the theme as painted changes, by a name that changes with it. A colour that is not one is refused in main's words and the field goes back to what is kept; three hex digits are kept as six. The dialog says when the design system could not reach the contrast it wants between two of the colours it built, and keeps the colours all the same. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…ion tests From the first review pass. A colour picked with the picker was never kept: the input was controlled by the colour kept, so it was put back to it after every step of a drag, and the picker's closing read the old colour. It is driven from the draft now, and the journey picks a colour. The fallback before the settings arrive is the module's 'system', not a branch in the component; the hex fields are in the terminal's type, which the rule for them reached only with a type on the input; the wiring test's Trac assertion opens a Trac window now, and the main window's colour is named as its own. The dark crossover is pinned, a colour spelled another way is shown as kept, and a prop nothing passed is gone. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…yping From the second review pass. What the field shows once main has answered a keep, and what the picker holds while the field holds no colour, are settings-view's to say, with their cases in its suite; an answer to an older draft leaves what has been typed since. The wiring test's Trac assertion seeds a path rather than building a repository it never opens, and the journey says where its two exact colours come from. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configuration
You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
The swatch sat in the field's row without the slot the design system pads and centres a prefix with: it stuck to the top of the row, the row shrank it narrower than it is tall, and the gap before it was one of its own. It stands in the slot now, with the slot's minimal padding, centred, and a square of its own size the row cannot shrink. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…er is removed The Windows unit job failed on this branch's head in a test this branch does not touch: the test's read helper settled on the stream's `end`, the stream closed its file a turn later, and the test's cleanup removed the folder in between, which Windows refuses while a file in it is open. The helper settles on `close`. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
zaerl
added a commit
that referenced
this pull request
Oct 7, 2026
…ed from the app's first render (#651) ## Why The follow-up recorded in #650: a custom theme was painted after a first render in the standard theme of its scheme, a flash of the wrong colours at every launch, because the settings, of which the theme is one, were read in an effect once the app had mounted. Part of #560. ## What changes **The settings are read before the first render**, beside the locale, which the window already waited for; the two reads run together, so the first paint waits for neither longer than before. What was read is handed to the hook that holds the settings, which asks main again only where that read failed, as it did before. No new bridge surface: the same ask, made earlier. **What remains:** before the page's script runs at all, the page paints the stylesheet's light surface, or under the dark scheme the dark seed `index.html` paints, over a window frame that is already the custom colour; a custom theme shows that for that moment, then its own colours. The review's idea of a transparent body until the provider mounts, so the frame shows through, was tried and measured: in the light scheme the window's colour did show through, but under the dark scheme Chromium paints its own canvas colour, `#121212`, over the frame, so a custom dark theme would still flash, and the standard dark theme would flash a grey that is not its seed. Not taken. The page cannot know its colour before its script runs without being handed it, which is new bridge surface. Mail rendering is unchanged here and tracked separately. ## How to test this Platforms: any. The journeys drive this on macOS and Windows. From the repository root, after `npm ci` and `npm run build:once`: ``` npx playwright test --project=journeys settings.spec ``` - `settings.spec.js`, the custom theme journey, at its end: the page is reloaded with a script in it before any of its own, which notes the document's own style at the moment the app mounts, before anything is painted; the custom colours are in it, and not the standard theme's, and not none. On the code before this change the journey receives an empty style there, since a custom light theme's first render was the standard light theme, which puts nothing on the document. Nothing else is new to test: the settings dialog's journeys and the language relaunch cover the hook as it is handed the settings. **Worth a look by hand** (any platform, the current head): Settings → General → Theme → Custom, a background such as `fff8e1`, quit, open the app: the window is cream from the app's first render, with no moment of the standard light theme after the page has mounted. With `102030`: navy from the app's first render; what the page paints before its script runs is the dark seed. **What must not have happened:** the app failing to open its window when the settings cannot be read (the read's failure is logged and the hook asks again); a slower first paint; the language relaunch offer appearing on a fresh launch. **What I could not test:** the packaged smoke on this head, since my own copy of the app held the single-instance lock; the change is the renderer's and the bridge is unchanged. ## Risks and limitations - Review: 2 passes, 1 finding fixed here, the follow-up tried and not taken, every style note applied. Details below. - A custom dark theme still shows the dark seed for the moment before the page's script runs. See above. ## Related Follow-up of #650, part of #560 and #542. --- <details> <summary>Review outcome (required — see AGENTS.md)</summary> **Pass 1: 1 [fix here] · 1 [follow-up] · 0 style notes. Pass 2 (on the fix-up): 0 [fix here] · 0 [follow-up] · 2 style notes. The finding fixed here, the follow-up tried and not taken, both style notes applied.** - **Review:** completed — a separate agent context, read-only; head `c3801dd` / base `e0ed3ee`; the hook's seeding against the language relaunch's `loaded`, the two reads' failure paths and timing, the reload against the deep-link queue and the provider's root count, the observer's timing against React's commit, the journey red on the old code; ESLint and the unit files run; 1 [fix here], 1 [follow-up]. - Fixed: the comment, the journey and the commit said "first frame" and "before anything is painted", where the page paints before its script runs; they say the app's first render. - Follow-up, tried and not taken: a transparent body until the provider mounts. Measured above. - **Review:** completed — a second separate agent context, read-only; head `d434c55` / base `e0ed3ee`; the wording checked place by place, the observer's timing against the provider's layout effect; ESLint and the unit files run; 0 findings, 2 style notes. - Style notes, applied: the first commit's message still said "first frame", so the fix-up is folded into it under the corrected words; the branch was named for the first paint and is named for the first render. - **Since review:** the fold and the rename, no change to the tree since `d434c55`. On the tree: `npm run lint`, `npm test`, `npm run test:electron` (2045), all 133 journeys and the docs build pass; the packaged smoke could not run, since the maintainer's own app holds the single-instance lock, and the bridge is unchanged. </details> <details> <summary>Implementation notes</summary> - `src/renderer/index.jsx`: `loadSettings`, `Root({ initialSettings })`, the two reads before the first render. `src/renderer/hooks/use-settings.jsx`: `useSettings(initial)`. `src/renderer/components/app-theme.jsx`: the comment. - `tests/e2e/journeys/settings.spec.js`: the reload with the mount observer. </details> <details> <summary>Screenshots or recording</summary> Not attached: what changed is the first frame, which a picture taken later does not show. The journey reads it. </details> 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com>
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
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.
Why
The prototype's dialog has a fourth theme, and the dark theme of #642 left it out: a background and an accent colour of the contributor's own, from which the design system builds every other colour as it builds the dark theme from its one seed. This adds it, under the name the design system gives the second colour: primary. Part of #560.
What changes
Custom, on the General tab. Under Theme, a fourth answer; chosen, two fields appear beneath it: Background and Primary, each typed as hex or picked with the system's colour picker, which is the swatch before the field. A colour is kept as six lowercase hex digits from three or six as typed; one that is not a colour is refused in main's words and the field goes back to what is kept. The picker's choice is kept when the picker is done, not at every step of a drag, since each keep is a write to the store; the field shows the colour under the pointer meanwhile. The colours start as the light theme's.
A custom theme is a scheme too. Main gives Electron light or dark by whether the background reads better with white text than with black, the crossover the design system's own ramp turns at, so the native form controls and the window's chrome match the page; and the window is made and painted in the background chosen, as is the Trac window, which main now hands its colour. The window reads the settings above the design system's provider, since the theme is one of them, and gives the provider the two colours; the terminal re-reads its colours whenever the theme as painted changes, by a name that changes with it.
When the colours do not read. The design system says when it could not reach the contrast it wants between two of the colours it built, and under Custom the dialog passes that on in one sentence. The colours are kept all the same: a contributor who chose them can read them, and the sentence is for the one who did not mean to.
Not in this pull request:
How to test this
Platforms: any. The journeys drive this on macOS and Windows. From the repository root, after
npm ciandnpm run build:once:settings.spec.js, the custom theme journey: the colours are asked for under Custom alone and start as the light theme's; a dark background typed and entered is kept, Electron is told the scheme, and the page, the body and the window are painted in it at once; a primary colour in three digits is kept in six and the brand surface is built from it; a colour spelled another way shows as kept; a colour picked with the picker is kept; a colour that is not one is refused in main's words and what is kept stays; a light background makes a light theme again; started again, the window is made in the background and the fields show the colours kept.i18n.spec.js: the control in the pseudo-locale.Six claims were broken one at a time and each turned the journey red: the page not seeded from the colours, Electron not told the scheme, a refusal leaving the typed text, three digits not expanded, the window not painted in the background, and the picker driven from what is kept rather than the draft, which was the bug the first review pass found.
tests/unit/theme.test.cjsholds the resolution, the crossover and the hex forms;settings.test.cjswhat is accepted;ipc-wiring.test.cjswhat Electron, the window and the Trac window are given.Worth a look by hand (any platform, the current head): Settings → General → Theme → Custom. Type
102030into Background and press Enter: the whole window is navy, the dialog included, and the native controls (the folder field's button, a select's list) are dark. Press the Primary swatch, drag in the picker: the field follows the pointer; close the picker: the buttons take the colour. Typenavy: refused under the fields, the field back to the colour kept. Typefff8e1: a cream theme with dark text and light controls. Quit and open the app: it opens in the cream, after a moment of the standard light theme.What must not have happened: the standard themes looking any different (the guide's pictures are the reference); a picker drag writing the store at every step; a colour kept that the field does not show; a server or build touched by a change.
What I could not test: Windows and Linux pickers, which the OS draws; the journeys drive the picker through its value, as a picker does when it is done.
Risks and limitations
Related
Part of #560, which is part of #542. Follows #642 and #649.
Design decisions and alternatives considered
Review outcome (required — see AGENTS.md)
Pass 1: 4 [fix here] · 1 [follow-up] · 4 style notes. Pass 2 (on the fix-up): 2 [fix here] · 0 [follow-up] · 3 style notes. All 6 fixed here, every style note applied, 1 follow-up left.
8b3658a/ base59afb0c; the store rule against the changed handlers, the theme's ordering in main against Electron'sthemeSource, the luminance maths against the design system's own ramp direction, React's controlled-input restore against the colour picker'schange, the design system's input and the provider's warnings; ESLint and the unit files run; 4 [fix here], 1 [follow-up].7fdf4e0/ base59afb0c; the picker's event order on a real picker and under Playwright's fill, the listener's double-commit, the accent ramp's first step for the journey's exact colours, the crossover recomputed; ESLint and the unit files run; 2 [fix here].7fdf4e0 → 0a9965dis those two and the notes, not reviewed again;31c125cstands the swatch in the design system's prefix slot as a centred square, from the maintainer's look at the dialog, not reviewed again. On0a9965d:npm run lint,npm test,npm run test:electron, all 133 journeys, the packaged smoke and the docs build pass; on the head:npm run lint,npm run test:electron(2045) and all 133 journeys, the packaged smoke not run since the maintainer's own app held the single-instance lock. The commit after it fixes a Windows-only cleanup race intests/unit/log-tail.test.cjs, a test this branch does not otherwise touch, which failed the Windows unit job on31c125c: the read helper now settles on the stream'sclose, so the folder is removed after the file is.Implementation notes
src/theme.cjs:THEMESwith 'custom',THEME_KEYS,resolveTheme,nativeThemeSource,normalizeHexColor,isHexColor,isDarkColor,PRIMARY.src/settings.cjs:customBackground,customPrimary,acceptColor.src/main.js:themeSettings,applyTheme,currentTheme,paintWindowForTheme;settings:setapplies for any theme key; the Trac window is handedbackgroundColor.src/trac-view.js:deps.backgroundColor.src/renderer/components/app-theme.jsx:AppTheme({ settings }),useThemeKey,useThemeWarnings.src/renderer/index.jsx:Rootreads the settings above the provider.src/renderer/hooks/use-site-terminal.jsx: re-reads on the theme's name.src/renderer/components/settings-dialog.jsx:ColorField, the fourth option, the warning.src/renderer/settings-view.cjs:themeItems.src/renderer/shell.css:.theme-colors,.color-field,.color-swatch.tests/unit/theme.test.cjs,settings.test.cjs,settings-view.test.cjs,ipc-wiring.test.cjs,trac-view.test.cjs;tests/e2e/journeys/settings.spec.js.Screenshots or recording
Not attached: I have no way to upload images from the command line. The custom theme journey drives everything that changed;
SHOTS_THEMEtakes light or dark only, so a picture of a custom theme is a hand's work.🤖 Generated with Claude Code