diff --git a/docs/reviews/SPEC-REVIEW-509-r1.md b/docs/reviews/SPEC-REVIEW-509-r1.md new file mode 100644 index 00000000..42f8d407 --- /dev/null +++ b/docs/reviews/SPEC-REVIEW-509-r1.md @@ -0,0 +1,312 @@ +# SPEC-REVIEW-509-r1 + +Issue: [#509](https://github.com/Matysh/houseplan-card/issues/509) — «Сводная +панель: при первом показе подвисание и «Source unavailable» вместо значений +до загрузки metrics-чанка». +Этап: spec (PROCESS.md §2.4). Заход: r1. Блокирующих циклов израсходовано 0 из 4. +Материал: тело issue #509, раздел `## ТЗ` (owner-решение #517, 2026-09-10 — +ТЗ полного трека тоже живёт в теле issue, файл `docs/specs/509-*.md` не +создаётся), на момент перехода `S3-spec → S4-spec-review` +(`unlabeled: S3-spec` / `labeled: S4-spec-review` в +`2026-09-10T07:56:11Z`, тот же момент несёт единственную правку тела после +S2-комментария). Рабочая копия `dev` на `d204bf5d` — код по этой задаче ещё +не существует, ветки `issue/509-*` на origin нет; ревью целиком по тексту. + +## 1. Скоуп ревью + +Трек — полный: метки issue — `bug`, `S4-spec-review`, `small` отсутствует. +Явного предложения «трек: small — нет, критерий не пройден» я в S2-комментарии +не нашёл дословно, но по существу критерий §5 «нет влияния на +производительность» действительно не пройден — вся задача построена вокруг +измеренных миллисекунд и мутационных гардов на перф-путь, что и оправдывает +полный трек фактически (см. Low-1 ниже: это гигиена формы, не решение). + +Ревью охватывает: +- тело issue #509 целиком: секцию «Симптом», секцию «Что видно в коде (для + S2)», комментарий S2-аналитики (владелец/автор, `2026-09-10T07:49:33Z`) и + секцию `## ТЗ`; +- соответствие `docs/SCOPE.md` (персона, поверхность, job), `PROCESS.md` + §7.1/§7.2/§5, `AGENTS.md`; +- фактические утверждения ТЗ о текущем коде — сверены чтением, не приняты на + слово; +- реализуемость каждого AC заявленным способом доказательства. + +Это первый заход — раздела «Унаследовано из r0» не требуется, разбор полный. + +## 2. Как проверялось + +- Прочитаны `docs/SCOPE.md`, `AGENTS.md`, `PROCESS.md` целиком (включая §2.2, + §2.4, §2.10, §4, §5, §7.1, §7.2, §8). +- Прочитано тело issue #509 и его единственный комментарий через + `gh issue view 509 --json body,comments,labels,state`. +- Прогон таймлайна issue (`gh api .../issues/509/timeline`) и + `userContentEdits` (GraphQL) — подтверждено: `S1-new → S2-analysis` в + `07:44:05`, комментарий-аналитика в `07:49:33`, переход в `S3-spec` уже в + `07:49:35` (через 2 секунды после своего же комментария), переход в + `S4-spec-review` в `07:56:11` — единственная правка тела issue приходится на + этот же момент (второй `userContentEdits`-узел). Значит между вопросом + владельцу («что видно в первые ~200 мс», вариант A по умолчанию) и записью + ТЗ отдельного ответа владельца физически не было — редакция ТЗ добавлена в + теле той же ~7-минутной сессией, которая задала вопрос. Это не идёт вразрез + с §2.2 (аналитик не обязан ждать подтверждения и явно объявил «если ответа + не будет — реализую A»), но заголовок ТЗ «решение владельца по плейсхолдеру + — в комментарии S2» точнее было бы читать как «дефолт автора с явным + объявлением», а не как отдельное решение владельца — содержательно на + вердикт не влияет (вариант A разумен, задокументирован, проверяем AC1/AC6), + поэтому не заводится отдельной находкой, но фиксирую здесь для + прослеживаемости следующего раунда. +- Каждое фактическое утверждение ТЗ и S2-комментария о текущем коде + перепроверено чтением на `dev` `d204bf5d`, а не принято на слово: + - `LoadedSummaryPanelRuntime.value()` (`src/summary-panel-runtime-loaded.ts:715-720`): + подтверждено — `resolved ?? this.t('summary.unavailable')`, и `resolved` + равно `null` как при отсутствующем `module` (`metricsModule` ещё не + загружен), так и при легитимно недоступном источнике — состояния + неразличимы в возвращаемом значении, ровно как описывает симптом. + - `metrics()` (`:687-712`): подтверждено — вызывается синхронно, мемо по + `cfgEpoch`/`layoutRev`/`registryRev` (device) и `cfgEpoch` (area), при + первом вычислении после загрузки модуля выполняется целиком в кадре. + - `totalCleanFloorAreaM2` (`src/summary-panel-metrics.ts:59-84`): + подтверждено — цикл по `space.rooms` вызывает `innerContourForRoom` без + `sharedRoomWallGeometry`/`sharedMultiWallNodes` на каждой комнате; сама + `innerContourForRoom` (`src/wall-thickness.ts:2750-2779`) в этом случае + заново вызывает `wallBodiesGeometry`/`multiWallNodesForGeometry` — точно + описанная причина 7,5-кратной разницы. + - Утверждение «карточка уже передаёт общую геометрию через `_innerContour`» + подтверждено: `_innerRoomContour` (`src/houseplan-card.ts:9219-9240`) + передаёт `roomWalls`/`multiWallNodes` из `_wallUnionGeometry()` (общий, + закешированный по эпохе объект) — карточка действительно не пересчитывает + геометрию стен на каждой комнате, а сводная панель — пересчитывает. + Не выдумка, реальный прецедент в том же файле. + - `SummaryPanelPresentation`/анимация 190 мс (issue #505) — подтверждено: + класс существует и подключён (`src/summary-panel-runtime-loaded.ts:23,93`), + #505 закрыт и смёржен (`gh issue view 505` → `state: CLOSED`, статусных + меток нет) — AC5 действительно проверяет уже существующий, но + (по симптому) заблокированный синхронным расчётом путь, а не выдуманную + фичу. + - `summary.unavailable` — реальный существующий ключ + (`src/summary-panel-i18n.ts:16,84,152,220`, en/ru/de/fr) — заявление + «новых i18n-ключей нет» корректно, скелет действительно чисто визуальный. + - Персона «житель дома» — используемый в проекте перевод `Household + members` из `docs/SCOPE.md` (совпадает с формулировкой в + `SPEC-REVIEW-212-r1.md`, `SPEC-REVIEW-374-r1.md`, `SPEC-REVIEW-505-r2.md`) + — не изобретённый термин. +- Проверена техническая реализуемость AC3 («один вызов… через инъекцию + счётчика»): `wallBodiesGeometry`/`multiWallNodesForGeometry` — экспортируемые + чистые функции (`src/wall-thickness.ts:2434,3662`), `innerContourForRoom` + уже принимает их как необязательные параметры — заявленный план (передать + общую геометрию один раз на пространство) технически реализуем без нового + паттерна, здесь это чистая инженерная перестановка вызовов, а не гипотеза. +- Проверено соответствие обязательных разделов §7.1 (таблица ниже). +- Гейты (`typecheck`/`test`/`build`) не прогонялись — кода в материале нет + (ветки `issue/509-*` не существует), на этапе `spec` эти гейты неприменимы + к пустому диффу; смотри также §5 «Чего не проверял». + +### Проверка обязательных разделов (§7.1) + +| Требование | Где в теле issue | +|---|---| +| Сценарий | `## ТЗ` → «Сценарий»: персона «житель дома», поверхность View + kiosk, момент — первое открытие/включение панели | +| Что человек увидит до/после | «Что человек увидит до и после» — одной фразой, без терминов реализации | +| Проблема | Секция «Симптом» + «Что видно в коде» (тело issue, до `## ТЗ`) | +| Скоуп и не-скоуп | «Контракт» (объём правки) + «Не-скоуп» | +| Контракт поведения | «Контракт», пп. 1–5 | +| UX | Явного отдельного раздела нет, но по существу покрыт («Контракт» пп.1,2,5 — нет новых элементов управления, только визуальные состояния уже существующих строк) | +| Модель данных и миграция | Преамбула ТЗ: «Модель данных и схема конфигурации не меняются» | +| i18n | Преамбула: «Новых i18n-ключей нет» — подтверждено чтением, см. выше | +| AC1…ACn с доказательством | «AC», 7 пунктов, у каждого назван unit/smoke | +| План автотестов | Не вынесен отдельным разделом, но покрыт AC + таблицей «Чем краснеет» (тот же уровень, что и прецедент #506, признанный там достаточным) | +| Риски | «Риски», 2 пункта | +| Откат | «Откат» | +| Release-артефакты | «Release-артефакты» | + +Все обязательные разделы присутствуют по существу; отсутствие отдельного +финального блока «Принятые технические предположения» разобрано как +Medium-1 ниже. + +## 3. Находки + +### Medium-1 (в скоупе) — отсутствует раздел «Принятые технические предположения» (PROCESS.md §7.1) + +ТЗ делает по ходу текста несколько односторонних технических решений, которые +пользователь не наблюдает, но которые прямо влияют на то, что именно будет +реализовано, и ни одно не вынесено в явный финальный блок «принято +предположительно, поменять свободно» — том самом формате, который PROCESS.md +§7.1 требует именно для того, чтобы ревьюер мог их оспорить, а не читать как +уже окончательное решение. Конкретно: + +1. **Механизм переноса расчёта «вне кадра» назван как альтернатива, не + решение.** П.4 контракта: «расчёт запускается после кадра + (микрозадача/`requestIdleCallback` с фолбэком на таймер)». Это не один + механизм с уточнением, а два содержательно разных: микрозадача выполняется + до следующей отрисовки (гарантированно быстро), `requestIdleCallback` + ждёт простоя главного потока и при устойчиво занятом потоке может + откладываться на неопределённое время — то есть ровно то поведение, + которое приводило бы к варианту, где готовое число появляется через + секунды после показа скелета на медленной машине. Ни один AC не + ограничивает, сколько именно может пройти между показом скелета и + заменой его значением (см. также Medium-2) — то есть от выбора между + этими двумя механизмами наблюдаемо зависит поведение, а формулировка ТЗ + не решает, какой из них, а перечисляет оба через слэш. +2. Способ технической инъекции счётчика вызовов для AC3 (как именно тест + заменит/оборачивает `wallBodiesGeometry`/`multiWallNodesForGeometry`, + учитывая, что они — обычные экспортируемые функции ES-модуля, а не + параметры) не назван ни как решённый вопрос, ни как «на усмотрение + реализации». +3. Форма хранения «прежнего значения» при инвалидации мемо (п.4 контракта: + «показывает прежнее значение до прихода нового») — остаётся ли это тем же + полем `deviceMemo`/`areaMemo`, что и сегодня, или требует нового состояния + («последнее успешно вычисленное» отдельно от «текущего валидного») — не + названо явно. + +Прецедент того же приёма находки, для той же подсистемы, у того же автора: +`SPEC-REVIEW-505-r1.md`, Medium «отсутствует обязательный раздел «Принятые +технические предположения»», со ссылкой на `docs/specs/493-...md` как +образец, где формат уже применялся. В #506 то же требование было закрыто +записью Low, а не Medium, потому что там содержание уже было распределено +по тексту («предпочтительный дизайн… точные имена и границы модуля +допускается уточнить») — здесь же пункт 1 выше не просто не назван свободным, +а прямо оставляет открытым выбор между двумя семантически разными +механизмами, поэтому не дотягивает даже до уровня #506 и остаётся Medium. + +**Как чинится:** добавить в конец ТЗ раздел «Принятые технические +предположения» и явно решить (или явно пометить «на усмотрение реализации, +не меняет ни один AC») три пункта выше — в первую очередь пункт 1, так как +он пересекается с Medium-2. + +### Medium-2 (в скоупе) — контракт п.4 (прежнее значение при инвалидации мемо, без скелета) не доказан ни одним AC + +П.4 контракта содержит отдельное поведенческое обязательство: «при +инвалидации панель показывает **прежнее** значение до прихода нового (не +скелет — скелет только когда значения ещё не было)». Это прямая защита +именно от того паттерна мигания, из-за которого заведена задача (значение +подменяется на «недоступно»/пусто и обратно) — то есть по существу это +самый близкий к первоначальному симптому кусок контракта, а не второстепенная +деталь. + +Ни один из AC1–AC7 его не проверяет: +- AC1/AC2 — про состояние «`metricsModule` ещё не загружен» vs «источник + недоступен», не про инвалидацию уже загруженного модуля; +- AC3 — про идентичность площади и число вызовов геометрии, не про то, что + показывается в UI между инвалидацией и пересчётом; +- AC4 — про длительность первого показа, не про повторные показы после смены + конфигурации; +- AC5/AC6 — про анимацию появления/исчезновения панели и уважение + `prefers-reduced-motion`, не про эту конкретную защиту; +- AC7 — про трейлеры/i18n/golden. + +**Сценарий проявления:** реализация меняет `cfgEpoch` (правка конфигурации, +редактор) и на время пересчёта (после п.4, теперь асинхронного) ошибочно +показывает скелет вместо уже известного старого числа — ни один автотест +здесь не покраснеет, регрессия обнаружится только ручным глазом на +следующей задаче, ровно как было с #234 (не этот баг конкретно, но тот же +класс: незамеченная регрессия смежного поведения, которую в этом процессе +призвана ловить именно связка «AC + автотест»). + +**Как чинится:** добавить AC (или явно расширить формулировку одного из +существующих, например AC1), проверяющий: `cfgEpoch` меняется → +`deviceMemo`/`areaMemo` уже когда-то были посчитаны → `value()`/рендер строки +показывает старое число, а не скелет, пока не готово новое. Разумный +свидетель — unit на `metrics()`/`value()` с последовательностью +{вычислено → `cfgEpoch++` → запрошено снова до завершения асинхронного +пересчёта}. + +### Low-1 (не блокирует, для протокола) — критерий отказа от лёгкого трека не назван явно + +PROCESS.md §2.2 п.8 требует при выборе полного трека называть конкретный +критерий §5, который задача не проходит («обычный трек» без названного +критерия обоснованием не является). S2-комментарий переходит в `S3-spec` +без такой явной фразы (в отличие от прецедента `SPEC-REVIEW-506-r1`, где +аналитик прямо процитировал «критерий small «нет влияния на +производительность» заведомо не выполнен»). По существу критерий +действительно не пройден — вся задача измеряет и бюджетирует +производительность (AC3–AC5, таблица «Чем краснеет», мутанты на перф-путь), +и никакого другого вывода из содержания ТЗ сделать нельзя, поэтому это +дефект формы аналитического комментария, а не двусмысленность самого ТЗ. +Не блокирует зелёный вердикт по этой задаче; отдельного действия не требует +(снимается записью). Упоминается ради единообразия с прецедентами #506/#505. + +## 4. Что проверено и корректно + +- **Формат этапа:** полный трек, ТЗ — в теле issue, что верно для задач, + открытых после решения #517 (2026-09-10); ссылки issue↔ТЗ на месте + (единственный документ и есть issue). +- **Персона и SCOPE.md:** «житель дома» (Household members) на View/kiosk — + укладывается в J1 («live spatial overview») напрямую; никакого конфликта с + `docs/SCOPE.md` не найдено, задача не расширяет функциональность, а + устраняет деградацию отзывчивости и вводящий в заблуждение текст ошибки. +- **Технический диагноз подтверждён чтением, не переписан со слов автора**: + все пять цитируемых мест кода (`value()`, `metrics()`, + `totalCleanFloorAreaM2`, `_innerRoomContour`, `SummaryPanelPresentation`) + проверены построчно на `dev` `d204bf5d` и совпадают с описанием ТЗ и + S2-комментария; числа (11 045 → 1 488 мс, 306.3 м²) взяты из измерения + того же комментария, а не изобретены заново в ТЗ. +- **AC3 технически реализуем** заявленным способом (общие экспортируемые + функции, `innerContourForRoom` уже принимает shared-параметры) — не + гипотеза без опоры на код. +- **AC5 не выдумывает новую фичу**: #505 (анимация 190 мс) уже смёржена и + закрыта, AC5 проверяет, что расчистка главного потока (пп. 3–4) даёт этой + уже существующей анимации реально отрисоваться, а не изобретает + анимацию заново. +- **i18n:** новых ключей действительно нет, `summary.unavailable` — реальный + существующий ключ на 4 языках; заявление ТЗ точное. +- **Модель данных / миграция / touch:** корректно помечены «не меняются» / + «не относится» — правка не касается конфигурации, хранения или touch-целей. +- **Риски** называют реальный источник флейка (порог AC4 меряется на CI с + запасом, не сравнивается между машинами) — соответствует уже принятому в + проекте паттерну perf-смоков (`PROCESS.md` §8). +- **Откат** тривиален и точен: правка локальна для `summary-panel-*` плюс + тесты, данных и миграций нет. +- Продуктовых вопросов владельцу, кроме уже заданного и закрытого дефолтом + (плейсхолдер = вариант A), в тексте не осталось; ни одна догадка о + видимом поведении не подана как факт без пометки (единственная зона + неопределённости — п.4/AC-разрыв — разобрана явно как находка, а не как + скрытая догадка). + +## 5. Чего не проверял + +- Не прогонялись `typecheck`/`test`/`build`/`process-gate` — кода в + материале нет, ветки `issue/509-*` не существует; это гейты этапа `code`, + не `spec`. +- Не запускались браузерные смоки/`golden`/`performance_smoke` — на этапе + ТЗ реализации не существует; они станут обязанностью код-ревью, когда + появятся файлы `demo/smoke_*`, названные в AC1/AC4/AC5/AC6. +- Не проверялась синтаксическая корректность будущего кода + (`requestIdleCallback`-план, структура нового состояния «прежнее + значение») — его ещё не существует; оценивалась только согласованность + плана с уже существующим кодом, на котором план основан. +- Не оценивалось качество будущих мутационных гардов из таблицы «Чем + краснеет» сверх того, что они названы и привязаны к конкретным функциям — + реализация появится в коде. +- Не проверялся сам механизм подсчёта хеша тела issue в блоке якорей + документа (`scripts/review-doc-guard.mjs`/`process.yml`, #517) — это + инфраструктура конвейера, а не предмет ревью ТЗ конкретной задачи. + +## 6. Вердикт + +High: 0. Medium: 2, обе в скоупе задачи, чинятся правкой того же текста без +нового технического анализа (обе опираются на уже написанные автором факты +и код, который я перепроверил). Low: 1, не блокирует, снимается записью. + +Обязательные разделы присутствуют по существу; технические утверждения о +текущем коде подтверждены чтением, а не приняты на слово; единственный +реальный пробел — контракт описывает защиту от повторного мигания при +инвалидации мемо (ровно то, из-за чего заведена задача), но не проверяет её +ни одним AC, и один из технических путей реализации (микрозадача vs +`requestIdleCallback`) оставлен как «или-или» без решения и без пометки +«на усмотрение реализации». + +Вердикт: жёлтый · заход r1 · блокирующих циклов 1/4 · High: 0 · Medium: 2 → в задаче + +--- + + + +## Материал раунда + +- Ветка: `dev`, коммит `d204bf5d7737` — ребейз его осиротит, и это нормально: ниже якоря, которые ребейз не меняет. +- Дерево материала: `cf36bd3b2a3141c427a40f9e10aac607faf02a1f` + ``` + git log --all --format='%H %T' | grep cf36bd3b2a31 + ``` +- Тело issue: `8d514ac196bbc453d5b25b352421dc27f9925b8668a89e44c00971f304f2c3e4` +- Вердикт конвейера: `yellow` · High 0