diff --git a/docs/reviews/SPEC-REVIEW-233-r1.md b/docs/reviews/SPEC-REVIEW-233-r1.md new file mode 100644 index 00000000..c8e05c60 --- /dev/null +++ b/docs/reviews/SPEC-REVIEW-233-r1.md @@ -0,0 +1,247 @@ +# SPEC-REVIEW-233-r1 + +- Issue: [#233](https://github.com/Matysh/houseplan-card/issues/233) — «Ресайз комнаты: показывать внутренние размеры (от стены до стены), а не по осевым» +- Документ ТЗ: `docs/specs/233-resize-inner-dimensions.md`, ветка `issue/233-resize-inner-dimensions`, SHA `77fa698` +- Этап: `S4-spec-review` · заход r1 · блокирующих циклов 0/4 +- Ревьюер: Claude (роль «ревьюер ТЗ», сессия отдельна от автора — Codex) + +## Скоуп + +Задача меняет только текст/число подписей ресайза в редакторе разметки: +`_rszEdgeLabels` (три длины) и `_rszScaleLabels` (габарит угловой рамки) в +`src/houseplan-card.ts`, плюс новую чистую функцию `innerEdgeSpan` в +`src/wall-thickness.ts`. Площадь и конфигурация не меняются. На момент ревью в +ветке присутствуют только два документных коммита +(`docs(spec): measure resize labels between wall faces`, +`docs(spec): register the #233 spec in the index`) — продуктового кода нет, +`git diff origin/dev...HEAD --stat` показывает только `docs/specs/**`. Ревью +корректно ведётся на этапе ТЗ, без кода. + +## Как проверялось + +1. Прочитан `docs/SCOPE.md` — задача попадает в J6 («keep the plan true… + drag/resize»), редактор разметки — admin-only поверхность, View/киоск не + затронуты; ТЗ верно ограничивает поверхность (§8). +2. Прочитан `docs/WALL-THICKNESS.md` целиком, в частности §2–4 (рост стен, + inset/флэт-кэп на стыке нулевой и ненулевой толщины, инвариант площади). +3. Прочитан текущий код: `src/houseplan-card.ts` (`_rszEdgeLabels`, + `_rszScaleLabels`, `_fmtLen`, `_renderResizeLayer`, `_rszEdgeDown`) и + `src/wall-thickness.ts` (`inwardNormal`, `insetContour`, `thicknessCmAt`, + `lookupWall`, `exactCoveringWall`, `atomicPolyForRoom`, `cmsForPoly`, + `roomWallProfile`, `innerContourForRoom`, `wallCmToUnits`) — сверка диагноза + §3 ТЗ построчно с исходником. Диагноз подтверждён точно: площадь уже + внутренняя, длины — осевые по вершинам полигона комнаты. +4. Проверена математика контракта §6 ручным разбором трёх сценариев: прямой + угол с равными толщинами (AC1), прямой угол с разными толщинами соседей + (AC2), угол с одной нулевой стороной (AC4) — с конкретными координатами, не + на словах. +5. Сверено, действительно ли `thicknessCmAt` на смешанном по толщине ребре + возвращает «толщину участка по середине ребра», как заявляет §7 ТЗ — + найден существующий юнит-тест, который прямо противоречит этому + утверждению (см. находку H2). +6. Прочитан `docs/USER-GUIDE.ru.md` (Resize, §406–424) — сверена текущая + формулировка («живые длины и чистую площадь по внутреннему контуру», + строка 417), подтверждён факт, что она описывает именно ту рассинхронизацию, + которую фиксит issue, и что ТЗ верно планирует её переписать. +7. Проверены обязательные разделы ТЗ по PROCESS.md §7.1 — все присутствуют + (сценарий, до/после, диагноз, продуктовые решения, скоуп/не-скоуп, + контракт, источник данных, поверхности, файлы/i18n, AC1…AC9, mutation + guards, план тестов, перф/безопасность/touch, откат, риски, + release-артефакты, блок предположений). +8. Гейты этого раунда: код не менялся, поэтому `tsc`/`test`/`build` прогонять + бессмысленно — рассматривался только текст ТЗ. Не прогонял ничего + намеренно (см. «Чего не проверял»). + +## Находки + +### H1 — Контракт §6 не покрывает открытую сторону, к которой ведёт продуктовое решение §4.3 (High) + +**Файл:** `docs/specs/233-resize-inner-dimensions.md`, §4 п.3, §6 (строки 60–62, 104–112) + +**Что не так.** Issue прямо требует: «Проёмы и незамкнутые стороны… длина +считается по осевой, как сейчас… Явно назвать в ТЗ, а не оставлять на +догадку» — то есть ребро, которое **само** является проёмом/открытой стороной +(`oSelf = 0`), должно всегда показывать осевую длину, независимо от соседей. +Контракт §6 формулирует только три отступления: + +1. `oSelf` и **оба** соседа равны нулю → осевая длина; +2. пересечение даёт ≤0 → 0; +3. **соседняя** сторона с нулевой толщиной → сокращения на этом конце нет. + +Отступление (3) защищает **стену** (`oSelf > 0`) от ошибочного сокращения, +когда её соседняя сторона открыта — это ровно случай AC4. Но оно не покрывает +обратный, явно названный в issue случай: сама измеряемая сторона открыта +(`oSelf = 0`), а соседняя — настоящая стена (`oPrev`/`oNext > 0`). Для него +работает только общий алгоритм пересечения линий (шаги 1–3 §6), и я проверил +его на конкретных координатах: + +``` +Комната-прямоугольник, V0=(0,0) V1=(300,0) V2=(300,400) V3=(0,400). +Нижняя сторона V0→V1 — проём (oSelf = 0). +Правая сторона V1→V2 — стена 15 см (половина = 7.5). + +Внутренняя линия нижней стороны при oSelf=0 не смещается: y = 0. +Внутренняя линия правой стороны смещена на 7.5 внутрь: x = 292.5. +Пересечение: (292.5, 0) — на 7.5 см левее исходной вершины V1=(300,0). +``` + +То есть буквальная реализация контракта §6 **сократит** осевую длину проёма +на половину толщины соседней стены (тут — 7.5 см из 300), хотя issue требует +показывать полную осевую длину. + +Это не теоретическая тонкость: то же самое место в `insetContour` +(`src/wall-thickness.ts:900-907`, документировано в `docs/WALL-THICKNESS.md` +§3 «A variable-offset join where exactly one adjacent edge has zero depth is a +local flat cap, not a mitre») — то есть код, который считает **площадь** +(AC8, которая не должна меняться) — обрабатывает этот же стык **иначе**: он +не пересекает линии, а оставляет вершину нулевой стороны нетронутой (flat +cap). Я прогнал ту же геометрию через логику `insetContour`: для стыка +oA=0/oB=7.5 функция кладёт `pa = v` (нетронутая вершина (300,0) — конец +инсета нижней стороны) и `pb = (292.5, 0)` (начало инсета правой стороны) как +**два отдельных** узла контура, то есть нижняя (открытая) сторона в площади +**не** укорачивается на этом конце. + +**Последствие.** Если реализовать §6 буквально, для любой комнаты с открытой +стороной, у которой хоть один конец примыкает к настоящей стене (обычный +случай: открытая планировка, кухня-гостиная, любой проём во всю стену), длина +проёма будет показана короче осевой — притом что площадь (через +`innerContourForRoom`) для того же угла не укоротится. Это воспроизводит +**ровно тот класс дефекта**, который весь issue #233 создан устранять +(«длина и площадь по разным конвенциям»), только на границе проёма, а не на +равномерной толщине. Ни один AC (AC1…AC9) и ни один mutation guard (§11) этот +случай не проверяют — регрессия останется незамеченной кодом-ревью, раз нечему +падать. + +**Как закрыть.** Добавить в §6 четвёртое, явное отступление: «если `oSelf == +0`, сторона сама является проёмом/открытой стороной — возвращается осевая +длина `|b−a|` без обращения к соседям» (симметрично уже описанному третьему +пункту, но для своей, а не соседской нулевой толщины). Нужно также явно +назвать, что происходит с **соседней** стеной в этом случае — судя по разбору +`insetContour`, она тоже не должна сокращаться на этом конце, иначе +AC4-подобная защита развалится в паре с новым правилом. + +### H2 — §7 приписывает `thicknessCmAt` поведение, которого у неё нет: заявлено фактом, не проверено (High) + +**Файл:** `docs/specs/233-resize-inner-dimensions.md`, §7 (строки 117–126) + +**Что не так.** §7 утверждает как установленный факт: «Если одно ребро +комнаты разрезано на атомарные участки с разной толщиной, `thicknessCmAt` +вернёт толщину участка по середине ребра. Это принятое упрощение… Записано +здесь, чтобы ревью не считало это недосмотром.» Формулировка прямо просит +ревью поверить автору на слово — и именно поэтому её нужно было проверить. + +Проверка показала обратное. В `test/wall-thickness.test.mjs:169-173` есть +именно такой тест: + +```js +test('thicknessCmAt exact-parent fallback does not leak from partial or + unrelated spans', () => { + const partial = setWallThickness([], [0, 0], [4, 0], 20, pitch); + assert.equal(thicknessCmAt(partial, [0, 0], [10, 0], pitch), 0); + ... +``` + +Толщина задана только на части ребра (`[0,0]-[4,0]`, 20 см); запрос +**полного** ребра `[0,0]-[10,0]` возвращает **0**, а не 20 (толщину куска у +середины или у начала). `lookupWall` ищет точное совпадение ключа полного +ребра или толерантный фолбэк по совпадению середины/угла — оба рассчитаны на +случай «одна запись = один цельный отрезок» (комментарий AUD-159B6-01 в +`wall-thickness.ts:224-233`), а не на «одно ребро комнаты собрано из +нескольких разно-толщинных атомарных кусков». `exactCoveringWall` — обратный +случай (короткий атомарный запрос против длинной родительской записи), он +тоже не покрывает «длинный запрос против нескольких коротких записей». + +Более того, в кодовой базе уже есть правильный, атомарно-осведомлённый +конвейер для этой задачи — `atomicPolyForRoom` → `cmsForPoly` → +`roomWallProfile`, тот самый, которым `innerContourForRoom` считает площадь +(и который AC8 обязана не менять). §6/§7 предлагают вместо него отдельный, +более грубый путь: `thicknessCmAt` прямо на вершинах полигона комнаты — тем +самым для длины и для площади толщина одного и того же ребра с атомарной +разнотолщинностью резолвится **двумя разными механизмами**, и один из них (по +цитированному тесту) для такого ребра тихо возвращает 0 вместо реальной +толщины. + +**Последствие.** Для любой комнаты, где одна сторона имеет неоднородную по +длине толщину (штатный, документированный случай — `docs/WALL-THICKNESS.md` +§1: «a different thickness or a virtual gap remains a real break»), подпись +длины по контракту §7 получит `oSelf`/`oPrev`/`oNext = 0` там, где физически +стена есть, — то есть либо не сократится вовсе, либо (в связке с находкой H1) +попадёт в ветку «осевая длина», разойдясь с площадью, которая по +`roomWallProfile` эту толщину видит и учитывает. Ни один AC это не ловит. + +**Как закрыть.** Либо явно доказать (ссылкой на код/тест, а не декларацией), +что вызывающий код в `_rszEdgeLabels`/`_rszScaleLabels` не может столкнуться с +таким ребром — но это неверно: сторона со сплит-толщиной — обычное состояние +после точечной правки инструментом «Толщина» на части стены; либо переключить +§7 на `roomWallProfile`/`cmsForPoly` (тот же путь, что уже даёт согласованную +с площадью толщину), а не на голый `thicknessCmAt` по вершинам комнаты. + +## Что проверено и корректно + +- **Диагноз (§3)** точен и подтверждён построчно: площадь уже строится через + `innerContourForRoom` + `floorMinusBodies`, длины — через `_fmtLen` по + вершинам полигона комнаты (осевые). Не догадка — цитаты совпадают с кодом. +- **Ловушка с `insetContour`, не сохраняющим число вершин** (§3), — + подтверждена чтением `insetContour`: митра даёт 1 точку, бевел/коллинеарный + стык/нулевая сторона — 2, что действительно делает наивный «взять ребро i + внутреннего контура» неверным. Решение — пересечение линий трёх рёбер, а не + индексация — обосновано корректно для основного случая (не-нулевые соседи). +- **AC1/AC2/AC6** математически согласованы с алгоритмом §6 для прямых углов: + сокращение равно половине толщины СОСЕДНЕЙ стены на каждом конце, + независимо от толщины самого ребра — я перепроверил это на числах (300 см, + 15/15 → 285; 15/30 → 300−7.5−15=277.5, что соответствует «сокращает на + 7.5+15»). AC6 (обрезка отрицательного результата до 0) — прямое следствие + шага 3 алгоритма. +- **AC5 (диагональ)** — корректно указывает, что наивная формула `|b−a| − + o − o` неверна для не прямых углов и что верный ответ — пересечение линий; + сама механика для диагоналей не отличается от прямого угла (не требует + доп. кода), что верно, поскольку `inwardNormal`/`lineIntersect` не + привязаны к прямым углам. +- **Границы задачи (§5)** — area, раскладка толщин по интервалам, подписи вне + ресайза корректно выведены из скоупа; ни одна из них не требует правки для + выполнения продуктового решения issue. +- **i18n/миграция/compatibility (§9)** — обоснованно «не затронуты»: новых + строк и полей конфигурации нет, что подтверждается описанным изменением + (число в существующей подписи, не текст). +- **Touch (§8)** — ресайз на тач уже работает и не меняется этой задачей; + ссылка на `docs/TOUCH-SUPPORT.md` корректна, задача не вводит новый UX. +- **Откат (§14)** и **release-артефакты (§16)** — соразмерны задаче (чистая + функция, одна ревизия, golden не участвует, поскольку подписи ресайза не + входят в матрицу golden). +- Все обязательные разделы ТЗ по PROCESS.md §7.1 присутствуют, включая блок + принятых технических предположений (§17), корректно отделённый от + продуктовых решений владельца (§4). +- Автор не задавал владельцу вопросов сверх пяти уже отвеченных в issue — + корректно для этой задачи: оставшаяся неоднозначность (H1/H2) техническая, + относится к тому, «что реализация должна сделать, чтобы соответствовать уже + принятому продуктовому решению», а не к новому продуктовому вопросу. + +## Чего не проверял + +- **Не прогонял `tsc`/`test`/`build`** — на этом этапе в ветке нет + продуктового кода (только `docs/specs/**`), гонять гейты не на чём. +- **Не оценивал независимость `_rszScaleLabels` от H1/H2 тестом**: этот метод + берёт габарит из `innerContourForRoom` (уже атомарно-осведомлённого через + `roomWallProfile`), поэтому предварительно похоже, что находки H1/H2 не + задевают угловую рамку — но это подтверждено только чтением кода, не + исполнением, и стоит перепроверить в код-ревью, когда появится реализация. +- **Не пытался предугадать финальную реализацию** `innerEdgeSpan` — находки + адресованы к контракту (§6/§7) ТЗ, а не к коду, которого ещё нет. +- **Golden/смоки/backend** не запускал — задача не меняет визуальную матрицу + и не трогает `custom_components/**/*.py`, а код ещё не написан. + +## Вывод + +Диагноз и большая часть контракта — качественная, проверенная работа: автор +сам нашёл и явно задокументировал главную ловушку (`insetContour` не сохраняет +число вершин) и синхронизировал возражение с существующим mutation-гейтом. +Но контракт §6/§7 не полностью реализует уже принятое продуктовое решение +issue (открытые/незамкнутые стороны — п.3 issue, §4.3 ТЗ) и содержит +непроверенное, фактически неверное утверждение о поведении `thicknessCmAt` +на разнотолщинном ребре. Оба дефекта — в скоупе задачи (они прямо про +контракт длины, который задача и переписывает), оба обнаруживаются без +исполнения кода — чтением существующих тестов и геометрии `insetContour`, и +оба, если не исправить в ТЗ, с высокой вероятностью воспроизведут в коде +именно ту рассинхронизацию длины и площади, которую issue #233 просит +устранить. Возврат автору для правки §6 (явное правило для `oSelf == 0`) и §7 +(источник толщины на смешанном по толщине ребре).