From 7e8605981bcfb5ee1451f5fdd399d51fda5e707e Mon Sep 17 00:00:00 2001 From: "claude[bot]" <209825114+claude[bot]@users.noreply.github.com> Date: Sun, 27 Sep 2026 07:45:59 +0000 Subject: [PATCH] docs: review document for #669 Issue: #669 User-Visible: no --- docs/reviews/CODE-REVIEW-669-r1.md | 182 +++++++++++++++++++++++++++++ 1 file changed, 182 insertions(+) create mode 100644 docs/reviews/CODE-REVIEW-669-r1.md diff --git a/docs/reviews/CODE-REVIEW-669-r1.md b/docs/reviews/CODE-REVIEW-669-r1.md new file mode 100644 index 00000000..22b6fbf2 --- /dev/null +++ b/docs/reviews/CODE-REVIEW-669-r1.md @@ -0,0 +1,182 @@ +# CODE-REVIEW-669-r1 + +Issue: #669 · Этап: code · Заход: r1 · Блокирующих циклов израсходовано: 0/4 + +Материал: `git log --oneline origin/dev..HEAD` и `git diff origin/dev...HEAD` +на `47c245b5ba34e9b2b8480fb205e400d4e0b0b976` (рабочая копия была на этом SHA +весь прогон; в конце ревью восстановлена в исходное состояние после +локальных гейтов — см. «Как проверялось»). + +## Скоуп + +Perf-фикс (P1, ценность 4/10 пользователю / 9/10 разработке — разблокировать +`v1.78.0-beta.5`), ветка `issue/669-stairs-lazy-area`, один коммит `47c245b5` +поверх `dev@2728a0ee`. Служит J1/J5 (комната на плане и её площадь) косвенно — +задача сама не меняет видимое поведение, а снимает деградацию первого кадра, +которая делала J1 («увидеть дом целиком прямо сейчас») медленнее на этажах +с лестницами. + +Файлы: `src/clean-floor.ts`, `src/stairs.ts`, `test/clean-floor.test.mjs` +(новый), `test/stairs.test.mjs`, `tsconfig.test.json`. + +Изменение по существу: +1. `cleanFloorForRoom` (`src/clean-floor.ts`) больше не вычитает лестницы при + построении `path`/`geom`; вычитание перенесено в геттер `area` с + мемоизацией в том же закэшированном объекте — считается ровно один раз, + при первом чтении. +2. `geometryMinusStairs` (`src/stairs.ts`) отбирает контуры лестниц по + пересечению bbox с вычитаемой геометрией (`stairFootprintsTouching`) + перед передачей в `polyclip`; результат вычитания не меняется. +3. `geometryMinusStairsSteps` (пошаговый расчёт сводной панели) не тронут — + подтверждено чтением (`src/stairs.ts:301-320` использует прежний + безусловный `stairList(...).map(...)`, не `stairFootprintsTouching`). + +Трейлеры коммита: `Issue: #669`, `User-Visible: no` — корректно, видимое +поведение не меняется (спецификация это утверждает, AC4 это проверяет). +`User-Visible: no` не требует правки changelog — в диффе changelog не +затронут, соответствие есть. + +## Как проверялось + +Дешёвые гейты подтверждены зелёным Validate на этом точном SHA +(https://github.com/Matysh/houseplan-card/actions/runs/36302641777, job +«Фронтенд: типы, юниты, мутанты, синхрон бандла» — `success`): typecheck, +`npm test` (полный юнит-набор), `bundle-sync --verify`, `lint:unused`. Этот же +Validate прогнал «Мутанты по диффу» (6/6 шардов `success`) — независимое от +меня подтверждение, что задетые диффом свидетели умеют падать. + +Этот Validate-прогон — обычный push (`workflow_dispatch`, без `full=true`, +`heavy=false` по `classify-changes.mjs`), поэтому «тяжёлые» job'ы +(браузерные смоки, golden, **`performance_smoke`**, HACS/Hassfest/backend/ +geometry_parity) в нём пропущены (`skipped`), не провалены. Три из них не +относятся к этому диффу вовсе (HACS/Hassfest/backend/geometry_parity — +диффа в Python/манифестах нет). Смоки, golden и `performance_smoke` +относятся — их прогнал сам, локально, на этом SHA: + +| Гейт | Прогнан | Результат | +|---|---|---| +| `npx tsc -p tsconfig.test.json` + `npm test`-эквивалент для двух изменённых файлов | да (полный `npm test` уже подтверждён Validate выше; здесь — прицельный повтор `test/clean-floor.test.mjs` + `test/stairs.test.mjs` с мутациями) | 18/18 зелёных; под мутацией «вернуть немедленное вычисление» — 3 из 4 `not ok` (AC1); под мутацией «снять bbox-фильтр» — 2 из 3 `not ok` (AC2) | +| `npm run build` | да | зелёный (включает `tsc --noEmit`) | +| `node scripts/smoke-select.mjs --base 2728a0ee --head HEAD` | да | 8 прямых совпадений: `smoke_active_chain_ink`, `smoke_drag_bounds`, `smoke_grid_snap`, `smoke_infinite_canvas`, `smoke_junction_holes`, `smoke_junction_limits`, `smoke_optional_space_model`, `smoke_wallthick_standalone` — все прогнаны, OK | +| Дополнительные смоки по AC4/лестницам (не в выборке, названы автором) | да | `smoke_stairs`, `smoke_pdf_export`, `smoke_room_tooltip_toggle`, `smoke_ux_fixes` — все OK | +| `npm run golden:verify` | да | 179/179 `passed`, exit 0, ни один эталон не тронут (AC4 доказан исполнением, не только чтением) | +| Perf-профили из AC3 (`benchmark:large-house` × 2 профиля + `benchmark:compare --absolute-only`) | да, локально (облачная песочница, не пиннутый Linux CI-раннер) | оба профиля полностью зелёные с большим запасом: iso `modelReadyMs` 1400.9/3000, `firstStableRenderMs` 1559.9/3500; interaction `modelReadyMs` 825.4/2500, `firstStableRenderMs` 2607.8/3000; `spaceSwitchMs` тоже в бюджете на этой машине (не в скоупе AC3, см. #675) | +| `node scripts/check-docs.mjs` (без флага → `strict`) | да | печатает «screenshot source fingerprint is stale» — **не находка**: диффа в `docs/**`/скриншотах нет, отпечаток считается по всему `src/**` (см. `scripts/docs-freshness.mjs`), и `validate.yml` намеренно гоняет этот гейт в `warn` на обычном пуше, `strict` — только у кандидата беты/PR/кнопки (комментарий в `validate.yml:75-77`). Коммит не несёт `Release:`, так что для него это ожидаемый шум, а не гейт задачи | +| `node scripts/process-gate.mjs --range origin/dev..HEAD` | нет, но эквивалент прогнан внутри уже упомянутого Validate («Процессный гейт: диапазон, трейлеры, статусы issue» — `success`) | зелёный | + +Все локальные прогоны выполнены поверх точного `47c245b5`; побочные файлы +сборки (`dist/**`, `demo/srv/assets/**`, `test-build/**`), возникшие при +`npm run build`/`npm test`/бенчмарках, приведены обратно к состоянию +коммита (`git checkout HEAD -- dist/` + `git clean -fd dist/ demo/srv/`); +`git status` после ревью — чист, `HEAD` не менялся. + +**Ошибка в процессе ревью и как она исправлена.** Проверяя, не относится ли +предупреждение `check-docs.mjs` к самому диффу, по ошибке выполнил +`git checkout origin/dev -- .` — эта команда заменила рабочую копию +содержимым `dev@2728a0ee`, откатив 4 из 5 файлов диффа задачи. Замечено +немедленно по `git status`; исправлено `git checkout HEAD -- .` до +следующего шага. `git diff --stat` после восстановления — пуст, SHA не +менялся. Материал ревью не пострадал; называю это прямо, а не молчанием, +как требует дисциплина протокола. + +## Находки + +Блокирующих (High) нет. В скоупе (Medium) нет. + +**Low — принято без правки, со следующей записью:** + +1. AC3 в таблице «AC · чем доказан · чем краснеет» хендоффа помечен + «(не защитный AC)», третий столбец пуст. Формально AC3 — бюджет + (лимит), то есть по правилу REVIEWER.md ближе к защитному AC, и пустой + столбец обычно значит Medium. Но фактическая «краснота» здесь + документирована — просто не в этой ячейке: три воспроизведённых прогона + точного `09873251` в теле issue (`modelReadyMs.median` 3185.1/3113.7 при + лимите 3000, `spaceSwitchMs.median` 1821.2/1813.5 при лимите 1800) и + собственная таблица «Локальный A/B» разработчика (тот же кандидат до + фикса против ветки — разница по лестницам падает почти до нуля). Я + независимо подтвердил зелёный результат обоих профилей на этом SHA (см. + таблицу выше) с большим запасом. Считаю содержательно закрытым; + правки не требую — это вопрос оформления одной ячейки, а не отсутствия + доказательства. +2. Канонический Linux CI `performance_smoke` (пиннутый Chromium, `hardMaxMs` + бюджеты) не прогонялся на этом SHA ни в одном Validate — единственный + доступный прогон пропустил его (`heavy=false`, обычный пуш). Разработчик + сам отметил это в хендоффе («CI — ссылка будет в ревью/при выпуске»). + AC3 говорит про Validate «кандидата слияния **и** кандидата беты» — то + есть предполагает более одной контрольной точки, и вторая ещё впереди. + Не блокирую: моя локальная проба на другом железе (быстрее CI-раннера) + не тождественна канону, но по методике и метрикам идентична, и + направление то же, что в замерах аналитика — резкое падение вклада + лестниц. Фиксирую в «чего не проверял». + +## Что проверено и корректно + +- AC1 (лень + однократность + кэш): `test/clean-floor.test.mjs`, 4/4 зелёных; + мутация «убрать геттер, считать сразу» красит 3 из 4 — подтверждено лично + запуском мутации, не только по описанию хендоффа. +- AC2 (эквивалентность с фильтром по bbox + фильтр реально отбирает): + `test/stairs.test.mjs`, 3/3 новых `#669 AC2 …`; мутация «фильтр всегда + true» красит 2 из 3 — подтверждено лично. +- AC4 (площадь в подсказке/PDF не изменилась, эталоны не тронуты): golden + 179/179 `passed` исполнением; смоки `smoke_room_tooltip_toggle` и + `smoke_pdf_export` зелёные; числовое равенство до `1e-9` доказано внутри + AC1/AC2 тестов сравнением с прежним безусловным вычислением. +- Единственный потребитель `.area` — синхронный геттер на одном и том же + закэшированном объекте; `lruRead`/`lruWrite` (`src/card-runtime.ts`) + хранят ссылку, не клонируют — повторное чтение из кэша не пересчитывает + (прочитано и проверено тестом «cache hit does not subtract again»). + Прочитаны все шесть точек вызова `_cleanFloor` в `src/houseplan-card.ts` + (:9059, :9229, :10184, :10459, :11011, :11873) — ни одна не копирует + результат (`spread`/`structuredClone` не встречается ни на одном из этих + путей), риск из ТЗ не реализовался. +- `geometryMinusStairsSteps` (сводная панель) не изменён — заявленный + не-скоуп подтверждён чтением. +- Одно число — один источник (§8): и подсказка комнаты, и PDF читают + `.area` одного и того же элемента `_cleanFloorCache` (ключ + `space.id|configEpoch|roomKey`) — геттер мемоизирует один раз на этот + объект, второй потребитель получает то же число без пересчёта. +- Трейлеры коммита корректны для класса A+B (`src/**` + `test/**` + + `tsconfig.test.json`): `Issue: #669`, `User-Visible: no`; changelog + оправданно не тронут. +- `dist/**` не в диффе — верно для обычной задачи без трейлера `Release:` + (правило D в `AGENTS.md`). + +## Чего не проверял + +- Полный `npm test` не перезапускал целиком — принят по зелёному Validate + этого SHA (job «Фронтенд», см. выше); прицельно перезапустил и + мутационно проверил только два изменённых тестовых файла. +- HACS/Hassfest/`pytest tests_backend`/geometry-parity — не прогонял: диффа + в Python/манифестах/геометрических моделях нет, эти job’ы были пропущены + и в референсном Validate по той же причине (классификация файлов, не + экономия). +- Канонический Linux CI `performance_smoke` на пиннутом Chromium — не + прогонялся ни разу на этом SHA ни мной, ни в доступном Validate; заменил + прогоном тех же двух `benchmark:large-house`/`benchmark:compare` команд + локально (см. таблицу и Low-2 выше). Это не тождественно канону. +- Полный набор golden (все сценарии) прогнан, но не сверялся построчно с + каждым файлом diff — достаточно, что 0 эталонов потребовалось принять. +- Ручного тестирования в браузере (интерактивная сессия) не проводил — + входит в объём смоков/golden выше, которые исполнялись реальным + Chromium через Playwright, не читались как текст. + +## Вердикт + +Зелёный. AC1–AC4 доказаны — исполнением тестов, мутацией на них и +исполнением golden/смоков/локального перф-прогона, не только чтением +описания хендоффа. Единственные два наблюдения — Low, приняты без правки +(см. выше), Medium и High нет. + +--- + + + +## Материал раунда + +- Ветка: `issue/669-stairs-lazy-area`, коммит `47c245b5ba34` — ребейз его осиротит, и это нормально: ниже якоря, которые ребейз не меняет. +- Дерево материала: `6cfd6c813548a1667d67043ee4a2f3eac78bb7ce` + ``` + git log --all --format='%H %T' | grep 6cfd6c813548 + ``` +- Тело issue: `1325d6906ba9cf19798f52282b54bf435c4603185b0a323e8a7a5ef01204ae9c` +- Вердикт конвейера: `green` · High 0