From deebcfb095bc082446e99d2576c8875f095b4bab Mon Sep 17 00:00:00 2001 From: "claude[bot]" <209825114+claude[bot]@users.noreply.github.com> Date: Thu, 10 Sep 2026 20:05:44 +0000 Subject: [PATCH] docs: review document for #521 Issue: #521 User-Visible: no --- docs/reviews/CODE-REVIEW-521-r1.md | 219 +++++++++++++++++++++++++++++ 1 file changed, 219 insertions(+) create mode 100644 docs/reviews/CODE-REVIEW-521-r1.md diff --git a/docs/reviews/CODE-REVIEW-521-r1.md b/docs/reviews/CODE-REVIEW-521-r1.md new file mode 100644 index 00000000..0bf3e05e --- /dev/null +++ b/docs/reviews/CODE-REVIEW-521-r1.md @@ -0,0 +1,219 @@ +# CODE-REVIEW-521-r1 + +Issue: #521 «fix: alignment guides follow the live gesture again» +Материал ревью: `0715321fdc69a2955d7850f3f384c1a0fdc25311` (ветка `issue/521-align-guides-live`, HEAD == `origin/dev` + 2 коммита: `53585b45` продукт, `0715321f` отпечаток скриншотов). +Заход: r1 · блокирующих циклов израсходовано 0 из 4. + +## Скоуп + +Регресс #451 (перевод жестов редактора на «живой» путь) убрал направляющие +выравнивания из трёх жестов десктопных редакторов (значок в `devices`, фигура +в `decor`, курсор в `plan`). Задача восстанавливает слой направляющих в живом +пути и чинит вторую поломку — `_alignPoint` в режиме `devices` читал +замороженный `_pos` вместо живого `_livePos`. Класс A, полный трек (владелец +назвал нарушенный критерий §5 — влияние на touch/перф-контракт кадра жеста). +Диф продукта: `src/live-editor.ts` (+19/−4), `src/houseplan-card.ts` (+4/−3). +Плюс: 4 новых мутанта (`scripts/mutation-gate.mjs`), тест-линт свидетеля +(`test/smoke-harness-contract.test.mjs`), полностью переписанный +`demo/smoke_align_guides.mjs`, оба changelog, пересобранный бандл, второй +коммит — обновление отпечатка скриншотов документации (`docs:accept +--identical`, картинки побайтово не изменились). + +## Как проверялось + +Ревью прочитан по коду и прогнан руками — часть независимо от заявлений +автора, часть с воспроизведением находок. Дешёвые гейты (`tsc`, `test`, +`build`+сверка бандла) уже зелёные на этом SHA в Validate +(https://github.com/Matysh/houseplan-card/actions/runs/34520976425) — не +перегонял их ради самого факта зелени, но `npm test` и локальную сборку всё +же выполнил повторно, поскольку без свежего `demo/srv/assets/houseplan-card.js` +(не коммитится, #255) браузерные смоки было не запустить. + +| Гейт | Прогнал | Результат | +|---|---|---| +| `npx tsc --noEmit` | да (часть `npm run build`) | чисто | +| `npm test` | да | 2515 pass / 0 fail / 1 skip | +| `npm run build` + сверка `dist` / `custom_components/.../frontend` / `demo/srv/assets` | да | побайтово идентичны рабочему дереву, `git status` чист после сборки | +| `node scripts/no-new-any.mjs --base origin/dev --head HEAD` | да | новых `any` нет (25 добавленных строк в 2 файлах) | +| `node scripts/check-docs.mjs` | да | зелёный (диф трогает `src/**`) | +| `node scripts/smoke-select.mjs --base origin/dev --head HEAD` | да | 11 прямых совпадений (`_pos`, `_livePos`, `_editorRuntime`, `_editorRuntimeOrThrow`) + 22 слабые связи | +| Прямые смоки (`align_guides`, `cover_no_plate`, `cover_plate_precedence`, `device_position_history`, `drag_bounds`, `edge_cases`, `glow`, `grid_snap`, `light_badges`, `modes`, `v8_draft_write`) | да, все 11 | зелёные | +| Точечно из «слабых»/AC-рисков (`editor_gestures`, `decor`, `merge_split`) | да | зелёные | +| `node scripts/mutation-gate.mjs --check` (применимость патчей) | да | все мутанты, включая 4 новых задачи, применимы | +| Мутанты задачи по одному (`live-editor-devices-drops-align-guides`, `live-editor-decor-drops-align-guides`, `live-editor-plan-drops-align-guides`, `align-point-reads-frozen-snapshot`) | да, все 4, полный прогон через `scripts/mutation-gate.mjs --id=…` (git worktree, пересборка, смок) | все 4 «поймано 1 из 1» — свидетель действительно краснеет | +| `node --test test/smoke-harness-contract.test.mjs` | да | 7/7, включая новый тест-линт #521 | +| AC9: `npm run benchmark:large-house-interaction` (7 образцов) + `benchmark:compare` против `budgets-large-house-interaction.json` | да, самостоятельно — база собрана в отдельном worktree на `fa7ac02c` (= `origin/dev`, тот же SHA, что назвал автор) | все метрики ✅, в частности `timing.interactionSeriesMs.median` 3175.4/3300, `timing.editorSeriesMs.median` 612.1/750, `longTask.maxSingleMs` 1184/1618.5 — без регрессии к базе | +| Собственный эксперимент: пришить обратно старое условие (`makeTransparent` только в ветке `plan`, т.е. без правки контракта №3) в изолированный `git worktree`, пересобрать, прогнать **сам** `demo/smoke_align_guides.mjs` | да | смок остался **зелёным** — см. находку Medium-1 | +| golden, `pytest tests_backend`, полный набор смоков | нет | не требуются диффом (визуал не менялся кроме отпечатка, бэкенд не тронут); полный набор — предрелизный гейт | + +Не прогонял намеренно: `golden:verify` (диф не меняет пиксели вне подтверждённого +identical-принятия), `pytest tests_backend` (Python не тронут), полную матрицу +смоков (239 штук — не требуется диффом), WSL/HA-харнесс. + +## Находки + +### Medium-1 (в скоупе) — AC5 заявляет доказательство, которого смок не даёт + +**AC5:** «Во время жеста в DOM ровно одна группа `.alignguides`»; «чем +краснеет»: *«снятие подавления осевого слоя даёт 2 — смок красный»* +(`docs/reviews` таблица AC, тело issue). + +Это не так. Правка вынесла +`makeTransparent(state, root, '.hp-editor-only-layer:not(.hp-plan-snap-layer)')` +(`src/live-editor.ts:358`) из ветки `if (host._mode === 'plan')` в +безусловное начало `paintHouseplanEditor` — то есть теперь осевая копия слоя +гасится в **любом** режиме редактора, а раньше гасилась только в `plan`. Это +разумная защита ровно от риска, названного в самом ТЗ («Двойной слой… если во +время жеста случится осевая отрисовка»). Но проверил экспериментально: в +отдельном `git worktree` (не в материале ревью) вернул эту строку внутрь +`if (host._mode === 'plan') { … }`, как было до задачи, пересобрал бандл и +прогнал **сам** переписанный `demo/smoke_align_guides.mjs` — все 26 полей +вывода, включая `devSingleGuideLayer`, `decorSingleGuideLayer`, +`planSingleGuideLayer`, остались `true`, смок напечатал `OK`. + +Причина: `_renderAlignGuides()` в осевшей сцене (`houseplan-card.ts:11698-11699`) +рисует `.alignguides` только когда `_alignPoint` не `null`; на момент +последней осевой отрисовки перед стартом жеста (`pointerdown`, до которого +`_deviceDrag.moved`/`_decorDraft`/`_cursorPt` ещё не в «жестовом» состоянии) +эта группа пуста, и других осевых перерисовок в ходе самого смока не +происходит — весь жест идёт по живому пути. Значит замороженная копия все +время остаётся пустой независимо от того, подавлена она `opacity:0` или нет, +и `groups() === 1` истинно в обоих случаях. Сценарий, который правка реально +защищает (осевая отрисовка **посреди** жеста — приход `hass`, ступенька +`_hdrH`, resize), смок не воспроизводит: он не форсирует ни одного стороннего +`requestUpdate()` во время движений. + +Проверил дальше (форсированный пробник в том же выброшенном worktree, не в +материале): если во время шагов перетаскивания значка искусственно вызвать +`c.requestUpdate('_holdFired')` (несвязанное реактивное свойство, эмулирует +внешний settled-рендер), эксперимент завис на превышении времени +Playwright — то есть даже принудительно вызвать вторую осевую отрисовку +внутри активного жеста в текущей демо-обвязке не тривиально; я не довёл этот +путь до результата и не настаиваю на нём как на предмете находки. Находка не +в том, что защита не работает, а в том, что **заявленное в AC5 доказательство +ложно**: третий столбец «чем краснеет» называет мутацию, которая, как +показано выше, смок не ловит. Это ровно тот случай, о котором PROCESS.md §2.7 +предупреждает отдельно: «пустой третий столбец — находка Medium»; здесь +столбец не пуст, но недостоверен, что не лучше. + +**Почему Medium, а не High:** сама защита в продукте существует и +концептуально верна (единственный явный визуальный риск — задвоенная линия у +пользователя, а не потеря данных/некорректная запись), просто не имеет +свидетеля, который её удержит. Регрессия по этой строке не сломает ни один +существующий тест. + +**Как чинить в скоупе задачи (предложение, не обязывающее автора):** заставить +смок форсировать осевую отрисовку в момент активного жеста — например, +`c.requestUpdate('_someUnrelatedSettledProp')` (свойство вне +`liveProperties`/`hoverProperties`/`gestureProperties`) на одном шаге каждого +из трёх сценариев, с проверкой `groups() === 1` сразу после, — либо честно +понизить формулировку AC5 до «проверено чтением», раз механизм осознанно +защищает окно, которое смок не воспроизводит. Второе дешевле; выбор — за +автором. + +### Наблюдение (не находка, для полноты) — AC6 тоже без мутанта + +Как и AC5, AC6 («один расчёт кандидатов на кадр») не имеет отдельной записи в +`scripts/mutation-gate.mjs`, только проверку внутри самого смока +(`candidatesAreComputedOncePerFrame`). В отличие от AC5, логика проверки +(`candidateCalls <= paints`, где пять `pointermove` шлются без ожидания между +собой) структурно способна поймать регрессию «расчёт на событие вместо кадра» +— каждое лишнее срабатывание `_alignCandidates()` вне кадра подняло бы +`candidateCalls` выше `paints`. Не воспроизводил такую мутацию отдельно (не +нашёл дешёвого точечного способа сместить расчёт в обработчик события без +более широкой правки); оставляю как «проверено чтением», не как находку. + +## Что проверено и корректно + +- **AC1–AC4** (направляющая в трёх живых жестах). Проверено смоком плюс + четырьмя мутантами — каждый мутант независимо пересобран и прогнан, каждый + дал красный смок («поймано 1 из 1»). Свидетель ведёт настоящие + `PointerEvent`/`click`, ждёт тишины `_hdrH` перед жестом, считает осевые + циклы за движения (обязаны быть 0) — воспроизвёл сам, наблюдения совпадают + с заявленными в issue числами (8 шагов сетки расхождения `_pos`/`_livePos`). +- **AC2** (живая точка выравнивания). `_alignPoint` в режиме `devices` читает + `_livePos(d)` (`houseplan-card.ts:12896`); `_livePos` читает `this._layout` + напрямую, минуя снимок `_renderDeviceSnapshot`, который использует `_pos`. + Мутант `align-point-reads-frozen-snapshot` подтверждает регрессию при + откате к `_pos`. +- **AC5 (частично)** — сама защита существует в коде и корректно расширена на + все три режима; не хватает свидетеля (Medium-1 выше). +- **AC6** — структура рендера гарантирует один расчёт на кадр: `editorTemplate` + вызывается один раз за `paintHouseplanEditor`, который сам собран в один + вызов на `requestAnimationFrame` через `scheduleHouseplanEditor` (гейт + `state.raf` не даёт повторной постановки в очередь). Проверено чтением + (`src/live-editor.ts:344-391`) и смоком (`candidatesAreComputedOncePerFrame` + = true при собственном прогоне). +- **AC7** (старые гарантии: нет гидов в Просмотре, нет гидов без совпадения, + #400-исключение перетаскиваемого маркера). Все три сохранены и переведены на + настоящий жест без прежней подмены состояния; проверено собственным + прогоном смока (`noneInView`, `devNoGuideOffAxis`, + `devCandidatesExcludeTheDraggedMarker`, `devGuideAnchorIsTheOtherMarker`). +- **AC8** (свидетель не фабрикует состояние жеста). `test/smoke-harness-contract.test.mjs` + регекспом проверяет отсутствие `_deviceDrag =`/`_decorDraft =` в + `demo/smoke_align_guides.mjs` — прогнал `node --test` на этом файле, 7/7, + включая новый тест; вручную проверил `grep` — присваиваний в файле + действительно нет, только чтение (`!!c._deviceDrag` и т. п.). +- **AC9** (перф). Перепрогнал независимо от автора: своя база на + `fa7ac02c` (голова `origin/dev`, тот же SHA, что назвал автор) в отдельном + `git worktree`, свой кандидат на материале ревью, `benchmark:compare` — + весь отчёт зелёный, ни одна строка не читает красным. Числа близки к тем, + что привёл автор в хендоффе (расхождение в пределах шума прогона). +- **`_renderAlignGuides` стал «мягким»** (`this._editorRuntime?._renderAlignGuides() ?? nothing`, + было `_editorRuntimeOrThrow()`). Проверено чтением: `_editing` (единственный + вызывающий контекст осевого пути) истинен только когда `_mode` уже + `plan`/`devices`/`decor`, а вход в эти режимы уже требует загруженного + `_editorRuntime` — так что смягчение не маскирует реальную ошибку на осевом + пути, а покрывает только окно живого жеста, начавшегося до полной загрузки + рантайма (контракт, пункт 5). +- **Трейлеры и changelog.** Оба коммита несут `Issue: #521` и корректный + `User-Visible`; `User-Visible: yes` коммит (`53585b45`) правит оба + changelog в этом же коммите (проверено `git show --stat`). +- **Отпечаток скриншотов** (`0715321f`) — принят через + `docs:accept --identical`, все 11 кадров побайтово совпали + (`imageSha256` не изменились), поэтому не требует ручной визуальной + приёмки владельца; коммит только меняет `sourceFingerprint`/`sourceSha256`. +- **Бандл.** Пересобрал с нуля из материала — `dist/`, + `custom_components/houseplan/frontend/`, `demo/srv/assets/` совпали с + закоммиченными файлами побайтово (`git status` чист после сборки). +- **Класс изменений и трек.** Класс A по прямому указанию владельца + (комментарий в issue), полный трек с названным нарушенным критерием §5 — + соответствует ТЗ и его зелёному ревью r3. + +## Чего не проверял + +- `npm run golden:verify` — диф не меняет видимую геометрию/стили сцены за + пределами уже принятого identical-скриншота; не увидел оснований гонять. +- `python -m pytest tests_backend` — `custom_components/**/*.py` не затронут. +- Полный набор из 239 смоков — не обосновано диффом (2 изменённых файла, + 7 символов на изменённых строках); прогнал 11 прямых совпадений плюс три + из широкого списка, относящихся к жестам редактора. +- WSL/полный HA-харнесс — вне гейта код-ревью. +- Не довёл до конца эксперимент с принудительным осевым рендером посреди + живого жеста (см. Medium-1) — не нашёл дешёвого способа в текущей демо- + обвязке и не стал тратить на это больше бюджета раунда; сама находка не + зависит от результата этого эксперимента. + +## Вердикт + +Единственная находка — Medium в скоупе задачи (недостоверное доказательство +AC5, не сама защита). High-находок нет. По правилу PROCESS.md §2.7 это жёлтый +вердикт с возвратом автору для правки в этой же задаче. + +Вердикт: жёлтый · заход r1 · блокирующих циклов 1/4 · High: 0 · Medium: 1 → в задаче + +Документ: docs/reviews/CODE-REVIEW-521-r1.md + +--- + + + +## Материал раунда + +- Ветка: `issue/521-align-guides-live`, коммит `0715321fdc69` — ребейз его осиротит, и это нормально: ниже якоря, которые ребейз не меняет. +- Дерево материала: `d8bf0a0fe057a7b6d2470eb288b88850c51d5a24` + ``` + git log --all --format='%H %T' | grep d8bf0a0fe057 + ``` +- Тело issue: `1e7f8d192f340e80e878aec0d6de383c982263254b408f8fc928f383130f5684` +- Вердикт конвейера: `yellow` · High 0