---
type: code-review
work_item_id: ET-007
title: "Review v2: Спутниковая карта (Схема / Спутник) — артефакты + код"
version: 2
status: APPROVED_WITH_COMMENTS
created_at: 2026-05-31
updated_at: 2026-05-31
authors:
  - "agent:reviewer"
branch: feature/ET-007-et-005
stage_reviewed: analysis + architecture + development (code + tests)
previous_review: v1 (REQUEST_CHANGES, 6 P1 / 6 P2 / 3 P3)
---

# Code Review v2 — ET-007: Спутниковая карта (Схема / Спутник)

## Вердикт

**APPROVED with comments.**

Все 6 P1-блокеров из v1 закрыты в спецификации **и** в коде. Реализация
корректна, прошли все 22 pytest-проверки (`tests/unit/test_base_layer.py`)
и все 33 поведенческих JS-теста (`tests/unit/base_layer.test.js`),
запускаемых под `node --test`. Архитектурные решения (Esri, M-A, S-B,
O-A, H-B) соблюдены в коде один-в-один; никаких отклонений от ADR-004
не обнаружено.

Остаются 4 не-блокирующих замечания P2 (часть из них — несвёрнутые
концы v1 P2) и 4 nice-to-have P3, включая мелкие расхождения между
текстом TRZ и фактической реализацией (код в одной точке делает чуть
больше, чем требует ТЗ — добавляет защитный `beforeId`). Эти правки
рекомендуется внести следующим коммитом, но они не препятствуют
переходу в `testing` / merge.

## Что проверено

### Артефакты (вторая итерация)
- `01-brd.md` v2 — P1-3 закрыт (риск №4: «авто-выключение hillshade
  не вводится»).
- `02-trz.md` v2 — P1-1, P1-2, P1-4, P1-5, P1-6 закрыты; добавлены
  §5.6 (контракт с `layerState.basemap`) и §5.7 (синхронизация halo).
- `03-acceptance-criteria.md` v2 — добавлены сценарии под P1-2/P1-5/P1-6.
- `06-adr/ADR-004-satellite-base-layer.md` (accepted) — добавлены
  §8 и §9 под P1-5 и P1-6; §5 переписан под реальные halo-id;
  §6 единая константа `#2a2a2a`; baseline dark исправлен `#1a1a2e`.
- `08-data-requirements.md` v2 — таблицы 5.1 / 5.2 (baseline POI per-
  theme и satellite-значения), `_savedBasemapState`, исправлен dark
  baseline на `#1a1a2e`.
- `10-tech-risks.md` v2 — R-1 переписан под реальные id; R-5
  обновлён с baseline `#1a1a2e`.
- `04-test-plan.yaml`, `04b-ui-test-cases.md`, `07-infra-requirements.md`
  — без изменений (v1 не требовали правок по тем findings).

### Код (новое в ветке)
- `src/web/index.html` (+11 строк) — блок `#base-seg` в `#terrain-popup`.
- `src/web/app.css` (+30 строк) — стили `.terrain-base-row`,
  `.base-seg`, CSS-hook `body.satellite-active #btn-basemap`.
- `src/web/app.js` (+368 строк) — блок ET-007, хук в
  `rebuildMapOverlays()`, синхронизация halo в `onTrailsCheckbox()` /
  `restoreTrailsState()`, инициализация в обеих ветках IIFE.
- `src/web/style.json` (+30) / `src/web/style-dark.json` (+30) — два
  halo-underlay-слоя `trails-track-halo-satellite` и
  `trails-path-bridleway-halo-satellite`, оба с `visibility: none` и
  размещены **перед** соответствующим базовым trails-слоем (z-order).

### Тесты
- `tests/unit/test_base_layer.py` (+301 строк) — 22 статических теста
  (HTML/CSS/JS-структура, halo-слои в обоих стилях, порядок halo
  перед базовым слоем, `restoreBaseLayerState()` первым в
  `rebuildMapOverlays()`, ≥4 вызова в init-путях) + сабпроцесс
  `node --test` для JS-suite.
- `tests/unit/base_layer.test.js` (+468 строк) — 33 поведенческих
  unit-теста через `new Function`-загрузку блока ET-007, с мок-DOM,
  мок-localStorage и мок-картой; покрывают U-01..U-05, U-10..U-11,
  I-01..I-04, I-07, halo, POI text-color/halo (P1-2), background P1-4
  обе темы, валидацию входа, отсутствие window._map, недоступный
  localStorage, z-order.

### Прогон
```
$ python -m pytest tests/unit/test_base_layer.py -v
22 passed in 0.16s

$ node --test tests/unit/base_layer.test.js
# tests 33, pass 33, fail 0
```
Полный `pytest tests/` падает только на `tests/unit/test_health.py`
из-за отсутствующего `shapely` в окружении — это инфраструктурная
проблема, не относится к ET-007.

