Fix CI and add more unit tests

[openint-bot]

Important

Fix CI by updating database configurations, test setups, and workflow files, and refactor connection handling in backend services.

• CI Configuration:
  • Update POSTGRES_DB and POSTGRES_PASSWORD in validate-workflow.yml.
  • Remove action-upterm setup in validate-workflow.yml.
• Database and Tests:
  • Increase Jest timeout in end-to-end.spec.ts.
  • Add migrate and enableExtensions options in auth.spec.ts.
  • Update schema.sql with new table and index definitions.
  • Add RLS tests in rls.spec.ts.
  • Modify describeEachDatabase in test-utils.ts to support random database names.
• Code and Scripts:
  • Update initDbNeon, initDbPg, and initDbPGLite to include $end method.
  • Add formatNeonProxyPayload.ts script for SQL query formatting.
  • Refactor connectionRouter.ts and sync-service.ts for better connection handling.

^{This description was created by for a9bc940. It will automatically update as commits are pushed.}

[vercel]

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
openint	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Mar 4, 2025 5:01am

[ellipsis-dev]

❌ Changes requested. Reviewed everything up to a9bc940 in 2 minutes and 43 seconds

More details

• Looked at 1903 lines of code in 17 files
• Skipped 0 files when reviewing.
• Skipped posting 29 drafted comments based on config settings.

1. packages/db/db.ts:64

• Draft comment:
  Typo: 'additioanlExtensions' should be 'additionalExtensions'.
• 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.

2. packages/db/lib/upsert.ts:105

• Draft comment:
  Ensure proper type guard for column existence before using col.name. Although the check exists, consider clarifying or refactoring its logic.
• Reason this comment was not posted:
  Confidence changes required: 50% <= threshold 50%
  None

3. packages/engine-backend/router/connectionRouter.ts:225

• Draft comment:
  Ensure that patchReturning includes connectorConfigId properly to bypass RLS issues as noted in the comment.
• Reason this comment was not posted:
  Comment looked like it was already resolved.

4. packages/engine-backend/services/sync-service.ts:61

• Draft comment:
  Review use of console.log for debugging in production code; consider using a proper logging library or environment-based logging.
• 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.

5. .github/workflows/validate-workflow.yml:20

• Draft comment:
  Updated PostgreSQL environment variables (DB name and password) are now consistent across services. Confirm these hardcoded values ('postgres' and 'password') are intended for all environments.
• 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 is a test/validation workflow file, not production code. The credentials are clearly meant to be simple test values. Asking for confirmation about intended values violates the rule against asking authors to confirm their intention. The consistency across services is actually a good thing, not a concern.
  Maybe these credentials could be security sensitive if exposed? Maybe there's a company policy about credential values even in test environments?
  This is a GitHub Actions workflow file that's already public, and these are clearly non-production test credentials. The values are simple and consistent which is appropriate for a test environment.
  Delete the comment. It violates the rule against asking authors to confirm their intentions, and the consistent credential values are appropriate for this test workflow context.

6. apps/web/__tests__/end-to-end.spec.ts:10

• Draft comment:
  Jest timeout increased to 60s. Make sure that this timeout is sufficient for Next.js compilation during CI.
• Reason this comment was not posted:
  Comment did not seem useful. Confidence is useful = 0% <= threshold 50%
  The comment is asking the PR author to ensure that the timeout is sufficient, which violates the rule against asking the author to ensure or double-check things. It does not provide a specific suggestion or point out a specific issue with the code.

7. packages-v1/api-v1/__tests__/auth.spec.ts:26

• Draft comment:
  Additional database options (migrate: true, enableExtensions: true) are now passed to describeEachDatabase. Verify these options work across all targeted drivers.
• Reason this comment was not posted:
  Comment did not seem useful. Confidence is useful = 0% <= threshold 50%
  The comment is asking the PR author to verify that the new database options work across all targeted drivers. This falls under the rule of not asking the author to ensure the behavior is intended or to double-check things. Therefore, this comment should be removed.

8. packages/db/__generated__/schema.sql:144

• Draft comment:
  The schema dump has been updated for PostgreSQL 17.2, with migration table renamed to __drizzle_migrations and foreign keys now marked as deferrable. Ensure these changes are backward-compatible and that migration history isn’t disrupted.
• Reason this comment was not posted:
  Comment did not seem useful. Confidence is useful = 0% <= threshold 50%
  This comment is purely informative, providing details about the schema dump update and suggesting to ensure backward compatibility and migration history integrity. It doesn't make a specific code suggestion or ask for a specific test to be written.

9. packages/db/__tests__/migration.spec.ts:8

