feat(escrow): idempotency keys + worker withdrawal + account-dialog hardening - #20
Conversation
- withdraw_wallet: add is_worker() check (A), p_amount NULL guard (F), p_idempotency_key NULL guard (C), insert-first/catch unique_violation pattern mirroring topup (D), public. schema qualifier (I), w. table alias to disambiguate available_balance vs RETURNS TABLE column - worker-form: replace as any cast with typed cast to fix pre-existing lint error surfaced during PR #19 pipeline - guard_milestones_status + guard_jobs_status: replace GUC bypass with direct session_user='postgres' check (E) - auto_release_milestones: drop set_config GUC line (E), add RAISE NOTICE on insufficient-locked-balance skip (H) - wallet.ts: mapWalletRpcError helper routing by error.code + exact RAISE EXCEPTION token (G), defense-in-depth is_worker() pre-check in withdrawWalletAction (A), raw error logging before generic fallback - topup-dialog + withdraw-dialog: stable idempotencyKey via useState initializer + useEffect reset on close (B), normalize 4-space to 2-space indent on withdraw-dialog (Copilot #9)
- uuid.ts: remove Math.random fallback, throw if secure crypto API unavailable; collision resistance is a correctness requirement for wallet idempotency keys (CodeRabbit follow-up #2) - docs/adr/0005: ADR approving in-place modification of auto_release_milestones per Copilot #1 (drop GUC bypass) and CodeRabbit #3 (RAISE NOTICE on silent skip), as required by ADR 0004 - pr19_review_fixes.sql: add ADR reference comment above auto_release_milestones rewrite for discoverability (CodeRabbit follow-up #3) Deferred to PR #20: Sentry.captureException wiring (CodeRabbit follow-up #1) — Sentry SDK installation is the dedicated scope of PR #20 per the roadmap; adding capture calls now would not compile.
- onboarding/actions.ts: remove PII console.log (fullName, categories, skillTags) that leaked KYC data into server logs on every onboarding attempt (CodeAnt Critical, #discussion_r3280686499) - onboarding/actions.ts: clean up orphaned selfie from kyc bucket when profiles or worker_profiles upsert fails after upload succeeds (CodeAnt Major, #discussion_r3280686511) - client/jobs/[id]/milestones/client-milestones.tsx: scope wallet fetch by profile_id of authenticated user instead of returning arbitrary first visible wallet row; prevents wrong-balance display and funding decisions if RLS ever surfaces multiple wallets (CodeAnt Critical, #discussion_r3280693926) - worker/feed/worker-feed.tsx: gate useInfiniteQuery on auth-ready via enabled:!!user?.id and scope queryKey by user?.id; prevents spurious unauthorized errors on initial mount before auth propagates (CodeAnt Major, #discussion_r3280707356) - worker/jobs/[id]/worker-job-detail.tsx: surface errors from msRes, matRes, appRes in Promise.all instead of silently coercing to empty arrays; CodeAnt flagged matRes specifically but the same pattern affected msRes and appRes (CodeAnt Major, #discussion_r3280707371)
- _actions/wallet.ts (topup+withdraw catch): replace raw err.message return with fixed generic copy to prevent backend internals (SQL, Supabase, runtime details) from being toasted to the user; server logs retain the full error via console.error (CodeAnt Major, #discussion_r3283709865, #discussion_r3283709876) - client/jobs/[id]/milestones/client-milestones.tsx: move auth resolution out of the fetcher; ClientMilestones now passes user.id into fetchMilestonesData and the useQuery is gated by enabled on user.id with user.id added to queryKey, matching the worker-feed pattern (CodeAnt Major, #discussion_r3283709880) - client-milestones.tsx: surface msRes.error and walletRes.error (with PGRST116 no-rows exception preserving zero-balance fallback) instead of silently rendering empty milestones / zero wallet on query failure (CodeRabbit Major, outside-diff line 91) - client-milestones.tsx + worker/jobs/[id]/milestones/worker-milestones.tsx: migrate idempotency_key generation from crypto.randomUUID to generateUuid so LAN/HTTP origins without secure context do not TypeError on submission (CodeRabbit Major, outside-diff line 208) - onboarding/actions.ts: capture and console.error storage cleanup failures in profileError and workerError branches; orphan accumulation is now observable in server logs pending Sentry wiring (CodeRabbit Nitpick, lines 148-171) - worker/feed/worker-feed.tsx: remove leftover WorkerFeed DEBUG console.log that fired on every render and dumped paginated job data to the browser console; also dropped the now-unused queryStatus destructure that the log was its only consumer (CodeRabbit Nitpick, line 148)
…on style Fix 1 - escrow.ts: add mapEscrowRpcError helper (mirrors mapWalletRpcError); replace raw error.message returns and raw outer-catch msg with user-friendly strings + structured console.error across fund/submit/approve/dispute actions. Fix 2 - worker-milestones.tsx: scope wallet query to profile_id, add enabled on user.id and user.id to queryKey in useQuery (CodeRabbit Major + CodeAnt Critical sibling-pattern follow-up to client-milestones.tsx). Fix 3 - client-job-detail.tsx: add enabled on user.id and user.id to queryKey in useQuery to prevent cold-start firing before auth resolves. Fix 4 - 20260518135600_withdraw_wallet.sql: rename unique index to idx_escrow_ledger_withdraw_owner_reference per repo idx_table_purpose naming convention (Copilot Medium). Fix 5 - 20260518181500_withdraw_wallet_fix_ambiguous_column.sql: add public. schema qualifier to function definition and GRANT; change search_path style to set search_path to 'public' for consistency with other SECURITY DEFINER functions (Copilot Med/Low x3). Fix 6 - worker-form.tsx: replace className block/hidden step mounting with conditional render so only the active Step component is mounted; hooks and subscriptions on inactive steps no longer run (CodeAnt Major perf). All form state hoisted in FormProvider so unmount is safe. Fix 7 - 20260521080000_pr19_review_fixes.sql: expand header comment to explicitly document that pakka.allow_milestone_status_change GUC was DROPPED entirely, and enumerate the three allowed guard bypass conditions (SECURITY DEFINER, postgres superuser/pg_cron, is_admin).
…ation UX, count aggregates Fix 1 - 20260521080000_pr19_review_fixes.sql: reorder withdraw_wallet to INSERT into escrow_ledger BEFORE acquiring the wallet FOR UPDATE lock. Race-losers now deterministically hit the function-level EXCEPTION WHEN unique_violation handler instead of racing past the pre-check and hitting insufficient_balance. Removes the now-redundant pre-check SELECT fast-path (the INSERT/unique_violation handles it). Fix 2 - docs/state-machine.md: update two Edge Function cron references to pg_cron (database scheduler) at transition 5 caller and Key Callers table, matching the scheduling mechanism introduced in 20260518225500. Fix 3 - client-milestones.tsx: add !user?.id guard before isLoading and error checks so auth hydration renders MilestonesSkeleton instead of the error UI. Fix 4 - client-job-list.tsx: replace milestones(id)/job_applications(id) full ID-array fetches with milestones_count:milestones(count) / applications_count:job_applications(count) Supabase aggregate syntax; update mapping to read [0]?.count ?? 0 instead of .length.
- adr/0006-nav-shell.md: strip indent from Decision:/Consequences: labels - worker/account/page.tsx: replace null blank-screen with empty-state card + retry - features/withdraw-dialog.tsx: use formatInr(100) instead of hardcoded '₹100' - _actions/escrow.ts: correct mapEscrowRpcError JSDoc (routes by error.message, not SQLSTATE) - worker-milestones.tsx: throw on msRes.error; throw on walletRes.error unless PGRST116 - worker-milestones.tsx: replace err.message leak in handleSubmit catch with fixed user-safe toast - worker/feed/worker-feed.tsx: milestones(id) full-array -> milestones_count:milestones(count) aggregate 16 other findings deferred: - PR #20 (Sentry): escrow.ts 8 capture sites - PR #22 (canonical dialog migration): apply-modal, change-phone-dialog, delete-account-dialog, edit-profile-dialog, worker-form, client-milestones escrow handlers, withdraw-dialog Zod/optimistic - PR #23 (realtime auth-hydration): client-milestones setAuth - Reply-dismiss: client/worker-milestones auth guards (middleware-guarded routes)
…tion x3, idempotency-key map x2 - post-job-form.tsx: extract parseFiniteFloat helper; apply to all 5 numeric onChange (budget, lat, lng, milestone amounts, material qty/amount) so transient '-' or '.' inputs write undefined not NaN into form state. - seed-demo-users.ts: correct wallet locked_balance to match seeded ledger: Priya 40k -> 60k (J1-M2 funded), Rohit 0 -> 20k (J2-M1 submitted), Anjali 120k -> 30k (J3-M2 disputed). Comments reference source ledger rows. - worker-milestones.tsx: add // TODO: Sentry.captureException(err) as first line of handleSubmit catch block (canonical grep target for PR #20 wiring). - client-job-detail.tsx / client-milestones.tsx / worker-milestones.tsx: destructure isAuthLoading from useUser(); gate skeleton on (isAuthLoading || !user?.id) before the data/error branch so the hydration boundary no longer renders a false 'Failed to load' error path. - client-milestones.tsx + worker-milestones.tsx: introduce idempotencyKeysRef (Map<string, string>) with getOrCreateIdempotencyKey and clearIdempotencyKey helpers. fund/approve/dispute (client) and handleSubmit (worker) now reuse the same key across retries within a single user intent and rotate only on definitive success. Closes the client side of the idempotency contract; server-side uniqueness enforcement on the escrow RPCs is tracked separately in the Phase 4.5 escrow idempotency PR. Refs round-8 review: CodeAnt CRITICAL x3 (idempotency), CodeAnt HIGH (auth hydration), CodeAnt Major x3, CR Minor.
- adr/0001-auth.md, 0002-escrow.md, 0003-dispute.md, 0005-realtime-contract.md, 0007-rpc-pattern.md: strip 2-space indent on Decision/Consequences headings so they render as top-level sections matching adr/template.md. - adr/0004-auto-release.md: same indent fix + rewrite Decision and Consequences to reflect pg_cron (not Edge Function cron) as the actual scheduler; reference migration 20260518225500_schedule_auto_release_cron.sql as source of truth. Round-9 #32/#33/#34 (CR Realtime setAuth on worker-milestones, client-milestones, client-job-detail): no code change. The singleton in src/lib/supabase/client.ts already calls realtime.setAuth() on both eager-prime and onAuthStateChange (resetting to anon key on sign-out, not null). Per-file setAuth wiring is unnecessary. Round-9 #41 + parallel client-milestones/withdraw idempotency findings: no code change. The escrow RPCs (submit_milestone, fund_escrow, approve_milestone, dispute_milestone) do not accept p_idempotency_key in their SQL signatures, so action-layer forwarding has nothing to bind to. RPC-level uniqueness enforcement is tracked in the Phase 4.5 escrow idempotency PR; the per-intent client-side key from round-8 Fix 7/8 is the application-layer half of that contract. Refs round-9 review: Copilot Medium x1 (auto-release doc drift), Copilot Low x5 (ADR heading indent), CR Nitpick x3 (Realtime setAuth - no-op), CodeAnt Major x1 (idempotency forwarding - deferred).
…e (round-10)
Fix 1/2 - worker-milestones.tsx + client-milestones.tsx: rewrite the
per-intent idempotency map comment to remove the overpromising
"server can deduplicate" claim. New wording: "forward-compatible with
server-side RPC idempotency". Grep confirmed exactly 2 occurrences
across the codebase; no helper JSDoc affected.
Fix 3 - wallet.ts: tighten mapWalletRpcError error parameter from an
ad-hoc structural type to PostgrestError. Mirrors the escrow.ts pattern
established in round-5. Import added from @supabase/supabase-js. Pure
type-tightening: supabase-js v2 (^2.104.1) types PostgrestError.code as
string (non-nullable), so existing error.code === "42501" / "22023"
comparisons remain valid without narrowing. Both call sites
(mapWalletRpcError("topup", err), mapWalletRpcError("withdraw", err))
already pass a PostgrestError.
Refs round-10 review: Copilot Medium x3. Round-10 Low (migration
bundling, _pr19_review_fixes.sql) reply-dismissed: the consolidation
is an intentional review artifact, section headers A/C/D/F/I/E/H
already document the per-concern split internally, and retroactively
splitting an applied migration on an active branch is destructive.
…), withdraw_wallet exception scope, ADR cron alignment
- worker/account/page.tsx: destructure error from supabase.auth.getUser();
surface a retryable error state on transient auth/network failure
instead of redirecting signed-in users to /login. Sentry TODO marker
added.
- worker-milestones.tsx, client-job-detail.tsx: split the combined
isAuthLoading || !user?.id branch into two — skeleton during hydration,
router.replace('/login') + return null once auth has resolved with no
user. useRouter import added to both. Belt-and-suspenders over the
middleware route guard.
- client-milestones.tsx: upgrade the post-round-8 split so the !user?.id
branch also router.replace('/login') + return null instead of rendering
the skeleton indefinitely. Consistent with the two files above.
- 20260521080000_pr19_review_fixes.sql (withdraw_wallet): scope the
unique_violation handler to only the INSERT INTO public.escrow_ledger
via a nested BEGIN..EXCEPTION block. Other UNIQUE violations (present
or future) now surface as errors instead of being misclassified as
idempotency replays.
- adr/0004-auto-release.md: correct the cron expression in the Decision
text to 30 20 * * * (20:30 UTC / 02:00 IST, daily) — matches the
actual schedule registered in
20260518225500_schedule_auto_release_cron.sql, which is the source of
truth.
Refs round-11 review: CodeAnt Major x3 (auth error, auth-hydration x2),
Copilot Medium x2 (cron mismatch, withdraw_wallet exception scope).
Round-11 CodeAnt CRITICAL x1 + Major x1 (idempotency key forwarding on
submit/fund) reply-dismissed in PR threads: see Phase 4.5 escrow
idempotency PR.
… race guard (round-12)
Fix 1 - worker-milestones.tsx, client-milestones.tsx, client-job-detail.tsx:
move router.replace("/login") out of the render path and into a
useEffect([isAuthLoading, user, router]). Render guard collapses back to
the combined `isAuthLoading || !user?.id` -> <Skeleton /> shape, so the
user sees a skeleton both during auth hydration and during the
post-resolution navigation tick. No flash of stale UI; no React
render-phase side-effect warning. Dependency array uses `user` (whole
object) rather than `user?.id` to avoid the exhaustive-deps lint warning
about optional chaining in deps. Closes the regression from round-11
Fix 2/3.
Fix 2 - worker/account/page.tsx: clear local profile and worker state
(setProfile(null), setWorker(null)) before router.replace("/login") in
load()'s !user branch, so stale private data cannot flash between
sign-out and navigation.
Fix 3 - worker/account/page.tsx: add a requestIdRef race-condition guard
to load(). Increments at call start; checks before every setState after
each await (post-getUser, post-Promise.all). Concurrent triggers
(initial mount, Retry, onSaved, onChanged) can no longer have an older
in-flight request overwrite fresher state. Pattern is dependency-free;
no AbortController used because supabase-js v2 doesn't support signal
forwarding on .from() queries.
Refs round-12 review: CodeAnt Major x4 (3 render-phase redirects, 1
stale state, 1 race condition; the render-phase finding was applied
proactively to worker-milestones.tsx as well even though CodeAnt only
flagged it on the two client files).
Round-12 CodeAnt CRITICAL x1 (client-milestones.tsx:238 fund
idempotency forwarding) reply-dismissed and conversation resolved in
PR thread: see Phase 4.5 escrow idempotency PR.
|
CodeAnt AI is reviewing your PR. Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds DB idempotency and idempotency-key RPC signatures, wires idempotency keys through server actions, adds deterministic RPC error mapping, caches idempotency keys client-side with realtime invalidation, hardens UI dialogs and account flows for async lifecycle safety, and updates generated DB types and docs. ChangesEscrow idempotency and wallet withdrawal
Server actions, client caching, and account dialogs
Estimated code review effort: Possibly related PRs
🚥 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 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 |
codeant-ai
Bot
added
the
size:XXL
There was a problem hiding this comment.
Pull request overview
This PR closes an end-to-end gap in escrow idempotency by updating the Postgres escrow RPC signatures to accept an idempotency key and wiring client-generated keys through server actions into the .rpc() calls. It also adds supporting DB constraints (partial unique indexes), a safe UUID generator for insecure dev contexts, and includes a wallet withdrawal flow plus widespread formatting/UX hardening.
Changes:
- Add a new Supabase migration that updates the 4 escrow RPC signatures, adds partial unique indexes, and implements DB-side idempotency checks/guards.
- Update server actions + client milestone UIs to consistently generate and pass idempotency keys through to the DB layer (using a shared
generateUuid()helper). - Add/extend wallet withdraw functionality (Zod schema, server action, withdraw dialog) and apply various UI/query/auth hygiene improvements.
Reviewed changes
Copilot reviewed 72 out of 78 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| supabase/migrations/20260603100000_escrow_idempotency.sql | Escrow idempotency migration + RPC signature changes |
| supabase/migrations/20260521080000_pr19_review_fixes.sql | Prior PR fixes migration (withdraw/guards/auto-release) |
| supabase/migrations/20260518225500_schedule_auto_release_cron.sql | pg_cron scheduling migration (context) |
| supabase/migrations/20260518181500_withdraw_wallet_fix_ambiguous_column.sql | Withdraw RPC fix migration (context) |
| supabase/migrations/20260518135600_withdraw_wallet.sql | Withdraw RPC initial migration (context) |
| supabase/functions/auto-release/index.ts | Edge function response formatting |
| supabase/functions/auto_release_milestones/index.ts | Remove unused scaffold function |
| src/lib/uuid.ts | Add safe UUID generator utility |
| src/lib/types/database.ts | Type formatting updates |
| src/lib/supabase/server.ts | Cookie setter formatting tweak |
| src/lib/supabase/client.ts | Auth change logging formatting |
| src/lib/schemas/wallet.ts | Add withdraw wallet Zod schema |
| src/lib/schemas/onboarding.ts | Onboarding schema formatting |
| src/lib/schemas/jobs.ts | Jobs schema formatting |
| src/lib/format.ts | Remove stray trailing whitespace |
| src/lib/escrow-machine.ts | Formatting-only refactor |
| src/lib/tests/escrow.property.test.ts | Formatting-only refactor |
| src/hooks/use-user.ts | Auth subscription formatting/structure |
| src/components/ui/textarea.tsx | UI component formatting/semicolons |
| src/components/ui/status-badge.tsx | UI component formatting |
| src/components/ui/skeleton.tsx | UI component formatting |
| src/components/ui/separator.tsx | UI component formatting |
| src/components/ui/progress.tsx | UI component formatting |
| src/components/ui/dialog.tsx | UI component formatting |
| src/components/ui/badge.tsx | UI component formatting |
| src/components/ui/alert-dialog.tsx | UI component formatting |
| src/components/nav/worker-nav-shell.tsx | Nav tab formatting |
| src/components/nav/client-nav-shell.tsx | Nav tab formatting |
| src/components/features/withdraw-dialog.tsx | New withdraw dialog UI |
| src/components/features/wallet-view.tsx | Show topup/withdraw based on role |
| src/components/features/topup-dialog.tsx | Persist idempotency key per dialog open |
| src/components/account/sign-out-button.tsx | Formatting-only refactor |
| src/components/account/edit-profile-dialog.tsx | Formatting-only refactor |
| src/components/account/delete-account-dialog.tsx | Formatting-only refactor |
| src/components/account/change-phone-dialog.tsx | Formatting-only refactor |
| src/app/worker/layout.tsx | Layout formatting |
| src/app/worker/jobs/[id]/worker-job-detail.tsx | Add missing error checks + formatting |
| src/app/worker/jobs/[id]/milestones/worker-milestones.tsx | Idempotency keys + auth redirect + wallet query fix |
| src/app/worker/jobs/[id]/milestones/page.tsx | Fix back-link path + params handling |
| src/app/worker/jobs/[id]/apply-modal.tsx | Formatting-only refactor |
| src/app/worker/feed/worker-feed.tsx | Use aggregate counts + query gating |
| src/app/worker/applications/worker-applications.tsx | Realtime formatting + select formatting |
| src/app/worker/account/page.tsx | Race-safe loader + improved auth error handling |
| src/app/page.tsx | Formatting-only tweak |
| src/app/onboarding/worker/worker-form.tsx | Conditional rendering + formatting |
| src/app/onboarding/actions.ts | Cleanup selfie file on downstream DB failure |
| src/app/login/actions.ts | Remove extra blank lines |
| src/app/client/layout.tsx | Layout formatting |
| src/app/client/jobs/new/post-job-form.tsx | Guard float parsing + formatting |
| src/app/client/jobs/client-job-list.tsx | Use aggregate counts (milestones/apps) |
| src/app/client/jobs/[id]/milestones/page.tsx | Fix back-link path + params handling |
| src/app/client/jobs/[id]/milestones/client-milestones.tsx | Idempotency keys + auth redirect + wallet query fix |
| src/app/client/jobs/[id]/fund/page.tsx | Formatting-only tweak |
| src/app/client/jobs/[id]/client-job-detail.tsx | Auth redirect pattern + realtime formatting |
| src/app/client/account/page.tsx | Formatting-only refactor |
| src/app/_actions/wallet.ts | Add withdraw action + deterministic RPC error mapping |
| src/app/_actions/jobs.ts | Formatting-only refactor |
| src/app/_actions/escrow.ts | Pass idempotency keys to RPC + friendly error mapping |
| scripts/seed-demo-users.ts | Seed data formatting + locked-balance adjustments |
| README.md | Demo users table formatting |
| docs/state-machine.md | Clarify auto-release caller + formatting |
| docs/data-model.md | Formatting-only cleanup |
| docs/adr/0006-escrow-idempotency.md | New ADR documenting idempotency design |
| docs/adr/0005-auto-release-milestones-modification.md | New ADR documenting prior change rationale |
| docs/adr/0004-migration-discipline.md | Formatting-only tweak |
| docs/adr/0003-auth-state-hygiene.md | Formatting-only tweak |
| docs/adr/0002-rls-and-security-definer-as-only-escrow-path.md | Formatting-only tweak |
| CLAUDE.md | Formatting-only tweak |
| AGENTS.md | Formatting-only tweak |
| adr/template.md | Formatting-only tweak |
| adr/0007-rpc-pattern.md | Formatting-only tweak |
| adr/0006-nav-shell.md | Formatting-only tweak |
| adr/0005-realtime-contract.md | Formatting-only tweak |
| adr/0004-auto-release.md | Update decision to pg_cron approach |
| adr/0003-dispute.md | Formatting-only tweak |
| adr/0002-escrow.md | Formatting-only tweak |
| adr/0001-auth.md | Formatting-only tweak |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| begin | ||
| select m.job_id, j.client_id, m.amount, m.status |
There was a problem hiding this comment.
Fixed in d2c132b — all 4 escrow functions now reject NULL p_idempotency_key with RAISE EXCEPTION 'invalid_idempotency_key' USING ERRCODE = '22023', matching the existing pattern in topup_wallet/withdraw_wallet. Also applied to the remote dev DB.
There was a problem hiding this comment.
Resolved — NULL keys are rejected with RAISE EXCEPTION 'invalid_idempotency_key' since d2c132b. Verified on remote DB via behavioral smoke test (SQLSTATE 22023).
| begin | ||
| select m.job_id, j.worker_id, m.status |
There was a problem hiding this comment.
Fixed in d2c132b — submit_milestone now rejects NULL keys with the same errcode/message. Added a comment explaining the key is accepted for API-consistency (the actual idempotency guard relies on milestone status).
There was a problem hiding this comment.
Resolved — same NULL guard applied since d2c132b.
| begin | ||
| select m.job_id, j.client_id, j.worker_id, m.amount, m.status |
There was a problem hiding this comment.
Fixed in d2c132b — approve_milestone now rejects NULL keys before the idempotency replay check. Applied to the remote dev DB and verified via pg_proc source inspection.
There was a problem hiding this comment.
Resolved — same NULL guard applied since d2c132b.
| begin | ||
| if p_reason is null or btrim(p_reason) = '' then | ||
| raise exception 'Reason is required'; | ||
| end if; |
There was a problem hiding this comment.
Fixed in d2c132b — dispute_milestone now rejects NULL keys at the top of the function body, before the reason check. Consistent errcode 22023 / message invalid_idempotency_key across all 5 RPCs.
There was a problem hiding this comment.
Resolved — same NULL guard applied since d2c132b.
|
CodeAnt AI finished reviewing your PR. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/lib/escrow-machine.ts (1)
7-14:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRemove stale
"approved"milestone status from the simulator contract.This simulator declares it mirrors Postgres exactly, but
MilestoneStatusstill includes"approved"while transitions and current escrow contract use"released". Keeping dead states in the model weakens property-test fidelity.Suggested fix
export type MilestoneStatus = | "pending" | "funded" | "submitted" - | "approved" | "released" | "disputed" | "refunded";Based on learnings, "Do not modify the protected financial state machine RPCs ... modifications require an ADR" and simulator contracts should stay aligned with the defined state machine behavior.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/escrow-machine.ts` around lines 7 - 14, The MilestoneStatus union in src/lib/escrow-machine.ts contains a stale "approved" variant that no longer exists in the real escrow state machine; update the type declaration for MilestoneStatus to remove the "approved" string literal, then search for any references to "approved" (tests, simulator transitions, or helpers) and replace or map them to "released" where appropriate to keep the simulator contract aligned with the real contract behavior (focus on the MilestoneStatus type and any functions that validate or transition milestone states).AGENTS.md (1)
23-30:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winPhase status here conflicts with
README.md.
AGENTS.mdnow reflects Phase 3 done / Phase 4 in progress (consistent with this escrow PR), butREADME.mdstill advertises "Phase 2 of 8". Flagging the staleREADME.mdstatus block separately; please keep the two in sync.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@AGENTS.md` around lines 23 - 30, AGENTS.md and README.md disagree on phase status (AGENTS.md shows Phase 3 done / Phase 4 in progress while README.md still says "Phase 2 of 8"); pick the authoritative source (update README.md to match AGENTS.md or update AGENTS.md if that was unintended) and make the status blocks consistent—specifically update the phase status text in README.md to reflect "Phase 3 (jobs CRUD + browse + apply): done" and "Phase 4 (escrow state machine): IN PROGRESS" (or mirror the chosen correct text), ensuring any commit references or badges that display phase number are updated accordingly.README.md (1)
6-6:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winStale project status — should track Phase 4.
The header (and the Status checklist at Lines 135-143) still says "Phase 2 of 8" with Phase 2 "in progress", but
AGENTS.mdand this escrow PR put the project at Phase 4 in progress. For a recruiter-facing README this mismatch is worth correcting.Separately, the architecture diagram at Lines 40-42 still attributes auto-release to Edge Functions, whereas this stack moves auto-release scheduling to pg_cron — consider updating it in the same pass.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@README.md` at line 6, Update the README header and status checklist to reflect the current project phase by changing the text "Phase 2 of 8" and any "Phase 2" items to "Phase 4 of 8" / "Phase 4 (in progress)" so the header line containing "🔗 **Live demo:**" and the Status checklist entries match AGENTS.md and the escrow PR; also edit the architecture diagram text that currently attributes auto-release to "Edge Functions" and replace it with "pg_cron" (or add a note that auto-release scheduling is handled by pg_cron) so the diagram lines referencing Edge Functions are accurate.
🧹 Nitpick comments (7)
src/app/onboarding/worker/worker-form.tsx (1)
107-112: ⚡ Quick winRevoke the object URL to avoid leaking blob URLs on step navigation.
URL.createObjectURLis never revoked. Combined with the new conditional step mounting (Lines 335-339),Step3unmounts/remounts whenever the user navigates away and back, leaking a fresh selfie blob URL each time. Move creation into an effect with cleanup.♻️ Proposed fix using an effect with cleanup
- const selfieFile = watch("selfie") as File | undefined; - - const previewUrl = useMemo(() => { - if (selfieFile instanceof File) { - return URL.createObjectURL(selfieFile); - } - return null; - }, [selfieFile]); + const selfieFile = watch("selfie") as File | undefined; + const [previewUrl, setPreviewUrl] = useState<string | null>(null); + + useEffect(() => { + if (!(selfieFile instanceof File)) { + setPreviewUrl(null); + return; + } + const url = URL.createObjectURL(selfieFile); + setPreviewUrl(url); + return () => URL.revokeObjectURL(url); + }, [selfieFile]);Add
useEffectto the React import (Line 3).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/app/onboarding/worker/worker-form.tsx` around lines 107 - 112, The previewUrl created with URL.createObjectURL inside the useMemo (previewUrl, selfieFile, useMemo) is never revoked, leaking blob URLs when Step3 unmounts/remounts; replace the useMemo with a useEffect that creates the object URL when selfieFile is a File and stores it in a local state (useState for previewUrl), revoke the previous URL before creating a new one and call URL.revokeObjectURL(previewUrl) in the effect cleanup so every created blob URL is cleaned up; also update the React import to include useEffect/useState.supabase/migrations/20260603100000_escrow_idempotency.sql (1)
143-200: 💤 Low valueUnused
p_idempotency_keyparameter insubmit_milestone.The function accepts
p_idempotency_key uuidbut never uses it. The idempotency logic relies solely on milestone status (if v_status = 'submitted' then return), which is correct behavior, but the unused parameter suggests either:
- Intentional API consistency with the other escrow RPCs (acceptable), or
- A missing implementation (e.g., storing the key in a tracking table).
If this is intentional for signature uniformity, consider adding a brief comment to clarify.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@supabase/migrations/20260603100000_escrow_idempotency.sql` around lines 143 - 200, The function submit_milestone declares p_idempotency_key but never uses it; either persist the key for true idempotency tracking (e.g., insert into an idempotency table keyed by p_idempotency_key and p_milestone_id before performing updates) or, if the unused parameter is intentional for API consistency, add a concise inline comment in submit_milestone noting that p_idempotency_key is unused by design for signature uniformity to avoid confusion (reference p_idempotency_key and submit_milestone to locate the change).src/app/_actions/wallet.ts (1)
137-152: 💤 Low valueMinor: Comment references incorrect tool name.
Line 140 mentions "CodeAnt
#1" but the review tool for this repository is CodeRabbit. While this doesn't affect functionality, updating the comment improves accuracy for future reference.📝 Suggested correction
// Defense-in-depth: explicit worker-role pre-check. The DB layer also // enforces this inside withdraw_wallet via // `if not public.is_worker() then raise exception 'forbidden_role' ...` - // (PR `#19` review, CodeAnt `#1`: addresses missing role check at action layer.) + // (PR `#19` review: addresses missing role check at action layer.) const { data: isWorker, error: roleErr } = await supabase.rpc("is_worker");🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/app/_actions/wallet.ts` around lines 137 - 152, Update the inline comment that currently reads "CodeAnt `#1`" to the correct tool name "CodeRabbit `#1`" in the wallet role-check block that calls supabase.rpc("is_worker") (the defense-in-depth comment referencing withdraw_wallet and the RPC check); keep the rest of the comment intact and only change the tool name to ensure accurate references.src/lib/schemas/wallet.ts (1)
12-17: ⚡ Quick winConsider adding a maximum withdrawal limit for consistency and defense-in-depth.
The
withdrawWalletSchemaenforces a minimum of ₹100 but has no maximum limit, unliketopupWalletSchemawhich caps at ₹1,00,000. While the database functionwithdraw_walletchecksavailable_balance, adding a reasonable schema-level max (e.g., ₹5,00,000 or ₹10,00,000) provides:
- Consistency with the top-up validation pattern
- Defense against UI bugs or accidental large withdrawal attempts
- Earlier validation feedback before hitting the database
♻️ Suggested enhancement
export const withdrawWalletSchema = z.object({ amount: z .number({ invalid_type_error: "Amount must be a number." }) - .min(100, "Minimum withdrawal is ₹100."), + .min(100, "Minimum withdrawal is ₹100.") + .max(500000, "Maximum withdrawal is ₹5,00,000."), idempotency_key: z.string().uuid(), });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/schemas/wallet.ts` around lines 12 - 17, The withdrawWalletSchema currently enforces a minimum but no maximum; update the zod schema for withdrawWalletSchema to include a .max(...) constraint (suggested value ₹500,000 or ₹1,000,000) on the amount field so it mirrors topupWalletSchema's upper bound, provides early validation, and prevents accidental huge withdrawals; locate the amount definition inside withdrawWalletSchema and add the appropriate .max(500000, "Maximum withdrawal is ₹5,00,000.") (or chosen limit) to the validation chain.docs/state-machine.md (1)
8-8: 💤 Low valueOptional: annotate the dead
'approved'status. Per ADR-0006,'approved'remains in themilestone_statusenum but is never set. Consider noting it here so readers don't assume it's reachable.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/state-machine.md` at line 8, Annotate that the 'approved' value in the milestone_status enum is deprecated/unused: update the milestone_status description in the state-machine docs to explicitly note that 'approved' remains in the enum (per ADR-0006) but is never set at runtime, so it is not a reachable state; reference the enum name milestone_status and the specific status string 'approved' so readers understand it's intentionally retained for compatibility only.adr/0004-auto-release.md (1)
2-2: 💤 Low valueStatus appears stale. The Decision now points at a committed migration (
20260518225500_…) as the source of truth, and ADR-0005 already treats this as the active design, yet the header still readsStatus: Proposed. Consider updating toAcceptedto avoid confusion.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@adr/0004-auto-release.md` at line 2, Update the ADR header status from "Status: Proposed" to "Status: Accepted" in ADR-0004-auto-release.md to reflect that the decision is implemented (the migration 20260518225500_… and ADR-0005 treat this as active); locate the top of ADR-0004-auto-release.md (the "Status: Proposed" line) and change it to "Status: Accepted" and, if present, adjust any metadata date field to match the committed migration or add a short note linking the migration ID for traceability.src/app/client/account/page.tsx (1)
28-56: ⚡ Quick winAlign
load()with the hardened worker-account pattern.The worker account page (same cohort) now guards against stale async results with a
requestIdRefand capturesauthErrorfromgetUser()before deciding to redirect. This client page still uses the older pattern, so:
- Concurrent
load()calls (initialuseEffect+onSaved/onChanged+ Retry) can resolve out of order and overwrite newer state.- A transient
authErroris ignored, so a failed session check falls through torouter.replace("/login")instead of surfacing a retryable error.Mirroring the worker implementation keeps both pages consistent and avoids the race/redirect edge cases.
♻️ Suggested parity changes
-export default function ClientAccountPage() { - const router = useRouter(); - const [profile, setProfile] = useState<Profile | null>(null); - const [loading, setLoading] = useState(true); - const [error, setError] = useState<string | null>(null); - - async function load() { - const supabase = createClient(); - setLoading(true); - setError(null); - const { - data: { user }, - } = await supabase.auth.getUser(); - if (!user) { - setLoading(false); - router.replace("/login"); - return; - } +export default function ClientAccountPage() { + const router = useRouter(); + const [profile, setProfile] = useState<Profile | null>(null); + const [loading, setLoading] = useState(true); + const [error, setError] = useState<string | null>(null); + const requestIdRef = useRef(0); + + async function load() { + const myRequestId = ++requestIdRef.current; + const supabase = createClient(); + setLoading(true); + setError(null); + const { + data: { user }, + error: authError, + } = await supabase.auth.getUser(); + if (myRequestId !== requestIdRef.current) return; + if (authError) { + // TODO: Sentry.captureException(authError) + setError("Couldn't verify your session. Please try again."); + setLoading(false); + return; + } + if (!user) { + setProfile(null); + setLoading(false); + router.replace("/login"); + return; + }Remember to add
useRefto the React import and themyRequestId !== requestIdRef.currentguard after the profile fetch as well.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/app/client/account/page.tsx` around lines 28 - 56, The load() function should be hardened like the worker-account pattern: add a requestIdRef via useRef and increment/assign a local myRequestId at the start of load(), capture both { data:{user}, error:authError } = await supabase.auth.getUser() and handle authError by setError(authError.message) and abort (do not immediately router.replace), after fetching the profile check if myRequestId !== requestIdRef.current and return early to avoid stale results, only call router.replace("/login") when there is no user and no authError (or after surfacing authError as retryable), and apply the same stale-guard (myRequestId vs requestIdRef.current) before calling setProfile, setLoading, and setError so concurrent load/onSaved/onChanged/retry calls cannot overwrite newer state.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/state-machine.md`:
- Around line 26-31: The state-machine doc incorrectly shows a submitted ->
approved transition and hedges the effect as "'released' or 'approved'"; update
the transition to read "submitted -> released", set the Effect to explicitly
state that escrow_held decreases by the milestone amount, worker.available
increases by that amount, and milestone.status is set to 'released' (remove any
mention of 'approved'), and ensure the Trigger is still
approve_milestone(milestone_id) and the Caller/realtime notification text
remains accurate to match ADR-0006.
In `@src/components/account/change-phone-dialog.tsx`:
- Around line 45-49: Replace the ad-hoc regex/length checks with the shared Zod
schema: remove usage of INDIAN_PHONE_RE in requestOtp and any other handlers
that validate phone/otp (e.g., the OTP submit/verify handlers referenced around
the same file) and instead import and use the shared schema (e.g., phoneSchema /
otpSchema) from the common validation boundary; call
phoneSchema.safeParse(newPhone) and show toast.error with the safeParse error
message on failure, returning early, and use the same otpSchema.safeParse(...)
for OTP validation so client-side validation matches server Zod contracts.
- Around line 45-101: Add synchronous in-flight and mounted guards: create
inFlightRef = useRef(false) and mountedRef = useRef(true) (set
mountedRef.current = false in a cleanup useEffect). In requestOtp and verifyOtp,
at the top return early if inFlightRef.current is true, then set
inFlightRef.current = true before any awaits and clear it in every exit path
(finally or before each return). Before calling setLoading, setStep, setOpen,
onChanged, or toast, check mountedRef.current to avoid state/toast after
unmount. Ensure all early-return/error paths clear inFlightRef and that
mountedRef gating is applied around all state updates in requestOtp and
verifyOtp.
In `@src/components/account/delete-account-dialog.tsx`:
- Around line 37-44: Create a Zod schema (e.g., DeleteAccountSchema) that models
the client contract for the deletion form (fields: confirm: string must equal
"DELETE"; reason: optional string with the same constraints as the server-side
schema) and use DeleteAccountSchema.safeParse(...) to validate the local values
for confirm and reason before proceeding; on validation failure call toast.error
with the parsed error messages and return, and only then setLoading(true) and
invoke supabase.rpc("request_account_deletion", { reason: parsed.reason || null
}); apply the same Zod validation and safeParse flow to the other handler
referenced around lines 81-92 so all inputs are validated on the client to match
the server contract.
- Around line 36-58: Modify handleDelete to use an inFlightRef and mountedRef to
prevent double-submits and avoid updating state/toasts after unmount: add const
inFlightRef = useRef(false) and const mountedRef = useRef(true) (or convert to
useMutation which wires these in), in a useEffect set mountedRef.current = false
on cleanup; at the start of handleDelete return early if inFlightRef.current is
true, then set inFlightRef.current = true; wrap all state updates/toast
calls/router.push so they only run when mountedRef.current is true (or guard
before each call), and clear inFlightRef.current = false in a finally block so
successive submissions are allowed. Ensure references to setLoading, toast, and
router.push inside handleDelete check mountedRef before invoking.
In `@src/components/account/edit-profile-dialog.tsx`:
- Around line 46-65: The onSubmit async handler performs side-effects (toast,
setOpen, onSaved) without guarding for component mount state or concurrent
submissions; add a mountedRef (useRef(true) with cleanup to set false) and an
inFlightRef (useRef(false)) and update onSubmit to early-return if
inFlightRef.current is true, set inFlightRef.current = true at start and false
in finally, and before invoking toast, setOpen, and onSaved check
mountedRef.current so those side-effects only run when the component is still
mounted; reference the onSubmit function, toast, setOpen, and onSaved when
adding these guards.
In `@src/components/account/sign-out-button.tsx`:
- Around line 23-35: Replace the manual loading state in handleSignOut with a
useMutation hook: remove loading state and the setLoading calls, create a
mutation (e.g., const signOutMutation = useMutation(async () =>
supabase.auth.signOut(), { onError: err => toast.error("Sign out failed: " +
err.message), onSuccess: () => router.push("/") })) and call
signOutMutation.mutate() from the button click instead of handleSignOut; bind
the button disabled/loading UI to signOutMutation.isPending (or
isLoading/isMutating depending on your useMutation implementation) and keep the
open state for dialog control as-is, referencing supabase.auth.signOut,
router.push, toast.error, and the old handleSignOut name while removing its
internal setLoading usage.
In `@supabase/migrations/20260518181500_withdraw_wallet_fix_ambiguous_column.sql`:
- Around line 30-33: The bounds check dropped the NULL check for p_amount
causing NULL to bypass validation; update the conditional that currently reads
"if p_amount < 100 then" to explicitly reject NULL (e.g., "if p_amount is null
or p_amount < v_min_amount" or similar) so p_amount is validated for NULL before
any arithmetic; adjust the same validation logic where p_amount is used
elsewhere in this migration/function to ensure downstream operations (like
available_balance - p_amount) never receive NULL.
In `@supabase/migrations/20260521080000_pr19_review_fixes.sql`:
- Around line 21-107: The withdraw_wallet function (public.withdraw_wallet) is
missing GRANT EXECUTE for the authenticated role; after the function definition
add a grant/revoke pair to allow only authenticated to call it and deny
anon/public: grant execute on function public.withdraw_wallet(numeric, uuid) to
authenticated; and revoke execute on function public.withdraw_wallet(numeric,
uuid) from anon, public; this ensures RPCs (e.g.,
supabase.rpc("withdraw_wallet", ...)) work for authenticated users only.
---
Outside diff comments:
In `@AGENTS.md`:
- Around line 23-30: AGENTS.md and README.md disagree on phase status (AGENTS.md
shows Phase 3 done / Phase 4 in progress while README.md still says "Phase 2 of
8"); pick the authoritative source (update README.md to match AGENTS.md or
update AGENTS.md if that was unintended) and make the status blocks
consistent—specifically update the phase status text in README.md to reflect
"Phase 3 (jobs CRUD + browse + apply): done" and "Phase 4 (escrow state
machine): IN PROGRESS" (or mirror the chosen correct text), ensuring any commit
references or badges that display phase number are updated accordingly.
In `@README.md`:
- Line 6: Update the README header and status checklist to reflect the current
project phase by changing the text "Phase 2 of 8" and any "Phase 2" items to
"Phase 4 of 8" / "Phase 4 (in progress)" so the header line containing "🔗
**Live demo:**" and the Status checklist entries match AGENTS.md and the escrow
PR; also edit the architecture diagram text that currently attributes
auto-release to "Edge Functions" and replace it with "pg_cron" (or add a note
that auto-release scheduling is handled by pg_cron) so the diagram lines
referencing Edge Functions are accurate.
In `@src/lib/escrow-machine.ts`:
- Around line 7-14: The MilestoneStatus union in src/lib/escrow-machine.ts
contains a stale "approved" variant that no longer exists in the real escrow
state machine; update the type declaration for MilestoneStatus to remove the
"approved" string literal, then search for any references to "approved" (tests,
simulator transitions, or helpers) and replace or map them to "released" where
appropriate to keep the simulator contract aligned with the real contract
behavior (focus on the MilestoneStatus type and any functions that validate or
transition milestone states).
---
Nitpick comments:
In `@adr/0004-auto-release.md`:
- Line 2: Update the ADR header status from "Status: Proposed" to "Status:
Accepted" in ADR-0004-auto-release.md to reflect that the decision is
implemented (the migration 20260518225500_… and ADR-0005 treat this as active);
locate the top of ADR-0004-auto-release.md (the "Status: Proposed" line) and
change it to "Status: Accepted" and, if present, adjust any metadata date field
to match the committed migration or add a short note linking the migration ID
for traceability.
In `@docs/state-machine.md`:
- Line 8: Annotate that the 'approved' value in the milestone_status enum is
deprecated/unused: update the milestone_status description in the state-machine
docs to explicitly note that 'approved' remains in the enum (per ADR-0006) but
is never set at runtime, so it is not a reachable state; reference the enum name
milestone_status and the specific status string 'approved' so readers understand
it's intentionally retained for compatibility only.
In `@src/app/_actions/wallet.ts`:
- Around line 137-152: Update the inline comment that currently reads "CodeAnt
`#1`" to the correct tool name "CodeRabbit `#1`" in the wallet role-check block that
calls supabase.rpc("is_worker") (the defense-in-depth comment referencing
withdraw_wallet and the RPC check); keep the rest of the comment intact and only
change the tool name to ensure accurate references.
In `@src/app/client/account/page.tsx`:
- Around line 28-56: The load() function should be hardened like the
worker-account pattern: add a requestIdRef via useRef and increment/assign a
local myRequestId at the start of load(), capture both { data:{user},
error:authError } = await supabase.auth.getUser() and handle authError by
setError(authError.message) and abort (do not immediately router.replace), after
fetching the profile check if myRequestId !== requestIdRef.current and return
early to avoid stale results, only call router.replace("/login") when there is
no user and no authError (or after surfacing authError as retryable), and apply
the same stale-guard (myRequestId vs requestIdRef.current) before calling
setProfile, setLoading, and setError so concurrent load/onSaved/onChanged/retry
calls cannot overwrite newer state.
In `@src/app/onboarding/worker/worker-form.tsx`:
- Around line 107-112: The previewUrl created with URL.createObjectURL inside
the useMemo (previewUrl, selfieFile, useMemo) is never revoked, leaking blob
URLs when Step3 unmounts/remounts; replace the useMemo with a useEffect that
creates the object URL when selfieFile is a File and stores it in a local state
(useState for previewUrl), revoke the previous URL before creating a new one and
call URL.revokeObjectURL(previewUrl) in the effect cleanup so every created blob
URL is cleaned up; also update the React import to include useEffect/useState.
In `@src/lib/schemas/wallet.ts`:
- Around line 12-17: The withdrawWalletSchema currently enforces a minimum but
no maximum; update the zod schema for withdrawWalletSchema to include a
.max(...) constraint (suggested value ₹500,000 or ₹1,000,000) on the amount
field so it mirrors topupWalletSchema's upper bound, provides early validation,
and prevents accidental huge withdrawals; locate the amount definition inside
withdrawWalletSchema and add the appropriate .max(500000, "Maximum withdrawal is
₹5,00,000.") (or chosen limit) to the validation chain.
In `@supabase/migrations/20260603100000_escrow_idempotency.sql`:
- Around line 143-200: The function submit_milestone declares p_idempotency_key
but never uses it; either persist the key for true idempotency tracking (e.g.,
insert into an idempotency table keyed by p_idempotency_key and p_milestone_id
before performing updates) or, if the unused parameter is intentional for API
consistency, add a concise inline comment in submit_milestone noting that
p_idempotency_key is unused by design for signature uniformity to avoid
confusion (reference p_idempotency_key and submit_milestone to locate the
change).
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 9ae7edad-307b-4dd5-89c5-002e44e772b2
📒 Files selected for processing (78)
.gitignoreAGENTS.mdCLAUDE.mdREADME.mdadr/0001-auth.mdadr/0002-escrow.mdadr/0003-dispute.mdadr/0004-auto-release.mdadr/0005-realtime-contract.mdadr/0006-nav-shell.mdadr/0007-rpc-pattern.mdadr/template.mddocs/adr/0002-rls-and-security-definer-as-only-escrow-path.mddocs/adr/0003-auth-state-hygiene.mddocs/adr/0004-migration-discipline.mddocs/adr/0005-auto-release-milestones-modification.mddocs/adr/0006-escrow-idempotency.mddocs/data-model.mddocs/state-machine.mdscripts/seed-demo-users.tssrc/app/_actions/escrow.tssrc/app/_actions/jobs.tssrc/app/_actions/wallet.tssrc/app/client/account/page.tsxsrc/app/client/jobs/[id]/client-job-detail.tsxsrc/app/client/jobs/[id]/fund/page.tsxsrc/app/client/jobs/[id]/milestones/client-milestones.tsxsrc/app/client/jobs/[id]/milestones/page.tsxsrc/app/client/jobs/client-job-list.tsxsrc/app/client/jobs/new/post-job-form.tsxsrc/app/client/layout.tsxsrc/app/login/actions.tssrc/app/onboarding/actions.tssrc/app/onboarding/worker/worker-form.tsxsrc/app/page.tsxsrc/app/worker/account/page.tsxsrc/app/worker/applications/worker-applications.tsxsrc/app/worker/feed/worker-feed.tsxsrc/app/worker/jobs/[id]/apply-modal.tsxsrc/app/worker/jobs/[id]/milestones/page.tsxsrc/app/worker/jobs/[id]/milestones/worker-milestones.tsxsrc/app/worker/jobs/[id]/worker-job-detail.tsxsrc/app/worker/layout.tsxsrc/components/account/change-phone-dialog.tsxsrc/components/account/delete-account-dialog.tsxsrc/components/account/edit-profile-dialog.tsxsrc/components/account/sign-out-button.tsxsrc/components/features/topup-dialog.tsxsrc/components/features/wallet-view.tsxsrc/components/features/withdraw-dialog.tsxsrc/components/nav/client-nav-shell.tsxsrc/components/nav/worker-nav-shell.tsxsrc/components/ui/alert-dialog.tsxsrc/components/ui/badge.tsxsrc/components/ui/dialog.tsxsrc/components/ui/progress.tsxsrc/components/ui/separator.tsxsrc/components/ui/skeleton.tsxsrc/components/ui/status-badge.tsxsrc/components/ui/textarea.tsxsrc/hooks/use-user.tssrc/lib/__tests__/escrow.property.test.tssrc/lib/escrow-machine.tssrc/lib/format.tssrc/lib/schemas/jobs.tssrc/lib/schemas/onboarding.tssrc/lib/schemas/wallet.tssrc/lib/supabase/client.tssrc/lib/supabase/server.tssrc/lib/types/database.tssrc/lib/uuid.tssupabase/functions/auto-release/index.tssupabase/functions/auto_release_milestones/index.tssupabase/migrations/20260518135600_withdraw_wallet.sqlsupabase/migrations/20260518181500_withdraw_wallet_fix_ambiguous_column.sqlsupabase/migrations/20260518225500_schedule_auto_release_cron.sqlsupabase/migrations/20260521080000_pr19_review_fixes.sqlsupabase/migrations/20260603100000_escrow_idempotency.sql
💤 Files with no reviewable changes (3)
- supabase/functions/auto_release_milestones/index.ts
- src/lib/format.ts
- src/app/login/actions.ts
Group A (money/runtime): - #7: GRANT EXECUTE withdraw_wallet to authenticated + REVOKE anon/public - #6: restore NULL guard on p_amount in withdraw_wallet - #1-#4: reject NULL idempotency keys in all 4 escrow functions Group B (correctness): - #5: document p_idempotency_key API-consistency in submit_milestone - #8,#9: state-machine doc: submitted->released, annotate dead 'approved' - #20: remove 'approved' from MilestoneStatus union in escrow-machine.ts - #16: fix blob URL leak in worker-form.tsx (useEffect + revokeObjectURL) - #17: fix comment attribution CodeAnt -> CodeRabbit in wallet.ts - #18: add .max(500000) to withdrawWalletSchema Group C (dialog hardening): - #10,#11: change-phone-dialog: Zod validation + inFlightRef/mountedRef - #12,#13: delete-account-dialog: Zod validation + inFlightRef/mountedRef - #14: edit-profile-dialog: inFlightRef/mountedRef guards - #15: sign-out-button: useMutation refactor Group D (doc/consistency): - #22: README: Phase 2->4, Edge Functions->pg_cron, approved->released - #21: reconcile phase status README<->AGENTS.md - #23: ADR-0004: Proposed->Accepted - #19: client account page: race-safe requestIdRef + authError capture
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/components/account/change-phone-dialog.tsx`:
- Around line 58-64: Normalize the input once into an E.164-style variable
(e.g., normalizedPhone) and use that for validation, equality check and the
update call instead of raw newPhone: compute digits = newPhone.replace(/^\+91/,
"") then set normalizedPhone = `+91${digits}` (or bail if digits length !== 10),
test indianPhoneRegex against digits, compare normalizedPhone to currentPhone
(ensure currentPhone is also normalized or normalize it similarly), and pass
normalizedPhone to updateUser; update all references of newPhone in change-phone
submission logic to this normalizedPhone.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 30f40d42-6502-4618-a384-c236f9a8a036
📒 Files selected for processing (15)
README.mdadr/0004-auto-release.mddocs/state-machine.mdsrc/app/_actions/wallet.tssrc/app/client/account/page.tsxsrc/app/onboarding/worker/worker-form.tsxsrc/components/account/change-phone-dialog.tsxsrc/components/account/delete-account-dialog.tsxsrc/components/account/edit-profile-dialog.tsxsrc/components/account/sign-out-button.tsxsrc/lib/escrow-machine.tssrc/lib/schemas/wallet.tssupabase/migrations/20260518181500_withdraw_wallet_fix_ambiguous_column.sqlsupabase/migrations/20260521080000_pr19_review_fixes.sqlsupabase/migrations/20260603100000_escrow_idempotency.sql
✅ Files skipped from review due to trivial changes (2)
- docs/state-machine.md
- README.md
|
@CodeAnt-AI review |
✅ Action performedReview finished.
|
…cy error mapper, M1/N1/N5 hardening
|
@greptileai review the escrow RPCs and idempotency indexes |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
supabase/migrations/20260603100000_escrow_idempotency.sql (2)
103-111:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReject fund-key reuse across different milestones.
This replay path only keys on
(to_wallet, reference_id). If the same client accidentally reuses an old idempotency key for a different milestone,fund_escrow()returns the previous ledger id and reports success without funding the requested milestone. Please verify that the matched row belongs top_milestone_idbefore returning, and raise22023on mismatches instead of treating them as a replay.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@supabase/migrations/20260603100000_escrow_idempotency.sql` around lines 103 - 111, In the idempotency check inside function fund_escrow(), after selecting el.id into v_ledger_id from public.escrow_ledger (matching el.reference_id = p_idempotency_key, el.type = 'fund' and el.to_wallet = v_client_id), verify that the matched row's milestone identifier (e.g. el.milestone_id) equals p_milestone_id before returning v_ledger_id; if it does not match, raise SQLSTATE '22023' with an appropriate error instead of returning the previous ledger id. Ensure you reference the existing variables v_ledger_id, p_idempotency_key, p_milestone_id and the escrow_ledger row (alias el) when adding this check.
83-101:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse the agreed errcodes on every auth/validation branch.
These recreated RPCs still emit plain exceptions for authorization failures and some invalid arguments (
p_milestone_idnull, blankp_reason). That breaks the deterministic error mapping added in this PR, because those paths will not surface the required42501/22023codes. As per coding guidelines, "Every state-changing RPC must follow the template: CREATE OR REPLACE FUNCTION with security definer, set search_path = public, null/bounds validation, and GRANT EXECUTE to authenticated only" and "Use Postgres errcodes (42501 for not_authenticated, 22023 for invalid arguments) in RPC exceptions; callers map codes to user-facing messages instead of exposing raw error text."Also applies to: 180-205, 250-272, 385-406
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@supabase/migrations/20260603100000_escrow_idempotency.sql` around lines 83 - 101, Replace plain raise exception messages in all validation and auth branches with raise exception statements that include the appropriate Postgres errcodes: use errcode = '22023' for invalid argument checks (e.g., p_idempotency_key null, p_milestone_id null, blank p_reason) and errcode = '42501' for authorization failures (e.g., when auth.uid() <> v_client_id and not public.is_admin()). Update every occurrence in this function and the other indicated blocks (around the p_milestone_id/p_reason checks and auth checks) to follow the pattern used elsewhere in the PR so callers receive deterministic error codes instead of raw text. Ensure the function still uses security definer/search_path/grants per the RPC template if any were missed.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@supabase/migrations/20260603100000_escrow_idempotency.sql`:
- Around line 103-111: In the idempotency check inside function fund_escrow(),
after selecting el.id into v_ledger_id from public.escrow_ledger (matching
el.reference_id = p_idempotency_key, el.type = 'fund' and el.to_wallet =
v_client_id), verify that the matched row's milestone identifier (e.g.
el.milestone_id) equals p_milestone_id before returning v_ledger_id; if it does
not match, raise SQLSTATE '22023' with an appropriate error instead of returning
the previous ledger id. Ensure you reference the existing variables v_ledger_id,
p_idempotency_key, p_milestone_id and the escrow_ledger row (alias el) when
adding this check.
- Around line 83-101: Replace plain raise exception messages in all validation
and auth branches with raise exception statements that include the appropriate
Postgres errcodes: use errcode = '22023' for invalid argument checks (e.g.,
p_idempotency_key null, p_milestone_id null, blank p_reason) and errcode =
'42501' for authorization failures (e.g., when auth.uid() <> v_client_id and not
public.is_admin()). Update every occurrence in this function and the other
indicated blocks (around the p_milestone_id/p_reason checks and auth checks) to
follow the pattern used elsewhere in the PR so callers receive deterministic
error codes instead of raw text. Ensure the function still uses security
definer/search_path/grants per the RPC template if any were missed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 115f3ee7-8bae-4d36-bf60-9e29566a5d73
📒 Files selected for processing (11)
package.jsonsrc/app/_actions/escrow.tssrc/components/account/delete-account-dialog.tsxsrc/lib/supabase/client.tssrc/lib/supabase/server.tssrc/lib/types/database.tssrc/lib/uuid.tssupabase/migrations/20260518135600_withdraw_wallet.sqlsupabase/migrations/20260518181500_withdraw_wallet_fix_ambiguous_column.sqlsupabase/migrations/20260521080000_pr19_review_fixes.sqlsupabase/migrations/20260603100000_escrow_idempotency.sql
✅ Files skipped from review due to trivial changes (3)
- supabase/migrations/20260518135600_withdraw_wallet.sql
- src/lib/uuid.ts
- src/lib/supabase/client.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- supabase/migrations/20260521080000_pr19_review_fixes.sql
- src/app/_actions/escrow.ts
- src/components/account/delete-account-dialog.tsx
Greptile SummaryThis PR wires caller-supplied idempotency keys end-to-end through the four escrow RPCs, adds a worker
Confidence Score: 4/5The escrow changes are internally consistent and the dialog hardening is correctly implemented, but several issues flagged in prior review rounds remain unresolved in the merged code. The idempotency wiring, withdrawal cap, and dialog guards are all logically correct. The fund_escrow function lacks a concurrent-duplicate guard on its ledger INSERT (previously raised), approve_milestone accepts but silently ignores the caller-supplied key (previously raised), and the delete-account dialog still sends an empty string instead of null for a missing reason (previously raised). These are known open issues, not new regressions, but they remain present in the code being merged. supabase/migrations/20260603100000_escrow_idempotency.sql — the fund_escrow ledger INSERT and the approve_milestone idempotency-key semantics warrant a second look before this ships to production. Important Files Changed
Reviews (4): Last reviewed commit: "fix(pr20): NULL-safe authz in dispute_mi..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| const { error } = await supabase.rpc("request_account_deletion", { | ||
| reason: reason || "", | ||
| }); |
There was a problem hiding this comment.
reason state variable with reason || "" rather than the Zod-validated value, and silently changes the argument from null (old behaviour) to "" when the user leaves the reason blank. If request_account_deletion checks p_reason IS NULL to mark "no reason given" — for example to filter or skip storing the column — every deletion from this point on will store an empty string instead, breaking that distinction.
| const { error } = await supabase.rpc("request_account_deletion", { | |
| reason: reason || "", | |
| }); | |
| const { error } = await supabase.rpc("request_account_deletion", { | |
| reason: parsed.data.reason ?? null, | |
| }); |
| where id = p_milestone_id; | ||
|
|
||
| -- bump parent job to in_progress if still 'assigned' | ||
| update public.jobs | ||
| set status = 'in_progress'::public.job_status | ||
| where id = v_job_id | ||
| and status = 'assigned'::public.job_status; | ||
|
|
||
| return p_milestone_id; | ||
| end; | ||
| $$; | ||
|
|
||
| -- ── approve_milestone ──────────────────────────────────────────────────────── | ||
| create or replace function public.approve_milestone( | ||
| p_milestone_id uuid, | ||
| p_idempotency_key uuid | ||
| ) | ||
| returns uuid | ||
| language plpgsql |
There was a problem hiding this comment.
p_idempotency_key not stored — deduplication silently uses p_milestone_id
The function accepts p_idempotency_key uuid with a null guard, but the ledger row is inserted with reference_id = p_milestone_id (not p_idempotency_key), and the early-return idempotency check also queries reference_id = p_milestone_id. The caller-supplied key is therefore never persisted or used for deduplication. The comment in the body explains the rationale, but the inconsistency with fund_escrow (where the caller key IS stored) means any future developer reading the function signature will expect p_idempotency_key to be the replay anchor — and it isn't. Consider renaming the parameter to _p_idempotency_key or adding a comment directly on the parameter declaration so the intent is unmistakable at the function boundary.
Greptile SummaryThis PR wires client-generated idempotency keys end-to-end into the four escrow RPCs (
Confidence Score: 4/5Safe to merge with minor follow-ups; the escrow RPCs are transactionally sound and the UNIQUE indexes are correctly scoped. The escrow RPC redesign is carefully constructed — FOR UPDATE locks serialize concurrent access, and the two functions that accept p_idempotency_key without using it (approve_milestone, submit_milestone) are documented intentionally. The main gaps are stylistic inconsistencies: fund_escrow is the only RPC without an ON CONFLICT guard, and the dead p_idempotency_key parameters in approve_milestone and submit_milestone could mislead future maintainers. The reason: empty-string vs null change in DeleteAccountDialog is a small behavioral shift worth verifying against the DB function. Nothing here is a transaction-safety break. supabase/migrations/20260603100000_escrow_idempotency.sql — the approve_milestone and submit_milestone dead-parameter pattern and the missing ON CONFLICT in fund_escrow are the areas worth a second look before merging. Important Files Changed
Sequence DiagramsequenceDiagram
participant Client as Browser / Server Action
participant RPC as Supabase RPC
participant Ledger as escrow_ledger (UNIQUE idx)
participant Wallet as wallets
participant Milestone as milestones
Note over Client,Milestone: fund_escrow (caller key stored)
Client->>RPC: fund_escrow(milestone_id, idempotency_key)
RPC->>Milestone: SELECT FOR UPDATE
RPC->>Ledger: "SELECT WHERE reference_id = key AND type='fund'"
alt Ledger row found (idempotent replay)
RPC-->>Client: return existing ledger_id
else First call
RPC->>Wallet: SELECT FOR UPDATE
RPC->>Wallet: "locked += amount, available -= amount"
RPC->>Milestone: status → funded
RPC->>Ledger: "INSERT reference_id = key (no ON CONFLICT guard)"
RPC-->>Client: return new ledger_id
end
Note over Client,Milestone: approve_milestone (key = milestone_id, shared with auto_release)
Client->>RPC: "approve_milestone(milestone_id, idempotency_key*)"
Note right of RPC: *key validated but unused
RPC->>Milestone: SELECT FOR UPDATE
RPC->>Ledger: "SELECT WHERE reference_id = milestone_id AND type='release'"
alt Ledger row found
RPC-->>Client: return existing ledger_id
else First call
RPC->>Wallet: SELECT FOR UPDATE (client + worker)
RPC->>Wallet: "client.locked -= amount, worker.available += amount"
RPC->>Milestone: status → released
RPC->>Ledger: "INSERT reference_id = milestone_id ON CONFLICT DO NOTHING"
RPC-->>Client: return ledger_id
end
Note over Client,Milestone: dispute_milestone (open-per-milestone index)
Client->>RPC: dispute_milestone(milestone_id, reason, key)
RPC->>Milestone: SELECT FOR UPDATE
RPC->>RPC: SELECT open dispute for milestone
alt Open dispute exists
RPC-->>Client: return existing dispute_id
else
RPC->>Milestone: status → disputed
RPC->>Ledger: INSERT disputes EXCEPTION WHEN unique_violation
RPC-->>Client: return dispute_id
end
Reviews (2): Last reviewed commit: "fix(pr20): H1 shared release reference_i..." | Re-trigger Greptile |
| as $$ | ||
| declare | ||
| v_job_id uuid; | ||
| v_worker_id uuid; | ||
| v_status public.milestone_status; | ||
| begin | ||
| -- p_idempotency_key is accepted for API-signature consistency with the other | ||
| -- escrow RPCs; the actual idempotency guard for submit relies on milestone | ||
| -- status (early-return when already 'submitted'). We still reject NULL to | ||
| -- enforce the client contract. | ||
| if p_idempotency_key is null then | ||
| raise exception 'invalid_idempotency_key' using errcode = '22023'; | ||
| end if; | ||
|
|
||
| select m.job_id, j.worker_id, m.status | ||
| into v_job_id, v_worker_id, v_status | ||
| from public.milestones m | ||
| join public.jobs j on j.id = m.job_id | ||
| where m.id = p_milestone_id | ||
| for update of m, j; | ||
|
|
||
| if v_job_id is null then |
There was a problem hiding this comment.
p_idempotency_key is accepted but never used in approve_milestone
The parameter passes the NULL guard (line 179) but is then completely unused — the actual reference_id stored in escrow_ledger is always p_milestone_id (line 283), not the caller-supplied key. A caller who generates a fresh UUID per retry thinking they need a "new" key will see the same deduplication behavior regardless. The comment on lines 250-255 explains the intent, but the parameter name p_idempotency_key implies it drives deduplication, which it does not. A rename to something like p_api_token or adding a prominent -- NOTE: not used as reference_id; see comment below in the parameter list would prevent future maintainers from relying on this key for deduplication semantics.
| insert into public.escrow_ledger ( | ||
| job_id, | ||
| milestone_id, | ||
| from_wallet, | ||
| to_wallet, | ||
| amount, | ||
| type, | ||
| reference_id | ||
| ) | ||
| values ( | ||
| v_job_id, | ||
| p_milestone_id, | ||
| v_client_id, | ||
| v_client_id, | ||
| v_amount, | ||
| 'fund'::public.ledger_type, | ||
| p_idempotency_key | ||
| ) | ||
| returning id into v_ledger_id; |
There was a problem hiding this comment.
fund_escrow ledger insert has no ON CONFLICT guard
All three other RPCs (approve_milestone, dispute_milestone, and auto_release_milestones in the earlier migration) handle the unique-violation that could arise from a concurrent insert via either ON CONFLICT DO NOTHING or a PL/pgSQL EXCEPTION WHEN unique_violation block. fund_escrow has neither: if two concurrent calls with the same idempotency key both slip past the early-return check, the second INSERT will surface a raw unique_violation to the caller instead of a clean idempotent return. Adding ON CONFLICT DO NOTHING with a follow-up re-read makes the defensive pattern consistent.
| insert into public.escrow_ledger ( | |
| job_id, | |
| milestone_id, | |
| from_wallet, | |
| to_wallet, | |
| amount, | |
| type, | |
| reference_id | |
| ) | |
| values ( | |
| v_job_id, | |
| p_milestone_id, | |
| v_client_id, | |
| v_client_id, | |
| v_amount, | |
| 'fund'::public.ledger_type, | |
| p_idempotency_key | |
| ) | |
| returning id into v_ledger_id; | |
| insert into public.escrow_ledger ( | |
| job_id, | |
| milestone_id, | |
| from_wallet, | |
| to_wallet, | |
| amount, | |
| type, | |
| reference_id | |
| ) | |
| values ( | |
| v_job_id, | |
| p_milestone_id, | |
| v_client_id, | |
| v_client_id, | |
| v_amount, | |
| 'fund'::public.ledger_type, | |
| p_idempotency_key | |
| ) | |
| on conflict (to_wallet, reference_id) where type = 'fund' do nothing | |
| returning id into v_ledger_id; | |
| -- If ON CONFLICT suppressed the INSERT (concurrent call won the race), re-read. | |
| if v_ledger_id is null then | |
| select el.id into v_ledger_id | |
| from public.escrow_ledger el | |
| where el.reference_id = p_idempotency_key | |
| and el.type = 'fund' | |
| and el.to_wallet = v_client_id; | |
| end if; |
| const { error } = await supabase.rpc("request_account_deletion", { | ||
| reason: reason || "", | ||
| }); | ||
| if (!mountedRef.current) return; |
There was a problem hiding this comment.
reason now sent as "" instead of null
The old call passed reason: reason || null; the new code passes reason: reason || "". If the request_account_deletion RPC stores the reason in a nullable column and uses IS NULL (or COALESCE) to detect "no reason provided", an empty string will be stored as a real value instead of a deliberate absence. Downstream queries or admin tooling that filter on reason IS NULL would no longer match these rows. Verify whether the DB function is neutral to "" vs null, or restore the null for the no-reason case.
|
@coderabbitai review |
✅ Action performedReview finished.
|
…row isolated auto-release + milestone-aware fund replay (CR-1)
|
@coderabbitai re: Reject fund-key reuse across different milestones Done in b9a4e32! fund_escrow now expects the milestone ID it originally funded; replays on different milestones hit a 22023 exception. |
|
@greptile We've tightened the escrow idempotency significantly! H1 money-safety is enforced with hard unique constraints and subtxn isolation, and CR-1 milestone-replay is fixed. Appreciate the detailed breakdown! |
|
Tip For best results, initiate chat on the files or code changes.
(ﾐↀ ω ↀﾐ)✧ 🧠 Learnings usedIf you found this review helpful, would you consider giving us a shout-out on X? Thank you for using CodeRabbit! |
|
Glad to hear it — those constraints are exactly the right layer to enforce this. The combination of DB-level UNIQUE indexes and subtransaction isolation means even concurrent retries can't produce duplicate ledger entries, which is the gold standard for money-movement idempotency. Nice work closing this out cleanly! Tip: You can customize Greptile's behavior for this repo with |
|
The One thing to verify: the loop
-- Lock wallets ...
if exists (...locked_balance >= v_milestone.amount) then
update wallets ...
update milestones ...
insert into escrow_ledger ... -- no subtxn around this
...
end if;
end loop;Without an actual subtransaction ( Could you point to where the per-row isolation lives in b9a4e32? If the intent is to let one failing row skip without aborting the batch, the subtransaction needs to be explicit, e.g.: loop
begin
-- ... wallet updates, milestone update, ledger insert ...
v_count := v_count + 1;
exception
when unique_violation then
raise notice 'skipping duplicate ledger for milestone %', v_milestone.id;
when others then
raise notice 'skipping milestone % due to error: %', v_milestone.id, sqlerrm;
end;
end loop;If the design intent is instead "hard-abort the whole batch on any conflict," that's fine too — but then the comment about isolated subtransactions may be inaccurate. |
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 (1)
supabase/migrations/20260521080000_pr19_review_fixes.sql (1)
21-110:⚠️ Potential issue | 🟠 Major | 💤 Low valueAdd/Reference an ADR for
public.withdraw_walletchanges
supabase/migrations/20260521080000_pr19_review_fixes.sqlrewritespublic.withdraw_wallet, but no ADR inadr/ordocs/adr/mentionswithdraw_wallet/public.withdraw_wallet(the only ADR explicitly referenced in the header is ADR-0005 forauto_release_milestones). Add awithdraw_wallet-specific ADR reference in the header or create an ADR covering this modification.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@supabase/migrations/20260521080000_pr19_review_fixes.sql` around lines 21 - 110, Migration rewrites the function public.withdraw_wallet but no ADR documents this change; create or update an ADR that records the withdraw_wallet change and reference it from the migration header: add an ADR (in your ADR directory) titled e.g. "ADR-XXXX: public.withdraw_wallet behavior and idempotency" that describes the new function signature public.withdraw_wallet(p_amount numeric, p_idempotency_key uuid), the idempotency handling (unique reference_id and replay path), role restriction (public.is_worker()), bounds checking (min/max amounts), and post-debit semantics, then add a one-line reference to that ADR at the top of the migration so reviewers can trace the design rationale.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scratch/test_concurrent.ts`:
- Around line 7-16: The script creates the Supabase client with
SUPABASE_SERVICE_ROLE_KEY which makes auth.uid() NULL inside SECURITY DEFINER
RPCs (causing fund_escrow, submit_milestone, approve_milestone to fail); update
run() to: use the service-role client only for DB setup (selecting/creating test
profiles), then obtain a real user session (e.g., via
supabase.auth.signInWithPassword() or creating a test user via
supabase.auth.admin.generateLink() and signing in) and instantiate a
user-authenticated client for calling the RPCs (or impersonate users), and
replace the non-null assertions on client_id and worker_id with proper existence
checks/guards before proceeding so the script fails gracefully if profiles are
missing.
---
Outside diff comments:
In `@supabase/migrations/20260521080000_pr19_review_fixes.sql`:
- Around line 21-110: Migration rewrites the function public.withdraw_wallet but
no ADR documents this change; create or update an ADR that records the
withdraw_wallet change and reference it from the migration header: add an ADR
(in your ADR directory) titled e.g. "ADR-XXXX: public.withdraw_wallet behavior
and idempotency" that describes the new function signature
public.withdraw_wallet(p_amount numeric, p_idempotency_key uuid), the
idempotency handling (unique reference_id and replay path), role restriction
(public.is_worker()), bounds checking (min/max amounts), and post-debit
semantics, then add a one-line reference to that ADR at the top of the migration
so reviewers can trace the design rationale.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: fdec4264-63b7-4a6f-b12d-a803874af1ef
📒 Files selected for processing (3)
scratch/test_concurrent.tssupabase/migrations/20260521080000_pr19_review_fixes.sqlsupabase/migrations/20260603100000_escrow_idempotency.sql
|
|
||
| select m.job_id, j.client_id, j.worker_id, m.status | ||
| into v_job_id, v_client_id, v_worker_id, v_status | ||
| from public.milestones m | ||
| join public.jobs j on j.id = m.job_id |
There was a problem hiding this comment.
NOT IN with NULL v_worker_id silently bypasses the authorization check
When a job has no assigned worker yet (worker_id IS NULL in jobs), v_worker_id is NULL, making auth.uid() NOT IN (v_client_id, NULL) evaluate to NULL — not TRUE. In PL/pgSQL, IF NULL is treated as falsy, so the exception is never raised. Any authenticated user who is not the client can call dispute_milestone on an unworked job and successfully mark both the milestone and the job as 'disputed'.
The parallel functions submit_milestone and approve_milestone guard against this explicitly with if v_worker_id is null then raise exception 'Job has no assigned worker'; end if; — that guard is absent here.
| select m.job_id, j.client_id, j.worker_id, m.status | |
| into v_job_id, v_client_id, v_worker_id, v_status | |
| from public.milestones m | |
| join public.jobs j on j.id = m.job_id | |
| if v_worker_id is null then | |
| raise exception 'Job has no assigned worker'; | |
| end if; | |
| if auth.uid() not in (v_client_id, v_worker_id) and not public.is_admin() then | |
| raise exception 'Only job participants or admin can raise dispute'; | |
| end if; |
There was a problem hiding this comment.
Good catch. Fixed in 87029a4 by migrating to a NULL-safe IS DISTINCT FROM chain. I also audited the rest of the escrow RPCs (fund_escrow, submit_milestone, approve_milestone) and proactively migrated their <> operators to IS DISTINCT FROM to structurally close this bug class across the board.
…+ remove scratch files from PR
|
Reply to Greptile on Reply to CodeRabbit on |
Escrow Idempotency + Worker Withdrawal + Account-Dialog Hardening
What
Wire client idempotency keys end-to-end into escrow RPCs, add worker wallet withdrawal flow, and harden all account dialogs.
Key changes
p_idempotency_key uuid. Added partial UNIQUE indexes onescrow_ledger(fund/release) anddisputes(open-per-milestone). NULL keys rejected at DB boundary.withdraw_walletRPC with min (₹100) / max (₹500K) / NULL guards, GRANT EXECUTE for authenticated, Zod schema, server action, andWithdrawDialogcomponent.change-phone,delete-account,edit-profile,sign-out) hardened withinFlightRef/mountedRefguards, Zod validation, anduseMutation.change-phone-dialognormalizes input to+91XXXXXXXXXXbefore compare,updateUser,verifyOtp, andprofiles.phoneupdate.supabase/functions/auto-release/— pg_cron (ADR-0004) is the authoritative scheduler.state-machine.mdsubmitted→released, ADR-0004 Accepted.Verification
pnpm lint/typecheck/test/buildall greenSummary by CodeRabbit
New Features
Bug Fixes
Documentation
UX