diff --git a/docs/reviews/CODE-REVIEW-602-r1.md b/docs/reviews/CODE-REVIEW-602-r1.md new file mode 100644 index 00000000..772b7401 --- /dev/null +++ b/docs/reviews/CODE-REVIEW-602-r1.md @@ -0,0 +1,257 @@ +# CODE-REVIEW-602-r1 + +Issue: #602 · Этап: code · Заход: r1 · блокирующих циклов израсходовано 0 из 4 +Материал: `c69d73f1f26b3277fde938a4eb08195c561e46ba` (issue/602-dialog-polish), рабочая копия ревью — на этом SHA, без отклонений (`git status` чист на всём протяжении разбора). + +## Скоуп + +ТЗ (принято владельцем на этапе spec, зелёный вердикт r2) устраняет визуальные +дефекты общего form-kit после редизайна #591/#600/#601 в четырёх диалогах +(Space, General settings, Room settings, Device) плюс общий confirm-dialog: +подписи-дубли, центрирование тумблера, обрезание числовых полей, выравнивание +подписей шкалы, однострочные футеры, порядок climate-настройки и замена +кнопки объявления радара на inline-тумблер с сохранением черновика между +Off/On в рамках сессии. + +Диапазон разбора — `git diff origin/dev...HEAD` (82 файла: `src/**`, +`demo/smoke_*`, `test/**`, локали, `docs/**`, `dist/**` и зеркала бандла). +Полный код-ревью, поскольку это первый заход по задаче. + +## Как проверялось + +| Гейт | Статус | Как проверено | +|---|---|---| +| `typecheck` / `npm test` / `npm run build` + сверка 3 копий бандла | зелёный, не перегонял | Validate на точном материале `c69d73f1` — [прогон 35535791268](https://github.com/Matysh/houseplan-card/actions/runs/35535791268), `conclusion: success`, `headSha` совпадает. Подтверждено запросом `gh run view`. Мутанты по диффу (6/6 шардов) там же зелёные. | +| `node scripts/check-docs.mjs --strict` | зелёный, перегонял сам | `node scripts/check-docs.mjs --strict` → «Documentation checks passed (7 files, 12 external links)». Отпечаток `src/**` актуален после `npm run bundle:sync` на чистом дереве. | +| `npm run bundle:sync` (пересборка для последующих шагов) | выполнялся трижды (перед смоками, после каждой мутации) | `git status --short` пуст после каждого прогона — рабочее дерево не отклонилось от `c69d73f1`. | +| Смоки — 7 файлов, изменённых в этом диффе (`smoke_device_settings_form`, `smoke_space_settings_form`, `smoke_dialog_footer_width`, `smoke_radar_setup`, `smoke_climate_temp`, `smoke_general_settings_form`, `smoke_room_settings_form`) | зелёные, перегонял сам | Каждый запущен `node demo/smoke_.mjs`, все напечатали `OK` без `FAILED`. `scripts/smoke-select.mjs --base origin/dev --head HEAD` вернул одно **прямое совпадение** (`smoke_radar_setup.mjs` ← `_markerDialog, isActive`, уже в списке выше) и 39 «слабых связей» по одному распространённому имени `_markerDialog` — по прочтении диффа это ожидаемо (символ есть почти в каждом смоке, трогающем диалог устройства), решил не гонять весь список: ни один из 39 не адресует изменённые CSS-контракты специфично. | +| Мутационная проверка «чем краснеет» для двух защитных AC | выполнил вручную (см. таблицу ниже) | См. раздел «Защитные AC». | +| `npm run golden:verify` | **красный**, перегонял сам | См. находку High/Medium ниже — 8 из 150 сцен `different`. | +| `npm run inventory`, `python -m pytest tests_backend` | не гонял | diff не трогает `custom_components/**/*.py`; нет причины запускать. | +| Performance-профили | не гонял | Не названы в AC, диффом не задеты чувствительные к перфу пути (ТЗ §13 подтверждает). | + +## Находки + +### Medium (в скоупе) — `golden:verify` красный на материале ревью, обновление golden-сцен из плана тестирования не сделано + +ТЗ, план тестирования, п.4: «Обновить/добавить golden scenes минимум для +Space desktop/mobile, Device desktop/mobile с radar off/on, discard confirm +mobile…». AC15 прямо требует: «…`golden:verify` зелёные». Ни один файл под +`demo/golden/baselines/**` в диффе не изменился (`git diff +origin/dev...HEAD --stat -- demo/golden` — пусто). + +Автор сам отметил риск в комментарии реализации r1: «`golden:verify` на +Windows выполнен диагностически и baseline не менял… канонический кандидат +должен снять Linux CI… Прошу проверить… Linux visual artifacts» — то есть +явно передал этот пункт на сторону код-ревью. Ревью выполняется на Linux, я +запустил канонический прогон сам: + +``` +npm run golden:verify +``` + +Результат — 8 из 150 сцен `different` (не `passed`): `device-dialog-desktop-en`, +`device-dialog-mobile-ru`, `device-dialog-desktop-de`, `device-help-popover-light-ru`, +`settings-help-zoom-200-en-light`, `settings-help-zoom-200-ru-dark`, +`device-ripple-color-popover-mobile-ru`, `space-room-color-popover-desktop-ru`. +По коду `demo/golden/policy.mjs:63` (`goldenRunFailed`) `mode==='verify'` и +любой статус `!== 'passed'` переводит `process.exitCode = 1` — гейт красный, +а не «прошёл с примечаниями». + +Диф-артефакт `artifacts/golden/diff/device-dialog-desktop-en.png` показывает +ожидаемые различия: убранный текст «Unsaved changes» в футере, изменённая +раскладка поля/шкалы «Brightness», сдвиг блока «Glow radius» и всего, что +ниже — то есть разница закономерно объясняется именно этим диффом (CSS +`.hpf-range`, `.hpf-toggle`, `footerStatus`), а не посторонней регрессией. +Значит правки корректны по существу, но обещанный тестовый артефакт +(обновлённые/принятые baseline-кадры) отсутствует. + +**Чем краснеет:** сам факт — `npm run golden:verify` красный на предъявленном +материале; это не гипотетическая мутация, а фактический прогон канонического +гейта, который AC15 требует зелёным. Артефакты прогона удалены после +проверки (`rm -rf artifacts/golden`), рабочее дерево осталось чистым на +`c69d73f1`. + +**Что нужно:** принять новые кадры через `npm run golden:accept -- +--reviewed` (плюс `scripts/golden-accept.mjs --expect-change=<8 id>`) на +канонической Linux-сборке этого материала, с коммитом, несущим `Release:` и +`Baseline-Reviewed:` (путь уже отработан на этой же ветке коммитом +`c69d73f1` для отдельного, независимого от `demo/golden/baselines` +docs-скриншотного механизма — `check-docs.mjs`/`docs/images/**`, который сам +по себе закрыт корректно и трейлеров не требовал, поскольку не трогает +`demo/golden/baselines/**`; это подтверждено чтением +`scripts/validate-commit-provenance.mjs:64-77`, где триггер — только +`demo/golden/baselines/*.{png,json}`). + +**Серьёзность:** Medium, в скоупе задачи (сам ТЗ называет это условием +приёмки AC15 и явно перечисляет затронутые сцены в плане тестирования) — +без High это жёлтый вердикт, фикс проходит следующий цикл в этом же issue +(#202). + +## Проверено и корректно + +- **AC1/AC8 (лишние подписи и заголовок убраны).** `space.title_hint`, + `space.scale_hint`, `marker.name_hint`, заголовок «Tap action» удалены из + разметки и всех 4 locale-файлов `i18n/settings/*.json` синхронно (RU/EN/DE/FR); + `marker.card_tap` синхронно вычищен из `i18n/{ru,en,de,fr}.json`. Подтверждено + смоками `redundantBasicsHintsRemoved`, `nameFieldWithoutRedundantHint`, + `tapActionCardHasNoLargeHeading` — все зелёные при собственном прогоне. +- **AC2 (центрирование тумблера).** `top: 50%; transform: translateY(-50%)` в + `form-kit.styles.ts`; смок `switchPartsAreGeometricallyCentered` сравнивает + computed `::before`/`::after` с серединой контрола (допуск 0.6px). **Чем + краснеет:** вручную вернул `top: 14px`, пересобрал (`npm run bundle:sync`), + прогнал `smoke_space_settings_form.mjs` — упал именно на этом поле + (`switchPartsAreGeometricallyCentered: expected true, got false`), затем + восстановил файл и пересобрал; `git status` после — чист. +- **AC3/AC5 (числовые поля не обрезаются/не перекрывают слайдер).** Ширина + `.hpf-unit` для прозрачности увеличена до `4.8em`, `.hpf-range` переведён на + grid с фиксированной колонкой значения (`--hpf-range-value-width: 84px`). + Смоки проверяют реальные `getBoundingClientRect()`: `opacity100FitsTheNumberField`, + `sizeAndAngleFieldsDoNotCoverSliders` (slider.right < unit.left, no + overflow) — зелёные. +- **AC4 (подписи шкалы совпадают с концами трека).** `rangeEnds()` теперь + оборачивает подписи в `.hpf-range-ends-track`, выровненный `grid-column: 1` + с тем же `84px` резервом под значение, что и сам слайдер — единый источник + ширины вместо независимого `padding-right: 122px`. Смок + `fontScaleEndsMatchTheSlider` сверяет пиксельные левый/правый края трека и + подписей (допуск 1px) — зелёный. +- **AC6/AC7/AC14 (однострочные футеры, mobile icon-only, confirm в одну + строку, 320/360/560).** `dialogsStyles`: `.hpf-footer-space` — CSS grid + (`auto auto minmax(0,1fr) auto`) с явными позициями `dialog-action-copy`/ + `dialog-action-danger`/`dialog-action-commit`; на ≤480px колонки сжимаются + до `44px 44px minmax(0,1fr) auto`, `.hpf-mobile-icon` прячет текстовую метку + и оставляет `aria-label`+`title`. `.danger-confirm-footer` получил тот же + `nowrap`/grid-на-двух-кнопках паттерн. Смоки `smoke_dialog_footer_width.mjs` + (`oneRow`, `iconOnlyDestructive`, ширины 320/360/560, RU/DE) и + `smoke_space_settings_form.mjs` (`discardActionsShareOneRow`) — зелёные при + собственном прогоне. +- **AC9 (climate-toggle после Additional actions).** Блок `toggleRow` для + `useClimateTemp` физически перенесён в `marker-dialog.ts` под + `_renderRadarSection(..., 'additional')` внутри карточки Details. Смок + `climateToggleFollowsAdditionalActions` сравнивает + `Node.DOCUMENT_POSITION_FOLLOWING` относительно заголовка «Additional + actions» — зелёный. Значение/dirty-detection/сохранение `useClimateTemp` не + тронуты (тот же биндинг `d.useClimateTemp`). +- **AC10 (нейтральный `dialog.unsaved` убран).** Убран из всех четырёх + `footerStatus(...)` вызовов (`space-form.ts`, `general-settings-dialog.ts`, + `room-settings-dialog.ts`, `marker-dialog.ts`) и из всех 4 locale JSON. + `dialog.review_fields` (ошибки полей) и dirty-controlled `Save` не + тронуты — смоки `saveEnabledWhenDirtyWithoutDuplicateStatus` (4 файла) + зелёные. +- **AC11–AC13 (радар: единая секция-тумблер, восстановление черновика, + virtual/saved-unsupported).** `renderRadarSection` теперь всегда рендерит + один `toggleRow` в `.radaradditional` внутри Details; конфигурация + раскрывается `:scope > .radargroup` строго под тумблером, вторая секция в + «Основных параметрах» отсутствует (проверено смоками + `radarEntryIsAToggleInAdditionalActions`, `noAutomaticSection`). Ключевое + изменение поведения — `remove()` теперь сохраняет `radar: d.radar` вместо + `radar: null`, так что Off не теряет черновик. **Чем краснеет:** вручную + вернул `radar: null` в `remove()`, пересобрал, прогнал оба смока: + `smoke_device_settings_form.mjs` упал на `radarOffKeepsSessionDraft` и + `radarOnRestoresSessionDraft`; `smoke_radar_setup.mjs` упал на + `savedOffKeepsOriginal`/`savedOnRestoresOriginal`; восстановил файл, + пересобрал, `git status` — чист. `savedUnsupported`/`virtual` ветки кода + структурно не тронуты (тот же `if (savedUnsupported || ...)`), только + обёрнуты в общий `.radaradditional` div с тем же тумблером сверху — + подтверждено смоком `virtualHasNoEntry`. +- **AC16 (release artifacts).** `docs/CHANGELOG.md`+`docs/CHANGELOG.ru.md` + правлены в том же коммите `746147d0` (`User-Visible: yes`), формулировки не + содержат внутренних CSS-классов/имён state-полей/номеров golden-сцен — + прочитано текстом, ссылка на #602 есть в обеих версиях. `docs/USER-GUIDE.md` + и `.ru.md` синхронно обновлены по описанным разделам (Basics/Tap + action/Details/Presence radars) — терминология («Additional actions», + «This is a presence radar», «Review N fields») совпадает с уже + используемой в остальном руководстве, не изобретена заново. +- **Трейлеры.** `746147d0`: `Issue: #602`, `User-Visible: yes` — оба + changelog в этом же коммите (проверено `git show --stat`). `c69d73f1`: + `Issue: #602`, `User-Visible: no`, плюс `Release:`/`Baseline-Reviewed:` — + эти два трейлера не были обязательны для этого конкретного коммита (он не + трогает `demo/golden/baselines/**`, только `docs/images/**`, а + `scripts/validate-commit-provenance.mjs:64` триггерит их только на первый + путь), но лишние трейлеры не нарушают провенанс и отражают реальный + Linux-прогон 35534819172. + +## Отдельно проверенный риск, оказавшийся не дефектом + +Заподозрил, что новая формула `active = !!d.radar && !d.radarRemove` +(`radar-section.ts:88`) включает тумблер «Это радар присутствия» сразу при +открытии диалога не только для уже **сохранённого** радара (что явно +разрешено §6.6 ТЗ), но и для любого **распознанного, но никогда не +сохранённого** устройства (LD2450-профиль) — поскольку `radarDraft()` +(`radar-editor.ts:154-165`) возвращает ненулевой драфт уже при +`recognition.eligible === true`, независимо от наличия `.original`, а +`_openMarkerDialog` (`houseplan-editor-runtime.ts:7586`) вызывает эту функцию +безусловно при каждом открытии. + +Проверил исполнением: поднял демо-стенд (`npm run bundle:sync` + +`demo/serve.mjs`), сконструировал синтетическое устройство с моделью +`HLK-LD2450` и entity-парой `..._target_1_x/_y`, зарегистрированным вне +HA-registry (чтобы `recognizeRadar` не подобрала настоящую модель из +реестра), открыл `_openMarkerDialog`. Результат: `radarEligible: true`, +`radar.enabled: false`, `radarRemove: false`, тумблер `checked: true`, форма +раскрыта — то есть подозрение подтвердилось техническим фактом. + +Но дальше проверил, было ли это поведением, привнесённым этим диффом: +`_openMarkerDialog` и `radarDraft()` **не входят в диапазон +`git diff origin/dev...HEAD`** — они не менялись. Прочитал версию +`radar-section.ts` на `origin/dev` (`git show origin/dev:...`): условие +`if (!d.radar || d.radarRemove) { if (recognition.eligible) {…fieldset +'Настроить'…} return html''; } const radar = d.radar; …полная форма…` при +уже ненулевом `d.radar` (тот же факт, та же функция) точно так же пропускало +ветку «detected, click Настроить» и сразу показывало полную форму — то есть +поведение «распознанный радар раскрывается сразу» уже существовало в `dev` +до #602, просто было завёрнуто во внутренний чекбокс `radar.enabled` вместо +внешнего именованного тумблера. Формулировка §6.6 «для нового/ещё не +настроенного радара повторное включение… возвращает draft» описывает цикл +Off→On внутри сессии, а не начальное состояние впервые открытого диалога, и +не противоречит найденному факту. И `docs/USER-GUIDE.md`/`.ru.md` (раздел +«Presence radars»), поставленные этим же диффом, прямым текстом описывают +именно это как задуманное поведение: «For a recognized presence radar, +Additional actions contains the **enabled** This is a presence radar +switch…» — не гипотеза, а согласованный текст руководства. Вывод: не +регрессия и не находка, а унаследованное и теперь корректно +задокументированное поведение; фиксирую как проверенный и закрытый риск. + +## Чего не проверял + +- **`npm test`/`npm run typecheck`/`npm run build` — не перегонял сам**, + положился на зелёный Validate на точном SHA `c69d73f1` (см. таблицу). + Дешёвые гейты §10.2/§2.10 разрешают это при неизменном материале. +- **Полная матрица `demo/smoke_*.mjs` (258 файлов)** — не гонял. Инструмент + выбора дал одно прямое совпадение (прогнано) и 39 слабых связей по общему + имени `_markerDialog`; полный прогон — предрелизная обязанность (§8), не + ревью. +- **`python -m pytest tests_backend`** — не гонял, diff не касается + `custom_components/**/*.py`. +- **`npm run invariants`** — не гонял: diff не трогает геометрию комнат/стен, + `layout`, `marker.space` или `open_spans`. +- **Performance-профили** — не гонял, не названы в AC и не задеты диффом + (ТЗ §13 подтверждает отсутствие новых циклов рендера/таймеров). +- **Полный `npm run golden:verify` c последующим `accept`** — гейт я + выполнил (см. находку), но фактическое принятие новых baseline-кадров — + действие автора, не ревьюера (роль «ревьюер не правит материал»). +- **Ручная проверка в браузере HA** — не проводилась (среды HA нет); + доказательство — DOM/computed-style смоки плюс golden-диф-изображение, + прочитанное визуально. + +## Материал раунда + +- SHA ветки: `c69d73f1f26b3277fde938a4eb08195c561e46ba` (`issue/602-dialog-polish`) +- Дерево материала: идентично рабочей копии на момент вывода вердикта + (`git status --short` пуст непосредственно перед выводом). +- ТЗ: тело issue #602, зелёный вердикт спецификации r2 + (`Вердикт: зелёный · заход r2 · блокирующих циклов 1/4 · High: 0 · Medium: 0`). + +--- + + + +## Материал раунда + +- Ветка: `issue/602-dialog-polish`, коммит `c69d73f1f26b` — ребейз его осиротит, и это нормально: ниже якоря, которые ребейз не меняет. +- Дерево материала: `ce2edc2506f2f52730a2fa576341f7eabf2dfccf` + ``` + git log --all --format='%H %T' | grep ce2edc2506f2 + ``` +- Тело issue: `8f8081ec29db1c104189682530643bde421d91127170b2c635107c595950a5a9` +- Вердикт конвейера: `yellow` · High 0