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

type, work_item_id, title, version, status, created_at, updated_at, authors, reviewed_branch, base_branch, reviewed_commits, verdict, findings_summary, changelog

type	work_item_id	title	version	status	created_at	updated_at	authors	reviewed_branch	base_branch	reviewed_commits	verdict	findings_summary	changelog
code-review	ET-008	Review: GPS-треки с публичных платформ на карте	2	REQUEST_CHANGES	2026-06-01	2026-06-01	agent:reviewer	feature/ET-008-gps	main	0060003 "feat(gps-tracks) ET-008 публичные GPS-треки с публичных платформ" 3734b98 "feat(ET-008) GPS tracks pipeline, API, frontend layer"	REQUEST_CHANGES	P0 P1 P2 P3 1 4 3 4	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 (массив):

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

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

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

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

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):

"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)

Имеется в коде:

"length_m": row["length_m"],
# и нет ни одного "length_km"

Последствие: В _renderTrackPopupHtml (gps_tracks.js стр. 325):

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":

  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

Имеется:

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 личный трек визуально выше публичных

Имеется:

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.

Что починить:

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).

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).

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:

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:

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 для разработчика

# 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.