diff --git a/docs/reviews/SPEC-REVIEW-231-r1.md b/docs/reviews/SPEC-REVIEW-231-r1.md new file mode 100644 index 00000000..c0636b5d --- /dev/null +++ b/docs/reviews/SPEC-REVIEW-231-r1.md @@ -0,0 +1,192 @@ +# SPEC-REVIEW-231-r1 + +- **Issue:** #231 — декоративный слой виден поверх заливок комнат +- **ТЗ:** `docs/specs/231-decor-layer-order.md` @ commit `e023adb` (dev) +- **Заход:** r1 · блокирующих циклов израсходовано 0 из 4 +- **Вердикт:** жёлтый · High: 0 · Medium: 1 (в скоупе, чинится в этом ТЗ) + +## Скоуп ревью + +Первый заход, дельты нет — разбор ТЗ целиком: продуктовая рамка (`docs/SCOPE.md`), +обязательные разделы §7.1 PROCESS.md, однозначность и доказуемость каждого AC, +отсутствие догадок, выданных за решение, соответствие терминологии +`docs/USER-GUIDE.ru.md`, согласованность с канонической документацией +подсистемы (`docs/BACKDROP.md`, `docs/DECOR-EDITOR.md`, `docs/CANVAS.md`, +`docs/SUN.md`) и с фактическим кодом. + +## Как проверялось + +1. Прочитаны `docs/SCOPE.md`, `AGENTS.md`, `PROCESS.md` §2.4/§7.1/§7.2/§2.9/§2.10. +2. Прочитано тело issue #231 и все 6 комментариев: два уточнения владельца по + целевой позиции слоя, аналитика, вопрос Q1 с default, решение владельца по + Q1, финальный хендофф со ссылкой на ТЗ. Открытых продуктовых вопросов на + момент ревью нет, `blocked` снят. +3. Прочитано ТЗ `docs/specs/231-decor-layer-order.md` целиком. +4. Сверено с фактическим кодом `src/houseplan-card.ts`: + - подтверждён текущий порядок вызова `_renderDecorLayer()` (строка 16584) — + он действительно предшествует рендеру заливок комнат (цикл с 16596), + hover-заливке (16726), обычным тоннелям (16727), Glow-base комнат (16728), + Glow-base тоннелей (16729), `_renderGlowLayer` (16730) и `_renderSunRays` + (16731); стены (16747), символы проёмов (16763) и `devlayer` (16794) идут + ещё позже. Абсолютные номера строк в теле issue (`:16042…:16238`) + разошлись с HEAD (там сейчас `:16584…`), но порядок элементов, который + единственно имеет значение для ТЗ, подтверждается независимо чтением кода + — не является дефектом ТЗ; + - целевая позиция ТЗ (между обоими проходами тоннелей и `_renderGlowLayer`) + соответствует названным строкам 16729/16730; + - проверено, что `.decorlayer .dshape { pointer-events: none; }` + (`src/styles.ts:1235`) и включается только в `.stage.mode-decor` — + утверждение ТЗ §9/§13 «decor не перехватывает tap/click вне редактора + декора» верно независимо от DOM-позиции слоя, не является догадкой; + - проверено `.glow-pools-frame/.glow-pools/.glow-spot { isolation: isolate }` + (`src/styles.ts:678`) — комментарий у кода прямо говорит, что этот блок + изолирует screen-blend от «room data fill, Glow base, paper and + backdrop»; ТЗ §8.2 корректно описывает это как риск и явно требует не + переносить decor молча, если целевая fixture покажет иной результат — + не догадка, а обозначенное предположение (§19.2) с открытым эскейпом; + - подтверждено, что `houseplan-space-card` не рисует decor вообще + (`grep decor src/space-render.ts` — одно упоминание, к теме не относится); + - подтверждено, что изометрический слой (`iso-shadows-svg`/`iso-walls-svg`, + 16774–16782) — отдельные `` после закрытия основного плана, поэтому + перестановка decor внутри основного SVG не меняет его позицию относительно + 3D-стен; риск в таблице §16 корректен. +5. Проверено `docs/BACKDROP.md` — «Layer order» там даёт грубую группировку + («sun / walls / openings / rooms / decor» одним пунктом), которая не + различает internal order и в этом и есть источник бага; ТЗ верно ставит её + актуализацию в release-артефакты (§18). +6. Проверено `docs/DECOR-EDITOR.md` — контракта видимого порядка в View там нет + вообще (только editor-opacity), обновление не требуется, ТЗ не настаивает + («при необходимости») — согласовано. +7. Проверено `docs/CANVAS.md` («who owns the pointer», «Architectural connection + overlay») и `docs/SUN.md` (four-phase background) — оба документа говорят о + decor на уровне, который порядок внутри плана не меняет; актуализация не + нужна, ТЗ их не трогает — корректно. +8. Проверена регистрация ТЗ в `docs/specs/README.md:71` — присутствует. +9. Проверены существующие смоки: `demo/smoke_decor.mjs`, `demo/smoke_decor_text.mjs`, + `demo/smoke_hide_layers.mjs`, `demo/smoke_backdrop.mjs`, `demo/smoke_glow.mjs` + (`hoverLayerOrder`, строки 244–261) — ни один не проверяет порядок decor + относительно hover-заливки комнаты (см. находку ниже). +10. Прогон гейтов для ревью спецификации не требуется (диапазон правок — + документация ТЗ и issue, продуктовый код не менялся); `git diff --check` + и `node scripts/check-docs.mjs --external`, упомянутые автором в хендоффе, + достаточны для этого этапа и не перепроверялись повторно — они не относятся + к предмету ревью (корректность и доказуемость контракта), а к гигиене + коммита ТЗ. + +## Находки + +### Medium (в скоупе — чинится в этом же ТЗ) + +**M1. Нормативная позиция decor относительно room hover fill не имеет +привязанного AC/доказательства.** + +- **Где:** `docs/specs/231-decor-layer-order.md` §8.1 (нормативный порядок + слоёв, пункт «room hover fill» между room fills и opening tunnels) и §14 + (AC1–AC8). +- **Суть:** §8.1 явно требует: «любой `[data-hp="decor"]` следует в DOM после + room fill, **hover fill**, обоих видов тоннелей и Glow-base» — hover-заливка + комнаты прямо включена в обязательный нижний контур. Однако ни один AC этого + не проверяет: + - AC1 говорит про «room fill» (заливку), не про hover; + - AC2 перечисляет ровно три вещи, после которых должен идти decor — + «обычный тоннель, Glow-base комнаты и Glow-base тоннель» — hover fill в + списке нет; + - AC3 описывает верхнюю границу (Glow/солнце/стены/символы/устройства), тоже + не про hover. + Значит Goal 5 из §5 («Защитить порядок… тестом, который падает при возврате + старого расположения слоя») в этой конкретной паре (decor vs hover fill) не + реализуется ни одним названным доказательством. Существующий + `demo/smoke_glow.mjs` уже умеет сравнивать DOM-позицию (`hoverLayerOrder`, + строка 260: `hoverFillLayer.compareDocumentPosition(glowLayer)`), но + `decorlayer` в нём не упоминается вовсе — расширить эту же проверку стоит + недорого. +- **Сценарий отказа:** реализация вставляет decor куда-то между room fill и + Glow-base (что удовлетворяет AC1/AC2/AC3 буква в букву — «после обычного + тоннеля» и т.д. формально не нарушены только если decor вставлен после + тоннелей, но ничего не мешает случайно вставить decor **до** hover fill, + например прямо перед строкой рендера hover — тоннели и Glow-base всё ещё идут + после него по факту кода, так что AC2 останется зелёным, а на реальном плане + при наведении на комнату с decor внутри hover-подсветка ляжет поверх + декоративной линии/фигуры, а не под неё, что расходится с §8.1 и остаётся + недоказанным дефектом. +- **Почему не High:** это пробел в покрытии доказательства, а не неверное + продуктовое решение — сам порядок (decor после hover fill) уже прямо задан + диаграммой владельца в теле issue (позиции `:16202` до целевой `:16205→:16206`) + и корректно перенесён в §8.1; чинится добавлением одной фразы в AC2 (или + отдельного AC) и одной строки в план тестов (§15.1/§15.2), без обращения к + владельцу. +- **Что сделать:** явно включить hover fill в формулировку AC2 (например: + «…после hover fill, обычного тоннеля, Glow-base комнаты и Glow-base + тоннеля…») и добавить в §15.2/§15.1 конкретное доказательство — расширение + `smoke_glow.mjs`/`hoverLayerOrder` или отдельный targeted-assert на + `compareDocumentPosition` между `.room-hover-fill-layer` и `.decorlayer`. + +## Что проверено и корректно + +- Продуктовая рамка: сценарий, персона, «что человек увидит до/после» — + присутствуют, соответствуют `docs/SCOPE.md`/аналитике владельца в issue + (владелец сам отнёс задачу к J1 в комментарии-аналитике; повторно отдельно + в ТЗ можно не обосновывать). +- Все обязательные разделы §7.1 PROCESS.md на месте: сценарий, что увидит + человек, проблема, скоуп/не-скоуп, контракт поведения (§8–§9), UX (§10), + данные/миграция/i18n (§11), AC1–AC8 (кроме отмеченного пробела), план + автотестов (§15), риски (§16), откат (§17), release-артефакты (§18). +- Явный блок «Принятые предположения» (§19) присутствует и корректно помечает + как assumption именно то, что действительно не было продиктовано владельцем + дословно (например, что Glow-base — часть пола, а live Glow/солнце — часть + света) — не выдаёт догадку за факт, ревьюер может её оспорить (не оспариваю: + подтверждается разделением функций `_renderGlowBaseRooms`/`_renderGlowLayer` + в коде). +- Причина бага (§3) точно подтверждена чтением кода — вызов decor + действительно предшествует рендеру комнат. +- Решение владельца по Q1 (без нового флага «под планом», единый порядок для + старого и нового decor) корректно и полностью перенесено в §8.3/§19.1, без + расширения скоупа. +- Открытый технический риск с Glow/CSS isolation (§8.2) не замалчивается: ТЗ + прямо требует не переносить decor поверх live Glow «молча» и возвращать + вопрос владельцу, если fixture покажет иное — правильная эскалация вместо + догадки. +- «Не входит в задачу» (§7) корректно исключает per-object флаг, изменение + геометрии старого декора, любые изменения заливки/Glow/солнца/стен, + добавление decor в статическую карточку и в скрытую изометрию отдельным + рендерером, новый UI/i18n/миграцию. +- Touch/UX (§10): корректно ссылается на `docs/TOUCH-SUPPORT.md`, не создаёт + новых focus targets/жестов; View/kiosk-контракт (блокирующий по + TOUCH-SUPPORT.md) не расширяется и не сужается. +- Данные/миграция/i18n (§11): корректно — рендер-порядок не персистентное + поле, миграция не нужна, что подтверждается отсутствием изменений в + `custom_components/houseplan/validation.py`/типах decor. +- AC5 (documented mutant) явно требует, чтобы smoke падал по существу + (AC1/AC2), а не просто по отсутствию DOM-узла — соответствует духу + `scripts/mutation-gate.mjs` и извлечённому там же уроку про тесты, которые + «ни разу не проверяли на способность падать». +- Golden-план (§15.3) корректно ограничивает принятие эталона отдельным + reviewed Linux-артефактом, Windows — только диагностика; согласуется с + AGENTS.md. +- Регистрация ТЗ в `docs/specs/README.md` на месте. + +## Чего не проверял + +- Не проверял и не мог проверить визуальный результат Glow-blend на decor в + браузере — эта проверка относится к этапу реализации/код-ревью (targeted + fixture, §8.2), на этапе спецификации нет кода, который можно запустить. +- Не прогонял тяжёлые гейты (typecheck/test/build/golden/smoke) — на этом + этапе продуктовый код не менялся, они не относятся к предмету ревью ТЗ и + будут частью код-ревью. +- Не проверял `docs/TESTING.md` на предмет уже существующей документации + layer-order/mutant contract для decor — в текущей редакции ТЗ такого + описания там нет, и ТЗ (§12) верно относит его актуализацию к зонам + изменений реализации, а не к самой спецификации. +- Не оценивал производительность фактически (нет кода) — только сверил, что + §13 не описывает новых наблюдателей/таймеров/network calls, что достаточно + для стадии ТЗ. + +## Резюме + +Одна находка Medium в скоупе (M1 — отсутствует доказательство для одной +конкретной, но явно нормативной, границы порядка: decor относительно room +hover fill). High-находок нет. Остальной контракт — порядок слоёв, hide/editor +parity, компат старого decor, touch/UX, данные/i18n, release-артефакты — точен, +проверяем и обоснован чтением кода, а не заявлением автора. Вердикт: жёлтый, +возврат автору на правку ТЗ (добавить hover fill в AC2 и тест-план), без +обращения к владельцу.