From c63a68c01ec311b62d976afab89abc921e60f68a Mon Sep 17 00:00:00 2001 From: Arnas Donauskas Date: Fri, 29 May 2026 17:50:11 +0200 Subject: [PATCH] chore(templates): polish pass from senior backend / FE / UX review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A focused cleanup PR that addresses the high-signal findings from a self-review of PRs #148-153. Every change is local to the templates surface; no schema or wire-format changes. Safety ------ * Delete now goes through a confirmation Dialog (mirrors the existing tag-manager pattern). Copy explicitly tells the user when Meta will be hit, so a slip on the trash icon doesn't take the template off Meta as well as locally. * AUTHENTICATION submit is now blocked by a persistent in-dialog banner + disabled submit button. The disappearing toast that used to fire on submit is gone — defensive `return` remains in handleSubmit as a second line of defense. * updateButton uses a per-variant switch + ButtonPatch union instead of the partial-spread + `as TemplateButton` cast. Each variant's spread keeps its discriminant pinned; TS enforces exhaustiveness. * New template-row-guard.ts surfaces malformed Supabase rows at the route boundary instead of letting buildSendComponents crash deep in the stack with an opaque TypeError. * UUID regex check on /[id] PATCH+DELETE so non-UUID payloads return a clean 400 before hitting Supabase. Submit-route cleanup -------------------- * Two near-identical upsert blocks (error path + success path) collapsed into a buildUpsertRow + upsertTemplateRow helper. Adding a column now touches one place, not two. UX copy + feedback ------------------ * Submit success toast sets the right expectation: "typical review time is 24 hours. Status updates automatically." Old copy just said "status will update once approved." * Header media URL helper text is now actually useful: HTTPS requirement, ~24h lifetime, per-format size/format recommendations (JPEG/PNG ≥800×418 ≤5 MB for images, MP4 ≤16 MB for video, etc). * Truncated-sync toast switches from `warning` (which auto-dismisses fast) to `error` with `duration: 10000` and concrete copy: "Synced the first 2000 templates only — your account has more. Sync again to continue." * Edit and Resubmit buttons gain visible text labels alongside their icons + better aria-label/title pairs. A lone rotate icon wasn't self-explanatory. * Broadcast picker drops the redundant "Approved" chip — every template in that list is APPROVED by filter, so the chip carried zero information. Unused templateStatusConfig import removed. State + a11y ------------ * handleSubmit now awaits fetchTemplates BEFORE closing the dialog so re-opening immediately doesn't see a stale list. * Header content / media URL / sample are preserved across format switches in the New Template form. The payload builder is already format-aware, so orphan values on hidden fields are harmless — and keeping them lets users switch formats to compare without losing typing. * Button-type Select gains the same null-guard the header Select got in PR #148 (per @base-ui semantics, onValueChange can fire with null on deselect). * Dynamic body-sample inputs get id + aria-label so each one is individually addressable (WCAG A — screen-reader announcement beat "edit text"). Header text + header sample inputs too. Deliberately NOT here --------------------- * The 940-line template-manager component is not split. It's cohesive (one feature surface) and splitting for size alone would be over-engineering. * extractVariableIndices in the buttons loop is not memoized. Regex on a short URL string is sub-microsecond; a real profiler would never flag it. * No JS heap secret-clearing for access tokens. Strings are immutable in JS — there's no way to actually clear one. Tests: 256/256 pass (no new tests required — pure refactor + copy + new UI elements). Lint 0 errors. Typecheck clean. Build succeeds. Co-Authored-By: Claude Opus 4.7 (1M context) --- src/app/api/whatsapp/broadcast/route.ts | 15 +- src/app/api/whatsapp/send/route.ts | 16 +- src/app/api/whatsapp/templates/[id]/route.ts | 18 ++ .../api/whatsapp/templates/submit/route.ts | 117 +++++---- .../broadcasts/step1-choose-template.tsx | 10 +- src/components/settings/template-manager.tsx | 241 ++++++++++++++---- src/lib/whatsapp/template-row-guard.ts | 51 ++++ 7 files changed, 354 insertions(+), 114 deletions(-) create mode 100644 src/lib/whatsapp/template-row-guard.ts diff --git a/src/app/api/whatsapp/broadcast/route.ts b/src/app/api/whatsapp/broadcast/route.ts index 11c1ade..78b640c 100644 --- a/src/app/api/whatsapp/broadcast/route.ts +++ b/src/app/api/whatsapp/broadcast/route.ts @@ -3,6 +3,7 @@ import { createClient } from '@/lib/supabase/server' import { sendTemplateMessage } from '@/lib/whatsapp/meta-api' import { decrypt } from '@/lib/whatsapp/encryption' import type { SendTimeParams } from '@/lib/whatsapp/template-send-builder' +import { isMessageTemplate } from '@/lib/whatsapp/template-row-guard' import { sanitizePhoneForMeta, isValidE164, @@ -137,13 +138,25 @@ export async function POST(request: Request) { // Load the template row once so sendTemplateMessage can build // header + button components on each iteration. Loading inside // the loop would N+1 against Supabase for every recipient. - const { data: templateRow } = await supabase + // Guard against a malformed local row crashing every send in + // the loop with the same opaque TypeError — fail loudly once. + const { data: rawTemplateRow } = await supabase .from('message_templates') .select('*') .eq('user_id', user.id) .eq('name', template_name) .eq('language', template_language || 'en_US') .maybeSingle() + if (rawTemplateRow && !isMessageTemplate(rawTemplateRow)) { + return NextResponse.json( + { + error: + 'Template row is malformed locally — run "Sync from Meta" in Settings to repair it before broadcasting.', + }, + { status: 500 }, + ) + } + const templateRow = rawTemplateRow ?? null const results: BroadcastResult[] = [] let sentCount = 0 diff --git a/src/app/api/whatsapp/send/route.ts b/src/app/api/whatsapp/send/route.ts index 8a8d8c0..d5ea7f2 100644 --- a/src/app/api/whatsapp/send/route.ts +++ b/src/app/api/whatsapp/send/route.ts @@ -15,6 +15,7 @@ import { RATE_LIMITS, } from '@/lib/rate-limit' import type { MessageTemplate } from '@/types' +import { isMessageTemplate } from '@/lib/whatsapp/template-row-guard' export async function POST(request: Request) { try { @@ -186,6 +187,10 @@ export async function POST(request: Request) { // index enforces — so multi-language templates work correctly. // Missing template falls through with `templateRow = null` and // the legacy body-only path runs. + // Load the template row so sendTemplateMessage can build header + // + button components from the definition. isMessageTemplate + // guards against a malformed row (e.g. from a partial sync) + // crashing the send-builder later in the stack. let templateRow: MessageTemplate | null = null if (message_type === 'template' && template_name) { const { data } = await supabase @@ -195,7 +200,16 @@ export async function POST(request: Request) { .eq('name', template_name) .eq('language', template_language || 'en_US') .maybeSingle() - templateRow = (data ?? null) as MessageTemplate | null + if (data && !isMessageTemplate(data)) { + return NextResponse.json( + { + error: + 'Template row is malformed locally — run "Sync from Meta" in Settings to repair it.', + }, + { status: 500 }, + ) + } + templateRow = data ?? null } const attempt = async (phone: string): Promise => { diff --git a/src/app/api/whatsapp/templates/[id]/route.ts b/src/app/api/whatsapp/templates/[id]/route.ts index 3fbb0e0..abf6b87 100644 --- a/src/app/api/whatsapp/templates/[id]/route.ts +++ b/src/app/api/whatsapp/templates/[id]/route.ts @@ -30,6 +30,12 @@ import { buildMetaTemplatePayload } from '@/lib/whatsapp/template-components' const EDITABLE_STATUSES = new Set(['APPROVED', 'REJECTED', 'PAUSED']) +// uuid v4 plus the looser shape Postgres gen_random_uuid emits. +// We don't need exhaustive RFC parsing — just enough to reject +// "../etc/passwd"-style payloads before they hit Supabase. +const UUID_RE = + /^[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}$/i + function isDryRun(): boolean { return ( process.env.WHATSAPP_TEMPLATES_DRY_RUN === 'true' || @@ -43,6 +49,12 @@ export async function PATCH( ) { try { const { id } = await context.params + if (!UUID_RE.test(id)) { + return NextResponse.json( + { error: 'Invalid template id.' }, + { status: 400 }, + ) + } const supabase = await createClient() const { data: { user }, @@ -197,6 +209,12 @@ export async function DELETE( ) { try { const { id } = await context.params + if (!UUID_RE.test(id)) { + return NextResponse.json( + { error: 'Invalid template id.' }, + { status: 400 }, + ) + } const supabase = await createClient() const { data: { user }, diff --git a/src/app/api/whatsapp/templates/submit/route.ts b/src/app/api/whatsapp/templates/submit/route.ts index 9ab5d4e..aafafd6 100644 --- a/src/app/api/whatsapp/templates/submit/route.ts +++ b/src/app/api/whatsapp/templates/submit/route.ts @@ -1,4 +1,5 @@ import { NextResponse } from 'next/server' +import type { SupabaseClient } from '@supabase/supabase-js' import { createClient } from '@/lib/supabase/server' import { decrypt } from '@/lib/whatsapp/encryption' import { submitMessageTemplate } from '@/lib/whatsapp/meta-api' @@ -9,6 +10,54 @@ import { import { buildMetaTemplatePayload } from '@/lib/whatsapp/template-components' import { normalizeStatus } from '@/lib/whatsapp/template-status-normalize' +/** + * Shared upsert payload builder — both the Meta-failure path and the + * Meta-success path write nearly identical rows; dropping the shared + * fields here means adding a column later only touches one spot. + */ +function buildUpsertRow( + userId: string, + payload: TemplatePayload, + extras: { + status: 'DRAFT' | string + metaTemplateId: string | null + submissionError: string | null + }, +) { + return { + user_id: userId, + name: payload.name, + category: payload.category, + language: payload.language, + header_type: payload.header_type ?? null, + header_content: payload.header_content ?? null, + header_media_url: payload.header_media_url ?? null, + header_handle: payload.header_handle ?? null, + body_text: payload.body_text, + footer_text: payload.footer_text ?? null, + buttons: payload.buttons ?? null, + sample_values: payload.sample_values ?? null, + status: extras.status, + meta_template_id: extras.metaTemplateId, + submission_error: extras.submissionError, + // Clear stale rejection_reason whenever we re-submit; the + // webhook will set it again if Meta still rejects. + rejection_reason: extras.submissionError ? null : null, + last_submitted_at: new Date().toISOString(), + } +} + +async function upsertTemplateRow( + supabase: SupabaseClient, + row: ReturnType, +) { + return supabase + .from('message_templates') + .upsert(row, { onConflict: 'user_id,name,language' }) + .select() + .single() +} + /** * Submit a template to Meta for approval AND persist it locally. * @@ -108,30 +157,16 @@ export async function POST(request: Request) { metaStatus = meta.status } catch (e) { const message = e instanceof Error ? e.message : 'Meta submit failed.' - // Persist the failure so the user can retry; tag with a 429 - // hint if Meta's rate-limit response leaked through. - await supabase - .from('message_templates') - .upsert( - { - user_id: user.id, - name: payload.name, - category: payload.category, - language: payload.language, - header_type: payload.header_type ?? null, - header_content: payload.header_content ?? null, - header_media_url: payload.header_media_url ?? null, - header_handle: payload.header_handle ?? null, - body_text: payload.body_text, - footer_text: payload.footer_text ?? null, - buttons: payload.buttons ?? null, - sample_values: payload.sample_values ?? null, - status: 'DRAFT', - submission_error: message, - last_submitted_at: new Date().toISOString(), - }, - { onConflict: 'user_id,name,language' }, - ) + // Persist the failure so the user can retry; row stays DRAFT + // until they fix and re-submit. + await upsertTemplateRow( + supabase, + buildUpsertRow(user.id, payload, { + status: 'DRAFT', + metaTemplateId: null, + submissionError: message, + }), + ) const isRateLimit = /\b429\b/.test(message) return NextResponse.json( { @@ -144,32 +179,14 @@ export async function POST(request: Request) { } } - const { data: row, error: upsertErr } = await supabase - .from('message_templates') - .upsert( - { - user_id: user.id, - name: payload.name, - category: payload.category, - language: payload.language, - header_type: payload.header_type ?? null, - header_content: payload.header_content ?? null, - header_media_url: payload.header_media_url ?? null, - header_handle: payload.header_handle ?? null, - body_text: payload.body_text, - footer_text: payload.footer_text ?? null, - buttons: payload.buttons ?? null, - sample_values: payload.sample_values ?? null, - status: normalizeStatus(metaStatus), - meta_template_id: metaTemplateId, - submission_error: null, - rejection_reason: null, - last_submitted_at: new Date().toISOString(), - }, - { onConflict: 'user_id,name,language' }, - ) - .select() - .single() + const { data: row, error: upsertErr } = await upsertTemplateRow( + supabase, + buildUpsertRow(user.id, payload, { + status: normalizeStatus(metaStatus), + metaTemplateId, + submissionError: null, + }), + ) if (upsertErr) { // The submit succeeded on Meta's side but we failed to persist diff --git a/src/components/broadcasts/step1-choose-template.tsx b/src/components/broadcasts/step1-choose-template.tsx index 8c2afd8..f766a2b 100644 --- a/src/components/broadcasts/step1-choose-template.tsx +++ b/src/components/broadcasts/step1-choose-template.tsx @@ -5,7 +5,6 @@ import { createClient } from '@/lib/supabase/client'; import { MessageTemplate } from '@/types'; import { Button } from '@/components/ui/button'; import { Loader2, FileText, ArrowRight } from 'lucide-react'; -import { templateStatusConfig } from '@/lib/template-status'; const categoryColors: Record = { Marketing: 'bg-purple-500/10 text-purple-400 border-purple-500/20', @@ -108,12 +107,9 @@ export function Step1ChooseTemplate({ selectedTemplate, onSelect, onNext, onBack

