Db v1 setup #280
Open
Db v1 setup #280
+2,235
−50
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 ↗︎
|
![recurseml[bot]](https://avatars.githubusercontent.com/in/1054195?s=60&v=4){kind=small}
| ) | ||
| .query(async ({ctx}) => { | ||
| const connectorConfigs = await ctx.db.query.connector_config.findMany({ | ||
| where: eq(schema.connector_config.org_id, ctx.viewer.orgId!), |
There was a problem hiding this comment.
Using the non-null assertion operator (!) with ctx.viewer.orgId is unsafe. If orgId is undefined, this will pass TypeScript checks but fail at runtime. Should either handle the undefined case explicitly or validate ctx.viewer.orgId exists earlier.
💬 Reply with
/praiseto tell me that this comment was useful.
Is this comment irrelevant to your project? Reply with
/ignoreto no longer receive comments like this in the future.
| import {sql} from 'drizzle-orm' | ||
| import postgres from 'postgres' | ||
| import {env} from '@openint/env' | ||
| import type {createDatabase} from './db' |
There was a problem hiding this comment.
The import path './db' is incorrect. Since the files are organized in packages, it should be imported from '@openint/db-v1/db' or similar package path.
💬 Reply with
/praiseto tell me that this comment was useful.
Is this comment irrelevant to your project? Reply with
/ignoreto no longer receive comments like this in the future.
| import postgres from 'postgres' | ||
| import {env} from '@openint/env' | ||
| import type {createDatabase} from './db' | ||
| import {createLiteDatabase, setupTestDatabase} from './db' |
There was a problem hiding this comment.
Similar to above, the import path './db' is incorrect and should reference the proper package path '@openint/db-v1/db'.
💬 Reply with
/praiseto tell me that this comment was useful.
Is this comment irrelevant to your project? Reply with
/ignoreto no longer receive comments like this in the future.
| // - invalid utf-8 sequence of 1 bytes from index 0 | ||
| /** For the full schema, pretty much only used for testing */ | ||
| export async function getMigrationStatements() { | ||
| return generateMigration(generateDrizzleJson({}), generateDrizzleJson(schema)) |
There was a problem hiding this comment.
The generateMigration function appears to be incorrectly used. According to the commented error messages and common Drizzle patterns, this approach is problematic and causes Next.js to crash. The code should use the official Drizzle migration tools like drizzle-orm/postgres-js/migrator or drizzle-orm/pglite/migrator as shown in the migration.ts file instead of direct schema manipulation through drizzle-kit/api.
💬 Reply with
/praiseto tell me that this comment was useful.
Is this comment irrelevant to your project? Reply with
/ignoreto no longer receive comments like this in the future.
| @@ -70,6 +71,7 @@ export const envConfig = { | |||
| DATABASE_URL_UNPOOLED: process.env['DATABASE_URL_UNPOOLED'], | |||
| VERCEL_URL: process.env['VERCEL_URL'], | |||
| INTEGRATION_TEST_SECRET: process.env['INTEGRATION_TEST_SECRET'], | |||
| PGLITE: process.env['PGLITE'], | |||
There was a problem hiding this comment.
The PGLITE environment variable is defined as z.boolean().optional() in the schema but is being read directly from process.env without type conversion. process.env values are always strings, so this will cause a type mismatch. It should be converted to boolean using something like process.env['PGLITE'] === 'true' or Boolean(process.env['PGLITE'])
💬 Reply with
/praiseto tell me that this comment was useful.
Is this comment irrelevant to your project? Reply with
/ignoreto no longer receive comments like this in the future.
|
😱 Found 5 issues. Time to roll up your sleeves! 😱 |
This reverts commit 6a1974b.
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
There was a problem hiding this comment.
❌ Changes requested. Reviewed everything up to 6a1974b in 3 minutes and 59 seconds
More details
- Looked at
2139lines of code in37files - Skipped
2files when reviewing. - Skipped posting
34drafted comments based on config settings.
1. packages-v1/api-v1/trpc/_base.ts:19
-
Draft comment:
This duplicates existing functionality. Please useprotectedProcedureinstead, which provides the same authentication plus additional features. -
function
protectedProcedure(trpc.ts) -
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%
Without seeing the implementation of getProtectedContext, I cannot be certain that protectedProcedure provides the exact same functionality. The new code implements specific role-based checks that may be intentionally different from protectedProcedure. The PR author likely had a reason for creating these new, more specific procedures.
I don't have full visibility into what protectedProcedure actually does. The new procedures might serve a different purpose with their specific role checks.
Without seeing the full implementation of protectedProcedure, I cannot confidently say this is duplicate functionality. The new code appears to implement specific role-based authorization that may be intentionally different.
Delete the comment as we don't have enough context to verify that protectedProcedure would provide equivalent functionality, and the new code implements specific role-based checks that may be intentionally different.
2. packages-v1/api-v1/trpc/_base.ts:35
-
Draft comment:
This appears to duplicate existing adminProcedure. Consider using or extending the existing implementation if you need slightly different role restrictions. -
exported constant
adminProcedure(trpc.ts) -
Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 30% vs. threshold = 50%
The comment identifies real duplication, but there may be intentional differences between the implementations. The packages seem to be separate (packages-v1 vs packages), suggesting they might need to be different. Without more context about why this new implementation exists in a v1 package, we can't be certain that reusing the existing implementation is correct.
I might be overlooking the importance of maintaining separate implementations in different package versions. Also, the differences in implementation might be intentional design choices rather than oversight.
While the duplication is real, the separate v1 package and implementation differences suggest this might be intentional rather than an oversight that needs fixing.
Given the uncertainty about whether this duplication is intentional due to versioning, and following the principle of removing comments unless we're very confident, we should remove this comment.
3. packages-v1/api-v1/app.ts:21
- Draft comment:
Consider using createNeonDatabase to measure neon latency instead of createDatabase, unless intentionally identical. - 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%
The variable names suggest an intention to measure two different types of database connections (postgresJs vs neon), but the code uses identical function calls. However, without seeing the definition of createNeonDatabase or knowing if it exists, I can't be certain the suggestion is valid. The comment is speculative without more context.
I might be missing important context about createNeonDatabase from other files. The identical measurements might be intentional for comparison purposes.
Even if createNeonDatabase exists elsewhere, we should follow the rule of not making speculative comments or assuming intent without strong evidence in the current file.
Delete the comment as it requires additional context to validate and could be speculative. The identical measurements might be intentional.
4. packages-v1/api-v1/trpc/routers/connection.ts:20
- Draft comment:
ctx.viewer.orgId is used without explicit guarantee; ensure the viewer object always contains orgId, especially since contextFromRequest sets role to 'system'. - 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%
The use of the non-null assertion (!) is a potential red flag. However, without seeing the type definitions for ctx.viewer or understanding the authentication setup, we can't be certain this is actually an issue. The comment is speculative without showing concrete evidence of the problem. The mention of 'contextFromRequest' isn't visible in the provided code.
We don't have access to the type definitions or authentication setup - the issue could be already handled elsewhere. The TODO comment suggests this code might be placeholder anyway.
While type safety is important, without seeing the full context of how ctx.viewer is guaranteed, this comment is too speculative and asks for verification rather than pointing out a concrete issue.
Delete the comment as it violates the rules by being speculative and asking for verification without strong evidence of an actual issue.
5. packages-v1/api-v1/trpc/routers/connectorConfig.ts:19
- Draft comment:
Similar to connection router, ensure that ctx.viewer.orgId is defined when querying connector_config. - Reason this comment was not posted:
Comment looked like it was already resolved.
6. apps/web/app/api/trpc/[...trpc]/route.ts:3
- Draft comment:
Using the 'edge' runtime is intentional; verify that its streaming behavior suits all TRPC responses. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%
The comment is asking the PR author to verify the behavior of the 'edge' runtime, which is against the rules. It does not provide a specific suggestion or point out a specific issue with the code. Therefore, it should be removed.
7. bin/tp.ts:87
- Draft comment:
Good addition to check for 'localtest.me' in the hostname; confirm that this covers all local testing scenarios. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%
The comment is asking the author to confirm their intention regarding the use of 'localtest.me' in the hostname. This violates the rule against asking the PR author to confirm their intention or to ensure the behavior is intended. Therefore, this comment should be removed.
8. docker-compose.yml:40
- Draft comment:
The neon-proxy service is added with proper dependency on postgres health; double‐check that the PG_CONNECTION_STRING format is correct. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%
The comment asks the PR author to double-check the PG_CONNECTION_STRING format, which violates the rule against asking the author to double-check things. It does not provide a specific suggestion or ask for a specific test to be written.
9. docker/neon-proxy/Caddyfile:1
- Draft comment:
Caddyfile setup for reverse proxy with TLS settings looks correct; ensure the trusted CA (server.pem) is up‐to‐date when regenerated. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%
This comment is asking the PR author to ensure that a specific file is up-to-date when certain settings are regenerated. This falls under the category of asking the author to ensure something is done, which is against the rules.
10. docker/neon-proxy/Dockerfile:56
- Draft comment:
Self-signed certificate generation and copying of start.sh look solid; add a comment if troubleshooting cert issues is needed in the future. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%
This comment is purely informative and does not provide a specific code suggestion or request for clarification. It suggests adding a comment for future troubleshooting, which is not actionable for the current PR.
11. docker/postgres/Dockerfile:2
- Draft comment:
This Dockerfile for pg_session_jwt extension isn’t fully functional yet; ensure the version updates and build instructions are revisited. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%
The comment is asking the author to ensure that the Dockerfile is fully functional and to revisit version updates and build instructions. This is a request for confirmation and testing, which violates the rules.
12. package.json:27
- Draft comment:
The environment pull scripts now use separate commands with a transformation pipeline. Confirm that the bun-based escape script reliably handles all edge cases. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%
The comment asks the PR author to confirm that a specific script handles all edge cases, which is a form of asking for confirmation or verification. This violates the rule against asking the PR author to confirm their intention or ensure behavior. Therefore, this comment should be removed.
13. packages-v1/api-v1/app.ts:18
- Draft comment:
Both Postgres and Neon latency checks use createDatabase with the same URL; consider using createNeonDatabase for the Neon check if distinct behavior is expected. - 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%
The code does look strange - why have two identical latency checks? However, I don't see any evidence of a createNeonDatabase function in the imports. The comment is speculative - "if distinct behavior is expected". We don't know the author's intention, and there could be valid reasons for checking the same connection twice.
I might be missing context about whether createNeonDatabase exists elsewhere in the codebase. The duplicate checks might be intentional for measuring variance.
Without clear evidence that createNeonDatabase exists or that using identical checks is wrong, this comment is too speculative.
The comment makes assumptions about intended behavior and suggests using a function we can't verify exists. It's too speculative to be actionable.
14. packages-v1/api-v1/scripts/generateAndExportKeys.ts:13
- Draft comment:
Key generation script for RSA keys using jose is well implemented; ensure the generated metadata (kid, use, alg) aligns with downstream verification requirements. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%
This comment is asking the PR author to ensure that the generated metadata aligns with downstream verification requirements. This falls under the category of asking the author to ensure something is tested or verified, which violates the rules.
15. packages-v1/api-v1/trpc/routers/connection.ts:20
- Draft comment:
Query uses ctx.viewer.orgId, yet the default context (e.g. in contextFromRequest) only sets role: 'system'. Ensure that orgId is populated when needed. - 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%
The comment raises a potentially valid concern about accessing orgId that might be undefined. However, we can't verify the claim about default context without seeing the context definition files. The TODO comment suggests this code isn't finished anyway. The comment is asking for verification rather than pointing out a clear issue.
I might be too quick to dismiss this - using undefined values could cause runtime errors. Also, the reviewer seems to have specific knowledge about the default context.
While the concern might be valid, the comment violates our rules by asking for verification rather than pointing out a clear issue. Additionally, the TODO suggests this code is incomplete anyway.
Delete the comment because it asks for verification ("Ensure that...") rather than pointing out a clear issue, and we can't verify the claim about default context without more files.
16. packages-v1/api-v1/trpc/routers/index.ts:18
- Draft comment:
Minor formatting: trailing comma in mergeRouters is harmless but review for consistency. - Reason this comment was not posted:
Confidence changes required:30%<= threshold50%
None
17. packages-v1/db-v1/neon.test.ts:14
- Draft comment:
Neon Pool test is currently skipped; consider enabling once the neon proxy setup is stable to validate WS connections. - 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 appears to be more of an informational comment suggesting to enable tests later. It's not pointing out a clear issue that needs to be fixed now. The .skip appears intentional based on the eslint-disable comment at the top and other skipped tests in the file. The comment is making assumptions about when tests should be enabled.
The test might be skipped for a good reason that I don't fully understand. There could be infrastructure dependencies that need to be set up first.
The comment is making assumptions about test infrastructure and timing without clear evidence that enabling the test now would be correct or useful.
This comment should be deleted as it's not pointing out a clear issue that needs fixing, but rather making suggestions about future test enabling that may depend on external factors.
18. packages-v1/db-v1/scripts/decode-jwt.sql:315
- Draft comment:
Double-check the RSA decryption logic (e.g. modular exponentiation and conversion) to ensure compatibility with all JWTs. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%
The comment is asking the PR author to double-check the RSA decryption logic for compatibility, which violates the rule against asking the author to confirm or ensure behavior. It doesn't provide a specific suggestion or point out a clear issue.
19. packages/env/env.ts:1
- Draft comment:
Importing z from '@opensdks/util-zod' – confirm that this is intentional and consistent with your use of zod elsewhere. - 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.
20. packages/env/env.ts:117
- Draft comment:
Overriding NEXT_PUBLIC env values from localStorage is useful for debugging, but ensure this mechanism is disabled or secure in production environments. - 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.
21. scripts/escape-env.ts:11
- Draft comment:
The escape script for Vercel env values addresses issue #11258; verify that the regex replacement works for all edge cases with embedded spaces or quotes. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%
The comment is asking the PR author to verify the regex replacement for edge cases, which falls under asking the author to double-check things. This violates the rules.
22. .github/workflows/validate-workflow.yml:120
- Draft comment:
Typo in comment: 'To test this with a locally install postgres, run' should be 'To test this with a locally installed postgres, run'. Please correct the wording for clarity. - 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.
23. bin/tp.ts:19
- Draft comment:
Typographical error: In the USAGE string definition at line 19, 'iun hex codes' should be corrected to 'in hex codes'. - 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.
24. docker-compose.yml:25
- Draft comment:
Typo found: 'lastest tag' should be corrected to 'latest tag' to avoid confusion. - Reason this comment was not posted:
Comment was on unchanged code.
25. docker/neon-proxy/Caddyfile:1
- Draft comment:
Typographical error: The listen directive on line 1 incorrectly uses two colons ('::4444 {') instead of one. Please update it to ':4444 {'. - Reason this comment was not posted:
Comment looked like it was already resolved.
26. packages-v1/api-v1/app.ts:28
- Draft comment:
Typo in comment: 'so we are forced tos specify url instead' should be corrected to 'so we are forced to specify url instead'. - Reason this comment was not posted:
Comment was on unchanged code.
27. packages-v1/db-v1/db.test.ts:1
- Draft comment:
Typo in the file comment: Line 1 still indicates 'test/index.test.ts' while the file is actually 'db.test.ts'. Consider updating the comment to match the new file name. - 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%
While technically correct, this is an extremely minor issue. The file header comment doesn't affect functionality, testing, or code quality. It's purely cosmetic and doesn't require any meaningful code changes. According to the rules, we should not make comments that are obvious or unimportant.
The comment is technically accurate and suggests a concrete fix. Incorrect documentation could potentially confuse future developers.
While accurate, this is too minor to warrant a PR comment. File header comments are often outdated and developers generally rely on actual file paths rather than header comments.
This comment should be deleted as it points out an unimportant cosmetic issue that doesn't meaningfully impact the code.
28. packages-v1/db-v1/migration.ts:17
- Draft comment:
The comment on line 17 says 'No need no more', which is a bit awkward. Consider rephrasing it to something like 'No longer needed' for clarity. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 0% vs. threshold = 50%
While the suggested wording is technically better English, commenting on comment wording is extremely low value. The comment itself is likely temporary and will be removed along with the commented code in a future cleanup. The actual code changes (new migration system) are what matter here.
Perhaps poor comment wording could confuse future developers? Maybe maintaining high quality even in comments is important for code readability?
The comment is clearly temporary documentation of removed code. Its exact wording has minimal impact on code quality or maintainability.
Delete this PR comment. Nitpicking temporary comment wording provides no real value and distracts from reviewing the actual code changes.
29. packages-v1/db-v1/neon.test.ts:14
- Draft comment:
Typo: In the commented-out code on line 14, the import statement is missing a closing quote. It should beimport('ws')instead ofimport('ws). - 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%
The code is commented out and appears to be example/reference code. The typo is minor and in non-functional code. Since the code isn't actually being used and is just a comment, fixing this typo won't impact functionality. The rules state we should only comment if there's clearly a code change required.
The typo could confuse future developers who try to uncomment and use this code. It's technically a valid issue.
While technically correct, commenting on typos in commented-out code is too minor and doesn't meet the bar of "clearly a code change required." Developers would quickly spot and fix this if they ever uncomment the code.
Delete the comment as it addresses a minor typo in commented-out code, which doesn't meet the threshold for requiring a PR comment.
30. packages-v1/db-v1/schema.ts:6
- Draft comment:
There is a typographical error in the comment on line 6. The phrase 'Needs to be create a new timestamp each time' should be corrected, e.g., to 'Needs to create a new timestamp each time to avoid implicit drizzle caching issues.' - 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%
While the grammatical error exists, fixing comment grammar is very low value. The meaning is still clear. The rules state to only keep comments that require clear code changes, and to avoid obvious or unimportant changes. This is purely cosmetic and doesn't affect functionality.
The comment does identify a real grammatical error, and clear documentation is important for maintainability.
However, the bar for PR comments should be higher - we should focus on substantive issues that affect code quality or functionality, not minor typos in comments.
This comment should be deleted as it addresses a trivial grammatical issue that doesn't impact code functionality or understanding.
31. packages-v1/db-v1/scripts/decode-jwt-no-verify.sql:2
- Draft comment:
On line 2, consider revising the comment for clarity. 'authorize issues on local for now...' could be rephrased to 'authorization issues on local environments for now...' to better convey the intended meaning. - 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%
The suggested change is extremely minor - changing "authorize" to "authorization" and adding "environments". While technically more correct English, this kind of nitpicking on comments doesn't improve code quality or functionality. Comments are meant to convey meaning, and the current wording, while slightly informal, is perfectly understandable.
The suggested wording is technically more correct English and could make the comment slightly clearer for non-native English speakers.
The benefit is too minor to justify a review comment. We should focus on substantive issues rather than minor wording tweaks in comments.
Delete this comment as it suggests an extremely minor wording change that doesn't meaningfully improve code quality or clarity.
32. packages/env/env.ts:39
- Draft comment:
Typo: Consider changing 'tho' to 'though' for clarity in the comment on the DEBUG variable (Line 39). - 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.
33. packages/env/env.ts:116
- Draft comment:
Typo: Correct 'overwriten' to 'overwritten' in the comment on Line 116. - 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.
34. scripts/escape-env.ts:1
- Draft comment:
On line 1, the comment "// Workaround for vercel env pull cannot properly handle value that contains " #11258" might be confusing due to the unescaped double quote. Consider rephrasing it (e.g., "// Workaround for Vercel env pull which cannot properly handle values containing double quotes; see #11258") for clearer communication. - 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%
While the suggested rewording might be marginally clearer, the original comment is still understandable and serves its purpose. The presence of the issue number (#11258) makes the context clear regardless. This seems like a purely stylistic suggestion that doesn't materially improve code quality or fix any actual issues.
The unescaped quote could potentially cause confusion for some readers. The suggested rewording is objectively clearer.
The marginal clarity improvement doesn't meet the threshold of "clearly a code change required" - the original comment is functional and includes the crucial issue reference.
Delete this comment as it suggests a purely stylistic change to documentation that doesn't materially improve code quality or fix any actual issues.
Workflow ID: wflow_05rJn5QGARheOW7G
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.
| fi | ||
|
|
||
| # Create required tables | ||
| psql -Atx $PG_CONNECTION_STRING \ |
There was a problem hiding this comment.
SQL commands to create the neon_control_plane tables work, but consider adding error checks to ensure these commands succeed before starting proxies.
| export const authenticatedProcedure = publicProcedure.use(({next, ctx}) => { | ||
| const viewer = ctx.viewer | ||
| if (!hasRole(viewer, ['customer', 'user', 'org'])) { | ||
| throw new TRPCError({code: 'FORBIDDEN', message: 'Admin only'}) |
There was a problem hiding this comment.
In authenticatedProcedure, the error message 'Admin only' may be misleading if non‐admin roles (like customer) are acceptable; consider a more generic access-denied message.
Suggested change
| throw new TRPCError({code: 'FORBIDDEN', message: 'Admin only'}) | |
| throw new TRPCError({code: 'FORBIDDEN', message: 'Access denied'}) |
| }) | ||
|
|
||
| afterAll(async () => { | ||
| // await dbs.pglite.$client.close() |
There was a problem hiding this comment.
The PGlite client close is commented out; ensure proper teardown of all database connections after tests.
Suggested change
| // await dbs.pglite.$client.close() | |
| await dbs.pglite.$client.close() |
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 14f5cd2 in 1 minute and 37 seconds
More details
- Looked at
16lines of code in1files - Skipped
0files when reviewing. - Skipped posting
3drafted comments based on config settings.
1. apps/web/app/api/trpc/[...trpc]/route.ts:1
- Draft comment:
Removal of 'runtime' export: confirm that the edge environment configuration is no longer needed. This change may affect deployment if running on edge was intended. - 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%
The comment asks for confirmation and verification, which violates our rules. It's asking the author to "confirm" something, which is explicitly forbidden. The comment is speculative about potential deployment issues. We don't have strong evidence that removing edge runtime is actually problematic.
Maybe the edge runtime is actually critical for this API endpoint and removing it could cause production issues?
Without clear evidence that edge runtime is required, we must assume the author knows what they're doing. The comment is asking for confirmation rather than pointing out a clear issue.
Delete this comment as it violates our rules by asking for confirmation and making speculative statements about potential issues without strong evidence.
2. apps/web/app/api/trpc/[...trpc]/route.ts:2
- Draft comment:
Removed 'export const runtime = "edge"'. Ensure this change is intentional as it affects the runtime environment. - 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%
The comment violates our rules by asking to "ensure" something is intentional. It's asking for confirmation rather than pointing out a clear issue. The PR author likely deliberately made this change, and if it causes issues, it would be caught in testing/deployment.
The runtime environment could be critical for functionality, and removing edge runtime might have significant performance implications.
While runtime changes are important, asking for confirmation isn't actionable. If there were specific issues with removing edge runtime, those should be pointed out directly.
Delete this comment as it merely asks for confirmation without providing actionable feedback or identifying a specific issue.
3. apps/web/app/api/trpc/[...trpc]/route.ts:4
- Draft comment:
Uncommented 'maxDuration'. Verify that 300 seconds is suitable for the current runtime configuration. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%
None
Workflow ID: wflow_Y5RHk9nEW265w0nH
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 759a916 in 1 minute and 21 seconds
More details
- Looked at
24lines of code in1files - Skipped
0files when reviewing. - Skipped posting
2drafted comments based on config settings.
1. packages-v1/api-v1/app.ts:22
- Draft comment:
Avoid forcing a cast with 'as unknown as Database'. Ensure createNeonDatabase returns an object matching the Database interface instead of bypassing type safety. - Reason this comment was not posted:
Marked as duplicate.
2. packages-v1/api-v1/app.ts:30
- Draft comment:
Typo: In the comment on line 30, 'forced tos specify url' should be corrected to 'forced to specify url'. - 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_y04CFBbM1MuiKV4W
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.
| const neonLatencyMs = await checkLatency( | ||
| createNeonDatabase({ | ||
| url: envRequired.DATABASE_URL, | ||
| }) as unknown as Database, |
There was a problem hiding this comment.
Avoid unsafe casting: instead of casting createNeonDatabase to Database, update its type or interface to avoid the unknown cast.
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
This PR sets up a comprehensive database system with Docker, API endpoints, environment configurations, and database migrations, enhancing the project's infrastructure and functionality.
postgres,supabase, andneon-proxyindocker-compose.yml.Dockerfileforneon-proxyandpostgreswith custom build steps.app.tsusingElysiaandswaggerfor OpenAPI specs.models.tsusingzodandzod-openapi.connectionandconnectorConfigintrpc/routers.env.tsto include new environment variables for database URLs and JWT keys.scripts/escape-env.tsto handle environment variable formatting issues.migrations/0000_initial.sql.migration.tsanddrizzle.config.ts.scripts/decode-jwt.sqlanddecode-jwt-no-verify.sql.db.test.tsandneon.test.ts.util-v1/id.ts.This description was created by
for 759a916. It will automatically update as commits are pushed.