diff --git a/docs/work-items/ET-007/12-review.md b/docs/work-items/ET-007/12-review.md new file mode 100644 index 0000000..5bb469d --- /dev/null +++ b/docs/work-items/ET-007/12-review.md @@ -0,0 +1,416 @@ +--- +type: code-review +work_item_id: ET-007 +title: "Review: Спутниковая карта (Схема / Спутник) — артефакты анализа и архитектуры" +version: 1 +status: REQUEST_CHANGES +created_at: 2026-05-31 +updated_at: 2026-05-31 +authors: + - "agent:reviewer" +branch: feature/ET-007-et-005 +stage_reviewed: analysis + architecture +--- + +# Code Review — ET-007: Спутниковая карта (Схема / Спутник) + +## Вердикт + +**REQUEST_CHANGES** — найдено 6 P1-блокирующих findings и 6 P2/3 +nice-to-have. Кода в ветке ещё нет (только документация анализа и +архитектуры); цель ревью — не пропустить эти расхождения в +разработку, иначе разработчик будет вынужден угадывать намерения +или возвращаться за уточнениями. + +Сами решения (Esri World Imagery; ленивый raster-source; восстановление +через `rebuildMapOverlays()`; гибридное halo) — обоснованы и +непротиворечивы внутри ADR-004. Проблема в нестыковках между ТЗ / +ADR / Data / Tech-Risks и в нескольких ссылках на сущности в коде +(`trails-grade1..5`, `paths-bridleway`, baseline-цвета), которых в +реальном `src/web/` нет. + +## Что проверено + +- `docs/work-items/ET-007/01-brd.md` (v1, status draft) +- `docs/work-items/ET-007/02-trz.md` (v1, status draft) +- `docs/work-items/ET-007/03-acceptance-criteria.md` (v1, status draft) +- `docs/work-items/ET-007/04-test-plan.yaml` (v1) +- `docs/work-items/ET-007/04b-ui-test-cases.md` (v1) — просмотрено +- `docs/work-items/ET-007/06-adr/ADR-004-satellite-base-layer.md` (accepted) +- `docs/work-items/ET-007/07-infra-requirements.md` (approved) +- `docs/work-items/ET-007/08-data-requirements.md` (approved) +- `docs/work-items/ET-007/10-tech-risks.md` (approved) +- `docs/architecture/README.md` (обновлён ET-007) +- `docs/architecture/adr/README.md` (добавлен ADR-004) +- Diff `origin/main...HEAD`: 12 файлов, 2020 вставок, 1 удаление — + только `docs/`. Кода в ветке нет; ревью на этапе разработки + понадобится повторно. +- Сверка с фактическим кодом: `src/web/app.js` (3132 строки), + `src/web/style.json` (136), `src/web/style-dark.json` (136). +- CLAUDE.md + +--- + +## P1 — must-fix перед началом разработки + +### P1-1 — TRZ / ADR / Data / Tech-Risks ссылаются на несуществующие слои + +**Где:** +- TRZ §1 REQ-F-04 таблица — строки «Trails (grade1..5)» и «Paths/bridleway». +- ADR-004 §5 пункт 1 — «Для линий **grade1..5** и paths/bridleway… парные + underlay-слои `*-halo-satellite`». +- Data §6 таблица — `trails-grade1`, `trails-grade2`, `paths-bridleway`, + и т.д. +- Tech-Risks R-1 — «массив `['trails-grade1', 'trails-grade2', ...]` с + производным правилом `-halo-satellite`». + +**Что в коде:** в `src/web/style.json` (стр. 42–92) и `style-dark.json` +определены только три line-слоя поверх OSM: + +| id в style.json | filter | как обрабатываются grade | +|---|---|---| +| `trails-asphalt` | `highway in primary/secondary/tertiary/residential` | без grade | +| `trails-track` | `highway == track` | **одно** paint-выражение `match` по `tracktype` (`grade1..5`) — см. style.json:64–72 | +| `trails-path-bridleway` | `highway in path/bridleway/footway` | без grade | + +Слоёв `trails-grade1`, `trails-grade2`, …, `trails-grade5`, +`paths-bridleway` **нет** ни в одном из стилей. Поэтому: + +- Halo-стратегия H-B (пара underlay-слоёв на каждый base-слой) на + практике даёт 2–3 пары (`trails-track-halo-satellite`, + `trails-path-bridleway-halo-satellite`, опционально + `trails-asphalt-halo-satellite`), а **не** 5+ пар, как написано. +- Чтобы получить «разный halo по grade», halo-слой `trails-track-halo-satellite` + должен сам содержать `match` по `tracktype` (если halo должен + зависеть от grade) — либо halo единого цвета на весь `trails-track`. + ADR не оговаривает. +- Tech-Risks R-1 митигация «массив `['trails-grade1', …, 'trails-grade5']`» + введёт разработчика в заблуждение и приведёт либо к `addLayer` от + несуществующих base, либо к молчаливому `getLayer === undefined`. + +**Действие:** обновить TRZ §1 REQ-F-04, ADR-004 §5, Data §6, Tech-Risks +R-1, чтобы они оперировали реальными id (`trails-track`, +`trails-path-bridleway`, при необходимости `trails-asphalt`) и явно +зафиксировали, нужен ли halo, чувствительный к `tracktype`, или +единый halo на весь `trails-track`. + +### P1-2 — Контраст POI labels на спутнике спецификацией сломан + +**Где:** +- TRZ §1 REQ-F-04 таблица, строка «POI labels» — «`text-halo-color: #000000`, + `text-halo-width: 2px` для читаемости на спутнике». +- ADR-004 §5 пункт 2 — «`text-halo-color` (`#000000` на спутнике …) и + `text-halo-width` (`2` на спутнике …) — через `setPaintProperty`». +- TestPlan I-24 — «`text-halo-color` === `#000000`». + +**Что в коде:** `style.json:128–133` для `poi-labels`: +``` +"text-color": "#333333", +"text-halo-color": "#ffffff", +"text-halo-width": 1.5 +``` +В `style-dark.json` аналогично, `text-color` тёмный. + +Меняем halo с белого на чёрный, при этом текст остаётся `#333333` — +**тёмный текст на тёмном halo не читаем**. Это прямо нарушает цель +требования («для читаемости на спутнике»). + +**Действие:** в TRZ/ADR явно поменять на спутнике **`text-color`** +тоже (например, `#ffffff` или `#f0ede6`) или зафиксировать другую +схему контраста (инвертированная пара). Если на схеме `text-color` +должен оставаться `#333333` — это нужно вернуть в `applyBaseLayer('schematic')`, +т.е. baseline POI text-color должен попасть в перечень «исходных +значений» Data §5. + +### P1-3 — Прямое противоречие BRD ↔ TRZ/ADR/AC насчёт hillshade + +**Где:** +- BRD §5 риск 4 (hillshade): «По умолчанию hillshade **отключается** + при включении спутника — поведение фиксируется в TRZ». +- TRZ §1 REQ-F-04 таблица, строка «Hillshade»: «Включается/выключается + чекбоксом как раньше; **по умолчанию НЕ авто-выключается**». +- ADR-004 §«Открытые вопросы» / Раздел «контекст / 1.5»: «оставить + как есть». +- AC-04 сценарий «Hillshade поверх спутника»: ожидается одновременный + показ обоих слоёв (т.е. поведение без авто-выключения). + +BRD говорит «отключать», все нижестоящие документы — «не отключать». +Развилка проектная, нужно выбрать одну версию. + +**Действие:** обновить BRD §5 риск 4, чтобы он совпадал с TRZ/ADR/AC +(митигация = «hillshade продолжает работать поверх спутника как и +поверх схемы; визуальную проверку даёт UI-тест AC-04»). Если же +правильное решение всё-таки «авто-выключать» — нужно +синхронно править TRZ, ADR-004, AC-04 и TestPlan. + +### P1-4 — Цвет фона `background-color` в режиме «Спутник» — три расходящихся источника + +**Где:** +- TRZ §1 REQ-F-03: «цвет фона `#2a2a2a` для тёмной темы и **`#1a1a1a` + для светлой темы** в режиме «Спутник»» — две константы, причём + светлая получает **более тёмный** фон (`#1a1a1a` < `#2a2a2a`), что + выглядит как опечатка / перестановка. +- ADR-004 §6: одна константа `#2a2a2a` для обеих тем + (`setPaintProperty('background', 'background-color', '#2a2a2a')`). +- Data §5 строка «baseline-значения `background-color`»: `#f0ede6` + (light), **`#1a1a1a`** (dark) — но фактическое значение в + `style-dark.json:28` — `"#1a1a2e"` (а не `#1a1a1a`). + +Итого: четыре цвета упомянуто, два не совпадают с фактическим стилем +и сам ADR противоречит ТЗ по количеству констант. + +**Действие:** +1. Выбрать модель: «одна satellite-bg-константа на обе темы» (ADR-вариант, + проще) или «по константе на тему» (TRZ-вариант, эстетичнее под тёмные + снимки). Зафиксировать в TRZ и ADR одинаково. +2. Если TRZ-вариант — проверить, что светлая тема не получает более + тёмный фон, чем тёмная (исправить вероятную опечатку + `#1a1a1a`/`#2a2a2a`). +3. В Data §5 baseline тёмной темы заменить `#1a1a1a` → `#1a1a2e` + (фактическое значение из `style-dark.json:28`). + +### P1-5 — В `app.js:380–386` уже есть `layerState.basemap: ['osm-base']` и `toggleLayer('basemap')`; контракт взаимодействия не описан + +**Где (код, app.js:380):** +```js +const layerState = { tracks: true, paths: true, poi: true, basemap: true }; +const layerGroups = { …, basemap: ['osm-base'] }; +function toggleLayer(group) { …setLayoutProperty('osm-base', 'visibility', …) } +``` +Это существующий UI-механизм, способный включать/выключать `osm-base` +независимо от подложки. Ни TRZ, ни ADR-004 его не упоминают. + +**Открытые вопросы (не отвечены ни в одном артефакте):** +- При `applyBaseLayer('satellite')` мы выставляем `osm-base.visibility=none`. + Что делать с `layerState.basemap`? Сбрасывать в `false` (тогда после + возврата на «Схему» пользователь обнаружит, что схема скрыта) или + оставлять `true` (тогда `layerState` рассинхронизирован с реальной + видимостью)? +- При возврате на «Схему» (`applyBaseLayer('schematic')`) мы безусловно + выставляем `osm-base.visibility=visible` (TRZ §5.2 шаг 3.1). Если у + пользователя `layerState.basemap === false`, это нарушит его выбор. +- В UI существует ли кнопка `btn-basemap` (упоминается в `toggleLayer`)? + Что с ней делает «Спутник»: скрыть, заблокировать, оставить как + «выключатель схемы поверх спутника»? + +**Действие:** в TRZ §5.2 (алгоритм `applyBaseLayer`) и в AC-02/AC-03 +добавить раздел «Взаимодействие с существующим toggleLayer('basemap')». +Рекомендация — на спутнике переключатель «Базовая карта» либо скрыт +из UI, либо означает «показывать схему поверх спутника» (гибрид) — +но это уже out of scope (BRD §3). Самый простой контракт: при входе +в «Спутник» запомнить `layerState.basemap`, при выходе — восстановить. + +### P1-6 — `restoreTrailsState()` и обработчики «Грунтовки»/«Тропы» не управляют halo-слоями + +**Где (код, app.js:2798–2821):** `restoreTrailsState()` выставляет +`visibility` только для `trails-track` и `trails-path-bridleway`. +Обработчики чекбоксов «Грунтовки» (`onTrailsTrackCb` / аналогично) — тоже. + +В режиме «Спутник» при выключении чекбокса «Грунтовки» базовый слой +скрывается, а **halo-underlay-слой `trails-track-halo-satellite` +останется видимым** (его никто не трогает). Получим «фантом» белой +линии на спутнике без основной линии сверху. + +Ни TRZ, ни ADR-004 не правят `restoreTrailsState()` и обработчики +чекбоксов. Контракт неполный. + +**Действие:** в TRZ §5 (либо отдельный пункт «синхронизация halo с +чекбоксами слоёв») задать, что: +- `restoreTrailsState()` должен учитывать текущий base layer и + одновременно править видимость пары halo-слоя; +- обработчики чекбоксов «Грунтовки» / «Тропы» — то же самое; +- источник истины: «halo видим ⇔ (base === satellite) AND (соответствующий + чекбокс ON)». + +То же замечание про POI: при выключении чекбокса «POI» в «Спутник» +динамические правки halo текста (P1-2) тоже не нужны — но это будет +автоматически, потому что меняется `visibility` всей группы; явно +зафиксировать в TRZ. + +--- + +## P2 — should-fix + +### P2-1 — TRZ §5.2 алгоритм противоречит решению ADR-004 «Вариант O» + +**Где:** TRZ §5.2 шаг 2.2: + +> 2.2. Если layer 'satellite-base' отсутствует — addLayer (см. 4.2), +> вставлять **beforeId** = id первого слоя trails-* или terrain-* +> (первый из существующих) — чтобы спутник оказался под terrain и trails. + +В ADR-004 §«Вариант O» именно вариант **O-B** («через явный `beforeId` +первого trails-слоя») **отклонён** как ненадёжный («terrain/trails +добавляются асинхронно; использовать `beforeId` слоёв, которых ещё +нет, нельзя»). Выбран **O-A** — «вызвать `restoreBaseLayerState()` +первым в `rebuildMapOverlays()`», без `beforeId`. + +Алгоритм в TRZ остаётся в варианте O-B. Разработчик возьмёт TRZ. + +**Действие:** в TRZ §5.2 шаг 2.2 убрать `beforeId` или поменять +формулировку на «addLayer без beforeId; порядок гарантируется тем, +что restoreBaseLayerState вызывается первым в rebuildMapOverlays +(ADR-004 §«Вариант O», O-A)». + +### P2-2 — Tech-Risks R-2 митигация противоречит BRD F-02 / ADR §«Вариант P» + +**Где:** Tech-Risks R-2: «быстрый switch на следующего по приоритету — +Mapbox или MapTiler — потребует только введения переменной окружения +для API-ключа». + +Но BRD F-02 явно требует «без API-ключа», ADR-004 §«Вариант P» +отклоняет Mapbox/MapTiler именно по этому критерию. Значит, такой +switch — **не** «один объект source-spec», а пересмотр функциональных +требований BRD (и инфра-требований: введение переменной окружения, +секретов, ротации). + +**Действие:** в R-2 заменить митигацию на честную: «при деградации +Esri единственный безапи-ключевой кандидат — отсутствует; реакция +требует пересмотра BRD F-02 (вход в Анализ) либо архитектурного +решения о self-hosted-тайлах». + +### P2-3 — Baseline `background-color` тёмной темы в Data §5 неверный + +**Где:** Data §5: «`#1a1a1a` (dark) — задублированы из `style*.json`». +**Что в коде:** `style-dark.json:28` — `"#1a1a2e"`. + +(Дубль с P1-4 в части цифры, но это узкая правка — поправить значение +в Data §5.) + +### P2-4 — `00-business-request.md` содержит только `TBD` + +**Где:** `00-business-request.md` — секция Description пустая (TBD). +По формальным правилам репозитория BR должен содержать первичное +формулирование запроса; всё остальное (BRD, TRZ, AC) уже опирается +на это как на «источник правды». Анализ-этап де-факто содержит +правду в BRD §1, но BR оставлен незаполненным. + +**Действие:** заполнить BR двумя-тремя предложениями (можно +конспектом из BRD §1) — это снимет одно из формальных оснований +для возврата. + +### P2-5 — Несогласованный заголовок: «ET-005» при `work_item_id: ET-007` + +**Где:** `00-business-request.md` — `# Business Request: ET-005: Спутниковая карта (Схема/Спутник)` / `Work Item ID: ET-007`. Аналогично `.task.md` title. + +ET-005 — это уже сделанная задача про единицы измерения (km/mi, +ADR-0001). Двойное использование «ET-005» в заголовке этой задачи +сбивает с толку. + +**Действие:** заменить заголовок на «ET-007: Спутниковая карта +(Схема / Спутник)» — как и в BRD/TRZ/AC. + +### P2-6 — TRZ §3.2 даёт две взаимоисключающие инструкции про позицию блока + +**Где:** +- §3.1 диаграмма: «Подложка» в **самом верху** попапа (над «Эндуро»). +- §3.2 текст: «в начале `#terrain-popup`, **сразу после** + `
Эндуро
` ИЛИ **выше него** — + по выбору разработчика». + +Диаграмма и текст не совпадают. AC-01 не уточняет позицию. + +**Действие:** в §3.2 зафиксировать одну позицию (рекомендуется «выше +заголовка Эндуро», что соответствует §3.1). + +--- + +## P3 — nice-to-have + +### P3-1 — TRZ §5.5: избыточная защита `typeof restoreBaseLayerState === 'function'` + +**Где:** TRZ §5.5 показывает пример встраивания: +```js +if (typeof restoreBaseLayerState === 'function') { + restoreBaseLayerState(); +} +``` +Эта защита оправдана для `rebuildGpxOverlays` (ET-006), потому что +GPX-фича живёт в отдельном файле `gpx.js`, который теоретически может +не подгрузиться. ADR-004 §2 явно решает, что фича остаётся в `app.js` +— значит функция всегда определена в том же файле, и `typeof === 'function'` +ничего не защищает. + +Не блокирует, но запутывает читателя кода. Уточнить намерение или +просто вызвать функцию напрямую. + +### P3-2 — AC-08 пороги без метода замера + +**Где:** AC-08 — «первая спутниковая плитка отображается в течение +≤ 500 мс», «смена визуально мгновенная (≤ 100 мс)». Тест-план не +указывает, как именно мерить (Performance API, throttling сети, +`map.once('idle')`). 500 мс ≈ длина сетевого RTT — пограничный +порог, который без чёткого метода замера сорвётся то в одну, то в +другую сторону. + +**Действие:** в TestPlan TP-Performance (если он есть в e2e suite, +сейчас в `04-test-plan.yaml` есть только e2e-base-layer-workflow без +явного НФТ-кейса) добавить процедуру замера и условия сети. + +### P3-3 — `rebuildMapOverlays()` уже вызывает `restoreTrailsState`/`restorePoiState` после `restoreTerrainState` — порядок halo-инициализации в «Спутник» нетривиален + +Маленькая ремарка к ADR-004 §«Вариант O»: правило «satellite-base +первым» гарантирует только z-order **базы** относительно terrain/trails. +Но halo-underlay-слои тоже должны лечь **под** соответствующие base-line-слои. +Если halo-слои добавлены прямо в `style.json` (как предполагает ADR §5), +их порядок относительно `trails-track` определяется порядком в массиве +`layers` стиля, а не порядком восстановления. Стоит явно отметить в +ADR §5 / Data §6, что в `style.json` halo-слой ставится **непосредственно +перед** соответствующим базовым (выше `osm-base`, ниже `trails-track`), +иначе halo окажется поверх базовой линии и испортит вид и на схеме, и +на спутнике (включён только тогда halo не виден из-за `visibility:none`, +но как только включится — проблема). + +--- + +## Сводка + +| ID | Severity | Категория | Действие | +|-------|----------|---------------------------------|-----------------| +| P1-1 | P1 | Несуществующие слои grade1..5 | TRZ/ADR/Data/Risks — переписать под реальные id | +| P1-2 | P1 | POI labels: тёмный текст + чёрный halo | TRZ/ADR — добавить смену text-color | +| P1-3 | P1 | BRD vs TRZ — hillshade авто-выкл| Обновить BRD §5 | +| P1-4 | P1 | background-color: 3 источника | Свести к одному; исправить опечатку и baseline | +| P1-5 | P1 | Контракт с layerState.basemap не задан | TRZ §5.2 + AC | +| P1-6 | P1 | halo не синхронизирован с чекбоксами Грунтовки/Тропы | TRZ §5 | +| P2-1 | P2 | TRZ §5.2 beforeId vs ADR O-A | Поправить TRZ §5.2 | +| P2-2 | P2 | Tech-Risks R-2 митигация противоречит BRD F-02 | Переформулировать R-2 | +| P2-3 | P2 | Baseline dark `#1a1a1a` ≠ `#1a1a2e` в стиле | Data §5 | +| P2-4 | P2 | BR.md — `TBD` | Заполнить | +| P2-5 | P2 | Заголовок «ET-005» при id ET-007| Переименовать | +| P2-6 | P2 | TRZ §3.2 двусмысленность позиции| Зафиксировать одну | +| P3-1 | P3 | Избыточный `typeof === 'function'`| Уточнить или убрать | +| P3-2 | P3 | AC-08 без метода замера | Добавить процедуру в TestPlan | +| P3-3 | P3 | Порядок halo в style.json не оговорён | Уточнить в ADR §5 | + +## Что хорошо + +- ADR-004 — добротный: явное обоснование выбора по 5 осям (P/M/S/O/H) + с таблицей альтернатив, явная классификация «minor change», + раздел «Технический долг» с конкретным сценарием миграции в + `basemap.js`. +- Архитектурный реестр `docs/architecture/adr/README.md` и + `docs/architecture/README.md` обновлены синхронно с ET-007 (раздел + «Внешние тайл-провайдеры»). +- Tech-Risks (R-1..R-10) — полное покрытие, со ссылками на конкретные + митигации (UI-тесты, идемпотентные паттерны, точки расширения). + R-2 — единственный «высокий», но честно зафиксирован. +- Data §7 (PII) и Infra §3 (внешние сетевые вызовы) чётко описывают + расширение круга третьих сторон и сохраняют «приватный по умолчанию» + через ленивое создание source. +- TestPlan по структуре богатый (unit + integration + e2e + error + handling). AC покрывает 10 сценариев, включая mobile (AC-09) и + регресс существующего функционала (AC-10). + +## Что делать дальше + +1. Исправить P1 (см. Действие в каждом пункте) — обновить TRZ, ADR, + BRD, Data, Tech-Risks. После правок поднять `version` затронутых + документов до v2. +2. Желательно — закрыть P2-1…P2-6 в том же раунде. +3. P3 — на усмотрение Owner/Architect. +4. После правок — повторное ревью артефактов (этот же документ, v2). +5. **Только после этого** — переход к stage `development`. + +Кода в ветке нет; ревью кода будет отдельным шагом после +реализации.