# Review — reset-item-missao

**Base**: working tree (unstaged) vs HEAD
**HEAD**: `730bf3b4`
**Arquivos tocados**: 4 (3 modificados + 1 novo teste)
**Linhas**: +77 / −16 (produção) + 514 (teste novo)

## Sumário

Feature é uma extensão cirúrgica em código V1 legado (`Inteligencias`/`Satelites` do `MissaoLogistica`), conforme já justificado no techplan. A spec, o techplan e o backend conversam coerentemente entre si: o dev seguiu o pseudocódigo quase à risca, manteve o estilo dos handlers irmãos do mesmo Satélite, resolveu corretamente a confusão `dados_adicionais` × `dados_coletados`, e cobriu o gating e o rebote conforme os ACs. Os testes são análise estática + lógica pura — não cobrem o caminho real de banco mas isso está documentado em `05-testes.md` e é proporcional para um bug-fix em legado. **Recomendação: merge** com uma correção minor de copy e validação opcional dos pontos abaixo. Os "blockers" da checklist V2 (sem raw SQL, sem extender V1 em código novo, etc.) não se aplicam aqui — esta feature é manutenção em código V1 explicitamente, com decisão documentada do TL.

## Issues

### 🔴 Blocker (0)

Nenhum. Os itens da checklist V2 que costumam virar blocker (raw SQL, código novo estendendo `Inteligencia` V1, migration sem `block_*`, controller mal-nomeado, lógica em controller, sem testes para Laboratório) não se aplicam: não há Sinal/Sonda/Laboratório/migration novos. A spec e o techplan determinam explicitamente que a edição é em código V1 existente.

### 🟡 Major (0)

Nenhum.

### 🟢 Minor (3)

#### M1 — `data-confirm-msg` perdeu menção a "histórico de conclusão e dados coletados"

**Onde**: `componentes/Missoes/Logistica/sinais/Designacoes/html/designacao_painel.php:1247`
**Problema**: a copy nova é mais curta e menciona apenas "dados de execução" + o rebote. A copy antiga era mais explícita ("Todo o histórico de conclusão e dados coletados deste item serão resetados"). A nova é OK semanticamente mas perde clareza para o gestor.
**Sugestão**: apenas confirmar com o time de produto se a copy curta é mesmo a desejada (a spec AC3.2 pediu "mesma linha de tom/tamanho do confirm atual"; o tom mudou ligeiramente). Não bloqueia merge.

#### M2 — `revert` na falha do `save()` da missão re-aplica `dados_coletados` como string crua

**Onde**: `componentes/Constelacoes/Logistica/MissaoLogistica/Satelites/MissoesItensEdicao.php:235-244` (bloco do best-effort revert)
**Problema**: o revert passa `'dados_coletados' => $item->dados_coletados ?? null`. Como `$item` veio de `find(...)->fetch()`, esse campo é a string JSON original (CMS Active Record retorna como string). Reescrever a string original de volta na coluna JSONB é seguro no PostgreSQL (cast implícito), mas vale uma sanidade: se algum dia a leitura passar a hidratar para array/objeto, o revert vai falhar silenciosamente. Pre-existente no padrão da Inteligencia V1, mas vale o comentário.
**Como mitigar**: documentar inline com `// dados_coletados retorna como JSON string crua, OK` ou fazer um teste de integração real (fora do TDD estático atual) para garantir round-trip.

#### M3 — Mensagem de retorno PT-only sem `show()` / i18n

**Onde**: `MissoesItensEdicao.php:241, 269, 271, 232, 220, 213, 196, 187`
**Problema**: a checklist V2 pede `show()` para strings visíveis e chaves em pt/en/es. Aqui as mensagens (`'Item voltado para pendente. Missão reaberta para Em Andamento.'`, `'Erro ao rebotar a missão...'`, etc.) são PT puro inline, sem `show()`. **Mas**: o satélite inteiro já é PT-only sem `show()` (todos os handlers irmãos `adicionar`, `remover`, `duplicar` seguem o mesmo padrão). Trocar agora seria fora de escopo. A spec inclusive cita "se aplicável; conferir convenção atual" — convenção atual aqui é PT-only.
**Sugestão**: deixar TODO/issue separada para migrar todo o `MissoesItensEdicao.php` para `show()` quando/se for migrado para V2. Não bloquear este merge.

