From c629c9c23835631923df97d0ead3dbfbee51a5b2 Mon Sep 17 00:00:00 2001 From: "claude[bot]" <209825114+claude[bot]@users.noreply.github.com> Date: Thu, 1 Oct 2026 10:06:31 +0000 Subject: [PATCH] docs: review document for #739 Issue: #739 User-Visible: no --- docs/reviews/CODE-REVIEW-739-r1.md | 225 +++++++++++++++++++++++++++++ 1 file changed, 225 insertions(+) create mode 100644 docs/reviews/CODE-REVIEW-739-r1.md diff --git a/docs/reviews/CODE-REVIEW-739-r1.md b/docs/reviews/CODE-REVIEW-739-r1.md new file mode 100644 index 00000000..eb21c411 --- /dev/null +++ b/docs/reviews/CODE-REVIEW-739-r1.md @@ -0,0 +1,225 @@ +# CODE-REVIEW-739-r1 + +**Issue:** #739 · «perf(iso): keep the theme paper across floor switches in 2.5D» +**Трек:** ask (перф, §5) · **Заход:** r1 · блокирующих циклов использовано 0/4 +**Материал:** `20733ac436faa574837812698bb17e98ebd09383` (один коммит поверх `origin/dev`, +merge-base `5a49c8c32df959a5f5450e856b204e0120b5125f`) +**Вердикт:** зелёный + +## Скоуп + +Единственная цель — К1 из ТЗ: в 2.5D с подложкой цвет «бумаги» (фон карточки +темы под изображением плана) должен определяться один раз на идентичность +темы+режима, а не на каждое переключение этажа. До правки каждое переключение +роняло кэш из-за `space` в ключе `isoPaperContext`, что давало второй полный +проход рендера (невидимую, но дорогую заставку `.bootveil` + `getComputedStyle` ++ `requestUpdate()`). Эффект — чисто перформансный; экран не меняется +(`User-Visible: no`, подтверждено в ТЗ и трейлерах коммита). + +Файлы диффа: `src/iso-first-frame.ts` (единственный продуктовый файл), +`test/iso-stage6.test.mjs`, `demo/smoke_iso_floor_switch.mjs` (новый), +`scripts/mutation-registry.mjs`, `scripts/smoke-links.mjs`, `docs/ISOMETRIC.md`. +`src/houseplan-card.ts` не тронут — соответствует заявлению автора и ТЗ +(«Затронутые файлы»). + +Закрывает: J1 (`docs/SCOPE.md`) — «живой обзор дома», частое действие +переключения этажей в киоске/на телефоне; ускоряет путь, которым пользуется +каждый дом с подложкой в 2.5D. + +## Как проверялось + +Дешёвые гейты подтверждены Validate на этом SHA (прогон +[36845529191](https://github.com/Matysh/houseplan-card/actions/runs/36845529191), +success): `npx tsc --noEmit`, `npm test`, `npm run build` + сверка бандла — не +перегонялись повторно. + +Сверх Validate прогнано лично в этом раунде: + +| Гейт | Команда | Результат | +|---|---|---| +| `smoke-select` по диффу | `node scripts/smoke-select.mjs --base origin/dev --head HEAD` | 2 «зарегистрированные связи»: `demo/smoke_iso_first_frame.mjs`, `demo/smoke_iso_floor_switch.mjs` (символы `commitPaper`, `themePaper`) — оба названы в AC2/AC3, оба прогнаны ниже | +| Мутант AC5, гард | `npx tsc -p tsconfig.test.json && node scripts/fix-test-build.mjs && node --test --test-name-pattern="#654\|#739" test/iso-stage6.test.mjs` | 4/4 passed | +| `scripts/mutation-gate.mjs --check` | — | `ok iso-paper-resolved-per-floor`, предупреждения те же, что на `dev` (4 общих + browser guards 201/200, те же, что были) | +| `demo/smoke_iso_floor_switch.mjs` (AC2) | `node demo/smoke_iso_floor_switch.mjs` | **OK** на материале; **красный** (`everySwitchRendersOnce`, `noSwitchProbesThePaperAgain`, `noSwitchInsertsTheVeil` = false, 2 обновления/1 проба/1 веил на переключение) после отката `src/iso-first-frame.ts` к версии `origin/dev` — тест умеет падать, проверено лично (не со слов автора) | +| `demo/smoke_iso_first_frame.mjs` (AC3) | `node demo/smoke_iso_first_frame.mjs` | OK, все 7 проверок true | +| `demo/smoke_isometric_contract.mjs` (AC3) | `node demo/smoke_isometric_contract.mjs` | OK | +| `demo/smoke_iso_flat_parity.mjs` (AC3) | `node demo/smoke_iso_flat_parity.mjs` | OK | +| `demo/smoke_iso_theme_walls.mjs` (AC3) | `node demo/smoke_iso_theme_walls.mjs` | OK | +| `node scripts/no-new-private-writes.mjs --base origin/dev --head HEAD` | — | «Новых записей в приватное состояние карточки нет» (перехват `card.update`/`card._cssColor` в смоке не считается нарушением; у `_cssColor` стоит `// private-ok: #739`) | +| `npm run bundle:budget` | — | initial View headroom 541 Б (ceiling не превышен); предупреждение про низкий запас — долг #367/#474, не новый, не из этого диффа | + +Перед смоками потребовалось `npm run build && node scripts/bundle-sync.mjs`, +чтобы `demo/srv/assets` увидел свежий бандл (сам по себе `dist/` на диске при +старте раунда не был синхронизирован с `demo/srv`). После проверок рабочая +копия возвращена `npm run bundle:clean` — `git status` чист. + +Полная матрица смоков, `golden:verify`, `pytest tests_backend`, performance — +не гонялись (обоснование ниже, «Чего не проверял»). + +## AC — разбор + +**AC1 (состояние, `test/iso-stage6.test.mjs`).** Тест добавлен и покрывает все +шаги (а)–(е) плюс отдельно прогон «уход в редактор и обратно». Прочитан код +`src/iso-first-frame.ts` построчно и прослежен исполнением: + +- `isoPaperContext` больше не кладёт `space` ни в `theme` (идентичность темы), + ни в `key` (теперь `key = [imagePlan, theme]`). Из-за этого `key` у двух + разных этажей с подложкой и одинаковой темой **совпадает** — это и есть + механизм К1: в `prepare()` условие `context.key === this.paperContext` + становится `true` уже на первом же сравнении, без обращения к + `themePaper`-кэшу вообще. +- Переход «подложка → нарисованный план → снова подложка» — другой случай: + `key` меняется ({imagePlan:false}≠{imagePlan:true}), `paperReady` сбрасывается, + но `prepare()` находит `this.themePaper?.theme === context.theme` и мгновенно + коммитит закэшированный rgb без похода в DOM — это шаг (г) теста. + `commitPaper` при этом помечает `floorMemo = null` (т.к. `!paperReady` на + момент вызова истинно), так что `lightFloors()` пересчитывается на новом + (не белом) paper — тест проверяет это явно (`[...]` → `[]` вместо + `['unfilled']`). +- `memoIsoLightFloorRooms` (не тронут диффом) сам ключует на `fills`+`paper` + (`src/iso-materials.ts:118`), поэтому коллапс `key` между разными этажами + с одинаковой темой не ломает пересчёт светлых комнат при смене `fills` — + проверено чтением, отдельного теста на эту комбинацию в АС нет, но риска + регрессии не вижу: `floorMemo` переиспользуется только если оба входа + (цвет и fills) совпали. +- Смена темы или `_mode` входит в `context.theme`, поэтому шаги (д)/(е) + сбрасывают `themePaper` (строка `if (this.themePaper.theme !== context.theme) + this.themePaper = null`) и идут путём #654 — подтверждено тестом и чтением. + +Проверено исполнением (`node --test`, фильтр `#654|#739`): 4/4 зелёные. +Проверено также, что тест красный на коде `origin/dev` конкретно на той +проверке, которую заявляет коммит-сообщение («a floor switch shows no veil», +`pending()` ожидаемо `true` на dev вместо `false`) — убедился чтением логики +(dev-версия не содержит `themePaper`, значит `pending()` после `prepare()` на +шаге (б) обязана остаться `true`). + +**AC2 (один рендер на переключение, `demo/smoke_iso_floor_switch.mjs`).** +Смок прогнан лично: на материале — `OK`, 1 `update`, 0 `_cssColor`, 0 вставок +`.bootveil` на каждое из 6 переключений, `readiness` всегда `ready`, светлые +комнаты совпадают с первым показом этажа. После отката файла до `origin/dev` — +смок **красный** ровно на тех трёх счётчиках, которые и должен был поймать +(`everySwitchRendersOnce`, `noSwitchProbesThePaperAgain`, `noSwitchInsertsTheVeil` += false, 2/1/1 вместо 1/0/0) — дисциплина «тест умеет падать» выполнена не со +слов автора, а исполнением. + +**AC3 (холодный путь и смежные контракты не изменились).** Четыре названных +смока (`smoke_iso_first_frame`, `smoke_isometric_contract`, +`smoke_iso_flat_parity`, `smoke_iso_theme_walls`) прогнаны — все зелёные. +`smoke-select` по диффу вернул только эти два файла как связанные с диффом +(«зарегистрированная связь», не «прямое совпадение» и не «неопределённость») — +оба в списке прогнанных. Красные песочницы (`smoke_summary_*`, +`smoke_infinite_canvas`, `smoke_live_pan_coverage`) из AC3 не проверялись +повторно — судит Validate на этом SHA, как и прописано в самом AC. + +**AC4 (без регрессий, Full Performance).** Ссылка на прогон +[36839009721](https://github.com/Matysh/houseplan-card/actions/runs/36839009721) +проверена через `gh run view`: `conclusion: success`, `headSha: +31464bb69125ec8eaa9df8c67b97f7e08ac9074f`. Эта SHA — более ранняя точка ветки +(до ребейза на текущий `dev`, когда база была `7ff2b5ae`); сам объект-коммит в +локальном репозитории недоступен (история переписана ребейзом), поэтому +побайтово дифф между `31464bb6` и `20733ac4` не сверить без `git fetch`, +который запрещён инструкцией материала. Компенсирующая проверка: `git diff +--stat 7ff2b5ae..origin/dev` показывает, что все 4 коммита, которые легли +между старой базой прогона и текущей, — `AGENTS.md`, `PROCESS.md`, +`docs/process/*`, `scripts/process-gate.mjs`, `scripts/task-packet.mjs`, +`test/moon.test.mjs`, `test/process-*`, `test/task-packet.test.mjs` — не +пересекаются ни с одним файлом диффа #739 (`src/iso-first-frame.ts`, +`test/iso-stage6.test.mjs`, `demo/smoke_iso_floor_switch.mjs`, +`scripts/mutation-registry.mjs`, `scripts/smoke-links.mjs`, +`docs/ISOMETRIC.md`). Значит ребейз не мог изменить проверяемый диф, и +прогон на `31464bb6` доказывает то же самое, что доказал бы прогон на +`20733ac4`. Числа в прогоне (9/9 профилей зелёные, `switchCycleMs` в пределах +±1% на плоских профилях, в isometric `modelReady` −9%, `switchCycleMs` −4% — +улучшение, не регрессия) совпадают с заявленным. Сам Full Performance заново +не гонялся — дорогой гейт, ссылка уже даёт нужное доказательство. + +**AC5 (гейт задачи).** `mutation-gate.mjs --check`: `ok +iso-paper-resolved-per-floor`, патч мутанта (добавляет `space` обратно в +идентичность темы) и гард (`#654|#739` в `test/iso-stage6.test.mjs`) +осмысленны — я прочитал: мутант ломает ровно ту строку, из-за которой правка +вообще нужна. `gate:small` не перегонялся отдельно — он часть Validate, +подтверждённого на этом SHA. `test/core-file-budget.test.mjs` не актуален — +`houseplan-card.ts` диффом не тронут. + +## Трейлеры и changelog + +Коммит несёт `Issue: #739` и `User-Visible: no` — соответствует ТЗ («экран не +меняется»). Правки обоих CHANGELOG не требуются и отсутствуют — корректно. +Единственное число, которое «видно дважды» в материале — бюджетный потолок +бандла View (не меняется диффом, только фактическое значение растёт в +пределах полосы 2000 Б, подтверждено `bundle:budget`, источник один — +`scripts/bundle-budget.mjs`). Других дублирующихся чисел в диффе нет (нет +новых UI-текстов, нет новых констант, видимых пользователю). + +## Отклонения ТЗ → реализация + +Три отклонения автор назвал сам в хендоффе, все проверены чтением и +совпадают с кодом: +1. `space` остался параметром `isoPaperContext`, но не входит в `theme`/`key` — + подтверждено (см. AC1 выше); тесты #654 действительно не менялись + (`git diff` в тестовом файле — только добавление нового теста). +2. Сброс кэша при смене `_mode` — действительно строже «принято + предположительно» ТЗ (там говорилось только про тему); обоснование + (редактор между подложками другого типа плана не должен брать старый + цвет) логично и не противоречит «Режимам» ТЗ («уход в редактор... цвет + ищется заново»). +3. `commitPaper`, `themePaper` добавлены и в запись `#654` `smoke-links` — + подтверждено в диффе `scripts/smoke-links.mjs`. + +## Что проверено и корректно + +- Устранение второго прохода рендера реализовано корректно и доказано и + юнитом, и смоком, с фактическим «тест умеет падать» (не теория). +- Коллапс ключа между разными этажами с одной темой — найден чтением, + совместим с `memoIsoLightFloorRooms`, не ломает пересчёт светлых комнат. +- Холодный путь (#654), смена темы/режима, отказ чанка — не затронуты + (код `pending`/`readiness`/`runtimeFailure` не менялся). +- Гейт приватных записей и бюджет бандла — зелёные, без новых предупреждений + сверх тех, что уже были на `dev`. +- Документация (`docs/ISOMETRIC.md`) обновлена в том же коммите, формулировка + соответствует новому поведению. + +## Чего не проверял + +- **Golden.** Диф меняет путь отрисовки 2.5D (`src/iso-first-frame.ts`), + видимый риск по правилу §5.1 реален в общем случае, но здесь не + реализуется: кадр не меняется — то же решение «нет» подтверждено ТЗ + («На экране ничего не меняется: та же сцена... заставки при переключении + нет — ни сейчас, ни после правки») и структурно: правка не трогает ни + геометрию, ни материалы/заливки (`iso-materials.ts`, `iso-scene-render.ts` + не в диффе), а только то, сколько раз и когда резолвится *тот же самый* + цвет и нужен ли промежуточный `.bootveil`, который и до, и после невидим + глазу (кадр с заставкой и без неё идёт в одной задаче до `requestAnimationFrame`). + Метки `ci:golden` на задаче нет, `golden:verify` не гонял. +- **Full Performance заново.** Не гонял — использовал существующий зелёный + прогон (см. AC4) с обоснованием, что ребейз его не инвалидировал. +- **`pytest tests_backend`.** Диф не трогает `custom_components/**/*.py` — + не применимо. +- **Инварианты модели.** Диф не трогает геометрию комнат/устройств — не + применимо. +- **Полная матрица смоков.** Не гонял — только смоки, названные в AC2/AC3 и + подтверждённые `smoke-select`; полный прогон — предрелизная обязанность. +- **Мутации вне реестра/регистрация нового мутанта в CI-прогоне.** Мутанты в + разработке не гоняются (§2.7, #709); якорь патча проверен `--check`. + +## Находки + +Нет. High: 0, Medium: 0. + +--- + +Вердикт: зелёный · заход r1 · блокирующих циклов 0/4 · High: 0 · Medium: 0 → в задаче + +--- + + + +## Материал раунда + +- Ветка: `issue/739-layout-reads-followup`, коммит `20733ac436fa` — ребейз его осиротит, и это нормально: ниже якоря, которые ребейз не меняет. +- Дерево материала: `886102777be1249389fcd2a662dbe551200250fc` + ``` + git log --all --format='%H %T' | grep 886102777be1 + ``` +- Тело issue: `b4865b454fae1e35d41752478fda842157505fded49f39f20bbaff6fa2794b37` +- Вердикт конвейера: `green` · High 0 · маршрут `fix`