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

---
type: code-review
work_item_id: ET-008
title: "Review: GPS-треки с публичных платформ на карте"
version: 2
status: REQUEST_CHANGES
created_at: 2026-06-01
updated_at: 2026-06-01
authors:
  - "agent:reviewer"
reviewed_branch: feature/ET-008-gps
base_branch: main
reviewed_commits:
  - 0060003 "feat(gps-tracks): ET-008 публичные GPS-треки с публичных платформ"
  - 3734b98 "feat(ET-008): GPS tracks pipeline, API, frontend layer"
verdict: REQUEST_CHANGES
findings_summary:
  P0: 1
  P1: 4
  P2: 3
  P3: 4
changelog:
  - "v1 (2026-06-01): REQUEST_CHANGES — на ветке отсутствовал код."
  - "v2 (2026-06-01): код появился (~7700 LOC). Проведено code-review против ТЗ v2, AC v2 и ADR-005..011."
---

# Code Review — ET-008 (v2)

## Verdict: **REQUEST_CHANGES**

На ветке появилась реализация (commits `0060003`, `3734b98`): backend
пакет `src/api/gps_tracks/`, pipeline `scripts/gps_collect.py`, фронт
`src/web/gps_tracks.js`, миграция, YAML-конфиги, docker-compose сервис
`gps-collector`, тесты, fixtures. Архитектурно сборка следует ADR-005…008
и REQ-F-01…F-03. Однако обнаружено **одно блокирующее (P0) расхождение**
с ТЗ, ломающее основной сценарий просмотра на детальном zoom, и
несколько P1-несоответствий контракту API.

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

1. ✅ `docs/work-items/ET-008/02-trz.md` v2 (draft) — REQ-F-01…F-20, REQ-NF-01…NF-07.
2. ✅ `docs/work-items/ET-008/03-acceptance-criteria.md` v2 (draft) — AC-01…AC-17.
3. ✅ `docs/work-items/ET-008/06-adr/` — ADR-005…011 (5 accepted, 2 proposed/blocking).
4. ✅ `CLAUDE.md` — конвенции, фазы, правила для агентов.
5. ✅ Git diff `main...feature/ET-008-gps` — 53 файла, +7705/-1218.
6. ✅ Прочитан исходник backend (config.py, db.py, dedup.py, endpoint.py,
   mvt.py, models.py, sources/{base,osm,enduro_russia,ttrails}.py).
7. ✅ Прочитан исходник frontend (gps_tracks.js целиком, изменения в
   app.js, app.css, index.html).
8. ✅ Прочитана миграция, scripts/gps_collect.py, оба YAML, requirements.txt,
   docker-compose.yml.
9. ✅ Прочитаны 4 тест-файла (dedup, endpoint, mvt, sources/osm) и fixtures.

## Findings

### F-01 [P0]: GeoJSON-слой полностью скрывается из-за рассогласования имён properties

**Severity:** P0 (blocker — нарушено REQ-F-13/F-14, AC-04, AC-08; основной
визуальный сценарий слоя на z ≥ 12 не работает).

**Где:**
- `src/api/gps_tracks/endpoint.py`, `_row_to_geojson_feature()` (стр. 51–84)
- `src/web/gps_tracks.js`, `applyGpsFilter()` (стр. 246–267) и
  `_buildColorExpression()` (стр. 71–88)

**Что обнаружено:**

GeoJSON endpoint отдаёт в `properties` поля `activity_type` и `sources`
(массив):

```python
"properties": {
    ...
    "activity_type": row["activity_type"],
    ...
    "sources": sources,           # list
    "external_urls": ext_urls,
    ...
}
```

MVT-эндпойнт (правильно по ТЗ §4.3) отдаёт `activity` (скаляр) и `source`
(скаляр первой sources):

```python
props = {
    ...
    "activity": row["activity_type"] or "other",
    "source": first_source,
    "sources": sources_str,        # comma-string
    ...
}
```

Клиентский фильтр в `applyGpsFilter()` использует **только** имена из
MVT-схемы:

```js
const filter = ['all',
  ['in', ['get', 'activity'], ['literal', activities]],
  ['in', ['get', 'source'], ['literal', sources]]
];
map.setFilter(window.gpsTracksLayer.layerGeoId, filter);
```

