mirror of
https://github.com/Matysh/houseplan-card
synced 2026-09-29 03:09:36 +00:00
committed by
Sergey Matyunin
parent
5ed2821161
commit
1937c32572
@@ -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` на `<hp-dialog>` в
|
||||
`_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` к открывающему тегу `<hp-dialog>`), подтверждено
|
||||
постатейным чтением 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**
|
||||
Reference in New Issue
Block a user