Unit test cases for all functions #120
Conversation
|
Warning Rate limit exceeded@Chrahuldeveloper has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 12 minutes and 28 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (2)
WalkthroughConverted many functions from ES modules to CommonJS, added Jest test scripts and multiple Jest suites, refactored database-cleaner to cursor-based batched cleanup with new existence checks, and applied validation, formatting, and minor control-flow adjustments across handlers and utilities. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45–60 minutes
Possibly related PRs
Suggested labels
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 20
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
functions/sync-all-documents-with-meilisearch/src/utils.js (1)
8-18: Code duplication: extract shared throwIfMissing to a common utility.The
throwIfMissingfunction is duplicated across multiple files (functions/match-maker/src/utils.js, functions/send-otp/src/utils.js, functions/sync-stories-with-meilisearch/src/utils.js, functions/sync-users-with-meilisearch/src/utils.js). This violates the DRY principle and makes maintenance harder.Consider creating a shared utility module that all functions can import:
- Create
shared/utils.jsat the repository root:const throwIfMissing = (obj, keys) => { const missing = []; for (let key of keys) { if (!(key in obj) || !obj[key]) { missing.push(key); } } if (missing.length > 0) { throw new Error(`Missing required fields: ${missing.join(', ')}`); } }; module.exports = { throwIfMissing };
- Update each function's utils.js to import from the shared module:
const { throwIfMissing } = require('../../shared/utils'); module.exports = { throwIfMissing };functions/sync-all-documents-with-meilisearch/src/main.js (1)
5-21: Validate Appwrite env vars now that you require an API keyYou added
.setKey(process.env.APPWRITE_API_KEY), butthrowIfMissingonly validates the Meili envs. If any ofAPPWRITE_ENDPOINT,APPWRITE_FUNCTION_PROJECT_ID, orAPPWRITE_API_KEYare missing, you’ll get harder‑to‑diagnose runtime errors from the Appwrite client.Consider:
- throwIfMissing(process.env, [ - 'MEILISEARCH_ENDPOINT', - 'MEILISEARCH_ADMIN_API_KEY', - ]); + throwIfMissing(process.env, [ + 'MEILISEARCH_ENDPOINT', + 'MEILISEARCH_ADMIN_API_KEY', + 'APPWRITE_ENDPOINT', + 'APPWRITE_FUNCTION_PROJECT_ID', + 'APPWRITE_API_KEY', + ]);to fail fast with a clear message.
🟠 Major comments (10)
functions/upcomingRoom-Message-Notification/package.json-13-15 (1)
13-15: Move Jest dependencies to devDependencies.Testing libraries should be in
devDependencies, notdependencies. Production deployments don't need testing tools, and including them independenciesunnecessarily increases bundle size.Apply this diff:
"license": "ISC", "dependencies": { - "@jest/globals": "^30.2.0", "firebase-admin": "^11.11.0", + "node-appwrite": "^17.0.0" + }, + "devDependencies": { + "@jest/globals": "^30.2.0", "jest": "^30.2.0", - "node-appwrite": "^17.0.0" } }Committable suggestion skipped: line range outside the PR's diff.
functions/upcomingRoom-isTime-checker/package.json-7-7 (1)
7-7: Move Jest packages to devDependencies.Test dependencies belong in
devDependencies, notdependencies, to keep production bundles lean and follow npm conventions.Apply this diff:
"scripts": { "test": "jest" }, "keywords": [], "author": "", "license": "ISC", + "devDependencies": { + "@jest/globals": "^30.2.0", + "jest": "^30.2.0" + }, "dependencies": { - "@jest/globals": "^30.2.0", "firebase-admin": "^11.11.0", - "jest": "^30.2.0", "node-appwrite": "^17.0.0" }Jest 30.2.0 is the latest stable version as of November 2025, confirming the version specified is current and stable.
Also applies to: lines 13-13, 15-15
functions/verify-otp/package.json-12-16 (1)
12-16: Move Jest to devDependencies.Jest and @jest/globals are test-time dependencies and should be in
devDependencies, notdependencies. Placing them in runtime dependencies increases bundle size and installation time in production.Apply this diff:
}, - "dependencies": { + "devDependencies": { "@jest/globals": "^30.2.0", "jest": "^30.2.0", - "node-appwrite": "^17.0.0" - }, - "devDependencies": { + "node-appwrite": "^17.0.0", "prettier": "^3.0.0" }Committable suggestion skipped: line range outside the PR's diff.
functions/join-room/src/main.js-10-18 (1)
10-18: Avoid parsing request body twice.
JSON.parse(req.body)is called on line 11 and again on line 18. This is inefficient and could theoretically produce different results if the body is modified between calls.Apply this diff to parse once:
+ let body; try { - throwIfMissing(JSON.parse(req.body), ["roomName", "uid"]); + body = JSON.parse(req.body); + throwIfMissing(body, ["roomName", "uid"]); } catch (err) { return res.json({ msg: err.message }, 400); } try { log(req); - const { roomName, uid: userId } = JSON.parse(req.body); + const { roomName, uid: userId } = body;functions/delete-room/package.json-12-17 (1)
12-17: Move Jest to devDependencies.Jest and @jest/globals are test-time only dependencies and should be in
devDependencies, notdependencies. This reduces production bundle size.Apply this diff:
}, "dependencies": { - "@jest/globals": "^30.2.0", - "jest": "^30.2.0", "livekit-server-sdk": "^1.2.6", "node-appwrite": "^17.0.0" }, "devDependencies": { + "@jest/globals": "^30.2.0", + "jest": "^30.2.0", "prettier": "^3.0.0" }functions/join-room/src/main.js-4-8 (1)
4-8: Move environment validation inside the request handler.Environment validation at module level executes when the module is loaded (on
require()), not per request. If any environment variable is missing, the entire service crashes on startup, making it impossible to handle the error gracefully or return a meaningful response.Apply this diff to move validation inside the handler:
const { generateToken } = require("./livekit.js"); const { throwIfMissing } = require("./utils.js"); + module.exports = async ({ req, res, log, error }) => { + try { throwIfMissing(process.env, [ "LIVEKIT_API_KEY", "LIVEKIT_API_SECRET", "LIVEKIT_SOCKET_URL", ]); + } catch (err) { + error(err.message); + return res.json({ msg: err.message }, 500); + } try {functions/sync-all-documents-with-meilisearch/package.json-12-17 (1)
12-17: Move Jest to devDependencies.Jest and @jest/globals should be in
devDependenciessince they're only needed during testing, not at runtime.Apply this diff:
"keywords": [], - "dependencies": { - "@jest/globals": "^30.2.0", - "jest": "^30.2.0", + "dependencies": { "meilisearch": "^0.40.0", "node-appwrite": "^14.1.0" }, "devDependencies": { + "@jest/globals": "^30.2.0", + "jest": "^30.2.0", "prettier": "^3.2.5" }functions/send-otp/src/main.js-17-33 (1)
17-33: Return a non-200 status code on sendOtp errors.In the catch block you log the error and return a JSON error payload, but you don’t set an explicit HTTP status, so it will likely return 200 even on failure. Other handlers in this PR use 500 for similar error cases; aligning with that would make error handling clearer for clients.
Consider:
- } catch (e) { - error(String(e)); - return res.json({ message: `Error: ${String(e)}` }); - } + } catch (e) { + error(String(e)); + return res.json({ message: `Error: ${String(e)}` }, 500); + }Committable suggestion skipped: line range outside the PR's diff.
functions/match-maker/src/main.js-19-23 (1)
19-23: Guard against missing event header and avoid logging full headers
triggerEventis read fromreq.headers["x-appwrite-event"]and then.splitis called immediately. If the header is absent, this will throw at runtime.- Logging the entire
req.headerscan leak sensitive information (auth tokens, signatures) into logs.Consider:
- log(req.headers); - const triggerEvent = req.headers["x-appwrite-event"]; - const parts = triggerEvent.split("."); + const triggerEvent = req.headers["x-appwrite-event"]; + if (!triggerEvent) { + return res.json({ message: "Missing x-appwrite-event header" }, 400); + } + log({ "x-appwrite-event": triggerEvent }); + const parts = triggerEvent.split(".");This both hardens error handling and limits logged data.
functions/database-cleaner/src/appwrite.js-32-47 (1)
32-47: FixforEachwith async callbacks and add pagination to match other cleaners
cleanParticipantsCollectionhas two issues:
- Race condition:
forEachdoes not await async callbacks, so the method resolves before deletions complete.- Incomplete cleanup: Without pagination, only the first ~25 participants are processed (Appwrite's default limit per request).
The
cleanActivePairsCollectionmethod (line 49+) implements the correct pattern withdo...whilepagination andPromise.all(). Apply the same approach here.async cleanParticipantsCollection() { + let done = false; + do { const participantDocs = await this.databases.listDocuments( process.env.MASTER_DATABASE_ID, process.env.PARTICIPANTS_COLLECTION_ID + [Query.limit(25)] ); - - participantDocs.documents.forEach(async (participantDoc) => { - if (!(await this.doesRoomExist(participantDoc.roomId))) { - await this.databases.deleteDocument( - process.env.MASTER_DATABASE_ID, - process.env.PARTICIPANTS_COLLECTION_ID, - participantDoc.$id - ); - } - }); + + await Promise.all( + participantDocs.documents.map(async (participantDoc) => { + if (!(await this.doesRoomExist(participantDoc.roomId))) { + await this.databases.deleteDocument( + process.env.MASTER_DATABASE_ID, + process.env.PARTICIPANTS_COLLECTION_ID, + participantDoc.$id + ); + } + }) + ); + + done = participantDocs.documents.length < 25; + } while (!done); }
🟡 Minor comments (5)
init.sh-8-9 (1)
8-9: Remove trailing whitespace on line 8 and reconsider the blank line addition.Line 8 introduces trailing whitespace after the string value
"UNKNOWN:$OS", which violates standard code quality practices and will likely fail linting checks. Additionally, the blank line added at line 9 appears arbitrary and unrelated to the PR's stated objective of adding unit tests. These formatting changes to the bash initialization script seem incidental to the PR scope.Apply this diff to remove the trailing whitespace:
- *) OS_TYPE="UNKNOWN:$OS" + *) OS_TYPE="UNKNOWN:$OS" - +functions/join-room/src/main.test.js-9-15 (1)
9-15: Mock implementation doesn't match actual utility behavior.The mock on line 12 throws
"Missing required field: ${key}"(singular), but the actualthrowIfMissingimplementation throws"Missing required fields: ${missing.join(', ')}"(plural with all missing keys). This discrepancy means the test on line 67 validates against mock behavior rather than actual behavior.Apply this diff to match the actual implementation:
jest.mock("./utils.js", () => ({ throwIfMissing: jest.fn((obj, arr) => { + const missing = []; arr.forEach((key) => { - if (!obj[key]) throw new Error(`Missing required field: ${key}`); + if (!(key in obj) || !obj[key]) missing.push(key); }); + if (missing.length > 0) { + throw new Error(`Missing required fields: ${missing.join(', ')}`); + } }), }));And update the test expectation:
expect(mockRes.json).toHaveBeenCalledWith( - { msg: "Missing required field: roomName" }, + { msg: "Missing required fields: roomName" }, 400 );functions/verify-otp/src/main.js-4-24 (1)
4-24: Tighten env + body validation to avoid hard crashesRight now you only validate
APPWRITE_API_KEYand DB/collection IDs, but you also unconditionally useAPPWRITE_FUNCTION_PROJECT_IDand assumereq.bodyis valid JSON withotpID,userOTP, andverify_ID. If any of these are missing/invalid, the function will throw before your try/catch blocks and may not return a clean JSON error.Consider:
- Adding
APPWRITE_FUNCTION_PROJECT_IDto thethrowIfMissingenv list.- Optionally wrapping
JSON.parse(req.body)+ destructuring in a small try/catch and validating the required body fields, returning a 400 on bad input instead of letting it bubble.functions/verify-email/src/main.js-5-23 (1)
5-23: Align env validation and response semantics with other functionsA couple of consistency / robustness points here:
- You validate only
APPWRITE_API_KEY, but also rely onAPPWRITE_FUNCTION_PROJECT_ID. If it’s missing,setProject(undefined)will blow up later in the SDK. Add it to thethrowIfMissinglist.- For symmetry with
verify-otpand clearer semantics, it’s worth returning an explicit 200:- throwIfMissing(process.env, ["APPWRITE_API_KEY"]); + throwIfMissing(process.env, [ + "APPWRITE_API_KEY", + "APPWRITE_FUNCTION_PROJECT_ID", + ]); … - return res.json({ message: "null" }); + return res.json({ message: "null" }, 200);You may also want to (optionally) wrap
JSON.parse(req.body)in a try/catch and treat malformed bodies as 400s.functions/delete-room/src/main.test.js-13-16 (1)
13-16: FixQuery.equalmock to return something usableThe current mock:
- Misspells
fieldasfeild.- Uses a block body without
return, soQuery.equalalways returnsundefined.If the code under test ever depends on the return value, this will cause misleading failures. A simple, self-describing stub works better:
- Query: { - equal: jest.fn((feild, value) => { - feild, value; - }), - }, + Query: { + equal: jest.fn((field, value) => ({ field, value })), + },
🧹 Nitpick comments (17)
functions/upcomingRoom-Message-Notification/src/main.test.js (1)
35-117: Consider adding error handling test cases.While the current tests cover success, partial failure, and missing tokens scenarios well, consider adding tests for:
- Firebase/Appwrite throwing exceptions
- Malformed request body (invalid JSON)
- Missing required fields (roomId or payload)
These would improve robustness validation but are not critical for the current PR scope.
functions/upcomingRoom-isTime-checker/src/main.test.js (1)
36-127: Test coverage is solid, but consider mocking Date for reliability.The tests correctly verify the ±5 minute window logic and handle multiple scenarios. However, each test creates its own
nowwhile the function under test creates a separatenow, introducing potential timing skew that could cause flakiness.For more reliable tests, consider either:
- Mock the Date constructor:
beforeEach(() => { jest.useFakeTimers(); jest.setSystemTime(new Date('2025-01-01T12:00:00Z')); }); afterEach(() => { jest.useRealTimers(); });
- Or refactor main.js to accept an optional time parameter for testing (requires changes outside this PR).
Additionally, consider adding tests for error scenarios (e.g., database failures, malformed dates).
functions/sync-stories-with-meilisearch/src/utils.js (1)
8-20: Extract shared utility to eliminate duplication.The
throwIfMissingfunction is duplicated across at least 5 files (match-maker, send-otp, sync-all-documents, sync-users, and this file). Consider extracting this to a shared utility module to reduce duplication and ensure consistency.Do you want me to generate a shared utility module structure for this project?
functions/delete-room/package.json (1)
8-8: Remove extra space in test script.There's an extra space before the colon in
"test" : "jest". While functional, it's inconsistent with the other scripts.Apply this diff:
"scripts": { - "test" : "jest", + "test": "jest", "start": "node src/main.js",functions/send-otp/src/utils.js (1)
1-13: throwIfMissing implementation is fine; confirm falsy‑value semanticsLogic and CommonJS export look good. Note that
!obj[key]treats0,false, and''as missing—fine for config/env checks but surprising if those are valid values. Adjust condition if you ever need to allow falsy-but-present values.functions/create-room/src/appwrite.test.js (1)
1-40: Solid AppwriteService test; consider resetting the shared mockThe mock setup correctly verifies
createRoom’s interaction withcreateDocumentand the returned$id. To keep future tests isolated, it’s worth addingcreateDocumentMock.mockReset()insidebeforeEachso additional tests don’t inherit prior call history.functions/sync-users-with-meilisearch/src/utils.js (1)
8-20: throwIfMissing is correct; consider centralizing this shared helperThe implementation and CommonJS export are fine. Since the same
throwIfMissinglogic appears in several utils modules, you might later want to centralize it (e.g., a shared helper package) to avoid copy‑paste divergence.functions/sync-stories-with-meilisearch/src/main.test.js (1)
1-80: Good coverage of Meilisearch sync flow; optional assertion on index name.The tests nicely cover create, update, delete, and validation (missing ID/trigger) paths with mocked MeiliSearch. If you want stronger regression protection, consider making
indexajest.fn()and asserting it’s called with the expected index name (e.g.,'stories'), but this is optional polish.functions/sync-users-with-meilisearch/src/main.test.js (1)
1-80: User sync tests are solid; optional enhancement similar to stories tests.This suite cleanly covers create/update/delete plus error cases for the users index. As with the stories tests, you could optionally assert the
indexmethod is invoked with'users'to lock down the correct index selection.functions/send-otp/src/main.test.js (1)
1-39: Happy-path OTP test is correct; consider adding error-path coverage.The test wiring and assertions for mail sending and OTP document creation look good and align with
sendOtp. To better protect against regressions, consider adding tests for failure scenarios (e.g.,sendMailorcreateOtpDocumentrejecting, or malformedreq.body) to assert that the error handler and JSON error response behave as expected.functions/sync-users-with-meilisearch/src/main.js (1)
1-56: User sync handler and new guards look correct; consider explicit handling for unknown events.The handler’s structure, MeiliSearch usage, and new guards for missing document IDs and non-Appwrite-triggered requests all look solid and match the tests. You might optionally add a final
else/default branch for unexpectedx-appwrite-eventvalues (neither delete nor create/update) to return a clear 4xx response instead of silently doing nothing.functions/database-cleaner/src/utils.js (1)
13-17: MakeRETENTION_PERIOD_DAYSparsing defensive
+(process.env.RETENTION_PERIOD_DAYS ?? 1)will yieldNaNif the env is non‑numeric, andsetDate(getDate() - NaN)results in an invalid date, sotoISOString()will throw.Suggest normalizing with a safe number and sane default, e.g.:
- const retentionPeriod = +(process.env.RETENTION_PERIOD_DAYS ?? 1); + const raw = Number(process.env.RETENTION_PERIOD_DAYS ?? 1); + const retentionPeriod = Number.isFinite(raw) && raw >= 0 ? raw : 1;This keeps behavior the same for valid values but avoids crashes on bad configuration.
functions/match-maker/src/main.test.js (1)
5-106: Good coverage of main branches; consider asserting env validationThe mocks and three tests nicely exercise the “not random”, “empty queue”, and “pairing error” paths, and the Appwrite client/database mocks are sufficient for the current behavior.
If you want to harden the contract a bit more, you could also assert that
throwIfMissingis called once at the start of the handler (since you already mock it), so future changes don’t accidentally drop env validation.functions/match-maker/src/utils.js (1)
3-15: LGTM; optional: deduplicatethrowIfMissingacross functionsThis
throwIfMissingimplementation is straightforward and consistent with the other function folders.If this pattern keeps spreading, consider extracting a shared helper (e.g., a small internal package or shared utils module) to avoid having to update the same logic in multiple places later.
functions/database-cleaner/src/appwrite.test.js (1)
1-125: Solid coverage; consider tightening async semantics ofcleanParticipantsCollectionThe mocks and three tests give good coverage of
doesRoomExist,cleanParticipantsCollection, andclearOldOTPs, including happy paths and error handling.The extra:
await new Promise(process.nextTick);in the
cleanParticipantsCollectiontest hints that the implementation may fire off async work without fully awaiting it. Ideally,await service.cleanParticipantsCollection()should only resolve once deletions are complete, letting you drop the manualnextTickand rely purely on the method’s Promise.Not a blocker, but worth revisiting when you next touch
AppwriteService.functions/sync-stories-with-meilisearch/src/main.js (1)
18-47: Return an explicit response for unsupported Appwrite event typesIf
x-appwrite-eventdoes not include.delete,.create, or.update, the function currently falls through thetryblock without sending any response. It would be clearer and safer to return an explicit 200/400 with a message indicating that the event type is ignored/unsupported.Example:
if (eventType.includes('.delete')) { ... - } else if ( + } else if ( eventType.includes('.create') || eventType.includes('.update') ) { ... + } else { + log(`Ignoring unsupported event type: ${eventType}`); + return res.text(`No action for event type: ${eventType}`, 200); }functions/match-maker/src/main.js (1)
5-10: Include APPWRITE_FUNCTION_PROJECT_ID in env validationThe Appwrite client is configured with
APPWRITE_FUNCTION_PROJECT_ID, but that variable is not part of thethrowIfMissingcheck. If it’s unset, the client will be misconfigured in a non-obvious way.throwIfMissing(process.env, [ "APPWRITE_API_KEY", + "APPWRITE_FUNCTION_PROJECT_ID", "DATABASE_ID", "REQUESTS_COLLECTION_ID", "ACTIVE_PAIRS_COLLECTION_ID", ]);This keeps configuration failures fail-fast and easier to diagnose.
Also applies to: 12-15
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (14)
functions/create-room/package-lock.jsonis excluded by!**/package-lock.jsonfunctions/database-cleaner/package-lock.jsonis excluded by!**/package-lock.jsonfunctions/delete-room/package-lock.jsonis excluded by!**/package-lock.jsonfunctions/join-room/package-lock.jsonis excluded by!**/package-lock.jsonfunctions/match-maker/package-lock.jsonis excluded by!**/package-lock.jsonfunctions/send-otp/package-lock.jsonis excluded by!**/package-lock.jsonfunctions/start-friend-call/package-lock.jsonis excluded by!**/package-lock.jsonfunctions/sync-all-documents-with-meilisearch/package-lock.jsonis excluded by!**/package-lock.jsonfunctions/sync-stories-with-meilisearch/package-lock.jsonis excluded by!**/package-lock.jsonfunctions/sync-users-with-meilisearch/package-lock.jsonis excluded by!**/package-lock.jsonfunctions/upcomingRoom-Message-Notification/package-lock.jsonis excluded by!**/package-lock.jsonfunctions/upcomingRoom-isTime-checker/package-lock.jsonis excluded by!**/package-lock.jsonfunctions/verify-email/package-lock.jsonis excluded by!**/package-lock.jsonfunctions/verify-otp/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (50)
functions/create-room/package.json(2 hunks)functions/create-room/src/appwrite.js(1 hunks)functions/create-room/src/appwrite.test.js(1 hunks)functions/create-room/src/livekit.js(1 hunks)functions/create-room/src/main.js(1 hunks)functions/database-cleaner/package.json(1 hunks)functions/database-cleaner/src/appwrite.js(1 hunks)functions/database-cleaner/src/appwrite.test.js(1 hunks)functions/database-cleaner/src/utils.js(1 hunks)functions/delete-room/package.json(1 hunks)functions/delete-room/src/main.js(1 hunks)functions/delete-room/src/main.test.js(1 hunks)functions/join-room/package.json(1 hunks)functions/join-room/src/main.js(1 hunks)functions/join-room/src/main.test.js(1 hunks)functions/match-maker/package.json(1 hunks)functions/match-maker/src/main.js(1 hunks)functions/match-maker/src/main.test.js(1 hunks)functions/match-maker/src/utils.js(1 hunks)functions/send-otp/package.json(1 hunks)functions/send-otp/src/appwrite.js(1 hunks)functions/send-otp/src/mail.js(1 hunks)functions/send-otp/src/main.js(1 hunks)functions/send-otp/src/main.test.js(1 hunks)functions/send-otp/src/utils.js(1 hunks)functions/start-friend-call/package.json(1 hunks)functions/start-friend-call/src/main.js(2 hunks)functions/sync-all-documents-with-meilisearch/package.json(1 hunks)functions/sync-all-documents-with-meilisearch/src/main.js(4 hunks)functions/sync-all-documents-with-meilisearch/src/main.test.js(1 hunks)functions/sync-all-documents-with-meilisearch/src/utils.js(2 hunks)functions/sync-stories-with-meilisearch/package.json(1 hunks)functions/sync-stories-with-meilisearch/src/main.js(3 hunks)functions/sync-stories-with-meilisearch/src/main.test.js(1 hunks)functions/sync-stories-with-meilisearch/src/utils.js(2 hunks)functions/sync-users-with-meilisearch/package.json(1 hunks)functions/sync-users-with-meilisearch/src/main.js(3 hunks)functions/sync-users-with-meilisearch/src/main.test.js(1 hunks)functions/sync-users-with-meilisearch/src/utils.js(2 hunks)functions/upcomingRoom-Message-Notification/package.json(1 hunks)functions/upcomingRoom-Message-Notification/src/main.test.js(1 hunks)functions/upcomingRoom-isTime-checker/package.json(1 hunks)functions/upcomingRoom-isTime-checker/src/main.test.js(1 hunks)functions/verify-email/package.json(1 hunks)functions/verify-email/src/main.js(1 hunks)functions/verify-email/src/main.test.js(1 hunks)functions/verify-otp/package.json(1 hunks)functions/verify-otp/src/main.js(1 hunks)functions/verify-otp/src/main.test.js(1 hunks)init.sh(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (28)
functions/join-room/src/main.test.js (1)
functions/join-room/src/main.js (2)
require(1-1)require(2-2)
functions/verify-otp/src/main.test.js (1)
functions/verify-otp/src/main.js (2)
require(1-1)require(2-2)
functions/match-maker/src/main.js (1)
functions/match-maker/src/utils.js (1)
throwIfMissing(3-13)
functions/match-maker/src/utils.js (4)
functions/send-otp/src/utils.js (2)
throwIfMissing(1-11)missing(2-2)functions/sync-all-documents-with-meilisearch/src/utils.js (2)
throwIfMissing(8-18)missing(9-9)functions/sync-stories-with-meilisearch/src/utils.js (2)
throwIfMissing(8-18)missing(9-9)functions/sync-users-with-meilisearch/src/utils.js (2)
throwIfMissing(8-18)missing(9-9)
functions/sync-users-with-meilisearch/src/utils.js (4)
functions/match-maker/src/utils.js (1)
throwIfMissing(3-13)functions/send-otp/src/utils.js (1)
throwIfMissing(1-11)functions/sync-all-documents-with-meilisearch/src/utils.js (1)
throwIfMissing(8-18)functions/sync-stories-with-meilisearch/src/utils.js (1)
throwIfMissing(8-18)
functions/send-otp/src/main.test.js (2)
functions/send-otp/src/main.js (3)
require(1-1)MailService(3-3)AppwriteService(2-2)functions/send-otp/src/utils.js (1)
throwIfMissing(1-11)
functions/database-cleaner/src/appwrite.test.js (2)
functions/create-room/src/appwrite.test.js (2)
AppwriteService(19-19)service(22-22)functions/database-cleaner/src/appwrite.js (2)
require(1-1)require(3-3)
functions/delete-room/src/main.js (1)
functions/delete-room/src/utils.js (2)
throwIfMissing(1-11)throwIfMissing(1-11)
functions/send-otp/src/mail.js (1)
functions/send-otp/src/main.js (3)
require(1-1)MailService(3-3)otp(21-21)
functions/create-room/src/livekit.js (1)
functions/create-room/src/main.js (1)
accessToken(50-50)
functions/verify-email/src/main.test.js (1)
functions/verify-email/src/main.js (3)
require(1-1)require(2-2)JSON(16-16)
functions/create-room/src/appwrite.test.js (1)
functions/create-room/src/appwrite.js (1)
require(1-1)
functions/verify-otp/src/main.js (2)
functions/verify-otp/src/main.test.js (1)
require(2-2)functions/verify-otp/src/utils.js (2)
throwIfMissing(1-11)throwIfMissing(1-11)
functions/upcomingRoom-isTime-checker/src/main.test.js (1)
functions/upcomingRoom-isTime-checker/src/main.js (1)
now(41-41)
functions/sync-users-with-meilisearch/src/main.test.js (1)
functions/sync-users-with-meilisearch/src/main.js (2)
require(1-1)require(2-2)
functions/sync-stories-with-meilisearch/src/utils.js (4)
functions/match-maker/src/utils.js (1)
throwIfMissing(3-13)functions/send-otp/src/utils.js (1)
throwIfMissing(1-11)functions/sync-all-documents-with-meilisearch/src/utils.js (1)
throwIfMissing(8-18)functions/sync-users-with-meilisearch/src/utils.js (1)
throwIfMissing(8-18)
functions/match-maker/src/main.test.js (1)
functions/match-maker/src/main.js (2)
require(1-1)require(2-2)
functions/send-otp/src/utils.js (4)
functions/match-maker/src/utils.js (2)
throwIfMissing(3-13)missing(4-4)functions/sync-all-documents-with-meilisearch/src/utils.js (2)
throwIfMissing(8-18)missing(9-9)functions/sync-stories-with-meilisearch/src/utils.js (2)
throwIfMissing(8-18)missing(9-9)functions/sync-users-with-meilisearch/src/utils.js (2)
throwIfMissing(8-18)missing(9-9)
functions/sync-all-documents-with-meilisearch/src/main.js (2)
functions/sync-stories-with-meilisearch/src/main.js (1)
meilisearch(11-14)functions/sync-users-with-meilisearch/src/main.js (1)
meilisearch(11-14)
functions/sync-stories-with-meilisearch/src/main.test.js (1)
functions/sync-stories-with-meilisearch/src/main.js (2)
require(1-1)require(2-2)
functions/database-cleaner/src/appwrite.js (1)
functions/database-cleaner/src/utils.js (1)
getExpiryDate(13-18)
functions/send-otp/src/appwrite.js (1)
functions/send-otp/src/main.js (3)
require(1-1)AppwriteService(2-2)otp(21-21)
functions/create-room/src/appwrite.js (1)
functions/create-room/src/appwrite.test.js (1)
AppwriteService(19-19)
functions/verify-email/src/main.js (2)
functions/verify-email/src/main.test.js (1)
require(2-2)functions/verify-email/src/utils.js (2)
throwIfMissing(1-11)throwIfMissing(1-11)
functions/sync-all-documents-with-meilisearch/src/main.test.js (1)
functions/sync-all-documents-with-meilisearch/src/main.js (3)
require(1-1)require(2-2)require(3-3)
functions/sync-all-documents-with-meilisearch/src/utils.js (4)
functions/match-maker/src/utils.js (1)
throwIfMissing(3-13)functions/send-otp/src/utils.js (1)
throwIfMissing(1-11)functions/sync-stories-with-meilisearch/src/utils.js (1)
throwIfMissing(8-18)functions/sync-users-with-meilisearch/src/utils.js (1)
throwIfMissing(8-18)
functions/sync-stories-with-meilisearch/src/main.js (3)
functions/sync-all-documents-with-meilisearch/src/main.js (4)
require(1-1)require(2-2)require(3-3)meilisearch(18-21)functions/sync-users-with-meilisearch/src/main.js (3)
require(1-1)require(2-2)meilisearch(11-14)functions/sync-stories-with-meilisearch/src/utils.js (1)
throwIfMissing(8-18)
functions/join-room/src/main.js (2)
functions/join-room/src/utils.js (2)
throwIfMissing(1-11)throwIfMissing(1-11)functions/join-room/src/livekit.js (3)
accessToken(4-10)generateToken(3-26)generateToken(3-26)
🔇 Additional comments (19)
functions/start-friend-call/src/main.js (1)
1-2: LGTM: CommonJS conversion.The conversion from ES modules to CommonJS is correct and aligns with the broader PR changes.
functions/upcomingRoom-Message-Notification/src/main.test.js (5)
1-31: Well-structured mocking setup.The mocking strategy appropriately isolates external dependencies (Firebase Admin, Firebase Messaging, and Appwrite) and uses virtual mocking for the service account file. The chained method setup for the Appwrite Client correctly mirrors the actual API.
35-57: Solid test setup with proper isolation.The
beforeEachhook correctly resets mocks and initializes environment variables, ensuring test isolation. The request/response/log/error stubs follow Jest best practices.
59-85: Comprehensive happy path test.This test effectively validates token aggregation from both subscription documents and creator tokens, verifies the correct multicast payload structure, and confirms the success response. The assertions clearly document expected behavior.
87-102: Good failure scenario coverage.This test verifies that partial Firebase failures are handled gracefully, with the function still returning success while logging the failure. This demonstrates resilient error handling.
104-117: Edge case handled correctly.The test confirms that the function handles missing tokens gracefully by sending an empty tokens array without throwing errors.
functions/upcomingRoom-Message-Notification/package.json (1)
13-15: I need to verify one more detail about the @jest/globals package version:Jest and @jest/globals versions are valid.
@jest/globals version 30.2.0 is the latest version published 2 months ago, matching the Jest version already confirmed in the previous search. Both packages are stable releases and compatible with each other.
functions/upcomingRoom-isTime-checker/src/main.test.js (1)
1-17: Mock setup looks good.The Appwrite SDK mocking correctly captures the builder pattern for Client and provides test doubles for Databases methods.
functions/sync-all-documents-with-meilisearch/src/utils.js (1)
20-20: The review comment is incorrect—getExpiryDatedoes not exist in this file.The utils.js file contains only the
throwIfMissingfunction, which is the only function being exported. The module.exports statement is correct and complete. There is nogetExpiryDatefunction to export. The original review appears to be based on inaccurate information.Likely an incorrect or invalid review comment.
functions/create-room/src/livekit.js (1)
4-41: LivekitService constructor/token/room creation look correctRoomServiceClient initialization, token grant configuration, and
createRoomwrapper are all consistent and straightforward; no issues spotted.functions/create-room/package.json (1)
8-20: Jest wiring looks good; double‑check version and ESM/CJS configAdding
"test": "jest"and Jest devDependencies makes sense here. Given this package is"type": "module"while several source/test files userequire/module.exports, please confirm your Jest config (likely at repo root) runs these tests in a compatible mode and that the specified Jest versions resolve correctly in your environment.functions/create-room/src/appwrite.js (1)
1-27: AppwriteService logic matches tests; confirm consumer import styleClient setup, Databases wiring, and
createRoomcorrectly passMASTER_DATABASE_ID,ROOMS_COLLECTION_ID, a unique ID, and the room payload, returning the document$id. Since this file now usesrequire/module.exports, please ensure all callers (e.g.,main.js) also use a compatible CommonJS import pattern so nothing breaks at runtime.functions/sync-all-documents-with-meilisearch/src/main.test.js (1)
1-73: MeiliSearch sync tests are well‑structured and cover key pathsMocks for node-appwrite and MeiliSearch are set up cleanly, pagination vs empty‑result behavior is exercised, and mocks/env are reset per test. This gives good confidence in the sync flow.
functions/send-otp/src/appwrite.js (1)
1-26: AppwriteService OTP write flow looks correctClient initialization, Databases wiring, and
createOtpDocument’s use ofVERIFICATION_DATABASE_ID,OTP_COLLECTION_ID, and the{ otp, date }payload all look good, and the CommonJS export matches how it’s consumed.functions/send-otp/src/mail.js (1)
1-25: MailService CommonJS migration looks correct.The nodemailer transporter setup and
sendMailbehavior are unchanged, and the switch torequire/module.exportsis consistent with the rest of the PR.functions/verify-email/src/main.test.js (1)
1-67: Email verification tests cover key success and failure paths well.The mocks for
node-appwriteandthrowIfMissingare set up correctly, and the tests validate both the happy path and an error fromupdateEmailVerification, including status code and error logging. This gives good confidence in the function’s behavior.functions/verify-otp/src/main.test.js (1)
1-95: OTP verification tests thoroughly exercise success and both error paths.The Databases mocks and assertions on
getDocument/createDocumentcalls plus 200/500 responses look correct and comprehensive. This suite gives strong coverage over the OTP verification function’s behavior.functions/sync-stories-with-meilisearch/src/main.js (1)
1-16: CommonJS conversion and MeiliSearch client setup look correctImports, env validation with
throwIfMissing, and MeiliSearch client/index initialization are consistent with the patterns used elsewhere and should work as expected.If you want extra safety, you can quickly confirm MeiliSearch client construction against the versioned docs you’re using.
functions/database-cleaner/src/appwrite.js (1)
5-16: Client and Databases initialization pattern looks goodThe constructor’s use of
Clientwith default endpoint fallback, project ID, and API key, followed bynew Databases(client), is consistent and should integrate cleanly with other Appwrite-based functions.If desired, double-check the endpoint/project env names against your deployment configuration.
There was a problem hiding this comment.
Actionable comments posted: 8
♻️ Duplicate comments (17)
functions/start-friend-call/src/main.js (1)
23-34: Critical: Error handling issue remains unresolved from previous review.The function still returns success even when notification sending fails. The error is caught and logged (lines 28-30), but the caller receives
{ message: "Notification sent" }regardless of the actual outcome, which is misleading.Additionally, mixing
awaitwith.then().catch()is not idiomatic. Use try/catch for clearer error handling and proper error propagation.Apply this diff to fix the error handling:
- await getMessaging(app) - .send(message) - .then((response) => { - log("Notifications were sent successfully, " + JSON.stringify(response)); - }) - .catch((error) => { - log("Error sending message:", error); - }); - - return res.json({ - message: "Notification sent", - }); + try { + const response = await getMessaging(app).send(message); + log("Notifications were sent successfully, " + JSON.stringify(response)); + return res.json({ + message: "Notification sent", + }); + } catch (err) { + log("Error sending message: " + JSON.stringify(err)); + return res.status(500).json({ + message: "Failed to send notification", + error: err.message, + }); + }functions/verify-email/package.json (2)
12-19: Move Jest packages todevDependencies(test-only).
jestand@jest/globalsare only needed at test time; keeping them independenciesbloats the deployed function bundle and may slow cold starts. Move them todevDependenciesalongsideprettier.Suggested change:
"dependencies": { - "@jest/globals": "^30.2.0", - "jest": "^30.2.0", - "node-appwrite": "^17.0.0" + "node-appwrite": "^17.0.0" }, "devDependencies": { + "@jest/globals": "^30.2.0", + "jest": "^30.2.0", "prettier": "^3.0.0" }
6-6: Fix CommonJS/ESM mismatch: remove"type": "module"or convert code to ESM.
src/main.jsnow uses CommonJS (require,module.exports), but"type": "module"forces Node to treat.jsfiles as ES modules, whererequire/module.exportsare not available. This will break the function at runtime. Either remove the"type"field (so it defaults to CommonJS) or convert the source back to ESM syntax.Recommended quick fix:
- "type": "module",functions/sync-all-documents-with-meilisearch/package.json (1)
6-14: Scope Jest to devDependencies and re-verify module type vs source syntax.
"@jest/globals"and"jest"are listed underdependencies, so they’ll ship with the function bundle even though they’re only needed for tests. For consistency with other functions and to keep production lighter, they should live underdevDependencies.package.jsonstill declares"type": "module". A previous review noted that this package’ssrcfiles were using CommonJS (require/module.exports). If that’s still true, this config will cause runtime issues.I’d suggest:
- Moving Jest packages into
devDependencies.- Re-checking the module system used in
functions/sync-all-documents-with-meilisearch/src/**and aligning either by dropping"type": "module"for full CommonJS, or by converting all sources to ESM.Example diff for dependency scoping:
"scripts": { "test" : "jest", "format": "prettier --write ." }, "keywords": [], "dependencies": { - "@jest/globals": "^30.2.0", - "jest": "^30.2.0", "meilisearch": "^0.40.0", "node-appwrite": "^14.1.0" }, "devDependencies": { - "prettier": "^3.2.5" + "@jest/globals": "^30.2.0", + "jest": "^30.2.0", + "prettier": "^3.2.5" }You can re-run a quick check like:
#!/bin/bash rg -n "module\\.exports|= require\\(" functions/sync-all-documents-with-meilisearch/src/ rg -n "^import " functions/sync-all-documents-with-meilisearch/src/ rg -n "^export " functions/sync-all-documents-with-meilisearch/src/to confirm what module syntax is in use now.
functions/database-cleaner/package.json (1)
6-14: Align module system and move Jest to devDependencies.Two things to tighten up here:
Jest in
dependencies:
"@jest/globals"and"jest"are test-only and should live underdevDependenciesto keep the deployed bundle lean.ESM vs CommonJS mismatch (re-raising):
package.jsondeclares"type": "module", but an earlier review foundfunctions/database-cleaner/srcmixingimport/exportwithrequire()/module.exports. Unless all sources have now been unified to pure ESM, this will break at runtime.Suggested change for dependency scoping:
"scripts": { "test" : "jest", "start": "node src/main.js", "format": "prettier --write ." }, "dependencies": { - "@jest/globals": "^30.2.0", - "jest": "^30.2.0", "node-appwrite": "^17.0.0" }, "devDependencies": { + "@jest/globals": "^30.2.0", + "jest": "^30.2.0", "prettier": "^3.0.0" }And please re-validate the module system choice (full ESM vs full CommonJS) for
functions/database-cleaner/src/**and update"type"or source files accordingly.You can reuse the previous check:
#!/bin/bash rg -n "module\\.exports|= require\\(" functions/database-cleaner/src/ rg -n "^import " functions/database-cleaner/src/ rg -n "^export " functions/database-cleaner/src/Also applies to: 20-20
functions/sync-users-with-meilisearch/package.json (1)
6-14: Move Jest to devDependencies and confirm module type matches source code.
"@jest/globals"and"jest"are currently independencies, so they’ll be installed in production even though they’re only used for tests.package.jsonstill declares"type": "module", while an earlier review showedfunctions/sync-users-with-meilisearch/srcusing CommonJS. If that hasn’t changed, this combo will cause runtime failures.Suggested dependency adjustment:
"dependencies": { - "@jest/globals": "^30.2.0", - "jest": "^30.2.0", "meilisearch": "^0.40.0" }, "devDependencies": { - "prettier": "^3.2.5" + "@jest/globals": "^30.2.0", + "jest": "^30.2.0", + "prettier": "^3.2.5" }And please re-check that
"type": "module"matches the actual module syntax insrc/**; either drop it and go full CommonJS, or convert all sources to ESM.You can confirm with:
#!/bin/bash rg -n "module\\.exports|= require\\(" functions/sync-users-with-meilisearch/src/ rg -n "^import " functions/sync-users-with-meilisearch/src/ rg -n "^export " functions/sync-users-with-meilisearch/src/Also applies to: 20-20
functions/delete-room/src/main.js (1)
60-73: Await participant deletions instead of usingforEachwith an async callback.
participantColRef.documents.forEach(async (participant) => { ... })fires deletes without awaiting them. The handler can return (and the function environment can tear down) before all participant documents are removed, leaving orphaned records.Use a
for...ofloop orPromise.allso all deletions complete before proceeding:- log(participantColRef); - participantColRef.documents.forEach(async (participant) => { - await databases.deleteDocument( - process.env.MASTER_DATABASE_ID, - process.env.PARTICIPANTS_COLLECTION_ID, - participant.$id - ); - }); + log(participantColRef); + for (const participant of participantColRef.documents) { + await databases.deleteDocument( + process.env.MASTER_DATABASE_ID, + process.env.PARTICIPANTS_COLLECTION_ID, + participant.$id + ); + }functions/start-friend-call/package.json (1)
6-7: Add Jest (and related) as devDependencies for this package.The
"test": "jest"script now points to Jest, but the package.json doesn’t declare Jest or@jest/globalsanywhere. Runningnpm testfor this function will fail.To align with other functions in this PR, add them under
devDependencies, e.g.:"license": "ISC", + "devDependencies": { + "jest": "^30.2.0", + "@jest/globals": "^30.2.0" + }, "dependencies": { "firebase-admin": "^11.11.0" } }After adding them, run tests for this package to confirm
jestis resolved correctly.functions/match-maker/package.json (2)
6-6: Module type mismatch flagged in previous review.This issue was already identified in a previous review. The
"type": "module"declaration conflicts with CommonJS usage in source files.
13-14: Jest dependencies placement flagged in previous review.This issue was already identified in a previous review. Test dependencies should be in
devDependencies, notdependencies.functions/send-otp/package.json (2)
6-6: Module type mismatch flagged in previous review.This issue was already identified in a previous review. The
"type": "module"declaration conflicts with CommonJS usage in source files.
13-14: Jest dependencies placement flagged in previous review.This issue was already identified in a previous review. Test dependencies should be in
devDependencies, notdependencies.functions/sync-stories-with-meilisearch/package.json (2)
6-6: Module type mismatch flagged in previous review.This issue was already identified in a previous review. The
"type": "module"declaration conflicts with CommonJS usage in source files.
13-14: Jest dependencies placement flagged in previous review.This issue was already identified in a previous review. Test dependencies should be in
devDependencies, notdependencies.functions/join-room/package.json (2)
6-6: Module type mismatch flagged in previous review.This issue was already identified in a previous review. The
"type": "module"declaration conflicts with the module syntax used in source files.
13-14: Jest dependencies placement flagged in previous review.This issue was already identified in a previous review. Test dependencies should be in
devDependencies, notdependencies.functions/match-maker/src/main.js (1)
37-88: Fix out-of-bounds access: iterate overdocuments.length, nottotalThe pairing loop still uses
requestDocsRef.totalas the upper bound even though the query appliesQuery.limit(25).totalis the total number of matching documents across all pages, whilerequestDocsRef.documentscontains only the current page. Whentotal > documents.length, indices ≥documents.lengthwill beundefined, leading to runtime errors when you access.uid,.$id, or.userName.Update the loop to iterate only over the returned page (or use a
for...of) and reference the current document via a local variable:- for (let index = 0; index < requestDocsRef.total; index++) { - try { - const newPairDoc = await db.createDocument( + for (const candidate of requestDocsRef.documents) { + try { + const newPairDoc = await db.createDocument( process.env.DATABASE_ID, process.env.ACTIVE_PAIRS_COLLECTION_ID, ID.unique(), { uid1: newRequestDoc.uid, - uid2: requestDocsRef.documents[index].uid, + uid2: candidate.uid, userDocId1: newRequestDocId, - userDocId2: requestDocsRef.documents[index].$id, + userDocId2: candidate.$id, ...(newRequestDoc.isAnonymous ? {} : { userName1: newRequestDoc.userName, - userName2: requestDocsRef.documents[index].userName, + userName2: candidate.userName, }), } ); ... - await db.deleteDocument( - process.env.DATABASE_ID, - process.env.REQUESTS_COLLECTION_ID, - requestDocsRef.documents[index].$id - ); + await db.deleteDocument( + process.env.DATABASE_ID, + process.env.REQUESTS_COLLECTION_ID, + candidate.$id + );This preserves behavior while avoiding undefined accesses when there are more matches than the page size.
🧹 Nitpick comments (22)
init.sh (1)
8-9: Remove trailing whitespace and unnecessary blank line for consistency.Line 8 contains a trailing space after the quoted string, and line 9 introduces an extra blank line within the case statement. While these changes don't affect functionality, removing the trailing whitespace and consolidating blank lines would improve consistency with the rest of the codebase and prevent linter warnings.
Apply this diff to clean up the formatting:
- *) OS_TYPE="UNKNOWN:$OS" - + *) OS_TYPE="UNKNOWN:$OS"functions/upcomingRoom-isTime-checker/package.json (1)
12-17: Move Jest dependencies to devDependencies.Jest and @jest/globals are test-only dependencies and should be in
devDependenciesrather thandependencies. This prevents them from being bundled in production deployments, reducing the serverless function's deployment size.Apply this diff to move test dependencies:
"license": "ISC", "dependencies": { - "@jest/globals": "^30.2.0", "firebase-admin": "^11.11.0", - "jest": "^30.2.0", "node-appwrite": "^17.0.0" + }, + "devDependencies": { + "@jest/globals": "^30.2.0", + "jest": "^30.2.0" }functions/verify-email/src/main.test.js (1)
1-67: Nice coverage; consider asserting env validation viathrowIfMissing.The tests cleanly cover both success and error paths. Since
./utils.jsis mocked, you can also assert thatthrowIfMissingis called with the expected env keys, tightening the contract around configuration:expect(throwIfMissing).toHaveBeenCalledWith(process.env, ["APPWRITE_API_KEY"]);This would fit naturally in the success test after calling
fn(...).functions/verify-email/src/main.js (1)
4-23: Tighten env validation and handle config errors via the same error path.Right now only
APPWRITE_API_KEYis validated, andthrowIfMissingruns outside thetry, so missing config throws before you hit thecatch/res.jsonpath.Consider:
- Validating all required env vars (at least
APPWRITE_API_KEYandAPPWRITE_FUNCTION_PROJECT_ID; keepAPPWRITE_ENDPOINToptional since you already default it).- Moving the env check and
Clientconstruction into thetryso configuration issues are reported via the same{ message, 500 }response as SDK errors.For example:
-module.exports = async ({ req, res, log, error }) => { - throwIfMissing(process.env, ["APPWRITE_API_KEY"]); - - const client = new Client() - .setEndpoint( - process.env.APPWRITE_ENDPOINT ?? "https://cloud.appwrite.io/v1" - ) - .setProject(process.env.APPWRITE_FUNCTION_PROJECT_ID) - .setKey(process.env.APPWRITE_API_KEY); - - try { +module.exports = async ({ req, res, log, error }) => { + try { + throwIfMissing(process.env, [ + "APPWRITE_API_KEY", + "APPWRITE_FUNCTION_PROJECT_ID", + ]); + + const client = new Client() + .setEndpoint( + process.env.APPWRITE_ENDPOINT ?? "https://cloud.appwrite.io/v1" + ) + .setProject(process.env.APPWRITE_FUNCTION_PROJECT_ID) + .setKey(process.env.APPWRITE_API_KEY); + log(req.body); const { userID } = JSON.parse(req.body); await new Users(client).updateEmailVerification(userID, true); + return res.json({ message: "null" }); } catch (e) { error(String(e)); return res.json({ message: String(e) }, 500); } - - return res.json({ message: "null" }); };functions/verify-otp/package.json (1)
6-18: Treat Jest as a devDependency, not a runtime dependency.In this function, Jest and
@jest/globalsare only needed for tests, but they’re independencies. That will pull them into production installs unnecessarily.Consider scoping them to
devDependencies:"scripts": { "test": "jest", "start": "node src/main.js", "format": "prettier --write ." }, - "dependencies": { - "@jest/globals": "^30.2.0", - "jest": "^30.2.0", - "node-appwrite": "^17.0.0" - }, - "devDependencies": { - "prettier": "^3.0.0" - } + "dependencies": { + "node-appwrite": "^17.0.0" + }, + "devDependencies": { + "@jest/globals": "^30.2.0", + "jest": "^30.2.0", + "prettier": "^3.0.0" + }Please also ensure your root Jest config (if any) picks up this function’s tests as expected after the change.
functions/sync-users-with-meilisearch/src/utils.js (1)
2-20: Clarify doc vs implementation behavior forthrowIfMissingThe helper also treats falsy values as “missing” (not just absent keys), which is appropriate for env validation but slightly broader than the JSDoc wording. Consider updating the comment to say “missing or falsy” and, longer term, centralizing this shared helper since the same implementation appears in multiple functions.
functions/create-room/src/appwrite.test.js (1)
1-39: AppwriteService test is valid; make env usage more explicitThe mock shape for
Client,Databases, andID.uniquematchesAppwriteServiceand the assertion oncreateDocument+ returned$idlooks good. To better catch mis-wiring of env vars, consider settingMASTER_DATABASE_IDandROOMS_COLLECTION_IDto known values inbeforeEachand asserting against those literals instead of relying on their current implicitprocess.envvalues.functions/sync-users-with-meilisearch/src/main.js (1)
1-57: Handler logic looks correct; optionally handle unsupported events explicitlyEnv validation, Meilisearch client/index setup, and the delete/create/update branches (plus the new 400 responses for missing
documentIdand missing trigger) all look consistent and correct. For non‑delete/create/update events, the function currently falls through and returnsundefined; you may want to log and return an explicit 400/204 in that case for more predictable behavior.functions/join-room/src/main.test.js (1)
1-85: Good coverage; alignthrowIfMissingmock with real util behaviorThe tests nicely cover the success path, missing body field, and token-generation error. Because the mocked
throwIfMissingreimplements both the validation and the exact error message, these tests will still pass even if the realutils.jsimplementation or its message wording changes. Consider delegating tojest.requireActual('./utils.js').throwIfMissingin the mock, or asserting that the error response message contains"roomName"rather than matching the full string.functions/create-room/src/appwrite.js (1)
1-27: CommonJS export and Appwrite wiring look consistentRequiring
Client,Databases, andIDand exportingAppwriteServiceviamodule.exportslines up with the new test file, andcreateRoomcorrectly forwards the database/collection IDs andID.unique(). As a small robustness improvement, you could add an env validation step in the constructor (e.g., via a sharedthrowIfMissing) so misconfigured env vars fail fast.functions/sync-stories-with-meilisearch/src/main.test.js (1)
1-80: Strong Meilisearch sync coverage; optionally add a 500-path testThe suite cleanly exercises create/update/delete flows and the two 400 cases (no document ID, no trigger), with the Meilisearch client nicely mocked. If you want to exercise the error handling in
main.js, you could add a test whereaddDocumentsordeleteDocumentthrows and assert that the handler logs the error and returns a 500 with the expected message.functions/sync-users-with-meilisearch/src/main.test.js (1)
1-80: User Meilisearch sync tests mirror stories suite wellThese tests mirror the stories sync coverage for the
userscollection and exercise the key success and 400 cases correctly. As with the stories tests, you might optionally add a case where Meilisearch operations throw to assert the 500-path behavior and logging, but the current coverage is already solid.functions/send-otp/src/main.test.js (1)
1-39: Happy-path OTP flow is covered; consider adding validation/error testsThis test confirms that the handler calls the mail and Appwrite services with the expected arguments and returns
{ message: 'mail sent' }, which is good. To harden the function, consider adding tests for (a) missing/invalid request fields and (b) failures from the mail or Appwrite layers, asserting that the catch block logs the error and returns the expected error response.functions/sync-stories-with-meilisearch/src/utils.js (1)
8-20: Consider consolidating duplicate utility functions.The
throwIfMissingfunction is duplicated across multiple modules (match-maker, send-otp, sync-all-documents-with-meilisearch, sync-users-with-meilisearch, and this module). This violates the DRY principle and makes maintenance harder.Consider creating a shared utilities package or module that all functions can import from:
// functions/shared/utils.js const throwIfMissing = (obj, keys) => { const missing = []; for (let key of keys) { if (!(key in obj) || !obj[key]) { missing.push(key); } } if (missing.length > 0) { throw new Error(`Missing required fields: ${missing.join(', ')}`); } }; module.exports = { throwIfMissing };Then import from the shared location:
const { throwIfMissing } = require('../shared/utils.js');functions/verify-otp/src/main.test.js (1)
54-94: Consider adding tests for OTP mismatch scenario.The current tests cover success and database error cases, but don't test the scenario where the user-provided OTP doesn't match the stored OTP. This is a common failure case in OTP verification.
Add a test case:
test("fail — OTP mismatch", async () => { mockGetDocument.mockResolvedValue({ otp: "1234" }); // Different from userOTP "5555" await fn({ req: mockReq, res: mockRes, log: mockLog, error: mockError }); expect(mockCreateDocument).not.toHaveBeenCalled(); expect(mockRes.json).toHaveBeenCalledWith( { message: "Invalid OTP" }, 400 ); });functions/verify-otp/src/main.js (1)
1-55: Handler flow looks solid; consider tightening env and input validationThe CommonJS conversion and overall control flow (env check → fetch OTP doc → write verification doc → respond) look good. Two optional improvements you might consider:
- Add
APPWRITE_FUNCTION_PROJECT_ID(and possiblyAPPWRITE_ENDPOINTif you don’t always want the default) to thethrowIfMissinglist so misconfiguration fails fast instead of at the Appwrite call site.- If you want clearer client semantics on bad requests, you could use
throwIfMissingon the parsed body to return a 4xx whenotpID,userOTP, orverify_IDare missing, instead of letting this fall through to a 500 from the Appwrite SDK.These are non-blocking polish items; current behavior is consistent and testable.
functions/sync-all-documents-with-meilisearch/src/main.js (1)
1-84: Sync logic is correct; consider DRY-ing pagination and expanding env checksThe Appwrite→MeiliSearch sync loops for stories and users are well-structured and use cursor pagination appropriately; the CommonJS migration also looks fine.
Two non-blocking improvements to consider:
- Env validation: you rely on
APPWRITE_ENDPOINT,APPWRITE_FUNCTION_PROJECT_ID, andAPPWRITE_API_KEYwhen building the Appwrite client but only validate the MeiliSearch envs. For consistency with other functions in this PR, you could add those Appwrite vars to thethrowIfMissinglist so misconfigurations fail fast.- Duplication: the stories and users sync blocks are nearly identical. Extracting a small helper (e.g.,
syncCollection({db, index, databaseId, collectionId, log})) would reduce duplication and keep future changes (like tweaking limits or logging) in one place.Both are refactors rather than required fixes.
functions/join-room/src/main.js (1)
1-30: Avoid doubleJSON.parseonreq.bodyLogic and status codes (400 for invalid/missing fields, 500 for internal errors) look good. You can simplify and avoid parsing twice by parsing once and reusing the payload:
- try { - throwIfMissing(JSON.parse(req.body), ["roomName", "uid"]); - } catch (err) { - return res.json({ msg: err.message }, 400); - } + let payload; + try { + payload = JSON.parse(req.body); + throwIfMissing(payload, ["roomName", "uid"]); + } catch (err) { + return res.json({ msg: err.message }, 400); + } ... - const { roomName, uid: userId } = JSON.parse(req.body); + const { roomName, uid: userId } = payload;This keeps behavior the same while reducing work and chance of divergence.
functions/sync-stories-with-meilisearch/src/main.js (1)
1-56: Add an explicit response for unsupported Appwrite event typesThe CommonJS migration and added guards (env validation, 400 when there’s no Appwrite trigger, 400 when
$idis missing) are good improvements.Within the
x-appwrite-triggerbranch, ifx-appwrite-eventdoesn’t contain.delete,.create, or.update, the function currently exits thetryblock without returning a response, which may leave the request hanging depending on the runtime.Consider adding a final
elseto return a 4xx or 2xx “no-op” response for unhandled event types, e.g.:if (eventType.includes('.delete')) { ... } else if (eventType.includes('.create') || eventType.includes('.update')) { ... } else { return res.text(`Event type ${eventType} not handled`, 400); }This makes behavior explicit and future-proof if new event variants appear.
functions/send-otp/src/main.js (1)
1-36: Solid wiring; consider explicit error status and fuller env validationThe CommonJS conversion and integration with
AppwriteServiceandMailServicelook good, and the OTP generation + persistence flow is straightforward.Two small improvements you might want:
- Return a failure status on exceptions: right now the catch block calls
res.json({ message: \Error: ${String(e)}` })` without a status, which likely defaults to 200. To let clients detect failures reliably, you can pass a 5xx:- return res.json({ message: `Error: ${String(e)}` }); + return res.json({ message: `Error: ${String(e)}` }, 500);
- Env validation parity: if
AppwriteServicedepends on additional env vars (e.g.,APPWRITE_FUNCTION_PROJECT_ID,APPWRITE_ENDPOINT), consider adding them to thethrowIfMissinglist so misconfigurations fail fast.Both are behavioral tweaks, not blockers.
functions/database-cleaner/src/appwrite.js (2)
49-72: Batch deletion loop for expired active pairs is reasonable; consider small clarity/safety tweaksThe pattern of querying expired pairs with
Query.lessThan("$createdAt", getExpiryDate()), deleting the current batch in parallel viaPromise.all, and looping untiltotal === 0is a reasonable approach.Two optional refinements you might consider:
- Initialize
donetofalsefor readability since its initial value is overwritten inside the loop anyway.- Add a defensive iteration cap or an extra break condition (e.g., if
activePairDocs.documents.length === 0whiletotal > 0) to avoid a potential infinite loop if the SDK semantics or query behavior ever change.
74-100: OTP cleanup logic works but may delete non-expired/future records; tighten predicate if neededThe loop structure mirrors
cleanActivePairsCollectionand looks fine. The main semantic question is the predicate:const currentDate = new Date().toDateString(); const queries = [Query.notEqual("date", currentDate), Query.limit(100)];
Query.notEqual("date", currentDate)will match all documents whosedatefield is not today, including any future‑dated OTPs or records without adateexactly equal totoDateString()format. If you ever pre‑create OTPs for future days or store more precise timestamps, you may want a stricter condition (e.g., a<comparison against a normalized cutoff) to avoid over‑deleting.If upstream guarantees that all non‑today OTPs are safe to remove, this is fine as is; otherwise, consider aligning this logic with the
getExpiryDate()helper or switching to a less‑than comparison on a timestamp field.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (14)
functions/create-room/package-lock.jsonis excluded by!**/package-lock.jsonfunctions/database-cleaner/package-lock.jsonis excluded by!**/package-lock.jsonfunctions/delete-room/package-lock.jsonis excluded by!**/package-lock.jsonfunctions/join-room/package-lock.jsonis excluded by!**/package-lock.jsonfunctions/match-maker/package-lock.jsonis excluded by!**/package-lock.jsonfunctions/send-otp/package-lock.jsonis excluded by!**/package-lock.jsonfunctions/start-friend-call/package-lock.jsonis excluded by!**/package-lock.jsonfunctions/sync-all-documents-with-meilisearch/package-lock.jsonis excluded by!**/package-lock.jsonfunctions/sync-stories-with-meilisearch/package-lock.jsonis excluded by!**/package-lock.jsonfunctions/sync-users-with-meilisearch/package-lock.jsonis excluded by!**/package-lock.jsonfunctions/upcomingRoom-Message-Notification/package-lock.jsonis excluded by!**/package-lock.jsonfunctions/upcomingRoom-isTime-checker/package-lock.jsonis excluded by!**/package-lock.jsonfunctions/verify-email/package-lock.jsonis excluded by!**/package-lock.jsonfunctions/verify-otp/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (50)
functions/create-room/package.json(2 hunks)functions/create-room/src/appwrite.js(1 hunks)functions/create-room/src/appwrite.test.js(1 hunks)functions/create-room/src/livekit.js(1 hunks)functions/create-room/src/main.js(1 hunks)functions/database-cleaner/package.json(1 hunks)functions/database-cleaner/src/appwrite.js(1 hunks)functions/database-cleaner/src/appwrite.test.js(1 hunks)functions/database-cleaner/src/utils.js(1 hunks)functions/delete-room/package.json(1 hunks)functions/delete-room/src/main.js(1 hunks)functions/delete-room/src/main.test.js(1 hunks)functions/join-room/package.json(1 hunks)functions/join-room/src/main.js(1 hunks)functions/join-room/src/main.test.js(1 hunks)functions/match-maker/package.json(1 hunks)functions/match-maker/src/main.js(1 hunks)functions/match-maker/src/main.test.js(1 hunks)functions/match-maker/src/utils.js(1 hunks)functions/send-otp/package.json(1 hunks)functions/send-otp/src/appwrite.js(1 hunks)functions/send-otp/src/mail.js(1 hunks)functions/send-otp/src/main.js(1 hunks)functions/send-otp/src/main.test.js(1 hunks)functions/send-otp/src/utils.js(1 hunks)functions/start-friend-call/package.json(1 hunks)functions/start-friend-call/src/main.js(2 hunks)functions/sync-all-documents-with-meilisearch/package.json(1 hunks)functions/sync-all-documents-with-meilisearch/src/main.js(4 hunks)functions/sync-all-documents-with-meilisearch/src/main.test.js(1 hunks)functions/sync-all-documents-with-meilisearch/src/utils.js(2 hunks)functions/sync-stories-with-meilisearch/package.json(1 hunks)functions/sync-stories-with-meilisearch/src/main.js(3 hunks)functions/sync-stories-with-meilisearch/src/main.test.js(1 hunks)functions/sync-stories-with-meilisearch/src/utils.js(2 hunks)functions/sync-users-with-meilisearch/package.json(1 hunks)functions/sync-users-with-meilisearch/src/main.js(3 hunks)functions/sync-users-with-meilisearch/src/main.test.js(1 hunks)functions/sync-users-with-meilisearch/src/utils.js(2 hunks)functions/upcomingRoom-Message-Notification/package.json(1 hunks)functions/upcomingRoom-Message-Notification/src/main.test.js(1 hunks)functions/upcomingRoom-isTime-checker/package.json(1 hunks)functions/upcomingRoom-isTime-checker/src/main.test.js(1 hunks)functions/verify-email/package.json(1 hunks)functions/verify-email/src/main.js(1 hunks)functions/verify-email/src/main.test.js(1 hunks)functions/verify-otp/package.json(1 hunks)functions/verify-otp/src/main.js(1 hunks)functions/verify-otp/src/main.test.js(1 hunks)init.sh(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (29)
functions/create-room/src/appwrite.test.js (1)
functions/create-room/src/appwrite.js (1)
require(1-1)
functions/create-room/src/livekit.js (1)
functions/create-room/src/main.js (1)
accessToken(50-50)
functions/sync-users-with-meilisearch/src/main.js (1)
functions/sync-users-with-meilisearch/src/utils.js (1)
throwIfMissing(8-18)
functions/sync-users-with-meilisearch/src/utils.js (4)
functions/match-maker/src/utils.js (1)
throwIfMissing(3-13)functions/send-otp/src/utils.js (1)
throwIfMissing(1-11)functions/sync-all-documents-with-meilisearch/src/utils.js (1)
throwIfMissing(8-18)functions/sync-stories-with-meilisearch/src/utils.js (1)
throwIfMissing(8-18)
functions/verify-otp/src/main.test.js (1)
functions/verify-otp/src/main.js (2)
require(1-1)require(2-2)
functions/sync-stories-with-meilisearch/src/utils.js (4)
functions/match-maker/src/utils.js (1)
throwIfMissing(3-13)functions/send-otp/src/utils.js (1)
throwIfMissing(1-11)functions/sync-all-documents-with-meilisearch/src/utils.js (1)
throwIfMissing(8-18)functions/sync-users-with-meilisearch/src/utils.js (1)
throwIfMissing(8-18)
functions/sync-users-with-meilisearch/src/main.test.js (1)
functions/sync-users-with-meilisearch/src/main.js (2)
require(1-1)require(2-2)
functions/database-cleaner/src/appwrite.test.js (1)
functions/database-cleaner/src/appwrite.js (2)
require(1-1)require(3-3)
functions/send-otp/src/main.test.js (4)
functions/send-otp/src/main.js (4)
sendOtp(5-34)require(1-1)MailService(3-3)AppwriteService(2-2)functions/send-otp/src/appwrite.js (1)
require(1-1)functions/send-otp/src/mail.js (1)
require(1-1)functions/send-otp/src/utils.js (1)
throwIfMissing(1-11)
functions/upcomingRoom-Message-Notification/src/main.test.js (2)
functions/sync-stories-with-meilisearch/src/main.test.js (1)
req(16-16)functions/sync-users-with-meilisearch/src/main.test.js (1)
req(16-16)
functions/sync-all-documents-with-meilisearch/src/main.test.js (1)
functions/sync-all-documents-with-meilisearch/src/main.js (3)
require(1-1)require(2-2)require(3-3)
functions/verify-email/src/main.test.js (1)
functions/verify-email/src/main.js (3)
require(1-1)require(2-2)JSON(16-16)
functions/match-maker/src/main.test.js (1)
functions/match-maker/src/main.js (2)
require(1-1)require(2-2)
functions/join-room/src/main.test.js (1)
functions/join-room/src/main.js (2)
require(1-1)require(2-2)
functions/upcomingRoom-isTime-checker/src/main.test.js (1)
functions/upcomingRoom-isTime-checker/src/main.js (1)
now(41-41)
functions/send-otp/src/appwrite.js (3)
functions/send-otp/src/main.js (3)
require(1-1)AppwriteService(2-2)otp(21-21)functions/sync-all-documents-with-meilisearch/src/main.js (4)
require(1-1)require(2-2)require(3-3)client(11-14)functions/database-cleaner/src/appwrite.js (2)
require(1-1)require(3-3)
functions/sync-stories-with-meilisearch/src/main.test.js (1)
functions/sync-stories-with-meilisearch/src/main.js (2)
require(1-1)require(2-2)
functions/send-otp/src/main.js (3)
functions/send-otp/src/appwrite.js (1)
require(1-1)functions/send-otp/src/mail.js (1)
require(1-1)functions/send-otp/src/utils.js (1)
throwIfMissing(1-11)
functions/verify-email/src/main.js (2)
functions/verify-email/src/main.test.js (1)
require(2-2)functions/verify-email/src/utils.js (2)
throwIfMissing(1-11)throwIfMissing(1-11)
functions/match-maker/src/main.js (1)
functions/match-maker/src/utils.js (1)
throwIfMissing(3-13)
functions/create-room/src/appwrite.js (1)
functions/create-room/src/appwrite.test.js (1)
AppwriteService(19-19)
functions/send-otp/src/mail.js (1)
functions/send-otp/src/main.js (4)
require(1-1)MailService(3-3)recipientEmail(19-19)otp(21-21)
functions/verify-otp/src/main.js (2)
functions/verify-otp/src/main.test.js (1)
require(2-2)functions/verify-otp/src/utils.js (2)
throwIfMissing(1-11)throwIfMissing(1-11)
functions/delete-room/src/main.js (2)
functions/delete-room/src/utils.js (2)
throwIfMissing(1-11)throwIfMissing(1-11)functions/livekit-webhook/src/main.js (1)
appwriteRoomDocId(30-30)
functions/create-room/src/main.js (1)
functions/create-room/src/utils.js (2)
throwIfMissing(1-11)throwIfMissing(1-11)
functions/send-otp/src/utils.js (4)
functions/match-maker/src/utils.js (2)
throwIfMissing(3-13)missing(4-4)functions/sync-all-documents-with-meilisearch/src/utils.js (2)
throwIfMissing(8-18)missing(9-9)functions/sync-stories-with-meilisearch/src/utils.js (2)
throwIfMissing(8-18)missing(9-9)functions/sync-users-with-meilisearch/src/utils.js (2)
throwIfMissing(8-18)missing(9-9)
functions/match-maker/src/utils.js (4)
functions/send-otp/src/utils.js (2)
throwIfMissing(1-11)missing(2-2)functions/sync-all-documents-with-meilisearch/src/utils.js (2)
throwIfMissing(8-18)missing(9-9)functions/sync-stories-with-meilisearch/src/utils.js (2)
throwIfMissing(8-18)missing(9-9)functions/sync-users-with-meilisearch/src/utils.js (2)
throwIfMissing(8-18)missing(9-9)
functions/database-cleaner/src/appwrite.js (1)
functions/database-cleaner/src/utils.js (1)
getExpiryDate(13-18)
functions/sync-all-documents-with-meilisearch/src/main.js (3)
functions/match-maker/src/main.js (2)
require(1-1)require(2-2)functions/sync-stories-with-meilisearch/src/main.js (3)
require(1-1)require(2-2)meilisearch(11-14)functions/sync-users-with-meilisearch/src/main.js (3)
require(1-1)require(2-2)meilisearch(11-14)
🔇 Additional comments (39)
functions/upcomingRoom-Message-Notification/package.json (1)
13-15: > Likely an incorrect or invalid review comment.functions/upcomingRoom-isTime-checker/package.json (1)
7-7: LGTM!The test script correctly configures Jest for running unit tests.
functions/upcomingRoom-Message-Notification/src/main.test.js (5)
1-33: LGTM!The mock setup correctly isolates external dependencies (Firebase Admin, Firebase Messaging, and Appwrite) and uses a virtual mock for the service account configuration file.
35-57: LGTM!The test setup properly initializes environment variables, resets mocks between tests, and configures request/response stubs.
59-85: LGTM!This test validates the happy path: token aggregation from both subscription documents and creator tokens, correct payload construction, and successful response handling.
87-102: LGTM!This test correctly validates the behavior when Firebase returns failures, ensuring the function still responds successfully while logging the failure.
104-117: LGTM!This test properly covers the edge case of missing tokens, ensuring graceful handling with an empty tokens array.
functions/upcomingRoom-isTime-checker/src/main.test.js (6)
1-19: LGTM!The mock setup correctly isolates the Appwrite client and database operations for unit testing.
21-34: LGTM!The test setup properly configures environment variables and resets mocks between tests.
36-60: LGTM!This test correctly validates the primary logic: updating
isTimetotruewhen the scheduled time is within the ±5 minute window and the flag is currentlyfalse.
62-80: LGTM!This test correctly validates idempotency: when
isTimeis alreadytrue, no update should be performed.
82-100: LGTM!This test properly validates the time boundary logic: rooms scheduled outside the ±5 minute window should not be updated.
102-126: LGTM!Excellent test coverage of multi-document processing. This validates that only rooms meeting both conditions (within time window AND
isTimeisfalse) are updated, while others are correctly skipped.functions/create-room/src/livekit.js (1)
4-41: LivekitService token and room creation logic look consistent.Constructor wiring, token generation (grants), and
createRoomusage are all straightforward and match expected LiveKit patterns; no issues from this change.functions/create-room/package.json (1)
6-8: Jest setup is correct; module type is properly aligned with source files.All source files in
functions/create-room/srcuse ESM syntax consistently (imports/exports), so"type": "module"is the correct configuration and must remain. No CommonJS patterns were found in any of the four source files (main.js, appwrite.js, livekit.js, utils.js). The Jest setup is sound.functions/create-room/src/main.js (1)
5-62: LGTM! Formatting improvements applied.The changes are formatting-only with no functional modifications. The logic, error handling, and control flow remain unchanged.
functions/send-otp/src/utils.js (1)
1-13: LGTM! Clean ES module to CommonJS conversion.The conversion properly maintains the function logic while updating the export pattern to CommonJS. The
throwIfMissingutility function behavior is preserved.functions/send-otp/src/appwrite.js (1)
1-26: ES module to CommonJS conversion is otherwise correct.The migration from ES module syntax to CommonJS is properly implemented. The class structure, constructor initialization, and
createOtpDocumentmethod logic are preserved.functions/delete-room/package.json (1)
6-6: No action needed. The"type": "module"declaration is correct—the source files (main.js,utils.js) use only ES module syntax withimportandexportstatements. No CommonJS patterns were detected.functions/sync-all-documents-with-meilisearch/src/main.test.js (3)
3-27: LGTM: Mock setup is well-structured.The mocks for
node-appwriteandmeilisearchare properly configured with Jest functions and expose the necessary methods for testing.
32-46: LGTM: Test setup follows best practices.Environment variables are properly configured and mocks are reset before each test, ensuring test isolation.
48-72: LGTM: Test cases cover key scenarios.The tests validate:
- Pagination handling with multiple document pages
- Empty document handling
- Proper response status and message
Both test cases correctly assert the expected number of
addDocumentscalls based on the paginated responses.functions/match-maker/src/main.test.js (3)
5-34: LGTM: Comprehensive mock setup.The mocks cover all necessary Appwrite client methods and utilities for testing the pairing functionality.
40-76: LGTM: Test setup aligns with implementation.The setup correctly mirrors the event structure and database behavior, including the fix for extracting the document ID from the event path.
99-106: LGTM: Error handling test is correct.The test properly validates that errors during pairing are logged with appropriate messages.
functions/verify-otp/src/main.test.js (3)
4-23: LGTM: Mock setup is well-structured.The mocks properly isolate the unit under test by mocking external dependencies (node-appwrite and utilities).
28-52: LGTM: Test setup follows best practices.The
beforeEachproperly resets mocks and configures all necessary test fixtures and environment variables.
54-69: Verify the success response message.The test expects
{ message: "null" }as the success response, which is unusual. Typically, success messages would be something like"OTP verified successfully"or"Success".Please verify that returning
{ message: "null" }is the intended behavior in the actual implementation. If this is a placeholder or incorrect, update both the implementation and test to use a meaningful success message.functions/database-cleaner/src/appwrite.test.js (4)
1-22: LGTM: Well-designed mock setup.The mocks properly simulate the Appwrite SDK behavior with appropriate return values for Query methods.
34-58: LGTM: Comprehensive test coverage for doesRoomExist.The test validates all three scenarios: success, expected 404 (returns false), and unexpected errors (rethrows). This is thorough and well-structured.
60-90: LGTM: Correct test logic for participant cleanup.The test properly validates that only participants associated with non-existent rooms are deleted, and uses
process.nextTickto handle asynchronous deletion operations.
92-125: LGTM: Thorough test for OTP cleanup with pagination.The test correctly validates pagination handling and ensures all old OTPs are deleted with the correct database/collection IDs.
functions/database-cleaner/src/utils.js (1)
13-18: LGTM: getExpiryDate logic is correct.The function properly calculates the expiry date based on the retention period with a sensible default of 1 day.
functions/match-maker/src/utils.js (1)
3-15: LGTM: Conversion to CommonJS is correct.The export mechanism has been properly updated to use
module.exports. Note that this function is duplicated across multiple modules (see comment onfunctions/sync-stories-with-meilisearch/src/utils.jsfor consolidation recommendation).functions/sync-all-documents-with-meilisearch/src/utils.js (1)
8-20: CommonJS migration forthrowIfMissinglooks correctThe helper implementation is sound and exporting it via
module.exports = { throwIfMissing }is consistent with the rest of the PR. No functional issues spotted here.functions/send-otp/src/mail.js (1)
1-25: MailService CommonJS conversion and behavior look correctThe nodemailer transporter setup and
sendMailimplementation are unchanged in behavior, and exporting the class viamodule.exports = MailServicealigns with how it’s consumed insend-otp/src/main.js. Error propagation is handled by the caller’s try/catch, so this class can stay focused on sending. No issues from this change.functions/database-cleaner/src/appwrite.js (3)
1-16: CommonJS imports and Appwrite client initialization look consistentThe switch to
require("node-appwrite")and wiring ofClient/Databasesin the constructor using env-driven endpoint/project/key looks straightforward and matches typical Appwrite usage. Just ensure tests cover behavior when required env vars are missing or malformed.
18-30: Confirm error shape (err.code) from Appwrite SDK indoesRoomExistLogic of treating any successful
getDocumentas “room exists” and only swallowing 404s while rethrowing other errors is sound. Please double‑check thatDatabases.getDocumentinnode-appwrite@17throws errors with an HTTP status onerr.code(and not e.g.err.statusCodeor nested undererr.response), otherwise the 404 check may never trigger.
103-103: CommonJS export ofAppwriteServiceis straightforwardExporting the class via
module.exports = AppwriteService;aligns with the new CommonJS import style and should make requiring this module from tests and other functions simple.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
functions/database-cleaner/src/appwrite.js(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
functions/database-cleaner/src/appwrite.js (1)
functions/database-cleaner/src/utils.js (1)
getExpiryDate(13-18)
🔇 Additional comments (3)
functions/database-cleaner/src/appwrite.js (3)
15-27: LGTM!The existence check pattern with 404-specific handling is correct and clean. Rethrowing non-404 errors ensures unexpected issues are properly surfaced.
57-83: LGTM!The batch deletion logic is correct. Since all fetched documents are deleted unconditionally, the pagination advances naturally and terminates when no matching documents remain.
85-92: Date format concern is not applicable—both creation and cleanup use the same format.OTP documents are created with
datefield usingnew Date().toDateString()(infunctions/send-otp/src/main.jsline 26), andclearOldOTPs()queries using the same format. The comparison will work correctly.However, using
toDateString()is locale-dependent and fragile. Consider using a consistent, locale-independent format like ISO date (YYYY-MM-DD):async clearOldOTPs() { let done = false; - const currentDate = new Date().toDateString(); + const currentDate = new Date().toISOString().split('T')[0];This requires updating the OTP creation in
functions/send-otp/src/main.jsto match:- const currentDate = new Date().toDateString(); + const currentDate = new Date().toISOString().split('T')[0];Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
functions/database-cleaner/src/appwrite.js (1)
31-57: Remove the duplicate method definition.This first definition of
cleanParticipantsCollectionis dead code—JavaScript will only use the second definition at lines 59-94. Additionally, this implementation contains the infinite loop bug (usingparticipantDocs.totalat line 55) that was flagged in previous reviews.Delete this entire method:
- async cleanParticipantsCollection() { - let done = false; - const queries = [Query.limit(25)]; - - while (!done) { - const participantDocs = await this.databases.listDocuments( - process.env.MASTER_DATABASE_ID, - process.env.PARTICIPANTS_COLLECTION_ID, - queries - ); - - await Promise.all( - participantDocs.documents.map(async (participantDoc) => { - const exists = await this.doesRoomExist(participantDoc.roomId); - if (!exists) { - await this.databases.deleteDocument( - process.env.MASTER_DATABASE_ID, - process.env.PARTICIPANTS_COLLECTION_ID, - participantDoc.$id - ); - } - }) - ); - - done = participantDocs.total === 0; - } - } -
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
functions/database-cleaner/src/appwrite.js(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
functions/database-cleaner/src/appwrite.js (4)
functions/send-otp/src/appwrite.js (1)
require(1-1)functions/send-otp/src/main.js (4)
require(1-1)AppwriteService(2-2)currentDate(24-24)otp(21-21)functions/database-cleaner/src/appwrite.test.js (1)
AppwriteService(24-24)functions/create-room/src/appwrite.test.js (1)
AppwriteService(19-19)
🪛 Biome (2.1.2)
functions/database-cleaner/src/appwrite.js
[error] 59-94: Duplicate class member name "cleanParticipantsCollection"
(lint/suspicious/noDuplicateClassMembers)
🔇 Additional comments (5)
functions/database-cleaner/src/appwrite.js (5)
1-2: LGTM: CommonJS conversion is correct.The imports are properly converted to CommonJS and align with the module system changes across the codebase.
5-15: LGTM: Constructor implementation is sound.The Appwrite client initialization is correct with proper environment variable handling and fallback endpoint.
17-29: LGTM: Proper error handling for room existence check.The method correctly distinguishes between non-existent rooms (404) and actual errors, which is essential for the cleanup logic.
59-94: LGTM: Cursor-based pagination correctly addresses the infinite loop issue.This implementation properly fixes the issue raised in previous reviews by using
Query.cursorAfterto advance through results and checkingdocuments.length === 0to terminate. The cursor-based approach ensures progress even when no deletions occur in a batch.
124-124: LGTM: CommonJS export is correct.The module export aligns with the CommonJS conversion and matches how the module is imported in test files.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
functions/database-cleaner/src/appwrite.js (1)
2-2: Remove unused import.
getExpiryDateis imported but never used in this file.-const { getExpiryDate } = require("./utils.js");functions/match-maker/src/main.test.js (3)
23-28: Consider returning mock query values from Query methods.The Query mock methods (notEqual, equal, orderAsc, limit) currently return undefined. If the actual code chains these calls or passes their return values to listDocuments, the tests might not accurately reflect real SDK behavior.
Apply this diff to make Query methods return mock values:
Query: { - notEqual: jest.fn(), - equal: jest.fn(), - orderAsc: jest.fn(), - limit: jest.fn(), + notEqual: jest.fn((field, value) => `notEqual("${field}", "${value}")`), + equal: jest.fn((field, value) => `equal("${field}", "${value}")`), + orderAsc: jest.fn((field) => `orderAsc("${field}")`), + limit: jest.fn((count) => `limit(${count})`), },
108-128: Enhance test by verifying deleteDocument arguments.While the test confirms deleteDocument is called twice, it doesn't verify that the correct document IDs are being deleted. This could miss bugs where the wrong documents are deleted.
Apply this diff to add argument verification:
expect(mockDb.deleteDocument).toHaveBeenCalledTimes(2); + expect(mockDb.deleteDocument).toHaveBeenNthCalledWith( + 1, + "db", + "requests", + "123" + ); + expect(mockDb.deleteDocument).toHaveBeenNthCalledWith( + 2, + "db", + "requests", + "456" + ); expect(res.json).toHaveBeenCalledWith(
78-128: Consider adding tests for additional edge cases.The current test suite covers basic scenarios but could benefit from testing more edge cases that could occur in production.
Consider adding test cases for:
Database operation failures:
- getDocument throws an error
- listDocuments throws an error
- deleteDocument throws an error
Malformed input:
- Missing or malformed x-appwrite-event header
- Event header with unexpected format
Environment validation:
- Missing required environment variables
Example test:
it("should handle getDocument failure gracefully", async () => { mockDb.getDocument.mockRejectedValueOnce(new Error("Database error")); await sendPairRequest({ req, res, log, error }); expect(error).toHaveBeenCalledWith(expect.stringContaining("Database error")); }); it("should handle missing event header", async () => { req.headers = {}; await sendPairRequest({ req, res, log, error }); expect(error).toHaveBeenCalled(); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
functions/database-cleaner/src/appwrite.js(1 hunks)functions/match-maker/src/main.test.js(1 hunks)functions/send-otp/src/appwrite.js(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
functions/send-otp/src/appwrite.js (2)
functions/database-cleaner/src/appwrite.js (2)
require(1-1)require(2-2)functions/sync-all-documents-with-meilisearch/src/main.js (4)
require(1-1)require(2-2)require(3-3)client(11-14)
functions/match-maker/src/main.test.js (1)
functions/match-maker/src/main.js (2)
require(1-1)require(2-2)
functions/database-cleaner/src/appwrite.js (1)
functions/database-cleaner/src/appwrite.test.js (1)
AppwriteService(24-24)
🪛 Biome (2.1.2)
functions/database-cleaner/src/appwrite.js
[error] 59-94: Duplicate class member name "cleanParticipantsCollection"
(lint/suspicious/noDuplicateClassMembers)
🔇 Additional comments (8)
functions/send-otp/src/appwrite.js (2)
4-10: Verify: Missing API key configuration.The
clientis not configured with.setKey(process.env.APPWRITE_API_KEY), unlike other services in this PR (e.g.,functions/database-cleaner/src/appwrite.jsandfunctions/sync-all-documents-with-meilisearch/src/main.js). Without an API key, the client may lack permissions to create OTP documents.If this function runs within an Appwrite function context where permissions are inherited, this is fine. Otherwise, add the API key:
const client = new Client() .setEndpoint(process.env.APPWRITE_ENDPOINT ?? "https://cloud.appwrite.io/v1") - .setProject(process.env.APPWRITE_FUNCTION_PROJECT_ID); + .setProject(process.env.APPWRITE_FUNCTION_PROJECT_ID) + .setKey(process.env.APPWRITE_API_KEY);
12-25: LGTM!The
createOtpDocumentmethod logic is correct, and the CommonJS export is consistent with the rest of the codebase changes.functions/database-cleaner/src/appwrite.js (5)
5-15: LGTM!The constructor properly initializes the Appwrite client with endpoint, project ID, and API key.
17-29: LGTM!The
doesRoomExistmethod correctly returnstrueon success,falseon 404, and rethrows other errors. This is proper error handling.
59-94: Cursor-based pagination correctly implemented.This implementation properly uses cursor-based pagination to avoid the infinite loop issue. The loop breaks when no documents are returned, and the cursor advances after each batch.
96-128: LGTM!The
clearOldOTPsmethod correctly implements cursor-based pagination, addressing the infinite loop issue from past reviews.
131-131: LGTM!CommonJS export is consistent with the rest of the codebase changes.
functions/match-maker/src/main.test.js (1)
99-106: The test filefunctions/match-maker/src/main.test.jsdoes not exist in the repository. No test files are present in thefunctions/match-makerdirectory, and the specific test case "should handle errors during pairing" cannot be found anywhere in the codebase. The review comment references non-existent code and therefore cannot be verified or acted upon.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
functions/match-maker/src/main.test.js (1)
32-34: The throwIfMissing mock still bypasses validation logic.This was flagged in a previous review as a major issue. Mocking throwIfMissing as a no-op means validation paths remain untested. Consider either using the real implementation or adding dedicated tests that verify throwIfMissing behavior.
🧹 Nitpick comments (2)
functions/match-maker/src/main.test.js (1)
97-104: Consider more flexible error assertions.The test expects exact error messages in a specific order. If the implementation adds additional logging or changes message formats, this test will become brittle.
Consider using more flexible matchers:
- expect(error).toHaveBeenCalledWith("That request is already paired: "); - expect(error).toHaveBeenCalledWith("Error: Pairing failed"); + expect(error).toHaveBeenCalledWith(expect.stringContaining("already paired")); + expect(error).toHaveBeenCalledWith(expect.stringContaining("Pairing failed"));functions/database-cleaner/src/appwrite.js (1)
31-66: Cursor-based participant cleanup correctly implements pagination best practicesThe implementation uses
Query.limit(25)andQuery.cursorAfter(cursor)as documented for node-appwrite v17; this approach correctly avoids the earlier infinite-loop risk and guarantees termination. The batch size of 25 matches the SDK's default page size. Optionally extract the limit value to a named constant (e.g.,PARTICIPANT_BATCH_SIZE) for improved readability and easier future tuning.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
functions/database-cleaner/package.json(1 hunks)functions/database-cleaner/src/appwrite.js(1 hunks)functions/match-maker/src/main.test.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- functions/database-cleaner/package.json
🔇 Additional comments (5)
functions/match-maker/src/main.test.js (2)
77-95: Well-structured edge case tests.Both test cases properly validate early-exit scenarios with appropriate mock configurations and clear assertions.
106-126: Excellent addition of the successful pairing test.This test properly validates the happy path that was missing in previous reviews. It correctly verifies pair creation, request cleanup, and response formatting.
functions/database-cleaner/src/appwrite.js (3)
5-15: Constructor wiring looks solidClient/env wiring and
Databasesinitialization are straightforward and appropriate for this service class; no changes needed.
17-29: 404 handling and error propagation are appropriate
doesRoomExistcleanly maps “found vs 404” totrue/falseand lets all other errors bubble, which is a good balance between resilience and debuggability.
68-100: The code shown in this review comment does not match the actual implementation in the repository. The actualclearOldOTPs()method (lines 76-110) uses ado...whileloop withdone = oneDayOTPs.total === 0, not the cursor-based pagination shown in the review snippet. The review's claim that this "avoids the earlier total-based loop bug" is inaccurate—the current implementation still relies ontotal.Regarding date formatting: both OTP creation (send-otp/src/main.js line 26) and cleanup (database-cleaner/src/appwrite.js line 80) consistently use
new Date().toDateString(), so the format alignment is already correct and does not require a refactor.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (7)
functions/match-maker/src/main.test.js (5)
23-28: Consider returning mock query objects from Query methods.The Query mock methods currently return
undefinedby default. While this works for the current tests, it doesn't match Appwrite's actual behavior where Query methods return query objects. This could mask issues if the implementation code chains or uses these query objects.Consider updating the mock:
Query: { - notEqual: jest.fn(), - equal: jest.fn(), - orderAsc: jest.fn(), - limit: jest.fn(), + notEqual: jest.fn(() => ({ method: 'notEqual' })), + equal: jest.fn(() => ({ method: 'equal' })), + orderAsc: jest.fn(() => ({ method: 'orderAsc' })), + limit: jest.fn(() => ({ method: 'limit' })), },
43-78: Add explicit mock cleanup for better test isolation.While Jest typically handles mock cleanup automatically, adding explicit
jest.clearAllMocks()in anafterEachblock ensures complete test isolation and is considered best practice.Add after the
beforeEachblock:afterEach(() => { jest.clearAllMocks(); });
100-107: Verify the response sent during error handling.The test checks that errors are logged but doesn't verify what response is sent to the client. Error paths should also validate the user-facing response to ensure proper error handling end-to-end.
Add an assertion to verify the response:
expect(error).toHaveBeenCalledWith("That request is already paired: "); expect(error).toHaveBeenCalledWith("Error: Pairing failed"); + expect(res.json).toHaveBeenCalledWith( + expect.objectContaining({ + ok: false, + }) + );
109-129: Consider verifying log calls for better observability testing.The test validates the core pairing logic but doesn't verify that success messages are logged. Testing log calls ensures proper observability in production.
Add assertion for log calls:
expect(log).toHaveBeenCalledWith(expect.stringContaining("Pair created"));
39-130: Add test coverage for event header edge cases.The tests don't cover scenarios where the event header is missing, malformed, or contains an invalid document ID pattern. These edge cases could cause runtime errors or unexpected behavior in the document ID extraction logic.
Add test cases like:
it("should handle missing event header gracefully", async () => { req.headers = {}; await sendPairRequest({ req, res, log, error }); expect(error).toHaveBeenCalled(); expect(res.json).toHaveBeenCalledWith( expect.objectContaining({ ok: false }) ); }); it("should handle malformed event header gracefully", async () => { req.headers["x-appwrite-event"] = "invalid-format"; await sendPairRequest({ req, res, log, error }); expect(error).toHaveBeenCalled(); });functions/delete-room/src/main.test.js (1)
77-89: Verify error logging and expand test coverage.The test should verify that the
error()function was called when an exception occurs.Add this assertion:
expect(res.json).toHaveBeenCalledWith( { msg: "Room deletion failed" }, 500 ); + expect(error).toHaveBeenCalledWith(expect.stringContaining("DB ERROR"));Additionally, the test suite is missing critical test cases:
- Success path: Room deleted successfully (verify deleteDocument, listDocuments, participant deletions, LiveKit deleteRoom are called)
- 403 case: Non-admin user attempting deletion
- Participant cleanup: Verify all participants are deleted
Would you like me to help generate these additional test cases?
functions/delete-room/src/main.js (1)
59-71: Consider more targeted logging for participant cleanup.Logging the entire
participantColRefobject (line 64) could expose sensitive data or create large log entries. Consider logging just the count or document IDs.const participantColRef = await databases.listDocuments( process.env.MASTER_DATABASE_ID, process.env.PARTICIPANTS_COLLECTION_ID, [Query.equal("roomId", [appwriteRoomDocId])] ); - log(participantColRef); + log(`Deleting ${participantColRef.documents.length} participant(s)`); for (const participant of participantColRef.documents) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
functions/delete-room/package.json(1 hunks)functions/delete-room/src/main.js(1 hunks)functions/delete-room/src/main.test.js(1 hunks)functions/join-room/package.json(1 hunks)functions/match-maker/src/main.test.js(1 hunks)functions/sync-users-with-meilisearch/package.json(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- functions/delete-room/package.json
- functions/join-room/package.json
🧰 Additional context used
🧬 Code graph analysis (2)
functions/delete-room/src/main.test.js (1)
functions/delete-room/src/main.js (4)
require(1-1)require(2-2)require(3-3)JSON(39-39)
functions/delete-room/src/main.js (2)
functions/delete-room/src/utils.js (2)
throwIfMissing(1-11)throwIfMissing(1-11)functions/livekit-webhook/src/main.js (1)
appwriteRoomDocId(30-30)
🔇 Additional comments (4)
functions/sync-users-with-meilisearch/package.json (1)
1-19: ✅ Package configuration is correct.The previous critical issues have been properly resolved:
- "type": "module" declaration has been removed, aligning CommonJS source files with Node.js module resolution.
- Jest and @jest/globals are correctly placed in
devDependencies, keeping test tooling separate from production dependencies.The test script is properly configured to run Jest, and dependency versions are appropriate.
functions/match-maker/src/main.test.js (3)
32-37: LGTM!Good use of the actual
throwIfMissingimplementation. This ensures validation logic is properly tested rather than mocked away.
80-88: LGTM!Clean test that verifies the early-return path for non-random requests.
90-98: LGTM!Correctly validates the behavior when no matching requests are found in the queue.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
functions/database-cleaner/src/utils.js (1)
1-21: throwIfMissing semantics still diverge from other modules (0/false handling).This implementation now only treats
null,undefined, and""as missing while other modules still use a simple falsy check (if (!obj[key])), which treats0,false, andNaNas missing as well. That means the same logical “required field” may be accepted here (e.g.,0orfalse) but rejected elsewhere, which is fragile and hard to reason about.I recommend choosing one contract and applying it everywhere, ideally via a shared helper:
- If the app-wide rule is “any falsy value is missing”, update this to use
if (!value)instead of the explicit null/undefined/empty-string checks.- If the stricter rule here is desired (0/false allowed), update the other
throwIfMissingimplementations to match, and centralize the function in a common utility so it can’t drift again.functions/delete-room/src/main.js (1)
53-65: Critical: Participant deletion may be incomplete due to pagination.The
listDocumentscall at line 53 doesn't handle pagination, meaning it will only retrieve the first page of results (typically 25 documents by default in Appwrite). If a room has more participants than the page limit, the remaining participants won't be deleted, leaving orphaned records in the database.Consider implementing cursor-based pagination to ensure all participants are deleted:
- const participantColRef = await databases.listDocuments( - process.env.MASTER_DATABASE_ID, - process.env.PARTICIPANTS_COLLECTION_ID, - [Query.equal("roomId", [appwriteRoomDocId])] - ); - log(participantColRef); - for (const participant of participantColRef.documents) { - await databases.deleteDocument( - process.env.MASTER_DATABASE_ID, - process.env.PARTICIPANTS_COLLECTION_ID, - participant.$id - ); + let cursor = null; + let hasMore = true; + + while (hasMore) { + const queries = [Query.equal("roomId", [appwriteRoomDocId]), Query.limit(100)]; + if (cursor) { + queries.push(Query.cursorAfter(cursor)); + } + + const participantColRef = await databases.listDocuments( + process.env.MASTER_DATABASE_ID, + process.env.PARTICIPANTS_COLLECTION_ID, + queries + ); + + for (const participant of participantColRef.documents) { + await databases.deleteDocument( + process.env.MASTER_DATABASE_ID, + process.env.PARTICIPANTS_COLLECTION_ID, + participant.$id + ); + } + + hasMore = participantColRef.documents.length === 100; + if (hasMore) { + cursor = participantColRef.documents[participantColRef.documents.length - 1].$id; + } }
🧹 Nitpick comments (1)
functions/match-maker/src/main.js (1)
21-25: Add defensive validation for the extracted document ID.If the
x-appwrite-eventheader is missing or malformed,triggerEvent.split(".")will throw or produce an empty/invalid document ID, causing a runtime error on the subsequentgetDocumentcall.log(req.headers); const triggerEvent = req.headers["x-appwrite-event"]; + if (!triggerEvent) { + return res.json({ message: "Missing trigger event header" }); + } const parts = triggerEvent.split("."); const newRequestDocId = parts[parts.length - 1]; + if (!newRequestDocId) { + return res.json({ message: "Could not extract document ID from event" }); + } log(newRequestDocId);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
functions/database-cleaner/src/utils.js(1 hunks)functions/delete-room/package.json(1 hunks)functions/delete-room/src/main.js(1 hunks)functions/delete-room/src/main.test.js(1 hunks)functions/join-room/package.json(1 hunks)functions/match-maker/package.json(1 hunks)functions/match-maker/src/main.js(1 hunks)functions/send-otp/package.json(1 hunks)functions/start-friend-call/src/main.js(2 hunks)functions/sync-all-documents-with-meilisearch/package.json(2 hunks)functions/sync-stories-with-meilisearch/package.json(1 hunks)functions/verify-email/package.json(1 hunks)functions/verify-otp/package.json(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- functions/delete-room/src/main.test.js
- functions/send-otp/package.json
- functions/match-maker/package.json
- functions/join-room/package.json
- functions/sync-all-documents-with-meilisearch/package.json
🧰 Additional context used
🪛 Biome (2.1.2)
functions/start-friend-call/src/main.js
[error] 40-42: This code will never be reached ...
... because either this statement ...
... or this statement will return from the function beforehand
(lint/correctness/noUnreachable)
🔇 Additional comments (18)
functions/sync-stories-with-meilisearch/package.json (1)
1-19: ✅ Package.json configuration is correct.The previous critical issues have been properly addressed:
"type": "module"has been removed, allowing CommonJS modules to function correctly- Jest and
@jest/globalsare now correctly placed indevDependenciesinstead ofdependencies- Test script is properly configured with
"jest"The file is well-structured and ready for use.
functions/verify-otp/package.json (1)
1-19: Critical issue from previous review has been resolved. The"type": "module"declaration has been removed, allowing the CommonJSrequire()/module.exportspattern to work correctly. Jest ^30.2.0 is the current stable version and properly configured with the test script and devDependencies.functions/start-friend-call/src/main.js (2)
1-2: LGTM: CommonJS conversion.The conversion from ES modules to CommonJS is consistent with the broader changes in this PR.
23-38: Excellent fix: Proper error handling implemented.The try/catch block correctly addresses the previous critical issue where failures were masked as successes. Both paths now return appropriate status codes and messages.
functions/verify-email/package.json (1)
6-18: Jest dependencies correctly configured.The package.json properly places Jest and @jest/globals in
devDependencies, the test script is correctly configured, and the removal of"type": "module"aligns with CommonJS source files. Jest 30.2.0 is a valid release (September 28, 2025).functions/delete-room/package.json (1)
1-20: Configuration looks good!The package.json is properly configured with Jest dependencies correctly placed in devDependencies (addressing the previous review feedback). The removal of
"type": "module"aligns with the CommonJS migration in the main.js file.functions/delete-room/src/main.js (6)
1-3: CommonJS migration looks correct.The imports have been properly converted from ES module syntax to CommonJS
requirestatements, aligning with the package.json change that removed"type": "module".
5-14: Environment validation is thorough.The module export has been correctly converted to CommonJS, and all necessary environment variables are validated upfront, preventing runtime errors later.
16-29: Client initialization is correct.Both Appwrite and LiveKit clients are properly configured with the required credentials and endpoints.
31-45: Request handling and authorization look good.The duplicate JSON parsing issue has been resolved, and the admin validation properly prevents unauthorized users from deleting rooms.
59-65: Good fix for awaiting participant deletions!The change from
forEachwith async callbacks to afor...ofloop properly ensures all participant deletions are awaited before proceeding.
66-71: LiveKit cleanup and error handling are appropriate.The function properly cleans up the LiveKit room and provides centralized error handling with logging. The generic error message in the response is acceptable for security purposes.
functions/match-maker/src/main.js (6)
1-2: LGTM!CommonJS imports are correctly structured for the Appwrite SDK components and local utility.
4-10: LGTM!Fail-fast validation of required environment variables ensures clear error messages if configuration is missing.
12-19: LGTM!Appwrite client initialization follows the standard pattern with sensible endpoint fallback.
27-37: LGTM with a consideration.The document fetch and
isRandomcheck correctly implements early-exit logic. Note that if the document was already deleted (e.g., by a concurrent pairing),getDocumentwill throw an unhandled exception. Depending on your function runtime's error handling, you may want to wrap this in a try-catch for cleaner error messages.
39-52: LGTM!The query correctly filters candidates and the previous issue with iterating over
totalinstead ofdocuments.lengthhas been addressed. The FIFO ordering viaorderAsc("$createdAt")ensures fair matching.
97-99: LGTM!Appropriate fallback response when no pairing occurs.
1 participant
Added unit test cases for all functions
Summary by CodeRabbit
Tests
Bug Fixes
Improvements
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.