diff --git a/docs/reviews/SPEC-REVIEW-131-r1.md b/docs/reviews/SPEC-REVIEW-131-r1.md new file mode 100644 index 00000000..b5f57b69 --- /dev/null +++ b/docs/reviews/SPEC-REVIEW-131-r1.md @@ -0,0 +1,226 @@ +# SPEC-REVIEW-131-r1 + +- **Issue:** https://github.com/Matysh/houseplan-card/issues/131 +- **ТЗ под ревью:** `docs/specs/131-readonly-cold-start.md` (коммит `0164e65`) +- **Роль:** ревьюер ТЗ (не автор), этап `S4-spec-review` +- **Трек:** обычный (не `small`/`trivial`) — владелец явно принял D1/D2 + (оценка сложности 4/10, но затрагивает async boot, cache/navigation + precedence, reload и warm continuity; лёгкий трек корректно не применён) +- **Цикл:** r1/4 + +## Скоуп ревью + +Проверялось соответствие ТЗ: + +- `docs/SCOPE.md` — попадание в Core user jobs, отсутствие расширения скоупа, + сохранение lock-инварианта и правила «никогда не удалять файл по догадке» + (задача файлов не касается); +- `PROCESS.md` §2.4, §2.5 (DoR), §7.1 (обязательные разделы), §12 (запреты); +- `AGENTS.md` — классы файлов (класс A: `src/houseplan-card.ts`; класс B: + тесты/demo; класс C: документация/changelog), ветка `issue/131-readonly-cold-start`, + трейлеры; +- `docs/TOUCH-SUPPORT.md` — View/kiosk как гарантированные touch-поверхности; +- `docs/USER-GUIDE.ru.md` — терминология «пространство», «вкладка», «киоск»; +- фактическому состоянию кода `src/houseplan-card.ts` — на предмет того, что + технические утверждения ТЗ (диагноз причины, поведение hash/`LS_NAV`/warm + viewport, no-op клика активной вкладки) не являются непроверенной догадкой, + а описывают код, который действительно существует. + +## Как проверялось + +1. Прочитан весь тред issue #131: исходный отчёт с двумя скриншотами, + аналитика S2 (таблица диагностики на `dev` SHA `0e69c4a18337`, два + пространства `home`/`upstairs`), решение владельца о defaults D1–D3 + (https://github.com/Matysh/houseplan-card/issues/131#issuecomment-5287280361), + комментарий «Взял» и комментарий «ТЗ готово». +2. Сверены обязательные разделы ТЗ (§7.1 `PROCESS.md`) построчно — таблица ниже. +3. Прочитан `setConfig()` (`src/houseplan-card.ts:2258-2321`): подтверждено, что + `default_floor` и cache-приоритет (`hash → LS_NAV → default_floor → model[0]`) + применяются здесь только когда есть валидный `LS_CFG`; без кэша `_space` + остаётся дефолтным `'f1'` до серверного ответа — совпадает с §3 ТЗ. +4. Прочитан `_loadFromServer()` (`src/houseplan-card.ts:3193-3316`): подтверждено + дословно то, что описывает §3/§8 ТЗ — + - `_adoptStructuralResponses()` (принятие config/layout/`can_write`) вызывается + **до** трёх последовательных `await this.hass.connection.subscribeEvents(...)` + (`houseplan_config_updated`, `houseplan_trail_updated`, `houseplan_layout_updated`); + - выбор `_space` по hash/`LS_NAV`/`_norm`-fallback и `_cacheSnapshot()` + находятся **после** этих трёх `await`, внутри того же `try`; + - отклонённый `subscribeEvents` уходит во внешний `catch`, который при уже + установленном `_serverCfg` вызывает `_scheduleLoadRetry(true)` — то есть + ошибка необязательной подписки трактуется как отказ всей загрузки и + провоцирует непрерывный retry. Это ровно то, что ТЗ называет в §3 п.4 и + в риске §14.2. +5. Прочитан `_warmAdoptViewport()` (`src/houseplan-card.ts:2451-2499`) и + окружающие флаги `_hashApplied`/`_navApplied` (`:1521-1522`, `:2296-2299`, + `:2466`, `:3266-3277`): подтверждено, что explicit `#space=` hash уже сегодня + выигрывает у warm-viewport (`if (this._hashApplied || …) { this._warmVp = null; return; }`), + а принятый warm viewport не сбрасывается менее точным `LS_NAV`/`default_floor` + (`_navApplied = true` защищает ветку в `_loadFromServer`). Формулировка §7.2 + ТЗ об этом взаимодействии технически точна, а не придумана заново. +6. Прочитан `_pickSpace()` (`src/houseplan-card.ts:1087-1097`): `if (id === this._space) return;` + — клик по уже активной вкладке уже сегодня no-op. AC4 ТЗ («после исправления + клик активной вкладки — no-op») не вводит новое поведение клика, а лишь + требует, чтобы `_space` корректно совпадал с реальным выбором к моменту клика. +7. Прочитана генерация id пространства (`src/houseplan-card.ts:12067`, + `spaceId = 's' + Date.now().toString(36)`): реальные id никогда не выглядят + как `f1`/`f2`, то есть легаси-дефолт `_space = 'f1'` (`:611`) — синтетический + сентинел, а не потенциально валидный id; диагностическая таблица ТЗ с + `home`/`upstairs` репрезентативна, коллизии не подразумевается. +8. Проверено `docs/CONFIG-COMPATIBILITY.md` — ни `LS_CFG`, ни server-config + schema там не упомянуты в связи с этим изменением; заявление ТЗ §9 «миграция + не нужна» не противоречит канону. +9. Проверено существующее browser-smoke покрытие: `demo/smoke_warm_remount.mjs`, + `demo/smoke_kiosk.mjs`, `demo/smoke_warm_owners.mjs`, `demo/smoke_warm_dialogs.mjs` + реально существуют — предположение §17 п.5 («smoke может расширить + существующий WS/warm lifecycle сценарий») опирается на реальную + инфраструктуру, а не на вымышленный файл. +10. Проверена запись в `docs/specs/README.md` — строка на #131 добавлена в том + же коммите `0164e65`, ссылка issue ↔ ТЗ двусторонняя. +11. Сверена терминология с `docs/USER-GUIDE.ru.md` (§7 «Пространства», §17 + «Киоск-режим», обычный просмотр «не отдельная вкладка») — ТЗ использует + «пространство», «вкладка», «киоск» ровно в этом значении, ничего не + изобретает. +12. Сверено с `docs/TOUCH-SUPPORT.md` — «View is fully supported/must be + convenient and reliable» и «kiosk is the primary supported environment»: + формулировка ТЗ §10 «View на touch блокирующий… в киоске требование ещё + строже» дословно соответствует канону, а не собственная эскалация автора. + +## Обязательные разделы (§7.1 PROCESS.md) + +| Раздел | Есть | Комментарий | +|---|---|---| +| Сценарий (персона/поверхность/момент) | ✅ | §1 | +| Что человек увидит до/после (без терминов реализации) | ✅ | §2, одной фразой каждое состояние | +| Проблема | ✅ | §3, с подтверждённой диагностикой и таблицей на конкретном SHA | +| Скоуп / не-скоуп | ✅ | §5 / §6 | +| Контракт поведения | ✅ | §7 (инвариант, приоритет, mandatory/optional, деградация, reload/cache) | +| UX / i18n / accessibility / touch | ✅ | §10 | +| Модель данных и миграция | ✅ | §9 | +| AC1…ACn с доказательством | ✅ | §11, 12 штук, каждый с типом (`unit`/`smoke`/«ревью кода»/`build`) | +| План автотестов | ✅ | §12 | +| Риски | ✅ | §14, 5 пунктов, каждый со ссылкой на закрывающий AC | +| Откат | ✅ | §16 | +| Release-артефакты | ✅ | §15 | + +Все обязательные разделы присутствуют и содержательны. Дополнительно есть +раздел §8 «Архитектурный контракт реализации» и §17 «Принятые технические +предположения» — оба корректно отделены от продуктового контракта. + +## Находки + +Находок уровня **High** и **Medium** нет. + +### Low-1 — плотная формулировка §7.2 про hash/warm-viewport взаимодействие + +**Файл:** `docs/specs/131-readonly-cold-start.md:146-160` + +Абзац описывает две ветки (без принятого warm viewport / с ним) одним плотным +текстом: «применяется существующий порядок… Explicit hash по-прежнему +выигрывает. Уже принятый валидный same-route warm viewport остаётся +существующим continuity-исключением… и не сбрасывается менее точным +saved/default значением». При беглом чтении можно ошибочно понять, что +`explicit hash` всегда переопределяет уже принятый warm viewport — в +реальности (проверено чтением `_warmAdoptViewport`, `src/houseplan-card.ts:2466`) +hash блокирует **принятие** нового warm viewport, но не выбивает уже +принятый в текущем цикле рендера; вторая фраза говорит о независимой ветке +«when warm viewport already accepted», а не о конкуренции с первой веткой в +рамках одного и того же кадра. Технически формулировка точна (я сверил её с +кодом и противоречия не нашёл), но растянута до состояния, что имплементатор +может перечитать её как конфликт правил. + +**Почему не блокирует:** AC1/AC5/AC6 explicitly не расширяют матрицу на +конкуренцию «hash vs уже принятый warm viewport» — этот случай прямо в +не-скоупе (§6: «изменение выбора пространства… deep link… не входит»), то +есть задача обязана лишь не сломать существующее поведение, а не +переописывать его заново. Формулировка описывает уже существующий, а не +новый контракт, и не создаёт риска неверной реализации, потому что §8 п.2 +и риск §14.1 уже требуют «одного resolver» и защищают его тем же AC1/AC5/AC6. + +**Решение ревьюера:** Low, не блокирует. На усмотрение автора — можно +перефразировать двумя явными пунктами («без warm viewport: …», «warm viewport +уже принят: …») при следующей правке или оставить как есть. + +## Что проверено и корректно + +- Соответствие `docs/SCOPE.md`: задача закрывает J1 («план должен быть читаемым + и полным сразу при открытии») и J6-грань «reload/reconnect/техническое + перемонтирование не должны менять состав видимого плана» — регрессия внутри + уже закрытых Core user jobs, не новая функциональность и не расширение + скоупа; ни один пункт «Out of scope» не задет. +- Гарантированный View/touch/kiosk-контракт (`docs/TOUCH-SUPPORT.md`) учтён + верно и процитирован дословно, не переизобретён. +- Владелец лично принял defaults D1–D3 и приоритет P1 (комментарий + 2026-08-14) — открытых продуктовых вопросов в финальной редакции ТЗ нет, и + это корректно: вопросы (триггер cold start vs прав, поведение при + нескольких пространствах, переживание reload) были заданы и закрыты на + этапе аналитики диагностическим прогоном, а не додуманы автором ТЗ. + Раздел §17 «Принятые технические предположения» отделяет свободно + изменяемые технические решения (имя resolver'а, форма orchestration + подписок, механизм retry, файл browser smoke) от продуктовых решений D1–D3, + которые пересмотру не подлежат — ни одна догадка не выдана за факт без + пометки. +- Технический диагноз причины (порядок `_adoptStructuralResponses` → три + `await subscribeEvents` → выбор пространства/`_cacheSnapshot`, отказ + подписки уходит во внешний `catch` и триггерит `_scheduleLoadRetry(true)`) + подтверждён построчным чтением `_loadFromServer()` — не голословное + утверждение автора, а точное описание существующего кода. +- Поведение hash/`LS_NAV`/warm-viewport и no-op клика активной вкладки + (AC4) подтверждено чтением `setConfig`, `_warmAdoptViewport`, `_pickSpace` + — расхождений с ТЗ не найдено (см. Low-1 только по ясности формулировки, + не по фактической корректности). +- AC1–AC12 однозначны, у каждого указан тип доказательства из допустимого по + DoR перечня (`unit`/`smoke`/«ревью кода»/`build`); план автотестов (§12) + даёт конкретный маршрут, включая явное требование покрыть матрицу + valid/stale hash × `LS_NAV` × `default_floor` × 0/1/N пространств (AC1) и + проверять «умеет падать» через фиксированный HA snapshot до/после no-op + клика (AC4). +- Не-скоуп (§6) корректно отсекает смежные соблазны: не менять backend + permissions/websocket API, не добавлять новый toast/recovery overlay, не + трогать порядок вкладок/deep-link/формат cache, не менять + `houseplan-space-card`, не переписывать весь boot lifecycle — типичные + места, где скоуп мог бы незаметно расшириться из-за близости к async boot. +- Раздел §13 (Производительность и security) обоснованно закрывает вопрос + «не ослабляет ли исправление read-only-границу»: подписка на запрещённые + события не эмулируется, write API не вызывается — прямо отражает + требование AC10. +- Release-артефакты (§15) перечисляют реальные файлы: + `docs/CHANGELOG.md`/`docs/CHANGELOG.ru.md` (существуют), `docs/ARCHITECTURE.md` + (существует), `test/*.test.mjs` и `demo/smoke_*.mjs` (существующая + инфраструктура, см. «Как проверялось» п.9), три bundle snapshot + (существующая практика синхронизации по `AGENTS.md`). +- Реестр `docs/specs/README.md` обновлён тем же коммитом, ссылка issue ↔ ТЗ + двусторонняя (`PROCESS.md` §7.1). +- Трейлеры коммита `0164e65` (`Issue: #131`, `User-Visible: no`) корректны: + изменение — только документация ТЗ, продуктовый код не тронут, что явно + подтверждено и в тексте самого ТЗ, и в комментарии «ТЗ готово». + +## Чего не проверял + +- Не проверял, что предложенная в §8 архитектурная декомпозиция (единый pure + resolver cold precedence) реализуема без побочных эффектов на смежные поля + `_hashApplied`/`_navApplied`/`_warmVpArmed` — по правилам ТЗ (§17 п.1–2) это + свободно изменяемое техническое предположение автора кода, предмет + код-ревью, а не ревью ТЗ. +- Не запускал никаких автотестов и не собирал бандл — на этапе `spec` это не + требуется; существование упомянутых тестовых файлов и smoke-инфраструктуры + проверено чтением файловой системы, не исполнением. +- Не проверял golden/скриншоты — ТЗ §12 явно и обоснованно заявляет их + ненужность (новый визуал совпадает с уже принятым состоянием после клика); + это будущий предмет пре-релизного гейта, не ревью ТЗ. +- Не проверял поведение `houseplan-space-card` (статическая карточка) — + явно вынесено в не-скоуп §6, и это верно: она получает пространство + отдельным обязательным параметром, а не через выбор, который чинит эта + задача. +- Не проверял, действительно ли трёх-подписочный orchestration в текущем + коде уже идемпотентен при повторном вызове `_loadFromServer` после отказа + (авторитет `_unsubCfg`/`_unsubTrail`/`_unsubLayout`, AC7/AC11) — + прочитал структуру `if (!this._unsubX) { this._unsubX = await … }` + (`src/houseplan-card.ts:3232-3263`) и она выглядит согласованной с + требованием, но полное покрытие гонок (двойной параллельный вызов + `_loadFromServer`) — предмет код-ревью на реализованном коде, не ревью ТЗ. + +## Вердикт + +Зелёный. High: 0, Medium: 0. Одна находка Low (плотность формулировки §7.2 +про hash/warm-viewport) — не блокирует, технически формулировка проверена и +корректна, оставлена автору на усмотрение с записью в этом документе.