diff --git a/docs/reviews/CODE-REVIEW-567-r1.md b/docs/reviews/CODE-REVIEW-567-r1.md new file mode 100644 index 00000000..567ba6c7 --- /dev/null +++ b/docs/reviews/CODE-REVIEW-567-r1.md @@ -0,0 +1,166 @@ +# CODE-REVIEW · issue #567 · заход r1 + +Материал: `03983946c561a7e8e3e27ef0c3ba40dbebca7790` (рабочая копия — на нём же, +HEAD detached). Трек — `trivial` (light), ТЗ и AC в теле issue, бюджет циклов — +2, израсходовано 0. + +## Скоуп изменения + +`keepTwoPoint` в `radarConfigFromDraft` (`src/radar-editor.ts`) сравнивал +`sources` через `JSON.stringify(...) === JSON.stringify(...)`, то есть текстом. +Билдер сам меняет порядок ключей (`slots`/`ranges`/`zones`/`occupancy_entity`/ +`count_entity` удаляются и дописываются заново, `availability_entity` остаётся +на месте) — конфиг, только что записанный этим же билдером, при следующем +сравнении оказывался «другим», и радар с `availability_entity` молча терял +`refs`/`rms_cm` двухточечной калибровки при первом обычном сохранении. + +Правка вводит `canonicalSources()` — каноничное по порядку ключей объектов +(рекурсивно, включая объекты внутри массивов) и чувствительное к порядку +*элементов* массивов сравнение через `JSON.stringify` с сортирующим +replacer'ом, сортировка по кодовым точкам. Диапазон правки соответствует +заявленному откату: одна функция плюс замена одной строки сравнения. + +Изменённые файлы вне генерируемых: `src/radar-editor.ts` (+23/−1), +`test/radar-editor.test.mjs` (+99, новые тесты AC1–AC3), `scripts/mutation- +registry.mjs` (+12, один мутант), `docs/images/screenshots.json` (пересчёт +`sourceFingerprint` после `docs:accept --identical`). `dist/**` и +`custom_components/houseplan/frontend/**` — синхронный билд. + +## Как проверялось + +Дешёвые гейты (`tsc --noEmit`, `npm test`, `npm run build` + сверка трёх копий +бандла) на этом SHA уже зелёные в Validate +(https://github.com/Matysh/houseplan-card/actions/runs/34780715957) — не +перегонял. + +Прогнал сам, точечно, под диффом: + +- `npx tsc -p tsconfig.test.json && node scripts/fix-test-build.mjs && + node --test --test-name-pattern="#567" test/radar-editor.test.mjs` — + **3/3 зелёные** (AC1, AC2, AC3). +- **Дисциплина «тест умеет падать»**: временно откатил `canonicalSources(...) + === canonicalSources(...)` на `JSON.stringify(...) === JSON.stringify(...)` + в рабочей копии и перепрогнал те же три теста — **AC1 и AC3 красные** + (`method`: ожидали `two_point`, получили `manual`), **AC2 остался зелёным**. + Это ровно то, что заявил автор в хендоффе (регресс ловится не ценой ложного + прохода AC2). Файл восстановлен из бэкапа, `git status --short` после + восстановления — пусто, дерево чистое. +- `node scripts/mutation-registry.mjs --check` — exit 0, без нарушений. +- `node scripts/smoke-select.mjs --base origin/dev --head HEAD` — вывод + **НЕОПРЕДЕЛЁННОСТЬ**, единственный незарегистрированный символ на изменённых + строках — `canonicalSources`. Проверил `demo/smoke_*.mjs` на упоминания + `two_point`/калибровки — ни один смок не трогает сохранение двухточечной + калибровки при повторном открытии редактора, значит запись в + `smoke-links.mjs` была бы ложной связью. Решение автора «браузерный смок не + нужен» — согласен: `canonicalSources` — приватный хелпер чистой функции, + единственный путь к нему из браузера — `radarConfigFromDraft`, целиком + закрытый тремя unit-AC и мутантом. Смок не прогонял (по диффу и AC не + требуется, а факт «широкой» связи не подтверждён). +- `node scripts/check-docs.mjs` — «Documentation checks passed (7 files, 12 + external links)», зелёный на пересчитанном `sourceFingerprint`. +- `diff -rq dist custom_components/houseplan/frontend` — пусто, деревья + идентичны (третья копия, `demo/srv/assets`, больше не коммитится, #255). +- `node --test test/single-source-numbers.test.mjs` — 3/3, для очистки + совести: правка не касается ничего отображаемого, но механический гейт + «одно число — один источник» остаётся зелёным. +- Грепнул `src/**` на `JSON.stringify(...) === JSON.stringify(...)` — есть ещё + четыре места (`houseplan-editor-runtime.ts:1941,2237,10019,10513`, + `summary-panel.ts:309`), но все они сравнивают массивы/объекты, чей порядок + ключей контролирует не билдер того же модуля симметрично тому, как здесь — + не предмет этой задачи (откат ТЗ прямо ограничивает её одной функцией), и в + issue не заявлены. Не заведение, а просто фиксирую, что смотрел: если один + из них когда-то проявит тот же паттерн (класс #319, как и написано в issue), + это отдельная задача. + +Не прогонял: `golden:verify` (диф не меняет рендер — чистая функция +преобразования конфига), `pytest tests_backend` (Python не тронут), +performance-профили (не заявлены в AC, путь не хайпасный). Полный набор +браузерных смоков — избыточен для одной внутренней функции без UI-поверхности. + +## AC — разобраны по коду и исполнением + +| AC | Доказательство | Вердикт | +|---|---|---| +| AC1 | `test/radar-editor.test.mjs` «#567 AC1», выполнил лично: `method: two_point`, `refs`, `rms_cm`, `mount`/`cell_cm`/`mirror` неизменны, `sources` содержит все три ключа по смыслу | подтверждено | +| AC2 | «#567 AC2», выполнил лично: смена `x_entity` → `method: manual`, `refs`/`rms_cm` удалены | подтверждено | +| AC3 | «#567 AC3», выполнил лично: перестановка полей внутри слота не мешает; добавление/удаление слота и перестановка сущностей между слотами — мешают | подтверждено | + +Регресс-проверка (тест умеет падать) выполнена лично, не только со слов автора +— см. раздел «Как проверялось». + +## Что проверено и корректно + +- `canonicalSources` рекурсивна: `JSON.stringify`-replacer вызывается и для + элементов массивов, поэтому порядок полей *внутри* слота (второй слой, + упомянутый в issue отдельно) канонизируется тем же кодом, без специального + случая — это закрывает наблюдение issue про `slots` заодно с + `availability_entity`. +- Порядок элементов массива не трогается — позиция слота осталась смыслом + (`target_N`), что и требует контракт ТЗ; проверено тестом AC3 (перестановка + сущностей между слотами инвалидирует калибровку). +- Единственный мутант, `radar-sources-compared-as-text`, пойман (`--check` + зелёный). Решение не заводить второго мутанта на сортировку массивов + задокументировано и математически верно: `!Array.isArray` регулирует только + то, применяется ли сортировка ключей к узлу; для массива это оставило бы его + элементы под числовыми индексами в исходном порядке (для длин, о которых + идёт речь — единицы слотов, лексикографический порядок строковых индексов + совпадает с числовым) — реального различия в поведении нет, ловить нечего. + Отдельная запись в AC3-тесте фиксирует смысловое свойство вместо этого. +- Геометрия (`project_local`, `mount`/`cell_cm`/`mirror`) не затронута — + проверено и код-ревью, и выполнением (AC1 отдельно утверждает неизменность + этих величин). +- `User-Visible: no` обоснован: `method` и `rms_cm` нигде не рендерятся + (грепом по `src/*.ts` подтверждено отсутствие обращений вне + `radar-editor.ts`/`radar-setup.ts`/`types.ts`), геометрия не двигается — + changelog не нужен. +- Трейлеры коммита: `Issue: #567`, `User-Visible: no` — оба на месте, коммит + один. +- Классы изменений соблюдены: `src/**` и `test/**`/`scripts/**` — по одному + issue; `dist/**` и `custom_components/houseplan/frontend/**` синхронны + (класс D, не редактировались вручную); `docs/images/screenshots.json` — + часть DoD правки `src/**` (обновление отпечатка через `docs:accept + --identical`, все 11 кадров попиксельно совпали). + +## Находки + +Нет. High: 0, Medium: 0, Low: 0. + +## Продуктовое рассуждение (SCOPE.md) + +Правка не расширяет и не меняет ни один Core user job — она чинит J7-смежный +внутренний контракт (сохранность результата двухточечной калибровки радара +между сохранениями), не вводя нового поведения и не трогая ни один экран. +Она укладывается в «Keep the plan true as the home evolves» (J6) косвенно: +предотвращает молчаливую деградацию уже настроенной инфраструктуры без +действия пользователя. Возражений по скоупу нет. + +## Чего не проверял и почему + +- `npx tsc --noEmit` / полный `npm test` / `npm run build` — не перегонял, + зелёный Validate на этом же SHA уже подтверждён и назван по ссылке. +- Браузерные смоки — не прогонял ни одного: `smoke-select` дал + НЕОПРЕДЕЛЁННОСТЬ по единственному непокрытому символу, ручная проверка + показала, что ни один смок не относится к сценарию двухточечной калибровки, + прогон был бы шумом без сигнала. +- `golden:verify`, `pytest tests_backend`, performance-профили — не по AC, не + по diff (нет Python, нет изменений рендера). + +## Вердикт + +Зелёный. Обе стороны регресса (ложное «изменилось» и ложное «не изменилось») +проверены исполнением, не только чтением; геометрия и достаточность отката +подтверждены; гейты, требуемые диффом, зелёные. + +--- + + + +## Материал раунда + +- Ветка: `issue/567-radar-canonical-sources`, коммит `03983946c561` — ребейз его осиротит, и это нормально: ниже якоря, которые ребейз не меняет. +- Дерево материала: `f3a9eb9b27aa5d0ecd52c09a27483f9e8e50bc06` + ``` + git log --all --format='%H %T' | grep f3a9eb9b27aa + ``` +- Тело issue: `aa2955659fda7d26ffd448236319e735d0a5c1d9ebec765ad9c00e559e82f000` +- Вердикт конвейера: `green` · High 0