docs: review document for #581

Issue: #581
User-Visible: no

docs/reviews/SPEC-REVIEW-581-r1.md

@@ -0,0 +1,196 @@
+# SPEC-REVIEW-581-r1
+
+Issue: #581 — «Комната сохраняет свой цвет заливки после возврата на «Как у пространства»»
+Этап: ревью ТЗ (PROCESS.md §2.4). Трек: **light (`small`)**, лимит циклов ТЗ — 2.
+Заход: r1 · блокирующих циклов израсходовано 0 из 2.
+SHA рабочей копии на момент ревью: `d37b2c9f8a4369082fec6970117cc97dd1e303c7`.
+
+## Скоуп ревью
+
+ТЗ живёт в теле issue #581, раздел `## ТЗ` (редакция r1, 2026-09-15). Первый цикл
+для этого issue — раздела «Закрытие раунда r0» и «Унаследовано из r0» не
+применимы (§2.10 действует со второго цикла).
+
+Продуктовые вопросы владельцу закрыты им же в теле issue тем же комментарием
+15.09: по всем трём вопросам принят вариант по умолчанию. Открытых продуктовых
+вопросов в тексте не осталось — проверять, что аналитик не оставил догадку под
+видом решения, было главной частью этого разбора.
+
+## Как проверялось
+
+Читал в порядке, предписанном ревьюеру: `docs/SCOPE.md`, `AGENTS.md`,
+`PROCESS.md` §1–§9, тело issue #581 целиком (ТЗ + аналитика + продуктовые
+вопросы/ответы), затем код на `dev`, которым ТЗ аргументирует диагноз, и
+артефакты, которые ТЗ обещает изменить.
+
+Поскольку ТЗ построено на утверждениях о поведении текущего кода («причина» —
+отдельный крупный раздел), я перепроверил каждое из этих утверждений чтением
+кода на SHA `d37b2c9f`, а не поверил автору на слово — именно так по инструкции
+и полагается ловить «догадку, выданную за решение»:
+
+| Утверждение ТЗ | Файл:строка | Подтверждено |
+|---|---|---|
+| `_roomFill` при открытии диалога берётся из `fill_mode` (`glow`→`''`) | `houseplan-editor-runtime.ts:11001-11003` | да |
+| `_roomCustomFill` при открытии диалога берётся из `custom_fill` **независимо от режима** | `houseplan-editor-runtime.ts:11004-11007` | да |
+| Радио «Как у пространства» меняет только `_roomFill`, `_roomCustomFill` не трогает | `houseplan-editor-runtime.ts:14036-14037` (`@change` без сброса `_roomCustomFill`) | да |
+| `_saveRoomEdit`: `custom_fill` пишется независимо от `fill_mode` | `houseplan-editor-runtime.ts:11046-11049` | да |
+| `roomCustomFillOf`: цвет комнаты побеждает независимо от режима | `logic.ts:1303-1310` | да, дословно: `own && typeof own === 'object' ? customFillOf(own, spaceFill) : spaceFill` — признак режима не читается вовсе |
+| `roomFillModeOf`: `glow` — легаси-токен, никогда не возвращается как текущий режим данных | `logic.ts:1923-1930` | да |
+| `resolveEffectiveRoomFill`/`roomFillStyle` применяют `customFill` только при `mode === 'custom'` | `logic.ts:1465` | да — это важно: цвет-«сирота» не течёт напрямую в резолвер, а именно через `roomCustomFillOf`, что и локализует дефект в одной функции, как заявляет ТЗ |
+| `_resolvedRoomFills`: ветка редактируемой комнаты берёт `customFill = this._roomCustomFill \|\| disp.customFill` без учёта режима | `houseplan-card.ts:9330-9335` | да — и это объясняет **живой** баг предпросмотра: если пространство само в `custom`, а `_roomCustomFill` не обнулили при переключении радио на «Как у пространства», предпросмотр в тот же кадр покажет цвет-сироту, а не A (это ровно то, что чинит AC5) |
+| `roomCustomFillOf` вызывается с **полным** `room` в `space-render.ts:508` и `houseplan-card.ts:9335` | оба места передают весь объект комнаты, не урезанный | да — подтверждает claim «потребители лечатся без правки сигнатуры» |
+| `demo/golden/harness.mjs`: фикстура `roomCustomFill` пишет `custom_fill` без `fill_mode` | `demo/golden/harness.mjs:765-775` | да — сегодняшняя golden-фикстура **сама** является примером «сироты», которую чинит АС1; ТЗ корректно называет доработку фикстуры (добавить `fill_mode: 'custom'`) частью работы |
+| `demo/smoke_space_settings.mjs`, шаг 4: override пишется как `custom_fill` без `fill_mode` | `demo/smoke_space_settings.mjs:38-41` | да, тот же паттерн |
+| `test/logic.test.mjs`, блок `roomCustomFillOf`: тесты закрепляют «цвет комнаты побеждает всегда» | `test/logic.test.mjs:619-627` | да, дословно — `roomCustomFillOf(space, { settings: { custom_fill: {...} } })` без `fill_mode` ожидает цвет комнаты; эти строки объявленно переписываются под К1 |
+| `docs/ARCHITECTURE.md` (#56): «пure projection room → space → default» без учёта режима | `docs/ARCHITECTURE.md:1657-1659` | да, текущий текст документирует именно старое (дефектное) поведение — правка требуется, как и заявлено в AC8 |
+| `docs/CONFIG-COMPATIBILITY.md`: тот же текст «room → space → default» | `docs/CONFIG-COMPATIBILITY.md:589-595` | да, аналогично требует правки |
+| `docs/USER-GUIDE.ru.md`, таблица «Наследование», строка «Комната»: «свой цвет … наследуются от пространства» без упоминания завязки на режим | `docs/USER-GUIDE.ru.md:1418-1424` | да, требует правки под AC8 |
+| i18n-ключи `room.custom_fill_own`/`room.custom_fill_space`, `fill.inherit`, `fill.custom` уже существуют (новых ключей не нужно) | `src/i18n/ru.json:420,773-775`, синхронно en/de/fr | да |
+| `demo/smoke_room_settings.mjs` и внутреннее API (`c._roomFill`, `c._saveRoomEdit()`, `c._curSpaceCfg`) пригодны для новых шагов AC3–AC5 | файл существует, паттерн прямого обращения к приватным полям карточки уже используется в существующих шагах | да |
+| `scripts/mutation-registry.mjs`, `scripts/smoke-links.mjs` существуют как площадка для нового мутанта и связи | `ls scripts/` | да |
+
+Ни одного случая, где ТЗ выдаёт непроверенную догадку за факт, не найдено —
+каждое нетривиальное техническое утверждение «причины» подтверждается кодом
+на текущем `dev`, включая тонкий момент (customFill применяется резолвером
+только при `mode==='custom'`, поэтому баг цепляет UI дважды: сохранённый
+результат — через `roomCustomFillOf`, живой предпросмотр диалога — через
+отдельную ветку `_resolvedRoomFills`, и ТЗ фиксирует оба места в контракте
+К1/К5 и К3/AC5 отдельно).
+
+## Разбор по разделам §7.1
+
+Все обязательные разделы присутствуют и в правильном порядке (сценарий и
+«что человек увидит» — первыми, продуктовые, как требует §7.1):
+
+- **Сценарий** — персона (администратор), поверхность (диалог комнаты в
+  редакторе плана), момент (шестерёнка → «Заливка в ЭТОЙ комнате»). ✅
+- **Что человек увидит до/после** — одной фразой, без терминов реализации,
+  плюс явно назван кейс уже испорченных «сирот» (Cabinet). ✅
+- **Проблема** — точна и совпадает с кодом (см. таблицу выше). ✅
+- **Скоуп/не-скоуп** — файлы и функции названы поимённо, не-скоуп (уровень
+  пространства, бэкенд-схема, автоматическая чистка без сохранения, Glow)
+  отделён явно. ✅
+- **Контракт К1–К5** — каждый пункт — проверяемое, однозначное утверждение о
+  наблюдаемом поведении (не о реализации): «участвует в раскраске только при…»,
+  «показывается только при…», «пишется только вместе с…». Ни один пункт не
+  описывает «как» вместо «что». ✅
+- **UX** — явно «только вычитающее», без новых элементов; корректно, я
+  проверил (см. ниже) что для этого действительно достаточно снять
+  строку цвета, а не добавить новую механику. ✅
+- **Модель данных и миграция** — форма не меняется, обратная совместимость
+  описана в обе стороны (старая карта читает новый конфиг / новая карта читает
+  старый конфиг с сиротой). ✅
+- **i18n** — «новых ключей нет», подтверждено чтением `ru.json`/`en.json`. ✅
+- **AC1–AC8** — у каждого указан способ доказательства (`unit`/`smoke`/
+  `golden`/«ревью кода») и «чем краснеет» — где применимо (АС8 — не защитный
+  критерий, «—» уместно per §2.7: правило о третьем столбце не
+  распространяется на AC текста/расположения). Каждый AC — однозначное,
+  проверяемое утверждение, ни один не сформулирован как «сделать вот так». ✅
+- **План автотестов** — конкретен, называет файлы и даже новый мутант по
+  имени. ✅
+- **Риски** — три риска, все с последствиями и указанием, что это
+  сознательно принятое решение (риск 1 — прямая ссылка на ответ на вопрос 3). ✅
+- **Откат** — обратный коммит, без обратной миграции; аргументация корректна:
+  старая карта на новом конфиге и без того ведёт себя как «без своего цвета». ✅
+- **Release-артефакты** — changelog RU+EN, три документа, golden без
+  пересъёмки, скриншоты — попиксельная приёмка. ✅
+- **«Принято предположительно»** — блок явный, помечен как технический и
+  «поменять свободно», ревьюер вправе оспорить. Использовал это право ниже
+  (см. «Находки», Low). ✅
+
+## Проверка отдельного тонкого места: строка цвета в диалоге
+
+Контракт К3 требует: строка цвета видна **только** при собственном режиме
+комнаты `custom` (не при эффективном/унаследованном). Сегодняшний рендер
+диалога вычисляет видимость через `effectiveFill = this.host._roomFill ||
+spaceDisplay.fill` (`houseplan-editor-runtime.ts:13996`, `:14041`) — то есть
+через **эффективный**, а не «собственный», режим. Если после исправления
+поведения оставить этот гейт как есть, то в ровно репродуцирующем баг случае
+(комната наследует, а пространство само в «Свой цвет») строка цвета
+продолжит показываться при повторном открытии диалога — читая на этот раз уже
+`disp.customFill`, а не сироту, но всё равно вопреки явно принятому ответу на
+продуктовый вопрос 2 («не показывать строку цвета»). АС3/АС4 корректно
+фиксируют желаемый конечный результат («при открытии диалога hp-color-opacity
+отсутствует»), поэтому смок поймает неверную реализацию, если разработчик не
+заметит эту деталь. Это не дефект ТЗ — К3 однозначно формулирует нужное
+поведение («в собственном режиме комнаты», не «в эффективном»), а какая именно
+переменная гейтит JSX-ветку, ТЗ решать не обязано (§7.1: «всё, чего
+пользователь не наблюдает, агенты решают сами»). Отмечаю как **Low**: блок
+«Принято предположительно» мог бы явно назвать этот гейт как меняющийся —
+сейчас он называет только сброс черновика и условие резолва — но отсутствие
+этой строчки не создаёт неоднозначности контракта и не блокирует.
+
+## Находки
+
+**High: 0. Medium (в скоупе): 0. Medium (вне скоупа): 0.**
+
+- **Low-1** — раздел «Принято предположительно» не называет явно, что гейт
+  видимости строки цвета в `_renderRoomDialog` (`effectiveFill === 'custom'`,
+  `houseplan-editor-runtime.ts:14041`) должен перейти на «собственный режим
+  комнаты», а не «эффективный» — см. разбор выше. Не блокирует: К3 уже
+  однозначен, AC3/AC4 дадут красный смок при неверной реализации. Решение
+  ревьюера: **снято с записью**, отдельного возврата не требует.
+- **Low-2** — трек `small` заявлен с обоснованием «одна поверхность» при
+  скоупе в три файла (`logic.ts`, `houseplan-editor-runtime.ts`,
+  `houseplan-card.ts`). Аналитик поясняет это как «одна поверхность —
+  заливка комнаты (резолв + её диалог)», что соответствует духу критерия
+  §5 (один диалог/один функциональный узел, а не буквально один файл) и
+  совпадает с прецедентом #487 (наследование температурных границ, тоже
+  light-трек, тоже три файла). Не является находкой по существу — фиксирую
+  только как явную проверку, не изменение.
+
+Обе записи — Low, не изменяют вердикт.
+
+## Что проверено и корректно
+
+- Диагноз ТЗ («причина») построен на чтении кода, а не на предположении —
+  каждое утверждение сверено построчно на SHA `d37b2c9f` (таблица выше).
+- Продуктовые вопросы владельцу заданы пачкой, с вариантами по умолчанию, и
+  закрыты им же в теле issue — открытых продуктовых вопросов не осталось.
+- AC1–AC8 однозначны, у каждого указан способ доказательства; защитные AC
+  (АС1–АС5) называют «чем краснеет» — конкретный мутант либо конкретную
+  порчу шага/фикстуры, а не общую фразу.
+- Не-скоуп отделяет уровень пространства и бэкенд-схему явно и обоснованно.
+- Ссылки на файлы/функции/тесты/доки, упомянутые в скоупе и AC, существуют и
+  соответствуют описанному состоянию (доки #56/USER-GUIDE/CONFIG-COMPATIBILITY
+  сейчас описывают именно старое, дефектное поведение — значит, их
+  действительно придётся переписать, а не «уже написано верно»).
+- Откат и совместимость проверены логически непротиворечивы: конфиг новой
+  версии, прочитанный старой картой, не образует новых несовместимых форм —
+  форма данных не меняется, только правило чтения ставшего строже поля.
+
+## Чего не проверял
+
+- Не запускал никаких гейтов (`npm test`, `tsc`, смоки) — на этапе ревью ТЗ
+  кода ещё нет, гейты неприменимы; весь разбор — чтение ТЗ и текущего кода на
+  `dev`.
+- Не проверял по существу верстку/CSS диалога (`plan.styles.ts`) дальше того,
+  что нужно для подтверждения существования `--room-fill`/`--room-fill-op` и
+  класса `.room.styled` — этого достаточно для проверки утверждений АС2, глубже
+  не требовалось.
+- Не проверял содержимое остальных языков (de/fr) построчно — только что ключи
+  существуют параллельно ru/en; ТЗ не заявляет новых ключей, этого достаточно.
+- Не оценивал производительность — ТЗ прямо и обоснованно называет влияние
+  «нет» (одна чистая функция, одно доп. чтение свойства за кадр), это в
+  пределах разумного для ревью ТЗ.
+
+## Вердикт
+
+Вердикт: зелёный · заход r1 · блокирующих циклов 0/2 · High: 0 · Medium: 0
+
+ТЗ проверяемо, однозначно, продуктовые вопросы закрыты владельцем, диагноз
+подтверждён чтением кода, а не догадкой. Готово к разработке.
+
+---
+
+<!-- material-anchors: сгенерировано конвейером (#414) -->
+
+## Материал раунда
+
+- Ветка: `dev`, коммит `d37b2c9f8a43` — ребейз его осиротит, и это нормально: ниже якоря, которые ребейз не меняет.
+- Дерево материала: `cd142c70cdb0ab25b12dd4fa2a77c9d955f01940`
+  ```
+  git log --all --format='%H %T' | grep cd142c70cdb0
+  ```
+- Тело issue: `b589afc4099fee83d6a37b7e233e37a4b71277d5a3bae884a70b28920a3b13ad`
+- Вердикт конвейера: `green` · High 0