From dd365cff4573ccfa7ae0e5bd42c420ceaa5b5e23 Mon Sep 17 00:00:00 2001 From: "claude[bot]" <209825114+claude[bot]@users.noreply.github.com> Date: Sun, 27 Sep 2026 20:51:06 +0000 Subject: [PATCH] docs: review document for #683 Issue: #683 User-Visible: no --- docs/reviews/CODE-REVIEW-683-r1.md | 246 +++++++++++++++++++++++++++++ 1 file changed, 246 insertions(+) create mode 100644 docs/reviews/CODE-REVIEW-683-r1.md diff --git a/docs/reviews/CODE-REVIEW-683-r1.md b/docs/reviews/CODE-REVIEW-683-r1.md new file mode 100644 index 00000000..47ae9b9a --- /dev/null +++ b/docs/reviews/CODE-REVIEW-683-r1.md @@ -0,0 +1,246 @@ +# CODE-REVIEW-683-r1 + +Issue: #683 — «Лестницы: собственные цвета, трапеция направления и равномерный +шаг ступеней». +Этап: code (код-ревью, PROCESS.md §2.7). +Заход: r1 · блокирующих циклов израсходовано 0 из 4. +Материал: `git log --oneline origin/dev..HEAD` и `git diff origin/dev...HEAD`, +ровно SHA `20d2863eea3f94a83269bf8c2ba460a81ebdb41f` (рабочая копия уже на нём; +`git rev-parse HEAD` сверен непосредственно перед подведением итогов). + +## Скоуп + +Задача обслуживает J1 («что происходит сейчас» — понятный символ, не +завязанный на цвет темы HA) и J4/J6 («GUI без ручной правки конфига» — два +стандартных `hp-color-opacity`, snapshot цвета декора) из `docs/SCOPE.md`. +Видимое поведение меняется: `User-Visible: yes` верен, оба CHANGELOG обновлены +в одном коммите (`5b5b8081`). + +Диапазон коммитов: `5b5b8081` (feat, продуктовый код + документация + +changelog), `64b544f1`/`2c9ff32d` (chore(golden), с `Release:` и +`Baseline-Reviewed-Local:` трейлерами), `1a7b1dfc` (fix: манифест +`scripts/config-schema.json`), `20d2863e` (docs: fingerprint скриншотов). +Трейлеры `Issue:`/`User-Visible:` присутствуют на каждом коммите; на +коммитах, трогающих `demo/golden/baselines/**`, есть ровно один +`Baseline-Reviewed-Local:` плюс `Release:` — по правилу. + +## Как проверялось + +Прочитаны: `docs/SCOPE.md`, `docs/process/REVIEWER.md`, `AGENTS.md`, тело +issue #683 (ТЗ r1, все 17 разделов, AC1–AC14), `docs/STAIRS.md`. Разобран +полный `git diff origin/dev...HEAD` (39 файлов) построчно: `src/stairs.ts` +(модель, `stairVisualStyle`/`stairVisualFields`/`stairStyleVars`, +`stairIntervalCount`, геометрия трапеции/ступеней, `renderFingerprint`), +`src/stairs-editor.ts`, `src/stairs-editor-model.ts`, `src/stairs-box.ts`, +`src/stairs-view.ts`, `src/space-render.ts`, `src/styles/plan.styles.ts`, +`src/pdf/pdf-scene.ts`, backend (`validation.py`, `import_export.py`, +`support_package.py`), i18n × 4, `docs/ARCHITECTURE.md`, +`docs/CONFIG-COMPATIBILITY.md`, оба `USER-GUIDE`, `scripts/bundle-budget.mjs`, +`scripts/config-schema.json`, `scripts/mutation-registry.mjs`, +`scripts/monolith-baseline.json`, `demo/golden/baselines/*`, +`demo/smoke_stairs.mjs`, `test/stairs.test.mjs`, `test/pdf-scene.test.mjs`, +backend тесты. + +### Таблица AC — доказательство и что я сделал сам + +| AC | Заявлено | Проверено как | +|---|---|---| +| AC1 | snapshot цвета/opacity/fill_opacity=0, независимость от будущего decor default | unit (`stairs.test.mjs`) + smoke (`newStairsSnapshotCurrentDecorStyle`, `legacyReadDoesNotMaterializeStyle`) — прогнано мной, все true | +| AC2 | два color picker, Save/Cancel/reopen, смена типа сохраняет | smoke (`propertiesExposeBothColourControls`, `cancelKeepsStairVisualStyle`, `reopenRestoresPersistedStyle`, `kindSwitchKeepsIdentityAndLink`) — прогнано мной | +| AC3 | цвет на outline/trapezoid/treads/arrow, fill на весь rect/circle, hover/selected — только временный outline | unit DOM (CSS в `plan.styles.ts`, прочитано) + golden (я прогнал `golden:verify`, 184/184) | +| AC4 | трапеция 100/80, отступы 10%, forward/«Вверх» 80→100, стрелка одинакова | unit geometry (`stairs.test.mjs`, прогнано: `backward.arrowPath === geometry.arrowPath`) + golden | +| AC5 | treads строго между боковыми сторонами, широкое основание не дублируется | unit (`trapezoid.length === 3`, прогнано) + golden | +| AC6 | N минимизирует \|L/N−30см\|, tie → большее N, resize повторяем | unit `stairIntervalCount` — прогнано (288→10, 40→2 с комментарием «tie», 0→1) | +| AC7 | «Вверх/Вниз» на 4 языках, spiral не меняется | unit i18n (`i18n.test.mjs`, прогнано, 27/27) + smoke | +| AC8 | legacy fallback без записи, backend отклоняет плохие color/opacity | frontend unit (`stairVisualStyle` не мутирует legacy-объект, прогнано) + backend (`test_validation.py`, читано; 4 новых `broken`-кейса на `color`/`opacity`/`fill_color`/`fill_opacity`) | +| AC9 | backup/import/export/copy/support сохраняют безопасные поля | backend (`test_ha_import_export.py`, `test_support_package.py`, читано: unknown-поле `future_stair`/`unknown` подтверждённо отброшено, цветовые сохранены) | +| AC10 | footprint/bounds/magnet/hit target не зависят от trapezoid | проверено чтением: `stairFootprintGeometry`/`stairOutline` не используют `trapezoid`; regression-тест `#663 stair magnet …` не тронут диффом | +| AC11 | одна geometry для View/editor/2.5D/PDF, PDF монохромный | проверено чтением: PDF и View/editor вызывают один и тот же `cachedStairRenderGeometry`; unit (`pdf-scene.test.mjs`, прогнано: `styledOutput.commands` идентичен `plainOutput.commands`) | +| AC12 | только active получает cursor pointer | smoke (`getComputedStyle(linkedNode).cursor === 'pointer'`, прогнано) + regression target-state тесты (не тронуты) | +| AC13 | light/dark: transparent default, **пользовательские цвета**, straight-направления, spiral, без theme-подмены | golden — **см. находку M1: пользовательские цвета в матрице не представлены** | +| AC14 | кеш геометрии не зависит от стиля; 250 лестниц в бюджете | unit (`cachedStairRenderGeometry` — правка цвета не инвалидирует кеш, прогнано) + `scripts/model-invariants.mjs --config` на экспорте `makeLargeHouseFixture` (250 лестниц) — прогнано мной, «Инварианты выполнены» | + +### Гейты — что прогнал я сам и почему + +CI Validate на точном SHA (`20d2863e`) я не считаю панацеей автоматически — +проверил, что именно выполнилось, а не пропущено через reuse-механизм: + +| Гейт | Статус | Источник | +|---|---|---| +| `npx tsc --noEmit`, `npm run build`, unit, bundle-sync | зелёные (реально исполнены) | run [36347331581](https://github.com/Matysh/houseplan-card/actions/runs/36347331581) job «Фронтенд: типы, юниты, мутанты, синхрон бандла» — success; я также сам прогнал `npm run build` локально — успех | +| Мутанты по диффу (6/6 шардов, включая 3 новых свидетеля #683: `stairs-tread-count-depends-on-render-scale`, `stairs-legacy-save-keeps-implicit-style`, `stairs-active-link-loses-pointer-cursor`) | зелёные, реально исполнены | run [36347608885](https://github.com/Matysh/houseplan-card/actions/runs/36347608885) — все 6 шардов success (не skipped) | +| Backend pytest с полным HA harness | зелёный, реально исполнен (не reuse) | run [36346278615](https://github.com/Matysh/houseplan-card/actions/runs/36346278615), SHA `99569753` (backend-релевантные файлы не менялись после этого коммита при ребейзе — сверено диффом) — job «Бэкенд: pytest в Home Assistant» success | +| `npm run golden:verify` (полная матрица, 184 сцены — `--only` не поддержан CLI, флаг был проигнорирован и прогнал всё) | **прогнал сам**, exit 0, все сцены `passed`, ни одной ошибки/расхождения | локально, эта сессия | +| `node demo/smoke_stairs.mjs` | **прогнал сам**, 62/62 проверки `true`, `OK` | локально, эта сессия | +| `node scripts/model-invariants.mjs --config <экспорт>` на `demo/fixtures/large-house.mjs` (250 лестниц, AC14) | **прогнал сам**, «Инварианты выполнены: ссылки разрешимы, записи толщины находятся» | локально, эта сессия | +| `node scripts/smoke-select.mjs --base origin/dev --head HEAD` | **прогнал сам**: 22 прямых совпадения, из них ровно один осмысленный (`demo/smoke_stairs.mjs`) — остальные 21 совпали по широким символам (`_decorStyle`, `cellCm`, `NORM_W`, `GRID_N`), общим для десятков несвязанных смоков; не прогонял их отдельно — риск для лестниц там нулевой | локально, эта сессия | +| `test/i18n.test.mjs`, `test/stairs.test.mjs`, `test/pdf-scene.test.mjs` | **прогнал сам** | локально: 27/27, 43/43, все pass | +| `python -m pytest tests_backend` | **не прогнал** — в этом окружении нет `.venv-backend`/`homeassistant`; полагаюсь на CI-прогон с реальным harness выше, а не на голый pytest (он бы скипнул `test_ha_*.py` и ничего не доказал, `docs/TESTING.md`) | — | +| `npm run golden:capture`/full performance suite | не прогонял — предрелизный объём, не требуется гейтом ревью (§8) | — | + +## Находки + +### M1 (Medium, в скоупе) — AC13 не показывает «пользовательские цвета» в golden + +`demo/golden/matrix.mjs:271-282` определяет ровно 4 сцены лестниц через один и +тот же фикстур `stairLayerFixture` (`demo/golden/matrix.mjs:210-223`). Ни один +из четырёх объектов там не задаёт `color`/`opacity`/`fill_color`/ +`fill_opacity` — все они рендерятся с fallback-значением decor-default +(`#607d8b`, `fillOpacity: 0`). Я проверил: во всём `demo/golden/matrix.mjs` и +`demo/golden/harness.mjs` нет ни одного места, где стул задаёт кастомный +цвет/заливку для лестницы. + +AC13 в теле issue дословно требует: «Светлая и тёмная темы показывают +transparent default, **пользовательские цвета**, оба straight-направления и +spiral без theme-dependent подмены» с единственным заявленным способом +доказательства — `golden`. Фактически принятая матрица показывает только +default-стиль (транспарентная заливка, дефолтный цвет) во всех четырёх темах/ +режимах; отдельного кастомного (не-default) цвета/прозрачности лестницы в +golden нет вообще. + +Прецедент в этом же фикстуре показывает, что проект такую ось считает нужной: +`geo-axis-h` (decor-линия в том же `golden-geometry`, `demo/fixtures/ +visual-matrix.mjs:88-89`) намеренно несёт явный нестандартный `color`/ +`opacity`, отличный от темы, и это видно в golden. У лестниц аналога нет. + +Смок (`coloursAndOpacityPersistAsOneEdit` в `demo/smoke_stairs.mjs`, я +прогнал — true) подтверждает, что кастомный цвет реально долетает до +`--hp-stair-line`/`--hp-stair-fill` в DOM, и CSS-правило, которое их +потребляет (`plan.styles.ts:1567-1585`), — тривиальный `var()`-проброс, +уже используемый для декора в других местах. Поэтому фактический +визуальный риск невелик — но заявленное доказательство (golden) для этой +конкретной фразы AC13 отсутствует, а не «ослаблено». + +**Чем краснеет:** пусто (defensive-таблица не заведена для этого пункта, +хотя AC13 — не защитный AC, а описательный про рендер; тем не менее +раздел §8/§2.7 требует не пустой ответ на вопрос «доказано ли заявленное»). +Возврат автору: либо добавить сцену/вариант с явным нестандартным +`color`/`fill_color`/`opacity` у одной из лестниц фикстуры и принять +получившийся кадр, либо сузить текст AC13 в issue до того, что матрица +реально показывает (default + оба направления + spiral + обе темы, без +пункта про custom-цвета) и явно отметить, где custom-цвет доказан иначе +(смок). + +Это Medium **в скоупе** задачи (сам golden-фикстур лестниц — часть +`demo/golden/matrix.mjs`, явно перечисленного в §11 ТЗ как область +изменений и являющегося прямым deliverable AC13/§13 плана тестов). Без +блокирующего High это дает жёлтый вердикт и возврат автору, а не отдельный +issue (#202). + +## Что проверено и корректно + +- **Геометрия трапеции/направления (§5, AC4/AC5).** Стрелка + (`from`/`tip` в `stairRenderGeometry`) больше не зависит от `direction` — + вычисляется один раз по канонической локальной оси; разворачивается + только `startHalfWidth`/`endHalfWidth` (0.4/0.5 в зависимости от + `forward`/`backward`). Совпадающее с внешним прямоугольником широкое + основание не дублируется: третья линия трапеции всегда берёт узкую + (0.4) сторону. Таблица режим→legacy→узкое/широкое основание из ТЗ + сходится с кодом построчно. +- **Равномерный шаг (§6, AC6).** `stairIntervalCount` — чистая функция: + `lower=floor`, `upper=ceil`, сравнение `error(upper) <= error(lower)` + корректно выбирает большее N на равенстве (строгое `<=` в пользу + `upper`). И прямая, и спиральная геометрия используют её на физической + длине в см (`stair.length * cellCm * GRID_N` — точная инверсия старого + `cmToNorm`), а не на нормализованных единицах. +- **Кеш геометрии не зависит от стиля (AC14).** `renderFingerprint` + строится только из `kind/x/y/angle/direction/length/width/radius` (и + `cellCm/scale`) — цветовые поля туда не входят; я убедился прогоном + теста, что правка `color/opacity/fill_color/fill_opacity` у уже + закешированного объекта не создаёт новую геометрию, а поворот — создаёт. +- **Snapshot и legacy fallback (§4, §9, AC1/AC8).** `stairVisualStyle` + не мутирует переданный `stair` (проверено и юнитом, и построением + функции — она только читает и возвращает новый объект). Материализация + происходит только в `saveDialog` через `stairVisualFields(dialog)`, + и «unchanged»-детектор специально сравнивает уже материализованный + `next` с `current` — поэтому первый Save старой записи всё равно + запишет полную четвёрку, даже если пользователь не менял ни одного + видимого значения. Это осознанное и корректное поведение, соответствует + ТЗ дословно. +- **Смена типа сохраняет цвета (§9, AC2/AC9).** `convertStairKind` + переписан на деструктуризацию `{ kind, direction, length/width/radius, + ...common }` — `common` включает цветовые поля и переживает конверсию в + обе стороны. +- **Backend-контракт (§9, AC8/AC9).** `STAIR_SCHEMA` получил `color`/ + `fill_color` через существующий `_COLOR` и `opacity`/`fill_opacity` + через `_finite` + `Range(0, 1)` — точное повторение уже существующего + `_DECOR_COMMON`-паттерна, не изобретение нового. Позитивные списки в + `import_export.py`/`support_package.py` дополнены четырьмя полями; + тесты явно проверяют, что случайное `unknown`/`future_stair` поле + всё равно отбрасывается, а разрешённые — нет. +- **PDF остаётся монохромным при тех же путях (§7, AC11).** Новый unit-тест + `#683 PDF keeps stair visual styling monochrome…` — я прогнал — + сравнивает командный поток PDF для стилизованной и обычной лестницы и + требует их побитового равенства; отдельно проверяет, что трапеция и + равные ступени реально попадают в PDF-путь (≥5 линий толщиной 0.25мм). +- **Cursor pointer только у active-ссылки (§7, AC12).** Живой DOM-смок + (`getComputedStyle(linkedNode).cursor === 'pointer'`) — не только + проверка класса/атрибута, а фактическое вычисленное CSS-свойство в + headless Chromium; я прогнал его сам и получил `true`. Регрессионные + проверки для `missing/self/deleted/fixed` состояний (`data-target-state`) + не задеты диффом и не сломаны. +- **i18n (§10, AC7).** Ключи `stairs.forward`/`stairs.backward` заменены + (не алиасированы) на `stairs.up`/`stairs.down` во всех 4 языках + одновременно; grep по всему дереву не нашёл ни одной оставшейся ссылки + на старые ключи. Общий i18n-тест ключевого паритета зелёный. +- **Бюджет (§14).** `INITIAL_VIEW_GZIP_CEILING` поднят с 299700 до 300200 + внутри того же общего лимита 301066 (не расширен), с подробным + обоснованием в комментарии и правкой того же коммита — не «тихая» + правка. +- **Манифест схемы (`scripts/config-schema.json`).** Отдельный `fix`-коммит + корректно регенерирует 4 новых поля тем же детерминированным скриптом, + найденная автором сама несостыковка (устаревший Validate) обработана + штатно (повторный S7, не апелляция к владельцу). +- **Golden-принятие.** Изменились ровно 4 объявленные сцены; аттестация + `Baseline-Reviewed-Local` в `baselines-index.json.localAttestation.sha256` + дословно совпадает с трейлером коммита `2c9ff32d`; полный прогон + `golden:verify`, который я выполнил сам в этой сессии, зелёный на всех + 184 сценах, включая 4 сцены лестниц — регрессий нет нигде в матрице. + +## Чего не проверял + +- Полный `python -m pytest tests_backend` с реальным Home Assistant — + недоступно в этом окружении (нет `.venv-backend`); опираюсь на CI-прогон + с harness (run 36346278615) на backend-эквивалентном дереве, а не на + голый pytest. +- Полный browser-smoke матрицы (278 смоков) — не требуется гейтом ревью + (§8, предрелизный объём); прогнал только `smoke-select`-совпадения и + явно AC-именованный `demo/smoke_stairs.mjs`. +- `performance_smoke`/perf-профили — не названы в AC явно (AC14 говорит + «performance-smoke/review», и я закрыл это через прямой прогон + `model-invariants` + чтение бюджета O(N); отдельный perf-профиль не + прогонял, посчитал избыточным для локальной геометрии одной лестницы). +- Реальный ручной клик в браузере (десктоп/тач) — ручного тестирования в + цикле нет по регламенту; полагаюсь на смок с реальным `getComputedStyle` + и golden-скриншоты вместо этого. +- Не проверял 2.5D-специфичный рендер глазами за пределами принятого + golden-кадра `stairs-isometric-dark` (доверяю авторскому visual-diff + review, отмеченному в handoff). + +## Вердикт + +Одна находка Medium (M1) внутри скоупа задачи: заявленное в AC13 +доказательство («golden» показывает «пользовательские цвета») фактически +отсутствует — все четыре принятые сцены лестниц используют только +fallback-стиль. Остальные 13 AC полностью доказаны исполненными гейтами +(частично прогнанными мной лично в этой сессии) или чтением с прямой +ссылкой на строку кода. High-находок нет. + +**Жёлтый.** Возврат автору: закрыть M1 (добавить кастомный цвет/заливку в +`stairLayerFixture` и принять получившийся кадр, либо явно сузить текст +AC13 под фактический охват и указать альтернативное доказательство для +оставшейся части фразы). + +--- + + + +## Материал раунда + +- Ветка: `issue/683-stair-visuals`, коммит `20d2863eea3f` — ребейз его осиротит, и это нормально: ниже якоря, которые ребейз не меняет. +- Дерево материала: `0c19e8c122b0627c86c740178ee3785f35e3ea1c` + ``` + git log --all --format='%H %T' | grep 0c19e8c122b0 + ``` +- Тело issue: `5f2e2a957f1cd2869b4e054901cbce315227e529cb9fcc68495857756083a7f6` +- Вердикт конвейера: `yellow` · High 0