From 75478905d5ebb457142eac51ae70c2d10eb1b0e1 Mon Sep 17 00:00:00 2001 From: "claude[bot]" <209825114+claude[bot]@users.noreply.github.com> Date: Thu, 3 Sep 2026 19:35:24 +0000 Subject: [PATCH] docs: review document for #441 Issue: #441 User-Visible: no --- docs/reviews/CODE-REVIEW-441-r1.md | 246 +++++++++++++++++++++++++++++ 1 file changed, 246 insertions(+) create mode 100644 docs/reviews/CODE-REVIEW-441-r1.md diff --git a/docs/reviews/CODE-REVIEW-441-r1.md b/docs/reviews/CODE-REVIEW-441-r1.md new file mode 100644 index 00000000..6f983a2b --- /dev/null +++ b/docs/reviews/CODE-REVIEW-441-r1.md @@ -0,0 +1,246 @@ +# CODE-REVIEW — issue #441 · заход r1 + +Ветка `issue/441-vacuum-route-draft`, коммит на ревью `fa41d298` (после +приведения к dev конвейером; исходный коммит автора — `014b7480`, +логически тот же диф). Заход первый, разбор полный. + +## Скоуп + +Баг: блок «Карты и этажи» писал второй и последующий маршрут пылесоса +с `space: ''`, что отвергают и семантический валидатор, и +voluptuous-схема. Кнопка «Добавить текущую карту» при этом оставалась +активной, а отклонённая запись ничего не откатывала. + +Три AC (trivial-трек, ТЗ = §9.2 из `docs/specs/162-vacuum-map-space-routing.md`): +- AC1 — черновик до выбора этажа; +- AC2 — только валидная атомарная запись; +- AC3 — отмена/отказ не оставляют фантом. + +Диф: `src/editors/vacuum-maps-section.ts`, `src/vacuum-route-edit.ts` +(новый черновичный API), `test/vacuum-routes.test.mjs`, +`demo/smoke_vacuum_route_draft.mjs` (новый), `scripts/smoke-links.mjs`, +`docs/CHANGELOG.md` / `.ru.md`, плюс однострочная правка вызова в +`src/houseplan-editor-runtime.ts` (убран неиспользуемый параметр +`setVac`). Backend, i18n, View, модель геометрии не тронуты — совпадает +с заявлением автора и с оценкой владельца. + +## Как проверялось + +Ручного тестирования в цикле нет, поэтому доказательства — тест +(с проверкой, что он умеет падать) и чтение кода. + +**AC1** — `beginVacuumRouteDraft` (`src/vacuum-route-edit.ts:30-35`): +`space: routes.length ? '' : dockSpace`. Первый маршрут (routes.length +== 0, включая виртуальные legacy-маршруты через `effectiveRoutes`) +наследует этаж дока, второй и следующие — пустая строка. Кнопка +«Сохранить» в черновике: `?disabled=${!!draft.saving || +!spaceIds.has(draft.space)}` (`vacuum-maps-section.ts:283`) — недоступна, +пока `space` не входит в множество существующих этажей. Доказано +юнит-тестом `test/vacuum-routes.test.mjs:279` (АC1) — **проверил, что +тест умеет падать**: временно вернул старую формулу +`space: dockSpace` (без `routes.length ?`), тест 25 упал +(`not ok 25`), после отката — снова зелёный (`node --test +test/vacuum-routes.test.mjs`: 32/32). Дополнительно смоком +`demo/smoke_vacuum_route_draft.mjs` (`firstRoutePreselectsDock`, +`secondCurrentStartsBlank`, `separateSourceStartsBlank`, +`blankDraftCannotSave`) — прогнан, зелёный. + +**AC2** — `commitVacuumRouteDraft` (`vacuum-route-edit.ts:45-56`) +возвращает `null`, если `space` не входит в `spaceIds`, либо если пара +`source`+`map_id` уже существует среди маршрутов; иначе — ровно один +новый маршрут через `addRoute`. `writeRoutes` в `vacuum-maps-section.ts:126-137` +конвертирует legacy весь целиком (`convertLegacyRoutes`, не тронут +этим дифом) и передаёт кандидат в `persistRoutes` только если он не +`null` — невалидная запись никогда не доходит до сохранения. Доказано +юнит-тестом `test/vacuum-routes.test.mjs:287` (AC2, включая проверку +«одну карту нельзя добавить дважды») и смоком +(`noEmptySpaceEverSubmitted`, `rejectedPayloadWasValid`, +`retryKeepsExactIdentity`). + +**AC3** — `persistRoutes` (`vacuum-maps-section.ts:89-123`) сохраняет +`previous = host._serverCfg` до применения, использует +`optimisticAttempt`/`rollbackOptimistic` (`src/serialized-write-queue.ts`) +для отката именно этой попытки (защищено сверкой `_cfgRev` — чужая +более новая правка откатом не будет затёрта). `confirmDraft` +(`vacuum-maps-section.ts:240-255`) при отказе восстанавливает исходный +`current` (без `saving: true`), при успехе — удаляет черновик; в обоих +случаях `pendingRoute` не трогается, если за время ожидания +изменилась identity черновика (гонка с параллельным закрытием +диалога). Отмена (`cancelDraft`) не вызывает `writeRoutes` вовсе — +конфигурация не меняется. Доказано смоком: `cancelLeavesAcceptedRoutes`, +`rejectionRestoresAcceptedMarker` (после синтетического отказа +бэкенда первый маршрут `r1` не изменился), `rejectionKeepsEditorUsable` +(черновик остаётся с выбранным этажом, повторное сохранение проходит +без переоткрытия диалога). + +Прочитал also `docs/SCOPE.md` (J4/J6 — штатная настройка через GUI и +поддержание валидности конфига), `AGENTS.md`/`PROCESS.md` (формат +вердикта, трейлеры), issue #441 целиком и все три комментария +(аналитика, взятие в работу, отчёт о реализации). + +## Гейты — что прогнал и с каким результатом + +- `npx tsc --noEmit` — чисто, без вывода. +- `npm test` — 1857 тестов, 1856 passed, 1 skipped, 0 fail (совпадает + с заявлением автора). Отдельно перепрогнал + `test/vacuum-routes.test.mjs` и подтвердил, что новые тесты AC1/AC2 + падают на буквальном откате фикса (см. выше) — дисциплина + «тест умеет падать» соблюдена для прогнанных тестов. +- `npm run build` и `npm run bundle:sync` — зелёные, три копии бандла + (`dist/`, `custom_components/houseplan/frontend/`, + `demo/srv/assets`) синхронны и байт-в-байт совпадают с + закоммиченными после пересборки — working tree после прогона чист. +- `node scripts/check-docs.mjs` — **КРАСНЫЙ**: `ERROR screenshot + source fingerprint is stale; run npm run build && node + demo/docs/capture.mjs`. Перепроверил на `origin/dev` (тот же + чекаут скриптов) — там гейт зелёный («Documentation checks passed»), + то есть красный статус — следствие именно этого дифа (правка + `src/editors/vacuum-maps-section.ts` затронула отпечаток всего + `src/**`), а не унаследованная проблема. Автор не называет этот шаг + в своём списке проверок. См. находку ниже. +- `node scripts/smoke-select.mjs --base origin/dev --head HEAD` — + 32 символа на изменённых строках, 23 «прямых совпадения» + 23 «слабые + связи», НЕОПРЕДЕЛЁННОСТИ нет. Все прямые совпадения — это общие + символы записи конфига (`_cfgRev`, `_saveConfigDebounced`, + `_cfgContentFingerprint`, `_showToast`, `DevItem`, `Marker`), + которые встречаются в диффе только потому что `persistRoutes` + впервые применяет к путям retarget/drop уже существующий разделяемый + механизм `optimisticAttempt`/`rollbackOptimistic` (сам механизм этим + дифом не тронут — правка только в вызывающем коде одного файла). + Прогнал точечно, а не весь список: `demo/smoke_vacuum_route_draft.mjs` + (новый, основной), `demo/smoke_vacuum_firstuse.mjs` и + `demo/smoke_vacuum_multifloor.mjs` (тема диффа — vacuum-раздел + редактора), `demo/smoke_save_race.mjs` и `demo/smoke_config_writer.mjs` + (тестируют именно `optimisticAttempt`/`rollbackOptimistic`/debounce, + которые `persistRoutes` теперь использует). Все пять — зелёные. + Остальные 18 прямых и 23 слабых совпадения не прогонял: правка не + трогает их собственную логику (decor, sun, discovery, room resize, + area relocation и т.д. используют те же общие поля конфигурации, но + не код, который правка меняет), а общий примитив записи проверен + выше двумя целевыми смоками. +- `npm run golden:verify` — не прогонял. Диф ограничен диалогом + редактора устройства (лениво загружаемый блок); ни один golden-сценарий + (`demo/golden/matrix.mjs`) не рендерит этот диалог — они снимают + View/canvas. Видимый результат View не меняется. +- `python -m pytest tests_backend -q` — не прогонял, диф не трогает + `custom_components/**/*.py` (только generated JS-бандл). + Perf-профили — не прогонял, в AC не названы, путь не + perf-чувствительный. +- `npm run invariants -- --config ...` — не прогонял. Диф пишет + `marker.vacuum.map_routes[].space` — по форме похоже на «ссылку на + геометрию», но `scripts/model-invariants.mjs` эту ссылку не + проверяет вовсе (там только `marker.vacuum.segment_map` → + `roomId`, см. `model-invariants.mjs:150-155`); рёбра комнат, записи + толщины, `layout`, `open_spans`, `marker.space` (то есть верхнеуровневая + привязка маркера к этажу, а не маршрут пылесоса) этим дифом не + затронуты. Собственная защита от dangling-ссылки — как раз предмет + AC2 (`spaceIds.has(draft.space)`), проверена юнит-тестом и падением + выше. +- `test/single-source-numbers.test.mjs` — прогнан отдельно (входит в + `npm test`), 3/3 зелёных. Дублирующегося пользовательского числа + диф не добавляет: `mapId`/`space` — идентификаторы, а не измеряемая + величина, и в черновике, и в подтверждённой строке читаются из + одного и того же значения (`currentMapId`/`host._vacObservedMapId`), + второго источника нет. +- Трейлеры коммита `fa41d298`: `Issue: #441`, `User-Visible: yes` — + оба CHANGELOG (`docs/CHANGELOG.md`, `docs/CHANGELOG.ru.md`) правлены + в этом же коммите, формулировки соответствуют реализации. + +## Находки + +### Medium (в скоупе, чинится в этой же ветке) — стал причиной жёлтого вердикта + +**Отпечаток скриншотов документации устарел, `check-docs.mjs` красный +на `fa41d298`.** +- Файл: `docs/images/screenshots.json` (не обновлён), причина — + `src/editors/vacuum-maps-section.ts`. +- Воспроизведение: `node scripts/check-docs.mjs` на текущем HEAD → + `ERROR screenshot source fingerprint is stale; run npm run build && + node demo/docs/capture.mjs`; тот же скрипт на `origin/dev` проходит + чисто — красный статус специфичен для этого дифа. +- Почему это не мелочь: ровно этот пропуск в #230 и #234 оставил + `dev` с красным job `docs` до следующей задачи (#237) — то есть при + мёрже без правки CI-джоб `docs` станет красным на `dev` для всех + последующих задач, пока кто-то не поймает и не почини́т его + отдельно. +- Что нужно: `npm run build && node demo/docs/capture.mjs`, закоммитить + обновлённые `docs/images/screenshots.json` и (если изменились пиксели) + сами PNG, затем перепрогнать `node scripts/check-docs.mjs` до + зелёного. + +Больше High/Medium в скоупе не найдено. AC1–AC3 выполнены и доказаны; +рассмотренный продуктовый сценарий (второй и следующие маршруты через +штатный UI) решается, регрессии в соседний сценарий (первый маршрут, +retarget, drop, легаси-конверсия, `trail_mode`/`live`) не внесено — +подтверждено чтением и прогоном смоков. + +## Что проверено и корректно (без отдельной находки) + +- Сигнатура `renderVacuumMapsSection` лишилась неиспользуемого + параметра `setVac`; единственный вызов в + `src/houseplan-editor-runtime.ts:10973` обновлён синхронно, `tsc` + подтверждает отсутствие расхождений. +- Побочный эффект дифа: `retarget`/`drop` (переещё раньше существовавшие + операции) раньше писали через `this._saveConfig()` (дебаунс, без + отката при отказе бэкенда — тот самый пробел, который отдельно + трекает #439) и теперь используют тот же атомарный + `persistRoutes`/`rollbackOptimistic`, что и новый draft-флоу. Это не + требовалось ни одним AC #441, но не является регрессией: это + строгое усиление (retarget/drop теперь тоже откатываются при + отказе), сам примитив уже покрыт `smoke_save_race.mjs` и + `smoke_config_writer.mjs` (оба прогнаны и зелены выше). Отмечаю как + наблюдение: выделенного браузерного смока именно на отказ + бэкенда при retarget/drop нет ни до, ни после этого дифа — не + блокирует, т.к. общий примитив протестирован, а AC #441 этого пути + не касаются. +- `.vacroute`/`.pending`/`.vacroute-draft-*` не имеют выделенных CSS-правил + нигде в дереве (`grep -rn vacroute src --include=*.ts` — только сам + файл раздела) — это унаследовано от #162, не введено этим дифом. +- Использование `host._saveConfigDebounced.cancel()` (а не `.flush()`, + как в большинстве других мест `houseplan-card.ts`/`houseplan-editor-runtime.ts`) + — корректно: `persistRoutes` сразу после отмены сам вызывает + `_saveConfigNow()` с уже актуальным `_serverCfg` (включающим любые + ожидавшие правки), поэтому `cancel()` избегает второго избыточного + сетевого вызова, а не теряет данные. +- Новых ключей i18n нет (`git diff` по `src/i18n/**` пуст) — совпадает + с заявлением аналитики в комментарии владельца. +- `docs/SCOPE.md`: задача закрывает J4 (штатная GUI-настройка без + YAML) и J6 (конфиг не должен становиться невалидным) — соответствует + заявке автора, продуктовое обоснование в скоупе. + +## Чего не проверял + +- Ручной запуск карточки в браузере вне смоков (нет такого шага в + этом цикле по правилам процесса) — заменено чтением кода и + прогоном `demo/smoke_vacuum_route_draft.mjs`, который управляет + теми же DOM-элементами, что и живой пользователь. +- 18 из 23 «прямых совпадений» и все 23 «слабые связи» из + `smoke-select.mjs` — см. обоснование в разделе «Гейты» (общий + механизм записи, а не специфика этого дифа, уже покрыт двумя + целевыми смоками). +- `npm run golden:verify`, `pytest tests_backend`, `npm run + invariants`, perf-профили — не запускал, обоснование по каждому — в + разделе «Гейты». +- Многопользовательский конфликт (два клиента редактируют один и тот + же маркер одновременно, оба добавляют разные маршруты) — не тестовый + сценарий этого issue; общая защита от такого конфликта — существующий + `_cfgRev`/`rollbackOptimistic`, не изменённый этим дифом. + +## Вердикт + +Жёлтый: AC выполнены и доказаны, но `check-docs.mjs` красный на +проверяемом SHA из-за пропущенного шага пересъёмки — Medium в скоупе, +возврат автору на прогон `npm run build && node demo/docs/capture.mjs` +и коммит обновлённого манифеста/скриншотов. + +--- + + + +## Материал раунда + +- Ветка: `issue/441-vacuum-route-draft`, коммит `014b74805533` — ребейз его осиротит, и это нормально: ниже якоря, которые ребейз не меняет. +- Дерево материала: `a372d3d387b4eefe464eda9079fa930f4d3f896c` + ``` + git log --all --format='%H %T' | grep a372d3d387b4 + ```