diff --git a/docs/reviews/SPEC-REVIEW-173-r1.md b/docs/reviews/SPEC-REVIEW-173-r1.md new file mode 100644 index 00000000..c2a77bd0 --- /dev/null +++ b/docs/reviews/SPEC-REVIEW-173-r1.md @@ -0,0 +1,306 @@ +# SPEC-REVIEW-173-r1 + +- **Issue:** https://github.com/Matysh/houseplan-card/issues/173 +- **ТЗ под ревью:** `docs/specs/173-unified-wall-tool.md` (коммит `d096a02`, + ветка `issue/173-unified-wall-tool`) +- **Роль:** ревьюер ТЗ (не автор), этап `S4-spec-review` +- **Трек:** обычный (не `small`/`trivial`) — аналитика владельца прямо + говорит «`small`/`trivial` неприменимы: это новый UX-контракт, несколько + модулей и высокая геометрическая цена ошибки», сложность/риск 9/10. + Полный трек и отдельный файл ТЗ выбраны верно. +- **Цикл:** r1/4 + +## Скоуп ревью + +Проверялось соответствие ТЗ: + +- `docs/SCOPE.md` — попадание в Core user jobs (J4/J6), отсутствие + расширения скоупа за пределы описанного объединения инструментов; +- `PROCESS.md` §2.4/§2.5 (DoR), §7.1 (обязательные разделы ТЗ, продуктовые + vs технические вопросы), §3/§12 (запрет «догадка вместо решения»); +- `AGENTS.md` — классы файлов, имя ветки, трейлеры коммита ТЗ; +- каноническим документам подсистемы: `docs/UX-MODES.md` (toolbar/tool + names, режимы, touch-политика), `docs/TOUCH-SUPPORT.md` (обязательная + формула деградации), `docs/CANVAS.md`/`docs/WALL-THICKNESS.md` + (planar-геометрия, provenance, толщина, island-контракт), `docs/ARCHITECTURE.md` + (`polyContainsPoly`/`islandsOf`); +- `docs/USER-GUIDE.ru.md` — текущая терминология «Контур комнаты» / + «Перегородка»; +- фактическому коду (`src/houseplan-card.ts`, `src/plan-snap-overlay.ts`, + `src/i18n/{ru,en}.json`) — чтобы диагноз текущего состояния (§3 ТЗ) не + оказался непроверенной догадкой, выданной за факт; +- полному треду issue #173 — продуктовая идея владельца, аналитика Codex, + пакет вопросов Q1–Q7 с default'ами, решения владельца, хендофф автора ТЗ. + +## Как проверялось + +1. Прочитан весь тред issue #173: исходная продуктовая идея владельца + (18.08.2026), аналитика Codex (ценность 9/10, сложность/риск 9/10, P1, + явное «`small`/`trivial` неприменимы»), пакет вопросов Q1–Q7 (каждый с + предложенным default), ответ владельца «Q1 принят с правкой… Q2–Q7 приняты + предложенные defaults без изменений», занятие автора ТЗ и хендофф со + ссылкой на `docs/specs/173-unified-wall-tool.md`. +2. Сверены обязательные разделы ТЗ (§7.1 PROCESS.md) построчно — таблица + ниже. +3. Прочитан код и построчно сверены ключевые фактические утверждения §3 ТЗ + о текущем состоянии: + - `type MarkupTool = ... | 'draw' | 'partition' | ...` + (`src/houseplan-card.ts:497`) — два отдельных инструмента подтверждены; + - `markup.add` = «Контур комнаты» / «Room outline», + `markup.partition` = «Перегородка» / «Partition» + (`src/i18n/ru.json:13,74`, `src/i18n/en.json:13,74`) — «До:» из §2 ТЗ + совпадает с реальными текущими подписями кнопок, а не с уже + переименованным текстом канона (см. Low-2 ниже про сам канон); + - `MAX_PARTITIONS = 2000` (`src/houseplan-card.ts:531`), + `_keepClosedAsPartitions` (`:11647`), `_contourClosed` getter (`:11177`) + — существующий механизм ровно такой, как описан в §3 п.2/5; + - `src/plan-snap-overlay.ts` существует отдельным модулем (692 строки + иначе не совпало бы), собирает `room`/`draft`/`partition` сегменты с + opening/open-span cuts (`:115-153`) и строит только endpoint/line-snap + кандидаты (`:265-289`) — не строит faces. Это подтверждает §3 п.4 и + §13.2 («второй opening resolver запрещён», «делит с detector общий pure + collector») как техническую границу, реально существующую в коде, а не + придуманную; + - `docs/ARCHITECTURE.md:990-991` подтверждает «действующий island-room + контракт» (`polyContainsPoly`, `islandsOf`), на который ссылается §10.1 + ТЗ, — не изобретённое понятие; + - `docs/WALL-THICKNESS.md:207` («an interior↔interior X crossing keeps + normal…») и `docs/specs/141-wall-junctions.md` подтверждают, что X не + становится persisted-узлом в #141 — ровно то ограничение, которое §3 п.7 + и §9.2.3 ТЗ фиксируют явно («X становится вычисляемым узлом только для + face topology; persisted записи и физический join-контракт #141 не + меняются»). Технической коллизии с #141 нет. + - `toast.room_overlap` (`src/houseplan-card.ts:6442,6692`) подтверждает, + что «текущая validation/toast семантика» partial-overlap (§10.1) — + реально существующий, а не гипотетический путь. +4. Прочитан `docs/UX-MODES.md` целиком (раздел «Plan — independent physical + objects», строки 164–180) и `docs/TOUCH-SUPPORT.md` целиком. Формулировка + ТЗ §12 «Touch editor: best effort / intentionally degraded» — буквальная + канон-метка, требуемая `TOUCH-SUPPORT.md` («Documentation rule»). Контракт + §12 (pinch/pan/pointercancel/second pointer не завершают цепочку и не + создают геометрию) прямо реализует «Safety floor», в частности пункт + «saving unintended geometry merely because a pinch, pointer cancellation or + second touch was misread as a click» — не изобретение автора. +5. Проверено разделение продуктовых и технических вопросов (PROCESS.md §7.1: + владельцу — только «что видит/делает» и «объём видимых изменений»). + Q1–Q7 — все о наблюдаемом поведении (когда завершается цепочка, что + считать эквивалентом «замыкания», в каком порядке идут диалоги, нужен ли + тихий режим, остаётся ли Split). Ни один не является техническим вопросом, + ошибочно вынесенным владельцу. Блок §20 («Принятые технические + предположения») отдельно перечисляет 10 технических решений, прямо + помеченных как «можно менять свободно», и завершается явным «Открытых + продуктовых вопросов нет: … Q2–Q7 — defaults, 18.08.2026» — это корректное + разделение, а не спрятанная догадка. +6. Проверены все 17 AC (§14) на однозначность и явное указание способа + доказательства — у каждого в скобках назван метод (`unit`/`smoke`/ + `golden`/`performance`/`code review`) и исполнитель. Отдельно проверено, + что AC1–AC13 покрывают все пункты Scope (§5), AC14–AC17 — совместимость, + производительность, touch и гейты реализации; расхождений между Scope и + AC не найдено. +7. Сверена трассируемость: `docs/specs/README.md:96` обновлён тем же + коммитом `d096a02`; `git diff --stat origin/dev...HEAD` содержит только + `docs/specs/173-unified-wall-tool.md` и `docs/specs/README.md` (класс C, + ни одного файла класса A — правило №1 не нарушено на этапе ТЗ). Коммит + несёт `Issue: #173`, `User-Visible: no` — верно для документа ТЗ. +8. Проверено, не дублирует ли задача открытые основания (#137, #138, #141, + #150, #172): все перечислены в ТЗ (заголовок «Связано») как готовые + строительные блоки (snap, adjacent-autoclose, стыки, толщина), а не как + пересекающийся скоуп; #138 (adjacent room autoclose) явно указан как + регрессионный сценарий, который должен остаться зелёным (AC7) — граница + с #173 (полный planar graph против узкого room-owned-edge контракта #138) + проведена явно в §3 п.3. + +## Обязательные разделы (§7.1 PROCESS.md) + +| Раздел | Есть | Комментарий | +|---|---|---| +| Сценарий (персона/поверхность/момент) | ✅ | §1 — администратор дома, desktop Plan editor, момент выбора инструмента перед первым кликом | +| Что человек увидит до/после | ✅ | §2, «До:»/«После:», проверено по факту i18n (см. «Как проверялось» п.3) | +| Проблема | ⚠️ | Явного раздела «Проблема» нет — см. Low-1; содержательно проблема сформулирована во вступлении и §1, а §3 «Подтверждённое текущее состояние» несёт функцию причины/диагноза | +| Скоуп / не-скоуп | ✅ | §5 / §6, 13 и 12 пунктов соответственно, границы явные (Split не трогается, нет persisted node/face, нет новой настройки) | +| Контракт поведения | ✅ | §7 (термины/инварианты) + §8 (UX) + §9 (planar graph) + §10 (создание комнат) | +| Модель данных и миграция | ✅ | §11 — явное «новых persisted полей нет», совместимость старых drafts/partitions/warm tool расписана по каждому случаю | +| UX, i18n, accessibility, touch | ✅ | §12, включая буквальную канон-метку touch-деградации | +| AC1…ACn с доказательством | ✅ | §14, 17 штук, каждый помечен методом и исполнителем | +| План автотестов | ✅ | §15, unit / targeted smoke / golden / performance+backend / implementation loop | +| Риски | ✅ | §18, 12 строк риск → мера | +| Откат | ✅ | §19 | +| Release-артефакты | ✅ | §17, конкретный список из 13 файлов/каналов, включая оба changelog в implementation-коммите | + +Единственное отклонение от конвенции — отсутствие отдельного заголовка +«Проблема» (все предыдущие ТЗ, например #138, #172, выделяют её отдельным +разделом). Содержательно проблема присутствует (см. вступление и §1: «перед +первым кликом надо решить внутренний вопрос модели… при ошибочном выборе +приходится менять инструмент либо перерисовывать геометрию»), поэтому это +Low, не High/Medium — см. находки. + +## Находки + +Находок уровня **High** нет. Находок уровня **Medium** нет — новых issue не +требуется. + +### Low-1 — нет отдельного раздела «Проблема» + +**Файл:** `docs/specs/173-unified-wall-tool.md` (между §2 и §4; текущий §3 +называется «Подтверждённое текущее состояние») + +PROCESS.md §7.1 перечисляет «проблема» отдельным обязательным разделом, и все +предшествующие ТЗ в репозитории (`138-adjacent-room-autoclose.md:44`, +`172-zero-divider-taper.md:36`) держат его отдельно от «текущего состояния». +В ТЗ #173 формулировка проблемы («пользователь должен заранее решить, что он +рисует… при ошибочном выборе — менять инструмент или перерисовывать») звучит +только во вступлении к issue и в первом абзаце §1, а раздел §3 сразу +переходит к построчному разбору кода. По существу пропуска нет — читатель +получает и мотивацию, и диагноз, — но формально это отход от структуры, +которая делает разделы машинно/визуально сверяемыми. + +**Решение ревьюера:** Low, не блокирует. Можно поправить косметически (дать +§3 заголовок «Проблема и подтверждённая причина», как в #172) при следующей +редакции либо снять записью без нового цикла. + +### Low-2 — `docs/UX-MODES.md` уже называет текущий инструмент «Walls» + +**Файл:** `docs/UX-MODES.md:83, 166` vs `docs/specs/173-unified-wall-tool.md` +§2 «До:», §8.1 + +Канонический документ подсистемы в описании **текущего** (до #173) состояния +Plan-редактора пишет «Toolbar tools: **Walls** / room outline… Partition, +Column…» и «**Walls** still draws room contours» — то есть уже сейчас +называет инструмент «Walls», хотя фактический публичный лейбл кнопки +(`markup.add`) — «Контур комнаты» / «Room outline» (проверено по +`src/i18n/{ru,en}.json:74`, см. «Как проверялось» п.3). ТЗ #173 корректно +использует настоящую текущую подпись («Контур комнаты» / «Room outline») в +§2/§3, но не упоминает, что `UX-MODES.md` уже содержит забежавшую вперёд +формулировку «Walls» для другого инструмента (сегодняшнего `draw`), что +создаёт двусмысленность при будущем чтении истории документа. Это дрейф, +существовавший в `UX-MODES.md` до #173, не внесённый этим ТЗ, и раздел +release-артефактов (§17) обязывает обновить `UX-MODES.md` в implementation +коммите — то есть у автора кода будет естественная точка исправить +формулировку заодно. + +**Решение ревьюера:** Low, не блокирует приёмку ТЗ. Фиксирую для code review +#173: при правке `docs/UX-MODES.md` в рамках этой задачи убедиться, что +итоговый текст однозначно называет единственный инструмент «Walls»/«Стены» и +не оставляет старой двусмысленной фразы про «Walls / room outline» рядом с +отдельным «Partition». + +### Low-3 — опечатка в AC17 + +**Файл:** `docs/specs/173-unified-wall-tool.md:522` + +`- **AC17 (`typecheck` + `unit` + `build` + documentation review`; разработчик):**` +— лишний обратный апостроф после «review» перед точкой с запятой (должно быть +`... + documentation review; разработчик)`). Чисто косметическая опечатка +форматирования Markdown, не меняющая смысл критерия. + +**Решение ревьюера:** Low, не блокирует. Правится одним символом при +следующей редакции. + +## Что проверено и корректно + +- **Соответствие `docs/SCOPE.md`:** задача закрывает **J4** («от нуля до + плана без Inkscape/YAML» — рисование стен без знания внутренней модели + данных) и **J6** («план остаётся правдивым по мере развития» — merge/split/ + drag редактируются тем же единым инструментом). Обе строки Closed; это + улучшение UX-контракта существующей функциональности, а не расширение + продукта за пределы SCOPE. +- **Легитимность полного трека:** сложность/риск 9/10 по собственной оценке + аналитики, новый UX-контракт, минимум четыре затронутых модуля/поверхности + (toolbar/tool state, planar-graph detector, room-creation/Split, snap- + overlay) — критерии `small`/`trivial` (§5/§5.1 PROCESS.md) не выполняются + ни по одному пункту, что аналитика прямо констатирует. +- **Продуктовые вопросы закрыты по процессу:** пакет Q1–Q7 задан одним + комментарием, каждый вопрос — с предложенным default и явной формулировкой + «что человек видит/делает»; issue корректно ушёл в `blocked` до ответа + владельца и вышел из `blocked` сразу после решения. Ни один вопрос не + является техническим, ошибочно вынесенным владельцу — все семь про + наблюдаемое поведение (завершение цепочки, эквиваленты замыкания, порядок + диалогов, судьба Split). +- **Диагноз текущего состояния не голословен.** Утверждения §3 о `MarkupTool`, + `_contourClosed`, `_keepClosedAsPartitions`, `MAX_PARTITIONS`, + `plan-snap-overlay.ts` и реальных подписях кнопок построчно сверены с + `src/houseplan-card.ts`, `src/plan-snap-overlay.ts` и `src/i18n/*.json` — + см. «Как проверялось» п.3. +- **Границы с соседними задачами (#137/#138/#141/#150/#172) проведены явно**, + без пересечения скоупа: #138 остаётся регрессионным контрактом (AC7), #141 + не получает нового persisted X-узла (§3 п.7, §9.2.3, сверено с + `docs/WALL-THICKNESS.md` и `docs/specs/141-wall-junctions.md`), #150/#172 + остаются единственным источником физической геометрии стен (§10.4 п.7). +- **Не-скоуп (§6) корректно отсекает соседние соблазны:** удаление Split, + автоматические предложения при reload/move/Align/Undo, фоновая миграция + старых drafts, persisted node/face, новая настройка «не предлагать», + создание комнаты через проём — всё явно исключено с обоснованием и + соответствует принятым Q2–Q7. +- **Модель данных и миграция (§11):** корректно заявлено «новых persisted + полей нет», отдельно расписана судьба legacy warm-tool значения + `partition`, старых drafts/partitions (остаются читаемыми, не + конвертируются автоматически), downgrade без rollback — соответствует + `docs/CONFIG-COMPATIBILITY.md`. +- **UX/touch (§12):** дословно использует обязательную канон-формулировку + `docs/TOUCH-SUPPORT.md` и отдельно перечисляет touch safety floor + (pinch/pan/second-pointer/pointercancel не создают геометрию) — прямая + реализация «Safety floor that still applies to touch editors». +- **Planar-graph контракт (§9) методологически надёжен:** явно запрещён + «pairwise любой цикл без выделения faces» (частая ошибка naive-детекторов), + явно требуется canonical ring identity, инвариантная к порядку/id/ + направлению — это именно то, что делает AC5 проверяемым unit-тестом с + перестановками, а не подверженным флаки. +- **Дисциплина «тест должен уметь падать»:** §15.1 прямо требует «минимум + один topology test должен краснеть на `origin/dev` без detector», а + ревьюеру кода прямо поручено проверить его mutation discipline — + соответствует PROCESS.md §2.7/§18. +- **Performance-контракт (§13, AC15)** ссылается на реально существующий + профиль `large-house-plan-snap-v1` + (`demo/performance/budgets-large-house-plan-snap.json`) и требует не + ослаблять исходные бюджеты — не изобретённая, а существующая точка + измерения. +- **Release-артефакты (§17)** перечисляют оба changelog в implementation- + коммите, конкретные документы для обновления (`USER-GUIDE.ru.md`, + `UX-MODES.md`, `CANVAS.md`, `ARCHITECTURE.md`, `WALL-THICKNESS.md`, + `TESTING.md`), golden matrix и три синхронные копии бандла — golden явно + ограничен `golden:accept -- --reviewed` по полному Linux-артефакту + (согласуется с PROCESS.md §8/§11.4/правилом 13). +- **Трассируемость:** `docs/specs/README.md:96` обновлён тем же коммитом + `d096a02`; ветка `issue/173-unified-wall-tool` и трейлеры (`Issue: #173`, + `User-Visible: no`) корректны для документа ТЗ, который сам не меняет + поведение продукта. `git diff --stat origin/dev...HEAD` не содержит ни + одного файла класса A — правило №1 не нарушено на этапе ТЗ. +- **Явный блок технических предположений (§20)** из 10 пунктов корректно + отделяет техническую свободу («какой именно алгоритм DCEL», «структура + кэша», «имена файлов/smoke/golden id») от продуктового решения и явно + фиксирует «открытых продуктовых вопросов нет» со ссылкой на дату и + комментарий владельца. + +## Чего не проверял + +- Не проверял реализуемость конкретного алгоритма half-edge/DCEL как + единственно возможного решения planar-graph детектора — §20 п.1 явно + оставляет выбор алгоритма технической свободой; код-ревью будет проверять + выполнение AC4/AC5, а не конкретную структуру данных. +- Не запускал автотесты, `golden`, browser-смоки или performance-профили — + на этапе `spec` это не требуется и большинство названных файлов + (`test/wall-face-graph.test.mjs`, `demo/smoke_unified_wall_tool.mjs`) ещё + не существуют по плану ТЗ, а не по недосмотру; существование уже + используемых зависимостей (`src/plan-snap-overlay.ts`, + `large-house-plan-snap-v1` профиль, `polyContainsPoly`/`islandsOf`) + проверено чтением кода/конфигурации, не исполнением. +- Не проверял корректность числовых оценок аналитики (9/10 · 6/10 · 9/10 · + P1) по существу — это поле владельца (PROCESS.md §2.2), уже принято явным + решением до написания ТЗ. +- Не проверял детально совместимость с #137 (`docs/specs/137-plan-snap-overlay.md`) + за пределами того, что понадобилось для подтверждения текущей + функциональности snap-коллектора (endpoint/line-snap без face-построения); + само #137 не входит в предмет этого ревью. +- Не оценивал, достаточно ли 17 AC с точки зрения «можно ли перепутать + порядок исполнения» на этапе кода — это будет предметом code review, когда + появится реализация и её тесты; на этапе ТЗ проверялось только то, что + каждый AC однозначен и называет способ доказательства. + +## Вердикт + +Зелёный. High: 0, Medium: 0. Три находки Low (отсутствие отдельного +заголовка «Проблема»; дрейф терминологии «Walls» в текущем тексте +`UX-MODES.md`, не внесённый этим ТЗ, но подлежащий исправлению в рамках +release-артефактов §17; опечатка-апостроф в AC17) — ни одна не блокирует +приёмку, все либо правятся косметически при следующей редакции, либо +снимаются этой записью без нового цикла.