Skip to content

Add dark mode with theme toggle and persisted preference - #8

Open
GodPuffin wants to merge 1 commit into
mainfrom
cursor/add-dark-mode-6931
Open

Add dark mode with theme toggle and persisted preference#8
GodPuffin wants to merge 1 commit into
mainfrom
cursor/add-dark-mode-6931

Conversation

@GodPuffin

Copy link
Copy Markdown
Owner

Introduce a header toggle that switches between light and dark themes, respects the system color-scheme preference on first visit, and saves the choice in localStorage. Dark styles are applied via CSS overrides so all existing templates are covered without per-page edits.

Introduce a header toggle that switches between light and dark themes,
respects the system color-scheme preference on first visit, and saves
the choice in localStorage. Dark styles are applied via CSS overrides
so all existing templates are covered without per-page edits.

Co-authored-by: Marcus Lee <GodPuffin@users.noreply.github.com>
@haunt-sh

haunt-sh Bot commented Jun 17, 2026

Copy link
Copy Markdown

Haunt is checking this pull request.

Check Status Details Updated (UTC)
Code review 🔴 6 findings (1 nit) Review · 2/5 Risky Aug 18, 2026 2:08am
User test 🔄 Waiting for the preview deployment Live report Aug 18, 2026 2:18am

Two major findings make this risky to merge as-is. The tailwind.config global is never read by the CDN script, so any theme/extend customization the config is meant to provide is silently dropped — this is a functional regression for dark mode and any extended tokens. The 270-line hand-rolled .dark .* overrides are an anti-pattern that fights the framework and, combined with the merged-bg-gray and duplicate text-gray findings, indicates the dark-mode pipeline is half-broken and tangled. The minor issues (missing null checks in setTheme/syncIcons, no try/catch on localStorage) are individually small but compound the sense that the theme code wasn't exercised end-to-end. Needs the tailwind config wiring fixed and the custom CSS folded back into dark: utilities (or the config honored) before merge.

Code-review findings digest
Severity Location Finding
🟠 major src/internal/assets/templates/layout.html:33 tailwind = { config: ... } assigns to an undeclared global, so tailwind.config is never read by the CDN script.
🟠 major src/internal/assets/static/style.css:1 The 270-line hand-rolled .dark .* overrides re-implement Tailwind's own dark: utility variants; the file should use dark: modifiers in templates instead. (unverified)
🟡 minor src/internal/assets/templates/layout.html:315 setTheme() calls syncIcons() with no null check on sunIcon/moonIcon either.
🟡 minor src/internal/assets/templates/layout.html:15 Inline <script> reads localStorage before any DOM/body paint but writes to <html> only; the FOUC brief flash remains benign but the script lacks a try/catch around localStorage.
🟡 minor src/internal/assets/static/style.css:28 bg-gray-50 and bg-gray-100 map to the same dark color, and so do bg-gray-200/bg-gray-300 and hover:bg-gray-200/hover:bg-gray-400 — duplication that should be merged.
⚪ nit src/internal/assets/static/style.css:94 .dark .text-gray-500 and .dark .text-gray-400 collapse to identical #9ca3af — the -400 rule is dead/duplicate.
Visual recap

🖼️ A visual recap of the change is on the code review.

e78a70f

</script>
<script src="https://cdn.tailwindcss.com"></script>
<style>
body {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟠 tailwind = { config: ... } assigns to an undeclared global, so tailwind.config is never read by the CDN script.

major · correctness

setTheme(root.classList.contains("dark") ? "light" : "dark");
});
})();
</script>

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 setTheme() calls syncIcons() with no null check on sunIcon/moonIcon either.

minor · correctness

var prefersDark = window.matchMedia("(prefers-color-scheme: dark)").matches;
if (stored === "dark" || (!stored && prefersDark)) {
document.documentElement.classList.add("dark");
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 Inline <script> reads localStorage before any DOM/body paint but writes to <html> only; the FOUC brief flash remains benign but the script lacks a try/catch around localStorage.

minor · correctness

background-color: #1f2937 !important;
}

.dark .bg-gray-50 {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 bg-gray-50 and bg-gray-100 map to the same dark color, and so do bg-gray-200/bg-gray-300 and hover:bg-gray-200/hover:bg-gray-400 — duplication that should be merged.

minor · simplification

color: #d1d5db !important;
}

.dark .text-gray-500 {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚪ .dark .text-gray-500 and .dark .text-gray-400 collapse to identical #9ca3af — the -400 rule is dead/duplicate.

nit · simplification

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