From 32e3a79dcdccb33a27491e7b912f53942d200070 Mon Sep 17 00:00:00 2001 From: "claude[bot]" <209825114+claude[bot]@users.noreply.github.com> Date: Tue, 18 Aug 2026 19:13:01 +0000 Subject: [PATCH] docs: review document for #57 Issue: #57 User-Visible: no --- docs/reviews/SPEC-REVIEW-57-r1.md | 302 ++++++++++++++++++++++++++++++ 1 file changed, 302 insertions(+) create mode 100644 docs/reviews/SPEC-REVIEW-57-r1.md diff --git a/docs/reviews/SPEC-REVIEW-57-r1.md b/docs/reviews/SPEC-REVIEW-57-r1.md new file mode 100644 index 00000000..4109c7ad --- /dev/null +++ b/docs/reviews/SPEC-REVIEW-57-r1.md @@ -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) рендерит + `` внутри popover-поверхности, то есть первый клик + открывает House Plan popover, а сам цвет по-прежнему делегирован системному + `` без 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» построчным поиском + `` рядом с отдельным ``, без + единого 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`) — тоже голый `` без + `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» в +диалоге) — это голый ``, вообще не использующий +`hp-color-opacity`, то есть требующий такой же работы, как и остальные call +sites, но не включённый в контракт, AC или тестовый план. + +Дополнительно обнаружена целая параллельная реализация того же паттерна: +`_renderColorRow()` (`houseplan-card.ts:13477-13487`) рендерит `` рядом с отдельным `` — тот же класс +проблемы, который 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): текущий компонент + действительно делегирует цвет `` внутри своего popover. +- **Инфраструктурные ссылки на #68 (`FloatingSurfaceController`, + `registerOverlay`, «exclusive transient surface») реальны и точны** — + подтверждены существующим кодом `floating-surface-controller.ts` и + `hp-dialog.ts:365`, а не изобретены. +- **Перечисленные (не все, см. Medium-1) существующие call sites точны** — + все указанные в §12 использования `` найдены построчно + в `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` построчно на предмет иных, ещё не + найденных мест с голым `` за пределами уже + идентифицированных `_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 будет считаться полным ответом на +исходный запрос.