diff --git a/docs/reviews/SPEC-REVIEW-203-r1.md b/docs/reviews/SPEC-REVIEW-203-r1.md new file mode 100644 index 00000000..d3ed8f6c --- /dev/null +++ b/docs/reviews/SPEC-REVIEW-203-r1.md @@ -0,0 +1,296 @@ +# Ревью ТЗ — issue #203, цикл r1 + +- Этап: `S4-spec-review` (PROCESS.md §2.4) +- Артефакт ТЗ: [`docs/specs/203-hide-room-names.md`](../specs/203-hide-room-names.md), + коммит `009fed9` на ветке `issue/203-hide-room-names` +- Issue: [#203](https://github.com/Matysh/houseplan-card/issues/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_`, + дефолт `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` — + +```ts +${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 → в задаче**