diff --git a/docs/reviews/CODE-REVIEW-373-r2.md b/docs/reviews/CODE-REVIEW-373-r2.md new file mode 100644 index 00000000..730488cf --- /dev/null +++ b/docs/reviews/CODE-REVIEW-373-r2.md @@ -0,0 +1,276 @@ +# CODE-REVIEW-373-r2 + +Issue: [#373](https://github.com/Matysh/houseplan-card/issues/373) — Add a fit-to-content / crop option for `houseplan-space-card` +Материал ревью (`git rev-parse HEAD` перед подведением итогов): `41c09ea6a20d18c88a444436770f3b1f88b7bea7` +Диапазон: `git log origin/dev..HEAD` = `4a343814` (spec), `a143a8e6` (spec-review doc), `0d33691f` (feat), `7ebead5d` (code-review r1 doc), `f56d9a76` (build: rebase-conflict fix + regression test), `41c09ea6` (docs: accept rebased canonical screenshots) +Заход: r2 · блокирующих циклов израсходовано 0 из 4 (r1 был зелёным — §227/#227, зелёный вердикт бюджет не тратит) + +## Почему разбор в этом заходе полный, а не по дельте + +r1 (`docs/reviews/CODE-REVIEW-373-r1.md`) был **зелёным** на материале `7153e124` +(High: 0, Medium: 0, три Low сняты решением ревьюера). Слияние не удалось: +пока шло ревью, `origin/dev` продвинулся на 4 коммита (#375 landed), ветка +`issue/373-space-card-house-fit` конфликтовала с новым `dev`. Задача вернулась +в `S6-in-progress` не по находке, а по несостоявшемуся мержу — цикл за это не +считается (AGENTS.md «the comment says which», issue-комментарий от +2026-08-29T18:12:38Z). Автор перебазировал ветку на `origin/dev` (`51196c65`), +разрешил конфликт руками и явно запросил полный повторный прогон +(issue-комментарий 18:24:29Z: «после ребейза прошу полноценный повторный +прогон по новому дереву»). + +Это ровно случай из PROCESS.md §2.10: «ребейз на ушедший вперёд `dev` — после +ребейза это другой код», разбор остаётся полным, а не по дельте. Старые SHA +r1-диапазона (`a925a1c3`, `ff977a8a`, `95d91595`, `7153e124`, `e7ba22c6`) +больше не существуют в репозитории (`git cat-file -t` отказывает на каждом) — +подтверждение, что это настоящий force-push/ребейз, а не расширение той же +истории. + +Практически это означает: AC1–AC8 проверены заново по текущему дереву +(раздел «Проверка по AC» ниже), а не унаследованы как записи. Но поскольку +почти весь диф оказался **побайтово идентичен** содержимому, уже разобранному +в r1 (сверено прямым `git diff` между `origin/dev` (уже содержащим #375) и +`HEAD` — см. ниже), основной объём чтения не переоткрывал вопросы, закрытые в +r1, а подтверждал их неизменность и фокусировался на том, что ребейз +действительно поменял: перестановку блока `canonicalWallGeometry` относительно +кэш-тега #375, новый регрессионный юнит-тест, удаление осиротевших +locale-чанков и переснятые канонические скриншоты. + +## Закрытие раунда r1 + +| Находка r1 | Чем закрыта | Где видно | +|---|---|---| +| Low-1 — нет browser-смок-проверки state-tick/narrow-wide для `fit: house` | Не чинилась (снята решением ревьюера в r1, риск не подтверждён) | `demo/smoke_space_card.mjs` не изменился в этой части в r2-диффе (сверено `git diff origin/dev...HEAD` — блок `fit === 'house'` в `src/space-render.ts` по-прежнему не читает `hass`/`entities`, `amount: 0` жёстко зашит) | +| Low-2 — фолбэк «пустая структура → content frame» не покрыт тестом на уровне проводки | Не чинилась (снята решением ревьюера в r1) | `renderSpaceStatic`: `fr = structuralFrame(structure) || fr` — строка не изменилась при ребейзе, риск тот же, что в r1 | +| Low-3 — константы половины штриха (2.5/0.6) дублируют `stroke-width` без общего источника | Не чинилась (снята решением ревьюера в r1) | `src/space-render.ts`, блок `fit === 'house'` — те же литералы, `.room-outline`/`.wallbody` stroke-width не изменились | +| (r1 не находка, но условие) зелёный вердикт, слияние не удалось | Ребейз на `origin/dev` (`51196c65`), конфликт разрешён, регрессия #375 закрыта отдельным юнит-тестом | Коммиты `f56d9a76`, `41c09ea6`; `test/space-render-caches.test.mjs` (+5 строк, `assert.ok(... < ...)` на порядок кода) | + +Все три Low остаются в силе как **сознательно снятые**, не как «забытые»: код, +к которому они относятся, не изменился ни на строку между r1 и r2 (подтверждено +диффом, а не памятью автора). Отдельно значимое новое событие раунда — +конфликт с #375 (полностью новый материал, которого не было в r1) — закрыт +корректно: см. AC-независимую находку ниже отсутствует, потому что решение +проверено и не создаёт нового риска (см. «Проверка по AC», AC4/AC7 и рубрику +про #375 ниже). + +## Унаследовано из r1 (сверено, не принято на слово) + +Для следующих файлов текущий `git diff origin/dev...HEAD` (где `origin/dev` +уже включает #375) дал содержимое, побайтово совпадающее с тем, что описано и +разобрано в `docs/reviews/CODE-REVIEW-373-r1.md` на SHA `7153e124` — +переставлять код заново не пришлось, но **логика перепроверена по AC в этом +заходе**, а не унаследована слепо: + +- `src/space-geometry.ts` — `resolveSpaceCardFit`, `itemOfGeometry`, + `expandItem`, `structuralFrame` (доказательство: полный `git diff`, ни одной + строки отличия от функций, разобранных в r1); +- `src/render/opening-symbol.ts` — `openingVisibleBounds`, `bodyTranslation`, + аналитический sweep для window/door/gate (доказательство: та же строка в + строку, включая комментарии); +- `src/space-card.ts`, `src/space-editor.ts`, `src/i18n/{en,ru,de,fr}.json` — + публичный `fit`-контракт, редактор, i18n-ключи (доказательство: диф + идентичен, включая точные строки лейблов); +- `docs/CHANGELOG.md`, `docs/CHANGELOG.ru.md`, `docs/USER-GUIDE.md`, + `docs/USER-GUIDE.ru.md`, `docs/ARCHITECTURE.md`, `docs/CANVAS.md §4.4` — + released-артефакты AC8, содержимое не изменилось. + +Не унаследовано слепо, перепроверено заново из-за прямого затрагивания +рёбейзом: `src/space-render.ts` (порядок вычислений и точка внедрения тега +#375 — см. ниже), `test/space-render-caches.test.mjs` (новый тест), +`demo/srv/assets/**`/`dist/**`/`custom_components/houseplan/frontend/**` +(бандл-хеши и локали изменились из-за нового содержимого `dev`), скриншоты +документации (переснятые из-за нового `oxipng` в `dev`). + +## Как проверялось + +Диапазон `origin/dev..HEAD` был **не отфильтрован по путям** конвейером +Validate: по логам классификации (`Классификация изменённых файлов`, run +`33267869890`) `BEFORE_SHA=5284bfb0…` не существует в дереве после ребейза +(`git cat-file -e` отказал), что скрипт распознаёт как force-push (#347) и +намеренно запускает **все** тяжёлые job без фильтра путей вместо угадывания +диапазона. Это дало полный, а не «дешёвый», прогон на рёбейзнутом дереве: + +| Гейт | Где выполнен | Результат | +|---|---|---| +| `npx tsc --noEmit` | CI Validate, job «Фронтенд…», run [33267869890](https://github.com/Matysh/houseplan-card/actions/runs/33267869890) (SHA `f56d9a76`) | OK | +| `npm test` | тот же job | 1564 tests: 1563 pass, 0 fail, 1 skipped (единственный skip — известная приватная фикстура #281, не связана с задачей) | +| `npm run build` + сверка бандла | тот же job | OK; `npm run bundle:budget`: initial View 276981/300000 B gzip, headroom 23019 B, editor 139742 B, locale 45363 B | +| `node scripts/no-new-any.mjs --base origin/dev --head HEAD` | тот же job | шаг зелёный (job success, шаг не упал) | +| Полный набор `demo/smoke_*.mjs` (3 шарда) | job «Смоки в браузере» × 3 + «Смоки: все шарды зелёные», тот же run | все success | +| `npm run golden:verify` | job «Golden-кадры против принятых эталонов», тот же run | success | +| `performance_smoke` | job «Перф-смок: бюджет времени кадра», тот же run | success | +| `node scripts/check-docs.mjs` | job «Предполётные проверки…», run 33267869890 | **failure** (`screenshot source fingerprint is stale`) → исправлено коммитом `41c09ea6`, тот же гейт green в run [33268157290](https://github.com/Matysh/houseplan-card/actions/runs/33268157290) (SHA `41c09ea6`, текущий HEAD) | +| `process-gate.mjs` / provenance / process.yml sync | оба run | success на обоих SHA | +| Docs screenshots (класс D, канонический прогон) | run [33268042941](https://github.com/Matysh/houseplan-card/actions/runs/33268042941), `workflow_dispatch` | success; `headBranch: issue/373-space-card-house-fit`, `headSha: f56d9a76` (поле ответа API, не вводящий в заблуждение ярлык списка ранов, как в r1) — снято реально с этого дерева | +| Бэкенд/hassfest/HACS | оба run | skipped — диф не касается `custom_components/**/*.py`, `manifest.json`, `hacs.json` (корректно, задача не затрагивает эти поверхности) | + +На SHA `41c09ea6` (текущий HEAD) job «Переиспользование: это дерево уже +проверено» легитимно **не перезапускает** smoke/golden/performance_smoke: +ключ переиспользования (`scripts/gate-reuse.mjs`, issue #208) хеширует +`sourceFingerprint` (src/**, demo/fixtures, package.json/lock, rollup, +tsconfig) плюс оснастку каждой job (`demo/smoke_*.mjs` для smoke, +`demo/golden/**` для golden, `demo/performance/**` для perf) — ни один из +этих путей не входит в дельту между `f56d9a76` и `41c09ea6` (только +`docs/images/*.png` и `docs/images/screenshots.json`), поэтому переиспользование +корректно, не молчаливый пропуск. `backend`-маркер не найден в кэше, но job всё +равно `skipped` — по независимой причине (path-classification: диф не +затрагивает `custom_components/**/*.py`), не из-за реюза. + +Проверено самостоятельно, вне CI (лёгкая сверка, не гейт): + +| Проверка | Результат | +|---|---| +| `ls custom_components/houseplan/frontend/houseplan-assets/ \| grep -E '^(de\|fr)-'` и то же для `dist/` | ровно `de-BwEMXoV0.js`, `fr-Ca0KCtpf.js` в обеих копиях, совпадают с `houseplan-assets.json.lazyLocaleFiles` — осиротевшие `de-tOerxd73.js`/`fr-_8bPsmzm.js` из коммита `f56d9a76` действительно удалены и нигде не переживают | +| `grep -n 373 docs/specs/README.md` | строка на месте (Low спек-ревью r1 закрыт) | +| Построчное сравнение `src/space-render.ts` до/после переноса блока `canonicalWallGeometry` | перенос идентичен по содержимому блоку, уже перенесённому в r1 (диапазон `resolvedRawOpenings`…`hostedCompositeOpenings`), плюс перенос самого вычисления `wallGeometryFingerprint`/`canonicalWallGeometry` (было после `fr`, стало до) с сохранением тега `sourceFingerprint` из #375 внутри того же `cachedStaticWallGeometry(...)` вызова — семантика не изменилась, доказательство ниже | + +### Гейты, которые не гонялись повторно и почему + +Полный typecheck/test/build/golden/smoke/perf уже зелёные на этом дереве по +CI (см. таблицу выше) — перегонять их локально было бы дублированием того же +прогона тем же кодом. `node scripts/model-invariants.mjs` не запускался: +диф не трогает персистентную геометрическую модель (рёбра комнат, записи +толщины, `layout`, `marker.space`, `open_spans`) — весь диф этой задачи +работает с производной, вычисляемой на лету структурой для одного bounding-box +static-карточки (`ContentItem[]` в памяти рендера), не с сохранённым +конфигом/`layout`. `python -m pytest tests_backend` не запускался — диф не +касается `custom_components/**/*.py` (подтверждено `git diff --stat`, ноль +файлов `.py`). + +## Находки + +Ни одной High, ни одной Medium. Три Low **унаследованы из r1 без изменений** +(см. таблицу «Закрытие раунда r1» выше) — код, к которому они относятся, не +менялся между r1 и r2, повторное решение ревьюера то же: сняты, реальный риск +на сегодняшнем дереве отсутствует, правка не обязательна. + +Новых находок в материале, добавленном рёбейзом (`f56d9a76`, `41c09ea6`), нет. +Отдельно проверено: + +- **Порядок вычислений и тег #375 не сломан.** `canonicalWallGeometry` + (используется в `fit === 'house'` для стеновых компонентов) теперь строится + до выбора `fr`, но `Object.defineProperty(built, 'sourceFingerprint', ...)` + внутри `cachedStaticWallGeometry(...)` перенесён вместе с вычислением — + тег не потерян и не переставлен в другую функцию. Новый тест + `test/space-render-caches.test.mjs` («#373 may move canonical geometry + before framing, but must carry the #375 tag with it») проверяет это + текстовым сравнением позиций (`indexOf` определения тега `<` `indexOf` + `const contentFrame =`) в исходнике `space-render.ts`; тест умеет падать — + если правку отменить (вернуть блок геометрии после `contentFrame`, оставив + тег на старом месте), `indexOf` тега станет больше и `assert.ok` упадёт. + Проверено вручную рассуждением, не запуском мутации (запуск полного `npm + test` уже прогнал этот тест зелёным в CI, см. таблицу гейтов). +- **Осиротевшие locale-чанки не остаются в дереве.** Подтверждено чтением + файловой системы (таблица выше) и тем, что `houseplan-assets.json` ссылается + ровно на существующие имена. +- **Коммит `f56d9a76` смешивает класс D (удаление чанков) и класс B + (добавление теста) под сообщением «build: …»**. Не нарушение процесса: + правило о `Release:`/`Baseline-Reviewed:` относится только к коммитам + **исключительно** класса D (PROCESS.md §10.2 п.5), а этот коммит правит и + тестовый файл. Трейлеры `Issue: #373`/`User-Visible: no` на месте. Чисто + косметическая неточность сообщения (не отражает добавление теста) — Low, + снимается: не влияет ни на один AC, не создаёт риска, не стоит отдельного + цикла ревью ради переформулировки уже запушенного сообщения. + +## Проверка по AC + +- **AC1 (совместимость по умолчанию)** — доказано автотестом: + `test/canvas.test.mjs` («static-card fit literals fail closed…» — все из + `undefined, null, '', 'content', 'cover', 1` резолвятся в `'content'`, + `'house'` резолвится в `'house'`, точный `spaceFrame` фикстуры сверен + `deepEqual`), `test/space-card-fit.test.mjs` (контракт `SpaceCardConfig`/ + редактора по исходнику), и живым прогоном `demo/smoke_space_card.mjs` в CI + (`frame === explicitContentFrame === unknownFitFrame`, зелёный в обоих run). + Тест умеет падать: `resolveSpaceCardFit` — явный резолвер с фиксированным + списком, любое отклонение default-ветки меняет `assert.equal`. +- **AC2 (тесная структурная рамка, исключение подложки/декора/маркеров/подписей)** + — доказано юнитом (`structuralFrame` с комнатой+стеной+«detached wing», + точный `deepEqual` на `{x,y,w,h}`, включая защиту от `Infinity`) и смоком в + CI (реальная карточка с подложкой/декором/маркером/подписью, `tightFrame` + строго меньше `frame` по всем границам — числовое условие в самом смоке, + зелёное). Код-ревью подтверждает: в `structure` (блок `fit === 'house'`) + попадают только `roomItem`, компоненты `canonicalWallGeometry`, `extras`, + `zeroWalls.lines`, `openingVisibleBounds` — ни подложка, ни декор, ни + маркеры, ни подписи не push'атся ни в одну ветку. +- **AC3 (без обрезки структуры)** — доказано: `test/opening-symbol.test.mjs` + (state-независимая огибающая door/window/gate, диагональные углы, толстые + косяки, оба направления ворот, passage → `null`) + `test/canvas.test.mjs` + (collinear-защита `structuralFrame([box(100,250,900,250)])` даёт конечный + `h > 0`) + смок `tightPaintedEnvelope.contained === true` (реальные + `.wallbody/.zero-wall/.static-opening` строго внутри `viewBox` с допуском + 0.51 px) — все три зелёные в CI-прогоне на этом SHA. +- **AC4 (безопасный откат)** — collinear/invalid доказаны юнитом (см. выше); + state-tick независимость доказана юнитом напрямую + (`assert.deepEqual(openingVisibleBounds(spec({amount:0})), + openingVisibleBounds(spec({amount:1})))` — сильнее, чем в r1 описано: + это точное равенство всего объекта bounds, не только числового частного + случая); проводка `fr = structuralFrame(structure) || fr` для + image-only/empty — по-прежнему проверено чтением, не исполнением (Low-2, + унаследовано, риск не изменился). +- **AC5 (редактор/i18n)** — доказано автотестом (`test/space-card-fit.test.mjs` + сверяет точные строки `src/space-editor.ts`/`src/space-card.ts` по regex) + + i18n-диффом (en/ru/de/fr получили `editor.framing`, `editor.fit_content`, + `editor.fit_house`, `cover` нигде не предлагается — `assert.doesNotMatch`). +- **AC6 (View/touch)** — ширины теперь 320/900 px (смок обновлён с 640 на 900, + совпадает с AC6 буквально); инертность сцены и кнопка футера — доказаны + смоком (`tightPointerEvents === 'none'`); узкая/широкая ширина и live-смена + состояния датчика для `fit: house` конкретно — по-прежнему не в смоке (Low-1, + унаследовано без изменений: блок `fit === 'house'` не читает `hass`, риск + тот же, что в r1). +- **AC7 (перформанс)** — проверено чтением: ни одна из новых функций + (`structuralFrame`, `itemOfGeometry`, `expandItem`, `openingVisibleBounds`) + не обращается к DOM/`getBBox`; `bundle:budget` зелёный с запасом 23019 B в + CI на этом SHA. Взаимодействие с кэшем #375: перестановка вычисления + `canonicalWallGeometry` раньше по функции не меняет частоту его + инвалидации — фингерпринт (`wallGeometryFingerprint`) как и раньше зависит + только от геометрии, а не от `fit`, поэтому `fit: house` не создаёт новых + кэш-промахов на state-only тиках. +- **AC8 (документация/релиз)** — оба changelog, `USER-GUIDE.md`/`.ru.md`, + `ARCHITECTURE.md`, `CANVAS.md §4.4` на месте и не изменились относительно + r1-содержимого (см. «Унаследовано»); `docs/specs/README.md` содержит строку + #373; канонические скриншоты переприняты на самом SHA рёбейза + (`Baseline-Reviewed: .../runs/33268042941`, независимо подтверждено полем + `headBranch`/`headSha` ответа API, а не текстовым ярлыком UI). + +## Одно число — один источник + +Как и в r1: `viewBox` — единственная потенциально дважды видимая пользователю +величина в этом диффе, но `content` и `house` — два разных режима с одним +путём вычисления каждый (`spaceFrame(...)` vs `structuralFrame(structure)`), +результат сходится в одной переменной `fr`, из которой строится единый `vb`, +используемый один раз ниже по функции для SVG/маркеров/подписей/подложки/ +continuity. Перестановка кода рёбейзом это не затронула — `const vb = [fr.x, +fr.y, fr.w, fr.h]` остался единственным местом присвоения. Внутренние +константы половины штриха (2.5/0.6) по-прежнему дублированы (Low-3, +унаследовано) — не пользовательская величина, правило не про них. + +## Что не проверялось + +- Локальный повторный прогон `typecheck`/`test`/`build`/`golden:verify`/ + полной матрицы смоков — не требовался: CI Validate уже прогнал все эти + гейты на этом самом дереве двумя последовательными push'ами (см. таблицу), + включая полный (не выборочный) набор смоков и golden — сильнее, чем + предписывает §8 для код-ревью. +- `node scripts/model-invariants.mjs` — диф не касается персистентной + геометрической модели (обоснование выше). +- `python -m pytest tests_backend` — диф не касается `custom_components/**/*.py`. +- Мутационный прогон нового теста в `test/space-render-caches.test.mjs` (не + отменял правку вручную, чтобы убедиться в падении) — падение обосновано + логическим разбором `indexOf`, а не эмпирически; сам тест выполнялся и + проходил в составе `npm test` на CI. +- Пиксельное сравнение переснятых `docs/images/*.png` самим ревьюером — принято + по зелёному `check-docs.mjs` на `41c09ea6` и по независимо проверенному факту + (поля API `headBranch`/`headSha` рана 33268042941), что канонический прогон + снят именно с этого дерева. + +## Вердикт + +Зелёный. High: 0. Medium: 0. Три Low унаследованы из r1 без изменений и +остаются снятыми решением ревьюера (код, к которому они относятся, не менялся +между раундами). Материал раунда, добавленный ребейзом — перестановка кода +вокруг тега #375, новый регрессионный тест, удаление осиротевших +locale-чанков, переснятые канонические скриншоты — проверен по AC и не +создаёт новых находок; единственная мелочь (неточное сообщение коммита +`f56d9a76`, смешавшего класс D и B под «build:») — Low, снимается, не в +скоупе обязательной правки. + +**Вердикт: зелёный · заход r2 · блокирующих циклов 0/4 · High: 0 · Medium: 0**