From ad9c6349019cf108eeb0ae6d312c42a71924e25b Mon Sep 17 00:00:00 2001 From: "claude[bot]" <209825114+claude[bot]@users.noreply.github.com> Date: Sat, 29 Aug 2026 18:46:52 +0000 Subject: [PATCH] docs: review document for #378 Issue: #378 User-Visible: no --- docs/reviews/SPEC-REVIEW-378-r1.md | 148 +++++++++++++++++++++++++++++ 1 file changed, 148 insertions(+) create mode 100644 docs/reviews/SPEC-REVIEW-378-r1.md diff --git a/docs/reviews/SPEC-REVIEW-378-r1.md b/docs/reviews/SPEC-REVIEW-378-r1.md new file mode 100644 index 00000000..05cb0f29 --- /dev/null +++ b/docs/reviews/SPEC-REVIEW-378-r1.md @@ -0,0 +1,148 @@ +# SPEC-REVIEW-378-r1 + +- **Issue:** https://github.com/Matysh/houseplan-card/issues/378 +- **Этап:** ТЗ на ревью (`S4-spec-review`), заход r1, блокирующих циклов израсходовано 0/4 +- **Артефакт ТЗ:** `docs/specs/378-value-face-source.md` +- **SHA материала:** `7d5d006cccb5eeb36f2ecc92dabd124b7d137e32` (ветка `issue/378-value-face-source`, HEAD на момент ревью) +- **Ревьюер:** Claude (роль «ревьюер ТЗ»), сессия без контекста автора +- **Первый раунд** — раздел «Унаследовано из r» неприменим, разбор полный. + +## Скоуп ревью + +Оценивался только этап ТЗ: тело issue #378, аналитический комментарий владельца +(2026-08-29T18:37:48Z) и файл `docs/specs/378-value-face-source.md` на коммите +`7d5d006c`. Продуктовый код не менялся и не оценивался как реализация — только +как источник фактов для проверки утверждений ТЗ (существуют ли названные +функции/поля, действительно ли устроено так, как описано). + +## Как проверялось + +Читал в порядке, предписанном процессом: + +1. `docs/SCOPE.md` — соответствие Core user jobs. Задача расширяет уже + закрытый J1/J5-механизм («лицо» устройства на плане), не создаёт новую + персону и не входит в out-of-scope список. Класс изменения (A, полный + трек) назван и обоснован верно: конфиг marker получает новое публичное + поле, задета миграция/reference seam, несколько renderer, touch-контракт — + именно тот критерий `small`, который называет сам автор как нарушенный. +2. `PROCESS.md` §2.3–2.5, §7.1, §5 — проверка обязательных разделов ТЗ и + выбора трека. +3. Тело issue #378 и оба комментария (аналитика, затем «ТЗ готово»). +4. `docs/USER-GUIDE.ru.md` (раздел «Четыре варианта „Отображение“» и «Бейдж + со значением рядом с устройством», строки ~1047–1113) — сверка + терминологии и уже задокументированного поведения. +5. `docs/DEVICE-PRESENTATION.md` — таблица решений F09–F12, S01–S15, + владение модулями (`device-presentation.ts`, `device-presentation-policy.ts`, + `device-face.ts`). +6. Код на SHA `7d5d006c` — не как реализация (её нет), а как проверка того, + что ТЗ не выдаёт догадку за факт: + - `src/device-value-badge.ts` целиком — `VALUE_BADGE_ATTRIBUTES`, + `valueBadgeCandidates()`, `valueBadgeSourceKey/FromKey()`, `resolveSource()`, + unavailable-контракт, кэш кандидатов; + - `src/device-presentation.ts:449-547` — `validStateValue()`, `resolveValue()`, + climate/power-gate/ambiguity эвристика, `signatureOf()`, + `presentationSourceSignature()`; + - `src/houseplan-editor-runtime.ts` (грепом) — существующий selector value + badge в диалоге (строки ~12586-12633), `innerValueSourceKey` (12122, + сравнение с `badgeSourceKey` для duplicate-hint, 12611), `_valueBadgeForBinding` + (11904-11922, сброс/рекомендация источника при смене binding); + - `custom_components/houseplan/validation.py:805-865` — + `validate_marker_value_badges`, доступные коды ошибок; + - `custom_components/houseplan/import_export.py:1090-1111, 1344-1391` — + rebind и space-transfer для `value_badge.source.ref`; + - `src/devices.ts:868-889` — `rewriteMarkerControlReferences()`, уже + переписывающий `value_badge.source.ref` при смене marker id; + - `docs/specs/README.md:116` — ссылка issue ↔ ТЗ на месте; + - четыре файла `src/i18n/*.json` — паритет локалей, на которые претендует ТЗ. + +Гейты (typecheck/test/build/golden и т.д.) на этом этапе не запускались: +кода нет, реализации нет, оценивать нечего. Это ожидаемо для этапа spec-review +и не является пропуском гейта. + +## Находки + +Блокирующих (High) и Medium-находок нет. + +**Low — не блокирует, снимаю с записью.** Контракт п.1.6 («При явной смене +HA binding в диалоге старый `value_source` сбрасывается в auto») асимметричен +уже существующему поведению соседнего селектора: `_valueBadgeForBinding()` +(`src/houseplan-editor-runtime.ts:11904-11922`) при смене binding не просто +сбрасывает `value_badge.source` в null, а вызывает `recommendedValueBadgeSource()` +и подставляет новую рекомендацию для только что привязанного устройства. ТЗ же +предписывает для `value_source` только сброс в auto, без аналогичной +рекомендации. Это может привести к тому, что при рebind марказы на другой cover +бейдж рядом получит разумную рекомендацию (`current_position`), а лицо значения +откатится к слову состояния — ровно к той проблеме, которую issue закрывает. +Решение не ошибочно (объяснение «перенос источника прежнего устройства был бы +ложной настройкой» состоятельно и последовательно с общим принципом «явный +источник не заменяется автоматически»), и это техническое, не продуктовое +решение (`PROCESS.md` §7.1: где строится дефолт после rebind — не то, что +спрашивают владельца). Оставляю как решение автора, но фиксирую для внимания +на код-ревью AC8: если реализация всё же скопирует `recommendedValueBadgeSource`- +паттерн для симметрии, это не должно расцениваться как отход от ТЗ. + +## Что проверено и корректно + +- **Полнота обязательных разделов §7.1**: сценарий, что человек увидит до/после, + проблема, скоуп/не-скоуп, контракт поведения, UX, модель данных и миграция, + i18n, AC1–AC10 с доказательством, план автотестов, риски, откат, + release-артефакты — присутствуют все, в этом порядке. +- **Продуктовые разделы отвечают на два обязательных вопроса**: персона + (администратор дома, desktop, диалог устройства), поверхность и момент + встречи, а также «что человек увидит до/после» одной фразой без терминов + реализации (`Open` → `42 %`) — до раздела с реализацией, как требует канон. +- **Выбор полного трека обоснован явно** названным нарушенным критерием + (публичное поле конфига + reference seam + несколько renderer + touch), + а не общим «сложно». +- **Ни одного факта не оказалось выдумкой.** Каждое техническое утверждение + ТЗ проверено против реального кода на SHA `7d5d006c` и подтвердилось: + - `VALUE_BADGE_ATTRIBUTES`, `valueBadgeCandidates()`, unavailable-контракт — + существуют буквально как описаны; + - общий resolver/formatter для лица и бейджа — реалистичная цель: уже есть + частичное сближение (`innerValueSourceKey` в редакторе уже сравнивается + с ключом бейджа для duplicate-hint, т.е. инфраструктура для п.2.8 контракта + существует, а не изобретается с нуля); + - reference seam для `derived_marker_state` (`rewriteMarkerControlReferences`, + backend `import_export.py` rebind/space-transfer) для `value_badge.source.ref` + уже реализован тем самым паттерном, который ТЗ предлагает распространить на + `value_source.ref` — это не новый механизм, а его вторая точка подключения; + - `resolveDevicePresentation()` действительно используется и основным планом, + и `space-card.ts`/`space-render.ts` — утверждение AC2 «полный план и static + space card показывают одно и то же» технически обосновано общим вызовом; + - `d.virtual` gate в `resolveValue()` срабатывает раньше любого другого + условия — контракт п.2.6 («virtual имеет приоритет над сохранённым + источником») соответствует реальному порядку проверок, изменений в этой + ветке не требуется; + - F09–F12 и связанные ID из `docs/DEVICE-PRESENTATION.md` использованы точно, + без придумывания несуществующих строк таблицы; + - i18n-паритет заявлен для ровно тех четырёх локалей, что есть в дереве + (`en/ru/de/fr`), лишних/недостающих не названо. +- **Не-скоуп совпадает 1:1** с разделом «Не входит» тела issue: новый display + mode, изменение самого списка атрибутов, изменение бейджа, перенацеливание + tap-действия — везде явно исключено и не просачивается в контракт. +- **Каждый AC называет способ доказательства** (unit/backend/smoke/golden/ + docs gate/commands), ни одного голого «проверено». +- **Откат безопасен и объяснён на уровне данных**: старый frontend/backend с + `ALLOW_EXTRA` пропустит поле, данные не теряются — соответствует модели + `docs/CONFIG-COMPATIBILITY.md` (lossless / untouched future literal). +- **Открытых продуктовых вопросов нет**, и это обоснованно: сценарий, объём + видимого изменения и границы деградации (unavailable → `—`, virtual → + икона) зафиксированы однозначно самим ТЗ, а не додуманы — раздел «Принято + предположительно» содержит только техническое (имя поля, извлечение модуля, + структура smoke-теста), что и требуется §7.1. + +## Чего не проверял + +- Не запускал `npx tsc --noEmit` / `npm test` / `npm run build` — на этапе + spec-review код ещё не написан, гейты неприменимы. +- Не проверял `MARKER_SCHEMA` (`validation.py:1730`) построчно на предмет того, + как именно будет описан `value_source` в схеме — это код-ревью реализации, + не предмет ТЗ. +- Не оценивал производительность нового обхода `valueBadgeCandidates()` в + реальном большом доме — раздел «Производительность» в ТЗ ссылается на + существующий кэш и budget-гейт, этого достаточно для этапа spec. +- Не проверял golden/скриншоты — они не существуют до реализации. + +## Вердикт + +Вердикт: зелёный · заход r1 · блокирующих циклов 0/4 · High: 0 · Medium: 0 → в задаче