diff --git a/docs/reviews/SPEC-REVIEW-294-r1.md b/docs/reviews/SPEC-REVIEW-294-r1.md new file mode 100644 index 00000000..e0d00a8b --- /dev/null +++ b/docs/reviews/SPEC-REVIEW-294-r1.md @@ -0,0 +1,164 @@ +# SPEC-REVIEW-294-r1 + +- **Issue:** https://github.com/Matysh/houseplan-card/issues/294 +- **Этап:** ревью ТЗ (PROCESS.md §2.4) +- **Заход:** r1 · блокирующих циклов израсходовано 0/4 → 1/4 после этого вердикта +- **Ревьюер:** Claude (роль «ревьюер ТЗ», сессия отдельна от автора) +- **Предмет ревью:** `docs/specs/294-wall-esc-detach.md` на SHA `8e86b0a5` + (ветка `issue/294-wall-esc-detach`), тело issue #294 и оба комментария + аналитики/хендоффа автора. +- **Трек:** обычный (не `small`) — верно: метка `small` на issue отсутствует, + спецификация оформлена файлом, как и требует §5. + +## Скоуп + +Первый заход, дельты нет — разбор полный по §2.4/§7.1: продуктовая рамка, +однозначность контракта Esc для активной цепочки стен, полнота AC и их +доказуемость, отсутствие выданных за факт догадок, соответствие +`docs/USER-GUIDE*.md` и терминологии SCOPE/UX-MODES. + +## Как проверялось + +1. Прочитаны `docs/SCOPE.md`, `AGENTS.md`, `PROCESS.md` (§1–§9, §7.1 отдельно). +2. Прочитаны тело issue #294 и все три комментария (аналитика, «Взял», + хендофф автора). +3. Прочитан `docs/specs/294-wall-esc-detach.md` целиком на SHA `8e86b0a5`. +4. Утверждения «Подтверждение по коду» из аналитики и §1/§3 ТЗ о текущем + поведении сверены с реальным исходником на той же ветке + (`src/houseplan-card.ts`, обработчик `_onKey`, `_finishWallChain`, + `_undoPoint`, `_undoActiveDraftPoint`, `_resumeLastDraft`, + `_resumeDraftBySpace`, `_activateMarkupTool`) — все технические утверждения + спецификации подтверждаются кодом, ни одно не является непомеченной + догадкой. +5. Сверены `docs/USER-GUIDE.ru.md` и `docs/USER-GUIDE.md` на предмет текущей + формулировки контракта Esc/Ctrl+Z для инструмента «Стены»/Draw, чтобы + проверить полноту AC7 и раздела 5 ТЗ. +6. `docs/TOUCH-SUPPORT.md` и `docs/UX-MODES.md` проверены на отсутствие + противоречащих утверждений о touch/лестнице Escape — противоречий нет. +7. Гейты (`typecheck`/`test`/`build`/`check-docs`) не запускались: изменения + этого раунда — только `docs/specs/**` (класс C), продуктовый код не + существует до `S5-ready`; сами по себе они ничего не проверяют для ТЗ. + Это сознательное решение объёма, а не пропуск. + +## Находки + +### Medium (в скоупе задачи, чинится в текущем ТЗ) — AC7 уже формулировкой не покрывает весь текст, который спецификация обязана исправить + +**Файл:** `docs/specs/294-wall-esc-detach.md`, раздел 5 и AC7. + +**Суть.** Раздел 5 обещает: «`docs/USER-GUIDE.md` и `docs/USER-GUIDE.ru.md` +должны явно описать тот же контракт в разделах keyboard/cancel и +Rooms/Walls». Но проверяемый критерий AC7 сужает обязательство: + +> Старое утверждение «Esc/Ctrl+Z — убрать точку» отсутствует в затронутых +> Walls-разделах; i18n parity зелёный. + +AC7 — это единственное, что реально проверяется («ревью кода» + +`unit`/i18n-parity), а не раздел 5. А формулировка старого утверждения, +которое AC7 обязуется убрать, привязана буквальной цитатой и словом +«Walls-разделах» — то есть не покрывает как минимум четыре конкретных места, +где тот же неверный контракт «Esc для Draw убирает/отменяет операцию» +сформулирован другими словами вне «Walls»-раздела: + +- `docs/USER-GUIDE.ru.md:223` (таблица поверхностей ввода, строка + «Рисование и точный drag»): `` `Esc` отменяет операцию ``; +- `docs/USER-GUIDE.ru.md:241` (раздел «Клавиши отмены», строка + «Незавершённый Draw/Split»): «Убирает последнюю точку или выходит из + инструмента»; +- `docs/USER-GUIDE.md:207` (таблица поверхностей ввода, строка «Draw or + precise drag»): `` `Esc` cancels the operation ``; +- `docs/USER-GUIDE.md:216-217` (раздел «Cancel and undo»): «`Esc` cancels an + unfinished path, current drag/resize/rotation, or the top dialog without + undoing an already committed action.» + +Ни одна из этих четырёх строк не содержит буквальной фразы «Esc/Ctrl+Z — +убрать точку» и не лежит в «Walls»-разделе — значит, автоматическая или +ручная проверка AC7 «по букве» пройдёт, даже если все четыре останутся +нетронутыми. После реализации это даст прямое текстовое противоречие внутри +одной документации: раздел про стены будет говорить «Esc завершает цепочку, +геометрия сохраняется», а общая таблица «Клавиши отмены»/«Cancel and undo» — +по-прежнему «Esc убирает точку/отменяет операцию» для того же инструмента +Draw. Это ровно тот класс дефекта, который PROCESS.md §8 требует ловить +(«одно число — один источник», расширительно — одно поведение не должно быть +описано двумя разными способами в одном документе). + +**Сценарий проявления:** пользователь читает раздел 8 «Rooms and walls» — +видит новый корректный контракт; тот же пользователь листает раздел 6 +«Cancel and undo» или сводную таблицу горячих клавиш — видит старое, теперь +неверное утверждение «Esc отменяет операцию/убирает точку». Документация +one-source-of-truth не выполняется, при этом AC7 в исполнении «по букве» +формально зелёный. + +**Почему это Medium, а не High:** для активной задачи №294 продуктовый +контракт (§3 ТЗ) сформулирован полно и однозначно, регресс поведения не +грозит — страдает только точность документации, causing user confusion, но +не потерю данных или неработающую функцию. Находка в скоупе текущей задачи +(тот же файл, тот же тип правки, что и остальные пункты раздела 5) — чинится +здесь же, отдельный issue не заводится (§202). + +**Что нужно поправить в ТЗ:** переформулировать AC7 (и, по необходимости, +раздел 5) так, чтобы явно перечислить все места, где сегодня утверждается +«Esc для Draw убирает точку/отменяет операцию» — включая обе таблицы +поверхностей ввода (RU/EN) и разделы «Клавиши отмены»/«Cancel and undo», а не +только цитату и «Walls-разделы». Также стоит явно решить для EN «Create a +room» (`docs/USER-GUIDE.md:294-299`): в отличие от RU-версии (строка 412), в +этом месте английского руководства сегодня вообще нет утверждения про +Esc/Ctrl+Z — паритет с RU-версией по этому контракту не назван требованием. + +## Что проверено и корректно + +- Персона/поверхность/момент действия читаются однозначно из текста, хотя и + без отдельного заголовка «Сценарий»: Home admin, Plan editor, инструмент + «Стены», активная незамкнутая цепочка. Согласуется с `docs/SCOPE.md` + (редакторы — только для Home admin, десктоп). +- Никаких догадок, выданных за факт: каждое утверждение о текущем поведении + («`Esc` вызывает `_undoPoint()`», «смена инструмента идёт через + `_finishWallChain()`», «переключение Стены→Колонна→Стены уже даёт нужный + результат») подтверждено чтением `src/houseplan-card.ts` на той же ветке — + см. пункт 4 «Как проверялось». +- Контракт Esc (§3.1–3.4 ТЗ) внутренне непротиворечив и совпадает с формой + существующего кода: тот же `history.wall_chain_finish`, тот же путь + materialization, тот же приоритет диалогов (`_roomDialog` проверяется в + обработчике раньше ветки `tool === 'draw'` и уже завершается `return`, то + есть требование «этот же keydown не должен дополнительно завершить + восстановленный draft» уже выполняется существующей структурой кода, а не + требует новой архитектуры). +- Различение Esc (finish) и Ctrl/Cmd+Z (undo point) — с AC4 отдельно и явно + проверяемо через реальный `keydown`, что закрывает риск «Esc и Undo слились + в одну кнопку», названный в разделе 9 ТЗ. +- AC1/AC2 намеренно используют различимые толщины `AB`/`BC`, чтобы merge + коллинеарных сегментов одной толщины (`_mergeSpacePartitions`) не смазал + проверку количества сегментов — разумная защита от ложноположительного + прохождения теста. +- Не-скоуп (раздел 6) чётко исключает face-detection, touch-команду finish, + формат хранения, Escape других инструментов — расширения скоупа не + просматривается. +- Модель данных/миграция/i18n/touch/perf/security (разделы 8, 13) — сделанные + предположения обоснованы и помечены как таковые в разделе 13, спор по ним + решается вердиктом ревьюера, а не владельцем; ни один пункт не требует + продуктового вопроса владельцу (нет открытых вопросов — подтверждаю). +- Trailers коммита спецификации корректны: `Issue: #294`, + `User-Visible: no` — спецификация не меняет поведение, только документ; + класс C, отдельный issue не требуется (входит в DoD текущего). +- Откат (раздел 10) достаточен: чистый revert, миграция назад не нужна из-за + отсутствия изменений схемы. + +## Чего не проверял + +- Гейты `typecheck`/`test`/`build`/`check-docs`/смоки — не запускались: + раунд правит только `docs/specs/**`, продуктового кода ещё нет + (issue не в `S5-ready`), эти гейты не применимы к этапу ТЗ. +- Реализуемость AC на уровне конкретного будущего диффа кода — предмет + код-ревью, не ТЗ-ревью; здесь проверялась только проверяемость и + однозначность формулировок. +- Issue #173 не читалась целиком построчно — использована как контекст по + цитате в теле #294 и по факту существования кода единого инструмента + «Стены», который уже найден и прочитан напрямую. + +## Вердикт + +Жёлтый: единственная находка — Medium в скоупе задачи (пробел в +формулировке AC7/раздела 5, из-за которого исправление может формально +пройти проверку и оставить документацию внутренне противоречивой). High нет. +Автор правит формулировку AC7 (и, если нужно, раздел 5) в этом же ТЗ, фикс +проходит повторный цикл ревью по дельте (§2.10).