17 KiB
Код-ревью #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), partitionaccepted(1),wall_column(1), decorline/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 снята решением ревьюера с записью (см. раздел «Находки») — правка не требуется.