From c08d5a88aee443ac116b96b0fe4ca69a863e7eea Mon Sep 17 00:00:00 2001 From: "claude[bot]" <41898282+claude[bot]@users.noreply.github.com> Date: Wed, 19 Aug 2026 22:21:08 +0000 Subject: [PATCH] docs: spec review for #210 Issue: #210 User-Visible: no --- docs/reviews/SPEC-REVIEW-210-r1.md | 212 +++++++++++++++++++++++++++++ 1 file changed, 212 insertions(+) create mode 100644 docs/reviews/SPEC-REVIEW-210-r1.md diff --git a/docs/reviews/SPEC-REVIEW-210-r1.md b/docs/reviews/SPEC-REVIEW-210-r1.md new file mode 100644 index 00000000..696634ad --- /dev/null +++ b/docs/reviews/SPEC-REVIEW-210-r1.md @@ -0,0 +1,212 @@ +# Ревью ТЗ — issue #210, цикл r1 + +- **Issue:** https://github.com/Matysh/houseplan-card/issues/210 +- **ТЗ:** `docs/specs/210-fixed-floor-card.md` (коммит `0bd6094`, ветка + `issue/210-fixed-floor`) +- **Трек:** обычный (аналитика явно исключила `small`/`trivial`; сложность + 6/10, риск 6/10, метка `small` на issue отсутствует) +- **Вердикт:** жёлтый · цикл r1/4 · High: 0 · Medium: 1 (в скоупе задачи) · + Low: 1 + +## Скоуп ревью + +Проверялось ТЗ `docs/specs/210-fixed-floor-card.md` как артефакт этапа +`S4-spec-review`: обязательные разделы §7.1 PROCESS.md, однозначность и +доказуемость AC1…AC12, соответствие `docs/SCOPE.md`, `docs/UX-MODES.md`, +`docs/TOUCH-SUPPORT.md`, `docs/CONFIG-COMPATIBILITY.md` и +`docs/USER-GUIDE.ru.md`, а также фактическая проверка причинных утверждений +ТЗ (§3, §7) против текущей реализации в `src/initial-load.ts`, +`src/houseplan-card.ts` и `src/editor.ts` — то есть не пересказ автора, а +независимое чтение кода, который ТЗ описывает как «до». Продуктового кода по +#210 не существует (issue в `S4-spec-review`, диапазон коммитов веток — +только документация), поэтому гейты §8 PROCESS.md к этому циклу неприменимы. + +## Как проверялось + +1. Прочитаны `docs/SCOPE.md`, `AGENTS.md`, `PROCESS.md` целиком. +2. Прочитано тело issue #210 и все три комментария (аналитика, + «взял в работу», «ТЗ готово»). +3. Прочитан файл ТЗ `210-fixed-floor-card.md` целиком (327 строк). +4. Прочитаны `docs/CONFIG-COMPATIBILITY.md` и `docs/UX-MODES.md` целиком, а + также `docs/TOUCH-SUPPORT.md` целиком — как канонические документы + затронутых подсистем (навигация, конфигурация, touch/kiosk-контракт). +5. Прочитан `docs/USER-GUIDE.ru.md` в месте, описывающем `default_floor` и + несколько карточек (строки 122-125, 1327-1339) — подтверждает, что именно + это описание текущего (ошибочного) поведения ТЗ обязано заменить/уточнить + в release-артефактах (§15 ТЗ это учитывает). +6. Независимо сверены причинные утверждения ТЗ §3/§7 с кодом: + - `resolveInitialSpace()` (`src/initial-load.ts:27-42`) — подтверждён + буквальный холодный приоритет `hash → current → saved → default → first`; + - общий ключ `houseplan_card_nav_v1` (`LS_NAV`, `src/houseplan-card.ts:482`, + `_savedNav`/`_saveNav` :5866-5881) — подтверждён как единственный, + без per-instance scoping; + - `_warmAdoptViewport()` (`src/houseplan-card.ts:2625-2644`) — подтверждено + восстановление пространства из viewport-memo поверх saved/default; + - множественные прямые присваивания `this._space = …` вне единой точки + (grep дал >15 мест: :1150, :1188, :1820, :2430, :2635, :2925, :3179, + :13522, :13531, :13573, :13644, :14297, :16459, :16487 и др.) — + подтверждает риск «новый guard пропустит прямое `_space =`» из таблицы + §14 ТЗ как реальный, а не гипотетический; + - kiosk swipe (`src/houseplan-card.ts:5405`) и cycle (`:1191-1193`, + `:1951-1953`) — подтверждены как условные пути, которые можно + дополнительно закрыть проверкой fixed-authority без переписывания их + структуры; + - GUI editor (`src/editor.ts:42-73`) — подтверждён текущий паттерн + `default_floor` (dropdown со stable ID при доступных `spaces`, textbox + fallback при недоступности WS) как прямой прецедент для нового поля + `floor`, на который ссылается §6.2 ТЗ. +7. Проверено соответствие `docs/SCOPE.md`: задача закрывает возможность + «Home admin» держать несколько выделенных fullscreen/kiosk-панелей по + этажам без взаимного вмешательства навигации — часть уже закрытых J1/J6 + («live spatial overview», «keep the plan true», несколько карточек с + собственным стартовым этажом уже документировано как поддерживаемый + сценарий в `USER-GUIDE.ru.md:1327-1332`); задача не расширяет и не + нарушает ни одну строку scope. +8. Код не запускался, гейты не прогонялись — на этапе ревью ТЗ продуктового + кода нет. + +## Находки + +### M1 — контракт не говорит явно, что очистка `floor` в GUI должна удалять ключ, а не записывать невалидную пустую строку + +**Файл:** `docs/specs/210-fixed-floor-card.md:101-111` (§6.2), в противоречии +с `:92` (§6.1, п.3) и AC8 (`:239`) + +§6.1 п.3 явно объявляет пустую строку невалидным значением `floor` — при её +наличии в конфиге карточка обязана уйти в fail-closed error state (§9, AC3: +«Unknown ID и все невалидные типы/числа fail closed… без чужого +плана/fallback»). При этом §6.2 описывает GUI-контроль как «dropdown со +stable ID и пустым вариантом «не закреплять»», а AC8 требует, чтобы GUI «умел +очистить `floor`» — но ни §6.2, ни §7, ни AC8 не говорят явно, **что именно** +происходит в конфиге при выборе этого пустого варианта: должен ли +`floor`-ключ удаляться из объекта конфигурации целиком (что вернёт legacy +navigation, как того требует «не закреплять»), или контрол просто запишет +`floor: ''` (что по буквальной формулировке §6.1 п.3 является невалидным +значением и включит error state). + +Это не абстрактный риск: `_valueChanged()` в текущем `src/editor.ts:99-104` +делает `{ ...this._config, ...ev.detail.value }` — то есть механически +переносит то, что вернул `ha-form`, без специальной зачистки пустых полей; +никакого существующего прецедента «optional selector с явной очисткой ключа» +в этом редакторе сейчас нет (`default_floor` never being cleared — у него нет +пустого варианта). Если реализация просто повторит текущий паттерн, самое +частое и ожидаемое админом действие — «я больше не хочу закреплять этот +экран» — приведёт не к обычной навигации, а к видимой ошибке ровно того типа, +который вся задача обязана устранить. + +**Почему это Medium, а не High:** ни один AC не сформулирован неверно и не +описывает ошибочное поведение как целевое — пробел чисто в определении, какую +именно мутацию конфигурации должен произвести один конкретный UI-контрол; +это устраняется одной уточняющей фразой в тексте ТЗ, не меняя ни архитектуру, +ни один AC, ни скоуп. Находка в скоупе собственного AC8 задачи — правится в +текущем ТЗ. + +**Требуется:** явно зафиксировать в §6.2 (и, для симметрии, в §16 как принятое +техническое решение), что выбор пустого варианта удаляет ключ `floor` из +конфигурации целиком, а не заменяет его на пустую строку/`null` — то есть +GUI обязан обеспечивать инвариант §6.1 «отсутствует, только если свойства нет +в конфиге» на собственном пути записи, а не полагаться на то, что позже это +поле случайно совпадёт с одним из уже описанных invalid-путей. + +### L1 — обязательный раздел «i18n» присутствует по содержанию, но не оформлен отдельным заголовком + +**Файл:** `docs/specs/210-fixed-floor-card.md` (содержание разбросано по +§6.2 — подписи полей GUI на двух языках, и §9 — требования к тексту ошибки) + +PROCESS.md §7.1 перечисляет i18n отдельным обязательным разделом ТЗ. Формально +такого заголовка в документе нет: русские/английские подписи полей GUI даны +прямой строкой в §6.2 («Fixed space / Закреплённое пространство», «Initial +space / Стартовое пространство»), а требования к содержимому error-текста — +в §9, без перечисления конкретных ключей `en.json`/`ru.json`. Это не создаёт +двусмысленности для реализации (именование ключей — техническая деталь, +которую агенты решают сами, PROCESS.md §7.1) и не блокирует ни один AC, но +формально отклоняется от структуры, которую требует процесс, и усложняет +беглую проверку «оба языка учтены» без чтения всего документа целиком. + +Не блокирует. Снимается на решение автора — либо выделить короткий +одноимённый раздел (даже как ссылку на уже написанные §6.2/§9), либо оставить +как есть с этой пометкой в документе ревью. + +## Проверено и признано корректным + +- **Причина дефекта (§3)** — построчно подтверждена кодом: единый + `houseplan_card_nav_v1`, приоритет `resolveInitialSpace()`, влияние + `_warmAdoptViewport()`; это не пересказанная, а самостоятельно + верифицированная причинно-следственная цепочка. +- **Обязательные разделы §7.1 PROCESS.md** — все присутствуют по содержанию: + сценарий (§1), что видит пользователь до/после (§2, без терминов + реализации), проблема (§3), скоуп/не-скоуп (§4/§5), контракт поведения + (§6-§9), UX (§8), данные/совместимость/безопасность (§10), архитектурный + контракт (§11), AC1…AC12 с доказательством для каждого (§12), план + автотестов (§13), риски и откат (§14), release-артефакты (§15). +- **Продуктовая часть контракта не содана как догадка**: аналитика (комментарий + владельца) прямо сказала «продуктовых вопросов по заданному контракту нет», + и §16 «Принятые предположения» корректно отделяет технические решения (7 + пунктов) от уже отвеченного владельцем продуктового контракта, помечая + каждое как решение автора, открытое для оспаривания ревьюером — ни одно не + подано как установленный факт без основания. +- **Предположение №1 (§16)** — расширение «floor выше всех источников + навигации» на URL hash, хотя исходная формулировка issue перечисляла только + saved nav/tabs/swipe/cycle — признано обоснованным: это техническая деталь, + которую issue прямо разрешает решать на этапе ревью («Technical details are + assumed and may change freely»), и оно последовательно с продуктовым + утверждением «After: … не участвует в общей истории выбора этажей». +- **Риск «новый guard пропустит прямое `_space =`»** (§14 таблицы) — + подтверждён как реальный, не гипотетический: код действительно содержит + более 15 разрозненных прямых присваиваний `this._space =`; выбранная мера + («инвентаризация всех assignments + единая mutation boundary», §11) — + адекватный ответ. +- **Соответствие `docs/TOUCH-SUPPORT.md`** — §8 несёт буквальный канонический + тег `Touch editor: best effort / intentionally degraded.`, требуемый + «Documentation rule»; удаление kiosk swipe/cycle/dots для fixed-инстанса не + нарушает гарантию «View/kiosk — fully supported touch surface», поскольку + pan/pinch/zoom и безопасные действия текущего пространства прямо сохранены, + а «переключение между этажами» перестаёт быть релевантным действием там, + где второго этажа для переключения не предусмотрено конфигурацией. +- **Соответствие `docs/UX-MODES.md`** — правило «Header в View: только + вкладки пространств, счётчик устройств, zoom-кластер» и принцип «редакторы + не просачиваются в View» не нарушаются: §8 ТЗ убирает лишние + навигационные элементы (другие вкладки, Add, kiosk dots), но не трогает + editors/gear текущего пространства и не вводит новых интерактивных + поверхностей. +- **`docs/CONFIG-COMPATIBILITY.md`** — корректно не упомянут как файл для + правки: `floor` — Lovelace card config, а не персистентное серверное поле + House Plan (§10 ТЗ), и уже существующие card-level опции (`default_floor`, + `cycle`, `kiosk`) в этом реестре также не зарегистрированы — задача не + создаёт исключения из этого паттерна. +- **AC1…AC12** — каждый однозначен и указывает способ доказательства + (unit/browser smoke/DOM assertions/config refresh test), включая AC12 + («новый guard доказан failing-before-fix») — это прямое выполнение + требования code review «тест умеет падать» уже на этапе постановки ТЗ. +- **Совместимость (§10, §16 п.6)** — отсутствие `floor` не меняет ни один + существующий путь навигации; `default_floor` остаётся ровно тем полем, + которое описывает действующий `USER-GUIDE.ru.md`; невалидный `floor` прямо + не откатывается на `default_floor`, что снимает риск «молча показать чужой + этаж» — именно то, из-за чего заведён issue. +- **Откат (§14)** — единый revert без миграции хранилища; отсутствие поля + на старом фронтенде корректно описано как «fixed guarantee отсутствует», а + не как повреждение данных. + +## Чего не проверял + +- **Фактическая работоспособность будущей реализации** — на этапе ревью ТЗ + продуктового кода не существует; проверка ограничена текстом ТЗ и текущим + («до») кодом. +- **`ha-form`/`ha-selector` внутреннее поведение при очистке optional select** + не тестировалось эмпирически (нет собранной карточки с новым полем) — M1 + сформулирована как пробел в тексте ТЗ, а не как утверждение о конкретном + поведении HA-компонента. +- **`demo/smoke_fixed_floor.mjs`, `smoke_nav_persist.mjs`, `smoke_kiosk.mjs`** + не запускались — файл нового смока пока не существует (создаётся в + реализации), а существующие смоки не относятся к этому циклу (нет кода для + проверки). +- Гейты `typecheck`/`test`/`build`/smoke/golden не прогонялись — на этапе + ревью ТЗ раздел неприменим (PROCESS.md §8: гейты код-ревью, не ТЗ-ревью). + +## Итог + +Блокирующих находок нет. Единственный Medium (M1) устраняется точечной +правкой текста §6.2 (и, по желанию автора, зеркальным пунктом в §16) — без +изменения архитектуры, скоупа или какого-либо AC. Low (L1) — на решение +автора, можно снять с запиской без правки. Вердикт — жёлтый, возврат автору +в рамках текущего issue.