fix(desktop): persist custom backend skins across restarts

[asimons81]

What does this PR do?

Fixes #71446: custom backend/runtime skins can survive a Desktop restart even when their registry is populated only after the first paint.

Root cause

Desktop used the same strict normalizeSkin() path for both:

1. persisted startup values, which may legitimately reference a backend YAML skin or runtime SDK theme that has not registered yet, and
2. live selections, which should reject unknown names immediately.

That caused a valid persisted custom skin to be rewritten to nous during the boot window.

The original version of this PR tried to bridge that window with a known-skins cache. Review correctly identified two problems with that approach: the cache auto-seeded validity from the value being validated, and it did not cover named-profile/runtime-plugin persistence.

Fix

• Keep normalizeSkin() strict for live setTheme() calls.
• Add a separate persisted-value path that preserves any non-empty, non-retired stored skin name until its registry becomes available.
• Continue rendering unresolved persisted names through the existing Nous fallback rather than treating them as a valid live selection.
• Let the existing backend/user/SDK registry reactivity repaint automatically when that late-bound name resolves.
• Apply the same persistence contract through skinPref, so it covers the legacy global slot and named profiles without maintaining a second cache of skin names.

This removes the known-skins cache entirely and covers backend YAML skins plus runtime Desktop SDK themes with one late-binding contract.

Regression coverage

Added coverage for:

• unresolved global skin persistence across restart
• unresolved named-profile skin persistence
• late runtime SDK theme registration repainting the persisted theme
• strict rejection of an unknown live theme selection
• retired skin migration to the default
• existing mode validation remaining strict

Manual test

1. Set display.skin: trt and create ~/.hermes/skins/trt.yaml.
2. Apply trt, fully quit Desktop, then relaunch.
3. During boot the unresolved name may paint through the Nous fallback briefly.
4. Once the backend skin registers, Desktop should repaint trt automatically without requiring /skin trt again.
5. Repeat with a named profile/runtime SDK theme to verify the same late-binding behavior.

Related

Fixes #71446

Thanks to @ildunari for identifying the generic persisted-vs-live split and the named-profile/runtime-plugin gap, and to @teknium1 for catching the self-seeding validation flaw.

[ildunari]

This also affects runtime Desktop SDK theme contributions, and the current implementation still misses per-profile selections.

knownSkinNames() auto-seeds only the legacy global hermes-desktop-theme-v2 key. A runtime plugin theme selected for a named profile lives in hermes-desktop-profile-themes-v1, so on relaunch normalizeSkin() still rejects that name before the disk plugin loads and resets the profile to Nous.

A smaller generic split worked in testing: preserve any non-retired stored skin name in skinPref.resolve(), while keeping live setTheme() selections on the existing strict resolveTheme() validation path. Unknown persisted names still render through the existing deriveTheme(... ) ?? nousTheme fallback, but can recover reactively when either a backend skin or SDK plugin registers. This avoids a second cache of names and covers global, per-profile, backend, and runtime-plugin themes through one late-binding contract.

Focused evidence on current main: profile-theme.test.ts (8/8) and full Desktop typecheck pass with that split. I can provide the two-file patch if useful.

[teknium1]

Thanks for isolating the startup normalization path. The reported restart failure is present on current main: apps/desktop/src/themes/context.tsx:47-48 rejects a skin before it is registered, and gateway.ready deliberately calls ingestBackendSkin(..., { apply: false }) at apps/desktop/src/app/session/hooks/use-message-stream/gateway-event.ts:285-288.

Problems

• apps/desktop/src/themes/known-skins.ts:35 seeds the known set from the legacy stored value itself. A corrupt non-retired global value therefore passes the new normalizeSkin() known-name check instead of falling back, contrary to the stated junk-value behavior.
• The cache is populated only by backend-sync.ts, but named profiles persist via context.tsx:58-69 and SDK themes resolve from the contribution registry at user-themes.ts:159-178. The documented profile/runtime-plugin gap remains.
• No regression tests are added; profile-theme.test.ts:44-47 covers only a named-profile junk value.

Suggested changes

• Separate lenient persisted-value recovery from strict live validation for setTheme() and apply that recovery through profilePref.
• Add global, named-profile, and late SDK-registration restart coverage.

This is an automated hermes-sweeper review.

[asimons81]

Hi maintainers — this fixes #71446: custom backend skins in ~/.hermes/skins/ don't survive a desktop restart because normalizeSkin() rejects any name not yet in the live registry, and backend skins register asynchronously after the gateway connects. The fix remembers which names resolved to valid backend skins in prior sessions, so the persisted skin restores automatically after boot (no manual /skin needed). Truly unknown/junk names still fall back to the default, and the existing test is unchanged. CI is green. Could a maintainer review when you have a moment?

[CliffWade]

Hi @asimons81 — I independently hit this exact bug (backend skins resetting to nous on every desktop restart) and traced it to the same root cause you fixed: normalizeSkin rejects a persisted name during the boot window because the backend skin registry isn't populated until the gateway connects.

