mirror of
https://github.com/Matysh/houseplan-card
synced 2026-09-29 03:09:36 +00:00
@@ -0,0 +1,260 @@
|
||||
# CODE-REVIEW-584-r1 — #584: физический габарит мебели равен заявленным размерам
|
||||
|
||||
- Issue: [#584](https://github.com/Matysh/houseplan-card/issues/584)
|
||||
- Этап: `code` (PROCESS.md §2.7)
|
||||
- Диапазон: `origin/dev...HEAD`, `origin/dev` = `7c5fd32a`, `HEAD` = `9bf41ec3`
|
||||
(ветка `issue/584-furniture-physical-bounds`, детач `HEAD`)
|
||||
- ТЗ: тело issue #584, раздел `## ТЗ`; ревью ТЗ зелёное на r2 —
|
||||
[`SPEC-REVIEW-584-r2.md`](SPEC-REVIEW-584-r2.md)
|
||||
- Заход: **r1** (первый заход код-ревью; §2.10 «по дельте» не применяется —
|
||||
это не повторный раунд)
|
||||
- Блокирующих циклов израсходовано: 0/4
|
||||
- Вердикт: **жёлтый**
|
||||
|
||||
## Скоуп ревью
|
||||
|
||||
Два коммита на материале `9bf41ec3`:
|
||||
|
||||
- `ef3bc6d1` (класс A/B/D, `User-Visible: yes`) — продуктовая правка:
|
||||
`src/furniture.ts` (legacy `plant`), новые `scripts/furniture-path-join.mjs`,
|
||||
`scripts/svg-path-bounds.mjs`, правка `scripts/generate-furniture-assets.mjs`
|
||||
(контракт границ, склейка путей), два новых мутанта в
|
||||
`scripts/mutation-registry.mjs`, 44 плановых SVG пака `fix-584-1`,
|
||||
перегенерированные `src/furniture-plan-art.generated.ts` /
|
||||
`src/furniture-menu-art.generated.ts`, тесты
|
||||
`test/furniture-visual-bounds.test.mjs` (новый),
|
||||
`test/furniture-path-join.test.mjs` (новый), оба changelog, синхронные копии
|
||||
бандла (`dist/**`, `custom_components/houseplan/frontend/**`).
|
||||
- `9bf41ec3` (класс D, `User-Visible: no`, `Release: v1.77.0-beta.1`,
|
||||
`Baseline-Reviewed: <прогон 35144164510>`) — приёмка 9 golden-кадров мебели и
|
||||
переснятого индекса `docs/images/screenshots.json`.
|
||||
|
||||
Не тронуты: редактор (`src/houseplan-card.ts`), hit-area, привязка к стенам,
|
||||
схема конфига, i18n, backend — подтверждено `git diff --stat origin/dev...HEAD`
|
||||
(AC7).
|
||||
|
||||
Прочитано до вердикта: `docs/SCOPE.md`, `PROCESS.md` (§1–§10.2, роли, гейты,
|
||||
лимит циклов), тело issue #584 целиком и все 10 комментариев (аналитика, оба
|
||||
раунда ревью ТЗ, ТЗ дизайнеру, приёмка пака, хендофф на код-ревью), весь
|
||||
продуктовый и тестовый диф из списка выше, `scripts/generate-furniture-assets.mjs`
|
||||
целиком, `scripts/bundle-budget.mjs` (в части, касающейся ленивого чанка
|
||||
мебели).
|
||||
|
||||
## Как проверялось
|
||||
|
||||
Материал ревью — ровно `9bf41ec3`; `git checkout`/`fetch` на другой коммит не
|
||||
делал.
|
||||
|
||||
| Гейт | Статус |
|
||||
|---|---|
|
||||
| `npx tsc --noEmit` | **не гонял сам** — зачтён по зелёному Validate на точном SHA (см. ниже) |
|
||||
| `npm test` | **не гонял сам** — зачтён по зелёному Validate (автор заявил 2743/2742/0 fail) |
|
||||
| `npm run build` + сверка 3 копий бандла | **не гонял сам** — зачтён по зелёному Validate |
|
||||
| `node scripts/check-docs.mjs` | **не гонял сам** — зачтён по зелёному Validate («Предполёт: документация, провенанс, процесс» — success) |
|
||||
| `furniture:check` (`--check`) | зачтён по зелёному Validate; отдельно прочитан код проверки (см. AC5) |
|
||||
| `node scripts/smoke-select.mjs --base origin/dev --head HEAD` | прогнал сам — см. ниже |
|
||||
| Браузерные смоки | не гонял сам — зачтены по зелёному Validate (`full=true`, все 3 шарда) |
|
||||
| `npm run golden:verify` | не гонял сам — зачтён по зелёному Validate («Golden-кадры против принятых эталонов» — success) |
|
||||
| `node scripts/model-invariants.mjs` | **не применимо** — диф не трогает геометрию комнат/стен/`layout`/`marker.space`/`open_spans`, только SVG-арт мебели и её собственный unit-box |
|
||||
| `python -m pytest tests_backend` | **не применимо** — `custom_components/**/*.py` не тронут |
|
||||
| Мутанты `furniture-symbol-may-keep-inner-padding`, `furniture-paths-joined-without-reset` | не запускал `mutation-gate` сам — проверил логику мутации чтением (ниже) и зачёл по зелёным «Мутанты по диффу (1–6/6)» в Validate `full=true` |
|
||||
|
||||
**Дешёвые гейты подтверждены на этом SHA дважды**, оба прогона проверены лично через `gh run view`:
|
||||
|
||||
- `35149944216` — light Validate, push-триггер, `headSha = 9bf41ec3`, `conclusion: success`, время создания 2026-09-16T21:00:48Z (после обоих коммитов материала);
|
||||
- `35145083038` — тот же `headSha = 9bf41ec3`, `full=true`, `conclusion: success`; поджобы `Мутанты по диффу (1–6/6)`, `Смоки: все шарды зелёные`, `Golden-кадры против принятых эталонов`, `Перф-смок: бюджет времени кадра`, `Предполёт: документация, провенанс, процесс` — все `success`.
|
||||
|
||||
Оба прогона относятся ровно к материалу ревью, а не к промежуточному коммиту —
|
||||
проверил `headSha` явно, а не поверил заявлению автора на слово.
|
||||
|
||||
`smoke-select.mjs` на диапазоне `origin/dev..HEAD` вернул **НЕОПРЕДЕЛЁННОСТЬ**:
|
||||
«символы, которых нет ни в одном смоке: `GENERATED_FURNITURE_ART`,
|
||||
`GENERATED_FURNITURE_MENU`» — выборочная связь не доказана. Это не привело к
|
||||
пропуску смоков: полный browser-smoke набор (все 250 файлов тремя шардами,
|
||||
включая `demo/smoke_furniture.mjs`, `demo/smoke_furniture_lazy_art.mjs`,
|
||||
`demo/smoke_furniture_polish.mjs`) уже прогнан и зелен в Validate `full=true`
|
||||
на этом самом SHA, поэтому неопределённость инструмента закрыта фактическим
|
||||
прогоном, а не игнорированием.
|
||||
|
||||
## Критерии приёмки — разбор
|
||||
|
||||
| AC | Чем доказан | Чем краснеет / комментарий |
|
||||
|---|---|---|
|
||||
| **AC1** видимые границы = `[0,artW]×[0,artH]` ±0.1 | `test/furniture-visual-bounds.test.mjs` → `AC1: рисунок каждого символа заполняет свой физический бокс` (все 56 символов + положительный контроль на числах из шапки issue, `52,615` внутри `60`) | мутант `furniture-symbol-may-keep-inner-padding`: `boxFillDeviation` принудительно возвращает `0`, если `width !== 'mutant-never-a-width'`. Прочитан код теста: положительный контроль требует `paddedDeviation > PLAN_BOUNDS_TOLERANCE`, при мутанте `paddedDeviation` = 0 → assert падает. Подтверждено логически чтением, зачтено по зелёной job «Мутанты по диффу» в Validate |
|
||||
| **AC2** одинаковые `w/h` → одинаковый габарит (`kitchen_floor`/`dishwasher`) | `test/furniture-visual-bounds.test.mjs` → `AC2`, явная пара из шапки issue | защита транзитивно наследуется от AC1 (если оба символа проходят допуск 0.1 по AC1, их внешние экстенты совпадают в пределах того же допуска) — отдельного мутанта не требует, не самостоятельная защита |
|
||||
| **AC3** трансформ сохраняет бокс при угле/зеркалировании/масштабе | `test/furniture-visual-bounds.test.mjs` → `AC3`: прямые углы — равенство, 15°/37° — вложенность и покрытие >97 % | трансформ (`furnitureRenderTransform`) в этом диапазоне не менялся — тест защищает связку «новая геометрия + старый трансформ», не новый код; отдельного мутанта нет, риск регрессии низкий (код неизменённого модуля) |
|
||||
| **AC4** склейка путей сохраняет относительные команды | `test/furniture-path-join.test.mjs`, включая реальные `stairs`/`tv` (единственные фикстуры, где старая склейка реально уводила рисунок за `viewBox` до 119 при 110) | мутант `furniture-paths-joined-without-reset`: `d.startsWith('M') || d.length >= 0` истинно всегда → функция вырождается в `join(' ')`, то есть в старый баг. Проверено логически: тест `'M3.765 40h2 M 56.26 26.96 l-3-3'` даст вместо этого `'M3.765 40h2 m56.26 26.96l-3-3'` → assert падает; тест на `stairs`/`tv` тоже упадёт (`fixed.maxX <= 110` не выполнится). Зачтено по зелёной job «Мутанты по диффу» |
|
||||
| **AC5** `furniture:check` падает на нарушении пп.1/2/5 | Прочитан код: `svgArt()` в `scripts/generate-furniture-assets.mjs` вызывается из `loadPack()` на реальных файлах `assets/furniture/houseplan-0.3.0/**`, а не на фикстурах; `paths.length !== 1`, regex на `m` после `M`, `boxFillDeviation > 0.1` — все три `fail()`. Отдельно автор (владелец, комментарий 2026-09-16T19:33) прогнал ту же функцию на **старом** наборе `dev` и получил 65 нарушений — эмпирическое подтверждение, что гейт различает старое/новое состояние | проверено чтением, не отдельным исполнением с моей стороны; `npm run furniture:check` зачтён по зелёному Validate |
|
||||
| **AC6** golden пересняты, диффы объяснены | Commit `9bf41ec3`: 9 кадров, каждый явно объяснён (6 мебельных — заполнение бокса + попутная правка иконки «Журнальный стол круглый»; 2 декоративных — контур дивана; 1 изометрический). Расхождение 0,16–1,48 % пикселей. Job «Golden-кадры против принятых эталонов» — success на этом SHA | принятие эталонов сделано `Baseline-Reviewed: <прогон 35144164510>`, что соответствует правилу 13 |
|
||||
| **AC7** скоуп ограничен паком/генератором/`furniture.ts`/тестами | `git diff --stat origin/dev...HEAD` — редактор, hit-area, привязка к стенам, схема конфига, i18n не изменены | проверено чтением диффа целиком |
|
||||
| **AC8** гейты зелёные на точном SHA | Validate `9bf41ec3` — оба прогона success (см. таблицу выше) | подтверждено `gh run view` по `headSha`, не на слово автора |
|
||||
|
||||
## Находки
|
||||
|
||||
### M1 — рост ленивого чанка `furniture-plan-art.generated` не назван числом, как того требует собственная ТЗ (Medium, в скоупе)
|
||||
|
||||
**Файлы:** `custom_components/houseplan/frontend/houseplan-assets/furniture-plan-art.generated-*.js`
|
||||
(и синхронные копии в `dist/**`).
|
||||
|
||||
**ТЗ (раздел «Производительность»):** «Число команд в путях выросло… поэтому
|
||||
перед бетой смотрим перф-профиль `large-house` и размер бандла:
|
||||
`furniture-plan-art.generated` лежит в ленивом чанке (#474), **его рост в
|
||||
килобайтах называется в отчёте**». Раздел «Риски» дублирует то же обязательство:
|
||||
«Рост размера ленивого чанка art; называется числом, решение — за владельцем,
|
||||
**если рост окажется заметным**».
|
||||
|
||||
**Измерено самостоятельно** (не входит ни в один автоматический гейт — `scripts/bundle-budget.mjs`
|
||||
проверяет только initial-граф и наличие `lazyFurnitureArtFiles`, размер самого
|
||||
ленивого чанка не бюджетируется):
|
||||
|
||||
```
|
||||
git cat-file blob $(git rev-parse origin/dev:.../furniture-plan-art.generated-Cn3m1_ZV.js) | wc -c # 33655 raw
|
||||
git cat-file blob $(git rev-parse HEAD:.../furniture-plan-art.generated-DH4ULp8h.js) | wc -c # 90517 raw
|
||||
# gzip (node zlib.gzipSync):
|
||||
# old: 10 276 Б
|
||||
# new: 23 383 Б (+13 107 Б, +127 %)
|
||||
```
|
||||
|
||||
Рост более чем вдвое по gzip — именно тот случай «заметности», для которого ТЗ
|
||||
явно резервирует решение владельцу. Число нигде не названо: ни в хендофф-
|
||||
комментарии на код-ревью (2026-09-16T21:00:06Z), ни в теле коммитов
|
||||
`ef3bc6d1`/`9bf41ec3`, ни в CHANGELOG. Раздел «Риски» ТЗ формально остался
|
||||
невыполненным пунктом, хотя весь остальной текст ТЗ выполнен добросовестно.
|
||||
|
||||
**Почему Medium, а не High.** Чанк ленивый (грузится только при наличии
|
||||
мебели на плане), перф-смок в Validate `full=true` зелёный, функционально
|
||||
ничего не сломано — AC1–AC8 не пострадали. Это не дефект поведения, а
|
||||
невыполненное собственное обязательство ТЗ по прозрачности для владельца.
|
||||
|
||||
**Почему в скоупе, не отдельный issue.** Обязательство сформулировано в ТЗ
|
||||
этой же задачи (раздел «Производительность»/«Риски» #584), значит по §2.7/§3.8
|
||||
чинится в этой же задаче, а не отдельным issue.
|
||||
|
||||
**Что нужно для закрытия:** одним комментарием в issue назвать число (gzip
|
||||
и/или raw, до/после) — этого достаточно, продуктовый код менять не требуется.
|
||||
Дальше решение по приемлемости роста — за владельцем, как и написано в ТЗ.
|
||||
|
||||
**Вердикт по находке:** блокирует переход в `S8-merged` только в смысле «жёлтый,
|
||||
не зелёный» — возврат автору на дополнение отчёта, новый цикл реализации кода
|
||||
не требуется.
|
||||
|
||||
### Low — не найдено
|
||||
|
||||
Явных Low-находок, требующих записи, не нашёл: имена файлов, форматирование,
|
||||
структура коммитов и трейлеры в порядке.
|
||||
|
||||
## Что проверено и корректно
|
||||
|
||||
- **Математика легаси `plant`.** Пересчитал независимо: старое поле
|
||||
`0.02…0.98` (span 0.96) растянуто от центра `0.5` в `1/0.96 = 1.041666…`;
|
||||
каждая новая координата и радиус эллипса в диффе `src/furniture.ts`
|
||||
совпадают с этим пересчётом с точностью до 8 знаков (`0.22→0.22916667`,
|
||||
`0.28→0.27083333`, `0.34→0.33333333`, `0.13→0.11458333` и симметричные).
|
||||
Форма (пропорции) сохранена, только масштаб.
|
||||
- **`svg-path-bounds.mjs`.** Прочитан целиком: дуги приводятся к кубикам той
|
||||
же формулой, что в `src/pdf/svg-path.ts` (эллиптическая дуга → серия кубик
|
||||
Безье через центровой угол); экстремумы по производной кубики, а не по
|
||||
контрольным точкам — тест `AC1` явно проверяет это положительным контролем
|
||||
и отдельный тест сверяет два независимых парсера («границы считаются той же
|
||||
математикой, что у продакшен-экспорта») до `1e-6` на всей библиотеке из
|
||||
56 символов.
|
||||
- **`furniture-path-join.mjs`.** Прочитан целиком, логика («канонизируется
|
||||
только начальный `moveto`, неявные пары после `m` остаются относительными
|
||||
`l`») соответствует и ТЗ п.6, и математике SVG-путей. Тест на реальных
|
||||
`stairs.svg`/`tv.svg` (иконки меню, не тронутые диффом сами по себе, но
|
||||
проходящие через тот же склеивающий код) независимо подтверждает: старая
|
||||
склейка выводила деталь за `viewBox 110×110` (`naive.maxX/maxY > 110`), новая
|
||||
укладывается и побайтно совпадает с «каждый путь считался от своего нуля».
|
||||
Diff `src/furniture-menu-art.generated.ts` для `id:"stairs"` глазами: было
|
||||
`m73.113 28.633-41.492 32.93…` (относительный перенос, продолжающий
|
||||
координаты предыдущего подпути), стало `M 73.113 28.633 l -41.492 32.93…`
|
||||
(абсолютный перенос + явная относительная линия) — то есть вторая находка
|
||||
из хендоффа (не заявленная в шапке issue) подтверждена и в исходном коде, и
|
||||
тестом, и объяснена в golden/changelog.
|
||||
- **Пак дизайнера принят добросовестно.** `pack.json` и `svg/menu/**`
|
||||
побайтно совпадают с `dev` (`git diff --stat` не показывает эти пути);
|
||||
изменились ровно 44 файла `svg/plan`, что соответствует заявлению автора.
|
||||
- **Скоуп (AC7).** Полный `git diff --stat origin/dev...HEAD` не содержит
|
||||
`src/houseplan-card.ts` и i18n-файлов — редактор, hit-area, привязка к
|
||||
стенам, конфиг и переводы не затронуты.
|
||||
- **Трейлеры и класс изменений.** `ef3bc6d1`: `Issue: #584`,
|
||||
`User-Visible: yes` — верно (пользователь видит другой размер мебели).
|
||||
`9bf41ec3`: `Issue: #584`, `User-Visible: no`, `Release: v1.77.0-beta.1`,
|
||||
`Baseline-Reviewed: <прогон>` — верно для коммита класса D (только golden +
|
||||
фингерпринт скриншотов), правило §10.2.5 выполнено.
|
||||
- **CHANGELOG.** Оба файла (`docs/CHANGELOG.md`, `docs/CHANGELOG.ru.md`)
|
||||
правлены в том же коммите `ef3bc6d1`, что и поведенческая правка; текст
|
||||
описывает видимое пользователю изменение (в т.ч. попутную находку про
|
||||
иконки меню), а не детали реализации.
|
||||
- **Touch/security.** ТЗ явно фиксирует «нет влияния» с обоснованием
|
||||
(hit-area/halo считаются от физического бокса, а не art-координат) —
|
||||
проверено чтением: диф не касается кода хэндлов/hit-area/halo. Security:
|
||||
диф не трогает сетевые вызовы, права, парсинг пользовательского ввода;
|
||||
`svgArt()` по-прежнему отклоняет активный XML/внешние ссылки/посторонние
|
||||
теги (не менялось в этой части, кроме дополнительных запретов на несколько
|
||||
`<path>` и относительную `m` у плановых символов — это ужесточение, не
|
||||
ослабление).
|
||||
- **«Одно число — один источник».** Новых пользовательских величин, видимых
|
||||
дважды, диф не вводит: ширина/глубина мебели по-прежнему только
|
||||
пользовательский ввод в свойствах, geometрия символа не показывается как
|
||||
отдельное число нигде в UI. Не применимо.
|
||||
|
||||
## Чего не проверял
|
||||
|
||||
- **`npx tsc --noEmit`, `npm test`, `npm run build` + сверка бандлов, `check-docs.mjs`,
|
||||
browser-смоки, `golden:verify`** — не гонял их лично на своей машине; зачёл
|
||||
по двум независимо проверенным зелёным прогонам Validate на точном
|
||||
`headSha = 9bf41ec3` (`35149944216` — light, `35145083038` — `full=true`).
|
||||
Это соответствует «дешёвые гейты на этом SHA уже подтверждены» — рассудил,
|
||||
что повторный прогон тех же детерминированных команд на том же дереве не
|
||||
добавляет доказательной силы, только тратит время.
|
||||
- **`mutation-gate` (весь набор, `--check`)** — не запускал сам; логику двух
|
||||
новых мутантов проверил чтением (таблица AC выше) и зачёл по зелёным
|
||||
job'ам «Мутанты по диффу (1–6/6)» в Validate `full=true` на этом же SHA.
|
||||
- **Полный перф-профиль `large-house-isometric`/`large-house-interaction`** —
|
||||
не относится к этому диффу (не тронуты `src/iso-*`, `src/live-*`,
|
||||
`src/render-*`); в Validate `full=true` отработал перф-смок «бюджет времени
|
||||
кадра», success.
|
||||
- **Визуальный просмотр самих PNG эталонов пиксель-в-пиксель** — не открывал
|
||||
9 принятых кадров глазами лично; полагаюсь на объяснение каждого кадра в
|
||||
коммите `9bf41ec3` (плюс зелёный `golden:verify` job на этом SHA) —
|
||||
разумно для Medium-риска «замаскированной регрессии», но это не то же
|
||||
самое, что собственный визуальный осмотр. Если бы диф расширялся дальше
|
||||
мебели, я бы настоял на визуальном осмотре.
|
||||
- **Реальный размер прироста в production build локально** — измерил через
|
||||
`git cat-file` + `zlib.gzipSync` по блобам коммитов, не через полный
|
||||
`npm run build` у себя (совпадает с уже подтверждённым зелёным build в
|
||||
Validate).
|
||||
- **`model-invariants.mjs`, `pytest tests_backend`** — не применимо, диф не
|
||||
трогает геометрию комнат/стен ни backend.
|
||||
|
||||
## Вердикт
|
||||
|
||||
**Жёлтый · заход r1 · блокирующих циклов 1/4 · High: 0 · Medium: 1 → в задаче.**
|
||||
|
||||
Реализация по существу сильная: контракт границ проверен независимым
|
||||
пересчётом (не на слово автора), два новых мутанта логически доказано красят
|
||||
защищаемые тесты, попутная находка про иконки меню `stairs`/`tv` подтверждена
|
||||
и в исходном SVG, и тестом на реальных фикстурах, скоуп не расширен, трейлеры
|
||||
и changelog в порядке, оба прогона Validate на точном SHA (light и `full=true`)
|
||||
зелёные и проверены по `headSha`, а не на слово.
|
||||
|
||||
Единственная блокирующая находка (Medium, в скоупе) — M1: собственное
|
||||
обязательство ТЗ «рост ленивого чанка называется числом в отчёте» не
|
||||
выполнено, хотя измеренный рост (gzip 10 276 → 23 383 Б, +127 %) как раз тот
|
||||
случай, который ТЗ называет «заметным» и оставляет на решение владельца.
|
||||
Закрывается одним комментарием с числом, без нового цикла кода.
|
||||
|
||||
---
|
||||
|
||||
<!-- material-anchors: сгенерировано конвейером (#414) -->
|
||||
|
||||
## Материал раунда
|
||||
|
||||
- Ветка: `issue/584-furniture-physical-bounds`, коммит `9bf41ec305be` — ребейз его осиротит, и это нормально: ниже якоря, которые ребейз не меняет.
|
||||
- Дерево материала: `cad9319e181df6c13ffa64ccde7471c59adfa4be`
|
||||
```
|
||||
git log --all --format='%H %T' | grep cad9319e181d
|
||||
```
|
||||
- Тело issue: `2b6d41955420844101142131ce4f789615f0ac788609ae654f4b416448b03b9b`
|
||||
- Вердикт конвейера: `yellow` · High 0
|
||||
Reference in New Issue
Block a user