From 1fb84755c22ba1fe990a96936399b579f0835a20 Mon Sep 17 00:00:00 2001 From: "claude[bot]" <209825114+claude[bot]@users.noreply.github.com> Date: Fri, 11 Sep 2026 19:07:13 +0000 Subject: [PATCH] docs: review document for #534 Issue: #534 User-Visible: no --- docs/reviews/SPEC-REVIEW-534-r2.md | 259 +++++++++++++++++++++++++++++ 1 file changed, 259 insertions(+) create mode 100644 docs/reviews/SPEC-REVIEW-534-r2.md diff --git a/docs/reviews/SPEC-REVIEW-534-r2.md b/docs/reviews/SPEC-REVIEW-534-r2.md new file mode 100644 index 00000000..77da4c0b --- /dev/null +++ b/docs/reviews/SPEC-REVIEW-534-r2.md @@ -0,0 +1,259 @@ +# SPEC-REVIEW-534-r2 + +## Скоуп + +Issue #534, этап `spec` (ревью ТЗ, PROCESS.md §2.4), заход r2, до этого +раунда израсходован 1/4 блокирующих цикла (r1 — жёлтый, Medium в скоупе). +ТЗ живёт в теле issue (решение владельца #517). + +Материал этапа spec — тело issue, не код. Между r1 и r2 продуктовый код не +менялся: `git diff 41ae5e99..HEAD -- . ':!docs/reviews'` пуст, рабочая +копия на `e40d3f18` содержит только коммит с документом +`SPEC-REVIEW-534-r1.md`. Единственная дельта раунда — правка тела issue +владельцем (комментарий-вердикт r1 и последующий комментарий Matysh от +2026-09-11T18:58:53Z). Хэш нормализованного тела изменился +(`sha256:18961b89…` в r2 против `sha256:98e12a0f…`, зафиксированного в +материале r1) — дельта подтверждена, а не предполагается. + +## Как проверялось + +1. Прочитан вердикт r1 (комментарий `IC_…`, 2026-09-11T18:53:19Z) и + документ `docs/reviews/SPEC-REVIEW-534-r1.md` на коммите `41ae5e99` + (дерево материала `e87f2fb0f5a9…`, зафиксировано в блоке «Материал + раунда» того документа). +2. Дельта тела issue объявлена (см. «Скоуп») и разобрана построчно: + старые формулировки К2/К3/«Принятые предположения»/AC5/«Риски» + реконструированы по цитатам r1-документа и сопоставлены с текущим + текстом (`gh issue view 534 --json body -q '.body'`, сохранено в + `/tmp/current_body.md`). +3. Технические ссылки новой редакции проверены чтением репозитория на + материале ревью (SHA `e40d3f18`, код не менялся с r1): + - `src/houseplan-card.ts:11787` (`repeat(devs, (d) => d.id, …)`) и + `:12996` (`repeat(items, (o) => o.id, …)`) — оба списка сегодня без + `keyed`, ссылки К2 «оба списка… `keyed(…, repeat(…))`» описывают + код, которого ещё нет, корректно как целевое состояние; + - `scripts/mutation-gate.mjs:7434-7456` — оба существующих мутанта + (`openings-rendered-without-keys`, `device-markers-rendered-without-keys`) + сделаны точным `find`/`replace` по буквальной строке с `repeat(…)`; + `applyPatches` (`mutation-gate.mjs:8910-8916`) бросает исключение, + если якорь не найден ровно один раз — промах анкера не тихий, гейт + красится сборкой мутанта, а не пропускает проверку молча; + - `src/styles/plan.styles.ts:531-549` — комментарий-ловушка #525 уже + сегодня объясняет `.op-leaf { transition: transform 0.6s ease }` и + `.op-arc { transition: stroke-dashoffset 0.6s ease }` как причину, + по которой список проёмов **уже с #525** рендерится через `repeat`, + наравне с маркерами устройств — тем же абзацем, который К2/К3 этого + раунда обязывают дополнить; + - `src/houseplan-card.ts:8226-8260` (`_openingsR`) — при + `!resolution.resolved` в режиме `plan` добавляется orphan-запись + (`:8242-8243`), а вне `plan` эта запись выпадает из списка + (`:8244`, `flatMap` возвращает `[]`) — состав `items` меняется от + переключения режима Plan/View **без смены пространства**, длина + массива и позиции последующих элементов сдвигаются; + - `src/houseplan-card.ts:12987-12996` (`_renderOpenings`) — рендерит + `items` из `_openingsR` тем же `repeat`, что и обсуждается в К2/К3. + +Гейты не гонялись: этап spec, продуктовый код не менялся (§2.4, §8) — +как и в r1. + +## Закрытие раунда r1 + +| находка r1 | чем закрыта | где это видно | +|---|---|---| +| Medium — допущение К2 «внутри пространства позиционное переиспользование маркеров безопасно» опровергнуто чтением (`devices.styles.ts:213`, живые переходы) | Формулировка К2 переписана: оба списка теперь `keyed(space.id, repeat(items, id, …))` — внутренний `repeat` **сохраняется**, а не убирается. Добавлен явный контракт К3 «почему пер-элементный ключ обязан остаться» с тем же техническим обоснованием, которое привёл ревьюер (переходы `.device-shell-frame`, `--device-shell-stroke` по состоянию). Допущение №2 переписано из утверждения о безопасности в утверждение о цене (диф `repeat` идёт по быстрому пути, измерено 905,9 vs 903,7 мс) — корректность больше не зависит от истинности этого допущения, потому что `repeat` защищает по ключу независимо от того, совпадают ли ключи по порядку. Добавлен новый AC2а — свидетель именно на сценарий «состав списка маркеров меняется без смены пространства». Риск 2 переписан под новый сценарий и ссылается на К3/AC2а/мутант. | Тело issue: К2 (строка 48), К3 (50), AC2а (60), Риск 2 (90), допущение №2 (96). Выполнено способом 1 из трёх предложенных ревьюером r1 (сохранить пер-элементную защиту), усилено способами 2 (допущение переформулировано честно) и 3 (добавлен AC-свидетель) — все три сразу, не одно на выбор. | +| Low — «проёмов единицы» (К3 в старой нумерации) неточно | Число исправлено на «порядка тридцати на этаж» | Допущение №3 (строка 97): «Проёмов на `large-house` порядка тридцати на этаж, не «единицы»» | + +Обе находки r1 закрыты текстом ТЗ, а не заявлением в комментарии — проверено построчным чтением текущего тела issue, а не принято на слово. + +## Унаследовано из r1 + +Без повторной проверки принято из `docs/reviews/SPEC-REVIEW-534-r1.md` +(документ и SHA `41ae5e99868db27e3bfde9a40426c0a636c374bd`, дерево +материала `e87f2fb0f5a9…`), потому что дельта r2 этих участков текста и +кода не касается: + +- Продуктовая рамка §7.1 (сценарий, «что человек увидит» до/после, + выбор полного трека по явно названному критерию §5, i18n/миграция/ + бэкенд/touch как явные «нет» вместо молчания) — текст сценария в r2 не + менялся; +- AC1 (существующий смок остаётся зелёным без правок) — смок и его + проверки не менялись, вывод r1 держится; +- AC3/AC4 (численный ориентир 910 мс и полный перф-гейт из девяти + профилей) — числа и формулировка не менялись между раундами; +- AC6 (golden) и базовая часть AC7 (записи в оба changelog, + `User-Visible: yes`) — не менялись; +- Существование директивы `keyed` в зависимости `lit`, точность ссылок + на номера строк `houseplan-card.ts:11787/12996` и на существующие + мутанты в `scripts/mutation-gate.mjs` — репозиторий на этих участках + не менялся с r1, перепроверка кода не требовалась (я всё же прочитал + эти строки заново в рамках проверки новой находки ниже, они совпадают + с тем, что зафиксировал r1). + +## Разбор AC (только то, чего касается дельта) + +- **AC2а** (новая) — формулировка однозначна, способ доказательства + назван (`smoke`, вероятно расширение того же + `demo/smoke_space_switch_transitions.mjs`, который назван в + «Затронутые файлы»), критерий прохождения проверяем по идентичности + узла, как и AC2. Двусмысленности нет. **Но её область — только + маркеры устройств**, см. находку ниже. +- **AC5** — обновлена под новую форму кода (два мутанта: снимающий + внешний `keyed` и снимающий внутренний `repeat`). Явно называет + переанкеровку `device-markers-rendered-without-keys`. **Не называет** + `openings-rendered-without-keys`, хотя его якорь (`mutation- + gate.mjs:7441`, буквальная строка с `repeat(items, (o) => o.id, …)`) + тоже перестанет совпадать один-в-один после того, как К2 обернёт + список проёмов в `keyed(…)` — та же механика, что и для маркеров. + Практически это не тихий пропуск (`applyPatches` бросает исключение + на несовпавшем якоре, PROCESS §8/находка выше), но ТЗ не проговаривает + требование явно, оставляя его исполнителю угадывать. +- **AC7** — ссылка на «комментарий К3» согласована с новой нумерацией + контракта (проверено: К3 в новом тексте — это статья про + пер-элементный ключ, а не про число проёмов, как в r1). «Затронутые + файлы» требуют дополнить комментарий-ловушку **в обоих** файлах стилей + (`plan.styles.ts` и `devices.styles.ts`) — см. находку ниже про то, что + сам контракт К3 обосновывает это только для одного из двух. + +## Находки + +### Medium — К2 распространяет `keyed+repeat` на список проёмов «для единообразия», но обоснование (К3) и свидетель (AC2а) написаны только для маркеров устройств + +**Что меняет К2.** «Оба списка — маркеры устройств и проёмы — рендерятся +как `keyed(<идентификатор пространства>, repeat(items, (item) => +item.id, …))». Это новое требование к списку проёмов: до этого раунда +(и в r1, и сегодня в коде) проёмы рендерятся голым +`repeat(items, (o) => o.id, …)` без внешнего `keyed`. Причина +унификации — допущение №3: «проёмов… не заслуживают отдельного режима, +поэтому оба списка приводятся к одной форме» — обоснование +единообразием, а не функциональной необходимостью. + +**К3 и AC2а покрывают только один из двух списков.** К3 («почему +пер-элементный ключ обязан остаться») целиком про маркеры: «Состав +списка маркеров меняется без смены пространства: призраки в редакторе +устройств (`showGhosts`) и появление устройства по живому синку +конфига… на `.device-shell-frame`… живы переходы… Убрать внутренний +ключ — значит вернуть тот же класс дефекта». AC2а — тоже буквально «свои +детель К3… **состав списка маркеров** меняется… ни один существующий +**маркер** не меняет свой DOM-узел». Ни К3, ни AC2а не упоминают проёмы. + +**Тот же риск для проёмов подтверждён чтением, а не гипотетичен.** + +1. Проёмы уже сегодня несут живые переходы того же типа, что маркеры, и + это задокументировано прямо в комментарии-ловушке #525 + (`src/styles/plan.styles.ts:531-542`, тот самый абзац, который К2/К3 + в этом раунде требуют дополнить): `.op-leaf { transition: transform + 0.6s ease }`, `.op-arc { transition: stroke-dashoffset 0.6s ease }`. + Комментарий прямо называет обе причины рядом: «the openings and the + device markers… with this issue [#525]» — то есть исходная задача + #525 уже трактовала оба списка как один класс риска, и текущее ТЗ + (К2) следует этой логике для перфоманса, но не переносит её в + контракт корректности К3. +2. У проёмов есть собственный, отличный от маркеров, но настоящий + триггер изменения состава списка **внутри одного пространства**: + `_openingsR` (`src/houseplan-card.ts:8226-8260`) добавляет + orphan-запись, только когда `this._mode === 'plan'` + (`:8241-8243`), и убирает её, когда режим не `plan` + (`:8244`, `flatMap` возвращает `[]` для нерешённого хоста). То есть + переключение между режимами Plan/View **в одном и том же + пространстве** меняет длину и, соответственно, позиции элементов + `items`, который затем рендерится тем же `repeat` в + `_renderOpenings` (`:12987-12996`) — прямой аналог `showGhosts` для + маркеров, только с другим триггером. + +**Почему это Medium, а не Low.** Если `repeat` для проёмов оставлен «по +инерции единообразия» без записанной причины и без свидетеля, следующий +читатель — ровно тот сценарий, которого боится сама К3 («иначе следующий +читатель уберёт «лишнюю» обёртку») — с равным основанием сочтёт его +избыточным именно для проёмов, потому что ни контракт, ни тест этого не +запрещают. Регрессия при этом ловится AC1/AC2 только частично: они +проверяют переключение *между* пространствами, а не смену режима +*внутри* одного. Находка того же класса и в той же строке контракта +(К2/К3), которую r1 уже поднимал для маркеров — только в r2 асимметрия +переехала на второй список, который сама эта правка впервые +подвела под общий режим. + +**Что нужно для DoR (решает автор):** +1. распространить К3 и AC2а на оба списка одним общим утверждением + (единый триггер формулировки — «состав списка меняется без смены + пространства» верно и для orphan-переключения Plan/View) и завести + AC2б либо расширить AC2а на проёмы конкретным сценарием (например, + вход/выход из режима Plan при наличии проёма с нерешённым хостом); либо +2. явно обосновать, почему для проёмов регресс невозможен либо не имеет + значения (например, если `.op-leaf`/`.op-arc` не проигрывают + транзишн в застывшем Plan-виде, или если orphan-переключение всегда + сопровождается прочим ре-рендером, который сбрасывает состояние) — и + принять это как записанный риск, а не молчаливое умолчание; +3. как минимум явно назвать в AC5 переанкеровку + `openings-rendered-without-keys` наравне с `device-markers- + rendered-without-keys` — это не закрывает риск (1)/(2), но убирает + как минимум неполноту самого AC5 относительно кода, который сам же + К2 меняет. + +**Решение ревьюера:** блокирует переход в `S5-ready` без правки одним из +способов 1 или 2 выше (способ 3 сам по себе недостаточен — устраняет +только неполноту AC5, но не закрывает риск отсутствия контракта/AC для +проёмов). Medium в скоупе задачи — К2 в этом самом раунде впервые +распространила изменение на список проёмов, поэтому решение относится к +предмету этой правки, а не к соседнему поведению; отдельный issue не +заводится (§2.4, §2.7, #202). + +## Что проверено и корректно + +- Обе находки r1 (Medium и Low) закрыты текстом ТЗ, проверено построчным + чтением, а не заявлением автора — таблица «Закрытие раунда r1» выше. +- Допущение №2 больше не путает конкретный фикс (#524, `box-shadow`) с + общим случаем: теперь это утверждение о производительности, а не о + безопасности, и корректность К3 от его истинности не зависит — `repeat` + защищает по ключу независимо от совпадения порядка. +- AC2а — новый, недвусмысленный AC с указанным способом доказательства. +- Нумерация контракта (К1–К5) внутренне согласована: перекрёстные ссылки + AC7→К3, AC5→AC2а, Риск 2→К3/AC2а/АС5 указывают на верные, актуальные + пункты нового текста. +- Технические ссылки К2 (директива `keyed`, номера строк 11787/12996, + существующие мутанты) точны — перепроверены на неизменном с r1 коде. +- Число проёмов («порядка тридцати на этаж») теперь соответствует + фикстуре `large-house` (было подтверждено ещё в r1). + +## Чего не проверял + +- Не запускал код и гейты — этап spec, продуктовый код не менялся с r1 + (§2.4, §8), как и в прошлом раунде. +- Не проверял, действительно ли `.op-leaf`/`.op-arc` физически + проигрывают переход в статичном Plan-виде при переключении режима + (например, если элемент в момент переключения не виден или + transition подавлен другим правилом) — находка выше опирается на + чтение CSS и `_openingsR`, а не на измерение в браузере; это ровно тот + вопрос, который DoR-требование (пункт 2 находки) просит закрыть + автору чтением/измерением, если он выберет этот путь вместо пункта 1. +- Не проверял частоту реального использования Plan/View-переключения с + ровно одним orphan-проёмом «в проде» — как и в r1 по аналогичному + вопросу для маркеров, для решения находки (правка контракта либо + обоснованный отказ) это не требуется: сценарий не исключён кодом и не + покрыт тестом, этого достаточно, чтобы вернуть на уточнение. + +## Вердикт + +Жёлтый. Один Medium в скоупе задачи: К2 в этом раунде распространила +`keyed(space.id, repeat(…))` на список проёмов «для единообразия», но +контракт К3 и его свидетель AC2а обосновывают и проверяют сохранение +внутреннего ключа только для маркеров устройств, хотя тот же класс риска +у проёмов подтверждён чтением (комментарий-ловушка #525 уже описывает их +как один класс, а `_openingsR` меняет состав списка при переключении +Plan/View внутри одного пространства). High нет. Обе находки r1 закрыты +текстом, проверено построчно. Возврат автору на правку одним из двух +названных способов (расширить К3/AC2а на проёмы, либо обосновать и +записать отказ как риск). + +--- + + + +## Материал раунда + +- Ветка: `dev`, коммит `e40d3f18d2fd` — ребейз его осиротит, и это нормально: ниже якоря, которые ребейз не меняет. +- Дерево материала: `d42689f768439e92dac62df2f8055b5f2ed7d1db` + ``` + git log --all --format='%H %T' | grep d42689f76843 + ``` +- Тело issue: `e9c78bcaf2429aff824dd80820d9b17d1a73c4b49c557df613d54a59d5590907` +- Вердикт конвейера: `yellow` · High 0