diff --git a/docs/reviews/SPEC-REVIEW-476-r1.md b/docs/reviews/SPEC-REVIEW-476-r1.md new file mode 100644 index 00000000..5a4afd81 --- /dev/null +++ b/docs/reviews/SPEC-REVIEW-476-r1.md @@ -0,0 +1,187 @@ +# SPEC-REVIEW-476-r1 + +- **Issue:** https://github.com/Matysh/houseplan-card/issues/476 +- **Этап:** ТЗ на ревью (PROCESS.md §2.4), заход r1, блокирующих циклов израсходовано 0/4 +- **Артефакт ТЗ:** `docs/specs/476-color-picker-ok.md` (ветка `issue/476-color-picker-ok`, HEAD `acd28558`) +- **Трек:** полный (обоснование в аналитике: новый наблюдаемый UX-контракт завершения) +- **Ревьюер:** Claude (роль «ревьюер ТЗ», отдельная от автора — Codex) + +## Скоуп проверки + +Читал в порядке из системного промпта: `docs/SCOPE.md`, `PROCESS.md` (§1–§10.4), +`AGENTS.md`, тело issue #476 и все 5 комментариев (аналитика, продуктовые +вопросы, решение владельца, занятие, хендофф автора), `docs/USER-GUIDE.ru.md` +(раздел про образец цвета/палитру), и сам ТЗ `docs/specs/476-color-picker-ok.md` +целиком. Канонического документа отдельно под color-picker нет — ближайший +контракт живёт в `USER-GUIDE.ru.md` и в самом компоненте. + +Задача — только документация (класс C), продуктовый код не менялся: рабочее +дерево уже содержит эту ветку, что позволило свести claims ТЗ с реальным +кодом `src/hp-color-opacity.ts`, `src/houseplan-card.ts`, `src/i18n/*.json`, +`demo/golden/matrix.mjs`, `demo/golden/baselines/baselines-index.json` вместо +того, чтобы верить пересказу автора. + +## Как проверялось + +Это ревью ТЗ, а не код-ревью: гейты `typecheck`/`test`/`build` к диффу класса C +не относятся и не гонялись (диф — `docs/specs/476-color-picker-ok.md` + +`docs/specs/README.md`, оба класса C). Вместо этого каждое фактическое +утверждение ТЗ о текущем поведении/API сверено с реальным исходником: + +| Утверждение ТЗ | Проверено по | Результат | +|---|---|---| +| `hp-color-opacity` эмитит `hp-color-opacity-change` с `{color, opacity}` сразу при live-изменении | `src/hp-color-opacity.ts:682-698` (`_emit`) | подтверждено | +| Закрытие через trigger/outside/`Escape`/disconnect уже существует и не откатывает live-значение | `_toggle`, `_outsidePointerDown`, `_keyDown`, `disconnectedCallback` | подтверждено | +| `_closePicker(refocus, reason)` — единый lifecycle-путь для всех close reasons | `_closePicker` (`hp-color-opacity.ts:436-456`) | подтверждено, сигнатура `(refocus=false, reason='exclusive')` | +| `ColorPickerLabels` сейчас без поля `confirm`, единый источник — `_colorPickerLabels` в `houseplan-card.ts` | `hp-color-opacity.ts:8-15`, `houseplan-card.ts:5927-5936` | подтверждено; все потребители (`decor-image-editor.ts`, `houseplan-editor-runtime.ts`, `houseplan-onboarding-runtime.ts`) читают именно этот геттер — правка одной точки закрывает всех | +| `showOpacity=false` потребители существуют (Glow, ripple) | `grep showOpacity=\${false}` → 5 мест, включая ripple/Glow | подтверждено | +| Ключи i18n уже используют плоский `color_picker.*` формат в en/ru/de/fr | `src/i18n/{en,ru,de,fr}.json:18-23` | подтверждено, добавление `color_picker.confirm` последовательно с существующим стилем | +| Пять названных golden-сцен существуют и покрывают заявленные комбинации (RU/EN, light/dark, desktop/touch, opacity/color-only) | `demo/golden/matrix.mjs:940-984`, `baselines-index.json:152-164` | подтверждено; `device-ripple-color-popover-mobile-ru` — реальный `showOpacity=false` кейс (ripple) | +| `docs/USER-GUIDE.ru.md` уже документирует, что изменения в палитре — «черновик до сохранения родительского диалога» | `USER-GUIDE.ru.md:295-296` | подтверждено и **согласуется** с Q1-решением владельца (ТЗ не создаёт новую семантику поверх задокументированной) | +| `docs/specs/README.md` получил строку на #476 | `git diff origin/dev..HEAD -- docs/specs/README.md` | подтверждено | + +Ни одно из проверенных утверждений не оказалось догадкой, выданной за факт — +все грамматически привязаны к реальному коду или реальному документу. + +## Находки + +### Medium (в скоупе, чинится в текущем ТЗ) — §18 противоречит §7.3/AC4 + +**Файл:** `docs/specs/476-color-picker-ok.md`, §18 «Принятые предположения» vs +§7.3 «Невалидный HEX» и AC4. + +**Суть.** §18 предполагает: «confirm вызывает существующий HEX commit helper +и использует результат валидации». Существующий `_commitHex()` +(`hp-color-opacity.ts:663-673`) на невалидном значении делает: + +```ts +if (!normalized) { + this._hexDraft = this._lastValidColor; // возвращает валидную строку в поле + this._hexInvalid = true; + return; +} +``` + +Это уже возвращает в `_hexDraft` валидную строку (последнее применённое +значение). Если «ОК» реализовать буквально как «вызвать `_commitHex()` и +посмотреть на результат», то **повторное нажатие «ОК» без нового ввода** +запустит `_commitHex()` второй раз над уже-валидным `_hexDraft` → +`normalizeHexColor` пройдёт → `_hexInvalid = false` → picker закроется. + +Ровно это прямо запрещено ТЗ: «повторное «ОК» без нового валидного +пользовательского ввода не имеет права снять состояние ошибки только потому, +что commit уже вернул в поле последнее валидное значение» (§7.3, последний +абзац), и это же явно закреплено как обязательный шаг теста в собственном +плане автора: «повторным «ОК» доказать отсутствие обхода» (§13, п.2) и как +защитный AC4: «Повторное «ОК» без нового валидного input также не закрывает +поверхность». + +**Воспроизведение (по коду, а не по исполнению — компонента не собран для +класса C диффа):** invalid HEX → blur/commit (error виден, `_hexDraft` = +last valid) → фокус НЕ трогая hex-поле → клик «ОК», реализованный по букве +§18 → `_commitHex()` над валидным `_hexDraft` → `_hexInvalid=false` → close. +AC4-мутант «доказать отсутствие обхода» покраснеет при такой реализации. + +**Почему это не мелочь.** Это не тонкость реализации, свободно решаемая +разработчиком: §18 прямо называет конкретный существующий helper и описывает +механизм, который математически не может выполнить соседний обязательный +AC. Технический автор (Codex), доверившись §18 буквально, произведёт код, +проваливающий собственный тест из плана автора — ровно тот класс дефекта, +который спек-ревью обязано ловить до кода, а не после. + +**Требуемая правка.** §18 должен явно называть механизм различения «поле +не менялось со времени ошибки» vs «новое валидное значение введено» — +например, отдельный флаг, сбрасываемый только по `input`-событию хекс-поля +(а не по `_commitHex()`), который «ОК» обязан проверить перед закрытием. +Формулировку «вызывает существующий HEX commit helper» нужно уточнить или +заменить, поскольку буквальное её прочтение ломает AC4. Это в скоупе задачи +(правится в тексте того же ТЗ), не в скоупе другого issue. + +### Low — §7.2 нумерованный список читается как безусловный + +**Файл:** `docs/specs/476-color-picker-ok.md`, §7.2, шаг 3: «возвращает +keyboard focus на swatch trigger» стоит в общем списке 1–4 без явного «при +валидном draft», хотя относится к тому же условию, что и шаг 2. §7.3 отдельно +и явно указывает, что при невалидном HEX фокус должен идти в HEX-поле, а не на +trigger — так что конфликта по существу нет, но при беглом чтении только §7.2 +шаг 3 можно принять за безусловное действие. Снимаю без правки (Low, решение +ревьюера с записью): §7.3 формулирует более специфичное и явное правило и +имеет приоритет по построению документа; смысловой неоднозначности, +влияющей на AC, нет. + +## Что проверено и корректно + +- Оба продуктовых вопроса (§7.1 «сценарий» и «что человек увидит») отвечены + предметно, персона и поверхность совпадают с `docs/SCOPE.md` (J4/J6), без + терминов реализации в разделе «после». +- Открытых продуктовых вопросов нет: все 4 вопроса Q1–Q4 закрыты явным + решением владельца в комментарии, ТЗ переносит эти решения в §4 без + расхождений и без добавления новых недоговорённостей. +- Полный трек обоснован названным критерием (§5 PROCESS.md: новый UX-контракт + завершения) — не «обычный трек» без причины. +- Скоуп/не-скоуп (§5–§6) разделены чётко, граница «расширение публичного + контракта → возврат в S3-spec» присутствует. +- AC1–AC8 пронумерованы, у каждого указан способ доказательства + (unit/smoke/golden/docs gate), формулировки допускают ровно одну трактовку + результата (числа событий, DOM-порядок, `aria-invalid`, focus target). + Не нашёл ни одного AC, не проверяемого автотестом или явным ревью. +- Модель данных/миграция/compatibility (§10): корректно «нет изменений» — + проверено, что `ColorPickerLabels` — compile-time тип House Plan, а не + persisted-конфигурация, добавление поля не требует миграции; downgrade-путь + описан и правдоподобен (просто пропадает кнопка, event contract не трогается). +- i18n (§9): 4 языка перечислены, ключ следует уже используемому плоскому + формату, defensive fallback `OK` для стороннего consumer описан и не + противоречит текущему `_labels()` fallback-паттерну (`hp-color-opacity.ts:512-522`, + который уже делает `labels.x || DEFAULT_LABELS.x` — новое поле встраивается + в тот же паттерн естественно). +- Touch/a11y (§8): требования (40px, native `button`, forced-colors, no + click-through) конкретны и проверяемы; не противоречат + `docs/TOUCH-SUPPORT.md` (touch — best effort, но не деградация уже + доступной поверхности). +- Риски (§16) перечислены по одному на каждый нетривиальный контракт, у + каждого назван AC, который его снимает — не общие слова. +- Откат (§17) реалистичен: удаление кнопки/label/i18n-ключа не требует + миграции данных. +- Release-артефакты (§14): changelog RU+EN, golden-обновление пяти реальных + (не выдуманных) сцен, явное «нет perf/security артефактов» с обоснованием. +- Автор не рецензирует своё же ТЗ — сессия ревью отдельная, роли соблюдены + (PROCESS.md §6). + +## Чего не проверял + +- Не гонял `typecheck`/`test`/`build`/`golden:verify` — диф класса C + (документация), продуктовый код не менялся; эти гейты относятся к + код-ревью следующего этапа этой же задачи. +- Не проверял `docs/CONFIG-COMPATIBILITY.md` построчно — ТЗ заявляет «нет + миграции/новых полей», и это утверждение проверено против кода + (`ColorPickerLabels` — не персистентный тип), этого достаточно для вывода + «нет compatibility-влияния» без полного чтения документа. +- Не оценивал будущую точность CSS-реализации (padding/scroll-behaviour на + очень маленьких viewport) — это подробность код-ревью и golden-приёмки, а + не то, что можно верифицировать по тексту ТЗ. +- Не связывался с владельцем: новых продуктовых вопросов не возникло, оба + найденных пункта — технические (реализация валидации/фокуса), решаются + автором и ревьюером без владельца по PROCESS.md §7.1. + +## Вывод + +Один Medium-находка **в скоупе** задачи (несогласованность §18 и §7.3/AC4 по +механизму анти-обхода невалидного HEX) без High — вердикт жёлтый. Автор +правит текст ТЗ (уточняет §18, не код), правка проходит повторный цикл +ревью. Low-находка снята с записью, чинить не нужно. + +--- + + + +## Материал раунда + +- Ветка: `issue/476-color-picker-ok`, коммит `acd28558b394` — ребейз его осиротит, и это нормально: ниже якоря, которые ребейз не меняет. +- Дерево материала: `f54408b100e7cd2929096cf49134046c02f5bb65` + ``` + git log --all --format='%H %T' | grep f54408b100e7 + ``` +- ТЗ `docs/specs/476-color-picker-ok.md`, блоб `ef9e07a28df7b6590c9bc0e0cfc241f0a85f256a` + ``` + git log --all --find-object=ef9e07a28df7b6590c9bc0e0cfc241f0a85f256a -- docs/specs/476-color-picker-ok.md + ```