diff --git a/docs/reviews/CODE-REVIEW-223-r1.md b/docs/reviews/CODE-REVIEW-223-r1.md new file mode 100644 index 00000000..5639124d --- /dev/null +++ b/docs/reviews/CODE-REVIEW-223-r1.md @@ -0,0 +1,166 @@ +# Код-ревью #223 — Optimize канонизирует координаты без floating-point шума + +- Цикл: r1/4 (код-ревью; ТЗ прошло отдельный лимит и закрыто зелёным на r3) +- Диапазон: `git diff origin/dev...HEAD` (7 коммитов, `origin/dev..HEAD`) +- Реализация: коммит `38f74e0` (`fix: canonicalize near-grid coordinates exactly`, + `Issue: #223`, `User-Visible: yes`) — единственный коммит с продуктовым + изменением; предыдущие 6 коммитов — ТЗ и его ревью (r1–r3), уже закрыты + отдельным циклом. +- ТЗ: `docs/specs/223-optimize-coordinate-canonicalization.md`, зелёное ревью + `docs/reviews/SPEC-REVIEW-223-r3.md`. + +## Скоуп проверки + +Полный разбор — это первый цикл код-ревью, §2.10 (дельта по раундам) здесь не +применяется. Список изменённых файлов (`git diff origin/dev...HEAD --name-only`) +сверен построчно с §4/§12 ТЗ — расхождений нет, попутных правок нет: +`src/align-grid.ts`, `src/houseplan-card.ts`, `src/i18n/{en,ru}.json`, +`test/align-grid.test.mjs`, `test/plan-optimizer.test.mjs`, `test/i18n.test.mjs`, +`scripts/mutation-gate.mjs`, `demo/smoke_optimize_coordinate_canonicalization.mjs`, +`docs/{CANVAS,USER-GUIDE.ru,TESTING,STATUS,CHANGELOG,CHANGELOG.ru}.md`, +`docs/specs/README.md`, три копии бандла. `custom_components/**/*.py` не +тронут — контракт ТЗ («persisted schema и backend не меняются») подтверждён +структурой diff, не только текстом. + +## Как проверялось (гейты) + +Прогнано в этом цикле: + +| Гейт | Команда | Результат | +|---|---|---| +| Typecheck | `npx tsc --noEmit` | pass, без вывода | +| Unit | `npm test` | `tests 975, pass 975, fail 0` | +| Build + сверка бандлов | `npm run build` затем `sha256sum dist/houseplan-card.js custom_components/houseplan/frontend/houseplan-card.js demo/srv/assets/houseplan-card.js` | все три файла — один и тот же хэш `3f7eaf824d79b33f190b9323e2d873aff7eb19d6eef7749488e96b39c76ad33a` (совпадает с хэшем из хендоффа автора) | +| Targeted browser smoke (AC6/AC7) | `node demo/smoke_optimize_coordinate_canonicalization.mjs` | `13/13 true`, `OK` | +| Смежные browser smoke (диф трогает общий `alignAllToGrid`/`snapN`, используемый этими сценариями) | `node demo/smoke_grid_snap.mjs` и `node demo/smoke_optimize_micro_interval.mjs` | оба `OK`, все проверки `true` | +| Mutation guard (AC1) | `node scripts/mutation-gate.mjs --id=snapn-returns-input-near-node` | `поймано 1 из 1` — мутант пойман, чистый прогон зелёный | +| Mutation registry hygiene | `node scripts/mutation-gate.mjs --check` | `ok snapn-returns-input-near-node` среди прочих, без ошибок уникальности `find` | +| Process gate (офлайн) | `node scripts/process-gate.mjs` | `гейт пройден, предупреждений 0` | + +Не прогонялось и почему: + +- **Полный набор из 127 браузерных смоков** — diff меняет ровно один + batch-путь (`alignAllToGrid` внутри explicit Optimize) и явно поименованный + смок плюс два смежных по тому же пути прогнаны выше; остальные 124 смока не + затрагивают ни `align-grid.ts`, ни отчёт Optimize. Полный набор — предрелизный + гейт (PROCESS.md §8), не гейт ревью. +- **`npm run golden:verify`** — diff не меняет разметку диалога (тот же + `.alignmsg`, добавлена подстрока в уже существующий текст) и не двигает + геометрию видимо: величина правки — доли `EPS` (≈4.2e-9 нормализованной + единицы), она гарантированно ниже порога любого golden-снапшота. Само ТЗ + §10 фиксирует это решение («Golden baseline не требуется... перед бетой + выполняется общий golden verify по процессу»), и я согласен с этой оценкой + после чтения diff. +- **`python -m pytest tests_backend`** — ни один файл + `custom_components/houseplan/**/*.py` не тронут (подтверждено списком файлов + выше). +- **performance-профили** — в AC не названы; изменение линейно по уже + обходимым координатам, добавляет одну точную проверку на компонент; ТЗ §11 + явно снимает необходимость отдельного performance-гейта, и чтение + `alignAllToGrid` (без новых циклов/аллокаций сверх O(1) на компоненту) + подтверждает это. + +## Находки + +Находок, блокирующих или требующих правки в задаче, нет. + +### Low — AC2 (неконечные числа) не имеет отдельного unit-доказательства + +- **Файл:** `src/align-grid.ts:69-72` (`snapN`), контракт — AC2 в + `docs/specs/223-optimize-coordinate-canonicalization.md:147`. +- **Что не так:** AC2 требует доказательства «boundary unit matrix» для двух + утверждений: (1) значение дальше `EPS` продолжает выравниваться как раньше — + это покрыто (`test/align-grid.test.mjs:48-50`, случай `off = node + S/3`); + (2) «не конечные числа сохраняют прежний результат» — для этого в текущем + наборе тестов нет ни одного `assert` на `NaN`/`Infinity`/`-Infinity`, ни в + новых тестах, ни в уже существовавших (`git show origin/dev:test/align-grid.test.mjs` + такого теста тоже не содержит). +- **Почему не блокирует:** строка `if (!Number.isFinite(v)) return v;` + (`src/align-grid.ts:70`) в этом diff не менялась — это тот же guard, что был + до задачи, и он был непокрыт тестом и раньше. Риск регрессии от этого diff + нулевой: изменённая строка (`return Math.round(...)`) находится строго после + guard и не может исполниться для нефинитного входа. +- **Решение ревьюера:** снимается без правки в этой задаче. AC2 по факту + доказан читкой (guard не тронут), а не только тестом; отдельная задача на + добавление такого unit-теста не заводится (тривиальная гигиена покрытия, + не дефект поведения) — по желанию автора может быть добавлена заодно со + следующей правкой этого файла. + +## Проверено по AC (код-ревью отвечает за «оно вообще работает») + +| AC | Доказательство | Как проверено | +|---|---|---| +| AC1 | `test/align-grid.test.mjs:41-51` (юнит), мутант `snapn-returns-input-near-node` | Юнит прогнан зелёным в составе `npm test`; мутант отдельно прогнан и **пойман** (`scripts/mutation-gate.mjs --id=...` → `1 из 1`) — тест умеет падать. | +| AC2 | Юнит на `off`-случай зелёный; неконечные числа — см. находку Low выше | Частично тестом, частично чтением (guard не тронут). | +| AC3 | `test/plan-optimizer.test.mjs:75-107` (six-room ULP fixture), `unionBodies` | Юнит зелёный в `npm test`; проверено, что тест реально утверждает `moved:0, maxShift:0, coordsCanonicalized>0, changed:true` и что `unionBodies` не падает на результате — тест умеет падать (до фикса `snapN` возвращал `v`, координаты остались бы носящими шум, `coordsCanonicalized` был бы `0`, `assert.ok(... > 0)` покраснел бы). | +| AC4 | `test/align-grid.test.mjs:53-94` (poly/rect/partition accepted+rejected/wall_column/decor/marker) | Юнит зелёный; читкой кода (`src/align-grid.ts:264-291`) подтверждено, что вклад партиции считается **после** ветвления `hostedFit`, а не на входе в `snapN` — ровно то, что требовал r1 ревью ТЗ. Тест явно проверяет, что `partitions[1].a[0]` (`hostedFit=false`) остаётся `noisy`, а счётчик не растёт от неё (`coordsCanonicalized === 8`, вручную пересчитано по фикстуре — сходится). | +| AC5 | `test/align-grid.test.mjs:158-162`, `test/plan-optimizer.test.mjs:101-106` | Юниты зелёные, оба проверяют `coordsCanonicalized: 0` и `changed: false` на повторном проходе, включая глубокое равенство. | +| AC6 | `test/i18n.test.mjs:30-40`, targeted smoke | i18n-юнит проверяет ровно обе строки RU/EN и обе точки использования в `houseplan-card.ts` через regex по исходнику; smoke проверяет реальный рендер (`previewNamesBothReportUnits`, `toastExplainsZeroMoveCleanup`) — оба зелёные при прогоне выше. | +| AC7 | `test/plan-optimizer.test.mjs:83` (`assert.deepEqual(config, before, ...)`), smoke | Юнит на немутацию preview зелёный; smoke подтверждает Cancel/Apply/Undo на реальном диалоге (`previewDoesNotWrite`, `cancelDoesNotWrite`, `applyUsesOneAtomicWrite`, `undoRestoresExactNoisyValues`, `undoIsOneDeep`) — все `true` в прогоне выше. | +| AC8 | Чтение: `grep -rn "alignAllToGrid\|snapN(" src/*.ts` | `alignAllToGrid` вызывается ровно из одного места — `plan-optimizer.ts:398`, внутри `optimizePlans()`, который сам вызывается только явным `_runAlignToGrid`/`_openAlignDialog` (нет вызовов из пути save/render). Проверено чтением, не исполнением. Полный `npm test` (975/975) зелёный, включая существующие тесты #218. | +| AC9 | `git diff` по docs, `sha256sum` трёх бандлов | Документация обновлена по списку §12 ТЗ; хэши бандлов идентичны (см. таблицу гейтов). | +| AC10 | Таблица гейтов выше | typecheck/unit/build/targeted-smoke/mutation — все зелёные. | + +## Что проверено и корректно (сверх таблицы AC) + +- **Архитектура счётчика.** `noteCanonicalCoordinate`/`noteCanonicalPoint` + (`src/align-grid.ts:157-166`) считают вклад строго по значению, реально + записанному в candidate: для `room_drafts` вклад учитывается только для точек, + попавших в итоговый `draft.points` (`written` строится параллельно с `points`, + включая тот же dedup по `EPS`), и только когда сам draft не был отфильтрован + фильтром `points.length >= 2` (иначе весь draft уходит в `removedDrafts`, а не + в candidate). Для partition — вклад учитывается только внутри + `if (snappedLength > EPS && hostedFit)`, то есть ровно в ветке, где `p.a`/`p.b` + реально присваиваются. +- **Скрещённая проверка арифметики.** Вручную пересчитан фикстурный тест + `near-node report counts only coordinate values actually written to the + candidate` (`test/align-grid.test.mjs:53-88`): из восьми `noisy`-вхождений в + фикстуре ожидаемый вклад — poly (1: только x первой вершины), rect (1: x + дальнего угла через `x0+w0`), partition `accepted` (1), `wall_column` (1), + decor `line`/`box`/`text` (по 1), marker (1) = 8, ровно то, что проверяет + `assert.equal(result.report.coordsCanonicalized, 8)`. Партиция `rejected` + корректно исключена. +- **`OptimizeReport` наследует поле честно.** `plan-optimizer.ts:38` — + `OptimizeReport extends AlignReport`, а итоговый report собирается через + `{ ...alignReport, ... }` (`plan-optimizer.ts:522`) без явного поля — + `coordsCanonicalized` переносится автоматически, не задваивается и не + теряется; `changed` в `optimizePlans()` вычисляется отдельно, через полное + сравнение JSON конфигурации (`plan-optimizer.ts:513`), поэтому корректно + становится `true` даже если единственная правка — ULP-чистка. +- **`moved`/`maxShift*` не растут от ULP-правок.** Это гарантирует не новый, а + существующий `note()` (`src/align-grid.ts:181-187`, не изменён этим diff): + `moved` увеличивается только при `d > EPS`. Новый код добавляет отдельный + счётчик параллельно, не переиспользуя и не искажая этот путь. +- **UX-терминология.** Финальная строка `gs.optimize_changes` (RU: «обновлено + пространств: {c}; устранён шум координат: {p}»; EN аналогично) разводит два + счётчика без пересечения с занятым во всём проекте термином «нормализовано»/ + «канонизировано» из `docs/CANVAS.md` — ровно то решение, которое было + согласовано на r2/r3 ревью ТЗ; я перечитал `docs/CANVAS.md:12,20,333,489,539` + и не нашёл нового конфликта термина с этой формулировкой. +- **Итоговый toast** (`gs.align_done`, `src/houseplan-card.ts:14288-14291`) + включает `coordsCanonicalized` в общую сумму обслуженных записей — предположение + §13.4 ТЗ реализовано буквально. +- **Отсутствие побочных путей записи.** Подтверждено чтением (см. AC8), что + `snapN`/`alignAllToGrid` недостижимы вне явного Optimize — обычные + read/render/Save не подвергаются риску скрытой мутации персистентных данных. +- **Три копии бандла синхронны**, коммит несёт верные трейлеры, оба changelog + правлены в том же коммите, что и поведение (`38f74e0`). + +## Чего не проверял + +- Полный browser-smoke набор (127 сценариев), `golden:verify`, + `pytest tests_backend`, performance-профили — не прогонялись; причины и + обоснование сужения — в таблице гейтов выше. Это решение ревьюера, а не + молчаливый пропуск. +- Ручного визуального прогона карточки в браузере (открыть демо, покликать + диалог глазами) не делал — заменён точечным browser-smoke и чтением + рендер-кода; в процессе фазы ручного тестирования нет (PROCESS.md §2.7), и + именно smoke здесь стоит на её месте. +- WSL/полный HA harness и Windows-специфичные smoke не запускались — сессия + ревью работает в Linux CI-подобном окружении, что и предусмотрено процессом + для код-ревью. + +## Вердикт + +Зелёный. High: 0. Medium: 0. Одна находка Low снята решением ревьюера с +записью (см. раздел «Находки») — правка не требуется.