diff --git a/docs/reviews/SPEC-REVIEW-266-r1.md b/docs/reviews/SPEC-REVIEW-266-r1.md new file mode 100644 index 00000000..741f4642 --- /dev/null +++ b/docs/reviews/SPEC-REVIEW-266-r1.md @@ -0,0 +1,214 @@ +# SPEC-REVIEW-266-r1 + +Issue: [#266](https://github.com/Matysh/houseplan-card/issues/266) — «Рефакторинг 3/5: расщепить styles.ts» +ТЗ: `docs/specs/266-split-styles.md` (коммит `4c93f28e`, ветка `issue/266-split-styles`) +Этап: spec (PROCESS.md §2.4) · заход r1 · блокирующих циклов израсходовано 0/4 +Вердикт: **жёлтый** + +## Скоуп ревью + +Класс изменения — A (`src/**`), но задача помечена как чистый refactor +(tech-debt, User-Visible: no): перенос правил из монолитного `src/styles.ts` +(3690 строк, один экспорт `cardStyles`) в пять файлов `src/styles/*.styles.ts` +по поверхностям + сборщик. Продуктового поведения нет и не должно появиться — +главный заявленный инвариант ТЗ: «пиксели не меняются». + +Ревью велось состязательно: без переписки с автором, по тексту ТЗ, телу issue +и фактическому состоянию кода/гейтов на `dev`. + +## Как проверялось + +- Прочитаны `docs/SCOPE.md`, `PROCESS.md`, `AGENTS.md` (полностью, включая §7.1, + §8, §10.2–10.4). +- Прочитано тело issue #266 и комментарий аналитики/автора (единственные два + комментария, ни одного цикла ревью до этого не было — подтверждает r1). +- Сверены с кодом фактические числа из ТЗ: + - `wc -l src/styles.ts` → 3690 строк — совпадает с текстом ТЗ (§0). + - `node -e "import('./demo/golden/matrix.mjs').then(m=>console.log(m.GOLDEN_SCENARIOS.length))"` + → 129 сцен — совпадает с AC3. + - Потребители `cardStyles`: `houseplan-card.ts`, `hp-device-preview.ts`, + `space-card.ts`; `editorSecondaryStyles` подключается вторым элементом + массива в `houseplan-card.ts:21973` — совпадает с §1.2. + - Грубая (regex-based) бакетизация правил по кластерам из §1.1 показала явный + перекос: `plan` ≈ 1300–1400 строк против заявленного лимита ≤ 1200 (AC2) — + см. находку L2 ниже, оставлено как наблюдение, не как блокирующее. + - Найдены и прочитаны все 18 `@media` блоков `src/styles.ts` (8× + `prefers-reduced-motion`, 2× `forced-colors`, 2× `prefers-color-scheme`, + 4× `max-width`, плюс дубли) и 11 `@keyframes` — см. находку M1. + - Проверено покрытие golden этих состояний: `demo/golden/harness.mjs:49` и + `demo/golden/run.mjs:607` — golden всегда эмулирует + `reducedMotion:'reduce'` и не эмулирует `forced-colors` вовсе. + - Проверено покрытие smoke: `demo/smoke_plan_snap_overlay.mjs:354-367` + (`forcedColors:'active'`) и `demo/smoke_preloader.mjs:120-121` + (`reducedMotion:'reduce'`, фаза «house does not pulse»); `grep` по + `test/*.mjs` на `forced-colors`/`prefers-reduced-motion` — только + `test/color-picker.test.mjs` (не относится к затронутым блокам) и + `test/isometric-contract.test.mjs` (проверяет только наличие класса в + разметке, не CSS-блок `forced-colors` на `src/styles.ts:594-601`). + - Проверена классификация файла `scripts/dev/styles-diff.mjs` против таблицы + классов PROCESS.md §1 — см. находку L1. + - `docs/ARCHITECTURE.md:17` содержит дерево с `editor-secondary.styles.ts`, + новый каталог `src/styles/` туда естественно вписывается — скоуп «раздел + ARCHITECTURE.md о структуре стилей» реалистичен, конфликтов с существующим + `src/styles/` нет (каталога пока не существует). +- Технических тяжёлых гейтов (typecheck/test/build/golden) не гонял: на этапе + ревью ТЗ кода изменения ещё нет (диф — один файл `docs/specs/266-*.md`), + гонять их не по чему. Это соответствует масштабу этапа spec, не пропуск. + +## Находки + +### M1 — Medium (в скоупе) — заявленный «объективный критерий» (golden) не видит два реальных CSS-состояния, которые ровно и переносятся этим рефакторингом + +`src/styles.ts` содержит 8 блоков `@media (prefers-reduced-motion: reduce)` и 2 +блока `@media (forced-colors: active)` (строки 594–601 — `.iso-*` тени +изометрии, 1922–1930 — `.plan-snap-line`/`.hidden-wall-line`). §1.3.1 ТЗ +называет `npm run golden:verify` «главным и объективным критерием» того, что +пиксели не изменились ни на одном слайсе. + +Фактически: +- `demo/golden/harness.mjs:49` и `demo/golden/run.mjs:607` жёстко фиксируют + `reducedMotion: 'reduce'` на **каждом** golden-снимке и никогда не эмулируют + `forced-colors`. Значит: (а) состояние «motion не ограничен» (обычный + пользователь без предпочтения) вообще никогда не рендерится golden'ом — если + слайс случайно утащит правило из-под `@media (prefers-reduced-motion: reduce)` + наружу или, наоборот, спрятает обычное правило внутрь этого блока, картинка + под форсированным `reduce` не изменится, и golden останется зелёным; (б) + `forced-colors: active` не проверяется golden'ом вообще, ни в одном из 129 + сцен. +- Единственный другой заявленный инструмент проверки — сверочный скрипт + §1.3.5/AC6 — сравнивает «нормализованное множество правил (селектор → текст + объявлений)» без упоминания контекста вложенности. Правило, переехавшее из + одного `@media`-блока в другой (или потерявшее обёртку `@media`/`@supports` + при копировании), даст тот же «селектор → текст объявлений», то есть + инструмент тоже не заметит подмену. +- Существующие browser-smoke закрывают это лишь частично: `smoke_plan_snap_overlay.mjs` + проверяет именно `forced-colors` для `.plan-snap-line`/`.hidden-wall-line` + (тот самый блок на 1922 строке), `smoke_preloader.mjs` проверяет, что + `hp-boot-pulse` не проигрывается при `reducedMotion:'reduce'` — но не + проверяет обратное (что анимация ЕСТЬ без этого предпочтения), и не + затрагивает остальные 7 `prefers-reduced-motion`-блоков. Блок `forced-colors` + для `.iso-*` теней (594–601) не покрыт вообще ничем — ни golden, ни smoke, ни + unit (`test/isometric-contract.test.mjs:25` проверяет только наличие класса + в разметке, не содержимое CSS-блока). +- План тестов ТЗ (§7) не называет ни один browser-smoke вообще, хотя AGENTS.md + («по необходимости... браузерные смоки») и PROCESS.md §8 требуют явно + перечислить относящиеся к диффу смоки и решение по каждому. + +**Сценарий отказа:** на слайсе `plan` (4-й по плану) автор переносит +`.plan-snap-line`/`.hidden-wall-line` в `plan.styles.ts`, но обёртка +`@media (forced-colors: active) { … }` теряется или переезжает в другой файл +без содержимого. В Windows High Contrast снап-оверлей становится нечитаемым +(регрессия ровно того класса, что уже случался — #234 про пиксельные +рассинхроны, найденные не тем гейтом, который должен был их найти). +`golden:verify` зелёный (forced-colors не эмулируется), сверочный инструмент +зелёный (текст правила не изменился, только его обёртка), и только если +кто-то вручную вспомнит прогнать `smoke_plan_snap_overlay.mjs`, дефект +всплывёт — а по плану §7 прогонять его не обязаны. + +**Почему в скоупе, не блокер целиком:** сама конструкция рефакторинга не +порочна, и исправление дёшево и целиком укладывается в уже запланированный +инструментарий: +1. Расширить ключ нормализации сверочного инструмента (§1.3.5/AC6) до полного + пути вложенности: селектор + весь стек охватывающих `@media`/`@supports`/ + `@keyframes`-условий, а не только «селектор → объявления». +2. Явно назвать в §7 два существующих смока + (`demo/smoke_plan_snap_overlay.mjs`, `demo/smoke_preloader.mjs`) как + обязательные к прогону на слайсах `plan`/`base`, раз они трогают именно + переносимые `forced-colors`/`prefers-reduced-motion` правила. +3. Добавить недостающую проверку (unit или расширение существующего смока) для + блока `.iso-*` `forced-colors` (594–601) до его переноса, либо явно + зафиксировать «проверено чтением» с указанием ревьюеру, что делать при + переносе этого конкретного блока. + +Без этого исправления AC3 («golden 129/129 — доказательство пиксельной +неизменности») фактически доказывает меньше, чем заявлено в §1.3.1, а именно +для тех самых мест, где перенос селекторов между файлами наиболее рискован. + +### L1 — Low — неверная классификация вспомогательного скрипта + +§1.3.4 (примечание к AC6) называет `scripts/dev/styles-diff.mjs` «класс C» +(документация). По таблице PROCESS.md §1 путь `scripts/**` целиком относится к +**классу B** («Гейты и инструменты»), не к C. Практического следствия нет — +файл уже привязан к issue #266 при любой классификации, — но формулировка в +самом ТЗ противоречит канону. Правится одним словом при следующей правке ТЗ; +не считаю нужным делать отдельный цикл только за это, фиксирую с записью. + +### L2 — Low (наблюдение, не требует правки ТЗ) — лимит «≤ 1200 строк» на файл (AC2) может не выполниться для `plan.styles.ts` + +Грубая эвристическая бакетизация правил `src/styles.ts` по кластерам §1.1 +(regex по началу правила, накопление до следующего распознанного селектора) +дала: `plan` ≈ 1382, `devices` ≈ 980, `dialogs` ≈ 865, `chrome` ≈ 407, `base` — +заведомо недооценён эвристикой. Это неточный инструмент (не учитывает все +переключения кластеров внутри `@media`), но сигнал достаточно сильный: +поверхность `plan` (сцена, стены, снап, декор-слой, компас, измерения, +`.room*`, `.bdframe`/`.dtframe`) — самая большая и самая связная, и её слайс +идёт четвёртым, то есть после того как более мелкие поверхности уже вычищены. +Если по факту `plan.styles.ts` выйдет за 1200 строк, AC2 не докажется буквально +— это будет находка код-ревью, а не крах архитектуры ТЗ. Оставляю как +наблюдение: автор может заранее заложить одно из двух — либо явно +предусмотреть в ТЗ право одного файла (`plan`) на больший потолок с +обоснованием, либо принять как есть и решить вопрос на слайсе 4. Не +блокирую ревью этим пунктом. + +## Что проверено и корректно + +- Все семь обязательных продуктовых/технических подразделов §7.1 + присутствуют в той или иной форме: сценарий (§0), контракт (§1), скоуп/не- + скоуп (§2), UX/данные/i18n/touch — единой строкой (§3, адекватно для + чистого CSS-рефакторинга с нулевым видимым эффектом), риски (§4), + release-артефакты (§5), AC с доказательством (§6), план тестов (§7), откат + (§8). Формулировка «что человек увидит» — «рендер обязан быть пиксельно + идентичен» — честный ответ для задачи без пользовательского эффекта, не + замаскированная техническая работа под видом продуктовой; прецедент такого + же лаконичного оформления для чистого рефакторинга есть в + `docs/specs/034-frontend-decomposition.md` (родительский зонтик, этап 5 + которого — этот issue). +- Числа в ТЗ (3690 строк, 129 golden-сцен, состав потребителей `cardStyles`, + порядок подключения `editorSecondaryStyles`) сверены с кодом дерева на + момент ревью и совпадают буквально — никакой догадки, выданной за факт, не + обнаружено. +- Инвариант «без дубликатов» (§1.3.2) и «фиксированный порядок склейки» + (§1.3.3) — оба однозначно проверяемые юнитом, доказательство названо. +- AC1 (`styles.ts` ≤ 40 строк, только импорты+экспорт) реалистичен и легко + проверяем ревью кода. +- Порядок слайсов (chrome → dialogs → devices → plan → base) обоснован в теле + issue и ТЗ («от изолированной поверхности к связной») и подтверждён + наблюдением: у тулбаров/чекбоксов кластеров существенно меньше пересечений + с остальными зонами, чем у сцены плана. +- Риск «скрытая зависимость каскада» (§4.1) назван прямо и с механизмом + устранения (перенос конфликтующего правила в `base` на прежнюю + относительную позицию) — это корректно закрывает случай межзонного + конфликта specificity внутри одного и того же слайса; отдельно не закрывает + M1 (это про типы состояний, не про порядок). +- Release-артефакты: `docs/ARCHITECTURE.md` — реальный кандидат на правку + (дерево модулей уже документирует `editor-secondary.styles.ts` по такому же + шаблону), CHANGELOG обоснованно не трогается (User-Visible: no, инвариант + «пиксели не меняются» делает это законным, а не обходом правила). +- Технический вопрос (порядок промежуточной склейки на слайсах 1–4, до того + как все пять файлов существуют) не эскалирован владельцу и решён ревью: + относится к «всё, чего пользователь не наблюдает» (PROCESS.md §7.1), автор + и ревьюер вправе договориться сами. Договорённость: относительный порядок + внутри промежуточного массива на каждом слайсе сохраняет исходную позицию + ещё не вынесенных правил, что и так неявно требуется инвариантом 1 и + ловится golden на каждом шаге (кроме состояний из M1). + +## Чего не проверял + +- Тяжёлые гейты (`npm test`, `npm run build`, `golden:verify`, + `model-invariants`, browser-smoke целиком) — не гонял: на этапе ревью ТЗ + продуктового кода ещё нет, гонять их не по чему; это не пропуск, а + соразмерность этапу (PROCESS.md §8). +- Точный построчный состав будущих пяти файлов — не пересчитывал вручную + построчно, кроме грубой эвристической оценки для L2; финальное распределение + проверяется код-ревью по факту переноса. +- Мутационный гейт (`scripts/mutation-gate.mjs`) — существует в проекте и + запускается стабильным релизом, не гейтом ревью; ТЗ корректно ссылается на + него в §7 как на «якоря вне styles.ts», не требуя отдельного прогона здесь. + +## Унаследовано из предыдущих раундов + +Не применимо — это первый заход (r1), предыдущих раундов нет. + +--- + +**Вердикт: жёлтый · заход r1 · блокирующих циклов 0/4 · High: 0 · Medium: 1 → в задаче**