From 135497b2727bfe93a215a353f169eb4a3904e567 Mon Sep 17 00:00:00 2001 From: "claude[bot]" <209825114+claude[bot]@users.noreply.github.com> Date: Tue, 18 Aug 2026 19:36:12 +0000 Subject: [PATCH] docs: review document for #113 Issue: #113 User-Visible: no --- docs/reviews/SPEC-REVIEW-113-r1.md | 328 +++++++++++++++++++++++++++++ 1 file changed, 328 insertions(+) create mode 100644 docs/reviews/SPEC-REVIEW-113-r1.md diff --git a/docs/reviews/SPEC-REVIEW-113-r1.md b/docs/reviews/SPEC-REVIEW-113-r1.md new file mode 100644 index 00000000..5677dd45 --- /dev/null +++ b/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.