reviewer(ET): auto-commit from reviewer run_id=18

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

@@ -2,9 +2,10 @@
 type: code-review
 work_item_id: ET-006
 title: "Code Review: Загрузка и визуализация GPX-треков"
-version: 1
-status: REQUEST_CHANGES
+version: 2
+status: APPROVED
 created_at: 2026-05-22
+updated_at: 2026-05-22
 authors:
   - "agent:reviewer"
 branch: feature/ET-006-gpx-upload
@@ -14,143 +15,113 @@ branch: feature/ET-006-gpx-upload
 
 ## Вердикт
 
-**REQUEST_CHANGES** — найден 1 баг уровня P1 (падение на больших треках,
-прямо нарушает REQ-NF-01). Остальное — P2/P3.
+**APPROVED** — блокирующих findings нет.
 
-Реализация в целом сильная: контракт интеграции ADR-002 соблюдён точно,
-стратегия парсинга ADR-003 выдержана, все REQ-F-01…F-13 присутствуют,
-unit-тесты добротные. Блокирует приёмку только P1-1.
+Повторное ревью после коммита `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 `main...HEAD`: `src/web/gpx.js` (новый, 1127 строк),
-  `src/web/index.html`, `src/web/app.css`, `src/web/app.js` (хук REQ-F-13),
+- 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
+## Статус findings прошлого ревью (v1)
 
-### P1 — must-fix
+### P1-1 — `trackStats` падал на больших треках — **ИСПРАВЛЕНО**
 
-#### 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 закрыт.
 
-`src/web/gpx.js:142-143`
+### P2-1 — статистика/профиль только по `tracks[0]` — **ИСПРАВЛЕНО**
 
-```js
-eleMin: Math.round(Math.min.apply(null, eles)),
-eleMax: Math.round(Math.max.apply(null, eles)),
-```
+Добавлены `aggregateStats()` (`gpx.js:211`) и `buildFileProfileSamples()`
+(`gpx.js:1119`). `renderStats` (`gpx.js:979`) и `renderElevationProfile`
+(`gpx.js:1011`) теперь сводят статистику и профиль по **всем** трекам
+файла. Решение по семантике корректное: длина и набор/сброс суммируются
+потрековно (скачок высоты на стыке треков не даёт ложный набор/сброс),
+мин/макс — экстремумы. Покрыто тестами (агрегация, трек без высот, все
+треки без высот, склейка профиля). Закрывает REQ-F-09/AC-02 для
+многотрековых файлов.
 
-`Function.prototype.apply` передаёт каждый элемент массива отдельным
-аргументом. Для трека с сотнями тысяч точек, у которых есть `<ele>`,
-массив `eles` превышает лимит числа аргументов JS-движка и
-`Math.min.apply` бросает `RangeError: Maximum call stack size exceeded`.
+### P2-2 — расчёт статистики не чанковался — **ИСПРАВЛЕНО**
 
-Путь отказа: `trackStats()` вызывается из `extractGpxModelChunked()`
-(`nextTrack`, строка 378) → исключение ловится в `parseGpxAsync().catch`
-(строка 453) и оборачивается в `PARSE_ERROR` → пользователь видит toast
-«Не удалось прочитать GPX-файл», файл не загружается.
+Добавлен `trackStatsChunked()` (`gpx.js:183`), вызывается в
+`extractGpxModelChunked` (`gpx.js:464`) — асинхронный путь парсинга
+больших файлов. Эквивалентность синхронному `trackStats` подтверждена
+тестом, в т.ч. на треке длиннее `CHUNK_SIZE`. ADR-003 §2 выдержан
+буквально. Синхронный `trackStats` остаётся в `extractGpxModel` — этот
+путь используется только unit-тестами, не загрузкой файла.
 
-Это прямо нарушает:
-- **REQ-NF-01** — «Рендеринг трека 500K точек… без видимых фризов»
-  (трек вообще не загрузится);
-- **E-03** теста — «Загрузить GPX-файл ~50 МБ»;
-- `08-data-requirements.md` §4 — «Предельный файл 50 МБ ≈ 500K+ точек».
+### P2-3 — «объём PR выходит за рамки ET-006» — **СНЯТО (ложное срабатывание)**
 
-Существующими unit-тестами не ловится — все тесты используют короткие
-массивы.
+Замечание 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 «одна задача = одна ветка» не нарушена. Претензий нет.
 
-**Фикс:** считать min/max обычным циклом (как уже сделано для
-`distanceKm` в той же функции и для `minE/maxE` в `buildProfileSamples`,
-строки 949-953 — там реализовано корректно). Достаточно убрать `apply`.
+### 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`), что и нужно для строгого режима модуля.
 
 ---
 