Для feature из GeoJSON-source-а `get('activity')` и `get('source')`
возвращают `null` → `['in', null, ['literal', […]]]` = `false` → **все
features фильтруются из показа**. То же касается `line-color`:
`_buildColorExpression('source')` matches по `['get', 'source']` →
GeoJSON-features попадают в fallback `'#808080'`.

**Воспроизведение:**
1. Включить чекбокс «Публичные треки».
2. Увеличить zoom до 12+. (`_syncGpsLayersVisibility` делает
   `gps-tracks-layer-geo` видимым и скрывает MVT-слой.)
3. На карте — ни одного публичного трека, хотя `/api/gps-tracks?bbox=…`
   отдаёт >0 features (`returned > 0`, `total_in_bbox > 0`). Toast
   «Показаны N треков из M…» возможен, но карта пустая.

**Ссылка на правило:** Reviewer severity «не реализовано требование ТЗ» → P0.
Затронуты:
- REQ-F-14 (фильтры мгновенно действуют через setFilter).
- REQ-F-17 (стили `gps-tracks-layer` с paint `match`).
- AC-04 Scenario «Поля feature.properties» — `feature.properties` обязан
  содержать `length_km`, см. также F-02 ниже.
- AC-08 «Фильтрация по активности»: «на карте отображаются только enduro
  и moto треки» — невозможно, т.к. на z ≥ 12 ВСЕ треки отфильтрованы.

**Что починить:**
Унифицировать contract. Один из двух вариантов:
- (рекомендуется) В `_row_to_geojson_feature` добавить дублирующие
  поля `activity` (= `activity_type`) и `source` (= `sources[0]`) —
  не ломая существующих потребителей. Параллельно проверить попап:
  `_renderTrackPopupHtml` уже читает `props.activity_type || props.activity`
  — менять не нужно.
- Либо переписать `applyGpsFilter`/`_buildColorExpression` так, чтобы они
  ветвились по `['has', 'activity']` vs `['get', 'activity_type']` + для
  source использовать `['in', 'osm', ['get', 'sources']]` через
  index-of / contains (MapLibre expressions поддерживают `in` с массивом
  правой части).

Тест, который должен поймать это (отсутствует):
`tests/web/gps_tracks.test.js` — feature `{activity_type:'enduro'}` после
`applyGpsFilter` остаётся видимой. Добавить в test-plan.

---

### F-02 [P1]: GeoJSON `feature.properties` отдаёт `length_m`, ТЗ требует `length_km`

**Severity:** P1 (must-fix — нарушение контракта API, описанного в
REQ-F-10 и AC-04; UI-попап показывает «—»).

**Где:** `src/api/gps_tracks/endpoint.py`, `_row_to_geojson_feature()`
(стр. 51–84).

**ТЗ REQ-F-10** (ст. 280–305):
```json
"properties": {
    "name": "...",
    "activity_type": "...",
    "user": "...",
    "created_at": "...",
    "length_km": 47.3,
    "sources": [...],
    "external_urls": [...]
}
```

**AC-04 Scenario «Поля feature.properties»:**
> Then каждая feature содержит: name, activity_type, user, created_at,
> length_km, sources (array), external_urls (array)

**Имеется в коде:**
```python
"length_m": row["length_m"],
# и нет ни одного "length_km"
```

**Последствие:** В `_renderTrackPopupHtml` (`gps_tracks.js` стр. 325):
```js
const lengthKm = typeof props.length_km === 'number' ? props.length_km.toFixed(1) : '—';
```
для GeoJSON-feature (z ≥ 12) `props.length_km` всегда `undefined` → в
попапе постоянно «📏 — км». MVT-features (z 8…11) показывают правильно,
т.к. MVT-builder уже считает `length_km` (`mvt.py` стр. 148).

**Что починить:** добавить в properties `length_km` (как минимум).
Поле `length_m` оставить, если используется. Аналогично уточнить
`points_count` и `created_at` для popup (см. также F-04).

---

### F-03 [P1]: REQ-F-04 не реализован полностью — все OSM-треки сохраняются как `activity_type='other'`

