diff --git a/docs/reviews/CODE-REVIEW-521-r2.md b/docs/reviews/CODE-REVIEW-521-r2.md new file mode 100644 index 00000000..71fc056e --- /dev/null +++ b/docs/reviews/CODE-REVIEW-521-r2.md @@ -0,0 +1,274 @@ +# CODE-REVIEW-521-r2 + +Issue: #521 «fix: alignment guides follow the live gesture again» +Материал ревью: `9a3af808d95684b32df8f102da4211f3f800703c` (ветка +`issue/521-align-guides-live`). Диапазон делты этого раунда — +`0715321fdc69a2955d7850f3f384c1a0fdc25311..9a3af808` (материал r1 → материал +r2): один коммит `deebcfb0` (публикация `CODE-REVIEW-521-r1.md` конвейером, +0 изменений кода) и один продуктовый/тестовый коммит `9a3af808` «fix: hide the +settled guides copy for the whole gesture, and prove it». +Заход: r2 · блокирующих циклов израсходовано 1 из 4 (r1 — жёлтый). + +## Скоуп + +Единственная находка r1 (Medium, в скоупе) — AC5 заявлял доказательство +(«снятие подавления осевой копии слоя даёт 2 группы `.alignguides` — смок +красный»), которого прежний свидетель не давал: осевая копия +`.hp-editor-only-layer` в сценарии смока всегда была пуста, потому что смок ни +разу не форсировал осевую отрисовку посреди самого жеста. Дельта r1→r2 — +исправление ровно этого: переписанный сценарий `devices` в +`demo/smoke_align_guides.mjs` форсирует настоящую осевую отрисовку +(`c._hdrH += 0.01`) в середине перетаскивания, проверяет, что она +действительно произошла, что владение слоем перешло к осевшей сцене с +правильной (живой) точкой, что видимая группа ровно одна на каждом шаге +передачи, и что следующее движение возвращает владение живому художнику; плюс +новый мутант `live-editor-keeps-the-settled-guides-visible` в +`scripts/mutation-gate.mjs`, откатывающий подавление обратно в ветку `plan` +(как было до задачи). + +Продуктовый код (`src/live-editor.ts`) в этой дельте не изменился по существу: +единственная правка — комментарий, объясняющий измеренный механизм передачи +слоя (было общее «плановый режим уже это делал», стало «владение чередуется +само, `_commitLiveEditor()` … измерено: две `.alignline` …»). Логика +`makeTransparent(...)` вне ветки `plan` осталась той же, что была принята и +проверена в r1 — сама защита не менялась, менялось только доказательство её +работы. + +Дельта локальна по критериям §2.10: не ребейз (единственный промежуточный +коммит — публикация документа предыдущего раунда), не смена контракта +поведения (контракт п.3/AC5 переформулированы под тот же механизм, а не +заменены другим), не новая подсистема, объём (17 строк комментариев в +`live-editor.ts`, 16 строк нового мутанта, ~90 строк в свидетеле) далеко не +сопоставим с исходной задачей. Полный повторный разбор всей задачи не +требуется — по AC1–AC4, AC6–AC9 переверяю только то, что могла задеть дельта +(сам свидетель), остальное наследую из r1. + +### Отдельная находка: тело issue правилось после зелёного ревью ТЗ (#517) + +Хеш нормализованного тела issue на момент зелёного `SPEC-REVIEW-521-r3.md` +(«Материал раунда», конец документа) — `1e7f8d192f340e80e878aec0d6de383c982263254b408f8fc928f383130f5684`. +Текущий хеш (пересчитан дважды независимо: `gh issue view --json body` и +`gh api repos/.../issues/521 -q .body`, оба дают одинаковый нормализованный +текст) — `d117679d25abeab7ebfad3ed7902a8222625fc2ad43d0396b56d6fdacc12b5d3`. +Хеши расходятся — тело менялось после зелёного ревью ТЗ. Дельту GitHub не +хранит, поэтому проверяю не разницу, а текущий текст целиком (см. «Что +проверено» ниже). + +Что именно изменилось, восстанавливается по признанию автора в комментарии +«Исправление по код-ревью r1 → заход r2»: «ТЗ: пункт контракта 3 и AC5 +переписаны под измеренный механизм». Прочитал текущий пункт 3 «Контракта» и +строку AC5 в теле issue — они дословно описывают именно тот механизм +(чередование владения через `_commitLiveEditor()`, измеренные «две +`.alignline`, осевая отстаёт на шаг»), который выяснился при разборе r1 и +теперь доказан переписанным свидетелем. Текст не расходится с кодом и тестами +этого раунда — я сверил это построчно (см. «Что проверено»), а не принял на +слово автора. + +**Оценка:** это находка процесса — правка раздела `## ТЗ`, уже прошедшего +зелёное ревью, должна по идее возвращаться на новый цикл `S4-spec-review`, а +не вноситься по ходу код-ревью. Но по существу правка не меняет ни персону, ни +сценарий, ни объём видимого изменения, ни один AC-номер — она уточняет +формулировку контракта и доказательства AC5 под механизм, который сам же +код-ревью r1 и вскрыл, и делает это прозрачно (объяснено в комментарии, а не +тихой правкой). Отдельного цикла ревью ТЗ эта поправка не заслуживает: именно +для такого случая существует делегирование «технический спор решает ревью +кода» (`PROCESS.md` §7.1) — здесь спора и нет, есть согласие с находкой r1, +выраженное в тексте. Классифицирую как **Low**, снимаю решением ревьюера: +текст ТЗ проверен целиком и корректен, действие не требуется. + +## Как проверялось + +Валидация на материале раунда (`9a3af808`) зелёная +(https://github.com/Matysh/houseplan-card/actions/runs/34526718586), включая +шесть параллельных джобов «Мутанты по диффу» — я прочитал логи всех шести +шардов напрямую (`gh run view --job --log`), а не поверил статусу +«success» на слово: + +| Гейт | Прогнал | Результат | +|---|---|---| +| Validate на `9a3af808` (`tsc`, `test`, `build`+bundle-sync, `no-new-any`) | нет, зачтено по зелёному прогону CI (см. ссылку) | success | +| Логи 6 шардов «Мутанты по диффу» в этом прогоне | да, прочитаны построчно | все 5 мутантов задачи пойманы: `live-editor-devices-drops-align-guides` (шард 5/6), `live-editor-decor-drops-align-guides` (4/6), `live-editor-plan-drops-align-guides` (1/6), `align-point-reads-frozen-snapshot` (2/6), **`live-editor-keeps-the-settled-guides-visible` (6/6)** — новый мутант этого раунда, «тест покраснел, как обязан» | +| `npm run bundle:sync` (пересборка + сверка `dist`/`custom_components/.../frontend`/`demo/srv/assets`) | да, локально (нужно было поднять `demo/srv/assets`, он не коммитится) | чисто, `git status` не показал diff после сборки — включает `tsc --noEmit` | +| `node demo/smoke_align_guides.mjs` (полный сценарий, все 34 поля) | да | все `true`, `OK` | +| `node scripts/mutation-gate.mjs --id=live-editor-keeps-the-settled-guides-visible` | да, независимо от CI, в рабочем дереве материала | «поймано 1 из 1» | +| Ещё 4 мутанта задачи по одному (`--id=…`) | да, все 4 | все «поймано 1 из 1» | +| `node --test test/smoke-harness-contract.test.mjs` (AC8) | да | 7/7 | +| `node scripts/check-docs.mjs` | да | зелёный, 7 файлов, 12 внешних ссылок | +| `node scripts/smoke-select.mjs --base 0715321f --head HEAD` (выборка по дельте раунда, не по всей задаче) | да | 1 прямое совпадение — `demo/smoke_align_guides.mjs` (символ `_alignPoint`); других смоков дельта не касается | +| `node demo/smoke_isometric_live_touch.mjs`, `node demo/smoke_touch_tips.mjs` | да (продуктовый код не менялся между r1/r2, но риск ТЗ по touch не был явно закрыт в r1 — закрываю здесь) | оба зелёные, все поля `true` | +| `cmp dist/houseplan-card.js custom_components/.../houseplan-card.js`, то же для `houseplan-panel.js` и `houseplan-assets.json` | да | побайтово идентичны | +| `git show -s --format=full 9a3af808` (трейлеры) | да | `Issue: #521`, `User-Visible: no` — верно: дельта не меняет видимого поведения (сама правка уже была `User-Visible: yes` в `53585b45`, эта дельта — доказательство и комментарий) | +| Хеш тела issue (`issueBodyDigest` из `scripts/review-doc-guard.mjs`, два независимых способа получения текста) | да | не совпадает с зафиксированным в зелёном `SPEC-REVIEW-521-r3.md` — находка выше | + +Не прогонял: `golden:verify` (дельта раунда не меняет визуал — только +комментарий, тестовый файл и конфиг мутанта; видимый рендер не тронут), +`pytest tests_backend` (Python не тронут), полный набор из 239 смоков (не +обосновано ни диффом, ни `smoke-select`), performance-профили (см. ниже), +WSL/HA-харнесс. + +**Перф (AC9).** Не перезамерял. Диф `src/live-editor.ts` между материалом r1 и +материалом r2 — только комментарии (проверил построчно: `if`/`makeTransparent` +вызовы в `paintHouseplanEditor` и `editorTemplate` идентичны байт в байт по +существу, разница — только текст комментариев). Комментарии не выполняются, +поэтому число операций в кадре жеста не изменилось ни на одну со времён r1, где +я независимо (не на слово автора) прогнал `benchmark:large-house-interaction` ++ `benchmark:compare` с собственной базой на `fa7ac02c` и получил зелёный отчёт +без регрессии (см. `docs/reviews/CODE-REVIEW-521-r1.md`, таблица гейтов). Этот +вывод наследуется без повторного прогона. + +## Закрытие раунда r1 + +| Находка r1 | Чем закрыта | Где это видно | +|---|---|---| +| Medium: AC5 заявлял «снятие подавления даёт 2 группы `.alignguides` — смок красный», но смок никогда не форсировал осевую отрисовку посреди жеста, поэтому осевая копия всегда была пуста и мутация не проверялась | Сценарий `devices` смока форсирует настоящую осевую отрисовку через `c._hdrH += 0.01` (свойство вне `liveProperties`/`hoverProperties`/`gestureProperties` — проверил по `src/live-editor.ts:80-86`, оно не маршрутизируется в живой путь ни при каком состоянии жеста), проверяет, что рендер действительно случился (`handoverHappened`, счётчик `willUpdate`), что слой перешёл сцене с правильной живой точкой (`handoverGivesTheLayerBackToTheSettledScene`, `handoverGuideStaysOnTheLivePoint`), что видимая группа ровно одна (`handoverKeepsExactlyOneGuide`), и что следующее движение возвращает владение живому художнику (`nextMoveTakesTheLayerBack`). Новый мутант `live-editor-keeps-the-settled-guides-visible` откатывает `makeTransparent(...)` обратно в ветку `plan` — смок красный именно на `nextMoveTakesTheLayerBack` | `demo/smoke_align_guides.mjs:162-197`; собственный прогон смока — все поля `true`; собственный прогон `mutation-gate.mjs --id=live-editor-keeps-the-settled-guides-visible` — «поймано 1 из 1»; то же самое независимо подтверждено логом шарда 6/6 в Validate на `9a3af808` | +| (побочно устранённая слабость свидетеля, не отдельная находка r1) счёт направляющих по числу узлов в DOM вместо видимых — гашение делает копию прозрачной, а не удаляет | `visible(selector)` фильтрует по `layer.style.opacity !== '0'` вместо сырого `querySelectorAll`; проверил, что `makeTransparent` (`src/live-editor.ts:157-163`) действительно ставит `element.style.opacity = '0'` строкой, а `restore()` снимает через `removeProperty('opacity')` | `demo/smoke_align_guides.mjs:27-36`, `src/live-editor.ts:157-163`, `:213-220` | + +Находка закрыта по существу: не «сделали AC5 неопровержимым переписыванием +слов», а заставили смок действительно воспроизвести сценарий (внешний осевой +рендер посреди активного жеста), который единственный делает защиту +наблюдаемой, и подтвердили мутацией, что без защиты сценарий ловится. + +## Унаследовано из r1 + +Без повторной проверки в этом раунде приняты выводы +`docs/reviews/CODE-REVIEW-521-r1.md` (материал: +`0715321fdc69a2955d7850f3f384c1a0fdc25311`), поскольку дельта r1→r2 их не +задевает: + +- диагноз и причинность регресса (`c0d61ca3`/#451, оба слома — слой и + замороженная точка) — не менялись, дельта их не касается; +- AC1 (направляющая в трёх живых жестах), AC2 (живая точка), AC3, AC4 — + свидетель для `decor`/`plan`-сценариев не менялся вовсе в этой дельте + (правки смока — только в сценарии `devices`, между строками 162 и 197); + мутанты `live-editor-devices-drops-align-guides`, + `live-editor-decor-drops-align-guides`, `live-editor-plan-drops-align-guides`, + `align-point-reads-frozen-snapshot` перепрогнаны мной заново тем не менее + (дёшево, один и тот же смок теперь длиннее) — все 4 по-прежнему «поймано 1 из + 1»; +- AC6 (один расчёт кандидатов на кадр) — код и проверка не менялись в этой + дельте, наследуется техническое заключение r1 («проверено чтением», без + отдельного мутанта — осталось так же, это не регрессия дельты); +- AC7 (старые гарантии, #400) — не менялся код и не менялся тот участок + смока; +- AC8 (свидетель не фабрикует состояние) — тест-линт `test/smoke-harness-contract.test.mjs` + не менялся; перепрогнал (7/7) на новом более длинном смоке — линт по-прежнему + проверяет отсутствие `_deviceDrag =`/`_decorDraft =`, новые строки (`c._hdrH + = …`, `c._layout = …` уже был в старой версии) под запрет не подпадают; +- AC9 (перф) — см. «Как проверялось» выше, база `fa7ac02c` и зелёный отчёт r1 + наследуются без повторного прогона, так как дельта не трогает исполняемый + код; +- бюджет `core-file-budget` и мягкость `_renderAlignGuides` на карточке + (`this._editorRuntime?._renderAlignGuides() ?? nothing`) — не менялись, + проверено в r1 чтением; +- трейлеры и changelog коммита `53585b45` — не менялись, `User-Visible: yes` + подтверждён в r1. + +Технический раздел «Чего не проверял» из r1 (golden, pytest, полная матрица +смоков, WSL/HA-харнесс) остаётся в силе тем же образом — дельта раунда их не +задевает и не расширяет их необходимость. + +## Находки + +### Low-1 (снимается, см. выше) — тело issue правилось после зелёного ревью ТЗ без нового цикла `S4-spec-review` + +См. раздел «Отдельная находка» в «Скоупе». Текст пункта 3 «Контракта» и AC5 +переписан автором вместе с исправлением по код-ревью r1, без возврата в +`S3-spec`/`S4-spec-review`. Проверено: новая формулировка точно описывает +реализованный и теперь доказанный механизм, персона/сценарий/объём/номера AC +не изменились, изменение раскрыто в комментарии автора. Действия не требую; +записываю, чтобы хеш в следующем зелёном документе (материал раунда ниже) +зафиксировал новое значение и цепочка не молчала. + +Других находок нет. + +## Что проверено и корректно + +- **Механизм передачи владения слоем воспроизведён чтением независимо от + комментария автора.** `updated()` (`houseplan-card.ts:4157`) на каждом + осевом рендере зовёт `_commitLiveEditor()` → + `commitHouseplanEditor` (`live-editor.ts:439-454`): отменяет + незавершённый `raf`, вызывает `restore(state)` (снимает `opacity`/`visibility`, + выставленные `makeTransparent`/`dim`/`hide`) и очищает живой корень + (`render(nothing, target)`). Значит после любого settled-рендера слой физически + переходит к осевшей сцене чистым, без «памяти» о прежнем подавлении. +- **`_hdrH` гарантированно не маршрутизируется в живой путь.** Прочитал + `liveProperties`/`hoverProperties`/`gestureProperties` + (`live-editor.ts:80-86`) и `routeHouseplanEditorUpdate` + (`live-editor.ts:120-145`) — `_hdrH` не входит ни в один набор, значит + `namedLive` ложно и осевой рендер происходит независимо от активного жеста. + Форсирование смоком (`c._hdrH += 0.01`) действительно моделирует внешний + settled-рендер (приход `hass`, ресайз), а не подглядывает во внутреннее + состояние жеста. +- **`_alignPoint` — геттер, а не хранимое состояние** + (`houseplan-card.ts:12879`), поэтому и живой, и осевой путь при вызове + `_renderAlignGuides()` во время самого settled-рендера читают одну и ту же + живую точку — вот почему «отставание на шаг» проявляется только НА + СЛЕДУЮЩЕМ живом кадре после хендовера, а не в момент самого хендовера; смок + проверяет обе фазы раздельно (`handoverGuideStaysOnTheLivePoint`, затем + `nextMoveTakesTheLayerBack`), и это совпадает с объяснением автора. +- **Мутант `live-editor-keeps-the-settled-guides-visible` патчит именно тот + код, который сейчас в файле** — сверил `find`-строку патча + (`scripts/mutation-gate.mjs`) с текущим `paintHouseplanEditor` + (`live-editor.ts:357-360`) посимвольно, совпадает; применимость и результат + подтверждены собственным прогоном и логом CI. +- **AC8 не сломан новыми строками смока.** `c._hdrH = …`, `c._layout = …` (уже + было раньше), `c.requestUpdate()` не запрещены тест-линтом — запрещены + только присваивания `_deviceDrag =`/`_decorDraft =`; перепрогнал линт, 7/7. +- **Touch-контракт не задет** (риск ТЗ явно требовал: «существующие + touch-смоки обязаны остаться зелёными») — `smoke_isometric_live_touch.mjs` и + `smoke_touch_tips.mjs` зелёные на материале раунда; в r1 эти два смока не + были явно названы прогнанными, закрываю пробел здесь, а не переношу его + дальше. +- **«Одно число — один источник» не применимо.** Направляющая — линия и точка, + не число; пользователю в этом диффе не показывается ни одна величина + дважды. +- **Бандл и трейлеры.** `bundle:sync` пересобрал дерево с нуля, `dist`/ + `custom_components/.../frontend`/`demo/srv/assets` побайтово совпали с + закоммиченными файлами; `git show -s --format=full 9a3af808` — трейлеры + `Issue: #521`/`User-Visible: no` корректны, changelog не тронут и не должен + быть (дельта раунда не меняет видимое поведение сверх уже опубликованного в + `53585b45`). + +## Чего не проверял + +- `npm run golden:verify` — дельта раунда не меняет визуал (комментарий + + тестовый файл + конфиг мутанта), в r1 видимый рендер тоже не менялся кроме + уже принятого identical-скриншота. +- `python -m pytest tests_backend` — Python не тронут ни в одном коммите + задачи. +- Полная матрица из 239 смоков — не обоснована ни диффом раунда, ни + `smoke-select.mjs` (одно прямое совпадение); полный набор — предрелизный + гейт. +- Не перезамерял `benchmark:large-house-interaction` — обосновано выше + (дельта раунда не исполняемый код). +- Не запрашивал у владельца дополнительного цикла `S4-spec-review` по поводу + правки ТЗ после зелёного вердикта r3 — see Low-1: решил вопрос по существу + сам, поскольку это чисто техническая правка формулировки под уже найденный в + код-ревью механизм, а не продуктовый вопрос из числа тех, что задаются + владельцу (§7.1). + +## Вердикт + +Единственная находка предыдущего раунда закрыта по существу и подтверждена +воспроизведением (собственный прогон смока и мутанта плюс независимое чтение +логов CI). Новых блокирующих находок нет; единственная новая находка — +процедурная (Low), снята решением ревьюера с записью выше. High: 0. Задача +готова к очереди на пре-релиз. + +Вердикт: зелёный · заход r2 · блокирующих циклов 1/4 · High: 0 · Medium: 0 → в задаче + +Документ: docs/reviews/CODE-REVIEW-521-r2.md + +--- + + + +## Материал раунда + +- Ветка: `issue/521-align-guides-live`, коммит `9a3af808d956` — ребейз его осиротит, и это нормально: ниже якоря, которые ребейз не меняет. +- Дерево материала: `20fbfc5c7f020a361c04f252900511c2e383ceba` + ``` + git log --all --format='%H %T' | grep 20fbfc5c7f02 + ``` +- Тело issue: `d117679d25abeab7ebfad3ed7902a8222625fc2ad43d0396b56d6fdacc12b5d3` +- Вердикт конвейера: `green` · High 0