docs: review document for #113

Issue: #113
User-Visible: no

docs/reviews/SPEC-REVIEW-113-r1.md

@@ -0,0 +1,328 @@
+# SPEC-REVIEW-113-r1
+
+- **Issue:** https://github.com/Matysh/houseplan-card/issues/113
+- **ТЗ под ревью:** `docs/specs/113-optional-space-model.md` (коммит
+  `5f02dd38a1b60fa5064ea1291559b32c502fa1f0`, ветка
+  `issue/113-optional-space-model`)
+- **Роль:** ревьюер ТЗ (не автор), этап `S4-spec-review`
+- **Трек:** обычный (не `small`/`trivial`) — аналитика оценила сложность и
+  риск 6/10 и 7/10, что превышает порог лёгкого трека (§5 PROCESS.md, ≤3);
+  ТЗ корректно лежит файлом в `docs/specs/`, зарегистрировано в
+  `docs/specs/README.md:85`.
+- **Цикл:** r1/4
+
+## Скоуп ревью
+
+Проверялось соответствие ТЗ:
+
+- `docs/SCOPE.md` — попадание задачи в J6 («план остаётся правдивым по мере
+  развития», системная защита от ложного/пустого плана), отсутствие
+  расширения скоупа за пределы честного optional-контракта;
+- `PROCESS.md` §2.4, §2.5 (DoR), §7.1 (обязательные разделы ТЗ), §3/§12
+  (запреты, включая «догадка вместо решения»);
+- `AGENTS.md` — классы файлов (класс C для этого коммита), ветка, трейлеры;
+- фактическому состоянию `src/houseplan-card.ts` — на предмет того, что
+  диагноз ТЗ и цифры issue (45 полевых обращений `_spaceModel().<поле>`,
+  недостижимость до render-гейта, факт того, что #111 закрыл один вызов) не
+  являются непроверенной догадкой;
+- прецедентам house-style в уже принятых `docs/reviews/SPEC-REVIEW-*.md`
+  (107, 122, 123, 131, 137, 138, 141, 146) — как единообразно оформлены
+  обязательные разделы §7.1 и как в этом репозитории калибруется
+  High/Medium/Low для находок такого типа (структурная полнота ТЗ против
+  дефекта контракта).
+
+## Как проверялось
+
+1. Прочитан весь тред issue #113: тело (находка M3 из ревью #111), комментарий
+   аналитики от 2026-08-14 (ценность 7/10 пользователю, 9/10 разработке,
+   сложность/риск 6/10, 7/10, P2, tech-debt, обычный трек, «продуктовых
+   вопросов нет — технические guard patterns решаются в ТЗ без вопроса
+   владельцу») и комментарий автора ТЗ от 2026-08-15 («продуктовых вопросов
+   нет; выбран честный optional API»). Продуктовых вопросов владельцу
+   корректно не задавалось — вся задача технического характера, что
+   соответствует правилу «владельцу задаются только продуктовые вопросы».
+2. Построчно сверены обязательные разделы ТЗ (§7.1 PROCESS.md) — таблица
+   ниже.
+3. Прочитан код, на который опирается диагноз ТЗ, чтобы отличить проверенный
+   факт от догадки:
+   - `_spaceModel(id?): SpaceModel` (`src/houseplan-card.ts:2984`) —
+     сигнатура и тело (`m.find(...) || m[0]`) подтверждают проблему буквально
+     как в §1 ТЗ.
+   - Подсчитано число полевых обращений `_spaceModel([^)]*)\.<поле>`:
+     `rooms` — 35, `wall_columns` — 6, `room_drafts` — 2, `bg` — 2, плюс один
+     дополнительный сайт `_spaceModel(spaceId).vb` (`:15897`), не учтённый в
+     таблице тела issue (45 vs фактических 46). Это неточность в теле самого
+     issue, не в ТЗ (ТЗ не повторяет число 45), и не влияет на контракт ТЗ —
+     не выношу отдельной находкой.
+   - `render()` (`:14281`) подтверждён как ранний empty-state gate
+     (`if (!model.length) return html\`...\`;`), `willUpdate`/`updated`
+     (`:3089`, `:3116`) подтверждены как выполняющиеся **до** и **независимо**
+     от результата `render()` (Lit вызывает их для каждого прохода жизненного
+     цикла), что подтверждает центральный тезис ТЗ и issue — недостижимость
+     держится на порядке вызовов, а не на типе.
+   - `_continuityAssetsReady()` (`:2763-2769`) подтверждён как фикс #111:
+     явный `if (!this._model.length) return false;` **до** вызова
+     `_spaceModel()` — соответствует описанию issue «#111 закрыл один вызов
+     из render snapshot».
+   - Default-parameter паттерн `space = this._spaceModel()`
+     (`_physicalBodiesR`, `_rawPhysicalBodiesR`, ещё один метод — `:11668`,
+     `:11679`, `:11688`) подтверждён как реально существующий — именно тот
+     паттерн, который §5.3 ТЗ явно требует убрать.
+   - Проверено отсутствие текущих non-null assertions на `_spaceModel()`
+     (`grep "_spaceModel([^)]*)!"` — пусто) — согласуется с ТЗ §8, который
+     запрещает их вводить, а не убирает существующие.
+   - Найдены реальные call sites с явным `id`, показательные для §6 ТЗ:
+     `_livePos(d)` (`:4535` → `this._spaceModel(d.space)`), `_vacStartFit`
+     (`:15376` → `this._spaceModel(d.space)`), `_vacPlanRoomAnchors`
+     (`:15302`), сайт создания/перемещения устройства (`:12043` →
+     `this._spaceModel(space || undefined)`) — все передают «стабильный»,
+     потенциально устаревающий `spaceId`, а не всегда `this._space`. Это
+     именно тот класс вызовов, который §6 ТЗ описывает как «команды с
+     stable spaceId».
+   - Существующие тесты вида `test/*-contract.test.mjs`
+     (`isometric-contract.test.mjs`, `release-contract.test.mjs`,
+     `performance-contract.test.mjs`, `native-select-contract.test.mjs`)
+     подтверждают, что «source-contract test» из §8/§10 ТЗ — не изобретённый
+     механизм, а продолжение существующего паттерна репозитория.
+   - `docs/ARCHITECTURE.md` и `docs/TESTING.md` существуют — release-артефакты
+     §13 ТЗ ссылаются на реальные файлы, не выдуманные.
+4. Сопоставлены между собой §4 (нормативный API), §6 (active id и fallback),
+   §10 AC8 и §14 (риски) на непротиворечивость — обнаружено расхождение,
+   см. Medium-1.
+5. Проверены трейлеры и class-принадлежность: `git show --stat 5f02dd3`
+   показывает только `docs/specs/113-optional-space-model.md` и
+   `docs/specs/README.md` (класс C, ни одного файла класса A — продуктовый
+   код не тронут до `S5-ready`, Rule #1 `AGENTS.md` соблюдено); коммит несёт
+   `Issue: #113` и `User-Visible: no` — корректно для документа ТЗ, который
+   сам не меняет поведение.
+
+## Обязательные разделы (§7.1 PROCESS.md)
+
+| Раздел | Есть | Комментарий |
+|---|---|---|
+| Сценарий (персона/поверхность/момент) | ⚠️ | Нет отдельного заголовка; содержание фактически распределено по §1 (кто и когда встречает пустой план) — см. Low-1 |
+| Что человек увидит до/после | ⚠️ | Нет отдельного заголовка; ответ («ничего видимо не меняется, это профилактика класса #111») восстановим из §1+§9, но не сформулирован явно одной фразой — см. Low-1 |
+| Проблема (с подтверждённой причиной) | ✅ | §1, факты подтверждены чтением кода (см. «Как проверялось» п.3) |
+| Скоуп / не-скоуп | ✅ | §2 (цели) / §3 (не входит в задачу), границы чёткие (без глобального `noUncheckedIndexedAccess`, без миграции, без empty-state UX) |
+| Контракт поведения | ✅ | §4–§8, классификация call sites по 4 категориям, конкретные примеры кода |
+| Модель данных и миграция | ✅ | §9 — явно «config/layout schema и revisions не меняются» |
+| UX, i18n, accessibility, touch | ⚠️ | §9 содержательно утверждает «editor touch safety floor не меняется», но не использует ни одну из трёх канонических формулировок `docs/TOUCH-SUPPORT.md` («Touch editor: supported/best effort/not exposed») — см. Low-3 |
+| AC1…ACn с доказательством | ⚠️ | §10, 10 штук, пронумерованы и в целом проверяемы, но ни один не несёт явной пометки способа доказательства (`unit`/`smoke`/…), в отличие от всех сверенных прецедентов (107, 122, 123, 131, 137, 141, 146) — см. Low-2. Отдельно для AC8 отсутствие явного доказательства — не косметика, а реальный пробел, см. Medium-1 |
+| План автотестов | ✅ | §11, разбит на unit/smoke/регрессию, включает mutation-тест («вернуть required signature — тест красный») |
+| Риски | ✅ | §14, таблица риск/мера, 5 строк |
+| Откат | ✅ | §14 (последний абзац) — без миграции данных |
+| Release-артефакты | ✅ | §13, конкретный список (`ARCHITECTURE.md`, `TESTING.md`), оба файла существуют, `User-Visible: no` корректно снимает требование changelog |
+
+Присутствует также раздел «Принятые технические предположения» (§15, 4
+пункта) — соответствует требуемому PROCESS.md §7.1 блоку «принято
+предположительно, поменять свободно», хотя заголовок не содержит буквально
+эту оговорку (косметика, не отдельная находка).
+
+## Находки
+
+### Medium-1 — AC8 не имеет предъявленного механизма доказательства; риск и контракт расходятся
+
+**Файл:** `docs/specs/113-optional-space-model.md:110-130` (§6),
+`:180` (AC8), `:186-201` (§11, unit-план), `:234-241` (§14, таблица рисков)
+
+§14 называет риск буквально: «Stale id редактирует первый space» и указывает
+единственную меру — «exact lookup для commands». Это ровно тот самый риск,
+который AC8 обязан закрывать: «Missing explicit stale space id не мутирует
+первый space».
+
+Но нормативный API §4 —
+
+```ts
+private _spaceModel(id?: string): SpaceModel | undefined {
+  const requested = id ?? this._space;
+  return this._model.find((space) => space.id === requested) ?? this._model[0];
+}
+```
+
+— **всегда** возвращает `_model[0]`, когда `requested` не найден, независимо
+от того, был ли `id` передан явно или нет. Для непустой модели эта функция
+никогда не возвращает `undefined`: она возвращает объект, только не тот,
+который просили. Проверено на реальных call sites: `_livePos(d)`
+(`houseplan-card.ts:4535`) вызывает `this._spaceModel(d.space)` для позиции
+устройства, `_vacStartFit` (`:15376`) — аналогично. Если пространство,
+которому принадлежит устройство (`d.space`), было удалено (что штатно для
+задачи #113 — «удаление последнего/произвольного space»), а другие
+пространства остались, `_spaceModel(d.space)` молча вернёт **первое
+оставшееся** пространство — тип этого не ловит, потому что результат
+определён (не `undefined`).
+
+Единственная названная в ТЗ мера — вынести отдельный
+`_spaceModelById(id)` без fallback (§6) — сформулирована как **опция**:
+«Чтобы исключить опасную двусмысленность, **допустимо** разделить API...
+Это техническое разделение **рекомендуется** для drag/history/dialog
+commands». Формулировка не обязывает автора реализации ввести этот метод
+и не обязывает каждый call site с явным id использовать его вместо
+`_spaceModel(id)`. Единственная альтернатива, которую ТЗ предлагает
+взамен, — ручная проверка identity на каждом call site («callers... должны
+проверять identity отдельно и abort-ить»), но:
+
+- ни §8 (source-contract test), ни §11 (план тестов) не называют проверку,
+  которая убедилась бы, что *каждый* call site с явным id либо использует
+  no-fallback lookup, либо содержит ручной identity-guard;
+- unit-пункт §11 «exact lookup не падает в first-space fallback» тестирует
+  сам вспомогательный selector в изоляции, а не то, что производственные
+  call sites (`_livePos`, `_vacStartFit`, сайт создания устройства `:12043`
+  и другие, использующие явный `id`/`spaceId`) действительно его
+  применяют.
+
+**Почему это Medium, а не High:** контракт не производит немедленную,
+гарантированную регрессию (в отличие от `SPEC-REVIEW-138-r1` High-1, где
+буквальное прочтение ТЗ ломает работающий сегодня клик) — сегодняшний код
+уже имеет такое же молчаливое поведение при stale `id`, задача #113 его не
+ухудшает. Риск в том, что заявленная в AC8 защита от этого класса дефектов
+не гарантирована структурно (типом или тестом), а оставлена на
+дисциплину конкретных call sites без перечня, что именно нужно проверить —
+то есть тот же класс проблемы, из-за которого появился сам #113
+(«недостижимость держится на порядке вызовов, а не на типе»), может
+частично воспроизвестись для explicit-id веток, если реализация не
+проявит собственную дисциплину сверх того, что явно требует контракт.
+
+**Что нужно поправить:** заменить «допустимо»/«рекомендуется» в §6 на
+обязательное правило — любой call site, передающий явный/стабильный
+`id`/`spaceId` в мутирующем или persist-контексте, обязан использовать
+no-fallback lookup (или эквивалентный identity-guard), и добавить в §8/§11
+конкретную проверку (source-contract grep по списку известных
+call sites, либо unit-тест, дергающий каждую публичную мутирующую точку со
+stale id и проверяющий отсутствие записи/side effect). Это техническое
+уточнение, не продуктовый вопрос — решается автором и ревьюером кода без
+эскалации владельцу.
+
+**Решение ревьюера:** Medium, заведён отдельный issue
+[#184](https://github.com/Matysh/houseplan-card/issues/184) со ссылкой на
+#113 и на этот документ. Не блокирует переход ТЗ в `S5-ready`: остальной
+контракт (9 из 10 AC, вся lifecycle/render/cleanup часть) самодостаточен,
+проверяем и корректен, а сама эта находка — уточнение контракта, которое
+разумно донести до автора кода явно, а не через возврат ТЗ на цикл.
+
+### Low-1 — нет отдельных заголовков «Сценарий» и «Что человек увидит до/после»
+
+**Файл:** `docs/specs/113-optional-space-model.md:9-27` (§1)
+
+PROCESS.md §7.1 требует эти два раздела первыми, отдельно от «Проблема».
+Все восемь других сверенных ТЗ репозитория (`107`, `122`, `123`, `131`,
+`137`, `138`, `141`, `146`) оформляют их отдельными заголовками. В
+`113-optional-space-model.md` содержание фактически присутствует —
+§1 называет причину и предшествующий инцидент (#111), §9 фиксирует, что
+видимое поведение не меняется, — но не сформулировано как явный ответ на
+«кто/где/когда» и «одна фраза до/после». Прецедент `SPEC-REVIEW-107-r1`
+(Low-1: «раздел «Проблема» не выделен отдельным заголовком») фиксирует тот
+же класс находки как некритичный, если содержание по существу
+присутствует.
+
+**Решение ревьюера:** Low, не блокирует. Можно поправить в следующей
+редакции (например, короткая явная фраза: «Персона — любой пользователь
+View/editors при удалении пространства или холодном старте с `spaces: []`;
+до — риск исключения при будущей правке порядка вызовов (класс #111),
+после — тот же надёжный empty-state, без видимых изменений»), либо снять
+записью в этом документе, если автор сочтёт §1 достаточным.
+
+### Low-2 — AC1…AC10 не несут явной пометки способа доказательства
+
+**Файл:** `docs/specs/113-optional-space-model.md:170-182` (§10)
+
+DoR (`PROCESS.md` §2.5) и §7.1 требуют «у каждого [AC] указано, чем он
+доказывается: unit / backend / smoke / golden / «ревью кода»». Ни один из
+десяти пунктов §10 такой пометки не несёт — способ доказательства
+приходится реконструировать по §11 (план автотестов), что для девяти из
+десяти AC делается однозначно (например, AC3/AC4/AC5/AC6 — browser smoke,
+AC1/AC2/AC10 — typecheck + source-contract test, AC7 — golden/существующий
+скриншот, AC9 — unit), но для AC8 реконструкция не удаётся структурно (см.
+Medium-1). Прецедент `SPEC-REVIEW-141-r1` (Low-3) относился к похожему, но
+более мягкому случаю (формулировка доказательства вне буквального
+перечня, а не полное отсутствие) и не блокировал.
+
+**Решение ревьюера:** Low, не блокирует. При следующей правке ТЗ
+рекомендуется приписать к каждому пункту §10 короткую пометку в скобках
+(`(smoke)`, `(unit)`, `(typecheck+source-contract)` и т.п.) — механическая
+правка без изменения контракта.
+
+### Low-3 — нет буквальной метки touch-контракта по `docs/TOUCH-SUPPORT.md`
+
+**Файл:** `docs/specs/113-optional-space-model.md:161-168` (§9)
+
+`docs/TOUCH-SUPPORT.md` требует одну из трёх формулировок (`Touch editor:
+supported` / `best effort / intentionally degraded` / `not exposed`) для
+спецификаций, затрагивающих поведение редакторов. §9 по существу описывает
+best-effort/unchanged-контракт («editor touch safety floor не меняется»),
+но не использует канонический ярлык. Тот же класс находки зафиксирован как
+Low и не блокировал в `SPEC-REVIEW-141-r1` (Low-1).
+
+**Решение ревьюера:** Low, не блокирует. Косметическая правка на усмотрение
+автора (добавить строку `Touch editor: best effort / intentionally
+degraded (unchanged)`).
+
+## Что проверено и корректно
+
+- **Соответствие `docs/SCOPE.md`:** задача закрывает J6 («план остаётся
+  правдивым по мере развития» — плановые правки не должны ронять карточку
+  при пустом плане) как профилактика, не расширяя продуктовую поверхность;
+  User-Visible: no подтверждён и содержанием ТЗ (§13), и отсутствием любых
+  UX/i18n/визуальных изменений в контракте.
+- **Легитимность полного трека:** сложность/риск 6/10 и 7/10 превышают
+  порог `small` (≤3) — полный трек и файл `docs/specs/` выбраны верно,
+  issue корректно НЕ помечен `small`.
+- **Технический диагноз не голословен.** Число полевых обращений
+  `_spaceModel().<поле>`, факт единственного гейта в `render()`, порядок
+  `willUpdate`/`updated` относительно `render()`, факт фикса #111 через
+  явный ранний `return false` — всё подтверждено чтением
+  `src/houseplan-card.ts`, а не пересказом issue.
+- **Классификация call sites (§5) полна и специфична для этой кодовой
+  базы:** default-parameter паттерн (`space = this._spaceModel()`) назван
+  и подтверждён существующим в трёх методах; onboarding-гейт `updated()`
+  (`this._model.length === 0`) как пример уже существующей ручной
+  дисциплины, которую ТЗ формализует типом.
+- **Запреты (§4) конкретны и адресуют реальный анти-паттерн:** явный запрет
+  на synthetic empty `SpaceModel`, на `!` у каждого consumer, на скрытый
+  fallback через `this._serverCfg.spaces[0]` — не общие слова, а прямая
+  реакция на то, как решались подобные проблемы в других частях кодовой
+  базы.
+- **Продуктовые вопросы закрыты по процессу:** задача целиком техническая
+  (tech-debt, «guard patterns»), аналитик и автор корректно не эскалировали
+  ничего владельцу; ни один вопрос, вынесенный бы во владельцу, на самом
+  деле не был продуктовым — сверено построчно, эскалаций нет вообще.
+- **Трейлеры и класс файлов:** коммит `5f02dd3` — только `docs/specs/**`
+  (класс C), `Issue: #113`, `User-Visible: no` — корректно для документа
+  ТЗ; ссылка issue ↔ ТЗ двусторонняя (issue → комментарий со ссылкой на
+  файл; `docs/specs/README.md:85` → issue).
+- **Release-артефакты не выдуманы:** `docs/ARCHITECTURE.md` и
+  `docs/TESTING.md`, которые §13 обязывает дополнить, существуют в
+  репозитории.
+
+## Чего не проверял
+
+- Не проверял реализуемость предложенного разделения `_spaceModel()` /
+  `_spaceModelById()` как факта работающего TypeScript-кода — на этапе ТЗ
+  реализации ещё нет, это предмет код-ревью.
+- Не запускал автотесты, не собирал бандл и не проверял backend — на этапе
+  `spec` продуктовый код не менялся (подтверждено `git show --stat`), гейты
+  из §8 PROCESS.md здесь неприменимы.
+- Не проверял полноту всех ~46 call sites `_spaceModel()` по отдельности —
+  проверена корректность классификации (§5 ТЗ) на представительной выборке
+  (lifecycle-хуки, default-parameter паттерн, explicit-id команды), а не
+  построчный аудит всех вхождений; это ожидаемо станет предметом код-ревью,
+  когда появится diff.
+- Не проверял `docs/TESTING.md` на предмет того, легко ли туда встроить
+  «empty-plan lifecycle matrix» — детали документации оставлены на
+  усмотрение автора реализации (класс C, не влияет на продуктовый контракт).
+- Не оценивал производительность предложенных изменений — ТЗ (§13) явно и
+  обоснованно откладывает performance-гейт до случая, когда diff
+  действительно заденет горячие render-пути; на этапе спецификации это не
+  проверяется.
+
+## Вердикт
+
+Зелёный. High: 0, Medium: 1 (заведён отдельным issue, не блокирует), Low: 3
+(отсутствие отдельных заголовков «Сценарий»/«Что человек увидит»; отсутствие
+явной пометки доказательства у AC1…AC10; отсутствие буквальной touch-метки
+— все три косметические, содержание по существу присутствует, не блокируют
+приёмку). ТЗ решает заявленную проблему (#111-класс дефектов) для
+lifecycle/render/cleanup путей полно и проверяемо; единственный содержательный
+пробел (Medium-1, explicit-id command paths) не отменяет ценность и
+корректность контракта для основного объёма из ~46 call sites, но должен
+быть закрыт до или во время реализации — что и обеспечивает заведённый
+issue.