diff --git a/docs/reviews/INDEX.md b/docs/reviews/INDEX.md index 62197b1e..1b5b623a 100644 --- a/docs/reviews/INDEX.md +++ b/docs/reviews/INDEX.md @@ -1,9 +1,10 @@ # Индекс ревью -Генерируется `node scripts/reviews-index.mjs` (#635) — не редактировать руками. Документов: 1105, issue: 395. Вердикт: 🟢 зелёный · 🟡 жёлтый · 🔴 красный · ⚪ не распознан (свободная форма старых документов). H/M — число High/Medium по строке вердикта или заголовкам находок. Файлы — пути, названные в находках; ищите по имени файла: `grep form-kit INDEX.md`. +Генерируется `node scripts/reviews-index.mjs` (#635) — не редактировать руками. Документов: 1106, issue: 396. Вердикт: 🟢 зелёный · 🟡 жёлтый · 🔴 красный · ⚪ не распознан (свободная форма старых документов). H/M — число High/Medium по строке вердикта или заголовкам находок. Файлы — пути, названные в находках; ищите по имени файла: `grep form-kit INDEX.md`. | Issue | Документ | Этап · раунд | Вердикт | H | M | Находки | Файлы | |---|---|---|---|---:|---:|---|---| +| #676 | [SPEC-REVIEW-676-r1.md](SPEC-REVIEW-676-r1.md) | spec · r1 | 🟡 жёлтый | 0 | 2 | AC8 не называет проверку двух из трёх ветвей подавления подсказки; риск «рамка перехватывает события под другим инструментом» не привязан ни к одному AC; не задан tie-break для равного протяжки по обеим осям (снимаю сам) | `src/stairs-editor-model.ts` `stairs-view.ts` `demo/smoke_stairs.mjs` `src/furniture.ts` | | #675 | [CODE-REVIEW-675-r1.md](CODE-REVIEW-675-r1.md) | code · r1 | 🟢 зелёный | 0 | 0 | — | — | | #673 | [CODE-REVIEW-673-r1.md](CODE-REVIEW-673-r1.md) | code · r1 | 🟡 жёлтый | 0 | 0 | docs/images/screenshots.json: закоммиченный sourceFingerprint не совпадает с тем, что р…; Комментарий автора в issue #673 называет SHA реализации 557d4e38e26b647437f2c931454737f… | `docs/images/screenshots.json` `demo/golden/matrix.mjs` `scripts/source-fingerprint.mjs` `accept.mjs` `policy.mjs` `AGENTS.md` `baselines-index.json` | | #673 | [CODE-REVIEW-673-r2.md](CODE-REVIEW-673-r2.md) | code · r2 | 🟢 зелёный | 0 | 0 | — | — | diff --git a/docs/reviews/SPEC-REVIEW-676-r1.md b/docs/reviews/SPEC-REVIEW-676-r1.md new file mode 100644 index 00000000..e8bd7dff --- /dev/null +++ b/docs/reviews/SPEC-REVIEW-676-r1.md @@ -0,0 +1,245 @@ +# SPEC-REVIEW-676-r1 + +- **Issue:** https://github.com/Matysh/houseplan-card/issues/676 +- **Этап:** `S4-spec-review` (ревью ТЗ, PROCESS.md §2.4) +- **Трек:** полный (метки `bug`, `P1`, `S4-spec-review`; аналитик явно + зафиксировал «лёгкий трек: нет» — несколько поверхностей, новый + UX-контракт размещения протяжкой и подсказки, сложность > 3). +- **Материал:** тело issue #676, раздел `## ТЗ` (редакция 1, 27.09) + 3 + комментария: (1) «Взял» аналитика, (2) полная аналитика — причины 9 + дефектов по коду и воспроизведению, решение «латать или переписывать», + оценка, (3) факт публикации ТЗ и перевод в `S4-spec-review`. +- **Заход:** r1 · блокирующих циклов израсходовано 0 из 4 +- **Роль:** ревьюер ТЗ (не автор) + +## Скоуп ревью + +Первая итерация лестниц (#663) даёт 9 дефектов редактирования, воспроизведённых +владельцем и подтверждённых аналитиком чтением кода и браузерным пробником +(`demo/probe_stairs_bugs.mjs`, не гейт). Задача переписывает слой +редактирования лестниц (`src/stairs-editor.ts`, `stairs-editor-model.ts`, +рамка узлов в `houseplan-card.ts`, подсказка в `stairs-view.ts`, стили) на +box-контракт, общий с декором/мебелью — жесты, курсоры, магнит по краям, +диалог свойств. Модель `stairs.ts`, площадь (#669), навигация по клику в +Просмотре и 2.5D не трогаются. + +**SCOPE-проверка (`docs/SCOPE.md`):** сценарий закрывает J6 («keep the plan +true», редактирование объектов плана без регресса в сохранённых данных) и +J1/J2 через корректную навигацию/подсказку лестницы в Просмотре. Из +«никогда не строить» ничего не задевается; из «партиально покрыто» — +touch-эргономика редакторов («best effort», не блокирует). + +## Как проверялось + +1. Прочитаны `docs/SCOPE.md`, `docs/process/REVIEWER.md` целиком; по ссылкам + конспекта открыт PROCESS.md §2.4, §2.5, §4, §7.1, §7.2 (§2.10 не + применялся — это r1). +2. Прочитано тело issue #676 целиком (`gh issue view 676 --json body`) и все + 3 комментария. +3. Прочитан канонический `docs/STAIRS.md` — контракт К1–К8 ему не + противоречит; раздел «Implementation boundary» уже называет + `stairs-editor-model.ts` владельцем чистых Plan-трансформаций — задача + продолжает существующее разделение, а не изобретает новое. +4. Сверены обязательные разделы §7.1: Сценарий, Что человек увидит до/после, + Проблема, Скоуп/Не-скоуп, Контракт поведения (К1–К8), UX, Модель данных и + миграция, i18n, Критерии приёмки AC1–AC11 со способом доказательства, План + автотестов, Риски, Откат, Release-артефакты — все на месте; первые два — + продуктовые, без терминов реализации. +5. Прочитан код, лежащий в основании диагностики «Проблема», чтобы отделить + факт от догадки: + - `src/stairs-editor.ts:2,3,5` — подтверждён импорт и вызов + `resizeFurnitureTransform`, `snapFurnitureToWall`, + `furnitureWallSurfacesFor` для текущего (дефектного) поведения; + - `src/houseplan-editor-runtime.ts:2214-2223,3333-3334` — подтверждён + `_markupClick`/`_suppressClick`, объясняющий дефекты 1/2/8 (копия при + ресайзе, потеря выделения); + - `src/wall-thickness.ts:222-243` — подтверждено, что `fieldToCm`/ + `cmToField`/`clampWallCm` зажимают в `[WALL_MIN_CM, WALL_MAX_CM]` = не + [30, 10000], а диапазон толщины стены — дефект 4 («100 см потолок») + реален и назван верно; + - `src/houseplan-card.ts:9097,11069-11088` — подтверждён порядок отрисовки: + `stairs.renderLayer()` стоит ДО `_renderWallBodies`, `_renderTextFrame` + (декор) — ПОСЛЕ; перенос рамки лестницы к `_renderTextFrame` (Скоуп) + действительно решает дефект 3 и AC7; + - `src/stairs-editor.ts:427-435`, `src/styles/plan.styles.ts:1595-1605` — + подтверждён единый класс `hp-stair-resize` (`nwse-resize` для всех 8 + узлов) и радиус `_gridPitch * 0.65` (единицы плана, не экрана) — дефекты + 5 и 9 реальны, К2 их закрывает; + - `src/furniture-placement.ts:7` (`FURN_WALL_CELLS = 6`) и + `src/furniture.ts:140-141` (`FURN_MIN_CM = 1`, `FURN_MAX_CM = 10000`) — + константы контракта (`STAIR_MAX_CM = 10000`, довод магнита 6 клеток) + совпадают с источником, на который ссылается ТЗ; + - `src/stairs-editor.ts:317-323` (`undoActiveDrag`) — Ctrl+Z во время + жеста уже реализован и работает (откат к `drag.original`, сброс жеста); + К6 в этой части не меняет контракт, а фиксирует уже верное поведение; + - i18n: `stairs.fixed_floor`, `history.stair_move` уже существуют во всех + четырёх словарях (`src/i18n/{en,ru,de,fr}.json`) — новые ключи + (`stairs.tooltip_navigate`, `history.stair_resize`, + `history.stair_rotate`) не конфликтуют и следуют тому же + нейминг-паттерну; `test/i18n.test.mjs` действительно проверяет + плейсхолдер-паритет всех словарей — AC10 доказуем как заявлено; + - golden: 4 существующих сцены лестниц подтверждены (`stairs-flat-hover- + dark`, `stairs-flat-normal-light`, `stairs-flat-selected-light`, + `stairs-isometric-dark`, `demo/golden/baselines/`) — AC9 корректно + называет ровно одну меняющуюся и три неизменные; + - `test/stairs-editor-model.test.mjs`, `demo/smoke_stairs.mjs` — второй + файл существует и расширяется, первый отсутствует и создаётся с нуля, + как и заявлено («новый»). + Диагностика «Проблема» и вся фактическая база контракта проверена по коду + и подтверждена — ни одного случая догадки, выданной за факт, не найдено. +6. Проверена math-корректность сектора курсора К2 (`round(θ/45) mod 8`, 0°= + +x экрана по часовой): раскладка по секторам (0/4→ew, 2/6→ns, 1/5→nwse, + 3/7→nesw) арифметически верна для экранной системы координат (y вниз) и + соответствует стандартным CSS-именам диагоналей (SE/NW действительно + лежат на диагонали `nwse-resize`, NE/SW — на `nesw-resize`). +7. Проверено покрытие исходных 9 дефектов отчёта критериями приёмки — все + девять сведены к AC1–AC11 (таблица построена вручную, см. «Что проверено + и корректно»). +8. Прогон гейтов **не выполнялся** — см. «Чего не проверял»: на этапе + `S4-spec-review` кода задачи ещё нет, ветка `issue/676-*` не создана, + рабочее дерево на `f1a87fba` содержит только исходную (дефектную) + реализацию #663. + +## Находки + +### Medium (в скоупе) — AC8 не называет проверку двух из трёх ветвей подавления подсказки + +**Что не так.** К8 описывает три причины, по которым подсказка Просмотра НЕ +показывается: «Лестница без цели, с битой целью **или** в карточке с +закреплённым этажом». Это ровно три состояния `stairTargetState()` +(`missing`, `deleted`, `fixed` — функция уже существует, +`src/stairs-editor-model.ts:97-102`). AC8 же называет доказательством только +одну ветвь: «наведение на лестницу с целью `garden` даёт … ; уход убирает; +**лестница без цели** подсказки не даёт» — про «битую цель» (deleted) и +«закреплённый этаж» (fixed) в столбце «Доказательство» нет ни слова. + +**Почему это находка, а не придирка.** Тултип — новый код (существующий +`_showTip` только переиспользуется, сам вызов на новом обработчике наведения +в `stairs-view.ts` — новый), а не старая, уже покрытая гвардом навигация; +то, что `stairTargetState` для missing/deleted/fixed уже протестирован для +навигации (не-скоуп), не гарантирует, что тот же guard правильно подключён +именно к вызову тултипа — это ровно тот код, который эта задача добавляет. +Реализация, читающая AC8 буквально, закроет её одним смоук-шагом («нет +цели → нет подсказки») и формально будет «выполнена», оставив нерабочим (или +непроверенным) случай удалённой цели и случай `floor`-карточки — то есть +именно тот регресс, который К8 прямо запрещает, останется недоказанным. + +**Чем закрывается в скоупе задачи.** Расширить столбец «Доказательство» AC8 +(или явно расщепить его на AC8a/AC8b) двумя смоук-утверждениями: наведение на +лестницу с удалённым/несуществующим `target_space_id` не показывает тултип; +наведение на лестницу с валидной целью в карточке, сконфигурированной с +`floor`, не показывает тултип. Технический характер уточнения (какой именно +смоук-шаг доказывает уже сформулированное продуктовое поведение) — решение +автора ТЗ, не продуктовый вопрос владельцу. + +### Medium (в скоупе) — риск «рамка перехватывает события под другим инструментом» не привязан ни к одному AC + +**Что не так.** К2 формулирует обязательное условие: «Рамка не перехватывает +события под другими инструментами плана». Раздел «Риски» повторяет его же +как угрозу регресса: «Рамка в оверлее должна оставаться инертной под +инструментами кроме «Выбор»/«Лестница» (К2), иначе перехватит стены». Ни в +таблице AC1–AC11, ни в «Плане автотестов» нет ни одной строки, которая бы +это проверяла — ни unit, ни smoke. + +**Почему это находка, а не придирка.** Рамка теперь стоит В ВЕРХНЕМ +ОВЕРЛЕЕ, выше тел стен (это и есть исправление дефекта 3) — именно +геометрическое перекрытие с кликабельной зоной инструмента «Стены» создаёт +риск, названный самим автором. DoR требует не просто «риски перечислены», а +чтобы задача имела для риска обозначенный путь проверки; риск, названный, но +не проверяемый ни одним AC, с равной вероятностью останется бумажным +предупреждением и уйдёт в релиз непроверенным — ровно то, ради чего вообще +существует колонка «Доказательство». + +**Чем закрывается в скоупе задачи.** Добавить в «План автотестов» один +смоук-шаг (или новый AC12): под инструментом «Стены» (или любым другим +не-«Выбор»/не-«Лестница» инструментом) клик в точке, где визуально +находится узел выделенной лестницы, должен воздействовать на стену/плоскость +плана, а не быть перехвачен рамкой лестницы (`pointer-events: none` на +оверлее вне активных инструментов — assert через `elementFromPoint` или +эквивалент, как уже делает `demo/smoke_stairs.mjs` для гвардов навигации). + +### Low — не задан tie-break для равного протяжки по обеим осям (снимаю сам) + +К1: «длинная ось протяжки — ось подъёма» для случая `|B−A|x == |B−A|y` +(ровно диагональная протяжка под 45°) не называет, какая ось считается +доминирующей — угол результата не определён детерминированно текстом ТЗ. +Не блокирует: это событие нулевой меры для реального указателя, а в +кодовой базе уже есть прецедент детерминированного тай-брейка при похожем +равенстве (`resizeFurnitureTransform`, `src/furniture.ts`: «Equal deltas +deliberately prefer width for deterministic ties»). Снимаю сам с +рекомендацией придерживаться того же соглашения (например, при точном +равенстip предпочитать +x/-x перед +y/-y) — чисто техническое решение, +владельца не касается. + +## Что проверено и корректно + +- Обязательные разделы §7.1 присутствуют и в правильном порядке; «Сценарий» + и «Что человек увидит» — продуктовые, без терминов реализации, называют + персону 1/2 по `docs/SCOPE.md`. +- Все 9 дефектов из «Что происходит» сведены к контрактам К1–К8 и покрыты + критериями: 1/2 → К6/AC6; 3 → К2/AC7; 4 → К7/AC5; 5 → К2/AC4; 6 → К4/AC3; + 7 → К8/AC8; 8 → К3+К4/AC3+AC11; 9 → К2/AC7. Ни один исходный дефект не + остался без критерия приёмки. +- Диагностика «Проблема» и числовые/структурные утверждения контракта + (константы, файлы, существующие функции) сверены с кодом и подтверждены + фактом, а не приняты на веру — см. «Как проверялось», п. 5. +- AC1, AC2, AC3, AC9, AC10, AC11 однозначны, называют способ доказательства + и воспроизводимы буквально по тексту. +- Модель данных не меняется, откат («revert коммитов, данные читаются + обеими версиями») корректен и согласован с `docs/CONFIG-COMPATIBILITY.md` + (задача явно заявляет, что не задевает этот документ, и это верно — + персистентная схема стула не меняется). +- Touch учтён минимально необходимо: К1 включает touch/pen в размещение + (курсорный жест общий для указателя любого типа, ничего специфичного не + требуется), AC8 явно фиксирует, что касание не показывает тултип + (наведения нет) и не меняет навигацию касанием — единственное + туч-чувствительное новое поведение проговорено. Экраны редактора вне View + — best effort по `docs/TOUCH-SUPPORT.md`, отдельного пункта не требуется. +- «Принято предположительно» корректно фиксирует семь технических развилок + как решённые автором (направление подъёма, поведение клика, шаг вращения, + предел доворота магнита 5°, минимальный размер 30 см, база отсчёта + магнита, техническая реализация рамки/подавления клика) — ни одна не + является продуктовым вопросом, эскалации владельцу не требовалось и не + произошло. +- Release-артефакты (changelog RU+EN, `docs/STAIRS.md`, руководство, + golden, «performance — нет, security — нет») названы явно, а не оставлены + подразумеваемыми; бюджет бандла назван риском с конкретным запасом (~2 КБ) + и существующим гейтом (`bundle:budget`). + +## Чего не проверял + +- Не запускал `npm run typecheck`/`npm test`/`npm run build` и + `node scripts/check-docs.mjs` — на этапе `S4-spec-review` кода задачи ещё + нет: ветка `issue/676-*` не создана, рабочее дерево на `f1a87fba` содержит + только исходную реализацию #663 без единой правки этой задачи. Дифф для + этих гейтов просто не существует; они относятся к этапу код-ревью. +- Не запускал `demo/smoke_stairs.mjs`, `node scripts/smoke-select.mjs`, + `golden:verify` и инварианты модели — по той же причине: браузерные смоки + и golden проверяют код, а на этапе ТЗ проверяемый код не существует. Их + прогон целиком ложится на код-ревью после реализации. +- Не проверял `demo/probe_stairs_bugs.mjs` (пробник аналитика) по существу — + он явно не гейт и не идёт в коммит; использовал только код продукта + (`src/*.ts`), который он якобы демонстрирует, и подтвердил утверждения + чтением этого кода напрямую (см. «Как проверялось», п. 5), а не доверием к + выводу пробника. +- Не проверял #663 и #669 по существу (смежные задачи) — использовал только + факт их состояния («работают, не трогаются» / «работает, #669 закрыт») как + вводную для не-скоупа этой задачи. + +## Вердикт + +Вердикт: жёлтый · заход r1 · блокирующих циклов 0/4 · High: 0 · Medium: 2 → в задаче + +--- + + + +## Материал раунда + +- Ветка: `dev`, коммит `f1a87fbad9d4` — ребейз его осиротит, и это нормально: ниже якоря, которые ребейз не меняет. +- Дерево материала: `4c3048e855e543bdf5ecf9edbdfed8146f31c648` + ``` + git log --all --format='%H %T' | grep 4c3048e855e5 + ``` +- Тело issue: `fe591d135b54a53f4596f5e657f3139e5fb064331dbd48ff529b63472395175d` +- Вердикт конвейера: `yellow` · High 0