From 8f11c9e90adc576f807a9b6726b8978b0879bda5 Mon Sep 17 00:00:00 2001 From: "claude[bot]" <209825114+claude[bot]@users.noreply.github.com> Date: Wed, 7 Oct 2026 12:29:41 +0000 Subject: [PATCH] docs: review document for #819 Issue: #819 User-Visible: no --- docs/reviews/INDEX.md | 3 +- docs/reviews/SPEC-REVIEW-819-r1.md | 268 +++++++++++++++++++++++++++++ 2 files changed, 270 insertions(+), 1 deletion(-) create mode 100644 docs/reviews/SPEC-REVIEW-819-r1.md diff --git a/docs/reviews/INDEX.md b/docs/reviews/INDEX.md index bca888b6..f88be3dc 100644 --- a/docs/reviews/INDEX.md +++ b/docs/reviews/INDEX.md @@ -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 | — | — | diff --git a/docs/reviews/SPEC-REVIEW-819-r1.md b/docs/reviews/SPEC-REVIEW-819-r1.md new file mode 100644 index 00000000..c8b167da --- /dev/null +++ b/docs/reviews/SPEC-REVIEW-819-r1.md @@ -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** + +--- + + + +## Материал раунда + +- Ветка: `dev`, коммит `4789c3f6368d` — ребейз его осиротит, и это нормально: ниже якоря, которые ребейз не меняет. +- Дерево материала: `c9d3c407e0823dcd555a5ed9a7b9cb09461df18b` + ``` + git log --all --format='%H %T' | grep c9d3c407e082 + ``` +- Тело issue: `3416fe8cd6e43b43d8a5fdd019c7b3806c7bd5da3106d16a1b10b7fb23052c91` +- Вердикт конвейера: `yellow` · High 0 · маршрут `fix` +