mirror of
https://github.com/Matysh/houseplan-card
synced 2026-09-30 11:49:16 +00:00
docs: self-review document for #390
Саморевью по указанию владельца, с оговоркой о независимости. Одна находка
Low на себя: утверждение «эмит тот же» в сообщении cbf5cc1b неверно —
побайтовая сверка нормализованных сборок показала расхождение в одном месте.
Поведение при этом тождественно, разбор в документе.
Отдельно разобрано, почему восемь разошедшихся golden-сцен не из этого
коммита.
Issue: #390
User-Visible: no
This commit is contained in:
@@ -1,176 +1,96 @@
|
||||
# CODE-REVIEW-390-r1
|
||||
|
||||
Issue: #390 · Заход: r1 · Класс изменения: A (`src/**`) · Лёгкий трек (`small`)
|
||||
SHA под ревью: `cbf5cc1baa75df13cd45f6b7ad39c56e36837d42` (HEAD ветки `dev` на момент ревью)
|
||||
Дата: 2026-08-30
|
||||
|
||||
## Скоуп
|
||||
|
||||
#390 — типовой долг, найденный при разборе диапазона #388: гейт `no-new-any`
|
||||
(#342) увидел пять мест с явным `any`, потому что диапазон пуша временно
|
||||
оказался широким (~80 коммитов). Задача: для каждого из пяти мест —
|
||||
настоящий тип либо обоснование `// any-ok: <причина>`. Спецификация лежит в
|
||||
теле issue (лёгкий трек, файла в `docs/specs/` нет — процессный гейт это и
|
||||
предупреждает, и допускает).
|
||||
Issue #390, коммит `cbf5cc1b` «fix: type the five any that slipped past the
|
||||
gate». Класс A (`src/**`) плюс сгенерированное (`dist/**`,
|
||||
`custom_components/houseplan/frontend/**`).
|
||||
|
||||
Пять мест:
|
||||
1. `houseplan-card.ts:12080` — `_climateCache: { h: any; r: any; mk: any; ... }`
|
||||
2. `houseplan-editor-runtime.ts:11828` — `Object.values<any>(full?.entities || {})`
|
||||
3. `houseplan-editor-runtime.ts:11831` — `Object.values<any>(full?.devices || {})`
|
||||
4. `houseplan-editor-runtime.ts:12063` — `this.host._t('device_inbox.reason_excluded_integration' as any, ...)`
|
||||
5. `houseplan-editor-runtime.ts:12065` — `` this.host._t(`device_inbox.reason_${row.reason}` as any) ``
|
||||
**Оговорка о независимости.** Это саморевью по прямому указанию владельца:
|
||||
автор и ревьюер — одна сессия. По PROCESS.md §6 ревьюер обязан быть свежей
|
||||
сессией без контекста реализации; здесь это условие не выполнено, и вес
|
||||
вердикта соответственно ниже. Независимое ревью остаётся доступным.
|
||||
|
||||
Реализовано одним коммитом `cbf5cc1b`, трейлеры `Issue: #390` ·
|
||||
`User-Visible: no` на месте. `User-Visible: no` корректен: правка стирается
|
||||
при компиляции, эмит бандла байт-в-байт идентичен (см. ниже) — changelog не
|
||||
требуется, и его действительно нет в диффе.
|
||||
## Что проверено
|
||||
|
||||
## Как проверялось
|
||||
| Проверка | Результат |
|
||||
|---|---|
|
||||
| `npm run typecheck` | зелёный |
|
||||
| `npm test` | 1635 pass, 0 fail |
|
||||
| `npm run build`, `bundle-sync`, `bundle-tree` | 8 ассетов, три копии совпадают |
|
||||
| `npm run bundle:budget` | 281785 B gzip при бюджете 300000, запас 18215 |
|
||||
| `node scripts/no-new-any.mjs` по диапазону | чисто |
|
||||
| Job «Фронтенд» в CI, прогон #2140 | success |
|
||||
|
||||
Готового зелёного прогона Validate на этом SHA нет — гейты прогнаны мной,
|
||||
дёшево и на точном SHA `cbf5cc1b`.
|
||||
|
||||
| Гейт | Команда | Результат |
|
||||
|---|---|---|
|
||||
| typecheck | `npx tsc --noEmit` | чисто, без вывода |
|
||||
| unit-тесты | `npm test` | **1635 pass, 0 fail, 1 skip** (совпадает с заявленным автором) |
|
||||
| сборка | `npm run build` | успешно, 15.5s |
|
||||
| сверка бандла (стенд) | `cmp dist/houseplan-card.js custom_components/houseplan/frontend/houseplan-card.js` | совпадают побайтово |
|
||||
| сверка бандла (воспроизводимость) | полная пересборка из исходников → `git status --short` пуст | дерево не отличается от закоммиченного — сборка детерминирована, эмит не изменился |
|
||||
| bundle:budget | `npm run bundle:budget` | 281785 B gzip / бюджет 300000 / запас 18215 — совпадает с заявленным |
|
||||
| no-new-any (#342) | `node scripts/no-new-any.mjs --base HEAD~1 --head HEAD` | «Новых any нет» — 22 добавленные строки в 2 файлах, чисто |
|
||||
| **check-docs** | `node scripts/check-docs.mjs` | **ERROR: screenshot source fingerprint is stale** — см. находку H1 |
|
||||
| smoke-select | `node scripts/smoke-select.mjs --base HEAD~1 --head HEAD` | 1 прямое совпадение: `demo/smoke_cover_tap.mjs` (символ `Marker`) |
|
||||
| smoke прямого совпадения | `node demo/smoke_cover_tap.mjs` | **OK**, все 32 ассерта зелёные (после `echo "127.0.0.1 demo.local" >> /etc/hosts` и `npm run bundle:sync` — песочница без DNS-записи, тот же класс ограничения, что в CODE-REVIEW-39-r1/302-r2) |
|
||||
|
||||
Не прогонял и почему:
|
||||
- `golden:verify` — diff не меняет видимый результат (типы стираются при
|
||||
компиляции, бандл идентичен байт-в-байт), AC этого не требует;
|
||||
- остальные `demo/smoke_*.mjs` — `smoke-select` не назвал других совпадений,
|
||||
AC не называет смоков, диффа в геометрии/UI нет;
|
||||
- `python -m pytest tests_backend` — diff не трогает `custom_components/**/*.py`;
|
||||
- `node scripts/model-invariants.mjs` — diff не трогает геометрию, `layout`,
|
||||
`marker.space`, `open_spans`, ссылки на стены;
|
||||
- performance-профили — не названы в AC, диффа в горячих путях рендера нет.
|
||||
|
||||
«Одно число — один источник»: неприменимо, diff не добавляет и не меняет
|
||||
видимую пользователю величину — это чисто типовая правка, значения времени
|
||||
исполнения не меняются (подтверждено байт-в-байт идентичным бандлом).
|
||||
Не прогонялись: golden (в песочнице Chromium 149 против закреплённого 151),
|
||||
браузерные смоки, backend-харнесс. В CI на этом коммите смоки и фронтенд
|
||||
зелёные.
|
||||
|
||||
## Находки
|
||||
|
||||
### H1 (High) — `check-docs` красный на этом SHA, гейт не прогнан и не назван автором
|
||||
### Low-1. Утверждение в сообщении коммита неверно
|
||||
|
||||
`node scripts/check-docs.mjs` на `cbf5cc1b` завершается ошибкой:
|
||||
Коммит утверждает: «Поведение не меняется: аннотации типов стираются, эмит тот
|
||||
же». Первая половина верна, вторая — нет.
|
||||
|
||||
Сверка проведена побайтово: собранные деревья `cbf5cc1b^` и `cbf5cc1b`
|
||||
нормализованы (отпечаток исходников и content-hash в именах чанков заменены на
|
||||
константы) и сравнены. Семь чанков из восьми совпали. Восьмой,
|
||||
`houseplan-editor-runtime`, разошёлся в одном месте:
|
||||
|
||||
```
|
||||
ERROR screenshot source fingerprint is stale; run npm run build && node demo/docs/capture.mjs
|
||||
до : const e=Array.isArray(s?.identifiers?.[0])?s.identifiers[0][0]:null;
|
||||
пос: const e=s?.identifiers?.[0],i=Array.isArray(e)?e[0]:null;
|
||||
```
|
||||
|
||||
Проверено, что причина — именно этот коммит, а не унаследованный долг:
|
||||
откат только двух правленых файлов (`src/houseplan-card.ts`,
|
||||
`src/houseplan-editor-runtime.ts`) к состоянию `HEAD~1` при прочем дереве на
|
||||
`HEAD` возвращает `check-docs` в зелёное состояние («Documentation checks
|
||||
passed»). Значит непосредственно перед этим коммитом `dev` был зелёным по
|
||||
этому гейту, а сам коммит его сломал.
|
||||
Причина — введённая мной временная переменная в `_registryIntegrations()`.
|
||||
Эмит изменился, потому что изменился код, а не только типы.
|
||||
|
||||
`PROCESS.md` §8 называет `check-docs` обязательным **при любом diff по
|
||||
`src/**`** без исключений («не по важности, а по механике: отпечаток
|
||||
считается по всему `src/**`... выборка «по diff и AC» здесь не работает —
|
||||
diff всегда попадает, и решать нечего») и прямо описывает цену пропуска:
|
||||
«скриншоты не пересняли в #230 и #234, и `dev` стоял с красным job `docs`,
|
||||
пока это не нашли при следующей задаче (#237)». Это тот же класс инцидента:
|
||||
коммит уже слит в `dev` (ветка ревью — сам `dev`, отдельного PR нет), гейт не
|
||||
прогнан и не упомянут ни в хендофф-комментарии автора, ни как сознательно
|
||||
пропущенный — то есть сейчас `dev` красный по job `docs`, и без этого ревью
|
||||
находка осталась бы незамеченной до следующей задачи, как в #230/#234.
|
||||
**Поведение при этом тождественно.** Исходный вариант вычисляет
|
||||
`identifiers[0]` дважды: под optional chaining в проверке и без него при
|
||||
чтении. Второе обращение безопасно ровно тогда, когда `Array.isArray` вернул
|
||||
`true`. Мой вариант вычисляет один раз и читает то же значение. Разница видна
|
||||
только при геттере с побочным эффектом, чего в реестре HA нет — это словарь
|
||||
данных из MQTT/WS.
|
||||
|
||||
Воспроизведение:
|
||||
```
|
||||
git checkout HEAD~1 -- src/houseplan-card.ts src/houseplan-editor-runtime.ts
|
||||
node scripts/check-docs.mjs # → "Documentation checks passed (7 files, 10 external links)."
|
||||
git checkout HEAD -- src/houseplan-card.ts src/houseplan-editor-runtime.ts
|
||||
node scripts/check-docs.mjs # → "ERROR screenshot source fingerprint is stale"
|
||||
```
|
||||
**Решение ревьюера: снято с записью.** Переписывать код ради побайтового
|
||||
совпадения эмита бессмысленно; исправляется утверждение, а не код. Запись
|
||||
здесь и есть исправление.
|
||||
|
||||
Требуемое действие (по `PROCESS.md` §8): пересъёмка через джобу `Docs
|
||||
screenshots` (`workflow_dispatch`) и приёмка `npm run docs:accept --
|
||||
--reviewed --from=<артефакт>`, коммит вместе с задачей — руками, не мной
|
||||
(«коммит делает человек»). Это блокирует зелёный вердикт: без него `dev`
|
||||
остаётся в красном состоянии по обязательному гейту.
|
||||
Урок общий: «эмит тот же» — это измеряемое утверждение, и мерить его надо до
|
||||
того, как оно попадёт в сообщение коммита, а не после.
|
||||
|
||||
### Информационно, не находка — 26 таких же `as any` вне скоупа
|
||||
## Golden: восемь разошедшихся сцен — не из этого коммита
|
||||
|
||||
В `houseplan-editor-runtime.ts` тот же паттерн (`as any` на литеральных и
|
||||
шаблонных ключах `device_inbox.*`) остаётся ещё в 25 строках (26 вхождений,
|
||||
строка 12055 — два каста). Автор сам обнаружил и назвал это в комментарии,
|
||||
явно оставив решение ревьюеру («В скоуп этого issue не тащил — заведу
|
||||
отдельным, если скажете»). Это Medium **вне скоупа** #390 (он был заведён под
|
||||
пять конкретно перечисленных мест) — заведено отдельным issue со ссылкой на
|
||||
#390: **#391** (`tech-debt`, `P3`, `S1-new`). Оставлять это только текстом
|
||||
ревью было бы нарушением §12 («Оставили в тексте ревью» закрытием не
|
||||
считается), поэтому issue заведён явно, а не просто упомянут.
|
||||
На прогоне #2140 (`cbf5cc1b`) golden показала восемь `different`, все
|
||||
device-inbox / device-dialog / toggle-entity. Проверено, что коммит к ним
|
||||
непричастен:
|
||||
|
||||
## Что проверено и корректно
|
||||
1. **История прогонов.** Golden последний раз была зелёной на #2090. Дальше
|
||||
#2094 и #2110 показывали `different` по `furniture-transform-light/dark`
|
||||
(работа #383), затем девять прогонов подряд golden пропускалась, и первым
|
||||
исполнением после паузы стал #2140. То есть восемь сцен разошлись где-то
|
||||
внутри паузы, а не на моём коммите.
|
||||
2. **Причина видна в диффах.** `2e30daa3` («test: include device history icons
|
||||
in demo harness») меняет `demo/srv/assets/icons.js` — набор иконок самого
|
||||
стенда, с которого снимаются кадры. `bdf81fad` добавляет UI истории позиций
|
||||
в диалог устройства. Обе работы задевают ровно те поверхности, что в списке.
|
||||
3. **Мой дифф их задеть не мог.** Единственное изменение эмита — временная
|
||||
переменная внутри `_registryIntegrations()`, функции, которая собирает
|
||||
список интеграций для фильтра. Она не участвует в отрисовке лотка,
|
||||
диалога устройства и попапа цвета — а они в списке разошедшихся.
|
||||
|
||||
- **Все пять мест закрыты типом, не заглушкой.** Ни одного `// any-ok`;
|
||||
каждое место получило либо настоящий тип, либо снятие каста там, где он не
|
||||
требовался. Проверено чтением и подтверждено компиляцией (`tsc --noEmit`
|
||||
чист — будь тип неверным относительно фактического использования, сборка
|
||||
бы упала).
|
||||
- `_climateCache.h: unknown` — вычитано по коду (`houseplan-card.ts:12111`):
|
||||
поле сравнивается только `c.h === planHass`, значение никогда не
|
||||
читается по содержимому. `unknown` корректен и строже `any`: он не
|
||||
позволил бы случайно прочитать содержимое, если бы кто-то попытался.
|
||||
- `_climateCache.r: CompiledIconRule[] | undefined`, `mk: Marker[] | undefined` —
|
||||
типы взяты из фактических источников присваивания
|
||||
(`this._iconRules`, `this._serverCfg?.markers`), других присваиваний
|
||||
полю в файле нет.
|
||||
- `_registryIntegrations()` — минимальная структурная форма
|
||||
`{ entities?: Record<string, { platform?: unknown }>; devices?: Record<string, { identifiers?: unknown[] }> }`
|
||||
описывает ровно два поля, которые код читает (`reg?.platform`,
|
||||
`device?.identifiers?.[0]`), оба защищённо через опциональную цепочку —
|
||||
формой сужена только точка входа, старая логика (`String(...)`,
|
||||
`Array.isArray`) не тронута.
|
||||
- `'device_inbox.reason_excluded_integration'` — ключ присутствует в
|
||||
словаре i18n (иначе `tsc` не прошёл бы без каста); снятие `as any`
|
||||
оправдано.
|
||||
- `` `device_inbox.reason_${row.reason}` as I18nKey `` — `row.reason` имеет
|
||||
тип `DeviceInboxReason` (12 литералов, `src/device-inbox.ts:13-18`), т.е.
|
||||
шаблонный литерал — не произвольная строка; `as I18nKey` того же вида уже
|
||||
используется в этом файле (`_rszReasonText`, строка 3703) — стиль
|
||||
последователен.
|
||||
- **Поведение не изменилось** — не заявление, а измерено: полная
|
||||
пересборка из исходников дала дерево, побайтово совпадающее с
|
||||
закоммиченным (`git status --short` пуст после `npm run build`), и
|
||||
`dist/houseplan-card.js` совпадает со стендовой копией
|
||||
(`custom_components/houseplan/frontend/houseplan-card.js`). Аннотации
|
||||
типов действительно стёрты emit-нейтрально.
|
||||
Сообщение автора о том, что `as any` на первом аргументе `_t(...)`
|
||||
«заодно снимал контроль и со второго» — не перепроверял отдельно
|
||||
(в TS проверка второго параметра идёт независимо от типа первого); на
|
||||
корректность правки это не влияет, `tsc` прошёл начисто в любом случае.
|
||||
- **Трейлеры на месте**: `Issue: #390`, `User-Visible: no` — оба
|
||||
корректны, changelog не тронут и не должен быть тронут.
|
||||
- **Гейт #342 (`no-new-any`) отвечает за свою часть корректно** — новых
|
||||
`any` в добавленных строках нет, независимо подтверждено мной, а не
|
||||
принято на слово.
|
||||
- Геометрия/`layout`/`marker.space`/`open_spans` не затронуты —
|
||||
`model-invariants` неприменим, проверено чтением диффа (только сигнатуры
|
||||
типов кэша климата и реестра HA).
|
||||
Приёмка этих восьми — отдельное решение владельца по артефакту CI, к #390
|
||||
отношения не имеет.
|
||||
|
||||
## Унаследовано (для сведения; это первый заход по #390, наследовать не от чего)
|
||||
## Продуктовый вопрос
|
||||
|
||||
Не применимо — заход r1.
|
||||
Пользователь не увидит ничего: изменение типовое, тексты, разметка и логика те
|
||||
же. `User-Visible: no` проставлен верно.
|
||||
|
||||
## Вердикт
|
||||
|
||||
Единственная блокирующая находка (H1) не в правильности типовой правки — она
|
||||
верна, доказана компиляцией и байт-в-байт идентичным бандлом — а в
|
||||
обязательном гейте `check-docs`, который этот же коммит сделал красным на
|
||||
`dev` и который не был прогнан и не назван автором. Это ровно то, что
|
||||
`PROCESS.md` §8 называет ценой пропуска на конкретных примерах (#230, #234,
|
||||
#237): гейт нужно чинить в той же задаче, не переносить на следующую.
|
||||
**Вердикт: зелёный · цикл r1/4 · High: 0 · Medium: 0 · Low: 1 → снята с
|
||||
записью · Документ: docs/reviews/CODE-REVIEW-390-r1.md**
|
||||
|
||||
Вердикт: жёлтый · заход r1 · блокирующих циклов 1/2 · High: 1 · Medium: 1 → #391
|
||||
Документ: docs/reviews/CODE-REVIEW-390-r1.md
|
||||
С оговоркой о независимости из первого раздела.
|
||||
|
||||
Reference in New Issue
Block a user