Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review infoConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughRefactors the external-links checker to a two-phase, concurrency-configurable flow with per-file aggregation and JSON reporting; adds a GitHub Actions workflow to run and format the checker output on PRs; removes the old Lychee workflow; and updates the npm script argument separator. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
🍈 Lychee Link Check Report3664 links: ✅ All links are working!Full Statistics Table
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
apps/docs/scripts/lint-external-links.ts (2)
297-316: Two-phase design (collect then validate) is a solid architectural choice.Separating link extraction from URL validation is cleaner and more efficient — you deduplicate URLs before hitting the network, and the two phases have independent concurrency knobs. Good stuff.
One note on lines 314–315:
perFileOccurrences.push(...)andextractedLinks += ...mutate shared state from within concurrent workers. This is safe in Node.js because there's noawaitbetween the mutation and the next yield point (JS is cooperative, not preemptive). Just calling it out so future readers don't panic — a brief inline comment would help.📝 Optional: add a clarifying comment
+ // Safe to mutate – no `await` between here and the next iteration, + // so no other worker can interleave. perFileOccurrences.push(localOccurrences); extractedLinks += links.length;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/docs/scripts/lint-external-links.ts` around lines 297 - 316, The concurrent worker body mutates shared variables perFileOccurrences and extractedLinks which can look unsafe; add a brief inline comment right above the perFileOccurrences.push(localOccurrences) and extractedLinks += links.length lines explaining that this mutation is safe because runWithConcurrency runs on Node's single-threaded event loop (no preemptive multithreading) and there is no await between the read/modify/write steps, so the operations are atomic in this context; reference the worker function passed to runWithConcurrency and the local variable localOccurrences in the comment so readers can locate the code quickly.
267-279:mergeOccurrencesaliases source arrays into target — consider cloning.On line 277, when a URL isn't yet in the target map, you set the reference directly:
target.set(url, occurrences). This meanstargetandsourcenow share the same array instance. If anything ever mutates the source map's arrays after merging, the target map silently reflects those changes too.In your current usage this is safe because
perFileOccurrencesentries aren't touched after merge. But it's a subtle footgun for future maintainers. A shallow copy ([...occurrences]) on line 277 would eliminate this coupling at negligible cost.♻️ Optional: break the shared reference
- target.set(url, occurrences); + target.set(url, [...occurrences]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/docs/scripts/lint-external-links.ts` around lines 267 - 279, The mergeOccurrences function currently aliases the source array into the target by calling target.set(url, occurrences), which creates a shared mutable reference; change the behavior so you insert a shallow copy of the occurrences array when the URL is new (i.e., set the target entry to a new array containing the same items instead of the original array) to avoid accidental future mutations of source affecting target while preserving current semantics; update the logic in mergeOccurrences where target.set is called for a non-existing key to store a copied array rather than the original.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@apps/docs/scripts/lint-external-links.ts`:
- Around line 297-316: The concurrent worker body mutates shared variables
perFileOccurrences and extractedLinks which can look unsafe; add a brief inline
comment right above the perFileOccurrences.push(localOccurrences) and
extractedLinks += links.length lines explaining that this mutation is safe
because runWithConcurrency runs on Node's single-threaded event loop (no
preemptive multithreading) and there is no await between the read/modify/write
steps, so the operations are atomic in this context; reference the worker
function passed to runWithConcurrency and the local variable localOccurrences in
the comment so readers can locate the code quickly.
- Around line 267-279: The mergeOccurrences function currently aliases the
source array into the target by calling target.set(url, occurrences), which
creates a shared mutable reference; change the behavior so you insert a shallow
copy of the occurrences array when the URL is new (i.e., set the target entry to
a new array containing the same items instead of the original array) to avoid
accidental future mutations of source affecting target while preserving current
semantics; update the logic in mergeOccurrences where target.set is called for a
non-existing key to store a copied array rather than the original.
External Link Check Report❌ Failed to parse JSON output from checker.Raw output |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
apps/docs/scripts/lint-external-links.ts (1)
153-153:MARKDOWN_IMAGE_REGEX.lastIndex = 0is dead code — the regex is never iterated.
MARKDOWN_IMAGE_REGEXis reset here but has no correspondingwhileloop incollectFileLinks. Image links are already excluded by the(?<!!)negative lookbehind inMARKDOWN_LINK_REGEX(line 30), so this reset serves no purpose.🧹 Proposed cleanup
- MARKDOWN_IMAGE_REGEX.lastIndex = 0; MARKDOWN_LINK_REGEX.lastIndex = 0; AUTOLINK_REGEX.lastIndex = 0; HTML_ANCHOR_REGEX.lastIndex = 0;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/docs/scripts/lint-external-links.ts` at line 153, Remove the dead reset of the MARKDOWN_IMAGE_REGEX in collectFileLinks: the statement "MARKDOWN_IMAGE_REGEX.lastIndex = 0" is unnecessary because MARKDOWN_IMAGE_REGEX is never iterated and image links are already excluded by the negative lookbehind in MARKDOWN_LINK_REGEX; delete that line (referencing MARKDOWN_IMAGE_REGEX and the collectFileLinks function) to clean up the codebase..github/workflows/external-links.yml (1)
33-42: Add a step-level timeout to prevent runaway link-checker runs.Without a
timeout-minutescap, a large docs site with hundreds of slow-responding external URLs could stall the job indefinitely (the script-levelTIMEOUT_MSonly gates individual URL fetches, not the whole run).⏱️ Proposed fix
- name: Run external link checker id: external-links + timeout-minutes: 30 shell: bash🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/external-links.yml around lines 33 - 42, The "Run external link checker" step (id: external-links) needs a step-level timeout to avoid runaway runs; add a timeout-minutes value (for example timeout-minutes: 30 or an appropriate cap for your CI budget) to the step definition that runs "pnpm run lint:external-links" so the whole step is terminated if it exceeds that limit, keeping the existing commands and exit handling intact.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In @.github/workflows/external-links.yml:
- Around line 33-42: The "Run external link checker" step (id: external-links)
needs a step-level timeout to avoid runaway runs; add a timeout-minutes value
(for example timeout-minutes: 30 or an appropriate cap for your CI budget) to
the step definition that runs "pnpm run lint:external-links" so the whole step
is terminated if it exceeds that limit, keeping the existing commands and exit
handling intact.
In `@apps/docs/scripts/lint-external-links.ts`:
- Line 153: Remove the dead reset of the MARKDOWN_IMAGE_REGEX in
collectFileLinks: the statement "MARKDOWN_IMAGE_REGEX.lastIndex = 0" is
unnecessary because MARKDOWN_IMAGE_REGEX is never iterated and image links are
already excluded by the negative lookbehind in MARKDOWN_LINK_REGEX; delete that
line (referencing MARKDOWN_IMAGE_REGEX and the collectFileLinks function) to
clean up the codebase.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.github/workflows/external-links.yml.github/workflows/lychee.ymlapps/docs/scripts/lint-external-links.tspackage.json
💤 Files with no reviewable changes (1)
- .github/workflows/lychee.yml
1 participant
Summary by CodeRabbit
New Features
Chores
Other