From 32791fb336e07417556a18e1986ed1a9f4e0d41b Mon Sep 17 00:00:00 2001 From: "claude[bot]" <209825114+claude[bot]@users.noreply.github.com> Date: Sun, 20 Sep 2026 11:48:27 +0000 Subject: [PATCH] docs: review document for #600 Issue: #600 User-Visible: no --- docs/reviews/CODE-REVIEW-600-r1.md | 273 +++++++++++++++++++++++++++++ 1 file changed, 273 insertions(+) create mode 100644 docs/reviews/CODE-REVIEW-600-r1.md diff --git a/docs/reviews/CODE-REVIEW-600-r1.md b/docs/reviews/CODE-REVIEW-600-r1.md new file mode 100644 index 00000000..ec271c3e --- /dev/null +++ b/docs/reviews/CODE-REVIEW-600-r1.md @@ -0,0 +1,273 @@ +# CODE-REVIEW-600-r1 + +Issue: #600 · Этап: code · Заход: r1 · Материал: `fa12045271f47bac64db153b60737bc7bb4e5336` (детач, диапазон `origin/dev..HEAD`, `origin/dev` = `6cee9504`) + +Вердикт: **жёлтый** · заход r1 · блокирующих циклов 1/4 · High: 0 · Medium: 4 → в задаче + +## Скоуп и что проверялось + +Задача переписывает внутренности четырёх диалогов настроек (Space, General +settings, Room settings, Device on the plan) под референс дизайнера, +сохраняя запись состояния (К1–К10 ТЗ). Диапазон: 15 коммитов поверх `dev`, +199 файлов, из них по существу — 35 файлов `src/**` (3554+/2191-), 64 файла +тестов/смоков/докс/чейнджлога. Референс, `SPEC.md`, `IMPLEMENTATION-GUIDE.md` +уже разложены в `docs/design/600-settings-dialogs/` (AC0, принят на спек-ревью). + +Спек-ревью r1 по этой же задаче — зелёный (комментарий issue от 20.09, +06:18), одна Low-находка снята без последствий для кода. Это первый заход +код-ревью, предыдущего кода-ревью раунда не было — раздела «Унаследовано из +r0» нет, разбор ниже полный. + +### Методология + +1. Прочитан `git diff origin/dev...HEAD` целиком по файлам (частями для + `marker-dialog.ts`, 1247 строк диффа) и структура `docs/design/...`. +2. Запущено четыре параллельных фоновых агента с точными предписаниями + (файлы, строки ТЗ, конкретные критерии) на AC1/AC4/AC8 (по диалогам), + AC3/AC5/AC6/AC7 и мутанты, AC9–AC13 + трейлеры/changelog. Их находки + перепроверены выборочно (see below), а не приняты на слово. +3. Самостоятельно: визуальное попарное сравнение референс/продукт по всем + 4 диалогам × 2 темы (`docs/design/600-settings-dialogs/pairs/*.png` против + `screenshots/*.png`, с покадровым zoom через Pillow) — так была найдена + находка №1 ниже, которую не поймал ни один смок и не назвал сам автор. +4. Проверка полноты CI-подтверждения на точном материале: dispatch-прогон + `35506392847` на `fa120452` **не** тяжёлый (`full=true` не задан) и для + `smoke`/`golden` не нашёл кэша переиспользования («Cache not found for + input keys: reuse-smoke-…», «reuse-golden-…» — see `Переиспользование` job + лога), то есть **ни smoke-матрица, ни golden не подтверждены CI на самом + материале ревью** — только на промежуточном `504c1609` (там же — одна + красная браузерная проверка, исправленная позже в `4bd80e46` без + последующего CI-подтверждения). Это закрыто мной лично (см. «Гейты» ниже), + а не принято на слово автора. + +## Гейты — что прогнано и чем + +Дешёвые гейты (`tsc --noEmit`, `npm test`, `npm run build`) уже зелёные на +`fa120452` по ссылке из задания (Validate `35506392847`, success) — не +перегонялись. + +Прогнано лично или агентами, с результатом: + +| Гейт | Кто | Результат | +|---|---|---| +| `npm run build && npm run bundle:sync` | я | чисто, дерево не изменилось | +| `npm run golden:verify` (полный, 173 сцены) | я | **173/173 passed** — закрывает пробел CI (см. выше); подтверждает AC10 на самом материале, включая 13 новых и все старые сцены побайтово | +| `node demo/smoke_room_tooltip_toggle.mjs` | я | OK, все 25 ключей `true` — закрывает пробел CI: у фикса из `4bd80e46` не было ни одного зелёного CI-прогона на финальном дереве | +| `node demo/smoke_dialog_config_parity.mjs` | агент A | OK (AC3, побайтовое сравнение конфига через UI-путь и state-путь) | +| `node demo/smoke_{space,general,room,device}_settings_form.mjs` | агенты A/B/C | все OK; проверяют собственный ключ + нетронутость соседнего для каждого нового контрола, `role=radiogroup`+`aria-label` на сегментах, цели ≥44px | +| `node demo/smoke_color_picker_consumers.mjs` | агент A | OK, `nativeColors()===0` держится | +| `node demo/smoke_esc_dialogs.mjs` | агент A | OK, включая переспрос при закрытии с изменениями | +| `node demo/smoke_help_affordance.mjs`, `node --test test/i18n-dead-keys.test.mjs` | агент A | OK | +| `node --test test/form-kit.test.mjs` | агент D | 5/5 pass, включая байтовое сравнение CSS панели с замороженной `test/fixtures/summary-panel-editor.css` (файл не тронут диапазоном — AC11 не фиктивен) | +| `npm run bundle:budget` | агент D | потолок `LAZY_EDITOR_GZIP_CEILING` 222900→243900 с построчным обоснованием по сериям; факт 243125, зазор 775/1225 Б (оба >500) | +| `node scripts/check-docs.mjs` | агент D | зелёный на HEAD (скриншоты пересняты в `fa120452`) | +| `npm test` (полный) | агенты A/D независимо | 2794/2793 pass, единственный флак — не связанный с #600 тест по времени сборки одного этажа (`#509`), не влияет на вердикт | + +### Чего не проверял и почему + +- **Мутанты из `scripts/mutation-registry.mjs` не исполнены живьём** (patch → + guard → RED → revert) — только статическая проверка, что anchor-строки + существуют один раз и обоснование соответствует коду. Полный mutation-run + дорог и не назван в AC как обязательный отдельно от диффовых мутантов, + которые уже зелёные в CI на этом SHA («Мутанты по диффу 1–6/6» — success). +- **Не все 88 «прямых совпадений» из `smoke-select.mjs` перепрогнаны поштучно + моей рукой.** Полная матрица уже прогнана CI дважды за время задачи + (`workflow_dispatch full=true` на `504c1609`, шарды 1/3 и 3/3 — success + целиком, шард 2/3 — одна красная, исправленная в `4bd80e46` и лично + перепроверенная мной на финальном дереве, см. таблицу выше). Дельта между + `504c1609` и `fa120452` — сам смок-фикс, два golden-коммита и один + docs-тест, ни один из них не переиграл более широкий смок. Прогон всех 88 + заново не пропорционален размеру этой дельты. +- **`python -m pytest tests_backend`** не запускался — диапазон не трогает + `custom_components/houseplan/**/*.py` по существу (только собранный + `frontend/**`, класс D). +- **Живые ha-dialog** (реальный Home Assistant) не поднимались — только + демо-харнесс с нативным ``; это и есть причина, по которой находка + №3 ниже (поповер «?») осталась неподтверждённой в обе стороны, а не закрытой. + +## Находки (все — Medium, в скоупе задачи, к исправлению в этом же заходе) + +### 1. Room settings: подпись «Temperature» в 5-сегменте переносится посреди слова + +`docs/design/600-settings-dialogs/pairs/room-light.png` и `room-dark.png` — +кадры, которые сам автор снял и приложил как доказательство AC1 — показывают +сегмент «Fill mode» (`None | Zigbee | Lights | Temperature | Custom`), где +активный пункт отображается как `Temperatur` / `e` двумя строками внутри +кнопки. Референс (тот же `SPEC.md` §6.1, тот же список из пяти пунктов) +рассчитан на одну строку на пункт. + +Причина — `src/styles/form-kit.styles.ts:295-312`: `.hpf-seg label` держит +`flex: 1 1 0; min-width: 0; overflow-wrap: anywhere` — при пяти равных +колонках слово `Temperature` (11 симв.) не помещается и рвётся в +произвольном месте вместо аккуратного переноса или уменьшения кегля. +`src/editors/room-settings-dialog.ts:197-200` документирует, что автор был в +курсе тесноты пяти сегментов («Короткие подписи там, где полные не +помещаются…») и сократил `lqi`→«Zigbee», `custom`→«Custom», но не тронул +`fill.temp` — то самое слово, которое рвётся. RU-вариант («По температуре», +14 символов) длиннее английского и пострадает не меньше; DE/FR — сопоставимо. + +`docs/design/600-settings-dialogs/ACCEPTANCE.md:69` при этом утверждает +«Подписи сегмента заливки … то же» и не называет это расхождение — +единственная строка ACCEPTANCE.md про подписи segment на самом деле не +покрывает пять из пяти пунктов честно. AC2 («без переноса, читаемо при +длинных RU/DE/FR») формально проверяет только отсутствие горизонтального +скролла контейнера (`demo/smoke_room_settings_form.mjs:192`, +`content.scrollWidth <= content.clientWidth`), а перенос внутри кнопки +scrollWidth контейнера не трогает — поэтому смок зелёный, а дефект есть. + +**Сценарий:** открыть Room settings любой комнаты с выключенным «As the +space», посмотреть на сегмент Fill mode — на активном/любом широком языке +пункт «Temperature» отображается разорванным словом. Это ровно тот класс +дефекта («ложный паритет», «сырая раскладка»), ради которого заведён #600. + +### 2. Device dialog: у Glow radius появилась не согласованная владельцем блокировка Save + +`src/editors/marker-form-state.ts:41-58` вводит `glowRadiusValid()` и новый +`MarkerProblem` `marker.error_glow_radius`, который через +`marker-dialog.ts` (`canSave = ... && problems.length === 0 ...`) **блокирует +кнопку Save**, если поле Glow radius непусто и не является положительным +числом. + +На `dev` это поле никогда не блокировало Save: запись конфига +(`houseplan-editor-runtime.ts:7991-7995`, код не изменился в этом диапазоне) +молча приводит любое непустое невалидное или неположительное значение к +`null` (= общий радиус) прямо на сохранении. К10 в теле issue перечисляет +закрытым списком, какие условия остаются ошибками — «пустое имя, режим файла +без изображения, невалидный диапазон температур, отсутствие привязки при +выборе из HA» — Glow radius в списке нет. Ни `OPEN-POINTS.md`, ни решения +владельца от 20.09, ни один комментарий issue не называют эту новую +проверку. Это функциональное расширение контракта Save (AC6/К10) за пределы +переразметки, не решённое владельцем. + +**Сценарий:** диалог устройства → «Light and glow» → Glow radius → ввести +`0` или `-1` → кнопка Save становится неактивной без объяснения, почему это +стало ошибкой, — на `dev` то же действие сохраняло маркер с общим радиусом. + +Это не потеря данных и не крах, но это несогласованное расширение +поведения контракта Save, а не просто вёрстка. Fix: либо снять валидацию +(вернуть прежнее молчаливое приведение к `null`, как и было), либо получить +явное решение владельца о пятом условии ошибки Save и внести его в текст +К10, если задача снова пойдёт на спек-ревью, либо (раз это в рамках +code-review light track) явно задокументировать это расширение как +осознанное отступление в `ACCEPTANCE.md`/`IMPLEMENTATION-GUIDE.md` — сейчас +это выглядит как незамеченный побочный эффект. + +### 3. Дефект №5 (поповер «?» обрезается границей диалога комнаты) не имеет свидетеля закрытия + +AC8 требует «девять прямых дефектов… закрыты поимённо», «по свидетелю на +каждый». Для дефекта №5 (Room settings) свидетеля нет: + +- в диапазоне нет ни одной строки, адресующей клиппинг поповера (нет правки + `overflow`, нет промоушена `.overlay-portal` в top layer, `hp-help.ts` не + тронут); +- `ACCEPTANCE.md` не упоминает дефект №5 ни разу — только №1 (Zigbee links); +- в демо-харнессе (нативный ``) клиппинг не воспроизводится ни в + одну, ни в другую сторону — харнесс использует другой путь модальности, + чем реальный `ha-dialog` (`src/hp-dialog.ts:553-609`, + `_usesHaDialog()`), так что харнесс не может ни подтвердить, ни опровергнуть + исходный дефект; +- ни `demo/smoke_room_settings_form.mjs`, ни `demo/smoke_help_affordance.mjs` + не проверяют клиппинг попапа границей диалога. + +Не факт, что дефект жив (структура вокруг `hp-help` не менялась этим PR), но +AC8 требует именованного доказательства закрытия для каждого из девяти +пунктов, а не «скорее всего не мой код» — сейчас его нет ни для одного, ни +для другого исхода. Нужно либо воспроизвести на реальном `ha-dialog` +(методика #505) и показать, что не режется, либо признать дефект +непроверенным и решить его в этом же заходе. + +### 4. `ACCEPTANCE.md`: заявлено 11 эталонных сцен, фактически принято 13 + +`docs/design/600-settings-dialogs/ACCEPTANCE.md:28` (правился в `c3e64dc6`, +до принятия эталонов) утверждает «Эталоны одиннадцати сцен». Фактически +`ed2af1b1` («эталоны 13 сцен диалогов настроек») меняет хэши 13 сцен в +`demo/golden/baselines/baselines-index.json` — расхождение 11 vs 13 нигде не +объяснено, документ не синхронизирован после приёмки эталонов. AC1/AC10 +прямо требуют, чтобы расхождения были «названы», а не «примерно посчитаны» — +это тот самый принцип, ради которого заведён #600 (не доверять цифре на +глаз). Фикс — одна строка в `ACCEPTANCE.md`. + +### 5. `docs/USER-GUIDE.ru.md:529` — испорченная строка таблицы + +``` +| Карточки комнат | Сигнал Zigbee рядом с устройствами | Переопределяет базовую настройку карточки для этого пространства |ожки они остаются видимыми | +``` + +Хвост «ожки они остаются видимыми» — остаток удалённой строки про +декоративный слой, слипшийся с текущей ячейкой без переноса строки: лишняя +`|` создаёт для парсера Markdown лишний столбец, и в отрендеренной таблице +пользователь увидит обрывок чужого предложения. Английская версия +(`docs/USER-GUIDE.md`) аналогичной порчи не содержит — расхождение только в +RU. Раз `User-Visible: yes` требует правку `USER-GUIDE` как часть DoD (и она +входит в явный скоуп задачи), это Medium-находка внутри скоупа, а не +придирка к стилю — текст, который увидит пользователь, буквально сломан. + +## Что проверено и корректно (по AC) + +- **AC0** — референс/докс/PNG на месте, отпечаток архива сверен ранее на + спек-ревью; ничего из `docs/design/**` не попадает в бандл (`bundle-tree` + зелёный в CI). +- **AC1** (кроме находки №1) — прямое попарное сравнение восьми кадров + (4 диалога × 2 темы) с референсом показывает подлинный, а не косметический + паритет: сегменты, строки-тумблеры, плитки значений/цвета, компактная + плашка цвета, футер со статусом, единый скролл — всё воспроизведено по + структуре, а не только по обёртке карточек. Дублирующийся заголовок Zigbee + links (дефект №1) закрыт и подтверждён смоком (`zigbeeHeadingOnce`). +- **AC3** — доказано исполнением на двух независимых уровнях: побайтовое + сравнение сохранённого конфига (`smoke_dialog_config_parity.mjs`) и + поконтрольные unit/смок-проверки «свой ключ меняется — соседний нет» для + каждого нового примитива. Тесты способны падать (проверено по формулировке + ассертов, не только по названию). +- **AC4** — все ветки К4 (Entity to toggle, What to run, Leading light + entity, Value source, температура климата, радар/пылесос, баннеры + привязки, `ha_registry_limited`) сверены построчно с `origin/dev` — условия + видимости не изменились, изменена только обёртка в новые примитивы. +- **AC5** — `hp-color-opacity` не переписан по семантике (диф — 6 строк, + только `hideLabel`), `nativeColors() === 0` держится и подтверждено + смоком; вокруг него ровно новая раскладка (свотч/hex/Opacity/Reset). +- **AC6** — dirty-tracking и confirm-on-close переиспользуют существующий + `_confirmDanger` (не новый механизм), Save заблокирован при `busy` и + ошибках, «Review N fields» реально переводит фокус на первое проблемное + поле (проверено `activeElement.id`, не только текстом). +- **AC7** — `rhint` в четырёх целевых диалогах отсутствует (0 совпадений); + оставшиеся вхождения (`vacuum-maps-section.ts`, `import`-диалог онбординга) + — вне скоупа четырёх диалогов и не регрессия этого PR (были такими же на + `dev`). Сообщения-состояния (К5) остаются открытыми callout-блоками. +- **AC9** — `role="radiogroup"`/`role="group"` с `aria-label`, `label[for]`, + `aria-invalid`, `aria-live`, reduced-motion/forced-colors — присутствуют в + коде и подтверждены геометрией/атрибутами в реальном DOM через Playwright + (не заглушка по названию класса). +- **AC10** — 173/173 сцен golden проходят на материале ревью (прогнано + лично, см. «Гейты»); трейлеры `Release`/`Baseline-Reviewed` на коммите с + эталонами на месте. +- **AC11** — независимый замороженный фикстур панели (`summary-panel-editor.css`) + не тронут диапазоном, тест сравнивает его побайтово с реальным собранным + CSS — доказательство не фиктивно, рост диалоговых фикстур это отдельный, + явно расширяемый набор. +- **AC12** — потолок поднят с построчным обоснованием по сериям, факт держит + запас >500 Б с обеих сторон полосы. +- **Трейлеры/changelog** — все 15 коммитов несут `Issue`/`User-Visible`, + единственный `User-Visible: yes` правит оба changelog в этом же коммите. + +## Итог + +Ни одной High-находки: данные не теряются, инвариантов геометрии/замков не +касается, полный набор автотестов и golden зелёные на материале ревью. +Четыре находки Medium — все внутри объявленного скоупа задачи (Room dialog, +Device dialog, собственная документация задачи, USER-GUIDE) — по правилу +#202 не заводят отдельный issue и чинятся в этом же заходе. Возврат автору: +жёлтый. + +--- + + + +## Материал раунда + +- Ветка: `issue/600-settings-dialogs`, коммит `fa12045271f4` — ребейз его осиротит, и это нормально: ниже якоря, которые ребейз не меняет. +- Дерево материала: `437733cefa9f7febee3f9edfed4e5290760d517a` + ``` + git log --all --format='%H %T' | grep 437733cefa9f + ``` +- Тело issue: `2871bca5cd192e74cd862e2a4db109f96d559aecf77c8c2032369475e76aa1c6` +- Вердикт конвейера: `yellow` · High 0