fix(db): switch from neon-http to neon-serverless pool for transaction support#740
Conversation
|
@VasundharaDP123 is attempting to deploy a commit to the Karan Mani Tripathi 's projects Team on Vercel. A member of the Team first needs to authorize it. |
|
CodeAnt AI is reviewing your PR. |
Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
github-actions
Bot
added
gssoc'26
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
⭐ Star Required
Hi @VasundharaDP123! Thank you for your contribution to DoubtDesk.
Before we can complete the review and merge this pull request, please star the DoubtDesk repository.
Once you have starred the repository, please drop a comment here saying "done" and we will proceed with reviewing your PR. Thank you!
|
@coderabbitai review |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThe PR switches Drizzle from Neon HTTP to a Pool-based serverless client, adjusts a transaction callback type, adds reply-vote route tests, and updates Inngest digest tests to invoke the wrapped function entrypoint. ChangesDatabase transactions and reply voting
Inngest digest test execution
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
done |
![codeant-ai[bot]](https://avatars.githubusercontent.com/in/646884?s=60&v=4){kind=small}
| const pool = new Pool({ connectionString: getDatabaseUrl() }); | ||
| export const db = drizzle({ client: pool }); |
There was a problem hiding this comment.
Suggestion: This creates a process-level pool with no lifecycle management, but the module does not expose or close it anywhere. In hot-reload/test-reset/serverless-reinit scenarios this can accumulate open DB sockets and cause connection exhaustion, so add an explicit pool lifecycle strategy (singleton guard and/or shutdown cleanup). [resource leak]
Severity Level: Major ⚠️
⚠️ Dev hot reload can accumulate unclosed Neon pools.
❌ Repeated serverless cold starts risk exceeding Neon connection limits.
⚠️ Background karma processor may stall under connection exhaustion.Steps of Reproduction ✅
1. Run `npm run dev` to start the Next.js development server; the server process imports
`src/configs/db.tsx`, which at lines 5–6 constructs a module-level Neon pool via `const
pool = new Pool({ connectionString: getDatabaseUrl() });` and wraps it in Drizzle with
`export const db = drizzle({ client: pool });`.
2. Make a server-side code change (for example in `src/app/api/karma/route.ts`, which
contains the `db.transaction` call at line 126 per grep) so that Next.js’ dev runtime
clears its module cache and hot-reloads server modules; on the next request to
`/api/karma`, the runtime re-imports `src/configs/db.tsx`, re-executing the top-level
`const pool = new Pool(...)` and creating a second `Pool` instance while the first still
holds open connections.
3. Inspect the code to verify there is no lifecycle management: `src/configs/db.tsx` only
exports `db` and a default re-export (lines 6–9) and never exposes `pool` or calls any
shutdown method; a grep for `pool.end` / `pool.close` / `pool.destroy` across
`/workspace/DoubtDesk` returns no matches, confirming that previously-created pools are
not explicitly closed when modules are reloaded.
4. Repeat this hot-reload process or deploy to a serverless environment where functions
are periodically reinitialized, causing multiple `Pool` instances to accumulate; over
time, endpoints that rely on transactions—such as the organizations API
`/api/organizations` (implemented in `src/app/api/organizations/route.ts`, where `const
createdOrg = await db.transaction(async (tx) => { ... })` appears at lines 18–35) and
background karma processing in `src/inngest/karma.ts` (`await db.transaction(async (tx) =>
{ ... })` at lines 32–64 and 204–238)—encounter connection exhaustion or slow failures
because orphaned pools keep database sockets open without any cleanup.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** src/configs/db.tsx
**Line:** 5:6
**Comment:**
*Resource Leak: This creates a process-level pool with no lifecycle management, but the module does not expose or close it anywhere. In hot-reload/test-reset/serverless-reinit scenarios this can accumulate open DB sockets and cause connection exhaustion, so add an explicit pool lifecycle strategy (singleton guard and/or shutdown cleanup).
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
|
CodeAnt AI finished reviewing your PR. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/app/api/doubts/[id]/accept/route.test.ts (1)
37-61: 🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
selectCallCountmust be renamed for thisjest.mock()factory.
next/jesthoistsjest.mock("@/configs/db", ...), soselectCallCountis an out-of-scope capture here. Rename it tomockSelectCallCounteverywhere in the file, including thebeforeEachresets.🤖 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 `@src/app/api/doubts/`[id]/accept/route.test.ts around lines 37 - 61, The Jest mock for db is capturing an out-of-scope variable, so the mock factory needs a hoist-safe name. Rename selectCallCount to mockSelectCallCount everywhere in src/app/api/doubts/[id]/accept/route.test.ts, including inside the jest.mock("`@/configs/db`") factory and any beforeEach/reset logic, so the mocked select() chain can track calls without violating next/jest hoisting rules.
🧹 Nitpick comments (3)
src/app/api/replies/vote/route.test.ts (2)
52-54: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueExposing the mock via
globalThisis a test-only code smell.Attaching
(globalThis as any).__voteDbMock = dbto reach into the mock from tests works, butjest.requireMock('@/configs/db').dbis the idiomatic way to retrieve a mocked module without polluting the global namespace.🤖 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 `@src/app/api/replies/vote/route.test.ts` around lines 52 - 54, The vote route test is leaking a test-only database mock onto globalThis, which should be avoided. Remove the __voteDbMock assignment from the db mock setup in route.test.ts and update the tests to access the mocked db through jest.requireMock('`@/configs/db`').db instead, keeping the mock scoped to the module and using the existing mocked db export.
65-129: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNo coverage for the "remove vote" (downvote) branch.
Both tests only exercise the "add vote" path (
existingLikeempty → insert + increment). Given this PR's goal is validating thatdb.transactionworks for the reply-vote flow, consider adding a case whereexistingLikeis non-empty to cover thetx.delete+ decrement branch as well.🤖 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 `@src/app/api/replies/vote/route.test.ts` around lines 65 - 129, Add test coverage for the reply vote “remove vote” path in the vote route tests. The current cases only hit the `existingLike` empty branch in `POST`, so add a scenario that seeds `replyLikesTable` with an existing like and verifies `db.transaction` exercises the `tx.delete` flow plus decrements the upvote count. Use the existing `POST` test setup and `selectResultQueue`/`updateResultQueue` patterns to keep the new case aligned with the current `route.test.ts` coverage.src/configs/db.tsx (1)
8-9: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMisleading comment.
The comment says
"Re-export the transaction helper so callers import from one place", but this line just re-exports the wholedbclient as default, not a distinct "transaction helper." Consider clarifying to avoid confusing future readers.🤖 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 `@src/configs/db.tsx` around lines 8 - 9, The top-level comment near the default export in db.tsx is misleading because it describes a transaction helper, but the symbol being re-exported is the db client itself. Update the comment to accurately describe that db is being re-exported as the default so callers can import the database client from one place, and keep the wording aligned with the export statement for clarity.
🤖 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.
Inline comments:
In `@src/app/api/replies/vote/route.test.ts`:
- Around line 3-6: Rename the hoisted Jest state bindings in the vote route test
to use mock-prefixed names, since currentUserMock, selectResultQueue,
updateResultQueue, and createQueryMock are captured by jest.mock factories and
can fail during collection. Update the related mock setup in the route.test.ts
file to follow the same mock* convention used in the sibling accept-route test,
and keep all references in the test body consistent with the new names.
In `@src/configs/db.tsx`:
- Around line 5-6: The database Pool setup in db.tsx is missing an error
listener, so idle-client errors can bubble up and crash the process. Update the
Pool initialization to register a pool.on('error', ...) handler right where pool
is created, and log or report the error there before exporting db from drizzle.
Use the existing pool symbol so the fix stays close to the current connection
setup.
- Around line 1-6: The Neon serverless pool in db creation is missing a
WebSocket constructor for Node runtimes, so queries can fail when this shared
client is used. Update the setup around the db module by configuring
neonConfig.webSocketConstructor before instantiating the Pool, or switch the
consuming routes to Edge if that is the intended runtime. Use the existing
getDatabaseUrl, Pool, and drizzle initialization in db to place the runtime
setup in the right spot.
---
Outside diff comments:
In `@src/app/api/doubts/`[id]/accept/route.test.ts:
- Around line 37-61: The Jest mock for db is capturing an out-of-scope variable,
so the mock factory needs a hoist-safe name. Rename selectCallCount to
mockSelectCallCount everywhere in src/app/api/doubts/[id]/accept/route.test.ts,
including inside the jest.mock("`@/configs/db`") factory and any beforeEach/reset
logic, so the mocked select() chain can track calls without violating next/jest
hoisting rules.
---
Nitpick comments:
In `@src/app/api/replies/vote/route.test.ts`:
- Around line 52-54: The vote route test is leaking a test-only database mock
onto globalThis, which should be avoided. Remove the __voteDbMock assignment
from the db mock setup in route.test.ts and update the tests to access the
mocked db through jest.requireMock('`@/configs/db`').db instead, keeping the mock
scoped to the module and using the existing mocked db export.
- Around line 65-129: Add test coverage for the reply vote “remove vote” path in
the vote route tests. The current cases only hit the `existingLike` empty branch
in `POST`, so add a scenario that seeds `replyLikesTable` with an existing like
and verifies `db.transaction` exercises the `tx.delete` flow plus decrements the
upvote count. Use the existing `POST` test setup and
`selectResultQueue`/`updateResultQueue` patterns to keep the new case aligned
with the current `route.test.ts` coverage.
In `@src/configs/db.tsx`:
- Around line 8-9: The top-level comment near the default export in db.tsx is
misleading because it describes a transaction helper, but the symbol being
re-exported is the db client itself. Update the comment to accurately describe
that db is being re-exported as the default so callers can import the database
client from one place, and keep the wording aligned with the export statement
for clarity.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: eeab6068-a19d-46eb-8c4c-bd5ce962ee86
📒 Files selected for processing (4)
src/__tests__/lib/anonymity.test.tssrc/app/api/doubts/[id]/accept/route.test.tssrc/app/api/replies/vote/route.test.tssrc/configs/db.tsx
…ing per CodeRabbit review
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/configs/db.tsx (1)
11-11: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueConsider making pool size configurable.
new Pool({ connectionString: getDatabaseUrl() })uses default pool sizing. In a serverless/Node deployment with many concurrent invocations, an explicitmax(andidleTimeoutMillis) tuned to the Neon plan's connection limits can avoid exhausting the connection budget under load.🤖 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 `@src/configs/db.tsx` at line 11, The Pool in db.tsx is currently created with default sizing, which can exhaust Neon connections under concurrency. Update the Pool initialization in the module-level pool setup to accept configurable limits, adding explicit max and idleTimeoutMillis values (ideally sourced from config/environment) alongside connectionString from getDatabaseUrl(). Keep the change localized to the pool creation so the existing database access helpers continue to use the same shared Pool instance.src/app/api/replies/vote/route.test.ts (1)
44-44: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMinor inconsistency:
tx.selectisn't a jest mock like its siblings.
db.select,db.insert, anddb.updateare alljest.fn()-wrapped (enabling call assertions), buttx.select(Line 44) is a plain arrow function. If a future test needs to assert on the in-transaction select call (e.g., verifying it queriedreplyLikesTable), this can't be done without refactoring.♻️ Proposed fix
- select: () => mockCreateQuery(mockSelectResultQueue.shift() ?? []), + select: jest.fn().mockImplementation(() => mockCreateQuery(mockSelectResultQueue.shift() ?? [])),🤖 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 `@src/app/api/replies/vote/route.test.ts` at line 44, `tx.select` in the vote route test is a plain arrow function while `db.select`, `db.insert`, and `db.update` are jest mocks, so it cannot be asserted in future tests. Update the transaction mock setup in `route.test.ts` so `tx.select` is wrapped with `jest.fn()` like the other query methods, keeping the existing `mockCreateQuery` behavior intact and making the in-transaction select call observable.
🤖 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.
Inline comments:
In `@src/__tests__/inngest/digest-functions.test.ts`:
- Line 119: The three `@ts-expect-error` comments above the sendDailyDigest.fn({
step } as any) calls are now unnecessary because the as any cast already
suppresses the type mismatch and will cause unused-directive TS2578 errors.
Remove those directives in the sendDailyDigest test cases and keep the existing
sendDailyDigest.fn({ step } as any) calls unchanged.
---
Nitpick comments:
In `@src/app/api/replies/vote/route.test.ts`:
- Line 44: `tx.select` in the vote route test is a plain arrow function while
`db.select`, `db.insert`, and `db.update` are jest mocks, so it cannot be
asserted in future tests. Update the transaction mock setup in `route.test.ts`
so `tx.select` is wrapped with `jest.fn()` like the other query methods, keeping
the existing `mockCreateQuery` behavior intact and making the in-transaction
select call observable.
In `@src/configs/db.tsx`:
- Line 11: The Pool in db.tsx is currently created with default sizing, which
can exhaust Neon connections under concurrency. Update the Pool initialization
in the module-level pool setup to accept configurable limits, adding explicit
max and idleTimeoutMillis values (ideally sourced from config/environment)
alongside connectionString from getDatabaseUrl(). Keep the change localized to
the pool creation so the existing database access helpers continue to use the
same shared Pool instance.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 8e1e9f9c-e8db-4d83-a334-63a0dc9b7f60
📒 Files selected for processing (9)
drizzle/0014_identity_system_update.sqldrizzle/0015_practice_attempts.sqldrizzle/0016_add_onboarding_fields.sqldrizzle/meta/_journal.jsonsrc/__tests__/api/teacher-insights.test.tssrc/__tests__/inngest/digest-functions.test.tssrc/app/api/doubts/[id]/accept/route.test.tssrc/app/api/replies/vote/route.test.tssrc/configs/db.tsx
✅ Files skipped from review due to trivial changes (1)
- src/tests/api/teacher-insights.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/app/api/doubts/[id]/accept/route.test.ts
…ix daily digest test casting
…connection leaks during development
|
Hi! Resolved all conflicts and added the new test cases from the review. Everything is passing and ready to go. Thanks! |
|
@vipul674 Resolve merge conflicts and also look into ci test failures and fix them |
github-actions
Bot
added
the
merge-conflict
knoxiboy
added
level:intermediate
github-actions
Bot
added
merge-conflict
|
CodeAnt AI is running Incremental review |
Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
|
CodeAnt AI Incremental review completed. |
|
CodeAnt AI is running Incremental review |
Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
github-actions
Bot
removed
the
merge-conflict
|
CodeAnt AI Incremental review completed. |
|
done |
github-actions
Bot
added
gssoc:approved
2 participants
User description
Description
I've switched the database driver from
neon-httptoneon-serverlessusing a connection Pool. This fixes the runtime crashes when callingdb.transaction()(which throws "No transactions support in neon-http driver").While getting this to compile and run, I also fixed a few issues in the test files:
accept/route.test.tsfrom Vitest to Jest since the repo is configured for Jest.anonymity.test.tsso the pre-commit hook builds successfully.src/app/api/replies/vote/route.test.tsto verify the counter increment logic.Related Issue
Closes #726
Type of Change
How Has This Been Tested?
Tested locally by running
npm test(all tests passed) and checkingnpm run build(build completed successfully with zero compiler errors).Checklist
npm run dev)anytypes)mainSummary by CodeRabbit
Bug Fixes
Tests
CodeAnt-AI Description
Fix database transactions in server runtime
What Changed
db.transaction()Impact
✅ Fewer transaction crashes✅ More reliable reply voting✅ Stable local database connections💡 Usage Guide
Checking Your Pull Request
Every time you make a pull request, our system automatically looks through it. We check for security issues, mistakes in how you're setting up your infrastructure, and common code problems. We do this to make sure your changes are solid and won't cause any trouble later.
Talking to CodeAnt AI
Got a question or need a hand with something in your pull request? You can easily get in touch with CodeAnt AI right here. Just type the following in a comment on your pull request, and replace "Your question here" with whatever you want to ask:
This lets you have a chat with CodeAnt AI about your pull request, making it easier to understand and improve your code.
Example
Preserve Org Learnings with CodeAnt
You can record team preferences so CodeAnt AI applies them in future reviews. Reply directly to the specific CodeAnt AI suggestion (in the same thread) and replace "Your feedback here" with your input:
This helps CodeAnt AI learn and adapt to your team's coding style and standards.
Example
Retrigger review
Ask CodeAnt AI to review the PR again, by typing:
Check Your Repository Health
To analyze the health of your code repository, visit our dashboard at https://app.codeant.ai. This tool helps you identify potential issues and areas for improvement in your codebase, ensuring your repository maintains high standards of code health.