feat: added concurrency limit for markdown file processing #3481
Conversation
Aditya0732
requested review from
derberg,
akshatnema,
magicmatatjahu,
anshgoyalevil,
Mayaleeeee and
asyncapi-bot-eve
as code owners
WalkthroughThe pull request introduces modifications to the AsyncAPI website project, focusing on dependency management and concurrency control for markdown file processing. A new function, Changes
Possibly related issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: .coderabbit.yaml ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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-3481--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 (7)
tests/markdown/check-markdown.test.js (3)
83-86: Avoid usingawaitinside a loop for better performanceUsing
awaitinside a loop can cause the iterations to run sequentially, which may impact performance. Consider usingPromise.allto execute these asynchronous operations concurrently.Apply this diff to refactor the code:
- for (let i = 0; i < 20; i++) { - await fs.writeFile( - path.join(tempDir, `test${i}.md`), - '---\ntitle: Test\n---' - ); - } + const writePromises = []; + for (let i = 0; i < 20; i++) { + writePromises.push( + fs.writeFile( + path.join(tempDir, `test${i}.md`), + '---\ntitle: Test\n---' + ) + ); + } + await Promise.all(writePromises);🧰 Tools
🪛 eslint
[error] 83-86: Unexpected
awaitinside a loop.(no-await-in-loop)
[error] 83-86: Replace
⏎··········path.join(tempDir,·test${i}.md),⏎··········'---\ntitle:·Test\n---'⏎········withpath.join(tempDir,·test${i}.md),·'---\ntitle:·Test\n---'(prettier/prettier)
117-121: Format theauthorsarray to comply with Prettier guidelinesThe
authorsarray formatting does not align with Prettier's style rules, which can affect code readability.Apply this diff to reformat the array:
- authors: [ - { name: 'John' }, - { photo: 'jane.jpg' }, - { name: 'Bob', photo: 'bob.jpg', link: 'not-a-url' }, - ], + authors: [{ name: 'John' }, { photo: 'jane.jpg' }, { name: 'Bob', photo: 'bob.jpg', link: 'not-a-url' }],🧰 Tools
🪛 eslint
[error] 117-121: Replace
⏎········{·name:·'John'·},⏎········{·photo:·'jane.jpg'·},⏎········{·name:·'Bob',·photo:·'bob.jpg',·link:·'not-a-url'·},⏎······],with{·name:·'John'·},·{·photo:·'jane.jpg'·},·{·name:·'Bob',·photo:·'bob.jpg',·link:·'not-a-url'·}](prettier/prettier)
151-153: Simplify theexpectstatement formattingThe
expectstatement can be reformatted to improve readability and comply with style guidelines.Apply this diff:
- expect(mockConsoleLog).toHaveBeenCalledWith( - expect.stringContaining('Errors in file invalid.md:'), - ); + expect(mockConsoleLog).toHaveBeenCalledWith(expect.stringContaining('Errors in file invalid.md:'));🧰 Tools
🪛 eslint
[error] 151-153: Replace
⏎······expect.stringContaining('Errors·in·file·invalid.md:'),⏎····withexpect.stringContaining('Errors·in·file·invalid.md:')(prettier/prettier)
scripts/markdown/check-markdown.js (4)
145-150: Improve function parameter formatting for readabilityThe parameters of the
checkMarkdownFilesfunction can be formatted on a single line to enhance readability.Apply this diff:
- async function checkMarkdownFiles( - folderPath, - validateFunction, - limit, - relativePath = '' - ) { + async function checkMarkdownFiles(folderPath, validateFunction, limit, relativePath = '') {🧰 Tools
🪛 eslint
[error] 145-150: Replace
⏎··folderPath,⏎··validateFunction,⏎··limit,⏎··relativePath·=·''⏎withfolderPath,·validateFunction,·limit,·relativePath·=·''(prettier/prettier)
166-171: Simplify recursive function call formattingThe recursive call to
checkMarkdownFilescan be formatted on a single line for better readability.Apply this diff:
- await checkMarkdownFiles( - filePath, - validateFunction, - limit, - relativeFilePath - ); + await checkMarkdownFiles(filePath, validateFunction, limit, relativeFilePath);🧰 Tools
🪛 eslint
[error] 166-171: Replace
⏎··········filePath,⏎··········validateFunction,⏎··········limit,⏎··········relativeFilePath⏎········withfilePath,·validateFunction,·limit,·relativeFilePath(prettier/prettier)
173-173: Add a space aftertrykeywordAdding a space after the
trykeyword enhances code readability.Apply this diff:
- try{ + try {🧰 Tools
🪛 eslint
[error] 173-173: Insert
·(prettier/prettier)
185-185: Insert a space after}catchFor consistency and readability, add a space between
}andcatch.Apply this diff:
- }catch (error) { + } catch (error) {🧰 Tools
🪛 eslint
[error] 185-185: Insert
·(prettier/prettier)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (3)
package.json(2 hunks)scripts/markdown/check-markdown.js(3 hunks)tests/markdown/check-markdown.test.js(1 hunks)
🧰 Additional context used
🪛 eslint
tests/markdown/check-markdown.test.js
[error] 41-41: Insert ··
(prettier/prettier)
[error] 42-42: Insert ···
(prettier/prettier)
[error] 43-43: Insert ···
(prettier/prettier)
[error] 44-44: Insert ···
(prettier/prettier)
[error] 78-78: Replace resolve with (resolve)
(prettier/prettier)
[error] 78-78: Return values from promise executor functions cannot be read.
(no-promise-executor-return)
[error] 82-82: Unary operator '++' used.
(no-plusplus)
[error] 83-86: Unexpected await inside a loop.
(no-await-in-loop)
[error] 83-86: Replace ⏎··········path.join(tempDir,·test${i}.md),⏎··········'---\ntitle:·Test\n---'⏎········ with path.join(tempDir,·test${i}.md),·'---\ntitle:·Test\n---'
(prettier/prettier)
[error] 100-100: Replace group with (group)
(prettier/prettier)
[error] 101-101: This line has a length of 126. Maximum allowed is 120.
(max-len)
[error] 101-101: Replace call·=>·Math.abs(call[0]·-·group[0])·<·10 with ⏎··········(call)·=>·Math.abs(call[0]·-·group[0])·<·10⏎········
(prettier/prettier)
[error] 117-121: Replace ⏎········{·name:·'John'·},⏎········{·photo:·'jane.jpg'·},⏎········{·name:·'Bob',·photo:·'bob.jpg',·link:·'not-a-url'·},⏎······], with {·name:·'John'·},·{·photo:·'jane.jpg'·},·{·name:·'Bob',·photo:·'bob.jpg',·link:·'not-a-url'·}]
(prettier/prettier)
[error] 129-129: Delete ,
(prettier/prettier)
[error] 138-138: Delete ··
(prettier/prettier)
[error] 143-146: Replace ⏎······path.join(tempDir,·'invalid.md'),⏎······---\ntitle:·Invalid·Blog\n---,⏎···· with path.join(tempDir,·'invalid.md'),·---\ntitle:·Invalid·Blog\n---``
(prettier/prettier)
[error] 151-153: Replace ⏎······expect.stringContaining('Errors·in·file·invalid.md:'),⏎···· with expect.stringContaining('Errors·in·file·invalid.md:')
(prettier/prettier)
[error] 164-164: Delete ,
(prettier/prettier)
[error] 172-172: Delete ,
(prettier/prettier)
[error] 179-181: Replace ⏎······checkMarkdownFiles(invalidFolderPath,·validateBlogs,·'',·pLimit(10)),⏎···· with checkMarkdownFiles(invalidFolderPath,·validateBlogs,·'',·pLimit(10))
(prettier/prettier)
[error] 189-192: Replace ⏎······path.join(referenceSpecDir,·'skipped.md'),⏎······---\ntitle:·Skipped·File\n---,⏎···· with path.join(referenceSpecDir,·'skipped.md'),·---\ntitle:·Skipped·File\n---``
(prettier/prettier)
[error] 199-201: Replace ⏎········'Errors·in·file·reference/specification/skipped.md',⏎······), with 'Errors·in·file·reference/specification/skipped.md')
(prettier/prettier)
[error] 212-214: Replace ⏎······checkMarkdownFiles(tempDir,·validateBlogs,·'',·pLimit(10)),⏎···· with checkMarkdownFiles(tempDir,·validateBlogs,·'',·pLimit(10))
(prettier/prettier)
[error] 215-218: Replace ⏎······expect.stringContaining(Error·in·directory),⏎······expect.any(Error),⏎···· with expect.stringContaining(Error·in·directory),·expect.any(Error)
(prettier/prettier)
[error] 230-233: Replace ⏎······'Failed·to·validate·markdown·files:',⏎······expect.any(Error),⏎···· with 'Failed·to·validate·markdown·files:',·expect.any(Error)
(prettier/prettier)
[error] 250-250: Insert ⏎
(prettier/prettier)
scripts/markdown/check-markdown.js
[error] 23-23: Replace Invalid·MARKDOWN_CONCURRENCY_LIMIT:·'${envLimit}'.·Value·must·be·a·number.·Falling·back·to·default·of·10. with ⏎······Invalid·MARKDOWN_CONCURRENCY_LIMIT:·'${envLimit}'.·Value·must·be·a·number.·Falling·back·to·default·of·10.⏎····
(prettier/prettier)
[error] 59-66: Replace ⏎····'title',⏎····'date',⏎····'type',⏎····'tags',⏎····'cover',⏎····'authors',⏎·· with 'title',·'date',·'type',·'tags',·'cover',·'authors'
(prettier/prettier)
[error] 128-131: Replace ⏎····frontmatter.weight·===·undefined·||⏎····typeof·frontmatter.weight·!==·'number'⏎·· with frontmatter.weight·===·undefined·||·typeof·frontmatter.weight·!==·'number'
(prettier/prettier)
[error] 145-150: Replace ⏎··folderPath,⏎··validateFunction,⏎··limit,⏎··relativePath·=·''⏎ with folderPath,·validateFunction,·limit,·relativePath·=·''
(prettier/prettier)
[error] 166-171: Replace ⏎··········filePath,⏎··········validateFunction,⏎··········limit,⏎··········relativeFilePath⏎········ with filePath,·validateFunction,·limit,·relativeFilePath
(prettier/prettier)
[error] 173-173: Insert ·
(prettier/prettier)
[error] 177-177: Delete ··
(prettier/prettier)
[error] 185-185: Insert ·
(prettier/prettier)
🔇 Additional comments (2)
package.json (2)
93-93: Dependency p-limit correctly added
The addition of "p-limit": "^6.1.0" to dependencies is appropriate for managing concurrency in markdown processing.
155-155: Confirm the necessity of fast-xml-parser in devDependencies
The fast-xml-parser package has been added to devDependencies. Ensure that this package is required for development purposes and is not unnecessarily included.
Run the following script to check for its usage:
✅ Verification successful
Let me check the usage context of fast-xml-parser to verify if it's appropriately placed in devDependencies.
Let me check the build-rss script to understand the complete picture.
fast-xml-parser is correctly placed in devDependencies
The package is only used in test files (tests/build-rss.test.js) to parse and validate the XML output during testing. The actual RSS feed generation uses jgexml/json2xml package, making fast-xml-parser purely a development dependency for testing purposes.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for usages of 'fast-xml-parser' in the codebase.
# Test: Find all import statements using 'fast-xml-parser'.
# Expect: Only files that require it for development tasks.
rg "require\('fast-xml-parser'\)|import.+from 'fast-xml-parser'"
Length of output: 152
Script:
#!/bin/bash
# Check the content of the test file to understand how fast-xml-parser is used
cat tests/build-rss.test.js
Length of output: 5911
Script:
#!/bin/bash
# Check the content of the RSS build script
cat scripts/build-rss.js
Length of output: 3431
| groups.forEach(group => { | ||
| const concurrentExecutions = mockValidateFunction.mock.calls.filter(call => Math.abs(call[0] - group[0]) < 10).length; | ||
| expect(concurrentExecutions).toBeLessThanOrEqual(5); | ||
| }); |
There was a problem hiding this comment.
Correct the calculation of concurrent executions in the test
The current method of calculating concurrentExecutions is inaccurate because mockValidateFunction.mock.calls contains the arguments passed to the mock function, not the timestamps. As a result, call[0] is undefined.
Consider tracking active executions to verify concurrency limits. Here's an alternative approach:
- groups.forEach(group => {
- const concurrentExecutions = mockValidateFunction.mock.calls.filter(call => Math.abs(call[0] - group[0]) < 10).length;
- expect(concurrentExecutions).toBeLessThanOrEqual(5);
- });
+ let activeCount = 0;
+ let maxActiveCount = 0;
+ const mockValidateFunction = jest.fn().mockImplementation(async () => {
+ activeCount++;
+ if (activeCount > maxActiveCount) maxActiveCount = activeCount;
+ await new Promise(resolve => setTimeout(resolve, 50));
+ activeCount--;
+ });
+ // After execution
+ expect(maxActiveCount).toBeLessThanOrEqual(5);This approach tracks the maximum number of concurrent executions during the test.
Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 eslint
[error] 100-100: Replace group with (group)
(prettier/prettier)
[error] 101-101: This line has a length of 126. Maximum allowed is 120.
(max-len)
[error] 101-101: Replace call·=>·Math.abs(call[0]·-·group[0])·<·10 with ⏎··········(call)·=>·Math.abs(call[0]·-·group[0])·<·10⏎········
(prettier/prettier)
scripts/markdown/check-markdown.js
Outdated
| return false; | ||
| } | ||
| try { | ||
| URL(str); |
There was a problem hiding this comment.
Fix incorrect usage of URL constructor
The URL constructor should be called with the new keyword to create a new URL object.
Apply this diff to correct the function:
- URL(str);
+ new URL(str);📝 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.
| URL(str); | |
| new URL(str); |
Aditya0732
force-pushed
the
feat/markdown-files
branch
from
1e8c182 to
42fe248
Compare
asyncapi-bot
requested review from
devilkiller-ag,
sambhavgupta0705,
BhaswatiRoy,
J0SAL,
TRohit20 and
VaishnaviNandakumar
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
scripts/markdown/check-markdown.js (3)
10-36: Consider extracting magic numbers as named constants.The implementation is robust, but readability could be improved by extracting magic numbers into named constants.
+const DEFAULT_CONCURRENCY_LIMIT = 10; + function getConcurrencyLimit() { const envLimit = process.env.MARKDOWN_CONCURRENCY_LIMIT; // If no env var is set, return default if (envLimit === undefined) { - return 10; + return DEFAULT_CONCURRENCY_LIMIT; } // Attempt to parse the environment variable const parsedLimit = parseInt(envLimit, 10); // Validate the parsed limit if (Number.isNaN(parsedLimit)) { console.warn(`Invalid MARKDOWN_CONCURRENCY_LIMIT: '${envLimit}'. Value must be a number. Falling back to default of 10.`); - return 10; + return DEFAULT_CONCURRENCY_LIMIT; } // Check for non-positive integers if (parsedLimit <= 0) { console.warn( `MARKDOWN_CONCURRENCY_LIMIT must be a positive integer greater than 0. Received: ${parsedLimit}. Falling back to default of 10.` ); - return 10; + return DEFAULT_CONCURRENCY_LIMIT; } return parsedLimit; }🧰 Tools
🪛 eslint
[error] 23-23: Replace
Invalid·MARKDOWN_CONCURRENCY_LIMIT:·'${envLimit}'.·Value·must·be·a·number.·Falling·back·to·default·of·10.with⏎······Invalid·MARKDOWN_CONCURRENCY_LIMIT:·'${envLimit}'.·Value·must·be·a·number.·Falling·back·to·default·of·10.⏎····(prettier/prettier)
145-184: Enhance error handling and documentation for concurrent processing.While the implementation is good, consider adding:
- Documentation about the concurrency behavior
- Error handling specific to the concurrency limiter
/** * Recursively checks markdown files in a folder and validates their frontmatter. * @param {string} folderPath - The path to the folder to check. * @param {Function} validateFunction - The function used to validate the frontmatter. * @param {string} [relativePath=''] - The relative path of the folder for logging purposes. * @param {import('p-limit').default} limit - Concurrency limiter. + * @throws {Error} When the concurrency limiter fails or when file operations fail + * @example + * // Process files with a concurrency limit of 5 + * const limit = pLimit(5); + * await checkMarkdownFiles(folderPath, validateFn, limit); */🧰 Tools
🪛 eslint
[error] 167-167: Delete
··(prettier/prettier)
176-178: Enhance error messages with more context.Consider improving error messages by including more context and using structured logging.
- console.error(`Error processing file ${relativeFilePath}:`, error); + console.error(`Error processing markdown file ${relativeFilePath} (validation failed):`, { + error: error.message, + code: error.code, + stack: error.stack + }); throw error; - console.error(`Error in directory ${folderPath}:`, err); + console.error(`Failed to process markdown files in directory ${folderPath}:`, { + error: err.message, + code: err.code, + stack: err.stack + }); throw err; - console.error('Failed to validate markdown files:', error); + console.error('Markdown validation process failed:', { + error: error.message, + code: error.code, + stack: error.stack + });Also applies to: 185-187, 207-209
tests/markdown/check-markdown.test.js (2)
72-109: Improve concurrency test reliability and readability.The concurrency test could be more reliable and readable with these improvements:
- Use a more descriptive test name
- Add assertions for minimum concurrent executions
- Use constants for test values
- it('respects concurrency limit during file processing', async () => { + it('should process files concurrently while respecting the configured limit', async () => { + const CONCURRENCY_LIMIT = 5; + const TOTAL_FILES = 20; + const PROCESSING_TIME_MS = 50; let activeCount = 0; let maxActiveCount = 0; const mockValidateFunction = jest.fn().mockImplementation(async () => { activeCount++; try { - // Track the maximum number of concurrent executions if (activeCount > maxActiveCount) { maxActiveCount = activeCount; } - // Simulate some processing time - await new Promise(resolve => setTimeout(resolve, 50)); + await new Promise(resolve => setTimeout(resolve, PROCESSING_TIME_MS)); } finally { activeCount--; } }); const writePromises = []; - for (let i = 0; i < 20; i++) { + for (let i = 0; i < TOTAL_FILES; i++) { writePromises.push( fs.writeFile( path.join(tempDir, `test${i}.md`), '---\ntitle: Test\n---' ) ); } await Promise.all(writePromises); - const limit = pLimit(5); // Set limit to 5 + const limit = pLimit(CONCURRENCY_LIMIT); await checkMarkdownFiles(tempDir, mockValidateFunction, '', limit); - // Verify that the maximum number of concurrent executions never exceeds the limit - expect(maxActiveCount).toBeLessThanOrEqual(5); + // Verify concurrent execution bounds + expect(maxActiveCount).toBe(CONCURRENCY_LIMIT); - // Verify that the mock validate function was called for all files - expect(mockValidateFunction).toHaveBeenCalledTimes(20); + expect(mockValidateFunction).toHaveBeenCalledTimes(TOTAL_FILES); });🧰 Tools
🪛 eslint
[error] 76-76: Unary operator '++' used.
(no-plusplus)
[error] 82-82: Delete
··········(prettier/prettier)
[error] 84-84: Replace
resolvewith(resolve)(prettier/prettier)
[error] 84-84: Return values from promise executor functions cannot be read.
(no-promise-executor-return)
[error] 86-86: Unary operator '--' used.
(no-plusplus)
[error] 89-89: Delete
····(prettier/prettier)
[error] 91-91: Unary operator '++' used.
(no-plusplus)
[error] 92-97: Replace
⏎············fs.writeFile(⏎··············path.join(tempDir,·test${i}.md),⏎··············'---\ntitle:·Test\n---'⏎··········)⏎········withfs.writeFile(path.join(tempDir,·test${i}.md),·'---\ntitle:·Test\n---')(prettier/prettier)
[error] 100-100: Delete
····(prettier/prettier)
[error] 103-103: Delete
····(prettier/prettier)
[error] 106-106: Delete
····(prettier/prettier)
153-170: Improve test organization and naming.Consider organizing related test cases using nested describe blocks and more descriptive test names.
- it('returns multiple validation errors for invalid blog frontmatter', async () => { + describe('Blog Frontmatter Validation', () => { + it('should return all validation errors when multiple fields are invalid', async () => { const frontmatter = { title: 123, date: 'invalid-date', type: 'blog', tags: 'not-an-array', cover: ['not-a-string'], authors: { name: 'John Doe' }, }; const errors = validateBlogs(frontmatter); expect(errors).toEqual([ 'Invalid date format: invalid-date', 'Tags should be an array', 'Cover must be a string', 'Authors should be an array' ]); - }); + }); + });🧰 Tools
🪛 eslint
[error] 160-160: Delete
,(prettier/prettier)
[error] 168-168: Delete
,(prettier/prettier)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
scripts/markdown/check-markdown.js(3 hunks)tests/markdown/check-markdown.test.js(1 hunks)
🧰 Additional context used
🪛 eslint
tests/markdown/check-markdown.test.js
[error] 41-41: Insert ··
(prettier/prettier)
[error] 42-42: Insert ···
(prettier/prettier)
[error] 43-43: Insert ···
(prettier/prettier)
[error] 44-44: Insert ···
(prettier/prettier)
[error] 76-76: Unary operator '++' used.
(no-plusplus)
[error] 82-82: Delete ··········
(prettier/prettier)
[error] 84-84: Replace resolve with (resolve)
(prettier/prettier)
[error] 84-84: Return values from promise executor functions cannot be read.
(no-promise-executor-return)
[error] 86-86: Unary operator '--' used.
(no-plusplus)
[error] 89-89: Delete ····
(prettier/prettier)
[error] 91-91: Unary operator '++' used.
(no-plusplus)
[error] 92-97: Replace ⏎············fs.writeFile(⏎··············path.join(tempDir,·test${i}.md),⏎··············'---\ntitle:·Test\n---'⏎··········)⏎········ with fs.writeFile(path.join(tempDir,·test${i}.md),·'---\ntitle:·Test\n---')
(prettier/prettier)
[error] 100-100: Delete ····
(prettier/prettier)
[error] 103-103: Delete ····
(prettier/prettier)
[error] 106-106: Delete ····
(prettier/prettier)
[error] 127-127: Delete ,
(prettier/prettier)
[error] 136-136: Delete ··
(prettier/prettier)
[error] 141-144: Replace ⏎······path.join(tempDir,·'invalid.md'),⏎······---\ntitle:·Invalid·Blog\n---,⏎···· with path.join(tempDir,·'invalid.md'),·---\ntitle:·Invalid·Blog\n---``
(prettier/prettier)
[error] 160-160: Delete ,
(prettier/prettier)
[error] 168-168: Delete ,
(prettier/prettier)
[error] 175-177: Replace ⏎······checkMarkdownFiles(invalidFolderPath,·validateBlogs,·'',·pLimit(10)),⏎···· with checkMarkdownFiles(invalidFolderPath,·validateBlogs,·'',·pLimit(10))
(prettier/prettier)
[error] 185-188: Replace ⏎······path.join(referenceSpecDir,·'skipped.md'),⏎······---\ntitle:·Skipped·File\n---,⏎···· with path.join(referenceSpecDir,·'skipped.md'),·---\ntitle:·Skipped·File\n---``
(prettier/prettier)
[error] 195-197: Replace ⏎········'Errors·in·file·reference/specification/skipped.md',⏎······), with 'Errors·in·file·reference/specification/skipped.md')
(prettier/prettier)
[error] 208-210: Replace ⏎······checkMarkdownFiles(tempDir,·validateBlogs,·'',·pLimit(10)),⏎···· with checkMarkdownFiles(tempDir,·validateBlogs,·'',·pLimit(10))
(prettier/prettier)
[error] 211-214: Replace ⏎······expect.stringContaining(Error·in·directory),⏎······expect.any(Error),⏎···· with expect.stringContaining(Error·in·directory),·expect.any(Error)
(prettier/prettier)
[error] 226-229: Replace ⏎······'Failed·to·validate·markdown·files:',⏎······expect.any(Error),⏎···· with 'Failed·to·validate·markdown·files:',·expect.any(Error)
(prettier/prettier)
[error] 246-246: Insert ⏎
(prettier/prettier)
scripts/markdown/check-markdown.js
[error] 23-23: Replace Invalid·MARKDOWN_CONCURRENCY_LIMIT:·'${envLimit}'.·Value·must·be·a·number.·Falling·back·to·default·of·10. with ⏎······Invalid·MARKDOWN_CONCURRENCY_LIMIT:·'${envLimit}'.·Value·must·be·a·number.·Falling·back·to·default·of·10.⏎····
(prettier/prettier)
[error] 45-45: Do not use 'new' for side effects.
(no-new)
[error] 59-66: Replace ⏎····'title',⏎····'date',⏎····'type',⏎····'tags',⏎····'cover',⏎····'authors',⏎·· with 'title',·'date',·'type',·'tags',·'cover',·'authors'
(prettier/prettier)
[error] 128-131: Replace ⏎····frontmatter.weight·===·undefined·||⏎····typeof·frontmatter.weight·!==·'number'⏎·· with frontmatter.weight·===·undefined·||·typeof·frontmatter.weight·!==·'number'
(prettier/prettier)
[error] 167-167: Delete ··
(prettier/prettier)
🔇 Additional comments (1)
scripts/markdown/check-markdown.js (1)
44-49:
Fix incorrect usage of URL constructor.
The URL constructor must be called with the new keyword.
function isValidURL(str) {
try {
- URL(str);
+ new URL(str);
return true;
} catch (err) {
return false;
}
}Likely invalid or redundant comment.
🧰 Tools
🪛 eslint
[error] 45-45: Do not use 'new' for side effects.
(no-new)
Aditya0732
force-pushed
the
feat/markdown-files
branch
from
e5463b5 to
c8e7e44
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
scripts/markdown/check-markdown.js (3)
8-38: Consider enhancing error handling and readability.The function logic is sound, but could be improved:
+// Constants for configuration +const DEFAULT_CONCURRENCY_LIMIT = 10; +const MIN_CONCURRENCY_LIMIT = 1; + function getConcurrencyLimit() { const envLimit = process.env.MARKDOWN_CONCURRENCY_LIMIT; if (envLimit === undefined) { return DEFAULT_CONCURRENCY_LIMIT; } const parsedLimit = parseInt(envLimit, 10); if (Number.isNaN(parsedLimit)) { - console.warn(`Invalid MARKDOWN_CONCURRENCY_LIMIT: '${envLimit}'. Value must be a number. Falling back to default of 10.`); + console.warn(`Invalid MARKDOWN_CONCURRENCY_LIMIT: '${envLimit}'. Expected a number, got ${typeof envLimit}. Falling back to default of ${DEFAULT_CONCURRENCY_LIMIT}.`); return DEFAULT_CONCURRENCY_LIMIT; } - if (parsedLimit <= 0) { + if (parsedLimit < MIN_CONCURRENCY_LIMIT) { console.warn( - `MARKDOWN_CONCURRENCY_LIMIT must be a positive integer greater than 0. Received: ${parsedLimit}. Falling back to default of 10.` + `MARKDOWN_CONCURRENCY_LIMIT must be a positive integer (>= ${MIN_CONCURRENCY_LIMIT}). Received: ${parsedLimit}. Falling back to default of ${DEFAULT_CONCURRENCY_LIMIT}.` ); return DEFAULT_CONCURRENCY_LIMIT; } return parsedLimit; }🧰 Tools
🪛 eslint
[error] 25-25: Replace
Invalid·MARKDOWN_CONCURRENCY_LIMIT:·'${envLimit}'.·Value·must·be·a·number.·Falling·back·to·default·of·10.with⏎······Invalid·MARKDOWN_CONCURRENCY_LIMIT:·'${envLimit}'.·Value·must·be·a·number.·Falling·back·to·default·of·10.⏎····(prettier/prettier)
145-195: Consider refactoring for better maintainability.The function handles multiple responsibilities and could benefit from separation of concerns.
Consider extracting the file processing logic into a separate function:
+async function processMarkdownFile(filePath, relativeFilePath, validateFunction) { + const fileContent = await fs.readFile(filePath, 'utf-8'); + const { data: frontmatter } = matter(fileContent); + + const errors = validateFunction(frontmatter); + if (errors) { + console.log(`Errors in file ${relativeFilePath}:`); + errors.forEach((error) => console.log(` - ${error}`)); + process.exitCode = 1; + } +} + async function checkMarkdownFiles(folderPath, validateFunction, limit, relativePath = '') { try { const files = await fs.readdir(folderPath); const filePromises = files.map(async (file) => { // ... existing path handling code ... if (stats.isDirectory()) { await checkMarkdownFiles(filePath, validateFunction, limit, relativeFilePath); } else if (path.extname(file) === '.md') { try { - await limit(async () => { - const fileContent = await fs.readFile(filePath, 'utf-8'); - const { data: frontmatter } = matter(fileContent); - - const errors = validateFunction(frontmatter); - if (errors) { - console.log(`Errors in file ${relativeFilePath}:`); - errors.forEach((error) => console.log(` - ${error}`)); - process.exitCode = 1; - } - }); + await limit(() => processMarkdownFile(filePath, relativeFilePath, validateFunction)); } catch (error) { console.error(`Error processing markdown file ${relativeFilePath}:`, { error: error.message, code: error.code, stack: error.stack }); throw error; } } }); await Promise.all(filePromises); } catch (err) { console.error(`Failed to process markdown files in directory ${folderPath}:`, { error: err.message, code: err.code, stack: err.stack }); throw err; } }🧰 Tools
🪛 eslint
[error] 167-167: Delete
··(prettier/prettier)
201-221: Enhance error handling in main function.The error handling could be more specific and informative.
async function main() { try { const concurrencyLimit = getConcurrencyLimit(); console.log(`Configured markdown processing with concurrency limit: ${concurrencyLimit}`); const limit = pLimit(concurrencyLimit); await Promise.all([ checkMarkdownFiles(docsFolderPath, validateDocs, limit, ''), checkMarkdownFiles(blogsFolderPath, validateBlogs, limit, '') ]); } catch (error) { console.error('Markdown validation process failed:', { + message: 'Failed to validate markdown files. Check the error details below.', error: error.message, code: error.code, + path: error.path, stack: error.stack }); process.exit(1); } }tests/markdown/check-markdown.test.js (2)
72-111: Improve concurrency test reliability and readability.The concurrency test could be more reliable and easier to understand.
it('should process files concurrently while respecting the configured limit', async () => { + // Test configuration const CONCURRENCY_LIMIT = 5; const TOTAL_FILES = 20; const PROCESSING_TIME_MS = 50; + const TEST_CONTENT = '---\ntitle: Test\n---'; + + // Concurrency tracking let activeCount = 0; let maxActiveCount = 0; + let processedCount = 0; + const mockValidateFunction = jest.fn().mockImplementation(async () => { activeCount++; try { - // Track the maximum number of concurrent executions if (activeCount > maxActiveCount) { maxActiveCount = activeCount; } - // Simulate some processing time await new Promise(resolve => setTimeout(resolve, PROCESSING_TIME_MS)); + processedCount++; } finally { activeCount--; } }); + // Create test files const writePromises = []; for (let i = 0; i < TOTAL_FILES; i++) { writePromises.push( - fs.writeFile( - path.join(tempDir, `test${i}.md`), - '---\ntitle: Test\n---' - ) + fs.writeFile(path.join(tempDir, `test${i}.md`), TEST_CONTENT) ); } await Promise.all(writePromises); const limit = pLimit(CONCURRENCY_LIMIT); await checkMarkdownFiles(tempDir, mockValidateFunction, limit, ''); - // Verify concurrent execution bounds expect(maxActiveCount).toBe(CONCURRENCY_LIMIT); - expect(mockValidateFunction).toHaveBeenCalledTimes(TOTAL_FILES); + expect(processedCount).toBe(TOTAL_FILES); });🧰 Tools
🪛 eslint
[error] 79-79: Unary operator '++' used.
(no-plusplus)
[error] 85-85: Delete
··········(prettier/prettier)
[error] 87-87: Replace
resolvewith(resolve)(prettier/prettier)
[error] 87-87: Return values from promise executor functions cannot be read.
(no-promise-executor-return)
[error] 89-89: Unary operator '--' used.
(no-plusplus)
[error] 92-92: Delete
····(prettier/prettier)
[error] 94-94: Unary operator '++' used.
(no-plusplus)
[error] 95-100: Replace
⏎············fs.writeFile(⏎··············path.join(tempDir,·test${i}.md),⏎··············'---\ntitle:·Test\n---'⏎··········)⏎········withfs.writeFile(path.join(tempDir,·test${i}.md),·'---\ntitle:·Test\n---')(prettier/prettier)
[error] 103-103: Delete
····(prettier/prettier)
[error] 106-106: Delete
····(prettier/prettier)
[error] 109-109: Delete
····(prettier/prettier)
176-184: Enhance error test case assertions.The error test case could have more specific assertions.
it('logs error to console when an error occurs in checkMarkdownFiles', async () => { const invalidFolderPath = path.join(tempDir, 'non-existent-folder'); + const expectedError = new Error('ENOENT'); await expect( checkMarkdownFiles(invalidFolderPath, validateBlogs, '', pLimit(10)), - ).rejects.toThrow('ENOENT'); + ).rejects.toThrow(expectedError); - expect(mockConsoleError.mock.calls[0][0]).toContain('Error in directory'); + expect(mockConsoleError).toHaveBeenCalledWith( + expect.stringContaining('Failed to process markdown files in directory'), + expect.objectContaining({ + error: expectedError.message, + code: 'ENOENT' + }) + ); });🧰 Tools
🪛 eslint
[error] 179-181: Replace
⏎······checkMarkdownFiles(invalidFolderPath,·validateBlogs,·'',·pLimit(10)),⏎····withcheckMarkdownFiles(invalidFolderPath,·validateBlogs,·'',·pLimit(10))(prettier/prettier)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
scripts/markdown/check-markdown.js(3 hunks)tests/markdown/check-markdown.test.js(1 hunks)
🧰 Additional context used
🪛 eslint
tests/markdown/check-markdown.test.js
[error] 41-41: Insert ··
(prettier/prettier)
[error] 42-42: Insert ···
(prettier/prettier)
[error] 43-43: Insert ···
(prettier/prettier)
[error] 44-44: Insert ···
(prettier/prettier)
[error] 79-79: Unary operator '++' used.
(no-plusplus)
[error] 85-85: Delete ··········
(prettier/prettier)
[error] 87-87: Replace resolve with (resolve)
(prettier/prettier)
[error] 87-87: Return values from promise executor functions cannot be read.
(no-promise-executor-return)
[error] 89-89: Unary operator '--' used.
(no-plusplus)
[error] 92-92: Delete ····
(prettier/prettier)
[error] 94-94: Unary operator '++' used.
(no-plusplus)
[error] 95-100: Replace ⏎············fs.writeFile(⏎··············path.join(tempDir,·test${i}.md),⏎··············'---\ntitle:·Test\n---'⏎··········)⏎········ with fs.writeFile(path.join(tempDir,·test${i}.md),·'---\ntitle:·Test\n---')
(prettier/prettier)
[error] 103-103: Delete ····
(prettier/prettier)
[error] 106-106: Delete ····
(prettier/prettier)
[error] 109-109: Delete ····
(prettier/prettier)
[error] 129-129: Delete ,
(prettier/prettier)
[error] 138-138: Delete ··
(prettier/prettier)
[error] 143-146: Replace ⏎······path.join(tempDir,·'invalid.md'),⏎······---\ntitle:·Invalid·Blog\n---,⏎···· with path.join(tempDir,·'invalid.md'),·---\ntitle:·Invalid·Blog\n---``
(prettier/prettier)
[error] 163-163: Delete ,
(prettier/prettier)
[error] 171-171: Delete ,
(prettier/prettier)
[error] 173-173: Insert ;
(prettier/prettier)
[error] 179-181: Replace ⏎······checkMarkdownFiles(invalidFolderPath,·validateBlogs,·'',·pLimit(10)),⏎···· with checkMarkdownFiles(invalidFolderPath,·validateBlogs,·'',·pLimit(10))
(prettier/prettier)
[error] 189-192: Replace ⏎······path.join(referenceSpecDir,·'skipped.md'),⏎······---\ntitle:·Skipped·File\n---,⏎···· with path.join(referenceSpecDir,·'skipped.md'),·---\ntitle:·Skipped·File\n---``
(prettier/prettier)
[error] 198-198: Replace expect.stringContaining('Errors·in·file·reference/specification/skipped.md') with ⏎······expect.stringContaining('Errors·in·file·reference/specification/skipped.md')⏎····
(prettier/prettier)
[error] 208-210: Replace ⏎······checkMarkdownFiles(tempDir,·validateBlogs,·'',·pLimit(10)),⏎···· with checkMarkdownFiles(tempDir,·validateBlogs,·'',·pLimit(10))
(prettier/prettier)
[error] 211-214: Replace ⏎······expect.stringContaining(Error·in·directory),⏎······expect.any(Error),⏎···· with expect.stringContaining(Error·in·directory),·expect.any(Error)
(prettier/prettier)
[error] 226-226: Insert ·
(prettier/prettier)
[error] 243-243: Insert ⏎
(prettier/prettier)
scripts/markdown/check-markdown.js
[error] 25-25: Replace Invalid·MARKDOWN_CONCURRENCY_LIMIT:·'${envLimit}'.·Value·must·be·a·number.·Falling·back·to·default·of·10. with ⏎······Invalid·MARKDOWN_CONCURRENCY_LIMIT:·'${envLimit}'.·Value·must·be·a·number.·Falling·back·to·default·of·10.⏎····
(prettier/prettier)
[error] 47-47: Do not use 'new' for side effects.
(no-new)
[error] 123-126: Replace ⏎····frontmatter.weight·===·undefined·||⏎····typeof·frontmatter.weight·!==·'number'⏎·· with frontmatter.weight·===·undefined·||·typeof·frontmatter.weight·!==·'number'
(prettier/prettier)
[error] 167-167: Delete ··
(prettier/prettier)
| try { | ||
| new URL(str); | ||
| return true; | ||
| } catch (err) { | ||
| return false; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix URL constructor usage.
The current implementation triggers an eslint error for using 'new' for side effects.
function isValidURL(str) {
try {
- new URL(str);
+ return Boolean(new URL(str));
- return true;
} catch (err) {
return false;
}
}📝 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.
| try { | |
| new URL(str); | |
| return true; | |
| } catch (err) { | |
| return false; | |
| } | |
| try { | |
| return Boolean(new URL(str)); | |
| } catch (err) { | |
| return false; | |
| } |
🧰 Tools
🪛 eslint
[error] 47-47: Do not use 'new' for side effects.
(no-new)
|
@Aditya0732 Pretty good stuff from what I can see. It seems from tests that it fails at some module import error. In that case, we would have to wait for TypeScript migration of the scripts in #3423 |
Right. Is someone working on it? |
Yeah, someone's already working on it. |
Implemented Configurable Concurrency Limit for Markdown File Processing
📝 Description
This pull request introduces a configurable concurrency limit for markdown file processing to improve performance, prevent resource exhaustion, and provide more controlled file validation.
🎯 Problem Statement
In large documentation projects, processing markdown files can:
✨ Features
MARKDOWN_CONCURRENCY_LIMITenvironment variablep-limitfor controlled concurrent file processing🚀 Implementation Details
Concurrency Limit Configuration
MARKDOWN_CONCURRENCY_LIMITExample Configuration
# Set custom concurrency limit MARKDOWN_CONCURRENCY_LIMIT=20 npm run validate-markdown💻 Code Changes
New Function:
getConcurrencyLimit()🌟 Benefits
🧪 Testing
Added comprehensive test cases for:
Verified behavior across various input scenarios
📋 Configuration Guidance
🔍 Potential Future Improvements
🚧 Breaking Changes
📦 Dependencies
p-limitpackage for concurrency management🤝 Code Review Checklist
ℹ️ Additional Notes
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Bug Fixes
Tests