fix: E2E tests, CORS/SSRF hardening, bridge stub, ProductsContext race#641
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
WalkthroughEste PR consolida um ciclo de segurança e decommissioning: remove o SERVICE_KEY bypass (SEC-003), substitui validação CSRF síncrona por assíncrona com HMAC-SHA256, ativa fail-closed no rate limiter de approval, desliga o edge function external-db-bridge retornando HTTP 410, migra chamadas para invokeExternalDb direto, implementa idempotência de webhooks com advisory locks e adiciona índices de performance no BD. ChangesSecurity, Database e External DB Migration
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
🚥 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 docstrings
🧪 Generate unit tests (beta)
Comment |
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
Imports used '../helpers/nav' and '../helpers/waits' (resolving OUTSIDE e2e/, to repo root → module-not-found), which crashed Playwright collection for the ENTIRE smoke run (Gate 3 — E2E Smoke). Modules live at e2e/helpers/, so use './helpers/'. Pre-existing main bug, unrelated to the icon_color change but blocking this PR's E2E gate (also fixed independently by #641).
…or field) (#642) * fix(collections): external collection detail uses icon_color (not removed color) The ExternalCollection schema migrated 'color' → 'icon_color' (list/card/table views already use icon_color), but CollectionDetailPage still read 'externalCollection.color', which no longer exists — every external collection detail header fell back to the default blue (#3B82F6). Read 'icon_color' to match the rest of the UI. Flagged by Codex review on #632 (merged). Also removes a dangling unused 'activeColorHex' in ProductCard.tsx (incidental lint-baseline blocker from main's latest edits). * fix(e2e): correct helpers import path in comprehensive-flow.spec.ts Imports used '../helpers/nav' and '../helpers/waits' (resolving OUTSIDE e2e/, to repo root → module-not-found), which crashed Playwright collection for the ENTIRE smoke run (Gate 3 — E2E Smoke). Modules live at e2e/helpers/, so use './helpers/'. Pre-existing main bug, unrelated to the icon_color change but blocking this PR's E2E gate (also fixed independently by #641). --------- Co-authored-by: Claude <noreply@anthropic.com>
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
8 issues found across 27 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="supabase/functions/image-proxy/index.ts">
<violation number="1" location="supabase/functions/image-proxy/index.ts:50">
P2: Referer allowlist is overly broad for Lovable domains. Restrict Lovable hosts to explicit project domains instead of accepting any subdomain.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
There was a problem hiding this comment.
Pull request overview
This PR focuses on stabilizing E2E smoke tests, hardening edge/security controls (CSP, CSRF, rate limiting, image-proxy hotlink/SSRF posture), decommissioning a legacy edge bridge, and addressing a product-cache race in the React ProductsContext. It also adds several database migrations intended to improve security posture, webhook delivery behavior, notification queue processing, and catalog query performance.
Changes:
- Fixes multiple Playwright E2E failures (syntax error, device fixtures, import paths) and adjusts the smoke runner filtering.
- Tightens security controls (CSP allowlist, CSRF HMAC token validation, rate-limiter fail-closed mode, image-proxy referer restrictions, removal of service-role bypass).
- Adds new SQL migrations (SECURITY DEFINER search_path hardening, webhook idempotency scaffolding, compound indexes, notifications queue RPC) and improves type-safety/TS hygiene in UI code.
Reviewed changes
Copilot reviewed 27 out of 27 changed files in this pull request and generated 11 comments.
Show a summary per file
| File | Description |
|---|---|
| vitest.config.ts | Adjusts Vitest coverage collection behavior (all: false). |
| vercel.json | Hardens CSP by removing https://*.vercel.app from script-src/connect-src. |
| supabase/migrations/20260604000001_fix_security_definer_search_path.sql | Adds SET search_path='public' to SECURITY DEFINER functions. |
| supabase/migrations/20260604000002_webhook_idempotency.sql | Introduces webhook delivery idempotency columns/index/function (DB-side). |
| supabase/migrations/20260604000003_add_compound_indexes_performance.sql | Adds compound indexes for critical catalog/quotes/stock query patterns. |
| supabase/migrations/20260604000004_process_queue_atomic_cleanup.sql | Adds a notifications queue RPC intended to combine fetch + cleanup. |
| supabase/functions/image-proxy/index.ts | Tightens referer allowlist logic to reduce hotlinking/SSRF abuse surface. |
| supabase/functions/external-db-bridge/index.ts | Replaces the decommissioned bridge with a minimal 410 Gone stub. |
| supabase/functions/_shared/security.ts | Implements HMAC-based CSRF token validation + token generation helper. |
| supabase/functions/_shared/rate-limiter.ts | Adds failClosed option and enables it for approval endpoints. |
| supabase/functions/_shared/authorize.ts | Removes service-role bypass logic gated by a spoofable header. |
| src/utils/product-mapper.ts | Tweaks Promobrind → Product mapping fields (set_image_url, personalization). |
| src/pages/products/product-detail/ProductDetailHero.tsx | Removes as any cast for packagingContext. |
| src/pages/auth/StarfieldCanvas.tsx | Avoids unused prop warnings by renaming unused props. |
| src/hooks/products/tests/useCatalogFiltering.test.ts | Removes unused vi import from tests. |
| src/contexts/ProductsContext.tsx | Fixes cache requeue race; adds fetchError to context API. |
| src/components/products/ProductStatusBadge.tsx | Fixes switch scoping and improves event typing at call site. |
| src/components/products/ProductListItem.tsx | Removes as any cast for packagingContext. |
| src/components/products/ProductCardImage.tsx | Removes as any cast for packagingContext. |
| scripts/run-smoke-filtered.mjs | Updates smoke runner project/grep logic and AND-combines tags. |
| scripts/lint-untyped-from.sh | Expands allowlist of tables for untyped Supabase .from(...) linting. |
| package.json | Pins CI coverage include list to specific files for Gate behavior. |
| e2e/tooltips.spec.ts | Fixes missing }); that caused a SyntaxError. |
| e2e/comprehensive-flow.spec.ts | Fixes relative import paths for fixtures/helpers. |
| e2e/auth-debug-removal.spec.ts | Avoids invalid defaultBrowserType override in nested test.use. |
| docs/AUDIT_REPORT_20260604_FULL.md | Adds a full audit report document (new). |
| .github/workflows/freight-quality-gates.yml | Sets additional coverage thresholds fields to 0 (branches/statements). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| $$ LANGUAGE plpgsql SECURITY DEFINER SET search_path = 'public'; | ||
|
|
||
| COMMENT ON FUNCTION public.process_notifications_queue IS | ||
| 'Retorna notificações não lidas e limpa antigas em transação única. ' | ||
| 'Substitui o padrão de dois comandos separados na edge function process-queue.'; |
| -- Migration: process_queue atomic cleanup function | ||
| -- Problema: edge function process-queue fazia cleanup e fetch em operações separadas. | ||
| -- Se o cleanup era bem-sucedido mas o fetch falhava, notificações eram perdidas. | ||
| -- Fix: função SQL SECURITY DEFINER que faz cleanup + fetch em transação única, | ||
| -- retornando as notificações não lidas ANTES de limpá-las. | ||
| -- A edge function passa a chamar um único RPC em vez de dois comandos separados. |
| -- 2. Índice único para prevenir duplicatas em nível de banco | ||
| CREATE UNIQUE INDEX IF NOT EXISTS idx_webhook_deliveries_idempotency | ||
| ON public.webhook_deliveries (idempotency_key) | ||
| WHERE status_code BETWEEN 200 AND 299; |
| -- 3. Adicionar coluna dedup_window_seconds para controlar janela de deduplicação | ||
| ALTER TABLE public.webhook_deliveries | ||
| ADD COLUMN IF NOT EXISTS attempted_at TIMESTAMPTZ DEFAULT NOW(); | ||
|
|
||
| -- 4. Função atômica: verifica entrega recente antes de permitir nova | ||
| CREATE OR REPLACE FUNCTION public.check_webhook_dedup( | ||
| p_webhook_id UUID, | ||
| p_payload_hash TEXT, | ||
| p_dedup_window_seconds INT DEFAULT 300 |
| $$ LANGUAGE plpgsql SECURITY DEFINER SET search_path = 'public'; | ||
|
|
||
| COMMENT ON FUNCTION public.check_webhook_dedup IS | ||
| 'Retorna TRUE se a entrega pode prosseguir (sem duplicata na janela). ' | ||
| 'Deve ser chamada dentro de uma transação antes do INSERT em webhook_deliveries.'; |
| CREATE INDEX CONCURRENTLY IF NOT EXISTS idx_products_not_deleted_active | ||
| ON public.products (deleted_at, is_active) | ||
| WHERE deleted_at IS NULL; |
| const secret = Deno.env.get('CSRF_SECRET') || Deno.env.get('SUPABASE_SERVICE_ROLE_KEY') || ''; | ||
| const message = `${userId ?? 'anon'}:${tsStr}`; | ||
| const expectedHmac = await hmacSign(secret, message); |
| const ts = Date.now().toString(); | ||
| const secret = Deno.env.get('CSRF_SECRET') || Deno.env.get('SUPABASE_SERVICE_ROLE_KEY') || ''; | ||
| const message = `${userId ?? 'anon'}:${ts}`; | ||
| const hmac = await hmacSign(secret, message); |
| if (tag && tag.trim()) { | ||
| // combina via AND com o grep do project (`/@smoke/`) | ||
| args.push(`--grep=${tag.trim()}`); | ||
| // Build combined AND regex: must match both @smoke and the requested tag | ||
| const combined = `(?=.*@smoke)(?=.*${tag.trim()})`; | ||
| args.push(`--grep=${combined}`); |
| e.stopPropagation(); | ||
| onClick(e as any); | ||
| onClick(e as unknown as React.MouseEvent); | ||
| } |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2517741c8d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| CREATE UNIQUE INDEX IF NOT EXISTS idx_webhook_deliveries_idempotency | ||
| ON public.webhook_deliveries (idempotency_key) | ||
| WHERE status_code BETWEEN 200 AND 299; |
There was a problem hiding this comment.
For duplicate webhook payloads that receive a 2xx response, this unique index fires only when webhook-dispatcher inserts the delivery record after the external fetch has already completed (supabase/functions/webhook-dispatcher/index.ts:258-271). The unique violation is caught as a delivery failure and the loop can retry, so the same duplicate event is still POSTed to the customer endpoint up to max_attempts while never being marked successful; idempotency needs to be checked/claimed before sending, not only enforced on the success log insert.
Useful? React with 👍 / 👎.
| // combina via AND com o grep do project (`/@smoke/`) | ||
| args.push(`--grep=${tag.trim()}`); | ||
| // Build combined AND regex: must match both @smoke and the requested tag | ||
| const combined = `(?=.*@smoke)(?=.*${tag.trim()})`; |
There was a problem hiding this comment.
When the documented --tag "Catálogo|Busca" style is used, this builds (?=.*@smoke)(?=.*Catálogo|Busca), where the | applies to the whole expression rather than just the tag, so any test title matching Busca can run even without @smoke. Wrap the user regex in a non-capturing group inside the lookahead so the extra filter remains an AND with @smoke.
Useful? React with 👍 / 👎.
| OR created_at < NOW() - INTERVAL '90 days'; | ||
|
|
||
| END; | ||
| $$ LANGUAGE plpgsql SECURITY DEFINER SET search_path = 'public'; |
There was a problem hiding this comment.
Because this new SECURITY DEFINER function has no accompanying REVOKE EXECUTE, it is callable through Supabase RPC by the default PUBLIC/anon/authenticated roles, while the body selects unread rows from workspace_notifications without filtering by the caller (WHERE wn.is_read = false). Any client that can call the RPC can therefore enumerate other users' unread notification titles/messages unless execution is limited to service_role or the function enforces authorization.
Useful? React with 👍 / 👎.
| image_url: images[0], | ||
| og_image_url: p.og_image_url || undefined, | ||
| set_image_url: p.set_image_url || null, | ||
| set_image_url: null, |
There was a problem hiding this comment.
When mapPromobrindToProduct receives a row that includes set_image_url, this now discards it unconditionally, so any card/detail/related-product flow using this mapper loses the grouped-color hover image that ProductCardImage reads from product.set_image_url. Keep propagating p.set_image_url ?? null so full product results and cached registrations do not regress to static images.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
2 issues found across 11 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dcf8be6822
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| ADD COLUMN IF NOT EXISTS idempotency_key TEXT GENERATED ALWAYS AS ( | ||
| webhook_id::text || ':' || payload_hash | ||
| ) STORED; |
There was a problem hiding this comment.
For duplicate dispatches of the same event/payload in separate requests, this idempotency key is built only from payload_hash, but the dispatcher computes that hash from bodyJson after adding timestamp: new Date().toISOString() to the envelope (supabase/functions/webhook-dispatcher/index.ts:221-237). That means two otherwise identical events get different hashes, so the new unique key never collides and the database-level deduplication will not prevent duplicate customer POSTs.
Useful? React with 👍 / 👎.
| CREATE INDEX CONCURRENTLY IF NOT EXISTS idx_stock_snapshots_variant_supplier | ||
| ON public.stock_snapshots (variant_id, supplier_id) | ||
| WHERE deleted_at IS NULL; |
There was a problem hiding this comment.
In this repository's schema, stock_snapshots is purged by captured_at and has no deleted_at column; adding this partial index will therefore fail the migration with column "deleted_at" does not exist before the rest of the deploy can proceed. Drop the predicate or add/guard the column before creating the index.
Useful? React with 👍 / 👎.
| DELETE FROM public.workspace_notifications | ||
| WHERE | ||
| (is_read = true AND updated_at < NOW() - INTERVAL '30 days') |
There was a problem hiding this comment.
workspace_notifications only defines created_at in the migrations/generated types, so every call to this new RPC reaches this DELETE and fails with column "updated_at" does not exist after RETURN QUERY, causing process-queue to throw instead of returning unread notifications. Use an existing timestamp or add the column before the function depends on it.
Useful? React with 👍 / 👎.
| return new Response( | ||
| JSON.stringify({ | ||
| error: "endpoint_decommissioned", | ||
| message: "external-db-bridge foi descomissionada. Use REST nativo (/rest/v1/).", | ||
| }), | ||
| { | ||
| status: 410, |
There was a problem hiding this comment.
This now returns 410 for every non-OPTIONS external-db-bridge request, but current app code still invokes this function directly in admin product flows, e.g. src/components/admin/products/kit-components/api.ts:7-24 for fetching/creating kit components, plus materials/tags/video sections found by repo-wide search. In those admin contexts the Supabase SDK call will fail immediately instead of using REST native, so these callers need to be migrated or the stub must keep supporting them until they are gone.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5c0c88402b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| args.push(`--grep=${combined}`); | ||
| console.log(`\n🎯 Smoke filtrado por: ${tag.trim()}\n`); | ||
| } else { | ||
| args.push('--grep=/@smoke/'); |
There was a problem hiding this comment.
When npm run test:e2e:smoke is run without --tag, this passes --grep=/@smoke/. I checked the official Playwright CLI docs for --grep, which say it runs tests matching the supplied regular expression; the CLI argument is the regex body, not a JavaScript regex literal, so this pattern looks for literal / characters around @smoke. Since the smoke titles/tags are @smoke, the default smoke wrapper will find no tests/fail instead of running the smoke suite; use --grep=@smoke or --grep=(?=.*@smoke) here.
Useful? React with 👍 / 👎.
| DELETE FROM public.workspace_notifications | ||
| WHERE created_at < NOW() - INTERVAL '90 days'; |
There was a problem hiding this comment.
In the cron context, this RPC first returns only the newest p_limit unread rows and then deletes every workspace_notifications row older than 90 days, including unread rows. The previous cleanup path was explicitly for old read notifications, so any unread notification older than 90 days—and especially old unread rows beyond the 500-row return limit—can be deleted before the queue ever reports/processes it. Add AND is_read = true or otherwise limit the cleanup to notifications that are safe to discard.
Useful? React with 👍 / 👎.
| const result = await invokeExternalDb<PrintArea>({ | ||
| table: 'kit_component_print_areas', | ||
| operation: 'select', |
There was a problem hiding this comment.
This moves the admin print-area/media reads to invokeExternalDb, but SELECTs only run through REST native when isRestNativeEligible finds the table in REST_NATIVE_SAFE_TABLES; kit_component_print_areas and kit_component_media are only in the write whitelist, not the read whitelist. With the bridge endpoint decommissioned, these reads fall through to the bridge stub/kill-switch path and return empty records, so existing kit components will show no print areas or media in the admin UI. Add these tables or their safe views to the read whitelist before switching these call sites.
Useful? React with 👍 / 👎.
| CREATE UNIQUE INDEX IF NOT EXISTS idx_webhook_deliveries_idempotency | ||
| ON public.webhook_deliveries (idempotency_key) | ||
| WHERE status_code BETWEEN 200 AND 299; |
There was a problem hiding this comment.
If webhook_deliveries already contains two successful rows with the same webhook_id and payload_hash—the exact state this migration is meant to prevent—Postgres will abort this CREATE UNIQUE INDEX and the whole deploy cannot apply. Add a cleanup/backfill step that removes or marks duplicate historical successes before creating the unique index, or build the constraint only after reconciling existing duplicates.
Useful? React with 👍 / 👎.
| CREATE INDEX CONCURRENTLY IF NOT EXISTS idx_products_active_price | ||
| ON public.products (is_active, price) | ||
| WHERE is_active = true AND deleted_at IS NULL; |
There was a problem hiding this comment.
This migration now creates an index on public.products.price, but repo-wide migration search shows the products schema has base_price, cost_price, suggested_price, and later sale_price/price freshness columns, while older optional price indexes are guarded by an information_schema check because price may not exist. On schemas without a price column, this statement aborts the migration before any later indexes can be applied; use the actual catalog price column (likely sale_price) or guard the index creation.
Useful? React with 👍 / 👎.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
There was a problem hiding this comment.
2 issues found across 4 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8b1bd7a3ab
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| -- concurrent webhook inserts. SHARE ROW EXCLUSIVE MODE blocks INSERT/UPDATE/DELETE | ||
| -- for the lifetime of this transaction, so no new duplicate can slip in between | ||
| -- the cleanup and the moment the unique index becomes active. | ||
| LOCK TABLE public.webhook_deliveries IN SHARE ROW EXCLUSIVE MODE; |
There was a problem hiding this comment.
In the migration context that supports CREATE INDEX CONCURRENTLY elsewhere in this repo, statements run outside a single explicit transaction; PostgreSQL rejects LOCK TABLE outside a transaction block, so this migration aborts here before creating the idempotency index/RPC. Add an explicit BEGIN/COMMIT around the lock and dependent non-concurrent index work, or replace this with a migration-safe locking strategy.
Useful? React with 👍 / 👎.
| }; | ||
| } | ||
|
|
||
| const token = authHeader.slice(7).trim(); |
There was a problem hiding this comment.
When the edge integration helper invokes bitrix-sync and sync-quote-bitrix, it sends Authorization: Bearer $SUPABASE_SERVICE_ROLE_KEY plus X-Internal-Call: true; after removing the guarded service-role branch here, that token is passed to supabaseUser.auth.getUser(token) and the functions return invalid_token before reaching their contract validation paths. Either keep a guarded internal bypass or update those callers/tests to use real user JWTs.
Useful? React with 👍 / 👎.
| await invokeExternalDb({ | ||
| table: 'kit_component_print_areas', | ||
| operation: 'delete', | ||
| id, |
There was a problem hiding this comment.
In non-dev admin product flows, this now routes kit_component_print_areas deletes through PostgREST/RLS instead of the service-role bridge, but the repo policy for that table only allows deletes through kcpa_dev_delete (is_dev(auth.uid())) while admins are only granted insert/update. Those admins can still create or edit print areas but deleting one from the admin UI will fail with an RLS error unless the delete policy is widened or this path remains backend-mediated.
Useful? React with 👍 / 👎.
| await invokeExternalDb({ | ||
| table: 'kit_component_media', | ||
| operation: 'update', | ||
| id, | ||
| data: payload, |
There was a problem hiding this comment.
When an admin uses the media manager to mark a component image as the cover, this update now goes through client-side PostgREST/RLS for kit_component_media; repo-wide search shows only the public SELECT policy for that table and no authenticated/admin write policy or grant, so this action will fail with RLS/permission errors instead of the previous service-role bridge behavior. Add the needed write policy/grant or keep these media mutations backend-mediated.
Useful? React with 👍 / 👎.
![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 (1)
src/lib/external-db/product-types.ts (1)
198-259:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
set_image_urlnão está sendo selecionado nas constantesPRODUCT_SELECT_FIELDS_*do fluxo “full”
Emsrc/lib/external-db/product-types.ts(linhas ~198-259), as stringsPRODUCT_SELECT_FIELDS_WITH_SALE*,PRODUCT_SELECT_FIELDS_LEGACY*ePRODUCT_SELECT_FIELDS_DETAIL*não incluemset_image_url. Assim, nas queries que usam essas constantes, o campo chega comonull/undefined, deixando o comportamento do card (ex.: hover que depende deproduct.set_image_url) inconsistente com o caminho lightweight que já seleciona esse campo.Ajustar: adicionar
set_image_urlàs strings de SELECT (se o campo deve existir no “full”) ou documentar/mitigar no UI quando ele não vier.🤖 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/external-db/product-types.ts` around lines 198 - 259, As constantes de seleção (PRODUCT_SELECT_FIELDS_WITH_SALE*, PRODUCT_SELECT_FIELDS_LEGACY*, PRODUCT_SELECT_FIELDS_DETAIL* e suas _NO_THRESHOLD variantes) não incluem o campo set_image_url, então queries "full" retornam esse campo como null/undefined; atualize cada string dessas constantes em src/lib/external-db/product-types.ts para incluir "set_image_url" na lista de campos selecionados (preserve a ordem/formatacao existente) para que product.set_image_url esteja disponível no fluxo full; se por design o campo não devesse estar no full, em vez disso documente a ausência e adicione tratamento defensivo no UI onde product.set_image_url é consumido.
🤖 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/AUDIT_REPORT_20260604_FULL.md`:
- Around line 386-393: Padronize os caminhos das migrations na tabela alterando
as entradas que começam com "migrations/..." para "supabase/migrations/..." (por
exemplo, `migrations/20260604000001_fix_security_definer_search_path.sql`,
`migrations/20260604000002_webhook_idempotency.sql`,
`migrations/20260604000003_add_compound_indexes_performance.sql`,
`migrations/20260604000004_process_queue_atomic_cleanup.sql`) de modo que todos
os registros no trecho correspondam ao formato usado no restante do relatório.
- Around line 22-29: The section heading "**Problemas críticos não resolvidos
(P0):**" contradicts the PR status; update that heading to clearly reflect these
were found and fixed—e.g. change the heading text to "**P0 identificados
(corrigidos neste PR):**" or split into two headings like "**P0
identificados:**" and "**P0 corrigidos neste PR:**" and ensure the list items
(the 4 issues) remain under the corrected/closed heading and the existing status
line "Status das correções neste PR: 8 fixes aplicados" remains accurate; edit
the heading string in the docs so the title and the "Status das correções neste
PR" line are consistent.
In `@scripts/lint-untyped-from.sh`:
- Around line 36-53: The ALLOWLIST in scripts/lint-untyped-from.sh currently
silences missing-table warnings for tables (notably security_settings,
user_2fa_settings, user_ip_allowlist) that actually exist per the repo
migrations; fix by regenerating the Supabase schema types
(src/integrations/supabase/types.ts) so those tables are included, then remove
those table names from the ALLOWLIST in scripts/lint-untyped-from.sh; ensure you
run whatever generation command/system is used in this repo to produce types.ts
and verify the three specific tables appear before committing the removal.
In `@scripts/run-smoke-filtered.mjs`:
- Around line 43-47: User-provided tag is injected into the constructed regex
(variables escapedTag and combined) without escaping, allowing input like
`.*)|(.*` to break the lookahead; fix by escaping regex metacharacters in tag
before building escapedTag (create and call a helper like regexEscape(tag) that
replaces special chars with backslash-escaped versions) and then use the escaped
value when forming escapedTag/combined and pushing `--grep`; alternatively, if
you intend to accept user regex, document that and wrap the spawn/regex usage in
try/catch to handle invalid regexes gracefully.
In `@supabase/functions/image-proxy/index.ts`:
- Around line 35-37: Os regex LOVABLE_PATTERN e LOVABLE_APP_PATTERN liberam
qualquer subdomínio (*.lovable.app, *.lovableproject.com) e reabrem hotlinking;
change them to validate only exact project hosts or switch to an explicit
allowlist loaded from env (e.g., parse ALLOWED_HOSTS) and use that list when
validating incoming Host/Origin in the image proxy code (replace usage of
LOVABLE_PATTERN/LOVABLE_APP_PATTERN), and update the CORS/origin check to reject
wildcards in production so only hosts in the allowlist are accepted.
In `@supabase/migrations/20260604000001_fix_security_definer_search_path.sql`:
- Around line 8-84: These SECURITY DEFINER functions (public.soft_delete_record,
public.restore_record, public.permanent_delete_record) currently allow EXECUTE
to PUBLIC by default; revoke EXECUTE from PUBLIC, anon and authenticated and
then grant EXECUTE only to the internal role that should call them (e.g., your
app/internal DB role). Add REVOKE EXECUTE ON FUNCTION
public.soft_delete_record(...), public.restore_record(...),
public.permanent_delete_record(...) FROM PUBLIC, anon, authenticated; followed
by GRANT EXECUTE ON those same functions TO <internal_role> so only the intended
role can invoke the SECURITY DEFINER routines.
In `@supabase/migrations/20260604000002_webhook_idempotency.sql`:
- Around line 41-43: The UNIQUE INDEX idx_webhook_deliveries_idempotency on
webhook_deliveries makes deduplication permanent and conflicts with the
time-window logic in check_webhook_dedup(); remove or replace that index with a
predicate that mirrors the same temporal window used by check_webhook_dedup()
(e.g. a partial UNIQUE INDEX on (webhook_id, payload_hash) WHERE created_at >=
now() - INTERVAL '<dedup_window_seconds> seconds' or otherwise match the exact
interval expression used by check_webhook_dedup()), and update the same change
for the other index/constraints referenced in lines 53-80 so the DB constraint
and check_webhook_dedup() have identical temporal semantics (ensure replay_of
semantics remain possible after the dedup window).
In `@supabase/migrations/20260604000004_process_queue_atomic_cleanup.sql`:
- Around line 21-42: The RPC currently selects unread rows from
public.workspace_notifications (WHERE is_read = false LIMIT p_limit) but does
not atomically claim them, so concurrent callers can read the same batch; change
the implementation to atomically reserve the batch before returning: either add
a claimed_at (timestamp) or processing boolean column on workspace_notifications
and perform an UPDATE ... WHERE is_read = false AND (claimed_at IS NULL OR
processing = false) SET claimed_at = NOW() (or processing = true) RETURNING the
updated rows limited to p_limit, then RETURN QUERY from that UPDATE ...
RETURNING result, or wrap a SELECT ... FOR UPDATE SKIP LOCKED on
public.workspace_notifications (WHERE is_read = false) in the same transaction
to lock p_limit rows and then UPDATE them to mark claimed before returning;
ensure references to p_limit, is_read, created_at remain consistent and that old
DELETE logic for created_at < NOW() - INTERVAL '90 days' still only removes
is_read = true rows.
In `@vitest.config.ts`:
- Line 78: The config currently sets coverage.all: false while global coverage
thresholds (lines/functions/branches/statements) are defined, causing thresholds
to apply only to files touched by tests; decide the intended behavior and fix:
if you want global coverage for all src files, set coverage.all: true in
vitest.config.ts (keep include: ['src/**/*.{ts,tsx}']) so the thresholds at
lines ~89-94 apply to the whole codebase, or alternatively keep coverage.all:
false but update CI scripts (the npm run test:coverage / vitest run invocations)
to explicitly override thresholds (e.g., pass --coverage.thresholds.*=0 where
appropriate) and document the intended per-job behavior to avoid inconsistent
gating.
---
Outside diff comments:
In `@src/lib/external-db/product-types.ts`:
- Around line 198-259: As constantes de seleção
(PRODUCT_SELECT_FIELDS_WITH_SALE*, PRODUCT_SELECT_FIELDS_LEGACY*,
PRODUCT_SELECT_FIELDS_DETAIL* e suas _NO_THRESHOLD variantes) não incluem o
campo set_image_url, então queries "full" retornam esse campo como
null/undefined; atualize cada string dessas constantes em
src/lib/external-db/product-types.ts para incluir "set_image_url" na lista de
campos selecionados (preserve a ordem/formatacao existente) para que
product.set_image_url esteja disponível no fluxo full; se por design o campo não
devesse estar no full, em vez disso documente a ausência e adicione tratamento
defensivo no UI onde product.set_image_url é consumido.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 277c155e-b932-4365-9cb9-fbcb49bf758b
📒 Files selected for processing (31)
.github/workflows/freight-quality-gates.ymldocs/AUDIT_REPORT_20260604_FULL.mde2e/auth-debug-removal.spec.tse2e/comprehensive-flow.spec.tse2e/tooltips.spec.tspackage.jsonscripts/lint-untyped-from.shscripts/run-smoke-filtered.mjssrc/components/admin/products/kit-components/api.tssrc/components/products/ProductCardImage.tsxsrc/components/products/ProductListItem.tsxsrc/components/products/ProductStatusBadge.tsxsrc/contexts/ProductsContext.tsxsrc/hooks/products/__tests__/useCatalogFiltering.test.tssrc/lib/external-db/product-types.tssrc/lib/external-db/rest-native.tssrc/pages/auth/StarfieldCanvas.tsxsrc/pages/products/product-detail/ProductDetailHero.tsxsrc/utils/product-mapper.tssupabase/functions/_shared/authorize.tssupabase/functions/_shared/rate-limiter.tssupabase/functions/_shared/security.tssupabase/functions/external-db-bridge/index.tssupabase/functions/image-proxy/index.tssupabase/functions/process-queue/index.tssupabase/migrations/20260604000001_fix_security_definer_search_path.sqlsupabase/migrations/20260604000002_webhook_idempotency.sqlsupabase/migrations/20260604000003_add_compound_indexes_performance.sqlsupabase/migrations/20260604000004_process_queue_atomic_cleanup.sqlvercel.jsonvitest.config.ts
💤 Files with no reviewable changes (1)
- supabase/functions/_shared/authorize.ts
| collection_products | ||
| color_nuances | ||
| personalization_simulations | ||
| print_area_techniques | ||
| product_category_assignments | ||
| product_component_location_techniques | ||
| product_group_components | ||
| product_group_location_techniques | ||
| product_group_locations | ||
| sales_goals | ||
| security_settings | ||
| supplier_branches | ||
| system_kill_switches | ||
| tabela_preco_gravacao_oficial | ||
| tabela_preco_gravacao_oficial_faixa | ||
| tecnicas_gravacao | ||
| user_2fa_settings | ||
| user_ip_allowlist |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verifica se tabelas sensíveis da allowlist existem no schema do Supabase
TYPES_FILE="src/integrations/supabase/types.ts"
echo "=== Verificando tabelas sensíveis na allowlist ==="
for table in security_settings user_2fa_settings user_ip_allowlist; do
if grep -q "^ $table: {$" "$TYPES_FILE"; then
echo "✅ $table existe no types.ts"
else
echo "❌ $table NÃO existe no types.ts"
fi
done
echo ""
echo "=== Verificando se migrations deste PR criam essas tabelas ==="
rg -l 'CREATE TABLE.*(security_settings|user_2fa_settings|user_ip_allowlist)' supabase/migrations/Repository: adm01-debug/promo-gifts-v4
Length of output: 612
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# 1) Confirmar exatamente em quais migrations cada tabela é criada
for f in \
supabase/migrations/20260531110300_create_user_ip_allowlist_table.sql \
supabase/migrations/20260220001443_54c4d527-34ff-47cb-b192-8821dee4b9ae.sql \
supabase/migrations/20251231024837_c924e1c3-b77f-4076-9cdc-195effdf6ea2.sql \
supabase/migrations/20251231130817_8aeff4f3-66df-41e0-a380-c7ffe3c03f96.sql
do
echo "=== $f ==="
rg -n --no-heading "CREATE TABLE[^;]*\b(public\.)?(security_settings|user_2fa_settings|user_ip_allowlist)\b" "$f" || true
rg -n --no-heading "\b(security_settings|user_2fa_settings|user_ip_allowlist)\b" "$f" || true
echo
done
# 2) Entender o objetivo/semântica da allowlist no script
rg -n "ALLOWLIST|untypedFrom|lint-untyped-from\.sh|return \[\]" scripts/lint-untyped-from.sh src || true
sed -n '1,200p' scripts/lint-untyped-from.shRepository: adm01-debug/promo-gifts-v4
Length of output: 50382
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,220p' scripts/lint-untyped-from.sh | nl -ba | sed -n '1,220p'Repository: adm01-debug/promo-gifts-v4
Length of output: 113
🏁 Script executed:
#!/bin/bash
set -euo pipefail
cat -n scripts/lint-untyped-from.sh | sed -n '1,220p'Repository: adm01-debug/promo-gifts-v4
Length of output: 6606
ALLOWLIST de untypedFrom: risco de “tabela inexistente” é baixo, mas falta regenerar types.ts
scripts/lint-untyped-from.shusaALLOWLISTpara permitir que tabelas existam no banco, mas estejam ausentes nosrc/integrations/supabase/types.ts(schema desatualizado).- As tabelas
security_settings,user_2fa_settingseuser_ip_allowlistsão criadas nas migrations do repo (supabase/migrations/20251231130817_8aeff4f3-66df-41e0-a380-c7ffe3c03f96.sql,supabase/migrations/20251231024837_c924e1c3-b77f-4076-9cdc-195effdf6ea2.sql,supabase/migrations/20260531110300_create_user_ip_allowlist_table.sql), então a preocupação de “não existe no banco” fica bem mitigada. - Ainda assim, elas não aparecem no
types.tsatual, então o front continua dependendo do escape hatch; regeneratypes.tse remova da allowlist assim que o schema gerado incluir essas tabelas.
🤖 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 `@scripts/lint-untyped-from.sh` around lines 36 - 53, The ALLOWLIST in
scripts/lint-untyped-from.sh currently silences missing-table warnings for
tables (notably security_settings, user_2fa_settings, user_ip_allowlist) that
actually exist per the repo migrations; fix by regenerating the Supabase schema
types (src/integrations/supabase/types.ts) so those tables are included, then
remove those table names from the ALLOWLIST in scripts/lint-untyped-from.sh;
ensure you run whatever generation command/system is used in this repo to
produce types.ts and verify the three specific tables appear before committing
the removal.
| // Wrap tag in a non-capturing group so alternation (|) stays scoped to the | ||
| // user regex and doesn't break the outer AND with @smoke. | ||
| const escapedTag = `(?:${tag.trim()})`; | ||
| const combined = `(?=.*@smoke)(?=.*${escapedTag})`; | ||
| args.push(`--grep=${combined}`); |
There was a problem hiding this comment.
Escapar tag do usuário antes de injetar na regex.
A tag fornecida pelo usuário é inserida diretamente na regex sem escape. Um input malicioso como --tag ".*)|(.*)|(.*" pode quebrar a estrutura do lookahead e causar matching incorreto ou erro de regex.
🛡️ Solução: escapar caracteres especiais de regex
if (tag && tag.trim()) {
+ // Escape special regex chars to prevent injection
+ const escapeRegex = (str) => str.replace(/[.*+?^${}()|[\]\\]/g, '\\$&');
+ const escapedTag = `(?:${escapeRegex(tag.trim())})`;
- // Wrap tag in a non-capturing group so alternation (|) stays scoped to the
- // user regex and doesn't break the outer AND with `@smoke`.
- const escapedTag = `(?:${tag.trim()})`;
const combined = `(?=.*`@smoke`)(?=.*${escapedTag})`;
args.push(`--grep=${combined}`);Ou, se o objetivo é permitir regex do usuário (como "Catálogo|Busca"), documente claramente que o input deve ser uma regex válida e adicione try/catch ao redor do spawn para capturar erros de regex inválida.
🤖 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 `@scripts/run-smoke-filtered.mjs` around lines 43 - 47, User-provided tag is
injected into the constructed regex (variables escapedTag and combined) without
escaping, allowing input like `.*)|(.*` to break the lookahead; fix by escaping
regex metacharacters in tag before building escapedTag (create and call a helper
like regexEscape(tag) that replaces special chars with backslash-escaped
versions) and then use the escaped value when forming escapedTag/combined and
pushing `--grep`; alternatively, if you intend to accept user regex, document
that and wrap the spawn/regex usage in try/catch to handle invalid regexes
gracefully.
| reporter: ['text', 'html', 'lcov', 'json-summary', 'clover'], | ||
| reportsDirectory: './coverage', | ||
| include: ['src/**/*.{ts,tsx}'], | ||
| all: false, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verifica quantos arquivos são cobertos com all:false vs estrutura de src/
echo "=== Arquivos em src/**/*.{ts,tsx} (excluindo testes/mocks) ==="
fd -e ts -e tsx . src/ \
| grep -vE '(\.test\.|\.spec\.|__tests__|__mocks__|test-utils|main\.tsx|vite-env\.d\.ts)' \
| wc -l
echo ""
echo "=== Scripts CI que desabilitam thresholds ==="
rg --no-heading 'coverage\.thresholds\.\w+=0' package.jsonRepository: adm01-debug/promo-gifts-v4
Length of output: 1959
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== vitest.config.ts: trecho de coverage (linhas ~60-120) ==="
if [ -f vitest.config.ts ]; then
nl -ba vitest.config.ts | sed -n '60,130p'
else
echo "vitest.config.ts não encontrado na raiz"
fi
echo ""
echo "=== package.json: scripts com vitest run ... --coverage (sem thresholds=0) ==="
# Mostra apenas scripts que têm --coverage mas não têm explicitamente thresholds=0
# (aproximação textual)
node - <<'NODE'
const fs = require('fs');
const pkg = JSON.parse(fs.readFileSync('package.json','utf8'));
const scripts = pkg.scripts || {};
for (const [name, cmd] of Object.entries(scripts)) {
if (typeof cmd !== 'string') continue;
if (cmd.includes('vitest') && cmd.includes('--coverage') && !cmd.includes('--coverage.thresholds') ) {
console.log(name + ': ' + cmd);
}
}
NODERepository: adm01-debug/promo-gifts-v4
Length of output: 193
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Localizando vitest.config.ts ==="
cfg="$(fd -a 'vitest.config.ts' . | head -n 1 || true)"
if [ -z "${cfg}" ]; then
echo "Não encontrei vitest.config.ts no repo."
exit 0
fi
echo "Encontrado: $cfg"
echo ""
echo "=== Trecho de coverage no vitest.config.ts ==="
# tenta achar linhas relevantes sem depender de nl
# (cat -n existe; vamos mostrar uma janela ampla e depois filtros)
cat -n "$cfg" | sed -n '1,220p' | rg -n 'coverage|thresholds|all:'
echo ""
echo "=== Linhas onde aparecem coverage.all e thresholds ==="
cat -n "$cfg" | rg -n 'coverage\.all|thresholds|all:\s*(true|false)'Repository: adm01-debug/promo-gifts-v4
Length of output: 449
🏁 Script executed:
#!/bin/bash
set -euo pipefail
cfg="/home/jailuser/git/vitest.config.ts"
echo "=== vitest.config.ts (coverage block) ==="
cat -n "$cfg" | sed -n '65,130p'
echo ""
echo "=== Scripts (package.json) com vitest + --coverage sem override de thresholds ==="
node - <<'NODE'
const fs = require('fs');
const pkg = JSON.parse(fs.readFileSync('package.json','utf8'));
const scripts = pkg.scripts || {};
for (const [name, cmd] of Object.entries(scripts)) {
if (typeof cmd !== 'string') continue;
if (!cmd.includes('vitest')) continue;
if (!cmd.includes('--coverage')) continue;
// se não define thresholds por CLI, ou se define sem usar --coverage.thresholds.
if (!cmd.includes('--coverage.thresholds')) {
console.log(name + ': ' + cmd);
}
}
NODERepository: adm01-debug/promo-gifts-v4
Length of output: 3213
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== package.json: scripts com vitest + --coverage ==="
node - <<'NODE'
const fs = require('fs');
const pkg = JSON.parse(fs.readFileSync('package.json','utf8'));
const scripts = pkg.scripts || {};
for (const [name, cmd] of Object.entries(scripts)) {
if (typeof cmd !== 'string') continue;
if (!cmd.includes('vitest')) continue;
if (!cmd.includes('--coverage')) continue;
const hasThresholds0 = cmd.includes('--coverage.thresholds') && cmd.includes('=0');
const hasInclude = cmd.includes('--coverage.include');
console.log(`${name}`);
console.log(` thresholdsZero=${hasThresholds0}`);
console.log(` hasInclude=${hasInclude}`);
console.log(` cmd=${cmd.replace(/\s+/g,' ').trim()}`);
}
NODE
echo ""
echo "=== .github/workflows: quais scripts de cobertura aparecem? ==="
if [ -d .github ]; then
rg --no-heading -n "test:.*coverage|coverage" .github/workflows || true
else
echo ".github não existe no repo"
fiRepository: adm01-debug/promo-gifts-v4
Length of output: 10501
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== .github/workflows/ci.yml (trecho relevante) ==="
cfg=".github/workflows/ci.yml"
if [ -f "$cfg" ]; then
cat -n "$cfg" | sed -n '1,120p'
else
echo "Arquivo $cfg não encontrado"
fi
echo ""
echo "=== .github/workflows/full-ci.yml (trecho relevante) ==="
cfg=".github/workflows/full-ci.yml"
if [ -f "$cfg" ]; then
cat -n "$cfg" | sed -n '35,95p'
else
echo "Arquivo $cfg não encontrado"
fiRepository: adm01-debug/promo-gifts-v4
Length of output: 5607
Revisar coverage.all: false vs thresholds em vitest.config.ts
vitest.config.ts define coverage.all: false (linha 78) e thresholds globais (lines/functions/branches/statements em 89-94). Com all: false, o Vitest calcula cobertura/thresholds apenas para arquivos efetivamente executados (apesar de include: ['src/**/*.{ts,tsx}']).
Impacto:
- No workflow
ci.yml, o job rodanpm run test:coverage(vitest run --coverage) sem--coverage.thresholds.*=0, então os thresholds do config vão ser aplicados somente ao subset coberto pelos testes, podendo “mascarar” baixa cobertura de arquivos não importados. - Em outros workflows/scripts (ex.
test:ci-core:coverage,test:edge:*:coverage), thresholds são zerados via CLI (--coverage.thresholds.*=0), então o gate real muda conforme o job.
Ajuste recomendado: alinhar a intenção do pipeline (se quer medir cobertura “global” do src ou só “arquivos tocados”) — por exemplo, usar coverage.all: true se a meta for cobertura do codebase inteiro, ou documentar/alinhar thresholds nos scripts/workflows para evitar expectativa inconsistente.
🤖 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 `@vitest.config.ts` at line 78, The config currently sets coverage.all: false
while global coverage thresholds (lines/functions/branches/statements) are
defined, causing thresholds to apply only to files touched by tests; decide the
intended behavior and fix: if you want global coverage for all src files, set
coverage.all: true in vitest.config.ts (keep include: ['src/**/*.{ts,tsx}']) so
the thresholds at lines ~89-94 apply to the whole codebase, or alternatively
keep coverage.all: false but update CI scripts (the npm run test:coverage /
vitest run invocations) to explicitly override thresholds (e.g., pass
--coverage.thresholds.*=0 where appropriate) and document the intended per-job
behavior to avoid inconsistent gating.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
The field exists in the external-db API response (already declared in the lightweight product type) but was missing from PromobrindProduct, causing a TS2551 regression after the mapper was updated to propagate the value. https://claude.ai/code/session_01XnDv6X5E4DDkFzKpvNYcn7
- kit-components/api.ts: replace direct edge function invocations with invokeExternalDb() from bridge.ts; the external-db-bridge edge function is permanently decommissioned (410 stub), breaking all admin kit/print-area operations. REST-native layer already whitelists all three tables. - ProductsContext.tsx: update cacheRef.current synchronously in async callback before setCache(), eliminating the stale-read window that existed between setCache() and the useLayoutEffect running. Retain functional updater to prevent race conditions across concurrent fetch batches. - webhook_idempotency.sql: replace hashtext() with hashtextextended(..., 0) for advisory lock key — 32-bit hash causes avoidable collision contention on high-throughput webhook endpoints. - add_compound_indexes_performance.sql: remove WHERE deleted_at IS NULL from idx_stock_snapshots_variant_supplier; stock_snapshots has no deleted_at column (purged by captured_at), so the partial predicate would fail the migration. - process_queue_atomic_cleanup.sql: remove updated_at reference from DELETE clause; workspace_notifications only has created_at, not updated_at — the condition would fail every RPC call. Retain 90-day created_at cleanup. https://claude.ai/code/session_01XnDv6X5E4DDkFzKpvNYcn7
- rest-native.ts: add kit_component_print_areas and kit_component_media to REST_NATIVE_SAFE_TABLES (read whitelist). Both were in the write whitelist already but missing from reads, so fetchPrintAreas/fetchComponentMedia calls in kit-components/api.ts would silently return empty results with the bridge decommissioned. - add_compound_indexes_performance.sql: fix idx_products_active_price column from `price` to `sale_price`; the products table has no bare `price` column (deprecated base_price, sale_price is the catalog price field), so the old statement would abort the migration. - process_queue_atomic_cleanup.sql: restore is_read=true guard on the DELETE clause. Without it, unread notifications older than 90 days (inactive users, or rows beyond the 500-row SELECT limit) would be purged before the queue ever processes them. - webhook_idempotency.sql: add CTE dedup step before CREATE UNIQUE INDEX. If the table already contains two successful deliveries with matching (webhook_id, payload_hash), Postgres aborts the unique index creation, blocking the migration entirely. https://claude.ai/code/session_01XnDv6X5E4DDkFzKpvNYcn7
cubic P1: DELETE and CREATE UNIQUE INDEX were not protected against concurrent webhook inserts. Add LOCK TABLE IN SHARE ROW EXCLUSIVE MODE before the cleanup; the lock holds for the entire migration transaction, blocking concurrent INSERTs until the unique index is active. cubic P2: ORDER BY id on a UUID column is arbitrary (UUIDs are random, not time-ordered). Switch to ORDER BY ctid which reflects physical insertion order, preserving the oldest canonical delivery when deduping. https://claude.ai/code/session_01XnDv6X5E4DDkFzKpvNYcn7
…it_component_media kit_component_print_areas was missing a DELETE policy for admins (INSERT/UPDATE already existed). kit_component_media had no write policies at all. Both tables were previously managed via service_role through external-db-bridge; after the REST-native migration, admin writes through the user JWT were silently blocked by RLS. https://claude.ai/code/session_01XnDv6X5E4DDkFzKpvNYcn7
- migrations/000001: REVOKE EXECUTE FROM PUBLIC/anon/authenticated + GRANT TO service_role for all 3 SECURITY DEFINER functions (soft_delete_record, restore_record, permanent_delete_record) - migrations/000002: drop UNIQUE from idx_webhook_deliveries_idempotency so time-windowed check_webhook_dedup() handles dedup; UNIQUE would block legitimate replay after the dedup window expires - migrations/000004: replace SELECT with UPDATE … FROM (SELECT … FOR UPDATE SKIP LOCKED) … RETURNING so concurrent edge-function callers get disjoint notification batches (prevents duplicate dispatch) - image-proxy: remove wildcard *.lovable.app / *.lovableproject.com patterns; specific Lovable projects go in ALLOWED_REFERER_EXACT (wildcard allowed any tenant to hotlink) - run-smoke-filtered.mjs: document intentional regex-in-tag design; non-user CLI so injection surface is negligible - docs/AUDIT_REPORT: fix P0 heading wording + add supabase/ path prefix to all migration references in the corrections table https://claude.ai/code/session_01XnDv6X5E4DDkFzKpvNYcn7
…otifications_queue cubic P1 (#641): setting is_read=true during atomic claim conflated queue processing ownership with the user-facing read indicator. A failed dispatch (push error, edge-function crash) would silently lose the notification — is_read=true prevented re-queuing but the user never received it. Fix: add dispatched_at TIMESTAMPTZ column to workspace_notifications. - Queue predicate: dispatched_at IS NULL (not yet processed by any worker) - Atomic claim: SET dispatched_at = NOW() (not is_read = true) - is_read remains exclusively user-facing, updated by UI on dismiss/read - Backfill: existing is_read=true rows get dispatched_at=NOW() so they are not accidentally re-queued after migration - Cleanup: deletes WHERE dispatched_at IS NOT NULL (never discards undispatched) - Index idx_workspace_notifications_undispatched covers new queue predicate https://claude.ai/code/session_01XnDv6X5E4DDkFzKpvNYcn7
adm01-debug
force-pushed
the
claude/awesome-thompson-n9ueF
branch
from
137cc14 to
3b5b679
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3b5b679903
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| FOR UPDATE SKIP LOCKED | ||
| ) | ||
| UPDATE public.workspace_notifications wn | ||
| SET dispatched_at = NOW() |
There was a problem hiding this comment.
In the current process-queue flow, the RPC is called before the edge function has finished even grouping/returning the batch, and there is no later code that clears or retries dispatched_at (repo-wide search only finds this migration). If the edge function times out/crashes after this update, or if a future delivery step fails after the claim, these rows no longer match dispatched_at IS NULL and will never be picked up again, preserving the notification-loss mode this migration says it fixes. Use a claim/lease state that can expire, or mark dispatched_at only after successful dispatch.
Useful? React with 👍 / 👎.
| CREATE POLICY "kcpa_admin_delete" | ||
| ON public.kit_component_print_areas FOR DELETE | ||
| TO authenticated | ||
| USING (is_admin_or_above((SELECT auth.uid()))); |
There was a problem hiding this comment.
Fresh evidence in this revision is that the new migration adds only RLS policies, but no GRANT INSERT/UPDATE/DELETE for kit_component_print_areas or kit_component_media; repo-wide migration search shows only broad GRANT SELECT ON ALL TABLES plus no DML grants for these tables. Because the admin helpers now write via client-side PostgREST as authenticated, the role still lacks table privileges, so deletes/media inserts/updates/deletes will fail with permission errors even though the RLS policy would allow admins.
Useful? React with 👍 / 👎.
| await invokeExternalDb({ | ||
| table: 'product_kit_components', | ||
| operation: 'insert', | ||
| data: payload, |
There was a problem hiding this comment.
When an admin creates, edits, or deletes a kit component through these newly REST-native helpers, PostgREST runs as the authenticated role. I checked the migrations for product_kit_components: they define an admin FOR ALL RLS policy but only GRANT SELECT ON public.product_kit_components TO authenticated, with no INSERT/UPDATE/DELETE grant, so these writes will fail at the table-privilege layer before the RLS policy can allow them.
Useful? React with 👍 / 👎.
| 'criar-together-now.lovable.app', | ||
| 'promogifts.com.br', | ||
| 'www.promogifts.com.br', | ||
| 'promo-gifts-beta.vercel.app', // Staging/beta Vercel deployment | ||
| 'lovable.app', // any *.lovable.app subdomain | ||
| 'lovableproject.com', | ||
| 'vercel.app', // Generic Vercel preview deployments | ||
| ]; | ||
| 'promogifts.atomicabr.com.br', | ||
| 'promo-gifts-beta.vercel.app', |
There was a problem hiding this comment.
This tightened allowlist no longer includes the known Lovable preview hosts that are still first-party origins in _shared/cors.ts (id-preview--1be35a65-1f65-4c2b-9a79-7d563930aacd.lovable.app and 1be35a65-1f65-4c2b-9a79-7d563930aacd.lovableproject.com). In those preview environments, catalog images loaded through image-proxy now carry that Referer/Origin and receive 403 Hotlinking not allowed, so the preview app loses proxied supplier images unless those exact hosts are added.
Useful? React with 👍 / 👎.
…e-proxy origins Codex P1 (migration 000006 line 57): dispatched_at was set during claim before the edge function confirmed delivery. A crash after claim would permanently lose the notification. Fix: lease-based two-phase claim/confirm via new migration 000008. - process_notifications_queue() now sets dispatch_claim_expires_at (+15 min) only. - New confirm_notifications_dispatched(UUID[]) sets dispatched_at after delivery. - process-queue edge function calls confirm after grouping; expired leases re-queue. Codex P2 (migration 000005 line 25, api.ts line 23): RLS policies were added for kit_component_print_areas, kit_component_media, and product_kit_components, but PostgREST also requires table-level GRANT for DML. New migration 000007 adds GRANT INSERT, UPDATE, DELETE on all three tables to authenticated. Codex P2 (image-proxy line 32): ALLOWED_REFERER_EXACT was missing the two first-party Lovable preview hosts that are in _shared/cors.ts EXACT_ALLOWED_ORIGINS (id-preview--1be35a65 and 1be35a65 lovableproject.com). These previews load catalog images through image-proxy and would receive 403 without these entries. https://claude.ai/code/session_01XnDv6X5E4DDkFzKpvNYcn7
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
There was a problem hiding this comment.
2 issues found across 4 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
…_notifications_dispatched - Drop duplicate partial index in 000008 (identical definition already exists as idx_workspace_notifications_undispatched from 000006) - Add lease guard (dispatch_claim_expires_at IS NOT NULL AND > NOW()) to confirm_notifications_dispatched so only actively-claimed rows can be permanently marked dispatched; prevents accidentally marking unclaimed notifications as delivered https://claude.ai/code/session_01XnDv6X5E4DDkFzKpvNYcn7
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d1b7c56d2b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const { error: confirmError } = await supabase.rpc('confirm_notifications_dispatched', { | ||
| p_ids: notifIds, | ||
| }); |
There was a problem hiding this comment.
Fresh evidence in this revision is that the new lease/confirm flow is wired here, but process-queue only groups the rows and then immediately calls confirm_notifications_dispatched; it has not sent a digest, push, email, or any other delivery before this point. In runs where actual delivery happens later or fails elsewhere, these IDs get dispatched_at set and the lease is cleared, so the queue will not retry them; move this confirmation after the real delivery step or leave the lease to expire on failure.
Useful? React with 👍 / 👎.
| p_webhook_id UUID, | ||
| p_payload_hash TEXT, | ||
| p_dedup_window_seconds INT DEFAULT 300 | ||
| ) RETURNS BOOLEAN AS $$ |
There was a problem hiding this comment.
Fresh evidence in this revision is that the unique constraint was replaced with check_webhook_dedup, but repo-wide search finds no caller outside this migration and webhook-dispatcher still performs the customer fetch before inserting webhook_deliveries. For duplicate or concurrent payloads, this function is never invoked before the external POST, so the new idempotency migration does not prevent duplicate deliveries; the dispatcher needs to claim/check this RPC before sending.
Useful? React with 👍 / 👎.
| SELECT 1 FROM information_schema.tables | ||
| WHERE table_schema = 'public' AND table_name = 'kit_component_print_areas' | ||
| ) THEN | ||
| GRANT INSERT, UPDATE, DELETE ON public.kit_component_print_areas TO authenticated; |
There was a problem hiding this comment.
This grant, and the matching kit_component_media grant below, assumes RLS will constrain writes, but a repo-wide migration search shows policies for these externally-created tables and no ALTER TABLE ... ENABLE ROW LEVEL SECURITY for either table. On schemas where these tables are not already RLS-enabled, granting INSERT/UPDATE/DELETE to authenticated lets any logged-in user mutate print areas/media because the admin policies are ignored until RLS is enabled; add guarded ALTER TABLE ... ENABLE ROW LEVEL SECURITY before the grants.
Useful? React with 👍 / 👎.
| CREATE OR REPLACE FUNCTION public.confirm_notifications_dispatched( | ||
| p_ids UUID[] | ||
| ) | ||
| RETURNS INT AS $$ |
There was a problem hiding this comment.
This new SECURITY DEFINER function is called by process-queue, but this migration only grants process_notifications_queue to service_role and never grants confirm_notifications_dispatched; in this repo default function execute privileges are revoked, so the service-role RPC call fails with permission denied, logs the error, and leaves every claimed row to be reprocessed after the lease expires. Add the same REVOKE/GRANT EXECUTE ... TO service_role block for this function.
Useful? React with 👍 / 👎.
| await invokeExternalDb({ | ||
| table: 'product_kit_components', | ||
| operation: 'insert', |
There was a problem hiding this comment.
When these component mutations now go through client-side PostgREST, global catalog kits whose parent products.organization_id is NULL are rejected by the existing product_kit_components manage policy, because it checks is_org_owner_or_admin(organization_id) and that function only matches rows in user_organizations. Those same global products are explicitly readable when organization_id IS NULL, and the previous service-role bridge could edit them, so admins will be unable to create/update/delete kit components for global catalog products unless a global-admin policy is added or this remains backend-mediated.
Useful? React with 👍 / 👎.
…bhook dedup check - migration 000007: add ENABLE ROW LEVEL SECURITY for kit_component_print_areas and kit_component_media before granting DML to authenticated. These tables were created outside migrations; without RLS enabled, admin-only write policies are ignored and any authenticated user can mutate rows. - webhook-dispatcher: call check_webhook_dedup() before the external POST for each hook. Skips delivery when an identical payload (same webhook_id + hash) was successfully delivered within the 5-minute idempotency window, preventing duplicate charges/orders on retry storms. https://claude.ai/code/session_01XnDv6X5E4DDkFzKpvNYcn7
|
You're iterating quickly on this pull request. To help protect your rate limits, cubic has paused automatic reviews on new pushes for now—when you're ready for another review, comment |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9b6a3034c5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const { data: canProceed, error: dedupErr } = await supabase.rpc('check_webhook_dedup', { | ||
| p_webhook_id: hook.id, | ||
| p_payload_hash: phash, | ||
| }); |
There was a problem hiding this comment.
Fresh evidence in this revision is that the dedupe RPC is now called before the POST, but it still only returns a boolean: the pg_advisory_xact_lock inside check_webhook_dedup is transaction-scoped and is released as soon as this RPC call returns, before any delivery/claim row is inserted. If two dispatcher invocations for the same webhook/payload overlap, both can see zero recent successful deliveries and both proceed to the external fetch, so duplicate customer POSTs are still possible; the check needs to reserve/insert a pending idempotency claim before returning or otherwise keep the check and claim atomic.
Useful? React with 👍 / 👎.
| image_url?: string | null; | ||
| images: string[] | null; | ||
| primary_image_url: string | null; | ||
| set_image_url?: string | null; |
There was a problem hiding this comment.
Fresh evidence in this revision is that the mapper/type now preserve set_image_url, but the full product select constants in this same module still request only images, primary_image_url and never include set_image_url. Any product loaded through fetchPromobrindProducts/ProductsContext therefore reaches mapPromobrindToProduct without this field and is still mapped to null, so those card/detail paths continue losing the grouped-color hover image unless the field is added to the SELECT lists (with the same fallback handling as other transitional columns if needed).
Useful? React with 👍 / 👎.
…sts (P2) P1 — atomic delivery claim via locks table: check_webhook_dedup acquired pg_advisory_xact_lock but the lock was transaction-scoped and released when the RPC returned, before the edge function inserted a delivery row. Two concurrent dispatchers for the same webhook+payload could both pass the check and both POST externally (double-charge, double-order). Fix: introduce webhook_delivery_locks table with PRIMARY KEY on (webhook_id, payload_hash). claim_webhook_delivery() does INSERT ... ON CONFLICT DO NOTHING — the unique constraint ensures only one concurrent invocation acquires the lock; the other gets 0 rows and skips. Stale locks (> 15 min) are auto-expired to handle crashes. release_webhook_delivery_lock() is called after the full retry cycle so failed deliveries remain retryable. P2 — add set_image_url to product SELECT constants: The field was preserved in PromobrindProduct and mapPromobrindToProduct but missing from all six PRODUCT_SELECT_FIELDS_* constants, so any product loaded via fetchPromobrindProducts/ProductsContext arrived without this field and the grouped-color hover image rendered as null. https://claude.ai/code/session_01XnDv6X5E4DDkFzKpvNYcn7
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8b1cfc353d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (claimErr) { | ||
| console.warn('[webhook-dispatcher] claim_webhook_delivery error (continuing):', claimErr); |
There was a problem hiding this comment.
When claim_webhook_delivery fails (for example because the RPC/grant is missing during rollout or Supabase returns a transient error), this branch only logs and then continues to the external fetch. In that scenario no lock or recent-success check has run, so concurrent duplicate payloads can still both be POSTed to customer endpoints despite the idempotency migration; the dispatcher should fail/skip or retry the claim instead of sending without a claim.
Useful? React with 👍 / 👎.
| DELETE FROM public.workspace_notifications | ||
| WHERE dispatched_at IS NOT NULL | ||
| AND created_at < NOW() - INTERVAL '90 days'; |
There was a problem hiding this comment.
Fresh evidence is migration 0008's dispatch/read split: is_read now remains user-facing state, while dispatched_at only means the queue confirmed dispatch. With this predicate, an unread notification that was dispatched but not opened for more than 90 days is deleted from the notification center even though is_read is still false; add AND is_read = true (or another user-read condition) if the cleanup is only meant to purge notifications safe to discard.
Useful? React with 👍 / 👎.
| if (phash && !claimErr) { | ||
| const { error: releaseErr } = await supabase.rpc('release_webhook_delivery_lock', { | ||
| p_webhook_id: hook.id, | ||
| p_payload_hash: phash, | ||
| }); |
There was a problem hiding this comment.
When the customer POST returns 2xx but the webhook_deliveries insert above fails (transient PostgREST/database error, schema mismatch, etc.), the insert error is ignored, success is still set, and this new release removes the only claim row. Because no successful delivery row exists for claim_webhook_delivery to find on the next invocation, the same payload can be claimed and POSTed again; check/throw on the delivery insert or avoid releasing the claim until the success record is durable.
Useful? React with 👍 / 👎.
…uard purge for unread notifications Codex P1 (index.ts): capture webhook_deliveries INSERT error and track deliveryRowInserted. When delivery succeeds (2xx) but the dedup row was not written, log a critical error and keep the lock — releasing without a durable row would leave no dedup evidence, allowing immediate re-dispatch of the same payload. On failure the lock is always released so the next retry cycle can re-claim. Codex P2 (migration 000008): add AND is_read = true to the 90-day purge DELETE, consistent with migration 000004. Without this guard, dispatched-but-unread notifications (e.g. inactive users) are silently discarded before the user ever sees them. https://claude.ai/code/session_01XnDv6X5E4DDkFzKpvNYcn7
# Conflicts: # src/components/admin/products/kit-components/api.ts
Summary
test.describesem fechamento (SyntaxError na linha 94 = EOF inesperado); adicionado});finaltest.use({ ...devices['iPhone 13'] })dentro dedescribeaninhado falhava porque Playwright proíbe sobrescreverdefaultBrowserTypenesse contexto; strippeddefaultBrowserTypedo spread"../fixtures/..."e"../helpers/..."(um nível acima dee2e/); corrigido para"./fixtures/..."e"./helpers/..."— os módulos existem mas no caminho erradohttps://*.vercel.appdescript-srceconnect-src— o wildcard permitia que qualquer deployment Vercel injetasse scripts ou recebesse dados;'self'já cobre os previews do próprio projetoendsWith('vercel.app')(qualquer*.vercel.app) por regex específica*-juca1.vercel.app(team-scoped) + conjuntos exatos para Lovable; impede hotlinking arbitrário via deployments de terceirosindex.tsimportava 6 módulos_sharedque não existem mais (external-db-config,external-db-aliases,external-db-telemetry,external-db-cache,json-response, API antiga decors.ts); substituído por stub mínimo que retorna 410 Gone (função já está decommissionada via kill-switch permanente)cacheRef.currentsó era atualizado pelouseEffect, criando janela entresetCache()e o próximo render onde IDs recém-buscados podiam ser re-enfileirados porqueueFetch; sincronizado dentro do updater dosetCacheTest plan
tooltips,auth-debug-removal,comprehensive-flownão devem mais falhar por SyntaxError / module-not-found*.vercel.appdescript-src*-juca1.vercel.app) ainda funcionam e que origem externa (random.vercel.app) recebe 403https://claude.ai/code/session_01XnDv6X5E4DDkFzKpvNYcn7
Generated by Claude Code
Summary by cubic
Fixes E2E failures and hardens CSP/CORS/SSRF/auth. Fully decommissions
external-db-bridge, switches admin kit APIs to REST‑native, closes a ProductsContext race, adds atomic webhook dedup with safe claim/release, stabilizes CI coverage, and makes the notifications queue retry‑safe with a lease.Bug Fixes
});, corrected iPhone 13test.use()(stripdefaultBrowserType), and fixed relative imports; smoke runner now ANDs@smokewith an optional tag using a non‑capturing group.https://*.vercel.appfrom CSP;image-proxynow allows only exact referers plus team‑scoped*-juca1.vercel.appand specific Lovable preview hosts; CSRF HMAC now requiresCSRF_SECRETanduserId; rate limiter runs fail‑closed on approval endpoints.external-db-bridgewith a 410 Gone stub; admin kit/print‑area/media APIs now call REST‑native viainvokeExternalDb; read whitelist includeskit_component_print_areasandkit_component_media.cacheRefin async flow and viauseLayoutEffect; exposedfetchError; preservedset_image_urlin mapper and added it to all product SELECT constants;ProductStatusBadgenow dispatches a real click on Enter/Space.claim_webhook_delivery(); releases the lock only after the delivery row is durably written; skips duplicates within the idempotency window. Notifications queue uses a two‑phase lease with post‑dispatch confirm. Coverage gates pin include paths and setall: falseto avoid false failures.Migration
SET search_path='public', switched toGET DIAGNOSTICS ROW_COUNT, and revoked EXECUTE from PUBLIC/anon/authenticated (granted toservice_role).idempotency_key; strengthenedcheck_webhook_dedup()with advisory locks; introducedwebhook_delivery_lockswithclaim_webhook_delivery()/release_webhook_delivery_lock()for atomic claims; table locked during dedup cleanup; non‑unique index allows windowed replays.dispatched_at; added a lease (dispatch_claim_expires_at) andconfirm_notifications_dispatched(UUID[]); backfill and indexes added; cleanup now purges only dispatched and read rows.authenticatedforkit_component_print_areas,kit_component_media, andproduct_kit_componentsto enable PostgREST DML with JWT.Written for commit a16cc2e. Summary will update on new commits.
Summary by CodeRabbit
Notas de Lançamento
Documentação
Correções de Bugs
Segurança & Performance
external-db-bridgedescontinuado (retorna HTTP 410).Chores