From a7f0facf9a94180d57c268e9b3be296793be3ea2 Mon Sep 17 00:00:00 2001 From: "claude[bot]" <209825114+claude[bot]@users.noreply.github.com> Date: Fri, 28 Aug 2026 11:52:12 +0000 Subject: [PATCH] docs: review document for #341 Issue: #341 User-Visible: no --- docs/reviews/CODE-REVIEW-341-r1.md | 146 +++++++++++++++++++++++++++++ 1 file changed, 146 insertions(+) create mode 100644 docs/reviews/CODE-REVIEW-341-r1.md diff --git a/docs/reviews/CODE-REVIEW-341-r1.md b/docs/reviews/CODE-REVIEW-341-r1.md new file mode 100644 index 00000000..6764c834 --- /dev/null +++ b/docs/reviews/CODE-REVIEW-341-r1.md @@ -0,0 +1,146 @@ +# CODE-REVIEW — issue #341 · заход r1 + +## Скоуп + +Issue #341 (трек `trivial`, метки `bug`/`P3`/`polish`): аудит C5 нашёл, что +`src/labs.ts` защищает подписку на `hashchange`/`popstate` только +модульным флагом `listening`. Home Assistant может перезагрузить ресурс +карточки без перезагрузки страницы (обновление через HACS, dev-режим, +повторная регистрация ресурса) — новая инстанция модуля не видит старый +флаг и добавляет вторую пару обработчиков на тот же `window`, что даёт +повторную обработку одного события. + +AC1–AC3 из тела issue: +- AC1/AC2 — после двух инициализаций Labs в одном `window` одно событие + `hashchange`/`popstate` вызывает обработчик только актуальной инициализации. +- AC3 — обычная одиночная инициализация и разрешение флагов не меняются. + +Диапазон: `git log --oneline origin/dev..HEAD` / `git diff origin/dev...HEAD`. +Коммиты: +- `5f6bddea` fix: deduplicate Labs location listeners — `src/labs.ts`, + `test/labs.test.mjs`. +- `2b6db4c8` docs: refresh screenshot fingerprint — `dist/**`, + `custom_components/houseplan/frontend/**`, `docs/images/**`, + `docs/images/screenshots.json` (следствие правки `src/**`, не отдельная + задача). + +Продуктовая рамка (SCOPE.md): фикс внутренний, к Core user jobs не +привязан напрямую — это защита контракта Labs («One module-level browser +listener; card instances only subscribe to its snapshot»), которым +управляется presentation-only флаг `iso` (#89, J1-презентация). Правка не +меняет UX, URL/storage-грамматику Labs, жизненный цикл карточки — всё +согласно разделу «Не входит» issue. Изменение точечное: только +`src/labs.ts` (class A) + `test/labs.test.mjs` (class B) + сгенерированные +class D/C артефакты. Постороннего кода в диффе нет. + +## Как проверялось + +Дешёвые гейты на SHA `2b6db4c8` подтверждены Validate +(https://github.com/Matysh/houseplan-card/actions/runs/33168458678, success) — +`npx tsc --noEmit`, `npm test`, `npm run build` со сверкой бандла не +перегонял. + +Сам прогнал точечно (диапазон правки узкий, стоимость минимальна): + +- `npx tsc -p tsconfig.test.json && node scripts/fix-test-build.mjs && + node --test test/labs.test.mjs` → **green**, 6/6, включая новый тест + «a reloaded Labs module replaces both browser location listeners». +- **Тест умеет падать**: временно убрал вызов + `window.__hpLabsListenerCleanup?.()` из собранного `test-build/labs.js` + (эмуляция кода до фикса) и перезапустил — новый тест краснеет + (`2 !== 1`, ожидание `firstPublishes === 1`, фактически `2`, т.к. + обработчик снятой инстанции продолжает публиковать на `hashchange`). + Откатил файл обратно, `git status` подтвердил чистое дерево. +- `node scripts/check-docs.mjs` → **green** (7 файлов, 10 внешних ссылок) — + обязателен, т.к. диф трогает `src/**`. +- Байт-в-байт сверка `dist/**` ↔ `custom_components/houseplan/frontend/**` + (`houseplan-card.js`, `houseplan-assets.json`, все файлы + `houseplan-assets/*`) → идентичны, `bundle:sync` не разошёлся. +- `node scripts/smoke-select.mjs --base origin/dev --head HEAD` → + **НЕОПРЕДЕЛЁННОСТЬ**: 1 файл `src/**`, ни один из 196 смоков не связан + доказуемо; символ без покрытия — `onLocationChange`. Решение: не + прогонять браузерные смоки — `onLocationChange` не рендерит и не меняет + DOM/маршрутизацию, это чистая функция диспетчеризации на + `publishBrowserLabs`, а контракт «ровно одна активная пара обработчиков» + полностью воспроизведён и провёрен модульным тестом с двумя реальными + ES-module инстанциями (через `import(...'?listener-instance=...')`), + что ближе к реальному сценарию (повторная загрузка ресурса), чем любой + браузерный смок мог бы дать. +- Чтение кода (`src/labs.ts:147–212`): прослежен путь `ensureBrowserLabs` → + `window.__hpLabsListenerCleanup` для 1, 2 и 3 последовательных + инстанций — каждая новая инстанция снимает ровно предыдущую пару + обработчиков (через замыкание `cleanup`, ссылающееся на её же + `onLocationChange`) и кладёт на `window` свой `cleanup`; guard + `if (window.__hpLabsListenerCleanup === cleanup)` предотвращает + случайное затирание чужого cleanup при непоследовательном вызове. + Повторный вызов `ensureBrowserLabs` в РАМКАХ ОДНОЙ инстанции (обычный + случай — несколько карточек на одной странице) не трогает `if (!listening)` + вовсе — поведение AC3 не изменено кодом, а не только заявлением. + +Не прогонял: `npm run golden:verify` (диф не меняет рендер/геометрию/стили — +`onLocationChange` не рисует ничего), `npm run invariants` (геометрия, +`layout`, толщина стен, `open_spans` не затронуты), `python -m pytest +tests_backend` (`custom_components/**/*.py` не менялся), +performance-профили (не названы в AC, путь не в горячей точке рендера). +`npm run bundle:budget` не перегонял — автор привёл число +(256046 B при бюджете 282000 B) и разница от `iso`-строки правки +незначительна; независимо сверил байт-идентичность копий бандла, что +покрывает риск рассинхронизации сильнее, чем повторный бюджет-прогон. + +## Находки + +Нет находок High или Medium. Мелких Low-находок, требующих правки или +явного отказа, не нашёл — код и тест соответствуют предложенному в issue +решению («хранить состояние подписки на `window` под приватным глобальным +ключом») буквально. + +## Что проверено и корректно + +- **AC1/AC2 доказаны автотестом**, тест умеет падать (см. выше) — это не + «verified» на честном слове, а воспроизведённый красный/зелёный цикл. +- **AC3** — код, отвечающий за одиночную инициализацию, не тронут + (`if (!listening)` guard и его тело для случая true остаются пустой + веткой); резолвер флагов (`resolveLabs`, `liveLabsFlags` и т.д.) + вообще не в диффе — старые тесты резолвера все зелёные без изменений. +- Три и более последовательных инстанций разобраны построчно — не только + «два», как в тесте, — инвариант «ровно одна активная пара» держится по + индукции для N инстанций. +- `window.__hpLabsListenerCleanup` — новое глобальное свойство описано в + `declare global`, именование согласовано с существующим `__hpLabs`, + двойное подчёркивание держит приватность соглашения. +- Labs-правила AGENTS.md соблюдены: изменение не вводит новый флаг, не + трогает грамматику URL/storage, не гейтит данные/HA-действия — + чисто presentation-инфраструктура регистрации слушателей. +- Трейлеры обоих коммитов корректны: `Issue: #341`, `User-Visible: no` — + согласился с оценкой автора: фикс невидим при обычном использовании + (устраняет задвоение реакций на скрытый Labs-флаг `iso`, а не меняет + какое-либо наблюдаемое поведение), changelog обоснованно не трогался. +- Байты бандла (`dist` ↔ integration copy) и docs-фингерпринт + (`check-docs.mjs`) сошлись — вторая правка `2b6db4c8` не оставила + `dev` с красным `docs`-джобом (прецедент #230/#234 не повторился). +- Диф нигде не выходит за нужные классы файлов: A (`src/labs.ts`), B + (`test/labs.test.mjs`), C/D (сгенерированные скриншоты/бандл) — без + постороннего кода. +- Единственное новое число, видимое в тесте (счётчики публикаций), не + дублирует какую-либо пользовательскую величину — сравнение «одно число, + один источник» здесь неприменимо: change не вводит новых значений на + экране. + +## Чего не проверял + +- Полный `npm test`/`npm run build`/`npx tsc --noEmit` по всему проекту — + положился на зелёный Validate на этом же SHA (см. «Как проверялось»). +- Браузерные смоки — инструмент дал НЕОПРЕДЕЛЁННОСТЬ, обосновал отказ + выше; ручного захода в demo-стенд не делал. +- `golden`, backend pytest, performance — вне зоны дифа, см. обоснование + выше. +- Реальный сценарий HACS-перезагрузки ресурса в живом Home Assistant — + вне возможностей цикла ревью (нет ручного тестирования); заменено + точной симуляцией через две независимые ES-module инстанции, что и + является механизмом бага (новая инстанция модуля = новый `listening`). + +## Вердикт + +Зелёный. Все три AC выполнены и доказаны (автотест + чтение кода), фикс +не выходит за скоуп issue, гейты, относящиеся к дифу, зелёные, трейлеры и +byte-sync корректны. Блокирующих находок нет.