Skip to content

Align design system and theme preference - #999

Open
yuripulga wants to merge 1 commit into
mainfrom
yuri/design-system-theme-alignment
Open

yuripulga wants to merge 1 commit into
mainfrom
yuri/design-system-theme-alignment

Conversation

@yuripulga

@yuripulga yuripulga commented Oct 26, 2026 •

Copy link
Copy Markdown
Collaborator

Aligns the app with the shared design-system tokens and adds account-scoped System, Light, and Dark preferences through the personal-settings API. The frontend uses generated CSS for System mode, explicit theme attributes for Light/Dark, and a small account-scoped browser cache only to prevent a first-query theme flash. It replaces legacy token aliases with canonical semantic names, unifies chart/display colors on the chart-series tokens, and intentionally removes the unused desat and mono palettes.

@yuripulga
yuripulga requested a review from czpython as a code owner October 26, 2026 17:15
@yuripulga
yuripulga force-pushed the yuri/design-system-theme-alignment branch from 94529ed to 647bd46 Compare October 26, 2026 17:33

@czpython czpython left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The backend half is close. The frontend half carries about 450 lines of machinery that a three-option select does not need, and the token layer leaves two names per concept.

Blockers

1. Offline retry for a theme select. personalSettingsWriteQueue.ts (plus its 93-line test), themePreferenceStorage.ts (plus its test), and preferences.tsx growing from 33 to 178 lines exist so a failed theme save gets cached, flagged "saved on this device", and retried on focus and online. Timezone, the sibling field in the same form, has none of this: a failed save keeps the draft and shows the error. The mechanism also has a bug. The catch in SettingsPages.tsx (line 407) marks the theme pending for any personal-save failure, including a 422 for a bad timezone, and then PATCHes {theme} alone on the next focus. Delete both lib files and their tests, the pending-sync copy in SettingsPanes.tsx, and the savedPersonalTheme threading. The provider keeps theme and previewTheme. Save and discard treat theme like timezone.

2. JS re-does what tokens.css already does. applyTheme resolves system to light or dark itself (preferences.tsx line 46) and always sets data-theme, then adds a matchMedia change listener. The generated :root:not([data-theme]) dark block in tokens.css is therefore never active after mount. Set data-theme only for light and dark, delete it for system, and the listener and the test's matchMedia stub go away. The whole provider addition is about this:

function applyTheme(theme: PersonalSettings['theme']) {
  const root = document.documentElement
  if (theme === 'system') delete root.dataset.theme
  else root.dataset.theme = theme
  document.querySelector('meta[name="theme-color"]')
    ?.setAttribute('content', getComputedStyle(root).getPropertyValue('--bg').trim())
}

const key = `druks:theme:${accountId}`
const [preview, setPreview] = useState<PersonalSettings['theme'] | null>(null)
const [stored] = useState(() => localStorage.getItem(key))
const saved = query.data?.theme ?? (stored === 'light' || stored === 'dark' ? stored : 'system')
const theme = preview ?? saved
useLayoutEffect(() => applyTheme(theme), [theme])
useEffect(() => { if (query.data) localStorage.setItem(key, query.data.theme) }, [key, query.data?.theme])

The one localStorage line stays only to avoid a flash of the OS theme before the settings query answers.

3. Two color helpers, one of them throws. providerChartColors raises for any id outside its map (providerChartColors.ts line 10). The registry today is anthropic and openai plus the usage route's unattributed bucket. The next registered provider crashes the usage panel at render. Meanwhile harnessColors keeps dark-only hexes that now also render on the light theme through --fam in Onboarding.tsx and SettingsPanes.tsx. Keep one helper: harnessColors rotating over --chart-series-1 to --chart-series-4. Delete providerChartColors.ts, its test, and the "unattributed bucket" UsagePanel test that only proves the map covers that id.

4. Alias layer leaves two names per token. styles.css lines 3 to 12 map ten old names onto design-system names, and the hand-written CSS then uses both sides of each pair: --text-tertiary beside --text-dim, --border-strong beside --border-loud, --danger beside --bucket-dead. One sed sweep to the design-system names, then the alias block goes. Here or as the next PR, but the aliases must not survive both.

old name new name sites
--text-mid --text-secondary 149
--text-dim, --text-faint --text-tertiary 207
--border-loud --border-strong 34
--bucket-dead, --outcome-failed --danger 90
--outcome-merged --success 36
--bucket-human, --decision-request-revision --warning 31
--outcome-abandoned --text-tertiary 9

The sweep makes one design change explicit: main had three greys for secondary text and the design system has two, so --text-faint and --text-dim become the same color.

