enduro-trails/docs/work-items/ET-007/12-review.md

type, work_item_id, title, version, status, created_at, updated_at, authors, branch, stage_reviewed, previous_review

type	work_item_id	title	version	status	created_at	updated_at	authors	branch	stage_reviewed	previous_review
code-review	ET-007	Review v2: Спутниковая карта (Схема / Спутник) — артефакты + код	2	APPROVED_WITH_COMMENTS	2026-05-31	2026-05-31	agent:reviewer	feature/ET-007-et-005	analysis + architecture + development (code + tests)	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():

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:

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:

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:

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-стенде.