diff --git a/docs/reviews/CODE-REVIEW-390-r1.md b/docs/reviews/CODE-REVIEW-390-r1.md index 612afbe7..f3411c88 100644 --- a/docs/reviews/CODE-REVIEW-390-r1.md +++ b/docs/reviews/CODE-REVIEW-390-r1.md @@ -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(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) `` +**Оговорка о независимости.** Это саморевью по прямому указанию владельца: +автор и ревьюер — одна сессия. По 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; 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). +Приёмка этих восьми — отдельное решение владельца по артефакту 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 +С оговоркой о независимости из первого раздела.