Files
2026-09-25 10:28:49 +00:00

18 KiB
Raw Permalink Blame History

SPEC-REVIEW-616-r1

  • Issue: https://github.com/Matysh/houseplan-card/issues/616
  • Этап: S4-spec-review (ревью ТЗ, PROCESS.md §2.4)
  • Трек: полный (аналитика владельца, комментарий #issuecomment-5805306423: «лёгкий трек: нет (новый UX-контракт, touch)»)
  • Материал: тело issue #616, раздел ## ТЗ (снимок на момент ревью, 2026-09-25, редакция r1); комментариев — два: вопросы владельцу и принятие умолчаний по всем четырём пунктам (#issuecomment-5830704284)
  • Заход: r1 · блокирующих циклов израсходовано 0 из 4
  • Роль: ревьюер ТЗ (не автор)

Скоуп ревью

ТЗ вводит компактную однострочную шапку View/редакторов на ширине окна ≤ 480 px: вкладки пространств + зум + кнопка-шестерёнка (у админа — ещё слот крестика редактора, #647), а все остальные действия (редакторы, общие настройки, PDF, помощь, сводная панель, объёмный вид, добавление пространства) переносятся во всплывающее меню шестерёнки. На > 480 px и в киоске поведение не меняется. Проверялось: обязательные разделы §7.1, однозначность и проверяемость каждого AC, соответствие описания «по коду» реальному коду, согласие со SCOPE.md, TOUCH-SUPPORT.md, UX-MODES.md, USER-GUIDE.ru.md, STYLING-HOOKS.md, полнота ответа на все четыре вопроса владельцу, отсутствие домыслов, выданных за факт.

Как проверялось

  1. docs/SCOPE.md — персона «Household members» на телефоне, для которой «View mode is the product for two of the three personas» — прямое попадание в это ограничение: задача убирает редакторские аффордансы и лишние ряды хрома именно на этой поверхности. Не пересекается ни с одним пунктом «Out of scope». В скоупе.
  2. docs/process/REVIEWER.md целиком и разделы PROCESS.md §2.4, §2.5, §7.1, §4, §7.2.
  3. Прочитан весь текст issue #616 и оба комментария владельца — все четыре вопроса закрыты умолчаниями («да / нет, скрыть на любой ширине / да, редакторы desktop-first / да, kiosk не меняется»), в ТЗ они отражены без искажения.
  4. Сверено описание «Проблема (по коду)» с src/houseplan-card.ts:10758-10852 построчно: заголовок карточки, nav.tabs (с .tabedit/.tabadd только при _canEdit — houseplan-card.ts:982,10778-10800), .modes (три вкладки режимов, только при _canEdit), .editor-close-slot (#647), .zoomctl, projection-toggle (только _labsIso && mode==='view' && !kiosk), три header-action (только при _canEdit), составная кнопка сводной панели (_summary?.renderControls, только !kiosk) — их состав и условия рендера в ТЗ переданы точно, ни один элемент шапки не упущен из таблицы меню в п.4 скоупа.
  5. Сверено .head { flex-wrap: wrap } и медиа-правило @media (max-width: 620px) в src/styles/dialogs.styles.ts:6-30 — описание «flex-wrap: wrap без правил для телефона, кроме уменьшения отступов на ≤ 620 px» совпадает дословно.
  6. Прочитаны docs/TOUCH-SUPPORT.md (контракт «View/kiosk — touch-first, редакторы — desktop-first», 44×44 таргет), docs/UX-MODES.md (текущее описание составной кнопки сводной панели и режимов — в шапке пока не упомянут телефонный кейс, ТЗ корректно планирует добавить его), docs/USER-GUIDE.ru.md §5 (абзаца «На телефоне» пока нет — ТЗ верно требует его добавить), docs/STYLING-HOOKS.md §7.3/7.4 (существующие хуки data-hp="settings"/"pdf"/"support"/"mode-tab"/"editor-close" и принцип «zoom остаётся в DOM внутри CSS-hidden киоск-шапки» — прямой прецедент для подхода ТЗ «прежние кнопки скрыты CSS, не удалены»), и docs/data-hp-contract.json (формат hooks.<name>{elements,since,audience} — новые header-menu/header-menu-item в ТЗ ложатся в этот формат). Противоречий не найдено; все перечисленные в разделе «Контракт и UX» файлы для правки названы точно.
  7. Подтверждено существование golden-фикстур german-view-mobile-light и version-mismatch-touch-light-ru (demo/golden/matrix.mjs:226,1060, demo/golden/baselines/baselines-index.json:34,179) и ещё ~13 фикстур на 390 px в матрице (диалоги, tray, decor-попапы) — компонент «и остальные мобильные с видимой шапкой» в AC8 не поимённый список, но задача снята §11.4: AC8 — предрелизный гейт (golden:verify), несовпадение любой не пересобранной фикстуры проявится диффом на CI независимо от того, назвал ли автор её в ТЗ явно (см. «Находки», Low-1).
  8. Подтверждено существование всех смоков, названных в «Плане тестов»: smoke_summary_panel(.mjs/_polish.mjs), smoke_support_feedback.mjs, smoke_gear_tabs.mjs, smoke_dialog_modal_recovery.mjs, smoke_houseplan_panel.mjs, smoke_isometric_live_touch.mjs, smoke_toolbar_stable_width.mjs, scripts/smoke-select.mjs. Прочитан demo/smoke_toolbar_stable_width.mjs:9-40 — он действительно гоняет WIDTHS = [1400, 1000, 768, 390] и на 390 px измеряет видимость трёх [data-hp="mode-tab"] (visibleTabs, ожидает width > 0); после правки .modes на ≤ 480 px будет display:none, и этот же смок красным подтвердит правильность утверждения риск-раздела ТЗ («проверка режимов остаётся на 768/1000/1400, на 390 проверяется слот ×») — риск описан точно, а не декларативно.
  9. Проверен потолок строк: test/core-file-budget.test.mjs:40 — 'src/houseplan-card.ts': 12889; wc -l показывает 12886 — запас 3 строки. ТЗ верно определяет это как жёсткий риск и явно выносит логику меню в новый src/header-menu.ts, а не расширяет монолит — без этого решения задача не проходила бы гейт core-file-budget почти гарантированно.
  10. Проверены локали en/ru/de/fr (src/i18n/{en,de,fr,ru}.json реально существуют) — новый ключ title.header_menu заявлен для всех поддерживаемых языков, ни один не пропущен и лишних не добавлено.
  11. Сверен таргет 44×44 с существующим стандартом docs/TOUCH-SUPPORT.md:14,72 — переиспользует принятую метрику, не изобретает новую.
  12. Проверены все восемь AC на однозначность и способ доказательства: каждый привязан к конкретному смоку/юниту и называет мутацию, на которой тест краснеет («чем краснеет» не пустая ни в одной строке) — включая защитные AC4 (отказ клика мимо меню) и AC6 (лимит размера тапа).

Находки

Low-1 — AC8 не перечисляет поимённо весь набор golden-фикстур

Файл: тело issue #616, раздел «Критерии приёмки», строка AC8.

AC8 называет по имени только german-view-mobile-light и version-mismatch-touch-light-ru, остальное — общей фразой «и остальные мобильные с видимой шапкой». В матрице (demo/golden/matrix.mjs) не менее ~15 фикстур на 390 px, часть из них — с открытым диалогом поверх шапки, где неочевидно, входит ли шапка в кадр диалога. Формально это блок с неоднозначной границей множества.

Не поднимаю до Medium: AC8 сам явно помечен предрелизным гейтом (golden:verify, §11.4), а не гейтом код-ревью, и любой не пересобранный кадр даёт пиксельный диф на CI независимо от того, был ли он поимённо анонсирован в ТЗ — недостающая пересъёмка сама себя обнаруживает, лишняя пересъёмка (кадр без видимых изменений) не ломает golden:verify. Снимаю без правки текста: при код-ревью после реализации достаточно свериться со списком реально изменившихся кадров в диффе, а не с этим перечислением.

Low-2 — AC2 не называет .projection-toggle в списке скрытых классов

Файл: тело issue #616, раздел «Критерии приёмки», строка AC2.

AC2 перечисляет классы, которые не должны быть видны у админа на ≤ 480 px (.modes, header-action, .summary-control, .tabedit, .tabadd), но не упоминает .projection-toggle (кнопка «Объёмный/Плоский вид», houseplan-card.ts:10820-10828), хотя по п.4 скоупа она тоже должна уйти в меню («только в просмотре, любая роль… как сейчас»). Формально смок, написанный буквально по перечню AC2, мог бы не проверить этот один класс отдельно.

Не поднимаю до Medium: тот же AC2 требует «одна строка ≤ 56 px» для ВСЕХ видимых элементов .head — если .projection-toggle останется видимым при включённом hp_alpha, это либо добавит четвёртую группу в строку (не .modes/header-action/.summary-control/.tabedit/.tabadd, но всё равно лишний элемент), либо не уместится в 56 px — оба случая ловятся тем же измерением высоты/переноса, которое уже входит в AC2. Дыры в приёмке нет, только в удобстве чтения перечня. Снимаю без правки текста.

Оба Low не блокируют; ни один не требует остановки конвейера.

Что проверено и корректно

  • Присутствуют все обязательные разделы §7.1: сценарий и персона (одним абзацем, без терминов реализации — «одна строка ≤ 56 px» вместо ссылок на классы), «что человек увидит до/после» (интегрировано в тот же абзац, явно посчитаны текущие ≈106/≈156 px хрома), проблема по коду, скоуп и не-скоуп (полностью раздельны, без пересечения с #437/#629/#485/#53/#89), контракт поведения и UX (атрибуты data-hp, aria-*, размеры), модель данных/миграция/i18n, таблица AC1–AC8 с доказательством и мутацией, план автотестов, риски, откат, release-артефакты.
  • Все четыре продуктовых вопроса владельцу закрыты именно умолчаниями, указанными в аналитике, без домыслов от лица владельца.
  • Таблица меню в п.4 скоупа покрывает все реальные элементы текущей шапки без остатка (см. «Как проверялось», п.4) — ни один существующий контрол не потерян и не задублирован.
  • «Принято предположительно» корректно разграничивает продуктовое (уже решено владельцем) от инженерного (порог по ширине окна, а не карточки; disclosure- а не menu-паттерн; модуль src/header-menu.ts; подложка для тапа мимо) — ревьюер не оспаривает ни один пункт, они действительно нейтральны для пользователя.
  • Откат («один коммит, данных не касается») и release-артефакты (оба changelog + 4 канонических документа + golden предрелизно) — полны и не требуют технического решения владельца.
  • Открытых продуктовых вопросов к владельцу не осталось — все сняты предыдущим циклом аналитики; технических вопросов, ошибочно вынесенных владельцу, в тексте нет.

Чего не проверял

  • Гейты (tsc, npm test, npm run build, check-docs.mjs, смоки, golden, инварианты) — не прогонял: этап spec, продуктового кода к этой правке ещё нет; предмет ревью — текст ТЗ в теле issue, а не диапазон коммитов. Это станет предметом код-ревью после реализации.
  • Не проверял визуально в браузере фактическую компоновку на 320/390 px — ревью ТЗ не предполагает исполнения/ручного тестирования UI; все выводы о соответствии коду сделаны чтением src/houseplan-card.ts и src/styles/dialogs.styles.ts, а не измерением в реальном рендере.
  • Не оценивал производительность открытия/закрытия меню — ТЗ не меняет геометрию сцены и не вводит новых тяжёлых пересчётов (чистая функция состава + CSS-попап), риск явно ниже порога, требующего отдельного раздела; по прецеденту SPEC-REVIEW-647-r1 для CSS-only правок такого масштаба это не считается пропуском ТЗ.
  • Не оценивал реализуемость «≤ 56 px» и «≥ 44 px» одновременно на пиксель — это инженерная, а не продуктовая величина; арифметически совместимо (56 − 44 = 12 px запаса на отступы), проверка факта — гейт код-ревью.

Вердикт

Найдено 0 High и 0 Medium в скоупе задачи; 2 Low сняты ревьюером с записью (см. «Находки»), правки текста не требуют. ТЗ переходит в «Готово к разработке».


Материал раунда

  • Ветка: dev, коммит e52afe63495b — ребейз его осиротит, и это нормально: ниже якоря, которые ребейз не меняет.
  • Дерево материала: 0977af5c3a98f43e9e74c633f2817e204e35f673
    git log --all --format='%H %T' | grep 0977af5c3a98
    
  • Тело issue: d31c85f266be16c9f37e1df9ea9155014dea6fcc17761e493e3a6945ad360df9
  • Вердикт конвейера: green · High 0