houseplan-card/docs/reviews/CODE-REVIEW-592-r1.md

Код-ревью #592 · заход r1

SHA материала: 61c74a70128a29871547519750bae32695f6e56a (b76f3e57 + фикс-коммит 61c74a70). Ветка: issue/592-extract-dialogs. Заход r1 (первое фактическое ревью: предыдущая попытка на b76f3e57 была отменена стражем до чтения кода — красный Validate на no-new-any, прогон 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, которая просто текстуально совпадает с удалённой:

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.

---

Материал раунда

• Ветка: issue/592-extract-dialogs, коммит 61c74a70128a — ребейз его осиротит, и это нормально: ниже якоря, которые ребейз не меняет.
• Дерево материала: f6590113b097f17d36e24483f2467ee7e12f73fa

  git log --all --format='%H %T' | grep f6590113b097
  

• Тело issue: b105f7b3eb3f70575ee5751d1a917940da949981400e342e9cb133153057f673
• Вердикт конвейера: yellow · High 0