• Draft comment:
  Migration tests now restrict drivers to 'pg' due to extension issues with pglite. Consider adding a comment or separate tests for pglite once the extension problems are resolved.
• 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 already has clear comments explaining the pglite removal and the specific error. The comment suggests adding more comments or tests, but this is speculative ("once the problems are resolved") and doesn't require immediate action. The existing code comments are actually more detailed than what this review comment suggests adding.
  The comment does raise a valid point about test coverage being reduced. Maybe tracking this technical debt is important?
  While test coverage is reduced, this is already well-documented in the code comments. Adding another comment or creating speculative future tasks isn't actionable right now.
  Delete the comment as it doesn't add value beyond the existing code comments and makes speculative suggestions about future work.

10. packages/db/__tests__/rls.spec.ts:4

• Draft comment:
  Row-level security tests are comprehensive. Verify that the policies in the schema behave as expected and that role settings ('manager_alice' and 'manager_bob') are correctly enforced.
• Reason this comment was not posted:
  Comment did not seem useful. Confidence is useful = 0% <= threshold 50%
  This comment is asking the PR author to verify the behavior of policies and role settings, which violates the rule against asking for confirmation or verification of intended behavior. It does not provide a specific suggestion or point out a specific issue.

11. packages/db/__tests__/test-utils.ts:61

• Draft comment:
  Dynamic test database naming and truncation is a neat addition; however, ensure that dropping and creating databases in parallel tests does not lead to race conditions.
• Reason this comment was not posted:
  Comment did not seem useful. Confidence is useful = 0% <= threshold 50%
  This comment is asking the PR author to ensure that a specific behavior (race conditions) is tested or verified, which violates the rule against asking the author to ensure behavior is intended or tested. It does not provide a specific suggestion or point out a specific issue in the code.

12. packages/db/db.neon.ts:49

• Draft comment:
  Neon DB initialization now bypasses setting GUC for system viewers to avoid extra transactions. Verify that this change doesn’t unintentionally affect queries that depend on role-specific settings.
• Reason this comment was not posted:
  Comment did not seem useful. Confidence is useful = 0% <= threshold 50%
  The comment is asking the PR author to verify that a change doesn't unintentionally affect queries, which is against the rules. It doesn't provide a specific suggestion or ask for a specific test to be written. Therefore, it should be removed.

13. packages/db/db.pg.ts:16

• Draft comment:
  The addition of a $end method that calls pool.end() is a good enhancement for proper cleanup.
• Reason this comment was not posted:
  Comment did not seem useful. Confidence is useful = 0% <= threshold 50%
  This comment is purely informative, as it simply states that the addition of a $end method is an enhancement for proper cleanup. It does not provide any actionable feedback or suggestions for improvement.

14. packages/db/db.pglite.ts:31

• Draft comment:
  The new $end method for pglite (calling pglite.close()) ensures proper resource cleanup. This is a valuable addition.
• Reason this comment was not posted:
  Comment did not seem useful. Confidence is useful = 0% <= threshold 50%
  This comment is purely informative, as it simply states that the new method ensures proper resource cleanup without suggesting any changes or asking for confirmation on specific aspects of the code.

15. packages/db/db.ts:48

• Draft comment:
  The Database interface now optionally includes the $end() method. Verify that higher-level code properly checks for its existence before calling.
• 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/lib/upsert-sql.spec.ts:81

• Draft comment:
  Upsert SQL query snapshot tests are updated and look correct. Ensure that the JSONB merge operations reflect the intended behavior.
• Reason this comment was not posted:
  Comment did not seem useful. Confidence is useful = 0% <= threshold 50%
  The comment is asking the PR author to ensure that the behavior is intended, which violates the rule against asking for confirmation of intention or ensuring behavior. It does not provide a specific suggestion or point out a specific issue.

17. packages/db/lib/upsert.spec.ts:106

• Draft comment:
  Enhanced error handling in the upsert logic by checking for '!col || !col.name'. Verify that this condition reliably catches all cases of missing columns.
• Reason this comment was not posted:
  Comment did not seem useful. Confidence is useful = 0% <= threshold 50%
  The comment is asking the PR author to verify that a condition reliably catches all cases, which is against the rules. It does not provide a specific suggestion or ask for a specific test to be written. Therefore, it should be removed.

18. packages/db/scripts/formatNeonProxyPayload.ts:35

• Draft comment:
  The utility script for formatting Neon Proxy payloads handles placeholder replacements and JSON formatting. Double-check that all edge cases (e.g. nested JSON objects) are correctly handled.
• Reason this comment was not posted:
  Comment did not seem useful. Confidence is useful = 0% <= threshold 50%
  The comment is asking the PR author to double-check edge cases, which violates the rule against asking for confirmation or double-checking. It does not provide a specific code suggestion or point out a specific issue.

