Files
2026-09-26 08:37:45 +00:00

28 KiB
Raw Permalink Blame History

SPEC-REVIEW-662-r1

  • Issue: https://github.com/Matysh/houseplan-card/issues/662
  • Этап: S4-spec-review (ревью ТЗ, PROCESS.md §2.4)
  • Трек: полный. Автор явно назвал критерии §5, которые задача не проходит: новый UX-контракт (новый вид маркера с собственной геометрией и хит-тестом), более одной поверхности (три рендерера + два редактора + бэкенд), влияние на touch (новый тип цели касания «ломаная» во View/киоске — блокирующая зона TOUCH-SUPPORT), влияние на перф (линейный источник света: K свипов видимости на ленту) — разбор по существу, выбор полного трека корректен.
  • Материал: тело issue #662 в текущей редакции (после снятия blocked) + все 4 комментария: (1) аналитика/SCOPE/трек, (2) вопросы владельцу 1–6 пачкой, (3) ответы владельца 1–6 внесены в тело + новые вопросы 7–9 (привязка сразу после рисования; судьба значка размещённого устройства; объём правки геометрии), (4) решение владельца 7–9 внесено, blocked снят → S4-spec-review.
  • Заход: r1 · блокирующих циклов израсходовано 0 из 4
  • Роль: ревьюер ТЗ (не автор)

Скоуп ревью

Показ протяжённого источника света (LED-лента) как ломаной вдоль стен вместо значка в точке: геометрия рисуется в редакторе плана новым инструментом «LED-лента» (контракт цепочки стен), привязывается к обычному light.* маркеру через существующий диалог «Добавить устройство»; во View/киоске/ houseplan-space-card лента рисуется как капсульная полоса on/off/unavailable с хит-тестом по всей длине, участвует в fill «Свечение» как линейный источник (объединение вееров видимости), поднята в 2.5D по правилам плиток устройств. Бэкенд: схема led_strips, конфиг-инвариант «один маркер — не более одной ленты», per-space export/import, support package. Редактор устройств — диалог маркера без выбора вида; ленты не перетаскиваются там.

SCOPE-проверка (docs/SCOPE.md): прямая строка — J1 («room fills (light/temp/LQI)», «live spatial overview») и J7-смежная точность представления источника; сам автор в аналитике формулирует это как устранение конкретной лжи на плане («подсветка гарнитура длиной 3 м рисуется как лампа посреди столешницы»), что бьёт в основную работу продукта «взглянул и понял, что где горит». Из списка «никогда не строить» ничего не задевается: адресные эффекты/сегменты явно исключены в «Не скоуп»; редакторы затронуты только для расстановки — View остаётся продуктом для двух персон. Новой scope-дыры не вижу.

