18 KiB
SPEC-REVIEW-300-r1
- Issue: #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