diff --git a/docs/reviews/SPEC-REVIEW-600-r1.md b/docs/reviews/SPEC-REVIEW-600-r1.md new file mode 100644 index 00000000..844ecf62 --- /dev/null +++ b/docs/reviews/SPEC-REVIEW-600-r1.md @@ -0,0 +1,280 @@ +# SPEC-REVIEW-600-r1 — #600: диалоги настроек разошлись с макетами #591 + +- Issue: [#600](https://github.com/Matysh/houseplan-card/issues/600) +- Этап: `spec` (PROCESS.md §2.4) +- ТЗ под ревью: тело issue #600, раздел `## ТЗ` (коммит `94c9d8ad`, ветка + `issue/600-settings-dialogs`, `HEAD` детач на этом SHA) +- Трек: полный (аналитика #600 явно отказалась от лёгкого — сложность 8/10 > 3, + четыре поверхности, новый UX-контракт Save/dirty, влияние на perf-потолок) +- Заход: **r1**, блокирующих циклов израсходовано **0/4** +- Вердикт: **зелёный** + +## Скоуп ревью + +Материал этапа — тело issue #600 (`## ТЗ`) и все 9 комментариев: разбор архива, +две записи аналитики (S2), пакет из одиннадцати развилок с умолчаниями автора, +решения владельца по Q1–Q11, подтверждение выполнения AC0. Плюс коммит +`94c9d8ad` (`docs: положить дизайн-референс диалогов настроек в репозиторий`), +который уже лежит в дереве на момент ревью и добавляет +`docs/design/600-settings-dialogs/` (`SPEC.md`, `IMPLEMENTATION-GUIDE.md`, +`OPEN-POINTS.md`, `field-maps/FIELD-MAP-space.md`, `reference/`, +`screenshots/`, `README.md`, `ACCEPTANCE.md`) — по решению владельца эти +документы **являются частью ТЗ**, а не приложением к нему, поэтому проверены +как нормативный материал, а не как справка. + +Референсный контекст, прочитанный до вердикта: `docs/SCOPE.md`, `AGENTS.md`, +`PROCESS.md` §1–§9 (в частности §2.2–§2.4, §2.6, §7.1, §7.2), тело и все +комментарии #600, `docs/USER-GUIDE.ru.md` (для проверки терминологии +переименований Q6 — сверка не потребовалась предметно, переименования не +затрагивают уже описанные в гайде формулировки настроек, кроме текстов, +которые сама задача обязана обновить как release-артефакт). + +## Как проверялось + +Ревью ТЗ, не ревью кода: рабочая копия уже на `94c9d8ad`, продуктовый код +(`src/**`) в диапазоне не менялся — только `docs/design/**`. Прогон +`npm test`/`npm run build` не требовался: класс изменения C (документация), +это подтверждено самим коммитом и `git show --stat 94c9d8ad`. + +| Проверка | Результат | +|---|---| +| Обязательные разделы ТЗ по PROCESS.md §7.1 | все присутствуют, см. таблицу ниже | +| Каждый AC0–AC13 — однозначность + способ доказательства | см. «AC» ниже | +| Технические утверждения ТЗ против реального кода | см. «Верификация фактов» | +| Прослеживаемость Q1–Q11 → решения владельца → правки ТЗ | сверено построчно | +| `docs/design/600-settings-dialogs/` реально существует и соответствует заявленному в AC0 | `ls`, сверка секций `SPEC.md` | +| Признаки «догадка выдана за решение» без пометки предположения | целевой поиск | +| Продуктовые вопросы владельцу вместо технических | сверено — все 11 вопросов продуктовые или лицензионные | + +### Обязательные разделы ТЗ (PROCESS.md §7.1) + +| Раздел | Есть | Где | +|---|---|---| +| Сценарий | да | «Сценарий» | +| Что человек увидит до/после | да | «Что человек увидит до и после» | +| Проблема | да | «Проблема» | +| Скоуп и не-скоуп | да, с явным перечнем файлов | «Скоуп / не-скоуп» | +| Контракт поведения | да, десять пунктов К1–К10 | «Контракт поведения» | +| UX | да — распределён по «Набору контролов», «Поведению на разных экранах», «Составу диалогов», а не под одним заголовком | см. ниже | +| Модель данных и миграция | да | «Нет» | +| i18n | да | «i18n» | +| AC1…ACn с доказательством | да, AC0–AC13, у каждого «чем доказывается» и «чем краснеет» | «Критерии приёмки» | +| План автотестов | да | «План автотестов» | +| Риски | да, семь пунктов | «Риски» | +| Откат | да | «Откат» | +| Release-артефакты | да | «Release-артефакты» | + +UX-раздел не собран под одним заголовком «UX», но материально присутствует: +«Набор контролов» (таблица примитивов с числами из §3.1 источника), +«Поведение на разных экранах (Q7)» (три полосы, семь контрольных ширин) и +«Состав диалогов» (ссылка на §4–§7 `SPEC.md`) вместе покрывают то же +содержание, что требует §7.1. Как и в прецеденте SPEC-REVIEW-89-r1 (находка +L1, риски без отдельного заголовка), рассеянность по разделам не делает текст +неисполнимым или непроверяемым — единственный критерий блокировки на этом +этапе. Отмечаю как Low-наблюдение, не как находку, требующую действия. + +### Верификация фактов + +Основная часть работы — задача сама объясняет, что предыдущие три захода +(#594, #598, #599) провалились именно из-за непроверенных утверждений +(«К7 выполнен, значит референс не нужен» и т.п.), поэтому каждое конкретное +техническое утверждение ТЗ сверено с текущим деревом на `94c9d8ad`, а не +принято на слово: + +- **Дефолтная ширина диалогов 500 px, а не 560.** Подтверждено: + `src/hp-dialog.ts:169` — `width: min(var(--hp-dialog-wide-width, 500px), 94vw)`. +- **`data-kind` уже проставлен на всех четырёх диалогах.** Подтверждено: + `grep -l data-kind src/editors/*.ts` возвращает все четыре файла + (`space-settings-dialog.ts`, `general-settings-dialog.ts`, + `room-settings-dialog.ts`, `marker-dialog.ts`). +- **Три сегмента уже стоят** (`gs-bg-mode`, `space-bg-mode`, + `space-zero-wall-style`). Подтверждено по исходникам этих трёх диалогов. +- **Двенадцать смоков держатся за `.srcrow`/`.dispsection`.** Подтверждено + ровно: `grep -rln "srcrow|dispsection" demo/smoke_*.mjs` даёт 12 файлов — + число из аналитики S2 не устарело на момент ревью. +- **Потолок ленивого графа: `LAZY_EDITOR_GZIP_CEILING = 222 900`, + `LAZY_GRAPH_CEILING_BAND = 2 000`.** Подтверждено дословно в + `scripts/bundle-budget.mjs:393-394`. +- **`editor.loading` = «Загружаем редактор…» в `ru.json`.** Подтверждено — + прямой дефект #9 описан точно, ключ существует и требует правки текста, не + переименования ключа; `editor.loading_aria` рядом и намеренно не трогается. +- **Замороженные фикстуры панели существуют** (`test/fixtures/summary-panel-editor.css`, + К9/AC11) — файл на месте. +- **Мутанты, названные существующими** (`state-callout-hidden-under-help`, + `dialog-card-loses-its-heading`) действительно есть в + `scripts/mutation-registry.mjs`; два новых (`dialog-control-writes-to-a-neighbour-key`, + `dialog-segment-drops-radio-semantics`) корректно не найдены — план + автотестов честно относит их к работе задачи, а не выдаёт за готовые. + Мутант «на инверсию слоёв» и «на Save активен без изменений» из плана + автотестов также ещё не существуют — ожидаемо, план их и не называет + существующими. +- **Смоки, названные по имени в AC6/AC7/AC5** (`smoke_esc_dialogs`, + `smoke_help_affordance`, `smoke_color_picker_consumers`) — все три файла + существуют в `demo/`. +- **`docs/design/600-settings-dialogs/` реально содержит заявленное в AC0**: + `README.md`, `ACCEPTANCE.md`, `SPEC.md` (273 строки, секции §1–§12 + + «Принятые технические предположения» — совпадает с описанием), `field-maps/FIELD-MAP-space.md`, + `reference/`, `screenshots/` — на месте. AC0 на дату ревью уже выполнен, что + и заявлено последним комментарием. +- **§4.2/§5.2/§6.2/§7.2 архивного `SPEC.md`** (полные карты «контрол → ключ + состояния», на которые ссылается К1) действительно существуют как + подразделы «Состав карточек» / «Состав и правила» внутри §4–§7 — ссылка ТЗ + не бьёт в пустоту. +- **Q1–Q11 → решения владельца → правки ТЗ** сверены построчно: Q2 (образцы + карточки остаются) и Q5 (только раскладка, без нативного пикера) — оба + места, где решение владельца разошлось с умолчанием автора и с `SPEC.md` + оригинала, — оба явно и без потери смысла отражены в разделе «Решения + владельца 20.09» и продублированы в контракте (К... нет отдельного пункта, + но упомянуты в «Набор контролов» и AC5/AC1). Ни одно решение владельца не + потеряно и не переврано при переносе в нормативный текст. +- **Вопросы владельцу все продуктовые или лицензионные**, ни один не является + техническим, замаскированным под продуктовый (Q11 — лицензия на перенос + материалов, тоже не техническая деталь реализации, а вопрос прав на + контент). Технические развилки (модуль `form-kit` vs новый + `settings-form-view.ts`, имена классов/ключей) автор решил сам и вынес в + «Принято предположительно», как и предписывает §7.1. + +Ни одного утверждения о текущем состоянии кода, не подтверждённого чтением +дерева на `94c9d8ad`, не найдено. + +## Находки + +Ни одной **High**. Ни одной **Medium**. Одна **Low** — разобрана и снята здесь +же с записью. + +### L1 — регресс-гарантия для существующих кнопок действий не названа отдельным AC + +Архивный `SPEC.md` (§11, AC8: «Copy, Delete, Hide, Skip, Keep as walls, +Export/Import, Optimize, Read ZHA / Update map, Attach, Pin, Open in HA +работают как до задачи») содержал отдельный критерий приёмки на то, что +существующие кнопки-действия внутри четырёх диалогов не сломаются при +перестройке карточек и футера вокруг них. При переносе в тело issue этот AC +не появился ни как отдельный пункт, ни как явно объявленное отступление (в +отличие от Q2/Q5, которые в тексте прямо помечены «против умолчания автора»). +Формально это расхождение с материалом, который сама задача называет частью +ТЗ, без объявленной причины — ровно то, чего требует избегать формулировка +«расхождение оформляется ссылкой на пункт документа, а не „на глаз“». + +Проверка по коду показала, что фактический риск невысок: клик-обработчики +этих кнопок (`_readZha`, `openSpaceCopyDialog`/`_deleteSpace`, +`_saveRoom`/`canSaveNew`, обработчик Pin в `marker-dialog.ts:546-549`) +находятся в стороне от заменяемых примитивов (радиосписок → сегмент, чекбокс +→ строка-тумблер и т.д.) — редизайн трогает только обёртку разметки, а не +привязку событий. Часть функций уже защищена независимыми тестами вне +двенадцати смоков-опор, которые будут сознательно переписаны +(`test/space-copy-runtime.test.mjs`, `test/space-dialog.test.mjs`, +`demo/smoke_danger_confirmation.mjs`, `test/zigbee-topology.test.mjs`, +`demo/smoke_zigbee_topology_hover.mjs`, `demo/smoke_space_copy.mjs` и другие +`smoke_optimize_*`/`smoke_*_reconciliation`). Явного smoke-свидетеля для +«Pin» (`marker.icon_pin_auto`) и «Attach»/«Keep as walls» в create-режиме не +нашлось, но это уже существующий пробел покрытия, не новый, и он не входит в +скоуп задачи (задача не трогает эту логику, только оборачивающую разметку). + +**Вердикт по находке:** снимается с записью. Отсутствие своего номера AC не +делает ТЗ неисполнимым — блокирующий критерий этапа ТЗ; AC13 («Полный набор +зелёный: `npm test`, `npm run gate:small`») и явное требование плана +автотестов «в хендоффе перечисляется, какой смок и почему» правкуется под +каждый из двенадцати переписываемых смоков — при выполнении этого требования +регресс в перечисленных кнопках будет пойман тем же механизмом, каким ловится +любой другой регресс со смены разметки. Рекомендация автору (не обязательна +для этой редакции, годится на код-ревью или в хендофф): явно перечислить эти +кнопки как часть AC4 или как отдельную строку плана автотестов — «действия +Copy/Delete/Skip/Keep as walls/Export/Import/Optimize/Read ZHA/Update +map/Attach/Pin/Open in HA не меняют поведение, только окружение» — так +код-ревью получит именованный критерий вместо необходимости реконструировать +его из архивного документа. + +## Что проверено и корректно + +- Все обязательные разделы §7.1 присутствуют по содержанию (см. таблицу + выше); UX рассредоточен, но не потерян. +- AC0–AC13 пронумерованы без пропусков, у каждого явно указано и «чем + доказывается», и «чем краснеет» — сильнее минимального требования §2.5 + («указано, чем доказывается»). +- AC0 на дату ревью уже фактически выполнен и проверен: `docs/design/600-settings-dialogs/` + на месте, содержит заявленные документы, референс не участвует в бандле + (утверждение коммита `94c9d8ad`, не перепроверялось повторным запуском + `bundle-tree` — см. «Чего не проверял»). +- Одиннадцать открытых точек (`OPEN-POINTS.md`) корректно доведены до + владельца одним пакетным комментарием, каждая с предложенным умолчанием и + рекомендацией автора — по форме и духу PROCESS.md §7.1 («вопросы задаются + одним комментарием, пачкой, каждый — что неясно / что изменится / вариант + по умолчанию»). Отклонения автора от умолчаний оригинального `SPEC.md` + (Q1 fallback, Q4, Q7 — «решено молча и неправильно» на прошлом заходе) + честно возвращены владельцу как открытые вопросы, а не оставлены + свершившимся фактом — именно то, что было нарушено в #598/#599. +- Два места, где решение владельца разошлось с рекомендацией/умолчанием + автора (Q2 — образцы карточки остаются; Q5 — только раскладка без + системного пикера), в тексте ТЗ прямо помечены как расхождение с `SPEC.md` + автора — выполнено требование «расхождение оформляется ссылкой на пункт + документа». +- Контракт поведения (К1–К10) покрывает данные (К1), инверсию только в UI + (К2), наследование (К3), скрытые прототипом ветки (К4), видимость + сообщений о состоянии (К5), единственный скролл (К6), доступность (К7), + граф загрузки (К8), заморозку панели (К9) и Save/закрытие (К10) — то есть + ровно те риски, которые аналитика #591/#598/#600 определила как причину + прошлого провала (ложный паритет, потеря `rhint`-текстов, скрытие + callout'ов под «?», заморозка смоков вместо честной проверки). +- Классы риска §2.6 явно пройдены в самом ТЗ (async — н/п, данные и права — + К1/AC3, геометрия — н/п, визуал — главный риск с AC10/AC1, объём/perf — + AC12, host/input — К7/AC2/AC9) — облегчает будущий DoR и код-ревью. +- Технические решения (структура `form-kit` вместо нового модуля, имена + классов/ключей, механика dirty-состояния) корректно отнесены в «Принято + предположительно, поменять свободно» и не заданы владельцу как вопросы — + ни одного технического вопроса в пакете Q1–Q11 не найдено. +- Провенанс архива-источника (`SHA-256 06f07fc2…`) сверен дважды (аналитик + → автор архива в #591, затем владелец лично передал архив 20.09) и + зафиксирован в `docs/design/600-settings-dialogs/README.md`; вопрос + лицензии (Q11) закрыт владельцем явно («сотрудник, внешний грант не + нужен»), а не предположен. +- Порядок работ (Space → General → Room → Device, один issue, парные отчёты + по каждой серии) прослеживается до решения владельца по Q9 без искажения. + +## Чего не проверял + +- Гейты `npm test`/`npm run build`/`npm run bundle:budget` не запускались — + на диапазоне этого ревью нет изменений класса A/B/D (только + `docs/design/**`), запуск проверял бы пустое множество; численные значения + потолка (`222 199`/`222 900`/`2 000`) сверены чтением исходника, а не + пересчётом бандла. +- Не перезапускал `bundle-tree`/`npm run bundle:sync` для повторной проверки + утверждения «в бандл из референса не попадает ничего» (заявлено в + предыдущем комментарии «AC0 выполнен») — доверился заявленному результату + зелёного `repo-hygiene` и полного набора «2793 pass, 0 fail», это будет + ре-проверено гейтами код-ревью на реализации. +- Не читал `reference/` (9 модулей, 388 строк CSS) построчно как код — по + решению владельца от 18.09 он не исполняется и используется только как + текст-источник чисел; численные значения из §3.1 `SPEC.md`, которые ТЗ + переиспользует, сверены с таблицей `SPEC.md`, а не с самим CSS референса. +- Не оценивал реализуемость конкретных CSS-чисел (радиус 11, gap 16 и т.п.) + на реальных темах HA — это предмет код-ревью и golden-эталонов, а не текста + спецификации. +- Не проверял, действительно ли `smoke_settings_dialog_cards.mjs` и другие + файлы, упомянутые в §10 `SPEC.md` как подлежащие правке, содержат ровно те + сценарии, которые там описаны — это будет предметом код-ревью, когда + правки появятся, а не ревью текста ТЗ. + +## Вердикт + +**Зелёный · заход r1/4 · High: 0 · Medium: 0 → нет новых issue.** + +ТЗ проверяемо, каждый AC0–AC13 имеет однозначный способ доказательства, +десять продуктовых развилок закрыты явными решениями владельца без потери +смысла при переносе в нормативный текст, а технические утверждения о +текущем состоянии кода выдержали построчную сверку с деревом на `94c9d8ad`. +Единственная находка (L1) снята с записью и рекомендацией на будущее. +Следующий статус — «Готово к разработке» (`S5-ready`). + +--- + + + +## Материал раунда + +- Ветка: `issue/600-settings-dialogs`, коммит `94c9d8addf3f` — ребейз его осиротит, и это нормально: ниже якоря, которые ребейз не меняет. +- Дерево материала: `84be0429b2b9a13a50086986b601bdd88bac4317` + ``` + git log --all --format='%H %T' | grep 84be0429b2b9 + ``` +- Тело issue: `2871bca5cd192e74cd862e2a4db109f96d559aecf77c8c2032369475e76aa1c6` +- Вердикт конвейера: `green` · High 0