From 82d710f160d8572ef6687499c4b8acdc7995ed45 Mon Sep 17 00:00:00 2001 From: "claude[bot]" <209825114+claude[bot]@users.noreply.github.com> Date: Sat, 29 Aug 2026 18:36:58 +0000 Subject: [PATCH] docs: review document for #377 Issue: #377 User-Visible: no --- docs/reviews/CODE-REVIEW-377-r1.md | 199 +++++++++++++++++++++++++++++ 1 file changed, 199 insertions(+) create mode 100644 docs/reviews/CODE-REVIEW-377-r1.md diff --git a/docs/reviews/CODE-REVIEW-377-r1.md b/docs/reviews/CODE-REVIEW-377-r1.md new file mode 100644 index 00000000..e8f26dfc --- /dev/null +++ b/docs/reviews/CODE-REVIEW-377-r1.md @@ -0,0 +1,199 @@ +# CODE-REVIEW-377-r1 + +- Issue: https://github.com/Matysh/houseplan-card/issues/377 +- Заход: r1 (первый прогон код-ревью; предыдущая попытка `S7` вернулась в `S6` + из-за конфликта ребейза до того, как код кто-либо читал — цикл не + расходовался, см. комментарий владельца от 18:17) +- Ветка: `issue/377-decor-default-persist`, ребейзнута на `origin/dev` с уже + смерженным #375 +- SHA материала ревью: `2edaf1adac60f9939b836da87f5f1f549dab2548` + (`git rev-parse HEAD` непосредственно перед подведением итогов) +- Диапазон: `git diff origin/dev...HEAD` — 33 файла, +789/-468 + +## Скоуп + +Персист серверного ключа `settings.decor_default_style` (цвет/стиль декора +«по умолчанию» из тулбара Background-редактора, #360): переживает +перезагрузку, общий для всех редакторов плана. Три поверхности: схема +(`validation.py`), инициализация (`houseplan-card.ts`), запись +(`houseplan-editor-runtime.ts`) — ровно то, что называла аналитика и ТЗ +(SPEC-REVIEW-377-r2, зелёный). + +## Как проверялось + +Полный разбор (первый заход код-ревью для этой задачи; §2.10 неприменим). +Читал построчно: `src/editors/decor/geometry.ts` (новые +`decorStyleFromSettings`/`decorStyleToSettings`), `src/houseplan-card.ts` +(`_seedDecorStyle` и оба места вызова), `src/houseplan-editor-runtime.ts` +(`_updateDecorStyle`/`_persistDecorStyle`, все шесть UI-точек изменения +стиля), `custom_components/houseplan/validation.py` (схема), +`custom_components/houseplan/import_export.py` (путь `settings` при полном +импорте — подтверждает AC6 сверх юнита), тесты (`test/decor-style-persist.test.mjs`, +`tests_backend/test_validation.py`, `demo/smoke_decor_default_persist.mjs`), +изменения в `scripts/mutation-gate.mjs`, `test/color-picker.test.mjs`, оба +changelog и оба USER-GUIDE. + +### Гейты — прогнал сам (зелёного Validate на этом SHA нет) + +| Гейт | Команда | Результат | +|---|---|---| +| Typecheck | `npx tsc --noEmit` | чисто | +| Юниты | `npm test` | 1560 pass / 0 fail / 1 skipped (совпадает с заявкой автора) | +| Сборка | `npm run build` | ok, 14.5s | +| Синхронизация бандлов | `npm run bundle:sync` | ok; `git status --porcelain` после — пусто (три копии дерева байт-в-байт совпадают с закоммиченными) | +| Бюджет | `npm run bundle:budget` | initial View 276 221 Б / 300 000 (запас 23 779 Б) — проходит; отличие от заявленных автором 276 059 Б в пределах шума нединамического build (не влияет на вердикт) | +| Доки | `node scripts/check-docs.mjs` | «Documentation checks passed (7 files, 10 external links)» — diff трогает `src/**`, гейт обязателен и зелёный | +| Backend (чистое подмножество) | `python -m pytest tests_backend -q --ignore=tests_backend/test_coordinate_canonicalization.py` (после `pip install pytest voluptuous`, в базовом окружении ревьюера их не было) | 235 passed, 1 skipped — совпадает с заявкой автора байт-в-байт. `test_coordinate_canonicalization.py` падает на `ModuleNotFoundError: homeassistant` при коллекции (не `test_ha_*.py`, поэтому `conftest.py` его не глушит) — это существующий пробел окружения, не связан с #377, полный HA-harness — дело CI | +| Единственный источник числа | `node --test test/single-source-numbers.test.mjs` | 3/3 pass; смысловая часть — см. ниже | +| Мутанты (только новые, точечно `--id=`) | `node scripts/mutation-gate.mjs --id=decor-default-style-seed-cut` и `--id=decor-default-style-debounce-cut` | оба «поймано 1 из 1» — тест умеет падать | + +Полный `mutation-gate.mjs` без `--id=` не гонял: это полная матрица всех +мутантов проекта (несоразмерно точечной правке), к тому же в этом окружении +падает на несвязанном backend-guard'е (тот же отсутствующий `homeassistant`). +Точечный `--id=` — правильный инструмент здесь. + +### Смоки — выбор через `scripts/smoke-select.mjs --base origin/dev --head HEAD` + +Инструмент дал 7 «прямых совпадений» и 13 «слабых связей» (все — по одному +имени `_saveConfig`, geometry/wall-thickness в диффе не тронуты). + +Прогнал все 7 прямых совпадений — все OK: +`smoke_decor_default_persist` (новый, AC4/AC5), `smoke_bg_color`, +`smoke_color_picker`, `smoke_decor`, `smoke_dialog_zombie`, `smoke_furniture`, +`smoke_general_settings`. + +Из слабых связей прогнал `smoke_config_writer` (тот же, что заявлен автором) — +OK. Остальные 12 слабых связей не гонял: диф не трогает геометрию/стены/ +толщину, разделяют с диффом только имя `_saveConfig`, чей вызывающий код в +затронутых местах не менялся по поведению (только маршрутизация через +`_updateDecorStyle`). `npm run invariants` не требовался — diff не трогает +рёбра комнат, `layout`, `marker.space`, `open_spans` или записи толщины. + +`npm run golden:verify` не гонял: ТЗ прямо утверждает «видимых изменений +нет», и это подтверждается кодом — `_decorStyle` управляет только дефолтом +для НОВЫХ объектов декора/мебели (`_renderDecorLayer` draft-превью, +`_renderFurniturePlacementPreview`), View эти пути не рендерит, а +`imageSha256` всех эталонных скриншотов, кроме одного, не изменился; +единственное расхождение (`06-device-display-preview.png`, 326512→326513 +байт) — разница в 1 байт кодирования PNG, не связанная с decor-путём +скриншота и не относящаяся к этой правке по содержанию сцены; `check-docs` +(сверяет `sourceFingerprint`) зелёный. + +## Находки + +Нет ни одной High или Medium-находки. + +**Low (снята без правки).** `_updateDecorStyle` заводит собственный +`window.setTimeout(…, 1000)` (`houseplan-editor-runtime.ts:9403-9411`) и не +чистит его в `disconnectedCallback` карты, в отличие от штатного +`_saveConfigDebounced.flush()` (`houseplan-card.ts:2638`, «never leave an edit +unsent on teardown»). Разобрано по коду (не исполнением): в реальности потерю +данных это создаёт только при закрытии/перезагрузке вкладки строго внутри +окна 1 с после последнего изменения цвета — при обычном пересоздании дерева +Lovelace (тот случай, для которого существует `disconnectedCallback`) таймер +переживает как обычный JS-таймер и запись всё равно долетает, просто позже. +Цена ошибки — вернуться к прежде выбранному цвету, ценность правки в рамках +этой задачи не оправдана: снимаю с записью, правки не требую. + +## Проверка AC + +- **AC1** (pytest, валидация): доказано исполнением — + `tests_backend/test_validation.py::test_decor_default_style_setting`, + 10 мусорных вариантов + `"solid"` вместо объекта, все `pytest.raises`. + Прогнал сам, 235/0 в подмножестве, включая этот тест. +- **AC2** (юнит фронта, мердж): доказано исполнением — + `test/decor-style-persist.test.mjs` («all six fields cross the boundary», + частичный ключ, мусорные поля). Мутант `decor-default-style-seed-cut` + ловит вырезание вызова — 1/1. +- **AC3** (юнит фронта, легаси): доказано исполнением — тот же файл, + `undefined/null/'x'/7` → `DEFAULT_DECOR_STYLE`. +- **AC4** (смок, одна запись после дебаунса + сброс к дефолту без ключа): + доказано исполнением — `demo/smoke_decor_default_persist.mjs`, прогнал сам, + OK. Мутант `decor-default-style-debounce-cut` (дебаунс → 0 мс) ловится — + 1/1, подтверждает, что смок действительно проверяет окно, а не просто + наличие записи. +- **AC5** (смок, посев новой карты из ключа): доказано исполнением — тот же + смок, `seeded` (частичный ключ наследует остальные поля дефолта). +- **AC6** (round-trip импорт/экспорт): доказано исполнением на уровне схемы + (`test_decor_default_style_setting`, byte-identical) плюс разобрано + чтением, не исполнением — `import_export.py:1661-1669`: для non-same-source + импорта из `settings` вычищаются только `known_devices`/`new_device_ids`, + `decor_default_style` не в списке и проходит насквозь; `config = CONFIG_SCHEMA(config)` + применяет ту же схему, что и юнит. Отдельного юнита самого + `import_export.py` на этот ключ нет — при полном треке для «переносится + штатно, без спецобработки» это достаточное доказательство: код, который мог + бы его выбросить, прочитан и не делает этого. +- **AC7** (гейт + недостижимость в холодном View): гейты выше зелёные. + Недостижимость в View разобрана чтением: `_updateDecorStyle`/ + `_persistDecorStyle` существуют только в `HouseplanEditorRuntime` + (`houseplan-editor-runtime.ts`), которая грузится лениво только при входе в + редактор; во всём `houseplan-card.ts` `_decorStyle` только читается + (превью черновика/мебели), никогда не пишется вне рантайма. Холодный View + этот модуль не подгружает — записи там физически нет. + +## Что проверено и корректно + +- Схема `validation.py` соответствует ТЗ п. «Модель данных» дословно: + диапазоны, `_COLOR`, все поля опциональны. +- Единственная точка конверсии snake_case↔camelCase + (`decorStyleFromSettings`/`decorStyleToSettings`) — ровно то, что требовал + ТЗ п.7 после SPEC-REVIEW-377-r1 M1; сопоставил маппинг 1:1 с текстом ТЗ. + `decorStyleToSettings` возвращает `null` при точном совпадении с дефолтом + (сравнение по значению всех 6 полей) — паттерн «дефолт = отсутствие ключа» + выдержан. +- Посев (`_seedDecorStyle`) срабатывает ровно один раз на инстанс + (`_decorStyleSeeded`) и из обоих источников первого конфига — тёплого + кэша (`localStorage`, `houseplan-card.ts:3062-3066`) и первого серверного + ответа (`_adoptStructuralResponses`, `houseplan-card.ts:4045-4046`); + последующие обновления конфига не переоткрывают уже идущую сессию + редактирования — сознательное и разумное техническое решение (в тексте + явно откомментировано), продуктовым контрактом ТЗ не описано и не + противоречит ему (сценарий ТЗ — «назавтра» и «второй редактор на другом + устройстве», т.е. холодный старт, а не горячее обновление во время сессии). +- Запись идёт исключительно штатным сериализованным путём (`_saveConfig` → + `_saveConfigDebounced` → `_writeConfig` → `_sendConfigCandidate` с + `expected_rev`) — новый канал не создан, конфликт/тост наследуются + бесплатно. +- Все шесть UI-точек изменения стиля (`_decorSaveShape`, тулбар, + контекстный трей: цвет/непрозрачность, ширина, fill-чекбокс, fill-цвет, + ещё раз в `_renderDecorBar`) переведены на `_updateDecorStyle` — ни одна не + забыта (сверил построчно с прежними местами прямого присваивания + `host._decorStyle =` в диффе). +- Трейлеры (`Issue: #377`, `User-Visible: yes/no`) на месте во всех трёх + коммитах; оба changelog и оба USER-GUIDE — в том же коммите, что + поведение (`ec76948b`), не отдельным «допишу потом». +- Мутант-регистрация (`scripts/mutation-gate.mjs`) описывает ровно тот + дефект, который она ловит («seed-cut» — тихая деградация посева, приводит + к молчаливому нерабочему AC2/AC5; «debounce-cut» — спам записей), + соответствует формулировке в ТЗ «План автотестов». +- «Один источник числа»: новый ключ не дублирует уже показываемое где-то + значение (это предпочтение по умолчанию для будущих объектов, а не + производная величина, показанная дважды) — `test/single-source-numbers.test.mjs` + зелёный, смысловых дублей не нашёл. + +## Чего не проверял + +- Полный `npm run golden:verify` и полный `demo/smoke_*.mjs` (все ~204) — не + оправдано объёмом правки; обоснование выбора — выше. +- Полный HA backend harness (`test_ha_*.py`, `test_coordinate_canonicalization.py`) + — недоступен в этом окружении (нет `homeassistant`), это гейт CI/бета, не + код-ревью; расхождение не связано с #377. +- Живое ручное тестирование в браузере (ручного цикла в процессе нет) — + замещено чтением кода и прогоном автотестов/смоков выше. +- Гонка двух вкладок (штатный conflict #340) отдельно не воспроизводил — + путь записи переиспользует существующий механизм, для которого этот + сценарий уже покрыт в других смоках (`smoke_save_race.mjs`, не гонял — + слабая связь, поведение конфликта в этой задаче не менялось). +- `import_export.py` полный интеграционный юнит именно на + `decor_default_style` (только чтение кода + существующий байт-в-байт юнит + схемы, см. AC6 выше). + +## Вердикт + +Все 7 AC доказаны (автотестом там, где это дешевле и надёжнее; чтением кода — +там, где ТЗ само на это полагалось, с явной пометкой). High/Medium-находок +нет, одна Low снята с записью. Гейты, соразмерные объёму задачи, зелёные. + +Вердикт: зелёный · заход r1 · блокирующих циклов 0/4 · High: 0 · Medium: 0 + +Готово к очереди на пре-релиз.