Skip to content

fix(docs-site): act on the review findings from #2850 - #2851

Open
do-operator wants to merge 1 commit into
devfrom
docs/diagram-review-followups
Open

fix(docs-site): act on the review findings from #2850#2851
do-operator wants to merge 1 commit into
devfrom
docs/diagram-review-followups

Conversation

@do-operator

Copy link
Copy Markdown

Five findings from CodeRabbit on #2850. That PR merged before I read them, so they land here. All five were valid; each was checked against the code rather than taken on trust.

The one that matters

The label register failed WCAG AA. --sl-color-gray-3 resolved to #7c87973.64:1 on white, where AA wants 4.5 — and it sets the sidebar group labels, the table-of-contents heading, the search shortcut and the footer meta. All of those are 11px uppercase mono, the hardest case in the file.

It is #687383 now: 4.81:1, and still a step lighter than the body's muted ink, so the ramp keeps its shape rather than collapsing gray-2 and gray-3 into one value. Dark mode was already 6.16:1 and is untouched.

The rest

Footer headings polluted the page outline. The column headings were <h2> inside <main>, so a reader skimming by heading met "Method" and "Community" among the article's own sections. Each column is now a <nav> labelled by its own visible text. Verified against the built page: the footer no longer appears among the <h2>s, and the labels still render.

Diagram.astro diverged from the plugin it mirrors. Its regex had no g flag and did not escape the key, so a key used twice in one drawing replaced only its first occurrence, and a key containing regex metacharacters could match the wrong element. Proven both ways before and after the fix. The rehype plugin walks the tree and has neither problem.

The cache guard skipped invalidation when no diagrams were found, so deleting the last one would leave the pages that embedded it rendering from cache — the exact failure the integration exists to prevent.

The accessible-name guard checked ariaLabelledBy but not ariaLabel, so a diagram naming itself that way still had its name replaced by the markdown alt text.

Verified

lint clean, Prettier clean, 119 tests passing, build clean.

🤖 Generated with Claude Code

Five findings, all valid, verified against the code rather than taken on
trust.

The label register failed WCAG AA. --sl-color-gray-3 resolved to #7c8797,
which is 3.64:1 on white, and it sets the sidebar group labels, the table
of contents heading, the search shortcut and the footer meta — 11px
uppercase mono, the hardest case in the file. It is #687383 now: 4.81:1,
and still a step lighter than the body's muted ink so the ramp keeps its
shape. Dark was already 6.16:1 and is untouched.

The footer's column headings were <h2> inside <main>, so they joined the
page's heading outline: a reader skimming by heading met "Method" and
"Community" among the article's own sections. Each column is now a nav
labelled by its own visible text.

Diagram.astro substituted labels with a regex that had no g flag and did
not escape the key, so a key used twice in one drawing replaced only its
first occurrence and a key containing regex metacharacters could match
the wrong element. The rehype plugin walks the tree and has neither
problem; this path now escapes and replaces globally.

The diagram cache guard returned early when no diagrams were found, so
deleting the last one left the pages that embedded it rendering from
cache. The empty list now reaches the digest.

The accessible-name guard checked ariaLabelledBy but not ariaLabel, so a
diagram naming itself that way still had its name replaced by the
markdown alt text.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@greptile-apps

greptile-apps Bot commented Sep 9, 2026

Copy link
Copy Markdown
Contributor

RetriggerView in GreptileConfidence Score: 5/5

The PR appears safe to merge with no actionable defects identified.

Summary

  • Improves light-theme contrast for small muted labels.
  • Removes footer headings from article outlines while preserving accessible navigation labels.
  • Makes component-level diagram localization replace repeated escaped keys.
  • Invalidates content caches after the final diagram is removed.
  • Preserves SVG-authored accessible names supplied through aria-label.

@coderabbitai

coderabbitai Bot commented Sep 9, 2026

Copy link
Copy Markdown

Review Change StackReview Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Advanced

Run ID: 4b592026-a103-48a1-be68-b8cf13253ed8

📥 Commits

Reviewing files that changed from the base of the PR and between 87d3407 and d574aa0.

📒 Files selected for processing (5)
  • docs-site/src/components/Diagram.astro
  • docs-site/src/components/SiteFooter.astro
  • docs-site/src/integrations/diagrams.js
  • docs-site/src/rehype-inline-diagrams.js
  • docs-site/src/styles/custom.css

Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review.


📝 Walkthrough

Walkthrough

The docs site now translates repeated and regex-sensitive diagram labels, invalidates caches after the last diagram is removed, improves footer and SVG accessibility markup, and updates the light-theme gray-3 color.

Changes

Docs site quality updates

Layer / File(s) Summary
Diagram translation and cache processing
docs-site/src/components/Diagram.astro, docs-site/src/integrations/diagrams.js
Diagram label replacement now handles all literal key occurrences. Empty diagram sets now continue through digest computation and cache invalidation.
Footer and SVG accessibility
docs-site/src/components/SiteFooter.astro, docs-site/src/rehype-inline-diagrams.js
Footer columns now use labelled <nav> elements. Existing SVG aria-label values are preserved when applying markdown alt text.
Light-theme color adjustment
docs-site/src/styles/custom.css
The light-theme gray-3 token now uses #687383 with contrast rationale documentation.

Priority: ⬇️ Low

Estimated code review effort: 3 (Moderate) | ~20 minutes

Merge Risk: ⚪ Minimal · up to d574a

This updates documentation diagram translation and cache behavior while improving footer and SVG accessibility and light-theme contrast. The reported checks are clean, and no concrete merge-blocking risk remains.

🚥 Pre-merge checks | ✅ 4
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately identifies the documentation fixes as follow-up work for the review findings from PR #2850. It is concise and related to the main changes.
Description check ✅ Passed The description directly explains all five changes, their rationale, and the reported verification results. It is clearly related to the changeset.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch docs/diagram-review-followups

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

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