From d06cca2c19a3b5a062b79b4073227e5682636c90 Mon Sep 17 00:00:00 2001 From: "claude[bot]" <209825114+claude[bot]@users.noreply.github.com> Date: Sat, 26 Sep 2026 13:02:12 +0000 Subject: [PATCH] docs: review document for #665 Issue: #665 User-Visible: no --- docs/reviews/CODE-REVIEW-665-r1.md | 216 +++++++++++++++++++++++++++++ 1 file changed, 216 insertions(+) create mode 100644 docs/reviews/CODE-REVIEW-665-r1.md diff --git a/docs/reviews/CODE-REVIEW-665-r1.md b/docs/reviews/CODE-REVIEW-665-r1.md new file mode 100644 index 00000000..370be38a --- /dev/null +++ b/docs/reviews/CODE-REVIEW-665-r1.md @@ -0,0 +1,216 @@ +# CODE-REVIEW-665-r1 + +Issue: [#665](https://github.com/Matysh/houseplan-card/issues/665) — «2.5D: показатели комнаты под названием «плавают» при зуме». +Этап: code-review, заход r1, лёгкий трек `small`, лимит циклов 2. +Материал: `git log --oneline origin/dev..HEAD` / `git diff origin/dev...HEAD`, +ровно +``` +head: 8ffbdf8a28f89f956fe6a0e550aec7b1d42efbf5 +base: 72d49040 (origin/dev) +``` +Рабочая копия на этом SHA (`git rev-parse HEAD` = `8ffbdf8a28f89f956fe6a0e550aec7b1d42efbf5`), fetch/checkout не выполнялись. + +## Скоуп + +Два коммита: +- `c93b3a90` (fix, User-Visible: yes) — `.stage.projection-iso.mode-view .roomlabel` + теряет `min-width/min-height: 44px` и `justify-content: center`; пол + касания 44×44 переносится в `::before` (приём `.oplock::before`), новый смок + `demo/smoke_iso_room_label_metrics.mjs`, мутант + `iso-room-label-44-box-centres-name`, `docs/ISOMETRIC.md`, оба changelog. +- `8ffbdf8a` (test, User-Visible: no) — тот же смок переведён с прямой записи + `card._view` на публичные кнопки зума (`data-hp="zoom-fit"` / + `"zoom-in"`), потому что первая версия красила `no-new-private-writes`. + +Класс A (`src/styles/plan.styles.ts`) + B (смок, реестр мутаций) + C (docs). +Работа закрывает J1/J5 (`docs/SCOPE.md`): показатели комнаты (температура, +свет и т. д.) в 2.5D-виде должны читаться так же надёжно, как во Flat, а не +съезжать к имени на зуме. + +## Как проверялось + +Валидировано, что зелёный Validate у `8ffbdf8a` +(https://github.com/Matysh/houseplan-card/actions/runs/36242169265) подтверждает +дешёвые гейты — они не перегонялись: + +| Гейт | Статус | Как подтверждён | +|---|---|---| +| `tsc --noEmit`, `npm test`, `npm run build`, `bundle-policy --verify` | не перегонялся | зелёный Validate на точном SHA материала | +| `check-docs` (диф трогает `src/**`) | не перегонялся | тот же зелёный Validate (job `docs`) | +| `demo/smoke_iso_room_label_metrics.mjs` (AC1, AC2) | **прогнан здесь** | свежая сборка (`npm run bundle:sync`) → `node demo/smoke_iso_room_label_metrics.mjs` → `OK`, все чеки true | +| `demo/smoke_isometric_contract.mjs` (`raisedTargetsOwn44Pixels`, C3) | **прогнан здесь** | `OK`, `raisedTargetsOwn44Pixels: true` | +| Мутант `iso-room-label-44-box-centres-name` (AC3) | **прогнан здесь, реально, не `--check`** | `node scripts/mutation-gate.mjs --id=iso-room-label-44-box-centres-name` (без `--check`, то есть патч реально применён и гвард реально выполнен) → `ok iso-room-label-44-box-centres-name: заявленный тест покраснел на мутанте`, `поймано 1 из 1` | +| Отрицательная проба на AC2 (свой мутант, не заявленный автором) | **прогнан здесь** | вручную вырезан блок `.roomlabel::before` из `plan.styles.ts`, пересобрано → тот же смок красит `floorIs44: false`, `floorOwnsTheTarget: false`. Автор в таблице AC2 сам отметил, что не мутировал это отдельно («отрицательной пробы для «пола нет» здесь нет») — закрыто мной, третий столбец таблицы AC подтверждён исполнением, а не только присутствием `owns44` в контрактном смоке | +| `node scripts/smoke-select.mjs --base 72d49040 --head 8ffbdf8a` | прогнан, результат НЕОПРЕДЕЛЁННОСТЬ | «дифф исполняемый, но ни один смок не связан доказуемо» — решение по AC и содержанию диффа: два смока выше названы в AC/ТЗ, прогнаны прямо | +| `npm run golden:verify` (диф меняет рендер `.roomlabel` в 2.5D) | **прогнан здесь, полностью, дважды** | см. находку ниже — потребовалось сравнение HEAD vs `origin/dev` | +| `docs:accept -- --identical` / скриншоты | не перегонялся | принято по заявлению автора: диф `docs/images/screenshots.json` — только `sourceFingerprint`/`sourceSha256`, все `imageSha256` не изменились → ни один захваченный кадр документации не задет этой правкой (визуально сверено по диффу файла) | +| `python -m pytest tests_backend` | не прогонялся | диф не трогает `custom_components/**/*.py` | +| `npm run invariants` | не прогонялся | диф не трогает геометрию модели/ссылки на неё, только CSS раскладку подписи | +| perf | не прогонялся | не назван в AC, ТЗ прямо говорит «перф — нет» | + +## Находки + +### Medium (в скоупе задачи) — golden-влияние диффа занижено в отчёте автора вдвое-без-остатка: 0 заявлено, 8 подтверждено + +**Воспроизведение.** Собрал HEAD (`npm run bundle:sync`) и прогнал +`node demo/golden/run.mjs --mode=verify` до конца (без укороченного таймаута — +предыдущая попытка с дефолтным 120-секундным лимитом обрывалась раньше +записи итогового `golden-report.json`, что само по себе стоит знать: короткий +таймаут даёт неполный список различий). Параллельно собрал `origin/dev` +(`72d49040`) в отдельном `git worktree` и прогнал тот же `golden:verify` там +для чистого сравнения «было / стало». + +`origin/dev` (`72d49040`) — 12 сценариев `different`, все — известный, +задокументированный шум (`OPENING_SYMBOL_EXISTING_GOLDEN_IMPACT` в +`demo/golden/matrix.mjs` частично их перечисляет; остальные — furniture/dialog +дрейф, тоже вне этой задачи). + +HEAD (`8ffbdf8a`) — те же 12 плюс **8 новых** `different`, ни один не входит в +`OPENING_SYMBOL_EXISTING_GOLDEN_IMPACT`: + +| Сценарий | differingPixels | diffRatio | roomMetrics/label_* включены? | +|---|---|---|---| +| `isometric-geometry-view-dark` | 679 | 0.000881 | нет — только имена комнат | +| `isometric-geometry-view-light` | 679 | 0.000881 | нет | +| `isometric-touch-kiosk-dark` | 211 | 0.000740 | нет | +| `isometric-large-warm-remount-dark` | 1837 | 0.002126 | нет | +| `isometric-stage3-overlays-light` | 836 | 0.001085 | да (`stage3Fixture.roomMetrics`) | +| `isometric-stage3-overlays-dark` | 1403 | 0.001821 | да | +| `isometric-stage3-forced-colors-dark` | 3032 | 0.003934 | да | +| `isometric-stage3-no-filter-dark` | 794 | 0.001030 | да | + +Визуально (`artifacts/golden/diff/*.png`) различия именно там, где и должны +быть по замыслу правки: в `isometric-stage3-overlays-dark` розовым — строка +показателей под именами комнат (сдвинулась ближе к имени, как и требует AC1); +в `isometric-geometry-view-dark` (где `label_*` не включены вовсе) розовым — +сам текст названий комнат («NW», «SW» и т. д.), сдвинувшийся на пару +пикселей, потому что якорь имени в подписи тоже зависел от снятых +`min-height`/`justify-content` (C1 в ТЗ это и требует: «положение самого имени +относительно якоря — как во Flat»). + +**Почему это находка, а не просто факт жизни.** Хендофф автора говорит +буквально: «golden: 2.5D-кадров с показателями комнат в матрице не нашёл +(`dayCycle`/iso-сценарии без `label_*`), но полная матрица — на кандидате +беты». Это утверждение неверно дважды: (1) кадры с `label_*` в матрице +**есть** — четыре сценария `isometric-stage3-*`, все они используют +`stage3Fixture: { roomMetrics: true, … }` (`demo/golden/harness.mjs` +включает `label_temp/label_hum/label_lqi/label_light`); (2) реальное +воздействие правки шире, чем «кадры с показателями»: она двигает якорь +самого имени в **любой** 2.5D-подписи с `show_names: true`, что задело ещё +четыре сценария без единого включённого `label_*`. Раздел «Release-артефакты» +самого ТЗ прямо требовал этой проверки: «golden: кадры 2.5D с показателями +комнат (если есть в матрице) изменятся — приёмка по Linux CI». Кадры есть, +блок ими не ограничен, а запись в issue утверждает обратное — это ровно тот +случай, когда «не нашёл» без названной команды и её результата не +является доказательством (`AGENTS.md`, «Verified» без команды). + +**Не то же самое, что дефект в реализации.** Сама правка работает верно: все +8 расхождений — маленькие (`diffRatio` 0.0007–0.0039, ниже +`maxDiffRatio: 0.0005` порога лишь на волос до полутора порядков, видимый +сдвиг на 1–3 px), согласуются с C1/AC1 и не открывают новых конфликтов +с уже проверенным C3 (`isoRaisedOverlayHalfSize` не тронут, `raisedGeometryTracksHtml` +зелёный). Обновлять сами PNG-baselines в этом коммите и нельзя: класс D, +`demo/golden/baselines/**` требует `Release:` + `Baseline-Reviewed*` трейлер, +то есть кандидат беты, не обычная задача. + +**Что чинить в скоупе этой задачи (без переноса в отдельный issue — это тот +же диф, что и породил расхождение).** Автору — поправить запись в issue: ⑴ +назвать восемь реально задетых сценариев (не «не нашёл»), ⑵ явно передать +эстафету: эти восемь ждут `npm run golden:accept -- --reviewed` на кандидате +беты вместе с уже известными двенадцатью, чтобы тот, кто будет принимать +кандидат, не тратил время на реатрибуцию и не принял расхождение вслепую +(`AGENTS.md`: «никогда не принимать … изображения просто чтобы CI позеленел»). +Кода это не касается — правка чисто в разделе учёта диффа. + +Без High-находок это Medium в скоупе задачи → жёлтый вердикт, возврат автору +(#202): отдельный issue не заводится. + +### Low, снято без возврата — расположение строки в `docs/ISOMETRIC.md` + +Строка про пол подписи (`docs/ISOMETRIC.md:326-328`) добавлена в раздел +«Raised tiles (`src/iso-tiles.ts`, `src/styles/iso-tiles.styles.ts`)», хотя +подписи комнат рендерятся из `src/styles/plan.styles.ts` и вообще не через +`iso-tiles.ts`. Спек-ревью уже отмечало этот пункт как Low и оставляло на +усмотрение исполнителя — здесь тоже не блокирует ни один AC (сама формулировка +верна, просто не в том разделе). Оставляю на усмотрение автора при следующей +правке этого файла; отдельного действия не требую. + +## Что проверено и подтверждено корректным + +- **Причина бага и её устранение** — построчно совпадают с тем, что зафиксировал + спек-ревью: `min-height: 44px` + `justify-content: center` на flex-контейнере + подписи центрировали имя в коробке 44 px, `.rlmetrics` (`position: absolute; + top: calc(100% + 0.15em)`) отсчитывалась от нижней границы этой коробки, а не + от имени — отсюда зависящий от зума зазор. Правка снимает оба свойства и + переносит пол 44×44 в `::before` с `z-index: -1` **внутри собственного + контекста наложения `.roomlabel`** (`position: absolute; z-index: 1` на самой + подписи, `src/styles/plan.styles.ts:557,565`) — это не мировой z-index, а + локальный порядок внутри одной подписи, поэтому пол рисуется под текстом + подписи, но не проваливается под соседние элементы плана. Приём — буквальная + копия `.oplock::before` (`src/styles/plan.styles.ts:541-551`), с тем же + `width/height: max(44px, 100%)` и центрированием `translate(-50%, -50%)`. +- **AC1** — доказан исполнением: `demo/smoke_iso_room_label_metrics.mjs`, + свежая сборка, зелёный. Отношение `gap/высота имени` совпадает в Flat и 2.5D + на обоих зумах (0.096 и 0.098 в паре точек, разница < 0.02) и не плывёт между + зумами — это ровно то же самое соотношение из таблицы issue (Flat: 0.1 + постоянно, было 2.5D: 2.0 → 0.16, стало 2.5D: то же 0.1, что и Flat). +- **AC2** — доказан исполнением дважды: положительно (`floorIs44`, + `floorOwnsTheTarget`, `areaLinkStaysClickable` — все true) и отрицательно + (ручное снятие `::before` красит именно эти два чека, проверено мной, не + автором — см. таблицу гейтов выше). +- **AC3** — мутант `iso-room-label-44-box-centres-name` реально пойман + (`mutation-gate.mjs` без `--check`, не только статическая проверка якоря). +- **C3** — `isoRaisedOverlayHalfSize` (`src/iso-scene-render.ts`) не тронут + этим диффом (диф ограничен `plan.styles.ts`); `smoke_isometric_contract` + зелёный целиком, включая `raisedTargetsOwn44Pixels`; `houseplan-space-card` + никогда не рендерит класс `projection-iso` (`grep` по `src/space-card.ts` — + пусто), поэтому не мог быть задет структурно, что независимо подтверждает, + что все восемь золотых расхождений — ожидаемый и единственный побочный эффект + этой правки, а не что-то посевное. +- **Трейлеры** — `Issue:`/`User-Visible:` на обоих коммитах корректны; `yes` + на `c93b3a90` сопровождается правкой обоих changelog в том же коммите; + `no` на `8ffbdf8a` — тестовая правка без изменения продукта, changelog не + трогает, верно. +- **Одно число — один источник.** Пиксельный литерал `44px` в новом + `::before` не создаёт второй источник истины: это тот же паттерн, что уже + используют `.oplock::before` и `.dev` (`src/styles/devices.styles.ts:181-182`) + — общей CSS-переменной для 44 px в кодовой базе нет нигде, буквальное + повторение литерала — существующая конвенция, не регресс. +- **`no-new-private-writes`** — второй коммит специально существует, чтобы + исправить эту находку CI; проверил, что `demo/smoke_iso_room_label_metrics.mjs` + больше не пишет `card._view` напрямую, а жмёт `data-hp="zoom-fit"`/`"zoom-in"`; + единственная мутация состояния конфигурации — `space.settings = {...}` на + локальной переменной `space`, не в цепочке с `_`-полем карточки, гейт бы её + не поймал и это ожидаемо. + +## Чего не проверял + +- Полную матрицу смоков (277 файлов) — прогнаны только два, названных AC, и + контрактный `smoke_isometric_contract`; остальные — предрелизная + обязанность. +- `pytest tests_backend` — диф не трогает Python. +- `npm run invariants` — диф не трогает геометрию модели. +- Performance — не назван в AC, ТЗ говорит «перф — нет» (одно CSS-правило, + довод принят без отдельного прогона). +- Скриншоты документации визуально (только по `imageSha256` в диффе + `screenshots.json` — не совпавших нет, поэтому не открывал сами PNG). +- Полный `golden:capture`/`accept` — не входит в обязанности код-ревью + (класс D требует кандидата беты); прогнан только `verify` (диагностика). + +## Вердикт + +Единственная блокирующая находка — Medium в скоупе задачи, без High. + +--- + + + +## Материал раунда + +- Ветка: `issue/665-iso-label-metrics`, коммит `8ffbdf8a28f8` — ребейз его осиротит, и это нормально: ниже якоря, которые ребейз не меняет. +- Дерево материала: `564583c27e4579c22778936761a60ee40306774c` + ``` + git log --all --format='%H %T' | grep 564583c27e45 + ``` +- Тело issue: `8da493f662bb94963a92e6ca40c224f16235947efbd137e809e313833e01cc97` +- Вердикт конвейера: `yellow` · High 0