docs: review document for #740

Issue: #740
User-Visible: no

docs/reviews/CODE-REVIEW-740-r1.md

@@ -0,0 +1,205 @@
+# CODE-REVIEW-740-r1
+
+Issue: #740 · «Производительность: слой лестниц — ≈50 мс на переключение на этаж 1»
+Трек: ask (перф, §5) · Этап: code · Заход: r1 · Материал: `1b46c3f42bb8a9df627d237dcee115a162c20dc3`
+(два коммита над `origin/dev`: `0225fbff` — К1, `1b46c3f4` — отпечаток скриншотов после ребейза)
+
+## Скоуп
+
+К1 (единственный пункт контракта): все ступени одной лестницы рисуются одним
+`<path class="hp-stair-tread">` в слое View и в редакторе плана вместо
+3–7 отдельных `<line>`. Строки разметки (контур и `d` ступеней) строятся один
+раз на объект геометрии из `cachedStairRenderGeometry` и кэшируются `WeakMap`
+(`cachedStairMarkup`, `stairTreadPath`, `src/stairs.ts`). Эффект доказывается
+Full Performance (AC4); пиксельная идентичность — golden (AC3, `ci:golden`).
+
+Файлы диффа: `src/stairs.ts`, `src/stairs-view.ts`, `src/stairs-editor.ts`,
+`test/stairs.test.mjs`, `demo/smoke_stairs.mjs`, `scripts/mutation-registry.mjs`,
+`scripts/smoke-links.mjs`, `docs/STAIRS.md`, `docs/testing-notes/mutation-browser-guards.md`,
+`docs/images/screenshots.json` (только отпечаток после ребейза).
+
+## Как проверялось
+
+| Гейт | Статус | Источник |
+|---|---|---|
+| `npx tsc --noEmit`, `npm test`, `npm run build`, `bundle-policy --verify` | зелёные | Validate на `1b46c3f4`: [36897816678](https://github.com/Matysh/houseplan-card/actions/runs/36897816678) — приняты как дешёвые, не перегонял |
+| `npm run build` + `npm run bundle:sync` | зелёные | прогнал сам, для локального запуска смока и golden-диагностики |
+| AC1 — `test/stairs.test.mjs` (две находки с `#740`) | прочитан построчно | не перезапускал отдельно — входит в зелёный `npm test` Validate выше |
+| AC2 — `node demo/smoke_stairs.mjs` | зелёный, все поля `true`, включая `editorTreadsAreOnePath`, `viewTreadsAreOnePath` | прогнал сам локально |
+| `node scripts/smoke-select.mjs --base origin/dev --head HEAD` | одна зарегистрированная связь — `smoke_stairs.mjs` (та же, что в AC2) | прогнал сам |
+| AC5 — `node scripts/mutation-gate.mjs --check` | `ok` для `stairs-view-tread-lines`, реестр и `mutation-browser-guards.md` согласованы; общий WARN 207/200 — существующий фон, не этой задачи | прогнал сам |
+| AC3 — golden (`ci:golden`) | 4 сцены лестниц (`stairs-flat-normal-light`, `stairs-flat-hover-dark`, `stairs-flat-selected-light`, `stairs-isometric-dark`) **passed** | не локально (см. «Чего не проверял») — проверено по логам CI, см. ниже |
+| AC4 — Full Performance | **не подтверждён на материале ревью** | см. находку ниже |
+
+### AC3 отдельно: почему локальный прогон не понадобился
+
+Моя попытка `npm run golden:verify` локально упёрлась в окружение песочницы
+(фетч собранного модуля таймаутит ещё до сверки Chromium — то же расхождение
+окружения, о котором уже писал автор про 3 PDF-сцены). Вместо повторной
+борьбы со средой проверил цепочку переиспользования самого конвейера: job
+«Golden-кадры против принятых эталонов» реально выполнился (не был
+переиспользован) на прогоне [36888262153](https://github.com/Matysh/houseplan-card/actions/runs/36888262153)
+(SHA `5b94c09e`) и по логам все 192 сцены, включая все 4 сцены лестниц,
+`passed`. Сам прогон в целом был `failure`, но из-за несвязанного флака
+`guard_tail_rejection.mjs` (#776 по словам автора) — job golden внутри него
+зелёный независимо. Дальше каждый следующий пуш (`fc083b96`→…→`1b46c3f4`)
+переиспользовал этот результат через маркер `reuse-golden-<hash входов>`
+(`#208`): лог Validate на `1b46c3f4` прямо называет источник —
+`source_sha=5b94c09e0b21701ac0dfcff1a800c5de55ee130d`, «golden не
+прогоняется: входы побайтово те же». Это доказательство исполнением, а не
+заявлением автора: я прочитал реальный вывод `passed` по каждой из четырёх
+сцен на конкретном SHA и прямую связь с материалом ревью по хэшу входов.
+AC3 — выполнен.
+
+## Находки
+
+### Medium (в скоупе) — AC4 не доказан на материале ревью: единственный Full Performance прогон стоит на устаревшей базе
+
+**Где:** AC4 таблицы приёмки (issue #740, раздел «Критерии приёмки»); данные
+в комментарии автора «Сделано».
+
+**Что не так.** На ветке есть ровно один прогон `performance.yml`:
+[36838891001](https://github.com/Matysh/houseplan-card/actions/runs/36838891001),
+на SHA `610134c3` (самый первый коммит задачи) против базы `dev` `7ff2b5ae`.
+После него ветка трижды ребейзилась на ушедший вперёд `dev` (причины —
+красный Validate: устаревший отпечаток скриншотов, конфликт счётчиков
+`mutation-browser-guards.md`, флак `guard_tail_rejection.mjs`) и в итоге
+слилась с `dev` `0606a366` без конфликтов в коде. Новый Full Performance
+после этих ребейзов не запускался — я проверил список прогонов workflow
+`performance.yml` для ветки, там только эта одна запись.
+
+Между измеренной базой (`7ff2b5ae`) и фактической (`0606a366`) в `dev`
+попали как минимум три коммита, напрямую меняющих ту же цену тёплого
+переключения этажа, которую АС4 сравнивает:
+
+- `9324d81b` perf(iso) #739 — убирает двойной рендер-проход в 2.5D с
+  фоновым изображением, локально ≈50 мс на тёплый заход. Это тот же участок
+  (тёплое переключение, 2.5D-профили `isometric`/`isometric-stage3`), что
+  измеряет АС4.
+- `fa18ca81` #747 — прямо по тексту коммита использует **в том числе и сам
+  прогон #740 (`36838891001`)** как один из четырёх калибровочных рядов,
+  чтобы подтянуть `hardMaxMs` вниз до 1550 мс (2.5D) и тоже назвать новый
+  потолок. АС4 утверждает «бюджеты и hardMaxMs не меняются» — для базы
+  `7ff2b5ae` это было верно, для фактической базы слияния `0606a366`
+  уже нет: потолок к этому моменту другой, и откалиброван отчасти по старым
+  числам этой же задачи.
+- `a0f72280`/`dc77852e` #742/#745 — переключение ключей комнатных фигур по
+  `space+id`, тоже на пути `switchCycle`.
+
+Риск 4 самого ТЗ предвидел именно это: «AC4 снимается на ветке, приведённой
+к актуальному dev» — но после финального приведения к `0606a366` новое
+измерение не опубликовано, остались только числа против `7ff2b5ae`.
+
+**Почему это находка, а не придирка к процедуре.** Код К1 между ребейзами не
+менялся (конфликты были только в `docs/testing-notes/mutation-browser-guards.md`
+и `docs/images/screenshots.json`), поэтому сам факт улучшения, скорее всего,
+сохранился. Но АС4 требует не «вероятно», а «медиана `switchCycleMs`
+кандидата в `large-house` ниже базы, в `isometric` и `plan-snap` не выше» —
+а один из профилей сравнения (`isometric`) напрямую пересекается с
+оптимизацией #739, которая уже снизила именно 2.5D-базу, которую АС4 не
+переизмерил. Относительный эффект лестниц (−185…−222 мс по старым данным)
+мог заметно сократиться на новой, уже ускоренной базе, а подтверждения, что
+`isometric`/`isometric-stage3` по-прежнему «не выше базы» на `0606a366`, в
+материале нет.
+
+**Чем доказано, что это отсутствует, а не что я придираюсь:** список
+прогонов `performance.yml` для ветки (ровно один, на устаревшем SHA);
+`git log --ancestry-path 7ff2b5ae..0606a366 -- src/ demo/performance` (пять
+перф-значимых коммитов, в т.ч. #739/#747, лежащих между измеренной и
+фактической базой); текст коммита `fa18ca81`, прямо называющий прогон
+`36838891001` своим калибровочным входом.
+
+**Чем закрывается:** перезапустить
+`gh workflow run performance.yml --ref issue/740-stairs-floor-switch -f comparison_ref=0606a366`
+(или актуальный на момент повторного раунда merge-base) и опубликовать в
+issue числа по всем профилям против него — именно то, что уже требует AC4 и
+что предвидел риск 4 самого ТЗ.
+
+Вердикт — жёлтый из-за этой находки; она в скоупе задачи (это её же AC4),
+отдельный issue не заводится.
+
+## Что проверено и корректно
+
+- **AC1.** `stairTreadPath` строит `d` как склейку `M a0 a1 L b0 b1` по
+  `treads` в порядке геометрии — прочитано построчно, совпадает с контрактом
+  (`src/stairs.ts:345`). `cachedStairMarkup` кэширует по объекту геометрии
+  через `WeakMap`, без пересборки при повторном вызове — тест
+  `test/stairs.test.mjs` доказывает это через `===`-идентичность
+  возвращаемого объекта (а не просто равенство строк), и отдельно показывает
+  инвалидацию при смене геометрии через `item.angle = 45`. Нулевой случай
+  (лестница короче 40 см, 0 ступеней → пустая строка) тоже покрыт.
+  Тест умеет падать: на `dev` символов `cachedStairMarkup`/`stairTreadPath`
+  нет, импорт не соберётся.
+- **AC2.** Разметка: `path.hp-stair-tread` ровно один вместо `line`-ов,
+  порядок и остальные классы (`outline`, `hit`, `trapezoid`×3/0, `arrow`) не
+  изменились — прочитано в `src/stairs-view.ts`/`src/stairs-editor.ts` и
+  подтверждено прогоном `demo/smoke_stairs.mjs` (все поля `true`, в т.ч.
+  новые `editorTreadsAreOnePath`/`viewTreadsAreOnePath`). У лестницы без
+  ступеней элемента нет (`markup.treads ? ... : nothing`).
+  `smoke-select` показывает, что `smoke_stairs.mjs` — единственная и уже
+  зарегистрированная связь для новых символов; новых непокрытых символов нет.
+- **AC3.** Golden для всех 4 сцен лестниц `passed`, проверено исполнением
+  через цепочку переиспользования CI (см. выше) — не только по словам
+  автора.
+- **AC5.** Мутант `stairs-view-tread-lines` зарегистрирован, откатывает
+  именно `src/stairs-view.ts` к старым `<line>`, гард —
+  `smoke_stairs.mjs`, который на такой откат покраснеет
+  (`editorTreadsAreOnePath`/`viewTreadsAreOnePath`/`count('line.hp-stair-tread')===0`
+  перестанут выполняться). Запись в `docs/testing-notes/mutation-browser-guards.md`
+  и `scripts/smoke-links.mjs` на месте. `mutation-gate.mjs --check` — зелёный.
+- **Остальной контракт К1.** Порядок DOM-элементов внутри `<g class="hp-stair...">`
+  не изменился (outline → hit → trapezoid → treads → arrow); класс
+  `hp-stair-view` по-прежнему различает слои — это использует и сам смок для
+  независимой проверки каждого слоя. `.hp-stair-tread` в
+  `src/styles/plan.styles.ts:1645` селектит и `path`, и `line` без правки,
+  как и требовал контракт. Редактор (`stairs-box.ts`, черновик, ручки) К1 не
+  трогает — изменений там нет. `src/space-render.ts` и `src/pdf/pdf-scene.ts`
+  не задеты, что соответствует «не-скоупу» issue.
+- **Трейлеры.** Оба коммита несут `Issue: #740` и `User-Visible: no`;
+  поведение действительно не видно пользователю (markup-рефакторинг под
+  капотом), changelog не правился — корректно.
+- **Числа.** DOM-счётчик «4210 → 2960» в issue и в теле коммита `0225fbff`
+  совпадает дословно — один источник, не разъехался.
+
+## Чего не проверял
+
+- **Full Performance** сам не запускал — это дорогой гейт (десятки минут,
+  выделенный workflow с `comparison_ref`); его обязан прогнать автор, см.
+  находку выше.
+- **`npm test` целиком и `npx tsc --noEmit`** отдельно не перегонял — принял
+  зелёный Validate на точном SHA материала (`1b46c3f4`) как подтверждение
+  дешёвых гейтов согласно прилагаемой инструкции; `npm run build` всё же
+  прогнал сам (для локального смока и попытки golden), он тоже зелёный.
+- **`golden:verify` локально** — не прогнал до конца (окружение песочницы:
+  Chromium/фетч бандла ведут себя иначе, чем в CI-профиле). Заменил на
+  чтение логов CI (см. AC3 выше) — это проверка исполнением на конкретном
+  SHA, а не доверие к заявлению автора.
+- **`npm run invariants`** — не прогонял: диапазон диффа не меняет саму
+  геометрию (`stairRenderGeometry`, `treads`, `outline` остались прежними
+  данными), меняется только то, как её точки сериализуются в разметку;
+  степень риска инвариантов геометрии к этой задаче не относится.
+- **`pytest tests_backend`** — не прогонял, диффа в Python нет.
+- **Полный `golden:capture`/ручной визуальный осмотр скриншотов** — не делал;
+  положился на пиксельное сравнение CI, которое строже визуального осмотра.
+
+## Вердикт
+
+Жёлтый. Единственная находка — Medium, в скоупе задачи (AC4 этой же issue):
+перезапустить Full Performance против актуального `dev` и опубликовать
+числа. Код К1, AC1/AC2/AC3/AC5 проверены и корректны — при зелёном AC4
+задача готова к повторному зелёному вердикту без доп. находок по коду.
+
+---
+
+<!-- material-anchors: сгенерировано конвейером (#414) -->
+
+## Материал раунда
+
+- Ветка: `issue/740-stairs-floor-switch`, коммит `1b46c3f42bb8` — ребейз его осиротит, и это нормально: ниже якоря, которые ребейз не меняет.
+- Дерево материала: `14c4fb8c2402beb23220a3ca0e3d4e45f06f5e43`
+  ```
+  git log --all --format='%H %T' | grep 14c4fb8c2402
+  ```
+- Тело issue: `9cfcfc9e8157d5a25961e793ed530de257848fb5d870dee885fbd71103ffb3bd`
+- Вердикт конвейера: `yellow` · High 0 · маршрут `fix`
+<!-- hp:usage input_tokens=4228 output_tokens=36126 cache_creation_input_tokens=122259 cache_read_input_tokens=4857388 num_turns=65 -->