From a4667562470dee913dfdc14b57adb7644eeafa07 Mon Sep 17 00:00:00 2001 From: "claude[bot]" <209825114+claude[bot]@users.noreply.github.com> Date: Sun, 20 Sep 2026 19:31:02 +0000 Subject: [PATCH] docs: review document for #602 Issue: #602 User-Visible: no --- docs/reviews/SPEC-REVIEW-602-r1.md | 283 +++++++++++++++++++++++++++++ 1 file changed, 283 insertions(+) create mode 100644 docs/reviews/SPEC-REVIEW-602-r1.md diff --git a/docs/reviews/SPEC-REVIEW-602-r1.md b/docs/reviews/SPEC-REVIEW-602-r1.md new file mode 100644 index 00000000..a3d5621a --- /dev/null +++ b/docs/reviews/SPEC-REVIEW-602-r1.md @@ -0,0 +1,283 @@ +# SPEC-REVIEW-602-r1 + +**Issue:** [#602](https://github.com/Matysh/houseplan-card/issues/602) — «Полировка редизайна диалогов пространства и устройства» +**Этап:** ТЗ на ревью (PROCESS.md §2.4) +**Трек:** full (аналитика #602 называет нарушенные критерии `small`: больше одной поверхности, новый UX-контракт radar-toggle, отдельная touch/mobile проверка — обоснование корректно) +**Заход:** r1 · блокирующих циклов израсходовано 0 из 4 + +Ревьюер работает без контекста реализации, материал — тело issue (раздел `## ТЗ`) и три комментария (аналитика, вопросы владельцу, решения владельца). + +--- + +## Скоуп проверки + +ТЗ описывает точечную полировку четырёх диалогов на общем form-kit («Настройки +пространства», «Общие настройки», «Настройки комнаты», «Настройки устройства»): +удаление избыточных подписей/статусов, выравнивание переключателя и числовых +полей шкал, однострочные футеры с icon-only деградацией на узких экранах, +перенос настройки climate-температуры и замену кнопки объявления radar на +form-kit toggle с раскрытием настроек под ним. + +Проверялось: + +1. Соответствие `docs/SCOPE.md` (персона/job) — task служит J4/J6, поверхность + редакторская, персона — Home admin. +2. Полнота обязательных разделов ТЗ по PROCESS.md §7.1. +3. Однозначность и проверяемость AC1–AC16. +4. Заземление фактических утверждений ТЗ (аналитика + §6) в реальном коде — + не выданы ли догадки за факты. +5. Согласованность с `docs/USER-GUIDE.ru.md` (терминология, уже + задокументированное поведение) и с `docs/TOUCH-SUPPORT.md` (мобильный + контракт редакторов). +6. Процесс: корректно снятый `blocked`, зафиксированные решения владельца по + Q1/Q2, метки (`bug`, `P2`, `polish`, `S4-spec-review`, без `small`/`trivial` + — согласуется с выбором полного трека). + +## Как проверялось + +Ревью велось состязательно: каждое фактическое утверждение ТЗ и аналитики +сверялось с исходником, а не принималось на веру. + +- `docs/SCOPE.md`, `AGENTS.md`, `PROCESS.md` (§1–§9) — прочитаны целиком. +- `docs/TOUCH-SUPPORT.md` — прочитан целиком (мобильный контракт редакторов, + правило «Touch editor: …» из раздела Documentation rule). +- `docs/USER-GUIDE.ru.md` — прочитаны разделы про футер/`dialog.unsaved` + (строки 395–414), про radar (1158–1179), про climate-бейдж (1365–1383). +- i18n: подтверждено существование и точный текст ключей + `space.title_hint`, `space.scale_hint`, `marker.name_hint`, + `dialog.unsaved`, `dialog.review_fields`, `radar.declare`, + `dialog.discard_title/confirm/keep` в `src/i18n/settings/ru.json` и + `src/i18n/ru.json`. +- Код диалогов: `src/styles/form-kit.styles.ts` (геометрия тумблера, `4em`/ + `5.8em` полей, `padding-right: 122px`/`125px` подписей шкалы), + `src/editors/marker-dialog.ts`, `src/editors/radar-section.ts`, + `src/radar-editor.ts`, `src/editors/room-settings-dialog.ts`, + `src/editors/general-settings-dialog.ts`, `src/hp-confirm.ts` — прочитаны + целиком/выборочно по каждому пункту §6 ТЗ. +- Предшественники: тела issue #591, #600 (через `gh issue view`) — сверка, + что мобильный контракт диалогов (320/390/480/560/768/1280/1920, + «touch остаётся best effort по SCOPE») уже установлен в #600 и не вводится + заново. +- Поиск прецедентов формулировки «Touch editor: …» по корпусу + `docs/reviews/*` (`SPEC-REVIEW-563-r1.md`, `SPEC-REVIEW-454-r1.md`) — + чтобы откалибровать, когда эта метка обязательна. +- Файловая проверка: `docs/CHANGELOG.md`, `docs/CHANGELOG.ru.md`, + `docs/USER-GUIDE.md` существуют; корневого `CHANGELOG.md` в репозитории + нет (`ls` вернул «No such file or directory»). +- Гейты кода в этом раунде не гонялись: этап — ревью ТЗ, продуктовый код не + менялся, для стадии `S4-spec-review` PROCESS.md §8/§10.4 не требует + прогона `typecheck`/`test`/`build` — они относятся к код-ревью (§2.7) и + реализации (§2.6). Это явное решение, а не пропуск: гонять их не по чему, + диапазон коммитов на ветке кода не менялся. + +## Находки + +### Medium (в скоупе) — неоднозначность «уже настроенного» radar-toggle + +**Файл:** тело issue #602, раздел `## ТЗ` §6.6 (радар toggle). + +**Формулировка ТЗ:** «Включение создаёт/возвращает radar draft и сразу +раскрывает radar-настройки непосредственно под тумблером в «Дополнительных +действиях»… **Для уже настроенного радара тумблер отображается включённым.** +Выключение сворачивает настройки и ставит `radarRemove=true`; фактическое +удаление происходит только после Save.» + +**Что проверено по коду.** `renderRadarSection` (`src/editors/radar-section.ts`) +рендерит toggle/кнопку `radar.declare` только при +`manualEntry = !savedUnsupported && bindingMode !== 'virtual' && (!d.radar || d.radarRemove) && !recognition.eligible` +(строка 77). `recognizeRadar()` (`src/radar-editor.ts:105-127`) для ЛЮБОГО +валидного сохранённого radar-конфига (независимо от того, был ли он объявлен +вручную через `radar.declare` или распознан по модели устройства) возвращает +`eligible: true, reason: 'saved'` — единственный признак, отличающий +auto-recognized от manually-declared, это **значение профиля** (`profile`), +а не отдельное поле происхождения: в схеме `MarkerRadar`/`RadarEditorDraft` +такого поля нет (`src/radar-editor.ts:7-49`, `src/radar-model.ts`). + +Следствие: как только у устройства **уже есть сохранённый** radar-конфиг +(любого происхождения), `recognition.eligible` = `true` → `manualEntry` = `false` +→ ветка `placement === 'additional'` возвращает `html\`\`` (ничего) → диалог +показывает не toggle, а старую карточку `
` с `legend` +`radar.title` («Присутствие на плане») прямо в «Основных параметрах» — +то есть **ровно то поведение, которое задача называет дефектом** (форма +радара не там, где ожидает пользователь), просто для другого триггера +(повторное открытие диалога, а не текущая сессия объявления). + +**Не хватает:** явного ответа, что означает «уже настроенный» — +(a) радар, объявленный **в рамках текущей открытой сессии диалога** +(`d.radar` уже создан через `begin()`, Save ещё не нажат) — тогда для этого +не нужна миграция данных, но текущая реализация `manualEntry` (условие +`!d.radar`) должна измениться так, чтобы toggle не исчезал сразу после +создания драфта, иначе он «мигает» и снова показывает форму сверху — это, +похоже, и есть цель задачи, но словами о «правках `manualEntry`» ТЗ не +говорит и оставляет это угадывать; или +(b) радар, **сохранённый на сервере в прошлой сессии** и подгруженный при +повторном открытии диалога — тогда для manually-declared радаров нужно +различать происхождение (например, по `profile !== 'esphome_ld2450_v1'`), +чтобы не сломать уже принятую и не входящую в скоуп («не входит: +…распознавания поддерживаемых моделей радара», §5) карточку `radar.title` +для по-настоящему распознанных устройств LD2450. Такое различение — решение, +видимое пользователю (что именно он увидит, открыв диалог заново), а не +техническая деталь: PROCESS.md §7.1 относит подобные вопросы к продуктовым. + +**Почему это не High.** ACn (AC11, AC12) описывают только поведение внутри +одной открытой сессии (создание/восстановление/удаление до Save) и +проходят проверку независимо от выбранной трактовки; сценарий повторного +открытия ранее сохранённого вручную-объявленного радара **не выражен ни +одним AC явно** — то есть задача не станет невыполнимой при любой из двух +трактовок, но реализация без уточнения рискует либо (a) не решить часть +исходной жалобы (форма при повторном открытии всё ещё «не там»), либо (b) +выйти за заявленный §5 «не входит» и потребовать различения происхождения +конфигурации, о котором ТЗ не упоминает вовсе. + +**Предлагаемая правка (пример, не решение за автора):** одно предложение в +§6.6 — «Для устройства, у которого radar уже сохранён из предыдущей сессии, +диалог продолжает использовать существующую карточку «Присутствие на плане» +без изменений (см. §5); toggle отражает состояние **только внутри текущей +открытой сессии диалога**, начиная с момента объявления/восстановления +драфта и до Save/закрытия.» — или явное продуктовое решение в обратную +сторону, если владелец действительно хочет перенести и уже сохранённые +вручную-объявленные радары под toggle. + +### Low (в скоупе, не блокирует) — неверная пара changelog-файлов + +**Файл:** тело issue #602, §10 «Затрагиваемые модули» и §16 «Release +artifacts». + +ТЗ дважды называет пару `CHANGELOG.md` + `docs/CHANGELOG.md`. В репозитории +корневого `CHANGELOG.md` не существует (`ls CHANGELOG.md` → +No such file or directory); настоящая пара — `docs/CHANGELOG.md` + +`docs/CHANGELOG.ru.md` (так и в `AGENTS.md`: «`User-Visible: yes` requires +edits to **both** changelogs — `docs/CHANGELOG.md` и `docs/CHANGELOG.ru.md`», +и в гейте `scripts/validate-commit-provenance.mjs:64`, который проверяет +именно эти два пути). RU-changelog в списке ТЗ вообще не назван. + +**Почему не блокирует.** Коммит-хук/`provenance` job проверяют реальные пути +(`docs/CHANGELOG.md`, `docs/CHANGELOG.ru.md`) независимо от текста ТЗ — +механически пропустить обновление RU-changelog не получится, коммит с +`User-Visible: yes` без него будет отклонён. Опечатка не создаёт риска для +AC16 (доказательство — «review»), но может на секунду сбить с толку человека, +сверяющего release-артефакты по тексту ТЗ. + +**Решение ревьюера:** снимается с записью — правка тривиальна (заменить +`CHANGELOG.md` на `docs/CHANGELOG.ru.md` в двух местах), настоящий гейт уже +защищает от практического следствия; отдельного цикла ради одной опечатки не +требуется, но автор может поправить текст заодно с ответом на находку выше. + +## Что проверено и признано корректным + +- **Обязательные разделы §7.1** — все присутствуют: сценарий (§1), что + человек видит до/после (§2), проблема (§3), скоуп/не-скоуп (§4/§5), + контракт (§6), UX/доступность (§7), данные и миграция (§8), i18n (§9), + AC1–AC16 с «Свидетелем» (§11), план автотестов (§12), риски (§14), откат + (§15), release-артефакты (§16). +- **Продуктовые вопросы отделены от технических корректно.** Q1 (icon-only + footer на узких экранах) и Q2 (семантика выключения radar-toggle для уже + сохранённого радара **внутри той же сессии**) — обе действительно + продуктовые («что видит/делает человек»), обе закрыты явным решением + владельца, `blocked` снят по факту. Технические детали (i18n-ключи можно + оставить или убрать по результатам поиска потребителей; какой модуль + держит guard) оставлены автору — соответствует PROCESS.md §7.1. +- **Ни одна фактическая формулировка ТЗ/аналитики не оказалась догадкой, + выданной за факт** — все проверенные утверждения подтвердились чтением + кода: + - фиксированные ширины `4em`/`5.8em` и `padding-right: 122px/125px` + (`src/styles/form-kit.styles.ts:388,421-422,496`) — источник дефектов + прозрачности/шкалы назван верно; + - `useClimateTemp`-тумблер сейчас в карточке света + (`marker-dialog.ts:501-509`, `marker.card_light`) и должен переехать в + `marker.card_details` после подраздела `radar.additional_actions` + (`marker-dialog.ts:842`, `radar-section.ts:82-89`) — подраздел реально + существует под этим именем и рендерится для типичного (не virtual, + не сохранённо-неподдерживаемого, не auto-recognized) устройства, в т.ч. + climate-bound, поскольку `_bindingHasClimate` (`houseplan-card.ts:5621-5623`) + для `binding === 'virtual'` всегда `false` — конфликта climate+virtual + не возникает; + - разрыв «кнопка в Additional actions → форма в Main params» воспроизведён + по коду именно так, как описан в §3 (два вызова `_renderRadarSection` с + разным `placement`, `marker-dialog.ts:319,842`); + - `hp-confirm` (`src/hp-confirm.ts`) — единственный компонент подтверждений + во всём приложении, всегда ровно две кнопки; риск «Confirm используется + не только discard» в §14 корректно закрыт этим наблюдением — новый + однорядный layout не может сломать иной by-design confirm, потому что + другого layout не существует; + - все процитированные i18n-строки (`space.title_hint`, `space.scale_hint`, + `marker.name_hint`, `dialog.unsaved`, `dialog.review_fields`, + `radar.declare`, `dialog.discard_title/confirm/keep`) существуют + дословно в `ru.json`/`settings/ru.json`. +- **Touch/mobile контракт не вводится заново.** Мобильная ширина от 320 px + для этих диалогов и статус «touch остаётся best effort по SCOPE» уже + зафиксированы в #600 (подтверждено чтением тела #600: таблица ширин + 320/390/480/560/768/1280/1920 и явная фраза про best effort). #602 — + точечное исправление внутри уже принятого контракта, не новая + editor-фича; по прецеденту `SPEC-REVIEW-563-r1.md` («задача не вводит + новую editor-фичу, поэтому явная метка «Touch editor: …» не требуется») + отдельная декларация `Touch editor: …` не обязательна. Icon-only-переход + Copy/Delete — новая деталь по сравнению с #600, но она строго внутри уже + описанного «best effort»-контракта и решена продуктовым вопросом Q1, а + не угадана. +- **Область задачи (4 диалога) не раздута.** Room/General settings попали в + скоуп не произвольно: оба реально импортируют общие хелперы `toggleRow`/ + `rangeEnds`/`footerStatus` из `src/editors/form-kit.ts` + (`room-settings-dialog.ts:19-22`, `general-settings-dialog.ts:13-16`) — + общий CSS-баг действительно задевает все четыре потребителя, объединение + оправдано тем же корнем причины, а не удобством. +- **Соответствие SCOPE.md.** Job — J4/J6, поверхность — только editor-диалоги + (админ), View/kiosk не затронуты; лок-инвариант и out-of-scope список не + задеты. +- **Метки/процесс.** `S4-spec-review` без `small`/`trivial` соответствует + названному в аналитике отказу от лёгкого трека; `blocked` корректно снят + после ответов владельца в третьем комментарии. + +## Чего не проверял + +- **Golden/скриншот-покрытие Room/General диалогов.** План автотестов (§12 + п.4) явно называет «минимум» голден-сцен для Space/Device/Confirm, но не + называет отдельных сцен для Room/General, хотя оба используют те же + `rangeEnds('50%','300%')` (подтверждено: `room-settings-dialog.ts:276`). + Это оставлено на усмотрение автора («минимум» допускает добавление) и не + формулирует нового продуктового решения — не поднимаю как находку, но + ревьюер кода должен свериться, что регресс в Room/General действительно + покрыт хотя бы layout-смоком, если не голденом. +- **Точная реализация центрирования шарика тумблера** (какая именно строка + CSS изменится) — не разбирал до пиксельной арифметики box-sizing, + поскольку ТЗ не обязано называть implementation-решение, а описывает + наблюдаемый результат («визуально центрирован… одинаковые внутренние + отступы») — тестируемо через layout-смок и golden, этого достаточно на + этапе ТЗ. +- **Автотесты, гейты `typecheck`/`test`/`build`/`golden:verify` и т.п.** — + не запускались: продуктовый код в этом раунде не менялся, предмет + ревью — исключительно текст ТЗ. Они относятся к код-ревью (§2.7) после + реализации. +- **Полный текст docs/USER-GUIDE.md (EN)** — сверял только русскую версию + построчно по конкретным терминам; английскую версию не читал целиком, + считаю её производной (перевод) и не источником терминологии по + правилам AGENTS.md. + +## Вердикт + +High: 0 · Medium: 1 (в скоупе — уточнение семантики «уже настроенного» +radar-toggle, §6.6) · Low: 1 (в скоупе, снят с запиской — неверная пара +changelog-файлов в §10/§16). + +Оба найденных дефекта чинятся правкой текста ТЗ (одно-два предложения), +без изменения архитектуры или AC. Medium-находка не позволяет поставить +зелёный вердикт по правилу §2.4: «Medium в скоупе задачи чинится в текущем +issue: без High это жёлтый вердикт, автор правит ТЗ, фикс проходит +повторный цикл.» + +**Вердикт: жёлтый · заход r1 · блокирующих циклов 1/4 (полный трек, лимит +ревью ТЗ — 4) · High: 0 · Medium: 1 → в задаче** + +--- + + + +## Материал раунда + +- Ветка: `dev`, коммит `3701c7bd201d` — ребейз его осиротит, и это нормально: ниже якоря, которые ребейз не меняет. +- Дерево материала: `3fe0f0cd2ce99828382138829f2d4ca0eccc9cce` + ``` + git log --all --format='%H %T' | grep 3fe0f0cd2ce9 + ``` +- Тело issue: `345487371ab35ce7a540ebb03b9eeb09cda56792a7322d0783036edb5866f78e` +- Вердикт конвейера: `yellow` · High 0