mirror of
https://github.com/Matysh/houseplan-card
synced 2026-09-28 19:01:34 +00:00
claude[bot]
@@ -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`.
|
||||
Reference in New Issue
Block a user