diff --git a/docs/reviews/CODE-REVIEW-74-r2.md b/docs/reviews/CODE-REVIEW-74-r2.md new file mode 100644 index 00000000..fe6eacad --- /dev/null +++ b/docs/reviews/CODE-REVIEW-74-r2.md @@ -0,0 +1,147 @@ +# CODE-REVIEW-74-r2 + +- **Issue:** https://github.com/Matysh/houseplan-card/issues/74 +- **Этап:** код-ревью (PROCESS.md §2.7) +- **Материал:** дельта `fc63f3bf36687cd98c9777ce683d2f2e49d39ff6..6d73d6e9` (SHA r1 → HEAD текущего + захода), ветка `issue/74-device-position-undo`, коммиты в дельте: `a6b0850a`, `4b5c9f23`, `2e30daa3`, + `6d73d6e9`. Полный диапазон `origin/dev..HEAD` не менялся структурно (10 коммитов, тот же перечень, + что и в r1, плюс эти четыре). +- **Заход:** r2 · блокирующих циклов израсходовано 1 из 4 (потрачен предыдущим жёлтым r1) +- **Вердикт:** зелёный +- **High:** 0 · **Medium:** 0 · **Low:** 0 + +## Скоуп разбора + +Разбор по дельте (PROCESS.md §2.9): предыдущий вердикт (r1, жёлтый, SHA `fc63f3bf`) закрыт одной +Medium-находкой и не тронул продуктовый код (`src/**` в дельте не изменялся — проверено +`git diff --stat fc63f3bf..HEAD -- 'src/**' 'custom_components/**'`, пусто). Дельта — только demo-харнесс +иконок, тестовая фикстура смока и канонический пересчёт docs-скриншотов. Изменение не является +ребейзом на ушедший вперёд `dev` (origin/dev не сдвигался относительно r1), не меняет контракт +поведения и не задевает новую подсистему — сокращение объёма разбора оправдано. + +Функциональные AC1–AC14 (транзакционная модель drag/preview/commit/abort, LIFO/50 глубина, +cross-space undo, fail-closed инвалидация, unknown-fields preservation, own/external revision echo, +serialised writes, rollback направления стека) дельтой не задеты — унаследованы из r1 без повторной +проверки, см. раздел ниже. + +## Закрытие раунда r1 + +| Находка r1 | Чем закрыта | Где это видно | +|---|---|---| +| **Medium-1**: Undo/Redo toolbar без иконок в demo/golden-харнессе (`mdi:undo-variant`/`mdi:redo-variant` отсутствовали в `demo/srv/assets/icons.js`), включая уже закоммиченный `docs/images/06-device-editor.png` | `demo/srv/assets/icons.js` пересобран штатным `node demo/gen_icons.mjs` (коммит `2e30daa3`) — обе иконки присутствуют; я независимо перегенерировал файл тем же скриптом и получил байт-в-байт идентичный результат (`diff -q` → identical), т.е. изменение не ручное. Весь канонический набор `docs/images/*.png` пересобран workflow `Docs screenshots` и закоммичен (`6d73d6e9`) — я открыл `docs/images/06-device-editor.png`: обе круглые стрелки Undo/Redo отрисованы рядом с кнопкой закрытия панели, вместо пустых кнопок из r1. | `git diff fc63f3bf..HEAD -- demo/srv/assets/icons.js` (добавлены ключи `mdi:undo-variant`/`mdi:redo-variant`); визуальный просмотр `docs/images/06-device-editor.png` на HEAD; `node scripts/check-docs.mjs` → passed на HEAD | +| **Low-1**: LIFO для двух устройств не показан реальным двойным drag в смоке | Не тронуто — принято r1 без действия (независимость команд по `deviceId` в `CommandStack`), эта дельта его не касается | наследуется из r1, действий не требовалось | +| **Low-2**: AC13 проверено на одной ширине/теме | Не тронуто — принято r1 без действия (desktop-first, риск низкий) | наследуется из r1, действий не требовалось | + +Дополнительно за раунд r2 самостоятельно обнаружена и в этой же дельте закрыта смежная проблема, +не поднятая r1: `demo/smoke_align_guides.mjs` в разделе «редактор устройств: drag значка» на SHA +`fc63f3bf` конструировал состояние драга через устаревшее поле `c._drag = { id: b.id, ... }`. Реализация +#74 (коммит `bdf81fad`) вынесла drag устройств в отдельное поле `_deviceDrag` (тип `DeviceDragState`, +`src/houseplan-card.ts:800-813`), оставив `_drag` только для drag ярлыков комнат (`rl_*`, `src/houseplan-card.ts:12350`). +Гид-логика для режима `devices` проверяет именно `this._deviceDrag` (`src/houseplan-card.ts:12357`), поэтому +после рефактора старая фикстура молча переставала проверять что-либо: `out.devGuide` был `false`, а смок не +входил в список гейтов ни у автора реализации, ни в r1-ревью — регрессия оставалась незамеченной. +Коммит `a6b0850a` («test: update device alignment guide fixture») чинит фикстуру, конструируя `_deviceDrag` +по актуальной форме (`spaceId`, `displayName`, `pointerId`, `source`, `before`, `start: c._devicePlacementForCanvas(...)`) — +поля точно совпадают с интерфейсом `DeviceDragState`. Не блокирует (уже исправлено в этой же дельте), но +фиксирую как находку, закрытую собственноручно, а не «как есть». + +## Унаследовано из r1 + +Без повторной проверки принято из `docs/reviews/CODE-REVIEW-74-r1.md` (SHA `fc63f3bf36687cd98c9777ce683d2f2e49d39ff6`, +файл присутствует в дереве HEAD, закоммичен как `4b5c9f23`): + +- продуктовая рамка и соответствие J6 (`docs/SCOPE.md`) — рамка не менялась с r1, дельта её не задевает; +- AC1–AC14 функционально доказаны (зелёные `tsc`, `npm test` 1633/1633 на момент r1, `build`+сверка бандла, + `check-docs`, `no-new-any`, `coordinate-write-barrier-guard`, `process-gate --issues`, целевые смоки + `smoke_device_position_history`/`smoke_drag_bounds`/`smoke_modes`/`smoke_pan_any_zoom`/`smoke_grid_snap`/ + `smoke_editor_tabs`/`smoke_layout_sync`, два точечных мутационных теста + `mutation-gate.mjs --id=device-position-cancel-routed-to-commit` и `--id=stale-space-position-guard-removed`); +- транзакционная модель drag/preview/commit/abort, fail-closed инвалидация истории, cross-space undo, + восстановление направления стека при неудачном Undo — проверены r1 и по коду, и по прогону; + `src/**` в дельте r2 не менялся, значит эти доказательства актуальны без повторного прогона; +- трейлеры `Issue`/`User-Visible` на коммитах диапазона `origin/dev..HEAD` (кроме новых в дельте, см. ниже) — + проверены r1 через `process-gate.mjs --issues`, я перепрогнал тот же гейт на HEAD (см. «Что проверено сам»), + расхождений нет. + +Основание доверия: дельта не трогает `src/**`/`custom_components/**` (проверено `git diff --stat`), не меняет +контракт поведения, не ребейзится на ушедший вперёд `dev` — origin/dev не сдвигался с момента r1. + +## Что проверено самостоятельно в r2 (зелёного Validate на SHA `6d73d6e9` не было) + +- `npx tsc --noEmit` — чисто, без вывода; +- `npm test` — 1634 тестов, 1633 passed, 0 failed, 1 skipped, 0 cancelled; +- `npm run build` — прошла (`tsc --noEmit && rollup -c`); +- `npm run bundle:sync` — прошла, три копии бандла синхронизированы (`custom_components/houseplan/frontend`, + `demo/srv/assets`), дерево осталось чистым (`git status --short` пусто после); +- `node scripts/check-docs.mjs` — «Documentation checks passed (7 files, 10 external links)» — прогнан, + хотя дельта `src/**` не трогает и по правилу не обязателен; прогнан для подтверждения фикса Medium-1; +- `node scripts/process-gate.mjs --base origin/dev --head HEAD --issues` — «гейт пройден, предупреждений 0» + на полном диапазоне 10 коммитов; +- `node demo/smoke_align_guides.mjs` — прогнан лично (автор его не прогонял, хотя менял этим же коммитом). + Результат: все 9 проверок `true`, включая `devGuide`/`devGuideGone`. Дисциплина «тест умеет падать»: + локально откатил правку фикстуры к старой форме `c._drag = {...}` (без изменения истории репозитория, + файл восстановлен из бэкапа сразу после), перезапустил — `devGuide: expected true, got false`, смок упал. + Это подтверждает, что тест реально ловит регрессию, а не просто существует. Рабочее дерево вернул в чистое + состояние (`git status --short` пусто); +- `node demo/gen_icons.mjs` — регенерация даёт байт-в-байт тот же `demo/srv/assets/icons.js`, что уже + закоммичен (`diff -q` → identical); сообщение генератора «icons: 155 of 157 referenced» одинаково + и до, и после фикса (проверено на SHA `fc63f3bf` тем же скриптом) — оставшиеся 2 нерезолвленные иконки + не связаны с #74 и не регрессия этой дельты; +- `node scripts/smoke-select.mjs --base fc63f3bf --head HEAD` — вывод: «Исполняемого frontend-диффа нет + (`src/**/*.ts` не тронут). Browser-smoke этим диффом не выбираются — это не пропуск проверки, а + «выбирать нечего»: смоки проверяют собранную карточку». Согласуется с тем, что дельта не трогает `src/**`; + единственный смок, который я всё же прогнал (`smoke_align_guides.mjs`), выбран не инструментом, а потому + что его правит сам коммит дельты; +- визуальный просмотр `docs/images/06-device-editor.png` на HEAD — иконки Undo/Redo отрисованы корректно, + рядом с кнопкой закрытия панели. + +Проверка поиска остаточных экземпляров того же класса бага: `grep -rn "_drag = {" demo/smoke_*.mjs` — +кроме исправленного `smoke_align_guides.mjs`, есть только `smoke_drag_bounds.mjs`/`smoke_grid_snap.mjs` +(легитимно используют `_drag` для `rl_*`, drag ярлыков комнат — отдельное поле, не задето рефактором) и +`smoke_optional_space_model.mjs` (использует `_drag` как generic-сентинел для проверки сброса состояния +при смене модели, не про гид-логику устройств — вне скоупа дельты, не трогался). Других скрытых поломок +того же паттерна не найдено. + +## Одно число — один источник + +Дельта не добавляет и не меняет ни одной величины, видимой пользователю (иконки — не число; скриншоты — +переснятое представление уже существующего UI). `test/single-source-numbers.test.mjs` прошёл в составе +`npm test`. Раздел неприменим по существу изменения. + +## Чего не проверял и почему + +- **`npm run golden:verify` полным набором** — не прогонял. Golden baseline для `geometry-devices-editor-dark` + сознательно не принимается на этапе код-ревью: по принятому ТЗ и по замечанию автора это Linux + prerelease-gate (PROCESS.md §8), требующий полного CI-артефакта и трейлера `Baseline-Reviewed` — не + инструмент код-ревью r2. Дельта не меняет продуктовый рендер (`src/**` не тронут), только demo-иконки и + docs-скриншоты, поэтому визуальной проверки одного PNG было достаточно для подтверждения самого фикса. +- **Браузерные смоки, кроме `smoke_align_guides.mjs`** — `scripts/smoke-select.mjs` не выбрал ни одного + (дельта не трогает `src/**/*.ts`); остальные 6 смоков, прогнанных в r1 на функциональность AC, дельтой не + задеты и не перезапускались — их доказательная сила унаследована из r1 (см. выше), а не заново подтверждена. +- **`python -m pytest tests_backend -q`** — не прогонял, `custom_components/**/*.py` в дельте не изменялся + (backend вне скоупа #74 с самого ТЗ). +- **`npm run invariants -- --config ...`** — не прогонял, дельта не трогает геометрию/рёбра/`layout`/ + `marker.space`/`open_spans`; `npm test` уже гоняет инварианты на моделях проекта и прошёл. +- **`npm run bundle:budget`** — не перепрогонял отдельно в r2 (запускал `bundle:sync`, который включает + `build`; бюджет по факту не мог измениться — иконки/тесты/докс не входят в бандл карточки, а последнее + измерение автора на этом же HEAD — 281796 B gzip, запас 18204 B). Если нужен формальный повтор — дёшево, + могу прогнать по запросу. +- **`performance_smoke`** — не тронут AC и не назван в ТЗ, дельта не содержит перф-чувствительных путей. + +## Трейлеры + +Все 4 коммита дельты несут `Issue: #74` и `User-Visible: no`. Признаю `User-Visible: no` корректным: +изменения видимы только в demo/docs-харнессе (`demo/srv/assets/icons.js`, `docs/images/*`), а не в +поставляемом продукте (`custom_components/**`, `dist/**` из этих коммитов не менялись содержательно — +только синхронизированы `bundle:sync` при моей же проверке, без изменения диффа). Правки в оба changelog +не требуются, т.к. User-Visible: no. + +## Вывод + +Единственная Medium-находка r1 закрыта полно и проверяемо: причина (устаревший demo icon-набор) устранена +у корня штатным генератором, следствие (документационный скриншот) переснято каноническим workflow, +я подтвердил результат независимо (регенерация иконок, визуальный просмотр PNG, docs-гейт). Обе Low-находки +r1 остаются некритичными записями, действий не требовали и не требуют. Дополнительно в этой же дельте +устранена не связанная с Medium-1, но реально существовавшая скрытая поломка регрессионного смока +(`smoke_align_guides.mjs`), самостоятельно найдена, проверена по коду и перепрогнана мной с подтверждением +«тест умеет падать». Новых находок, блокирующих зелёный вердикт, нет.