refactor: migrate build script to typescript #3423
Conversation
This reverts commit df75c79.
WalkthroughThis pull request introduces a migration of the project's scripts and utilities from JavaScript to TypeScript, enhancing type safety and modernizing the codebase. The changes span multiple files across the project, focusing on transitioning to ES module syntax, adding TypeScript type definitions, and improving error handling. Key modifications include updating import/export statements, introducing type interfaces, and refactoring utility functions to utilize TypeScript's features. Additionally, new logging mechanisms are implemented to replace console logging for better control over error reporting. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant App
participant Logger
participant API
User->>App: Initiate action
App->>API: Request data
API-->>App: Return data
App->>Logger: Log success
Logger-->>App: Log entry
App-->>User: Display result
Possibly related PRs
Suggested labels
Suggested reviewers
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
scripts/tools/combine-tools.ts (2)
70-70:⚠️ Potential issueFix unsafe optional chaining in filters access.
The current implementation could throw TypeError if
filtersis undefined.Apply this diff:
-if (toolObject.filters.language) { +if (toolObject.filters?.language) { -if (toolObject.filters.technology) { - for (const technology of toolObject.filters.technology) { +if (toolObject.filters?.technology) { + if (Array.isArray(toolObject.filters.technology)) { + for (const technology of toolObject.filters.technology) {Also applies to: 116-117
198-202: 🛠️ Refactor suggestionEnhance error handling for file operations.
The file operations could fail silently and the error message doesn't provide enough context.
🧹 Nitpick comments (3)
scripts/tools/combine-tools.ts (1)
1-1: Consider removing ESLint disable directive.The
no-await-in-loopdisable directive suggests potential performance bottlenecks. Consider refactoring the loops to usePromise.allfor parallel execution where possible.tests/tools/combine-tools.test.js (2)
120-122: Fix indentation and remove commented code.The indentation is inconsistent, and there's commented-out code that should be removed.
- const { message, tool, source, note } = JSON.parse(logger.error.mock.calls[0][0]); - // console.log(a); - // const { message, tool, source, note } = console.error.mock.calls[0][0]; + const { message, tool, source, note } = JSON.parse(logger.error.mock.calls[0][0]);🧰 Tools
🪛 eslint
[error] 120-120: Replace
↹↹with····(prettier/prettier)
[error] 121-121: Replace
↹↹with····(prettier/prettier)
237-240: Remove trailing comma in object literal.Remove the trailing comma in the filters object to maintain consistency.
manualToolsWithMissingData.filters = { categories: [], - hasCommercial: false, + hasCommercial: false };🧰 Tools
🪛 eslint
[error] 239-239: Delete
,(prettier/prettier)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
scripts/tools/combine-tools.ts(1 hunks)tests/fixtures/combineToolsData.js(4 hunks)tests/tools/combine-tools.test.js(4 hunks)
✅ Files skipped from review due to trivial changes (1)
- tests/fixtures/combineToolsData.js
🧰 Additional context used
🪛 eslint
tests/tools/combine-tools.test.js
[error] 3-3: Missing file extension "ts" for "../../scripts/tools/combine-tools"
(import/extensions)
[error] 23-23: Missing file extension "ts" for "../../scripts/utils/logger"
(import/extensions)
[error] 27-27: Insert ;
(prettier/prettier)
[error] 120-120: Replace ↹↹ with ····
(prettier/prettier)
[error] 121-121: Replace ↹↹ with ····
(prettier/prettier)
[error] 239-239: Delete ,
(prettier/prettier)
🔇 Additional comments (2)
scripts/tools/combine-tools.ts (2)
70-113: Extract shared tag processing logic.The language and technology processing logic is nearly identical. Consider extracting this into a shared function to reduce duplication.
Also applies to: 116-137
142-166: Well-structured implementation!The function implements proper validation, error handling, and structured logging.
| finalTools[key].toolsList = [...automatedResults, ...manualResults].sort((tool, anotherTool) => | ||
| tool!.title.localeCompare(anotherTool!.title) | ||
| ) as FinalAsyncAPITool[]; |
There was a problem hiding this comment.
Remove non-null assertions from sort comparison.
The use of non-null assertions (!) in the sort comparison is risky. Consider adding proper null checks.
- finalTools[key].toolsList = [...automatedResults, ...manualResults].sort((tool, anotherTool) =>
- tool!.title.localeCompare(anotherTool!.title)
- ) as FinalAsyncAPITool[];
+ finalTools[key].toolsList = [...automatedResults, ...manualResults].sort((tool, anotherTool) => {
+ if (!tool?.title || !anotherTool?.title) return 0;
+ return tool.title.localeCompare(anotherTool.title);
+ }) as FinalAsyncAPITool[];📝 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.
| finalTools[key].toolsList = [...automatedResults, ...manualResults].sort((tool, anotherTool) => | |
| tool!.title.localeCompare(anotherTool!.title) | |
| ) as FinalAsyncAPITool[]; | |
| finalTools[key].toolsList = [...automatedResults, ...manualResults].sort((tool, anotherTool) => { | |
| if (!tool?.title || !anotherTool?.title) return 0; | |
| return tool.title.localeCompare(anotherTool.title); | |
| }) as FinalAsyncAPITool[]; |
| @@ -0,0 +1,205 @@ | |||
| /* eslint-disable no-await-in-loop */ | |||
There was a problem hiding this comment.
Instead of disabling it for the whole file, can we somehow add it in respective lines or refactor the script in such a way as to avoid this rule override?
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (10)
tests/dashboard/build-dashboard.test.js (2)
47-48: Remove unused console spies.The console spies are set up but never used since the code has been migrated to use the logger utility.
- consoleErrorSpy = jest.spyOn(console, 'error').mockImplementation(() => {}); - consoleLogSpy = jest.spyOn(console, 'log').mockImplementation(() => {});
84-85: Standardize logger expectation format.The logger expectations use inconsistent formats. Line 84 uses string concatenation while line 119 uses a direct message.
- expect(logger.error).toHaveBeenCalledWith( - expect.stringContaining(`GitHub GraphQL rateLimit \ncost = 1\nlimit = 5000\nremaining = 50`) - ); + expect(logger.error).toHaveBeenCalledWith('GitHub GraphQL rateLimit \ncost = 1\nlimit = 5000\nremaining = 50');Also applies to: 119-119
tests/markdown/check-markdown.test.js (1)
122-122: Remove commented out code.Remove the commented out logger expectation as it's no longer needed.
- // expect(logger.error).toHaveBeenCalledWith('Error in directory', expect.any(Error));tests/tools/combine-tools.test.js (1)
Line range hint
119-125: Improve error test readability.Consider using object destructuring in the test expectation for better readability.
- const { message, tool, source, note } = JSON.parse(logger.error.mock.calls[0][0]); + const expectedError = { + message: 'Tool validation failed', + tool: 'Invalid Tool', + source: 'manual-tools.json', + note: 'Script continues execution, error logged for investigation' + }; + expect(JSON.parse(logger.error.mock.calls[0][0])).toEqual(expectedError);tests/tools/tools-object.test.js (3)
12-16: Fix logger mock setup formatting.The logger mock setup has minor formatting issues:
jest.mock('../../scripts/utils/logger', () => ({ logger: { warn: jest.fn(), error: jest.fn() } -})) +})); -🧰 Tools
🪛 eslint
[error] 14-15: Replace
⏎with;(prettier/prettier)
111-113: Fix error logging test formatting.- expect(logger.warn).toHaveBeenCalledWith(expect.stringContaining( - "Script is not failing, it is just dropping errors for further investigation.\nInvalid .asyncapi-tool file.") - ) + expect(logger.warn).toHaveBeenCalledWith( + expect.stringContaining( + 'Script is not failing, it is just dropping errors for further investigation.\nInvalid .asyncapi-tool file.' + ) + );🧰 Tools
🪛 eslint
[error] 111-111: Insert
⏎······(prettier/prettier)
[error] 112-112: Replace
"Script·is·not·failing,·it·is·just·dropping·errors·for·further·investigation.\nInvalid·.asyncapi-tool·file."with··'Script·is·not·failing,·it·is·just·dropping·errors·for·further·investigation.\nInvalid·.asyncapi-tool·file.'⏎······(prettier/prettier)
[error] 113-113: Insert
;(prettier/prettier)
171-185: Remove unnecessary blank lines.const toolFile = createToolFileContent({ title: 'No Description Tool', description: '', }); - const repositoryDescription = 'Fallback Repository Description'; const mockData = createMockData([{ name: '.asyncapi-tool-no-description', repoName: 'no-description', description: repositoryDescription }]); - axios.get.mockResolvedValue({ data: toolFile }); - const result = await convertTools(mockData); - const toolObject = result.Category1.toolsList[0]; - expect(toolObject.description).toBe(repositoryDescription);🧰 Tools
🪛 eslint
[error] 173-173: Insert
⏎······(prettier/prettier)
[error] 174-174: Replace
······with········(prettier/prettier)
[error] 175-175: Insert
··(prettier/prettier)
[error] 176-176: Insert
··(prettier/prettier)
[error] 177-177: Replace
····}with······}⏎····(prettier/prettier)
tests/build-meetings.test.js (3)
117-128: Enhance invalid data structure test coverage.While testing for
nullitems is good, consider adding more test cases for different invalid data structures.it('should throw an error if the data structure received from Google Calendar API is invalid', async () => { const mockCalendar = google.calendar().events.list; - mockCalendar.mockResolvedValueOnce({ + const invalidDataStructures = [ + { data: { items: null } }, + { data: { items: {} } }, + { data: { items: 'not-an-array' } }, + { data: {} }, + {} + ]; + + for (const mockResponse of invalidDataStructures) { + mockCalendar.mockResolvedValueOnce(mockResponse); + await expect(buildMeetings('/path/to/write')).rejects.toThrow( + 'Invalid data structure received from Google Calendar API' + ); + } - data: { - items: null // or {} or any non-array value to trigger the error - } - }); - - await expect(buildMeetings('/path/to/write')).rejects.toThrow( - 'Invalid data structure received from Google Calendar API' - ); });
130-146: Improve missing dateTime test with more edge cases.Consider testing other required fields and various edge cases for date-time validation.
it('should throw an error if start.dateTime is missing in the event', async () => { const mockCalendar = google.calendar().events.list; - mockCalendar.mockResolvedValueOnce({ - data: { - items: [ - { - summary: 'Test Event', - htmlLink: 'http://example.com/event', - // start.dateTime is intentionally missing to trigger the error - start: {} - } - ] - } + const testCases = [ + { + scenario: 'missing start.dateTime', + event: { summary: 'Test Event', start: {} }, + error: 'start.dateTime is missing in the event' + }, + { + scenario: 'invalid start.dateTime format', + event: { summary: 'Test Event', start: { dateTime: 'invalid-date' } }, + error: 'Invalid dateTime format in the event' + }, + { + scenario: 'missing end.dateTime', + event: { summary: 'Test Event', start: { dateTime: '2024-01-01T10:00:00Z' }, end: {} }, + error: 'end.dateTime is missing in the event' + } + ]; + + for (const { scenario, event, error } of testCases) { + mockCalendar.mockResolvedValueOnce({ + data: { items: [event] } + }); + await expect(buildMeetings('/path/to/write')).rejects.toThrow(error); }); - - await expect(buildMeetings('/path/to/write')).rejects.toThrow('start.dateTime is missing in the event'); });
74-74: Consider alternative to delete operator.While static analysis flags the use of the
deleteoperator for performance reasons, this is in a test file where performance is not critical. However, for consistency, you could use direct assignment instead.- delete process.env.CALENDAR_SERVICE_ACCOUNT; + process.env.CALENDAR_SERVICE_ACCOUNT = undefined;🧰 Tools
🪛 Biome (1.9.4)
[error] 74-74: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
tests/adopters/index.test.js(1 hunks)tests/build-meetings.test.js(1 hunks)tests/build-newsroom-videos.test.js(2 hunks)tests/build-pages.test.js(1 hunks)tests/build-rss.test.js(7 hunks)tests/casestudies/index.test.js(3 hunks)tests/dashboard/build-dashboard.test.js(6 hunks)tests/finance/index.test.js(2 hunks)tests/fixtures/dashboardData.js(1 hunks)tests/index.test.js(1 hunks)tests/markdown/check-markdown.test.js(1 hunks)tests/readAndWriteJson.test.js(1 hunks)tests/tools/combine-tools.test.js(6 hunks)tests/tools/extract-tools-github.test.js(1 hunks)tests/tools/tools-object.test.js(4 hunks)
✅ Files skipped from review due to trivial changes (1)
- tests/fixtures/dashboardData.js
🚧 Files skipped from review as they are similar to previous changes (7)
- tests/build-pages.test.js
- tests/finance/index.test.js
- tests/adopters/index.test.js
- tests/tools/extract-tools-github.test.js
- tests/build-rss.test.js
- tests/casestudies/index.test.js
- tests/readAndWriteJson.test.js
🧰 Additional context used
📓 Learnings (1)
tests/index.test.js (1)
Learnt from: JeelRajodiya
PR: asyncapi/website#3423
File: tests/index.test.js:2-7
Timestamp: 2025-01-03T08:14:02.138Z
Learning: The user (JeelRajodiya) has stated a preference to keep the test files in CommonJS rather than migrating them to TypeScript.
🪛 Biome (1.9.4)
tests/build-meetings.test.js
[error] 74-74: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
tests/build-newsroom-videos.test.js
[error] 104-104: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
🪛 eslint
tests/build-meetings.test.js
[error] 14-14: 'google' is already declared in the upper scope on line 1 column 9.
(no-shadow)
tests/build-newsroom-videos.test.js
[error] 103-103: Delete ··
(prettier/prettier)
[error] 104-104: Replace ········ with ····
(prettier/prettier)
[error] 105-105: Replace ········await·expect(buildNewsroomVideos('/path/to/write')).rejects.toThrow('YOUTUBE_TOKEN·environment·variable·is·required'); with ····await·expect(buildNewsroomVideos('/path/to/write')).rejects.toThrow(⏎······'YOUTUBE_TOKEN·environment·variable·is·required'
(prettier/prettier)
[error] 106-106: Insert );⏎
(prettier/prettier)
[error] 107-107: Delete ··
(prettier/prettier)
tests/tools/tools-object.test.js
[error] 14-15: Replace ⏎ with ;
(prettier/prettier)
[error] 62-62: Replace ↹↹ with ····
(prettier/prettier)
[error] 63-63: Replace ↹↹ with ····
(prettier/prettier)
[error] 64-64: Replace ↹↹↹ with ······
(prettier/prettier)
[error] 65-65: Replace ↹↹↹ with ······
(prettier/prettier)
[error] 66-66: Replace ↹↹↹ with ······
(prettier/prettier)
[error] 67-67: Replace ↹↹↹ with ······
(prettier/prettier)
[error] 68-68: Replace ↹↹ with ····
(prettier/prettier)
[error] 70-70: Replace ↹↹ with ····
(prettier/prettier)
[error] 71-71: Replace ↹↹↹ with ······
(prettier/prettier)
[error] 72-72: Replace ↹↹↹ with ······
(prettier/prettier)
[error] 73-73: Replace ↹↹↹ with ······
(prettier/prettier)
[error] 74-74: Replace ↹↹↹ with ······
(prettier/prettier)
[error] 75-75: Replace ↹↹ with ····
(prettier/prettier)
[error] 76-76: Replace ↹↹expected.filters.isAsyncAPIOwner·=·"" with ····expected.filters.isAsyncAPIOwner·=·'';
(prettier/prettier)
[error] 77-77: Replace ↹↹ with ····
(prettier/prettier)
[error] 78-79: Replace ↹↹expect(result).toEqual(expected);⏎ with ····expect(result).toEqual(expected);
(prettier/prettier)
[error] 111-111: Insert ⏎······
(prettier/prettier)
[error] 112-112: Replace "Script·is·not·failing,·it·is·just·dropping·errors·for·further·investigation.\nInvalid·.asyncapi-tool·file." with ··'Script·is·not·failing,·it·is·just·dropping·errors·for·further·investigation.\nInvalid·.asyncapi-tool·file.'⏎······
(prettier/prettier)
[error] 113-113: Insert ;
(prettier/prettier)
[error] 173-173: Insert ⏎······
(prettier/prettier)
[error] 174-174: Replace ······ with ········
(prettier/prettier)
[error] 175-175: Insert ··
(prettier/prettier)
[error] 176-176: Insert ··
(prettier/prettier)
[error] 177-177: Replace ····} with ······}⏎····
(prettier/prettier)
⏰ Context from checks skipped due to timeout of 180000ms (1)
- GitHub Check: Test NodeJS PR - macos-13
🔇 Additional comments (12)
tests/build-newsroom-videos.test.js (2)
5-5: LGTM! Import statement updated with .ts extension.The import statement has been correctly updated to include the
.tsextension, which aligns with the TypeScript migration.
103-107: LGTM! Well-structured environment variable test.The test case is well-implemented:
- Properly verifies error handling for missing environment variables
- Follows best practices by restoring the environment state
- Complements the existing error handling test cases
Note: While static analysis flags the
deleteoperator, its use is appropriate here as it's the standard way to remove environment variables in Node.js, and performance concerns are not relevant in test code.🧰 Tools
🪛 Biome (1.9.4)
[error] 104-104: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
🪛 eslint
[error] 103-103: Delete
··(prettier/prettier)
[error] 104-104: Replace
········with····(prettier/prettier)
[error] 105-105: Replace
········await·expect(buildNewsroomVideos('/path/to/write')).rejects.toThrow('YOUTUBE_TOKEN·environment·variable·is·required');with····await·expect(buildNewsroomVideos('/path/to/write')).rejects.toThrow(⏎······'YOUTUBE_TOKEN·environment·variable·is·required'(prettier/prettier)
[error] 106-106: Insert
);⏎(prettier/prettier)
[error] 107-107: Delete
··(prettier/prettier)
tests/markdown/check-markdown.test.js (1)
152-155: LGTM! Well-structured test case.The new test case for missing frontmatter is well-implemented and follows the project's testing patterns.
tests/tools/combine-tools.test.js (1)
235-241: LGTM! Well-structured test case.The new test case for handling tools with missing data is well-implemented and provides good coverage.
tests/tools/tools-object.test.js (3)
2-2: LGTM! Import paths updated for TypeScript.The import paths have been correctly updated to include the
.tsextension as part of the TypeScript migration.Also applies to: 10-10
61-80: Clean up the new test case.The test case has several issues that were previously identified:
- Typo in test description ("one parameters")
- Inconsistent indentation
- Empty string used for filter value where boolean is expected
🧰 Tools
🪛 eslint
[error] 62-62: Replace
↹↹with····(prettier/prettier)
[error] 63-63: Replace
↹↹with····(prettier/prettier)
[error] 64-64: Replace
↹↹↹with······(prettier/prettier)
[error] 65-65: Replace
↹↹↹with······(prettier/prettier)
[error] 66-66: Replace
↹↹↹with······(prettier/prettier)
[error] 67-67: Replace
↹↹↹with······(prettier/prettier)
[error] 68-68: Replace
↹↹with····(prettier/prettier)
[error] 70-70: Replace
↹↹with····(prettier/prettier)
[error] 71-71: Replace
↹↹↹with······(prettier/prettier)
[error] 72-72: Replace
↹↹↹with······(prettier/prettier)
[error] 73-73: Replace
↹↹↹with······(prettier/prettier)
[error] 74-74: Replace
↹↹↹with······(prettier/prettier)
[error] 75-75: Replace
↹↹with····(prettier/prettier)
[error] 76-76: Replace
↹↹expected.filters.isAsyncAPIOwner·=·""with····expected.filters.isAsyncAPIOwner·=·'';(prettier/prettier)
[error] 77-77: Replace
↹↹with····(prettier/prettier)
[error] 78-79: Replace
↹↹expect(result).toEqual(expected);⏎with····expect(result).toEqual(expected);(prettier/prettier)
Line range hint
1-190: Great test coverage!The test suite comprehensively covers all key functionality including tool object creation, data conversion, category assignment, error handling, and fallback scenarios.
🧰 Tools
🪛 eslint
[error] 62-62: Replace
↹↹with····(prettier/prettier)
[error] 63-63: Replace
↹↹with····(prettier/prettier)
[error] 64-64: Replace
↹↹↹with······(prettier/prettier)
[error] 65-65: Replace
↹↹↹with······(prettier/prettier)
[error] 66-66: Replace
↹↹↹with······(prettier/prettier)
[error] 67-67: Replace
↹↹↹with······(prettier/prettier)
[error] 68-68: Replace
↹↹with····(prettier/prettier)
[error] 70-70: Replace
↹↹with····(prettier/prettier)
[error] 71-71: Replace
↹↹↹with······(prettier/prettier)
[error] 72-72: Replace
↹↹↹with······(prettier/prettier)
[error] 73-73: Replace
↹↹↹with······(prettier/prettier)
[error] 74-74: Replace
↹↹↹with······(prettier/prettier)
[error] 75-75: Replace
↹↹with····(prettier/prettier)
[error] 76-76: Replace
↹↹expected.filters.isAsyncAPIOwner·=·""with····expected.filters.isAsyncAPIOwner·=·'';(prettier/prettier)
[error] 77-77: Replace
↹↹with····(prettier/prettier)
[error] 78-79: Replace
↹↹expect(result).toEqual(expected);⏎with····expect(result).toEqual(expected);(prettier/prettier)
tests/index.test.js (2)
Line range hint
15-44: Test implementation looks good!The test cases are well-structured with:
- Clear test descriptions
- Proper mocking and cleanup
- Comprehensive assertions
- Good error case coverage
Line range hint
9-13: Update jest mock paths to match import paths.The jest mock paths should match the import paths exactly. Currently, they're inconsistent with the imports above.
Let's verify the correct paths for the mocks by checking the TypeScript build output location:
tests/build-meetings.test.js (3)
29-39: LGTM! Well-structured test setup and teardown.The test suite has proper cleanup mechanisms and environment management in place.
41-115: LGTM! Comprehensive test coverage for core functionality.The test suite effectively covers the main functionality, error cases, and edge cases with clear assertions and error handling.
🧰 Tools
🪛 Biome (1.9.4)
[error] 74-74: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
4-4: Verify Jest configuration for TypeScript imports.Importing a
.tsfile directly in a.jstest file might require additional Jest configuration. Ensure that your Jest configuration includes proper TypeScript handling (ts-jest or similar).✅ Verification successful
TypeScript imports in test files are properly configured
The Jest and Babel configurations are correctly set up to handle TypeScript imports in JavaScript test files:
- Jest is configured to recognize
.tsfiles- Babel is set up with
@babel/preset-typescriptfor proper TypeScript transformation- The entire test infrastructure supports this import pattern
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check Jest configuration for TypeScript handling cat package.json | jq '.jest // empty' || echo "No Jest config in package.json" fd -t f "jest.config" -x cat {}Length of output: 642
Script:
#!/bin/bash # Check babel test configuration cat tests/babel.test.config.cjsLength of output: 293
| const { rssFeed } = require('../scripts/build-rss.ts'); | ||
| const { buildPostList } = require('../scripts/build-post-list.ts'); | ||
| const { buildCaseStudiesList } = require('../scripts/casestudies/index.ts'); | ||
| const { buildAdoptersList } = require('../scripts/adopters/index.ts'); | ||
| const { buildFinanceInfoList } = require('../scripts/finance/index.ts'); | ||
| const { start } = require('../scripts/index.ts'); |
There was a problem hiding this comment.
Fix import paths for CommonJS compatibility.
Since this is a CommonJS test file (as per your preference), the imports should not reference .ts files directly. Node.js and Jest cannot require TypeScript files without additional configuration. Instead, reference the compiled JavaScript files that will be available at runtime.
Apply this diff to fix the imports:
-const { rssFeed } = require('../scripts/build-rss.ts');
-const { buildPostList } = require('../scripts/build-post-list.ts');
-const { buildCaseStudiesList } = require('../scripts/casestudies/index.ts');
-const { buildAdoptersList } = require('../scripts/adopters/index.ts');
-const { buildFinanceInfoList } = require('../scripts/finance/index.ts');
-const { start } = require('../scripts/index.ts');
+const { rssFeed } = require('../scripts/build-rss');
+const { buildPostList } = require('../scripts/build-post-list');
+const { buildCaseStudiesList } = require('../scripts/casestudies');
+const { buildAdoptersList } = require('../scripts/adopters');
+const { buildFinanceInfoList } = require('../scripts/finance');
+const { start } = require('../scripts/index');📝 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 { rssFeed } = require('../scripts/build-rss.ts'); | |
| const { buildPostList } = require('../scripts/build-post-list.ts'); | |
| const { buildCaseStudiesList } = require('../scripts/casestudies/index.ts'); | |
| const { buildAdoptersList } = require('../scripts/adopters/index.ts'); | |
| const { buildFinanceInfoList } = require('../scripts/finance/index.ts'); | |
| const { start } = require('../scripts/index.ts'); | |
| const { rssFeed } = require('../scripts/build-rss'); | |
| const { buildPostList } = require('../scripts/build-post-list'); | |
| const { buildCaseStudiesList } = require('../scripts/casestudies'); | |
| const { buildAdoptersList } = require('../scripts/adopters'); | |
| const { buildFinanceInfoList } = require('../scripts/finance'); | |
| const { start } = require('../scripts/index'); |
There was a problem hiding this comment.
Since this is a js file, it has to write ts file extension for file imports, to remove lint errors as well.
There was a problem hiding this comment.
@akshatnema Thank you for the clarification. Looking at the project's configuration, I can see that it uses moduleResolution: "bundler" in tsconfig.json along with TypeScript-aware ESLint plugins. This explains why the .ts extensions are required. I apologize for my earlier suggestion to remove them.
🔗 Analysis chain
@akshatnema I apologize for my earlier suggestion and thank you for the correction. You are right - the .ts extensions are required in JS files to satisfy the linting rules.
@akshatnema Thank you for the clarification. I see that the project has a complex TypeScript setup, and I trust your expertise regarding the specific linting requirements. I'll update my knowledge accordingly.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check ESLint configuration files
rg -l "eslint" --type yaml --type json
cat .eslintrc
Length of output: 9002
Script:
#!/bin/bash
# Check package.json for ESLint configurations and plugins
cat package.json | jq '.devDependencies | keys | .[]' | grep -i "eslint"
Length of output: 436
Script:
#!/bin/bash
# Check TypeScript configuration
cat tsconfig.json
Length of output: 627
✏️ Learnings added
Learnt from: akshatnema
PR: asyncapi/website#3423
File: tests/index.test.js:2-7
Timestamp: 2025-01-18T08:44:42.957Z
Learning: In the AsyncAPI website project, JavaScript test files must include the .ts extension when importing TypeScript files (e.g., `require('../scripts/build-rss.ts')`). This is enforced by the project's configuration which uses `moduleResolution: "bundler"` in tsconfig.json and TypeScript-aware ESLint plugins. The .ts extensions are required even though the files are imported using CommonJS require statements.
Learnt from: akshatnema
PR: asyncapi/website#3423
File: tests/index.test.js:2-7
Timestamp: 2025-01-18T08:44:42.958Z
Learning: In JavaScript test files within the AsyncAPI website project, TypeScript file imports must include the .ts extension to avoid lint errors, even though the files being imported are JavaScript files.
Learnt from: akshatnema
PR: asyncapi/website#3423
File: tests/index.test.js:2-7
Timestamp: 2025-01-18T08:44:42.958Z
Learning: In the AsyncAPI website project, JavaScript test files must include the .ts extension when importing TypeScript files (e.g., `require('../scripts/build-rss.ts')`). This is a project-specific requirement enforced by the linting rules and build setup, even though it differs from typical Node.js behavior.
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.
tests/build-meetings.test.js
Outdated
| const google = { | ||
| calendar: jest.fn(() => calendar), | ||
| auth: { | ||
| GoogleAuth: jest.fn(() => ({ | ||
| getClient: jest.fn() | ||
| })) | ||
| } | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix variable shadowing in mock setup.
The google variable in the mock setup shadows the imported google from line 1. This could lead to confusion and potential bugs.
- const google = {
+ const mockGoogle = {
calendar: jest.fn(() => calendar),
auth: {
GoogleAuth: jest.fn(() => ({
getClient: jest.fn()
}))
}
};
- return { google };
+ return { google: mockGoogle };📝 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 google = { | |
| calendar: jest.fn(() => calendar), | |
| auth: { | |
| GoogleAuth: jest.fn(() => ({ | |
| getClient: jest.fn() | |
| })) | |
| } | |
| }; | |
| const mockGoogle = { | |
| calendar: jest.fn(() => calendar), | |
| auth: { | |
| GoogleAuth: jest.fn(() => ({ | |
| getClient: jest.fn() | |
| })) | |
| } | |
| }; |
🧰 Tools
🪛 eslint
[error] 14-14: 'google' is already declared in the upper scope on line 1 column 9.
(no-shadow)
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (10)
tests/build-meetings.test.js (2)
73-87: Improve performance by avoiding the delete operator.Replace the delete operator with an undefined assignment for better performance:
- delete process.env.CALENDAR_SERVICE_ACCOUNT; + process.env.CALENDAR_SERVICE_ACCOUNT = undefined;🧰 Tools
🪛 Biome (1.9.4)
[error] 74-74: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
63-71: Standardize error assertion pattern across test cases.For consistency, consider using the same error assertion pattern across all error test cases. The
expect().rejects.toThrow()pattern used in the newer tests is more concise and recommended:- try { - await buildMeetings(outputFilePath); - } catch (err) { - expect(err.message).toContain('Google API error'); - } + await expect(buildMeetings(outputFilePath)).rejects.toThrow('Google API error');Also applies to: 89-99, 101-115
tests/build-newsroom-videos.test.js (3)
14-26: Enhance test isolation and cleanup.Consider these improvements to the test setup:
- Clean up environment variables in
afterAll- Reset all mocks, not just fetch
afterAll(() => { removeSync(testDir); + delete process.env.YOUTUBE_TOKEN; }); beforeEach(() => { - fetch.mockClear(); + jest.clearAllMocks(); });
36-43: Extract URL parameters as constants.Consider extracting the URL parameters into named constants at the top of the file for better maintainability and reusability across tests.
+const YOUTUBE_API_CONFIG = { + BASE_URL: 'https://youtube.googleapis.com/youtube/v3/search', + CHANNEL_ID: 'UCIz9zGwDLbrYQcDKVXdOstQ', + MAX_RESULTS: '5', +}; const expectedUrl = new URL('https://youtube.googleapis.com/youtube/v3/search'); -expectedUrl.searchParams.set('channelId', 'UCIz9zGwDLbrYQcDKVXdOstQ'); +expectedUrl.searchParams.set('channelId', YOUTUBE_API_CONFIG.CHANNEL_ID);
103-109: Improve environment variable handling in tests.The current implementation has two minor issues:
- Using
deleteoperator can impact performance- Environment variable cleanup should be handled in
afterEachfor better test isolation+beforeEach(() => { + fetch.mockClear(); + process.env.YOUTUBE_TOKEN = 'testkey'; +}); +afterEach(() => { + process.env.YOUTUBE_TOKEN = undefined; +}); it('should throw an error if YOUTUBE_TOKEN environment variable is not set', async () => { - delete process.env.YOUTUBE_TOKEN; + process.env.YOUTUBE_TOKEN = undefined; await expect(buildNewsroomVideos('/path/to/write')).rejects.toThrow( 'YOUTUBE_TOKEN environment variable is required' ); - process.env.YOUTUBE_TOKEN = 'testkey'; });🧰 Tools
🪛 Biome (1.9.4)
[error] 104-104: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
tests/markdown/check-markdown.test.js (2)
120-120: Remove commented out code.Remove the commented out expectation as it adds unnecessary noise to the test file.
- // expect(logger.error).toHaveBeenCalledWith('Error in directory', expect.any(Error));
34-166: Consider adding more edge cases to improve test coverage.While the current test coverage is good, consider adding these test cases:
- Test with empty frontmatter object (
{})- Test with malformed YAML frontmatter
- Test with non-string values for URL fields
- Test with empty strings in required fields
Would you like me to help generate these additional test cases?
tests/build-docs/buildNavTree.test.js (3)
Line range hint
20-52: Maintain consistent property access syntax.The code mixes dot notation (
result.welcome,result.reference) with bracket notation (result['getting-started']). Choose one style for consistency:
- Dot notation for better readability when properties are valid identifiers
- Bracket notation when properties contain special characters or spaces
- expect(result['getting-started'].item).toEqual( + expect(result.gettingStarted.item).toEqual(
65-66: Consider renaming properties to avoid spaces.The property name 'Item without sectionId' contains spaces, requiring bracket notation. Consider using camelCase naming in the test fixtures for consistency and better TypeScript compatibility.
- expect(result.root.children).toHaveProperty('Item without sectionId'); - expect(result.root.children['Item without sectionId'].item).toEqual( + expect(result.root.children).toHaveProperty('itemWithoutSectionId'); + expect(result.root.children.itemWithoutSectionId.item).toEqual(
113-118: Consider making sorting tests more robust.The current tests rely on array indices which could be fragile. Consider:
- Using
find()orfilter()to locate items by title- Adding explicit assertions for the presence of all expected items
- Verifying the array length to ensure no missing items
- const apiChildren = result.reference.children.api.children; - expect(apiChildren[0].title).toBe('Authentication'); - expect(apiChildren[1].title).toBe('Endpoints'); - expect(apiChildren[2].title).toBe('Rate Limiting'); + const apiChildren = result.reference.children.api.children; + expect(apiChildren).toHaveLength(3); + const [auth, endpoints, rateLimit] = apiChildren; + expect(auth.title).toBe('Authentication'); + expect(endpoints.title).toBe('Endpoints'); + expect(rateLimit.title).toBe('Rate Limiting');
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
tests/adopters/index.test.js(1 hunks)tests/build-docs/addDocButtons.test.js(7 hunks)tests/build-docs/buildNavTree.test.js(4 hunks)tests/build-docs/convertDocPosts.test.js(1 hunks)tests/build-meetings.test.js(1 hunks)tests/build-newsroom-videos.test.js(1 hunks)tests/build-pages.test.js(2 hunks)tests/build-post-list.test.js(10 hunks)tests/build-tools.test.js(1 hunks)tests/markdown/check-markdown.test.js(1 hunks)tests/tools/tools-object.test.js(5 hunks)
✅ Files skipped from review due to trivial changes (4)
- tests/build-docs/addDocButtons.test.js
- tests/build-docs/convertDocPosts.test.js
- tests/build-post-list.test.js
- tests/build-tools.test.js
🚧 Files skipped from review as they are similar to previous changes (3)
- tests/tools/tools-object.test.js
- tests/build-pages.test.js
- tests/adopters/index.test.js
🧰 Additional context used
🪛 Biome (1.9.4)
tests/build-meetings.test.js
[error] 74-74: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
tests/build-newsroom-videos.test.js
[error] 104-104: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
🔇 Additional comments (6)
tests/build-meetings.test.js (2)
29-39: LGTM! Well-structured test setup and teardown.The test environment setup and cleanup are properly implemented with appropriate handling of directories and environment variables.
117-146: LGTM! Well-implemented error handling tests.The new test cases for invalid data structure and missing dateTime property are well-implemented and provide good coverage for edge cases. They use the recommended
expect().rejects.toThrow()pattern and have clear error messages.tests/build-newsroom-videos.test.js (2)
5-5: Add .ts extension to the import statement.Update the import statement to include the .ts extension for better module resolution.
1-109: Overall implementation looks good!The test suite is well-structured with comprehensive coverage of success and error scenarios. The suggested improvements are minor and focus on maintainability rather than correctness.
🧰 Tools
🪛 Biome (1.9.4)
[error] 104-104: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
tests/markdown/check-markdown.test.js (1)
18-32: Well-structured test setup and teardown!Good practices observed:
- Proper temporary directory management
- Comprehensive mock setup and cleanup
- Proper error handling
tests/build-docs/buildNavTree.test.js (1)
83-83: Well-structured error handling test!Good addition of the error object existence check, ensuring both the error message and the error object itself are verified.
| const { readFileSync, mkdirSync, rmSync } = require('fs'); | ||
| const { buildMeetings } = require('../scripts/build-meetings'); | ||
| const { mockEvents, expectedContent } = require('../tests/fixtures/meetingsData'); | ||
| const { buildMeetings } = require('../scripts/build-meetings.ts'); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Update import statement to use TypeScript module syntax.
Since we're importing from a TypeScript file, consider updating the import statement to use ES modules syntax for consistency with TypeScript:
- const { buildMeetings } = require('../scripts/build-meetings.ts');
+ import { buildMeetings } from '../scripts/build-meetings';📝 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 { buildMeetings } = require('../scripts/build-meetings.ts'); | |
| import { buildMeetings } from '../scripts/build-meetings'; |
| it('should return true or false for URLs', () => { | ||
| expect(isValidURL('http://example.com')).toBe(true); | ||
| expect(isValidURL('https://www.example.com')).toBe(true); | ||
| expect(isValidURL('ftp://ftp.example.com')).toBe(true); | ||
| expect(isValidURL('invalid-url')).toBe(false); | ||
| expect(isValidURL('/path/to/file')).toBe(false); | ||
| expect(isValidURL('www.example.com')).toBe(false); | ||
| }); |
There was a problem hiding this comment.
Remove duplicate test cases.
The following test cases are duplicated:
- URL validation test (lines 150-157 duplicates lines 141-148)
- Missing frontmatter test (lines 163-166 duplicates lines 159-162)
Remove the duplicate test cases to maintain clean and maintainable tests.
Also applies to: 163-166
| } = require('../../scripts/markdown/check-markdown.ts'); | ||
| const { logger } = require('../../scripts/utils/logger.ts'); |
There was a problem hiding this comment.
Fix incorrect TypeScript import extensions.
Node.js require statements cannot directly import TypeScript files. The .ts extension should be removed or changed to .js as the TypeScript files will be compiled to JavaScript before execution.
-} = require('../../scripts/markdown/check-markdown.ts');
-const { logger } = require('../../scripts/utils/logger.ts');
+} = require('../../scripts/markdown/check-markdown');
+const { logger } = require('../../scripts/utils/logger');📝 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.
| } = require('../../scripts/markdown/check-markdown.ts'); | |
| const { logger } = require('../../scripts/utils/logger.ts'); | |
| } = require('../../scripts/markdown/check-markdown'); | |
| const { logger } = require('../../scripts/utils/logger'); |
| @@ -1,24 +1,23 @@ | |||
| const { buildNavTree } = require('../../scripts/build-docs'); | |||
| const { buildNavTree } = require('../../scripts/build-docs.ts'); | |||
There was a problem hiding this comment.
Fix the import path for the TypeScript module.
The import statement with .ts extension won't work directly with Node.js/Jest. You should:
- Import the compiled JavaScript file instead of the TypeScript source
- Remove the file extension from the import path (let the module resolution handle it)
-const { buildNavTree } = require('../../scripts/build-docs.ts');
+const { buildNavTree } = require('../../scripts/build-docs');📝 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 { buildNavTree } = require('../../scripts/build-docs.ts'); | |
| const { buildNavTree } = require('../../scripts/build-docs'); |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
tests/markdown/check-markdown.test.js (4)
13-15: Consider using jest.spyOn for logger mocks.Instead of manually creating mock functions, consider using
jest.spyOnfor better type safety and automatic cleanup:-jest.mock('../../scripts/utils/logger', () => ({ - logger: { error: jest.fn(), warn: jest.fn() } -})); +jest.mock('../../scripts/utils/logger'); + +beforeEach(() => { + jest.spyOn(logger, 'error').mockImplementation(); + jest.spyOn(logger, 'warn').mockImplementation(); +}); + +afterEach(() => { + jest.clearAllMocks(); +});
28-32: Improve error handling in cleanup.The cleanup could fail silently. Consider adding error handling:
afterEach(async () => { mockConsoleError.mockRestore(); mockProcessExit.mockRestore(); - await fs.rm(tempDir, { recursive: true, force: true }); + try { + await fs.rm(tempDir, { recursive: true, force: true }); + } catch (error) { + console.warn(`Failed to clean up temp directory: ${error.message}`); + } });
62-70: Improve test assertion specificity.The test checks for errors in markdown files but doesn't verify the specific error message content. Consider adding more specific assertions:
await checkMarkdownFiles(tempDir, validateBlogs); - expect(logger.warn).toHaveBeenCalledWith(expect.stringContaining('Errors in file invalid.md:')); + expect(logger.warn).toHaveBeenCalledWith( + expect.stringContaining('Errors in file invalid.md:'), + expect.arrayContaining([ + 'Date is required', + 'Tags are required', + 'Cover is required', + 'Authors are required' + ]) + ); mockConsoleLog.mockRestore();
91-97: Add test coverage for specific error types.The test verifies that an error is thrown but doesn't check specific error handling for different scenarios. Consider adding test cases for:
- Permission denied errors
- Invalid file format errors
- Network errors when reading remote files
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
tests/markdown/check-markdown.test.js(1 hunks)tests/utils.test.js(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- tests/utils.test.js
🔇 Additional comments (2)
tests/markdown/check-markdown.test.js (2)
10-11: Fix incorrect TypeScript import extensions.Node.js
requirestatements cannot directly import TypeScript files. The.tsextension should be removed or changed to.jsas the TypeScript files will be compiled to JavaScript before execution.
150-157: Remove duplicate test cases.The following test cases are duplicated:
- URL validation test (lines 150-157 duplicates lines 141-148)
- Missing frontmatter test (lines 163-166 duplicates lines 159-162)
Remove the duplicate test cases to maintain clean and maintainable tests.
Also applies to: 163-166
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
scripts/dashboard/build-dashboard.ts (2)
29-35: Consider extracting magic number to a named constantThe magic number
2592000would be more maintainable as a named constant with clear units.+const SECONDS_PER_MONTH = 30 * 24 * 60 * 60; // 30 days * 24 hours * 60 minutes * 60 seconds + function monthsSince(date: string) { const seconds = Math.floor((new Date().valueOf() - new Date(date).valueOf()) / 1000); - // 2592000 = number of seconds in a month = 30 * 24 * 60 * 60 - const months = seconds / 2592000; + const months = seconds / SECONDS_PER_MONTH;
259-262: Improve error handling in start functionConsider propagating the error to allow proper handling by callers.
} catch (e) { logger.error('There were some issues parsing data from github.'); logger.error(e); + throw e; // Propagate error to allow proper handling by callers }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
scripts/dashboard/build-dashboard.ts(1 hunks)
🔇 Additional comments (4)
scripts/dashboard/build-dashboard.ts (4)
1-20: Well-structured imports and type definitions!The separation of type imports and the organization of dependencies is clean and follows TypeScript best practices.
21-23: Correct ESM path resolution implementation!The use of
fileURLToPathanddirnameensures proper path resolution in ESM context.
143-147: Remove non-null assertion for better type safetyThe non-null assertion on
reviews.nodescould lead to runtime errors.- discussion.reviews.nodes!.reduce((acc, curr) => acc + curr.comments.totalCount, 0) + (discussion.reviews.nodes?.reduce((acc, curr) => acc + curr.comments.totalCount, 0) ?? 0)
249-251: Type assertions are intentionalAs confirmed in previous discussions, the type assertions are intentional and the types are well-understood.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
scripts/build-rss.ts (3)
11-15:⚠️ Potential issueRefactor getAllPosts to prevent build system failures.
Based on previous learnings from PR #3101, making
getAllPosts()asynchronous can cause build system failures. Additionally:
- Using
anytype assertion reduces TypeScript's type safety benefits- Direct JSON import at build time might be problematic
Consider this alternative implementation:
-async function getAllPosts() { - const posts = ((await import('../config/posts.json')) as any).default; - - return posts; -} +import type { BlogPost } from '@/types/scripts/build-rss'; +import posts from '../config/posts.json'; + +interface Posts { + [key: string]: BlogPost[]; +} + +function getAllPosts(): Posts { + return posts; +}
23-34: 🛠️ Refactor suggestionConsider using a robust HTML sanitization library.
The current manual string replacements could miss edge cases or unhandled HTML entities, potentially leading to XSS vulnerabilities.
Consider using a dedicated HTML sanitization library like
html-entities:-function clean(s: string) { - let cleanS = s; - - cleanS = cleanS.split('<span>').join(''); - cleanS = cleanS.split('&').join('&'); - cleanS = cleanS.split(''').join("'"); - cleanS = cleanS.split('<').join('<'); - cleanS = cleanS.split('>').join('>'); - cleanS = cleanS.split('"').join('"'); - - return cleanS; -} +import { decode } from 'html-entities'; + +function clean(s: string): string { + return decode(s); +}
129-129: 🛠️ Refactor suggestionReplace dummy enclosure length.
Using a dummy value for the enclosure length could cause RSS reader compatibility issues.
Consider calculating the actual file size:
async function getImageSize(url: string): Promise<number> { try { const response = await fetch(url); const contentLength = response.headers.get('content-length'); return contentLength ? parseInt(contentLength, 10) : 0; } catch (error) { console.warn(`Failed to get image size for ${url}:`, error); return 0; } }
🧹 Nitpick comments (13)
scripts/build-docs.ts (3)
1-6: Optimize lodash imports for better tree-shaking.Instead of importing the entire lodash library, use the modular import for better tree-shaking:
-import lodash from 'lodash'; -const { sortBy } = lodash; +import { sortBy } from 'lodash';
115-117: Improve error message construction in buildNavTree.The error message could be more descriptive by properly handling different error types:
- throw new Error(`Failed to build navigation tree: ${err}`); + throw new Error(`Failed to build navigation tree: ${err instanceof Error ? err.message : String(err)}`);
184-239: Consider extracting navigation logic into smaller functions.The map operation is quite complex with multiple nested conditions. Consider breaking it down into smaller, more focused functions for better maintainability:
- Extract next page logic into
getNextPage(post, index, structuredPosts)- Extract previous page logic into
getPrevPage(post, index, structuredPosts, rootSections)Example:
function getNextPage(index: number, structuredPosts: Details[]): NavigationPage { if (index + 1 >= structuredPosts.length) return {}; if (!structuredPosts[index + 1].isRootElement && !structuredPosts[index + 1].isSection) { return { title: structuredPosts[index + 1].title, href: structuredPosts[index + 1].slug }; } return { title: `${structuredPosts[index + 1].title} - ${structuredPosts[index + 2].title}`, href: structuredPosts[index + 2].slug }; }scripts/build-rss.ts (1)
45-144: Consider breaking down the rssFeed function.The function is quite long and handles multiple responsibilities. Consider splitting it into smaller, more focused functions for better maintainability.
Suggested breakdown:
validatePosts: Handle post validation (lines 48-63, 88-92)createRSSChannel: Set up the RSS channel metadata (lines 68-86)createRSSItem: Create individual RSS items (lines 107-134)generateAndSaveXML: Handle XML generation and file writing (lines 136-140)scripts/build-post-list.ts (3)
1-2: Consider addressing ESLint warnings instead of disabling them.The ESLint rules
no-await-in-loopandmax-depthare disabled globally. These rules typically indicate potential performance issues and complex code structure.Consider:
- Refactoring loops to use
Promise.all()where possible- Breaking down deeply nested code into smaller functions
35-47: Enhance theslugifyToCfunction implementation.The function could be improved for better type safety and performance.
-export function slugifyToC(str: string) { +export function slugifyToC(str: string): string { + // Pre-compile regex for better performance + static readonly HEADING_ID_REGEX = /[\s]*(?:\{#([a-zA-Z0-9\-_]+)\}|<a[\s]+name="([a-zA-Z0-9\-_]+)")/; + - if (typeof str !== 'string') return ''; - if (!str.trim()) return ''; + if (typeof str !== 'string' || !str.trim()) return ''; let slug = ''; const idMatch = str.match(HEADING_ID_REGEX); const [, headingId, anchorId] = idMatch || []; slug = (headingId || anchorId || '').trim(); return slug; }
258-284: Improve error handling inbuildPostList.The error handling could be enhanced with better typing and more specific error messages.
+type BuildPostListError = Error & { + code?: 'INVALID_BASE_PATH' | 'INVALID_WRITE_PATH' | 'EMPTY_DIRECTORIES' | 'PROCESSING_ERROR'; +}; export async function buildPostList( postDirectories: string[][], basePath: string, writeFilePath: string ): Promise<void> { try { if (!basePath) { - throw new Error('Error while building post list: basePath is required'); + const error: BuildPostListError = new Error('basePath is required'); + error.code = 'INVALID_BASE_PATH'; + throw error; } // ... rest of the validation } catch (error) { - throw new Error(`Error while building post list: ${(error as Error).message}`, { cause: error }); + const buildError = error as BuildPostListError; + throw new Error( + `Build failed: ${buildError.code ? `[${buildError.code}] ` : ''}${buildError.message}`, + { cause: error } + ); } }scripts/markdown/check-markdown.ts (6)
16-25: Consider restricting URL protocols for better security.The current URL validation accepts any valid URL format. Consider restricting to specific protocols (http/https) for better security.
function isValidURL(str: string) { try { // eslint-disable-next-line no-new - new URL(str); + const url = new URL(str); + return ['http:', 'https:'].includes(url.protocol); - return true; } catch (err) { return false; } }
30-38: Enhance type safety with more specific types.Consider using string literals for type and adding validation constraints through TypeScript.
interface FrontMatter { title: string; - date: string; + date: `${number}-${number}-${number}`; // YYYY-MM-DD format - type: string; + type: 'blog' | 'doc'; tags: string[]; cover: string; weight?: number; authors: { name: string; link: string; photo: string; }[]; }
63-65: Strengthen date validation.The current date validation accepts any valid date format. Consider enforcing a specific format (e.g., YYYY-MM-DD).
- if (frontmatter.date && Number.isNaN(Date.parse(frontmatter.date))) { + const dateRegex = /^\d{4}-\d{2}-\d{2}$/; + if (!frontmatter.date || !dateRegex.test(frontmatter.date) || Number.isNaN(Date.parse(frontmatter.date))) { errors.push(`Invalid date format: ${frontmatter.date}`); }
97-99: Remove unnecessary istanbul ignore.The
istanbul ignore nextseems unnecessary here as this line should be covered by tests.- /* istanbul ignore next */ return errors.length ? errors : null;
131-174: Consider batch processing and explicit error handling.For large directories, processing all files in parallel might consume too much memory. Consider implementing batch processing and adding explicit error handling for file operations.
async function checkMarkdownFiles( folderPath: string, validateFunction: (frontmatter: FrontMatter) => string[] | null, relativePath = '' ) { try { const files = await fs.readdir(folderPath); - const filePromises = files.map(async (file) => { + // Process files in batches of 50 + const BATCH_SIZE = 50; + for (let i = 0; i < files.length; i += BATCH_SIZE) { + const batch = files.slice(i, i + BATCH_SIZE); + const filePromises = batch.map(async (file) => { const filePath = path.join(folderPath, file); const relativeFilePath = path.join(relativePath, file);
182-192: Implement graceful shutdown handling.Consider implementing signal handlers for graceful shutdown, especially important for large directories.
async function main() { + const shutdown = async (signal: string) => { + logger.warn(`Received ${signal}. Cleaning up...`); + // Allow ongoing validations to complete + await Promise.allSettled([/* ongoing promises */]); + process.exit(0); + }; + + process.on('SIGTERM', () => shutdown('SIGTERM')); + process.on('SIGINT', () => shutdown('SIGINT')); + try { await Promise.all([ checkMarkdownFiles(docsFolderPath, validateDocs),
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
package.json(5 hunks)scripts/build-docs.ts(1 hunks)scripts/build-post-list.ts(1 hunks)scripts/build-rss.ts(1 hunks)scripts/markdown/check-markdown.ts(1 hunks)scripts/tools/tools-object.ts(1 hunks)tests/dashboard/build-dashboard.test.js(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- tests/dashboard/build-dashboard.test.js
- scripts/tools/tools-object.ts
- package.json
🧰 Additional context used
📓 Learnings (1)
scripts/build-rss.ts (1)
Learnt from: akshatnema
PR: asyncapi/website#3101
File: scripts/build-rss.js:4-6
Timestamp: 2024-11-12T06:59:33.852Z
Learning: Converting `getAllPosts()` to asynchronous in `scripts/build-rss.js` causes the existing build system to fail.
🔇 Additional comments (8)
scripts/build-docs.ts (3)
133-134: Previous review comment about guarding against undefined access is still valid.
146-147: Previous review comment about error message construction is still valid.
247-247: LGTM!The exports are clean and follow best practices.
scripts/build-rss.ts (1)
1-4: LGTM! Clean imports following TypeScript best practices.The imports are well-organized, using ES module syntax and properly importing types from a dedicated types file.
scripts/build-post-list.ts (2)
16-23: Consider encapsulating global state.The global mutable variables
specWeight,finalResult, andreleaseNotesshould be encapsulated within a class or function scope to prevent potential issues in concurrent scenarios.
147-247: Refactor walkDirectories for better maintainability.The function is complex and should be split into smaller, more focused functions with better error handling.
scripts/markdown/check-markdown.ts (2)
1-10: LGTM! Well-structured imports and setup.The code uses modern ES modules, async file operations, and a custom logger utility instead of console.log.
110-119: Remove unnecessary istanbul ignores and improve test coverage.The
istanbul ignore elsedirectives suggest incomplete test coverage. Consider adding test cases for these conditions instead of ignoring them.
| */ | ||
| export async function rssFeed(type: BlogPostTypes, rssTitle: string, desc: string, outputPath: string) { | ||
| try { | ||
| let posts = (await getAllPosts())[`${type}`] as any[]; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove type assertion to improve type safety.
The type assertion to any[] reduces TypeScript's type safety benefits.
- let posts = (await getAllPosts())[`${type}`] as any[];
+ let posts = (await getAllPosts())[type];Committable suggestion skipped: line range outside the PR's diff.
| for (const dir of directories) { | ||
| const directory = posix.normalize(dir[0]); | ||
| /* istanbul ignore next */ | ||
| const sectionSlug = dir[1] || ''; | ||
| const files = await readdir(directory); | ||
|
|
||
| for (const file of files) { | ||
| let details: Details; | ||
| const fileName = normalize(join(directory, file)); | ||
| const fileNameWithSection = normalize(join(fileName, '_section.mdx')); | ||
| const slug = `/${normalize(relative(basePath, fileName)).replace(/\\/g, '/')}`; | ||
| const slugElements = slug.split('/'); | ||
|
|
||
| if (await isDirectory(fileName)) { | ||
| if (await pathExists(fileNameWithSection)) { | ||
| // Passing a second argument to frontMatter disables cache. See https://github.com/asyncapi/website/issues/1057 | ||
| details = frontMatter(await readFile(fileNameWithSection, 'utf-8'), {}).data as Details; | ||
| /* istanbul ignore next */ | ||
| details.title = details.title || capitalize(basename(fileName)); | ||
| } else { | ||
| details = { | ||
| title: capitalize(basename(fileName)) | ||
| }; | ||
| } | ||
| details.isSection = true; | ||
| if (slugElements.length > 3) { | ||
| details.parent = slugElements[slugElements.length - 2]; | ||
| details.sectionId = slugElements[slugElements.length - 1]; | ||
| } | ||
| if (!details.parent) { | ||
| details.isRootSection = true; | ||
| details.rootSectionId = slugElements[slugElements.length - 1]; | ||
| } | ||
| details.sectionWeight = sectionWeight; | ||
| details.slug = slug; | ||
| addItem(details); | ||
| const rootId = details.parent || details.rootSectionId; | ||
|
|
||
| await walkDirectories( | ||
| [[fileName, slug]], | ||
| resultObj, | ||
| basePath, | ||
| details.title, | ||
| details.sectionId, | ||
| rootId, | ||
| details.sectionWeight | ||
| ); | ||
| } else if (file.endsWith('.mdx') && !fileName.endsWith(`${sep}_section.mdx`)) { | ||
| const fileContent = await readFile(fileName, 'utf-8'); | ||
| // Passing a second argument to frontMatter disables cache. See https://github.com/asyncapi/website/issues/1057 | ||
| const { data, content } = frontMatter(fileContent, {}); | ||
|
|
||
| details = data as Details; | ||
| details.toc = toc(content, { slugify: slugifyToC }).json; | ||
| details.readingTime = Math.ceil(readingTime(content).minutes); | ||
| details.excerpt = details.excerpt || markdownToTxt(content).substr(0, 200); | ||
| /* istanbul ignore next */ | ||
| details.sectionSlug = sectionSlug || slug.replace(/\.mdx$/, ''); | ||
| details.sectionWeight = sectionWeight; | ||
| details.sectionTitle = sectionTitle; | ||
| details.sectionId = sectionId; | ||
| details.rootSectionId = rootSectionId; | ||
| details.id = fileName.replace(/\\/g, '/'); | ||
| details.isIndex = fileName.endsWith(join('index.mdx')); | ||
| details.slug = details.isIndex ? sectionSlug : slug.replace(/\.mdx$/, ''); | ||
| if (details.slug.includes('/reference/specification/') && !details.title) { | ||
| const fileBaseName = basename(details.slug); | ||
| const versionDetails = getVersionDetails(details.slug, specWeight--); | ||
|
|
||
| details.title = versionDetails.title; | ||
| details.weight = versionDetails.weight; | ||
|
|
||
| if (releaseNotes.includes(details.title)) { | ||
| details.releaseNoteLink = `/blog/release-notes-${details.title}`; | ||
| } | ||
|
|
||
| details = handleSpecificationVersion(details, fileBaseName); | ||
| } | ||
|
|
||
| // To create a list of available ReleaseNotes list, which will be used to add details.releaseNoteLink attribute. | ||
| if (file.startsWith('release-notes') && dir[1] === '/blog') { | ||
| const { name } = parse(file); | ||
| const version = name.split('-').pop(); | ||
|
|
||
| releaseNotes.push(version); | ||
| } | ||
|
|
||
| addItem(details); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Add proper error handling for file operations.
The walkDirectories function performs multiple file system operations without proper error handling.
Add try-catch blocks around file operations:
for (const dir of directories) {
+ try {
const directory = posix.normalize(dir[0]);
const sectionSlug = dir[1] || '';
const files = await readdir(directory);
for (const file of files) {
// ... rest of the code
}
+ } catch (error) {
+ throw new Error(`Failed to process directory: ${error instanceof Error ? error.message : 'Unknown error'}`);
+ }
}📝 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.
| for (const dir of directories) { | |
| const directory = posix.normalize(dir[0]); | |
| /* istanbul ignore next */ | |
| const sectionSlug = dir[1] || ''; | |
| const files = await readdir(directory); | |
| for (const file of files) { | |
| let details: Details; | |
| const fileName = normalize(join(directory, file)); | |
| const fileNameWithSection = normalize(join(fileName, '_section.mdx')); | |
| const slug = `/${normalize(relative(basePath, fileName)).replace(/\\/g, '/')}`; | |
| const slugElements = slug.split('/'); | |
| if (await isDirectory(fileName)) { | |
| if (await pathExists(fileNameWithSection)) { | |
| // Passing a second argument to frontMatter disables cache. See https://github.com/asyncapi/website/issues/1057 | |
| details = frontMatter(await readFile(fileNameWithSection, 'utf-8'), {}).data as Details; | |
| /* istanbul ignore next */ | |
| details.title = details.title || capitalize(basename(fileName)); | |
| } else { | |
| details = { | |
| title: capitalize(basename(fileName)) | |
| }; | |
| } | |
| details.isSection = true; | |
| if (slugElements.length > 3) { | |
| details.parent = slugElements[slugElements.length - 2]; | |
| details.sectionId = slugElements[slugElements.length - 1]; | |
| } | |
| if (!details.parent) { | |
| details.isRootSection = true; | |
| details.rootSectionId = slugElements[slugElements.length - 1]; | |
| } | |
| details.sectionWeight = sectionWeight; | |
| details.slug = slug; | |
| addItem(details); | |
| const rootId = details.parent || details.rootSectionId; | |
| await walkDirectories( | |
| [[fileName, slug]], | |
| resultObj, | |
| basePath, | |
| details.title, | |
| details.sectionId, | |
| rootId, | |
| details.sectionWeight | |
| ); | |
| } else if (file.endsWith('.mdx') && !fileName.endsWith(`${sep}_section.mdx`)) { | |
| const fileContent = await readFile(fileName, 'utf-8'); | |
| // Passing a second argument to frontMatter disables cache. See https://github.com/asyncapi/website/issues/1057 | |
| const { data, content } = frontMatter(fileContent, {}); | |
| details = data as Details; | |
| details.toc = toc(content, { slugify: slugifyToC }).json; | |
| details.readingTime = Math.ceil(readingTime(content).minutes); | |
| details.excerpt = details.excerpt || markdownToTxt(content).substr(0, 200); | |
| /* istanbul ignore next */ | |
| details.sectionSlug = sectionSlug || slug.replace(/\.mdx$/, ''); | |
| details.sectionWeight = sectionWeight; | |
| details.sectionTitle = sectionTitle; | |
| details.sectionId = sectionId; | |
| details.rootSectionId = rootSectionId; | |
| details.id = fileName.replace(/\\/g, '/'); | |
| details.isIndex = fileName.endsWith(join('index.mdx')); | |
| details.slug = details.isIndex ? sectionSlug : slug.replace(/\.mdx$/, ''); | |
| if (details.slug.includes('/reference/specification/') && !details.title) { | |
| const fileBaseName = basename(details.slug); | |
| const versionDetails = getVersionDetails(details.slug, specWeight--); | |
| details.title = versionDetails.title; | |
| details.weight = versionDetails.weight; | |
| if (releaseNotes.includes(details.title)) { | |
| details.releaseNoteLink = `/blog/release-notes-${details.title}`; | |
| } | |
| details = handleSpecificationVersion(details, fileBaseName); | |
| } | |
| // To create a list of available ReleaseNotes list, which will be used to add details.releaseNoteLink attribute. | |
| if (file.startsWith('release-notes') && dir[1] === '/blog') { | |
| const { name } = parse(file); | |
| const version = name.split('-').pop(); | |
| releaseNotes.push(version); | |
| } | |
| addItem(details); | |
| } | |
| } | |
| } | |
| for (const dir of directories) { | |
| try { | |
| const directory = posix.normalize(dir[0]); | |
| /* istanbul ignore next */ | |
| const sectionSlug = dir[1] || ''; | |
| const files = await readdir(directory); | |
| for (const file of files) { | |
| let details: Details; | |
| const fileName = normalize(join(directory, file)); | |
| const fileNameWithSection = normalize(join(fileName, '_section.mdx')); | |
| const slug = `/${normalize(relative(basePath, fileName)).replace(/\\/g, '/')}`; | |
| const slugElements = slug.split('/'); | |
| if (await isDirectory(fileName)) { | |
| if (await pathExists(fileNameWithSection)) { | |
| // Passing a second argument to frontMatter disables cache. See https://github.com/asyncapi/website/issues/1057 | |
| details = frontMatter(await readFile(fileNameWithSection, 'utf-8'), {}).data as Details; | |
| /* istanbul ignore next */ | |
| details.title = details.title || capitalize(basename(fileName)); | |
| } else { | |
| details = { | |
| title: capitalize(basename(fileName)) | |
| }; | |
| } | |
| details.isSection = true; | |
| if (slugElements.length > 3) { | |
| details.parent = slugElements[slugElements.length - 2]; | |
| details.sectionId = slugElements[slugElements.length - 1]; | |
| } | |
| if (!details.parent) { | |
| details.isRootSection = true; | |
| details.rootSectionId = slugElements[slugElements.length - 1]; | |
| } | |
| details.sectionWeight = sectionWeight; | |
| details.slug = slug; | |
| addItem(details); | |
| const rootId = details.parent || details.rootSectionId; | |
| await walkDirectories( | |
| [[fileName, slug]], | |
| resultObj, | |
| basePath, | |
| details.title, | |
| details.sectionId, | |
| rootId, | |
| details.sectionWeight | |
| ); | |
| } else if (file.endsWith('.mdx') && !fileName.endsWith(`${sep}_section.mdx`)) { | |
| const fileContent = await readFile(fileName, 'utf-8'); | |
| // Passing a second argument to frontMatter disables cache. See https://github.com/asyncapi/website/issues/1057 | |
| const { data, content } = frontMatter(fileContent, {}); | |
| details = data as Details; | |
| details.toc = toc(content, { slugify: slugifyToC }).json; | |
| details.readingTime = Math.ceil(readingTime(content).minutes); | |
| details.excerpt = details.excerpt || markdownToTxt(content).substr(0, 200); | |
| /* istanbul ignore next */ | |
| details.sectionSlug = sectionSlug || slug.replace(/\.mdx$/, ''); | |
| details.sectionWeight = sectionWeight; | |
| details.sectionTitle = sectionTitle; | |
| details.sectionId = sectionId; | |
| details.rootSectionId = rootSectionId; | |
| details.id = fileName.replace(/\\/g, '/'); | |
| details.isIndex = fileName.endsWith(join('index.mdx')); | |
| details.slug = details.isIndex ? sectionSlug : slug.replace(/\.mdx$/, ''); | |
| if (details.slug.includes('/reference/specification/') && !details.title) { | |
| const fileBaseName = basename(details.slug); | |
| const versionDetails = getVersionDetails(details.slug, specWeight--); | |
| details.title = versionDetails.title; | |
| details.weight = versionDetails.weight; | |
| if (releaseNotes.includes(details.title)) { | |
| details.releaseNoteLink = `/blog/release-notes-${details.title}`; | |
| } | |
| details = handleSpecificationVersion(details, fileBaseName); | |
| } | |
| // To create a list of available ReleaseNotes list, which will be used to add details.releaseNoteLink attribute. | |
| if (file.startsWith('release-notes') && dir[1] === '/blog') { | |
| const { name } = parse(file); | |
| const version = name.split('-').pop(); | |
| releaseNotes.push(version); | |
| } | |
| addItem(details); | |
| } | |
| } | |
| } catch (error) { | |
| throw new Error(`Failed to process directory: ${error instanceof Error ? error.message : 'Unknown error'}`); | |
| } | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
scripts/markdown/check-edit-links.ts (1)
186-187: Log errors usinglogger.errorbefore throwing in themainfunctionIn the
mainfunction, if an error occurs, you throw a new error without logging it. It's a good practice to log errors before throwing them to aid in debugging and monitoring.Apply this diff to improve error logging:
} catch (error) { - throw new Error(`Failed to check edit links: ${error}`); + logger.error(`Failed to check edit links: ${error}`); + throw new Error(`Failed to check edit links: ${error}`); }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
package.json(5 hunks)scripts/markdown/check-edit-links.ts(5 hunks)tests/markdown/check-edit-links.test.js(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- package.json
🔇 Additional comments (3)
scripts/markdown/check-edit-links.ts (1)
2-2:⚠️ Potential issueCorrect the module name for 'node-fetch' import
On line 2, the module
'node-fetch-2'is being imported. The standard package name is'node-fetch'. Please verify the module name and ensure it matches the installed dependency in your project.Apply this diff to correct the import statement:
-import fetch from 'node-fetch-2'; +import fetch from 'node-fetch';If you intended to use a different package named
'node-fetch-2', please ensure it's properly installed and compatible with your codebase.tests/markdown/check-edit-links.test.js (2)
10-12: Ensure compatibility when requiring TypeScript files in JavaScript testsYou're requiring TypeScript files directly in your JavaScript test file. This can cause runtime errors if your testing environment doesn't support importing TypeScript modules directly. Consider one of the following:
- Compile the TypeScript files before running tests.
- Convert your test file to TypeScript for consistency.
- Adjust your test environment to handle TypeScript modules.
Please ensure that your test environment is configured to handle TypeScript modules appropriately.
Line range hint
2-2: Correct the module name for 'node-fetch' importOn line 2, the module
'node-fetch-2'is being required. The correct package name is'node-fetch'. Ensure that the package name matches the installed dependency to prevent runtime errors.Apply this diff to correct the require statement:
-const fetch = require('node-fetch-2'); +const fetch = require('node-fetch');If a different package named
'node-fetch-2'is intended, verify that it's installed and compatible with your test environment.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (7)
scripts/markdown/check-edit-links.ts (7)
1-8: Consider using a more modern HTTP client.The
node-fetch-2package is outdated. Consider usingundici(Node.js built-in fetch) ornode-fetchv3 for better maintenance and security.-import fetch from 'node-fetch-2'; +import { fetch } from 'undici';
16-20: Enhance interface documentation.The
PathObjectinterface could benefit from JSDoc comments describing each property's purpose.+/** + * Represents a markdown file's paths and its corresponding edit link + */ interface PathObject { + /** The file system path to the markdown file */ filePath: string; + /** The URL path where the file will be served */ urlPath: string; + /** The GitHub edit link for the file, null if not applicable */ editLink: string | null; }
Line range hint
27-52: Improve error handling in processBatch.The error handling could be improved in several ways:
- The error is rejected without being logged
- The timeout logic could be extracted into a reusable utility
Consider this refactor:
+const checkWithTimeout = async (url: string, timeout: number) => { + const controller = new AbortController(); + const timeoutId = setTimeout(() => controller.abort(), timeout); + try { + const response = await fetch(url, { + method: 'HEAD', + signal: controller.signal + }); + return response; + } finally { + clearTimeout(timeoutId); + } +}; async function processBatch(batch: PathObject[]): Promise<(PathObject | null)[]> { const TIMEOUT_MS = Number(process.env.DOCS_LINK_CHECK_TIMEOUT) || 5000; return Promise.all( batch.map(async ({ filePath, urlPath, editLink }) => { try { if (!editLink || ignoreFiles.some((ignorePath) => filePath.endsWith(ignorePath))) return null; - const controller = new AbortController(); - const timeout = setTimeout(() => controller.abort(), TIMEOUT_MS); - const response = await fetch(editLink, { - method: 'HEAD', - signal: controller.signal - }); - - clearTimeout(timeout); + const response = await checkWithTimeout(editLink, TIMEOUT_MS); if (response.status === 404) { return { filePath, urlPath, editLink }; } return null; } catch (error) { + logger.error(`Error checking ${editLink}:`, error); return Promise.reject(new Error(`Error checking ${editLink}: ${error}`)); } }) ); }
60-86: Improve type safety in checkUrls.The function could benefit from stronger typing and better error handling:
- The batch size should be validated
- The pause duration should be configurable
Consider this enhancement:
+const DEFAULT_BATCH_SIZE = 5; +const DEFAULT_PAUSE_DURATION = 1000; -async function checkUrls(paths: PathObject[]): Promise<PathObject[]> { +async function checkUrls( + paths: PathObject[], + options?: { + batchSize?: number; + pauseDuration?: number; + } +): Promise<PathObject[]> { const result: PathObject[] = []; - const batchSize = Number(process.env.DOCS_LINK_CHECK_BATCH_SIZE) || 5; + const batchSize = Math.max(1, Number(process.env.DOCS_LINK_CHECK_BATCH_SIZE) || options?.batchSize || DEFAULT_BATCH_SIZE); + const pauseDuration = Math.max(0, Number(process.env.DOCS_LINK_CHECK_PAUSE_DURATION) || options?.pauseDuration || DEFAULT_PAUSE_DURATION); // ... rest of the function ... - await pause(1000); + await pause(pauseDuration);
95-113: Improve type safety in determineEditLink.The function could benefit from a custom type for edit options and better null handling:
+interface EditOption { + /** The path segment to match against */ + value: string; + /** The base URL for edit links */ + href: string; +} function determineEditLink( urlPath: string, filePath: string, - editOptions: { value: string; href: string }[] + editOptions: EditOption[] ): string | null { // Remove leading 'docs/' if present for matching const pathForMatching = urlPath.startsWith('docs/') ? urlPath.slice(5) : urlPath; - const target = editOptions.find((edit) => pathForMatching.includes(edit.value)); + const target = editOptions.find((edit) => + edit.value === '' || pathForMatching.includes(edit.value) + ); - // Handle the empty value case (fallback) - if (target?.value === '') { + if (!target) { + return null; + } + + if (target.value === '') { return `${target.href}/docs/${urlPath}.md`; } - // For other cases with specific targets - /* istanbul ignore next */ - return target ? `${target.href}/${path.basename(filePath)}` : null; + return `${target.href}/${path.basename(filePath)}`; }
144-145: Remove unnecessary istanbul ignore.The
/* istanbul ignore else */comment seems unnecessary as theelsebranch is covered by the subsequentelse ifcondition.- /* istanbul ignore else */ - if (stats.isDirectory()) {
166-192: Improve main function configuration and error handling.The main function could benefit from:
- Configuration options for batch size and pause duration
- Better error logging before throwing
+interface CheckLinksOptions { + batchSize?: number; + pauseDuration?: number; + timeout?: number; +} -async function main() { +async function main(options?: CheckLinksOptions) { const editOptions = editUrls; try { const currentFilePath = fileURLToPath(import.meta.url); const currentDirPath = dirname(currentFilePath); const docsFolderPath = path.resolve(currentDirPath, '../../markdown/docs'); const paths = await generatePaths(docsFolderPath, editOptions); logger.info('Starting URL checks...'); - const invalidUrls = await checkUrls(paths); + const invalidUrls = await checkUrls(paths, { + batchSize: options?.batchSize, + pauseDuration: options?.pauseDuration + }); if (invalidUrls.length > 0) { logger.info('\nURLs returning 404:\n'); invalidUrls.forEach((url) => logger.info(`- ${url.editLink} generated from ${url.filePath}\n`)); logger.info(`\nTotal invalid URLs found: ${invalidUrls.length}`); } else { logger.info('All URLs are valid.'); } } catch (error) { + logger.error('Failed to check edit links:', error); throw new Error(`Failed to check edit links: ${error}`); } }
Description
✅ Deploy Preview for asyncapi-website ready!
Built without sensitive environment variables
Toggle QR Code...
Use your smartphone camera to open QR code link.
To edit notification comments on pull requests, go to your Netlify site configuration.
Related issue(s)
Related to #3187
Summary by CodeRabbit
Release Notes
🚀 New Features
next-i18next.config.cjs.🔧 Improvements
🧹 Chores
🔒 Security & Performance
💻 Developer Experience