docs: correct the FOUC mechanism in the critical-CSS and guard comments - #175
Merged
Conversation
The CRITICAL_CSS comment in app/root.tsx and the fouc.spec.ts header both said WebKit paints the freshly-navigated document for ~1 frame before the head stylesheets apply. The v2.5.0 review found otherwise, and probing confirms it: WebKit holds first paint until the render-blocking stylesheets apply. A rAF-from-document-start sampler against the static build never caught an unstyled frame — cold or warm cache, across every fixture page, even with all CSS responses delayed by 800ms. The flash the critical CSS guards against comes ~200ms AFTER first paint instead, when React hydration fails (#418/#423) and the recovery client render re-patches whatever diverged. A head-level divergence re-creates head nodes (#126 measured a missing <style> re-inserted at 195ms); a re-inserted <link> re-applies asynchronously while an inline <style> applies the instant it lands, so the critical CSS is the only styling in that gap. A body-level mismatch recovers without touching the head at all (probed by injecting a stray node into the served body: both errors fire, zero head mutations, no flash). Also measured, worth recording: current builds hydrate cleanly — no #418/#423 on any fixture page, dev server or static build — so the failure #126 reported no longer reproduces, and the probe that says so detects the errors when a mismatch is forced. Comments only; no behaviour change. tsc clean, webkit-fouc suite 2 passed.
Contributor
|
Contributor
🎭 Visual regression results
Details
Skipped testsmobile-chrome › theme.spec.ts › QuantEcon theme — visual regression › without JavaScript › drawer-opens-without-javascript |
There was a problem hiding this comment.
🟢 Approval recommended
It is a comments-only documentation change with no functional impact, and the rewritten text is accurate, internally consistent, and consistent with the surrounding code and referenced issues.
Pull request overview
This PR corrects the explanatory comments that describe the FOUC (Flash Of Unstyled Content) mechanism in the critical-CSS block. Previously, both the CRITICAL_CSS comment in app/root.tsx and the fouc.spec.ts header attributed the flash to WebKit painting a freshly-navigated document for ~1 frame before the head stylesheets apply. Fresh measurements against v2.5.0 established that WebKit actually holds first paint until the render-blocking head stylesheets apply, and the unstyled flash instead arrives ~200ms later during React's hydration-recovery re-render (#418/#423), when a re-created head node can briefly drop the stylesheet. The comments are rewritten to document this verified model and the practical rule it implies (anything that must not flash needs an inline critical-CSS rule, since React is already mounted at flash time).
Changes:
- Rewrites the
CRITICAL_CSSmechanism comment inapp/root.tsxto describe the post-hydration re-render as the true flash window rather than a pre-stylesheet first paint. - Rewrites the
fouc.spec.tsheader to match the corrected mechanism and re-justify thewebkit-fouc-only scoping by where the flash was actually observed. - No behavioral/code changes — comments only.
File summaries
| File | Description |
|---|---|
app/root.tsx |
Corrects the CRITICAL_CSS block comment to explain the hydration-recovery flash mechanism and the inline-CSS-only mitigation rule. |
tests/visual/fouc.spec.ts |
Updates the test's header comment to reflect the corrected mechanism and re-scope the WebKit-only justification. |
The rewritten comments are internally consistent, consistent with the pre-existing back-to-top explanation already present in app/root.tsx (lines 127–136), and consistent with the referenced issues (#126, React #418/#423, quantecon-theme-src#66). No spelling, grammar, factual, or cross-reference issues were found, and the JSDoc blocks remain well-formed.
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 0
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
This was referenced Sep 9, 2026
Open
This was referenced Sep 10, 2026
quantecon-services
pushed a commit
that referenced
this pull request
Sep 11, 2026
Reverts the CHANGELOG half of b8b8101, keeping the app/seo.ts og:type comment it also carried. I added the entry on a Copilot review flag citing CONTRIBUTING's "add your change to `## [Unreleased]`" rule, without checking whether this PR had already taken a position. It had: the description says no entry, because like #175 -- which corrected the FOUC comments and touched no CHANGELOG -- this changes nothing a consumer of the theme can see. Verified that 1aae2c8 (#175) indeed has no CHANGELOG entry, so the cited precedent holds. The author's call stands. It was also the sole cause of the merge conflict against main: #199 released v2.7.0, moving the `## [Unreleased]` entries this added to into `## [2.7.0]`. With the entry gone the PR no longer touches CHANGELOG.md at all. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QAy8brFG9x8fXet3AV9dJE
mmcky
added a commit
that referenced
this pull request
Sep 11, 2026
* chore(comments): code comments explain the code, not its history Comments across app/, styles/, tests/, the workflows and config named project phases, tracker issues and PLAN.md items, and justified values by pointing at the Sphinx build. Rewrite them to state the reason itself: the measurement, the contrast ratio, the constraint. A link stays only where its thread holds detail a comment cannot give in a line (a fork feature the code reads, a guarded regression, measured timings), and ported code keeps one provenance line. Add a "Code comments" section to CONTRIBUTING.md with the rule. Comment-only: every changed code file is identical once comments are stripped (TypeScript printer for TS/JS, postcss for CSS, parsed YAML and JSON). The one data change drops an issue tag from the stderr output text in the no-thebe fixture notebook. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * chore(tests): name test suites by behaviour, not by issue number Drop the issue numbers from four describe titles, the Sphinx framing from an seo unit-test title, and the tracker link from the FOUC guard's CI step name. Screenshot baselines are named explicitly in each toHaveScreenshot call, so no snapshot moves. Also restate the tailwind.config.js grid-track comment and a FOUC-suite note in the present tense. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * chore(comments): correct comment facts found in an accuracy review An adversarial review of the rewritten comments (reviewers over every changed file, a whole-codebase sweep for missed framing, and a three-lens panel per finding) confirmed factual errors and leftover history framing. Each was re-checked before applying: - contrast: the stderr label is 5.5:1 and the link 5.08:1, six light code colours miss AA, and dark-mode ratios are restated for the #222 content panel the text actually sits on - sizes: the UNITS root under the toolbar font-size control, and figure and paragraph spacing quoted in px for what is em - comments contradicting the code: the seo and route meta tags, the first-paint font, the dark-bg keep-in-sync pointer, the static-build claims in playwright.config.ts and preview.yml, the relative-css directory note, and the template.yml README pointer - history framing the first pass missed: Makefile, .npmrc, serve.sh, mpl-widget.css, Document.tsx, the launch-urls tests, PageHeaderHistory.tsx and a release.yml step comment - CONTRIBUTING: git log -L rather than git blame, and measurements as a reason to keep a link Still comment-only: every changed file is identical to main once comments are stripped from both sides. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * chore(comments): say why Colab is the only launch target Restore the design decision the rewrite dropped from the launch-colab test comment: Binder and JupyterHub are deliberately not offered. And reword the code-palette note so it no longer explains the missing toggle by an option this theme never had. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * chore(comments): keep the reason when the reason is another system Follow-up to the code-comments rewrite, from an accuracy review of #200. The rewrite read "matches the Sphinx build" as project framing and removed it everywhere: Sphinx mentions in source fell from 95 across 15 files to 1. For this theme that phrase is the specification rather than history -- the theme reproduces the deployed lecture sites -- so where it was the only justification for a value, the replacement restated the declaration instead of giving a reason. CONTRIBUTING.md: state the section's scope (code comments, including YAML, shell and fixture comments; prose docs keep their history), fold the overlapping first two bullets into one ban plus one exception, and say that a constraint from another system is a reason, with a worked example and a note to record deliberate divergences. styles/quantecon.css: restore the derivation for the four values whose only reason was removed -- the footer's 0.9rem and 0.7 opacity, the 5px rule's 18px gap, and the back-to-top 15.3px, whose comment had come to contradict itself by deriving a deliberately unpinned size from `.article`. Restore the two deleted notes: the known divergences on link underlines and `:visited`, and that self-hosting Source Code Pro is separate work. Add a header note so a reader knows what the measurements are measurements of. List all six AA-failing code colours rather than counting them, and record that the palette is inherited from quantecon-book-theme, so changing it is a decision for both themes (see #172). static.spec.ts: the provider `Link` would resolve against the un-slashed SSR pathname; the code does not use it, so the present tense contradicted the same explanation in Outline.tsx. Fixture frontmatter comments: drop the issue labels the rule bans, matching the myst.yml.in files beside them. The labels in features.md, lists.md and the lists.md fancy-list line are rendered page text feeding features.png and lists.png, so they are left for a run of update-snapshots.yml. Reflow the comment lines that were edited in place and left short. Comment-only, verified mechanically against 200c4d1 with a comment stripper for TS/TSX/JS and CSS: no code token changes. tests/unit is unchanged at 17 passing; the 2 failures are missing node_modules in this environment and are identical before and after. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QAy8brFG9x8fXet3AV9dJE * docs(contributing): record the match-except-for-WCAG rule The theme already diverges from the lecture builds in four places and every one is an accessibility fix -- the outline's undimmed resting entries, its non-colour-only active state, the dark footer link, and the flat rem-rooted content size. That rule was only discoverable by reading four scattered comments, so state it: match the lecture builds, except where matching them fails WCAG, and say so with the measurement where you diverge. Changing an inherited value for any other reason stays a two-theme decision (#172 for the code palette, #201 for the footer). Note that a contrast ratio must be computed on the composited colour, since an `opacity` on the element is what the reader actually sees. That is the step the footer missed: the dark footer link was corrected for it and the light one never was. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QAy8brFG9x8fXet3AV9dJE * docs(changelog): record the comment rewrite, and say why og:type is "website" CONTRIBUTING.md requires every change to land an `## [Unreleased]` entry; this PR had none. Added under `### Changed`, with the scope, the comment-only property and the seven string edits it rests on. app/seo.ts: `og:type "website"` was left without a reason when the Sphinx reference came off, and "article" is the choice a future reader would reach for on a page with a byline. Records why it is not: the lectures carry none of the `article:*` metadata that choice commits to, and the deployed sites declare "website" throughout (which is what the comment on `main` said). Both raised in review on this PR -- the CHANGELOG entry by the Copilot reviewer, the og:type reason by quantecon-services. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QAy8brFG9x8fXet3AV9dJE * Revert "docs(changelog): record the comment rewrite" Reverts the CHANGELOG half of b8b8101, keeping the app/seo.ts og:type comment it also carried. I added the entry on a Copilot review flag citing CONTRIBUTING's "add your change to `## [Unreleased]`" rule, without checking whether this PR had already taken a position. It had: the description says no entry, because like #175 -- which corrected the FOUC comments and touched no CHANGELOG -- this changes nothing a consumer of the theme can see. Verified that 1aae2c8 (#175) indeed has no CHANGELOG entry, so the cited precedent holds. The author's call stands. It was also the sole cause of the merge conflict against main: #199 released v2.7.0, moving the `## [Unreleased]` entries this added to into `## [2.7.0]`. With the entry gone the PR no longer touches CHANGELOG.md at all. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QAy8brFG9x8fXet3AV9dJE --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Comment-only change. The CRITICAL_CSS block in
app/root.tsxand thefouc.spec.tsheader both explained the FOUC as WebKit painting the freshly-navigated document for ~1 frame before the head stylesheets apply. The v2.5.0 review established otherwise, and this PR rewrites both comments to the verified mechanism so the next person chasing a flash reasons from the right model.What actually happens, per fresh measurements against v2.5.0 (rAF-from-document-start sampler plus a head MutationObserver, WebKit, both
myst startand the static build):<style>re-inserted at 195ms); a re-inserted<link>re-applies asynchronously while a re-inserted inline<style>applies the instant it lands, so the critical CSS is the only styling in that gap.<body>fires both React errors, with zero head mutations and no flash. (This also proves the error probe can fail.)The rewritten comments also record the practical rule this implies: anything that must not flash (hidden by opacity, gated by a transition) needs a critical-CSS rule, because at flash time React is already mounted — mount-gating cannot help, and only the inline CSS survives. That is the pattern #168 applied to the back-to-top button.
The
webkit-fouc-only justification in the test header ("Chromium paint-holds and cannot exhibit this flash") leaned on the old model, so it now points at where the flash was actually observed (quantecon-theme-src#66) instead.Testing
npm run compileclean.webkit-foucPlaywright project: 2 passed (guard and control both still meaningful under the corrected model; the abort-the-stylesheets simulation is mechanism-agnostic)./update-snapshotsneeded.