195 lines
13 KiB
Markdown
195 lines
13 KiB
Markdown
---
|
||
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` склеивает все
|
||
`<trkseg>` одного `<trk>` в один плоский `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 по `<name>` |
|
||
| 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/Анализа, приёмку не задерживают.
|