fix(client): page TOC stacks vertically — !important survives minifier

[adnaan]

Summary

Recurring bug, finally root-caused. PR #254 fixed the page-TOC items being laid out horizontally across the sidebar by adding `display: block` to `.page-toc-list` and `display: list-item` to `.page-toc-item`. The fix worked locally but kept regressing on every release.

Why it kept regressing: esbuild's CSS minifier (`--minify` in `client/package.json`'s `build:browser` script) strips `display: block` from `

` selectors as a "redundant default" optimization (since `block` IS the user-agent default for `
`). Same for `display: list-item` on `
• `. The minifier doesn't see the cascade context — it can't know PicoCSS's `nav ul { display: flex }` is overriding the default that's about to be stripped.

  Result: every `npm run build:browser` silently undid PR fix(client): page TOC items render vertically, not horizontally #254's cascade-override fix.

  Fix

  Add `!important` to the three layout-defining display rules:

  • `.page-toc-list` — `display: block !important`
  • `.page-toc-item` — `display: list-item !important`
  • `@media (max-width: 768px) .page-toc-list` — `display: none !important` (matches the desktop `!important` so the mobile rule still wins via specificity tie-break + the media context)

  esbuild preserves `!important`. Comment in source explains both why the rule is needed (PicoCSS cascade) AND why `!important` is needed (minifier behavior) so the next maintainer doesn't clean it up and reintroduce the regression.

  Bundled assets in `internal/assets/client/` regenerated via `cd client && npm run build:browser` + cp.

  Verified locally

  • Source: `display: block !important` in `page-toc.css`
  • Bundled: `page-toc-list{display:block!important;...}` in committed asset
  • Served: same — `curl /assets/tinkerdown-client.css` confirms
  • iPhone manual signoff: TOC entries stack vertically (was horizontal-flow regression)

  Test plan

  • `go build ./...` clean
  • `go test` (non-e2e) on internal packages — clean
  • Local docs site renders with vertical TOC under "Your First App"
  • CI green
  • Release v0.2.2 after merge

  🤖 Generated with Claude Code

[claude]

Code Review

Overview

This PR root-causes a recurring CSS regression where the page TOC items rendered horizontally instead of vertically. The fix adds `!important` to three `display` rules in `page-toc.css` to survive esbuild's CSS minifier, which was silently stripping the cascade-override rules on every release build. The diagnosis is correct and the explanation is solid.

---

What's Good

• Root cause is correctly identified. The two-factor explanation (PicoCSS specificity cascade + esbuild minifier stripping "redundant defaults") is accurate and the fix addresses both.
• The comment is excellent. Explaining why `!important` is load-bearing — rather than just "needed for override" — is exactly the kind of comment that prevents the next person from "cleaning it up" and reintroducing the bug. This is the right approach.
• Minimal footprint. Three targeted lines changed in the source; no structural changes to the CSS.

---

Issues and Suggestions

1. JS bundle changed in a CSS-only fix — needs explanation

The diff touches `tinkerdown-client.browser.js` and its source map. A CSS-only fix should not modify the JS bundle. This suggests the build was run against a tree that had uncommitted (or unrelated) JS changes, which then silently snuck into the bundle artifact. Before merging, it's worth verifying:

git diff HEAD~ -- client/src/**/*.js client/src/**/*.ts

If there are no JS source changes on this branch, the JS bundle diff is stale contamination and should be regenerated cleanly from a fresh checkout.

2. Minor comment typo

In the mobile media query block:

/* !important matches the desktop rule's hammer so this win on mobile. */

Should be "so this wins on mobile."

3. Committed minified artifacts make review very hard

The diff was ~758 KB, almost entirely minified/sourcemapped output. A CSS-only change produced a 29-addition/20-deletion PR that was effectively unreadable. This is a pre-existing pattern in the repo, but given the JS-bundle contamination risk raised above, it's worth considering whether the build artifacts should be committed at all (or committed only in a dedicated release step). At minimum, a `.gitattributes` entry marking them as `linguist-generated` would declutter GitHub's diff view.

4. Consider specificity over `!important` (optional, low priority)

An alternative to `!important` is raising the selector's own specificity to beat PicoCSS's `nav ul`:

nav .page-toc-list { display: block; }
nav .page-toc-item { display: list-item; }

