houseplan-card/docs/reviews/SPEC-REVIEW-377-r1.md

SPEC-REVIEW-377-r1

• Issue: https://github.com/Matysh/houseplan-card/issues/377 — «Персист цвета декора по умолчанию в серверный конфиг»
• Этап: ТЗ на ревью (PROCESS.md §2.4), полный трек (выделено из #376в по SPEC-REVIEW-376-r1 H1: новый персистентный ключ settings.decor_default_style
  • кросс-модульный путь записи — критерий §5 «нет новых compatibility-полей» не проходит)
• ТЗ: docs/specs/377-decor-default-persist.md, ревизия 1, зафиксирована комментарием автора на dev e2e4cec1
• Материал ревью: тело issue #377 + оба комментария (аналитика §2.2, объявление ревизии 1) сверены с dev на e2e4cec1
• Заход: r1 · блокирующих циклов израсходовано 0 из 4 (полный трек)

Скоуп ревью

Первый заход — предыдущего вердикта по #377 нет, разбор полный. Проверялось: соответствие docs/SCOPE.md; корректность классификации полного трека (уже установлена предшествующим SPEC-REVIEW-376-r1, проверена повторно на согласованность); наличие всех обязательных разделов §7.1; однозначность и доказуемость каждого AC; отсутствие догадок, выданных за факт — каждое техническое утверждение ТЗ о текущем коде сверено построчно с исходниками, а не принято на слово.

