mirror of
https://github.com/Matysh/houseplan-card
synced 2026-09-29 03:09:36 +00:00
@@ -1,6 +1,6 @@
|
||||
# Индекс ревью
|
||||
|
||||
Генерируется `node scripts/reviews-index.mjs` (#635) — не редактировать руками. Документов: 1010, issue: 352. Вердикт: 🟢 зелёный · 🟡 жёлтый · 🔴 красный · ⚪ не распознан (свободная форма старых документов). H/M — число High/Medium по строке вердикта или заголовкам находок. Файлы — пути, названные в находках; ищите по имени файла: `grep form-kit INDEX.md`.
|
||||
Генерируется `node scripts/reviews-index.mjs` (#635) — не редактировать руками. Документов: 1011, issue: 353. Вердикт: 🟢 зелёный · 🟡 жёлтый · 🔴 красный · ⚪ не распознан (свободная форма старых документов). H/M — число High/Medium по строке вердикта или заголовкам находок. Файлы — пути, названные в находках; ищите по имени файла: `grep form-kit INDEX.md`.
|
||||
|
||||
| Issue | Документ | Этап · раунд | Вердикт | H | M | Находки | Файлы |
|
||||
|---|---|---|---|---:|---:|---|---|
|
||||
@@ -26,6 +26,7 @@
|
||||
| #621 | [CODE-REVIEW-621-r1.md](CODE-REVIEW-621-r1.md) | code · r1 | 🟢 зелёный | 0 | 0 | — | — |
|
||||
| #619 | [CODE-REVIEW-619-r1.md](CODE-REVIEW-619-r1.md) | code · r1 | 🟢 зелёный | 0 | 0 | — | — |
|
||||
| #618 | [SPEC-REVIEW-618-r1.md](SPEC-REVIEW-618-r1.md) | spec · r1 | 🟢 зелёный | 0 | 0 | нотация h<ref> в B5 не встречается в коде | `docs/FILTERING.md` |
|
||||
| #617 | [SPEC-REVIEW-617-r1.md](SPEC-REVIEW-617-r1.md) | spec · r1 | 🟢 зелёный | 0 | 0 | «новый необязательный параметр» уже существует | `src/backdrop-pick.ts` `houseplan-editor-runtime.ts` |
|
||||
| #615 | [SPEC-REVIEW-615-r1.md](SPEC-REVIEW-615-r1.md) | spec · r1 | 🟡 жёлтый | 0 | 1 | AC3 называет несуществующую защиту от расползания на плашки цвета | `smoke_room_settings_form.mjs` `smoke_space_settings_form.mjs` `smoke_device_settings_form.mjs` `smoke_dialog_polish_605.mjs` `smoke_general_settings_form.mjs` |
|
||||
| #614 | [SPEC-REVIEW-614-r1.md](SPEC-REVIEW-614-r1.md) | spec · r1 | 🟢 зелёный | 0 | 0 | — | — |
|
||||
| #614 | [CODE-REVIEW-614-r1.md](CODE-REVIEW-614-r1.md) | code · r1 | 🟡 жёлтый | 0 | 1 | AC1/AC2/AC3 требуют unit-тест, а он не написан | `test/dialog-baseline.test.mjs` `test/space-dialog.test.mjs` `general-form-state.ts` `space-form-state.ts` `marker-form-state.ts` `tsconfig.test.json` `dialog-baseline.ts` `scripts/mutation-registry.mjs` |
|
||||
|
||||
@@ -0,0 +1,229 @@
|
||||
# SPEC-REVIEW-617-r1
|
||||
|
||||
**Issue:** [#617](https://github.com/Matysh/houseplan-card/issues/617) — «Загрузка плана > ~3 МиБ обрывает WebSocket без сообщения: MAX_PLAN_BYTES 8 МиБ недостижим через 4 МиБ кадр»
|
||||
**Этап:** ТЗ на ревью (PROCESS.md §2.4), трек `small`
|
||||
**Заход:** r1 · блокирующих циклов израсходовано 0 из 2 (лимит лёгкого трека)
|
||||
**Ревьюер:** независимая сессия, без контекста автора ТЗ
|
||||
|
||||
**Вердикт: зелёный**
|
||||
|
||||
---
|
||||
|
||||
## Скоуп ревью
|
||||
|
||||
ТЗ живёт в теле issue #617, раздел `## ТЗ` (решение владельца #517 — файл в
|
||||
`docs/specs/` не создаётся). Материал ревью — ровно этот текст, см. блок
|
||||
«Материал раунда» в конце документа.
|
||||
|
||||
Задача: 4 МиБ дефолтный кадр WS aiohttp делает объявленный `MAX_PLAN_BYTES = 8
|
||||
МиБ` недостижимым — файл плана > ≈3 МиБ рвёт соединение вместо честной ошибки
|
||||
`too_large`. Выбран вариант (б): новый HTTP-view `POST
|
||||
/api/houseplan/plans/upload` со стриминговым пределом, карточка грузит план
|
||||
только через него; WS `houseplan/plan/set` остаётся для старых закешированных
|
||||
карточек без изменения контракта.
|
||||
|
||||
## Как проверялось
|
||||
|
||||
Ревью состязательное: identifiers и утверждения о существующем поведении из ТЗ
|
||||
сверены с реальным кодом на `dev` (SHA см. ниже), а не приняты на слово автора.
|
||||
|
||||
Прочитано перед вынесением вердикта:
|
||||
- `docs/SCOPE.md` (персона «Home admin», out-of-scope список — задача не
|
||||
задевает ни один пункт: не CAD/PDF/3D/радар/оверлей, обычная правка
|
||||
editor-only поверхности);
|
||||
- `PROCESS.md` §1–§10 (классы файлов, ЖЦ, лёгкий трек §5, лимит циклов §4,
|
||||
формат вердикта §7.2);
|
||||
- `AGENTS.md` (роли, трейлеры, гейты);
|
||||
- `docs/TOUCH-SUPPORT.md` (проверка утверждения ТЗ «touch не затронут» —
|
||||
подтверждено: правка живёт в editor-only диалоге пространства, View/kiosk не
|
||||
трогается, а редакторы по продукту desktop-first);
|
||||
- само тело issue #617 целиком, включая аналитический комментарий-обоснование
|
||||
трека `small`.
|
||||
|
||||
Код читан выборочно, но по каждому явному утверждению ТЗ о существующем
|
||||
поведении — не по касательной:
|
||||
|
||||
| Утверждение ТЗ | Файл:строка | Результат сверки |
|
||||
|---|---|---|
|
||||
| `MAX_PLAN_BYTES = 8 * 1024 * 1024` в `validation.py` | `custom_components/houseplan/validation.py:24` | совпадает |
|
||||
| `ws_plan_set` шлёт `too_large` при `len(raw) > MAX_PLAN_BYTES`, копия-при-записи `<space>.<token>.<ext>`, `check_quota`+`atomic_write` под `upload_lock` | `custom_components/houseplan/websocket_api.py:2330–2384` | совпадает |
|
||||
| `may_write` / `_check_write` — общая точка авторизации WS и HTTP | `custom_components/houseplan/auth.py:16`, `websocket_api.py:306` | совпадает, один helper |
|
||||
| `HouseplanDecorAssetUploadView` буферизует тело в памяти блоками, `_FLUSH_AT`-преflight по `Content-Length` | `custom_components/houseplan/http_api.py:218–352`, `:68` | совпадает — это и есть ближайший шаблон для «принято предположительно» п.5 |
|
||||
| `HouseplanUploadView` — паттерн `fetchWithAuth` + `FormData`, разбор `err.too_large`/`err.unauthorized`/`err.bad_ext` | `houseplan-editor-runtime.ts:7371–7412` | совпадает, включая точный текст `err.too_large` с `{mb}` из ответа |
|
||||
| `classifyPlanFile(file, guardAboveBytes)` — svg проходит без проверки размера, растр уходит в guard при `size > guardAboveBytes` независимо от `probe.kind` | `src/backdrop-pick.ts:87-99` | совпадает: раздельная обработка SVG/растра в ТЗ верна |
|
||||
| `renderBackdropGuard(..., allowOriginal = true)` | `src/backdrop-pick.ts:157-163`, вызов для декора `houseplan-editor-runtime.ts:7949` (`allowOriginal = size <= 2 МиБ`) | **параметр уже существует и уже используется декором** — см. находку Low ниже |
|
||||
| `PlanFilePayload.b64`, `space-dialog.ts` тип, `space-form-state.ts` сравнивает только `.name` | `src/backdrop-pick.ts:14-19`, `src/space-dialog.ts:17`, `src/editors/space-form-state.ts:30` | совпадает |
|
||||
| `_saveSpaceDialog`: «загрузить ДО изменения конфига», перечитывание конфига в catch, `toast.error` | `houseplan-editor-runtime.ts:8084-8096`, `:8213-8227` | совпадает |
|
||||
| `probeBackdrop` даёт ровно `safe/warn/hard/unknown` | `src/backdrop-probe.ts:29,50-52` | совпадает |
|
||||
| USER-GUIDE уже обещает «8 МБ» (ru/en) | `docs/USER-GUIDE.ru.md:2225`, `docs/USER-GUIDE.md:1385` | совпадает |
|
||||
| `PLAN_EXTENSIONS`, `valid_space_id`, `CONTENT_URL`, `atomic_write`, `check_quota` | `validation.py:22,1156`, `const.py:21`, `plans.py:29,240` | все существуют, сигнатуры как в ТЗ |
|
||||
| `demo/smoke_backdrop_guard.mjs` сегодня сверяет `planFile.b64` побайтово («Оставить оригинал») | `demo/smoke_backdrop_guard.mjs:73-84,108,270` | совпадает — правка смока действительно необходима и её объём соответствует заявленному |
|
||||
| `scripts/mutation-registry.mjs`, `scripts/mutation-gate.mjs`, `scripts/check-inputs.mjs`, `scripts/smoke-select.mjs` существуют | `ls scripts/*.mjs` | совпадает |
|
||||
|
||||
Отдельно проверена «граница включительная» (AC: `MAX_PLAN_BYTES` проходит,
|
||||
`+1` отклоняется): и `ws_plan_set` (`> MAX_PLAN_BYTES`), и
|
||||
`HouseplanDecorAssetUploadView` (`> MAX_DECOR_ASSET_BYTES`) используют строгое
|
||||
`>`, т.е. равенство пределу проходит — контракт для нового view, списанный с
|
||||
этого же шаблона, воспроизводим и непротиворечив.
|
||||
|
||||
Это не гейт-прогон (стадия ТЗ, кода ещё нет) — типизация/тесты/сборка здесь
|
||||
неприменимы. Проверка кода была вместо «поверил автору» — цель прочитанного
|
||||
выше.
|
||||
|
||||
## Разделы §7.1 — присутствуют все
|
||||
|
||||
Сценарий · что человек увидит до/после · проблема · скоуп/не-скоуп · контракт
|
||||
поведения · UX · модель данных и миграция (явное «нет») · i18n · AC1…AC8 с
|
||||
таблицей «чем доказан / чем краснеет» · план автотестов · перф/touch · риски ·
|
||||
откат · release-артефакты · «принято предположительно» (8 пунктов). Пункты DoR
|
||||
§2.5 покрыты: миграция/compatibility — явное «нет» со ссылкой на
|
||||
`CONFIG-COMPATIBILITY.md`; touch — явное «не затронут» с корректным
|
||||
обоснованием через персону/поверхность; открытых продуктовых вопросов нет.
|
||||
|
||||
## AC — проверка однозначности и доказуемости
|
||||
|
||||
AC1–AC8 пронумерованы, у каждого в таблице указан и тест, и мутация/негативная
|
||||
проба («чем краснеет») — защитные AC (AC2–AC5, AC7 частично) не остаются без
|
||||
названного свидетеля. Разобраны предметно:
|
||||
|
||||
- **AC1** — «PNG, проба `safe`» как явное условие теста снимает
|
||||
недетерминированность (без этого уточнения 5 МиБ файл мог бы попасть в guard
|
||||
по `warn`); формулировка «ровно один POST … и ни одного `plan/set`» считается
|
||||
счётчиком, не текстом — однозначно.
|
||||
- **AC2/AC3** — раздельные ветки SVG/растра в контракте совпадают с реальным
|
||||
разделением в `classifyPlanFile` (SVG обходит проверку размера в самой
|
||||
функции, поэтому проверка предела для SVG обязана жить отдельно в
|
||||
`_pickPlanFile` — ТЗ описывает это как отдельный пункт списка, а не путает
|
||||
два пути). Условие AC3 «с читаемыми размерами» корректно исключает
|
||||
`hard`/`unknown`, для которых контракт сознательно не меняет поведение (см.
|
||||
находку Low о непокрытом случае — не блокирует).
|
||||
- **AC4** — таблица кодов ответа полная и без пересечений (403/503/413×2/400×3/507/200);
|
||||
граница `MAX`/`MAX+1` и отсутствие временных файлов после 413 — измеримо
|
||||
прогоном backend-теста.
|
||||
- **AC5** — единственный AC, доказывающий защиту «клиент лжёт/обойдён» —
|
||||
свидетель назван (мутация «удаление разбора `too_large`»), третий столбец не
|
||||
пуст.
|
||||
- **AC6** — «одно число» (см. предупреждение о числах в §8 гейтов) закрыто
|
||||
явным тестом, читающим оба источника плюс оба USER-GUIDE — ровно то, что
|
||||
требует правило «одно число — один источник».
|
||||
- **AC7/AC8** — паритет и «общий хелпер» доказываются существующими тестами +
|
||||
ревью кода с явной пометкой «проверено чтением, не исполнением» ожидается на
|
||||
этапе код-ревью — здесь это корректно поставлено в план, не выдано за
|
||||
готовый факт.
|
||||
|
||||
Ни одного места, где решение приложения выдано за факт без пометки: восемь
|
||||
пунктов «принято предположительно» покрывают именно то, что пользователь не
|
||||
наблюдает (выбор варианта (б) обоснован в аналитике и в самом ТЗ, а не
|
||||
голословен) либо являются реализационными деталями (URL, буферизация,
|
||||
именование). Продуктовых вопросов владельцу не задано и не нужно было —
|
||||
формулировка «что человек увидит до/после» отвечает на оба вопроса, которые
|
||||
вправе решать только он.
|
||||
|
||||
## Находки
|
||||
|
||||
### Low-1 — «новый необязательный параметр» уже существует
|
||||
`docs/reviews` н/п. **Файл:** `src/backdrop-pick.ts:157-163` (сигнатура
|
||||
`renderBackdropGuard`), уже используется декором в
|
||||
`houseplan-editor-runtime.ts:7949`.
|
||||
ТЗ (раздел «Не-скоуп») формулирует: «Общий `renderBackdropGuard` меняется
|
||||
только добавлением необязательного параметра; поведение декора прежнее» —
|
||||
подразумевая, что параметр `allowOriginal` появится этой задачей. Он уже
|
||||
существует и уже используется тем же декором, на который ТЗ ссылается как на
|
||||
прецедент. Значит для скрытия кнопки «Оставить оригинал» этой задаче не нужно
|
||||
менять сигнатуру вовсе — только передать шестой аргумент из вызова для плана.
|
||||
Действительно новым остаётся только параметр, переключающий ТЕЛО диалога
|
||||
(`over_limit_body` vs `large_body`), который в разделе выше назван отдельно и
|
||||
верно. Не блокирует ни один AC и не меняет объём работы по сути — это неточная
|
||||
атрибуция «что уже есть» vs «что появится», сама работа от этого не меняется.
|
||||
Правится словом при следующей правке ТЗ или снимается с записью — не
|
||||
возвращает задачу на цикл.
|
||||
|
||||
### Наблюдение (не находка, не блокирует)
|
||||
Случай «растр больше предела **и** проба `hard`» ТЗ явно не разбирает (AC3
|
||||
условие «с читаемыми размерами» его обходит стороной). Сегодняшний код и без
|
||||
этой задачи не даёт в `hard`-случае ни одной кнопки действия, кроме «Отмена»
|
||||
(`renderBackdropGuard`: `${hard ? null : …}`) — то есть пользователь и сейчас
|
||||
не может загрузить/уменьшить `hard`-файл, независимо от байтового предела.
|
||||
Задача не ухудшает этот путь и не обязана его чинить (не про размер в байтах,
|
||||
а про безопасность декодирования — чужой скоуп). Упоминаю, чтобы будущий
|
||||
ревьюер кода не считал это пропущенным AC.
|
||||
|
||||
### Трек `small` — соразмерность (не находка)
|
||||
Задача добавляет новый аутентифицированный write-эндпоинт — обычно повод для
|
||||
внимательности при выборе лёгкого трека. Аналитический комментарий явно прошёл
|
||||
все пять критериев §5 и обосновал каждый (сложность 3 — копия двух уже
|
||||
существующих шаблонов view почти дословно; риск закрыт паритетом проверок с
|
||||
`ws_plan_set` и общим writer'ом; одна поверхность — один диалог, один
|
||||
эндпоинт; нет миграции/UX-контракта/перф/touch-влияния). Сверка кода в этом
|
||||
документе подтверждает, что описанные шаблоны (`HouseplanDecorAssetUploadView`,
|
||||
`HouseplanUploadView`, `may_write`, `check_quota`, `atomic_write`) существуют
|
||||
и совпадают буквально — обоснование не голословно. Оставляю как отмеченное, а
|
||||
не как находку: критерии §5 не про «есть ли новый эндпоинт», а про
|
||||
сложность/риск/поверхность/миграцию/UX/перф-touch, и все пять по существу
|
||||
закрыты.
|
||||
|
||||
## Что проверено и корректно
|
||||
|
||||
- Все обязательные разделы §7.1 присутствуют, DoR §2.5 выполним по тексту ТЗ.
|
||||
- Каждый явный технический факт о существующем коде, на который опирается
|
||||
контракт, сверен построчно (таблица выше) — ни одной догадки, выданной за
|
||||
факт.
|
||||
- AC1–AC8 пронумерованы, однозначны, у каждого назван способ доказательства;
|
||||
защитные AC несут третий столбец «чем краснеет» с конкретной мутацией.
|
||||
- «Одно число» (`MAX_PLAN_BYTES`) имеет план проверки на трёх источниках сразу
|
||||
(TS, Python, оба USER-GUIDE) — правило §8 учтено на стадии ТЗ, а не оставлено
|
||||
коду.
|
||||
- i18n: ключи ru/en перечислены дословно, de/fr делегированы с сохранением
|
||||
плейсхолдеров и стиля существующего `err.too_large`; мёртвых ключей не
|
||||
вводится.
|
||||
- Совместимость: WS `plan/set` не меняет контракт (не-скоуп явно), общий writer
|
||||
для обоих путей записи специфицирован как единственная точка проверок.
|
||||
- Откат тривиален и описан честно (файлы не мигрируют, revert коммита
|
||||
достаточен).
|
||||
- Touch/перф корректно обоснованы как «не затронуты» со ссылкой на
|
||||
правильную персону/поверхность (сверено с `TOUCH-SUPPORT.md`).
|
||||
|
||||
## Чего не проверял
|
||||
|
||||
- Не проверял сами тесты/смоки — на стадии ТЗ их ещё нет; их придётся
|
||||
перепроверить на код-ревью, включая «умеет ли тест падать» для каждого из
|
||||
трёх заявленных мутантов.
|
||||
- Не проверял, что `PLANS_DIR`, `MAX_PLANS_BYTES`, `MAX_PLANS_FILES`
|
||||
(папочные квоты) действительно не нуждаются в правке для нового
|
||||
write-пути — ТЗ утверждает переиспользование через общий writer, что снимает
|
||||
вопрос конструктивно (не два места квот), но исполнение проверит код-ревью.
|
||||
- Не гонял никаких CI/локальных гейтов — на этапе ТЗ они неприменимы (нет кода
|
||||
для проверки; ссылка на них в самом ТЗ — план, а не отчёт об исполнении).
|
||||
- Не оценивал производительность потоковой буферизации 8 МиБ в памяти под
|
||||
параллельными аплоадами — тот же паттерн, что и у декора (2 МиБ), риск не
|
||||
новый по порядку величины и явно назван в разделе «Риски» ТЗ.
|
||||
|
||||
## Материал раунда
|
||||
|
||||
```
|
||||
Issue: #617
|
||||
Repo: Matysh/houseplan-card
|
||||
SHA рабочей копии (репозиторий, не материал ТЗ): 0f97e3e639c4eb06b7866b8c1c757d98e2c0a5f7
|
||||
ТЗ: тело issue #617, раздел «## ТЗ»
|
||||
sha256(нормализованное тело issue, UTF-8, как получено `gh issue view 617 --json body`): 696d986cca3720a468ba776055d544cececd4713062b4c5e5b6e2e44ced87092
|
||||
Длина тела: 18220 байт
|
||||
Аналитический комментарий (трек/оценка): Matysh, 2026-09-24T01:01:56Z
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
**Вердикт: зелёный · заход r1 · блокирующих циклов 0/2 · High: 0 · Medium: 0**
|
||||
|
||||
---
|
||||
|
||||
<!-- material-anchors: сгенерировано конвейером (#414) -->
|
||||
|
||||
## Материал раунда
|
||||
|
||||
- Ветка: `dev`, коммит `0f97e3e639c4` — ребейз его осиротит, и это нормально: ниже якоря, которые ребейз не меняет.
|
||||
- Дерево материала: `f2603b2998c17fd63d3bc1248b5e2be36e4b598f`
|
||||
```
|
||||
git log --all --format='%H %T' | grep f2603b2998c1
|
||||
```
|
||||
- Тело issue: `f8d7f51e97db367fb59302863a5ab105d72a8aa34c7fa955fdc6ee29139973c6`
|
||||
- Вердикт конвейера: `green` · High 0
|
||||
Reference in New Issue
Block a user