### ⚪ Nit (3)

#### N1 — Aviso de validação enganoso no `MissaoLogistica::save()`

**Onde**: `componentes/Constelacoes/Logistica/MissaoLogistica/Inteligencias/MissaoLogistica.php:74-77`
**Problema**: ao usar `make(['id' => $missaoId, 'status' => ...])` + `save()`, se `required()` falhar a mensagem de warning fala em "tipo_missao_id ou status" — mas neste fluxo `tipo_missao_id` realmente não está em scope. Hoje `$required = []` então isso nunca dispara. Pre-existente, fora de escopo desta feature, só registrado para visibilidade.

#### N2 — Alinhamento de arrays inconsistente

**Onde**: `MissoesItensEdicao.php:202-212` vs `:235-244`
**Problema**: o primeiro array de update usa `=>` alinhados, o array de revert também — estilo OK; mas a mistura de espaços antes do `=>` (1 ou 2 espaços para alinhar) gera diff visualmente ruidoso. Cosmético, não impacta linter.

#### N3 — `data-confirm-msg` poderia escapar HTML

**Onde**: `designacao_painel.php:1247`
**Problema**: a string contém apenas ASCII e está dentro de atributo `data-...="..."` no HTML. Não há risco de injeção (string estática). Pure nit.

## Pontos fortes

- **Resolveu corretamente a confusão `dados_adicionais` × `dados_coletados`** detectada pelo techplan (linha 56 do 02-techplan.md). O array de update é cirúrgico e preserva metadados de domínio (`dados_adicionais`, `pulado_vezes`, `motivo_remocao`) — exatamente o que a spec AC1.3 pedia.
- **Replicou o padrão dos handlers irmãos** (`adicionar`, `remover`, `duplicar`) em vez de tentar introduzir Sonda/Laboratório novos no meio do legado — alinhado ao techplan e à decisão do TL.
- **Best-effort revert documentado** (linhas 218-241): a estratégia segue a documentação de atomicidade do techplan e tem early-return idempotente para item já pendente, o que mitiga o risco residual descrito.
- **Mensagens diferenciadas** para os dois caminhos (sem rebote / com rebote) — atende AC2.5 com texto exatamente igual ao do techplan.
- **Enum `MISSAO_REABERTA`** adicionado nos três `match()` (`descricao`, `icon`, `categoria`) — mantém o invariante de exaustividade que o techplan riscou como risco 4.
- **Teste TDD cobre as ramificações relevantes** (gating, rebote, no-op, campos zerados, presença de copy no painel). Embora seja análise estática + lógica pura, é proporcional ao escopo (bug-fix em V1 legado) e o `05-testes.md` é honesto sobre os gaps de integração.
- **Não chamou `transicionar()`** (decisão TL do risco 5 do techplan) — confirmado pelo teste 3.14.

## Recomendação

✅ **Merge.** Zero blockers, zero majors. Os 3 minors são todos comentários/sugestões de copy ou TODOs de migração V2 fora do escopo desta feature. Antes do commit, considere apenas:

1. Validar a copy do `data-confirm-msg` com produto (M1).
2. Abrir issue separada para migração `MissoesItensEdicao` → `show()` / V2 (M3) — não nesta feature.
3. Rodar manualmente os cenários 5a-5d do techplan em `dev_gx_goldie` antes de subir para staging — o TDD cobre a estática mas o caminho real de banco (especialmente o `dados_coletados` como JSONB e o `MissaoLogistica::save()` parcial) merece um smoke test ao vivo.