chore(comments): code comments explain the code, not its history - #200
Conversation
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>
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>
|
🎭 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.
🔵 Needs a closer look
It spans 41 files of dense, domain-specific comment rationale (contrast ratios, hydration timings, layout math) that warrant human domain verification, and the repo's required ## [Unreleased] CHANGELOG entry (CONTRIBUTING.md:187-189) is absent though it falls outside the PR's changed files.
Pull request overview
This PR is a documentation/comment cleanup that rewrites code comments across the theme so they explain what the code does and why, rather than narrating the parity project's history (phase numbers like Phase 6 (#92), PLAN.md items, tracker issues used as labels, and "matches the Sphinx build" as a justification). It also codifies the rule in a new Code comments section of CONTRIBUTING.md. The theme's runtime behavior is unchanged: apart from seven documented, baseline-neutral string edits (test titles, one CI step name, one fixture stderr string), every change is comment text. Along the way it corrects a handful of now-stale claims (e.g. five light-mode code colours fail AA, not two; numbers measure 3.2:1) and moves a misplaced doc comment back above the Multilingual editions suite.
Changes:
- Rewrite ~41 files' comments to drop project framing (phase numbers,
PLAN.md, issue-number labels) and replace "matches the Sphinx build" with the actual reason (measurement, contrast ratio, layout constraint), keeping one provenance line for ported code. - Add a Code comments section to
CONTRIBUTING.mdestablishing the rule for new code. - Fix stale factual claims and reposition the multilingual doc comment; retain issue links (#126, #130, #172, #117) only where they carry detail a line cannot.
File summaries
| File | Description |
|---|---|
| CONTRIBUTING.md | Adds the "Code comments" convention (no project framing, no issue labels, reasons not comparisons). |
| styles/quantecon.css | Bulk of the change: ~40 Sphinx-comparison justifications replaced with each value's own reason; corrects AA-failure count and number contrast to 3.2:1. |
| tests/visual/theme.spec.ts | De-labels test titles, corrects the site-options/twitter:site note, moves the multilingual doc comment above its suite. |
| tests/visual/static.spec.ts, static-server.mjs, serve-static.sh | De-labels #186/#138/#150 and generalizes "lecture sites" to "static hosts". |
| tests/visual/fouc.spec.ts | Reframes hydration wording as conditional; keeps #126 for timings. |
| tests/visual/fixture*/*.yml.in, notebook.ipynb, unit/seo.test.mjs, unit/i18n.test.mjs | Drop issue labels from comments; one fixture stderr string and one test title renamed. |
| app/root.tsx, revalidate.ts, seo.ts, routes/$.tsx, _index.tsx | Rewrite meta/hydration/revalidate comments; drop #92/#186 labels, keep #126. |
| app/i18n.ts, links.ts, renderers.tsx, types.ts, components/* | Replace book-theme/Sphinx framing with reasons and one-line provenance. |
| tailwind.config.js, template.yml, playwright.config.ts, styles/rtl.css, styles/app.css | De-frame layout/units/config comments. |
| scripts/relative-css-asset-urls.mjs, plugins/git-metadata.mjs | Drop #138/#150/PLAN.md references. |
| .github/workflows/{ci,preview,release}.yml | De-label a CI step name and drop stale PLAN.md/make deploy paragraphs. |
Review details
- Files reviewed: 40/41 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.
quantecon-services
left a comment
quantecon-services
left a comment
There was a problem hiding this comment.
Reviewed the full diff and independently checked the claims: stripped comments from both branches (esbuild for TS/TSX/JS/CSS, parsed YAML and JSON) and recomputed every contrast ratio the comments cite.
Verified
- Behaviour is unchanged. After stripping comments, 30 of 40 code files are byte-identical to
main; the other 10 differ only in the seven renamed strings listed in the description. Screenshot baselines are named explicitly, so the suite renames move nothing. - The
#172correction is right: five light palette values fall short of AA (2.75 / 2.94 / 3.24 / 3.65 / 4.21), and numbers are 3.24:1, not 3.4:1. Every other figure in the rewritten comments checks out (#af5f5f 4.54:1 and 3.85:1 on stone-900, #d78787 6.42:1, #44403c 10.3:1, #6cb6ff 7.4:1 on #222, the 0.95625rem derivation, the 122px outline offset, the 1.75rem sub-entry padding). - No PLAN.md, phase or Sphinx references remain in code apart from the three
template.ymldescriptions kept on purpose.
Suggested improvements
-
Inherited wrong figure in
styles/quantecon.css(stderr fold). The comment says the#856404label is 7.0:1 on white. It is 5.5:1 (still AA). The old comment had the same error, but since this PR is about comments carrying the true measurement, worth fixing here. -
Reword the code-palette sentence. "It ships unconditionally, with no toggle, since no site turns it off" refers to a Sphinx option this theme never had. Suggest: "It ships unconditionally; there is no toggle."
-
Three removals dropped a design decision rather than history. The rule as written does not distinguish the two, so these may be worth a single retained clause each, or an explicit "deliberate design decisions" exception in the CONTRIBUTING section:
styles/quantecon.csslinks block: the note that hover-only underline and#004979visited links were consciously left out.tests/visual/theme.spec.tslaunch-colab: BinderHub deliberately not offered and JupyterHub removed. The test now asserts "the only launch target" with no reason why.app/seo.ts:og:type "website"now has no stated reason. A lecture page could plausibly be"article", so a future reader may change it.
-
Optional, same rule applied to docs.
README.mdstill describes the OpenGraph set and stderr fold in terms of the Sphinx sites, andtests/visual/README.mdcarries three issue-number labels. Out of scope here, but a natural follow-up alongside thefigure/lists px comments already listed.
Items 1 and 2 are one-line edits in the same file; the rest are judgement calls. Approve either way.
Generated by Claude Code
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>
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>
|
Thanks for the review. Where each point stands as of 200c4d1:
|
Review — reviewed at
|
| Claim | Computed |
|---|---|
#0072bc on white 5.08:1 |
5.08 |
#af5f5f on #222 3.50:1 |
3.50 |
#d78787 on #222 5.84:1 |
5.84 |
#5fafaf / #d7af5f on #222 6.2 / 7.7 |
6.24 / 7.72 |
#856404 on white 5.5:1 |
5.49 |
#0072bc @70% over #222 ≈2.2:1 |
2.17 |
blue-300 @70% over #222 ≈5.0:1 |
5.04 |
The re-grounding from stone-900 to the #222 content panel is the right call and one I had missed: .article sits inside Page.tsx's dark:bg-qepage-dark, not on the body. "Six light values fall short of AA" is now correct (#70a0d0 2.75, #60a0b0 2.94, #40a070 3.24, #00a000 3.48, #bb60d5 3.65, #0e84b5 4.21). The figure px comments (18px/9px) and the UNITS/toolbar-control reconciliation are right too.
The one substantive disagreement: bullet 3 over-fires
Sphinx mentions in source fall from 95 across 15 files to 1; book-theme from 23 to 9. "Matches the Sphinx build" reads as project framing, but for this theme it is the specification — README.md still describes the theme in those terms. styles/quantecon.css is ~700 lines of values obtained by measuring the deployed lecture builds, and now carries no statement that it was derived from anything, though bullet 3 itself grants ported code a provenance line.
Where the comparison was the only justification for a magic number, the replacement restates the declaration instead of giving a reason:
styles/quantecon.css |
Now reads | Gap |
|---|---|---|
font-size: 0.9rem (footer) |
"chrome at a fixed size" | Doesn't say why 0.9. Was .8rem of an 18px root. |
opacity: 0.7 |
"fades the rule, badge and text together" | Restates what opacity does. |
margin-bottom: 1.25em |
"18px, in place of its 20px mb-5" |
Doesn't say why 18. |
back-to-top 15.3px / 600 |
"a small blue pill" | Reads as arbitrary; it is a measurement. |
Also deleted without replacement: "Known differences from the Sphinx build, deliberately out of scope here: it underlines only on hover, and colours :visited links #004979." — a recorded known divergence, not history, and now nowhere in the repo. Same for "Self-hosting Source Code Pro is a separate change."
Suggestion: amend the bullet so a cross-system constraint counts as a reason — /* 14.4px: the lecture builds' .8rem of an 18px root */ satisfies both goals — and restore a handful of provenance anchors.
Still open
styles/quantecon.css:616-618self-contradicts. "a fixed size, not the content base", then ".85 of the 18px content size". A maintainer moving.articleto 19px is told by the second sentence to do what the first forbids. The value is pinned to the ported button, not to.article.static.spec.ts:84contradictsOutline.tsx:25on the same decision — "the providerLink, which resolves … and turns 'Top' into a full reload" vs "the provider would resolve … 'Top' would leave the document".Outline.tsxis right.- Seven bare labels survive in fixture Markdown in directories this PR edited:
features.md:12(#173),intro.md:2/lists.md:3(#143),outline.md:7+features.md:100(#182),lists.md:53(#121),lists.md:9QuantEcon/mystmd#50. The first three are inert frontmatter;features.md:100,lists.md:9andlists.md:53are rendered body prose feedingfeatures.png/lists.png, so those would move baselines — a real reason to defer, worth stating. - The rule's bullets 1 and 2 overlap — bullet 1 bans tracker issues outright, bullet 2 then permits them conditionally — and the section never states its scope. It is applied to YAML and shell comments here while
README.md,docs/,tests/visual/README.mdandPLAN.mdkeep the banned framing. - Nothing enforces it.
.eslintrc.jsexists but there is nolintscript or CI step. - ~10 comment lines were edited in place without re-wrapping (
theme.spec.ts:20,:451,fixture/myst.yml.in).
Worth keeping
serve-static.sh going from (#138, #150, #186) to "asset URLs that only resolve under myst start, loader fetches a static host cannot answer" is the PR at its best — the number replaced by the thing it stood for. Likewise the outline sub-list padding decomposed into this file's own values, the playwright.config.ts baseline-vs-candidate recipe generalised out of a one-off 2.0.0 upgrade note, and the misplaced "Multilingual editions" doc comment moved back over its own suite.
Generated by Claude Code
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
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
How the review was addressed — now at
|
There was a problem hiding this comment.
🔵 Needs a closer look
It spans 47 files with many nuanced, individually hard-to-verify technical assertions (measured pixel values, contrast ratios, upstream behaviour) where my sampled checks all passed but exhaustive verification of every rewritten comment warrants a human maintainer's sign-off.
Review details
- Files reviewed: 50/51 changed files
- Comments generated: 0 new
- Review effort level: Balanced
…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
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
Brings in the v2.7.0 release (#199). No conflicts once the CHANGELOG entry reverted above is gone -- this branch no longer touches that file.
Code comments across the theme explained the code through the history of the parity project: phase numbers (
Phase 6 (#92)), tracker issues used as labels,PLAN.mditems, and "matches the Sphinx build" given as the reason for a value. That context already lives in git, the PRs andCHANGELOG.md, and in the code it goes stale once the project closes. This PR rewrites those comments to say what the code does and why, corrects the comments that turned out to be wrong about the code, and adds the rule to CONTRIBUTING.md so new code follows it.The theme's behaviour does not change. Apart from the renamed test titles, one CI step name and one fixture string listed below, every edit is to comment text.
The rule
A new Code comments section in CONTRIBUTING.md:
PLAN.mditems, milestones or tracker issues.What changed
47 files: 19 in
app/, 13 intests/, 4 instyles/, 3 workflows,plugins/git-metadata.mjs,scripts/relative-css-asset-urls.mjs, andCONTRIBUTING.md,Makefile,.npmrc,playwright.config.ts,tailwind.config.jsandtemplate.ymlat the root.The largest share, about a third of the changed lines, is
styles/quantecon.css, where about 40 values were justified by the Sphinx build; those comments now give each value's own reason. Most of the rest is the same kind of rewrite, with reasons that leaned on the Sphinx build or history told in the past tense restated as what the code does (chieflytests/visual/theme.spec.ts,app/components/Outline.tsx,app/types.tsandtests/visual/fouc.spec.ts), plus short labels such as(Phase 4, #90)or-- #186coming off otherwise sound comments.Edits that are not comment text:
tests/visual/theme.spec.tsOn this page outline (#182)On this page outlinetests/visual/theme.spec.tsMeta/SEO and notebook output polish (#92)Meta/SEO and notebook outputtests/visual/theme.spec.tsSite options reach the theme (#173)Site options reach the themetests/visual/static.spec.tsStatic build -- in-page anchors (#186)Static build -- in-page anchorstests/unit/seo.test.mjsthe Sphinx set, on a lecture page …the full set, on a lecture page ….github/workflows/ci.ymlstepRun FOUC guard (QuantEcon/quantecon-theme-src#66)Run FOUC guard(#92)Screenshot baselines are named explicitly in each
toHaveScreenshotcall rather than derived from test titles, and nothing filters tests by title, so the renames move no baseline. The CI step name is not a status check; job names are unchanged.Corrected along the way
Two review passes turned up comments that were wrong about the code; they now match it:
#222content panel, so they are restated for that ground.1remfor what is1em(18px) and0.5em(9px).app/seo.tsand both routemetafunctions described the social tags as filling gaps in upstream's set; they also replace upstream'stwitter:card, andtwitter_logo_urlwins over a page thumbnail.app/root.tsxsaid thesans-seriftail renders before the webfont lands, where normally the metric-matched fallback face does, and its keep-in-sync list pointed at upstream's Document for the<body>class that this theme renders inapp/components/Document.tsx.playwright.config.tsandpreview.ymlsaid the Playwright harness never runs a staticmyst build --html; thestatic-chromeproject does.tests/visual/fouc.spec.ts(current builds hydrate cleanly); asite-optionsnote callingtwitter:sitean open gap (social-metaasserts it); arelease.ymlparagraph about amake deploytarget that no longer exists; aPLAN.mdclaim about the plugin's long-term home; twoquantecon.csspointers to a removedtailwind.config.jsnote; atemplate.ymlpointer to a README section that does not exist (nowdocs/launch.md); "a directory above"_assets/inscripts/relative-css-asset-urls.mjs, whereroutes/sits beside it; and the multilingual suite's doc comment, which had drifted above the outline suite.References kept
A link stays where its thread holds detail the comment cannot carry:
QuantEcon/mystmd#50: the fork PR that adds thestyleanddelimiterfields the list renderer reads.QuantEcon/mystmd#51: the theme-rendering follow-up;app/renderers.tsxcites it for when the override can be dropped.quantecon-theme-src#66: the WebKit flash of unstyled content that the FOUC suite guards.#117: the regressionexecute-nested-cells.test.mjsexists to catch.#70: the unlinked sidebar entries thesidebar-opentest exists to catch.One provenance line stays on code ported from
quantecon-book-theme(launch.py,scrollspy.js, the git-metadata and i18n helpers, the_code.scsspalette) and onupdate-snapshots.yml. The threetemplate.ymloption descriptions that name the book-theme equivalents are left as they are: they are user-facing, and a lecture repo migrating its_config.ymlneeds that mapping.Verification
mainafter stripping comments from both sides: TypeScript's printer for TS, TSX and JS, postcss for CSS, parsed YAML with shell comments dropped fromrun:blocks, parsed JSON, and full-line#comments dropped from the shell scripts,Makefileand.npmrc. All are identical once the seven renamed strings in the table are reversed. The checker was first shown to fail on a planted code change for each of those comparisons.@ts-*,eslint,/*!,rtl:or similar comment was added or removed.tests/unit/seo.test.mjsandtests/unit/launch-urls.test.mjspass locally.Changelog. No entry: like #175, which corrected the FOUC comments, this changes nothing a consumer of the theme can see.
🤖 Generated with Claude Code