mirror of
https://github.com/Matysh/houseplan-card
synced 2026-10-01 12:18:51 +00:00
claude[bot]
@@ -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
|
||||
|
||||
---
|
||||
|
||||
<!-- material-anchors: сгенерировано конвейером (#414) -->
|
||||
|
||||
## Материал раунда
|
||||
|
||||
- Ветка: `issue/521-align-guides-live`, коммит `0715321fdc69` — ребейз его осиротит, и это нормально: ниже якоря, которые ребейз не меняет.
|
||||
- Дерево материала: `d8bf0a0fe057a7b6d2470eb288b88850c51d5a24`
|
||||
```
|
||||
git log --all --format='%H %T' | grep d8bf0a0fe057
|
||||
```
|
||||
- Тело issue: `1e7f8d192f340e80e878aec0d6de383c982263254b408f8fc928f383130f5684`
|
||||
- Вердикт конвейера: `yellow` · High 0
|
||||
Reference in New Issue
Block a user