356 lines
22 KiB
Markdown
356 lines
22 KiB
Markdown
---
|
||
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-стенде.
|