fix: simplified error handling in build-dashboard.js #3521
Conversation
sahitya-chandra
requested review from
derberg,
akshatnema,
magicmatatjahu,
anshgoyalevil,
Mayaleeeee and
asyncapi-bot-eve
as code owners
WalkthroughThe changes focus on modifying error handling in the Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command 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
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Welcome to AsyncAPI. Thanks a lot for creating your first pull request. Please check out our contributors guide useful for opening a pull request.
Keep in mind there are also other channels you can use to interact with AsyncAPI community. For more details check out this issue.
✅ Deploy Preview for asyncapi-website ready!Built without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify site configuration. |
|
⚡️ Lighthouse report for the changes in this PR:
Lighthouse ran on https://deploy-preview-3521--asyncapi-website.netlify.app/ |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #3521 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 19 20 +1
Lines 668 673 +5
=========================================
+ Hits 668 673 +5 ☔ View full report in Codecov by Sentry. |
sahitya-chandra
changed the title
|
@sambhavgupta0705 @devilkiller-ag Can you please review this. |
|
@devilkiller-ag Sorry Sir I requested for review again... It happened by mistake. |
|
@sahitya-chandra why are we having these changes? |
@sambhavgupta0705 Sir the code is returning a promise with error instead we can just throw an error |
|
I have just simplified the error handling in build-dashboard.js file as mentioned in #3305 issue |
|
we are using async/await so yes we can use this |
|
/update |
|
@sambhavgupta0705 @devilkiller-ag thanks for the review sir. |
|
@anshgoyalevil can you please take a look at this |
|
This does solve the linked issue, but we are still looking for a better error handling strategy. Can we get #3358 done instead of just getting this |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
scripts/tools/logger.js (1)
1-19: Consider adjusting the formatting to meet Prettier style requirements.According to the static analysis hints, the file should avoid multiline bracket formatting in lines 5-8 and 11-14 and remove the trailing comma on line 16. Implementing these suggestions will keep the codebase consistent.
Here is a potential diff to adjust the formatting:
const logger = createLogger({ level: 'error', // minimum level - format: format.combine( - format.timestamp(), - format.json() - ), + format: format.combine(format.timestamp(), format.json()), transports: [ new transports.Console({ - format: format.combine( - format.colorize(), - format.simple() - ) + format: format.combine(format.colorize(), format.simple()) }) - ], + ] });🧰 Tools
🪛 eslint
[error] 5-8: Replace
⏎····format.timestamp(),⏎····format.json()⏎··withformat.timestamp(),·format.json()(prettier/prettier)
[error] 11-14: Replace
⏎········format.colorize(),⏎········format.simple()⏎······withformat.colorize(),·format.simple()(prettier/prettier)
[error] 16-16: Delete
,(prettier/prettier)
[error] 19-19: Insert
⏎(prettier/prettier)
tests/build-pages.test.js (2)
26-32: Use arrow function parentheses for clarity.For the callback in
[TEST_DIR, NEW_TEST_DIR].forEach(dir => { ... }), consider adding parentheses around the parameter for readability. This aligns with Prettier’s recommended styling.-[TEST_DIR, NEW_TEST_DIR].forEach(dir => { +[TEST_DIR, NEW_TEST_DIR].forEach((dir) => {🧰 Tools
🪛 eslint
[error] 26-27: Replace
⏎····[TEST_DIR,·NEW_TEST_DIR].forEach(dirwith····[TEST_DIR,·NEW_TEST_DIR].forEach((dir)(prettier/prettier)
44-50: Similarly apply arrow function parentheses.We can mirror the same style in the
afterAllcallback for consistency.🧰 Tools
🪛 eslint
[error] 44-45: Replace
⏎····[TEST_DIR,·NEW_TEST_DIR].forEach(dirwith····[TEST_DIR,·NEW_TEST_DIR].forEach((dir)(prettier/prettier)
[error] 50-50: Delete
····(prettier/prettier)
scripts/tools/tools-object.js (1)
11-11: Replace ".js" extension for consistency.The static analysis suggests avoiding explicit file extensions. Using
'./logger'instead of'./logger.js'is an option if the codebase style allows it.-const logger = require("./logger.js"); +const logger = require("./logger");🧰 Tools
🪛 eslint
[error] 11-11: Replace
"./logger.js"with'./logger.js'(prettier/prettier)
[error] 11-11: Unexpected use of file extension "js" for "./logger.js"
(import/extensions)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (4)
package.json(1 hunks)scripts/tools/logger.js(1 hunks)scripts/tools/tools-object.js(2 hunks)tests/build-pages.test.js(3 hunks)
🧰 Additional context used
🪛 eslint
scripts/tools/logger.js
[error] 5-8: Replace ⏎····format.timestamp(),⏎····format.json()⏎·· with format.timestamp(),·format.json()
(prettier/prettier)
[error] 11-14: Replace ⏎········format.colorize(),⏎········format.simple()⏎······ with format.colorize(),·format.simple()
(prettier/prettier)
[error] 16-16: Delete ,
(prettier/prettier)
[error] 19-19: Insert ⏎
(prettier/prettier)
scripts/tools/tools-object.js
[error] 11-11: Replace "./logger.js" with './logger.js'
(prettier/prettier)
[error] 11-11: Unexpected use of file extension "js" for "./logger.js"
(import/extensions)
[error] 111-111: Insert );⏎
(prettier/prettier)
[error] 117-117: Delete ,
(prettier/prettier)
tests/build-pages.test.js
[error] 26-27: Replace ⏎····[TEST_DIR,·NEW_TEST_DIR].forEach(dir with ····[TEST_DIR,·NEW_TEST_DIR].forEach((dir)
(prettier/prettier)
[error] 44-45: Replace ⏎····[TEST_DIR,·NEW_TEST_DIR].forEach(dir with ····[TEST_DIR,·NEW_TEST_DIR].forEach((dir)
(prettier/prettier)
[error] 50-50: Delete ····
(prettier/prettier)
[error] 65-66: Delete ⏎
(prettier/prettier)
🔇 Additional comments (4)
tests/build-pages.test.js (2)
23-23: New constant introduced for directory handling.
Defining NEW_TEST_DIR segregates it from TEST_DIR, improving clarity in test directory setup. This looks good.
65-68: Ensure directory is always cleaned up.
This test effectively verifies ensureDirectoryExists. The explicit cleanup of NEW_TEST_DIR is a good safety check. No issues here.
🧰 Tools
🪛 eslint
[error] 65-66: Delete ⏎
(prettier/prettier)
scripts/tools/tools-object.js (1)
114-118: Structured logging is well-implemented.
Logging the message, stack, and phase helps debugging. This integration with Winston is properly done. Great job.
🧰 Tools
🪛 eslint
[error] 117-117: Delete ,
(prettier/prettier)
package.json (1)
118-118: Dependency addition recognized.
The winston library is widely used for logging. Including it here makes sense to support structured logging as implemented in scripts/tools/logger.js.
|
@anshgoyalevil Please have a look sir... |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
scripts/tools/logger.js (1)
3-11: Consider enhancing logger configuration for better observabilityThe Winston logger setup implements good practices with structured logging (JSON format + timestamp). However, consider these enhancements:
- The 'error' level might miss important warnings. Consider using 'info' as the base level and filtering at the transport level.
- Add file transport for persistent logging in production.
const logger = createLogger({ - level: 'error', // minimum level of messages logger will handle + level: 'info', // capture all important logs format: format.combine(format.timestamp(), format.json()), transports: [ new transports.Console({ + level: 'error', // filter console output to errors format: format.combine(format.colorize(), format.simple()) - }) + }), + new transports.File({ + filename: 'error.log', + level: 'error', + format: format.combine( + format.timestamp(), + format.json() + ) + }) ] });scripts/tools/tools-object.js (2)
11-11: Fix quote consistencyUse single quotes consistently across the file.
-const logger = require("./logger"); +const logger = require('./logger');🧰 Tools
🪛 eslint
[error] 11-11: Replace
"./logger"with'./logger'(prettier/prettier)
Line range hint
89-91: Standardize error handling approachThe nested try-catch blocks have inconsistent error handling approaches. The inner catch throws directly while the outer catch logs and wraps. Consider standardizing the approach.
} catch (err) { - console.error(err) - throw err; + logger.error('Error processing individual tool', { + message: err.message, + stack: err.stack, + phase: 'individual_tool_processing', + }); + throw new Error(`Error processing individual tool: ${err.message}`); } })) return finalToolsObject; } catch (err) { logger.error('Error processing tool', { message: err.message, stack: err.stack, phase: 'tool_processing', }); throw new Error(`Error processing tool: ${err.message}`) }Also applies to: 111-120
🧰 Tools
🪛 eslint
[error] 8-8: Replace
"uri"])with'uri']);(prettier/prettier)
[error] 9-9: Insert
;(prettier/prettier)
[error] 11-11: Replace
"./logger"with'./logger'(prettier/prettier)
tests/build-pages.test.js (1)
27-31: Optimize test cleanup codeThe directory cleanup logic is duplicated in beforeAll and afterAll. Consider extracting it to a helper function.
+const cleanupTestDirs = () => { + [TEST_DIR, NEW_TEST_DIR].forEach((dir) => { + if (fs.existsSync(dir)) { + fs.rmSync(dir, { recursive: true, force: true }); + } + }); +}; beforeAll(() => { - [TEST_DIR, NEW_TEST_DIR].forEach((dir) => { - if (fs.existsSync(dir)) { - fs.rmSync(dir, { recursive: true, force: true }); - } - }); + cleanupTestDirs(); // ... rest of setup }); afterAll(() => { - [TEST_DIR, NEW_TEST_DIR].forEach(dir => { - if (fs.existsSync(dir)) { - fs.rmSync(dir, { recursive: true, force: true }); - } - }); + cleanupTestDirs(); });Also applies to: 45-49
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
scripts/tools/logger.js(1 hunks)scripts/tools/tools-object.js(2 hunks)tests/build-pages.test.js(3 hunks)
🧰 Additional context used
🪛 eslint
scripts/tools/logger.js
[error] 13-13: Insert ⏎
(prettier/prettier)
scripts/tools/tools-object.js
[error] 11-11: Replace "./logger" with './logger'
(prettier/prettier)
[error] 111-111: Insert );⏎
(prettier/prettier)
[error] 117-117: Delete ,
(prettier/prettier)
tests/build-pages.test.js
[error] 26-27: Delete ⏎
(prettier/prettier)
[error] 44-45: Replace ⏎····[TEST_DIR,·NEW_TEST_DIR].forEach(dir with ····[TEST_DIR,·NEW_TEST_DIR].forEach((dir)
(prettier/prettier)
[error] 50-50: Delete ····
(prettier/prettier)
[error] 65-66: Delete ⏎
(prettier/prettier)
🔇 Additional comments (1)
scripts/tools/tools-object.js (1)
114-118: Good structured logging implementation
The error logging implementation provides good context with message, stack trace, and processing phase.
🧰 Tools
🪛 eslint
[error] 117-117: Delete ,
(prettier/prettier)
| if (fs.existsSync(NEW_TEST_DIR)) { | ||
| fs.rmSync(NEW_TEST_DIR, { recursive: true, force: true }); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Handle potential race condition in directory check
The existence check and directory creation are not atomic operations. Consider using mkdir with the recursive option to handle this more safely.
- if (fs.existsSync(NEW_TEST_DIR)) {
- fs.rmSync(NEW_TEST_DIR, { recursive: true, force: true });
- }
+ try {
+ fs.mkdirSync(NEW_TEST_DIR, { recursive: true });
+ } catch (err) {
+ if (err.code !== 'EEXIST') {
+ throw err;
+ }
+ fs.rmSync(NEW_TEST_DIR, { recursive: true, force: true });
+ fs.mkdirSync(NEW_TEST_DIR, { recursive: true });
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (fs.existsSync(NEW_TEST_DIR)) { | |
| fs.rmSync(NEW_TEST_DIR, { recursive: true, force: true }); | |
| } | |
| try { | |
| fs.mkdirSync(NEW_TEST_DIR, { recursive: true }); | |
| } catch (err) { | |
| if (err.code !== 'EEXIST') { | |
| throw err; | |
| } | |
| fs.rmSync(NEW_TEST_DIR, { recursive: true, force: true }); | |
| fs.mkdirSync(NEW_TEST_DIR, { recursive: true }); | |
| } |
There was a problem hiding this comment.
@sahitya-chandra, this might indicate that the test was closely tied to the original directory setup/cleanup sequence and is now encountering a mismatch in expectations after your refactor. If you want to keep the new approach (handling potential race conditions in mkdirSync), you can update the failing test so it aligns with the new logic—specifically around when the directory is expected to exist or be removed. If you’re pressed for time or the changes aren’t critical, you can revert the refactor to restore the test to a passing state. However, I recommend adjusting the test to handle the directory creation behavior consistently rather than reverting, because the improved approach makes your code more robust.
If you need more insight into the test failure, you could log or debug the point where it fails to confirm whether the directory removal or creation order is at play, or whether a race condition is causing a mismatch in the test’s timing. Once you confirm what the test is expecting, you can modify it accordingly to accommodate the new behavior.
There was a problem hiding this comment.
the code suggestions that you gave above leads to failing the test.
what should I do ?? should I stick with my changes.
There was a problem hiding this comment.
@sahitya-chandra, it’s generally a good idea to keep the more robust directory-handling approach if possible. However, the test might be failing because it anticipates a very specific sequence of directory cleanup and creation. If you want to preserve the improved approach, you can update the test so that it expects directories to be created (or removed) in a non-racy way. For instance, ensure that the test logic accounts for the possibility that the directory may already exist when mkdirSync is called.
If you need to keep the test unchanged and the timing/race concerns are minimal in this context, then reverting to your previous simpler approach (checking if the directory exists and removing it if so) is also an option. It really depends on your priorities: a robust approach versus minimal test modifications. If the directory creation logic is important enough to risk a non-trivial refactor of the test, I recommend updating the test. Otherwise, revert the changes to restore the passing state.
Description
Simplified the error handling in the build-dashboard.js file by replacing
return Promise.reject(e)tothrow eas suggested in PR fix: fixed dashboard scripts #3304added custom logservice using winston for error logging as mentioned in PR feat: add tests for tool-object script #3265
error logging img
build-pages.test.jsfileRelated issue(s)
resolved #3305 #3358
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Bug Fixes
Tests