From c3278ddd07727e407b265a9c925a84c0b04e5172 Mon Sep 17 00:00:00 2001 From: "claude[bot]" <209825114+claude[bot]@users.noreply.github.com> Date: Thu, 20 Aug 2026 20:17:57 +0000 Subject: [PATCH] docs: review document for #220 Issue: #220 User-Visible: no --- docs/reviews/SPEC-REVIEW-220-r1.md | 213 +++++++++++++++++++++++++++++ 1 file changed, 213 insertions(+) create mode 100644 docs/reviews/SPEC-REVIEW-220-r1.md diff --git a/docs/reviews/SPEC-REVIEW-220-r1.md b/docs/reviews/SPEC-REVIEW-220-r1.md new file mode 100644 index 00000000..26a71130 --- /dev/null +++ b/docs/reviews/SPEC-REVIEW-220-r1.md @@ -0,0 +1,213 @@ +# SPEC-REVIEW-220-r1 + +- **Issue:** https://github.com/Matysh/houseplan-card/issues/220 +- **ТЗ:** `docs/specs/220-space-tab-reorder.md` (коммит `0fd2331`) +- **Ревьюер:** Claude (роль «ревьюер ТЗ», PROCESS.md §2.4) +- **Цикл:** r1/2 (обычный трек — лимит 4, но фиксирую фактический номер цикла) +- **Вердикт:** жёлтый · High: 0 · Medium: 2 (обе в скоупе задачи) + +## Скоуп ревью + +Полный разбор — первый цикл. Проверено: + +1. `docs/SCOPE.md` — соответствие job J6, персона «администратор плана». +2. `AGENTS.md`, `PROCESS.md` §2.4, §2.5, §7.1 — обязательные разделы ТЗ, + критерии DoR, формат вердикта. +3. Тело issue #220 и оба комментария (аналитика → `S3-spec`; хендофф ТЗ → + `S4-spec-review`). +4. `docs/USER-GUIDE.ru.md` — терминология «пространство», «вкладка». +5. `docs/TOUCH-SUPPORT.md` — канонический документ для editor-only фичи с + явным touch-решением владельца. +6. `docs/CONFIG-COMPATIBILITY.md` — паттерн описания новых/условных полей. +7. Код на `dev` (`src/houseplan-card.ts`, `src/logic.ts`, `src/devices.ts`) — + сверка всех фактических утверждений ТЗ (номера строк, сигнатуры, + поведение). + +## Как проверялось + +- Построчное чтение ТЗ (`docs/specs/220-space-tab-reorder.md`, 236 строк) + против требуемых разделов §7.1 — все присутствуют: сценарий, что человек + увидит до/после, подтверждённая причина, скоуп/не-скоуп, контракт поведения, + данные/i18n/a11y/security, performance, риски, AC1…AC8 с доказательством, + план автотестов, мутационный гейт, release-артефакты, откат, блок + принятых предположений. +- Каждая техническая ссылка на код (`houseplan-card.ts:15717/6692/3350/4273/ + 1165`, `devices.ts:1058/1063/1249`, `logic.ts:1870-1879`) сверена с текущим + `dev` через `grep`/чтение файлов — расхождение только в точных номерах строк + (код на `dev` сдвинулся на десятки строк с момента анализа), сама логика, + сигнатуры и поведение подтверждены дословно: + - `navigationSpaces = fixed.kind === 'valid' ? [space] : model` (`:15673`, + рендер вкладок `:15746`) — подтверждает побочный эффект: при активном + `floor` (#210) в панели всегда ровно один таб, значит правило AC5 «одно + пространство — обработчики не навешиваются» само по себе накрывает и этот + случай, отдельной ветки в контракте не требуется; + - `firstSpaceId: this._model[0]?.id || ''` встречается дважды + (`houseplan-card.ts:3350`, `:4273`), потребляется в `devices.ts` в + `resolveExplicitMarkerPlacement` и в ветке virtual-маркера — подтверждён + риск, ради которого написан §8.3/AC3; + - `swipeTarget(dx, dy, zoom, spaceIds, current, minPx)` (`logic.ts:1870`) + принимает `spaceIds` параметром, вызывающий код передаёт + `this._model.map((m) => m.id)` (`houseplan-card.ts:5719`) — заявление §8.4 + «изменений не требует» подтверждено буквально; + - `_fixedFloorState` (`:1165`) действительно имеет ветвь + `out-of-range-index` — таблица в §«подтверждённая причина» точна; + - `_canEdit` (`:845`, серверный `can_write`/`is_admin`) и `_mode` (тип + `'view'|'plan'|'devices'|'decor'`, `mode-transition.ts:1`) — разные + независимые гварды, оба перечислены в §8.1 корректно и раздельно; + - `_writeConfig`/`expected_rev` (`:6702`) — существующий механизм, на + который опирается AC6, подтверждён. + - Итог: ни одного факта, выданного за решение без опоры на код или на + явный owner-decision, не найдено. Все «подтверждённые причины» в ТЗ + подтверждаются. +- `docs/TOUCH-SUPPORT.md` прочитан целиком — раздел «Documentation rule» + (строки 153–165) обязывает каждую спецификацию новой editor-фичи явно + указать один из трёх ярлыков: `Touch editor: supported` / + `best effort / intentionally degraded` / `not exposed`. В ТЗ #220 такой + строки нет (см. находку M1). +- `docs/CONFIG-COMPATIBILITY.md` прочитан как образец того, как описываются + новые/условные поля конфигурации, для сверки с находкой M2. +- AC1…AC8 проверены на однозначность и наличие способа доказательства — + все восемь пронумерованы и каждый называет `unit`/`smoke`/`diff` (см. + ниже раздел «AC»). +- Блок §17 «Принятые предположения» проверен на то, что в нём действительно + лежат только технические решения (не продуктовые) — да, все четыре пункта + такие; продуктовые вопросы (тач, права, клавиатура) закрыты явными + owner-decision в §4, что подтверждается вторым комментарием issue. + +## Находки + +### M1 — Medium, в скоупе. Отсутствует обязательная touch-классификация + +`docs/TOUCH-SUPPORT.md` («Documentation rule», строки 153–165) требует, чтобы +спецификация новой editor-фичи явно называла один из трёх статусов: +`Touch editor: supported` / `best effort / intentionally degraded` / +`not exposed`. Задача #220 добавляет новую editor-фичу (перетаскивание +вкладок), которая по прямому решению владельца (§4.1 ТЗ) на сенсорных +экранах не работает вовсе — это классический случай `not exposed`. ТЗ нигде +не содержит этой строки (проверено `grep -n -i "touch editor" +docs/specs/220-space-tab-reorder.md` → пусто). + +**Почему это не формальность.** Правило существует специально для того, чтобы +touch-ограничение было явным, а не выведенным читателем ТЗ из контекста: §103 +того же документа («Deliberate degradation rule») требует пяти условий, +включая «touch limitation is deliberate, described in the user-facing +limitations» — описание в USER-GUIDE запланировано (§15 ТЗ), но сама +классификация как акт признания решения — нет. + +**Чем закрывается:** одна строка вида `Touch editor: not exposed — +перетаскивание вкладок работает только мышью в редакторах (§4.1, §7)` в ТЗ, +рядом с §4 или §9. Не требует новых AC и не меняет скоуп. + +### M2 — Medium, в скоупе. §9 утверждает «нет нового поля», хотя §8.3/§17.3 предполагают обратное + +§9 («Данные») формулирует как решённый факт: *«только порядок элементов +массива `spaces`. Ни одно поле не добавляется и не удаляется; миграции и +compatibility-полей нет»*. + +Это противоречит собственному §8.3/§17.3 того же документа. Причина +противоречия не косметическая, а структурная: сегодня «первое пространство» +физически **не имеет** другого представления, кроме позиции 0 в массиве — +отдельного поля-якоря или временной метки создания не существует +(подтверждено чтением `devices.ts` и `houseplan-card.ts`: `firstSpaceId` +везде вычисляется как `_model[0]?.id`). Значит, чтобы якорь **пережил +перезагрузку страницы** — а он обязан её пережить, иначе после reload +`_model[0]` снова укажет на новый первый элемент (тот, что оказался на месте +0 после перестановки), и AC3 перестанет выполняться при следующей сессии — +единственный способ сохранить инвариант «маркер не меняет `space`» состоит в +том, чтобы **где-то записать исходный id постоянно**. Собственный блок +предположений автора (§17.3) прямо это и предлагает: «хранить якорь в +`settings` при первой записи конфигурации». + +То есть §9 отвечает «нет» на вопрос, который DoR (`PROCESS.md` §2.5: «миграция +и compatibility-поля решены по `docs/CONFIG-COMPATIBILITY.md`») требует +решить, а не задекларировать закрытым текстом, который тут же опровергается +соседним разделом. Технический выбор (само устройство поля) законно оставлен +автору — но факт «поле, скорее всего, появится» должен быть проговорён в §9, +а не спрятан только в необязательном для чтения блоке предположений. + +**Последствие, если не поправить.** Если реализация действительно добавит +поле в `settings` (наиболее вероятный путь по собственному §17.3), +`docs/CONFIG-COMPATIBILITY.md` не получит записи о новом поле, хотя документ +существует ровно для таких случаев (сравните с трактовкой `known_devices`, +`custom_fill`, `bg_mode` — все новые поля `settings` в этом файле +задокументированы отдельным разделом с историей совместимости). + +**Чем закрывается:** переформулировать §9 в духе «если якорь потребует нового +поля в `settings` — оно аддитивно и forward-compatible: старые клиенты его +игнорируют, при отсутствии поля fallback остаётся прежним (`_model[0]`)», и +добавить короткую запись в `docs/CONFIG-COMPATIBILITY.md` в том же коммите, +где появится реализация (не обязательно сейчас, но контракт должен быть +проговорён в ТЗ, а не отрицаться). + +### Low — не является блокирующим, не требует правки + +- Гвард начала drag (`pointerdown` по `.tab`, порог 4px) не оговаривает + явно элемент `.tabedit` (шестерёнка настройки пространства) внутри вкладки: + текущий обработчик шестерёнки — `@click` с `e.stopPropagation()` + (`houseplan-card.ts:15754`), но `pointerdown`, скорее всего, будет повешен + на `.tab`, а не на кнопку, и `stopPropagation` в `@click` не остановит + всплытие `pointerdown`. Реализация почти наверняка и так исключит `.tabedit` + из зоны старта drag (иначе шестерёнка перестанет открывать диалог при + случайном микросдвиге курсора) — это тривиальная деталь без продуктовой + неоднозначности и без риска потери данных. Снимаю без правки: чисто + реализационная деталь, накрываемая кодревью. + +## AC — проверка на однозначность и доказуемость + +| AC | Однозначен | Способ доказательства указан | Комментарий | +|---|---|---|---| +| AC1 | да | `smoke` | конкретное поведение: позиция + переживает reload | +| AC2 | да | `smoke` | порог 4px, оба исхода (клик/drag) проверяемы | +| AC3 | да | `unit` (`buildDevices`) | явно требует «тест красный до развязки» — хорошая практика | +| AC4 | да | `unit`+`smoke` | подтверждено чтением кода: `swipeTarget` не требует правок | +| AC5 | да | `unit`+`smoke` | четыре условия исключения перечислены явно и проверяемо | +| AC6 | да | `unit`/`smoke` | опирается на существующий механизм `expected_rev` | +| AC7 | да | `smoke` | «один раз за сессию» — паттерн уже есть в коде (`_onboardingShown`), не изобретён | +| AC8 | да | `diff` + сверка бандлов | стандартный release-гейт | + +Ни один AC не сформулирован расплывчато, у каждого назван корректный способ +доказательства применительно к тому, что он утверждает. + +## Что проверено и корректно + +- Персона и job (`docs/SCOPE.md` J6) подобраны верно, сценарий и «что человек + увидит» — в пользовательских терминах, без реализации. +- Продуктовые решения (§4) действительно закрывают все три продуктовых + вопроса из аналитики (тач/права/клавиатура) явной ссылкой на owner-decision + 2026-08-20, а не на догадку исполнителя. +- Все три места, где порядок массива несёт скрытый смысл (`firstSpaceId`, + `swipeTarget`, числовой `floor`), названы, подтверждены по коду и закрыты + разделами контракта (§8.3, §8.4, §8.5) — это тот самый риск, ради которого + задача вообще была оценена в сложность 4/10, а не 1/10. +- Технические предположения (§17) действительно технические — ни один пункт + не требует продуктового решения владельца, что соответствует правилу + «размытое место не додумывается, а выносится владельцу» из PROCESS.md §7.1 + (здесь размытых продуктовых мест не осталось). +- Мутационный гейт целится в самые опасные регрессии (потеря записи, съеденный + клик, возврат порядко-зависимого `firstSpaceId`, снятие проверки + `pointerType`) — это именно те четыре мутанта, которые действительно ловят + типовые ошибки такой фичи. +- Release-артефакты (§15) и откат (§16) присутствуют и покрывают оба + changelog, оба USER-GUIDE, отсутствие влияния на golden/perf. + +## Чего не проверял + +- Не проверял реализацию — её ещё нет (это ревью ТЗ, код не существует). +- Не запускал никаких гейтов/тестов: на этапе ревью ТЗ гейты (`typecheck`, + `npm test`, `npm run build`) неприменимы — класса A/B изменений нет, только + документация (класс C). +- Не проверял `docs/specs/README.md` на полноту сверх наличия строки про + #220 — строка присутствует, этого достаточно для этого этапа. +- Не оценивал абсолютную величину порога 4px и механику «вставка перед» — + оба явно помечены автором как «принято предположительно, поменять + свободно» (§17.1–17.2), оспаривать нечего: реализация вправе выбрать другое + значение без нового цикла ревью. + +## Итог + +Два Medium-замечания, обе в скоупе задачи, обе устраняются точечной правкой +текста ТЗ (без изменения AC, скоупа или контракта поведения). High-находок +нет. Вердикт — жёлтый: автор правит §4/§9 ТЗ (добавляет touch-классификацию и +снимает противоречие в разделе «Данные»), правка проходит второй цикл ревью +ТЗ. Задача не `small`, лимит циклов ревью ТЗ — 4 (`PROCESS.md` §4); этот +раунд — первый из них.