From 4b8a82c08a7eb92ee1cdbc1acdd37ab1f87adc91 Mon Sep 17 00:00:00 2001 From: "claude[bot]" <209825114+claude[bot]@users.noreply.github.com> Date: Sat, 26 Sep 2026 07:23:13 +0000 Subject: [PATCH] docs: review document for #660 Issue: #660 User-Visible: no --- docs/reviews/INDEX.md | 3 +- docs/reviews/SPEC-REVIEW-660-r1.md | 324 +++++++++++++++++++++++++++++ 2 files changed, 326 insertions(+), 1 deletion(-) create mode 100644 docs/reviews/SPEC-REVIEW-660-r1.md diff --git a/docs/reviews/INDEX.md b/docs/reviews/INDEX.md index 07b63190..8d805454 100644 --- a/docs/reviews/INDEX.md +++ b/docs/reviews/INDEX.md @@ -1,9 +1,10 @@ # Индекс ревью -Генерируется `node scripts/reviews-index.mjs` (#635) — не редактировать руками. Документов: 1072, issue: 378. Вердикт: 🟢 зелёный · 🟡 жёлтый · 🔴 красный · ⚪ не распознан (свободная форма старых документов). H/M — число High/Medium по строке вердикта или заголовкам находок. Файлы — пути, названные в находках; ищите по имени файла: `grep form-kit INDEX.md`. +Генерируется `node scripts/reviews-index.mjs` (#635) — не редактировать руками. Документов: 1073, issue: 379. Вердикт: 🟢 зелёный · 🟡 жёлтый · 🔴 красный · ⚪ не распознан (свободная форма старых документов). H/M — число High/Medium по строке вердикта или заголовкам находок. Файлы — пути, названные в находках; ищите по имени файла: `grep form-kit INDEX.md`. | Issue | Документ | Этап · раунд | Вердикт | H | M | Находки | Файлы | |---|---|---|---|---:|---:|---|---| +| #660 | [SPEC-REVIEW-660-r1.md](SPEC-REVIEW-660-r1.md) | spec · r1 | 🔴 красный | 2 | 1 | Раздел ## ТЗ в теле issue отсутствует целиком; Изменение прямо противоречит двум местам; AC «расстояние уменьшено ровно вдвое» не | `docs/process/AUTHOR.md` `REVIEWER.md` `test/core-file-budget.test.mjs` `scripts/smoke-select.mjs` `demo/helpers/hp-test.mjs` `docs/UX-MODES.md` `docs/reviews/SPEC-REVIEW-647-r1.md` | | #656 | [CODE-REVIEW-656-r1.md](CODE-REVIEW-656-r1.md) | code · r1 | 🟢 зелёный | 0 | 0 | — | — | | #654 | [SPEC-REVIEW-654-r1.md](SPEC-REVIEW-654-r1.md) | spec · r1 | 🟡 жёлтый | 0 | 1 | «Release-артефакты» не называют обновление docs/ISOMETRIC.md | `docs/ISOMETRIC.md` `docs/CHANGELOG.md` `docs/CHANGELOG.ru.md` `docs/reviews/INDEX.md` | | #654 | [SPEC-REVIEW-654-r2.md](SPEC-REVIEW-654-r2.md) | spec · r2 | 🟢 зелёный | 0 | 0 | — | — | diff --git a/docs/reviews/SPEC-REVIEW-660-r1.md b/docs/reviews/SPEC-REVIEW-660-r1.md new file mode 100644 index 00000000..a209b9f6 --- /dev/null +++ b/docs/reviews/SPEC-REVIEW-660-r1.md @@ -0,0 +1,324 @@ +# SPEC-REVIEW-660-r1 + +- **Issue:** https://github.com/Matysh/houseplan-card/issues/660 +- **Этап:** `S4-spec-review` (ревью ТЗ, PROCESS.md §2.4) +- **Трек:** полный (аналитика автора обосновывает отказ от `small`: >1 + поверхности — layout шапки + summary-runtime/mobile-меню + клавиатурный + автомат Esc — и новый UX-контракт положения крестика и обработки `Esc`) +- **Материал:** тело issue #660 целиком (создано 2026-09-26T06:50:32Z, + событий `edited` в таймлайне issue нет — тело не менялось с момента + создания); комментарии: «Аналитика» (`#issuecomment-5844066158`) и ответ + владельца на Q1/Q2 (`#issuecomment-5844172216`) +- **Заход:** r1 · блокирующих циклов израсходовано 0 из 4 (полный трек) +- **Роль:** ревьюер ТЗ (не автор) + +## Скоуп ревью + +Задача — полировка шапки основной панели после #647: (1) inline-кнопки +сводной панели остаются видимыми/активными во всех трёх редакторах на +ширинах >480 px; (2) расстояние между блоком кнопок редакторов и блоком +зума уменьшается вдвое; (3) резерв под крестик закрытия переезжает внутрь +общей подложки блока редакторов и всегда стоит сразу после активной +кнопки; (4) `Esc` получает единый терминальный выход в View из всех трёх +редакторов, если нет более приоритетного слоя/действия. + +Проверялось: наличие и полнота обязательных разделов ТЗ (PROCESS.md §7.1), +однозначность и проверяемость каждого AC вместе со способом доказательства +(§2.5 DoR), соответствие описания коду «как есть», согласие с +`docs/SCOPE.md` и каноническим документом подсистемы (`docs/UX-MODES.md`), +отсутствие домыслов, выданных за факт. + +## Как проверялось + +1. Прочитаны `docs/SCOPE.md`, `AGENTS.md`, `docs/process/REVIEWER.md` и + разделы PROCESS.md §2.3, §2.4, §2.5, §4, §7.1, §7.2. +2. Прочитано тело issue #660 целиком и оба комментария. +3. Сверена история меток issue (`gh api .../timeline`): `S1-new` → + `S2-analysis` (06:52:45) → `S3-spec` + `blocked` (06:54:38, тем же + моментом, что и комментарий «Аналитика») → владелец отвечает на Q1/Q2 + (07:12:46) → `S3-spec`/`blocked` сняты, `S4-spec-review` поставлена + (07:12:48, через 2 секунды после ответа владельца). Событий `edited` + для тела issue в таймлайне нет — значит тело **никогда не + редактировалось** после первичного создания владельцем в 06:50:32. +4. Сопоставлен формат #660 с образцовым ТЗ того же полного/лёгкого трека — + `docs/reviews/SPEC-REVIEW-647-r1.md` и текстом раздела `## ТЗ` в теле + issue #647 (Сценарий, Что человек увидит до/после, Проблема по коду, + Скоуп/Не-скоуп, Контракт поведения К1…К7, UX/i18n/модель данных, таблица + «AC · чем доказан · чем краснеет», План автотестов, Риски, Откат, + Release-артефакты, «Принято предположительно»). +5. Прочитан `docs/UX-MODES.md` целиком (канонический документ подсистемы, + явно указан для этой задачи) — строки 56-61 и 116-125 описывают именно + ту архитектуру, которую #660 заменяет. +6. Прочитан код, который ТЗ описывает как причину бага: + `src/summary-panel-runtime-loaded.ts:262-263` (`renderControls` + возвращает `nothing`, если `this.host._mode !== 'view'`), + `src/header-menu.ts:79` (пункты сводной панели добавляются только при + `input.mode === 'view'`), `src/houseplan-card.ts:10812-10829` + (`.editor-close-slot` — отдельный сосед `.modes`, между ними и + `.zoomctl` — `.spacer`), `src/styles/chrome.styles.ts` (раздельные gap + на `.modes`/`.editor-close-slot`/`.zoomctl`) — описание проблемы в теле + issue совпадает с кодом. +7. Прочитан `src/houseplan-card.ts:2758-2992` (`_onKey`) — проверены + утверждения комментария «Аналитика» построчно: + - `decor` (Background): цепочка отмены действительно оканчивается + `else this._setMode('view')` (строка 2872) — уже выходит в View в + нейтральном состоянии, как и заявлено. + - `devices`: `if (e.key === 'Escape' && this._deviceDrag) { ... + _cancelDeviceDrag(); } return;` (2888-2892) — при отсутствии + активного drag `Escape` не делает ничего, выхода в View нет — + подтверждено. + - `plan` (`_markup`): цепочка (2936-2992 и далее) отменяет + draw/resize/split/merge/physicalSel и т.д., но ни одна ветка не + вызывает `_setMode('view')` — в нейтральном состоянии `Esc` тоже + ничего не делает — подтверждено. +8. Гейты не гонялись: этап `spec`, диапазон ревью — текст тела issue, а не + код. Ожидаемо для ревью ТЗ. + +## Находки + +### High-1 (в скоупе). Раздел `## ТЗ` в теле issue отсутствует целиком — +есть только исходная постановка владельца и два продуктовых ответа, +без единой из обязательных частей §7.1 + +**Файл:** тело issue #660 (весь документ, п.3 «Как проверялось» — снимок +таймлайна подтверждает отсутствие правок тела после создания). + +**Что не так.** Решение #517 и `docs/process/AUTHOR.md`/`REVIEWER.md` +фиксируют: ТЗ — это раздел `## ТЗ` в теле issue, который автор ТЗ пишет на +шаге «ТЗ в работе» (`S3-spec`), а не исходная жалоба владельца. Для +сравнения — прямой аналог того же класса задачи, #647, на том же полном +формате прошёл ровно этот шаг: после `---` в теле появился раздел `## ТЗ` +с Сценарий → Что человек увидит до/после → Проблема (по коду) → Скоуп/ +Не-скоуп → Контракт поведения (К1…К7) → UX/i18n/модель данных → таблица +«AC · чем доказан · чем краснеет» → План автотестов → Риски → Откат → +Release-артефакты → «Принято предположительно». + +В #660 таймлайн показывает, что `S3-spec` был выставлен **в тот же +момент**, что и комментарий «Аналитика» (06:54:38), тут же получил +`blocked` в ожидании ответа владельца, и сразу после ответа владельца +(07:12:46) — **через 2 секунды** (07:12:48) — был выставлен +`S4-spec-review`, без единой правки тела issue. То есть шаг «написать +полную первую редакцию по §7» (выход из `S3-spec`, PROCESS.md §2.3) не +состоялся: то, что попало на ревью — это исходная жалоба владельца +(«Проблема» / «Требуемое поведение» / «Принятые решения владельца» / +«Критерии приёмки» / «Регрессионные проверки» / «Связанные задачи» / +«Пользовательский результат»), без единой из следующих обязательных +частей §7.1: + +- **Сценарий** — какая персона, на какой поверхности, в какой момент, + явным разделом (сейчас это можно только реконструировать из контекста). +- **Что человек увидит до и после** — одной фразой без терминов + реализации, в паре «до»/«после» (сейчас есть только «Пользовательский + результат» — фраза о результате без явного «до»). +- **Не-скоуп** — ни слова о границах: не тронуты ли kiosk-заголовок, + диалоги, крестики `.barclose` нижних панелей редакторов, контракт View + overlay сводной панели вне ширины >480 px, порядок/действия других + кнопок шапки. +- **Модель данных и миграция** — не упомянуты вообще (даже как явное + «нет»), при том что «show/hide меняет сохранённый локальный выбор» — + это чтение/запись существующего состояния, которое стоило явно назвать. +- **i18n** — не упомянут вообще: новых ключей нет или переиспользуются + старые (`title.close_editor` и т.п.) — это нигде не сказано явно. +- **Способ доказательства для каждого AC** — отсутствует полностью. Ни + одна из 10 строк чек-листа «Критерии приёмки» не помечена + `unit`/`backend`/`smoke`/`golden`/«ревью кода», что прямо требуется + DoR §2.5 («у каждого указано, чем он доказывается») и является одним из + двух пунктов, которые ревьюер ТЗ обязан проверить (§2.4, §7.1). Раздел + «Регрессионные проверки» называет типы смоков в общем виде («Browser + layout smoke», «Keyboard smoke»), но не привязывает ни один из них к + конкретному AC и не называет ни одного файла/механизма (в отличие от + ТЗ #647, где план автотестов ссылается на конкретные + `test/core-file-budget.test.mjs`, `scripts/smoke-select.mjs`, + `demo/helpers/hp-test.mjs`, фасад `data-hp="mode-tab"`). +- **Риски** — не названы. Не оценён риск golden-регресса (шапка почти во + всех кадрах меняется третий раз подряд после #647/#195), риск задеть + общий CSS-токен gap (текущее расстояние `.modes` → `.zoomctl` — не один + `gap`, а слот + два межэлементных gap + `.spacer`, по признанию самой + «Аналитики»; если хотя бы один из участвующих CSS-токенов переиспользуется + в другом месте шапки/диалогов, «уменьшить вдвое» механической правкой + переменной заденет то, что не должно меняться), риск сломать + существующие смоки #647/#195, которые ищут крестик в `.editor-close-slot` + как отдельного соседа `.modes`, а не как элемент внутри общей подложки. +- **Откат** — не назван (обычно тривиален — `git revert» — но должен быть + явно написан, а не подразумеваться). +- **Release-артефакты** — не названы: не сказано, что `User-Visible: yes` + требует правки обоих changelog в том же коммите, и не назван + `docs/UX-MODES.md` (см. High-2 ниже). +- **«Принято предположительно, поменять свободно»** — блок отсутствует, а + нерешённых технических деталей достаточно: точный CSS-токен(ы), + участвующие в «уменьшить вдвое» (общий или новый), конкретные ширины, + на которых проверяется расстояние (текущее расстояние differs: 100 px на + 768–1400 px и 81 px на 481–620 px по данным самой «Аналитики» — ТЗ не + говорит, должны ли оба значения быть независимо разделены пополам или + сведены к одному новому), формат сдвига кнопок правее активной (анимация + или мгновенно), что именно считается «активным отменяемым действием + инструмента» для точной границы приоритета в K8/AC8 сверх трёх примеров + из текста. + +Эффект: ни один из 10 AC нельзя признать проверяемым в смысле §2.4/§2.5 — +не потому, что формулировки плохи (большинство однозначны как продуктовое +намерение), а потому, что для каждого из них отсутствует обязательная +часть требования — способ доказательства. Это не редакционная мелочь: это +именно тот шаг ревью ТЗ, который явно поручен ревьюеру («Проверь +обязательные разделы §7.1, однозначность каждого AC и указание способа +доказательства»), и по нему обязательные части не выполнены ни на одну +из десяти строк. + +**Как закрыть.** Автору ТЗ нужно фактически выполнить шаг `S3-спецификация` +(PROCESS.md §2.3) — дописать раздел `## ТЗ` в теле issue по образцу +`docs/reviews/SPEC-REVIEW-647-r1.md`/тела issue #647: Сценарий · Что +человек увидит до/после · Проблема (по коду, с конкретными строками) · +Скоуп/Не-скоуп · Контракт поведения (по каждому из 4 пунктов «Требуемого +поведения») · UX/i18n/модель данных · таблица AC с указанием способа +доказательства (и, где применимо, «чем краснеет») · План автотестов с +именами существующих механизмов (`scripts/smoke-select.mjs`, +`demo/helpers/hp-test.mjs`, фасад `data-hp`) · Риски · Откат · +Release-артефакты (см. High-2) · «Принято предположительно». + +### High-2 (в скоупе). Изменение прямо противоречит двум местам +канонического `docs/UX-MODES.md`, а обновление этого документа нигде не +запланировано + +**Файл:** `docs/UX-MODES.md:56-61`, `docs/UX-MODES.md:116-125`; тело +issue #660 (раздел «Release-артефакты» отсутствует — см. High-1). + +**Что не так.** Действующий канон дословно описывает архитектуру, которую +#660 меняет: + +> its own fixed 24 × 24 px slot right after the mode tabs (#647): the slot +> keeps its size, empty and hidden from assistive technology, **outside the +> editors** (docs/UX-MODES.md:57-58) + +— #660 переносит крестик внутрь общей подложки блока редакторов и требует, +чтобы он «всегда располагался непосредственно после активной кнопки» +(т.е. позиция внутри группы меняется в зависимости от режима) — это прямо +не «fixed slot right after the mode tabs», а другая, режимо-зависимая +компоновка. + +> All summary surfaces disappear in Plan, Devices and Background; +> `houseplan-space-card` never renders them. (docs/UX-MODES.md:124-125) + +— это ровно то предложение, которое AC1 из #660 отменяет для широких +экранов (inline-кнопки остаются видимыми и активными во всех трёх +редакторах). После реализации #660 это предложение канона станет неверным +буквально в момент мержа, и ни один раздел ТЗ (потому что раздела +«Release-артефакты» просто нет — High-1) не требует его исправить — +прецедент такого требования уже был для этой же подсистемы: SPEC-REVIEW- +647-r1 находкой Medium-2 обязал автора включить `docs/UX-MODES.md` в +Release-артефакты и обновить обе строки про крестик; ТЗ #660 повторяет +тот же пробел на том же документе. + +**Как закрыть.** Добавить в ТЗ явную правку `docs/UX-MODES.md`: заменить +строки 57-58/60-61 описанием новой, режимо-зависимой позиции крестика +внутри общей подложки (без «outside the editors» и без «fixed... right +after the mode tabs» в прежнем смысле) и заменить строку 124 указанием, +что на ширинах >480 px inline-кнопки сводной панели остаются активными в +Plan/Devices/Background, а сам overlay — по-прежнему нет. Внести +`docs/UX-MODES.md` в Release-артефакты рядом с changelog. + +### Medium-1 (в скоупе). AC «расстояние уменьшено ровно вдвое» не +называет допуск и не привязан к конкретным ширинам, при том что исходное +расстояние неоднородно по брейкпоинтам + +**Файл:** тело issue #660, раздел «Критерии приёмки», пункт 2; сверено с +комментарием «Аналитика» (`#issuecomment-5844066158`, п.3). + +**Что не так.** Формулировка — «расстояние… уменьшено ровно вдвое +относительно текущего `dev`» — не указывает допустимую погрешность (для +сравнения, соседний AC о ширине блока редакторов прямо называет «допуск +не более 1 CSS px»). Собственный разбор автора в «Аналитике» установил, +что на `dev` это расстояние **не одно число**: 100 px на ширинах +768/1000/1200/1400 px и 81 px на 481–620 px — то есть при буквальном +прочтении AC нужно раздельно проверять двоение к 50 px и к 40.5 px +(дробный px) на разных диапазонах ширины, либо свести к единому новому +значению, а ТЗ не говорит, какой из двух вариантов верен, и не задаёт +допуск для сравнения с дробным px. Без этого браузерный смок, +который должен ловить регресс, не может быть написан однозначно — два +разных исполнителя закономерно напишут два разных (оба «соответствующих +тексту ТЗ») смока с разными числовыми порогами. + +**Как закрыть.** В ТЗ явно перечислить целевые значения по диапазонам +ширины (например: 50 px на 768–1400 px, 40 px на 481–620 px, округление +до ближайшего целого CSS px) и указать допуск (по аналогии с другими AC — +не более 1 CSS px). + +## Что проверено и корректно + +- Аналитика корректно относит задачу к J4/J6 и desktop-first контракту + редакторов (`docs/SCOPE.md`) — это обоснованный polish существующего, + уже принятого механизма (#647), а не новая функция вне скоупа продукта. +- Отказ от лёгкого трека обоснован верно: задействовано больше одной + поверхности (layout шапки, summary-runtime/мобильное меню, клавиатурный + автомат `Esc`) и вводится новый UX-контракт положения крестика — оба + критерия §5 нарушены, полный трек — правильный выбор. +- Оба вопроса владельцу (Q1, Q2) — продуктовые по существу («что видит и + делает пользователь при клике внутри редактора», «меняется ли состав + мобильного меню»), поставлены с предлагаемым вариантом по умолчанию, как + требует §7.1; отдельных технических вопросов, вынесенных владельцу + неправомерно, не найдено. +- Утверждения о текущем коде, на которых строится обоснование задачи, + проверены и подтвердились дословно: `renderControls()` гасится вне View + (`src/summary-panel-runtime-loaded.ts:263`), пункты сводной панели в + мобильном меню гасятся так же (`src/header-menu.ts:79`), + `.editor-close-slot` — отдельный сосед `.modes` со `.spacer` перед + `.zoomctl` (`src/houseplan-card.ts:10812-10829`); поведение `Esc` в + каждом из трёх редакторов (`_onKey`, `src/houseplan-card.ts:2825-2992`) + соответствует описанному: `decor` уже выходит в View в нейтральном + состоянии, `devices` и `plan` — нет. +- Критерии приёмки как продуктовые формулировки (без учёта отсутствующего + способа доказательства, см. High-1) в основном однозначны и не содержат + скрытых домыслов, кроме AC о расстоянии (Medium-1). +- «Регрессионные проверки» верно называют, что нужно защитить: контракт + #647 (стабильная ширина) и #195 (hit-target крестика) — упомянуты явно + и не ослабляются на словах. + +## Чего не проверял + +- Гейты (`tsc`, `npm test`, `npm run build`, `check-docs.mjs`, смоки, + golden, инварианты) — не прогонял: этап `spec`, продуктового кода к этой + правке ещё нет, диапазон ревью — текст тела issue, а не диапазон + коммитов. Это предмет код-ревью после реализации. +- Не измерял в браузере фактическое расстояние `.modes` → `.zoomctl` и не + проверял независимо числа 100/81 px из «Аналитики» — принял их как + добросовестное измерение автора; при следующем заходе, если числа войдут + в исправленный ТЗ (Medium-1), стоит перепроверить смоком на заявленных + ширинах. +- Не оценивал производительность/анимацию перехода между режимами — ТЗ не + меняет этот механизм, вне скоупа задачи. +- Не проверял touch-контракт по `docs/TOUCH-SUPPORT.md` подробно за + пределами констатации, что редакторы остаются desktop-first/best-effort; + вопрос, нужно ли обновлять `docs/TOUCH-SUPPORT.md:74-79` (описание + сводной панели как «View surfaces»), не поднят ни ТЗ, ни этим ревью и + должен быть закрыт вместе с переписыванием раздела Release-артефакты. + +## Вердикт + +Найдено 2 High и 1 Medium, все в скоупе задачи. Раздел `## ТЗ` в теле +issue отсутствует целиком: то, что дошло до ревью, — исходная постановка +владельца плюс два продуктовых ответа, без единой из обязательных частей +§7.1 (Сценарий, Не-скоуп, модель данных/i18n, способ доказательства по +каждому AC, План автотестов, Риски, Откат, Release-артефакты, «Принято +предположительно»). Отдельно от этого системного пробела — конкретное +противоречие с каноническим `docs/UX-MODES.md` (крестик описан как +«fixed slot… outside the editors», сводная панель — как «disappears in +Plan, Devices and Background»), которое #660 меняет, но не планирует +исправить в документации, и отсутствие допуска/целевых чисел в AC о +расстоянии до блока зума. Красный вердикт: ТЗ возвращается автору на +полную первую редакцию раздела `## ТЗ` по образцу #647, отдельные issue +не заводятся. + +Вердикт: красный · заход r1 · блокирующих циклов 1/4 · High: 2 · Medium: 1 → в задаче + +--- + + + +## Материал раунда + +- Ветка: `dev`, коммит `9287f798b382` — ребейз его осиротит, и это нормально: ниже якоря, которые ребейз не меняет. +- Дерево материала: `268f8f32087b5c4866a4a6a695ae66bcfce7bc71` + ``` + git log --all --format='%H %T' | grep 268f8f32087b + ``` +- Тело issue: `70fd6272cab8ef94f29ce0fb31389d319d247b8c435d9eed3e4200c5e3d23cb0` +- Вердикт конвейера: `red` · High 2