Neon database client migration #251
Draft
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
There was a problem hiding this comment.
👍 Looks good to me! Reviewed everything up to 59adc8e in 1 minute and 43 seconds
More details
- Looked at
428lines of code in8files - Skipped
1files when reviewing. - Skipped posting
24drafted comments based on config settings.
1. apps/web/__tests__/end-to-end.spec.ts:125
- Draft comment:
Update to access results viarows.rowsas returned by neon. Ensure all tests use the correct response structure. - Reason this comment was not posted:
Marked as duplicate.
2. apps/web/app/api/connections/[connectionId]/sql/route.ts:62
- Draft comment:
Mapping now usesrows.rows.mapfor CSV export. Confirm that all downstream code expects neon's response format. - Reason this comment was not posted:
Marked as duplicate.
3. kits/cdk/internal/oauthConnector.ts:126
- Draft comment:
Updated promise chain in postConnect to cast response with potential error fields. Verify that error handling properly distinguishes oauth errors. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
4. packages/db/index.ts:16
- Draft comment:
Transitioned to using neon HTTP client: new constant 'sql' is built with neon(urlString). Verify that logger and schema options work as expected. - Reason this comment was not posted:
Marked as duplicate.
5. packages/db/package.json:15
- Draft comment:
Added dependency on '@neondatabase/serverless'. Ensure version selection fits with other packages. - Reason this comment was not posted:
Marked as duplicate.
6. packages/db/upsert.spec.ts:395
- Draft comment:
Tests updated to use neon connection (drizzle(neon(...))) and rows.rows indexing. Verify consistency in test expectations. - Reason this comment was not posted:
Marked as duplicate.
7. packages/engine-backend/inngest.ts:37
- Draft comment:
Changed result extraction to use res.rows from neon API. Confirm that event persistence logic now works with the new response shape. - Reason this comment was not posted:
Marked as duplicate.
8. packages/meta-service-postgres/makePostgresMetaService.ts:124
- Draft comment:
Updated mapping of query results to use rows.rows to handle neon responses. Double-check all conversions using camelCaseKeys to ensure consistency. - Reason this comment was not posted:
Marked as duplicate.
9. apps/web/__tests__/end-to-end.spec.ts:3
- Draft comment:
Include 'neon' in the import from @openint/db and ensure tests use the updated result structure (accessing rows via rows.rows). - Reason this comment was not posted:
Marked as duplicate.
10. apps/web/__tests__/end-to-end.spec.ts:16
- Draft comment:
Database initialization now wraps the URL using neon(). Confirm that drizzle(neon(...)) is used consistently. - Reason this comment was not posted:
Comment did not seem useful.
11. apps/web/__tests__/end-to-end.spec.ts:24
- Draft comment:
Return a test DB initialized with drizzle(neon(url.toString())). Ensure that later queries access results as rows.rows. - Reason this comment was not posted:
Marked as duplicate.
12. apps/web/__tests__/end-to-end.spec.ts:125
- Draft comment:
Update expectation to access query results using 'rows.rows[0]' instead of 'rows[0]'. - Reason this comment was not posted:
Marked as duplicate.
13. apps/web/__tests__/end-to-end.spec.ts:178
- Draft comment:
Access query result using 'rows.rows[0]' (for the greenhouse connection) to match the neon client’s return structure. - Reason this comment was not posted:
Marked as duplicate.
14. apps/web/app/api/connections/[connectionId]/sql/route.ts:62
- Draft comment:
Access query results using 'rows.rows.map' to correctly iterate over results returned by the neon client. - Reason this comment was not posted:
Marked as duplicate.
15. kits/cdk/internal/oauthConnector.ts:126
- Draft comment:
Enhanced type assertion in the .then block now includes an error property. This ensures proper type handling of oauth connection settings. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
16. packages/db/index.ts:16
- Draft comment:
Migrated to neon by using neon(urlString) and drizzle from 'drizzle-orm/neon-http'. Ensure that all subsequent query executions correctly access results via .rows. - Reason this comment was not posted:
Marked as duplicate.
17. packages/db/index.ts:35
- Draft comment:
Confirm that ensureSchema now checks existence using r.rows[0] instead of r[0]. - Reason this comment was not posted:
Marked as duplicate.
18. packages/db/package.json:15
- Draft comment:
Added '@neondatabase/serverless' dependency. Verify version '^0.10.4' is compatible with your neon migration. - Reason this comment was not posted:
Marked as duplicate.
19. packages/db/upsert.spec.ts:275
- Draft comment:
Ensure that test assertions use 'ret.rows[0]' (and similar) due to the neon client returning results with a 'rows' property. - Reason this comment was not posted:
Marked as duplicate.
20. packages/db/upsert.spec.ts:392
- Draft comment:
Clearing the dialect casing cache and recreating the DB connection via neon is a valid workaround for schema caching issues. - Reason this comment was not posted:
Marked as duplicate.
21. packages/engine-backend/inngest.ts:37
- Draft comment:
Chain .then((r) => r.rows) after db.execute to correctly extract rows from the neon client. - Reason this comment was not posted:
Marked as duplicate.
22. packages/meta-service-postgres/makePostgresMetaService.ts:124
- Draft comment:
Ensure all functions mapping query results use res.rows for iterating records (e.g., in searchCustomers and integration search). - Reason this comment was not posted:
Marked as duplicate.
23. packages/meta-service-postgres/makePostgresMetaService.ts:226
- Draft comment:
In the isHealthy check, use res.rows.length instead of res.length to validate that exactly one row is returned. - Reason this comment was not posted:
Marked as duplicate.
24. packages/meta-service-postgres/makePostgresMetaService.ts:304
- Draft comment:
In metaTable's get method, accessing the first row via rows.rows[0] properly handles the neon client's result format. - Reason this comment was not posted:
Marked as duplicate.
Workflow ID: wflow_iDuqKT3ak5U2vyQP
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
|
✨ No issues found! Your code is sparkling clean! ✨ |
There was a problem hiding this comment.
❌ Changes requested. Incremental review on 3bce91c in 1 minute and 8 seconds
More details
- Looked at
52lines of code in2files - Skipped
1files when reviewing. - Skipped posting
7drafted comments based on config settings.
1. apps/web/__tests__/end-to-end.spec.ts:18
- Draft comment:
Confirm that using 'WITH (FORCE)' for DROP DATABASE is safe and only used in testing. It may bypass active connections unexpectedly. - Reason this comment was not posted:
Comment did not seem useful: The comment asks the PR author to confirm their intention and ensure the behavior is intended, which violates the rules. It does not provide a specific code suggestion or ask for a specific test to be written. Therefore, it should not be approved.
2. apps/web/__tests__/end-to-end.spec.ts:18
- Draft comment:
Ensure the added 'WITH (FORCE)' option is supported in all environments and that the dynamically interpolated dbName is safe for testing. - Reason this comment was not posted:
Confidence changes required:50%
None
3. packages/db/index.ts:1
- Draft comment:
Verify the new imports (neonConfig, Pool) are fully compatible with existing usage. - Reason this comment was not posted:
Confidence changes required:30%
None
4. packages/db/index.ts:13
- Draft comment:
Double-check the fetchEndpoint logic; ensure it covers all expected environments. - Reason this comment was not posted:
Confidence changes required:30%
None
5. packages/db/index.ts:23
- Draft comment:
Consider using structured error handling for pool errors instead of direct console.error. - Reason this comment was not posted:
Confidence changes required:40%
None
6. packages/db/index.ts:33
- Draft comment:
Returning the pool along with db and sql may affect consumers expecting the previous return shape; confirm no breakage occurs. - Reason this comment was not posted:
Confidence changes required:40%
None
7. packages/db/index.ts:60
- Draft comment:
Ensure that opts.orderBy and opts.order are sanitized to prevent possible SQL injection; consider using sql.identifier for dynamic identifiers. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
Workflow ID: wflow_jfiDmU7xZqt8QGEu
Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 971953a in 47 seconds
More details
- Looked at
39lines of code in1files - Skipped
0files when reviewing. - Skipped posting
4drafted comments based on config settings.
1. packages/db/index.ts:1
- Draft comment:
Removal of neonConfig.fetchEndpoint: Confirm that removing custom endpoint configuration is safe. - Reason this comment was not posted:
Comment did not seem useful: The comment asks the PR author to confirm the safety of removing a configuration, which violates the rule against asking for confirmation or ensuring behavior. It does not provide a specific suggestion or question about the code itself.
2. packages/db/index.ts:15
- Draft comment:
Pool removal in getDb: Ensure that dropping the connection pool and error listener is intentional. - Reason this comment was not posted:
Comment did not seem useful: The comment starts with 'Ensure that...', which is a red flag according to the rules. It asks the PR author to confirm their intention, which is not allowed. Therefore, this comment should not be approved.
3. packages/db/index.ts:8
- Draft comment:
Custom fetchEndpoint configuration removed. Confirm the new neon client handles endpoint selection (e.g. for 'db.localtest.me') as needed. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
4. packages/db/index.ts:13
- Draft comment:
Pool creation and error handling were removed from getDb. Ensure that no consumers rely on the returned 'pool' object. - Reason this comment was not posted:
Marked as duplicate.
Workflow ID: wflow_xgeXTqsUUIpPZutK
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on efa7b8b in 40 seconds
More details
- Looked at
33lines of code in2files - Skipped
1files when reviewing. - Skipped posting
5drafted comments based on config settings.
1. packages/db/package.json:14
- Draft comment:
Removed '@neondatabase/serverless' dependency. Ensure no code relies on it. - Reason this comment was not posted:
Comment did not seem useful: The comment is asking the PR author to ensure that no code relies on a removed dependency. This falls under the rule of not asking the author to ensure or verify something, which is not allowed.
2. packages/meta-service-postgres/makePostgresMetaService.ts:1
- Draft comment:
Deleted import for NeonQueryFunction. Confirm it isn't needed. - Reason this comment was not posted:
Marked as duplicate.
3. packages/meta-service-postgres/makePostgresMetaService.ts:66
- Draft comment:
Changed DbClient type; removed '$client'. Ensure no references to '$client' remain. - Reason this comment was not posted:
Comment did not seem useful: The comment is asking the PR author to ensure that no references to '$client' remain after a type change. This falls under the rule of not asking the author to double-check or ensure something, which is not allowed.
4. packages/db/package.json:12
- Draft comment:
Ensure no remaining code depends on '@neondatabase/serverless', since its dependency was removed. - Reason this comment was not posted:
Marked as duplicate.
5. packages/meta-service-postgres/makePostgresMetaService.ts:1
- Draft comment:
Removal of the NeonQueryFunction import and the $client property in DbClient is correct—verify no downstream code relies on $client. - Reason this comment was not posted:
Marked as duplicate.
Workflow ID: wflow_TBw9aFUlXVcmpp5J
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on ace6a4b in 30 seconds
More details
- Looked at
12lines of code in1files - Skipped
1files when reviewing. - Skipped posting
2drafted comments based on config settings.
1. packages/db/package.json:15
- Draft comment:
Ensure version compatibility with other DB dependencies. Verify that ^0.10.4 works harmoniously with drizzle-kit and drizzle-orm, and update documentation/tests if needed. - Reason this comment was not posted:
Comment did not seem useful: The comment asks the PR author to ensure version compatibility and verify that a specific version works with other dependencies. This violates the rule against asking the author to ensure or verify things, especially related to dependencies.
2. packages/db/package.json:15
- Draft comment:
Ensure @neondatabase/serverless v0.10.4 is compatible and any required migration configs are updated. - Reason this comment was not posted:
Confidence changes required:33%
None
Workflow ID: wflow_VmkosCXjDzMm0i72
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on f9ba48a in 22 seconds
More details
- Looked at
13lines of code in1files - Skipped
0files when reviewing. - Skipped posting
2drafted comments based on config settings.
1. .github/workflows/validate-workflow.yml:45
- Draft comment:
Ensure that 'db.localtest.me' resolves as intended in CI. Typically, the PostgreSQL service is referenced using its service name (e.g., 'postgres'). Verify that this change works in all environments. - Reason this comment was not posted:
Comment did not seem useful: The comment starts with 'Ensure that...' and asks the author to verify the change in all environments, which violates the rules. It is not a specific code suggestion or question, and it is speculative about the change working in all environments.
2. .github/workflows/validate-workflow.yml:45
- Draft comment:
Ensure 'db.localtest.me' correctly resolves in the CI environment. If using a Docker service, verify that this hostname maps to the intended container (e.g., 127.0.0.1) or consider using the service alias. - Reason this comment was not posted:
Marked as duplicate.
Workflow ID: wflow_e6SquRXqo7qtjiwu
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 319e7b9 in 37 seconds
More details
- Looked at
65lines of code in2files - Skipped
0files when reviewing. - Skipped posting
5drafted comments based on config settings.
1. .github/workflows/release-workflow.yml:155
- Draft comment:
Removed extraneous blank line. Verify that formatting remains as intended. - Reason this comment was not posted:
Confidence changes required:0%
None
2. .github/workflows/validate-workflow.yml:7
- Draft comment:
Ensure that replacing the service container with 'docker compose up -d' covers all DB service needs for Neon migration. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
3. .github/workflows/release-workflow.yml:155
- Draft comment:
Minor formatting update: removal of an extra blank line before the 'Stably Runner Action' step. - Reason this comment was not posted:
Confidence changes required:0%
None
4. .github/workflows/validate-workflow.yml:9
- Draft comment:
The Postgres service block was removed. Ensure that the docker-compose setup replicates the previous service configuration (ports, credentials, health checks) to avoid DB connectivity issues. - Reason this comment was not posted:
Comment did not seem useful: The comment is asking the PR author to ensure that the docker-compose setup replicates the previous service configuration, which violates the rule against asking the author to ensure something. It is not a direct suggestion or question about the code itself.
5. .github/workflows/validate-workflow.yml:19
- Draft comment:
New Docker setup steps added. Verify that a valid docker-compose file exists and that it properly starts the required DB (and other) services to match the DATABASE_URL configuration. - Reason this comment was not posted:
Marked as duplicate.
Workflow ID: wflow_EgPO0mfZtybAvfyS
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 7ca1464 in 30 seconds
More details
- Looked at
13lines of code in1files - Skipped
0files when reviewing. - Skipped posting
2drafted comments based on config settings.
1. .github/workflows/validate-workflow.yml:16
- Draft comment:
Avoid hardcoding DB credentials; use secrets or env vars. - Reason this comment was not posted:
Marked as duplicate.
2. .github/workflows/validate-workflow.yml:16
- Draft comment:
Avoid hardcoding DB credentials; consider using GitHub secrets. Also, confirm that using 'postgres:[email protected]:5432/postgres' correctly reflects your Neon configuration. - Reason this comment was not posted:
Comment did not seem useful: The comment suggests avoiding hardcoding DB credentials, which is a valid suggestion. However, it also asks the author to confirm their configuration, which violates the rule against asking for confirmation. The first part of the comment is useful, but the second part is not allowed.
Workflow ID: wflow_Xb3IrgxkgzpKfDE7
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on d444b41 in 21 seconds
More details
- Looked at
42lines of code in2files - Skipped
0files when reviewing. - Skipped posting
4drafted comments based on config settings.
1. apps/web/__tests__/end-to-end.spec.ts:10
- Draft comment:
Consider using environment variables or configuration to set protocol/port instead of hardcoding it in tests. This avoids potential issues if the endpoint changes. - Reason this comment was not posted:
Confidence changes required:50%
None
2. packages/db/index.ts:11
- Draft comment:
Re-exporting neonConfig looks intentional; ensure consumers are aware of any required configuration when using it. - Reason this comment was not posted:
Confidence changes required:0%
None
3. apps/web/__tests__/end-to-end.spec.ts:10
- Draft comment:
Hardcoding neonConfig.fetchEndpoint in the test file works for now, but consider using env vars or a config helper to avoid magic numbers in other environments. - Reason this comment was not posted:
Confidence changes required:33%
None
4. packages/db/index.ts:11
- Draft comment:
Re-exporting neonConfig is fine; ensure consumers are updated to configure it as needed. - Reason this comment was not posted:
Confidence changes required:0%
None
Workflow ID: wflow_bCXevjxeKf5YHLyb
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 13c3daa in 1 minute and 0 seconds
More details
- Looked at
17lines of code in1files - Skipped
0files when reviewing. - Skipped posting
6drafted comments based on config settings.
1. packages/db/index.ts:13
- Draft comment:
Avoid global mutation of neonConfig.fetchEndpoint. Consider using a config parameter or injection to override endpoint behavior. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable:
The comment raises a valid architectural concern about global state mutation. However, looking at the @neondatabase/serverless library, this is actually the intended way to configure the endpoint behavior. The library is designed to use this global configuration pattern. Moving this to a parameter would require significant architectural changes and might not even be possible given the library's design.
The comment suggests a better practice, but may not understand the constraints of the neon library. Also, this could be the team's established pattern for configuration.
While global configuration isn't ideal, this is following the library's intended usage pattern and provides a clean way to handle local development environments.
Delete the comment. While it suggests a better practice in general, it's not practical given the library's design and would require excessive refactoring for minimal benefit.
2. packages/db/index.ts:14
- Draft comment:
Hardcoding 'db.localtest.me' and port numbers can reduce flexibility. Consider using environment variables or a config object. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable:
The hardcoded values are specifically for local development ('db.localtest.me') vs production environment. This is a common pattern for local development. The port 4444 is likely a standard local development port, and 443 is the standard HTTPS port. Moving these to env vars would add complexity without clear benefit since these are standard values that rarely change.
The comment has a point about flexibility - what if we need to support different local development setups or different ports in the future?
The current approach follows the principle of "configuration as code" for development environments, making it explicit and version controlled. Adding env vars for standard ports would be overengineering.
Delete the comment. The hardcoded values are reasonable defaults for local development vs production, and moving them to environment variables would add unnecessary complexity.
3. packages/db/index.ts:16
- Draft comment:
Explicitly appending default ports for HTTP/HTTPS might be unnecessary. Verify if this is needed, as URL constructors can apply defaults automatically. - Reason this comment was not posted:
Comment did not seem useful: The comment suggests verifying if appending default ports is necessary, which falls under asking the PR author to confirm or double-check something. This violates the rule against asking for confirmation or verification.
4. packages/db/index.ts:13
- Draft comment:
Consider adding an explicit type for the 'host' parameter (e.g., host: string) to improve type safety. - Reason this comment was not posted:
Confidence changes required:33%
None
5. packages/db/index.ts:15
- Draft comment:
Avoid hardcoding 'db.localtest.me'. Consider using a configuration or environment variable to allow flexibility for different local test domains. - Reason this comment was not posted:
Confidence changes required:33%
None
6. packages/db/index.ts:16
- Draft comment:
Explicitly including port 443 for HTTPS might be redundant since it's the default. Confirm that this is required for your endpoint routing. - Reason this comment was not posted:
Confidence changes required:33%
None
Workflow ID: wflow_p23VRzKMhODThFcm
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 1aa3ea7 in 39 seconds
More details
- Looked at
31lines of code in1files - Skipped
0files when reviewing. - Skipped posting
6drafted comments based on config settings.
1. apps/web/__tests__/end-to-end.spec.ts:212
- Draft comment:
Changing fixed length check to >2. Confirm that a minimum of 3 items is intended. Consider using toHaveLength if the count is known. - Reason this comment was not posted:
Comment did not seem useful: The comment asks the author to confirm their intention, which violates the rule against asking for confirmation. However, it also provides a specific suggestion to usetoHaveLength, which is a valid code suggestion. The comment is partially useful but needs to be rephrased to remove the request for confirmation.
2. apps/web/__tests__/end-to-end.spec.ts:251
- Draft comment:
Ensure that the greaterThan(2) check is correct in context. If the exact number is known, toHaveLength might be clearer. - Reason this comment was not posted:
Marked as duplicate.
3. apps/web/__tests__/end-to-end.spec.ts:272
- Draft comment:
Verify that expecting more than 1 integration item is correct. If 1 is expected, use toHaveLength(1) instead. - Reason this comment was not posted:
Comment did not seem useful: The comment starts with 'Verify that...', which is a clear indication that it is asking the PR author to confirm their intention. This violates the rule against asking the author to confirm or ensure behavior. Therefore, this comment should not be approved.
4. apps/web/__tests__/end-to-end.spec.ts:212
- Draft comment:
Using toBeGreaterThan(2) instead of an exact length reduces test determinism. If extra connectors are expected after migration, please document why and consider if an exact count check is more appropriate. - Reason this comment was not posted:
Marked as duplicate.
5. apps/web/__tests__/end-to-end.spec.ts:251
- Draft comment:
Updated length check to toBeGreaterThan(2) for connector config info. Please confirm that this relaxed check reflects the intended outcome after migration. - Reason this comment was not posted:
Marked as duplicate.
6. apps/web/__tests__/end-to-end.spec.ts:272
- Draft comment:
The assertion now checks for more than one integration instead of exactly one. Ensure that this relaxed check is intentional and documented. - Reason this comment was not posted:
Marked as duplicate.
Workflow ID: wflow_7ezeZsPqqRGSLieK
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on e0c5f7f in 37 seconds
More details
- Looked at
40lines of code in1files - Skipped
0files when reviewing. - Skipped posting
8drafted comments based on config settings.
1. apps/web/__tests__/end-to-end.spec.ts:212
- Draft comment:
Updated expectation using 'toBeGreaterThanOrEqual(2)' for list connections. Verify that this threshold change aligns with the intended test scenario. - Reason this comment was not posted:
Confidence changes required:0%
None
2. apps/web/__tests__/end-to-end.spec.ts:236
- Draft comment:
Threshold updated to 'toBeGreaterThanOrEqual(40)' for connector metas. Confirm this value reflects the current API contract. - Reason this comment was not posted:
Confidence changes required:0%
None
3. apps/web/__tests__/end-to-end.spec.ts:251
- Draft comment:
Updated expectation to 'toBeGreaterThanOrEqual(2)' for connector config info list length. Ensure this meets the required minimum. - Reason this comment was not posted:
Confidence changes required:0%
None
4. apps/web/__tests__/end-to-end.spec.ts:272
- Draft comment:
Changed expectation to 'toBeGreaterThanOrEqual(1)' for configured integrations. Confirm that having at least one integration is sufficient. - Reason this comment was not posted:
Confidence changes required:0%
None
5. apps/web/__tests__/end-to-end.spec.ts:212
- Draft comment:
Changed assertion: using 'toBeGreaterThanOrEqual(2)' allows exactly 2 connections. Ensure this boundary is intended. - Reason this comment was not posted:
Confidence changes required:0%
None
6. apps/web/__tests__/end-to-end.spec.ts:235
- Draft comment:
Updated boundary: 'toBeGreaterThanOrEqual(40)' now accepts exactly 40 keys. Confirm this matches expected data. - Reason this comment was not posted:
Confidence changes required:0%
None
7. apps/web/__tests__/end-to-end.spec.ts:251
- Draft comment:
Adjusted assertion to 'toBeGreaterThanOrEqual(2)' to include boundary case. Verify this is deliberate. - Reason this comment was not posted:
Confidence changes required:0%
None
8. apps/web/__tests__/end-to-end.spec.ts:272
- Draft comment:
Modified condition to 'toBeGreaterThanOrEqual(1)' allowing a single integration. Confirm this aligns with expectations. - Reason this comment was not posted:
Confidence changes required:0%
None
Workflow ID: wflow_HiZB6QvmRKm25rkb
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on f867960 in 53 seconds
More details
- Looked at
17lines of code in1files - Skipped
0files when reviewing. - Skipped posting
2drafted comments based on config settings.
1. packages/meta-service-postgres/makePostgresMetaService.ts:68
- Draft comment:
Prefer using a logging utility rather than console.log, and add error handling for new URL(databaseUrl).host in case databaseUrl is invalid. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable:
The logging suggestion is a minor style preference - console.log is fine for debug statements. The URL error handling suggestion is unnecessary since databaseUrl comes from zPgConfig which likely validates the URL format. This is debug-only code anyway. Neither suggestion identifies a real issue that needs fixing.
I could be wrong about the URL validation - maybe zPgConfig doesn't actually validate URL format. And maybe there are company standards around logging I'm not aware of.
Even if URL validation is missing from zPgConfig, this is debug-only code so a crash here wouldn't affect production. And logging standards typically focus on production logging, not debug statements.
The comment makes style suggestions about debug-only code that don't identify real issues needing fixes. The comment should be deleted.
2. packages/meta-service-postgres/makePostgresMetaService.ts:68
- Draft comment:
Consider using a proper logging library instead of console.log for debug output. Also, ensure that databaseUrl is valid to avoid errors in new URL() during debug logging. - Reason this comment was not posted:
Confidence changes required:33%
None
Workflow ID: wflow_lu4ZqxwIKlb4WEjl
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
❌ Changes requested. Incremental review on 85c5491 in 1 minute and 27 seconds
More details
- Looked at
1077lines of code in6files - Skipped
1files when reviewing. - Skipped posting
6drafted comments based on config settings.
1. packages/db/indexNeonBak.ts:33
- Draft comment:
TODO remains re-connecting logic; consider addressing reconnection handling if applicable. - Reason this comment was not posted:
Confidence changes required:50%<= threshold50%
None
2. packages/db/index.ts:37
- Draft comment:
Ensure that using r[0] (instead of r.rows[0]) is intentional based on the new postgres-js client return format. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50%
This comment violates our rules by asking for verification/confirmation. The author clearly made this change intentionally as part of switching from neon to postgres-js. The change is consistent with the overall migration pattern. If it was wrong, it would fail immediately in testing.
The change in access pattern could potentially be a source of runtime errors if the assumption about the return format is wrong.
This is a straightforward client library migration where the author explicitly changed the access pattern. The postgres-js documentation would make it clear what the correct access pattern is.
Delete this comment as it merely asks for verification of an intentional change that would be caught in basic testing.
3. packages/meta-service-postgres/makePostgresMetaService.ts:60
- Draft comment:
Await the db.execute() calls inside the assumeRole loop to ensure that all set_config commands complete before proceeding. - Reason this comment was not posted:
Marked as duplicate.
4. packages/meta-service-postgres/non-interactive.ts:80
- Draft comment:
Remove or secure the logging of raw query parameters to avoid exposing sensitive information in production. - Reason this comment was not posted:
Marked as duplicate.
5. packages/meta-service-postgres/interactive.ts:63
- Draft comment:
Consider using explicit typing for the 'client' parameter in runQueries instead of 'any' to improve type safety. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50%
While using explicit types is generally good practice, in this case: 1) The client type appears to be from an external library 2) The usage is consistent throughout the file 3) The client is used only for basic SQL operations 4) There's no evidence that the current typing has caused any issues 5) The type would likely be complex and possibly fragile to changes in the underlying library.
The comment does point out a legitimate TypeScript best practice. Explicit typing could help catch errors at compile time.
However, the current usage is consistent and pragmatic. The client type is likely complex and could make the code more brittle. The benefit doesn't clearly outweigh the cost.
While the suggestion follows TypeScript best practices, the current approach is pragmatic and the benefit of changing it is not clear enough to justify the comment.
6. packages/db/indexNeonBak.ts:34
- Draft comment:
Consider handling connection errors more robustly instead of only logging them, to ensure proper reconnection logic in your pool. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50%
While the suggestion is technically valid, there's already a TODO comment acknowledging this exact issue. The comment doesn't provide specific guidance on how to implement better error handling. It's more of a general suggestion that the code author is already aware of.
The comment could be valuable since proper error handling is important for database connections. The current implementation is quite basic.
However, the existing TODO comment shows the author is aware of this limitation, and the comment doesn't provide specific actionable guidance beyond what's already acknowledged.
Delete the comment since it's redundant with the existing TODO and doesn't provide specific enough guidance to be actionable.
Workflow ID: wflow_piaLPc62teymWo3S
Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
| @@ -57,7 +57,7 @@ async function assumeRole(options: { | |||
| const {db, viewer} = options | |||
| for (const [key, value] of Object.entries(localGucForViewer(viewer))) { | |||
| // true is for isLocal, which means it will only affect the current transaction, not the whole session | |||
| await db.execute(sql`SELECT set_config(${key}, ${value}, true)`) | |||
| db.execute(sql`SELECT set_config(${key}, ${value}, true)`) | |||
There was a problem hiding this comment.
Consider awaiting the db.execute call in assumeRole. Currently, line 60 calls db.execute without await, which might lead to race conditions.
| // true is for isLocal, which means it will only affect the current transaction, not the whole session | ||
| queries.push(neonSql`SELECT set_config(${key}, ${value}, true)`) | ||
| } | ||
| console.log('query', query.parameterizedQuery) |
There was a problem hiding this comment.
Remove or limit debug logging (e.g. logging query.parameterizedQuery) before production deployment.
packages/db/indexNeonBak.ts
Outdated
| query: any, | ||
| opts: {limit?: number; offset?: number; orderBy?: string; order?: string}, | ||
| ) { | ||
| const order = opts.order ? ` ${opts.order}` : '' |
There was a problem hiding this comment.
Consider parameterizing dynamic SQL parts (especially orderBy and order) to mitigate potential SQL injection risks if these inputs are not sanitized.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Important
Migrated database client to Neon, updated workflows, tests, and meta services to support the new client.
indexNeonBak.ts,upsert.spec.ts, andinteractive.ts.neonConfig.fetchEndpointto handle local and remote endpoints.postgreswithneonin database connection logic.release-workflow.yml,stably-workflow.yml, andvalidate-workflow.ymlto use Neon configurations.UnlyEd/github-action-await-vercel@v1for deployment checks.end-to-end.spec.tsto use Neon for database operations.makePostgresMetaService.tsandnon-interactive.tsto support Neon client.localGucForViewer.@neondatabase/serverlessdependency inpackage.json.docker-compose.ymlto includeneon-proxy.This description was created by
for 85c5491. It will automatically update as commits are pushed.