houseplan-card/docs/reviews/SPEC-REVIEW-203-r1.md

Ревью ТЗ — issue #203, цикл r1

• Этап: S4-spec-review (PROCESS.md §2.4)
• Артефакт ТЗ: docs/specs/203-hide-room-names.md, коммит 009fed9 на ветке issue/203-hide-room-names
• Issue: #203
• Ревьюер: Claude (роль «ревьюер ТЗ», отдельная сессия от аналитика/автора)
• Трек: обычный (не small) — верно и совпадает с меткой issue (bug, P2, S4-spec-review, без small); аналитика владельца прямо называет причину дисквалификации от лёгкого трека («затронуты два независимых renderer и визуальная регрессия»), что действительно нарушает критерий «одна поверхность» (§5 PROCESS.md) — и, как показано ниже, независимых мест на самом деле три, а не два.

Скоуп ревью

Оценивалось ТЗ docs/specs/203-hide-room-names.md целиком: наличие обязательных разделов §7.1 PROCESS.md, однозначность и доказуемость AC1–AC10, отсутствие догадок, выданных за решённый факт, соответствие docs/SCOPE.md (job J4/J6), docs/UX-MODES.md, docs/STYLING-HOOKS.md, docs/USER-GUIDE.ru.md, docs/TESTING.md, AGENTS.md (раздел Labs flags) и текущему коду (src/houseplan-card.ts, src/space-render.ts, src/logic.ts, src/labs.ts). Продуктовый код не менялся и не мог быть изменён (задача на этапе ТЗ).

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

1. Прочитаны docs/SCOPE.md, AGENTS.md, PROCESS.md целиком.
2. Прочитано тело issue #203 и оба комментария (аналитика владельца, хендофф автора ТЗ).
3. Прочитан весь текст ТЗ построчно, сверен с §7.1 (обязательные разделы) и §2.5 (DoR-чеклист).
4. Прочитан docs/USER-GUIDE.ru.md (строка настройки «Показывать названия» — «Рисует карточки/названия комнат; их можно двигать в редакторе плана», раздел «Настройки пространства») — подтверждено: текущая документация уже однозначно обещает «рисует или не рисует», без второго «статического» режима, что снимает продуктовую неоднозначность и оправдывает «открытых продуктовых вопросов нет» в ТЗ.
5. Прочитан docs/UX-MODES.md целиком, включая таблицу «What a space may choose not to draw» — подтверждён существующий прецедент (show_borders, hide_decor, hide_openings: display-only переключатель скрывает слой везде, кроме владеющего им редактора, «because a layer you cannot see is a layer you cannot edit») — предположение ТЗ №2 (§14, Plan editor держит подпись видимой) не изобретает новое правило, а копирует уже принятое.
6. Прочитан docs/STYLING-HOOKS.md §1–3 целиком — обнаружено, что таблица §3 документирует .rlabel/text («Room name, no metrics») как часть публичного и стабильного контракта («We promise the names in §3 are stable. If we ever have to change one, it is a breaking change and it goes in the changelog»).
7. Прочитан код всех причастных функций:
   • src/houseplan-card.ts:10910-10918 (_renderSvgRoomLabels) — подтверждено дословно: if (this._renderProjection === 'iso' || space.bg || disp.showNames || this._markup) return svg\`;— SVG-текст рисуется **только** когдаshowNames` ложно, без bg, без iso, вне Plan;
   • src/space-render.ts:345-352 (staticSvgLabels) — тот же !space.bg && !disp.showNames, независимая копия того же дефекта;
   • src/houseplan-card.ts:15825 — ${disp.showNames || (iso && !space.bg) || this._markup ? … _renderRoomLabel … : nothing} — подтверждено: HTML-карточка в изометрии форсируется условием iso && !space.bg независимо от disp.showNames; это третье, отдельное от двух SVG-веток место дефекта (см. находку Medium ниже);
   • src/houseplan-card.ts:1203-1205 (get _markup) — подтверждено: _markup === (this._mode === 'plan'), то есть Devices editor не входит в принудительное исключение и попадает под общий дефект наравне с View — расширение матрицы §6.1 на Devices editor не является новым продуктовым решением, это неизбежное следствие общего рендер-пути (_renderRoomLabel вызывается одинаково для view и devices);
   • src/logic.ts:1244 (spaceDisplayOf) — showNames: s.show_names ?? noPlan — подтверждено: explicit false читается как false, дефект не в сохранении;
   • src/houseplan-card.ts:13488 (_saveSpaceDialog, save-путь) — show_names: draw && d.mode === 'create' ? true : d.showNames — подтверждено: запись существующего пространства действительно пишет d.showNames без искажений;
   • src/houseplan-card.ts:10741-10760 (_spaceDisplayForRender) — подтверждено: пока открыт edit-диалог того же пространства, showNames берётся из черновика диалога — заявленный live-preview (§6.4 ТЗ) реален, а не гипотеза.
