mirror of
https://github.com/Matysh/houseplan-card
synced 2026-10-02 12:49:56 +00:00
@@ -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 | — | — |
|
||||
|
||||
@@ -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`.
|
||||
|
||||
---
|
||||
|
||||
<!-- material-anchors: сгенерировано конвейером (#414) -->
|
||||
|
||||
## Материал раунда
|
||||
|
||||
- Ветка: `dev`, коммит `84ed38e3d5d5` — ребейз его осиротит, и это нормально: ниже якоря, которые ребейз не меняет.
|
||||
- Дерево материала: `51385b4bd49434fbfe510a14c8824a1c48b01d0b`
|
||||
```
|
||||
git log --all --format='%H %T' | grep 51385b4bd494
|
||||
```
|
||||
- Тело issue: `3e716989f90524ce41441b1b6ac157229af3aec237befdc309a8e08b66ad6816`
|
||||
- Вердикт конвейера: `yellow` · High 0
|
||||
Reference in New Issue
Block a user