Files
houseplan-card/docs/reviews/CODE-REVIEW-584-r1.md
2026-09-16 21:12:40 +00:00

26 KiB
Raw Permalink Blame History

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 (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')
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 %) как раз тот случай, который ТЗ называет «заметным» и оставляет на решение владельца. Закрывается одним комментарием с числом, без нового цикла кода.


Материал раунда

  • Ветка: issue/584-furniture-physical-bounds, коммит 9bf41ec305be — ребейз его осиротит, и это нормально: ниже якоря, которые ребейз не меняет.
  • Дерево материала: cad9319e181df6c13ffa64ccde7471c59adfa4be
    git log --all --format='%H %T' | grep cad9319e181d
    
  • Тело issue: 2b6d41955420844101142131ce4f789615f0ac788609ae654f4b416448b03b9b
  • Вердикт конвейера: yellow · High 0