Как проверялось

  1. Прочитаны целиком docs/SCOPE.md, AGENTS.md, docs/process/REVIEWER.md и по ссылкам конспекта — PROCESS.md §7.1 (обязательные разделы, цепочка вопросов владельцу), §2.4, §2.5, §4, §7.2, §2.10 (не применялся — это r1).
  2. Прочитано тело issue #662 целиком (gh issue view 662 --json body) и все 4 комментария (gh issue view 662 --json comments) — история решений владельца воспроизведена в «Материал» выше.
  3. Сверены обязательные разделы §7.1: Сценарий, Что человек увидит до/после, Проблема, Скоуп, Контракт поведения (C1–C12), UX-тексты, Модель данных и миграция, Критерии приёмки AC1–AC14 с указанным способом доказательства (backend/unit/smoke/golden/ревью кода), План автотестов, Риски, Откат, Release-артефакты, «Принятые предположения» — почти все на месте; см. Находка 2 про «Не скоуп».
  4. Технические утверждения ТЗ построчно сверены с реальным кодом, а не приняты на слово:
    • resolveGlowAppearance, GLOW_FALLOFF, GLOW_FADE_MS = 500, glowAlpha — реальны (src/glow-scene.ts:46, src/logic.ts).
    • visibilityPolygon/splitAtIntersections — реальны (src/light-visibility.ts, src/glow-scene.ts); clipCache с LRU-лимитом — реален (src/glow-scene.ts:105-160).
    • device-hit-owner.ts существует, капсула + порог 44 px (cellSize = 44) — реальная константа (src/device-hit-owner.ts:165-167,292), пороги AC5 (20/30 px) согласуются с предложенным max(22 px, половина толщины).
    • glow-blend.ts (resolvedSvgScreenBlend, svgScreenBlendSupported) — реальный зонд деградации screen, используется в houseplan-card.ts/space-card.ts; .glow-pools уже несёт blend-screen/blend-normal на уровне всей группы пятен — вложенная isolation:isolate+lighten-группа для капсул одной ленты — новый, но технически состоятельный слой поверх существующего механизма (CSS compositing поддерживает вложенные группы), и риск деградации назван явно.
    • MarkupTool — реальный union-тип в houseplan-editor-runtime.ts:341 ('select' | 'draw' | 'column' | 'merge' | 'split' | 'resize' | 'opening' | 'wallthick' | 'delroom'); добавление литерала 'strip' — прямое расширение по образцу. Инструмент «Стены», описанный в аналитике как прецедент цепочки (тап добавляет точку, Shift — 45°, pointercancel/ второй палец/пинч/подавленный синтетический клик не добавляют), — дословно совпадает с docs/TOUCH-SUPPORT.md:151-162 и с реальными i18n-ключами markup.hint_start/markup.hint_points (src/i18n/ru.json:157-158) — ни один термин не выдуман.
    • docs/ISOMETRIC.md:307-328 — «Raised tiles», подъём 0.075 D — то же число, что и C9 предлагает переиспользовать для ленты (не новая константа).
    • MARKER_SCHEMA (custom_components/houseplan/validation.py:1826+) подтверждает все поля, которые C2 объявляет «не действуют» для маркера ленты (display, ripple_color, ripple_size, size, angle — строки 1941-1946) и все поля, которые «действуют» (hidden, tap_*, controls, is_light, light_entity, toggle_entity, glow_color, glow_radius_cm, value_badge/value_source, use_climate_temp) — ни одно имя поля не выдумано, а space уже существует как отдельное от layout поле маркера (строка 1835) — модель C1/C4 «маркер со space, но без layout» технически возможна уже сегодня, а не требует новой схемы.
    • Per-space export (_project_plan_only_space, _marker_owned в import_export.py:151-360) реально существует и фильтрует маркеры по положению — расширение на led_strips в C11 корректно называет реальный механизм, а не гипотетический.
    • AC14 называет только реально существующие документы (docs/DEVICE-PRESENTATION.md, docs/LIGHT.md, docs/DEVICE-LIGHT-SETTINGS-MATRIX.ru.md, docs/ARCHITECTURE.md — разделы «Device markers»/«Markup editor» существуют дословно под этими заголовками, docs/ISOMETRIC.md, docs/CONFIG-COMPATIBILITY.md, docs/TOUCH-SUPPORT.md); раздел «Linear sources» в LIGHT.md пока не существует — ТЗ корректно описывает его как новый, не выдаёт за существующий.
  5. Проверена внутренняя непротиворечивость числовых порогов: толщины 0,12 D/0,08 D, хит 22 px/половина толщины, выборка 100 см/≤8 точек, MAX_MARKERS=2000/MAX_POLY_POINTS=500 (validation.py:1198,1222) — лимиты ленты (2…50 точек, ≤50 лент) заметно консервативнее уже принятых потолков, конфликта нет.
  6. Проверена цепочка вопросов владельцу: пачка 1 (6 вопросов) и пачка 2 (3 вопроса) заданы с предложенным умолчанием по каждому пункту, ответы внесены в «Решения владельца» 1–9, blocked корректно ставился/снимался, открытых продуктовых вопросов не осталось.
  7. Прогнаны дешёвые гейты по требованию раздела гейтов: git diff origin/dev...HEAD --stat пуст, git status --short чист, git rev-parse HEAD = 27517db8a4b933e54368927851f76352567d5568 — точное совпадение с материалом. Гейты (tsc, test, build, смоки, golden, инварианты) неприменимы: продуктового кода для #662 ещё нет, стадия spec.
  8. Сверена классификация каталога устройств (src/device-inbox.ts:190-260) против модели C2/C4/C5/AC8 — см. Находку 1.

Находки

Один Medium (в скоупе, чинится автором без возврата на отдельный issue) и один Low (снят здесь же).

