From e40d3f18d2fdb1bc5f2f9f6dec969c490cddde1f Mon Sep 17 00:00:00 2001 From: "claude[bot]" <209825114+claude[bot]@users.noreply.github.com> Date: Fri, 11 Sep 2026 18:53:32 +0000 Subject: [PATCH] docs: review document for #534 Issue: #534 User-Visible: no --- docs/reviews/SPEC-REVIEW-534-r1.md | 246 +++++++++++++++++++++++++++++ 1 file changed, 246 insertions(+) create mode 100644 docs/reviews/SPEC-REVIEW-534-r1.md diff --git a/docs/reviews/SPEC-REVIEW-534-r1.md b/docs/reviews/SPEC-REVIEW-534-r1.md new file mode 100644 index 00000000..0600602f --- /dev/null +++ b/docs/reviews/SPEC-REVIEW-534-r1.md @@ -0,0 +1,246 @@ +# SPEC-REVIEW-534-r1 + +## Скоуп + +Issue #534 (`bug`, `P1`, полный трек — критерий §5 «нет влияния на +производительность» явно назван нарушенным в самой ТЗ). ТЗ живёт в теле +issue под заголовком `## ТЗ` (решение владельца #517). Issue открыт, метка +`S4-spec-review`, заход r1, бюджет циклов 0/4 не тронут. + +Предмет: линейка v1.74.0 добавила 7 длинных задач P95 на профилях +`large-house` и `plan-snap` — регресс внесён #525 (перевод списка маркеров +устройств на `repeat(devs, (d) => d.id, …)`, чтобы Lit не переиспользовал +узел позиционно и не проигрывал переход на неслучившемся событии). На 200 +маркерах `repeat` строит и обходит карту ключей на каждый рендер, а при +смене пространства ключи не пересекаются вовсе — диф оплачивается напрасно. +Решение ТЗ: обернуть слой маркеров в `keyed(space.id, …)` +(`lit/directives/keyed.js`) вокруг обычного `map()`, оставить проёмы на +`repeat` (там элементов на порядок меньше). + +## Как проверялось + +Прочитаны `docs/SCOPE.md`, `PROCESS.md` (§1–§10, включая §7.1, §2.4, §2.10, +§5, §8), тело issue #534 целиком и единственный комментарий (аналитика +Codex, S2, с бисектом и измерением на пробе). Дополнительно, раз ТЗ +опирается на конкретные строки, числа и поведение существующих механизмов, +прочитан репозиторий на материале ревью (SHA `41ae5e99`, см. блок якорей): + +- `src/houseplan-card.ts:11787` (`repeat(devs, (d) => d.id, …)`, слой + маркеров) и `:12996` (`repeat(items, (o) => o.id, …)`, слой проёмов) — + оба подтверждены чтением, ссылки в ТЗ точны; +- `node_modules/lit/directives/keyed.js` — директива существует в составе + уже установленной зависимости `lit`, ссылка К2 на неё не голословна; +- `scripts/mutation-gate.mjs:7440` (`openings-rendered-without-keys`) и + `:7446` (`device-markers-rendered-without-keys`) — оба существующих + мутанта нацелены ровно на строки, которые правка меняет; find/replace + `device-markers-rendered-without-keys` перестанет применяться после + правки (текст `${repeat(devs, …)}` исчезнет) — ТЗ (AC5) корректно + предвидит необходимость его переанкеровки; +- `demo/smoke_space_switch_transitions.mjs` — прочитан целиком. Подтверждено: + AC1/AC2 действительно можно закрыть существующим смоком без правок — + проверка `noMarkerNodeReusedForAnotherDevice` (строки 124–130) судит по + идентичности узла (`entry.node.isConnected && entry.node.dataset.id !== + entry.id`), а не по побочному признаку, как и требует AC2; +- `src/styles/plan.styles.ts:531–542` — комментарий-ловушка #525 + существует, называет `.device-shell-frame` (`box-shadow`) как причину + для маркеров; ТЗ в разделе «Затронутые файлы» верно указывает, что этот + комментарий нужно дополнить (К3); +- `src/styles/devices.styles.ts:196-217` — проверена природа переходов на + маркере (детали см. в находке ниже); +- `demo/fixtures/large-house.mjs:10-11` — `DEVICE_COUNT = 200`, + `OPENING_COUNT = 100` (на 3 этажа, т.е. ~33 на пространство) — цифры ТЗ о + 200 маркерах точны; слово «единицы» про проёмы (К3) — преувеличение + (реально несколько десятков на этаж), но не влияет ни на один AC; +- `docs/CHANGELOG.md`/`docs/CHANGELOG.ru.md` — записи #524/#525 в разделе + «Unreleased»/«Не выпущено» существуют, формат черновика (AC7) совпадает + со стилем файла; +- `test/device-marker-polish-contract.test.mjs`, `test/space-order.test.mjs`, + `test/device-face.test.mjs` — ни один не проверяет позиционную + стабильность маркера при изменении состава списка внутри одного + пространства (относится к находке ниже). + +Гейты не гонялись: на этапе ревью ТЗ продуктовый код не менялся, дешёвые +гейты (`typecheck`/`test`/`build`) к этому этапу неприменимы (§2.4, §8). + +## Продуктовая рамка + +Сценарий и «что человек увидит» сформулированы одной фразой без терминов +реализации, как требует §7.1: «каждое переключение дороже примерно на +80 мс главного потока… карточка перестаёт отвечать на ввод чуть дольше, +чем должна» → «переключение возвращается к цене, которая была до #525». +Персона — домочадцы/гости на большом плане, `docs/SCOPE.md` называет View +mode продуктом для этих персон; регресс блокирует именно это. Полный трек +обоснован явно названным нарушенным критерием §5 («нет влияния на +производительность»), а не молчаливым выбором — соответствует требованию +2026-08-27. i18n/миграция/бэкенд и touch закрыты явным «ничего» / +«тот же путь» — ТЗ учло Low-находку из SPEC-REVIEW-525-r1 (там эти пункты +пришлось поднимать отдельно), здесь они на месте с самого начала. + +## Разбор AC + +- **AC1** — «остаётся зелёным без правок»: подтверждено чтением, существующий + смок уже проверяет то, что требуется, и правка К2/К3 его не задевает + структурно (тест не завязан на `repeat` изнутри, только на видимое + поведение DOM/CSS). +- **AC2** — свидетель по идентичности узла уже существует (см. «Как + проверялось»), формулировка ТЗ точно описывает то, что тест делает. +- **AC3** — численный ориентир (910 мс) подтверждён измерением автора на + `main` + патч (903,7 мс), не выдуман. +- **AC4** — внешний гейт (Full Performance), критерий однозначен (девять + профилей, включая оба проблемных). +- **AC5** — корректно предвидит необходимость правки мутанта + (подтверждено выше — старый find/replace перестанет матчиться). +- **AC6/AC7** — golden и ревью кода, стандартные, без двусмысленности. + +Двусмысленности, которая заставила бы гадать «что считается прохождением», +в текстах AC не нашёл. + +## Находки + +### Medium — ключевое техническое допущение К2/«Принято предположительно» №2 неверно, и это открывает непроверенный регресс той же природы, что чинит вся задача + +**Утверждение ТЗ:** «Внутри одного пространства позиционное переиспользование +маркеров безопасно: после #524 на маркере не осталось переходов, которые +могла бы запустить смена значений.» + +**Проверено чтением и опровергнуто:** `src/styles/devices.styles.ts:213` +несёт `.device-shell-frame { … transition: border-color .15s, opacity .2s; }` +— оба перехода живы. #524 убрал только переход `box-shadow` (см. комментарий +на той же строке: «no box-shadow here: cqw-sized, restarts on every +container resize (#524)»), а не «переходы» вообще — допущение спутало +конкретный фикс с общим случаем. `--device-shell-stroke` (цвет рамки) +переопределяется по состоянию устройства (`devices.styles.ts:296,312,322,421` +— alert-цвета) — то есть у двух РАЗНЫХ устройств в одном списке этот цвет +типично отличается. + +**Почему это ломает контракт К2.** К2 предписывает конкретную реализацию: +`keyed(space.id, …)` вокруг **обычного `map()`** для слоя маркеров — то есть +внутри одного пространства элементы списка теряют пер-элементный ключ, +который сейчас даёт `repeat(devs, (d) => d.id, …)`. Если состав `devs` +меняется без смены `space.id` — а он меняется: `showGhosts` (`houseplan- +card.ts:11307`, `this._mode === 'devices' && this._showAll`) переключает +видимость части устройств в редакторе устройств, а само появление нового +устройства в пространстве от лайв-синка конфигурации (J6, «new-device +flag») тоже меняет состав `devs` без переключения пространства — Lit +переиспользует DOM-узлы **по позиции**, не по устройству. Маркер, стоявший +на позиции N, может получить данные другого устройства с другим +`--device-shell-stroke`/`opacity`, и уже живой `transition: border-color +.15s, opacity .2s` эти узла честно проиграет — ровно тот класс дефекта +(«анимация события, которого не было»), который и #525, и эта самая задача +называют главным риском (Риск 1 в ТЗ), только для другого триггера: не +переключение пространства, а изменение состава списка внутри пространства. + +**Почему это не поймано ни одним AC.** `demo/smoke_space_switch_transitions.mjs` +(AC1/AC2) тестирует только переключение между двумя пространствами; ни один +существующий тест (`test/device-marker-polish-contract.test.mjs`, +`test/space-order.test.mjs`, `test/device-face.test.mjs`) не проверяет +переход, вызванный изменением состава списка маркеров внутри одного +пространства. Риски ТЗ (1–3) эту ситуацию не называют. + +**Почему это в скоупе.** К2 — это ровно тот код, который правит задача; +регрессия, если она проявится, будет внесена этим же коммитом, а не +существует независимо от него сегодня (сейчас защиту даёт per-item ключ +`repeat`, который К2 явно убирает). + +**Что нужно для DoR (любое из трёх, решает автор):** +1. исправить допущение и сохранить пер-элементную защиту внутри + пространства — например, `keyed(space.id, () => repeat(devs, (d) => + d.id, …))`: смена `space.id` по-прежнему выбрасывает поддерево целиком + без дорогого сравнения с 200 чужими ключами (устраняет весь измеренный + регресс), а `repeat` внутри защищает от переупорядочивания в пределах + одного пространства, как и сегодня; либо +2. показать конкретным измерением/рассуждением, что состав `devs` в + реальных сценариях (не только в фикстуре бенчмарка) не меняется без + смены `space.id` так, чтобы порядок сдвигался — и явно принять остаточный + риск в разделе «Риски», а не в «Принятые предположения» как решённый + факт; либо +3. добавить AC и свидетеля на сценарий «состав списка маркеров меняется + внутри пространства» по образцу уже существующего свидетеля #525/#528. + +**Решение ревьюера:** блокирует переход в `S5-ready` без правки одним из +трёх способов выше — Medium в скоупе задачи, чинится в этом же issue +(§2.4, §2.7 #202), отдельный issue не заводится. + +### Low — «их единицы» (К3) неточно описывает число проёмов + +К3 обосновывает, почему проёмы остаются на `repeat`: «их единицы, диф ничего +не стоит». По фикстуре `large-house` (`DEVICE_COUNT=200`, +`OPENING_COUNT=100` на 3 этажа) на пространство приходится порядка +30 проёмов — не «единицы», хотя и на порядок меньше 200 маркеров, так что +общий вывод (репит на проёмах дешевле) не меняется и ни один AC от этого не +зависит. + +**Решение ревьюера:** не блокирует, снимается этой записью; можно поправить +формулировку заодно с остальной правкой К2, отдельного цикла не открываю. + +## Что проверено и корректно + +- Обязательные продуктовые разделы §7.1 (сценарий, что человек увидит до и + после) присутствуют и однозначны. +- Полный трек обоснован явно названным нарушенным критерием §5. +- i18n, миграция/бэкенд, touch — явные «нет»/«тот же путь», а не молчание + (что в #525 пришлось поднимать Low-находкой — здесь уже учтено). +- AC1–AC7 пронумерованы, у каждого указан способ доказательства; численные + пороги AC3 (910 мс) и AC4 (девять профилей) проверяемы и не выдуманы — + число AC3 сверено с измерением автора на пробе. +- AC5 корректно предвидит поломку существующего мутанта новой формой кода + и требует его переанкеровки — не тихое исчезновение защиты. +- Технические ссылки (номера строк, наличие директивы `keyed` в + зависимостях, наличие мутантов, содержимое свидетеля) подтверждены + чтением репозитория, ни одна не оказалась пустой. +- Откат — один revert, без миграции данных, корректно. +- Риски 1 и 3 предметны и привязаны к AC/гейтам, которые их ловят. +- Release-артефакты (changelog RU+EN, `User-Visible: yes`) названы верно. + +## Чего не проверял + +- Не запускал никакого кода и ни одного гейта — на этапе ревью ТЗ + продуктовый код не менялся, дешёвые гейты (`typecheck`/`test`/`build`) + неприменимы к этой стадии (§2.4, §8). +- Не проверял golden-эталоны и реальные тайминги на Chromium/раннере — + появятся только с реализацией; численные ориентиры ТЗ (903,7 мс, 910 мс) + доверены измерению автора (внутренне согласованы с бисектом в + комментарии) и будут окончательно подтверждены Full Performance по AC4. +- Не проверял частоту реальных сценариев `showGhosts`/лайв-добавления + устройства «в проде» количественно — риск в находке выше обоснован кодом + (условие фильтра, наличие живых CSS-переходов, наличие state-зависимого + цвета рамки), а не измерением частоты; для итогового решения (пункт 1–3 + находки) этого достаточно, эмпирическая частота не меняет того, что + сценарий кодом не исключён и не покрыт тестом. + +## Вердикт + +Жёлтый. Один Medium в скоупе задачи (допущение К2 о безопасности +внутрипространственного позиционного переиспользования маркеров неверно и +открывает непроверенный регресс той же природы, которую чинит вся задача); +High нет. Возврат автору на правку одним из трёх названных способов; +одна Low-находка о формулировке снята этой записью. + +--- + + + +## Материал раунда + +- Ветка: `dev`, коммит `41ae5e99868db27e3bfde9a40426c0a636c374bd`. +- Дерево материала: `e87f2fb0f5a99184cd9795a5e59155b96b3ac2f6` + ``` + git log --all --format='%H %T' | grep e87f2fb0f5a9 + ``` +- Тело issue (нормализовано `gh issue view 534 --json body -q '.body'`): + `sha256:98e12a0f7bec0c050802b1caee9c139db3bbc7b7c1169744fb1ccd567e35babd` +- Вердикт: `yellow` · High 0 · Medium 1 (в скоупе) + +--- + + + +## Материал раунда + +- Ветка: `dev`, коммит `41ae5e99868d` — ребейз его осиротит, и это нормально: ниже якоря, которые ребейз не меняет. +- Дерево материала: `e87f2fb0f5a99184cd9795a5e59155b96b3ac2f6` + ``` + git log --all --format='%H %T' | grep e87f2fb0f5a9 + ``` +- Тело issue: `3ba57688eafe3310c9e3d0157758ea519c3b89d980958e504d2c2b54b3a43cb3` +- Вердикт конвейера: `yellow` · High 0