Apply and remember the ?theme= light/dark override in the docs embed#4380
Apply and remember the ?theme= light/dark override in the docs embed#4380zenoachtig wants to merge 5 commits into
?theme= light/dark override in the docs embed#4380Conversation
The docs embed ignored ?theme=light/dark: the query was dropped when the embed redirected to its default tab, and the assistant/search tabs were forced static so the theme header was never read. Forward the query on redirect, drop force-static from those dynamic-tree pages, and persist the resolved forced theme to the embed's own storage so it is remembered across tab navigation instead of only applying while the query is present. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
![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: 6881ec4134
ℹ️ 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".
|
The latest updates on your projects. Learn more about Argos notifications ↗︎
|
Persisting theme.forcedTheme wrote the site's own default into embed storage on every plain visit to a single-theme site, which could overwrite a remembered explicit theme and keep winning if the site later became multi-theme. Gate persistence on an actual URL ?theme= override and a toggleable site. (PR #4380 review) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
🦋 Changeset detectedLatest commit: 6c2638e The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
| export const dynamic = 'force-static'; | ||
|
|
There was a problem hiding this comment.
are we sure about that ? seems dangerous to change this
There was a problem hiding this comment.
Good instinct to double-check — it's safe, and it's load-bearing for this fix:
- This is only the dynamic-tree copy. The middleware routes here only for genuinely dynamic requests (
?theme=, a customization override, adaptive/VA/contextId, or thegitbook-dynamic-routecookie). The normal cacheable case is still served by the static tree, which keepsforce-static(untouched) — so nothing that was cached before stops being cached. - Under
force-static,headers()is empty, so the layout'sgetThemeFromMiddleware()(and thex-gitbook-customizationoverride) always resolved to null — the page silently dropped?theme=and customization overrides. That's the RND-11571 bug; removing it lets those header-driven requests render correctly. - Those requests are effectively uncacheable anyway (
contextIdresponses already carrymax-age=0, must-revalidate), soforce-staticgave no caching benefit here — only wrong output. - Precedent: the docs tab (
page/[pagePath]) in this same dynamic tree already has noforce-static; this just makesassistant/searchconsistent with it.
If you'd still rather keep force-static, the only alternative is applying the theme purely client-side, which reintroduces a first-paint flash and still wouldn't fix the dropped customization override — so I'd recommend against it. Happy to talk it through.
🤖 Addressed by Claude Code
There was a problem hiding this comment.
Zeno the human here, I'm also a bit concerned so I'll leave it out of this PR and seeing if the first-paint is a big problem or not. If we get reports on it we can reconsider, but I'd rather not touch it.
There was a problem hiding this comment.
hello Zeno the human 👋🏼, I'm checking on my side the path claude is mentioning
There was a problem hiding this comment.
@zenoachtig looks like it's safe, maybe @nicolas you can confirm ?
There was a problem hiding this comment.
Reworked per @conico974's suggestion — the routes stay force-static. Instead of making them dynamic, the middleware now threads the forced theme into the embed route context (a new embedTheme on the encoded site data), scoped to the embed route only (the main site keeps its existing dynamic header path). The embed layouts read it from the route params — which works under static rendering — and the redirect forwards it to the default tab. The theme becomes part of the route's static cache key, so each theme is its own cached entry. force-static is restored on assistant/search. Pushed in 6cc98ad.
🤖 Addressed by Claude Code
conico974
left a comment
conico974
left a comment
There was a problem hiding this comment.
This is not how we should do it. Those route should never be dynamic
…atic Following review: the embed routes must stay statically rendered, so reading the theme from request headers (which are empty under force-static) doesn't work. Instead the middleware threads the forced theme into the embed route context (a new `embedTheme` on the encoded site data), scoped to the embed and not the main site. The embed layouts read it from the route params, the redirect forwards it to the default tab, and force-static is restored on the assistant/search tabs. The theme becomes part of the route's static cache key. The main site keeps its existing dynamic header-based theme handling. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
3 participants
Overview
?theme=light/?theme=dark(RND-11571)./assistantor/page/.export const dynamic = 'force-static'from the dynamic-treeassistantandsearchpages, so the layout can actually read the forced theme (and customization overrides) from request headers instead of rendering statically with emptyheaders().gitbook-theme-embed:<siteId>), so it's remembered across tab navigation and reloads instead of only applying while the query is present. An explicit?theme=on a later load still overrides the remembered one; single-theme sites (which intentionally ignore the override) don't persist it.color-schemeis intentionally not included — the embed's dark styling is class-driven (Tailwinddark:,.dark-gated rules) and CSS has no selector for an element's used color-scheme, so it can't be done CSS-natively without reworking the design-system dark variant.?theme=is the supported control.Demo
Verifone-style embed on a dark-mode device — before vs after
?theme=light:Theme remembered across tab navigation (no
?theme=on the second tab):— Authored by Claude