Skip to content

perf(settings): apply and mirror the theme only when a theme input changes (#784) - #1400

Merged
Juliusolsson05 merged 2 commits into
mainfrom
fix/theme-sync-projection
Sep 27, 2026
Merged

Juliusolsson05 merged 2 commits into
mainfrom
fix/theme-sync-projection

Conversation

@Juliusolsson05

@Juliusolsson05 Juliusolsson05 commented Sep 27, 2026 •

Copy link
Copy Markdown
Owner

Fixes #784.

Plan: docs/plans/2026-09-27-theme-sync-projection.md.

Problem

useThemeSync selected the whole settings object, and the settings slice re-applied the theme inside chrome-only toggles. So an unrelated preference or a dispatch colour flag rewrote the inline palette, fonts and radii, fired THEME_CHANGED_EVENT (which every xterm and Monaco re-reads), and sent the full settings object to main for the phone.

Measured on origin/main (the new test's counts): each of these costs 2 theme applications:

  • a usage-header toggle;
  • the status-mode toggle;
  • the worktree-badge toggle;
  • the usage-level cycle.

A dispatch flag costs 1. Every mirror carries 40 keys.

What merges

  • THEME_SETTING_KEYS in theme.ts: exactly the settings applyTheme / resolveThemePayload read (mode, contrast, accent, fontFamily, cornerStyle, savedThemes, customAppearanceJson). The WHY comment says a new field applyTheme reads must be added here.
  • useThemeSync selects only those keys (useShallow) plus the installed extensions. It applies and mirrors only when one of them changes, and the mirror now carries only the theme keys. The phone merges the snapshot into its defaults and runs applyTheme, nothing else (applyRemoteThemeSettings).
  • The slice:
    • setSettings still applies synchronously, so the change is visible in the same frame, but only when the patch touches a theme key;
    • the status-mode, worktree-badge and usage-header toggles, and the usage-level cycle, no longer apply the theme.

Preserved:

  • the pre-hydration default;
  • hydration;
  • reset;
  • saved-theme edits;
  • extension installs.

Tests

useThemeSync.renderer.test.tsx counts theme applications (THEME_CHANGED_EVENT) and mirror calls per change:

  • five unrelated changes → 0 and 0;
  • an accent change → applied, and mirrored once with theme keys only;
  • a saved-theme edit → applied and mirrored;
  • an extension change → mirrored.

After review a:

  • every key in THEME_SETTING_KEYS applies and mirrors its new value;
  • the exact key list is pinned;
  • an active extension theme mirrors as mode: 'custom' with its resolved palette;
  • a hydration that bypasses setSettings still paints;
  • a picker change paints before effects flush.

Fail-first: 6 of the original 8 fail on origin/main.

Mutations, each killed:

  • setSettings always applying;
  • dropping mode, fontFamily or customAppearanceJson from the list;
  • mirroring without the extension conversion;
  • no hook apply;
  • no synchronous apply.

Gates: npx tsc -b + src/renderer/src/features/settings + src/renderer/src/app-state/{settings,uiShell} + src/remote-client on 3831fc31 (36 files, 244 tests).

🤖 Generated with Claude Code

Juliusolsson05 and others added 2 commits September 27, 2026 03:27
…anges (#784)

useThemeSync selected the whole settings object and the slice re-applied
inside chrome-only toggles, so unrelated preferences rewrote the palette,
fired THEME_CHANGED_EVENT for every xterm/Monaco, and sent the full settings
over IPC. Measured on main: 2 applications per toggle, 1 per dispatch flag.
Now 0; theme edits still apply immediately and mirror once, with only the
theme keys.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…, hydration and same-frame paint (#1400 review a)

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@Juliusolsson05

Copy link
Copy Markdown
Owner Author

Review disposition (head 3831fc31). a: FIX-BEFORE-MERGE, then MERGE-READY in round 2. b: MERGE-READY.

Fixed (a, round 1: all three were test gaps; the code was already correct)

  • Theme-key completeness: dropping mode, fontFamily or customAppearanceJson from THEME_SETTING_KEYS left the suite green. Now every key is applied and mirrored with its new value, and the exact list is pinned.
  • The phone's extension-theme conversion: it was unprotected. An active extension theme now has to mirror as mode: 'custom' with its resolved palette.
  • Hydration and same-frame paint: both could break unnoticed. A hydration that bypasses setSettings must paint, and a picker change must be on the DOM before effects flush.

All six of a's surviving mutations are killed.

Declined (b, minor)

  • Reconnect replay (RemoteServer.ts hello.themeSettings) has no direct assertion. That code is unchanged by this PR: it replays whatever the desktop last mirrored, which is now the theme keys. b verified the replay is correct today. A test for it belongs with the remote transport suite, not this settings change.

🤖 Generated with Claude Code

@Juliusolsson05
Juliusolsson05 marked this pull request as ready for review September 27, 2026 11:18
@Juliusolsson05 Juliusolsson05 added the type:perf Performance or resource use label Sep 27, 2026
@Juliusolsson05

Copy link
Copy Markdown
Owner Author

Disposition addendum: reviewer c MERGE-READY. c traced six journeys end to end (theme change, saved-theme edit, extension theme, unrelated toggle, hydration, phone reconnect) with no regressions.

Declined (c, minor): the slice's toggleUsageHeader / toggleStatusMode / toggleWorktreeBadges / cycleUsageHeaderLevel have no production caller (Settings rows use setSettings, and the palette commands were retired). Removing their applyTheme therefore cannot regress anything, and the 'usage header' test row covers the live setSettings path, not those actions. Deleting the dead actions is out of scope for this perf change.

🤖 Generated with Claude Code

@Juliusolsson05
Juliusolsson05 merged commit ef9a122 into main Sep 27, 2026
2 checks passed
Juliusolsson05 added a commit that referenced this pull request Sep 27, 2026
@Juliusolsson05
Juliusolsson05 deleted the fix/theme-sync-projection branch September 27, 2026 12:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

type:perf Performance or resource use

Projects

None yet

Development

Successfully merging this pull request may close these issues.

perf(settings): avoid theme DOM updates and remote sync for unrelated settings

1 participant