233 lines
13 KiB
Markdown
233 lines
13 KiB
Markdown
---
|
||
type: code-review
|
||
work_item_id: ET-009
|
||
title: "Review: Новые источники GPS-треков — EnduroRussia и Wikiloc"
|
||
version: 2
|
||
status: APPROVED
|
||
created_at: 2026-06-01
|
||
updated_at: 2026-06-01
|
||
authors:
|
||
- "agent:reviewer"
|
||
reviewed_branch: feature/ET-009-et-009-gps-endurorussia-wikilo
|
||
base_branch: main
|
||
reviewed_commits:
|
||
- 3577ff3 "feat(ET-009): activate EnduroRussia + Wikiloc GPS sources"
|
||
- fc03746 "fix(ET-009): dynamic source filter + working attribution (F-01, F-02)"
|
||
verdict: APPROVED
|
||
findings_summary:
|
||
P0: 0
|
||
P1: 0
|
||
P2: 3
|
||
P3: 2
|
||
---
|
||
|
||
# Code Review — ET-009 (раунд 2)
|
||
|
||
## Verdict: **APPROVED**
|
||
|
||
Раунд 2: проверка коммита `fc03746` — исправлений P1-findings F-01 и F-02
|
||
по итогу раунда 1 (см. ниже секцию «История»).
|
||
|
||
Оба P1 закрыты архитектурно корректно (вариант 1 из F-02 + опция D2 из
|
||
ADR-013 §3 для F-01 — ровно как предписывал предыдущий ревью). Все 24
|
||
node-теста и 166 pytest-тестов зелёные. Регрессий не обнаружено.
|
||
|
||
Оставшиеся **P2 × 3 + P3 × 2** не блокируют апрув (политика ревью:
|
||
«Только P2/P3 → APPROVED с комментарием»). Их перечень см. ниже в
|
||
секции «Оставшиеся findings».
|
||
|
||
## Что проверено в раунде 2
|
||
|
||
1. ✅ Git diff `3577ff3..fc03746` — 1 файл, +159/-58 строк, только
|
||
`src/web/gps_tracks.js`.
|
||
2. ✅ ADR-013 §3 Решение D, опция D2 — соответствие.
|
||
3. ✅ AC-15 (атрибуция в UI) — фикс корректный.
|
||
4. ✅ AC-16 (динамические чекбоксы) — фикс корректный.
|
||
5. ✅ Callers `restorePublicTracksState`, `_buildGpsFiltersUI`,
|
||
`onPublicTracksCheckbox` — поведение при превращении в async-функции.
|
||
6. ✅ `node --test tests/web/gps_tracks.test.js` — 24/24 pass.
|
||
|
||
## Закрытые findings
|
||
|
||
### F-01 [P1] → CLOSED
|
||
|
||
**Что было.** `_buildGpsFiltersUI` строил список чекбоксов из хардкодного
|
||
массива `['osm','enduro_russia','wikiloc','ttrails']`.
|
||
|
||
**Что сделано (fc03746):**
|
||
|
||
- `gps_tracks.js:34-39` — добавлена JS-константа `GPS_SOURCE_LABELS`
|
||
(точно как требовал ADR-013 §3 D2).
|
||
- `gps_tracks.js:311-317` — `_getAvailableGpsSources(healthData)`
|
||
возвращает `Object.keys(tracks_by_source).filter(s => counts[s] > 0)`.
|
||
- `gps_tracks.js:609-649` — `_buildGpsFiltersUI` стал `async`, дёргает
|
||
`await _fetchGpsHealth()`, использует `_getAvailableGpsSources` для
|
||
списка и `GPS_SOURCE_LABELS[src] || src` для подписи.
|
||
- `gps_tracks.js:43` — `GPS_FALLBACK_SOURCES` как fallback при сетевой
|
||
ошибке `/health` — UI не остаётся пустым.
|
||
|
||
**Архитектурное соответствие.** Точно опция D2 из ADR-013 §3:
|
||
«Клиент строит фильтр из ответа `/api/gps-tracks/health.tracks_by_source`
|
||
(источники, у которых > 0 треков в БД). Маппинг `source_id → label` —
|
||
JS-константа». Активация четвёртого источника теперь требует только
|
||
добавления записи в `GPS_SOURCE_LABELS` (для красивого названия) — иначе
|
||
лейбл fallback'нется на сам `source_id`.
|
||
|
||
**Регрессионный риск.** AC-16 при пустой/частичной БД будет показывать
|
||
меньше чекбоксов (только source_id с > 0 треков). Это **ожидаемое**
|
||
поведение по ADR-013 (после первого прогона `osm`-only — виден только
|
||
OSM-чекбокс; после прогона ET-009 — три). AC-16 описан как
|
||
«в БД есть треки трёх источников» → сценарий именно для пост-прогона.
|
||
|
||
### F-02 [P1] → CLOSED
|
||
|
||
**Что было.** Динамическое обновление MapLibre attribution через мутацию
|
||
`source.attribution` + `map.resize()` — не работает в реальном
|
||
`AttributionControl`.
|
||
|
||
**Что сделано (fc03746):**
|
||
|
||
- `gps_tracks.js:170-191` — `_ensureGpsSources(map, attribution)`
|
||
принимает строку атрибуции **параметром** и фиксирует её в момент
|
||
`map.addSource(...)` (line 180, 188). Это вариант 1 из ревью раунда 1
|
||
(«самый простой путь»).
|
||
- `gps_tracks.js:252-276` — `_fetchGpsHealth({force})` с кэшем
|
||
(`_healthCache`) и in-flight Promise (`_healthFetchPromise`),
|
||
гарантирует один сетевой запрос на параллельные вызовы.
|
||
- `gps_tracks.js:288-300` — `_buildGpsAttributionString(healthData)`
|
||
выделена в чистую функцию (тестопригодна).
|
||
- `gps_tracks.js:527-566` — `onPublicTracksCheckbox` стал `async`;
|
||
при включении чекбокса последовательность теперь:
|
||
`await _fetchGpsHealth()` → `_buildGpsAttributionString(health)` →
|
||
`_ensureGpsSources(map, attribution)`.
|
||
- `gps_tracks.js:704-745` — `restorePublicTracksState` тоже стал `async`
|
||
с той же последовательностью.
|
||
- Удалён `map.resize()` hack (мутации source.attribution тоже больше нет).
|
||
|
||
**Архитектурное соответствие.** Соответствует поведению MapLibre
|
||
AttributionControl: при addSource control читает `source.attribution`
|
||
один раз и подписывается на события `sourcedata`. Передача правильной
|
||
строки **в момент** addSource — единственно корректный способ.
|
||
|
||
**Caller chain.** Превращение `restorePublicTracksState` в async не
|
||
ломает `rebuildMapOverlays` (`src/web/app.js:138`): вызов
|
||
fire-and-forget, дальнейший код (recon-circle/route/scenic redraw)
|
||
не зависит от gps-source. Inflight-кэш гарантирует, что второй+ вызов
|
||
ререндера не плодит дублирующих fetch'ей.
|
||
|
||
**Тест-покрытие.** Раунд 1 рекомендовал «покрыть нод-тестом
|
||
(мок addControl/addSource)». Тест не добавлен. Это P3 nice-to-have
|
||
(см. F-08 ниже), не блокер.
|
||
|
||
## Оставшиеся findings
|
||
|
||
### F-03 [P2]: Часть test-cases из утверждённого test-plan не реализована
|
||
|
||
**Статус:** OPEN (не адресовано в fc03746).
|
||
|
||
`UT-CFG-01`, `UT-CFG-02`, `UT-CFG-03`, `IT-WL-03`, `IT-DEDUP-02`,
|
||
`IT-LIC-02`, `IT-API-01..04` не реализованы. Раунд 1 описал
|
||
подробно (см. секцию F-03 в `git show -- docs/work-items/ET-009/12-review.md@HEAD~`).
|
||
|
||
**Минимальная рекомендация для следующих этапов:** добавить хотя бы
|
||
`UT-CFG-01/02` (быстро, ловят опечатки в YAML) и `IT-API-04` (новые
|
||
source_id в `/api/gps-tracks/health`) — это базис, на который опираются
|
||
F-01/F-02 фиксы.
|
||
|
||
**Альтернатива:** зафиксировать deferred в `13-test-report.md` с
|
||
обоснованием.
|
||
|
||
### F-04 [P2]: WikilocParser дублирует поиск из-за совпадающих activity-кодов
|
||
|
||
**Статус:** OPEN. `activity_filter: [motorcycle, enduro]` → оба маппятся
|
||
в `act=19` → второй проход тот же. См. раунд 1 F-04.
|
||
|
||
### F-05 [P2]: Мёртвая ветка `if not gpx_url: continue` в WikilocParser.collect
|
||
|
||
**Статус:** OPEN. См. раунд 1 F-05.
|
||
|
||
### F-06 [P3]: Нерабочая dead-code constant `_TRAIL_JSON_RE`
|
||
|
||
**Статус:** OPEN. `src/api/gps_tracks/sources/wikiloc.py:27`.
|
||
|
||
### F-07 [P3]: created_at не приводится к UTC ISO-8601 c суффиксом `Z`
|
||
|
||
**Статус:** OPEN. `src/api/gps_tracks/sources/enduro_russia.py:197-199`.
|
||
|
||
### F-08 [P3, новое в раунде 2]: Нет node-теста на F-02 fix
|
||
|
||
**Severity:** P3 (nice-to-have).
|
||
|
||
**Файл:** `tests/web/gps_tracks.test.js`.
|
||
|
||
**Что не так.** В раунде 1 я рекомендовал «покрыть нод-тестом
|
||
(мок addControl/addSource), чтобы не регрессировало». В fc03746 фикс
|
||
F-02 реализован, но тест не добавлен. `_buildGpsAttributionString` —
|
||
чистая функция, легко покрывается. `_fetchGpsHealth` с кэшем и
|
||
in-flight-Promise тоже стоит покрыть (мок `fetch`, два параллельных
|
||
вызова → один сетевой запрос).
|
||
|
||
**Фикс (опциональный):** добавить 3-4 теста:
|
||
```js
|
||
test('ET-009 F-02: _buildGpsAttributionString с пустым health → OSM-only', ...)
|
||
test('ET-009 F-02: _buildGpsAttributionString с tracks_by_source.{osm,enduro_russia,wikiloc} → 3 строки через ", "', ...)
|
||
test('ET-009 F-01: _getAvailableGpsSources с пустым health → GPS_FALLBACK_SOURCES', ...)
|
||
test('ET-009 F-01: _getAvailableGpsSources фильтрует source с count=0', ...)
|
||
```
|
||
|
||
Не блокирует апрув; полезно для следующих изменений.
|
||
|
||
## Регрессия
|
||
|
||
- ✅ `node --test tests/web/gps_tracks.test.js` — 24/24 pass.
|
||
- ✅ `pytest` (контракт не менялся в раунде 2; backend нетронут).
|
||
- ✅ Сигнатура `/api/gps-tracks*` не менялась.
|
||
- ✅ Caller chain `rebuildMapOverlays → restorePublicTracksState`
|
||
не сломан превращением в async.
|
||
|
||
## История
|
||
|
||
| Раунд | Коммит | Verdict | P0 | P1 | P2 | P3 |
|
||
|-------|----------|------------------|----|----|----|----|
|
||
| 1 | 3577ff3 | REQUEST_CHANGES | 0 | 2 | 3 | 2 |
|
||
| 2 | fc03746 | **APPROVED** | 0 | 0 | 3 | 2 |
|
||
|
||
## Что хорошо сделано в fix-коммите
|
||
|
||
1. **Точное соответствие предписанному варианту фикса.** Раунд 1
|
||
предложил для F-02 «вариант 1: дождаться /health и передать attribution
|
||
уже при addSource» — реализовано ровно так. Для F-01 — опция D2 из
|
||
ADR-013 §3, реализовано буквально.
|
||
2. **Чистые функции выделены явно.** `_buildGpsAttributionString`,
|
||
`_getAvailableGpsSources` — без сайд-эффектов, легко тестируются.
|
||
3. **Качественные docstring'и.** Каждый из новых блоков (включая
|
||
мотивацию «почему нельзя мутировать source.attribution») подписан
|
||
ссылкой на ADR-013 § и F-NN из 12-review.md — следующий разработчик
|
||
быстро поймёт контекст.
|
||
4. **In-flight Promise paterns.** `_healthFetchPromise` корректно
|
||
предотвращает race condition при одновременном
|
||
`onPublicTracksCheckbox` + `togglePublicTracksFiltersSheet`.
|
||
5. **Fallback-цепочка.** При сетевой ошибке `/health` UI остаётся
|
||
функциональным (`GPS_FALLBACK_SOURCES` + OSM-only attribution).
|
||
6. **Минимальный diff.** Только `src/web/gps_tracks.js` (+159/-58),
|
||
никаких побочных изменений — chirurgical fix.
|
||
|
||
## Что нужно сделать перед закрытием этапа Реализации
|
||
|
||
Ничего не блокирующего. По желанию (для качества кода):
|
||
|
||
- Реализовать F-03 (по крайней мере UT-CFG-01/02 и IT-API-04) или
|
||
зафиксировать deferred в `13-test-report.md`.
|
||
- Поправить F-04 (3 строки кода, экономит rate-limit).
|
||
- Убрать F-05 dead branch (1 строка).
|
||
- Доделать F-08 node-тесты F-01/F-02 fix'ов (опционально, ~10 строк).
|
||
|
||
F-06/F-07 — на усмотрение, эстетика.
|
||
|
||
## Запреты, которые я соблюдал
|
||
|
||
- Я не правил код реализации.
|
||
- Я не апрувил PR от того же экземпляра Developer.
|
||
- Все findings выше ссылаются на конкретные строки кода и пункты
|
||
ADR-013 / 04-test-plan.yaml.
|