diff --git a/docs/reviews/CODE-REVIEW-277-r2.md b/docs/reviews/CODE-REVIEW-277-r2.md new file mode 100644 index 00000000..4f44a3b2 --- /dev/null +++ b/docs/reviews/CODE-REVIEW-277-r2.md @@ -0,0 +1,185 @@ +# CODE-REVIEW-277-r2 + +- **Issue:** #277 — безопасный Resize без изменения топологии +- **Заход:** r2 (второй код-ревью-заход; спецификация прошла отдельные заходы + SPEC-REVIEW-277-r1 → r2, зелёный, не переоценивается на этой стадии — §10.4 + PROCESS.md, «цикл считается по этапу») +- **Ветка:** `issue/277-safe-resize` +- **SHA r1 (предыдущий код-ревью):** `241821c40e9b2f43b79945af3377fc3b3f4ac084` + (взят из `docs/reviews/CODE-REVIEW-277-r1.md`, раздел «SHA ревью»; в + терсовом вердикт-комментарии issue SHA не назван — восстановлен из + полного документа, не по времени коммитов. Это повторение процессного L2 из + `SPEC-REVIEW-277-r2` на новой стадии: снимаю с записью, не блокирует, см. + «Находки») +- **SHA этого ревью:** `a548d4f17dc5611d4cfd6f3ffe1042f91a365840` (HEAD) +- **Дельта:** `git diff 241821c4..HEAD`, 2 продуктовых коммита автора между + r1 и HEAD: + +| Коммит | Тема | Класс | User-Visible | +|---|---|---|---| +| `9c5b66c3` | perf: fingerprint Resize handles once per frame | A (правит `src/houseplan-card.ts`) | no | +| `a548d4f1` | docs: accept Resize performance screenshots | C | no | + +(`448ceea3` в диапазоне — коммит самого документа `CODE-REVIEW-277-r1.md`, +положенный шагом публикации r1; не авторская правка, не часть дельты для +разбора.) + +## Скоуп + +Ровно одна Medium-находка предыдущего раунда (M1: eligibility-кэш ручек +Resize пересчитывал полный geometry fingerprint на КАЖДУЮ ручку на КАЖДЫЙ +рендер вместо одного раза на рендер-слой). Дельта — точечный perf-фикс этой +находки плюс тест/бенчмарк/документация на неё. Геометрия (`src/resize.ts`), +safety-контракт (`resolveSafeResize`/`applySafeResize`/`validateSafeResize`), +i18n и весь остальной код не тронуты. AC1–AC12, AC14–AC17 дельта не задевает +логически (см. «Унаследовано»); AC13 (перформанс) дельта расширяет — +предмет этого раунда. + +Разбор — по дельте (§2.9), не полный: дельта локальна (один файл продукта, +один метод и один вызывающий цикл), контракт поведения не меняется, +новая подсистема не затронута, объём дельты (13 строк продукта) несопоставим +с объёмом исходной задачи. + +## Как проверялось + +| Гейт | Команда | Результат | +|---|---|---| +| Typecheck | `npx tsc --noEmit` | зелёный | +| Unit | `npm test` | 1195/1195 passed, 0 fail (было 1194 в r1: +1 новый тест) | +| Build + байт-идентичность 2 отслеживаемых копий бандла | `npm run build` + `cmp dist/houseplan-card.js custom_components/houseplan/frontend/houseplan-card.js` | зелёный, байт-в-байт | +| Docs fingerprint | `node scripts/check-docs.mjs` | «Documentation checks passed (7 files, 10 external links)» | +| Выбор смоков по дельте | `node scripts/smoke-select.mjs --base 241821c4 --head HEAD` | «Изменено файлов src/\*\*: 1 · символов на изменённых строках: 4» → 6 «прямых совпадений» (все через символ `_rszDrag`), 0 «зарегистрированных связей», 0 «неопределённостей»; полный вывод ниже | +| Смоки (6 прямых совпадений) | `node demo/smoke_.mjs` × 6 | все 6 зелёные (список ниже) | +| Golden (полный) | `npm run golden:verify` | 86/86 passed, 0 failed, включая `safe-resize-handles-clamp-{light,dark}` | +| Новый perf-бенчмарк | `npm run benchmark:safe-resize-render` | `"pass": true`; render p95 1.2–1.8 мс (бюджет 25 мс); `snapshotCalls: 20` = ровно 1 на кадр при 20 семплах | +| Мутационная проверка бенчмарка (моя, не автора) | вручную откачен фикс (`git apply -R` на диапазон `9c5b66c3` по `src/houseplan-card.ts`), `npm run bundle:sync`, повторный `npm run benchmark:safe-resize-render`, затем восстановлено `git apply` + `bundle:sync` | на старом коде: `snapshotCalls: 1600` (80 на кадр), render p95 27.9 мс > бюджета 25 мс, `"pass": false` — бенчмарк ловит именно регрессию M1; рабочее дерево восстановлено, `git status` чист | +| Performance (числа) | см. выше | p95 1.2–1.8 мс против 130–224 мс из воспроизведения M1 в r1 на той же large-house фикстуре (20 комнат/80 ручек) — не просто «в бюджете», а на два порядка быстрее исходной регрессии | +| Model invariants | не запускались | геометрия (`src/resize.ts`, `resolveSafeResize` и т.д.) не тронута этой дельтой — только порядок вычисления уже существующего fingerprint в вызывающем коде рендера; наследуется из r1 | +| CI-провенанс `Reviewed artifact` (docs screenshots) | `gh run view 32691836025` | run зелёный (`workflow_dispatch`, job `capture`), соответствует коммиту `a548d4f1` | +| Backend | — | не запускался: дельта не трогает `custom_components/**/*.py` | + +Полный вывод `smoke-select.mjs`: + +``` +Изменено файлов src/**: 1 · символов проекта на изменённых строках: 4 +Матрица: 177 смоков · порог «широкого» символа: больше 35 смоков + +Прямое совпадение (6): + demo/smoke_pan_any_zoom.mjs ← _rszDrag + demo/smoke_resize_audit_1550.mjs ← _rszDrag + demo/smoke_resize_inner_dimensions.mjs ← _rszDrag + demo/smoke_resize_virtual_thick.mjs ← _rszDrag + demo/smoke_resize_wall_thickness.mjs ← _rszDrag + demo/smoke_room_resize.mjs ← _rszDrag + +Выборка дополняет AC задачи и суждение ревьюера, а не заменяет их. +Полный прогон матрицы остаётся предрелизной обязанностью на точном SHA. +``` + +Решение по строке: прогнать все 6 — узкий диапазон, символ (`_rszDrag`) +прямо задействован в новом коде (`this._rszDrag?.snap || renderSnapshot || +this._rszSnapshot()`), «слабых» связей нет. Полная матрица (177) не +прогонялась — предрелизный гейт (§8), не гейт этого ревью. + +## Находки + +### Low (снимаю с записью, не блокирует) + +**L1 (процессная, повтор паттерна из SPEC-REVIEW-277-r2/L2).** Вердикт-комментарий +`CODE-REVIEW-277-r1` в issue не называет SHA, на котором получен зелёный/жёлтый +результат — SHA есть только внутри полного документа +(`docs/reviews/CODE-REVIEW-277-r1.md`, строка «SHA ревью»). Восстановлено без +труда (сам документ уже закоммичен и однозначен, гадать по времени коммитов не +потребовалось), не стоило дополнительного цикла. Не блокирует: тот же паттерн +уже был отмечен и снят с записью на стадии spec этой же задачи; здесь — +подтверждение, что практика не изменилась на стадии code. Рекомендация на +будущее (не мандат ревьюера): переносить строку «SHA ревью» из документа в +терсовый комментарий issue, раз она уже вычисляется. + +Находка предыдущего раунда (M1) закрыта полностью — см. раздел ниже. Новых +находок в продуктовом коде дельты (`src/houseplan-card.ts`, тест, бенчмарк, +docs) не обнаружено. + +## Закрытие r1 + +| Находка r1 | Чем закрыта | Где это видно | +|---|---|---| +| **M1** (Medium): eligibility-кэш ручек Resize пересчитывал полный `JSON.stringify(_geometrySnapshot())` для каждой ручки на каждый рендер, а не один раз на изменение геометрии — противоречило §13 ТЗ и `ARCHITECTURE.md` | Коммит `9c5b66c3`: `_renderResizeLayer()` теперь вычисляет `renderSnapshot` один раз перед циклом по ручкам (`const renderSnapshot = this._rszDrag?.snap \|\| this._rszSnapshot();`) и передаёт его в `_rszResolution(roomId, edge, renderSnapshot)`; `_rszResolution` использует переданный снапшот вместо повторного вызова `_rszSnapshot()`, приоритет `this._rszDrag?.snap` во время активного жеста сохранён без изменений | `src/houseplan-card.ts:8332-8339` (сигнатура и тело `_rszResolution`) и `:8705-8716` (`renderSnapshot` вычислен один раз, передан в цикле). Численно: новый `npm run benchmark:safe-resize-render` даёт `snapshotCalls: 20` = 1 на кадр (было бы 1600 = 80 на кадр на старом коде — проверено мной откатом фикса, см. «Как проверялось»); render p95 упал с воспроизведённых в r1 130–224 мс до 1.2–1.8 мс на той же large-house фикстуре (20 комнат/80 ручек) | +| L1 (Low, r1): мёртвый `_rszSel`/устаревший комментарий про удалённую угловую рамку | Не затронуто этой дельтой — было явно снято с записью в r1 без обязательства править («безопасный кандидат на мелкую уборку», не AC, не блокирует); дельта r2 не касается `_rszSel` | `src/houseplan-card.ts:7386-7389` не менялся между `241821c4` и HEAD (не входит в diffstat дельты) | + +## Унаследовано из r1 + +Без повторной проверки принято из `docs/reviews/CODE-REVIEW-277-r1.md` +(SHA `241821c40e9b2f43b79945af3377fc3b3f4ac084`), так как дельта их не +задевает: + +- AC1–AC12, AC14–AC17 — доказаны в r1 автотестами (мутанты 1/1) и живым + pointer-жестом через production-бандл; ни `src/resize.ts`, ни safety-порядок + причин disabled, ни i18n, ни persist/undo-путь этой дельтой не тронуты. +- Полный `golden:verify` 80/80 из r1 (в этом раунде перепрогнан целиком и + дал 86/86 — расширение числа сцен между раундами не относится к дельте + #277, само по себе зелёное). +- Mutation-gate: 5 новых мутантов `safe-resize-*` (r1, дорогой прогон 1/1 + каждый) — geometry-функции, которые они бьют, не изменены дельтой r2; + не перепрогонялись. +- `Baseline-Reviewed`/провенанс golden-baselines коммита `241821c4` — + проверено в r1 через `gh run view --log`, не переоценивается. +- Риск-таблица §12.1 ТЗ (порядок #276→#277→#278, неучастие #264) — состояние + задач не проверялось повторно в этом раунде (продуктовый вопрос, не + затронутый дельтой). +- «Одно число — один источник» (`_rszEdgeLabels()` берёт данные из того же + `res.polys`, что коммитится) — код длин/площади не тронут дельтой. + +## Что проверено и корректно + +- Фикс M1 корректен по построению: во время активного драга приоритет + `this._rszDrag.snap` не изменился (переданный `renderSnapshot` используется + только как второй по приоритету источник, после drag-снапшота и до + дорогого пересчёта) — поведение при перетаскивании ручки идентично + дореформенному, разница только в числе вызовов `_rszSnapshot()` вне драга. + Подтверждено чтением обоих условий `snap = this._rszDrag?.snap || + renderSnapshot || this._rszSnapshot()` и не найдено ветки, где старое и + новое поведение расходятся. +- Регрессия из воспроизведения M1 численно устранена на порядки (1.2–1.8 мс + против 130–224 мс), а не просто «вошла в бюджет» — проверено собственным + контрольным откатом фикса (мутационная проверка вручную, см. таблицу + гейтов), а не только заявлением коммита. +- Новый unit-тест (`test/resize-production-path.test.mjs`) умеет падать: он + требует ровно строку с `renderSnapshot` в обоих местах и явно запрещает + старую двухаргументную форму вызова — при откате фикса тест обязан упасть + (регэксп `_rszResolution\(r\.id, i\);` совпал бы со старым кодом). +- Оба коммита несут `Issue: #277` и `User-Visible: no` корректно: изменение + не меняет наблюдаемое поведение (то же дерево ручек, то же disabled- + состояние, та же геометрия) — только скорость вычисления уже существующего + кэша: правки в changelog не требуются и не сделаны. +- `docs/RESIZE.md`/`docs/TESTING.md` дополнены двумя точными строками про + новый бенчмарк — согласуются с реальным поведением скрипта (бюджет 25 мс, + один снапшот на кадр), не расходятся с кодом. +- Обновление скриншотов (`a548d4f1`) — ожидаемое следствие: source-fingerprint + документации считается по всему `src/**` (см. правило проекта), поэтому + правка `9c5b66c3` сделала прежние скриншоты устаревшими; `Reviewed + artifact`-ссылка проверена по факту (`gh run view`), не только по формату. + +## Чего не проверял + +- Полный набор `demo/smoke_*.mjs` (177 файлов) — предрелизный гейт (§8); + прогнаны только 6 «прямых совпадений» дельты. +- `python -m pytest tests_backend` — дельта не трогает `custom_components/**/*.py`. +- Model invariants (`npm test`/`scripts/model-invariants.mjs` на конкретной + конфигурации) — геометрия не тронута этой дельтой, наследуется из r1 без + повторного прогона. +- Мутанты mutation-gate (все ~60, включая 5 `safe-resize-*`) — не + перепрогонялись; дельта не касается функций, которые они бьют. +- AC1–AC12, AC14–AC17, риск-таблица §12.1, i18n — не переоценивались, + см. «Унаследовано из r1». +- Попиксельный ручной пересмотр всех 86 golden-сцен и двух пересобранных + скриншотов документации — доверился отчётам `golden:verify`/`check-docs` и + зелёному CI-run `capture`, отдельного визуального ревью пикселей не делал. + +## Вердикт + +Зелёный: единственная Medium-находка r1 (M1) закрыта и проверена численно +(включая контрольный откат фикса, а не только чтением диффа), новых находок +в продуктовом коде дельты нет. Одна процессная Low (SHA не назван в +терсовом вердикте issue) — снята с записью, не блокирует, повторяет уже +принятый на стадии spec паттерн.