diff --git a/docs/reviews/CODE-REVIEW-661-r1.md b/docs/reviews/CODE-REVIEW-661-r1.md new file mode 100644 index 00000000..99890e61 --- /dev/null +++ b/docs/reviews/CODE-REVIEW-661-r1.md @@ -0,0 +1,214 @@ +# CODE-REVIEW-661-r1 + +Вердикт: **зелёный** · заход r1 · блокирующих циклов 0/4 · High: 0 · Medium: 0 + +Материал ревью: `origin/dev...349af7da66d10549507e8175972d3ad707332b36` +(три коммита: `c8c470ed` фича, `59e05482` golden accept, `349af7da` +отпечаток скриншотов). Рабочая копия уже стояла на этом SHA, `git fetch`/ +`checkout` не делал. + +## Скоуп + +Issue #661, трек `ask`. ТЗ (раздел `## ТЗ` тела issue) — луна в текущей фазе +на фоне «Следует за Солнцем» (сумерки/ночь), переключатель `settings.moon` в +общих настройках, ленивый расчёт в карточке (без бэкенда), ассет-пайплайн +дизайнера, i18n ×4, документы, golden ×2, смоки, мутанты. Ревью ТЗ этой +задачи уже прошло зелёным (`docs/reviews/SPEC-REVIEW-661-r1.md`, 0/0), +поэтому здесь — только код-ревью C1–C9/AC1–AC12. + +Продуктовая рамка (`docs/SCOPE.md`): фича расширяет J1 («живой вид дома +целиком») декоративной деталью ночного окружения — в скоупе, не +самостоятельный источник действий, никаких сервисов/сущностей не создаёт. + +Три отличия от буквы ТЗ задокументированы и приняты владельцем ДО кода +(комментарии 29.09/30.09 в issue, `docs/SUN.md` «Moon» уже переписан под +них): луна не зеркалится по полушарию (лит-сторона всегда левая, `waning` +проходит состояния `waxing` в обратном порядке — мутант `moon-mirror-off` +заменён на `moon-lit-side-flips`/`moon-lazy-gate-off`→фактически +`moon-lit-side-flips`), маска — через `` с растушёванным +терминатором, бокс размера сидит на `.hp-day-cycle-env` (не на окне). Это +не находки: решения владельца предшествуют коду, а не подменяют его после +факта. + +## Как проверялось + +Дешёвые гейты на `349af7da` уже зелёные (Validate run 36701023959, ссылка в +промпте) — `tsc`, `npm test`, `npm run build` + `bundle-policy --verify` не +перегонял отдельно, но локально всё равно выполнил `npm run bundle:sync` +(= `tsc --noEmit && rollup -c` + `bundle-sync.mjs`), чтобы получить свежий +`dist/demo/srv` для смоков — оба шага прошли чисто, что заодно +переподтверждает typecheck и build на этом SHA. + +Дальше — по AC и по выводу `smoke-select`: + +| Гейт | Прогнал | Результат | +|---|---|---| +| `npx tsc -p tsconfig.test.json && node scripts/fix-test-build.mjs` | да | чисто | +| `node --test test/moon.test.mjs test/moon-settings.test.mjs test/bundle-assets.test.mjs test/golden-matrix.test.mjs` | да | 106/106 pass | +| `node scripts/bundle-budget.mjs` | да | initial View 300328 B — **совпадает** с числом хендоффа (300 328 vs 300 338 на dev), headroom 738 B, `lazy moon` 11387 B — своим ленивым графом, не в initial | +| `demo/smoke_moon.mjs` (AC5) | да, в браузере | все 14 проверок true, `OK` | +| `demo/smoke_daycycle_layer_budget.mjs` (AC6) | да, в браузере | 7 слоёв ночью без луны = 7 с луной, `.hp-moon` не создаёт свой слой, `OK` | +| `demo/smoke_sun_rim.mjs`, `demo/smoke_sun.mjs` (прямое совпадение по `RAY_ELEVATION_MIN`/`RAY_FADE_MS`, `_settingsDialog`) | да | `OK` | +| `demo/smoke_general_settings_form.mjs`, `smoke_gs_always.mjs`, `smoke_settings_dialog_cards.mjs`, `smoke_dialog_config_parity.mjs`, `smoke_warm_dialogs.mjs`, `smoke_readonly_cold_start.mjs` (слабая связь по `_settingsDialog`/`_settings`, решение — прогнать) | да | все `OK` — правка диалога (17 строк `t→st`, новый toggleRow) не задела форму, парность конфигурации, warm-remount | +| `demo/smoke_summary_dialog_scroll.mjs`, `demo/smoke_summary_first_paint.mjs` (числились красными в хендоффе как флак #712) | да | оба зелёные на этом дереве сейчас — подтверждает, что автор верно квалифицировал их как флак площадки, не регрессию диффа | +| `python -m pytest tests_backend/test_settings_moon.py tests_backend/test_validation.py` | **нет** | в песочнице ревью нет интерпретатора с зависимостями (`voluptuous` не установлен, venv backend отсутствует) — прочитано чтением (см. ниже), плюс Validate уже прогнал pytest pure (510 passed, хендофф) | +| `npm run golden:verify` (метка `ci:golden`) | нет, повторно | golden уже принят и подтверждён в задаче отдельным коммитом `59e05482` с `Baseline-Reviewed` на артефакт CI; Validate на итоговом `349af7da` зелёный — пересъёмка всей матрицы ради ревью избыточна | +| Полный `npm test` (3302 юнит-теста) | нет | покрыт зелёным Validate; прогнал только относящиеся к диффу файлы явно (см. выше) | +| Мутанты из реестра | нет (не гоняются на `ask` в разработке, #709) | все 9 заявленных в ТЗ мутантов физически присутствуют в `scripts/mutation-registry.mjs` и синтаксически корректны (see below) | + +`node scripts/smoke-select.mjs --base origin/dev --head HEAD`: 6 прямых +совпадений, 17 слабых связей (все — `_settingsDialog`, дно диалога общих +настроек), 4 зарегистрированные связи (`smoke_moon.mjs`, +`smoke_daycycle_layer_budget.mjs` — оба уже названы автором в +`smoke-links.mjs` и прогнаны выше; `smoke_entry_stale.mjs`/ +`smoke_lazy_editor_chunk.mjs` — про `EditorRuntimeLoader`, общий с луной +только паттерн лениво-загружаемого чанка, а не общий код; не прогонял — +`moon-gate.ts` не переиспользует `EditorRuntimeLoader`, ссылка на разные +константы retry-URL). Из прямых совпадений и слабых связей прогнал все, +которые не дублируют уже прогнанные AC5/AC6-смоки (список — в таблице +выше); остальные 12 слабых `_settingsDialog`-совпадений (bg_color, +color_picker_consumers, dialog_polish_605, dialog_segments_i18n, +dialog_zombie, discard_copy, esc_dialogs, ha_controls, help_affordance, +room_tooltip_toggle, sun.mjs [прогнан], zigbee_topology_hover) не прогонял +точечно — правка диалога чисто аддитивна (новый `toggleRow` в конце секции +«Солнце», перенос строк в ленивый словарь без изменения значений), и шесть +уже прогнанных представителей этой же категории (форма, парность, +warm-remount, readonly cold start, карточки диалога, sun.mjs) покрывают +затронутый код путь; полная матрица — задача полного Validate, не ревью. + +## Находки + +Нет ни одной. Ниже — что проверялось прицельно и не дало повода для записи. + +## Что проверено и корректно + +**Математика и пороги (AC1–AC4, C1–C4).** `src/moon.ts` — короткий ряд Меёса ++ топоцентрический параллакс, проверено тестом против 12 точек JPL Horizons +из ТЗ (допуск 1.5°/2.5 п.п., тест реально попадает в границу: точка +2026-10-31 в реестре мутантов помечена как та, что «красит» `moon-parallax-off` +— это не пустая формальность, я прочитал формулу и вижу, что без вычитания +`parallax·cos(geocentric)` высота у горизонта уходит на градусы, что и ловит +тест). `MOON_ELEVATION_MIN`/`MOON_MIN_ILLUMINATION` — литералы (не импорт из +`sun.ts`), с явным тестом равенства `RAY_ELEVATION_MIN`; сознательный компромисс +ради бюджета initial-графа, задокументирован и защищён тестом на дрейф. + +**Фаза и лит-сторона (AC3, C5), отклонение от буквы ТЗ.** `moonPhasePath(k)` +строит путь без зависимости от `waxing`/широты — лит-сторона всегда левая. +Прочитал геометрию пути руками (не только тест): внешняя дуга — полуокружность +бокса радиуса 256 от (256,0) до (256,512) через левый край; терминатор — +дуга эллипса `rx=|2k-1|·240, ry=240` до северного полюса диска (256,16); +свип-флаг 0 при `k>0.5` бросает выпуклость вправо (в тёмную половину — доля +освещённого растёт), свип 1 при `k<0.5` — влево. Тест +`AC3: the mask is the left half plus or minus the terminator half-ellipse` +декодирует ту же дугу и независимо пересчитывает `litShare`, сверяя с `k` — +это не regex по строке, а геометрическая проверка. Мутант `moon-lit-side-flips` +меняет именно свип-флаг внешней дуги — «краснеет» ожидаемо. + +**Слой и покрытие планом (C6, owner decision 7).** Луна — последний ребёнок +`.hp-day-cycle-env`, элемент без собственного композитного слоя (`filter`/ +`will-change` отсутствуют — проверено чтением CSS-строки и смоком +`smoke_daycycle_layer_budget.mjs`, число слоёв не растёт). `smoke_moon.mjs` +зондирует пиксель под планом (не меняется при скрытии луны) и в открытом +поле (меняется) — это прямое доказательство «план поверх луны», не косвенное. + +**Ленивая загрузка (C7, AC8).** `moon-gate.ts` — тот же контракт, что у +`iso-scene-render`/редакторов, в меньшем объёме: чанк грузится только когда +`settings.moon`, окружение существует и фаза не `day` (тест +`C7: the chunk is asked for at night only` подтверждает — 0 обращений днём, +1 успешный рендер после прихода чанка ночью). Бюджет: `bundle-budget.mjs` +у меня локально дал initial View 300328 B — число сошлось с хендоффом +день-в-день, `lazy moon` 11387 B отдельным графом, не пересекается с initial +(`assertBundleBudget` бросает на пересечение — проверено чтением +`scripts/bundle-budget.mjs`). + +**Настройки и i18n (AC9, AC10).** `moonDraftOf`/`writeMoonSetting` — строго +`true`/удаление ключа, как `volumetric_view`; тест читает разметку диалога +структурно (`toggleRow` после `sun-rays`, иконка, подписи) — не хрупкий +regex по всему файлу, а срез между известными якорями. Бэкенд: +`vol.Optional("moon"): bool` отклоняет `"yes"/1/0/None/[]/{}"`, support +package копирует только `isinstance(..., bool)`, `DEFAULT_CONFIG.moon = True` +только для новых установок — путь мерджа в `websocket_api.py` +(`{**DEFAULT_CONFIG, **config}`) не протекает в существующие: `or DEFAULT_CONFIG` +срабатывает только при полном отсутствии сохранённого конфига, а не частично +(проверено чтением, не исполнением — pytest недоступен в песочнице, см. +таблицу гейтов). Экспорт/импорт — `test_issue_661_full_export_and_import_preserve_the_moon` +гоняет оба значения `True`/`False` через реальные `create_export`/`parse_document`. + +Строки перенесены из initial-словаря (`en.json` и т.д.) в ленивый +`settings/*.json` для ~17 ключей диалога — сверил все 4 языка построчно, +ни одна строка не потеряна и не задвоена; тест `moon-settings.test.mjs` +и `bundle-assets.test.mjs` синхронизацию подтверждают структурно, я же +сверил построчно смысл (переводы на месте, не машинный мусор). + +**Golden (AC7).** Два новых кадра зафиксированы в `demo/golden/matrix.mjs` +с явными координатами/часами/`k`; `test/golden-matrix.test.mjs` проверяет, +что старые `day-cycle-*` кадры луну не получили (`scenario.moon === undefined`). +Коммит `59e05482` несёт `Release:`+`Baseline-Reviewed:` на прогон CI, где +эти кадры и появились; `imageSha256` всех 11 кадров в `docs/images/screenshots.json` +не изменился между `c8c470ed` и `349af7da` — сдвинулся только отпечаток +сборки, что и заявлено в сообщении коммита. + +**Ассет-пайплайн.** `scripts/generate-moon-assets.mjs` — жёсткий гейт на +входной SVG (запрещённые теги, `href`/`url()` наружу, `DOCTYPE`/`ENTITY`, +шаблонные литералы `${}`/backtick — то есть инъекция в собственный `svg\`…\`` +литерал исключена), лимит размера, обязательный `viewBox 0 0 512 512`. +`pack.json` провалидирован (`schema_version`, `mirror:false`, `lit_side:left` +как решение владельца) — тест дергает и позитивный, и три негативных случая. + +**Трейлеры.** Все три коммита проверены вручную: `c8c470ed` — +`Issue: #661` + `User-Visible: yes`, changelog (RU+EN) в том же коммите; +`59e05482` — `Release:` + `Baseline-Reviewed:` (ссылка на прогон CI) + +`Issue:`/`User-Visible: no`, как требует правка `demo/golden/baselines/**`; +`349af7da` — только `docs/images/screenshots.json`, класс C (документация), +трейлеры не нужны — соответствует правилу. + +**Одно число — один источник (§8).** Единственное видимое пользователю +число здесь — «3°»/«3 %» в подсказке `gs.moon_hint` (все 4 языка) и в +`docs/SUN.md`/`USER-GUIDE`; источник — константы `MOON_ELEVATION_MIN`/ +`MOON_MIN_ILLUMINATION` в `src/moon.ts`, тексты в документах и переводах +переписаны вручную под них (не генерируются), но само число нигде не +разошлось между документами, кодом и переводами — сверил все вхождения. + +## Чего не проверял + +- `python -m pytest tests_backend/...` — сессия без `voluptuous`/venv + backend; см. таблицу гейтов. Полагаюсь на зелёный Validate (хендофф: + «pytest pure — 510 passed») и на прочтение диффа `validation.py`/ + `support_package.py`/`const.py`, который тривиален и симметричен уже + проверенному коду (`volumetric_view`). +- Полную матрицу `npm run golden:verify` — не переснимал; принято в + задаче отдельным `Baseline-Reviewed`-коммитом, Validate на итоговом SHA + зелёный. +- Полный `npm test` (3302 теста) — не гонял целиком, только файлы, + которых касается дифф, плюс полагаюсь на зелёный Validate. +- Мутанты из реестра — не гонял (правило #709: не гоняются в разработке + ни на каком треке); проверил, что все 9 заявленных в ТЗ (`moon-threshold-off`, + `moon-new-moon-shown`, `moon-day-visible`, `moon-parallax-off`, + `moon-lit-side-flips`, `moon-tick-renders-every-time`, `moon-lazy-gate-off`, + `moon-over-paper`, `moon-validation-accepts-anything`) физически + зарегистрированы и по тексту патча действительно бьют по описанному + инварианту (прочитано, не исполнено). +- 12 из 17 «слабых связей» смоук-выборки на `_settingsDialog`, не + прогнанные точечно (см. обоснование в разделе «Как проверялось»). +- Ручное тестирование в браузере не по смокам (визуальный осмотр диалога + глазами) — не делал; смоки покрывают структуру и поведение, golden — + внешний вид. + +## Материал раунда + +`git rev-parse HEAD` = `349af7da66d10549507e8175972d3ad707332b36`. Для +следующего раунда (если потребуется) дельта считается от этого SHA. + +--- + + + +## Материал раунда + +- Ветка: `issue/661-moon`, коммит `349af7da66d1` — ребейз его осиротит, и это нормально: ниже якоря, которые ребейз не меняет. +- Дерево материала: `b9213300bbaafff88c296d71923bc79a49c962d6` + ``` + git log --all --format='%H %T' | grep b9213300bbaa + ``` +- Тело issue: `77ff1c023b01033841f4fffc73cbbb86c5ef3d37b91b3d2260d9538b729dc625` +- Вердикт конвейера: `green` · High 0