---

## P1 — must-fix

**Нет.** Все 6 P1-findings из v1 закрыты.

| v1 ID | Категория | Где закрыто |
|-------|-----------|-------------|
| P1-1 | Несуществующие слои grade1..5 | TRZ §1 REQ-F-04, ADR-004 §5, Data §6, Tech-Risks R-1 — реальные id `trails-track-halo-satellite`, `trails-path-bridleway-halo-satellite` |
| P1-2 | POI text-color на спутнике | TRZ §1 REQ-F-04-POI, ADR-004 §5, Data §5.2, код `_applyPoiSatellitePaint()`, JS-тесты «P1-2» |
| P1-3 | BRD vs TRZ hillshade | BRD §5 риск 4 переписан под TRZ/ADR/AC: авто-выключение не вводится |
| P1-4 | background-color 3 источника | ADR-004 §6, TRZ §1 REQ-F-03, Data §5 — единая `#2a2a2a` для обеих тем; baseline dark `#1a1a2e`; JS-тесты обе темы |
| P1-5 | Контракт с `layerState.basemap` | TRZ §5.6, AC-02/AC-03 новые сценарии, ADR-004 §8, код `_savedBasemapState` + `_setBodyClass('satellite-active', …)`, CSS `body.satellite-active #btn-basemap { display:none !important }` |
| P1-6 | halo не синхронизирован с чекбоксами | TRZ §5.7, AC-04 новые сценарии, ADR-004 §9, код `_applyTrailHaloVisibility(map, base)` + хуки в `onTrailsCheckbox()` и `restoreTrailsState()` |

Спецификация и реализация на уровне поведения согласованы. Z-order
проверен JS-тестом «Z-order: satellite-base вставляется beforeId=первый
terrain/trails/poi слой» и Python-тестом
`test_halo_layers_below_real_trails`.

---

## P2 — should-fix

### P2-1 — `00-business-request.md` остался с `TBD` и неверным заголовком

**Где:** `docs/work-items/ET-007/00-business-request.md`:
```
# Business Request: ET-005: Спутниковая карта (Схема/Спутник)
Work Item ID: ET-007
## Description
TBD
```

v1 пункты P2-4 и P2-5 не закрыты. Заголовок всё ещё «ET-005»
(пересекается с фактической ET-005 «единицы измерения»), Description
— пустой. BRD/TRZ/AC уже содержат целевую формулировку, поэтому
блокировать поставку этим нельзя, но формальное основание для
возврата остаётся.

**Действие:** заменить заголовок на «Business Request: ET-007:
Спутниковая карта (Схема / Спутник)»; в Description вставить 2–3
предложения из BRD §1.

### P2-2 — Tech-Risks R-2 митигация частично противоречит BRD F-02

**Где:** `10-tech-risks.md` R-2 «Альтернативные провайдеры …
быстрый switch на следующего по приоритету — Mapbox или MapTiler —
потребует только введения переменной окружения для API-ключа (это
уже инфра-изменение, выходящее за scope ET-007)».

Caveat «выходящее за scope ET-007» добавлен в v2 — это улучшение.
Но формулировка «потребует только введения переменной окружения для
API-ключа» по-прежнему противоречит BRD F-02 («без API-ключа»),
ADR-004 §«Вариант P» (Mapbox/MapTiler отклонены именно по этому
критерию) и описанию out-of-scope в BRD §3.

**Действие:** в R-2 переписать митигацию честно: «при деградации Esri
без-API-ключевых публичных альтернатив с глобальным покрытием в
момент написания нет; реакция требует либо пересмотра BRD F-02
(возврат в Анализ), либо архитектурного решения о self-hosted
satellite tiles (новый ADR)».

### P2-3 — TRZ §3.2 оставлено двусмысленное указание про позицию блока

**Где:** TRZ §3.2: «в начале `#terrain-popup`, **сразу после**
`<div class="terrain-popup-title">Эндуро</div>` ИЛИ выше него — по
выбору разработчика».

Реализация выбрала «выше заголовка» (совпадает с диаграммой §3.1 и
закреплено тестом `test_base_toggle_placed_at_top_of_terrain_popup`).
ТЗ нужно привести в соответствие, иначе следующая итерация фичи
может молча вернуть блок ниже заголовка.

**Действие:** в §3.2 убрать ветку «сразу после» и оставить только
«в самом верху попапа, выше заголовка «Эндуро»».

### P2-4 — Отсутствует автотест для CSS-hook `body.satellite-active #btn-basemap` и для контракта halo-синхронизации с чекбоксами

**Где:** `tests/unit/test_base_layer.py` и `base_layer.test.js`.

