diff --git a/docs/reviews/SPEC-REVIEW-354-r1.md b/docs/reviews/SPEC-REVIEW-354-r1.md new file mode 100644 index 00000000..7776f7c6 --- /dev/null +++ b/docs/reviews/SPEC-REVIEW-354-r1.md @@ -0,0 +1,188 @@ +# SPEC-REVIEW-354-r1 + +- **Issue:** https://github.com/Matysh/houseplan-card/issues/354 +- **Этап:** ревью ТЗ (PROCESS.md §2.4), лёгкий трек (`small`) +- **Заход:** r1 · блокирующих циклов израсходовано 0 из 2 (лимит ТЗ на лёгком треке — 2) +- **ТЗ:** тело issue #354 (файл в `docs/specs/` отсутствует — корректно для `small`) +- **SHA на момент ревью:** `77302eaf` (`dev`, рабочее дерево чистое) +- **Вердикт: жёлтый** + +## Скоуп + +Задача техдолга: продакшн-объект `LANGUAGE_RUNTIME` в `src/i18n/registry.ts` — +рукописный литерал (`germanDictionary`/`germanPending`/`germanFailed`/ +`settleGerman`), логически дублирующий уже протестированный класс +`LanguageRuntime` (`src/i18n/language-runtime.ts`), который покрывает +`test/i18n-runtime.test.mjs`. Тесты сейчас доказывают свойства не того кода. +Фикс: собрать `LANGUAGE_RUNTIME` как `new LanguageRuntime(...)`, удалить +рукописные поля, добавить контроль от возврата дубля (К3) и, попутно (N7, Low), +тост при отказе загрузки словаря вместо тихого `console.warn`. + +По `docs/SCOPE.md`: задача — техдолг вокруг J4/J6 (инфраструктура i18n из #62), +пользовательского функционала не добавляет, кроме одного нового тоста (N7). +Отдельного раздела «Job» не требует — это внутренняя правка тестируемости плюс +мелкое UX-улучшение по уже существующему паттерну (`editor.load_failed`). +Конфликта со SCOPE.md нет. + +## Как проверялось + +1. Прочитаны `docs/SCOPE.md`, `PROCESS.md` целиком (жизненный цикл, §4, §5, + §7.1, §2.10). +2. Прочитаны тело issue #354 и комментарий аналитика (S2→S3, единственный + комментарий на issue). +3. Прочитан текущий код: `src/i18n/language-runtime.ts` (класс `LanguageRuntime`, + контракт `LanguageRuntimeContract`), `src/i18n/registry.ts` (текущий рукописный + `LANGUAGE_RUNTIME`) — проблема из issue подтверждена буквально: поля + `germanDictionary`/`germanPending`/`germanFailed`/`settleGerman` существуют + ровно как описано, ни один тест их не импортирует (`grep` по `test/**` — + пусто). +4. Прочитан `test/i18n-runtime.test.mjs` — подтверждает, что класс + `LanguageRuntime` уже кроет dedup/retry/fingerprint/render-gate сценарии, + которые дублирует рукописный объект. +5. Проверены все точки использования `LANGUAGE_RUNTIME` (`houseplan-card.ts`, + `editor.ts`, `space-card.ts`, `space-editor.ts`) — все обращаются только + через контракт `state/dictionary/ensure` (`languageRenderGate(this, + LANGUAGE_RUNTIME, …)`), замена литерала на экземпляр класса безопасна для + всех четырёх поверхностей без изменения их кода (К1 не ломает вызывающих). +6. Проверен существующий тост-паттерн: `_showToast`/`_toast` **только** в + `src/houseplan-card.ts` (`grep -n "toast" editor.ts space-editor.ts + space-card.ts` — пусто). Проверен паттерн `editor.load_failed` + + `lazyLoadFailureMessage` как образец для N7. +7. Проверены существующие паритет-тесты локалей: `test/i18n.test.mjs` + («every registered dictionary carries the English key set», «no empty + values», «placeholders match between languages», German glossary/sentinel + checks) — заявление ТЗ «паритет-тесты уже стерегут» новый ключ + подтверждено, это не догадка. +8. Проверен существующий смок `demo/smoke_german_locale.mjs` (84 строки) — + сценарий «both retries failed → English» уже есть + (`out.failureFallsBackAndUnblocks`, `out.failureWarnsOnce`), в него + действительно можно добавить проверку тоста без новой инфраструктуры смоков. +9. Проверен паттерн подписки на странице (`subscribeLabs`/ + `subscribePageVisibility` в `connectedCallback`/`disconnectedCallback` + `houseplan-card.ts`) — предложенный `subscribeLanguageLoadFailures` + стилистически идентичен уже принятым конвенциям, это не новый паттерн. +10. Сверены соответствия К1 «warn-текст для de байт-в-байт совпадает с + классовым» — оба места используют один и тот же формат + `` `[houseplan] unable to load ${entry.code} locale; using English` ``, + утверждение верно на сегодняшнем коде. + +Гейты не гонялись — стадия ревью ТЗ по коду не работает (кода ещё нет, ветка +не создана); это ожидаемо для S4-spec-review и не является пропуском. + +## Находки + +### Medium (в скоупе, чинится в тексте ТЗ) — 1 + +**M1. Не решено, на каких из четырёх поверхностей рендер-гейта показывается +новый тост N7 (К2).** + +- **Где:** контракт К2 в теле issue #354 — «Карточка подписывается при + подключении... показывает тост». +- **Почему это находка, а не мелочь:** `LANGUAGE_RUNTIME` и + `languageRenderGate` используются на **четырёх** самостоятельных корневых + поверхностях (зафиксировано в каноническом `docs/specs/348-german-localization.md` + §6.3: `houseplan-card`, `houseplan-space-card`, GUI editor `houseplan-card`, + GUI editor `houseplan-space-card`) — все четыре подпишутся на один и тот же + `LANGUAGE_RUNTIME` и все четыре могут столкнуться с отказом загрузки de. + Однако инфраструктура тоста (`_showToast`/`_toast`) существует **только** в + `src/houseplan-card.ts`; в `editor.ts`, `space-editor.ts` и `space-card.ts` + тоста нет вовсе (проверено `grep -n "toast"` по всем трём файлам — пусто + строк). ТЗ говорит «Карточка» в единственном числе и не называет, что + происходит на трёх остальных поверхностях: тихий статус-кво + (`console.warn`, как сейчас) или им тоже нужен тост — а тост на редакторах + означал бы новую UX-поверхность (`_showToast` там ещё не существует) и уже + не укладывался бы в критерий лёгкого трека «нет нового UX-контракта» / «одна + поверхность». +- **Как проявится, если не поправить:** разработчик может по-разному + трактовать «Карточка» — реализовать только `houseplan-card.ts` (наиболее + вероятное и дешёвое прочтение, соответствует единственному существующему + смоку `demo/smoke_german_locale.mjs`, который создаёт только + `houseplan-card`), либо решить, что нужно тащить тост-инфраструктуру и в три + других файла, расширяя скоуп лёгкой задачи без разрешения. AC4 и K2 сейчас + одинаково совместимы с обоими прочтениями — цикл код-ревью не сможет + предъявить претензию ни к одному варианту, потому что ТЗ ничего не + фиксирует. +- **Требуемая правка:** одно предложение в К2, явно называющее это решённым — + предлагаемый вариант по умолчанию (не продуктовый вопрос владельцу, ровно + тот случай, который снимается ревьюером/автором самостоятельно): «Тост + показывается только в `houseplan-card` (View); `space-card`, оба GUI-редактора + продолжают писать только `console.warn`, как до задачи — тост-поверхности там + нет, и заводить её не входит в эту задачу». Это не расширяет объём работы, а + сужает и фиксирует уже существующий фактический скоуп. + +### Low (снимается с записью, не блокирует) — 1 + +**L1. Ключ `i18n.load_failed` не следует конвенции именования тостов.** + +- Все существующие ключи, показываемые через `_showToast`, лежат в + пространстве `toast.*` (`toast.cfg_reload_failed`, `toast.space_order_changed`, + `toast.conflict`, `toast.pos_save_failed`, ещё 30+ примеров в `en.json`) — ни + одного ключа `i18n.*` в репозитории не существует + (`grep -n '"i18n\.' src/i18n/en.json` пусто). Предложенный `i18n.load_failed` + вводит новое, единственное в своём роде пространство имён вместо очевидного + `toast.locale_load_failed` или аналога. +- Не блокирует: паритет-тесты (`test/i18n.test.mjs`) одинаково защищают любой + ключ независимо от префикса, поведенчески это не меняет ничего наблюдаемого + пользователем. +- **Решение ревьюера:** не блокировать, зафиксировать в этом документе. + Автору стоит рассмотреть переименование в `toast.*`-пространство при + реализации ради единообразия со всеми соседними тостами, но это не + критерий приёмки и не повод для повторного цикла. + +## Что проверено и корректно + +- **К1 реализуем без побочных поломок.** Все четыре вызывающих места + (`houseplan-card.ts:10713`, `editor.ts:118`, `space-card.ts:766`, + `space-editor.ts:60`) используют `LANGUAGE_RUNTIME` только через контракт + `LanguageRuntimeContract` (`state`/`dictionary`/`ensure`), которому + `LanguageRuntime`-инстанс соответствует по построению — замена литерала на + `new LanguageRuntime(...)` их не касается. +- **Публичная поверхность действительно не меняется** (раздел «Откат»): + `dictionaryFor`/`ensureLanguage`/`languageOptions` — сигнатуры и экспорт + сохраняются, откат — один revert. +- **AC1–AC3 однозначны и механически проверяемы:** grep по + `germanPending|germanFailed|settleGerman`, `instanceof`-юнит, мутационный тест + с инлайн-литералом вместо `new LanguageRuntime(...)` — все три воспроизводимы + без дополнительных решений на этапе кода. +- **Заявление «паритет-тесты уже стерегут» — не догадка**, а точное описание + существующего `test/i18n.test.mjs` (проверено чтением тест-файла). +- **Заявление «warn-текст байт-в-байт совпадает с классовым» — верно** на + сегодняшнем коде обоих файлов. +- **N7 использует существующий паттерн `_showToast`**, а не изобретает новый + UX-примитив (кроме открытого вопроса M1 о том, где именно он вызывается). +- **Подписка/отписка (К2) соответствует уже принятой конвенции** + `subscribeXxx(...)`/`_xxxUnsub` в `connectedCallback`/`disconnectedCallback`, + а не является новым архитектурным паттерном. +- Формат ТЗ для лёгкого трека соблюдён: проблема · контракт · AC1…AC4 с + доказательством · откат — все четыре раздела на месте (§5 PROCESS.md). +- Трейлер `User-Visible: yes` в теле issue корректен — задача добавляет + видимый тост. + +## Чего не проверял + +- Код по задаче ещё не написан (стадия ТЗ), поэтому гейты `tsc`/`test`/`build` + не запускались — на этом этапе они неприменимы, а не пропущены. +- Не проверял реальный текст немецкого/русского перевода нового ключа — его + ещё нет; при код-ревью нужно свериться с паритет-тестами и glossary из + `docs/specs/348-german-localization.md`. +- Не проверял, действительно ли `subscribeLanguageLoadFailures`-колбэк способен + получить локаль-код `de` из `warn(message, error)` (сигнатура класса передаёт + только текстовое сообщение, не структурированный код). Технически это + решаемо уже сегодня (в реестре ровно один ленивый locale, `de` — код может + быть захвачен замыканием при построении `warnWithNotify`, без парсинга + строки), поэтому не поднимаю как отдельную находку, но это стоит явно + проговорить при код-ревью, если добавится второй ленивый язык. + +## Раунд не первый? — не применимо + +r1: разделы «Закрытие раунда r0» и «Унаследовано из r0» не пишутся — +предыдущего цикла ревью по этой задаче нет. + +## Итог + +Вердикт **жёлтый**: High-находок нет, но одна Medium-находка (M1) — скоуп +тоста N7 по четырём render-gate поверхностям не зафиксирован, хотя факт +асимметрии (только `houseplan-card.ts` имеет тост-инфраструктуру) уже виден в +коде и должен быть явно записан как решение, а не оставлен на усмотрение +исполнителя. Автор правит ТЗ (одно предложение в К2), Low-находка (L1) +зафиксирована и не требует правки для перехода в `S5-ready`.