diff --git a/docs/reviews/SPEC-REVIEW-149-r1.md b/docs/reviews/SPEC-REVIEW-149-r1.md new file mode 100644 index 00000000..5fb41322 --- /dev/null +++ b/docs/reviews/SPEC-REVIEW-149-r1.md @@ -0,0 +1,264 @@ +# Ревью ТЗ — issue #149, цикл r1 + +- **Issue:** https://github.com/Matysh/houseplan-card/issues/149 +- **ТЗ:** `docs/specs/149-touch-view-settings-affordance.md`, коммит `2d5f0a5f0f66faf1614a9f1f367ce108011fcfbe` +- **Этап:** spec (PROCESS.md §2.4) +- **Трек:** обычный (не `small`/`trivial` — сам issue называет причину: новый + видимый элемент, таймер, снятие существующего жеста, попадание в геометрию + контура, затронуты все три персоны); лимит цикла — 4 +- **Вердикт:** красный · цикл r1/4 · High: 1 · Medium: 0 + +## Скоуп ревью + +Прочитаны: `docs/SCOPE.md`, `AGENTS.md`, `PROCESS.md` (§1, §2.2–2.5, §3, §4, +§7.1–7.2, §12), тело issue #149 и все три комментария владельца (аналитика с +Q1–Q3 и defaults, решение владельца по Q1–Q3, объявление о готовом ТЗ), весь +текст `docs/specs/149-touch-view-settings-affordance.md`, `docs/UX-MODES.md` +(принцип режимов, политика ввода, kiosk-раздел), `docs/TOUCH-SUPPORT.md` +(контракт, safety floor, deliberate degradation), `docs/CANVAS.md` (§1–4, +модель content frame — чтобы не путать её с новым «physical footprint»), +`docs/WALL-THICKNESS.md` (§1–4, модель стен/partition/column, кэширование +структурного прохода), `docs/USER-GUIDE.ru.md` (раздел 6 «Навигация, масштаб и +жесты», раздел 17 «Киоск-режим», полнотекстовый поиск терминов «настройки +вида» и «киоск»), `docs/CONFIG-COMPATIBILITY.md` (чтобы подтвердить, что задача +действительно не задевает реестр совместимости server config). + +Дополнительно, поскольку ТЗ описывает удаляемое/переносимое поведение как +факт о текущем продукте, а не как предположение — прочитан сам код: +`src/houseplan-card.ts` (`_stagePointerDown` ок. строк 5082–5104, +`_kioskScale`/`_kioskDialog`/`LS_KIOSK` ок. строк 1551–1553, 2313–2318, +14244–14277, применение множителя в devlayer — строка 14725, +`_interruptViewGesture` — строка 4300), `src/i18n/ru.json`/`en.json` (ключи +`kiosk.*`), `demo/smoke_kiosk.mjs`, `docs/CHANGELOG.md` (запись v1.41.0, где +эта функция впервые появилась) и `docs/TESTING.md` (пункт «Kiosk mode», +строки 768–774). + +## Как проверялось + +1. Сверил формальные обязательные разделы ТЗ (PROCESS.md §7.1) с текстом + `docs/specs/149-touch-view-settings-affordance.md`: сценарий и персона/ + поверхность/момент — §1; что человек увидит до/после — §1; скоуп и + не-скоуп — §4/§5; контракт поведения — §6–9; UX — §8/§10; модель данных и + миграция — §11; i18n — §10/§14; AC1…AC12 — §12; план автотестов — §13; + риски — §15; откат — §15; release-артефакты — §14. Раздела с буквальным + заголовком «Проблема» нет, но его содержание (почему долгий тап — плохой + affordance) присутствует в issue и подразумевается мотивацией §1; отмечаю + как Low ниже. +2. Прогнал через код каждое фактическое утверждение ТЗ о «существующем» + поведении (§1, §4 п.4, §9, §11), а не поверил формулировке — именно это + PROCESS.md называет «догадкой, выданной за решение», и именно на это + ревьюер обязан ловить автора. +3. Проверил все три owner-defaults (Q1–Q3 из комментария + `#issuecomment-5298450448`, подтверждённых `#issuecomment-5301106992`) на + соответствие тексту ТЗ §2 — совпадают буквально. +4. Проверил геометрические термины ТЗ (§6: «physical footprint», «unbounded + exterior», holes/courtyard, detached porch/terrace) на отсутствие + конфликта с уже канонической моделью `CANVAS.md` (content frame — другая + сущность, используется для камеры, не для hit-теста фона) и + `WALL-THICKNESS.md` (thick walls, partitions, columns, кэш по structural + fingerprint) — конфликтов не нашёл, переиспользование корректно оговорено + как предположение №1 в §16. +5. Проверил i18n-термин «Настройки вида» на предмет «терминология изобретена, + а не взята из USER-GUIDE» — в `docs/USER-GUIDE.ru.md` этой фразы сейчас + нет вообще (полнотекстовый поиск — 0 совпадений); фраза происходит из + заголовка/тела issue (то есть от владельца), не придумана автором ТЗ, и + ТЗ уже включает обновление USER-GUIDE в release-артефакты (§14) — это + штатный путь введения нового термина, не нарушение. +6. Проверил каждый AC (§12, 12 пунктов) на однозначность и на то, назван ли + способ доказательства в §13 — не построчным сопоставлением заголовков (в + ТЗ они не пронумерованы 1:1 с AC), а по содержательному пересечению + формулировок. + +## Находки + +### High-1 — §11 утверждает как факт то, что код опровергает; §4 не включает +необходимое для этого утверждения изменение + +**Что не так.** §11 «Модель данных и compatibility» пишет: «kiosk и normal +View используют один per-screen value, **как и до изменения**» — то есть +заявляет, что до этой задачи обычный View и kiosk уже одинаково применяют +`_kioskScale` (масштаб иконок/текста) к рендеру. Это не так. + +**Воспроизведение по коду (проверено чтением, не исполнением):** + +- `src/houseplan-card.ts:5088` — весь блок, открывающий диалог размеров по + долгому тапу, обёрнут в `if (this._kiosk) { … }`. В обычном View + (`kiosk: false`) этот код не выполняется вовсе — то есть сегодня в обычном + View нет *никакого* способа, ни скрытого, ни явного, открыть этот диалог. +- `src/houseplan-card.ts:14725` — множитель фактически применяется тоже + только в kiosk: `` this._kiosk ? this._kioskScale.icon : 1 `` и то же для + `--rl-font`. Значение читается из `localStorage` (`LS_KIOSK`, + строки 2313–2318) независимо от режима, но **применяется к рендеру** icon/ + font только когда `this._kiosk === true`. +- `docs/USER-GUIDE.ru.md:185–193` — таблица жестов документирует «Сброс + киоск-масштаба» и «Локальный размер киоска» строго в столбце «Сенсорный + экран» **киоска**, без единой строки для обычного View; это не пробел в + документации, это точное описание текущего продукта. +- `docs/CHANGELOG.md:2826–2836` (v1.41.0) — функция изначально задумана и + описана как kiosk-only: «every tablet/TV tunes itself once». Ограничение + не случайно, это осознанное решение выпуска 1.41.0, а не забытый кусок кода. + +**Почему это блокирует, а не техническая деталь на усмотрение автора.** +Вопрос «увидит ли пользователь эффект от сдвига слайдера в новом диалоге, +открытом кнопкой в обычном View» — продуктовый и наблюдаемый, а не +внутренняя реализация. Как написано сейчас, реализация по ТЗ откроет в +обычном View **тот же диалог с теми же слайдерами**, но применение +`this._kiosk ? … : 1` в devlayer никто не просит поменять — §4 «Скоуп» не +называет это пунктом работы, §16 «Принятые технические предположения» не +называет это допущением, а §11 прямо утверждает обратное факту. Результат: +кнопка появится у домочадца и гостя на телефоне/планшете (как и требует +owner-решение «доступны всем touch-поверхностям, отдельного поведения для +киоска не делаем»), но движение слайдера «Размер значков устройств» ничего +не изменит на экране — рабочий контрол, который визуально не работает. Это +прямое попадание в `docs/TOUCH-SUPPORT.md`: «A touch-only failure in View is +a product defect, not an accepted limitation of the editor policy» — только +здесь дефект дня выпуска, а не более позднего открытия. + +Это также ровно тот класс дефекта, который PROCESS.md называет «худшим +видом»: утверждение о поведении («используют один per-screen value, как и до +изменения»), которого не подтверждает ни один документ и которое не помечено +как предположение — оно читается как решённый факт и поэтому проходит ревью +на автомате, если его не сверить с кодом. + +**Не эскалируется владельцу как новый вопрос.** Сам факт нужности такого +изменения не требует решения владельца: намерение уже зафиксировано его же +формулировкой «Настройки вида доступны всем touch-поверхностям… отдельного +поведения для киоска не делаем» (issue, «Решения владельца», +подтверждено `#issuecomment-5298450448`). Контрол, открытый на всех +поверхностях, но работающий только на части из них, — это не то, что +владелец согласовывал; значит нужно не спрашивать, а **дописать ТЗ**: явно +включить в §4 «Скоуп» снятие/обобщение условия `this._kiosk ?` в применении +множителя к рендеру (или эквивалентное решение), завести под это отдельный +AC («изменение слайдера в обычном View видимо меняет размер иконок/текста +так же, как в kiosk») и соответствующий unit/smoke пункт в §13, и поправить +§11, чтобы он описывал целевое, а не мнимое текущее поведение. + +**Серьёзность.** High — блокирует переход в `S5-ready`. Без исправления +задача либо тихо доставит нерабочий контрол в двух из трёх персон (что +`docs/SCOPE.md`/`TOUCH-SUPPORT.md` не допускают), либо реализация по ходу +дела сама «дорешит» продуктовый вопрос без записи, что запрещено §7.1 +(«размытое место не додумывается, а выносится либо явно фиксируется»). + +## Что проверено и корректно + +- **Owner-решения Q1–Q3 перенесены буквально.** §2 ТЗ воспроизводит все три + default-а (`touch/coarse only`, 5-секундный таймер с перезапуском и + немедленным скрытием на pan/zoom/смену пространства, «вне плана» = + unbounded exterior с исключением detached porch/terrace и holes) без + искажений и без добавления новых решений от себя. +- **Геометрический контракт (§6) не путает две разные модели.** «Physical + footprint» (архитектура: floor+walls+partitions+columns, без decor/devices/ + Glow/sun) корректно отличается от `contentFrame` из `CANVAS.md` (камера/ + fit, включает decor и devices) — общий пул терминов не смешан, никакой + скрытой ре-дефиниции существующего понятия нет. +- **Производительность (§6.3, AC12).** Требование «топология считается + только при structural fingerprint change, не на pointermove/HA tick» + прямо аналогично уже работающей модели `WALL-THICKNESS.md` §2/§3 («one + cached structural pass in flat renderers; live HA state ticks do not + repeat the boolean topology») — это перенос уже принятого паттерна, а не + новая непроверенная идея. +- **Владение жестом (§7) корректно разграничивает hit-test фона и клики по + интерактивным целям**, явно перечисляет device/room/opening/vacuum/header/ + dialog как приоритетные над новым «чистым тапом», и явно требует, чтобы + `preventDefault()` не расширялся за пределы нового чистого тапа — это + прямое соответствие safety floor `TOUCH-SUPPORT.md` (`pinch`/`pointercancel`/ + второй палец не должны читаться как клик). +- **Права (§9).** Явно и неоднократно (§9, AC3, AC11) отделяет новый affordance + от административного `_settingsDialog`/«Общих настроек» и от `_canEdit` — + соответствует SCOPE.md (lock invariant в духе «намеренно, не по случайности» + распространён здесь на права: контрол для non-admin не получает доступа к + admin-поверхности). +- **Accessibility (§10).** Требование «focusable button, не SVG-only», + локализованные aria-label, focus trap диалога, приостановка auto-hide при + keyboard focus — конкретно и проверяемо, ссылается на существующий dialog + contract, а не изобретает новый. +- **Совместимость (§11), помимо разобранного выше пункта.** Отсутствие + миграции, отсутствие новых серверных полей, отсутствие новых полей + `localStorage` подтверждено — задача действительно не задевает + `docs/CONFIG-COMPATIBILITY.md` (реестр — про server config/backend + validation, сюда не относится), и в «Связано» этот документ обоснованно не + включён. +- **Термин «Настройки вида» не изобретён автором ТЗ** — он происходит от + формулировки владельца в issue и вводится в канон через release-артефакты + §14 (обновление `docs/USER-GUIDE.ru.md`), что штатный путь. +- **Не-скоуп (§5) конкретен и вычёркивает реальные соседние риски** + (desktop mouse/hover, «Общие настройки», миграция диапазонов масштаба, + постоянная кнопка в header, показ по внутренним поверхностям) — не общая + фраза «остальное не трогаем». +- **Откат (§15)** описан предметно: удаление временной кнопки, возврат кода + без миграции, совместимость `localStorage`/server config — проверяем без + code review. +- **Трек и лимит цикла** выбраны верно: задача явно не `small`/`trivial` по + собственному разделу «Влияние» issue, лимит цикла — 4, документ лежит по + канону `docs/specs/149-touch-view-settings-affordance.md`. + +## Low — не блокируют, зафиксированы с решением ревьюера + +1. **Нет раздела с буквальным заголовком «Проблема».** Содержание есть + (в issue и в мотивации §1), но в самом файле ТЗ не выделено отдельным + заголовком, как того просит PROCESS.md §7.1. Снимается без правки: смысл + присутствует и не расходится с issue; при следующей правке (см. High-1) + стоит добавить подзаголовок «Проблема» из issue «Почему это стоит сделать» + заодно, но отдельного цикла ревью на это не требуется. +2. **AC8, AC10, AC12 не имеют явно названного способа доказательства в §13.** + §13 сгруппирован по категориям (Unit / Touch smoke / Golden), а не 1:1 по + номеру AC, и для AC8 («desktop mouse/hover не меняется, mouse click не + показывает кнопку»), AC10 (accessibility/focus) и AC12 (geometry cache) + не нашлось однозначно соответствующего пункта ни в одном из трёх списков. + Не блокирует понимание контракта, но при возврате на исправление High-1 + стоит добавить: unit-пункт на `pointerType !== 'touch'/coarse'` → кнопка + не показывается; smoke/unit-пункт на focus trap и aria-label; unit/perf- + пункт, считающий вызовы построения footprint на серию тапов без изменения + геометрии. Снимается автором в той же правке, без нового issue — это + дополнение к тесту уже принятого AC, а не отдельная находка о продукте. +3. **Название кнопки и название диалога расходятся терминологически.** + Предложенный aria-label/title кнопки — «Настройки вида» / «View settings» + (§10), а существующий заголовок диалога в `ru.json`/`en.json` + (`kiosk.title`) — «Размеры на этом экране» / текущий английский эквивалент. + Тап по кнопке «Настройки вида» открывает диалог, который сам себя называет + иначе. Не мешает работе функции и не противоречит ни одному документу — + отмечаю, чтобы при реализации выбрали одно из двух: либо оставить как + мелкое, но заметное расхождение вида, либо привести название диалога и + aria-label к одному термину в release-артефактах (§14 это разрешает). + +## Чего не проверял + +- Реализацию — кода `src/**` под это issue ещё нет (ветка + `issue/149-touch-view-settings-affordance` на момент ревью содержит только + файл ТЗ, `git log --oneline` подтверждает единственный коммит `2d5f0a5`). + Проверка кода на этом этапе не требуется PROCESS.md §2.4. +- Golden-эталоны и живые performance-профили — не существуют до реализации; + §13 описывает план, не результат. +- Полный текст `docs/SUN.md`/`docs/LIGHT.md` — не читал построчно: задача не + трогает освещение/солнце (§5 явно перечисляет их эффекты как то, что не + расширяет physical footprint, но не меняет), релевантно только фактом, что + их эффекты не входят в hit-test, что уже подтверждено §6.1 текста ТЗ, и + этого достаточно для этого ревью. +- `docs/specs/README.md` — не проверял, зарегистрирована ли запись на этот + документ там; при пересмотре в r2 стоит свериться, но это не продуктовая + находка. +- Существующие браузерные smoke-файлы (`demo/smoke_kiosk*.mjs` и др.) прочитаны + только частично, ровно в объёме, нужном чтобы подтвердить/опровергнуть + утверждения ТЗ о текущем поведении (см. «Как проверялось» и High-1); + полный аудит покрытия smoke-suite не проводился — на этапе ТЗ без + реализации это не даёт дополнительной информации. + +## Вывод + +ТЗ методологически сильное: geometry-контракт корректно отделён от camera- +модели `CANVAS.md`, owner-решения перенесены буквально, safety floor и права +разобраны конкретно, откат и не-скоуп предметны. Но в основании ТЗ лежит +фактическая ошибка о текущем продукте (§11: «kiosk и normal View используют +один per-screen value, как и до изменения»), которую код прямо опровергает: +сегодня масштаб иконок/текста применяется к рендеру только в kiosk, а в +обычном View — никогда, вне зависимости от значения в `localStorage`. Раз +owner-решение требует единого affordance на всех touch-поверхностях, эта +задача не «делает видимым существующий скрытый вход», а частично **создаёт +новую функцию для обычного View** — и эта часть работы не названа ни в +скоупе, ни в AC, ни как принятое допущение. Это ровно тот guess-as-fact, +который PROCESS.md прямо называет худшим видом дефекта, потому что он +выглядит решением и проходит ревью, если его не сверить с исходником. + +Возврат в `S3-spec`. Исправление: явно добавить в §4/§12/§13 работу по +применению множителя иконок/текста в обычном View (не только kiosk), +поправить факт в §11, и по возможности закрыть три Low-пункта той же правкой.