fix: wrap multi-step DB operations in PostgreSQL transactions #90
Conversation
Replace fragile manual rollback patterns with atomic RPC functions. All mutation steps now run in a single PostgreSQL transaction — automatic rollback on any failure, zero chance of partial state. - Add 5 RPC functions: move_to_maintenance, restore_to_board, batch_update_statuslist_positions, batch_update_repocard_orders, batch_update_board_positions - All use SECURITY INVOKER to respect existing RLS policies - Refactor repo-cards.ts: replace 3-step insert→update→delete + manual rollback with single RPC call - Refactor board.ts: replace Promise.all N updates with atomic batch RPC calls - Net reduction: -161 lines of fragile rollback code, +110 lines of robust transaction-safe code
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 WalkthroughWalkthroughThis PR replaces multi-step database operations with atomic PostgreSQL RPC functions. Three batch update operations and two complex card management operations (moveToMaintenance, restoreToBoard) transition from manual Promise.all patterns and multi-step SQL with rollback logic to server-side atomic transactions, eliminating consistency risks. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 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 |
🤖 Morph Preview Test⚡ Looks like you hit your rate limits! Please upgrade your limits here, or wait a few minutes and try again. If you need help, reach out to support@morphllm.com. Automated testing by Morph |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #90 +/- ##
=======================================
Coverage 74.33% 74.33%
=======================================
Files 119 119
Lines 3873 3873
Branches 1030 1062 +32
=======================================
Hits 2879 2879
Misses 971 971
Partials 23 23 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/lib/actions/board.ts (1)
569-570:⚠️ Potential issue | 🟡 MinorStale JSDoc: "in parallel" should be "atomically".
Line 569 says "Updates multiple boards' position values in parallel" but the operation is now a single atomic RPC call, not parallel updates.
📝 Suggested doc fix
/** * Batch update board positions for DnD reordering. - * Updates multiple boards' position values in parallel. + * Updates multiple boards' position values atomically via a PostgreSQL RPC. *src/lib/actions/repo-cards.ts (1)
272-294:⚠️ Potential issue | 🟡 MinorOrphaned JSDoc block — not attached to any function.
This JSDoc for
moveToMaintenanceis stranded aboverestoreToBoard's JSDoc+function (lines 296–419). The actualmoveToMaintenanceexport is at line 521. This block documents nothing as-is.Move it directly above line 521, or remove it since the function itself could carry its own doc.
🤖 Fix all issues with AI agents
In `@src/supabase/migrations/20260212_transaction_rpc_functions.sql`:
- Around line 153-158: batch_update_board_positions currently updates only
position for each board row but does not set updated_at like the other batch
functions; either add updated_at = now() to the UPDATE in
batch_update_board_positions (the UPDATE statement inside the FOR v_item IN
jsonb_array_elements(p_updates) loop) to match batch_update_statuslist_positions
and batch_update_repocard_orders, or if this omission is intentional add a brief
comment above the function explaining why position-only updates must not touch
updated_at so reviewers know it’s deliberate.
🧪 E2E Coverage Report (Sharded: 12 parallel jobs)
📊 Full report available in workflow artifacts |
- Add updated_at = now() to batch_update_board_positions for consistency - Fix stale JSDoc: "in parallel" → "atomically via PostgreSQL RPC" - Move orphaned moveToMaintenance JSDoc to actual function location
🤖 Morph Preview Test⚡ Looks like you hit your rate limits! Please upgrade your limits here, or wait a few minutes and try again. If you need help, reach out to support@morphllm.com. Automated testing by Morph |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/supabase/migrations/20260212_transaction_rpc_functions.sql`:
- Around line 1-3: The migration filename uses an 8-digit date; rename
"20260212_transaction_rpc_functions.sql" to the 14-digit Supabase CLI format
like "20260212000000_transaction_rpc_functions.sql" (keep the
"transaction_rpc_functions" slug) and regenerate the migration via the Supabase
CLI (supabase migration new transaction_rpc_functions) or manually rename the
file to match YYYYMMDDHHMMSS_<description>.sql so it follows Supabase migration
ordering and tooling expectations; ensure any references to the old filename in
deployment scripts or changelogs are updated accordingly.
🧹 Nitpick comments (2)
src/lib/actions/repo-cards.ts (1)
566-586: Atomic move-to-maintenance via RPC looks solid.Same pattern as
restoreToBoard. One note:p_user_idis passed as a parameter to the RPC rather than usingauth.uid()inside the SQL function. This is safe here because the value comes from the server-sidesupabase.auth.getUser()call (not client input), and RLS on themaintenancetable should independently enforce ownership. Still, usingauth.uid()directly inside the function would be more defense-in-depth.♻️ Optional: use auth.uid() inside the SQL function
In the migration SQL (
move_to_maintenance), replace thep_user_idparameter usage withauth.uid():CREATE OR REPLACE FUNCTION move_to_maintenance( p_card_id UUID, - p_user_id UUID, p_repo_owner TEXT, p_repo_name TEXT ) ... INSERT INTO maintenance (user_id, repo_owner, repo_name) - VALUES (p_user_id, p_repo_owner, p_repo_name) + VALUES (auth.uid(), p_repo_owner, p_repo_name) RETURNING id INTO v_maint_id;Then drop
p_user_idfrom the TS call site as well.src/supabase/migrations/20260212_transaction_rpc_functions.sql (1)
89-108: Batch update functions silently skip non-matching IDs.All three batch functions (
batch_update_statuslist_positions,batch_update_repocard_orders,batch_update_board_positions) iterate the JSONB array and run UPDATE statements, but if an ID doesn't match any row (e.g., stale UUID, RLS filters it out), the update silently succeeds with zero affected rows. The caller gets no indication that some items weren't updated.This is probably fine for position reordering (the TS layer validates inputs), but worth documenting. If you want stronger guarantees, you could check
FOUNDafter each UPDATE and raise an exception.Also applies to: 116-135, 143-161
Rename 20260212_ → 20260212000000_ to follow Supabase CLI convention
🤖 Morph Preview Test⚡ Looks like you hit your rate limits! Please upgrade your limits here, or wait a few minutes and try again. If you need help, reach out to support@morphllm.com. Automated testing by Morph |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@src/supabase/migrations/20260212000000_transaction_rpc_functions.sql`:
- Around line 35-36: After deleting the repocard row using DELETE FROM repocard
WHERE id = p_card_id, verify that the DELETE actually removed a row (e.g., check
FOUND or use GET DIAGNOSTICS row_count) and if zero rows were affected RAISE an
exception to abort the transaction so the previously inserted maintenance record
(inserted earlier in this RPC) is rolled back; reference the repocard table, the
p_card_id parameter and the earlier INSERT that creates the maintenance record
when adding the check and RAISE.
- Around line 76-77: The DELETE from maintenance using p_maintenance_id can be a
no-op and leave an orphaned maintenance row after the repocard insert; after
performing DELETE FROM maintenance WHERE id = p_maintenance_id, check that a row
was actually removed (e.g. DELETE ... RETURNING id INTO var OR use GET
DIAGNOSTICS / ROW_COUNT) and if no row was deleted, RAISE an exception to abort
and roll back the transaction so the repocard creation (the earlier insert that
mirrors move_to_maintenance behavior) cannot leave inconsistent data.
- Around line 9-17: Add a SET search_path clause to each SECURITY INVOKER RPC
function header in this file to ensure PostgREST resolves unqualified table
names consistently; specifically, modify the function signatures (e.g.,
move_to_maintenance and the other four RPC functions) to include SET search_path
= pg_catalog, public in the function header (i.e., alongside SECURITY INVOKER)
so the header becomes: ... RETURNS ... LANGUAGE plpgsql SECURITY INVOKER SET
search_path = pg_catalog, public ... before the function body.
src/supabase/migrations/20260212000000_transaction_rpc_functions.sql
Outdated
Show resolved
Hide resolved
src/supabase/migrations/20260212000000_transaction_rpc_functions.sql
Outdated
Show resolved
Hide resolved
- Add SET search_path = pg_catalog, public to all 5 RPC functions - Add IF NOT FOUND guards after DELETE in move_to_maintenance - Add IF NOT FOUND guards after DELETE in restore_to_board
🤖 Morph Preview Test⚡ Looks like you hit your rate limits! Please upgrade your limits here, or wait a few minutes and try again. If you need help, reach out to support@morphllm.com. Automated testing by Morph |
2 participants
Summary
Closes #61
moveToMaintenanceandrestoreToBoardwith singlesupabase.rpc()callsPromise.allN individual updates in 3 batch functions with atomic batch RPC callsSECURITY INVOKERto respect existing RLS policiesRPC Functions Added
move_to_maintenancerestore_to_boardbatch_update_statuslist_positionsbatch_update_repocard_ordersbatch_update_board_positionsKey Design Decisions
projectinfo_target_exclusive_checkrequires exactly one ofrepo_card_id/maintenance_idnon-null. The single UPDATE statement in move/restore RPCs sets both columns atomically — PostgreSQL evaluates CHECK at statement boundary, so the final state passes.Test plan
pnpm typecheck— cleanpnpm lint— 0 warningspnpm build— successfulpnpm test— 85 files, 1202 tests passedpnpm e2e— 317 passed, 1 skipped (pre-existing), 0 failedpnpm db:resetagainst local Supabase to verify migration appliesSummary by CodeRabbit
Bug Fixes
Performance