diff --git a/docs/reviews/SPEC-REVIEW-363-r1.md b/docs/reviews/SPEC-REVIEW-363-r1.md new file mode 100644 index 00000000..c8102bda --- /dev/null +++ b/docs/reviews/SPEC-REVIEW-363-r1.md @@ -0,0 +1,182 @@ +# SPEC-REVIEW-363-r1 + +Issue: #363 · Этап: spec (PROCESS.md §2.4) · Трек: `small` (лёгкий) · Заход r1 · +блокирующих циклов израсходовано 0 из 2 (лимит лёгкого трека — 2, §5) + +## Материал ревью + +- Тело issue #363 (владелец: требование + контекст + «Что было до #29»), + далее секция «ТЗ · revision 1 · small track», написанная автором ТЗ. +- Единственный комментарий issue — аналитика владельца (S2), метки: + `P2`, `feature`, `polish`, `small`, `S4-spec-review`. +- Файла в `docs/specs/` нет и не должно быть — метка `small` это разрешает + (`scripts/process-gate.mjs` строка `NO_SPEC_FILE = ['small', 'trivial']`, + проверено чтением). +- Кода по задаче ещё нет: `git log --all --oneline | grep 363` и + `git branch -a | grep 363` ничего не находят, кроме случайных совпадений с + другими номерами issue (`3633a3db`, `58363731` и т.п., не относятся к #363). + Гейты `typecheck`/`test`/`build`/`check-docs` в этом заходе не прогонялись: + прогонять их не над чем — это ревью текста, не кода (§2.4, не §2.7). + +## Как проверялось + +Не поверил ни одному фактическому утверждению ТЗ и аналитики на слово — +перепроверил каждое чтением текущего дерева `dev` (`HEAD=c19a0540`): + +1. **`_openMarkerDialog()` жив и не требует правок.** + `src/houseplan-card.ts:9343` — приватный метод карточки, делегирует в + `src/houseplan-editor-runtime.ts:7628` `public _openMarkerDialog(d?: DevItem)`. + Вызов без аргумента — тот же путь, что и раньше. Номера строк в аналитике + (`:9308`) слегка разошлись с текущими из-за более поздних коммитов, но метод + и его контракт те, что описаны — не находка. +2. **Ключи i18n действительно отсутствуют и восстанавливаются побайтово.** + `grep devbar\.|title.add_device src/i18n/{en,ru,de}.json` не находит ничего, + кроме `devbar.rules`. Проверил сам коммит-виновник: + `git show cab8d128^:src/i18n/en.json` и `...ru.json` дают ровно + `"title.add_device": "Add a device to the plan"` / `"devbar.add": "Add"` и + `"Добавить устройство на план"` / `"Добавить"` — таблица ТЗ совпадает + посимвольно. Немецкий перевод `"Hinzufügen"` уже используется в словаре для + того же действия (`device_inbox.add`, `de.json:462`) — предположение + согласовано с терминологией, а не придумано. +3. **Паритет словарей действительно ловит забытую локаль.** + `test/i18n.test.mjs:109-112` — `assert.deepEqual(Object.keys(dictionary).sort(), enKeys)` + для каждого языка реестра. Тест умеет падать: пропуск любого из шести + значений (2 ключа × 3 локали) даёт красный `npm test`. AC4 доказуемо. +4. **Место вставки существует.** `src/houseplan-editor-runtime.ts`, + `_renderDevicesBar()` — первая кнопка `device_inbox.button` («Устройства»), + вторая `devbar.rules` («Правила иконок»). До #29 первой кнопкой была + «Добавить» (подтверждено чтением `cab8d128^:src/houseplan-card.ts:20752-20753`). + Вставка перед «Устройства» восстанавливает этот порядок. +5. **Каталог и его добавление виртуального устройства не затронуты.** + `_renderDeviceInbox()` и `openVirtual()` в том же файле не упоминаются в + диапазоне правок; `demo/smoke_device_inbox.mjs` существует и его заголовок + прямо ссылается на #29 («one lifecycle catalog replaces the separate Add / + hidden-device paths») — правильный существующий регресс-щуп для AC3. +6. **RU-таблица тулбара действительно потеряла строку.** + `docs/USER-GUIDE.ru.md:779-786` — таблица «Редактор устройств» содержит + «Устройства» и «Добавить виртуальное устройство», строки про «Добавить» нет. + EN-версия (`docs/USER-GUIDE.md:519-529`) — список, а не таблица, тоже без + пункта «Devices editor: Add». Оба документа в «Затронутых файлах» названы. +7. **Golden-сцены разведены правильно.** `demo/golden/matrix.mjs:381` — + `geometry-devices-editor-dark` снимает `mode: 'devices'` **без** открытого + диалога — тулбар виден целиком, кнопка на нём действительно появится. + `device-inbox-*` (там же, строки 383-390) открывают `dialog: 'device-inbox'`, + который перекрывает тулбар модальным окном — эти три сцены обоснованно не + должны измениться. `--expect-change=geometry-devices-editor-dark` — точно + нужная и единственная нужная пометка. +8. **Проверил ловушку, в которую эта задача могла попасть незамеченной:** + риск незаявленного изменения одного и того же числа/кадра в двух местах. + Открыл сам `docs/images/06-device-editor.png` (документационный скриншот, + отдельный от golden) — тулбар редактора устройств на нём **полностью + перекрыт открытым диалогом** (диалог начинается от x=0 и шире, чем + `editbar-tools`), виден только правый `barclose` («✕»), который эта задача + не трогает. Значит пиксели этого конкретного файла не изменятся — здесь + утверждение ТЗ «новый docs screenshot не требуется» дословно верно. + Но это не то же самое, что «шаг с docs-скриншотами не нужен вовсе» — см. + находку M1 ниже: сам гейт `docs` не привязан к пикселям. +9. **UX-MODES.md, TOUCH-SUPPORT.md** — прочитаны на предмет конфликта с + контрактом «постоянный инструмент в основном тулбаре, Close в своём торце» + (UX-MODES.md:38-41) и «редакторы desktop-first, touch — best effort» + (TOUCH-SUPPORT.md:10). Конфликтов нет: кнопка — персистентный инструмент, + в правый торец не претендует, новых touch-целей не создаёт. + +## Находки + +### M1 (Medium, в скоупе задачи) — Release-артефакты не называют обязательный шаг для гейта `docs` + +**Что не так.** Раздел «Release-артефакты» ТЗ утверждает: «новый docs +screenshot не требуется: канонический набор не фиксирует Device editor +toolbar как отдельный читаемый пользовательский фрагмент» — и это верно только +в узком смысле «новый файл сцены заводить не нужно» (проверено в п.8 выше). +Но ТЗ нигде не называет действие, которое всё равно обязательно: обновить +`docs/images/screenshots.json` через `.github/workflows/docs-screenshots.yml` + +`npm run docs:accept -- --reviewed --from=...`, коммитом в задаче. + +**Почему это обязательно, а не перестраховка.** Гейт `check-docs.mjs` +сравнивает `manifest.sourceFingerprint` с `visualFingerprint(ROOT)` +(`scripts/source-fingerprint.mjs`), а `visualFingerprint` считается по **всему** +`src/**` без исключений для i18n-словарей (`fingerprintFiles()` включает +`sourceFiles(resolve(root, 'src'))` целиком). Задача правит +`src/houseplan-editor-runtime.ts` и все три `src/i18n/*.json` — оба пути внутри +корпуса отпечатка. Значит `manifest.sourceFingerprint` разойдётся с деревом для +**всех 9** сценариев `demo/docs/screenshots.mjs`, не только для +`device-editor`, и `docs` job в CI Validate станет красным независимо от того, +что ни один PNG не изменится по пикселям — сам гейт различий по пикселям не +делает, он сверяет хэш дерева. + +Это ровно тот сценарий, который уже дважды стоил продукту дня простоя `dev` +(#230, #234, исправлено в #237) — и который прецедент в этом же репозитории +разбирал явно: `docs/reviews/CODE-REVIEW-337-r4.md` фиксирует «H7-фикс» +коммитом `0bd5570a` — пересъёмку и приёмку ради одного изменившегося +`imageSha256` после правки того же тулбара. АС7 ссылается на «docs/process +gates» как на доказательство, но этого недостаточно как формулировки: она не +называет действие, которое делает этот гейт зелёным, и без него автор с +высокой вероятностью повторит #230/#234 — сам факт, что ТЗ не упомянуло +`visualFingerprint`, показывает, что автор про этот механизм не думал. + +**Серьёзность и скоуп.** Medium, внутри уже заявленного скоупа задачи (класс C, +`docs/**`, часть DoD пункта «Release-артефакты» и AC7) — не отдельный issue +(владелец, 2026-08-19, #202). + +**Требуемая правка ТЗ.** В «Release-артефакты» и/или AC7 добавить явный пункт: +после правки `src/**` — прогнать `Скриншоты документации` +(`workflow_dispatch` на ветке задачи), принять артефакт +`npm run docs:accept -- --reviewed --from=<распакованный>` и закоммитить +получившийся `docs/images/screenshots.json` (плюс любые PNG, которые всё же +разойдутся побайтово из-за среды рендера — прецедент #231 упоминает разницу в +7-8 байт при неизменном кадре). Ожидаемый результат — обновлённый +`sourceFingerprint`/`sourceSha256` во всех 9 сценариях при явном ожидании: все +9 PNG остаются пиксельно идентичными (в отличие от golden, где ожидается +измениться ровно один кадр). + +## Что проверено и корректно + +- Скоуп/не-скоуп разведены точно и не заходят на каталог, backend, View/kiosk, + Plan/Background editors — соответствует `docs/SCOPE.md` (персона 1, + администратор дома, оба редактора) и «View mode is the product» инварианту: + режим View не тронут вовсе. +- Все AC1–AC8 проверяемы и каждой указан способ доказательства; ни один не + опирается на несуществующий инструмент — `test/i18n.test.mjs` (AC4), + `demo/smoke_device_inbox.mjs` (AC3), `demo/golden/matrix.mjs` записи (AC8) + реально существуют и умеют падать на нарушении соответствующего утверждения. +- Ни одной догадки, выданной за решение: все продуктовые детали (подпись, + порядок, иконка) обоснованы либо прямой цитатой владельца («как до #29»), + либо байт-в-байт восстановленным историческим состоянием — раздел «Принятые + предположения» содержит только техническое (CSS wrap, ариа помимо + существующей практики, немецкая формулировка), что и предписано процессом + (владельцу — только продуктовые вопросы). +- Трек `small` подтверждён по всем пяти критериям §5 одновременно (сложность + ≤3, одна поверхность — тулбар, без миграции конфига, без нового + UX-контракта — поведение и подпись это то же самое, что было до #29, без + влияния на перф/touch). +- i18n-таблица (en/ru/de) соответствует существующей терминологии словаря + (`Hinzufügen` уже используется для того же действия). +- Golden-план различает сцену, которая обязана измениться + (`geometry-devices-editor-dark`, тулбар виден целиком), от сцен, которые не + должны (`device-inbox-*`, тулбар закрыт диалогом) — реальное чтение сцен это + подтверждает. +- Обе версии USER-GUIDE названы в затронутых файлах и обе действительно + требуют правки (EN — список, RU — таблица, оба без строки «Добавить»). +- Откат описан и достаточен: удалить кнопку, 6 i18n-записей, тестовые ожидания + и документацию; миграции данных нет, потому что persisted-модель не менялась. + +## Чего не проверял + +- Не запускал `npm run typecheck`/`test`/`build`/`check-docs.mjs` — кода нет, + это стадия ревью ТЗ (§2.4), не код-ревью (§2.7); гейты будут обязательны на + выходе из «В разработке». +- Не оценивал реальный визуальный вид будущей кнопки (макета/скриншота нет) — + оценивал только текстовый контракт (иконка/подпись/позиция/порядок), это и + есть предмет ТЗ-ревью. +- Не проверял `npm run bundle:budget` фактически — утверждение «budget не + повышается» для одной кнопки и 6 строк i18n правдоподобно на глаз, но не + измерено; риск оцениваю как пренебрежимо малый, отдельной находкой не делаю. +- Не проверял немецкую формулировку `title.add_device` носителем языка — + только на согласованность с существующим словарём; ТЗ само помечает её как + предположение, которое ревьюер вправе поправить одной строкой в код-ревью. + +## Вердикт + +Единственная находка — Medium, в скоупе, без High. По PROCESS.md §2.4/§7.2 это +жёлтый вердикт: ТЗ возвращается автору на правку раздела Release-артефакты/AC7, +без создания отдельного issue.