diff --git a/docs/reviews/CODE-REVIEW-660-r1.md b/docs/reviews/CODE-REVIEW-660-r1.md new file mode 100644 index 00000000..6c8b8fa8 --- /dev/null +++ b/docs/reviews/CODE-REVIEW-660-r1.md @@ -0,0 +1,191 @@ +# CODE-REVIEW-660-r1 + +Issue: #660 · Заход: r1 · Материал: `93d93f8c8911f74eb761106e22244ac1ba799949` +(HEAD, `git diff origin/dev...HEAD`, два коммита: +`ea11a6d6` feat + `93d93f8c` test). + +## Скоуп + +Follow-up к #647: крестик закрытия редактора переносится из внешнего +соседнего слота внутрь сегментированной группы `.modes` (сразу после активной +кнопки), расстояние `.modes` → `.zoomctl` уменьшается вдвое измеримым +контрактом (50/40.5 px), desktop-кнопки управления сводной панелью становятся +доступны во всех трёх редакторах (сам overlay остаётся View-only), и `Esc` +получает терминальный выход в View для Plan и Devices (Decor его уже имел). +Мобильный gear-меню (Q2 — альтернатива) не меняется. Задача — полный +продуктовый трек; ТЗ прошло SPEC-REVIEW-660-r1/r2/r3 (зелёный на r3), реализация +описана как её точное следствие. Работа закрывает J4/J6 (обслуживание плана +предсказуемее), полироль desktop-first контракта редакторов, без новой +предметной функции — в рамках `docs/SCOPE.md`. + +## Как проверялось + +Дешёвые гейты (`typecheck`, `npm test`, `npm run build`, `bundle-policy +--verify`, `no-new-any`, `no-new-private-writes`, диф-мутанты 6/6 шардов) +подтверждены зелёным Validate на этом самом SHA +(https://github.com/Matysh/houseplan-card/actions/runs/36229721971, +`workflow_dispatch`) — но в этом конкретном прогоне `changes`-джоб пометил +только фронтенд-путь как затронутый: **все «тяжёлые» джобы (smoke, golden, +performance_smoke, backend, hacs, hassfest, geometry_parity) в этом Validate +были `skipped`**, не «success». Раздел «AC · чем доказан · чем краснеет» из ТЗ +предполагает исполненные browser-смоки и golden-сверку — этого в материале не +было, поэтому прогнал сам: + +| Гейт | Результат | Комментарий | +|---|---|---| +| `npx tsc --noEmit` / `npm test` / `npm run build` | не перегонял | зелёный Validate на этом SHA, дешёвые гейты сошлись (#343) | +| `bundle-policy --verify`, диф-мутанты (6/6 шардов) | не перегонял | тот же зелёный Validate; шарды диф-мутантов явно включают все шесть новых/изменённых #660-свидетелей (проверено по exact match строк в `scripts/mutation-registry.mjs`) | +| `node scripts/smoke-select.mjs --base origin/dev --head HEAD` | прогнал | 8 прямых совпадений (`_deviceDrag`, `_mode`, `stopPropagation`), 43 слабых; ниже — какие взял | +| `node demo/smoke_editor_tabs.mjs` (AC1, AC4, AC5, AC7, AC8 — назван в ТЗ) | **зелёный** | включает новые `planEscapeKeepsInternalPriority`, `planNeutralEscapeExits`, `deviceNeutralEscapeExits`, `decorEscapeKeepsInternalPriority`, `decorNeutralEscapeExits`, `summaryControlsStayInPlan`, `summaryControlsStayInEveryEditor` | +| `node demo/smoke_toolbar_stable_width.mjs` (AC3, AC4, AC5, AC6, AC9 — назван в ТЗ) | **зелёный** | все 7 ширин (1400/1200/1000/768/620/481/390); `modeToZoomGapMatchesSpec` и `closeSlotLivesInsideModes` true на каждой | +| `node demo/smoke_mobile_view_header.mjs` (AC2, AC9 — назван в ТЗ) | **зелёный** | `crossClosesOnPhone` (реальный клик по ×), `admin481_unchanged`, `admin768_unchanged`, `admin1400_unchanged` | +| `node demo/smoke_modes.mjs` (прямое совпадение) | зелёный | не регрессировал переключением режимов | +| `node demo/smoke_drag_bounds.mjs` (прямое совпадение) | зелёный | геометрия decor/markers не задета | +| `node demo/smoke_device_position_history.mjs` (прямое совпадение, `_deviceDrag`) | зелёный | `escapeAbortsWithoutWrite` — подтверждает приоритет отмены drag над новым терминальным Esc (AC8 для Devices) | +| `node demo/smoke_kiosk.mjs`, `node demo/smoke_summary_panel_polish.mjs` (риск по смежным поверхностям) | зелёные | kiosk и summary-persist контракты не задеты | +| `node scripts/check-docs.mjs --screenshots=warn` (диф трогает `src/**`) | зелёный | 1 ожидаемое `WARN` про устаревший скриншот-отпечаток (см. «Промотион», не гейт ревью); термин-скан по `docs/UX-MODES.md`/`USER-GUIDE*` не нашёл запрещённых старых формулировок | +| `npm run golden:verify` (видимое изменение) | прогнал, см. ниже | ожидаемый диф, без сюрпризов | +| `python -m pytest tests_backend -q` | не прогонял | диф не касается `custom_components/**/*.py` | +| `npm run invariants` | не прогонял | диф не трогает геометрию/её модель | +| performance-профиль | не прогонял | не назван в AC | + +### Golden: анализ диффа, а не просто «есть diff» + +`npm run golden:verify` (после `npm run bundle:sync` на свежий билд) даёт ~30 +`different` из ~90 кадров. Проверил, что рисунок diff'а совпадает ровно с +объявленным изменением, а не течёт куда-то ещё: + +- Все geometry/lighting/junction/device-icon/device-value кадры — `passed` + (канва и рендер устройств не задеты). +- Все `*-mobile-ru` и `*-phone-*` кадры — `passed` (мобильный контракт + Q2/AC2/AC9 не изменился — ожидаемо). +- `different` — ровно desktop-сценарии с видимым `.hdr` (диалоги + `*-dialog-desktop-en/de`, попoверы `*-popover-desktop-en/ru`, + `support-desktop-*`, `backup-*-desktop-*`, `optimize-*-dialog-*`) и + editor-режимы (`tray-wide/medium-*`, `furniture-*`) — именно там, где + крестик сменил позицию, появились/остались summary-controls и сместился + промежуток до zoom. + +Это ожидаемый, локализованный diff, а не регрессия. Baseline-приёмка — +предрелизный шаг (`npm run golden:accept -- --reviewed` на Linux/аттестованном +WSL), не обязанность код-ревью; сам риск назван автором в ТЗ. + +## AC · чем доказано (сверка таблицы автора) + +| AC | Заявлено ТЗ | Проверено | Как | +|---|---|---|---| +| AC1 (desktop summary в редакторах, overlay View-only) | unit + browser smoke | ✅ | `test/summary-panel-runtime.test.mjs` (`renderControls(false)` ≠ `nothing` в plan/devices/decor, `renderControls(true)` = `nothing`), `smoke_editor_tabs` `summaryControlsStayIn*`; чтением подтверждено, что `renderPanel()`/`syncPresentation()` не тронуты и `eligible` по-прежнему требует `_mode === 'view'` (`summary-panel-runtime-loaded.ts:860`) | +| AC2 (мобильное меню не меняется) | unit + smoke на 390/480 | ✅ | `header-menu.ts` diff — только комментарий, `mode === 'view'` guard (:79) не тронут; `smoke_mobile_view_header` зелёный на 390/320/481 | +| AC3 (gap 50/40.5 ±1) | browser layout smoke | ✅ | проверено чтением CSS-каскада (`.head{gap:10px}` из `dialogs.styles.ts` / `6px` в `@media 620px`, `.spacer{flex:0 0 30px/28.5px}`) — арифметика 10+30+10=50 и 6+28.5+6=40.5 совпадает с исполненным `modeToZoomGapMatchesSpec` на всех 7 ширинах | +| AC4/AC5 (порядок и стабильность слота) | browser smoke | ✅ | `closeSlotLivesInsideModes`, `widthAndTabsStable`, `slotOrder` (View=lastChild, редактор=после активной) — все true | +| AC6 (доступность ×) | smoke | ✅ | `crossVisibleAndSized`, `crossHitTarget`, `idleSlotInert`, `idleSlotKeepsSize` — true; `aria-hidden` в View подтверждено чтением шаблона | +| AC7 (нейтральный Esc) | keyboard smoke | ✅ | `planNeutralEscapeExits`, `deviceNeutralEscapeExits`, `decorNeutralEscapeExits` — true; чтением подтверждено, что каждая ветка `_onKey` заканчивается `return` (нет двойного действия) | +| AC8 (приоритет Esc) | smoke-сценарии | ✅ | `planEscapeKeepsInternalPriority`, `decorEscapeKeepsInternalPriority` (новые), `escapeAbortsWithoutWrite` в `smoke_device_position_history` (существующий, для Devices+drag) | +| AC9 (узкие ширины, touch-выход) | mobile/layout smoke | ✅ | `crossClosesOnPhone` (реальный клик), `w390/w481_noHeadOverflow`, `.modes` схлопнут до 24px, `.modetab` скрыты — не весь контейнер | +| AC10 (документация) | check-docs + ревью диффа | ✅ | `docs/UX-MODES.md`, `USER-GUIDE(.ru).md` обновлены непротиворечиво; term-scan прошёл | + +Каждый защитный AC также подтверждён исполненным диф-мутантом +(`scripts/mutation-registry.mjs`, секция `#660`) — 5 новых witness'ов +(`toolbar-close-slot-leaves-mode-group`, `toolbar-mode-zoom-gap-regresses`, +`editor-summary-controls-return-to-view-only`, +`phone-editor-close-hidden-with-mode-buttons`, +`editor-neutral-escape-does-not-exit`) плюс find/replace-обновление всех +унаследованных #647/#616-мутантов под новую разметку; Validate на материале +подтвердил все 6 шардов диф-мутантов зелёными — то есть каждый снятый guard +реально покраснел на своей мутации, а не только заявлен текстом. + +## Что проверено и корректно + +- `_onKey`: и в ветке `devices`, и в терминале ветки `plan` (после лестницы + инструментов) добавлен явный `return` — не может быть двойного действия + одним нажатием (по коду и по исполненным сценариям выше). +- Изменение сигнатуры `renderControls(kiosk=false)` — `if (kiosk && + this.host._mode !== 'view') return nothing;` — оставляет kiosk-путь + View-only (kiosk и так всегда в View по продуктовому контракту, редакторы + туда не рендерятся), не открывает новую поверхность. +- `.editor-close-slot` теперь потомок `.modes` во всех режимах (в шаблоне — + внутри `.map()` для активного редактора и после `.map()` для View), CSS + `.editor-close-slot[aria-hidden='true']{pointer-events:none}` сохраняет + инертность пустого резерва. +- Мобильный медиа-запрос `@media (max-width:480px)` больше не прячет весь + `.modes` целиком (что было бы High, как в SPEC-REVIEW-660-r2): прячет только + `.modetab`, коллапсирует контейнер до 24px без фона — крестик остаётся + единственным touch-выходом, подтверждено кликом в смоке. +- Трейлеры: `feat`-коммит `User-Visible: yes` содержит оба changelog + (`docs/CHANGELOG.md`, `docs/CHANGELOG.ru.md`) в одном коммите; `test`-коммит + `User-Visible: no`. Оба несут `Issue: #660`. +- Числа продублированы в одном источнике: 50/40.5 px — единственное место + расчёта (CSS `gap`+`flex-basis`), смок измеряет фактический + `getBoundingClientRect`, а не копию константы; расхождения быть не может. +- Второй коммит (`93d93f8c`) — легитимный ответ на красный Validate первого + захода (`no-new-private-writes` поймал прямые записи в `_setMode`/`_tool` в + собственном же новом тесте): переведён на публичный факад + `window.__hpTest.setMode/setTool` (реальные клики, `demo/helpers/hp-test.mjs` + подтверждает существование обоих методов), а не подавлен. Оставшаяся + приватная запись `c._areaSel = savedAreaSel` — не новая (существовала на + `origin/dev` до этой задачи), помечена `private-ok` по существу (нет + публичного сеттера для отката тестовой фикстуры). + +## Находки + +**Low (снята с записью, не блокирует).** `src/houseplan-card.ts` в `_onKey` +диф без функциональной необходимости убрал два поясняющих WHY-комментария: +«close the topmost open dialog; info popups first, then editors» (был перед +лестницей диалогов) и «The palette is one explicit surface: Escape closes it +and returns to Select in one step…» (перед веткой `_decorTool === 'furniture'`). +Код под ними не менялся — это чистая потеря документации порядка приоритета, +не относящаяся к скоупу задачи. Функционального риска нет (те же исполненные +сценарии AC8 подтверждают порядок), поэтому не блокирую; отмечаю для автора на +будущее внимание к сопутствующим правкам в часто редактируемом файле. + +High/Medium не найдено. + +## Чего не проверял + +- Полный HA backend harness (`pytest tests_backend` с реальной Home Assistant) + — диф не касается `custom_components/**/*.py`. +- `npm run invariants` — диф не меняет геометрическую модель ни её ссылки. +- `performance_smoke` / отдельный performance-профиль — не назван в AC, диф не + выглядит как влияющий на кадровый бюджет (только CSS/шаблон шапки и ветка + клавиатуры). +- Полная браузерная матрица (~276 смоков) — выбор ограничен AC-названными + смоками, прямыми совпадениями `smoke-select` и явно рискованными соседями + (kiosk, summary-persist, device-drag-history); полный набор — предрелизная + обязанность, а не гейт код-ревью. +- Принятие golden-baseline'ов (`golden:accept`) — предрелизный шаг на + Linux/аттестованном WSL, не входит в код-ревью; проверил только форму диффа + (см. выше), не выполнял приёмку. +- Ручного визуального просмотра в реальном браузере (сверх Playwright-смоков) + не делал — не было доступного дисплея; вся визуальная сверка прошла через + `getBoundingClientRect`/скриншот-diff инструменты. + +## Материал раунда + +- SHA материала: `93d93f8c8911f74eb761106e22244ac1ba799949`. +- ТЗ: редакция r3 (см. SPEC-REVIEW-660-r3, зелёный). +- Validate (workflow_dispatch, review-кандидат): + https://github.com/Matysh/houseplan-card/actions/runs/36229721971 — success + на фронтенд/провенанс/диф-мутантах; тяжёлые джобы (smoke/golden/backend/ + hacs/hassfest/geometry_parity/performance) — skipped в этом прогоне, закрыты + ревьюером локально (см. таблицу гейтов). + +## Вердикт + +Зелёный. Все 10 AC доказаны исполнением (unit + browser smoke + diff-мутанты), +защитные AC имеют исполненного покрасневшего свидетеля, документация и +changelog согласованы, трейлеры верны. Единственная находка — Low, снята с +записью, не блокирует. + +--- + + + +## Материал раунда + +- Ветка: `issue/660-toolbar-polish`, коммит `93d93f8c8911` — ребейз его осиротит, и это нормально: ниже якоря, которые ребейз не меняет. +- Дерево материала: `152eca2a0629e2bb15b609e6cf594282c17ef0f6` + ``` + git log --all --format='%H %T' | grep 152eca2a0629 + ``` +- Тело issue: `8934e3862ceec6de29d1fd24fe32df49790efd62d8288fb28e36eecb05e44387` +- Вердикт конвейера: `green` · High 0