Medium-1. «Входящие» не существует в UI, и текущая классификация каталога кладёт неразмещённую ленту в другую вкладку

  • Файл/раздел: тело issue #662, ## ТЗ → «Модель данных и миграция» («Входящие» устройств его не показывают»), C5 («маркер остаётся в конфиге как неразмещённый — виден во «Входящих» устройств»), AC8 («Отвязать» → устройство во «Входящих»).
  • Что не так: реальные вкладки каталога устройств — ровно четыре: on_plan/available/hidden/readd (src/device-inbox.ts:13), в USER-GUIDE.ru.md:1081 они подписаны «На плане», «Доступны», «Скрытые», «Доступны снова». Вкладки «Входящие» не существует нигде — ни в коде, ни в docs/USER-GUIDE.ru.md, ни в i18n (grep "Входящ" src/i18n/ru.json находит только несвязанную строку backup.import_detail.dropped_links). Это прямое нарушение требования брать терминологию видимого поведения из docs/USER-GUIDE.ru.md, а не изобретать её (AGENTS.md «Read this first»).
  • Хуже того — вопрос не только в названии. Реальная классификация (src/device-inbox.ts:219-224) кладёт запись в on_plan, если существует live-маркер (запись в config.markers, не removed, не hidden) — независимо от наличия layout; в available запись попадает, только если live-маркера нет вовсе (см. ветку else if (candidate) category = 'available', куда управление не доходит, если live истинен). Маркер ленты, отвязанной или удалённой по C5 («маркер остаётся неразмещённым», запись config.markers не трогается, removed не проставляется), — это ровно случай «live, но без layout»: по действующему коду он попадёт на вкладку «На плане», а не «Доступны»/«Входящие», при этом визуально ничего не будет отрисовано (ни значка, ни ленты) — фантомная строка «на плане», которую нечем найти на самом плане.
  • Как воспроизвести (мысленный прогон по коду, не исполнение): led_strips[i].marker = null → маркер остаётся в config.markers с space, без layout, removed не установлен → в buildDeviceInboxRows (src/device-inbox.ts:200-224) liveByBinding его подхватывает → category = 'on_plan' (строка 222) → каталог покажет его во вкладке «На плане», хотя ни один рендерер (View/2.5D/space-card) ничего не рисует для маркера без layout и без активной ленты.
  • Почему Medium, не High: это не продуктовый вопрос (владелец решил поведение — «снова не размещено», п. 8–9 решений), а техническая недосказанность контракта: не хватает одного явного правила — например, «маркер со space, без layout и не referenced ни одной лентой, классифицируется как available» (меняет src/device-inbox.ts:219-224) — и правки термина «Входящие» → «Доступны» по всему ТЗ и AC8. И то, и другое — в пределах компетенции автора (техническое решение по PROCESS.md §7.1, «где хранится состояние… — решает автор»), без обращения к владельцу. Но пока это не зафиксировано явно, AC8 не может быть проверен: смок demo/smoke_led_strip_bind.mjs не будет знать, какую вкладку/строку утверждать, и разночтение всплывёт только на код-ревью — дороже, чем сейчас.
  • Что делать: заменить «Входящие» на «Доступны» (реальная вкладка) во всех трёх местах (модель данных, C5, AC8) и явно дописать в C5 или в «Принятые предположения», как классификатор device-inbox.ts отличает «маркер представлен лентой» (остаётся on_plan, это ожидаемо и корректно, пока лента жива) от «маркер осиротел после отвязки/удаления ленты» (должен переклассифицироваться в available, иначе он зависает фантомом на вкладке «На плане»).

Low-1. «Не скоуп» отсутствует внутри формального ## ТЗ

  • Раздел ## Не скоуп (кандидаты в отдельные issue) расположен до заголовка ## ТЗ, в аналитической части issue. §7.1 требует «скоуп и не-скоуп» как разделы самого ТЗ (раздел ## ТЗ, #517); внутри ### Скоуп под ## ТЗ пункта «не-скоуп» нет вовсе. Для сравнения — в #661 (тот же трек, тот же ревьюер-конспект) не-скоуп дан строкой прямо внутри ### Скоуп под ## ТЗ (SPEC-REVIEW-661-r1.md, «Скоуп/Не-скоуп разделены чётко»). Содержательно у #662 всё названо (сегменты/эффекты адресных лент, вставка/удаление вершин, высота крепления, произвольная форма гирлянды, тап в space-card) и совпадает с решениями владельца — вопросов к содержанию нет, только к месту. Почему Low: содержание присутствует и однозначно, ближайший заголовок над ним прямо озаглавлен «Не скоуп» и явно относится к этой же задаче — риск, что автор реализации или код-ревьюер его не найдёт, минимален. Снимаю без возврата на цикл; автору стоит при следующей правке текста (например, вместе с Medium-1) продублировать одну строку «Не скоуп: …» в ### Скоуп под ## ТЗ, чтобы раздел был самодостаточным.

