From 6d094ca01b1add4ea123b8f7fabc9d668ff199bd Mon Sep 17 00:00:00 2001 From: "claude[bot]" <209825114+claude[bot]@users.noreply.github.com> Date: Tue, 25 Aug 2026 08:33:33 +0000 Subject: [PATCH] docs: review document for #301 Issue: #301 User-Visible: no --- docs/reviews/SPEC-REVIEW-301-r1.md | 279 ++++++++++++++++------------- 1 file changed, 154 insertions(+), 125 deletions(-) diff --git a/docs/reviews/SPEC-REVIEW-301-r1.md b/docs/reviews/SPEC-REVIEW-301-r1.md index 8bbddee7..ace064a1 100644 --- a/docs/reviews/SPEC-REVIEW-301-r1.md +++ b/docs/reviews/SPEC-REVIEW-301-r1.md @@ -1,160 +1,189 @@ # SPEC-REVIEW — issue #301, заход r1 **Тема:** поиск/фильтр в селекторах контакта и замка диалога проёма (дверь/окно/ворота). -**Трек:** `small` — ТЗ в теле issue, ревью комментарием (этот документ прикладывается как файл ревьюера; в issue уходит краткий комментарий). +**Трек:** `small` — ТЗ в теле issue, ревью комментарием (этот документ — файл ревьюера; в issue уходит краткий комментарий). **Заход:** r1 · блокирующих циклов израсходовано 0 из 2 (лимит лёгкого трека — 2). -**Материал:** тело issue #301 + комментарий-ТЗ от 2026-08-25 (Codex), код на `dev` @ `4b8f17ba`. +**Материал:** тело issue #301 + комментарий-ТЗ (Codex, редактирован 2026-08-25, текущая версия на момент этого ревью), код на `dev` @ `65e55872`. + +## Контекст: почему это снова r1, а не r2 + +Автоматическое ревью уже отработало один раз (комментарий от `claude`, +2026-08-25T08:09:14Z, документ `docs/reviews/SPEC-REVIEW-301-r1.md`, коммит +`68498e52`, вердикт жёлтый, Medium: 2). Владелец зафиксировал сбой конвейера +*после* публикации вердикта: статусная метка не переставилась +(issue-комментарий 2026-08-25T05:14:19Z, прогон +`actions/runs/32811973025`). По правилу «после прогона ревью метка всегда +меняется; если не сменилась — упал сам прогон» (`AGENTS.md`) этот заход +считается не состоявшимся механически, и настоящий прогон переисполняет его +как r1, а не как r2 — с полным разбором, не по дельте (§2.10 применяется +начиная со второго *засчитанного* цикла). + +Автор уже ответил на находки того захода правкой текста ТЗ (комментарий +2026-08-25T08:23:38Z: «M1 — accept, M2 — accept, Low — accept»). Ниже — +самостоятельная проверка текущего (уже отредактированного) ТЗ с нуля, а не +доверие к заявлению автора о зачёте. Совпадение с прежними находками там, где +оно есть, отмечено явно. ## Скоуп -Заменить нативный `` (локальный хелпер `opt()`, +`houseplan-card.ts:19463`) в диалоге проёма для двух полей — контакта и замка — +на существующий визуальный паттерн `dropbtn`/`droppanel`/`candlist` с текстовым +полем поиска, без изменения набора кандидатов, их базового порядка (кроме как +под фильтром) и формата конфига (`opening.contact`/`opening.lock`). ## Как проверялось -Ревью ТЗ на этом этапе — не код-ревью: кода нет, есть только текст ТЗ. Задача -ревьюера — проверить, что описанное реализуемо и не противоречит себе, и что -каждая ссылка на код/паттерн/строку i18n в тексте ТЗ соответствует -действительности (иначе автор строит решение на несуществующей опоре). -Прочитаны: `docs/SCOPE.md`, `AGENTS.md`, `PROCESS.md` §2.4/§2.10/§7.1/§7.2, -`docs/TOUCH-SUPPORT.md`, `docs/USER-GUIDE.ru.md` (раздел 9, «Настройки проёма»). -Каждая фактическая ссылка ТЗ сверена с текущим `src/houseplan-card.ts` на `dev`: +Кода нет — есть только текст ТЗ, поэтому это не код-ревью. Задача — проверить, +что описанное реализуемо, не противоречит себе и что каждая фактическая ссылка +на код/паттерн/строку i18n соответствует действительности (иначе автор строит +решение на несуществующей опоре). Прочитаны: `docs/SCOPE.md`, `AGENTS.md`, +`PROCESS.md` §2.4/§2.5/§2.10/§7.1/§7.2/§5, `docs/TOUCH-SUPPORT.md`, +`docs/USER-GUIDE.ru.md` (раздел «Настройки проёма»). Каждая фактическая ссылка +ТЗ сверена с текущим `src/houseplan-card.ts` на `dev`@`65e55872`: | Ссылка ТЗ | Проверка | Результат | |---|---|---| -| `opt()` — локальный select-хелпер, `:19463` | `grep`/чтение | подтверждено, строка 19463 | -| `_contactCandidates()` `:12871`, `_lockCandidates()` `:12888` | чтение | подтверждено, сигнатуры и сортировка как описаны | -| Паттерн `dropbtn`/`droppanel`/`candlist` у диалога маркера, фильтрация `:13901` | чтение | подтверждено — это `_bindingCandidates()` (13832–13905), рендер в 20868–20903 | -| Фильтр — подстрока по `label+sub+value`, регистронезависимый, кап 200 | чтение `_bindingCandidates()` | подтверждено (`filtered.slice(0, 200)`, строка ~13905) | -| `marker.nothing_found`, `opening.none`, `opening.contact_label`, `opening.lock_label`, `opening.invert`, `marker.search_ph` | `grep` в `src/i18n/{ru,en}.json` | все ключи существуют в обоих файлах | -| `_saveOpening` | чтение (`:12751`, вызов `:19557`) | подтверждено | -| `passage` не имеет селектора контакта | чтение блока рендера (`d.type !== 'passage'` guard, `:19519`) | подтверждено — блок контакта/замка целиком под условием | -| Лок-селектор только для door/gate | чтение (`d.type === 'door' || d.type === 'gate'`, `:19528`) | подтверждено | -| `scripts/mutation-gate.mjs`, `test-build` (`fix-test-build.mjs`) | `ls`/`grep package.json` | существуют | -| `opt(` используется только в контакте/замке (не задевает другой локальный `opt` в другом диалоге, `:21656`) | `grep` всех вызовов | подтверждено — разные локальные функции, конфликта нет | +| `opt()` — локальный select-хелпер, `:19463` | чтение | подтверждено (строка 19463, ровно тот блок) | +| `_contactCandidates()` `:12871`, дверные `device_class` первыми | чтение (12871–12886) | подтверждено — сортировка `doorish ? 0 : 1` перед алфавитом | +| `_lockCandidates()` `:12888` | чтение (12888–12892) | подтверждено — только `lock.*`, алфавитная сортировка | +| Контактный селектор скрыт для `passage` | чтение (guard `d.type !== 'passage'`, `:19519`) | подтверждено — весь блок контакта/замка под условием | +| Замковый селектор только для `door`/`gate` | чтение (guard `:19528`) | подтверждено | +| Паттерн `dropbtn`/`droppanel`/`candlist` существует | `grep dropbtn/droppanel` | подтверждено — **два** живых экземпляра: `_bindingCandidates()` (рендер 20868–20903) и `_roomSrcCandidates()` (рендер 21705–21728), не один, как можно понять из текста ТЗ | +| `_bindingCandidates()` пересортировывает безусловно даже без запроса | чтение (`:13901`, `filtered.sort(...)` вне ветки `f ?`) | подтверждено — ровно то, что называет ТЗ как причину отказа от этого образца | +| `_runCandidates()` не пересортировывает при фильтрации | чтение (сборка/сортировка один раз `:13815–13827`, фильтр без ресорта на рендере `:20981`) | подтверждено | +| Кап 200 «как у поисковых селекторов карточки» | чтение (`_bindingCandidates` `:13905`, `_roomSrcCandidates` `:18722`) | подтверждено — оба капают на 200. `_runCandidates()`, чей алгоритм фильтрации ТЗ просит взять за образец, сам капает иначе (`.slice(0, 40)`, `:20981`) — ТЗ берёт кап числом от одного образца, а форму фильтрации от другого; явных противоречий в тексте это не создаёт, но стоило назвать оба источника | +| `opening.none`, `opening.contact_label`, `opening.lock_label`, `opening.invert`, `marker.nothing_found` | `python -m json` по `src/i18n/{en,ru}.json` | все пять ключей существуют в обоих языках с ожидаемым смыслом | +| `opening.search_ph` — новый ключ | тот же разбор | подтверждено отсутствие — ТЗ корректно называет его новым | +| `_saveOpening` | чтение (`:12751`, вызов из футера диалога `:19557`) | подтверждено | +| `scripts/mutation-gate.mjs`, `scripts/fix-test-build.mjs` | `ls` | оба существуют | +| Именование смока `demo/smoke_opening_entity_search.mjs` | `ls demo/smoke_opening*.mjs` | согласуется с существующим рядом (`smoke_opening_binding.mjs`, `smoke_opening_measure.mjs`, `smoke_opening_preview.mjs`, …) | +| Раздел `docs/USER-GUIDE.ru.md` «Настройки проёма» существует и описывает контакт без поиска | чтение (строки 611–629) | подтверждено — там же явно сказано, из чего выбирается контакт; корректное место для правки release-артефакта | -Не проверялось (и не должно на этом этапе): код ещё не написан, поэтому -`typecheck`/`test`/`build`/смоки не прогонялись — предмета для них нет. Это не -пропуск гейта, а корректная граница этапа ТЗ. +Не проверялось (и не должно на этом этапе): кода нет, поэтому +`typecheck`/`test`/`build`/смоки/`check-docs`/инварианты модели не прогонялись — +предмета для них нет. Это граница этапа ТЗ, а не пропущенный гейт. ## Находки -### [Medium, в скоупе] AC6 противоречит явно указанному образцу для переиспользования +### [Medium, в скоупе] Контракт не решает, что показывает строка кандидата в открытой панели -П.4 «Контракт поведения» требует: «Наборы кандидатов и их порядок не меняются: -те же `_contactCandidates()` (дверные первыми)… тот же кап, что у биндинга -маркера (200)» — и AC6 требует: «Без запроса порядок прежний: дверные -device_class первыми». +П.4.1 фиксирует, что в **закрытом** состоянии кнопка `dropbtn` показывает +`friendly_name` **и** `entity_id` подписью. П.4.2 описывает открытую панель как +«поле поиска + список кандидатов», но не говорит, что видно в каждой строке +`candlist` — только `label`, или `label` плюс `entity_id` как подпись. -Но образец, на который ТЗ прямо ссылается в п.2 как на паттерн для -переиспользования — `_bindingCandidates()` (`houseplan-card.ts:13832`, -фильтрация на `:13901`) — устроен так: +Это не мелочь ради красоты: у двух реально существующих `candlist`-виджетов +в проекте поведение расходится. `_bindingCandidates()` показывает под именем +`sub` — модель устройства (`sub: dev.model || …`), а `_roomSrcCandidates()` +показывает под именем ровно `entity_id` (`sub: eid`, `houseplan-card.ts:18722`). +Тип, который возвращают `_contactCandidates()`/`_lockCandidates()` +(`{ value: string; label: string }[]`), вообще не содержит поля `sub` — +значит, без явного решения реализация с равной вероятностью повторит любой +из двух образцов или не покажет `entity_id` в строке вовсе. -``` -const filtered = f ? list.filter(...) : list; -filtered.sort((a, b) => a.label.localeCompare(b.label)); // безусловно, даже без запроса (f === '') -return filtered.slice(0, 200); -``` +Это напрямую задевает AC2 («фильтр находит и по `entity_id`: запрос +`binary_sensor.win` находит сущность, чьё имя не содержит `win`») по духу, а +не только по факту: AC2 останется технически проверяемым смоком без этого +решения (можно кликнуть единственный оставшийся кандидат и проверить +сохранённое значение), но пользователь, ради которого A2 существует, увидит +находку без объяснения, почему она совпала — ровно то отличие, которое делает +`entity_id` в подписи полезным, а не декоративным. Учитывая, что `value` +контакта/замка уже и есть исходный `entity_id` (в отличие от `_bindingCandidates`, +где `value` — это `device:id`/`entity:id` с префиксом), техническое решение +дешёвое: `sub: c.value`, без изменения сигнатур `_contactCandidates()`/ +`_lockCandidates()`. -Сортировка по алфавиту применяется **безусловно**, независимо от того, пуст ли -запрос. Если реализация буквально повторит эту функцию для контакта/замка (что -и предлагает текст ТЗ, называя её образцом), результат без запроса окажется -отсортирован по алфавиту — а не «дверные первыми», как того явно требует AC6. -Второй существующий в проекте образец, `_runCandidates()` + рендер `:20981`, -устроен иначе: фильтр применяется **в момент рендера** над уже готовым -(единожды отсортированным) списком, без повторной пересортировки внутри -фильтра — именно эта форма сохраняет исходный порядок при пустом запросе. +**Что нужно поправить:** одно предложение в п.4.2 — какая из двух форм строки +берётся (рекомендация: подпись `entity_id` под именем, по образцу +`_roomSrcCandidates()`, поскольку именно `entity_id`, а не модель устройства, +и есть второй канал поиска по AC2). -ТЗ называет ровно тот образец, который ломает собственный AC6, и не -оговаривает, что реализация должна взять форму без пересортировки. Это не -продуктовый вопрос (человек как раз получает однозначное требование в AC6) — -это техническое противоречие внутри самого текста, которое реализация, -написанная «по образцу из п.2», воспроизведёт как дефект. Автор решает это -сам (`AGENTS.md`: то, чего пользователь не наблюдает как решение — можно -принять любым способом), но должен явно записать выбор, а не оставить два -взаимоисключающих образца рядом. +**Как воспроизвести неоднозначность:** взять текст ТЗ буквально и попросить +двух разных исполнителей написать рендер строки кандидата — получится два +разных, оба «по образцу из ТЗ». -**Что нужно поправить:** одно предложение в п.4 или блок «принято -предположительно»: новая функция фильтрации контакта/замка не должна -безусловно пересортировывать список — она либо фильтрует уже отсортированный -`_contactCandidates()`/`_lockCandidates()` без повторного `.sort()` (модель -`_runCandidates()`), либо сортирует **только** непустой результат фильтра, -оставляя пустой запрос как есть. +### [Medium, в скоупе] DoR требует явного заявления о влиянии на производительность — в ТЗ его нет -### [Medium, в скоупе] AC-раздел не указывает способ доказательства по каждому пункту +`PROCESS.md` §2.5 перечисляет обязательные пункты «Готово к разработке», +включая «влияние на производительность и бюджеты названо (или явно «нет»)» — +и это пункт из категории «все обязательны», трек `small` упрощает форму ТЗ +(§5: «проблема · контракт · AC1…ACn с доказательством · откат»), но не снимает +требований DoR при выходе из ревью в `S5-ready`. -DoR (`PROCESS.md` §2.5) требует у каждого AC явно названный способ -доказательства: `unit`/`backend`/`smoke`/`golden`/«ревью кода». Раздел 6 -перечисляет AC1…AC8 без этой пометки; раздел 7 «План тестов» описывает тесты -общо (юнит на функцию фильтрации, один смок, мутанты), но не привязывает -явно, какой AC каким пунктом доказывается. Большинство связей угадывается -(AC1–AC4, AC6, AC8 → юнит на чистую функцию фильтрации; AC5 → тот же юнит для -замка; AC7 → смок/проверка сохранённого конфига), но это должно быть написано, -а не восстановлено ревьюером — иначе на DoR это будет заново всплывать как -«не готово». +В тексте раздела 9 («Риски и откат») и раздела 8 («Release-артефакты») нет ни +слова о производительности — ни явного «нет», ни оценки. Мотивирующий сценарий +issue («сотни `binary_sensor` и `cover`») — это ровно тот случай, где +«очевидно, что дешёво» стоило написать явно: фильтрация подстрокой на каждое +нажатие клавиши без debounce по массиву в несколько сотен записей — тот же +порядок величины, что уже фильтруют `_bindingCandidates()`/`_roomSrcCandidates()` +сегодня без debounce, поэтому риска здесь по факту нет, но раздел, где это +следовало сказать, пуст. -**Что нужно поправить:** приписать к каждому AC1…AC8 в скобках способ -доказательства, например `AC1 (unit)`, `AC7 (smoke: demo/smoke_opening_entity_search.mjs)`. +**Что нужно поправить:** одна строка в разделе 9, например: «Производительность: +фильтрация подстрокой по тому же порядку кандидатов (десятки–сотни записей), +что уже обрабатывают `_bindingCandidates()`/`_roomSrcCandidates()` без debounce +— нового узкого места не создаёт». -### [Low] «Что человек увидит до и после» не выделено отдельным предложением +### [Low, к сведению] Второй живой образец `dropbtn`/`droppanel` не назван -§7.1 требует отдельно от «сценария» одну фразу без терминов реализации о том, -что человек видит до/после. Раздел 1 ТЗ смешивает сценарий, причину и намёк на -результат («минута прокрутки на каждый проём» — это «до»; отдельного «после» -нет, оно восстанавливается из AC). Не блокирует — не блокирую, так как раздел -6 (AC) фактически описывает «после» покритериально, но при правке по Medium- -находкам выше стоит добавить одну строку явно, чтобы не тратить время -следующего ревью на восстановление того же вывода. +Текст ТЗ (раздел 2) называет ровно один образец, который «не подходит +буквально» — `_bindingCandidates()`. Но безусловно пересортировывает список и +второй существующий `candlist`-виджет с идентичной разметкой, +`_roomSrcCandidates()` (`houseplan-card.ts:13901` там нет, ресорт на +`:18722`, вне ветки `q ?`, — то есть тот же дефект «сортирует, даже если запрос +пуст»). Вывод ТЗ (взять форму фильтрации `_runCandidates()`) от этого не +меняется — он верен для обоих контрпримеров, — но перечисление только одного +из двух даёт читателю неполную картину «почему». Не блокирует, снимаю +записью: автор может поправить текст или оставить как есть — вывод не +меняется независимо от того, назван ли один контрпример или оба. -## Что проверено и корректно +## Проверено и корректно -- Персона и job по `docs/SCOPE.md`: единственный, кто видит диалог проёма — Home - admin в Редакторе плана (десктоп-first поверхность); job — J4/J6 («держать - план актуальным», «GUI без внешних инструментов») — усложнение поиска входит - в тот же job, что и сам диалог. В скоуп попадает. -- Конфликта со `SCOPE.md` не найдено: не расширяет функциональность за пределы - UI выбора существующего значения, формат конфига не меняется (§4 п.6), - подтверждено — свойства `opening.contact`/`opening.lock` остаются как есть в - `OpeningCfg`. -- `passage` действительно не имеет и не получит контакта — подтверждено кодом, - а не только текстом ТЗ. -- Touch: заявлено «поддерживается» — обоснованно, потому что переиспользуемый - паттерн (`droppanel` с текстовым полем) уже несёт ту же тач-поверхность в - диалоге маркера; отдельного тач-риска не вносится, `TOUCH-SUPPORT.md` не - требует нового смока для точечного расширения существующего паттерна. -- i18n-план (7 существующих ключей + 1 новый, `opening.search_ph`, EN+RU) — - корректен, ни один из перечисленных существующих ключей не «придуман». -- «Не входит» — явный и обоснованный (правила отбора кандидатов, `ha-entity-picker`, - прочие селекторы вне диалога проёма). -- Откат — «одним revert, конфиг не меняется» — верно, чисто UI-правка, ничего - персистентного не создаётся. -- Release-артефакты (оба changelog, строка в `USER-GUIDE.ru.md`, скриншоты по - требованию `check-docs`) названы верно и по формату AGENTS.md/User-Visible: yes. -- Единственное число — источник: фича не добавляет никакой новой видимой - пользователю величины (только текстовый фильтр над существующим списком), - `test/single-source-numbers.test.mjs` не затрагивается по природе задачи. +- Все фактические ссылки ТЗ на код и i18n подтверждены (таблица выше) — + решение не построено на несуществующей опоре. +- **M1 предыдущего (сбойного) захода закрыт по существу.** ТЗ теперь явно + называет форму фильтрации без пересортировки (`_runCandidates()`) и явно + объясняет, почему буквальное копирование `_bindingCandidates()` сломало бы + AC6 (безусловный `.sort()`). Я перепроверил это заявление по коду независимо + (см. таблицу) — оно точное, а не пересказ автора. +- **M2 предыдущего захода закрыт.** Каждый AC1…AC8 несёт пометку `unit` + и/или `smoke`; раздел 7 (план тестов) соответствует этим пометкам по + содержанию (чистая функция фильтрации для unit, реальный диалог для smoke). +- **Low предыдущего захода закрыт.** Раздел 1 содержит отдельное предложение + «Что человек увидит после» без терминов реализации. +- Скоуп укладывается в `docs/SCOPE.md` (J4/J6 — GUI-настройка проёмов, + поддержание актуальности плана на больших инсталляциях), без конфликта с + «View mode is the product»: правка целиком внутри редактора плана, ничего + не протекает в Просмотр. +- Формат конфига не меняется (`opening.contact`/`opening.lock` — те же поля, + раздел 5 явно исключает изменение модели), поэтому `CONFIG-COMPATIBILITY.md` + не затрагивается — миграция не нужна, и ТЗ это не придумывает, а выводит из + раздела «Не входит». +- Touch: диалог общий для всех вводов, новый виджет — воспроизведение уже + работающего на touch паттерна (`_bindingCandidates`/`_roomSrcCandidates` + используют тот же `dropbtn`/`droppanel` без специальной touch-деградации) — + утверждение «поддерживается» не противоречит `docs/TOUCH-SUPPORT.md` + (best-effort редактора, без нового регресса). +- AC1–AC8 однозначны и проверяемы по отдельности; ни один не выдаёт догадку за + факт — там, где текст опирается на конкретный код (образец фильтрации, + наличие i18n-ключей), это подтверждено чтением, а не заявлено на веру. +- Release-артефакты названы верно: `docs/USER-GUIDE.ru.md` действительно имеет + подходящий раздел («Настройки проёма», строки 611–629) с описанием текущего + (безпоискового) выбора контакта — естественное место для правки. ## Чего не проверял -- Код не написан — не прогонялись `typecheck`/`test`/`build`/`check-docs`/смоки: - предмета нет, это этап ТЗ, а не код-ревью. -- Не проверял поведение `hp-dropdown`-паттерна на реальном тач-устройстве — - ссылка на «уже работает в диалоге маркера» принята по чтению кода и по - отсутствию отдельного тач-дефекта в истории по этому паттерну, не по живому - прогону. -- Не оценивал производительность на «сотнях» сущностей — паттерн уже несёт кап - 200 и используется на маркерах с сопоставимым объёмом сущностей; отдельного - риска не вижу, но не измерял. +- Реальную реализацию — её нет, это стадия ТЗ. +- `typecheck`/`test`/`build`/`check-docs`/смоки/инварианты модели — неприменимо + без кода; это не пропуск гейта, а корректная граница этапа. +- Не сверял поведение `_openingEntityAvailable()` (фильтр доступности сущностей + внутри `_contactCandidates`/`_lockCandidates`) построчно — оно не меняется + этим ТЗ и не упомянуто как предмет изменения, вне скоупа проверки ссылок. -## Вывод +## Вердикт -Изменение попадает в скоуп продукта (job J4/J6), ТЗ методологически полное -(все обязательные разделы присутствуют по существу), но содержит техническое -противоречие между заявленным образцом для переиспользования и собственным -AC6, плюс отсутствие явной привязки AC → способ доказательства. High-находок -нет. Оба Medium — в скоупе задачи, решаются правкой текста ТЗ (не кода), без -нового issue. Вердикт — жёлтый, возврат автору на правку текста. +Жёлтый. High: 0, Medium: 2 (оба в скоупе — правятся текстом ТЗ, без нового +issue), Low: 1 (к сведению, не блокирует). Заход r1 (переисполнение сбойного +автоматического прогона), блокирующих циклов израсходовано 0 из 2 до этого +вердикта.