diff --git a/docs/reviews/SPEC-REVIEW-354-r1.md b/docs/reviews/SPEC-REVIEW-354-r1.md index 7776f7c6..3bc50d0a 100644 --- a/docs/reviews/SPEC-REVIEW-354-r1.md +++ b/docs/reviews/SPEC-REVIEW-354-r1.md @@ -1,188 +1,173 @@ -# SPEC-REVIEW-354-r1 +# SPEC-REVIEW-354-r2 - **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`, рабочее дерево чистое) -- **Вердикт: жёлтый** +- **Заход:** r2 · блокирующих циклов израсходовано 1 из 2 (лимит ТЗ на лёгком треке — 2; r1 был жёлтым и потратил 1 цикл — §4/#227) +- **ТЗ:** тело issue #354, «Ревизия 2» (файл в `docs/specs/` отсутствует — корректно для `small`) +- **SHA на момент ревью:** `149b2af7` (`dev`, рабочее дерево чистое; с момента r1 (`77302eaf`) в код не внесено ни одного продуктового коммита — единственный новый коммит `149b2af7` кладёт документ ревью r1, продуктового/тестового кода не касается) +- **Вердикт: зелёный** + +## Примечание о нумерации захода + +Стартовые данные этой сессии указывали «Заход: r1 · блокирующих циклов +израсходовано 0 из 2». Это не соответствует фактическому состоянию: в +`docs/reviews/SPEC-REVIEW-354-r1.md` (закоммичен `149b2af7`) и в комментариях +issue уже есть завершённый раунд r1 — жёлтый вердикт от `claude` (комментарий +`IC_kwDOTOcLQM8AAAABRRPAyw`, 2026-08-28T14:40:01Z, SHA `77302eaf`), за которым +последовала правка автора («Ревизия 2 ТЗ по r1», комментарий +`IC_kwDOTOcLQM8AAAABRRPa0g`) и возврат метки `S4-spec-review`. Раунд, который +разбирает эта сессия, — второй по факту (`r2`), а не первый; бюджет цикла к +этому моменту уже потратил 1 из 2 (жёлтый вердикт r1 расходует бюджет по §4). +Документ поэтому назван и пронумерован как `r2`, вопреки стартовым данным — +иначе он затёр бы уже существующий `SPEC-REVIEW-354-r1.md` или конфликтовал +бы с ним по номеру, ровно тот сценарий, от которого предостерегает сам номер +захода. Расхождение стартовых данных со стоянием репозитория — само по себе +не блокирует эту задачу, но стоит показать тому, кто обслуживает конвейер. ## Скоуп -Задача техдолга: продакшн-объект `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`. +Без изменений относительно r1: собрать продакшн-`LANGUAGE_RUNTIME` в +`src/i18n/registry.ts` как `new LanguageRuntime(...)` вместо рукописного +литерала (`germanDictionary`/`germanPending`/`germanFailed`/`settleGerman`), +чтобы существующий `test/i18n-runtime.test.mjs` действительно проверял код +продакшна, плюс попутный (N7) тост при отказе загрузки словаря локали. +Ревизия 2 не меняет предмет задачи и не расширяет её — правит формулировку +одного контракта (К2) и переименовывает один i18n-ключ. Конфликта со +`docs/SCOPE.md` по-прежнему нет (см. SPEC-REVIEW-354-r1.md, раздел «Скоуп» — +наследуется, ниже). -По `docs/SCOPE.md`: задача — техдолг вокруг J4/J6 (инфраструктура i18n из #62), -пользовательского функционала не добавляет, кроме одного нового тоста (N7). -Отдельного раздела «Job» не требует — это внутренняя правка тестируемости плюс -мелкое UX-улучшение по уже существующему паттерну (`editor.load_failed`). -Конфликта со SCOPE.md нет. +## Дельта r1 → r2 -## Как проверялось +`git diff ..HEAD` не показателен: SHA r1 (`77302eaf`) и текущий +(`149b2af7`) отличаются только doc-коммитом ревью r1, продуктового кода нет +вообще (стадия ТЗ). Предмет дельты — правка тела issue #354, зафиксированная +автором как «Ревизия 2 ТЗ по r1» (комментарий `IC_kwDOTOcLQM8AAAABRRPa0g`). +Сверено построчно с текстом, процитированным в `SPEC-REVIEW-354-r1.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` ``, - утверждение верно на сегодняшнем коде. +1. **К2** — было (цит. по r1, находка M1): «Тост показывается ... Карточка + подписывается при подключении... показывает тост» (без указания, какая из + четырёх поверхностей рендер-гейта). Стало: «Тост показывается ТОЛЬКО в + `houseplan-card` (View — единственная поверхность с тост-инфраструктурой + `_showToast`)... Остальные три поверхности рантайма (`space-card`, + GUI-редакторы обеих карточек) остаются при `console.warn`, как сейчас, — + тост-инфраструктура там не заводится.» +2. **Ключ тоста** — было `i18n.load_failed` (находка L1). Стало + `toast.locale_load_failed`, в К2 и в AC4. +3. Остальной текст (К1, К3, AC1–AC3, «Откат», трейлер `User-Visible: yes`) + не изменился — сверено посимвольно с цитатами в `SPEC-REVIEW-354-r1.md` + (разделы «Что проверено и корректно», «Скоуп»). -Гейты не гонялись — стадия ревью ТЗ по коду не работает (кода ещё нет, ветка -не создана); это ожидаемо для S4-spec-review и не является пропуском. +Дельта локальна (правка формулировки одного контракта плюс переименование +одного ключа), не задевает новую подсистему, не меняет контракт поведения +сверх уже согласованного в r1, по объёму несопоставима с исходной задачей — +условие «разбор остаётся полным» (PROCESS.md §2.10) не наступает. Разбор +этого раунда ограничен дельтой: заново проверялись только К2 (N7) и ключ +тоста — единственное, что дельта задевает. + +## Как проверялось (r2) + +1. Прочитано текущее тело issue #354 целиком, сверено с цитатами r1 — + найдены ровно две правки (см. «Дельта» выше), больше отличий нет. +2. Прочитан комментарий владельца «Ревизия 2 ТЗ по r1» — подтверждает, что + обе правки целенаправленно закрывают M1 и L1, а не что-то другое. +3. Перечитан текст К2 revision 2 на однозначность: названа ровно одна + поверхность (`houseplan-card`/View), явно перечислены три поверхности, + остающиеся при `console.warn`, явно сказано «тост-инфраструктура там не + заводится» — двух прочтений эта формулировка больше не допускает. +4. `grep -n '"toast\.' src/i18n/en.json` — подтверждён неймспейс `toast.*` + (40+ существующих ключей); `grep -rn "locale_load_failed\|i18n.load_failed" + src/ test/ demo/` — пусто: новый ключ не конфликтует с существующим кодом + и ещё не реализован (ожидаемо для стадии ТЗ). +5. `grep -n "6.3\|houseplan-space-card" docs/specs/348-german-localization.md` + — подтверждён раздел «6.3. Render gate» и список из четырёх поверхностей, + на который опирается формулировка К2 (та же ссылка, что использовал r1). +6. `git log 77302eaf..HEAD --oneline` — один коммит (`149b2af7`, doc r1), + без изменений в `src/**`/`test/**`: код и его состояние идентичны + зафиксированным в r1, повторная проверка К1/К3/AC1–AC3 по коду не нужна. + +Гейты (`tsc`/`test`/`build`) не гонялись — стадия ревью ТЗ, кода по задаче ещё +нет; как и в r1, это не пропуск, а неприменимость этапа. ## Находки -### Medium (в скоупе, чинится в тексте ТЗ) — 1 +Нет. High: 0, Medium: 0, Low: 0. -**M1. Не решено, на каких из четырёх поверхностей рендер-гейта показывается -новый тост N7 (К2).** +Обе находки r1 закрыты правкой текста, новых находок дельта не вносит. -- **Где:** контракт К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`, как до задачи — тост-поверхности там - нет, и заводить её не входит в эту задачу». Это не расширяет объём работы, а - сужает и фиксирует уже существующий фактический скоуп. +## Закрытие раунда r1 -### Low (снимается с записью, не блокирует) — 1 +| Находка r1 | Чем закрыта | Где это видно | +|---|---|---| +| **M1** (Medium, в скоупе) — К2 не называл, на какой из четырёх поверхностей рендер-гейта показывается тост N7; формулировка «Карточка» в единственном числе была совместима с двумя разными объёмами работы. | К2 переписан: «Тост показывается ТОЛЬКО в `houseplan-card` (View...)... Остальные три поверхности рантайма (`space-card`, GUI-редакторы обеих карточек) остаются при `console.warn`, как сейчас, — тост-инфраструктура там не заводится.» Формулировка совпадает с предложенным в r1 вариантом по умолчанию почти дословно. | Тело issue #354, раздел «Контракт», К2, первый и последний абзацы. | +| **L1** (Low, зафиксирована) — ключ `i18n.load_failed` был единственным вне пространства `toast.*`. | Ключ переименован в `toast.locale_load_failed` — и в К2, и в AC4. | Тело issue #354, К2 («новым ключом `toast.locale_load_failed`») и AC4 («текстом `toast.locale_load_failed`»). | -**L1. Ключ `i18n.load_failed` не следует конвенции именования тостов.** +## Унаследовано из r1 -- Все существующие ключи, показываемые через `_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.*`-пространство при - реализации ради единообразия со всеми соседними тостами, но это не - критерий приёмки и не повод для повторного цикла. +Без повторной проверки в этом раунде — код и тесты с r1 не менялись +(единственный новый коммит — doc-коммит ревью, класс C), проверка велась +только над текстом ТЗ: -## Что проверено и корректно +- **Скоуп задачи и соответствие `docs/SCOPE.md`** — принято по + `SPEC-REVIEW-354-r1.md`, раздел «Скоуп» (SHA `77302eaf`). +- **К1 (замена литерала на `new LanguageRuntime(...)`) реализуема без + побочных поломок** — все четыре вызывающих места используют только + контракт `state/dictionary/ensure` — принято по `SPEC-REVIEW-354-r1.md`, + раздел «Что проверено и корректно», пп. 1–2 (SHA `77302eaf`, строки кода + `houseplan-card.ts:10713`, `editor.ts:118`, `space-card.ts:766`, + `space-editor.ts:60`). +- **АС1–АС3 однозначны и механически проверяемы** (grep, instanceof-юнит, + мутационный тест на К3) — принято по `SPEC-REVIEW-354-r1.md`, раздел «Что + проверено и корректно» (SHA `77302eaf`); текст АС1–АС3 в ревизии 2 не + менялся (сверено построчно, см. «Дельта» выше). +- **Паритет-тесты (`test/i18n.test.mjs`) действительно стерегут новый ключ + независимо от его имени** — принято по `SPEC-REVIEW-354-r1.md`, пп. 7 и + L1 (SHA `77302eaf`); вывод не зависит от конкретного имени ключа, поэтому + переименование в `toast.locale_load_failed` его не меняет. +- **Существующий смок `demo/smoke_german_locale.mjs` уже содержит сценарий + «оба ретрая упали → English»**, в который AC4 добавляет проверку тоста без + новой инфраструктуры смоков — принято по `SPEC-REVIEW-354-r1.md`, п. 8 + (SHA `77302eaf`). +- **Подписка/отписка (К2) соответствует принятой конвенции + `subscribeXxx(...)`** — принято по `SPEC-REVIEW-354-r1.md`, п. 9 (SHA + `77302eaf`). +- **Открытый нефинирующий пункт для код-ревью**: не проверялось, как + `subscribeLanguageLoadFailures`-колбэк получит код локали `de` из + `warn(message, error)` (сигнатура класса передаёт только текст). Не + находка (решаемо замыканием при единственном сегодня ленивом языке), но + стоит явно проговорить на код-ревью, если добавится второй ленивый язык — + перенесено из `SPEC-REVIEW-354-r1.md`, раздел «Чего не проверял». -- **К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 корректен — задача добавляет - видимый тост. +## Что проверено и корректно (r2, по дельте) + +- К2 revision 2 однозначен: ровно одна поверхность для тоста + (`houseplan-card`), явно перечислены три поверхности без тоста, явно + сказано, что новая тост-инфраструктура на них не заводится — двух + прочтений формулировка не допускает, находка M1 закрыта по существу, а не + косметически. +- Новый ключ `toast.locale_load_failed` следует существующей конвенции + именования тостов (проверено grep по `src/i18n/en.json`, 40+ ключей в том + же пространстве) и не конфликтует с уже существующим кодом (grep пуст). +- Правка не расширяет скоуп и не вводит новый UX-контракт: критерии лёгкого + трека (§5 PROCESS.md) по-прежнему выполнены — ревизия 2 их не колеблет + (одна поверхность тоста явно зафиксирована как единственная, а не + расширена на четыре). +- АС4 (после ревизии) по-прежнему проверяем механически: юнит на + `subscribeLanguageLoadFailures`, смок `smoke_german_locale` с проверкой + `card._toast`, мутационный тест на «тост выброшен». ## Чего не проверял -- Код по задаче ещё не написан (стадия ТЗ), поэтому гейты `tsc`/`test`/`build` - не запускались — на этом этапе они неприменимы, а не пропущены. -- Не проверял реальный текст немецкого/русского перевода нового ключа — его - ещё нет; при код-ревью нужно свериться с паритет-тестами и glossary из - `docs/specs/348-german-localization.md`. -- Не проверял, действительно ли `subscribeLanguageLoadFailures`-колбэк способен - получить локаль-код `de` из `warn(message, error)` (сигнатура класса передаёт - только текстовое сообщение, не структурированный код). Технически это - решаемо уже сегодня (в реестре ровно один ленивый locale, `de` — код может - быть захвачен замыканием при построении `warnWithNotify`, без парсинга - строки), поэтому не поднимаю как отдельную находку, но это стоит явно - проговорить при код-ревью, если добавится второй ленивый язык. - -## Раунд не первый? — не применимо - -r1: разделы «Закрытие раунда r0» и «Унаследовано из r0» не пишутся — -предыдущего цикла ревью по этой задаче нет. +- Код по задаче ещё не написан (стадия ТЗ) — гейты `tsc`/`test`/`build` не + запускались, как и в r1; на этой стадии они неприменимы. +- Не проверял заново K1/K3/AC1–AC3 по коду — код с момента r1 не менялся + (см. «Дельта» и «Унаследовано из r1» выше), текст этих пунктов ТЗ тоже не + менялся. +- Не проверял реальные тексты переводов для `toast.locale_load_failed` + (en/ru/de) — их ещё нет; сверка с паритет-тестами и glossary + `docs/specs/348-german-localization.md` — задача код-ревью. ## Итог -Вердикт **жёлтый**: High-находок нет, но одна Medium-находка (M1) — скоуп -тоста N7 по четырём render-gate поверхностям не зафиксирован, хотя факт -асимметрии (только `houseplan-card.ts` имеет тост-инфраструктуру) уже виден в -коде и должен быть явно записан как решение, а не оставлен на усмотрение -исполнителя. Автор правит ТЗ (одно предложение в К2), Low-находка (L1) -зафиксирована и не требует правки для перехода в `S5-ready`. +Вердикт **зелёный**: обе находки r1 (M1 Medium, L1 Low) закрыты точечной +правкой текста, дельта не вносит новых находок и не задевает то, что r1 уже +принял. Issue переходит в `S5-ready`.