From e189867fa90d2aee44c26baf689f1ce8bd8a7cc6 Mon Sep 17 00:00:00 2001 From: "claude[bot]" <209825114+claude[bot]@users.noreply.github.com> Date: Sun, 6 Sep 2026 11:47:42 +0000 Subject: [PATCH] docs: review document for #476 Issue: #476 User-Visible: no --- docs/reviews/CODE-REVIEW-476-r1.md | 204 +++++++++++++++++++++++++++++ 1 file changed, 204 insertions(+) create mode 100644 docs/reviews/CODE-REVIEW-476-r1.md diff --git a/docs/reviews/CODE-REVIEW-476-r1.md b/docs/reviews/CODE-REVIEW-476-r1.md new file mode 100644 index 00000000..8de1ce17 --- /dev/null +++ b/docs/reviews/CODE-REVIEW-476-r1.md @@ -0,0 +1,204 @@ +# CODE-REVIEW-476-r1 + +- **Issue:** #476 — Явное завершение выбора цвета кнопкой «ОК» +- **Этап:** код-ревью (PROCESS.md §2.7), заход r1, блокирующих циклов израсходовано 0 из 4 +- **Ветка:** `issue/476-color-picker-ok`, HEAD `b5a1001e` (приведена к `dev` конвейером, + поверх легло 4 коммита `dev`: `34a6b276 → b5a1001e`). Разбор — полный, не по дельте + (ребейз на ушедший вперёд `dev`, §7.2/§2.10). +- **Диапазон:** `git log --oneline origin/dev..HEAD` — 7 коммитов (2 ТЗ, 2 документа + ревью ТЗ, 3 реализации); материал — `git diff origin/dev...HEAD`, 53 файла. +- **ТЗ:** `docs/specs/476-color-picker-ok.md`, лёгкий трек не применялся (полный трек по + собственному решению аналитики — новый UX-контракт), ревью ТЗ зелёное на r2 + (`docs/reviews/SPEC-REVIEW-476-r2.md`). + +## Скоуп диффа + +Общий `hp-color-opacity`: полноширинная кнопка «ОК» (последний DOM-control), +CSS-контракт (100% width, ≥40px, forced-colors), новый признак `_hexNeedsValidInput` +(защита от снятия ошибки повторным «ОК» без нового ввода), `color_picker.confirm` в +4 словарях, per-card labels в `houseplan-card.ts`, обновлённые unit/i18n-тесты, +расширенный `demo/smoke_color_picker.mjs` и правки `demo/smoke_help_affordance.mjs` +(fallback-путь), запись в `scripts/smoke-links.mjs`, оба changelog, `docs/TESTING.md`, +пересобранные `dist/**`/`custom_components/houseplan/frontend/**`, обновлённый +`docs/images/screenshots.json` (только fingerprint, PNG не менялись). Backend, модель +геометрии, config/storage не затронуты. + +## Как проверялось + +Зелёного Validate на `b5a1001e` не найдено — прогнал дешёвые и часть тяжёлых гейтов сам. + +| Гейт | Команда | Результат | +|---|---|---| +| Typecheck | `npx tsc --noEmit` | чисто, без вывода | +| Unit-тесты | `npm test` | `2077 tests, pass 2076, fail 0, skipped 1` — совпадает с заявленным в хендоффе | +| Build + sync | `npm run build && npm run bundle:sync` | `dist` собран; `cmp dist/houseplan-card.js custom_components/houseplan/frontend/houseplan-card.js` — идентичны; `git status --short` после `bundle:sync` пуст (demo/srv/assets, custom_components и dist совпадают с закоммиченным, включая несобственную копию стенда) | +| Docs-гейт | `node scripts/check-docs.mjs` | `Documentation checks passed (7 files, 12 external links)` — обязателен, diff трогает `src/**` | +| Новый `any` | `node scripts/no-new-any.mjs --base origin/dev --head HEAD` | «Новых any нет» (59 добавленных строк в 2 файлах) | +| Выбор смоков | `node scripts/smoke-select.mjs --base origin/dev --head HEAD` | Зарегистрированная связь (2): `smoke_color_picker.mjs`, `smoke_help_affordance.mjs`. Слабая связь (15, общее имя `stopPropagation`) — просмотрел список: `smoke_cover_*`, `smoke_decor`, `smoke_edit_walk`, `smoke_editor_gestures`, `smoke_furniture`, `smoke_hide_layers`, `smoke_inert_openings`, `smoke_modes`, `smoke_room_cards`, `smoke_space_settings`, `smoke_tap_ctx`, `smoke_tap_run`, `smoke_toggle_confirmation`, `smoke_value_face_source` — все про другие компоненты, ни один не адресует `hp-color-opacity`; не гонял | +| Целевой смок 1 | `node demo/smoke_color_picker.mjs` | `OK`, все 21 поле `true` | +| Целевой смок 2 | `node demo/smoke_help_affordance.mjs` | `OK`, все поля `true`, включая новые `fallbackPickerHasConfirm`/`fallbackPickerConfirmCloses` | +| Bundle budget | `npm run bundle:budget` | initial View `299683 B` gzip, потолок `300500±2000` — в бюджете; `::warning::` про низкий запас (1383 Б) — долг #367, не новый и не вызван этим диффом | +| Golden (advisory) | `npm run golden:verify` | 5 названных ТЗ сцен (`decor-color-popover-mobile-ru`, `decor-color-popover-desktop-en`, `general-color-popover-desktop-en`, `device-ripple-color-popover-mobile-ru`, `space-room-color-popover-desktop-ru`) — `different`, ожидаемо (новая кнопка); остальные ~100 сцен — `passed`. Просмотрел `actual/`+`diff/` для обеих цветовых сцен глазами — разница только в кнопке и вертикальном сдвиге содержимого попапа, цвет/геометрия/тема не поехали. Принятие эталонов — предрелизный гейт (`golden:accept -- --reviewed` на Linux CI), не гейт код-ревью; автор в хендоффе прямо назвал это «НЕ сделано» | +| Docs screenshots | ссылка автора: run `34029831929` | `gh run view` → `completed/success`; SHA `ce07db74` — предок HEAD; `check-docs.mjs` на HEAD зелёный, то есть текущий фингерпринт уже совпадает с деревом | +| Process-gate | `node scripts/process-gate.mjs` | «гейт пройден, предупреждений 0» | +| Трейлеры | `git log origin/dev..HEAD` (7 коммитов) | у всех `Issue: #476`; `11cc5386` (реализация) — `User-Visible: yes`, оба changelog в том же коммите; остальные 6 — `User-Visible: no` | + +**Мутации, прогнанные лично** (§2.7 «чем краснеет», защитные AC): + +| # | Мутация | Файл | Прогон | Результат | +|---|---|---|---|---| +| 1 | Снял ветку `if (this._hexNeedsValidInput) { this._hexInvalid = true; return; }` в `_commitHex` — повторное «ОК» без нового ввода закрывало бы picker | `src/hp-color-opacity.ts` | `npm run bundle:sync && node demo/smoke_color_picker.mjs` | `repeatedConfirmCannotBypassInvalidHex: expected true, got false` — тест красится, как обязан (см. M1) | +| 2 | Убрал `if (normalized !== this._lastValidColor)` — `_commitHex` эмитит всегда | `src/hp-color-opacity.ts` | то же | `confirmClosesWithoutDuplicateOrClickThrough: false`, `validCorrectionAllowsConfirm: false` — AC2 доказан исполнением | +| 3 | Убрал `event.stopPropagation()` из `_confirm` | `src/hp-color-opacity.ts` | то же + отдельный probe-скрипт с листенерами на каждом уровне DOM | `confirmClosesWithoutDuplicateOrClickThrough: true` — **тест НЕ покраснел** (см. M2); probe показал, что событие гасится на `.editor-secondary`, на один уровень выше `hp-color-opacity`, независимо от этой мутации | + +После каждой мутации источник восстановлен из `git show HEAD:src/hp-color-opacity.ts`, +`npm run bundle:sync` прогнан повторно, `git status --short` — пусто, `demo/smoke_color_picker.mjs` +снова `OK` (проверено). + +**Не гонял и почему:** +- `python -m pytest tests_backend` — diff не трогает `custom_components/**/*.py`. +- `npm run invariants` — diff не меняет рёбра комнат, записи толщины, `layout`, + `marker.space`, `open_spans`; геометрия не затронута. +- Performance-профили — не названы в AC; §15 ТЗ обоснованно утверждает отсутствие + влияния (один статический `button`, один `click`-handler в уже открытом lazy-picker). +- Полная матрица `demo/smoke_*.mjs` и полный `scripts/mutation-gate.mjs` — предрелизные + гейты (PROCESS §8), не гейт код-ревью; по дельте прогнаны только зарегистрированные + смоки плюс личные точечные мутации. +- `npm run golden:accept` — не моя роль на этом этапе; смотри выше. + +## Находки + +### Medium (в скоупе задачи, обе) + +**M1 — AC4: защита от обхода невалидного HEX повторным «ОК» не имеет постоянного +свидетеля в `scripts/mutation-gate.mjs`.** + +Признак `_hexNeedsValidInput` — это ровно то, что r1 ревью ТЗ потребовало добавить +(`SPEC-REVIEW-476-r1.md`), и ровно то, что доказывается только браузерным смоком +(`demo/smoke_color_picker.mjs`, дорогой гейт: пересборка бандла + Chromium). По +PROCESS.md §2.7: «Мутант в `scripts/mutation-gate.mjs` обязателен, когда защита живёт +в продуктовом коде и проверяется дорогим гейтом… там ревьюер не воспроизведёт +отрицательный прогон второй раз». Диф не добавляет ни одной записи в +`scripts/mutation-gate.mjs` (только запись-указатель в `scripts/smoke-links.mjs`, это +другой реестр — для выбора смоков, не для мутационного гейта). + +Я лично применил ровно эту мутацию (см. таблицу выше, #1) и получил красный +`repeatedConfirmCannotBypassInvalidHex`, так что защита реальна и смок сегодня её ловит +— но без постоянной записи в реестре это разовое доказательство ревьюера, а не +воспроизводимый гейт: следующий рефакторинг того же кода останется незамеченным, если +не начитает эту главу отчёта. + +**Чинится:** зарегистрировать мутант (по образцу #366/#384/#330 — `guard: node +demo/smoke_color_picker.mjs`, патч — снятие ветки `if (this._hexNeedsValidInput)` в +`_commitHex`), прогнать `node scripts/mutation-gate.mjs --check` и целевой `--id=`. + +**M2 — AC3/§7.2.4: заявленная защита «клик не проваливается в план/toolbar под +поверхностью» не имеет проверки, способной упасть на снятой защите.** + +Я убрал `event.stopPropagation()` из `_confirm` (единственный код защиты, добавленный +этой задачей) и прогнал `demo/smoke_color_picker.mjs` — `confirmClosesWithoutDuplicateOrClickThrough` +остался `true`. Добавил зонды-слушатели на каждом уровне DOM (кнопка → `.picker` → +shadowRoot picker → хост `hp-color-opacity` → `.editor-secondary` → shadowRoot `card` +→ `card`) и увидел, что событие в проверяемом сценарии (decor-tool color внутри +`.editor-secondary`) гасится ещё на `.editor-secondary`, на уровень выше самого +пикера, независимо от того, вызывает ли `_confirm` `stopPropagation()`. + +Продуктового бага здесь нет — клик в план сегодня не проваливается, даже с двойным +запасом. Но это значит, что заявленная в АС3/§7.2.4 защита самого `hp-color-opacity` +не доказана ни одним тестом, который способен упасть: в единственном +протестированном потребителе есть посторонний внешний гаситель, а другие потребители +общего пикера (general settings, room/space, ripple color) не обязательно обёрнуты +таким же контейнером — для них `_confirm`'s `stopPropagation()` может быть +единственной защитой, и она никем не проверяется. + +**Чинится** одним из двух путей: (а) перенести click-through-пробу в потребителя без +такого внешнего гасителя (например, `general-color` или `device-ripple-color`, +благо они уже в golden-матрице) так, чтобы снятие `stopPropagation()` красило смок; +либо (б) зарегистрировать мутант в `scripts/mutation-gate.mjs` с guard'ом на +`demo/smoke_color_picker.mjs`, но тогда сам смок сначала должен научиться падать по +пункту (а) — иначе мутант зарегистрирован, но бесполезен. + +High: 0. Medium вне скоупа: 0. + +### Low (сняты с записью, не блокируют) + +- **L1** `src/houseplan-card.ts:5932` — `invalidHex: …, confirm: …` на одной строке + (`git diff` слепил две записи объекта). Чисто косметика, синтаксис и typecheck + корректны; не мешает чтению настолько, чтобы требовать отдельного цикла. Снято. +- **L2** AC3 «keyboard activation» (Enter/Space по кнопке) проверяется в смоке только + через `.click()`, не через реальный `KeyboardEvent`. Риск исчезающий: кнопка — + нативный `