test #4200
test #4200
Conversation
sagarkori143
requested review from
Mayaleeeee,
akshatnema,
anshgoyalevil,
asyncapi-bot-eve,
derberg and
sambhavgupta0705
as code owners
WalkthroughA new orchestration system for build tasks is introduced, with runner modules wrapping individual build scripts and a centralized Changes
Sequence Diagram(s)sequenceDiagram
participant Orchestrator (npm/index.ts)
participant Runner Modules
participant Build Scripts
participant Logger
Orchestrator->>Runner Modules: Import and initialize build tasks
Orchestrator->>Runner Modules: Execute all tasks concurrently (Promise.allSettled)
Runner Modules->>Build Scripts: Invoke exported async build functions
Build Scripts-->>Runner Modules: Return result or throw error
Runner Modules-->>Orchestrator: Resolve or reject promise
Orchestrator->>Logger: Log success or error details for each task
Orchestrator->>Orchestrator: Set process exit code based on results
Possibly related PRs
Suggested reviewers
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
npm warn EBADENGINE Unsupported engine { ✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
❌ Deploy Preview for asyncapi-website failed.Built without sensitive environment variables
|
|
We require all PRs to follow Conventional Commits specification. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #4200 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 22 22
Lines 778 778
Branches 144 144
=========================================
Hits 778 778 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🔭 Outside diff range comments (1)
npm/runners/build-tools-runner.ts (1)
9-23: Remove the unnecessary try-catch block and clean up formatting.The try-catch block serves no purpose since it only rethrows the original error. Also, remove the extra blank lines at the end of the file.
-export async function runBuildTools() { - try { - const automatedToolsPath = resolve(currentDirPath, '../../config', 'tools-automated.json'); - const manualToolsPath = resolve(currentDirPath, '../../config', 'tools-manual.json'); - const toolsPath = resolve(currentDirPath, '../../config', 'tools.json'); - const tagsPath = resolve(currentDirPath, '../../config', 'all-tags.json'); - - await buildTools(automatedToolsPath, manualToolsPath, toolsPath, tagsPath); - } catch (error) { - throw error; - } -} - - +export async function runBuildTools() { + const automatedToolsPath = resolve(currentDirPath, '../../config', 'tools-automated.json'); + const manualToolsPath = resolve(currentDirPath, '../../config', 'tools-manual.json'); + const toolsPath = resolve(currentDirPath, '../../config', 'tools.json'); + const tagsPath = resolve(currentDirPath, '../../config', 'all-tags.json'); + + await buildTools(automatedToolsPath, manualToolsPath, toolsPath, tagsPath); +}
🧹 Nitpick comments (2)
npm/runners/build-finance-info-list-runner.ts (1)
11-20: Consider using parseInt for year parsing.Using
parseFloatfor parsing years works butparseIntwould be more semantically appropriate since years are integers..filter((file) => { - return !Number.isNaN(parseFloat(file)); + return !Number.isNaN(parseInt(file, 10)); }) // sort the years in descending order .sort((a, b) => { - return parseFloat(b) - parseFloat(a); + return parseInt(b, 10) - parseInt(a, 10); });npm/runners/build-meetings-runner.ts (1)
8-11: Fix formatting: add spaces before opening braces.Missing spaces before opening braces affects code readability and consistency.
Apply this diff to fix the formatting:
-export async function runBuildMeetings(){ - try{ +export async function runBuildMeetings() { + try { await buildMeetings(resolve(currentDirPath, '../../config', 'meetings.json')); - }catch (error) { + } catch (error) {
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
npm/index.ts(1 hunks)npm/runners/build-adopters-list-runner.ts(1 hunks)npm/runners/build-dashboard-runner.ts(1 hunks)npm/runners/build-finance-info-list-runner.ts(1 hunks)npm/runners/build-meetings-runner.ts(1 hunks)npm/runners/build-newsroom-videos-runner.ts(1 hunks)npm/runners/build-pages-runner.ts(1 hunks)npm/runners/build-post-list-runner.ts(1 hunks)npm/runners/build-rss-runner.ts(1 hunks)npm/runners/build-tools-runner.ts(1 hunks)npm/runners/case-studies-runner.ts(1 hunks)npm/runners/compose-blog-runner.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
npm/runners/build-rss-runner.ts (1)
Learnt from: akshatnema
PR: asyncapi/website#3101
File: scripts/build-rss.js:4-6
Timestamp: 2024-11-01T09:35:23.912Z
Learning: Converting `getAllPosts()` to asynchronous in `scripts/build-rss.js` causes the existing build system to fail.
🧬 Code Graph Analysis (6)
npm/runners/build-adopters-list-runner.ts (1)
scripts/adopters/index.ts (1)
buildAdoptersList(18-20)
npm/runners/build-pages-runner.ts (1)
scripts/build-pages.ts (2)
ensureDirectoryExists(14-18)copyAndRenameFiles(48-81)
npm/runners/build-post-list-runner.ts (1)
scripts/build-post-list.ts (1)
buildPostList(288-314)
npm/runners/case-studies-runner.ts (1)
scripts/casestudies/index.ts (1)
buildCaseStudiesList(18-39)
npm/index.ts (11)
npm/runners/build-post-list-runner.ts (1)
runBuildPostList(8-21)npm/runners/build-dashboard-runner.ts (1)
runBuildDashboard(9-16)npm/runners/build-tools-runner.ts (1)
runBuildTools(9-20)npm/runners/case-studies-runner.ts (1)
runCaseStudies(10-22)npm/runners/build-newsroom-videos-runner.ts (1)
runBuildNewsroomVideos(8-19)npm/runners/build-meetings-runner.ts (1)
runBuildMeetings(8-18)npm/runners/build-finance-info-list-runner.ts (1)
runBuildFinanceInfoList(7-37)npm/runners/build-adopters-list-runner.ts (1)
runBuildAdoptersList(3-9)npm/runners/build-pages-runner.ts (1)
runBuildPages(7-14)npm/runners/build-rss-runner.ts (1)
runBuildRss(3-9)scripts/helpers/logger.ts (1)
logger(18-18)
npm/runners/build-finance-info-list-runner.ts (1)
scripts/finance/index.ts (1)
buildFinanceInfoList(28-58)
🪛 Biome (1.9.4)
npm/runners/build-tools-runner.ts
[error] 18-18: The catch clause that only rethrows the original error is useless.
An unnecessary catch clause can be confusing.
Unsafe fix: Remove the try/catch clause.
(lint/complexity/noUselessCatch)
npm/runners/build-adopters-list-runner.ts
[error] 7-7: The catch clause that only rethrows the original error is useless.
An unnecessary catch clause can be confusing.
Unsafe fix: Remove the try/catch clause.
(lint/complexity/noUselessCatch)
npm/runners/build-rss-runner.ts
[error] 7-7: The catch clause that only rethrows the original error is useless.
An unnecessary catch clause can be confusing.
Unsafe fix: Remove the try/catch clause.
(lint/complexity/noUselessCatch)
npm/runners/build-pages-runner.ts
[error] 12-12: The catch clause that only rethrows the original error is useless.
An unnecessary catch clause can be confusing.
Unsafe fix: Remove the try/catch clause.
(lint/complexity/noUselessCatch)
npm/runners/build-post-list-runner.ts
[error] 19-19: The catch clause that only rethrows the original error is useless.
An unnecessary catch clause can be confusing.
Unsafe fix: Remove the try/catch clause.
(lint/complexity/noUselessCatch)
npm/runners/case-studies-runner.ts
[error] 20-20: The catch clause that only rethrows the original error is useless.
An unnecessary catch clause can be confusing.
Unsafe fix: Remove the try/catch clause.
(lint/complexity/noUselessCatch)
npm/runners/build-dashboard-runner.ts
[error] 14-14: The catch clause that only rethrows the original error is useless.
An unnecessary catch clause can be confusing.
Unsafe fix: Remove the try/catch clause.
(lint/complexity/noUselessCatch)
⏰ Context from checks skipped due to timeout of 180000ms (1)
- GitHub Check: Test NodeJS PR - windows-latest
🔇 Additional comments (15)
npm/runners/build-finance-info-list-runner.ts (1)
26-36: Good error handling with meaningful context.Unlike other runner files, this error handling is valuable as it provides additional context about the failure source, making debugging easier.
npm/runners/build-dashboard-runner.ts (1)
1-8: Path resolution logic looks good.The import structure and path resolution using
fileURLToPathanddirnameis correct for ES modules.npm/runners/compose-blog-runner.ts (2)
5-33: Interactive prompts are well-structured.The inquirer prompts are well-defined with appropriate types and choices for the blog composition workflow.
35-45: Error handling provides meaningful context.The error handling correctly checks for TTY-specific errors and provides appropriate user feedback, which is valuable for interactive CLI tools.
npm/runners/build-meetings-runner.ts (1)
12-17: Error handling provides meaningful context.The error handling correctly adds context to errors while preserving the original error as the cause, which is valuable for debugging.
npm/runners/build-newsroom-videos-runner.ts (3)
1-4: LGTM - Clean imports and ES module setup.The imports are well-organized and the ES module path utilities are correctly imported for dynamic path resolution.
5-7: LGTM - Proper ES module path resolution.The path resolution using
import.meta.urlandfileURLToPathis the correct modern approach for ES modules to determine the current file's directory.
8-19: LGTM - Excellent error handling pattern.The error handling implementation is consistent with the best practices seen in
build-meetings-runner.ts. It properly:
- Wraps errors with contextual messages
- Preserves the original error as the cause
- Handles both Error instances and other thrown values
- Uses the modern
{ cause: error }pattern for error chainingThis is superior to the simpler error handling patterns used in some other runners.
npm/index.ts (7)
1-11: LGTM - Well-organized imports.The imports are clearly organized with runner functions imported first, followed by the logger utility. All necessary dependencies are included.
13-27: LGTM - Clean task definition structure.The build tasks array provides a clean, maintainable way to define and organize all build operations. The naming convention is consistent and descriptive.
29-33: LGTM - Proper concurrent execution with Promise.allSettled.Using
Promise.allSettledis the correct approach here as it allows all tasks to complete regardless of individual failures, which is ideal for a build orchestration system where you want to see all errors at once.
34-52: LGTM - Comprehensive error handling and logging.The result processing logic is well-implemented:
- Properly distinguishes between rejected and fulfilled promises
- Extracts detailed error information for Error instances
- Provides fallback handling for non-Error thrown values
- Logs both successes and failures with context
- Tracks error state for final exit code determination
54-59: LGTM - Proper CI integration with exit codes.The exit code handling correctly sets
process.exitCode = 1when errors occur, which will properly signal failure to CI systems while still allowing the process to complete and log all results.
60-64: LGTM - Appropriate top-level error handling.The outer try-catch provides a safety net for unexpected errors during orchestration, with proper logging and error re-throwing to maintain the error state.
66-66: LGTM - Direct main function invocation.The immediate invocation of the main function is appropriate for a build script entry point.
| export async function runBuildAdoptersList() { | ||
| try{ | ||
| await buildAdoptersList(); | ||
| }catch(error){ | ||
| throw error; | ||
| } | ||
| } No newline at end of file |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove the unnecessary try-catch block and fix formatting.
The try-catch block serves no purpose since it only rethrows the original error. Additionally, there's a formatting issue with missing space after try.
-export async function runBuildAdoptersList() {
- try{
- await buildAdoptersList();
- }catch(error){
- throw error;
- }
-}
+export async function runBuildAdoptersList() {
+ await buildAdoptersList();
+}📝 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.
| export async function runBuildAdoptersList() { | |
| try{ | |
| await buildAdoptersList(); | |
| }catch(error){ | |
| throw error; | |
| } | |
| } | |
| export async function runBuildAdoptersList() { | |
| await buildAdoptersList(); | |
| } |
🧰 Tools
🪛 Biome (1.9.4)
[error] 7-7: The catch clause that only rethrows the original error is useless.
An unnecessary catch clause can be confusing.
Unsafe fix: Remove the try/catch clause.
(lint/complexity/noUselessCatch)
🤖 Prompt for AI Agents
In npm/runners/build-adopters-list-runner.ts lines 3 to 9, remove the
unnecessary try-catch block that only rethrows the error without handling it,
and fix the formatting by adding a space after the `try` keyword. Simply call
`await buildAdoptersList()` directly inside the async function without wrapping
it in try-catch.
| export async function runBuildRss() { | ||
| try { | ||
| await rssFeed('blog', 'AsyncAPI Blog', 'Latest news and updates from AsyncAPI.', 'rss.xml'); | ||
| } catch (err) { | ||
| throw err; | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove the unnecessary try-catch block.
The try-catch block serves no purpose since it only rethrows the original error without adding any value.
-export async function runBuildRss() {
- try {
- await rssFeed('blog', 'AsyncAPI Blog', 'Latest news and updates from AsyncAPI.', 'rss.xml');
- } catch (err) {
- throw err;
- }
-}
+export async function runBuildRss() {
+ await rssFeed('blog', 'AsyncAPI Blog', 'Latest news and updates from AsyncAPI.', 'rss.xml');
+}📝 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.
| export async function runBuildRss() { | |
| try { | |
| await rssFeed('blog', 'AsyncAPI Blog', 'Latest news and updates from AsyncAPI.', 'rss.xml'); | |
| } catch (err) { | |
| throw err; | |
| } | |
| } | |
| export async function runBuildRss() { | |
| await rssFeed( | |
| 'blog', | |
| 'AsyncAPI Blog', | |
| 'Latest news and updates from AsyncAPI.', | |
| 'rss.xml' | |
| ); | |
| } |
🧰 Tools
🪛 Biome (1.9.4)
[error] 7-7: The catch clause that only rethrows the original error is useless.
An unnecessary catch clause can be confusing.
Unsafe fix: Remove the try/catch clause.
(lint/complexity/noUselessCatch)
🤖 Prompt for AI Agents
In npm/runners/build-rss-runner.ts around lines 3 to 9, remove the try-catch
block inside the runBuildRss function because it only catches and rethrows the
error without any additional handling. Simply call await rssFeed(...) directly
and let any errors propagate naturally.
| export function runBuildPages() { | ||
| try { | ||
| ensureDirectoryExists(TARGET_DIR); | ||
| copyAndRenameFiles(SRC_DIR, TARGET_DIR); | ||
| } catch (err) { | ||
| throw err; | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove the unnecessary try-catch block.
The try-catch block serves no purpose since it only rethrows the original error without adding any value.
-export function runBuildPages() {
- try {
- ensureDirectoryExists(TARGET_DIR);
- copyAndRenameFiles(SRC_DIR, TARGET_DIR);
- } catch (err) {
- throw err;
- }
-}
+export function runBuildPages() {
+ ensureDirectoryExists(TARGET_DIR);
+ copyAndRenameFiles(SRC_DIR, TARGET_DIR);
+}📝 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.
| export function runBuildPages() { | |
| try { | |
| ensureDirectoryExists(TARGET_DIR); | |
| copyAndRenameFiles(SRC_DIR, TARGET_DIR); | |
| } catch (err) { | |
| throw err; | |
| } | |
| } | |
| export function runBuildPages() { | |
| ensureDirectoryExists(TARGET_DIR); | |
| copyAndRenameFiles(SRC_DIR, TARGET_DIR); | |
| } |
🧰 Tools
🪛 Biome (1.9.4)
[error] 12-12: The catch clause that only rethrows the original error is useless.
An unnecessary catch clause can be confusing.
Unsafe fix: Remove the try/catch clause.
(lint/complexity/noUselessCatch)
🤖 Prompt for AI Agents
In npm/runners/build-pages-runner.ts around lines 7 to 14, remove the try-catch
block from the runBuildPages function because it only catches and immediately
rethrows the error without any additional handling. Simply keep the function
body with the calls to ensureDirectoryExists and copyAndRenameFiles directly,
allowing errors to propagate naturally.
| try { | ||
| await start(resolve(currentDirPath, '..', '..', 'dashboard.json')); | ||
| } | ||
| catch (error) { | ||
| throw error; | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove the useless try-catch block.
The try-catch block only rethrows the original error without adding any value. This makes the code unnecessarily verbose and can be confusing.
Apply this diff to simplify the function:
export async function runBuildDashboard() {
- try {
- await start(resolve(currentDirPath, '..', '..', 'dashboard.json'));
- }
- catch (error) {
- throw error;
- }
+ await start(resolve(currentDirPath, '..', '..', 'dashboard.json'));
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| try { | |
| await start(resolve(currentDirPath, '..', '..', 'dashboard.json')); | |
| } | |
| catch (error) { | |
| throw error; | |
| } | |
| } | |
| export async function runBuildDashboard() { | |
| await start(resolve(currentDirPath, '..', '..', 'dashboard.json')); | |
| } |
🧰 Tools
🪛 Biome (1.9.4)
[error] 14-14: The catch clause that only rethrows the original error is useless.
An unnecessary catch clause can be confusing.
Unsafe fix: Remove the try/catch clause.
(lint/complexity/noUselessCatch)
🤖 Prompt for AI Agents
In npm/runners/build-dashboard-runner.ts around lines 10 to 16, remove the
try-catch block that only rethrows the caught error without any additional
handling. Simply call the start function with await and let any errors propagate
naturally, which simplifies the code and improves readability.
| try { | ||
| const postDirectories = [ | ||
| ['./pages/blog', '/blog'], | ||
| ['./pages/docs', '/docs'], | ||
| ['./pages/about', '/about'] | ||
| ]; | ||
| const basePath = '../../pages'; | ||
| const writeFilePath = resolve(currentDirPath, '../../config', 'posts.json'); | ||
| await buildPostList(postDirectories, basePath, writeFilePath); | ||
| } catch (err) { | ||
| throw err; | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove useless try-catch and fix basePath resolution.
The try-catch block only rethrows without adding value, and the basePath should be properly resolved relative to the current directory.
Apply this diff to improve the function:
export async function runBuildPostList() {
- try {
- const postDirectories = [
- ['./pages/blog', '/blog'],
- ['./pages/docs', '/docs'],
- ['./pages/about', '/about']
- ];
- const basePath = '../../pages';
- const writeFilePath = resolve(currentDirPath, '../../config', 'posts.json');
- await buildPostList(postDirectories, basePath, writeFilePath);
- } catch (err) {
- throw err;
- }
+ const postDirectories = [
+ ['./pages/blog', '/blog'],
+ ['./pages/docs', '/docs'],
+ ['./pages/about', '/about']
+ ];
+ const basePath = resolve(currentDirPath, '../../pages');
+ const writeFilePath = resolve(currentDirPath, '../../config', 'posts.json');
+ await buildPostList(postDirectories, basePath, writeFilePath);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| try { | |
| const postDirectories = [ | |
| ['./pages/blog', '/blog'], | |
| ['./pages/docs', '/docs'], | |
| ['./pages/about', '/about'] | |
| ]; | |
| const basePath = '../../pages'; | |
| const writeFilePath = resolve(currentDirPath, '../../config', 'posts.json'); | |
| await buildPostList(postDirectories, basePath, writeFilePath); | |
| } catch (err) { | |
| throw err; | |
| } | |
| } | |
| export async function runBuildPostList() { | |
| const postDirectories = [ | |
| ['./pages/blog', '/blog'], | |
| ['./pages/docs', '/docs'], | |
| ['./pages/about', '/about'] | |
| ]; | |
| const basePath = resolve(currentDirPath, '../../pages'); | |
| const writeFilePath = resolve(currentDirPath, '../../config', 'posts.json'); | |
| await buildPostList(postDirectories, basePath, writeFilePath); | |
| } |
🧰 Tools
🪛 Biome (1.9.4)
[error] 19-19: The catch clause that only rethrows the original error is useless.
An unnecessary catch clause can be confusing.
Unsafe fix: Remove the try/catch clause.
(lint/complexity/noUselessCatch)
🤖 Prompt for AI Agents
In npm/runners/build-post-list-runner.ts around lines 9 to 21, remove the
try-catch block since it only rethrows the error without handling it. Also, fix
the basePath by resolving it relative to the current directory using
path.resolve or an equivalent method instead of a hardcoded string. This ensures
correct path resolution and cleaner error propagation.
| try { | ||
| const caseStudyDirectory = resolve(projectRoot, 'config', 'casestudies'); | ||
| if (!fs.existsSync(caseStudyDirectory)) { | ||
| console.error(`Directory does not exist: ${caseStudyDirectory}`); | ||
| return; | ||
| } | ||
| const writeFilePath = resolve(projectRoot, 'config', 'case-studies.json'); | ||
| await buildCaseStudiesList(caseStudyDirectory, writeFilePath); | ||
| } catch (error) { | ||
| throw error; | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve error handling consistency.
Multiple issues with the error handling approach:
- The try-catch block only rethrows without adding value
- Uses
console.errorinstead ofloggerfor consistency - Early return without throwing is inconsistent with error handling patterns
Apply this diff to improve the function:
+import { logger } from '@/scripts/helpers/logger';
export async function runCaseStudies() {
- try {
- const caseStudyDirectory = resolve(projectRoot, 'config', 'casestudies');
- if (!fs.existsSync(caseStudyDirectory)) {
- console.error(`Directory does not exist: ${caseStudyDirectory}`);
- return;
- }
- const writeFilePath = resolve(projectRoot, 'config', 'case-studies.json');
- await buildCaseStudiesList(caseStudyDirectory, writeFilePath);
- } catch (error) {
- throw error;
- }
+ const caseStudyDirectory = resolve(projectRoot, 'config', 'casestudies');
+ if (!fs.existsSync(caseStudyDirectory)) {
+ logger.error(`Directory does not exist: ${caseStudyDirectory}`);
+ throw new Error(`Case studies directory not found: ${caseStudyDirectory}`);
+ }
+ const writeFilePath = resolve(projectRoot, 'config', 'case-studies.json');
+ await buildCaseStudiesList(caseStudyDirectory, writeFilePath);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| try { | |
| const caseStudyDirectory = resolve(projectRoot, 'config', 'casestudies'); | |
| if (!fs.existsSync(caseStudyDirectory)) { | |
| console.error(`Directory does not exist: ${caseStudyDirectory}`); | |
| return; | |
| } | |
| const writeFilePath = resolve(projectRoot, 'config', 'case-studies.json'); | |
| await buildCaseStudiesList(caseStudyDirectory, writeFilePath); | |
| } catch (error) { | |
| throw error; | |
| } | |
| } | |
| import { resolve } from 'path'; | |
| import fs from 'fs'; | |
| +import { logger } from '@/scripts/helpers/logger'; | |
| export async function runCaseStudies() { | |
| - try { | |
| - const caseStudyDirectory = resolve(projectRoot, 'config', 'casestudies'); | |
| - if (!fs.existsSync(caseStudyDirectory)) { | |
| - console.error(`Directory does not exist: ${caseStudyDirectory}`); | |
| - return; | |
| - } | |
| - const writeFilePath = resolve(projectRoot, 'config', 'case-studies.json'); | |
| - await buildCaseStudiesList(caseStudyDirectory, writeFilePath); | |
| - } catch (error) { | |
| - throw error; | |
| - } | |
| + const caseStudyDirectory = resolve(projectRoot, 'config', 'casestudies'); | |
| + if (!fs.existsSync(caseStudyDirectory)) { | |
| + logger.error(`Directory does not exist: ${caseStudyDirectory}`); | |
| + throw new Error(`Case studies directory not found: ${caseStudyDirectory}`); | |
| + } | |
| + const writeFilePath = resolve(projectRoot, 'config', 'case-studies.json'); | |
| + await buildCaseStudiesList(caseStudyDirectory, writeFilePath); | |
| } |
🧰 Tools
🪛 Biome (1.9.4)
[error] 20-20: The catch clause that only rethrows the original error is useless.
An unnecessary catch clause can be confusing.
Unsafe fix: Remove the try/catch clause.
(lint/complexity/noUselessCatch)
🤖 Prompt for AI Agents
In npm/runners/case-studies-runner.ts around lines 11 to 22, improve error
handling by removing the redundant try-catch block that only rethrows the error,
replace console.error with the consistent logger.error for error messages, and
instead of early returning when the directory does not exist, throw an
appropriate error to maintain consistent error handling patterns throughout the
function.
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
scripts/build-tools.ts (1)
22-34: Remove useless try-catch block.The try-catch block that only rethrows the original error serves no purpose. Based on the orchestration architecture, error handling should be at the runner level, not in the core function.
-export async function buildTools(automatedToolsPath: string, manualToolsPath: string, toolsPath: string, tagsPath: string) { - try { - const githubExtractData = await getData(); - const automatedTools = await convertTools(githubExtractData); - - await fs.writeFile(automatedToolsPath, JSON.stringify(automatedTools, null, ' ')); - - const manualTools = JSON.parse(await fs.readFile(manualToolsPath, 'utf-8')); - - await combineTools(automatedTools, manualTools, toolsPath, tagsPath); - } catch (err) { - throw err; - } -} +export async function buildTools(automatedToolsPath: string, manualToolsPath: string, toolsPath: string, tagsPath: string) { + const githubExtractData = await getData(); + const automatedTools = await convertTools(githubExtractData); + + await fs.writeFile(automatedToolsPath, JSON.stringify(automatedTools, null, ' ')); + + const manualTools = JSON.parse(await fs.readFile(manualToolsPath, 'utf-8')); + + await combineTools(automatedTools, manualTools, toolsPath, tagsPath); +}
🧹 Nitpick comments (2)
scripts/utils/readAndWriteJson.ts (2)
12-36: Consider renaming the function to better reflect its full functionality.The function name
writeJSONis misleading as it actually reads a file, converts its content to JSON, then writes the result. Consider renaming toconvertFileToJsonorreadConvertWriteJsonfor clarity.The implementation approach with separate try-catch blocks for each operation provides good error isolation and meaningful error messages.
-export async function writeJSON(readPath: string, writePath: string) { +export async function convertFileToJson(readPath: string, writePath: string) {
20-20: Error messages could be more descriptive.Consider enhancing error messages to include the file paths involved for better debugging context.
- throw new Error(`Error while reading file\nError: ${err}`); + throw new Error(`Error while reading file '${readPath}'\nError: ${err}`);- throw new Error(`Error while conversion\nError: ${err}`); + throw new Error(`Error while converting content from '${readPath}' to JSON\nError: ${err}`);- throw new Error(`Error while writing file\nError: ${err}`); + throw new Error(`Error while writing JSON to '${writePath}'\nError: ${err}`);Also applies to: 27-27, 34-34
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
scripts/adopters.ts(1 hunks)scripts/adopters/index.ts(1 hunks)scripts/build-meetings.ts(3 hunks)scripts/build-newsroom-videos.ts(3 hunks)scripts/build-pages.ts(0 hunks)scripts/build-tools.ts(2 hunks)scripts/compose.ts(2 hunks)scripts/dashboard/build-dashboard.ts(2 hunks)scripts/utils/logger.ts(1 hunks)scripts/utils/readAndWriteJson.ts(1 hunks)
💤 Files with no reviewable changes (1)
- scripts/build-pages.ts
✅ Files skipped from review due to trivial changes (1)
- scripts/utils/logger.ts
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: sagarkori143
PR: asyncapi/website#4192
File: npm/runners/build-pages-runner.ts:7-14
Timestamp: 2025-06-19T13:51:27.459Z
Learning: In the AsyncAPI website build system, individual runner functions use try-catch blocks that rethrow errors to maintain consistent error boundaries, while the top-level orchestrator in npm/index.ts provides meaningful error logging with task-specific context using logger.error(`Error building ${taskName}:`, error). This separation of concerns allows for clean error handling at different architectural layers.
🧬 Code Graph Analysis (3)
scripts/utils/readAndWriteJson.ts (1)
scripts/helpers/utils.ts (1)
convertToJson(46-46)
scripts/adopters/index.ts (2)
scripts/adopters.ts (1)
buildAdoptersList(16-24)scripts/helpers/readAndWriteJson.ts (1)
writeJSON(14-40)
scripts/adopters.ts (1)
scripts/utils/readAndWriteJson.ts (1)
writeJSON(12-36)
🪛 Biome (1.9.4)
scripts/adopters/index.ts
[error] 22-22: The catch clause that only rethrows the original error is useless.
An unnecessary catch clause can be confusing.
Unsafe fix: Remove the try/catch clause.
(lint/complexity/noUselessCatch)
scripts/adopters.ts
[error] 22-22: The catch clause that only rethrows the original error is useless.
An unnecessary catch clause can be confusing.
Unsafe fix: Remove the try/catch clause.
(lint/complexity/noUselessCatch)
scripts/build-tools.ts
[error] 32-32: The catch clause that only rethrows the original error is useless.
An unnecessary catch clause can be confusing.
Unsafe fix: Remove the try/catch clause.
(lint/complexity/noUselessCatch)
⏰ Context from checks skipped due to timeout of 180000ms (1)
- GitHub Check: Lighthouse CI
🔇 Additional comments (13)
scripts/build-meetings.ts (2)
5-6: LGTM: Proper dotenv configuration for orchestration.The addition of dotenv configuration at the module level ensures environment variables are loaded when the module is imported, which is correct for the new orchestration pattern.
16-16: LGTM: Function export enables orchestration.Converting
buildMeetingsto an exported function aligns with the refactoring to enable external orchestration while maintaining the original logic.scripts/build-newsroom-videos.ts (2)
7-8: LGTM: Consistent dotenv configuration pattern.The dotenv configuration follows the same pattern as other refactored scripts, ensuring environment variables are properly loaded.
24-24: LGTM: Proper function export for orchestration.The function export maintains the original signature and logic while enabling external orchestration.
scripts/build-tools.ts (1)
21-21: LGTM: Proper function export for orchestration.Converting
buildToolsto an exported function aligns with the refactoring pattern for external orchestration.scripts/dashboard/build-dashboard.ts (2)
18-19: LGTM: Consistent dotenv configuration.The dotenv configuration follows the established pattern across all refactored scripts.
295-295: LGTM: Proper function export with preserved error handling.The
startfunction is correctly exported for orchestration while maintaining its original error handling logic, which is appropriate for this complex function.scripts/compose.ts (2)
15-21: LGTM: Type export enables better modularity.Exporting the
ComposePromptTypealigns well with the architectural refactoring to separate interactive prompting from the core logic.
137-147: Error handling follows established patterns correctly.The error handling approach of logging before rejecting aligns with the architectural pattern established in the build system, where individual functions handle errors locally while maintaining clean error boundaries for the orchestrator.
scripts/adopters/index.ts (2)
21-23: The try-catch rethrow pattern is intentional for error boundary management.The static analysis tool flagged this as a "useless catch," but based on the project's architectural patterns, this rethrow approach maintains consistent error boundaries between individual runner functions and the top-level orchestrator. This allows the orchestrator in
npm/index.tsto provide meaningful error logging with task-specific context.
3-3: Verify the import path consistency.The import references
../helpers/readAndWriteJsonbut the utility file provided is located atscripts/utils/readAndWriteJson.ts. Ensure the import path matches the actual file location.#!/bin/bash # Check if the helper file exists at the expected location fd -t f "readAndWriteJson.ts" scripts/scripts/adopters.ts (2)
21-23: The try-catch rethrow pattern aligns with the project's error handling architecture.Similar to the other adopters file, this rethrow pattern is intentional for maintaining consistent error boundaries in the build system, allowing the orchestrator to provide meaningful error logging.
16-24: Verify if this duplicates functionality from scripts/adopters/index.ts.This function appears very similar to the one in
scripts/adopters/index.ts. Both export abuildAdoptersListfunction with nearly identical implementations. Ensure this duplication is intentional and both files serve different purposes in the build system.#!/bin/bash # Compare the two adopters files to check for duplication echo "=== Content of scripts/adopters/index.ts ===" cat scripts/adopters/index.ts 2>/dev/null || echo "File not found" echo -e "\n=== Content of scripts/adopters.ts ===" cat scripts/adopters.ts 2>/dev/null || echo "File not found" echo -e "\n=== Checking for references to both files ===" rg -n "scripts/adopters" --type ts
| const fileName = answers.title | ||
| .toLowerCase() | ||
| .replace(/[^a-zA-Z0-9 ]/g, '') | ||
| .replace(/ /g, '-') | ||
| .replace(/-+/g, '-'); | ||
| const frontMatter = genFrontMatter(answers); | ||
| const filePath = `pages/blog/${fileName || 'untitled'}.md`; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Strengthen filename sanitization to handle edge cases.
The current sanitization logic has potential edge cases that could cause issues:
- Empty filename risk: If the title contains only special characters,
fileNamecould become an empty string after sanitization - Filename length: No validation for excessively long titles that could exceed filesystem limits
- Path construction: Direct string concatenation for file paths
Consider this more robust approach:
+import path from 'path';
+
// Remove special characters and replace space with -
-const fileName = answers.title
- .toLowerCase()
- .replace(/[^a-zA-Z0-9 ]/g, '')
- .replace(/ /g, '-')
- .replace(/-+/g, '-');
+const sanitizedTitle = answers.title
+ .toLowerCase()
+ .replace(/[^a-zA-Z0-9 ]/g, '')
+ .replace(/ /g, '-')
+ .replace(/-+/g, '-')
+ .slice(0, 100); // Limit filename length
+const fileName = sanitizedTitle || 'untitled';
const frontMatter = genFrontMatter(answers);
-const filePath = `pages/blog/${fileName || 'untitled'}.md`;
+const filePath = path.join('pages', 'blog', `${fileName}.md`);📝 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 fileName = answers.title | |
| .toLowerCase() | |
| .replace(/[^a-zA-Z0-9 ]/g, '') | |
| .replace(/ /g, '-') | |
| .replace(/-+/g, '-'); | |
| const frontMatter = genFrontMatter(answers); | |
| const filePath = `pages/blog/${fileName || 'untitled'}.md`; | |
| import path from 'path'; | |
| const sanitizedTitle = answers.title | |
| .toLowerCase() | |
| .replace(/[^a-zA-Z0-9 ]/g, '') | |
| .replace(/ /g, '-') | |
| .replace(/-+/g, '-') | |
| .slice(0, 100); // Limit filename length | |
| const fileName = sanitizedTitle || 'untitled'; | |
| const frontMatter = genFrontMatter(answers); | |
| const filePath = path.join('pages', 'blog', `${fileName}.d.md`); |
🤖 Prompt for AI Agents
In scripts/compose.ts around lines 129 to 135, improve filename sanitization by
first trimming and sanitizing the title to ensure it is not empty, defaulting to
'untitled' if it is. Then, add a check to truncate the filename to a safe
maximum length to avoid filesystem limits. Finally, replace direct string
concatenation for the file path with a path-joining method from the path module
to ensure cross-platform compatibility and correctness.
2 participants
Description
Related issue(s)
Summary by CodeRabbit
New Features
Improvements
Bug Fixes