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

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 — приняты как дешёвые, не перегонял
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 (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, на 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 задача готова к повторному зелёному вердикту без доп. находок по коду.

---

Материал раунда

• Ветка: issue/740-stairs-floor-switch, коммит 1b46c3f42bb8 — ребейз его осиротит, и это нормально: ниже якоря, которые ребейз не меняет.
• Дерево материала: 14c4fb8c2402beb23220a3ca0e3d4e45f06f5e43

  git log --all --format='%H %T' | grep 14c4fb8c2402
  

• Тело issue: 9cfcfc9e8157d5a25961e793ed530de257848fb5d870dee885fbd71103ffb3bd
• Вердикт конвейера: yellow · High 0 · маршрут fix