mirror of
https://github.com/Matysh/houseplan-card
synced 2026-10-02 21:01:21 +00:00
@@ -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<WallBodiesGeometryResult,...> \| {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.
|
||||
|
||||
---
|
||||
|
||||
<!-- material-anchors: сгенерировано конвейером (#414) -->
|
||||
|
||||
## Материал раунда
|
||||
|
||||
- Ветка: `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
|
||||
```
|
||||
Reference in New Issue
Block a user