Skip to content

W15-A17: settings URL subtabs + 6 theme palettes - #217

Merged
Ghenghis merged 1 commit into
developfrom
claude/w15-a17-settings-themes
May 10, 2026
Merged

Ghenghis merged 1 commit into
developfrom
claude/w15-a17-settings-themes

Conversation

@Ghenghis

@Ghenghis Ghenghis commented May 10, 2026 •

Copy link
Copy Markdown
Owner

Summary

  • 8 URL-addressable Settings subtabs (#settings/<sub>) with clean replaceState + hashchange round-trip
  • 6 named theme palettes (default, cyberpunk, matrix, tron, industrial-forge, aurora-operator) with localStorage persistence at h3d.theme.palette
  • Live preview wired through applyPalette() in theme/palettes/; ThemeProvider re-applies after every light/dark flip; default is a transparent no-op so light mode keeps white backgrounds

Subtabs verified

  • #settings/general, /providers, /agents, /mcp, /printers, /environment, /updates, /about
  • 8/8 routes covered by tests/unit/SettingsPage.test.tsx (it.each block + 5 additional behaviour tests)

Palette WCAG AA contrast (all PASS)

palette fg:bg muted:bg primary:bg
default 16.34 5.55 10.65
cyberpunk 17.55 7.18 6.41
matrix 18.28 7.86 14.96
tron 18.32 8.47 16.16
industrial-forge 15.83 7.05 9.35
aurora-operator 16.55 7.57 7.98

Threshold: body text (fg, muted) >= 4.5; UI/large (primary) >= 3.0. Verified at compile time via tests/unit/palettes.test.ts.

Self-audit

  • npx tsc --noEmit PASS (0 errors)
  • npm run build PASS (vite production build, 1.36s)
  • npx vitest run PASS — 139 tests / 4 skipped (was 118 / 4; +21 new)
  • npx playwright test --config tests/visual/palettes.playwright.config.ts PASS — 6/6 palette screenshots written to tests/visual/__snapshots__/palette-preview/

Coordination

  • store.ts change is additive (new SETTINGS_SUBTAB_KEYS, settingsSubtabFromHash, generic subtabFromHash). A18 (Voice) can add voiceSubtabFromHash next to it without conflict.
  • theme/palettes/ is a new directory; ThemeProvider edit is a single re-apply call. No file overlaps with A18 / A19 / A11-A16 lanes per hermes_list_locks.

Honest fallback

Backend /api/settings/themes (A20 lane) does not exist yet. Palette is preview-only: changes persist via localStorage and survive reload via themeBootstrap. Banner text in the Settings UI states this explicitly.

Sources cited

  1. WCAG 2.1 §1.4.3 Contrast (Minimum) — https://www.w3.org/WAI/WCAG21/Understanding/contrast-minimum.html
  2. VS Code "Customizing colors" — https://code.visualstudio.com/docs/getstarted/themes (modelled palette key shape after workbench.colorCustomizations)

Hermes lock

  • Owner: claude-w15-a17-settings
  • Task: W15-A17-SETTINGS-THEMES-2026-05-10
  • Locks will be released immediately after this PR is opened.

Test plan

  • Visit each #settings/<sub> URL on the running GUI; the matching subtab renders.
  • Click each subtab in the rail; the hash updates without bloating browser history.
  • Switch palettes from Settings > General; live preview swaps colours, refresh keeps the choice.
  • Toggle light/dark from the topbar while on a non-default palette; the named palette wins.

Co-Authored-By: Claude Opus 4.7 (1M context) noreply@anthropic.com

Summary by CodeRabbit

Release Notes

  • New Features

    • Settings page now supports URL-based navigation directly to specific configuration subtabs
    • Introduced six new customizable color palettes with live preview support
    • User color palette preferences are automatically persisted
  • Tests

    • Added comprehensive unit and visual test coverage for palette selection and settings navigation

URL-addressable subtabs (8):
  #settings/general, /providers, /agents, /mcp, /printers, /environment,
  /updates, /about — clicks rewrite via replaceState, hashchange + popstate
  re-sync the subtab so deep links and back/forward navigate cleanly.

Named theme palettes (6):
  default, cyberpunk, matrix, tron, industrial-forge, aurora-operator.
  Each palette ships all 13 --h3d-color-* vars; live-preview applies them
  immediately, Save persists to localStorage[h3d.theme.palette]. Backend
  /api/settings/themes is owned by A20 — palette is preview-only until it
  ships. ThemeProvider re-applies the named palette after every light/dark
  flip; the `default` id is a transparent no-op so light-mode users keep
  white backgrounds.

WCAG AA verified for every palette:
  fg:bg >= 15.83  muted:bg >= 5.55  primary:bg >= 6.41

Tests: 21 new unit tests (8 hash routes + 13 palette guarantees) plus
6 Playwright preview screenshots under tests/visual/__snapshots__/.

Sources:
  - WCAG 2.1 contrast: https://www.w3.org/WAI/WCAG21/Understanding/contrast-minimum.html
  - VS Code color customizations: https://code.visualstudio.com/docs/getstarted/themes

Task: W15-A17-SETTINGS-THEMES-2026-05-10

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented May 10, 2026 •

Copy link
Copy Markdown
📝 Walkthrough

Walkthrough

This PR adds a named-palette color theming system for the UI and extends Settings navigation from 4 to 8 subtabs with URL-hash–driven routing. It includes six theme palettes (default, cyberpunk, matrix, tron, industrial-forge, aurora-operator), palette persistence/application logic, a palette picker in GeneralSubtab, comprehensive WCAG contrast validation, and visual/unit tests.

Changes

Palette & Routing Unified Feature

Layer / File(s) Summary
Routing & Palette Type Contracts
03_implementation/ui/src/app/store.ts, 03_implementation/ui/src/theme/palettes/index.ts, 03_implementation/ui/src/theme/index.ts
Introduces generic subtabFromHash helper for parsing nested hash routes; defines SETTINGS_SUBTAB_KEYS and SettingsSubtabKey type; adds NamedPaletteId, NamedPalette interface, and palette registries/lookup; re-exports all palette APIs from theme index.
Palette Definitions & Infrastructure
03_implementation/ui/src/theme/palettes/default.ts, 03_implementation/ui/src/theme/palettes/cyberpunk.ts, 03_implementation/ui/src/theme/palettes/aurora-operator.ts, 03_implementation/ui/src/theme/palettes/industrial-forge.ts, 03_implementation/ui/src/theme/palettes/matrix.ts, 03_implementation/ui/src/theme/palettes/tron.ts, 03_implementation/ui/src/theme/palettes/index.ts
Defines six named theme palettes with full --h3d-color-* CSS variable tokens and metadata. Implements applyPalette to stamp palette attributes and conditionally apply CSS variables (skip overwrites for default palette), isNamedPaletteId validation, and localStorage read/write helpers (readStoredPaletteId, writeStoredPaletteId) with safe fallback to default.
Settings URL Hash Routing & Tests
03_implementation/ui/src/components/settings/SettingsPage.tsx, 03_implementation/ui/tests/unit/SettingsPage.test.tsx
Expands hardcoded 4-subtab model to data-driven 8-subtab model. Derives initial subtab from window.location.hash (fallback general), syncs bidirectionally: hash-change → subtab selection (via hashchange/popstate/500ms polling), subtab selection → hash update (via history.replaceState), dispatches synthetic hashchange for listener reaction. Tests verify default subtab fallback, initial hash selection, click-to-hash sync, external hash-change reaction, and invalid-subtab fallback using setHash helper.
GeneralSubtab Palette Picker
03_implementation/ui/src/components/settings/GeneralSubtab.tsx
Adds palette state initialized from storage; useEffect applies palette CSS variables on change for live preview; dirty-state includes palette changes; save handler persists palette via storage and emits settings.general.saved with palette in payload. UI gains radiogroup fieldset for selecting from NAMED_PALETTES with swatch previews and default-revert hint.
Theme Bootstrap & Provider Palette Layering
03_implementation/ui/src/theme/themeBootstrap.ts, 03_implementation/ui/src/theme/ThemeProvider.tsx
Updates bootstrapTheme and ThemeProvider.applyTheme to layer persisted palettes after base light/dark CSS variables via applyPalette(readStoredPaletteId()), enabling palette overrides on first paint and during theme switches.
Palette Infrastructure & WCAG Contrast Unit Tests
03_implementation/ui/tests/unit/palettes.test.ts
Validates six-palette registry completeness, all required CSS variable presence (valid hex format), WCAG AA contrast ratios (body 4.5:1, UI 3.0:1), isNamedPaletteId guarding, applyPalette stamping behavior (default-mode attribute-only vs. non-default full variable application), and localStorage persistence with fallback to default on invalid/missing data.
Palette Visual Preview Tests & Config
03_implementation/ui/tests/visual/palettes-preview.spec.ts, 03_implementation/ui/tests/visual/palettes.playwright.config.ts
Playwright spec generates PNG preview cards for all six palettes, injecting cssVars into document, rendering styled mockups with labels/swatches to __snapshots__/palette-preview/{id}.png. Config runs spec in single-worker headless Chromium with output to test-results/palettes-preview.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related PRs

  • Ghenghis/Hermes3D#202: Overlaps with settings hash-routing helpers, SettingsPage changes, ThemeProvider updates, and palette module structures.
  • Ghenghis/Hermes3D#204: Mounts ThemeProvider at app root, enabling the palette application logic added here to take effect at runtime.
  • Ghenghis/Hermes3D#35: Introduced the original 4-subtab SettingsPage model; this PR extends it to 8 subtabs with URL hash routing.

Poem

🎨 Six palettes bloom in the CSS night,
Subtabs navigate by hash-light,
From cyberpunk neon to matrix green,
Themes persist—the finest seen!
WCAG contrasts whisper true,
A rabbit paints the world anew. 🐰✨

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 28.57% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and specifically summarizes the two main changes in this PR: URL-addressable Settings subtabs routing and the addition of six theme palettes.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch claude/w15-a17-settings-themes

Comment @coderabbitai help to get the list of available commands and usage tips.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Code Review

This pull request introduces a named theme palette system and implements URL-hash-based routing for settings subtabs. Feedback focuses on the live preview mechanism in the General subtab, identifying issues where the 'default' palette fails to revert styles, previews desync during light/dark mode toggles, and previewed styles leak when navigating away. The reviewer suggested using the theme hook to coordinate base variables and adding a cleanup effect to ensure consistent behavior.

Comment on lines +36 to +45
import {
applyPalette,
DEFAULT_PALETTE_ID,
isNamedPaletteId,
NAMED_PALETTES,
PALETTE_STORAGE_KEY,
readStoredPaletteId,
writeStoredPaletteId,
type NamedPaletteId,
} from "../../theme/palettes";

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

high

To correctly handle the live preview coordination and cleanup, you'll need to import useTheme and themeCssVars from the theme module. This allows the component to re-apply base variables when switching back to the 'default' palette or when the underlying light/dark mode changes.

Suggested change
import {
applyPalette,
DEFAULT_PALETTE_ID,
isNamedPaletteId,
NAMED_PALETTES,
PALETTE_STORAGE_KEY,
readStoredPaletteId,
writeStoredPaletteId,
type NamedPaletteId,
} from "../../theme/palettes";
import {
applyPalette,
DEFAULT_PALETTE_ID,
isNamedPaletteId,
NAMED_PALETTES,
PALETTE_STORAGE_KEY,
readStoredPaletteId,
writeStoredPaletteId,
useTheme,
themeCssVars,
type NamedPaletteId,
} from "../../theme";

Comment on lines +148 to +150
useEffect(() => {
applyPalette(palette);
}, [palette]);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

high

The current live preview implementation has three issues:

  1. Broken 'Default' Preview: Since applyPalette(DEFAULT_PALETTE_ID) is a no-op for CSS variables, clicking 'Default' after a named palette won't actually revert the colors in the UI.
  2. Theme Toggle Desync: If the user toggles light/dark mode while previewing a named palette, the preview is lost because ThemeProvider re-applies the saved palette.
  3. Leaky Preview: Navigating away from the Settings tab without saving leaves the previewed palette active on the entire application until a page reload.

Using useTheme to coordinate with the base theme and adding a cleanup effect solves these issues.

  const { resolvedTheme } = useTheme();

  // Live preview: every palette change re-applies the CSS variables.
  // We coordinate with the base theme to ensure 'default' correctly reverts
  // and that the preview survives light/dark mode toggles.
  useEffect(() => {
    if (palette === DEFAULT_PALETTE_ID) {
      const vars = themeCssVars(resolvedTheme);
      const root = document.documentElement;
      for (const [name, value] of Object.entries(vars)) {
        root.style.setProperty(name, value);
      }
      root.dataset.h3dPalette = DEFAULT_PALETTE_ID;
    } else {
      applyPalette(palette);
    }
  }, [palette, resolvedTheme]);

  // Cleanup: restore the actually persisted palette when navigating away
  // to prevent the preview from 'leaking' to the rest of the app.
  useEffect(() => {
    return () => {
      applyPalette(readStoredPaletteId());
    };
  }, []);

Comment on lines +113 to +117
if (palette.id === DEFAULT_PALETTE_ID) {
// No-op for CSS vars — keep whatever ThemeProvider has set so
// light/dark mode and the default look stay consistent.
return palette.id;
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

The 'no-op' behavior for DEFAULT_PALETTE_ID is problematic when applyPalette is called for live previews (e.g., in GeneralSubtab). If a named palette was previously applied, its inline styles will persist because this branch doesn't clear or overwrite them. While this works within the applyTheme flow (which sets base variables first), it creates a bug for other callers. Consider documenting this requirement or allowing applyPalette to accept base variables for resetting.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Actionable comments posted: 3

🧹 Nitpick comments (1)
03_implementation/ui/src/components/settings/SettingsPage.tsx (1)

71-84: 💤 Low value

Dev check only verifies length, not key membership.

The mismatch check compares array lengths but won't catch if SUBTABS contains a typo or different key than SETTINGS_SUBTAB_KEYS. Consider validating key membership for stronger protection during development.

♻️ Suggested improvement
 if (__isDev && SUBTABS.length !== SETTINGS_SUBTAB_KEYS.length) {
   // eslint-disable-next-line no-console
   console.warn(
     `[SettingsPage] SUBTABS (${SUBTABS.length}) and SETTINGS_SUBTAB_KEYS (${SETTINGS_SUBTAB_KEYS.length}) length mismatch`,
   );
 }
+if (__isDev) {
+  const subtabKeys = new Set(SUBTABS.map((s) => s.key));
+  const storeKeys = new Set(SETTINGS_SUBTAB_KEYS);
+  const missing = SETTINGS_SUBTAB_KEYS.filter((k) => !subtabKeys.has(k));
+  if (missing.length > 0) {
+    // eslint-disable-next-line no-console
+    console.warn(`[SettingsPage] SUBTABS missing keys from SETTINGS_SUBTAB_KEYS: ${missing.join(", ")}`);
+  }
+}
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@03_implementation/ui/src/components/settings/SettingsPage.tsx` around lines
71 - 84, The current dev-only guard only compares lengths; change it to also
validate membership between SUBTABS and SETTINGS_SUBTAB_KEYS by computing the
set difference both ways (items in SUBTABS not in SETTINGS_SUBTAB_KEYS and vice
versa) and, if any differences exist, emit a console.warn that includes the
offending keys; keep the existing __isDev guard and the descriptive message but
augment it to list missing/extra keys to catch typos or mismatches for symbols
SUBTABS and SETTINGS_SUBTAB_KEYS in SettingsPage.tsx.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@03_implementation/ui/src/components/settings/GeneralSubtab.tsx`:
- Around line 148-150: Summary: Selecting the "default" palette currently is a
no-op because applyPalette("default") doesn't undo previously set CSS variables;
implement a proper reset path so the live preview truly reverts. Fix: update
applyPalette (and its consumer in GeneralSubtab.tsx where useEffect watches
palette) so that when palette === "default" it removes or clears any
palette-specific CSS custom properties (or reapplies the baseline/theme CSS)
instead of doing nothing; alternatively add a resetPalette/clearPalette helper
called from the useEffect when palette === "default". Reference symbols:
applyPalette, palette, GeneralSubtab.tsx useEffect (the effect block that
currently calls applyPalette(palette)).

In `@03_implementation/ui/src/theme/palettes/index.ts`:
- Around line 113-117: The early return when palette.id === DEFAULT_PALETTE_ID
leaves previously-applied inline CSS variables on the document, so update that
branch to remove/reset managed palette CSS vars before returning: when hitting
the DEFAULT_PALETTE_ID case call or implement a small helper (e.g.,
clearManagedCssVars or remove CSS vars by iterating
PALETTE_CSS_VARS/MANAGED_CSS_VARS and calling
document.documentElement.style.removeProperty(varName)) and then return
palette.id; reference DEFAULT_PALETTE_ID and palette.id to locate the
conditional and ensure all palette-specific CSS custom properties are cleared.
- Around line 70-84: The type guard is vulnerable to prototype properties
because NAMED_PALETTE_BY_ID is built as a plain object and isNamedPaletteId uses
the `in` operator; rebuild NAMED_PALETTE_BY_ID using a prototype-less object
(use Object.create(null) as the initial accumulator in the NAMED_PALETTES.reduce
that constructs NAMED_PALETTE_BY_ID) and change isNamedPaletteId to check
ownership with Object.prototype.hasOwnProperty.call(NAMED_PALETTE_BY_ID,
String(value)) (also ensure you coerce value to string before the hasOwnProperty
check) so only own keys qualify as NamedPaletteId.

---

Nitpick comments:
In `@03_implementation/ui/src/components/settings/SettingsPage.tsx`:
- Around line 71-84: The current dev-only guard only compares lengths; change it
to also validate membership between SUBTABS and SETTINGS_SUBTAB_KEYS by
computing the set difference both ways (items in SUBTABS not in
SETTINGS_SUBTAB_KEYS and vice versa) and, if any differences exist, emit a
console.warn that includes the offending keys; keep the existing __isDev guard
and the descriptive message but augment it to list missing/extra keys to catch
typos or mismatches for symbols SUBTABS and SETTINGS_SUBTAB_KEYS in
SettingsPage.tsx.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 1674c250-90f1-4776-bf0b-3b00b342b169

📥 Commits

Reviewing files that changed from the base of the PR and between 3934eb8 and dd022f8.

⛔ Files ignored due to path filters (6)
  • 03_implementation/ui/tests/visual/__snapshots__/palette-preview/aurora-operator.png is excluded by !**/*.png
  • 03_implementation/ui/tests/visual/__snapshots__/palette-preview/cyberpunk.png is excluded by !**/*.png
  • 03_implementation/ui/tests/visual/__snapshots__/palette-preview/default.png is excluded by !**/*.png
  • 03_implementation/ui/tests/visual/__snapshots__/palette-preview/industrial-forge.png is excluded by !**/*.png
  • 03_implementation/ui/tests/visual/__snapshots__/palette-preview/matrix.png is excluded by !**/*.png
  • 03_implementation/ui/tests/visual/__snapshots__/palette-preview/tron.png is excluded by !**/*.png
📒 Files selected for processing (17)
  • 03_implementation/ui/src/app/store.ts
  • 03_implementation/ui/src/components/settings/GeneralSubtab.tsx
  • 03_implementation/ui/src/components/settings/SettingsPage.tsx
  • 03_implementation/ui/src/theme/ThemeProvider.tsx
  • 03_implementation/ui/src/theme/index.ts
  • 03_implementation/ui/src/theme/palettes/aurora-operator.ts
  • 03_implementation/ui/src/theme/palettes/cyberpunk.ts
  • 03_implementation/ui/src/theme/palettes/default.ts
  • 03_implementation/ui/src/theme/palettes/index.ts
  • 03_implementation/ui/src/theme/palettes/industrial-forge.ts
  • 03_implementation/ui/src/theme/palettes/matrix.ts
  • 03_implementation/ui/src/theme/palettes/tron.ts
  • 03_implementation/ui/src/theme/themeBootstrap.ts
  • 03_implementation/ui/tests/unit/SettingsPage.test.tsx
  • 03_implementation/ui/tests/unit/palettes.test.ts
  • 03_implementation/ui/tests/visual/palettes-preview.spec.ts
  • 03_implementation/ui/tests/visual/palettes.playwright.config.ts

Comment on lines +148 to +150
useEffect(() => {
applyPalette(palette);
}, [palette]);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Default palette does not actually revert live preview.

At Line 149, applyPalette("default") is a no-op, so previously stamped non-default CSS vars remain active. That means users can’t truly preview/reset to baseline by selecting default without a reload/theme reapply.

Proposed fix
+import { themeCssVars } from "../../theme/tokens";
...
  useEffect(() => {
-    applyPalette(palette);
+    if (palette === DEFAULT_PALETTE_ID) {
+      const resolved = document.documentElement.dataset.h3dTheme === "light" ? "light" : "dark";
+      for (const [k, v] of Object.entries(themeCssVars(resolved))) {
+        document.documentElement.style.setProperty(k, v);
+      }
+      document.documentElement.dataset.h3dPalette = DEFAULT_PALETTE_ID;
+      return;
+    }
+    applyPalette(palette);
  }, [palette]);
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@03_implementation/ui/src/components/settings/GeneralSubtab.tsx` around lines
148 - 150, Summary: Selecting the "default" palette currently is a no-op because
applyPalette("default") doesn't undo previously set CSS variables; implement a
proper reset path so the live preview truly reverts. Fix: update applyPalette
(and its consumer in GeneralSubtab.tsx where useEffect watches palette) so that
when palette === "default" it removes or clears any palette-specific CSS custom
properties (or reapplies the baseline/theme CSS) instead of doing nothing;
alternatively add a resetPalette/clearPalette helper called from the useEffect
when palette === "default". Reference symbols: applyPalette, palette,
GeneralSubtab.tsx useEffect (the effect block that currently calls
applyPalette(palette)).

Comment on lines +70 to +84
export const NAMED_PALETTE_BY_ID: Record<NamedPaletteId, NamedPalette> = Object.freeze(
NAMED_PALETTES.reduce(
(acc, p) => {
acc[p.id] = p;
return acc;
},
{} as Record<NamedPaletteId, NamedPalette>,
),
);

export const DEFAULT_PALETTE_ID: NamedPaletteId = "default";

export function isNamedPaletteId(value: unknown): value is NamedPaletteId {
return typeof value === "string" && value in NAMED_PALETTE_BY_ID;
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

cat -n 03_implementation/ui/src/theme/palettes/index.ts | head -100

Repository: Ghenghis/Hermes3D

Length of output: 4396


🏁 Script executed:

cat -n 03_implementation/ui/src/theme/palettes/index.ts | sed -n '100,140p'

Repository: Ghenghis/Hermes3D

Length of output: 2131


🏁 Script executed:

rg "readStoredPaletteId|writeStoredPaletteId|applyPalette" --type ts --type tsx

Repository: Ghenghis/Hermes3D

Length of output: 89


🏁 Script executed:

rg "readStoredPaletteId|writeStoredPaletteId|applyPalette" -t ts

Repository: Ghenghis/Hermes3D

Length of output: 3507


🏁 Script executed:

cat -n 03_implementation/ui/tests/unit/palettes.test.ts

Repository: Ghenghis/Hermes3D

Length of output: 6491


🏁 Script executed:

python3 << 'EOF'
# Test the prototype-chain vulnerability
obj = {}
print(f"'__proto__' in obj: {'__proto__' in obj}")
print(f"'constructor' in obj: {'constructor' in obj}")
print(f"obj.hasOwnProperty('__proto__'): {hasattr(obj, '__proto__')}")

# Simulate what happens in JavaScript
import json
dict_obj = {}
print(f"\nIn Python dict: '__proto__' in dict_obj = {'__proto__' in dict_obj}")

# The concern is about JavaScript's prototype chain
print("\nIn JavaScript, a plain object {} has inherited properties from Object.prototype:")
print("- '__proto__' would be inherited")
print("- 'constructor' would be inherited")
print("- 'toString' would be inherited")
print("\nUsing 'in' operator checks both own and inherited properties")
print("Using hasOwnProperty checks only own properties")
EOF

Repository: Ghenghis/Hermes3D

Length of output: 495


🏁 Script executed:

cat -n 03_implementation/ui/src/theme/palettes/index.ts | sed -n '82,84p'

Repository: Ghenghis/Hermes3D

Length of output: 228


Harden palette-id validation to prevent prototype-chain properties from passing the type guard.

Line 83 uses value in NAMED_PALETTE_BY_ID, which on a plain object accepts inherited prototype properties like __proto__ and constructor. Although applyPalette has a fallback that prevents runtime crashes, the type guard itself becomes unreliable—isNamedPaletteId("__proto__") would return true despite "__proto__" not being a valid NamedPaletteId. This violates the type guard contract and could cause type-safety issues if the fallback is removed or other code relies on the guard's accuracy.

Use Object.create(null) to eliminate the prototype chain, and Object.prototype.hasOwnProperty.call() to check only own properties:

Suggested fix
-export const NAMED_PALETTE_BY_ID: Record<NamedPaletteId, NamedPalette> = Object.freeze(
+export const NAMED_PALETTE_BY_ID: Readonly<Record<NamedPaletteId, NamedPalette>> = Object.freeze(
   NAMED_PALETTES.reduce(
     (acc, p) => {
       acc[p.id] = p;
       return acc;
     },
-    {} as Record<NamedPaletteId, NamedPalette>,
+    Object.create(null) as Record<NamedPaletteId, NamedPalette>,
   ),
 );
@@
 export function isNamedPaletteId(value: unknown): value is NamedPaletteId {
-  return typeof value === "string" && value in NAMED_PALETTE_BY_ID;
+  return (
+    typeof value === "string" &&
+    Object.prototype.hasOwnProperty.call(NAMED_PALETTE_BY_ID, value)
+  );
 }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
export const NAMED_PALETTE_BY_ID: Record<NamedPaletteId, NamedPalette> = Object.freeze(
NAMED_PALETTES.reduce(
(acc, p) => {
acc[p.id] = p;
return acc;
},
{} as Record<NamedPaletteId, NamedPalette>,
),
);
export const DEFAULT_PALETTE_ID: NamedPaletteId = "default";
export function isNamedPaletteId(value: unknown): value is NamedPaletteId {
return typeof value === "string" && value in NAMED_PALETTE_BY_ID;
}
export const NAMED_PALETTE_BY_ID: Readonly<Record<NamedPaletteId, NamedPalette>> = Object.freeze(
NAMED_PALETTES.reduce(
(acc, p) => {
acc[p.id] = p;
return acc;
},
Object.create(null) as Record<NamedPaletteId, NamedPalette>,
),
);
export const DEFAULT_PALETTE_ID: NamedPaletteId = "default";
export function isNamedPaletteId(value: unknown): value is NamedPaletteId {
return (
typeof value === "string" &&
Object.prototype.hasOwnProperty.call(NAMED_PALETTE_BY_ID, value)
);
}
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@03_implementation/ui/src/theme/palettes/index.ts` around lines 70 - 84, The
type guard is vulnerable to prototype properties because NAMED_PALETTE_BY_ID is
built as a plain object and isNamedPaletteId uses the `in` operator; rebuild
NAMED_PALETTE_BY_ID using a prototype-less object (use Object.create(null) as
the initial accumulator in the NAMED_PALETTES.reduce that constructs
NAMED_PALETTE_BY_ID) and change isNamedPaletteId to check ownership with
Object.prototype.hasOwnProperty.call(NAMED_PALETTE_BY_ID, String(value)) (also
ensure you coerce value to string before the hasOwnProperty check) so only own
keys qualify as NamedPaletteId.

Comment on lines +113 to +117
if (palette.id === DEFAULT_PALETTE_ID) {
// No-op for CSS vars — keep whatever ThemeProvider has set so
// light/dark mode and the default look stay consistent.
return palette.id;
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Reset managed CSS vars when switching back to default.

On Line 113, default returns early without removing previously applied non-default inline overrides. If a user previews a palette and then picks default, the old palette vars can remain active until another base-theme write happens.

Suggested fix
   root.dataset.h3dPalette = palette.id;
   if (palette.id === DEFAULT_PALETTE_ID) {
-    // No-op for CSS vars — keep whatever ThemeProvider has set so
-    // light/dark mode and the default look stay consistent.
+    // Remove palette overrides so base ThemeProvider vars are visible again.
+    for (const name of Object.keys(defaultPalette.palette.cssVars)) {
+      root.style.removeProperty(name);
+    }
     return palette.id;
   }
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@03_implementation/ui/src/theme/palettes/index.ts` around lines 113 - 117, The
early return when palette.id === DEFAULT_PALETTE_ID leaves previously-applied
inline CSS variables on the document, so update that branch to remove/reset
managed palette CSS vars before returning: when hitting the DEFAULT_PALETTE_ID
case call or implement a small helper (e.g., clearManagedCssVars or remove CSS
vars by iterating PALETTE_CSS_VARS/MANAGED_CSS_VARS and calling
document.documentElement.style.removeProperty(varName)) and then return
palette.id; reference DEFAULT_PALETTE_ID and palette.id to locate the
conditional and ensure all palette-specific CSS custom properties are cleared.

@Ghenghis
Ghenghis merged commit 6b5e880 into develop May 10, 2026
16 checks passed
@Ghenghis
Ghenghis deleted the claude/w15-a17-settings-themes branch May 10, 2026 22:39
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.

1 participant