diff --git a/docs/reviews/SPEC-REVIEW-39-r1.md b/docs/reviews/SPEC-REVIEW-39-r1.md new file mode 100644 index 00000000..eec66d65 --- /dev/null +++ b/docs/reviews/SPEC-REVIEW-39-r1.md @@ -0,0 +1,237 @@ +# SPEC-REVIEW-39-r1 + +- Issue: https://github.com/Matysh/houseplan-card/issues/39 — «[HP-UX-09] большие подложки» +- Этап: ТЗ на ревью (PROCESS.md §2.4) +- Заход: r1 · блокирующих циклов израсходовано 0/4 +- ТЗ: `docs/specs/039-large-backdrops.md`, ревизия 2, SHA `156be645` +- Трек: полный (владелец, 2026-08-15: «P3, polish/tech-debt, обычный трек») +- Предыдущего вердикта по этому ТЗ нет — это первый проход ревью, раздел + «Унаследовано из r0» не применяется, разбор полный по §2.10 (не второй цикл). + +## Скоуп проверки + +Диагностика разрешения/decoded-памяти PNG/JPEG/WebW до какой-либо тяжёлой +аллокации, warn/hard-диалог, клиентский downscale до 4096px, замена ручного +base64-цикла на `FileReader`. Один файл изменён в этом коммите: +`docs/specs/039-large-backdrops.md` (165 строк, Class C). Продуктового/кода +diff нет — ревью касается только текста ТЗ. + +## Как проверялось + +1. `docs/SCOPE.md` — сценарий укладывается в надёжность onboarding-загрузки + плана (соседствует с J4), сама задача уже протриажена владельцем как + valid tech-debt (комментарий от 2026-08-14), продуктового конфликта со + стандартными правилами (never-delete-a-file, lock invariant) нет. +2. `AGENTS.md`, `PROCESS.md` §2.3–2.5, §7.1 — состав обязательных разделов ТЗ + и критерии DoR. +3. Issue #39: тело + оба комментария (аналитика 2026-08-14, актуализация + 2026-08-29). Вопросов к владельцу автор не поднимал («Вопросы: нет»). +4. `docs/USER-GUIDE.ru.md` — сверил терминологию: «подложка», «Редактор + подложки», диалоги `hp-dialog` — используются в ТЗ корректно, не + изобретены. +5. `docs/CANVAS.md` → обнаружил ссылку на `docs/BACKDROP.md` (канонический + документ калибровки/размещения подложки на канвасе, не входит в список + в промпте, но напрямую относится к подсистеме). Прочитан: описывает + `plan_x/y`, scale, rotation — геометрию подложки, а не upload/decode. + Задача этой геометрии не касается («Вне скоупа» ТЗ подтверждает), конфликта + нет. +6. Сверка технических якорей ТЗ с текущим кодом (раздел «не додумывается — + выносится» требует, чтобы заявленные факты о коде были фактами, а не + догадкой): + - `_pickPlanFile` (`src/houseplan-editor-runtime.ts:8243`) — ручной + base64-цикл `String.fromCharCode` подтверждён построчно (:8253-8255); + разрешение действительно не проверяется; aspect действительно берётся + через `.onload` (:8261-8266) — то есть полный decode уже + происходит на этом шаге. Совпадает с текстом ТЗ дословно. + - `MAX_FILE_BYTES = 50 * 1024 * 1024` подтверждён в + `custom_components/houseplan/validation.py:24`, использование в + `http_api.py:252` — совпадает с «50 МБ» в ТЗ. + - `check_quota` в `custom_components/houseplan/plans.py:216` — квота + подтверждена, комментарий рядом («Files are never removed for getting + old») согласуется с standing rule SCOPE.md «never delete a user's file». + - Транзакционный комментарий в `_saveSpaceDialog` + (`src/houseplan-editor-runtime.ts:~8375-8390`, «Upload BEFORE touching + the config...») подтверждает заявление ТЗ «staging не трогает конфиг до + успешного save» — это не догадка, а верно прочитанный существующий код. + Вывод: технические анкеры ТЗ точны, автор не выдаёт предположение за факт + там, где дело касается существующего кода. +7. Сверка ревизии 1 → 2 (`git show 156be645^:docs/specs/039-large-backdrops.md` + vs текущая) — чтобы не приписать ревизии 2 разрыв, унаследованный от + ревизии 1. Структурные пробелы (см. находки ниже) присутствовали уже в + ревизии 1 — не новый регресс, но это первый **ревью** документа, поэтому + они предъявляются сейчас. +8. i18n: подтвердил, что проект поддерживает en/ru/de (`src/i18n/{en,ru,de}.*`), + так что «Тексты en/ru/de» в ТЗ — не опечатка и не лишний язык. +9. `docs/TOUCH-SUPPORT.md` — `_pickPlanFile` живёt только в Plan editor + (`houseplan-editor-runtime.ts`), редакторы — desktop-first/best-effort, + View/kiosk (блокирующая часть touch-контракта) это изменение не + затрагивает. Конфликта нет, но ТЗ не проговаривает это явно (см. находки). + +## Гейты + +Диапазон изменений — только `docs/specs/039-large-backdrops.md` (Class C, +0 файлов в `src/**`/`custom_components/**`). Код не менялся, поэтому: + +| Гейт | Применимо? | Причина | +|---|---|---| +| `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` | нет | нет реализации, смоки из ТЗ (`demo/smoke_backdrop_guard.mjs`) ещё не существуют — они появятся в коде, не в этом коммите | + +Ничего не прогонял намеренно — прогон дорогих или дешёвых гейтов на +docs-only коммите ничего не проверяет по существу и не относится к предмету +ревью этапа `spec`. + +## Находки + +Все находки — Medium, в скоупе задачи (правятся автором ТЗ в этом же issue, +без отдельного issue). High-находок нет: ТЗ реализуемо и в основном +проверяемо, ни одна находка не требует решения владельца. + +### M1 — нет обязательных продуктовых разделов §7.1 «сценарий» и «что человек увидит» + +PROCESS.md §7.1: «Два первых раздела — продуктовые... Сценарий: какая +персона, на какой поверхности, в какой момент. Что человек увидит: одной +фразой, без терминов реализации. ТЗ, которое не может ответить на эти два +вопроса, описывает работу, а не изменение продукта.» + +Текущий документ начинается сразу с «Цель» → «Текущее состояние (анкеры +кода)» — реализационный язык с первой содержательной строки. Персона (Home +admin), поверхность (Plan editor, диалог создания/замены пространства) и +момент (выбор файла подложки при создании/редактировании floor plan) нигде +не названы явно, хотя ответ на изучение (см. «Как проверялось», п.6, 9) есть — +что и делает пробел исправимым без обращения к владельцу. + +**Как воспроизвести:** прочитать `docs/specs/039-large-backdrops.md` — раздел +«Сценарий» и фраза «что человек увидит до/после» без терминов реализации +отсутствуют физически. + +**Правка:** добавить два первых раздела по шаблону §7.1. + +### M2 — i18n-раздел не перечисляет ключи, DoR §2.5 не закрыть + +§7.1 требует раздел «i18n» в самом ТЗ; DoR §2.5 требует «ключи en + ru +перечислены» как отдельный обязательный пункт. Текущий текст — одна строка +внутри UX: «Тексты en/ru/de; диалог на мобильной ширине без горизонтального +скролла» — ни одного имени ключа. + +Три новых пользовательских текста как минимум (warn-заголовок/тело, +кнопка «Загрузить уменьшенную копию», кнопка «Оставить оригинал», hard-текст +с советом уменьшить файл) не имеют предполагаемых ключей — это чисто +техническое решение (§7.1: «Всё, чего пользователь не наблюдает [в смысле +имени ключа], агенты решают сами»), но раздел должен *содержать* список, +не быть пустым. + +**Как воспроизвести:** grep `docs/specs/039-large-backdrops.md` на `\.title\|\.body\|key` — +ни одного предложенного ключа. + +**Правка:** перечислить en+ru ключи (можно предположительно, «assumed, change +freely») — это разблокирует DoR-пункт. + +### M3 — нет разделов «Риски» и «Release-артефакты», обязательных по §7.1 + +§7.1 перечисляет обязательные разделы: «...план автотестов · **риски** · +откат · **release-артефакты**». В документе есть «Откат» и «Тесты и +мутанты», но нет ни выделенного раздела «Риски» (хотя по существу риск +называн — «честная оговорка» про недоступный reference-планшет, — он не +собран в заявленный по процессу раздел и не сопровождён явной пометкой +«принято предположительно, поменять свободно», как требует §7.1 в конце +документа), ни раздела «Release-артефакты» (changelog RU+EN, документация, +golden/скриншоты, performance/security — ни один пункт не назван, хотя +`User-Visible: yes` почти наверняка потребуется на новый UX-диалог). + +**Как воспроизвести:** grep заголовков `^##` в файле — «Риски» и +«Release-артефакты» отсутствуют. + +**Правка:** добавить оба раздела; риски можно перенести из «Честной +оговорки», Release-артефакты — минимум подтвердить `User-Visible: yes` и оба +changelog. + +### M4 — AC4 объединяет два разных момента наступления `hard` без единого UI-контракта + +`probeBackdrop` классифицирует `hard` синхронно по заголовку (сторона +>16384px) — до какого-либо клика пользователя, поэтому в этом случае +Cancel-only диалог логично показывается сразу вместо `safe`/`warn`. Но АС4 +также включает «decode-ошибка или таймаут 10 с **на этапе уменьшения**» — +это происходит ПОСЛЕ того, как пользователь уже увидел `warn`-диалог и нажал +«Загрузить уменьшенную копию» (шаг из UX-раздела, «Уменьшение: +`createImageBitmap`... OffscreenCanvas... aspect сохраняется»). В этот момент +пользователь уже закрыл первый диалог и, вероятно, видит какое-то состояние +busy/loading. + +ТЗ не говорит, что именно пользователь увидит при отказе на этом втором +шаге: тот же Cancel-only диалог переоткрывается, показывается инлайн-ошибка +поверх текущего состояния, или тост — сказано только «текущая подложка и +staging не изменились» (что описывает данные, а не экран). Это ровно тот +класс вопроса, который §7.1 called «продуктовый» («что человек видит или +делает» в пограничном случае) и который автор обязан решить явно (не +обязательно спрашивая владельца — решение внутри компетенции автора, но оно +должно быть записано, а не подразумеваться). + +Без этого AC4 не полностью проверяем одним однозначным тестом: тест-автор +должен будет сам придумать, какой UI ожидать после ошибки на втором шаге, и +разные реализации/тесты разойдутся. + +**Как воспроизвести:** перечитать AC4 и раздел UX «hard» — оба говорят про +данные (staging/config не меняются), ни один не описывает экран после +runtime-отказа downscale, случившегося уже после клика в `warn`-диалоге. + +**Правка:** одна фраза, например «повторно показывается тот же hard-диалог поверх +прерванного действия» или «инлайн-ошибка в уже открытом warn-диалоге, без его +закрытия» — и синхронизировать с AC4. + +## Что проверено и корректно + +- Технические анкеры кода (base64-цикл, aspect via ``, MAX_FILE_BYTES, + quota, staging-до-save транзакция) — точны, сверены построчно с текущим + `dev`, не догадка. +- Бенчмарк-матрица (4–165 МП) внутренне согласована, источник (headless + Chromium, `createImageBitmap`+downscale) назван, ограничение (нет + reference wall-tablet) явно оговорено, а не скрыто; направление ошибки + консервативно (раньше предупреждаем, а не позже) — это разумная инженерная + позиция, а не выданная за факт догадка. +- Константы собраны в одном модуле (`src/backdrop-probe.ts`) с явной целью + «полевая рекалибровка = правка одного файла» — соответствует духу + «assumed, change freely». +- AC1–AC3, AC5–AC9 — каждый привязан к способу доказательства (unit/smoke/ + ревью) и формулирует наблюдаемое поведение однозначно; для AC1 и AC9 в + тексте прямо продумано, как тест проверит «до-decode» инвариант (spy на + `window.createImageBitmap`, отсутствие canvas в юните) — «тест умеет + падать» проверяемо уже на этапе спеки через реестр мутантов (4 пункта, + каждый называет, что именно ломается). +- Терминология («подложка», диалоги) соответствует `docs/USER-GUIDE.ru.md`, + не изобретена. +- Не найдено нарушений standing rules SCOPE.md (never-delete-a-file, lock + invariant — оба нерелевантны этой задаче и не задеты). +- `docs/BACKDROP.md` (геометрия/калибровка подложки) не конфликтует — + задача его не касается, что подтверждено разделом «Вне скоупа». +- Раздел «Вне скоупа» и «Откат» по существу закрывают «модель данных и + миграция» (эндпоинты/схема не меняются) — содержательно ОК, только не под + отдельным заголовком (см. M3, куда это включено). + +## Чего не проверял + +- Реальное поведение в браузере — реализации ещё нет, диалог/downscale не + существуют в коде; нечего запускать. +- `demo/benchmark_backdrop_decode.mjs` — упомянут как будущий файл, в + репозитории отсутствует (`git ls-files` не находит), не мог сверить числа + бенчмарка независимо — доверился as-is как research-отчёту автора, + корректность самих чисел проверке на этом этапе не подлежит (это войдёт в + реализацию). +- Не оценивал производительность реального downscale на low-end wall-tablet + устройстве — сам автор честно называет это недоступным; ревью не может + закрыть то, что недоступно и автору. +- Не проверял `test/backdrop-probe.test.mjs` и `demo/smoke_backdrop_guard.mjs` + — они не существуют, появятся в реализации; их «умение падать» — предмет + код-ревью, не этого этапа. + +## Вердикт + +0 High, 4 Medium (все в скоупе задачи, возвращаются автору для правки ТЗ, +без отдельных issue). Технический фундамент ТЗ хороший (анкеры кода точны, +бенчмарк честен, AC в основном проверяемы), но структурная неполнота против +чек-листа §7.1 (M1–M3) и одна реальная развилка поведения без явного решения +(M4) не позволяют закрыть DoR §2.5 как есть. + +**Вердикт: жёлтый · заход r1 · блокирующих циклов 1/4 · High: 0 · Medium: 4 → в задаче**