diff --git a/docs/reviews/CODE-REVIEW-580-r1.md b/docs/reviews/CODE-REVIEW-580-r1.md new file mode 100644 index 00000000..d0e494ad --- /dev/null +++ b/docs/reviews/CODE-REVIEW-580-r1.md @@ -0,0 +1,196 @@ +# CODE-REVIEW-580-r1 + +Issue: #580 · Этап: code · Заход: r1 · Материал: `4cb6827c3d1ea0e8307ff7f406b425262c25649a` +(рабочая копия проверена на этом SHA, `git fetch`/`checkout` не выполнялись) + +## Скоуп + +Диапазон `origin/dev..HEAD` — три содержательных коммита плюс два коммита +документов ревью (`515cf057`, `a18e41e2`, унаследованы от этапа spec, не +трогаются): + +- `23e66392` (`fix`, User-Visible: yes) — сама правка: окклюзия комнатной + части внешнего луча внутренними откосами толстой стены. +- `c22eaf7d` (`fix`, User-Visible: no) — только типизация того же кода + (`intersection`/`Geom` вместо `as any`), поведение не меняет — сверено + построчно, дифф идентичен по логике. +- `4cb6827c` (`test`, User-Visible: no, несёт `Release:`/`Baseline-Reviewed:`) — + принятие двух golden-эталонов (DPR1/DPR2). + +Класс изменений: A (`src/sun.ts`), B (`test/sun.test.mjs`, +`demo/smoke_sun.mjs`, `demo/golden/matrix.mjs`, `test/golden-matrix.test.mjs`, +`scripts/mutation-registry.mjs`), C (`docs/SUN.md`, оба CHANGELOG), D +(бандл в `dist/**` и `custom_components/houseplan/frontend/**`, +`demo/golden/baselines/**`) — соответствует AC7: UI, i18n, touch-контракт и +схема конфигурации не тронуты (`custom_components/houseplan/validation.py` в +диффе отсутствует). + +Трейлеры: у `23e66392` `User-Visible: yes` и правки **обоих** changelog в +том же коммите (проверено `git show --stat` — `docs/CHANGELOG.md` и +`docs/CHANGELOG.ru.md` оба в списке файлов). У коммита с baseline — +`Release: v1.76.0-beta.4` и `Baseline-Reviewed:` со ссылкой на реальный run. + +## Как проверялось + +### Гейты + +**Уже подтверждены на этом SHA (не перегонял):** `npx tsc --noEmit`, +`npm test`, `npm run build` — по ссылке из вводного сообщения ревью +(run `34934599122`, `completed/success`, `headSha = 4cb6827c...`). + +**Проверил сам через `gh run view` (а не поверил заявлению автора):** +нашёл на этом же SHA более ранний run `34933022019` (тот же `headSha`), где +тяжёлые джобы реально выполнились, а не были пропущены реюзом: + +| job | conclusion | +|---|---| +| Смоки в браузере (шард 1/2/3 из 3) | success | +| Смоки: все шарды зелёные | success | +| Golden-кадры против принятых эталонов | success | +| Перф-смок: бюджет времени кадра | success | +| Мутанты по диффу (1–6/6) | success | + +Это закрывает AC4 (browser smoke), AC5 (golden на точном SHA, включая +только что принятые `lighting-sun-window-outer-occluded-dpr{1,2}-dark`) и +AC6 без повторного локального прогона. Второй run на том же SHA +(`34934599122`) эти джобы пропустил через «Переиспользование: это дерево +уже проверено» — ожидаемо, т.к. дерево между двумя прогонами не менялось. + +**Не гонял и не требовалось:** `node scripts/check-docs.mjs` отдельно — +джоб `docs`/«Предполёт» зелёный в обоих run на этом SHA, а diff не меняет +скриншотный контракт (только `src/sun.ts`, без UI). `python -m pytest +tests_backend` — `custom_components/**/*.py` не тронут. `npm run +invariants` — diff не меняет рёбра комнат, записи толщины, `layout`, +`marker.space` ни `open_spans`; толщина стены здесь читается +(`wallDepthByOpening`), не записывается, модель геометрии не изменена. +Полный `demo/smoke_*` набор — не требовался, три browser-shard'а уже +покрывают изменённый `demo/smoke_sun.mjs` (см. выше). + +### Разбор кода (чтением, не исполнением, сверх того что покрыл CI) + +Проверил математику исправления в `src/sun.ts` независимо от тестов — +разложил `away` в базисе {нормаль, тангенциальная ось окна}: для точки +параллельного луча с параметром `u` на внешнем пролёте, сдвиг на внутренней +плоскости `u' = u + s0·σ`, где `s0 = d/cos`, `σ = away·t̂` — константа, +не зависящая от глубины проникновения в комнату. Значит +`intersectRings(quad, rayQuad(innerA, innerB, away, len), clipPoly)` +(`src/sun.ts:459-472`) корректно оставляет только `u`, для которых и `u`, и +`u'` лежат в `[-half, half]` — это и есть «луч, видимый через оба пролёта». +Туннельная часть (`intersectRings(quad, tunnel)`) не изменена относительно +дореформенного кода. + +Пересчитал вручную (по формуле выше) числа нового unit-теста +`test/sun.test.mjs:495-517` (`#580: an oblique outer ray...`, окно на западной +стене, `d=20`, `azimuth=240`, `elevation=60`, `northDeg=0`) — `cos ≈ 0.866`, +`σ = -0.5`, `s0 ≈ 23.094`, отсюда допустимый диапазон `u ∈ [-40, 28.4529…]`, +что даёт `min(seam)=260`, `max(seam)=300+28.4529946…=328.4529946162075` — +совпадает с константой в тесте **до 13 знаков**, то есть значение не +подогнано под текущий (возможно, ошибочный) вывод программы, а следствие +независимо выведенной геометрии. + +Rim (`rayRimEdges`, `src/sun.ts:627-636`): `rimSources = [a,b,innerA,innerB]` +даёт кандидатов на боковые линии; `uniqueSources` дедуплицирует линии с +одинаковым тангенциальным смещением (`eps=1e-4`) — при `d=0`/режиме `inner` +`ray.rimSources` остаётся `undefined`, и функция откатывается на старое +поведение `[ray.a, ray.b]` (AC3 не нарушен). + +Проверил, что новый unit-тест умеет падать не только логически, но и +институционально: `scripts/mutation-registry.mjs` содержит два новых +мутанта именно на изменённых строках — +`sun-ray-outer-jamb-occlusion-ignored` (откатывает комнатную часть на старый +`clipToRoom(quad, clipPoly)`, guard гоняет оба теста `#580:`) и +`sun-ray-outer-occluded-rim-source-ignored` (обрезает `rimSources` до +`[a,b]`, guard целится в тест про `an oblique`) — `find`-паттерны обоих +патчей побайтово совпадают со строками текущего `src/sun.ts`, оба мутанта +из `6/6` шардов упомянутого прогона зелёные (то есть красные до фикса, +подтверждено CI, не только моим чтением). + +`demo/smoke_sun.mjs` (`outer_oblique_*`, строки 344-352): независимая +третья проверка тех же величин — `innerFaceX` вычислен не копированием +формулы из `src/sun.ts`, а отражением экрана относительно центра стены +(`wallCenterX*2 - r.a[0]`), и `pointInRing` — самостоятельная +point-in-polygon реализация, не переиспользующая `pointInPolygon` продукта. +Проверил, что ассерты `innerSeamMinY > 560 && < 580` и +`blockedPointLit === false` не могли пройти на дореформенном коде: до фикса +`polys` для комнатной части строился без учёта внутреннего пролёта, поэтому +шов на внутренней грани всегда покрывал бы полную высоту окна (560↔640) — +проверка `> 560` (строгое неравенство) на этом коде проваливалась бы. + +`demo/golden/matrix.mjs`: новая пара сценариев использует `azimuth=240` +(предыдущий `outer-thick` — перпендикулярный `azimuth=270`), т.е. это не +дубликат существующего кадра, а действительно новый косой угол; проверка +через контрольный кадр без лучей (`sunRayPixels`) исключает «пустой принятый +эталон». Хэши двух PNG (`dpr1`/`dpr2`) в `baselines-index.json` совпадают +побайтово — проверил размеры (345×195 у обоих) и sha256; это не ошибка +конвейера: `run.mjs` явно проверяет `devicePixelRatio` рендерера +(`Math.abs(actualScale - expectedScale) > 0.001` кидает ошибку), так что +второй прогон гарантированно рисовал при масштабе 2 — здесь совпадение +результата после кропа честное, а не признак того, что DPR2 не +перегенерировался. + +Проверил, что `clipToRoom` не используется больше нигде за пределами +`src/sun.ts:449` (единственный сохранившийся вызов) — рефакторинг в +`intersectRings` не задел других потребителей. + +## Найдено + +Блокирующих находок нет. + +Замечание вне блокировки (не Medium, зафиксировано для памяти): claim в ТЗ +«одно дополнительное пересечение» неточен буквально — ветка `outer && d>0` +раньше уже делала два вызова `clipToRoom`, сейчас тоже два (`intersectRings` +с тремя кольцами вместо двух + один с двумя), стоимость сопоставима, не +per-frame/per-pixel цикл. Не требует правки, т.к. контракт AC («не вводить +новые циклы по пикселям/кадрам») формально не нарушен. + +## Что проверено и корректно + +- AC1, AC2 — доказаны юнит-тестами `test/sun.test.mjs` (два новых теста), + числа перепроверены независимым выводом формулы (см. выше). +- AC3 — режим `inner` и `d<=0` идентичны прежней геометрии (ветка кода не + меняется, `rimSources` остаётся `undefined`); отдельный тест `#580: + normal incidence...` проверяет неизменность полного пролёта. +- AC4 — `demo/smoke_sun.mjs`, зафиксировано зелёным в CI на точном SHA + (3/3 browser-шардов), плюс независимая переформулировка геометрии внутри + самого смока (не копия формулы продукта). +- AC5 — два golden-сценария (DPR1/DPR2), приняты только из полного Linux CI + артефакта (`Baseline-Reviewed:` со ссылкой), `golden:verify` зелёный на + точном SHA. +- AC6 — Validate зелёный на точном финальном SHA (двумя независимыми run). +- AC7 — diff ограничен солнечной геометрией/тестами/документацией; UI, + i18n, touch, схема конфигурации не изменены. +- Трейлеры и оба changelog — в правильном коммите. +- Единственный источник числа: новая величина (обрезанная ширина луча) не + дублируется отдельно нигде во View — она входит в тот же `SunRay.polys`, + что рендерится и что использует `rayRimEdges`; второго независимого + вычисления той же геометрии в продукте нет. + +## Чего не проверял + +- Не запускал `npx tsc --noEmit`, `npm test`, `npm run build` локально — + переиспользовал зелёный Validate на этом самом SHA (двойное + подтверждение, включая тяжёлые джобы, см. таблицу выше). +- Не запускал `node scripts/check-docs.mjs` и `npm run golden:verify` + локально — оба покрыты зелёными джобами `docs`/`golden` в CI на этом + SHA. +- Не запускал `npm run invariants` — diff не касается персистентной модели + геометрии (рёбра/толщина/layout), только рендер солнечных лучей. +- Не запускал `python -m pytest tests_backend` — `custom_components/**/*.py` + не тронут. +- Не запускал `node scripts/smoke-select.mjs` вручную — все три шарда + `demo/smoke_*` уже прогнаны и зелёные в CI на этом SHA, включая изменённый + `demo/smoke_sun.mjs`. + +--- + + + +## Материал раунда + +- Ветка: `issue/580-outer-sun-wall-occlusion`, коммит `4cb6827c3d1e` — ребейз его осиротит, и это нормально: ниже якоря, которые ребейз не меняет. +- Дерево материала: `eb607a98d8c5a3f34ff9545a589358b50988c20f` + ``` + git log --all --format='%H %T' | grep eb607a98d8c5 + ``` +- Тело issue: `558be5cfc33a8fe1f475fbc5a0210392339265c20fad0c22e82858b57c96b848` +- Вердикт конвейера: `green` · High 0