(feat) Create backend-pgsql#51
Merged
Merged
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (12)
📝 WalkthroughWalkthroughBackend API gains a write-mode generic ('snapshot' | 'event') with conditional state shapes; describeBackend is mode-aware. A new event-mode PostgreSQL backend, schema, migrations, tests, and package/tooling are added; existing snapshot backends are annotated to use 'snapshot'. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant PgBackend
participant DrizzleDB
participant SecretsTable
Client->>PgBackend: commit(events: StateEvent[])
PgBackend->>DrizzleDB: BEGIN TRANSACTION
loop per event
PgBackend->>DrizzleDB: upsert provider (if needed)
PgBackend->>DrizzleDB: insert/update/delete endpoint row
PgBackend->>SecretsTable: insert/update/delete secret row (if applicable)
end
DrizzleDB-->>PgBackend: transaction result
PgBackend-->>Client: ResultAsync<void, BackendError>
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Minterl
force-pushed
the
feat/backend-pgsql
branch
from
e7624d2 to
ac5225a
Compare
Minterl
force-pushed
the
feat/backend-pgsql
branch
from
71dee8f to
26c2d1c
Compare
Minterl
force-pushed
the
feat/backend-pgsql
branch
from
26c2d1c to
2daa037
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/backend/pgsql/compose.yaml`:
- Around line 4-5: The published Postgres port in the docker-compose ports
mapping ('54329:5432') exposes the test DB on all interfaces; update the ports
entry in compose.yaml to bind to loopback only by replacing '54329:5432' with
'127.0.0.1:54329:5432' so Docker publishes Postgres on 127.0.0.1 and prevents
external access while keeping the same host and container ports.
In `@src/backend/pgsql/drizzle/20260420160720_tiresome_zaladane/migration.sql`:
- Around line 1-22: Make the foreign keys non-nullable and add the missing
uniqueness constraints: change "endpoints"."provider_id" and
"secrets"."endpoint_id" to NOT NULL (update the CREATE TABLE for endpoints and
secrets and the ALTER constraints endpoints_provider_id_providers_id_fkey and
secrets_endpoint_id_endpoints_id_fkey accordingly), add a UNIQUE constraint on
"providers"."name", and add a composite UNIQUE on ("endpoints"."provider_id",
"endpoints"."handle") so endpoint handles are unique per provider; update the
Drizzle schema definitions and regenerate the snapshot to reflect these column
changes and new unique constraints.
In `@src/backend/pgsql/package.json`:
- Line 41: Update package.json to replace the pre-release pins for drizzle
packages: change the dependency "drizzle-orm" (currently "1.0.0-beta.22") and
the devDependency "drizzle-kit" (currently "1.0.0-beta.22") to the latest stable
release (e.g., "0.45.2"). Ensure both entries are updated so package managers
install the stable 0.45.2 versions, then run your lockfile update
(npm/yarn/pnpm) to refresh the lockfile and verify builds/tests pass; look for
the keys "drizzle-orm" and "drizzle-kit" to locate and modify the entries.
In `@src/backend/pgsql/src/db/schema.ts`:
- Around line 24-27: The providers table currently allows duplicate names;
update the providers pgTable definition so providers.name is unique (e.g., add a
unique constraint or unique index for the name column) to enforce provider-name
uniqueness at the DB level; modify the exported providers constant (the
pgTable('providers', {...}) definition) to declare the uniqueness on the name
column (or add a migration that creates a unique index like
providers_name_unique) so concurrent creates cannot produce duplicate provider
rows.
- Around line 29-35: The secrets table allows multiple rows per endpoint and
writeSecret uses a check-then-act flow causing a race; update the schema so the
endpointId column (endpoint_id) is notNull and has a unique constraint/index to
enforce one secret per endpoint, and change the writeSecret implementation to an
atomic upsert (INSERT ... ON CONFLICT (endpoint_id) DO UPDATE SET secret =
EXCLUDED.secret, ... ) instead of doing a SELECT then INSERT; reference the
secrets table and endpointId column in schema changes and the writeSecret
function for the upsert conversion.
- Around line 13-15: The providerId and endpointId columns defined as
integer('provider_id').references(() => providers.id, ...) and
integer('endpoint_id').references(() => endpoints.id, ...) must be made
non-nullable to match the backend inserts and cascade semantics; update both
column definitions by adding .notNull() to providerId and endpointId in
schema.ts and then regenerate the SQL migrations so the DB enforces non-null
foreign keys and prevents orphaned records.
In `@src/backend/pgsql/src/index.ts`:
- Around line 187-206: The update path for endpoint.updated currently writes
event.handle and event.after.url without validation; change the handler to
validate using createEndpointHandle(event.handle) and
createEndpointUrl(event.after.url) before persisting, and on validation failure
return an err BackendError with while: 'write' (not 'read') and an appropriate
message; apply the same validation-and-write pattern to the other occurrence
noted (the second endpoint update block) so no invalid handle/url can be stored.
- Around line 62-70: The current commit function launches commitEvent for every
event concurrently via events.map, causing out-of-order writes and partial
commits; change commit to open a single DB transaction (begin), then iterate
events sequentially calling commitEvent(db, event) (await each ResultAsync and
stop on first Err), and if any event returns an Err rollback the transaction and
return that error, otherwise commit the transaction and return Ok; ensure you
use the same db/transaction handle passed into each commitEvent call and avoid
ResultAsync.combine so processing is strictly ordered and atomic.
- Around line 161-185: The current flow always creates a provider when missing
which allows non-create events (update/delete) to cause unintended writes and
cross-provider mutations; change logic so only endpoint.created events insert a
new provider: in the handler around provider resolution (the provider variable
and the db.insert(schema.providers) block), if event.type !== 'endpoint.created'
do not insert — instead return a NotFoundError when provider is missing;
additionally, for update/delete paths that call the endpoint update/delete
queries, add providerId (from the resolved provider.id) to the WHERE predicates
(the clauses that currently only match by handle) so updates/deletes include
providerId to scope mutations to the correct provider. Ensure WriteRejectedError
usage remains for failed inserts in the endpoint.created insert path.
- Around line 58-61: The current use of ResultAsync.fromSafePromise around async
DB helpers (readState, readSecret, writeSecret, deleteSecret) is unsafe because
those helpers can reject; replace each ResultAsync.fromSafePromise(...) call
with ResultAsync.fromPromise(helperCall, (e) => ({ kind: 'BackendError', name:
'UnknownError', message: `Database error: ${e}`, while: '<operation>' })) where
<operation> is 'read'|'readSecret'|'writeSecret'|'deleteSecret' respectively, or
alternatively add try/catch inside the helper functions
(readState/readSecret/writeSecret/deleteSecret) to convert thrown errors into
Result.ok/err before returning; update callers to use the new error-mapped
ResultAsync.fromPromise form to keep rejections inside the Result abstraction.
In `@src/backend/pgsql/test/index.test.ts`:
- Around line 66-80: The readiness-check loop opens a PostgreSQL Client but
never closes it on the success path, leaking a connection; inside the while loop
after successfully connecting and running the query (the success branch just
before the break) call await client.end() to close the client (using the
existing Client instance created with new Client({ connectionString: databaseUrl
})) before breaking, ensuring the connection is always closed regardless of the
success or error path while keeping the existing timeout/retry logic around
start, CONN_TIMEOUT and CONN_RETRY_DELAY.
In `@src/backend/pgsql/tsconfig.json`:
- Around line 5-7: The tsconfig path alias currently uses the mapping "~/*":
["src/*"]; update this mapping to "~/*": ["./src/*"] to match the core package
convention and ensure consistent module resolution in the composite TypeScript
project (change the right-hand side array entry from "src/*" to "./src/*").
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 5f474d0f-fffa-4b8f-bf32-7e90651c6eb7
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (22)
src/backend/backend/src/backend.tssrc/backend/backend/src/index.tssrc/backend/backend/src/local.test.tssrc/backend/backend/src/local.tssrc/backend/pgsql/README.mdsrc/backend/pgsql/compose.yamlsrc/backend/pgsql/drizzle.config.tssrc/backend/pgsql/drizzle/20260420160720_tiresome_zaladane/migration.sqlsrc/backend/pgsql/drizzle/20260420160720_tiresome_zaladane/snapshot.jsonsrc/backend/pgsql/drizzle/20260421151532_first_cammi/migration.sqlsrc/backend/pgsql/drizzle/20260421151532_first_cammi/snapshot.jsonsrc/backend/pgsql/eslint.config.mjssrc/backend/pgsql/package.jsonsrc/backend/pgsql/src/db/schema.tssrc/backend/pgsql/src/index.tssrc/backend/pgsql/test/index.test.tssrc/backend/pgsql/tsconfig.build.jsonsrc/backend/pgsql/tsconfig.jsonsrc/backend/pgsql/turbo.jsonsrc/backend/pgsql/vitest.config.mjssrc/backend/remote/package.jsonsrc/backend/remote/src/backend.ts
Minterl
force-pushed
the
feat/backend-pgsql
branch
2 times, most recently
from
daf02bc to
96888fd
Compare
There was a problem hiding this comment.
Actionable comments posted: 7
♻️ Duplicate comments (1)
src/backend/pgsql/src/index.ts (1)
208-282:⚠️ Potential issue | 🟠 Major
while: 'read'in write-path validation errors.Inside
commitEvent(a write operation), the handle/url validation failures reportwhile: 'read'at lines 216, 225, 246, and 255. These should be'write', matching the rest of the write path (and matching the past-review guidance that update/create validation failures belong to the write operation).🩹 Suggested fix
- while: 'read', + while: 'write',Apply to each of the four
satisfies BackendErrorblocks undercase 'endpoint.created'andcase 'endpoint.updated'.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/backend/pgsql/src/index.ts` around lines 208 - 282, In commitEvent's write-path cases ('endpoint.created' and 'endpoint.updated') the validation error objects for createEndpointHandle and createEndpointUrl incorrectly set while: 'read'; change those four occurrences to while: 'write' so the BackendError objects in the create/update branches reflect the write operation (update the error objects in the cases handling createEndpointHandle and createEndpointUrl inside the 'endpoint.created' and 'endpoint.updated' switch blocks).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/backend/backend/src/backend.ts`:
- Around line 197-210: The overloads on describeBackend (with TStateWriteMode
extends 'snapshot' and extends 'event') force callers to pass a literal mode and
can confuse consumers who try to parameterize over the union
BackendStateWriteMode; add a concise JSDoc above the describeBackend overloads
explaining that the overload set intentionally requires a literal 'snapshot' or
'event' mode (referencing describeBackend and BackendDescriptor) and that
callers should choose the concrete mode rather than a union type, so downstream
integrators understand why a union type will not match an overload.
In `@src/backend/pgsql/package.json`:
- Around line 32-37: The package.json exports entry is incorrect: change the
subpath key "./index" to the root "." so consumers can import
`@hookplane/backend-pgsql`, and within that export object reorder the conditional
map to place the "types" condition before "default" (i.e., ensure the export for
"." contains "types": "./dist/index.d.ts" followed by "default":
"./dist/index.js") so Node/TypeScript module resolution picks up the .d.ts types
correctly.
In `@src/backend/pgsql/src/index.ts`:
- Around line 119-130: The current null check against endpoints is dead because
db.select(...).where(...) always returns an array; update the logic in the block
that calls
db.select().from(schema.endpoints).where(eq(schema.endpoints.providerId,
provider.id)) to either (A) treat an empty array as "not found" by checking
endpoints.length === 0 and returning the NotFoundError (adjusting the
message/context as needed), or (B) remove the unreachable if branch entirely if
an empty endpoints array should be allowed; make the change where endpoints,
db.select, schema.endpoints, provider.id and providerName are referenced in
index.ts.
- Around line 284-311: The handler for case 'endpoint.deleted' currently
auto-deletes the provider by calling tx.delete(schema.providers) when no
endpoints remain, which causes surprising lifecycle coupling; remove that
auto-delete and instead either (a) delete the tx.delete(schema.providers) call
and leave a TODO/log so providers persist independently, or (b) gate the
deletion behind an explicit cleanup policy/config flag (e.g., a
cleanupProvidersOnLastEndpoint flag checked inside the 'endpoint.deleted' case)
so deletion only happens when configured; update the block handling
endpoint.deleted (the case 'endpoint.deleted' branch that queries
schema.endpoints and calls tx.delete(schema.providers)) accordingly and adjust
tests to expect providers to remain unless the cleanup policy is enabled.
- Around line 132-156: The error messages concatenate the Result objects (handle
and url) instead of their underlying errors; when createEndpointHandle or
createEndpointUrl returns an Err, include the actual error (e.g., handle.error
or url.error / the Err property provided by your Result type) in the
BackendError message and/or set it as the error's cause. Update the error
returns in the endpoint initialization block (where endpointIndex is built) and
the analogous checks in commitEvent (the checks around
createEndpointHandle/createEndpointUrl at the mentioned lines) to use the
Result's error property rather than stringifying the Result object, and ensure
you still satisfy the BackendError shape.
- Around line 70-82: The transaction callback passed to
ResultAsync.fromSafePromise in commit uses tx.rollback() which throws
TransactionRollbackError, so the subsequent return err(...) is unreachable and
thrown errors escape the wrapper; replace ResultAsync.fromSafePromise with
ResultAsync.fromPromise (or otherwise catch thrown DB errors) around
db.transaction so thrown TransactionRollbackError and other rejections are
converted into Err<BackendError>, and inside the transaction callback avoid
calling tx.rollback() directly — instead return an Err result (from commitEvent)
and let the transaction resolver handle committing/rolling back; update the
commit function (and similarly read, readSecret, writeSecret, deleteSecret) to
use db.transaction with a promise-returning callback and map thrown errors to
BackendError before returning a Result.
In `@src/backend/pgsql/test/index.test.ts`:
- Around line 30-36: Reorder the teardown deletes in the finally block to remove
child rows before parent rows: run await client.query('delete from endpoints')
and await client.query('delete from secrets') (the child tables) before await
client.query('delete from providers') (the parent), then await client.end();
update the delete sequence in the finally block in
src/backend/pgsql/test/index.test.ts accordingly.
---
Duplicate comments:
In `@src/backend/pgsql/src/index.ts`:
- Around line 208-282: In commitEvent's write-path cases ('endpoint.created' and
'endpoint.updated') the validation error objects for createEndpointHandle and
createEndpointUrl incorrectly set while: 'read'; change those four occurrences
to while: 'write' so the BackendError objects in the create/update branches
reflect the write operation (update the error objects in the cases handling
createEndpointHandle and createEndpointUrl inside the 'endpoint.created' and
'endpoint.updated' switch blocks).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 4d8692d7-700e-4ebc-a378-8ea02bfcd455
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (7)
src/backend/backend/src/backend.tssrc/backend/backend/src/local.test.tssrc/backend/backend/src/local.tssrc/backend/pgsql/package.jsonsrc/backend/pgsql/src/index.tssrc/backend/pgsql/test/index.test.tssrc/backend/remote/src/backend.ts
Comment on lines
+119
to
+130
| const endpoints = await db | ||
| .select() | ||
| .from(schema.endpoints) | ||
| .where(eq(schema.endpoints.providerId, provider.id)) | ||
| if (!endpoints) { | ||
| return err({ | ||
| kind: 'BackendError', | ||
| name: 'NotFoundError', | ||
| message: `no endpoints found for provider name ${providerName}`, | ||
| while: 'read', | ||
| } satisfies BackendError) | ||
| } |
There was a problem hiding this comment.
Dead if (!endpoints) check.
db.select()...where(...) always resolves to an array — possibly empty, but never nullish. This branch is unreachable; if you want to distinguish "provider has no endpoints" from "provider not found", gate on endpoints.length === 0 (though the current read semantics arguably allow an empty provider state). Otherwise, remove the branch.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/backend/pgsql/src/index.ts` around lines 119 - 130, The current null
check against endpoints is dead because db.select(...).where(...) always returns
an array; update the logic in the block that calls
db.select().from(schema.endpoints).where(eq(schema.endpoints.providerId,
provider.id)) to either (A) treat an empty array as "not found" by checking
endpoints.length === 0 and returning the NotFoundError (adjusting the
message/context as needed), or (B) remove the unreachable if branch entirely if
an empty endpoints array should be allowed; make the change where endpoints,
db.select, schema.endpoints, provider.id and providerName are referenced in
index.ts.
Comment on lines
+132
to
+156
| const endpointIndex: EndpointIndex<Provider> = new Map() | ||
| for (const endpoint of endpoints) { | ||
| const handle = createEndpointHandle(endpoint.handle) | ||
| if (handle.isErr()) { | ||
| return err({ | ||
| kind: 'BackendError', | ||
| name: 'InternalError', | ||
| message: 'invalid endpoint handle: ' + handle, | ||
| while: 'read', | ||
| } satisfies BackendError) | ||
| } | ||
| const url = createEndpointUrl(endpoint.url) | ||
| if (url.isErr()) { | ||
| return err({ | ||
| kind: 'BackendError', | ||
| name: 'InternalError', | ||
| message: 'invalid endpoint url: ' + url, | ||
| while: 'read', | ||
| } satisfies BackendError) | ||
| } | ||
| endpointIndex.set(handle.value, { | ||
| url: url.value, | ||
| events: endpoint.events, | ||
| config: endpoint.config, | ||
| }) |
There was a problem hiding this comment.
String-concatenating a Result object into error messages.
handle and url here (and identically in commitEvent at lines 215, 224, 245, 254) are Result instances, not strings. 'invalid endpoint handle: ' + handle stringifies the Result object (typically [object Object] or Err(...)), making the error messages useless for diagnosis. Surface the underlying error instead.
🩹 Suggested fix
- message: 'invalid endpoint handle: ' + handle,
+ message: `invalid endpoint handle: ${String(handle.error)}`,
...
- message: 'invalid endpoint url: ' + url,
+ message: `invalid endpoint url: ${String(url.error)}`,Consider also forwarding the underlying error as cause so it's not lost.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const endpointIndex: EndpointIndex<Provider> = new Map() | |
| for (const endpoint of endpoints) { | |
| const handle = createEndpointHandle(endpoint.handle) | |
| if (handle.isErr()) { | |
| return err({ | |
| kind: 'BackendError', | |
| name: 'InternalError', | |
| message: 'invalid endpoint handle: ' + handle, | |
| while: 'read', | |
| } satisfies BackendError) | |
| } | |
| const url = createEndpointUrl(endpoint.url) | |
| if (url.isErr()) { | |
| return err({ | |
| kind: 'BackendError', | |
| name: 'InternalError', | |
| message: 'invalid endpoint url: ' + url, | |
| while: 'read', | |
| } satisfies BackendError) | |
| } | |
| endpointIndex.set(handle.value, { | |
| url: url.value, | |
| events: endpoint.events, | |
| config: endpoint.config, | |
| }) | |
| const endpointIndex: EndpointIndex<Provider> = new Map() | |
| for (const endpoint of endpoints) { | |
| const handle = createEndpointHandle(endpoint.handle) | |
| if (handle.isErr()) { | |
| return err({ | |
| kind: 'BackendError', | |
| name: 'InternalError', | |
| message: `invalid endpoint handle: ${String(handle.error)}`, | |
| while: 'read', | |
| } satisfies BackendError) | |
| } | |
| const url = createEndpointUrl(endpoint.url) | |
| if (url.isErr()) { | |
| return err({ | |
| kind: 'BackendError', | |
| name: 'InternalError', | |
| message: `invalid endpoint url: ${String(url.error)}`, | |
| while: 'read', | |
| } satisfies BackendError) | |
| } | |
| endpointIndex.set(handle.value, { | |
| url: url.value, | |
| events: endpoint.events, | |
| config: endpoint.config, | |
| }) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/backend/pgsql/src/index.ts` around lines 132 - 156, The error messages
concatenate the Result objects (handle and url) instead of their underlying
errors; when createEndpointHandle or createEndpointUrl returns an Err, include
the actual error (e.g., handle.error or url.error / the Err property provided by
your Result type) in the BackendError message and/or set it as the error's
cause. Update the error returns in the endpoint initialization block (where
endpointIndex is built) and the analogous checks in commitEvent (the checks
around createEndpointHandle/createEndpointUrl at the mentioned lines) to use the
Result's error property rather than stringifying the Result object, and ensure
you still satisfy the BackendError shape.
Comment on lines
+284
to
+311
| case 'endpoint.deleted': { | ||
| const result = await tx | ||
| .delete(schema.endpoints) | ||
| .where( | ||
| and( | ||
| eq(schema.endpoints.providerId, provider.id), | ||
| eq(schema.endpoints.handle, event.handle), | ||
| ), | ||
| ) | ||
| if (result.rowCount === 0) { | ||
| return err({ | ||
| kind: 'BackendError', | ||
| name: 'NotFoundError', | ||
| message: `endpoint ${event.handle} not found`, | ||
| while: 'write', | ||
| } satisfies NotFoundError) | ||
| } | ||
| const providerEvents = await tx | ||
| .select() | ||
| .from(schema.endpoints) | ||
| .where(eq(schema.endpoints.providerId, provider.id)) | ||
| if (providerEvents.length === 0) { | ||
| await tx | ||
| .delete(schema.providers) | ||
| .where(eq(schema.providers.id, provider.id)) | ||
| } | ||
| break | ||
| } |
There was a problem hiding this comment.
Auto-deleting the provider on last endpoint delete is a surprising side effect.
When the final endpoint is deleted, the provider row is also removed. This couples provider lifecycle to endpoints, which:
- breaks referential intent if providers are meant to persist independently (e.g., a later
endpoint.createdfor the same provider races and must re-insert); - is not symmetric with
endpoint.created, which only auto-creates providers; - and can cause surprising
ProviderNotFoundErrorfromreadafter an unrelated delete, even when the caller still considers the provider configured.
The // TODO: we need a better solution for "stale" providers comment at line 97 already acknowledges this. Consider removing the auto-delete here and cleaning up stale providers out-of-band, or gating this on an explicit cleanup policy.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/backend/pgsql/src/index.ts` around lines 284 - 311, The handler for case
'endpoint.deleted' currently auto-deletes the provider by calling
tx.delete(schema.providers) when no endpoints remain, which causes surprising
lifecycle coupling; remove that auto-delete and instead either (a) delete the
tx.delete(schema.providers) call and leave a TODO/log so providers persist
independently, or (b) gate the deletion behind an explicit cleanup policy/config
flag (e.g., a cleanupProvidersOnLastEndpoint flag checked inside the
'endpoint.deleted' case) so deletion only happens when configured; update the
block handling endpoint.deleted (the case 'endpoint.deleted' branch that queries
schema.endpoints and calls tx.delete(schema.providers)) accordingly and adjust
tests to expect providers to remain unless the cleanup policy is enabled.
Comment on lines
+30
to
+36
| } finally { | ||
| await client.query('delete from providers') | ||
| await client.query('delete from endpoints') | ||
| await client.query('delete from secrets') | ||
|
|
||
| await client.end() | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
fd -t f 'schema.ts' src/backend/pgsql/src/db | xargs -I{} cat {}Repository: interlandi-io/hookplane
Length of output: 1038
🏁 Script executed:
#!/bin/bash
# Check the actual test file to see the cleanup block in context
sed -n '25,40p' src/backend/pgsql/test/index.test.tsRepository: interlandi-io/hookplane
Length of output: 507
Delete child records before parent records for clarity and safety.
The foreign key constraints have ON DELETE CASCADE configured, so deletions will succeed in any order. However, it's clearer and safer to delete child records before parent records.
🧹 Suggested order
} finally {
- await client.query('delete from providers')
- await client.query('delete from endpoints')
await client.query('delete from secrets')
+ await client.query('delete from endpoints')
+ await client.query('delete from providers')
await client.end()
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| } finally { | |
| await client.query('delete from providers') | |
| await client.query('delete from endpoints') | |
| await client.query('delete from secrets') | |
| await client.end() | |
| } | |
| } finally { | |
| await client.query('delete from secrets') | |
| await client.query('delete from endpoints') | |
| await client.query('delete from providers') | |
| await client.end() | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/backend/pgsql/test/index.test.ts` around lines 30 - 36, Reorder the
teardown deletes in the finally block to remove child rows before parent rows:
run await client.query('delete from endpoints') and await client.query('delete
from secrets') (the child tables) before await client.query('delete from
providers') (the parent), then await client.end(); update the delete sequence in
the finally block in src/backend/pgsql/test/index.test.ts accordingly.
96888fd to
1d92098
Compare
Minterl
force-pushed
the
feat/backend-pgsql
branch
from
c5699d9 to
96fd512
Compare
Minterl
added a commit
that referenced
this pull request
Apr 22, 2026
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Summary by CodeRabbit
New Features
Infrastructure
Tests
Documentation