From 68cfbdff6901311d1598e917e1f806656d44e04b Mon Sep 17 00:00:00 2001 From: "claude[bot]" <209825114+claude[bot]@users.noreply.github.com> Date: Sat, 22 Aug 2026 11:00:12 +0000 Subject: [PATCH] docs: review document for #239 Issue: #239 User-Visible: no --- docs/reviews/CODE-REVIEW-239-r1.md | 188 +++++++++++++++++++++++++++++ 1 file changed, 188 insertions(+) create mode 100644 docs/reviews/CODE-REVIEW-239-r1.md diff --git a/docs/reviews/CODE-REVIEW-239-r1.md b/docs/reviews/CODE-REVIEW-239-r1.md new file mode 100644 index 00000000..84c0490c --- /dev/null +++ b/docs/reviews/CODE-REVIEW-239-r1.md @@ -0,0 +1,188 @@ +# CODE-REVIEW-239-r1 + +- Issue: [#239](https://github.com/Matysh/houseplan-card/issues/239) — масштаб сетки не должен менять внешний вид плана +- Этап: код-ревью, заход **r1** (первый заход код-ревью; ТЗ уже прошло свои r1/r2 отдельно и здесь не пересматривается) +- Ветка/SHA: `issue/239-grid-scale-invariance` @ `0d91c1e18e60bf2fa3adafd6a57bbfb4df3751ba` +- ТЗ: `docs/specs/239-grid-scale-invariance.md`, ревью ТЗ зелёное (`SPEC-REVIEW-239-r2.md`) +- Вердикт: **жёлтый** · High: 0 · Medium: 1 (в скоупе) + +## Скоуп + +Диапазон `origin/dev...HEAD`, один продуктовый коммит `0d91c1e` (`Issue: #239`, +`User-Visible: yes`). 35 файлов, из них по классам: + +- **A** (продукт): `src/grid-scale.ts` (новый), `src/render/opening-symbol.ts`, + `src/houseplan-card.ts`, `src/styles.ts`, `src/space-card.ts`, + `src/space-render.ts`, `src/space-geometry.ts`, `src/types.ts`, + `src/i18n/{en,ru}.json`; +- **B** (гейты/инструменты): `demo/smoke_grid_scale_invariance.mjs` (новый), + `demo/smoke_space_scale_defaults.mjs` (новый), `scripts/mutation-gate.mjs` + (+12 мутантов), `test/grid-scale.test.mjs` (новый), правки + `test/canvas.test.mjs`, `test/opening-symbol.test.mjs`, + `test/isometric-contract.test.mjs`, `test/open-passage-contract.test.mjs`, + `tsconfig.test.json`; +- **C** (документация): `docs/CHANGELOG.md`/`.ru.md`, `docs/USER-GUIDE.md`/`.ru.md`, + `docs/CANVAS.md`, `docs/ARCHITECTURE.md`, `docs/TESTING.md`, + `docs/specs/README.md`, `docs/specs/239-*.md`, `docs/reviews/SPEC-REVIEW-239-r{1,2}.md`; +- **D** (сгенерированное): три копии `houseplan-card.js`, `docs/images/03-space-create.png`, + `docs/images/screenshots.json`. + +Никаких файлов `custom_components/**/*.py` — backend вне скоупа, как и заявлено +в ТЗ (§5.2, «миграция не нужна»). + +## Как проверялось + +### Гейты + +| Гейт | Команда | Результат | +|---|---|---| +| typecheck | `npx tsc --noEmit` | green | +| unit | `npm test` | 1056/1056 pass, 0 fail, 0 skip (см. примечание ниже) | +| build + сверка бандлов | `npm run build && cmp dist/houseplan-card.js custom_components/houseplan/frontend/houseplan-card.js && cmp dist/houseplan-card.js demo/srv/assets/houseplan-card.js` | green, три копии побайтно идентичны; `git status` после сборки чист | +| docs fingerprint | `node scripts/check-docs.mjs` | green («7 files, 10 external links») — обязателен, diff трогает `src/**` | +| mutation gate | `node scripts/mutation-gate.mjs --check` | все 12 новых `grid-scale-*` мутантов и вся существующая таблица — `ok` | +| целевой smoke (AC2/3/4/6/7/8/13/14/15) | `node demo/smoke_grid_scale_invariance.mjs` | **нестабилен**, см. находку M1 ниже — 6 красных из 14 прогонов, всегда на одной и той же проверке | +| целевой smoke (AC9/10/11/12) | `node demo/smoke_space_scale_defaults.mjs` | green, 3/3 прогона подряд | +| именованные regression smokes (по grep изменённых символов `gridVisualScale`/`gridVisualUnits`/`op-outline`/`op-hit`/`ISO_WALL_HEIGHT`) | `smoke_opening_preview`, `smoke_opening_measure`, `smoke_inert_openings`, `smoke_open_passage`, `smoke_registryless_opening`, `smoke_styling_hooks`, `smoke_pan_any_zoom`, `smoke_plan_snap_overlay`, `smoke_free_walls`, `smoke_decor`, `smoke_space_card`, `smoke_isometric_contract`, `smoke_isometric_live_touch` | все green, по одному прогону каждый | + +**Не прогонялось и почему:** `npm run golden:verify`, полный набор из 163 +браузерных smoke и `performance_smoke` — по §8/§11.4 это предрелизный гейт, не +гейт код-ревью; `python -m pytest tests_backend` — diff не затрагивает +`custom_components/**/*.py`, гейту нет предмета. + +Примечание по unit-тестам: хендофф автора заявляет «1055 pass, 1 штатный +skip», в этом прогоне — 1056 pass, 0 skip. Расхождение не влияет на вердикт +(итог зелёный в обоих случаях), но фиксирую как замеченное, а не подтверждённое +слово в слово. + +### AC — чем доказан каждый + +Сверено с матрицей §13.1 ТЗ; ниже — что именно я проверил (тест либо чтение). + +| AC | Доказательство | Проверено | +|---|---|---| +| AC1 | `test/grid-scale.test.mjs` | unit зелёный, значения проверены вручную (5/1/2.54/10/25, invalid→1) | +| AC2 | `smoke_grid_scale_invariance.mjs` (flat View) | метрики и pixel diff совпадают **кроме** случайного провала на dark theme, см. M1 | +| AC3 | opening unit tests + тот же smoke | `test/opening-symbol.test.mjs` новый тест `jambHalf/outlineHalf/hitHalf ×5`; смок кликает реальную внешнюю точку hitbox — `openingEdgeHitAndActionMatch: true` во всех 14 прогонах | +| AC4 | тот же smoke, Plan с маской grid | `planPixelsMatchWithEquivalentGridHidden`, `snapPrecisionIsFiveTimesFiner`, `screenFixedPlanSnapStrokeIsNotDoubleScaled` — все true во всех прогонах | +| AC5 | unit negative controls + pair smoke + мутант `grid-scale-physical-double-scaled` | мутант ловится (`ok`), `iconUnit`-тест в `canvas.test.mjs` подтверждает `5× NORM_W` при `cell_cm:1` | +| AC6 | pair smoke Devices/Background | `devicesCriticalMetricsMatch/PixelsMatch`, `backgroundPointerCancelIsMutationFree` — true во всех прогонах | +| AC7 | pair pixel/DOM `houseplan-space-card` | `staticPixelsMatch: true` во всех прогонах; `space-render.ts`/`space-card.ts` читают тот же `gridVisualScale`/`gridVisualUnits` — прочитано в коде | +| AC8 | iso unit (переиспользуемые `iso-projection.test.mjs`/`iso-walls.test.mjs`, не менялись — уже параметризованы по высоте) + pair pixel light/dark + `isoRemainsLabsOnly` | все true во всех прогонах; source-regex в `isometric-contract.test.mjs` подтверждает, что `wallHeight`/`floorEdgeHeight` идут через `gridVisualUnits` | +| AC9 | `grid-scale.test.mjs` + `smoke_space_scale_defaults.mjs` | `metricManualCreateStoresOneCm`, `imperialManualCreateStoresOneInch` — green | +| AC10 | тот же smoke, floors import | `metricFloorImportUsesOneCm`, `imperialFloorImportUsesOneInch` — green; прочитано в `_openNextImport()` — использует тот же `newSpaceCellCm()` | +| AC11 | unit + smoke open→save без правки поля | `existingFiveUntouchedIsLossless`, `existingFractionUntouchedIsLossless`, `missingCellUsesLegacyFiveAndMaterializesFive` — green; прочитано: save-путь берёт `d.cellCm` (canonical), не `d.cellCmInput` | +| AC12 | smoke, EN/RU × metric/imperial, language rerender | `metricRussianField`, `metricLanguageRerenderKeepsCanonical` — green | +| AC13 | **проверено чтением, не исполнением** + мутант `grid-scale-visual-factor-constant`/`-inverted` | `gridVisualScale(5) === 1` — явная ранняя ветка в коде без деления с плавающей точкой; смок подтверждает `referenceFactorIsOne: true` во всех 14 прогонах | +| AC14 | targeted touch/opening smoke | `openingEdgeHitAndActionMatch`, `backgroundPointerCancelIsMutationFree` — true во всех прогонах (реальный `pointerdown`/`pointercancel` с `pointerType: 'touch'`, сверка `_serverCfg` до/после) | +| AC15 | **проверено чтением, не исполнением** | diff не добавляет обходов геометрии по всему плану; каждый `gridVisualUnits()`/`gridVisualScale()` — O(1) арифметика на уже вычисленных величинах; CSS-переменная считается один раз на stage/style-строку | +| AC16 | `check-docs` + чтение обоих changelog/guide | зелёный гейт; тексты RU/EN проверены построчно, согласованы с §9–10 ТЗ | + +## Находки + +### Medium (в скоупе задачи, чинится в этом issue) + +**M1. `demo/smoke_grid_scale_invariance.mjs` нестабилен на проверке `darkViewPixelsMatch` — ~43% прогонов красные без изменений в коде.** + +Воспроизведение: 14 последовательных запусков `node demo/smoke_grid_scale_invariance.mjs` +на неизменном коммите `0d91c1e`, без каких-либо правок. Красные — 6 из 14 +(#1, #6, #8, #11, #12, #13), все остальные проверки во всех 14 запусках +неизменно `true`. Красная проверка всегда одна и та же — +`darkViewPixelsMatch`, и диагностика идентична между падениями: + +``` +"darkViewDiff": { "sameSize": true, "changed": 114, "maxDelta": 41, + "meanDelta": 0.016858721655042284, "pixels": 593580 } +``` + +Порог в смоке — `maxDelta <= 40` (`pixelEquivalent()`, строка ~375); падение +идёт ровно на 1 единицу дельты сверх порога, всегда с одинаковым числом +изменившихся пикселей (114 из 593580) и одинаковым `meanDelta`. Это не +похоже на случайный antialiasing-шум (тогда числа плавали бы), а похоже на +двух-модовую гонку — какое-то состояние либо успевает осесть в окне +`settle()` (150 мс после 3 `requestAnimationFrame`), либо нет, и в +неосевшем состоянии рендер стабильно отличается на 114 пикселей именно в +паре View/dark theme. Ни одна другая пара (light View+static, Plan, Devices, +Background, iso light/dark) не дала ни одного красного прогона за то же +число попыток — нестабильность локализована именно в переключении темы в +режиме View. + +Я не свёл её к одной строке кода (проверил явные candidate-транзишны: +`.op-leaf`/`.op-arc` — накрыты `@media (prefers-reduced-motion: reduce)` в +`src/styles.ts:875-877`, day-cycle слои — тоже накрыты `:255-261`, +`.glow-spot` — тоже `:691-694`; смок уже включает `reducedMotion: 'reduce'` +через `launch()` и переустанавливает его в каждом `setTheme()`). Причина +может быть глубже в тайминге асинхронного пересчёта устройств +(`_maybeRebuildDevices()`) относительно окна `settle()`, но я не берусь +называть точную строку без дальнейшей отладки автором — это его код и его +конвейер таймингов. + +**Почему это находка, а не просто «иногда так бывает».** AC2 и AC13 в матрице +доказательств ТЗ (§13.1) называют именно этот смок единственным +автоматическим доказательством попиксельной инвариантности View. Хендофф +автора заявляет «`demo/smoke_grid_scale_invariance.mjs` — green» без +указания числа прогонов; при вероятности ~43% реальный прогон вполне мог +быть тем самым единственным зелёным, а не свидетельством стабильности. +Нестабильный гейт на `S8-merged`/пре-релизе — ровно тот сценарий, который +`AGENTS.md` описывает как причину лишнего цикла ревью (там же зафиксирована +цена: пропуск гейта на #230/#234 стоил `dev` красным `docs`-job до +следующей задачи). Здесь тот же класс риска, но для `smoke` job. + +Это Medium, не High: сама физика инварианта доказана — 13/14 проверок, +включая ту же пару в режиме light (со static-card), совпадают стабильно на +всех 14 прогонах; проблема в допуске/таймінге смока, а не в +продукте. Чинится в этом же issue (владелец 2026-08-19, #202): например, +увеличить `settle()` перед `capture()` при смене темы, либо обосновать и +слегка ослабить `maxDelta`, либо явно дождаться состояния, гонка с которым +и даёт эти 114 пикселей. + +## Что проверено и корректно + +- Классификация размеров (Physical/Screen/Plan-relative/Visual + unit/Grid, §6 ТЗ) выдержана в коде: `wallCmToUnits`-путь, decor/furniture, + Glow, штриховка #230, snap-узлы 5/10 см — не проходят через + `gridVisualScale`/`gridVisualUnits` (подтверждено grep + мутант + `grid-scale-physical-double-scaled`, ловится). +- `gridVisualScale(5) === 1` — точное совпадение без накопления погрешности + плавающей точки (явная ветка `if (value === GRID_VISUAL_REFERENCE_CELL_CM) return 1`). +- Общий CSS-transform на весь SVG отсутствует; масштабирование — точечное, + через `calc(px * var(--hp-cell-visual-scale, 1))` в `styles.ts` и + через `gridVisualUnits()` в TS-геометрии — соответствует запрету §6/§7.2 ТЗ. +- `--hp-cell-visual-scale` выставляется и в full card (`houseplan-card.ts`), и + в static card (`space-render.ts`), с тем же helper'ом; `space-card.ts` + дублирует только ту одну CSS-правку (wallbody stroke), которая не приходит + через общий `cardStyles`. +- Скрытая изометрия: `wallHeight`/`floorEdgeHeight` вычисляются один раз на + сцену и передаются как параметры в уже существовавшие (не изменённые в этом + диффе) `buildIsoWallGeometry`/`buildIsoOpeningBasis`/`buildIsoFloorGeometry`/ + `projectedFrame`; попадают в `isoGeometryFingerprint` — тёплый remount не + вернёт геометрию другого `cell_cm` (проверено чтением сигнатур и + `isometric-contract.test.mjs`). +- Default/compat: manual create и floors-import используют один + `newSpaceCellCm(this._imperial)`; legacy fallback для отсутствующего/ + некорректного `cell_cm` остаётся 5 и во frontend (`houseplan-card.ts`, + `space-geometry.ts`), и не тронут в backend/optimizer (вне диапазона диффа). +- Lossless imperial edit: сохранение читает канонический `d.cellCm`, а не + проекцию `d.cellCmInput` — round-trip через дюймы невозможен без реального + `input`-события (мутант `grid-scale-imperial-roundtrip-drift` ловится). +- i18n: новый ключ `space.scale_unit_imperial` заведён в EN и RU, подпись поля + переключается по `this._imperial`; оба changelog и оба user-guide правлены + в том же коммите, трейлеры (`Issue: #239`, `User-Visible: yes`) на месте. +- Три поставляемых бандла побайтно идентичны после локальной пересборки; + `check-docs` зелёный, значит скриншот-отпечаток учтён. +- Mutation gate: все 12 новых мутантов `grid-scale-*` пойманы существующими + guard'ами (unit-тест либо целевой smoke) — правило «тест умеет падать» + выполнено для покрытых ими путей. + +## Чего не проверял + +- `npm run golden:verify`, полный набор 163 browser-smoke, `performance_smoke` + — по процессу это предрелизный гейт, не гейт код-ревью, и в диффе нет + сигналов, требующих его досрочного запуска. +- Backend (`python -m pytest tests_backend`) — diff не касается + `custom_components/**/*.py`. +- Не устанавливал точную причину гонки в M1 (не отследил конкретную строку/ + промис, ответственные за таймінг) — это для автора, у которого есть + контекст реализации `_maybeRebuildDevices()`/`settle()`. +- Не проверял ручное поведение в реальном браузере вне demo-харнесса + (Home Assistant, реальные темы) — вне доступного окружения ревью.