diff --git a/docs/reviews/CODE-REVIEW-439-r1.md b/docs/reviews/CODE-REVIEW-439-r1.md new file mode 100644 index 00000000..ea3dc1a8 --- /dev/null +++ b/docs/reviews/CODE-REVIEW-439-r1.md @@ -0,0 +1,221 @@ +# CODE-REVIEW-439-r1 + +Issue: #439 — «Отказ сохранения настроек пространства оставляет применённым то, +чего на сервере нет» +Ветка: `issue/439-settings-save-rollback` +SHA на момент ревью: `3883352f` (родитель `f0574c1f`) +Трек: `trivial` (короткий, ТЗ в теле issue). Заход r1, блокирующих циклов +израсходовано 0 из 2. + +## Скоуп + +Диалог общих настроек пространства применял новый `settings` к +`_serverCfg` **оптимистично**, до подтверждения записи сервером +(`houseplan/config/set`). При обычном отказе записи (не `conflict`) откат не +происходил: карточка продолжала показывать непринятые сервером настройки, а +`show_room_tooltip` (и любая другая опция) вела себя так, будто запись +прошла — при том что ТЗ #426 объявляло этот риск снятым. + +Три AC короткого трека: +- **AC1** — обычный отказ: `_serverCfg.settings` и его content fingerprint + откатываются к снимку до нажатия «Сохранить»; диалог остаётся открыт с + введённым draft, `busy=false`, тост ошибки. +- **AC2** — конфликт: авторитетный `config/get` после `conflict` не + перезаписывается локальным rollback-снимком. +- **AC3** — успех: не регрессирует. + +Изменения: `src/serialized-write-queue.ts` (+49, новые +`optimisticAttempt`/`rollbackOptimistic`), `src/houseplan-editor-runtime.ts` +(точка вызова в `_saveSettingsDialog`, ~10 строк), новый браузерный smoke +`demo/smoke_room_tooltip_toggle.mjs` (+69), юнит-тесты на сам механизм, +оба changelog, собранный бандл (класс D), принятый fingerprint +документационных скриншотов. + +## Как проверялось + +### Чтение кода (доказательство по AC) + +- **AC1.** `_saveSettingsDialog` (`src/houseplan-editor-runtime.ts:10228+`) + до оптимистичного присваивания сохраняет `attempt = + optimisticAttempt(cfg, nextConfig, _cfgContentFingerprint, _cfgRev, + contentFingerprint)` — глубокий клон предыдущего `_serverCfg` плюс его + fingerprint и текущую ревизию. В `catch` вызывается `rollbackOptimistic`, + которая переписывает `_serverCfg`/`_cfgContentFingerprint` обратно и + дёргает `requestUpdate()`, **только если** `host._cfgRev` не сдвинулась и + текущий `_serverCfg` либо тот же объект, либо содержательно совпадает с + неудавшейся попыткой. `_cfgRev` в `_sendConfigCandidate` + (`houseplan-card.ts:7677-7683`) обновляется **только после** успешного + `callWS` — на отказе строка `this._cfgRev = response?.rev …` не + выполняется, значит guard пропускает откат. Проверено чтением и + подтверждено smoke (ниже). +- **AC2.** Конфликтная ветка `_saveConfigNow` + (`houseplan-editor-runtime.ts:8930-8941`) уже до правки перечитывала + конфиг через `_reloadConfigOnly()` и **до** повторного throw. Ревизия из + ответа `config/get` монотонно больше ожидавшейся (иначе `conflict` не + возник бы), поэтому к моменту, когда внешний `catch` в + `_saveSettingsDialog` вызывает `rollbackOptimistic`, `host._cfgRev !== + attempt.revision` истинно и функция возвращает `false`, не трогая уже + авторитетный `_serverCfg`. Проверено чтением и smoke. +- **AC3.** Успешный путь `attempt` не читает вовсе — ветка `try` не + изменена в части закрытия диалога/применения настроек. Регрессия + подтверждена существующими и новыми smoke-проверками. + +### Гейты, которые прогнал сам (на SHA `3883352f`, зелёного Validate на нём не найдено) + +| Гейт | Команда | Результат | +|---|---|---| +| Тайпчек | `npx tsc --noEmit` | чисто, без вывода | +| Юнит-тесты | `npm test` | 1814 passed, 0 failed, 1 skipped — совпадает с тем, что заявил автор | +| Сборка + сверка 3 копий бандла | `npm run build`, затем `npm run bundle:sync` | сборка чистая; `git status --porcelain` после обеих команд пуст — `dist/`, `custom_components/.../frontend/`, `demo/srv/assets` уже в актуальном состоянии, commit не разошёлся с исходником | +| Документационный фингерпринт | `node scripts/check-docs.mjs` (diff трогает `src/**`) | «Documentation checks passed (7 files, 10 external links)» — подтверждает принятый в `3883352f` fingerprint корректен | +| Named smoke (AC) | `node demo/smoke_room_tooltip_toggle.mjs` | все 21 проверка `true`, `OK` | +| Дисциплина «тест умеет падать» | временно заменил `if (attempt) rollbackOptimistic(...)` на no-op, пересобрал бандл, перезапустил тот же smoke | покраснели ровно 3 проверки: `rejectedSettingsRolledBack`, `rejectedFingerprintRolledBack`, `rejectedRuntimeUsesServerSettings` — то есть именно AC1. Дерево восстановлено (`cp` бэкапа + `bundle:sync`), `git status --porcelain` снова пуст | +| Бюджет бандла (дёшево, для полноты) | `npm run bundle:budget` | initial View 291175 B gzip / 300000 бюджет, есть предупреждение о запасе <15000 Б (#367) — это существующее давление на бюджет всего проекта, не следствие 40 строк этого диффа; не блокирует | + +### Выбор смоков по дельте + +`node scripts/smoke-select.mjs --base origin/dev --head HEAD`: +изменено файлов `src/**`: 2, символов на изменённых строках: 9, матрица 216 +смоков (`ls demo/smoke_*.mjs | wc -l` = 216). + +**Прямое совпадение (4)** — прогнал все: +- `demo/smoke_room_tooltip_toggle.mjs` — назван в AC, см. выше. +- `demo/smoke_area_relocation_safety.mjs` — зелёный, все 11 проверок `true`. +- `demo/smoke_v8_draft_write.mjs` — зелёный, все 16 проверок `true`. +- `demo/smoke_optimize_coordinate_canonicalization.mjs` — зелёный, все 17 + проверок `true`. + +**Слабая связь — одно распространённое имя (20, `_cfgRev`)** — не прогонял. +Решение: диф ограничен единственной точкой входа — +`catch`-блоком `_saveSettingsDialog`, который не переиспользуется другими +диалогами/потоками записи; `_cfgRev` в остальных 20 смоках фигурирует по +несвязанным путям (drag, layout, virtual light, discovery-фильтры и т.д.), +ни один из них не проходит через `_saveSettingsDialog`. Считаю прогон +избыточным для этой дельты. + +Golden/perf/backend: не прогонял — diff не меняет геометрию, стили или слои +в успешном пути (единственное видимое отличие — поведение при уже +подтверждённом отказе записи, вне сценариев golden-капчура); backend +(`custom_components/**/*.py`) не тронут; perf не назван в AC. Инварианты +модели (`npm run invariants`) не прогонял — diff не трогает рёбра комнат, +записи толщины, `layout`, `marker.space`, `open_spans`. + +## Находки + +Ни одной блокирующей (High) или Medium-в-скоупе находки. Одна Low, +снимается с записью ниже — без отдельного issue (в скоуп не выходит). + +**Low — обещанный в самом ТЗ второй сдвиг `_cfgEpoch` не реализован.** +Раздел «Реализация и границы» в теле issue: «Одновременно восстановить +`_cfgContentFingerprint`, сдвинуть cache epoch и запросить render.» +`rollbackOptimistic` (`src/serialized-write-queue.ts`) восстанавливает +`_serverCfg` и `_cfgContentFingerprint`, дёргает `requestUpdate()`, но +**не трогает `_cfgEpoch`** — тип `host` в её сигнатуре его даже не +перечисляет. `_cfgEpoch` бампается один раз в начале `_saveConfigNow` +(строка 8931) и второй раз после отката не сдвигается. + +Разобрал, порождает ли это видимый дефект: `_cfgEpoch` участвует в ключах +кэшей геометрии/предпросмотра (контуры комнат, объединение стен, солнечные +лучи) и в редакторских кэшах декора — но геометрию/декор этот диалог не +меняет, а единственный settings-зависимый кэш, ключующийся по эпохе +(солнечные лучи, `houseplan-card.ts:10488`), включает `north`/значения +режима **прямо в строку ключа**, а не полагается на эпоху как на +единственный признак изменения. Поэтому даже кэш-промах после несостоявшегося +второго бампа не может отдать неверные лучи для другого `north` — ключ +и так другой; а для того же `north` результат совпадает по построению. +Цвета/glow/фон читаются из `_serverCfg.settings` в рендере без отдельного +эпоха-кэша. Видимого дефекта не нашёл ни рассуждением, ни smoke (который +как раз проверяет наблюдаемое поведение runtime после отката — +`rejectedRuntimeUsesServerSettings`). + +Снимаю без правки: разрыв между текстом ТЗ и кодом реален, но не создаёт +наблюдаемого расхождения при нынешней архитектуре кэшей. Если в будущем +появится settings-зависимый эпоха-кэш без значения в ключе (например, +для `bg_color`/`glow_radius_cm`), тот же паттерн уже будет тихо ломаться — +стоит иметь в виду при следующей работе с `rollbackOptimistic`, но заводить +это отдельным issue сейчас избыточно: одно наблюдение без текущего +пользовательского эффекта. + +## Что проверено и корректно + +- Идентити-guard (`_cfgRev` + ссылочное/содержательное сравнение + `_serverCfg`) корректно различает «наш неудавшийся кандидат» и + «авторитетный конфликтный reload» / «более новую правку» — подтверждено + и чтением кода, и двумя юнит-тестами + (`test/serialized-write-queue.test.mjs`), и smoke AC2. + `_cfgRev` меняется исключительно после успешного ответа сервера + (`_sendConfigCandidate`), поэтому на обычном отказе guard всегда + пропускает откат — структурно, а не по счастливой случайности с таймингом. +- Глубокий клон снимка (`JSON.parse(JSON.stringify(previous))` в + `optimisticAttempt`) — восстановленный `_serverCfg` не делит ссылку с + последующими мутациями; проверено юнит-тестом + (`assert.notEqual(host._serverCfg, previous, ...)`). +- draft в диалоге не трогается веткой rollback — `_settingsDialog` в + `catch` только теряет `busy`; введённые значения остаются, что и требует + AC1/AC2 (smoke `rejectedDraftKept`, `conflictDraftKept`). +- `_saveConfigNow`'s conflict-ветка (`await + this.host._reloadConfigOnly()` до повторного throw) не переписана этим + диффом — новый код добавлен строго в внешнем `catch`, не пересекается с + существующей веткой rollback для физической геометрии + (`physicalGeometryRolledBack`, `_reloadRejectedPhysicalWrite`), которая + этот диф тоже не трогает. +- Оба changelog в том же коммите (`f0574c1f`), формулировка соответствует + фактическому поведению (AC1), не переобещает про AC2 (внутренняя защита, + не новое видимое поведение) — текст `docs/CHANGELOG.md` / + `docs/CHANGELOG.ru.md` сверен построчно. +- Трейлеры `Issue: #439` и `User-Visible: yes|no` на обоих коммитах на + месте; `User-Visible: yes` в `f0574c1f` сопровождается правкой обоих + changelog в этом же коммите. +- Классификация `trivial`: один диалог/модуль, 3 AC, ожидаемое поведение + зафиксировано контрактом #426 — критерии §5.1 выполнены, спор не вижу. +- Единственный источник числа/значения: диф не вводит новое отображаемое + значение — только восстанавливает существующий источник (`_serverCfg`) + к его прежнему состоянию; `test/single-source-numbers.test.mjs` в + прогоне `npm test` зелёный без изменений в его области. + +## Чего не проверял и почему + +- Полный набор из 216 смоков и `golden:verify`/`performance_smoke` — diff + не расширяется за пределы одного диалога, это предрелизная обязанность + (§8), не гейт этого ревью. +- 20 смоков со слабой связью по `_cfgRev` — см. обоснование выше. +- `npm run invariants` — diff не касается геометрии/толщины/`layout`. +- `python -m pytest tests_backend` — `custom_components/**/*.py` не + тронут. +- Ручное тестирование в браузере (вне smoke-харнесса) — не запускал, + цикл ревью не предполагает ручной прогон; smoke дублирует ровно тот же + DOM/hass-стенд. +- Кросс-языковая идентичность канонизации координат (JS↔Python, + `LATTICE_GRID_N=240`, `LATTICE_NOISE_STEPS=1e-4`) как потенциальный + источник unstable fingerprint после `canonicalizeConfigGeometry` внутри + `_writeConfig` — рассмотрел теоретически (если бы бэкенд не гарантировал + фиксированную точку, guard мог бы пропустить откат для конфигов с + «грязной» геометрией). Не нашёл сценария: backend + (`custom_components/houseplan/websocket_api.py`) канонизирует конфиг на + каждом `config/get`/`config/set` тем же алгоритмом, поэтому входящий + `_serverCfg` уже канонический и повторная канонизация — no-op по + построению. Не свежий риск этого диффа (канонизация всего конфига при + любой записи settings существовала и до #439); не заводил как находку. + +## Вердикт + +Все три AC доказаны: AC1/AC3 существующим+новым smoke и юнит-тестами, +AC2 — smoke плюс структурным чтением guard'а (ревизия монотонна только +вверх и обновляется исключительно на успехе). Единственная находка — Low, +снята с запиской, без наблюдаемого дефекта. Гейты, которые обязаны быть +зелёными на этом заходе, зелёные (пересчитано мной, не только заявлено +автором). Trivial-классификация обоснована. + +**Вердикт: зелёный.** + +--- + + + +## Материал раунда + +- Ветка: `issue/439-settings-save-rollback`, коммит `3883352f43e7` — ребейз его осиротит, и это нормально: ниже якоря, которые ребейз не меняет. +- Дерево материала: `39c0bc135ef59643339eb2d2fbee5a52e4ad6b3f` + ``` + git log --all --format='%H %T' | grep 39c0bc135ef5 + ```