feat: add tests for build post list script #3284
Conversation
✅ Deploy Preview for asyncapi-website ready!Built without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify site configuration. |
vishvamsinh28
requested review from
derberg,
akshatnema,
magicmatatjahu,
anshgoyalevil,
Mayaleeeee and
asyncapi-bot-eve
as code owners
WalkthroughThe pull request introduces significant modifications to the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
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
CodeRabbit Configuration File (
|
|
⚡️ Lighthouse report for the changes in this PR:
Lighthouse ran on https://deploy-preview-3284--asyncapi-website.netlify.app/ |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #3284 +/- ##
===========================================
+ Coverage 67.77% 80.05% +12.28%
===========================================
Files 21 21
Lines 664 672 +8
===========================================
+ Hits 450 538 +88
+ Misses 214 134 -80 ☔ View full report in Codecov by Sentry. 🚨 Try these New Features:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (9)
scripts/index.js (3)
11-15: LGTM! Consider extractingpostDirectoriesfor improved maintainability.The new
postDirectoriesarray provides a clear and flexible way to define the directories to be processed bybuildPostList. This change enhances the configurability of the script.For improved maintainability, consider extracting this array to a separate configuration file. This would allow easier updates to the directory list without modifying the main script. For example:
const { postDirectories } = require('../config/build-config.js');
16-17: LGTM! Consider usingresolveforbasePathas well.The introduction of
basePathandwriteFilePathvariables improves the flexibility of the script by allowing easy configuration of input and output paths.For consistency, consider using
resolveforbasePathas well:const basePath = resolve(__dirname, '..', 'pages');This ensures that the path is always resolved correctly, regardless of the current working directory.
19-19: LGTM! Consider adding error handling.The updated
buildPostListfunction call correctly uses the new parameters, providing more flexibility in the post list building process.Consider adding error handling to manage potential failures in the
buildPostListfunction:try { await buildPostList(postDirectories, basePath, writeFilePath); } catch (error) { console.error('Error building post list:', error); // Optionally, you might want to throw the error or handle it in a way that's appropriate for your application }This will help in identifying and debugging any issues that may occur during the post list building process.
tests/build-post-list.test.js (5)
37-53: LGTM with suggestion: Consider enhancing assertion specificityThe test case effectively verifies the basic functionality of buildPostList. It checks for the existence of the output file and the presence of expected properties in the output.
To further improve the test, consider adding more specific assertions about the content of the output. For example, you could check for the exact number of entries in each category or verify specific fields of the blog entry beyond just the title.
55-82: LGTM with suggestion: Enhance negative test casesThese test cases effectively cover important scenarios: handling directories with only section files and processing multiple release notes. The assertions verify the presence of expected entries in the output.
To make these tests more robust, consider adding negative assertions. For example, in the "handles multiple release notes correctly" test, you could also verify that no unexpected release notes are present in the output.
84-101: LGTM with suggestions: Enhance error handling test and expand slugifyToC testsThe error handling test and slugifyToC tests are good additions to the test suite. However, there are opportunities for improvement:
For the error handling test, consider asserting on the specific type of error thrown, not just that an error is thrown.
The slugifyToC tests cover various input scenarios well. Consider adding a test case for a regular heading without an ID to ensure the function behaves correctly in this common scenario.
Example:
it('handles regular headings without ids', () => { const input = '## My Regular Heading'; expect(slugifyToC(input)).toBe('my-regular-heading'); });
148-189: LGTM with suggestion: Enhance error message assertionsThese additional error handling tests are valuable additions to the test suite, covering important scenarios such as unreadable directories, unparseable front matter, and the case where no post directories are provided.
To further improve these tests, consider making the error message assertions more specific. Instead of just checking for a partial match with "Error while building post list", you could assert on more detailed error messages that provide information about the specific error condition. This would ensure that the function is not only throwing errors but also providing helpful error messages for debugging.
Example:
expect(error.message).toMatch(/Error while building post list: Unable to read directory/);
1-191: Great job on the comprehensive test suite!This test suite for the buildPostList and slugifyToC functions is well-structured and covers a wide range of scenarios, including both happy paths and error cases. The use of beforeEach and afterEach hooks for setup and teardown is commendable.
To further enhance the test suite, consider adding the following:
- Performance test: Add a test case with a large number of files to ensure the function performs well under load.
- Edge case: Test with very long file names or paths to ensure there are no issues with path length limits.
- Internationalization: If relevant, add tests with non-ASCII characters in file names and content to ensure proper handling of international content.
These additions would make an already strong test suite even more robust.
scripts/build-post-list.js (1)
39-40: Remove commented out codeThere is a commented-out
console.logstatement at line 39. If it's no longer needed, consider removing it to keep the code clean.Apply this diff to remove the commented code:
- // console.log(inspect(result, { depth: null, colors: true }))
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
- scripts/build-post-list.js (6 hunks)
- scripts/index.js (1 hunks)
- tests/build-post-list.test.js (1 hunks)
- tests/index.test.js (1 hunks)
🧰 Additional context used
🔇 Additional comments (8)
tests/index.test.js (2)
Line range hint
1-35: LGTM! The changes are consistent throughout the file.The updated import statement for
buildPostListis the only change in this file, and it's correctly reflected in the rest of the code. The test cases and mocks usebuildPostListdirectly, which is consistent with the new named import. The overall structure and functionality of the tests remain intact.
2-2: LGTM! Verify the corresponding export in the source file.The change from default import to named import is a good practice and improves code clarity. This change suggests that
buildPostListis now exported as a named export in thebuild-post-list.jsfile.To ensure consistency, please verify that the export in
scripts/build-post-list.jsmatches this import style. You can run the following script to check:✅ Verification successful
Export Style Verified
The
scripts/build-post-list.jsfile correctly exportsbuildPostListas a named export:module.exports = {slugifyToC, buildPostList}🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the export style of buildPostList in the source file # Test: Search for the export of buildPostList. Expect: Named export rg --type javascript 'exports?.buildPostList|module\.exports.*buildPostList' scripts/build-post-list.jsLength of output: 140
Script:
#!/bin/bash # Description: Verify the export style of buildPostList in the source file without specifying file type # Test: Search for the export of buildPostList. Expect: Named export rg 'exports?.buildPostList|module\.exports.*buildPostList' scripts/build-post-list.jsLength of output: 132
scripts/index.js (2)
4-19: Summary: Good refactoring, ensure consistency across the codebase.The changes to
scripts/index.jssignificantly improve the flexibility and configurability of the post list building process. The introduction ofpostDirectories,basePath, andwriteFilePathvariables, along with the updatedbuildPostListfunction call, allow for easier maintenance and future extensions.To ensure consistency across the codebase:
- Verify that all files importing
buildPostListhave been updated to use the new named import syntax.- Check if there are any other scripts or components that might be affected by these changes and update them accordingly.
- Update any documentation or comments related to the post list building process to reflect these new configurations.
Run the following script to identify potential areas that might need updates:
#!/bin/bash # Description: Identify potential areas affected by the changes # Test 1: Find all files that import from build-post-list.js echo "Files importing from build-post-list.js:" rg --type js "from ['\"].*build-post-list['\"]" # Test 2: Find all files that mention buildPostList echo "Files mentioning buildPostList:" rg --type js "buildPostList" # Test 3: Check for any hardcoded paths related to blog, docs, or about pages echo "Files with hardcoded paths:" rg --type js "pages/(blog|docs|about)"Review the output of this script to ensure all affected areas are properly updated.
4-4: Verify the export change inbuild-post-list.jsand update other imports.The import statement for
buildPostListhas been changed from a default import to a named import. This suggests a corresponding change in thebuild-post-list.jsfile.Please run the following script to verify the export in
build-post-list.jsand check for any other import statements in the codebase that might need updating:Ensure that all occurrences are updated to use the new named import syntax.
✅ Verification successful
Export and Imports Verified Successfully.
The
build-post-list.jsfile correctly exportsbuildPostListas a named export, and all import statements in the codebase, includingscripts/index.js, have been appropriately updated. No further issues were detected.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the export in build-post-list.js and check for other imports # Test 1: Check the export in build-post-list.js echo "Checking export in build-post-list.js:" rg --type js "export.*buildPostList" scripts/build-post-list.js # Test 2: Check for other import statements in the codebase echo "Checking for other import statements:" rg --type js "import.*buildPostList.*from ['\"]./build-post-list['\"]" --glob '!scripts/index.js'Length of output: 2201
tests/build-post-list.test.js (2)
1-35: LGTM: Well-structured test setupThe import statements and describe block setup are well-organized. The use of beforeEach and afterEach hooks to create and clean up a temporary directory structure ensures isolated and repeatable tests. This approach follows testing best practices.
103-146: LGTM: Comprehensive coverage of specification file handlingThese tests effectively verify that the buildPostList function correctly handles various types of specification files that should not be processed. The tests cover files without a title, files with "next-spec" in the filename, and files with "explorer" in the filename. The consistent approach across these tests and the verification of the absence of these entries in the output demonstrate thorough testing of this functionality.
scripts/build-post-list.js (2)
163-163: Ensure imports are updated after exportingbuildPostListNow that
buildPostListis exported, make sure all modules that import it are updated accordingly.Run the following script to find all import statements of
build-post-list:#!/bin/bash # Description: Find all import statements of `build-post-list`. # Expectation: Imports should destructure `buildPostList` from the module. rg --type js "require\(.+build-post-list.+\)" -A 2
29-43: Ensure all calls tobuildPostListare updatedThe
buildPostListfunction now acceptspostDirectories,basePath, andwriteFilePathas parameters. Ensure that all places wherebuildPostListis called have been updated to use the new signature.Run the following script to find all calls to
buildPostListand verify their parameters:✅ Verification successful
All calls to
buildPostListhave been updated to use the new signature with three arguments.
tests/build-post-list.test.jsscripts/index.js🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Find all calls to `buildPostList` in the codebase. # Expectation: Calls should use the new signature with three arguments. rg --type js "buildPostList\(" -A 2Length of output: 714
Script:
#!/bin/bash # Description: Find all direct calls to `buildPostList` in the JavaScript codebase. # Improved search pattern to capture function calls with varying whitespace and argument structures. rg --type js "\bbuildPostList\s*\(" -A 2Length of output: 2505
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
tests/build-post-list.test.js (3)
37-53: Consider adding more specific assertionsWhile this test case covers the basic functionality of
buildPostList, it could be enhanced by adding more specific assertions about the content of the output. For example, you could check for the exact number of entries in each category (docs, blog, about) and verify the structure of individual entries.Here's an example of how you could add more specific assertions:
expect(output.docs).toHaveLength(1); expect(output.blog).toHaveLength(1); expect(output.about).toHaveLength(1); const docEntry = output.docs[0]; expect(docEntry).toHaveProperty('title', 'Docs Home'); expect(docEntry).toHaveProperty('slug', '/docs'); expect(docEntry).toHaveProperty('excerpt');
84-101: Enhance error handling test and add a case for slugifyToCThe error handling test is good, but it could be more specific:
For the error handling test, consider asserting the specific type of error thrown:
await expect(buildPostList([invalidDir], tempDir, writeFilePath)).rejects.toThrow(Error);Add a test case for slugifyToC with a regular heading without an ID:
it('handles regular headings without ids', () => { const input = '## My Regular Heading'; expect(slugifyToC(input)).toBe('my-regular-heading'); });These additions will improve the test coverage and make the error handling more robust.
148-187: Enhance error handling testsWhile these additional error handling tests cover important scenarios, they could be improved for more precise error checking:
Instead of catching the error and checking its message, use
expect().rejects.toThrow()for asynchronous functions:it('throws an error if the directory cannot be read', async () => { const invalidDir = [join(tempDir, 'non-existent-dir'), '/invalid']; await expect(buildPostList([invalidDir], tempDir, writeFilePath)) .rejects.toThrow(/Error while building post list/); });Apply the same pattern to the other two error tests.
Consider adding more specific error message checks if the
buildPostListfunction throws different error messages for different scenarios.These changes will make the tests more robust and easier to maintain.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- tests/build-post-list.test.js (1 hunks)
🧰 Additional context used
🔇 Additional comments (4)
tests/build-post-list.test.js (4)
1-35: LGTM: Well-structured test setupThe import statements and describe block setup are well-organized. The use of beforeEach and afterEach hooks ensures a clean test environment for each test case, which is a good practice for maintaining test isolation.
55-82: LGTM: Good coverage of edge casesThese test cases effectively cover important scenarios:
- Handling directories with only section files.
- Processing multiple release notes correctly.
The tests are well-structured and include specific assertions to verify the correct behavior of the
buildPostListfunction in these scenarios.
103-146: LGTM: Comprehensive tests for specification file filteringThese test cases effectively cover the filtering of specification files:
- Files without a title
- Files with "next-spec" in the filename
- Files with "explorer" in the filename
The tests are well-structured and include appropriate assertions to verify that these files are not processed by the
buildPostListfunction. This ensures that only the intended files are included in the final output.
1-189: Overall: Well-structured and comprehensive test suiteThis test file provides a thorough and well-organized suite of tests for the
buildPostListandslugifyToCfunctions. Key strengths include:
- Comprehensive coverage of happy paths, edge cases, and error scenarios.
- Consistent structure throughout the file.
- Good use of beforeEach and afterEach hooks for test isolation.
- Effective testing of file filtering logic.
While there are some minor suggestions for improvements (as noted in previous comments), the overall quality of the test suite is high. It should provide robust validation of the functions' behavior and help catch potential regressions in future development.
|
Tests are passing on Ubuntu but failing on Windows. I will keep it as a draft until I find a solution for it. |
vishvamsinh28
force-pushed
the
buildPostTest
branch
2 times, most recently
from
98f835b
to
ecf927e
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
scripts/build-post-list.js (2)
29-43: Improved flexibility and error handling inbuildPostListThe changes to the
buildPostListfunction are well-implemented:
- The function now accepts parameters, enhancing its configurability and reusability.
- Error handling for empty
postDirectorieshas been added, which is a good practice.- The use of a
writeFilePathparameter allows for more flexible output options.These changes align well with the PR objectives of adding tests, as they make the function more testable.
However, the error handling in the catch block could be improved to preserve the original error stack trace.
Consider updating the error handling to preserve the original stack trace:
} catch (error) { - throw new Error(`Error while building post list: ${error.message}`); + throw new Error('Error while building post list', { cause: error }); }This change will provide more context for debugging while preserving the original error information.
125-128: Improve variable naming consistency in release notes handlingWhile the logic for extracting version information from release notes file names has been updated, the variable naming could be improved for consistency and clarity.
Consider updating the variable names to follow the camelCase convention used elsewhere in the code:
- const fileName_without_extension = file.slice(0, -4) + const fileNameWithoutExtension = file.slice(0, -4)This change will improve code consistency and readability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- scripts/build-post-list.js (7 hunks)
- tests/build-post-list.test.js (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/build-post-list.test.js
🧰 Additional context used
🔇 Additional comments (6)
scripts/build-post-list.js (6)
2-2: Improved cross-platform compatibilityThe addition of
joinandsepfrom thepathmodule enhances the code's ability to handle file paths consistently across different operating systems. This is a positive change that aligns with best practices for cross-platform Node.js applications.
47-47: Improved consistency inwalkDirectoriesfunctionThe addition of
basePathto thewalkDirectoriesfunction signature and its recursive call ensures consistent handling of the base path throughout the directory traversal process. This change enhances the function's flexibility and aligns well with the modifications made tobuildPostList.Also applies to: 83-83
55-57: Enhanced cross-platform compatibility in file path handlingThe updates to file path handling using
joinandsep, as well as the platform-independent index file check, significantly improve the code's cross-platform compatibility. These changes ensure consistent behavior across different operating systems and align with Node.js best practices for file system operations.Also applies to: 98-98
57-57: Improved slug generation for cross-platform consistencyThe update to slug generation now uses the
basePathparameter and normalizes separators to forward slashes. This change ensures consistent slug generation across different platforms, further enhancing the code's cross-platform compatibility.
162-163: Improved module API with updated exportsThe module exports have been updated to include both
slugifyToCandbuildPostListfunctions. This change enhances the module's API by making both functions available for external use, which aligns well with the PR objectives of adding tests for thebuild-post-list.jsscript. This modification will facilitate easier testing and potential reuse of these functions in other parts of the project.
Line range hint
1-163: Overall assessment of changes tobuild-post-list.jsThe modifications to this file are well-implemented and align closely with the PR objectives of adding tests for the
build-post-list.jsscript. Key improvements include:
- Enhanced cross-platform compatibility through better file path handling.
- Improved function flexibility with parameterized inputs.
- Better error handling, particularly in the
buildPostListfunction.- Updated module exports for easier testing and potential reuse.
These changes contribute positively to the codebase by improving testability, maintainability, and cross-platform consistency. The script is now better equipped to handle various input scenarios, which should facilitate more comprehensive testing.
There are a few minor suggestions for improvement, such as preserving the original error stack trace and maintaining consistent variable naming conventions. Addressing these points will further enhance the overall quality of the code.
vishvamsinh28
force-pushed
the
buildPostTest
branch
from
da6c058
to
fe9c3da
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (1)
tests/build-post-list.test.js (1)
62-64: Remove unnecessaryconsole.logstatements in testsThe
console.logstatements at lines 62, 64, and 72 are unnecessary and can clutter the test output. Consider removing them to keep the test output clean.Apply this diff to remove the
console.logstatements:- console.log("output before ", output); - console.log("output after ", output); - console.log("blog entry ", blogEntry);Also applies to: 72-72
vishvamsinh28
force-pushed
the
buildPostTest
branch
from
0b9b50e
to
7ae14bd
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (4)
tests/index.test.js (1)
Line range hint
16-33: Consider updating the test forbuildPostList.The test suite currently only checks if
buildPostListis called, but doesn't verify its parameters. SincebuildPostListnow accepts three parameters (postDirectories,basePath, andwriteFilePath), consider updating the test to ensure it's called with the correct arguments.Here's a suggested update to the test:
test('should call all functions in the correct order', async () => { await start(); expect(buildPostList).toHaveBeenCalledWith( expect.any(Array), // postDirectories expect.any(String), // basePath expect.any(String) // writeFilePath ); // ... rest of the test remains the same });This change will provide more robust verification of the
buildPostListfunction usage.scripts/index.js (2)
11-17: LGTM: New variables improve configurability.The introduction of
postDirectories,basePath, andwriteFilePathvariables enhances the flexibility of the post list building process. This is a good improvement.Consider adding a brief comment explaining the structure of the
postDirectoriesarray for improved readability:// Each item in postDirectories is a pair of [directoryPath, urlPath] const postDirectories = [ ['pages/blog', '/blog'], ['pages/docs', '/docs'], ['pages/about', '/about'] ];
Line range hint
1-70: Overall assessment: Changes improve flexibility and maintainability.The modifications to
scripts/index.jsalign well with the PR objectives. They support the new functionality being tested in thebuild-post-list.jsscript by updating the import statement and function call. These changes enhance the flexibility and configurability of the post list building process.Consider documenting the new parameter structure for
buildPostListin a README or inline documentation to help future developers understand the configuration options quickly.scripts/build-post-list.js (1)
71-72: LGTM: Improved handling of nested sectionsThe changes in how
parentandsectionIdare set, along with the updated recursive call towalkDirectories, improve the handling of nested sections. This makes the code more robust when dealing with different directory structures.For added clarity, consider adding a comment explaining the logic behind using
slugElements.length - 2andslugElements.length - 1forparentandsectionIdrespectively.Also applies to: 82-82
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
- scripts/build-post-list.js (7 hunks)
- scripts/index.js (1 hunks)
- tests/build-post-list.test.js (1 hunks)
- tests/index.test.js (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/build-post-list.test.js
🧰 Additional context used
🔇 Additional comments (10)
tests/index.test.js (2)
Line range hint
1-33: Overall, the changes look good with room for improvement.The import statement has been correctly updated to reflect the changes in the
buildPostListfunction. However, the test suite could be enhanced to verify the new function signature. Consider implementing the suggested changes to improve test coverage and ensure consistency with the updatedbuildPostListfunction.
2-2: LGTM! Import statement updated correctly.The change from default import to named import is correct and consistent with the changes in the
buildPostListfunction.To ensure consistency, let's verify if the
startfunction has been updated to use the newbuildPostListparameters:✅ Verification successful
Verified! The
startfunction correctly usesbuildPostListwith the updated parameters.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if the start function uses the new buildPostList parameters # Test: Search for buildPostList usage in the start function rg -A 5 'function start' scripts/index.js | rg 'buildPostList'Length of output: 62
Script:
#!/bin/bash # Description: Verify that buildPostList is called with three parameters in scripts/index.js # Search for buildPostList function calls with exactly three arguments ast-grep --pattern 'buildPostList($_, $_, $_)' scripts/index.jsLength of output: 150
scripts/index.js (2)
4-4: LGTM: Import statement updated correctly.The change to use destructuring for importing
buildPostListis consistent with modern JavaScript practices and improves code readability.
19-19: LGTM: Function call updated correctly.The
buildPostListfunction call has been correctly updated to include the new parameters, which is consistent with the changes in the function's signature.To ensure that the
buildPostListfunction in thebuild-post-list.jsfile correctly handles these new parameters, please run the following verification script:This script will help verify that the function signature has been updated correctly and that there are no remaining calls to
buildPostListwithout parameters.scripts/build-post-list.js (6)
2-2: LGTM: Cleaned up importsGood job on cleaning up the imports by only importing the
basenamefunction from thepathmodule. This is a best practice that helps keep the code clean and potentially improves performance.
20-26: Improved readability, but consider removing empty else blockThe changes to the
addItemfunction improve readability with better whitespace. However, the emptyelse { }block at line 26 is still present and serves no purpose. Consider removing it as suggested in the previous review.
29-43: Improved function signature and error handling, but consider preserving error stack traceThe changes to the
buildPostListfunction are good improvements:
- The updated function signature allows for more flexibility.
- The new error handling for empty
postDirectoriesis a good addition.- Writing to a specified
writeFilePathenhances the function's configurability.However, as mentioned in a previous review, consider preserving the original error stack trace when rethrowing errors. You can do this by either using
throw erroror using thecauseproperty to retain the error's context.
47-47: LGTM: Updated walkDirectories function signatureThe addition of the
basePathparameter to thewalkDirectoriesfunction signature is a good change. It aligns with the updates in thebuildPostListfunction and allows for more flexible handling of directory paths.
99-105: Consider renaming variables and improving consistencyThe changes in this section make minor adjustments to the logic for handling specification references. However, as mentioned in a previous review, the variable
fileNameis still being reused, which could lead to confusion. Consider renaming the innerfileNameto something likeversionNamefor clarity.Also, for consistency, consider using template literals for string concatenation in line 105:
details.title = `${capitalize(fileName.slice(1))}`Also applies to: 110-110
125-128: Improved exports, but consider refining file name handlingThe export of
slugifyToCandbuildPostListfunctions is a good practice for modularity. However, there are a couple of points to consider:
As mentioned in a previous review, instead of manually slicing the file name to remove the extension, consider using
path.basenameandpath.extnamefor safer and more reliable handling of file names and extensions.The logic for extracting the version from the file name could be simplified and made more robust. Consider using a regular expression to extract the version number directly:
const versionMatch = file.match(/release-notes-(.+)\.md$/); if (versionMatch) { const version = versionMatch[1]; releaseNotes.push(version); }This approach would be more resilient to changes in file naming conventions.
Also applies to: 162-163
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
scripts/build-post-list.js (2)
Line range hint
63-86: Consider extracting directory processing logicThe nested conditionals and directory processing logic could be simplified by extracting it into a separate function for better readability and maintainability.
+ async function processDirectory(fileName, fileNameWithSection, slugElements, slug, basePath) { + let details; + if (await pathExists(fileNameWithSection)) { + details = frontMatter(await readFile(fileNameWithSection, 'utf-8'), {}).data; + details.title = details.title || capitalize(basename(fileName)); + } else { + details = { + title: capitalize(basename(fileName)), + }; + } + details.isSection = true; + // ... rest of the directory processing logic + return details; + } if (await isDirectory(fileName)) { - if (await pathExists(fileNameWithSection)) { - // ... current directory processing logic - } + const details = await processDirectory(fileName, fileNameWithSection, slugElements, slug, basePath); addItem(details); // ... continue with walkDirectories }
129-132: Use consistent naming conventionsVariable names should follow camelCase convention:
- const fileNameWithoutExtension = basename(file, extname(file)) + const fileNameWithoutExt = basename(file, extname(file))
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
scripts/build-post-list.js(7 hunks)tests/build-post-list.test.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/build-post-list.test.js
🔇 Additional comments (5)
scripts/build-post-list.js (5)
1-2: LGTM! Good improvement in using async fs operations
The switch from synchronous fs operations to async fs-extra is a positive change that can improve performance by preventing blocking operations.
28-45: LGTM! Well-structured function with proper error handling
The changes improve the function's flexibility and robustness:
- Good input validation for required parameters
- Proper error handling with preserved stack traces
- Clear separation of concerns
159-160: LGTM! Clean async implementation
The async conversion is well-implemented and maintains a clear single responsibility.
167-167: LGTM! Clear and explicit exports
Good practice to explicitly export the required functions using a named exports object.
49-53: 🛠️ Refactor suggestion
Consider optimizing async operations in loops
The sequential processing of directories and files with multiple await calls could impact performance. Consider using Promise.all to parallelize these operations.
async function walkDirectories(directories, result, basePath, sectionWeight = 0, sectionTitle, sectionId, rootSectionId) {
- for (let dir of directories) {
- let directory = posix.normalize(dir[0]);
- let sectionSlug = dir[1] || '';
- let files = await readdir(directory)
+ await Promise.all(directories.map(async ([dirPath, sectionSlug = '']) => {
+ const directory = posix.normalize(dirPath);
+ const files = await readdir(directory);
// Process files here
+ }));Likely invalid or redundant comment.
| @@ -1,5 +1,5 @@ | |||
| const { readdirSync, statSync, existsSync, readFileSync, writeFileSync } = require('fs') | |||
| const { resolve, basename } = require('path') | |||
| const { readdir, stat, pathExists, readFile, writeFile } = require('fs-extra') | |||
There was a problem hiding this comment.
Why are we changing this library here?
There was a problem hiding this comment.
When using the fs module, tests might sometimes fail when performing a write operation because the parent directory may not be created for some reason. fs-extra solves this issue by creating the directory if it doesn't exist, and it also offers additional features that reduce the amount of code needed
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
scripts/build-post-list.js (3)
20-26: Consider using an object literal for mapping routes to arraysWhile the current implementation is functional, it could be more maintainable using an object literal for route mapping.
Consider this alternative implementation:
- if (details.slug.startsWith('/docs')) - result["docs"].push(details) - else if (details.slug.startsWith('/blog')) - result["blog"].push(details) - else if (details.slug.startsWith('/about')) - result["about"].push(details) + const routeMap = { + '/docs': 'docs', + '/blog': 'blog', + '/about': 'about' + }; + const route = Object.keys(routeMap).find(key => details.slug.startsWith(key)); + if (route) { + result[routeMap[route]].push(details); + }
31-33: Fix indentation in error messageThe error message has inconsistent indentation.
- throw new Error('Error while building post list: basePath and writeFilePath are required'); + throw new Error('Error while building post list: basePath and writeFilePath are required');
Based on the gathered information, I can now generate the final response:
Add error handling for file operations in walkDirectories function
The
walkDirectoriesfunction contains multiple async operations (readdir, isDirectory, pathExists, readFile) without try-catch blocks, while the parentbuildPostListfunction has proper error handling. This could lead to unhandled promise rejections if file operations fail. Add error handling for:
readdir(directory)operationreadFileoperations for both section and content filesisDirectoryandpathExistschecksThe parent's error handling in
buildPostListwill catch these, but local error handling would provide better error context.🔗 Analysis chain
Line range hint
49-87: Verify error handling in nested async operationsThe nested async operations could potentially fail silently. Let's verify the error handling coverage.
The previous search might have failed due to regex complexity. Let's simplify and broaden the search to understand the error handling context better.
Let's examine the implementation details to better understand the async operations and their error handling context.
We need to see the complete implementation and any shared error handling patterns to make a proper assessment.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for try-catch blocks around async operations rg -U "try\s*\{[\s\S]*?await.*?}.*?catch" scripts/Length of output: 52
Script:
#!/bin/bash # Let's check the entire error handling context in the file rg -A 5 -B 5 "catch|throw|try" scripts/build-post-list.js # Also check if there are any error handling patterns in the calling code rg -l "walkDirectories" scripts/Length of output: 1229
Script:
#!/bin/bash # Let's check the actual implementation of walkDirectories to see all async operations rg -A 10 "async function walkDirectories" scripts/build-post-list.js # Also check for any error handling utilities or wrappers used in the project rg -l "handleError|errorHandler|catch" scripts/Length of output: 1065
Script:
#!/bin/bash # Let's check the error handling in utils.js since it might contain shared error handling patterns rg -A 5 -B 5 "catch|throw|try" scripts/utils.js # Let's also see the full implementation of walkDirectories to understand all async operations rg -A 50 "async function walkDirectories" scripts/build-post-list.jsLength of output: 3441
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
scripts/build-post-list.js(7 hunks)
🔇 Additional comments (3)
scripts/build-post-list.js (3)
1-2: LGTM! Good choice using fs-extra
The switch to fs-extra is a good improvement as it provides better directory handling and reduces boilerplate code.
159-160: LGTM! Clean async conversion
The async conversion of isDirectory is clean and properly implemented.
167-167: LGTM! Clear exports
The exports are well-defined and properly structured.
scripts/build-post-list.js
Outdated
| try { | ||
|
|
||
| if (!basePath || !writeFilePath) { | ||
| throw new Error('Error while building post list: basePath and writeFilePath are required'); |
|
@coderabbitai resolve |
✅ Actions performedComments resolved. |
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (12)
tests/index.test.js (1)
Line range hint
16-36: Test needs to be updated to match new buildPostList implementationThe test suite needs to be updated to account for the significant changes made to the
buildPostListfunction:
- The test doesn't verify the required parameters (postDirectories, basePath, writeFilePath)
- It doesn't account for the asynchronous nature of the function
Here's how to fix these issues:
test('should call all functions in the correct order', async () => { await start(); - expect(buildPostList).toHaveBeenCalled(); + expect(buildPostList).toHaveBeenCalledWith( + expect.arrayContaining([expect.any(String)]), // postDirectories + expect.any(String), // basePath + expect.any(String) // writeFilePath + ); expect(rssFeed).toHaveBeenCalledWith( 'blog', 'AsyncAPI Initiative Blog RSS Feed', 'AsyncAPI Initiative Blog', 'rss.xml' ); expect(buildCaseStudiesList).toHaveBeenCalled(); expect(buildAdoptersList).toHaveBeenCalled(); expect(buildFinanceInfoList).toHaveBeenCalled(); });Additionally, consider adding specific test cases:
test('buildPostList should be called with correct parameters', async () => { await start(); // Add specific expectations for the actual values expect(buildPostList).toHaveBeenCalledWith( ['path/to/posts'], // Replace with actual expected directories 'expected/base/path', // Replace with actual expected base path 'expected/write/path' // Replace with actual expected write path ); }); test('should handle buildPostList errors', async () => { buildPostList.mockRejectedValueOnce(new Error('Test error')); await expect(start()).rejects.toThrow('Test error'); });scripts/index.js (1)
11-15: Consider moving directory mappings to a configuration file.While the current implementation is clear, moving these directory mappings to a separate configuration file (e.g.,
config/post-directories.json) would:
- Make it easier to modify paths without touching the main script
- Follow the pattern of other configurations in the project (like finance data)
- Improve maintainability
Example implementation:
- const postDirectories = [ - ['pages/blog', '/blog'], - ['pages/docs', '/docs'], - ['pages/about', '/about'] - ]; + const postDirectories = require('../config/post-directories.json');Would you like me to help create this configuration file structure?
tests/build-post-list.test.js (6)
19-22: Remove redundant directory creationThe directory creation at lines 19-20 is redundant as it's immediately followed by another creation of the same directory at line 22.
- const normalizedDir = normalize(join(tempDir, 'blog')); - await fs.ensureDir(normalizedDir); - await fs.ensureDir(join(tempDir, 'blog'));
46-49: Enhance output structure verificationThe current assertions only check for property existence. Consider adding more specific type and structure checks.
- expect(output).toHaveProperty('docs'); - expect(output).toHaveProperty('blog'); - expect(output).toHaveProperty('about'); - expect(output).toHaveProperty('docsTree'); + expect(output).toMatchObject({ + docs: expect.arrayContaining([ + expect.objectContaining({ + title: expect.any(String), + slug: expect.any(String) + }) + ]), + blog: expect.arrayContaining([ + expect.objectContaining({ + title: expect.any(String), + slug: expect.any(String) + }) + ]), + about: expect.any(Array), + docsTree: expect.any(Object) + });
90-124: Consolidate specification file testsThe three specification file tests (no title, next-spec, explorer) follow the same pattern and could be consolidated using test.each for better maintainability.
- it('does not process specification files without a title', async () => { - const specDir = join(tempDir, 'docs', 'reference', 'specification'); - await fs.writeFile(join(specDir, 'v2.1.0-no-title.mdx'), '---\n---\nContent of specification without a title.'); - - await buildPostList(postDirectories, tempDir, writeFilePath); - - const output = JSON.parse(await fs.readFile(writeFilePath, 'utf-8')); - const noTitleEntry = output.docs.find(item => item.slug.includes('/reference/specification/v2.1.0-no-title')); - - expect(noTitleEntry).toBeUndefined(); - }); - - it('does not process specification files with "next-spec" in the filename', async () => { - const specDir = join(tempDir, 'docs', 'reference', 'specification'); - await fs.writeFile(join(specDir, 'v2.1.0-next-spec.1.mdx'), '---\n---\nContent of pre-release specification v2.1.0-next-spec.1.'); - - await buildPostList(postDirectories, tempDir, writeFilePath); - - const output = JSON.parse(await fs.readFile(writeFilePath, 'utf-8')); - const nextSpecEntry = output.docs.find(item => item.slug.includes('/reference/specification/v2.1.0-next-spec.1')); - - expect(nextSpecEntry).toBeUndefined(); - }); - - it('does not process specification files with "explorer" in the filename', async () => { - const specDir = join(tempDir, 'docs', 'reference', 'specification'); - await fs.writeFile(join(specDir, 'explorer.mdx'), '---\n---\nContent of explorer specification.'); - - await buildPostList(postDirectories, tempDir, writeFilePath); - - const output = JSON.parse(await fs.readFile(writeFilePath, 'utf-8')); - const explorerEntry = output.docs.find(item => item.slug.includes('/reference/specification/explorer')); - - expect(explorerEntry).toBeUndefined(); - }); + test.each([ + ['no title', 'v2.1.0-no-title.mdx', 'Content of specification without a title.'], + ['next-spec', 'v2.1.0-next-spec.1.mdx', 'Content of pre-release specification v2.1.0-next-spec.1.'], + ['explorer', 'explorer.mdx', 'Content of explorer specification.'] + ])('does not process specification files with %s', async (_, filename, content) => { + const specDir = join(tempDir, 'docs', 'reference', 'specification'); + await fs.writeFile(join(specDir, filename), '---\n---\n' + content); + + await buildPostList(postDirectories, tempDir, writeFilePath); + + const output = JSON.parse(await fs.readFile(writeFilePath, 'utf-8')); + const entry = output.docs.find(item => item.slug.includes(`/reference/specification/${filename.replace('.mdx', '')}`)); + + expect(entry).toBeUndefined(); + });
126-142: Enhance error message assertionsThe error handling tests could be more specific in their error message assertions to ensure the exact error conditions are being tested.
- await expect(buildPostList(postDirectories, tempDir, undefined)).rejects.toThrow(/Error while building post list: basePath and writeFilePath are required/); + await expect(buildPostList(postDirectories, tempDir, undefined)).rejects.toThrow( + expect.objectContaining({ + message: expect.stringMatching(/Error while building post list: basePath and writeFilePath are required/), + name: 'Error' + }) + );
144-160: Add edge cases to slugifyToC testsThe current test suite for slugifyToC could be enhanced with additional edge cases.
describe('slugifyToC', () => { + it('handles multiple IDs in the same heading', () => { + const input = '## My Heading {#first-id} {#second-id}'; + expect(slugifyToC(input)).toBe('first-id'); + }); + + it('handles malformed ID syntax', () => { + const input = '## My Heading {#invalid-id'; + expect(slugifyToC(input)).toBe(''); + }); + + it('handles headings without IDs', () => { + const input = '## Regular Heading'; + expect(slugifyToC(input)).toBe(''); + });
1-161: Consider implementing shared test utilitiesThe test file contains several utility patterns (file creation, output verification) that could be useful across other test files. Consider extracting these into shared test utilities.
- Create a
test-utils.jsfile with common functions:
- File creation helpers
- Output verification helpers
- Common test data setup
- This would improve maintainability and reduce duplication across test files.
scripts/build-post-list.js (4)
32-32: Adjust indentation for consistencyThe indentation on line 32 is deeper than the surrounding code. Align it with the standard indentation level to maintain code readability.
Apply this diff to correct the indentation:
if (!basePath || !writeFilePath) { - throw new Error('Error while building post list: basePath and writeFilePath are required'); + throw new Error('Error while building post list: basePath and writeFilePath are required'); }🧰 Tools
🪛 eslint
[error] 32-32: Delete
······(prettier/prettier)
45-45: Ensure proper error wrapping and stack trace preservationRe-throwing a new error with additional context is good practice. However, consider whether adding the original error message in the new error's message is necessary since the
causeproperty already retains the original error.You can simplify the throw statement:
} catch (error) { - throw new Error(`Error while building post list: ${error.message}`, { cause: error }); + throw new Error('Error while building post list', { cause: error }); }This approach keeps the error message clean while preserving the original error for debugging.
129-132: Improve variable naming for clarity and consistencyConsider renaming
fileNameWithoutExtensiontofileBaseNameto align with existing naming conventions and enhance readability.Apply this diff:
- const fileNameWithoutExtension = basename(file, extname(file)) + const fileBaseName = basename(file, extname(file))Update subsequent references to
fileBaseNameaccordingly.🧰 Tools
🪛 eslint
[error] 129-129: Replace
"release-notes")·&&·dir[1]·===·"/blog"with'release-notes')·&&·dir[1]·===·'/blog'(prettier/prettier)
[error] 130-130: Insert
;(prettier/prettier)
[error] 132-132: Replace
"-")·+·1)with'-')·+·1);(prettier/prettier)
Line range hint
88-102: Handle reading and parsing of markdown files more efficientlyProcessing markdown files can be optimized by checking for file existence and caching results if appropriate.
Consider implementing file existence checks and utilizing caching mechanisms to improve performance.
🧰 Tools
🪛 eslint
[error] 97-97: Insert
;(prettier/prettier)
[error] 98-98: Insert
;(prettier/prettier)
[error] 99-99: Insert
;(prettier/prettier)
[error] 100-100: Insert
;(prettier/prettier)
[error] 101-101: Insert
;(prettier/prettier)
[error] 102-102: Insert
;(prettier/prettier)
[error] 104-104: Replace
·with;(prettier/prettier)
[error] 105-105: Insert
;(prettier/prettier)
[error] 106-106: Unary operator '--' used.
(no-plusplus)
[error] 106-106: Insert
;(prettier/prettier)
[error] 109-109: 'capitalize' was used before it was defined.
(no-use-before-define)
[error] 109-109: Insert
;(prettier/prettier)
[error] 111-111: 'capitalize' was used before it was defined.
(no-use-before-define)
[error] 111-111: Insert
;(prettier/prettier)
[error] 115-115: Insert
;(prettier/prettier)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
scripts/build-post-list.js(7 hunks)scripts/index.js(1 hunks)tests/build-post-list.test.js(1 hunks)tests/index.test.js(1 hunks)
🔇 Additional comments (7)
tests/index.test.js (2)
2-2: LGTM: Import statement correctly updated
The change from default import to named import aligns with the modifications in the source file.
Line range hint 16-36: Verify error handling in the start function
The test suite should include error handling scenarios for all the build functions.
Let's verify if error handling is implemented in the start function:
scripts/index.js (2)
4-4: LGTM! Import statement updated correctly.
The change to a named import using destructuring syntax is appropriate and aligns with the module's new export structure.
16-19: Verify error handling for buildPostList parameters.
The function call looks correct, but let's ensure proper error handling exists for these scenarios:
- Invalid base path
- Write permissions for the JSON file
- Non-existent directories in postDirectories
✅ Verification successful
Let me gather more information about the file writing implementation to complete the verification.
Based on the verification results, I can now provide the final response:
Error handling is properly implemented for buildPostList parameters
The function includes comprehensive error handling:
- Validates required parameters (basePath and writeFilePath)
- Checks for empty postDirectories array
- Uses fs-extra which provides enhanced error handling over native fs
- Wraps all operations in try-catch with detailed error messages and error chaining
The implementation uses fs-extra's writeFile which internally handles common edge cases and provides better error messages than the native fs module. While explicit write permission checks aren't present, fs-extra's writeFile will fail gracefully with clear error messages if permissions are insufficient.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify error handling in buildPostList function
# Check if the function has error handling
echo "Checking error handling implementation..."
rg -A 5 "throw|catch|error" ./scripts/build-post-list.js
# Verify write permissions check
echo "Checking for write permission validation..."
ast-grep --pattern 'fs.access($path, fs.constants.W_OK)'
Length of output: 1082
Script:
#!/bin/bash
# Check file writing implementation
echo "Checking file writing implementation..."
rg -A 5 "writeFile|writeFileSync" ./scripts/build-post-list.js
# Check if fs.promises is used
echo "Checking for fs.promises usage..."
rg "fs.promises" ./scripts/build-post-list.js
# Look for directory existence checks
echo "Checking directory validation..."
rg -A 3 "existsSync|fs.access.*R_OK" ./scripts/build-post-list.js
Length of output: 1320
scripts/build-post-list.js (3)
31-33: Correct error handling for missing parameters
The introduced validation for basePath and writeFilePath ensures that essential parameters are provided before proceeding. This enhances the robustness of the buildPostList function.
🧰 Tools
🪛 eslint
[error] 32-32: Delete ······
(prettier/prettier)
159-160: Consistent use of async functions
The isDirectory function is correctly converted to an asynchronous function and is awaited wherever it's called, ensuring proper execution flow.
🧰 Tools
🪛 eslint
[error] 160-160: Insert ;
(prettier/prettier)
86-86: Avoid deep recursion without termination conditions
The recursive call to walkDirectories could potentially lead to a stack overflow if there is a cyclic directory structure or very deep nesting.
Run the following script to check for cyclic dependencies or excessively deep directory structures:
This script lists directories with a nesting depth greater than 100, which may indicate potential issues.
🧰 Tools
🪛 eslint
[error] 86-86: This line has a length of 135. Maximum allowed is 120.
(max-len)
[error] 86-86: Unexpected await inside a loop.
(no-await-in-loop)
[error] 86-86: Replace [[fileName,·slug]],·resultObj,·basePath,·details.title,·details.sectionId,·rootId,·details.sectionWeight) with ⏎··········[[fileName,·slug]],⏎··········resultObj,⏎··········basePath,⏎··········details.title,⏎··········details.sectionId,⏎··········rootId,⏎··········details.sectionWeight⏎········);
(prettier/prettier)
scripts/build-post-list.js
Outdated
| details.slug = details.isIndex ? sectionSlug : slug.replace(/\.mdx$/, '') | ||
| if(details.slug.includes('/reference/specification/') && !details.title) { | ||
| if (details.slug.includes('/reference/specification/') && !details.title) { | ||
| const fileBaseName = basename(data.slug) // ex. v2.0.0 | v2.1.0-next-spec.1 |
There was a problem hiding this comment.
Correct variable usage for consistency
The variable data.slug may not be defined at this point. Since details.slug has been set, use it instead to avoid potential undefined errors.
Apply this diff to use the correct variable:
- const fileBaseName = basename(data.slug) // ex. v2.0.0 | v2.1.0-next-spec.1
+ const fileBaseName = basename(details.slug) // ex. v2.0.0 | v2.1.0-next-spec.1📝 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 fileBaseName = basename(data.slug) // ex. v2.0.0 | v2.1.0-next-spec.1 | |
| const fileBaseName = basename(details.slug) // ex. v2.0.0 | v2.1.0-next-spec.1 |
🧰 Tools
🪛 eslint
[error] 104-104: Replace · with ;
(prettier/prettier)
There was a problem hiding this comment.
@vishvamsinh28 Is this correct?
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.
yes. replaced it with (data.slug || details.slug)
This way, it defaults to details.slug if data.slug is missing, avoiding any errors.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (4)
scripts/build-post-list.js (4)
19-26: Consider these improvements to the addItem function
- Use dot notation instead of bracket notation for better readability
- Add input validation for the details parameter
const addItem = (details) => { + if (!details?.slug) { + throw new Error('Invalid details object: slug is required'); + } - if (details.slug.startsWith('/docs')) - result["docs"].push(details) - else if (details.slug.startsWith('/blog')) - result["blog"].push(details) - else if (details.slug.startsWith('/about')) - result["about"].push(details) + if (details.slug.startsWith('/docs')) + result.docs.push(details) + else if (details.slug.startsWith('/blog')) + result.blog.push(details) + else if (details.slug.startsWith('/about')) + result.about.push(details) };🧰 Tools
🪛 eslint
[error] 20-21: Replace
⏎····result["docs"].push(details)with·result['docs'].push(details);(prettier/prettier)
[error] 21-21: ["docs"] is better written in dot notation.
(dot-notation)
[error] 22-23: Replace
⏎····result["blog"].push(details)with·result['blog'].push(details);(prettier/prettier)
[error] 23-23: ["blog"] is better written in dot notation.
(dot-notation)
[error] 24-25: Replace
⏎····result["about"].push(details)with·result['about'].push(details);(prettier/prettier)
[error] 25-25: ["about"] is better written in dot notation.
(dot-notation)
49-53: Consider parallelizing directory operations for better performanceThe current implementation uses
awaitinside loops, which processes files sequentially. Consider usingPromise.allto parallelize operations.async function walkDirectories(directories, resultObj, basePath, sectionTitle, sectionId, rootSectionId, sectionWeight = 0) { - for (let dir of directories) { - let directory = posix.normalize(dir[0]); - let sectionSlug = dir[1] || ''; - let files = await readdir(directory) + await Promise.all(directories.map(async ([dirPath, sectionSlug = '']) => { + const directory = posix.normalize(dirPath); + const files = await readdir(directory);🧰 Tools
🪛 eslint
[error] 49-49: This line has a length of 125. Maximum allowed is 120.
(max-len)
[error] 49-49: Replace
directories,·resultObj,·basePath,·sectionTitle,·sectionId,·rootSectionId,·sectionWeight·=·0with⏎··directories,⏎··resultObj,⏎··basePath,⏎··sectionTitle,⏎··sectionId,⏎··rootSectionId,⏎··sectionWeight·=·0⏎(prettier/prettier)
[error] 50-50: 'dir' is never reassigned. Use 'const' instead.
(prefer-const)
[error] 51-51: 'directory' is never reassigned. Use 'const' instead.
(prefer-const)
[error] 52-52: 'sectionSlug' is never reassigned. Use 'const' instead.
(prefer-const)
[error] 53-53: 'files' is never reassigned. Use 'const' instead.
(prefer-const)
[error] 53-53: Unexpected
awaitinside a loop.(no-await-in-loop)
[error] 53-53: Insert
;(prettier/prettier)
129-136: Improve release notes file name handlingThe current implementation could be more robust by using path utilities consistently.
- if (file.startsWith("release-notes") && dir[1] === "/blog") { - const fileNameWithoutExtension = basename(file, extname(file)) - const version = fileNameWithoutExtension.slice(fileNameWithoutExtension.lastIndexOf("-") + 1) + if (file.startsWith('release-notes') && dir[1] === '/blog') { + const version = basename(file, extname(file)) + .split('-') + .slice(-1)[0];🧰 Tools
🪛 eslint
[error] 129-129: Replace
"release-notes")·&&·dir[1]·===·"/blog"with'release-notes')·&&·dir[1]·===·'/blog'(prettier/prettier)
[error] 130-130: Insert
;(prettier/prettier)
[error] 132-132: Replace
"-")·+·1)with'-')·+·1);(prettier/prettier)
[error] 134-134: This line has a length of 164. Maximum allowed is 120.
(max-len)
[error] 135-135: Insert
;(prettier/prettier)
163-165: Consider improving the capitalize functionThe current implementation could be more robust and efficient.
function capitalize(text) { - return text.split(/[\s\-]/g).map(word => `${word[0].toUpperCase()}${word.substr(1)}`).join(' ') + return text + .split(/[\s-]/g) + .map(word => word ? word.charAt(0).toUpperCase() + word.slice(1) : '') + .filter(Boolean) + .join(' ') }🧰 Tools
🪛 eslint
[error] 164-164: Replace
.split(/[\s\-]/g).map(word·=>·${word[0].toUpperCase()}${word.substr(1)}).join('·')with⏎····.split(/[\s\-]/g)⏎····.map((word)·=>·${word[0].toUpperCase()}${word.substr(1)})⏎····.join('·');(prettier/prettier)
[error] 164-164: Unnecessary escape character: -.
(no-useless-escape)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
-
scripts/build-post-list.js(7 hunks)
🧰 Additional context used
🪛 eslint
scripts/build-post-list.js
[error] 1-1: Insert ;
(prettier/prettier)
[error] 2-2: Insert ;
(prettier/prettier)
[error] 20-21: Replace ⏎····result["docs"].push(details) with ·result['docs'].push(details);
(prettier/prettier)
[error] 21-21: ["docs"] is better written in dot notation.
(dot-notation)
[error] 22-23: Replace ⏎····result["blog"].push(details) with ·result['blog'].push(details);
(prettier/prettier)
[error] 23-23: ["blog"] is better written in dot notation.
(dot-notation)
[error] 24-25: Replace ⏎····result["about"].push(details) with ·result['about'].push(details);
(prettier/prettier)
[error] 25-25: ["about"] is better written in dot notation.
(dot-notation)
[error] 29-30: Delete ⏎
(prettier/prettier)
[error] 38-38: Insert ;
(prettier/prettier)
[error] 39-39: 'walkDirectories' was used before it was defined.
(no-use-before-define)
[error] 39-39: Insert ;
(prettier/prettier)
[error] 40-40: Replace "docs"].filter((p)·=>·p.slug.startsWith('/docs/'))) with 'docs'].filter((p)·=>·p.slug.startsWith('/docs/')));
(prettier/prettier)
[error] 40-40: ["docs"] is better written in dot notation.
(dot-notation)
[error] 41-41: Replace "docsTree"]·=·treePosts with 'docsTree']·=·treePosts;
(prettier/prettier)
[error] 41-41: ["docsTree"] is better written in dot notation.
(dot-notation)
[error] 42-42: Replace "docs"]·=·addDocButtons(result["docs"],·treePosts) with 'docs']·=·addDocButtons(result['docs'],·treePosts);
(prettier/prettier)
[error] 42-42: ["docs"] is better written in dot notation.
(dot-notation)
[error] 42-42: ["docs"] is better written in dot notation.
(dot-notation)
[error] 43-43: Insert ;
(prettier/prettier)
[error] 49-49: This line has a length of 125. Maximum allowed is 120.
(max-len)
[error] 49-49: Replace directories,·resultObj,·basePath,·sectionTitle,·sectionId,·rootSectionId,·sectionWeight·=·0 with ⏎··directories,⏎··resultObj,⏎··basePath,⏎··sectionTitle,⏎··sectionId,⏎··rootSectionId,⏎··sectionWeight·=·0⏎
(prettier/prettier)
[error] 50-50: 'dir' is never reassigned. Use 'const' instead.
(prefer-const)
[error] 51-51: 'directory' is never reassigned. Use 'const' instead.
(prefer-const)
[error] 52-52: 'sectionSlug' is never reassigned. Use 'const' instead.
(prefer-const)
[error] 53-53: 'files' is never reassigned. Use 'const' instead.
(prefer-const)
[error] 53-53: Unexpected await inside a loop.
(no-await-in-loop)
[error] 53-53: Insert ;
(prettier/prettier)
[error] 55-55: 'file' is never reassigned. Use 'const' instead.
(prefer-const)
[error] 58-58: Insert ;
(prettier/prettier)
[error] 59-59: Insert ;
(prettier/prettier)
[error] 60-60: Replace '/') with ·'/');
(prettier/prettier)
[error] 61-61: Insert ;
(prettier/prettier)
[error] 63-63: Unexpected await inside a loop.
(no-await-in-loop)
[error] 63-63: 'isDirectory' was used before it was defined.
(no-use-before-define)
[error] 64-64: Unexpected await inside a loop.
(no-await-in-loop)
[error] 66-66: Unexpected await inside a loop.
(no-await-in-loop)
[error] 66-66: Insert ;
(prettier/prettier)
[error] 75-75: Insert ;
(prettier/prettier)
[error] 76-76: Insert ;
(prettier/prettier)
[error] 86-86: This line has a length of 135. Maximum allowed is 120.
(max-len)
[error] 86-86: Unexpected await inside a loop.
(no-await-in-loop)
[error] 86-86: Replace [[fileName,·slug]],·resultObj,·basePath,·details.title,·details.sectionId,·rootId,·details.sectionWeight) with ⏎··········[[fileName,·slug]],⏎··········resultObj,⏎··········basePath,⏎··········details.title,⏎··········details.sectionId,⏎··········rootId,⏎··········details.sectionWeight⏎········);
(prettier/prettier)
[error] 87-87: Unexpected string concatenation.
(prefer-template)
[error] 88-88: Unexpected await inside a loop.
(no-await-in-loop)
[error] 88-88: Insert ;
(prettier/prettier)
[error] 100-100: Insert ;
(prettier/prettier)
[error] 101-101: Insert ;
(prettier/prettier)
[error] 102-102: Insert ;
(prettier/prettier)
[error] 104-104: Insert ;
(prettier/prettier)
[error] 105-105: Insert ;
(prettier/prettier)
[error] 106-106: Unary operator '--' used.
(no-plusplus)
[error] 106-106: Insert ;
(prettier/prettier)
[error] 109-109: 'capitalize' was used before it was defined.
(no-use-before-define)
[error] 109-109: Insert ;
(prettier/prettier)
[error] 111-111: 'capitalize' was used before it was defined.
(no-use-before-define)
[error] 111-111: Insert ;
(prettier/prettier)
[error] 129-129: Replace "release-notes")·&&·dir[1]·===·"/blog" with 'release-notes')·&&·dir[1]·===·'/blog'
(prettier/prettier)
[error] 130-130: Insert ;
(prettier/prettier)
[error] 132-132: Replace "-")·+·1) with '-')·+·1);
(prettier/prettier)
[error] 134-134: This line has a length of 164. Maximum allowed is 120.
(max-len)
[error] 160-160: Insert ;
(prettier/prettier)
[error] 164-164: Replace .split(/[\s\-]/g).map(word·=>·${word[0].toUpperCase()}${word.substr(1)}).join('·') with ⏎····.split(/[\s\-]/g)⏎····.map((word)·=>·${word[0].toUpperCase()}${word.substr(1)})⏎····.join('·');
(prettier/prettier)
[error] 164-164: Unnecessary escape character: -.
(no-useless-escape)
[error] 167-167: Insert ;
(prettier/prettier)
🔇 Additional comments (2)
scripts/build-post-list.js (2)
1-2: LGTM: Good choice using fs-extra!
The switch to fs-extra is a good improvement as it provides more robust directory handling and automatically creates directories when needed, reducing potential edge cases.
🧰 Tools
🪛 eslint
[error] 1-1: Insert ;
(prettier/prettier)
[error] 2-2: Insert ;
(prettier/prettier)
28-46: LGTM: Good error handling improvements!
The addition of parameter validation and proper error handling with the cause property is excellent. The error messages are clear and descriptive.
🧰 Tools
🪛 eslint
[error] 29-30: Delete ⏎
(prettier/prettier)
[error] 38-38: Insert ;
(prettier/prettier)
[error] 39-39: 'walkDirectories' was used before it was defined.
(no-use-before-define)
[error] 39-39: Insert ;
(prettier/prettier)
[error] 40-40: Replace "docs"].filter((p)·=>·p.slug.startsWith('/docs/'))) with 'docs'].filter((p)·=>·p.slug.startsWith('/docs/')));
(prettier/prettier)
[error] 40-40: ["docs"] is better written in dot notation.
(dot-notation)
[error] 41-41: Replace "docsTree"]·=·treePosts with 'docsTree']·=·treePosts;
(prettier/prettier)
[error] 41-41: ["docsTree"] is better written in dot notation.
(dot-notation)
[error] 42-42: Replace "docs"]·=·addDocButtons(result["docs"],·treePosts) with 'docs']·=·addDocButtons(result['docs'],·treePosts);
(prettier/prettier)
[error] 42-42: ["docs"] is better written in dot notation.
(dot-notation)
[error] 42-42: ["docs"] is better written in dot notation.
(dot-notation)
[error] 43-43: Insert ;
(prettier/prettier)
scripts/build-post-list.js
Outdated
| if (details.slug.includes('/reference/specification/') && !details.title) { | ||
| const fileBaseName = basename(data.slug || details.slug) // ex. v2.0.0 | v2.1.0-next-spec.1 | ||
| const versionName = fileBaseName.split('-')[0] // v2.0.0 | v2.1.0 | ||
| details.weight = specWeight-- | ||
|
|
||
| if (fileName.startsWith('v')) { | ||
| details.title = capitalize(fileName.slice(1)) | ||
| if (versionName.startsWith('v')) { | ||
| details.title = capitalize(versionName.slice(1)) | ||
| } else { | ||
| details.title = capitalize(fileName) | ||
| details.title = capitalize(versionName) |
There was a problem hiding this comment.
Fix inconsistent variable usage and improve version name handling
The code uses both data.slug and details.slug, which could lead to undefined behavior. Also, the version name handling could be more robust.
- if (details.slug.includes('/reference/specification/') && !details.title) {
- const fileBaseName = basename(data.slug || details.slug) // ex. v2.0.0 | v2.1.0-next-spec.1
- const versionName = fileBaseName.split('-')[0] // v2.0.0 | v2.1.0
+ if (details.slug.includes('/reference/specification/') && !details.title) {
+ const fileBaseName = basename(details.slug) // ex. v2.0.0 | v2.1.0-next-spec.1
+ const versionName = fileBaseName.split('-')[0] // v2.0.0 | v2.1.0📝 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.
| if (details.slug.includes('/reference/specification/') && !details.title) { | |
| const fileBaseName = basename(data.slug || details.slug) // ex. v2.0.0 | v2.1.0-next-spec.1 | |
| const versionName = fileBaseName.split('-')[0] // v2.0.0 | v2.1.0 | |
| details.weight = specWeight-- | |
| if (fileName.startsWith('v')) { | |
| details.title = capitalize(fileName.slice(1)) | |
| if (versionName.startsWith('v')) { | |
| details.title = capitalize(versionName.slice(1)) | |
| } else { | |
| details.title = capitalize(fileName) | |
| details.title = capitalize(versionName) | |
| if (details.slug.includes('/reference/specification/') && !details.title) { | |
| const fileBaseName = basename(details.slug) // ex. v2.0.0 | v2.1.0-next-spec.1 | |
| const versionName = fileBaseName.split('-')[0] // v2.0.0 | v2.1.0 | |
| details.weight = specWeight-- | |
| if (versionName.startsWith('v')) { | |
| details.title = capitalize(versionName.slice(1)) | |
| } else { | |
| details.title = capitalize(versionName) |
🧰 Tools
🪛 eslint
[error] 104-104: Insert ;
(prettier/prettier)
[error] 105-105: Insert ;
(prettier/prettier)
[error] 106-106: Unary operator '--' used.
(no-plusplus)
[error] 106-106: Insert ;
(prettier/prettier)
[error] 109-109: 'capitalize' was used before it was defined.
(no-use-before-define)
[error] 109-109: Insert ;
(prettier/prettier)
[error] 111-111: 'capitalize' was used before it was defined.
(no-use-before-define)
[error] 111-111: Insert ;
(prettier/prettier)
There was a problem hiding this comment.
@vishvamsinh28 Apply this quick fix
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 done
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (3)
scripts/build-post-list.js (3)
19-26: Consider these improvements to the addItem function
- Use dot notation instead of array notation for better readability
- Add type checking for the details parameter
const addItem = (details) => { + if (!details || typeof details.slug !== 'string') { + throw new Error('Invalid details object'); + } if (details.slug.startsWith('/docs')) - result["docs"].push(details) + result.docs.push(details); else if (details.slug.startsWith('/blog')) - result["blog"].push(details) + result.blog.push(details); else if (details.slug.startsWith('/about')) - result["about"].push(details) + result.about.push(details); };🧰 Tools
🪛 eslint
[error] 20-21: Replace
⏎····result["docs"].push(details)with·result['docs'].push(details);(prettier/prettier)
[error] 21-21: ["docs"] is better written in dot notation.
(dot-notation)
[error] 22-23: Replace
⏎····result["blog"].push(details)with·result['blog'].push(details);(prettier/prettier)
[error] 23-23: ["blog"] is better written in dot notation.
(dot-notation)
[error] 24-25: Replace
⏎····result["about"].push(details)with·result['about'].push(details);(prettier/prettier)
[error] 25-25: ["about"] is better written in dot notation.
(dot-notation)
129-136: Improve release notes file name handlingThe current implementation uses string manipulation for parsing version numbers. Consider using a more robust approach with regular expressions.
- if (file.startsWith("release-notes") && dir[1] === "/blog") { - const fileNameWithoutExtension = basename(file, extname(file)) - const version = fileNameWithoutExtension.slice(fileNameWithoutExtension.lastIndexOf("-") + 1) + if (file.startsWith('release-notes') && dir[1] === '/blog') { + const versionMatch = file.match(/release-notes-(.+?)\./); + if (versionMatch) { + const version = versionMatch[1]; + releaseNotes.push(version); + } - releaseNotes.push(version) }🧰 Tools
🪛 eslint
[error] 129-129: Replace
"release-notes")·&&·dir[1]·===·"/blog"with'release-notes')·&&·dir[1]·===·'/blog'(prettier/prettier)
[error] 130-130: Insert
;(prettier/prettier)
[error] 132-132: Replace
"-")·+·1)with'-')·+·1);(prettier/prettier)
[error] 134-134: This line has a length of 164. Maximum allowed is 120.
(max-len)
[error] 135-135: Insert
;(prettier/prettier)
163-165: Improve the capitalize functionThe current implementation has an unnecessary escape character and could be simplified.
function capitalize(text) { - return text.split(/[\s\-]/g).map(word => `${word[0].toUpperCase()}${word.substr(1)}`).join(' ') + return text.split(/[\s-]/g) + .map(word => word.charAt(0).toUpperCase() + word.slice(1)) + .join(' '); }🧰 Tools
🪛 eslint
[error] 164-164: Replace
.split(/[\s\-]/g).map(word·=>·${word[0].toUpperCase()}${word.substr(1)}).join('·')with⏎····.split(/[\s\-]/g)⏎····.map((word)·=>·${word[0].toUpperCase()}${word.substr(1)})⏎····.join('·');(prettier/prettier)
[error] 164-164: Unnecessary escape character: -.
(no-useless-escape)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
-
scripts/build-post-list.js(7 hunks)
🧰 Additional context used
🪛 eslint
scripts/build-post-list.js
[error] 1-1: Insert ;
(prettier/prettier)
[error] 2-2: Insert ;
(prettier/prettier)
[error] 20-21: Replace ⏎····result["docs"].push(details) with ·result['docs'].push(details);
(prettier/prettier)
[error] 21-21: ["docs"] is better written in dot notation.
(dot-notation)
[error] 22-23: Replace ⏎····result["blog"].push(details) with ·result['blog'].push(details);
(prettier/prettier)
[error] 23-23: ["blog"] is better written in dot notation.
(dot-notation)
[error] 24-25: Replace ⏎····result["about"].push(details) with ·result['about'].push(details);
(prettier/prettier)
[error] 25-25: ["about"] is better written in dot notation.
(dot-notation)
[error] 29-30: Delete ⏎
(prettier/prettier)
[error] 38-38: Insert ;
(prettier/prettier)
[error] 39-39: 'walkDirectories' was used before it was defined.
(no-use-before-define)
[error] 39-39: Insert ;
(prettier/prettier)
[error] 40-40: Replace "docs"].filter((p)·=>·p.slug.startsWith('/docs/'))) with 'docs'].filter((p)·=>·p.slug.startsWith('/docs/')));
(prettier/prettier)
[error] 40-40: ["docs"] is better written in dot notation.
(dot-notation)
[error] 41-41: Replace "docsTree"]·=·treePosts with 'docsTree']·=·treePosts;
(prettier/prettier)
[error] 41-41: ["docsTree"] is better written in dot notation.
(dot-notation)
[error] 42-42: Replace "docs"]·=·addDocButtons(result["docs"],·treePosts) with 'docs']·=·addDocButtons(result['docs'],·treePosts);
(prettier/prettier)
[error] 42-42: ["docs"] is better written in dot notation.
(dot-notation)
[error] 42-42: ["docs"] is better written in dot notation.
(dot-notation)
[error] 43-43: Insert ;
(prettier/prettier)
[error] 49-49: This line has a length of 125. Maximum allowed is 120.
(max-len)
[error] 49-49: Replace directories,·resultObj,·basePath,·sectionTitle,·sectionId,·rootSectionId,·sectionWeight·=·0 with ⏎··directories,⏎··resultObj,⏎··basePath,⏎··sectionTitle,⏎··sectionId,⏎··rootSectionId,⏎··sectionWeight·=·0⏎
(prettier/prettier)
[error] 50-50: 'dir' is never reassigned. Use 'const' instead.
(prefer-const)
[error] 51-51: 'directory' is never reassigned. Use 'const' instead.
(prefer-const)
[error] 52-52: 'sectionSlug' is never reassigned. Use 'const' instead.
(prefer-const)
[error] 53-53: 'files' is never reassigned. Use 'const' instead.
(prefer-const)
[error] 53-53: Unexpected await inside a loop.
(no-await-in-loop)
[error] 53-53: Insert ;
(prettier/prettier)
[error] 55-55: 'file' is never reassigned. Use 'const' instead.
(prefer-const)
[error] 58-58: Insert ;
(prettier/prettier)
[error] 59-59: Insert ;
(prettier/prettier)
[error] 60-60: Replace '/') with ·'/');
(prettier/prettier)
[error] 61-61: Insert ;
(prettier/prettier)
[error] 63-63: Unexpected await inside a loop.
(no-await-in-loop)
[error] 63-63: 'isDirectory' was used before it was defined.
(no-use-before-define)
[error] 64-64: Unexpected await inside a loop.
(no-await-in-loop)
[error] 66-66: Unexpected await inside a loop.
(no-await-in-loop)
[error] 66-66: Insert ;
(prettier/prettier)
[error] 75-75: Insert ;
(prettier/prettier)
[error] 76-76: Insert ;
(prettier/prettier)
[error] 86-86: This line has a length of 135. Maximum allowed is 120.
(max-len)
[error] 86-86: Unexpected await inside a loop.
(no-await-in-loop)
[error] 86-86: Replace [[fileName,·slug]],·resultObj,·basePath,·details.title,·details.sectionId,·rootId,·details.sectionWeight) with ⏎··········[[fileName,·slug]],⏎··········resultObj,⏎··········basePath,⏎··········details.title,⏎··········details.sectionId,⏎··········rootId,⏎··········details.sectionWeight⏎········);
(prettier/prettier)
[error] 87-87: Unexpected string concatenation.
(prefer-template)
[error] 88-88: Unexpected await inside a loop.
(no-await-in-loop)
[error] 88-88: Insert ;
(prettier/prettier)
[error] 100-100: Insert ;
(prettier/prettier)
[error] 101-101: Insert ;
(prettier/prettier)
[error] 102-102: Insert ;
(prettier/prettier)
[error] 104-104: Replace · with ;
(prettier/prettier)
[error] 105-105: Insert ;
(prettier/prettier)
[error] 106-106: Unary operator '--' used.
(no-plusplus)
[error] 106-106: Insert ;
(prettier/prettier)
[error] 109-109: 'capitalize' was used before it was defined.
(no-use-before-define)
[error] 109-109: Insert ;
(prettier/prettier)
[error] 111-111: 'capitalize' was used before it was defined.
(no-use-before-define)
[error] 111-111: Insert ;
(prettier/prettier)
[error] 129-129: Replace "release-notes")·&&·dir[1]·===·"/blog" with 'release-notes')·&&·dir[1]·===·'/blog'
(prettier/prettier)
[error] 130-130: Insert ;
(prettier/prettier)
[error] 132-132: Replace "-")·+·1) with '-')·+·1);
(prettier/prettier)
[error] 134-134: This line has a length of 164. Maximum allowed is 120.
(max-len)
[error] 160-160: Insert ;
(prettier/prettier)
[error] 164-164: Replace .split(/[\s\-]/g).map(word·=>·${word[0].toUpperCase()}${word.substr(1)}).join('·') with ⏎····.split(/[\s\-]/g)⏎····.map((word)·=>·${word[0].toUpperCase()}${word.substr(1)})⏎····.join('·');
(prettier/prettier)
[error] 164-164: Unnecessary escape character: -.
(no-useless-escape)
[error] 167-167: Insert ;
(prettier/prettier)
🔇 Additional comments (1)
scripts/build-post-list.js (1)
1-2: LGTM: Good choice using fs-extra!
The switch to fs-extra is beneficial as it provides better error handling and automatic directory creation during write operations.
🧰 Tools
🪛 eslint
[error] 1-1: Insert ;
(prettier/prettier)
[error] 2-2: Insert ;
(prettier/prettier)
| async function walkDirectories(directories, resultObj, basePath, sectionTitle, sectionId, rootSectionId, sectionWeight = 0) { | ||
| for (let dir of directories) { | ||
| let directory = dir[0] | ||
| let sectionSlug = dir[1] || '' | ||
| let files = readdirSync(directory); | ||
| let directory = posix.normalize(dir[0]); | ||
| let sectionSlug = dir[1] || ''; | ||
| let files = await readdir(directory) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Optimize async operations in directory traversal
Multiple sequential await calls in loops can impact performance. Consider using Promise.all to parallelize file operations.
async function walkDirectories(directories, resultObj, basePath, sectionTitle, sectionId, rootSectionId, sectionWeight = 0) {
- for (let dir of directories) {
- let directory = posix.normalize(dir[0]);
- let sectionSlug = dir[1] || '';
- let files = await readdir(directory)
+ await Promise.all(directories.map(async ([dirPath, sectionSlug = '']) => {
+ const directory = posix.normalize(dirPath);
+ const files = await readdir(directory);
// ... rest of the function
+ }));
}📝 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.
| async function walkDirectories(directories, resultObj, basePath, sectionTitle, sectionId, rootSectionId, sectionWeight = 0) { | |
| for (let dir of directories) { | |
| let directory = dir[0] | |
| let sectionSlug = dir[1] || '' | |
| let files = readdirSync(directory); | |
| let directory = posix.normalize(dir[0]); | |
| let sectionSlug = dir[1] || ''; | |
| let files = await readdir(directory) | |
| async function walkDirectories(directories, resultObj, basePath, sectionTitle, sectionId, rootSectionId, sectionWeight = 0) { | |
| await Promise.all(directories.map(async ([dirPath, sectionSlug = '']) => { | |
| const directory = posix.normalize(dirPath); | |
| const files = await readdir(directory); | |
| // ... rest of the function | |
| })); | |
| } |
🧰 Tools
🪛 eslint
[error] 49-49: This line has a length of 125. Maximum allowed is 120.
(max-len)
[error] 49-49: Replace directories,·resultObj,·basePath,·sectionTitle,·sectionId,·rootSectionId,·sectionWeight·=·0 with ⏎··directories,⏎··resultObj,⏎··basePath,⏎··sectionTitle,⏎··sectionId,⏎··rootSectionId,⏎··sectionWeight·=·0⏎
(prettier/prettier)
[error] 50-50: 'dir' is never reassigned. Use 'const' instead.
(prefer-const)
[error] 51-51: 'directory' is never reassigned. Use 'const' instead.
(prefer-const)
[error] 52-52: 'sectionSlug' is never reassigned. Use 'const' instead.
(prefer-const)
[error] 53-53: 'files' is never reassigned. Use 'const' instead.
(prefer-const)
[error] 53-53: Unexpected await inside a loop.
(no-await-in-loop)
[error] 53-53: Insert ;
(prettier/prettier)
There was a problem hiding this comment.
tests are failing after applying this suggestion
There was a problem hiding this comment.
@vishvamsinh28, understood. It appears that the suggested change is causing tests to fail due to concurrent modifications to the resultObj in walkDirectories. The sequential execution in the loop is necessary to maintain the correct state. It's best to retain the original loop structure to ensure the code functions properly.
✏️ Learnings added
Learnt from: vishvamsinh28
PR: asyncapi/website#3284
File: scripts/build-post-list.js:49-53
Timestamp: 2024-11-10T18:16:22.063Z
Learning: In `scripts/build-post-list.js`, the `walkDirectories` function depends on sequential execution because it mutates `resultObj`. Parallelizing the loop with `Promise.all` causes tests to fail.
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.
Don't add it as a learning. We will see it later if it can be implemented inside this function or not.
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.
@coderabbitai Don't add it as a learning. We will see it later if it can be implemented inside this function or not.
There was a problem hiding this comment.
@akshatnema, understood. I've removed the learning as per your request.
| async function buildPostList(postDirectories, basePath, writeFilePath) { | ||
| try { | ||
|
|
||
| if (!basePath || !writeFilePath) { | ||
| throw new Error('Error while building post list: basePath and writeFilePath are required'); | ||
| } | ||
|
|
||
| module.exports = async function buildPostList() { | ||
| walkDirectories(postDirectories, result) | ||
| const treePosts = buildNavTree(result["docs"].filter((p) => p.slug.startsWith('/docs/'))) | ||
| result["docsTree"] = treePosts | ||
| result["docs"] = addDocButtons(result["docs"], treePosts) | ||
| if (process.env.NODE_ENV === 'production') { | ||
| // console.log(inspect(result, { depth: null, colors: true })) | ||
| if (postDirectories.length === 0) { | ||
| throw new Error('Error while building post list: No post directories provided'); | ||
| } | ||
| const normalizedBasePath = normalize(basePath) | ||
| await walkDirectories(postDirectories, result, normalizedBasePath) | ||
| const treePosts = buildNavTree(result["docs"].filter((p) => p.slug.startsWith('/docs/'))) | ||
| result["docsTree"] = treePosts | ||
| result["docs"] = addDocButtons(result["docs"], treePosts) | ||
| await writeFile(writeFilePath, JSON.stringify(result, null, ' ')) | ||
| } catch (error) { | ||
| throw new Error(`Error while building post list: ${error.message}`, { cause: error }); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider avoiding global state mutation
The function mutates the global result object, which could lead to issues in concurrent scenarios or when running multiple builds. Consider making the result object local to the function.
-const result = {
- docs: [],
- blog: [],
- about: [],
- docsTree: {}
-}
async function buildPostList(postDirectories, basePath, writeFilePath) {
try {
+ const result = {
+ docs: [],
+ blog: [],
+ about: [],
+ docsTree: {}
+ };
if (!basePath || !writeFilePath) {
throw new Error('Error while building post list: basePath and writeFilePath are required');
}
// ... rest of the function
+ return result;
}
}Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 eslint
[error] 29-30: Delete ⏎
(prettier/prettier)
[error] 38-38: Insert ;
(prettier/prettier)
[error] 39-39: 'walkDirectories' was used before it was defined.
(no-use-before-define)
[error] 39-39: Insert ;
(prettier/prettier)
[error] 40-40: Replace "docs"].filter((p)·=>·p.slug.startsWith('/docs/'))) with 'docs'].filter((p)·=>·p.slug.startsWith('/docs/')));
(prettier/prettier)
[error] 40-40: ["docs"] is better written in dot notation.
(dot-notation)
[error] 41-41: Replace "docsTree"]·=·treePosts with 'docsTree']·=·treePosts;
(prettier/prettier)
[error] 41-41: ["docsTree"] is better written in dot notation.
(dot-notation)
[error] 42-42: Replace "docs"]·=·addDocButtons(result["docs"],·treePosts) with 'docs']·=·addDocButtons(result['docs'],·treePosts);
(prettier/prettier)
[error] 42-42: ["docs"] is better written in dot notation.
(dot-notation)
[error] 42-42: ["docs"] is better written in dot notation.
(dot-notation)
[error] 43-43: Insert ;
(prettier/prettier)
There was a problem hiding this comment.
same for this as well tests are failing after applying this suggestion
There was a problem hiding this comment.
@vishvamsinh28, understood. If moving the result object inside the function causes tests to fail, it's acceptable to keep it as a global variable. I've noted this for future reviews.
(っ◞‸◟c)
✏️ Learnings added
Learnt from: vishvamsinh28
PR: asyncapi/website#3284
File: scripts/build-post-list.js:28-46
Timestamp: 2024-11-10T18:16:35.326Z
Learning: In the `scripts/build-post-list.js` JavaScript file, tests rely on the `result` object being a global variable. Moving it inside the `buildPostList` function causes tests to fail; therefore, keep `result` as a global variable.
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.
|
@akshatnema Are we good to merge this PR, or do I need to change anything else? |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (7)
tests/build-post-list.test.js (7)
19-22: Remove redundant directory creationThe directory creation at line 19-20 is redundant as it's immediately followed by another creation of the same directory at line 22.
- const normalizedDir = normalize(join(tempDir, 'blog')); - await fs.ensureDir(normalizedDir); - await fs.ensureDir(join(tempDir, 'blog'));
34-36: Add cleanup verificationConsider adding verification that the temporary directory is properly cleaned up after each test.
afterEach(async () => { await fs.remove(tempDir); + const exists = await fs.pathExists(tempDir); + expect(exists).toBe(false); });
44-54: Enhance output structure validationThe test could be more thorough in validating the complete output structure.
const output = JSON.parse(await fs.readFile(writeFilePath, 'utf-8')); - expect(output).toHaveProperty('docs'); - expect(output).toHaveProperty('blog'); - expect(output).toHaveProperty('about'); - expect(output).toHaveProperty('docsTree'); + expect(output).toMatchObject({ + docs: expect.arrayContaining([ + expect.objectContaining({ + title: 'Docs Home', + slug: '/docs/index' + }) + ]), + blog: expect.arrayContaining([ + expect.objectContaining({ + title: 'Release Notes 2.1.0', + slug: '/blog/release-notes-2.1.0' + }) + ]), + about: expect.arrayContaining([ + expect.objectContaining({ + title: 'About Us', + slug: '/about/index' + }) + ]), + docsTree: expect.any(Object) + });🧰 Tools
🪛 eslint
[error] 51-51: Replace
(itemwith((item)(prettier/prettier)
64-65: Make section file assertions more specificThe current assertions could be more specific about the expected structure of section files in the output.
- expect(output.docs.length).toBeGreaterThan(0); - expect(output.docs.find(item => item.title === 'Section 1')).toBeDefined(); + const sectionEntry = output.docs.find(item => item.title === 'Section 1'); + expect(sectionEntry).toMatchObject({ + title: 'Section 1', + slug: expect.stringContaining('/docs/section1'), + isSection: true + });🧰 Tools
🪛 eslint
[error] 65-65: Replace
itemwith(item)(prettier/prettier)
75-82: Enhance release notes validationConsider adding assertions for release notes ordering and additional properties.
const firstReleaseNote = output.blog.find(item => item.slug === '/blog/release-notes-2.1.0'); const secondReleaseNote = output.blog.find(item => item.slug === '/blog/release-notes-2.1.1'); - expect(firstReleaseNote).toBeDefined(); - expect(firstReleaseNote.title).toBe('Release Notes 2.1.0'); - - expect(secondReleaseNote).toBeDefined(); - expect(secondReleaseNote.title).toBe('Release Notes 2.1.1'); + expect(firstReleaseNote).toMatchObject({ + title: 'Release Notes 2.1.0', + slug: '/blog/release-notes-2.1.0', + type: 'release-note' + }); + expect(secondReleaseNote).toMatchObject({ + title: 'Release Notes 2.1.1', + slug: '/blog/release-notes-2.1.1', + type: 'release-note' + }); + + // Verify release notes are ordered correctly in the array + const releaseNotes = output.blog.filter(item => item.type === 'release-note'); + expect(releaseNotes).toEqual([secondReleaseNote, firstReleaseNote]);🧰 Tools
🪛 eslint
[error] 75-75: Replace
itemwith(item)(prettier/prettier)
[error] 76-76: Replace
itemwith(item)(prettier/prettier)
126-142: Make error message assertions more specificThe error handling tests could be more specific about the expected error messages.
it('throws an error if the front matter cannot be parsed', async () => { await fs.writeFile(join(tempDir, 'docs', 'invalid.mdx'), '---\ninvalid front matter\n---\nContent'); - await expect(buildPostList(postDirectories, tempDir, writeFilePath)).rejects.toThrow(/Error while building post list/); + await expect(buildPostList(postDirectories, tempDir, writeFilePath)) + .rejects + .toThrow('Error while building post list: Invalid front matter in file'); }); it('throws an error if no post directories are provided', async () => { - await expect(buildPostList([], tempDir, writeFilePath)).rejects.toThrow(/Error while building post list/); + await expect(buildPostList([], tempDir, writeFilePath)) + .rejects + .toThrow('Error while building post list: No post directories provided'); });🧰 Tools
🪛 eslint
[error] 128-128: Delete
··(prettier/prettier)
[error] 129-129: This line has a length of 123. Maximum allowed is 120.
(max-len)
[error] 129-129: Replace
/Error·while·building·post·list/with⏎······/Error·while·building·post·list/⏎····(prettier/prettier)
[error] 131-131: Delete
··(prettier/prettier)
[error] 135-135: Delete
··(prettier/prettier)
[error] 137-137: This line has a length of 166. Maximum allowed is 120.
(max-len)
[error] 137-137: Replace
/Error·while·building·post·list:·basePath·and·writeFilePath·are·required/with⏎······/Error·while·building·post·list:·basePath·and·writeFilePath·are·required/⏎····(prettier/prettier)
[error] 139-139: Delete
··(prettier/prettier)
[error] 141-141: This line has a length of 160. Maximum allowed is 120.
(max-len)
[error] 141-141: Replace
/Error·while·building·post·list:·basePath·and·writeFilePath·are·required/with⏎······/Error·while·building·post·list:·basePath·and·writeFilePath·are·required/⏎····(prettier/prettier)
[error] 142-142: Delete
··(prettier/prettier)
144-159: Add edge cases to slugifyToC testsConsider adding test cases for additional scenarios:
describe('slugifyToC', () => { + it('handles multiple IDs in the same heading', () => { + const input = '## My Heading {#first-id} {#second-id}'; + expect(slugifyToC(input)).toBe('first-id'); + }); + + it('handles malformed ID syntax', () => { + const input = '## My Heading {#invalid-id'; + expect(slugifyToC(input)).toBe(''); + }); + + it('handles heading without any ID', () => { + const input = '## My Heading'; + expect(slugifyToC(input)).toBe(''); + }); // ... existing tests ... });🧰 Tools
🪛 eslint
[error] 144-145: Delete
⏎(prettier/prettier)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
tests/build-post-list.test.js(1 hunks)
🧰 Additional context used
🪛 eslint
tests/build-post-list.test.js
[error] 16-16: Delete ,
(prettier/prettier)
[error] 23-23: Replace join(tempDir,·'blog',·'release-notes-2.1.0.mdx'),·'---\ntitle:·Release·Notes·2.1.0\n---\nThis·is·a·release·note.' with ⏎······join(tempDir,·'blog',·'release-notes-2.1.0.mdx'),⏎······'---\ntitle:·Release·Notes·2.1.0\n---\nThis·is·a·release·note.'⏎····
(prettier/prettier)
[error] 26-26: Replace join(tempDir,·'docs',·'index.mdx'),·'---\ntitle:·Docs·Home\n---\nThis·is·the·documentation·homepage.' with ⏎······join(tempDir,·'docs',·'index.mdx'),⏎······'---\ntitle:·Docs·Home\n---\nThis·is·the·documentation·homepage.'⏎····
(prettier/prettier)
[error] 51-51: Replace (item with ((item)
(prettier/prettier)
[error] 58-58: Replace join(tempDir,·'docs',·'section1',·'_section.mdx'),·'---\ntitle:·Section·1\n---\nThis·is·section·1.' with ⏎······join(tempDir,·'docs',·'section1',·'_section.mdx'),⏎······'---\ntitle:·Section·1\n---\nThis·is·section·1.'⏎····
(prettier/prettier)
[error] 65-65: Replace item with (item)
(prettier/prettier)
[error] 69-69: Replace join(tempDir,·'blog',·'release-notes-2.1.1.mdx'),·'---\ntitle:·Release·Notes·2.1.1\n---\nThis·is·a·release·note.' with ⏎······join(tempDir,·'blog',·'release-notes-2.1.1.mdx'),⏎······'---\ntitle:·Release·Notes·2.1.1\n---\nThis·is·a·release·note.'⏎····
(prettier/prettier)
[error] 75-75: Replace item with (item)
(prettier/prettier)
[error] 76-76: Replace item with (item)
(prettier/prettier)
[error] 97-97: Replace item with (item)
(prettier/prettier)
[error] 104-104: Replace join(specDir,·'v2.1.0-next-spec.1.mdx'),·'---\n---\nContent·of·pre-release·specification·v2.1.0-next-spec.1.' with ⏎······join(specDir,·'v2.1.0-next-spec.1.mdx'),⏎······'---\n---\nContent·of·pre-release·specification·v2.1.0-next-spec.1.'⏎····
(prettier/prettier)
[error] 109-109: Replace item with (item)
(prettier/prettier)
[error] 121-121: Replace item with (item)
(prettier/prettier)
[error] 128-128: Delete ··
(prettier/prettier)
[error] 129-129: This line has a length of 123. Maximum allowed is 120.
(max-len)
[error] 129-129: Replace /Error·while·building·post·list/ with ⏎······/Error·while·building·post·list/⏎····
(prettier/prettier)
[error] 131-131: Delete ··
(prettier/prettier)
[error] 135-135: Delete ··
(prettier/prettier)
[error] 137-137: This line has a length of 166. Maximum allowed is 120.
(max-len)
[error] 137-137: Replace /Error·while·building·post·list:·basePath·and·writeFilePath·are·required/ with ⏎······/Error·while·building·post·list:·basePath·and·writeFilePath·are·required/⏎····
(prettier/prettier)
[error] 139-139: Delete ··
(prettier/prettier)
[error] 141-141: This line has a length of 160. Maximum allowed is 120.
(max-len)
[error] 141-141: Replace /Error·while·building·post·list:·basePath·and·writeFilePath·are·required/ with ⏎······/Error·while·building·post·list:·basePath·and·writeFilePath·are·required/⏎····
(prettier/prettier)
[error] 142-142: Delete ··
(prettier/prettier)
[error] 144-145: Delete ⏎
(prettier/prettier)
tests/build-post-list.test.js
Outdated
| const normalizedDir = normalize(join(tempDir, 'blog')); | ||
| await fs.ensureDir(normalizedDir); |
There was a problem hiding this comment.
| const normalizedDir = normalize(join(tempDir, 'blog')); | |
| await fs.ensureDir(normalizedDir); |
tests/build-post-list.test.js
Outdated
| expect(output).toHaveProperty('docs'); | ||
| expect(output).toHaveProperty('blog'); | ||
| expect(output).toHaveProperty('about'); | ||
| expect(output).toHaveProperty('docsTree'); |
There was a problem hiding this comment.
| expect(output).toHaveProperty('docs'); | |
| expect(output).toHaveProperty('blog'); | |
| expect(output).toHaveProperty('about'); | |
| expect(output).toHaveProperty('docsTree'); | |
| expect(output).toMatchObject({ | |
| docs: expect.arrayContaining([ | |
| expect.objectContaining({ | |
| title: 'Docs Home', | |
| slug: '/docs/index' | |
| }) | |
| ]), | |
| blog: expect.arrayContaining([ | |
| expect.objectContaining({ | |
| title: 'Release Notes 2.1.0', | |
| slug: '/blog/release-notes-2.1.0' | |
| }) | |
| ]), | |
| about: expect.arrayContaining([ | |
| expect.objectContaining({ | |
| title: 'About Us', | |
| slug: '/about/index' | |
| }) | |
| ]), | |
| docsTree: expect.any(Object) | |
| }); |
tests/build-post-list.test.js
Outdated
| expect(output.docs.length).toBeGreaterThan(0); | ||
| expect(output.docs.find(item => item.title === 'Section 1')).toBeDefined(); |
There was a problem hiding this comment.
| expect(output.docs.length).toBeGreaterThan(0); | |
| expect(output.docs.find(item => item.title === 'Section 1')).toBeDefined(); | |
| const sectionEntry = output.docs.find(item => item.title === 'Section 1'); | |
| expect(sectionEntry).toMatchObject({ | |
| title: 'Section 1', | |
| slug: expect.stringContaining('/docs/section1'), | |
| isSection: true | |
| }); |
|
@akshatnema Updated the test |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (5)
tests/build-post-list.test.js (5)
1-4: Remove unused import.The
normalizefunction is imported but never used in the code.-const { resolve, join, normalize } = require('path'); +const { resolve, join } = require('path');🧰 Tools
🪛 eslint (1.23.1)
[error] 2-2: 'normalize' is assigned a value but never used.
(no-unused-vars)
11-11: Use a more unique temporary directory name.Using a fixed name like
test-configcould lead to conflicts if multiple test runs occur simultaneously.- tempDir = resolve(__dirname, `test-config`); + tempDir = resolve(__dirname, `test-config-${Date.now()}-${Math.random().toString(36).slice(2)}`);
19-27: Extract test content into constants.Consider extracting the test content into constants at the top of the file for better maintainability and reusability.
+const TEST_CONTENT = { + RELEASE_NOTES: '---\ntitle: Release Notes 2.1.0\n---\nThis is a release note.', + DOCS_HOME: '---\ntitle: Docs Home\n---\nThis is the documentation homepage.', + ABOUT_PAGE: '---\ntitle: About Us\n---\nThis is the about page.', +}; beforeEach(async () => { // ... - await fs.writeFile(join(tempDir, 'blog', 'release-notes-2.1.0.mdx'), '---\ntitle: Release Notes 2.1.0\n---\nThis is a release note.'); + await fs.writeFile(join(tempDir, 'blog', 'release-notes-2.1.0.mdx'), TEST_CONTENT.RELEASE_NOTES); // ... similar changes for other files🧰 Tools
🪛 eslint (1.23.1)
[error] 20-20: Replace
join(tempDir,·'blog',·'release-notes-2.1.0.mdx'),·'---\ntitle:·Release·Notes·2.1.0\n---\nThis·is·a·release·note.'with⏎······join(tempDir,·'blog',·'release-notes-2.1.0.mdx'),⏎······'---\ntitle:·Release·Notes·2.1.0\n---\nThis·is·a·release·note.'⏎····(prettier/prettier)
[error] 23-23: Replace
join(tempDir,·'docs',·'index.mdx'),·'---\ntitle:·Docs·Home\n---\nThis·is·the·documentation·homepage.'with⏎······join(tempDir,·'docs',·'index.mdx'),⏎······'---\ntitle:·Docs·Home\n---\nThis·is·the·documentation·homepage.'⏎····(prettier/prettier)
35-85: Enhance test assertions for better coverage.Consider adding more specific assertions to verify:
- The exact structure of the output object
- The relationships between sections in the docs tree
expect(output.docsTree).toBeDefined(); + // Verify docsTree structure + expect(output.docsTree).toMatchObject({ + items: expect.arrayContaining([ + expect.objectContaining({ + title: 'Reference', + items: expect.arrayContaining([ + expect.objectContaining({ + title: 'Specification' + }) + ]) + }) + ]) + });🧰 Tools
🪛 eslint (1.23.1)
[error] 47-47: Delete
,(prettier/prettier)
[error] 52-52: Delete
,(prettier/prettier)
[error] 57-57: Delete
,(prettier/prettier)
[error] 58-58: Delete
,(prettier/prettier)
[error] 66-66: Delete
,(prettier/prettier)
[error] 67-67: Delete
,(prettier/prettier)
[error] 70-70: Delete
····(prettier/prettier)
[error] 75-75: Delete
,(prettier/prettier)
[error] 76-76: Delete
,(prettier/prettier)
[error] 79-79: Delete
····(prettier/prettier)
[error] 80-80: Delete
····(prettier/prettier)
[error] 82-82: Replace
itemwith(item)(prettier/prettier)
120-124: Improve test descriptions for better clarity.The error handling test descriptions could be more specific about the expected behavior.
- it('handles errors gracefully', async () => { + it('throws an error when accessing non-existent directory', async () => { - it('throws an error if the front matter cannot be parsed', async () => { + it('throws "Error while building post list" when front matter is invalid', async () => { - it('throws an error if basePath is missing', async () => { + it('throws specific error message when basePath parameter is undefined', async () => { - it('throws an error if writeFilePath is missing', async () => { + it('throws specific error message when writeFilePath parameter is undefined', async () => {Also applies to: 161-177
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
tests/build-post-list.test.js(1 hunks)
🧰 Additional context used
🪛 eslint (1.23.1)
tests/build-post-list.test.js
[error] 2-2: 'normalize' is assigned a value but never used.
(no-unused-vars)
[error] 16-16: Delete ,
(prettier/prettier)
[error] 20-20: Replace join(tempDir,·'blog',·'release-notes-2.1.0.mdx'),·'---\ntitle:·Release·Notes·2.1.0\n---\nThis·is·a·release·note.' with ⏎······join(tempDir,·'blog',·'release-notes-2.1.0.mdx'),⏎······'---\ntitle:·Release·Notes·2.1.0\n---\nThis·is·a·release·note.'⏎····
(prettier/prettier)
[error] 23-23: Replace join(tempDir,·'docs',·'index.mdx'),·'---\ntitle:·Docs·Home\n---\nThis·is·the·documentation·homepage.' with ⏎······join(tempDir,·'docs',·'index.mdx'),⏎······'---\ntitle:·Docs·Home\n---\nThis·is·the·documentation·homepage.'⏎····
(prettier/prettier)
[error] 47-47: Delete ,
(prettier/prettier)
[error] 52-52: Delete ,
(prettier/prettier)
[error] 57-57: Delete ,
(prettier/prettier)
[error] 58-58: Delete ,
(prettier/prettier)
[error] 66-66: Delete ,
(prettier/prettier)
[error] 67-67: Delete ,
(prettier/prettier)
[error] 70-70: Delete ····
(prettier/prettier)
[error] 75-75: Delete ,
(prettier/prettier)
[error] 76-76: Delete ,
(prettier/prettier)
[error] 79-79: Delete ····
(prettier/prettier)
[error] 80-80: Delete ····
(prettier/prettier)
[error] 82-82: Replace item with (item)
(prettier/prettier)
[error] 89-89: Replace join(tempDir,·'docs',·'section1',·'_section.mdx'),·'---\ntitle:·Section·1\n---\nThis·is·section·1.' with ⏎······join(tempDir,·'docs',·'section1',·'_section.mdx'),⏎······'---\ntitle:·Section·1\n---\nThis·is·section·1.'⏎····
(prettier/prettier)
[error] 95-95: Replace item with (item)
(prettier/prettier)
[error] 104-104: Replace join(tempDir,·'blog',·'release-notes-2.1.1.mdx'),·'---\ntitle:·Release·Notes·2.1.1\n---\nThis·is·a·release·note.' with ⏎······join(tempDir,·'blog',·'release-notes-2.1.1.mdx'),⏎······'---\ntitle:·Release·Notes·2.1.1\n---\nThis·is·a·release·note.'⏎····
(prettier/prettier)
[error] 110-110: Replace item with (item)
(prettier/prettier)
[error] 111-111: Replace item with (item)
(prettier/prettier)
[error] 132-132: Replace item with (item)
(prettier/prettier)
[error] 139-139: Replace join(specDir,·'v2.1.0-next-spec.1.mdx'),·'---\n---\nContent·of·pre-release·specification·v2.1.0-next-spec.1.' with ⏎······join(specDir,·'v2.1.0-next-spec.1.mdx'),⏎······'---\n---\nContent·of·pre-release·specification·v2.1.0-next-spec.1.'⏎····
(prettier/prettier)
[error] 144-144: Replace item with (item)
(prettier/prettier)
[error] 156-156: Replace (item with ((item)
(prettier/prettier)
[error] 163-163: Delete ··
(prettier/prettier)
[error] 164-164: This line has a length of 123. Maximum allowed is 120.
(max-len)
[error] 164-164: Replace /Error·while·building·post·list/ with ⏎······/Error·while·building·post·list/⏎····
(prettier/prettier)
[error] 166-166: Delete ··
(prettier/prettier)
[error] 170-170: Delete ··
(prettier/prettier)
[error] 172-172: This line has a length of 166. Maximum allowed is 120.
(max-len)
[error] 172-172: Replace /Error·while·building·post·list:·basePath·and·writeFilePath·are·required/ with ⏎······/Error·while·building·post·list:·basePath·and·writeFilePath·are·required/⏎····
(prettier/prettier)
[error] 174-174: Delete ··
(prettier/prettier)
[error] 176-176: This line has a length of 160. Maximum allowed is 120.
(max-len)
[error] 176-176: Replace /Error·while·building·post·list:·basePath·and·writeFilePath·are·required/ with ⏎······/Error·while·building·post·list:·basePath·and·writeFilePath·are·required/⏎····
(prettier/prettier)
[error] 177-177: Delete ··
(prettier/prettier)
[error] 179-180: Delete ⏎
(prettier/prettier)
🔇 Additional comments (1)
tests/build-post-list.test.js (1)
1-195: Overall, this is a well-structured and comprehensive test suite!
The test file demonstrates good testing practices:
- Proper setup and teardown
- Comprehensive coverage of functionality
- Good error handling tests
- Clear separation of concerns
The suggested improvements are minor and the code is ready for production.
🧰 Tools
🪛 eslint (1.23.1)
[error] 2-2: 'normalize' is assigned a value but never used.
(no-unused-vars)
[error] 16-16: Delete ,
(prettier/prettier)
[error] 20-20: Replace join(tempDir,·'blog',·'release-notes-2.1.0.mdx'),·'---\ntitle:·Release·Notes·2.1.0\n---\nThis·is·a·release·note.' with ⏎······join(tempDir,·'blog',·'release-notes-2.1.0.mdx'),⏎······'---\ntitle:·Release·Notes·2.1.0\n---\nThis·is·a·release·note.'⏎····
(prettier/prettier)
[error] 23-23: Replace join(tempDir,·'docs',·'index.mdx'),·'---\ntitle:·Docs·Home\n---\nThis·is·the·documentation·homepage.' with ⏎······join(tempDir,·'docs',·'index.mdx'),⏎······'---\ntitle:·Docs·Home\n---\nThis·is·the·documentation·homepage.'⏎····
(prettier/prettier)
[error] 47-47: Delete ,
(prettier/prettier)
[error] 52-52: Delete ,
(prettier/prettier)
[error] 57-57: Delete ,
(prettier/prettier)
[error] 58-58: Delete ,
(prettier/prettier)
[error] 66-66: Delete ,
(prettier/prettier)
[error] 67-67: Delete ,
(prettier/prettier)
[error] 70-70: Delete ····
(prettier/prettier)
[error] 75-75: Delete ,
(prettier/prettier)
[error] 76-76: Delete ,
(prettier/prettier)
[error] 79-79: Delete ····
(prettier/prettier)
[error] 80-80: Delete ····
(prettier/prettier)
[error] 82-82: Replace item with (item)
(prettier/prettier)
[error] 89-89: Replace join(tempDir,·'docs',·'section1',·'_section.mdx'),·'---\ntitle:·Section·1\n---\nThis·is·section·1.' with ⏎······join(tempDir,·'docs',·'section1',·'_section.mdx'),⏎······'---\ntitle:·Section·1\n---\nThis·is·section·1.'⏎····
(prettier/prettier)
[error] 95-95: Replace item with (item)
(prettier/prettier)
[error] 104-104: Replace join(tempDir,·'blog',·'release-notes-2.1.1.mdx'),·'---\ntitle:·Release·Notes·2.1.1\n---\nThis·is·a·release·note.' with ⏎······join(tempDir,·'blog',·'release-notes-2.1.1.mdx'),⏎······'---\ntitle:·Release·Notes·2.1.1\n---\nThis·is·a·release·note.'⏎····
(prettier/prettier)
[error] 110-110: Replace item with (item)
(prettier/prettier)
[error] 111-111: Replace item with (item)
(prettier/prettier)
[error] 132-132: Replace item with (item)
(prettier/prettier)
[error] 139-139: Replace join(specDir,·'v2.1.0-next-spec.1.mdx'),·'---\n---\nContent·of·pre-release·specification·v2.1.0-next-spec.1.' with ⏎······join(specDir,·'v2.1.0-next-spec.1.mdx'),⏎······'---\n---\nContent·of·pre-release·specification·v2.1.0-next-spec.1.'⏎····
(prettier/prettier)
[error] 144-144: Replace item with (item)
(prettier/prettier)
[error] 156-156: Replace (item with ((item)
(prettier/prettier)
[error] 163-163: Delete ··
(prettier/prettier)
[error] 164-164: This line has a length of 123. Maximum allowed is 120.
(max-len)
[error] 164-164: Replace /Error·while·building·post·list/ with ⏎······/Error·while·building·post·list/⏎····
(prettier/prettier)
[error] 166-166: Delete ··
(prettier/prettier)
[error] 170-170: Delete ··
(prettier/prettier)
[error] 172-172: This line has a length of 166. Maximum allowed is 120.
(max-len)
[error] 172-172: Replace /Error·while·building·post·list:·basePath·and·writeFilePath·are·required/ with ⏎······/Error·while·building·post·list:·basePath·and·writeFilePath·are·required/⏎····
(prettier/prettier)
[error] 174-174: Delete ··
(prettier/prettier)
[error] 176-176: This line has a length of 160. Maximum allowed is 120.
(max-len)
[error] 176-176: Replace /Error·while·building·post·list:·basePath·and·writeFilePath·are·required/ with ⏎······/Error·while·building·post·list:·basePath·and·writeFilePath·are·required/⏎····
(prettier/prettier)
[error] 177-177: Delete ··
(prettier/prettier)
[error] 179-180: Delete ⏎
(prettier/prettier)
This script adds tests for build-post-list.js script
Summary by CodeRabbit
New Features
buildPostListandslugifyToCfunctions.Bug Fixes
Documentation