mirror of
https://github.com/Matysh/houseplan-card
synced 2026-09-29 03:09:36 +00:00
claude[bot]
@@ -0,0 +1,300 @@
|
||||
# Код-ревью #592 · заход r1
|
||||
|
||||
SHA материала: `61c74a70128a29871547519750bae32695f6e56a` (`b76f3e57` + фикс-коммит `61c74a70`).
|
||||
Ветка: `issue/592-extract-dialogs`. Заход r1 (первое фактическое ревью: предыдущая
|
||||
попытка на `b76f3e57` была отменена стражем до чтения кода — красный Validate
|
||||
на `no-new-any`, [прогон 35369760264](https://github.com/Matysh/houseplan-card/actions/runs/35369760264);
|
||||
цикл ревью не израсходован). Разбор — полный, дельты предыдущего раунда нет.
|
||||
|
||||
## Скоуп
|
||||
|
||||
Шаг 0 эпика #591: механический перенос разметки четырёх диалогов настроек
|
||||
(`_renderMarkerDialog`, `_renderSpaceDialog`, `_renderSettingsDialog`,
|
||||
`_renderRoomDialog`) из `src/houseplan-editor-runtime.ts` в четыре новых модуля
|
||||
`src/editors/*.ts`, без единого видимого изменения (К1). Плюс попутный
|
||||
фикс-коммит `61c74a70`, чинящий гейт `no-new-any`, который красил перенесённые
|
||||
(не новые) строки `any`.
|
||||
|
||||
## Как проверялось
|
||||
|
||||
### Побайтовая идентичность (К1, ядро доказательства AC1/AC2)
|
||||
|
||||
Извлёк тело каждого из четырёх методов из `origin/dev:src/houseplan-editor-runtime.ts`
|
||||
по номерам строк из аналитики issue и сравнил `diff` с телом соответствующей
|
||||
экспортируемой функции в новом модуле:
|
||||
|
||||
| Метод → модуль | Диапазон в `origin/dev` | Результат |
|
||||
|---|---|---|
|
||||
| `_renderMarkerDialog` → `marker-dialog.ts` | 12885–13609 (725 строк) | `diff` пуст |
|
||||
| `_renderSettingsDialog` → `general-settings-dialog.ts` | 10338–10494 (157 строк) | `diff` пуст |
|
||||
| `_renderSpaceDialog` → `space-settings-dialog.ts` | 13612–13906 (295 строк) | `diff` пуст |
|
||||
| `_renderRoomDialog` → `room-settings-dialog.ts` | 13985–14095 (111 строк) | `diff` пуст |
|
||||
|
||||
Все четыре тела совпадают с оригиналом посимвольно, включая отступы. Делегаты
|
||||
в классе (`return renderMarkerDialog.call(this);` и три аналогичных) сверены
|
||||
чтением — корректны, по одному на метод.
|
||||
|
||||
Проверил также вынесенные вместе с диалогами величины: `CELL_CM_MIN = 0.1`,
|
||||
`CELL_CM_MAX = 1000` в `space-settings-dialog.ts` совпадают со значениями,
|
||||
удалёнными из рантайма; `DISPLAY_LABEL_KEYS`/`DISPLAY_HINT_KEYS` переехали в
|
||||
`marker-dialog.ts` целиком и не остались дублем в рантайме (`grep` — ноль
|
||||
совпадений там). `_radarSetup` стал `public` — единственный новый потребитель
|
||||
вне класса (`marker-dialog.ts`) обращается к нему как к контроллеру, остальной
|
||||
класс и так живёт на `public _x` (проверено чтением, соглашение).
|
||||
|
||||
### Гейты — что прогнал и что унаследовал от Validate на этом SHA
|
||||
|
||||
`npx tsc --noEmit`, `npm test`, `npm run build` (со сверкой бандла) числятся
|
||||
зелёными на Validate `61c74a70` (ссылка в контексте задачи) — не перегонял их
|
||||
ради самого факта, но `npm test` и `npm run build` всё равно понадобились для
|
||||
последующих проверок ниже, так что фактически перепрогнаны:
|
||||
|
||||
| Гейт | Команда | Результат |
|
||||
|---|---|---|
|
||||
| Юниты | `npm test` | 2770 pass / 0 fail / 1 skip (2749 из них — субтесты, дерево `node --test`) |
|
||||
| Сборка | `npm run bundle:sync` (build + sync) | ok, дерево бандла синхронизировано |
|
||||
| Гейт `any` | `node scripts/no-new-any.mjs --base origin/dev --head HEAD` | «Новых any нет»; 1395 добавленных строк в 5 файлах, из них 1297 признаны перенесёнными |
|
||||
| Мутанты | `node scripts/mutation-gate.mjs --check` | все патчи (включая переехавший `same-binding-click-resets-source` и новый `no-new-any-counts-every-added-line-as-moved`) — `ok` |
|
||||
| Малый гейт | `node scripts/gate-small.mjs` | 69 смоков, 0 упавших, 3 «широких» символа — не гонялись автоматически (см. ниже) |
|
||||
| Документация | `node scripts/check-docs.mjs --external --screenshots=$(node scripts/classify-changes.mjs --screenshots-mode)` (режим на этом пуше — `warn`) | `WARN` про устаревший отпечаток скриншотов (ожидаемо: правится к бете, #479), `EXIT:0` |
|
||||
| Бюджет бандла | `npm run bundle:budget` | initial View 291356 B gzip (было 291346, +10 Б — шум), запас 9710 Б < 15000 Б — предупреждение, не новое: не связано с этим диффом (сам факт вынесенной разметки initial View не меняет) |
|
||||
| golden | `npm run golden:verify` (полная матрица, требование самого раннера) | см. ниже отдельно — AC2 |
|
||||
| single-source | `node --test test/single-source-numbers.test.mjs` | 3/3, не применимо к диффу (K1: видимых чисел не появилось) |
|
||||
|
||||
Не гонял: `npx tsc --noEmit`/`npm run build` как отдельно взятые проверки ради
|
||||
самого факта (уже зелёные на Validate этого SHA, а `npm run build` я и так
|
||||
выполнил как шаг перед golden/bundle:budget); `python -m pytest tests_backend`
|
||||
(диф не трогает `custom_components/**/*.py`); инварианты модели
|
||||
(`npm run invariants`) — диф не трогает геометрию, `layout`, `marker.space`,
|
||||
`open_spans`, скоуп задачи прямо это исключает; performance-профили — К1
|
||||
заявляет отсутствие видимых изменений и AC их не называет.
|
||||
|
||||
### AC2 — golden:verify
|
||||
|
||||
Полный прогон (`npm run golden:verify`, 172 сценария) на этом SHA:
|
||||
|
||||
- Все 15 эталонов, названных в AC2 (`device-dialog-desktop-{en,de}`,
|
||||
`device-dialog-mobile-ru`, `device-help-popover-light-ru`,
|
||||
`device-ripple-color-popover-mobile-ru`, `room-temperature-dialog-{desktop-en,mobile-ru}`,
|
||||
`toggle-entity-dialog-{desktop-en,mobile-ru}`, `general-color-popover-desktop-en`,
|
||||
`settings-help-zoom-200-{en-light,ru-dark}`, `space-room-color-popover-desktop-ru`) —
|
||||
`passed`.
|
||||
- Два сценария вне списка AC2 — `device-icon-state-table-light` и
|
||||
`device-icon-state-table-dark` — вышли `different`. Это сценарий `mode: 'view'`
|
||||
(#588), никак не связанный с диалогами настроек и не читающий переехавший
|
||||
код; `houseplan-card.ts` (где живёт View-рендер и собственная, независимая
|
||||
копия `DISPLAY_LABEL_KEYS`/`DISPLAY_HINT_KEYS`) в этом диффе не тронут вовсе.
|
||||
Чтобы отделить причину, прогнал ту же полную матрицу в отдельном worktree на
|
||||
чистом `origin/dev` (тот же Chromium песочницы, тот же индекс эталонов) —
|
||||
**те же два сценария различаются и там**. Это подтверждает вывод хендоффа
|
||||
(«Chromium 152 песочницы против 151 в индексе эталонов») — предсуществующий
|
||||
дрейф окружения, не регрессия этой задачи. AC2 в части, которую задача
|
||||
заявляет, выполнен: 0 расхождений по названным пятнадцати кадрам.
|
||||
|
||||
### Смоки
|
||||
|
||||
AC1 называет 10 смоков поимённо (плюс `smoke_value_face_source` в хендоффе) —
|
||||
все прогнаны напрямую (`node demo/smoke_*.mjs`), все `OK`:
|
||||
`smoke_general_settings`, `smoke_room_settings`, `smoke_device_preview_parity`,
|
||||
`smoke_color_picker_consumers`, `smoke_static_icon`, `smoke_device_inbox`,
|
||||
`smoke_room_temperature_thresholds`, `smoke_gear_tabs`, `smoke_esc_dialogs`,
|
||||
`smoke_dialog_footer_width`, `smoke_value_face_source`.
|
||||
`git diff --stat origin/dev...HEAD -- demo/` пуст — ни один файл смоков не
|
||||
тронут, соответствует заявлению AC1.
|
||||
|
||||
`node scripts/smoke-select.mjs --base origin/dev --head HEAD` напечатал 93
|
||||
«прямых совпадения» из 250 (широкими признаны только `_devices`, `_serverCfg`,
|
||||
`requestUpdate` — не признаны таковыми `_markerDialog`/`_spaceDialog`/
|
||||
`_settingsDialog`/`_config`, хотя они и попали в десятки смоков; так и должно
|
||||
быть: перенос тела метода diff'ом виден как удаление+добавление, поэтому
|
||||
каждый идентификатор внутри 1300 строк отмечается «изменённым», а не только
|
||||
разметка диалога). Прогонять все 93 — не соразмерно риску: тела уже доказаны
|
||||
побайтово идентичными, поэтому поведение любого метода, вызываемого изнутри
|
||||
диалога, не изменилось ни на символ. Прогнал выборочно 18 смоков, покрывающих
|
||||
все четыре диалога сверх списка AC1 (наибольший риск — 760-строчный диалог
|
||||
устройства): `smoke_ha_controls`, `smoke_binding_picker`, `smoke_radar_setup`,
|
||||
`smoke_controls`, `smoke_cover_not_primary`, `smoke_toggle_entity`,
|
||||
`smoke_value_face_source`, `smoke_hidden_flag`, `smoke_subarea`,
|
||||
`smoke_dialog_zombie` (устройство); `smoke_room_autoclose`, `smoke_font_scales`,
|
||||
`smoke_merge_split` (комната); `smoke_bg_color`,
|
||||
`smoke_space_create_display_defaults`, `smoke_backdrop_guard` (пространство/
|
||||
общие настройки); `smoke_post_write_adoption`, `smoke_danger_confirmation`
|
||||
(общие). Все 18 — `OK`. Оставшиеся ~75 не гонял: все они матчатся на те же
|
||||
широко используемые имена полей хоста, а не на код, специфичный для
|
||||
конкретного диалога.
|
||||
|
||||
### AC3 — потолок
|
||||
|
||||
`test/core-file-budget.test.mjs`: потолок `houseplan-editor-runtime.ts` опущен
|
||||
14100 → 12810, факт — 12807 строк (мера ratchet, `split('\n').length`), запас
|
||||
3 строки — совпадает с заявленным. Прогнан в составе `npm test`.
|
||||
|
||||
### AC4 — состояние не переехало
|
||||
|
||||
`test/editor-dialog-modules.test.mjs` — прочитан и проверен мутацией: добавил
|
||||
`const _spaceDialog = null;` в `marker-dialog.ts` и перезапустил тест — тест
|
||||
покраснел (`fail 1`), откатил правку (`git checkout -- src/editors/marker-dialog.ts`,
|
||||
`git status` после отката чист). Тест умеет падать. Сам тест по чтению корректен:
|
||||
проверяет отсутствие `@state(`, `class`, и объявлений каждого из четырёх
|
||||
черновиков в каждом из четырёх модулей, плюс что рантайм импортирует все
|
||||
четыре модуля, а карточка (`houseplan-card.ts`) — ни один (`grep` — ноль
|
||||
совпадений, K3 подтверждён).
|
||||
|
||||
### AC5 — ленивый граф
|
||||
|
||||
`npm run bundle:budget`: initial View 291356 B gzip, было 291346 — разница
|
||||
+10 Б, в пределах шума. `test/bundle-assets.test.mjs` прогнан в составе полного
|
||||
теста — зелёный.
|
||||
|
||||
### AC6 — якоря мутантов
|
||||
|
||||
`node scripts/mutation-gate.mjs --check` — весь реестр `ok`, включая
|
||||
переехавший патч `same-binding-click-resets-source` (единственный из 33,
|
||||
текст якоря `if (c.value === d.binding) { … }` уникален в дереве — проверено
|
||||
`grep`) и новый мутант `no-new-any-counts-every-added-line-as-moved`. Автор
|
||||
назвал переехавший патч поимённо в хендоффе — совпадает с тем, что нашёл я.
|
||||
|
||||
### AC7 — полный набор
|
||||
|
||||
`npm test` — 2770 pass / 0 fail / 1 skip. `node scripts/gate-small.mjs` —
|
||||
69 смоков, 0 упавших.
|
||||
|
||||
## Находки
|
||||
|
||||
### Medium (в скоупе, правится в этой же задаче) — «перенесённая» строка в гейте `no-new-any` матчится по всему диффу, а не по паре файлов реального переноса
|
||||
|
||||
`scripts/no-new-any.mjs` (фикс-коммит `61c74a70`) считает добавленную строку
|
||||
«перенесённой» и не судит её, если где-то **в любом файле того же диффа**
|
||||
нашлась дословно совпадающая удалённая строка (`movedLinesByFile`, бюджет —
|
||||
глобальная `Map<text, count>` без привязки к конкретной паре файлов
|
||||
источник→приёмник). Для механического переноса тела функции это работает
|
||||
верно. Но гейт — не разовый скрипт для этой задачи, а постоянная часть
|
||||
`src/**`-инфраструктуры (#342), и в этом виде он остаётся навсегда.
|
||||
|
||||
Воспроизвёл конкретный сценарий обхода: два **не связанных** файла в одном
|
||||
диффе — один удаляет произвольную строку с `any` в рамках несвязанной
|
||||
уборки, другой (совершенно новый файл, не перенос) добавляет **новую**
|
||||
строку `any`, которая просто текстуально совпадает с удалённой:
|
||||
|
||||
```js
|
||||
import { movedLinesByFile, findNewAnyViolations } from './scripts/no-new-any.mjs';
|
||||
const diff = [
|
||||
'diff --git a/src/unrelated-cleanup.ts b/src/unrelated-cleanup.ts',
|
||||
'--- a/src/unrelated-cleanup.ts', '+++ b/src/unrelated-cleanup.ts',
|
||||
'@@ -5,1 +5,0 @@', '- const cast = v as any;',
|
||||
'diff --git a/src/brand-new-feature.ts b/src/brand-new-feature.ts',
|
||||
'--- /dev/null', '+++ b/src/brand-new-feature.ts',
|
||||
'@@ -0,0 +1,1 @@', '+ const cast = v as any;',
|
||||
].join('\n');
|
||||
const moved = movedLinesByFile(diff); // -> Map { 'src/brand-new-feature.ts' => Set{1} }
|
||||
findNewAnyViolations({ files: [{
|
||||
path: 'src/brand-new-feature.ts', text: ' const cast = v as any;\n',
|
||||
addedLines: new Set([1]), movedLines: moved.get('src/brand-new-feature.ts'),
|
||||
}] }); // -> [] (должно быть 1 находка: это НЕ перенос, это новый any)
|
||||
```
|
||||
|
||||
Прогнал этот скрипт против рабочего дерева на `61c74a70` — воспроизводится:
|
||||
`moved` содержит строку 1 `brand-new-feature.ts`, `findNewAnyViolations`
|
||||
возвращает пустой список вместо находки. Гейт молча пропускает новый долг.
|
||||
|
||||
Это не гипотетическая экзотика: в этом же коммите естественным образом
|
||||
возникла пара строк с идентичным текстом
|
||||
(`<span>${this.host._t(k as any)}</span>` — и в `room-settings-dialog.ts:66`,
|
||||
и в `space-settings-dialog.ts:240`), то есть короткие типовые паттерны с `any`
|
||||
(`v as any`, `(e: any) => …`, `k as any`) в этой кодовой базе (887 явных `any`
|
||||
в `src/**`) повторяются часто. Любой будущий коммит, где новая строка `any`
|
||||
текстуально совпадёт с какой-нибудь удалённой строкой в другом файле того же
|
||||
диффа (например, обычная уборка мёртвого кода рядом), пройдёт `no-new-any`
|
||||
незамеченной — то есть ровно то, ради недопущения чего гейт #342 существует.
|
||||
|
||||
Добавленный мутант `no-new-any-counts-every-added-line-as-moved` эту дыру не
|
||||
ловит: он проверяет другую, более узкую поломку («любая добавленная строка
|
||||
считается перенесённой»), а не то, что сопоставление ведётся без привязки к
|
||||
файлу источника.
|
||||
|
||||
**Чем красит:** сценарий выше (можно оформить как юнит-тест в
|
||||
`test/no-new-any.test.mjs`, аналогично уже имеющимся трём тестам на
|
||||
`movedLinesByFile`/`findNewAnyViolations`).
|
||||
**Возможное направление фикса:** сузить сопоставление так, чтобы «перенос»
|
||||
засчитывался только для пар файлов, где такой перенос ожидаем (например, явный
|
||||
список путей текущего диффа, участвующих в декларируемом переносе, — как это
|
||||
уже делает `test/houseplan-source.mjs` через `DIALOG_MODULES`), либо требовать
|
||||
контекст в несколько строк вокруг совпадения вместо одной строки, чтобы
|
||||
случайное текстовое совпадение короткой типовой строки перестало быть
|
||||
достаточным условием.
|
||||
|
||||
Находка **в скоупе** этого issue: `scripts/no-new-any.mjs` — часть диффа,
|
||||
внесённая для решения задачи (класс B, переиспользует номер #592). Без её
|
||||
устранения — жёлтый вердикт, правка ожидается в этом же issue, не отдельным.
|
||||
|
||||
### Low (снята с записью)
|
||||
|
||||
Нет — все второстепенные наблюдения либо покрыты объяснением автора (device-icon-state-table
|
||||
— пре-существующий дрейф окружения, не относится к диффу), либо не заслуживают
|
||||
отдельной строки.
|
||||
|
||||
## Что проверено и корректно
|
||||
|
||||
- Все четыре тела диалогов перенесены побайтово идентично (независимая сверка
|
||||
diff'ом, не на слово автора).
|
||||
- Делегаты в классе корректны, по одному на метод.
|
||||
- `CELL_CM_MIN`/`CELL_CM_MAX`, `DISPLAY_LABEL_KEYS`/`DISPLAY_HINT_KEYS`
|
||||
переехали полностью, без дублей в старом файле.
|
||||
- `_radarSetup` публичен и используется только новым модулем плюс существующим
|
||||
кодом класса — не расширяет поверхность сверх нужного.
|
||||
- Состояние не переехало (AC4), тест умеет падать (проверено мутацией).
|
||||
- Ленивый граф не задет: карточка не импортирует ни один новый модуль (K3, AC5).
|
||||
- Якоря мутантов (AC6) — все живы, единственный переехавший патч назван
|
||||
автором верно, текст якоря нигде не дублируется случайно.
|
||||
- AC2: все названные в задаче эталоны — 0 расхождений; расхождение на двух
|
||||
незаявленных кадрах отделено экспериментом (тот же прогон на чистом
|
||||
`origin/dev`) и не связано с диффом.
|
||||
- Три отклонения от ТЗ, перечисленные автором в хендоффе (сборщик
|
||||
`test/houseplan-source.mjs` подставляет тело вместо делегата;
|
||||
`test/space-dialog.test.mjs` читает новый файл; имена файлов пространства и
|
||||
комнаты уточнены против ТЗ) — все три проверены чтением, обоснованы и не
|
||||
меняют скоуп задачи, все три разрешены разделом «принято предположительно»
|
||||
ТЗ.
|
||||
- Трейлеры: оба коммита несут `Issue: #592` и `User-Visible: no`; изменений в
|
||||
`docs/CHANGELOG*.md` нет, что и требуется при `User-Visible: no`.
|
||||
- Отклонение фикс-коммита от заявленного скоупа ТЗ (`scripts/no-new-any.mjs`
|
||||
не назван в разделе «Скоуп») задокументировано автором прямо в теле коммита
|
||||
— не скрыто.
|
||||
|
||||
## Чего не проверял
|
||||
|
||||
- `python -m pytest tests_backend` — диф не трогает `custom_components/**/*.py`.
|
||||
- `npm run invariants` — диф не трогает геометрию/`layout`/`marker.space`/
|
||||
`open_spans`; скоуп задачи прямо это исключает (К3/не-скоуп в ТЗ).
|
||||
- Полную матрицу `demo/smoke_*.mjs` (250 файлов) — прогнал 29 из них (11 из
|
||||
AC1 + 18 выборочно по диалогам); остальные матчатся только на широко
|
||||
используемые имена полей хоста, не на код, специфичный для перенесённых
|
||||
диалогов.
|
||||
- Ручной HA-харнесс и перф-профили — не заявлены в AC, K1 отрицает видимые
|
||||
изменения.
|
||||
- Полный `docs:capture`/пересъёмку скриншотов — не требуется на этом треке
|
||||
(не кандидат беты), предупреждение `check-docs` ожидаемо и не ново.
|
||||
|
||||
## Вердикт
|
||||
|
||||
Жёлтый. Механический перенос сделан образцово и доказан сильнее, чем требует
|
||||
процесс (побайтовая сверка, а не «тесты прошли»); единственная находка — не в
|
||||
самой задаче #592, а в сопутствующем гейте, который эта задача добавляет
|
||||
в постоянную инфраструктуру. Гейт можно обойти способом, не покрытым его же
|
||||
тестами и мутантами, и это не гипотетика — демонстрируется тем же коммитом.
|
||||
Правка — в `scripts/no-new-any.mjs` и `test/no-new-any.test.mjs`, в этом же
|
||||
issue.
|
||||
|
||||
---
|
||||
|
||||
<!-- material-anchors: сгенерировано конвейером (#414) -->
|
||||
|
||||
## Материал раунда
|
||||
|
||||
- Ветка: `issue/592-extract-dialogs`, коммит `61c74a70128a` — ребейз его осиротит, и это нормально: ниже якоря, которые ребейз не меняет.
|
||||
- Дерево материала: `f6590113b097f17d36e24483f2467ee7e12f73fa`
|
||||
```
|
||||
git log --all --format='%H %T' | grep f6590113b097
|
||||
```
|
||||
- Тело issue: `b105f7b3eb3f70575ee5751d1a917940da949981400e342e9cb133153057f673`
|
||||
- Вердикт конвейера: `yellow` · High 0
|
||||
Reference in New Issue
Block a user