mirror of
https://github.com/Matysh/houseplan-card
synced 2026-09-29 03:09:36 +00:00
claude[bot]
@@ -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-классификация обоснована.
|
||||
|
||||
**Вердикт: зелёный.**
|
||||
|
||||
---
|
||||
|
||||
<!-- material-anchors: сгенерировано конвейером (#414) -->
|
||||
|
||||
## Материал раунда
|
||||
|
||||
- Ветка: `issue/439-settings-save-rollback`, коммит `3883352f43e7` — ребейз его осиротит, и это нормально: ниже якоря, которые ребейз не меняет.
|
||||
- Дерево материала: `39c0bc135ef59643339eb2d2fbee5a52e4ad6b3f`
|
||||
```
|
||||
git log --all --format='%H %T' | grep 39c0bc135ef5
|
||||
```
|
||||
Reference in New Issue
Block a user