mirror of
https://github.com/Matysh/houseplan-card
synced 2026-10-02 12:49:56 +00:00
claude[bot]
committed by
Sergey Matyunin
Sergey Matyunin
parent
5582e9a35d
commit
06c7934667
@@ -0,0 +1,86 @@
|
||||
# SPEC-REVIEW-238-r1
|
||||
|
||||
- Issue: [#238](https://github.com/Matysh/houseplan-card/issues/238) — «Превью проёма: показывать расстояния до внутренних граней комнаты и стен»
|
||||
- ТЗ: `docs/specs/238-opening-inner-distances.md`
|
||||
- SHA ревью: `0498dda7cc49e5769eee51a402204bba92ac304b` (ветка `issue/238-opening-inner-distances`)
|
||||
- Заход: r1 · блокирующих циклов израсходовано 0 из 4
|
||||
- Этап: ТЗ на ревью (PROCESS.md §2.4)
|
||||
- Метка `small`/`trivial` отсутствует (сложность 7/10, новый UX-контракт) → полный трек, спека в `docs/specs/`, что и сделано корректно.
|
||||
|
||||
## Скоуп проверки
|
||||
|
||||
Первый заход — разбор полный (§2.10 неприменим до второго цикла). Проверено:
|
||||
|
||||
1. `docs/SCOPE.md` — привязка к J4/J6, персона «администратор дома», Plan editor desktop-first.
|
||||
2. `AGENTS.md`, `PROCESS.md` §2.4/§7.1/§7.2/§4 — обязательные разделы, лимит циклов, формат вердикта.
|
||||
3. Тело issue #238 и оба комментария (аналитика S2, «ТЗ готово к ревью»).
|
||||
4. `docs/USER-GUIDE.ru.md` (терминология проёмов, размеров Resize, инвариант Shift) и `docs/CANVAS.md` §9.4, `docs/WALL-THICKNESS.md` — канонические документы затронутой подсистемы.
|
||||
5. Сам файл `docs/specs/238-opening-inner-distances.md` целиком (406 строк) и запись в `docs/specs/README.md`.
|
||||
6. Исходный код, на который ссылается диагностика ТЗ (§3, §7–§11): `src/houseplan-card.ts`, `src/opening-placement.ts`, `src/logic.ts` (`openingShoulders`), `src/wall-thickness.ts` (`wallIntervals`, `roomWallProfile`, `innerContourForRoom`, `innerEdgeSpan`, `ownEdgeOffsets`), `src/physical-geometry.ts` (`partitionBody`, `wallBodiesGeometry`).
|
||||
7. Существование упомянутых регрессионных smoke-файлов и golden-baseline.
|
||||
|
||||
## Как проверялось
|
||||
|
||||
Не код-ревью, поэтому автотесты не гонялись — это ревью ТЗ (§2.4): проверялась выполнимость и проверяемость, а не реализация. Конкретно:
|
||||
|
||||
- `git log`/`git show 0498dda --stat` — коммит содержит только `docs/specs/238-opening-inner-distances.md` и `docs/specs/README.md`, `User-Visible: no`, `Issue: #238` — трейлеры корректны для чисто документационного коммита.
|
||||
- `grep`/`Read` по `src/*.ts` — сверка каждого технического утверждения §3 «Подтверждённый диагноз» с реальным кодом (см. таблицу ниже).
|
||||
- `grep -n shiftKey src/` — проверка утверждения AC10 про Shift на предмет «догадки, выданной за факт».
|
||||
- Сверка списка файлов §14 и regression-smoke §17 с реальным деревом (`ls demo/smoke_*.mjs`, `ls demo/golden/baselines`).
|
||||
|
||||
## Находки
|
||||
|
||||
**High: 0 · Medium: 0 · Low: 0** (после проверки один кандидат в Low снят как необоснованный — см. ниже).
|
||||
|
||||
Блокирующих и требующих правки замечаний нет. Ниже — два пункта, которые были целенаправленно проверены как потенциальные «догадки, выданные за факт», и оба сняты с объяснением, а не молча пропущены.
|
||||
|
||||
### Проверенный кандидат №1: AC10 и упоминание Shift
|
||||
|
||||
AC10: «Centre magnet/tick остаётся осевым; Shift не выключает его...». В коде `_resolveOpeningPlacement`/`_opRuler` (`src/houseplan-card.ts:11935-12238`) нет обращения к `shiftKey` вообще — центровочный магнит проёма безусловен. На первый взгляд это похоже на факт о поведении, которого в коде нет и который не помечен как предположение (что было бы замечанием по инструкции ревью).
|
||||
|
||||
Проверка канона показала обратное: `docs/CANVAS.md` §9.4 фиксирует общий, не привязанный к конкретному инструменту, инвариант редактора — «`Shift` never suspends coordinate snapping» — и явно перечисляет, что для прочих инструментов Shift меняет только текущий жест (квадрат/круг, независимые оси resize, компас, обход furniture-магнита), но никогда не снимает базовый snap. AC10 — корректное применение уже существующего канонического правила к новому UX-элементу (центр-магнит проёма), а не изобретённая деталь. Поскольку в текущем коде Shift вообще не читается на этом пути, AC тривиально выполняется и работает как regression guard на будущее — не дефект. **Снято, находка не подтвердилась.**
|
||||
|
||||
### Проверенный кандидат №2: §3 не называет `innerEdgeSpan`/`ownEdgeOffsets`
|
||||
|
||||
§3 «Подтверждённый диагноз» перечисляет уже существующую физическую базу (`wallIntervals`, `roomWallProfile`, `innerContourForRoom`, `partitionBody`), но не упоминает `innerEdgeSpan`/`ownEdgeOffsets` (`src/wall-thickness.ts:876-959`), которые #233 уже завёл для ровно того же продуктового принципа — «мерить до внутренней физической грани, а не до оси» — и импортированы в `houseplan-card.ts:74`.
|
||||
|
||||
Прочитал обе функции: `ownEdgeOffsets` сэмплирует толщину соседей **по одной точке — середине** цельного нарисованного ребра комнаты, а `innerEdgeSpan` режет это ребро целиком по двум соседним рёбрам. Для Resize (#233) это корректно: перетаскивается всё ребро целиком. Для §238 этого недостаточно **по существу**: превью проёма стоит в произвольной точке ребра, и на этой точке может лежать не тот атомарный отрезок толщины, что в середине ребра (ребро комнаты может быть склеено из нескольких atomic-интервалов с разной толщиной соседей — ровно то, что описывает модель в `docs/WALL-THICKNESS.md` §1, «atomic collinear spans»). Требование ТЗ §7.1 п.1 — «участок, покрывающий **центр candidate**», а не середину всего ребра — поэтому обоснованно требует нового резолвера на уровне atomic-профиля, а не переиспользования #233 «как есть». Отсутствие явной ссылки на `innerEdgeSpan`/`ownEdgeOffsets` в §3 — стилистический пробел (было бы полезно явно написать «недостаточно, потому что…», как самому ревью пришлось восстанавливать), но не ошибка и не выдуманное поведение. **Не поднимаю как находку** — ниже, чем Low, чисто редакторское замечание.
|
||||
|
||||
## Что проверено и корректно
|
||||
|
||||
Построчная сверка §3 «Подтверждённый диагноз» с кодом — всё подтвердилось:
|
||||
|
||||
| Утверждение ТЗ | Проверка |
|
||||
|---|---|
|
||||
| `openingShoulders()` считает по осевым `roomEdges(rooms)`, не видит толщины | `src/logic.ts:269-308` — использует `pts` из `room.poly` напрямую, без offset/half-width |
|
||||
| `_resolveOpeningPlacement()`/`resolveOpeningPlacementResult()` — единая точка разрешения candidate для hover/click | `src/houseplan-card.ts:11935,11956`; `_openingHoverCandidate` кеш переиспользуется кликом (`:12041-12044`, `:12588-12598`) |
|
||||
| `OpMeasure` хранит только точку и текст, без геометрии сегмента | `src/houseplan-card.ts:688-691`: `{ labels: {x,y,text}[], guide }` — подтверждено, `from/to` действительно нет |
|
||||
| `wallIntervals()` даёт half-thickness и roomId по каждому атомарному участку | `src/wall-thickness.ts:1294-1360`: `WallInterval{roomId, half, open, ...}` |
|
||||
| `innerContourForRoom()`/`roomWallProfile()` строят внутренний контур через `insetContour` | `src/wall-thickness.ts:1545-1562` |
|
||||
| `partitionBody()` строит тело независимой перегородки с её толщиной | `src/physical-geometry.ts:80-85` |
|
||||
| Дорогой plan-wide boolean union существует и должен избегаться на pointermove | `wallBodiesGeometry` — тяжёлый вызов на весь `space.rooms`+`walls`+`openCuts` (`src/houseplan-card.ts:5175-5190,15583-15588`); кеш-инвалидация по `_cfgEpoch` — реальное поле (`:3329`, инкрементируется на изменениях конфига, уже используется как ключ кеша структурной геометрии, `:6711,6739`) |
|
||||
| Разные `roomId` на одном сегменте → общая стена, распад до legacy при недоказуемости | Логика в `wallIntervals()` (по одному входу на каждую комнату) делает это семантически корректным без дополнительной дедупликации в исходнике |
|
||||
|
||||
Проверено также:
|
||||
|
||||
- Все обязательные разделы §7.1 PROCESS.md присутствуют и в верном порядке (сценарий/что увидит человек — первыми): §1–§21 покрывают сценарий, персону, диагноз/проблему, скоуп и не-скоуп, контракт поведения, модель данных/миграцию, i18n, touch/безопасность, AC1–AC15 с проверяемым доказательством (`unit`/`smoke`/`golden`/`ревью кода`), план автотестов, риски, откат, release-артефакты.
|
||||
- AC1 арифметически проверен вручную: ось 400u, проём 80u по центру → legacy плечо (400-80)/2=160u; примыкающая стена глубиной 20u даёт half=10u инсет на каждом конце по правилу «growth half in / half out» (`docs/WALL-THICKNESS.md` §2) → 160-10=150u. Число в AC не взято с потолка.
|
||||
- Регрессионные smoke-цели существуют: `demo/smoke_opening_measure.mjs`, `demo/smoke_opening_preview.mjs` — по факту; `demo/smoke_partition_opening_jamb.mjs` из текста не существует, но ТЗ прямо и корректно оговаривает это как предположительное имя («сверить с inventory») — реальный файл `demo/smoke_partition_openings.mjs` содержит нужный jamb-margin тест (issue #132/#185/#186).
|
||||
- Golden: существующие `opening-placement-*-thick-wall-{light,dark}.png` в `demo/golden/baselines/` подтверждают, что сцена placement-preview для переиспользования/расширения реально существует, как и оговорено в §17 «либо новая минимальная».
|
||||
- `docs/specs/README.md`: строка добавлена в верном месте таблицы, в двустороннем формате issue↔ТЗ, ссылки консистентны.
|
||||
- Не-скоуп (§5.2) корректно опирается на формулировки issue: все три упоминания «размещения»/«превью проёма» в теле issue относятся к **новому** проёму, а не к drag уже сохранённого — сужение до `AC11`/§5.2 не является домысливанием.
|
||||
- §21 «Принятые предположения» — блок присутствует, все 5 пунктов действительно технические/интерпретационные, не продуктовые в смысле §7.1 PROCESS.md (что видит/делает человек уже прямо зафиксировано issue), помечены как оспоримые.
|
||||
- Аналитика (S2) корректно классифицировала задачу как обычный трек — сложность 7/10 превышает порог `small` (≤3).
|
||||
- Откат (§19) реалистичен: persisted-полей нет, откат — чисто код.
|
||||
|
||||
## Чего не проверял
|
||||
|
||||
- Реализацию — её не существует, стадия ТЗ.
|
||||
- Golden/визуальные baseline для новых 2/4 линий — они появятся только в коде; на этой стадии оценивался только план их получения (§17).
|
||||
- Полный HA backend/`tests_backend` — задача не трогает `custom_components/**`, ТЗ явно это фиксирует (§12), не перепроверялось глубже grep по `custom_components`.
|
||||
- Производительность фактическая (только план кеширования по тексту ТЗ, не измерения — на этой стадии измерений и не может быть).
|
||||
- Не проверял весь `docs/UX-MODES.md`/`docs/TOUCH-SUPPORT.md` построчно — только целевые фрагменты про Shift/canvas и точечно про touch/best-effort инвариант, поскольку §13 ТЗ не меняет touch-контракт и это подтверждается отсутствием новых жестов в тексте.
|
||||
|
||||
## Вердикт
|
||||
|
||||
Зелёный. ТЗ выполнимо, каждый AC проверяем и привязан к способу доказательства, диагностика подтверждена чтением исходного кода, продуктовых вопросов владельцу нет — 5 явных предположений корректно оформлены и оспоримы. High: 0, Medium: 0.
|
||||
Reference in New Issue
Block a user