Blittech/braidsbymira
4
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

fix: address review — reschedule desync, pg_net comment, self-healing unique index

Browse Source 
Fixes 3 Important issues from re-review of the previous fast-follow-up commit:

1. booked_slots desync on reschedule: the cleanup trigger was `after update
   of status`, so changing date/time on a still-confirmed booking never fired
   it -- the OLD slot stayed blocked and the NEW slot stayed shown as free.
   Changed to an unqualified `after update`, and the UPDATE branch now
   compares the full (date, time, status) triple instead of just status, so
   it frees the old slot on any change away from it and (re)mirrors the new
   one when the row is still/now confirmed. Added pgTAP coverage: reschedule
   removes the old slot's mirror row and creates the new one.

2. The Edge Functions' comments/commit message claimed a non-200 response
   would make pg_net "retry" -- that's not how pg_net works, it's
   fire-and-forget and never inspects the response status. Corrected the
   comments to state the real reasoning (nothing consumes the status code,
   so a 500 wouldn't help and would misrepresent an already-successful
   booking/inquiry as failed). Closed the actual gap that created -- a failed
   Resend send was only visible in Edge Function logs nobody reads -- by
   adding a `notification_failures` table (new migration, admin-only select
   via RLS, no anon/authenticated write access, only service_role writes)
   and having both handlers insert a row (booking/inquiry id + error
   message) whenever the Resend call fails, the API key is missing, or an
   unexpected error occurs. Insert failures are swallowed (best-effort --
   already in a failure path, must not throw a louder one). Added pgTAP
   tests for the table's RLS/grant shape.

3. `20260812000002_bookings_slot_uniqueness.sql` could have failed outright
   on `supabase db push` against live data if any existing confirmed booking
   already shared a (date, time) with another. Added
   `dedupe_confirmed_booking_slots()`, called before the index is created:
   it collapses each duplicate group down to the earliest-created row
   (kept 'confirmed', others cancelled) and logs what it did via
   `raise notice`, so index creation is guaranteed to succeed either way.
   Extracted as a standalone, service_role-only-executable function (rather
   than inlining the SQL) specifically so the pgTAP test can exercise the
   exact same logic the migration runs. Added a test that seeds two
   duplicate confirmed bookings (having first dropped the index to simulate
   the pre-migration state), runs the dedupe function, and asserts exactly
   one confirmed booking remains -- the earliest one -- before recreating
   the index.

pgTAP suite grew from 23 to 37 tests. Verified locally: supabase db reset
(all 7 migrations apply cleanly), supabase test db (37/37 pass), vitest run
(51/51 pass), npm run build, npm run lint, and npm run test:functions
(6/6 pass) all succeed. Still nothing here touches the live Supabase
project.

Claude-Session: https://claude.ai/code/session_01NhxA4npAXAxmCDEJt6kxSH
This commit is contained in:
belviskhoremk
2026-08-12 10:42:25 +02:00
parent c74f1bb6cb
commit e6061d7ba4
6 changed files with 289 additions and 42 deletions
Download Patch File Download Diff File Expand all files Collapse all files

52
supabase/functions/on-booking-created/index.ts
View File

