mirror of
https://github.com/Matysh/houseplan-card
synced 2026-10-07 23:19:14 +00:00
claude[bot]
@@ -1,6 +1,6 @@
|
||||
# Индекс ревью
|
||||
|
||||
Генерируется `node scripts/reviews-index.mjs` (#635) — не редактировать руками. Документов: 349, issue: 177. Вердикт: 🟢 зелёный · 🟡 жёлтый · 🔴 красный · ⚪ не распознан (свободная форма старых документов). H/M — число High/Medium по строке вердикта или заголовкам находок. Файлы — пути, названные в находках; ищите по имени файла: `grep form-kit INDEX.md`.
|
||||
Генерируется `node scripts/reviews-index.mjs` (#635) — не редактировать руками. Документов: 350, issue: 178. Вердикт: 🟢 зелёный · 🟡 жёлтый · 🔴 красный · ⚪ не распознан (свободная форма старых документов). H/M — число High/Medium по строке вердикта или заголовкам находок. Файлы — пути, названные в находках; ищите по имени файла: `grep form-kit INDEX.md`.
|
||||
|
||||
| Issue | Документ | Этап · раунд | Вердикт | H | M | Находки | Файлы |
|
||||
|---|---|---|---|---:|---:|---|---|
|
||||
@@ -8,6 +8,7 @@
|
||||
| бета v1.80.0-beta.1 | [SHIP-REVIEW-v1.80.0-beta.1.md](SHIP-REVIEW-v1.80.0-beta.1.md) | пакетное ревью ship · — | ⚪ — | 0 | 0 | — | — |
|
||||
| бета v1.79.0-beta.2 | [SHIP-REVIEW-v1.79.0-beta.2.md](SHIP-REVIEW-v1.79.0-beta.2.md) | пакетное ревью ship · — | ⚪ — | 0 | 0 | — | — |
|
||||
| бета v1.79.0-beta.1 | [SHIP-REVIEW-v1.79.0-beta.1.md](SHIP-REVIEW-v1.79.0-beta.1.md) | пакетное ревью ship · — | ⚪ — | 0 | 0 | — | — |
|
||||
| #819 | [SPEC-REVIEW-819-r1.md](SPEC-REVIEW-819-r1.md) | spec · r1 | 🟡 жёлтый | 0 | 1 | отсутствуют обязательные разделы «риски» и «откат» (§7.1) | `docs/reviews/SPEC-REVIEW-814-r1.md` |
|
||||
| #815 | [CODE-REVIEW-815-r1.md](CODE-REVIEW-815-r1.md) | code · r1 | 🟢 зелёный | 0 | 0 | — | — |
|
||||
| #814 | [SPEC-REVIEW-814-r1.md](SPEC-REVIEW-814-r1.md) | spec · r1 | 🟢 зелёный | 0 | 0 | — | — |
|
||||
| #813 | [SPEC-REVIEW-813-r1.md](SPEC-REVIEW-813-r1.md) | spec · r1 | 🟢 зелёный | 0 | 0 | — | — |
|
||||
|
||||
@@ -0,0 +1,268 @@
|
||||
# SPEC-REVIEW-819-r1
|
||||
|
||||
Issue: #819 · этап: spec · трек: ask · заход: r1 · блокирующих циклов израсходовано (после этого раунда): 1/4
|
||||
|
||||
Материал: тело issue #819, раздел `## ТЗ`, состояние на момент комментария
|
||||
Matysh 2026-10-07T12:18:00Z («ТЗ обновлено… `blocked` снят, ТЗ — на ревью»),
|
||||
т.е. с учётом обоих комментариев владельца (оценка + вопросы; решение по
|
||||
вопросу 1, умолчания по вопросам 2–3 приняты). Кода/ветки продукта не
|
||||
существует — задача не реализована (`issue/819-*` на удалённом и локальном
|
||||
репозитории не найдена, рабочая копия на `dev 4789c3f6368d94f7621817984b2777ddc1f3aec6`).
|
||||
Проверка велась чтением текущего `dev` на этом SHA — того самого, который
|
||||
называет issue в разделе «Где сейчас».
|
||||
|
||||
## Скоуп ревью
|
||||
|
||||
Деструктивный UX-контракт для диалога настроек пространства (редактор и
|
||||
онбординг): прокрутка диалога к предупреждению о блокирующих устройствах,
|
||||
новая кнопка «Удалить пространство вместе с устройствами» и расширение
|
||||
`houseplan/space/delete` на атомарное удаление пространства вместе с
|
||||
блокирующими его маркерами (включая скрытые). Персона — администратор дома,
|
||||
поверхность — desktop и телефон, редактор и онбординг; View, домочадцы и
|
||||
киоск не затронуты (сам ТЗ верно декларирует эту границу). Работа закрывает
|
||||
J6 docs/SCOPE.md («keep the plan true as the home evolves», три редактора,
|
||||
merge/split, оптимистическая блокировка) и частично J4 (онбординг — тот же
|
||||
`_deleteSpace()` путь продублирован в `houseplan-onboarding-runtime.ts`);
|
||||
отдельной строки SCOPE не требует, конфликта со SCOPE нет. Трек `ask`
|
||||
обоснован автором (новый деструктивный UX-контракт + публичный WS-контракт)
|
||||
и соответствует критерию §5 (новый UX-ключ, несколько поверхностей).
|
||||
|
||||
## Как проверялось
|
||||
|
||||
1. **Обязательные разделы §7.1 и однозначность AC** — чтением тела issue.
|
||||
2. **Фактическая точность технических утверждений ТЗ против `dev 4789c3f6`** —
|
||||
не на слово, построчным чтением:
|
||||
- `src/houseplan-editor-runtime.ts:8272` и `src/houseplan-onboarding-runtime.ts:412`
|
||||
— оба `_deleteSpace()` подтверждены: при `dependencies.count &&
|
||||
!deletingLastSpace` код ставит `deleteBlockers` и `return`, без прокрутки —
|
||||
ровно то, что описывает «Проблема».
|
||||
- `src/editors/space-form.ts:144-147` — `callout({ kind: 'warning', role:
|
||||
'alert', ... })` действительно последний элемент `.body` (после
|
||||
`renderSunAndLight`), подтверждает «предупреждение остаётся за краем».
|
||||
- `src/editors/form-kit.ts:385` — `callout()` уже принимает `{ text, kind,
|
||||
icon, action, role, id }` — технический план («у `callout` — `id`»)
|
||||
реализуем без изменения сигнатуры хелпера.
|
||||
- `src/space-deletion.ts` (`collectSpaceMarkerDependencies`,
|
||||
`createSpaceDeletionCandidate`, `spaceDeletionMessage`) — подтверждено:
|
||||
активные маркеры считаются по `marker.space`, по `room_id` комнат
|
||||
пространства и по `layout[id].s`, `removed !== true` — скрытые маркеры
|
||||
(видимые только как `hidden:true`, без `removed:true`) действительно
|
||||
входят в `count`, как заявлено.
|
||||
- `custom_components/houseplan/websocket_api.py:1907-1990`
|
||||
(`ws_space_delete`, `_space_delete_candidate`) — подтверждено: сервер
|
||||
отвечает `space_in_use`, если зависимости есть и пространство не
|
||||
последнее; `expected_config_rev`/`expected_layout_rev` уже проверяются
|
||||
и дают `conflict` — инфраструктура для AC4 уже на месте, расширение не
|
||||
требует новой гонки-защиты с нуля.
|
||||
- `src/devices.ts:932-947` (`deletePlanMarkerRecords`) — тождество маркера
|
||||
после удаления: `{ id, binding, removed: true, hidden: true }` —
|
||||
подтверждает формулировку AC2 «в том же состоянии, в какое его переводит
|
||||
«Удалить» в диалоге устройства»; `removedPlanBindings()` действительно
|
||||
читает такие записи для списка «Доступны снова».
|
||||
- `docs/USER-GUIDE.ru.md:556-650` (§7 «Пространства» → «Настройки
|
||||
пространства») — текущее описание блокировки и поведения последнего
|
||||
пространства совпадает с кодом и с «Где сейчас» дословно; раздел,
|
||||
который должен обновить AC6, существует и назван верно. Термин
|
||||
«Доступны снова» — из §10 USER-GUIDE (строки 1140, 1150, 1268), не
|
||||
изобретён.
|
||||
- `docs/DEVICE-PRESENTATION.md` (строка S07, «все saved controls
|
||||
отфильтрованы tombstone») — важная проверка: контракт п.4 обещает, что
|
||||
удалённый маркер исчезает «из всех данных плана… controls». Код
|
||||
единичного удаления маркера (`removeMarkerControlReferences`,
|
||||
`houseplan-editor-runtime.ts:7834`) явно вычищает `controls[]` у ДРУГИХ
|
||||
маркеров, но ни `createSpaceDeletionCandidate` (TS-кандидат,
|
||||
`src/space-deletion.ts`), ни `_space_delete_candidate` (Python,
|
||||
`websocket_api.py:1843`) сегодня этого не делают. Проверил, не ломает ли
|
||||
это контракт п.4 при пакетном удалении: нет — S07 в DEVICE-PRESENTATION.md
|
||||
зафиксирован отдельным, уже протестированным defensive-path
|
||||
(«controller-role сохранён, availability дают свои diagnostics»),
|
||||
рассчитанным именно на устаревшие `controls`-ссылки на tombstone
|
||||
независимо от момента их появления. Так что даже если реализация
|
||||
перепутает порядок очистки или не продублирует
|
||||
`removeMarkerControlReferences`/`removeMarkerAreaSnapshots`/
|
||||
`unlinkMarkers` в пакетном пути, видимого дефекта не будет — это не
|
||||
довод против AC2, поэтому не поднимаю это до отдельной находки.
|
||||
3. **Соответствие свежему прецеденту того же репозитория.** Сверил структуру
|
||||
этого ТЗ с `docs/reviews/SPEC-REVIEW-814-r1.md` (зелёный, r1, тот же
|
||||
трек, тот же порядок этапа, issue принят владельцем 2026-10-07 — то есть
|
||||
актуальный образец, не архивный) и с телом issue #814: там раздел
|
||||
«Файлы, риски, откат и release-артефакты» присутствует с содержательным
|
||||
абзацем «Главные риски — …» и строкой «Откат — …». Это показывает, что
|
||||
требование §7.1 к этим двум разделам в этом репозитории не формальность и
|
||||
не самостоятельно снимается слиянием с другими разделами — недавний
|
||||
принятый документ именно их и содержит.
|
||||
4. **Открытый продуктовый вопрос.** Искал вопрос, который требовал бы решения
|
||||
персоны/видимого поведения и не был задан. Автор уже задал три
|
||||
содержательных вопроса (семантика удаления устройств, скрытые устройства,
|
||||
форма подтверждения) в верном формате («что неясно · что изменится ·
|
||||
предлагаемый вариант по умолчанию»), владелец ответил по существу —
|
||||
пересмотра не требуют. Отдельно искал смешанный вопрос, который стоило
|
||||
бы разделить (§7.1): не нашёл — все три решённых вопроса были чисто
|
||||
продуктовыми, разделять нечего.
|
||||
|
||||
## Находки
|
||||
|
||||
### Medium — отсутствуют обязательные разделы «риски» и «откат» (§7.1)
|
||||
|
||||
**Файл:** тело issue #819, раздел `## ТЗ` целиком.
|
||||
|
||||
**Воспроизведение.** Полнотекстовый поиск по телу issue («риск», «откат»)
|
||||
даёт одно совпадение — «его сбой не откатывает удаление» в абзаце про
|
||||
`houseplan/trail/delete», что говорит о транзакционной механике одного
|
||||
вызова, а не является разделом «откат». Ни главных рисков задачи, ни
|
||||
описания отката нет нигде в документе — ни отдельным заголовком, ни абзацем
|
||||
внутри «Контракта» или «Принято предположительно».
|
||||
|
||||
Обязательные разделы ТЗ по §7.1 явно перечисляют «риски» и «откат» наравне
|
||||
со сценарием, AC и release-артефактами. Это не формальность: свежий,
|
||||
принятый в этом же репозитории документ на той же стадии процесса —
|
||||
#814 (ревью `docs/reviews/SPEC-REVIEW-814-r1.md`, зелёный, r1, владелец
|
||||
подтвердил трек в тот же день, 2026-10-07) — содержит ровно такой раздел с
|
||||
реальным содержанием:
|
||||
|
||||
> Главные риски — неполный ключ, мутация aliased entry, потеря fallback, рост
|
||||
> памяти, перенос работы на pointerdown. Их закрывают uncached oracle,
|
||||
> lifecycle-сценарии, пределы пулов и измерение полного пользовательского
|
||||
> цикла… Откат — обратный коммит оптимизации и связанных контрактов без
|
||||
> преобразования сохранённых данных.
|
||||
|
||||
У #819 есть минимум один риск, который стоило назвать и явно принять, а не
|
||||
оставить implicit: это первая задача, где «Удалить» у пространства с
|
||||
устройствами перестаёт быть недеструктивным по умолчанию для
|
||||
НЕпоследнего пространства — раньше заблокированное удаление вообще не могло
|
||||
стереть позицию/вложения/след устройства, теперь может, одним кликом, без
|
||||
списка устройств в предупреждении (сознательно вне скоупа) и без отмены
|
||||
после записи (решение владельца, п.3). Совокупность «один клик без списка +
|
||||
без отмены» — ровно то, что раздел «риски» должен называть и привязывать к
|
||||
принятой владельцем мере (K/N в подтверждении, атомарность, защита по
|
||||
ревизии AC4), а не оставлять читателя ТЗ самостоятельно складывать эту
|
||||
картину из контракта.
|
||||
|
||||
«Откат» для этой задачи нетривиален иначе, чем в #814: `remove_markers`
|
||||
(или как его назовут) — необязательный параметр, обратная совместимость
|
||||
очевидна, но сама операция удаления, которую он включает, необратима для
|
||||
пользователя (п.3 — «отмены после записи нет»). Разница между «откатить
|
||||
код» (просто) и «откатить то, что код уже сделал с чужими данными»
|
||||
(невозможно) достаточно важна для первой по-настоящему деструктивной правки
|
||||
в этой задаче, чтобы быть явно проговорённой, а не подразумеваемой.
|
||||
|
||||
**Почему Medium, не High.** AC1–AC6 сами по себе однозначны и проверяемы без
|
||||
этого раздела — отсутствие рисков/отката не блокирует реализацию ни одного
|
||||
AC и не создаёт двусмысленности в контракте. Это полноценный, но дешёвый для
|
||||
автора пробел: абзац по образцу #814 (несколько предложений) закрывает
|
||||
находку.
|
||||
|
||||
**Чем закрывается.** Добавить в тело issue раздел (можно объединённый, как в
|
||||
#814): главный риск — необратимость массового удаления устройств без списка
|
||||
и без отмены, с привязкой к мерам контракта (K/N, атомарность, AC3/AC4);
|
||||
откат — реализация под фичефлагом не нужна (параметр опционален, обратная
|
||||
совместимость полная), но уже случившееся удаление пользовательских данных
|
||||
откату не подлежит — это сознательно принято решением владельца п.3, а не
|
||||
забыто.
|
||||
|
||||
## Что проверено и корректно
|
||||
|
||||
- Обязательные разделы §7.1, кроме рисков/отката (см. находку), присутствуют:
|
||||
проблема и её конкретная причина (не прокручивается длинный диалог) —
|
||||
первой; «что увидит человек после» — без терминов реализации, конкретным
|
||||
пользовательским сценарием с тостом; скоуп очерчен через «Контракт» (6
|
||||
пунктов) и явный «Вне скоупа» (список устройств, перенос из диалога,
|
||||
поведение последнего пространства — не меняется); UX исчерпывающе задан в
|
||||
тех же 6 пунктах контракта; i18n — отдельный AC6; план автотестов и
|
||||
модель данных/бэкенд-механика — в «Принято предположительно» (корректная
|
||||
категория для технических решений по §7.1: «где хранится состояние… агенты
|
||||
решают сами»).
|
||||
- Все процитированные в «Где сейчас» пути и построчные утверждения
|
||||
(`houseplan-editor-runtime.ts`, `houseplan-onboarding-runtime.ts`,
|
||||
`space-form.ts`, `space-deletion.ts`, `websocket_api.py`) подтверждены
|
||||
построчным чтением `dev 4789c3f6` без расхождений — включая деталь про
|
||||
скрытые устройства (`removed !== true` допускает `hidden:true`) и про
|
||||
already-существующий `callout({ action, id })`.
|
||||
- AC1–AC6 однозначны; у каждого есть прослеживаемый способ доказательства:
|
||||
AC1 → смок на двух вьюпортах (1000×700 и 375×812, вторая величина — новый,
|
||||
но разумный выбор для «на телефоне тем более» из самой «Проблемы»; ранее в
|
||||
demo/ таких размеров не было, но ширина 1000 для десктоп-смоков — принятая
|
||||
в проекте норма); AC2–AC4 → юниты кандидата TS/Python + pytest WS с
|
||||
параметром и без (перечислено в «Принято предположительно»); AC5 →
|
||||
существующие тесты (названо явно в самом AC); AC6 → i18n во всех
|
||||
каталогах, USER-GUIDE §7 (ru/en) и оба changelog — раздел §7 существует и
|
||||
сейчас содержит именно то описание, которое придётся переписать.
|
||||
- Контракт п.6 («без устройств» и «последнее пространство» — без изменений)
|
||||
проверен на отсутствие противоречия с AC2: условие блокировки
|
||||
(`dependencies.count && !deletingLastSpace`) делает новую кнопку
|
||||
недостижимой именно для последнего пространства, так что старое
|
||||
поведение «последнее пространство не разрушает устройства» сохраняется
|
||||
структурно, а не только текстом ТЗ — это самосогласованно.
|
||||
- Терминология «Доступны снова», «Скрытые», формулировка тоста и названия
|
||||
вкладок сверены с `docs/USER-GUIDE.ru.md` §10 и §7 — не изобретены.
|
||||
- Решения владельца (3 штуки) оформлены по шаблону §7.1 (что неясно · что
|
||||
меняется · предложенный дефолт), ответ владельца получен по существу, ТЗ
|
||||
обновлено синхронно — никаких несогласованностей между вопросами,
|
||||
ответами и финальным текстом контракта/AC не нашёл.
|
||||
- Отдельно проверил потенциальный источник дефекта parity («controls» в
|
||||
контракте п.4 vs отсутствие серверной очистки `controls[]` у ЧУЖИХ
|
||||
маркеров при пакетном удалении) — не нахожу его находкой, поскольку
|
||||
`docs/DEVICE-PRESENTATION.md` (S07) документирует и тестирует именно этот
|
||||
defensive-path как часть принятой архитектуры вне зависимости от момента
|
||||
появления устаревшей ссылки.
|
||||
- View/домочадцы/киоск корректно исключены из поверхности (редакторская,
|
||||
деструктивная операция, `UX-MODES.md`-граница не нарушается, в ТЗ это
|
||||
прямо написано).
|
||||
|
||||
## Чего не проверял
|
||||
|
||||
- **Не проверял исполнением** — этап `spec`, кода и ветки нет; весь разбор —
|
||||
чтение ТЗ и чтение текущего (ещё не изменённого) `dev 4789c3f6`, не запуск
|
||||
тестов.
|
||||
- Не прогонял `npx tsc --noEmit`, `npm test`, `npm run build` — не гейт
|
||||
этого этапа (зависимости и Chromium не устанавливались, продуктовый код не
|
||||
менялся). Validate на этом SHA уже зелёный
|
||||
(https://github.com/Matysh/houseplan-card/actions/runs/37602320851), но он
|
||||
подтверждает только состояние `dev`, не код задачи — задачи ещё нет.
|
||||
- Смок/golden/pytest/model-invariants — не применимо: реализация
|
||||
(`scrollIntoView`+`focus` хелпер, расширение `_space_delete_candidate`,
|
||||
новый WS-параметр, новые i18n-ключи) не существует. Их выбор и прогон —
|
||||
предмет код-ревью этой задачи.
|
||||
- Не проверял реальное поведение `scrollIntoView({ block: 'nearest' })` и
|
||||
`focus({ preventScroll: true })` в браузере (Chromium не установлен на
|
||||
этом этапе) — это чтение плана, а не проверка будущего кода.
|
||||
- Не перепроверял все восемь каталогов i18n (`src/i18n/*.json` ×4,
|
||||
`custom_components/houseplan/translations/*.json` ×4) на предмет того,
|
||||
какие именно из них получат новые строки — AC6 формулирует это как «все
|
||||
каталоги i18n», что по контексту (текст диалога карточки) однозначно
|
||||
читается как `src/i18n/*`, а не как каталоги HA config-flow; не поднимаю
|
||||
как находку, так как разночтение разрешается чтением кода на этапе
|
||||
реализации без продуктового решения.
|
||||
- Не связывался с #161 (упомянут автором как закрытый дубликат) —
|
||||
доверился утверждению «дубликатов нет», не критично для проверки ТЗ.
|
||||
|
||||
## Вердикт
|
||||
|
||||
ТЗ в основном готово к разработке: сценарий, контракт, AC1–AC6, i18n,
|
||||
скоуп/не-скоуп фактически точны против кода `dev 4789c3f6` — построчная
|
||||
проверка не нашла ни одного расхождения с текущим состоянием репозитория.
|
||||
Единственная находка — отсутствие обязательных по §7.1 разделов «риски» и
|
||||
«откат», при этом свежий принятый документ в том же репозитории (#814, тот
|
||||
же день) показывает, что это не формальность: у #819 есть конкретный,
|
||||
специфический для первой по-настоящему необратимой операции риск
|
||||
(«один клик без списка устройств и без отмены»), который стоит явно назвать
|
||||
и привязать к уже принятым мерам, а не оставлять implicit. Находка — Medium
|
||||
в скоупе задачи, правится несколькими предложениями без нового решения
|
||||
владельца.
|
||||
|
||||
**Вердикт: жёлтый · заход r1 · блокирующих циклов 1/4 · High: 0 · Medium: 1 → в задаче · Документ: docs/reviews/SPEC-REVIEW-819-r1.md**
|
||||
|
||||
---
|
||||
|
||||
<!-- material-anchors: сгенерировано конвейером (#414) -->
|
||||
|
||||
## Материал раунда
|
||||
|
||||
- Ветка: `dev`, коммит `4789c3f6368d` — ребейз его осиротит, и это нормально: ниже якоря, которые ребейз не меняет.
|
||||
- Дерево материала: `c9d3c407e0823dcd555a5ed9a7b9cb09461df18b`
|
||||
```
|
||||
git log --all --format='%H %T' | grep c9d3c407e082
|
||||
```
|
||||
- Тело issue: `3416fe8cd6e43b43d8a5fdd019c7b3806c7bd5da3106d16a1b10b7fb23052c91`
|
||||
- Вердикт конвейера: `yellow` · High 0 · маршрут `fix`
|
||||
<!-- hp:usage input_tokens=4082 output_tokens=44080 cache_creation_input_tokens=147285 cache_read_input_tokens=4410008 num_turns=63 -->
|
||||
Reference in New Issue
Block a user