Repository navigation
Start the server or the watch when a site is opened, and start again what a quit stopped - #641
Merged
Merged
Conversation
…what a quit stopped The fourth of the settings (#559). Two switches start a site's development server, its build watch or both when the site is opened, as a press of Start would: the row becoming the open one is the edge, consumed once it is ready for it, and nothing starts under a setup, an update or a deletion, nor what is already running. A choice for the quit remembers which sites had a server or a watch running: the quit stops them, as it always has, and the next launch starts them again, whichever site it opens on. Main writes the list in the quit handler, through the store's synchronous accessor, from what it tracks of its children; the window reads it once as it opens, and main forgets it as it is read. The stickiness journey sizes its window itself, as its neighbour does: on a tall screen the tray at its largest still leaves the details room, which its second half needs it not to. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…server, and keep the sweep from a store that cannot be written From the review. Which edge to consume and what to start on opening a site is a pure module with its own tests, and the watch is started after the server's start has answered where that start did not bring it up, instead of predicting from a copy of whether the site is built. A site that had both its server and its watch running is remembered under both, which the guide promised. A store that cannot be written at quit is logged and does not stop the sweep. The journeys' sleeps are the round trip to main that TESTING.md names as the signal for nothing asked; the stickiness journey waits for its window to be resized and says what size the window opens at. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…ould not be started From the second review. A start asks main only after awaits of its own, so a round trip to main was not a sign that nothing was asked; what a start does first is set the site's controls to starting, so the journeys wait for the settings to be in the page and the effects to have run, and then read the controls. The resume journey seeds the list a quit now writes, a Core site's watch beside its server, and holds that the watch is asked for once. A start that could not be made is said in the log, where it was swallowed; a watch the server's start was refused is not asked for a second time; and the quit's test reads the log as well. 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 |
zaerl
added a commit
that referenced
this pull request
Oct 6, 2026
## Why #560, and the last row of #559's dialog. The window painted light only, and said so: its colour-scheme declaration was held to `light` by a test, since the older component library ships light styles and a promise of dark that only reached the native form controls gave charcoal inputs on white cards. A contributor whose machine is in dark mode got a white window all evening, with no way to say otherwise. This adds a dark theme, chosen in the settings or following the system. ## What changes **Theme, on the General tab**, under Appearance: **Light**, **Dark** or **System**, which is the default and follows the operating system. A change is on screen as it is made; nothing running is touched. The prototype's fourth answer, a custom pair of colours, is not offered. **One source of truth.** Main gives the choice to Electron's `nativeTheme`, read from the store before the window is made and set again as the setting changes. Chromium answers the page's `prefers-color-scheme` from it and paints the window's chrome and the native form controls to match, so the window only reads that answer: a wrapper around the design system's provider seeds the dark ramp from the prototype's dark background when the scheme is dark, and passes no colour when it is light, so the light theme is pixel for pixel what it was and no picture in the guide moves. The provider builds every token from the seed, for its own components, for the older library's through the variables it maps for them, and for the app's own styles; the diff and log panes follow through their tokens. **What the provider does not reach.** The terminal takes its colours as values and reads them again when the scheme changes. The older library's dialogs and popovers are painted white in its own stylesheet, with no token behind the colour, and are painted here with tokens; the popover's ring and shadow are restated in the dark scheme only. **No white before the page paints.** A dark window is made in the dark colour and kept in it as the theme changes, and the page paints its body the same colour under the dark scheme until the provider has mounted, which it marks on the document. `color-scheme.test.cjs` now holds the declaration to `light dark` and that colour to the theme module. **The pictures and the journeys.** Playwright holds a page to the light scheme whatever the machine says, so the guide's pictures are pinned to light through the app's own setting (`SHOTS_THEME=dark` for a dark one), and every journey stays light; the theme journey alone lets the page follow the app. **Not in this pull request:** - The patch and Trac windows, which are made without a theme and paint their own bar white. Follow-up. - A custom theme. - 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 i18n.spec terminal.spec logs.spec ``` - `settings.spec.js`, the theme journey: stored dark, the window starts dark, Electron is told, the body and the terminal are painted with the dark tokens; Light chosen is kept, Electron is told, the page, the body, the terminal and the window's own colour are light at once with no relaunch; System is kept and the window is the colour of whichever theme Electron then says; started again with dark kept, the window is made dark and the control says so. - `i18n.spec.js`: the control in the pseudo-locale. - `terminal.spec.js`, `logs.spec.js`: the terminal's and the panes' colours are the tokens, as before. I broke six claims one at a time and each was caught: the stored theme not applied at startup, the setting not telling Electron, the window not made dark, the terminal not re-reading its colours, the page not seeded dark, the window not painted on a setting change (the last by `ipc-wiring.test.cjs`; the journey is kept green by Electron's own `updated` for that case). `tests/unit/theme.test.cjs`, `settings.test.cjs`, `settings-view.test.cjs` and `color-scheme.test.cjs` hold the model, the control's entries and the page's declaration. **Worth a look by hand** (any platform, the current head): Settings → General → Theme. Press **Dark**: the whole window, the dialog you are in and the footer's **Give feedback** popover are dark at once; the site menu, the create-site dialog and the review dialog's diff are dark when opened. Open the Terminal tray before and after: it follows. Press **System** and change your machine's appearance: the window follows. Quit with **Dark** kept and open the app: it opens dark, with no white flash. **What must not have happened:** the light theme looking any different from before (the guide's pictures are the reference); a white window or page in the moment before a dark page paints; the terminal keeping the old colours after a switch; a server or build stopped or started by the change. **What I could not test:** Windows, where the journeys run in CI; the system's theme changing under **System** by hand, which the unit test drives through Electron's `updated` with a stubbed answer, and which I could not stage on the Mac without changing its appearance under the running suite. The pictures of the dark theme in this description were taken with `SHOTS_THEME=dark` and are not in the diff: the guide shows the light theme. ## Risks and limitations - Review: 2 passes, 6 findings fixed here, 1 follow-up left (the patch and Trac windows), every style note applied. Details below. - **The patch and Trac windows stay white** under Dark, as they were. Follow-up. - **The dark theme is the design system's ramp from one seed**, not a hand-made palette: a surface or a stroke that reads wrong in dark is the ramp's to fix, and the one token the app sets by hand is the pre-mount body colour, held to the seed by a test. - The older library's buttons keep a handful of literal colours for a disabled or a destructive state, readable in dark but not the ramp's. ## Related Closes #560. Part of #559, which is part of #542. Follows #641. --- <details> <summary>Design decisions and alternatives considered</summary> - **`nativeTheme` as the one place the choice is made.** The prototype resolves 'system' in the page with `matchMedia`. Here main resolves nothing: it gives Electron the setting, and the page reads `prefers-color-scheme`, which Electron answers from it. One chain, setting → Electron → page → tokens, and the window's chrome and the native controls come with it. - **No seed in the light scheme.** Generating the light ramp from a seed moves every colour by a hair; the tokens stylesheet already holds the light values, and the guide's pictures show them. - **The window's own colour set from the setting, not left to `updated`.** Electron promises the event for a change to light or dark, not to system; the listener stays for the system's theme changing under 'system'. - **A pre-mount body rule in index.html, scoped to before the provider mounts.** The page's first paint is before its script runs; the rule stops at the attribute the provider sets, so the ramp's surface is the body from then on. The colour is the seed, and a test holds the two together. - **Three themes, not four.** A custom pair of colours is a feature the prototype has and nobody has asked for. </details> <details> <summary>Review outcome (required — see AGENTS.md)</summary> **Pass 1: 3 [fix here] · 1 [follow-up] · 5 style notes. Pass 2 (on the fix-up): 3 [fix here] · 0 [follow-up] · 1 style note. All 6 fixed here, every style note applied, 1 follow-up left.** - **Review:** completed — a separate agent context, read-only; head `8a7457b` / base `3866e1a`; the store rule checked against the startup read and the handler, the provider's effect ordering against React's commit phases, the pre-mount rule's specificity, the deep-link path that can open the window before the ready path, the old library's rules the shell overrides, translation extraction; ESLint and the unit files run; 3 [fix here], 1 [follow-up]. - Fixed: the window's colour was set once and went stale on a theme change, an OS change and a deep link's early window; the journey read the light body before the app had repainted; the popover's ring and shadow were restated in the light theme too, and a menu-group rule matched nothing. - Follow-up left: the patch and Trac windows are not themed. - Style notes, applied: a stale comment on the store's first read; translator context for the theme's words; `SHOTS_THEME` checked; the tests naming the colours by their constants; a unit test that restated constants. - **Review:** completed — a second separate agent context, read-only; head `64cad1a` / base `3866e1a`; the `updated` listener against Electron 43's documentation, the stub's assumptions, the light theme against the library's and the tokens' values; ESLint and the unit files run; 3 [fix here]. - Fixed: nothing showed Electron emits `updated` for a change to 'system', so the window is painted from the setting itself and the journey reads the window's colour; a test's name claimed a deep link's window it did not exercise; `SHOTS_THEME=system` passed the check that exists to keep the system's theme out of the pictures. - Style note, applied: the dark popover's soft layer is a glow, and the comment says so. - **Since review:** `64cad1a → HEAD` is those three and the note, not reviewed again. On the head: `npm run lint`, `npm test`, `npm run test:electron` (2029), all 124 journeys, the packaged smoke and the docs build pass. </details> <details> <summary>Implementation notes</summary> - `src/theme.cjs`: `THEMES`, `themeColorSeeds`, `windowBackground`, the two seeds. `src/settings.cjs`: `theme`. `src/main.js`: `applyStoredTheme`, `paintWindowForTheme`, the `updated` listener, `backgroundColor` on the window, `settings:set`. - `src/renderer/components/app-theme.jsx`: `AppTheme`, `useDarkScheme`. `src/renderer/index.jsx`: mounted under it. `src/renderer/hooks/use-site-terminal.jsx`: the re-read. `src/renderer/terminal-theme.cjs`: the header. `src/renderer/shell.css`: the old library's dialogs and popovers. `src/renderer/index.html`: `color-scheme` and the pre-mount rule. - `src/renderer/settings-view.cjs`: `themeItems`. `src/renderer/components/settings-dialog.jsx`: `ThemeControl`. - `scripts/screenshots/fixtures.cjs`: `SHOTS_THEME`. `scripts/screenshots/capture.cjs`, `tests/e2e/helpers/app.cjs`: `colorScheme`. - Tests: `tests/unit/theme.test.cjs`, `color-scheme.test.cjs`, `settings.test.cjs`, `settings-view.test.cjs`, `ipc-wiring.test.cjs` (the electron stub's `nativeTheme`); `tests/e2e/journeys/settings.spec.js`. </details> <details> <summary>Screenshots or recording</summary> Not attached: I have no way to upload images from the command line. `SHOTS_THEME=dark npm run shots` takes the guide's pictures in the dark theme into `docs/public/screenshots/` for a look (and leaves them changed; restore them after). </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
Fourth part of #559, on the dialog of #638. Every site started cold: its development server and its build watch waited for a press, every time the site was opened and every time the app was. A contributor who works on one site all afternoon, and a mentor who opens the app to find the sites they left running, each had a press to make first.
What changes
Opening and quitting, on the General tab.
How a site decides. Opening a site is an edge, and so is the list a quit left: each is consumed once, when the site is ready for it, in a pure module (
src/renderer/auto-start.cjs) the row asks. The row keeps the gates and the calls: the server's start first, awaited, then the watch where that start did not bring it up. A watch the server's start was refused, because the terminal is held, is not asked for twice.How the quit remembers.
before-quitruns synchronously and is not awaited, so the write goes through the store's synchronous accessor, which the first read at startup has made; a store that cannot be written is logged and does not stop the sweep. A site with both its server and its watch running is listed under both.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:auto-start.spec.js: with the server set to start, the site the window opens on asks for its server once, and on Core the watch with it; opening another site asks for that site's; coming back to one whose server runs asks for nothing and does not stop it. With the watch set to start, the watch is asked for and no server; turned off, a relaunch starts nothing. With a list a quit left, the served site's server and the watched site's watch are asked for, whichever site the window opens on, the served site's watch once; the list is forgotten as it is read; a relaunch with nothing left starts nothing, and a list left under 'restart' is not followed under 'stop'. The quit setting is chosen and kept.settings.spec.js,i18n.spec.js: the dialog, in English and in the pseudo-locale.site-header.spec.js: the details' stickiness journey now sizes its window itself.Nothing is run: the handlers that start the server and a script are stood in, and the journeys read what they were asked. The quit's own write is
ipc-wiring.test.cjs's subject: under 'restart' it writes the sites with a server and those running their project's watch, under 'stop' nothing, and a store that cannot be written is logged and the sweep still reaches every child. I broke the open-site gate (every row started) and the quit's write, and each was caught.Worth a look by hand (any platform, the current head, a site that is set up): Settings → General, turn on Start the server when I open a site, close, open another site and come back: the server starts, and the Logs tray shows its lines. Set When I quit to Stop them, and start them again next time, leave the server running, quit, open the app: the server starts again on that site, whichever site the window opens on. Quit again with it running and set the quit back to Stop them before you open the app: nothing starts.
What must not have happened: a server started for a site whose setup or update is under way; a server stopped by the setting (it only starts); a second watch started on a site whose server's start started one; a server or watch running past the quit; the list followed twice after a relaunch that was not a quit.
What I could not test: the real quit path end to end, since the journeys stand in for the server; the wiring test drives the handler with recorded children.
Risks and limitations
build/under the just-started server for its rebuild, as both settings on do.Related
Part of #559, which is part of #542. Follows #640. The quit sweep: #83.
Design decisions and alternatives considered
before-quitis not awaited by Electron; the store is an ESM import made at startup for the locale, so it is there by the quit, and the sweep must not wait on it.Review outcome (required — see AGENTS.md)
Pass 1: 5 [fix here] · 2 [follow-up]. Pass 2 (on the fix-up): 3 [fix here] · 2 [follow-up]. All 12 applied here.
e5a0ba4/ basec34aa45; the quit path read against the store rule and the sweep, the effect's edges and gates, Playground's port choice for two servers at once, the journeys against the standard; ESLint, the unit files and the pot extraction run, Playwright not; 5 [fix here], 2 [follow-up].c75017f/ basec34aa45; the awaited server start traced through the hook's two paths for whether the watch's state is set before it resolves, the throwing-store test checked to be red on the first commit; ESLint and the unit files run; 3 [fix here], 2 [follow-up].c75017f → 5117052is those five, not reviewed again. On the head:npm run lint,npm test(2018),npm run test:electron(2019), all 123 journeys, the packaged smoke and the docs build pass.Implementation notes
src/settings.cjs:autoStartServer,autoStartWatch,quitBehavior.src/resume-sites.cjs:sitesToResume,readResume.src/settings-store.js:peekStore.src/main.js:scriptByRunId,rememberRunningSitesinbefore-quit,sites:resume.src/preload.js:takeResumeList.src/renderer/auto-start.cjs:autoStartPlan.src/renderer/settings-view.cjs:quitItems,resumeFor.src/renderer/index.jsx: the row's effect; App reads the list once.src/renderer/components/settings-dialog.jsx:OpeningAndQuitting.tests/unit/auto-start.test.cjs,resume-sites.test.cjs,settings.test.cjs,settings-view.test.cjs,ipc-wiring.test.cjs;tests/e2e/journeys/auto-start.spec.js.Screenshots or recording
Not attached: I have no way to upload images from the command line. The journeys drive everything that changed, and the guide page says what each control does.
🤖 Generated with Claude Code