From 97bb0eceea42502c3d7aded74e30449e3a431019 Mon Sep 17 00:00:00 2001 From: "claude[bot]" <209825114+claude[bot]@users.noreply.github.com> Date: Sat, 29 Aug 2026 17:58:51 +0000 Subject: [PATCH] docs: review document for #377 Issue: #377 User-Visible: no --- docs/reviews/SPEC-REVIEW-377-r1.md | 248 +++++++++++++++++++++++++++++ 1 file changed, 248 insertions(+) create mode 100644 docs/reviews/SPEC-REVIEW-377-r1.md diff --git a/docs/reviews/SPEC-REVIEW-377-r1.md b/docs/reviews/SPEC-REVIEW-377-r1.md new file mode 100644 index 00000000..06f57174 --- /dev/null +++ b/docs/reviews/SPEC-REVIEW-377-r1.md @@ -0,0 +1,248 @@ +# 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 → в задаче`