mirror of
https://github.com/Matysh/houseplan-card
synced 2026-10-03 21:28:59 +00:00
@@ -0,0 +1,196 @@
|
|||||||
|
# CODE-REVIEW-400-r1
|
||||||
|
|
||||||
|
- Issue: https://github.com/Matysh/houseplan-card/issues/400
|
||||||
|
- ТЗ: `docs/specs/400-beta-polish.md` (r1), ревью ТЗ зелёное (комментарий 2026-08-31T04:44:56Z)
|
||||||
|
- Диапазон: `origin/dev..HEAD`, HEAD на момент разбора и на момент вердикта — `73ecfbd1d74a6d194f62e8e83766294018f8eb81`
|
||||||
|
- Заход r1 (первый код-ревью этой задачи), блокирующих циклов израсходовано 0 из 4
|
||||||
|
|
||||||
|
## Скоуп
|
||||||
|
|
||||||
|
Три несвязанные правки из ТЗ:
|
||||||
|
|
||||||
|
1. Угловые ручки рамки декора (`.dtframe`) рисуются последними, чтобы забирать хит у осевых на мелкой мебели (AC1/AC2).
|
||||||
|
2. `_alignCandidates` в режиме `devices` исключает перетаскиваемый маркер по `_deviceDrag`, а не по мёртвому `_drag` (AC4/AC5).
|
||||||
|
3. Решение по 38 help-строкам #86 зафиксировано в `docs/ARCHITECTURE.md`: остаются в initial-чанке, с числом и причиной (AC3/AC6).
|
||||||
|
|
||||||
|
Коммиты: `60c211c8` (ТЗ), `532743b0` (документ ревью ТЗ), `fe3b85c0` (реализация,
|
||||||
|
оба changelog в одном коммите с `User-Visible: yes`), `73ecfbd1` (пересборка
|
||||||
|
бандлов + пересъёмка двух doc-скриншотов, `User-Visible: no`). Трейлеры на всех
|
||||||
|
четырёх в порядке.
|
||||||
|
|
||||||
|
## Как проверялось
|
||||||
|
|
||||||
|
Материал — `git diff origin/dev...HEAD`, без ручного тестирования. Ниже —
|
||||||
|
таблица гейтов с результатами; часть прогнана заново, часть свежих чисел взята
|
||||||
|
из хендоффа и перепроверена.
|
||||||
|
|
||||||
|
| Гейт | Команда | Результат |
|
||||||
|
|---|---|---|
|
||||||
|
| typecheck | `npx tsc --noEmit` | чисто |
|
||||||
|
| unit | `npm test` | 1668 tests, 1667 pass, 0 fail, 1 skipped — совпадает с хендоффом |
|
||||||
|
| build + сверка бандлов | `npm run build && cmp dist/houseplan-card.js custom_components/houseplan/frontend/houseplan-card.js` | совпадают; `npm run bundle:sync` не внёс изменений (дерево уже синхронизировано в `73ecfbd1`) |
|
||||||
|
| any-гейт | `node scripts/no-new-any.mjs --base origin/dev --head HEAD` | 24 добавленные строки, новых `any` нет |
|
||||||
|
| check-docs | `node scripts/check-docs.mjs` | обязателен, т.к. diff трогает `src/**`: чисто (7 файлов, 10 внешних ссылок) |
|
||||||
|
| бюджет | `npm run bundle:budget` | initial 285 425 / 300 000 — не вырос против заявленного; отдельный варнинг про запас < 15 000 Б — фоновый (#367), не следствие этой задачи |
|
||||||
|
| смок-выборка | `node scripts/smoke-select.mjs --base origin/dev --head HEAD` | 8 «прямых совпадений» по символам `_drag`/`_deviceDrag`: `smoke_align_guides`, `smoke_drag_bounds`, `smoke_decor`, `smoke_device_position_history`, `smoke_grid_snap`, `smoke_modes`, `smoke_optional_space_model`, `smoke_pan_any_zoom` |
|
||||||
|
| смоки (прямое совпадение + целевые AC) | `node demo/smoke_{align_guides,drag_bounds,decor,device_position_history,grid_snap,modes,optional_space_model,pan_any_zoom,furniture_polish}.mjs` | все 9 зелёные, прогнаны заново вживую |
|
||||||
|
| мутанты (полный, дорогой прогон — оправдан, т.к. это прямая проверка «тест умеет падать» для AC1/AC4) | `node scripts/mutation-gate.mjs --id=furniture-edge-handles-steal-the-corner` | **красный**: «тест остался зелёным на сломанном коде», поймано 0 из 1 — находка ниже |
|
||||||
|
| мутанты | `node scripts/mutation-gate.mjs --id=align-guides-exclude-dead-source` | зелёный: «тест покраснел, как обязан», поймано 1 из 1 |
|
||||||
|
|
||||||
|
Не прогонялось: `npm run golden:verify` (обоснование — см. «Что проверено»),
|
||||||
|
`python -m pytest tests_backend` (diff не трогает `custom_components/**/*.py`),
|
||||||
|
`node scripts/model-invariants.mjs` (diff не трогает геометрию/ссылки на неё —
|
||||||
|
ни рёбер комнат, ни толщины, ни `layout`, ни `marker.space`, ни `open_spans`),
|
||||||
|
performance-профили (не названы в AC, diff не на горячем пути рендера плана),
|
||||||
|
полный набор смоков (диф — два файла с точечными правками, широкая выборка
|
||||||
|
`scripts/smoke-select.mjs` уже покрывает все прямые совпадения; расширять до
|
||||||
|
всего дерева здесь непропорционально задаче).
|
||||||
|
|
||||||
|
## Находки
|
||||||
|
|
||||||
|
### Medium (в скоупе задачи — чинится в этом issue)
|
||||||
|
|
||||||
|
**M1. Мутант `furniture-edge-handles-steal-the-corner` не воспроизводит регрессию, которую обязан ловить (AC1).**
|
||||||
|
|
||||||
|
`scripts/mutation-gate.mjs:749-759`. Патч мутанта:
|
||||||
|
|
||||||
|
```
|
||||||
|
find: " ${/* #400: corners LAST."
|
||||||
|
replace: " ${/* mutant: corners no longer last."
|
||||||
|
```
|
||||||
|
|
||||||
|
Это правка **внутри JS-комментария** в `src/houseplan-card.ts:8475` — она не
|
||||||
|
меняет порядок отрисовки `sides.map(...)` / `corners.map(...)`, только текст
|
||||||
|
внутри `/* ... */`. Комментарии не влияют на поведение шаблона: физический
|
||||||
|
порядок двух блоков в исходнике остаётся прежним (`sides` перед `corners`),
|
||||||
|
и «сломанный» бандл рендерит рамку так же, как исправленный.
|
||||||
|
|
||||||
|
**Воспроизведено исполнением**, не расчётом:
|
||||||
|
|
||||||
|
```
|
||||||
|
$ node scripts/mutation-gate.mjs --id=furniture-edge-handles-steal-the-corner
|
||||||
|
ok чистый прогон: node demo/smoke_furniture_polish.mjs
|
||||||
|
FAIL furniture-edge-handles-steal-the-corner: тест остался зелёным на сломанном коде
|
||||||
|
поймано 0 из 1
|
||||||
|
```
|
||||||
|
|
||||||
|
Второй мутант (`align-guides-exclude-dead-source`) свою регрессию ловит верно
|
||||||
|
(«поймано 1 из 1», проверено тем же способом) — дефект локален к первому пункту.
|
||||||
|
|
||||||
|
Коммит `fe3b85c0` утверждает «Both mutants run by hand: reverting the paint
|
||||||
|
order reddens the 40 cm probe» — это, видимо, было проверено вручную корректной
|
||||||
|
правкой (перестановкой блоков), но то, что легло в постоянный реестр
|
||||||
|
`scripts/mutation-gate.mjs` (файл класса B, который будет гонять
|
||||||
|
`.github/workflows/mutation-gate.yml` перед каждым стабильным релизом без
|
||||||
|
участия человека), эту регрессию не ловит. AC1 в ТЗ прямо называет мутант как
|
||||||
|
доказательство «тест умеет падать» — сейчас это неверно для постоянного
|
||||||
|
артефакта, а не только для разового ручного прогона.
|
||||||
|
|
||||||
|
**Почему Medium, а не High**: сама функциональная правка верна и доказана
|
||||||
|
живым браузерным смоком на реальном DOM (`smallFurnitureCornerWinsTheHit: true`,
|
||||||
|
`largeFurnitureKeepsBothHandles: true` — прогнано заново вживую, не только по
|
||||||
|
хендоффу). Пользователь получает исправленное поведение уже сейчас. Ломается
|
||||||
|
не продукт, а будущая защита от регресса — обнаружится не раньше, чем перед
|
||||||
|
следующим стабильным релизом, когда её прогонят по-настоящему, и к тому
|
||||||
|
моменту будет поздно понять, что она никогда не работала.
|
||||||
|
|
||||||
|
**Ожидаемо**: патч мутанта должен реально возвращать `corners` перед `sides`
|
||||||
|
(или иным способом менять фактический порядок рендера), а не текст комментария.
|
||||||
|
Проверка приёмки: `node scripts/mutation-gate.mjs --id=furniture-edge-handles-steal-the-corner` → «поймано 1 из 1».
|
||||||
|
|
||||||
|
### Low (снимаются с записью, доработки не требуют)
|
||||||
|
|
||||||
|
**L1. AC4 доказан не тем видом теста, что назван в ТЗ.**
|
||||||
|
|
||||||
|
План тестов ТЗ называет для AC4 «юнит на `_alignCandidates`»
|
||||||
|
(`test/align-candidates.test.mjs` либо существующий файл). Такого юнит-теста в
|
||||||
|
диффе нет — `git diff --stat -- test/` пуст. Вместо этого AC4 проверяется
|
||||||
|
браузерным смоком `demo/smoke_align_guides.mjs` (`devGuideComesFromAnotherMarker`),
|
||||||
|
который вызывает тот же `_alignCandidates()` напрямую на живом инстансе и
|
||||||
|
сравнивает списки кандидатов с активным `_deviceDrag` и без — по существу то
|
||||||
|
же утверждение, что просило ТЗ, просто через e2e, а не через изолированный
|
||||||
|
юнит. Проверено исполнением: смок зелёный, а отрицательный прогон мутанта
|
||||||
|
`align-guides-exclude-dead-source` подтверждает, что проверка умеет падать.
|
||||||
|
Снимаю без правок: доказательство сильнее, чем просило ТЗ (реальный DOM вместо
|
||||||
|
изолированного вызова), просто не в той форме файла.
|
||||||
|
|
||||||
|
**L2. Число «2 654 Б gzip» для 38 help-строк в `docs/ARCHITECTURE.md` не
|
||||||
|
перепроверено независимо.**
|
||||||
|
|
||||||
|
Прочитано и признано правдоподобным (38 ключей, общий вес словарей 47 754 Б
|
||||||
|
gzip, итоговый бюджет 285 425 — совпадает с хендоффом), но точный вес именно
|
||||||
|
этих 38 ключей отдельно не измерялся мной — потребовал бы отдельного скрипта
|
||||||
|
подсчёта, которого в репозитории нет. AC3 требует записанного решения с
|
||||||
|
причиной, а не независимого замера ревьюером; решение и причина на месте.
|
||||||
|
Снимаю без правок.
|
||||||
|
|
||||||
|
## Что проверено и корректно
|
||||||
|
|
||||||
|
- **AC1/AC2** (приоритет угловой ручки на мелкой мебели, отсутствие регрессии
|
||||||
|
на крупной) — доказаны исполнением: `node demo/smoke_furniture_polish.mjs`
|
||||||
|
зелёный, диагностика `handleProbeDiagnostics` показывает ровно одно
|
||||||
|
срабатывание на угловой ручке и ноль на осевой для 40 см и для 160 см.
|
||||||
|
Порядок отрисовки в `src/houseplan-card.ts:8467-8485` подтверждён чтением:
|
||||||
|
`sides.map` идёт раньше `corners.map`, радиусы хита идентичны (`hr` общий на
|
||||||
|
`:8440`).
|
||||||
|
- **AC4** — доказан исполнением через смок (см. L1); чтением подтверждено, что
|
||||||
|
`_alignCandidates` (`src/houseplan-editor-runtime.ts:10970-10979`) теперь
|
||||||
|
берёт `this.host._deviceDrag?.id ?? this.host._drag?.id`, и что в режиме
|
||||||
|
`devices` `_drag` действительно остаётся `null` (не тронуто этим диффом,
|
||||||
|
проверено чтением `houseplan-card.ts:2547` и присваиваний `_drag` — ни одно
|
||||||
|
не относится к ветке `devices`).
|
||||||
|
- **AC5** — доказан исполнением, включая отрицательный прогон мутанта
|
||||||
|
(«поймано 1 из 1»): смок различает направляющую от другого маркера и от
|
||||||
|
себя, сравнением списков кандидатов, а не по заранее посчитанной точке —
|
||||||
|
учтён риск гонки координат, названный в ТЗ.
|
||||||
|
- **AC3** — решение (2) «en/ru неделимы» записано в `docs/ARCHITECTURE.md` с
|
||||||
|
причиной (стоимость раздельной поставки против экономии), проверено чтением.
|
||||||
|
- **AC6** — бюджет initial не вырос: `npm run bundle:budget` = 285 425 / 300 000,
|
||||||
|
прогнано заново, совпадает с хендоффом.
|
||||||
|
- Интерфейс `HouseplanEditorHostPort._deviceDrag` — новое поле только в типе,
|
||||||
|
реальное приватное поле `_deviceDrag` в `HouseplanCard` существовало и до
|
||||||
|
этого диффа (`houseplan-card.ts:2548`, не изменено) — подтверждено чтением,
|
||||||
|
`tsc --noEmit` подтверждает совместимость типов.
|
||||||
|
- Трейлеры: все четыре коммита несут `Issue: #400` и корректный
|
||||||
|
`User-Visible:`; `User-Visible: yes` (`fe3b85c0`) правит оба changelog в том
|
||||||
|
же коммите — подтверждено `git show --stat`.
|
||||||
|
- Видимых числовых значений, которые пользователь видел бы дважды, в этом
|
||||||
|
диффе нет (правки — хит-тестинг и внутренний список кандидатов
|
||||||
|
направляющих, не отображаемые величины) — «одно число, один источник» не
|
||||||
|
применимо.
|
||||||
|
- Диф не трогает геометрию комнат/толщины/`layout`/`marker.space`/`open_spans`
|
||||||
|
— инварианты модели не требуются.
|
||||||
|
|
||||||
|
## Чего не проверял и почему
|
||||||
|
|
||||||
|
- **`npm run golden:verify`** — не прогонял. Обоснование: правка (1) меняет
|
||||||
|
DOM-порядок двух групп невидимых (прозрачных) хит-кругов и парных с ними
|
||||||
|
`.dtknob`; сами бусины позиционно не пересекаются друг с другом (углы и
|
||||||
|
середины сторон — разные точки), поэтому перестановка групп не может менять
|
||||||
|
итоговые пиксели рамки редактирования декора. Риск назван и разобран в самом
|
||||||
|
ТЗ («перестановка не должна менять картинку»). Golden-баз для оверлея рамки
|
||||||
|
редактирования декора в матрице предположительно нет (оверлей — состояние
|
||||||
|
выделения в редакторе, а не View); не проверял это утверждение отдельным
|
||||||
|
прогоном golden, полагаюсь на разбор по коду.
|
||||||
|
- **`python -m pytest tests_backend`** — не прогонял, diff не касается
|
||||||
|
`custom_components/**/*.py`.
|
||||||
|
- **`node scripts/model-invariants.mjs`** — не прогонял, diff не касается
|
||||||
|
геометрии.
|
||||||
|
- **Полный `ls demo/smoke_*.mjs` (209 смоков)** — не прогонял; прогнаны 9,
|
||||||
|
названные `scripts/smoke-select.mjs` как прямые совпадения, плюс целевые
|
||||||
|
`align_guides`/`furniture_polish` из AC. Полный набор — предрелизный гейт.
|
||||||
|
- **Точный байтовый вес 38 help-строк отдельно** — не перемерял (см. L2).
|
||||||
|
- **Ручное DOM-тестирование в браузере вне смоков** — не проводилось; в этом
|
||||||
|
процессе фазы ручного тестирования нет, автотесты и код-ревью её заменяют
|
||||||
|
(см. таблицу гейтов выше).
|
||||||
|
|
||||||
|
## Итог
|
||||||
|
|
||||||
|
Один Medium в скоупе задачи (M1), без High — жёлтый вердикт. Функциональные
|
||||||
|
правки (1) и (2) верны и доказаны исполнением; постоянный регресс-гейт для
|
||||||
|
пункта (1) сломан и не защищает от отката ровно той регрессии, которую
|
||||||
|
описывает ТЗ. Правка ожидается в этом же issue — заменить патч мутанта
|
||||||
|
`furniture-edge-handles-steal-the-corner` на такой, что реально возвращает
|
||||||
|
`corners` перед `sides`, и подтвердить прогоном
|
||||||
|
`node scripts/mutation-gate.mjs --id=furniture-edge-handles-steal-the-corner`
|
||||||
|
(«поймано 1 из 1»).
|
||||||
Reference in New Issue
Block a user