diff --git a/docs/reviews/CODE-REVIEW-140-r1.md b/docs/reviews/CODE-REVIEW-140-r1.md new file mode 100644 index 00000000..29b84b09 --- /dev/null +++ b/docs/reviews/CODE-REVIEW-140-r1.md @@ -0,0 +1,205 @@ +# CODE-REVIEW-140-r1 + +- **Issue:** [#140](https://github.com/Matysh/houseplan-card/issues/140) — узкий футер диалогов свойств проёма и физического объекта +- **Трек:** `trivial` (короткий трек, §5.1 PROCESS.md) +- **Коммит:** `40550dc618b5df59a1e123008d7207a899179d4b` "Fix editor property dialog footer width", ветка `issue/140-dialog-footer-width` +- **Диапазон:** `origin/dev..HEAD` = один коммит +- **Ревьюер:** Claude (код-ревью), сессия без контекста реализации + +## Скоуп + +Диапазон `git diff origin/dev...HEAD` затрагивает 8 файлов: + +- `src/houseplan-card.ts` — добавлен атрибут `wide` на `` в + `_renderOpeningDialog()` (:16431) и `_renderPhysicalDialog()` (:16745). Больше + ничего в файле не менялось. +- `test/golden-matrix.test.mjs` — существующий unit-тест + «all destructive editor dialogs use the shared responsive footer groups» + переименован и расширен проверкой `assert.match(openTag, /\bwide\b/, …)` для + `_renderOpeningDialog`, `_renderPhysicalDialog`, `_renderSpaceDialog`. +- `demo/smoke_dialog_footer_width.mjs` — новый browser-smoke (215 строк), + измеряет реальную геометрию футера в обеих локалях на десктопной и узкой + ширине. +- `docs/CHANGELOG.md`, `docs/CHANGELOG.ru.md` — по одному пункту, со ссылкой на + #140. +- `dist/houseplan-card.js`, `custom_components/houseplan/frontend/houseplan-card.js`, + `demo/srv/assets/houseplan-card.js` — три копии сгенерированного бандла, + идентичные друг другу (см. ниже). + +`src/styles.ts` (общий `dialog-action-footer`/перенос) не тронут — это +соответствует явному «не в скоупе» из решения владельца. Диалог пространства +(`_renderSpaceDialog`, :18042) не менялся: он уже использовал `wide`. + +Изменение соответствует зафиксированному владельцем решению в комментариях к +issue (два уточнения: перенос дефекта на `hp-dialog.ts`/атрибут `wide`, затем +включение диалога физических объектов в тот же скоуп) и не расширяет его — +CSS-контракт переноса футера не тронут, третья ширина не введена, диалог +черновика (4-кнопочный вариант того же `_renderPhysicalDialog`) не получил +требования «в одну строку», только требование не переполняться. + +Продуктовая линия по `docs/SCOPE.md`: правка полирует J6 («Keep the plan true +as the home evolves» — два редактора, повседневная работа админа дома в +десктопном браузере) — чисто визуальный дефект layout в admin-only +поверхности, не открывает новых возможностей и не противоречит инвариантам +View mode. + +## Как проверялось + +Соразмерность гейтов: диапазон меняет два вызова компонента и добавляет один +таргетный smoke; полный `smoke`/`golden`/`performance`/`pytest` не запускался +— обоснование в разделе «Чего не проверял». + +| Гейт | Команда | Результат | +|---|---|---| +| Typecheck | `npx tsc --noEmit` | green, без вывода | +| Unit | `npm test` | green, 789/789 | +| Build + sync копий | `npm run build` затем `cmp` трёх копий | green; после локальной пересборки рабочее дерево осталось чистым (`git status --short` пусто) — закоммиченный бандл байт-в-байт воспроизводим | +| Bundle hash | `sha256sum dist/houseplan-card.js custom_components/houseplan/frontend/houseplan-card.js demo/srv/assets/houseplan-card.js` | все три `9b87823ed5b7b0346657c9cd630f61346adf51e0d228225f9f7baa49bc4d8171` | +| Целевой smoke | `node demo/smoke_dialog_footer_width.mjs` (после `npm run build` + копирования в `demo/srv/assets`) | `OK`, все 24 проверки `true` — числа ниже | + +**Числа из прогона smoke** (десктоп 1000×820, узкий 320×820): + +| Диалог | Язык | `surfaceWidth` | `spareWidth` (запас) | `oneRow` | +|---|---|---|---|---| +| opening | en | 500.00 | 240.31 | true | +| physical | en | 500.00 | 240.31 | true | +| opening | ru | 500.00 | 187.09 | true | +| physical | ru | 500.00 | 187.09 | true | +| space (контроль) | ru | 500.00 | — | true, не регрессировал | + +Узкая ширина (320 px): `opening`/`physical` `surfaceWidth = 300.8`, вписан в +`320*0.94+1 = 301.8`, `wrapped: true` (перенос сохранён), `noHorizontalOverflow: +true`. Четырёхкнопочный `draft` (сегмент черновика) — `buttons: 4`, +`buttonsContained: true`, `insideViewport: true`, без требования одной строки, +как и оговорено в issue. + +**Дисциплина «тест должен уметь падать»** — проверена для обоих прогнанных +тестов, не только предъявлена: + +1. Unit-тест `test/golden-matrix.test.mjs` (сабтест «all destructive editor + dialogs use the medium shell…»): временно откатил `wide` в + `_renderOpeningDialog()`, прогнал `node --test test/golden-matrix.test.mjs` + → сабтест 13 упал (`AssertionError: _renderOpeningDialog must reserve the + existing 500 px desktop shell`). Откатил правку, `git status --short` снова + пуст. +2. Smoke `demo/smoke_dialog_footer_width.mjs`: тем же способом откатил `wide` + у `_renderOpeningDialog()`, пересобрал (`npm run build`, копия в + `demo/srv/assets`), прогнал smoke → `opening_en_medium_shell: false`, + `opening_ru_medium_shell: false` (остальные проверки, не зависящие от этого + диалога, остались `true`, как и должно быть). Откатил правку, пересобрал, + сверил хеш бандла — снова совпадает с закоммиченным. + +Оба теста реагируют на регресс именно в той точке, которую призваны защищать; +это не тавтологичные проверки. + +## AC — разбор + +- **AC1** (три кнопки в одну строку в обоих диалогах, en/ru). Доказано smoke: + `..._three_actions_one_row: true` для всех четырёх комбинаций + диалог×язык, `buttons: 3` в каждом случае, `oneRow: true`. Соответствует + требованию владельца «Удалить» слева / «Отмена», «Сохранить» справа — + подтверждено по коду (`dialog-action-danger` содержит одну кнопку `btn + danger` с `_deletePhysicalSelection`/аналог для проёма, `dialog-action-commit` + — `Cancel`+`Save`) и геометрически (`dangerRect`/`commitRect` на одной + строке через `oneRow`). +- **AC2** (запас положительный, перенос не срабатывает, числа названы). + Доказано smoke: `spareWidth` — 240.31 px (en) и 187.09 px (ru) на обоих + диалогах, `positive_localization_headroom: true`, `oneRow: true` — перенос + не сработал ни в одной из измеренных комбинаций. Числа приведены выше и + воспроизведены мной отдельным прогоном, а не переписаны из хендоффа. +- **AC3** (узкий экран без горизонтального скролла, перенос сохранён, диалог + пространства и четырёхкнопочный футер черновика не регрессируют). Доказано + smoke на 320 px: `opening_narrow_fits_viewport`, `physical_narrow_fits_viewport`, + `opening_narrow_keeps_responsive_wrap`, `physical_narrow_keeps_responsive_wrap`, + `draft_four_actions_remain_contained`, `space_narrow_not_regressed` — все + `true`. `wrapped: true` подтверждает, что CSS-перенос (не тронутый диффом) + по-прежнему срабатывает на узком экране, а не был случайно отключён атрибутом + `wide` (обе ширины заданы через `min(px, vw)`, как и предполагал владелец в + комментарии, и это подтверждено измерением, а не одним лишь чтением кода). + +Все три AC доказаны исполняемым smoke с реальными числами, а не «verified» +без команды. + +## Проверено чтением, не исполнением + +- Диапазон правки в `src/houseplan-card.ts` ограничен ровно двумя строками + (добавление токена `wide` к открывающему тегу ``), подтверждено + постатейным чтением diff — никаких сопутствующих изменений в теле методов, + в `_deletePhysicalSelection`, `_deleteDraftSegment`, `_deleteDraftWhole` или + в обработчиках `_saveMarker`/`_savePhysicalDialog` нет. +- `src/hp-dialog.ts` не менялся; свойство `wide = false` с `reflect: true` + (:36, :220) и правило `:host([wide]) .surface { width: min(500px, 94vw); }` + (:157) — уже существующий, семь раз используемый до этой правки контракт, + что подтверждает заявление issue «не вводит нового UX-контракта». +- `src/styles.ts` (`dialog-action-footer`, `dialog-action-group`, + `dialog-action-danger`/`commit`, :2989–3012) не менялся — контракт переноса + футера, о котором явно сказано «не менять», прочитан и подтверждён нетронутым + по diffstat. +- Диалог сегмента черновика (`d.kind === 'draft'`, :16780) рендерится тем же + `_renderPhysicalDialog()`, получил `wide` наравне с диалогом физического + объекта — это не отдельная ветка кода, значит расширение ширины + распространяется на него автоматически; узкий smoke подтверждает + containment (4 кнопки), но не требует одной строки, что соответствует явному + «не в скоупе» issue. +- Трейлеры коммита: `Issue: #140`, `User-Visible: yes` — оба changelog + правлены в этом же коммите (`docs/CHANGELOG.md`, `docs/CHANGELOG.ru.md`), + формулировки используют терминологию `docs/USER-GUIDE.ru.md` («проём», + «Удалить», «Отмена», «Сохранить»). +- Ветка `issue/140-dialog-footer-width` соответствует имени issue; коммит один, + без промежуточных «допишу потом». + +## Что проверено и корректно + +- Продуктовая правка минимальна и точна: два атрибута `wide`, использующих + существующий, уже семикратно применяемый вариант ширины — ровно то решение, + которое зафиксировал владелец. +- CSS-контракт переноса футера и вторая (узкая) ширина диалога не тронуты — + соответствует «не в скоупе». +- Три копии бандла синхронны и воспроизводимы локальной пересборкой + байт-в-байт (проверено дважды: до и после экспериментов с откатом). +- Тесты (unit + smoke) реально проверяют регресс, а не тавтологичны — оба + прожаты в «падающем» состоянии. +- Трейлеры, changelog, формат хендоффа и именование ветки соответствуют + PROCESS.md/AGENTS.md. +- Автоматическое ревью до этого не отработало (см. комментарий issue, + [прогон](https://github.com/Matysh/houseplan-card/actions/runs/31789374712)); + данный документ и вердикт — первый фактический код-ревью по этой задаче, + цикл r1/2 (лимит короткого трека). + +## Чего не проверял — и почему + +- **`node demo/smoke_*.mjs` (остальные 126 из 127)** — не прогонял. Diff не + трогает canvas-геометрию, солнце, LQI, освещение, изометрию, тач-инпут и + прочие поверхности; единственная затронутая поверхность (футер двух + диалогов свойств) покрыта специально написанным `smoke_dialog_footer_width.mjs`, + который я прогнал и проверил на способность падать. Прогон всех 127 уместен + «когда задача задевает всё» — здесь не тот случай (§8 PROCESS.md, + issue #127). +- **`npm run golden:verify`** — не прогонял. Просмотрел + `demo/golden/matrix.mjs` целиком: ни один из golden-сценариев не открывает + `_renderOpeningDialog`/`_renderPhysicalDialog` (там есть `dialog: 'device'`, + `'decor-color'`, `'backup-full'`, `'backup-space'`, но не свойства проёма или + физического объекта) — набор физически не видит эту поверхность и не дал бы + дополнительного сигнала. `styles.ts` (общая геометрия/цвета плана) не + тронут, так что риска регресса в покрытых golden сценах тоже нет. +- **`python -m pytest tests_backend -q`** — не прогонял. `custom_components/**/*.py` + не входит в diff. +- **Performance-профили** — не прогонял. Ни AC, ни diff не называют + чувствительных к перфу путей; правка — статический HTML-атрибут в двух + местах рендера диалога. +- **Скриншот в реальном браузере (визуальная приёмка человеком)** — не делал; + вместо этого — измерение геометрии через Playwright в smoke (тот же уровень + автоматической проверки, который процесс признаёт доказательством). + +## Находки + +Нет. High: 0, Medium: 0, Low: 0. + +## Вердикт + +Все три AC доказаны исполняемым тестом с реальными числами, дисциплина «тест +должен уметь падать» подтверждена для обоих прогнанных тестов, гейты по +таблице выше зелёные, трейлеры и changelog в порядке, скоуп не расширен и не +сужен относительно решения владельца. + +**Вердикт: зелёный · цикл r1/2 · High: 0 · Medium: 0 → нет новых issue**