mirror of
https://github.com/Matysh/houseplan-card
synced 2026-09-29 03:09:36 +00:00
committed by
Sergey Matyunin
parent
029aba6b88
commit
338700bdcb
@@ -0,0 +1,226 @@
|
||||
# SPEC-REVIEW-451-r1
|
||||
|
||||
- **Issue:** https://github.com/Matysh/houseplan-card/issues/451
|
||||
- **Этап:** spec (PROCESS.md §2.4)
|
||||
- **Заход:** r1 (первое ревью ТЗ, лёгкий трек не применяется — S2-analysis назвал
|
||||
четыре нарушенных критерия §5: риск 9/10, больше одной поверхности, влияние
|
||||
на performance, влияние на touch)
|
||||
- **Материал:** `docs/specs/451-render-performance.md`, коммит `71c3360363991a042fbf3d84a6aeb4ecb772c83f`
|
||||
(`git rev-parse HEAD` на момент вывода), ветка `issue/451-render-performance`
|
||||
(HEAD detached at `origin/issue/451-render-performance`)
|
||||
- **Вердикт:** жёлтый · заход r1 · блокирующих циклов 1/4 · High: 0 · Medium: 3 → в задаче
|
||||
|
||||
## Скоуп ревью
|
||||
|
||||
Проверялся только сам файл ТЗ и тело/комментарии issue #451 — продуктового кода
|
||||
для #451 ещё нет (коммит `71c33603` содержит только `docs/specs/451-render-performance.md`
|
||||
и запись в `docs/specs/README.md`). Ревью состязательное: разбор велся без
|
||||
устных пояснений автора, по тексту ТЗ и по текущему `dev`.
|
||||
|
||||
## Как проверялось
|
||||
|
||||
1. Прочитаны `docs/SCOPE.md`, `AGENTS.md`, `PROCESS.md` целиком (включая §2.4,
|
||||
§2.10, §4, §5, §7.1, §7.2, §8).
|
||||
2. Прочитано тело issue #451 и все комментарии (замеры, причины A/B/C/D,
|
||||
аналитика, вопросы Q1/Q2 и решения владельца).
|
||||
3. Прочитан ТЗ-документ целиком (`docs/specs/451-render-performance.md`, 516 строк).
|
||||
4. Факты, заявленные в §3 ТЗ как установленные причины, сверены с текущим
|
||||
`dev` построчно:
|
||||
- `_renderBody()` безусловно вызывает `houseplanDiagnostics()` —
|
||||
`src/houseplan-card.ts:11315`, подтверждено;
|
||||
- обход всех живых bindings в `houseplanDiagnostics()`/`_bindingStatus()` —
|
||||
`src/houseplan-card.ts:4748-4771`, подтверждено;
|
||||
- три `data-*` атрибута читают один и тот же объект `diagnostics`,
|
||||
вычисленный один раз на рендер (строки `11349-11351` и `11430-11432`
|
||||
используют одну и ту же переменную `diagnostics`) — «одно число, один
|
||||
источник» соблюдено уже в текущем коде и не нарушается контрактом ТЗ;
|
||||
- `_view`/`_zoom` зарегистрированы `state: true` — `src/houseplan-card.ts:2557-2558`,
|
||||
подтверждено;
|
||||
- `shouldUpdate()` в файле отсутствует (есть только `willUpdate()`) —
|
||||
подтверждено;
|
||||
- `createRenderDeviceSnapshot()` существует и используется — `src/houseplan-card.ts:4687`,
|
||||
подтверждено.
|
||||
5. Сверены абсолютные потолки, упомянутые в §11.3 ТЗ, с `demo/performance/budgets.json`:
|
||||
`panZoomMs.hardMaxMs = 500`, `stateUpdateMs.hardMaxMs = 1000` — совпадает
|
||||
с тем, что ТЗ называет уже существующим.
|
||||
6. Проверено существование каждого упомянутого в ТЗ файла-адресата документации:
|
||||
`docs/PERFORMANCE.md`, `docs/SCREENSHOTS.md`, `demo/performance/README.md`,
|
||||
`demo/benchmark_large_house.mjs`, `demo/performance/card-contract.mjs` —
|
||||
через `find`/`ls` по репозиторию.
|
||||
7. Проверены связанные issue (#34, #82, #137, #156, #380, #396, #449) через
|
||||
`gh issue view` — все существуют и релевантны тому, для чего их приводит ТЗ.
|
||||
8. Проверен термин `POINTER_HOVER_QUERY`/«fine hover capability» из §9 ТЗ —
|
||||
совпадает с реальной константой `src/pointer-modality.ts:5`.
|
||||
9. Численные данные §11.3 («наблюдавшихся 3 432–5 538 мс») сверены с телом
|
||||
issue и всеми его комментариями (`gh issue view --json body,comments`) —
|
||||
`grep` на «5538»/«5 538» не дал ни одного совпадения ни в issue, ни где-либо
|
||||
в репозитории вне самого нового ТЗ.
|
||||
|
||||
Гейты (`typecheck`/`test`/`build`/смоки/perf) не прогонялись и не должны были:
|
||||
на этом этапе нет продуктового кода, диапазон изменений — только `docs/specs/**`.
|
||||
Это осознанное решение по этапу, а не пропуск.
|
||||
|
||||
## Что проверено и корректно
|
||||
|
||||
- Обязательные разделы §7.1 присутствуют и в правильном порядке: сценарий (§1),
|
||||
что человек увидит до/после (§2), проблема (§3), скоуп/не-скоуп (§4-5),
|
||||
контракт поведения (§6-8), UX/touch/a11y (§9), модель данных/совместимость/
|
||||
безопасность (§10), performance (§11), AC1-AC13 с методом доказательства
|
||||
(§12), план автотестов (§13), план реализации (§14), release-артефакты (§15),
|
||||
риски (§16), откат (§17), явный блок «принято предположительно» (§18).
|
||||
- Продуктовые решения владельца (Q1 — deferred-last-wins после `pointerup`/
|
||||
`pointercancel`; Q2 — обычный hover входит в задачу через lightweight-слой)
|
||||
корректно перенесены в контракт (§6.3, §8.3) и не переоткрываются как
|
||||
вопросы — ТЗ их фиксирует как решённые, а не как предположения автора.
|
||||
- Диагноз причин A/B/C в §3 не является догадкой: каждое утверждение проверяется
|
||||
чтением текущего кода (см. «Как проверялось», п.4) и совпадает с ним дословно.
|
||||
- Собственное наблюдение автора из комментария issue («редакторские жесты не
|
||||
замерялись, синтетический `pointermove` не доходит до обработчиков») корректно
|
||||
закрыто требованием в плане тестов: «Synthetic events обязаны доходить до
|
||||
production handlers; `0` событий у handler не принимается как доказательство
|
||||
`0` renders» (§13.2). Это именно то усиление, которого просил сам owner-комментарий
|
||||
(«жест не перерисовывает содержимое» проверяется на демо-стенде, где портить
|
||||
нечего).
|
||||
- Non-scope (§5) точно очерчивает границу: внешний вид, hit area, snap
|
||||
tolerance, geometry-алгоритмы, persisted config/schema, ослабление бюджетов —
|
||||
всё явно исключено, что снижает риск скрытого расширения скоупа при
|
||||
реализации сложной (9/10) задачи.
|
||||
- AC1-AC13 пронумерованы, у каждого указан метод доказательства
|
||||
(`unit`/`smoke`/`golden`/`performance`/«код-ревью»/«review»), формулировки
|
||||
проверяемы по структурным assertions §11.2 (счётчики full render, а не
|
||||
субъективные «стало быстрее»).
|
||||
- «Один жест — один результат» выдержан симметрично для всех трёх редакторов
|
||||
через таблицу §8.4, без привилегирования Плана перед Устройствами/Подложкой.
|
||||
- Откат (§17) реалистичен: revert коммитов, точечный аварийный fallback на
|
||||
один жест без отключения diagnostics cache/HA filter, с обязательным issue —
|
||||
это не пустая формальность, а конкретный механизм.
|
||||
- Ссылки на связанные issue (#34, #82, #137, #156, #380, #396, #449) все
|
||||
существуют и действительно относятся к тем контрактам, которые ТЗ обязуется
|
||||
не сломать (проверено `gh issue view`).
|
||||
|
||||
## Находки
|
||||
|
||||
### M1 — `docs/PERFORMANCE.md` не существует, но назван обязательным артефактом (Medium, в скоупе)
|
||||
|
||||
**Файл:** `docs/specs/451-render-performance.md`, строки 13, 390-391 (AC13), 452
|
||||
(план реализации, п.7), 469 (release-артефакты).
|
||||
|
||||
**Проблема:** ТЗ трижды в разных разделах требует обновить `docs/PERFORMANCE.md`
|
||||
как уже существующий канонический документ performance harness («**Связано:**
|
||||
… `docs/PERFORMANCE.md`»; AC13 «`docs/PERFORMANCE.md` и `demo/performance/README.md`
|
||||
описывают новый профиль…»; §15 «Также обновляются: `docs/PERFORMANCE.md`»).
|
||||
Такого файла в репозитории нет ни на `dev`, ни где-либо ещё — проверено
|
||||
`find docs -iname "*performance*"`. Канонический документ performance harness
|
||||
на сегодня — `demo/performance/README.md` (на него явно ссылается
|
||||
`docs/DEVELOPMENT.md:165`).
|
||||
|
||||
**Как проявится:** разработчик, следуя ТЗ буквально, либо не найдёт файл и
|
||||
пропустит часть AC13 «не сделано» под видом выполненного, либо создаст новый
|
||||
`docs/PERFORMANCE.md` параллельно уже существующему `demo/performance/README.md`,
|
||||
получив два источника документации об одном и том же harness — ровно тот
|
||||
класс дублирования, который процесс просит избегать («параллельных бэклоги/
|
||||
документы» и принцип «одно число/один источник» по духу, здесь — один
|
||||
документ на один факт).
|
||||
|
||||
**Требуется:** исправить ссылку на `demo/performance/README.md` (или явно
|
||||
решить создать новый `docs/PERFORMANCE.md` как отдельный AC — тогда это
|
||||
техническое решение места документации, которое должно быть либо снято, либо
|
||||
явно помечено как «новый файл» в §18/§13.3, а не как правка существующего).
|
||||
|
||||
### M2 — `docs/SCREENSHOTS.md` не существует (Medium, в скоупе)
|
||||
|
||||
**Файл:** `docs/specs/451-render-performance.md`, строка 428 (§13.3).
|
||||
|
||||
**Проблема:** «после изменения `src/**` — canonical screenshots по
|
||||
`docs/SCREENSHOTS.md`, inspection и существующие golden» — файла
|
||||
`docs/SCREENSHOTS.md` в репозитории нет (`find docs -iname "SCREENSHOTS.md"`
|
||||
не находит ничего). Термин «canonical screenshots» в проекте реален (встречается
|
||||
в сообщениях коммитов и предыдущих код-ревью, например
|
||||
`docs/reviews/CODE-REVIEW-126-r1.md:6`), но процесс их пересъёмки и приёмки
|
||||
описан в `PROCESS.md` §8 (`npm run build && node demo/docs/capture.mjs`,
|
||||
`npm run docs:accept -- --reviewed --from=<артефакт>`) и в `scripts/check-docs.mjs`,
|
||||
а не в файле с таким названием.
|
||||
|
||||
**Как проявится:** тот же риск, что в M1 — ссылка на несуществующий файл в
|
||||
плане автотестов не даёт разработчику конкретного места, которое нужно
|
||||
обновить, и не является проверяемым шагом «как есть».
|
||||
|
||||
**Требуется:** заменить ссылку на фактический процесс (`PROCESS.md` §8 /
|
||||
`scripts/check-docs.mjs` / `demo/docs/capture.mjs`) или на реальный файл, если
|
||||
он появится к моменту реализации.
|
||||
|
||||
### M3 — верхняя граница диапазона «наблюдавшихся» замеров в §11.3 не подтверждена ни одним источником (Medium, в скоупе)
|
||||
|
||||
**Файл:** `docs/specs/451-render-performance.md`, строки 329-332 (§11.3).
|
||||
|
||||
**Проблема:** обоснование bootstrap `hardMaxMs` в таблице §11.3 звучит так:
|
||||
«…полученные как безопасный порядок ниже наблюдавшихся **3 432–5 538 мс**
|
||||
полного-render времени…». Число `3 432 мс` действительно есть в issue
|
||||
(«Перетаскивание: суммарно в обновлениях | 3 432 мс»). Число `5 538 мс`
|
||||
не встречается ни в теле issue, ни в одном из шести комментариев
|
||||
(`gh issue view 451 --json body,comments`, поиск `5538`/`5 538` — ноль
|
||||
совпадений), ни где-либо в репозитории вне самого этого ТЗ.
|
||||
|
||||
**Как проявится:** конкретные абсолютные потолки (`500/500/500/750/250 мс`,
|
||||
`maxSingleLongTaskMs<=150`, `longTaskTotalMs<=300`) станут блокирующим CI-гейтом
|
||||
(«fail the runner независимо от timing result», §11.2). Если верхняя граница
|
||||
диапазона, из которого эти потолки якобы выведены «безопасным порядком»,
|
||||
не подкреплена измерением, у порогов нет прослеживаемой доказательной базы —
|
||||
именно то «утверждение, которого нет ни в одном документе и не помечено как
|
||||
предположение», которое ревью обязано ловить отдельно от продуктовых догадок.
|
||||
Не исключено, что число реально измерено автором (в issue упомянуто отдельное
|
||||
инструментирование «снято со страницы»), но в тексте ТЗ это не названо явно как
|
||||
несохранённое сырое измерение, а подано как факт наравне с процитированным
|
||||
`3 432 мс`.
|
||||
|
||||
**Требуется:** либо назвать источник `5 538 мс` (например, «долгие задачи —
|
||||
30 шт/3 298 мс плюс базовая стоимость рендера» с явным выводом формулы), либо
|
||||
убрать конкретное число и явно пометить диапазон как «оценка автора спецификации,
|
||||
поменять свободно» в §18, либо пересчитать от фактически процитированных в issue
|
||||
цифр.
|
||||
|
||||
## Что не проверялось и почему
|
||||
|
||||
- **Продуктовый код** — не прогонялись `npx tsc --noEmit`, `npm test`, `npm run
|
||||
build`, смоки, golden, invariants, performance-профили. Причина: диапазон
|
||||
этого коммита — только `docs/specs/**`, продуктового кода для #451 ещё нет;
|
||||
это гейты этапа `code`, а не `spec`.
|
||||
- **Полнота dependency-списка §6.2** (все ли реальные top-level `hass.*` поля,
|
||||
которые сейчас читает `houseplan-card.ts`, туда попадут) — не проверялась
|
||||
построчным аудитом всех обращений к `this.hass.*`: §18 прямо оставляет точный
|
||||
состав dependency projection «принято предположительно, поменять свободно»
|
||||
и делегирует полноту code review реализации (AC2 доказывается `unit` + `targeted
|
||||
smoke`, а не спек-ревью).
|
||||
- **Достижимость абсолютных бюджетов §11.3 в реальном рантайме** — это будет
|
||||
доказано конкретным CI-прогоном нового профиля при реализации (AC10), спек-ревью
|
||||
проверяет только прослеживаемость происхождения чисел (см. M3), а не их
|
||||
реалистичность.
|
||||
- **Полная матрица редакторских жестов** (Plan/Devices/Decor continuous
|
||||
interactions за пределами трёх «representative editor series») — ТЗ сознательно
|
||||
переносит её в deterministic smoke, а не в performance-профиль (§18), это
|
||||
решается на этапе реализации и проверяется в code review по факту
|
||||
написанных смоков.
|
||||
|
||||
## Материал раунда
|
||||
|
||||
- Ветка: `issue/451-render-performance`
|
||||
- HEAD на момент вывода: `71c3360363991a042fbf3d84a6aeb4ecb772c83f`
|
||||
- Дерево материала: `docs/specs/451-render-performance.md` (единственный
|
||||
затронутый ТЗ-файл), `docs/specs/README.md` (индексная строка)
|
||||
- Команда поиска дерева: `git log --all --format='%H %T' | grep <дерево HEAD>`
|
||||
|
||||
---
|
||||
|
||||
<!-- material-anchors: сгенерировано конвейером (#414) -->
|
||||
|
||||
## Материал раунда
|
||||
|
||||
- Ветка: `issue/451-render-performance`, коммит `71c336036399` — ребейз его осиротит, и это нормально: ниже якоря, которые ребейз не меняет.
|
||||
- Дерево материала: `c395260a0f49bc3004d1cb443cae97c4f9b88001`
|
||||
```
|
||||
git log --all --format='%H %T' | grep c395260a0f49
|
||||
```
|
||||
- ТЗ `docs/specs/451-render-performance.md`, блоб `d9c4dfdc78feeeecfaf1ec41bdf665f22d21944e`
|
||||
```
|
||||
git log --all --find-object=d9c4dfdc78feeeecfaf1ec41bdf665f22d21944e -- docs/specs/451-render-performance.md
|
||||
```
|
||||
Reference in New Issue
Block a user