feat: implement explicit dark theme foundations - #39
Conversation
feuersteiner
left a comment
There was a problem hiding this comment.
Automated review round 1 — gpt-5.6-sol (high) — head 4c50e4c
Findings:
- [P2] Preserve a visible focus indicator in forced-colors mode.
- [P3] Remove the escaped Markdown backticks around
<Theme />.
|
Automated review round tracker
|
|
Review round 1 addressed — gpt-5.6-terra (high)
|
feuersteiner
left a comment
There was a problem hiding this comment.
Automated review round 2 — gpt-5.6-sol (high) — head b8aa8d4
Finding:
- [P2] Document only the spacing tokens that actually exist.
Checks: bun run check, bun run build, changed-file Prettier, compiled theme/forced-colors CSS, semantic-token usage, and contrast values passed. bun run lint reaches the documented pre-existing Prettier failure in src/lib/base/toggle/toggle.svelte.
|
Automated review round tracker
|
|
Review round 2 addressed — gpt-5.6-terra (high) Fixed items:
Tests:
Declined items: none. |
feuersteiner
left a comment
There was a problem hiding this comment.
Automated review round 3 — gpt-5.6-sol (high) — head 385ce69
Findings:
- [P2] Remove the now-unused colord production dependency from package.json and bun.lock. This PR deletes its only imports, while parent issue #1 requires Svelte to be the sole runtime dependency.
- [P2] Complete the published token contract. src/routes/theming/snippets.ts and the theming page omit the primary/secondary/status aliases and their soft/contrast variants that Theme defines and public components consume.
- [P2] Finish the “components consume semantic tokens rather than raw colors” acceptance criterion. Button and IconButton still mix states with raw black, and Slider still embeds raw rgba shadow colors instead of theme tokens.
Checks: bun run check; bun run build (including package and publint); changed-file Prettier; compiled theme/forced-colors CSS and package exports inspected. bun run lint reaches only the documented pre-existing Prettier failure in src/lib/base/toggle/toggle.svelte.
|
Automated review round tracker
|
|
Review round 3 addressed — gpt-5.6-terra (high) Fixed items:
Tests:
Declined items: none. Round-3 feedback was supplied in the review body; GraphQL reports no unresolved inline review threads to reply to or resolve. |
feuersteiner
left a comment
There was a problem hiding this comment.
Automated review round 4 — gpt-5.6-sol (high) — head 940e7f4
Findings:
- [P2] Preserve the documentation shell's centering after removing global body alignment.
- [P2] Complete the published static token contract; several defined and component-consumed tokens remain undocumented.
- [P2] Raise the danger-on-danger-soft contrast from 4.40:1 to at least 4.5:1 for normal button text.
Checks: exact head reverified; bun install --frozen-lockfile, bun run check, bun run build (including package and publint), changed-file Prettier, and targeted ESLint for materially changed implementation files passed. bun run lint reaches the documented pre-existing Prettier failure in src/lib/base/toggle/toggle.svelte; broader targeted ESLint also reports pre-existing unchanged-line issues in src/routes/+page.svelte and src/routes/types/+page.svelte. Dependency/lock cleanup, runtime-theme removal, semantic color replacement, compiled theme/forced-colors CSS, raw-color/security searches, package exports, and token contrast values were inspected.
|
Automated review round tracker
|
|
Review round 4 addressed — gpt-5.6-terra (high) Fixed items:
Tests:
Declined items: none. Replied to and resolved all three round-4 inline threads. |
feuersteiner
left a comment
There was a problem hiding this comment.
Automated review round 5 — gpt-5.6-sol (high) — head b449d1f
Finding:
- [P2] Preserve normal-text contrast in Button and IconButton hover/active states. The resting danger-soft pair now passes at 4.77:1, but the interaction mixes reduce several states below 4.5:1; danger measures about 3.82:1 on soft hover, 3.01:1 on soft active, and 3.82:1 on filled active. The same pattern affects all semantic soft hover/active states and most filled active states. Adjust the tokens or state treatment in both button implementations and validate every semantic color/state.
Checks: exact head reverified; bun install --frozen-lockfile, bun run check, bun run build (including package and publint), changed-file Prettier, and targeted ESLint passed. bun run lint reaches only the documented pre-existing formatting issue in src/lib/base/toggle/toggle.svelte. Browser verification confirmed the 960px shell is centered in a 1280px viewport, dark computed styles and forced-colors focus CSS are present, and exactly 66 computed theme tokens are installed. Both documentation surfaces exactly match all 66 defined tokens; component theme-token consumers resolve; dependency/runtime-theme/raw-color/security searches and all resting semantic contrast pairs were inspected.
|
Automated review round tracker
|
|
Review round 5 addressed — gpt-5.6-terra (high) Fixed items:
Tests:
Declined items: none. Replied to and resolved the round-5 review thread. |
feuersteiner
left a comment
There was a problem hiding this comment.
Automated review round 6 — gpt-5.6-sol (high) — head 5bda3b1
Finding:
- [P2] Cover the public
foreground/backgroundaliases in the interaction-state treatment. The documented 108-state validation covers only six palette colors, while the publicColortype and component demos expose eight.backgroundghost Button/IconButton instances render#181818content on the transparent#181818page at rest and reach only approximately 1.24:1/1.61:1 on hover/active;foregroundsoft also changes only about#111111→#121212→#131313.
Checks: exact head reverified; bun install --frozen-lockfile, bun run check, bun run build, bun run prepack, publint, changed-file Prettier, and targeted ESLint passed. Both component demos, all eight color aliases across filled/soft/ghost rest-state rendering, compiled interaction CSS, all 144 color × variant × state × component contrast calculations, 66-token definitions/documentation, token consumers, layout/focus CSS, dependencies, runtime-theme removal, raw component colors, and security-sensitive patterns were inspected. bun run lint reaches only the documented pre-existing Prettier failure in src/lib/base/toggle/toggle.svelte.
|
Automated review round tracker
|
|
Review round 6 addressed — gpt-5.6-terra (high) Fixed items:
Tests:
Declined items: none. Replied to and resolved the round-6 inline thread. |
feuersteiner
left a comment
There was a problem hiding this comment.
Automated review round 7 — gpt-5.6-sol (high) — head f021bf7
Finding:
- [P2] Make
validate:button-colorsexercise the actual Button and IconButton source of truth. The current script models the expected mapping and state formulas independently, so real component regressions can still pass.
Checks: exact head reverified; bun install --frozen-lockfile, bun run validate:button-colors, bun run check, bun run build (including package and publint), changed-file Prettier, targeted ESLint, token-contract equality, runtime-theme/security searches, and rendered Button/IconButton demos passed. All 48 rendered rest combinations resolve correctly (minimum measured contrast 4.77:1); compiled interaction CSS and the 144 modeled state calculations were inspected. bun run lint reaches only the documented pre-existing formatting failure in src/lib/base/toggle/toggle.svelte.
|
Automated review round tracker
|
|
Review round 7 addressed — gpt-5.6-terra (high)
|
feuersteiner
left a comment
There was a problem hiding this comment.
Automated review round 8 — gpt-5.6-sol (high) — head bb96652
Findings:
- [P2] Use browser-accurate selector/state/cascade evaluation; the current hand parser accepts disabled-only hover rules and ignores specificity, so runtime interaction/contrast regressions can pass.
- [P2] Assert semantic alias identity; importing the mapping as truth lets
dangermap toprimary(orprimarytosecondary) while the validator still passes all 144 cells.
Checks: exact head reverified; frozen install, validate:button-colors --mutation-proof, check, build, prepack, publint, changed-file Prettier, targeted ESLint, 66-token contract equality, runtime rest-state contrast for all 48 Button/IconButton combinations, focus/forced-colors and font-scaling inspection, dependency/runtime-theme/raw-color/security searches, and package output passed. Four independent adversarial mutations all returned status 0. bun run lint reaches only the documented pre-existing formatting failure in src/lib/base/toggle/toggle.svelte.
|
Automated review round tracker
|
feuersteiner
left a comment
There was a problem hiding this comment.
Automated review round 9 — gpt-5.6-sol (high) — head d70b271
Findings:
- [P2] Exercise actual Svelte Button and IconButton instances in the Chromium matrix; the current synthetic fixtures bypass component markup, prop wiring, inline token installation, and nested icons.
- [P2] Validate effective semantic alias declarations; the current first-match parser accepts a correct declaration followed by a later remapping that wins in inline-style order.
Checks: exact head reverified; frozen install; Chromium validator and built-in mutation proof; four exact round-8 mutations independently injected and rejected with browser/identity evidence; check; build; prepack; publint; changed-file Prettier; targeted ESLint; repeated baseline validation; 66-token definition/documentation equality; actual built Button/IconButton instances and nested icon colors; dark scheme, focus, and forced-colors behavior; raw-color, runtime-theme, dependency, package-output, portability, and security inspection. Independent component-binding and duplicate-alias mutations both still returned status 0.
|
Automated review round tracker
|
feuersteiner
left a comment
There was a problem hiding this comment.
Automated review round 10 — gpt-5.6-sol (high) — head 56f6ccd
Findings:
- [P2] Validate the rendered nested Icon foreground, not only the button element’s computed
color.renderedStatereadsgetComputedStyle(element).color, but IconButton’s only visible content is a child Icon whose color classes override inheritance. Independently changing IconButton tocontrastMode={false}makes a filled primary icon render primary-on-primary (about 1:1), yet the command still exits 0 with all 288 cells valid and reports 4.71:1 minimum contrast. Inspect each rendered icon (and its effective background) so icon prop/class regressions fail. - [P2] Compare semantic identity against the effective computed cascade. The oracle reads custom properties from
element.style, so stylesheet declarations that win over inline styles are invisible to it. An independently injected.dangerrule remapping all three aliases to primary with!importantchanged Chromium’s effective tokens but still exited 0 with all 288 cells valid. Read the three properties fromgetComputedStyle(element)(while retaining the independent alias oracle) so higher-priority stylesheet remaps are rejected.
Checks: exact head reverified; frozen install; actual 288-cell Chromium baseline; built-in mutation proof; both exact round-9 mutations independently injected and rejected; check; build; prepack; publint; changed-file Prettier; targeted ESLint; exact 66-token definition/snippet/page equality; focus/forced-colors, scalable sizing, runtime-theme removal, semantic colors, package output, diff integrity, and security-sensitive additions inspected. Full lint reaches only the documented pre-existing Prettier failure in src/lib/base/toggle/toggle.svelte. bun audit reports the same 55 existing toolchain advisories on base and head; Playwright is not named.
|
Automated review round tracker
|
feuersteiner
left a comment
There was a problem hiding this comment.
Review round 10 addressed — gpt-5.6-terra (high)
feuersteiner
left a comment
There was a problem hiding this comment.
Automated review round 11 — gpt-5.6-sol (high) — head 51a1742
No findings.
Checks: exact head reverified; frozen install; built-in Chromium mutation proof (288 actual Svelte Button/IconButton cells, eight aliases, three variants, rest/hover/active plus disabled, minimum 4.71:1 contrast and 0.015 interaction delta); four independently isolated exact/adjacent Icon/Glyph and computed-cascade/!important mutations rejected; check; build; package/publint; changed-file Prettier; targeted ESLint; exact 66-token definition/documentation equality; dark theme, centered shell, focus/forced-colors, and 200% font-scaling runtime inspection; diff/security/package review. Full lint reaches only the pre-existing Prettier failure in src/lib/base/toggle/toggle.svelte. Base and head each report the same 55 existing toolchain advisories; Playwright is not named. GitHub does not permit approving a self-authored PR, so this is a signed no-findings COMMENT.
|
Automated review round tracker
|
Summary
Closes #4
Validation
bun run checkbun run buildVerification note
bun run lintstill fails on the pre-existing formatting issue insrc/lib/base/toggle/toggle.svelte; this PR does not modify that file.