diff --git a/docs/reviews/SPEC-REVIEW-583-r1.md b/docs/reviews/SPEC-REVIEW-583-r1.md new file mode 100644 index 00000000..d7856266 --- /dev/null +++ b/docs/reviews/SPEC-REVIEW-583-r1.md @@ -0,0 +1,177 @@ +# SPEC-REVIEW-583-r1 + +**Issue:** [#583](https://github.com/Matysh/houseplan-card/issues/583) — «2.5D после #570: лишние тени и обводки, перекрытие двери стеной, смещение иконок» +**Этап:** ТЗ на ревью (PROCESS.md §2.4), трек — полный (сложность/риск 8/10, обосновано в комментарии «Аналитика») +**Заход:** r1 · блокирующих циклов израсходовано 0 из 4 +**Ревьюер:** независимая сессия, без контекста реализации; автор ТЗ не является ревьюером + +## Скоуп ревью + +Материал — раздел `## ТЗ` в теле issue #583 (редакция после согласования Q1–Q5, +зафиксированного владельцем в комментарии от 2026-09-15T18:10:30Z). Задача +корректирует четыре presentation-дефекта скрытого 2.5D-режима (`#89`/`#570`): +контактные/падающие тени, обводка полотен door/gate, «вросшая» в стену дверь, +взаимные наложения device/lock overlay после проекции. Явно вне скоупа: Flat, +редакторы, `houseplan-space-card`, геометрическая модель, Glow/sun, размеры +иконок/touch target, публичные настройки. + +## Как проверялось + +1. Прочитаны `docs/SCOPE.md`, `AGENTS.md`, `PROCESS.md` §2.4/§2.9/§2.10/§7.1/§7.2. +2. Прочитано тело issue #583 целиком и все 4 комментария: аналитика, пакет + вопросов Q1–Q5, ответ с уточнениями, ратификация ответов владельцем. +3. Прочитан канонический документ подсистемы `docs/ISOMETRIC.md` (Stage 1/2/4) + и `docs/TOUCH-SUPPORT.md` (44×44 px инвариант). +4. Каждое техническое утверждение ТЗ и комментария «Аналитика» сверено с кодом + на текущем `dev`-состоянии (в рабочей копии на `5867ee02`, где Stage 4 из + #570 уже слит), чтобы отличить обоснованное решение от догадки, выданной за + факт: + - `iso-contact-shadow`/`geometry.contactPath` и `iso-leaf-shadow`/`panel.shadowD` + — подтверждено в `src/iso-walls.ts:29,163` и `src/iso-scene-render.ts:1135-1190`; + - общий `stroke` у `.iso-opening-panel` и отдельный у + `.iso-material-matte-leaf` — подтверждено в `src/styles/plan.styles.ts:329-348`; + классы `leaf-front/leaf-back/leaf-edge/leaf-top` существуют в + `src/iso-openings.ts:139-142,430-445` — терминология AC2/§6.2 не изобретена; + - единая `buildIsoWallDepthQueue()` для стен и створок — `src/iso-scene-render.ts:86,1214`; + - `resolveIsoOverlayPlacement()` в `src/iso-overlays.ts` на сегодня решает + коллизию каждого overlay только со стенами (`isNear`/`collidesAt` против + `wallSilhouettes`), взаимной коллизии device↔device/lock нет — подтверждает + центральную предпосылку задачи (п.4 «Аналитики» и контракт §6.3); + - `ISO_OVERLAY_MAX_NUDGE_CSS_PX = 48` и `ISO_OVERLAY_SAFETY_GAP_CSS_PX = 4` + — существующие константы (`src/iso-overlays.ts:11-12`), ТЗ не поднимает + бюджет, а требует уместить в нём же общий (wall-avoidance + mutual) сдвиг; + - `ISO_OVERLAY_PLACEMENT_CACHE_LIMIT` и сигнатура кэша, завязанная на + `wallSilhouettes`/`unitsPerPixel` — существуют (`src/iso-scene-render.ts:730-745`), + подтверждает выполнимость требования §6.4 о расширении сигнатуры, а не + изобретение нового механизма с нуля; + - существующие golden-фикстуры (`demo/golden/baselines/isometric-*`) не + содержат сцены класса «диагональная стена + дверь у угла», описанного в + issue — то есть AC13 требует построить новый обобщённый (не «списанный» с + реального этажа) фикстур; это инженерная работа автора, а не пробел ТЗ, + и §4/§7 explicit запрещают координатные/ID-исключения по этому этажу. +5. Проверено соответствие обязательным разделам §7.1: Сценарий, Что человек + увидит до/после, Проблема, Скоуп/Не-скоуп, Контракт поведения, UX и + доступность, Модель данных и миграция, i18n, AC1…AC14 с доказательством, + План автотестов, Риски, Откат, Release-артефакты — присутствуют все. +6. Проверено происхождение продуктовых решений: пакет вопросов Q1–Q5 задан + владельцем (`Matysh`, `OWNER`) с default по каждому; развёрнутый ответ дал + `singlmolt-prog` (`authorAssociation: NONE`), но следующим комментарием + владелец прямо ратифицировал именно эти уточнения как продуктовые решения + («Ответы из последнего комментария приняты владельцем в текущей сессии»). + Формально вопрос-ответ происходит не в одном аккаунте, но решение явно + принято владельцем в issue — это не находка, а зафиксированное решение по + протоколу §7.1 («владелец отвечает... пачкой, с default»). +7. Проверена согласованность с `docs/SCOPE.md`: задача не расширяет узкое + исключение #89 (то же J1/J2/J3-состояние, геометрия и действия, без новой + камеры/модели/сохранённых derived-координат) — контракт §6.3 первым пунктом + фиксирует, что canonical floor anchor остаётся единственным источником + позиции и что 2.5D-коррекция не пишется в layout/config. + +Для этапа ТЗ дешёвые/тяжёлые гейты (typecheck/test/build/smoke/golden) не +запускались — материал ревью текстовый, продуктовый код не менялся и не +существует для этой задачи; чтение кода использовалось только для проверки +достоверности утверждений ТЗ, а не как гейт. + +## Находки + +Блокирующих (High) находок нет. Находок Medium в скоупе или вне скоупа нет. + +### Low — 1, снята решением ревьюера с записью + +**L1. В разделе «Что человек увидит до и после» (§2 ТЗ) продуктовое описание +использует единицы реализации.** Фраза «...детерминированно разводятся в +пределах своей комнаты и **48 CSS px** от исходной **проекции**» смешивает +implementation-термины (CSS px, проекция) с предназначенным для не-технического +изложением разделом — §7.1 требует именно «без терминов реализации» для этого +абзаца. + +*Воспроизведение:* тело issue, раздел `## ТЗ` → `### 2. Что человек увидит до и +после`, последнее предложение. + +*Решение ревьюера:* не возвращаю в работу. Смысл предложения не теряется для +читателя (это число уже фигурирует как согласованный продуктовый лимит в +контракте §6.3 и в ответе автора на Q3), критерии приёмки от этой фразы не +зависят, а полноценно нейтральная формулировка («устройства и замки не +накладываются друг на друга и остаются рядом со своим исходным местом на +плане») ничего не добавляет к проверяемости. Фиксирую как принятое отклонение, +менять ТЗ не требую. + +## Что проверено и корректно + +- Все 14 AC пронумерованы, для каждого указан способ доказательства (unit / + browser smoke / Linux golden / performance artifact), формулировки допускают + однозначный pass/fail (пороговые числа: 48 CSS px, 44×44 CSS px, конкретные + классы состояний door/gate: closed/0.5/open × horizontal/vertical/diagonal × + face/flip). +- Контракт §6.1–6.4 без противоречий покрывает все 4 симптома issue; каждый + пункт контракта отображён минимум в одном AC (проверено построчным + сопоставлением, таблица не прикладывается — соответствие прямое и не + требует отдельной визуализации). +- Технические факты в «Аналитике» и в контракте (источники теней, общий + `stroke`, единая depth queue, отсутствие взаимной коллизии между overlay, + существующий бюджет 48 px, существующая структура кэша) подтверждены прямым + чтением исходников — ни один не оказался догадкой, выданной за факт. +- Продуктовая неоднозначность закрыта: 5 вопросов (тени/участники + коллизии/поведение при нехватке места/приоритет/место снятия обводки) + заданы одним пакетом с default, ответ принят и ратифицирован владельцем; + открытых продуктовых вопросов не осталось. +- Не-скоуп (§5) корректно исключает геометрическую модель, Flat, редакторы, + `houseplan-space-card`, Glow/sun, размеры/touch-target, публичные настройки, + что соответствует узкому исключению #89 в `docs/SCOPE.md` — задача не + расширяет продуктовый скоуп 2.5D-эксперимента. +- «Принятые технические предположения» (§15) корректно выделены отдельным + явно помеченным блоком и не содержат продуктовых решений — это именно то, + что процесс разрешает автору решать самостоятельно (§7.1: «где хранится + состояние… решение записывается явным блоком»). +- i18n, модель данных/миграция, откат, release-артефакты — по одному + предложению каждый, все корректно отвечают «нет изменений» с обоснованием + (не мигрирует ничего, потому что меняется только derived presentation). +- Порядок операций в group-pass (сначала существующая защита от стен, затем + общий детерминированный проход) и правило приоритета (меньшее отклонение + выигрывает, tie-break — стабильный ключ) сформулированы без пробелов, + допускающих два взаимоисключающих прочтения. + +## Чего не проверял + +- Не запускал никакие автотесты/гейты — на этапе ТЗ продуктового кода для + задачи не существует, снимать нечего. +- Не оценивал сложность/трудозатраты реализации предложенного group-collision + resolver — это инженерная деталь, прямо оставленная автору (§15.1). +- Не проверял, действительно ли предложенный bounded search влезет в + существующий perf-бюджет `large-house-isometric`/`isometric-stage3-dense` — + это заявлено как критерий приёмки (AC11) и будет доказано на этапе + код-ревью performance-артефактом, а не на этапе ТЗ. +- Не выносил самостоятельного мнения о том, не стоило ли разбить четыре + симптома на отдельные issue — граница объёма issue решена владельцем + (P2/полный трек уже зафиксированы в «Аналитике»), а не предмет ревью ТЗ. + +## Вердикт + +**Зелёный.** ТЗ полное по §7.1, все AC проверяемы и однозначны, продуктовая +неоднозначность закрыта и ратифицирована владельцем, технические утверждения +подтверждены чтением кода, а не приняты на веру. Единственная находка — Low, +снята ревьюером без возврата автору. + +--- + +## Материал раунда + +- Issue: #583 +- Ревью проведено на тексте раздела `## ТЗ` тела issue после комментария + владельца от 2026-09-15T18:10:30Z (ратификация Q1–Q5); более поздних правок + ТЗ на момент ревью не зафиксировано. +- Рабочая копия репозитория для проверки технических утверждений: `5867ee02b66a3e442dfd6b3eebd67d554ab8f08f`. + +--- + + + +## Материал раунда + +- Ветка: `dev`, коммит `5867ee02b66a` — ребейз его осиротит, и это нормально: ниже якоря, которые ребейз не меняет. +- Дерево материала: `3591c72c8d60958e629e11b702472d0169e174ab` + ``` + git log --all --format='%H %T' | grep 3591c72c8d60 + ``` +- Тело issue: `5c6f59d62afacaf36715df788207cc8b3df775aab0991091d0d0ae322f9ac844` +- Вердикт конвейера: `green` · High 0