From 2af7b0b5a2fecdd550c8b937a14a8e7703ffc287 Mon Sep 17 00:00:00 2001 From: "claude[bot]" <209825114+claude[bot]@users.noreply.github.com> Date: Sat, 22 Aug 2026 17:56:41 +0000 Subject: [PATCH] docs: review document for #242 Issue: #242 User-Visible: no --- docs/reviews/CODE-REVIEW-242-r1.md | 266 +++++++++++++++++++++++++++++ 1 file changed, 266 insertions(+) create mode 100644 docs/reviews/CODE-REVIEW-242-r1.md diff --git a/docs/reviews/CODE-REVIEW-242-r1.md b/docs/reviews/CODE-REVIEW-242-r1.md new file mode 100644 index 00000000..147a3130 --- /dev/null +++ b/docs/reviews/CODE-REVIEW-242-r1.md @@ -0,0 +1,266 @@ +# CODE-REVIEW-242-r1 + +- Issue: [#242](https://github.com/Matysh/houseplan-card/issues/242) — «Проём в толстой стене рисуется у произвольной грани, а не по центру толщины» +- Этап: code (PROCESS.md §2.7) +- Заход: r1 (первый код-цикл; SPEC-REVIEW-242-r1 — зелёный) +- Диапазон: `origin/dev..HEAD` = `0033138..5e13e34` (5 коммитов), реализация — `6d6db12` (`fix(openings): center symbols across wall depth`, `User-Visible: yes`) +- Вердикт: **жёлтый** + +## Скоуп + +ТЗ `docs/specs/242-opening-symbol-center.md` требует: символ проёма (дверь/окно/ворота) +по умолчанию центрируется по толщине стены на всех render surfaces (Flat/View, +preview, hosted Static, скрытый Iso); сохранённый `flip_v: true` у двери/окна +остаётся ручным edge alignment; у ворот `flip_v` **не двигает** створки, а только +меняет знак существующего 10° поворота (AC4, §4.3, §7 таблица). Направление должно +быть детерминированным и не зависеть от порядка комнат/направления независимой +стены (AC2). + +## Как проверялось + +Прочитаны docs/SCOPE.md, PROCESS.md, docs/specs/242-opening-symbol-center.md, +docs/WALL-THICKNESS.md, docs/ARCHITECTURE.md, docs/ISOMETRIC.md, +docs/USER-GUIDE.ru.md, тело issue и все комментарии (аналитика, Q1–Q4, решения +владельца, handoff разработчика). + +Дифф прочитан построчно: `src/opening-symbol-placement.ts` (новый), +`src/wall-thickness.ts`, `src/render/opening-symbol.ts`, `src/iso-openings.ts`, +плюс unit/smoke/mutation-gate правки. `src/houseplan-card.ts` и +`src/partition-openings.ts` диффом не тронуты — они читались как неизменный +контекст, через который резолвится `face`. + +Гейты, реально прогнанные в этой сессии: + +| Гейт | Результат | +|---|---| +| `npx tsc --noEmit` | green | +| `npm test` | green, 1095/1095 | +| `npm run build` + сверка `dist` / `demo/srv/assets` / `custom_components/.../frontend` | green, все три sha256 = `3befe09b…`, `git status` чист | +| `node scripts/check-docs.mjs` | green (7 файлов, 10 внешних ссылок) | +| `node scripts/mutation-gate.mjs --check` | green, включая 3 новых мутанта `opening-symbol-*` | +| `node scripts/smoke-select.mjs --base origin/dev --head HEAD` | «Зарегистрированная связь» → `smoke_isometric_contract.mjs`, `smoke_wall_thickness.mjs` (оба помечены автором как прогнанные и зелёные; полный вывод инструмента ниже) | +| `npm run golden:verify` на HEAD | red (см. раздел «Находки», M1) — прогнан дважды подряд, результат детерминирован | +| `npm run golden:verify` на `origin/dev` (5bf1868, отдельный `git worktree`) | red по своему меньшему набору сцен — снят как база для сравнения, не для приёмки | + +Дополнительно я independently проверил геометрический контракт **прямым запуском +в headless Chromium** через `demo/serve.mjs` (тот же харнесс, что используют +существующие `demo/smoke_*`), а не только чтением кода — конкретно AC4 для +ворот, потому что до-релизная логика face/flip у ворот инвертирует `flip_v` +дважды (`faceFlipV = !o.flip_v` в `src/houseplan-card.ts` при построении face, +затем сырой `o.flip_v` ещё раз в `sy` внутри `renderOpeningVisibleGeometry`), и +я не был уверен в результате по одному чтению formулы. Три сценария (свежее +состояние карточки на каждый, без переиспользования state): + +1. Ворота на **внешней** (однокомнатной) толстой стене (`_wallThickClick`, + cm=25) — `flip_v: false` → рычаги `rotate(-10deg)/rotate(10deg)`, + `flip_v: true` → `rotate(10deg)/rotate(-10deg)`. Меняется, как требует AC4. +2. Ворота на **общей** стене двух комнат (тот же сценарий, что и исходный баг + — стена между `r1`/`r2`) — `flip_v: false` и `flip_v: true` дают **одинаковый** + `rotate(10deg)/rotate(-10deg)`; направление не меняется. Проверено дважды, + плюс с перестановкой `sp().rooms` — результат идентичен во всех четырёх + комбинациях. +3. Ворота на **независимой стене** (`host: { kind: 'partition', … }`), прямые и + развёрнутые endpoints — `flip_v: false` и `flip_v: true` также дают + одинаковый `rotate(10deg)/rotate(-10deg)` в обеих ориентациях перегородки. + +Скрипты воспроизведения не коммитились (репозиторий трогать нельзя), но +последовательность — это буквально `demo/smoke_wall_thickness.mjs` до момента +создания стены, плюс `sp().openings = [{ type: 'gate', … }]` и чтение +`.op-leaf` style после `c._cfgEpoch++/requestUpdate/updateComplete`; сравнение +`flip_v` до/после и общей/независимой стены до/после — воспроизводится любым +инструментом с доступом к `window.__card`. + +## Находки + +### H1 (High, в скоупе) — AC4 не выполнен: `flip_v` не меняет направление 10° поворота ворот на общей стене и на независимой стене + +**Что не так.** Спецификация (§4.3, §7 таблица: `gate | true | (0,0) | +противоположный знак 10° поворота`) и AC4 («Gate при обоих значениях `flip_v` +центрирован, но знак 10° поворота меняется») требуют, чтобы флаг `flip_v` у +ворот всегда переключал сторону лёгкого поворота створок, при неизменном +центрированном origin. Перевод «центр не двигается» (translation) реализация +делает верно — `openingSymbolOffset` безусловно возвращает `{0,0}` для +`type==='gate'`. Но требование «знак меняется» не выполняется для двух из трёх +типов host, включая тот, что явно назван в самом issue как источник бага: + +- **общая стена двух комнат** — именно тот кейс, ради которого затевалась вся + задача («порядок комнат», «то с одной стороны, то с другой»). Створки ворот + на общей стене поворачиваются **одинаково** при `flip_v: false` и + `flip_v: true` (см. воспроизведение выше, сценарий 2); +- **независимая (partition) стена** — тот самый путь, про который в issue + сказано: «сторона фиксирована знаком... то есть по умолчанию всегда одна и + та же грань» (сценарий 3). После правки трансляция действительно исчезла + (это была цель), но направление поворота по‑прежнему не реагирует на + `flip_v` вообще — то есть первоначальный дефект (фиксированный знак) для + этого пути **не устранён**, просто перестал быть заметен из-за убранного + смещения. + +Направление меняется корректно только для ворот на **однокомнатной внешней** +стене (сценарий 1) — самом редком практическом месте для ворот. + +**Причина по коду.** `src/houseplan-card.ts` инвертирует `flip_v` перед +резолвом грани только для ворот: `faceFlipV = o.type === 'gate' ? !o.flip_v : +o.flip_v` (строки 11988, 18551, 12066 и в Iso-ветке 5289), затем передаёт +**сырой** `o.flip_v` в `visibleSpec.flipV`/`sy`. `src/render/opening-symbol.ts` +считает `gateAngle = spec.face.side * sy * 10 * amount`. Для стороны, +получаемой через `partitionOpeningFace` (`side = flipV ? 1 : -1`, не тронут +этим диффом) и через «общую стену» ветку `wall-thickness.ts` +(`naturalSide = association.negative && association.positive ? -1 : +available[0].side`, тоже реагирует на `flip_v` только знаком) двойная +инверсия (`faceFlipV` затем `sy`) арифметически гасит сама себя: `side * sy` +даёт одно и то же значение при `flip_v=false` и `flip_v=true`. Единственный +случай, где это не совпадает — однокомнатная стена, потому что там +`naturalSide` берётся из `available[0].side`, единственного реального +кандидата, а не из пары `negative`/`positive`, и по независимым от меня +причинам (сама схема `_openingFace`/`resolveOpeningWallAssociation`) итоговый +знак там ведёт себя иначе; я подтвердил это эмпирически, а не через ручной +вывод формулы (see «Как проверялось»), и не настаиваю на объяснении «почему +именно этот случай работает» — важен наблюдаемый факт. + +**Кто ещё вводится в заблуждение.** Оба changelog, `docs/WALL-THICKNESS.md`, +`docs/ARCHITECTURE.md`, `docs/ISOMETRIC.md` и `docs/USER-GUIDE.ru.md` **прямо +утверждают** обратное: «ворота остаются по центру, а флаг меняет только +направление их 10° открытия» (и англ. эквивалент «a gate flip changes only +turn direction»). Это неверно для двух из трёх host-путей. + +**Почему не поймано тестами.** `test/opening-symbol.test.mjs` для ворот +проверяет `rotate(10deg)`/`rotate(-10deg)` только у **одного** (не-flipped) +рендера — второй (`flippedGate`) собирается, но его `rotate(...)` значения +нигде не сравниваются с первым. `test/iso-openings.test.mjs` сравнивает +`gate`/`gateFlipped`, но эти два фикстура одновременно меняют и `flipV`, и +`face.side` вручную — тест не воспроизводит, что в реальном рендере `side` +сам является функцией от `flip_v` через `faceFlipV`, поэтому не ловит гашение +знаков. `demo/smoke_isometric_contract.mjs` проверяет только +`Math.abs(leaf.turnDeg) === 10`, не сравнивая знак с неперевёрнутым вариантом. +Ни один golden-сценарий с воротами `flip_v: false/true` рядом не добавлен (см. +M1) — а именно такая сцена сделала бы дефект видимым на глаз. + +**Серьёзность.** High: явный, воспроизводимый разрыв между заявленным в ТЗ/ +docs контрактом и фактическим поведением для двух из трёх типов host-стен, +включая тот, что был первопричиной issue. Блокирует. + +**Как исправить (для автора, не мной):** либо не инвертировать `flip_v` при +резолве `face` для ворот в `houseplan-card.ts` (раз "сторона" в `face.side` +и так уже несёт нужный знак), либо использовать в `gateAngle` **не** +`spec.face.side`, а сырой `!!o.flip_v`/детерминированную локальную сторону +напрямую, независимо от того, как резолвер комбинирует её с `flip_v` +дважды. Нужна регрессионная проверка знака на пути «общая стена» и +«partition», а не только «однокомнатная стена». + +### M1 (Medium, в скоупе) — заявленный golden-охват в handoff занижен на порядок; обязательные по §12.3 новые сцены не добавлены + +Handoff-комментарий разработчика утверждает: «Targeted golden: +`opening-placement-door-thick-wall-dark` и `openings-thick-wall-dark` ожидаемо +`different`». Фактический `npm run golden:verify` на точном `5e13e34` +(прогнан дважды, детерминированно) даёт **`different` по ~48 сценам**, не по +двум — сравнение с тем же прогоном на чистом `origin/dev` (отдельный +`git worktree`, тот же локальный Chromium) показывает, что подавляющее +большинство новых расхождений (`isometric-geometry-*`, `day-cycle-*`, +`lighting-*`, `hover-*`, `device-*`, `optimize-preflight-dialog-*`, +`backup-*`, `room-label-parity-*` и др.) на dev были `passed`, а на HEAD стали +`different`. Причина ожидаемая и не является отдельным багом: эти сцены +используют общие фикстуры (`golden-geometry`, `golden-lighting`), в которых +уже стоит дверь/окно/ворота в толстой стене — символ у них теперь центрирован, +и это меняет пиксели кадра, даже когда сценарий проверяет не связанную с +проёмами вещь (солнце, hover, диалог). Это не находка «что-то сломалось +непричастное», а находка «отчёт разработчика не соответствует +действительности на порядок величины» — то же самое, что ревью #231-r1 уже +находило (M1 там: «список golden-сцен неполон») и что #231-r2 потребовало +зафиксировать явным списком в `docs/TESTING.md`, а не оставлять в тексте +handoff-комментария. + +Отдельно: §12.3 ТЗ требует **добавить** новые semantic-сцены — «centered +door/window/gate на толстой room wall, Light», «те же типы on diagonal +partition, Dark», «door/window с `flip_v: true` и gate `false/true` рядом», +«hidden Iso parity for center and gate direction» — и обновить golden semantic +guard, чтобы он **до** PNG-сравнения проверял wall centerline, visible-group +center, jamb depth, `flip_v` и gate angle. Ни одна из этих сцен не добавлена +в `demo/golden/matrix.mjs` (диф по этому файлу — только правка комментария +для существующей `opening-placement-door-thick-wall-dark`); никакого нового +semantic guard тоже нет. Именно сцена «gate `false/true` рядом» — то, что +сделало бы H1 видимым на глаз ещё до code review. + +**Серьёзность.** Medium, в скоупе задачи: тестовый план §12.3 не выполнен +полностью, а фактический охват регрессии не задокументирован для +предрелизного `golden:accept -- --reviewed`, который будет принимать эти ~48 +сцен без знания их полного списка и происхождения. Чинится добавлением +описанных в §12.3 сцен/guard и точного списка затронутых существующих сцен +(аналогично `docs/TESTING.md` в #231-r2) — без необходимости отдельного issue. + +## Что проверено и корректно + +- **AC1/AC2 для room-wall пути** (без `flip_v`, центр стены, независимость от + порядка комнат). `openingInnerFaceOffsetFromIndex` разделяет физическую + половину (`piece.half`/`piece.cm`, не зависит от `flip_v`) и направление + (`side` из `resolveOpeningWallAssociation`, вычисляется из локальной нормали + открывания `nx,ny = f(opening.angle)` и геометрического `edge.inward`, + **не** зависит от порядка `rooms`/`candidateOrder`). `test/wall-thickness.test.mjs` + добавил `reversed`/`reversedFlip` deepEqual-проверку — я прочитал и + проверил, что тест умеет падать (откатил численно вручную: без замены + сортировки `.order` на геометрический `side` возвращается старое значение). +- **AC2 для partition-пути.** `resolvePartitionOpening` нормализует `angle` + через `atan2` + приведение к `[-90,90)`, что инвариантно к развороту + `a`/`b` (проверено чтением и совпадает с уже существующим тестом + `test/partition-openings.test.mjs:35`, не тронутым этим диффом). + `openingSymbolOffset` использует `Math.hypot(face.ox, face.oy)` — модуль, а + не знак — поэтому разворот `partitionOpeningFace`'s `ox/oy` (который + реально зависит от направления оси, см. `axis.ux/uy`) не протекает в + видимое смещение. Это подтверждено и unit-тестом + (`test/opening-symbol-placement.test.mjs`: `positiveFace`/`negativeFace` дают + одинаковый результат), и моим собственным browser-тестом сценария 3 выше + (симметрия сохраняется, хоть и с той же H1-проблемой по направлению). +- **AC3 (door/window `flip_v: true`, окно целиком).** `test/opening-symbol.test.mjs` + проверяет, что `op-glass`/leaves/arc едут одним `translate`; я прочитал + `renderOpeningVisibleGeometry` — стекло действительно внутри той же + ``, что и створки/дуги. +- **Jambs full-depth.** `jambHalf` в `openingVisibleMetrics` считается из + `spec.face.cm` независимо от `swingTx/swingTy` — косяки не входят в + переносимую группу, как требует §7. Проверено чтением, не исполнением + отдельного визуального теста. +- **AC7 (без миграции/схемы).** `src/opening-symbol-placement.ts` — чистые + функции, `OpeningCfg.flip_v` не тронут, новых полей/типов конфигурации нет. + `git diff` подтверждает: единственный новый файл — чистый TS-модуль без + side-effects. +- **User-Visible / трейлеры.** `6d6db12` несёт `Issue: #242` и + `User-Visible: yes`; оба changelog обновлены в том же коммите (см. + `git show 6d6db12 --stat`). +- **Три копии бандла.** sha256 `dist/houseplan-card.js`, + `demo/srv/assets/houseplan-card.js`, + `custom_components/houseplan/frontend/houseplan-card.js` совпадают между + собой и с заявленным автором хэшем; `npm run build` воспроизводит их + без диффа в рабочем дереве. +- **Mutation-gate.** Три новых мутанта (`opening-symbol-default-uses-room-face`, + `opening-symbol-partition-follows-endpoints`, `opening-gate-flip-translates-leaves`) + проходят `--check`; я прочитал патчи — они действительно откатывают именно + центрирование/order-independence (не гейт-обход), но, как и unit/golden + выше, не покрывают H1 (мутант про ворота проверяет только «не появилась ли + трансляция», а не «меняется ли знак поворота»). + +## Чего не проверял + +- **Полный Linux golden-артефакт и его reviewed-приёмка** — вне цикла код-ревью + по правилу §13 (`golden:accept` только по полному Linux release artifact). + Локальный `golden:verify`, который я прогнал, обслуживает только M1 (охват + дельты), не приёмку эталонов. +- **`python -m pytest tests_backend`** — не прогонял: `custom_components/**/*.py` + диффом не тронут. +- **Performance-профили** — не прогонял: задача не называет их в AC, изменение + чисто геометрическое без новых аллокаций/обходов по стенам (проверено + чтением: `openingSymbolOffset` — O(1) арифметика на уже разрешённом `face`). +- **Ручной пиксельный просмотр каждой из ~48 изменившихся golden-сцен** — я + установил только факт и причину расхождения (общая фикстура с дверью), не + разбирал каждую сцену построчно; это и есть предмет M1. +- **AC1 буквально на 1/15/100 см.** Существующие unit-тесты (`wall-thickness.test.mjs`, + `opening-symbol-placement.test.mjs`) используют другие конкретные значения + cm (15/20/25/30); отдельного sweep по 1/15/100 см для нового contract'а нет. + Не поднимаю как отдельную находку — арифметика линейна по `cm` + (`piece.half`, масштаб) и не имеет условных ветвей по величине толщины, риск + низкий, но формальное доказательство AC1 в заявленном виде отсутствует. +- **Полная browser-smoke матрица (167 сценариев)** — не прогонял всю. Выбор + ограничен `smoke-select`-выводом (2 зарегистрированные связи, обе прогнаны + автором и мной перепроверены через отдельные ad hoc browser-скрипты на + реальном харнессе, не только через существующие smoke-файлы) плюс + дополнительные `smoke_partition_openings.mjs`/`smoke_opening_preview.mjs`, + которые автор тоже прогнал зелёными. Полный прогон не требуется: диф не + задевает несвязанные подсистемы (unrelated golden-diff в M1 объяснён общей + фикстурой, а не изменением поведения самих этих подсистем).