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
WalkthroughThe pull request introduces a GitHub Actions workflow for automated checking of edit links, modifies the Changes
Assessment against linked issues
Poem
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
| 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!
There was a problem hiding this comment.
@akshatnema I don't get it. The coderrabit is not suggesting adding a variable for configurable batch size. In fact, we have it already
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 19
Lines 668 668
=========================================
Hits 668 668 ☔ View full report in Codecov by Sentry. |
| '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 |
|
|
||
| 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.
Description
Related issue(s)
fixes #3221
Summary by CodeRabbit
Release Notes
New Features
Chores
Bug Fixes
.mdxextension.