Покрытие P1-5 и P1-6 на уровне поведения присутствует в JS-suite
**только** для частей, лежащих внутри блока ET-007 (применение
satellite-active класса делегировано на `_setBodyClass`, который в
мок-DOM деградирует в no-op — см. `_setBodyClass` lines 2989–2997).
В итоге не тестируется:

1. **CSS-rule** `body.satellite-active #btn-basemap { display:none }`
   — Python-тест `test_base_toggle_styles_defined` проверяет только
   `.terrain-base-row` / `.terrain-base-label` / `.base-seg`. AC-02
   сценарий «Кнопка «Базовая карта» скрывается на спутнике (P1-5)»
   формально не покрыт автоматическим тестом.
2. **Вызов `_applyTrailHaloVisibility(...)` из `onTrailsCheckbox`** —
   функция `onTrailsCheckbox` живёт ВНЕ блока ET-007 и в JS-suite не
   подгружается через `new Function`. Python-сторона лишь проверяет
   присутствие id `trails-*-halo-satellite` в `app.js`, но не сам
   вызов хука. AC-04 сценарии «Выключение «Грунтовки» скрывает и halo»
   формально не покрыты регресс-тестом.
3. **`_savedBasemapState` save/restore цикл** — поведение
   реализовано, но в JS-suite нет теста, который бы выставил
   `layerState.basemap = false` до перехода в спутник и проверил,
   что после возврата `osm-base.visibility === 'none'`. AC-02/03
   сценарий «Запоминание выбора Базовая карта» формально не покрыт.

Гэп ровно по тем границам, по которым были претензии v1 (P1-5/P1-6).
Код корректен (проверено вручную при ревью), но без регрессионных
тестов будущий рефакторинг `onTrailsCheckbox` или `applyBaseLayer`
может молча сломать контракт.

**Действие (минимум):**
- Python-тест: assert `'body.satellite-active'` и `'#btn-basemap'` в
  `app.css`.
- Python-тест: внутри `function onTrailsCheckbox(` тело содержит
  `_applyTrailHaloVisibility`; то же для `restoreTrailsState`.
- JS-тест: один кейс на цикл «schematic (basemap=false) → satellite
  → schematic», проверяющий, что `osm-base` остаётся `none` после
  возврата. Сейчас в moc-окружении нет `layerState` (он вне блока
  ET-007) — потребуется либо экспортировать `_savedBasemapState`,
  либо добавить тонкую заглушку `layerState` в `loadBaseLayerModule`.

Эту работу можно сделать одним PR. Не блокирует merge ET-007, но
блокирует «зрелость» теста.

---

## P3 — nice-to-have

### P3-1 — Расхождение TRZ §5.2 vs код в части `beforeId`

TRZ v2 §5.2 шаг 2.2: «addLayer (см. 4.2) **без beforeId**. Корректный
z-order гарантируется тем, что restoreBaseLayerState вызывается
ПЕРВЫМ в rebuildMapOverlays».

Код `app.js` `applyBaseLayer()`:
```js
const before = _firstOverlayLayerId(map);
map.addLayer({ id: SATELLITE_LAYER_ID, ... }, before);
```

Код **дополнительно** вычисляет `beforeId` через `_firstOverlayLayerId`
(ищет первый слой с префиксом `terrain-` / `trails-` / `poi-`). Это
защита от случая, когда `restoreBaseLayerState` вызван не первым (на
старте приложения, например). Поведение **более устойчивое**, чем
требует ТЗ, и тесты I-02 и Z-order это подтверждают. Но формально
spec ↔ code расходятся.

**Действие:** либо обновить TRZ §5.2 шаг 2.2 (добавить «с
опциональным `beforeId` от `_firstOverlayLayerId(map)` как
страховкой — не обязателен, поскольку порядок гарантирован O-A»),
либо снять защиту из кода и положиться чисто на O-A. Первый вариант
проще и оставляет код более устойчивым.

### P3-2 — Мёртвая константа `SATELLITE_HALO_LAYER_IDS`

`app.js:2914–2917`:
```js
const SATELLITE_HALO_LAYER_IDS = [
  'trails-track-halo-satellite',
  'trails-path-bridleway-halo-satellite',
];
```

Константа объявлена и экспортируется из фабрики юнит-тестов, но в
рантайме не используется ни единого раза — `_applyTrailHaloVisibility`
работает по локальному `pairs`. Либо использовать константу
(`pairs.forEach((p) => …)` → читать halo-id из неё), либо удалить.

### P3-3 — Мёртвая функция `_toggleSatelliteHalo`

`app.js:3107–3110`:
```js
function _toggleSatelliteHalo(map, enabled) {
  _applyTrailHaloVisibility(map, enabled ? 'satellite' : 'schematic');
}
```

