diff --git a/docs/reviews/CODE-REVIEW-694-r1.md b/docs/reviews/CODE-REVIEW-694-r1.md new file mode 100644 index 00000000..b0f4256d --- /dev/null +++ b/docs/reviews/CODE-REVIEW-694-r1.md @@ -0,0 +1,233 @@ +# CODE-REVIEW-694-r1 + +Issue: #694 «Исследовать и устранить performance-регрессии v1.78 относительно v1.77» +Трек: ask · Заход: r1 · блокирующих циклов израсходовано 0 из 4 +Материал: `git log --oneline origin/dev..HEAD` = 1 коммит, +`35c89fc67dc8476dde0aab8c7f4e21ecea4c271f` +(`perf(card): a floor switch stops re-querying the same subtrees (#694)`), +рабочая копия на этом SHA. `git diff origin/dev...HEAD` — 9 файлов, +246 вставок / 20 удалений. + +## Скоуп + +Три точечных правки без изменения видимого поведения (`User-Visible: no`, +трейлер `Issue: #694` на коммите — оба присутствуют и корректны, changelog не +трогается, что и требуется при `no`): + +- **П1** `src/device-hit-owner.ts` — логика пачки записей + `MutationObserver` наблюдателя hover-указателя вынесена в чистую функцию + `deviceLayerMutated`: узел проверяется не больше раза за пачку (`Set`), а + после первого совпадения `.devlayer` проверки прекращаются. Добавленные + узлы по-прежнему безусловно проходят `syncAdded` + (`_syncPointerHoverSubtree`). +- **П2** `src/stairs-view.ts` `renderLayer` — геттер `_model` хоста читается + один раз в начале рендера вместо одного чтения на каждую лестницу. +- **П3** `src/i18n/language-runtime.ts` `languageRenderGate` — `setAttribute('lang', …)` + вызывается только когда значение отличается от текущего (`getAttribute`). + +Работа закрывает J1/J6-смежный сценарий из `docs/SCOPE.md` («живой план», +отзывчивость при частом действии — переключении этажей на планшете/кионе); +правки эксплуатационные, не меняют поверхность продукта, доп. вопросов к +`docs/USER-GUIDE.ru.md` нет — текста интерфейса они не касаются. + +## Материал: проверка целостности SHA + +`git log origin/dev..HEAD` содержит ровно 1 коммит, совпадающий с +`REVIEWER`-инструкцией (`35c89fc6`, рабочая копия уже на нём). Коммит несёт +`Issue: #694`, `User-Visible: no`, `Co-Authored-By`. Мутанты и AC-текст в +issue ссылаются на более ранний SHA ветки — `d6ad81120fef23822715cd396976a533ae980401` +(из комментария «Сделано»). Это не нарушение #312/#499: `d6ad8112` — тот же +логический коммит до pre-review rebase на ушедший вперёд `dev` (между ними +легли №724, №732, №733 и чисто докс-коммиты, не конфликтующие с файлами +#694). Проверено не на слово автора, а сравнением через GitHub API +(`compare/d6ad8112...35c89fc6`): патчи `src/device-hit-owner.ts`, +`src/stairs-view.ts`, `src/i18n/language-runtime.ts`, +`test/device-hit-owner.test.mjs`, `test/i18n-runtime.test.mjs`, +`test/stairs.test.mjs`, `tsconfig.test.json` и добавленные в +`scripts/mutation-registry.mjs` три записи `#694 …` — побайтово идентичны в +обоих коммитах. Единственная разница в `src/houseplan-card.ts` и остальных +записях `mutation-registry.mjs` между `d6ad8112` и `35c89fc6` — не связанный +со стеком #694 рефакторинг оверлеев `#724/#732` (`wallSilhouettes` → +`structure`), пришедший с `dev` при ребейзе. Вывод: доказательства AC1–AC5, +собранные на `d6ad8112`, валидны и для текущего материала `35c89fc6` — код +трёх правок и их тестов не менялся. + +## Как проверялось + +**Дёшевые гейты.** Validate `35c89fc6` (run [36794107188](https://github.com/Matysh/houseplan-card/actions/runs/36794107188)) +— зелёный только для джоба «Фронтенд: типы, юниты, мутанты, синхрон +бандла» (typecheck + `npm test` + сверка бандла). Проверено по факту через +`gh api .../jobs`, а не на слово: джобы «Мутанты по диффу», «Смоки в +браузере», «Golden-кадры», «Бэкенд: pytest», «Перф-смок» в этом прогоне — +`skipped`, не `success`. Поэтому `npx tsc --noEmit`/`npm test`/`npm run build` +не перегонял (сошлись по инструкции), а смоки и остальное — ниже, своими +руками, потому что Validate их не покрыл. + +**AC1 (unit).** Прочитан `deviceLayerMutated` и тест `test/device-hit-owner.test.mjs` +(`#694 AC1 …`, 3 теста). Логический разбор эквивалентности: исходный код +пересчитывал `inDeviceLayer` заново на каждую запись, но ответ детерминирован +текущим DOM на момент колбэка (все синхронные мутации уже произошли до того, +как наблюдатель получает управление), поэтому кэширование через `Set` в +границах одной пачки не меняет результат — при условии, что `syncAdded` +внутри пачки не меняет структуру поддерева. Проверил: `_syncPointerHoverSubtree` +(`src/houseplan-card.ts:7120`) только переключает атрибут +`data-pointer-hover`, структуру DOM не трогает — условие выполнено, кэш +безопасен. Тест `#694 AC1 a batch into one subtree queries each node at most +once` жёстко проверяет счётчик (`stage.queries === 2`, не больше) — +разрушающая мутация в реестре (`id: +pointer-hover-batch-requeries-shared-subtree`, убирает `checked.has(node)`) +эту проверку красит: без мемоизации тот же узел встретится как `target` +нескольких записей и будет опрошен повторно, счётчик вырастет. Мутант не +гонял (правило §10.4/#709 — ревьюер мутанты не применяет), но прочитал +реестр и тест и убедился, что разрушение действительно ломает конкретное +числовое утверждение теста — не косвенно, а напрямую. Проверено чтением, не +исполнением мутанта. + +**AC2 (unit).** `stairs-view.ts` `renderLayer` читает `this.owner._model` +один раз в локальную `const model` (строка 66) и переиспользует её и для +`spaceIds`, и для `targetTitle`. Тест `test/stairs.test.mjs` (`#694 AC2`) +гонит `StairViewRuntime(host).renderLayer()` при 0/1/2/7/40 лестницах и +проверяет `host.reads === 1` через spy-геттер — мутант +`stairs-view-reads-model-per-stair` возвращает второе чтение через +`this.owner._model.find(...)`, что при ≥1 лестнице подняло бы `reads` до 2 и +уронило утверждение. Подписи этажа (`targetTitle`) и tooltip-условия +(`active`) не меняются — тест это же и проверяет построчно по сценариям +upper/attic/ground/null/gone. + +**AC3 (unit).** `languageRenderGate` теперь делает +`if (host.getAttribute?.('lang') !== lang) host.setAttribute(...)`. +Опциональная цепочка — у хоста без `getAttribute` (как старый `FakeHost` в +уже существующих тестах) `undefined !== lang` всегда истинно, то есть +поведение для таких вызывающих не меняется — запись как раньше на каждом +рендере; регрессии существующих тестов нет (и это подтверждено зелёным +`npm test` на Validate). Реальные хосты (`houseplan-card.ts`, `space-card.ts`, +`space-editor.ts`, `editor.ts`) — настоящие DOM-элементы, `getAttribute` +есть всегда. Новый тест `#694 AC3` (`test/i18n-runtime.test.mjs`) гоняет +повторный рендер одного языка (`langWrites` остаётся `['en']`), смену языка, +английский fallback и смену языка после внешнего искажения атрибута — +мутант `language-gate-rewrites-unchanged-lang` убирает условие, тест на +точной последовательности `langWrites` это ловит. + +**AC4 (perf).** Full Performance на `d6ad8112` (run +[36790354931](https://github.com/Matysh/houseplan-card/actions/runs/36790354931), +создан 2026-09-30, `head_sha` подтверждён через `gh api`): все 9 джобов +зелёные. Цифры в issue (isometric `longTask.countP95` 13 vs порог 20, +`isometric-stage3` 24 vs 30, `plan-snap` 14 vs 24.3, остальные метрики с +запасом) соответствуют заявленному. Так как содержимое трёх правок +идентично между `d6ad8112` и материалом (см. раздел выше), повторный полный +прогон (часы CI-времени, не «дешёвый» гейт) не требуется — его смысла +дельта не меняет. Исключение AC4 (атрибуция к лестницам) не понадобилось, +все профили прошли бюджет без него. + +**AC5 (smoke + golden) — прогнано лично, Validate это пропустил.** +Собрал бандл (`npm run bundle:sync`, затем `npm run bundle:clean` после +проверки, чтобы не оставить дифф в `dist/`) и прогнал: +- `demo/smoke_stairs.mjs` — OK, 37/37 проверок; +- `demo/smoke_device_hit_capsules.mjs` — OK, 7/7; +- `demo/smoke_french_locale.mjs` — OK, 5/5, включая `langAttrIsFr: true` + (атрибут `lang` виден корректным и при записи-по-изменению). + +`node scripts/smoke-select.mjs --base a49f7095 --head HEAD`: «прямых +совпадений» и «зарегистрированных связей» нет; «слабая связь» — 24 смока по +символу `_model` (общий, не новый контракт — `_model` не меняется по +значению, меняется только частота чтения в `stairs-view.ts`), решение +ревьюера — не гонять весь список, а точечно проверить два смока, прямо +завязанных на переключение этажей/поведение `_model` при навигации: +`demo/smoke_fixed_floor.mjs` (OK, 14/14) и `demo/smoke_cold_view_toggle.mjs` +(OK, 12/12). Остальные 22 — не гонял: правка не меняет, что возвращает +`_model`, только сколько раз его читают за рендер, риск для их сценариев +отсутствует. «Визуальный минимум» (8 смоков) в выдаче `smoke-select` помечен +инструментом как часть `gate:small -- --smokes` (#690); не перегонял +отдельно — это тот же набор, что покрывает Validate-эквивалент `gate:small`, +а изменение не трогает ни один из восьми (markup/CSS-слоёв layout, grid, +decor-order и т.д. в диффе нет). +`golden:verify` не гонял: метки `ci:golden` на issue нет, а диффу нечего +регенерировать — ни один из трёх файлов не меняет шаблон/вёрстку/пиксели +(П1 — JS-логика обработчика, П2 — порядок чтения геттера, П3 — условие +записи атрибута при той же итоговой строке). `python -m pytest +tests_backend` не гонял: диффа в `custom_components/**/*.py` нет. +`npm run invariants` не гонял: диффа в геометрии (`space-geometry`, +`wall-thickness` и т.п.) нет. + +**Мутанты.** Три новых записи в `scripts/mutation-registry.mjs` +(`pointer-hover-batch-requeries-shared-subtree`, +`stairs-view-reads-model-per-stair`, `language-gate-rewrites-unchanged-lang`), +guard-команды нацелены точно на `#694 AC1`/`AC2`/`AC3`. `node +scripts/mutation-registry-check.mjs` — без вывода (реестр структурно +валиден). Мутанты не применял (§10.4/#709 — ловлю мутантов на любом треке +проверяет ночной прогон, не ревью); для каждого прочитал патч и соответствующий +тест и убедился разбором, что патч ломает конкретное числовое/последовательное +утверждение теста — не общую зелёность, а именно то число, которое AC +называет. + +## Находки + +Нет. High: 0. Medium: 0. Low: 0. + +Риски, названные автором в ТЗ, закрыты кодом: +1. `core-file-budget` на `src/houseplan-card.ts` — файл не вырос, а сжался + (12874 строки сейчас; диапазон #694 убрал в этом файле больше строк, чем + добавил — логика вынесена в `device-hit-owner.ts`). +2. Этаж с лестницами выше порога — не реализовалось, AC4 прошёл без + исключения. +3. П1 пропускает узел хотя бы раз — опровергнуто и разбором (кэш безопасен, + `syncAdded` не меняет структуру), и тестом (`synced` содержит все + добавленные узлы в порядке записей, включая узлы после первого хита). +4. П3 `lang` не восстановится, пока язык не сменится — поведение + подтверждено тестом (`a foreign value is corrected` только на смене + языка) и соответствует явно принятому предположению в ТЗ. + +## Что проверено и корректно + +- Трейлеры (`Issue: #694`, `User-Visible: no`) на единственном коммите + диапазона — присутствуют, changelog не тронут, что и требуется при `no`. +- `tsconfig.test.json` добавляет `src/stairs-view.ts` в проверяемый диапазон + — согласовано с новым тестом, который импортирует собранный + `stairs-view.js` из `test-build/`. +- Эквивалентность поведения П1–П3 старому коду доказана по каждому пункту + выше (AC1–AC3), не только наличием теста, но и прочтением логики на + предмет того, что тест действительно падает на внесённой мутации. +- AC4 (перформанс) и AC5 (смоки) — выполнены, перф подтверждён на + побайтово идентичном материале, смоки прогнаны лично на текущем SHA. +- Scope корректен: ровно П1–П3, никакого попутного рефакторинга или + расширения (архитектура DOM лестниц и прочие кандидаты профилирования + явно вынесены в «Не входит» / #725 и не затронуты). + +## Чего не проверял + +- Полный матричный прогон смоков (283 позиции) — не гейт ревью, предрелизная + обязанность; прогнаны только 3 названных в AC5 плюс 2 точечных из + «слабой связи». +- `golden:verify` — нет метки `ci:golden`, диффу нечего регенерировать + (аргументация выше). +- `python -m pytest tests_backend`, `npm run invariants` — неприменимо, + диффа в соответствующих поверхностях нет. +- Повторный Full Performance прогон на точном SHA `35c89fc6` — не гонял; + вместо этого подтвердил байт-в-байт идентичность содержимого трёх правок + между SHA прогона (`d6ad8112`) и материалом (`35c89fc6`) через GitHub + compare API, так что имеющийся зелёный прогон остаётся доказательством. +- Мутанты из реестра — не применял (правило: ревьюер мутанты не гоняет ни + на каком треке, ловлю проверяет ночной прогон, #709); проверил + разбором кода, что каждый из трёх ломает конкретное утверждение + соответствующего теста. +- HA E2E / Hassfest / HACS — не применимо, Python/манифесты не менялись. + +## Вердикт + +Зелёный. AC1–AC5 выполнены и доказаны (автотестами с подтверждённой +способностью падать на зарегистрированных мутантах — разбором, не +исполнением; плюс перф и смоки). Находок нет. + +--- + + + +## Материал раунда + +- Ветка: `issue/694-floor-switch-cost`, коммит `35c89fc67dc8` — ребейз его осиротит, и это нормально: ниже якоря, которые ребейз не меняет. +- Дерево материала: `0df6abb0a936a66d2e00d73acdc28bf53363a50c` + ``` + git log --all --format='%H %T' | grep 0df6abb0a936 + ``` +- Тело issue: `02419f4111e3d31c9b6850dd135fcd78c7d57892ba54c752a6088ccb070a5a05` +- Вердикт конвейера: `green` · High 0