reviewer(ET): auto-commit from reviewer run_id=29
All checks were successful
CI / lint (push) Successful in 3s
CI / test (push) Successful in 5s
CI / build (push) Successful in 3s

This commit is contained in:
2026-05-31 20:07:34 +00:00
parent 231c99c045
commit 29d8461c0c

View File

@@ -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', ...]` с
производным правилом `<base>-halo-satellite`».
**Что в коде:** в `src/web/style.json` (стр. 4292) и `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:6472 |
| `trails-path-bridleway` | `highway in path/bridleway/footway` | без grade |
Слоёв `trails-grade1`, `trails-grade2`, …, `trails-grade5`,
`paths-bridleway` **нет** ни в одном из стилей. Поэтому:
- Halo-стратегия H-B (пара underlay-слоёв на каждый base-слой) на
практике даёт 23 пары (`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:128133` для `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:380386` уже есть `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:27982821):** `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`, **сразу после**
`<div class="terrain-popup-title">Эндуро</div>` ИЛИ **выше него**
по выбору разработчика».
Диаграмма и текст не совпадают. 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`.
Кода в ветке нет; ревью кода будет отдельным шагом после
реализации.