diff --git a/docs/reviews/INDEX.md b/docs/reviews/INDEX.md index 9ab5894c..2b412700 100644 --- a/docs/reviews/INDEX.md +++ b/docs/reviews/INDEX.md @@ -1,10 +1,11 @@ # Индекс ревью -Генерируется `node scripts/reviews-index.mjs` (#635) — не редактировать руками. Документов: 232, issue: 112. Вердикт: 🟢 зелёный · 🟡 жёлтый · 🔴 красный · ⚪ не распознан (свободная форма старых документов). H/M — число High/Medium по строке вердикта или заголовкам находок. Файлы — пути, названные в находках; ищите по имени файла: `grep form-kit INDEX.md`. +Генерируется `node scripts/reviews-index.mjs` (#635) — не редактировать руками. Документов: 233, issue: 113. Вердикт: 🟢 зелёный · 🟡 жёлтый · 🔴 красный · ⚪ не распознан (свободная форма старых документов). H/M — число High/Medium по строке вердикта или заголовкам находок. Файлы — пути, названные в находках; ищите по имени файла: `grep form-kit INDEX.md`. | Issue | Документ | Этап · раунд | Вердикт | H | M | Находки | Файлы | |---|---|---|---|---:|---:|---|---| | бета v1.79.0-beta.1 | [SHIP-REVIEW-v1.79.0-beta.1.md](SHIP-REVIEW-v1.79.0-beta.1.md) | пакетное ревью ship · — | ⚪ — | 0 | 0 | — | — | +| #740 | [SPEC-REVIEW-740-r1.md](SPEC-REVIEW-740-r1.md) | spec · r1 | 🟢 зелёный | 0 | 0 | устаревший номер строки в «Проблема», п.2 | `src/stairs-view.ts` `src/stairs-editor.ts` `stairs-view.ts` `stairs-editor.ts` `stairs.ts` `large-house.mjs` `matrix.mjs` | | #739 | [SPEC-REVIEW-739-r1.md](SPEC-REVIEW-739-r1.md) | spec · r1 | 🟢 зелёный | 0 | 0 | — | — | | #737 | [SPEC-REVIEW-737-r1.md](SPEC-REVIEW-737-r1.md) | spec · r1 | 🟢 зелёный | 0 | 0 | — | — | | #732 | [CODE-REVIEW-732-r1.md](CODE-REVIEW-732-r1.md) | code · r1 | 🟢 зелёный | 0 | 0 | — | — | diff --git a/docs/reviews/SPEC-REVIEW-740-r1.md b/docs/reviews/SPEC-REVIEW-740-r1.md new file mode 100644 index 00000000..159ef7a2 --- /dev/null +++ b/docs/reviews/SPEC-REVIEW-740-r1.md @@ -0,0 +1,212 @@ +# SPEC-REVIEW-740-r1 + +**Issue:** #740 «Производительность: слой лестниц — ≈50 мс на переключение на +этаж 1» +**Этап:** spec (PROCESS.md §2.4) · **Заход:** r1 · блокирующих циклов 0/4 +**Трек:** `ask` (критерий §5 «перф»: меняется цена переключения этажа, файлы +класса A; дополнительно «геометрия» — меняется разметка символа лестницы) +**Материал:** тело issue #740, раздел `## ТЗ` (снимок на момент ревью, issue +открыт, лейблы `S4-spec-review`, `tech-debt`, `track:ask`; один комментарий — +«Оценка», трек подтверждён аналитиком) +**Проверка кода велась против рабочей копии на** `b27ae06ef81538db6f4fcd183036918088cbca85` +(`HEAD`, совпадает с `origin/dev` на момент ревью; ветки `issue/740-*` нет — +кода по задаче ещё не существует) — не как материал ревью (ТЗ ещё не код), а +чтобы проверить, что номера строк, имена функций и константы в ТЗ отражают +текущий код, а не домысел. + +## Скоуп + +К1 — ступени одной лестницы (`src/stairs-view.ts`, `src/stairs-editor.ts`) +переходят с N отдельных `` на один +`` со строкой `d`, построенной один раз на объект +геометрии из `cachedStairRenderGeometry`, а не на каждом рендере. Контур, +трапеция, стрелка, `g.hp-stair` и его атрибуты/обработчики не меняются. +Эффект доказывается Full Performance (`switchCycleMs` фикстуры `large-house`, +`isometric`). Обслуживает J1 `docs/SCOPE.md` («переключение этажей — +частое действие», сценарий явно называет домочадцев на киоске/телефоне и +администратора). `User-Visible: no` — заявлена пиксельная идентичность кадра, +что проверяется тем же AC3, которое доказывает отсутствие видимого изменения. + +## Как проверялось + +Ревью ТЗ на этапе spec не гоняет тестовые гейты — кода ещё нет (ветки `issue/ +740-*` не существует, подтверждено `git branch -a` и `git log --all --oneline` +по номеру задачи). Вместо этого каждое фактическое утверждение ТЗ (номер +строки, имя константы/функции, число элементов, существующая golden-сцена) +сверено с текущим деревом на `b27ae06e`, поскольку «догадка, записанная как +факт» — находка (§7.1). + +Прочитано и сверено построчно: +- `src/stairs-view.ts` — `renderLayer()` (:63, ТЗ указывает то же), строка + контура `outline.map(...).join(' ')` (:71, совпадает), блок ступеней + `geometry.treads.map(...)` (:115, совпадает — это и есть точка контракта К1), + геттер `stairs` → `stairList(...)` (фактически :35, ТЗ указывает :34 — см. + находку ниже). +- `src/stairs-editor.ts` — `renderStair()` → `geometry.treads.map(...)` (:535, + совпадает — вторая точка контракта К1). +- `src/stairs.ts` — `cachedStairRenderGeometry` (:320, совпадает), + `STAIR_STROKE_CM = 3.6` (:8, совпадает с «одна физическая толщина 3.6 см»), + `MAX_STAIRS_PER_SPACE = 250` (:9, совпадает с «предел на этаж» и фикстурой), + `stairIntervalCount` (:170) и внутренний радиус ступеней винтовой лестницы + `stair.radius * scale * 0.18` (:298, совпадает с «начинаются на 0.18 + радиуса»), `isStair` (:180) — подтверждает, что валидация принимает любой + `radius > 0`, без нижней границы 30 см: претензия риска №2 («импорт с + радиусом меньше 30 см» как уже существующее, не вносимое этой задачей + состояние) подтверждена чтением, а не аннотацией. +- `demo/fixtures/large-house.mjs` — `STAIR_COUNT = 250` (:15), применение на + этаже 1 (:250) — совпадает с фикстурой бенчмарка, описанной в «Сценарии». +- `demo/golden/matrix.mjs` — все четыре названные в AC3 сцены существуют + (`stairs-flat-normal-light`, `stairs-flat-hover-dark`, + `stairs-flat-selected-light`, `stairs-isometric-dark`, :273–283) и + используют `stairLayerFixture`, в котором у `golden-stair-straight-small` + уже задано `opacity: 0.85` — то самое сочетание «частичная прозрачность + + прямая лестница», от которого риск №1 (расхождение сглаживания `path` и + `line`) зависит сильнее всего. Заявление «новых сцен не нужно» подтверждено. +- `demo/smoke_stairs.mjs`, `test/stairs.test.mjs`, `scripts/mutation-registry.mjs` + — все три файла существуют; в реестре мутантов уже есть прецеденты вида + `stairs-view-pan-opens-target-floor` (гард `node demo/smoke_stairs.mjs`, + файл `src/stairs-view.ts`), то есть предложенный в АС5 формат нового мутанта + `stairs-view-tread-lines` с тем же гардом — не изобретение, а повторение + устоявшегося в этом же реестре паттерна «один файл — один мутант», даже + когда смок проверяет разметку сразу двух рантаймов (View и редактор). +- PROCESS.md §5 — «перф» и «геометрия» в списке критериев `ask` + («геометрия, миграции конфига, публичные контракты, перф и touch, новый + UX-контракт») — трек соответствует заявленному обоснованию в шапке ТЗ. + +## Находки + +### Low — устаревший номер строки в «Проблема», п.2 + +ТЗ ссылается на «повторную проверку сырого конфига `stairList(raw)` (:34)» в +`src/stairs-view.ts`. Строка :34 сейчас — это объявление +`private get stairs(): Stair[] {`, сам вызов `stairList(...)` — строкой ниже, +:35. Разница в одну строку, не найдено её источника (рабочая копия и issue +сверены на одном и том же `b27ae06e`, `dev` за время ревью не двигался) — +похоже на опечатку при подсчёте, а не на дрейф от более нового коммита. + +**Почему Low, а не Medium:** ссылка — часть описательной прозы профилирования +(«что происходит при заходе на этаж 1»), не часть контракта К1 или оракула +AC1/AC2. Оба контрактных указателя — `src/stairs-view.ts:115` и +`src/stairs-editor.ts:535` — точны и проверены отдельно. Реализатора эта +строка не введёт в заблуждение: соседний код однозначно идентифицируется по +имени функции (`stairList`) и его единственному вызову в файле. Снимаю находку +сам, без возврата автору. + +Других расхождений факт/код не найдено: полная выборка из ~12 процитированных +номеров строк, имён функций и констант по пяти файлам (`stairs-view.ts`, +`stairs-editor.ts`, `stairs.ts`, `large-house.mjs`, `matrix.mjs`) совпала, +включая менее очевидные ссылки (`:320` на `cachedStairRenderGeometry`, `:9` на +`MAX_STAIRS_PER_SPACE`, значение 0.18 для внутреннего радиуса ступеней +винтовой лестницы). + +## Что проверено и корректно + +- **Обязательные разделы §7.1 — все на месте:** сценарий, что человек увидит + до/после, проблема (с цифрами и профилем), скоуп/не-скоуп, контракт + поведения (К1), UX·данные·i18n·touch, граничные случаи (§2.6: данные, async, + редактор, объём, визуал — пять из шести явно пройдены, host/input неприменим + и это корректно для чисто рендер-перф задачи без ввода), AC1–AC5 с + доказательством и oracle, план автотестов (включая «чем краснеет» по + AC1–AC3), затронутые файлы, производительность и бюджеты, риски, откат, + release-артефакты. +- **Каждый AC однозначен и имеет названный способ доказательства** (unit / + smoke / golden / Full Performance / gate), с конкретными проверяемыми + условиями, а не «примерно»: AC1 — точная формула сборки `d` и инвариант «не + пересобирается повторно» с явным механизмом проверки (счётчик или `===`); + AC2 — точные числа элементов по кind и по поверхности (View/редактор); AC3 — + четыре названные golden-сцены без обновления baseline; AC4 — сравнение + медиан по конкретным профилям с прописанным планом на случай + недостижения эффекта («разбор `longTasks.switchCycle` по индексам 0,3,6,9, + без него AC не выполнен» — явный «красный» случай для количественного AC); + AC5 — конкретный мутант с именем и гардом. +- **Защитные AC доказаны**: АС1/АС2 имеют заполненную строку «чем краснеет» + («помощника нет на текущем коде» / «3–7 `line.hp-stair-tread` на текущем + коде»), АС5 называет конкретный мутант в реестре с гардом, который реально + красит АС2. Пустых столбцов нет. +- **Пиксельная идентичность (AC3) разобрана предметно**, а не декларативно: + спецификация выводит числовую границу непересечения ступеней (винтовая — + минимум 6.77 см при r=32 см против толщины линии 3.6 см) и называет + единственный класс визуального риска (сглаживание `path` vs `line` при + сильном отдалении), покрытый существующими сценами, включая комбинацию + «прямая лестница + `opacity: 0.85`», которая больнее всего для этого риска. +- **Не-скоуп обоснован фактурой, а не декларацией**: вариант s1 (трапеция + одним `path`) явно отклонён с конкретной причиной (видимое потемнение в + углах при `opacity < 1`, золотая сцена, которая это поймает), а не просто + «не делаем»; JS-гигиена из s2 отклонена с числом («не сдвинула заход на + этаж 1 дальше шума»), а не на глаз. +- **DoR (§2.5) закрыт полностью**: файлы и модули перечислены; i18n — «нет» + (обоснованно, разметка без текста); миграция/compatibility — «нет» + (персистентная модель не меняется); touch — адресован явной ссылкой на + `docs/TOUCH-SUPPORT.md` и смоком `smoke_stairs`; перф-бюджеты названы явно + («не меняются», бандл — полоса ±2000 Б, #699); release-артефакты по каждому + пункту явно «да» или «нет»; откат описан и тривиален (revert коммита, ни + данных, ни миграций, ни флагов); открытых продуктовых вопросов нет. +- **Единственный продуктовый вопрос решён автором по умолчанию, а не + вынесен владельцу голословно**: «делать ли К1 при маргинальной пользе для + реальных домов» закрыт ссылкой на уже данное владельцем поручение + («довести открытые задачи до S8») и явной инженерной оценкой цены/выгоды — + это не техническая деталь, а продуктовый вопрос приоритизации, и формат + («что неясно · предлагаемый вариант по умолчанию, владелец может + поменять») соответствует требуемому §7.1. +- **«Принято предположительно» — действительно технические пункты**: + расположение помощника строк разметки, не-изменение карточки + пространства/PDF, судьба трапеции/стрелки, временный харнесс для локальных + метрик АС4 — ни один не наблюдаем пользователем, продуктовых вопросов, + ошибочно не заданных владельцу, не нашёл. +- **Риск пересечения с #725/#739 признан, а не замолчан**: в момент ревью + #725 — `S4-spec-review`/`S7` по словам автора («на код-ревью»), не `S8`; + ТЗ явно требует снятия AC4 на ветке, приведённой к актуальному `dev`, а не + предполагает, что файлы разошлись раз и навсегда. +- **Трек `ask` обоснован корректно**: критерии «перф» и «геометрия» из §5 + применимы (цена переключения этажа меняется, меняется разметка символа), + файлы класса A (`src/stairs-view.ts`, `src/stairs-editor.ts`) есть, задача + не инфраструктурная. + +## Чего не проверял + +- Не гонял `typecheck`/`npm test`/`npm run build` — на этапе spec кода для + задачи не существует (ветки `issue/740-*` нет), это обязанность + реализатора и следующего код-ревью; зависимости/Chromium на этапе spec не + ставились (#696). +- Не запускал Full Performance и не оценивал правдоподобность абсолютных + цифр 37–46/171–231 мс и прогнозов «вдвое меньше на CI» числовым + моделированием — ТЗ само помечает их как «оценка, а не обещание», и + единственный судья — AC4 на реальном прогоне после реализации. +- Не проверял, что `s3`/`s1` эксперименты автора (ветки на его локальной + машине) существуют где-либо кроме описания в issue — принял методологию + («3 сэмпла × 3/2 раунда, конфигурации чередуются, медианы») как разумную + инженерную практику, без доступа к сырым данным. +- Не оценивал числовую границу «6.77 см при r=32 см» в АС полным перебором + радиусов 30–10000 см самостоятельно — проверил формулу (`inner = 0.18 × + radius`, `STAIR_STROKE_CM = 3.6`) на согласованность с текстом, не + пересчитал весь диапазон. +- Не проверял состояние #725/#739 глубже слов автора о статусе («на + код-ревью») — не моя обязанность на этом этапе; риск пересечения + зафиксирован в ТЗ как риск №4, а не как блокер DoR. + +## Вердикт + +Зелёный. ТЗ полное по §7.1, каждый AC однозначен, проверяем и имеет названный +способ доказательства; защитные AC (АС1, АС2) доказаны без пустых столбцов; +продуктовый вопрос решён автором по умолчанию в верном формате, без скрытых +допущений, выданных за факт. Единственная находка — Low, устаревший номер +строки в описательной прозе профилирования, не в контракте — снята +ревьюером без возврата автору. Открытых продуктовых вопросов нет. + +--- + +**Вердикт:** зелёный · заход r1 · блокирующих циклов 0/4 · High: 0 · Medium: 0 + +--- + + + +## Материал раунда + +- Ветка: `dev`, коммит `b27ae06ef815` — ребейз его осиротит, и это нормально: ниже якоря, которые ребейз не меняет. +- Дерево материала: `791d1e396d66c509f253ed021e03b0811e40055b` + ``` + git log --all --format='%H %T' | grep 791d1e396d66 + ``` +- Тело issue: `9cfcfc9e8157d5a25961e793ed530de257848fb5d870dee885fbd71103ffb3bd` +- Вердикт конвейера: `green` · High 0 · маршрут `fix`