Your known-skins approach is solid. I'd like to contribute the missing piece: test coverage. The PR currently has no test changes, so the boot-window contract is unpinned. I wrote a test file against your implementation and verified it on your branch (51248359f5): 9/10 pass in the desktop vitest suite.

File: apps/desktop/src/themes/known-skins.test.ts (drop-in addition to this PR):

import { beforeEach, describe, expect, it } from 'vitest'

import { knownSkinNames, rememberSkinName } from './known-skins'
import { skinPref } from './context'
import { DEFAULT_SKIN_NAME } from './presets'

// The bug: a backend skin (user YAML in ~/.hermes/skins/) is registered into
// the live registry only AFTER the gateway connects. But the desktop's
// boot-time paint reads the persisted skin BEFORE that, so normalizeSkin
// would reject the name and reset every launch to the default. known-skins
// remembers which names resolved last session so they survive the boot
// window. These tests pin that contract.

const KNOWN_KEY = 'hermes-desktop-known-skin-names-v1'
const SKIN_KEY = 'hermes-desktop-theme-v2'

beforeEach(() => {
  window.localStorage.clear()
})

describe('knownSkinNames', () => {
  it('returns an empty set when nothing was ever remembered', () => {
    expect(knownSkinNames().size).toBe(0)
  })

  it('reads back names that were remembered', () => {
    rememberSkinName('trt')
    rememberSkinName('ares')
    expect(knownSkinNames().has('trt')).toBe(true)
    expect(knownSkinNames().has('ares')).toBe(true)
  })

  it('auto-seeds the currently persisted legacy skin so the first launch after the fix survives', () => {
    window.localStorage.setItem(SKIN_KEY, 'ember')
    expect(knownSkinNames().has('ember')).toBe(true)
  })

  it('does not auto-seed retired/empty names', () => {
    window.localStorage.setItem(SKIN_KEY, 'default')
    expect(knownSkinNames().size).toBe(0)
    window.localStorage.setItem(SKIN_KEY, '')
    expect(knownSkinNames().size).toBe(0)
  })

  it('ignores corrupt storage gracefully', () => {
    window.localStorage.setItem(KNOWN_KEY, '{not-json')
    expect(knownSkinNames().size).toBe(0)
  })
})

describe('rememberSkinName', () => {
  it('persists across reads', () => {
    rememberSkinName('ares')
    // Fresh read, same storage — the name must still be there.
    expect(knownSkinNames().has('ares')).toBe(true)
  })

  it('is idempotent', () => {
    rememberSkinName('trt')
    rememberSkinName('trt')
    const names = knownSkinNames()
    expect(names.size).toBe(1)
    expect(names.has('trt')).toBe(true)
  })
})

describe('skinPref boot-window persistence (the known-skins fix)', () => {
  it('keeps a remembered backend skin even before the registry resolves it', () => {
    // The user's persisted skin from last session, remembered in the known
    // set. The live registry is empty (gateway not connected yet) — but the
    // name was valid last session, so it must survive, not fall back.
    rememberSkinName('trt')
    window.localStorage.setItem(SKIN_KEY, 'trt')
    expect(skinPref.resolve('default')).toBe('trt')
  })

  it('still rejects names that were never valid', () => {
    window.localStorage.setItem(SKIN_KEY, 'nope')
    expect(skinPref.resolve('default')).toBe(DEFAULT_SKIN_NAME)
  })

  it('still rejects retired names even if remembered', () => {
    rememberSkinName('gold')
    window.localStorage.setItem(SKIN_KEY, 'gold')
    expect(skinPref.resolve('default')).toBe(DEFAULT_SKIN_NAME)
  })
})

One edge the tests surfaced — the auto-seed in knownSkinNames():

const active = storedString(SKIN_KEY)
if (active && !RETIRED_SKINS.has(active) && !names.includes(active)) {
  names.push(active)
}

This trusts any non-retired value in the legacy skin key, even if it never resolved to a real skin. Test still rejects names that were never valid fails against the current implementation: a junk value like 'nope' in hermes-desktop-theme-v2 gets auto-seeded into the known set and then survives normalization. In practice this rarely fires (the legacy key is only written by setTheme, which already validates), but if you want the auto-seed to be airtight, gate it on a name that actually resolved before — e.g. only auto-seed when the value is not already in the known set AND was seen at registration time, or drop the auto-seed entirely and rely on the first rememberSkinName call after connect (which only happens for real skins).

Happy to adjust the tests if you'd prefer a different structure, or open a PR against your branch if that's easier. Thanks for the fix — this was biting me daily.

[asimons81]

Reworked this rather than bumping it again. The self-seeding known-skins cache is gone; persisted skin recovery is now a separate late-binding path, while live setTheme() validation stays strict. The branch is rebased onto current main, and the regression suite now covers global + named-profile restart persistence, late runtime SDK registration/repaint, retired aliases, and live junk rejection. I also resolved the outdated review thread after replying with the implementation details. CI is running on the new head now. Thanks @ildunari and @teknium1 for the specific review trail that made the cleaner fix obvious.