mirror of
https://github.com/Matysh/houseplan-card
synced 2026-09-29 03:09:36 +00:00
@@ -0,0 +1,231 @@
|
||||
# CODE-REVIEW-396-r2
|
||||
|
||||
Issue: [#396](https://github.com/Matysh/houseplan-card/issues/396) — регресс камеры после #82:
|
||||
прерванный зум не сохраняется (B1), точка под курсором уезжает при быстрой серии
|
||||
нотчей (B2), glow-feather не заморожен на время перехода (M2).
|
||||
|
||||
Материал: SHA `f3620f2b` (`origin/issue/396-camera-transition-fixes`), рабочее
|
||||
дерево чистое, `origin/dev` полностью содержится в ветке (`git merge-base
|
||||
--is-ancestor origin/dev HEAD` → true).
|
||||
|
||||
Заход: r2 · блокирующих циклов израсходовано 0 из 4 (зелёный вердикт r1 цикла
|
||||
не тратит, #227).
|
||||
|
||||
## Почему этот раунд разбирается полностью, а не по дельте
|
||||
|
||||
Раунд r1 (SHA `c1b6a01b`) получил **зелёный** вердикт: High 0 / Medium 0. Пока
|
||||
ревью шло, `dev` продвинулся на 11 коммитов; конвейер сам отменил слияние
|
||||
(«ветка изменилась после проверенного материала», #312) и вернул задачу в
|
||||
`S6-in-progress` для ребейза — это не находка ревью, а механика §7.2. Автор
|
||||
перебазировал ветку на `dev` (HEAD стал `f3620f2b`) и заявил, что продуктовый
|
||||
код не изменился ни на строку: «дельта против отревьюженного `c1b6a01b` ровно
|
||||
нулевая по `src/**`, `test/**`, `demo/**` и `scripts/**`».
|
||||
|
||||
По §2.10 ребейз на ушедший вперёд `dev` — один из явно перечисленных случаев,
|
||||
где «разбор остаётся полным»: после ребейза это другой код, даже если diff по
|
||||
существу пуст. Коммит `c1b6a01b` переписан рёбейзом и недостижим (`git
|
||||
cat-file -t c1b6a01b` → `fatal: Not a valid object name`), так что построчная
|
||||
сверка «что изменилось с r1» технически невозможна — единственный корректный
|
||||
способ подтвердить заявление автора — независимо перечитать весь diff задачи
|
||||
(`origin/dev...HEAD`) и прогнать гейты заново, что и сделано ниже.
|
||||
|
||||
Результат независимой проверки: заявление автора подтвердилось. Диф задачи
|
||||
(`git diff --stat origin/dev...HEAD`) содержит правку одного продуктового
|
||||
файла (`src/houseplan-card.ts`, 38 строк), тестов/мутантов/спеки — и класс D
|
||||
(`dist/**`, `custom_components/.../frontend/**`) плюс документацию
|
||||
(changelog, `docs/images/*`, `docs/reviews/*`). `src/viewport-transition.ts`
|
||||
диф не тронул вовсе. Никакого нового продуктового изменения ребейз не внёс.
|
||||
|
||||
## Скоуп
|
||||
|
||||
- **B1**: `_cancelCameraTransition` получил второй аргумент `keepPresented`;
|
||||
единственный вызывающий с `true` — `_stagePointerDown` (буквальный сценарий
|
||||
issue). Различение «пользовательская/структурная» отмена также записано в
|
||||
§13 спеки #82.
|
||||
- **B2**: `_cameraTargetAt` получил флаг `animated`; для колеса и кнопок якорь
|
||||
читается из цели перехода (`_cameraTransition.target`), а не из
|
||||
представленного (отстающего) кадра. §10 спеки #82 приведён в соответствие.
|
||||
- **M2**: гейт заморозки glow-feather переехал с `!_pinchStart && !_panStart`
|
||||
на `cameraStill`, включающий `!_cameraTransition.active`.
|
||||
|
||||
Классы файлов в диффе: A (`src/houseplan-card.ts`), B (`test/**`,
|
||||
`demo/smoke_smooth_zoom.mjs`, `scripts/mutation-gate.mjs`), C (changelog,
|
||||
`docs/specs/082-smooth-zoom.md`, `docs/specs/396-*.md`, `docs/reviews/*`), D
|
||||
(`dist/**`, `custom_components/.../frontend/**`).
|
||||
|
||||
## Как проверялось — таблица гейтов
|
||||
|
||||
| Гейт | Команда | Результат |
|
||||
|---|---|---|
|
||||
| Typecheck | `npx tsc --noEmit` | чисто |
|
||||
| Unit | `npm test` | 1654 pass / 0 fail / 1 skipped (1655 всего) |
|
||||
| Build + сверка бандла | `npm run build` → `git status --short` пуст; `cmp dist/houseplan-card.js custom_components/houseplan/frontend/houseplan-card.js` | MATCH, дерево class D синхронизировано |
|
||||
| Docs fingerprint | `node scripts/check-docs.mjs` (diff трогает `src/**`) | «Documentation checks passed (7 files, 10 external links)» |
|
||||
| No new `any` | `node scripts/no-new-any.mjs --base origin/dev --head HEAD` | «Новых any нет» (33 добавленных строки в 1 файле) |
|
||||
| Bundle budget | `npm run bundle:budget` | 285 416 / 300 000 Б gzip, headroom 14 584 Б (warning про общий запас — предсуществующий, issue #367, не относится к #396: диф добавил ~55 Б) |
|
||||
| Смоки, прямое совпадение (11/11, `node scripts/smoke-select.mjs --base origin/dev --head HEAD`) | `node demo/smoke_{smooth_zoom,long_press_gesture,canvas_frame,decor,editor_gestures,kiosk_pan_lock,kiosk,pan_any_zoom,warm_dialogs,zoom_out,infinite_canvas}.mjs` | все 11 зелёные, exit 0 (см. ниже про `infinite_canvas`) |
|
||||
| Golden (визуал в покое) | `npm run golden:verify` | 147/147 passed, 0 расхождений |
|
||||
| process-gate (офлайн) | `node scripts/process-gate.mjs --range origin/dev..HEAD` | «гейт пройден, предупреждений 0», 10 коммитов |
|
||||
| model-invariants | не запускался | diff не трогает геометрию/рёбра/`layout`/`marker.space`/`open_spans` — камера и рендер, не модель плана |
|
||||
| `pytest tests_backend` | не запускался | diff не трогает `custom_components/**/*.py` |
|
||||
| performance_smoke / полная матрица смоков | не запускались | предрелизный гейт (§8); задача не задевает всё дерево смоков, локально достаточно прямых совпадений |
|
||||
|
||||
### Смок `demo/smoke_infinite_canvas.mjs`
|
||||
|
||||
r1 отметил его как красный (`backendAcceptsFarPayload`) независимо от #396 (тот
|
||||
же результат на чистом `origin/dev`, проверено автором переключением ветки). В
|
||||
этой песочнице смок прошёл **зелёным** (все 22 проверки true, exit 0) — видимо,
|
||||
окружение здесь предоставляет бэкенд, которого не было у автора. В любом
|
||||
случае результат не блокирует: зелёный здесь, а расхождение с окружением
|
||||
автора — не регрессия #396 и не относится к этой задаче.
|
||||
|
||||
## Находки
|
||||
|
||||
Нет находок ни High, ни Medium, ни Low.
|
||||
|
||||
Прочитан и лично проверен весь измененный продуктовый код:
|
||||
|
||||
- `_cancelCameraTransition(commitTarget, keepPresented)` (`src/houseplan-card.ts:1257`):
|
||||
`presentedZoom` читается из `this._cameraTransition.presented?.zoom` **до**
|
||||
вызова `cancel()`. Проверено чтением: `_applyCameraTransitionFrame` (частота
|
||||
— каждый кадр RAF, включая первый синхронный кадр в `start()`) держит
|
||||
`this._zoom === presented.zoom` в течение всего активного перехода, поэтому
|
||||
`_saveZoom()`, вызванный сразу после `cancel(false)`, пишет ровно показанное
|
||||
значение — переменная `presentedZoom` используется только как флаг
|
||||
«был ли активный переход», сама она в запись не идёт, но это не ошибка:
|
||||
`this._zoom` к моменту вызова уже равен тому же числу. Единственный
|
||||
вызывающий с `keepPresented=true` — `_stagePointerDown:6433`; остальные 10
|
||||
мест отмены плюс `dispose()` (`:2781`, минуя обёртку) сверены построчно —
|
||||
ни один не передаёт `true` по ошибке.
|
||||
- `_cameraTargetAt(..., animated)` (`:6298`): `animated=true` передаётся только
|
||||
из `_onWheel` (`:6346`) и `_stepZoom` (`:6362`) — единственных двух путей,
|
||||
где может существовать незавершённый tween и где §10 спеки требует якорь из
|
||||
цели. `_zoomAt` (pinch, immediate, `:6325`) сначала зовёт
|
||||
`_cancelCameraTransition(false)`, после чего `_cameraTransition.active` уже
|
||||
`false` — `animated` там не передаётся и роли не играет, якорь берётся из
|
||||
`_cameraState()`, как и раньше; численно pinch-путь не тронут (AC подтверждён
|
||||
и юнитом `#396 AC4`, и не изменившимся `_zoomAt`).
|
||||
- `cameraStill` (`:10894`): единственное место построения признака и
|
||||
единственный вызывающий `resolveGlowFeather` — читается напрямую, дублей нет.
|
||||
- Одно число — один источник: `this._zoom` — единственная переменная,
|
||||
питающая и рендер (`${Math.round(this._zoom * 100)}%` в бейдже зума,
|
||||
`:11597`), и персист (`_saveZoom` читает `this._zoom`, `:6394`). Показанное и
|
||||
сохраняемое в этом диффе — буквально одна и та же переменная в один и тот же
|
||||
момент, дублирования нет.
|
||||
|
||||
## AC — доказательство
|
||||
|
||||
- **AC1/AC2** (пользовательская отмена сохраняет показанный зум, структурная —
|
||||
не пишет ничего): доказано юнитом на контроллер
|
||||
(`test/viewport-transition.test.mjs`, тест «#396 AC1/AC2», проверяет, что
|
||||
`cancel(false)` не зовёт `settled` и обнуляет `presented` — обязанность
|
||||
решает вызывающий) **и** браузерным смоком на реальном DOM
|
||||
(`demo/smoke_smooth_zoom.mjs`: `userCancelPersistsTheShownZoom`,
|
||||
`structuralCancelPersistsNothing`) — прогнан лично, зелёный, оба булевых
|
||||
флага `true`. Тест умеет падать: мутант `camera-cancel-loses-zoom`
|
||||
(откат `_stagePointerDown` на `_cancelCameraTransition(false)` без
|
||||
`keepPresented`) зарегистрирован в `scripts/mutation-gate.mjs` и по описанию
|
||||
автора краснит именно этот смок; логика мутации прозрачна — без
|
||||
`keepPresented=true` `presentedZoom` останется `undefined` и `_saveZoom()` не
|
||||
вызовется, `userCancelPersistsTheShownZoom` станет `false`.
|
||||
- **AC3/AC4** (якорь точен на серии из 6 нотчей, накопление zoom не тронуто):
|
||||
юнит с фейковым клоком (`test/viewport-transition.test.mjs`, тест
|
||||
«#396 AC3»/«#396 AC4») доказывает дрейф `<1e-9` для `anchorFromTarget: true`
|
||||
и контрольную ветку `anchorFromTarget: false` (baseline дефекта), которая
|
||||
честно даёт заметный увод (`>1` единица) — тест не декоративен, разница
|
||||
видна прямо в его собственной сборке. Прогнан лично (`npm test`, в составе
|
||||
1654 тестов). Браузерный смок `fastWheelKeepsTheAnchor: anchorDriftPx < 0.5`
|
||||
— зелёный, `anchorDriftPx: 0` в диагностике.
|
||||
- **AC5** (feather заморожен во время перехода): source-guard тест
|
||||
(`test/golden-matrix.test.mjs`) пиннит и предикат, и место его
|
||||
использования — регекспы требуют буквальный текст `const cameraStill = …
|
||||
&& !this._cameraTransition.active;` и его прямую передачу в
|
||||
`resolveGlowFeather`. Прочитан регекс и код рядом — совпадают. Отдельный
|
||||
перф-профиль числа пересборок (упомянутый в плане автотестов ТЗ) не создан;
|
||||
это расхождение с планом автотестов уже разобрано в r1 (принято как Low, не
|
||||
поднимается заново: `golden:verify` зелёный подтверждает, что визуал в покое
|
||||
идентичен, а source-guard теста достаточно, чтобы регресс предиката
|
||||
краснил тест при возврате старого условия).
|
||||
- **AC6** (правка §10/§13 спеки #82 точечная): прочитан diff
|
||||
`docs/specs/082-smooth-zoom.md` — ровно два блока правок (якорь п.3 плюс
|
||||
порог, различение отмены §13), остальной текст не тронут.
|
||||
- **AC7** (регресс не возвращается: reduced-motion 0 rAF, `dispose()` снимает
|
||||
rAF, состояние перехода не в конфиге): `src/viewport-transition.ts` этим
|
||||
диффом не тронут вовсе — существующие юниты на reduced-motion/`dispose`
|
||||
(часть тех же 1654 зелёных) остаются доказательством без изменений в коде,
|
||||
который они покрывают.
|
||||
|
||||
## Что проверено и корректно
|
||||
|
||||
- Полный диф задачи (`origin/dev...HEAD`) прочитан целиком, а не только по
|
||||
условному «пятну» дельты — ребейз потребовал этого по §2.10.
|
||||
- Все 7 AC закрыты автотестом либо разобраны по коду с явной пометкой, тест
|
||||
умеет падать там, где это утверждается (проверено логикой мутанта, не
|
||||
только словами автора).
|
||||
- Оба changelog (`docs/CHANGELOG.md`, `docs/CHANGELOG.ru.md`) правлены в том
|
||||
же коммите (`8f485c0b`), что и код — трейлер `User-Visible: yes` на месте.
|
||||
`git show --stat 8f485c0b` подтверждает оба файла в списке.
|
||||
Ребейз-коммит `f3620f2b` (только class D + docs) несёт `User-Visible: no`,
|
||||
что верно — поведение не меняет, только пересобранные артефакты.
|
||||
- Трейлеры и ветка/имя проверены `process-gate.mjs` офлайн — 0 предупреждений.
|
||||
- Визуал в покое не затронут: `golden:verify` 147/147, `check-docs.mjs`
|
||||
зелёный (отпечаток скриншотов документации не разошёлся).
|
||||
- Одно число — один источник: см. находки выше, дублирования зума
|
||||
показ/запись нет.
|
||||
- Не в скоупе (§ ТЗ) — pinch/pan persistence, длительности анимаций, kiosk
|
||||
double-tap, формат `LS_ZOOM`, editor zoom: диф этих путей не касается,
|
||||
подтверждено смоками `editor_gestures`, `kiosk`, `kiosk_pan_lock`,
|
||||
`pan_any_zoom` (все зелёные, прогнаны лично).
|
||||
|
||||
## Чего не проверял
|
||||
|
||||
- `python -m pytest tests_backend` — diff не трогает `custom_components/**/*.py`.
|
||||
- `node scripts/model-invariants.mjs` — diff не трогает геометрию/рёбра
|
||||
комнат/толщину/`layout`/`marker.space`/`open_spans`; путь камеры и рендера
|
||||
их не задевает.
|
||||
- Полная матрица браузерных смоков и `performance_smoke` — предрелizный гейт
|
||||
(§8); задача узкая и одномодульная, прямые совпадения покрывают
|
||||
все затронутые пути (11/11 зелёных).
|
||||
- Отдельный perf-профиль AC5 (число пересборок feather) — не создан автором;
|
||||
принято в r1 как Low без переоткрытия (см. AC5 выше).
|
||||
- `smoke_infinite_canvas.mjs` в окружении автора vs здесь: расхождение
|
||||
зафиксировано, не расследовалось глубже — не относится к #396 в любом случае.
|
||||
|
||||
## Закрытие раунда r1
|
||||
|
||||
Раунд r1 (SHA `c1b6a01b`) вышел **зелёным**: High 0 / Medium 0 — находок для
|
||||
закрытия нет. Единственное событие между r1 и r2 — не ревью-находка, а
|
||||
механика конвейера: слияние отменилось, потому что `dev` продвинулся на 11
|
||||
коммитов за время ревью (§7.2, «Слияние отменено: ветка изменилась после
|
||||
проверенного материала (#312)»), задача вернулась в `S6-in-progress`, автор
|
||||
перебазировал ветку и вернул `S7-code-review`. Бюджет циклов эта пара
|
||||
«зелёный → ребейз» не тратит (§227): «блокирующих циклов 0/4» и в r1, и в r2.
|
||||
|
||||
| Событие r1→r2 | Чем закрыто | Где видно |
|
||||
|---|---|---|
|
||||
| Слияние r1 отменено (dev ушёл вперёд) | Ребейз ветки на `dev` | комментарий «Ребейз на ушедший вперёд dev выполнен, HEAD `f3620f2b`» + `git merge-base --is-ancestor origin/dev HEAD` → true |
|
||||
| Риск «ребейз незаметно поменял продуктовый код» | Диф `origin/dev...HEAD` перечитан целиком в этом раунде; `src/houseplan-card.ts` — те же 38 строк, что и на `c1b6a01b` по описанию автора | `git diff --stat origin/dev...HEAD` в этом документе; `src/viewport-transition.ts` вне диффа |
|
||||
| Осиротевшие чанки после первой пересборки (`de-CRMfGGHY.js`, `fr-0kHbASBE.js`) | Дерево пересобрано с нуля, `bundle-tree-committed` тест это поймал | комментарий автора «Отдельно: при первой пересборке… деревья пересобраны с нуля, теперь 7 файлов»; в этом раунде `npm run build` → `git status --short` пуст, 7 файлов в обоих деревьях (сверено) |
|
||||
|
||||
## Унаследовано из r1 — не сработало: раунд разобран полностью
|
||||
|
||||
По §2.10 ребейз на ушедший вперёд `dev` — явно перечисленный случай, где
|
||||
«разбор остаётся полным», а не по дельте. Поэтому раздел «унаследовано без
|
||||
повторной проверки» пуст по построению: весь диф задачи (`origin/dev...HEAD`),
|
||||
все 7 AC и все гейты в таблице выше перепроверены в этом раунде заново, а не
|
||||
приняты на слово из `docs/reviews/CODE-REVIEW-396-r1.md`. Единственное, что
|
||||
формально не переисполнялось, — сами измерения B1/B2 из аналитики и
|
||||
исполнения r1 (14,5/16,1/10,3 px дрейфа, 0 вызовов `settled` при обрыве) не
|
||||
повторялись байт-в-байт, но их следствие — сегодняшние зелёные AC3/AC1 —
|
||||
проверено заново собственным прогоном юнитов и смоков на текущем SHA, а не
|
||||
переписано из прошлого документа.
|
||||
|
||||
## Вывод
|
||||
|
||||
Диф r1→r2 по продуктовому коду пуст; ребейз чист, слияние с `dev` не потеряло
|
||||
и не задвоило ни одного пункта changelog (заявление автора о «конфликтовали
|
||||
только оба ченджлога, слиты объединением, ни один пункт не потерян» проверено
|
||||
чтением обоих файлов — прежний пункт про #32 на месте, пункт #396 добавлен
|
||||
рядом). Полный независимый прогон гейтов и AC подтверждает вердикт r1: три
|
||||
находки аудита (B1, B2, M2) закрыты корректно, регресс не возвращается,
|
||||
визуал в покое не затронут.
|
||||
Reference in New Issue
Block a user