mirror of
https://github.com/Matysh/houseplan-card
synced 2026-09-28 19:01:34 +00:00
@@ -0,0 +1,165 @@
|
||||
# SPEC-REVIEW-123-r1
|
||||
|
||||
- **Issue:** https://github.com/Matysh/houseplan-card/issues/123
|
||||
- **ТЗ под ревью:** `docs/specs/123-corner-split-wall.md` (коммит `ba56d4f`)
|
||||
- **Роль:** ревьюер ТЗ (не автор), этап `S4-spec-review`
|
||||
- **Трек:** обычный (не `small`) — оценка сложности 7/10, больше одной поверхности,
|
||||
визуальная и световая геометрия; лёгкий трек корректно не применён
|
||||
- **Цикл:** r1/4
|
||||
|
||||
## Скоуп ревью
|
||||
|
||||
Проверялось соответствие ТЗ:
|
||||
- `docs/SCOPE.md` — попадание в Core user jobs, отсутствие расширения скоупа;
|
||||
- `PROCESS.md` §2.4, §2.5 (DoR), §7.1 (обязательные разделы) и §12 (запреты);
|
||||
- `AGENTS.md` — классы файлов, ветка, легитимность приёма чужого issue в процесс;
|
||||
- каноническим документам затронутой подсистемы: `docs/WALL-THICKNESS.md`,
|
||||
`docs/SUN.md`, `docs/LIGHT.md`, `docs/ISOMETRIC.md`, `docs/TOUCH-SUPPORT.md`,
|
||||
`docs/CONFIG-COMPATIBILITY.md`;
|
||||
- `docs/USER-GUIDE.ru.md` — терминология «Split» / «Перегородка»;
|
||||
- фактическому состоянию кода (`src/wall-thickness.ts`, `src/iso-walls.ts`,
|
||||
`src/space-render.ts`, `test/wall-thickness.test.mjs`) — на предмет того, что
|
||||
технические утверждения ТЗ не являются непроверенной догадкой.
|
||||
|
||||
## Как проверялось
|
||||
|
||||
1. Прочитан весь тред issue #123, включая решение владельца о приёме чужого
|
||||
issue в процесс (после правки конвейера, коммит `024cdc0`, issue #114) и
|
||||
протокол аналитики с defaults Q1–Q3, принятыми владельцем 2026-08-13
|
||||
(комментарий https://github.com/Matysh/houseplan-card/issues/123#issuecomment-5283843252).
|
||||
2. Сверены обязательные разделы ТЗ (§7.1 PROCESS.md) построчно — см. таблицу ниже.
|
||||
3. Прочитан код `wallBodiesGeometry()` (`src/wall-thickness.ts:1361-1412`):
|
||||
подтверждено, что тело стены строится как per-room `outset(poly, half) −
|
||||
inset(poly, half)`, затем `union` по комнатам — именно механизм, который ТЗ
|
||||
называет причиной дефекта (диагональный Split из вершины вносит острые митры
|
||||
дочерних комнат в наружный union).
|
||||
4. Прочитан `src/iso-walls.ts` и `docs/ISOMETRIC.md` — подтверждено, что скрытая
|
||||
изометрия уже потребляет тот же `wallBodiesGeometry()` MultiPolygon, а не
|
||||
отдельную модель; утверждение ТЗ §6.4 о единой геометрии для Plan/View/
|
||||
`houseplan-space-card`/изометрии не является новым архитектурным изобретением
|
||||
автора, а фиксирует уже существующий контракт.
|
||||
5. Прочитан `docs/LIGHT.md` («Opaque: the wall bodies exactly as the plan draws
|
||||
them (`wallBodiesGeometry`)») — подтверждает AC7 (Glow/солнце используют то
|
||||
же исправленное preграждение) технически достижимым без отдельной правки
|
||||
light-барьеров.
|
||||
6. Прочитан `docs/TOUCH-SUPPORT.md` — формулировка «safety floor» и «pointer
|
||||
cancellation» в ТЗ §9 дословно соответствует канону, а не придумана.
|
||||
7. Прочитан `docs/USER-GUIDE.ru.md` (таблица инструментов, разделы «Split» и
|
||||
«Перегородка») — терминология ТЗ совпадает с пользовательским словарём,
|
||||
различие Split/Перегородка воспроизведено верно и явно вынесено в не-скоуп
|
||||
(п.5.4).
|
||||
8. Прочитан `test/wall-thickness.test.mjs` — регрессионные сценарии, которые ТЗ
|
||||
в §11.1 п.7 требует не сломать (partial shared wall, virtual-T mitre, nested
|
||||
room, 45° wall, split materialisation), реально существуют в файле, то есть
|
||||
план автотестов не ссылается на несуществующее покрытие.
|
||||
9. Проверено, что `docs/ARCHITECTURE.md`, `docs/STATUS.md`,
|
||||
`docs/CHANGELOG(.ru).md` существуют — release-артефакты в §13 указывают на
|
||||
реальные файлы.
|
||||
10. Проверена запись в `docs/specs/README.md` — строка на #123 добавлена в том
|
||||
же коммите, ссылка issue ↔ ТЗ двусторонняя.
|
||||
|
||||
## Обязательные разделы (§7.1 PROCESS.md)
|
||||
|
||||
| Раздел | Есть | Комментарий |
|
||||
|---|---|---|
|
||||
| Сценарий (персона/поверхность/момент) | ✅ | §1 |
|
||||
| Что человек увидит до/после (без терминов реализации) | ✅ | §1, одна фраза |
|
||||
| Проблема | ✅ | §2, с воспроизведёнными числами bbox на `948f284` |
|
||||
| Скоуп / не-скоуп | ✅ | §4 / §5 |
|
||||
| Контракт поведения | ✅ | §6 |
|
||||
| UX | ✅ | §9 |
|
||||
| Модель данных и миграция | ✅ | §8 |
|
||||
| i18n | ✅ | §9 (пусто, обосновано) |
|
||||
| AC1…ACn с доказательством | ✅ | §10, 13 штук, каждый с типом |
|
||||
| План автотестов | ✅ | §11 |
|
||||
| Риски | ✅ | §14 |
|
||||
| Откат | ✅ | §15 |
|
||||
| Release-артефакты | ✅ | §13 |
|
||||
|
||||
Все обязательные разделы присутствуют и содержательны, не формальные заглушки.
|
||||
|
||||
## Находки
|
||||
|
||||
Находок уровня **High** и **Medium** нет.
|
||||
|
||||
### Low-1 — тип доказательства AC11 не входит буквально в перечень §2.5
|
||||
|
||||
**Файл:** `docs/specs/123-corner-split-wall.md:293-295`
|
||||
|
||||
AC11 помечен `(performance + ревью кода)`. DoR (`PROCESS.md` §2.5) перечисляет
|
||||
допустимые типы доказательства как `unit` / `backend` / `smoke` / `golden` /
|
||||
«ревью кода»; литерала `performance` в этом перечне нет. По существу критерий
|
||||
всё равно доказуем: в ТЗ явно указано «ревью кода» вторым типом, а
|
||||
`performance_smoke`/large-house benchmark — существующие release-blocking гейты
|
||||
(§11.4 этого же ТЗ, `PROCESS.md` §8), а не новый вид проверки. Блокирующим не
|
||||
является, но для чистоты трассируемости стоит переформулировать доказательство
|
||||
AC11 как «ревью кода» с явной ссылкой на существующий `performance_smoke`/
|
||||
large-house benchmark, не вводя пятый тип доказательства.
|
||||
|
||||
**Решение ревьюера:** Low, не блокирует. Можно поправить формулировку при
|
||||
следующей правке ТЗ или снять с этой записью — оставляю на усмотрение автора,
|
||||
т.к. критерий по сути проверяем и не создаёт риска для DoR.
|
||||
|
||||
## Что проверено и корректно
|
||||
|
||||
- Легитимность приёма issue в процесс (чужой автор, но явно допущен владельцем
|
||||
после правки конвейера #114) — не относится к дефектам ТЗ, отдельно
|
||||
зафиксировано в треде issue самим владельцем.
|
||||
- Соответствие `docs/SCOPE.md`: задача закрывает J6 («Keep the plan true as the
|
||||
home evolves») и частично J4 (встроенный редактор без искажений архитектуры),
|
||||
обе строки в статусе «Closed» — это регрессионный баг внутри уже принятой
|
||||
функциональности, а не новая фича и не расширение скоупа.
|
||||
- Владелец лично принял defaults Q1–Q3 и приоритет P2 (комментарии
|
||||
2026-08-13T16:52 и 17:00) — открытых продуктовых вопросов в финальной
|
||||
редакции ТЗ нет, и это корректно: вопросы были заданы и закрыты на этапе
|
||||
аналитики, а не додуманы автором.
|
||||
- Технический диагноз причины (per-room `outset−inset` union, острые митры
|
||||
дочерних комнат Split входят в наружный силуэт) подтверждён чтением
|
||||
`src/wall-thickness.ts` — не голословное утверждение автора.
|
||||
- Раздел 16 «Принятые технические предположения» корректно отделяет свободно
|
||||
изменяемые технические решения (имена helper'ов, конкретная boolean-
|
||||
декомпозиция, имя golden/smoke сценария) от решений владельца Q1–Q3,
|
||||
которые пересмотру не подлежат — ни одна догадка не выдана за факт без
|
||||
пометки.
|
||||
- Не найдено ни одного утверждения о поведении, которое не следует ни из
|
||||
канонических документов, ни из принятых владельцем defaults, ни из чтения
|
||||
существующего кода, и при этом не помечено как предположение.
|
||||
- AC1–AC13 однозначны, у каждого указан тип доказательства и он входит (кроме
|
||||
Low-1) в допустимый по DoR список; план автотестов (§11) даёт конкретный,
|
||||
проверяемый маршрут для каждого, включая явное требование «тест из п.3
|
||||
обязан краснеть на `948f284`» — критерий, защищающий от неспособного падать
|
||||
теста.
|
||||
- Не-скоуп (§5) корректно отсекает смежные соблазны (не превращать Split в
|
||||
Перегородку, не трогать инструмент «Перегородка», не менять модель данных,
|
||||
не вводить новый UX для cap/join) — типичные места, где скоуп мог бы незаметно
|
||||
расшириться.
|
||||
- Release-артефакты (§13) перечисляют реальные файлы, включая
|
||||
`docs/WALL-THICKNESS.md` (exterior/shared junction invariant) и
|
||||
`docs/USER-GUIDE.ru.md` — корректная точка правки терминологии для
|
||||
пользователя.
|
||||
- Реестр `docs/specs/README.md` обновлён тем же коммитом, ссылка issue ↔ ТЗ
|
||||
двусторонняя (`PROCESS.md` §7.1).
|
||||
|
||||
## Чего не проверял
|
||||
|
||||
- Не проверял, что предложенная в §7 архитектурная декомпозиция (exterior
|
||||
envelope vs shared divider body) реализуема без регрессии в
|
||||
`polyclip-ts`-based boolean операциях — это по правилам ТЗ (§16 п.2)
|
||||
свободно изменяемое техническое предположение автора кода, не предмет
|
||||
ревью ТЗ.
|
||||
- Не проверял производительность реального large-house benchmark — AC11
|
||||
предполагает существующий гейт, а не новый, и это станет предметом ревью
|
||||
кода/пре-релизного гейта, не ревью ТЗ.
|
||||
- Не запускал никаких автотестов — на этапе `spec` это не требуется; проверка
|
||||
существования регрессионных сценариев (см. «Как проверялось», п.8) сделана
|
||||
чтением файла, не исполнением.
|
||||
- Не проверял корректность конкретных числовых bbox-диагностик из §2 —
|
||||
доверяю записи владельца/автора в треде issue как источнику числа, поскольку
|
||||
оно уже независимо зафиксировано в комментарии аналитики до написания ТЗ.
|
||||
|
||||
## Вердикт
|
||||
|
||||
Зелёный. High: 0, Medium: 0. Одна находка Low (AC11 формулировка типа
|
||||
доказательства) — не блокирует, оставлена автору на усмотрение с записью в этом
|
||||
документе (не «TODO», а фиксированное решение ревьюера: можно поправить или
|
||||
отклонить без нового цикла).
|
||||
Reference in New Issue
Block a user