diff --git a/docs/reviews/INDEX.md b/docs/reviews/INDEX.md index 415fd0d2..40118b9b 100644 --- a/docs/reviews/INDEX.md +++ b/docs/reviews/INDEX.md @@ -1,9 +1,10 @@ # Индекс ревью -Генерируется `node scripts/reviews-index.mjs` (#635) — не редактировать руками. Документов: 1064, issue: 375. Вердикт: 🟢 зелёный · 🟡 жёлтый · 🔴 красный · ⚪ не распознан (свободная форма старых документов). H/M — число High/Medium по строке вердикта или заголовкам находок. Файлы — пути, названные в находках; ищите по имени файла: `grep form-kit INDEX.md`. +Генерируется `node scripts/reviews-index.mjs` (#635) — не редактировать руками. Документов: 1065, issue: 376. Вердикт: 🟢 зелёный · 🟡 жёлтый · 🔴 красный · ⚪ не распознан (свободная форма старых документов). H/M — число High/Medium по строке вердикта или заголовкам находок. Файлы — пути, названные в находках; ищите по имени файла: `grep form-kit INDEX.md`. | Issue | Документ | Этап · раунд | Вердикт | H | M | Находки | Файлы | |---|---|---|---|---:|---:|---|---| +| #651 | [SPEC-REVIEW-651-r1.md](SPEC-REVIEW-651-r1.md) | spec · r1 | 🟡 жёлтый | 0 | 1 | устаревшая формулировка «экспериментальный 2.5D-вид» противоречит текущему статусу функции | `CHANGELOG.md` `CHANGELOG.ru.md` `docs/ISOMETRIC.md` `docs/USER-GUIDE.ru.md` `docs/STATUS.md` `USER-GUIDE.ru.md` | | #650 | [CODE-REVIEW-650-r1.md](CODE-REVIEW-650-r1.md) | code · r1 | 🟢 зелёный | 0 | 0 | — | — | | #649 | [SPEC-REVIEW-649-r1.md](SPEC-REVIEW-649-r1.md) | spec · r1 | 🟡 жёлтый | 0 | 1 | в скоупе задачи (возвращается автору); принято ревьюером с записью, правки не требует | `lab.js` `houseplan-card.ts` | | #649 | [SPEC-REVIEW-649-r2.md](SPEC-REVIEW-649-r2.md) | spec · r2 | 🟢 зелёный | 0 | 0 | — | — | diff --git a/docs/reviews/SPEC-REVIEW-651-r1.md b/docs/reviews/SPEC-REVIEW-651-r1.md new file mode 100644 index 00000000..5c303b78 --- /dev/null +++ b/docs/reviews/SPEC-REVIEW-651-r1.md @@ -0,0 +1,90 @@ +# SPEC-REVIEW-651-r1 + +**Issue:** [#651](https://github.com/Matysh/houseplan-card/issues/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