From 338700bdcb39b2c3729a18810fd87a1f86edcc68 Mon Sep 17 00:00:00 2001 From: "claude[bot]" <209825114+claude[bot]@users.noreply.github.com> Date: Fri, 4 Sep 2026 16:37:26 +0000 Subject: [PATCH] docs: review document for #451 Issue: #451 User-Visible: no --- docs/reviews/SPEC-REVIEW-451-r1.md | 226 +++++++++++++++++++++++++++++ 1 file changed, 226 insertions(+) create mode 100644 docs/reviews/SPEC-REVIEW-451-r1.md diff --git a/docs/reviews/SPEC-REVIEW-451-r1.md b/docs/reviews/SPEC-REVIEW-451-r1.md new file mode 100644 index 00000000..8b33a778 --- /dev/null +++ b/docs/reviews/SPEC-REVIEW-451-r1.md @@ -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>` + +--- + + + +## Материал раунда + +- Ветка: `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 + ```