From 2f1be1a86a859366e5d01738f7689e73b329f55c Mon Sep 17 00:00:00 2001 From: "claude[bot]" <209825114+claude[bot]@users.noreply.github.com> Date: Wed, 30 Sep 2026 20:28:41 +0000 Subject: [PATCH] docs: review document for #694 Issue: #694 User-Visible: no --- docs/reviews/INDEX.md | 3 +- docs/reviews/SPEC-REVIEW-694-r1.md | 270 +++++++++++++++++++++++++++++ 2 files changed, 272 insertions(+), 1 deletion(-) create mode 100644 docs/reviews/SPEC-REVIEW-694-r1.md diff --git a/docs/reviews/INDEX.md b/docs/reviews/INDEX.md index 4f88a412..531d533a 100644 --- a/docs/reviews/INDEX.md +++ b/docs/reviews/INDEX.md @@ -1,6 +1,6 @@ # Индекс ревью -Генерируется `node scripts/reviews-index.mjs` (#635) — не редактировать руками. Документов: 201, issue: 96. Вердикт: 🟢 зелёный · 🟡 жёлтый · 🔴 красный · ⚪ не распознан (свободная форма старых документов). H/M — число High/Medium по строке вердикта или заголовкам находок. Файлы — пути, названные в находках; ищите по имени файла: `grep form-kit INDEX.md`. +Генерируется `node scripts/reviews-index.mjs` (#635) — не редактировать руками. Документов: 202, issue: 97. Вердикт: 🟢 зелёный · 🟡 жёлтый · 🔴 красный · ⚪ не распознан (свободная форма старых документов). H/M — число High/Medium по строке вердикта или заголовкам находок. Файлы — пути, названные в находках; ищите по имени файла: `grep form-kit INDEX.md`. | Issue | Документ | Этап · раунд | Вердикт | H | M | Находки | Файлы | |---|---|---|---|---:|---:|---|---| @@ -29,6 +29,7 @@ | #696 | [CODE-REVIEW-696-r1.md](CODE-REVIEW-696-r1.md) | code · r1 | 🟢 зелёный | 0 | 0 | — | — | | #695 | [CODE-REVIEW-695-r1.md](CODE-REVIEW-695-r1.md) | code · r1 | 🟡 жёлтый | 0 | 1 | «инфраструктура без метки трека» не читается как track:show нигде в коде | `PROCESS.md` `.github/workflows/_process.yml` `scripts/task-packet.mjs` `_process.yml` `task-packet.mjs` `docs/process/AUTHOR.md` `test/task-packet.test.mjs` | | #695 | [CODE-REVIEW-695-r2.md](CODE-REVIEW-695-r2.md) | code · r2 | 🟢 зелёный | 0 | 0 | — | — | +| #694 | [SPEC-REVIEW-694-r1.md](SPEC-REVIEW-694-r1.md) | spec · r1 | 🟡 жёлтый | 0 | 0 | 1. Отсутствуют разделы «Сценарий» и «Что человек увидит до и после», обязательные по §7…; 1. UX, модель данных/миграция, i18n и touch-влияние не названы явно «нет» отдельными ст… | `docs/SCOPE.md` | | #692 | [CODE-REVIEW-692-r1.md](CODE-REVIEW-692-r1.md) | code · r1 | 🟢 зелёный | 0 | 0 | — | — | | #691 | [SPEC-REVIEW-691-r1.md](SPEC-REVIEW-691-r1.md) | spec · r1 | 🟡 жёлтый | 0 | 1 | направление edge-swipe не квантифицировано и не отмечено как допущение | `src/houseplan-card.ts` | | #691 | [SPEC-REVIEW-691-r2.md](SPEC-REVIEW-691-r2.md) | spec · r2 | 🟢 зелёный | 0 | 0 | — | — | diff --git a/docs/reviews/SPEC-REVIEW-694-r1.md b/docs/reviews/SPEC-REVIEW-694-r1.md new file mode 100644 index 00000000..eb66cb03 --- /dev/null +++ b/docs/reviews/SPEC-REVIEW-694-r1.md @@ -0,0 +1,270 @@ +# SPEC-REVIEW-694-r1 — «Исследовать и устранить performance-регрессии v1.78 относительно v1.77» + +Issue: [#694](https://github.com/Matysh/houseplan-card/issues/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