Skip to content

Improve docs chrome navigation metadata#283

Merged
adnaan merged 1 commit into
mainfrom
docs-quality-chrome
Jun 1, 2026
Merged

Improve docs chrome navigation metadata#283
adnaan merged 1 commit into
mainfrom
docs-quality-chrome

Conversation

@adnaan
Copy link
Copy Markdown
Contributor

@adnaan adnaan commented Jun 1, 2026

Summary

  • render search affordance in the server-side sidebar and wire existing client search to it
  • expose source repo/path/ref/short commit metadata in page chrome
  • honor server.debug for the tinkerdown debug meta tag
  • include H3 entries in long-page TOCs while keeping short pages H2-only

Validation

  • npm run build (client)
  • go test ./internal/server
  • used by the docs worktree preview at http://0.0.0.0:8084

Note: I started a full go test ./... run but stopped it after the changed server package passed because the broader tinkerdown root package suite is browser-heavy and long-running.

@claude
Copy link
Copy Markdown

claude Bot commented Jun 1, 2026

Code Review

Overall this is a clean, well-scoped PR. The four concerns below are in descending order of severity.


1. URL injection risk in renderSourceMetainternal/server/server.go

parts = append(parts, fmt.Sprintf(`source: <a href="%s" target="_blank" rel="noopener">%s</a>`,
    html.EscapeString(page.SourceRepo),
    html.EscapeString(strings.TrimPrefix(page.SourceRepo, "https://github.com/")),
))

html.EscapeString escapes HTML entities but does not strip javascript: URIs. If SourceRepo were set to javascript:alert(document.cookie), the resulting href attribute would execute JS in the user's browser.

SourceRepo comes from frontmatter in controlled markdown files, so the practical risk is low today, but it's a fragile invariant — a single malformed or adversarially-crafted sync could introduce it. A minimal fix:

func isSafeURL(u string) bool {
    lower := strings.ToLower(strings.TrimSpace(u))
    return strings.HasPrefix(lower, "https://") || strings.HasPrefix(lower, "http://")
}

Then guard the <a> arm:

if page.SourceRepo != "" && isSafeURL(page.SourceRepo) {
    parts = append(parts, fmt.Sprintf(`source: <a href="%s" ...>`, html.EscapeString(page.SourceRepo), ...))
} else if page.SourceRepo != "" {
    parts = append(parts, fmt.Sprintf(`source: %s`, html.EscapeString(page.SourceRepo)))
}

2. Server-side renderSidebar escapes the site title — existing code, new exposure

html.WriteString(fmt.Sprintf(`<div class="nav-header"><h2>%s</h2></div>`, s.config.Title))
html.WriteString(`<button ... aria-label="Search docs">...`)

This is pre-existing, but this PR is the first to inject adjacent HTML (the search button) right next to the unescaped title interpolation. Not a new bug, but worth noting if s.config.Title can ever contain user-controlled content.


3. parseSections performs a redundant DOM query — client/src/core/page-toc.ts

const h2s = document.querySelectorAll('.content-wrapper h2');
const includeH3 = h2s.length >= 8 || document.querySelectorAll('.content-wrapper h3').length >= 10;
const headings = document.querySelectorAll(includeH3 ? '.content-wrapper h2, .content-wrapper h3' : '.content-wrapper h2');

When includeH3 is false, headings is a fresh query returning the same nodes already in h2s. The existing h2s NodeList can be reused directly:

const h2s = document.querySelectorAll('.content-wrapper h2');
const includeH3 = h2s.length >= 8 || document.querySelectorAll('.content-wrapper h3').length >= 10;
const headings = includeH3
  ? document.querySelectorAll('.content-wrapper h2, .content-wrapper h3')
  : h2s;

Minor on any page, but a cheap fix.


4. page-toc-level-2 class is added but has no CSS rule

The template literal now emits page-toc-level-2 on every H2 <li>, but page-toc.css only defines .page-toc-level-3. This isn't broken — H2 items still pick up styling through .page-toc-item — but adding an unused class introduces a subtle inconsistency. Either add a no-op .page-toc-level-2 {} comment acknowledging this is intentional, or emit the level class only when level === 3.


Minor notes

  • The SourceRef / SourceCommit wiring in page.go / tinkerdown.go / parser.go is clean and consistent with existing SourceRepo / SourcePath patterns. No concerns.
  • Fixing the hardcoded "true" debug meta to "%t" with the actual config value is a genuine bug fix — good catch.
  • The existingButton short-circuit in search.ts cleanly avoids double-attaching the click listener when the server renders the button. The data-search-button sentinel attribute is a nice coordination mechanism.
  • The H3 inclusion thresholds (≥8 H2s or ≥10 H3s) are reasonable and the comment explains the intent well.

@adnaan adnaan merged commit 7f2d97f into main Jun 1, 2026
4 checks passed
@adnaan adnaan deleted the docs-quality-chrome branch June 1, 2026 21:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant