From aad625a84dd15f2600d5af19a2dab50dfec7122d Mon Sep 17 00:00:00 2001 From: "claude[bot]" <209825114+claude[bot]@users.noreply.github.com> Date: Fri, 14 Aug 2026 12:23:28 +0000 Subject: [PATCH] docs: review document for #138 Issue: #138 User-Visible: no --- docs/reviews/SPEC-REVIEW-138-r1.md | 218 +++++++++++++++++++++++++++++ 1 file changed, 218 insertions(+) create mode 100644 docs/reviews/SPEC-REVIEW-138-r1.md diff --git a/docs/reviews/SPEC-REVIEW-138-r1.md b/docs/reviews/SPEC-REVIEW-138-r1.md new file mode 100644 index 00000000..b3f6e0b8 --- /dev/null +++ b/docs/reviews/SPEC-REVIEW-138-r1.md @@ -0,0 +1,218 @@ +# Ревью ТЗ — issue #138, цикл r1 + +- **Issue:** https://github.com/Matysh/houseplan-card/issues/138 +- **ТЗ:** `docs/specs/138-adjacent-room-autoclose.md`, коммит `611c5a76f56ce83587eafbd00dbd6ad1a0f8c9d8` +- **Этап:** spec (PROCESS.md §2.4) +- **Вердикт:** красный · цикл r1/4 · High: 1 · Medium: 0 + +## Скоуп ревью + +Прочитаны в указанном порядке: `docs/SCOPE.md`, `AGENTS.md`, `PROCESS.md` (§1, +§2, §3, §4, §7, §12), тело issue #138 и оба комментария владельца (аналитика +Q1–Q5 с defaults, подтверждение ТЗ), `docs/USER-GUIDE.ru.md` (терминология +диалога комнаты, кнопка «Оставить замкнутыми стенами», секции про соседние +комнаты и общую стену), и связанный `docs/specs/137-plan-snap-overlay.md` как +основа, на которой строится #138. + +Задача не помечена `small`, ТЗ корректно лежит в `docs/specs/138-adjacent-room-autoclose.md` +и зарегистрировано в `docs/specs/README.md:49`. Формат ревью — полный документ, +не комментарий. + +## Как проверялось + +Ревью читало не только текст ТЗ, но и код, на который оно опирается, чтобы +отличить обоснованное техническое утверждение от догадки, выданной за факт: + +- `src/houseplan-card.ts`: `_markupClick()` (6454–6575), `_closeRoomContour()` + (6426–6452), `_resolvePlanDrawPoint()` (6011–6031), `_draftEndAt()` + (6580–6594) — сверка с §3 «Причина дефекта», §7.1 «Порядок разрешения + клика», §8 «Контракт замыкания». +- `src/plan-snap-overlay.ts` (1–180): структура `PlanSnapSegment`, + `sourceKind: 'room'|'draft'|'partition'`, `cutSegments`/`roomEdges`, + дедупликация по `axisKey` — сверка с §7.2 «Допустимый существующий + интервал» и терминологией «room-owned solid interval». +- `src/wall-thickness.ts:461` `applyWallThicknessToNewRoom()` — сверка с §9 + (наследование толщины общей стены) и AC7. +- `src/i18n/ru.json:9,267–269` — существующие `btn.keep_as_walls`, + `toast.room_overlap`, `toast.contour_min_edges`, `toast.contour_cannot_close` + — сверка с заявлением «новых i18n-ключей не требуется» (§4.4, §6). +- `docs/specs/README.md` — регистрация spec-файла. + +Каждое утверждение ТЗ о текущем поведении, которое удалось проверить чтением +кода, подтвердилось: терминология (`sourceKind`, `_markupClick`, +`_closeRoomContour`, `_resolvePlanDrawPoint`, `applyWallThicknessToNewRoom`, +«Оставить замкнутыми стенами») — не изобретена, а взята из реального кода. +Отдельно оценивалась полнота контракта: не пропущен ли граничный случай, +который спецификация не проговаривает и не относит явно в non-scope. + +## Находки + +### High-1 — контракт автозамыкания перехватывает и блокирует обычный второй клик по той же стене + +**Где:** `docs/specs/138-adjacent-room-autoclose.md` §7.1 (строки 105–119), +§7.2 (121–139), §8.1 (155–173), §8.2 (175–186). + +**Формулировка:** §7.1 п.3 требует проверять на автозамыкание **любой** +обычный клик, отличный от клика в собственную первую точку контура — без +условия «в контуре уже есть хотя бы одно нарисованное ребро». §7.2 требует +только геометрических условий (общий room-owned solid interval, содержит `A` +и финальную точку, положительная длина, точки различны) — тоже без условия на +уже накопленное число вершин. §8.1 требует **минимум три различные вершины** +у prospective polygon `[..., P, B] + B—A`, иначе (§8.2) диалог не открывается, +`B`/`P—B`/`B—A` не записываются, **и клик не превращается в обычное +добавление `B`** — то есть точка не появляется в drafting вообще, только +показывается existing validation toast. + +Отсюда прямое следствие: если пользователь ставит **первую** точку `A` на +существующем углу (`this._path = [A]`, `houseplan-card.ts:6544`), а **второй** +клик кладёт на другой конец **той же самой** прямой существующей стены (что +является совершенно естественным способом начать соседнюю комнату — сначала +отметить оба конца общей стены, а затем обвести остальной контур), — +`P` на этот момент равен `A` (в пути одна точка), и prospective polygon равен +`[A, B] + B—A`: **две** различные вершины, не три. Проверка §8.1 обязана +провалиться, и по букве §8.2 клик обязан быть отклонён с toast, а `B` — +**не добавлен в draft вообще**. + +Сегодня, до этой задачи, тот же клик работает штатно: `_markupClick()` не +делает такой проверки и просто добавляет `B` как обычную точку контура +(`houseplan-card.ts:6573–6574`, путь `this._path = [...this._path, pt]; +this._persistActiveDraftSegment();`). Показательно, что для симметричного +случая — клика в собственную первую точку — система уже сегодня специально +избегает этой ловушки: строка `houseplan-card.ts:6522` +(`const closing = this._path.length >= 3 && this._samePt(pt, this._path[0]);`) +трактует клик как попытку замкнуть контур **только когда уже накоплено +достаточно вершин**; при недостатке вершин точка добавляется как обычная — +ошибка «нужно минимум два ребра» никогда не показывается на пустом контуре. +Новый контракт для автозамыкания (§7.1 п.3) не воспроизводит этот защитный +гейт: он назначает автозамыкание кандидатом для проверки независимо от того, +сколько вершин уже нарисовано, а §8.2 явно запрещает деградацию к обычному +добавлению точки при провале. + +**Сценарий воспроизведения (по тексту ТЗ, до реализации — логическая проверка контракта):** + +1. Существует завершённая комната с прямой стеной `A—C` (общая длина, `A` и + `C` — её концы). +2. Пользователь выбирает «Контур комнаты», кликает в `A` — `this._path = [A]`. +3. Следующим кликом отмечает `C` — противоположный конец **той же** стены, + намереваясь провести по ней первую грань новой комнаты, а затем обвести + остальной периметр и замкнуть контур явным кликом в `A` (существующий, + неизменяемый способ, приоритет 1 из §7.1). +4. По §7.1 п.3 клик в `C` не равен `_path[0]=A`, не является Ctrl/Cmd — значит + проверяется как кандидат на автозамыкание. По §7.2 `A` и `C` лежат на одном + `sourceKind: room` сплошном интервале, точки различны, длина положительна — + eligibility выполнена. +5. По §8.1 prospective polygon `[A, C] + C—A` содержит 2 различные вершины, + что меньше требуемых трёх → невалиден. +6. По §8.2 диалог не открывается, `C` не добавляется в draft, показывается + toast «Чтобы замкнуть контур, сначала нарисуйте минимум две грани» + (`toast.contour_min_edges`), а **клик не превращается в обычное добавление + точки**. +7. Результат: пользователь не может продолжить рисовать — второй клик по + общей стене, который сегодня работает и является естественным способом + начать примыкающую комнату, теперь ничего не делает, кроме показа + сообщения об ошибке, не имеющего отношения к тому, что человек пытался + сделать. + +Это прямая регрессия текущего, работающего поведения — причём именно в той +области (рисование вдоль общей стены соседней комнаты), которую задача #138 +должна улучшить, а не ухудшить. Ни один AC (§13) и ни один пункт плана +unit-тестов (§14.1, пп.1–9) не покрывает этот случай: пункт 7 +(«одинаковые A/B, zero-length и current anchor не подходят») — про **совпадающие** +точки, а не про две **различные** точки на одном интервале при пустом ранее +контуре. Риск не упомянут и в таблице §17. + +**Почему это High, а не Medium:** правило §3.8 и §2.4 требует, чтобы контракт +поведения был однозначным и не содержал воспроизводимой логической ошибки; +здесь ошибка выводится напрямую из текста ТЗ без домысливания реализации, и +затрагивает основной, ежедневный workflow редактора (первые же клики при +разметке соседней комнаты — ровно сценарий из тела issue). Это не пограничный +кейс, а типичный способ начать рисовать общую стену. + +**Что нужно поправить в ТЗ (не решение реализации, а контракт):** +Автозамыкание должно проверяться только когда в контуре уже достаточно +вершин, чтобы в принципе замкнуться (аналогично существующему гейту +`this._path.length >= 3` для клика в первую точку — т.е. до клика в `B` уже +должно быть отрисовано **хотя бы одно** дополнительное ребро, не лежащее +целиком на том же интервале что и `A—B`), либо явно указать, что при провале +именно по причине «меньше трёх вершин» (в отличие от self-intersection/overlap/ +zero-area) клик **деградирует к обычному добавлению точки**, а не блокируется +toast'ом. Выбор между этими двумя вариантами — продуктовый (что видит +пользователь при клике по второй точке общей стены сразу после первой): +либо клик тихо добавляет обычную точку (как сегодня), либо показывает +ошибку. Это ровно тот вопрос, который стоило задать владельцу вместе с Q1–Q5, +и он остался незамеченным. + +## Что проверено и корректно + +- **Терминология и техническая база не выдуманы.** Все ссылки на + `_resolvePlanDrawPoint`, `_markupClick`, `_closeRoomContour`, `_draftEndAt`, + `_persistActiveDraftSegment`, `sourceKind: 'room'|'draft'|'partition'`, + `applyWallThicknessToNewRoom`, `plan-snap-overlay.ts`, кнопку «Оставить + замкнутыми стенами» и существующие toast-ключи — соответствуют + действительному коду. §3 «Причина дефекта» описывает код, каким он есть + сегодня, а не предположение. +- **Обязательные разделы §7.1 PROCESS.md на месте:** сценарий/персона/ + поверхность (§1), «что человек увидит до/после» одной фразой без терминов + реализации (§2), проблема (§3), scope/non-scope (§5–6), контракт поведения + (§7–9), UX и touch-деградация (§10), модель данных/миграция (§11), + AC1…AC12 с указанием способа доказательства (§13), план автотестов (§14), + риски (§17), откат (§18), release-артефакты (§16). +- **i18n корректно закрыт:** новых ключей нет, переиспользуются существующие + `toast.contour_min_edges`, `toast.contour_cannot_close`, `toast.room_overlap`, + `btn.keep_as_walls` — подтверждено чтением `src/i18n/ru.json`/`en.json`. +- **Наследование толщины общей стены (AC7, §9)** грамотно опирается на уже + существующий `applyWallThicknessToNewRoom()` (`wall-thickness.ts:461–484`), + который и сегодня пропускает интервалы с уже ненулевой толщиной (`cms[i] > + 0`) — новый код для этого не нужен, утверждение ТЗ верно. +- **Проёмы и cuts (§7.3, AC4)** корректно опираются на уже существующий + `cutSegments`/canonical cuts из #137 (`plan-snap-overlay.ts`), не вводят + второй resolver — соответствует Non-scope и архитектурному контракту §12. +- **Дедупликация общей стены между двумя комнатами (§9, «не создаёт вторую + физическую стену»)** согласуется с найденной в `plan-snap-overlay.ts` + дедупликацией сегментов по `axisKey` (один `PlanSnapSegment` на общую ось + независимо от числа комнат-источников) — заявление обосновано, не догадка. +- **Открытых продуктовых вопросов действительно не осталось** для того, что + было явно задано: Q1–Q5 из комментария аналитики и все технические defaults + подтверждены владельцем 2026-08-14 (issue-комментарий), ссылка есть в ТЗ + §4 и §19.7. «Ни одного открытого вопроса» здесь не является тревожным + сигналом самим по себе — вопросы были заданы и закрыты пакетом, а не + обойдены. +- **Release-артефакты и трек соответствуют процессу:** `User-Visible: yes`, + оба changelog в одном коммите, `docs/CANVAS.md`/`ARCHITECTURE.md`/ + `USER-GUIDE.ru.md` в списке правок (§16), полный трек (не `small`/`trivial`) + обоснован в аналитике — сложность и число задетых инвариантов оправдывают + выбор. +- **Non-scope сформулирован конкретно и исключает реальные соблазны** + расширения (путь по нескольким рёбрам, второй resolver проёмов, новая + визуальная индикация, hit-radius #137) — не оставляет скрытого расширения + скоупа. + +## Чего не проверял + +- Реализацию — её ещё нет; это ревью ТЗ, а не кода (PROCESS.md §2.4 не + предполагает исполнения на этом этапе). +- Golden/визуальные артефакты — ТЗ корректно утверждает, что новых пикселей + нет (§14.3), баз для сверки нет и не требовалось. +- Производительность вживую — утверждение об O(S)-поиске только на click + (§12, AC10) проверено чтением архитектуры snap-геометрии (кэшированный + snapshot в `_planSnapGeometrySnapshot()`), не профилированием — на этапе ТЗ + профилирование не требуется. +- Backend/schema — ТЗ утверждает отсутствие изменений (§11, AC12); код + `custom_components/houseplan/**/*.py` не затрагивается по scope ТЗ, отдельно + не сверялся построчно, поскольку заявленный scope его не касается. + +## Вывод + +Один блокирующий (High) дефект контракта: связка §7.1 п.3 + §7.2 + §8.1 + §8.2 +без защитного гейта по минимальному числу уже нарисованных вершин превращает +обычный, сегодня работающий второй клик по общей стене в блокирующую ошибку — +регрессия ровно того сценария, который issue #138 должен исправить. Остальной +контракт, включая проёмы, толщину, drafts/partitions, historyCancel/Save, +touch-деградацию и release-артефакты, проверен по коду и корректен. + +Возврат в «ТЗ в работе» (`S3-spec`), цикл r1/4. Исправление: явно решить (и +записать в ТЗ), что происходит при клике на другую точку того же интервала, +когда в контуре ещё недостаточно вершин для валидного замыкания — деградация к +обычному добавлению точки (по аналогии с текущим гейтом `path.length >= 3` для +закрытия по первой точке) либо иной явно обоснованный вариант, покрытый +отдельным AC и unit-тестом.