19. packages/engine-backend/router/connectionRouter.ts:198

• Draft comment:
  Renaming 'int' to 'ccfg' improves clarity. Also, the patch now includes 'connectorConfigId' to avoid RLS violations. Ensure all usages of connector config are consistent.
• Reason this comment was not posted:
  Comment looked like it was already resolved.

20. packages/engine-backend/services/sync-service.ts:60

• Draft comment:
  The sync service now includes detailed logging and default pipeline creation logic. Review the usage of 'ensurePipelinesForConnection' and the subsequent pipeline sync; make sure potential race conditions and error handling are adequately addressed.
• 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. packages/db/__tests__/migration.spec.ts:3

• Draft comment:
  Typographical error: In the comment at line 3, consider changing "Extension always cause issues..." to "Extension always causes issues..." for grammatical correctness.
• 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 suggestion is technically correct, commenting on grammar in code comments is not particularly valuable. The meaning is clear despite the minor grammatical error. This kind of nitpicking doesn't improve code quality or functionality and could be seen as noise in the review process.
  The grammar error could make the code less professional looking. Some teams might value maintaining high standards even in comments.
  While professionalism is important, this is an internal comment explaining a technical limitation, and the minor grammar issue doesn't impact understanding or maintainability.
  Delete this comment as it's purely about grammar in a comment, which doesn't affect code quality or functionality.

22. packages/db/__tests__/test-utils.ts:18

• Draft comment:
  Typographical issue: In the comment starting at line 18, 'neon driver does not work well for migration at the moment and
  and should therefore not be used for running migrations', the word 'and' is repeated. Consider removing one instance to improve clarity.
• Reason this comment was not posted:
  Comment was on unchanged code.

23. packages/db/db.neon.ts:13

• Draft comment:
  Typographical error: In the function signature at line 13, there's a stray colon after the closing parenthesis of the parameter list. It should simply be a ')' before the opening brace. Please remove the extra colon.
• 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. packages/db/db.pglite.ts:2

• Draft comment:
  Typo: Consider capitalizing 'typescript' as 'TypeScript' in the comment on line 2 for consistency with common naming conventions.
• 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.

25. packages/db/db.ts:64

• Draft comment:
  Typographical error: 'additioanlExtensions' is misspelled. Please rename it to 'additionalExtensions' for clarity and consistency.
• 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.

26. packages/db/lib/upsert-sql.spec.ts:291

• Draft comment:
  In the comment on line 292, consider changing 'explicit name setting cause the column.keyAsName to be set to false' to 'explicit name setting causes the column.keyAsName to be set to false' for grammatical correctness.
• 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 grammar suggestion is technically correct, it's just a minor typo in a code comment. The meaning is still perfectly clear. Code comments are informal documentation and don't need to be grammatically perfect. This kind of nitpick creates noise in PR reviews without adding value.
  Grammar errors in documentation could be seen as unprofessional. Clear communication is important in code comments.
  While clear communication is important, this tiny grammar fix doesn't meaningfully improve clarity or understanding. The original meaning is already clear.
  Delete this comment. It's an extremely minor grammar fix that doesn't impact code quality or clarity.

27. packages/db/lib/upsert.ts:188

• Draft comment:
  Typo in comment: It currently reads 'For the purpose of upserting, we only care about ] whether a column is jsonb or now because it needs different encoding'. Consider removing the stray ']' and changing 'now' to 'not' (or appropriate 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.

28. packages/engine-backend/services/sync-service.ts:98

• Draft comment:
  Typo: In the log message for creating the default outgoing pipeline, 'sync-serivce' is used. Please correct it to 'sync-service'.
• 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.

29. packages/engine-backend/services/sync-service.ts:112

• Draft comment:
  Typo: In the log message for creating the default incoming pipeline, 'sync-serivce' is used. Please change it to 'sync-service'.
• 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_7HzAbipWu74DBX9N

---

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.

[ellipsis-dev]

Consider handling edge cases for parameter formatting (e.g., embedded quotes) – current wrapping of strings in single quotes might cause SQL issues if not escaped.

[ellipsis-dev]

It appears that the default value for the id column in the public.event table is written as a literal string ('concat(''evt_'', generate_ulid())') rather than invoking the concatenation of the prefix 'evt_' with the result of generate_ulid(). Consider changing it to use an expression (e.g. DEFAULT concat('evt_', public.generate_ulid())) to ensure it evaluates as intended.

Suggested change

	id character varying DEFAULT 'concat(''evt_'', generate_ulid())'::character varying NOT NULL,
	id character varying DEFAULT concat('evt_', public.generate_ulid()) NOT NULL,