Test docs index-file fallback - #1072
Conversation
The docs path manifest canonicalizes `<path>/index.md` files to `<path>` (getCanonicalDocsPath strips the trailing `/index`), so the route resolver answers 'render <path>' for pages like pacer's Core API Reference (docs/reference/index.md). Loading then fetched `<path>.md`, which does not exist, so every root-level `reference/index` page 404ed and the framework-scoped ones bounced to the framework overview instead of rendering. Retry `<path>/index.md` when `<path>.md` is missing, before falling back to frontmatter redirects and not-found. Fetchers are injectable so the route pipeline is testable without GitHub network/cache, mirroring collectRedirectEntriesForFile. Reported in TanStack/pacer#237.
Covers, with pacer's real docs layout (TanStack/pacer#237): root-level and framework-scoped reference/index pages loading at their canonical paths, plain docs loading without an extra fallback fetch, genuinely missing paths still resolving to not-found, and the existing section-index redirect staying intact.
📝 WalkthroughWalkthroughChangesDocs loading now accepts an optional injected document fetcher and retries missing document paths with Docs loading and route resolution
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
tests/docs-route-index-fallback.test.ts (1)
29-61: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winConsider a test for non-404 errors propagating instead of being swallowed.
All current tests exercise the
{isNotFound: true}path. Nothing verifies that a genuine error (e.g. a network failure) fromfetchDocsduring the index-fallback attempt still propagates viathrow errorinloadDocsIndexFile/loadDocsRouterather than being treated as "not found". A regression that widens theisDocsNotFoundErrorcheck would silently swallow real errors and this suite wouldn't catch it.Example addition
test('non-404 errors from fetchDocs are not swallowed by the index fallback', async () => { const { fetchers } = createFetchers() const boomFetchers: LoadDocsRouteFetchers = { ...fetchers, fetchDocs: async () => { throw new Error('boom') }, } await assert.rejects(() => loadRoute('reference', boomFetchers), /boom/) })🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/docs-route-index-fallback.test.ts` around lines 29 - 61, Add a test alongside the existing index-fallback tests that overrides createFetchers().fetchDocs to throw a genuine Error, invokes loadRoute for the fallback scenario, and asserts the error is rejected with its original message. Ensure the test verifies non-404 failures propagate through loadDocsIndexFile/loadDocsRoute rather than being treated as missing documents.src/utils/docs.ts (1)
211-246: 🚀 Performance & Scalability | 🔵 TrivialIndex-fallback logic is correct but adds a guaranteed extra round-trip for every canonical index page.
For any resolved path backed by
<path>/index.md(e.g.reference,framework/react/reference), every load now does two sequentialfetchDocscalls: a guaranteed-miss on<path>.mdfollowed by the successful<path>/index.mdretry. IffetchDocshits GitHub's API/raw content (uncached), this doubles latency and request volume for what looks like it will be a common page (Core API Reference is linked from nav). Worth confirming there's caching in front offetchDocs, or consider whether the manifest could flag index-backed paths to avoid the guaranteed-miss request — though that would revisit the "no manifest changes" constraint called out in the PR objective.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/utils/docs.ts` around lines 211 - 246, Review the loadDocs and loadDocsIndexFile flow to avoid an uncached sequential miss for canonical paths backed by index.md. Confirm whether fetchers.fetchDocs is cached; if not, use existing resolution information to select the index file before attempting the .md path, without changing the manifest format or altering fallback behavior for non-index pages.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/utils/docs.ts`:
- Around line 211-246: Review the loadDocs and loadDocsIndexFile flow to avoid
an uncached sequential miss for canonical paths backed by index.md. Confirm
whether fetchers.fetchDocs is cached; if not, use existing resolution
information to select the index file before attempting the .md path, without
changing the manifest format or altering fallback behavior for non-index pages.
In `@tests/docs-route-index-fallback.test.ts`:
- Around line 29-61: Add a test alongside the existing index-fallback tests that
overrides createFetchers().fetchDocs to throw a genuine Error, invokes loadRoute
for the fallback scenario, and asserts the error is rejected with its original
message. Ensure the test verifies non-404 failures propagate through
loadDocsIndexFile/loadDocsRoute rather than being treated as missing documents.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: c2b196bd-e754-4d3b-90b5-3e7f636da262
📒 Files selected for processing (2)
src/utils/docs.tstests/docs-route-index-fallback.test.ts
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/docs-route-index-fallback.test.ts (1)
3-5: 🎯 Functional Correctness | 🔵 Trivial | 🏗️ Heavy liftRetain route-level regression coverage.
These tests only exercise
loadDocs, while this PR also changesloadDocsRouteand redirect resolution. Add root/frameworkloadDocsRoutecases proving reference indexes render, plus a section-index redirect case, so the reported routing regression is covered end-to-end.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/docs-route-index-fallback.test.ts` around lines 3 - 5, Add end-to-end regression cases around loadDocsRoute, covering root and framework routes that render reference indexes and a section-index route that resolves to the expected redirect. Keep the existing loadDocs coverage, and exercise route handling and redirect resolution rather than testing only the lower-level loader.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@tests/docs-route-index-fallback.test.ts`:
- Around line 3-5: Add end-to-end regression cases around loadDocsRoute,
covering root and framework routes that render reference indexes and a
section-index route that resolves to the expected redirect. Keep the existing
loadDocs coverage, and exercise route handling and redirect resolution rather
than testing only the lower-level loader.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: a0a67178-8154-41d6-918d-09f5afc8947d
📒 Files selected for processing (2)
src/utils/docs.tstests/docs-route-index-fallback.test.ts
|
Thanks for the work on this. The runtime fix has already shipped on main, and we’ve reverified that the affected Form and Pacer reference routes are returning 200. Since the remaining value here is regression coverage rather than an unshipped fix, we’re going to close this PR and keep the review queue focused. Appreciate the contribution. |
What changed
mainalready shipped the runtime fix in1e23c84b:loadDocsretries a missing<path>.mdas<path>/index.md.After resolving the conflict, this PR now keeps that implementation and adds focused regression coverage for:
The only production-code addition is a typed optional fetch function so the loader can be tested without changing its default runtime behavior.
Evidence
This covers the root cause reported in TanStack/pacer#237 and tracked in #1069. The affected live root and framework reference URLs now return 200 responses.
Validation
pnpm testRisk
Low. Runtime path resolution remains the implementation already on
main; this PR primarily pins its behavior with tests.Summary by CodeRabbit
Bug Fixes
index.mdfallback when the direct document isn’t found.Tests