**Severity:** P1 (must-fix — функциональный пробел; пользователь не
сможет осмысленно использовать фильтр по активности, который явно
закреплён в AC-08).

**Где:** `src/api/gps_tracks/sources/osm.py`, `_parse_gpx_trackpoints()`
(стр. 154–284).

**ТЗ REQ-F-04** (ст. 119–144):
> Для треков с gpx_id — дополнительный запрос
> `GET /api/0.6/gpx/<id>` для метаданных (name, description, tags, user,
> timestamp). Этот запрос делаем отложенно в batch: накопить 100 id →
> запросить. Тип активности — из tags GPX-трека (`Tag: enduro/moto/mtb/hike/...`).

**Реализовано:**
- batch-запрос метаданных НЕ сделан;
- `name`, `description`, `tags`, `user` всегда `None`/`[]`;
- `activity_type` явно зашит как `"other"`:
  ```python
  track = TrackInsert(..., activity_type="other", ...)
  ```
- константа `OsmParser.MAPPING` (стр. 25–39) объявлена, но `map_activity`
  не вызывается — мёртвый код.

**Последствие:** в БД ВСЕ треки от OSM (единственного включённого
источника) попадают как `activity_type='other'`. Фильтр по активности
теряет смысл — пользователь видит только «Другое». AC-08 Scenario
«Фильтрация по активности» не может пройти на реальных данных.

**Что починить:** реализовать batch-fetch на `/api/0.6/gpx/<id>` по
накоплению ID, как описано в ТЗ. Использовать `self.map_activity(...)`
для tags. Если решено отложить — оформить ADR/коммент с явной отметкой
«частичная реализация REQ-F-04, follow-up tracked в …» и понизить
ожидания AC-08 в ТЗ (но это работа аналитика, не разработчика).

---

### F-04 [P1]: Health endpoint не соответствует REQ-F-12 / AC-06

**Severity:** P1 (must-fix — нарушение контракта; AC-06 явно перечисляет
обязательные поля, которых нет).

**Где:** `src/api/gps_tracks/endpoint.py`, `gps_health()` (стр. 196–232).

**ТЗ REQ-F-12 / AC-06** требуют поля:
| Поле               | Тип    |
| ------------------ | ------ |
| `db_path`          | str    |
| `db_size_mb`       | float  |
| `tracks_total`     | int    |
| `tracks_by_source` | dict   |
| `tracks_by_activity` | dict |
| `last_pipeline_run` | object с полями started/finished/regions/sources_ok/sources_error |
| `tile_cache_size`  | int    |

**Имеется:**
```python
return {
    "status": "ok",
    "db_path": db_path,
    "total_tracks": total_tracks,                # должно быть tracks_total
    "by_activity": by_activity,                  # должно быть tracks_by_activity
    "recent_pipeline_runs": recent_runs,         # должно быть last_pipeline_run (объект)
}
```

Отсутствуют: `db_size_mb`, `tracks_by_source`, `tile_cache_size`.
Переименованы: `tracks_total → total_tracks`, `tracks_by_activity →
by_activity`, `last_pipeline_run → recent_pipeline_runs` (массив, не
объект).

Также `recent_pipeline_runs` отдаёт «10 последних запусков», а ТЗ
требует ОДИН последний (агрегированно). Это влияет на UI/админский
view.

**Что починить:** привести JSON-схему к контракту. Минимум — добавить
`tracks_by_source` (вычислить по `sources_json` агрегацией в Python или
JSON_EACH в SQL), `db_size_mb` (через `os.path.getsize`), `tile_cache_size`
(через `len(_gps_tile_cache)` из `mvt.py`), `last_pipeline_run` объект
(берём первую строку из `pipeline_runs ORDER BY started_at DESC`,
агрегируем `sources_ok`/`sources_error` по последнему region).

Tests `test_i40_health_endpoint` сейчас закреплены на текущей неправильной
схеме — их тоже придётся обновить.

---

### F-05 [P1]: Z-order ET-006 не зафиксирован — `gps-tracks-layer` может оказаться выше `gpx-layer-*`

