diff --git a/docs/reviews/SPEC-REVIEW-176-r1.md b/docs/reviews/SPEC-REVIEW-176-r1.md new file mode 100644 index 00000000..b5bc2f22 --- /dev/null +++ b/docs/reviews/SPEC-REVIEW-176-r1.md @@ -0,0 +1,259 @@ +# SPEC-REVIEW-176-r1 + +- **Issue:** https://github.com/Matysh/houseplan-card/issues/176 +- **ТЗ под ревью:** тело issue #176 (лёгкий трек, файл `docs/specs/` не + создаётся — метка `small` подтверждена аналитиком и не оспорена владельцем) +- **Роль:** ревьюер ТЗ (не автор), этап `S4-spec-review` +- **Трек:** лёгкий (`small`), лимит циклов ревью ТЗ — 2 (§4 PROCESS.md) +- **Цикл:** r1/2 + +## Скоуп ревью + +ТЗ #176 удаляет мёртвый внутренний tool-state `'partition'` из +`src/houseplan-card.ts` (значение `MarkupTool`, `MARKUP_TOOLS`, метод +`_partitionClick`, ~15 условных веток `this._tool === 'partition'`) и три +осиротевших i18n-ключа, оставленных после #173 (замена one-shot Partition +единым Walls-инструментом). Persisted-модель независимых стен +(`space.partitions`, `PartitionCfg`, `kind: 'partition'` у selection/opening +host, весь UI работы с уже созданными стенами) не меняется. +`User-Visible: no`; changelog не требуется. + +Находка изначально заведена самим ревьюером кода на #173 +(`docs/reviews/CODE-REVIEW-173-r1.md`, Medium-1) и подтверждена владельцем как +issue — проверка «issue создан явным решением владельца» выполнена (первая +запись в issue — команда владельца на вход в `S2-analysis`). + +Не в скоупе ревью: продуктовый код не написан (issue в `S4-spec-review`; +ветки `issue/176-*` в репозитории нет — проверено `git branch -a`), поэтому +реализация, тесты и гейты — предмет будущего код-ревью (PROCESS.md §2.7). + +## Как проверялось + +1. Прочитаны `docs/SCOPE.md`, `AGENTS.md`, `PROCESS.md` целиком (актуальная + редакция, включая §2.4/§2.5/§5/§7.1/§8) и тело issue #176 со всеми + комментариями (аналитика владельца, передача на ревью, отметка о сбое + автоматического прогона). +2. Построчно сверены технические утверждения ТЗ с текущим `dev` + (`src/houseplan-card.ts`, `grep -in partition`, полный список ~140 + вхождений): + - `type MarkupTool = ... | 'partition' | ...` — строка 516, подтверждено; + - `'partition'` в `MARKUP_TOOLS` — строка 547, подтверждено; + - `private _partitionClick(...)` — строка 7143, реализует «два клика → одна + перегородка» через `_recordGeometry('history.partition_add', …)`; + подтверждено; + - диспетчер клика: `if (this._tool === 'partition') { this._partitionClick(...); }` + — строка 6834-6835, единственная точка вызова метода — подтверждено, что + `_partitionClick` не достижим никаким иным путём; + - ветки `this._tool === 'draw' || this._tool === 'partition'` (лимиты, + snap, hints, Undo/Escape, рендер превью) — найдены на строках 2241, + 5362, 5429, 5450, 5487, 6364, 6601, 17202, 17923, 18039, 18041, 18091, + 9597; одиночные `this._tool === 'partition'` — строки 2270, 2323, 6834, + 9559, 9598, 11908 — количество и характер веток соответствуют заявленным + в issue «порядка 15». +3. Проверена причина недостижимости `this._tool === 'partition'` — ключевое + техническое утверждение issue, а не декларация: + - `normalizeMarkupTool` (`houseplan-card.ts:551-562`) — единственное место, + присваивающее `this._tool` из внешнего/сохранённого значения + (`this._tool = normalizeMarkupTool(vp.tool)` при восстановлении warm + viewport, строка 2656); + - `normalizeMarkupTool` безусловно вызывает + `value = normalizeUnifiedWallTool(value)` **до** проверки + `MARKUP_TOOLS.has(...)` (строка 555); + - `normalizeUnifiedWallTool` (`src/wall-face-graph.ts:53-55`): + `return value === 'partition' ? 'draw' : value;` — литерал `'partition'` + маппится в `'draw'` безусловно. + - Других обработчиков, присваивающих `this._tool = 'partition'` из + пользовательского ввода, не найдено (единственное текстовое совпадение + — сравнения `===`, не присваивания). Вывод issue «код недостижим, а не + редко используем» подтверждён чтением, не декларацией автора. +4. Проверено, что тип `MarkupTool` и `MARKUP_TOOLS` не используются больше + нигде в `src/` (`grep -rn "MarkupTool\|MARKUP_TOOLS" src/`) — оба + контейнера частные для `houseplan-card.ts`; удаление литерала не пробивает + границу модуля. +5. Проверена граница «мёртвый код инструмента» vs «живая persisted-модель» + построчно: `kind: 'partition'` в типах selection/drag/dialog (строки 1225, + 1228, 1234, 1243, 7196, 7257, 7283, 7374, 7401, 7415, 7434, 7462, 7505, + 11485, 12005, 17840, 17863, 17881, 17981), `space.partitions`/`PartitionCfg`, + `_partitionDeleteDialog`/`_confirmPartitionDelete`, + `_partitionOpeningCuts`/`resolvePartitionOpeningCompat` и весь модуль + `partition-openings.ts` — ни один из них не завязан на `this._tool`, все + работают с уже существующими объектами независимо от активного + инструмента. Граница, которую ТЗ объявляет неприкосновенной (контракт п.3), + реально проходит там, где заявлено. +6. Проверены i18n-ключи (`src/i18n/en.json`, `src/i18n/ru.json`): + `title.markup_partition`, `markup.hint_partition`, `physical.partition_size_title` + — все три существуют в обоих locale и (`grep -rn` по `src/`) используются + только через `this._tool === 'partition'` (строки 9559, 9598) либо нигде + не используются вне собственного определения (`title.markup_partition`) — + подтверждено, что это ровно осиротевший набор, ни ключом больше, ни + меньше. Ключи `markup.partition`, `physical.partition_properties`, + `history.partition_add`, `opening.host_partition`, + `confirm.delete_partition_openings_*`, `opening.rebind_partition` — заняты + persisted-объектом и диалогами, ТЗ верно не включает их в список удаления. +7. Проверен `test/golden-matrix.test.mjs:72`: + `assert.equal(['draw', 'partition'].includes(scenario.planSnap.tool), true, …)` + — сейчас допускает оба значения. Проверены реальные сценарии + `demo/golden/matrix.mjs` (`planSnap: {...}`, строки 73-80) — оба + существующих `planSnap`-сценария уже используют `tool: 'draw'`; ни один + golden-сценарий не задаёт `'partition'`. Сужение assert до одного `'draw'` + не меняет ни одного изображения — заявление ТЗ «визуальные эталоны не + меняются» подтверждено чтением фикстур, а не предположением. +8. Проверены названные в плане автотестов файлы: + `demo/smoke_free_walls.mjs` (существует; строки 57-60, 177-181 напрямую + ставят `c._tool = 'partition'` и зовут `c._partitionClick(...)` в обход + публичного dispatch — ровно то приватное использование, которое ТЗ обязуется + перевести на публичный flow), `demo/smoke_plan_snap_overlay.mjs` + (существует; строка 185 `card._tool = 'partition'` перед `_markupClick`, + что действительно проходит через диспетчер строки 6834-6835), и + `demo/smoke_unified_wall_tool.mjs` (существует; не использует приватный + tool-state — уже написан против публичного `_tool = 'draw'` + + `_keepClosedAsPartitions()`, строка 128 — служит рабочим образцом того, + как должны выглядеть переписанные сценарии). +9. Проверено существование паттерна «source-contract test», на который + ссылается AC1 как способ доказательства: `test/isometric-contract.test.mjs`, + `open-passage-contract.test.mjs`, `optional-space-model-contract.test.mjs`, + `release-contract.test.mjs` и другие — это существующая практика репозитория + (в т.ч. для внутренних инвариантов), а не изобретённый под это ТЗ механизм. +10. Проверено `docs/USER-GUIDE.ru.md:343` — таблица инструментов Плана уже + фиксирует «отдельного инструмента «Перегородка» нет» на пользовательском + уровне; терминология ТЗ («persisted-объект остаётся «Перегородка», + внутренний tool-token удаляется») не расходится и не переизобретает то, + что задокументировано. +11. Проверено `docs/TESTING.md:486-497` («Room markup editor») и + `docs/STATUS.md` (`grep -in partition` — без совпадений) — ни один + текущий пункт не описывает приватный `_tool='partition'`; изменение кода + не требует правки формулировок этих файлов по существу, перечисление их + в «Затрагиваемые поверхности» — корректная страховка на случай появления + новых заметок в реализации, не декларация обязательной правки. +12. Проверено, что issue #176 не оспорил `small`: сложность/риск 3/10, одна + поверхность (`src/houseplan-card.ts` + производные тесты/i18n), нет + миграции, нового UX-контракта, влияния на perf/touch — все критерии §5 + выполнены одновременно; `trivial` корректно не применён (тип `tech-debt`, + не `bug`). +13. Проверено отсутствие ветки `issue/176-*` (`git branch -a | grep 176` — + пусто) и отсутствие более раннего `docs/reviews/SPEC-REVIEW-176-*.md` + (`ls docs/reviews | grep 176` — пусто) — это первый цикл, r1/2. + +Гейты (`typecheck`/`test`/`build`) не прогонялись — на этапе ревью ТЗ +продуктового кода нет; прогон гейтов не относится к этому этапу (PROCESS.md +§2.7/§8). + +## Обязательные разделы (§7.1 PROCESS.md, лёгкий трек — тело issue) + +| Раздел | Есть | Комментарий | +|---|---|---| +| Сценарий (персона/поверхность/момент) | N/A, обоснованно | задача не создаёт наблюдаемого изменения продукта (`User-Visible: no`, контракт п.6); §7.1 формулирует эти два раздела как продуктовые именно для отделения «работы» от «изменения продукта» — здесь по существу нет стороны-наблюдателя, это подтверждено чтением кода (см. п.3), а не заявлено голословно | +| Что человек увидит до/после | N/A, обоснованно | то же: «до» и «после» с точки зрения пользователя идентичны — задача про внутреннюю модель, не про интерфейс | +| Проблема | ✅ | подтверждена чтением `houseplan-card.ts` и `wall-face-graph.ts` (см. «Как проверялось» п.2-3), а не декларацией автора | +| Контракт (скоуп/не-скоуп внутри пунктов 1-3) | ✅ | граница мёртвый-код/persisted-модель проверена построчно (п.5) и проходит ровно там, где заявлено | +| Модель данных и миграция | ✅ | «persisted-модель не меняется» — подтверждено (п.5); миграции нет, потому что нет изменения хранимой схемы | +| i18n | ✅ | ровно три ключа, ни больше ни меньше — подтверждено построчной сверкой (п.6) | +| AC1…ACn с доказательством | ✅ | 3 штуки, у каждого назван реалистичный и проверяемый способ доказательства (typecheck+source-contract test / unit+targeted smokes / locale unit+build diff+golden:verify) | +| План автотестов | ✅ | ссылается на реально существующие файлы (п.7-8), включая точную строку смока, которая сейчас использует приватный API и должна быть переписана | +| Риски | частично | явного раздела «Риски» нет, но риск («будущий контрибьютор вернёт мёртвую ветку по аналогии») назван в теле issue и закрывается самим фактом удаления кода — для лёгкого трека §5 отдельного раздела «Риски» не требует (шаблон: проблема · контракт · AC · откат) | +| Откат | ✅ | один revert, без миграции данных — обоснованно, так как persisted-схема не тронута | +| Release-артефакты | ✅ | явно и корректно: `User-Visible: no` → changelog не требуется; три bundle snapshot синхронизируются по стандартному процессу сборки | + +Раздел «Принятые предположения» присутствует и корректно отделяет то, что +уже прочно установлено чтением кода (`partition` в persisted-типах — не +мёртвый код; legacy-нормализатор остаётся намеренно), от того, что не +является продуктовым вопросом вовсе (нет UI, нет changelog) — ни одно +утверждение раздела не выдаёт техническую догадку за факт. + +## Находки + +Отсутствуют. Ниже — то, что специально проверено на предмет типичных для +этого класса дефектов (пропущенный call site, недосказанная граница +скоупа, домысленное поведение) и не подтвердилось. + +- **Домыслы вместо решения не найдены.** Единственное потенциально спорное + техническое утверждение — «код недостижим, а не редко используем» — не + принято на веру, а прослежено до кода нормализации (`normalizeUnifiedWallTool`, + «Как проверялось» п.3): это не догадка автора, а проверяемый факт. +- **Скоуп i18n не занижен и не завышен.** Отдельно проверено, что среди трёх + удаляемых ключей есть `physical.partition_size_title`, которого не было в + исходной находке код-ревью #173 (там названы только `title.markup_partition` + и `markup.hint_partition`) — авторская аналитика нашла его самостоятельно и + подтвердила чтением (`this._tool === 'partition' ? 'physical.partition_size_title'`, + строка 9559); ключ действительно используется только приватным + tool-state и нигде в persisted-UI. +- **Golden-эталоны не сузятся ошибочно.** Проверено, что сужение + `test/golden-matrix.test.mjs:72` до одного `'draw'` не затрагивает ни один + реальный сценарий `demo/golden/matrix.mjs` — оба `planSnap`-сценария уже + на `'draw'`. + +## Что проверено и корректно + +- **Соответствие `docs/SCOPE.md`:** задача не создаёт нового Core user job и + не расширяет ни один из них напрямую — это ожидаемо для `tech-debt`. + Косвенная связь с J4/J6 (поддержание единственного тестируемого пути Plan + editor, без альтернативного нетестируемого API) не притянута: старый + `_partitionClick` действительно обходит crash-safe/finish/limit-контракт, + установленный #173 для «unified Walls chain» (J6 — «keep the plan true as + the home evolves»). +- **Легитимность лёгкого трека:** сложность 3/10, одна поверхность, нет + миграции/нового UX-контракта/влияния на perf или touch — критерии §5 + выполнены все одновременно; аналитик верно не предложил `trivial` (тип + `tech-debt`, критерий §5.1 требует `bug`). +- **Продуктовых вопросов действительно нет.** Единственный класс вопросов, + который вообще уместен на этом этапе — объём видимого изменения — здесь + вырожден: видимого изменения нет вовсе (`User-Visible: no`), и это не + недосмотр автора, а прямое следствие того, что удаляется недостижимый код. + Технические решения (source-contract test, перевод смоков на публичный + flow) корректно оставлены на усмотрение разработчика записью в разделе + «Принятые предположения», как и предписывает §7.1 PROCESS.md. +- **Граница мёртвый-код / persisted-модель проведена точно и без пропусков** + — построчная проверка (см. «Как проверялось» п.5) не нашла ни одного + места, где persisted `kind: 'partition'` зависел бы от `this._tool`, и ни + одного места, где `this._tool === 'partition'` относился бы к чему-то, + кроме активного инструмента разметки. +- **Ссылки на существующие файлы не выдуманы** — все три названных в плане + автотестов смока (`smoke_free_walls.mjs`, `smoke_plan_snap_overlay.mjs`, + `smoke_unified_wall_tool.mjs`) существуют, и первые два действительно + используют приватный `_tool = 'partition'`/`_partitionClick` способом, + который ТЗ обязуется устранить — заявление в AC2 не голословно. +- **Обратная совместимость не сужается.** Legacy warm-viewport токен + `'partition'` продолжает нормализоваться в `'draw'` уже существующим кодом + (`normalizeUnifiedWallTool`), который ТЗ явно не трогает и защищает + unit-тестом (контракт п.2) — соответствует `docs/CONFIG-COMPATIBILITY.md` + в части «не сужать уже принятый вход», хотя формально warm viewport — + page-memory, а не persisted storage schema, которую документ описывает + напрямую. +- **Откат тривиален и корректен** — один revert без миграции данных, + поскольку persisted-схема не меняется ни в одной части. + +## Чего не проверял + +- Реализацию — её нет: issue в `S4-spec-review`, ветки `issue/176-*` не + существует (`git branch -a` пуст по этому номеру) — проверено. +- Гейты `typecheck`/`test`/`build`/browser smoke/golden — не относятся к + этапу ревью ТЗ; предмет будущего код-ревью (PROCESS.md §2.7/§8). +- `python -m pytest tests_backend` — backend (`custom_components/**/*.py`) не + упомянут ни в issue, ни в затронутых файлах, и не содержит `partition` + как tool-state (только независимая от frontend-инструмента persisted-схема, + если вообще есть — не проверялась специально, так как ТЗ прямо заявляет + «Backend … не меняется», а весь код-путь — frontend-only `MarkupTool`). +- Точный будущий текст source-contract теста и точный способ переписать + `smoke_free_walls.mjs`/`smoke_plan_snap_overlay.mjs` на публичный + draw/finish flow — это техническое решение реализации (§7.1: «всё, чего + пользователь не наблюдает, агенты решают сами»), а не предмет ревью ТЗ. +- Реальный визуальный результат `npm run golden:verify` — CSS/геометрия не + меняются по заявлению ТЗ и по чтению фикстур (см. «Как проверялось» п.7), + но фактический прогон гейта относится к реализации/пре-релизу, не к этому + этапу. +- Точность числовых оценок аналитики (ценность 1/10, сложность 3/10, P3) по + существу — поле владельца (PROCESS.md §2.2), не предмет ревью ТЗ. + +## Вердикт + +Зелёный. High: 0, Medium: 0, Low: 0. ТЗ описывает точно очерченную, +доказанную чтением кода (не декларированную) границу между мёртвым +tool-state и живой persisted-моделью независимых стен; все технические +утверждения (недостижимость `this._tool === 'partition'`, точный список +осиротевших i18n-ключей, безопасность сужения golden-matrix assert) +проверены построчно и подтвердились. Продуктовых вопросов нет обоснованно — +задача не создаёt наблюдаемого изменения, а не умалчивает о нём. AC однозначны +и снабжены доказуемыми способами проверки. + +**Вердикт: зелёный · цикл r1/2 · High: 0 · Medium: 0 → нет · Документ: +docs/reviews/SPEC-REVIEW-176-r1.md**