diff --git a/docs/reviews/SPEC-REVIEW-461-r1.md b/docs/reviews/SPEC-REVIEW-461-r1.md new file mode 100644 index 00000000..2c14171f --- /dev/null +++ b/docs/reviews/SPEC-REVIEW-461-r1.md @@ -0,0 +1,198 @@ +# SPEC-REVIEW-461-r1 + +- **Issue:** https://github.com/Matysh/houseplan-card/issues/461 +- **ТЗ:** `docs/specs/461-wall-draw-click-performance.md` +- **Материал:** SHA `1ef21037a8c32ad203a9cad05c801420223db8b6`, ветка + `issue/461-wall-draw-click-performance`, коммит `docs: specify wall draw + click performance` (`docs/specs/461-wall-draw-click-performance.md` + + `docs/specs/README.md`, `Issue: #461 · User-Visible: no`) +- **Заход:** r1 · блокирующих циклов израсходовано 0 из 4 +- **Вердикт: зелёный** + +## Скоуп ревью + +Этап spec (PROCESS.md §2.4). Задача полного трека (S4-spec-review), ТЗ живёт +файлом в `docs/specs/`. Продуктовый код не менялся — коммит только +документационный, code review этот раунд не задевает. + +## Как проверялось + +1. Прочитал issue #461 целиком: тело (с редакцией измерений) и все четыре + комментария — первую (ошибочную) фикстуру, перемер на плане дачи, + живой замер в браузере владельца и аналитику. +2. Прочитал `docs/SCOPE.md` — сверил заявленную привязку к J4/J6. +3. Прочитал `PROCESS.md` §2.2–2.4, §3, §4, §7.1–7.2 — обязательные разделы, + формат AC, порог продуктовых вопросов, лимит циклов. +4. Прочитал ТЗ целиком (473 строки, все 18 разделов). +5. Сверил каждую техническую предпосылку ТЗ с actual `dev`, а не поверил на + слово — потому что ТЗ цитирует конкретные номера строк, сигнатуры функций + и существующий прецедент (#451/resize), а догадка, выданная за факт, — + отдельный класс замечания: + - `_persistActiveDraftSegment` (2777), `_commitPhysicalGeometry` (2128), + `_checkSpacePhysicalGeometry` (2143, 2214), `commitWallSegmentModel` + (2194), `_junctionLimitsIntroduced` (2229) — прочитал целиком + (`src/houseplan-editor-runtime.ts:2048-2248`). Порядок операций и место + единственного оптимизируемого call site (2795, внутри + `_persistActiveDraftSegment`) подтверждены буквально; остальные вызовы + `_commitPhysicalGeometry` (2998, 3087…8012) не относятся к + `_persistActiveDraftSegment` — исключение их из скоупа (§3.2) + соответствует коду; + - resize-прецедент (`resizeLiveJunctionRoomIds`, `resizeLiveCandidateSpace`, + `_rszSpaceCandidateGeometry`, `_junctionLimitViolations` с + `sharedGeometry`/`roomIds`) — прочитал целиком + (`src/houseplan-editor-runtime.ts:3640-3772`). Подтверждено: resize уже + подставляет урезанное `liveSpace` в полный candidate и гоняет ту же + `checkSpacePhysicalGeometry`/`_junctionLimitViolations` с `affected` + roomIds и `{status:'lightweight'}` — именно тот механизм, на который ТЗ + ссылается в §5–§7 как на существующий сейм; + - пороговые числа §8 (15°, 6 стен в узле, 20 см длина, 5 см дистанция, + 25 см² просвет) сверены с `docs/USER-GUIDE.ru.md:521-533` — совпадают + дословно, включая пограничные случаи (14/15°, 6/7, 19/20 см, 4/5 см); + - `wall-degraded-extra` (§8) — фикстура реально существует и используется + (`src/plan-geometry-preflight.ts`, `test/plan-geometry-preflight.test.mjs`, + `test/wall-union-isolation.test.mjs` и др.), не выдумана; + - методология перфоманс-профиля (семь замеров после одного warm-up, + exact-SHA Linux gate, structural assertions отдельно от timing) сверена с + `demo/performance/README.md` и `.github/workflows/performance.yml` + (`--samples=7 --warmups=1`) — это существующий паттерн `large-house-*-v1`, + не новое изобретение; + - `room_drafts` как имя поля подтверждено (`src/align-grid.ts`, + `src/coincident-partitions.ts`); + - `demo/smoke_wall_chain_merge.mjs` существует — заявленная в AC8 регрессия + merge двух drafts опирается на реальный smoke, а не на несуществующий. +6. Проверил трассируемость: `docs/specs/README.md:184` содержит строку на + #461, коммит несёт трейлеры `Issue: #461 · User-Visible: no` (правка кода + ещё не начата — верно для `User-Visible: no`). + +Код не менялся — `npx tsc --noEmit`, `npm test`, `npm run build`, golden, +инварианты модели и mutation-gate к этому раунду не относятся: гейты этапа +spec — это проверка самого ТЗ на прочитанность и однозначность, а не гейты +реализации. Явно не прогонял: ни один из перечисленных в PROCESS.md §8/§10.4 +кодовых гейтов, потому что диапазон `origin/dev..HEAD` не содержит +изменений `src/**`/`test/**`. + +## Находки + +Блокирующих (High) находок нет. Находок уровня Medium в скоупе или вне скоупа +нет. + +### Low (не блокирует, оставляю с записью — правка на усмотрение автора) + +1. **§9.3, formula несогласованность между двумя scaling-проверками не + объяснена одной фразой.** Node/unit-бенчмарк проверяет «80 сегментов дороже + 40 не более чем вдвое» (растущий локальный компонент), а + production-профиль проверяет remote-вариант («не более `base × 1.5 + 20 + мс»`) для сегментов, которые НЕ входят в локальный компонент. Формулы разные + не потому, что противоречат друг другу — они проверяют разные вещи (рост + входящего компонента vs накладные расходы от роста всего конфига при + неизменном локальном компоненте), но в тексте это различие не + проговорено, и человек, читающий только §9.3, может принять это за + непоследовательность. **Снимаю без правки**: раздел «Принятые + предположения» (§18, п.4-5) уже даёт ревьюеру право оспорить точные числа + без влияния на UX/AC, и code review сможет проверить сам факт двух разных, + но не противоречащих друг другу порогов по факту прохождения обоих + assertions. +2. **§6.2 не проговаривает явно, что закрытие локального компонента — + итеративное до fixed point, а не однопроходное.** Формулировка «все rays + конкретного junction, если в компонент вошёл хотя бы один его ray» и + «room wall atoms… чьи envelopes касаются seed либо первого слоя включённых + соседей» подразумевают, что включение нового соседа может открыть новый + узел, который открывает следующий слой — то есть закрытие должно быть + рекурсивным до неподвижной точки, а не одним проходом. Текст этого не + называет явно. **Снимаю без правки**: корректность закрытия в любом случае + доказывается не описанием алгоритма, а parity-матрицей §8 и тремя + отрицательными мутантами AC4 («выбросить соседнюю комнату», «не включать + чужой draft/column», «не замыкать junction rays») плюс независимым + terminal full check (§4.3) — если реализация ошибётся с однопроходным + закрытием, один из этих гейтов покраснеет вне зависимости от того, назвал + ли текст ТЗ слово «fixed point». + +Ни одна из этих двух записей не влияет на выполнимость или проверяемость AC1…AC12, +поэтому не поднимаю их до Medium. + +## Что проверено и корректно + +- **Обязательные разделы §7.1** все присутствуют и в правильном порядке: + сценарий (§2) → что человек увидит до/после (§2 «До/После») → проблема (§1) + → скоуп/не-скоуп (§3) → контракт поведения (§4-§9) → UX (§4) → модель + данных/миграция/i18n (§12) → AC1-AC12 с методом доказательства (§10) → план + автотестов (§11) → риски (§15) → откат (§16) → release-артефакты (§17). +- **Продуктовая рамка**: привязка к J4/J6 `docs/SCOPE.md` обоснована — + задача про десктопный редактор, поддержание плана «true as it evolves»; + явно не расширяет touch-паритет (§13, ссылка на `TOUCH-SUPPORT.md`). +- **Каждый AC** имеет указанный способ доказательства (unit / smoke / + performance / mutation witness / code review / documentation review) — + ни одного «просто похоже на правду». +- **Явное разделение продуктового и технического** соблюдено: единственное + продуктовое решение («не показываем сегмент до verdict», «не удаляем + брошенные drafts автоматически») уже принято владельцем прямо в теле/ + комментариях issue и корректно перенесено в §3.2/§18 как принятое решение, + а не как оставленный открытым вопрос. Технические детали (имена файлов, + форма кэша) явно помечены в §18 п.5 как «технические и могут меняться при + ревью» — ровно то разделение, которое требует §7.1. Отсутствие вопросов + владельцу в конце ТЗ не является недосмотром: развилка, которая обычно + требовала бы продуктового вопроса («что делать с накопленными кликами при + большом плане»), была закрыта до старта ТЗ самим владельцем в комментариях + (аналитика: «продуктовых вопросов, блокирующих первую редакцию ТЗ, по + текущему описанию нет»). +- **Ни одной догадки, выданной за факт, не найдено.** Все числовые пороги + (15°, 6 стен, 20 см, 5 см, 25 см², 150/250 мс) либо взяты из + `docs/USER-GUIDE.ru.md`, либо прямо привязаны к измерениям владельца + (1 278–1 309 мс → 150 мс с десятикратным запасом), либо помечены как + bootstrap-потолок в §18 п.4. +- **Технические утверждения о текущем коде проверены чтением, не + исполнением**, и совпадают построчно: место единственного изменяемого call + site, состав операций generic barrier, существование и сигнатура + resize-прецедента, существование фикстуры `wall-degraded-extra`, имя поля + `room_drafts`, существование smoke `wall_chain_merge`, методология + семи-замерного perf-раннера. Ни одной ссылки на несуществующую функцию, + файл или fixture не найдено. +- **Не-скоуп (§3.2) не противоречит коду**: `_commitPhysicalGeometry` + вызывается из ~15 разных мест рантайма, ТЗ ускоряет только один + (`_persistActiveDraftSegment`, строка 2795) и явно исключает остальные — + это соответствует буквальному тексту «не входит: ускорение остальных + generic-вызовов». +- **Откат (§16)** реалистичен: fast path — чистая оптимизация одного call + site без изменения формата данных, ревёрт не требует миграции. +- **i18n/данные (§12)**: schema/model_version не меняются, новых ключей нет — + соответствует характеру задачи (внутренний write barrier). + +## Чего не проверял + +- Не прогонял `npx tsc --noEmit` / `npm test` / `npm run build` / golden / + инварианты модели / mutation-gate — код не менялся, гейты этого раунда не + касаются (проверено `git diff origin/dev...HEAD` — только два файла в + `docs/specs/`). +- Не проверял осуществимость итогового замера «≤150 мс median / ≤250 мс max» + на реальном браузере — это будет предметом code review (AC9), сейчас важно + только, что бюджет привязан к измерению, а не к произвольному числу, что + подтверждено. +- Не пересчитывал квадратичную асимптотику `checkSpacePhysicalGeometry` + самостоятельно — принял измерения владельца (Node и live-браузер) как + первичный источник; это его собственный, дважды перепроверенный замер + (первая версия была explicitly отозвана в комментарии тем же автором). + +## Итог + +ТЗ полное, однозначное, каждый AC имеет проверяемый метод доказательства, +технические утверждения о текущем коде подтверждены построчным чтением, +продуктовая граница зафиксирована самим владельцем, скоуп/не-скоуп совпадает +с реальной структурой кода. High-находок нет, Medium нет — ни в скоупе, ни +вне его. Две Low-записи оставлены с обоснованием, почему не требуют правки. +Вердикт — зелёный, задача может идти в `S5-ready`. + +--- + + + +## Материал раунда + +- Ветка: `issue/461-wall-draw-click-performance`, коммит `1ef21037a8c3` — ребейз его осиротит, и это нормально: ниже якоря, которые ребейз не меняет. +- Дерево материала: `6a30077b8244b2cfac5d69b3c544256fce730add` + ``` + git log --all --format='%H %T' | grep 6a30077b8244 + ``` +- ТЗ `docs/specs/461-wall-draw-click-performance.md`, блоб `ccf374b1c0797ace0156b7cfb366007f1271fd35` + ``` + git log --all --find-object=ccf374b1c0797ace0156b7cfb366007f1271fd35 -- docs/specs/461-wall-draw-click-performance.md + ```