mirror of
https://github.com/Matysh/houseplan-card
synced 2026-09-29 03:09:36 +00:00
@@ -0,0 +1,184 @@
|
||||
# SPEC-REVIEW-180-r1
|
||||
|
||||
- **Issue:** [#180](https://github.com/Matysh/houseplan-card/issues/180) — hp-color-opacity: unified picker не покрывает ripple/activity-color и state-color settings (wall/light/temp/LQI/glow)
|
||||
- **ТЗ:** `docs/specs/180-all-color-call-sites.md` (коммит `3540d24`, ветка `issue/180-unified-color-picker`)
|
||||
- **Трек:** обычный (метка `small` не установлена; лимит циклов ревью ТЗ — 4)
|
||||
- **Ревьюер:** Claude, роль «ревьюер ТЗ» (сессия отдельна от анализа/автора)
|
||||
- **Цикл:** r1/4
|
||||
|
||||
## Вердикт
|
||||
|
||||
**Зелёный.** High: 0 · Medium: 0. Одна Low-находка, ниже — с записью «waived».
|
||||
|
||||
## Скоуп проверки
|
||||
|
||||
Этап — ревью ТЗ (PROCESS.md §2.4). Материал: тело issue #180, оба комментария
|
||||
(аналитика владельца + хендофф автора ТЗ), файл `docs/specs/180-all-color-call-sites.md`,
|
||||
`docs/specs/README.md`, канонический `docs/specs/057-color-opacity-picker.md`
|
||||
(родительский контракт `hp-color-opacity`), `docs/USER-GUIDE.ru.md`,
|
||||
`docs/TOUCH-SUPPORT.md`, `docs/CONFIG-COMPATIBILITY.md` и фактическое состояние
|
||||
`src/houseplan-card.ts`, `src/hp-color-opacity.ts`, `src/logic.ts`, `src/types.ts`,
|
||||
`src/i18n/{en,ru}.json` на ветке `issue/180-unified-color-picker` (совпадает с
|
||||
`origin/dev` + два документационных коммита). Продуктовый код на этой ветке не
|
||||
менялся — проверено `git diff --name-only origin/dev...HEAD`: только
|
||||
`docs/specs/180-all-color-call-sites.md` и `docs/specs/README.md`.
|
||||
|
||||
## Как проверялось
|
||||
|
||||
Ревью состязательное: каждое фактическое утверждение ТЗ (номера полей, названия
|
||||
свойств API, i18n-ключи, персистентная модель) сверено с реальным кодом на ветке,
|
||||
а не принято со слов автора.
|
||||
|
||||
1. Прочитаны `docs/SCOPE.md`, `AGENTS.md`, `PROCESS.md` (полностью, включая §7.1,
|
||||
§4, §5) — для критериев обязательных разделов, лимита циклов и трека.
|
||||
2. Прочитано issue #180 целиком: исходная находка ревьюера #57, комментарий
|
||||
аналитики владельца (2026-08-19, сам зафиксировал продуктовый контракт —
|
||||
мигрировать все 15 полей), хендофф автора ТЗ.
|
||||
3. Прочитан весь файл ТЗ `docs/specs/180-all-color-call-sites.md` (17 разделов).
|
||||
4. Прочитан канонический `docs/specs/057-color-opacity-picker.md` — сверен
|
||||
заявленный публичный API `hp-color-opacity` (`color`, `opacity`,
|
||||
`showOpacity`, `disabled`, `label`, `opacityLabel`, `pickerLabels`,
|
||||
событие `hp-color-opacity-change`) с тем, что ТЗ #180 обещает переиспользовать.
|
||||
5. Построчно сверены с исходным кодом (`Read`/`Grep`):
|
||||
- все пять native `<input type="color">` в `src/houseplan-card.ts`
|
||||
(14503, 15085, 19332, 19529, 19548) — состав и группировка полей совпадают
|
||||
с таблицей §4 ТЗ (11 через `_renderColorRow`, плюс bgColor/ripple/roomColor/
|
||||
space bgColor);
|
||||
- публичные свойства `hp-color-opacity` (`src/hp-color-opacity.ts:34-62`) —
|
||||
`showOpacity`, `pickerLabels`, `opacityLabel`, `disabled` реально существуют;
|
||||
- уже мигрированный Glow (`houseplan-card.ts:19158-19168`) как прецедент
|
||||
паттерна `showOpacity=false` + игнорирование opacity в draft — совпадает с
|
||||
§7.2 и техническим предположением №2 ТЗ;
|
||||
- модель данных: `FillColors` (`src/types.ts:1287-1302`, тип `{c,a}`),
|
||||
`space.settings.room_color/room_opacity` (`src/logic.ts:1245-1246`,
|
||||
`houseplan-card.ts:13447-13448`), `marker.ripple_color`
|
||||
(`src/types.ts:131`), `settings.bg_color`/`space.settings.bg_color`
|
||||
(`houseplan-card.ts:13449, 14436-14437`) — все совпадают с §9 ТЗ буквально;
|
||||
- i18n-ключи, на которые ссылается ТЗ (`gs.wall_fill`, `gs.bg_default`,
|
||||
`gs.bg_theme`, `space.bg_inherit`, `space.bg_inherited`,
|
||||
`marker.activity_color`, `marker.ripple_size`, `space.room_color`,
|
||||
`space.opacity`) — присутствуют в обоих `en.json`/`ru.json` парами.
|
||||
6. Терминология сверена с `docs/USER-GUIDE.ru.md`: «Общие настройки», «Пространство»,
|
||||
формулировка decor-picker («нажмите на образец цвета, чтобы открыть выбор цвета
|
||||
и прозрачности», строка 947) и существующее описание пульсации активности
|
||||
(строки 721-748) — ТЗ не придумывает новых терминов.
|
||||
7. Формулировка `Touch editor: supported` (§11 ТЗ) сверена с каноническим
|
||||
списком трёх допустимых фраз в `docs/TOUCH-SUPPORT.md:149-151` — совпадает
|
||||
буквально.
|
||||
8. `docs/CONFIG-COMPATIBILITY.md` — подтверждено, что задача не вводит и не
|
||||
меняет персистентные поля, поэтому реестр совместимости не затрагивается;
|
||||
ТЗ §9 прямо констатирует это, а не молчит об этом.
|
||||
9. Проверена трассируемость: `docs/specs/README.md:113-114` содержит обе строки
|
||||
(issue ↔ ТЗ) для #57 и #180; коммит `3540d24` несёт корректные трейлеры
|
||||
`Issue: #180` / `User-Visible: no` (`git log -1 --format=... 3540d24`).
|
||||
10. Проверена феासибельность golden-плана: `demo/golden/{harness,accept,matrix}.mjs`
|
||||
построены вокруг произвольного `scenario.id`/`viewport`/`theme` — добавление
|
||||
трёх новых сценариев (general settings, marker mobile, space) механически
|
||||
возможно без архитектурных изменений инфраструктуры.
|
||||
11. Проверено отсутствие незамеченных мест: полный `grep -rn "type=[\"']color[\"']" src/*.ts`
|
||||
— ровно 5 совпадений в `houseplan-card.ts` (совпадает с инвентаризацией ТЗ)
|
||||
плюс одно в `src/styles.ts:2162` — CSS-селектор, разбор ниже.
|
||||
12. Проверено, что day/night фон (`bgMode: 'daynight'`) не хранит собственных
|
||||
цветов (процедурный градиент по `docs/SUN.md`), то есть инвентаризация ТЗ
|
||||
(пять шаблонов/15 полей) действительно исчерпывающая, а не пропускает шестое
|
||||
скрытое место.
|
||||
|
||||
Гейты для этапа спек-ревью не прогонялись — на этой стадии нет продуктового кода
|
||||
для typecheck/test/build; сам факт отсутствия правок в `src/**` подтверждён diff'ом
|
||||
(п.10 выше), а не декларацией автора.
|
||||
|
||||
## Находки
|
||||
|
||||
### Low-1 — CSS-селектор `input[type='color']` не упомянут в плане очистки
|
||||
|
||||
**Файл:** `src/styles.ts:2162` (`.colorrow input[type='color'] { width: 42px; ... }`).
|
||||
|
||||
После миграции ни один элемент в DOM не будет соответствовать этому селектору —
|
||||
правило станет мёртвым. ТЗ формулирует source-invariant AC1 как «в `src/**/*.ts`
|
||||
отсутствует `input[type=color]` в любой допустимой Lit-форме» — эта фраза точна
|
||||
для шаблонов, но явно не называет `styles.ts` и не включает очистку CSS в план
|
||||
тестов (§13, п.1: «Рекурсивный scan `src/**/*.ts` запрещает native
|
||||
`input[type=color]`»). Риск скорее методологический, чем поведенческий: если
|
||||
исполнитель реализует scan буквальным regex по всему `src/**/*.ts` без разбора
|
||||
Lit-шаблонов от строковых литералов CSS, тест либо ложно упадёт на не относящемся
|
||||
к делу файле, либо (если scan ограничат template literals) осиротевшее правило
|
||||
останется в бандле молча.
|
||||
|
||||
**Воспроизведение:** `grep -rn "type=\"color\"\|type='color'" src/*.ts` — 5
|
||||
совпадений в `houseplan-card.ts` (реальные шаблоны) + 1 в `styles.ts` (CSS
|
||||
селектор), тогда как ТЗ и его AC1 описывают только первые пять.
|
||||
|
||||
**Серьёзность:** Low — не блокирует ни один AC, не меняет наблюдаемое поведение,
|
||||
исправляется одной строкой при реализации.
|
||||
|
||||
**Решение ревьюера:** waived, с записью. AC1 в применении к «допустимой Lit-форме»
|
||||
де-факто ограничивает scope шаблонами; удаление мёртвого CSS-правила — естественное
|
||||
следствие реализации АС2/АС3 (все пять consumers перестают рендерить native input)
|
||||
и не требует отдельного продуктового решения владельца. Не выношу отдельным issue:
|
||||
находка ниже порога Medium (PROCESS.md §3.8), достаточно фиксации здесь.
|
||||
|
||||
## Что проверено и корректно
|
||||
|
||||
- **Обязательные разделы §7.1** — все присутствуют и в правильном порядке:
|
||||
сценарий (§1), что человек увидит до/после (§2), проблема (§3), scope/не-scope
|
||||
(§4-5), контракт поведения (§6-7), UX/accessibility (§8), модель данных и
|
||||
совместимость (§9), i18n (§10), touch как часть UX-контракта (§11), AC1-AC8 с
|
||||
доказательством у каждого (§12), план автотестов (§13), риски (§14), откат (§15),
|
||||
release-артефакты (§16). Плюс два продуктовых обязательных пункта — персона/
|
||||
поверхность/момент и однострочное «что увидит человек» — закрыты §1-2 без
|
||||
терминов реализации.
|
||||
- **Трек выбран верно.** Сложность 4/10, пять разных dialog families, alpha/
|
||||
nullable/inherit контракты — превышает порог «одна поверхность» и «сложность
|
||||
≤3» из §5 PROCESS.md; `small` не применили обоснованно.
|
||||
- **Инвентаризация полная и точна.** Все 15 полей и все 5 native-шаблонов из §4
|
||||
ТЗ подтверждены построчно в текущем `src/houseplan-card.ts`; ни одно
|
||||
дополнительное место (day/night, decor, room custom fill, Glow — уже
|
||||
мигрированные #57 consumers) не пропущено и не задвоено.
|
||||
- **Технические утверждения не являются догадками.** Каждое поведенческое
|
||||
обещание (nullable/default/inherit модель, `showOpacity=false` не пишет alpha,
|
||||
`hp-color-opacity` API) имеет прямой аналог в уже смерженном коде (#57, Glow) —
|
||||
это перенос существующего паттерна, а не изобретённое поведение.
|
||||
- **AC проверяемы и однозначны.** Все 8 AC пронумерованы, у каждого — конкретный
|
||||
способ доказательства (unit/source-scan, DOM/browser smoke, golden, round-trip
|
||||
unit); ни один не сформулирован расплывчато настолько, чтобы допускать
|
||||
произвольную трактовку «выполнено».
|
||||
- **Продуктовая неопределённость закрыта на входе, а не додумана.** Ключевой
|
||||
продуктовый вопрос («что входит в #180 — весь остаток native color inputs или
|
||||
уже нет») задан и решён самим владельцем в комментарии аналитики 2026-08-19,
|
||||
до написания ТЗ; спека честно ссылается на это решение (§4, преамбула) вместо
|
||||
того чтобы выдавать его за собственную догадку автора.
|
||||
- **Assumed-block корректен.** §17 содержит 5 явно технических предположений
|
||||
(структура компонентов, обработка `opacity=1` для color-only, локальность
|
||||
layout-классов, отсутствие reset callback у picker, golden thresholds на
|
||||
реализации) — ни одно из них не является продуктовым вопросом, который следовало
|
||||
бы задать владельцу.
|
||||
- **Терминология и touch-формулировка соответствуют канону** — сверено с
|
||||
`docs/USER-GUIDE.ru.md` и `docs/TOUCH-SUPPORT.md` построчно (см. «Как
|
||||
проверялось», пп. 6-7).
|
||||
- **Совместимость.** §9 явно и верно констатирует отсутствие изменений схемы/
|
||||
миграции; сверка с `docs/CONFIG-COMPATIBILITY.md` подтверждает, что реестр не
|
||||
требует новой записи.
|
||||
- **Трейлеры и трассируемость корректны** для документационного коммита на этом
|
||||
этапе (`Issue: #180`, `User-Visible: no`, ссылки issue ↔ ТЗ в
|
||||
`docs/specs/README.md`).
|
||||
|
||||
## Чего не проверял
|
||||
|
||||
- Golden pixel-thresholds и точные scenario id — по ТЗ (§17, п.5) выбираются на
|
||||
реализации по полному Linux-артефакту; на этапе спек-ревью нет артефакта для
|
||||
проверки.
|
||||
- Реальную работу браузерных smoke/golden сценариев — код ещё не написан
|
||||
(подтверждено diff'ом), поэтому запуск `npm test`/`npm run build`/
|
||||
`node demo/smoke_color_picker.mjs` на этом этапе неприменим и не выполнялся.
|
||||
- Backend/Python — задача не касается `custom_components/**/*.py`; pytest не
|
||||
запускался, это верно для class A/B TS-only изменения.
|
||||
- Производительность на реальном железе — компонентная модель без новых
|
||||
observers/network подтверждена чтением, а не профилированием (профиль не
|
||||
требуется до S7-code-review при отсутствии заявленного perf-риска).
|
||||
|
||||
## Итог
|
||||
|
||||
ТЗ #180 выполнимо, проверяемо и не содержит выданных за факт догадок: каждое
|
||||
поведенческое утверждение либо повторяет уже принятый контракт #57, либо сверено
|
||||
с текущим кодом. Единственная находка (Low, мёртвый CSS-селектор вне явного
|
||||
scope AC1) waived с запиской, доработки ТЗ не требует. Готово к `S5-ready`.
|
||||
Reference in New Issue
Block a user