Диапазон правки этого раунда — файл docs/specs/377-decor-default-persist.md (добавлен, продуктовый код не менялся): git diff --stat 4eede5bc..e2e4cec1 показывает только два review-документа (#375, #376) и новый файл ТЗ.

Как проверялось

1. Прочитаны целиком docs/SCOPE.md, PROCESS.md (§2.4, §2.9/§4 бюджет циклов, §5 критерии трека, §7.1 обязательные разделы, §12 запреты), сверены с сохранённым в памяти пониманием процесса.
2. Прочитано тело issue #377 и оба комментария (аналитика, объявление ревизии).
3. Прочитан весь текст docs/specs/377-decor-default-persist.md.
4. Прочитан предшествующий docs/reviews/SPEC-REVIEW-376-r1.md целиком — проверить, что классификация полного трека и содержание находок этого документа корректно унаследованы новым ТЗ, а не переизобретены.
5. Каждое фактическое утверждение ТЗ о текущем коде сверено с dev:
   • src/houseplan-card.ts:984 — private _decorStyle: DecorStyle = { ...DEFAULT_DECOR_STYLE } — подтверждено, только in-memory поле.
   • src/houseplan-editor-runtime.ts:4481, 5306-5419 — оба места записи (тулбар и контекстный трей) действительно пишут в один и тот же this.host._decorStyle — подтверждено.
   • src/houseplan-editor-runtime.ts:1764 — _writeConfig() сериализованный путь записи, использующий enqueueSerializedWrite/_sendConfigCandidate — подтверждено, это общий канал, новый не создаётся.
   • src/houseplan-editor-runtime.ts:9403-9417 (_saveSettingsDialog) — подтверждён паттерн «дефолт = отсутствие ключа» для bg_color/ fill_colors, на который ссылается контракт п.5.
   • custom_components/houseplan/validation.py:1883 — vol.Optional( "settings", default=dict): vol.Schema({...}, extra=vol.ALLOW_EXTRA) — номер строки и ALLOW_EXTRA подтверждены дословно; используется как основание раздела «Откат» (старый бэкенд/фронт не ломается на новом ключе) — корректно.
   • custom_components/houseplan/validation.py:1307-1330 (_DECOR_COMMON, rect/ellipse) — диапазоны, предложенные для схемы decor_default_style (opacity 0..1, width_cm 0.1..100, fill_opacity 0..1), дословно совпадают с уже принятыми диапазонами для decor-фигур — не придуманы заново.
   • custom_components/houseplan/import_export.py:1145-1168 (build_space_merge) и :1660-1670 (полный импорт) — подтверждено: пространственный импорт копирует target_config = _json_copy(current_config) и не трогает глобальный settings, полный импорт переносит settings целиком за вычетом known_devices/new_device_ids только при !same_source — заявление «settings переносится целиком» для AC6 корректно в заявленном объёме (полный экспорт/импорт, не пространственный).
   • src/editors/decor/geometry.ts:144 — width_cm: Math.max(0.1, Math.min( 100, Number(style.widthCm) || 0.1)) — существующий явный код конвертации из DecorStyle.widthCm (camelCase) в персистентное поле width_cm (snake_case). Это прямо противоречит утверждению ТЗ (см. находку M1 ниже).
   • src/editors/decor/types.ts:78-85 — интерфейс DecorStyle объявлен как { color, opacity, widthCm, fill, fillColor, fillOpacity } — camelCase, не snake_case, как утверждает ТЗ (см. M1).
   • docs/CHANGELOG.ru.md (запись #360) — подтверждён факт существования тулбар-пикера «основной цвет и прозрачность», на который опирается раздел «Сценарий».
   • demo/smoke_*.mjs, debounce() (src/houseplan-editor-runtime.ts:722) — подтверждена техническая осуществимость дебаунс-записи (используемый примитив уже существует в модуле), план автотестов не требует нового инфраструктурного решения.
6. Проверено отсутствие незакрытых меток неопределённости («уточнить», TBD, голый «?») — не найдено.
7. Проверена структура на соответствие обязательному списку §7.1: сценарий · что человек увидит до/после · проблема · скоуп/не-скоуп · контракт поведения · UX · модель данных и миграция · i18n · AC1…AC7 с доказательством · план автотестов · риски · откат · release-артефакты — все присутствуют.
8. Гейты (tsc/test/build/check-docs/инварианты/смоки) не прогонялись: diff этого раунда — только новый файл в docs/specs/, продуктовый код (src/**, custom_components/**) не тронут, гейтам нечего было бы доказать (тот же вывод, что и в SPEC-REVIEW-376-r1 для аналогичной стадии).

Находки

M1 (Medium, в скоупе) — контракт называет формат ключа DecorStyle неверно и не как предположение, а как проверенный факт

Файл: docs/specs/377-decor-default-persist.md, раздел «Контракт поведения», пункт 7.

Текст: «Формат ключа — snake_case, поля фронтового DecorStyle (src/editors/decor/types.ts:78): color, opacity, width_cm, fill, fill_color, fill_opacity.»

Это не так: src/editors/decor/types.ts:78-85 объявляет DecorStyle в camelCase — color, opacity, widthCm, fill, fillColor, fillOpacity. Snake_case-именование (width_cm, fill_color, fill_opacity) относится к персистентным полям фигур (DecorBase/DecorRect в том же файле, строки 10-52), не к DecorStyle. Существующий код уже явно разводит эти две конвенции: src/editors/decor/geometry.ts:144 содержит ручную конвертацию width_cm: Math.max(0.1, Math.min(100, Number(style.widthCm) || 0.1)) именно потому, что DecorStyle.widthCm и персистентное width_cm — разные идентификаторы.

Показательно, что ровно эта развилка (snake/camelCase формат нового ключа) в предшественнике этой задачи была корректно оформлена как явное предположение: SPEC-REVIEW-376-r1, раздел «Что проверено и корректно» — «Раздел «Принятые предположения» корректно выносит технические, не наблюдаемые пользователем решения (формат ключа snake/camelCase, путь записи только из runtime-редактора) в явный блок». В нынешнем ТЗ #377 такого раздела «Принятые предположения» нет вообще, а тот же вопрос вместо этого подан как проверенный факт со ссылкой на конкретную строку — и эта ссылка, при сверке, факт не подтверждает.

Почему это находка, а не мелочь. AC2 требует «конфиг с ключом → _decorStyle = мердж ключа поверх DEFAULT_DECOR_STYLE», а AC1/AC6 фиксируют формат серверного ключа как snake_case. Между snake_case-ключом настроек и camelCase-полем _decorStyle нужен явный маппинг (width_cm ↔ widthCm, fill_color ↔ fillColor, fill_opacity ↔ fillOpacity); без него код, написанный «по букве» текущего пункта 7 (взять ключи настроек и наложить их на _decorStyle как если бы имена совпадали), молча не будет мержить три из шести полей — регресс ровно на тех полях, где имя меняется. Пока формулировка стоит как «факт со ссылкой», а не как предположение, у автора кода нет сигнала, что это место требует отдельного решения.

Чем чинится в этой же задаче: один абзац, например: «Ключ настроек snake_case (color, opacity, width_cm, fill, fill_color, fill_opacity) конвертируется в camelCase-поля DecorStyle (color, opacity, widthCm, fill, fillColor, fillOpacity) по прямому соответствию имён — по аналогии с уже существующей конвертацией в src/editors/decor/geometry.ts:144. Принято предположительно, поменять свободно.» Ссылку на types.ts:78 при этом стоит поправить на то, что там реально объявлено (camelCase), либо убрать как источник утверждения о snake_case.

Что проверено и корректно

• Классификация полного трека обоснована и унаследована корректно: причина («новый персистентный ключ + кросс-модульный путь записи», критерий §5 «нет новых compatibility-полей» не пройден) совпадает с находкой H1 SPEC-REVIEW-376-r1, ТЗ прямо на неё ссылается, а не переизобретает рассуждение заново.
• Оба Medium предшественника (SPEC-REVIEW-376-r1 M1 «нет раздела откат», M2 «нет AC на текстовые пункты») к #377 не относятся — они были про пункты (а/б/д/е) исходного #376, которые остались в #376 на small; #377 — только пункт (в), и его собственный раздел «Откат» и AC1…AC7 присутствуют полностью, каждый со способом доказательства (unit/pytest/смок/ гейт).
• Задача в скоупе docs/SCOPE.md: полиш уже принятой фичи #360 (главный тулбар), не открывает новый Core user job, не входит в «Out of scope»; соответствует «Design consequence» — правка касается только Background-редактора (admin-only поверхность), View не получает новых путей записи.
• Продуктовых вопросов владельцу не осталось: решение «персистить в серверный конфиг» уже принято владельцем 29.08 и зафиксировано в теле issue; ТЗ на него ссылается, а не переизобретает.
• Сценарий и «что человек увидит до/после» отвечают на оба обязательных вопроса §7.1 конкретно, без терминов реализации, и соответствуют фактическому поведению тулбар-пикера #360 (подтверждено записью в CHANGELOG.ru.md).
• Контракт поведения (пп. 1-6, кроме уже описанного в M1 п.7) построчно сверен с кодом и подтверждён: легаси-путь без ключа, дебаунс через существующий примитив, единственный сериализованный канал записи _writeConfig/expected_rev (#340), удаление ключа при возврате к дефолту по образцу bg_color/fill_colors, запись только из editor-runtime (холодный View класса #357 не задет — подтверждено: чтения _decorStyle в houseplan-card.ts ограничены превью черновика/мебели, которые в View не рендерятся).
• Диапазоны валидации в наброске схемы (opacity 0..1, width_cm 0.1..100, fill_opacity 0..1) не придуманы заново, а дословно совпадают с уже принятыми диапазонами для decor-фигур той же кодовой базы — снижает риск, что AC1 окажется неисполнимым или несогласованным с соседней схемой.
• AC6 (импорт/экспорт byte-в-byte) корректно ограничен по объёму: заявление подтверждено для полного экспорта/импорта; пространственный (build_space_merge) импорт глобальный settings не трогает вовсе, что не противоречит контракту (ключ глобальный, не per-space) и не заявлено в AC6.
• i18n корректно отмечен как незадетый — в UI не добавляется новый текст, проверка существующим паритет-гейтом достаточна.
• Риски названы конкретно вместе со снимающим механизмом (гонка вкладок → штатный conflict #340 + дебаунс; захламление settings → дебаунс + удаление ключа при дефолте; обратная совместимость → опциональность в обе стороны), а не общими словами.
• Release-артефакты (CHANGELOG.md+.ru.md, USER-GUIDE.ru.md) названы по правилу docs/specs/README.md — ключ меняет пользовательское поведение (переживание перезагрузки), артефакты перечислены явно.

Чего не проверял

• Гейты (npx tsc --noEmit, npm test, npm run build, check-docs.mjs, инварианты модели, browser-смоки) — не прогонял: diff этого раунда не затрагивает src/** или custom_components/**, только новый файл docs/specs/377-decor-default-persist.md. Гейтам, завязанным на код, нечего было бы доказать на стадии ТЗ (тот же вывод сделан в SPEC-REVIEW-376-r1 для той же стадии).
• Реализуемость дебаунса «1000 мс» и точную формулировку будущего предложения в USER-GUIDE.ru.md — контракт называет смысл и число, не финальный текст промо-абзаца; это в пределах допустимой недосказанности для технической/текстовой детали, не находка.
• Согласованность нового ключа settings.decor_default_style с scripts/config-field-registry.mjs — реестр по docs/CONFIG-COMPATIBILITY.md прямо заявлен как неполный («initially covers the known compatibility and internal-field debt... not yet the complete canonical schema»); регистрация нового (не легаси) поля не требуется этим документом и станет предметом код-ревью, если понадобится.
• Точный будущий API взаимодействия дебаунс-таймера с существующей очередью _writeChain/enqueueSerializedWrite при одновременном геометрическом изменении — техническая деталь реализации, агенты решают её сами (§7.1), не наблюдаема пользователем и не входит в критерии готовности ТЗ.

Низкая находка (снята с записью)

• Раздел «UX»/«Риски» не содержит явной фразы о влиянии на touch-контракт (docs/TOUCH-SUPPORT.md), которую требует чек-лист DoR §2.5. По существу влияние действительно нулевое: задача не добавляет новых элементов UI («Ничего нового в UI не появляется», уже добавленные тулбар/трей-пикеры #360 не меняются), поэтому вывод «нет влияния» тривиально следует из уже написанного текста. Снимаю без правки, а не как повод для цикла — но фиксирую, чтобы автор при следующей правке ТЗ добавил явную строку «touch: нет» для формального прохождения DoR-чек-листа без повторной проверки ревьюером.

Вердикт

ТЗ структурно полное (все обязательные разделы §7.1 на месте), классификация полного трека унаследована корректно от SPEC-REVIEW-376-r1 и не оспаривается, почти все технические утверждения о текущем коде подтвердились построчно. Единственная находка — M1: контракт называет формат ключа DecorStyle snake_case со ссылкой на конкретную строку кода, которая при проверке показывает обратное (camelCase), и не помечает это место как предположение, хотя тот же вопрос в предшествующем ТЗ (#376) был корректно оформлен именно как явное предположение. Без исправления этого пункта реализация, написанная «по букве» контракта, рискует не замержить три из шести полей стиля при чтении настроек — то есть ровно то поведение, которое проверяет AC2. High-находок нет, Medium — один, в скоупе, чинится в этой же задаче одним абзацем без нового цикла по существу решения (только техническая правка формулировки ТЗ).

Вердикт: жёлтый · заход r1 · блокирующих циклов 0/4 · High: 0 · Medium: 1 → в задаче