-### P2 — should-fix
-
-#### P2-1. Статистика и профиль высот учитывают только первый трек файла
-
-`src/web/gpx.js:890` (`renderStats`), `src/web/gpx.js:921`
-(`renderElevationProfile`)
-
-```js
-var st = file.tracks.length ? file.tracks[0].stats : null;   // renderStats
-var track = file.tracks.length ? file.tracks[0] : null;       // renderElevationProfile
-```
-
-Файл с несколькими `<trk>` (REQ-F-02; AC-02 «загружен GPX-файл с 3
-треками … в панели — одна запись (имя файла) с 3 треками внутри»)
-показывает в панели одну строку, но статистика (длина, набор/сброс,
-мин/макс) и профиль высот считаются **только по `tracks[0]`**. Длина и
-рельеф 2-го и 3-го треков нигде не отображаются.
-
-REQ-F-10/REQ-F-11 говорят «для активного трека», но активная сущность
-панели — файл (REQ-F-09, AC-02). ТЗ не определяет поведение для
-многотрекового файла. Нужно либо агрегировать статистику по всем трекам
-файла, либо дать выбор трека внутри файла — и зафиксировать это в ТЗ/AC.
-
-#### P2-2. Расчёт статистики не разбит на чанки (отклонение от ADR-003)
-
-`src/web/gpx.js:378` — `trackStats(points)` вызывается синхронно по
-полному массиву точек.
-
-ADR-003 §2 (решение, п.2): «Конвертация DOM → GeoJSON **и расчёт
-статистики** — чанками». Конвертация DOM → модель действительно разбита
-на чанки (`mapChunked`, `CHUNK_SIZE`), но `trackStats` выполняется одним
-синхронным проходом Haversine по всем точкам. Доминирующая стоимость
-(обход DOM) чанкуется корректно, поэтому намерение ADR в основном
-выдержано, но буква решения нарушена. Для 500K-точечного трека это
-дополнительный синхронный проход.
-
-#### P2-3. Объём PR выходит за рамки ET-006
-
-`git diff main...HEAD` помимо ET-006 содержит артефакты ET-002
-(`09-review.md`, `12-review.md`, `13-test-report.md`) и **всю фичу
-ET-005**: новый `src/web/units.js` (190 строк), ~180 строк правок
-`src/web/app.js`, тесты ET-005. Лог ветки содержит мерж-коммит
-`6effac9 Merge … ET-005 … into main`.
-
-Разработчик чужие артефакты не правил (правило CLAUDE.md №2 не нарушено)
-— ET-005/ET-002 «приехали» вместе с веткой. Но мерж этого PR молча
-вольёт два других work-item. Конвенция CLAUDE.md — одна задача на ветку
-`feature/PROJ-NNN-slug`. Owner/reviewer должен осознанно подтвердить,
-что это намеренный stacking веток, до мержа.
-
----
+## Остаточные findings (не блокируют)
 
 ### P3 — nice-to-have
 
-- **P3-1.** Несогласованность единиц в профиле высот. `renderStats`
-  форматирует длину через `formatKm` (учитывает `Units` из ET-005 →
-  может показать мили), а подписи оси (`gpx.js:987-989`) и tooltip
-  (`gpx.js:1043`) жёстко выводят «км». При выборе миль статистика —
-  в милях, профиль — в км.
-- **P3-2.** `childText()` (`gpx.js:241`) по имени подразумевает поиск
-  по прямым потомкам, но использует `getElementsByTagName` (поиск по
-  всем потомкам). На структуре GPX безвредно, имя вводит в заблуждение.
 - **P3-3.** Грязная история git: мерж устаревшей ветки (`8dc150a`)
