From 5f0dfa84e1de6499f5b917d47fbf227a5eea2df4 Mon Sep 17 00:00:00 2001 From: "claude[bot]" <209825114+claude[bot]@users.noreply.github.com> Date: Fri, 11 Sep 2026 19:51:30 +0000 Subject: [PATCH] docs: review document for #534 Issue: #534 User-Visible: no --- docs/reviews/CODE-REVIEW-534-r1.md | 193 +++++++++++++++++++++++++++++ 1 file changed, 193 insertions(+) create mode 100644 docs/reviews/CODE-REVIEW-534-r1.md diff --git a/docs/reviews/CODE-REVIEW-534-r1.md b/docs/reviews/CODE-REVIEW-534-r1.md new file mode 100644 index 00000000..3a275c72 --- /dev/null +++ b/docs/reviews/CODE-REVIEW-534-r1.md @@ -0,0 +1,193 @@ +# CODE-REVIEW-534-r1 + +Issue: [#534](https://github.com/Matysh/houseplan-card/issues/534) · Материал: `5afdb644c78d60cdffa8f67a465b10af0d769c1e` (единственный коммит поверх `origin/dev`) · Заход r1 (первый код-ревью, ТЗ прошло r1→r3, зелёный вердикт на r3) · блокирующих циклов израсходовано 0/4 + +## Скоуп + +Регресс `longTask.countP95` в линейке v1.74.0 (два профиля Full Performance: +`large-house`, `plan-snap`), бисектом сведённый к #525: списки маркеров +устройств и проёмов перевели с `map()` на `repeat(items, (o) => o.id, …)`, +чтобы Lit не переиспользовал узлы по позиции при смене пространства. На плане +с двумястами маркерами `repeat` на смене пространства честно строит две карты +непересекающихся ключей, чтобы всё равно выбросить всё и создать заново — 80 мс +на цикл, 6–13 длинных задач. + +Правка: оба списка обёрнуты в `keyed(space.id, repeat(items, (item) => item.id, +…))`. Внешний `keyed` выбрасывает поддерево целиком на смене пространства +(дорогой диф не выполняется вовсе), внутренний `repeat` остаётся и защищает от +позиционного переиспользования **внутри** пространства — состав обоих списков +там тоже едет: у маркеров от призраков редактора устройств и живого синка +конфига, у проёмов от orphan-записи, которая существует только в режиме Plan. + +ТЗ прошло три раунда ревью ТЗ (r1/r2 — жёлтый, обе Medium-находки о том, что +допущение о безопасности голого `map()` внутри пространства неверно — сначала +для маркеров, затем для проёмов; r3 — зелёный). Код-ревью разбирает +реализацию против финального текста ТЗ (тела issue) целиком, это первый заход +код-ревью. + +## Как проверялось + +Дешёвые гейты подтверждены зелёным Validate на этом SHA +([прогон 34639320867](https://github.com/Matysh/houseplan-card/actions/runs/34639320867), +`workflow_dispatch`, событие проверено через `gh run view --json jobs`): +job «Фронтенд: типы, юниты, мутанты, синхрон бандла» — success, job +«Предполётные проверки» (docs/provenance/process-gate) — success, шесть +шардов «Мутанты по диффу» — success. `smoke`/`golden`/`backend`/ +`performance_smoke` в этом прогоне **skipped** — классификатор не счёл его +heavy (обычный push, не кандидат и не `full=true`), поэтому эти гейты я +прогнал сам. + +| Гейт | Команда | Результат | +|---|---|---| +| Пересборка бандла | `npm run build && npm run bundle:sync` | пересобранный бандл побайтово совпал с закоммиченным (`git status` пуст после пересборки) — сверх доверия к Validate | +| `check-docs` (`src/**` тронут) | `node scripts/check-docs.mjs` | `Documentation checks passed (7 files, 12 external links)` | +| Выбор смоков по диффу | `node scripts/smoke-select.mjs --base origin/dev --head HEAD` | 8 прямых совпадений (все на `_openingsR`) + 1 зарегистрированная связь (`smoke_space_switch_transitions.mjs` ← `_renderDevice`); широких/неопределённых связей нет | +| Смок AC1/AC2/AC2а | `node demo/smoke_space_switch_transitions.mjs` | зелёный, все поля `true`, `unexpectedAnimations: []`, `swappedMarkerNodes: []`, `swappedOpeningNodes: []` | +| 8 прямых смоков | `node demo/smoke_{glow,isometric_live_touch,open_passage,opening_binding,opening_entity_search,opening_preview,partition_openings,registryless_opening}.mjs` | все 8 зелёные | +| Мутант AC2а (маркеры) | `node scripts/mutation-gate.mjs --id=device-markers-rendered-without-keys` | «покраснел, как обязан», 1 из 1 | +| Мутант AC2а (проёмы) | `node scripts/mutation-gate.mjs --id=openings-rendered-without-keys` | «покраснел, как обязан», 1 из 1 | +| Свидетель отклонения (см. находку/не-находку ниже) | ручная мутация: снят внешний `keyed(space.id, …)` в обоих местах, пересборка, `node demo/smoke_space_switch_transitions.mjs` | смок остаётся зелёным — подтверждает заявление автора, что внешний ключ не ловится ни одним функциональным свидетелем; дерево возвращено к исходному состоянию, `git status` чист | +| AC6 (golden, диф трогает рендер) | `npm run golden:verify` | код возврата 0, все просмотренные сценарии `passed`, включая `day-cycle-*-dark` — расхождение, которое автор описал как окружение песочницы, в этой песочнице не воспроизвелось | +| AC3/AC4 (перф) | не прогонял | см. «Чего не проверял» | +| `npm test`, `npx tsc --noEmit`, `python -m pytest tests_backend` | не прогонял отдельно | покрыты зелёным Validate на этом SHA; бэкенд не тронут (diff не касается `custom_components/**/*.py`) | +| Инварианты модели | не прогонял | diff не трогает геометрию/`layout`/`marker.space`/`open_spans` — только директиву рендера поверх тех же данных | + +## Находки + +Нет находок High или Medium. + +**Одно отклонение от буквы ТЗ, заявленное автором открыто и проверенное мной +как корректное, не находка.** Хендофф-комментарий сообщает: мутанты на внешний +`keyed(space.id, …)` (`device-layer-loses-its-space-key`, +`opening-layer-loses-its-space-key`) написаны, прогнаны и не ловятся ни одним +тестом («0 из 1»), поэтому в реестр не добавлены. Я воспроизвёл это +независимо (см. таблицу выше): сняв внешний `keyed` в обоих местах и +пересобрав бандл, `smoke_space_switch_transitions.mjs` остаётся полностью +зелёным — идентичность узлов после смены пространства одинакова что с +внешним ключом, что без него, потому что идентификаторы маркеров и проёмов +между пространствами и так не пересекаются (это гарантия #525, не этой +задачи). Финальный текст AC5 в теле issue формулирует свидетеля внешнего +ключа не как обязательный мутант, а как «красит свидетеля AC2 **либо** +возвращает замер AC3 к 936 мс» — то есть ТЗ само называет перф-замер законной +альтернативой мутационному гейту для этой конкретной защиты, и именно замер +(бисект автора: 936,0 на `main` → 903,7/905,9 с `keyed`) и есть предъявленное +доказательство. Решение принимаю: реестр мутантов не нуждается в +заведомо неубиваемом мутанте — держать его там было бы хуже, чем не держать +(врёт о покрытии), а сам факт «не ловится ничем, кроме числа» проверен, а не +предположен. + +Low, не блокирует, правки не требует: в `docs/CHANGELOG.md`/`.ru.md` +экономия описана как «about a tenth of a second» / «около десятой доли +секунды», хотя измеренная разница — около 80 мс (889,1 против 936,0 на +`main`). Это качественное округление в пользовательском тексте, а не число с +претензией на точность рядом с другим таким же числом (правило «одно +число — один источник» здесь не задето: в UI это значение нигде не +показывается вторично), так что не завожу как отдельную правку. + +## Что проверено и корректно + +- **К1/К2 (форма).** `src/houseplan-card.ts`: оба места — список маркеров + (было `repeat(devs, …)`) и список проёмов (было `repeat(items, …)`) — теперь + `keyed(space.id, repeat(…, (x) => x.id, …))`. Один способ на оба списка, без + исключений, как требует К2. +- **К1 (узел не переживает смену пространства).** Подтверждено исполнением: + `smoke_space_switch_transitions.mjs`, разделы 1–2, зелёные без изменений в + самом тесте (AC1 не требовал правок и остался таким). +- **К3 (пер-элементный ключ обязателен внутри пространства, для обоих + списков).** Подтверждено и чтением, и исполнением: + - `src/styles/devices.styles.ts:213` — `.device-shell-frame` действительно + хранит `transition: border-color .15s, opacity .2s` (правильно: #524 снял + только `box-shadow`, что и было находкой r1 ревью ТЗ); + - `src/houseplan-card.ts` — `_openingsR` отдаёт запись с нерешённым хостом + только когда `this._mode === 'plan'`, то есть переключение режима внутри + пространства меняет состав списка проёмов (находка r2 ревью ТЗ, принятая и + закрытая в r3); + - оба триггера воспроизведены нагрузочно новым разделом 3 смока (AC2а): + вставка устройства в начало списка и переключение Plan↔View с + orphan-записью — оба сценария не переставляют существующие узлы и не + запускают анимаций. +- **Комментарий-ловушка на месте у обеих подсистем.** `plan.styles.ts:539-554` + объясняет обе половины формы и оба триггера; `devices.styles.ts:211-213` + ссылается на него рядом с живыми переходами — ровно как требует К3 + («объясняется комментарием там же, где стоят анимации»). +- **AC5, пер-элементная половина.** Оба существующих мутанта + (`device-markers-rendered-without-keys`, `openings-rendered-without-keys`) + переанкерены под новую форму (`scripts/mutation-gate.mjs:7434-7460`) и оба + красные при снятии внутреннего ключа — проверено запуском, не заявлением. +- **AC7.** Оба чейнджлога отредактированы в том же коммите, что и поведение; + трейлеры `Issue: #534` и `User-Visible: yes` на месте; ветка + `issue/534-space-switch-keyed-layer`. Комментарий К3 стоит рядом с + анимациями (см. выше) — отдельного требования из АС7 выполнено. +- **Бюджет строк.** `test/core-file-budget.test.mjs` поднимает потолок + `src/houseplan-card.ts` на 1 (13650→13651) с датированной запиской + «`import { keyed }`, переносить нечего» — соответствует диффу: единственная + добавленная непустая строка кода вне самого рендера — это импорт. +- **Бандл.** `docs/images/screenshots.json` меняет только + `sourceFingerprint`/`sourceSha256` (пиксели те же, `imageSha256` не + тронуты) — путь `docs:accept -- --identical`, как и предписывает §8 для + правок без изменения кадра. +- **Не найдено смежных регрессий.** 8 смоков, прямо завязанных на + `_openingsR` по выбору `smoke-select.mjs`, зелёные без исключений; `npm run + golden:verify` — 0 расхождений по всей матрице в этой песочнице. +- **Скоуп.** Правка восстанавливает перф, которая была до #525, ничего не + добавляет и не убирает в контракте #525 (дверь, которой не было, по-прежнему + не анимируется) — новых экранов, полей или UX-контрактов нет; под + `docs/SCOPE.md` подпадает как поддержание существующей отзывчивости плана + (J1/J6), а не новая функциональность, так что вопрос «какую строку Core user + jobs это закрывает» решается тривиально в пользу уже закрытых. + +## Чего не проверял + +- **AC3 (`npm run benchmark:large-house`) и AC4 (Full Performance, 9 + профилей).** Не прогонял: перф-числа в этой песочнице не воспроизводят + условия раннера (другое железо, шум), а само ТЗ называет их «ориентиром» + и явно передаёт «окончательный приговор» гейту Full Performance на + релизном раннере — это пре-релизный гейт по §8 PROCESS.md, не гейт + ревью, и для этой линейки задачи он и есть источник истины (сама задача + #534 началась с его красного прогона). Числа автора (`switchCycleMs` + 889,1 против 936,0 на `main` и 871,2 до #525, «2–3» длинных задачи вместо + «7–13») внутренне согласованы с ходом бисекта в аналитике и не + противоречат ничему, что я проверил чтением или исполнением. +- **`python -m pytest tests_backend`.** Diff не касается + `custom_components/**/*.py` — гейт неприменим. +- **`node scripts/model-invariants.mjs`.** Diff не трогает геометрию, `layout`, + `marker.space` или `open_spans` — только директиву рендера поверх тех же + данных, гейт неприменим. +- **Полная матрица `demo/smoke_*.mjs` (244 файла).** Прогнаны только + выбранные диффом (8 прямых + 1 зарегистрированная связь) — задача не задевает + всё дерево смоков, полный прогон это пре-релизная обязанность. +- **Мутанты на внешний `keyed(space.id, …)`.** Не добавлял в реестр — см. + раздел «Находки»: воспроизвёл заявление автора, что такой мутант не + ловится ничем, кроме перф-замера, и посчитал реестр без него правильным + решением, а не пробелом. + +## Материал раунда + +- SHA материала: `5afdb644c78d60cdffa8f67a465b10af0d769c1e` (единственный + коммит `issue/534-space-switch-keyed-layer` поверх `origin/dev`). +- Диапазон: `git diff origin/dev...HEAD` (55 файлов: 3 файла класса A/B по + содержанию — `src/houseplan-card.ts`, `src/styles/plan.styles.ts`, + `src/styles/devices.styles.ts` — плюс `scripts/mutation-gate.mjs`, + `demo/smoke_space_switch_transitions.mjs`, `test/core-file-budget.test.mjs`, + оба чейнджлога, `docs/images/screenshots.json` и три копии бандла класса D). +- Рабочая копия после всех локальных прогонов и ручной проверки отклонения + возвращена в состояние коммита (`git status` пуст на момент публикации + вердикта). + +## Вердикт + +Вердикт: зелёный · заход r1 · блокирующих циклов 0/4 · High: 0 · Medium: 0 + +--- + + + +## Материал раунда + +- Ветка: `issue/534-space-switch-keyed-layer`, коммит `5afdb644c78d` — ребейз его осиротит, и это нормально: ниже якоря, которые ребейз не меняет. +- Дерево материала: `10993a92cf52d54a6fe21138a74f514f535f2bef` + ``` + git log --all --format='%H %T' | grep 10993a92cf52 + ``` +- Тело issue: `9d969d777f9787e5a50d02b2e675ad1448e945a27b47899d2d37c445a612f30e` +- Вердикт конвейера: `green` · High 0