feat: add workflow and script to check edit links on docs #3557
Conversation
anshgoyalevil
requested review from
derberg,
akshatnema,
magicmatatjahu,
Mayaleeeee and
asyncapi-bot-eve
as code owners
WalkthroughA comprehensive enhancement to the documentation link checking and editing workflow has been implemented. The changes introduce a new GitHub Actions workflow for weekly link validation, modify the edit link generation logic in the DocsLayout component, update configuration files, and add a new script for checking edit links. The implementation includes robust error handling, path resolution, and URL validation mechanisms to ensure the accuracy of documentation edit links across the repository. Changes
Assessment against linked issues
Poem
Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
✅ Deploy Preview for asyncapi-website ready!Built without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify site configuration. |
|
⚡️ Lighthouse report for the changes in this PR:
Lighthouse ran on https://deploy-preview-3557--asyncapi-website.netlify.app/ |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
.github/workflows/check-edit-links.yml (2)
32-37: Improve shell script robustness.The error extraction step could be more robust by following shell scripting best practices.
- name: Extract 404 URLs from output id: extract-404 run: | - ERRORS=$(sed -n '/URLs returning 404:/,$p' output.log) - echo "errors<<EOF" >> $GITHUB_OUTPUT - echo "$ERRORS" >> $GITHUB_OUTPUT - echo "EOF" >> $GITHUB_OUTPUT + { + ERRORS="$(sed -n '/URLs returning 404:/,$p' output.log)" + echo "errors<<EOF" + echo "${ERRORS}" + echo "EOF" + } >> "$GITHUB_OUTPUT"🧰 Tools
🪛 actionlint (1.7.4)
32-32: shellcheck reported issue in this script: SC2129:style:2:1: Consider using { cmd1; cmd2; } >> file instead of individual redirects
(shellcheck)
32-32: shellcheck reported issue in this script: SC2086:info:2:23: Double quote to prevent globbing and word splitting
(shellcheck)
32-32: shellcheck reported issue in this script: SC2086:info:3:19: Double quote to prevent globbing and word splitting
(shellcheck)
32-32: shellcheck reported issue in this script: SC2086:info:4:15: Double quote to prevent globbing and word splitting
(shellcheck)
38-49: Consider adding error categorization in Slack notification.The Slack notification could be more informative by categorizing errors.
SLACK_MESSAGE: | 🚨 The following URLs returned 404 during the link check:${{ steps.extract-404.outputs.errors }}
+ Note: These errors might be due to: + - Missing documentation files + - Incorrect edit link paths + - Repository structure changes
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
.github/workflows/check-edit-links.yml(1 hunks)components/layout/DocsLayout.tsx(1 hunks)config/edit-page-config.json(2 hunks)package.json(1 hunks)scripts/markdown/check-editlinks.js(1 hunks)
🧰 Additional context used
🪛 eslint
scripts/markdown/check-editlinks.js
[error] 58-58: Unexpected await inside a loop.
(no-await-in-loop)
[error] 59-59: Unexpected await inside a loop.
(no-await-in-loop)
🪛 actionlint (1.7.4)
.github/workflows/check-edit-links.yml
32-32: shellcheck reported issue in this script: SC2129:style:2:1: Consider using { cmd1; cmd2; } >> file instead of individual redirects
(shellcheck)
32-32: shellcheck reported issue in this script: SC2086:info:2:23: Double quote to prevent globbing and word splitting
(shellcheck)
32-32: shellcheck reported issue in this script: SC2086:info:3:19: Double quote to prevent globbing and word splitting
(shellcheck)
32-32: shellcheck reported issue in this script: SC2086:info:4:15: Double quote to prevent globbing and word splitting
(shellcheck)
⏰ Context from checks skipped due to timeout of 180000ms (1)
- GitHub Check: Lighthouse CI
🔇 Additional comments (4)
components/layout/DocsLayout.tsx (1)
37-39: Ensure correct conversion of.mdxto.mdin edit linksBy replacing
.mdxwith.mdin edit links, please verify that all.mdxfiles correspond to.mdfiles in the repository. Ensure that this change does not introduce broken edit links for any files that should remain as.mdx.config/edit-page-config.json (1)
4-4: Verify the updatedhrefpath for the AsyncAPI Generator docsThe
hreffor/tools/generatorhas been updated to a new path. Please confirm thathttps://github.com/asyncapi/generator/tree/master/apps/generator/docsis the correct location for the generator's documentation and that the link is accessible..github/workflows/check-edit-links.yml (1)
3-7: LGTM! Well-configured workflow triggers.The workflow is appropriately scheduled to run weekly and includes manual trigger capability.
package.json (1)
27-27: Verify the edit links checker script and its dependencies.The script addition looks good, but let's ensure the required files and dependencies are in place.
✅ Verification successful
✓ Edit links checker implementation is complete and properly configured
All required dependencies and configuration files are in place:
- node-fetch-2 is available in package.json
- edit-page-config.json exists with valid mappings
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the existence of the edit links checker script and related files # Check if the script file exists if ! [ -f "scripts/markdown/check-editlinks.js" ]; then echo "Error: check-editlinks.js script is missing" exit 1 fi # Check for required dependencies in package.json rg "scripts/markdown/check-editlinks.js" -lLength of output: 105
Script:
#!/bin/bash # Check script content and dependencies cat scripts/markdown/check-editlinks.js # Also check package.json for any related dependencies cat package.json | jq '.dependencies, .devDependencies'Length of output: 9008
Script:
#!/bin/bash # Check if the config file exists if [ -f "config/edit-page-config.json" ]; then echo "Config file exists" cat config/edit-page-config.json else echo "Config file missing" fiLength of output: 705
scripts/markdown/check-editlinks.js
Outdated
| const batchResults = await processBatch(batch); | ||
| await pause(1000); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider refactoring to avoid 'await' inside a loop
Static analysis tools have flagged the use of await inside a loop on lines 58-59. Using await in a loop can lead to sequential execution and potential performance issues. Consider refactoring the code to process batches concurrently.
Apply this diff to refactor the code:
- for (let i = 0; i < paths.length; i += batchSize) {
- console.log(`Processing batch ${Math.floor(i / batchSize) + 1}/${Math.ceil(paths.length / batchSize)}`);
- const batch = paths.slice(i, i + batchSize);
- const batchResults = await processBatch(batch);
- await pause(1000);
-
- // Filter out null results and add valid URLs to results
- result.push(...batchResults.filter((url) => url !== null));
- }
+ const batches = [];
+ for (let i = 0; i < paths.length; i += batchSize) {
+ const batch = paths.slice(i, i + batchSize);
+ batches.push(batch);
+ }
+
+ console.log(`Processing ${batches.length} batches concurrently...`);
+ const batchResultsArray = await Promise.all(
+ batches.map(async (batch) => {
+ const batchResults = await processBatch(batch);
+ // Introduce a pause between batches if necessary
+ await pause(1000);
+ return batchResults.filter((url) => url !== null);
+ })
+ );
+
+ // Flatten the array of results
+ result.push(...batchResultsArray.flat());📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const batchResults = await processBatch(batch); | |
| await pause(1000); | |
| const batches = []; | |
| for (let i = 0; i < paths.length; i += batchSize) { | |
| const batch = paths.slice(i, i + batchSize); | |
| batches.push(batch); | |
| } | |
| console.log(`Processing ${batches.length} batches concurrently...`); | |
| const batchResultsArray = await Promise.all( | |
| batches.map(async (batch) => { | |
| const batchResults = await processBatch(batch); | |
| // Introduce a pause between batches if necessary | |
| await pause(1000); | |
| return batchResults.filter((url) => url !== null); | |
| }) | |
| ); | |
| // Flatten the array of results | |
| result.push(...batchResultsArray.flat()); |
🧰 Tools
🪛 eslint
[error] 58-58: Unexpected await inside a loop.
(no-await-in-loop)
[error] 59-59: Unexpected await inside a loop.
(no-await-in-loop)
There was a problem hiding this comment.
We already use such a logic inside our Dashboard script and it works great. let's keep a uniform style
There was a problem hiding this comment.
@anshgoyalevil Understood, I'll respect the existing pattern used in the Dashboard script. Thanks for the clarification!
(｀･ω･´)ゞ
✏️ Learnings added
Learnt from: anshgoyalevil
PR: asyncapi/website#3557
File: scripts/markdown/check-editlinks.js:58-59
Timestamp: 2025-01-08T15:15:00.620Z
Learning: In the AsyncAPI codebase, batch processing operations (like in the Dashboard script and check-editlinks.js) follow a sequential pattern using await in loops, which is the preferred approach for maintaining consistency across the codebase.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
There was a problem hiding this comment.
@anshgoyalevil The pattern used in the dashboard script is not appropriate. I think CodeRabbitAI has suggested a more configurable approach where you can change the size of the batch, using a variable. Try to implement this suggestion.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #3557 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 19 20 +1
Lines 668 731 +63
=========================================
+ Hits 668 731 +63 ☔ View full report in Codecov by Sentry. |
jest.config.js
Outdated
| 'scripts/compose.js', | ||
| 'scripts/tools/categorylist.js', | ||
| 'scripts/tools/tags-color.js', | ||
| 'scripts/markdown/check-editlinks.js' |
There was a problem hiding this comment.
I have added the new script to ignore Jest coverage to pass CI. Will create a new good first issue for other contributors to add test
There was a problem hiding this comment.
I think we should have a practice to add test along with the code, so don't add the file here. Instead add the relevant tests for the file.
There was a problem hiding this comment.
uhm okay. will add it
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
jest.config.js(1 hunks)
⏰ Context from checks skipped due to timeout of 180000ms (3)
- GitHub Check: Redirect rules - asyncapi-website
- GitHub Check: Header rules - asyncapi-website
- GitHub Check: Pages changed - asyncapi-website
🔇 Additional comments (1)
jest.config.js (1)
14-14: LGTM!The testMatch pattern correctly excludes Netlify edge function tests while including other test files.
|
@asyncapi/bounty_team |
scripts/markdown/check-editlinks.js
Outdated
|
|
||
| if (invalidUrls.length === 0) { | ||
| console.log('All URLs are valid.'); | ||
| process.exit(0); |
There was a problem hiding this comment.
We shouldn't use process.exit functions like this. Make the conditional rendering more appropriate. Like, make this if block for invalidUrls only.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
tests/fixtures/markdown/check-editlinks-data.js (1)
3-5: Ensure consistency in URL path formatsThe URL paths in the test data are inconsistent - some start with '/' while others don't. This inconsistency could lead to issues in path matching and URL generation.
Apply this diff to maintain consistency:
- urlPath: 'docs/concepts/application', + urlPath: '/docs/concepts/application', - urlPath: 'concepts/application', + urlPath: '/concepts/application', - urlPath: '/tools/cli', + urlPath: 'tools/cli',Also applies to: 8-10, 13-15
tests/markdown/check-editlinks.test.js (1)
90-93: Add timeout handling test caseThe network error test case only covers generic errors. Add a specific test case for request timeouts.
Add this test case:
it('should handle request timeouts', async () => { fetch.mockImplementation(() => new Promise((_, reject) => { setTimeout(() => reject(new Error('Request timeout')), 5000); })); await expect(processBatch(testBatch)).rejects.toThrow('Request timeout'); });scripts/markdown/check-editlinks.js (1)
103-136: Implement pagination for large directoriesThe
generatePathsfunction loads all files into memory at once, which could cause issues with large directories.Consider implementing pagination to process files in chunks:
async function* generatePathsIterator(folderPath, editOptions, batchSize = 100) { const stack = [{ path: folderPath, relativePath: '' }]; let batch = []; while (stack.length > 0) { const { path: currentPath, relativePath } = stack.pop(); const files = await fs.readdir(currentPath); for (const file of files) { const filePath = path.join(currentPath, file); const relativeFilePath = path.join(relativePath, file); const stats = await fs.stat(filePath); if (stats.isDirectory()) { stack.push({ path: filePath, relativePath: relativeFilePath }); } else if (stats.isFile() && file.endsWith('.md') && file !== '_section.md') { const urlPath = relativeFilePath.split(path.sep).join('/').replace('.md', ''); batch.push({ filePath, urlPath, editLink: determineEditLink(urlPath, filePath, editOptions) }); if (batch.length >= batchSize) { yield batch; batch = []; } } } } if (batch.length > 0) { yield batch; } }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
jest.config.js(1 hunks)package.json(1 hunks)scripts/markdown/check-editlinks.js(1 hunks)tests/fixtures/markdown/check-editlinks-data.js(1 hunks)tests/fixtures/markdown/edit-page-config.json(1 hunks)tests/markdown/check-editlinks.test.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- package.json
- jest.config.js
🧰 Additional context used
📓 Learnings (1)
scripts/markdown/check-editlinks.js (1)
Learnt from: anshgoyalevil
PR: asyncapi/website#3557
File: scripts/markdown/check-editlinks.js:58-59
Timestamp: 2025-01-08T15:15:00.759Z
Learning: In the AsyncAPI codebase, batch processing operations (like in the Dashboard script and check-editlinks.js) follow a sequential pattern using await in loops, which is the preferred approach for maintaining consistency across the codebase.
🔇 Additional comments (2)
tests/fixtures/markdown/edit-page-config.json (1)
15-17: Verify empty value fallback handlingThe empty value entry is used as a fallback, but this could lead to incorrect URL generation if not handled carefully. Ensure that the
determineEditLinkfunction properly validates the target before using it.✅ Verification successful
Empty value fallback is properly implemented and tested
The empty value entry in the configuration is intentionally used as a fallback, with proper handling in the
determineEditLinkfunction. The implementation adds necessary path components and has comprehensive test coverage verifying this behavior.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if determineEditLink handles null/undefined targets ast-grep --pattern 'function determineEditLink($_, $_, $_) { $$$ if (!target) { $$$ } $$$ }'Length of output: 106
Script:
#!/bin/bash # Search for determineEditLink function with broader context rg -l "determineEditLink" # Search for files containing both "edit" and "link" rg -l "edit.*link|link.*edit" --type js --type ts # Look for test files related to edit links fd "edit.*test|test.*edit" -e js -e ts -e jsx -e tsxLength of output: 402
Script:
#!/bin/bash # Check implementation cat scripts/markdown/check-editlinks.js # Check test cases cat tests/markdown/check-editlinks.test.js # Check test data cat tests/fixtures/markdown/check-editlinks-data.jsLength of output: 13950
scripts/markdown/check-editlinks.js (1)
147-153: 🛠️ Refactor suggestionImprove error handling in main function
The main function's error handling could be more informative and should set appropriate exit codes.
Apply this diff to improve error handling:
if (invalidUrls.length > 0) { console.log('\nURLs returning 404:\n'); invalidUrls.forEach((url) => console.log(`- ${url.editLink} generated from ${url.filePath}\n`)); console.log(`\nTotal invalid URLs found: ${invalidUrls.length}`); + process.exitCode = 1; } else { console.log('All URLs are valid.'); + process.exitCode = 0; }Likely invalid or redundant comment.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
scripts/markdown/check-editlinks.js (4)
3-3: Consider using native fetch or updating to latest node-fetchThe code uses
node-fetch-2which is an older version. Consider using the nativefetchAPI (available since Node.js 18) or updating to the latest version ofnode-fetch.-const fetch = require('node-fetch-2'); +const fetch = (...args) => import('node-fetch').then(({default: fetch}) => fetch(...args));
6-10: Move ignore list to configuration fileConsider moving the
ignoreFilesarray to the configuration file (edit-page-config.json) to make it more maintainable and configurable without code changes.
142-142: Enhance error message detailThe error message could be more informative by including the original error details.
- throw new Error(`Error processing directory ${folderPath}:`, err); + throw new Error(`Error processing directory ${folderPath}: ${err.message}`);
163-163: Enhance error message detailSimilar to the previous suggestion, include the original error message for better debugging.
- throw new Error('Failed to check edit links:', error); + throw new Error(`Failed to check edit links: ${error.message}`);
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
scripts/markdown/check-editlinks.js(1 hunks)
🧰 Additional context used
📓 Learnings (1)
scripts/markdown/check-editlinks.js (2)
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.
Learnt from: anshgoyalevil
PR: asyncapi/website#3557
File: scripts/markdown/check-editlinks.js:58-59
Timestamp: 2025-01-08T15:15:00.759Z
Learning: In the AsyncAPI codebase, batch processing operations (like in the Dashboard script and check-editlinks.js) follow a sequential pattern using await in loops, which is the preferred approach for maintaining consistency across the codebase.
🔇 Additional comments (4)
scripts/markdown/check-editlinks.js (4)
12-20: LGTM! Well-documented helper functionThe pause function is well-implemented with clear JSDoc documentation.
58-79: Align batch processing pattern with codebase standardsThe current implementation uses concurrent processing with
Promise.all, but based on the codebase patterns (as seen in the Dashboard script), sequential processing is preferred.Would you like to maintain consistency with the codebase's sequential pattern or keep the current concurrent implementation? Please confirm your preference.
88-101: LGTM! Robust edit link determinationThe implementation correctly handles the fallback case and special path prefixes. The code aligns with the established pattern where an empty string value in
editOptionsserves as a catch-all fallback.
172-172: LGTM! Clean exportsThe module exports are well-organized and include all necessary functions.
scripts/markdown/check-editlinks.js
Outdated
| @@ -0,0 +1,172 @@ | |||
| const fs = require('fs').promises; | |||
| const path = require('path'); | |||
| const fetch = require('node-fetch-2'); | |||
There was a problem hiding this comment.
Why node-fetch-2 instead of normal fetch API?
There was a problem hiding this comment.
The module problem we had—I don't exactly remember what it was but we weren't able to use node-fetch because of some version issue.
Here's the PR:
#3038
| ``` | ||
| ${{ steps.extract-404.outputs.errors }} | ||
| ``` | ||
| MSG_MINIMAL: true |
There was a problem hiding this comment.
What does this parameter state for Slack?
There was a problem hiding this comment.
It removes a bunch of useless information from the message.
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
.github/workflows/check-edit-links.yml (3)
4-6: Add detailed cron syntax documentation.While the cron syntax is correct, consider adding a more detailed comment explaining each field for better maintainability:
schedule: - - cron: '0 0 * * 0' # Runs every week at midnight on Sunday + - cron: '0 0 * * 0' # minute(0) hour(0) day(*) month(*) weekday(0=Sunday) - Runs every Sunday at midnight UTC
38-40: Consider moving the if condition.As suggested in the past review, consider moving the if condition above the step name for better readability and to follow the standard pattern in GitHub Actions:
- - name: Notify Slack - if: ${{ steps.extract-404.outputs.errors != '' }} + - if: ${{ steps.extract-404.outputs.errors != '' }} + name: Notify Slack
49-49: Document the MSG_MINIMAL parameter.The
MSG_MINIMALparameter's purpose isn't immediately clear. Add a comment explaining its effect on the Slack notification:- MSG_MINIMAL: true + MSG_MINIMAL: true # Sends a compact message without additional metadata like GitHub workflow details
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
.github/workflows/check-edit-links.yml(1 hunks)package.json(1 hunks)scripts/dashboard/build-dashboard.js(1 hunks)scripts/markdown/check-edit-links.js(1 hunks)tests/markdown/check-edit-links.test.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- package.json
🧰 Additional context used
📓 Learnings (2)
scripts/markdown/check-edit-links.js (1)
Learnt from: anshgoyalevil
PR: asyncapi/website#3557
File: scripts/markdown/check-editlinks.js:58-59
Timestamp: 2025-01-08T15:15:00.759Z
Learning: In the AsyncAPI codebase, batch processing operations (like in the Dashboard script and check-editlinks.js) follow a sequential pattern using await in loops, which is the preferred approach for maintaining consistency across the codebase.
.github/workflows/check-edit-links.yml (1)
Learnt from: anshgoyalevil
PR: asyncapi/website#3557
File: .github/workflows/check-edit-links.yml:25-29
Timestamp: 2025-01-08T15:16:27.655Z
Learning: In GitHub workflows running scripts with process.exit statements for error handling (like check-editlinks.js in asyncapi/website), avoid adding error suppression (|| true) at the workflow level as it would mask the intended error reporting mechanism.
🪛 eslint
scripts/dashboard/build-dashboard.js
[error] 184-184: Replace ·getLabel,·monthsSince,·mapGoodFirstIssues,·getHotDiscussions,·getDiscussionByID,·getDiscussions,·writeToFile,·start,·processHotDiscussions,·pause· with ⏎··getLabel,⏎··monthsSince,⏎··mapGoodFirstIssues,⏎··getHotDiscussions,⏎··getDiscussionByID,⏎··getDiscussions,⏎··writeToFile,⏎··start,⏎··processHotDiscussions,⏎··pause⏎
(prettier/prettier)
🪛 actionlint (1.7.4)
.github/workflows/check-edit-links.yml
32-32: shellcheck reported issue in this script: SC2129:style:2:1: Consider using { cmd1; cmd2; } >> file instead of individual redirects
(shellcheck)
32-32: shellcheck reported issue in this script: SC2086:info:2:23: Double quote to prevent globbing and word splitting
(shellcheck)
32-32: shellcheck reported issue in this script: SC2086:info:3:19: Double quote to prevent globbing and word splitting
(shellcheck)
32-32: shellcheck reported issue in this script: SC2086:info:4:15: Double quote to prevent globbing and word splitting
(shellcheck)
⏰ Context from checks skipped due to timeout of 180000ms (1)
- GitHub Check: Test NodeJS PR - macos-13
🔇 Additional comments (6)
scripts/markdown/check-edit-links.js (2)
83-93:⚠️ Potential issueHandle undefined 'target' to prevent runtime errors in 'determineEditLink'.
In the
determineEditLinkfunction, if no matching edit option is found,targetcould beundefined, leading to aTypeErrorwhen accessingtarget.value. Please add a check to handle the case whentargetisundefinedto prevent runtime errors.Apply this diff to fix the issue:
function determineEditLink(urlPath, filePath, editOptions) { // Remove leading 'docs/' if present for matching const pathForMatching = urlPath.startsWith('docs/') ? urlPath.slice(5) : urlPath; const target = editOptions.find((edit) => pathForMatching.includes(edit.value)); + if (!target) { + // Handle the case where no matching edit option is found + return null; // Or handle appropriately based on your application's needs + } // Handle the empty value case (fallback) if (target.value === '') { return `${target.href}/docs/${urlPath}.md`; } // For other cases with specific targets return `${target.href}/${path.basename(filePath)}`; }Ensure that downstream code can handle a
nullreturn value from this function, or adjust the handling accordingly.
3-3:⚠️ Potential issueCorrect the import statement for 'node-fetch'.
It appears that
'node-fetch-2'is not a valid package. To import version 2 ofnode-fetch, you should use'node-fetch'. Please update the import statement accordingly.Apply this diff to fix the import statement:
-const fetch = require('node-fetch-2'); +const fetch = require('node-fetch');Ensure that the
node-fetchpackage is installed in yourpackage.jsondependencies.scripts/dashboard/build-dashboard.js (1)
184-184: Reformat 'module.exports' for better readability as per Prettier guidelines.The static analysis tool indicates that the
module.exportsstatement should be reformatted for consistency and readability.Apply this diff to format
module.exports:-module.exports = { getLabel, monthsSince, mapGoodFirstIssues, getHotDiscussions, getDiscussionByID, getDiscussions, writeToFile, start, processHotDiscussions, pause }; +module.exports = { + getLabel, + monthsSince, + mapGoodFirstIssues, + getHotDiscussions, + getDiscussionByID, + getDiscussions, + writeToFile, + start, + processHotDiscussions, + pause +};Ensure that the updated formatting aligns with the project's coding standards and passes the linter checks.
🧰 Tools
🪛 eslint
[error] 184-184: Replace
·getLabel,·monthsSince,·mapGoodFirstIssues,·getHotDiscussions,·getDiscussionByID,·getDiscussions,·writeToFile,·start,·processHotDiscussions,·pause·with⏎··getLabel,⏎··monthsSince,⏎··mapGoodFirstIssues,⏎··getHotDiscussions,⏎··getDiscussionByID,⏎··getDiscussions,⏎··writeToFile,⏎··start,⏎··processHotDiscussions,⏎··pause⏎(prettier/prettier)
tests/markdown/check-edit-links.test.js (1)
2-2:⚠️ Potential issueCorrect import and mocking of 'node-fetch'.
It appears that
'node-fetch-2'is not a valid package name. Replace'node-fetch-2'with'node-fetch'in both the import statement and the Jest mock.Apply these diffs:
At line 2:
-const fetch = require('node-fetch-2'); +const fetch = require('node-fetch');At line 13:
-jest.mock('node-fetch-2', () => jest.fn()); +jest.mock('node-fetch', () => jest.fn());Ensure that the
node-fetchpackage is correctly installed and updated in yourpackage.jsondependencies.Also applies to: 13-13
.github/workflows/check-edit-links.yml (2)
9-11: LGTM! Well-structured job configuration.The job configuration follows best practices with clear naming and appropriate runner selection.
25-29: Approved: Proper error handling implementation.The link checker step correctly allows the script's built-in error handling to work without suppression, as noted in the previous learning. This ensures proper error reporting and Slack notifications.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
scripts/markdown/check-edit-links.js (3)
3-3: Consider using native fetch or updating to latest node-fetch.The script uses
node-fetch-2which is an older version. Consider:
- Using the native
fetchAPI (available since Node.js 18.0.0)- Updating to the latest
node-fetchversion-const fetch = require('node-fetch-2'); +// If using Node.js >= 18.0.0 +const fetch = (...args) => import('node-fetch').then(({default: fetch}) => fetch(...args));
19-19: Extract timeout duration as a named constant.The timeout duration is a magic number. Consider extracting it to a named constant at the module level for better maintainability.
+const URL_CHECK_TIMEOUT_MS = 5000; + async function processBatch(batch) { - const TIMEOUT_MS = 5000; + const TIMEOUT_MS = URL_CHECK_TIMEOUT_MS;
51-51: Extract batch size as a named constant.The batch size is a magic number. Consider extracting it to a named constant at the module level for better configuration.
+const URL_CHECK_BATCH_SIZE = 5; + async function checkUrls(paths) { const result = []; - const batchSize = 5; + const batchSize = URL_CHECK_BATCH_SIZE;
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
scripts/markdown/check-edit-links.js(1 hunks)
🧰 Additional context used
📓 Learnings (1)
scripts/markdown/check-edit-links.js (1)
Learnt from: anshgoyalevil
PR: asyncapi/website#3557
File: scripts/markdown/check-editlinks.js:58-59
Timestamp: 2025-01-08T15:15:00.759Z
Learning: In the AsyncAPI codebase, batch processing operations (like in the Dashboard script and check-editlinks.js) follow a sequential pattern using await in loops, which is the preferred approach for maintaining consistency across the codebase.
🔇 Additional comments (2)
scripts/markdown/check-edit-links.js (2)
102-135: Well-implemented recursive file processing.The function is well-structured with:
- Proper error handling
- Concurrent file processing
- Clear documentation
- Edge case handling (e.g., _section.md files)
137-163: Clean main function implementation with proper exports.The implementation is solid with:
- Comprehensive error handling
- Clear console output
- Well-organized exports
- Proper module execution check
| @@ -0,0 +1,163 @@ | |||
| const fs = require('fs').promises; | |||
| const path = require('path'); | |||
| const fetch = require('node-fetch-2'); | |||
There was a problem hiding this comment.
Why node-fetch-2 is used here?
There was a problem hiding this comment.
There was a problem hiding this comment.
Like we use it in other "js" scripts.
scripts/markdown/check-edit-links.js
Outdated
| * @returns {Promise<string[]>} Array of URLs that returned 404 | ||
| */ | ||
| async function processBatch(batch) { | ||
| const TIMEOUT_MS = 5000; |
There was a problem hiding this comment.
Make it configurable with env variable as well.
scripts/markdown/check-edit-links.js
Outdated
| */ | ||
| async function checkUrls(paths) { | ||
| const result = []; | ||
| const batchSize = 5; |
There was a problem hiding this comment.
Kindly make it configurable using env variable.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
scripts/markdown/check-edit-links.js (3)
7-11: Consider moving ignore list to configuration file.The
ignoreFileslist could be moved to the configuration file (config/edit-page-config.json) to make it more maintainable and configurable without code changes.
102-135: Consider adding additional file validation.While the implementation is solid, consider adding validation for:
- Maximum file size to prevent processing extremely large files
- File name patterns to exclude temporary or backup files (e.g.,
.md~,.md.bak)
137-163: Consider enhancing error reporting.While the error handling is good, consider adding:
- Total number of URLs checked in the success message
- Structured error output (e.g., JSON format) for better parsing
- Error categorization (timeout vs 404 vs other errors)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
scripts/markdown/check-edit-links.js(1 hunks)
🧰 Additional context used
📓 Learnings (1)
scripts/markdown/check-edit-links.js (1)
Learnt from: anshgoyalevil
PR: asyncapi/website#3557
File: scripts/markdown/check-editlinks.js:58-59
Timestamp: 2025-01-08T15:15:00.759Z
Learning: In the AsyncAPI codebase, batch processing operations (like in the Dashboard script and check-editlinks.js) follow a sequential pattern using await in loops, which is the preferred approach for maintaining consistency across the codebase.
🔇 Additional comments (3)
scripts/markdown/check-edit-links.js (3)
18-42: Well-implemented batch processing with timeout handling!The implementation includes:
- Configurable timeout via environment variable
- Proper request cancellation using AbortController
- Comprehensive error handling
- Test coverage exclusions where appropriate
49-70: Efficient implementation of concurrent batch processing!The implementation includes:
- Configurable batch size via environment variable
- Concurrent processing with Promise.all
- Rate limiting prevention with pauses between batches
79-92: Clean and efficient edit link determination logic!The implementation correctly handles both specific targets and fallback cases, with proper path manipulation.
Description
Related issue(s)
fixes #3221
Summary by CodeRabbit
Release Notes
New Features
Improvements
Testing
Chores