mirror of
https://github.com/Matysh/houseplan-card
synced 2026-10-01 04:09:17 +00:00
claude[bot]
@@ -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
|
||||
|
||||
---
|
||||
|
||||
<!-- material-anchors: сгенерировано конвейером (#414) -->
|
||||
|
||||
## Материал раунда
|
||||
|
||||
- Ветка: `issue/534-space-switch-keyed-layer`, коммит `5afdb644c78d` — ребейз его осиротит, и это нормально: ниже якоря, которые ребейз не меняет.
|
||||
- Дерево материала: `10993a92cf52d54a6fe21138a74f514f535f2bef`
|
||||
```
|
||||
git log --all --format='%H %T' | grep 10993a92cf52
|
||||
```
|
||||
- Тело issue: `9d969d777f9787e5a50d02b2e675ad1448e945a27b47899d2d37c445a612f30e`
|
||||
- Вердикт конвейера: `green` · High 0
|
||||
Reference in New Issue
Block a user