diff --git a/docs/reviews/CODE-REVIEW-390-r1.md b/docs/reviews/CODE-REVIEW-390-r1.md new file mode 100644 index 00000000..612afbe7 --- /dev/null +++ b/docs/reviews/CODE-REVIEW-390-r1.md @@ -0,0 +1,176 @@ +# 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/` нет — процессный гейт это и +предупреждает, и допускает). + +Пять мест: +1. `houseplan-card.ts:12080` — `_climateCache: { h: any; r: any; mk: any; ... }` +2. `houseplan-editor-runtime.ts:11828` — `Object.values(full?.entities || {})` +3. `houseplan-editor-runtime.ts:11831` — `Object.values(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) `` + +Реализовано одним коммитом `cbf5cc1b`, трейлеры `Issue: #390` · +`User-Visible: no` на месте. `User-Visible: no` корректен: правка стирается +при компиляции, эмит бандла байт-в-байт идентичен (см. ниже) — changelog не +требуется, и его действительно нет в диффе. + +## Как проверялось + +Готового зелёного прогона 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 не добавляет и не меняет +видимую пользователю величину — это чисто типовая правка, значения времени +исполнения не меняются (подтверждено байт-в-байт идентичным бандлом). + +## Находки + +### H1 (High) — `check-docs` красный на этом SHA, гейт не прогнан и не назван автором + +`node scripts/check-docs.mjs` на `cbf5cc1b` завершается ошибкой: + +``` +ERROR screenshot source fingerprint is stale; run npm run build && node demo/docs/capture.mjs +``` + +Проверено, что причина — именно этот коммит, а не унаследованный долг: +откат только двух правленых файлов (`src/houseplan-card.ts`, +`src/houseplan-editor-runtime.ts`) к состоянию `HEAD~1` при прочем дереве на +`HEAD` возвращает `check-docs` в зелёное состояние («Documentation checks +passed»). Значит непосредственно перед этим коммитом `dev` был зелёным по +этому гейту, а сам коммит его сломал. + +`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. + +Воспроизведение: +``` +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` вне скоупа + +В `houseplan-editor-runtime.ts` тот же паттерн (`as any` на литеральных и +шаблонных ключах `device_inbox.*`) остаётся ещё в 25 строках (26 вхождений, +строка 12055 — два каста). Автор сам обнаружил и назвал это в комментарии, +явно оставив решение ревьюеру («В скоуп этого issue не тащил — заведу +отдельным, если скажете»). Это Medium **вне скоупа** #390 (он был заведён под +пять конкретно перечисленных мест) — заведено отдельным issue со ссылкой на +#390: **#391** (`tech-debt`, `P3`, `S1-new`). Оставлять это только текстом +ревью было бы нарушением §12 («Оставили в тексте ревью» закрытием не +считается), поэтому issue заведён явно, а не просто упомянут. + +## Что проверено и корректно + +- **Все пять мест закрыты типом, не заглушкой.** Ни одного `// 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; devices?: Record }` + описывает ровно два поля, которые код читает (`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). + +## Унаследовано (для сведения; это первый заход по #390, наследовать не от чего) + +Не применимо — заход r1. + +## Вердикт + +Единственная блокирующая находка (H1) не в правильности типовой правки — она +верна, доказана компиляцией и байт-в-байт идентичным бандлом — а в +обязательном гейте `check-docs`, который этот же коммит сделал красным на +`dev` и который не был прогнан и не назван автором. Это ровно то, что +`PROCESS.md` §8 называет ценой пропуска на конкретных примерах (#230, #234, +#237): гейт нужно чинить в той же задаче, не переносить на следующую. + +Вердикт: жёлтый · заход r1 · блокирующих циклов 1/2 · High: 1 · Medium: 1 → #391 +Документ: docs/reviews/CODE-REVIEW-390-r1.md