Refine docs dark theme contrast#42
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
![mogplex[bot]](https://avatars.githubusercontent.com/in/3107927?s=60&v=4){kind=small}
There was a problem hiding this comment.
Mogplex PR Review
Status: Attention needed
Solid, well-structured theming overhaul. The token system is clean and the dark-only intent is consistent. Three targeted issues: a WCAG contrast failure on tertiary text, a hardcoded color value that escapes the token system, and a potentially broken pseudo-element positioning dependency. Nothing is a showstopper, but the contrast failure affects all TOC items and could be a real accessibility regression.
4 findings were added inline.
| --border-default: rgba(255, 255, 255, 0.08); | ||
| --border-strong: rgba(255, 255, 255, 0.12); | ||
|
|
||
| --text-primary: rgba(255, 255, 255, 0.92); |
There was a problem hiding this comment.
Warning: --text-tertiary fails WCAG AA contrast (TOC, prose bullets)
--text-tertiary: rgba(255, 255, 255, 0.38) on --bg-canvas: #0a0a0c yields approximately 3.1:1 contrast — below the WCAG AA minimum of 4.5:1 for normal-sized text.
Affected surfaces:
#nd-toc,#nd-toc #toc-title,#nd-toc a— all non-active TOC items render at this opacity--tw-prose-countersand--tw-prose-bulletsin the prose block
For a docs site, TOC navigation and list counters are core reading aids; failing AA here is a real accessibility regression.
Suggestion: Raise --text-tertiary to at least rgba(255, 255, 255, 0.50) (~4.8:1 on #0a0a0c), or use --text-secondary (0.55, ~5.2:1) for these surfaces instead.
| nav: { | ||
| title: ( | ||
| <span className="inline-flex items-center gap-2 text-fd-foreground"> | ||
| <span className="inline-flex items-center gap-2 text-[rgba(255,255,255,0.96)]"> |
There was a problem hiding this comment.
Warning: Hardcoded RGBA in layout.shared.tsx bypasses token system
text-[rgba(255,255,255,0.96)] on the nav logo span is a one-off value that isn't represented in the design token set defined in global.css. The closest token is --text-primary at 0.92; 0.96 is neither defined nor documented as intentionally different.
This means if the token palette shifts later, this element won't follow.
Suggestion: Either add a --text-logo: rgba(255, 255, 255, 0.96) token (if the logo truly needs a distinct value) and use it via a utility, or align to --text-primary (0.92) which is imperceptible to the human eye at this luminance range.
| #nd-sidebar a[data-active='true'], | ||
| #nd-sidebar-mobile a[data-active='true'] { | ||
| background: var(--accent-soft) !important; | ||
| color: var(--text-primary) !important; |
There was a problem hiding this comment.
Suggestion: Active sidebar indicator ::after relies on undeclared position:relative
The ::after pseudo-element for the active sidebar accent bar uses position: absolute with inset-block/inset-inline-start to place a 2px left pill:
#nd-sidebar a[data-active='true']::after {
position: absolute;
inset-block: 0.625rem;
inset-inline-start: 0;
...
}For this to render correctly, the <a> ancestor must have position: relative. This currently works only if fumadocs sets that on its sidebar link elements. If a fumadocs upgrade changes that, the indicator will escape its container and visually glitch.
Suggestion: Add position: relative to #nd-sidebar a[data-active='true'] to make this self-contained and upgrade-safe.
| #nd-page h4, | ||
| #nd-page h5, | ||
| #nd-page h6 { | ||
| color: var(--text-primary); |
There was a problem hiding this comment.
Suggestion: #nd-page > p selector is likely dead CSS
#nd-page > p { color: var(--text-body); } targets only direct children <p> elements of #nd-page. In practice, prose content is always nested inside .prose containers, never as direct children of the page root. This rule has no effect in normal navigation and can be removed to keep the stylesheet clean.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f577cee93c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| :root, | ||
| .light, | ||
| .dark { |
There was a problem hiding this comment.
The theme variables are now assigned identically for :root, .light, and .dark, so selecting Light (or using System with a light OS preference) no longer changes docs styling. Because the docs sidebar still renders ThemeSwitcher with Light/System options, users can choose a mode that has no visual effect, which is a functional regression in theme behavior rather than just a style tweak.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Dismissing — the theme switcher is disabled in layout.shared.tsx (themeSwitch.enabled: false), so users cannot pick Light or System. The site is dark-only by design; the .light token block is kept for completeness but is never applied at runtime.
There was a problem hiding this comment.
Mogplex PR Review
Status: Attention needed
This is a CSS theming refactor that introduces a well-structured custom token layer on top of Fumadocs' built-in design system. The approach is sound and the PR description matches the diff. There are two correctness issues worth fixing: (1) the dark-mode token cascade has a structural flaw that causes :root dark tokens to be overwritten by the .light class assignment, and (2) WCAG contrast ratios for several body-text tokens fall below the AA threshold. One architecture warning about selector coupling to Fumadocs internals is also worth noting.
4 findings were added inline.
|
|
||
| :root, | ||
| .light, | ||
| .dark { |
There was a problem hiding this comment.
Critical: :root defaults are dark, but the fd-token block unconditionally applies to :root — light mode overrides are silently clobbered
In global.css, the custom tokens are defined as dark-defaults on :root (lines 6–28), then overridden for .light (lines 30–44). However, the Fumadocs mapping block (:root, .light, .dark { … }) re-resolves all --color-fd-* vars using var(--bg-surface), var(--text-body), etc., which are correct at evaluation time.
The real problem is ordering: because :root carries the dark defaults AND the Fumadocs mapping block also targets :root, a browser that has no .dark or .light class on <html> (e.g., during SSR or before next-themes mounts) will render dark tokens — but the .light overrides will not fire until that class is present. This is likely intentional for a dark-first site, but it means that if next-themes defaults to 'system' and the OS preference is light, there will be a flash where dark tokens are applied before the .light class is injected.
More concretely: the .light block only overrides the raw tokens (--bg-canvas, etc.), but the Fumadocs mapping selector :root, .light, .dark re-evaluates them correctly once .light is added. The cascade is technically safe post-hydration. However, any SSR-rendered HTML for light-mode visitors will have incorrect dark token values baked into the initial paint, because :root is always dark and the server cannot add a .light class without a cookie hint. Verify that next-themes is configured with defaultTheme: 'dark' or that a cookie-based theme hint is in place; otherwise light-mode visitors will see a FOIT/FART.
There was a problem hiding this comment.
Dismissing — same reason as the sibling Codex thread. Theme switcher is disabled (themeSwitch.enabled: false in layout.shared.tsx), so visitors cannot select Light or System. next-themes is not in play here; the site ships dark-only and there is no light-mode SSR path to flash through.
| --border-strong: rgba(255, 255, 255, 0.12); | ||
|
|
||
| --text-primary: rgba(255, 255, 255, 0.92); | ||
| --text-body: rgba(255, 255, 255, 0.74); |
There was a problem hiding this comment.
Warning: Several text tokens fail WCAG AA contrast on their respective backgrounds
The new token values produce contrast ratios below 4.5:1 (AA for normal text) in some combinations:
--text-body: rgba(255,255,255,0.74)on--bg-surface: hsl(0 0% 2%)(~#050505) → effective colour ≈ #C0C0C0 → contrast ~6.3:1 ✅--text-secondary: rgba(255,255,255,0.55)on--bg-sidebar: hsl(0 0% 8%)(~#141414) → effective ≈ #8F8F8F → contrast ~3.9:1 ❌ (used for all sidebar nav links, TOC title, table cell text)--text-tertiary: rgba(255,255,255,0.5)on--bg-canvas: hsl(0 0% 2%)→ effective ≈ #818181 → contrast ~3.6:1 ❌ (used for TOC links at rest)--border-subtle: rgba(255,255,255,0.05)is fine for decoration but is also used as--tw-prose-hrand--tw-prose-th-borders— low-contrast table headers may be hard to distinguish.
Sidebar navigation links (color: var(--text-secondary)) and TOC links (color: var(--text-tertiary)) are interactive controls and must meet AA. Consider bumping --text-secondary to rgba(255,255,255,0.65) and --text-tertiary to rgba(255,255,255,0.6) to clear 4.5:1 on both canvas and sidebar backgrounds.
| #nd-page h4, | ||
| #nd-page h5, | ||
| #nd-page h6 { | ||
| color: var(--text-primary); |
There was a problem hiding this comment.
Warning: Fumadocs internal ID selectors (#nd-sidebar, #nd-page, etc.) create a brittle coupling to library internals
The entire theming strategy relies on overriding styles via #nd-sidebar, #nd-sidebar-mobile, #nd-toc, #nd-page, #nd-docs-layout, and class-name fragment [class*='border-fd-foreground']. These are internal implementation details of fumadocs-ui and are not part of its public API. A minor version bump of the library could rename, restructure, or remove these IDs, silently breaking the entire theme without a build error.
Fumadocs exposes a CSS variable override mechanism (the --color-fd-* tokens, which this PR correctly populates). Where possible, prefer driving the theme exclusively through those tokens. The !important overrides on #nd-sidebar background and border-color suggest the token approach wasn't fully sufficient — it's worth filing an issue upstream or pinning the fumadocs-ui version explicitly in package.json with a comment explaining why.
| --color-fd-border: var(--border-default); | ||
| --color-fd-ring: var(--border-strong); | ||
| --color-fd-overlay: rgba(0, 0, 0, 0.62); | ||
| } |
There was a problem hiding this comment.
Suggestion: Inline rgba(0,0,0,0.62) hardcoded for --color-fd-overlay instead of using a semantic token
Line ~73: --color-fd-overlay: rgba(0, 0, 0, 0.62) is hardcoded in the :root,.light,.dark block rather than being derived from a named token like the rest of the palette. If the overlay needs to differ between light and dark (e.g., rgba(0,0,0,0.4) in light mode), there's no hook to change it. Define --bg-overlay in both :root and .light and reference it here for consistency.
- Bump --text-secondary 0.55 → 0.62 to clear WCAG AA on sidebar - Add --bg-overlay token so the Fumadocs overlay is themeable - Move Copy Markdown + View Options into the TOC footer - Expose /<slug>.md (and /index.md) via rewrite for agent curl access - Emit <link rel="alternate" type="text/markdown"> for auto-discovery
There was a problem hiding this comment.
Mogplex PR Review
Status: Attention needed
A focused dark-theme contrast refinement with a small but meaningful set of functional changes: new .md URL rewrites for agent-friendly markdown access, TOC footer relocation, and a text/markdown alternate link in metadata. The CSS overhaul is well-structured. There are two correctness issues worth addressing — one in the rewrite destination pattern and one in the CSS theme-token cascade — plus a few minor concerns.
6 findings were added inline.
| root: __dirname, | ||
| }, | ||
| async rewrites() { | ||
| // Agent-friendly: `curl https://docs/<slug>.md` returns raw markdown. |
There was a problem hiding this comment.
Critical: Rewrite destination path separator — slash vs. dot mismatch
The catch-all rewrite rule uses /:path*.md as the source, which captures path segments separated by /. For a URL like /configure-and-extend/connections-and-mcp.md, Next.js will expand :path* to configure-and-extend/connections-and-mcp — but the destination is /llms.mdx/docs/:path*/content.md, which becomes /llms.mdx/docs/configure-and-extend/connections-and-mcp/content.md. That looks correct for multi-segment paths. However, for a single-segment URL like /getting-started.md, :path* will be getting-started, which maps to /llms.mdx/docs/getting-started/content.md. That also seems fine.
The actual risk is at the boundary: the .md extension is part of the literal source pattern, not captured into :path*. So /foo/bar.md → :path* = foo/bar, destination = /llms.mdx/docs/foo/bar/content.md. Cross-check this against how getPageMarkdownUrl builds its segments array ([...page.slugs, 'content.md']) — slugs don't include the .md suffix, so the destination aligns. This appears correct on careful reading, but the comment on the rule says curl https://docs/<slug>.md returns raw markdown — confirm with an integration test that a nested page like /configure-and-extend/connections-and-mcp.md actually resolves and that the pre-rendered static file exists at the destination path. If the static route isn't pre-rendered at that exact path the rewrite will 404 silently.
|
|
||
| :root, | ||
| .light, | ||
| .dark { |
There was a problem hiding this comment.
Critical: :root defaults dark tokens — .light class won't override html/body background correctly in SSR
:root is defined with the dark token set (e.g. --bg-canvas: hsl(0 0% 2%)). The .light class redefines the same tokens, but html and body both receive background: var(--bg-canvas) unconditionally, with no selector specificity that accounts for .light vs. the root default.
The cascade for html, body { background: var(--bg-canvas) } resolves at parse time using whatever --bg-canvas is on :root. If the page renders server-side without a .light class on <html> (which is typical for SSR before hydration), the dark canvas token applies. Once JS hydrates and adds .light, the custom property on :root itself is not changed — only .light's scoped value is available when the selector matches. Because var(--bg-canvas) on html/body uses the inheriting value, this should resolve correctly IF .light is placed on <html>. But the ThemeSwitcher has its theme switch disabled (enabled: false in layout.shared.tsx), so .light class application path needs to be verified. If fumadocs-ui or next-themes never adds .light to <html>, the light theme tokens are dead code and only dark tokens ever apply.
Separately: the :root, .light, .dark block maps ALL three contexts to the same fd-* variables. This means .dark and .light map identically, which is only correct because the contextual tokens (--bg-surface, etc.) differ per class. But :root maps dark tokens → fd-* variables, so if a user has prefers-color-scheme: light but no .light class, they get a dark UI. Verify this is intentional (docs are dark-first) and add a comment to prevent future confusion.
| turbopack: { | ||
| root: __dirname, | ||
| }, | ||
| async rewrites() { |
There was a problem hiding this comment.
Warning: .md rewrite conflicts with Next.js static asset handling
Next.js serves files in the public/ directory as static assets before rewrites are evaluated. If any .md file is ever placed in public/ (e.g. a CHANGELOG.md or README.md for agent crawling), it would bypass the rewrite silently. This is low risk currently but worth a short comment on the rewrite to document the assumption that no .md files live in public/.
| segments, | ||
| url: `${docsContentRoute}/${segments.join('/')}`, | ||
| }; | ||
| // Agent-friendly URL (via next.config rewrite): /<slug>.md or /index.md for root. |
There was a problem hiding this comment.
Warning: getPageMarkdownUrl now returns agent-friendly URL as url — callers using the old internal path may break
Previously url was the canonical static pre-render path (${docsContentRoute}/${segments.join('/')}). It's now the rewrite-facing public URL (/index.md or /<slug>.md). Any callers that relied on url as the direct static-file path (e.g. sitemap generation, generateMetadata alternates, OG image logic) now get the rewritten URL instead. Check all call sites of getPageMarkdownUrl(page).url — in this PR, MarkdownCopyButton, ViewOptionsPopover, and generateMetadata alternates all consume it. Confirm MarkdownCopyButton and ViewOptionsPopover are designed to accept the public-facing URL (which seems intentional) and that the alternates.types['text/markdown'] value in metadata is also meant to be the public URL (it is, for discoverability). Just ensure no other caller in the codebase expected the old static path.
| @@ -45,6 +89,197 @@ html > body[data-scroll-locked] { | |||
| --removed-body-scroll-bar-size: 0px !important; | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
Warning: Hard-coded #nd-* fumadocs internal IDs are a fragile integration contract
Roughly 200 lines of the new CSS target internal fumadocs DOM IDs (#nd-docs-layout, #nd-page, #nd-sidebar, #nd-toc, #nd-sidebar-mobile). These IDs are not part of fumadocs-ui's documented public API and could change in a minor version bump without a breaking-change notice. Consider pinning fumadocs-ui to an exact version in package.json if not already done, or add a comment referencing the fumadocs version these IDs were validated against so future upgrades know to re-verify.
|
|
||
| #nd-toc [class*='border-fd-foreground'] { | ||
| border-color: var(--border-subtle) !important; | ||
| } |
There was a problem hiding this comment.
Suggestion: Prose link color is --text-body (not --accent) — may be intentional but reduces discoverability
Links in prose are styled color: var(--text-body) with only an underline decoration to distinguish them. This is a deliberate low-contrast aesthetic choice (per the PR title), but WCAG 2.1 SC 1.4.1 requires that links not differentiated by color alone must have a non-color visual indicator on the default state — the underline satisfies this. Just flagging for awareness if accessibility audit is planned.
1 participant
Summary
Verification
Need help on this PR? Tag
@codesmithwith what you need.