mirror of
https://github.com/Matysh/houseplan-card
synced 2026-10-01 20:29:00 +00:00
@@ -0,0 +1,231 @@
|
||||
# CODE-REVIEW #32 · r2 — единое подтверждение опасных действий
|
||||
|
||||
- Issue: https://github.com/Matysh/houseplan-card/issues/32
|
||||
- Этап: code (PROCESS.md §2.7)
|
||||
- Заход: r2 · блокирующих циклов израсходовано 1 из 4
|
||||
- Материал: `git diff 7b363caa..HEAD` (см. «Скоуп диффа» — почему база 7b363caa, а не
|
||||
SHA из вердикта r1), `git log --oneline origin/dev..HEAD`
|
||||
- ТЗ: `docs/specs/032-unified-danger-confirmation.md` (spec-review зелёный на r2,
|
||||
документы `docs/reviews/SPEC-REVIEW-32-r{1,2}.md`)
|
||||
- Предыдущий раунд: `docs/reviews/CODE-REVIEW-32-r1.md`, вердикт-комментарий
|
||||
https://github.com/Matysh/houseplan-card/issues/32#issuecomment-5471546641
|
||||
(жёлтый · заход r1 · High:1 · Medium:0)
|
||||
|
||||
Разбор по дельте (PROCESS.md §2.9/§2.10): `dev` не ушёл вперёд
|
||||
(`origin/dev` = `8955c02e`, полностью содержится в HEAD — ребейза не было),
|
||||
контракт поведения не менялся, новая подсистема не задета, объём дельты много
|
||||
меньше исходной задачи. Условия для полного разбора не выполнены → разбор по
|
||||
дельте, полный разбор не требуется.
|
||||
|
||||
## Находка по трассируемости самого процесса (наследуется в этот документ)
|
||||
|
||||
Вердикт-комментарий r1 не называет SHA, на котором он получен — это ровно тот
|
||||
случай, который PROCESS.md §2.10 п.1 требует явно отметить. Документ
|
||||
`CODE-REVIEW-32-r1.md` называет SHA `7b363caa` в разделе «Как проверялось»
|
||||
(«Зелёного Validate на SHA `7b363caa` нет... гейты прогнаны на этом SHA»), но
|
||||
коммит `f3ac0ac0` (правка `smoke_free_walls.mjs`) по факту уже существовал в
|
||||
ветке на момент, когда вердикт r1 был опубликован (коммит запушен в
|
||||
21:59:37Z, вердикт — 22:06:37Z по журналу issue), хотя раздел H1 того же
|
||||
документа воспроизводит `smoke_free_walls` как падающий с той же сигнатурой
|
||||
ошибки, которая была в коде до `f3ac0ac0`. Похоже на то, что материал r1 был
|
||||
зафиксирован в начале разбора (`7b363caa`), а не сверен с фактическим HEAD
|
||||
непосредственно перед вынесением вердикта, как требует PROCESS.md §2.7.
|
||||
|
||||
Практического ущерба это не нанесло: `f3ac0ac0` действительно правит только
|
||||
`smoke_free_walls.mjs`, а H1 требовал ещё 4 файла (`smoke_registryless_opening`,
|
||||
`smoke_binding_picker`, `smoke_hidden_flag`, `smoke_optional_space_model`),
|
||||
которые на момент вердикта исправлены ещё не были — так что жёлтый вердикт
|
||||
всё равно был обоснован, просто по неполному списку последствий. Использую
|
||||
`7b363caa` как базу дельты именно потому, что это единственный SHA, названный
|
||||
хоть где-то в цепочке r1; это же самая консервативная граница (шире, чем
|
||||
момент публикации вердикта), так что ни одно изменение не выпадает из разбора.
|
||||
|
||||
**Серьёзность: Low, процессная, не блокирует.** Это не находка по продукту и
|
||||
не входит в компетенцию автора задачи — это дефект работы ревьюера
|
||||
предыдущего раунда. Отдельный issue не заводится: PROCESS.md §12 требует
|
||||
`process`-issue только когда правило удалось нарушить *незаметно*; здесь
|
||||
несовпадение видно из собственного текста документа r1 и не имело
|
||||
последствий для вердикта. Фиксирую как заметку на будущее правило «сверяться
|
||||
с `git rev-parse HEAD` перед итогом» (§2.7).
|
||||
|
||||
## Закрытие раунда r1
|
||||
|
||||
| Находка r1 | Чем закрыта | Где это видно |
|
||||
|---|---|---|
|
||||
| **H1** (High) — миграция на `await this._confirmDanger(...)` сломала 5 демо-смоков: 2 дают неверный вердикт (`smoke_free_walls`, `smoke_registryless_opening`), 3 виснут навсегда (`smoke_binding_picker`, `smoke_hidden_flag`, `smoke_optional_space_model`) | Все пять файлов переведены с подмены `window.confirm` на подмену `c._confirmDanger = async () => true` и получили `await` на местах, где раньше вызов не ждался; дополнительно тем же приёмом поправлен `smoke_orphan_space_references.mjs` (не входил в список H1, но использовал тот же устаревший паттерн) | `git diff 7b363caa..HEAD` — коммиты `f3ac0ac0` (free_walls) и `2f021046` (остальные 5); построчный дифф см. ниже. Исполнением: все 6 смоков зелёные, ни один не виснет (см. таблицу гейтов) |
|
||||
| **L1** (Low) — 6 старых односложных ключей i18n (`confirm.delete_draft`, `confirm.delete_draft_segment`, `confirm.remove_marker`, `confirm.delete_space`, `confirm.unlock`, `confirm.delete_plan`) без потребителей | Не тронуты умышленно (решение автора, записано в handoff-комментарии) | Перепроверено заново в этом раунде: `grep` по `src/`, `test/`, `demo/*.mjs` на точную строку ключа в кавычках — по-прежнему 0 потребителей во всех шести случаях (см. «Находки» ниже, я подтверждаю снятие) |
|
||||
|
||||
Построчное подтверждение закрытия H1 (сравнение с воспроизведением r1):
|
||||
|
||||
- `demo/smoke_free_walls.mjs`: `window.confirm = () => true` → `c._confirmDanger = async () => true`; оба вызова `c._deletePhysicalSelection()` теперь `await`-ятся (было: синхронный вызов, `_deleteDraftWhole()` уходил в фон без ожидания).
|
||||
- `demo/smoke_registryless_opening.mjs`: та же замена; `card._lockAction(lockId, 'unlock')` теперь `await`-ится, исходный `window.confirm` восстанавливается после.
|
||||
- `demo/smoke_binding_picker.mjs`, `demo/smoke_hidden_flag.mjs`, `demo/smoke_optional_space_model.mjs`, `demo/smoke_orphan_space_references.mjs`: тот же паттерн — сохраняется и восстанавливается `c._confirmDanger`, а не `window.confirm`, что и было причиной зависания (промис `_confirmDanger` резолвится только явным решением диалога, которое эти смоки не эмулировали).
|
||||
|
||||
## Как проверялось
|
||||
|
||||
Зелёного Validate на этом SHA (`2f021046`) нет — гейты ниже прогнаны мной лично
|
||||
в рабочей копии на этом SHA.
|
||||
|
||||
### Дешёвые гейты (прогнаны в этом раунде — код изменился, стоят минуты, §2.10)
|
||||
|
||||
| Команда | Результат |
|
||||
|---|---|
|
||||
| `npx tsc --noEmit` | чисто, без вывода |
|
||||
| `npm test` | `tests 1652 · pass 1651 · fail 0 · skipped 1` — то же число, что и в r1: дельта не трогает `src/**`/`test/**` |
|
||||
| `npm run build && npm run bundle:sync` | build ok; `cmp dist/houseplan-card.js custom_components/houseplan/frontend/houseplan-card.js` — идентичны; `git status --porcelain` после `bundle:sync` — пусто (три копии синхронны) |
|
||||
| `npm run bundle:budget` | `initial View: 285355 B gzip (budget 300000 B, headroom 14645 B)` — число не изменилось с r1 (AC-13 не затронут дельтой, которая не трогает `src/**`) |
|
||||
| `node scripts/no-new-any.mjs --base 7b363caa --head HEAD` | `Проверено добавленных строк в src/**/*.ts: 0 в 0 файл(ах). Новых any нет.` — ожидаемо: дельта не трогает `src/**` |
|
||||
| `node scripts/check-docs.mjs` | `Documentation checks passed (7 files, 10 external links)` — не обязателен для этой дельты (не трогает `src/**`), прогнан для полноты, зелёный |
|
||||
|
||||
`npm run invariants` не прогонял: дельта не трогает рёбра/толщину/`layout`/
|
||||
`marker.space`/`open_spans` — только тестовые стабы в `demo/**`.
|
||||
|
||||
`npm run golden:verify` не прогонял: дельта не меняет рендер/геометрию/стили,
|
||||
только поведение стаба подтверждения внутри смоков.
|
||||
|
||||
`python -m pytest tests_backend -q` не прогонял: `custom_components/**/*.py`
|
||||
дельтой не тронут.
|
||||
|
||||
Performance-профили не прогонял: не названы в AC, чувствительные к перфу пути
|
||||
дельтой не затронуты.
|
||||
|
||||
### Browser smoke — выбор и результат
|
||||
|
||||
`node scripts/smoke-select.mjs --base 7b363caa --head HEAD`:
|
||||
|
||||
```
|
||||
Исполняемого frontend-диффа нет (src/**/*.ts не тронут).
|
||||
Browser-smoke этим диффом не выбираются — это не «пропустить проверки»,
|
||||
а «выбирать нечего»: смоки проверяют собранную карточку.
|
||||
Тронуто файлов: 7.
|
||||
```
|
||||
|
||||
Инструмент не выбирает ничего механически, потому что дельта не трогает
|
||||
`src/**` — это ожидаемо для чисто тестовой правки и не является основанием
|
||||
ничего не прогонять: сама находка H1 — про демо-смоки, поэтому релевантная
|
||||
проверка — прямой запуск изменённых файлов, а не `smoke-select`, который
|
||||
индексирует связь «прод-код → смок», а не «смок → смок». Прогнал все 6
|
||||
изменённых в этой дельте смоков плюс `smoke_danger_confirmation` (эталонный
|
||||
контракт диалога, не изменён дельтой, но это единственная проверка, которая
|
||||
целиком доказывает контракт `_confirmDanger`, а не только его использование
|
||||
как стаба):
|
||||
|
||||
| Смок | Результат |
|
||||
|---|---|
|
||||
| `demo/smoke_free_walls.mjs` | OK, все под-проверки true, включая `deleteOnDraftRemovesWholeOutline` и `invalidThicknessCreatesNothing` — именно те, что падали в воспроизведении H1 |
|
||||
| `demo/smoke_registryless_opening.mjs` | OK, включая `explicitInfoActionStillWorks` — падал в H1 |
|
||||
| `demo/smoke_binding_picker.mjs` | OK, не виснет (было: `Terminated` по таймауту в H1) |
|
||||
| `demo/smoke_hidden_flag.mjs` | OK, не виснет |
|
||||
| `demo/smoke_optional_space_model.mjs` | OK, не виснет |
|
||||
| `demo/smoke_orphan_space_references.mjs` | OK, включая `deleteExplainsBlockerWithoutConfirmOrWrite` (не входил в H1, был в порядке и раньше — теперь на новом контракте) |
|
||||
| `demo/smoke_danger_confirmation.mjs` | OK, все 13 сценариев true — не затронут дельтой, прогнан как эталон, что общий контракт `_confirmDanger` не пострадал от правок в тестовых стабах |
|
||||
|
||||
Остальные смоки (полный набор — `ls demo/smoke_*.mjs | wc -l` = 209 на этом
|
||||
дереве) не прогонял: дельта не трогает `src/**`, `smoke-select` не находит
|
||||
связи, а прямая связь есть только у семи файлов выше — это и есть полный
|
||||
список файлов, изменённых дельтой (плюс сам ревью-документ r1, класс C).
|
||||
Полный набор — предрелизный гейт (§8), а не гейт ревью, и здесь не нужен: он
|
||||
уже прогонялся по существу в r1 (39+81 смоков разобраны там), а эта дельта не
|
||||
расширяет их множество.
|
||||
|
||||
## Находки (этот раунд)
|
||||
|
||||
Кроме зафиксированной выше процессной Low-заметки о ненайденном SHA в
|
||||
вердикте r1 (унаследованная, не в скоупе автора) — новых находок в дельте
|
||||
`7b363caa..HEAD` нет.
|
||||
|
||||
### L1 из r1 — статус: снимаю решением ревьюера, с записью
|
||||
|
||||
Перепроверено заново (не наследование — дельта могла случайно тронуть эти
|
||||
ключи, поэтому проверил): все шесть ключей (`confirm.delete_draft`,
|
||||
`confirm.delete_draft_segment`, `confirm.remove_marker`, `confirm.delete_space`,
|
||||
`confirm.unlock`, `confirm.delete_plan`) по-прежнему присутствуют во всех
|
||||
четырёх словарях (`src/i18n/{en,ru,de,fr}.json`) и по-прежнему не имеют ни
|
||||
одного потребителя в `src/**`, `test/**`, `demo/**` (точный поиск по строке
|
||||
ключа в кавычках). Автор явно решил не трогать их в этой правке
|
||||
(«сохранение ключей не меняет runtime и избегает несвязанного churn
|
||||
канонических screenshots») — решение обоснованное: правка чисто косметическая,
|
||||
не меняет поведение и не относится к предмету H1 (регрессия смоков), а
|
||||
трогать все 4 локали ради шести неиспользуемых строк — риск несвязанного
|
||||
диффа без пользы. AC-11 (парность словарей) не нарушена мёртвыми ключами:
|
||||
`test/i18n.test.mjs` сравнивает множества ключей между словарями, а не с
|
||||
потребителями в коде.
|
||||
|
||||
**Решение ревьюера: снимаю без правки.** Не блокирует эту и следующие задачи;
|
||||
если ключи будут мешать (например, при следующей правке i18n-словарей),
|
||||
удалить их тогда же — попутной правкой к работе над этим файлом, а не
|
||||
отдельной задачей.
|
||||
|
||||
## Унаследовано из r1 (принято без повторной проверки)
|
||||
|
||||
Ссылка: `docs/reviews/CODE-REVIEW-32-r1.md`, SHA `7b363caa` (см. пояснение
|
||||
выше о несовпадении с фактическим временем публикации вердикта — тем не
|
||||
менее это единственная точка отсчёта, названная в цепочке r1, и я
|
||||
использую её как консервативную нижнюю границу дельты).
|
||||
|
||||
Дельта `7b363caa..HEAD` не касается ни одного из файлов, которыми доказаны
|
||||
следующие пункты, поэтому они наследуются без повторного прогона:
|
||||
|
||||
- **AC-1/AC-2** (нет прямых `confirm()`, ровно 8 вызовов `_confirmDanger`) —
|
||||
`test/danger-confirmation.test.mjs` не изменён дельтой.
|
||||
- **AC-3…AC-9** (cancel-safety всех 6 классов операций, replace-семантика,
|
||||
race-safety, фокус/трап/320px/локали) — `demo/smoke_danger_confirmation.mjs`
|
||||
не изменён дельтой; перепрогнан в этом раунде для очистки совести (см.
|
||||
таблицу выше), зелёный, как и в r1.
|
||||
- **AC-10** (lazy editor chunk не тянется) — `demo/smoke_lazy_editor_chunk.mjs`
|
||||
не изменён дельтой и не входит в список файлов дельты.
|
||||
- **AC-11** (парность словарей en/ru/de/fr) — `test/i18n.test.mjs` и сами
|
||||
словари не изменены дельтой; `npm test` в этом раунде подтверждает тот же
|
||||
зелёный результат.
|
||||
- **AC-12** (специализированные диалоги не задеты) — файлы, которыми это
|
||||
доказано в r1 (`_roomDeleteDialog`, `_partitionDeleteDialog`,
|
||||
`_editorSecondaryDialogBlocked` и связанные смоки), не входят в дельту.
|
||||
- **AC-13** (бюджет initial View) — `src/**` не тронут дельтой; число
|
||||
`285355 B gzip` перепроверено исполнением в этом раунде и совпадает с
|
||||
зафиксированным в r1.
|
||||
- **Race-safety §7 ТЗ** (identity capture до/после `await` в 7 переписанных
|
||||
call sites), **unlock revalidation**, **порядок blocker-проверки до
|
||||
confirm**, **bundle sync**, **трейлеры коммитов реализации**, **документация
|
||||
(ARCHITECTURE.md/USER-GUIDE.ru.md/CHANGELOG)** — все доказаны чтением и
|
||||
исполнением файлов, которых дельта не касается; листинг файлов дельты (см.
|
||||
«Скоуп диффа» и таблицу гейтов) не пересекается ни с одним из них.
|
||||
|
||||
Дополнительно проверено заново, а не унаследовано: трейлеры двух новых
|
||||
коммитов дельты (`f3ac0ac0`, `2f021046`) — оба несут `Issue: #32` и
|
||||
`User-Visible: no`, что корректно (правки только демо-смоков, поведение
|
||||
продукта не меняется).
|
||||
|
||||
## Что проверено и корректно (сводно по этому раунду)
|
||||
|
||||
- H1 закрыт: воспроизведение из r1 (2 неверных вердикта + 3 зависания) не
|
||||
повторяется на HEAD — все 6 связанных смоков зелёные, ни один не виснет.
|
||||
- L1 корректно снят с записью — решение ревьюера, не находка.
|
||||
- Дешёвые гейты и `check-docs` зелёные, числа (тесты, bundle budget) не
|
||||
разъехались с r1 там, где дельта не должна была на них влиять — ожидаемо,
|
||||
так как дельта не трогает `src/**`.
|
||||
- Ветка не отстаёт от `origin/dev` (`8955c02e` — предок HEAD), ребейза не
|
||||
было, повторный полный разбор не требуется.
|
||||
- Трейлеры коммитов дельты корректны.
|
||||
|
||||
## Чего не проверял
|
||||
|
||||
- Полный набор из 209 демо-смоков — не требуется (предрелизный гейт, §8);
|
||||
`smoke-select` не находит связей для этой дельты, а прямая связь есть
|
||||
только у 7 файлов, все прогнаны.
|
||||
- `npm run golden:verify`, `python -m pytest tests_backend`,
|
||||
`npm run invariants`, performance-профили — не затронуты дельтой
|
||||
(обоснование в «Как проверялось»).
|
||||
- Ручное браузерное/тач-тестирование вне headless-смоков — вне возможностей
|
||||
цикла, как и в r1; не требуется этой дельтой (она не меняет ни фокус, ни
|
||||
touch-путь, только тестовые стабы).
|
||||
- Всё, что перечислено в разделе «Унаследовано из r1», — сознательно не
|
||||
перепроверялось повторно, дельта туда не дотягивается.
|
||||
|
||||
## Вывод
|
||||
|
||||
Единственная блокирующая находка предыдущего раунда (H1) закрыта полностью и
|
||||
подтверждена исполнением, а не заявлением автора. L1 корректно снят решением
|
||||
ревьюера с записью. Новых находок в дельте нет; процессная заметка о
|
||||
ненайденном SHA в вердикте r1 — Low, не блокирует, не входит в скоуп автора.
|
||||
|
||||
Вердикт: зелёный · заход r2 · блокирующих циклов 1/4 · High: 0 · Medium: 0.
|
||||
Reference in New Issue
Block a user