mirror of
https://github.com/Matysh/houseplan-card
synced 2026-10-04 21:58:56 +00:00
claude[bot]
@@ -0,0 +1,235 @@
|
||||
# CODE-REVIEW-585-r1
|
||||
|
||||
Материал: `8769905b4e0e872ef284570452924965a6b25c9a` (`issue/585-iso-overlay-boundary-search`), диапазон `origin/dev..HEAD` — 15 коммитов.
|
||||
Заход r1, блокирующих циклов израсходовано 0 из 4.
|
||||
|
||||
## Скоуп
|
||||
|
||||
Задача #585 чинит регрессию #583: в скрытом 2.5D-виде (`hp_alpha`) грубая решётка
|
||||
4 CSS px поиска свободного места для маркеров устройств/замков теряет разрешимую
|
||||
щель, если она уже 4 px — в одной из 12 изометрических golden-сцен два маркера
|
||||
устройства визуально сливаются. ТЗ требует заменить решётку/полный скан диска на
|
||||
детерминированный поиск по границам запрещённых областей (O(периметра) вместо
|
||||
O(площади)), сохранив все существующие точные проверки, и вернуть временно
|
||||
расширенные performance-допуски двух изометрических профилей к исходным
|
||||
150/60/75.
|
||||
|
||||
Диапазон изменений строго соответствует заявленному скоупу: только
|
||||
`src/iso-overlays.ts` (алгоритм) и `src/iso-scene-render.ts` (минимальное
|
||||
подключение кеш-хинта, как разрешено §13 ТЗ), плюс тесты, golden одной сцены,
|
||||
два performance-budget файла, канонические доки и сгенерированный `dist`/
|
||||
`custom_components/.../frontend`. i18n, конфиг, `CHANGELOG*` не тронуты — везде
|
||||
`User-Visible: no`, что верно для скрытого экспериментального вида.
|
||||
|
||||
## Как проверялось
|
||||
|
||||
1. Прочитано полное ТЗ из тела issue #585 и оба предыдущих комментария
|
||||
(аналитика, зелёный spec-review r1).
|
||||
2. Прочитан весь `src/iso-overlays.ts` (1648 строк) и diff
|
||||
`src/iso-scene-render.ts` целиком; прослежена последовательность
|
||||
промежуточных коммитов (`6f226a5b` → `df836c81`), каждый из которых чинит
|
||||
конкретный найденный на golden/perf прогонах дефект предыдущего шага
|
||||
(кэш-хинт как точная граница, а не досрочный успех; сохранение
|
||||
утверждённого tie-break; раскрытие границы комнаты только по свидетелю).
|
||||
3. Прочитан весь diff `test/iso-overlays.test.mjs` (7 новых тестов) и сверен
|
||||
с AC1/AC3/AC4.
|
||||
4. Прочитан diff `test/performance-budget.test.mjs` и обоих
|
||||
`demo/performance/budgets-*.json` — допуски 150/60/75 и неизменные
|
||||
`hardMaxMs`/`maxRegressionRatio` (2200/600, 0.2) подтверждены построчно.
|
||||
5. Прочитан diff `docs/ISOMETRIC.md`, `docs/ARCHITECTURE.md`, `docs/STATUS.md`.
|
||||
6. Проверен единственный изменившийся golden (`demo/golden/baselines/
|
||||
baselines-index.json` — построчный diff показывает ровно одну изменившуюся
|
||||
запись сцены, остальные 105 хэшей не тронуты) и сам PNG прочитан визуально:
|
||||
в Room 2.5 видны два раздельных (хоть и близко стоящих) маркера вместо
|
||||
одного слитого пятна — соответствует ожиданию AC2.
|
||||
7. Независимо проверено выполнение AC6 (Linux CI, exact-SHA, полные профили):
|
||||
`gh run list --workflow=performance.yml` нашёл прогон `35477092908`
|
||||
(`workflow_dispatch`, `headSha=8769905b…`, `conclusion=success`); в логах
|
||||
job `isometric` и `isometric-stage3` — таблицы `benchmark:compare` с
|
||||
`candidate-sha=8769905b…`, все строки `✅`, лимиты посчитаны от
|
||||
восстановленных 150/60/75 (например `panZoomMs` isometric: candidate 256.1
|
||||
≤ limit 261.9). Это ни автор, ни spec-review не привязали ссылкой нигде —
|
||||
см. находку ниже.
|
||||
8. `node scripts/smoke-select.mjs --base origin/dev --head HEAD` — см. раздел
|
||||
про смоки.
|
||||
9. `git diff origin/dev...HEAD --stat` полностью просмотрен: подтверждено, что
|
||||
не тронуты `custom_components/**/*.py`, i18n, `CHANGELOG*`, конфиг/схема.
|
||||
|
||||
## Находки
|
||||
|
||||
### Medium (в скоупе, чинится в этой же ветке) — устаревшая строка docs/STATUS.md и отсутствующий хендофф evidence
|
||||
|
||||
`docs/STATUS.md` был обновлён один раз, коммитом `6f226a5b` (22:50, самое
|
||||
начало работы над задачей) и с тех пор не трогался. Текущая строка снапшота
|
||||
гласит:
|
||||
|
||||
> «In implementation: #585 replaces the hidden 2.5D overlay's lossy 4 px
|
||||
> lattice with iterative critical-boundary events, restores the dense Room 2.5
|
||||
> marker pair and returns both full isometric profiles to their ordinary
|
||||
> performance allowances; **Linux golden/performance evidence is still
|
||||
> required before merge**.»
|
||||
|
||||
На момент этого код-ревью (материал `8769905b`) полный Linux performance-прогон
|
||||
на точном SHA уже существует и зелёный (run `35477092908`, см. пункт 7 выше),
|
||||
golden уже принят (`Baseline-Reviewed` в `86dffbc7`). Утверждение «evidence is
|
||||
still required» больше не верно, а в самой задаче нет комментария с этими
|
||||
ссылками — ни ссылки на `35477092908`, ни на прогон golden в финальном виде не
|
||||
попали в хендофф issue (три существующих комментария — аналитика, зелёный
|
||||
spec-review, «Взял: агент»). Это прямо расходится с ТЗ §12.5 («Запустить оба
|
||||
полных изометрических performance-профиля на точном SHA кандидата и сохранить
|
||||
ссылки/run ids в handoff») и AC8 («канонические документы... описывают...
|
||||
снятие временного performance-исключения» — снятие исключения не отражено как
|
||||
свершившийся факт).
|
||||
|
||||
**Почему это Medium, а не Low:** AC7 требует «перед передачей на код-ревью
|
||||
приложены требуемые targeted golden/performance evidence» — формально это не
|
||||
выполнено, я нашёл и проверил прогон сам, а не по ссылке автора. AC8 явно
|
||||
называет `docs/STATUS.md` файлом, который должен отражать актуальный статус;
|
||||
сейчас он говорит будущему читателю, что доказательство ещё не получено, хотя
|
||||
оно уже есть и зелёное. Это не блокирует функциональность (сам фикс работает и
|
||||
доказан), но воспроизводит ровно тот сценарий, о котором предупреждает процесс
|
||||
ревью: «Verified без названной команды и её результата доказательством не
|
||||
является» — только тут наоборот, доказательство есть, но не названо нигде,
|
||||
кроме как найдено ревьюером постфактум.
|
||||
|
||||
**Что нужно поправить:** одним коммитом в этой же ветке —
|
||||
1. обновить строку `docs/STATUS.md`, отразив, что фикс завершён и оба полных
|
||||
профиля прошли на точном SHA (со ссылкой на run);
|
||||
2. оставить в issue #585 комментарий с ссылками на `35477092908` (performance)
|
||||
и на golden-прогон/baseline-review, как требует §12.5 ТЗ — это же служит
|
||||
базой для «дисциплины хендоффа», на которую опирается сам процесс ревью.
|
||||
|
||||
## Проверено и корректно
|
||||
|
||||
- **AC1 (узкая щель, unit):** `group collision finds a legal one-pixel slit
|
||||
between the coarse nodes` (`test/iso-overlays.test.mjs:139`) — геометрия,
|
||||
где старая решётка 4 px не находила офсет 41 (только 40 «внутри» и 44 «уже
|
||||
кладка»), теперь находится точно; `residualPairs` пуст, `status: 'ok'`.
|
||||
Тест умеет падать: до фикса (коммит `6f226a5b`) это был red-witness, что
|
||||
подтверждается сообщением коммита «fix(iso): находить узкие щели по
|
||||
границам препятствий».
|
||||
- **AC2 (проблемная сцена, golden + частично unit):** golden
|
||||
`isometric-large-warm-remount-dark` принят с явным `Baseline-Reviewed`
|
||||
(`86dffbc7`, run `35470468142`), изображение прочитано — маркеры Room 2.5
|
||||
визуально раздельны. Дополнительный unit-свидетель на синтетической, а не
|
||||
буквальной геометрии Room 2.5 (см. замечание ниже, снято без записи как
|
||||
Low) — по духу требования «golden не единственный оракул» выполнено: тот же
|
||||
топологический дефект (щель между узлами 4 px решётки) воспроизведён и
|
||||
проверяется без растровой сверки в `group collision finds a legal
|
||||
one-pixel slit…` и в `group collision boundary events stay sparse…`.
|
||||
- **AC3 (сохранение контрактов, unit):** порядок/перестановка
|
||||
(`group collision separates a solvable dense set independently of input
|
||||
order`), макс. радиус и деградация (`cap and ambiguous ownership fail safe…`,
|
||||
`group collision reports a deterministic residual…`), room/hole/wall safety
|
||||
(`nudge never crosses an island hole…`, `strict room ownership excludes
|
||||
holes…`), исключение room labels (`isoOverlayPlane`/owner-тесты, строки
|
||||
49-55, 260-261), fit-mode skip (код `mode === 'live'` в
|
||||
`iso-scene-render.ts` не тронут, кеш-хинт добавлен только внутри той же
|
||||
ветки), деградация без бросков (`malformed collision input degrades…`) — всё
|
||||
на месте, ничего не удалено.
|
||||
- **AC4 (ограниченность поиска, unit + чтение кода):**
|
||||
`buildIsoOverlayBoundaryCandidates` — публичный чистый генератор, покрыт
|
||||
тестом на разреженность (`candidates.length < 7238/10`) и на отсутствие
|
||||
ложных перекрёстных пересечений несвязанных прямоугольников (`boundary
|
||||
events do not cross unrelated rectangle axes…`). Чтением кода подтверждено:
|
||||
`addBoundaryCircle` — O(radius), не O(radius²); `localRefinementOffsets`
|
||||
ограничена окном 9×9; расширение стен/оверлеев в главном цикле
|
||||
`resolveIsoOverlayCollisions` — по мере обнаружения (`pendingWalls`,
|
||||
`encounteredOverlayBoundaries`), а не заранее по всей сцене. Полного
|
||||
однопиксельного скана диска в коде нет.
|
||||
- **AC5 (визуальная локальность):** построчный diff
|
||||
`baselines-index.json` показывает изменение ровно одного хэша сцены
|
||||
(`isometric-large-warm-remount-dark`), остальные 105 записей и
|
||||
`docs/images/screenshots.json`/`docs/images/*.png` — отдельная, явно
|
||||
объяснённая история про уже смёрженный #598 (`61935250`, со своей
|
||||
Linux-проверкой и просмотром трёх кадров), не про рендер этой задачи.
|
||||
- **AC6 (performance, Linux CI):** оба файла budgets вернули точно 150/60/75
|
||||
при неизменных `hardMaxMs` (2200/600) и `maxRegressionRatio` (0.2) для всех
|
||||
метрик — построчно сверено с ТЗ §10. Независимо найден и прочитан
|
||||
зелёный прогон `performance.yml` run `35477092908` на точном SHA `8769905b`;
|
||||
оба профиля (`isometric`, `isometric-stage3`) — `candidate-sha` совпадает,
|
||||
все проверки `✅`. Формально не хендофнуто автором — см. находку выше.
|
||||
- **AC7 (обычные гейты):** Validate на этом SHA зелёный (см. вводные к
|
||||
ревью, run 35477091271) — `tsc`/`test:unit`/`build` не перегонялись повторно
|
||||
по правилам сужения гейтов. `node scripts/check-docs.mjs` не запускался
|
||||
отдельно: diff трогает `src/**`, но строгий docs-гейт уже входит в Validate
|
||||
и он зелёный на этом SHA; отдельного повода сомневаться в его результате нет
|
||||
(fingerprint обновлён последним коммитом `8769905b`, подтверждён Linux-
|
||||
прогоном 35476979319 в тексте коммита).
|
||||
- **AC8 (документация):** `docs/ISOMETRIC.md` и `docs/ARCHITECTURE.md`
|
||||
корректно описывают именно тот алгоритм, что реализован (границы, а не
|
||||
решётка/скан; порядок фильтров: приблизительные bounds → точные предикаты);
|
||||
сверено построчно с кодом. `docs/STATUS.md` обновлён не полностью — см.
|
||||
находку выше.
|
||||
- Инварианты геометрии плана (`npm run invariants`) не требуются: задача
|
||||
явно не трогает сохранённую геометрию/toolchain модели (§8 ТЗ), только
|
||||
производную экранную геометрию текущего кадра.
|
||||
|
||||
## Чего не проверял и почему
|
||||
|
||||
- **`npx tsc --noEmit`, `npm test`, `npm run build`** — не перегонял.
|
||||
Validate на точном материале `8769905b` зелёный (ссылка в вводных к
|
||||
ревью, run 35477091271), включает все три. Дополнительных изменений после
|
||||
этого прогона в дереве нет (материал — тот же коммит).
|
||||
- **`npm run golden:verify` (полная матрица)** — не перегонял отдельно;
|
||||
Validate на этом SHA уже гоняет полный golden-набор (в diff видно
|
||||
«остальные 172 сцены Linux-прогона совпали с эталонами» в `86dffbc7`, и
|
||||
финальный докс-фингерпринт подтверждён последующими зелёными
|
||||
Linux-прогонами вплоть до самого HEAD).
|
||||
- **`npm run invariants -- --config …`** — не требуется: diff не касается
|
||||
рёбер комнат, записей толщины, `layout`, `marker.space`, `open_spans`; это
|
||||
чисто экранная 2.5D-геометрия, не персистентная модель.
|
||||
- **`python -m pytest tests_backend`** — не требуется, `custom_components/**/
|
||||
*.py` не тронут.
|
||||
- **Browser-смоки `demo/smoke_*.mjs`** — не гонял.
|
||||
`node scripts/smoke-select.mjs --base origin/dev --head HEAD` вернул
|
||||
«прямое совпадение» на символе `pointInRing` для 4 смоков
|
||||
(`smoke_multiwall_strip_containment`, `smoke_sun`,
|
||||
`smoke_wall_thickness_transition`, `smoke_zero_divider_taper`). Проверено
|
||||
чтением: `pointInRing` в `src/iso-overlays.ts` — приватная, неэкспортируемая
|
||||
функция этого модуля; grep по `src/*.ts` не находит одноимённой функции ни в
|
||||
wall-thickness/sun-геометрии, ни где-либо ещё, откуда эти смоки могли бы её
|
||||
импортировать. Совпадение — по имени символа на изменённых строках, не по
|
||||
графу вызовов; сами смоки проверяют плоский вид и толщину стен, которые ТЗ
|
||||
прямо исключает из скоупа (§5) и которые физически не используют этот
|
||||
приватный helper. Это ровно случай «слабая связь — повод посмотреть, а не
|
||||
обязанность прогонять» из инструкции; посмотрел, прогонять не стал.
|
||||
- **Golden PNG у изменившейся сцены** — не пересчитывал построением бандла;
|
||||
прочитал уже принятый в коммите файл визуально (см. «Как проверялось», п.6)
|
||||
и полагаюсь на канонический процесс `Baseline-Reviewed` (`86dffbc7`,
|
||||
run `35470468142`), который для этого и существует.
|
||||
- **Формальное построчное доказательство корректности итеративного
|
||||
worklist-алгоритма** (`resolveIsoOverlayCollisions`, ~350 новых строк) —
|
||||
не проводил как отдельную формальную процедуру: слишком дорого для code
|
||||
review. Доверие построено на комбинации (а) 172+ golden-сцен, не изменившихся
|
||||
побайтно, (б) целевых unit-тестов на конкретные топологии (щель, диагональная
|
||||
комната + кэш-хинт, вогнутая стена вместо бампера её bbox), (в) видимой
|
||||
дисциплины автора — каждый найденный на golden/perf прогоне контрпример чинился
|
||||
отдельным коммитом с новым регрессионным тестом (`f5365f49`, `ca739743`,
|
||||
`df836c81`). Остаточный риск: топология уже стен + уже собственных
|
||||
оверлеев одновременно в очень плотной сцене вне покрытых сценариев — не
|
||||
исключаю, но это тот же риск, что explicitly назван в ТЗ §14 и который
|
||||
golden-матрица (172 сцены) частично снимает.
|
||||
|
||||
## Вердикт
|
||||
|
||||
Жёлтый. High: 0, Medium: 1 (в скоупе, возвращается автору в этой же ветке —
|
||||
не заводится отдельным issue). Функциональность фикса подтверждена (AC1–AC6
|
||||
доказаны, включая независимо найденный зелёный exact-SHA performance-прогон);
|
||||
единственная находка — процессная/документационная: `docs/STATUS.md` не
|
||||
обновлён после получения evidence и сам handoff с run-ссылками не оставлен в
|
||||
issue, как того требует ТЗ §12.5/AC7/AC8.
|
||||
|
||||
---
|
||||
|
||||
<!-- material-anchors: сгенерировано конвейером (#414) -->
|
||||
|
||||
## Материал раунда
|
||||
|
||||
- Ветка: `issue/585-iso-overlay-boundary-search`, коммит `8769905b4e0e` — ребейз его осиротит, и это нормально: ниже якоря, которые ребейз не меняет.
|
||||
- Дерево материала: `871720d6cd52ae82a178847032d4b7225b3c01d6`
|
||||
```
|
||||
git log --all --format='%H %T' | grep 871720d6cd52
|
||||
```
|
||||
- Тело issue: `7e85ec65b07414bb734b5f19ffb10510ee60b3af4fdb52ed429df29d2593ebc8`
|
||||
- Вердикт конвейера: `yellow` · High 0
|
||||
Reference in New Issue
Block a user