diff --git a/docs/reviews/CODE-REVIEW-442-r1.md b/docs/reviews/CODE-REVIEW-442-r1.md new file mode 100644 index 00000000..dfd72a6d --- /dev/null +++ b/docs/reviews/CODE-REVIEW-442-r1.md @@ -0,0 +1,271 @@ +# CODE-REVIEW-442-r1 + +- Issue: https://github.com/Matysh/houseplan-card/issues/442 +- Этап: code (PROCESS.md §2.7) +- Заход: r1 (первый код-ревью цикл; спек-ревью r1 уже зелёный) +- Ветка: `issue/442-marker-write-rollback`, HEAD `ffbf8242abf441196f731012f454f850c102dfaa` +- ТЗ: `docs/specs/442-marker-write-rollback.md` (принято зелёным на SHA `a8426268`) +- Диапазон: `git diff origin/dev...HEAD` — 40 файлов, +1732/-459 (без учёта `dist/**`, + `custom_components/houseplan/frontend/**` — class D, не проверяется построчно, + проверена побайтовая идентичность с пересобранным `dist`) + +## Скоуп + +Атомарный immutable candidate + guarded rollback для трёх путей, которые могут +задеть четыре marker semantic validators: основной Save устройства +(`_saveMarker`), запись матрицы калибровки робота (`saveVacuumMatrix` / +`_vacSaveMatrix`), три сценария calibration UX (low-residual auto, high-residual +proposal, manual fit). Плюс отдельная граница durable config acceptance vs +layout/file side effects, и более строгий `rollbackOptimistic` guard (сравнение +content по фингерпринту обязательно даже при совпадении object identity, #442 +AC2). Не в скоупе, и диффом не тронуто: 18 generic `_saveConfig()` вызовов +(Hide/Show, discovery seeding, обычные vacuum-настройки), общий settings-путь +`_saveSettingsDialog` (#439), backend/schema/i18n, геометрия, маршрутизация +карт кроме сохранения уже готового `persistRoutes()` (#441). + +## Как проверялось + +**Дешёвые гейты (прогнаны сам, зелёного Validate на этом SHA не было):** + +| Гейт | Результат | +|---|---| +| `npx tsc --noEmit` | green, 0 ошибок | +| `npm test` | 1863 pass, 1 skip, 0 fail (`# tests 1864`) | +| `npm run build` | green; `git status --porcelain` после билда пуст — dist воспроизводится байт-в-байт | +| `npm run bundle:sync` | green; SHA-256 `dist/houseplan-card.js` и `houseplan-assets.json` совпадают с `custom_components/houseplan/frontend/**` | +| `node scripts/check-docs.mjs` | green: «Documentation checks passed (7 files, 10 external links)» — diff трогает `src/**`, поэтому обязателен | + +**Инварианты модели:** неприменимо — diff не трогает рёбра комнат, толщину, +`layout`, `marker.space`/`open_spans`; единственная геометрическая функция +(`canonicalizeConfigGeometry`) не изменена, только обёрнута в +`_prepareConfigCandidate` без изменения поведения (подтверждено чтением и +`npm test`, включая `#278`/`#224`-серии). + +**Mutation witnesses (`scripts/mutation-gate.mjs --id=`), каждый +запущен лично и подтверждён как «покраснел, как обязан»:** + +| id | guard | Что проверяет | +|---|---|---| +| `marker-reject-keeps-optimistic-candidate` | `smoke_marker_write_rollback` | AC1: отсутствие `rollbackOptimistic` красит смок | +| `marker-rollback-keeps-enqueue-time-revision` | `smoke_marker_write_rollback` | AC2: без ребиндинга `attempt.revision` смок красит | +| `accepted-marker-rolled-back-by-layout-failure` | `smoke_marker_write_rollback` | AC3: безусловный rollback после accept красит смок | +| `vacuum-reject-keeps-optimistic-matrix` | `test/vacuum-calibration-write.test.mjs` | AC5–AC7: без rollback в `saveVacuumMatrix` тест красит | +| `vacuum-auto-reports-success-before-acceptance` | `test/vacuum-calibration-write.test.mjs` | AC4: ранний `saved=true` до ответа сервера красит тест | +| `optimistic-rollback-skips-same-root-fingerprint` | `test/serialized-write-queue.test.mjs` | AC2: возврат identity-шортката красит тест | + +Также перепроверены два **не новых**, но переехавших вместе с рефакторингом +мутанта (`src/houseplan-card.ts`→`src/houseplan-editor-runtime.ts` и +`src/vacuum-calibration-write.ts`): `vacuum-manual-fit-after-proposal-uses-the-dock` +и `child-readd-clears-parent-tombstone` — оба по-прежнему красят целевой +смок/гейт после переноса паттерна. Полный `mutation-gate` без `--id` не +гонялся: он падает на несвязанном `backend-test-guard` (`No module named +pytest`), окружение без Python-гейта — не относится к этой задаче. + +**Browser smokes.** `node scripts/smoke-select.mjs --base origin/dev --head HEAD` +насчитал diff по 56 символам на изменённых строках `src/**` (4 файла), матрица +219 смоков, порог «широкого» символа — 43. Инструмент выдал 36 «прямое +совпадение» + 43 «слабая связь» (полный вывод учтён, не прикладываю целиком). + +Прогнаны лично (все green), 12 из 36 прямых совпадений плюс AC8: + +- `smoke_marker_write_rollback` (новый, AC1–AC3) — все 12 полей `true` +- `smoke_vacuum_firstuse` (расширен, AC4–AC7) — все 23 поля `true` +- `smoke_vacuum_multifloor`, `smoke_vacuum` — `OK` +- `smoke_vacuum_route_draft` (AC8, автор назвал явно) — все 21 поле `true` +- `smoke_dialog_zombie` (автор назвал явно) — все 8 полей `true` +- `smoke_v8_draft_write` — прямое совпадение на `_cfgContentFingerprint`, + `_cfgRev`, `_dropLegacySegments` (сигнатура которого реально изменилась: + необязательный параметр `config`), `_saveConfigDebounced` — 16 полей `true` +- `smoke_lattice_write_barrier` — прямое совпадение на `_writeConfig` + (сигнатура которого реально изменилась: параметр `attempt`) — 9 полей `true` +- `smoke_wall_thickness` — прямое совпадение на `_dropLegacySegments` — 18 полей `true` +- `smoke_save_race` — прямое совпадение на разделяемый writer/`_cfgRev` — 4 поля `true` +- `smoke_danger_confirmation` — прямое совпадение на `_markerDialog`/`_saveConfigNow` + (сигнатура изменилась: параметр `attempt`) — 18 полей `true` +- `smoke_config_writer` — прямое совпадение на `_cfgRev`/`_saveConfig` — `OK` + +Решение по каждой из непрогнанных 24 «прямых совпадений» (`smoke_decor_default_persist`, +`smoke_area_relocation_safety`, `smoke_controls`, `smoke_cover_tap`, +`smoke_device_inbox`, `smoke_device_position_history`, `smoke_device_preview_parity`, +`smoke_discovery_filters`, `smoke_editor_tabs`, `smoke_help_affordance`, +`smoke_optimize_coordinate_canonicalization`, `smoke_partition_openings`, +`smoke_room_resize`, `smoke_room_tooltip_toggle`, `smoke_sun`, `smoke_tap_ctx`, +`smoke_value_face_source`, `smoke_danger_confirm_branches`, `smoke_decor_images`, +`smoke_editor_gestures`, `smoke_fixed_floor`, `smoke_junction_limits`, +`smoke_resize_audit_1550`, `smoke_space_tab_reorder`, +`smoke_zero_wall_migration_unblocked`): не прогонялись. Совпадение у них — +на общие символы (`_cfgRev`, `_showToast`, `_writeConfig`, `_commitSpace`, +`_markerDialog`/`Marker`/`DevItem` как типы), а не на изменённое поведение; +ни один не тестирует Device editor save, calibration UX или rollback guard +напрямую. Сами изменённые функции (`_writeConfig`, `_saveConfigNow`, +`_dropLegacySegments`, `rollbackOptimistic`, `optimisticAttempt`) уже +прогнаны в 6 смоках выше плюс покрыты 1863 unit-тестами, включая +`test/coordinate-canonicalization.test.mjs` (#224-серия, строчная сверка +источника `_writeConfig`) и `test/serialized-write-queue.test.mjs`. Слабую +связь (43 смока) не прогонял — ни один не совпал на изменённом символе, +только на распространённых именах вне контракта записи. + +`npm run golden:verify` не гонял — diff не меняет видимый рендер, только +busy/disabled-атрибуты кнопок (`?disabled`, `aria-busy`); `check-docs` +подтвердил 0 изменённых PNG и единственно принятый `sourceFingerprint` +(скриншоты сделаны без калибровочного pending/reject UI, что ТЗ явно +допускает: «Новая постоянная golden surface не требуется»). + +`python -m pytest tests_backend` не гонял — diff не трогает +`custom_components/**/*.py`. + +Performance-профили не гонял — не названы в AC, diff не меняет +чувствительные к перфу пути (только optimistic write/rollback JS-логику +объёмом O(размер конфига), не в цикле рендера). + +## Разбор кода + +**`src/serialized-write-queue.ts`.** `rollbackOptimistic` больше не +принимает object-identity как самостоятельное основание для отката — +теперь всегда сравнивается `fingerprint(current) !== attempt.attemptedFingerprint`, +и для той же ссылки тоже. Это именно то, что требует AC2: «более новая +in-place правка не может быть затёрта старым reject». Проверено новым +unit-тестом (`newer in-place content on the attempted root also wins`) и +персонально перепроверенным мутантом `optimistic-rollback-skips-same-root-fingerprint`. + +**`src/houseplan-editor-runtime.ts` — `_saveMarker()`.** Строит `candidate` +глубоким клоном `cfg` **до** отправки (`JSON.parse(JSON.stringify(cfg))`), +не мутирует исходный `cfg`. Перед установкой `optimisticAttempt` есть явная +guard-проверка на смену корня во время async file-migrate шага +(`this.host._serverCfg !== cfg || this.host._cfgRev !== baseRevision || +contentFingerprint(cfg) !== baseContent`) — конфликт корректно показывает +`toast.conflict` и не строит candidate поверх устаревшего root. `attempt` +создаётся, кандидат устанавливается оптимистично, debounce отменяется, +`_saveConfigNow(attempt)` awaited. `configAccepted` выставляется в `true` +**только** после успешного `_saveConfigNow`; catch откатывает +(`rollbackOptimistic`) исключительно если `!configAccepted`. Layout +update/obsolete cleanup/file cleanup идут после этой границы и их отказ не +трогает `configAccepted` — соответствует контракту §1 ТЗ. Подтверждено +чтением и смоком `smoke_marker_write_rollback` (`sideEffectFailureKeepsAcceptedConfig`). + +**`_writeConfig(attempt)`.** Новая строка `if (attempt && candidateFingerprint +=== attempt.attemptedFingerprint) attempt.revision = this.host._cfgRev;` +решает реальную гонку: если попытка встала в очередь `_writeChain` позади +другой уже выполняющейся записи, `_cfgRev` к моменту фактической отправки +может уже уйти вперёд, и без ребиндинга guard в `rollbackOptimistic` увидел +бы устаревшую `attempt.revision` и **тихо не откатил** законно отклонённый +кандидат. Мутант `marker-rollback-keeps-enqueue-time-revision`, +удаляющий эту строку, красит `smoke_marker_write_rollback` — сценарий +«queue behind a prior write» там воспроизведён явно (строки 44–56 смока). + +**`src/vacuum-calibration-write.ts` (новый модуль).** `saveVacuumMatrix` +клонирует `previous`, находит или материализует minimal first-use marker +на клоне (не на `previous`), пишет матрицу через существующий +`writeVacuumMatrix()`, ставит `optimisticAttempt`, отменяет debounce, +ждёт `_saveConfigNow(attempt)`; на reject — `rollbackOptimistic` + +rebuild + toast. `saveAutomaticCalibration` дедуплицирует параллельные +Apply через `WeakSet`, держит `_markerDialog.busy` до ответа, +снимает его в `finally` независимо от исхода. `applyCalibrationProposal` +и `saveManualCalibration` следуют тому же паттерну busy→await→(close+toast +| restore-draft+busy:false). Все четыре пути подтверждены и unit-тестом +(`test/vacuum-calibration-write.test.mjs`, включая проверку `assert.notEqual` +на изолированность отката от live-объекта), и browser-смоком +(`smoke_vacuum_firstuse`), и мутантами. + +**`src/houseplan-card.ts`.** Только типизация (`VacuumFit`/`CalibrationProposal` +вместо инлайновых типов) и busy-wiring в разметке: `?disabled=${busy}` на +кнопках Auto-calibrate/Fit/Save/Cancel/Apply, `aria-busy` на диалоге +предложения и панели fit, Esc/scrim guard на `busy`. Согласуется с разделом +«Touch, клавиатура и доступность» ТЗ — busy это реальный disabled, а не +только визуальный индикатор. + +## Находки + +Ничего блокирующего. Два Low, оба вне обязательного скоупа задачи — +оставлены с запиской, не правятся и не заводятся отдельным issue (severity +ниже Medium, PROCESS.md §12 не требует issue для Low). + +1. **Low — `docs/specs/README.md`, таблица раздела «P2».** Новая строка + `#442` вставлена сразу после `#10`, а не в конце секции, где физически + находится диапазон номеров `#4xx` (например, `#440`, `#443` идут в + других местах таблицы). Секция и так не строго монотонна (`#294` после + `#373`, `#443` после `#434`), так что это чисто косметическая + навигационная неровность — `check-docs.mjs` не проверяет порядок, + ссылка рабочая. Не блокирует. + +2. **Low/наблюдение, не находка задачи — `_saveSettingsDialog()` (#439, + `src/houseplan-editor-runtime.ts:10260`, diff этой задачи не трогает).** + Этот путь по-прежнему вызывает `this._saveConfigNow()` **без** передачи + `attempt`, поэтому новый ребиндинг `attempt.revision = this.host._cfgRev` + в `_writeConfig` для него не работает — тот же класс гонки, который #442 + AC2 закрывает для маркеров (запись в очереди позади другой, `_cfgRev` + ушёл вперёд), теоретически остаётся в settings-пути. Это **не + регрессия этой задачи**: поведение не менялось, риск существовал уже + в #439, воспроизведение требует двух параллельных pending-записей и не + проверено мной в обе стороны в рамках бюджета этого раунда. Settings + dialog не входит в скоуп #442 (не задевает четыре marker semantic + validators) и явно исключён ТЗ («атомаризация всех 18 generic + `_saveConfig()` call sites» — не-скоуп). Оставляю как наблюдение для + будущей задачи, а не как находку, требующую отдельного issue сейчас — + недостаточно данных, чтобы утверждать это как подтверждённый дефект, а + не гипотетический край. + +## Что проверено и корректно + +- AC1–AC9 — каждый либо доказан автотестом, который я лично прогнал и чьи + свидетели проверены на способность покраснеть (mutation-gate), либо + browser-smoke-ом с явными полями результата, все `true`. +- Guarded rollback различает: обычный reject (откатывает), conflict reload + (не трогает authoritative reload), более новую in-place правку той же + ссылки (не трогает) — по коду и по unit/smoke сценариям. +- Durable boundary между `configAccepted` и layout/file side effects — по + коду и by `sideEffectFailureKeepsAcceptedConfig`/`sideEffectFailureHasNoSuccess` + в смоке. +- First-use synthetic marker не остаётся при reject — unit + (`rejected first-use calibration leaves no synthetic marker`) и смок + (`fitFreshNoMarker`, `highResidualRejectRestoresMatrix`). +- #441 (route CRUD) не задет — `smoke_vacuum_route_draft` зелёный целиком, + основной Save по коду не пишет `map_routes` напрямую (переносит блок + `vacuum` как есть в кандидат). +- Busy — настоящий disabled на кнопках/полях, Esc/scrim не воскрешают UI + во время pending — по коду и `smoke_vacuum_firstuse` + (`autoWaitsBusyDisabled`) плюс ручное чтение `_onKey`/шаблонов. +- Оба changelog обновлены в одном user-visible коммите (`fcd4e3dd`), + трейлеры `Issue:`/`User-Visible:` корректны на всех 5 коммитах диапазона. +- `docs/CONFIG-COMPATIBILITY.md` и `docs/VACUUM.md` формулировки совпадают + с фактическим контрактом кода (revision+content guard, durable boundary, + retry-draft). +- Screenshot provenance (`docs/images/screenshots.json`): только + `sourceFingerprint` изменился на всех 10 сценариях, все `imageSha256` + идентичны — реальных визуальных изменений нет, что ожидаемо (только + busy/disabled-атрибуты, не заснятые в текущей матрице). + +## Чего не проверял + +- Полный `mutation-gate` без `--id` (падает на несвязанном + backend-test-guard из-за отсутствия pytest в этом окружении — вне + диапазона задачи). +- 24 из 36 «прямое совпадение» и все 43 «слабая связь» browser smoke — + см. таблицу выше с обоснованием по каждой не прогнанной группе. +- `npm run golden:verify`, `performance_smoke`, полный Linux HA harness — + предрелизный гейт по процессу, не гейт код-ревью; diff не меняет + видимый рендер (только busy/disabled-атрибуты) и не трогает backend. +- Ручное подтверждение в реальном браузере (только автоматизированные + Playwright-смоки и unit-тесты). + +## Вердикт + +Зелёный. AC1–AC9 закрыты и доказаны воспроизводимо (тесты умеют падать — +проверено лично на 6 новых + 2 переехавших мутантах); дешёвые гейты и +целевая browser-смок-выборка зелёные; находок уровня High/Medium нет. + +--- + + + +## Материал раунда + +- Ветка: `issue/442-marker-write-rollback`, коммит `ffbf8242abf4` — ребейз его осиротит, и это нормально: ниже якоря, которые ребейз не меняет. +- Дерево материала: `4bc5d44a158273057ec9cf1cf34591def23ad7ce` + ``` + git log --all --format='%H %T' | grep 4bc5d44a1582 + ``` +- ТЗ `docs/specs/442-marker-write-rollback.md`, блоб `a9dcca2ff269a1c309387902875a4937efbd4ebf` + ``` + git log --all --find-object=a9dcca2ff269a1c309387902875a4937efbd4ebf -- docs/specs/442-marker-write-rollback.md + ```