Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #117 +/- ##
=======================================
Coverage ? 41.47%
=======================================
Files ? 23
Lines ? 892
Branches ? 91
=======================================
Hits ? 370
Misses ? 516
Partials ? 6 ☔ View full report in Codecov by Sentry. |
test/connectors/neon.test.ts
Outdated
| import connector from "../../src/connectors/neon"; | ||
| import { testConnector } from "./_tests"; | ||
|
|
||
| describe.runIf(process.env.POSTGRESQL_URL)( |
There was a problem hiding this comment.
I wish we could somehow mock @neondatabase/serverless (not sure if in their SDK they have it already). We could use something like pglite to emulate their interface in order to allow automated tests.
|
Warning Rate limit exceeded
⌛ 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. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds a Neon Serverless PostgreSQL connector: implementation, registration, docs, dependency declarations, and tests (mocked Neon via pglite). Introduces a new connector name and options (url) and registers the connector in the connectors map. No existing public signatures removed. Changes
Sequence DiagramsequenceDiagram
participant App as Application
participant Connector as Neon Connector
participant NeonClient as Neon Client
participant DB as Neon Database
App->>Connector: create/use connector (url)
Connector->>Connector: lazy init client on first query
App->>Connector: prepare(sql) / execute(sql, params)
Connector->>Connector: normalizeParams(sql)
rect rgba(100,150,200,0.5)
Note over Connector: Replace ? with $1, $2, ...
end
Connector->>NeonClient: query(normalized_sql, params)
NeonClient->>DB: execute SQL
DB-->>NeonClient: rows
NeonClient-->>Connector: rows
Connector-->>App: results
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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: 4
🧹 Nitpick comments (1)
src/connectors/neon.ts (1)
42-48: Consider implementing optionaldisposemethod for connection cleanup.The
Connectorinterface includes an optionaldisposemethod. While not strictly required, implementing it would allow users to properly clean up the Neon connection when needed, following the pattern established by other connectors.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/connectors/neon.ts` around lines 42 - 48, Add an optional dispose method to the object returned by the neon connector so callers can cleanly close the Neon connection; specifically, extend the returned connector (currently with name, dialect, getInstance, exec, prepare) to include a dispose: async () => { ... } that obtains the connection via getConnection() (or getInstance()), calls the appropriate shutdown method on the underlying client (e.g., end(), close(), or release() as supported), and gracefully handles/propagates errors; update any types if needed to satisfy the Connector interface and ensure StatementWrapper, query, getConnection references remain unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/connectors/neon.ts`:
- Around line 23-26: The function neonConnector currently declares its return
type Connector<NeonQueryFunction<undefined, undefined>> which no longer matches
the corrected ConnectorOptions generic parameters; update the return type of
neonConnector to use the same generic type parameters as ConnectorOptions (so
the Connector<> wraps NeonQueryFunction with the same generics used by
ConnectorOptions) and ensure the internal _connection variable declaration
(NeonQueryFunction<...>) uses the same matching generics as well.
- Around line 71-77: The run method spreads the array result (res from
this.#query) into the returned object, which creates numeric keys and violates
the BoundableStatement contract that run() should return only { success: boolean
}; update the run method (function run in this class using this.#query and
this.#sql) to stop spreading ...res and instead return an object with only
success: true (or success: false on error), and if you need to expose rows keep
them in a separate explicit property (e.g., rows) rather than spreading the
array into the object.
- Around line 37-40: The InternalQuery type currently doesn't match the actual
return shape from neon()<false,false>; update the InternalQuery type signature
to return the correct Promise<Record<string, unknown>[]> (or a union that
includes that shape) so the query constant (const query: InternalQuery = async
(sql, params) => { const connection = getConnection(); return
connection(normalizeParams(sql), params); }) correctly types the result; locate
and change the InternalQuery type definition to reflect neon()'s <false, false>
return type (rows as Record<string, unknown>[]), and ensure any call sites
expecting the previous type are adjusted if necessary.
- Around line 9-12: ConnectorOptions extends HTTPTransactionOptions with invalid
generics (undefined, undefined); update the generics to proper boolean literals
to satisfy the type constraints in `@neondatabase/serverless`: change the extends
clause for ConnectorOptions to use the intended ArrayMode and FullResults
booleans (e.g., HTTPTransactionOptions<false, false> if you want rows as objects
and only rows returned) so TypeScript compiles; locate the ConnectorOptions
declaration in src/connectors/neon.ts and replace the undefined generics with
the appropriate true/false values matching the desired transaction result shape.
---
Nitpick comments:
In `@src/connectors/neon.ts`:
- Around line 42-48: Add an optional dispose method to the object returned by
the neon connector so callers can cleanly close the Neon connection;
specifically, extend the returned connector (currently with name, dialect,
getInstance, exec, prepare) to include a dispose: async () => { ... } that
obtains the connection via getConnection() (or getInstance()), calls the
appropriate shutdown method on the underlying client (e.g., end(), close(), or
release() as supported), and gracefully handles/propagates errors; update any
types if needed to satisfy the Connector interface and ensure StatementWrapper,
query, getConnection references remain unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6d062f6a-c51b-4875-8bb1-61ef239d4472
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (5)
docs/2.connectors/neon.mdpackage.jsonsrc/_connectors.tssrc/connectors/neon.tstest/connectors/neon.test.ts
| const query: InternalQuery = async (sql, params) => { | ||
| const connection = getConnection(); | ||
| return connection(normalizeParams(sql), params); | ||
| }; |
There was a problem hiding this comment.
Fix InternalQuery return type to match neon() actual return type.
The neon() function with <false, false> parameters returns rows as Record<string, unknown>[]. The InternalQuery type should reflect this more accurately.
🔧 Proposed fix
-type InternalQuery = (sql: string, params?: Primitive[]) => Promise<unknown[]>;
+type InternalQuery = (sql: string, params?: Primitive[]) => Promise<Record<string, unknown>[]>;🧰 Tools
🪛 GitHub Check: tests
[failure] 37-37:
Type '(sql: string, params: Primitive[] | undefined) => Promise<Record<string, any>[] | any[][] | FullQueryResults>' is not assignable to type 'InternalQuery'.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/connectors/neon.ts` around lines 37 - 40, The InternalQuery type
currently doesn't match the actual return shape from neon()<false,false>; update
the InternalQuery type signature to return the correct Promise<Record<string,
unknown>[]> (or a union that includes that shape) so the query constant (const
query: InternalQuery = async (sql, params) => { const connection =
getConnection(); return connection(normalizeParams(sql), params); }) correctly
types the result; locate and change the InternalQuery type definition to reflect
neon()'s <false, false> return type (rows as Record<string, unknown>[]), and
ensure any call sites expecting the previous type are adjusted if necessary.
| async run(...params: Primitive[]) { | ||
| const res = await this.#query(this.#sql, params); | ||
| return { | ||
| success: true, | ||
| ...res, | ||
| }; | ||
| } |
There was a problem hiding this comment.
Spreading an array into an object produces unexpected results.
The res variable is an array (unknown[]), so spreading it into an object with ...res will produce numeric keys (0, 1, 2...) rather than meaningful properties. The run() method should only return { success: boolean } per the BoundableStatement interface contract.
🐛 Proposed fix
async run(...params: Primitive[]) {
- const res = await this.#query(this.#sql, params);
+ await this.#query(this.#sql, params);
return {
success: true,
- ...res,
};
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| async run(...params: Primitive[]) { | |
| const res = await this.#query(this.#sql, params); | |
| return { | |
| success: true, | |
| ...res, | |
| }; | |
| } | |
| async run(...params: Primitive[]) { | |
| await this.#query(this.#sql, params); | |
| return { | |
| success: true, | |
| }; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/connectors/neon.ts` around lines 71 - 77, The run method spreads the
array result (res from this.#query) into the returned object, which creates
numeric keys and violates the BoundableStatement contract that run() should
return only { success: boolean }; update the run method (function run in this
class using this.#query and this.#sql) to stop spreading ...res and instead
return an object with only success: true (or success: false on error), and if
you need to expose rows keep them in a separate explicit property (e.g., rows)
rather than spreading the array into the object.
|
Thanks for rebasing PR @arashsheyda ❤️ We have another variant of PR #191 that uses pg client. @atilafassina and i were discussing to reach out to original SDK connector to figure out which is best. |
|
@pi0 oh nice! thanks |
2 participants
🔗 Linked issue
related to #32
❓ Type of change
📚 Description
this PR adds support for
neonconnector using@neondatabase/serverless+ an example ofneon📝 Checklist
Summary by CodeRabbit
New Features
Documentation
Tests
Chores