8. Прочитан AGENTS.md (раздел «Labs flags») и docs/ISOMETRIC.md — подтверждено, что iso — Labs-only эксперимент.
9. Прочитан src/labs.ts целиком (LABS_FLAGS, parseVersionCore, liveLabsFlags) и версия карты CARD_VERSION = '1.65.0-beta.4' (src/houseplan-card.ts:259) — вычислено и подтверждено исполнением (см. ниже), что запись iso (expires: '1.65.0') уже истекла на текущей версии: числовое ядро версии [1,65,0] не меньше числового ядра expires [1,65,0], значит compareVersion(current, expires) < 0 ложно и флаг не попадает в live.
10. Для проверки пункта 9 собран бандл (npm run build, копии в custom_components/houseplan/frontend/ и demo/srv/assets/) и запущен node demo/smoke_isometric_contract.mjs — прошёл (OK, isoRendered: true), но исходник смока (demo/smoke_isometric_contract.mjs:13-20) явно комментирует, что делает это в обход истёкшего Labs-флага: card._onLabsSnapshot({ active: ['iso'], space: '' }) напрямую, а не через реальный URL-резолвер (history.replaceState + hashchange там присутствуют, но фактическую активацию делает fixture-хук). Это подтверждает: обычный пользователь сегодня не может включить iso штатным способом (?hp-labs=iso) — ровно то, что предсказывает расчёт по labs.ts.
11. Проверены имена файлов, на которые ссылается план тестов (§10.2–10.3): demo/smoke_space_settings.mjs, demo/smoke_styling_hooks.mjs, demo/smoke_room_cards.mjs, demo/smoke_room_link.mjs, demo/smoke_isometric_contract.mjs, demo/smoke_isometric_live_touch.mjs — все существуют, имена не выдуманы.
12. Проверен docs/TESTING.md §1 (правила для новых тестов, включая правило 4 «тест, охраняющий механизм, сопровождается мутантом») и команда node scripts/mutation-gate.mjs --check, которую называет AC9, — совпадает с реальным инструментом.
13. Проверено, что файл ТЗ и запись в docs/specs/README.md добавлены одним коммитом 009fed9 с трейлерами Issue: #203 / User-Visible: no — корректно для документации без изменения поведения на этой стадии.

Код не менялся, чтения и точечного запуска существующего смока/сборки было достаточно: вопрос ревью ТЗ — «выполнимо и проверяемо ли», а не «работает ли реализация».

Проверено и корректно

