From 28581754a7ce9eeb4b0da605c3caed6b3dbde232 Mon Sep 17 00:00:00 2001 From: "claude[bot]" <41898282+claude[bot]@users.noreply.github.com> Date: Fri, 14 Aug 2026 08:54:30 +0000 Subject: [PATCH] docs: spec review for #141 Issue: #141 User-Visible: no --- docs/reviews/SPEC-REVIEW-141-r1.md | 269 +++++++++++++++++++++++++++++ 1 file changed, 269 insertions(+) create mode 100644 docs/reviews/SPEC-REVIEW-141-r1.md diff --git a/docs/reviews/SPEC-REVIEW-141-r1.md b/docs/reviews/SPEC-REVIEW-141-r1.md new file mode 100644 index 00000000..ee3c8a17 --- /dev/null +++ b/docs/reviews/SPEC-REVIEW-141-r1.md @@ -0,0 +1,269 @@ +# SPEC-REVIEW-141-r1 + +- **Issue:** https://github.com/Matysh/houseplan-card/issues/141 +- **ТЗ под ревью:** `docs/specs/141-wall-junctions.md` (коммит `59c66dd`, + ветка `issue/141-wall-joints`) +- **Роль:** ревьюер ТЗ (не автор), этап `S4-spec-review` +- **Трек:** обычный (не `small`/`trivial`) — сложность/риск 7/10, две+ + поверхности (перегородки/drafts и рисование комнатных стен), влияние на + golden, свет и производительность; лёгкий/короткий трек владелец и автор + корректно не применили. +- **Цикл:** r1/4 + +## Скоуп ревью + +Проверялось соответствие ТЗ: + +- `docs/SCOPE.md` — попадание в Core user jobs, отсутствие расширения скоупа; +- `PROCESS.md` §2.4, §2.5 (DoR), §7.1 (обязательные разделы), §3/§12 (запреты); +- `AGENTS.md` — классы файлов, ветка, трейлеры коммита ТЗ; +- каноническим документам: `docs/WALL-THICKNESS.md` (модель толщины, + mitre/bevel, union тел), `docs/LIGHT.md` (барьеры/`_lightBarriers`), + `docs/SUN.md` (occluders), `docs/ISOMETRIC.md` (`wallBodiesGeometry` + + extra physical bodies), `docs/CANVAS.md` (координаты/масштаб), + `docs/TOUCH-SUPPORT.md` (best-effort editors), `docs/CONFIG-COMPATIBILITY.md` + (миграция — в задаче её нет, но проверено, что это верно); +- `docs/USER-GUIDE.ru.md` — терминология «Контур комнаты» / «Перегородка»; +- фактическому коду (`src/wall-thickness.ts`, `src/physical-geometry.ts`, + `src/houseplan-card.ts`) — на предмет того, что технический диагноз ТЗ не + является непроверенной догадкой, выданной за факт. + +## Как проверялось + +1. Прочитан весь тред issue #141: исходное описание с уточнением владельца + («проявляется и на прямых, и на непрямых углах»), полная аналитика + (ценность 8/10, сложность/риск 7/10, P2, тип `bug`, полный трек) с Q1–Q3 и + явными defaults, решение владельца «принимаю все defaults», финальный + комментарий автора со ссылкой на коммит ТЗ. +2. Сверены обязательные разделы ТЗ (§7.1 PROCESS.md) построчно — таблица ниже. +3. Прочитан код и подтверждены построчно все технические утверждения §3 ТЗ: + - `drawWallPreviewD()` (`src/wall-thickness.ts:490`) действительно строит + `outset−inset` только для `closed`-контура (:496); открытый путь — + независимый quad на каждый сегмент (:512-529). Диагноз ТЗ п.1 точен. + - `partitionBody()` (`src/physical-geometry.ts:41`) строит прямоугольник с + плоскими торцами без топологии узла; `physicalBodies()` (:89) просто + собирает такие прямоугольники в список. Диагноз ТЗ п.2 точен. + - `MITRE_LIMIT = 4` (`src/wall-thickness.ts:28`) и его использование в трёх + местах (:669, :1429, :1805) существуют — ссылка ТЗ §7.2.3/AC3 не + изобретена. + - Изометрия (`src/houseplan-card.ts:4606-4611`) уже вызывает + `wallBodiesGeometry(..., extras)` с `extras = physicalBodies(...)` — + значит full/static/iso уже объединяют независимые тела через + `wallBodiesUnionPath`/`wallBodiesGeometry` (тот же приём подтверждён в + `src/space-render.ts:338` для статической карточки). Ровно то, что ТЗ + утверждает в §3.5 и §8.2 («full/static/isometric render уже объединяют…»). + - `_lightBarriers()` (`src/houseplan-card.ts:13556`) вызывает + `wallBodiesGeometry()` **без** `extras` (:13623) и затем отдельно кладёт + сырые ребра каждого `physical`-тела в occluders (:13637: + `for (const body of physical) occluders.push(...polygonSegments(body));`). + Солнце (`src/houseplan-card.ts:12705-12716`) вызывает + `directionalOccluders(physical, ...)` напрямую на тех же сырых + прямоугольниках. Это подтверждает диагноз ТЗ §3.5/§8.2: свет и солнце + сегодня действительно обходят raw-прямоугольники отдельно от joined-пути + рендера, а не голословное утверждение. + - `floorMinusBodies()` вызывается с тем же `_physicalBodiesR()` (сырые + тела) и для чистой площади (:7366, :7388), и для проверки source-inside + (:11655) — совпадает с AC8. +4. Прочитан `docs/WALL-THICKNESS.md` целиком: подтверждён существующий + контракт mitre/bevel для комнатных стен (§3) и отдельный раздел «9. + Independent partitions, drafts and columns» — независимые тела уже + юнионятся с комнатными только после вырезов проёмов и участвуют в + clean-floor/Glow/sun occlusion. ТЗ §7.3.2, §9 и AC6/AC10 корректно + продолжают именно эту, а не новую, модель. +5. Прочитан `docs/LIGHT.md` и `docs/SUN.md`: подтверждено, что `_lightBarriers` + держит один общий барьерный набор на пространство и кэшируется по + геометрическому fingerprint, а не по `_cfgEpoch` — ТЗ §8.3.1-2 корректно + продолжает существующий кэш-контракт, не изобретая новый механизм + инвалидации. +6. Прочитан `docs/ISOMETRIC.md`: `src/iso-walls.ts` явно описан как + потребитель «канонического `wallBodiesGeometry()` MultiPolygon after + openings and extra physical bodies have been resolved» — буквально + подтверждает ТЗ §8.2 («full/static/isometric не создают разные join + алгоритмы»), т.е. это не домысел автора, а прямая цитата канона. +7. Прочитан `docs/TOUCH-SUPPORT.md`: политика требует, чтобы «новые + спецификации editor-фич и код-ревью» явно указывали одно из трёх значений + `Touch editor: supported / best effort / not exposed`. ТЗ §10 описывает + ровно best-effort-поведение словами, но не использует эту точную метку — + см. Low-1. +8. Прочитан `docs/USER-GUIDE.ru.md` (раздел «Создание комнаты», таблица + «Инструменты плана», раздел «Виртуальные стены»): термины «Контур комнаты» + и «Перегородка» в ТЗ (§4.1, §5.3 и др.) совпадают с интерфейсным словарём + ровно там, где употребляются полностью; там же нашёл существующую строку + «Углы | Соседние тела стен соединяются…» (:347) — актуальное описание, + которое release-артефакты ТЗ (§15) обязывают дополнить для partitions — + корректно учтено. +9. Прочитан `src/types.ts:45-72` — `RoomDraftCfg`, `PartitionCfg`, + `space.room_drafts`, `space.partitions` существуют как реальные поля + схемы, а не придуманы для ТЗ; AC4/AC5 про кросс-типовые соединения + (partition↔partition, draft↔partition, partition/draft↔room wall) + технически имеют смысл на этой модели. +10. Проверена запись `docs/specs/README.md:85` — строка на #141 добавлена в + том же коммите; ссылка issue ↔ ТЗ двусторонняя (ТЗ ссылается на issue в + заголовке, issue-комментарий ссылается на файл ТЗ и коммит). +11. Проверены трейлеры и class-принадлежность: `git diff --stat + origin/dev...HEAD` показывает только `docs/specs/141-wall-junctions.md` и + `docs/specs/README.md` (класс C, ни одного файла класса A); коммит + `59c66dd` несёт `Issue: #141` и `User-Visible: no` — корректно для + документа ТЗ, который сам не меняет поведение. + +## Обязательные разделы (§7.1 PROCESS.md) + +| Раздел | Есть | Комментарий | +|---|---|---| +| Сценарий (персона/поверхность/момент) | ✅ | §1 — домашний администратор, desktop Plan editor, момент второго клика | +| Что человек увидит до/после | ✅ | §2, «До:»/«После:» — см. Low-2 | +| Проблема (с подтверждённой причиной) | ✅ | §3, шесть пунктов, все проверены по коду (см. выше) | +| Скоуп / не-скоуп | ✅ | §5 / §6, явные границы (без snap #137, без нового cap, без #138) | +| Контракт поведения | ✅ | §7 (геометрия) + §8 (архитектура) | +| Модель данных и миграция | ✅ | §9 — явное «schema не меняется», «читается исправленно, без записи» | +| UX, i18n, accessibility, touch | ✅ | §10 — см. Low-1 | +| AC1…ACn с доказательством | ✅ | §12, 13 штук, каждый с типом доказательства и наблюдаемым результатом | +| План автотестов | ✅ | §13, по подпунктам на unit/smoke/golden/performance | +| Риски | ✅ | §16, таблица с вероятностью/ущербом/снижением | +| Откат | ✅ | §17 | +| Release-артефакты | ✅ | §15, конкретный список файлов документации и обоих changelog | + +Все обязательные разделы присутствуют и содержательны. Дополнительно есть +раздел «Решения владельца» (§4, явно фиксирует принятые Q1–Q3), архитектурный +контракт (§8), план реализации (§14, помечен как техническая свобода) и явный +блок «принятые технические предположения» (§18) — соответствует духу +PROCESS.md §7.1 об отделении продуктового решения от технического. + +## Находки + +Находок уровня **High** и **Medium** нет — новых issue не требуется. + +### Low-1 — нет явной метки touch-контракта по `docs/TOUCH-SUPPORT.md` + +**Файл:** `docs/specs/141-wall-junctions.md:252-261` (§10) + +`docs/TOUCH-SUPPORT.md` требует: «New editor feature specifications and code +reviews must state one of: `Touch editor: supported`; `Touch editor: best +effort / intentionally degraded`; `Touch editor: not exposed`.» §10 ТЗ по +существу описывает best-effort-контракт словами («Plan editor остаётся +desktop-first… новый hover parity не обещается») и корректно ссылается на +safety floor (сохранённая geometry тапнутого сегмента должна совпадать), но не +использует ни одну из трёх канонических формулировок буквально. + +**Решение ревьюера:** Low, не блокирует. При следующей правке ТЗ или в +хендоффе код-ревью достаточно добавить одну строку `Touch editor: best effort +/ intentionally degraded` — по содержанию это именно то, что §10 уже +описывает. + +### Low-2 — «что человек увидит» длиннее одной фразы + +**Файл:** `docs/specs/141-wall-junctions.md:28-37` (§2) + +PROCESS.md §7.1 требует «одной фразой, без терминов реализации». Раздел +написан двумя короткими абзацами («До:» / «После:»), по одному предложению +каждый; по существу требование выполнено (без терминов реализации, конкретно +и однозначно), но формально это не «одна фраза». Тот же класс находки уже +фиксировался как Low и не блокировал приёмку в `SPEC-REVIEW-137-r1`. + +**Решение ревьюера:** Low, не блокирует. Косметическая правка на усмотрение +автора. + +### Low-3 — доказательство AC10 («code review») до кода не проверяемо, но это ожидаемо для code-review-этапа + +**Файл:** `docs/specs/141-wall-junctions.md:315-321` (AC10, AC11) + +AC10 и AC11 указывают доказательство «`unit` + code review» / «`performance` + +code review» — это не входит буквально в список §2.5 PROCESS.md +(`unit`/`backend`/`smoke`/`golden`/«ревью кода»), но «code review» — это то же +самое «ревью кода» иначе сформулированное, и AC11 корректно называет +`performance`, обязывая код-ревьюера прогнать performance-профиль по правилу +«гейты соразмерны AC» (`PROCESS.md` §8). Не дефект по существу, тот же класс +находки не блокировал `SPEC-REVIEW-137-r1` (Low-3 там). + +**Решение ревьюера:** Low, не блокирует. Оставить как есть. + +## Что проверено и корректно + +- **Соответствие `docs/SCOPE.md`:** задача закрывает **J4** («от нуля до + плана без внешнего редактора» — администратор должен доверять форме стены в + момент рисования, не после замыкания) и **J6** («план остаётся правдивым по + мере развития» — независимые перегородки не деградируют в кривую геометрию + навсегда). Обе строки Closed — это исправление дефекта внутри принятой + функциональности, не расширение продукта. +- **Легитимность полного трека:** сложность/риск 7/10, минимум две + поверхности (перегородки и рисование стен комнаты), влияние на golden, свет + и производительность — критерии `small`/`trivial` (§5/§5.1 PROCESS.md) не + выполняются ни по одному пункту; полный трек выбран верно. +- **Продуктовые вопросы закрыты по процессу:** Q1 (какие соединения входят), + Q2 (нормализуется ли rubber-band до клика) и Q3 (что делать со свободными + торцами) заданы одним пакетным комментарием, каждый с предлагаемым default, + issue корректно ушёл в `blocked`+`S3-spec` до ответа и вышел из `blocked` + сразу после решения владельца. Ни одна догадка не выдана за факт без + пометки — §18 отдельно и явно перечисляет технически свободные решения и + прямо фиксирует «нет открытых продуктовых вопросов» (§18.10). +- **Технический диагноз не голословен.** Все шесть пунктов §3 (независимый + путь `drawWallPreviewD`, плоские торцы `partitionBody`, отсутствие топологии + узла в `physicalBodies`, отдельный путь mitre для закрытого контура, + частичное объединение в full/static/iso против необъединённых barriers у + Glow/sun, неподтверждённость отдельного дефекта свободного торца по + скриншоту) построчно проверены по исходному коду и совпадают с ним — см. + «Как проверялось» п.3. +- **Не-скоуп (§6) корректно отсекает соседние соблазны:** snap-tolerance/#137, + автоматическое дробление persisted-геометрии, превращение partition в + границу комнаты/HA-зону, новый persisted junction-тип и миграция, новый + cap/join selector, #138 (замыкание контура по углам существующей комнаты) — + все явно исключены с указанием, почему это не эта задача. +- **Регрессионные гарантии сформулированы явно:** AC10 поимённо защищает + проёмы комнатных стен, virtual-T, nested/partial стены, экстерьер #123 и + колонны — то есть ровно те механизмы, которые уже используют + `MITRE_LIMIT`/union и могли бы негласно пострадать от новой топологии узлов. +- **Производительность:** §11/AC11 корректно ссылаются на существующую + 60-partition фикстуру `large-house-v1` и явно запрещают full-house boolean + union на каждый pointermove/HA-tick (§8.3.3) — согласуется с + `docs/LIGHT.md` («barriers keyed by geometry fingerprint, never `_cfgEpoch`») + и не вводит новый бюджет без решения процесса. +- **Модель данных и миграция (§9):** корректно заявлено «schema не меняется», + «читается исправленно без записи», без прямой/обратной миграции — сверено с + `docs/CONFIG-COMPATIBILITY.md`: задача не создаёт нового + compatibility-случая, потому что не меняет persisted-представление. +- **Release-артефакты (§15)** перечисляют конкретные существующие документы + (`WALL-THICKNESS.md`, `LIGHT.md`, `SUN.md`, `ISOMETRIC.md`, `ARCHITECTURE.md`, + `TESTING.md`, `STATUS.md`, `USER-GUIDE.ru.md`) и оба changelog в одном + implementation-коммите — соответствует §7.1/правилу 11 PROCESS.md. Golden + корректно ограничен принятием только через `npm run golden:accept -- + --reviewed` по полному Linux-артефакту (§13.3), perf/golden/browser-suite + верно отнесены к пре-релизному, а не implementation-гейту (§8, §11.4 + PROCESS.md). +- **Откат (§17)** корректно опирается на отсутствие миграции данных: revert + implementation-коммита восстанавливает прежний (дефектный) визуал без + риска для сохранённых данных. +- **Трассируемость:** `docs/specs/README.md:85` обновлён тем же коммитом + (`59c66dd`); ветка `issue/141-wall-joints` и трейлеры (`Issue: #141`, + `User-Visible: no`) корректны для документа класса C, который сам не меняет + поведение. `git diff --stat origin/dev...HEAD` не содержит ни одного файла + класса A — продуктовый код не тронут до `S5-ready` (правило №1). + +## Чего не проверял + +- Не проверял реализуемость «одного чистого вычисляемого frame» (§8.1) как + конкретной структуры данных/API — по правилам ТЗ это явно свободное + техническое решение автора кода (§18.2), не предмет ревью ТЗ. +- Не запускал автотесты, `golden`, `performance` или browser-смоки — на этапе + `spec` это не требуется; существование фикстур (`large-house-v1`, реальные + поля схемы, конкретные функции и их поведение) проверено чтением кода, а не + исполнением. +- Не проверял, что boolean-decomposition конкретной реализации (mitre patch + для узла степени 3+, T-разрез сквозного сегмента) технически осуществима + средствами `polyclip-ts` в разумное время — это открытое для автора кода + техническое решение, отмеченное в ТЗ как свободно изменяемое (§18.3), и + фактическая проверка бюджета (AC11) относится к код-ревью и пре-релизному + гейту. +- Не проверял корректность конкретных числовых оценок аналитики (8/10 · 7/10 · + P2) по существу — это поле владельца (PROCESS.md §2.2), уже принято явным + решением владельца до написания ТЗ. +- Не проверял связанные issue #123/#137/#138 по существу за пределами того, + что понадобилось для верификации ссылок ТЗ (регрессия #123, снэп #137, + разграничение с #138) — они не входят в предмет этого ревью. + +## Вердикт + +Зелёный. High: 0, Medium: 0. Три находки Low (отсутствие буквальной метки +touch-контракта по `docs/TOUCH-SUPPORT.md`; «что человек увидит» длиннее +одной фразы; типы доказательства AC10/AC11 вне буквального перечня §2.5) — ни +одна не блокирует приёмку, все либо правятся косметически при следующей +редакции, либо снимаются этой записью без нового цикла.