diff --git a/docs/reviews/SPEC-REVIEW-293-r1.md b/docs/reviews/SPEC-REVIEW-293-r1.md new file mode 100644 index 00000000..6cd94bdd --- /dev/null +++ b/docs/reviews/SPEC-REVIEW-293-r1.md @@ -0,0 +1,165 @@ +# SPEC-REVIEW-293-r1 + +- **Issue:** https://github.com/Matysh/houseplan-card/issues/293 +- **Этап:** spec (PROCESS.md §2.4) +- **Заход:** r1 · блокирующих циклов израсходовано 0 из 4 +- **Ветка/SHA:** `issue/293-resize-pointer-noop` @ `c514b534` (комментарий автора называет этот SHA явно) +- **Артефакт под ревью:** `docs/specs/293-resize-pointer-noop.md` (211 строк, добавлен впервые — единственный файл в `git diff origin/dev...c514b534`, продуктовый код не менялся) + +## Скоуп ревью + +Первый заход — разбор полный (§2.10 не применяется, дельты предыдущего раунда нет). +Проверялись: соответствие `docs/SCOPE.md` (Core user jobs), формальная полнота ТЗ +по §7.1, однозначность и проверяемость AC1–AC10, отсутствие догадок, выданных за +факт, согласованность с каноническим `docs/RESIZE.md` и `docs/TOUCH-SUPPORT.md`, +терминологическое соответствие `docs/USER-GUIDE.ru.md`, фактическая точность +утверждений о текущем коде и фикстуре. + +## Как проверялось + +1. `gh issue view 293 --comments` — тело issue + оба комментария (аналитика + 2026-08-24, подготовка ТЗ 2026-08-24). +2. `git diff origin/dev...origin/issue/293-resize-pointer-noop --stat` — подтверждён + единственный изменённый файл, продуктового кода в диффе нет. +3. Прочитан весь текст `docs/specs/293-resize-pointer-noop.md` (`git show :...`). +4. Прочитаны `docs/SCOPE.md`, `AGENTS.md`, `PROCESS.md` (§1–§9), `docs/RESIZE.md` + (целиком), `docs/TOUCH-SUPPORT.md` (выборочно, safety-floor раздел), + `docs/USER-GUIDE.ru.md` (раздел Resize + поиск терминов toast/Undo/preview). +5. Численно проверено ключевое фактическое утверждение раздела 1 ТЗ: в + `test/fixtures/real-plan-second-floor.json` `room-a.poly[2]` = `[0.4,-0.208…]→ + [0.4,1.267…]`, `room-b.poly[2]` — та же пара в обратном порядке. Общая стена с + точным совпадением концов подтверждена, это не догадка. +6. Прочитан код `src/houseplan-card.ts` вокруг `_rszEdgeDown/_rszMove/_rszUp/ + _rszApplyPreview` (строки 8548–8735) — все символы, названные в ТЗ как текущее + поведение, реальны и ведут себя так, как описано (projection на `plan.n`, + заморозка последнего валидного preview при reject, `_svgPoint` через + `clientX/clientY` + `getBoundingClientRect`). +7. Прочитан существующий `demo/smoke_room_resize.mjs` целиком — его `pointer()` + диспатчит `dispatchEvent(new PointerEvent(...))` из контекста страницы, а + `enter()`/`setRooms()` подставляют `_tool` и `_serverCfg.spaces[…].rooms` + программно, минуя тулбар и серверную загрузку — ровно то ограничение стенда, + которое issue называет ограничением собственного репро. +8. Сверено с `docs/specs/281-resize-zero-range.md` и `docs/specs/README.md` — + для установления действующей конвенции секций/индекса на этом треке. + +Гейты `typecheck`/`test`/`build` не запускались — на этапе ревью ТЗ они не +предмет проверки (нет изменённого продуктового кода); они предмет код-ревью +(§2.7) после реализации. + +## Находки + +### Medium (в скоупе — жёлтый вердикт, чинится в этом же ТЗ) + +**M1. Отсутствуют два обязательных раздела §7.1 — «риски» и «откат».** + +- Файл: `docs/specs/293-resize-pointer-noop.md` +- PROCESS.md §7.1 перечисляет обязательные разделы ТЗ: «…план автотестов · + риски · откат · release-артефакты». В документе нет ни одного явного или + скрытого упоминания риска или отката — проверено `grep -i "риск\|откат\| + rollback\|revert"` по всему файлу, совпадений ноль. +- Сценарий отказа: DoR §2.5 требует «откат: как выключить или вернуть назад» и + «риски перечислены» как обязательные пункты чек-листа перехода в «Готово к + разработке». Без этого раздела задача формально не может уйти в DoR даже при + зелёном ревью — ревьюер такого перехода не заметит, ведущему разработчику + придётся дописывать по памяти позже, когда контекст уже потерян. +- Это не абстрактная бюрократия для этой задачи: сюда стоит явно вписать хотя бы + — «откат = revert коммита, флага/миграции нет, схема не меняется» и риск + «дефект интеграционный, точная причина не установлена до реализации; + вероятна повторная итерация по пункту 1» плюс риск для touch-режима + (safety-floor уже описан в §8, но как риск-фактор не назван). +- Фикс: дописать два коротких раздела (или один совмещённый) в ТЗ. Чисто + редакторская правка, не меняет контракт. + +### Low (снимается с записью либо правится по желанию автора) + +**L1. `docs/specs/293-resize-pointer-noop.md` не добавлен в индекс `docs/specs/README.md`.** + +- Каждый предыдущий P1-спек этой же подсистемы (#277, #280, #281) имеет строку + в таблице `## P1` этого файла; #293 такой строки не получил (`git diff + origin/dev...HEAD -- docs/specs/README.md` — пусто). +- Не блокирует: README.md сам говорит, что канонический статус — в issue, а + таблица — навигационная. Но конвенция для этого трека соблюдается всеми + соседними спеками, и разрыв в ней имеет свойство накапливаться незаметно. +- Можно чинить сейчас (одна строка) либо снять с записью «добавится при + следующей правке README» — на решение автора. + +## Что проверено и признано корректным + +- **Соответствие SCOPE.md.** Дефект — в J6 («Keep the plan true… drag/resize»), + Resize — часть закрытого core job, регрессия в нём безусловно в скоупе; + никакого конфликта с out-of-scope списком нет. +- **Проблема не является догадкой.** Раздел 1 фактически проверяем: топология + фикстуры подтверждена численно (см. «Как проверялось», п.5), названные + символы кода (`_rszDrag`, `_rszMove`, `_rszApplyPreview`, `_rszPreview`) + существуют и ведут себя как описано. Утверждение «чистая математика исправна» + соответствует реальной сигнатуре `clampSafeResize/applySafeResize` в + `src/resize.ts`/`docs/RESIZE.md`. +- **Не-скоуп корректен и не расширяется.** Явно исключены изменения eligibility + (#292/#289/#290), partial-shared/diagonal/>2-комнатный resize, схема/storage, + прямая правка геометрии фикстуры тестом — всё ровно то, что подсказывает + «Не входит» issue. +- **Контракт не выдуман, а восстанавливает документированное.** Раздел 3 + (Pointer contract) и раздел 4 (Live overlay contract) переизлагают уже + зафиксированный в `docs/RESIZE.md` контракт («immutable snapshot», + «_serverCfg не меняется до pointerup», «один Undo», «Esc/pointercancel/ + lostpointercapture — без записи») — это не новая продуктовая рамка, это + починка нарушенного исполнения существующей. +- **Новая часть контракта (toast при непредсказуемом reject в разделе 2/4) не + является придуманным UX.** Она прямо запрошена вторым комментарием владельца + («ТЗ отдельно требует отличать ошибку pointer-координат от молчаливого + отката live-preview») и использует существующий механизм (`_showToast`, + каталог `toast.*`, уже применяемый в `resize.disabled.*` по `docs/RESIZE.md` + строка 25) — не новая абстракция. +- **AC проверяемы и способны провалиться.** AC1/AC2/AC5/AC8 прямо требуют + mutation-тест («мутант, перестающий обновлять `d`, обязан убивать смок») — + это ровно дисциплина «тест умеет падать», которую требует процесс. AC не + подставляют один и тот же результат под два источника: preview — из одного + `applySafeResize`-результата, commit переиспользует тот же результат + (раздел 4, согласуется с `docs/RESIZE.md` «Preview and commit»), правило + «одно число — один источник» не нарушено. +- **Touch/безопасность.** Раздел 8 согласован с `docs/TOUCH-SUPPORT.md`: + запрет коммита от pinch/pan/pointercancel буквально повторяет safety-floor + этого документа («saving unintended geometry merely because a pinch, pointer + cancellation… »). Новых HA-действий или security boundary нет. +- **i18n.** Раздел 9 корректно ставит добавление RU/EN ключей в условие («если + добавляется runtime reason»), а не отдельным решением владельца — выбор + конкретного текста тоста — техническая, не продуктовая деталь по критерию + PROCESS §7.1 («именование… решает автор»). +- **Модель данных/миграция.** Явно «не меняется» в метаданных документа — + корректно для чистого pointer-pipeline фикса. +- **Release-артефакты.** Раздел 10 корректно требует `Issue:`/`User-Visible: yes` + и оба changelog в том же коммите, закрытие только через бету — соответствует + AGENTS.md/PROCESS §2.8. + +## Чего не проверял (сознательно, вне пределов этапа spec) + +- **Не проверял, действительно ли причина инертности — screen-to-SVG + конвертация, а не что-то ещё.** ТЗ сам явно оставляет это открытым («реализация + обязана сначала зафиксировать точную причину… а затем исправить») — это + корректная позиция для этапа ТЗ, не находка. +- **Не гонял `typecheck`/`test`/`build`.** Продуктовый код не менялся (см. «Как + проверялось» п.2); эти гейты относятся к код-ревью (§2.7), не к ревью ТЗ. +- **Не запускал существующие смоки** (`demo/smoke_room_resize.mjs` и т.п.) — на + этапе ТЗ смоков для новой фикстуры ещё не существует, а регресс по AC9 + проверяется в код-ревью. +- **Отложенное наблюдение, не оформленное как находка:** формулировка раздела 5 + («отправляет browser pointer events по фактическим screen coordinates handle») + не уточняет механизм диспатча. Проверка кода показала, что `_svgPoint()` + читает только `clientX/clientY` + `getBoundingClientRect()` — эти свойства + идентичны для доверенных (`page.mouse.*`, как в issue-репро) и синтетических + (`dispatchEvent(new PointerEvent())`, как в текущем `smoke_room_resize.mjs`) + событий, поэтому у меня нет доказательства, что разница механизма диспатча + сама по себе объясняет расхождение issue-репро и текущего зелёного смока — + выдвигать это как находку значило бы выдать догадку за факт. Указываю как + риск для реализатора: если новый смок унаследует `pointer()`-хелпер + буквально, стоит явно проверить, что он падает на `dev` (pre-fix) — то есть + соблюсти дисциплину «тест умеет падать» именно на этой фикстуре, а не + полагаться на то, что похожий хелпер уже где-то используется. + +## Итог + +High: 0. Medium: 1 (M1, в скоупе — фиксируется автором тем же ТЗ без нового +issue, #202). Low: 1 (L1, на решение автора). Продуктовый контракт технически +обоснован, факты в разделе 1 проверены по коду и фикстуре, скоуп не размыт и не +расширен, AC способны провалиться. Возврат — жёлтый вердикт из-за отсутствующих +обязательных разделов §7.1.