• Все обязательные разделы §7.1 присутствуют: сценарий и персона (§1), что человек увидит до/после без терминов реализации (§2), проблема и подтверждённая причина (§3), scope/non-scope (§4–5), контракт поведения (§6), данные/совместимость/styling hooks (§7), UX/i18n/touch (§8), AC1–AC10 с доказательством (§9), план автотестов (§10), риски (§11), rollback (§12), release-артефакты (§13), явный блок принятых предположений (§14).
• Диагноз дефекта подтверждён построчным чтением кода, а не заявлен на веру — см. «Как проверялось» п.7: обе SVG-ветки, форсирующий iso && !space.bg, чтение/запись show_names и live-preview диалога совпадают с ТЗ дословно, включая нетривиальную деталь про Devices editor, которую ТЗ не называет отдельным продуктовым решением (и правильно — это прямое следствие общего кода, а не новая договорённость).
• Все AC пронумерованы, однозначны и несут явный способ доказательства (browser smoke / mutation gate / golden / diff документации) — выполняется требование DoR (§2.5 PROCESS.md).
• Non-scope точен и не пересекается с AC (§5): изменение текста переключателя, tooltip/диалоги/HA Area, сброс layout.rl_<roomId>, дефолт show_names для новых/legacy пространств (корректно отнесён к отдельному #204), schema/backend/migration/Optimize, публичное включение изометрии — каждый пункт реальная граница.
• Продуктовых вопросов владельцу нет, и это оправданно (см. «Как проверялось» п.4): формулировка настройки в docs/USER-GUIDE.ru.md уже однозначно описывает целевое поведение «рисует или не рисует», решать на продуктовом уровне действительно нечего.
• Технические решения корректно помечены как предположения (§14), включая нетривиальное и обоснованное прецедентом UX-MODES.md решение по Plan editor (п.2) и решение не создавать новый styling hook взамен удаляемого (п.4).
• Данные/миграция/откат корректны (§7, §12): формат settings.show_names не меняется, миграции нет, downgrade не теряет данные — подтверждено чтением spaceDisplayOf/_saveSpaceDialog.
• UX/i18n/touch раздел корректно пуст (§8): новых строк, узлов и touch-путей нет.
• AC9 корректно требует падающего мутанта для обеих SVG-веток (full и compact) с явным условием «два мутанта, если ветки независимы» — это ровно правило 4 docs/TESTING.md.

Находки

Medium (в скоупе задачи) — план тестов не даёт мутанта третьей, независимо ломающейся ветке (форсированная HTML-подпись в hidden iso)

Файл: docs/specs/203-hide-room-names.md, §10.3 («Golden и mutation gate»); связанный контракт — §6.1 (строка «Full View, hidden iso») и AC3.

Суть: ТЗ верно определяет, что дефект живёт в двух независимых SVG-fallback ветках (_renderSvgRoomLabels() в houseplan-card.ts и staticSvgLabels в space-render.ts) и требует по мутанту на каждую, «чтобы каждый renderer умел независимо покраснеть» (§10.3). Но у бага есть третье, столь же независимое место: src/houseplan-card.ts:15825 —

${disp.showNames || (iso && !space.bg) || this._markup
  ? space.rooms.map((r) => this._renderRoomLabel(r, space, view, disp))
  : nothing}

Здесь HTML-карточка .roomlabel форсируется условием iso && !space.bg независимо от disp.showNames — это ровно тот баг, который ТЗ описывает в §3 («Скрытая изометрическая ветка дополнительно форсирует HTML-карточки при iso && !space.bg, даже если showNames === false») и требует доказать через AC3. Но AC3 назначает доказательством только «Расширенный isometric contract/live smoke» — без mutation-гейта. Ветка синтаксически и по местоположению не связана ни с одной из двух веток, для которых мутанты уже запланированы: её можно случайно вернуть (например, при следующей правке изометрии) независимо от того, целы ли SVG-fallback'и, и browser-смок без сопровождающего мутанта не даёт гарантии, что он вообще способен упасть на этой конкретной регрессии (docs/TESTING.md, правило 4: «тест, охраняющий механизм, сопровождается мутантом»; правило 1: «наличие атрибута, класса или узла — не проверка поведения», а расширенный isometric-смок по описанию §10.2 ТЗ проверяет именно факт наличия/отсутствия карточки).

Почему это находка уровня Medium, а не Low: сама задача не пошла по лёгкому треку именно из-за «двух независимых renderer» (аналитика владельца) — то есть риск «один путь пофиксили, соседний остался» уже признан достаточно серьёзным, чтобы требовать полного ТЗ и мутантов. Третий путь несёт тот же риск и тот же исторический прецедент, который docs/TESTING.md прямо называет причиной своих правил («смок непрерывности не заметил удаления механизма, который защищает»). Это не блокирует выполнимость ТЗ и не требует решения владельца — правка целиком техническая и укладывается в уже существующий §10.3.

Как чинится: добавить в §10.3 третью запись мутационного гейта (например, hidden-iso-forces-room-label), возвращающую iso && !space.bg до (iso && !space.bg) независимо от showNames, и потребовать её red на чистом коде / green после исправления — по аналогии с уже описанными двумя. Альтернатива, тоже приемлемая: явно обосновать в ТЗ, почему для этой ветки достаточно browser-смока без мутанта (например, если реализация сведёт три условия к одному общему предикату — тогда мутантов на SVG-ветки хватит и на эту тоже, но это тогда стоит сказать явно, а не оставлять предположением читателя).

Решение: возврат автору, не более 4 циклов (§2.4/§4 PROCESS.md).

Low — истёкший Labs-флаг iso не упомянут, хотя ТЗ строит на нём AC3

Файл: docs/specs/203-hide-room-names.md, §6.1/§14 (упоминания «hidden iso»).

Суть: на текущей версии карты (CARD_VERSION = '1.65.0-beta.4', src/houseplan-card.ts:259) запись iso в src/labs.ts (since: '1.62.0', expires: '1.65.0') уже истекла — подтверждено расчётом и исполнением (см. «Как проверялось» пп.9–10): обычный пользователь не может включить изометрию через документированный ?hp-labs=iso, только через внутренний fixture-хук _onLabsSnapshot, которым уже пользуется существующий demo/smoke_isometric_contract.mjs. ТЗ описывает исправление в этой ветке как часть матрицы §6.1 без единого слова о том, что для реального пользователя сценарий сейчас недостижим и что тестирование пойдёт тем же обходным путём, что и существующие iso-смоки.

Почему не блокирует: это не меняет корректность AC3 и не создаёт новый продуктовый вопрос — исправление кода, который всё равно останется в бандле (и тестируется тем же приёмом, что и остальной hidden-iso код), не нарушает политику Labs из AGENTS.md («never extend expiry as an incidental change» касается продления жизни флага, а не починки бага внутри уже написанного, хотя и временно неактивного, кода). Это вопрос полноты объяснения, не корректности контракта.

Решение ревьюера: снимается без правки ТЗ, с записью в этом документе (разрешено §2.4/§3 PROCESS.md: «Low либо правится, либо снимается решением ревьюера с записью»). Рекомендация для реализации: одна строка в §14 или сноска к §6.1, что isometric-путь на момент задачи тестируется через _onLabsSnapshot-обход, как и существующие смоки — не требует отдельного цикла ревью.

Low — «затронутые файлы и модули» (DoR, §2.5) не собраны в один список

Файл: docs/specs/203-hide-room-names.md целиком.

Суть: DoR-чеклист (PROCESS.md §2.5) требует, чтобы «перечислены затронутые файлы и модули». В ТЗ эта информация присутствует и точна, но распределена: конкретные функции/файлы названы в §3 (диагноз) и §10.2/§13 (тесты и артефакты), а единого блока «Affected files» нет.

Почему не блокирует: по существу требование выполнено — каждый затронутый файл действительно назван хотя бы один раз, и ревьюер (см. «Как проверялось» п.7) не нашёл ни одного места дефекта, не упомянутого в тексте ТЗ. Это вопрос читаемости документа, а не полноты контракта.

Решение ревьюера: снимается без правки ТЗ, с записью в этом документе.

Других находок — Low, Medium или High — не выявлено.

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

• Не проверялась реализация — на этапе ТЗ её не существует; код-ревью будет отдельным циклом (S7-code-review) после написания кода.
• Не запускались npm test/npm run typecheck: код не менялся, гейты ТЗ не требуют их прогона на этом этапе (документация — класс C, продуктовый код не тронут). npm run build и один существующий смок (smoke_isometric_ contract.mjs) были запущены не как гейт приёмки ТЗ, а точечно — чтобы подтвердить или опровергнуть фактическое утверждение про истёкший Labs-флаг (находка Low выше); результат: смок зелёный, но подтверждает обход, а не штатную активацию.
• Не оценивались конкретные golden-сцены §10.3 (light/dark для «нарисованного пространства») — файлы фикстур не существуют на этапе ТЗ, план описывает сценарий содержательно, выбор конкретного пространства/файла — предмет реализации и код-ревью.
• Не оценивалась производительность на реальном большом плане — риск в §11 обоснованно сведён к «удаляются DOM/SVG узлы, новых проходов/таймеров нет», что проверяется код-ревью, а не спецификацией.
• Не проверялся текст будущих записей changelog (docs/CHANGELOG.md/.ru.md) — они не написаны на этапе ТЗ; §13 корректно называет оба файла как обязательные при User-Visible: yes.

Вердикт

Обязательные разделы на месте, диагноз дефекта подтверждён построчным чтением кода (включая нетривиальные детали: Devices editor не является отдельным продуктовым решением, Plan editor опирается на существующий прецедент UX-MODES.md, живой preview диалога — реальный код, а не гипотеза), продуктовых вопросов к владельцу нет и это обоснованно. Единственная находка, требующая правки, — Medium в скоупе задачи: план мутационных тестов покрывает две из трёх независимо ломающихся веток одного и того же бага; третья (форсирование HTML-подписи в hidden iso, houseplan-card.ts:15825) осталась без мутанта, хотя её собственный смок описан только как проверка наличия/отсутствия DOM-узла. Без High-находок это жёлтый вердикт: автор дополняет §10.3 и переходит повторный цикл.

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