diff --git a/docs/reviews/CODE-REVIEW-32-r1.md b/docs/reviews/CODE-REVIEW-32-r1.md new file mode 100644 index 00000000..b882f518 --- /dev/null +++ b/docs/reviews/CODE-REVIEW-32-r1.md @@ -0,0 +1,294 @@ +# CODE-REVIEW #32 · r1 — единое подтверждение опасных действий + +- Issue: https://github.com/Matysh/houseplan-card/issues/32 +- Этап: code (PROCESS.md §2.7) +- Заход: r1 · блокирующих циклов израсходовано 0 из 4 +- Материал: `git log --oneline origin/dev..HEAD`, `git diff origin/dev...HEAD` +- ТЗ: `docs/specs/032-unified-danger-confirmation.md` (spec-review зелёный на r2, + SHA `206bcd1a`, документы `docs/reviews/SPEC-REVIEW-32-r{1,2}.md`) +- Это первый раунд код-ревью — предыдущего кода-раунда нет, разбор полный, + разделы «Унаследовано из r0» и «Закрытие r0» не нужны. + +## Скоуп диффа + +`origin/dev...HEAD`, 57 файлов, ключевые продуктовые: + +- `src/danger-confirm.ts` (новый) — `HpConfirmController`, replace-not-queue, + token-based resolve. +- `src/hp-confirm.ts` (новый) — presentation поверх `hp-dialog`. +- `src/houseplan-card.ts` — controller-owner, host API `_confirmDanger`/ + `_cancelDangerConfirm`, cancel на смене space/mode/route/disconnect, unlock + call site. +- `src/houseplan-editor-runtime.ts`, `src/houseplan-onboarding-runtime.ts` — + миграция 7 из 8 call sites (draft whole/segment, marker, plan×2, space×2). +- `src/i18n/{en,ru,de,fr}.json` — новые title/body ключи. +- `src/styles/dialogs.styles.ts` — стили `.danger-confirm-*`. +- `test/danger-confirmation.test.mjs` (новый), правки + `test/render-device-snapshot.test.mjs`, `tsconfig.test.json`. +- `demo/smoke_danger_confirmation.mjs` (новый), правки `demo/smoke_lock_action.mjs`, + `demo/smoke_lock_invariant.mjs`, `demo/smoke_saved_plans.mjs`. +- Документация: `docs/ARCHITECTURE.md`, `docs/USER-GUIDE.ru.md`, оба CHANGELOG, + сгенерированное дерево бандла (dist + custom_components + demo/srv). + +Основа сверена читением: все 8 call sites §4.1 ТЗ найдены в диффе как +`await this._confirmDanger(...)` / `await this.host._confirmDanger(...)`; +прямых вызовов `confirm(` в `src/**` не осталось (перепроверено `rg` и +собственным прогоном `test/danger-confirmation.test.mjs`). + +## Как проверялось + +Зелёного Validate на SHA `7b363caa` нет — все гейты ниже прогнаны мной лично +в рабочей копии на этом SHA. + +### Дешёвые гейты (прогнаны) + +| Команда | Результат | +|---|---| +| `npx tsc --noEmit` | чисто, без вывода | +| `npm test` | `tests 1652 · pass 1651 · fail 0 · skipped 1` | +| `npm run bundle:sync` | build ok; `git status --porcelain` после — пусто (три дерева синхронны) | +| `npm run bundle:budget` | `initial View: 285355 B gzip (budget 300000 B, headroom 14645 B)` — совпадает с заявкой автора (+1300 B к baseline 284055, ≤3 KiB по AC-13) | +| `node scripts/check-docs.mjs` | `Documentation checks passed (7 files, 10 external links)` | +| `node scripts/no-new-any.mjs --base origin/dev --head HEAD` | `Новых any нет` (379 добавленных строк в 6 файлах) | + +`npm run invariants` не прогонял: дифф не трогает рёбра/толщину/`layout`/ +`marker.space`/`open_spans` — только диалоговый слой и call sites, +геометрия не затронута. + +`npm run golden:verify` не прогонял: новый диалог не входит в golden-матрицу, +и diff не показывает изменения существующего baseline (только служебные +скриншоты документации, см. ниже). + +`python -m pytest tests_backend -q` не прогонял: `custom_components/**/*.py` +не тронут (только сгенерированное `frontend/*`). + +Performance-профили не прогонял: не названы в AC, чувствительные к перфу пути +не затронуты. + +### Browser smoke — выбор и результат + +`node scripts/smoke-select.mjs --base origin/dev --head HEAD`: матрица 209 +смоков, 39 «прямое совпадение», 81 «слабая связь» (остальное неопределённость). +Прямые совпадения в основном объясняются тем, что дифф трогает общие поля +хоста (`_mode`, `_markerDialog`, `_spaceDialog`, `_physicalDialog`, +`_curSpaceCfg`, `_openingInfo`, `_commitSpace`) в функциях-обёртках +(`_setMode`, `_commitSpace`, `_editorSecondaryDialogBlocked`), а не потому что +эти смоки проверяют подтверждение как таковое. + +Прогнал (актуальный контракт диалога — cancel/accept/replace/focus/touch, +уже прогнанные автором плюс независимая перепроверка): + +`smoke_danger_confirmation`, `smoke_esc_dialogs`, `smoke_dialog_footer_width`, +`smoke_toggle_confirmation`, `smoke_opening_binding`, `smoke_lock_action`, +`smoke_lock_invariant`, `smoke_saved_plans`, `smoke_warm_dialogs`, +`smoke_editor_tabs` — все 10 зелёные (полный JSON output эквивалентен +заявке автора). + +Дополнительно прогнал прямые совпадения на `_physicalDialog`/`_openingInfo`, +которых не было в списке автора, потому что ровно они называют мигрированные +call sites (`_deleteDraftWhole`/`_deleteDraftSegment`, `_lockAction`): +`smoke_free_walls`, `smoke_registryless_opening`, `smoke_inert_openings`, +`smoke_open_passage`, `smoke_lazy_editor_chunk` (AC-10). + +Это вскрыло H1 (ниже): `smoke_free_walls` и `smoke_registryless_opening` +падают. Разбирая причину, прошёлся по всем демо-смокам, которые вообще +вызывают один из мигрированных методов +(`grep -rl "_deleteMarker\|_deleteSpace()\|_deleteServerPlan\|_deleteDraftWhole\|_deleteDraftSegment\|_deletePhysicalSelection\|_lockAction(" demo/smoke_*.mjs` +→ 11 файлов) и отдельно все, что стабили `window.confirm` +(`grep -rl "window\.confirm\s*=" demo/smoke_*.mjs` → 6 файлов), чтобы отделить +не задетые (`lock`-only, `partition`/`column`-kind, blocked-путь) от реально +сломанных. Результат — раздел «Находки» ниже. + +Остальные 29 прямых совпадений и все 81 слабых не прогонял: по чтению кода +дифф трогает эти поля только через общий `_editorSecondaryDialogBlocked`/ +`_cancelDangerConfirm` — они не меняют логику, которую эти смоки проверяют +(room dialog, merge/split, tap confirm, vac calibration, палитра мебели и +т.п.), а не потому что «выбор по теме» — их предмет прямо не пересекается ни +с одним из 8 мигрированных call sites (подтверждено `grep` выше: только +перечисленные 11 файлов вообще вызывают эти методы). + +## Находки + +### H1 — миграция ломает существующие смоки: 2 дают неверный вердикт, 3 виснут навсегда + +**Файлы:** `demo/smoke_free_walls.mjs`, `demo/smoke_registryless_opening.mjs`, +`demo/smoke_binding_picker.mjs`, `demo/smoke_hidden_flag.mjs`, +`demo/smoke_optional_space_model.mjs`. + +Задача заменяет синхронный `confirm()` на `await this._confirmDanger(...)`, +который резолвится только реальным кликом по кнопке диалога. Дифф обновил под +это ровно 3 существующих смока (`smoke_lock_action.mjs`, +`smoke_lock_invariant.mjs`, `smoke_saved_plans.mjs` — видно в +`git diff origin/dev...HEAD --stat`), но в дереве есть ещё 5 смоков, которые +вызывают один из тех же восьми call sites и по-старому либо стабят +`window.confirm = () => true` без взаимодействия с диалогом, либо не ждут +промис вовсе. Воспроизведено исполнением, не догадкой: + +``` +$ node demo/smoke_free_walls.mjs +FAILED (2): + - deleteOnDraftRemovesWholeOutline: expected true, got false + - invalidThicknessCreatesNothing: expected true, got false + +$ node demo/smoke_registryless_opening.mjs +FAILED (1): + - explicitInfoActionStillWorks: expected true, got false + +$ timeout 25 node demo/smoke_binding_picker.mjs +Terminated # процесс не завершается сам, требуется kill + +$ timeout 25 node demo/smoke_hidden_flag.mjs +Terminated + +$ timeout 25 node demo/smoke_optional_space_model.mjs +Terminated +``` + +Причина видна в исходниках смоков: + +- `smoke_free_walls.mjs:13` ставит `window.confirm = () => true;`, затем + синхронно вызывает `c._deletePhysicalSelection()` на `draft`-выборе (строки + 195–197) и тут же читает результат — `_deletePhysicalSelection` для + `kind: 'draft'` делает `this._deleteDraftWhole(); return;` без `await` + (`src/houseplan-editor-runtime.ts:3072`), поэтому реальное удаление + происходит позже, после клика по несуществующей кнопке — который никогда не + случается. +- `smoke_registryless_opening.mjs:89-91` тем же способом стабит `window.confirm` + и вызывает `card._lockAction(lockId, 'unlock')` без `await`, читая + `serviceCalls.at(-1)` сразу же. +- `smoke_binding_picker.mjs:33-34`, `smoke_hidden_flag.mjs:83-84,96-97`, + `smoke_optional_space_model.mjs:91-93` **ждут** промис + (`await c._deleteMarker()` / `await card._deleteSpace()`), но промис + `_confirmDanger` резолвится только явным `hp-confirm-decision`, а стаб + `window.confirm` больше никем не читается — сцена не диспатчит клик, + дизконнект или замену запроса, поэтому `await` виснет бесконечно. Три файла + проверял отдельно — во всех трёх процесс не завершается, `finish()` + (`demo/serve.mjs:35`, выставляет `process.exitCode = 1` при провале) вообще + не достигается. Это хуже явного провала: в CI такой смок не падает быстро, а + занимает слот до тайм-аута раннера. + +Не задеты (проверено, не потребовалось чинить): `smoke_orphan_space_references.mjs` +стабит `window.confirm` и ждёт `_deleteSpace()`, но сценарий останавливается на +blocker-проверке до `_confirmDanger` (`deleteExplainsBlockerWithoutConfirmOrWrite`), +поэтому не виснет — прогнан, зелёный. `smoke_opening_binding.mjs` и +`smoke_partition_openings.mjs` вызывают `_lockAction(..., 'lock')` (не 'unlock') +и `_deletePhysicalSelection()` на `partition`/`column`-выборе — ни один путь не +проходит через `_confirmDanger` по контракту §4.1/§7 ТЗ, поэтому оба зелёные +без правок. + +Это не «сторонний тест устарел сам по себе» — регресс создан ровно теми +изменениями, которые в скоупе задачи (§4.1 ТЗ, миграция `_deleteDraftWhole`, +`_deleteMarker`, `_deleteSpace`, `_lockAction`). Три из восьми уже +адаптированных смоков показывают правильный образец правки — кликать по +`hp-confirm .danger-confirm-footer .btn...` после `await c.updateComplete` +(см. `demo/smoke_lock_action.mjs`, `demo/smoke_lock_invariant.mjs`, +`demo/smoke_saved_plans.mjs`), тот же приём применим к оставшимся 5 файлам. +До правки полный pre-release smoke-прогон (§8) зависнет на трёх из них. + +**Серьёзность: High.** Блокирует; чинится в этой же задаче (§4.1 её +собственный скоуп), issue не заводится. + +### L1 — шесть старых односложных ключей i18n остались без потребителей + +`confirm.delete_draft`, `confirm.delete_draft_segment`, `confirm.remove_marker`, +`confirm.delete_space`, `confirm.unlock`, `confirm.delete_plan` присутствуют во +всех четырёх словарях, но после миграции ни один файл в `src/**`, `test/**`, +`demo/**` их не использует — проверено: + +``` +$ grep -rn "confirm\.delete_draft'" src/ test/ demo/ # и так для всех шести — пусто +``` + +ТЗ §8 разрешает удаление старых ключей «только если source/test search +доказывает отсутствие потребителей» — этот поиск и есть доказательство. +AC-11 (парность словарей) не нарушен: возврат чист, находка не о корректности, +а об оставленном мёртвом коде. + +**Серьёзность: Low.** Не блокирует; можно поправить в этой же правке (удалить +шесть ключей из всех четырёх словарей) либо оставить с запиской — на +усмотрение автора. + +## Что проверено и совпало с кодом + +- **AC-1/AC-2** — source-contract test (`test/danger-confirmation.test.mjs`, + тест «all dangerous-action call sites use the shared confirmation contract») + считает 0 вызовов `confirm(` и ровно 8 `await this(.host)?._confirmDanger(` + в перечисленных трёх файлах, плюс проверяет все 6 диагностических `key`. + Тест умеет падать: `nativeCalls.length === 0` и `sharedCalls.length === 8` + оба жёстко зафиксированы, любое отклонение красит тест. +- **AC-3/AC-4/AC-5/AC-6/AC-7/AC-8/AC-9** — `demo/smoke_danger_confirmation.mjs` + прогнан лично, зелёный; закрывает cancel-safety по всем 6 классам операций + (включая оба lazy runtime для plan/space), replace-семантику, race + (`staleContextCannotMutate`), фокус/трап/320px/локали. +- **AC-10** — `demo/smoke_lazy_editor_chunk.mjs` (не тронут диффом, прогнан + лично) остаётся зелёным: `onboardingDoesNotLoadEditor: true` — controller + и `hp-confirm` действительно в eager root, не тянут editor chunk. +- **AC-11** — `npm test` включает `test/i18n.test.mjs`, generic key-set parity + (`assert.deepEqual(Object.keys(dictionary).sort(), enKeys)`) прошёл для всех + четырёх словарей. +- **AC-12** — читением: `_roomDeleteDialog`, `_partitionDeleteDialog`, + `confirm.erase_decor`, backup/optimize диалоги не тронуты диффом (нет их в + списке изменённых файлов/функций), `_editorSecondaryDialogBlocked` получил + добавление `_dangerConfirm` в общий OR-список блокировки secondary-панели — + это тот же паттерн, что уже применён к `_tapConfirm`/`_roomDialog` и не + меняет их собственную логику. `smoke_editor_tabs`, `smoke_esc_dialogs` + (оба трогают `_markerDialog`/`_roomDialog`/`_spaceDialog`) зелёные. +- **AC-13** — см. таблицу гейтов выше, число совпадает с заявкой автора. +- **Race-safety §7** — читением всех 7 переписанных call sites + (`_deleteDraftWhole`, `_deleteDraftSegment`, `_deleteMarker`, + `_deleteServerPlan`×2, `_deleteSpace`×2): каждый захватывает identity + (`spaceId`/`draftId`/`segment`/`targetId`/`plan.url+modified`) до `await`, + затем перепроверяет её после — включая пересчёт `collectSpaceMarkerDependencies` + и `deletingLastSpace` заново для `_deleteSpace`, ровно как требует §7 ТЗ. + Дополнительно подтверждено исполнением: `staleContextCannotMutate` в + `smoke_danger_confirmation.mjs` меняет `_space` между открытием и `resolve` + и проверяет, что геометрия не мутировала. +- **Unlock revalidation** — `_lockAction` (`src/houseplan-card.ts:12766`) + перепроверяет `_openingEntityAvailable`, что `_openingInfo` — тот же + proём/тип/`lock` id, и что состояние всё ещё `'locked'`, иначе не вызывает + `hass.callService`. `test/render-device-snapshot.test.mjs` теперь проверяет + тот же порядок статическим разбором продакшен-исходника. +- **Blocker-проверка раньше confirm** (§4.1 таблица сценариев, «пространство») + — в обеих реализациях `_deleteSpace` блокер-чек и ветка «последнее + пространство» стоят строго до `_confirmDanger`, без изменений порядка + относительно dev. +- **Bundle sync** — сгенерированное дерево (`dist/**`, + `custom_components/houseplan/frontend/**`, `demo/srv/assets/**`) идентично + тому, что производит `npm run bundle:sync` на этом SHA (`git status` + пустой после локальной сборки). +- **Трейлеры** — все 7 коммитов имеют `Issue: #32`; `User-Visible: yes` стоит + ровно у коммита `c91ac74e`, и оба CHANGELOG изменены в этом же коммите + (`git show --stat c91ac74e`). +- **Документация** — `docs/ARCHITECTURE.md` (новый абзац One modal contract), + `docs/USER-GUIDE.ru.md` (терминология «Отмена», «Esc», диалог называет + объект — согласовано с текущим текстом гайда, не изобретено). + +## Чего не проверял + +- 29 из 39 «прямых» и все 81 «слабых» строк `smoke-select` — обоснование в + разделе «Как проверялось». +- `npm run golden:verify`, `python -m pytest tests_backend`, + performance-профили, `npm run invariants` — обоснование там же (не + затронуты диффом/AC). +- Ручное браузерное тестирование вне headless-смоков (touch-жесты на реальном + устройстве, экранный ридер) — вне возможностей цикла; доверился + `demo/smoke_danger_confirmation.mjs` и existing `smoke_esc_dialogs`/ + `smoke_dialog_footer_width` как прокси для focus trap/320px/screen-reader + разметки (`role`/`aria` наследуются от `hp-dialog`, не проверял вручную + скринридером). +- Полный прогон всех 209 демо-смоков — не требуется на этапе ревью + (PROCESS.md §8, это пред-релизный гейт), но именно поэтому H1 не является + «пропустил бы CI» — это ревью и должно было его поймать до пред-релизного + гейта, что и произошло. + +## Вывод + +AC-1…AC-13 подтверждены по коду и/или исполнением, кроме побочного эффекта +миграции на пять не входящих в §4.1 файлов демо-смоков (H1) — тот же класс +регресса, что и в описанном риске ТЗ «синхронный caller продолжит работу до +решения», только на testing-инфраструктуре, а не в продакшен-пути. Продуктовый +код и текстовый контракт корректны и полностью соответствуют ТЗ; блокирует +только H1. + +Вердикт: жёлтый. Возврат автору для правки 5 смоков (образец есть в этом же +диффе) и по желанию — снятия L1.