-  продублировал коммиты ET-006 (`62c2ee8`/`fda40f2`,
+  продублировал 4 коммита ET-006 (`62c2ee8`/`fda40f2`,
   `2104f12`/`a0546ab`, `73b29ae`/`9fc1ef4`, `dcf3d24`/`9b930c5`).
-  Итоговое дерево корректно, но `git log`/`git bisect` запутаны.
-- **P3-4.** Дублирующийся `'use strict'` — `gpx.js:1` и `gpx.js:27`.
+  Итоговое дерево корректно, diff чистый; запутаны лишь `git log` /
+  `git bisect`. Исправление потребовало бы rewrite истории — на
+  усмотрение Owner при мерже (можно squash).
 - **P3-5.** Многосегментные треки: `collectRawTracks` склеивает все
   `<trkseg>` одного `<trk>` в один плоский `points` → один `LineString`.
-  Это соответствует модели TRZ §4 / data-requirements §3 (плоский
-  `points` на трек), то есть формально по ТЗ. Но при разрыве сегментов
-  рисуется прямая-перемычка и разрыв попадает в `distanceKm`. На будущее
-  стоит рассмотреть `MultiLineString` (TRZ REQ-F-04 это допускает).
+  Соответствует модели 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).
+  «≈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 коммит-фикс не внёс — проверено построчно.
 
 ---
 
@@ -169,24 +140,25 @@ ET-005**: новый `src/web/units.js` (190 строк), ~180 строк пра
 | REQ-F-07 множественная загрузка | OK | накопление, цвет по индексу |
 | REQ-F-08 удаление | OK | снимаются слои/источники/маркеры; активный → детали скрываются |
 | REQ-F-09 GPX Sheet | OK | `#sheet-gpx` на `.bottom-sheet`, авто-открытие, `tb-gpx` |
-| REQ-F-10 профиль высот | ⚠ | canvas 120px, интерактивность OK; **P2-1** (только tracks[0]) |
-| REQ-F-11 статистика | ⚠ | 5 полей, «—» без высот OK; **P2-1** (только tracks[0]) |
+| 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 производительность | ❌ | **P1-1** — падение на больших треках; **P2-2** — стат. не чанкуется |
+| 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. Частичное отклонение — см. **P2-2**.
+- **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) устранено.
 
 ---
 
@@ -194,28 +166,29 @@ ET-005**: новый `src/web/units.js` (190 строк), ~180 строк пра
 
 - Контракт интеграции ADR-002 выдержан буквально — минимальная
   поверхность связности, `app.js` остаётся валидным без `gpx.js`.
-- Z-order REQ-F-04 проверен: `ROUTE_BASE_LAYERS` совпадает с реальными
-  id слоёв OSRM (`route-line-0-outline` / `route-line-0` в `app.js`);
-  в `rebuildMapOverlays` маршрут пересоздаётся раньше GPX, поэтому
-  z-order переживает `setStyle` (AC-12, I-07).
+- 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
-  плюс чистые функции; компактный `MockDOMParser` без тяжёлого jsdom.
+  корректно снимаются (`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` это явно фиксирует). К разработчику претензий нет.
+автоматизированы — ожидаемо, они в зоне этапа тестирования
+(`test_gpx_upload.py` это фиксирует). К разработчику претензий нет.
 
 ---
 
-## Что нужно для APPROVED
+## Итог
 
-1. Исправить **P1-1** (обязательно).
-2. Разобраться с **P2-1** — решение по многотрековым файлам
-   (агрегация либо явная фиксация в ТЗ/AC).
-3. **P2-2**, **P2-3** — на усмотрение, желательно адресовать.
-4. P3 — по возможности.
+Блокирующих findings нет. PR готов к этапу тестирования. P3-3 / P3-5 /
+P3-6 — на усмотрение Owner/Анализа, приёмку не задерживают.