fix: the bug in the Netlify function response structure #4694
Conversation
Devnil434
requested review from
Mayaleeeee,
akshatnema,
anshgoyalevil,
asyncapi-bot-eve,
derberg,
princerajpoot20 and
sambhavgupta0705
as code owners
WalkthroughThe changes enable Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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. Comment |
asyncapi-bot
requested review from
CBID2,
bandantonio,
devilkiller-ag and
vishvamsinh28
✅ Deploy Preview for asyncapi-website ready!Built without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify project configuration. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #4694 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 22 22
Lines 799 801 +2
Branches 146 148 +2
=========================================
+ Hits 799 801 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Devnil434
changed the title
✅ Deploy Preview for asyncapi-website ready!Built without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
⚡️ Lighthouse report for the changes in this PR:
Lighthouse ran on https://deploy-preview-4694--asyncapi-website.netlify.app/ |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
netlify/functions/github_discussions.ts (1)
56-61: Error response now uses correct Netlify shape; consider aligning with PR intent and hardening error typingThe main bug (missing
bodyon the error response) is fixed: the handler now returns a proper Netlify Function response with a JSON‑stringified body, which is good.Two follow‑ups worth considering:
Align with the PR’s described contract
The PR description says the function should consistently return a body like:{ success: false, message: error.message }but the implementation currently returns only:
{ message: error.response.data.message }If any client code relies on a
successflag (or onerror.messagerather thanerror.response.data.message), it will still break. Consider updating the body payload to includesuccess: falseas intended.Be defensive around the error shape
const error: ErrorResponse = err;is a type assertion only; at runtimeerrmay not have aresponsefield (e.g. non‑HTTP or unexpected errors). In that caseerror.responsewill beundefinedand this block will throw, masking the original error. A small defensive tweak here would make the function more robust.For example, you could do something along these lines:
- } catch (err) { - const error: ErrorResponse = err; - - return { - statusCode: error.response.status, - body: JSON.stringify({ - message: error.response.data.message - }) - }; - } + } catch (err) { + const error = err as Partial<ErrorResponse>; + const statusCode = error.response?.status ?? 500; + const message = + error.response?.data?.message ?? + 'Something went wrong while creating the discussion.'; + + return { + statusCode, + body: JSON.stringify({ + success: false, + message + }) + }; + }This keeps the new, correct Netlify response structure while better matching the stated response contract and avoiding crashes when
errdoesn’t matchErrorResponse.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
netlify/functions/github_discussions.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 180000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Redirect rules - asyncapi-website
- GitHub Check: Header rules - asyncapi-website
- GitHub Check: Pages changed - asyncapi-website
…hub.com/Devnil434/website into fix-Netlify-function-response-structure
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/index.test.ts (1)
38-46: Relaxed spy assertion is sensible; consider using Jest’s matcher for readabilityChanging the expectation from an exact call count to
toBeGreaterThanOrEqual(1)makes the test less brittle while still ensuringstart()inspects the finance directory before throwing, which aligns with the updated behavior.You could make this a bit clearer/idiomatic by using Jest’s matcher instead of reaching into
mock.calls:- expect(readdirSyncSpy.mock.calls.length).toBeGreaterThanOrEqual(1); + expect(readdirSyncSpy).toHaveBeenCalled();Semantics stay the same (at-least-once), but the intent is more obvious to readers.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/index.test.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (9)
📓 Common learnings
Learnt from: anshgoyalevil
Repo: asyncapi/website PR: 3557
File: tests/fixtures/markdown/check-edit-links-data.js:3-11
Timestamp: 2025-01-19T04:51:41.255Z
Learning: In the AsyncAPI website repository, the test data in `tests/fixtures/markdown/check-edit-links-data.js` intentionally includes inconsistent paths (with and without 'docs' prefix) to verify the script's ability to normalize and handle ambiguous path structures.
Learnt from: anshgoyalevil
Repo: asyncapi/website PR: 3950
File: scripts/utils/check-locales.ts:122-129
Timestamp: 2025-04-20T16:05:16.482Z
Learning: In the AsyncAPI website project, Next.js throws errors at runtime when locale files are missing, making additional validation for missing files unnecessary in the check-locales script.
Learnt from: vishvamsinh28
Repo: asyncapi/website PR: 3378
File: tests/markdown/check-markdown.test.js:133-138
Timestamp: 2024-11-29T17:36:09.783Z
Learning: When testing the 'main' function in 'check-markdown.test.js', it's acceptable to simply ensure it doesn't throw any errors, as the functions it calls are already tested elsewhere.
Learnt from: JeelRajodiya
Repo: asyncapi/website PR: 3423
File: jest.config.cjs:6-7
Timestamp: 2024-12-30T11:00:42.064Z
Learning: The user only wants coverage for scripts, not for .tsx files, because the existing tests are focused on scripts.
Learnt from: akshatnema
Repo: asyncapi/website PR: 3423
File: tests/index.test.js:2-7
Timestamp: 2025-01-18T08:44:43.614Z
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
Repo: asyncapi/website PR: 3423
File: tests/index.test.js:2-7
Timestamp: 2025-01-18T08:44:43.614Z
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.
Learnt from: vishvamsinh28
Repo: asyncapi/website PR: 3284
File: scripts/build-post-list.js:28-46
Timestamp: 2024-11-10T18:16:35.551Z
Learning: In the `scripts/build-post-list.js` JavaScript file, tests rely on the `result` object being a global variable. Moving it inside the `buildPostList` function causes tests to fail; therefore, keep `result` as a global variable.
📚 Learning: 2024-11-01T09:55:20.531Z
Learnt from: akshatnema
Repo: asyncapi/website PR: 3101
File: tests/build-rss.test.js:25-27
Timestamp: 2024-11-01T09:55:20.531Z
Learning: In `tests/build-rss.test.js`, replacing `jest.resetModules()` with `jest.resetAllMocks()` in the `afterEach()` block causes errors. It is necessary to use `jest.resetModules()` to reset the module registry between tests in this file.
Applied to files:
tests/index.test.ts
📚 Learning: 2024-11-29T17:36:09.783Z
Learnt from: vishvamsinh28
Repo: asyncapi/website PR: 3378
File: tests/markdown/check-markdown.test.js:133-138
Timestamp: 2024-11-29T17:36:09.783Z
Learning: When testing the 'main' function in 'check-markdown.test.js', it's acceptable to simply ensure it doesn't throw any errors, as the functions it calls are already tested elsewhere.
Applied to files:
tests/index.test.ts
📚 Learning: 2025-02-16T12:57:24.575Z
Learnt from: anshgoyalevil
Repo: asyncapi/website PR: 0
File: :0-0
Timestamp: 2025-02-16T12:57:24.575Z
Learning: Tests failing after dependency upgrades indicate potential breaking changes and might require code changes rather than test modifications to support newer versions.
Applied to files:
tests/index.test.ts
📚 Learning: 2024-11-10T18:16:22.273Z
Learnt from: vishvamsinh28
Repo: asyncapi/website PR: 3284
File: scripts/build-post-list.js:49-53
Timestamp: 2024-11-10T18:16:22.273Z
Learning: In `scripts/build-post-list.js`, the `walkDirectories` function depends on sequential execution because it mutates `resultObj`. Parallelizing the loop with `Promise.all` causes tests to fail.
Applied to files:
tests/index.test.ts
📚 Learning: 2024-11-01T13:32:15.472Z
Learnt from: akshatnema
Repo: asyncapi/website PR: 3101
File: tests/fixtures/rssData.js:1-57
Timestamp: 2024-11-01T13:32:15.472Z
Learning: In the `tests/fixtures/rssData.js` file of the `asyncapi/website` project, tests for edge cases such as empty strings for title or excerpt, very long text content, international characters (UTF-8), or malformed URLs in `slug` or `cover` are not necessary because these cases will not occur.
Applied to files:
tests/index.test.ts
📚 Learning: 2025-01-18T08:44:43.614Z
Learnt from: akshatnema
Repo: asyncapi/website PR: 3423
File: tests/index.test.js:2-7
Timestamp: 2025-01-18T08:44:43.614Z
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.
Applied to files:
tests/index.test.ts
📚 Learning: 2024-11-01T09:35:23.912Z
Learnt from: akshatnema
Repo: asyncapi/website PR: 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.
Applied to files:
tests/index.test.ts
📚 Learning: 2025-01-18T08:44:43.614Z
Learnt from: akshatnema
Repo: asyncapi/website PR: 3423
File: tests/index.test.js:2-7
Timestamp: 2025-01-18T08:44:43.614Z
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.
Applied to files:
tests/index.test.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
tests/index.test.ts (2)
16-36: Test name vs. assertions (order is not actually validated)The test
"should call all functions in the correct order"currently only asserts that the functions are called (plus the exactrssFeedarguments), but not their relative order.If call order is important, consider asserting it explicitly (e.g. via
mock.invocationCallOrderor orderedtoHaveBeenNthCalledWithexpectations). Otherwise, you might want to relax the test name to avoid implying an ordering guarantee that isn’t enforced.
38-49: Update the comment to match current start() semanticsThe comment says
start()is called when the module is imported, but with the new guard inscripts/index.ts,start()only runs when explicitly invoked or when the file is executed directly. In this test you’re explicitly callingstart():await expect(start()).rejects.toThrow('No finance data found in the finance directory.');To avoid confusion for future readers, consider rephrasing the comment to reflect that you’re testing the explicit call to
start()with an empty finance directory, not an import‑time side effect.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
config/all-tags.json(2 hunks)scripts/index.ts(1 hunks)tests/index.test.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (13)
📓 Common learnings
Learnt from: anshgoyalevil
Repo: asyncapi/website PR: 3950
File: scripts/utils/check-locales.ts:122-129
Timestamp: 2025-04-20T16:05:16.482Z
Learning: In the AsyncAPI website project, Next.js throws errors at runtime when locale files are missing, making additional validation for missing files unnecessary in the check-locales script.
📚 Learning: 2024-11-01T09:35:23.912Z
Learnt from: akshatnema
Repo: asyncapi/website PR: 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.
Applied to files:
scripts/index.tstests/index.test.ts
📚 Learning: 2025-06-19T13:51:27.459Z
Learnt from: sagarkori143
Repo: asyncapi/website PR: 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.
Applied to files:
scripts/index.ts
📚 Learning: 2024-11-10T18:16:22.273Z
Learnt from: vishvamsinh28
Repo: asyncapi/website PR: 3284
File: scripts/build-post-list.js:49-53
Timestamp: 2024-11-10T18:16:22.273Z
Learning: In `scripts/build-post-list.js`, the `walkDirectories` function depends on sequential execution because it mutates `resultObj`. Parallelizing the loop with `Promise.all` causes tests to fail.
Applied to files:
scripts/index.tstests/index.test.ts
📚 Learning: 2025-06-19T13:49:29.796Z
Learnt from: sagarkori143
Repo: asyncapi/website PR: 4192
File: npm/runners/build-newsroom-videos-runner.ts:8-15
Timestamp: 2025-06-19T13:49:29.796Z
Learning: In the AsyncAPI website modularization project, error handling is centralized in the top-level orchestrator function (npm/index.ts) with comprehensive logging and context. Individual runner functions in npm/runners/ are kept simple and let errors propagate naturally to the centralized handler, avoiding redundant try-catch blocks that only rethrow errors.
Applied to files:
scripts/index.ts
📚 Learning: 2025-01-18T08:44:43.614Z
Learnt from: akshatnema
Repo: asyncapi/website PR: 3423
File: tests/index.test.js:2-7
Timestamp: 2025-01-18T08:44:43.614Z
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.
Applied to files:
tests/index.test.ts
📚 Learning: 2025-01-18T08:44:43.614Z
Learnt from: akshatnema
Repo: asyncapi/website PR: 3423
File: tests/index.test.js:2-7
Timestamp: 2025-01-18T08:44:43.614Z
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.
Applied to files:
tests/index.test.ts
📚 Learning: 2025-01-18T08:44:43.614Z
Learnt from: akshatnema
Repo: asyncapi/website PR: 3423
File: tests/index.test.js:2-7
Timestamp: 2025-01-18T08:44:43.614Z
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.
Applied to files:
tests/index.test.ts
📚 Learning: 2025-01-03T08:14:02.138Z
Learnt from: JeelRajodiya
Repo: asyncapi/website PR: 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.
Applied to files:
tests/index.test.ts
📚 Learning: 2024-11-29T17:36:09.783Z
Learnt from: vishvamsinh28
Repo: asyncapi/website PR: 3378
File: tests/markdown/check-markdown.test.js:133-138
Timestamp: 2024-11-29T17:36:09.783Z
Learning: When testing the 'main' function in 'check-markdown.test.js', it's acceptable to simply ensure it doesn't throw any errors, as the functions it calls are already tested elsewhere.
Applied to files:
tests/index.test.ts
📚 Learning: 2024-11-01T09:55:20.531Z
Learnt from: akshatnema
Repo: asyncapi/website PR: 3101
File: tests/build-rss.test.js:25-27
Timestamp: 2024-11-01T09:55:20.531Z
Learning: In `tests/build-rss.test.js`, replacing `jest.resetModules()` with `jest.resetAllMocks()` in the `afterEach()` block causes errors. It is necessary to use `jest.resetModules()` to reset the module registry between tests in this file.
Applied to files:
tests/index.test.ts
📚 Learning: 2025-02-16T12:57:24.575Z
Learnt from: anshgoyalevil
Repo: asyncapi/website PR: 0
File: :0-0
Timestamp: 2025-02-16T12:57:24.575Z
Learning: Tests failing after dependency upgrades indicate potential breaking changes and might require code changes rather than test modifications to support newer versions.
Applied to files:
tests/index.test.ts
📚 Learning: 2024-11-01T13:32:15.472Z
Learnt from: akshatnema
Repo: asyncapi/website PR: 3101
File: tests/fixtures/rssData.js:1-57
Timestamp: 2024-11-01T13:32:15.472Z
Learning: In the `tests/fixtures/rssData.js` file of the `asyncapi/website` project, tests for edge cases such as empty strings for title or excerpt, very long text content, international characters (UTF-8), or malformed URLs in `slug` or `cover` are not necessary because these cases will not occur.
Applied to files:
tests/index.test.ts
🧬 Code graph analysis (1)
scripts/index.ts (7)
scripts/dashboard/build-dashboard.ts (1)
start(328-328)scripts/build-post-list.ts (1)
buildPostList(288-314)scripts/build-rss.ts (1)
rssFeed(35-132)scripts/casestudies/index.ts (1)
buildCaseStudiesList(18-39)scripts/build-tools.ts (1)
buildToolsManual(110-110)scripts/usecases/index.ts (1)
buildUsecasesList(18-20)scripts/finance/index.ts (1)
buildFinanceInfoList(28-58)
⏰ Context from checks skipped due to timeout of 180000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Test NodeJS PR - windows-latest
🔇 Additional comments (2)
scripts/index.ts (1)
48-75: Finance year discovery and error handling look solidThe new logic that filters
config/financeentries down to numeric directories, sorts them descending, and throws a clear error when none are found is coherent and matches the expectations intests/index.test.ts. I don't see functional issues here; the behavior (including the explicit error message) looks intentional and well covered by tests.config/all-tags.json (1)
83-87: New PHP / Laravel / Symfony tags are consistent with existing schemaThe added
PHPlanguage entry and thePHP,Laravel, andSymfonytechnology entries follow the existing shape (name,color,borderColor) and fit visually with neighboring items. I don’t see any structural or data issues here.Also applies to: 185-199
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
scripts/index.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: anshgoyalevil
Repo: asyncapi/website PR: 3950
File: scripts/utils/check-locales.ts:122-129
Timestamp: 2025-04-20T16:05:16.482Z
Learning: In the AsyncAPI website project, Next.js throws errors at runtime when locale files are missing, making additional validation for missing files unnecessary in the check-locales script.
Learnt from: akshatnema
Repo: asyncapi/website PR: 3423
File: tests/index.test.js:2-7
Timestamp: 2025-01-18T08:44:43.614Z
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.
📚 Learning: 2024-11-01T09:35:23.912Z
Learnt from: akshatnema
Repo: asyncapi/website PR: 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.
Applied to files:
scripts/index.ts
📚 Learning: 2024-11-10T18:16:22.273Z
Learnt from: vishvamsinh28
Repo: asyncapi/website PR: 3284
File: scripts/build-post-list.js:49-53
Timestamp: 2024-11-10T18:16:22.273Z
Learning: In `scripts/build-post-list.js`, the `walkDirectories` function depends on sequential execution because it mutates `resultObj`. Parallelizing the loop with `Promise.all` causes tests to fail.
Applied to files:
scripts/index.ts
📚 Learning: 2025-06-19T13:51:27.459Z
Learnt from: sagarkori143
Repo: asyncapi/website PR: 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.
Applied to files:
scripts/index.ts
🧬 Code graph analysis (1)
scripts/index.ts (5)
scripts/build-post-list.ts (1)
buildPostList(288-314)scripts/build-rss.ts (1)
rssFeed(35-132)scripts/casestudies/index.ts (1)
buildCaseStudiesList(18-39)scripts/usecases/index.ts (1)
buildUsecasesList(18-20)scripts/finance/index.ts (1)
buildFinanceInfoList(28-58)
⏰ Context from checks skipped due to timeout of 180000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Test NodeJS PR - windows-latest
🔇 Additional comments (4)
scripts/index.ts (4)
1-10: LGTM!The imports are properly structured and all are necessary for the file's functionality.
12-17: LGTM!The path constants are properly initialized and will support the direct-execution guard correctly.
78-81: LGTM! Direct-execution guard is robust and cross-platform.The guard properly addresses the previous concern about URL comparison brittleness by:
- Checking for undefined
process.argv[1]- Using
resolve()to normalize paths before comparison- Comparing filesystem paths instead of URLs
This implementation works correctly across platforms and with various TypeScript runners.
48-76: Clarify the finance directory structure assumption in the filtering logic.The finance year detection now filters for directories with numeric names using
fs.statSync(itemPath).isDirectory(). This assumes finance data is organized in year-named subdirectories (e.g.,config/finance/2023/,config/finance/2024/) rather than as individual year-named files. Confirm this matches the actual directory layout to ensure the build won't break when no year directories are found.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
scripts/index.ts (1)
20-30: Update JSDoc to reflect directory filtering.Line 26 still states "filters and sorts numeric filenames representing years", but the implementation now filters directories with numeric names, not files.
🧹 Nitpick comments (1)
scripts/index.ts (1)
48-76: Consider using async file operations for consistency.The finance data discovery uses synchronous file operations (
readdirSync,statSync) while the rest of the function uses async/await patterns. While acceptable in a build script, usingreaddirandstatfromfs/promiseswould maintain consistency throughout the function.Apply this diff to use async operations:
+import { readdir, stat } from 'fs/promises'; + export async function start() { // ... existing code ... const financeDir = resolve('.', 'config', 'finance'); // loop through all the directories in finance directory and find the latest year to build the finance info list - const yearsList = fs - .readdirSync(financeDir) - // filter out any items that are not directories with numeric names - .filter((item) => { - const itemPath = resolve(financeDir, item); - return fs.statSync(itemPath).isDirectory() && !Number.isNaN(parseFloat(item)); - }) - // sort the years in descending order - .sort((a, b) => { - return parseFloat(b) - parseFloat(a); - }); + const items = await readdir(financeDir); + const yearsList = ( + await Promise.all( + items.map(async (item) => { + const itemPath = resolve(financeDir, item); + const stats = await stat(itemPath); + return stats.isDirectory() && !Number.isNaN(parseFloat(item)) ? item : null; + }) + ) + ) + .filter((item): item is string => item !== null) + .sort((a, b) => parseFloat(b) - parseFloat(a)); if (yearsList.length === 0) {
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
scripts/index.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: anshgoyalevil
Repo: asyncapi/website PR: 3950
File: scripts/utils/check-locales.ts:122-129
Timestamp: 2025-04-20T16:05:16.482Z
Learning: In the AsyncAPI website project, Next.js throws errors at runtime when locale files are missing, making additional validation for missing files unnecessary in the check-locales script.
Learnt from: akshatnema
Repo: asyncapi/website PR: 3423
File: tests/index.test.js:2-7
Timestamp: 2025-01-18T08:44:43.614Z
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.
📚 Learning: 2024-11-10T18:16:22.273Z
Learnt from: vishvamsinh28
Repo: asyncapi/website PR: 3284
File: scripts/build-post-list.js:49-53
Timestamp: 2024-11-10T18:16:22.273Z
Learning: In `scripts/build-post-list.js`, the `walkDirectories` function depends on sequential execution because it mutates `resultObj`. Parallelizing the loop with `Promise.all` causes tests to fail.
Applied to files:
scripts/index.ts
📚 Learning: 2024-11-01T09:35:23.912Z
Learnt from: akshatnema
Repo: asyncapi/website PR: 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.
Applied to files:
scripts/index.ts
📚 Learning: 2025-06-19T13:51:27.459Z
Learnt from: sagarkori143
Repo: asyncapi/website PR: 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.
Applied to files:
scripts/index.ts
🧬 Code graph analysis (1)
scripts/index.ts (7)
scripts/dashboard/build-dashboard.ts (1)
start(328-328)scripts/build-post-list.ts (1)
buildPostList(288-314)scripts/build-rss.ts (1)
rssFeed(35-132)scripts/casestudies/index.ts (1)
buildCaseStudiesList(18-39)scripts/build-tools.ts (1)
buildToolsManual(110-110)scripts/usecases/index.ts (1)
buildUsecasesList(18-20)scripts/finance/index.ts (1)
buildFinanceInfoList(28-58)
⏰ Context from checks skipped due to timeout of 180000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Test NodeJS PR - windows-latest
🔇 Additional comments (4)
scripts/index.ts (4)
1-18: LGTM!The imports and path setup follow Node.js ESM best practices correctly.
31-31: LGTM!Exporting the
startfunction enables testing and reusability, consistent with patterns used in other build scripts.
32-47: LGTM!The build orchestration correctly sequences async operations and follows the project's error-handling pattern where individual build functions throw errors that propagate to the top-level caller.
78-81: LGTM!The direct execution guard correctly compares filesystem paths, ensuring cross-platform compatibility. This addresses the previous review concern about Windows support.
Devnil434
force-pushed
the
fix-Netlify-function-response-structure
branch
from
6890dbe to
3c39501
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
scripts/index.ts (1)
78-81: Direct-exec guard: normalizeargv[1]before comparing. Even with the improvedcurrentFilePathapproach,process.argv[1]can be relative or differently normalized thanfileURLToPath(import.meta.url).+import { dirname, resolve } from 'path'; +// ... if (process.argv[1] === currentFilePath) { start().catch(console.error); } + +// Consider: +// const argvPath = process.argv[1] ? resolve(process.argv[1]) : ''; +// if (argvPath === currentFilePath) { ... }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
scripts/index.ts(1 hunks)tests/index.test.ts(1 hunks)tests/scripts/index-direct-execution.test.ts(1 hunks)tsconfig.json(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/index.test.ts
🧰 Additional context used
🧠 Learnings (11)
📓 Common learnings
Learnt from: akshatnema
Repo: asyncapi/website PR: 3423
File: tests/index.test.js:2-7
Timestamp: 2025-01-18T08:44:43.614Z
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
Repo: asyncapi/website PR: 3423
File: tests/index.test.js:2-7
Timestamp: 2025-01-18T08:44:43.614Z
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.
Learnt from: akshatnema
Repo: asyncapi/website PR: 3423
File: tests/index.test.js:2-7
Timestamp: 2025-01-18T08:44:43.614Z
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: JeelRajodiya
Repo: asyncapi/website PR: 3423
File: jest.config.cjs:6-7
Timestamp: 2024-12-30T11:00:42.064Z
Learning: The user only wants coverage for scripts, not for .tsx files, because the existing tests are focused on scripts.
Learnt from: anshgoyalevil
Repo: asyncapi/website PR: 3950
File: scripts/utils/check-locales.ts:122-129
Timestamp: 2025-04-20T16:05:16.482Z
Learning: In the AsyncAPI website project, Next.js throws errors at runtime when locale files are missing, making additional validation for missing files unnecessary in the check-locales script.
Learnt from: anshgoyalevil
Repo: asyncapi/website PR: 3301
File: scripts/markdown/check-markdown.js:0-0
Timestamp: 2024-10-18T17:24:45.053Z
Learning: In `scripts/markdown/check-markdown.js`, the script is not run in an asynchronous environment, so refactoring it to use `async/await` is unnecessary.
📚 Learning: 2025-01-18T08:44:43.614Z
Learnt from: akshatnema
Repo: asyncapi/website PR: 3423
File: tests/index.test.js:2-7
Timestamp: 2025-01-18T08:44:43.614Z
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.
Applied to files:
tests/scripts/index-direct-execution.test.tstsconfig.json
📚 Learning: 2025-01-18T08:44:43.614Z
Learnt from: akshatnema
Repo: asyncapi/website PR: 3423
File: tests/index.test.js:2-7
Timestamp: 2025-01-18T08:44:43.614Z
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.
Applied to files:
tests/scripts/index-direct-execution.test.ts
📚 Learning: 2024-12-30T11:00:42.064Z
Learnt from: JeelRajodiya
Repo: asyncapi/website PR: 3423
File: jest.config.cjs:6-7
Timestamp: 2024-12-30T11:00:42.064Z
Learning: The user only wants coverage for scripts, not for .tsx files, because the existing tests are focused on scripts.
Applied to files:
tests/scripts/index-direct-execution.test.ts
📚 Learning: 2025-01-18T08:44:43.614Z
Learnt from: akshatnema
Repo: asyncapi/website PR: 3423
File: tests/index.test.js:2-7
Timestamp: 2025-01-18T08:44:43.614Z
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.
Applied to files:
tests/scripts/index-direct-execution.test.ts
📚 Learning: 2024-11-01T09:55:20.531Z
Learnt from: akshatnema
Repo: asyncapi/website PR: 3101
File: tests/build-rss.test.js:25-27
Timestamp: 2024-11-01T09:55:20.531Z
Learning: In `tests/build-rss.test.js`, replacing `jest.resetModules()` with `jest.resetAllMocks()` in the `afterEach()` block causes errors. It is necessary to use `jest.resetModules()` to reset the module registry between tests in this file.
Applied to files:
tests/scripts/index-direct-execution.test.ts
📚 Learning: 2024-11-10T18:16:22.273Z
Learnt from: vishvamsinh28
Repo: asyncapi/website PR: 3284
File: scripts/build-post-list.js:49-53
Timestamp: 2024-11-10T18:16:22.273Z
Learning: In `scripts/build-post-list.js`, the `walkDirectories` function depends on sequential execution because it mutates `resultObj`. Parallelizing the loop with `Promise.all` causes tests to fail.
Applied to files:
tests/scripts/index-direct-execution.test.tsscripts/index.ts
📚 Learning: 2024-11-29T17:36:09.783Z
Learnt from: vishvamsinh28
Repo: asyncapi/website PR: 3378
File: tests/markdown/check-markdown.test.js:133-138
Timestamp: 2024-11-29T17:36:09.783Z
Learning: When testing the 'main' function in 'check-markdown.test.js', it's acceptable to simply ensure it doesn't throw any errors, as the functions it calls are already tested elsewhere.
Applied to files:
tests/scripts/index-direct-execution.test.ts
📚 Learning: 2024-11-10T18:16:35.551Z
Learnt from: vishvamsinh28
Repo: asyncapi/website PR: 3284
File: scripts/build-post-list.js:28-46
Timestamp: 2024-11-10T18:16:35.551Z
Learning: In the `scripts/build-post-list.js` JavaScript file, tests rely on the `result` object being a global variable. Moving it inside the `buildPostList` function causes tests to fail; therefore, keep `result` as a global variable.
Applied to files:
tests/scripts/index-direct-execution.test.ts
📚 Learning: 2024-11-01T09:35:23.912Z
Learnt from: akshatnema
Repo: asyncapi/website PR: 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.
Applied to files:
scripts/index.ts
📚 Learning: 2025-06-19T13:51:27.459Z
Learnt from: sagarkori143
Repo: asyncapi/website PR: 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.
Applied to files:
scripts/index.ts
🧬 Code graph analysis (1)
scripts/index.ts (7)
scripts/dashboard/build-dashboard.ts (1)
start(328-328)scripts/build-post-list.ts (1)
buildPostList(288-314)scripts/build-rss.ts (1)
rssFeed(35-132)scripts/casestudies/index.ts (1)
buildCaseStudiesList(18-39)scripts/build-tools.ts (1)
buildToolsManual(110-110)scripts/usecases/index.ts (1)
buildUsecasesList(18-20)scripts/finance/index.ts (1)
buildFinanceInfoList(28-58)
⏰ Context from checks skipped due to timeout of 180000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Redirect rules - asyncapi-website
- GitHub Check: Header rules - asyncapi-website
- GitHub Check: Pages changed - asyncapi-website
- GitHub Check: Test NodeJS PR - windows-latest
🔇 Additional comments (2)
scripts/index.ts (1)
31-46: Exportedstart()+ orchestration looks fine. Good separation between “importable API” and “direct execution” behavior.tsconfig.json (1)
15-21:allowImportingTsExtensionsis properly configured and compatible with your setup. The option is supported in TypeScript 5.3.3 (available since TS 5.0) and works correctly with yournoEmit: trueconfiguration, which is the required prerequisite for using this flag. This aligns with the project's need to import TypeScript files with explicit.tsextensions in tests and scripts.
|
please review and merge this pr @asyncapi-bot , @Mayaleeeee , @akshatnema , @anshgoyalevil , @derberg , @princerajpoot20 , @sambhavgupta0705 as soon as it possible |
2 participants
Description
catchblock (lines 56–59) to return a proper Netlify Function response, including abodyfield with JSON-stringified error details.Summary by CodeRabbit
Bug Fixes
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.