From 9c9a22354b2b66cb9dde8f14cf7e0d82bcc411fd Mon Sep 17 00:00:00 2001 From: "claude[bot]" <209825114+claude[bot]@users.noreply.github.com> Date: Tue, 1 Sep 2026 16:32:22 +0000 Subject: [PATCH] docs: review document for #406 Issue: #406 User-Visible: no --- docs/reviews/CODE-REVIEW-406-r2.md | 268 +++++++++++++++++++++++++++++ 1 file changed, 268 insertions(+) create mode 100644 docs/reviews/CODE-REVIEW-406-r2.md diff --git a/docs/reviews/CODE-REVIEW-406-r2.md b/docs/reviews/CODE-REVIEW-406-r2.md new file mode 100644 index 00000000..ed695f8a --- /dev/null +++ b/docs/reviews/CODE-REVIEW-406-r2.md @@ -0,0 +1,268 @@ +# CODE-REVIEW-406-r2 + +- Issue: https://github.com/Matysh/houseplan-card/issues/406 +- Этап: code (PROCESS.md §2.7) +- Заход: r2 · блокирующих циклов израсходовано 1 из 4 (перед этим раундом) +- Ветка: `issue/406-beta2-polish` +- Коммит на ревью: `f8cfac9c77332d3061b81239fe0c965add54b7df` +- ТЗ: `docs/specs/406-beta2-polish.md`, revision 4, зелёный `SPEC-REVIEW-406-r4` +- Предыдущий раунд: `docs/reviews/CODE-REVIEW-406-r1.md`, жёлтый, коммит на ревью `fe385cbf` (SHA не сохранился — ветка была перебазирована после r1, см. ниже), единственная находка Medium (устаревший отпечаток скриншотов документации) +- База сравнения: `origin/dev` = `2903374b72a1b16d82eb87acac7233a1db3a3c5e` + +## Почему разбор полный, а не по дельте + +PROCESS.md §2.10 требует полного разбора, если «дельта не локальна: ребейз на +ушедший вперёд `dev`». Именно это произошло между r1 и r2: + +1. r1 (16:14:53) вынес жёлтый вердикт по коммиту `fe385cbf` с единственной + находкой — устаревший отпечаток `docs/images/screenshots.json`. +2. Автор (16:16:16) сообщил, что на финальном SHA `6a34396f` отпечаток уже + обновлён, и попросил повторить ревью. +3. Повторный запуск (16:16:45) **не состоялся** — при ребейзе ветки на + `origin/dev` обнаружился конфликт в `scripts/mutation-gate.mjs` (там же + независимо приземлился #404). Цикл не израсходован: код никто не читал, + вердикта не было. +4. Автор (16:18:41) разрешил конфликт, сохранив мутанты и #404, и #406, + перебазировал ветку. + +Из-за этого у всех коммитов ветки, начиная со старого `fe385cbf`, изменились +хеши — старый SHA не существует в текущей истории (`git cat-file -e +fe385cbf9d5…` → отсутствует), поэтому прямой `git diff fe385cbf..HEAD` +невозможен. Восстановил дельту по содержимому: сравнил текст находок r1 с +текущим деревом и с телом самих коммитов (`git show ` по каждому +коммиту диапазона). + +Дельта оказалась **не локальной** ещё по одной причине, отдельной от ребейза: +между `fe385cbf` (проверено r1) и финальным авторским SHA `6a34396f` +приземнился не только `chore: refresh docs source fingerprint` (закрытие +находки r1), но и отдельный поведенческий коммит `fix: preserve the ordinary +dialog path`, которого r1 не видел и не мог видеть — он был запушен позже +(комментарий 16:14:38, уже после хендоффа на ревью). Это реальная правка кода в +`src/hp-dialog.ts`, а не техническая перестановка. Поэтому разбор веду по всем +двенадцати AC заново, а не только по находке r1. + +## Скоуп + +Тот же, что в r1 — четыре независимых пункта ТЗ: +(а) удаление 13 мёртвых ключей i18n + AST-гейт; (б) `role="alertdialog"` + +`aria-describedby` для `hp-confirm`; (в) смок на обе ветки (HA/native); +(г) уборка `marker_area_snapshot` для исчезнувших устройств при авторитетном +реестре + разворот правила усечения по лимиту. + +Материал — `git log --oneline origin/dev..HEAD` (12 коммитов) и +`git diff origin/dev...HEAD` (44 файла). + +## Что изменилось с r1 (не только rebase) + +`git show` по каждому коммиту диапазона восстанавливает содержательную +последовательность после спек-ревью (`d3412e6f`): + +| Коммит (текущий хеш) | Содержание | Был ли виден r1 | +|---|---|---| +| `5d2f2957` fix: close beta 2 polish gaps | основная реализация (а)-(г), `User-Visible: yes` | да, это и есть `fe385cbf` по содержанию | +| `d5f050fd` fix: preserve the ordinary dialog path | **новая находка ниже** — правка `src/hp-dialog.ts`, добавляет второй ``-шаблон | **нет**, запушен после хендоффа r1 | +| `13593c1b` chore: refresh docs source fingerprint | закрывает Medium r1 | нет, запушен в ответ на вердикт r1 | +| `f8cfac9c` docs: review document for #406 | публикация `CODE-REVIEW-406-r1.md` | — | +| `2903374b` (в `origin/dev`, не в ветке) fix: exception guard… (#404) | не относится к #406, но конфликтовал с #406 в `scripts/mutation-gate.mjs` при ребейзе | — | + +## Как проверялось + +Ручного тестирования в цикле нет. Зелёного Validate на `f8cfac9c` не найдено — +всё ниже прогнано лично. + +| Гейт | Результат | +|---|---| +| `npx tsc --noEmit` | green, без вывода | +| `npm test` | 1711 passed, 0 failed, 1 skipped (было 1706 в r1 — разница ровно от рёбер #404 в `dev`) | +| `npm run build` | green, 15.7s | +| `npm run bundle:sync` (build + `bundle-sync.mjs --check` эквивалент — прогнал `bundle:sync` целиком и проверил `git status` после) | green, `git status --porcelain` пуст после — три копии бандла синхронны | +| `npm run bundle:budget` | initial 287370 B / budget 300000 B, headroom 12630 B (>0 → AC12 выполнен); предупреждение о низком запасе — фон #367, не регрессия | +| `node scripts/mutation-gate.mjs --check` | все мутанты `ok` (331), включая три мутанта #406 (`i18n-dead-key-returns`, `confirm-dialog-loses-alertdialog`, `area-snapshot-cleanup-ignores-authority`) и два мутанта #404, приземлившихся рядом при конфликте — оба набора пережили ребейз | +| `node scripts/no-new-any.mjs --base origin/dev --head HEAD` | «Новых any нет» (проверено 55 добавленных строк в 3 файлах) | +| `node scripts/check-docs.mjs` | **green** — находка r1 закрыта | +| `node scripts/check-docs.mjs --external` | green, 10 внешних ссылок | +| `node scripts/process-gate.mjs` | «гейт пройден, предупреждений 0» | +| `node demo/smoke_danger_confirm_branches.mjs` | green, все 24 поля `true` (AC6–AC8), включая `ordinaryDialogForwardsHaAria`/`ordinaryDialogUsesHaBranch` | +| `node demo/smoke_area_relocation.mjs` | green, все 20 полей `true` (AC9–AC10) | +| `node scripts/smoke-select.mjs --base origin/dev --head HEAD` | «Прямое совпадение (1)»: `demo/smoke_help_affordance.mjs ← _focusInitial` — символ появился на изменённой строке именно из-за нового дублированного ``-шаблона (см. находку); прогнал | +| `node demo/smoke_help_affordance.mjs` | green, все поля `true`, включая `keyboardFocus` | + +**Не прогонял и почему:** + +- Полная матрица `demo/smoke_*.mjs` (212 файлов) — диапазон touches только + `hp-dialog`/`hp-confirm`, `device-area-relocation` и словари; единственный + «прямой» кандидат из `smoke-select` прогнан, остальные символы (в основном + внутренние геттеры `device-area-relocation.ts`) уже покрыты юнитами + построчно (проверено чтением). Полный прогон — предрелизная обязанность. +- `npm run golden:verify` — `demo/golden/matrix.mjs` не менялся с r1 (сверил + диффом с `origin/dev`); ни один сценарий (`device-dialog-*` и др.) не + открывает destructive/warning `hp-confirm`. +- `npm run invariants` / `python -m pytest tests_backend` — диапазон не + трогает `.py`, рёбра комнат, `layout`, `marker.space`, `open_spans` + (проверено `git diff --stat` по `custom_components/**/*.py` — пусто). +- Перфоманс-профили — не названы в AC. +- Полный HA-харнесс / реальный визуальный просмотр в HA — вне цикла ревью. + +## Находки + +### Low (в скоупе, снимается с записью) — правка `fix: preserve the ordinary dialog path` не имеет собственного быстрого регресс-теста + +`src/hp-dialog.ts:452-467` (текущий диапазон, коммит `d5f050fd`, ранее не +рецензировался): рендер обычного (не-alert) `hp-dialog` при зарегистрированном +`ha-dialog` теперь разделён на два шаблона: + +```html +if (this.describedBy) { + return html`……`; +} +return html`……`; +``` + +До этой правки (в версии, которую видел r1, коммит `5d2f2957`) была одна +ветка с `.ariaDescribedBy=${this.describedBy || undefined}` — то есть свойство +`ariaDescribedBy` явно выставлялось в `undefined` на **всех** ~30 существующих +вызовах `` без описания (маркер, калибровка, бэкдроп, импорт/экспорт +и т.д. — весь обычный, не-`hp-confirm`, путь). Автор обнаружил это не тестом, а +побочно: обязательная пересъёмка `check-docs.mjs` дала пиксельную дельту в двух +кадрах обычных диалогов (комментарий 16:14:38), и это был реальный визуальный +регресс на самом частом пути карточки — большинство диалогов не альтернативны. + +Правка верна и подтверждена: канонический прогон «Docs screenshots» +(run [33530447151](https://github.com/Matysh/houseplan-card/actions/runs/33530447151)) +дал побайтовое совпадение всех 10 PNG после фикса — я сверил +`docs/images/screenshots.json`: `imageSha256` всех сценариев не изменился, +поменялись только `sourceFingerprint`/`sourceSha256`. Это надёжное +доказательство отсутствия визуальной регрессии на зафиксированных кадрах. + +Но у самого фикса (ветка «нет описания» на HA-пути) нет **быстрого** +автотеста: `demo/smoke_danger_confirm_branches.mjs` проверяет только случай «с +описанием» (`ordinaryDialogForwardsHaAria`, `describedBy = 'ordinary-description'` +— истинное значение); AC7 в ТЗ тоже требует доказательство только для этого +случая. Случай «без описания», который и был реальным дефектом, не покрыт ни +юнитом, ни смоком, ни мутантом `mutation-gate.mjs` — единственная защита от +повторного регресса это сам `check-docs.mjs` (обязателен по механике при любой +правке `src/**`) плюс ручная пиксельная сверка при следующей пересъёмке. +Это тот же класс риска, что и «тихий успех» из PROCESS.md §2.9 (#171, #207): +если кто-то в будущем снова объединит обе ha-dialog-ветки в одну ради +дедупликации кода, `npm test`/`mutation-gate --check`/оба browser-смока этой +задачи останутся зелёными, и только пиксельный дифф на следующей пересъёмке +скриншотов покажет проблему — то есть с существенной задержкой относительно +коммита, который её внёс. + +**Снимаю без возврата в работу**: поведение на этом SHA доказанно корректно +(канонический скриншот-прогон + факт, что `check-docs.mjs` green), находка не +блокирует AC7 и не относится к сценарию, который ТЗ просило доказать. Рекомендация +на будущее — не для этого цикла: добавить в `mutation-gate.mjs` мутанта, +убирающего ветвление `if (this.describedBy)` (schlagen: замена на старую +безусловную форму), guard — `node demo/smoke_danger_confirm_branches.mjs` +с новым полем-утверждением «обычный диалог без описания не передаёт +`ariaDescribedBy`». Дешёво и ловит именно этот класс регрессии за секунды, а +не за цикл релиза. + +## Закрытие раунда r1 + +| Находка r1 | Чем закрыта | Где это видно | +|---|---|---| +| Medium: `node scripts/check-docs.mjs` красный на `fe385cbf` — устаревший отпечаток `docs/images/screenshots.json` | Коммит `13593c1b` (`chore: refresh docs source fingerprint`) обновил `sourceFingerprint`/`sourceSha256` по каноническому прогону «Docs screenshots» run 33530447151; `imageSha256` не изменился ни для одного из 10 сценариев | `git diff origin/dev...HEAD -- docs/images/screenshots.json`: меняются только два поля-хеша фингерпринта на строку, `imageSha256` идентичен. `node scripts/check-docs.mjs` — green (прогнал лично) | + +## Унаследовано из r1 — с оговоркой + +Формально это полный разбор (§2.10, ребейз на ушедший вперёд `dev`), поэтому +все AC перепроверены заново в этом раунде, а не унаследованы бланково. Но там, +где текущий файл байт-в-байт совпадает с версией, которую видел и проверил +тестом/смоком r1, я не повторяю тот же путь доказательства второй раз, а +опираюсь на сочетание «r1 уже прогнал тест X» + «файл не менялся с тех пор, +проверено `git diff origin/dev...HEAD -- ` и содержимым коммитов между +`fe385cbf`-эквивалентом и `HEAD`»: + +- **`src/device-area-relocation.ts` (AC9–AC11)** — не менялся ни одним из + коммитов после `5d2f2957`. Логика (авторитетный реестр строит `liveIds`/ + `liveBindings`, инвариант `.slice(-MARKER_AREA_SNAPSHOT_LIMIT)`) идентична + описанной в `CODE-REVIEW-406-r1.md`. Перечитал файл и оба продуктовых + вызова (`houseplan-card.ts:5054/5166`) самостоятельно — совпадает с + описанием r1. Тесты `test/device-area-relocation.test.mjs` (14/14) и смок + `demo/smoke_area_relocation.mjs` (20/20 полей) прогнаны мной лично в этом + раунде, не по памяти r1. +- **AC1–AC5 (мёртвые ключи i18n)** — файлы словарей и + `test/i18n-dead-keys.test.mjs`/`test/unified-wall-tool-source.test.mjs` не + менялись после `5d2f2957`. Перепрогнал `node --test + test/i18n-dead-keys.test.mjs` (2/2) и полный `npm test` лично. +- **Раздел «одно число — один источник»** — диапазон по-прежнему не добавляет + видимых пользователю величин (роль/aria — не число); `initial View` в + `bundle:budget` — единственное число, и оно считается один раз сборкой, + что я перепроверил (287370 B совпадает и в выводе `bundle:budget`, и в + `houseplan-assets.json`). `test/single-source-numbers.test.mjs` в составе + `npm test`, прошёл. +- **Трейлеры/changelog** — перепроверил самостоятельно все 12 коммитов + диапазона (`git show -s --format=%B`): `Issue: #406` на каждом, + `User-Visible: yes` только на `5d2f2957`, оба changelog правятся в этом же + коммите (сверил `git show --stat 5d2f2957`). + +Ссылка на документ прежнего раунда: `docs/reviews/CODE-REVIEW-406-r1.md` +(доступен в дереве этой ветки, коммит `f8cfac9c`). + +## Что проверено и корректно (AC1–AC12, этот раунд) + +- **AC1–AC3**: `test/i18n-dead-keys.test.mjs` (AST по `src/**/*.ts`, + `derivedHelpAria.size === 19`) — 2/2, прогнано лично. +- **AC4**: коллизия с `test/unified-wall-tool-source.test.mjs` разрешена той + же строкой, что и в r1 — `'history.partition_add'` убрана из проверяемого + списка в том же коммите, что и ключ из словарей. +- **AC5**: паритет словарей — `npm test` включает `test/i18n.test.mjs`, + прошёл; сверил `git diff` по всем четырём словарям — 13×4=52 строки. +- **AC6**: оба `kind` `hp-confirm` безусловно получают `.alert=${true}` и + `.describedBy=${descriptionId}` (`src/hp-confirm.ts:40-41`, перечитал файл + целиком) → `_usesHaDialog()` (`this._useHaDialog && !this.alert`) всегда + `false` для confirm → нативный `` (`src/hp-dialog.ts:483`). Смок: `noHa*IsDescribedAlert`, + `ha*StaysNativeAlert`, `realAccessibilityTreeIncludesConsequence` — все + `true`. Мутант `confirm-dialog-loses-alertdialog` ловится (`mutation-gate + --check`, ok). +- **AC7**: перечитал новую (двухветочную) реализацию `_usesHaDialog()===true` + пути — новая находка описана выше, но контракт AC7 доказан: + `ordinaryDialogUsesHaBranch`/`ordinaryDialogForwardsHaAria` — `true` в + смоке, прогнан лично. +- **AC8**: фокус на «Отмена», Esc → `false`, клик по «Отмена» → `false` в + обоих окружениях — все соответствующие поля смока `true`, прогнан лично. +- **AC9/AC10**: см. раздел «Унаследовано» — перепроверено чтением и тестом + заново в этом раунде, не по памяти r1. +- **AC11**: юнит-тест `defensive snapshot reader keeps the newest entries + when over its limit` — `ok 13` в `test/device-area-relocation.test.mjs`, + прогнан лично; символ правки не менялся с r1. +- **AC12**: `npm run bundle:budget` → 287370 B / 300000 B, положительный + запас; число на 50 Б больше, чем в r1 (287320 → 287370) — ожидаемо: новая + ветка `if (this.describedBy)` добавляет несколько байт разметки. +- **Трейлеры и changelog**: см. «Унаследовано», перепроверено самостоятельно + на всех 12 коммитах диапазона, не только на `5d2f2957`. + +## Чего не проверял + +- Полную матрицу `demo/smoke_*.mjs` (212 файлов) — обоснование в «Как + проверялось». +- `npm run golden:verify` — ни один голден-сценарий не открывает + затронутый диалог, сверил `demo/golden/matrix.mjs` заново на этом SHA. +- `npm run invariants` / `python -m pytest tests_backend` — диапазон их не + задевает. +- Перфоманс-профили — не названы в AC, не тронуты. +- Подлинность прогона `run 33530447151` — доверяю ссылке на GitHub Actions, + не открывал сам артефакт; сверил только зафиксированные в + `docs/images/screenshots.json` хеши образов (не изменились) как косвенное, + но веское подтверждение. +- Реальный визуальный вид нативного диалога в живом Home Assistant — только + по коду, accessibility-дереву Chromium и каноническому скриншот-прогону; + ручного просмотра в этом цикле нет и не может быть. + +## Вердикт + +Единственная находка этого раунда — Low, по правке, которая появилась в +дельте после r1 (`fix: preserve the ordinary dialog path`), не покрыта +собственным быстрым тестом, но её корректность доказана канонической +пересъёмкой скриншотов (побайтовое совпадение) и обычным browser-смоком для +того случая, который требует AC7. Не блокирует, снимаю с рекомендацией на +будущее (см. находку). Находка r1 (устаревший отпечаток документации) закрыта +по существу и проверена лично, а не по слову автора. Все 12 AC доказаны +тестом или смоком, который я лично прогнал в этом раунде и который умеет +падать (мутанты подтверждают три ключевых контракта, включая оба +приземлившихся после ребейза набора — #406 и #404). High-находок нет. + +**Зелёный.**