diff --git a/docs/reviews/SPEC-REVIEW-193-r1.md b/docs/reviews/SPEC-REVIEW-193-r1.md new file mode 100644 index 00000000..0edbedf7 --- /dev/null +++ b/docs/reviews/SPEC-REVIEW-193-r1.md @@ -0,0 +1,266 @@ +# SPEC-REVIEW-193-r1 + +- **Issue:** https://github.com/Matysh/houseplan-card/issues/193 +- **ТЗ под ревью:** [`docs/specs/193-passage-placement-preview.md`](https://github.com/Matysh/houseplan-card/blob/issue/193-passage-preview/docs/specs/193-passage-placement-preview.md) + (коммит `fc22d9a`), обычный трек — не `small`/`trivial` +- **Роль:** ревьюер ТЗ (не автор), этап `S4-spec-review` +- **Трек:** обычный, лимит циклов ревью ТЗ — 4 (§4 PROCESS.md) +- **Цикл:** r1/4 + +## Скоуп ревью + +ТЗ #193: инструмент «Открытый проём» в Plan editor получает специальную +placement-preview геометрию только для `candidate.type === 'passage'` — +полупрозрачный сегмент будущего разреза стены и две поперечные засечки +границ, поверх уже существующих точки привязки и линеек. Сохранённый passage, +door/window/gate preview, конфиг, backend, i18n, миграция и touch-контракт не +меняются по тексту ТЗ. + +Не в скоупе ревью: продуктовый код. Ветка `issue/193-passage-preview` +существует и содержит один коммит (`fc22d9a6c55b085dc273a70bdc8e2db22ec9fe19`, +`Issue: #193 · User-Visible: no`) — только спецификация, реализации нет. +Гейты (`typecheck`/`test`/`build`) не прогонялись: на этапе ревью ТЗ +продуктового кода не существует, прогон не относится к этому этапу +(PROCESS.md §2.4/§8). + +## Как проверялось + +1. Прочитаны `docs/SCOPE.md`, `AGENTS.md`, `PROCESS.md` целиком (действующая + редакция, включая §1, §2.4, §2.5, §5, §7.1, §8, §10.2). +2. Прочитано тело issue #193 и оба комментария: аналитика владельца + (`Matysh`, `OWNER`, оценка 6/10 · 3/10 · 3/10 · P2 · `enhancement` · + обычный трек, явное обоснование, почему `small`/`trivial` не подходят — + новый UX-контракт preview) и хендофф «ТЗ готово к ревью». +3. Построчно сверены все технические утверждения раздела «Подтверждённая + техническая база» (§4 ТЗ) с кодом на этой ветке: + - `_renderOpeningPlacementPreview()` — `src/houseplan-card.ts:17350` + (заявлено 17351, расхождение на строку из-за doc-комментария, не + дефект); вызывает `renderOpeningVisibleGeometry(visibleSpec)` без + ветвления по типу — подтверждает диагноз «инструмент слеп для passage». + - `renderOpeningVisibleGeometry()` (`src/render/opening-symbol.ts:51-55`): + `if (spec.type === 'passage') return svg\`\`;` — подтверждено дословно, + включая обоснование в комментарии кода («negative space», «no jamb, leaf, + arc, gate panel or standalone frame»). + - `OpeningPlacementCore` (`src/opening-placement.ts:33-48`) содержит ровно + заявленные поля: `x`, `y`, `angle`, `renderedLength`, `target` + (`OpeningPlacementTarget.physicalHalfWidth`, строка 17), `measure`, + `flipH`/`flipV`, `type` — ТЗ не выдумало ни одного поля. + - `openingPlacementTargets()` (строки 109-140): для совпадающих + `segmentKey` берёт `Math.max(previous.physicalHalfWidth, interval.half)` + — подтверждает точную формулировку ТЗ «для совпадающих room-owned копий + берётся максимальная реальная половина толщины». + - `_opMeasureView` — `src/houseplan-card.ts:11895` (заявлено 11896, тот же + класс погрешности) возвращает `OpMeasure`, используется на 15384 — + подтверждено, линейки действительно не требуют изменений. + - Порядок SVG (`src/houseplan-card.ts:15690-15733`): `_renderWallBodies()` + на строке 15723, `_renderOpeningPlacementPreview()` на 15727 — preview + реально рисуется после тел стен и будет виден поверх кладки без нового + слоя, как заявлено. + - `.opening-preview` (группа, `houseplan-card.ts:17366-17368`) уже несёт + `aria-hidden="true" pointer-events="none"`; `.opening-preview-dot` + (17371-17372) — то же. Оба факта подтверждены буквально. + - Дочерний `` точки использует `r=${this._gridPitch * 0.18}` + (17372) — подтверждает §16 п.3 ТЗ: выступ засечек, предложенный тем же + коэффициентом, действительно равен радиусу существующей preview-точки, + а не выдуман. + - Golden harness (`demo/golden/harness.mjs:450`) — allowlist + `['window', 'door', 'gate']`, требует `.op-leaf` (строка 474) — + подтверждает точный диагноз «harness валидирует только типы с + `.op-leaf` и не принимает passage». + - Golden matrix (`demo/golden/matrix.mjs:100-106`) — существует ровно один + сценарий `opening-placement-door-thick-wall-dark` (без light-варианта); + ТЗ корректно называет его существующим прецедентом и не завышает его + охват. + - CSS `.opening-preview { opacity: 0.5; }` (`src/styles.ts:1372-1375`) — + подтверждает реальность риска «opacity применяется дважды» (§13 ТЗ): + новый сегмент рисуется внутри той же группы, и наивная реализация с + `opacity: 0.35` на самом rect действительно даст `0.5 × 0.35 = 0.175`, + как явно предупреждает ТЗ. Риск назван точно, а не гипотетически. + - `demo/smoke_opening_preview.mjs:152` — `out.saveMatchesResolver` уже + существует под этим именем; AC2 ссылается на реальный, а не придуманный + идентификатор. + - `test/opening-symbol.test.mjs` существует (`ls test/`), AC3 ссылается на + реальный файл. +4. Прочитан `docs/USER-GUIDE.ru.md` (разделы 1 «Термины», 9 «Двери, окна, + открытые проёмы, ворота и замки»): терминология ТЗ («Открытый проём», + «полупрозрачный символ», «толстая стена», ширина по умолчанию 90 см) + совпадает с существующим текстом (строки 428-465), включая уже описанное + поведение door/window/gate preview («Полупрозрачный символ показывает + точный будущий вид... виден поверх тела толстой стены», строки 437-438) — + ТЗ не изобретает интерфейсную лексику. +5. Прочитан `docs/WALL-THICKNESS.md` целиком — модель `half`/толщины, + growth ±½, masonry cuts согласуются с использованием + `target.physicalHalfWidth` в ТЗ; прямого противоречия канону нет. +6. Прочитан `docs/TOUCH-SUPPORT.md` целиком, включая «Documentation rule» + (строки 142-154): требование буквальной строки `Touch editor: …` для + «New editor feature specifications». В тексте ТЗ #193 такой строки нет + ни в одном разделе (проверено `grep -n touch` по файлу — единственное + упоминание, строка 123 Non-scope: «расширение touch-гарантий Plan editor») + — см. находку Low-1. +7. Проверены прямые ссылки ТЗ на файлы демо/тестов — + `demo/smoke_opening_preview.mjs`, `demo/golden/matrix.mjs`, + `demo/golden/harness.mjs`, `test/opening-symbol.test.mjs` — все существуют + и содержат ровно те механизмы, на которые ссылается ТЗ (см. п.3 выше). +8. Проверено соответствие `docs/SCOPE.md`: задача — геометрия Plan editor + (J4 «zero to a working plan... room polygons» / J6 «keep the plan true as + the home evolves», обе персона home admin, поверхность — desktop Plan + editor), View/kiosk/backend не затрагиваются; попадает в допустимый скоуп, + не создаёт нового job'а. Родитель #157 (сам open passage) уже принят и + закрыт как реализованный J4/J6-функционал — #193 закрывает его собственный + UX-пробел, не расширяя скоуп. +9. Проверено, что issue не помечен `small`/`trivial`: аналитика владельца + явно называет причину (новый UX-контракт preview) — критерий §5 PROCESS.md + «нет нового UX-контракта» не выполняется, обычный трек обоснован корректно. + +Гейты (`typecheck`/`test`/`build`, browser smoke, golden) не прогонялись — +продуктового кода нет, что и ожидается на этапе ревью ТЗ. + +## Обязательные разделы (§7.1 PROCESS.md) + +| Раздел | Есть | Комментарий | +|---|---|---| +| Сценарий (персона/поверхность/момент) | ✅ | §1: home admin, desktop Plan editor, наведение инструментом до клика | +| Что человек увидит до/после | ✅ | §2: одна фраза до/после, без терминов реализации | +| Проблема | ✅ | §3, подтверждена чтением кода (см. «Как проверялось» п.3) | +| Скоуп / не-скоуп | ✅ | §5/§6, скоуп ограничен геометрией+визуалом+тестами, не-скоуп явно исключает drag, символ passage, i18n, touch-расширение | +| Контракт поведения | ✅ | §7 (появление, hover/клик, после сохранения, остальные типы, события/доступность) | +| UX | ✅ | §8, таблица состояний, 7 строк | +| Модель данных и миграция | ✅ | §9, «не меняется», обоснованно для editor-only transient preview | +| i18n | ✅ | §10, «новых строк нет» | +| AC1…ACn с доказательством | ✅ | §11, 6 штук, у каждого назван способ доказательства (unit/smoke/golden/source contract) | +| План автотестов | ✅ | §12, unit/smoke/golden по отдельности, falsifiability явно потребована («на `origin/dev` до реализации обязаны краснеть») | +| Риски | ✅ | §13, 8 строк риск/мера, включая подтверждённый двойной-opacity риск | +| Откат | ✅ | §14, удаление ветки/helper/стилей/тестов, без миграции | +| Release-артефакты | ✅ | §15, оба changelog, RU/EN guide, TESTING.md, dist-синхронизация, golden candidates отложены на release runbook | + +Присутствует также обязательный блок «Принято предположительно» (§16, 5 +пунктов) — технические, не продуктовые решения, корректно отделены от +продуктовых вопросов, которых, по утверждению автора и аналитики, для этой +задачи нет. Проверка не нашла в §16 замаскированного продуктового вопроса: +все пять пунктов (расположение чистого helper'а, имена классов, коэффициент +выступа засечек, общая golden-fixture, поведение при нулевой толщине) +касаются исключительно того, чего пользователь не наблюдает или что уже +предрешено принципом «не выдумывать несуществующую толщину». + +## Находки + +### Low-1 — отсутствует обязательная декларация `Touch editor: …` + +**Файл:** `docs/specs/193-passage-placement-preview.md`, весь документ (нет +подходящего раздела); ближайшее место — §6 Non-scope, строка 123. + +`docs/TOUCH-SUPPORT.md`, раздел «Documentation rule» (строки 142-154): +«New editor feature specifications and code reviews must state one of: +`Touch editor: supported`; `Touch editor: best effort / intentionally +degraded`; `Touch editor: not exposed`.» ТЗ #193 добавляет новую видимую +geometry в Plan editor (`hover`-preview) — то есть однозначно подпадает под +«new editor feature specification». Полнотекстовый поиск `touch` по файлу +даёт единственное совпадение (строка 123 Non-scope: «расширение +touch-гарантий Plan editor»), которое по смыслу подразумевает ответ (не +расширяем существующий best-effort статус), но не содержит требуемой +буквальной строки. Тот же класс проверки в `SPEC-REVIEW-192-r1` (issue #192) +нашёл строку `**Touch editor:** picker поддерживается как в #57` и закрыл +вопрос без находки — здесь эквивалентной строки просто нет. + +**Обоснование severity:** ответ не является продуктовой неопределённостью — +Plan editor по умолчанию desktop-first/best-effort (`docs/TOUCH-SUPPORT.md`, +таблица «Product contract»), preview — чисто presentation, hover-based +эффект, на устройстве без реального hover просто не активируется тем же +путём, что и сейчас для door/window/gate; отдельного решения владельца не +требуется. Дефект — отсутствие обязательной для канона декларации, не +дефект поведения. AC1…AC6 не зависят от этой строки. + +**Решение ревьюера:** Low, не блокирует. Снимаю без возврата ТЗ на правку с +условием: автор добавляет в ТЗ (в §6 Non-scope или отдельной строкой рядом +с §7.5) явную декларацию `**Touch editor:** best effort / intentionally +degraded (наследует существующий статус Plan editor; preview — presentation-only +hover-эффект, не расширяет touch-гарантии)` до перевода issue в +`S5-ready` — это чисто редакционное дополнение, не требующее нового цикла +ревью ТЗ, и должно быть зафиксировано в хендоффе реализации или проверено +на код-ревью, если автор пропустит этот шаг. + +## Что проверено и корректно + +- **Соответствие `docs/SCOPE.md`:** J4/J6, персона home admin, desktop Plan + editor; View/kiosk/backend не затронуты; #193 закрывает собственный + UX-пробел уже принятого #157, не расширяя его скоуп. +- **Легитимность обычного трека:** аналитика владельца верно называет + причину, по которой `small`/`trivial` не подходят (новый UX-контракт + preview) — не самоощущение исполнителя, а проверяемый критерий §5 + PROCESS.md. +- **Продуктовых вопросов владельцу нет и не додумано новых.** Единственный + потенциально продуктовый вопрос — объём видимого эффекта (сегмент + две + засечки, без нового цвета/настройки) — уже зафиксирован самим текстом + issue (владелец); ревью не нашло скрытой догадки, выданной за факт: каждое + утверждение §4 «Подтверждённая техническая база» проверено построчно по + реальному коду ветки (см. «Как проверялось» п.3) и подтвердилось без + исключений. +- **Технический диагноз проблемы не голословен** — `renderOpeningVisibleGeometry` + дословно возвращает пустой SVG для `passage`, `_renderOpeningPlacementPreview` + дословно не ветвится по типу; диагноз «слепой инструмент» доказан кодом. +- **Скоуп реально согласован с существующим pipeline:** `OpeningPlacementCore` + уже содержит все поля, на которые ссылается ТЗ (`x`, `y`, `angle`, + `renderedLength`, `target.physicalHalfWidth`, `measure`) — реализация не + потребует нового вычисления геометрии, только новую ветку рендера, как и + заявлено. +- **Риск двойного `opacity` реален, а не гипотетичен** — `.opening-preview` + действительно несёт CSS `opacity: 0.5` (`styles.ts:1372`); ТЗ верно + определило нужную меру (отдельная стилевая область + computed-value + проверка в AC6) до того, как эта ошибка попала бы в реализацию. +- **AC1–AC6 однозначны и проверяемы**, каждый снабжён допустимым по §2.5 + PROCESS.md способом доказательства (unit/smoke/golden/source contract); + план автотестов (§12) явно требует falsifiability до реализации — + «unit геометрии и smoke DOM обязаны краснеть на `origin/dev`». +- **Другие типы openings защищены negative-контрактом** (AC4) и + существующим door-golden, который явно не меняется — предотвращает + случайную регрессию window/door/gate preview. +- **Существующие файлы и идентификаторы, на которые ссылается ТЗ, реальны** + — `demo/smoke_opening_preview.mjs` (`saveMatchesResolver`, строка 152), + `test/opening-symbol.test.mjs`, `demo/golden/harness.mjs` (allowlist + `['window','door','gate']`, строка 450), `demo/golden/matrix.mjs` + (`opening-placement-door-thick-wall-dark`, строка 100) — ни одна ссылка не + выдумана. +- **Откат тривиален** — preview-only ветка/helper/стили/тесты удаляются без + миграции данных; #157 продолжает работать без деградации. +- **Release-артефакты** ссылаются на существующие файлы (`docs/CHANGELOG.md`, + `docs/CHANGELOG.ru.md`, `docs/USER-GUIDE.ru.md`, `docs/TESTING.md`) и + корректно откладывают golden baseline acceptance на release runbook, а не + выдают его за часть implementation loop. +- **«Принято предположительно» (§16)** не маскирует продуктовый вопрос под + техническое решение — все пять пунктов однозначно технические. + +## Чего не проверял + +- Реализацию — её нет: ветка содержит один документный коммит + (`fc22d9a`, `docs/specs/193-passage-placement-preview.md`), продуктовый код + (`src/**`) не менялся — проверено `git show fc22d9a --stat`. +- Гейты `typecheck`/`test`/`build`/browser smoke/golden — не относятся к + этапу ревью ТЗ; предмет будущего код-ревью (PROCESS.md §2.4/§8). +- `python -m pytest tests_backend` — задача не затрагивает backend ни кодом, + ни ТЗ. +- Реальный визуальный результат (точные пиксельные пороги AC6, computed + `fill-opacity` в браузере) — CSS/рендер ещё не написаны; заявленные меры + (отдельная стилевая область, semantic pixel guard внутри `.wallbody-fill`) + зафиксированы в ТЗ как план, не как исполненный факт. +- Точность числовых оценок аналитики (ценность 6/10, сложность 3/10, P2) по + существу — поле владельца (PROCESS.md §2.2), уже принятое явным решением + до написания ТЗ. +- Возможность технического спора автор/ревьюер по деталям §16 — не + возникла: все пять пунктов приняты как разумные без необходимости + оспаривать. + +## Вердикт + +Зелёный. High: 0, Medium: 0, Low: 1 (отсутствует буквальная декларация +`Touch editor: …`, обязательная по `docs/TOUCH-SUPPORT.md` для новых +editor-фич; ответ однозначен — best effort, наследуется от существующего +статуса Plan editor — снимается без возврата ТЗ на правку, с условием +дописать строку до `S5-ready`). ТЗ подтверждено построчным чтением кода на +ветке задачи: каждое техническое утверждение §4 оказалось фактом, а не +догадкой; единственный потенциально продуктовый вопрос уже закрыт +владельцем в теле issue; AC1–AC6 однозначны, проверяемы и снабжены +допустимыми способами доказательства; риски названы точно, включая реально +существующий риск двойного `opacity`. + +**Вердикт: зелёный · цикл r1/4 · High: 0 · Medium: 0 → нет · Документ: +docs/reviews/SPEC-REVIEW-193-r1.md**