From e1dd7039a7b95dca45e9dcba0be54917b166d292 Mon Sep 17 00:00:00 2001 From: claude-bot Date: Fri, 22 May 2026 06:04:55 +0000 Subject: [PATCH] reviewer(ET): auto-commit from reviewer run_id=18 --- docs/work-items/ET-006/12-review.md | 241 ++++++++++++---------------- 1 file changed, 107 insertions(+), 134 deletions(-) diff --git a/docs/work-items/ET-006/12-review.md b/docs/work-items/ET-006/12-review.md index 4265723..8a9ab98 100644 --- a/docs/work-items/ET-006/12-review.md +++ b/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` передаёт каждый элемент массива отдельным -аргументом. Для трека с сотнями тысяч точек, у которых есть ``, -массив `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 -``` - -Файл с несколькими `` (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` склеивает все `` одного `` в один плоский `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/Анализа, приёмку не задерживают.