@@ -28,12 +28,27 @@ Deno.serve(async (req) => {
})
}
try {
const supabaseAdmin = createClient(
Deno.env.get('SUPABASE_URL')!,
Deno.env.get('SUPABASE_SERVICE_ROLE_KEY')!,
)
const supabaseAdmin = createClient(
Deno.env.get('SUPABASE_URL')!,
Deno.env.get('SUPABASE_SERVICE_ROLE_KEY')!,
)
// Best-effort: records a failure so it's visible somewhere a human could
// plausibly check (e.g. a future admin panel view), rather than only in
// Edge Function logs nobody reads. Swallow any error from this insert
// itself -- we're already in a failure path and must not let logging the
// failure become a second, louder failure.
const recordFailure = async (message: string) => {
try {
await supabaseAdmin
.from('notification_failures')
.insert({ kind: 'booking', record_id: booking.id, error_message: message })
} catch (err) {
console.error('on-booking-created: failed to record notification_failures row', err instanceof Error ? err.message : err)
}
}
try {
const { data: service } = await supabaseAdmin
.from('services')
.select('name')
@@ -54,7 +69,9 @@ Deno.serve(async (req) => {
const resendApiKey = Deno.env.get('RESEND_API_KEY')
if (!resendApiKey) {
console.error(`on-booking-created: RESEND_API_KEY is not set, skipping email for service ${booking.service_id}`)
const message = 'RESEND_API_KEY is not set'
console.error(`on-booking-created: ${message}, skipping email for service ${booking.service_id}`)
await recordFailure(message)
} else {
const resendRes = await fetch('https://api.resend.com/emails', {
method: 'POST',
@@ -66,23 +83,28 @@ Deno.serve(async (req) => {
})
if (!resendRes.ok) {
const body = await resendRes.text()
console.error(
`on-booking-created: Resend send failed for service ${booking.service_id} (status ${resendRes.status})`,
await resendRes.text(),
body,
)
await recordFailure(`Resend responded ${resendRes.status}: ${body}`)
}
}
} catch (err) {
// The booking itself already succeeded (this handler only runs after
// the row was inserted) -- a failure here means the notification email
// didn't go out, not that the booking failed. Log for debugging and
// still return 200: a non-200 would make pg_net treat this as a
// retry-worthy failure, but retrying re-sends against the same booking
// row and won't fix a Resend outage or bad payload.
console.error(
`on-booking-created: unexpected error for service ${booking?.service_id}`,
err instanceof Error ? err.message : err,
)
// didn't go out, not that the booking failed. We still return 200:
// pg_net is fire-and-forget (it doesn't inspect the response status or
// retry on non-2xx -- the response just lands in net._http_response),
// so returning 500 wouldn't trigger a retry or help anyone; nothing
// consumes this status code. What it *would* do is misrepresent the
// request as failed when the booking itself succeeded. The actual gap
// this creates -- a failed send being invisible -- is what
// notification_failures (recorded above/below) exists to close.
const message = err instanceof Error ? err.message : String(err)
console.error(`on-booking-created: unexpected error for service ${booking?.service_id}`, message)
await recordFailure(message)
}
return new Response(JSON.stringify({ ok: true }), { headers: { 'Content-Type': 'application/json' } })

52
supabase/functions/on-inquiry-created/index.ts
View File

@@ -28,12 +28,27 @@ Deno.serve(async (req) => {
})
}
try {
const supabaseAdmin = createClient(
Deno.env.get('SUPABASE_URL')!,
Deno.env.get('SUPABASE_SERVICE_ROLE_KEY')!,
)
const supabaseAdmin = createClient(
Deno.env.get('SUPABASE_URL')!,
Deno.env.get('SUPABASE_SERVICE_ROLE_KEY')!,
)
// Best-effort: records a failure so it's visible somewhere a human could
// plausibly check (e.g. a future admin panel view), rather than only in
// Edge Function logs nobody reads. Swallow any error from this insert
// itself -- we're already in a failure path and must not let logging the
// failure become a second, louder failure.
const recordFailure = async (message: string) => {
try {
await supabaseAdmin
.from('notification_failures')
.insert({ kind: 'inquiry', record_id: inquiry.id, error_message: message })
} catch (err) {
console.error('on-inquiry-created: failed to record notification_failures row', err instanceof Error ? err.message : err)
}
}
try {
const { data: product } = await supabaseAdmin
.from('products')
.select('name')
@@ -47,7 +62,9 @@ Deno.serve(async (req) => {
const resendApiKey = Deno.env.get('RESEND_API_KEY')
if (!resendApiKey) {
console.error(`on-inquiry-created: RESEND_API_KEY is not set, skipping email for product ${inquiry.product_id}`)
const message = 'RESEND_API_KEY is not set'
console.error(`on-inquiry-created: ${message}, skipping email for product ${inquiry.product_id}`)
await recordFailure(message)
} else {
const resendRes = await fetch('https://api.resend.com/emails', {
method: 'POST',
@@ -59,23 +76,28 @@ Deno.serve(async (req) => {
})
if (!resendRes.ok) {
const body = await resendRes.text()
console.error(
`on-inquiry-created: Resend send failed for product ${inquiry.product_id} (status ${resendRes.status})`,
await resendRes.text(),
body,
)
await recordFailure(`Resend responded ${resendRes.status}: ${body}`)
}
}
} catch (err) {
// The inquiry itself already succeeded (this handler only runs after
// the row was inserted) -- a failure here means the notification email
// didn't go out, not that the inquiry failed. Log for debugging and
// still return 200: a non-200 would make pg_net treat this as a
// retry-worthy failure, but retrying re-sends against the same inquiry
// row and won't fix a Resend outage or bad payload.
console.error(
`on-inquiry-created: unexpected error for product ${inquiry?.product_id}`,
err instanceof Error ? err.message : err,
)
// didn't go out, not that the inquiry failed. We still return 200:
// pg_net is fire-and-forget (it doesn't inspect the response status or
// retry on non-2xx -- the response just lands in net._http_response),
// so returning 500 wouldn't trigger a retry or help anyone; nothing
// consumes this status code. What it *would* do is misrepresent the
// request as failed when the inquiry itself succeeded. The actual gap
// this creates -- a failed send being invisible -- is what
// notification_failures (recorded above/below) exists to close.
const message = err instanceof Error ? err.message : String(err)
console.error(`on-inquiry-created: unexpected error for product ${inquiry?.product_id}`, message)
await recordFailure(message)
}
return new Response(JSON.stringify({ ok: true }), { headers: { 'Content-Type': 'application/json' } })

48
supabase/migrations/20260812000002_bookings_slot_uniqueness.sql
View File

@@ -11,6 +11,54 @@
-- 'cancelled': a cancelled booking must not permanently block the slot it
-- once held, and multiple cancelled bookings may legitimately share a
-- (date, time) (e.g. several people asked for the same slot, all declined).
--
-- SELF-HEALING: on `supabase db push` against the live project, this
-- `create unique index` could fail outright if any existing confirmed
-- booking already shares a (date, time) with another -- exactly the bug
-- being fixed here, so entirely plausible in already-live data. Rather than
-- have the migration fail and block deploy, dedupe_confirmed_booking_slots()
-- below collapses each duplicate group down to the earliest-created row
-- (keeping it 'confirmed') and cancels the rest before the index is
-- created, so index creation is guaranteed to succeed either way. It's
-- defined as a standalone function (called once below, but also directly
-- callable) so the pgTAP test can exercise the exact same logic the
-- migration runs, rather than a copy that could drift from it.
create or replace function public.dedupe_confirmed_booking_slots()
returns void
language plpgsql
security definer
set search_path = ''
as $$
declare
dup record;
begin
for dup in
select date, time, count(*) as cnt
from public.bookings
where status = 'confirmed'
group by date, time
having count(*) > 1
loop
update public.bookings
set status = 'cancelled'
where date = dup.date and time = dup.time and status = 'confirmed'
and id <> (
select id from public.bookings
where date = dup.date and time = dup.time and status = 'confirmed'
order by created_at asc, id asc
limit 1
);
raise notice 'dedupe_confirmed_booking_slots: cancelled % duplicate confirmed booking(s) for slot % %, keeping the earliest-created one',
dup.cnt - 1, dup.date, dup.time;
end loop;
end;
$$;
revoke execute on function public.dedupe_confirmed_booking_slots() from public, anon, authenticated;
grant execute on function public.dedupe_confirmed_booking_slots() to service_role;
select public.dedupe_confirmed_booking_slots();
create unique index bookings_active_slot_idx
on public.bookings (date, time)
where status = 'confirmed';

39
supabase/migrations/20260812000003_booked_slots_cleanup.sql
View File

@@ -4,7 +4,7 @@
-- already-applied 20260808000002 file.
--
-- Problem: sync_booked_slot() only ran `after insert`, and unconditionally
-- mirrored every new booking regardless of status. Two consequences:
-- mirrored every new booking regardless of status. Three consequences:
-- 1. If a confirmed booking was later cancelled (status update) or
-- deleted, its booked_slots mirror row was never removed, so the slot
-- stayed permanently blocked even though it was actually free again.
@@ -12,14 +12,22 @@
-- insert policy allows any status, not just the 'confirmed' default)
-- would still create a booked_slots row, incorrectly blocking a slot
-- nobody had actually confirmed.
-- 3. Rescheduling a confirmed booking (changing date/time while it stays
-- 'confirmed') didn't fire the trigger at all under an `of status`
-- qualifier, leaving the OLD slot permanently blocked and the NEW slot
-- showing as free even though it's now taken. Caught in review: a
-- status-qualified trigger silently misses any update that doesn't
-- touch the status column.
--
-- Fix: the trigger function now branches on TG_OP. On insert/update it only
-- mirrors the slot when the new row is 'confirmed' (fixing #2 above). On
-- update-away-from-confirmed or delete, it removes the mirror row, but only
-- if no *other* confirmed booking still holds that (date, time) -- the
-- partial unique index added in 20260812000002 guarantees there can be at
-- most one, but this guard keeps the function correct even if that
-- assumption ever changes.
-- Fix: the trigger function now branches on TG_OP and the trigger fires on
-- every UPDATE (not just `of status`). On insert/update it only mirrors the
-- slot when the new row is 'confirmed' (fixing #2 above). On update where
-- the (date, time, status) triple actually changed and the OLD row was
-- confirmed, or on delete of a confirmed row, it removes the OLD slot's
-- mirror row, but only if no *other* confirmed booking still holds that
-- (date, time) -- the partial unique index added in 20260812000002
-- guarantees there can be at most one, but this guard keeps the function
-- correct even if that assumption ever changes.
create or replace function public.sync_booked_slot()
returns trigger
language plpgsql
@@ -40,7 +48,12 @@ begin
end if;
if tg_op = 'UPDATE' then
if old.status = 'confirmed' and new.status <> 'confirmed' then
-- Covers both a status change away from 'confirmed' (cancellation) and a
-- reschedule of a still-confirmed booking to a different (date, time):
-- either way, the OLD slot may no longer have any confirmed booking
-- holding it and its mirror row needs to go.
if old.status = 'confirmed'
and (old.date, old.time, old.status) is distinct from (new.date, new.time, new.status) then
delete from public.booked_slots
where date = old.date and time = old.time
and not exists (
@@ -66,8 +79,12 @@ begin
end;
$$;
create trigger booking_status_updated_sync_slot
after update of status on public.bookings
-- Unqualified `after update` (not `of status`): a reschedule that changes
-- date/time while status stays 'confirmed' must also resync the mirror --
-- the old slot needs freeing and the new slot needs blocking -- and an
-- `of status` trigger would never fire for that case.
create trigger booking_updated_sync_slot
after update on public.bookings
for each row
execute function public.sync_booked_slot();

31
supabase/migrations/20260812000004_notification_failures.sql Normal file
View File

@@ -0,0 +1,31 @@
-- Follow-up to the final review of this fast-follow-up branch. The Edge
-- Functions (on-booking-created, on-inquiry-created) already log a failed
-- Resend send via console.error, but Edge Function logs are not something
-- Mira (the salon owner) would ever look at -- a failed notification email
-- was effectively invisible. This table gives failures a place a human
-- could plausibly check (e.g. surfaced later in an admin panel), without
-- making the booking/inquiry request itself fail or retry (see the
-- reasoning in the Edge Functions' try/catch blocks).
create table public.notification_failures (
id uuid primary key default gen_random_uuid(),
kind text not null check (kind in ('booking', 'inquiry')),
record_id uuid not null,
error_message text not null,
created_at timestamptz not null default now()
);
alter table public.notification_failures enable row level security;
-- Admin-only select; no insert/update/delete policy exists for anyone --
-- only the Edge Functions' service-role client writes here, and
-- service_role bypasses RLS entirely (same pattern as booked_slots'
-- security-definer-trigger-only write path).
create policy "notification_failures_admin_read" on public.notification_failures
for select using ((select auth.uid()) = '21c90224-e0b2-412e-a431-8e7f097c1cef'::uuid);
-- No grant to anon at all (these rows can reference booking/inquiry PII via
-- error_message, same protection level as bookings/inquiries themselves).
-- authenticated gets select (narrowed to the admin UID by the RLS policy
-- above); only service_role can write.
grant select on public.notification_failures to authenticated;
grant select, insert, update, delete on public.notification_failures to service_role;

109
supabase/tests/database/rls.test.sql
View File

@@ -1,5 +1,5 @@
begin;
select plan(23);
select plan(37);
-- Seed reference data as service_role (bypasses RLS)
set local role service_role;
@@ -171,5 +171,112 @@ select is_empty(
'booked_slots mirror row is removed after the booking is deleted'
);
-- Rescheduling: an update that changes date/time while status stays
-- 'confirmed' must free the OLD slot's mirror row and create one for the
-- NEW slot. A trigger scoped to `after update of status` would never fire
-- for this (status doesn't change), which was the bug caught in review.
select lives_ok(
$$ insert into public.bookings (service_id, date, time, contact_name, contact_phone, contact_email, contact_notes)
values ('knotless', '2026-10-03', '11:00 AM', 'F Client', '555-0106', 'f@example.com', '') $$,
'confirmed booking for the reschedule test succeeds'
);
select lives_ok(
$$ update public.bookings set date = '2026-10-04', time = '1:00 PM'
where date = '2026-10-03' and time = '11:00 AM' and status = 'confirmed' $$,
'rescheduling the confirmed booking to a new date/time succeeds'
);
select is_empty(
$$ select * from public.booked_slots where date = '2026-10-03' and time = '11:00 AM' $$,
'booked_slots mirror row for the OLD slot is removed after a reschedule'
);
select isnt_empty(
$$ select * from public.booked_slots where date = '2026-10-04' and time = '1:00 PM' $$,
'booked_slots mirror row for the NEW slot is created after a reschedule'
);
-- notification_failures: admin-only select; only service_role writes.
select lives_ok(
$$ insert into public.notification_failures (kind, record_id, error_message)
values ('booking', gen_random_uuid(), 'Resend responded 401: unauthorized') $$,
'service_role can insert into notification_failures'
);
set local role anon;
select throws_ok(
$$ select * from public.notification_failures $$,
'42501',
null,
'anon cannot read notification_failures'
);
select throws_ok(
$$ insert into public.notification_failures (kind, record_id, error_message)
values ('booking', gen_random_uuid(), 'hack') $$,
'42501',
null,
'anon cannot insert into notification_failures'
);
set local role authenticated;
set local request.jwt.claim.sub = '21c90224-e0b2-412e-a431-8e7f097c1cef';
select isnt_empty(
$$ select * from public.notification_failures $$,
'admin can read notification_failures'
);
-- Self-healing unique index migration: replay the exact dedupe logic
-- 20260812000002 runs before creating bookings_active_slot_idx, proving
-- that pre-existing duplicate confirmed bookings for a slot get collapsed
-- to a single confirmed row (the earliest-created one) rather than making
-- the index creation fail outright.
-- DDL (drop/create index) requires table ownership, which service_role
-- doesn't have -- reset to the connecting (owner) role for this block. The
-- dedupe_confirmed_booking_slots() call further down still exercises the
-- service_role grant path via an explicit role switch.
reset role;
select lives_ok(
$$ drop index if exists public.bookings_active_slot_idx $$,
'temporarily dropping the unique index to simulate pre-migration data succeeds'
);
select lives_ok(
$$ insert into public.bookings (service_id, date, time, contact_name, contact_phone, contact_email, contact_notes, status, created_at)
values
('knotless', '2026-11-01', '4:00 PM', 'Dup Earliest', '555-0201', 'dup1@example.com', '', 'confirmed', now() - interval '1 hour'),
('knotless', '2026-11-01', '4:00 PM', 'Dup Later', '555-0202', 'dup2@example.com', '', 'confirmed', now()) $$,
'seeding two duplicate confirmed bookings for the same slot succeeds without the unique index'
);
-- Exercises the same execute grant the migration relies on: only
-- service_role can call this function (revoked from anon/authenticated).
set local role service_role;
select lives_ok(
$$ select public.dedupe_confirmed_booking_slots() $$,
'dedupe_confirmed_booking_slots() runs without error'
);
select is(
(select count(*)::int from public.bookings where date = '2026-11-01' and time = '4:00 PM' and status = 'confirmed'),
1,
'the self-heal dedupe leaves exactly one confirmed booking per duplicated slot'
);
select results_eq(
$$ select contact_name from public.bookings where date = '2026-11-01' and time = '4:00 PM' and status = 'confirmed' $$,
$$ values ('Dup Earliest'::text) $$,
'the self-heal dedupe keeps the earliest-created booking as confirmed'
);
-- Back to the owner role for the final DDL step.
reset role;
select lives_ok(
$$ create unique index bookings_active_slot_idx on public.bookings (date, time) where status = 'confirmed' $$,
'the unique index can be created after the self-heal dedupe removes the duplicate'
);
select * from finish();
rollback;