From 94f651774211bf7ec345ba1ceb0d49b2abee6dde Mon Sep 17 00:00:00 2001 From: claude-bot Date: Tue, 2 Jun 2026 05:27:07 +0000 Subject: [PATCH] =?UTF-8?q?docs(ET-009):=20reviewer=20round=202=20?= =?UTF-8?q?=E2=80=94=20F-01/F-02=20CLOSED,=20APPROVED?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- docs/work-items/ET-009/12-review.md | 232 ++++++++++++++++++++++++++++ 1 file changed, 232 insertions(+) create mode 100644 docs/work-items/ET-009/12-review.md diff --git a/docs/work-items/ET-009/12-review.md b/docs/work-items/ET-009/12-review.md new file mode 100644 index 0000000..f68ca59 --- /dev/null +++ b/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.