diff --git a/docs/reviews/SPEC-REVIEW-300-r2.md b/docs/reviews/SPEC-REVIEW-300-r2.md new file mode 100644 index 00000000..baeda982 --- /dev/null +++ b/docs/reviews/SPEC-REVIEW-300-r2.md @@ -0,0 +1,196 @@ +# SPEC-REVIEW-300-r2 + +- Issue: [#300](https://github.com/Matysh/houseplan-card/issues/300) — Подписи при ресайзе: подсвечивать измеряемые стены, убрать размер перетаскиваемой, площадь показывать по бокам от неё +- Этап: spec (PROCESS.md §2.4) +- ТЗ: `docs/specs/300-resize-measurement-layout.md`, коммит `2b7c98e6bcc4a7fed920c05f2298a06a138683fd` на ветке `issue/300-resize-labels` +- Заход: r2 · блокирующих циклов израсходовано (до этого раунда) 1/4 +- Ревьюер: Claude (роль «ревьюер ТЗ», не автор) + +## Скоуп разбора + +Раунд не первый → разбор по дельте (PROCESS.md §2.9, issue #214), не заново. + +1. Вердикт r1 найден в комментарии issue #300 (2026-08-24T19:14:59Z): жёлтый, + заход r1, блокирующих циклов 1/4, High 0 / Medium 2 → в задаче. SHA, на + котором получен вердикт r1, в самом комментарии не назван — это находка, + отдельно не заводится (не Medium, не блокирует), восстановлен из + `docs/reviews/SPEC-REVIEW-300-r1.md` (шапка документа) и из тела следующего + коммита: **`ab9a83e755211a599ba8d73be0a3716696a5168b`**. +2. Автор ответил комментарием 2026-08-24T19:19:16Z: правки внесены и запушены + в **`2b7c98e6bcc4a7fed920c05f2298a06a138683fd`**, явно указана дельта для + повторного ревью: `git diff ab9a83e..2b7c98e -- docs/specs/300-resize-measurement-layout.md`. +3. Дельта проверена: `git diff ab9a83e7..2b7c98e6 -- docs/specs/300-resize-measurement-layout.md` + — правка ограничена файлом спецификации, 40 добавленных / 18 удалённых + строк. Дополнительно `git diff ab9a83e7..2b7c98e6 --stat` показывает, что + между раундами появился только сам документ `SPEC-REVIEW-300-r1.md` + (публикация предыдущего ревью) — это не продуктовый код и не меняет предмет + разбора. +4. Дельта локальна: тот же файл, тот же раздел (правки только в §3, §4 п.6, + §5, §6.3, §7, §10, §11 (AC7/AC11), §12, §13, §14, §18, заключительный + абзац §18) — не ребейз на ушедший вперёд `dev` (весь `origin/dev..HEAD` + всё ещё только markdown), не смена контракта поведения целиком, не задета + новая подсистема. Полный повторный разбор не требуется; проверяются: + закрытие M1/M2, и все AC/разделы, чьё доказательство эта дельта задевает + (AC6, AC7, AC11 и связанные с ними §6.3, §7, §12, §13, §14, §18). +5. Гейты кода неприменимы, как и в r1: класс изменений — C (документация), + диапазон `origin/dev..HEAD` не содержит `src/**`/`test/**`. + +## Как проверялось + +Проверено чтением (не исполнением), только по дельте: + +| Утверждение правки | Где проверено | +|---|---| +| Класс `.rlgearbtn` — реальное имя кнопки настроек комнаты (не `.roomgear`, как было в r1) | `src/styles.ts:1000`, `src/houseplan-card.ts:18705` (`_renderRoomGear`) | +| `demo/benchmark_safe_resize_render.mjs` действительно использует абсолютный потолок `RENDER_P95_MS = 25`, без relative-сравнения | `demo/benchmark_safe_resize_render.mjs:9,64,75` — совпадает с формулировкой нового AC11 | +| Утверждение нового §6.3/§18.4 «screen-fixed footprint `.rlgearbtn`» проверено против реального CSS кнопки | `src/styles.ts:996-1008` — `--gear-h: calc(var(--icon-size, 2.5cqw) * 0.77)`; `--icon-size` вычисляется `iconCqw()` | +| `iconCqw()` явно и намеренно **зависит от текущего zoom/viewBox**, а не постоянна в screen px | `src/space-geometry.ts:463-489` — докстринг: «An icon is a percentage of the PLAN, not of the viewport: it scales with the plan as you zoom… (owner, 2026-08-03)»; формула `(iconPct * iconUnit(space) * k) / w`, где `w = viewW` | +| Тот же вывод подтверждён комментарием в стилях | `src/styles.ts:997-998`: «icon-size already rescales with the view… so the button zooms WITH the plan instead of keeping a constant screen size (owner's spec)» | +| `_renderRoomGear` позиционирует кнопку в процентах контейнера (`left/top: %`), что синхронно с zoom — сама позиция не проблема | `src/houseplan-card.ts:18701-18703` | + +## Закрытие раунда r1 + +| Находка r1 | Чем закрыта | Где это видно | +|---|---|---| +| **M1** — временное скрытие `.roomgear` во время Resize объявлено «acceptance contract» без подтверждения владельца и без прецедента в коде | Автор не стал спрашивать владельца и не оставил догадку — убрал сам guess: кнопка теперь **всегда видима** (не меняется относительно текущего поведения, значит не требует продуктового решения), а перекрытие снимается сдвигом area-плашки вдоль стены. То, что реально новое и видимо пользователю («площадь не перекрывает видимую кнопку»), явно оставлено в контракте; конкретный алгоритм сдвига явно вынесен в §18 п.4 как оспариваемое техническое предположение («допустима другая pure screen-space стратегия») | §4 п.6, §6.3 (новый абзац «Для collision check…»), §7 («Room settings buttons продолжают рендериться и не получают нового состояния»), §18 п.4, заключительный абзац §18 («…отсутствие overlap с видимой room settings button — это acceptance contract») | +| **M2** — AC11 требовал относительный регресс-бюджет (10%/1ms), которого не считает `demo/benchmark_safe_resize_render.mjs` | AC11 переформулирован под реальный механизм инструмента — абсолютный потолок `RENDER_P95_MS = 25`; §10 теперь явно называет этот файл в списке участвующих файлов с пояснением «исходник менять не требуется»; §13 п.5 явно требует прогонять его перед код-ревью | AC11 (раздел 11), §10 («`demo/benchmark_safe_resize_render.mjs` — существующий real-render gate с абсолютным `RENDER_P95_MS = 25`»), §13 п.5 | + +Оба закрытия проверены не на слово: `.rlgearbtn` и `RENDER_P95_MS = 25` +сверены с реальным кодом (таблица выше). Оба Medium из r1 закрыты корректно. + +## Находки + +### Medium — M3: новый текст утверждает, что footprint `.rlgearbtn` «screen-fixed», хотя размер кнопки явно и намеренно зависит от zoom + +**Файл:** `docs/specs/300-resize-measurement-layout.md`, §6.3 (новый абзац «Для +collision check…») и §18 п.4 — обе фразы введены именно этой правкой (в r1 +их не было вовсе, тема отсутствовала, так как r1 просто прятал кнопку). + +**В чём проблема.** Новый механизм collision-avoidance описан так: +«консервативный **screen-space footprint** плашки, вычисленный из +форматированного текста, font/padding tokens и **screen-fixed footprint** +`.rlgearbtn`» (§6.3), и повторно в §18 п.4: «screen-fixed footprint room +gear». В документе термин «screen-fixed» уже занят и используется +последовательно в другом, точном смысле — «не меняется при zoom SVG»: то же +§6.3 про leader-линию («Перевод screen px в render units использует текущий +viewBox/stage size; stroke остаётся **screen-fixed**») и §6.2 про подсветку +(`vector-effect="non-scaling-stroke"`, «одинаково читается при zoom»). + +Реальный `.rlgearbtn` этому определению не соответствует. +`--gear-h: calc(var(--icon-size, 2.5cqw) * 0.77)`, а `--icon-size` +вычисляется `iconCqw()`, чей докстринг прямо говорит: «An icon is a +percentage of the PLAN, not of the viewport: it scales with the plan as you +zoom… (owner, 2026-08-03)» — то есть размер кнопки в CSS px на экране +**растёт и уменьшается вместе с zoom плана**, а не остаётся константой. Это +подтверждено ещё и соседним комментарием в `styles.ts`: «icon-size already +rescales with the view… so the button zooms WITH the plan instead of keeping +a constant screen size **(owner's spec)**» — то есть зависимость от zoom не +случайность реализации, а явное продуктовое решение того же владельца. + +Если реализация буквально возьмёт формулировку ТЗ («screen-fixed footprint +`.rlgearbtn`») и один раз посчитает/захардкодит размер кнопки без учёта +текущего zoom (что и подсказывает слово «screen-fixed» рядом с «не читает +layout в pointermove»), консервативный footprint будет верным только на том +zoom, для которого его посчитали, и разойдётся с реальным на любом другом — +ровно то, от чего должен защищать сам механизм (AC7: «её фактический DOM +rectangle не пересекает area-плашку»). Существующий риск-пункт 4 в §15 +(«Консервативный footprint разойдётся с фактическим CSS… smoke сравнивает +реальные `getBoundingClientRect()`») называет только font/padding-погрешность, +а не zoom-масштабирование, и план тестов (§13 п.2) не называет конкретный +нестандартный zoom как fixture — то есть смок на дефолтном zoom формально +пройдёт даже с неверной («screen-fixed») трактовкой формулы. + +**Сценарий проявления.** Администратор дома открывает Resize на плане, +предварительно отдалённом (zoomed out) от дефолтного масштаба — сценарий +никак не запрещён и не редок при работе с большими планами. `.rlgearbtn` +на экране меньше, чем при дефолтном zoom (или больше — при приближении). +Реализация, посчитавшая «screen-fixed» footprint по дефолтному размеру, +либо пропускает реальное пересечение (кнопка настроек оказывается частично +под площадью — ровно проблема 3 из тела issue, которую задача должна решить), +либо наоборot излишне отодвигает плашку туда, где реального пересечения нет. +Оба исхода — нарушение AC7 на zoom, не покрытом smoke-фикстурой. + +**Почему не High.** Правится на этом же этапе без пересмотра остального +документа: либо явно указать, что footprint `.rlgearbtn` должен пересчитываться +по той же zoom-зависимой формуле, что и `--icon-size`/`iconCqw()` (то есть +заменить «screen-fixed» на «screen-space, пересчитываемый от текущего +view.w» в двух местах — §6.3 и §18 п.4), либо явно добавить в §13 п.2 fixture +с нестандартным zoom для доказательства, что консервативный footprint +остаётся консервативным не только на дефолтном масштабе. Технический +алгоритм и так помечен в §18 п.4 как свободно оспариваемое предположение — +менять нужно только формулировку факта о самой кнопке, не продуктовый +контракт AC7. + +## Что проверено и признано корректным (в рамках дельты) + +- M1 и M2 закрыты корректно и проверены по коду, не на слово (таблица выше). +- Новое решение по M1 не создаёт нового недекларированного guess: сохранение + видимости кнопки — это отсутствие изменения существующего поведения, а не + новое решение, требующее подтверждения владельца; конкретный алгоритм сдвига + корректно оформлен как оспариваемое предположение в §18 п.4, а сам факт + «кнопка видима и не перекрыта» верно зафиксирован как acceptance contract. +- AC7 переформулирован непротиворечиво: «unit + smoke» вместо старого + «smoke», согласуется с тем, что появился чистый collision-helper, который + можно проверить unit-тестом отдельно от production DOM-пути. +- Переименование mutation guard `resize-labels-gear-during-drag` → + `resize-labels-ignore-gear-collision` (§12) соответствует новому механизму + (сдвиг вместо скрытия) и продолжает указывать на AC7. +- §13 п.5 корректно добавляет прогон `demo/benchmark_safe_resize_render.mjs` + как обязательное доказательство AC11 перед код-ревью — согласуется с новой + формулировкой AC11. +- §7 («Area projection использует тот же вычисленный visual centre, что + `_renderRoomGear()`») — корректное техническое требование по *позиции* + (совпадает с `poleOfInaccessibility(r.poly)` в `_renderRoomGear`); в отличие + от заявления о *размере* (M3), про позицию текст не грешит против кода. +- Риск-пункт 4 в §15 добавлен обоснованно (несовпадение расчётного и + фактического CSS — реальный риск), но не покрывает конкретно + zoom-масштабирование — см. M3. + +## Чего не проверял + +- Полные гейты (`typecheck`/`test`/`build`, `check-docs`, `smoke-select`, + golden, invariants, backend, `python -m pytest`) — не запускались: диапазон + `origin/dev..HEAD` содержит только markdown, класса A/B изменений нет, + как и в r1. Эта проверка относится к будущему код-ревью того же issue. +- Реализацию проекции (`src/resize-labels.ts` и т.д.) — она ещё не написана. +- Прочие AC (AC1–AC5, AC8–AC10) и разделы §1, §2, §6.1, §6.2, §8, §9, §16, + §17, §18 п.1–3,5 — не затронуты дельтой, повторно не проверялись, см. + «Унаследовано из r1». +- Численно, действительно ли `--icon-size`/`iconCqw()` в реализации даёт + «консервативный» (заведомо не меньше фактического) footprint при любом + zoom, если формула будет исправлена по M3 — это вопрос реализации, не + наблюдаем на этапе ТЗ. + +## Унаследовано из r1 + +Всё, что не затронуто дельтой между `ab9a83e7` и `2b7c98e6`, принято без +повторной проверки на основании `docs/reviews/SPEC-REVIEW-300-r1.md` +(SHA `ab9a83e755211a599ba8d73be0a3716696a5168b`), раздел «Что проверено и +признано корректным»: + +- обязательные разделы §7.1 присутствуют и не перепутаны местами; сценарий + и «что человек увидит» — первые два раздела; +- диагноз текущего кода (три длины, `poleOfInaccessibility` для площади, + отсутствие отдельной подсветки измеряемого ребра) подтверждён чтением кода; +- технический контракт §6 согласован с реальной формой `SafeResizePlan` + (`roomIds`, `edgeByRoom` в `src/resize.ts`); +- продуктовое решение по Q1 (narrow-room fallback — площадь всегда видна, + выносится за контур с leader-линией) верно перенесено из решения владельца + дословно; +- граница с #233 (математика длины/площади) и #277 (eligibility/commit/Undo) + проведена верно и не переоткрывает их; +- модель данных/миграция/i18n корректно поданы как «без изменений»; +- AC1–AC5, AC8–AC10 однозначны, у каждого указан способ доказательства; + golden-сцена `safe-resize-handles-clamp-{light,dark}` действительно + эмулирует активный preview (`safeResizePreview: true`); +- откат описан верно (одна frontend-ревизия, без миграции); +- смежные issue (#233/#277/#238/#52) корректно отнесены к «не входит». + +## Вердикт + +Одно новое Medium (M3) в скоупе задачи, устранимо без пересмотра остального +документа — правкой формулировки в двух местах (§6.3, §18 п.4) либо +добавлением non-default-zoom fixture в §13 п.2. High нет. Оба Medium +предыдущего раунда (M1, M2) закрыты корректно и проверены по коду. + +Вердикт: жёлтый · заход r2 · блокирующих циклов 2/4 · High: 0 · Medium: 1 → в задаче · Документ: docs/reviews/SPEC-REVIEW-300-r2.md