From 63eea47ac6d2cd39009eca7d7362a80e5d64327d Mon Sep 17 00:00:00 2001 From: "claude[bot]" <41898282+claude[bot]@users.noreply.github.com> Date: Tue, 18 Aug 2026 17:51:16 +0000 Subject: [PATCH] docs: review document for #173 Issue: #173 User-Visible: no --- docs/reviews/CODE-REVIEW-173-r2.md | 220 +++++++++++++++++++++++++++++ 1 file changed, 220 insertions(+) create mode 100644 docs/reviews/CODE-REVIEW-173-r2.md diff --git a/docs/reviews/CODE-REVIEW-173-r2.md b/docs/reviews/CODE-REVIEW-173-r2.md new file mode 100644 index 00000000..47ad805a --- /dev/null +++ b/docs/reviews/CODE-REVIEW-173-r2.md @@ -0,0 +1,220 @@ +# CODE-REVIEW-173-r2 + +- **Issue:** https://github.com/Matysh/houseplan-card/issues/173 +- **Диапазон:** `origin/dev..HEAD`, 5 коммитов: `8a25b52` (ТЗ), `e206e87` + (ревью ТЗ), `fc1e795` `feat: unify Plan wall drawing` (единственный коммит + класса A/B), `aac2978` (документ `CODE-REVIEW-173-r1.md`), `f54b9c0` + (`docs: refresh screenshot fingerprint for #173`) +- **Роль:** ревьюер кода (не автор), этап `S7-code-review`, сессия без + контекста реализации +- **Цикл:** r2/4 — **не по находкам**. r1 (`docs/reviews/CODE-REVIEW-173-r1.md`) + дал зелёный вердикт с High:0, Medium:2 (обе вынесены отдельными issue, + #176/#177, не блокировали); слияние в `dev` конфликтовало с параллельно + влившимся #174, задача вернулась в `S6-in-progress` для ребейза, а не для + правок по находкам ревью (комментарий владельца в issue). После ребейза на + актуальный `dev` (`1f11f8f`, объединяющий #173 и #174) и docs-only коммита, + чинящего screenshot-fingerprint, издана `S7-code-review` заново. Это другой + код (переигранный поверх ушедшего вперёд `dev`), поэтому повторная проверка + не формальность. + +## Скоуп ревью + +`fc1e795` — тот же продуктовый диф, что и в r1 (`f931159` до ребейза): 28 +файлов, `+3304/-1926`, идентичный набор файлов и идентичные суммы +insertions/deletions. Это сильный признак того, что при ребейзе патч +`src/wall-face-graph.ts` / `src/houseplan-card.ts` / тестов / smoke применился +без текстового конфликта — конфликтовали только файлы, которые параллельно +менял #174 (`docs/specs/README.md`, оба `CHANGELOG*`, три копии бандла). +Отдельно проверено (см. ниже), что слияние в этих файлах корректно сохранило +записи обоих issue. + +Новое в r2 относительно r1: + +- `fc1e795` перебазирован на `dev`, включающий #174 (`S8-merged`); +- `f54b9c0` — docs-only (`docs/images/*.png`, `docs/images/screenshots.json`), + обновляет `sourceFingerprint` после объединения #173+#174 в общий `src/`. + +Не в скоупе изменений (не тронуто ни в `fc1e795`, ни в `f54b9c0`): +`custom_components/houseplan/**/*.py`, backend schema, `demo/golden/**`. + +## Что именно проверялось в r2 (сверх r1) + +Поскольку продуктовый код (`fc1e795`) идентичен по diffstat уже +провалидированному в r1, r2 сфокусирован на: (а) действительно ли текущее +дерево green на гейтах, а не только по отчёту в issue; (б) не испортило ли +слияние с #174 общие файлы; (в) легитимность нового docs-only коммита; (г) +что Medium-находки r1 не потерялись и не были тихо «закрыты» вместо issue. +Алгоритмический разбор `wall-face-graph.ts`, построчная сверка AC1–AC17 по +существу и мутационная проверка «тест умеет падать» не повторялись заново +пофайлово — они зафиксированы в `docs/reviews/CODE-REVIEW-173-r1.md` и не +имеют оснований измениться, так как сам продуктовый диф не изменился. + +## Как проверялось — гейты + +| Гейт | Команда | Результат | +|---|---|---| +| typecheck | `npx tsc --noEmit` | pass, без ошибок | +| unit | `npm test` | **848/848 pass** (после слияния с #174 набор больше r1-шного 843/843 на 5 тестов #174 — ожидаемо) | +| build + sync бандла | `npm run build && cmp dist/houseplan-card.js custom_components/houseplan/frontend/houseplan-card.js && cmp dist/houseplan-card.js demo/srv/assets/houseplan-card.js` | pass, три копии побайтно идентичны, SHA-256 `00391dde7211db685c6c3c83c06b53808fb9a8a6a6c6c6b91b8346c5b27fc1ea` — совпадает с числом, которое владелец привёл в хендоффе ребейза | +| целевой smoke (AC1–AC13) | `node demo/smoke_unified_wall_tool.mjs` | **19/19 pass** | +| регрессия #138/AC7 | `node demo/smoke_room_autoclose.mjs` | 9/9 pass | +| регрессия толщины (AC1/AC12) | `node demo/smoke_draw_wall_thickness.mjs` | 11/11 pass | +| touch safety floor (AC3/AC16) | `node demo/smoke_editor_gestures.mjs` | 5/5 pass | +| performance (AC15) | `npm run benchmark:large-house-plan-snap` | pass, без брошенных contract-ошибок; `wallFaceAcceptedClickMs` ≈ 2.8–4.3 мс (порог 1000 мс); `wallFaceGraph` cache entries = 2, growth = 0 — совпадает с r1 | +| process-gate (локально) | `node scripts/process-gate.mjs` | «гейт пройден, предупреждений 0», диапазон `origin/dev..HEAD`, 5 коммитов | +| docs freshness/links | `node scripts/check-docs.mjs --external` | «Documentation checks passed (7 files, 10 external links)» — совпадает с отчётом владельца; **подтверждает**, что `sourceFingerprint` в `docs/images/screenshots.json` после `f54b9c0` реально соответствует текущему `src/` (`check-docs.mjs:132` сравнивает `manifest.sourceFingerprint` с живым `sourceFingerprint(ROOT)`, а не просто наличие поля) | +| merge-корректность shared-файлов | чтение `docs/specs/README.md`, `docs/CHANGELOG.md`, `docs/CHANGELOG.ru.md` | обе записи (#173, #174) присутствуют, не дублированы, не потеряны; `grep -rn '^<<<<<<<\|^=======$\|^>>>>>>>'` по репозиторию — 0 совпадений, конфликтных маркеров не осталось | + +**Не прогонялось и почему:** + +- `npm run golden:verify` — diff (`fc1e795`, `f54b9c0`) не содержит изменений + `demo/golden/**`; ни один baseline не принимался. AC12 остаётся проверенным + чтением кода, как в r1 (переиспользование немодифицированных canonical + хелперов рендера) — предрелизный гейт, не гейт код-ревью. +- `python -m pytest tests_backend -q` — ни один файл + `custom_components/houseplan/**/*.py` не тронут ни в `fc1e795`, ни в + `f54b9c0` (`git diff --stat origin/dev...HEAD`). +- Полный набор из 138+ browser-smoke — как и в r1, тронута ровно одна + поверхность (Plan editor); прогнаны целевой smoke плюс три соседних по + diff-риску, совпадающих с r1. Продуктовый код не изменился с r1, поэтому + расширять выборку в r2 нет причины, вытекающей из diff. +- Алгоритмическая мутационная проверка «тест умеет падать» (снятие + `consumed.has(atom.key)` и т.п.) — не повторялась: код `wall-face-graph.ts` + и `_applyWallFaceBatch` в `fc1e795` идентичен по diffstat уже + промутированному в r1 коду; повторный прогон того же эксперимента на том же + коде не добавляет доказательной силы, только тратит цикл. +- `npm run benchmark:compare` против baseline SHA — как и в r1, сохранённого + отчёта базового SHA нет в этой сессии; сырые цифры сверены вручную с + `budgets-large-house-plan-snap.json`, ни один бюджет не ослаблен. + +## Проверка AC1–AC17 + +Все 17 AC остаются подтверждёнными по существу анализа r1 +(`docs/reviews/CODE-REVIEW-173-r1.md`, раздел «Проверка AC1–AC17») — продуктовый +код не изменился. В r2 дополнительно подтверждено исполнением: + +- AC1, AC2, AC3, AC7, AC10, AC11, AC16 — повторным зелёным прогоном целевого и + трёх соседних smoke на актуальном дереве (см. таблицу гейтов); +- AC15 — повторным прогоном `benchmark:large-house-plan-snap` на актуальном + дереве, числа совпадают по порядку величины с r1; +- AC14 — `git diff --stat origin/dev...HEAD` по новому диапазону подтверждает + отсутствие Python/schema изменений и в `fc1e795`, и в `f54b9c0`; +- AC17 — typecheck/unit/build зелёные на актуальном дереве, три копии бандла + идентичны, `check-docs.mjs` подтверждает свежесть screenshot-fingerprint + (то, чего не хватало непосредственно после ребейза и что чинит `f54b9c0`). + +AC4–AC6, AC8, AC9, AC12, AC13 не переисполнялись отдельно в r2 (unit-suite +`wall-face-graph.test.mjs` зелёный в общем прогоне 848/848, код не менялся) — +проверено тем, что `npm test` покрывает их без регрессии, и явной пометкой, +что построчный разбор не повторялся, так как нет предмета для повторного +разбора (diff идентичен). + +## Находки + +Находок уровня **High** нет. Новых находок уровня **Medium** нет. + +Обе Medium-находки r1 остаются в силе, не блокируют и уже вынесены отдельными +issue — проверено, что они не потерялись при ребейзе и не были закрыты +задним числом: + +- [#176](https://github.com/Matysh/houseplan-card/issues/176) (`S1-new`, + `tech-debt`, `P3`) — мёртвый код старого инструмента `partition` не удалён. + Перепроверено: `MARKUP_TOOLS` (строка 532, было 533 — сдвиг на строку из-за + не связанной с #173 правки, не регрессия), `_partitionClick` определён на + 6943 и вызывается на 6644, `'partition'` встречается 51 раз в + `src/houseplan-card.ts` — состояние не изменилось между r1 и r2. +- [#177](https://github.com/Matysh/houseplan-card/issues/177) (`S1-new`, + `tests`, `tech-debt`, `P3`) — AC8 не имеет unit/smoke-доказательства именно + для новой интеграции `_offerWallFaces`/`_applyWallFaceBatch`. Код, + реализующий AC8, не изменился с r1, находка не переоткрывается заново. + +Обе Low-находки r1 (`if/else` без скобок в `_activateMarkupTool`; +`wallFaceAcceptedClickMs` не входит в отслеживаемые performance-бюджеты) +остаются как записано в r1 — не блокируют, решение прежнего ревьюера в силе, +код не изменился. + +### Проверка самого ребейза — отдельный предмет r2 + +Единственный содержательно новый риск этого цикла — не в продуктовом коде, а +в слиянии с параллельной веткой #174. Проверено: + +1. **Нет дублирования/потери записей.** `docs/specs/README.md` содержит + ровно по одной строке на #173 и #174; оба `CHANGELOG.md`/`CHANGELOG.ru.md` + содержат ровно по одной записи `Unreleased` на каждый issue, без + дублирующихся или осиротевших абзацев. +2. **Нет маркеров конфликта.** `grep` по всему репозиторию на + `<<<<<<<`/`=======`/`>>>>>>>` — 0 совпадений. +3. **Бандл пересобран из объединённого дерева, а не унаследован от старой + ветки.** Собственная пересборка (`npm run build`) даёт три побайтно + идентичные копии с тем же SHA-256, который владелец указал в хендоффе + ребейза — то есть закоммиченные копии не устарели относительно текущего + `src/`. +4. **`docs/images/screenshots.json` не рассинхронизирован.** + `scripts/check-docs.mjs` явно сравнивает закоммиченный `sourceFingerprint` + с фингерпринтом живого `src/` (`check-docs.mjs:132`) — прогон зелёный, + то есть коммит `f54b9c0` не просто «обновил какие-то числа», а актуален + прямо сейчас, на этом дереве, а не только на дереве владельца в момент + коммита. +5. **#174 действительно уже принят** (`S8-merged`) — рероллы его кода этим + ребейзом не создают повторного риска, требующего отдельной ревью-нагрузки + в рамках #173. + +## Что проверено и корректно + +- Всё содержание раздела «Что проверено и корректно» `CODE-REVIEW-173-r1.md` + остаётся в силе без изменений: алгоритм planar-graph, дисциплина «тест + умеет падать» (мутационная проверка r1), finish-контракт на трёх точках + выхода, cancel/escape restore terminal draft, лимиты до mutation, clean + split reuse, provenance/толщина, совместимость (AC14), i18n RU/EN, + трейлеры/changelog — всё это проверялось на идентичном продуктовом дифе. +- Дополнительно к r1: ребейз на `dev`, включающий #174, произведён без + потери/дублирования продуктовых записей в shared-файлах (`docs/specs/ + README.md`, оба changelog), без оставленных маркеров конфликта, с + пересобранным и сверенным бандлом. +- `f54b9c0` — легитимный docs-only коммит: трогает только + `docs/images/*.png` и `docs/images/screenshots.json` (не + `demo/golden/baselines/**`), поэтому не требует `Release:`/ + `Baseline-Reviewed:` трейлеров; несёт `Issue: #173`, `User-Visible: no` + корректно (сами скриншоты — не продуктовое поведение); `check-docs.mjs` + подтверждает, что обновлённый fingerprint соответствует текущему `src/`. +- `process-gate.mjs` зелёный на новом диапазоне `origin/dev..HEAD` (5 + коммитов, было 3 в r1) — трейлеры на всех коммитах класса A/B/C корректны. + +## Чего не проверял + +- Не повторял построчный разбор алгоритма `wall-face-graph.ts` и AC4–AC6/AC8/ + AC9/AC12/AC13 по существу — сделано в r1 на идентичном по diffstat коде; + повторный построчный разбор того же самого кода не производит новой + информации, только повторяет уже потраченный цикл. Подтверждено только + косвенно: `npm test` зелёный (848/848) на актуальном дереве без изменений + в затронутых файлах. +- Не повторял мутационную проверку «тест умеет падать» — тот же аргумент, + код `wall-face-graph.ts`/`_applyWallFaceBatch` не менялся с момента, когда + она была выполнена в r1. +- Не прогонял `golden:verify` и `pytest tests_backend` — как в r1, diff не + задевает golden/backend поверхности; это предрелизный гейт. +- Не прогонял полный набор 138+ browser-smoke — продуктовый код не изменился + с r1, поверхность та же (Plan editor), расширять выборку без нового риска + в diff не оправдано. +- Не воспроизводил multi-client optimistic-lock conflict живым прогоном + (AC13) — как в r1, проверено чтением, что batch не вводит новый conflict- + путь; код batch не менялся. +- Не запускал `npm run benchmark:compare` против сохранённого baseline SHA — + такого отчёта нет в этой сессии; сверил абсолютные числа вручную с + бюджетным файлом, как в r1. + +## Вердикт + +Зелёный. High: 0, Medium: 0 новых (обе Medium-находки r1 остаются в силе, +уже вынесены отдельными issue — [#176](https://github.com/Matysh/houseplan-card/issues/176), +[#177](https://github.com/Matysh/houseplan-card/issues/177), — и не +переоткрываются, так как код, к которому они относятся, не изменился). Low: 2, +перенесены из r1 без изменений, не блокируют. + +Причина цикла r2 не в качестве кода, а в необходимом по процессу повторном +прогоне после ребейза на ушедший вперёд `dev` (слияние с #174). Продуктовый +диф идентичен уже принятому в r1 по составу файлов и объёму изменений; новая +проверочная нагрузка этого цикла была сосредоточена на самом слиянии и на +корректности docs-only коммита `f54b9c0` — обе проверки пройдены. Ни одна +находка не свидетельствует, что изменение не решает заявленный сценарий или +ухудшает смежное поведение.