Что проверено и корректно

  • Все обязательные разделы §7.1, кроме отмеченного в Low-1, присутствуют и в правильном порядке; Сценарий и Что человек увидит отвечают на «какая персона/поверхность/момент» и «что видно без терминов реализации».
  • Контракт C1–C12 самосогласован и почти полностью переиспользует существующие механизмы (см. «Как проверялось» п.4) вместо изобретения параллельных: геометрия ломаной — как у стен/комнат (coordinate- canonicalization.ts), хит-тест — новый тип цели в уже существующем device-hit-owner.ts, свечение — линейный кандидат в уже существующем glow-scene.ts/light-visibility.ts, привязка — существующий диалог «Добавить устройство», 2.5D — существующая формула поднятых плиток.
  • Каждый AC1–AC14, кроме AC8 (см. Medium-1), однозначен и называет способ доказательства (backend/unit/smoke/golden/ревью кода); граничные значения заданы числом по обе стороны порога (AC1: <2/>50 точек, >50 лент; AC5: 20 px → лента, 30 px → нет).
  • Модель данных: новый необязательный массив без миграции, политика неизвестных полей для старого фронтенда/бэкенда описана явно и совпадает с реальным поведением конфиг-загрузки; раздел docs/CONFIG-COMPATIBILITY.md назван в AC14 для формализации.
  • Поля маркера, которые лента «отключает» или «включает» (C2), построчно совпадают с MARKER_SCHEMA (см. «Как проверялось» п.4) — ни одно не выдумано.
  • i18n-таблица дана для ru/en с ключами по существующей конвенции (title.markup_<key>, markup.hint_<tool>_*), паритет de/fr обещан переводом; AC13 требует тест паритета по всем четырём словарям.
  • Риски называют главные технические неопределённости честно (перф линейного света, смешивание lighten/деградация, торец на диагоналях, новое состояние «маркер без layout» — что как раз материал Medium-1, автор его заметил, но не закрыл до уровня «AC можно проверить»), откат описан на двух уровнях и не противоречит правилу «не удалять файл/данные пользователя на догадку».
  • Мутанты (8 штук) целятся в конкретные защитные точки будущего кода (порог хита, деление радиуса пополам, обрезка по одной точке вместо веера, снятие lighten, сохранение layout при привязке, добавление точки при пане на touch, уникальность маркера на бэкенде, отрисовка непривязанной ленты) — реальный план для код-ревью, а не отчётность.
  • Продуктовые вопросы 1–9 заданы владельцу пачками с предложенным умолчанием каждый раз, ответы внесены в тело, blocked использовался и снят корректно — открытых продуктовых вопросов не осталось.

Чего не проверял

  • Гейты (npx tsc --noEmit, npm test, npm run build, node scripts/check-docs.mjs, смоки, golden:verify, model-invariants) — не прогонял: git diff origin/dev...HEAD --stat пуст, продуктового кода для #662 нет, стадия spec. Предмет код-ревью после реализации.
  • Не оценивал осуществимость самого механизма «капсулы lighten внутри изолированной группы + внешний screen» построчно (не писал прототип) — это новая, но правдоподобная композиция существующих CSS-примитивов (isolation, mix-blend-mode), явно отмеченная как риск с планом деградации; предмет код-ревью, не спек-ревью.
  • Не проверял реализуемость объединения вееров видимости из нескольких точек ломаной (visibilityPolygon сейчас строится из одной точки) построчно — плановое расширение помечено «принято предположительно, поменять свободно»; это код-ревью, не спек-ревью.
  • Не проверял иконку mdi:led-strip-variant на существование в used MDI-наборе карточки — декоративная деталь кнопки инструмента, не влияет на AC.
  • Не проверял продуктовую правомерность решений владельца 1–9 (ломаная вместо отрезка, видимость выключенной ленты, радиус вдвое, тап по всей длине, лента на полу в 2.5D, без ручек в v1, привязка сразу после рисования, судьба значка, объём правки геометрии) — прямые владельческие решения, ревьюер их не оспаривает, только сверяет соответствие контракта (см. «Как проверялось» п.6).

Вердикт

Обязательные разделы почти полны (один Low по месту, не по содержанию), контракт C1–C12 глубоко переиспользует существующие механизмы и не содержит догадок, выданных за факты, — каждое нетривиальное техническое утверждение сверено с реальным кодом и подтвердилось. Но один AC (AC8) и три места текста (модель данных, C5, AC8) называют несуществующую вкладку каталога устройств «Входящие» и, что важнее названия, не формулируют правило, по которому осиротевший маркер ленты (со space, без layout, не удалённый) должен попасть в available, а не зависнуть фантомом в on_plan по действующей логике device-inbox.ts. Это техническая недосказанность в пределах компетенции автора, не продуктовый вопрос — но без неё AC8 не проверяем. Возвращаю автору без отдельного issue (Medium в скоупе).

Вердикт: жёлтый · заход r1 · блокирующих циклов 1/4 · High: 0 · Medium: 1 → в задаче


Материал раунда

  • Ветка: dev, коммит 27517db8a4b9 — ребейз его осиротит, и это нормально: ниже якоря, которые ребейз не меняет.
  • Дерево материала: 8eaeacefa90fcaebfe8444496827be42e6e06012
    git log --all --format='%H %T' | grep 8eaeacefa90f
    
  • Тело issue: 960ef8ab4f944382a75391de287ebf208e2000449760a9c84c12793c820412c6
  • Вердикт конвейера: yellow · High 0