diff --git a/docs/reviews/SPEC-REVIEW-32-r1.md b/docs/reviews/SPEC-REVIEW-32-r1.md new file mode 100644 index 00000000..75b3b5c7 --- /dev/null +++ b/docs/reviews/SPEC-REVIEW-32-r1.md @@ -0,0 +1,150 @@ +# SPEC-REVIEW-32-r1 + +- Issue: https://github.com/Matysh/houseplan-card/issues/32 +- Этап: ревью ТЗ (PROCESS.md §2.4), заход r1 +- Материал: `docs/specs/032-unified-danger-confirmation.md` на SHA `206bcd1a` + (коммит «docs: refresh danger confirmation spec»), тело issue #32, все + комментарии (актуализации 2026-08-14, 2026-08-30, occupation-комментарий, + комментарий о готовности ТЗ на SHA 206bcd1a) +- Трек: полный (владелец снял `small` 2026-08-30: не проходит критерии «одна + поверхность» и «нет нового UX-контракта», задета touch) +- Бюджет: заход r1, блокирующих циклов израсходовано 0 из 4 (лимит 4, полный трек) + +## Скоуп проверки + +ТЗ описывает замену восьми прямых browser `confirm()` единым +`hp-confirm`-компонентом поверх `hp-dialog` с promise-based host API, +race-safety-контрактом (revalidation после `await`), touch/i18n-требованиями и +границами со специализированными диалогами (комната, стена с проёмами, декор, +backup/optimize, Tap confirmation). + +Проверялось: + +1. `docs/SCOPE.md` — соответствие J3/J6 и lock-инварианту («единственная + разрешённая поверхность актуации замка — кнопка в карточке проёма, которая + дополнительно подтверждает перед unlock»); +2. `AGENTS.md`, `PROCESS.md` §2.4/§2.5/§7.1/§4 — обязательные разделы ТЗ, + критерии DoR, бюджет циклов; +3. `docs/ARCHITECTURE.md` § One modal contract / One transient-surface contract; +4. `docs/TOUCH-SUPPORT.md` § Safety floor; +5. фактический код: все 8 call sites `confirm(` в `src/**`, существующие + специализированные диалоги (`_roomDeleteDialog`, `_partitionDeleteDialog`, + `confirm.erase_decor`, backup/optimize), `src/hp-dialog.ts` (focus session, + autofocus, Escape), `src/houseplan-editor-runtime.ts::_deleteSpace` + (blocker-проверка перед confirm); +6. два спеки-precedента (`337-lazy-editor-chunk.md`, + `348-german-localization.md`) как калибровка того, что в этом репозитории + считается «затронутые файлы» и «влияние на производительность» в ТЗ + сопоставимого масштаба. + +## Находки + +### Medium (в скоупе задачи — чинится в этой же правке ТЗ) + +**M1. DoR-пункт «влияние на производительность и бюджеты названо» не закрыт, +хотя задача добавляет новый eager-код.** + +- Файл: `docs/specs/032-unified-danger-confirmation.md` +- Строка: §5.2 (владение состоянием) и §10 (тестовый план) +- Суть: §5.2 явно решает архитектурный вопрос — «Controller находится в eager + root, поэтому onboarding не тянет editor chunk, а View unlock не загружает + его». Это осознанное добавление нового кода (`hp-confirm.ts` + controller + + разметка для всех 6 сценариев) в **eager root graph**, который в этом + проекте является бюджетируемой величиной: `AGENTS.md` называет + `npm run run bundle:budget # initial View graph <= 256000 B gzip (#337)` в + одном ряду с `bundle:sync` как обязательный после-сборочный гейт, и + прецедентный `docs/specs/337-lazy-editor-chunk.md` трактует именно этот + initial-view-бюджет как central product constraint с отдельным разделом и + AC. Раздел 10 текущего ТЗ (обязательные команды перед S7) перечисляет + `bundle:sync`, но не `bundle:budget`; ни один AC (§9) не привязан к размеру + eager-бандла. +- Воспроизведение: `npx tsc --noEmit` / `npm test` этого не поймают — + `bundle-budget.mjs` работает только по manifest после `npm run build`, и + если его не прогнать явно, регресс initial View graph (пусть и небольшой) + останется незамеченным до пре-релизного гейта, где чинить дороже, чем сейчас + дописать одну строку в тестовый план. +- Почему это Medium, а не Low: DoR (PROCESS.md §2.5) прямо требует «влияние на + производительность и бюджеты названо (или явно «нет»)» как условие входа в + `S5-ready`; сейчас оно не названо ни явно, ни как «нет» — и «нет» здесь было + бы неверным утверждением, раз §5.2 сознательно добавляет код в eager graph. + Фикс дешёвый: добавить предложение с ожидаемым порядком величины прироста + (или явную оценку «в пределах текущего headroom») и добавить + `npm run bundle:budget` в список команд §10 рядом с `bundle:sync`. +- Находка в скоупе issue #32 (сам ТЗ), чинится в этой же правке ТЗ — не + отдельный issue (owner's decision 2026-08-19, #202). + +## Что проверено и корректно + +- **Полнота охвата call sites.** Таблица §4.1 называет ровно 8 сценариев; + `rg 'confirm\('` по `src/**` даёт ровно те же 8 строк в + `houseplan-card.ts`, `houseplan-editor-runtime.ts`, + `houseplan-onboarding-runtime.ts` — расхождений нет. +- **Границы со специализированными диалогами не выдуманы.** Проверено чтением + кода: `_roomDeleteDialog` (houseplan-editor-runtime.ts:11599-11618) — уже + `hp-dialog` с двумя вариантами (keep/with walls); `_partitionDeleteDialog` + (:11564-11587) — уже `hp-dialog` со списком проёмов; `confirm.erase_decor` + (:5539) — уже кастомный `hp-dialog`; backup/import/optimize — многошаговые + диалоги с собственными подтверждениями (`backup.confirm_detach`, + `backup.replace_warning`). Исключение этих четырёх групп из scope (§4.2) + соответствует факту, а не догадке. +- **Claim о существующем focus/autofocus/Escape-контракте hp-dialog не + выдуман.** `src/hp-dialog.ts` действительно реализует focus session, + scoped к shadow root, `[autofocus]`-резолюцию и обработку `Escape` (строки + 237-396) — §6.1 ТЗ корректно ссылается на наследуемое поведение, а не + придумывает новое. +- **Race-safety claim по пространству проверен по коду.** + `_deleteSpace` (houseplan-editor-runtime.ts:8758-8784) действительно + выполняет dependency/blocker-проверку **до** `confirm()` — ровно то, что + §6.3 таблицы утверждает («существующая blocker-проверка остаётся раньше + confirm»). +- **Lock-инвариант SCOPE.md не нарушается и не переопределяется.** §4.2 + явно оставляет закрытие замка без подтверждения (безопасное действие) и не + трогает единственную разрешённую поверхность актуации; AC-9 отдельно требует, + чтобы unlock не вызывался после cancel/stale state. +- **Обязательные разделы §7.1 присутствуют**: сценарий и персона (§1), + «до/после» одной фразой без терминов реализации (§2), проблема и цель (§3), + scope/не-scope (§4), контракт поведения и API (§5, §7), UX (§6), данные и + миграция (§8 — явно «миграция отсутствует»), AC1-12 с доказательством (§9), + план автотестов (§10), риски (§11), откат (§12), release-артефакты (§13). +- **Явных догадок, выданных за факт, не найдено.** Единственное место, где ТЗ + сознательно оставляет свободу выбора — §6.1 «action использует warning/on + акцент» для unlock-кнопки — не выдаёт решение за факт, а явно формулирует + как одно из двух допустимых значений; в проекте нет отдельного `.btn.warning` + класса, только warning-баннер, так что оба варианта («on»-акцент или новый + warning-акцент) реалистичны и не противоречат существующим стилям. Отношу это + к техническому допущению (§14 ТЗ его не называет явно, но по духу оно туда + подходит) — не блокирую, ревьюер кода вправе потребовать явного выбора при + коде. +- **Открытых продуктовых вопросов нет** — комментарий владельца от 2026-08-30 + подтверждает, что контракт достаточно определён; ревьюер не находит + продуктового вопроса, который стоило бы вынести отдельно (единственный + недостающий пункт, M1, технический и не требует решения владельца). +- **Ссылки issue ↔ ТЗ на месте** в обоих направлениях (тело issue → файл ТЗ; + `docs/specs/README.md:39` → issue). + +## Чего не проверял + +- Реализацию — её нет, стадия `S4-spec-review`, код не писался. +- Точный список i18n-ключей (en/ru/de/fr) — ТЗ описывает контент body/title по + сценариям (§6.3), но не называет конкретные строковые ключи. Признано + приемлемым для этой стадии: AC-11 требует машинной проверки паритета через + `test/i18n.test.mjs`, а конкретные имена ключей — техническая деталь + (PROCESS.md §7.1: «именование» решают агенты сами), не продуктовый вопрос. + Не поднимаю до Medium. +- Golden/perf профили не запускал — на этой стадии нет кода для прогона; + `npm run typecheck`/`npm test`/`npm run build` не прогонялись, так как + ревью ТЗ не гейтится кодовыми командами (правки затрагивают только + `docs/specs/032-*.md`, класс C). +- Не сверял ТЗ с `docs/CONFIG-COMPATIBILITY.md` построчно — §8 ТЗ утверждает + «persisted schema не меняется, миграция отсутствует», что для чисто + presentation-контракта (без изменения config/layout) выглядит правдоподобно + и не противоречит ничему найденному в CONFIG-COMPATIBILITY.md; не стал + открывать весь документ ради этой проверки, так как diff ТЗ прямо + утверждает «вне scope: формат config/layout». + +## Вердикт + +Единственная находка (M1) — Medium, строго в скоупе этого же ТЗ, чинится +дополнением двух мест в тексте (не код, не отдельный issue). High-находок нет. +Итог: **жёлтый**, автор ТЗ правит §5.2/§10, повторный заход разбирается по +дельте (PROCESS.md §2.10) относительно этого документа и SHA `206bcd1a`.