27 KiB
SPEC-REVIEW-662-r2
- Issue: https://github.com/Matysh/houseplan-card/issues/662
- Этап:
S4-spec-review(ревью ТЗ, PROCESS.md §2.4) - Трек: полный (подтверждено в r1: новый UX-контракт, более одной поверхности, влияние на touch и на перф — критерии §5 названы автором явно и разобраны по существу).
- Материал: тело issue #662 в текущей редакции (после комментария 6, снявшего Medium-1/Low-1 из r1) + все 7 комментариев, включая вердикт r1 (комментарий 5) и правки автора (комментарий 6).
- Заход: r2 · блокирующих циклов израсходовано 1 из 4 (потрачен r1; зелёный вердикт этого захода бюджет не увеличивает, PROCESS.md §4)
- Роль: ревьюер ТЗ (не автор)
Скоуп ревью
Без изменений с r1: показ протяжённого источника света (LED-лента) как
ломаной вдоль стен вместо значка в точке. Геометрия рисуется в редакторе
плана инструментом «LED-лента» (контракт цепочки стен), привязывается к
обычному light.* маркеру через существующий диалог «Добавить устройство»;
во View/киоске/houseplan-space-card — капсульная полоса on/off/unavailable с
хит-тестом по всей длине, линейный источник в fill «Свечение», поднята в 2.5D.
Бэкенд: схема led_strips, инвариант «один маркер — не более одной ленты»,
per-space export/import, support package.
SCOPE-проверка не переоткрывается: r1 уже сверил задачу с J1/J7 и «никогда не строить» — ничего не изменилось (правки r1→r2 чисто технические, скоуп не трогают).
Закрытие раунда r1
| Находка (r1) | Чем закрыта | Где это видно |
|---|---|---|
Medium-1. «Входящие» — несуществующая вкладка каталога; текущая классификация (device-inbox.ts:219-224, else if (runtime || live) category = 'on_plan' — независимо от layout) кладёт осиротевший после отвязки/удаления ленты маркер в on_plan как фантом без отрисовки |
Термин «Входящие» убран из ТЗ целиком (проверено — grep -c "Входящ" body = 0). Контракт переписан под реальную модель вместо переклассификации: маркер, представленный лентой, — обычный live-маркер (on_plan/visible_explicit); после «Отвязать»/«Удалить ленту» он снова фактически отрисовывается значком — на layout, если он сохранился, либо по авторасстановке (_defaultPositions) в комнате своей области; фантома не остаётся, потому что рисовать действительно есть что |
Тело issue: «Решения владельца» п.8; ## ТЗ → C2 («Запись layout … не читается и не требуется, пока представлен лентой… в авторасстановке не участвует»), C4 («при привязке размещённого устройства layout не трогается»), C5 («маркер остаётся в config.markers без изменений и снова представлен значком: на сохранённой позиции… либо по авторасстановке… Никакого состояния «не размещено» не возникает»), «Модель данных и миграция», AC8 (переписан), AC19 (новый, закрывает каталог+авто-сетку явно); комментарий 6 |
Low-1. Раздел «Не скоуп» лежит до ## ТЗ, а не внутри ### Скоуп |
Строка «Не скоуп: …» продублирована внутри ### Скоуп, развёрнутый список кандидатов оставлен выше как справочный |
Тело issue, ## ТЗ → ### Скоуп, последний абзац: «Не скоуп: сегменты и эффекты адресных лент; вставка/удаление отдельных вершин; высота крепления…»; комментарий 6 |
Обе находки закрыты по существу, не только по формулировке — Medium-1 особо:
автор не подогнал термин, а привёл контракт к реальному коду device-inbox.ts
(проверено чтением, см. ниже).
Унаследовано из r1 (не проверялось повторно)
- Обязательные разделы §7.1 (Сценарий, Что человек увидит, Проблема, Контракт
C1–C12, UX-тексты, Модель данных, План автотестов, Риски, Откат,
Release-артефакты, Принятые предположения) присутствуют и в верном порядке —
SPEC-REVIEW-662-r1.md, «Как проверялось» п.3, дерево материала8eaeacefa90fcaebfe8444496827be42e6e06012. - Построчная сверка технических терминов C1–C12 с реальным кодом
(
resolveGlowAppearance,GLOW_FALLOFF,GLOW_FADE_MS=500,visibilityPolygon/splitAtIntersections,clipCache,device-hit-owner.tscellSize=44,MarkupToolunion, MARKER_SCHEMA поля, per-space export,docs/ISOMETRIC.md0.075 D) —SPEC-REVIEW-662-r1.md«Как проверялось» п.4; текст этих пунктов между r1 и r2 не менялся (правки — только C2/C4/C5/ модель данных/AC8, см. таблицу выше), пере-проверка не требуется. - Продуктовые вопросы 1–9 и решения владельца по ним —
SPEC-REVIEW-662-r1.md«Как проверялось» п.6; не пересматривались. - Внутренняя непротиворечивость числовых порогов (толщины, хит-радиус,
MAX_MARKERS/MAX_POLY_POINTS) —SPEC-REVIEW-662-r1.mdп.5; новые пороги C13 (эпсилон грани = порог магнита мебели) проверены заново ниже, поскольку относятся к неразобранному материалу.
Что нового в r2 и как проверялось
Ключевой момент этого захода: решение владельца 10 (C13, AC15–AC18) в r1 не разбиралось — автор сам отметил это в комментарии 6 («Ревью r1 разбирало редакцию до решения 10»), и материал r1 подтверждает это документально («Как проверялось» п.4 не упоминает C13; «Находки» тоже). Это не точечная правка внутри уже проверенного текста, а целый непроверенный раздел контракта (стены: упор при рисовании, односторонний свет от грани, поведение внутри утолщённой стены) — по правилу §2.10 «дельта не локальна», разбираю его полностью, а не по диффу к r1.
- C13, тело стены при рисовании/перетаскивании (AC15, AC17). Сверено с
docs/LIGHT.md«What stops light»: контурные стены с толщиной (wallBodiesGeometry), независимые перегородки/колонны и окна — опорная масонри-геометрия; проёмыdoor|gate|passage— разрывы в НЕЙ по геометрии, а не по состоянию двери. Это подтверждается реальным кодом: комментарий вsrc/houseplan-card.ts:10390-10397(«Exterior opening tunnels deliberately remain inmasonryGeometry… a valid interior doorway remains transparent») —masonryGeometry/opaqueBodies, на которых строится проверка «источник внутри тела» (#92,glowSourceInOpaqueBody), действительно не зависят от текущего состояния двери; динамика (закрытая дверь = дополнительныйoccluder) живёт отдельно, вrevision/resolveLightBarrierRevision, и не участвует в определении «тела» для #92 и для упора при рисовании. C13 корректно называет именно эту, статичную, часть — «без учёта состояния двери» здесь не ошибка, а точное описание того, какая геометрия используется для упора пера и для проверки «точка внутри тела».furnitureWallSurfacesFor(src/furniture-wall-surface.ts:161) реально существует и возвращает физические грани — снап ленты к ним, а не к осям, для стен с толщиной технически обоснован. - C13, свет от грани (AC16). Раздел отдельно и правильно вводит
стилезависимость нулевых стен (Solid — барьер для лучей, кроме
самопересечения точки, лежащей на нём; Dashed — барьера нет вовсе), что
дословно совпадает с
docs/LIGHT.md«Opaque»/«Transparent» («every zero-thickness wall when its space uses the Solid style» / «…Dashed style»). Это не противоречит первому пункту (толстые стены, двери, окна) — тело для рисования/#92 и барьер для луча описаны раздельно и оба привязаны к реально существующим механизмам (_lightBarriers,buildLightBarrierScene,resolveZeroWalls), а не изобретены. Формулировка вводного абзаца C13 («Телом стены считается то же, что для света») по первому прочтению читается как единое определение на оба подраздела; при внимательном чтении вместе с самим текстом «Свет от грани» она не противоречит реальному коду — но неопытный исполнитель без чтения кода рискует принять «без учёта состояния двери» за правило и для лучевой развёртки тоже. Это стилистический риск понимания, не фактическая ошибка контракта (реальный код и оба абзаца C13, если читать их вместе, согласованы) — не поднимаю до Medium, но фиксирую как наблюдение ниже. - AC16 не содержит сценария с дверью/проёмом для лучевой развёртки самой
ленты (только толстая стена и ось/пересечение нулевой стены). Поскольку
развёртка ленты, по аналитике и C7, использует ту же цепочку
visibilityPolygon/кэш ревизии, что и обычные лампы — то же самое поведение около двери уже покрыто существующими тестами точечных источников и не нуждается в дублирующем AC для ленты; отсутствие такого кейса в AC16 не оставляет её непроверяемой. Даю это как наблюдение (см. ниже), не как находку. - AC15 / «упор в стену». Сверено построчно с описанием: сквозь толстую
стену → упор на грани; вдоль грани → допустимо; сквозь нулевую стену любого
стиля → допустимо; сквозь
door/gate/passage→ допустимо; сквозь окно → упор; точка внутри тела → не принята. Все шесть исходов совпадают с телом, определённым в C13 п.1, ни один не выдуман; прецедент упора («упирается в первую небезопасную позицию») — реальный приём, уже используемый изменением размера стен (аналитика ссылается на него по памяти, не проверял построчно кодresize— не требуется: сам приём для AC15 достаточно описан текстом ТЗ, реализация — код-ревью). - Мутанты C13 (
strip-wall-crossing-allowed,strip-face-offset-off,strip-inside-wall-emits) целятся точно в защитные точки AC15/AC17/AC16 (снятие упора, снятие сдвига от грани, эмиссия из точки внутри тела) — план код-ревью полон, «пустого третьего столбца» по существу нет: сама ТЗ называет мутацию для каждого нетривиального инварианта C13. - Каталог/авто-сетка (Medium-1 fix).
src/device-inbox.ts:219-224(else if (runtime || live) category = 'on_plan') иdocs/USER-GUIDE.ru.md:1088(«Автоматически найденный маркер уже относится к «На плане», даже если для него ещё нет сохранённой записи marker») подтверждают дословно новую формулировку C2/C5: живой маркер безlayout— не фантом, а обычное «На плане» с авторасстановкой. Дочитано_defaultPositions/_livePos(src/houseplan-card.ts:5188-5246): маркер безsaved-записи (или чейsaved.sне совпадает с текущимd.space) получает позицию из_defPos— авторасстановка реально размещает его в комнате его области, а не оставляет невидимым. Проверено чтением, не исполнением. - Терминология каталога («На плане», «Доступны», «Скрытые», «Доступны
снова») сверена построчно с
docs/USER-GUIDE.ru.md:1081— совпадает дословно; «Входящих» не осталось нигде (grep -c "Входящ"по телу issue = 0).
Наблюдение (не находка, не требует правки)
Вводный абзац C13 («Телом стены считается то же, что для света… без учёта
состояния двери») формально относится и к упору при рисовании, и к свету от
грани, хотя реально в коде это две разные вещи: статичная масонри-геометрия
(без учёта состояния двери — для обоих подразделов корректно) и динамический
список барьеров лучевой развёртки (occluders, зависит от состояния двери —
именно он гасит свет позади закрытой двери для ЛЮБОЙ лампы, включая ленту,
через общий _lightBarriers()). Текст C13 не ошибается по существу (я сверил
оба чтения с кодом и они совпадают), но чтение «в лоб» способно навести
исполнителя на мысль, что лента как источник света игнорирует состояние
двери целиком. Автору стоит при следующей технической правке (не обязательно
в этом раунде) явно развести «тело» (масонри, статично) и «барьер развёртки»
(occluders, по состоянию) одним предложением — но это редакционная
подстраховка, не блокер: код-ревью проверит фактическое поведение AC16 против
реальных дверей, и общий механизм _lightBarriers() уже даёт правильный
результат без специального кода.
Как проверялось
- Прочитаны
docs/SCOPE.md,docs/process/REVIEWER.md,AGENTS.md(уже знакомые из r1, но заново — конспект мог измениться; изменений с r1 не обнаружено). - Прочитано тело issue #662 целиком в текущей редакции и все 7 комментариев, включая вердикт r1 и правки автора.
- Восстановлен и прочитан
SPEC-REVIEW-662-r1.mdиз коммита422221fe(git show 422221fe:docs/reviews/SPEC-REVIEW-662-r1.md) — материал и находки предыдущего раунда. git diff origin/dev...HEAD --stat— пусто;git status --short— чисто;git rev-parse HEAD=422221fe9342c88d35ab389515960ba05552ea86— точное совпадение с материалом ревью. Продуктового кода для #662 по-прежнему нет (стадияspec), гейты (tsc,test,build, смоки, golden, инварианты) неприменимы по той же причине, что в r1.- Закрытие Medium-1 проверено не на слово: прочитан
src/device-inbox.ts(классификацияon_plan/available, строки 189-260) иsrc/houseplan-card.ts(_defaultPositions,_livePos, строки 5188-5246) — реальный код подтверждает, что живой маркер безlayoutполучает авто-позицию и реально отрисовывается, а не зависает фантомом. - Закрытие Low-1 проверено чтением текущего тела issue — строка «Не скоуп»
присутствует внутри
### Скоуп. - Полностью разобран C13/AC15-18 (см. «Что нового в r2» выше): сверено с
docs/LIGHT.md«What stops light»,src/houseplan-card.ts(_lightBarriers,glowSourceInOpaqueBody, комментарий проmasonryGeometryиoccluders),src/furniture-wall-surface.ts(furnitureWallSurfacesFor). - Сверена терминология каталога с
docs/USER-GUIDE.ru.md:1081,1088. grep -c "Входящ"по телу issue — 0 вхождений, подтверждает полное устранение термина.
Находки
Нет ни одной находки (High/Medium/Low). Единственное замечание — стилистическое наблюдение выше, снятое без действия: оно не влияет ни на один AC и не меняет поведение.
Что проверено и корректно
- Обе находки r1 (Medium-1, Low-1) закрыты по существу, не косметически:
контракт приведён к реальной модели
device-inbox.ts/_defaultPositions, а не подогнан под старое поведение под новым именем. - Ранее непроверенный материал (C13, AC15–AC18, мутанты
strip-wall-crossing-allowed/strip-face-offset-off/strip-inside-wall-emits) самосогласован, ссылается только на реально существующие механизмы (wallBodiesGeometry,furnitureWallSurfacesFor,_lightBarriers,glowSourceInOpaqueBody,docs/LIGHT.md) и не содержит догадок, выданных за факт. - AC8/AC19 однозначны и проверяемы: явно называют вкладку каталога и правило авто-сетки, смок и unit-тест из «Плана автотестов» знают, что утверждать.
- Мутанты по-прежнему по одному на каждый нетривиальный инвариант нового раздела C13 — пустых защитных AC без «чем краснеет» не осталось.
- Терминология видимого поведения (каталог, вкладки) берётся из
docs/USER-GUIDE.ru.md, а не изобретается — выполнено по всему телу issue.
Чего не проверял
- Гейты (
npx tsc --noEmit,npm test,npm run build,node scripts/check-docs.mjs, смоки,golden:verify, инварианты модели) — не прогонял: диф кdevпуст, продуктового кода для #662 нет, стадияspec. Предмет код-ревью после реализации. - Не проверял построчно код
resize-инструмента стен на предмет точного совпадения приёма «упирается в ближайшую грань» с тем, что потребуется для ленты, — сам приём достаточно описан текстом C13/AC15 для целей спек-ревью; точное соответствие реализации — код-ревью. - Не проверял реализуемость объединения вееров видимости из нескольких точек ломаной построчно (по-прежнему код-ревью, не спек-ревью, как и в r1).
- Не переоценивал продуктовые решения владельца 1–10 — не оспариваю их, сверяю только соответствие контракта (как в r1).
- Не проверял правку C4 про кросс-пространственную переустановку
spaceу уже размещённого устройства (частный случай, не покрытый отдельным AC): по коду (_livePos) при несовпаденииsaved.sс новымd.spaceпозиция корректно откатывается на авторасстановку — деградация безопасна, но нет выделенного AC для этого конкретного пути; не поднимаю до находки, так как описанное в контракте поведение («как любой маркер безlayout») уже покрывает этот случай по смыслу, а не только для общего случая.
Вердикт
Обе находки r1 закрыты не косметически, а по существу — проверено чтением реального кода классификации каталога и авторасстановки, а не принято на слово. Материал, который r1 физически не успел разобрать (решение владельца 10 — стены, C13, AC15–AC18), разобран полностью в этом раунде: контракт самосогласован, ссылается только на существующие механизмы, каждый защитный AC называет мутацию. Находок нет; одно стилистическое наблюдение снято без действия, так как не влияет на проверяемость ни одного AC.
Вердикт: зелёный · заход r2 · блокирующих циклов 1/4 · High: 0 · Medium: 0
Материал раунда
- Ветка:
dev, коммит422221fe9342c88d35ab389515960ba05552ea86. - Issue: https://github.com/Matysh/houseplan-card/issues/662, тело в редакции после комментария 6 (26.09), все 7 комментариев учтены.
- Предыдущий документ:
docs/reviews/SPEC-REVIEW-662-r1.md(материал r1: дерево8eaeacefa90fcaebfe8444496827be42e6e06012, тело — редакция до решения 10, вердиктyellow, High 0, Medium 1).
Материал раунда
- Ветка:
dev, коммит422221fe9342— ребейз его осиротит, и это нормально: ниже якоря, которые ребейз не меняет. - Дерево материала:
c4e3f48a513e0468285959a11f937aa9b13c9754git log --all --format='%H %T' | grep c4e3f48a513e - Тело issue:
f20b5d07de0349a2c8bea21863514e70645bea367de0f23797805ec10b577f7a - Вердикт конвейера:
green· High 0