docs: review document for #645

Issue: #645
User-Visible: no
This commit is contained in:
claude[bot]
2026-09-25 08:06:13 +03:00
committed by Sergey Matyunin
parent 744b4a8398
commit 15e047949e
2 changed files with 352 additions and 1 deletions
+2 -1
View File
@@ -1,10 +1,11 @@
# Индекс ревью
Генерируется `node scripts/reviews-index.mjs` (#635) — не редактировать руками. Документов: 1041, issue: 366. Вердикт: 🟢 зелёный · 🟡 жёлтый · 🔴 красный · ⚪ не распознан (свободная форма старых документов). H/M — число High/Medium по строке вердикта или заголовкам находок. Файлы — пути, названные в находках; ищите по имени файла: `grep form-kit INDEX.md`.
Генерируется `node scripts/reviews-index.mjs` (#635) — не редактировать руками. Документов: 1042, issue: 367. Вердикт: 🟢 зелёный · 🟡 жёлтый · 🔴 красный · ⚪ не распознан (свободная форма старых документов). H/M — число High/Medium по строке вердикта или заголовкам находок. Файлы — пути, названные в находках; ищите по имени файла: `grep form-kit INDEX.md`.
| Issue | Документ | Этап · раунд | Вердикт | H | M | Находки | Файлы |
|---|---|---|---|---:|---:|---|---|
| #646 | [CODE-REVIEW-646-r1.md](CODE-REVIEW-646-r1.md) | code · r1 | 🟢 зелёный | 0 | 0 | — | — |
| #645 | [SPEC-REVIEW-645-r1.md](SPEC-REVIEW-645-r1.md) | spec · r1 | 🟡 жёлтый | 0 | 2 | Отсутствует обязательная строка Touch editor: … из docs/TOUCH-SUPPORT.md → «Documentati…; ТЗ не учитывает существующую зависимость live-подписей Resize от позиции кнопки «Настро…; / Low | `docs/TOUCH-SUPPORT.md` `docs/reviews/SPEC-REVIEW-449-r1.md` `docs/specs/359-furniture-placement-preview.md` `docs/specs/449-double-fit-all.md` `src/houseplan-editor-runtime.ts` `src/houseplan-card.ts` `src/resize-labels.ts` `docs/process/REVIEWER.md` |
| #643 | [CODE-REVIEW-643-r1.md](CODE-REVIEW-643-r1.md) | code · r1 | 🟢 зелёный | 0 | 0 | — | — |
| #642 | [SPEC-REVIEW-642-r1.md](SPEC-REVIEW-642-r1.md) | spec · r1 | 🟢 зелёный | 0 | 0 | — | — |
| #642 | [CODE-REVIEW-642-r1.md](CODE-REVIEW-642-r1.md) | code · r1 | 🟢 зелёный | 0 | 0 | — | — |
+350
View File
@@ -0,0 +1,350 @@
# SPEC-REVIEW-645-r1
Issue: #645 — «Plan editor: временно перемещать кнопку "Настройки комнаты",
если она перекрывает элементы» (bug/polish, P2, полный трек — лёгкий трек
аналитика отклонила: нарушен критерий §5 «нет нового UX-контракта», плюс
задача трогает touch-контракт редактора).
Заход: r1 · блокирующих циклов израсходовано 0 из 4.
## Скоуп ревью
ТЗ живёт в теле issue #645, раздел `## ТЗ` (решение владельца 2026-09-10,
#517). Ревьюер читает только тело issue и комментарии, без устных пояснений
автора. Материал: тело issue на момент чтения (2026-09-24), метки `bug`,
`P2`, `polish`, `S4-spec-review`. Реализация ещё не начиналась (хендофф-
комментарий автора: «продуктовый код не менялся… реализация не начиналась»),
поэтому диапазон `origin/dev...HEAD` пуст для класса A/B — на этом этапе
проверяется только текст.
## Как проверялось
1. Прочитаны `docs/SCOPE.md`, `docs/process/REVIEWER.md`, `AGENTS.md`
(порядок из инструкции), `PROCESS.md` §2.3–§2.5, §2.10, §4, §7.1, §7.2 —
секции, определяющие объём и формат ревью ТЗ.
2. Прочитано полностью тело issue #645: секции до `## ТЗ` («Проблема»,
«Предлагаемое UX-решение», «Время жизни позиции», «Почему не только
автоматическое уклонение», «Ожидаемое поведение», «Проверки для будущего
ТЗ») и сам раздел `## ТЗ` целиком (Сценарий → «Принято предположительно»),
плюс оба комментария («Оценка…», «Взял…», «Сделано…»).
3. Сверены все 14 пунктов «Контракта поведения» и 10 AC на однозначность,
проверяемость и полноту покрытия — каждый пункт контракта закрыт минимум
одним AC (проверено построчным сопоставлением, таблица приведена ниже в
«Что проверено»).
4. Каждое фактическое утверждение ТЗ о текущем коде сверено с исходниками, а
не принято на слово:
- `_renderRoomGear()`, `poleOfInaccessibility(room.poly)`, `.rlgearbtn`,
`pointer-events: auto` — `src/houseplan-editor-runtime.ts:10276–10300`.
Подтверждено: центр — плановая точка `c`, транслируется в `left/top %`
через текущий `view` (`(c[0]-view.x)/view.w*100`), то есть модель
«временная позиция в координатах плана» технически реализуема поверх
существующего рендера без структурных изменений.
- «Существующий редакторский порог 3 CSS px» (контракт п.3) — найден и
подтверждён: `src/houseplan-card.ts:7031`,
`Math.abs(ev.clientX-drag.sx)+Math.abs(ev.clientY-drag.sy) > 3` в
`_pointerMoveNow` (device drag), `clientX/clientY` уже в CSS-пикселях,
от DPR/zoom не зависят — заявление ТЗ точное, не догадка.
- Существующий паттерн подавления synthetic click —
`_suppressClick`/`setTimeout(…, 0)` в `src/houseplan-card.ts` (строки
1905, 5598, 6708, 6804, 6903–6904, 6941, 7062–7063) — подтверждает, что
механизм из «Принято предположительно» («конкретный механизм… одноразового
подавления synthetic click») уже есть готовый прецедент в этом же файле.
- `pointInPolygon` существует (`src/logic.ts`, используется также в
`src/houseplan-editor-runtime.ts`, `src/houseplan-card.ts`) — ссылка ТЗ
в «Принято предположительно» не выдумана.
- Терминология кнопки (`room.settings_title` / `room.settings_short` =
«Настройки комнаты») сверена с `src/i18n/ru.json:765,800` и
`docs/USER-GUIDE.ru.md:1621` — совпадает, ТЗ не изобретает термин.
5. Проверено взаимодействие с соседним поведением, читающим позицию кнопки:
поиск `gearCenter`/`poleOfInaccessibility` по `src/**` вскрыл
`src/resize-labels.ts:25–27,97,113–116` и вызывающий код
`src/houseplan-editor-runtime.ts:3404–3435` (`_rszEdgeLabels` →
`placeResizeAreaLabel({ …, gearCenter: poleOfInaccessibility(poly), … })`)
— живая раскладка длин/площади при Resize стены уклоняется именно от
позиции кнопки «Настройки комнаты». См. находку M2.
6. Проверено `docs/TOUCH-SUPPORT.md` целиком, в частности «Documentation
rule» (строки 214–223: три обязательных значения `Touch editor: …`) —
применительно к тому, что #645 является «new editor feature
specification», трогающей touch-путь Plan editor. См. находку M1.
7. Проверены `docs/UX-MODES.md` (модель трёх режимов/вкладок редактора,
`_mode: 'view'|'plan'|'devices'|'decor'` в `src/houseplan-card.ts:851`) —
пункт контракта 12 («сессия… заканчивается при выходе из редактора
"План"») однозначно читается как «переключение на вкладку Устройств/
Подложки завершает сессию», конфликта или недосказанности не найдено.
8. Проверены обязательные разделы ТЗ по чек-листу `PROCESS.md` §7.1 —
присутствуют все: сценарий, что человек увидит, проблема, скоуп/не-скоуп,
контракт поведения, UX и доступность, модель данных/миграция/i18n, AC1–10
с доказательством, план автотестов, риски, откат, release-артефакты.
9. Проверен DoR-чек-лист `PROCESS.md` §2.5 пункт за пунктом (список — в
«Что проверено»).
Этап spec: тяжёлые/browser-гейты не запускались и не нужны — код ещё не
написан, ни один AC не реализован; проверке подлежит только текст ТЗ и его
непротиворечивость уже существующему коду.
## Находки
### Medium (в скоупе задачи — без High это жёлтый вердикт, правится в #645)
**M1. Отсутствует обязательная строка `Touch editor: …` из
`docs/TOUCH-SUPPORT.md` → «Documentation rule».**
`docs/TOUCH-SUPPORT.md:214–223` требует буквально для «new editor feature
specifications»: одно из трёх значений — `Touch editor: supported` /
`Touch editor: best effort / intentionally degraded` / `Touch editor: not
exposed`, — записанное явной строкой. ТЗ #645 добавляет именно новую
touch-возможность редактора (одиночный touch/pen drag капсулы кнопки,
контракт п.2/8, AC6) и в прозе объясняет, что редактор «остаётся best effort
по `docs/TOUCH-SUPPORT.md`», но нигде не содержит требуемую формальную
строку. Тот же формальный пробел уже разбирался ревью на #449 (закрыт
добавлением ровно одной строки в шапку ТЗ, `docs/reviews/SPEC-REVIEW-449-r1.md`
M1) — прецедент показывает, что это не придирка к форме, а работающий
механизм, который здесь просто не применён.
**Воспроизведение:**
```
grep -n "Touch editor:" <(gh issue view 645 --repo Matysh/houseplan-card --json body -q .body)
```
даёт пусто; в то же время `docs/specs/359-furniture-placement-preview.md:12`
и `docs/specs/449-double-fit-all.md:9` показывают действующий образец записи
для сопоставимых по риску editor-фич.
**Почему это находка:** это не стилистика — правило `docs/TOUCH-SUPPORT.md`
существует именно чтобы код-ревью и будущие читатели не собирали
touch-классификацию по кусочкам прозы, а видели один явный маркер. Задача
прямо подпадает под критерий «new editor feature specification».
**Как чинится:** добавить одну строку вида `Touch editor: best effort /
intentionally degraded` (или `supported` — выбор буквы предположительно за
автором, поскольку это техническая классификация, а не продуктовый вопрос)
рядом с разделом «Сценарий» или «UX и доступность», с краткой ссылкой на
`docs/TOUCH-SUPPORT.md`.
**M2. ТЗ не учитывает существующую зависимость live-подписей Resize от
позиции кнопки «Настройки комнаты» — после реализации риск воспроизвести ту
же проблему («подпись перекрывает кнопку»), которую issue должен решить.**
`src/houseplan-editor-runtime.ts:3404–3435` (`_rszEdgeLabels`, вызывается во
время активного перетаскивания стены в режиме Resize того же редактора
«План», см. `src/houseplan-card.ts:1663–1667` — Resize это часть `_mode ===
'plan'`) строит живые подписи длины/площади и передаёт в
`placeResizeAreaLabel()` (`src/resize-labels.ts:25–27,97,113–116`)
`gearCenter: poleOfInaccessibility(poly)` — то есть **всегда** автоматический
центр, безусловно, без какой-либо временной поправки. Кнопка `.rlgearbtn`
при этом продолжает рендериться параллельно (`this._markup ?
space.rooms.map(...this._renderRoomGear...) : nothing`,
`src/houseplan-card.ts:11181`) и, согласно контракту #645 п.9/12, может в
этот момент фактически находиться в другой, перетащенной пользователем
точке той же комнаты.
Итог: если пользователь в рамках одной сессии редактора сначала перетащил
кнопку в свободное место, а затем начал Resize стены той же комнаты,
алгоритм уклонения площади/длины по-прежнему обходит **старую** (уже не
отображаемую) автоматическую точку. Он может как оставить подпись рядом с
пустым местом без причины, так и, что хуже, отрисовать подпись поверх
**фактической**, перетащенной позиции кнопки — то есть воспроизвести ровно
тот дефект «элемент перекрыт и недоступен», ради устранения которого заведён
#645, только теперь в паре «подпись Resize ↔ кнопка», а не «кнопка ↔
устройство/мебель/проём».
Раздел «Затронутые модули» не называет ни `src/resize-labels.ts`, ни
`_rszEdgeLabels`/`placeResizeAreaLabel`; «Скоуп» и «Не-скоуп» не упоминают
Resize вообще; ни один из AC1–AC10 не покрывает этот сценарий. Это не
догадка — оба вызова и точная передача `poleOfInaccessibility(poly)` без
альтернативы прочитаны в текущем коде (пути указаны выше).
**Воспроизведение (чтением, не исполнением — кода ещё нет):**
```
sed -n '3404,3435p' src/houseplan-editor-runtime.ts # gearCenter: poleOfInaccessibility(poly)
sed -n '25,27p;90,120p' src/resize-labels.ts # gearCenter используется для уклонения подписи
sed -n '11175,11182p' src/houseplan-card.ts # кнопка рендерится в том же _markup-режиме
```
**Почему это находка, а не придирка:** ревью ТЗ обязано искать не только
внутреннюю непротиворечивость AC, но и то, решает ли изменение заявленный
сценарий, не ухудшая соседний (`docs/process/REVIEWER.md`, «Находки и
вердикт»; `PROCESS.md` §2.7 по духу применимо уже на этапе спецификации,
поскольку риск детерминированно вытекает из уже читаемого кода, а не из
будущей реализации). Здесь риск не гипотетический: оба фрагмента кода уже
существуют и жёстко связаны единственным явным чтением
`poleOfInaccessibility(poly)`.
**Как чинится (любой вариант закрывает находку):**
- явно включить в скоуп: `_rszEdgeLabels`/`placeResizeAreaLabel` должны
получать эффективную (временную, если она есть) позицию кнопки, а не
всегда автоматическую — с отдельным AC и добавлением файла в «Затронутые
модули»; либо
- явно вынести в «Не-скоуп» с обоснованием и явным принятым риском (например:
«одновременная комбинация активного Resize и перетащенной кнопки в той же
комнате достаточно редка, деградация принимается»), чтобы код-ревью не
спрашивал о ней как о непокрытой находке.
Любой из вариантов — правка внутри текущего ТЗ, отдельный issue не нужен:
дефект — прямое следствие этой же задачи.
### High / Low
Находок этих уровней нет.
## Что проверено и корректно
- **Выбор полного трека обоснован явно** — аналитика называет нарушенный
критерий §5 («нет нового UX-контракта») и указывает на touch-контракт
редактора, а не отделывается фразой «обычный трек» (issue #338).
- **Оба первых обязательных раздела `PROCESS.md` §7.1 присутствуют и
корректны**: «Сценарий» называет персону (администратор дома,
`docs/SCOPE.md`), поверхность (Plan editor, desktop) и момент (кнопка
закрывает элемент); «Что человек увидит до и после» — одной фразой без
терминов реализации.
- **Обязательные разделы ТЗ присутствуют все** (сценарий · что человек
увидит · проблема · скоуп/не-скоуп · контракт поведения · UX/доступность ·
модель данных/миграция · i18n · AC1–AC10 · план автотестов · риски · откат
· release-артефакты) — `PROCESS.md` §7.1 выполнен формально и по
содержанию.
- **Контракт поведения (14 пунктов) покрыт критериями приёмки без пробелов**:
1→AC1, 2→AC2/AC3 (косвенно, начало жеста), 3→AC3, 4→AC1, 5→AC2, 6→AC4,
7→AC3, 8→AC6, 9→AC5, 10→AC7, 11→AC8, 12→AC7, 13→AC7, 14→AC9; AC10
закрывает регрессии вне редактора. Ни один пункт контракта не остался без
проверяемого AC.
- **Каждый AC однозначен, привязан к конкретному наблюдаемому исходу и несёт
способ доказательства** (`unit`/`browser smoke`/`ревью кода`, без смешения
«ручное тестирование» как доказательства — раздел Release-артефакты прямо
это исключает).
- **Технические заявления о текущем коде верны**, что подтверждено чтением
(см. «Как проверялось» п.4): `_renderRoomGear`, порог 3 px, механизм
подавления click, `pointInPolygon` — ничего не придумано.
- **Ограничение полигоном для вогнутой комнаты специфицировано корректно**:
«доходит до ближайшей допустимой точки на пути жеста» (а не «до ближайшей
евклидовой точки границы») — формулировка исключает типичную ошибку
прыжка в несвязанную допустимую область, реализуемо через пересечение
сегмента жеста с границей полигона.
- **Модель координат согласована с реальным рендером**: `_renderRoomGear`
уже хранит и транслирует плановую точку через `view` в проценты экрана
(`src/houseplan-editor-runtime.ts:10289–10290`) — контракт п.9 (позиция
переживает pan/zoom) не требует новой модели преобразований, только замену
источника точки `c`.
- **Не-скоуп сформулирован явно и без размывания**: collision-aware
раскладка относительно устройств/мебели, сохранение позиции вне сессии,
Undo/Redo, изменение исходного алгоритма/вида кнопки, выход за пределы
комнаты, полная touch-паритетность — всё явно исключено, без формулировок
«и подобное».
- **Данные/миграция/совместимость закрыты явным «нет»** — соответствует
DoR-пункту (§2.5): постоянная схема не меняется, `docs/CONFIG-
COMPATIBILITY.md` не применяется к чисто временному UI-состоянию.
- **i18n закрыт корректно**: новых строк нет, существующие ключи
`room.settings_title`/`room.settings_short` не меняются (сверено с
`src/i18n/ru.json`, `src/i18n/en.json`).
- **Производительность названа предметно**, не общей фразой: O(1) по
`roomId`, ограничение геометрии только для активного жеста, переиспользование
существующего coalescing pointermove (тот же паттерн, что уже используется
для device drag, `_queuePointerMove('device', …)`,
`src/houseplan-card.ts:7016`) — реалистично, не выдумана мощность.
- **Откат назван и реалистичен**: убрать обработчики/временное отображение,
`_renderRoomGear()` возвращается к чистому автоматическому центру без
миграции — пользовательских данных для отката нет, потому что позиция
никогда не сохранялась.
- **Release-артефакты названы полно**: оба changelog со ссылкой на #645,
оба `USER-GUIDE` (en+ru), `STATUS.md` при наличии соответствующего
раздела, явное «нет» для golden (обоснованно — статичный idle-кадр не
меняется, см. также M2 о смежном Resize-сценарии, не о golden) и для
performance/security/backend/migration notes.
- **Риски перечислены содержательно и каждый закрыт конкретным AC**
(проверено построчно): synthetic click → AC3; потерянный pointerup/
`grabbing` → AC6 (см. также ниже про `lostpointercapture`, отмечено как
«не проверял» — технический нюанс уже прикрыт разделом «Принято
предположительно»); второй touch/pinch-owner → AC6; смешение экранных/
плановых координат при DPR → AC5; clamp для вогнутой комнаты → AC4;
устаревший `roomId`/полигон → AC8; лишний `requestUpdate` на pointermove →
раздел «Производительность» + план автотестов.
- **Блок «Принято предположительно» корректно отделяет техническое от
продуктового**: имена полей, структура состояния (`Map`), выбор
`pointInPolygon`/бинарного поиска, механизм pointer capture и подавления
click, величина визуального подъёма — всё нечувствительно для пользователя
и справедливо оставлено на усмотрение исполнителя/ревьюера код-ревью, без
подмены продуктовых решений (позиция кнопки, её видимый вид, порог 3 px,
время жизни) техническими догадками.
- **Открытых продуктовых вопросов действительно нет**: единственная
потенциально спорная точка — граница «сколько touch-паритета входит в
скоуп» — разрешена ТЗ явно и без выдумки (одиночный pointer — да, полная
паритетность — нет), совпадает с прежним owner-решением
`docs/TOUCH-SUPPORT.md` («editors are desktop-first… best effort»), не
требует нового решения владельца.
## Чего не проверял и почему
- **Реализацию** — её нет: диапазон `git diff origin/dev...HEAD` не содержит
ни одного файла класса A/B на момент ревью; хендофф-комментарий автора
прямо это подтверждает.
- **`npx tsc --noEmit` / `npm test` / `npm run build` / `node
scripts/check-docs.mjs`** — не гонял: ни одна строка кода ещё не изменена,
прогон гейтов на этапе ревью ТЗ бессмыслен (§2.4 их не требует).
- **Browser smoke / golden / performance / mutation** — не запускались по
той же причине; план автотестов ТЗ оценён на реалистичность (существующие
примитивы `_queuePointerMove`, `_suppressClick`, `pointInPolygon`,
`mutation-gate.mjs`-конвенции существуют), но не исполнялся.
- **Тонкость `lostpointercapture` при штатном завершении drag** —
контракт п.7/8 и AC3/AC6 в сумме требуют, чтобы обычный `pointerup` после
drag сохранял позицию, а `lostpointercapture` — откатывал к началу жеста;
в браузерах `lostpointercapture` штатно следует сразу за `pointerup`,
когда элемент удерживал capture, так что наивная реализация «любой
`lostpointercapture` = отмена» откатывала бы даже успешный drag. Это не
противоречие ТЗ по существу — стандартная идиома (различать «capture
потеряна без завершающего `pointerup` той же последовательности» и
«capture отпущена вследствие `pointerup`») закрывает вопрос, и ТЗ уже
относит «конкретный механизм pointer capture» к «Принято предположительно»
— технический выбор, а не пробел контракта. Не поднимаю отдельной
находкой (Low, снята решением ревьюера с записью): проверяемо будущим
unit-тестом реордера жеста (план автотестов, п.1), и не требует
вмешательства владельца.
- **Существование/содержимое смоков, которые план автотестов обещает
повторно прогнать** (`smoke_room_settings`, `smoke_room_cards`,
`smoke_feedback_v2`, `smoke_hide_room_names`) — не открывал построчно;
для целей spec-ревью достаточно, что файлы называются по существующей
конвенции (`demo/smoke_*.mjs`), их фактическое существование и охват —
забота код-ревью при реализации.
- **`docs/CANVAS.md`, `docs/WALL-THICKNESS.md`, `docs/LIGHT.md`, `docs/SUN.md`
целиком** — не читал: ни один из них не является каноническим документом
предметной области этой задачи (Plan editor UI-жест, не геометрия стен,
свет или солнце); `docs/UX-MODES.md` и `docs/TOUCH-SUPPORT.md` прочитаны
как релевантные каноны.
- **История issue #645 до текущей редакции тела** — issue не редактировался
после публикации (`edited: false` во всех трёх комментариях и в самом
issue по данным `gh issue view`), поэтому вопрос «на каком тексте вынесен
вердикт» не возникает; хеш тела для якоря ниже.
## Материал раунда
- Issue: #645, метки на момент ревью: `bug`, `P2`, `polish`, `S4-spec-review`.
- ТЗ: раздел `## ТЗ` тела issue #645 (единственная редакция, r1,
`edited: false`).
- Комментарии учтены: «Оценка…» (аналитика, полный трек обоснован), «Взял:
автор ТЗ · сессия Codex · ветка `issue/645-room-settings-button-drag`»,
«Сделано: … реализация не начиналась».
- Рабочее дерево репозитория на момент чтения кода: `2b3959e55bcb139b6ad872a312ad212de36f44db`
(справочно, для проверки фактических заявлений ТЗ о текущем коде — не
является материалом самого ТЗ, т.к. ветка задачи не создавала коммитов
поверх него).
## Вердикт
Вердикт: жёлтый · заход r1 · блокирующих циклов 0/4 · High: 0 · Medium: 2 →
в задаче | — · Документ: docs/reviews/SPEC-REVIEW-645-r1.md
Обе находки (M1, M2) — в скоупе задачи #645 и без High-находок дают жёлтый
вердикт (`PROCESS.md` §2.4, §12): ТЗ возвращается автору на правку. M1
чинится одной строкой; M2 требует явного решения — включить Resize-подписи в
скоуп с AC или явно и обоснованно исключить их с принятием риска. Обе правки
локальны для текста ТЗ, следующий цикл разбирается по дельте (§2.10).
---
<!-- material-anchors: сгенерировано конвейером (#414) -->
## Материал раунда
- Ветка: `issue/645-room-settings-button-drag`, коммит `2b3959e55bcb` — ребейз его осиротит, и это нормально: ниже якоря, которые ребейз не меняет.
- Дерево материала: `7f08cac08613119d6e48b96c345db826e22d4b9a`
```
git log --all --format='%H %T' | grep 7f08cac08613
```
- Тело issue: `9102a02fd0ea8a92464cbbe6662e2932cb711640384b5f99ce28f04f3acded91`
- Вердикт конвейера: `yellow` · High 0