fix(contracts): complete P1 parseContract coverage#314
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughMigra contratos P1 v1/v2 e testes; reforça tipagem e consultas Supabase; refatora bridge/invoke; torna migrations replay-safe; adiciona HMAC/retry/readiness no runner; reorganiza imports e lazy-loading no frontend. ChangesMigração P1: Contratos v1/v2, Tipagem e Hardening SQL
Estimated code review effort 🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
|
Updates to Preview Branch (fix/contracts-p1-parse-contract) ↗︎
Tasks are run on every commit but only new migration files are pushed.
View logs for this Workflow Run ↗︎. |
5 tasks
Contributor
There was a problem hiding this comment.
Pull request overview
Completa o lote P1 da migração para parseContract, fechando critérios de aceite do issue #47 com cobertura de testes para os 5 endpoints e hardening adicional no fluxo de external-db bridge (telemetria/kill-switch).
Changes:
- Expande
tests/contracts/migrated-endpoints.contract.test.tspara cobrir v1/v2, strict mode, versões inválidas (406), headersDeprecation/Sunsete campos obrigatórios dos 5 endpoints P1. - Endurece regras de
sync-external-dbv2 exigindoidempotency_keye documenta notas P1 v2 no README de contratos. - Ajusta tipagens e telemetria do bridge/kill-switch (novas operações, casts tipados, assinatura de
emitBridgeStatus, mocks mais estáveis em testes).
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| tests/contracts/migrated-endpoints.contract.test.ts | Aumenta a cobertura de contract tests P1 (v1/v2, strict, unsupported_version, Deprecation/Sunset, required fields). |
| supabase/functions/_shared/contracts/schemas/sync-external-db.ts | Enforce de idempotency_key no schema v2 (strict) e atualização do comentário do contrato. |
| src/lib/telemetry/bridgeCallMetrics.ts | Amplia BridgeOperation para incluir 'invoke'. |
| src/lib/external-db/kill-switch-telemetry.ts | Remove any via client minimalista tipado e renomeia reset para uso em testes. |
| src/lib/external-db/kill-switch-client.ts | Substitui casts any por interfaces tipadas (cadeia from/select/eq/maybeSingle). |
| src/lib/external-db/invoke.ts | Tipagem mais forte de operação do bridge com allowlist e import type-only de BridgeOperation. |
| src/lib/external-db/bridge-status-events.ts | Refina o tipo de input de emitBridgeStatus para permitir ts opcional por variante. |
| src/lib/external-db/tests/kill-switch-client.test.ts | Ajusta mock from() para assinatura estável/compatível com tipagem. |
| src/hooks/useKillSwitchBanner.ts | Usa import type-only de KillSwitchActiveError. |
| docs/contracts/README.md | Documenta regras/observações P1 v2 (idempotency_key e strict). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
supabase/migrations/20260515030000_onda19_numeric_precision.sql (1)
48-71: 💤 Low valueConsiderar USING clause para narrowing de precisão numérica.
Os
ALTER COLUMN ... TYPE numeric(n,m)podem falhar silenciosamente ou com erro se dados existentes excederem a nova precisão. Por exemplo,numeric(5,2)permite no máximo 999.99 (3 dígitos inteiros).O comentário indica "~0 linhas" e "zero drift detectado", então o risco é baixo neste caso. Para migrações futuras com dados existentes, considerar:
ALTER COLUMN markup_percent TYPE numeric(5,2) USING ROUND(markup_percent, 2)::numeric(5,2);Isso garante truncamento explícito ao invés de falha.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@supabase/migrations/20260515030000_onda19_numeric_precision.sql` around lines 48 - 71, The ALTER COLUMN numeric precision changes can fail if existing values exceed the new precision; update each ALTER TABLE ... ALTER COLUMN ... TYPE to include a USING clause that explicitly rounds/casts the existing column to the target precision (e.g., use ROUND(column, 2)::numeric(5,2) for columns moving to numeric(5,2) and ROUND(column, 2)::numeric(3,2) for confidence), applying this to quotes.discount_percent, quotes.negotiation_markup_percent, quotes.real_discount_percent, seller_discount_limits.max_discount_percent, seller_discount_limits.approval_required_above, tabela_preco_gravacao_oficial.markup_percent, supplier_technique_mappings.confidence, and variant_supplier_sources.supplier_ipi_rate so existing values are deterministically rounded/cast instead of causing errors.docs/contracts/README.md (1)
122-128: ⚡ Quick winClarificar se
trends-insightsexige ou nãoidempotency_key.A nota menciona que
trends-insightsusa v2 strict, mas não esclarece se exigeidempotency_key(diferente deownership-auditque é explícito).Se
trends-insightsé operação de leitura/análise sem side-effects (comoownership-audit), adicione na lista. Se consome recursos de IA (tokens, cache) e tem side-effects, deveria estar no primeiro grupo junto comsync-external-db.Sugestão para deixar mais claro:
📝 Proposta de clarificação
Notas P1 v2: - `ownership-repair`, `simulation-orchestrator` e `sync-external-db` exigem `idempotency_key` por executarem operacoes com side-effect. -- `ownership-audit` permanece sem `idempotency_key` porque e leitura/auditoria. +- `ownership-audit` e `trends-insights` permanecem sem `idempotency_key` porque são operações de leitura/auditoria. -- `trends-insights` usa v2 strict para limitar o payload aceito pelo fluxo de IA. +(OU, se trends-insights tiver side-effects:) +- `ownership-repair`, `simulation-orchestrator`, `sync-external-db` e `trends-insights` exigem + `idempotency_key` por executarem operações com side-effect (sync, orquestração, consumo de tokens IA). +- `ownership-audit` permanece sem `idempotency_key` porque é leitura/auditoria.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/contracts/README.md` around lines 122 - 128, Clarify whether the "trends-insights" contract requires an idempotency_key: determine if trends-insights is read-only/analysis (like ownership-audit) or performs side-effects/consumes AI resources (like ownership-repair, sync-external-db); then update the README note near the bullets to explicitly state "trends-insights requires idempotency_key" or "trends-insights does not require idempotency_key (read-only)", and if keeping the "v2 strict" remark, append whether v2 strict enforces or omits idempotency_key for trends-insights so the intent is unambiguous.
🤖 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.
Nitpick comments:
In `@docs/contracts/README.md`:
- Around line 122-128: Clarify whether the "trends-insights" contract requires
an idempotency_key: determine if trends-insights is read-only/analysis (like
ownership-audit) or performs side-effects/consumes AI resources (like
ownership-repair, sync-external-db); then update the README note near the
bullets to explicitly state "trends-insights requires idempotency_key" or
"trends-insights does not require idempotency_key (read-only)", and if keeping
the "v2 strict" remark, append whether v2 strict enforces or omits
idempotency_key for trends-insights so the intent is unambiguous.
In `@supabase/migrations/20260515030000_onda19_numeric_precision.sql`:
- Around line 48-71: The ALTER COLUMN numeric precision changes can fail if
existing values exceed the new precision; update each ALTER TABLE ... ALTER
COLUMN ... TYPE to include a USING clause that explicitly rounds/casts the
existing column to the target precision (e.g., use ROUND(column,
2)::numeric(5,2) for columns moving to numeric(5,2) and ROUND(column,
2)::numeric(3,2) for confidence), applying this to quotes.discount_percent,
quotes.negotiation_markup_percent, quotes.real_discount_percent,
seller_discount_limits.max_discount_percent,
seller_discount_limits.approval_required_above,
tabela_preco_gravacao_oficial.markup_percent,
supplier_technique_mappings.confidence, and
variant_supplier_sources.supplier_ipi_rate so existing values are
deterministically rounded/cast instead of causing errors.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c549097e-d60c-435a-96bf-e792a13a45aa
📒 Files selected for processing (15)
.github/workflows/deploy-gates.ymldocs/contracts/README.mdsrc/hooks/useKillSwitchBanner.tssrc/lib/external-db/__tests__/kill-switch-client.test.tssrc/lib/external-db/bridge-status-events.tssrc/lib/external-db/invoke.tssrc/lib/external-db/kill-switch-client.tssrc/lib/external-db/kill-switch-telemetry.tssrc/lib/telemetry/bridgeCallMetrics.tssupabase/functions/_shared/contracts/schemas/sync-external-db.tssupabase/migrations/20260514220543_onda13_rls_audit_logs_admin_only.sqlsupabase/migrations/20260515030000_onda19_numeric_precision.sqlsupabase/migrations/20260524210100_harden_anon_graphql_exposure.sqltests/contracts/migrated-endpoints.contract.test.tstests/search/GlobalSearchHelpers.test.tsx
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/components/providers/AppBootstrap.tsx (2)
21-23:⚠️ Potential issue | 🟠 Major | ⚡ Quick winComparação de
maintenance_modeaceita só string e pode falhar com boolean.Em Line 21,
data.value === 'true'é frágil para colunajsonb; se viertrue(boolean), manutenção nunca ativa.🔧 Ajuste sugerido
- if (data && data.value === 'true') { + const isMaintenance = data?.value === true || data?.value === 'true'; + if (isMaintenance) { setMaintenanceMode(true); }🤖 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/components/providers/AppBootstrap.tsx` around lines 21 - 23, No trecho em AppBootstrap.tsx onde você chama setMaintenanceMode, a comparação atual usa data.value === 'true' e falhará se data.value vier como booleano true; altere a verificação para aceitar tanto o booleano true quanto a string "true" (ou normalize/parse data.value primeiro) antes de invocar setMaintenanceMode(true), referenciando a variável data.value e a função setMaintenanceMode para localizar o lugar a corrigir.
15-26:⚠️ Potential issue | 🟠 Major | ⚡ Quick winCorrigir leitura do
maintenance_mode: tratarerrordo Supabase e interpretarvaluecomoJson(boolean/string)
maybeSingle()entrega o erro em{ error }(semthrowOnErrorno client), então só ocatchnão impede falha silenciosa quando a query falha.system_settings.valueéJsone o seed/migration demaintenance_modeusa tantofalse(boolean) quanto'false'(string);data.value === 'true'falha quando o tipo vier boolean.🔧 Ajuste sugerido
- const { data } = await supabase + const { data, error } = await supabase .from('system_settings') .select('value') .eq('key', 'maintenance_mode') .maybeSingle(); + if (error) throw error;Sugestão de parse robusto antes do
setMaintenanceMode(true):const raw = data?.value; const enabled = raw === true || (typeof raw === "string" && raw.toLowerCase() === "true"); if (enabled) setMaintenanceMode(true);🤖 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/components/providers/AppBootstrap.tsx` around lines 15 - 26, The maintenance_mode read currently relies on try/catch and compares data.value to the string "true", which misses Supabase query errors returned as an { error } field from maybeSingle() and fails when value is stored as a boolean; update the supabase query handling (the block around supabase.from('system_settings').select(...).maybeSingle()) to check the returned error object and handle it (log/early return) instead of relying solely on catch, then normalize/parse data.value from Json before calling setMaintenanceMode (use a robust boolean/string check for raw === true or string "true" case-insensitively) so setMaintenanceMode(true) is only called when enabled..github/workflows/deploy-gates.yml (1)
50-50:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGate de testes pode passar sem executar teste.
Em Line 50,
npm test --if-presentpermite sucesso mesmo sem scripttest, enfraquecendo o gate de qualidade. Para gate de deploy, o ideal é falhar se o script não existir.🔧 Sugestão objetiva
- - run: npm test --if-present + - run: npm test🤖 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 @.github/workflows/deploy-gates.yml at line 50, The workflow uses the command string "npm test --if-present" which allows the job to succeed when no test script exists; change that step to run "npm test" (remove the --if-present flag) so the job fails if the test script is missing, ensuring the deploy gate enforces tests; update the workflow step that currently contains the "npm test --if-present" command accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In @.github/workflows/deploy-gates.yml:
- Line 50: The workflow uses the command string "npm test --if-present" which
allows the job to succeed when no test script exists; change that step to run
"npm test" (remove the --if-present flag) so the job fails if the test script is
missing, ensuring the deploy gate enforces tests; update the workflow step that
currently contains the "npm test --if-present" command accordingly.
In `@src/components/providers/AppBootstrap.tsx`:
- Around line 21-23: No trecho em AppBootstrap.tsx onde você chama
setMaintenanceMode, a comparação atual usa data.value === 'true' e falhará se
data.value vier como booleano true; altere a verificação para aceitar tanto o
booleano true quanto a string "true" (ou normalize/parse data.value primeiro)
antes de invocar setMaintenanceMode(true), referenciando a variável data.value e
a função setMaintenanceMode para localizar o lugar a corrigir.
- Around line 15-26: The maintenance_mode read currently relies on try/catch and
compares data.value to the string "true", which misses Supabase query errors
returned as an { error } field from maybeSingle() and fails when value is stored
as a boolean; update the supabase query handling (the block around
supabase.from('system_settings').select(...).maybeSingle()) to check the
returned error object and handle it (log/early return) instead of relying solely
on catch, then normalize/parse data.value from Json before calling
setMaintenanceMode (use a robust boolean/string check for raw === true or string
"true" case-insensitively) so setMaintenanceMode(true) is only called when
enabled.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 42b9082a-96c4-4d86-a68f-a1f23e723390
📒 Files selected for processing (4)
.github/workflows/deploy-gates.ymlsrc/components/products/ProductSparkline.tsxsrc/components/providers/AppBootstrap.tsxtests/components/products/ProductSparkline.labels.test.tsx
💤 Files with no reviewable changes (1)
- src/components/products/ProductSparkline.tsx
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Contexto
Fecha a issue #47 consolidando o lote P1 de contratos
parseContract. A migracao basica ja estava nomain, mas faltava fechar criterios de aceite com cobertura suficiente e um hardening de v2 parasync-external-db.Closes #47
Mudancas
tests/contracts/migrated-endpoints.contract.test.tspara os 5 endpoints P1Deprecation/Sunsete campos obrigatoriosidempotency_keyemsync-external-dbv2 por ser sincronizacao com side effectdocs/contracts/README.mdemitBridgeStatusValidacao
npx.cmd vitest run tests/contracts/migrated-endpoints.contract.test.ts --reporter=dot: 60 passed$env:TZ='America/Sao_Paulo'; npx.cmd vitest run tests/contracts/ --reporter=dot: 456 passed$env:TZ='America/Sao_Paulo'; npx.cmd vitest run tests/contracts/ src/lib/external-db/__tests__/kill-switch-client.test.ts --reporter=dot: 464 passednpm.cmd run typecheck: passed, 501 erros atuais vs baseline 508, drift positivo 7npm.cmd run lint:baseline: passed, 128 erros / 221 warnings vs baseline 128git diff --check: passedObservacao:
npm.cmd run test -- tests/contracts/falha no Windows porque o script usa sintaxe POSIXTZ=...; por isso rodei o equivalente com$env:TZ.Summary by cubic
Completes P1
parseContractv2 across five endpoints with strict parsing, version/deprecation headers, 406 on unsupported versions, and idempotency for side‑effect flows. Also trims public boot with lazy layouts/MFA and a lazysupabaseclient, adds Lighthouse placeholder guards, stabilizes the Elite UX E2E flow, and scopes CI to faster core gates.Bug Fixes
product-webhooknow uses shared CORS with explicit allow headers/methods and proper OPTIONS; contract smoke calls are HMAC‑signed with a dedicated user agent and retry transient 50x/timeout upstreams.CI
test:ci-core,test:ci-core:coverage, andtest:deploy-gate; coverage job timeout reduced to 15m.product-webhookreadiness.Written for commit d2c9fc3. Summary will update on new commits. Review in cubic
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Chores