diff --git a/docs/reviews/SPEC-REVIEW-239-r1.md b/docs/reviews/SPEC-REVIEW-239-r1.md new file mode 100644 index 00000000..7aab1be2 --- /dev/null +++ b/docs/reviews/SPEC-REVIEW-239-r1.md @@ -0,0 +1,195 @@ +# SPEC-REVIEW-239-r1 + +- **Issue:** https://github.com/Matysh/houseplan-card/issues/239 +- **ТЗ:** `docs/specs/239-grid-scale-invariance.md` +- **Ветка / SHA:** `issue/239-grid-scale-invariance` @ `4c065f5` +- **Этап:** spec (PROCESS.md §2.4) · трек: обычный (`small`/`trivial` явно + отклонены автором в аналитике issue — несколько рендереров, новый default, + i18n, влияние на touch/performance) +- **Заход:** r1 · блокирующих циклов израсходовано 0/4 до этого вердикта +- **Ревьюер:** Claude, роль «ревьюер ТЗ» + +## Скоуп + +Оценивалось только ТЗ #239 — единый визуальный контракт «масштаб сетки +(`cell_cm`) не должен менять внешний вид плана» плюс новый default нового +пространства (1 см метрика / 1 дюйм imperial). Код не менялся и не +оценивался: этап S4, реализации нет, весь диапазон `origin/dev..HEAD` +состоит из одного docs-коммита. + +## Как проверялось + +1. Прочитаны `docs/SCOPE.md`, `AGENTS.md`, `PROCESS.md` целиком (первый цикл, + правило «по дельте» §2.10 не применяется). +2. Прочитано тело issue #239 и оба комментария (аналитика с оценкой + 9/9/8/8/P1/bug/обычный трек, затем хендофф «ТЗ готово»). +3. Прочитан файл `docs/specs/239-grid-scale-invariance.md` целиком (481 + строка) и запись в `docs/specs/README.md`. +4. Построчно перепроверены фактические ссылки ТЗ на код (не поверено на + слово — большинство утверждений §3 являются "подтверждено по коду", что + обязывает проверить): + - `src/houseplan-card.ts:14088`, `:14459` — `cellCm: 5` в manual create и + floors-import draft — совпадает; + - `src/houseplan-card.ts:20591-20595` — поле `space.scale_label` / + `space.scale_unit` всегда рендерит сырые см без ветвления на + `hass.config.unit_system.length === 'mi'` — совпадает буквально + (заявление ТЗ §3.3, а не догадка); + - `src/houseplan-card.ts:295-296` — `CELL_CM_MIN = 0.1`, `CELL_CM_MAX = + 1000` — совпадает с §5.2/§7.1 (диапазон не расширяется); + - `src/render/opening-symbol.ts:34-42` — `jambHalf` fallback `4`, + `outlineHalf = Math.max(16, jambHalf+8, gateDepth+8)`, `hitHalf = + Math.max(20, ...+10, ...+12)` — совпадает с заявленными «4-12 units»; + - `src/wall-thickness.ts:31-43` (`HATCH_REFERENCE_CELL_CM = 5`, + `HATCH_BASE_STEP_UNITS = 8`) — арифметика `8 × (5/5) × (шаг сетки)` даёт + заявленные 9.6 см из #230 и AC5 — проверено вычислением, не с чужих слов; + - `src/iso-projection.ts:29-30` — `ISO_WALL_HEIGHT = 64`, + `ISO_FLOOR_EDGE_HEIGHT = 10` — заявленные «user units» константы + существуют и не параметризованы `cell_cm`, совпадает с §3.2.5; + - `src/houseplan-card.ts:5241,5178-5254` — `_isoGeometryCache.get(source.key)` + подтверждает риск «тёплый remount вернёт старую высоту», если `source.key` + не будет включать масштаб — корректно учтено в таблице рисков §16, а не + пропущено; + - `src/space-geometry.ts:442-478` (`iconUnit`, `iconCqw`) — числитель и + знаменатель оба выведены из содержимого плана в тех же plan units, поэтому + при физически эквивалентном плане (`k = 5/cell_cm`) множитель `k` + сокращается сам — заявление §3.1 «эти пути уже верны, не домножать» + подтверждено алгеброй, а не принято на веру; + - `docs/USER-GUIDE.ru.md:1092,1145` — термины «дюйм»/«фут» для физических + размеров уже существуют в продукте, значит предлагаемая подпись «1 дюйм на + клетку» не изобретает новую терминологию. +5. Сверены §5 (входит/не входит) с §6 (классификация размеров) и §8 + (контракт по поверхностям) на отсутствие противоречий — реестр + `Physical/Screen/Plan-relative/Visual unit/Grid` покрывает все пункты §3.2 + без пропусков и без двойного назначения одного размера в два класса. +6. Сопоставлены критерии приёмки §13 (AC1–AC16) с планом автотестов §14 — + не как формальность, а по прямому указанию для этого этапа: «указание + способа доказательства». Для сверки взяты два прецедента из этого же + репозитория: `docs/specs/230-hatch-density-normalization.md` §13 (план + тестов поимённо покрывает AC1–AC12) и `docs/specs/238-opening-inner-distances.md` + §15 (таблица AC с отдельной графой «Доказательство» для всех 15 AC, включая + мета-критерии производительности и документации — AC14 «unit со + счётчиком/ревью кода», AC15 «provenance + ревью кода»). См. находку ниже. +7. Проверен гейт `node scripts/check-docs.mjs` — единственный применимый на + docs-only диффе класса C. `npx tsc --noEmit` / `npm test` / `npm run build` + не запускались: диапазон изменений не содержит ни одного файла класса A/B + (`git diff origin/dev...HEAD --stat` — только `docs/specs/239-*.md` и + строка в `docs/specs/README.md`), эти гейты ничего не проверяют на чистой + документации и были бы тратой времени без предмета (см. «Объём гейтов + соразмерен задаче»). + +## Находки + +### Medium (в скоупе задачи — исправляется в этом ТЗ) + +**M1. §13 Acceptance criteria не указывает способ доказательства для AC13, +AC14, AC16, и это не восстанавливается однозначно ни из §14 «План +автотестов», ни из таблицы рисков §16.** + +- AC13 (инвариант `cell_cm: 5` = pre-#239 контракт) частично закрыт: в самом + тексте AC упомянут «current golden», так что проверяемость угадывается, но + §14.3 говорит лишь «Golden baseline разработчик не принимает» — не называет + golden прямым доказательством AC13. +- AC14 (touch/gesture-инвариант: pan/pinch/pointercancel не получают новых + действий, opening action area не меньше эталонной) не назван ни в одном + пункте §14, а из таблицы рисков (§16, строка «Touch target станет physical») + восстанавливается только половина критерия — про размер hit-area, но не про + «никакой новый жест ничего не запускает». +- AC16 (EN/RU user guide и оба changelog объясняют новый контракт) не назван + ни в §14, ни в release-артефактах §17 как проверяемый пункт, хотя оба + changelog там перечислены как артефакт. + +Это не придирка к форме: у этой же задачи есть два прямых прецедента в этом +репозитории, оба прошли ревью зелёными на этом самом требовании процесса. +`docs/specs/230-hatch-density-normalization.md` §13 поимённо связывает каждый +AC1–AC12 с тестом. `docs/specs/238-opening-inner-distances.md` §15 — тот же +issue-поток, тот же автор, документ на день раньше #239 — держит отдельную +графу «Доказательство» и заполняет её даже для мета-критериев вроде +производительности (AC14: «unit со счётчиком/ревью кода») и документации +(AC15: «provenance + ревью кода»). #239 явно откатывается от этой практики без +объяснения, и без неё код-ревью не имеет явного контракта: чем именно должна +быть закрыта строка «pan/pinch не получают новых действий» — тестом или +утверждением автора. Учитывая объём и риск задачи (сложность 8/10, риск 8/10 в +собственной оценке автора), для «not by accident» touch-инварианта и для +одного из главных anti-regression AC (AC13) это не косметика. + +**Как починить:** добавить в §13 колонку «Доказательство» (как в #238 §15) +либо явно перечислить AC13/AC14/AC16 в §14 с указанием конкретного смока, +unit-теста или «ревью кода» — по аналогии с уже принятым для этой же задачи +подходом «AC15 — только ревью кода, потому что perf-гейт до беты» (эта фраза +в документе уже есть для AC15, её не хватает для соседних AC). + +### Low (снимается с записью, не блокирует) + +**L1. §10 «Данные, i18n...» не называет конкретный текст/ключ новой imperial- +подписи внутри самого i18n-раздела** — формулировка «новые EN/RU строки нужны +для imperial unit label» не самодостаточна. Текст фактически дан двумя +разделами ниже, в §9.2 («дюйм на клетку» / `in per cell`), так что +неоднозначности для реализации нет — но это расходится с конвенцией +остальных 239 спеков репозитория (например, #157: «`opening.passage`, +`opening.passage_binding_warning`»; #167: «label «Plan only»»; #094: +«`tap.toggle`: «Переключить состояние»»), где i18n-раздел сам перечисляет +строки, а не отсылает к UX-разделу. Снимаю как Low: содержательно текст уже +в документе, вопрос чисто в консолидации. Автор может перенести/продублировать +строку в §10 попутно с фиксом M1 либо оставить как есть. + +## Что проверено и корректно + +- **Продуктовая рамка.** Сценарий и «что человек увидит» — обе продуктовые + секции §7.1 присутствуют первыми, отвечают на «какая персона / какая + поверхность / какой момент» и не используют термины реализации. Задача + закрывает J4/J6 из `docs/SCOPE.md`, конфликтов с out-of-scope нет. +- **Ни одной догадки, выданной за факт.** Каждое диагностическое утверждение + §3 («что уже масштабируется правильно» / «где инвариант нарушен») проверено + построчно по коду (см. «Как проверялось», п.4) и совпадает буквально, а не + на уровне пересказа. Технические решения (§19) явно помечены как «assumed, + change freely» и не выданы за продуктовое решение. +- **Продуктовых вопросов владельцу нет, и это оправдано.** Единственный + потенциально продуктовый вопрос — «должен ли визуально измениться уже + существующий сохранённый план с `cell_cm ≠ 5` после фикса» — уже + разрешён самим текстом issue («Нужно проверить и при необходимости + нормализовать весь визуальный контракт»): это и есть цель бага, не + побочный эффект, и AC13/§17 changelog корректно фиксируют это как + осознанное, объявленное в changelog поведение, а не тихую перерисовку. +- **Скоуп/не-скоуп непротиворечивы.** §5.2 явно запрещает менять + `GRID_N`/`GRID_PITCH`/snap-алгоритм и migration существующих `cell_cm` — + это ровно то, что держит задачу в границах «визуальный слой», не + «координатная модель», и совпадает с диапазоном 0.1…1000, подтверждённым в + коде. +- **Классификация размеров (§6) внутренне непротиворечива** и предотвращает + ровно ту ошибку, которую называет главным риском сам автор (общий CSS/SVG + transform повторно умножил бы уже верные physical/plan-relative пути) — + проверено алгеброй на `iconCqw`/`iconUnit` (см. «Как проверялось», п.4). +- **AC1–AC12 однозначны и алгебраически проверяемы** без реализации: + например AC1 (`gridVisualScale(5) === 1`, `5/cellCm` для остальных, `1` для + невалидных), AC5 (physical-контроль не должен домножаться второй раз — уже + сформулирован как мутационный тест, а не как описание), AC9 (canonical + `2.54` для imperial — точное значение 1 дюйма, не приближение). +- **Откат (§18) и release-артефакты (§17) полны**: один revert коммита, + явное заявление «данные не мигрируют и остаются валидными и после отката», + оба changelog, оба USER-GUIDE, CANVAS.md, ARCHITECTURE.md, TESTING.md и + пересъёмка public docs screenshot — ничего из обязательного набора не + забыто. +- **Риски (§16) содержательны, не общие слова**: каждая строка называет + конкретный механизм провала (например, «px/unitless `calc()` в SVG + вычислится не в те units») и конкретную защиту, без карго-культных «будем + осторожны». +- **Гейт docs зелёный**: `node scripts/check-docs.mjs` → `Documentation + checks passed (7 files, 10 external links)`. + +## Чего не проверял + +- `npx tsc --noEmit`, `npm test`, `npm run build` — не запускал: диапазон + диффа не содержит файлов класса A/B (только `docs/specs/239-*.md` и строка + реестра), эти гейты не имеют предмета проверки на этом этапе и их прогон на + чистой документации был бы потерей времени, а не тщательностью. +- Реализацию классов Physical/Screen/Plan-relative/Visual/Grid — кода ещё + нет, оценивалась только логическая непротиворечивость классификации, не её + будущее исполнение. +- Полный текст остальных ~470 существующих ТЗ репозитория — выбраны только + #230 (единственный прямой предок контракта) и #238 (непосредственно + предыдущая задача в очереди, тот же автор) как контрольные образцы + конвенции «AC → доказательство»; это не полная статистика по всем спекам, + а два целевых прецедента, достаточных для находки M1. +- Golden/browser smoke/performance — не запускались и не должны: реализации + нет, эти гейты относятся к S6/S7 и предрелизному прогону (PROCESS.md §8), + не к ревью ТЗ.