mirror of
https://github.com/Matysh/houseplan-card
synced 2026-09-29 03:09:36 +00:00
claude[bot]
@@ -0,0 +1,210 @@
|
||||
# SPEC-REVIEW-199-r1 — Optimize geometry preflight
|
||||
|
||||
- **Issue:** https://github.com/Matysh/houseplan-card/issues/199
|
||||
- **ТЗ:** `docs/specs/199-optimize-geometry-preflight.md`, коммит `0ac63b6`
|
||||
на ветке `issue/199-optimize-geometry-preflight`
|
||||
(родитель `c397b78` → `4dbdb44` → `dev@0d91c1e` на момент ревью).
|
||||
- **Трек:** обычный (issue не помечен `small`), файл ТЗ обязателен и создан.
|
||||
- **Заход:** r1 (первый цикл этого этапа для #199).
|
||||
- **Вердикт:** см. итог ниже.
|
||||
|
||||
## Скоуп ревью
|
||||
|
||||
Разобрано ТЗ целиком: сценарий, продуктовые решения владельца (Q1–Q3 в
|
||||
комментариях issue), контракт preflight (§7), UX-контракт (§8), backend/
|
||||
atomicity (§9), производительность (§10), AC1–AC14 (§11), план тестов (§12),
|
||||
риски (§13), touch (§14), release-артефакты и rollback (§15), блок принятых
|
||||
технических предположений (§16).
|
||||
|
||||
Ревью строго по PROCESS.md §2.4: ищу, где ТЗ невыполнимо или непроверяемо, а
|
||||
не соглашаюсь с автором. Устных пояснений автора не было — только issue,
|
||||
комментарии и файл ТЗ.
|
||||
|
||||
## Как проверялось
|
||||
|
||||
1. Прочитаны `docs/SCOPE.md`, `AGENTS.md`, `PROCESS.md` целиком.
|
||||
2. Прочитано тело issue #199 и все 6 комментариев (аналитика, продуктовые
|
||||
вопросы Q1–Q3, решения владельца, хендофф автора ТЗ).
|
||||
3. Прочитан файл ТЗ целиком (419 строк).
|
||||
4. Каждая техническая ссылка ТЗ на существующий код проверена по `origin/dev`
|
||||
(не по слухам автора):
|
||||
- все перечисленные символы (`_openAlignDialog`, `optimizePlans`,
|
||||
`wallBodiesGeometry`, `floorFootprintGeometry`, `_runAlignToGrid`,
|
||||
`_renderAlignDialog`, `contentFingerprint`, `resolveOpenCuts`,
|
||||
`wallIntervals`, `partitionOpeningHasCompositeRoomWall`,
|
||||
`physicalBodyParts`, `spaceModels`, `NORM_W`, `GRID_STEP_N`,
|
||||
`GRID_PITCH`, `gs.align_none`, `gs.align_done`) найдены `git grep` в
|
||||
заявленных файлах;
|
||||
- прочитан код `_openAlignDialog` / `_runAlignToGrid` / `_renderAlignDialog`
|
||||
(`src/houseplan-card.ts:14903-15985`) — подтверждает описанный в ТЗ
|
||||
текущий контракт диалога (`changed:false` → `gs.align_none` без Apply,
|
||||
`changed:true` → отчёт + Apply);
|
||||
- прочитаны оба реальных вызова `wallBodiesGeometry()` в продуктовом коде:
|
||||
iso-путь (`_isoSource`, строка ~5206) **бросает** `Error` при `null` и
|
||||
наличии стен/тел — это ровно тот "structural failure", который ТЗ
|
||||
требует ловить; light-barrier-путь (~15652) деградирует мягко
|
||||
(fallback на сырой контур). Плюс `_wallUnionGeometry`
|
||||
(~11259) — путь, которым реально рисуется масонри в 2D-виде: при
|
||||
`null`/пустом union он тоже молча откатывается на `paperRoomShapes`,
|
||||
то есть именно так пользователь видел "исчезнувшую кладку" в #197;
|
||||
- прочитан `wallBodiesUnionPath()` и `floorFootprintGeometry()` в
|
||||
`src/wall-thickness.ts` — подтверждает различение ТЗ §7.3 между `null`
|
||||
(structural failure) и «успешный пустой результат» (`united && !d` →
|
||||
не ошибка, просто нет масонри);
|
||||
- подтверждён large-house fixture (`demo/fixtures/large-house.mjs`):
|
||||
`FLOOR_COUNT=3`, `ROOMS_PER_FLOOR=20` → 60 комнат, `OPENING_COUNT=100`,
|
||||
`PARTITION_COUNT=60`, `COLUMN_COUNT=40` — числа в §10 ТЗ совпадают
|
||||
буквально;
|
||||
- проверены мутационные ID в `scripts/mutation-gate.mjs` — формат
|
||||
(`kebab-case`, уже есть прецеденты вроде `union-failure-kills-space`,
|
||||
`union-failure-silent`) совпадает с предложенными в ТЗ ID.
|
||||
5. Прочитан `docs/CANVAS.md` §9.5 («Оптимизировать планы») — контракт диалога,
|
||||
`optimizePlans`, one-deep snapshot, «отчёт — верхняя граница» совпадают с
|
||||
тем, как ТЗ их описывает.
|
||||
6. Прочитан `docs/TOUCH-SUPPORT.md` — safety floor («no data corruption or
|
||||
silent loss of saved plan data») дословно то основание, на которое ссылается
|
||||
ТЗ §1/§14.
|
||||
7. Прочитан `docs/USER-GUIDE.ru.md` (разделы про Optimize/толщину стен) —
|
||||
используемая в ТЗ терминология («Общие настройки → Оптимизировать планы»,
|
||||
«предпросмотр», «отмена») не изобретена, а взята оттуда.
|
||||
8. Сверены два ближайших прецедента той же подсистемы для проверки
|
||||
единообразия: `docs/specs/223-optimize-coordinate-canonicalization.md` и
|
||||
реализация #224 (`git show 4a798e3 --stat`) — чтобы понять, что в этом
|
||||
диалоге принято фиксировать в ТЗ дословно (RU+EN строки диалога) и что
|
||||
действительно исторически остаётся вне DoD (английский `USER-GUIDE.md`
|
||||
в #223/#224 не трогался — значит расхождение с ним не считается дефектом
|
||||
и в ТЗ #199 тоже).
|
||||
9. Проверен формат записи `docs/specs/README.md` (двухколоночная таблица
|
||||
issue↔ТЗ, без колонки «Статус ТЗ») — соответствует актуальному состоянию
|
||||
PROCESS.md §7.3 п.1.
|
||||
|
||||
Гейты кода в этом раунде не запускались — на этапе ревью ТЗ гейты (typecheck/
|
||||
test/build) неприменимы, продукт не менялся.
|
||||
|
||||
## Находки
|
||||
|
||||
### Medium (в скоупе — чинится в этом же ТЗ)
|
||||
|
||||
**M1. Английский текст сообщения об отказе и подсказки не зафиксирован
|
||||
дословно — только «эквивалент», что делает AC6 непроверяемым для EN-локали.**
|
||||
|
||||
- Файл: `docs/specs/199-optimize-geometry-preflight.md`, §8.1.
|
||||
- Текущий текст:
|
||||
```
|
||||
- RU: «Не удалось безопасно проверить геометрию следующих пространств:
|
||||
{spaces}{more}.»
|
||||
- RU hint: «Планы не изменены. Обновите House Plan и повторите. Если ошибка
|
||||
останется, приложите экспорт пространства к отчёту об ошибке.»
|
||||
- EN: equivalent plain-language text without implementation terms.
|
||||
```
|
||||
Для двух главных предложений диалога EN-строка не приведена вообще — только
|
||||
директива «эквивалентный текст». При этом в том же §8.1 для меньших
|
||||
подстрок (fallback-имя `Space N`, суффикс `and N more`) EN дан дословно —
|
||||
несогласованность внутри одного раздела.
|
||||
- Почему это находка, а не техническая деталь: это пользовательский текст
|
||||
нового диалогового состояния, то есть ровно то, что PROCESS.md §7.1 требует
|
||||
фиксировать в ТЗ, а не отдавать на изобретение при реализации («Владельцу
|
||||
задаются только продуктовые вопросы» не означает, что английскую копию
|
||||
придумывает реализатор без утверждённого текста — второй язык тоже
|
||||
наблюдаем пользователем).
|
||||
- Прецедент того же диалога: `docs/specs/223-optimize-coordinate-canonicalization.md`
|
||||
§7 добавляет новую строку в тот же Align-диалог и даёт пару целиком:
|
||||
```
|
||||
- RU: «обновлено пространств: {c}; устранён шум координат: {p}»;
|
||||
- EN: «spaces updated: {c}; noisy coordinate values removed: {p}».
|
||||
```
|
||||
#199 — прямое продолжение той же серии задач (#197/#198/#223/#224/#229) над
|
||||
тем же диалогом; стандарт "RU+EN дословно" в ней уже установлен и не должен
|
||||
тихо ослабляться для более заметного по важности сообщения (блокирующий
|
||||
отказ, а не информационная строка).
|
||||
- Как воспроизвести проверку: открыть §8.1 ТЗ, попытаться написать unit-тест
|
||||
на AC6 («i18n/UI unit») для английской локали — не из чего писать
|
||||
assertion на конкретную строку, только на «что-то не техническое».
|
||||
- Фикс: дописать в §8.1 дословный EN-эквивалент RU-сообщения и RU-hint (по
|
||||
образцу уже данных в том же разделе пар "Space N"/"and N more").
|
||||
- Серьёзность: **Medium**, в скоупе задачи. High-находок нет, поэтому цикл
|
||||
жёлтый, правка — в этом же ТЗ, без нового issue (#202).
|
||||
|
||||
### Low (снимается без действия, для протокола)
|
||||
|
||||
- Хендофф-комментарий автора ТЗ в issue содержит артефакт шаблона: «Коммит:
|
||||
$sha.» — переменная не подставлена. Это дефект комментария, не ТЗ; на
|
||||
содержание, полноту и проверяемость документа `docs/specs/199-*.md` не
|
||||
влияет. Не блокирует, отмечаю для протокола; ревьюер не правит issue-комментарии.
|
||||
|
||||
## Что проверено и корректно
|
||||
|
||||
- **Продуктовая рамка (§1–§2).** Персона (домашний администратор), поверхность
|
||||
(«Общие настройки → Оптимизировать планы») и момент названы; предложение
|
||||
«до/после» дано без терминов реализации. Явно назван J6 из SCOPE.md и
|
||||
safety floor TOUCH-SUPPORT.md — оба подтверждены чтением исходников этих
|
||||
документов, ссылка не декоративна.
|
||||
- **Проблема подтверждена по коду, а не заявлена.** Пять шагов §3 (preview →
|
||||
candidate → Apply → backend без polyclip-проверки) построчно совпадают с
|
||||
реальным `_openAlignDialog`/`_runAlignToGrid` в `src/houseplan-card.ts`; в
|
||||
частности, реальный fallback `_wallUnionGeometry` → `paperRoomShapes` при
|
||||
`null`/пустом union — это ровно механизм "молча исчезнувшая кладка" из #197,
|
||||
на который ссылается ТЗ.
|
||||
- **Различение `null` (structural failure) и «успешный пустой результат»
|
||||
(§7.3)** — не абстракция, а точное описание существующего поведения
|
||||
`wallBodiesUnionPath()`/`floorFootprintGeometry()`: первое — жёсткая
|
||||
ошибка (iso-путь её бросает исключением), второе — легитимный «пустая
|
||||
комната без кладки» результат, который renderer рисует как обычные комнаты.
|
||||
AC2/AC3 формулируют именно эту границу, и она проверяема.
|
||||
- **Продуктовые решения владельца (Q1–Q3) перенесены в контракт без
|
||||
искажения**: whole-plan block при любом failure (§7.1, AC4), блокировка
|
||||
уже деградированного пространства без исключения «не стало хуже» (§4 п.2,
|
||||
AC5), сообщение с именами пространств без технических деталей и без кнопки
|
||||
Apply (§8.1, AC6) — всё совпадает с текстом решений владельца в issue.
|
||||
- **Non-scope (§6)** явно исключает ремонт найденной геометрии, частичный
|
||||
Apply, смену geometry engine, миграцию схемы, изменение времени жизни Undo —
|
||||
снимает риск расползания скоупа, который прямо запрещён PROCESS.md §12.
|
||||
- **Performance-бюджет (§10) — не оценка на глаз.** Числа fixture (3
|
||||
пространства/60 комнат/100 проёмов/60 partitions/40 columns) и базовое
|
||||
измерение (median 155.9 ms / p95 162.56 ms / max 163.67 ms) сверены с
|
||||
`demo/fixtures/large-house.mjs` и являются реальным замером автора, а не
|
||||
придуманным ориентиром; относительный бюджет (20%+15ms поверх прямого
|
||||
builder) корректно отделяет overhead preflight-обвязки от уже дорогого
|
||||
polyclip-прохода.
|
||||
- **Backend/atomicity (§9)** не добавляет новый параметр endpoint и не требует
|
||||
второй реализации polyclip на Python — согласуется с Non-scope и с описанным
|
||||
в CANVAS.md §9.5 контрактом одной атомарной транзакции.
|
||||
- **AC1–AC14** — каждый пронумерован, однозначен и указывает способ
|
||||
доказательства (unit/smoke/golden/mutation/benchmark/code review), что
|
||||
требуется §2.5 DoR.
|
||||
- **Блок §16 «Принятые технические предположения»** корректно отделяет
|
||||
ненаблюдаемые пользователем решения (имена модулей, fingerprint-механизм,
|
||||
отсутствие `preflight_passed` в backend) от продуктового контракта и явно
|
||||
помечен как «ревьюер может оспорить» — соответствует PROCESS.md §7.1.
|
||||
- **Формальная структура ТЗ** покрывает все обязательные разделы §7.1
|
||||
(сценарий · что видит человек · проблема · скоуп/не-скоуп · контракт ·
|
||||
UX · данные/миграция · i18n · AC с доказательством · план тестов · риски ·
|
||||
откат · release-артефакты); запись `docs/specs/README.md` обновлена в
|
||||
формате, актуальном после чистки колонки «Статус ТЗ».
|
||||
- **Rollback (§15)** — простой revert реализационного коммита, персистентные
|
||||
данные не меняются новым preflight, что верно логически (preflight ничего
|
||||
не пишет и не канонизирует сверх уже полученного Optimize-кандидата, §7.3).
|
||||
|
||||
## Чего не проверял
|
||||
|
||||
- Не проверялся сам будущий код реализации — его ещё нет, ревью только ТЗ.
|
||||
- Не запускались `typecheck`/`test`/`build` — на этапе ревью ТЗ продукт не
|
||||
меняется, гейты неприменимы.
|
||||
- Не оценивалась реальная производительность на CI-машине (замер в ТЗ —
|
||||
замер автора на его Windows-чекауте); согласно ТЗ §10 это будущая забота
|
||||
code-review при нестабильности абсолютного бюджета в CI.
|
||||
- Не проверялись мутационные тесты `scripts/mutation-gate.mjs` предметно
|
||||
(их ещё нет для #199) — сверен только формат ID с существующими записями.
|
||||
- Не исследовался `docs/CONFIG-COMPATIBILITY.md` построчно — ТЗ прямо
|
||||
заявляет «без изменений и миграции», и этот пункт не оспаривается: он
|
||||
логически следует из того, что preflight — чистая read-only проверка
|
||||
существующего candidate, ничего не пишущая в schema.
|
||||
|
||||
## Итог
|
||||
|
||||
Единственная находка — Medium, в скоупе задачи, чинится правкой текста ТЗ
|
||||
(добавить дословный EN-текст в §8.1), без нового issue (#202). High-находок
|
||||
нет. Технические предположения корректны и проверены по коду `origin/dev`
|
||||
предметно, а не приняты на слово автора.
|
||||
|
||||
**Вердикт: жёлтый · заход r1 · блокирующих циклов 1/4 · High: 0 · Medium: 1 → в задаче**
|
||||
Reference in New Issue
Block a user