mirror of
https://github.com/Matysh/houseplan-card
synced 2026-09-29 03:09:36 +00:00
@@ -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.
|
||||
|
||||
---
|
||||
|
||||
<!-- material-anchors: сгенерировано конвейером (#414) -->
|
||||
|
||||
## Материал раунда
|
||||
|
||||
- Ветка: `issue/665-iso-label-metrics`, коммит `8ffbdf8a28f8` — ребейз его осиротит, и это нормально: ниже якоря, которые ребейз не меняет.
|
||||
- Дерево материала: `564583c27e4579c22778936761a60ee40306774c`
|
||||
```
|
||||
git log --all --format='%H %T' | grep 564583c27e45
|
||||
```
|
||||
- Тело issue: `8da493f662bb94963a92e6ca40c224f16235947efbd137e809e313833e01cc97`
|
||||
- Вердикт конвейера: `yellow` · High 0
|
||||
Reference in New Issue
Block a user