diff --git a/docs/reviews/CODE-REVIEW-337-r3.md b/docs/reviews/CODE-REVIEW-337-r3.md new file mode 100644 index 00000000..473c39e9 --- /dev/null +++ b/docs/reviews/CODE-REVIEW-337-r3.md @@ -0,0 +1,264 @@ +# CODE-REVIEW-337-r3 + +- Issue: https://github.com/Matysh/houseplan-card/issues/337 +- Ветка: `issue/337-lazy-editor-chunk`, ревьюемый SHA: `16c616be165604aca8eb9c7cdeab59024a828006` +- Заход: r3 · блокирующих циклов израсходовано **2/4** до этого разбора +- **Вердикт: красный.** + +## Скоуп разбора + +Предыдущий вердикт — CODE-REVIEW-337-r2, SHA ревью `2beafc06398c16d1cad80fddbe77e6a40f111b9a` +(красный, High:1 — H6, Medium:0). Этот SHA и SHA r1 (`6d338b78`) в текущей истории +отсутствуют: ветка была перебазирована. Проверил, на что именно: + +``` +git log --oneline 59ae6b1 -3 +59ae6b10 ci: полная история без блобов там, где содержимое старых файлов не нужно +80a664bc docs: рекомендовать blobless-клон вместо полного +508945c0 fix: a 0° pair is the shared-wall model, not a duplicate — field revert of #331 §2.2 +``` + +`508945c0` — это тот же `dev`, на котором уже стояли и r1, и r2 (см. r2-документ: +«branch и так уже стоит на `508945c0` = `origin/dev`»). Между r2 и этим заходом `dev` +продвинулся всего на два коммита класса C/B (`docs`, `ci`, ни один не в `src/**` и не +в `custom_components/**/*.py`) — не «ребейз на ушедший вперёд dev» в смысле §2.10/§7.2 +(другая подсистема, смена контракта), а техническая синхронизация без содержательного +чужого кода. Полный разбор с нуля не требуется; веду по дельте. + +**Материал дельты r2→r3** — единственный содержательный коммит: + +``` +16c616be fix: preserve warm editor camera during lazy adoption (src/houseplan-card.ts +28/-4, плюс зеркальные dist/*) +``` + +(Коммиты `8afc8636` — документ r2, `6b482f4c`/`946f67ac`/`6f9df14d` предшествуют r2 и +уже разобраны в CODE-REVIEW-337-r2 — не входят в дельту этого раунда.) + +Единственная находка предыдущего раунда — **H6** (потеря бит-точности камеры +warm-remount при lazy-adopt). Разбор ограничен: (а) верификацией фикса H6, (б) всеми +проверками, до которых дотягивается 15-файловый diff (`src/houseplan-card.ts` + +зеркальные `dist/`/`custom_components/houseplan/frontend/`), (в) обязательным +по AC13/§8 `check-docs` (диф трогает `src/**`). + +## Как проверялось + +Дешёвые гейты — прогнаны лично на `16c616be`, не приняты со слов автора: + +| Гейт | Команда | Результат | +|---|---|---| +| typecheck | `npx tsc --noEmit` | pass, exit 0 | +| unit | `npm test` | 1440 total, **1439 pass, 1 skip, 0 fail** (у автора 1438/2 — расхождение на один conditional skip, см. ниже, не регрессия) | +| build+sync+budget | `npm run build && npm run bundle:sync && npm run bundle:budget` | initial View **255 910 B** ≤ 256 000 B (запас **90 B**, у автора — то же число); lazy editor 131 779 B; три дерева (`dist`, `custom_components/houseplan/frontend`, `demo/srv/assets`) синхронны | +| docs fingerprint (диф трогает `src/**`, AC13 прямо называет `check-docs` доказательством) | `node scripts/check-docs.mjs` | **FAIL, exit 1**: `ERROR screenshot source fingerprint is stale; run npm run build && node demo/docs/capture.mjs` — см. находку H7 | +| mutation, changed-range | `node scripts/mutation-gate.mjs --check --changed=16c616be^..16c616be` | 30/30 применимых guard'ов ловят подмену | + +Разница в skip-счётчике: у меня единственный skip — `issue 281 private exact fixture +has no enabled zero-range handle # SKIP private #281 fixture is not present`, условный +пропуск из-за отсутствующей приватной фикстуры в песочнице, к #337 не относится. Не +исследовал, какой второй тест пропущен у автора на Windows — сумма (1440) совпадает, +расхождение не в счёте тестов #337. + +Целевые browser smokes (по дельте — файл трогает ровно зону `_requestMode` / +`_refitView` / `_setMode` / disconnect-guard, разбор не «по названию», а по +затронутым символам): + +| Smoke | Зачем выбран | Результат | +|---|---|---| +| `smoke_warm_dialogs.mjs` ×7 подряд, свежий `bundle:sync` | тот самый смок, поймавший H6 (5/7 fail в r2) | **7/7 OK**, `bViewBitExact` не подводит ни разу | +| `smoke_warm_owners.mjs` | та же warm-adopt зона (`_warmAdoptViewport`/`_requestMode(adopt)`) | OK | +| `smoke_preloader_lifecycle.mjs` | та же зона, ранее ловил H1 | OK | +| `smoke_kiosk.mjs` | ранее ловил H2, эта зона трогает общий `_requestMode` | OK | +| `smoke_mode_transition.mjs` | напрямую использует `_modeTransitionBusy`, которым теперь делит guard `_refitView` | OK | +| `smoke_resize_wall_thickness.mjs` | использует resize/refit во время editor-режима | OK | +| `smoke_warm_remount.mjs` | смежный warm-remount контракт, слабая связь по `smoke-select` | OK | +| `smoke_room_resize.mjs`, `smoke_optimize_geometry_preflight.mjs` | H3-зона (`this.host.` preflight), проверка на отсутствие регресса от нового `isConnected`/`_warmModeRequest` раннего return | OK | + +`node scripts/smoke-select.mjs --base origin/dev --head HEAD` даёт 175/195 «прямое +совпадение» — бесполезно как фильтр для этого раунда: `origin/dev` не содержит всей +задачи #337 целиком, поэтому инструмент показывает список для ВСЕЙ ветки, а не для +дельты r2→r3. Использовал его для контекста (список выше уже включает `smoke_warm_dialogs`, +`smoke_mode_transition` и `smoke_warm_remount` из числа тех, что он называет), но +подбирал набор вручную по символам самого диффа, а не по выдаче инструмента. + +Полный census 195 smoke-файлов и `golden:verify` **не перезапускал**: он уже +пройден дважды (195/195 в r1, 195/195 в r2) на неизменной по этому раунду части +дерева, а этот раунд — один файл в одной узкой зоне таймингов, для которой census +уже доказанно нечувствителен (сам census не поймал H6 ни разу, только точечный +7×-прогон). Полный набор — предрелизный гейт (§8), не гейт ревью при такой дельте. + +`model-invariants` — не запускал: diff не меняет ни один формат геометрической +записи (`layout`, `marker.space`, `open_spans`, ключи толщины) — правка только +про `_view`/`_zoom` кэш камеры и таймер refit. Backend/pytest — не тронут этой +дельтой, не перепрогонял. + +## Находки + +### High (блокирует) + +**H7. `check-docs` красный на ревьюемом SHA — стало устаревшим ровно из-за +этого коммита, AC13 не доказан.** + +`node scripts/check-docs.mjs` → `ERROR screenshot source fingerprint is stale` +(exit 1). Причина механическая и не спорная: `scripts/source-fingerprint.mjs` +хэширует весь `src/**`; `16c616be` меняет `src/houseplan-card.ts` (единственный +src-файл в этом коммите) и не обновляет `sourceFingerprint` в +`docs/images/screenshots.json`, поэтому хэши расходятся. На SHA r2 +(`2beafc06`) `check-docs` был зелёным (сам документ r2: «pass (7 файлов, 10 +внешних ссылок)») — регрессия введена именно этим, последним коммитом +раунда, а не унаследована. + +Это прямо предусмотренный в задаче случай, а не мелочь: AC13 спецификации +(`docs/specs/337-lazy-editor-chunk.md:346-348`) называет `check-docs` **прямым +доказательством** этого критерия приёмки, а §8 `PROCESS.md` называет его +«реальным блокером» именно за то, что отпечаток покрывает весь `src/**` — «любая +правка фронтенда делает его устаревшим… выбирать тут нечего». Цена пропуска +уже была измерена дважды на этой же кодовой базе: «скриншоты не пересняли в +#230 и #234, и `dev` стоял с красным job `docs`, пока это не нашли при +следующей задаче (#237)». Слияние `16c616be` в `dev` в текущем виде +воспроизводит ровно этот сценарий. + +Автор в хендоффе смешал эту находку с H5/#346: «известный stale screenshot +fingerprint; H5 уже независимо подтверждён ревьюером и вынесен в #346» — но +H5/#346 про **golden-эталоны** (`demo/golden/baselines/**`, устаревший baseline +тулбара Device editor и mobile-ru диалогов, не связан с #337 и воспроизводится +на чистом `dev`), а это — про **fingerprint скриншотов документации** +(`docs/images/screenshots.json`, `docs-section` в `README`/`USER-GUIDE`), +совершенно другой гейт с другим владением. Ссылка на #346 не закрывает эту +находку: #346 не затрагивает `sourceFingerprint`, а причина здесь — собственный +код-коммит этой ветки, а не предсуществующее состояние `dev`. + +Сам автор уже дважды в этой же задаче правильно закрывал этот же гейт без +замены единого PNG («fingerprint обновлён без замены PNG, потому что +наблюдаемого UI-изменения нет» — хендофф после H1–H4). Здесь этот шаг просто +пропущен для последнего коммита. + +**Воспроизведение:** `git checkout 16c616be -- .` (или просто на HEAD), затем +`npm run build && node scripts/check-docs.mjs` → exit 1 с текстом выше. +**Исправление в скоупе задачи**: прогнать `Docs screenshots` (workflow_dispatch) +и принять новый fingerprint через `npm run docs:accept -- --reviewed +--from=<артефакт>` (замены PNG не требуется, если рендер не изменился — +ровно то, что уже делалось между r1 и r2 этой же задачи). + +### Medium + +Нет находок этого уровня в дельте r2→r3. + +### Low + +Нет. + +## Что проверено и корректно + +- **Сама механика фикса H6 логически закрывает найденную гонку.** Разобрал + правку построчно (`src/houseplan-card.ts:803-925, 2549, 5880, 6662`): + - новое поле `_warmModeRequest` выставляется только на `adopt`-пути + `_requestMode` (`:894-901`) и гейтит `_refitView` (`:5880`: `if + (this._modeTransitionBusy || this._warmModeRequest) return;`) — то есть + ResizeObserver-триггерный пересчёт `x`/`w` камеры (источник H6) подавлен + на всё время окна между синхронным восстановлением `_view` и асинхронным + переключением `_mode` через `_requestMode(restoredMode, false, true)`; + - после `updateComplete` и двух `requestAnimationFrame` (реальная + расстановка раскладки после переключения режима) код берёт фактический + `stage.clientWidth/Height` как новый `_lastValidStageSize` и снимает + guard (`:918-924`) — следующий настоящий resize сравнивается с верной + базой, а не с устаревшей; + - guard снимается на всех путях выхода: неудача `_ensureEditorRuntime()` + (`:903`), опережающий запрос или дисконнект во время `await` + (`:906-907`, добавленная проверка `!this.isConnected`), обычная смена + режима пользователем через `_setMode` (`:6662`), и + `disconnectedCallback` (`:2549`) — то есть защита не может «залипнуть». + - `_warmAdoptViewport`/`adopt=true` вызывается только на **свежесозданном** + экземпляре карточки при warm-remount (`:3110, 3243`), где `_refitRaf`/ + `_pendingRefitSize` заведомо в начальном состоянии — отмена «на всякий + случай» на `:899-900` не теряет ничьих легитимных запросов. +- **Эмпирически гонка не воспроизводится.** `smoke_warm_dialogs.mjs` — тот же + файл и тот же сценарий, что дал 5 отказов из 7 в r2 — **7/7 OK** на этом + SHA после `bundle:sync`. Это выполняет дисциплину «тест умеет падать»: он + падал большинством прогонов на предыдущей ревизии кода и на новой этого не + делает ни разу — не совпадение единичного зелёного прогона. +- **Соседние сценарии той же зоны не пострадали**: `warm_owners`, + `preloader_lifecycle` (H1-зона), `kiosk` (H2-зона), `room_resize`, + `optimize_geometry_preflight` (H3-зона), `mode_transition`, + `resize_wall_thickness`, `warm_remount` — все зелёные лично. +- **Бюджет (AC1)** — 255 910 B ≤ 256 000 B, посчитано лично. Запас **90 B**, + сократился с 222 B (r2) — тенденция, названная риском уже в r1/r2, + продолжается и здесь и стоит держать в уме на любой следующей правке; + формально AC1 выполнен, не блокирую этим отдельно. +- **Mutation gate по коммиту** — 30/30 применимых guard'ов на + `16c616be^..16c616be` ловят подмену; изменённая зона осталась под + контролем существующих guard'ов, новых guard'ов эта точечная правка не + требовала. +- **Трейлеры.** `Issue: #337`, `User-Visible: no` — корректно: пользовательский + changelog для #337 уже зафиксирован отдельным коммитом ранее (r1), сама + правка не добавляет нового видимого поведения, а завершает контракт до + релиза (задача ещё не в `dev`, User-Visible относится к тому, что увидит + пользователь после выпуска). +- **Одно число — один источник.** Диф не вводит и не дублирует ни одной + видимой пользователю величины: `_view`/`_zoom`/`_lastValidStageSize` — это + внутреннее состояние камеры рендера, не выводится текстом/подписью нигде + в UI. Правило `test/single-source-numbers.test.mjs` не затронуто и + проходит в общем `npm test`. + +## Закрытие раунда r2 + +| Находка r2 | Чем закрыта | Где видно | +|---|---|---| +| **H6** (потеря бит-точности камеры warm-remount, 5/7 fail на `smoke_warm_dialogs`) | `_refitView` подавлен на время адаптации новым `_warmModeRequest`-guard'ом; после двух `rAF` после `updateComplete` берётся фактический размер сцены как новая база для рефита — устраняет саму гонку резайза во время переключения режима | `src/houseplan-card.ts:806, 894-925, 2549, 5880, 6662`; эмпирически — `smoke_warm_dialogs.mjs` 7/7 OK (прогнал лично) против 5/7 FAIL на предыдущей ревизии | + +## Унаследовано из r2 + +Принято без повторной проверки в этом раунде — дельта (`16c616be`, один +src-файл + зеркальные `dist/`) их не касается: + +- **AC1 (initial budget)** — пересчитан лично в этом же раунде (см. таблицу + гейтов), не просто унаследован: 255 910 B ≤ 256 000 B. +- **AC2 (lazy boundary), AC5 (loader atomicity), AC6 (failure сохраняет View)** — + вне дельты; вывод CODE-REVIEW-337-r2.md (SHA `2beafc06`, раздел «Закрытие + раунда r1» + «Что проверено и корректно») остаётся в силе. +- **AC7 (asset security), AC8 (полнота distribution), AC9 (CSS-минификатор)** — + не тронуты этой дельтой; вывод r2 (наследующий вывод r1, SHA `6d338b78`) + остаётся в силе. +- **AC10 (fingerprint mismatch runtime — версии не смешиваются), AC11 + (onboarding/async config editor), AC12 (no model drift)** — вне дельты, + наследуются из r2/r1. +- **H2, H3, H4, M1–M3 (r1)** и **H5/M4 → #346 (golden baselines, вне скоупа + #337)** — закрыты и подтверждены независимым прогоном ещё в r2 + (CODE-REVIEW-337-r2.md, SHA `2beafc06`); эта дельта их зону не трогает. +- **Полный browser-smoke census (195/195) и `golden:verify` (127/131, 4 + «different» = #346)** — пройдены дважды (r1 SHA `6d338b78`, r2 SHA + `2beafc06`) на неизменной этим раундом части дерева; не перезапускал + целиком, см. «Как проверялось». + +Отдельно от AC-цепочки: **AC13 (docs) не наследуется** — именно на нём найдена +новая находка H7 этого раунда (было зелёным на SHA r2, стало красным на SHA r3). + +## Чего не проверял + +- **Полный HA-harness backend** (`python -m pytest tests_backend -q` с + установленным `homeassistant`) — недоступен в этой песочнице, дельта не + трогает `custom_components/**/*.py`. +- **`node scripts/model-invariants.mjs`** — не запускал, дельта не меняет + формат геометрической записи (см. «Как проверялось»). +- **Полный browser-smoke census (195 файлов) и `golden:verify`** — не + перезапускал в этом раунде; обоснование выше и в разделе «Унаследовано». +- **Perf-профили** — не запускал, не названы в AC и не затронуты этой + дельтой. +- **Частоту гонки H6 в headless Linux CI** отдельно от своей песочницы — не + измерял; 7/7 подряд в своём окружении расцениваю как достаточное + доказательство закрытия конкретно найденной гонки (тот же порог, что + задавал сам r2-документ), но абсолютную частоту в CI не оцениваю числом. +- **Ручное тестирование в реальном Home Assistant** — не выполнялось (в + процессе нет фазы ручного тестирования, код-ревью отвечает за AC вместо + него). + +## Итог + +Фикс H6 логически и эмпирически закрывает единственную блокирующую находку +r2: разобран по коду, зона правки атомарна и не расширяет скоуп, гонка не +воспроизводится ни разу за 7 прогонов там, где раньше падала в 5 из 7. +Однако тот же самый коммит красит обязательный по AC13 гейт `check-docs` — +не унаследованная, а новая, введённая этим раундом находка (H7), с уже +дважды на этой задаче отработанным способом закрытия. Вердикт остаётся +красным по формальному правилу «High блокирует», при том что содержательная +причина возврата — узкая: один пропущенный шаг (пересъёмка/приёмка +fingerprint), а не дефект самого фикса камеры.