fix: catch unhandled promise rejections in logging middleware#291
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds a Jest test suite for the api_usage middleware that mocks logging, analytics, and Redis helpers; verifies listener registration, successful analytics capture, and error resilience. Also adjusts middleware error-logging to print error messages instead of raw error objects. ChangesAPI Usage Middleware Testing
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labelslevel:intermediate Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/public-api/src/__tests__/api_usage.test.js (1)
81-81: ⚡ Quick winConsider verifying the Redis key pattern and TTL arguments.
The test asserts that
incrWithTtlAtomicis called but doesn't verify the key pattern or TTL. Adding argument verification would catch potential regressions in the Redis key construction or TTL handling.Enhanced assertion
- expect(mockIncrWithTtlAtomic).toHaveBeenCalled(); + expect(mockIncrWithTtlAtomic).toHaveBeenCalledWith( + expect.anything(), // redis client + 'project:usage:req:count:test_project_id:2026-06-08', + 86400, // DEFAULT_DAILY_TTL_SECONDS + undefined // amount defaults to 1 + );🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/public-api/src/__tests__/api_usage.test.js` at line 81, The test currently only checks that mockIncrWithTtlAtomic was called; update the assertion to verify the Redis key pattern and TTL arguments for incrWithTtlAtomic (e.g., assert mockIncrWithTtlAtomic was called with a key string matching the expected pattern using expect.stringMatching(/.../) and the expected TTL value), referencing the mockIncrWithTtlAtomic call in the test so it fails if the key format or TTL changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@apps/public-api/src/__tests__/api_usage.test.js`:
- Line 81: The test currently only checks that mockIncrWithTtlAtomic was called;
update the assertion to verify the Redis key pattern and TTL arguments for
incrWithTtlAtomic (e.g., assert mockIncrWithTtlAtomic was called with a key
string matching the expected pattern using expect.stringMatching(/.../) and the
expected TTL value), referencing the mockIncrWithTtlAtomic call in the test so
it fails if the key format or TTL changes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2f3e98b5-6ef1-4dc0-b3a7-b986070c2225
📒 Files selected for processing (2)
apps/public-api/src/__tests__/api_usage.test.jsapps/public-api/src/middlewares/api_usage.js
|
Please mention issue, what it solves. |
|
Hey @yash-pouranik , |
|
ok to merge? |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
# Get PR details and status
gh pr view 291 --repo geturbackend/urBackend --json title,state,reviews,statusCheckRollup,mergeable,mergeStateStatus,body
echo "---REVIEW_COMMENTS---"
gh pr reviews 291 --repo geturbackend/urBackend
echo "---COMMENTS---"
gh pr view 291 --repo geturbackend/urBackend --json comments --jq '.comments[] | {author: .author.login, body: .body}'
echo "---CHECKS---"
gh pr checks 291 --repo geturbackend/urBackend
echo "---DIFF---"
gh pr diff 291 --repo geturbackend/urBackendLength of output: 30906
✅ All CI checks pass — frontend-lint, public-api-tests, dashboard-api-tests, sdk-tests, python-sdk-tests, and CodeQL all green. ✅ Merge state is CLEAN and the branch has no conflicts. ✅ The fix is correct and minimal:
Verdict: The PR is good to merge. The core bug fix is solid and well-tested. The nitpick can be addressed in a follow-up if desired. |
|
fix these 2 things. |
…lAtomic assertion
|
Hey @yash-pouranik, |
🚀 Pull Request Description
Fixes the issue where MongoDB/Mongoose logging write failures caused
UnhandledPromiseRejectioncrashes..catch()error handler to the fire-and-forgetLog.create(...)promise call in api_usage.js. This ensures that any asynchronous rejections during request logging (such as MongoDB connection drops or schema validation errors) are captured gracefully and do not crash the Node.js process.ApiAnalytics.create(...)insidesetImmediateto logerr.message || errinstead of the broad error object, reducing verbose log dumps.🛠️ Type of Change
🧪 Testing & Validation
Backend Verification:
npm test(via workspace test suites) and all tests passed.Frontend Verification:
npm run lintin thefrontend/directory.📸 Screenshots / Recordings (Optional)
N/A
✅ Checklist
Built with ❤️ for urBackend.
Summary by CodeRabbit
Tests
Bug Fixes