--- type: code-review work_item_id: ET-006 title: "Code Review: Загрузка и визуализация GPX-треков" version: 2 status: APPROVED created_at: 2026-05-22 updated_at: 2026-05-22 authors: - "agent:reviewer" branch: feature/ET-006-gpx-upload --- # Code Review — ET-006: Загрузка и визуализация GPX-треков ## Вердикт **APPROVED** — блокирующих findings нет. Повторное ревью после коммита `25e4476` («fix(gpx): устранить падение статистики на больших треках, учесть все треки файла»), который закрывает все замечания v1. P1-1, P2-1, P2-2 исправлены и подтверждены чтением кода и регрессионными тестами. P2-3 снят как ложное срабатывание (см. ниже). Остаются только три P3 nice-to-have — не блокируют приёмку. Реализация сильная: контракт интеграции ADR-002 соблюдён буквально, стратегия парсинга ADR-003 выдержана полностью, REQ-F-01…F-13 присутствуют, unit-тесты добротные и пополнены регрессиями по ревью. ## Что проверено (v2) - ТЗ: `docs/work-items/ET-006/02-trz.md` (v2) - AC: `docs/work-items/ET-006/03-acceptance-criteria.md` (v2) - ADR: `06-adr/ADR-002` (структура модуля), `06-adr/ADR-003` (парсинг) - Data Requirements: `08-data-requirements.md` - Diff `origin/main...HEAD` (а не stale local `main` — см. P2-3): `src/web/gpx.js` (новый, 1242 строки), `src/web/index.html` (+46), `src/web/app.js` (+2: хук REQ-F-13), `src/web/app.css` (+148), `tests/unit/gpx.test.js`, `tests/unit/test_gpx_upload.py` - Коммит-фикс `25e4476` целиком (`gpx.js` +233/-60, `gpx.test.js` +96) - CLAUDE.md --- ## Статус findings прошлого ревью (v1) ### P1-1 — `trackStats` падал на больших треках — **ИСПРАВЛЕНО** `Math.min/max.apply` по массиву высот убран. Расчёт переписан на однопроходный аккумулятор (`makeStatsAccumulator` / `accumulatePoint` / `finalizeStats`, `gpx.js:102-172`). `apply` в горячем пути отсутствует. Проверена эквивалентность нового однопроходного расчёта старому: длина и мин/макс — по всем точкам, набор/сброс — по той же подпоследовательности точек с высотой, тот же фильтр шума `ELE_NOISE_M`. Добавлена регрессия `gpx.test.js` — трек 500K точек, `assert.doesNotThrow`. REQ-NF-01 закрыт. ### P2-1 — статистика/профиль только по `tracks[0]` — **ИСПРАВЛЕНО** Добавлены `aggregateStats()` (`gpx.js:211`) и `buildFileProfileSamples()` (`gpx.js:1119`). `renderStats` (`gpx.js:979`) и `renderElevationProfile` (`gpx.js:1011`) теперь сводят статистику и профиль по **всем** трекам файла. Решение по семантике корректное: длина и набор/сброс суммируются потрековно (скачок высоты на стыке треков не даёт ложный набор/сброс), мин/макс — экстремумы. Покрыто тестами (агрегация, трек без высот, все треки без высот, склейка профиля). Закрывает REQ-F-09/AC-02 для многотрековых файлов. ### P2-2 — расчёт статистики не чанковался — **ИСПРАВЛЕНО** Добавлен `trackStatsChunked()` (`gpx.js:183`), вызывается в `extractGpxModelChunked` (`gpx.js:464`) — асинхронный путь парсинга больших файлов. Эквивалентность синхронному `trackStats` подтверждена тестом, в т.ч. на треке длиннее `CHUNK_SIZE`. ADR-003 §2 выдержан буквально. Синхронный `trackStats` остаётся в `extractGpxModel` — этот путь используется только unit-тестами, не загрузкой файла. ### P2-3 — «объём PR выходит за рамки ET-006» — **СНЯТО (ложное срабатывание)** Замечание v1 — артефакт измерения. Прошлое ревью считало diff против **локального `main`** (`832099c`), который был устаревшим. Против `origin/main` (`6effac9`, уже содержит ET-001/002/005) diff PR — **строго ET-006**: 19 файлов, `gpx.js` + 3 правки разметки + 2 строки `app.js` + тесты + docs ET-006. Ни одного чужого work-item. Конвенция CLAUDE.md «одна задача = одна ветка» не нарушена. Претензий нет. ### P3-1 — единицы профиля высот — **ИСПРАВЛЕНО** Ось (`gpx.js:1078-1080`) и tooltip (`gpx.js:1155`) форматируют расстояние через `formatKm()` — согласовано со статистикой и выбором км/мили (ET-005). ### P3-2 — имя `childText` — **ИСПРАВЛЕНО** Переименована в `firstTagText` (`gpx.js:326`), JSDoc уточнён. ### P3-4 — дублирующийся `'use strict'` — **ИСПРАВЛЕНО** Верхнеуровневый `'use strict'` убран; остался один — внутри IIFE (`gpx.js:26`), что и нужно для строгого режима модуля. --- ## Остаточные findings (не блокируют) ### P3 — nice-to-have - **P3-3.** Грязная история git: мерж устаревшей ветки (`8dc150a`) продублировал 4 коммита ET-006 (`62c2ee8`/`fda40f2`, `2104f12`/`a0546ab`, `73b29ae`/`9fc1ef4`, `dcf3d24`/`9b930c5`). Итоговое дерево корректно, diff чистый; запутаны лишь `git log` / `git bisect`. Исправление потребовало бы rewrite истории — на усмотрение Owner при мерже (можно squash). - **P3-5.** Многосегментные треки: `collectRawTracks` склеивает все `` одного `` в один плоский `points` → один `LineString`. Соответствует модели TRZ §4 / data-requirements §3, формально по ТЗ. При разрыве сегментов рисуется прямая-перемычка, разрыв попадает в `distanceKm`. На будущее — рассмотреть `MultiLineString` (REQ-F-04 это допускает). - **P3-6.** Числовые примеры в `04-test-plan.yaml` некорректны: U-10 «≈28.3 км» (Haversine даёт ≈25.5 км), U-11 «elevLoss = 70 м (30+20)» (30+20=50). Реализация и тесты следуют TRZ §5 правильно; расхождение задокументировано в шапке `gpx.test.js`. Примеры стоит поправить на этапе Анализа (правило CLAUDE.md №3) — артефакт не входит в код PR. Новых P0/P1/P2 коммит-фикс не внёс — проверено построчно. --- ## Соответствие требованиям ### ТЗ — REQ-F / REQ-NF | Требование | Статус | Комментарий | |---|---|---| | REQ-F-01 кнопка загрузки | OK | `btn-gpx-upload` между компасом и геолокацией; `accept=".gpx" multiple` | | REQ-F-02 парсинг | OK | DOMParser, GPX 1.1, trk/trkseg/wpt/rte, fallback без namespace | | REQ-F-03 валидация | OK | лимит 50 МБ, toast для empty и oversize | | REQ-F-04 отрисовка трека | OK | line 4px, opacity 0.85, палитра 8 цветов, z-order ниже OSRM | | REQ-F-05 waypoints | OK | circle + symbol, цвет файла, label по `` | | REQ-F-06 fit bounds | OK | padding 50, только по последнему файлу | | REQ-F-07 множественная загрузка | OK | накопление, цвет по индексу | | REQ-F-08 удаление | OK | снимаются слои/источники/маркеры; активный → детали скрываются | | REQ-F-09 GPX Sheet | OK | `#sheet-gpx` на `.bottom-sheet`, авто-открытие, `tb-gpx` | | REQ-F-10 профиль высот | OK | canvas 120px, интерактивность; сводно по всем трекам файла (P2-1 закрыт) | | REQ-F-11 статистика | OK | 5 полей, «—» без высот; сводно по всем трекам файла (P2-1 закрыт) | | REQ-F-12 интерактивность на карте | OK | click активирует трек, cursor pointer | | REQ-F-13 сохранение при setStyle | OK | хук в `rebuildMapOverlays`, данные на `window`, z-order восстанавливается | | REQ-NF-01 производительность | OK | P1-1 устранён; парсинг и расчёт статистики чанкуются (P2-2 закрыт) | | REQ-NF-03 UX | OK | toast вместо alert, параллельно с роутингом | | REQ-NF-04 хранение | OK | только в памяти, без localStorage | ### ADR - **ADR-002** — соблюдён буквально. `gpx.js` — отдельный классический скрипт, подключён после `app.js`; `app.js` получил ровно один хук `if (typeof rebuildGpxOverlays === 'function') rebuildGpxOverlays();` (+ строка-комментарий), защищённый `typeof`; контракт интеграции (`_map`, `openSheet`/`closeSheet`, `showToast`) выдержан. - **ADR-003** — соблюдён полностью. `DOMParser` в основном потоке, Web Worker не используется, конвертация DOM → модель **и расчёт статистики** идут чанками с отдачей управления event loop. Частичное отклонение из v1 (P2-2) устранено. --- ## Положительные моменты - Контракт интеграции ADR-002 выдержан буквально — минимальная поверхность связности, `app.js` остаётся валидным без `gpx.js`. - P1-1-фикс сделан правильно: однопроходный аккумулятор переиспользован и синхронным `trackStats`, и чанковым `trackStatsChunked` — единый путь расчёта, без дублирования логики. - P2-1-фикс корректен по семантике: набор/сброс агрегируются потрековно, а не по сквозному потоку точек — стык треков не порождает ложный набор высоты. - Z-order REQ-F-04: `ROUTE_BASE_LAYERS` совпадает с реальными id слоёв OSRM; в `rebuildMapOverlays` маршрут пересоздаётся раньше GPX — z-order переживает `setStyle` (AC-12). - Безопасность: имя файла экранируется (`escapeHtml`), имена waypoints идут в MapLibre `text-field` (не в HTML), генерируемые id безопасны. - Обработчики событий карты отслеживаются по файлу (`mapHandlers`) и корректно снимаются (`clearMapHandlers`) — нет утечки слушателей. - Unit-тесты добротные: исполняют реальный модуль, покрывают U-01…U-21, чистые функции и регрессии по ревью (P1-1, P2-1, P2-2). Интеграционные (I-01…I-12) и e2e (E-01…E-10) сценарии не автоматизированы — ожидаемо, они в зоне этапа тестирования (`test_gpx_upload.py` это фиксирует). К разработчику претензий нет. --- ## Итог Блокирующих findings нет. PR готов к этапу тестирования. P3-3 / P3-5 / P3-6 — на усмотрение Owner/Анализа, приёмку не задерживают.