houseplan-card/docs/reviews/SPEC-REVIEW-694-r1.md

SPEC-REVIEW-694-r1 — «Исследовать и устранить performance-регрессии v1.78 относительно v1.77»

Issue: #694 Этап: spec (§2.4) Трек: ask (производительность, метка track:ask) Заход: r1 (первый; разделы «Унаследовано из r0» и «Закрытие раунда r0» не нужны — §2.10 применяется со второго захода)

Вердикт

Жёлтый. High: 0. Medium в скоупе: 3 (возвращаются автору, отдельный issue не заводится, #202). Medium вне скоупа: 0. Low: 1 (снята решением ревьюера с записью ниже, доработка не обязательна).

Скоуп разбора

Полный разбор: тело issue #694 целиком (описание принятого performance-долга до раздела ## ТЗ, сам раздел ## ТЗ — «Что остаётся», «Причины и правки», «Критерии приёмки», «Не входит», «Принятые предположения») и все пять комментариев (два от владельца о выпуске v1.78.0 и решении по viewToggleMs, три от автора — атрибуция регрессий по бетам, прогон после #711/#713, разбор long tasks, финальная «Оценка»). Дополнительно прочитаны docs/SCOPE.md, AGENTS.md, docs/process/REVIEWER.md, PROCESS.md §1, §2.3–§2.5, §2.10, §4, §5, §7.1, §7.2.

Поскольку все три названные в ТЗ правки (П1–П3) — точные утверждения о текущем коде, а не только план на будущее, разбор включает чтение dev на SHA материала (84ed38e3, рабочая копия детач на нём, продуктовый код ещё не менялся — задача только начинается, ветка issue/694-… не заведена):

• src/houseplan-card.ts:2542–2567 (connectedCallback, _pointerHoverObserver) — подтверждает П1 буквально: для каждой записи мутации локальная функция inDeviceLayer вызывает element.matches(...) и element.querySelector('.devlayer') по всему поддереву цели заново, без дедупликации между записями одной пачки; for (const node of record.addedNodes) this._syncPointerHoverSubtree(node) вызывается безусловно для каждой записи — именно то, что AC1 требует оставить без изменений;
• src/stairs-view.ts:63–77 (renderLayer) — подтверждает П2: this.owner._model читается один раз вне цикла (строка 64, для spaceIds) и второй раз внутри .map по лестницам (строка 76, для targetTitle) — то есть геттер вызывается 1 + количество активных лестниц раз за рендер, а не один; src/houseplan-card.ts:3822 подтверждает, что _model — геттер с внутренним кэшем по _cfgEpoch + _cfgFingerprint(), так что лишние вызовы — лишняя пересборка отпечатка, как и написано в ТЗ;
• src/i18n/language-runtime.ts:105–120 (languageRenderGate) — подтверждает П3: host.setAttribute('lang', …) вызывается безусловно на каждом проходе с state !== 'pending', без сравнения с уже установленным значением;
• test/core-file-budget.test.mjs:52 — 'src/houseplan-card.ts': 12896, факт wc -l src/houseplan-card.ts = 12895: запас ровно одна строка, ограничение «П1 компенсируется в той же функции, потолок не поднимается» в «Принятых предположениях» — не декларация, а точное описание реального состояния храповика; src/stairs-view.ts и src/i18n/language-runtime.ts в CAPS не числятся, бюджетного ограничения для П2/П3 нет, и ТЗ корректно не упоминает его для них;
• demo/smoke_stairs.mjs, demo/smoke_device_hit_capsules.mjs, demo/smoke_french_locale.mjs — все три существуют, AC5 не ссылается на несуществующие смоки;
• test/device-hit-owner.test.mjs, test/device-hit-owner-contract.test.mjs — существуют, _invalidateDeviceHitGeometry (строка 5561) — та же функция, которую вызывает наблюдатель П1 при deviceGeometryChanged, то есть регресс по AC1 действительно ловится этими тестами, а не случайным соседом;
• demo/performance/evaluate.mjs:34,149–157 — метрика longTask.countP95 из AC4 реальна и вычисляется именно так, как названо в разборе комментариев; demo/performance/budgets-{isometric,isometric-stage3-dense,large-house-*,interaction}*.json — все профили из AC4 (isometric, isometric-stage3, interaction, plan-snap, large-house) существуют как отдельные бюджеты;
• git log --oneline -1 = 84ed38e3 test(perf): the Stage 4 dense contract requires no nudged overlay (#719) — подтверждает пункт ТЗ «уже сделано: … #719 (контракт бенчмарка плотной сцены)»: это не будущее намерение, а факт на материале ревью.

Проверка §7.1 — комплектность

Присутствуют: проблема («Что остаётся» — точные цифры превышений с источником-прогоном) · контракт поведения (П1–П3 явно: «поведение не меняется», подтверждено чтением кода выше) · скоуп и не-скоуп («Не входит» — архитектура DOM лестниц, прочие кандидаты профилирования, методика бенчмарка — с явной мотивировкой каждого пункта) · критерии приёмки AC1–AC5 с доказательством (unit/perf/smoke + golden у каждого) · затронутые файлы (src/houseplan-card.ts, src/stairs-view.ts, src/i18n/language-runtime.ts названы поимённо в П1–П3) · release-артефакты для User-Visible: no (явно: «строка CHANGELOG не нужна»; AC4 требует приложить in-issue цифры по окнам, если часть бюджетов останется красной).

Отсутствуют или не выделены явным разделом (находки ниже):

• сценарий и что человек увидит до и после — оба обязательных продуктовых раздела §7.1 отсутствуют в самом ## ТЗ;
• риски — не собраны отдельным разделом;
• откат — не назван вообще, ни как раздел, ни как строка.

UX, модель данных/миграция, i18n и touch-влияние явно не названы «нет», но по содержанию ТЗ однозначно читаются как неприменимые (три внутренние оптимизации без новых экранов, полей конфигурации, i18n-ключей или touch-путей) — это Low, не Medium (см. находки).

Проверка AC1–AC5 на однозначность и доказуемость

• AC1 — однозначен: наблюдаемый oracle назван явно (счётчик запросов поддерева на инструментированном узле), граница фикса точна (только дедуп проверки inDeviceLayer, _syncPointerHoverSubtree остаётся безусловным), подтверждено чтением реального кода наблюдателя (см. «Скоуп разбора»).
• AC2 — однозначен: spy на геттере _model, ожидание «ровно один раз за любое число лестниц», инвариант «подписи целевого этажа не меняются» — измеримо и falsifiable.
• AC3 — однозначен: условие «не вызывает setAttribute, если значение уже равно нужному» плюс два позитивных случая (смена языка, fallback на en) — полная таблица истинности для этой функции.
• AC4 — доказательство названо точно (Full Performance, v1.77.0, пять профилей, включая longTask.countP95), все имена профилей и метрика существуют в demo/performance/** (см. «Скоуп разбора»). Ambiguity — см. находку Medium ниже: оговорка про лестницы не называет способ подтвердить саму причину.
• AC5 — однозначен, все три названных smoke-файла существуют, требование «DOM и пиксели не меняются, golden без новых кадров» — стандартная и проверяемая формулировка для behavior-preserving рефакторинга.

Ни один AC не описывает решение, для которого в issue/каноне нет опоры: П1–П3 и AC1–AC3 линейно соответствуют друг другу, AC4/AC5 соответствуют разделу «Критерии готовности» исходного описания issue.

Находки

Medium (в скоупе, возврат автору)

1. Отсутствуют разделы «Сценарий» и «Что человек увидит до и после», обязательные по §7.1. ТЗ описывает исключительно техническую сторону регрессии (метрики, коммиты, причины) и ни разу не формулирует, какая персона docs/SCOPE.md и на какой поверхности сталкивается с проблемой, которую закрывает задача. Сам материал это знает — комментарий от 30.09 прямо называет: «Это частое действие (киоск, вкладки этажей) … главная оставшаяся цель задачи» — то есть медленное тёплое переключение этажей на вкладках, которое видят household members на кухонном планшете и владелец на десктопе (J1 docs/SCOPE.md). Но эта формулировка осталась в комментарии-анализе и не перенесена в ## ТЗ как отдельный раздел. §7.1 прямо предупреждает: «ТЗ, которое не может ответить на эти два вопроса, описывает работу, а не изменение продукта» — сейчас формально это так. Почему это не техническая деталь: без явного сценария код-ревьюер на следующем этапе не может проверить, что производительность улучшилась именно там, где это видно пользователю (а не только в синтетическом бенчмарке) — а именно этот вопрос уже поднимался в комментариях автора («это частое действие … главная оставшаяся цель»). Правка: двумя предложениями перенести в ## ТЗ то, что уже есть в комментарии 30.09 — кто и когда встречает регрессию (кухонный планшет/киоск, тёплое переключение вкладок этажей, админ при проверке плотных сцен) и что человек увидит после фикса («переключение этажей снова быстрое, экран не меняется ни на пиксель»).

2. Отсутствуют разделы «Риски» и «Откат», оба обязательны по §7.1, «Откат» также обязателен по DoR (§2.5: «откат: как выключить или вернуть назад»). Ни в ## ТЗ, ни в «Принятых предположениях» нет ни одной строки о том, как выключить или отменить правку, если П1–П3 после мержа проявят регресс, который не поймали unit/smoke/golden (например, дедуп в П1 пропустит реальное изменение геометрии устройства на редкой комбинации мутаций). Риск-материал у задачи есть и весьма содержателен — «запас в core-file-budget ровно одна строка» (Принятые предположения), «бенчмарк isometric-stage3 был сломан до #719» (Что остаётся / история комментариев), «часть бюджетов может остаться красной по причине лестниц, не по причине этой задачи» (AC4) — но он рассыпан по разным разделам и не назван риском нигде явно. Как стоит сейчас, ТЗ не может пройти чеклист DoR §2.5 буквально: пункт «откат» не закрыт ни фактом, ни явным «нет» (а «нет» здесь и не подошло бы — любая продуктовая функция может быть отменена обычным git revert, но это тоже нужно написать хотя бы одной строкой, поскольку правка №1 лежит в файле на пределе бюджета строк и в самом чувствительном к производительности пути карточки). Правка: добавить раздел «Риски» (собрать три пункта выше) и раздел «Откат» (для внутренних поведенчески-нейтральных правок это может быть одна строка: обычный revert коммита, флагов/миграций нет — но она должна быть написана, а не подразумеваться).

3. AC4 содержит исключение без названного способа его подтвердить. Формулировка: «Если профиль остаётся красным только из-за этажа с лестницами (цена самой фичи, причина 4 разбора), в issue — цифры по окнам и предложение владельцу». Эскалация владельцу здесь корректна по духу §7.1 (продуктовый компромисс — решение владельца, не автора), но сам ТЗ не называет, как отличить «редness только из-за лестниц» от «правки недостаточны». В комментариях уже есть готовый метод («Если убрать лестницы из фикстуры, dev в 2D совпадает с базой» — «Что остаётся»), но он не зафиксирован как обязательный шаг проверки в самом AC4. Без этого код-ревьюер следующего этапа не сможет отличить обоснованную эскалацию от недоделанного фикса, выданного за «фичевую цену» — а это ровно тот вопрос, который §7.1 требует не оставлять на усмотрение читателя. Правка: в AC4 явно назвать метод атрибуции (например: «прогон с той же фикстурой без этажа лестниц воспроизводит бюджет v1.77.0 → редность отнесена на лестницы»), а не оставлять решение «только из-за лестниц» недоказуемым утверждением.

Low (снята решением ревьюера, доработка не обязательна)

1. UX, модель данных/миграция, i18n и touch-влияние не названы явно «нет» отдельными строками (DoR §2.5). По содержанию всех трёх правок (внутренний дедуп MutationObserver, кэш чтения геттера, условный setAttribute) это однозначно неприменимые разделы — ни новых экранов, ни новых полей конфигурации, ни i18n-ключей, ни touch-специфичного кода П1–П3 не затрагивают, а AC1/AC3 явно фиксируют «поведение не меняется». Не поднимаю до Medium: содержание уже отвечает на все четыре вопроса имплицитно и однозначно, добавление четырёх строк «нет» — гигиена оформления, а не пробел в контракте. Автору стоит добавить эти строки при следующей правке ТЗ заодно с находками Medium выше, отдельного цикла ради этого не нужно.

Что проверено и корректно

• Трек ask обоснован верно: это P1-перф-долг с продуктовыми развилками (эскалация владельцу по AC4), не подходит под show/ship (§5).
• Все технические утверждения о текущем коде (П1–П3) сверены построчно с dev@84ed38e3 и совпадают дословно, включая точный номер строки геттера _model, механику _cfgFingerprint и безусловность setAttribute — ни одна причина не оказалась устаревшей или неточной.
• Ограничение core-file-budget (запас 1 строка) численно подтверждено (test/core-file-budget.test.mjs:52 = 12896, факт 12895).
• Все имена тестов, смоков и performance-профилей, на которые ссылаются AC1–AC5, существуют в дереве материала — ни один AC не указывает на несуществующий артефакт.
• «Уже сделано» (#711, #713, #719, #720) соответствует состоянию dev: HEAD-коммит материала — именно #719 («no nudged overlay»), заявленный как выполненный.
• Продуктовых вопросов владельцу в тексте ТЗ не осталось: предыдущий открытый вопрос из комментария 30.09 («раскладывать иконки по размеру, не зависящему от состояния?») закрыт по факту — автор нашёл поведенчески-нейтральное решение (отсечение по лексикографической границе, #711/#713), и в текущее ТЗ этот вопрос не попал как нерешённый; повторно поднимать его не нужно.
• AC4 корректно направляет решение о недостижимых бюджетах владельцу, а не принимает его самостоятельно (единственный найденный пробел — не в этом принципе, а в способе доказать применимость исключения, находка Medium 3).
• Раздел «Не входит» мотивирует каждый пункт (архитектура DOM лестниц, прочие кандидаты профилирования, методика бенчмарка) и не пытается тихо расширить или сузить скоуп относительно исходного описания issue.

Чего не проверял

• Не запускал npx tsc --noEmit, npm test, npm run build, гейты производительности или golden — на этапе ТЗ продуктового диффа ещё нет (ветка issue/694-… не создана, детач-копия на dev@84ed38e3 без изменений), гонять гейты не над чем; зелёный Validate на этом SHA (материал дешёвых гейтов) сюда не относится — это гейт code-review, не spec-review.
• Не проверял историческую точность процентов и медиан в таблицах комментариев (например, точные +164%/+131% по бетам) — это принятый владельцем анализ предыдущего цикла (#692 поправлен и подтверждён самим автором), не предмет ревью текущего ТЗ.
• Не выполнял предложенный в комментариях локальный прототип (лексикографическое отсечение, кэш «иконка у стены/в комнате») — эти правки уже слиты отдельными задачами (#711, #713) до открытия текущего issue и не входят в скоуп П1–П3.
• Не оценивал техническую реализуемость самого механизма «инструментированный счётчик запросов поддерева» из AC1 — это деталь исполнения, оставленная автору («принято предположительно, поменять свободно»), а не часть контракта, который проверяет ревью ТЗ.

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

Issue #694, тело на момент вынесения вердикта (описание принятого performance-долга плюс раздел ## ТЗ, включая пять комментариев с атрибуцией регрессий, промежуточными замерами и финальной оценкой). Рабочая копия — detached HEAD на dev@84ed38e3d5d511687de48e8ca4dae714f82efe2f без продуктовых изменений (ветка issue/694-… не заведена); код читался с этого дерева исключительно как контекст для проверки фактических утверждений ТЗ (П1–П3, core-file-budget, имена тестов/профилей), а не как материал стадии code-review. sha256 тела issue на момент проверки: efc82985614a54c4fc89e485322018a7f5525fbc0bc3a0304f231fe8453c9c4b.

---

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

• Ветка: dev, коммит 84ed38e3d5d5 — ребейз его осиротит, и это нормально: ниже якоря, которые ребейз не меняет.
• Дерево материала: 51385b4bd49434fbfe510a14c8824a1c48b01d0b

  git log --all --format='%H %T' | grep 51385b4bd494
  

• Тело issue: 3e716989f90524ce41441b1b6ac157229af3aec237befdc309a8e08b66ad6816
• Вердикт конвейера: yellow · High 0