Reject duplicate docs IDs - #404
Conversation
📝 WalkthroughWalkthroughThe PR separates chart-factory and capability headings, adds duplicate-ID validation for rendered documentation components and prerendered HTML routes, and adds regression tests for content and redirect pages. ChangesDocumentation validation
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant PublicPage
participant DocumentationRenderer
participant IDValidator
participant HTMLRouteValidator
PublicPage->>DocumentationRenderer: render documentation components
DocumentationRenderer->>IDValidator: provide component tree
IDValidator-->>PublicPage: report duplicate literal IDs
PublicPage->>HTMLRouteValidator: provide prerendered HTML
HTMLRouteValidator-->>PublicPage: report duplicate element IDs
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Merging this PR will not alter performance
Comparing Footnotes
|
There was a problem hiding this comment.
All reported issues were addressed across 7 files
Reply with feedback, questions, or to request a fix.
Fix all with cubic | Re-trigger cubic
Greptile SummaryThe PR disambiguates documentation headings and chart-factory groupings, then adds duplicate static-ID validation across prerendered public and redirect routes.
Confidence Score: 4/5The PR appears safe to merge, with the non-blocking concern that its new documentation build gate lacks focused regression coverage. The heading and grouping updates remain internally consistent, while the duplicate-ID validator is applied broadly and would benefit from tests pinning accepted and rejected HTML behavior. Files Needing Attention: docs/app/scripts/check_html_routes.py
|
| Filename | Overview |
|---|---|
| docs/app/scripts/check_html_routes.py | Adds whole-document duplicate-ID parsing and applies it to documentation and redirect routes; the behavior lacks focused regression tests. |
| docs/app/xy_docs/api_reference.py | Separates chart() and facet_chart() into uniquely named groups while preserving both in the documented factory set. |
| scripts/gen_capability_matrix.py | Generates distinct note headings consistently for both specification and public capability documents. |
| docs/api-reference/chart-factories.md | Updates authored prose and the chart-container heading to match the regrouped generated API reference. |
| docs/styling/capabilities.md | Updates public capability headings to avoid repeated static IDs. |
| spec/api/capability-matrix.md | Keeps the generated specification synchronized with the generator’s distinct headings. |
Reviews (1): Last reviewed commit: "Trim duplicate-ID validation" | Re-trigger Greptile
|
|
||
| def duplicate_html_ids(source: str) -> tuple[str, ...]: | ||
| """Return duplicate element IDs from a complete prerendered document.""" | ||
| parser = _ElementIdParser() | ||
| parser.feed(source) | ||
| parser.close() |
There was a problem hiding this comment.
Duplicate-ID gate lacks focused tests
The new parser and build-failing validation path have no focused positive and negative tests, so changes to empty-ID handling, inline SVG parsing, or duplicate reporting can either reject valid documentation output or allow ambiguous anchors without a targeted failure identifying the regression.
Knowledge Base Used: Testing and Benchmarks
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
There was a problem hiding this comment.
1 issue found across 3 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="docs/app/tests/test_docs_site.py">
<violation number="1">
P2: This delta removes the only two tests covering the duplicate element-ID validation in check_html_routes.main(): test_prerendered_route_validator_rejects_ids_from_layout_and_raw_svg and test_redirect_route_validator_rejects_duplicate_ids. The production validator is still fully active — main() calls validate_unique_html_ids for both content routes and DOCS_REDIRECTS — so the core feature of this PR (rejecting duplicate docs IDs, including the redirect branch previously added for the fallback route) is left completely untested. Consider keeping at least the redirect integration test, or relocating both tests into a dedicated validator test module rather than dropping coverage.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Fix all with cubic | Re-trigger cubic
What changed
chart()only once.Notessections in both the generator and generated docs/spec output.Why
Authored and generated headings collided on Chart Factories, while repeated
Notesheadings collided on the Capability Matrix. Duplicate IDs make anchor links and generated navigation ambiguous.Impact
Public docs routes now reject duplicate IDs during the existing post-build route validation without adding a separate source checker or dedicated test suite.
Validation
docs/app/.venv/bin/pytest -q docs/app/tests— 103 passed, 1 xfaileddocs/app/.venv/bin/ruff check .docs/app/.venv/bin/ruff format --check .docs/app/.venv/bin/codespell .docs/app/.venv/bin/python scripts/gen_capability_matrix.py --checkFixes #396