mirror of
https://github.com/Matysh/houseplan-card
synced 2026-09-29 03:09:36 +00:00
claude[bot]
@@ -0,0 +1,232 @@
|
||||
# CODE-REVIEW-229-r2
|
||||
|
||||
Issue: [#229](https://github.com/Matysh/houseplan-card/issues/229) — «прямая стена, нарисованная в несколько кликов, остаётся одной записью, а не рядом швов».
|
||||
Заход: r2. Ревью ТЗ прошло тремя заходами (r1 M×3 → r2 M×1 → r3 зелёный, SHA `1ecd267`).
|
||||
Код-ревью: r1 — красный (High×2, SHA `50a00e4`), r2 — этот документ, SHA `ed13b4a`.
|
||||
Блокирующих циклов до этого раунда: 1/4.
|
||||
|
||||
## Скоуп раунда (§2.10)
|
||||
|
||||
Предыдущий заход (r1) получен на диапазоне `origin/dev...HEAD` при `HEAD = 50a00e4`
|
||||
(коммиты `e6be43b` feat + `50a00e4` docs-скриншоты). SHA не был назван в самом
|
||||
тексте вердикта — фиксирую его здесь по факту: комментарий-вердикт содержит только
|
||||
имена коммитов, точку HEAD восстановил из истории веток (`git log --oneline
|
||||
origin/dev..HEAD`, коммит непосредственно перед фикс-коммитом `ed13b4a`).
|
||||
|
||||
Дельта раунда — `git diff 50a00e4..HEAD` (коммит `ed13b4a`, один коммит):
|
||||
|
||||
```
|
||||
custom_components/houseplan/frontend/houseplan-card.js | 6 +- (сгенерировано)
|
||||
demo/srv/assets/houseplan-card.js | 6 +- (сгенерировано)
|
||||
dist/houseplan-card.js | 6 +- (сгенерировано)
|
||||
docs/images/screenshots.json | 22 +- (отпечаток источника, ожидаемо)
|
||||
docs/reviews/CODE-REVIEW-229-r1.md | 282 ++ (публикация документа r1, не код)
|
||||
demo/smoke_wall_chain_merge.mjs | 22 +-
|
||||
scripts/mutation-gate.mjs | 25 ++
|
||||
src/houseplan-card.ts | 9 +-
|
||||
src/plan-optimizer.ts | 7 +-
|
||||
test/plan-optimizer.test.mjs | 20 ++
|
||||
```
|
||||
|
||||
Дельта локальна: один коммит, пять содержательных файлов, оба High из r1 и
|
||||
только они. Продуктовая рамка не менялась, новая подсистема не задета, ребейза
|
||||
не было (`50a00e4` — предок `HEAD` напрямую). Полный повторный разбор не требуется
|
||||
по правилу §2.10 — разбор ограничен дельтой плюс тем, до чего дельта дотягивается
|
||||
(AC2 и §8.6/AC8, единственные AC, чьё доказательство эта правка меняет).
|
||||
|
||||
## Закрытие раунда r1
|
||||
|
||||
| Находка r1 | Чем закрыта | Где это видно |
|
||||
|---|---|---|
|
||||
| **High-1**: `_mergeSpacePartitions` (houseplan-card.ts) и `optimizePlans` (plan-optimizer.ts) делили уже сырой полигон комнаты (`roomPoly`) на `NORM_W` второй раз — комната «улетала» в область ~0.0001, T-стык к её стороне не находился | Обе обвязки перестали делить `roomPolygons` на `NORM_W`: `src/houseplan-card.ts:6557-6560`, `src/plan-optimizer.ts:507-510`. Комментарий на месте объясняет, что `roomPoly` уже возвращает координаты в масштабе перегородок | Прочитано и перепроверено: `roomPoly` (`src/logic.ts:103-108`) отдаёт `r.x/r.y/r.w/r.h` без масштабирования; `sp.partitions[i].a` в `_finishWallChain` — тоже сырые координаты (`segment.a[0] / NORM_W`, `houseplan-card.ts:6609`). Т.е. деление было лишним объективно, не только по утверждению автора. Дополнительно воспроизведено исполнением: новый юнит `test/plan-optimizer.test.mjs:497-516` («a node on the side of a room survives the sweep») зелёный на текущем коде, красный при ручном возврате деления (см. «Как проверялось») |
|
||||
| **High-2**: `_finishWallChain` вызывает слияние (`houseplan-card.ts:6612` тогда) раньше, чем убирает завершаемый черновик из `sp.room_drafts`, поэтому собственные концы черновика ложно считались чужим примыканием | `draftEnds` теперь исключает запись с `id === this._activeDraftId` (`src/houseplan-card.ts:6566`), тем же паттерном, что `plan-snap-overlay.ts` | Прочитано: исключение стоит до `this._activeDraftId = null` (строка 6626), то есть значение ещё актуально в момент вызова. Воспроизведено исполнением: `demo/smoke_wall_chain_merge.mjs` теперь рисует продолжение реальными кликами (`click()`, строки 53-62) вместо присвоения `_path`, и это зелёное; при ручном откате исключения (снятие условия `if (draft?.id === this._activeDraftId) return [];`) смок красный — подтверждено запуском мутанта `chain-merge-sees-own-draft` через `node scripts/mutation-gate.mjs --id=chain-merge-sees-own-draft` (см. ниже) |
|
||||
|
||||
Обе High-находки закрыты по существу, не заплаткой: в обоих случаях автор
|
||||
объяснил причину (общая система координат `roomPoly`/`partitions`; порядок
|
||||
операций в `_finishWallChain`) и добился падения теста на прежнем коде, а не
|
||||
подобрал совпадающие числа постфактум — я это проверил самостоятельно (см. ниже).
|
||||
|
||||
## Унаследовано из r1
|
||||
|
||||
Из документа `docs/reviews/CODE-REVIEW-229-r1.md` (SHA `50a00e4`) принято без
|
||||
повторной проверки — дельта не касается их доказательной поверхности:
|
||||
|
||||
- **AC1** (прямая цепочка — одна запись) — не тронут; `demo/smoke_wall_chain_merge.mjs`
|
||||
проверки `straightRunIsOneRecord`/`straightRunSpansTheChain`/`straightRunKeepsThickness`
|
||||
не менялись.
|
||||
- **AC3** (проём не двигается, оба представления — `host.t` и legacy `x/y/angle`) —
|
||||
`wall-merge.ts` и его юниты не менялись в этой дельте.
|
||||
- **AC4** (разная толщина не сращивается), **AC5** (допуски `EPS_JOIN`),
|
||||
**AC6** (детерминизм по порядку) — та же пара, `wall-merge.ts`/`wall-merge.test.mjs`
|
||||
вне дельты.
|
||||
- **AC7** (оптимизатор сращивает накопленное, идемпотентность) — логика сведения
|
||||
накопленных швов в `plan-optimizer.ts` не менялась, изменилась только область
|
||||
видимости комнат внутри той же функции.
|
||||
- **AC9** (release-артефакты) — не применим к этому коммиту: `User-Visible: no`,
|
||||
changelog не требуется; сам факт трейлера перепроверен (см. «Гейты»).
|
||||
- Общая корректность структуры ТЗ, i18n, touch-контракт (`Touch editor: not exposed`) —
|
||||
вне дельты кода, r1 их не пересматривал.
|
||||
|
||||
## Что проверялось и как
|
||||
|
||||
**Всегда (быстрые гейты, прогнаны заново на `ed13b4a`):**
|
||||
|
||||
| Гейт | Команда | Результат |
|
||||
|---|---|---|
|
||||
| Типы | `npx tsc --noEmit` | чисто |
|
||||
| Юниты | `npm test` | 1004/1004, 0 fail |
|
||||
| Сборка + сверка бандла | `npm run build` + `cmp` трёх копий (`dist`, `custom_components/houseplan/frontend`, `demo/srv/assets`) | идентичны байт-в-байт, дерево чистое после копирования |
|
||||
|
||||
**По необходимости, определённой дельтой:**
|
||||
|
||||
- `node demo/smoke_wall_chain_merge.mjs` — единственный смок, прямо изменённый
|
||||
дельтой (H2) и покрывающий сценарий High-1/High-2 на живом клик-пути. Зелёный,
|
||||
9/9 проверок.
|
||||
- Соседние смоки того же семейства (тронутый `_finishWallChain`/`_mergeSpacePartitions`
|
||||
и оптимизатор): `optimize_coordinate_canonicalization`, `wall_junctions`,
|
||||
`partition_openings`, `free_walls` — зелёные, регрессий не внесено.
|
||||
- `node scripts/check-docs.mjs` — прошёл (7 файлов, отпечаток `docs/images/screenshots.json`
|
||||
синхронен с пересобранным бандлом — сам факт изменения этого файла в дельте ожидаем,
|
||||
не дефект).
|
||||
- `node scripts/process-gate.mjs` — пройден, 0 предупреждений; трейлеры `Issue: #229` /
|
||||
`User-Visible: no` на `ed13b4a` корректны, changelog не требовался и не менялся.
|
||||
- **Дисциплина «тест умеет падать» — проверена исполнением, не на словах,** для обеих
|
||||
High-находок:
|
||||
- вручную вернул деление на `NORM_W` в `src/plan-optimizer.ts`, пересобрал
|
||||
`test-build` (`npx tsc -p tsconfig.test.json && node scripts/fix-test-build.mjs`)
|
||||
и прогнал `node --test --test-name-pattern="issue 229" test/plan-optimizer.test.mjs`:
|
||||
новый тест «a node on the side of a room survives the sweep» красный
|
||||
(`1 !== 0`, ровно симптом из r1 — `partitionsMerged` вместо ожидаемых 0);
|
||||
восстановил файл — тест снова зелёный, дерево чистое;
|
||||
- прогнал мутант `node scripts/mutation-gate.mjs --id=chain-merge-sees-own-draft`
|
||||
через штатный харнесс (git worktree + пересборка бандла): чистый прогон зелёный,
|
||||
мутант красный, «поймано 1 из 1» — H2 подтверждён штатным гейтом.
|
||||
|
||||
**Не прогонялось и почему:** полный набор из 127 смоков, `golden:verify`,
|
||||
`pytest tests_backend` — дельта не трогает Python, рендер, слои и любую другую
|
||||
поверхность вне рисования перегородок и оптимизатора; полные наборы — предрелизный
|
||||
гейт (§8), не гейт ревью. Performance-профили не запускал — в AC не названы,
|
||||
дельта не касается путей, отмеченных как чувствительные к перфу.
|
||||
|
||||
## Находки
|
||||
|
||||
### Medium-1 (в скоупе задачи): регрессионное покрытие High-1 закрывает только один из двух исправленных вызовов
|
||||
|
||||
`src/houseplan-card.ts` (`_mergeSpacePartitions`, живой путь рисования) и
|
||||
`src/plan-optimizer.ts` (`optimizePlans`, «Оптимизировать планы») содержали
|
||||
**одинаковый** дефект лишнего деления `roomPolygons` на `NORM_W`, и оба
|
||||
исправлены в этом коммите идентичным образом. Но новый регрессионный тест
|
||||
(`test/plan-optimizer.test.mjs:497-516`) и сторожащий его мутант
|
||||
`partition-merge-rescales-rooms` (`scripts/mutation-gate.mjs:718-730`) проверяют
|
||||
**только** путь `plan-optimizer.ts`. Путь `houseplan-card.ts` — тот самый, для
|
||||
которого r1 привёл второе, самостоятельное воспроизведение («в браузере через
|
||||
реальные клики, `space.partitions.length === 1` вместо 2») — не получил ни
|
||||
юнита, ни смок-сценария, ни мутанта.
|
||||
|
||||
**Воспроизведение (исполнением):** вручную вернул то же лишнее деление
|
||||
**только** в `src/houseplan-card.ts` (оставив `plan-optimizer.ts` в исправленном
|
||||
виде), пересобрал бандл и `test-build`, прогнал весь набор:
|
||||
|
||||
- `npm test` — 1004/1004 pass, **ни один тест не покраснел**;
|
||||
- `node demo/smoke_wall_chain_merge.mjs` — все 9 проверок зелёные, дефект не
|
||||
затронут ни одной из них (в этом смоке ни разу не участвует комната — только
|
||||
голые перегородки).
|
||||
|
||||
Восстановил файл — поведение снова корректно (проверено чтением: `roomPoly`
|
||||
и координаты перегородок в `_finishWallChain`/`_mergeSpacePartitions` — один и
|
||||
тот же масштаб, деление объективно лишнее в обоих местах). Сам текущий код
|
||||
корректен; дефект — в отсутствии автоматической защиты именно этого вызова:
|
||||
следующая правка `_mergeSpacePartitions` может тихо вернуть тот же баг на живом
|
||||
пути рисования, и ни `npm test`, ни штатный набор смоков этого не заметят.
|
||||
|
||||
**Почему это Medium, а не High:** правка, которая лежит в этом коммите, сама по
|
||||
себе корректна и доказана чтением плюс ручным воспроизведением (я его провёл
|
||||
сам, а не поверил заявлению автора) — AC2 для этого вызова не «предположительно
|
||||
доказан», а доказан наблюдением. Дефект — в будущей защите, а не в текущем
|
||||
поведении, поэтому не блокирует, но должен быть закрыт в этой же задаче: она и
|
||||
её мутационный гейт — единственное место, где это естественно сделать, и Medium
|
||||
в скоупе чинится в текущем issue (§2.7, #202).
|
||||
|
||||
**Предложение:** добавить в `demo/smoke_wall_chain_merge.mjs` сценарий с
|
||||
комнатой, к середине стороны которой рисуется примыкающая перегородка через
|
||||
настоящие клики (по образцу нового юнита в `plan-optimizer.test.mjs`, но через
|
||||
живой `_finishWallChain`), и зарегистрировать для `src/houseplan-card.ts`
|
||||
второй мутант-близнец `partition-merge-rescales-rooms`, гвардом на этот смок.
|
||||
|
||||
### Low (снято без действия): точный SHA предыдущего вердикта не был назван явно
|
||||
|
||||
Комментарий-вердикт r1 называет коммиты (`e6be43b`, `50a00e4`), но не отдельно
|
||||
итоговый SHA HEAD на момент разбора — пришлось восстанавливать по истории
|
||||
(см. «Скоуп раунда»). Не влияет на проверяемость: коммиты названы однозначно,
|
||||
диапазон восстанавливается детерминированно. Снимаю без действия, отмечаю для
|
||||
будущих раундов этой же задачи.
|
||||
|
||||
## Находка вне скоупа — заводится отдельным issue
|
||||
|
||||
При проверке «тест умеет падать» для High-1 через штатный харнесс
|
||||
(`node scripts/mutation-gate.mjs --id=partition-merge-rescales-rooms`) прогон
|
||||
завершился на этапе чистого прогона:
|
||||
|
||||
```
|
||||
FAIL чистый прогон: node --test --test-name-pattern="issue 229" test/plan-optimizer.test.mjs
|
||||
красный без мутанта
|
||||
Error [ERR_MODULE_NOT_FOUND]: Cannot find module '.../test-build/plan-optimizer.js'
|
||||
```
|
||||
|
||||
Причина — не в мутанте и не в тесте: `makeWorktree()`/`buildBundle()`
|
||||
(`scripts/mutation-gate.mjs:964-990`) создают чистый git-worktree и пересобирают
|
||||
только `rollup` (бандл), но никогда не выполняют `npx tsc -p tsconfig.test.json
|
||||
&& node scripts/fix-test-build.mjs`. Любой мутант, чей `guard` — короткая форма
|
||||
`node --test --test-name-pattern="…" test/<файл>.test.mjs`, где `<файл>.test.mjs`
|
||||
импортирует `../test-build/*.js` (а не читает `src/*` текстом), в свежем
|
||||
worktree не находит `test-build/` вообще и падает с `ERR_MODULE_NOT_FOUND` — то
|
||||
есть «гейт словил поломку» на самом деле означает «гейт не может исполниться»,
|
||||
а `runCleanGuards()` (базовый прогон без мутанта) тоже красный по той же причине,
|
||||
поэтому полный прогон встаёт на первом же таком мутанте с кодом выхода 2.
|
||||
|
||||
Это не специфично для #229: тот же паттерн гварда уже используют мутанты issue
|
||||
#220 (`chain-merge-sweeps-...` — нет, конкретно `stale-space-position-...` и три
|
||||
мутанта у `test/space-order.test.mjs`, `scripts/mutation-gate.mjs:848-894`,
|
||||
внесены коммитом `a8aeecc`, который старше `#229` целиком) и старые мутанты
|
||||
самого #229 из `e6be43b` (`test/wall-merge.test.mjs`, шесть мутантов,
|
||||
`scripts/mutation-gate.mjs:757-820`) — я прогнал один из них
|
||||
(`partition-merge-ignores-thickness`) и получил ту же ошибку. Значит недостаток
|
||||
живёт в общей инфраструктуре `scripts/mutation-gate.mjs`, предшествует этой
|
||||
задаче и не создан её диффом (дифф только добавил ещё один мутант того же,
|
||||
уже дефектного, вида). Правка чужого скоупа с этой ветки запрещена (#202) —
|
||||
заведён отдельный issue:
|
||||
|
||||
**Новый issue: [#235](https://github.com/Matysh/houseplan-card/issues/235)**
|
||||
«`mutation-gate.mjs` не пересобирает `test-build` в worktree-мутантах — гварды,
|
||||
читающие `../test-build/*.js`, всегда красны на чистом прогоне» → метки
|
||||
`tech-debt`, `P2`, `S1-new`, ссылка на #229 и #220 как источники затронутых
|
||||
мутантов.
|
||||
|
||||
Само поведение фиксов High-1/High-2 это не отменяет: я подтвердил обе находки
|
||||
без штатного харнесса — прямым воспроизведением в основном рабочем дереве
|
||||
(ручной откат патча → пересборка `test-build` в реальном, не временном,
|
||||
дереве → тест красный → восстановление) и, для High-2, штатным прогоном
|
||||
мутанта, чей guard не зависит от `test-build` (браузерный смок).
|
||||
|
||||
## Итог по AC
|
||||
|
||||
| AC | Статус | Доказательство |
|
||||
|---|---|---|
|
||||
| AC1 | не тронут дельтой | унаследовано из r1 |
|
||||
| **AC2** | подтверждён повторно | юнит `plan-optimizer.test.mjs` (комната) + смок (перегородка↔перегородка) — оба зелёные и оба доказано умеют падать; путь `houseplan-card.ts` доказан чтением кода, автотеста нет (Medium-1) |
|
||||
| AC3 | не тронут дельтой | унаследовано из r1 |
|
||||
| AC4-AC6 | не тронуты дельтой | унаследовано из r1 |
|
||||
| AC7 | не тронут дельтой | унаследовано из r1 |
|
||||
| **AC8 (§8.6)** | подтверждён повторно | смок `chain_wall_merge`, мутант `chain-merge-sees-own-draft` через штатный харнесс — красный на откате, зелёный на текущем коде |
|
||||
| AC9 | не применим к этому коммиту | `User-Visible: no`, трейлеры проверены `process-gate.mjs` |
|
||||
|
||||
## Вердикт
|
||||
|
||||
Обе High-находки r1 закрыты по существу и перепроверены исполнением, а не по
|
||||
заявлению автора. Найден один новый Medium **в скоупе** задачи (регрессионное
|
||||
покрытие High-1 однобокое — защищён только путь оптимизатора, путь живого
|
||||
рисования нет) и один Medium **вне скоупа** (инфраструктурный дефект
|
||||
`mutation-gate.mjs`, предшествующий этой задаче, заведён отдельным issue).
|
||||
High нет — вердикт жёлтый.
|
||||
|
||||
Вердикт: жёлтый · заход r2 · блокирующих циклов 2/4 · High: 0 · Medium: 1 → в задаче
|
||||
Reference in New Issue
Block a user