diff --git a/docs/reviews/CODE-REVIEW-714-r1.md b/docs/reviews/CODE-REVIEW-714-r1.md new file mode 100644 index 00000000..150344ee --- /dev/null +++ b/docs/reviews/CODE-REVIEW-714-r1.md @@ -0,0 +1,206 @@ +# CODE-REVIEW-714-r1 + +**Issue:** #714 «Удалить код поиска места значков #651 — после #713 он не вызывается» +**Трек:** show · **Заход:** r1 · блокирующих циклов израсходовано 0/2 +**Материал:** `git log --oneline origin/dev..HEAD` = `8f09cdfd` (единственный коммит); +`git diff origin/dev...HEAD` — 8 файлов, +112/-2561 строк. +Рабочая копия проверялась ровно на `8f09cdfd1a6c8af0581e611af047b72b93a5bbbf` +(подтверждено `git rev-parse HEAD` до и после прогонов). Ветка к `dev` не +приводилась (трек show, #696) — это ожидаемо, не находка. + +## Скоуп + +Чисто техдолг: удаление кода раскладки #651 (`resolveIsoOverlayRigidGroups`, +`resolveIsoOverlayCollisions`, поиск сдвига в `resolveIsoOverlayPlacement`, +поля `nudge*`/`nearWall*`/`cleared`/`capped`/`status`/`reason`, константы +`ISO_OVERLAY_MAX_NUDGE_CSS_PX`/`ISO_OVERLAY_SAFETY_GAP_CSS_PX`, быстрый путь +при зуме в `iso-scene-render.ts`), которого живая сцена не вызывает с #713. +User-Visible: no, что соответствует «Что видит человек: Ничего» в теле issue. +Работа обслуживает не прямую строку `docs/SCOPE.md` Core user jobs, а +поддержание J1–J7 читаемым кодом (сам issue и оценка владельца прямо называют +это техдолгом ценностью для разработки, не для пользователя — легитимно по +`docs/process/REVIEWER.md`, т.к. Rule #1 разрешает изменения класса A при +принятой issue, а не только изменения, закрывающие Core user job впрямую). + +## Как проверялось + +Прочитан весь дифф (`git diff origin/dev...HEAD`) файл за файлом, не по +диагонали — `src/iso-overlays.ts` (2010→249 строк) прочитан целиком в +итоговом виде, не только как дифф. + +### Гейты — что прогнано и результат + +| Гейт | Статус | Результат | +|---|---|---| +| Validate на `8f09cdfd` (tsc/test/build/bundle-policy) | подтверждён ссылкой | success, https://github.com/Matysh/houseplan-card/actions/runs/36771186918 | +| `npx tsc --noEmit` (перепроверка) | прогнан | чисто, без ошибок | +| `node --test test/iso-overlays.test.mjs test/iso-scene-render.test.mjs test/isometric-contract.test.mjs test/monolith-text-anchors.test.mjs` | прогнан | 53/53 green | +| Точечная мутация в `resolveIsoOverlayPlacement` (`visualOffset → visualOffset*2`) | прогнана и откачена | 6 тестов покраснели — набор умеет падать, не только зеленеть | +| Якоря 8 оставшихся мутантов `iso-overlays.ts`/`iso-scene-render.ts` в реестре | сверены скриптом | все 8 находят свой `find`-паттерн в текущем коде | +| Якоря 7 удалённых мутантов | сверены | ни один `find`-паттерн больше не существует в `src/**` — удаление мутантов согласовано с удалением кода, не потеряло охват действующего кода | +| `npm run golden:capture --scenario=` для всех 21 2.5D-сцены (`isometric-*`, `stage3-*`, `stage6-*`, `wall-union-isolation-view-*`, `stairs-isometric-dark`) | прогнано по одной (`golden:verify` не даёт `--scenario`, полный набор — предрелизный гейт) | все 21 `passed` против принятых эталонов, включая 4 сцены с `requireOneRise` | +| `node scripts/smoke-select.mjs --base origin/dev --head HEAD` | прогнан | см. ниже | +| `demo/smoke_isometric_live_touch.mjs` (прямое совпадение по `_openingsR`) | прогнан | все 44 поля `true`, `OK` | +| `demo/smoke_pan_any_zoom.mjs` (прямое совпадение по `_baseVb`/`_stageEl`/`fitView` — именно то, что диф вынул из вызова сцены оверлеев) | прогнан | все поля `true`, `OK` | +| `demo/smoke_iso_flat_parity.mjs` (слабая связь по `_baseVb`, но название прямо про изометрию) | прогнан | все поля `true`, `OK`, включая явный `wideAC3OneStraightUpShift`/`tallAC3OneStraightUpShift` — каждое устройство получает один и тот же вертикальный сдвиг | +| `npm run mutation-gate -- --check` (полный реестр) | не прогнан | по правилу трека show мутанты в разработке не гоняются ни автором, ни ревьюером (#709); анкеры сверены вручную (строка выше) | +| `npm run golden:verify` (полный набор) | не прогнан | полный набор — предрелизный гейт (§8), не гейт ревью; без метки `ci:golden`; диагностический прогон по всем 21 сценам сделан точечно через `capture` | +| `python -m pytest tests_backend` | не прогнан | дифф не касается `custom_components/**/*.py` | +| `npm run invariants` | не прогнан | дифф не меняет геометрию модели, только внутреннюю раскладку оверлеев поверх неизменной геометрии | +| performance-профили | не прогнан | не названы в AC; `demo/benchmark_large_house.mjs` и `demo/benchmark_safe_resize.mjs` только читают `data-hp-iso-nudged` как константу — не запускались | + +Вывод `smoke-select.mjs`: 22 «прямое совпадение», 31 «слабая связь», 1 +«зарегистрированная связь» (`demo/smoke_isometric_contract.mjs` ← +`unprojectFloorPoint`, с пометкой «#713: … не названы смоком»), +плюс несколько `pointInRing`-совпадений, не относящихся к удалённому коду +(`pointInRing` используется и осталась в файле). Из 22 прямых прогнаны три +самых релевантных диффу (иземетрия live-touch, panorama/zoom+fitView/_baseVb, +iso-flat-parity); остальные 19 — по несвязанным символам (`_openingsR`, +`pointInRing`, `Axis`) в файлах, к которым этот дифф не прикасается +содержательно (класс: touch/drag/wall-thickness/opening-binding смоки, где +`_baseVb`/`_openingsR` совпадают по имени символа, а не по изменённой +логике) — решение: не гонять, слабая связь не обязывает. + +**Chromium был установлен вручную** (`npx playwright install --with-deps +chromium`) — отсутствовал в среде; тело issue явно не называет смоук, но +AC1 прямо требует «golden проходит на принятых эталонах», что само по себе +требование гейта задачи (не зависит от узкого правила про смоук-триггер). +`127.0.0.1 demo.local` был дописан в `/etc/hosts` (требуется `demo/serve.mjs`, +прецедент — `legacy/reviews/v1.69.0/CODE-REVIEW-39-r1.md`). После проверки +рабочая копия репозитория возвращена в чистое состояние +(`npm run bundle:clean`, `git checkout -- demo/srv/assets/icons.js`; +`git status --short` пуст, `HEAD` = `8f09cdfd`). + +## AC — разбор + +**AC1 (unit + golden без правок).** +Проверено чтением и исполнением. Список из тела issue — `resolveIsoOverlayRigidGroups`, +`resolveIsoOverlayCollisions`, поиск сдвига, поля `nudge*`/`nearWall*`/`cleared`/ +`capped`, обе константы, быстрый путь при зуме в `iso-scene-render.ts` — отсутствует +в итоговых файлах (`grep` по всем именам из списка дал 0 совпадений в `src/**`, +кроме случайного текстового совпадения `safePointer`/`safePoint` в +`demo/benchmark_safe_resize.mjs`, не относящегося к удалённому коду). +`resolveIsoOverlayPlacement` теперь просто проецирует `floorAnchor` на +`visualOffset` без поиска — независимо подтверждено: до #713 живая сцена уже +вызывала резолвер с `wallSilhouettes: []`, из-за чего `nearWallBefore` всегда +было `false`, а поисковая ветка — фактически мёртвой в проде ещё до этого +коммита; #714 лишь убрал мёртвый код, а не поменял геометрическое поведение. +Golden: все 21 2.5D-сцена (включая 4 с `requireOneRise`) проходят против +принятых эталонов без новых кадров — перепроверено лично, не только со слов +автора. Смоки `smoke_isometric_live_touch`, `smoke_pan_any_zoom`, +`smoke_iso_flat_parity` зелёные. **AC1 выполнен, доказан исполнением.** + +**AC2 (реестр мутантов).** +7 мутантов удалены из `scripts/mutation-registry.mjs` +(`iso-aabb-rejects-touching-wall`, `iso-rigid-groups-use-live-zoom-scale`, +`iso-rigid-groups-split-close-row`, `iso-rigid-groups-cross-room-boundary`, +`iso-rigid-fallback-drops-room-wall-overlap-priority`, +`iso-scene-live-placement-search-returns`, +`stage4-w11-nudged-overlay-restores-tether`) — подсчёт `id: '` в файле +подтверждает дельту 1094→1087. Для каждого проверено, что его `find`-паттерн +физически отсутствует в `src/iso-overlays.ts`/`src/iso-scene-render.ts` (скрипт +выше) — значит эти мутанты не просто «показались лишними», они действительно +целились в снесённый код, а не в код, который остался без защиты. 8 оставшихся +мутантов в этих двух файлах нашли свои якоря. Точечная мутация, внесённая мной +в упрощённую функцию (`visualOffset*2`), убила 6 тестов — подтверждает, что +оставшееся покрытие не декоративное. `mutation-gate --check` не перезапускался +мной: по правилу трека show мутанты в разработке не гоняет ни автор, ни +ревьюер (#709, `docs/process/REVIEWER.md` «Трек show»); отсутствие прогона — +не находка. **AC2 выполнен, доказан по коду + точечным исполнением.** + +**AC3 (документация).** +`docs/ISOMETRIC.md` правки прочитаны целиком: фраза «invisible collision +footprints» → «invisible overlay footprints», абзац про #651-резолверы заменён +на явную историческую справку «History: #651 used to search a place for every +marker … #713 stopped calling that search, and #714 removed it». Текущее +поведение описано корректно («a placement depends only on the anchor, its +owner room, the footprint and the rise»). Раздел про 1.12×-масштаб маркеров +избавлен от слова «collisions». Ни одного места, где раскладка #651 всё ещё +описывалась бы как текущая, не найдено. **AC3 выполнен, проверено чтением.** + +## Находки + +Находок нет — ни High, ни Medium, ни Low. + +Отдельно зафиксирую и снимаю два места, которые проверил специально, т.к. они +похожи на потенциальные находки, но ими не являются: + +1. **`resolveCollisions`/`collisionMode`/два слота кэша `fit`/`live`** в + `iso-scene-render.ts` — это название и разделение кэша остались от эпохи + поиска места, хотя результат `fit` и `live` теперь всегда совпадает (автор + сам называет это в «Риски»: «лежат в разных слотах кэша; слить их — + отдельная задача»). Не находка: вне списка AC1 (issue перечисляет + конкретные функции/поля/константы дословно, этого параметра там нет), не + меняет наблюдаемое поведение, задокументировано автором как осознанный + остаток, слияние — это уже не «удаление», а рефакторинг с более широким + риском, разумно оставить отдельной задаче. +2. **`test('Stage 4 reuses pure overlay placements …')`** передаёт + `view: {...}` и `stageSize: {...}` в `buildIsoOverlayRenderScene`, хотя оба + поля убраны из `IsoOverlaySceneInput` — тест по-прежнему проходит, но + лишние поля больше ничего не тестируют (сигнатура их не читает ни до, ни + после). Не находка уровня Medium/Low: тест не стал ложным (не делает вид, + что проверяет то, чего нет, — сами имена тестов описывают corrent + поведение «resize/zoom is not a layout event», что верно и без этих полей), + просто содержит несколько строк мёртвого фикстурного мусора. Не в скоупе + issue (issue просит чистку `src/**`, не `test/**` сверх удалённых + #585/#651 кейсов), эффекта на пользователя или регрессию не несёт. + +## Что проверено и корректно + +- Полное соответствие списка удалений из тела issue фактическому диффу. +- Отсутствие мёртвых ссылок на удалённые символы во всём репозитории (кроме + случайного текстового совпадения имени, не связанного с удалённым кодом). +- `src/houseplan-card.ts`: вызовы `_isoOverlayScene`/`resolveIsoOverlayFitEnvelope` + обновлены синхронно с новой сигнатурой (`view`/`stageSize` убраны из + раскладки оверлеев, но `stageSize` для `resolveIsoOverlayFitEnvelope` — + другого назначения (fit-огибающая K8) — оставлен верно, не удалён по + ошибке). `data-hp-iso-nudged` захардкожен в `'false'`, что соответствует + реальному поведению (нигде больше `nudged` не вычисляется) и тому, что + читают golden/смоук/бенчмарк. +- Трейлеры коммита: `Issue: #714`, `User-Visible: no` — корректны, changelog + не тронут, как и требуется при `no`. +- Мутанты и тесты синхронно вычищены без потери фактического покрытия + действующего кода (проверено якорями и точечной мутацией). +- Ни один тест, читающий монолит как текст, не добавлен к списку + `test/monolith-text-anchors.test.mjs` — правка `isometric-contract.test.mjs` + лишь обновляет regex существующего, уже учтённого файла. + +## Чего не проверял + +- Полный `npm run golden:verify` (все сцены матрицы, не только 21 + изометрическую) и полный `npm run mutation-gate -- --check` — предрелизные + гейты, без метки `ci:golden`, объём ревью show их не требует; точечно все + 21 iso-сцены и все затронутые мутанты всё равно проверены отдельно. +- 19 из 22 «прямых совпадений» `smoke-select` (touch/drag/wall-thickness/ + opening-binding смоки) и все 31 «слабых» — связаны с диффом только общим + именем символа (`_baseVb`, `_openingsR`, `pointInRing`), не с изменённой + логикой; решение — не гонять, слабая связь не обязывает, широкий символ (39 + изменённых строк, порог «широкого» — 56) не сработал. +- `python -m pytest tests_backend`, `npm run invariants`, + performance-профили — гейты не применимы к этому диффу (нет правок Python, + геометрии модели или названных в AC перформанс-сценариев). +- Ручная проверка в браузере (не headless-смоук/golden) — не выполнялась; + доказательства строятся на golden-скриншотах и смоук-ассертах, а не на + визуальном осмотре вживую. + +## Вердикт + +Зелёный. Все три AC выполнены и доказаны исполнением (golden — лично +перепрогнан на всех 21 сцене, а не принят со слов автора; unit-тесты — +перепрогнаны и дополнительно проверены точечной мутацией на способность +падать; реестр мутантов — сверен якорями для всех затронутых мутантов, и +удалённых, и оставшихся). Находок нет. + +--- + + + +## Материал раунда + +- Ветка: `issue/714-drop-651-search`, коммит `8f09cdfd1a6c` — ребейз его осиротит, и это нормально: ниже якоря, которые ребейз не меняет. +- Дерево материала: `569e21b91ea4863052ae294726e4bf0111b1fb61` + ``` + git log --all --format='%H %T' | grep 569e21b91ea4 + ``` +- Тело issue: `265208f3d6c426f1a44691f2acb0cf9e77464441e7acf3e9e0cec777615aeefb` +- Вердикт конвейера: `green` · High 0