This would survive the minifier (it can't strip `display: block` from a non-`ul` selector) and avoids `!important` in the cascade. That said, given the two-layered problem and the excellent comment, the current approach is pragmatic and correct — this is more a "consider for future" note than a blocker.

5. esbuild minifier workaround not documented in build tooling

The root cause is a build-tool behavior that will silently bite any future CSS that overrides `display` on `ul`/`li` inside `nav`. A one-line note in `client/package.json`'s `build:browser` script or a `CONTRIBUTING.md` section on "known esbuild gotchas" would help future contributors.

---

Summary

Approve with suggestions. The fix is correct and the source CSS change is clean. The main thing to verify before merging is whether the JS bundle diff contains unintended changes — if it does, regenerate from a clean checkout. The typo and the `!important` vs. specificity trade-off are minor; the build-artifact size issue is a pre-existing concern that doesn't need to block this PR.

[Copilot]

Pull request overview

This PR addresses a recurring docs-site sidebar regression where nested “page TOC” entries render horizontally after release builds, by making the TOC’s layout-defining display rules survive esbuild’s CSS minification and ensuring the bundled client assets reflect the fix.

Changes:

• Add !important to .page-toc-list / .page-toc-item display rules (and the mobile display: none) with an explanatory comment.
• Regenerate and commit the bundled client CSS + sourcemap under internal/assets/client/.
• Regenerate and commit the bundled client JS asset under internal/assets/client/.

Reviewed changes

Copilot reviewed 2 out of 3 changed files in this pull request and generated 3 comments.

File	Description
client/src/core/page-toc.css	Makes TOC layout overrides minifier-resistant via !important and documents rationale.
internal/assets/client/tinkerdown-client.browser.css	Updated bundled CSS containing the !important display rules.
internal/assets/client/tinkerdown-client.browser.css.map	Updated sourcemap to match the new CSS source content.
internal/assets/client/tinkerdown-client.browser.js	Updated bundled JS asset (large diff) as part of the rebuild/copy step.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[claude]

Code Review

Overall: Approve with minor notes. The fix is correct, the root cause analysis is solid, and the comment quality is excellent. A few things worth flagging.

---

What the PR does

Adds !important to three display rules in page-toc.css to survive esbuild's CSS minifier, which was silently stripping display: block / display: list-item on every release build — causing the PR #254 fix to keep regressing.

---

Strengths

• Root cause, not symptom. The recurring regression is now explained and defended against at the source, not just re-applied.
• Comment quality is high. The block comment explains why !important is needed (two distinct reasons), and the inline comment on .page-toc-item cross-references it. This is exactly the right level of documentation for a non-obvious constraint.
• Minimal change. Only the three rules that need it got !important. No unrelated cleanup.
• Bundled assets committed. The regenerated .browser.css and .browser.css.map are included, so the repo stays self-contained and the regression can be verified without a local build.

---

Issues / Questions

1. Specificity claim in the comment may be imprecise

The comment states that PicoCSS's nav ul { display: flex } wins on specificity over .page-toc-list { display: block }. By the standard specificity algorithm:

• nav ul → (0, 0, 2) — two element selectors
• .page-toc-list → (0, 1, 0) — one class selector

A class beats element selectors, so .page-toc-list should win by specificity, not lose. The comment's specificity reasoning may be wrong (or PicoCSS uses a more qualified selector like [data-theme] nav ul or body nav ul that pushes it above (0, 1, 0) — in which case the comment should name the actual Pico selector).

The minifier stripping reason is unambiguously correct and sufficient on its own to justify !important. I'd suggest softening reason #1 in the comment to avoid misleading future maintainers, e.g.:

/* 1. PicoCSS's `nav ul { display: flex }` may win on specificity or source
 *    order depending on how Pico is loaded — the exact cascade depends on
 *    context we don't control. `!important` sidesteps this entirely.

Or, if you can confirm Pico's actual selector, be specific about it.

2. Minifier configuration not considered as an alternative (informational)

esbuild's --minify bundles --minify-identifiers, --minify-syntax, and --minify-whitespace. The redundant-display stripping comes from --minify-syntax. Splitting to --minify-identifiers --minify-whitespace (omitting --minify-syntax) would preserve the original declarations without !important. Not necessarily the right call — !important is simpler and the PR description calls it "the minimum hammer" — but it's worth having considered and the PR description doesn't mention it. Noting it here for completeness; not a blocking concern.

3. Mobile media query — confirm ordering

The PR description mentions display: none !important in the @media (max-width: 768px) block. When two !important rules on the same property have equal specificity, the later declaration in source order wins. As long as the @media block appears after the base .page-toc-list rule (which it should), the mobile hide will win correctly on small screens. Just double-check the final source order in the committed asset is what's expected.

---

No concerns on

• Security: CSS-only change, no attack surface.
• Test coverage: CSS layout changes can't realistically be unit-tested; the iPhone manual signoff and local verification described in the PR description are appropriate.
• Go build: go build ./... passing is expected since this is a static asset change.

---

Summary

The fix is correct and the root cause is well-understood. The main thing I'd ask for is a small clarification/correction on the specificity claim in the comment (reason #1), since .page-toc-list actually outranks nav ul by standard specificity math — the !important is justified by the minifier alone, and the comment should reflect that accurately so the next maintainer doesn't get confused.