From 6c668d4bfef5c6207b65fd957bc048e5586c426b Mon Sep 17 00:00:00 2001 From: "claude[bot]" <209825114+claude[bot]@users.noreply.github.com> Date: Fri, 4 Sep 2026 05:39:54 +0000 Subject: [PATCH] docs: review document for #447 Issue: #447 User-Visible: no --- docs/reviews/SPEC-REVIEW-447-r1.md | 253 +++++++++++++++++++++++++++++ 1 file changed, 253 insertions(+) create mode 100644 docs/reviews/SPEC-REVIEW-447-r1.md diff --git a/docs/reviews/SPEC-REVIEW-447-r1.md b/docs/reviews/SPEC-REVIEW-447-r1.md new file mode 100644 index 00000000..72b560c5 --- /dev/null +++ b/docs/reviews/SPEC-REVIEW-447-r1.md @@ -0,0 +1,253 @@ +# SPEC-REVIEW-447-r1 + +- Issue: https://github.com/Matysh/houseplan-card/issues/447 +- Этап: `spec` (PROCESS.md §2.4), полный трек (метка `S4-spec-review`, без `small`) +- Материал: `docs/specs/447-exterior-furniture-snap-keyboard-nudge.md` на коммите + [`a1fbea07`](https://github.com/Matysh/houseplan-card/commit/a1fbea07d2876d51afac3a9df12bfa756424df99) + (docs-only, класс C: `docs/specs/447-...md` + `docs/specs/README.md`, `git show --stat a1fbea07`) +- Заход: r1 · блокирующих циклов до этого раунда: 0/4 + +## Скоуп ревью + +ТЗ описывает две независимые части одной issue: + +1. наружная физическая поверхность для внешней стены (`roomFurnitureWallSurfaces`, + `snapFurnitureToWall` в `src/furniture-wall-surface.ts` / `src/furniture-placement.ts`); +2. Arrow-сдвиг выбранного объекта декора на одну ячейку сетки в Редакторе подложки + (`_keyHandler`, `_decorSel`, `_decorList` в `src/houseplan-card.ts` / + `src/houseplan-editor-runtime.ts`). + +Продуктовых вопросов к владельцу нет: тело issue и оба комментария владельца +(04.09.2026) уже закрывают Shift-шаг, подложку плана, точную величину шага и связь +с #41. Проверялась не полнота согласования, а исполнимость и однозначность +контракта и AC. + +## Как проверялось + +- Прочитаны `docs/SCOPE.md`, `AGENTS.md`, `PROCESS.md` (§1–§10), тело issue #447 и + все три комментария, ТЗ целиком. +- Прочитан код, который ТЗ описывает как текущее поведение и на который опирается + контракт: `src/furniture-wall-surface.ts`, `src/furniture-placement.ts`, фрагмент + `_keyHandler` вокруг `src/houseplan-card.ts:3018`, `_decorMoveUpdate` + (`src/houseplan-editor-runtime.ts:4336-4376`), `_gridPitch`/`_cellCm` + (`src/houseplan-card.ts:7336-7348`), константы `GRID_PITCH`/`GRID_STEP_N`/`NORM_W` + (`src/space-geometry.ts:12,212-216`), `segmentCm` (`src/logic.ts:63-66`), + `decorCmToUnits`/`decorUnitsToCm` (`src/editors/decor/geometry.ts:76-114`). +- Сверены утверждения ТЗ о текущем баге (`sideScore` как тай-брейк, а не фильтр) с + реальным кодом `snapFurnitureToWall` — подтверждены построчно. +- Прочитаны `docs/CANVAS.md` §9 (grid/snap contract), `docs/FURNITURE.md`, + `docs/USER-GUIDE.ru.md` (разделы «14. Редактор подложки», термин «Декор», + таблица инструмента «Выбрать») на предмет терминологии и уже + зафиксированного поведения. +- Сверена связь с #41 — прочитан текст issue и `docs/specs/041-keyboard-object-editing.md` + (`origin/dev`): формулировка «arrows move = один grid node» в #41 не называет + конкретную единицу измерения, так что #447 не наследует оттуда готовое решение + предмета находки ниже, это самостоятельная неточность #447. +- Проверено существование файлов, на которые ссылается план тестирования и карта + реализации: `docs/FURNITURE.md`, `docs/DECOR-EDITOR.md`, `demo/smoke_furniture.mjs`, + `demo/smoke_decor*.mjs` — все существуют. +- Проверены `DecorKind`/`DecorShape` (`src/editors/decor/types.ts:8-88`) — ровно + шесть видов `line | rect | ellipse | text | furniture | image`, как и в ТЗ. + +### Гейты + +Диапазон изменений на этом SHA — docs-only (класс C по AGENTS.md: только +`docs/specs/**` и `docs/specs/README.md`), `src/**` не тронут. Условия, +включающие `typecheck`/`test`/`build`/`check-docs`/`model-invariants`/смоки +(«если менялся `src/**`», «если менялась геометрия»), не выполнены — ни один из +них не применим к этому диффу, гонять их бессмысленно: они бы просто +подтвердили, что `dev`-код (не тронутый) уже проходит эти гейты, и ничего не +скажут о качестве самого ТЗ. Явно не прогонялись: `npx tsc --noEmit`, `npm test`, +`npm run build`, `node scripts/check-docs.mjs`, `node scripts/model-invariants.mjs`, +любые `demo/smoke_*.mjs`, `npm run golden:verify`, `pytest tests_backend` — все по +причине «diff их не касается», не «пропущено». + +## Находки + +### H1 — AC4 и §3 путают единицы «шага сетки»: render-space `gridPitch` vs normalised `GRID_STEP_N`, что при буквальной реализации ломает саму суть Part 2 + +**Файл:** `docs/specs/447-exterior-furniture-snap-keyboard-nudge.md`, разделы +«3. Сдвиг выбранного декора стрелками» (таблица «Дельта render-координат») и +«AC4. Каждый вид выбранного декора двигается на одну клетку», плюс комментарий +владельца от 04.09.2026 («В коде это ровно один `gridPitch`, и никакого деления +на `cell_cm` не нужно»). + +**В чём проблема.** ТЗ и комментарий владельца называют величину шага буквальным +именем кодовой константы `gridPitch` и требуют применять её к «позиционным +полям» decor-объекта (`x`, `y`, `x1`, `y1`, `x2`, `y2` — то, что реально лежит в +`space.decor[]`). Но в текущем коде это два **разных** масштаба, и это не +случайность, а осознанная граница, задокументированная в самом коде: + +- `GRID_PITCH = NORM_W / GRID_N = 1000 / 240 ≈ 4.1667` — шаг сетки в + **render-единицах** (`src/space-geometry.ts:212-214`, geometry в масштабе + холста 0…1000). Используется там, где считают на уровне пикселей/SVG: + `space-render.ts`, `snapR()`, `_gridPitch` getter в `houseplan-card.ts:7336-7342` + («NORM_W / GRID_N и ничего больше»). +- `GRID_STEP_N = 1 / GRID_N ≈ 0.0041667` — тот же шаг в **нормализованных + единицах** (`src/space-geometry.ts:216`, комментарий «what the config and the + layout store»). Именно в этом масштабе хранятся `x`/`y` room-полигонов, стен, + ключей и — что здесь решает — decor: `houseplan-card.ts:9061` прямым текстом + «Keys are always stored in normalised space (`GRID_STEP_N`)». + +Что фактически хранится в `space.decor[].x/y` подтверждается кодом уже +существующего mouse-drag: `_decorMoveUpdate` +(`src/houseplan-editor-runtime.ts:4336-4376`) считает якорь в render-единицах, а +затем **делит на `NORM_W`** (для x) или на `this.host._decorH` (для y), прежде +чем записать `x: o.x + dx` — то есть `dx`, попадающий в конфиг, на три порядка +меньше `GRID_PITCH` (`dx = (anchor[0] - ax0) / NORM_W`, `NORM_W = 1000`, +`src/space-geometry.ts:12`). + +Спецификация же формулирует AC4 буквально как «меняют… позиционные поля на +`gridPitch`» без единого слова о делении на `NORM_W`/`_decorH` или о разнице +между координатой в момент пересчёта (render) и координатой на диске +(normalised). Комментарий владельца ещё и explicit отрицает необходимость +какого-либо деления («никакого деления на `cell_cm` не нужно») — верно для +`cell_cm`, но по факту скрывает необходимое деление на `NORM_W`, о котором ни в +ТЗ, ни в комментарии не сказано ни слова. + +**Почему это блокирует, а не техническая мелочь на усмотрение реализации.** +Раздел «Принятые предположения» разрешает менять «техническую раскладку pure +helper», но не сам числовой контракт — а именно числовой контракт здесь и +сформулирован неоднозначно/неверно в AC. Если разработчик реализует AC4 так, как +она написана — прибавит экспортированную `GRID_PITCH` (≈4.1667) напрямую к +нормализованному `x` (обычно в диапазоне порядка 0…2–3 для видимого плана) — то +одно нажатие стрелки сдвинет объект на ~4 ширины холста. Это даже не упрётся в +`clampCanvasN`/`CANVAS_LIMIT = 5000` (`src/space-geometry.ts:205,240-241`) — +предмет просто окажется далеко за пределами видимой области без единой ошибки в +консоли, то есть эффект прямо противоположен заявленному «сдвиг ровно на одну +клетку сетки, а не число, которое надо помнить». Хуже: если модульный тест по +AC4 будет (как и написано в тексте AC) утверждать «поле изменилось на +`GRID_PITCH`», такой тест зафиксирует именно неверную величину и молча пройдёт — +ровно тот сценарий, о котором предупреждает сам процесс ревью («тест, +спрашивавший то, чего не проверял», #423) и принцип «одно число — один +источник»: видимая величина шага (то, что рисует сетку на экране, в +render-масштабе) и сохранённая величина (то, что пишется в конфиг, в +normalised-масштабе) обязаны быть одним и тем же физическим шагом, посчитанным +через одно преобразование, а не два независимых числа с одинаковым именем в +тексте ТЗ. + +**Как воспроизвести рассуждение.** Взять любой существующий decor-объект, +например `x = 0.4` (типичное значение для видимого плана). Применить +буквально AC4: `x -= gridPitch` → `x = 0.4 - 4.1667 = -3.7667`. Сравнить с +ожидаемым «на одну клетку сетки» — при `cell_cm = 5` перемещение на одну клетку +означает сдвиг на 5 см реального плана, что в normalised-единицах равно +`GRID_STEP_N ≈ 0.0041667`, а не `4.1667`. Разница — ровно `NORM_W` (1000×). + +**Что нужно исправить.** Явно указать в контракте (§3 и AC4), что дельта +считается в render-координатах (как и озаглавлена таблица), а на «позиционные +поля» decor записывается **после** того же преобразования, которым уже +пользуется `_decorMoveUpdate` — деление на `NORM_W` (для x/x1/x2) и на +высоту холста по Y (для y/y1/y2), либо прямо назвать нормализованный шаг +`GRID_STEP_N` как то, что фактически прибавляется к `x`/`y`. Это техническое по +природе уточнение (никакая видимая пользователю величина не меняется — «одна +клетка» остаётся «одной клеткой» в сантиметрах), поэтому не требует вопроса +владельцу и решается прямо в тексте ТЗ. + +**Категория:** correctness / однозначность AC. **Серьёзность: High** — как +написано, AC4 не исполним корректно; часть 2 задачи (клавиатурная доводка) +буквально по тексту ломается на первом же нажатии стрелки. + +Других High или Medium в скоупе задачи находок нет. + +## Что проверено и корректно + +- **Диагноз части 1 подтверждён построчно.** `roomFurnitureWallSurfaces` + (`src/furniture-wall-surface.ts:41-82`) действительно строит только одну + грань на атом — `axis + inwardNormal * half` — и не создаёт наружного + кандидата; `normal` явно документирован как «points from masonry into the + owning room» (строка 14). В `snapFurnitureToWall` + (`src/furniture-placement.ts:148-153`) `sideScore` участвует в выборе только + при `Math.abs(candidate.dist - best.dist) <= tieEps`, первичный критерий — + `candidate.dist < best.dist - tieEps`; при единственном кандидате в радиусе + сторона не проверяется вовсе. Оба факта, на которых стоит вся первая часть + ТЗ, воспроизведены чтением, а не с чужих слов. +- **Контракт части 1 закрывает найденный баг по существу, а не косметически.** + Требование «предмет не переносится через ось на сторону, противоположную + точке намерения» — это явный перевод `sideScore` из тай-брейка в фильтр (один + из двух вариантов, которые сама issue считает приемлемыми), а не просто + добавление наружного кандидата поверх старой логики отбора по `dist` — то + есть ТЗ не оставляет прежний баг между старой внутренней и новой внешней + поверхностью. +- **Инварианты общей/нулевой/independent стены названы явно и с AC/mutation** + (AC2), включая нечувствительность к порядку комнат и winding — учтён риск, + под который заведён отдельный пункт в «Рисках». +- **Виды decor согласованы с типами.** `DecorKind` (`src/editors/decor/types.ts:10`) + — ровно `line | text | rect | ellipse | furniture | image`; ТЗ перечисляет + все шесть и не добавляет несуществующих. +- **Guard-условия Arrow (§3, AC8) переиспользуют уже существующий паттерн.** + Текущий `_keyHandler` (`src/houseplan-card.ts:2978-3060`) уже отличает + `inField` (composedPath по `input, textarea, select, [contenteditable]`) и + `inEditorSecondary` (класс `editor-secondary`) для Delete/Backspace в этом же + режиме — ТЗ требует того же для Arrow, что не вводит новый непроверенный + guard-паттерн. +- **Раздельные пути истории названы верно.** `history.decor_move` + (`src/i18n/ru.json:124`, использование — `houseplan-card.ts:7018`) — + существующий ключ, переиспользуемый и mouse-drag, и клавиатурой; новых + i18n-строк ТЗ не требует. +- **Обязательные разделы §7.1 все присутствуют**: сценарий, что человек увидит + до/после, проблема, скоуп/не-скоуп, контракт поведения, UX, данные/миграция/i18n, + производительность, крайние случаи, AC1…AC9 с методом доказательства, план + автотестов, риски, откат, release-артефакты, принятые предположения. Каждый + AC называет способ доказательства (`unit`/`smoke`/`review`). +- **Трек и лимит согласованы.** Full track обоснован (сложность/риск >3, два + независимых поведенческих контракта, новый клавиатурный UX-контракт) — + критерии `small` (PROCESS.md §5) действительно не выполнены, названо явно в + аналитике; лимит ревью ТЗ для полного трека — 4 цикла, что соответствует + «блокирующих циклов 0/4» на входе. +- **Терминология сверена с USER-GUIDE.ru.md.** «Редактор подложки» — верное имя + всего режима (раздел 14, таблица «Декор | Элемент подложки»), не только + картинки плана; путаницы между «Редактор подложки» и «декор» в ТЗ нет. +- **Rollback и release-артефакты адекватны масштабу**: чистый revert + frontend/tests/docs/bundle, нет схемы/миграции для отката; changelog RU+EN, + `docs/FURNITURE.md`, `docs/DECOR-EDITOR.md`, screenshot fingerprint через + `src/**` — корректный список для класса A правки такого объёма. + +## Чего не проверял + +- Не прогонялись `typecheck`/`test`/`build`/`check-docs`/`model-invariants`/смоки + — диапазон изменений на этом SHA docs-only и их не касается (см. «Гейты» + выше); они станут релевантны на код-ревью, когда появится диапазон + `origin/dev..HEAD` с изменениями в `src/**`. +- Не проверял, что owner-группировка атома («ровно одной комнате») реализуема + за один проход без учёта направления рёбер и winding — это заявлено как + требование к реализации (AC1/AC2 + permutation/winding tests), а не как + готовый алгоритм в самом ТЗ, поэтому это предмет код-ревью, а не спек-ревью. +- Не проверял `docs/DECOR-EDITOR.md` и `docs/FURNITURE.md` построчно на предмет + того, что именно там придётся дописать — только подтвердил, что оба файла + существуют и являются подходящим местом для новых абзацев (release-артефакты + ТЗ их и называют). +- Не оценивал производительность фактическим профилированием — раздел + «Производительность» ТЗ содержит качественное, проверяемое утверждение (линейный + рост числа кандидатов, отсутствие новых imports в View graph), количественная + проверка (`bundle:budget`) относится к код-ревью. + +## Вердикт + +ТЗ полное, обязательные разделы на месте, диагноз части 1 подтверждён чтением +кода, риски и инварианты части 1 закрыты явно. Но AC4 (и опирающийся на неё +раздел 3) содержит невыполнимое буквально требование из-за смешения +render-масштаба (`gridPitch` ≈4.1667) и normalised-масштаба (`GRID_STEP_N` +≈0.0041667) constant, в котором фактически хранятся decor-координаты — при +реализации «как написано» вторая часть задачи (клавиатурная доводка) ломается +на первом нажатии стрелки, и это ровно то расхождение, которое несколько раз +уже стоило продукту дефекта («одно число — один источник»). Находка технической +природы, снимается автором правкой текста ТЗ без обращения к владельцу. + +**Вердикт: красный · заход r1 · блокирующих циклов 1/4 · High: 1 · Medium: 0 → +в задаче** + +--- + + + +## Материал раунда + +- Ветка: `issue/447-exterior-snap-keyboard`, коммит `a1fbea07d287` — ребейз его осиротит, и это нормально: ниже якоря, которые ребейз не меняет. +- Дерево материала: `dd1a85b41c1caba79488df331dd813f5b7522dd0` + ``` + git log --all --format='%H %T' | grep dd1a85b41c1c + ``` +- ТЗ `docs/specs/447-exterior-furniture-snap-keyboard-nudge.md`, блоб `5003f5995be9dd06e296a8c0056d63c1d21a8fc6` + ``` + git log --all --find-object=5003f5995be9dd06e296a8c0056d63c1d21a8fc6 -- docs/specs/447-exterior-furniture-snap-keyboard-nudge.md + ```