mirror of
https://github.com/Matysh/houseplan-card
synced 2026-09-30 19:58:50 +00:00
committed by
Sergey Matyunin
parent
2ef32417bd
commit
07b0b3dec2
@@ -0,0 +1,229 @@
|
||||
# CODE-REVIEW-195-r1
|
||||
|
||||
- **Issue:** https://github.com/Matysh/houseplan-card/issues/195
|
||||
- **Трек:** `trivial` (короткий, §5.1 PROCESS.md) — ТЗ живёт в теле issue,
|
||||
AC записаны автором в блоке «Диагностика и AC короткого трека (2026-08-19)»
|
||||
до перевода в `S5-ready`. Файла `docs/specs/195-*.md` нет и не требуется.
|
||||
- **Диапазон:** `git log --oneline origin/dev..HEAD` /
|
||||
`git diff origin/dev...HEAD` — два коммита на ветке
|
||||
`issue/195-editor-close-hit-target`:
|
||||
- `140a56f` `fix: enlarge editor close hit target` (`Issue: #195`,
|
||||
`User-Visible: yes`)
|
||||
- `a169cc6` `test: sample editor close glyph before removal` (`Issue: #195`,
|
||||
`User-Visible: no`)
|
||||
- **Роль:** ревьюер кода (не автор), этап `S7-code-review`
|
||||
- **Цикл:** r1/2 (лимит короткого трека). Два предыдущих запуска
|
||||
автоматического конвейера (comments 2026-08-19T10:47:37Z, 10:51:30Z) упали
|
||||
инфраструктурно, без публикации вердикта — по §4 PROCESS.md это не
|
||||
расходует цикл.
|
||||
|
||||
## Скоуп ревью
|
||||
|
||||
Диагностика в issue (аналитика 2026-08-19) подтвердила ровно одну причину
|
||||
бага «крестик не закрывает редактор с первого клика» — К1: фактическая
|
||||
DOM hit-zone `.modetab .closex` равна `13×13 px`, промах в 2–3 px попадает в
|
||||
кнопку активной вкладки и превращается в документированный no-op
|
||||
(`docs/UX-MODES.md`). Кандидаты К2 (незавершённая цепочка стен) и К3 (клик во
|
||||
время перехода режима) не подтвердились и явным решением автора **не
|
||||
переписываются** — только закрепляются регрессионными проверками. Ревью
|
||||
проверяло:
|
||||
|
||||
- что продуктовая правка ограничена CSS `.modetab .closex` и не касается
|
||||
`_setMode`/`_finishWallChain` (иначе трек `trivial` был бы неверным выбором
|
||||
и надо было возвращать issue в `S3-spec`, §5.1);
|
||||
- что новый hit-target `≥24×24 px` не сдвигает и не меняет высоту/перенос
|
||||
header modes-строки (контракт AC1, `docs/UX-MODES.md`);
|
||||
- что все три AC действительно доказаны исполняемым тестом, а не только
|
||||
заявлены, и что этот тест умеет падать;
|
||||
- трейлеры, классы файлов, оба changelog в одном `User-Visible: yes`
|
||||
коммите (§10 AGENTS.md).
|
||||
|
||||
## Как проверялось
|
||||
|
||||
1. Прочитан весь diff: `git diff origin/dev...HEAD --stat` и файлы по
|
||||
отдельности. Продуктовый код класса A — только `src/styles.ts`, 10
|
||||
изменённых строк в одном селекторе `.modetab .closex`. `src/houseplan-card.ts`
|
||||
не тронут (`git diff origin/dev...HEAD -- src/houseplan-card.ts` — 0 строк),
|
||||
что подтверждает решение автора не переписывать К2/К3.
|
||||
2. Прочитан `docs/SCOPE.md`: правка — не новая функциональность, а починка
|
||||
существующего интерфейса выхода из редактора, часть основы J6/UX-MODES
|
||||
(«View mode is the product», editors admin-only). В скоупе.
|
||||
3. Прочитан `docs/UX-MODES.md` и `docs/USER-GUIDE.ru.md` — термин «крестик
|
||||
активного редактора» (USER-GUIDE.ru.md:166) совпадает с формулировкой в
|
||||
изменённых `docs/UX-MODES.md`, `docs/CHANGELOG.ru.md`.
|
||||
4. Прочитан `AGENTS.md`/`PROCESS.md` §5.1, §8, §10 — критерии короткого трека
|
||||
(тип `bug`, одна поверхность, без миграций/i18n/perf/touch-контракта, AC ≤3,
|
||||
поведение уже зафиксировано) сверены построчно с фактическим diff и issue —
|
||||
выполнены.
|
||||
5. Прочитан весь текст issue #195 и все комментарии (`gh issue view 195
|
||||
--comments`): исходный отчёт владельца, аналитика автора (К1 подтверждён,
|
||||
К2/К3 — нет), два хендоффа реализации (первый и скорректированный после
|
||||
найденного автором дефекта собственного теста), два упавших инфраструктурно
|
||||
прогона ревью.
|
||||
6. Пересчитана вручную CSS-геометрия нового правила `.modetab .closex`
|
||||
(`width/height: 24px`, `box-sizing: border-box`,
|
||||
`margin: -5.5px -5.5px -5.5px -3.5px`): при неизменном `gap: 6px` между
|
||||
flex-элементами `.modetab` итоговая позиция 13-пиксельного глифа
|
||||
(центрируемого `justify-content/align-items: center` внутри нового бокса)
|
||||
совпадает пиксель-в-пиксель со старой позицией (глиф равен border-box
|
||||
старого правила `13×13` + `margin-left: 2px`), а суммарный вклад элемента в
|
||||
поток (`width + marginLeft + marginRight = 15px`, `height + marginTop +
|
||||
marginBottom = 13px`) идентичен старому. Проверено чтением/расчётом, не
|
||||
исполнением — дополнительно подтверждено эмпирически в п.9.
|
||||
7. Прогнаны обязательные дешёвые гейты лично (не переиспользована декларация
|
||||
автора) — см. таблицу ниже.
|
||||
8. Для каждого нового AC-утверждения в `demo/smoke_editor_tabs.mjs`
|
||||
целенаправленно откачен продуктовый CSS-фикс (`git show 140a56f^:src/styles.ts`
|
||||
восстановлен во временную рабочую копию, бандл пересобран и скопирован в
|
||||
`demo/srv/assets/`) и подтверждено, что смок падает именно на новой проверке
|
||||
`tabCrossTargetsAtLeast24` (`FAILED (1): tabCrossTargetsAtLeast24: expected
|
||||
true, got false`, exit code 1); затем рабочее дерево восстановлено к
|
||||
состоянию коммита (`git status` чист), бандл пересобран заново — три копии
|
||||
совпадают и идентичны варианту до отката.
|
||||
9. Независимой ad hoc проверкой (page.evaluate вне закоммиченного смока)
|
||||
измерены `getBoundingClientRect()` для `.hdr`, `.modes`, активной
|
||||
`.modetab` до и после входа в редактор: высота `.modetab` (`27px`) и
|
||||
`.modes` (`35px`) не меняется при активации вкладки, а прямоугольник
|
||||
расширенного `.closex` (`24×24`) полностью лежит внутри границ своей
|
||||
`.modetab` (`crossOverflowsTabVertically: false`,
|
||||
`crossOverflowsTabHorizontally: false`) — эмпирическое подтверждение расчёта
|
||||
из п.6, устраняющее опасение, что отрицательный margin может вытолкнуть
|
||||
бокс за пределы кнопки или строки табов.
|
||||
10. Проверены трейлеры и классы файлов: `git show 140a56f --stat` /
|
||||
`git show a169cc6 --stat` — `Issue: #195` на обоих, `User-Visible: yes`
|
||||
только на продуктовом коммите, оба changelog (`docs/CHANGELOG.md`,
|
||||
`docs/CHANGELOG.ru.md`) редактируются в том же коммите `140a56f`, что и
|
||||
CSS-фикс.
|
||||
11. Прочитан второй коммит `a169cc6` целиком: исправляет реальный дефект теста
|
||||
(computed style глифа читался **после** клика, когда активный `.closex`
|
||||
уже удалён из DOM) — не сокрытие, а корректная правка измерения,
|
||||
аналогично прецеденту #89 (правка фикстуры/измерения при доказанном
|
||||
дефекте в нём самом, не в проверяемом коде).
|
||||
|
||||
## Обязательные гейты (всегда)
|
||||
|
||||
| Гейт | Команда | Результат |
|
||||
|---|---|---|
|
||||
| Typecheck | `npx tsc --noEmit` | чисто, без вывода |
|
||||
| Unit-тесты | `npm test` | `900/900`, 0 fail |
|
||||
| Build + сверка бандлов | `npm run build` затем `cmp dist/houseplan-card.js custom_components/houseplan/frontend/houseplan-card.js` и `cmp dist/houseplan-card.js demo/srv/assets/houseplan-card.js` | оба `cmp` без вывода — три копии идентичны; `git status --short` после сборки пуст (рабочее дерево уже содержало актуальный бандл) |
|
||||
|
||||
Falsifiability (тест должен уметь падать), применена к прогнанному browser
|
||||
smoke:
|
||||
|
||||
- временно восстановлена дореформенная версия `.modetab .closex`
|
||||
(`margin-left: 2px`, без `width/height: 24px`) → `demo/smoke_editor_tabs.mjs`
|
||||
корректно упал ровно на новой проверке `tabCrossTargetsAtLeast24` (exit
|
||||
code 1); остальные 47 проверок остались зелёными, что ожидаемо — они не
|
||||
зависят от размера hit-zone. Код восстановлен, бандл пересобран, три копии
|
||||
совпадают, `git status` чист.
|
||||
|
||||
## Гейты по необходимости
|
||||
|
||||
| Гейт | Почему запускался / не запускался | Результат |
|
||||
|---|---|---|
|
||||
| `node demo/smoke_editor_tabs.mjs` | назван прямо в AC1–AC3 issue как способ доказательства, единственный смок из 127, затрагивающий изменённую поверхность (`.modetab`, header editor tabs) | зелёный, все 48 проверок `true`; falsifiability подтверждена выше |
|
||||
| Остальные 126 `demo/smoke_*.mjs` | diff — один CSS-селектор на header editor tabs, не затрагивает остальные поверхности (canvas, диалоги, устройства и т.д.); полный прогон соразмерен только задачам, задевающим всё (PROCESS.md §8) | не прогонялись — сознательное сужение |
|
||||
| `npm run golden:verify` | AC1 требует «геометрия/переносы header modes не меняются». Ручной пересчёт CSS-геометрии (п.6) и эмпирическая проверка рендера (п.9) показывают, что видимая позиция 13 px глифа и размеры `.modetab`/`.modes` пиксель-в-пиксель совпадают со старой версией — видимый результат не меняется, только невидимая hit-zone. Существующих golden-сценариев, специально нацеленных на header modes tabs, в матрице нет (`grep` по `demo/golden/*` ничего не нашёл) | не запускался — обоснованное сужение, задокументировано как решение, а не молчаливый пропуск |
|
||||
| `python -m pytest tests_backend -q` | diff не касается `custom_components/**/*.py` (`git diff --stat` подтверждает) | не запускался — неприменим |
|
||||
| performance-профили | AC не называют производительность, правка — статический CSS без анимаций/новых расчётов в hot path | не запускались — неприменимо |
|
||||
|
||||
## Что проверено и корректно
|
||||
|
||||
- **AC1 (DOM + browser smoke, hit-zone ≥24×24, глиф 13 px, layout не
|
||||
меняется).** Для всех трёх активных вкладок (`Plan editor`, `Device editor`,
|
||||
`Background editor`) `.closex.getBoundingClientRect()` даёт `24×24`,
|
||||
`--mdc-icon-size` остаётся `13px`, а клик по расширенной зоне вне старого
|
||||
13 px глифа одним действием переводит карточку в `view`
|
||||
(`tabCrossTargetsAtLeast24`, `tabCrossGlyphStays13`, `tabCrossExpandedEdgeWorks`,
|
||||
`tabCrossWorks` — все `true`, п.8 подтверждает падение до фикса). Геометрия
|
||||
header-строки не меняется — подтверждено расчётом (п.6) и независимым
|
||||
измерением (п.9): высота `.modetab`/`.modes` идентична до/после активации
|
||||
вкладки, расширенный бокс `.closex` не выходит за границы своей кнопки.
|
||||
- **AC2 (клик по активной вкладке вне hit-zone остаётся no-op; клик X во время
|
||||
`_modeTransitionBusy` завершает переход).** `reclickNoop` подтверждает общий
|
||||
no-op контракт клика по кнопке вкладки (не изменён этой правкой — обработчик
|
||||
`_setMode(m)` с ранним return по-прежнему единственный путь). Новые проверки
|
||||
`tabCrossCloseStartsDuringEnter`/`tabCrossCloseDuringEnterWorks` подтверждают,
|
||||
что клик по `.closex` входящей (ещё анимирующейся) вкладки завершает переход
|
||||
в `view` — воспроизводит вывод аналитики «К3 не подтвердился» тестом, а не
|
||||
только текстом.
|
||||
- **AC3 (незавершённая Walls chain закрывается тем же кликом; при
|
||||
`MAX_PARTITIONS` редактор остаётся открыт с тостом, без потери draft).**
|
||||
`tabCrossFinishesWallChain` подтверждает материализацию двухсегментного
|
||||
черновика в `partitions` и переход в `view` тем же кликом по X;
|
||||
`tabCrossLimitKeepsDraftWithFeedback` подтверждает, что при насильно
|
||||
переполненном `partitions` (`new Array(2000)`) карточка остаётся в `plan`,
|
||||
черновик (`_path.length === 2`) не теряется, и показывается существующий
|
||||
локализованный тост `toast.physical_limit`. Дополнительно подтверждено
|
||||
чтением: `src/houseplan-card.ts` не изменился, значит guard
|
||||
`_finishWallChain`/`MAX_PARTITIONS` — тот же код, что был в dev, поведение не
|
||||
переписано, только закреплено регрессией.
|
||||
- **Причина бага устранена корректно, а не замаскирована.** Обработчик
|
||||
`.closex` (`houseplan-card.ts:15484-15486`) — отдельный элемент с
|
||||
`stopPropagation`; расширение именно его border-box до `24×24` — это
|
||||
расширение реальной кликабельной area браузера для этого узла, а не хак
|
||||
через `::before`/псевдоэлемент с ручным перехватом координат. Решение
|
||||
устраняет саму причину К1 (промах координат в родительскую кнопку), а не
|
||||
прячет симптом.
|
||||
- **Второй коммит (`a169cc6`) — легитимная правка теста.** Дефект
|
||||
(«computed style глифа читался после клика, когда `.closex` уже удалён из
|
||||
DOM») найден и исправлен самим автором до передачи на ревью; исправление
|
||||
меняет порядок измерения, а не ослабляет проверку — эквивалент прецедента
|
||||
#89 (правка доказанного дефекта в фикстуре/измерении, не сокрытие).
|
||||
- **Трейлеры/классы файлов/changelog.** `140a56f`: `Issue: #195`,
|
||||
`User-Visible: yes`, `docs/CHANGELOG.md` и `docs/CHANGELOG.ru.md` в том же
|
||||
коммите, три копии бандла (`dist/`, `custom_components/houseplan/frontend/`,
|
||||
`demo/srv/assets/`) синхронны. `a169cc6`: `Issue: #195`,
|
||||
`User-Visible: no`, касается только `demo/smoke_editor_tabs.mjs` — корректно
|
||||
без changelog.
|
||||
- **Терминология.** `docs/CHANGELOG.md`/`.ru.md`, `docs/UX-MODES.md`,
|
||||
`docs/STATUS.md` используют формулировку, совпадающую с
|
||||
`docs/USER-GUIDE.ru.md:166` («крестик активного редактора»), новых терминов
|
||||
интерфейса не изобретено.
|
||||
- **Скоуп не расширен.** Продуктовый diff — 10 строк в одном CSS-селекторе;
|
||||
К2/К3 не переписаны, toggle-поведение активной вкладки не введено (это было
|
||||
явно продуктовым вопросом в issue и аналитик решил его сам —
|
||||
«Toggle-поведение активной вкладки не вводить» — корректно, т.к. это решение
|
||||
не наблюдаемо пользователем как новое поведение, а лишь выбор одного из
|
||||
предложенных в issue направлений фикса, не требующий эскалации владельцу).
|
||||
|
||||
## Находки
|
||||
|
||||
Находок нет. High: 0, Medium: 0, Low: 0.
|
||||
|
||||
## Чего не проверял
|
||||
|
||||
- **Полный набор `demo/smoke_*.mjs` (127 файлов).** Diff — один CSS-селектор
|
||||
на header editor tabs; остальные смоки проверяют не связанные поверхности
|
||||
(canvas, диалоги устройств, декор, стены и т.д.), которые эта правка не
|
||||
трогает. Прогон только `smoke_editor_tabs.mjs`, единственного смока по
|
||||
затронутой поверхности — соразмерное решение по PROCESS.md §8.
|
||||
- **`npm run golden:verify`.** Не запускался. Обоснование — не молчаливый
|
||||
пропуск: ручной пересчёт CSS box-модели (п.6) и независимое измерение
|
||||
реального рендера (п.9) показывают, что видимая позиция глифа и размеры
|
||||
header modes-строки не меняются ни на пиксель, то есть критерий «менялся
|
||||
визуал» из PROCESS.md §8 не выполнен для этой конкретной правки. Полный
|
||||
golden-прогон всё равно случится перед бетой (предрелизный гейт) и покажет
|
||||
любое расхождение, если этот анализ ошибочен.
|
||||
- **`python -m pytest tests_backend -q` и performance-профили.** Не
|
||||
применимы — diff не касается `custom_components/**/*.py` и не имеет
|
||||
измеримого влияния на производительность (статический CSS, без изменений в
|
||||
hot path рендера/анимаций).
|
||||
- **Multi-touch/реальное тач-устройство.** Issue и аналитика прямо
|
||||
зафиксировали, что touch-контракт не меняется (`docs/TOUCH-SUPPORT.md`:
|
||||
editors — desktop-first, best effort); `24×24` — WCAG-минимум, названный в
|
||||
самом issue, не HA-тач-рекомендация 40 px, и это соответствует заявленному
|
||||
контракту, а не занижает его.
|
||||
|
||||
## Вердикт
|
||||
|
||||
Зелёный. High: 0, Medium: 0. Все три AC доказаны исполняемым
|
||||
`demo/smoke_editor_tabs.mjs`, ревьюер лично прогнал смок, подтвердил падение
|
||||
целевой проверки на дореформенном CSS и убедился, что после восстановления
|
||||
фикса дерево идентично коммиту. Продуктовая правка ограничена одним CSS-
|
||||
селектором, не переписывает подтверждённо-нерелевантные пути (К2/К3),
|
||||
геометрия header modes-строки пиксель-в-пиксель сохранена (проверено расчётом
|
||||
и независимым измерением). Трейлеры, классы файлов и оба changelog в
|
||||
одном `User-Visible: yes` коммите — в порядке. Задача готова к слиянию в
|
||||
`dev` без возврата на правки.
|
||||
Reference in New Issue
Block a user