docs(ET-009): reviewer round 2 — F-01/F-02 CLOSED, APPROVED

docs/work-items/ET-009/12-review.md

@@ -0,0 +1,232 @@
+---
+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.