--- 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`, **сразу после** `
Эндуро
` ИЛИ выше него — по выбору разработчика». Реализация выбрала «выше заголовка» (совпадает с диаграммой §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-стенде.