diff --git a/docs/reviews/CODE-REVIEW-377-r2.md b/docs/reviews/CODE-REVIEW-377-r2.md new file mode 100644 index 00000000..5bd6b09b --- /dev/null +++ b/docs/reviews/CODE-REVIEW-377-r2.md @@ -0,0 +1,223 @@ +# CODE-REVIEW-377-r2 + +- Issue: https://github.com/Matysh/houseplan-card/issues/377 +- Заход: r2 (номер — для имени документа; предыдущий код-ревью, r1, был + зелёным и цикл не тратил, см. §4/#227) +- Ветка: `issue/377-decor-default-persist`, HEAD `82d710f1` +- SHA материала ревью: `82d710f160d8572ef6687499c4b8acdc7995ed45` + (`git rev-parse HEAD`) +- Диапазон: `git diff origin/dev...HEAD` — `origin/dev` = `720200e3` + (текущий тип), 43 файла, +1008/-488 + +## Почему это ПОЛНЫЙ разбор, а не разбор по дельте + +Между r1 и r2 ветка была **ребейзнута на ушедший вперёд `dev`**: r1 вынесен +на материале `2edaf1adac60f9939b836da87f5f1f549dab2548` — этого объекта в +репозитории больше нет (`git cat-file -t` → `fatal: could not get object +info`), он выпал при ребейзе. Автор сам зафиксировал это в issue сразу после +публикации r1: «Пока шло ревью, `dev` продвинулся на 7 коммит(ов)... слияние +приведёт ветку к dev, и это другой код (§7.2)» и следом провёл ребейз, +затянувший в базу #373 (`feat: add tight house framing to space card`, +`0d33691f`). Это ровно исключение из инструкции («ребейз на ушедший вперёд +dev — после ребейза это другой код»), поэтому объём разбора в r2 — полный, +а не только по находкам r1 (которых, забегая вперёд, и не было ни одной +блокирующей). + +`git merge-base HEAD origin/dev` = `origin/dev` — ветка полностью лежит на +текущей вершине `dev`, конфликтов нет, `git grep '<<<<<<<'` по дереву чист. + +## Скоуп + +Без изменений с r1: персист серверного ключа `settings.decor_default_style` +(цвет/стиль декора «по умолчанию» из тулбара Background-редактора, #360) — +переживает перезагрузку, общий для всех редакторов плана. Три поверхности: +схема (`validation.py`), инициализация (`houseplan-card.ts`), запись +(`houseplan-editor-runtime.ts`). + +## Как проверялось + +Два независимых источника уверенности, оба обязательны для полного разбора +после ребейза: + +**1. Текстовое сличение продуктового кода #377 «было vs стало».** Для +каждого из файлов, которые правит именно #377 (`validation.py`, +`src/editors/decor/geometry.ts`, `src/houseplan-card.ts`, +`src/houseplan-editor-runtime.ts`, `test/decor-style-persist.test.mjs`, +`demo/smoke_decor_default_persist.mjs`, `tests_backend/test_validation.py`) +взял текущий `git diff origin/dev...HEAD` построчно и сверил его с тем, что +описывает и цитирует `docs/reviews/CODE-REVIEW-377-r1.md` (сам файл — часть +этого диапазона, довезён ребейзом через cherry-pick, как и написал автор). +Совпадает дословно: тот же маппинг 6 полей в `decorStyleFromSettings`/ +`decorStyleToSettings`, тот же `_seedDecorStyle` (единственный сдвиг — +номера строк из-за более раннего кода, добавленного #373 в этом же файле), +тот же `_updateDecorStyle`/`_persistDecorStyle` с дебаунсом 1000 мс и все +шесть UI-точек, переведённые на него, та же схема `validation.py`, тот же +набор из 10 мусорных вариантов + `"solid"` в pytest, тот же смок. Значит +семантика правки не изменилась — только база под ней. + +**2. Проверка, что ребейз безопасен по существу**, а не только по +отсутствию текстовых конфликтов: `#373` (единственный feat-коммит, +затянутый в базу между r1 и r2) правит только `src/space-geometry.ts` — три +новых экспорта (`resolveSpaceCardFit`, `itemOfGeometry`, `expandItem`, +`structuralFrame`), все аддитивные, используются только `src/space-card.ts`. +Ни один из файлов #377 (`houseplan-card.ts`, `houseplan-editor-runtime.ts`, +`editors/decor/geometry.ts`, `validation.py`) не пересекается с диффом #373. +Разобрано чтением: общих символов между двумя изменениями нет, путь записи +декора (`_updateDecorStyle` → `_saveConfig` → `expected_rev`) и путь фрейминга +space-card не имеют точек соприкосновения. + +**3. Гейты — прогнаны заново лично на актуальном SHA** (зелёного Validate +на `82d710f1` нет — см. плашку задачи), поскольку старый прогон r1 стоял на +объекте, которого больше не существует, и переносить его результат без +повторного прогона на новой базе было бы недопустимо. + +### Гейты — прогнал сам + +| Гейт | Команда | Результат | +|---|---|---| +| Typecheck | `npx tsc --noEmit` | чисто | +| Юниты | `npm test` | 1567 pass / 0 fail / 1 skipped (совпадает с заявкой автора после ребейза: «юниты 1567/0») | +| Сборка | `npm run build` | ok, 9.6s | +| Синхронизация бандлов | `npm run bundle:sync` | ok; `git status --porcelain` после — пусто (три копии дерева байт-в-байт совпадают с закоммиченными) | +| Бюджет | `npm run bundle:budget` | initial View 277 310 Б / 300 000 (запас 22 690 Б) — байт-в-байт совпадает с заявкой автора после ребейза | +| Доки | `node scripts/check-docs.mjs` | «Documentation checks passed (7 files, 10 external links)» — diff трогает `src/**` (в т.ч. через #373), гейт обязателен и зелёный. Отпечаток скриншотов (`sourceFingerprint`) обновлён коммитом `40ba222a` вместе с 10 канонических PNG — ожидаемо: любая правка `src/**` делает старый отпечаток устаревшим, а #373 такую правку внесла | +| Backend (чистое подмножество) | `python -m pytest tests_backend -q --ignore=tests_backend/test_coordinate_canonicalization.py` (после `pip install pytest voluptuous`) | 235 passed, 1 skipped — совпадает с заявкой автора и с r1. `test_coordinate_canonicalization.py` падает на отсутствующем `homeassistant` при коллекции — не связано с #377, гейт CI | +| Единственный источник числа | `node --test test/single-source-numbers.test.mjs` | 3/3 pass | +| Мутанты (только новые, точечно `--id=`) | `mutation-gate.mjs --id=decor-default-style-seed-cut` и `--id=decor-default-style-debounce-cut` | оба «поймано 1 из 1» — тесты умеют падать | + +Полный `mutation-gate.mjs` без `--id=` не гонял — та же причина, что в r1: +несоразмерно точечной правке, к тому же в этом окружении падает на +несвязанном backend-guard'е (нет `homeassistant`). + +### Смоки — `scripts/smoke-select.mjs --base origin/dev --head HEAD` + +Инструмент на актуальном диапазоне дал те же 7 «прямых совпадений» и те же +13 «слабых связей» (единое имя `_saveConfig`), что и в r1 — состав диффа +#377 не изменился, поэтому выбор не изменился. + +Прогнал все 7 прямых совпадений — все OK: `smoke_decor_default_persist` +(AC4/AC5), `smoke_bg_color`, `smoke_color_picker`, `smoke_decor`, +`smoke_dialog_zombie`, `smoke_furniture`, `smoke_general_settings`. + +Из 13 слабых связей прогнал `smoke_config_writer` (та же, что в r1 и что +заявлял автор) — OK. Остальные 12 не гонял: diff #377 по-прежнему не трогает +геометрию/стены/толщину, а #373 (единственное новое в базе) отдельно +проверено на отсутствие пересечения файлов (см. выше) — обоснование +переносится из r1 без изменений. `npm run invariants` не требовался — diff +#377 не трогает рёбра комнат, `layout`, `marker.space`, `open_spans` или +записи толщины. + +`npm run golden:verify` не гонял: продуктовый код #377 не рендерит View +(`_decorStyle` управляет только дефолтом для новых объектов декора/мебели в +редакторе, не View) — это не изменилось с r1. Визуальные изменения в +диапазоне (10 канонических PNG) целиком принадлежат #373 (fit-to-content +пространства) и уже прошли отдельный цикл ревью этой задачи (`docs: review +document for #373`, трижды в истории `dev`) — повторно ревьюить чужой +змерженный и принятый результат в рамках #377 не требуется. + +## Закрытие раунда r1 + +| Находка r1 | Чем закрыта | Где видно | +|---|---|---| +| Low: `_updateDecorStyle` заводит свой `setTimeout(1000)` и не флашит его в `disconnectedCallback` | Не чинилась — r1 снял находку без правки как приемлемый риск (цена ошибки мала, штатный `disconnectedCallback` не единственный путь доставки таймера) | `houseplan-editor-runtime.ts:9403-9411` (номера строк сдвинуты ребейзом, код идентичен); решение зафиксировано в `docs/reviews/CODE-REVIEW-377-r1.md`, раздел «Находки» | + +High/Medium-находок в r1 не было — таблица закрытия по ним пуста, возврата +на правки не было. + +## Унаследовано из r1 + +Полностью, без переоткрытия по существу, наследуется анализ AC1–AC7 и +раздел «Что проверено и корректно» из `docs/reviews/CODE-REVIEW-377-r1.md` +(SHA материала того ревью: `2edaf1adac60f9939b836da87f5f1f549dab2548`, +документ смёржен в ветку cherry-pick'ом при ребейзе) — построчная сверка +выше (раздел «Как проверялось», пункт 1) подтвердила, что продуктовый код +#377 между r1 и r2 не изменился ни на символ, изменилась только база под +ним. В частности без повторной сверки по существу приняты: + +- маппинг snake_case↔camelCase в `decorStyleFromSettings`/ + `decorStyleToSettings` и соответствие ТЗ п.7; +- логика однократного посева `_seedDecorStyle` из кэша и первого серверного + ответа, не переоткрывающая живую сессию; +- то, что запись идёт исключительно штатным сериализованным путём + (`_saveConfig` → `_saveConfigDebounced` → `_writeConfig` → + `_sendConfigCandidate` с `expected_rev`); +- то, что все шесть UI-точек изменения стиля переведены на + `_updateDecorStyle` (сверка «ни одна не забыта» из r1); +- разбор AC6 через чтение `import_export.py` (путь `settings` для + non-same-source импорта не трогает `decor_default_style`); +- разбор AC7 (недостижимость в холодном View — код рантайма грузится лениво + и нигде вне него `_decorStyle` не пишется). + +Не наследуется, а перепроверено заново в r2: все автоматические гейты (иной +SHA — не тот же прогон), совместимость с #373 (новое в базе, r1 об этом +знать не мог), актуальность отпечатка скриншотов документации. + +## Находки + +Новых находок в r2 нет. High/Medium — 0. + +**Low (унаследована из r1, по-прежнему снята без правки).** См. таблицу +«Закрытие раунда r1» выше — код не менялся, довод не изменился. + +**Наблюдение вне находок.** Коммит `40ba222a` (`build: refresh bundle trees +for #377; align the color-picker contract`) помимо ожидаемого пересбора +бандлов и обновления `test/color-picker.test.mjs:67` (замена ожидаемого +паттерна `this._decorStyle = {...}` на `this._updateDecorStyle({...})` — +тест синхронизирован с уже существующим кодом `feat`-коммита, а не наоборот: +сверено, что `houseplan-editor-runtime.ts` действительно вызывает +`_updateDecorStyle` в этом месте с самого `feat`-коммита `10a1a266`). Это +задним числом закрывает пробел покрытия, оставленный в `feat`-коммите +(тест не был обновлён вместе с кодом), но не меняет поведение и не +затрагивает AC — не поднимаю до Low, так как ошибки не произошло: `npm +test` был бы красным до этого коммита, и это увидели бы до публикации +ветки на ревью, что и произошло (гейт-перегон автора после ребейза). + +## Что проверено и корректно (специфично для r2) + +- Ребейз чист: `git merge-base HEAD origin/dev` = `origin/dev`, маркеров + конфликта нет. +- Диапазон #377 (файлы, диффы, коммиты) не пересекается по файлам с #373 — + проверено построчным чтением диффа #373. +- Все гейты, зелёные в r1, остаются зелёными на новой базе; числовые + показатели (юниты, backend, бюджет бандла) совпадают с тем, что заявил + автор в двух комментариях после ребейза («юниты 1567/0», «бюджет 277 + 310/300 000»). +- Трейлеры (`Issue: #377`, `User-Visible`) на месте во всех 4 коммитах + диапазона; `feat`-коммит (`User-Visible: yes`) правит оба changelog и оба + USER-GUIDE в себе же, не отдельным коммитом. +- `docs/reviews/CODE-REVIEW-377-r1.md` присутствует в дереве как обычный + файл истории ревью, довезённый ребейзом — само по себе не находка. + +## Чего не проверял + +- Заново — семантику продуктового кода #377 построчно «с нуля»: сверил + текстовым сличением с уже проверенным в r1 кодом (см. «Как проверялось», + п.1) вместо повторного независимого прочтения каждой строки — обоснование + выше (§7.2 требует полноты разбора при таком ребейзе, но не требует + игнорировать то, что код внутри диапазона #377 доказуемо не менялся). +- Визуальную корректность фрейминга space-card (#373) — чужая, уже принятая + задача, вне скоупа #377. +- Полный `npm run golden:verify` и полный `demo/smoke_*.mjs` (все ~204) — + не оправдано объёмом правки #377; обоснование — выше. +- Полный HA backend harness (`test_ha_*.py`, + `test_coordinate_canonicalization.py`) — недоступен в этом окружении (нет + `homeassistant`), гейт CI/бета, расхождение не связано с #377. +- Живое ручное тестирование в браузере — замещено чтением кода и прогоном + автотестов/смоков. +- Гонку двух вкладок (#340) — не тронута этой задачей, отдельно не + воспроизводил, как и в r1. + +## Вердикт + +Ребейз на ушедший вперёд `dev` потребовал полного разбора (§7.2): проведена +как текстовая сверка продуктового кода #377 «было vs стало» (не изменился ни +на символ), так и проверка отсутствия пересечения с тем, что реально +привнёс новый `dev` (#373 — аддитивные, несвязанные файлы). Все 7 AC +доказаны — унаследовано из r1 с подтверждением, что доказательство всё ещё +соответствует коду. Все гейты, соразмерные объёму задачи, прогнаны заново на +актуальном SHA и зелёные. High/Medium-находок нет, одна Low унаследована из +r1 и остаётся снятой без правки. + +Вердикт: зелёный · заход r2 · блокирующих циклов 0/4 · High: 0 · Medium: 0 + +Готово к очереди на пре-релиз.