mirror of
https://github.com/Matysh/houseplan-card
synced 2026-09-29 03:09:36 +00:00
committed by
Sergey Matyunin
parent
ffb10843c9
commit
32e3a79dcd
@@ -0,0 +1,302 @@
|
||||
# SPEC-REVIEW-57-r1
|
||||
|
||||
- **Issue:** https://github.com/Matysh/houseplan-card/issues/57
|
||||
- **ТЗ под ревью:** `docs/specs/057-color-opacity-picker.md` (коммит
|
||||
`b8d2613a18f53f0d1bae9e02d9966496d49ad22d`, ветка
|
||||
`issue/57-color-opacity-picker`)
|
||||
- **Роль:** ревьюер ТЗ (не автор), этап `S4-spec-review`
|
||||
- **Трек:** обычный (метки issue: `P3`, `polish`, `tech-debt`, `S4-spec-review`
|
||||
— метки `small`/`trivial` нет), файл в `docs/specs/` создан корректно, а не
|
||||
ТЗ в теле issue.
|
||||
- **Цикл:** r1/4
|
||||
|
||||
## Скоуп ревью
|
||||
|
||||
Проверялось соответствие ТЗ:
|
||||
|
||||
- `docs/SCOPE.md` — легитимность задачи как editor usability polish, а не
|
||||
расширение продукта за пределы job'ов;
|
||||
- `PROCESS.md` §2.4, §2.5 (DoR), §7.1 (обязательные разделы), §3/§12 (в т.ч.
|
||||
«догадка вместо решения», Medium → отдельный issue);
|
||||
- `AGENTS.md` — классы файлов, ветка, трейлеры коммита ТЗ;
|
||||
- `docs/TOUCH-SUPPORT.md` — контракт editors (best effort) и требование явной
|
||||
метки `Touch editor: …` для новых editor-фич;
|
||||
- полному тексту issue #57 и всем трём комментариям (аналитика с Q1 и
|
||||
предложенным default, решение владельца по Q1, финальный хендофф автора);
|
||||
- фактическому состоянию `src/hp-color-opacity.ts` и
|
||||
`src/floating-surface-controller.ts` — на предмет того, что технические
|
||||
утверждения ТЗ о существующей инфраструктуре (#68) не выдуманы;
|
||||
- фактическим call sites `hp-color-opacity` и связанных цветовых настроек в
|
||||
`src/houseplan-card.ts` — на предмет полноты списка «Existing call sites»
|
||||
(§12/§16 ТЗ) относительно текста issue («decor, room colors, ripple»).
|
||||
|
||||
## Как проверялось
|
||||
|
||||
1. Прочитан весь тред issue #57 (`gh issue view 57 --json body,comments`):
|
||||
исходное описание (research task → recommendation → swap behind
|
||||
`hp-color-opacity` API, **явно называет три категории call sites: "decor,
|
||||
room colors, ripple"**), аналитика 2026-08-14 (ценность 4/10, сложность
|
||||
4/10, риск 5/10, P3, обычный трек, один продуктовый вопрос Q1 с default),
|
||||
решение владельца 2026-08-15 по Q1 («убрать вложенность: один клик — одна
|
||||
поверхность с цветом и прозрачностью вместе, без второго системного
|
||||
dialog»), финальный комментарий автора со ссылкой на коммит и файл ТЗ.
|
||||
Вопрос задан корректно — единственный, продуктовый, с default; ни одного
|
||||
технического вопроса владельцу не эскалировано.
|
||||
2. Сверены обязательные разделы ТЗ (§7.1 PROCESS.md) построчно — таблица ниже.
|
||||
3. Прочитан текущий `src/hp-color-opacity.ts` целиком: подтверждено, что
|
||||
проблема из §3 ТЗ реальна — `_pickerTemplate()` (строка 347) рендерит
|
||||
`<input type="color">` внутри popover-поверхности, то есть первый клик
|
||||
открывает House Plan popover, а сам цвет по-прежнему делегирован системному
|
||||
`<input type=color>` без alpha. Формулировка проблемы не голословна.
|
||||
4. Прочитан `src/floating-surface-controller.ts` и `registerOverlay` в
|
||||
`src/hp-dialog.ts:365` — оба реальны, используются уже сегодня в
|
||||
`hp-color-opacity.ts`. Ссылки ТЗ §7/§9/§10 на «общий floating-surface/
|
||||
overlay lifecycle из #68» и «exclusive transient surface, конкурирующая с
|
||||
`hp-help`» подтверждены существующим кодом и терминологией
|
||||
`docs/specs/068-help-affordance.md:50` («одна exclusive transient-
|
||||
поверхность на диалог, `hp-help` и `hp-color-opacity` используют один
|
||||
контракт»), а не изобретены заново.
|
||||
5. Проверены заявленные в §12 ТЗ «Existing call sites» построчным поиском
|
||||
`<hp-color-opacity` в `src/houseplan-card.ts`: найдены decor color/fill
|
||||
(строки 9152, 9167, 9325, 9395, 9442), marker Glow override (17917,
|
||||
`showOpacity=false` подтверждён на 17918), room/space custom fill (18365,
|
||||
18549). Список ТЗ для этих сайтов точен.
|
||||
6. Тем же поиском по всему файлу найдена «третья» линия цветовых контролов,
|
||||
которая **не** проходит через `hp-color-opacity` вовсе:
|
||||
`_renderColorRow()` (`houseplan-card.ts:13477-13487`) — рендерит голый
|
||||
`<input type="color">` рядом с отдельным `<input type="range">`, без
|
||||
единого swatch/popover. Используется 11 раз в диалоге "Общие настройки"
|
||||
(`gs.*`): `light_on/off/none` (14003-14005), `temp_cold/ok/hot`
|
||||
(14007-14009), `lqi_low/high` (14011-14012), `glow_base/glow_light`
|
||||
(14014-14015), `wall_fill` (14028) — тот же класс проблемы («второй клик
|
||||
по цвету — системный picker без прозрачности», здесь даже без единого
|
||||
swatch), но не упомянуто ни в §12 (call sites), ни в §16 (затронутые
|
||||
поверхности), ни в §5 (не входит в задачу) ТЗ.
|
||||
7. Отдельно проверено значение слова **«ripple»** из текста issue: marker
|
||||
`activity color` (`rippleColor`, `houseplan-card.ts:18087-18090`, тип
|
||||
`ripple_color` в `types.ts:1442`) — тоже голый `<input type="color">` без
|
||||
`hp-color-opacity` и без alpha. Это ровно та категория, которую issue
|
||||
называет по имени («decor, room colors, **ripple**») — и она отсутствует в
|
||||
§12 ТЗ, который вместо неё называет «marker Glow override» (уже
|
||||
мигрировавший на `hp-color-opacity` компонент, не требующий работы). См.
|
||||
Medium-1 ниже.
|
||||
8. Проверено наличие требуемой `docs/TOUCH-SUPPORT.md` буквальной декларации
|
||||
`Touch editor: …` — отсутствует; см. Low-1.
|
||||
9. Проверены трейлеры и class-принадлежность: `git show b8d2613 --stat`
|
||||
показывает только `docs/specs/057-color-opacity-picker.md` и
|
||||
`docs/specs/README.md` (класс C, ни одного файла класса A — правило №1
|
||||
AGENTS.md соблюдено, продуктовый код не тронут на этапе ТЗ). Коммит несёт
|
||||
`Issue: #57` и `User-Visible: no` — корректно для документа класса C.
|
||||
`docs/specs/README.md` обновлён тем же коммитом, ссылка issue↔ТЗ в обе
|
||||
стороны на месте.
|
||||
10. Проверено существование release-артефактов, которые §18 ТЗ обещает
|
||||
обновить: `docs/TESTING.md`, `docs/CHANGELOG.md`, `docs/CHANGELOG.ru.md`,
|
||||
`docs/USER-GUIDE.ru.md` — все существуют, ссылки не на несуществующие
|
||||
файлы.
|
||||
11. Проверены i18n-конвенции: явные ключи не перечислены буквально (только
|
||||
человеко-читаемые названия controls в §13), но такой же уровень
|
||||
детализации принят в прошедшем ревью `docs/specs/068-help-affordance.md`
|
||||
(тоже описывает механизм именования, а не конкретные строки) — не
|
||||
считаю это дефектом при спек-ревью, конкретные ключи — свободное
|
||||
техническое решение реализации.
|
||||
|
||||
## Обязательные разделы (§7.1 PROCESS.md)
|
||||
|
||||
| Раздел | Есть | Комментарий |
|
||||
|---|---|---|
|
||||
| Сценарий (персона/поверхность/момент) | ✅ | §1 — home admin, editor-диалоги, момент клика по образцу цвета |
|
||||
| Что человек увидит до/после | ✅ | §2, одна фраза до/после, без глубоких implementation-терминов |
|
||||
| Проблема (с подтверждённой причиной) | ✅ | §3, подтверждена чтением `hp-color-opacity.ts` (см. «Как проверялось» п.3) |
|
||||
| Скоуп / не-скоуп | ⚠️ | §4/§5 — полны для перечисленного, но не покрывают все call sites из текста issue, см. Medium-1 |
|
||||
| Контракт поведения | ✅ | §7 (interaction contract), §8 (значения/преобразования) |
|
||||
| Модель данных и миграция | ✅ | §12 — API/schema неизменны, обоснованно (presentation-only компонент) |
|
||||
| UX, i18n, accessibility, touch | ✅ | §9-11, §13; touch — см. Low-1 (нет буквальной метки) |
|
||||
| AC1…ACn с доказательством | ✅ | §14, 9 штук, у каждого назван способ доказательства (unit/smoke/golden/build artifact) |
|
||||
| План автотестов | ✅ | §15, unit/smoke/golden/performance по отдельности |
|
||||
| Риски | ✅ | §17, таблица риск/мера, 5 строк |
|
||||
| Откат | ✅ | §17 — public API и данные неизменны, откат = revert реализации |
|
||||
| Release-артефакты | ✅ | §18, конкретные существующие документы, оба changelog в одном `User-Visible: yes` коммите |
|
||||
|
||||
Присутствует и явный блок «Принятые технические предположения» (§19) —
|
||||
разделение продуктового/технического, которого требует §7.1: 5 пунктов, всё
|
||||
базовая реализация/API-неизменность, не user-facing решения.
|
||||
|
||||
## Находки
|
||||
|
||||
### Medium-1 — ТЗ молча сужает названные в issue call sites: «ripple» пропущен, параллельная семья «Общие настройки» не упомянута вовсе
|
||||
|
||||
**Файл:** `docs/specs/057-color-opacity-picker.md:179-181` (§12, «Existing
|
||||
call sites»), `:253-259` (§16, «Затронутые поверхности»), `:46-54` (§5, «Не
|
||||
входит в задачу») против `src/houseplan-card.ts:13477-13487`
|
||||
(`_renderColorRow`), `:14003-14028` (11 вызовов), `:18087-18090`
|
||||
(`rippleColor`).
|
||||
|
||||
Issue #57 явно перечисляет три категории call sites, которые должны
|
||||
обновиться «at once»: **«decor, room colors, ripple»**. ТЗ §12 вместо этого
|
||||
называет «decor stroke/fill/text/furniture, room/space custom fill **и marker
|
||||
Glow override**» — «ripple» нигде не упомянут, ни в scope, ни в §5 «Не входит
|
||||
в задачу» как явно отложенный пункт. Проверка кодом подтверждает: marker
|
||||
`ripple_color` (`houseplan-card.ts:18089`, «Marker: activity color» в
|
||||
диалоге) — это голый `<input type="color">`, вообще не использующий
|
||||
`hp-color-opacity`, то есть требующий такой же работы, как и остальные call
|
||||
sites, но не включённый в контракт, AC или тестовый план.
|
||||
|
||||
Дополнительно обнаружена целая параллельная реализация того же паттерна:
|
||||
`_renderColorRow()` (`houseplan-card.ts:13477-13487`) рендерит `<input
|
||||
type="color">` рядом с отдельным `<input type="range">` — тот же класс
|
||||
проблемы, который issue описывает как причину задачи («второй клик по цвету
|
||||
— системный picker без прозрачности»), только даже без единого swatch. Она
|
||||
используется 11 раз в диалоге "Общие настройки" для `light_on/off/none`,
|
||||
`temp_cold/ok/hot`, `lqi_low/high`, `glow_base/glow_light`, `wall_fill` —
|
||||
именно тех состояний, вокруг которых построены **J1/J5/J7** SCOPE.md (живая
|
||||
заливка комнат по свету/температуре/LQI). Она не упомянута ТЗ вовсе — ни как
|
||||
included, ни как explicitly excluded.
|
||||
|
||||
**Воспроизведение:** после реализации ТЗ как написано, пользователь по-прежнему
|
||||
встретит исходную проблему (второй клик по цвету открывает системный picker,
|
||||
прозрачность теряется/недоступна одновременно) на: (1) marker "activity color"
|
||||
(инструмент разметки, диалог устройства, `display: icon_ripple`) — явно
|
||||
названный issue как «ripple»; (2) любом из 11 полей диалога "Общие настройки".
|
||||
То есть заявленная в issue ценность «all call sites... upgrade at once»
|
||||
достигается лишь частично, и в самом ТЗ этот пробел не зафиксирован как
|
||||
осознанное решение — то есть выглядит как полное покрытие, не будучи им.
|
||||
|
||||
Это не делает текущий контракт невыполнимым или непроверяемым — AC1-9
|
||||
самодостаточны и проверяемы для того набора call sites, который ТЗ
|
||||
действительно перечисляет. Поэтому уровень Medium, а не High: реализация по
|
||||
этому ТЗ работает и не содержит логической ошибки, дефект — в неполноте
|
||||
заявленного покрытия и в тишине вокруг этой неполноты.
|
||||
|
||||
**Решение ревьюера:** Medium, заведён отдельным issue
|
||||
[#180](https://github.com/Matysh/houseplan-card/issues/180) со ссылкой на
|
||||
#57, метки `tech-debt`/`P3`/`S1-new`. Не блокирует `S5-ready`. Рекомендация
|
||||
автору — на выбор: (а) явно перечислить `ripple` в §12/§16 и включить в объём
|
||||
этого issue, раз он и так использует тот же компонент с
|
||||
`showOpacity=false` по образцу Glow (стоимость мала, паттерн уже есть); либо
|
||||
(б) добавить одну строку в §5 «Не входит в задачу», явно называющую `ripple`
|
||||
и "Общие настройки" state-color rows отложенными в #180 — чтобы ТЗ не
|
||||
выглядело полным покрытием, будучи им лишь частично. Выбор (а)/(б) — не
|
||||
продуктовый вопрос сам по себе (объём этого конкретного issue уже решён
|
||||
владельцем неявно: он не разбирал этот список построчно), поэтому это может
|
||||
решить сам автор технически; если он предпочтёт спросить владельца — это
|
||||
корректный продуктовый вопрос («входит ли ripple в объём #57») с default
|
||||
(б).
|
||||
|
||||
### Low-1 — нет буквальной декларации `Touch editor: …` по `docs/TOUCH-SUPPORT.md`
|
||||
|
||||
**Файл:** `docs/specs/057-color-opacity-picker.md:124-137` (§9)
|
||||
|
||||
`docs/TOUCH-SUPPORT.md`, раздел «Documentation rule», требует, чтобы новая
|
||||
спецификация editor-фичи явно указывала одно из: `Touch editor: supported` /
|
||||
`best effort / intentionally degraded` / `not exposed`. §9 ТЗ подробно
|
||||
описывает, что сам picker получает «явную touch-поддержку» (pointer capture,
|
||||
multi-touch safety, 40×40 touch targets), но не содержит буквальной строки
|
||||
формата `Touch editor: …`. Тот же класс замечания последовательно
|
||||
фиксировался как Low в прошлых спек-ревью этого репозитория
|
||||
(`SPEC-REVIEW-89-r1`, `SPEC-REVIEW-122-r1`, `SPEC-REVIEW-141-r1`,
|
||||
`SPEC-REVIEW-146-r1`), включая прецедент `docs/specs/068-help-affordance.md:10`
|
||||
(«Touch editor: **поддерживается для самого affordance**» — компонент,
|
||||
получающий явную touch-гарантию поверх общего best-effort правила
|
||||
редакторов, ровно как здесь).
|
||||
|
||||
**Решение ревьюера:** Low, не блокирует. Рекомендация — добавить одну строку
|
||||
вида «Touch editor: supported (сам picker; остальные операции родительских
|
||||
редакторов остаются best effort по `docs/TOUCH-SUPPORT.md`)» рядом с §9.
|
||||
Оставляю на усмотрение автора; фиксирую здесь как условие «низкое либо
|
||||
правится, либо снимается с записью» (§3.8 PROCESS.md) — снимаю без правки,
|
||||
так как содержательно требование уже выполнено прозой, отсутствует только
|
||||
буквальный формат метки.
|
||||
|
||||
## Что проверено и корректно
|
||||
|
||||
- **Соответствие `docs/SCOPE.md`:** задача — editor usability polish, не
|
||||
создаёт нового продуктового job'а и не расширяется на профессиональный
|
||||
графический редактор (владелец явно закрыл этот вопрос в аналитике:
|
||||
«в scope как editor usability, но без необходимости создавать
|
||||
профессиональный графический редактор»); эксплуатирует существующие J1/J4/
|
||||
J5/J6/J7 через editor usability, не создавая нового.
|
||||
- **Легитимность полного трека:** issue не помечен `small`/`trivial`, файл
|
||||
ТЗ в `docs/specs/` создан по правилу (не в теле issue) — соответствует
|
||||
сложности 4/10 и множеству поверхностей (decor/room/space/marker).
|
||||
- **Продуктовый вопрос закрыт по процессу:** единственный вопрос Q1
|
||||
(«насколько сложным должен быть picker») задан батчем с default,
|
||||
`blocked` был выставлен и снят владельцем при ответе; технических
|
||||
вопросов владельцу не эскалировано.
|
||||
- **Главное решение владельца воспроизведено точно:** §7 ТЗ («один клик —
|
||||
одна поверхность, где сразу доступны цвет и прозрачность, без вложенного
|
||||
системного picker») дословно соответствует формулировке владельца в
|
||||
комментарии от 2026-08-15.
|
||||
- **Технический диагноз §3 не голословен** — подтверждён прямым чтением
|
||||
`src/hp-color-opacity.ts` (см. «Как проверялось» п.3): текущий компонент
|
||||
действительно делегирует цвет `<input type=color>` внутри своего popover.
|
||||
- **Инфраструктурные ссылки на #68 (`FloatingSurfaceController`,
|
||||
`registerOverlay`, «exclusive transient surface») реальны и точны** —
|
||||
подтверждены существующим кодом `floating-surface-controller.ts` и
|
||||
`hp-dialog.ts:365`, а не изобретены.
|
||||
- **Перечисленные (не все, см. Medium-1) существующие call sites точны** —
|
||||
все указанные в §12 использования `<hp-color-opacity>` найдены построчно
|
||||
в `houseplan-card.ts` с совпадающей семантикой (`showOpacity=false` для
|
||||
Glow подтверждён).
|
||||
- **API/compatibility (§12)** корректно не расширяет config/storage schema —
|
||||
оправданно: изменяется только внутренняя реализация presentation-only
|
||||
компонента, внешний контракт (`color`, `opacity`, событие) не меняется.
|
||||
- **Bundle-budget (§6, §14 AC8)** сформулирован как измеримый критерий с
|
||||
точным способом доказательства (exact production build artifact,
|
||||
raw+gzip), включает условный путь для vendored-альтернативы с теми же
|
||||
ограничениями — не оставляет технический выбор недоказуемым.
|
||||
- **AC1-AC9 однозначны** и снабжены допустимым по §2.5 PROCESS.md способом
|
||||
доказательства (unit/smoke/golden/review), ни один не оставлен
|
||||
неопределённым.
|
||||
- **Release-артефакты (§18)** ссылаются на существующие файлы документации,
|
||||
оба changelog в одном `User-Visible: yes` коммите — соответствует правилу
|
||||
11 PROCESS.md.
|
||||
- **Трассируемость:** `docs/specs/README.md` обновлён тем же коммитом
|
||||
(раздел «P3», ссылка на ТЗ); коммит `b8d2613` несёт `Issue: #57`,
|
||||
`User-Visible: no`, класс C — корректно для документа ТЗ. `git show
|
||||
--stat` не содержит ни одного файла класса A — правило №1 AGENTS.md
|
||||
соблюдено.
|
||||
- **«Принятые технические предположения» (§19)** корректно отделяют
|
||||
свободные для автора реализации решения от продуктового контракта —
|
||||
соответствует требуемой в §7.1 PROCESS.md структуре.
|
||||
|
||||
## Чего не проверял
|
||||
|
||||
- Не прогонял никаких гейтов (`typecheck`/`test`/`build`/`golden`/смоки) — на
|
||||
этапе ТЗ продуктовый код не существует, гейты неприменимы; существование
|
||||
упомянутых механизмов (`FloatingSurfaceController`, `registerOverlay`,
|
||||
`demo/golden`, `docs/TESTING.md`) проверено чтением файловой системы, а не
|
||||
исполнением.
|
||||
- Не проверял реализуемость конкретной геометрии saturation/value field,
|
||||
round-trip toleranse ≤1 RGB channel или точный алгоритм hue-memory для
|
||||
achromatic цвета как кода — по тексту ТЗ (§19) это явно свободное
|
||||
техническое решение автора кода, подлежащее доказательству тестами на
|
||||
реализации/код-ревью, а не предмет ревью ТЗ.
|
||||
- Не проверял точность числовых оценок аналитики (ценность 4/10, сложность
|
||||
4/10, риск 5/10, P3) по существу — это поле владельца (PROCESS.md §2.2),
|
||||
уже принятое явным решением до написания ТЗ.
|
||||
- Не проверял весь `src/houseplan-card.ts` построчно на предмет иных, ещё не
|
||||
найденных мест с голым `<input type=color>` за пределами уже
|
||||
идентифицированных `_renderColorRow`/`ripple_color`/`bg_color` — поиск был
|
||||
ограничен паттернами `hp-color-opacity`, `input type=.?color.?` и
|
||||
`_renderColorRow` через grep, не исчерпывающим построчным чтением всего
|
||||
файла (~18500 строк). `bg_color` (фон, без alpha) сознательно не включён в
|
||||
Medium-1: у него нет отдельного opacity-компонента, то есть не тот класс
|
||||
проблемы, который описывает issue.
|
||||
- Не проверял, действительно ли `docs/USER-GUIDE.ru.md` требует конкретной
|
||||
правки текста прямо сейчас (спека верно откладывает точную формулировку в
|
||||
release-артефакты §18, реализация ещё не начата).
|
||||
|
||||
## Вердикт
|
||||
|
||||
Зелёный. High: 0, Medium: 1 (заведён отдельным issue
|
||||
[#180](https://github.com/Matysh/houseplan-card/issues/180), не блокирует),
|
||||
Low: 1 (косметика, снята с записью в этом документе, не требует правки перед
|
||||
`S5-ready`). ТЗ корректно и проверяемо решает то, что само заявляет как
|
||||
скоуп: устраняет вложенный system picker для decor/room/space/Glow call
|
||||
sites, техническая база (#68) подтверждена чтением кода, продуктовый вопрос
|
||||
владельца закрыт по процессу. Единственный содержательный пробел — молчаливое
|
||||
сужение относительно явно названного в issue call site «ripple» и не
|
||||
упомянутой параллельной семьи «Общие настройки» — не делает написанный
|
||||
контракт невыполнимым, поэтому не блокирует, но обязан быть решён отдельно
|
||||
(issue #180) до того, как задача #57 будет считаться полным ответом на
|
||||
исходный запрос.
|
||||
Reference in New Issue
Block a user