mirror of
https://github.com/Matysh/houseplan-card
synced 2026-09-29 03:09:36 +00:00
claude[bot]
@@ -0,0 +1,221 @@
|
||||
# CODE-REVIEW-520-r1
|
||||
|
||||
Issue: #520 · Этап: code · Заход: r1 · Материал: `a625871538c2cf58c39efd2b0e52be912f2e04e4`
|
||||
Вердикт: **красный** · High: 1 · Medium: 0
|
||||
|
||||
## Скоуп
|
||||
|
||||
ТЗ (тело issue #520, зафиксировано зелёным на SPEC-REVIEW r3) требует убрать
|
||||
лишнюю эпоху конфигурации на старте, которую внёс #500: `_serverCfg` и
|
||||
`_layout` перестают быть объявленными реактивными свойствами Lit, реактивность
|
||||
сохраняется через `_adoption → onBodyReplaced → requestUpdate`. Диапазон
|
||||
коммитов: `37ef970d`, `3d439805`, `27ea23c1` (диагностика бенчмарка),
|
||||
`e181b08f` (сама правка + тесты + мутанты + бюджеты), `a6258715` (второй
|
||||
мутант). Диф продуктового кода: 19 строк в `src/houseplan-card.ts` — ровно
|
||||
контракт §1 ТЗ, без сюрпризов.
|
||||
|
||||
## Как проверялось
|
||||
|
||||
Прочитан диф всех файлов диапазона (`git diff origin/dev...HEAD`), тело issue
|
||||
#520 и вся история комментариев (S2-анализ → ТЗ r1–r3 → хендофф на код-ревью).
|
||||
Дешёвые гейты подтверждены зелёным Validate на этом SHA
|
||||
(https://github.com/Matysh/houseplan-card/actions/runs/34499852060), но этот
|
||||
прогон — обычный пуш в ветку задачи (`heavy=false` в `classify-changes.mjs`),
|
||||
поэтому `performance_smoke`, `golden` и `smoke` в нём **skipped**, не success —
|
||||
они не подтверждают ничего сверх typecheck/юнитов/сборки/мутантов по диффу.
|
||||
`Полные бенчмарки производительности` (`performance.yml`) на этом SHA не
|
||||
запускались вовсе: последний прогон был на `27ea23c1` (диагностика,
|
||||
до правки) и он красный, как и ожидалось для дорегрессионного кода.
|
||||
|
||||
AC4/AC5 этой задачи — это именно то, что дорогой Full Performance гейт
|
||||
проверяет, а автор прямо пометил их «ждёт прогона» (пост-мерж CI). Это ровно
|
||||
случай из инструкции ревьюера: «любой гейт, который требуют AC задачи»
|
||||
Validate не покрывает и я отвечаю за него сам. В песочнице обнаружился
|
||||
установленный Chromium (`~/.cache/ms-playwright` уже заполнен), поэтому я
|
||||
прогнал полное сравнение сам, а не поверил записи «ждёт прогона».
|
||||
|
||||
### Прогон Full Performance профиля `interaction` (ядро AC3–AC5)
|
||||
|
||||
Воспроизвёл процедуру `performance.yml`: `git worktree add /tmp/hpc-baseline
|
||||
a44fbd37` (база, зафиксированная в ТЗ), `npm ci && npm run build &&
|
||||
node scripts/bundle-sync.mjs` в обоих деревьях, затем
|
||||
`npm run benchmark:large-house-interaction -- --target-root=<base|.>
|
||||
--samples=7 --warmups=1` и `npm run benchmark:compare -- --budgets=demo/performance/budgets-large-house-interaction.json`.
|
||||
|
||||
**База `a44fbd37` (7 образцов):** 18 циклов обновления, 3 сборки модели, 3
|
||||
эпохи конфигурации, `epochs=[0->1@_saveConfig<_syncNewDevices ;
|
||||
1->2@_saveConfig<_seedHiddenDevices ; 2->3@willUpdate<performUpdate]`,
|
||||
`firstStableRenderMs` медиана ≈2814–2828 — совпадает с числами, которые автор
|
||||
привёл в issue для этой базы (2792.7, диапазон те же порядки).
|
||||
|
||||
**Кандидат `a6258715` (7 образцов, HEAD этого ревью, правка применена, дист
|
||||
пересобран и сверен построчно — `_serverCfg`/`_layout` подтверждённо
|
||||
отсутствуют в `nv.properties=` минифицированного бандла, только геттеры/
|
||||
сеттеры):**
|
||||
|
||||
```
|
||||
updates=19 models=4 cfgEpoch=4
|
||||
epochs=[0->1@willUpdate<performUpdate ← ТА ЖЕ лишняя, первая
|
||||
1->2@_saveConfig<_syncNewDevices
|
||||
2->3@_saveConfig<_seedHiddenDevices
|
||||
3->4@willUpdate<performUpdate]
|
||||
firstStableRenderMs медиана ≈3320–3496 (7/7 образцов)
|
||||
```
|
||||
|
||||
Это **дословно** та же трасса, которую issue цитирует для ДОРЕГРЕССИОННОГО
|
||||
(добуквенно багованного) кандидата на `27ea23c1`, до правки. Правка не
|
||||
изменила ни число эпох, ни число сборок модели, ни порядок событий.
|
||||
|
||||
`npm run benchmark:compare` на официальных бюджетах
|
||||
(`demo/performance/budgets-large-house-interaction.json`) на этих семи
|
||||
образцах даёт:
|
||||
|
||||
```
|
||||
❌ timing.modelReadyMs.median 1912.3 лимит 1622.01 база 1247.7
|
||||
❌ timing.firstStableRenderMs.median 3334 лимит 3000 база 2814.1
|
||||
❌ longTask.maxSingleMs 1729 лимит 1492.4 база 1148
|
||||
❌ cache.entries.cleanFloor 120 лимит 100 —
|
||||
Performance budget failed: timing.modelReadyMs.median,
|
||||
timing.firstStableRenderMs.median, longTask.maxSingleMs,
|
||||
cache.entries.cleanFloor
|
||||
```
|
||||
|
||||
Дополнительно проверен профиль изометрии (1 образец, для скорости):
|
||||
`updates=19 models=4 cfgEpoch=4`, та же лишняя эпоха первой, `modelReadyMs`
|
||||
2617–2824 против относительного лимита 2598.72 (тот же расчёт, что в ТЗ) —
|
||||
тоже красный.
|
||||
|
||||
### Что это означает по AC
|
||||
|
||||
| AC | Требование | Измерено на `a6258715` | Статус |
|
||||
|---|---|---|---|
|
||||
| AC1 | тела не объявлены в `static properties` | подтверждено чтением бандла и тестом `config-adoption-ownership.test.mjs` | ✅ выполнено |
|
||||
| AC2 | замена тела будит `onBodyReplaced` | мутант `adoption-notifies-no-host-on-config-replacement` пойман 1/1 | ✅ выполнено |
|
||||
| AC3 (запись) | 3 эпохи и 3 сборки на старте, как до #500 | **4 эпохи, 4 сборки** — не изменилось относительно бага | ❌ не выполнено |
|
||||
| AC4 | `firstStableRenderMs` ≤3000 и обе метрики старта внутри относительного лимита на обоих профилях | `firstStableRenderMs` 3334 > 3000 (абсолютный потолок нарушен), `modelReadyMs` 1912.3 > 1622.01 (относительный лимит нарушен); то же на изометрии | ❌ не выполнено |
|
||||
| AC5 | `cache.entries.cleanFloor` ≤100 | 120 (лимит уже возвращён на 100 правкой — job красный ровно как описывает сам AC5 «чем краснеет») | ❌ не выполнено |
|
||||
|
||||
**AC1 и AC2 доказывают только синтаксический контракт** («свойство не
|
||||
объявлено» и «вызов есть»), но не поведенческий эффект, ради которого вся
|
||||
задача затевалась. Диагностика, которую сам автор оставил в дереве (`bootDiag`),
|
||||
и есть инструмент, который это ловит — но её вывод никто не прочитал на
|
||||
кандидате после правки: PR-комментарий явно фиксирует AC3–AC5 как «ждёт
|
||||
прогона», а сам прогон (Full Performance) на этом SHA не выполнялся.
|
||||
|
||||
### Находка 1 (High) — правка не устраняет регрессию, которую должна была устранить
|
||||
|
||||
Убрав `_serverCfg`/`_layout` из `static properties`, эпоха конфигурации на
|
||||
старте большого дома **не вернулась** к 3 (как требует AC3) — она осталась
|
||||
равна 4, с той же лишней эпохой на первом `willUpdate`, что и до правки.
|
||||
`firstStableRenderMs` не вернулся в бюджет (3320–3496 мс против потолка 3000
|
||||
и базы ≈2814–2828), `cache.entries.cleanFloor` остаётся 120 против
|
||||
восстановленного потолка 100. Проверено дважды (7 образцов на профиле
|
||||
взаимодействия + отдельно на изометрии), числа воспроизводимы и совпадают
|
||||
между прогонами.
|
||||
|
||||
Судя по диагностике (`willUpdate` бампает эпоху, когда
|
||||
`changed.has('_serverCfg')` истинно, а `this._cfgEpochPreservedConfig` не
|
||||
совпадает с текущим `_serverCfg`), первичная загрузка конфигурации идёт через
|
||||
`adoptStructuralResponses` → `MutableConfigAdoption.setConfig`, которая
|
||||
уведомляет `onBodyReplaced` напрямую, минуя `_setAreaLifecycleConfig` (тот
|
||||
единственный путь, что заранее выставляет `_cfgEpochPreservedConfig` в то же
|
||||
значение и гасит бамп). Это не Lit-специфичный механизм `wrapped`, который
|
||||
диагностировало ТЗ, — а исходное состояние `_cfgEpochPreservedConfig`,
|
||||
которое остаётся рассинхронизировано с первым реальным приходом конфига
|
||||
независимо от того, объявлено ли свойство в Lit. Это наблюдение, не
|
||||
диагноз «что чинить» — корень требует отдельного расследования автора,
|
||||
не входит в мой мандат.
|
||||
|
||||
Это блокирующая находка: ядро задачи (перф-регрессия #520) остаётся
|
||||
неисправленным, при том что диф прошёл оба узких мутанта и весь остальной
|
||||
набор дешёвых гейтов. Без повторного изменения продуктового кода (не только
|
||||
диагностики бенчмарка) AC3/AC4/AC5 не могут стать зелёными.
|
||||
|
||||
## Что проверено и корректно
|
||||
|
||||
- Диф `src/houseplan-card.ts` (19 строк) — ровно контракт §1 ТЗ: две строки
|
||||
объявления убраны, добавлен комментарий-ловушка над `static properties`
|
||||
(§5 ТЗ). Постороннего кода нет.
|
||||
- AC1: `test/config-adoption-ownership.test.mjs` — прочитан, тест умеет
|
||||
падать (мутант `adoption-bodies-declared-reactive`, прогнан локально:
|
||||
«поймано 1 из 1»).
|
||||
- AC2: мутант `adoption-notifies-no-host-on-config-replacement`, прогнан
|
||||
локально: «поймано 1 из 1».
|
||||
- `npx tsc --noEmit` — чисто.
|
||||
- `npm test` — 2509 pass / 0 fail / 1 skipped (2510 тестов), совпадает с
|
||||
заявлением автора.
|
||||
- `npm run build` — чисто, бандл собирается.
|
||||
- `node scripts/mutation-gate.mjs --check` — реестр свидетелей цел.
|
||||
- Бюджеты `budgets-interaction-smoke.json` и `budgets-large-house-interaction.json`:
|
||||
`cleanFloor` возвращён на 100 в обоих файлах, как того требует AC5 (сама
|
||||
цифра в бюджете корректна — красный результат означает, что КОД не
|
||||
укладывается в правильно выставленный бюджет, а не что бюджет неверен).
|
||||
`budgets-large-house-isometric.json` не трогался — там потолок уже был 100,
|
||||
запись в ТЗ (r2→r3) подтверждена.
|
||||
- Гигиена ветки: `.gitignore` (`node_modules` без слэша) и удаление
|
||||
симлинка — проверено, в `git diff --stat` `node_modules` не фигурирует.
|
||||
- `demo/performance/card-contract.mjs`, `test/performance-contract.test.mjs` —
|
||||
добавленные поля (`_buildModel`, `_cfgEpoch`, `_adoptAuthoritative`) реальны
|
||||
в проде, тест это перепроверяет по компилированному бандлу.
|
||||
- Трейлеры коммитов — `Issue: #520` присутствует на всех коммитах диапазона.
|
||||
`User-Visible: no` — корректно: контракт поведения снаружи не меняется
|
||||
(просто ещё не действует так, как задумано).
|
||||
|
||||
## Чего не проверял
|
||||
|
||||
- **HA-харнесс** (`tests_backend/test_ha_*.py`) — правка не касается
|
||||
`custom_components/**`, diff не затрагивает бэкенд.
|
||||
- **Golden-кадры** — diff не меняет рендер/геометрию/стили, только
|
||||
реактивность двух приватных полей и бюджеты в JSON; рендер-контракт не
|
||||
затронут.
|
||||
- **Инварианты модели** (`npm run invariants`) — diff не трогает рёбра
|
||||
комнат, `layout`, `marker.space`, `open_spans`; геометрия не переименована,
|
||||
только владение реактивностью. Не запускал.
|
||||
- **Полный набор смоков** — `node scripts/smoke-select.mjs` не запускал
|
||||
явно (инструмент недоступен без указания base/head в этой сессии review
|
||||
script), но раз ядро задачи уже красное на перф-гейте, останавливаться на
|
||||
находке High и возвращать в задачу — по процессу верное действие;
|
||||
добивать остальные тяжёлые гейты до починки бессмысленно.
|
||||
- **`process-gate.mjs` на диапазоне** — принял на слово прогон автора
|
||||
(Validate `success` на этом SHA включает эквивалентный шаг в preflight).
|
||||
- Прогонял всё внутри своей рабочей копии/временного `git worktree`
|
||||
(`/tmp/hpc-baseline`, удалён после использования) — репозиторий ревью не
|
||||
тронут, `git status` чист.
|
||||
|
||||
## Гейты: что прогнал сам и почему
|
||||
|
||||
| Гейт | Прогнан | Результат |
|
||||
|---|---|---|
|
||||
| `npx tsc --noEmit` | да (дёшево, доп. проверка) | чисто |
|
||||
| `npm test` | да | 2509/2510 pass, 1 skip |
|
||||
| `npm run build` + bundle-sync | да | чисто, дист пересобран для обоих деревьев |
|
||||
| `node scripts/mutation-gate.mjs --check` + 2 мутанта задачи | да | реестр цел, оба мутанта пойманы 1/1 |
|
||||
| Full Performance `interaction` (7 образцов, база `a44fbd37` vs кандидат) | да, вручную (Chromium уже установлен в песочнице) | **budget failed** — см. находку |
|
||||
| Full Performance `isometric` (1 образец, для скорости) | да | тот же паттерн лишней эпохи, `modelReadyMs` за относительным лимитом |
|
||||
| golden, HA pytest, полный smoke-набор, invariants | нет | вне диффа/уже нерелевантно после High-находки |
|
||||
|
||||
## Вердикт
|
||||
|
||||
Красный. Находка High блокирует: механизм задачи (лишняя эпоха конфигурации
|
||||
на старте большого дома) не устранён, измерено официальным инструментом
|
||||
проекта (`benchmark:large-house-interaction` + `benchmark:compare`) на
|
||||
зафиксированном материале ревью `a6258715`, воспроизводимо на 7 образцах и
|
||||
повторно на изометрии. AC1/AC2 (синтаксический контракт, проверен тестами и
|
||||
мутантами) выполнены; AC3/AC4/AC5 (поведенческий/перф эффект, ради которого
|
||||
затевалась вся задача) — нет. Возврат автору на доработку продуктового кода
|
||||
(не диагностики бенчмарка), с новым измерением на исправленном кандидате до
|
||||
следующего захода.
|
||||
|
||||
---
|
||||
|
||||
<!-- material-anchors: сгенерировано конвейером (#414) -->
|
||||
|
||||
## Материал раунда
|
||||
|
||||
- Ветка: `issue/520-first-frame`, коммит `a625871538c2` — ребейз его осиротит, и это нормально: ниже якоря, которые ребейз не меняет.
|
||||
- Дерево материала: `fbd3b83fa6559f4470124d1b45e4272b4eadcdec`
|
||||
```
|
||||
git log --all --format='%H %T' | grep fbd3b83fa655
|
||||
```
|
||||
- Тело issue: `60492a3aec5ff7fed54f7b32c040b7a118526c209609d736ee68e116eb8f0aa6`
|
||||
- Вердикт конвейера: `red` · High 1
|
||||
Reference in New Issue
Block a user