From d7e97687ce3a0f5ddde2c89ffe718d53f643650e Mon Sep 17 00:00:00 2001 From: "claude[bot]" <209825114+claude[bot]@users.noreply.github.com> Date: Fri, 4 Sep 2026 22:11:57 +0000 Subject: [PATCH] docs: review document for #451 Issue: #451 User-Visible: no --- docs/reviews/CODE-REVIEW-451-r3.md | 223 +++++++++++++++++++++++++++++ 1 file changed, 223 insertions(+) create mode 100644 docs/reviews/CODE-REVIEW-451-r3.md diff --git a/docs/reviews/CODE-REVIEW-451-r3.md b/docs/reviews/CODE-REVIEW-451-r3.md new file mode 100644 index 00000000..24b81a4e --- /dev/null +++ b/docs/reviews/CODE-REVIEW-451-r3.md @@ -0,0 +1,223 @@ +# CODE-REVIEW #451 — заход r3 + +- **Issue:** #451 — «План тормозит: диагностика считается на каждый кадр, перетаскивание перерисовывает всё, нет фильтра обновлений» +- **Этап:** код-ревью (PROCESS.md §2.7) +- **Заход:** r3 · блокирующих циклов до этого раунда: 2/4 +- **Материал раунда:** ветка `issue/451-render-performance`, `HEAD = 444562e47cb746dc1c7d740b66b2f832ca02f064` + (сверено `git rev-parse HEAD` непосредственно перед выводом, дерево чистое, PROCESS.md §2.7/#312) +- **Дельта раунда:** `git diff cb68492c990b27e91c35130fa3a77806bd157c5e..444562e4` — SHA взят из машинного блока + «Материал раунда» документа `docs/reviews/CODE-REVIEW-451-r2.md` (дерево `aaa8519cee54…`, сверено + `git cat-file -p cb68492c^{tree}` — совпадает). Файлы дельты (без `dist/**` и бандла стенда): + `src/houseplan-card.ts`, `src/houseplan-editor-runtime.ts`, `src/junction-limits.ts`, + `src/live-editor.ts`, `src/resize-controller.ts`, `test/resize-controller.test.mjs`, + `docs/images/screenshots.json`, `docs/reviews/CODE-REVIEW-451-r2.md`. + +## Скоуп раунда + +r2 (красный, High:3) закончился на коммите `cb68492c` (диагностика — 8 файлов). С тех пор в ветку +легли 7 коммитов: `fe03eae2`, `8d787530`, `4b6b4abc`, `a7bee0f5`, `d513f13a` (документ r2), +`046efe96`, `444562e4`. Формально они делятся на «закрытие H1–H3 из r2» и одно **не заявленное +в r2 замечание** — CI поймал регресс DOM после live-reconciliation ещё на `a7bee0f5` (до публикации +документа r2), и он же чинится в этом окне. Разбираю всю дельту по коду, а не только «после +документа r2», потому что часть закрывающих коммитов (H2 — `fe03eae2`, H3 — `4b6b4abc`) физически +предшествует публикации `d513f13a` — r2 их не проверял (его собственная цитата диапазона +`1850eb18..cb68492c` их не включает), и раз так, «наследовать без проверки» для них не могу. + +## Как проверялось + +Дельта не геометрию персистентного формата, а раннее прекращение/typing нескольких вызовов и одну +внутреннюю функцию (`resizeLivePreflightAllowed`). Прогнал сам, не полагаясь на цитаты из комментариев: + +| Команда | Результат | +|---|---| +| `npm run build` (после — `git status --short`) | зелёный, дерево не изменилось — бандл воспроизводим байт-в-байт | +| `npm run bundle:sync` | зелёный, стенд для браузерных смоков синхронизирован | +| `npm test` | 1956 passed / 0 failed / 1 skipped (совпадает с заявленным) | +| `node scripts/check-docs.mjs` | «Documentation checks passed (7 files, 12 external links)» — H1 закрыт | +| `node scripts/no-new-any.mjs --base cb68492c --head 444562e4` | «Новых any нет» (77 строк в 5 файлах) — H2 закрыт для всей дельты, не только для `fe03eae2` | +| `node demo/smoke_room_resize.mjs` | OK — H3 закрыт (`preflight_visible_reason`/`preflight_reason_once` снова true/1) | +| `node demo/smoke_resize_pointer_real_plan.mjs` | OK — закрыт не заявленный в r2 регресс `unrelated_pointer_ignored`/`capture_loss_restores_dom` | +| `node demo/smoke_resize_outer_reconciliation.mjs` | OK | +| `node demo/smoke_resize_audit_1550.mjs` | OK | +| `node demo/smoke_resize_inner_dimensions.mjs` | OK | +| `node demo/smoke_resize_labels.mjs` | OK | +| `node demo/smoke_resize_wall_thickness.mjs` | OK | +| `node demo/smoke_junction_limits.mjs` | OK | +| `node demo/smoke_wall_key_roundtrip.mjs` | OK | +| `node demo/smoke_bg_color.mjs` | OK | +| `node demo/smoke_pan_any_zoom.mjs` | OK | + +Плюс независимая архивная проверка через `gh`: CI `33920551036` (SHA `a7bee0f5`, до публикации r2) +показал `Смоки в браузере (шард 3 из 3): failure` с точными именами +`resize_pointer.unrelated_pointer_ignored` / `resize_pointer.capture_loss_restores_dom` — это +доказывает, что регресс был реальным (тест умел падать), а не выдумкой из комментария автора. +Финальный CI `33922485666` (SHA `444562e4`) зелёный целиком, включая все 3 шарда смоков и агрегатор. + +`node scripts/smoke-select.mjs --base cb68492c --head 444562e4`: 29 прямых совпадений (`_resize`, +`_openingsR`, `_spaceWalls`, `NORM_W`, `_junctionLimitViolations`, `_mode`, `_spaceDisplayForRender`, +`SpaceModel`, `WallEntry`). Прогнал все, что касаются resize/geometry напрямую (список выше, 11 из +29); остальные 18 прямых совпадений — тот же класс, что уже прогнан (`_openingsR`/`NORM_W`-смоки +опенингов/декора, не тронутых логически в этой дельте, тип-рефакторинг без изменения поведения) — +не гонял отдельно, см. «Чего не проверял». + +## Закрытие r2 + +| Находка r2 | Чем закрыта | Где видно | +|---|---|---| +| **H1** (`check-docs --external` красный, отпечаток скриншотов устарел) | Каноническая Linux-съёмка на точном SHA, обновлён только source fingerprint | `444562e4`; сам прогнал `node scripts/check-docs.mjs` на HEAD — зелёный | +| **H2** (11 новых `any` в `houseplan-card.ts:7792`, `junction-limits.ts:51,54,57,73`, `live-editor.ts:61,62,65,157,163,164`) | Типизация `_junctionLimitViolations`/`junctionLimitViolations` через новый экспортируемый `JunctionSharedGeometry` (`Pick \| {status:'lightweight',...}`), typed `LiveEditorHost` (`SpaceDisplay`, `SpaceModel`, `WallEntry`, `RenderOpening`) | `fe03eae2` (`src/houseplan-card.ts`, `src/houseplan-editor-runtime.ts`, `src/junction-limits.ts`, `src/live-editor.ts`); сам прогнал `no-new-any.mjs --base cb68492c --head 444562e4` — 0 новых `any` | +| **H3** (`_rszProjectPreview` больше не вызывает `_checkSpacePhysicalGeometry()` ни разу во время drag — `smoke_room_resize` падал) | Новая `resizeLivePreflightAllowed(rooms, edgeBudget=64)`: если суммарный периметр (число вершин) комнат кандидата ≤ 64, `_rszProjectPreview` вызывает `_rszSpaceCandidateGeometry` → тот же `_checkSpacePhysicalGeometry`, что и раньше, и отклоняет шаг при `!ok`; иначе (большие планы) остаётся только дешёвая junction-limit проверка, а точная — безусловно на `pointerup` через `_commitPhysicalGeometry` (не изменился, вызывает `_checkSpacePhysicalGeometry` без всяких условий) | `4b6b4abc` (`src/resize-controller.ts:resizeLivePreflightAllowed`, `src/houseplan-editor-runtime.ts:3653,3691,3715-3734`); прочитал `_commitPhysicalGeometry` (строки 2126–2148) — вызов fail-closed проверки безусловный, не зависит от live-preflight; сам прогнал `smoke_room_resize.mjs` — OK | + +Дополнительно (не было заявлено r2 находкой, но CI поймал регресс на `a7bee0f5` — SHA, на котором +физически лежал код r2 к моменту анализа, хотя r2 цитировал более ранний `cb68492c`): смоки +`resize_pointer.unrelated_pointer_ignored`/`capture_loss_restores_dom` красные. Причина — +`_rszMove` ставил в очередь `_rszMoveNow` для ЛЮБОГО pointerId (проверка владения была только внутри +`_rszMoveNow`), из-за чего чужой указатель вытеснял из очереди уже поставленное обновление +настоящего владельца. Закрыто `046efe96`: guard `ownsPointer` перенесён в начало `_rszMove` (до +постановки в очередь), плюс новый флаг `_resizeBaseFrameStable` — если во время resize случился +полноценный render (`routeLiveEditorUpdate` вернул «не live» при активном `_resize.preview`), +`_rszCancelDrag` больше не притворяется, что можно оставить старый терминальный DOM, и делает один +обязательный reconciliation render. Сам прогнал `smoke_resize_pointer_real_plan.mjs` — OK; независимо +подтверждено CI `33922485666` (все 3 шарда зелёные). + +## Унаследовано из r1 (через r2, без повторной проверки) + +Эта дельта не касается доказательной базы следующих пунктов r1/r2 — принимаю как есть: + +- AC1–AC2, AC7 (разделение intake/visual invalidation, dependency projection, last-wins HA во + время жеста) — не тронуты дельтой r2→r3 (файлы фильтра `hass`/dependency classifier в диффе + `cb68492c..444562e4` отсутствуют). Документ: `CODE-REVIEW-451-r2.md`, SHA `cb68492c`. +- AC4 (diagnostics cache) — не тронут этой дельтой. +- AC9 (golden/canonical screenshots) — r2 подтвердил 153/153 без диффов на `cb68492c`; резерв + CI на `444562e4` («Переиспользование: это дерево уже проверено» → успех, «Golden-кадры» → skipped) + подтверждает, что источник для golden не менялся с последнего полного прогона. Отдельно не + перепрогонял: дельта r2→r3 не меняет settled-состояние (см. ниже про resize-preflight — эффект + только во время активного жеста, до golden-снимка дело не доходит). +- Инварианты модели по всем моделям проекта — часть `npm test` (сам прогнал на HEAD, зелёный); + конфиг-специфичная команда `npm run invariants -- --config …` не нужна отдельно: дельта не меняет + персистентную форму (`walls[]`, `wall_segments`, `marker.space`, `open_spans`) — только момент + вызова validation-функции и типизацию сигнатур. +- `INITIAL_VIEW_GZIP_CEILING` 297000→298000 — обоснование и бюджет не тронуты этой дельтой (только + `bundle-budget.mjs` логика из r1, файл не в диффе r2→r3). + +## Находки + +### M1 — не заявленный в ТЗ постоянный отказ от live-preflight на больших планах, противоречит собственному changelog (в скоупе, чинится в этой же задаче) + +**Файлы:** `src/resize-controller.ts:18-27` (`resizeLivePreflightAllowed`), +`src/houseplan-editor-runtime.ts:3653` (использование в `_rszProjectPreview`). + +Фикс H3 (`4b6b4abc`) не просто вернул прежнее поведение — он расколол его по размеру плана. +Для «bounded» контуров (суммарно ≤ 64 вершин комнат текущего пространства) во время resize-жеста +работает тот же точный `_checkSpacePhysicalGeometry()`, что был до #451: невозможная геометрия сразу +отклоняется, тост «последняя безопасная позиция» показывается посреди жеста. Для «больших» планов +(> 64 вершин) эта проверка **безусловно выключена во время жеста** — живёт только дешёвая +junction-limit проверка (углы/валентность), а точная геометрия проверяется один раз, на `pointerup` +(это безопасно для данных — commit всё ещё fail-closed, я прочитал `_commitPhysicalGeometry` +построчно, — но не для обратной связи пользователю). + +Порог не абстрактный: фикстура `demo/fixtures/large-house.mjs`, на которой считается сам бюджет +`large-house-interaction-v1` (тот самый профиль, ради которого выключалась проверка), — это 20 +прямоугольных комнат по 4 вершины = **80 вершин**, то есть уже выше порога 64. А персона из ТЗ +(«администратор дома... средний или большой план», issue: «5 пространств... 8 комнат в текущем +пространстве... средний по размеру план, не рекорд») — это ровно тот случай, для которого порог +скорее всего будет превышен, если комнаты не прямоугольные (8 комнат × 8 вершин = 64, граница +ровно на пороге). + +Почему это находка, а не техническая деталь реализации: + +1. **ТЗ прямо запрещает это как область изменения.** §5 Non-scope: «изменение hit areas, snap + tolerance, grid, gesture thresholds, animation duration/easing, click/double-click/long-press + или commit/cancel semantics» не входит в задачу. §2: «Преднамеренных визуальных изменений нет. + После завершения любого жеста канонический итог и итоговый кадр совпадают с текущим поведением» + — про итоговый кадр это верно (commit не изменился), но живая обратная связь **во время** жеста + для больших планов теперь другая, и это не техническая деталь — это то, что видит пользователь. + Ни §16 (риски), ни §18 (что можно менять свободно без владельца) не упоминают эту развилку. +2. **Прямо противоречит changelog, который уже в ветке.** `docs/CHANGELOG.md`/`.ru.md` (коммит + `c0d61ca3`, до r1) заявляют: «interactions remain visually and functionally unchanged» / + «внешний вид и результат действий не меняются». Для планов с > 64 вершин комнат в текущем + пространстве это не так: раньше resize посреди жеста предупреждал о невозможной позиции, теперь — + нет. +3. **Ни в одном комментарии issue владелец этот компромисс не видел и не утверждал** — я прочитал + всю переписку (`Аналитика`, `Вопросы для ТЗ`, `Решения владельца`, оба «Исправления по + CODE-REVIEW»): обсуждались Q1 (relevant HA update во время жеста) и Q2 (обычный hover), но не + порог 64 и не деление resize-preflight по размеру плана. + +Это Medium, не High: данные не портятся ни при каком сценарии (проверил код коммита, unconditional +fail-closed check на pointerup), поэтому это не блокирующий баг, а нераскрытое изменение поведения +вне заявленного скоупа задачи — ровно формулировка PROCESS.md «жёлтый вердикт допустим при +выполненных AC, если изменение ухудшает смежный сценарий». Находка в скоупе (введена этой же веткой +ради выполнения AC10) — чинится в этой же задаче, отдельный issue не заводится. + +**Что нужно от автора (не мой выбор, а перечисление опций для очередного цикла):** либо восстановить +живую точную проверку и для больших планов (тогда решать заново конфликт с AC10-бюджетом, из-за +которого её и убрали в r1), либо получить у владельца явное решение принять этот компромисс и +одновременно поправить формулировку changelog («без функциональных изменений» перестаёт быть верным +для больших планов), либо найти более дешёвый способ живой проверки (например, только у затронутой +комнаты вместо канонического union всего пространства — H1-r1 уже показал, что именно full clearance +был дорогим, а не сам факт проверки). + +## Что проверено и корректно + +- H1/H2/H3 из r2 закрыты — таблица выше, каждая проверена самостоятельно прогоном, а не с чужих слов. +- Регресс DOM `resize_pointer.unrelated_pointer_ignored`/`capture_loss_restores_dom` (пойман CI на + `a7bee0f5`, не был отдельной находкой r2) закрыт `046efe96`, подтверждено прогоном и CI. +- Commit-time geometry check (`_commitPhysicalGeometry` → `_checkSpacePhysicalGeometry`) остаётся + безусловным независимо от размера плана — прочитано построчно, не зависит от M1. +- Типизация (`fe03eae2`) семантически эквивалентна прежнему `any`-коду — прочитал diff + `junction-limits.ts` построчно (переход через промежуточный `completeGeometry` даёт тот же результат + для 'ok'/'degraded-extra'/'lightweight'/`null`/`undefined`, что и старое тройное сравнение). + `live-editor.ts` получил один дополнительный guard (`!!room.id &&`) — сужение, не расширение + поведения, риска регрессии нет. +- Новый unit-тест `resize-controller.test.mjs` («#451 live resize preflight is bounded by authored + contour complexity») проверяет именно границу (16×4=64 → true, 17×4=68 → false, один контур 65 → false) + — тест способен падать (проверил через чтение реализации: убери `<=` на `<`, и 64 упадёт). +- `no-new-any`, `check-docs`, `npm test`, `npm run build`+`bundle:sync` (сверка бандла) — зелёные + на точном HEAD, прогнано лично. +- Трейлеры `Issue: #451` — присутствуют во всех коммитах дельты. `User-Visible: no` для `046efe96` + и `444562e4` — точен (внутренний DOM-баг и обновление отпечатка, не новое поведение). Для `4b6b4abc` + трейлер тоже `User-Visible: no` — и вот это неточно ровно по причине M1. + +## Чего не проверял и почему + +- Полный `npm run golden:verify` — не гонял отдельно: дельта не меняет settled-состояние (эффект + M1 виден только во время активного resize-жеста, до commit/settled-кадра), а CI на HEAD показал + «Переиспользование: это дерево уже проверено» → golden job skipped легитимно (тот же tree-hash, + что уже проверялся 153/153 в r2). +- Полный `npm run benchmark:large-house-interaction` — не перегонял: диапазон дельты не меняет + веса editor-series (единственное затронутое ответвление — resize live-preflight, а он теперь + для фикстуры large-house **выключен** тем же порогом 64 < 80, то есть числа бюджета из r2 + (`editor median 484.7 ms ≤ 750`) по построению не изменятся от этой дельты — сам факт этого и есть + часть находки M1: бюджет проходит именно потому, что фикстура выше порога). +- Оставшиеся 18 «прямых совпадений» smoke-select (декор/opening-смоки на `_openingsR`/`NORM_W`) — + дельта в этих файлах чисто типовая (import type вместо value import, сигнатуры), не гонял отдельно; + 25 «слабых» совпадений на `_mode` не гонял — типовой рефакторинг чужого для них кода. +- `python -m pytest tests_backend` — диф не трогает `custom_components/**/*.py` (проверено чтением + `git diff --name-only`). +- `npm run invariants -- --config <...>` для конкретного конфига — не требуется отдельно (см. + «Унаследовано», дельта не меняет персистентную геометрическую форму). +- Мутация для `resizeLivePreflightAllowed` — не заводил: функция чистая, покрыта unit-тестом на все + три граничных случая, и мутационный тест не добавил бы нового при уже красном-способном юните. + +## Итог + +**Вердикт: жёлтый.** High: 0, Medium: 1 (M1, в скоупе — возвращается автору, отдельный issue не +заводится). H1–H3 из r2 закрыты корректно, дополнительный DOM-регресс закрыт корректно. Единственная +находка этого раунда — не баг в данных, а нераскрытое и не согласованное с ТЗ/changelog изменение +поведения resize-preflight для больших планов, появившееся как побочный эффект закрытия H3. + +--- + + + +## Материал раунда + +- Ветка: `issue/451-render-performance`, коммит `444562e47cb7` — ребейз его осиротит, и это нормально: ниже якоря, которые ребейз не меняет. +- Дерево материала: `9489e0f2e24a6b4df6bd587703b2fa058ebb3a04` + ``` + git log --all --format='%H %T' | grep 9489e0f2e24a + ``` +- ТЗ `docs/specs/451-render-performance.md`, блоб `7c323a29110974aae369077214b9e2a74d9387c1` + ``` + git log --all --find-object=7c323a29110974aae369077214b9e2a74d9387c1 -- docs/specs/451-render-performance.md + ```