mirror of
https://github.com/Matysh/houseplan-card
synced 2026-09-29 03:09:36 +00:00
claude[bot]
@@ -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-находка снята с записью, чинить не нужно.
|
||||
|
||||
---
|
||||
|
||||
<!-- material-anchors: сгенерировано конвейером (#414) -->
|
||||
|
||||
## Материал раунда
|
||||
|
||||
- Ветка: `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
|
||||
```
|
||||
Reference in New Issue
Block a user