Files
houseplan-card/docs/reviews/SPEC-REVIEW-651-r1.md
T
2026-09-26 00:14:38 +00:00

20 KiB
Raw Blame History

SPEC-REVIEW-651-r1

Issue: #651 — «2.5D: значки устройств смещаются при зуме и меняют взаимное выравнивание относительно 2D» Этап: spec (ревью ТЗ, PROCESS.md §2.4) Заход: r1 · блокирующих циклов израсходовано (после этого вердикта): 1 из 4 Материал раунда: тело issue #651, раздел ## ТЗ r1 (снято на момент ревью, 2026-09-26); связанные issue #644 (эпик-предшественник, closed), #570/#583/#585 (Stage 4); канонические документы docs/SCOPE.md, docs/ISOMETRIC.md, docs/USER-GUIDE.ru.md; код src/iso-overlays.ts, src/iso-scene-render.ts, тесты test/iso-overlays.test.mjs, test/iso-scene-render.test.mjs, perf-фикстуры demo/performance/budgets-large-house-isometric.json, demo/performance/isometric-stage3-dense-fixture.mjs.

Вердикт

Жёлтый. High: 0 · Medium: 1 (в скоупе, возвращается автору) · Low: 0.

Единственная находка — устаревшая терминология «экспериментальный 2.5D-вид» и вытекающая из неё инструкция по changelog, которые противоречат canonical-документам после Stage 6 (#649): 2.5D — публичная настройка, не альфа-функция. Остальная часть ТЗ — обязательные разделы §7.1, однозначность AC, способ доказательства каждого AC, явный блок технических предположений — выполнена корректно и не требует правок.

Скоуп проверки

Проверено соответствие ТЗ:

  1. docs/SCOPE.md — какую строку Core user jobs закрывает задача;
  2. обязательным разделам ТЗ по PROCESS.md §7.1 (сценарий, что человек увидит, проблема, скоуп/не-скоуп, поведенческий контракт, UX, данные/миграция, i18n, AC1…ACn с доказательством, план автотестов, риски, откат, release-артефакты);
  3. однозначности и проверяемости каждого AC;
  4. терминологии — против docs/USER-GUIDE.ru.md и docs/ISOMETRIC.md (обязательное чтение по инструкции ревью для видимого поведения);
  5. фактической продуктовой истории вопроса — прочитан эпик #644 целиком (включая Q1–Q10 и ответы дизайнера), чтобы понять, откуда в ТЗ #651 взято решение о групповом сдвиге, пределе 48 CSS px и запрете уменьшения, и не противоречит ли ТЗ #651 более раннему явному ответу по деградации;
  6. существования артефактов, на которые ссылаются AC и план автотестов (файлы тестов, perf-фикстуры, golden-набор), — чтобы отличить проверяемое ТЗ от ссылок на несуществующий код.

Как проверялось

  • Тело issue #651 получено через gh issue view 651 --json body (MCP get_issue/get_issue_comments были недоступны без разрешения пользователя в этой сессии — использован gh CLI как эквивалентный путь получения того же публичного API).
  • Прочитаны docs/SCOPE.md, docs/process/REVIEWER.md, docs/ISOMETRIC.md целиком, фрагмент docs/USER-GUIDE.ru.md (раздел про объёмный вид плана, строки 336–346), docs/STATUS.md (строка про 2.5D View).
  • Прочитан целиком эпик #644 (тело + все комментарии, включая ответы дизайнера singlmolt-prog, association NONE, — оценка авторства подтверждена через gh api .../issues/644/comments --jq '.[] | {user, association}').
  • grep/glob по репозиторию: подтверждено существование resolveIsoOverlayPlacement, resolveIsoOverlayCollisions, buildIsoOverlayRenderScene в src/iso-overlays.ts/src/iso-scene-render.ts; подтверждено существование perf-фикстур large-house-isometric и isometric-stage3-dense, упомянутых в AC8; прочитан фрагмент test/iso-scene-render.test.mjs (тест #473 W3), чтобы понять характер существующих ожиданий по зуму, на которые ссылается «Аналитика».
  • Код продукта не менялся, гейты (tsc/test/build) не запускались — на этапе spec они не относятся к предмету ревью (ревью ТЗ, не диффа).

Находки

Medium (в скоупе) — устаревшая формулировка «экспериментальный 2.5D-вид» противоречит текущему статусу функции

Файл: тело issue #651, раздел ## ТЗ r1, п. 1 «Пользовательский сценарий» и п. 13 «Release-артефакты».

Воспроизведение:

  • п. 1 сценария: «Пользователь смотрит готовый план в View или kiosk… включает экспериментальный 2.5D-вид…»
  • п. 13 release-артефактов: «CHANGELOG.md и CHANGELOG.ru.md: исправление дрейфа/распада раскладки значков в 2.5D (User-Visible: yes). Не раскрывать способ включения экспериментальных функций.»

Это прямо расходится с canonical-документами, которые инструкция ревью требует читать для видимого поведения:

  • docs/ISOMETRIC.md:7-8: «Since Stage 6 (#649) the 2.5D View is a public mode» и далее (docs/ISOMETRIC.md:10-28, раздел «Activation»): «One installation-wide setting, settings.volumetric_view: boolean, switched in General settings › Display › Show the plan in 2.5D… There is no toggle on the card and no alpha entry: iso is gone from LABS_FLAGS… The hp_alpha switch itself remains as a mechanism without experiments.»
  • docs/USER-GUIDE.ru.md:338-346: «Объёмный вид плана включает администратор один раз: Общие настройки → Отображение → Объёмный вид плана (2.5D)…» — раздел написан для конечного пользователя, без единого упоминания «экспериментальный» или «альфа».
  • docs/STATUS.md:42-43: релиз-кандидат beta.3 явно описан как «It makes 2.5D a public opt-in setting (#649)», «#649 Stage 6 makes it public… replaces the alpha entry, the header toggle and the phone-menu item».

Почему это находка, а не стилистика. Формулировка «экспериментальный» — не безобидная деталь сценария: из неё прямо выведена инструкция для release-артефакта («не раскрывать способ включения экспериментальных функций»). Скрывать нечего: активация — документированная публичная настройка в общем разделе UI, уже описанная в USER-GUIDE.ru.md открытым текстом с названием пункта меню. Если разработчик буквально выполнит инструкцию п. 13, changelog получится обеднённым или обфусцированным без причины — вместо обычной практики называть публичную настройку по имени, как делают другие записи changelog про этот же режим (см. docs/STATUS.md:42, где #649 описан открыто). Формулировка также немного искажает картину аудитории бага для читателя ТЗ: это не редкий альфа-путь, а обычный публичный тумблер, доступный любому администратору.

Чем закрывается. Заменить «экспериментальный 2.5D-вид» на терминологию USER-GUIDE.ru.md («объёмный вид плана (2.5D)» / «Показать план в 2.5D» — как в интерфейсе), убрать из п. 13 инструкцию «не раскрывать способ включения» (либо переформулировать в нейтральное «changelog не описывает внутренний контракт наложений/кэша», если цель была не про активацию, а про технический контракт — сейчас не читается однозначно).

Класс: Medium, в скоупе задачи — правится прямо в теле ТЗ, отдельный issue не заводится (#202).

Что проверено и корректно

  • Обязательные разделы §7.1 — все присутствуют: сценарий (п.1, с «до → после одной фразой»), проблема (вводный ## Проблема/## Наблюдаемое поведение), скоуп/не-скоуп (п.3–4), поведенческий контракт (п.5.1–5.5), UX (п.6), данные/миграция (п.7), локализация (п.8), AC1–AC9 с доказательством (п.9), план автотестов (п.10), риски (п.11), откат (п.12), release-артефакты (п.13). Дополнительно указаны затрагиваемые модули (п.14) и явный блок технических предположений (п.15) — точно то, что требует PROCESS.md §7.1 для «размытых мест».
  • Соответствие docs/SCOPE.md. Задача чинит J1/J2/J3 в рамках уже принятого исключения #89 («deterministic 2.5D presentation of the existing canonical plan… same geometry»); новой модели, камеры, настройки или редактора не вводит. Согласуется с «Не входит» (п.4 ТЗ), явно исключающим новую настройку/схему/ручное позиционирование.
  • Однозначность и доказуемость AC1–AC9. Каждый AC называет конкретный тестовый файл/тип проверки (test/iso-overlays.test.mjs, test/iso-scene-render.test.mjs, browser smoke, Linux golden, exact-SHA perf) и наблюдаемый критерий (компоненты групп, единый вектор, инвариантность к zoom/pan, предел 48 px, совпадение visual/hit root, отсутствие побочных изменений). Ни один AC не сформулирован как самоцель реализации («сделать функцию X») — все формулируют наблюдаемый результат.
  • Продуктовая история вопроса подтверждена по первоисточнику. Эпик #644 (тело + комментарии) прочитан целиком: групповой сдвиг (Q1), предел 48 CSS px и запрет уменьшения (Q5, первая часть) — были явно приняты автором задачи (дизайнером) как продуктовое решение и корректно перенесены в ТЗ #651 (п.5.3.6: «Принятое в #644 решение о групповом сдвиге, пределе 48 px и запрете уменьшения сохраняется»).
  • Пересмотр деградации (§5.3.5–6) — рассмотрен отдельно и принят. #644 Q5 ранее фиксировал другую деградацию при переполнении 48 px («делится на одиночные значки»); ТЗ #651 заменяет это на «группа остаётся жёсткой, допускается остаточное наложение», обосновывая более поздним явным требованием исходного бага («если несколько значков в 2D находились на одной оси… они также остаются на одной оси… повторные зум и переключение не должны накапливать смещение» — это формулировка из раздела «Ожидаемое поведение» самого issue, а не собственная догадка автора ТЗ). Разбор: это пограничный случай «что считать приемлемой деградацией» (категория, которая по §7.1 обычно эскалируется владельцу), но здесь он (а) прямо выводится из уже сформулированного в issue требования, а не придуман автором ТЗ, (б) явно раскрыт в трёх местах согласованно — поведенческий контракт (5.3.5–6), UX (п.6) и блок технических предположений (п.15.5) с пометкой «ревьюер вправе оспорить», а не спрятан как решённый факт. Технический спор автора ТЗ и ревьюера по такого рода пункту по PROCESS.md §7.1 решается вердиктом ревьюера — вывод: обоснование достаточно, менять не требую.
  • Отсутствие открытых продуктовых вопросов проверено, а не принято на слово. Кроме разобранного выше пункта о деградации, других мест, где догадка выдаётся за факт, не найдено.
  • Артефакты, на которые ссылаются AC и план автотестов, существуют. resolveIsoOverlayPlacement, resolveIsoOverlayCollisions, buildIsoOverlayRenderScene — реальные экспорты src/iso-overlays.ts/src/iso-scene-render.ts; perf-бюджеты large-house-isometric (demo/performance/budgets-large-house-isometric.json) и isometric-stage3-dense (demo/performance/isometric-stage3-dense-fixture.mjs, demo/performance/budgets-isometric-stage3-dense.json) существуют и совпадают с именами в AC8 — это не выдуманные ссылки. demo/smoke_isometric_overlay_stability.mjs из плана автотестов (п.10.3) пока не существует, но ТЗ прямо говорит «добавить/расширить», это не находка.
  • Отсутствие схемы/миграции (п.7) соответствует действительности: изменение — исключительно runtime-presentation слой (iso-overlays.ts/iso-scene-render.ts), схема конфигурации не затрагивается.
  • i18n (п.8) — новых строк нет, что верно для чисто геометрического фикса без новых UI-элементов.
  • Откат (п.12) и golden-протокол (AC7) сформулированы в соответствии с процессом: новый baseline — только из reviewed Linux artifact, не из author-коммита.

Чего не проверял

  • Не запускал tsc/npm test/npm run build — на этапе spec предмет ревью текст ТЗ, а не код; кода изменений ещё нет (ветка issue/651-iso-device-layout-stability создана от origin/dev, но не содержит правок реализации на момент ревью).
  • Не проверял руками поведение в браузере (нет реализации, нечего запускать) — это работа код-ревью следующего этапа.
  • Не читал test/iso-overlays.test.mjs построчно целиком (только структуру и заголовки, плюс точечно test/iso-scene-render.test.mjs) — на этапе ТЗ этого достаточно, чтобы убедиться, что упомянутые в АС файлы и функции существуют; полный аудит тестов на «умеет падать» — задача код-ревью (§2.7).
  • Не проверял вторую часть ответа на Q5 в #644 («уменьшение группы котельной до 86% было костылём лаборатории, в продукт не переносится») на предмет отдельных скрытых противоречий за пределами прямого предмета #651 — вне скоупа этой задачи.
  • Полные наборы golden/perf/smoke не прогонял — на этом этапе кода для прогона ещё нет; это часть п.10 «План автотестов» и войдёт в работу при реализации (§2.6) и последующее код-ревью (§2.7).

Рекомендация автору

Одна точечная правка текста ТЗ (терминология + инструкция п.13) — не требует нового цикла анализа по существу контракта. После правки формулировки задача готова к статусу «Готово к разработке» без дополнительного захода по существу поведенческого контракта.


Материал раунда

  • Ветка: issue/651-iso-device-layout-stability, коммит ec2b8c15220f — ребейз его осиротит, и это нормально: ниже якоря, которые ребейз не меняет.
  • Дерево материала: a6750c5fd9b9b80f3a4f0f7b1642a5148c938b6f
    git log --all --format='%H %T' | grep a6750c5fd9b9
    
  • Тело issue: ea257bbcd4d329123e5400e69da0f9fb7460dfd29bcd0ddd2871e5e0a81b9e97
  • Вердикт конвейера: yellow · High 0