fix(sitemap): exclude failed prerender pages from the sitemap#7804
fix(sitemap): exclude failed prerender pages from the sitemap#7804wildlyinaccurate wants to merge 1 commit into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughPrerender retry scheduling now permits retry requeues without duplicating crawled pages. Exhausted failures are excluded from the sitemap before optionally being rethrown, with tests covering direct, successful, crawled, and retried pages. ChangesPrerender sitemap exclusion
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/start-plugin-core/src/prerender.ts (1)
215-225: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winEnsure exhausted retries actually reach sitemap exclusion.
With
retryCount > 0, Line 222 re-callsaddCrawlPageTask(page), but Line 112 returns because the path is already inseen. The retry is never run, so this new exhaustion branch is never reached and the failed page remains sitemap-eligible. Reschedule retries without re-running discovery/deduplication, and add aretryCount: 1regression test.Proposed fix
-function addCrawlPageTask(page: Page) { - if (seen.has(page.path)) return +function addCrawlPageTask(page: Page, isRetry = false) { + if (!isRetry && seen.has(page.path)) return seen.add(page.path) - if (page.fromCrawl) { + if (page.fromCrawl && !isRetry) { startConfig.pages.push(page) } ... - addCrawlPageTask(page) + addCrawlPageTask(page, true)🤖 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 `@packages/start-plugin-core/src/prerender.ts` around lines 215 - 225, Update the retry flow in the prerender logic around addCrawlPageTask so retries bypass the existing seen-path discovery/deduplication guard and are actually executed. Preserve normal deduplication for newly discovered pages, ensure exhausted retries reach the page.sitemap exclusion branch, and add a regression test covering retryCount: 1.
🧹 Nitpick comments (1)
packages/start-plugin-core/tests/prerender-sitemap-exclude.test.ts (1)
4-5: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReplace broad
anycasts with concrete fixture types.The
anycasts suppress the config and page-contract checks this suite is intended to exercise. TypemakeStartConfigand the page lookups against the production config/page types instead.As per coding guidelines,
**/*.{ts,tsx}must “use TypeScript strict mode with extensive type safety.”Also applies to: 12-13, 38-62, 80-80, 129-130
🤖 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 `@packages/start-plugin-core/tests/prerender-sitemap-exclude.test.ts` around lines 4 - 5, Replace the broad any casts in the vi.mock('../src/utils') setup and the related makeStartConfig/page lookup code with the concrete production config and page types. Update all referenced locations, including the mock’s importActual typing, so TypeScript validates the fixture configuration and page contracts without changing the test behavior.Source: Coding guidelines
🤖 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.
Outside diff comments:
In `@packages/start-plugin-core/src/prerender.ts`:
- Around line 215-225: Update the retry flow in the prerender logic around
addCrawlPageTask so retries bypass the existing seen-path
discovery/deduplication guard and are actually executed. Preserve normal
deduplication for newly discovered pages, ensure exhausted retries reach the
page.sitemap exclusion branch, and add a regression test covering retryCount: 1.
---
Nitpick comments:
In `@packages/start-plugin-core/tests/prerender-sitemap-exclude.test.ts`:
- Around line 4-5: Replace the broad any casts in the vi.mock('../src/utils')
setup and the related makeStartConfig/page lookup code with the concrete
production config and page types. Update all referenced locations, including the
mock’s importActual typing, so TypeScript validates the fixture configuration
and page contracts without changing the test behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 181b5598-701a-4f4c-94ff-881dc9109d30
📒 Files selected for processing (2)
packages/start-plugin-core/src/prerender.tspackages/start-plugin-core/tests/prerender-sitemap-exclude.test.ts
wildlyinaccurate
force-pushed
the
fix/sitemap-exclude-failed-pages
branch
from
5e40498 to
841af08
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/start-plugin-core/tests/prerender-sitemap-exclude.test.ts (1)
79-168: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDuplicate
handlerobject literal across all four tests.The same
{ getClientOutputDirectory, request: requestMock }shape is repeated in every test. Extracting a small factory (e.g.,makeHandler(requestMock)) would reduce duplication.♻️ Proposed helper extraction
+function makeHandler(requestMock: ReturnType<typeof vi.fn>) { + return { + getClientOutputDirectory: () => '/client', + request: requestMock, + } +}Then replace each inline object with
makeHandler(requestMock).🤖 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 `@packages/start-plugin-core/tests/prerender-sitemap-exclude.test.ts` around lines 79 - 168, Extract the repeated handler object creation into a shared makeHandler helper near the tests, accepting requestMock and returning the existing getClientOutputDirectory and request properties. Replace each inline handler literal in the four tests with makeHandler(requestMock), preserving the current behavior and types.
🤖 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 `@packages/start-plugin-core/tests/prerender-sitemap-exclude.test.ts`:
- Around line 79-168: Extract the repeated handler object creation into a shared
makeHandler helper near the tests, accepting requestMock and returning the
existing getClientOutputDirectory and request properties. Replace each inline
handler literal in the four tests with makeHandler(requestMock), preserving the
current behavior and types.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5904f5e9-dc24-4228-85e8-e64b3be17f5d
📒 Files selected for processing (2)
packages/start-plugin-core/src/prerender.tspackages/start-plugin-core/tests/prerender-sitemap-exclude.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/start-plugin-core/src/prerender.ts
|
Fixes from CodeRabbit's first review have been applied and squashed. |
wildlyinaccurate
force-pushed
the
fix/sitemap-exclude-failed-pages
branch
from
841af08 to
cbcc2d6
Compare
1 participant
I have a large prerendered static site. Sometimes pages fail to prerender, but they are still included in the sitemap. This causes crawlers like Google to crawl URLs that ultimately 404.
Summary by CodeRabbit