mirror of
https://github.com/Matysh/houseplan-card
synced 2026-09-29 03:09:36 +00:00
committed by
Sergey Matyunin
parent
f4098fcf09
commit
b7a9cc4edf
@@ -0,0 +1,72 @@
|
||||
# SPEC-REVIEW-234-r1
|
||||
|
||||
- Issue: [#234](https://github.com/Matysh/houseplan-card/issues/234) — «Толщина отрезка цепочки молча сбрасывается на 15 см, хотя превью показывает нарисованную»
|
||||
- ТЗ: `docs/specs/234-chain-segment-thickness.md`
|
||||
- Ветка: `issue/234-chain-segment-thickness`, вершина на момент ревью: `88d8cfc`
|
||||
- Этап: spec-review, заход **r1** (первый), блокирующих циклов до этого раунда: 0/4
|
||||
- Трек: обычный (метки `bug`, `P2`, `S4-spec-review`; `small` не выставлен, что соответствует §5: затронуто пять мест записи/чтения — больше одной поверхности)
|
||||
|
||||
## Скоуп ревью
|
||||
|
||||
Проверялось:
|
||||
- соответствие ТЗ `docs/SCOPE.md` (какую строку core user jobs закрывает);
|
||||
- обязательные разделы ТЗ по PROCESS.md §7.1;
|
||||
- точность диагноза — сверка каждой ссылки на код (`src/houseplan-card.ts`, `src/wall-face-graph.ts`) с фактическим содержимым файлов на `88d8cfc`, чтобы отличить проверенный факт от догадки, выданной за решение;
|
||||
- однозначность и доказуемость каждого AC1…AC9;
|
||||
- согласованность с канонами `docs/WALL-THICKNESS.md`, `docs/CONFIG-COMPATIBILITY.md`, `docs/TOUCH-SUPPORT.md`, `docs/USER-GUIDE.ru.md`;
|
||||
- корректность классификации продуктового вопроса (§2.2/§7.1) и того, что не осталось скрытых продуктовых развилок под видом технических предположений.
|
||||
|
||||
Не проверялось (не требуется на этапе spec): исполнение кода, гейты `typecheck`/`test`/`build` — код ещё не написан, ревьюется только контракт.
|
||||
|
||||
## Продуктовая рамка
|
||||
|
||||
Задача закрывает **J6** «Keep the plan true as the home evolves» из `docs/SCOPE.md`: план не должен молча расходиться с тем, что человек нарисовал. Второстепенно касается **J4** (черновики, загруженные до фикса). Соответствует и `docs/WALL-THICKNESS.md` §6 («Every persisted draft segment remembers the value used when it was placed») и `docs/USER-GUIDE.ru.md:355` («Толщина каждого ранее поставленного отрезка остаётся своей») — предлагаемое исправление восстанавливает уже задокументированное поведение, а не вводит новый контракт. Расхождения с каноном не найдено.
|
||||
|
||||
## Проверка диагноза по коду
|
||||
|
||||
Диагноз ТЗ (§3) сверен построчно с `src/houseplan-card.ts` и `src/wall-face-graph.ts` на `88d8cfc`. Пять формул толщины отрезка, три разных fallback-паттерна, неатомарная запись точки (`:7259`/`:7260`) и слепое копирование `_draftSegmentCms` при восстановлении (`:2618`, `:7237-7239`, `:12721`, `:7364`, `:3083`, `:7501`, `:12738`) — подтверждены фактическим содержимым кода. Номера строк в ТЗ смещены на 1–3 (метод/вызов начинается чуть раньше или позже цитируемой строки) — это шум, не искажающий диагноз, отдельного замечания не требует.
|
||||
|
||||
Единственная содержательная неточность: текущий `wallChainSegments` (`wall-face-graph.ts:69`) считает валидным `cms[i] >= 0` (ноль — валидное значение), а контракт §6 ТЗ определяет валидность как `> 0`. Это не догадка — ТЗ прямо называет это риском (§15.3: «старые черновики могут содержать нули… правило §6 считает валидным только `> 0`») и принимает решение сознательно, а не выдаёт его за существующее поведение. Отражено в разделе «Находки» ниже как Low.
|
||||
|
||||
## Находки
|
||||
|
||||
### Medium (в скоупе, чинится в этом же ТЗ)
|
||||
|
||||
**M1 — раздел `i18n` из обязательного списка §7.1 отсутствует в документе целиком.**
|
||||
`PROCESS.md` §7.1: «Обязательные разделы ТЗ: … модель данных и миграция · **i18n** · критерии приёмки …». В `docs/specs/234-chain-segment-thickness.md` нет ни заголовка, ни строки про i18n — не «не затронут», а полное отсутствие раздела. Аналитический комментарий в issue («i18n не задет») это утверждение содержит, а сам документ ТЗ, который проходит в DoR по §2.5 («i18n: ключи en + ru перечислены»), нет.
|
||||
*Почему это находка, а не формальность:* §2.5 делает этот пункт обязательным условием статуса «Готово к разработке»; ТЗ без него не должно проходить в `S5-ready`, даже если по существу ответ — «не затронут».
|
||||
*Воспроизведение:* `grep -c i18n docs/specs/234-chain-segment-thickness.md` → `0`.
|
||||
*Что нужно:* одна строка вида «i18n: не затронут — задача не добавляет и не меняет строк интерфейса» в новом разделе или в §5/§8. Проверено чтением всего файла, не догадкой по объёму.
|
||||
|
||||
### Low (снимается с записью)
|
||||
|
||||
**L1 — граница `> 0` vs текущее `>= 0` не отражена явно в примерах AC1.**
|
||||
`wall-face-graph.ts:69` сегодня трактует `cms[i] === 0` как валидную запись (не «дырку»); контракт §6 меняет это на «`0` — невалидно, наследуется». ТЗ называет это риском (§15.3) и явно фиксирует правило (`> 0`), то есть решение принято, не спрятано. Но пример входных данных в AC1 («включая… `NaN`, `-5`, `null`») не называет `0` отдельно, хотя это ровно та граница, которая меняет поведение относительно текущего кода — и именно такие границы мутационный гейт (§11) обычно ловит по недосмотру теста, а не по недосмотру контракта.
|
||||
*Почему Low, не Medium:* правило контракта однозначно (`> 0`, буквально в тексте §6), и `docs/WALL-THICKNESS.md` §9 подтверждает, что валидный ввод для отрезка черновика — 1…100 см, то есть 0 в проде через UI недостижим и годной альтернативы (`>= 0`) не оставляет продуктового спора — это техническая полнота примера, а не неоднозначность решения.
|
||||
*Решение ревьюера:* снимается с записью. Не блокирует — при реализации `0` естественно покрывается таблицей входов AC1/AC2 («любые входы»), а расхождение с текущим `wallChainSegments` уже названо и снимается тем, что эта функция лишается собственного fallback (§6, последний абзац).
|
||||
|
||||
## Что проверено и корректно
|
||||
|
||||
- Все обязательные разделы §7.1 присутствуют, кроме i18n (M1): сценарий и персона (§1), что человек увидит до/после (§2), диагноз/проблема (§3), продуктовые решения (§4), скоуп/не-скоуп (§5), контракт поведения (§6), инвариант (§7), поверхности с явной touch-классификацией `Touch editor: not exposed` (§8, корректно по `docs/TOUCH-SUPPORT.md` — рисование стен на тач сегодня не выведено, значит не затронуто по построению), изменяемые файлы (§9), AC1…AC9 с доказательством (§10), mutation guards (§11), план автотестов (§12), перф/безопасность/touch (§13), откат (§14), риски (§15), release-артефакты (§16), блок принятых технических предположений (§17).
|
||||
- Продуктовый вопрос («чем заполнять отсутствующую запись») — ровно один, сформулирован в терминах видимого поведения, с предложенным дефолтом, закрыт по §2.2 (молчание = согласие); в ТЗ явно отделён от технических предположений (§17, последний абзац: «Не является предположением…»).
|
||||
- Диагноз проверен построчно по фактическому коду (см. выше) — не догадка, выданная за факт.
|
||||
- Все AC однозначны, у каждого указан способ доказательства (unit/smoke/код-ревью); AC3 и AC4 воспроизводят ровно диагностированный симптом (`30, 30, 15` → `30, 30, 30`).
|
||||
- Упомянутые регрессионные смоки (`demo/smoke_draw_wall_thickness.mjs`, `demo/smoke_wall_thickness_transition.mjs`) существуют — проверено чтением каталога `demo/`, не выдумано.
|
||||
- Мутационные гварды (§11) реалистичны: формат `{id, mutants:[{find,replace}]}` в `scripts/mutation-gate.mjs` совпадает с уже существующими записями.
|
||||
- Модель данных/миграция и compatibility-поля: формат `partitions[].cm` / `room_drafts[].segments[].cm` не меняется, `docs/CONFIG-COMPATIBILITY.md` не требует новой записи — задача не вводит и не читает по-новому ни одно совместимое поле оттуда.
|
||||
- `MAX_DRAFT_POINTS = 500` (`houseplan-card.ts:581`) — константа существует, довод о перф в §13 обоснован.
|
||||
- Откат (§14) реалистичен: формат данных не меняется, откат — одна ревизия кода.
|
||||
|
||||
## Чего не проверял
|
||||
|
||||
- Реализацию — её не существует, стадия — spec.
|
||||
- Golden/скриншоты — заявлено «не затронуто», это утверждение не тестировалось (нет кода), принято на основании того, что задача не меняет формулы рендера, только запись/чтение данных.
|
||||
- Точное значение константы `DRAW_WALL_DEFAULT_CM` (что это буквально `15`) — не открывал файл с определением, принял по согласованности с `docs/WALL-THICKNESS.md` («default 15 cm») и текстом ТЗ.
|
||||
|
||||
## Вердикт
|
||||
|
||||
Единственная блокирующая-по-Medium находка (M1) — отсутствие обязательного раздела `i18n`, требует одной строки правки. High-находок нет. Диагноз проверен и точен, контракт однозначен, AC доказуемы, продуктовый вопрос закрыт корректно.
|
||||
|
||||
**Вердикт: жёлтый · заход r1 · блокирующих циклов 1/4 · High: 0 · Medium: 1 · Документ: docs/reviews/SPEC-REVIEW-234-r1.md**
|
||||
|
||||
Возврат автору: добавить раздел/строку `i18n` в ТЗ (M1). L1 снят с записью, отдельного действия не требует.
|
||||
Reference in New Issue
Block a user