diff --git a/docs/reviews/SPEC-REVIEW-39-r2.md b/docs/reviews/SPEC-REVIEW-39-r2.md new file mode 100644 index 00000000..616a8936 --- /dev/null +++ b/docs/reviews/SPEC-REVIEW-39-r2.md @@ -0,0 +1,241 @@ +# SPEC-REVIEW-39-r2 + +- Issue: https://github.com/Matysh/houseplan-card/issues/39 — «[HP-UX-09] большие подложки» +- Этап: ТЗ на ревью (PROCESS.md §2.4) +- Заход: r2 · блокирующих циклов израсходовано 1/4 +- ТЗ: `docs/specs/039-large-backdrops.md`, ревизия 3, SHA `f787de99` +- Предыдущий вердикт: жёлтый, r1, SHA `156be645` (`docs/reviews/SPEC-REVIEW-39-r1.md`) — 0 High, 4 Medium (M1–M4) +- Трек: полный (владелец, 2026-08-15: «P3, polish/tech-debt, обычный трек») — без изменений с r1 + +## Скоуп проверки (дельта r1→r2) + +Дельта локальна и полностью укладывается в границы одной правки одного файла: +`git diff 156be645..HEAD -- docs/specs/039-large-backdrops.md` — 93 добавленных/ +изменённых строки в `docs/specs/039-large-backdrops.md`, ничего больше (commit +`f787de99`, `Issue: #39`, `User-Visible: no`). Никакого ребейза на ушедший +вперёд `dev` (`156be645` — прямой родитель по линии этого файла, между r1 и r2 +в `dev` были только чужие docs/build-коммиты, не трогавшие эту подсистему). +Контракт поведения не менялся с r0→r1: ревизия 3 отвечает точечно на M1–M4, +новых AC не добавляла (кроме переименования AC4→AC4+AC4б, что и есть предмет +M4). Полный разбор с нуля не требуется — веду по дельте согласно PROCESS.md +§2.9/§2.10. + +## Закрытие раунда r1 + +| Находка r1 | Чем закрыта | Где это видно | +|---|---|---| +| **M1** — нет разделов «Сценарий»/«Что человек увидит» | Добавлен раздел «Сценарий»: персона (владелец дома), поверхность (диалог пространства, «Файл»), момент (выбор скана плана 150–600 DPI); отдельно «До»/«После» одной фразой без терминов реализации | `docs/specs/039-large-backdrops.md:13–26` | +| **M2** — i18n без перечня ключей | Добавлен раздел «i18n» с 7 новыми ключами `backdrop.*`, каждый с en+ru текстом; de оговорён как перевод тех же ключей | `docs/specs/039-large-backdrops.md:112–136` | +| **M3** — нет разделов «Риски»/«Release-артефакты» | Оба раздела добавлены: «Риски» — 4 пункта со смягчением каждого; «Release-артефакты» — changelog RU+EN, `docs/BACKDROP.md`, USER-GUIDE, `docs/TESTING.md`, i18n, golden, performance | `docs/specs/039-large-backdrops.md:181–206` — закрыта **частично**, см. r2-M2 ниже | +| **M4** — AC4 объединял два разных момента `hard` без UI-контракта | `hard` явно разделён на «фазу 1» (синхронная, >16384 по заголовку, диалог `too_large_*`, только «Отмена») и «фазу 2» (асинхронная, decode-fail/таймаут 10 с после выбора «Уменьшенной копии» — тост `downscale_failed`, staging чист, инпут сброшен, явно нет автофолбэка на оригинал); AC разделён на 4 и 4б | `docs/specs/039-large-backdrops.md:94–104` (UX), `:155–159` (AC) — закрыта **частично**, см. r2-M1 ниже | + +M1 и M2 закрыты полностью. M3 и M4 закрыты в части, ради которой их подняли +(структура/UI-контракт), но правка потянула за собой недоделанный хвост в +соседнем обязательном разделе — см. находки ниже. + +## Унаследовано из r1 + +Без повторной проверки (дельта их не касается) приняты выводы +`docs/reviews/SPEC-REVIEW-39-r1.md` на SHA `156be645`: + +- Технические анкеры кода (`_pickPlanFile` base64-цикл, aspect via ``, + `MAX_FILE_BYTES`, `check_quota`, транзакция staging-до-save) — сверены + построчно в r1, дельта их текст не меняла. +- Бенчмарк-матрица (4–165 МП, headless Chromium) и константы + `src/backdrop-probe.ts` — не менялись, «честная оговорка» про недоступный + reference-планшет присутствует так же. +- AC1–AC3, AC5–AC9 (кроме переименования AC4) — не менялись, признаны + проверяемыми в r1. +- Терминология («подложка», диалоги `hp-dialog`) соответствует + `docs/USER-GUIDE.ru.md` — сверено в r1, в дельте новых терминов, кроме уже + проверенных ниже i18n-ключей, нет. +- `docs/BACKDROP.md` (геометрия/калибровка) не конфликтует — задача его не + касается. +- Standing rules `docs/SCOPE.md` (never-delete-a-file, lock invariant) — + нерелевантны, не задеты. +- `docs/SCOPE.md` J4 («from zero to a working plan… image/PDF/draw») — задача + по-прежнему укладывается в надёжность onboarding-загрузки плана, сама задача + уже протриажена владельцем. + +## Как проверялось в этом раунде + +1. Прочитал вердикт r1 и его SHA в комментарии issue #39 (`156be645`, + явно назван автором ревью — не пришлось восстанавливать по догадке). +2. `git diff 156be645..HEAD -- docs/specs/039-large-backdrops.md` — построчно, + без сокращений (см. вывод выше). +3. Прочитал итоговый файл целиком (217 строк) — не только diff-хунки, чтобы + увидеть места, где новый текст должен был согласоваться со старым (i18n ↔ + UX, AC4б ↔ «Тесты и мутанты») и не согласовался. +4. Сверил новые i18n-ключи с существующей toast-инфраструктурой: + `this.host._showToast` уже вызывается ровно в `_pickPlanFile` + (`src/houseplan-editor-runtime.ts:8252`, `toast.plan_formats`) — тост из + этого же метода технически достижим, это не новый недоступный механизм + (комментарий про «only the View card owns toast infrastructure», + `houseplan-card.ts:2495`, относится к locale-failure по #354 и не запрещает + тосты из `HouseplanEditorRuntime`, который вызывает `this.host._showToast` + в этом самом методе на соседней строке). +5. `docs/specs/README.md:14–21` — канонический список обязательных + release-артефактов, включая условие «release/performance/security + artifacts, если они входят в acceptance gate» — сверил текущий раздел + «Release-артефакты» против этого списка пункт за пунктом. +6. Перечитал «Тесты и мутанты» (не менялся в дельте) против новых AC4/AC4б, + которые дельта менял — согласованность способа доказательства с новым + текстом AC. +7. `gh issue view 39 --comments` — тело + все 4 комментария, включая + комментарий владельца о ревизии 3 (совпадает с diff, новых устных решений + не содержит). +8. `docs/SCOPE.md` J4 — не переоценивал заново (см. «Унаследовано»), только + убедился, что новый текст сценария не расширяет скоуп задачи за пределы + onboarding-загрузки. + +## Гейты + +Diff — только `docs/specs/039-large-backdrops.md`, 0 файлов в `src/**`/ +`custom_components/**`. Как и в r1: + +| Гейт | Применимо? | Причина | +|---|---|---| +| `npx tsc --noEmit` / `npm test` / `npm run build` | нет | diff не содержит кода | +| `node scripts/check-docs.mjs` | нет | `src/**` не тронут | +| `npm run invariants` | нет | геометрия/`layout`/толщина стен не затронуты | +| browser-смоки, `golden:verify`, `pytest tests_backend` | нет | реализации по-прежнему нет, смоки из ТЗ не существуют | + +Ничего не прогонял намеренно — тот же аргумент, что в r1: docs-only коммит, +прогон гейтов не проверит предмет ревью этапа `spec`. + +## Находки + +Обе находки — Medium, в скоупе задачи (правятся автором ТЗ в этом же issue, +без отдельного issue). High-находок нет. + +### r2-M1 — AC4б добавлена, но «Тесты и мутанты» не обновлён под неё: способ доказательства не назван + +M4 (r1) требовал явного UI-контракта для второй фазы `hard` — он появился +(тост, сброс инпута, чистый staging, явный отказ от автофолбэка). Но раздел +«Тесты и мутанты» (`:169–179`) остался буквально тем же текстом, что был до +разделения AC4 на AC4/AC4б: смок описан одной строкой «hard-путь» без +уточнения, какую из двух физически разных фаз она проверяет (синхронная +классификация по заголовку до клика vs асинхронный decode-fail/таймаут 10 с +**после** клика «Уменьшенная копия»). Мутант (3) «`hard` понижен до +warn — смок красный» относится по формулировке только к фазе 1 (классификация +по заголовку). Ни один пункт не называет способ проверки для: + +- собственно триггера фазы 2 (как смок вообще заставит `createImageBitmap` + упасть или не уложиться в 10 с — мок глобала, форсированный reject, + укороченный таймер под тестами? ничего не сказано, а 10 секунд реального + ожидания в смоке — это сам по себе вопрос, который автор ТЗ обязан решить, + а не тест-автор кода); +- показа тоста именно с ключом `backdrop.downscale_failed`; +- сброса поля выбора файла (упомянуто в UX как факт, но не как утверждение, + которое тест обязан проверить); +- главной новой гарантии AC4б — **отсутствия автофолбэка на оригинал** (текст + прямо называет это осознанным решением: «молча грузить то, от чего + пользователь только что отказался, нечестно» — но ни один мутант реестра не + ловит регресс «фолбэк на оригинал появился»). + +Это ровно тот случай, когда правка по одному замечанию (M4) способна оставить +AC непроверяемым, если её не протянуть до соседнего обязательного раздела — +раздел «план автотестов» из §7.1 обязателен наравне с «Сценарием» и +«Release-артефактами», и для AC4б он сейчас пуст по существу (общая фраза +«hard-путь» её не покрывает, потому что фаза 2 — не то же самое действие, что +фаза 1, ни по триггеру, ни по результату на экране). + +**Как воспроизвести:** прочитать `:169–179` («Тесты и мутанты») рядом с +`:94–104` (UX) и `:155–159` (AC4/AC4б) — ни слова про decode-fail/таймаут, +тост, сброс инпута или мутант на «автофолбэк вернулся». + +**Правка:** одна-две строки в «Тесты и мутанты»: назвать способ форсировать +decode-fail/таймаут в смоке (мок `window.createImageBitmap`, ускоренный +таймер под флагом теста — на усмотрение автора, но названный), добавить +проверку ключа тоста и сброса инпута в описание смока, добавить пятый пункт в +реестр мутантов («автофолбэк на оригинал при decode-fail — тест/смок +красный»). + +### r2-M2 — «Release-артефакты» закрывает performance, но не называет security даже отрицательно + +M3 (r1) указывал на отсутствие «ни одного пункта» performance/security. +Ревизия 3 добавила явный пункт performance (`demo/benchmark_backdrop_decode.mjs`, +вне перф-гейта CI), но пункта security нет вовсе — ни утвердительного, ни +отрицательного. `docs/specs/README.md:21` требует называть +«release/performance/security artifacts, **если они входят в acceptance +gate**» — условие снимает обязательность, только если её снятие +*сформулировано*, а не просто отсутствует. Задача вводит новый парсер +заголовков произвольных PNG/JPEG/WebP-файлов, пришедших от пользователя +(`backdrop-probe.ts`, ручной разбор офсетов IHDR/SOF/VP8x) — именно тот код, +для которого вопрос «а не упадёт ли парсер на специально испорченном +заголовке» стандартно задаётся explicitly, а не подразумевается. Раздел +«Риски» касается близкой темы («зоопарк заголовков» → `unknown`, не throw), +но это довод про корректность, не про security-обзор конкретно. + +Не берусь сказать, нужен ли здесь полноценный security-гейт (решение +владельца/автора, не моё) — но ТЗ обязано *явно* сказать «не входит в gate, +потому что…», а не промолчать, иначе DoR §2.5 «release-артефакты +перечислены» не закрывается буквально: перечень сейчас неполон по своему же +шаблону (перечислены changelog/docs/golden/i18n/performance, но не security). + +**Как воспроизвести:** grep `docs/specs/039-large-backdrops.md` на `security` +— ноль совпадений; `docs/specs/README.md:21` требует явного решения по этому +пункту. + +**Правка:** одна строка в «Release-артефакты», например: «security: не входит +в gate — parser читает только фиксированные смещения в границах уже +считанного `Uint8Array`, без сети/eval, покрыт юнит-таблицей битых +заголовков (см. „Риски“)» — или, если автор считает иначе, назвать нужный +артефакт. + +## Что проверено и корректно + +- M1, M2 закрыты полностью и без остатка (см. таблицу выше). +- Новый раздел «Сценарий» отвечает на оба вопроса §7.1 одной фразой каждый, + без терминов реализации («вкладка замирает», а не «OOM»/«heap»). +- Новые i18n-ключи (кроме одного отступления, см. «Чего не проверял») дают + согласованные en/ru пары, используют существующую переменную-интерполяцию + в духе остального проекта (`{w}`, `{h}`, `{fileMb}`). +- Тост как механизм для фазы 2 технически достижим из того же метода, что уже + вызывает тост сегодня (`_pickPlanFile` → `this.host._showToast`) — не + выдуманный недоступный UI-примитив. +- «Риски» — 4 пункта, каждый со смягчением, включая явную честную оговорку + про недоступный reference-планшет (унаследована из r1, не потеряна при + переносе в новый раздел). +- «Откат» и «Вне скоупа» не менялись, остаются согласованными с новым + текстом (ни сценарий, ни i18n, ни risks/release-артефакты не расширяют + скоуп). +- Владельцу вопросов не задано — обе новые Medium-находки (r2-M1, r2-M2) + решаются автором ТЗ в рамках его компетенции, продуктового решения + владельца не требуют. + +## Чего не проверял + +- Реальное поведение в браузере — реализации по-прежнему нет. +- Название `backdrop.unknown_title` — в i18n-разделе для `unknown`-состояния + назван только `backdrop.unknown_body` (:119–122), без отдельного title-ключа; + текст предполагает переиспользование `backdrop.large_title` («Large image») + для диалога о файле с нечитаемыми размерами, что смыслово немного не + совпадает («не факт, что файл большой — просто не прочитались размеры»). + Не поднимаю это до Medium: реализация вправе переиспользовать существующий + заголовок или добавить свой — оба варианта не меняют AC6 и не блокируют + DoR, это чисто техническое решение в духе «assumed, change freely» из + самого М2. Отмечаю как Low, не блокирует; автор может добавить строку либо + оставить как есть. +- Возможность `hp-dialog` показывать 3 кнопки действия одновременно (нужно + для warn-диалога: «Уменьшенную копию»/«Оставить оригинал»/«Отмена») — не + нашёл готового прецедента с тремя действиями в текущем `houseplan-card.ts` + за разумное время поиска; это UI-компонентный вопрос кода, а не текста ТЗ, + и не был поднят ни в r1, ни ревизией 3 — не расширяю разбор на компонент, + которого дельта не касается. +- Точность чисел бенчмарка (4–165 МП) — не пересчитывал, как и в r1: не + предмет этого этапа. +- `test/backdrop-probe.test.mjs`, `demo/smoke_backdrop_guard.mjs` — не + существуют, появятся в реализации, их «умение падать» — предмет + код-ревью. + +## Вердикт + +0 High, 2 Medium (обе в скоупе, возвращаются автору для правки ТЗ в этом же +раунде, без отдельных issue). M1 и M2 из r1 закрыты полностью; M3 и M4 +закрыты в своей продуктовой/UI части, но каждая потянула недоделанный хвост в +соседнем обязательном разделе процесса (план автотестов для новой AC4б; +security-строка в release-артефактах) — оба хвоста мелкие (одна-две строки), +но DoR §2.5 буквально не закрыт, пока они не добавлены. + +**Вердикт: жёлтый · заход r2 · блокирующих циклов 1/4 · High: 0 · Medium: 2 → в задаче**