From 4132e036428feb4a00ffb768b9cb004184360ce3 Mon Sep 17 00:00:00 2001 From: "claude[bot]" <209825114+claude[bot]@users.noreply.github.com> Date: Sun, 30 Aug 2026 21:49:53 +0000 Subject: [PATCH] docs: review document for #396 Issue: #396 User-Visible: no --- docs/reviews/SPEC-REVIEW-396-r1.md | 252 +++++++++++++++++++++++++++++ 1 file changed, 252 insertions(+) create mode 100644 docs/reviews/SPEC-REVIEW-396-r1.md diff --git a/docs/reviews/SPEC-REVIEW-396-r1.md b/docs/reviews/SPEC-REVIEW-396-r1.md new file mode 100644 index 00000000..cfb1de48 --- /dev/null +++ b/docs/reviews/SPEC-REVIEW-396-r1.md @@ -0,0 +1,252 @@ +# SPEC-REVIEW-396-r1 + +- **Issue:** https://github.com/Matysh/houseplan-card/issues/396 +- **ТЗ под ревью:** `docs/specs/396-camera-transition-fixes.md`, коммит `4e0a30a78161a4ff59679454d1f8b5c471e10008` (HEAD ветки `issue/396-camera-transition-fixes`) +- **Этап:** spec (PROCESS.md §2.4) +- **Трек:** обычный (без метки `small`/`trivial`) +- **Заход:** r1 · блокирующих циклов ревью ТЗ израсходовано 0 из 4 (до этого вердикта) + +## Скоуп ревью + +Issue #396 переносит три находки внешнего аудита (`AUDIT-2026-08-31-v1700beta1.md`, +в репозитории не лежит — внешний документ, ссылка не проверяется) в одно ТЗ, +поскольку все три касаются одной подсистемы (камера-переход из #82): + +- **B1 (High)** — прерванный пользователем зум не сохраняется: регресс против + v1.69.0; +- **B2 (High)** — якорь под курсором вычисляется от отстающего кадра при + быстрой серии wheel-событий; +- **M2 (Medium)** — glow-feather не заморожен во время нового camera tween. + +Ревьюер не видел устных пояснений автора; материал — только issue #396, ТЗ +`docs/specs/396-camera-transition-fixes.md` и код на `origin/dev`/ветке задачи. +Продуктового кода к этому изменению ещё нет (ветка содержит один doc-коммит), +поэтому единственный вопрос ревью — проверить каждое фактическое утверждение +ТЗ о поведении `src/viewport-transition.ts` и `src/houseplan-card.ts` построчно +против реального кода, ровно так же, как это требует PROCESS.md §2.4 и +прецедент SPEC-REVIEW-197-r1. + +## Как проверялось + +1. Прочитаны целиком `docs/SCOPE.md`, `PROCESS.md` (действующая редакция, + §1, §2.4, §2.5, §2.10, §7.1, §7.2), `AGENTS.md`. +2. Прочитано тело issue #396 (единственный комментарий — сам текст issue, + других комментариев нет). +3. Прочитан `docs/specs/082-smooth-zoom.md` целиком — канонический документ + подсистемы, поскольку #396 правит его §10 и §13 (AC6). +4. Прочитан весь текст ТЗ `docs/specs/396-camera-transition-fixes.md` (206 + строк), сверены обязательные разделы §7.1 PROCESS.md — присутствуют все: + сценарий, что человек увидит до/после, проблема и контракты по пунктам, + скоуп/не-скоуп, UX, модель данных и миграция, i18n, критерии приёмки + AC1–AC7 (у каждого назван способ доказательства), план автотестов, риски, + откат, release-артефакты, блок принятых технических предположений. +5. Построчно сверены все технические утверждения ТЗ с фактическим кодом: + - `src/viewport-transition.ts` (212 строк) прочитан целиком — + `CameraTransitionController.cancel(commitTarget)` (`:189-200`), + `dispose()` (`:202`), `_settle()` (`:204-211`); + - все вызовы `_cancelCameraTransition(...)` и `_cameraTransition.cancel/dispose` + в `src/houseplan-card.ts` пересчитаны поимённо: + `grep -n "_cancelCameraTransition(false)\|_cancelCameraTransition(true)\|_cameraTransition\.\(cancel\|dispose\)" src/houseplan-card.ts`; + - каждый найденный вызов прочитан в контексте функции, которая его содержит + (`_onMotionChange`, `_startCameraTransition`, `_cancelModeTransition`, + `_adoptMode`, `_commitSpace`, `_adoptStructuralResponses` ×2, `_applyView`, + `_refitView`, `_zoomAt`, `_restoreZoom`, `_stagePointerDown`, + `disconnectedCallback`); + - оба вызова `_zoomAt(...)` (`:6517`, `:6897`, внутри pinch-путей + `_stagePointerMove`/`_touchGesture...`) прочитаны с окружением, чтобы + проверить, действительно ли за ними сразу следует `_saveZoom()`; + - `_cameraTargetAt` (`:6262-6279`) и `cameraTargetAtAnchor` (`viewport-transition.ts:94-116`) + прочитаны вместе, чтобы проверить claim B2 (мировая точка считается от + `this._cameraState()`, то есть от представленного кадра, независимо от + того, какой `targetZoom` передал вызывающий код); + - `resolveGlowFeather(...)` вызов на `:10845-10847` сверен с цитатой ТЗ + (`:10842-10847`) — совпадает дословно. +6. Гейты (`typecheck`/`test`/`build`) не прогонялись — на этапе ревью ТЗ + продуктового кода не существует (только doc-коммит), что вне скоупа этапа + (PROCESS.md §2.4/§8). + +## Находки + +### High-1 — центральное фактическое утверждение B1 («семь мест») не совпадает с реальными вызовами и не покрывает главный сценарий issue + +**Файл:** `docs/specs/396-camera-transition-fixes.md`, раздел «(1) B1 — прерванный +зум не сохраняется» и раздел «Скоуп / не-скоуп». + +ТЗ утверждает: «`_cancelCameraTransition(false)` зовут семь мест: `:1267`, +`:1401`, `:1551`, `:4188`, `:4216`, `:6089`, `:6206` — pointerdown по сцене, +смена пространства, `_refitView`, adoption, reload, `_zoomAt`, disconnect.» +(позиционное соответствие семи чисел семи описаниям в том же порядке). + +Фактическая проверка (`grep -n "_cancelCameraTransition(false)\|_cancelCameraTransition(true)\|_cameraTransition\.\(cancel\|dispose\)" src/houseplan-card.ts` +на HEAD ветки задачи) даёт **десять** мест с `commitTarget=false` плюс одно +место, где котроллер отменяется напрямую, минуя обёртку: + +| Строка | Функция | Что это на самом деле | +|---|---|---| +| `:1267` | `_startCameraTransition`, no-op ветка (`sameCameraState(current, target)`) | не «pointerdown», а ранний выход при запросе уже показанного состояния | +| `:1401` | `_cancelModeTransition` | общий helper для смены mode/space, не «смена пространства» отдельно | +| `:1551` | `_commitSpace` | это и есть настоящая «смена пространства» — но в списке ей приписана строка `:1401` | +| `:4188` | `_adoptStructuralResponses` (config adopt) | «adoption» — совпадает | +| `:4216` | `_adoptStructuralResponses` (layout adopt) | второе «adoption», в списке названо «reload» | +| `:6089` | `_applyView` | не «`_zoomAt`» — другая функция | +| `:6206` | `_refitView` | resize, не «disconnect» | +| `:6283` | `_zoomAt` (pinch, immediate) | **отсутствует в списке семи** | +| `:6360` | `_restoreZoom` | **отсутствует в списке семи**, не упомянута нигде в ТЗ | +| `:6387` | `_stagePointerDown` | **отсутствует в списке семи** — это и есть буквальный «pointerdown по сцене» из пользовательского сценария issue | +| `:2745` | `disconnectedCallback` | вызывает `this._cameraTransition.dispose()` напрямую, минуя `_cancelCameraTransition` — не через строку `:6206`, как подразумевает список | + +Т.е. ни одна из строк реального `_stagePointerDown` (`:6387`) и реального +`_zoomAt` (`:6283`) не входит в перечисленные семь чисел, хотя проза ТЗ прямо +называет «pointerdown по сцене» и «`_zoomAt`» в качестве двух из этих семи. +Disconnect вообще не проходит через `_cancelCameraTransition` — там отдельный +вызов `.dispose()` на `:2745`. + +**Проверено исполнением/чтением, что `:6387` — это настоящий воспроизводящий +сценарий из issue.** `_stagePointerDown` (`houseplan-card.ts:6386-6446`) вызывает +`this._cancelCameraTransition(false)` первой строкой и не содержит ни одного +вызова `_saveZoom()` внутри себя или в путях, которые гарантированно +выполняются после простого клика (не переросшего в pan/pinch). Ровно это — +«колесо/кнопка «+» → сразу клик по плану» из раздела «Пользовательский +сценарий» B1. Сопутствующая проверка: `_zoomAt` (`:6283`) технически тоже +вызывает `_cancelCameraTransition(false)`, но оба его реальных вызывающих +контекста (`:6517` — pinch move, `:6897` — touch multitouch move) сразу же +следом вызывают `_saveZoom()` — то есть через `_zoomAt` состояние фактически +не теряется, вопреки тому, что список подписывает его как один из семи +«теряющих» вызовов. + +**Почему это блокирует, а не опечатка в номерах строк.** Раздел «Скоуп / +не-скоуп» ТЗ перечисляет затронутые функции: «`_startCameraTransition`, +`_cameraTargetAt`, `_onWheel`, `_stepZoom`, `_cancelCameraTransition`, +glow-гейт» — и **не называет `_stagePointerDown` ни разу**. DoR (PROCESS.md +§2.5) требует «перечислены затронутые файлы и модули» именно для того, чтобы +разработчик не должен был заново открывать код в поисках места правки. Если +разработчик читает этот список как объявление границы работы (а не как +приглашение заново обыскать весь файл), правка `_stagePointerDown` не +попадёт в реализацию — а именно там лежит буквальный баг из пользовательского +сценария issue. Раздел «Риски» ТЗ («Тонкая грань «пользовательская против +структурной»») сам ссылается на «список мест перечислен в спеке #82 §13» как +на средство защиты от ошибки классификации — но источник (список из этого же +ТЗ) неточен, то есть заявленная мера риска опирается на тот же дефектный +список. + +Смягчающее обстоятельство: план автотестов (шаг 5 browser smoke, «Колесо → +pointerdown по плану до окончания перехода → перезагрузка карты → +восстановленный зум равен показанному до клика (AC1 на реальном DOM)») сам по +себе устроен так, что реализация, не тронувшая `_stagePointerDown`, эту +проверку не пройдёт — то есть дефект будет пойман на код-ревью. Но это +означает лишний цикл там, где спек-ревью для того и существует, чтобы поймать +это раньше и дешевле (тот же аргумент, что в SPEC-REVIEW-197-r1 High-1: +разработчик рискует чинить не тот код, опираясь на неверную фактическую базу +раздела). + +**Что нужно поправить.** В разделе «(1) B1» — заменить список из семи чисел +на фактическую сверку (реальных мест десять плюс `disconnectedCallback`, +использующий `.dispose()` напрямую), явно отметить, какие из них +пользовательские (как минимум `:6387`), а какие структурные, и без +двусмысленности показать, что `_zoomAt` (`:6283`/`:6517`/`:6897`) уже не +теряет состояние благодаря существующему `_saveZoom()` сразу после — то есть +не требует правки по контракту B1. В разделе «Скоуп / не-скоуп» — добавить +`_stagePointerDown` в список затронутых функций (и, при необходимости, +`_restoreZoom`, если ревью реализации решит, что он тоже нуждается в явной +классификации). + +## Что проверено и корректно + +- **B1 — контракт и AC1/AC2 сформулированы верно**, несмотря на дефектный + список строк: `CameraTransitionController.cancel(false)` + (`viewport-transition.ts:189-200`) действительно не вызывает `_hooks.settled` + и, следовательно, никогда не проходит через `_settleCameraTransition` → + `_saveZoom()` (`houseplan-card.ts:1232-1244`) — регресс реален и подтверждён + чтением обоих файлов независимо от того, какие конкретно строки его + проявляют. Различение «пользовательская отмена / структурная отмена» + (таблица в разделе «(1)») продуктово согласовано с уже принятым §11 + `docs/specs/082-smooth-zoom.md` («Pointerdown, который начинает pan, pinch, + draw, drag или selection, сначала фиксирует представленный кадр и отменяет + tween без скачка» — то есть #82 уже требовал этого поведения, #396 первым + делает его явным контрактом с AC). +- **B2 подтверждён чтением и математически обоснован.** `_cameraTargetAt` + (`houseplan-card.ts:6262-6279`) всегда строит якорь от + `this._cameraState()` (`:6273`) — то есть от представленного, а не целевого + кадра — независимо от того, что `_onWheel` (`:6294-6308`) уже правильно + берёт `baseZoom` из `this._cameraTransition.target?.zoom ?? this._zoom` + (`:6301`). Это ровно расхождение «масштаб — из target, точка — из + presented», которое ТЗ описывает как корень утечки якоря. Контрактная + формулировка («масштаб и мировая точка берутся из одного и того же + состояния») корректно закрывает разрыв, а замечание про `_zoomAt` + («там tween заведомо отменён, представленное и есть текущее») подтверждено: + `_zoomAt` (`:6282-6292`) вызывает `_cancelCameraTransition(false)` первой + строкой, поэтому к моменту вычисления якоря активного перехода уже нет и + presented/target совпадают по определению. +- **M2 подтверждён дословным совпадением цитаты.** `resolveGlowFeather(...)` + вызов на `houseplan-card.ts:10845-10847` с третьим аргументом + `!this._pinchStart && !this._panStart` совпадает с цитатой ТЗ буквально — + признак «камера движется» не учитывает новый camera tween, только pinch/pan + gesture state. +- **Правка `docs/specs/082-smooth-zoom.md` §10/§13 обоснована.** Прочитан + документ целиком (386 строк): §10 п.3 действительно говорит «в + представленном viewport», а порог ниже — «не более 0.5 CSS px», то есть + спека #82 сама себе противоречит после реализации B2-бага (п.3 выполнен, + порог нарушен) — формулировка AC6 «привести в соответствие» корректна и не + требует продуктового решения владельца (чисто согласование текста с уже + принятым поведенческим контрактом п.2 §10). +- **Структура ТЗ и обязательные разделы (PROCESS.md §7.1)** — все на месте, + включая i18n («Новых строк нет» — верно, ни один из трёх контрактов не + добавляет UI-текст), модель данных/миграция («формат `LS_ZOOM` не меняется» + — верно, персист остаётся тем же ключом/структурой), touch (пункт «Немедленный + `_zoomAt` (pinch) продолжает работать от представленного состояния» — + корректно определяет границу с touch-контрактом), откат (точечный, три + независимых правки, без миграции данных — согласуется с характером задачи). +- **Трек.** Обычный трек (не `small`) обоснован по факту: три независимых + находки на одной подсистеме, правка канонического документа #82 — критерий + §5 PROCESS.md («нет нового UX-контракта» технически спорен, но задача явно + не «одна поверхность» и трогает канон #82, что само по себе исключение из + лёгкого трека). +- **Открытых продуктовых вопросов к владельцу не осталось и не должно + остаться** — единственный найденный пробел (High-1) технический (какие + именно строки/функции меняются), а не продуктовый: ни «что видит + пользователь», ни «объём видимых изменений в issue» здесь не решаются. + Технический спор автора и ревьюера решается вердиктом, не владельцем + (PROCESS.md §7.1). +- **Мутанты названы конкретно** (`camera-anchor-from-presented`, + `camera-cancel-loses-zoom`, `glow-feather-thaws-during-camera`) и каждый + указывает, какой юнит должен покраснеть — соответствует правилу «тест умеет + падать» (PROCESS.md §2.7, применимо и к дизайну теста на этапе ТЗ). + +## Чего не проверял + +- Код продукта не существует на этой стадии (только ТЗ) — гейты + `typecheck`/`test`/`build`/`check-docs`/`invariants`/смоки/`golden:verify` + неприменимы и не запускались; это нормально для этапа ТЗ, а не пропуск. +- Не проверял содержимое внешнего аудита `AUDIT-2026-08-31-v1700beta1.md` — + файл не лежит в репозитории (только ссылка на него в теле issue), поэтому + его точные измерения (14.5/16.1/10.3 px и т.п.) не перепроверялись + инструментально; для ревью достаточно того, что оба бага (B1, B2) + независимо подтверждены чтением реального кода на `origin/dev`. +- Не прогонял `test-build/viewport-transition.js` с фейковым клоком, на + который ссылается ТЗ («переход 1.0 → 1.15 за 220 мс, обрыв на 120 мс → + `settled` вызван 0 раз, `presented.zoom = 1.1413`») — подтвердил тот же + вывод логическим чтением `CameraTransitionController.cancel()` + (`viewport-transition.ts:189-200`: `commitTarget=false` выходит до вызова + `this._hooks.settled`), числовое значение `1.1413` не пересчитывал. +- Не искал дополнительные вызовы `_cancelCameraTransition`/`_cameraTransition.cancel` + за пределами `src/houseplan-card.ts` (например, в `src/houseplan-editor-runtime.ts`) + — раздел «Скоуп» ТЗ ограничивает камеру ядром `houseplan-card.ts`, и + `grep -n "_cameraTransition\b" src/houseplan-editor-runtime.ts` не выполнялся; + при исправлении High-1 автору стоит перепроверить и этот файл на всякий + случай, хотя #82 §19 явно оставляет бизнес-логику zoom в core. + +## Вердикт + +Красный. High: 1 (B1 — список мест «`_cancelCameraTransition(false)`» +фактически неверен и не включает `_stagePointerDown`/`_zoomAt`, а раздел +«Скоуп» не называет `_stagePointerDown` как затрагиваемую функцию — риск, что +буквальный пользовательский сценарий issue останется незачинённым). Medium: 0. +Контракты B1/B2/M2 и AC1–AC7 по существу верны и не требуют продуктового +решения владельца; правка текстовая — исправить раздел «(1) B1» и «Скоуп / +не-скоуп» по фактической сверке выше, без изменения AC. После правки — +повторный заход по дельте (PROCESS.md §2.10): проверить перечень мест и то, +что `_stagePointerDown` явно назван местом правки, остальные разделы +(B2/M2, AC3-AC7, риски §10/§13) наследуются без повторной проверки. + +**Вердикт: красный · заход r1 · блокирующих циклов 1/4 · High: 1 · Medium: 0 +→ в задаче · Документ: docs/reviews/SPEC-REVIEW-396-r1.md**