**Severity:** P1 (must-fix — нарушение REQ-F-17 paint requirements §7.1
и AC-10 Scenario «личный трек визуально выше публичных»).

**Где:** `src/web/gps_tracks.js`, `_findGpsInsertPosition()` (стр. 191–196).

**ТЗ §7.1:**
> На карте оба видны параллельно; z-order:
> `gps-tracks-layer` < `gpx-layer-*` (личные треки выше).

**AC-10 Scenario «Совместимость с ET-006»:**
> Then оба видны параллельно
> And личный трек визуально выше публичных

**Имеется:**
```js
function _findGpsInsertPosition(map) {
  const style = map.getStyle && map.getStyle();
  if (!style || !style.layers) return undefined;
  const routeLayer = style.layers.find(l => l.id === 'route-line' || l.id.startsWith('route-'));
  return routeLayer ? routeLayer.id : undefined;
}
```

Функция ищет только `route-*`. Если `gpx-layer-*` уже добавлен в стиль
(ET-006), но route-line ещё нет, gps-tracks-layer добавится **в конец**
(`before = undefined` → addLayer без beforeId → поверх всего стиля),
**в том числе поверх gpx-layer**. Это нарушает обязательное правило
из §7.1 / AC-10.

**Что починить:**
```js
function _findGpsInsertPosition(map) {
  const style = map.getStyle && map.getStyle();
  if (!style || !style.layers) return undefined;
  // Приоритет beforeId: gpx-layer-* (ET-006), затем route-* (если нет gpx).
  const gpxLayer = style.layers.find(l => l.id.startsWith('gpx-layer'));
  if (gpxLayer) return gpxLayer.id;
  const routeLayer = style.layers.find(l => l.id === 'route-line' || l.id.startsWith('route-'));
  return routeLayer ? routeLayer.id : undefined;
}
```

Соответствующий unit-тест добавить в `tests/web/gps_tracks.test.js`
(в test-plan он есть как WEB-INTEG).

---

### F-06 [P2]: Валидация bbox по площади отсутствует (REQ-NF-01)

**Severity:** P2 (should-fix).

**Где:** `src/api/gps_tracks/endpoint.py`, `_parse_bbox()` (стр. 17–48).

**ТЗ REQ-NF-01:**
> Bbox-параметр валидируется (диапазон координат, площадь).

**Имеется:** валидация диапазона `[-180,180]/[-90,90]`, проверка
`west<east`/`south<north`. Площадь не проверяется.

**Что починить:** добавить max площадь bbox (например, 10° × 10° = 100°²)
для предотвращения «штормового» сканирования БД.

---

### F-07 [P2]: Дефолт `gps-tracks-sources` в localStorage включает отключённые источники

**Severity:** P2.

**Где:** `src/web/gps_tracks.js`, `window.gpsTracksLayer.filters.sources`
(стр. 55).

```js
sources: ['osm', 'enduro_russia', 'ttrails'],
```

При том, что `config/gps_sources.yaml` явно держит `enduro_russia` и
`ttrails` в `enabled: false` (соответствует ADR-010/011 status=proposed).
ТЗ REQ-F-15 говорит дефолт = «все enabled». Не блокирует, но создаёт
галки в UI для источников, по которым в БД ничего нет.

**Что починить:** инициализировать sources списком из
`/api/gps-tracks/health.tracks_by_source.keys()` при отсутствии
localStorage-ключа. Это естественно подтянется после F-04.

---

### F-08 [P2]: «LRU-кэш» в `mvt.py` — на самом деле FIFO

**Severity:** P2.

**Где:** `src/api/gps_tracks/mvt.py`, `_gps_tile_cache` (стр. 12–24).

```python
def set_gps_cached_tile(z, x, y, data):
    if len(_gps_tile_cache) >= _GPS_TILE_CACHE_MAX:
        _gps_tile_cache.pop(next(iter(_gps_tile_cache)))   # FIFO, не LRU
    _gps_tile_cache[(z, x, y)] = data
```

ТЗ REQ-NF-04: «LRU-кэш в памяти процесса FastAPI: 1024 записи». Текущая
реализация — FIFO. Для тайлов это особо ухудшает: часто запрашиваемый
тайл, попавший в кэш первым, будет вытеснен раньше, чем редкий тайл,
попавший позже.

