diff --git a/docs/reviews/SPEC-REVIEW-264-r1.md b/docs/reviews/SPEC-REVIEW-264-r1.md new file mode 100644 index 00000000..8ee4c2ca --- /dev/null +++ b/docs/reviews/SPEC-REVIEW-264-r1.md @@ -0,0 +1,233 @@ +# SPEC-REVIEW-264-r1 — контроллер Resize и commit-инвариант кладки + +- **Issue:** https://github.com/Matysh/houseplan-card/issues/264 +- **Этап:** ТЗ (S4-spec-review), заход r1, лимит циклов 4 (полный трек) +- **Ревьюер:** независимая сессия, без контекста автора ТЗ +- **Материал:** `docs/specs/264-resize-controller.md` на SHA `6e1aea3a` + (коммиты `a3e32995` «specify resize controller extraction», + `6e1aea3a` «add resize controller user scenario»; `git diff origin/dev...HEAD` + затрагивает только этот один файл, продуктовый код не менялся) +- **Вердикт: жёлтый · заход r1 · блокирующих циклов 1/4 · High: 0 · Medium: 2 → в задаче** + +## Скоуп проверки + +Полный разбор (первый заход, дельты нет). Прочитаны: +`docs/SCOPE.md`, `AGENTS.md`, `PROCESS.md`, тело issue #264 и все 4 комментария +(включая обе аналитики владельца от 2026-08-24 и 2026-08-27), `docs/RESIZE.md` +(канон Resize), `docs/specs/034-frontend-decomposition.md` (зонтичный слайс), +тело issue #277 (безопасный fixed-topology контракт, prerequisite). + +## Как проверялось + +Поскольку это этап ТЗ, а не код-ревью, тяжёлые гейты (`typecheck`/`test`/`build`) +не запускались — `git diff origin/dev...HEAD` показывает изменение только +`docs/specs/264-resize-controller.md`, продуктовый код не тронут. Вместо +прогона гейтов сверял каждое фактическое утверждение ТЗ о **текущем** состоянии +кода с реальным `src/houseplan-card.ts`, `src/resize.ts`, `src/wall-thickness.ts`, +`src/open-spans.ts`, `scripts/model-invariants.mjs`, `package.json` и `demo/`, +командами `grep`/`sed` — то, что задача прямо требует называть предположением +или доказывать, а не декларировать. + +## Находки + +### M1 (Medium, в скоупе) — утверждение «Escape в idle Resize уже сейчас сразу возвращает Draw» не соответствует коду + +**Файл:** `docs/specs/264-resize-controller.md`, §2 п.7 и §19 (строка риска +про `_rszSel`). + +**Утверждение ТЗ:** +> Мёртвое session-only поле `_rszSel` удаляется: после #277 оно не имеет +> renderer-потребителя и не влияет на пиксели, config или Undo. **Escape в +> idle Resize, как и фактически сейчас, сразу возвращает neutral Draw.** + +и в таблице рисков (§19): +> Removal of `_rszSel` changes warm/Escape behavior | Source/runtime parity +> test; **it has no render consumer and idle Escape ends at Draw.** + +**Воспроизведение по коду.** `src/houseplan-card.ts:2701-2710` +(обработчик Escape, тул `resize`, вне драга): + +```ts +if (this._tool === 'resize') { + e.preventDefault(); + if (this._rszDrag) { + this._rszCancelDrag(); + return; + } + if (this._rszSel) this._rszSel = null; + else this._tool = 'draw'; + return; +} +``` + +Это ветвится **не всегда одинаково**: если пользователь щёлкнул по площади +комнаты в инструменте Resize (обработчик клика на `src/houseplan-card.ts:7802-7804` +ставит `this._rszSel = room?.id || null`), первый Escape только сбрасывает +`_rszSel` и остаётся в Resize; тул на Draw переключает только **второй** +Escape. То есть сегодня «idle Escape» **не** всегда сразу возвращает Draw — +это зависит от того, была ли до этого выбрана комната кликом. + +`_rszSel` действительно не имеет визуального потребителя (проверено: +`demo/smoke_hide_layers.mjs:159` и `demo/smoke_pan_any_zoom.mjs:134` +намеренно выставляют `_rszSel`, доказывая лишь отсутствие старой corner-frame +разметки #277 — это подтверждает часть про «не влияет на пиксели», но не +часть про Escape). Значит убрать это поле «как есть» без изменения самого +`if (this._rszDrag) {...} if (this._rszSel) ... else this._tool = 'draw';` +означает буквально удалить условную ветку — и тогда Escape при выбранной +(невидимо) комнате в Resize станет **сразу** переключать на Draw, чего +сегодня не происходит. + +**Почему это находка, а не мелочь.** §1 ТЗ прямо обещает персоне, что слайс +«обязан быть полностью незаметен», и в перечне неизменного в самом начале +документа: «DOM, пиксели, тексты, **жесты** и persisted config не меняются». +Escape-последовательность — это жест. Документ одновременно (а) заявляет +парность с текущим поведением как факт, (б) сам же перечисляет удаление +`_rszSel` как часть скоупа (§10) и своей же мерой риска называет то самое +неверное утверждение о «текущем» поведении. Это ровно тот класс дефекта, +о котором предупреждает процесс: догадка о поведении, записанная как факт, +проходит ревью, потому что выглядит решением. + +**Требуемое исправление одно из двух** (решает автор, не владелец — это +техническая деталь конечного автомата контроллера, не то, что видит +пользователь по существу задачи): +1. явно зафиксировать, что рефакторинг **меняет** это конкретное поведение + (Escape после выбора комнаты в Resize отныне сразу выходит в Draw), и + убрать формулировку «как фактически сейчас» — задокументировать как + осознанное упрощение; либо +2. сохранить двухшаговый Escape, перенеся понятие «комната выбрана» + в состояние контроллера (например, отдельный `hasSelection`/`picked` + в typed outcome конечного автомата §6), а не как «мёртвое» поле для + простого удаления. + +Ни один из вариантов не требует нового вопроса владельцу — это находка +внутри уже принятой владельцем границы «что делает автор» (§7.1 PROCESS.md). + +### M2 (Medium, в скоупе) — несуществующий файл смока в планируемом diff + +**Файл:** `docs/specs/264-resize-controller.md`, §15 «Файлы и модули». + +ТЗ перечисляет как существующий поведенческий тест: +> существующие `demo/smoke_safe_resize.mjs`, `demo/smoke_resize_wall_thickness.mjs`, +> `demo/smoke_resize_inner_dimensions.mjs` — behavioral parity; + +`demo/smoke_safe_resize.mjs` **не существует**: + +``` +$ ls demo/smoke_safe_resize.mjs +ls: cannot access 'demo/smoke_safe_resize.mjs': No such file or directory +``` + +Канонический список Resize-смоков — раздел «Verification» в `docs/RESIZE.md` — +называет `demo/smoke_room_resize.mjs` (production bundle pointer handlers, +preview/commit/Undo, disabled a11y, production-preflight failure и +cancellation — именно то, что §15 хочет накрыть словом «behavioral parity») +и `demo/smoke_resize_pointer_real_plan.mjs`. Оба реально существуют в `demo/`. +Тело исходного issue #264 в разделе «Как проверять» тоже не называет +`smoke_safe_resize`, только `smoke_resize_wall_thickness` и +`smoke_resize_inner_dimensions` — значит ошибка внесена автором ТЗ, не +унаследована из issue. + +**Почему это находка.** Список файлов в DoR — то, по чему исполнитель и +ревьюер кода сверяют полноту diff'а (§2.5, §7.1 PROCESS.md). Несуществующее +имя тут — либо повод по ошибке создать новый файл-дубликат вместо запуска +существующего `smoke_room_resize.mjs`, либо просто нерабочая команда при +локальном прогоне. Дешёво чинится заменой имени на `demo/smoke_room_resize.mjs` +(и, по желанию автора, добавлением `demo/smoke_resize_pointer_real_plan.mjs` +рядом, раз он тоже покрывает production pointer path). + +## Что проверено и корректно + +- **Структура ТЗ** покрывает все обязательные разделы §7.1 PROCESS.md: + сценарий и персона (§1, добавлен вторым коммитом — реагирует именно на то, + что требует шаблон), что человек увидит до/после (§1), проблема (§1), + скоуп/не-скоуп (§10/§11), контракт поведения (§4-§9 плюс явная ссылка на + канон #277), UX/touch (§14), данные и миграция (§18 — нет), i18n (§18 — + нет), AC1…AC12 с методом доказательства (§16), план автотестов (§17), + риски (§19), откат (§20), release-артефакты (§18). +- **Продуктовый вопрос из тела issue закрыт корректно.** Исходный открытый + вопрос «что делать при нарушении инварианта» (отменить жест / записать + диагностику / показать пользователю) снят ссылкой на уже выпущенный #277 + (fail-closed, ноль записи) — это подтверждено и в комментарии владельца от + 2026-08-24/27, и напрямую в §2 ТЗ. Новых открытых продуктовых вопросов + документ не содержит и не должен: остальные решения (имена типов, файлы, + разбиение controller/root) — техническая территория автора по §7.1. +- **Все ссылки на существующий код в архитектурном разделе (кроме M2) + фактически верны**, что важно для refactor-only задачи с высоким риском + «догадки, выданной за решение»: + - все перечисленные `_rsz*` mutable-поля (`_rszDrag`, `_rszPreview`, + `_rszLive`, `_rszEligibilityCache`, `_rszSel`) реально существуют в + `src/houseplan-card.ts` (119 упоминаний `_rsz` в файле); + - «inline сравнение внутри Resize» из §0 реально существует — + `src/houseplan-card.ts` (`_rszApplyPreview`) сравнивает + `JSON.stringify(beforeWallCms) !== JSON.stringify(afterWallCms)` + отсортированных мультимножеств `cm`, что уже сегодня фактически является + exact-multiplicity проверкой (включая `cm:0`), которую §8 предлагает + формально вынести в общий модуль — согласуется; + - `checkWallRecordsPreserved(before, after, { allowClear = false })` в + `scripts/model-invariants.mjs:301` действительно фильтрует `cm > 0` + (presence-семантика #254), т.е. новый режим `exactMultiplicity` из §8 — + это расширение, а не переписывание текущего поведения; + - паттерн «CLI импортирует production-модуль из `test-build/*`, собранного + `tsc -p tsconfig.test.json`» уже используется для + `plan-geometry-preflight`/`near-axis`/`coordinate-canonicalization` + (`scripts/model-invariants.mjs:21-26`), и `npm run invariants` в + `package.json` действительно гоняет `tsc -p tsconfig.test.json` перед + CLI — план §8 по добавлению туда же `wall-record-preservation` технически + исполним без нового механизма; + - все геометрические функции, на которые ссылается §4.3/§6 + (`resolveSafeResize`, `clampSafeResize`, `applySafeResize`, + `validateSafeResize`, `safeResizePointerDisplacement` в `src/resize.ts`; + `rekeyWallsAfterMoveChecked` в `src/wall-thickness.ts`; + `rekeyOpenSpansAfterMove` в `src/open-spans.ts`) существуют под теми же + именами; + - `docs/TESTING.md`, `docs/ARCHITECTURE.md`, `docs/STATUS.md`, + `tsconfig.test.json` — все существуют, ссылки на них в §15/§18 корректны; + - `WarmViewport.rszSel` (`src/houseplan-card.ts:430`) — это поле + неперсистентного модульного кэша `_warmVp` («the memo is module state, + never serialised»), не часть сохранённого конфига — утверждение §9 о + том, что удаление `rszSel` из `WarmViewport` не меняет сериализуемый + config, подтверждено чтением кода. +- **Performance-бюджеты §13** (pointermove p95 ≤16 мс/≤20% над baseline, + pointerup p95 ≤75 мс) дословно совпадают с уже канонизированными в + `docs/RESIZE.md`, а не изобретены заново. +- **Класс изменения и трейлеры.** Class A+B+C, полный флоу — верно для + задачи, трогающей `src/**` (Class A) даже как рефакторинг; `User-Visible: no` + корректен для содержания ТЗ *после* исправления M1 (сейчас документ сам + себе противоречит по этому пункту, см. находку). +- **Соответствие SCOPE.md.** Задача не создаёт нового пользовательского + поведения, служит устойчивости J6 («Keep the plan true as the home + evolves» — drag/resize) снижением риска регрессий в самом багоносном + инструменте; входит в уже согласованный владельцем зонтичный слайс #34. + Продуктовая ценность честно указана низкой (2/10), инженерная — обоснованно + высокой (9/10); это не попытка протащить фичу под видом рефакторинга. +- **Порядок slice vs #34.** Расхождение «сначала диалог или сначала Resize», + которое исходный текст issue называл открытым, закрыто прямым решением + владельца («Взято в работу по решению владельца 2026-08-27») — ТЗ корректно + это не пересматривает. + +## Чего не проверял + +- **Гейты `typecheck`/`test`/`build`/`check-docs`/`invariants` не + запускались** — на этом этапе нет продуктового кода, диапазон + `git diff origin/dev...HEAD` содержит только markdown-файл спецификации. + Это решение по объёму, а не пропуск: гейты станут обязательны на код-ревью + (§7 PROCESS.md, S7-code-review). +- **Смоки `demo/smoke_*resize*.mjs` не запускал** — на этом этапе нечего + прогонять, поведение ещё не реализовано; факт существования файлов + (кроме M2) подтверждён только `ls`, не исполнением. +- **Не проверял детально типизацию generics `ResizeControllerOutcome`** — §5 сама оговаривает «конкретные имена типов + могут уточняться при реализации», это явно техническая, не продуктовая + граница, оставляю автору. +- **Не сверял построчно все 18 пунктов матрицы edge cases (§12) с кодом** — + выборочно (пп. 5, 6, 12, 14) сверил с текущей реализацией `_rszMove`/`_rszUp` + и они совпадают; полную построчную сверку всех 18 не делал, так как это + либо прямое наследование уже выпущенного #277 (не предмет этого ТЗ), либо + будет доказано тестами на код-ревью. + +## Итог + +Two Medium findings, both in scope, ни один не является продуктовым вопросом +для владельца — оба технические и решаются автором в этом же issue. High-находок +нет, вердикт жёлтый, документ засчитывает заход r1 и тратит первый из четырёх +циклов бюджета §4.