diff --git a/docs/reviews/SPEC-REVIEW-62-r1.md b/docs/reviews/SPEC-REVIEW-62-r1.md new file mode 100644 index 00000000..90ce94f4 --- /dev/null +++ b/docs/reviews/SPEC-REVIEW-62-r1.md @@ -0,0 +1,197 @@ +# SPEC-REVIEW-62-r1 + +- **Issue:** https://github.com/Matysh/houseplan-card/issues/62 +- **ТЗ:** `docs/specs/062-i18n-registry.md` +- **Материал ревью:** ветка `issue/62-i18n-registry`, SHA `21d47c52` + (совпадает с SHA, названным автором в хендоффе на ревью) +- **Заход:** r1 (первый формальный проход; issue шёл `S3-spec` с 2026-08-15, + ТЗ актуализировано и отправлено на `S4-spec-review` 2026-08-27) +- **Трек:** полный. Аналитика и сам документ явно называют нарушенный критерий + `small` («одна поверхность») — задета class A i18n сразу в нескольких + поверхностях (registry, resolver, editor selector, frontend/backend gate, + CONTRIBUTING). Соответствует текущему умолчанию AGENTS.md (#338): отказ от + лёгкого трека обосновывается названным критерием, что и сделано. + +## Скоуп + +Диапазон `origin/dev...HEAD` содержит только два документных коммита класса C: +`docs/specs/062-i18n-registry.md` (новый файл, 292 строки) и добавление одной +строки-ссылки в `docs/specs/README.md`. Продуктового кода (class A) в диапазоне +нет — это ожидаемо для этапа ТЗ, реализация ещё не начиналась. + +Задача: убрать дублирование списка языков (`src/i18n.ts` вручную задаёт `Lang`/ +`DICTS`, `src/editor.ts` вручную перечисляет `en`/`ru` в selector, +`test/i18n.test.mjs` вручную проверяет только эту пару, frontend/backend +translation-каталоги никак не сверяются) через единый `src/i18n/registry.ts`, +без изменения видимого поведения English/Russian. + +## Как проверялось + +1. Прочитаны `docs/SCOPE.md`, `AGENTS.md`, `PROCESS.md` целиком. +2. Прочитано тело issue #62 и все три комментария (аналитика 2026-08-14, + заведение ТЗ 2026-08-15, актуализация и отправка на ревью 2026-08-27) — + через `gh issue view` (MCP `get_issue`/`get_issue_comments` были недоступны + без выданного разрешения). +3. Прочитан ТЗ `docs/specs/062-i18n-registry.md` целиком, сверен на разделы + §7.1 PROCESS.md. +4. Каждое фактическое утверждение ТЗ о текущем коде сверено с реальным + деревом, а не принято на слово: + - `src/i18n.ts` — подтверждён ручной `Lang`, ручные imports, ручной `DICTS`, + докстрока действительно обещает добавление языка без правок TypeScript, + что не соответствует коду (`langOf`/`t`/`DICTS` требуют правки); + - `src/editor.ts` — подтверждён ручной перечень `en`/`ru` в + `selector.select.options` (строки 100–104), без обработки неизвестного + сохранённого значения; + - `test/i18n.test.mjs` — подтверждено прямое чтение `en.json`/`ru.json`, + без registry, без backend file-set проверки; + - `custom_components/houseplan/translations/` — подтверждён набор + `en.json`/`ru.json`, структурно отличный от frontend-словарей (вложенный + HA config_flow формат против плоского key→string) — ТЗ говорит о паритете + **набора файлов**, а не содержимого, поэтому структурное различие не + противоречит контракту; + - `tsconfig.test.json` — подтверждено отсутствие `src/i18n.ts` в `include`, + то есть заявленный в §6.4 шаг «включить в tsconfig.test.json» реален и + нужен; подтверждён существующий паттерн импорта скомпилированных модулей + тестами из `../test-build/*.js` (`npm test` = `tsc -p tsconfig.test.json` + → `node --test`), на который ТЗ опирается для registry-driven gate — + это не техническая догадка, а перенос уже работающего паттерна; + - `src/types.ts:276` — подтверждён комментарий + `language?: string; // 'en' | 'ru' | '' (auto — HA profile)`, который ТЗ + обещает исправить (цель 6); + - `CONTRIBUTING.md` — прочитан целиком (см. находку ниже); + - `docs/USER-GUIDE.ru.md` — единственное упоминание `language` (строка 129) + ограничено таблицей `auto`/`ru`/`en`; ТЗ не меняет эту строку, что + согласуется с заявлением «пользователь en/ru изменений не увидит»; + - `docs/CONFIG-COMPATIBILITY.md` — прочитан целиком; поле `language` не + геометрическое и не относится к категориям, которые реестр обязан + покрывать (модель стен/layout/marker.space), поэтому отсутствие записи в + этом реестре не является пробелом ТЗ. +5. Грепом по `src/**` подтверждено, что знание о конкретных кодах `en`/`ru` + вне `src/i18n.ts`/`src/editor.ts`/`src/types.ts` больше нигде не зашито — + заявленный ТЗ список затронутых файлов полон. +6. Продуктового кода в диапазоне нет, поэтому `typecheck`/`test`/`build` не + применимы к этому ревью — гонять их не на чем: диапазон не содержит ни + одного файла `src/**`/`custom_components/**/*.py`. `check-docs.mjs` + аналогично не запускался: скриншотный отпечаток зависит от `src/**`, а этот + диапазон его не трогает. + +## Находки + +### Medium (в скоупе задачи — чинится в этом же ТЗ) + +**M1. `CONTRIBUTING.md` останется противоречить новому contribution flow — +AC9 не покрывает существующую строку.** + +- **Файл:** `docs/specs/062-i18n-registry.md`, §9 (и AC9, §10) +- **Воспроизведение:** текущий `CONTRIBUTING.md` уже содержит утверждение + (раздел «Ground rules»): + > Adding a language = adding one JSON file + registering it in `src/i18n.ts`. + + После реализации ТЗ добавление языка = frontend JSON + backend JSON + одна + запись в `src/i18n/registry.ts`; `src/i18n.ts` при добавлении языка больше не + редактируется (§6.1: «`src/editor.ts`, `langOf()` и список локалей в тестах + больше не редактируются» — но и сам `src/i18n.ts` тоже не редактируется, + поскольку registry выносится в отдельный модуль). §9 ТЗ описывает **новый** + раздел «Translations» в `CONTRIBUTING.md`, но нигде не говорит, что + существующую фразу в «Ground rules» нужно убрать или привести в соответствие. + Реализация, буквально следующая ТЗ, добавит верный раздел «Translations» и + оставит в том же файле, несколькими экранами выше, ложную инструкцию, + указывающую редактировать `src/i18n.ts` — то есть ровно тот файл, который + задача выводит из процесса добавления языка. +- **Почему это находка, а не придирка:** AC9 формулируется как «CONTRIBUTING и + комментарии описывают фактический contribution flow» — с этим пробелом AC9 + по букве может быть выполнен (новый раздел добавлен), а по смыслу нет: + документ будет противоречить сам себе, и следующий contributor, дочитавший + до «Ground rules» раньше «Translations», получит неверную инструкцию. +- **Как закрыть:** добавить в §9 явное указание убрать/переписать эту строку + «Ground rules» так, чтобы она либо ссылалась на новый раздел «Translations», + либо была удалена как дублирующая его. +- **Серьёзность:** Medium, в скоупе (документация — часть DoD этой же задачи, + правка тривиальна, отдельный issue не заводится согласно #202). + +### Low (снимается с записью) + +**L1. AC2 не имеет явного маркера «Доказательство:» в отличие от остальных +восьми AC.** Формулировка «покрыты матрицей unit-тестов» по смыслу эквивалентна +`Доказательство: unit`, метод проверки не теряется. Косметика, не блокирует. + +**L2. AC5 и AC8 подмешивают в доказательство слово «inspection» (`inspection +compiled schema`, `production build inspection») — не входит в канонический +словарь `unit`/`backend`/`smoke`/`golden`/«ревью кода».** По содержанию это и +есть «проверено чтением кода/бандла, не исполнением» — допустимая категория +для code-review, но на этапе ТЗ стоило явно назвать её этим термином, а не +свободным словом. Обе AC при этом дополнительно подкреплены конкретным +автотестом (unit) или диффом, так что решение проверяемо в любом случае. +Снимается без правки текста ТЗ. + +Оба Low не блокируют переход и не создают риска молчаливого пропуска: метод +проверки в обоих случаях восстановим по тексту без домысливания. + +## Что проверено и признано корректным + +- Обязательные разделы §7.1 присутствуют все: сценарий, что человек увидит, + проблема, скоуп/не-скоуп, контракт поведения, UX, модель данных и миграция, + i18n, AC1–AC9 с доказательствами, план автотестов, риски, откат, + release-артефакты — плюс два необязательных (план реализации, «технические + предположения, можно менять»). +- **Продуктовые вопросы отсутствуют по делу, а не по недосмотру.** Единственный + пограничный случай с пользовательской видимостью — что видит редактор + карточки при неизвестном сохранённом `language` (§6.3, §7, AC6) — решён + автором явно и обоснованно (защита от тихой потери значения при + редактировании несвязанного поля, в духе уже принятого в `docs/SCOPE.md` + принципа «не терять данные пользователя на догадке»), а не спрятан как + открытый вопрос. Формулировка достаточно точна для реализации и + автотеста без дополнительных уточнений: временная option появляется только + для незарегистрированного кода, исчезает после явного выбора, не переживает + несвязанное изменение формы в смысле «взаимодействие не должно её стереть». + Эскалации владельцу это решение не требовало — оно не меняет поведение + English/Russian и не вводит новый пользовательский сценарий, только + расширяет обработку уже существующего (произвольная строка в + `CardConfig.language`, доступная через YAML) случая. +- Резолюция языка (§6.2: exact tag → primary subtag → English fallback, + регистронезависимо, `_`→`-`) при двух текущих локалях **эквивалентна** + нынешнему `l.startsWith('ru') ? 'ru' : 'en'` для всех обычных значений HA + locale (`ru`, `ru-RU`, `ru_RU`, `en`, `en-US`, что угодно ещё → `en`) — + проверено разбором обеих реализаций построчно. AC7 («видимый selector и + значения словарей не меняются») этим не нарушается. +- Заявление «`en`/`ru` пользователь изменений не увидит» подтверждено: + единственная новая видимая ветвь (временная option) активируется только для + кода, отсутствующего в registry, то есть никогда для нынешних инсталляций. +- Тестовая стратегия (§11) реалистична: паттерн «скомпилированный TS-модуль + импортируется тестом из `test-build/`» уже используется в проекте (проверено + на пяти существующих тестовых файлах), а не изобретается заново. +- Non-scope (§5) закрывает именно те пункты, которые в issue были спорными — + явно снят lazy loading (ранее упомянутый в заголовке issue и вычеркнутый + самим автором из формулировки), явно исключены RTL, plural rules, Weblate/ + Crowdin, привязка backend runtime к frontend registry. +- Откат (§13) и release-артефакты (§14) заполнены осмысленно, не шаблонной + фразой; `User-Visible: no` обоснован отсутствием видимого изменения. +- Трек и его обоснование (полный, а не `small`) соответствуют текущему + умолчанию AGENTS.md/#338 — критерий назван прямо. + +## Чего не проверял и почему + +- **Гейты `typecheck`/`npm test`/`npm run build`/`check-docs.mjs`/browser + smoke/golden/backend pytest** — не запускались. Диапазон `origin/dev...HEAD` + не содержит ни одного файла class A/B (`src/**`, `test/**`, + `custom_components/**`), только два файла `docs/**`. Гонять их не на чем: + это этап ТЗ, продуктовый код ещё не написан. Они будут первым осмысленным + гейтом на этапе код-ревью этой же задачи. +- **Инварианты модели / `smoke-select.mjs`** — не применимо: диапазон не + трогает геометрию, `layout`, `marker.space`, `open_spans` и не содержит + browser-исполняемого кода. +- **Реализуемость `Lang`, выведенного из `as const`-массива registry, без + отдельного union** — не проверялась компиляцией (кода ещё нет), только + разбором формулировки на внутреннюю непротиворечивость; это TypeScript-паттерн + без выявленных противоречий, но окончательное слово — у код-ревью, когда + появится реальный `.d.ts`. + +## Вердикт + +Один Medium-дефект в скоупе задачи (документация, AC9), без High. Дефект +конкретный, воспроизводимый цитатой существующего файла и правится без +пересмотра контракта — не требует нового цикла продуктового мышления, только +дополнения §9 одной фразой про существующую строку `CONTRIBUTING.md`. + +**Вердикт: жёлтый · заход r1 · блокирующих циклов 1/4 · High: 0 · Medium: 1 → +в задаче**