mirror of
https://github.com/Matysh/houseplan-card
synced 2026-09-28 19:01:34 +00:00
claude[bot]
@@ -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 нет.
|
||||
|
||||
---
|
||||
|
||||
<!-- material-anchors: сгенерировано конвейером (#414) -->
|
||||
|
||||
## Материал раунда
|
||||
|
||||
- Ветка: `issue/669-stairs-lazy-area`, коммит `47c245b5ba34` — ребейз его осиротит, и это нормально: ниже якоря, которые ребейз не меняет.
|
||||
- Дерево материала: `6cfd6c813548a1667d67043ee4a2f3eac78bb7ce`
|
||||
```
|
||||
git log --all --format='%H %T' | grep 6cfd6c813548
|
||||
```
|
||||
- Тело issue: `1325d6906ba9cf19798f52282b54bf435c4603185b0a323e8a7a5ef01204ae9c`
|
||||
- Вердикт конвейера: `green` · High 0
|
||||
Reference in New Issue
Block a user