26 KiB
CODE-REVIEW-584-r1 — #584: физический габарит мебели равен заявленным размерам
- Issue: #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 - Заход: r1 (первый заход код-ревью; §2.10 «по дельте» не применяется — это не повторный раунд)
- Блокирующих циклов израсходовано: 0/4
- Вердикт: жёлтый
Скоуп ревью
Два коммита на материале 9bf41ec3:
ef3bc6d1(класс A/B/D,User-Visible: yes) — продуктовая правка:src/furniture.ts(legacyplant), новые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') |
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), новая укладывается и побайтно совпадает с «каждый путь считался от своего нуля». Diffsrc/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)» в Validatefull=trueна этом же SHA.- Полный перф-профиль
large-house-isometric/large-house-interaction— не относится к этому диффу (не тронутыsrc/iso-*,src/live-*,src/render-*); в Validatefull=trueотработал перф-смок «бюджет времени кадра», success. - Визуальный просмотр самих PNG эталонов пиксель-в-пиксель — не открывал
9 принятых кадров глазами лично; полагаюсь на объяснение каждого кадра в
коммите
9bf41ec3(плюс зелёныйgolden:verifyjob на этом 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 %) как раз тот случай, который ТЗ называет «заметным» и оставляет на решение владельца. Закрывается одним комментарием с числом, без нового цикла кода.
Материал раунда
- Ветка:
issue/584-furniture-physical-bounds, коммит9bf41ec305be— ребейз его осиротит, и это нормально: ниже якоря, которые ребейз не меняет. - Дерево материала:
cad9319e181df6c13ffa64ccde7471c59adfa4begit log --all --format='%H %T' | grep cad9319e181d - Тело issue:
2b6d41955420844101142131ce4f789615f0ac788609ae654f4b416448b03b9b - Вердикт конвейера:
yellow· High 0