From 55da996e5231f2501bd1d42293f2ae1728d99c39 Mon Sep 17 00:00:00 2001 From: Arnas Donauskas Date: Sun, 31 May 2026 19:18:58 +0300 Subject: [PATCH] perf(db,webhook): backend follow-ups from the multi-user review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit PR A of the post-review fix series. Addresses the two must-fix backend findings (flow-media account scoping; webhook contact lookup at scale) plus the two should-fix composite-index additions, all in one migration + two TS files. Migration 020 (`supabase/migrations/020_account_sharing_followups.sql`) - Composite partial index `idx_automations_account_active_trigger` on `automations(account_id, trigger_type) WHERE is_active`. The 006-era partial index was account-blind so the planner had to post-filter by account_id on every webhook dispatch. With 100+ admins in a shared account the post-filter step shows up in query plans; this index answers all three predicates from one lookup. - Same shape for flows: `idx_flows_account_active` on `flows(account_id) WHERE status='active'` — paired to the `findEntryFlow` query in the runner. - `flow-media` storage RLS rewritten to allow uploads / updates / deletes by any member of the account whose folder name matches `account-/...`. Legacy `/...` paths remain writable by the original uploader so files uploaded pre-020 don't break. Reads stay public (the bucket was already public so Meta can fetch URLs without creds). Webhook contact lookup (`src/app/api/whatsapp/webhook/route.ts`) - The PR-8 conversion swapped `eq('user_id', userId)` for `eq('account_id', accountId)` but kept "pull every row, filter in JS" semantics. That was fine when account = one user but scales linearly with the team size × per-user contact count. - Now pre-filters in SQL by the phone's last-8-digit suffix (mirroring `phonesMatch`'s trunk-prefix tolerance). The trailing-suffix `like` can't use a B-tree index, but the account_id filter on top of `idx_contacts_account` keeps the scan to a small account-scoped subset. Re-applies the strict `phonesMatch` check in JS on the candidate set for correctness — typically 0-2 rows, basically free. Flow-media upload path (`src/components/flows/forms/node-config-form.tsx`) - Resolves the caller's `profile.account_id` before upload and pathes the file under `account-/...` to match the new 020 RLS policy. Legacy `/...` paths keep working via the back-compat clause in the policy. Verification - `npm run lint` → 0 errors (same 19 pre-existing warnings) - `npm run typecheck` → clean - `npm test` → 366/366 pass - `npm run build` → succeeds Co-Authored-By: Claude Opus 4.7 (1M context) --- CHANGELOG.md | 22 ++++ src/app/api/whatsapp/webhook/route.ts | 31 ++++- .../flows/forms/node-config-form.tsx | 20 ++- .../020_account_sharing_followups.sql | 122 ++++++++++++++++++ 4 files changed, 187 insertions(+), 8 deletions(-) create mode 100644 supabase/migrations/020_account_sharing_followups.sql diff --git a/CHANGELOG.md b/CHANGELOG.md index 07547d0..527dcec 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -40,6 +40,28 @@ as the sole owner of their own account and sees identical data. ### Changed +- **Flow-media storage is now account-scoped.** Migration 016 + pathed uploaded files under `auth.uid()/...`, which orphaned + flow media when a teammate left a shared account. New uploads + go under `account-/...` and any account member + with the right role can edit them. Legacy paths remain + writable by the original uploader for backward compatibility. +- **Webhook contact lookup now pre-filters in SQL.** Previously + pulled every contact in an account just to JS-filter to one + row by phone — fine when account = one user, painful when + account = team. Pre-filter by phone suffix on the database + side; re-apply `phonesMatch` on the (typically 0-2 row) + candidate set. + +### Migration required + +- `supabase/migrations/020_account_sharing_followups.sql` — + composite partial indexes on `automations(account_id, + trigger_type) WHERE is_active` and `flows(account_id) WHERE + status='active'` for the engine dispatch hot path; updated + `flow-media` storage RLS to allow account-member writes under + the new path convention. Idempotent. + - **Role-aware UI gating across the app.** The inbox composer's send button + textarea, the "New broadcast / automation / flow" buttons, the "Add pipeline / deal" buttons, and the "Add / diff --git a/src/app/api/whatsapp/webhook/route.ts b/src/app/api/whatsapp/webhook/route.ts index 6006cdb..e7098ef 100644 --- a/src/app/api/whatsapp/webhook/route.ts +++ b/src/app/api/whatsapp/webhook/route.ts @@ -875,21 +875,42 @@ async function findOrCreateContact( phone: string, name: string ): Promise { - // Look up existing contacts for this account. Switched from - // user_id to account_id in PR 8 of the multi-user series so an - // inbound message lands on the team's existing contact row even - // if a different teammate created it. + // Look up existing contacts for this account. We pre-filter in SQL + // by the phone's last-8-digit suffix so we don't ship every contact + // in the account over the wire just to JS-filter to one row. This + // matters at scale: a shared account with 5 teammates × 100 contacts + // each is 500 rows — the prior implementation pulled all of them on + // every inbound message. + // + // The `phonesMatch` helper considers two phones equal if they share + // the last 8 digits (trunk-prefix tolerance). We mirror that here as + // a `like` pattern, then re-run the strict comparison in JS on the + // narrowed candidate set. The candidate set is typically 0-2 rows, + // so the JS pass is effectively free. + // + // The trailing-suffix `like` cannot use a B-tree index, but the + // `account_id` filter on top of `idx_contacts_account` (017) means + // we sequential-scan a small, account-scoped subset. + const normalizedSender = phone.replace(/\D/g, '') + const phoneSuffix = + normalizedSender.length >= 8 + ? normalizedSender.slice(-8) + : normalizedSender + const { data: contacts, error: contactsError } = await supabaseAdmin() .from('contacts') .select('*') .eq('account_id', accountId) + .like('phone', `%${phoneSuffix}`) if (contactsError) { console.error('Error fetching contacts:', contactsError) return null } - // Use phonesMatch for flexible matching + // Re-apply phonesMatch on the candidate set for correctness — the + // SQL `like` is a coarse pre-filter; phonesMatch handles edge cases + // like leading-`+` and explicit trunk-zero handling. const existingContact = contacts?.find((c: ContactRow) => phonesMatch(c.phone, phone)) if (existingContact) { diff --git a/src/components/flows/forms/node-config-form.tsx b/src/components/flows/forms/node-config-form.tsx index 9f12718..928c713 100644 --- a/src/components/flows/forms/node-config-form.tsx +++ b/src/components/flows/forms/node-config-form.tsx @@ -913,8 +913,22 @@ function SendMediaForm({ if (userErr || !user) { throw new Error("Not signed in."); } - // Path convention matches migration 016's RLS policy: first - // segment must equal auth.uid(). Timestamp + random suffix + // Resolve the caller's account_id so the path is account- + // scoped rather than user-scoped. The 020 migration's RLS + // policy allows any account member to write under + // `account-/...`, so a flow's media survives a + // teammate leaving the account (which was a data-integrity + // hole in the original 016 storage policies). + const { data: profile, error: profileErr } = await supabase + .from("profiles") + .select("account_id") + .eq("user_id", user.id) + .maybeSingle(); + if (profileErr || !profile?.account_id) { + throw new Error("Could not resolve your account."); + } + // Path convention matches migration 020's RLS policy: first + // segment is `account-`. Timestamp + random suffix // prevents collisions between two builders open at once. const ext = file.name.split(".").pop()?.toLowerCase() ?? "bin"; const safeBase = @@ -922,7 +936,7 @@ function SendMediaForm({ .replace(/\.[^.]+$/, "") .replace(/[^a-zA-Z0-9_-]+/g, "_") .slice(0, 40) || "file"; - const path = `${user.id}/${Date.now()}-${safeBase}.${ext}`; + const path = `account-${profile.account_id}/${Date.now()}-${safeBase}.${ext}`; const { error: upErr } = await supabase.storage .from(FLOW_MEDIA_BUCKET) .upload(path, file, { diff --git a/supabase/migrations/020_account_sharing_followups.sql b/supabase/migrations/020_account_sharing_followups.sql new file mode 100644 index 0000000..e74348c --- /dev/null +++ b/supabase/migrations/020_account_sharing_followups.sql @@ -0,0 +1,122 @@ +-- ============================================================ +-- 020_account_sharing_followups.sql — review-board fixes for +-- the multi-user accounts series (#167-#177). +-- +-- Two concerns this migration addresses: +-- +-- 1. Engine dispatch indexes — the per-inbound automations and +-- flows lookups now scope by `account_id + trigger_type/status +-- + is_active/status='active'`. The pre-017 partial indexes +-- (`idx_automations_active_trigger`, no flows equivalent) were +-- account-blind. For shared accounts with 100+ teammates each +-- authoring rules, the planner ends up post-filtering by +-- account_id. Composite partial indexes drop the post-filter +-- cost to zero on the hot path. +-- +-- 2. Flow-media storage scoping — migration 016 created the +-- `flow-media` bucket with per-user RLS policies keyed on +-- `auth.uid() = path[0]`. After the multi-user move, flows +-- are account-scoped but the storage paths remained user- +-- scoped: an agent who left the account would orphan every +-- flow node referencing media they had uploaded. This +-- migration switches the write policies to account-scoped +-- paths (`account-/...`) while leaving the +-- legacy `/...` paths writable by their original +-- uploader for backward compatibility. The bucket is public, +-- so reads are unchanged. +-- +-- Idempotent — safe to run multiple times. +-- ============================================================ + +-- ============================================================ +-- COMPOSITE INDEXES — engine dispatch hot path +-- ============================================================ + +-- `runAutomationsForTrigger` queries +-- automations WHERE account_id = X AND trigger_type = Y AND is_active = TRUE +-- Migration 006 added a partial index on (trigger_type) WHERE is_active. +-- Composite + partial index lets the planner answer all three predicates +-- from one index lookup. The existing partial index can stay as belt-and- +-- braces for any code path that filters only by trigger_type. +CREATE INDEX IF NOT EXISTS idx_automations_account_active_trigger + ON automations(account_id, trigger_type) + WHERE is_active = TRUE; + +-- `findEntryFlow` queries +-- flows WHERE account_id = X AND status = 'active' +-- Migration 017 only added `idx_flows_account`; this partial composite +-- is tuned for the engine's lookup and skips archived/draft rows. +CREATE INDEX IF NOT EXISTS idx_flows_account_active + ON flows(account_id) + WHERE status = 'active'; + +-- ============================================================ +-- FLOW-MEDIA STORAGE — account-scoped writes +-- +-- New path convention: `account-/-.` +-- Legacy path convention: `/-.` (where +-- the uuid is auth.uid() — preserved for back-compat). +-- +-- Reads stay public (the bucket is public so Meta can fetch media +-- URLs without credentials). Only the write policies change. +-- +-- Drop existing per-user policies and replace with account-aware +-- ones that accept either path convention. +-- ============================================================ +DROP POLICY IF EXISTS "Users can upload their own flow media" ON storage.objects; +DROP POLICY IF EXISTS "Users can update their own flow media" ON storage.objects; +DROP POLICY IF EXISTS "Users can delete their own flow media" ON storage.objects; + +DROP POLICY IF EXISTS "Members can upload flow media" ON storage.objects; +CREATE POLICY "Members can upload flow media" + ON storage.objects FOR INSERT + WITH CHECK ( + bucket_id = 'flow-media' + AND ( + -- New: any account member uploading under their account's folder. + -- `'account-' || account_id` is how we namespace the folder, so + -- two accounts that happen to be in the same Supabase project + -- can never accidentally collide. + EXISTS ( + SELECT 1 FROM public.profiles p + WHERE p.user_id = auth.uid() + AND ('account-' || p.account_id::text) = (storage.foldername(name))[1] + ) + -- Legacy: the original uploader keeps write access to files they + -- already uploaded under the pre-020 path convention. + OR auth.uid()::text = (storage.foldername(name))[1] + ) + ); + +DROP POLICY IF EXISTS "Members can update flow media" ON storage.objects; +CREATE POLICY "Members can update flow media" + ON storage.objects FOR UPDATE + USING ( + bucket_id = 'flow-media' + AND ( + EXISTS ( + SELECT 1 FROM public.profiles p + WHERE p.user_id = auth.uid() + AND ('account-' || p.account_id::text) = (storage.foldername(name))[1] + ) + OR auth.uid()::text = (storage.foldername(name))[1] + ) + ); + +DROP POLICY IF EXISTS "Members can delete flow media" ON storage.objects; +CREATE POLICY "Members can delete flow media" + ON storage.objects FOR DELETE + USING ( + bucket_id = 'flow-media' + AND ( + EXISTS ( + SELECT 1 FROM public.profiles p + WHERE p.user_id = auth.uid() + AND ('account-' || p.account_id::text) = (storage.foldername(name))[1] + ) + OR auth.uid()::text = (storage.foldername(name))[1] + ) + ); + +-- Public read policy from 016 stays as-is; reads cross both path +-- conventions without modification.