--- 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/` для метаданных (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, как описано в ТЗ. Использовать `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= _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/` и 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.