5. One enum for the theme values. Literal["system", "light", "dark"] appears twice in user_settings/schemas.py, and the frontend repeats the union in types.ts and preferences.tsx. Add Theme to accounts/enums.py as a StrEnum beside AccountKind. The response becomes theme: Theme, the request theme: Theme | None = None, the column default Theme.SYSTEM; the column stays Mapped[str] like kind. The check constraint and the migration keep the SQL literal, as browser_sessions_status_check does. On the frontend drop ThemePreference and use PersonalSettings['theme'].

Should fix

6. BrandLockup is a one-caller component with an unused variant. mark-only has no caller; only the CSS rule and the test mention it. Inline the two spans back into Sidebar.tsx, delete the component, its test, and the .mark-only rule.

7. Duplicate tab underline. The inline style in App.tsx (line 432) repeats command-center.css line 86. Delete the inline style.

8. Migration docstring is wrong. It says Revises: 3b9e7a1f5c24; down_revision is 8c2f4e6a1d37, which is main's head, so the chain is right and only the text lies. Also use the siblings' revision: str = ... form.

9. A test of Postgres. The pytest.raises(IntegrityError) block in test_personal_settings.py exercises the check constraint. The parametrized 422 case already covers the policy. Delete the block.

Nits

10. Three ::selection rules with one value (activity.css line 85, chat.css line 47, styles.css line 1688), two in srgb and one in oklch. One global rule. The new color-mix(in srgb lines in chat.css and the --dui-primary hover should use oklch like the other 88.

11. The tokens.css header names tools/export_tokens_css.py and two SHA-256 hashes; neither the tool nor the hashed files are in this repo. One line: generated from the design system, do not edit.

12. --space-4, --space-24, --space-48, and --fs-title each serve one rule beside px literals everywhere else. Inline the px.

13. UserPreferencesProvider defaults accountId to 'anonymous'. Make it required and pass it in the five test render sites.

14. The desat and mono palettes were dead on main; nothing ever set data-palette. Deleting them is right, but the PR body should say so.

@yuripulga
yuripulga force-pushed the yuri/design-system-theme-alignment branch 2 times, most recently from c1cc12d to 96f1abb Compare October 26, 2026 19:18
@yuripulga
yuripulga requested a review from czpython October 26, 2026 20:09

@czpython czpython left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All fourteen points from the first pass landed: the retry machinery, the second color helper, the alias layer, the enum, the lockup component, the tab style, the migration text, the constraint test. The sweep is complete. No old token name survives at the head, and every var(--…) the frontend reads is defined in tokens.css or styles.css. What is left is residue of the deleted machinery, each a few lines.

Should fix

1. Dead lines in the personal save. The personal branch of the save loop in SettingsPages.tsx now ends with previewTheme(saved.theme), discard(page), continue. The loop tail already calls discard(page), and discard resets the preview to null, which resolves to the saved theme through the query data. The branch is main's two lines again.

2. dirtyPages.forEach((page) => discard(page)). discard is back to one parameter, so main's dirtyPages.forEach(discard) stands.

3. The matchMedia stub flip. SettingsPages.test.tsx changes main's stub from matches: true to false. Nothing in the theme path reads matchMedia now. The only reader left is the sidebar's (min-width: 650px) query, so false puts every settings test in the mobile sidebar layout. Revert the hunk.

4. UsagePanel residue. The harnessColor to chartColor renames, h to provider, the HoursTile prop rename, the todayResponse parameter on the test's stubFetch, and the "renders the backend unattributed usage bucket" test all came in with providerChartColors. With harnessColors rotating over the chart tokens, nothing is specific to that id anymore. Revert them. The four var(--…) swaps in Spark stay.

5. import('../api/types').PersonalSettings['theme'] three times in SettingsPanes.tsx. The file already imports types from ../api/types; add PersonalSettings to that import.

6. Trailing block in tokens.css. --space-4, --space-24, --space-48, and --fs-title sit in a second :root after the generated blocks, and nothing reads them now. Delete the block, so the file is only what the generator wrote.

Nits

7. previewTheme is setPreview; the useCallback wrapper adds nothing. useThemePreference can return ctx instead of rebuilding it.

8. theme: savedTheme in SettingsPages.tsx names the applied theme, which is the preview while one is active. Keep the name theme.

9. The .brand-lockup span is a one-caller wrapper inside .sidebar-brand. Put the class on the Link and drop the span.

@yuripulga
yuripulga force-pushed the yuri/design-system-theme-alignment branch from 96f1abb to 83ce8c3 Compare October 26, 2026 20:53
@yuripulga
yuripulga requested a review from czpython October 26, 2026 21:01

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants