diff --git a/docs/reviews/CODE-REVIEW-532-r1.md b/docs/reviews/CODE-REVIEW-532-r1.md new file mode 100644 index 00000000..43318b7b --- /dev/null +++ b/docs/reviews/CODE-REVIEW-532-r1.md @@ -0,0 +1,207 @@ +# CODE-REVIEW-532-r1 + +Вердикт: **жёлтый** · заход r1 · блокирующих циклов израсходовано 0 из 4 · High: 0 · Medium: 2 (в скоупе, возврат автору) + +Материал: `bb04f8605f9003c0ec6643359fa47af9319247bc` (ветка приведена к `dev` +конвейером; `origin/dev..HEAD` содержит один коммит, диплист диффа полный, не по +дельте — цикл первый). + +## Скоуп + +Issue #532: тройной `drop-shadow` внешнего контура плана (`.hp-paperg`) живёт в +общем композиционном слое с планом, поэтому любая перерисовка плана (ховер, +панорама) заново прогоняет три прохода размытия по габариту всего листа — +на машине владельца (Firefox) это ~9 к/с и 23 МБ текстур/кадр. Правка: +`will-change: filter` на правиле `.stage.daycycle .hp-paperg, +.hp-static-stage.daycycle .hp-paperg`, уводящее отфильтрованную бумагу в свой +слой. Побочный эффект контракта (К2, принят на спек-ревью r2): промоушен делит +SVG на два слоя, из-за чего диагональная штриховка стен антиалиасится иначе — +4 golden-кадра `day-cycle-{dawn,day,dusk,night}-dark` расходятся и подлежат +пересъёмке на кандидате беты, а не в этом коммите. + +Продуктовая рамка: View-режим — продукт для двух из трёх персон (`SCOPE.md`), +настенный планшет и телефон читают план именно там; ускорение растеризации +дневного цикла закрывает именно этот сценарий, палитру/геометрию/слои +окружения/блендинг не трогает (К3, К4 — вне скоупа, подтверждено кодом, см. +ниже). Продукт кода вне `plan.styles.ts` не тронут. + +Спек-ревью прошло два раунда (r1, r2 — оба зелёные); r2 переписал К2/AC3 после +того, как автор сам обнаружил на реализации, что «golden останется зелёным» не +подтвердилось, и вернул ТЗ на переработку вместо подгонки факта под старый +текст. Код-ревью (этот документ) — первый заход, полный разбор. + +## Как проверялось + +Дешёвые гейты (`tsc`, `npm test`, `npm run build` + сверка 3 копий бандла, +`check-docs.mjs`) подтверждены зелёным прогоном Validate на этом самом SHA +(https://github.com/Matysh/houseplan-card/actions/runs/34622197915) — повторно +не гонял, кроме случаев ниже, где перегон был нужен как побочный эффект. + +Гейты, которые я прогнал сам (а не поверил хендоффу): + +| Гейт | Команда | Результат | +|---|---|---| +| Смок-свидетель | `node demo/smoke_daycycle_raster.mjs` (после `npm run bundle:sync`, локальный ratio 0.73 — другая машина, тот же порядок, что у автора 0.76/0.82) | OK, 8/8 | +| Мутант | `node scripts/mutation-gate.mjs --id=daycycle-outline-not-promoted` | `тест покраснел, как обязан` — поймано 1 из 1 | +| Селектор смоков | `node scripts/smoke-select.mjs --base origin/dev --head HEAD` | НЕОПРЕДЕЛЁННОСТЬ (0 символов на изменённой строке) — совпадает с заявленным риском 5 | +| Полная golden-матрица | `node demo/golden/run.mjs --mode=verify` (169 сценариев) | ровно 4 `different`: `day-cycle-{dawn,day,dusk,night}-dark`; всё остальное `passed`, включая 7 сценариев, которые в хендоффе автора были помечены как «шум песочницы» — здесь (пиновый Chromium) они все зелёные | +| Мой собственный замер среднего цвета кадра по всем 4 кадрам (не только `night-dark`, как в хендоффе) | одноразовый скрипт через `createImageBitmap`/`getImageData`, baseline vs `artifacts/golden/actual/*`, удалён после прогона | `dawn` Δ−0.001, `day` Δ−0.014, `dusk` Δ+0.047, `night` Δ+0.080 из 255 — все четыре внутри заявленного допуска «не дальше 0.1» | +| Ограничивающий прямоугольник расхождения по всем 4 кадрам | тот же одноразовый прогон | `[120‑121, 36, 867, 757‑758]` — идентичен по всем четырём кадрам и совпадает с рамкой, которую автор привёл для `night-dark`; расхождение не задевает ореол снаружи плана | +| Три копии бандла | `diff -q dist/houseplan-card.js custom_components/houseplan/frontend/houseplan-card.js` (+ `houseplan-assets.json`) | идентичны | + +Не прогонял: `pytest tests_backend` (бэкенд не задет), `npm run invariants` +(диффа геометрии/модели/`layout`/`marker.space`/`open_spans` нет — правка +чисто в CSS), performance-профили Validate (не названы в AC), полный +`npm test`/`tsc`/`build` заново как отдельный гейт (уже зелёные на этом SHA +по Validate; `npm run build`+`bundle:sync` я всё равно выполнил как побочный +эффект, чтобы поднять локальный смок/golden — красных мест не дали). +`smoke_glow*`/`smoke_discovery_filters` не гонял: слабое совпадение по имени +(«свет», «фильтр»), но К3/К4 прямо объявляют слои окружения и блендинг вне +скоупа, а код-факт (`svgScreenBlendSupported` не читает `bg_mode`) это +подтверждает — смотреть не было необходимости. + +Рабочая копия оставлена чистой: все временные скрипты и `artifacts/golden/` +удалены после использования, `git status` пуст. + +## AC · чем доказан · чем краснеет (проверено самостоятельно) + +| # | AC | Чем доказан | Чем краснеет | Проверено | +|---|---|---|---|---| +| AC1 | Подсказка `will-change: filter` стоит только при `daynight`, статичный фон — без неё | `demo/smoke_daycycle_raster.mjs`, часть 1 | мутант `daycycle-outline-not-promoted` | сам прогнал: смок зелёный на коде, красный на мутанте | +| AC2 | Отношение растеризации на панораме ≤ 2.0 | тот же смок, часть 2 (CDP `RasterTask`) | тот же мутант (без декларации отношение ~15) | сам прогнал: 0.73 на коде, мутант красный | +| AC3 | Изменились ровно 4 кадра `day-cycle-*-dark`; расхождение — антиалиасинг штриховки, не изменение вида (ореол цел, средний цвет кадра сдвинут ≤0.1/255) | полная golden-матрица + разбор пикселей | 5-й кадр либо сдвиг среднего цвета | сам прогнал полную матрицу (169 сценариев): ровно 4 different; сам посчитал средний цвет и bbox расхождения для **всех четырёх** кадров (не только `night-dark`, как в хендоффе) — все внутри допуска, bbox идентичен по всем четырём | +| AC4 | Статичный фон не платит: ни `filter`, ни `will-change` | тот же смок | декларация без `.daycycle` | сам прогнал вместе с AC1 | +| AC5 | `docs/SUN.md` описывает промоушен как часть контракта; оба чейнджлога на месте | построчная сверка ревьюером | расхождение документа и кода | сверил построчно — обе строки чейнджлога и абзац SUN.md на месте, но см. находки ниже | + +AC3 закрыт по существу мной лично, а не на веру: спек-ревью r2 прямо +отметило, что автор экстраполировал порог сдвига цвета с одного разобранного +кадра (`night-dark`) на оставшиеся три и делегировало код-ревью обязанность +проверить это «на реальном прогоне». В хендоффе автора эта проверка +по-прежнему покрывала только `night-dark` количественно (три другие только +помечены `different` в таблице). Я прогнал полную матрицу и лично посчитал +средний цвет и bbox расхождения для `dawn`/`day`/`dusk`-dark — все три ведут +себя идентично `night-dark` (тот же bbox, тот же порядок сдвига цвета), так +что риск экстраполяции снят фактом, а не доверием. + +## Находки + +### Medium-1 (в скоупе) — чейнджлог утверждает то, что опровергает сам контракт задачи + +`docs/CHANGELOG.md` и `docs/CHANGELOG.ru.md` (новая запись в `## Unreleased`): + +> The picture is unchanged, to the pixel ([#532]). +> Картинка не изменилась ни на пиксель ([#532]). + +Это буквально неверно и противоречит К2/AC3 этой же задачи: 3.3–3.5 % +пикселей на каждом из четырёх кадров дневного цикла расходятся (максимальное +отклонение канала 81 из 255) — именно поэтому эталоны `day-cycle-*-dark` +пересматриваются отдельным коммитом на кандидате беты. Сообщение самого +коммита формулирует это точнее («the diagonal wall hatch anti-aliases +differently and the four day-cycle baselines are re-taken»), а публичный +чейнджлог — нет. + +**Воспроизведение:** сравнить текст `docs/CHANGELOG.md` с любым из четырёх +диффов golden-матрицы (`node demo/golden/run.mjs --mode=verify`, +`day-cycle-night-dark`: 26 442 / 770 640 пикселей выше `maxChannelDelta: 10`). + +**Правка:** перефразировать обе строки чейнджлога так, чтобы не утверждать +побайтовую идентичность — например, в духе текста коммита («картинка читается +так же; антиалиасинг штриховки стен смещается на несколько пикселей, эталоны +дневного цикла пересматриваются отдельно, см. #532»). + +### Medium-2 (в скоупе) — `docs/SUN.md` приписывает измерение в Chromium браузеру Firefox + +`docs/SUN.md`, новый абзац (AC5): + +> …without the hint every repaint of the plan re-ran three blur passes over +> the whole sheet, which cost **a Firefox window** about **fifteen times** +> the rasterization of a static background (#532). + +Отношение «пятнадцать раз» — это измерение мутанта/смока через CDP-трассировку +в headless **Chromium** (см. хендофф: «без правки… отношение 15.06»; тот же +порядок в моём собственном прогоне мутанта). Профиль владельца в Firefox дал +качественные цифры (23 МБ текстур/кадр, ~9 к/с, вклад drop-shadow 72–75 % от +стоимости растеризации), но никогда не давал коэффициент «×15» — сам документ +ТЗ и ревью r1/r2 явно фиксируют, что Gecko не профилировался этим свидетелем +(«Chromium — не Gecko… приговор по Gecko даёт профиль владельца»). Формулировка +SUN.md смешивает два источника числа под одной меткой, что противоречит +собственной дисциплине проекта «одно число — один источник». + +**Воспроизведение:** сверить абзац SUN.md с хендоффом/комментарием аналитики — +цифра 15× нигде не связана с Firefox, только с Chromium/CDP. + +**Правка:** заменить «a Firefox window» на «our Chromium/CI measurement» (или +эквивалент), не приписывая коэффициент профилю владельца. + +## Что проверено и корректно + +- Селектор и объявление `will-change: filter` в `src/styles/plan.styles.ts` + дословно совпадают с К1 (в т.ч. вторая ветка селектора `.hp-static-stage`); + комментарий над правилом даёт числа Chromium/CDP корректно (852 мс / 28 мс / + 108 мс) — в отличие от SUN.md, здесь атрибуция источника числа верна. +- `.hp-paperg` действительно группа только бумажных силуэтов + (`src/houseplan-card.ts:11534`, `_paperShapes(space.rooms)`), не меняется + при ховере/панораме — механика из ТЗ подтверждена чтением, не только словом + аналитика. +- `scripts/mutation-gate.mjs`: патч `daycycle-outline-not-promoted` находит + ровно ту строку, которая добавлена в диффе (сверено побайтово через `grep`), + мутация подтверждена прогоном. +- `demo/golden/matrix.mjs` содержит ровно 4 сценария с `bgMode: 'daynight'` — + структурно подтверждает и «К2 живёт только под `.daycycle`», и «ровно четыре + кадра» из AC3. +- `svgScreenBlendSupported` (`src/glow-blend.ts`) — рантайм-проба, не читающая + `bg_mode` — подтверждает К4 (блендинг света вне скоупа) кодом. +- `docs/images/screenshots.json`: изменился только `sourceFingerprint` + (отпечаток `src/**`), все 11 `imageSha256` не изменились — соответствует + заявлению «`npm run docs:accept -- --identical`», документация не поехала. +- Три копии бандла (`dist/`, `custom_components/houseplan/frontend/`) + побайтово идентичны; сборка воспроизводится (`npm run build` + + `bundle:sync` на этом дереве прошли чисто). +- Коммит несёт `Issue: #532` и `User-Visible: yes`; обе строки чейнджлога — в + том же коммите, что и поведение (не в отдельном). +- Продуктовый вопрос автора («приемлемо ли платить пересъёмкой 4 эталонов за + скорость») не новый: это ровно К2/AC3, уже разобранные и принятые зелёным + спек-ревью r2 с полным раскрытием факта (byte-for-byte ореол, сдвиг + среднего цвета, план пересъёмки с `Release:`/`Baseline-Reviewed:`). Код + реализует то, что было одобрено; переоткрывать это как продуктовое решение + на этапе код-ревью нет оснований — я лишь перепроверил цифру на всех + четырёх кадрах вместо одного (см. AC3 выше). +- Риск 5 из ТЗ (`smoke-select` не находит связь) подтверждён независимо: + тот же вывод «НЕОПРЕДЕЛЁННОСТЬ» на моём прогоне, что и в хендоффе. + +## Чего не проверял + +- `pytest tests_backend` — диффа в `custom_components/**/*.py` нет. +- `npm run invariants` — диффа геометрии/модели (рёбра, `layout`, + `marker.space`, `open_spans`) нет, только CSS-свойство. +- Performance-профили Validate (`performance_smoke` и т.п.) — не названы в + AC, диффа в этих путях нет. +- Приговор по настоящему Firefox (профиль владельца до/после) — вне + возможностей код-ревью; ТЗ явно оставляет это отдельным шагом суждения + владельца, а не AC этой задачи. +- Полный `smoke_*` матрица (244 файла) — не прогонял целиком; `smoke-select` + честно вернул «неопределённость», просмотрел список смоков с похожими + именами (`glow*`, `discovery_filters`) и решил не гонять — К3/К4 прямо + выводят эти поверхности из скоупа, и код (`svgScreenBlendSupported`) это + подтверждает. + +--- + +**Материал раунда:** SHA `bb04f8605f9003c0ec6643359fa47af9319247bc`, +`origin/dev..HEAD` = 1 коммит, диапазон диффа `origin/dev...HEAD` (53 файла, +включая 3 копии бандла и golden-fingerprint документации). Первый заход — +раздела «Унаследовано из r0» нет. + +--- + + + +## Материал раунда + +- Ветка: `issue/532-daycycle-raster`, коммит `bb04f8605f90` — ребейз его осиротит, и это нормально: ниже якоря, которые ребейз не меняет. +- Дерево материала: `959371418fa5d512b6314804fb4d9cf13e86efe4` + ``` + git log --all --format='%H %T' | grep 959371418fa5 + ``` +- Тело issue: `12e66588332b94c78100156f01e4832881b8163fb6f8fdca83acfcb8e90f4545` +- Вердикт конвейера: `yellow` · High 0