refactor: optimize directory traversal in check-edit-links script #4126
Conversation
NalinDalal
requested review from
Mayaleeeee,
akshatnema,
anshgoyalevil,
asyncapi-bot-eve,
derberg and
sambhavgupta0705
as code owners
WalkthroughReplaces recursive directory traversal with an async generator to stream Markdown files; reimplements Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor CLI as User/CI
participant Script as check-edit-links
participant FS as FileSystem
participant Net as HTTP
CLI->>Script: main()
Script->>Script: generatePaths(folderPath, editOptions)
Script->>FS: walkDirectory(dir) (async generator)
loop for each streamed markdown file
FS-->>Script: { filePath, relativeFilePath }
Script->>Script: determineEditLink(relativeFilePath, editOptions)
Script->>Script: emit PathObject
end
Script->>Script: processBatch(batch)
loop each PathObject
alt no editLink or matched ignore
Script-->>Script: skip
else
Script->>Net: HEAD editLink (AbortController + timeout)
Net-->>Script: 200 / 404 / error
alt 404
Script-->>Script: return PathObject (record 404)
else if error
Script-->>Script: return error
else
Script-->>Script: return null (success)
end
end
end
Script-->>CLI: aggregated results
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 180000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
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 |
✅ Deploy Preview for asyncapi-website ready!Built without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify project configuration. |
NalinDalal
changed the title
NalinDalal
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
scripts/markdown/check-edit-links.ts (1)
117-121: Simplify error handling in async function.Since this is an async function, you can throw the error directly instead of using
Promise.reject.Apply this diff to simplify the error handling:
- } catch (error) { - return Promise.reject( - new Error(`Error checking ${editLink}: ${error}`), - ); + } catch (error) { + throw new Error(`Error checking ${editLink}: ${error}`);
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
components/CaseStudyCard.tsx(1 hunks)components/campaigns/AnnouncementHero.tsx(1 hunks)pages/tools/generator.tsx(1 hunks)scripts/markdown/check-edit-links.ts(6 hunks)
🧰 Additional context used
🧠 Learnings (1)
scripts/markdown/check-edit-links.ts (1)
Learnt from: anshgoyalevil
PR: asyncapi/website#3557
File: scripts/markdown/check-editlinks.js:80-93
Timestamp: 2025-01-14T09:23:32.728Z
Learning: In the AsyncAPI website's edit link generation system, the `editOptions` array in `edit-page-config.json` includes a fallback entry with an empty string value (`''`) that matches any URL path, ensuring that `determineEditLink()` function always finds a target and never returns null.
🪛 GitHub Actions: PR testing - if Node project
scripts/markdown/check-edit-links.ts
[error] 84-84: Error walking directory /home/runner/work/website/website/markdown/docs: TypeError [ERR_INVALID_ARG_TYPE]: The "path" argument must be of type string. Received undefined
⏰ Context from checks skipped due to timeout of 180000ms (4)
- GitHub Check: Redirect rules - asyncapi-website
- GitHub Check: Header rules - asyncapi-website
- GitHub Check: Pages changed - asyncapi-website
- GitHub Check: Lighthouse CI
🔇 Additional comments (5)
components/campaigns/AnnouncementHero.tsx (1)
63-63: CSS class reordering looks fine, but inconsistent with PR objectives.The Tailwind CSS class reordering doesn't affect functionality since CSS class order doesn't impact the styling output. However, there's a significant inconsistency between the PR objectives (optimizing directory traversal in check-edit-links script) and the actual changes (CSS class reordering in React components).
Likely an incorrect or invalid review comment.
components/CaseStudyCard.tsx (1)
22-22: CSS class reordering is functionally safe.The reordering of
justify-centerandgap-3classes maintains the same visual output since Tailwind CSS class order doesn't affect their application. This change continues the pattern of cosmetic class reordering across components.pages/tools/generator.tsx (1)
56-56:✅ Verification successful
CSS reordering is safe, but main PR objective missing.
The class reordering from
'mx-auto h-auto lg:w-1/2 object-cover'to'mx-auto h-auto object-cover lg:w-1/2'doesn't impact functionality. However, the primary focus of this PR was supposed to be optimizing directory traversal inscripts/markdown/check-edit-links.ts, which is not included in the files for review.
🏁 Script executed:
#!/bin/bash # Description: Search for the check-edit-links script that should be the main focus of this PR # Expected: Find the script file and verify it contains the directory traversal optimizations mentioned in PR objectives # Search for check-edit-links script file fd check-edit-links # Search for any TypeScript files that might contain edit link checking logic fd -e ts | xargs rg -l "edit.*link|editLink" | head -10 # Search for directory traversal or async generator patterns in TypeScript files rg -A 5 "async.*generator|for await.*of.*readdir" --type tsLength of output: 1829
🏁 Script executed:
#!/bin/bash # Inspect directory traversal implementation in the check-edit-links script rg -n "readdir" scripts/markdown/check-edit-links.ts || true rg -n "async" scripts/markdown/check-edit-links.ts || true rg -n "for await" scripts/markdown/check-edit-links.ts || trueLength of output: 605
🏁 Script executed:
#!/bin/bash # Compare the check-edit-links script in this PR branch against main to verify if it's been updated set -e # Ensure we have the main branch to compare against git fetch origin main echo "---- Diff for scripts/markdown/check-edit-links.ts (origin/main → PR branch) ----" git diff origin/main -- scripts/markdown/check-edit-links.ts || echo "[No changes detected]"Length of output: 69
#!/bin/bash # Determine the remote default branch and diff the check-edit-links script against it set -e # Fetch all remote refs git fetch origin # Identify the default branch remote_head=$(git symbolic-ref refs/remotes/origin/HEAD) default_branch=${remote_head#refs/remotes/origin/} echo "Default branch: $default_branch" # Show diff for the check-edit-links script echo "---- Diff for scripts/markdown/check-edit-links.ts (origin/$default_branch → PR branch) ----" git diff origin/"$default_branch" -- scripts/markdown/check-edit-links.ts || echo "[No changes detected]"
All PR objectives met—no missing script and CSS reordering is safe
- The file
scripts/markdown/check-edit-links.tsis present in this branch.- It implements directory traversal optimizations using an async generator (
walkDirectory) withfs.readdirandfor await…of, covering the PR’s stated goals.- The CSS class reorder in
pages/tools/generator.tsxremains purely cosmetic and has no functional impact.No further action required.
scripts/markdown/check-edit-links.ts (2)
36-57: Great use of async generators for memory-efficient directory traversal!The refactoring from recursive
Promise.allto async generators is a significant improvement. This approach:
- Reduces memory usage by processing files one at a time
- Improves error handling granularity
- Makes the code more maintainable and easier to understand
Also applies to: 67-80
41-56:❌ Incorrect review comment
Add validation to prevent undefined path errors.
The pipeline error suggests that a path argument is undefined. This could happen if
entry.nameis undefined in certain edge cases.Add defensive checks to prevent undefined values:
async function* walkDirectory( dir: string, relativePath = '', ): AsyncGenerator<{ filePath: string; relativeFilePath: string }> { const entries = await fs.readdir(dir, { withFileTypes: true }); for (const entry of entries) { + if (!entry || !entry.name) { + console.warn(`Skipping invalid entry in directory: ${dir}`); + continue; + } const absPath = path.join(dir, entry.name); const relPath = path.join(relativePath, entry.name);Let me verify if there are any edge cases in the directory structure:
🏁 Script executed:
#!/bin/bash # Description: Check for any unusual files or directories that might cause issues # Check if the docs directory exists if [ -d "markdown/docs" ]; then echo "Checking markdown/docs directory structure..." # Look for symbolic links echo "Symbolic links:" find markdown/docs -type l -ls 2>/dev/null | head -20 # Look for files with unusual names echo -e "\nFiles with special characters or spaces:" find markdown/docs -name "*[[:space:]]*" -o -name "*[^[:alnum:]._/-]*" 2>/dev/null | head -20 # Check for empty directories echo -e "\nEmpty directories:" find markdown/docs -type d -empty 2>/dev/null | head -20 else echo "markdown/docs directory not found" fiLength of output: 493
Ignore entry.name validation —
Dirent.nameis always definedOur directory‐structure check found no symlinks, special‐character filenames, or empty directories under
markdown/docs, and Node’sDirentobjects always include a non-emptynamestring. Adding a guard forentry.nameisn’t necessary and won’t address the pipeline error.• Instead, review where
walkDirectoryis first invoked and ensure thedirargument (or any upstream path) can’t beundefined.
• Check any dynamic or recursive calls to confirm they pass a valid string.
• Consider adding a validation or assertion at the very start ofwalkDirectoryto catch unexpected inputs early:async function* walkDirectory(dir: string, relativePath = '') { if (typeof dir !== 'string' || !dir) { throw new Error(`Invalid directory argument passed to walkDirectory: ${dir}`); } // …rest of implementation… }Likely an incorrect or invalid review comment.
NalinDalal
changed the title
There was a problem hiding this comment.
Hi @NalinDalal please follow relevant comments convention and address the coderabbit's comments please. Thank you
|
@CodeRabbit can you check again the code, I have added the code comments and documented it as asked by @TRohit20 |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain@NalinDalal, I'll check the updated code to review the documentation and comments you've added. 🏁 Script executed: Length of output: 9043 @NalinDalal Great work on the documentation! I've reviewed the updated code and the documentation improvements are excellent. Here's what I noticed: 🎉 Excellent Documentation QualityComprehensive JSDoc Coverage:
Outstanding Design Documentation:
Professional Code Comments:
Key Strengths:
The code is now well-documented, maintainable, and follows best practices. This addresses @TRohit20's feedback perfectly! The documentation quality is professional-grade and will help future contributors understand both the implementation and the reasoning behind design decisions. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
scripts/markdown/check-edit-links.ts (1)
71-82: Approve refactor with a formatting suggestion for readability.The refactored
generatePathsfunction correctly implements streaming path generation using the async generator, which successfully addresses the PR objectives. The input validation (lines 72-74) is a good defensive practice, and the urlPath construction (line 80) properly handles cross-platform path separators.However, line 81 has a formatting issue that impacts readability:
Consider reformatting the object literal for better readability:
- result.push({filePath,urlPath,editLink: determineEditLink(urlPath, filePath, editOptions)}); + result.push({ + filePath, + urlPath, + editLink: determineEditLink(urlPath, filePath, editOptions) + });
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
scripts/markdown/check-edit-links.ts(2 hunks)tests/markdown/check-edit-links.test.ts(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/markdown/check-edit-links.test.ts
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: anshgoyalevil
PR: asyncapi/website#3301
File: scripts/markdown/check-markdown.js:0-0
Timestamp: 2024-10-18T17:28:41.718Z
Learning: In `scripts/markdown/check-markdown.js`, when processing large markdown files with recursion and loops, synchronous file reads (`fs.readFileSync`) are preferred over asynchronous reads.
🔇 Additional comments (2)
scripts/markdown/check-edit-links.ts (2)
37-50: LGTM! Async generator correctly implements streaming traversal.The
walkDirectoryasync generator successfully addresses the PR objective of replacing synchronous directory reads with a memory-efficient streaming approach. The implementation correctly:
- Uses
fs.readdirwithwithFileTypes: truefor efficient directory entries- Recursively traverses subdirectories with
yield*- Streams file paths incrementally instead of loading the entire tree into memory
- Properly filters for
.mdfiles while excluding_section.mdErrors from
fs.readdirwill propagate to the caller's try-catch block (lines 78-89), which provides appropriate error handling.
85-89: Enhanced error handling correctly implemented.The error handling improvements properly preserve both the error message and stack trace, which addresses the past review feedback about masking original error details. This implementation will help identify root causes when directory traversal fails.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/markdown/check-edit-links.test.ts (1)
62-70: This test contradicts documented behavior.According to the retrieved learnings, the
editOptionsconfiguration includes a fallback entry with an empty value that acts as a catch-all matcher, meaningdetermineEditLink()will always return a match and never returnnull. Testing with an emptyeditOptionsarray contradicts this established behavior and may not reflect real-world usage.Based on learnings.
Consider either:
- Removing this test if the empty-array scenario is truly unreachable in production, or
- Documenting why this edge case is being tested despite the fallback guarantee.
♻️ Duplicate comments (2)
tests/markdown/check-edit-links.test.ts (1)
16-18: The mock path mismatch remains unresolved.As noted in previous reviews, the mock targets
'../../scripts/helpers/logger.ts'but the import path is'../../scripts/helpers/logger'(without.ts). Additionally, onlylogger.infois mocked, but if the implementation logs errors or warnings, tests may fail unexpectedly.Apply this diff to fix the mock path and include all logger methods:
-jest.mock('../../scripts/helpers/logger.ts', () => ({ - logger: { info: jest.fn() } +jest.mock('../../scripts/helpers/logger', () => ({ + logger: { info: jest.fn(), error: jest.fn(), warn: jest.fn() } }));scripts/markdown/check-edit-links.ts (1)
36-36: Remove the inline marker comment.Based on previous maintainer feedback, code should be self-explanatory without additional comments. The comment
// NEW: Async generator for efficient directory traversalis unnecessary—the function name and JSDoc already convey this information.Apply this diff:
- -// NEW: Async generator for efficient directory traversal async function* walkDirectory(dir: string, relativePath = ''): AsyncGenerator<{ filePath: string; relativeFilePath: string }> {
🧹 Nitpick comments (3)
tests/markdown/check-edit-links.test.ts (3)
105-105: Improve array formatting for readability.The inline array has no spacing between elements, making it harder to read.
Apply this diff:
- const mockReaddir = jest.spyOn(fs, 'readdir').mockImplementation(async (dir, opts) => [dirent('test.js', true, false),dirent('test.md', true, false)]); + const mockReaddir = jest.spyOn(fs, 'readdir').mockImplementation(async (dir, opts) => [ + dirent('test.js', true, false), + dirent('test.md', true, false) + ]);
130-131: Improve array formatting for readability.Multiple inline arrays lack spacing, reducing clarity.
Apply this diff:
- jest.spyOn(fs, 'readdir').mockImplementationOnce(async () => [dirent('subdir', false, true),dirent('main.md', true, false)]) - .mockImplementationOnce(async () => [dirent('subfile.md', true, false)]); + jest.spyOn(fs, 'readdir') + .mockImplementationOnce(async () => [ + dirent('subdir', false, true), + dirent('main.md', true, false) + ]) + .mockImplementationOnce(async () => [dirent('subfile.md', true, false)]);
187-208: Consider consolidating duplicate test scenarios.Lines 187-197 test skipping files with no
editLinkor inignoreFiles, which overlaps with the existing test at lines 162-175. The new batch definition (lines 187-191) and test (lines 193-197) duplicate coverage already provided by the earlier test.Consider removing the duplicate test block (lines 187-208) or merging unique assertions into the existing test at lines 162-175 to reduce redundancy.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
scripts/markdown/check-edit-links.ts(2 hunks)tests/markdown/check-edit-links.test.ts(4 hunks)
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: anshgoyalevil
PR: asyncapi/website#3301
File: scripts/markdown/check-markdown.js:0-0
Timestamp: 2024-10-18T17:28:41.718Z
Learning: In `scripts/markdown/check-markdown.js`, when processing large markdown files with recursion and loops, synchronous file reads (`fs.readFileSync`) are preferred over asynchronous reads.
📚 Learning: 2025-01-14T15:12:29.840Z
Learnt from: anshgoyalevil
PR: asyncapi/website#3557
File: scripts/markdown/check-edit-links.js:83-88
Timestamp: 2025-01-14T15:12:29.840Z
Learning: In the AsyncAPI website's edit link generation logic, the `editUrls` configuration (config/edit-page-config.json) includes a fallback entry with an empty value that acts as a catch-all matcher. Therefore, `editOptions.find()` will always return a match, making null checks on the result unnecessary and untestable.
Applied to files:
tests/markdown/check-edit-links.test.ts
📚 Learning: 2025-01-19T04:51:41.255Z
Learnt from: anshgoyalevil
PR: asyncapi/website#3557
File: tests/fixtures/markdown/check-edit-links-data.js:3-11
Timestamp: 2025-01-19T04:51:41.255Z
Learning: In the AsyncAPI website repository, the test data in `tests/fixtures/markdown/check-edit-links-data.js` intentionally includes inconsistent paths (with and without 'docs' prefix) to verify the script's ability to normalize and handle ambiguous path structures.
Applied to files:
tests/markdown/check-edit-links.test.ts
📚 Learning: 2025-01-14T09:23:32.728Z
Learnt from: anshgoyalevil
PR: asyncapi/website#3557
File: scripts/markdown/check-editlinks.js:80-93
Timestamp: 2025-01-14T09:23:32.728Z
Learning: In the AsyncAPI website's edit link generation system, the `editOptions` array in `edit-page-config.json` includes a fallback entry with an empty string value (`''`) that matches any URL path, ensuring that `determineEditLink()` function always finds a target and never returns null.
Applied to files:
tests/markdown/check-edit-links.test.ts
📚 Learning: 2024-11-29T17:36:09.783Z
Learnt from: vishvamsinh28
PR: asyncapi/website#3378
File: tests/markdown/check-markdown.test.js:133-138
Timestamp: 2024-11-29T17:36:09.783Z
Learning: When testing the 'main' function in 'check-markdown.test.js', it's acceptable to simply ensure it doesn't throw any errors, as the functions it calls are already tested elsewhere.
Applied to files:
tests/markdown/check-edit-links.test.ts
⏰ Context from checks skipped due to timeout of 180000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Redirect rules - asyncapi-website
- GitHub Check: Header rules - asyncapi-website
- GitHub Check: Pages changed - asyncapi-website
- GitHub Check: Test NodeJS PR - windows-latest
🔇 Additional comments (5)
scripts/markdown/check-edit-links.ts (4)
37-50: LGTM! The async generator optimizes memory usage.The streaming approach using an async generator addresses the PR objective to replace synchronous full-tree reads with incremental yielding. This reduces memory footprint and improves performance for large documentation trees, as files are processed on-the-fly rather than loaded into memory all at once.
72-74: Good defensive validation.The input validation guards against invalid or undefined
folderPatharguments, which aligns with the test coverage added in lines 118-122 of the test file. The error message clearly indicates the expected type and constraint.
85-89: Good improvement to error handling.Preserving the error message and stack trace ensures that debugging information is not lost, which will help diagnose issues like the
TypeErrorabout undefined paths mentioned in pipeline failures. This addresses previous feedback.
79-82: Path separator normalization logic is correct. ThewalkDirectorygenerator emits OS-specific separators viapath.join, and replacingpath.sepwith'/'reliably produces forward-slash URLs on all platforms.tests/markdown/check-edit-links.test.ts (1)
72-77: Clarify fallback link prefix duplication
The fallback branch always prepends/docs/tourlPath, so passingdocs/anythingyields/docs/docs/anything.md; confirm whether to strip the leadingdocs/(e.g. viapathForMatching) before concatenation.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/markdown/check-edit-links.test.ts (1)
106-117: Fix ReferenceError: Remove undefined mockStat.Line 116 attempts to restore
mockStatwhich is never defined in this test, causing the pipeline failure. The test only createsmockReaddir, so only that mock needs restoration.Apply this diff:
mockReaddir.mockRestore(); - mockStat.mockRestore();
🧹 Nitpick comments (4)
tests/markdown/check-edit-links.test.ts (4)
72-84: Minor: Inconsistent indentation in test expectations.The test expectations at lines 76 and 83 have unusual indentation. Consider aligning them consistently for better readability.
Example:
- determineEditLink('docs/anything', 'docs/anything.md', fallbackOption), - ).toBe('https://github.com/org/repo/edit/main/docs/docs/anything.md'); + determineEditLink('docs/anything', 'docs/anything.md', fallbackOption) + ).toBe('https://github.com/org/repo/edit/main/docs/docs/anything.md');
108-108: Improve mock fidelity: Specify withFileTypes option.Since the implementation uses dirent objects (which require
withFileTypes: true), the mock should explicitly handle the options parameter to better reflect the actual API usage.Consider:
- const mockReaddir = jest.spyOn(fs, 'readdir').mockImplementation(async (dir, opts) => [dirent('test.js', true, false),dirent('test.md', true, false)]); + const mockReaddir = jest.spyOn(fs, 'readdir').mockImplementation(async (dir, opts) => { + if (opts?.withFileTypes) { + return [dirent('test.js', true, false), dirent('test.md', true, false)] as any; + } + return ['test.js', 'test.md']; + });
198-219: Consider relocating the batch constant.The
batcharray defined at lines 198-202 is only used in the tests below it. Consider moving it into a nesteddescribeblock or into the specific tests to improve locality and reduce scope.Example:
describe('processBatch edge cases', () => { const batch = [ {filePath: 'file1.md', urlPath: 'docs/file1', editLink: 'https://...'}, {filePath: 'reference/specification/v2.x.md', urlPath: 'docs/reference/specification/v2.x', editLink: 'https://...'}, { filePath: 'file2.md', urlPath: 'docs/file2', editLink: null } ]; it('skips files with no editLink or in ignoreFiles', async () => { // test implementation }); // more tests... });
222-222: Remove unnecessary comment as per coding conventions.The comment
// ----------- checkUrls tests -----------is redundant since thedescribe('checkUrls', ...)block immediately follows and clearly indicates the test suite. Based on previous feedback, comments like this should be removed to let the code be self-explanatory.Apply this diff:
- // ----------- checkUrls tests ----------- describe('checkUrls', () => {
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/markdown/check-edit-links.test.ts(6 hunks)
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: anshgoyalevil
PR: asyncapi/website#3301
File: scripts/markdown/check-markdown.js:0-0
Timestamp: 2024-10-18T17:28:41.718Z
Learning: In `scripts/markdown/check-markdown.js`, when processing large markdown files with recursion and loops, synchronous file reads (`fs.readFileSync`) are preferred over asynchronous reads.
📚 Learning: 2025-01-14T15:12:29.840Z
Learnt from: anshgoyalevil
PR: asyncapi/website#3557
File: scripts/markdown/check-edit-links.js:83-88
Timestamp: 2025-01-14T15:12:29.840Z
Learning: In the AsyncAPI website's edit link generation logic, the `editUrls` configuration (config/edit-page-config.json) includes a fallback entry with an empty value that acts as a catch-all matcher. Therefore, `editOptions.find()` will always return a match, making null checks on the result unnecessary and untestable.
Applied to files:
tests/markdown/check-edit-links.test.ts
📚 Learning: 2025-01-19T04:51:41.255Z
Learnt from: anshgoyalevil
PR: asyncapi/website#3557
File: tests/fixtures/markdown/check-edit-links-data.js:3-11
Timestamp: 2025-01-19T04:51:41.255Z
Learning: In the AsyncAPI website repository, the test data in `tests/fixtures/markdown/check-edit-links-data.js` intentionally includes inconsistent paths (with and without 'docs' prefix) to verify the script's ability to normalize and handle ambiguous path structures.
Applied to files:
tests/markdown/check-edit-links.test.ts
📚 Learning: 2025-01-14T09:23:32.728Z
Learnt from: anshgoyalevil
PR: asyncapi/website#3557
File: scripts/markdown/check-editlinks.js:80-93
Timestamp: 2025-01-14T09:23:32.728Z
Learning: In the AsyncAPI website's edit link generation system, the `editOptions` array in `edit-page-config.json` includes a fallback entry with an empty string value (`''`) that matches any URL path, ensuring that `determineEditLink()` function always finds a target and never returns null.
Applied to files:
tests/markdown/check-edit-links.test.ts
📚 Learning: 2024-11-29T17:36:09.783Z
Learnt from: vishvamsinh28
PR: asyncapi/website#3378
File: tests/markdown/check-markdown.test.js:133-138
Timestamp: 2024-11-29T17:36:09.783Z
Learning: When testing the 'main' function in 'check-markdown.test.js', it's acceptable to simply ensure it doesn't throw any errors, as the functions it calls are already tested elsewhere.
Applied to files:
tests/markdown/check-edit-links.test.ts
🪛 GitHub Actions: PR testing - if Node project
tests/markdown/check-edit-links.test.ts
[error] 116-116: ReferenceError: mockStat is not defined. In generatePaths test, mockStat is used but not defined. npm test --if-present failed.
🔇 Additional comments (3)
tests/markdown/check-edit-links.test.ts (3)
20-22: Good addition for test readability.The
direnthelper function effectively simulates filesystem directory entries, making the test mocks more maintainable and easier to understand.
125-144: Excellent error handling coverage.These new tests effectively cover critical error paths including invalid inputs, filesystem errors, and subdirectory traversal. The chained mock implementations for subdirectory testing are well-structured.
243-249: LGTM! Clear and focused test.This test effectively verifies that
checkUrlscorrectly filters and returns only the 404 responses from a batch of mixed results.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/markdown/check-edit-links.test.ts (1)
106-117: Remove undefinedmockStatreference.Line 116 calls
mockStat.mockRestore()butmockStatis never defined in this test, causing the ReferenceError reported in the pipeline failures.Apply this diff:
it('should skip non-markdown files', async () => { // Create a mock implementation to test the else branch const mockReaddir = jest.spyOn(fs, 'readdir').mockImplementation(async (dir, opts) => [dirent('test.js', true, false),dirent('test.md', true, false)]); const result = await generatePaths(testDir, editOptions); // Only the markdown file should be included, not the js file expect(result.length).toBe(1); expect(result[0].filePath.endsWith('.md')).toBe(true); mockReaddir.mockRestore(); - mockStat.mockRestore(); });
♻️ Duplicate comments (1)
tests/markdown/check-edit-links.test.ts (1)
16-18: Fix mock path and add missing logger methods.As noted in a previous review, the mock targets
'../../scripts/helpers/logger.ts'but the import is from'../../scripts/helpers/logger'(without.ts), so the mock won't apply. Additionally, onlylogger.infois mocked; if the implementation callslogger.errororlogger.warn, tests may fail unexpectedly.Apply this diff:
-jest.mock('../../scripts/helpers/logger.ts', () => ({ - logger: { info: jest.fn() } +jest.mock('../../scripts/helpers/logger', () => ({ + logger: { info: jest.fn(), error: jest.fn(), warn: jest.fn() } }));
🧹 Nitpick comments (2)
tests/markdown/check-edit-links.test.ts (2)
108-108: Add spaces after commas in object/array literals.Several lines contain object/array literals with missing spaces after commas, reducing readability. Consider adding spaces for consistency with common JavaScript/TypeScript style guides.
Examples:
-const mockReaddir = jest.spyOn(fs, 'readdir').mockImplementation(async (dir, opts) => [dirent('test.js', true, false),dirent('test.md', true, false)]); +const mockReaddir = jest.spyOn(fs, 'readdir').mockImplementation(async (dir, opts) => [dirent('test.js', true, false), dirent('test.md', true, false)]);- jest.spyOn(fs, 'readdir').mockImplementationOnce(async () => [dirent('subdir', false, true),dirent('main.md', true, false)]) - .mockImplementationOnce(async () => [dirent('subfile.md', true, false)]); + jest.spyOn(fs, 'readdir').mockImplementationOnce(async () => [dirent('subdir', false, true), dirent('main.md', true, false)]) + .mockImplementationOnce(async () => [dirent('subfile.md', true, false)]);- const batch = [ - {filePath: 'file1.md',urlPath: 'docs/file1',editLink: 'https://github.com/org/repo/edit/main/file1.md'}, - {filePath: 'reference/specification/v2.x.md',urlPath: 'docs/reference/specification/v2.x',editLink: 'https://github.com/org/repo/edit/main/v2.x.md'}, - { filePath: 'file2.md', urlPath: 'docs/file2', editLink: null } // no editLink - ]; + const batch = [ + { filePath: 'file1.md', urlPath: 'docs/file1', editLink: 'https://github.com/org/repo/edit/main/file1.md' }, + { filePath: 'reference/specification/v2.x.md', urlPath: 'docs/reference/specification/v2.x', editLink: 'https://github.com/org/repo/edit/main/v2.x.md' }, + { filePath: 'file2.md', urlPath: 'docs/file2', editLink: null } + ];- mockFetch.mockImplementation(() =>Promise.reject(new Error('Network error'))); - await expect(processBatch([{filePath: 'file.md',urlPath: 'docs/file',editLink: 'https://github.com/org/repo/edit/main/file.md'}])).rejects.toThrow('Network error'); + mockFetch.mockImplementation(() => Promise.reject(new Error('Network error'))); + await expect(processBatch([{ filePath: 'file.md', urlPath: 'docs/file', editLink: 'https://github.com/org/repo/edit/main/file.md' }])).rejects.toThrow('Network error');- mockFetch.mockImplementation((url) =>Promise.resolve({ status: url.includes('bad') ? 404 : 200 })); - const paths = [{filePath: 'good.md',urlPath: 'docs/good',editLink: 'https://github.com/org/repo/edit/main/good.md'},{filePath: 'bad.md',urlPath: 'docs/bad',editLink: 'https://github.com/org/repo/edit/main/bad.md'}]; + mockFetch.mockImplementation((url) => Promise.resolve({ status: url.includes('bad') ? 404 : 200 })); + const paths = [{ filePath: 'good.md', urlPath: 'docs/good', editLink: 'https://github.com/org/repo/edit/main/good.md' }, { filePath: 'bad.md', urlPath: 'docs/bad', editLink: 'https://github.com/org/repo/edit/main/bad.md' }];Also applies to: 138-139, 198-202, 217-217, 244-245
222-222: Remove unnecessary section comment.As noted in previous review feedback, the code should be self-explanatory without additional section divider comments like
// ----------- checkUrls tests -----------. Thedescribeblock already clearly indicates the test suite scope.Apply this diff:
- // ----------- checkUrls tests ----------- describe('checkUrls', () => {
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/markdown/check-edit-links.test.ts(6 hunks)
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: anshgoyalevil
PR: asyncapi/website#3301
File: scripts/markdown/check-markdown.js:0-0
Timestamp: 2024-10-18T17:28:41.718Z
Learning: In `scripts/markdown/check-markdown.js`, when processing large markdown files with recursion and loops, synchronous file reads (`fs.readFileSync`) are preferred over asynchronous reads.
📚 Learning: 2025-01-14T15:12:29.840Z
Learnt from: anshgoyalevil
PR: asyncapi/website#3557
File: scripts/markdown/check-edit-links.js:83-88
Timestamp: 2025-01-14T15:12:29.840Z
Learning: In the AsyncAPI website's edit link generation logic, the `editUrls` configuration (config/edit-page-config.json) includes a fallback entry with an empty value that acts as a catch-all matcher. Therefore, `editOptions.find()` will always return a match, making null checks on the result unnecessary and untestable.
Applied to files:
tests/markdown/check-edit-links.test.ts
📚 Learning: 2025-01-19T04:51:41.255Z
Learnt from: anshgoyalevil
PR: asyncapi/website#3557
File: tests/fixtures/markdown/check-edit-links-data.js:3-11
Timestamp: 2025-01-19T04:51:41.255Z
Learning: In the AsyncAPI website repository, the test data in `tests/fixtures/markdown/check-edit-links-data.js` intentionally includes inconsistent paths (with and without 'docs' prefix) to verify the script's ability to normalize and handle ambiguous path structures.
Applied to files:
tests/markdown/check-edit-links.test.ts
📚 Learning: 2025-01-14T09:23:32.728Z
Learnt from: anshgoyalevil
PR: asyncapi/website#3557
File: scripts/markdown/check-editlinks.js:80-93
Timestamp: 2025-01-14T09:23:32.728Z
Learning: In the AsyncAPI website's edit link generation system, the `editOptions` array in `edit-page-config.json` includes a fallback entry with an empty string value (`''`) that matches any URL path, ensuring that `determineEditLink()` function always finds a target and never returns null.
Applied to files:
tests/markdown/check-edit-links.test.ts
📚 Learning: 2024-11-29T17:36:09.783Z
Learnt from: vishvamsinh28
PR: asyncapi/website#3378
File: tests/markdown/check-markdown.test.js:133-138
Timestamp: 2024-11-29T17:36:09.783Z
Learning: When testing the 'main' function in 'check-markdown.test.js', it's acceptable to simply ensure it doesn't throw any errors, as the functions it calls are already tested elsewhere.
Applied to files:
tests/markdown/check-edit-links.test.ts
🪛 GitHub Actions: PR testing - if Node project
tests/markdown/check-edit-links.test.ts
[error] 116-116: ReferenceError: mockStat is not defined. The test attempts to call mockStat.mockRestore() but mockStat is not defined in the scope.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/markdown/check-edit-links.test.ts (1)
108-117: Critical: Remove undefinedmockStat.mockRestore()call.Line 116 references
mockStatwhich is never defined, causing the pipeline failure. The test only mocksfs.readdir, notfs.stat.Apply this diff:
mockReaddir.mockRestore(); - mockStat.mockRestore(); });
♻️ Duplicate comments (1)
tests/markdown/check-edit-links.test.ts (1)
222-222: Remove redundant comment.The
describeblock already labels this test section. As per coding guidelines, code should be self-explanatory without additional comments.Based on coding guidelines.
Apply this diff:
- // ----------- checkUrls tests ----------- describe('checkUrls', () => {
🧹 Nitpick comments (1)
tests/markdown/check-edit-links.test.ts (1)
198-219: LGTM: Good coverage of batch processing edge cases.The new tests properly verify:
- Skipping files without editLinks or in the ignore list
- Returning files with 404 editLinks
- Propagating network errors
Minor: Consider adding whitespace around object properties for readability.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/markdown/check-edit-links.test.ts(6 hunks)
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: anshgoyalevil
PR: asyncapi/website#3301
File: scripts/markdown/check-markdown.js:0-0
Timestamp: 2024-10-18T17:28:41.718Z
Learning: In `scripts/markdown/check-markdown.js`, when processing large markdown files with recursion and loops, synchronous file reads (`fs.readFileSync`) are preferred over asynchronous reads.
📚 Learning: 2025-01-19T04:51:41.255Z
Learnt from: anshgoyalevil
PR: asyncapi/website#3557
File: tests/fixtures/markdown/check-edit-links-data.js:3-11
Timestamp: 2025-01-19T04:51:41.255Z
Learning: In the AsyncAPI website repository, the test data in `tests/fixtures/markdown/check-edit-links-data.js` intentionally includes inconsistent paths (with and without 'docs' prefix) to verify the script's ability to normalize and handle ambiguous path structures.
Applied to files:
tests/markdown/check-edit-links.test.ts
📚 Learning: 2025-01-14T15:12:29.840Z
Learnt from: anshgoyalevil
PR: asyncapi/website#3557
File: scripts/markdown/check-edit-links.js:83-88
Timestamp: 2025-01-14T15:12:29.840Z
Learning: In the AsyncAPI website's edit link generation logic, the `editUrls` configuration (config/edit-page-config.json) includes a fallback entry with an empty value that acts as a catch-all matcher. Therefore, `editOptions.find()` will always return a match, making null checks on the result unnecessary and untestable.
Applied to files:
tests/markdown/check-edit-links.test.ts
📚 Learning: 2025-01-14T09:23:32.728Z
Learnt from: anshgoyalevil
PR: asyncapi/website#3557
File: scripts/markdown/check-editlinks.js:80-93
Timestamp: 2025-01-14T09:23:32.728Z
Learning: In the AsyncAPI website's edit link generation system, the `editOptions` array in `edit-page-config.json` includes a fallback entry with an empty string value (`''`) that matches any URL path, ensuring that `determineEditLink()` function always finds a target and never returns null.
Applied to files:
tests/markdown/check-edit-links.test.ts
📚 Learning: 2024-11-29T17:36:09.783Z
Learnt from: vishvamsinh28
PR: asyncapi/website#3378
File: tests/markdown/check-markdown.test.js:133-138
Timestamp: 2024-11-29T17:36:09.783Z
Learning: When testing the 'main' function in 'check-markdown.test.js', it's acceptable to simply ensure it doesn't throw any errors, as the functions it calls are already tested elsewhere.
Applied to files:
tests/markdown/check-edit-links.test.ts
🪛 GitHub Actions: PR testing - if Node project
tests/markdown/check-edit-links.test.ts
[error] 116-116: ReferenceError: mockStat is not defined.
🔇 Additional comments (5)
tests/markdown/check-edit-links.test.ts (5)
20-22: LGTM: Clean test helper for directory mocks.The
direnthelper simplifies mockingfs.readdirresults with properisFile()andisDirectory()methods.
26-26: LGTM: Better scope for shared test constant.Moving
testDirto top-level scope improves reusability across test cases.
125-144: LGTM: Comprehensive error handling and traversal tests.The new tests properly cover:
- Type validation for invalid inputs
- Error propagation from filesystem operations
- Subdirectory traversal with nested Markdown files
The use of chained
mockImplementationOncecalls effectively simulates directory tree traversal.
243-249: LGTM: Good test for 404 filtering logic.The test correctly verifies that
checkUrlsreturns only files with 404 responses, filtering out successful URLs.
72-84: LGTM: Good coverage for fallback and specific match scenarios.The new tests properly verify:
- Fallback link generation when
editOption.valueis empty- Correct link resolution for specific path matches
These tests align with the learnings about the fallback entry in the edit options configuration.
Based on learnings.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
scripts/markdown/check-edit-links.ts (1)
71-90: Improve object literal formatting for readability.The refactored
generatePathscorrectly uses the async generator for streaming traversal and includes proper input validation and error handling. However, line 81 has the entirePathObjectconstruction on a single line without spacing, which reduces readability.Apply this diff to improve formatting:
- result.push({filePath,urlPath,editLink: determineEditLink(urlPath, filePath, editOptions)}); + result.push({ + filePath, + urlPath, + editLink: determineEditLink(urlPath, filePath, editOptions) + });
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
scripts/markdown/check-edit-links.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: anshgoyalevil
PR: asyncapi/website#3301
File: scripts/markdown/check-markdown.js:0-0
Timestamp: 2024-10-18T17:28:41.718Z
Learning: In `scripts/markdown/check-markdown.js`, when processing large markdown files with recursion and loops, synchronous file reads (`fs.readFileSync`) are preferred over asynchronous reads.
⏰ Context from checks skipped due to timeout of 180000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Lighthouse CI
🔇 Additional comments (1)
scripts/markdown/check-edit-links.ts (1)
37-50: LGTM! Async generator correctly implements streaming traversal.The
walkDirectorygenerator efficiently streams file paths instead of loading the entire directory tree into memory. The implementation correctly:
- Uses
withFileTypes: truefor efficient dirent iteration- Recursively yields from subdirectories with
yield*- Filters for
.mdfiles while excluding_section.md- Provides both absolute and relative paths for URL construction
This addresses the core objective from issue #3586 to replace synchronous full-tree reads with an async streaming approach.
4 participants
Description
This PR introduces a script to validate "Edit on GitHub" links for all markdown documentation files.
Purpose:
editLinkfor each.mdfile based on configuration.editLinkexists (doesn’t return a 404).Related issue(s)
Fixes #3586
Additional changes:
scripts/markdown/check-edit-links.tsfor this issue.If anything comes up (issues, wrong code, or other concerns), please let me know.
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Tests
Chores