**Что починить:** `from functools import lru_cache` нельзя из-за
mutable invalidation; использовать `collections.OrderedDict` с
`move_to_end` при чтении, либо `cachetools.LRUCache`. Совместимая идея —
сделать `get_gps_cached_tile`:
```python
def get_gps_cached_tile(z, x, y):
    key = (z, x, y)
    if key in _gps_tile_cache:
        _gps_tile_cache.move_to_end(key)   # OrderedDict
        return _gps_tile_cache[key]
    return None
```

---

### F-09 [P3]: Мёртвое поле конфигурации `save_user_field`

**Severity:** P3 (nice-to-have).

**Где:** `config/gps_sources.yaml` — поле задано (`save_user_field:
true|false`); ни один модуль его не читает. AC-16 Scenario «Pipeline не
сохраняет запрещённые поля» рассчитывает, что это поле уважается.

**Что починить:** в `OsmParser`/upsert обрабатывать `save_user_field=false`
→ `user=None`. Сейчас OSM всё равно ставит `user=None` (см. F-03), но
поле должно работать как контракт для будущих источников.

---

### F-10 [P3]: Лишний импорт `pytest_asyncio` в `tests/api/test_gps_tracks_endpoint.py`

**Severity:** P3.

`import pytest_asyncio` есть, но `@pytest_asyncio.fixture` нигде не
используется (только `@pytest.mark.asyncio`). Не блокирует, но в чистом
коде убирается.

---

### F-11 [P3]: `MockRow(dict)` в `tests/api/test_gps_tracks_mvt.py`

**Severity:** P3.

Тесты используют `class MockRow(dict)` как замену `sqlite3.Row`. Работает
для текущего кода, но `sqlite3.Row` не поддерживает `__contains__` так
же, как dict. Если в `mvt.py` появится `if "x" in row:`, тесты
разойдутся с продом. Безопаснее использовать `sqlite3.Row` напрямую через
`open_db + INSERT + fetchone()`.

---

### F-12 [P3]: Лишняя проверка `"source_priority" in existing.keys()` в `db.py`

**Severity:** P3.

`src/api/gps_tracks/db.py` стр. 116:
```python
existing_priority = existing["source_priority"] if "source_priority" in existing.keys() else 999
```
Колонка `source_priority` объявлена в миграции (`migrations/gps_tracks_001_init.sql`
ст. 22) с `NOT NULL DEFAULT 999`. Проверка избыточна (read-protection
осталась от какой-то ранней итерации). Лучше убрать — иначе создаётся
впечатление, что колонка опциональна.

---

## Соответствие ADR

| ADR | Status | Соблюдение в коде | Замечания |
|-----|--------|-------------------|-----------|
| ADR-005 storage-schema | accepted | ✅ соблюдено | таблица `tracks` (+`source_priority`), индексы и `pipeline_runs` совпадают |
| ADR-006 dedup-algorithm | accepted | ✅ соблюдено | `compute_dedup_key` 1-в-1; покрыт unit-тестами U-10…U-14 |
| ADR-007 pipeline-architecture | accepted | ✅ соблюдено | сервис `gps-collector` с `profiles:["batch"]`, license-guard в `_check_license_adr` |
| ADR-008 tile-vs-geojson | accepted | ⚠️ частично | переключение по zoom есть; но contract фич GeoJSON vs MVT расходится (см. F-01) |
| ADR-009 osm-licensing | accepted | ✅ соблюдено | attribution в source, рабочий парсер |
| ADR-010 enduro-russia (proposed) | proposed | ✅ соблюдено | parser — заглушка, `enabled: false`, license-guard сработает |
| ADR-011 ttrails (proposed) | proposed | ✅ соблюдено | то же |

License-guard в `scripts/gps_collect.py` `_check_license_adr()` корректно
читает YAML front-matter ADR. Покрытия unit-тестом нет — рекомендую
добавить (`test_pipeline_skips_unaccepted_source`).

## Тесты — оценка

