diff --git a/docs/reviews/SPEC-REVIEW-203-r2.md b/docs/reviews/SPEC-REVIEW-203-r2.md new file mode 100644 index 00000000..19a685b7 --- /dev/null +++ b/docs/reviews/SPEC-REVIEW-203-r2.md @@ -0,0 +1,230 @@ +# Ревью ТЗ — issue #203, цикл r2 + +- Этап: `S4-spec-review` (PROCESS.md §2.4) +- Артефакт ТЗ: [`docs/specs/203-hide-room-names.md`](../specs/203-hide-room-names.md), + правка коммитом `f7b811a` поверх `009fed9` на ветке `issue/203-hide-room-names` +- Issue: [#203](https://github.com/Matysh/houseplan-card/issues/203) +- Ревьюер: Claude (роль «ревьюер ТЗ») +- Трек: обычный (не `small`), лимит цикла — 4 (§4 PROCESS.md); это второй цикл +- Предыдущий цикл: [`SPEC-REVIEW-203-r1.md`](SPEC-REVIEW-203-r1.md) — жёлтый, + High: 0, Medium: 1 (в скоупе) + +## Скоуп ревью + +Полный повторный разбор `docs/specs/203-hide-room-names.md` — не только +дельта коммита `f7b811a`, а весь документ целиком, как того требует §2.4 +(«ревьюер получает issue и ТЗ, без устных пояснений автора»): обязательные +разделы §7.1 PROCESS.md, однозначность и доказуемость AC1–AC10, отсутствие +догадок, выданных за факт, соответствие `docs/SCOPE.md` (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`, `scripts/mutation-gate.mjs`). Дополнительный фокус цикла — +устранение Medium-находки r1 (третье независимое место дефекта, forced HTML +в hidden iso, без сопровождающего мутанта). Продуктовый код не менялся между +r1 и r2 (`git diff 009fed9..HEAD -- src/` пуст) — верно для стадии ТЗ. + +## Как проверялось + +1. Перечитаны `docs/SCOPE.md`, `AGENTS.md`, `PROCESS.md` целиком заново (не + по памяти предыдущего цикла). +2. Перечитано тело issue #203 и все четыре комментария, включая хендофф + автора о правке r1 (`f7b811a`, «AC3 теперь требует отдельное мутационное + доказательство hidden iso, а §10.3 фиксирует три независимых + entries/guards»). +3. Прочитан весь текст ТЗ построчно заново, сверен с §7.1 (обязательные + разделы) и §2.5 (DoR-чеклист) — все присутствуют, без регресса после + правки. +4. Прочитан точечный diff `git show f7b811a`: правка ограничена AC3 (строка + таблицы AC) и §10.3 (таблица мутантов); остальной текст не тронут. +5. Проверено содержательное соответствие правки находке r1 построчным чтением + актуального кода: + - `src/houseplan-card.ts:10910-10919` (`_renderSvgRoomLabels`) — условие + `this._renderProjection === 'iso' || space.bg || disp.showNames || + this._markup` дословно совпадает с описанием мутанта + `hidden-room-names-full-svg-fallback`; + - `src/space-render.ts:345-352` (`staticSvgLabels`) — условие + `!space.bg && !disp.showNames` дословно совпадает с + `hidden-room-names-compact-svg-fallback`; + - `src/houseplan-card.ts:15825` — `disp.showNames || (iso && !space.bg) || + this._markup` дословно совпадает с новым третьим мутантом + `hidden-room-names-iso-override`; это ровно та строка, которую r1 указал + как непокрытую. +6. Проверена **техническая реализуемость** нового мутанта, а не только текст + ТЗ: каждый из трёх якорей (`grep -c`) встречается в соответствующем файле + ровно один раз — требование `scripts/mutation-gate.mjs` («`find` обязан + встречаться в файле ровно один раз») выполнимо для всех трёх записей, а не + только продекларировано. +7. Прочитан `scripts/mutation-gate.mjs` (шапка и существующие записи реестра, + например `empty-space-cleanup-disabled` с `guard: 'node + demo/smoke_optional_space_model.mjs'`) — подтверждено, что предложенная + форма мутанта (id · guard = существующий/расширяемый browser-smoke · + патч, ломающий один анкер) соответствует принятому в проекте шаблону, а не + изобретает новый. +8. Перепроверены сами SVG/HTML fallback-ветки на отсутствие иных, ещё не + учтённых мест того же дефекта: `grep -rn "rlabel" src/*.ts` даёт ровно два + определения (`houseplan-card.ts`, `space-render.ts`) плюс CSS в + `styles.ts`; других мест создания `.rlabel`/принудительного показа + `.roomlabel` в источниках нет — после правки r1 все известные независимые + места действительно покрыты, четвёртого не обнаружено. +9. Прочитан `docs/TESTING.md` §«Правила для новых тестов» (правила 1, 3, 4) — + подтверждено, что новая формулировка AC3 («+ отдельный iso mutant») + закрывает именно тот риск, который правило 4 называет («тест, охраняющий + механизм, сопровождается мутантом»), и что DOM-присутствие/отсутствие + само по себе (правило 1) для всех трёх AC теперь подкреплено мутантом, а + не голым ассертом. +10. Проверено текущее содержимое существующих iso-смоков + (`demo/smoke_isometric_contract.mjs:28`, `demo/smoke_isometric_live_touch.mjs:25`) + — оба сейчас жёстко ставят `show_names: true` и не покрывают `false`; + подтверждено, что план §10.2 п.5 («повторить ключевую проверку в hidden + iso») действительно требует нового покрытия, а не переиспользования уже + существующего ассерта, то есть AC3 не завышает готовность. +11. Заново прочитан `docs/UX-MODES.md`, раздел «What a space may choose not to + draw» — таблица трёх «display-only» переключателей (`show_borders`, + `hide_decor`, `hide_openings`) не упоминает `show_names`; см. находку Low + ниже. +12. Заново прочитан `docs/STYLING-HOOKS.md` §1 и §3 — подтверждена + самообязывающая формулировка «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»; `grep -rn "rlabel" src/*.ts` подтверждает, что строка + `.rlabel` в §3 не имеет ни одного легитимного (не багового) источника — + удаление строки корректно, см. находку Low ниже про формулировку + changelog-записи. +13. Перечитан `docs/USER-GUIDE.ru.md`, строка «Показывать названия» — текст + не требует правки: он не обещает второго «статического» режима и + остаётся честным после исправления. +14. Проверено существование всех файлов, на которые ссылается план тестов: + `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` + — все существуют. +15. Проверено, что оба Low из r1 (устаревший Labs-флаг `iso`, несобранный в + один блок список затронутых файлов) не требовали правки ТЗ — они были + сняты решением ревьюера r1 без возврата автору, и в правке `f7b811a` + корректно не тронуты. + +Код не менялся, поэтому повторной сборки/прогона гейтов для этого цикла не +требовалось — вопрос ревью ТЗ остаётся «выполнимо и проверяемо ли», а не +«работает ли реализация». Точечные `grep`/чтение кода использовались только +для проверки фактических утверждений документа. + +## Проверено и корректно + +- **Medium-находка r1 устранена полностью и точно**: третье независимое место + дефекта (`iso && !space.bg` override на `houseplan-card.ts:15825`) теперь + имеет собственный мутант `hidden-room-names-iso-override` с явным guard + (isometric contract/live smoke из AC3), список из трёх мутантов + зарегистрирован в §10.3 отдельными строками с явным запретом объединять их + в один — ровно то исправление, которое просил r1, без побочных изменений + контракта. +- Все три анкера дефекта уникальны в исходном коде (проверено `grep -c`) — + план мутантов не просто текстуально корректен, а технически реализуем под + правило `scripts/mutation-gate.mjs` «`find` встречается ровно один раз». +- Правка ограничена ровно двумя точками документа (AC3, §10.3); остальной + текст, ранее признанный корректным в r1 (сценарий, матрица видимости, + scope/non-scope, риски, rollback, release-артефакты, блок предположений), + не пострадал и не требует повторной валидации по существу — но был + перечитан целиком (см. «Как проверялось» пп.3, 11–13) и по-прежнему + корректен. +- Все обязательные разделы §7.1 присутствуют, AC1–AC10 однозначны и несут + явное доказательство; в частности AC3 после правки формулирует «browser + smoke **+** мутант» вместо одного лишь смока. +- Диагноз дефекта (три независимых места) подтверждён самостоятельно, заново, + чтением исходников — не принят на веру из r1 или из текста ТЗ. +- Продуктовых вопросов владельцу по-прежнему нет и это обоснованно: ни одна + из находок этого цикла не требует решения о том, что видит или делает + пользователь — обе ниже являются вопросами полноты документации уже принятых + решений. + +## Находки + +### Low — таблица «What a space may choose not to draw» в `docs/UX-MODES.md` не получает строку `show_names` + +**Файл:** `docs/UX-MODES.md`, раздел «What a space may choose not to draw» +(строки 112–124); связано с `docs/specs/203-hide-room-names.md` §13 +(Release-артефакты). + +**Суть:** после этого исправления `show_names` начинает вести себя ровно по +правилу, которое этот раздел документирует как канон для трёх других +переключателей — «display only… each layer stays visible in the editor that +owns it, because a layer you cannot see is a layer you cannot edit» +(`show_borders`, `hide_decor`, `hide_openings`). Название комнаты — четвёртый +такой слой (виден в Plan editor через `_markup`, скрыт во View/Devices editor/ +compact card), но таблица этого раздела его не перечисляет. `docs/specs/ +203-hide-room-names.md` §13 требует обновить `USER-GUIDE.ru.md`, +`STYLING-HOOKS.md` и `TESTING.md`, но не называет `UX-MODES.md` — канонический +документ подсистемы, который как раз формулирует это самое правило. + +**Почему не блокирует:** ТЗ не изобретает новое правило — оно явно (§14, п.2) +опирается на уже принятый прецедент этого самого раздела, и правильно его +применяет к Plan editor. Отсутствие строки в таблице — вопрос полноты +канонического документа, а не корректности контракта AC1–AC10; ни один AC не +зависит от текста `UX-MODES.md`, и его отсутствие не меняет проверяемость +задачи. + +**Решение ревьюера:** снимается без правки ТЗ, с записью в этом документе. +Рекомендация для реализации: одной строкой добавить `show_names` в таблицу +`docs/UX-MODES.md` при обновлении документации в implementation-коммите — +дёшево и закрывает дрейф канона от кода, но не стоит отдельного цикла ревью. + +### Low — release-артефакты не называют явно, что запись в changelog обязана отметить снятие `.rlabel` как «breaking change» styling hook + +**Файл:** `docs/specs/203-hide-room-names.md` §7, §13; связано с +`docs/STYLING-HOOKS.md` §1 («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 ТЗ корректно фиксирует, что `text.rlabel` перестаёт быть +доступным hook и что документацию `STYLING-HOOKS.md` нужно поправить, но не +связывает это явно с собственным правилом `STYLING-HOOKS.md` о том, что +изменение записи §3 — breaking change, требующий отдельного упоминания в +changelog (а не только общей формулировки бага «названия комнат теперь +скрываются полностью»). Пользователь card-mod, ранее нацеливший CSS на +`.rlabel` (что было доступно только в багованном состоянии, но синтаксически +валидно), теряет цель без явного предупреждения. + +**Почему не блокирует:** §13 уже требует правки обоих changelog и +`STYLING-HOOKS.md` в implementation-коммите; добавление одной фразы про +удаление hook — редакционное дополнение существующего обязательного пункта, +не новое AC и не продуктовое решение. + +**Решение ревьюера:** снимается без правки ТЗ, с записью в этом документе. +Рекомендация для реализации: changelog-запись (RU+EN) явно называет удаление +`.rlabel`/`text.rlabel` как исправление ошибочного styling-контракта, а не +только пользовательский эффект «имена корректно скрываются». + +Других находок — Low, Medium или High — не выявлено. High: 0, Medium: 0. + +## Чего не проверял + +- Реализация не существует на этапе ТЗ; код-ревью будет отдельным циклом + (`S7-code-review`). +- Не прогонялись `npm run typecheck`/`npm test`/`npm run build` в этом цикле: + продуктовый код не менялся с r1 (проверено `git diff 009fed9..HEAD -- src/` + — пусто), гейты ТЗ их не требуют на этой стадии. +- Не запускались браузерные смоки повторно в этом цикле — точечный прогон + `smoke_isometric_contract.mjs` в r1 уже подтвердил механику Labs-флага; + content смоков (строки 28/25 с `show_names: true`) проверен чтением файла, + не исполнением, только чтобы подтвердить отсутствие уже существующего + покрытия `false`-ветки — это не гейт, а факт-чек утверждения ТЗ. +- Не оценивались golden-сцены §10.3 — фикстуры не существуют на этапе ТЗ. +- Не оценивалась производительность — риск в §11 не менялся между r1 и r2 и + был признан корректным в r1. +- Не проверялся текст будущих записей changelog — они не написаны на этапе + ТЗ; §13 называет оба файла обязательными, находка Low выше — рекомендация + к их содержанию, не блокирующее требование. + +## Вердикт + +Medium-находка r1 (третье независимое место дефекта — форсированная +HTML-подпись в hidden iso, `houseplan-card.ts:15825`) устранена точно и +полно: новый мутант `hidden-room-names-iso-override` привязан к правильному +анкеру, анкер уникален в исходнике, guard корректно ссылается на AC3, три +мутанта явно запрещено объединять. Самостоятельная повторная проверка не +нашла четвёртого независимого места того же дефекта и не нашла регресса в +остальном тексте ТЗ, ранее признанном корректным. Обе новые находки — +Low, документационная полнота уже принятых решений, не создающая продуктовой +неопределённости и не влияющая на проверяемость AC1–AC10 — сняты решением +ревьюера с рекомендациями для реализации. High: 0, Medium: 0. + +**Вердикт: зелёный · цикл r2/4 · High: 0 · Medium: 0**