Комментарий заявляет «обратная совместимость для существующих
unit-тестов», но в `tests/unit/base_layer.test.js` функция нигде не
вызывается и из фабрики не экспортируется. Если она использовалась в
промежуточной версии тестов — её можно безопасно удалить.

### P3-4 — Избыточная защита `typeof restoreBaseLayerState === 'function'` в `rebuildMapOverlays()`

`app.js:127–131`:
```js
if (typeof restoreBaseLayerState === 'function') {
  restoreBaseLayerState();
}
```

ADR-004 §2 явно решает, что фича остаётся в `app.js` — функция всегда
определена в том же файле. Защита оправдана для `rebuildGpxOverlays`
(ET-006), где функция живёт в отдельно подгружаемом `gpx.js`. Здесь
ничего не защищает.

Не блокирует, но запутывает читателя кода. То же относится к двум
вхождениям в init-IIFE (там защита тоже стоит).

---

## Сводка

| ID    | Severity | Категория                                    |
|-------|----------|----------------------------------------------|
| P2-1  | P2       | BR.md: TBD и заголовок «ET-005»              |
| P2-2  | P2       | Tech-Risks R-2 митигация частично vs BRD F-02|
| P2-3  | P2       | TRZ §3.2 — двусмысленное указание про позицию|
| P2-4  | P2       | Нет автотестов для CSS-hook и hook'ов halo (граница AC-02/04 P1-5/P1-6) |
| P3-1  | P3       | TRZ §5.2 «без beforeId» vs код с `_firstOverlayLayerId` |
| P3-2  | P3       | Мёртвая константа `SATELLITE_HALO_LAYER_IDS` |
| P3-3  | P3       | Мёртвая функция `_toggleSatelliteHalo`       |
| P3-4  | P3       | Избыточный `typeof === 'function'`           |

---

## Что хорошо

- **Покрытие P1 v1 — 6/6 в коде и в спецификации одновременно.** Это
  редкий случай: чаще в одном из двух фронтов остаются концы.
- **JS-suite 33 теста** через `new Function`-загрузку блока — изящный
  способ исполнить реальный production-код в Node без бандлера и без
  переписывания app.js в ES-модуль. Покрытие включает edge-cases
  (private mode → localStorage недоступен, `window._map` отсутствует,
  невалидное stored-значение, повторный toggle, dark/light темы,
  z-order при пустом наборе overlay'ев).
- **Маркеры блока `// >>> ET-007 base layer toggle block` /
  `// <<<`** — позволяют как читать блок целиком (поиск 30+ функций
  в 3132-строчном `app.js` — боль), так и подгружать его в тестах. То
  же решение применил ET-002 для POI; единый паттерн.
- **Декларативные halo-underlay-слои в `style.json` / `style-dark.json`,
  расположенные перед соответствующими `trails-*`** — z-order
  явно зафиксирован в стиле и закреплён регресс-тестом
  `test_halo_layers_below_real_trails` (оба файла стиля). Любое
  будущее «перенесём слой» сломает тест немедленно.
- **`_savedBasemapState` + CSS-class hook** — корректное и минимально
  инвазивное решение P1-5: модуль ET-007 не правит `layerState`
  существующего модуля, не редактирует `toggleLayer()`, не лезет в
  его обработчики; контракт реализован через одну приватную
  переменную и одну CSS-зависимость. Это самый низкий blast radius,
  который можно было выбрать.
- **Idempotent `addSource/addLayer`** через `if (!map.getSource(…))` /
  `if (!map.getLayer(…))` (R-6) — соответствует паттерну
  `terrain` / `trails` / `poi`.
- **Документация и changelog** во всех артефактах v2 точно указывают,
  какие P-findings из v1 ими закрыты (`changelog: "v2 ... P1-1..P1-6"`).
  Это резко упрощает повторное ревью.

## Что делать дальше

1. **APPROVED → merge в main.** Никаких P0/P1 нет, регресс не
   обнаружен, тесты зелёные.
2. **Параллельно или одним PR** закрыть P2-1, P2-2, P2-3, P2-4. P2-4
   — самое полезное (закрывает регрессионный риск для P1-5/P1-6
   логики). Остальные — гигиена документации.
3. **P3 — на усмотрение** разработчика/Owner. Самый полезный из них —
   P3-1: либо синхронизировать TRZ с кодом (рекомендуется), либо
   снять защитный `beforeId` (тогда поведение строго совпадает с
   ADR §O-A).
4. После merge — переход в `testing` stage. Браузерные кейсы
   (E-01..E-10, TC-UI-01..14) требуют Playwright-инфраструктуры,
   которой в репозитории пока нет (см. ET-002 ADR-0001, 07-infra-
   requirements.md). До её появления покрытие AC обеспечивается
   связкой статических Python-тестов + JS-unit + ручной приёмки
   на test-стенде.
