diff --git a/docs/reviews/SPEC-REVIEW-300-r1.md b/docs/reviews/SPEC-REVIEW-300-r1.md new file mode 100644 index 00000000..25d78427 --- /dev/null +++ b/docs/reviews/SPEC-REVIEW-300-r1.md @@ -0,0 +1,196 @@ +# SPEC-REVIEW-300-r1 + +- Issue: [#300](https://github.com/Matysh/houseplan-card/issues/300) — Подписи при ресайзе: подсвечивать измеряемые стены, убрать размер перетаскиваемой, площадь показывать по бокам от неё +- Этап: spec (PROCESS.md §2.4) +- ТЗ: `docs/specs/300-resize-measurement-layout.md`, коммит `ab9a83e755211a599ba8d73be0a3716696a5168b` на ветке `issue/300-resize-labels` +- Заход: r1 · блокирующих циклов израсходовано (до этого раунда) 0/4 +- Трек: обычный (аналитика прямо называет `трек: обычный`; меток `small`/`trivial` на issue нет) +- Ревьюер: Claude (роль «ревьюер ТЗ», не автор) + +## Скоуп разбора + +Первый раунд — разбор полный, по PROCESS.md §2.10 сокращение объёма не +применяется. Прочитаны в порядке из инструкции: `docs/SCOPE.md`, `AGENTS.md`, +`PROCESS.md`, тело issue #300 и все 5 комментариев (аналитика, занятие, вопрос +Q1, решение владельца по Q1, публикация ТЗ), `docs/USER-GUIDE.ru.md` (раздел +Resize), канонические `docs/RESIZE.md`, `docs/UX-MODES.md`, а также связанные +специи `docs/specs/233-resize-inner-dimensions.md` и +`docs/specs/277-safe-resize.md`. + +Диагноз ТЗ по коду проверен построчно, а не принят на слово: прочитан +`_rszEdgeLabels()`, `_rszMove()`, `_rszEdgeDown()`, `_renderRoomGear()` и тип +`SafeResizePlan` в `src/houseplan-card.ts` / `src/resize.ts`, а также +существующие golden-сцены `safe-resize-handles-clamp-{light,dark}` в +`demo/golden/matrix.mjs` и бенчмарк `demo/benchmark_safe_resize_render.mjs`. + +Ветка на момент ревью содержит ровно один коммит поверх `origin/dev` +(`git diff origin/dev..HEAD` = только новый файл ТЗ и правка +`docs/specs/README.md`) — ребейз не требовался, `dev` не ушёл вперёд. + +## Как проверялось + +Гейты кода в этом раунде неприменимы: класс изменений — только C +(документация), продуктовый код не тронут. Прогон `typecheck`/`test`/`build` +не требуется — ветка не содержит правок `src/**`/`test/**`. Единственная +проверка этапа — соответствие ТЗ формату §7.1 и фактическому состоянию кода, +на которое ссылается диагноз. + +Проверено чтением (не исполнением): + +| Утверждение ТЗ | Где проверено | +|---|---| +| `_rszEdgeLabels()` кладёт три длины: previous/moving/next | `src/houseplan-card.ts:8739-8784`, цикл `for (const edge of [(i-1+n)%n, i, j])` | +| Площадь ставится в `poleOfInaccessibility(floor)` | `src/houseplan-card.ts:8776` | +| `.roomgear`-кнопка тоже стоит в `poleOfInaccessibility(r.poly)` — заявленное перекрытие реально | `src/houseplan-card.ts:18688-18711` (`_renderRoomGear`) | +| `SafeResizePlan` содержит `roomIds`/`edgeByRoom` | `src/resize.ts:65-72` | +| Существующий 12px-сдвиг у подписи размера проёма (принятое предположение §18.2) | `src/styles.ts:1227-1233`, `.opdimension` использует `-12px` | +| golden-сцена `safe-resize-handles-clamp-{light,dark}` уже симулирует активный preview (`safeResizePreview: true`) | `demo/golden/matrix.mjs:246-250` | +| `demo/benchmark_safe_resize_render.mjs` меряет только абсолютный потолок `RENDER_P95_MS=25`, без сравнения с историческим baseline | `demo/benchmark_safe_resize_render.mjs:8-9,58-64` | +| Ни один существующий путь рендера не скрывает `.roomgear` или другую интерактивную кнопку во время активного жеста (drag/draw/move) | `grep` по `_roomDrag`/`_moveDrag`/`_drawDrag`/`hide.*drag` в `src/houseplan-card.ts` — 0 совпадений | +| `docs/UX-MODES.md` и `docs/USER-GUIDE.ru.md` не описывают исчезновение кнопки настроек комнаты во время Resize | текстовый поиск по обоим файлам | + +## Находки + +### Medium — M1: временное скрытие кнопки настроек комнаты — недекларированная догадка, выданная за принятое решение + +**Файл:** `docs/specs/300-resize-measurement-layout.md`, раздел 4 п.6 и раздел 18 +(последний абзац). + +**В чём проблема.** П.6 раздела 4 «Зафиксированные продуктовые решения» +гласит: «Во время активного Resize кнопки настроек комнат временно скрыты». +Раздел 18 явно исключает этот пункт из списка предположений: «Не являются +предположениями: … и **временное отсутствие room gear** — это acceptance +contract». То есть автор фиксирует новое видимое поведение — интерактивная +кнопка пропадает с экрана во время жеста — как решённый факт, не как +предположение и не как продуктовый вопрос владельцу. + +Между тем: + +- в теле issue и во всех пяти комментариях (включая единственный заданный + владельцу вопрос Q1 и его решение) кнопка настроек комнаты не упоминается + вовсе — только требование «плашка площади не пересекается с кнопкой» (AC4 + issue, AC7 ТЗ); +- ни `docs/RESIZE.md`, ни `docs/UX-MODES.md`, ни `docs/USER-GUIDE.ru.md` не + фиксируют исчезновение `.roomgear` во время какого-либо жеста; +- в коде нет прецедента: ни один существующий drag (move комнаты, draw стены, + furniture-drag) не скрывает интерактивные кнопки на время жеста — это будет + первый такой случай. + +Согласие/несогласие «не пересекается» можно было бы решить и иначе (например, +подвинуть саму плашку так, чтобы она физически не доставала до центра комнаты +в обычных пропорциях, либо явно спросить владельца форматом Q1 — «скрывать +кнопку на время жеста или…», с default). Автор выбрал одно конкретное решение +и записал его как неоспоримый контракт, а не как оспариваемое предположение +(§7.1 ТЗ прямо предусматривает для этого блок «принято предположительно, +поменять свободно» — сюда это решение не попало). + +Это ровно тот класс дефекта, о котором прямо предупреждает PROCESS.md §7.1: +«Догадка, записанная как факт, — худший вид дефекта: она проходит ревью, потому +что выглядит решением» — здесь она к тому же явно помечена как «не +предположение», то есть застрахована от последующего оспаривания на код-ревью. + +**Сценарий проявления.** Администратор дома тянет стену комнаты; в этот момент +кнопка «⚙ Настройки» соседней (или той же) комнаты пропадает с экрана без +предупреждения и без документированного контракта — если владелец на самом +деле ожидал что-то другое (например, кнопку, отодвинутую в сторону, а не +скрытую), это выяснится только после того, как код и AC7/mutation-guard +`resize-labels-gear-during-drag` уже реализованы вокруг скрытия. + +**Почему не High.** Правится на этом же этапе без переписывания остального +ТЗ: либо явное подтверждение владельца батч-вопросом с default (по образцу +Q1), либо перенос пункта в раздел 18 как явно оспариваемое техническое +предположение с обоснованием, почему скрытие — единственный практичный вариант +и почему это не поменяет продуктовый контракт. + +### Medium — M2: AC11 требует относительный регресс-бюджет, которого не существует у названного инструмента + +**Файл:** `docs/specs/300-resize-measurement-layout.md`, раздел 11 (AC11) и +раздел 10 (список файлов). + +**В чём проблема.** AC11 требует: «`benchmark_safe_resize_render` не +регрессирует больше чем на 10% либо 1 ms p95 (берётся больший допуск)», +доказательство — «benchmark + code review». Прочитанный +`demo/benchmark_safe_resize_render.mjs` не считает такую величину: он меряет +20 warm-сэмплов и сравнивает p95 с единственным **абсолютным** потолком +`RENDER_P95_MS = 25` (`Object.assign`-объект `budgets: { renderP95Ms: +RENDER_P95_MS, … }`). Никакого сохранённого исторического baseline или +same-run сравнения «до/после» в этом файле нет — в отличие от соседнего +`demo/benchmark_safe_resize.mjs`, который действительно считает `baseline` +и `relativeLimit` для *другого* сценария (курсор pointer, не render layer). + +Раздел 10 «Изменяемые файлы и модули» при этом не называет +`demo/benchmark_safe_resize_render.mjs` в списке ожидаемых правок — то есть +ТЗ не проговаривает, что этот файл придётся переписывать, чтобы у AC11 вообще +появился механизм сравнения «регресс не больше 10%». Как AC сформулирован +сейчас, его не с чем сверить: либо метрика придумана без учёта реального +инструмента, либо реализация должна тихо добавить в бенчмарк baseline-логику, +которую ТЗ не анонсирует. + +**Сценарий проявления.** На код-ревью разработчик и ревьюер по-разному прочитают +AC11: один добавит baseline-сравнение в бенчмарк (незапланированная работа вне +раздела 10), другой просто проверит, что p95 остаётся в старых 25 ms, и +формально АC11 «не регрессирует более чем на 10%/1мс» не проверен никем, +потому что делать не с чем сравнивать. + +**Почему не High.** Легко устраняется формулировкой на этом же этапе: либо +«остаётся в пределах существующего абсолютного бюджета 25 ms p95» +(соответствует реальному инструменту), либо явно добавить +`demo/benchmark_safe_resize_render.mjs` в раздел 10 с описанием, что baseline +записывается тем же способом, что в `benchmark_safe_resize.mjs`. + +## Что проверено и признано корректным + +- Обязательные разделы §7.1 (сценарий, что видит человек, проблема, скоуп/не- + скоуп, контракт, UX, модель данных/миграция/i18n, AC с доказательством, план + автотестов, риски, откат, release-артефакты) присутствуют и не перепутаны + местами; сценарий и «что человек увидит» — первые два раздела, как требует + процесс. +- Диагноз текущего кода (три длины, `poleOfInaccessibility` для площади и + gear, отсутствие отдельной подсветки измеряемого ребра) подтверждён чтением + кода — не догадка. +- Технический контракт §6 согласован с реальной формой `SafeResizePlan` + (`roomIds`, `edgeByRoom` существуют в `src/resize.ts`). +- Продуктовое решение по Q1 (narrow-room fallback — площадь всегда видна, + выносится за контур с leader-линией) верно перенесено из решения владельца + дословно, без искажения; помечено, что заменяет исходный AC6 issue, с + ссылкой на комментарий. +- Не входит математика длины/площади (#233) и eligibility/commit/Undo (#277) — + граница со смежными контрактами проведена верно и не переоткрывает их. +- Модель данных/миграция/i18n корректно поданы как «без изменений»: новых + config-полей, WebSocket-вызовов и i18n-ключей нет, что соответствует объёму + задачи (только layout существующего оверлея). +- AC1–AC10 однозначны, у каждого указан способ доказательства (unit/smoke/ + golden/code review), и golden-сцена `safe-resize-handles-clamp-{light,dark}` + действительно уже эмулирует активный preview (`safeResizePreview: true`), + так что план «переиспользовать существующую сцену» для AC9 реалистичен. +- Раздел 18 «Принятые предположения» корректно оформлен как оспариваемый и + содержит только действительно техническую разметку (расположение файла, + величина 12px, порядок отрисовки leader) — кроме пункта, вынесенного в M1, + который туда не попал, хотя по характеру должен был. +- Откат описан верно (одна frontend-ревизия, без миграции). +- Не входит в скоуп задачи и не относится к #300 никаких признаков смешения + с чужими issue — упомянутые #233/#277/#238/#52 корректно отнесены к «не + входит». + +## Чего не проверял + +- Полные гейты (`typecheck`/`test`/`build`, `check-docs`, `smoke-select`, + golden, invariants, backend) — не запускались, так как класса A/B изменений + в ветке нет: диапазон `origin/dev..HEAD` содержит только markdown. Проверка + гейтов на этом этапе относится к будущему код-ревью того же issue. +- Реализацию проекции (`src/resize-labels.ts` и т.д.) — она ещё не написана, + это предмет `S6-in-progress`. +- Численную величину 25ms/10%/1ms как перф-бюджет по существу (сколько + реально стоит рендер двух highlight-полосок и двух area-плашек) — на этом + этапе это не наблюдаемо, будет видно в реализации; отмечен только + методологический разрыв AC11 (M2). +- Доступность (`aria-hidden`, focus-order) заявленного measurement-layer — не + верифицируема без DOM; раздел 8 её обещает, содержательных противоречий не + найдено при чтении. + +## Вердикт + +Оба blocking-замечания в скоупе задачи и устранимы без пересмотра остального +документа. High нет. + +Вердикт: жёлтый · заход r1 · блокирующих циклов 1/4 · High: 0 · Medium: 2 → в задаче · Документ: docs/reviews/SPEC-REVIEW-300-r1.md