diff --git a/docs/reviews/INDEX.md b/docs/reviews/INDEX.md index 4ffc80a1..b510564d 100644 --- a/docs/reviews/INDEX.md +++ b/docs/reviews/INDEX.md @@ -1,9 +1,10 @@ # Индекс ревью -Генерируется `node scripts/reviews-index.mjs` (#635) — не редактировать руками. Документов: 1045, issue: 367. Вердикт: 🟢 зелёный · 🟡 жёлтый · 🔴 красный · ⚪ не распознан (свободная форма старых документов). H/M — число High/Medium по строке вердикта или заголовкам находок. Файлы — пути, названные в находках; ищите по имени файла: `grep form-kit INDEX.md`. +Генерируется `node scripts/reviews-index.mjs` (#635) — не редактировать руками. Документов: 1046, issue: 368. Вердикт: 🟢 зелёный · 🟡 жёлтый · 🔴 красный · ⚪ не распознан (свободная форма старых документов). H/M — число High/Medium по строке вердикта или заголовкам находок. Файлы — пути, названные в находках; ищите по имени файла: `grep form-kit INDEX.md`. | Issue | Документ | Этап · раунд | Вердикт | H | M | Находки | Файлы | |---|---|---|---|---:|---:|---|---| +| #647 | [SPEC-REVIEW-647-r1.md](SPEC-REVIEW-647-r1.md) | spec · r1 | 🟡 жёлтый | 0 | 2 | П.4 ТЗ переносит на новый слот прежнее; ТЗ не упоминает и не защищает документированный | `src/styles/dialogs.styles.ts` `src/styles.ts` `smoke_glow_blending.mjs` `smoke_test_facade.mjs` `smoke_unified_wall_tool.mjs` `docs/UX-MODES.md` `docs/reviews/CODE-REVIEW-195-r1.md` `src/styles/chrome.styles.ts` | | #646 | [CODE-REVIEW-646-r1.md](CODE-REVIEW-646-r1.md) | code · r1 | 🟢 зелёный | 0 | 0 | — | — | | #645 | [SPEC-REVIEW-645-r1.md](SPEC-REVIEW-645-r1.md) | spec · r1 | 🟡 жёлтый | 0 | 2 | Отсутствует обязательная строка Touch editor: … из docs/TOUCH-SUPPORT.md → «Documentati…; ТЗ не учитывает существующую зависимость live-подписей Resize от позиции кнопки «Настро…; / Low | `docs/TOUCH-SUPPORT.md` `docs/reviews/SPEC-REVIEW-449-r1.md` `docs/specs/359-furniture-placement-preview.md` `docs/specs/449-double-fit-all.md` `src/houseplan-editor-runtime.ts` `src/houseplan-card.ts` `src/resize-labels.ts` `docs/process/REVIEWER.md` | | #645 | [SPEC-REVIEW-645-r2.md](SPEC-REVIEW-645-r2.md) | spec · r2 | 🟢 зелёный | 0 | 0 | — | — | diff --git a/docs/reviews/SPEC-REVIEW-647-r1.md b/docs/reviews/SPEC-REVIEW-647-r1.md new file mode 100644 index 00000000..5dc07d32 --- /dev/null +++ b/docs/reviews/SPEC-REVIEW-647-r1.md @@ -0,0 +1,254 @@ +# SPEC-REVIEW-647-r1 + +- **Issue:** https://github.com/Matysh/houseplan-card/issues/647 +- **Этап:** `S4-spec-review` (ревью ТЗ, PROCESS.md §2.4) +- **Трек:** `small` (лёгкий), по аналитике автора в комментарии + `#issuecomment-5828900656` +- **Материал:** тело issue #647, раздел `## ТЗ` (снимок на момент ревью, + 2026-09-25); комментариев — один («Аналитика») +- **Заход:** r1 · блокирующих циклов израсходовано 0 из 2 (лёгкий трек) +- **Роль:** ревьюер ТЗ (не автор) + +## Скоуп ревью + +ТЗ описывает две связанные правки шапки основной панели: (1) убрать +текстовый счётчик устройств «N устр.» и (2) вынести крестик закрытия +редактора из активной кнопки режима в отдельный слот фиксированной ширины +на месте бывшего счётчика, чтобы вход/выход из редактора не менял ширину +шапки. Проверялось: обязательные разделы §7.1, однозначность и +проверяемость каждого AC, соответствие описания коду «как есть», согласие +со SCOPE.md и с каноническим документом подсистемы (`docs/UX-MODES.md`), +отсутствие домыслов, выданных за факт. + +## Как проверялось + +1. Прочитан `docs/SCOPE.md` — задача обслуживает J6 (поддержание + актуальности/качества интерфейса) как polish-правку самого механизма + переключения режимов из J1/J6; прямой ссылки-строки на «полировку шапки» + в Core user jobs нет, но это ожидаемо для `bug`/`polish` в лёгком треке — + правка не добавляет функциональность, а чинит визуальный дефект уже + принятого механизма (UX-MODES.md). Внутри скоупа. +2. Прочитан `docs/process/REVIEWER.md` и разделы PROCESS.md §2.4, §2.5, + §7.1, §4, §7.2. +3. Прочитан весь текст issue #647 и комментарий «Аналитика». +4. Прочитан `docs/USER-GUIDE.ru.md` — упоминаний счётчика устройств в + шапке или крестика активного редактора с привязкой к тексту нет, + условие ТЗ «если упомянут — убрать» корректно не требует правки этого + документа. +5. Прочитан `docs/UX-MODES.md` целиком (канонический документ подсистемы, + явно указан в материале для этой задачи) — найдены два предложения, + которые ТЗ #647 делает неточными, но не планирует обновлять + (находка ниже). +6. Прочитан код, который ТЗ описывает как причину бага: + `src/houseplan-card.ts:10797-10820` (кнопки `.modetab`, крестик + `.closex` внутри активной кнопки, `` после `.modes` + как отдельный сиблинг) — описание совпадает с кодом дословно. +7. Проверено число использований ключа `count.devices`: + `grep -rn "count.devices" src/` — единственное место рендера + (`src/houseplan-card.ts:10820`) плюс переводы в четырёх словарях; + сводная панель #437 использует другой источник, ключ `count.devices` + ею не читается. Утверждение ТЗ «ключ используется только счётчиком» + подтверждено, а не принято на слово. +8. Прочитан `src/styles/chrome.styles.ts:123-172` (`.modes`, `.modetab`, + `.modetab .closex`) и `src/styles/dialogs.styles.ts:6-37` (`.head`, + `.count`) — обе таблицы стилей входят в единый `cardStyles` + (`src/styles.ts:17-19`), то есть правило `.head .count` из + `dialogs.styles.ts` действует и на шапку основной панели, не только на + диалоги (имя файла вводит в заблуждение, но код общий с #266). +9. Прочитаны `docs/reviews/CODE-REVIEW-195-r1.md` и `r2.md` — предыдущая + правка того же крестика (issue #195, «крестик не закрывает редактор с + первого клика») зафиксировала контракт «hit-target ≥24×24 px» именно + потому, что промах 2–3 px превращался в документированный no-op. + Контракт записан в `docs/UX-MODES.md:41-44`. +10. Проверено существование технических опор плана автотестов: + `test/core-file-budget.test.mjs`, `scripts/smoke-links.mjs`, + `scripts/mutation-registry.mjs`, `scripts/smoke-select.mjs`, + `demo/helpers/hp-test.mjs` (там же — фасад `modeTab: + '[data-hp="mode-tab"]...'`, ссылка на #629) — все существуют, ссылки + точны. + +Гейты не гонялись: этап `spec`, кода ещё нет (ветка `issue/647-...` +упомянута в занятии, но диапазон ревью — только текст ТЗ). Это ожидаемо +для ревью ТЗ, а не пропуск. + +## Находки + +### Medium-1 (в скоупе). П.4 ТЗ переносит на новый слот прежнее +адаптивное поведение счётчика, не разбирая, что у счётчика и у крестика +разные требования к видимости — риск спрятать крестик закрытия на очень +частой ширине + +**Файл:** тело issue #647, раздел «Скоуп», п.4. + +**Формулировка ТЗ:** «Узкая ширина: существующие медиа-правила и +перенос/скролл шапки сохраняются; слот участвует в них так же, как прежде +счётчик (не создаёт нового горизонтального overflow)». + +**Что не так.** Существующее правило, которое здесь имеется в виду, +конкретно: `src/styles/dialogs.styles.ts:27-29` + +```css +@media (max-width: 1100px) { + .head .count { display: none; } +} +``` + +Оно входит в общий `cardStyles` (`src/styles.ts:17-19`) и действует на +шапку основной панели, а не только на диалоги. `max-width: 1100px` — +это не редкий узкий кейс: большинство собственных смоков этого репозитория +запускают страницу именно на ширине 1100 px (`demo/smoke_device_preview_ +parity.mjs:5`, `smoke_glow_blending.mjs:8`, `smoke_test_facade.mjs:10`, +`smoke_unified_wall_tool.mjs:4` и другие) — то есть это обычная, а не +крайняя ширина браузера/окна. + +Счётчик было не жалко скрывать на такой ширине: это декоративная цифра. +Крестик — не декоративная: это единственный способ выйти из редактора +через шапку (`docs/UX-MODES.md:29-30`: «re-clicking the active tab does +nothing»). Если реализация буквально выполнит п.4 — например, оставит +слоту класс `.count` или скопирует то же медиа-правило — крестик исчезнет +из шапки при ширине окна ≤1100 px, то есть в самой обычной обстановке, а +не только на «узкой» ширине, которую описывает риск-раздел ТЗ. + +Ни один AC этого не ловит: AC5/К5 проверяет только `scrollWidth <= +clientWidth` на 390 и 768 px (overflow), а не видимость крестика; AC4/К4 +проверяет, что крестик «виден и срабатывает только в редакторе», но не +указывает диапазон ширин, на которых это должно быть верно, и не +исключает диапазон 721–1100 px. Формально реализация, скрывающая крестик +на 900 px ширины, может пройти все перечисленные AC. + +Смягчающее обстоятельство: у каждого редактора есть свой независимый +крестик в его нижней панели инструментов (`.btn.barclose`, +`src/houseplan-editor-runtime.ts:5141,11176,11223`) — то есть выйти из +редактора можно и без крестика в шапке. Поэтому это не полный отказ +функции, а тихий регресс одного из двух документированных способов +закрытия редактора, воспроизводимый на самой обычной ширине окна. + +**Как закрыть в ТЗ.** Явно указать, что новый слот **не** наследует +скрытие `.head .count` (например: слот получает собственное имя класса +или явное `display: <контейнер> !important` вне медиа-правила), и +добавить к AC5/К5 проверку «крестик виден и кликабелен» хотя бы на +ширинах из существующего диапазона теста (390/768 px) плюс на ширине +900–1100 px, раз именно там расположено унаследованное правило. + +### Medium-2 (в скоупе). ТЗ не упоминает и не защищает документированный +контракт «hit-target крестика ≥24×24 px» (docs/UX-MODES.md:41-44, +issue #195); канонический документ подсистемы не запланирован к правке + +**Файл:** `docs/UX-MODES.md:41-44`; тело issue #647, разделы «Контракт +поведения», «AC · чем доказан», «Release-артефакты». + +**Что не так.** Действующий контракт зафиксирован дословно: + +> The header X keeps its compact 13 px glyph but owns a hit target of at +> least 24 × 24 px without changing the tab's layout footprint. + +Это не декоративная деталь, а результат реального разбора бага (issue +#195, `docs/reviews/CODE-REVIEW-195-r1.md`): промах 2–3 px по глифу 13×13 +делал клик по крестику no-op. Фикс расширил кликабельную зону крестика до +24×24 (`src/styles/chrome.styles.ts:154-165`, приём с отрицательными +полями `margin: -5.5px -5.5px -5.5px -3.5px`, чтобы визуальный футпринт +кнопки-режима не рос). + +ТЗ #647 переносит крестик из кнопки режима в новый, независимый слот, но: +- ни в «Контракте поведения» (К1–К5), ни в таблице AC нет строки, + требующей сохранить минимальный hit-target ≥24×24 px в новом слоте — + AC4/К4 проверяет только факт клика и доступность фокуса/aria, но не + размер кликабельной области; +- «принято предположительно» фиксирует только то, что ширина слота — «одна + CSS-переменная, равная размеру кнопки закрытия», не называя это значение + и не связывая его с существующим контрактом ≥24×24 px; +- «Release-артефакты» планирует правку `USER-GUIDE.ru/en` (условно — там + и так ничего нет, см. «как проверялось» п.4) и golden, но не называет + `docs/UX-MODES.md`, хотя это явно указанный в материале канонический + документ подсистемы, и он станет фактически неточным сразу в двух + местах: строка 13 («space tabs, editor navigation, **device count**, + zoom and actions remain» — счётчика больше не будет) и строки 41-44 + (описывают крестик как часть футпринта кнопки-режима, чего после этой + правки уже не будет). + +Отдельно: комментарий «Аналитика» обосновывает лёгкий трек в том числе +тезисом «touch-контракт не меняется (кнопки, их размер и hit area +прежние...)». Это утверждение не подкреплено ни ТЗ, ни кодом — hit area +крестика как раз перестаёт быть тем, чем была (переезжает из кнопки +режима в новый слот с неназванным размером), и это именно то, что +описывает §7.1 как догадку, записанную как факт. + +**Как закрыть в ТЗ.** Добавить в «Контракт поведения» пункт: минимальная +кликабельная область крестика в новом слоте ≥24×24 px (или явно +задокументировать другое решение и обновить `docs/UX-MODES.md` этим же +изменением, раз ТЗ меняет описанную там архитектуру), добавить это в +таблицу AC с проверкой размера (`getBoundingClientRect` кликабельного +элемента, а не только факт клика), и включить `docs/UX-MODES.md` в +«Release-артефакты» рядом с USER-GUIDE. + +## Что проверено и корректно + +- Обязательные разделы §7.1 присутствуют все: сценарий, «что человек + увидит до/после», проблема, скоуп/не-скоуп, контракт поведения, UX/i18n/ + модель данных, таблица AC с доказательством, план автотестов, риски, + откат, release-артефакты. +- Описание бага «по коду» и границы правки (счётчик — отдельный элемент + после `.modes`, крестик — внутри активной кнопки режима) точно совпадают + с `src/houseplan-card.ts:10797-10820`. +- Удаление ключа `count.devices` безопасно: подтверждено, что у него ровно + одно место чтения в коде (сводная панель #437 использует другой + источник данных, её AC6 в таблице корректно требует не трогать эту + логику). +- AC1–AC3, AC6 однозначны, привязаны к конкретному способу доказательства + (смок с именем файла, конкретный DOM-путь/метрика) и называют мутацию, + на которой тест должен покраснеть — колонка «чем краснеет» не пустая ни + в одной строке таблицы, кроме AC5, где отсутствие мутанта прямо + объяснено как регресс-проверка (приемлемо для не-защитного AC об + overflow). +- План автотестов ссылается на реально существующие механизмы: + `test/core-file-budget.test.mjs`, `scripts/smoke-links.mjs`, + `scripts/mutation-registry.mjs`, `scripts/smoke-select.mjs`, фасад + `data-hp="mode-tab"` в `demo/helpers/hp-test.mjs` (#629) — ссылки не + выдуманы, соответствуют текущему коду. +- Не-скоуп сформулирован явно и без пересечения с #437. +- Риски раздела учитывают golden-регресс (шапка почти во всех кадрах) и + необходимость обновить смоки, которые ищут крестик внутри кнопки + режима — это отдельно от находок выше и закрыто корректно. +- `User-Visible: yes` и требование обоих changelog в одном коммите + указаны верно. +- Открытых продуктовых вопросов владельцу нет и не требовалось: поведение + полностью описано в теле issue, включая явное указание места крестика. + +## Чего не проверял + +- Гейты (`tsc`, `npm test`, `npm run build`, `check-docs.mjs`, смоки, + golden, инварианты) — не прогонял: этап `spec`, продуктового кода к этой + правке ещё нет, диапазон ревью — текст ТЗ в теле issue, а не диапазон + коммитов. Это будет предметом код-ревью после реализации. +- Не проверял визуально в браузере текущую ширину/расположение крестика и + счётчика (ревью ТЗ не предполагает ручного тестирования UI) — вывод + о правиле `.head .count` и ширине `1100px` сделан чтением CSS и состава + `cardStyles`, а не измерением в реальном рендере; при код-ревью это + стоит перепроверить смоком на ширинах 900–1100 px, если автор примет + находку Medium-1. +- Не оценивал производительность/аниматику перехода между режимами — ТЗ + не меняет этот механизм, вне скоупа задачи. + +## Вердикт + +Найдено 0 High и 2 Medium в скоупе задачи. Оба Medium закрываются +правкой текста ТЗ (не кода): явно оговорить, что новый слот не наследует +скрытие бывшего счётчика на обычных, не только «узких», ширинах, и +явно защитить документированный контракт минимального hit-target +крестика (≥24×24 px, `docs/UX-MODES.md:41-44`), включая обновление самого +`docs/UX-MODES.md` в Release-артефактах. Без High это жёлтый вердикт — +ТЗ возвращается автору на доработку текста, отдельные issue не заводятся. + +--- + + + +## Материал раунда + +- Ветка: `dev`, коммит `b4d6effbca80` — ребейз его осиротит, и это нормально: ниже якоря, которые ребейз не меняет. +- Дерево материала: `973c05543e3f539e1a4cdf476e8cd13e42e86413` + ``` + git log --all --format='%H %T' | grep 973c05543e3f + ``` +- Тело issue: `3fcd14cbc60c13c45ead7c441a8eb79b1b203acb2be8fd56fa40f5f253c523af` +- Вердикт конвейера: `yellow` · High 0