From 6297af6701f990e0f067017f2a00107e4b85d8ec Mon Sep 17 00:00:00 2001 From: "claude[bot]" <209825114+claude[bot]@users.noreply.github.com> Date: Sat, 26 Sep 2026 05:22:56 +0000 Subject: [PATCH] docs: review document for #654 Issue: #654 User-Visible: no --- docs/reviews/INDEX.md | 3 +- docs/reviews/SPEC-REVIEW-654-r1.md | 261 +++++++++++++++++++++++++++++ 2 files changed, 263 insertions(+), 1 deletion(-) create mode 100644 docs/reviews/SPEC-REVIEW-654-r1.md diff --git a/docs/reviews/INDEX.md b/docs/reviews/INDEX.md index 7f982f31..aa0cad78 100644 --- a/docs/reviews/INDEX.md +++ b/docs/reviews/INDEX.md @@ -1,9 +1,10 @@ # Индекс ревью -Генерируется `node scripts/reviews-index.mjs` (#635) — не редактировать руками. Документов: 1068, issue: 376. Вердикт: 🟢 зелёный · 🟡 жёлтый · 🔴 красный · ⚪ не распознан (свободная форма старых документов). H/M — число High/Medium по строке вердикта или заголовкам находок. Файлы — пути, названные в находках; ищите по имени файла: `grep form-kit INDEX.md`. +Генерируется `node scripts/reviews-index.mjs` (#635) — не редактировать руками. Документов: 1069, issue: 377. Вердикт: 🟢 зелёный · 🟡 жёлтый · 🔴 красный · ⚪ не распознан (свободная форма старых документов). H/M — число High/Medium по строке вердикта или заголовкам находок. Файлы — пути, названные в находках; ищите по имени файла: `grep form-kit INDEX.md`. | Issue | Документ | Этап · раунд | Вердикт | H | M | Находки | Файлы | |---|---|---|---|---:|---:|---|---| +| #654 | [SPEC-REVIEW-654-r1.md](SPEC-REVIEW-654-r1.md) | spec · r1 | 🟡 жёлтый | 0 | 1 | «Release-артефакты» не называют обновление docs/ISOMETRIC.md | `docs/ISOMETRIC.md` `docs/CHANGELOG.md` `docs/CHANGELOG.ru.md` `docs/reviews/INDEX.md` | | #651 | [SPEC-REVIEW-651-r1.md](SPEC-REVIEW-651-r1.md) | spec · r1 | 🟡 жёлтый | 0 | 1 | устаревшая формулировка «экспериментальный 2.5D-вид» противоречит текущему статусу функции | `CHANGELOG.md` `CHANGELOG.ru.md` `docs/ISOMETRIC.md` `docs/USER-GUIDE.ru.md` `docs/STATUS.md` `USER-GUIDE.ru.md` | | #651 | [SPEC-REVIEW-651-r2.md](SPEC-REVIEW-651-r2.md) | spec · r2 | 🟢 зелёный | 0 | 0 | — | — | | #651 | [CODE-REVIEW-651-r1.md](CODE-REVIEW-651-r1.md) | code · r1 | 🟡 жёлтый | 0 | 2 | AC1 «другая комната исключена» не доказан ни тестом, ни мутацией; AC4 «деградированный fallback группы» не доказан ни тестом, ни мутацией; неточная формулировка в комментарии к реализации (снято без правки) | `src/iso-overlays.ts` `test/iso-overlays.test.mjs` `scripts/mutation-registry.mjs` | diff --git a/docs/reviews/SPEC-REVIEW-654-r1.md b/docs/reviews/SPEC-REVIEW-654-r1.md new file mode 100644 index 00000000..4a3f8acc --- /dev/null +++ b/docs/reviews/SPEC-REVIEW-654-r1.md @@ -0,0 +1,261 @@ +# SPEC-REVIEW-654-r1 + +**Issue:** [#654](https://github.com/Matysh/houseplan-card/issues/654) — «2.5D: цвет бумаги читается `getComputedStyle` в `render()` — на первом кадре все полы «тёмные»; вероятная вспышка Flat→2.5D при холодной загрузке дашборда» +**Этап:** spec (ревью ТЗ, PROCESS.md §2.4) +**Заход:** r1 · блокирующих циклов израсходовано (после этого вердикта): 1 из 4 + +## Вердикт + +**Жёлтый.** High: 0 · Medium: 1 (в скоупе, возвращается автору) · Low: 0. + +Единственная находка — «Release-артефакты» ТЗ не называют обновление +`docs/ISOMETRIC.md` («Activation»), хотя задача вводит новый, ранее нигде не +документированный элемент этого контракта: план скрыт загрузочной +поверхностью, пока ленивый 2.5D-рантайм не установлен (в т.ч. в kiosk, где +общий boot veil сегодня отключается мгновенно). Остальная часть ТЗ — +обязательные разделы §7.1, однозначность и доказуемость AC1–AC6, явный блок +технических предположений — выполнена корректно, а фактические технические +предпосылки (`getComputedStyle` на `.hp-paper`, `_effectiveProjection()`, +`_isoSceneRuntimeLoader`, поведение kiosk в `setConfig()`) проверены по коду +`dev` и подтвердились буквально. + +## Скоуп проверки + +1. Какую строку `docs/SCOPE.md` закрывает задача — J1 («живой обзор дома») в + рамках уже принятого узкого исключения #89 (детерминированная 2.5D-презентация + той же геометрии, без второй модели/свободной камеры); задача не расширяет + это исключение, а чинит дефект внутри него. +2. Обязательные разделы ТЗ по PROCESS.md §7.1 (сценарий и «что человек увидит», + проблема, скоуп/не-скоуп, поведенческий контракт, UX, данные/миграция, + i18n, AC1…ACn с доказательством, план автотестов, риски, откат, + release-артефакты). +3. Однозначность и проверяемость каждого AC (AC1–AC6) и то, что заявленный + способ доказательства (unit/AST/smoke) реален, а не ссылается на + несуществующий код. +4. Соответствие фактических технических утверждений ТЗ действительному + состоянию `src/houseplan-card.ts`, `src/iso-materials.ts` на `dev` (это не + код-ревью диффа — диффа ещё нет; это проверка, что ТЗ не выдаёт догадку за + факт о существующем поведении). +5. Терминология видимого поведения — против `docs/USER-GUIDE.ru.md` и + канонического `docs/ISOMETRIC.md` (обязательное чтение для видимого + поведения по инструкции ревью). +6. Наличие открытых продуктовых вопросов, которые ТЗ решило само вместо + вынесения владельцу, и их обоснованность. + +## Как проверялось + +- Тело issue #654 и комментарии получены через `gh issue view 654 --json + body,comments,labels,state` (MCP `mcp__github__get_issue`/`get_issue_comments` + были недоступны без разрешения пользователя в этой сессии — `gh` CLI как + эквивалентный путь к тому же публичному API). +- Прочитаны целиком `docs/SCOPE.md`, `docs/process/REVIEWER.md`, `AGENTS.md` + (обязательный порядок чтения ревьюера), раздел PROCESS.md §7.1 (обязательные + разделы ТЗ, шаблон вердикта). +- Прочитан целиком `docs/ISOMETRIC.md` (в первую очередь «Activation» и Stage 6 + «Raised tiles» — источник формулы `isoLightFloorRooms`/luma > 0.55). +- Код `dev` (`git rev-parse HEAD` = `803c0f0eaab3aea1ea9a87215c35b638a679dafb`) + проверен построчно на все фактические утверждения ТЗ: + - `src/houseplan-card.ts:10701` — буквально совпадает с цитатой в issue + (`getComputedStyle(this.renderRoot.querySelector('.hp-paper') ?? this).fill` + внутри `_renderBody()`, вызываемой из `render()`). + - `src/houseplan-card.ts:6002-6008` (`_effectiveProjection()`) — подтверждён + ранний `return finish('flat')`, пока `!this._isoSceneRuntime`. + - `src/houseplan-card.ts:2568` (`connectedCallback`) — подтверждена + предзагрузка `if (this._isoEnabled) void this._ensureIsoSceneRuntime();`. + - `src/houseplan-card.ts:3095` (`setConfig()`) — подтверждено, что kiosk + немедленно гасит `_booting`/`_bootFading` («kiosk: 100dvh, nothing to + settle»), т.е. общий boot veil в kiosk сегодня действительно не + появляется — ровно то, что называет issue. + - `src/houseplan-card.ts:332-337` (`BOOT_MIN_MS/BOOT_QUIET_MS/BOOT_MAX_MS`, + `_bootWatch()` `:6356-6373`) — существующий boot-veil таймер имеет + безусловный потолок `BOOT_MAX_MS = 1200` мс. Это прямое технической + напряжение с «переиспользовать существующий bootveil» (если ленивый чанк + ставится дольше 1.2 с, безусловное снятие veil обнажит Flat-кадр вопреки + AC3) — ТЗ уже закрывает этот риск не как факт, а как открытый технический + выбор в разделе «Принято предположительно»: «использование существующего + `bootveil` **либо** визуально идентичного внутреннего состояния + iso-pending». Разбор ниже, в «Что проверено и корректно». + - `src/iso-materials.ts:91-99` (`isoLightFloorRooms`) — подтверждено, что + функция уже является чистой и принимает явный `Rgb`, DOM не трогает; + баг живёт только в вызывающем коде (`houseplan-card.ts:10701`), что + напрямую подтверждает реалистичность AC1 (unit-тест на чистую функцию уже + возможен без новой архитектуры). + - `test/iso-stage6.test.mjs:7,43` — `isoLightFloorRooms` уже юнит-тестируется + без DOM, ссылка AC1 не на пустое место. + - `demo/smoke_volumetric_setting.mjs` (90 строк, без `page.reload()`) — + подтверждена цитата issue «проверяет только переключение без + перезагрузки». + - `demo/smoke_isometric_contract.mjs`, `demo/smoke_iso_tiles.mjs`, + `demo/smoke_iso_theme_walls.mjs` — существуют (план автотестов ссылается + на реальные файлы, не выдуманные). +- `docs/reviews/INDEX.md` проверен на прецедент: обе смежные задачи (#651, + #649) в списке затронутых файлов называли `docs/ISOMETRIC.md`, а ревью #651 + прямо содержало Medium-находку за расхождение терминологии ТЗ с этим же + каноническим документом — это основа находки ниже, а не единичное мнение. +- Код продукта не менялся, гейты (`tsc`/`npm test`/`npm run build`) не + запускались: на этапе `spec` предмет ревью — текст ТЗ, диффа ещё нет (ветка + реализации не создана). + +## Находки + +### Medium (в скоупе) — «Release-артефакты» не называют обновление `docs/ISOMETRIC.md` + +**Файл:** тело issue #654, раздел `## ТЗ`, подраздел «Release-артефакты» (и +смежно — «Контракт поведения и UX», п. 1–2). + +**Воспроизведение:** +- «Release-артефакты» ТЗ перечисляют только: `docs/CHANGELOG.md` + + `docs/CHANGELOG.ru.md` (да), «пользовательское руководство/i18n/config + migration: нет», golden/perf/security — без единого упоминания + `docs/ISOMETRIC.md`. +- При этом «Контракт поведения и UX» вводит новый пункт жизненного цикла, + которого сегодня в `docs/ISOMETRIC.md` («Activation») нет ни в каком виде: + «Пока ленивый 2.5D-рантайм загружается, план не показывается в + промежуточном Flat-виде… Это относится и к kiosk, где общий boot veil + обычно отключён» — то есть kiosk получает *новое* видимое поведение + (кратковременная загрузочная поверхность), которого у него сегодня нет + вовсе (`setConfig()` гасит `_booting`/`_bootFading` безусловно, см. «Как + проверялось»). +- Текущий текст `docs/ISOMETRIC.md:10-28` («Activation») описывает только: + когда включена ленивая загрузка чанка, что переключение без перезагрузки + мгновенно, и фингерпринт-фолбэк — ни слова о том, что скрывается сам план на + время установки чанка при холодном старте, и что это распространяется на + kiosk, где обычный boot veil не работает. + +**Почему это находка, а не придирка к формулировке.** `docs/ISOMETRIC.md` — +канонический документ подсистемы (AGENTS.md, «Reading this first»; PROCESS.md +§2.10 требует смотреть его строки перед разбором подсистемы), и обе соседние +задачи по той же подсистеме (#649, #651 — см. `docs/reviews/INDEX.md`) +называли именно этот файл среди затронутых при заметно менее существенных +изменениях поведения (терминология, geometry). Здесь же вводится реальный +новый инвариант жизненного цикла — «первый кадр 2.5D скрыт до готовности +рантайма, включая kiosk» — который следующий разработчик, открывший только +`docs/ISOMETRIC.md`, не увидит и рискует нарушить неосознанно (например, при +следующей правке `_bootWatch()`/`BOOT_MAX_MS` или логики `setConfig()` для +kiosk). Отсутствие этого пункта в «Release-артефактах» — это ровно тот +паттерн, что уже стоил Medium-находки в #651. + +**Чем закрывается.** Добавить в «Release-артефакты» строку об обновлении +`docs/ISOMETRIC.md` (раздел «Activation» — или новый короткий подраздел о +first-frame gating), фиксирующую: план скрыт до готовности ленивого рантайма +при `volumetric_view: true`, это распространяется на kiosk, terminal failure +безопасно возвращает Flat. Формулировка — на усмотрение автора, это +техническая правка текста ТЗ, а не новый продуктовый вопрос владельцу. + +**Класс:** Medium, в скоупе задачи — правится прямо в теле ТЗ, отдельный issue +не заводится (#202). + +## Что проверено и корректно + +- **Обязательные разделы §7.1** — присутствуют все: «Сценарий и видимое + изменение» (отвечает на оба продуктовых вопроса — какая персона/поверхность/ + момент и что человек видит одной фразой, без терминов реализации), «Проблема + и причины», «Скоуп»/«Не скоуп», «Контракт поведения и UX», «Модель данных и + совместимость», «i18n», «Критерии приёмки» (AC1–AC6 — см. ниже), + «План автотестов и доказательств», «Производительность и touch» (не + обязателен, но уместен для render-path фикса), «Риски», «Принято + предположительно, поменять свободно», «Откат», «Release-артефакты». +- **Однозначность и доказуемость AC1–AC6.** Каждый AC называет конкретный вид + доказательства (unit/AST/smoke) и наблюдаемый критерий, а не самоцель + реализации: + - AC1 — чистая функция классификации без DOM; белая бумага + светлая заливка + → light-floor, тёмная → нет; cold-reload smoke сверяет первый кадр с + установившимся. Подтверждено технически: `isoLightFloorRooms` уже чистая + (см. «Как проверялось»), баг — только в вызове. + - AC2 — отсутствие `getComputedStyle`/`getBoundingClientRect` в + `render`/`_renderBody`/`willUpdate` и в iso render-helper поверхностях, + AST-проверка + браузерный счётчик обращений на повторных HA-рендерах без + смены темы/режима. Способ доказательства (AST, не regex по тексту монолита) + соответствует требованию ревьюера не полагаться на текстовые якоря. + - AC3 — атомарность холодного старта при искусственно задержанном чанке, для + обычной карточки и kiosk; критерий (`.stage.projection-iso`, устойчивый + viewBox) — наблюдаемый и не расплывчатый. + - AC4 — безопасный отказ: конечная загрузочная поверхность, доступный Flat, + настройка не меняется, приватные данные не в диагностике — опирается на + уже существующий контур `EditorRuntimeLoader`/`safeRuntimeDiagnostic`, не + изобретает новый механизм отказа. + - AC5 — точная инвалидация memo по бумаге/fills/rooms; переход в редактор и + обратно не переносит цвет одного режима в классификацию другого. + - AC6 — соседние контракты (`volumetric_view` как есть, Flat не запрашивает + чанк, structural fallback, zoom/устройства) явно перечислены как + регрессионный периметр. +- **Технические предпосылки ТЗ проверены по коду, а не приняты на слово.** + Все процитированные строки и функции (`_renderBody`/`render`, + `_effectiveProjection`, `connectedCallback`-предзагрузка, + `setConfig()`-поведение kiosk, `EditorRuntimeLoader`) существуют буквально + там, где заявлено, и ведут себя так, как описано — включая менее очевидное + наблюдение issue про kiosk (`setConfig()` действительно гасит `_booting` + раньше, чем что-либо успевает отрисоваться). Догадок, выданных за факт, в + части воспроизведения дефекта не найдено. +- **Раздел «Принято предположительно» реалистично закрывает найденное + техническое напряжение.** `BOOT_MAX_MS = 1200` мс — безусловный потолок + существующего boot veil — потенциально конфликтует с «дождаться ленивого + рантайма» при медленной сети. ТЗ не скрывает эту развилку: явно оставляет + автору выбор между переиспользованием `bootveil` и отдельным + `iso-pending`-состоянием, а ревьюер вправе его оспорить (§7.1). Технической + ошибки/недосказанности здесь нет — граница ответственности проведена верно. +- **Отсутствие открытых продуктовых вопросов проверено, а не принято на + слово.** Кроме разобранной выше Medium-находки, ни одного места, где догадка + о видимом поведении подана как факт без пометки предположения, не найдено. + Формулировка «Продуктовых вопросов для владельца нет» в аналитике — + обоснована: единственная пограничная зона (новое поведение kiosk) уже прямо + зафиксирована как решение в контракте, а не спрятана. +- **Соответствие `docs/SCOPE.md`.** Задача остаётся строго внутри узкого + исключения #89 (никакой новой камеры/модели/настройки), корректно исключает + #651 (дрейф значков при зуме) и `_pointInRoom` из скоупа со ссылкой на + конкретные issue. +- **i18n** — новых строк нет, что верно: явно запрещён новый текст/кнопка/ + индикатор (Контракт поведения, п. 1). +- **Модель данных/совместимость** — эфемерный runtime-кэш экземпляра карточки, + без схемы/YAML/localStorage/миграции; согласуется с характером фикса + (render-path + lifecycle-гейт, не персистентное состояние). +- **Откат** — простой revert продуктового коммита, без обратной миграции; + корректно для чисто presentation-слойного изменения. +- **Артефакты плана автотестов существуют.** `demo/smoke_isometric_contract.mjs`, + `demo/smoke_iso_tiles.mjs`, `demo/smoke_iso_theme_walls.mjs`, + `demo/smoke_volumetric_setting.mjs` — реальные файлы, не выдуманные ссылки; + последний подтверждённо не покрывает `page.reload()`, что и обосновывает + необходимость нового/расширенного smoke по AC3. + +## Чего не проверял + +- `tsc`/`npm test`/`npm run build` и прочие гейты §8 не запускал — на этапе + `spec` предмет ревью текст ТЗ, реализации ещё нет (ветка задачи не создана + на момент ревью). +- Не проверял браузерное поведение вручную — реализации нет, нечего + запускать; это работа код-ревью следующего этапа. +- Не читал построчно весь `src/iso-scene-render.ts`/`src/iso-sun.ts`/ + `src/iso-tiles.ts` — только точечно то, что нужно для проверки фактических + утверждений ТЗ (существование `isoLightFloorRooms`, `_isoSceneRuntime`, + boot-veil таймеров). Полный аудит «какие именно файлы входят в 2.5D + render-helper поверхности» для AC2 — задача код-ревью, когда появится AST-тест + и станет видно его фактический периметр. +- Не оценивал real-world длительность загрузки чанка `iso-scene-render` + (совпадает ли она обычно с `BOOT_MAX_MS`) — это эмпирический вопрос + реализации/код-ревью, а не спецификации; отметил риск как технически + реальный, но признал его закрытым явным «принято предположительно», а не + находкой. +- Полные наборы golden/perf/smoke не прогонял — кода для прогона ещё нет; это + часть «Плана автотестов» и войдёт в работу при реализации (§2.6) и + последующем код-ревью (§2.7). + +## Рекомендация автору + +Одна точечная правка текста ТЗ — дополнить «Release-артефакты» пунктом об +обновлении `docs/ISOMETRIC.md`. Правка не требует нового цикла анализа по +существу поведенческого контракта: сам контракт (AC1–AC6, риски, допущения) +корректен и не меняется. + +--- + + + +## Материал раунда + +- Ветка: `dev`, коммит `803c0f0eaab3` — ребейз его осиротит, и это нормально: ниже якоря, которые ребейз не меняет. +- Дерево материала: `b4eeb2b0840189d8031f291834e57fab092af49b` + ``` + git log --all --format='%H %T' | grep b4eeb2b08401 + ``` +- Тело issue: `bd46753eac9a5a3b908addb0135b2054eea85c66f89eda4b4f8bdae8d80d78cd` +- Вердикт конвейера: `yellow` · High 0