diff --git a/docs/reviews/SPEC-REVIEW-384-r1.md b/docs/reviews/SPEC-REVIEW-384-r1.md new file mode 100644 index 00000000..ad1a6a3e --- /dev/null +++ b/docs/reviews/SPEC-REVIEW-384-r1.md @@ -0,0 +1,150 @@ +# SPEC-REVIEW-384-r1 + +Issue: [#384](https://github.com/Matysh/houseplan-card/issues/384) · +этап: `S4-spec-review` · трек: `small` (ТЗ в теле issue) · заход r1 · +SHA рабочего дерева на момент ревью: `9c162c09`, ветка `dev`. + +## Скоуп + +Родитель — #373 (`fit: house`, docs/specs/373-space-card-house-fit.md, в +`dev` с v1.69.0). Баг: невидимые тела стен (`canonicalWallGeometry.components`), +extras (независимые стены/драфты/колонны) и нулевые стены безусловно участвуют +в structure-цикле кадра `fit: house` (`src/space-render.ts:428-448`), даже +когда `show_borders: false` и они физически не рисуются. Видимые проёмы уже +корректно гейтятся `!disp.hideOpenings` (:449). + +ТЗ (лёгкий трек, в теле issue) предлагает: +1. гейтить вклад тел стен (~:432), extras (~:438) и нулевых стен (~:442) тем же + `disp.showBorders`, которым уже гейтится их рендер (:657, :882, :914); +2. вернуть `needsCanonicalWallGeometry` (:388) к виду до #373: + `walls.length || (extras.length && disp.showBorders)`. + +Единственная затронутая поверхность — `src/space-render.ts`; трек `small` +заявлен корректно (см. «Проверка трека» ниже). + +## Как проверялось + +- Прочитаны `docs/SCOPE.md`, `PROCESS.md` (§1–§9), `AGENTS.md` полностью. +- Прочитано тело issue #384 и комментарий аналитики (единственный комментарий). +- Сверены построчные ссылки ТЗ с текущим кодом `src/space-render.ts` (чтением, + без исполнения): :388, :424-467, :654-657, :882, :914 — все совпадают + дословно с описанием в issue. +- `git log -p -S"needsCanonicalWallGeometry"` и `-S"fit === 'house'"` по + `src/space-render.ts` — подтверждено, что условие `walls.length || + (extras.length && disp.showBorders)` было исходным (коммит `955de3e6`, + до #373), а `|| fit === 'house'` добавлен именно коммитом #373 + (`0d33691f`). Заявление ТЗ «возвращается к виду до #373» фактически точное, + а не догадка. +- Прочитан `docs/specs/373-space-card-house-fit.md` (контракт-родитель) — + контракт п.5 говорит о «visible architectural extents», что и является + опорой для этого фикса. +- Прочитан `demo/smoke_space_card.mjs` целиком — существующая tight-фикстура + жёстко фиксирует `show_borders: true` (:16), поэтому её текущие ассерты + (`tightFrame`, `tightPaintedEnvelope.contained` ±0.51px, `tightNoTitleFrame`) + физически не заденет предложенный гейт: AC2 (регресс) проверяем. +- Прочитаны `docs/USER-GUIDE.md`, `docs/USER-GUIDE.ru.md` (разделы про + `fit: house`) и `docs/CANVAS.md` §4 (структурный кадр) — см. находку ниже. +- Код продукта не менялся, реализации нет — это ревью ТЗ, не код-ревью. + +## Находки + +### Medium (в скоупе задачи) — «Доки не задеты» неверно: три документа обещают безусловное участие стен/колонн в кадре + +**Файл:** тело issue #384, раздел «7. i18n / Release-артефакты». + +ТЗ утверждает: *«Доки: не задеты (спека #373 уже описывает желаемое поведение; +USER-GUIDE про fit не детализирует слои)»*. Это фактически неверно — оба +пользовательских гайда и канонический документ подсистемы (CANVAS.md, из +обязательного списка чтения AGENTS.md) уже детализируют состав кадра, и +детализируют его **безусловно**, без оговорки на `show_borders`: + +- `docs/USER-GUIDE.md:798-800`: *«`house` removes that intentional padding and + fits every room, wall, partition, column and opening symbol»* — без единого + слова о видимости стен; +- `docs/USER-GUIDE.ru.md:1543-1544`: *«Карточка сохранит в кадре все комнаты, + стены, перегородки, колонны и полный размах символов проёмов»* — то же самое + по-русски, снова безусловно; +- `docs/CANVAS.md:232-233`: *«every sane room, positive/zero wall, independent + wall or saved draft, column and complete door/window/gate symbol envelope + participates»* — канонический контракт подсистемы, тоже без оговорки. + +После фикса это перестаёт быть правдой для `show_borders: false`: стены, +extras и нулевые стены **больше не участвуют** в кадре, и все три документа +станут вводить в заблуждение читателя ровно про тот кейс, который эта задача +чинит. Симметричная оговорка для проёмов (`hide_openings`) в этих документах +уже отсутствует тоже, но её отсутствие не создаёт эта задача — а вот +безусловность по стенам она напрямую разрушает. + +**Сценарий воспроизведения:** пользователь читает `docs/USER-GUIDE.ru.md`, +включает `show_borders: false` и `fit: house`, ожидает (по документации) что +«все стены» останутся в кадре — и после фикса получает кадр без них, +документация при этом не предупреждает. + +Находка в скоупе: правки нужны в тех же трёх файлах, которые описывают именно +эту функцию (`fit: house`), и правило PROCESS.md §11 («документация — в том +же коммите, что и поведение») требует их обновить вместе с кодом. Без High +это не блокирует полностью, но раздел 7 ТЗ должен назвать все три файла +(`docs/USER-GUIDE.md`, `docs/USER-GUIDE.ru.md`, `docs/CANVAS.md`) как +затронутые release-артефакты, с оговоркой вида «скрытые стены (`show_borders: +false`) не голосуют» рядом с уже существующей оговоркой про backdrop/decor. + +## Что проверено и корректно + +- **Фактическая база ТЗ.** Все построчные ссылки (:388, :428-449, :657, :882, + :914) и утверждение о происхождении условия `needsCanonicalWallGeometry` + подтверждены чтением кода и git-историей — ни одной догадки, выданной за + факт, не найдено в техническом описании. +- **Согласованность предложенного гейта.** Прослежено, что реверт + `needsCanonicalWallGeometry` не меняет поведение при `disp.showBorders === + true` (условие остаётся истинным через тот же дизъюнкт) и не задевает + независимый путь «чистого пола» (`floorMinusBodies` в :791 использует сырой + `extras`, а не `canonicalWallGeometry`, и не зависит от этого флага) — + побочных эффектов на рендер пола вокруг колонн не возникает. +- **AC1–AC3** однозначны, каждый называет способ доказательства (юнит/смок) и + проверяемы: AC1/AC3 — новая twin-фикстура с `show_borders: false`, сравнение + с кадром «только комнаты+проёмы»; AC2 — регресс на существующих + tight-ассертах, которые используют фиксированный `show_borders: true` и + физически не пересекаются с новым гейтом. +- **AC4** («полный гейт зелёный; бюджет ≈ 0») корректен: изменение — два + булевых гейта плюс возврат одного условия, дополнительных вычислений не + вводит (скорее исключает лишний union там, где он был не нужен). +- **План тестов** называет конкретный механизм (twin-фикстура) и даже + mutation-gate («убрать новый гейт → красный смок») — это ровно та + дисциплина «тест умеет падать», которую требует код-ревью впоследствии; + для ТЗ-этапа это не обязательно, но повышает доверие к плану. +- **Откат** — обычный `git revert`, без флагов/миграций/персистентных данных; + корректно, изменение чисто в рендер-функции. +- **Раздел 8 («принятые предположения»)** — единственное явное предположение + (проёмы продолжают голосовать при `show_borders: false`) явно помечено как + предположение, а не выдано за факт; технически корректно и не требует + продуктового вопроса владельцу. +- **Проверка трека `small`.** Один модуль (`src/space-render.ts`), один + структурный цикл, ни миграции конфига, ни нового UX-контракта (контракт уже + зафиксирован #373, фикс восстанавливает его), ни влияния на perf/touch — + все пять критериев §5 выполняются одновременно; аналитика (единственный + комментарий issue) содержит явные оценки и не эскалирует продуктовых + вопросов владельцу, что соответствует §2.2. +- **Открытых продуктовых вопросов нет**, и это верно: поведение до/после + однозначно описано контрактом #373 («visible architectural extents»), + решать нечего — только привести код к уже принятому контракту. + +## Чего не проверял + +- Не запускал никакие гейты (`tsc`, `test`, `build`, смоки) — это ревью ТЗ, + реализации ещё не существует; кода для проверки нет. +- Не проверял en/de/fr переводы USER-GUIDE (нет отдельного файла с этим + текстом кроме `docs/USER-GUIDE.md`, который и есть канонический английский + источник) — задета только формулировка кадра, i18n строк интерфейса + (label'ы) фикс не касается, ТЗ верно говорит «i18n: не задето» в смысле + `src/i18n/*.json`. +- Не проверял поведение `fit: content` — по коду и ТЗ оно физически не + пересекается с правкой (гейт живёт только внутри блока `if (fit === + 'house')`, :424-467), поэтому не разбирал отдельно. +- Не оценивал производительность практически (профили не прогонял) — по + характеру изменения (два добавленных булевых условия) риск отсутствует, а + AC4 явно требует лишь «бюджет ≈ 0», не отдельного профиля. + +## Вердикт + +Жёлтый. High: 0. Medium: 1, в скоупе задачи — правится в этом же ТЗ (не +блокирует переход после исправления, но требует ещё один цикл ревью ТЗ).