| Тест-файл | Покрытие test-plan | Качество |
|-----------|-------------------|----------|
| `test_gps_tracks_dedup.py` | U-10…U-14 | ✅ хорошо |
| `test_gps_tracks_mvt.py` | U-50…U-52 | ✅ адекватно (см. F-11) |
| `test_gps_tracks_endpoint.py` | I-20…I-23, I-30…I-31, I-40 | ⚠️ AC-06 не покрыт корректно — тест зафиксирован на неверной схеме (F-04) |
| `test_gps_tracks_sources_osm.py` | U-42…U-44 | ✅ defusedxml проверен фикстурой xxe-payload.gpx |

**Не покрыто тестами:**
- `tests/web/gps_tracks.test.js` — заявлен в ТЗ §5 ст. 787 и
  `04-test-plan.yaml`, ОТСУТСТВУЕТ. Был бы первой защитой от F-01.
- pipeline (`scripts/gps_collect.py`) — нет ни одного теста на
  `_check_license_adr`, `_collect_source_for_region`, dry-run, exit-code.
  AC-02 Scenarios «Падение одного источника не валит остальные» и
  «Dry-run» по факту не верифицированы.
- bbox area-валидация — отсутствует и в коде (F-06), и в тестах.

## Замечания, не доходящие до P-finding

- `_simplify_coords` в `mvt.py` дублирует `simplify_coords` из `src/api/main.py`.
  Не критично, но напрашивается общая утилита.
- `src/api/main.py` ст. 17–20: `GPS_TRACKS_DB_PATH` вычисляется **до**
  импорта shapely/fastapi, в строгом смысле это «нечистый» импорт-time
  side-effect. Не блок.
- В `endpoint.py` `init_db` вызывается на каждый запрос (`_get_conn`).
  Это означает, что `executescript` выполняется на каждый запрос. SQL
  использует `IF NOT EXISTS`, так что функционально ок, но это лишний
  I/O. Рекомендую инициализировать БД один раз при `create_gps_router`.
- `src/api/main.py` ст. 1255: `app.include_router(gps_router)` хорошо
  встроено перед `StaticFiles` mount — порядок правильный.

## Воспроизведение P0 для разработчика

```bash
# 1. Запустить с тестовой БД (см. test_i20_geojson_basic):
pytest tests/api/test_gps_tracks_endpoint.py::test_i20_geojson_basic -q

# 2. Вытащить feature.properties — наблюдать "activity_type", "sources" (list),
#    отсутствие "length_km".

# 3. Открыть DevTools в браузере на dev-стенде, проверить:
window._map.queryRenderedFeatures({layers:['gps-tracks-layer-geo']})
# → пустой массив на z >= 12, потому что setFilter с ['get','activity'] всё скрыл.

# 4. Временный обход для отладки:
window._map.setFilter('gps-tracks-layer-geo', null)
# → треки появятся, но серые (line-color fallback) — что подтверждает F-01.
```

## Рекомендация для CI

- `pytest tests/api/` сейчас зелёный, потому что:
  - `test_i40_health_endpoint` фиксирует текущую неправильную схему
    (F-04) → нужно поправить вместе с фиксом;
  - frontend-тесты отсутствуют (F-01 не отлавливается).
- `make lint` ожидаемо проходит.
- Перед закрытием задачи CI должен прогонять и frontend-тесты
  (`tests/web/gps_tracks.test.js`), которые на данный момент не написаны.
- Не закрывать ET-008 в Plane как Done.

## Итог

**REQUEST_CHANGES.** Минимальный объём правок:
1. F-01 (P0) — унифицировать имена properties между MVT и GeoJSON, +
   web-тест на `applyGpsFilter`.
2. F-02 (P1) — добавить `length_km` в GeoJSON.
3. F-03 (P1) — реализовать batch-fetch `/api/0.6/gpx/<id>` и mapping
   `activity_type` (или явная декларация частичной реализации REQ-F-04).
4. F-04 (P1) — выровнять `gps_health()` под REQ-F-12 / AC-06.
5. F-05 (P1) — корректный `beforeId` в `_findGpsInsertPosition`.

После исправлений P0/P1 — повторить ревью; P2/P3 могут пройти отдельным
PR follow-up.