{template.body_text}

{template.language ?? 'en_US'} - {template.status && ( - <> - - - {templateStatusConfig[template.status].label} - - )} + {/* Status is omitted on purpose — every template + shown here is already filtered to APPROVED, + so the chip carried no information. */}
); diff --git a/src/components/settings/template-manager.tsx b/src/components/settings/template-manager.tsx index cacbe53..cf74850 100644 --- a/src/components/settings/template-manager.tsx +++ b/src/components/settings/template-manager.tsx @@ -132,6 +132,11 @@ export function TemplateManager() { // dialog title + CTA. Set to the template id to pre-fill from a row. const [editingId, setEditingId] = useState(null); const [deletingId, setDeletingId] = useState(null); + // Template selected for the confirm-delete dialog. The destructive + // action goes through this two-step so a slip on the trash icon + // doesn't take the template off Meta as well as locally. + const [templateToDelete, setTemplateToDelete] = + useState(null); // Body variable indices — `[1, 2, 3]` for "{{1}} {{2}} {{3}}". We // re-run the extractor on every render to keep the sample-value rows @@ -240,12 +245,9 @@ export function TemplateManager() { } async function handleSubmit() { - if (form.category === 'Authentication') { - toast.error( - 'AUTHENTICATION templates aren’t supported here yet — create them in Meta WhatsApp Manager and use Sync from Meta.', - ); - return; - } + // AUTHENTICATION is blocked by the persistent banner + disabled + // submit button; this is a defensive second line of defense. + if (form.category === 'Authentication') return; try { setSubmitting(true); const isEdit = editingId !== null; @@ -263,19 +265,21 @@ export function TemplateManager() { data?.error || `${isEdit ? 'Edit' : 'Submit'} failed (HTTP ${res.status})`, ); } + // Refresh first, then close — re-opening the dialog + // immediately should not show a stale list. + if (user) await fetchTemplates(user.id); toast.success( data.dry_run ? isEdit ? 'Template updated (dry-run — no Meta call)' : 'Template saved (dry-run — no Meta call)' : isEdit - ? 'Edit submitted — Meta will re-review.' - : 'Submitted to Meta — status will update once approved.', + ? 'Edit submitted — Meta typically reviews within 24 hours.' + : 'Submitted to Meta — typical review time is 24 hours. Status updates automatically.', ); setDialogOpen(false); setForm(emptyForm); setEditingId(null); - if (user) await fetchTemplates(user.id); } catch (err) { console.error('Submit error:', err); toast.error(err instanceof Error ? err.message : 'Failed to submit'); @@ -309,8 +313,12 @@ export function TemplateManager() { toast.error(`Failed to sync: ${preview.join(', ')}${suffix}`); } if (data.truncated) { - toast.warning( - 'Hit Meta pagination cap — more templates may exist. Contact support if this persists.', + // Use error (not warning) so the message survives long + // enough to read — sonner's `warning` auto-dismisses on + // the same short timer as `success`. + toast.error( + 'Synced the first 2000 templates only — your account has more. Sync again to continue, or contact support if this persists.', + { duration: 10000 }, ); } await fetchTemplates(user.id); @@ -322,14 +330,15 @@ export function TemplateManager() { } } - async function handleDelete(id: string) { - if (deletingId) return; - setDeletingId(id); + async function confirmDelete() { + const target = templateToDelete; + if (!target || deletingId) return; + setDeletingId(target.id); try { // Route handler scopes the Meta delete via hsm_id (so sibling // language variants survive) and falls through to remove the // local row. Local-only rows skip the Meta call. - const res = await fetch(`/api/whatsapp/templates/${id}`, { + const res = await fetch(`/api/whatsapp/templates/${target.id}`, { method: 'DELETE', }); const data = await res.json().catch(() => ({})); @@ -337,7 +346,8 @@ export function TemplateManager() { throw new Error(data?.error || `Delete failed (HTTP ${res.status})`); } toast.success('Template deleted'); - setTemplates((prev) => prev.filter((t) => t.id !== id)); + setTemplates((prev) => prev.filter((t) => t.id !== target.id)); + setTemplateToDelete(null); } catch (err) { console.error('Delete error:', err); toast.error(err instanceof Error ? err.message : 'Failed to delete template'); @@ -346,10 +356,57 @@ export function TemplateManager() { } } - function updateButton(index: number, patch: Partial) { + // The patch type unions every field across button variants. The + // conditional rendering below ensures only fields valid for the + // current button's `type` reach this function, so the runtime + // assertion + per-type spread preserves discriminated-union + // invariants without forcing every call site to thread the type + // through generics (which TS can't infer from a partial literal). + type ButtonPatch = { + text?: string; + url?: string; + phone_number?: string; + example?: string; + }; + function updateButton(index: number, patch: ButtonPatch) { setForm((prev) => { + const current = prev.buttons[index]; + if (!current) return prev; const next = [...prev.buttons]; - next[index] = { ...next[index], ...patch } as TemplateButton; + // Per-variant spread keeps the discriminant pinned. Switch + // exhaustiveness is enforced by TypeScript. + switch (current.type) { + case 'QUICK_REPLY': + next[index] = { + ...current, + ...(patch.text !== undefined && { text: patch.text }), + }; + break; + case 'URL': + next[index] = { + ...current, + ...(patch.text !== undefined && { text: patch.text }), + ...(patch.url !== undefined && { url: patch.url }), + ...(patch.example !== undefined && { example: patch.example }), + }; + break; + case 'PHONE_NUMBER': + next[index] = { + ...current, + ...(patch.text !== undefined && { text: patch.text }), + ...(patch.phone_number !== undefined && { + phone_number: patch.phone_number, + }), + }; + break; + case 'COPY_CODE': + next[index] = { + ...current, + ...(patch.text !== undefined && { text: patch.text }), + ...(patch.example !== undefined && { example: patch.example }), + }; + break; + } return { ...prev, buttons: next }; }); } @@ -487,40 +544,49 @@ export function TemplateManager() { )} -
+
{statusKey === 'APPROVED' && ( )} {(statusKey === 'REJECTED' || statusKey === 'PAUSED') && ( )}
)}
@@ -805,9 +887,13 @@ export function TemplateManager() {