30 KiB
CODE-REVIEW #451 — заход r4
- Issue: #451 — «План тормозит: диагностика считается на каждый кадр, перетаскивание перерисовывает всё, нет фильтра обновлений»
- Этап: код-ревью (PROCESS.md §2.7)
- Заход: r4 · блокирующих циклов до этого раунда: 3/4
- Материал раунда: ветка
issue/451-render-performance,HEAD = 07ba2ffbd1c02172d114ef27fb7cee2e40cf326e(свереноgit rev-parse HEADнепосредственно перед выводом, дерево чистое, PROCESS.md §2.7/#312) - Дельта раунда:
git diff 444562e47cb746dc1c7d740b66b2f832ca02f064..07ba2ffb— SHA взят из машинного блока «Материал раунда» документаdocs/reviews/CODE-REVIEW-451-r3.md(дерево9489e0f2e24a6…, свереноgit log --all --format='%H %T' | grep 9489e0f2e24a— совпадает с444562e4). Файлы дельты (безdist/**и копий бандла стенда):src/houseplan-card.ts,src/houseplan-editor-runtime.ts,src/resize-controller.ts, новыйsrc/resize-live-preflight.ts,test/resize-controller.test.mjs,demo/benchmark_large_house.mjs,demo/performance/card-contract.mjs,docs/images/screenshots.json,docs/reviews/CODE-REVIEW-451-r3.md.
Скоуп раунда
r3 (жёлтый, High:0/Medium:1) закончился на 444562e4. Единственная находка r3 — M1: фикс H3 из r2
не просто вернул прежнее поведение, а ввёл порог resizeLivePreflightAllowed(rooms, edgeBudget=64) —
точная physical-geometry проверка во время resize-жеста была живой только для планов ≤ 64 вершин
комнат, для больших планов работала только на pointerup. Не заявлено в ТЗ, противоречило changelog
«внешний вид и результат действий не меняются».
Автор ответил одним коммитом 5692a288 («fix: preserve live resize validation on large plans»,
User-Visible: no): порог полностью удалён, вместо него — новый модуль resize-live-preflight.ts с
тремя функциями (resizeLiveRoomIds, resizeLiveJunctionRoomIds, resizeLiveCandidateSpace), которые
строят «локальный кандидат» — только затронутые комнаты плюс геометрически близкие к их границе
стены/сегменты/партиции/проёмы — и гоняют через него ту же самую _checkSpacePhysicalGeometry,
безусловно, на любом размере плана. 07ba2ffb поверх — только обновление отпечатка скриншотов
документации (docs: refresh screenshot fingerprint for #451).
Разбираю всю дельту по коду: это новый файл на 210 строк с нетривиальной геометрией (distance-to-segment, segment-intersection, AABB-touch), напрямую заменяющий защитный механизм, который весь путь r2→r3→r4 и был предметом ревью.
Как проверялось
Дельта не трогает персистентный формат — только момент и объём вызова уже существующей проверки.
Валидация на точном HEAD (07ba2ffb) уже зелёная в CI: Validate run 33927104551,
success. Это покрывает npx tsc --noEmit, npm test, npm run build+сверку бандла и check-docs
(docs job зелёный на этом SHA) — не перегонял эти четыре повторно, см. правило дешёвых гейтов §8/§2.10.
Что прогнал сам в этом раунде:
| Команда | Результат |
|---|---|
npx tsc -p tsconfig.test.json && node scripts/fix-test-build.mjs |
зелёный (нужно для собственных проб ниже) |
node scripts/no-new-any.mjs --base 444562e4 --head HEAD |
«Новых any нет» (225 строк в 3 файлах) |
npm run build && npm run bundle:sync |
зелёные, дерево бандла пересобрано без ошибок |
node scripts/smoke-select.mjs --base 444562e4 --head HEAD |
19 прямых совпадений + 2 зарегистрированные связи, все по теме wall-union/junction/masonry/resize (список ниже) |
node demo/smoke_room_resize.mjs |
OK — реальные (не замоканные) сценарии owner_boundary_clamped, corner_clamped, mixed_role_* не задеты |
node demo/smoke_resize_pointer_real_plan.mjs |
OK |
node demo/smoke_junction_holes.mjs |
OK |
node demo/smoke_glow_fail_dark.mjs |
OK |
node demo/smoke_glow.mjs |
OK |
node demo/smoke_junction_patch_resilience.mjs |
OK |
node demo/smoke_multiwall_junction.mjs |
OK |
node demo/smoke_multiwall_strip_containment.mjs |
OK |
node demo/smoke_opening_measure.mjs |
OK |
node demo/smoke_optional_space_model.mjs |
OK |
node demo/smoke_wall_key_roundtrip.mjs |
OK |
node demo/smoke_wall_thickness_transition.mjs |
OK |
node demo/smoke_wall_union_isolation.mjs |
OK (включая генуинный, не замоканный degradedPhysicalEditRejected на commit-пути) |
node demo/smoke_zero_divider_taper.mjs |
OK |
node demo/smoke_real_plan_masonry.mjs |
OK («зарегистрированная связь» smoke-select — реальный план, разрывы кладки, которые синтетика не ловит) |
node demo/smoke_resize_wall_thickness.mjs |
OK («зарегистрированная связь») |
Плюс собственная проба (не входит в существующий гейт, привожу как воспроизведение находки ниже):
прямой вызов checkSpacePhysicalGeometry/resizeLiveCandidateSpace/resizeLiveJunctionRoomIds из
test-build/*.js на реальном производственном фикстуре регрессии test/fixtures/278-wall-union-isolation.json
(#278) плюс синтетические «дальние» комнаты для имитации большого плана.
Закрытие r3
| Находка r3 | Чем закрыта | Где видно |
|---|---|---|
M1 (порог edgeBudget=64: точная live-проверка выключена целиком для планов > 64 вершин комнат, не заявлено в ТЗ/changelog) |
Порог resizeLivePreflightAllowed удалён вовсе (src/resize-controller.ts, было 13 строк функции — теперь нет). Вместо него resizeLiveCandidateSpace/resizeLiveRoomIds (src/resize-live-preflight.ts) строят локальный физический кандидат и _rszProjectPreview гоняет _checkSpacePhysicalGeometry через него безусловно, на любом размере плана — деление по порогу исчезло как класс |
5692a2880f9ba645b9e2a897a828a9ea169a3a1a (src/houseplan-editor-runtime.ts:3654-3656); сам прочитал — вызов больше не обёрнут в if (resizeLivePreflightAllowed(...)); demo/benchmark_large_house.mjs теперь структурно требует resizeLivePreflightChecks >= 1 на large-house фикстуре (80 вершин, выше старого порога) — не даёт тихо вернуть скип |
Формально M1 закрыта в буквальном прочтении (порог убран, вызов безусловный на любом размере). Но дельта заменила один раскрытый компромисс на другой, нераскрытый — см. находку ниже: сам факт «вызов происходит всегда» не означает «проверка обнаруживает то же самое, что обнаруживала до #451».
Находки
M1 (r4) — локальный кандидат для physical-geometry исключает соседнюю комнату, от которой зависит валидность стыка; живая проверка может пропустить реальный дефект кладки на любом размере плана (в скоупе, чинится в этой же задаче)
Файлы: src/resize-live-preflight.ts:59-65 (resizeLiveRoomIds), :91-124 (resizeLiveCandidateSpace),
src/houseplan-editor-runtime.ts:3654-3656 (_rszProjectPreview, использование).
resizeLiveCandidateSpace(sp, changedRoomIds) строит кандидат для _rszSpaceCandidateGeometry (→
_checkSpacePhysicalGeometry), отбирая только комнаты, чей id входит в changedRoomIds
(resizeLiveRoomIds — точное совпадение id, без расширения на соседей). Стены/сегменты/партиции
отбираются отдельно, по геометрической близости к границе этих комнат. Соседняя комната, которая
физически не изменилась в этом кадре (её id не в changedRoomIds), в кандидат не попадает вовсе — даже
если общий с ней узел/стена как раз и есть источник невалидности.
Это не гипотетическое рассуждение о коде — воспроизвёл прогоном на реальном production-фикстуре
регрессии #278 (test/fixtures/278-wall-union-isolation.json, две комнаты r1/r2, общая стена
толщиной 20, историческая дефектная кладка, статус degraded-extra):
import { checkSpacePhysicalGeometry } from './test-build/plan-geometry-preflight.js';
import { resizeLiveCandidateSpace } from './test-build/resize-live-preflight.js';
// baseSpace = spaces[0] из test/fixtures/278-wall-union-isolation.json (r1, r2, стена 20см)
// + 80 «дальних» синтетических комнат (10..90 по x), чтобы получить план > 64 вершин.
checkSpacePhysicalGeometry({ spaces: [largeSpace] }, largeSpace.id)
// -> { status: 'failed', reason: 'wall-degraded-extra', ok: false } ← ПОЛНОЕ пространство: дефект виден,
// независимо от того, сколько в плане посторонних комнат (80 или 0)
const liveSpace = resizeLiveCandidateSpace(largeSpace, ['r1']); // r1 «изменилась», r2 — нет
liveSpace.rooms.map(r => r.id) // -> ['r1'] (r2 выброшена целиком)
checkSpacePhysicalGeometry({ spaces: [liveSpace] }, largeSpace.id)
// -> { status: 'ok', ok: true } ← ТОТ ЖЕ дефект больше не виден
Итог: во время resize-жеста, когда двигается только r1, а r2 (владелец второй стороны той же
дефектной стены) не входит в changedRoomIds, живая проверка сообщает «геометрия в порядке» для
кандидата, который на самом деле нарушает контракт масонри — тот же самый контракт, ради которого
существует _checkSpacePhysicalGeometry и весь путь #278/H3(r2)/M1(r3). Это воспроизводится на любом
размере плана — добавление или удаление 80 посторонних комнат ничего не меняет, значит находка не
«ещё один порог», а более фундаментальная: набор комнат для physical-geometry кандидата определяется
по changedRoomIds (точное совпадение id), а не по геометрической смежности.
Показательно, что для другого кандидата в том же методе — _resizePreviewNodes (junction-limit
проверка, #329) — автор уже использует resizeLiveJunctionRoomIds (один слой AABB-соседей), а не голый
changedRoomIds. Подставил тот же более широкий набор в physical-geometry кандидат — и дефект снова
виден:
const junctionIds = resizeLiveJunctionRoomIds(largeSpace.rooms, ['r1']); // -> ['r1', 'r2']
const liveSpaceViaJunctionIds = resizeLiveCandidateSpace(largeSpace, junctionIds);
checkSpacePhysicalGeometry({ spaces: [liveSpaceViaJunctionIds] }, largeSpace.id)
// -> { status: 'failed', reason: 'wall-degraded-extra', ok: false } ← дефект снова обнаружен
Почему это Medium, а не техническая деталь:
- Данные не портятся.
_commitPhysicalGeometry(не в этой дельте, прочитал — не изменился) вызывает_checkSpacePhysicalGeometryна полном пространстве безусловно наpointerup. Невалидная геометрия всё ещё не может сохраниться. - Но живая обратная связь пользователю может отсутствовать именно там, где её восстановление и было
предметом M1(r3). Пользователь дотягивает жест до конца, не видя «последней безопасной позиции», а
затем получает отказ commit (
resize.commit_failed, класс регресса, который уже описан в H3 r2) без предупреждения по пути — то есть худший, а не лучший исход по сравнению с раскрытым порогом r3: тот хотя бы предсказуемо и одинаково выключал проверку выше 64 вершин; этот — непредсказуемо, в зависимости от того, какая именно комната «официально изменилась» в данном кадре resize-солвера, и на любом размере плана, включая маленькие. - Ни ТЗ, ни changelog, ни коммит-сообщение (
fix: preserve live resize validation on large plans,User-Visible: no) не упоминают эту границу — коммит заявляет ровно противоположное тому, что происходит для дефектов, зависящих от соседней комнаты. - Существующее покрытие не могло эту находку поймать. Новые unit-тесты в
test/resize-controller.test.mjsпроверяют только структуру фильтрации (какие id/стены попадают в кандидат), не пропуская результат черезcheckSpacePhysicalGeometry. Новая проверка вdemo/benchmark_large_house.mjs(deltas.resizeLivePreflightChecks < 1роняет раннер) доказывает только, что проверка вызывается, а не что она может вернутьfalseдля реально невалидного кандидата.smoke_room_resize.mjs(единственный реальный смок с геометрией без мока) использует фикстуры, где невалидность — либо однокомнатная топология (corner_clamped), либо ровно совпадающая с существующей стеной (owner_boundary_clamped,wall-metadata-путь, отдельная от physical-geometry проверка, использует полныйsp.rooms— не задета этой находкой); ни один существующий тест не строит #278-подобный «дефект стыка, видимый только если обе стороны в модели».
Что нужно от автора (не мой выбор, перечисление опций для следующего цикла): либо расширить набор
комнат physical-geometry кандидата тем же способом, что уже используется для junction-кандидата
(resizeLiveJunctionRoomIds, один слой AABB-соседей — проба выше показывает, что этого достаточно для
воспроизведённого случая; открытый вопрос — достаточно ли одного слоя для более сложных многосторонних
узлов, это стоит явного теста), либо обосновать и явно задокументировать в ТЗ/коде, почему набора
changedRoomIds достаточно для physical-geometry (если у меня неверна модель угрозы), и в любом случае
добавить тест, который прогоняет resizeLiveCandidateSpace-кандидат через checkSpacePhysicalGeometry
на заведомо дефектной (не замоканной) геометрии — по образцу пробы выше, лучше всего на самом фикстуре
#278, — чтобы AC «восстановлена live-валидация» имело названного свидетеля, который умеет краснеть.
Унаследовано из r3 (и через r3 из r1/r2), без повторной проверки
Эта дельта не касается доказательной базы следующих пунктов — принимаю как есть:
- H1–H3 из r2 (docs fingerprint, новые
any, регрессsmoke_room_resize) и дополнительный DOM-регресс (resize_pointer.unrelated_pointer_ignored/capture_loss_restores_dom) — файлы их фиксов (houseplan-card.tsтипизация junction-limits,live-editor.ts,046efe96DOM-guard) не входят в дельту444562e4..HEAD. Документ:CODE-REVIEW-451-r2.md/-r3.md, SHAcb68492c/444562e4. - AC1–AC2, AC7 (разделение intake/визуальной инвалидизации, dependency projection, last-wins HA во время
жеста) — файлы фильтра
hass/dependency classifier не в дельте444562e4..HEAD. Документ:CODE-REVIEW-451-r2.md, SHAcb68492c. - AC4 (diagnostics cache) — не тронут этой дельтой.
- AC9 (golden/canonical screenshots) — единственный тронутый в этой дельте артефакт —
docs/images/screenshots.json(отпечаток источника, коммит07ba2ffb); Linux-съёмка на предыдущем SHA (444562e4) была канонической (r3: [run 33926924502]) и её результат (10/10 без диффов) не меняется этой дельтой — сама дельта не трогаетsrc/**рендер-путь, только момент вызова физической проверки во время resize, до commit/settled-кадра. Отдельно не перепрогонял golden — не требуется: результирующий кадр послеpointerupидентичен (полная проверка на commit не изменилась). - Инварианты модели по всем моделям проекта — часть
npm test(зелёный на HEAD в CI); конфиг-специфичная командаnpm run invariants -- --config …не нужна отдельно: дельта не меняет персистентную форму (walls[],wall_segments,marker.space,open_spans) — только состав кандидата, временно собираемого в памяти для live-проверки во время жеста, никогда не записываемого в конфиг. INITIAL_VIEW_GZIP_CEILING297000→298000 и его обоснование — не тронуты этой дельтой.
Что проверено и корректно
- Порог
resizeLivePreflightAllowed/edgeBudget=64из r3 действительно удалён целиком — прочитал diffresize-controller.ts, функции больше нет;_rszProjectPreviewбольше не содержит ветвления по размеру плана для physical-geometry вызова. - 17 из 21 отмеченных
smoke-selectтестов (19 прямых + 2 зарегистрированных за вычетом 6 слабыхcellCm-совпадений, см. «Чего не проверял») прогнаны лично, все зелёные — включая генуинный (не замоканный) commit-timedegradedPhysicalEditRejectedвsmoke_wall_union_isolation.mjsи реальный план вsmoke_real_plan_masonry.mjs. - Реальные (не замоканные) мелкоплановые сценарии
smoke_room_resize.mjs(owner_boundary_clamped,corner_clamped,mixed_role_*) не регрессировали при переходе на безусловный локальный кандидат — прогнал, зелёные. _commitPhysicalGeometry(полное пространство, безусловный вызов наpointerup) не тронут этой дельтой — прочитал, данные при коммите остаются fail-closed независимо от находки M1(r4): невалидная геометрия не может сохраниться, регресс только в live-обратной связи посреди жеста.- Новые unit-тесты
resize-controller.test.mjsкорректно проверяют то, что они заявляют проверять (структура фильтрации id/стен/партиций/проёмов на 100-комнатном синтетическом плане) — прочитал, тест умеет падать на этом узком контракте (например,resizeLiveRoomIdsвернёт лишний id, если убрать фильтр поchanged). Находка M1(r4) не в том, что эти тесты неверны, а в том, что они не покрывают промежуточный конечный результат (checkSpacePhysicalGeometryна построенном кандидате). no-new-any --base 444562e4 --head HEAD— 0 новыхanyв 225 добавленных строках 3 файлов.- Новая инструментация
demo/benchmark_large_house.mjs(physicalPreflightCount/physicalPreflightMs, ассертresizeLivePreflightChecks < 1) корректно доказывает то немногое, что доказывает: вызов происходит хотя бы раз во время editor-резайза на large-house фикстуре — не более. - Трейлеры
Issue: #451присутствуют,User-Visible: noдля5692a288и07ba2ffb— точны для заявленного эффекта (реализация не меняет видимое поведение, когда проверка срабатывает), но не раскрывают найденную M1(r4) границу, где она не срабатывает.
Чего не проверял и почему
- Полный
npm run golden:verify,npx tsc --noEmit,npm test,npm run build+сверка бандла отдельно от CI — не перегонял: Validate зелёный на точном HEAD07ba2ffb(run 33927104551), дешёвые гейты §8 сошлись на этом прогоне. - 6 слабых
smoke-select-совпадений по единственному общему идентификаторуcellCm(smoke_backdrop_guard,smoke_danger_confirmation,smoke_decor,smoke_grid_scale_invariance,smoke_help_affordance,smoke_space_scale_defaults) — не гонял:cellCm— параметр с fallback по умолчанию в новом файле, общий для всей кодовой базы идентификатор без содержательной связи с темой этих смоков (ни один не про resize/physical-geometry); риск по существу покрыт целевыми wall-union/junction/resize смоками выше. - Полный
npm run benchmark:large-house-interaction— не перегонял сам; автор привёл 7 прогонов с зелёными абсолютными бюджетами на этом SHA. Дельта этого раунда не меняет веса editor-series (то же число вызовов physical-geometry на move, что и раньше, только на другом наборе комнат) — по построению не должна была измениться, и находка M1(r4) не является перформанс-регрессией (наоборот: локальный кандидат обычно дешевле полного пространства). python -m pytest tests_backend— диф не трогаетcustom_components/**/*.py(провереноgit diff --name-only 444562e4..HEAD).- Мутация для
resizeLiveCandidateSpace/resizeLiveRoomIdsчерезscripts/mutation-gate.mjs— не заводил (это не моя роль); вместо этого привёл воспроизводимую пробу через прямой вызов скомпилированных пары чистых функций на production-фикстуре #278 — она и есть демонстрация «чем краснеет» для находки M1(r4). Постоянный мутант в гейте — тоже часть того, что нужно от автора при закрытии находки. - Полная browser-smoke матрица (222 файла) — не прогонял; это предрелизный гейт (PROCESS.md §8), дельта
локальна (4 файла кода) и
smoke-selectпо точному диапазону раунда покрыл релевантную тему.
Итог
Вердикт: жёлтый. High: 0, Medium: 1 (M1(r4), в скоупе — возвращается автору, отдельный issue не
заводится). Порог из M1(r3) действительно удалён, но замена (resizeLiveCandidateSpace с фильтрацией
комнат по точному changedRoomIds) вводит новый, более скрытый пробел того же класса: воспроизведён
прогоном на реальном production-фикстуре #278, что живая physical-geometry проверка может вернуть ok
для кандидата, эквивалентного заведомо дефектной (wall-degraded-extra) полной геометрии, если
источник дефекта — стык с соседней, формально «неизменившейся» комнатой. Данные не портятся
(_commitPhysicalGeometry на pointerup не изменился и остаётся безусловным), поэтому находка не
блокирует как High, но AC «restore live resize validation» в исходном смысле (было в r1/r2 до #451
сломавших это) не является полностью восстановленным для этого класса дефектов ни на одном размере
плана — направление минимального фикса (переиспользовать уже существующий resizeLiveJunctionRoomIds)
подтверждено той же пробой.
Материал раунда
- Ветка:
issue/451-render-performance, коммит07ba2ffbd1c0— ребейз его осиротит, и это нормально: ниже якоря, которые ребейз не меняет. - Дерево материала:
c2def02d1a1409564ed4eaa3661b663cbba356c1git log --all --format='%H %T' | grep c2def02d1a14 - ТЗ
docs/specs/451-render-performance.md, блоб7c323a29110974aae369077214b9e2a74d9387c1git log --all --find-object=7c323a29110974aae369077214b9e2a74d9387c1 -- docs/specs/451-render-performance.md