diff --git a/docs/reviews/CODE-REVIEW-592-r1.md b/docs/reviews/CODE-REVIEW-592-r1.md new file mode 100644 index 00000000..86aa60fc --- /dev/null +++ b/docs/reviews/CODE-REVIEW-592-r1.md @@ -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` без привязки к конкретной паре файлов +источник→приёмник). Для механического переноса тела функции это работает +верно. Но гейт — не разовый скрипт для этой задачи, а постоянная часть +`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` +возвращает пустой список вместо находки. Гейт молча пропускает новый долг. + +Это не гипотетическая экзотика: в этом же коммите естественным образом +возникла пара строк с идентичным текстом +(`${this.host._t(k as any)}` — и в `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. + +--- + + + +## Материал раунда + +- Ветка: `issue/592-extract-dialogs`, коммит `61c74a70128a` — ребейз его осиротит, и это нормально: ниже якоря, которые ребейз не меняет. +- Дерево материала: `f6590113b097f17d36e24483f2467ee7e12f73fa` + ``` + git log --all --format='%H %T' | grep f6590113b097 + ``` +- Тело issue: `b105f7b3eb3f70575ee5751d1a917940da949981400e342e9cb133153057f673` +- Вердикт конвейера: `yellow` · High 0