From 90a00670a79d492827e18738ad5b0ec32e47ffc2 Mon Sep 17 00:00:00 2001 From: "claude[bot]" <209825114+claude[bot]@users.noreply.github.com> Date: Thu, 20 Aug 2026 21:41:11 +0000 Subject: [PATCH] docs: review document for #220 Issue: #220 User-Visible: no --- docs/reviews/CODE-REVIEW-220-r1.md | 277 +++++++++++++++++++++++++++++ 1 file changed, 277 insertions(+) create mode 100644 docs/reviews/CODE-REVIEW-220-r1.md diff --git a/docs/reviews/CODE-REVIEW-220-r1.md b/docs/reviews/CODE-REVIEW-220-r1.md new file mode 100644 index 00000000..93403f66 --- /dev/null +++ b/docs/reviews/CODE-REVIEW-220-r1.md @@ -0,0 +1,277 @@ +# Код-ревью issue #220 — порядок вкладок пространств перетаскиванием + +- **Issue:** https://github.com/Matysh/houseplan-card/issues/220 +- **ТЗ:** `docs/specs/220-space-tab-reorder.md`, зелёное ревью r3 + (`docs/reviews/SPEC-REVIEW-220-r3.md`), заход исчерпан на 2/4. +- **Заход код-ревью:** r1/4 (первый цикл код-ревью; счётчик отдельный от + ревью ТЗ, §10.4 PROCESS.md). +- **Диапазон:** ветка `issue/220-space-tab-reorder`, коммит `8369c0e` + (единственный коммит реализации поверх `e57e1d9`). +- **Ревьюер:** свежая сессия, без контекста реализации. + +## 1. Скоуп проверки + +Диапазон — весь коммит `8369c0e` (первый код-ревью для этой задачи, полный +разбор, §2.10 не применяется): `src/space-order.ts` (новый), правки +`src/houseplan-card.ts` (обработчики drag, `_commitTabOrder`), `src/styles.ts`, +i18n en/ru, `scripts/mutation-gate.mjs`, `test/space-order.test.mjs`, +`demo/smoke_space_tab_reorder.mjs`, оба changelog, оба USER-GUIDE, три копии +бандла. + +## 2. Как проверялось + +| Гейт | Команда | Результат | +|---|---|---| +| Typecheck | `npx tsc --noEmit` | чисто | +| Unit | `npm test` | 982/982, 0 fail | +| Build + сверка копий | `npm run build && cmp dist/houseplan-card.js custom_components/houseplan/frontend/houseplan-card.js && cmp dist/houseplan-card.js demo/srv/assets/houseplan-card.js` | пересобран, все три копии побайтово совпадают, `git status` после сборки чист | +| Смок AC1/2/4/5/7 | `node demo/smoke_space_tab_reorder.mjs` | OK, все 18 проверок зелёные | +| Смок соседнего контракта (#210) | `node demo/smoke_fixed_floor.mjs` | OK | +| Смок соседнего контракта (размещение маркеров) | `node demo/smoke_subarea.mjs` | OK | +| Mutation-gate, дешёвая проверка якорей | `node scripts/mutation-gate.mjs --check` | все патчи, включая 4 новых, находят свой якорь в исходниках | +| **Прямой вызов production-функций** (не гейт, целевая проверка находки H1) | см. §4.1 | воспроизводит расхождение | + +**Не прогонялось и почему:** `golden:verify` — задача не трогает визуал в +состоянии покоя (панель вкладок вне матрицы, §15 ТЗ), diff подтверждает: правки +только в `.tab`/`.dragging`/`.droptarget` стилях активного взаимодействия; +`pytest tests_backend` — ни один файл `custom_components/**/*.py` не тронут; +полный (дорогой) прогон `mutation-gate` с пересборкой на каждого мутанта — это +предрелizный гейт, не гейт ревью (§8 PROCESS.md); остальные 124 браузерных +смока — diff не задевает их поверхности (canvas-инструменты, устройства, +партиции и т.д.), только панель вкладок и её ближайших соседей, которые +прогнаны выше. + +## 3. Находки + +### H1 (High) — материализация неверно определяет «зависимые от порядка» маркеры: обычный маркер устройства с площадью из реестра HA получает чужой `space` + +**Что не так.** `markersNeedingPlacement` (`src/space-order.ts:101-119`) решает, +какому маркеру нужно материализовать `firstSpaceId`, глядя только на +персистентные поля самого маркера — `marker.area` и `marker.space`: + +```ts +const area = typeof marker.area === 'string' ? marker.area : ''; +if (area && areaToSpace[area]) continue; +out.push({ id, space: firstSpaceId }); +``` + +Но реальное размещение маркера, привязанного к HA-устройству или сущности +(`binding: 'device:...'` / `'entity:...'`), решает не только `marker.area` — +`resolveExplicitMarkerPlacement` (`src/devices.ts:1045-1065`) в первую очередь +берёт **область из реестра HA** (`dev?.area_id` / `reg?.area_id`), и только +если её нет — откатывается к `marker.area`, `marker.space`, затем +`firstSpaceId`: + +```ts +const area = marker.area || registryArea || ''; +return { area, space: (area && areaToSpace[area]) || marker.space || firstSpaceId }; +``` + +**Ровно так и создаются обычные маркеры.** `_markerDraft` +(`houseplan-card.ts:18868-18913`) и её аналог для существующей формы +(`houseplan-card.ts:13305-13344`) пишут `marker.area`/`marker.space` **только +если** `d.binding === 'virtual' || d.room` — то есть только для виртуальных +маркеров или маркеров, которым вручную выбрана комната. Любой маркер, +созданный обычным способом «привязать существующее HA-устройство» (переименовать, +сменить иконку, добавить контролы, спрятать с плана) без ручного выбора +комнаты — **самый частый путь в реальной инсталляции** (SCOPE.md: 20–200 +устройств, в основном автообнаруженных) — сохраняется **без** `area` и без +`space` вовсе. Такой маркер сегодня прекрасно резолвится через область своего +HA-устройства и от порядка `spaces` не зависит. + +`markersNeedingPlacement` не видит `registryArea` — у неё в сигнатуре нет ни +самого устройства, ни его области, только поля маркера. Поэтому она +классифицирует **любой такой маркер** как «зависящий от `firstSpaceId`» и +`_commitTabOrder` (`houseplan-card.ts:1300-1330`) прямо в транзакции +перестановки дописывает ему `marker.space = <старое первое пространство>` — +хотя AC3/§8.3 ТЗ прямо требуют, чтобы такие маркеры «остались побитово +прежними». + +**Воспроизведение** (прямой вызов той же production-сборки, без браузера): + +```js +import { buildDevices } from './test-build/devices.js'; +import { markersNeedingPlacement } from './test-build/space-order.js'; + +const hass = { devices: { boiler: { + id: 'boiler', name: 'Boiler', model: 'X', area_id: 'attic', + identifiers: [['demo', 'boiler']], entry_type: null, via_device_id: null, +} }, entities: {}, states: {}, areas: {} }; +const marker = { id: 'boiler-marker', binding: 'device:boiler' }; // ни area, ни space +const ctx = { hass, areaToSpace: { attic: 'f2' }, markers: [marker], settings: {}, + excluded: new Set(), showAll: false, firstSpaceId: 'f1', loc: () => '' }; + +buildDevices(ctx).find((d) => d.id === 'boiler-marker').space; +// → 'f2' — маркер сегодня корректно и стабильно резолвится через область HA, +// пространство 'f2' не имеет отношения к firstSpaceId + +markersNeedingPlacement([marker], { attic: 'f2' }, 'f1'); +// → [{ id: 'boiler-marker', space: 'f1' }] +// _commitTabOrder на основании этого результата запишет marker.space = 'f1' — +// пространство, в котором маркер никогда не находился. +``` + +Запущено против собранного `test-build` этой самой ветки (`npx tsc -p +tsconfig.test.json`) — расхождение воспроизводится, не гипотетическое. + +**Почему это не ловится существующими тестами.** Юнит `test/space-order.test.mjs` +(«only order-dependent markers are pinned») и смок (`smoke-dangling`, marker с +`binding: 'virtual'`) проверяют только маркеры, у которых «отсутствие +зависимости от порядка» и так видно по их собственным полям (`area: 'kitchen'`, +`space: 'f2'`). Ни один тест не заводит обычный `device:`/`entity:`-маркер без +area/space, чьё реальное размещение решает область его HA-устройства — +единственный случай, где расхождение проявляется. Мутационный гейт тоже не +покрывает эту ветку: из пяти записей §14 ТЗ реализованы четыре (см. M2 ниже), +причём именно недостающая `materialization-touches-bound-markers` +концептуально ближе всего к этой находке. + +**Практическое следствие.** Прямо сейчас маркер не переезжает визуально: в +`resolveExplicitMarkerPlacement` `area && areaToSpace[area]` стоит раньше +`marker.space` в приоритете, поэтому испорченный `marker.space` временно +маскируется правильной резолюцией через область. Но конфиг тихо получает +недостоверное персистентное поле на **каждом** обычном маркере устройства без +ручной комнаты при **каждой** перестановке вкладок: как только HA-область этого +устройства когда-либо станет недоступна (переназначение area в HA, замена +устройства, повторная привязка сущности) — `area && areaToSpace[area]` +перестанет резолвиться, и маркер молча прыгнет в то самое `firstSpaceId`, +которое было актуально на момент случайной перестановки вкладок месяцы назад, +без какой-либо связи с текущим действием пользователя. Это ровно тот +сценарий («маркеры уезжают»), который §8.3 ТЗ называет главным риском задачи — +только отложенный во времени и оттого более коварный. + +**Это находка в скоупе задачи** (§8.3/AC3 — центральный контракт этой самой +задачи), блокирует зелёный вердикт. + +### M1 (Medium, в скоупе) — перетаскивание вкладки не захватывает указатель: отпускание за пределами панели оставляет «зависшее» состояние drag + +`_tabPointerDown` (`houseplan-card.ts:1259-1272`) не вызывает +`setPointerCapture`/`capturePointer`, в отличие от **всех** остальных +pointer-based drag-жестов в этом файле — resize краёв/углов +(`_rszEdgeDown`/`_rszCornerDown`, `capturePointer(ev)`), компас +(`setPointerCapture` напрямую, `:14271`), инструменты декора (`:9598`, `:9608`, +`:14944` и другие) — все идут через явный захват указателя или через +специально написанный для этого хелпер: + +```ts +// houseplan-card.ts:667-673 +const capturePointer = (ev: PointerEvent): void => { + try { (ev.target as Element | null)?.setPointerCapture?.(ev.pointerId); } + catch { /* an inactive pointerId must never kill the drag */ } +}; +``` + +Без захвата `pointermove`/`pointerup` у `_tabPointerMove`/`_tabPointerUp` +привязаны к каждому `.tab`-элементу и получают события только пока указатель +физически находится над этим элементом. Если пользователь, естественно +двигая руку по горизонтали, слегка уводит курсор за пределы строки вкладок +(вниз, на «+», в промежуток между вкладками) и там отпускает кнопку — ни +`pointerup`, ни `pointercancel` не срабатывают ни на одном `.tab`. Результат: + +1. `_tabDrag` остаётся с `moved: true` навсегда — вкладка-источник продолжает + показывать класс `dragging` (`opacity: 0.55`), последняя наведённая — + `droptarget` (синяя рамка), пока не начнётся и не завершится следующий + полноценный drag на каком-то `.tab`. +2. `_tabClick` (`houseplan-card.ts:1290-1293`) читает `this._tabDrag?.moved`, + чтобы отличить клик от драга. Пока `_tabDrag` в таком «зависшем» виде, + следующий тап **другим** типом указателя (например, тач на гибридном + тач-экране ноутбука — актуально: `docs/TOUCH-SUPPORT.md` прямо + рассматривает такие устройства) не пройдёт через `_tabPointerDown` + (`canStartTabDrag` вернёт `false` для `pointerType !== 'mouse'`, ранний + `return`), состояние не сбросится, и `_tabClick` молча проглотит переключение + пространства. + +Спек §8.1 требует «при отпускании вне панели порядок не меняется» — это +формально верно (commit не вызывается), но не покрывает визуальный/интерактивный +артефакт, который отпускание вне панели создаёт. Смок не ловит это: `drag()` в +`demo/smoke_space_tab_reorder.mjs` всегда завершает `pointerup` на целевой +вкладке (`to`), сценарий «отпустить вне панели» не воспроизведён ни разу. + +**Почин.** Один вызов `capturePointer(event)` (используя уже существующий +хелпер `houseplan-card.ts:667`) в начале `_tabPointerDown`, как это уже сделано +для каждого другого перетаскивания в файле. + +### M2 (Medium, в скоупе) — мутационный гейт реализован не полностью относительно принятого §14 ТЗ + +Спека (`docs/specs/220-space-tab-reorder.md` §14) фиксирует пять записей +мутационного гейта, в том числе: + +``` +| materialization-touches-bound-markers | материализовать space и у маркеров с area | юнит AC3 | +``` + +В `scripts/mutation-gate.mjs` добавлены только четыре: `tab-reorder-not-persisted`, +`reorder-skips-materialization`, `tab-reorder-eats-click`, +`tab-reorder-ignores-pointer-type`. Хендофф-комментарий в issue тоже называет +«четыре записи, все прогнаны» — без пометки, что пятая из принятого ТЗ +пропущена, и без объяснения почему. Раз AC5.md §14 — часть зелёного ТЗ (не +«принятое предположение»), тихое сокращение состава гейта — отклонение от DoR, +а не право исполнителя. + +Показательно, что именно эта недостающая проверка концептуально ближе всего к +H1: она должна была утверждать «материализация не трогает маркеры с area» — +и её реализация (более широкая, чем нынешний юнит-тест, который проверяет +только буквальное поле `marker.area`) с большей вероятностью поймала бы +находку выше на этапе реализации. + +**Не отдельный issue** — это Medium в скоупе текущей задачи (§14 — её +собственный принятый план тестов), правится в этой же ветке добавлением +пятой записи мутационного гейта с юнит-тестом, покрывающим маркер, чьё +размещение решает область HA-устройства, а не собственное поле `area`. + +## 4. AC — чем доказано (сверх заявленного автором) + +| AC | Проверка ревьюера | Вывод | +|---|---|---| +| AC1 | смок прогнан повторно, зелёный; код `_commitTabOrder` + `reorderSpaceIds`/`applySpaceOrder` прочитан | доказано | +| AC2 | смок повторно; unit `passedDragThreshold`/`canStartTabDrag` прочитаны | доказано | +| AC3 | смок повторно зелёный **для сценария из смока**, но разбор по коду нашёл незакрытый случай — **H1** | **не доказано полностью**, находка блокирует | +| AC4 | `swipeTarget` берёт `this._model.map(m => m.id)` (`:5825`), `_model` пересчитывается по `_cfgEpoch`, который `_saveConfig()` инкрементирует синхронно до дебаунса записи — прочитано, логика верна | проверено чтением, корректно | +| AC5 | смок повторно; `canStartTabDrag` разобран построчно, семь юнит-веток на отрицание | доказано | +| AC6 | `_commitTabOrder` → `_saveConfig()` → `_saveConfigDebounced` → `_writeConfig()` — тот же путь конфликта, что у остальных правок (`toast.conflict`, `_reloadConfigOnly`), своего кода обработки не добавлено — прочитано | проверено чтением, не исполнением (согласен с автором) | +| AC7 | смок повторно зелёный | доказано | +| AC8 | md5 всех трёх копий бандла совпадает после независимой пересборки; changelog/USER-GUIDE поменяны в обоих языках | доказано — **с оговоркой**: формулировка «Markers stay exactly where they were» станет точной только после фикса H1 | + +## 5. Что проверено и корректно + +- Порог 4 px, разделение клика и драга, вставка перед целевой вкладкой (а не + обмен местами) — соответствует §17 ТЗ. +- `.tabadd` («+») не участвует: у неё нет `pointerdown`/`pointermove`/`pointerup` + обработчиков, поэтому она не может стать ни источником, ни целью — код + подтверждает намерение спеки без отдельного смок-кейса. +- Одно пространство — `canStartTabDrag` возвращает `false` при `spaceCount <= 1` + (юнит-тест есть). +- `_hasFixedFloor` корректно блокирует drag (панель с фиксированным этажом + показывает одну вкладку — двойная защита, не только через `spaceCount`). +- View/киоск/тач исключены на уровне чистой функции и подтверждены смоком. +- Атомарность записи (порядок + материализация одним `cfg`-мутированием перед + единственным `_saveConfig()`) соблюдена — ровно то, что требовал норматив + §17.3 ТЗ и на чём настоял ревью ТЗ r2/r3 (M3). +- Тост «once per session» — `_tabOrderWarned` не персистится, инстанс-поле, + сбрасывается только при пересоздании карточки — соответствует формулировке + АС7 «за сессию». +- Три копии бандла побайтово идентичны после независимой пересборки ревьюером. + +## 6. Чего не проверял + +- Полный (дорогой, предрелизный) прогон `mutation-gate.mjs` с реальной + пересборкой на каждый мутант — вне объёма код-ревью (§8 PROCESS.md), прогнан + только `--check`. +- `golden:verify` — визуал панели вкладок не в матрице, изменений в покое нет; + не прогонялся сознательно (см. §2). +- `pytest tests_backend` — Python не тронут. +- Остальные ~124 браузерных смока за пределами панели вкладок и её + непосредственных соседей (#210, привязка маркеров) — diff их поверхностей не + касается. +- Ручного тестирования в браузере (drag мышью вживую) не было — по регламенту + цикла (нет фазы ручного тестирования); H1 и M1 доказаны разбором по коду и + прямым вызовом продакшен-функций соответственно, а не интерактивной сессией. + +## 7. Итог + +Один High (H1) — блокирует. Два Medium (M1, M2) — в скоупе задачи, правятся в +этой же ветке. AC3 не доказан полностью: смок проверяет только тот подкласс +«зависимых от порядка» маркеров, который сам код классифицирует явными +полями, и не покрывает самый частый в реальных конфигурациях случай — +маркер устройства, чьё размещение решает область HA, а не поле маркера. + +Возврат автору: `S6-in-progress`.