diff --git a/docs/reviews/SPEC-REVIEW-396-r2.md b/docs/reviews/SPEC-REVIEW-396-r2.md new file mode 100644 index 00000000..c145a7c3 --- /dev/null +++ b/docs/reviews/SPEC-REVIEW-396-r2.md @@ -0,0 +1,197 @@ +# SPEC-REVIEW-396-r2 + +- **Issue:** https://github.com/Matysh/houseplan-card/issues/396 +- **ТЗ под ревью:** `docs/specs/396-camera-transition-fixes.md`, коммит `d5fd5926faab12ec0883458b1d4a932ea9344bb3` (HEAD, `docs: #396 spec revision 2 per SPEC-REVIEW-396-r1`) +- **Этап:** spec (PROCESS.md §2.4) +- **Трек:** обычный (без метки `small`/`trivial`) +- **Заход:** r2 · блокирующих циклов ревью ТЗ израсходовано 1 из 4 (до этого вердикта) + +## Скоуп ревью + +Раунд r1 вернул красный вердикт с одной High-находкой: раздел «(1) B1» ТЗ +утверждал, что переход отменяют семь конкретных мест +(`:1267,:1401,:1551,:4188,:4216,:6089,:6206`), а фактическая сверка показала +десять мест плюс отдельный `.dispose()` на disconnect — при этом ни настоящий +`_stagePointerDown` (`:6387`, буквальный сценарий issue), ни настоящий +`_zoomAt` (`:6283`) в семёрке не фигурировали, а раздел «Скоуп» не называл +`_stagePointerDown` затрагиваемой функцией. Владелец ответил ревизией 2: +заменил список семи чисел таблицей из одиннадцати мест с классификацией +«пользовательская/структурная», добавил `_stagePointerDown` в «Скоуп», +переформулировал AC1/AC2. + +Это ревью — по дельте (PROCESS.md §2.9): единственный правленый раздел — +«(1) B1» плюс «Скоуп/не-скоуп» плюс AC1/AC2 (см. диф ниже). B2, M2, AC3–AC7, +UX, модель данных, i18n, риски, откат, release-артефакты дельту не задевают и +наследуются из r1 без повторной проверки. Причина полного нового прочтения +таблицы вместо точечной сверки одной строки: сама таблица — это ровно тот +артефакт, для которого r1 объявил высокий риск фактической ошибки, и +инструкция раунда прямо требует не доверять заявлению автора на слово, а +сверять «строку кода или текста». + +``` +git diff 4e0a30a78161a4ff59679454d1f8b5c471e10008..HEAD -- docs/specs/396-camera-transition-fixes.md +``` +(коммит `4e0a30a7` — HEAD ветки на момент вердикта r1). + +Продуктового кода в ветке по-прежнему нет: `git diff --stat 4e0a30a7..HEAD` +показывает только `docs/reviews/SPEC-REVIEW-396-r1.md` (публикация прошлого +вердикта) и `docs/specs/396-camera-transition-fixes.md`. `src/**` не тронут. + +## Как проверялось + +1. Прочитан весь диф правленых разделов ТЗ (раздел «(1) B1», «Скоуп/не-скоуп», + AC1, AC2, строка «Ревизия»). +2. Каждая строка новой таблицы одиннадцати мест сверена с фактическим кодом на + HEAD той же командой, что и в r1: + `grep -n "_cancelCameraTransition(false)\|_cancelCameraTransition(true)\|_cameraTransition\.\(cancel\|dispose\)" src/houseplan-card.ts` + — 14 строк (13 точек вызова + строка-определение самого `_cancelCameraTransition` + на `:1242`, которая не точка вызова и в таблицу не входит). +3. Каждая из 13 точек вызова прочитана в контексте содержащей функции: + `:1159` (`_onMotionChange`), `:1267` (`_startCameraTransition`, no-op + ветка), `:1401` (`_cancelModeTransition`), `:1551` (`_commitSpace`), + `:2363` (`_pageVisibility`), `:2745` (`disconnectedCallback` → + `.dispose()` напрямую), `:4188` и `:4216` (обе — внутри + `_adoptStructuralResponses`, `:4167-4224`), `:6089` (`_applyView`), + `:6206` (`_refitView`), `:6283` (`_zoomAt`), `:6360` (`_restoreZoom`), + `:6387` (`_stagePointerDown`). +4. Отдельно перепроверены оба вызывающих контекста `_zoomAt` (`:6517` — + pinch-move, `:6897` — touch multitouch move): оба вызывают `_saveZoom()` + сразу после `_zoomAt(...)` — заявление таблицы «правки не требует» верно. +5. Сверено, что `_stagePointerDown` (`:6387`) действительно первой строкой + вызывает `_cancelCameraTransition(false)` и не содержит гарантированного + `_saveZoom()` на пути простого клика — это подтверждает и контракт AC1, и + то, что High-1 из r1 закрыт по существу (строка теперь в «Скоуп» и в AC1). +6. Гейты (`typecheck`/`test`/`build`) не прогонялись — продуктового кода нет + (только doc-коммиты), это вне скоупа этапа spec (PROCESS.md §2.4/§8), как + и в r1. + +## Находки + +### Medium-1 — таблица приписывает строки `:4188`/`:4216` несуществующей функции `_load` + +**Файл:** `docs/specs/396-camera-transition-fixes.md`, раздел «(1) B1», +строка таблицы `| \`:4188\`, \`:4216\` \`_load\` | adoption конфига и layout | структурная |`. + +Фактическая проверка: в `src/houseplan-card.ts` идентификатора `_load` не +существует (`grep -n "\b_load\b" src/houseplan-card.ts` — пусто). Обе строки +`:4188` и `:4216` лежат внутри одной и той же функции +`_adoptStructuralResponses` (`:4167-4224` — сигнатура на `:4167`, config-adopt +ветка на `:4188`, layout-adopt ветка на `:4216`), которую вызывают +`_loadFromServer` (`:4247`, вызов на `:4284`) и второй caller на `:4460` +(config-only reload). Ни один из этих реальных идентификаторов не совпадает +с `_load`: ближайшие по написанию — `_loadFromServer`, `_loadOk`, `_loading`, +`_loadTries`, `_loadRetryTimer` — все разные функции/поля. + +Это ровно тот класс дефекта, который r1 квалифицировал как High (таблица +называет несуществующее/неверное имя функции, хотя визуально оформлена как +проверенный факт — «сверено `grep`'ом… каждое прочитано»), и он **новый**: +в тексте r1 (и в собственном ответе ревьюера, и в разборе владельца) эти же +строки корректно названы `_adoptStructuralResponses` — ревизия 2, поправляя +High-1, сама внесла эту неточность взамен верного имени. + +**Почему Medium, а не High.** У этой строки нет требуемой правки кода — класс +«структурная», и правка по контракту B1 нужна только в `_stagePointerDown` +(`:6387`), которая в «Скоуп» названа явно и корректно. Ложное имя `_load` не +рискует тем, что разработчик пропустит обязательную правку (в отличие от +High-1 r1, где отсутствие `_stagePointerDown` в списке грозило оставить +буквальный сценарий issue незачиненным) — оно рискует ввести в заблуждение +читателя, сверяющего полноту классификации (найдёт `_loadFromServer`, не +найдёт `_load`, потратит время на поиск несуществующего имени), и подрывает +доверие к самому заявлению «каждое прочитано» в этой же таблице. + +**Что нужно поправить.** Заменить `_load` на `_adoptStructuralResponses` в +строке таблицы для `:4188`/`:4216`. + +### Low-1 (waived) — вторая функция в строке `:1159`/`:2363` не названа + +Строка таблицы `| \`:1159\` \`_onMotionChange\`, \`:2363\` | ... |` называет +функцию только для первого числа; `:2363` фактически лежит в `_pageVisibility` +(`:2357-2363`, поле-стрелка класса), но таблица этого не пишет. В отличие от +Medium-1 здесь нет ложного утверждения — просто пропуск имени, классификация +(«`cancel(true)`, цель коммитится, уже сохраняется через `settled`») верна для +обеих строк. Не блокирует; можно поправить попутно при правке Medium-1, а +можно оставить — снимаю без требования правки. + +## Закрытие раунда r1 + +| Находка r1 | Чем закрыта | Где это видно | +|---|---|---| +| High-1: список из семи чисел не совпадает с реальными вызовами; `_stagePointerDown` (`:6387`) и `_zoomAt` (`:6283`) отсутствуют в списке; «Скоуп» не называет `_stagePointerDown` | Список заменён таблицей из одиннадцати мест (все 13 точек вызова, включая `.dispose()` на `:2745`), с явной классификацией «пользовательская/структурная»; `_stagePointerDown` добавлен в «Скоуп» жирным и назван «буквальный сценарий issue» с явной пометкой «сохранять»; `_zoomAt` явно помечен «правки не требует» с обоснованием | `docs/specs/396-camera-transition-fixes.md`, раздел «(1) B1» (новая таблица) и раздел «Скоуп / не-скоуп» (`**\`_stagePointerDown\`**`); проверено построчной сверкой с кодом в этом раунде (см. «Как проверялось» п.2–5) — таблица теперь фактически верна за единственным исключением Medium-1 (имя `_load` вместо `_adoptStructuralResponses` для `:4188`/`:4216`) | +| Риск «поймает только код-ревью, лишний цикл» | Снят тем же исправлением: `_stagePointerDown` теперь явно в скоупе и в AC1, реализация не сможет случайно пропустить это место, читая раздел «Скоуп» как границу работы | AC1: «После пользовательской отмены перехода (`_stagePointerDown` — касание плана...) сохранённый зум пространства равен показанному» | + +## Унаследовано из r1 + +Не проверялось повторно в этом раунде — дельта их не касается, приняты по +документу `docs/reviews/SPEC-REVIEW-396-r1.md` на коммите `4e0a30a78161a4ff59679454d1f8b5c471e10008`: + +- **B2 (якорь от отстающего кадра)** — контракт, математическое обоснование + (`_cameraTargetAt` строит якорь от `_cameraState()`, presented, независимо + от `target`), правка §10 `docs/specs/082-smooth-zoom.md` (AC6) — раздел + «(2) B2» ТЗ не входит в диф r1→r2, не перечитывался. +- **M2 (feather не заморожен)** — цитата `resolveGlowFeather(..., + !this._pinchStart && !this._panStart)` (`houseplan-card.ts:10845-10847`) + сверена дословно в r1; в этом раунде дополнительно подтверждено, что строка + не сдвинулась (см. «Как проверялось», п. проверки M2 в контексте + неизменного дифа) — раздел «(3) M2» ТЗ тоже вне дифа r1→r2. +- **AC3–AC7** — формулировки не менялись (вне дифа), контракты и способы + доказательства признаны верными в r1. +- **Структура ТЗ / обязательные разделы PROCESS.md §7.1** — все на месте, + проверено в r1 (сценарий, до/после, скоуп, UX, модель данных, i18n, план + автотестов, риски, откат, release-артефакты). +- **Правка §10/§13 `docs/specs/082-smooth-zoom.md` (AC6)** — обоснованность + и то, что она не требует продуктового решения владельца, подтверждены в r1. +- **Трек (обычный, не `small`)** — обоснован в r1 и не оспаривается. +- **Открытых продуктовых вопросов владельцу нет** — подтверждено в r1; + Medium-1 этого раунда — тоже чисто технический вопрос (имя функции в + таблице), не продуктовый, решается вердиктом, а не владельцем. +- **Мутанты названы конкретно** и подтверждают правило «тест умеет падать» — + раздел «Мутанты» вне дифа r1→r2. + +## Что проверено и корректно (в этом раунде, по дельте) + +- Новая таблица из одиннадцати мест фактически верна для 12 из 13 точек + вызова (все, кроме `:4188`/`:4216`) — построчно сверено с кодом на HEAD. +- `_stagePointerDown` корректно классифицирован как единственное + пользовательское место, требующее правки, и корректно добавлен в «Скоуп». +- `_zoomAt` корректно исключён из мест, требующих правки: оба его реальных + вызывающих контекста (`:6517`, `:6897`) действительно вызывают `_saveZoom()` + сразу после — проверено чтением обоих мест. +- AC1 и AC2 переформулированы в соответствии с новой таблицей, без изменения + сути контракта B1/B2/M2 из r1; AC2 корректно охватывает `_restoreZoom` как + структурное место, которое ничего не пишет в `localStorage` (сам + `_restoreZoom` и вызываемый им `_applyView` не содержат `_saveZoom()`). +- Раздел «Ревизия» корректно ссылается на SPEC-REVIEW-396-r1 и называет + High-1 явно — соответствует ожиданию «показать, чем именно закрыта находка + предыдущего раунда» текстом, а не только заявлением. + +## Чего не проверял + +- Разделы «(2) B2», «(3) M2», UX, модель данных/миграция, i18n, план + автотестов, риски, откат, release-артефакты — не входят в диф r1→r2, + унаследованы из r1 (см. раздел выше), повторно не перечитывались построчно + против кода в этом раунде. +- Гейты `typecheck`/`test`/`build`/`check-docs`/`invariants`/смоки/ + `golden:verify` — неприменимы: продуктового кода в ветке нет (только + doc-коммиты), как и в r1. +- Содержимое внешнего аудита `AUDIT-2026-08-31-v1700beta1.md` — по-прежнему + не в репозитории, не перепроверялось (не входит в дельту этого раунда). +- `docs/specs/082-smooth-zoom.md` целиком — не перечитывался повторно в этом + раунде, так как правки §10/§13 вне дифа r1→r2; принято на веру из r1. + +## Вердикт + +Жёлтый. High: 0. Medium: 1 (Medium-1 — строка таблицы раздела «(1) B1» +приписывает `:4188`/`:4216` несуществующей функции `_load` вместо +фактической `_adoptStructuralResponses`; в скоупе этого же раунда, чинится в +нём же, без отдельного issue). Low: 1, waived (пропуск имени функции для +`:2363` — не блокирует, можно поправить попутно). + +High-1 из r1 закрыт по существу: `_stagePointerDown` (буквальный сценарий +issue) теперь явно в «Скоуп» и в AC1, `_zoomAt` корректно исключён из мест, +требующих правки, таблица одиннадцати мест фактически верна за единственным +исключением Medium-1. После правки Medium-1 (заменить `_load` на +`_adoptStructuralResponses` в одной строке таблицы) — заход по дельте +(PROCESS.md §2.9): проверить только эту строку, всё остальное наследуется без +повторной проверки. + +**Вердикт: жёлтый · заход r2 · блокирующих циклов 2/4 · High: 0 · Medium: 1 → в задаче · Документ: docs/reviews/SPEC-REVIEW-396-r2.md**