From e54ff33ca1569f126b7d1beee3733670a0228c50 Mon Sep 17 00:00:00 2001 From: "claude[bot]" <209825114+claude[bot]@users.noreply.github.com> Date: Tue, 15 Sep 2026 05:58:40 +0000 Subject: [PATCH] docs: review document for #581 Issue: #581 User-Visible: no --- docs/reviews/SPEC-REVIEW-581-r1.md | 196 +++++++++++++++++++++++++++++ 1 file changed, 196 insertions(+) create mode 100644 docs/reviews/SPEC-REVIEW-581-r1.md diff --git a/docs/reviews/SPEC-REVIEW-581-r1.md b/docs/reviews/SPEC-REVIEW-581-r1.md new file mode 100644 index 00000000..0dd9e37d --- /dev/null +++ b/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 + +ТЗ проверяемо, однозначно, продуктовые вопросы закрыты владельцем, диагноз +подтверждён чтением кода, а не догадкой. Готово к разработке. + +--- + + + +## Материал раунда + +- Ветка: `dev`, коммит `d37b2c9f8a43` — ребейз его осиротит, и это нормально: ниже якоря, которые ребейз не меняет. +- Дерево материала: `cd142c70cdb0ab25b12dd4fa2a77c9d955f01940` + ``` + git log --all --format='%H %T' | grep cd142c70cdb0 + ``` +- Тело issue: `b589afc4099fee83d6a37b7e233e37a4b71277d5a3bae884a70b28920a3b13ad` +- Вердикт конвейера: `green` · High 0