diff --git a/docs/reviews/CODE-REVIEW-778-r1.md b/docs/reviews/CODE-REVIEW-778-r1.md new file mode 100644 index 00000000..5d8a867c --- /dev/null +++ b/docs/reviews/CODE-REVIEW-778-r1.md @@ -0,0 +1,194 @@ +# CODE-REVIEW-778-r1 + +Материал: `68fc5d1f4b3570bedf6955fbb66612b99c7c4d55` (HEAD, `origin/dev..HEAD` = один коммит). +Трек: show. Заход r1. Блокирующих циклов израсходовано 0 из 2 (до этого вердикта). + +## Скоуп + +Задача #778 — инфраструктурное расследование нестабильного превышения +`longTask.editorSeries.maxSingleMs` (163 мс против потолка 150 мс на одном +Validate-прогоне #762). Предложенная автором работа (тело issue, раздел +«Предлагаемая работа») — три пункта: + +1. снять сопоставимую base/candidate серию, явно сохранить SHA/окружение/ + распределение, отличить шум раннера от реальной синхронной работы; +2. разобрать длинную задачу ресайза на фазы (preflight, публикация, DOM/paint, + GC), проверить, что окно и календарь событий воспроизводят ввод пользователя; +3. если найден дефект измерения — исправить его с отрицательным свидетелем; + если продуктовая задержка — вынести конкретную оптимизацию в продуктовую + задачу. Не убирать из окна реальную работу, не поднимать лимиты. + +Диапазон правок — только класс B (`demo/**`, `test/**`), `src/**` не тронут: +`demo/benchmark_large_house.mjs`, `demo/performance/resize-attribution.mjs` +(новый), `demo/performance/card-contract.mjs`, `demo/performance/README.md`, +`test/performance-contract.test.mjs`, `test/performance-resize-attribution.test.mjs` +(новый). Трейлеры коммита: `Issue: #778`, `User-Visible: no` — корректно для +правки только харнесса измерения, без изменения продукта или бюджетов. + +## Как проверялось + +- Прочитан диапазон `git diff origin/dev...HEAD` целиком (6 файлов, 423/−2). +- Прочитано тело issue #778 и оба комментария автора (взятие в работу, + итоговый комментарий с выводом и списком изменений). +- Прочитан `docs/SCOPE.md`, `AGENTS.md`, `docs/process/REVIEWER.md`. +- Код `attributeResizeLongTask` разобран вручную построчно и пересчитан на + всех трёх позитивных юнит-кейсах теста (первый CI-образец 135 мс, задача без + шага ресайза, задержка вне тайм-вызовов) — результаты функции совпали с + ручным пересчётом по формулам модуля, расхождений не найдено. +- Прочитан `demo/benchmark_large_house.mjs` вокруг `startResizeAttribution` + (строки ~884–978): подтверждено, что `card._resize` уже существует к моменту + обёртки (создаётся `_rszEdgeDown` до неё), что обёртка ставится и снимается + строго вокруг окна, что восстановление `move` учитывает `hasOwnProperty` + (не плодит метод на прототипе), и что окно Long Task открывается уже после + обёртки (как заявлено в комментарии кода). +- Прочитан `card-contract.mjs`: новая ветка `optionalMethodsOf` пропускает + отсутствующий `_resize` и отсутствующий `move` целиком (отличает «нет + владельца» от «владелец без метода»), красит только метод неправильного + типа — соответствует описанию в README и тесту. + +## Гейты: что прогнано и почему + +- **`npx tsc --noEmit`, `npm test` (полный), `npm run build` + сверка бандла — + не прогонялись мной.** Validate на этом же SHA `68fc5d1f` зелёный + (https://github.com/Matysh/houseplan-card/actions/runs/37550065681) — дешёвые + гейты уже подтверждены этим прогоном, повторный прогон ничего не добавил бы. +- **Точечно прогнано мной** (дёшево, для собственной уверенности, не замена + Validate): `node --test test/performance-resize-attribution.test.mjs + test/performance-contract.test.mjs` — 16/16 зелёных. +- **Негативный свидетель проверен лично.** Внёс ручную мутацию в + `demo/performance/resize-attribution.mjs` (`task.duration += 50` после + `if (!task) return empty;`), прогнал + `test/performance-resize-attribution.test.mjs` — 3 из 4 тестов покраснели + (`parts add up`, «без шага ресайза», «задержка вне тайм-вызовов»; тест на + `supported:false` не затронут мутацией — ожидаемо). Файл восстановлен из + копии, `git status` чист. Это и есть требуемое «тест умеет падать» для + AC этой задачи — для чистого юнита, без продукта и без дорогого гейта, + мутант в `scripts/mutation-registry.mjs` не обязателен (`PROCESS.md` о + защитном AC: «для чистых юнитов достаточно отрицательного случая в самом + тесте»), и тест действительно содержит отдельные негативные кейсы (задача + без шага ресайза, отсутствие Long Task/spans). +- **Смоки, golden, pytest, инварианты геометрии, performance (AC-named) — не + прогонялись и не нужны.** Изменения не трогают `src/**`, никакой геометрии, + рендера или Python-кода; AC задачи не называют конкретный смок. `ci:full` + уже стоит на issue — Full Performance увидит новое поле + `interactionDiagnostics.resizeLongTask` в отчёте по строке CI, это вне + ревью кода (сама задача просит не гонять полные наборы ради ревью). +- **Я не прогонял Full Performance и не проверял реальный CI-образец** — + это дорогой гейт, задача явно не требует от ревью его повторения; численные + утверждения README (202 образца, распределение по раннерам) проверены + чтением текста и логики кода, не повторным сбором серии — это диагностика + инфраструктурной задачи уровня анализа данных, а не защищаемый продуктом AC. + +## Находки + +### Medium (в скоупе — AC п.3, вторая ветка не закрыта) + +Собственная постановка задачи (issue, «Предлагаемая работа», п.3) требует: +*«если продуктовая задержка — вынести конкретную оптимизацию в продуктовую +задачу»*. Итоговый комментарий автора называет конкретные продуктовые причины +длинной задачи резайза: + +> «`_rszEdgeLabels` → `floorMinusBodies` дважды за шаг заново объединяет тела +> этажа, ~24 % задачи; живой preflight `wallBodiesGeometry` — ~70 %, из них +> BigNumber polyclip-ts — ~29 % self time» + +— то есть продуктовая задержка **найдена**, а не исключена (вывод «нет +дефекта измерения» относится к дефекту *измерения*, не к вопросу о наличии +продуктовой задержки вообще). Но ссылка на её разбор — «Продуктовые причины — +в документе находок волны» — не резолвится ни в материале ревью (только +`demo/**`/`test/**`, никакого нового документа), ни в репозитории: поиск по +`wallBodiesGeometry`, `floorMinusBodies`, `polyclip-ts`, `rszEdgeLabels` не +находит такого документа ни в `docs/`, ни в `docs/reviews/`, ни в открытых +issues (проверены issues, созданные 2026-10-01…2026-10-06, включая #789 — +не про это; никакого issue про оптимизацию preflight ресайза после #778 не +заведено). Формулировка «находок волны» также не совпадает с принятым в этом +репозитории словоупотреблением «волна» (многоэтапные ТЗ вроде #680/#677, не +имеющие отношения к #778). + +**Воспроизведение:** `gh issue list --repo Matysh/houseplan-card --search +"polyclip OR wallBodiesGeometry OR floorMinusBodies OR rszEdgeLabels" --state +all` и `git grep -l "wallBodiesGeometry\|floorMinusBodies\|polyclip-ts" docs/` +— ни один результат не указывает на задачу, выносящую найденную продуктовую +причину в работу. + +**Почему это Medium, а не Low.** Это невыполненный пункт AC задачи, а не +бухгалтерия: трек show прямо называет «невыполненный AC» критерием Medium. +Без видимого адресата находка рискует остаться только прозой в закрытом +issue — ровно то, против чего написан п.3 ТЗ («вынести… в продуктовую +задачу», а не «упомянуть»). + +**Что нужно для зелёного:** либо автор называет SHA/ссылку на уже +существующий документ находок (и я его перечитаю), либо заводит отдельное +issue с конкретной оптимизацией (`wallBodiesGeometry`/preflight, +`floorMinusBodies`/`_rszEdgeLabels`) и ссылкой на #778, либо правит +формулировку итогового комментария, если это не было фактическим +обязательством (но тогда п.3 ТЗ не закрыт и это нужно явно признать). + +Находка в скоупе #778 — отдельный issue не завожу. + +## Что проверено и корректно + +- Нет изменений в `src/**`, бюджетах, окнах измерения или лимитах — соответствует + запрету «не поднимать лимиты ради зелёного CI» и классу B (не A). +- `attributeResizeLongTask` (новый модуль) — чистая функция, независимо + пересчитана вручную на всех заявленных кейсах, расхождений с кодом нет. + Разделение на `preflightMs`/`projectOtherMs`/`publishMs`/`labelsMs`/ + `moveOtherMs`/`updateMs`/`otherMs` корректно не задваивает preflight, + вложенный в `project`, и не приписывает фазы задаче, в которой нет шага + ресайза (`geometryMoves: 0` ветка проверена тестом и пересчётом). +- Обёртка `ResizeController.move` в раннере ставится и снимается вокруг ровно + одного окна; «неизвестная форма» (`project`/`publish`/`measure` не функции) + репортится как `supported:false` с причиной, а не считается нулевыми + долями — проверено чтением и тестом `performance-contract.test.mjs`. +- `card-contract.mjs`: `optionalMethodsOf` — новый необязательный метод + контракта, не ломает прохождение базовой сборки v1.68.1 (`_rszDrag`, без + `_resize`) и красит только случай «метод есть, но не функция» — доказано + `assert.doesNotThrow`/`assert.throws` в тесте. +- Защитные AC (неизвестная форма move; невалидный тип необязательного метода) + доказаны негативными кейсами в самом тесте — для чистых юнитов без + дорогого гейта этого достаточно, реестр мутантов не требуется. +- Трейлеры `Issue: #778` и `User-Visible: no` на коммите корректны: изменение + не видно пользователю продукта, changelog не требуется. +- `demo/performance/README.md` корректно описывает состав поля + `interactionDiagnostics.resizeLongTask` и логику чтения следующего отказа — + текст совпадает с кодом (проверено построчно). + +## Чего не проверял + +- Полные `tsc`/`npm test`/`npm run build`+bundle — не гонял: Validate уже + зелёный на этом SHA (см. «Гейты» выше), бюджет раунда не трачу на повтор. +- Сами 202 исторических CI-образца (числа из прошлых прогонов Full + Performance/Validate, которые пересказывает README) — не пересчитывал + заново из артефактов CI; доверяю им как документированному историческому + анализу вне материала этого коммита (они не являются кодом, который может + «покраснеть»). +- Поведение в реальном браузере (Long Animation Frames, Chrome-трассировка, + профиль GC) — не воспроизводил; это заявлено автором как ручная локальная + диагностика (Chromium 141), не автоматизированная часть диапазона ревью, и + honest-метка «проверено чтением, не исполнением» применяется к этой части. +- `ci:full` (Full Performance на ветке задачи) — ещё не обязан быть виден на + момент этого ревью; его не запускал и не ждал, задача сама пометила issue + этой меткой для последующего прогона. +- Golden/pytest/инварианты модели — не прогонял: диапазон не трогает рендер, + Python-бэкенд или геометрию модели. + +## Вердикт + +Один High отсутствует, один Medium в скоупе (невыполненный пункт AC — +эскалация продуктовой причины). Жёлтый вердикт возвращает задачу автору; +цикл расходуется (1 из 2 на track:show). + +--- + + + +## Материал раунда + +- Ветка: `issue/778-resize-longtask-attribution`, коммит `68fc5d1f4b35` — ребейз его осиротит, и это нормально: ниже якоря, которые ребейз не меняет. +- Дерево материала: `2c52f7e78071c2afaf88614dbe7558783c84c230` + ``` + git log --all --format='%H %T' | grep 2c52f7e78071 + ``` +- Тело issue: `c5a06e8f343baa4956489a535a51db06cfaee2bc8b1e55d26e4996b42021afb5` +- Вердикт конвейера: `yellow` · High 0 · маршрут `fix` +