docs: overhaul modules docs navigation and UX#125
Merged
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
📝 WalkthroughSummary by CodeRabbit
WalkthroughImplements a data-driven docs system: adds Changes
Sequence DiagramsequenceDiagram
actor User
participant Browser as Browser DOM
participant Storage as localStorage
participant Fetch as Network / Search Index
participant Lunr as Lunr Index
participant Mermaid as Mermaid Renderer
rect rgba(0, 150, 200, 0.5)
Note over User,Storage: Theme Toggle Flow
User->>Browser: Click theme toggle
Browser->>Storage: Read/Write `specfact-theme`
Browser->>Browser: Update `document.documentElement[data-theme]`
Browser->>Mermaid: Call rerenderMermaid(newTheme)
Mermaid->>Mermaid: Re-initialize diagrams with themeVariables
end
rect rgba(150, 0, 150, 0.5)
Note over User,Lunr: Search Flow
User->>Browser: Focus or type in search input
Browser->>Lunr: Check in-memory index
alt Index missing
Browser->>Fetch: GET /assets/js/search-index.json
Fetch->>Browser: Return JSON
Browser->>Lunr: Build index (title/keywords/content)
end
User->>Browser: Type query (>=2 chars)
Browser->>Lunr: Run query (with wildcard)
Lunr->>Browser: Return results
Browser->>Browser: Render results (snippets, tags)
User->>Browser: Navigate via keyboard/enter
Browser->>Browser: Navigate to result URL
end
rect rgba(200, 150, 0, 0.5)
Note over User,Storage: Expertise Filter Flow
User->>Browser: Click expertise pill
Browser->>Storage: Persist `specfact-expertise`
Browser->>Browser: Toggle pill active state
Browser->>Browser: Add/remove `.hidden-by-filter` on `li[data-expertise]`
Browser->>Browser: Hide empty bundle `<details>`
Browser->>Browser: Update expertise count display
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
djm81
added
documentation
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 21
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/_layouts/default.html (1)
210-216:⚠️ Potential issue | 🔴 CriticalMalformed HTML: Extra closing
</div>tags cause unpaired tag error.The HTMLHint error is valid. Lines 214-216 close three
</div>tags, but only two were opened (.footer-col-wrapperat 210 and.footer-colat 211). This causes the.wrapperdiv to close prematurely, leaving the footer links and trademark outside the wrapper.🐛 Proposed fix: Remove extra closing tag
<div class="footer-col"> <p class="p-name">{{ site.title | escape }}</p> <p>{{ site.description | escape }}</p> </div> - </div> - </div> + </div> <div class="footer-links">The structure should be:
<div class="wrapper">(line 209)
<div class="footer-col-wrapper">(line 210)
<div class="footer-col">...</div>(lines 211-214)</div>(close footer-col-wrapper)<div class="footer-links">...- ...
</div>(close wrapper, line 229)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/_layouts/default.html` around lines 210 - 216, The footer markup has one too many closing </div> tags: remove the extra closing tag after the .footer-col block so the DOM tree becomes .wrapper > .footer-col-wrapper > .footer-col (closed) then close .footer-col-wrapper and keep the surrounding .wrapper open so .footer-links and trademark remain inside .wrapper; locate the block containing the classes "wrapper", "footer-col-wrapper", and "footer-col" and delete the redundant closing </div>.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/_includes/breadcrumbs.html`:
- Around line 4-15: The breadcrumb loop uses forloop.last which becomes true for
the trailing empty segment of a trailing-slash URL, causing the real last
non-empty segment to render as a link; fix this by first computing the last
non-empty segment (e.g., add a preliminary loop over url_parts that sets a
variable like last_segment when part != ''), then in the rendering loop compare
part == last_segment (instead of forloop.last) to decide when to render the
<span class="current"> for the current page; reference page.url, url_parts,
crumb_path, last_segment, and forloop in your changes.
In `@docs/_includes/expertise-filter.html`:
- Around line 2-5: The buttons with class "expertise-pill" (the elements
currently rendering "All", "Beginner", "Intermediate", "Advanced") are missing
an explicit type and will default to submit when inside a form; update each
button element to include type="button" (e.g., the <button
class="expertise-pill" ...> instances) so they no longer trigger unintended form
submissions.
In `@docs/_includes/theme-toggle.html`:
- Line 1: The theme toggle button currently relies on the default button type
(submit) which can accidentally submit a surrounding form; update the <button>
element used for the theme toggle (the element invoking toggleTheme()) to
include an explicit type="button" attribute so it won’t trigger form submission
if placed inside a form.
In `@docs/_layouts/default.html`:
- Around line 127-145: The selector in rerenderMermaid is matching child <svg>
elements (".mermaid svg") so removeAttribute('data-processed') and data-source
restoration are applied to the wrong node; update rerenderMermaid to query only
container elements (e.g., document.querySelectorAll('.mermaid[data-processed]'))
and iterate those containers to call removeAttribute('data-processed') and
restore data-source, and if you need to clear nested SVGs do that by querying
container.querySelectorAll('svg') inside the loop; adjust mermaid.run logic
accordingly in the rerenderMermaid function.
- Around line 147-157: The race condition occurs because initMermaid() triggers
mermaid rendering before you save the original diagram text into the data-source
attribute; modify the DOMContentLoaded handler so you first iterate
document.querySelectorAll('.mermaid') and set data-source from
el.textContent.trim() (only if not already set) before calling
initMermaid(currentTheme)/mermaid.initialize(startOnLoad: true), ensuring the
original source is preserved for re-rendering; reference the initMermaid
function, the data-source attribute, and the querySelectorAll('.mermaid') loop
when making the change.
In `@docs/assets/js/filters.js`:
- Line 6: Wrap accesses to localStorage around try/catch: when reading the
initial value for the variable "stored" (currently assigned via
localStorage.getItem('specfact-expertise') || 'all') catch any exceptions and
fall back to 'all'; likewise wrap any localStorage.setItem calls (the one
referenced near where you update 'specfact-expertise') in try/catch to avoid
throwing when storage is unavailable, optionally silently ignoring errors or
logging them via console.warn; update code references to localStorage.getItem
and localStorage.setItem accordingly to use these guarded accesses.
In `@docs/assets/js/search.js`:
- Around line 12-35: The loadIndex function currently assumes fetch succeeds and
lunr is present; add defensive checks and error handling: verify lunr is defined
before calling lunr(...) (and log/report a clear error if missing), wrap the
fetch(indexUrl) promise with a .catch to handle network errors and set a
fallback state (e.g., leave lunrIndex null and surface an error message), and
add a .catch on the JSON parsing / index building sequence to catch and log
errors from malformed data or this.add failures; refer to loadIndex, lunrIndex,
searchData, indexUrl and the use of lunr(...) when adding these checks and
logging so search failures fail gracefully instead of throwing.
- Around line 60-68: The rendering concatenates unescaped search data
(audience/expertise elements, tag, doc.url, doc.title, snippet) into HTML,
enabling XSS; update the search rendering to escape or sanitize all dynamic
values before inserting into the DOM (e.g., implement an escapeHtml helper and
use it for tag, doc.title, and snippet, and set link href via element property
or validate/encode doc.url rather than embedding raw into innerHTML), or replace
string concatenation with DOM APIs (createElement, textContent, setAttribute) in
the function that builds results so no user-controlled string is injected
directly.
In `@docs/assets/main.scss`:
- Around line 11-117: Add a baseline set of CSS custom properties under a :root
selector so variables are never unset (use the dark-theme values as sensible
defaults); update docs/assets/main.scss by adding a :root { --primary-color:
..., --bg-color: ..., --text-color: ..., etc. } block (matching the same
variable names used in the [data-theme="dark"] and [data-theme="light"] blocks)
placed before the themed blocks and media queries so the styles fall back
correctly when no data-theme attribute or prefers-color-scheme applies.
In `@docs/guides/brownfield-engineer.md`:
- Around line 8-10: This redirect page's frontmatter includes a title and extra
metadata (the title field causes inclusion in search-index.json), so either
remove the metadata fields (e.g., delete the keywords/audience/expertise_level
frontmatter) from brownfield-engineer.md and other redirect pages like
brownfield-journey.md to prevent stale content from being indexed, or add a
clear comment and documentation stating the intent to surface old URLs in
search; locate the frontmatter title field and metadata block in the file and
either strip the non-essential keys or annotate them as intentional so the
template/filtering logic around search-index.json handles redirects correctly.
In `@docs/guides/common-tasks.md`:
- Around line 7-12: The frontmatter keywords line currently lists "keywords:
[common-tasks, reference, quickstart, cheatsheet, cli]" which no longer matches
the page content (a redirect notice); update that keywords array to reflect the
page's redirect purpose (for example change to "keywords: [common-tasks,
redirect, getting-started]" or a similar set you prefer) by editing the keywords
line in this file so search/discovery aligns with the redirect notice.
In `@docs/index.md`:
- Around line 21-62: The path card links in the HTML block (elements with class
names path-cards and path-card) use bare relative URLs (e.g.,
"getting-started/choose-your-modules/") which is inconsistent with other links
that use Jekyll's relative_url filter; update each <a href="..."> in the
path-card blocks to use the Jekyll filter form (for example replace
getting-started/choose-your-modules/ with {{
'/getting-started/choose-your-modules/' | relative_url }}) so all links in
docs/index.md are consistent and robust when deployed to a subdirectory.
In
`@openspec/changes/docs-13-nav-search-theme-roles/specs/cross-module-workflow-docs/spec.md`:
- Line 1: The first heading in spec.md uses "## MODIFIED Requirements" which
violates markdownlint MD041; change the first heading to an H1 by replacing or
promoting "## MODIFIED Requirements" to "# MODIFIED Requirements" (or insert an
H1 above it) so the first line of the file is an H1; locate the "MODIFIED
Requirements" heading in the file to make the change.
In
`@openspec/changes/docs-13-nav-search-theme-roles/specs/docs-client-search/spec.md`:
- Around line 1-56: Change the top-level heading "## ADDED Requirements" to an
H1 ("# ADDED Requirements") to satisfy MD041, and add blank lines before and
after each heading (e.g., "### Requirement: Search index generation", "####
Scenario: Search index contains all pages", "### Requirement: Search UI in
sidebar", etc.) so every heading has an empty line above and below it to satisfy
MD022; follow the same blank-line pattern used in the suggested patch across all
"###" and "####" headings in this file.
In
`@openspec/changes/docs-13-nav-search-theme-roles/specs/docs-nav-data-driven/spec.md`:
- Around line 1-4: Add a top-level H1 heading and ensure a blank line before and
after each heading to satisfy MD041/MD022; specifically insert an H1 (e.g.,
"Specs — Navigation Data Driven") above the existing "## ADDED Requirements" and
add blank lines around the "## ADDED Requirements" and "### Requirement:
Navigation data file" headings so the file's headings (including the referenced
`docs/_data/nav.yml` requirement) conform to markdown lint rules.
In
`@openspec/changes/docs-13-nav-search-theme-roles/specs/modules-docs-command-validation/spec.md`:
- Around line 1-3: Add a top-level H1 heading at the start of the file and
ensure blank lines before and after each existing heading (e.g., "## MODIFIED
Requirements" and "### Requirement: Docs validation SHALL reject stale command
and resource references") so the file complies with MD041 and MD022; insert a
leading "# <Title>" line as the first line and add/adjust surrounding blank
lines around the "##" and "###" headings to satisfy markdown-lint rules.
In `@openspec/changes/docs-13-nav-search-theme-roles/tasks.md`:
- Line 1: Add a top-level H1 heading before the existing "## 1. Data-Driven
Navigation" to satisfy MD041; update the first heading by promoting it to "# 1.
Data-Driven Navigation" (or insert a new "# Data-Driven Navigation" line
immediately above the existing "## 1. Data-Driven Navigation") so the file
starts with a single top-level heading; ensure the string "## 1. Data-Driven
Navigation" is either replaced with or preceded by the H1 to preserve the
section title.
In `@openspec/changes/docs-13-nav-search-theme-roles/TDD_EVIDENCE.md`:
- Around line 7-46: The committed TDD_EVIDENCE.md contains workstation-specific
absolute paths (e.g.,
“/home/dom/git/.../feature/docs-13-nav-search-theme-roles/docs/_config.yml”)
inside the captured command output blocks; update TDD_EVIDENCE.md to replace
those absolute paths with repo-relative paths or placeholders (for example
“<repo-root>/feature/docs-13-nav-search-theme-roles/docs/_config.yml” or
“/REPO_ROOT/...”) throughout the file, including the Jekyll build and validation
output blocks, ensuring all occurrences are sanitized while preserving the
surrounding context and output formatting.
- Around line 11-115: The TDD_EVIDENCE.md is missing the explicit failing-test
(red-phase) evidence before the passing gates; update the TDD_EVIDENCE.md by
inserting a clearly labeled failing-test block immediately before the "##
Validation commands" / before the "Final quality gates" section (or exactly
where the reviewer suggests) that shows the pre-implementation command(s), the
command(s) run, and the captured failing output (plain-text) that prompted the
change; ensure the block uses the same formatting as other entries (bash
command, Result:, and ```text``` failing output) so the sequence reads: spec
delta -> failing tests -> implementation -> passing tests.
In `@scripts/check-docs-commands.py`:
- Around line 342-357: In _validate_nav_data_links, guard the call to
yaml.compose(raw_text) against yaml.YAMLError by wrapping the compose call in a
try/except and, on parse failure, append a ValidationFinding (e.g., category
"nav-parse") for the nav_path with an informative message including the
exception text and the approximate line number (or 1) instead of letting the
exception propagate; reference the _iter_yaml_url_nodes usage so you only call
it when compose succeeds and catch yaml.YAMLError to create the failure finding.
- Around line 305-319: The function _route_for_doc generates malformed "//" for
root-level readme.md and for root-level files without extensions; fix by
explicitly detecting a root parent like the index.md case and returning "/" when
the parent is ".". For the readme branch (relative_path.name.lower() ==
"readme.md") compute parent = relative_path.parent and return "/" if str(parent)
== "." else f"/{'/'.join(parent.parts)}/". For the general fallback, compute
path_no_ext = relative_path.with_suffix('') and if str(path_no_ext.parent) ==
"." return f"/{path_no_ext.name}/" or simply "/" when the parts are empty;
otherwise join path_no_ext.parts as before to avoid producing a leading double
slash.
---
Outside diff comments:
In `@docs/_layouts/default.html`:
- Around line 210-216: The footer markup has one too many closing </div> tags:
remove the extra closing tag after the .footer-col block so the DOM tree becomes
.wrapper > .footer-col-wrapper > .footer-col (closed) then close
.footer-col-wrapper and keep the surrounding .wrapper open so .footer-links and
trademark remain inside .wrapper; locate the block containing the classes
"wrapper", "footer-col-wrapper", and "footer-col" and delete the redundant
closing </div>.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: fef75e63-54f3-4fbd-bc69-90b841f51e37
📒 Files selected for processing (138)
docs/_data/nav.ymldocs/_includes/breadcrumbs.htmldocs/_includes/expertise-filter.htmldocs/_includes/search.htmldocs/_includes/sidebar-nav.htmldocs/_includes/theme-toggle.htmldocs/_layouts/default.htmldocs/adapters/azuredevops.mddocs/adapters/backlog-adapter-patterns.mddocs/adapters/github.mddocs/assets/js/filters.jsdocs/assets/js/search-index.jsondocs/assets/js/search.jsdocs/assets/js/theme.jsdocs/assets/main.scssdocs/authoring/adapter-development.mddocs/authoring/creating-custom-bridges.mddocs/authoring/custom-registries.mddocs/authoring/extending-projectbundle.mddocs/authoring/module-development.mddocs/authoring/module-signing.mddocs/authoring/publishing-modules.mddocs/bundles/backlog/delta.mddocs/bundles/backlog/dependency-analysis.mddocs/bundles/backlog/overview.mddocs/bundles/backlog/policy-engine.mddocs/bundles/backlog/refinement.mddocs/bundles/code-review/ledger.mddocs/bundles/code-review/overview.mddocs/bundles/code-review/rules.mddocs/bundles/code-review/run.mddocs/bundles/codebase/analyze.mddocs/bundles/codebase/drift.mddocs/bundles/codebase/overview.mddocs/bundles/codebase/repro.mddocs/bundles/codebase/sidecar-validation.mddocs/bundles/govern/enforce.mddocs/bundles/govern/overview.mddocs/bundles/govern/patch.mddocs/bundles/project/devops-flow.mddocs/bundles/project/import-migration.mddocs/bundles/project/overview.mddocs/bundles/spec/generate-tests.mddocs/bundles/spec/mock.mddocs/bundles/spec/overview.mddocs/bundles/spec/validate.mddocs/getting-started/choose-your-modules.mddocs/getting-started/first-steps.mddocs/getting-started/installation.mddocs/getting-started/module-bootstrap-checklist.mddocs/getting-started/tutorial-backlog-quickstart-demo.mddocs/getting-started/tutorial-backlog-refine-ai-ide.mddocs/getting-started/tutorial-daily-standup-sprint-review.mddocs/getting-started/tutorial-openspec-speckit.mddocs/guides/agile-scrum-workflows.mddocs/guides/ai-ide-workflow.mddocs/guides/brownfield-engineer.mddocs/guides/brownfield-examples.mddocs/guides/brownfield-faq-and-roi.mddocs/guides/brownfield-faq.mddocs/guides/brownfield-journey.mddocs/guides/brownfield-modernization.mddocs/guides/brownfield-roi.mddocs/guides/ci-cd-pipeline.mddocs/guides/command-chains.mddocs/guides/common-tasks.mddocs/guides/competitive-analysis.mddocs/guides/contract-testing-workflow.mddocs/guides/copilot-mode.mddocs/guides/cross-module-chains.mddocs/guides/custom-field-mapping.mddocs/guides/daily-devops-routine.mddocs/guides/dual-stack-enrichment.mddocs/guides/ide-integration.mddocs/guides/installation.mddocs/guides/installing-modules.mddocs/guides/integrations-overview.mddocs/guides/marketplace.mddocs/guides/migration-0.16-to-0.19.mddocs/guides/migration-cli-reorganization.mddocs/guides/migration-guide.mddocs/guides/module-marketplace.mddocs/guides/openspec-journey.mddocs/guides/speckit-comparison.mddocs/guides/speckit-journey.mddocs/guides/specmatic-integration.mddocs/guides/team-collaboration-workflow.mddocs/guides/template-customization.mddocs/guides/testing-terminal-output.mddocs/guides/troubleshooting.mddocs/guides/use-cases.mddocs/guides/using-module-security-and-extensions.mddocs/guides/ux-features.mddocs/guides/workflows.mddocs/index.mddocs/integrations/devops-adapter-overview.mddocs/module-publishing-guide.mddocs/modules/code-review.mddocs/reference/README.mddocs/reference/architecture.mddocs/reference/authentication.mddocs/reference/bridge-registry.mddocs/reference/command-syntax-policy.mddocs/reference/commands.mddocs/reference/debug-logging.mddocs/reference/dependency-resolution.mddocs/reference/directory-structure.mddocs/reference/documentation-url-contract.mddocs/reference/feature-keys.mddocs/reference/modes.mddocs/reference/module-categories.mddocs/reference/module-contracts.mddocs/reference/module-security.mddocs/reference/parameter-standard.mddocs/reference/projectbundle-schema.mddocs/reference/schema-versioning.mddocs/reference/specmatic.mddocs/reference/telemetry.mddocs/reference/thorough-codebase-validation.mddocs/team-and-enterprise/agile-scrum-setup.mddocs/team-and-enterprise/enterprise-config.mddocs/team-and-enterprise/multi-repo.mddocs/team-and-enterprise/team-collaboration.mdopenspec/CHANGE_ORDER.mdopenspec/changes/docs-13-nav-search-theme-roles/.openspec.yamlopenspec/changes/docs-13-nav-search-theme-roles/TDD_EVIDENCE.mdopenspec/changes/docs-13-nav-search-theme-roles/design.mdopenspec/changes/docs-13-nav-search-theme-roles/proposal.mdopenspec/changes/docs-13-nav-search-theme-roles/specs/bundle-overview-pages/spec.mdopenspec/changes/docs-13-nav-search-theme-roles/specs/cross-module-workflow-docs/spec.mdopenspec/changes/docs-13-nav-search-theme-roles/specs/docs-client-search/spec.mdopenspec/changes/docs-13-nav-search-theme-roles/specs/docs-nav-data-driven/spec.mdopenspec/changes/docs-13-nav-search-theme-roles/specs/docs-role-expertise-nav/spec.mdopenspec/changes/docs-13-nav-search-theme-roles/specs/docs-theme-toggle/spec.mdopenspec/changes/docs-13-nav-search-theme-roles/specs/modules-docs-command-validation/spec.mdopenspec/changes/docs-13-nav-search-theme-roles/specs/team-setup-docs/spec.mdopenspec/changes/docs-13-nav-search-theme-roles/tasks.mdscripts/check-docs-commands.py
💤 Files with no reviewable changes (6)
- docs/getting-started/tutorial-openspec-speckit.md
- docs/reference/parameter-standard.md
- docs/reference/debug-logging.md
- docs/reference/feature-keys.md
- docs/guides/using-module-security-and-extensions.md
- docs/reference/directory-structure.md
There was a problem hiding this comment.
Actionable comments posted: 8
♻️ Duplicate comments (2)
docs/_includes/breadcrumbs.html (1)
5-17:⚠️ Potential issue | 🟠 MajorCurrent-page crumb detection is incorrect for repeated path segments.
Line 15 compares by segment value (
part == last_segment), so paths like/foo/bar/foo/will mark the firstfooas current. Use non-empty segment index tracking instead of value equality.Suggested fix
{% assign url_parts = page.url | split: '/' %} -{% assign last_segment = '' %} +{% assign non_empty_count = 0 %} {% for part in url_parts %} {% if part != '' %} - {% assign last_segment = part %} + {% assign non_empty_count = non_empty_count | plus: 1 %} {% endif %} {% endfor %} {% assign crumb_path = '' %} +{% assign crumb_index = 0 %} {% for part in url_parts %} {% if part != '' %} + {% assign crumb_index = crumb_index | plus: 1 %} {% assign crumb_path = crumb_path | append: '/' | append: part %} - {% if part == last_segment %} + {% if crumb_index == non_empty_count %} <span class="separator">/</span> <span class="current">{{ page.title | default: part | replace: '-', ' ' | capitalize }}</span> {% else %}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/_includes/breadcrumbs.html` around lines 5 - 17, The current comparison using last_segment causes incorrect "current" detection when segments repeat; replace value-equality with index-tracking: first iterate url_parts to compute the index of the last non-empty segment (e.g., last_index) by counting non-empty parts, then in the second loop maintain a running non-empty index (e.g., current_index) and mark the crumb as current only when current_index == last_index; update references to last_segment and the conditional in the loop that renders the current crumb (variables: url_parts, last_segment, crumb_path, page.title).docs/_layouts/default.html (1)
101-109:⚠️ Potential issue | 🟠 MajorPersist
data-sourceon converted Mermaid blocks.
DOMContentLoadedonly snapshots.mermaidnodes that already exist. WheninitMermaid()replaces a fencedlanguage-mermaidblock with a new.mermaidcontainer, that node never getsdata-source, so the first theme toggle removes the rendered SVG andrerenderMermaid()has nothing to restore.Suggested fix
var mermaidDiv = document.createElement('div'); mermaidDiv.className = 'mermaid'; - mermaidDiv.textContent = mermaidCode.trim(); + var normalizedCode = mermaidCode.trim(); + mermaidDiv.textContent = normalizedCode; + mermaidDiv.setAttribute('data-source', normalizedCode); if (pre) { pre.parentNode.replaceChild(mermaidDiv, pre); } else {Also applies to: 148-156
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/_layouts/default.html` around lines 101 - 109, When initMermaid() creates a new mermaid container from a fenced code block, it must persist the original source to data-source so rerenderMermaid() can restore the SVG on theme toggles; locate initMermaid() where it creates mermaidDiv in place of code blocks and set mermaidDiv.dataset.source (or setAttribute('data-source', ...)) to the trimmed mermaidCode before inserting it into the DOM, and apply the same change to the other conversion site referenced around the second conversion block (the similar code at the 148-156 region) so all generated .mermaid elements carry the original source.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/_includes/theme-toggle.html`:
- Around line 2-15: The two decorative SVGs (the elements with class "icon-sun"
and "icon-moon") are missing accessibility attributes; update both <svg>
elements to include aria-hidden="true" and focusable="false" so they are ignored
by assistive technologies, while keeping the existing button label/accessible
name intact (ensure the theme toggle button still has an accessible label or
aria-label elsewhere).
In `@docs/assets/js/search.js`:
- Around line 26-35: The first user query is being dropped because loadIndex is
only triggered on focus and not memoized, so doSearch runs before the index is
ready; change loadIndex so it returns and memoizes a Promise (e.g., store a
lunrIndexPromise inside loadIndex) or add a pendingQuery mechanism, then update
the input event / doSearch path to await that promise (or check for
lunrIndexPromise and .then) before performing the search; refer to loadIndex,
doSearch, and the debounced input handler and ensure the first typed query is
either queued (pendingQuery) or awaited against lunrIndexPromise so results are
produced when the initial index load completes.
- Around line 99-126: The current listbox semantics mismatch: either remove the
listbox role from the container or implement listbox/option ARIA properly; to
fix, decide which approach you want and apply changes in createResultLink and
updateHighlight: if keeping role="listbox" on .docs-search-results, set
role="option" (or role="option" on a child wrapper) and a stable id for each
item in createResultLink, ensure updateHighlight sets
aria-selected="true"/"false" on options and updates the container's
aria-activedescendant to the active item's id; if removing listbox semantics,
remove role="listbox" from the container and do not add ARIA option attributes
in createResultLink or updateHighlight so items remain plain links for screen
readers.
In `@docs/assets/main.scss`:
- Around line 39-145: The file repeats the same CSS custom properties across
:root, [data-theme="dark"], [data-theme="light"], and the prefers-color-scheme
fallbacks; replace the duplication by defining SCSS maps (e.g., $theme-dark,
$theme-light) containing the variable names and values, add a mixin (e.g.,
`@mixin` apply-theme($theme)) that iterates the map and emits --<name>: <value>
custom properties, then call that mixin for [data-theme="dark"],
[data-theme="light"], and the two :root fallbacks and ensure you still set the
color-scheme property appropriately for each emitted block.
- Line 523: Add a short explanatory comment above the CSS rule containing
"min-height: calc(100vh - 8rem);" documenting that the 8rem represents the
combined header and footer height (e.g., header + footer), and if your SCSS has
variables for those heights (e.g., $header-height, $footer-height) prefer
replacing the literal 8rem with "calc(100vh - (#{$header-height} +
#{$footer-height}))" or equivalent to avoid the magic number; ensure the comment
names the components (header/footer) and references the "min-height: calc(100vh
- 8rem);" declaration so future maintainers understand the value's origin.
- Around line 245-249: Add a defensive default so the theme toggle shows the
dark-theme icon when no data-theme is present: set the base selectors
.theme-toggle .icon-sun to display: none and .theme-toggle .icon-moon to
display: block, and keep the existing [data-theme="light"] and
[data-theme="dark"] rules to override that default (refer to the .theme-toggle
.icon-sun and .theme-toggle .icon-moon selectors and the [data-theme="light"] /
[data-theme="dark"] rules in the diff).
In
`@openspec/changes/docs-13-nav-search-theme-roles/specs/docs-client-search/spec.md`:
- Around line 19-21: The spec's requirement for the sidebar search placeholder
and shortcut rendering is out of sync with the shipped include; update either
the spec text in specs/docs-client-search/spec.md or the include template
docs/_includes/search.html to match. Specifically, either change the requirement
to state that the input uses placeholder="Search docs..." and the "Ctrl+K"
shortcut is rendered separately (referencing the search input element and the
shortcut element in docs/_includes/search.html), or modify
docs/_includes/search.html so the input placeholder equals "Search docs...
(Ctrl+K)". Make the change so the requirement and the implementation are
identical and the spec remains testable.
In `@scripts/check-docs-commands.py`:
- Around line 143-153: The _extract_front_matter function currently lets
yaml.safe_load raise and crash the validator; wrap the yaml.safe_load call in a
try/except that catches yaml.YAMLError (and optionally Exception), and on parse
failure return a dict signaling invalid front matter (e.g.
{"_invalid_front_matter_error": "<error message>"} or similar) instead of
propagating the exception so _build_valid_docs_routes can report a targeted
validation error for that file; update the return path after the try/except to
ensure only dicts are returned (keep the existing isinstance check) and include
the YAML error string so callers can log or report the specific parse issue.
---
Duplicate comments:
In `@docs/_includes/breadcrumbs.html`:
- Around line 5-17: The current comparison using last_segment causes incorrect
"current" detection when segments repeat; replace value-equality with
index-tracking: first iterate url_parts to compute the index of the last
non-empty segment (e.g., last_index) by counting non-empty parts, then in the
second loop maintain a running non-empty index (e.g., current_index) and mark
the crumb as current only when current_index == last_index; update references to
last_segment and the conditional in the loop that renders the current crumb
(variables: url_parts, last_segment, crumb_path, page.title).
In `@docs/_layouts/default.html`:
- Around line 101-109: When initMermaid() creates a new mermaid container from a
fenced code block, it must persist the original source to data-source so
rerenderMermaid() can restore the SVG on theme toggles; locate initMermaid()
where it creates mermaidDiv in place of code blocks and set
mermaidDiv.dataset.source (or setAttribute('data-source', ...)) to the trimmed
mermaidCode before inserting it into the DOM, and apply the same change to the
other conversion site referenced around the second conversion block (the similar
code at the 148-156 region) so all generated .mermaid elements carry the
original source.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 1931d566-ee39-4fa3-88dc-abfdafc2a4a2
📒 Files selected for processing (21)
docs/_includes/breadcrumbs.htmldocs/_includes/expertise-filter.htmldocs/_includes/theme-toggle.htmldocs/_layouts/default.htmldocs/assets/js/filters.jsdocs/assets/js/search.jsdocs/assets/main.scssdocs/guides/brownfield-engineer.mddocs/guides/brownfield-faq.mddocs/guides/brownfield-journey.mddocs/guides/brownfield-roi.mddocs/guides/common-tasks.mddocs/guides/ide-integration.mddocs/index.mdopenspec/changes/docs-13-nav-search-theme-roles/TDD_EVIDENCE.mdopenspec/changes/docs-13-nav-search-theme-roles/specs/cross-module-workflow-docs/spec.mdopenspec/changes/docs-13-nav-search-theme-roles/specs/docs-client-search/spec.mdopenspec/changes/docs-13-nav-search-theme-roles/specs/docs-nav-data-driven/spec.mdopenspec/changes/docs-13-nav-search-theme-roles/specs/modules-docs-command-validation/spec.mdopenspec/changes/docs-13-nav-search-theme-roles/tasks.mdscripts/check-docs-commands.py
💤 Files with no reviewable changes (4)
- docs/guides/brownfield-engineer.md
- docs/guides/brownfield-journey.md
- docs/guides/brownfield-faq.md
- docs/guides/brownfield-roi.md
Comment on lines
+2
to
+15
| <svg class="icon-sun" xmlns="http://www.w3.org/2000/svg" width="18" height="18" viewBox="0 0 24 24" fill="none" stroke="currentColor" stroke-width="2" stroke-linecap="round" stroke-linejoin="round"> | ||
| <circle cx="12" cy="12" r="5"></circle> | ||
| <line x1="12" y1="1" x2="12" y2="3"></line> | ||
| <line x1="12" y1="21" x2="12" y2="23"></line> | ||
| <line x1="4.22" y1="4.22" x2="5.64" y2="5.64"></line> | ||
| <line x1="18.36" y1="18.36" x2="19.78" y2="19.78"></line> | ||
| <line x1="1" y1="12" x2="3" y2="12"></line> | ||
| <line x1="21" y1="12" x2="23" y2="12"></line> | ||
| <line x1="4.22" y1="19.78" x2="5.64" y2="18.36"></line> | ||
| <line x1="18.36" y1="5.64" x2="19.78" y2="4.22"></line> | ||
| </svg> | ||
| <svg class="icon-moon" xmlns="http://www.w3.org/2000/svg" width="18" height="18" viewBox="0 0 24 24" fill="none" stroke="currentColor" stroke-width="2" stroke-linecap="round" stroke-linejoin="round"> | ||
| <path d="M21 12.79A9 9 0 1 1 11.21 3 7 7 0 0 0 21 12.79z"></path> | ||
| </svg> |
There was a problem hiding this comment.
Hide decorative theme icons from screen readers.
Line 2 and Line 13 SVGs are purely visual; add aria-hidden="true" (and focusable="false" for broader compatibility) so assistive tech reads only the button label.
Suggested fix
- <svg class="icon-sun" xmlns="http://www.w3.org/2000/svg" width="18" height="18" viewBox="0 0 24 24" fill="none" stroke="currentColor" stroke-width="2" stroke-linecap="round" stroke-linejoin="round">
+ <svg class="icon-sun" aria-hidden="true" focusable="false" xmlns="http://www.w3.org/2000/svg" width="18" height="18" viewBox="0 0 24 24" fill="none" stroke="currentColor" stroke-width="2" stroke-linecap="round" stroke-linejoin="round">
@@
- <svg class="icon-moon" xmlns="http://www.w3.org/2000/svg" width="18" height="18" viewBox="0 0 24 24" fill="none" stroke="currentColor" stroke-width="2" stroke-linecap="round" stroke-linejoin="round">
+ <svg class="icon-moon" aria-hidden="true" focusable="false" xmlns="http://www.w3.org/2000/svg" width="18" height="18" viewBox="0 0 24 24" fill="none" stroke="currentColor" stroke-width="2" stroke-linecap="round" stroke-linejoin="round">📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| <svg class="icon-sun" xmlns="http://www.w3.org/2000/svg" width="18" height="18" viewBox="0 0 24 24" fill="none" stroke="currentColor" stroke-width="2" stroke-linecap="round" stroke-linejoin="round"> | |
| <circle cx="12" cy="12" r="5"></circle> | |
| <line x1="12" y1="1" x2="12" y2="3"></line> | |
| <line x1="12" y1="21" x2="12" y2="23"></line> | |
| <line x1="4.22" y1="4.22" x2="5.64" y2="5.64"></line> | |
| <line x1="18.36" y1="18.36" x2="19.78" y2="19.78"></line> | |
| <line x1="1" y1="12" x2="3" y2="12"></line> | |
| <line x1="21" y1="12" x2="23" y2="12"></line> | |
| <line x1="4.22" y1="19.78" x2="5.64" y2="18.36"></line> | |
| <line x1="18.36" y1="5.64" x2="19.78" y2="4.22"></line> | |
| </svg> | |
| <svg class="icon-moon" xmlns="http://www.w3.org/2000/svg" width="18" height="18" viewBox="0 0 24 24" fill="none" stroke="currentColor" stroke-width="2" stroke-linecap="round" stroke-linejoin="round"> | |
| <path d="M21 12.79A9 9 0 1 1 11.21 3 7 7 0 0 0 21 12.79z"></path> | |
| </svg> | |
| <svg class="icon-sun" aria-hidden="true" focusable="false" xmlns="http://www.w3.org/2000/svg" width="18" height="18" viewBox="0 0 24 24" fill="none" stroke="currentColor" stroke-width="2" stroke-linecap="round" stroke-linejoin="round"> | |
| <circle cx="12" cy="12" r="5"></circle> | |
| <line x1="12" y1="1" x2="12" y2="3"></line> | |
| <line x1="12" y1="21" x2="12" y2="23"></line> | |
| <line x1="4.22" y1="4.22" x2="5.64" y2="5.64"></line> | |
| <line x1="18.36" y1="18.36" x2="19.78" y2="19.78"></line> | |
| <line x1="1" y1="12" x2="3" y2="12"></line> | |
| <line x1="21" y1="12" x2="23" y2="12"></line> | |
| <line x1="4.22" y1="19.78" x2="5.64" y2="18.36"></line> | |
| <line x1="18.36" y1="5.64" x2="19.78" y2="4.22"></line> | |
| </svg> | |
| <svg class="icon-moon" aria-hidden="true" focusable="false" xmlns="http://www.w3.org/2000/svg" width="18" height="18" viewBox="0 0 24 24" fill="none" stroke="currentColor" stroke-width="2" stroke-linecap="round" stroke-linejoin="round"> | |
| <path d="M21 12.79A9 9 0 1 1 11.21 3 7 7 0 0 0 21 12.79z"></path> | |
| </svg> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/_includes/theme-toggle.html` around lines 2 - 15, The two decorative
SVGs (the elements with class "icon-sun" and "icon-moon") are missing
accessibility attributes; update both <svg> elements to include
aria-hidden="true" and focusable="false" so they are ignored by assistive
technologies, while keeping the existing button label/accessible name intact
(ensure the theme toggle button still has an accessible label or aria-label
elsewhere).
Comment on lines
+26
to
+35
| function loadIndex() { | ||
| if (lunrIndex) return Promise.resolve(); | ||
| if (typeof lunr === 'undefined') { | ||
| console.error('SpecFact docs search could not start because lunr is unavailable.'); | ||
| setSearchState('Search is temporarily unavailable.'); | ||
| return Promise.resolve(); | ||
| } | ||
|
|
||
| var indexUrl = searchRoot.getAttribute('data-search-index-url') || '/assets/js/search-index.json'; | ||
| return fetch(indexUrl) |
There was a problem hiding this comment.
Don't drop the first query while the index is still loading.
The debounced input path calls doSearch(query) immediately, but the initial fetch/build only starts on focus and isn't memoized. If the user finishes typing before that first load resolves, the query is discarded and results stay empty until another keypress.
Suggested fix
var lunrIndex = null;
var searchData = null;
var debounceTimer = null;
var highlightedIndex = -1;
+ var indexLoadPromise = null;
function loadIndex() {
if (lunrIndex) return Promise.resolve();
+ if (indexLoadPromise) return indexLoadPromise;
if (typeof lunr === 'undefined') {
console.error('SpecFact docs search could not start because lunr is unavailable.');
setSearchState('Search is temporarily unavailable.');
return Promise.resolve();
}
var indexUrl = searchRoot.getAttribute('data-search-index-url') || '/assets/js/search-index.json';
- return fetch(indexUrl)
+ indexLoadPromise = fetch(indexUrl)
.catch(function(error) {
console.error('SpecFact docs search index fetch failed for ' + indexUrl + '.', error);
setSearchState('Search is temporarily unavailable.');
lunrIndex = null;
searchData = null;
@@
.catch(function(error) {
console.error('SpecFact docs search initialization failed for ' + indexUrl + '.', error);
setSearchState('Search is temporarily unavailable.');
lunrIndex = null;
searchData = null;
+ })
+ .finally(function() {
+ if (!lunrIndex) {
+ indexLoadPromise = null;
+ }
});
+ return indexLoadPromise;
}
@@
clearTimeout(debounceTimer);
var query = searchInput.value.trim();
debounceTimer = setTimeout(function() {
- doSearch(query);
+ if (query.length < 2 || lunrIndex) {
+ doSearch(query);
+ return;
+ }
+ loadIndex().then(function() {
+ if (lunrIndex) {
+ doSearch(query);
+ }
+ });
}, 150);
});Also applies to: 182-187
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/assets/js/search.js` around lines 26 - 35, The first user query is being
dropped because loadIndex is only triggered on focus and not memoized, so
doSearch runs before the index is ready; change loadIndex so it returns and
memoizes a Promise (e.g., store a lunrIndexPromise inside loadIndex) or add a
pendingQuery mechanism, then update the input event / doSearch path to await
that promise (or check for lunrIndexPromise and .then) before performing the
search; refer to loadIndex, doSearch, and the debounced input handler and ensure
the first typed query is either queued (pendingQuery) or awaited against
lunrIndexPromise so results are produced when the initial index load completes.
Comment on lines
+99
to
+126
| function createResultLink(doc) { | ||
| var link = document.createElement('a'); | ||
| link.className = 'docs-search-result'; | ||
| link.href = isSafeRelativeUrl(doc.url) ? doc.url : '/'; | ||
|
|
||
| var titleEl = document.createElement('div'); | ||
| titleEl.className = 'result-title'; | ||
| titleEl.textContent = doc.title || ''; | ||
| link.appendChild(titleEl); | ||
|
|
||
| var snippetEl = document.createElement('div'); | ||
| snippetEl.className = 'result-snippet'; | ||
| snippetEl.textContent = ((doc.content || '').substring(0, 120) + '...').trim(); | ||
| link.appendChild(snippetEl); | ||
|
|
||
| var audience = Array.isArray(doc.audience) ? doc.audience : []; | ||
| var expertise = Array.isArray(doc.expertise_level) ? doc.expertise_level : []; | ||
| var tags = audience.concat(expertise).filter(Boolean); | ||
| if (tags.length) { | ||
| var tagsEl = document.createElement('div'); | ||
| tagsEl.className = 'result-tags'; | ||
| tags.forEach(function(tag) { | ||
| tagsEl.appendChild(createTag(tag)); | ||
| }); | ||
| link.appendChild(tagsEl); | ||
| } | ||
|
|
||
| return link; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "search.html" -type f | head -20Repository: nold-ai/specfact-cli-modules
Length of output: 100
🏁 Script executed:
cd docs && find . -name "search.html" -o -name "search.js" | head -20Repository: nold-ai/specfact-cli-modules
Length of output: 117
🏁 Script executed:
git ls-files | grep -E "(search\.(js|html)|_includes)" | head -20Repository: nold-ai/specfact-cli-modules
Length of output: 257
🏁 Script executed:
cat -n docs/_includes/search.htmlRepository: nold-ai/specfact-cli-modules
Length of output: 434
🏁 Script executed:
cat -n docs/assets/js/search.jsRepository: nold-ai/specfact-cli-modules
Length of output: 8517
Implement proper listbox ARIA semantics or remove the listbox role.
.docs-search-results has role="listbox", but children are plain <a> elements without role="option", aria-selected, or corresponding updates to aria-activedescendant on the listbox container. The updateHighlight() function (lines 170–174) only toggles CSS; it does not update ARIA attributes. Screen reader users will not receive proper state information about which option is active.
Either implement the full listbox pattern (role="option" on items, aria-selected, and aria-activedescendant on the container) or remove role="listbox" and keep this as a plain link list.
Also applies to: 170-174
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/assets/js/search.js` around lines 99 - 126, The current listbox
semantics mismatch: either remove the listbox role from the container or
implement listbox/option ARIA properly; to fix, decide which approach you want
and apply changes in createResultLink and updateHighlight: if keeping
role="listbox" on .docs-search-results, set role="option" (or role="option" on a
child wrapper) and a stable id for each item in createResultLink, ensure
updateHighlight sets aria-selected="true"/"false" on options and updates the
container's aria-activedescendant to the active item's id; if removing listbox
semantics, remove role="listbox" from the container and do not add ARIA option
attributes in createResultLink or updateHighlight so items remain plain links
for screen readers.
Comment on lines
+39
to
+145
| [data-theme="dark"] { | ||
| --primary-color: #57e6c4; | ||
| --primary-hover: #6eefd3; | ||
| --text-color: #ccd6f6; | ||
| --text-light: #8892b0; | ||
| --text-muted: #495670; | ||
| --bg-color: #0a192f; | ||
| --bg-light: #112240; | ||
| --bg-alt: #1d2d50; | ||
| --border-color: rgba(100, 255, 218, 0.1); | ||
| --border-hover: rgba(100, 255, 218, 0.3); | ||
| --border-color: rgba(87, 230, 196, 0.12); | ||
| --border-hover: rgba(87, 230, 196, 0.3); | ||
| --code-bg: #1d2d50; | ||
| --link-color: #64ffda; | ||
| --link-hover: #7affeb; | ||
| --link-color: #57e6c4; | ||
| --link-hover: #6eefd3; | ||
| --search-bg: #1d2d50; | ||
| --search-border: rgba(87, 230, 196, 0.2); | ||
| --card-bg: #112240; | ||
| --card-border: rgba(87, 230, 196, 0.15); | ||
| --breadcrumb-color: #8892b0; | ||
| --badge-bg: rgba(87, 230, 196, 0.12); | ||
| --badge-color: #57e6c4; | ||
| --filter-bg: #1d2d50; | ||
| color-scheme: dark; | ||
| } | ||
|
|
||
| // ============================================================================ | ||
| // Light theme | ||
| // ============================================================================ | ||
| [data-theme="light"] { | ||
| --primary-color: #0d9488; | ||
| --primary-hover: #0f766e; | ||
| --text-color: #1a1a2e; | ||
| --text-light: #475569; | ||
| --text-muted: #94a3b8; | ||
| --bg-color: #ffffff; | ||
| --bg-light: #f8fafc; | ||
| --bg-alt: #f1f5f9; | ||
| --border-color: #e2e8f0; | ||
| --border-hover: #cbd5e1; | ||
| --code-bg: #f1f5f9; | ||
| --link-color: #0d9488; | ||
| --link-hover: #0f766e; | ||
| --search-bg: #ffffff; | ||
| --search-border: #cbd5e1; | ||
| --card-bg: #f8fafc; | ||
| --card-border: #e2e8f0; | ||
| --breadcrumb-color: #64748b; | ||
| --badge-bg: #f0fdfa; | ||
| --badge-color: #0d9488; | ||
| --filter-bg: #f1f5f9; | ||
| color-scheme: light; | ||
| } | ||
|
|
||
| // System preference fallback (when no data-theme attribute) | ||
| @media (prefers-color-scheme: light) { | ||
| :root:not([data-theme]) { | ||
| --primary-color: #0d9488; | ||
| --primary-hover: #0f766e; | ||
| --text-color: #1a1a2e; | ||
| --text-light: #475569; | ||
| --text-muted: #94a3b8; | ||
| --bg-color: #ffffff; | ||
| --bg-light: #f8fafc; | ||
| --bg-alt: #f1f5f9; | ||
| --border-color: #e2e8f0; | ||
| --border-hover: #cbd5e1; | ||
| --code-bg: #f1f5f9; | ||
| --link-color: #0d9488; | ||
| --link-hover: #0f766e; | ||
| --search-bg: #ffffff; | ||
| --search-border: #cbd5e1; | ||
| --card-bg: #f8fafc; | ||
| --card-border: #e2e8f0; | ||
| --breadcrumb-color: #64748b; | ||
| --badge-bg: #f0fdfa; | ||
| --badge-color: #0d9488; | ||
| --filter-bg: #f1f5f9; | ||
| color-scheme: light; | ||
| } | ||
| } | ||
|
|
||
| @media (prefers-color-scheme: dark) { | ||
| :root:not([data-theme]) { | ||
| --primary-color: #57e6c4; | ||
| --primary-hover: #6eefd3; | ||
| --text-color: #ccd6f6; | ||
| --text-light: #8892b0; | ||
| --text-muted: #495670; | ||
| --bg-color: #0a192f; | ||
| --bg-light: #112240; | ||
| --bg-alt: #1d2d50; | ||
| --border-color: rgba(87, 230, 196, 0.12); | ||
| --border-hover: rgba(87, 230, 196, 0.3); | ||
| --code-bg: #1d2d50; | ||
| --link-color: #57e6c4; | ||
| --link-hover: #6eefd3; | ||
| --search-bg: #1d2d50; | ||
| --search-border: rgba(87, 230, 196, 0.2); | ||
| --card-bg: #112240; | ||
| --card-border: rgba(87, 230, 196, 0.15); | ||
| --breadcrumb-color: #8892b0; | ||
| --badge-bg: rgba(87, 230, 196, 0.12); | ||
| --badge-color: #57e6c4; | ||
| --filter-bg: #1d2d50; | ||
| color-scheme: dark; | ||
| } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider using SCSS maps to reduce theme variable duplication.
The same ~20 CSS custom properties are defined four times (:root, [data-theme="dark"], [data-theme="light"], and both prefers-color-scheme fallbacks). While functionally correct, this makes future palette changes error-prone.
♻️ Optional refactor using SCSS maps
// Define theme palettes as SCSS maps
$theme-dark: (
primary-color: `#57e6c4`,
primary-hover: `#6eefd3`,
text-color: `#ccd6f6`,
// ... remaining variables
);
$theme-light: (
primary-color: `#0d9488`,
primary-hover: `#0f766e`,
text-color: `#1a1a2e`,
// ... remaining variables
);
// Mixin to emit CSS custom properties from a map
`@mixin` apply-theme($theme) {
`@each` $key, $value in $theme {
--#{$key}: #{$value};
}
}
// Usage
:root { `@include` apply-theme($theme-dark); color-scheme: dark; }
[data-theme="dark"] { `@include` apply-theme($theme-dark); color-scheme: dark; }
[data-theme="light"] { `@include` apply-theme($theme-light); color-scheme: light; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/assets/main.scss` around lines 39 - 145, The file repeats the same CSS
custom properties across :root, [data-theme="dark"], [data-theme="light"], and
the prefers-color-scheme fallbacks; replace the duplication by defining SCSS
maps (e.g., $theme-dark, $theme-light) containing the variable names and values,
add a mixin (e.g., `@mixin` apply-theme($theme)) that iterates the map and emits
--<name>: <value> custom properties, then call that mixin for
[data-theme="dark"], [data-theme="light"], and the two :root fallbacks and
ensure you still set the color-scheme property appropriately for each emitted
block.
Comment on lines
+245
to
+249
| // Show correct icon per theme | ||
| [data-theme="dark"] .theme-toggle .icon-sun { display: none; } | ||
| [data-theme="dark"] .theme-toggle .icon-moon { display: block; } | ||
| [data-theme="light"] .theme-toggle .icon-sun { display: block; } | ||
| [data-theme="light"] .theme-toggle .icon-moon { display: none; } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Add fallback for theme toggle icons when data-theme is unset.
If theme.js fails to load or execute, neither [data-theme] selector matches and both icons could be visible. Since :root defaults to dark, hide .icon-sun by default.
🛡️ Suggested defensive fallback
+// Default state (matches baseline dark theme)
+.theme-toggle .icon-sun { display: none; }
+.theme-toggle .icon-moon { display: block; }
+
// Show correct icon per theme
[data-theme="dark"] .theme-toggle .icon-sun { display: none; }
[data-theme="dark"] .theme-toggle .icon-moon { display: block; }
[data-theme="light"] .theme-toggle .icon-sun { display: block; }
[data-theme="light"] .theme-toggle .icon-moon { display: none; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/assets/main.scss` around lines 245 - 249, Add a defensive default so the
theme toggle shows the dark-theme icon when no data-theme is present: set the
base selectors .theme-toggle .icon-sun to display: none and .theme-toggle
.icon-moon to display: block, and keep the existing [data-theme="light"] and
[data-theme="dark"] rules to override that default (refer to the .theme-toggle
.icon-sun and .theme-toggle .icon-moon selectors and the [data-theme="light"] /
[data-theme="dark"] rules in the diff).
| align-items: flex-start; | ||
| gap: 0; | ||
| align-items: stretch; | ||
| min-height: calc(100vh - 8rem); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Document the magic number in min-height calculation.
The 8rem value likely represents combined header and footer heights. A brief comment aids future maintainers.
- min-height: calc(100vh - 8rem);
+ min-height: calc(100vh - 8rem); // header (~4rem) + footer (~4rem)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| min-height: calc(100vh - 8rem); | |
| min-height: calc(100vh - 8rem); // header (~4rem) + footer (~4rem) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/assets/main.scss` at line 523, Add a short explanatory comment above the
CSS rule containing "min-height: calc(100vh - 8rem);" documenting that the 8rem
represents the combined header and footer height (e.g., header + footer), and if
your SCSS has variables for those heights (e.g., $header-height, $footer-height)
prefer replacing the literal 8rem with "calc(100vh - (#{$header-height} +
#{$footer-height}))" or equivalent to avoid the magic number; ensure the comment
names the components (header/footer) and references the "min-height: calc(100vh
- 8rem);" declaration so future maintainers understand the value's origin.
Comment on lines
+19
to
+21
| ### Requirement: Search UI in sidebar | ||
|
|
||
| A search input field SHALL be rendered in the sidebar above the navigation sections via `docs/_includes/search.html`. The search input SHALL have placeholder text "Search docs... (Ctrl+K)". |
There was a problem hiding this comment.
This requirement is already out of sync with the shipped include.
docs/_includes/search.html currently renders placeholder="Search docs..." and shows Ctrl+K in a separate shortcut element, so this SHALL does not match the implementation in the same change. Please update either the requirement text or the include so the spec stays testable.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@openspec/changes/docs-13-nav-search-theme-roles/specs/docs-client-search/spec.md`
around lines 19 - 21, The spec's requirement for the sidebar search placeholder
and shortcut rendering is out of sync with the shipped include; update either
the spec text in specs/docs-client-search/spec.md or the include template
docs/_includes/search.html to match. Specifically, either change the requirement
to state that the input uses placeholder="Search docs..." and the "Ctrl+K"
shortcut is rendered separately (referencing the search input element and the
shortcut element in docs/_includes/search.html), or modify
docs/_includes/search.html so the input placeholder equals "Search docs...
(Ctrl+K)". Make the change so the requirement and the implementation are
identical and the spec remains testable.
Comment on lines
+143
to
+153
| def _extract_front_matter(text: str) -> dict[str, object]: | ||
| lines = text.splitlines() | ||
| if not lines or lines[0].strip() != YAML_FRONT_MATTER_DELIMITER: | ||
| return {} | ||
|
|
||
| for index in range(1, len(lines)): | ||
| if lines[index].strip() != YAML_FRONT_MATTER_DELIMITER: | ||
| continue | ||
| data = yaml.safe_load("\n".join(lines[1:index])) or {} | ||
| return data if isinstance(data, dict) else {} | ||
| return {} |
There was a problem hiding this comment.
Report invalid front matter instead of crashing the validator.
A malformed YAML header will raise out of _extract_front_matter() here and abort _build_valid_docs_routes() with a traceback. That means one bad page suppresses the rest of this script's findings instead of producing a targeted validation error for the offending file.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/check-docs-commands.py` around lines 143 - 153, The
_extract_front_matter function currently lets yaml.safe_load raise and crash the
validator; wrap the yaml.safe_load call in a try/except that catches
yaml.YAMLError (and optionally Exception), and on parse failure return a dict
signaling invalid front matter (e.g. {"_invalid_front_matter_error": "<error
message>"} or similar) instead of propagating the exception so
_build_valid_docs_routes can report a targeted validation error for that file;
update the return path after the try/except to ensure only dicts are returned
(keep the existing isinstance check) and include the YAML error string so
callers can log or report the specific parse issue.
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
1 participant
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
Overhauls
modules.specfact.ionavigation and page shell for thedocs-13scope. This replaces the stale hardcoded sidebar with data-driven navigation, adds client-side search, expertise filtering, persistent dark/light theme support, refreshed header/footer treatment, breadcrumbs, and role-based landing-page entry points. It also enriches page metadata, restores the retired/guides/ide-integration/route as a redirect, and records the matching OpenSpec change and validation evidence in-branch.Refs:
Scope
packages/registry/index.json,packages/*/module-package.yaml).github/workflows/*)docs/*,README.md,AGENTS.md)scripts/sign-modules.py,scripts/verify-modules-signature.py)Bundle Impact
List impacted bundles and version updates:
nold-ai/specfact-project:no changenold-ai/specfact-backlog:no changenold-ai/specfact-codebase:no changenold-ai/specfact-spec:no changenold-ai/specfact-govern:no changeValidation Evidence
Local validation completed on the feature branch after restoring the branch-local OpenSpec change and TDD evidence.
Required local gates
hatch run formathatch run type-checkhatch run linthatch run yaml-linthatch run check-bundle-importshatch run contract-testhatch run smart-test(orhatch run test)Signature + version integrity (required)
hatch run verify-modules-signature --require-signature --payload-from-filesystem --enforce-version-bumpValidation notes:
contract-test:462 passed, 1 warningsmart-test:462 passed, 1 warningtest:462 passed, 1 warningopenspec validate docs-13-nav-search-theme-roles --strict: passedpython3 scripts/check-docs-commands.py: passedcd docs && bundle exec jekyll build: passeddocs/adapters/anddocs/reference/CI and Branch Protection
verify-module-signaturesquality (3.11)quality (3.12)quality (3.13)Docs / Pages
docs/)docs-pages.yml, if changed)specfact-clidocs updated (if applicable)Checklist