mirror of
https://github.com/Matysh/houseplan-card
synced 2026-09-29 03:09:36 +00:00
claude[bot]
committed by
Sergey Matyunin
Sergey Matyunin
parent
a851c8f958
commit
ad9c634901
@@ -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<N-1>» неприменим, разбор полный.
|
||||
|
||||
## Скоуп ревью
|
||||
|
||||
Оценивался только этап ТЗ: тело 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 → в задаче
|
||||
Reference in New Issue
Block a user