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

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

@@ -0,0 +1,224 @@
+---
+type: review
+work_item_id: ET-005
+title: "Code Review: Переключение единиц измерения расстояний (км/мили)"
+version: 1
+status: approved
+verdict: APPROVED
+created_at: 2026-05-21
+authors:
+  - "agent:reviewer"
+---
+
+# Code Review — ET-005
+
+## Вердикт
+
+**APPROVED** (с комментариями).
+
+P0/P1-findings нет. Зафиксировано 1×P2 и 2×P3 — все некритичные, не
+блокируют merge. P2 — побочный эффект оркестратора (сброс выбора варианта
+связки при переключении единиц), не нарушает требований ТЗ.
+
+## Объект ревью
+
+- Ветка: `feature/ET-005-`
+- Код-коммит: `2fe5cfe` `feat(web): переключатель единиц измерения расстояний (км/мили)`
+- Изменённые файлы кода:
+  - `src/web/units.js` (новый, 190 строк)
+  - `src/web/index.html` (+11 строк)
+  - `src/web/app.js` (+118 / −24 строк)
+  - `src/web/app.css` (+20 строк)
+  - `tests/unit/units.test.js` (новый, 219 строк)
+  - `tests/unit/test_unit_toggle.py` (новый, 211 строк)
+  - `CHANGELOG.md` (+5 строк)
+- Прочитано: `02-trz.md`, `03-acceptance-criteria.md`, `04-test-plan.yaml`,
+  `06-adr/adr-0001-unit-toggle-client-side.md`, `10-tech-risks.md`,
+  `CLAUDE.md`.
+
+## 1. Соответствие ТЗ
+
+### Функциональные требования
+
+| Требование | Статус | Комментарий |
+|------------|--------|-------------|
+| ФТ-1 Кнопка-toggle в панели настроек карты | ✅ | `index.html:62-69` — сегментированный `.seg-control` в попапе `#terrain-popup`, после чекбокса POI, отделён `<hr>` |
+| ФТ-2 Два состояния: km (default) / mi | ✅ | `units.js` `DEFAULT_UNIT = 'km'`, `VALID_UNITS = ['km','mi']` |
+| ФТ-3 Пересчёт всех видимых расстояний | ✅ | Единый оркестратор `onUnitChange()` — карточки маршрутов, мини-карточка, лист точек, линейка, scale-bar, связка, «красивый» маршрут. См. R-01 (P2) — побочный сброс выбора варианта связки |
+| ФТ-4 Сохранение выбора в `localStorage` (`distance_unit`) | ✅ | `units.js` `setUnit()` → `writeStored()`, ключ `distance_unit` |
+| ФТ-5 При загрузке — чтение из `localStorage` | ✅ | `getUnit()` лениво читает ключ; `syncUnitToggleUI()` на `DOMContentLoaded` восстанавливает вид кнопок |
+
+### Нефункциональные требования
+
+| Требование | Статус | Комментарий |
+|------------|--------|-------------|
+| Пересчёт < 100 мс | ✅ | Чисто клиентская конвертация, нет ни сети, ни перезапроса тайлов |
+| Кнопка доступна на всех размерах экрана | ✅ | Переиспользован адаптивный `.seg-control`; статически подтверждено `test_unit_toggle_reuses_seg_control_component`. Браузерный e2e на 375px (TP-05) — на этап тестирования |
+| Не блокирует другие UI элементы | ✅ | Аддитивный ряд `.terrain-unit-row` с разделителем `<hr>`; `margin-bottom:0` для последнего элемента попапа |
+
+### Технический дизайн ТЗ
+
+Два пункта раздела «Технический дизайн» ТЗ скорректированы в ADR-0001 —
+оба отклонения **обоснованы и легитимны**, дефектом не являются:
+
+- путь `src/web/static/js/units.js` → `src/web/units.js` (ADR-0001 п.2:
+  каталога `static/js/` нет, сборщика нет, `app.js` — классический
+  скрипт; Вариант C отклонён);
+- «все компоненты слушают `unitchange`» → единый оркестратор
+  `onUnitChange()` (ADR-0001 п.6; Вариант B отклонён ради низкой
+  связности).
+
+Согласно CLAUDE.md ADR обладает архитектурным мандатом; раздел
+«Технический дизайн» ТЗ носит рекомендательный характер. Реализация
+следует ADR — это корректно.
+
+Все функциональные и нефункциональные требования ТЗ выполнены.
+
+## 2. Соответствие ADR-0001
+
+| Пункт решения ADR | Статус | Комментарий |
+|-------------------|--------|-------------|
+| п.1 Только клиент; backend/БД/инфра без изменений | ✅ | Затронут только `src/web/` + тесты |
+| п.2 Модуль `src/web/units.js`, подключён перед `app.js` | ✅ | `index.html:415` `units.js` строго до `app.js`; статически подтверждено `test_units_js_loaded_before_app_js` (R7) |
+| п.3 Неймспейс `window.Units` (`getUnit`/`setUnit`/`formatDistance`/`KM_TO_MI`) | ✅ | Публичный контракт собран; дополнительно `module.exports` для Node-тестов |
+| п.4 Каноническая единица — метры; конвертация только при форматировании | ✅ | `formatDistance(meters,...)` принимает метры; внутреннее состояние (`route.distance_m`, `rulerTotal`) не трогается |
+| п.5 `localStorage` ключ `distance_unit`, дефолт `km` при отсутствии/мусоре | ✅ | `getUnit()` через `isValidUnit()` деградирует к `DEFAULT_UNIT` |
+| п.6 Единый оркестратор — ровно одна подписка на `unitchange` | ✅ | `document.addEventListener('unitchange', onUnitChange)` единственная; подтверждено `test_app_js_single_unitchange_subscription` |
+| п.7 Все 13 мест форматирования → `Units.formatDistance()`; GPX остаётся метрическим | ✅ | grep-аудит: вне `generateGPX()` захардкоженного `(m/1000).toFixed()+' км'` не осталось; GPX-экспорт намеренно метрический (R6), подтверждено `test_app_js_gpx_export_stays_metric` |
+| п.8 UI на готовом `.seg-control` в `#terrain-popup` | ✅ | Новый CSS-компонент не вводится; добавлены только обёрточные стили `.terrain-unit-row` |
+| п.9 C4 без изменений | ✅ | Состав компонентов не меняется |
+
+Реализация полностью соответствует принятому ADR-0001. Риски R1–R8 из
+`10-tech-risks.md` отработаны: R1 (call-sites) — централизация + аудит;
+R2 (суб-км значения) — явная политика в `formatDistance()`; R3
+(scale-bar) — отдельная unit-aware ветка + включение в оркестратор; R4
+(разделитель) — единая запятая в `units.js` (по scale-bar см. R-02); R5
+(точность) — параметр `precision`; R6 — GPX исключён; R7 — порядок
+скриптов; R8 — переиспользование `.seg-control`.
+
+## 3. Acceptance Criteria
+
+| AC | Покрытие |
+|----|----------|
+| AC-1 Кнопка km/mi в панели, показывает выбор, клик переключает | ✅ `test_unit_toggle_present_in_html`; `syncUnitToggleUI()` переключает класс `.active` |
+| AC-2 Пересчёт всех расстояний, коэф. 0.621371, округление до 1 знака | ✅ JS `TP-02`, `AC-2: KM_TO_MI`, `точность по умолчанию — 1 знак`. Карточки маршрутов — `precision:0` (R5, осознанная трактовка), суб-км в милях — 2 знака (R2) |
+| AC-3 Сохранение и восстановление из `localStorage`, дефолт km | ✅ JS `TP-01`, `TP-03`, `недоступный localStorage` |
+| AC-4 Кнопка не перекрывает элементы, mobile, переключение < 100мс | ✅ (структурно) `test_unit_toggle_has_styles`; e2e mobile (TP-05) — этап тестирования |
+
+Поведенческое ядро (модуль `units.js`) покрыто полностью. Замечание по
+покрытию слоя `app.js` — см. R-03 (P3).
+
+## 4. Качество кода
+
+Сильные стороны:
+
+- **Единый источник истины.** `KM_TO_MI` объявлен ровно один раз;
+  форматирование расстояний централизовано в `units.js`; 13 call-sites
+  в `app.js` сведены к `Units.formatDistance()`. Дублирования нет.
+- **Защитное программирование.** `readStored()`/`writeStored()` обёрнуты
+  в `try/catch` (private mode); `dispatchEvent` защищён от headless-среды;
+  `formatDistance()` корректно отдаёт `'-'` для `null/undefined/NaN`;
+  `setUnit()` валидирует вход и не шлёт событие при неизменной единице.
+- **Каноническое состояние в метрах.** Конвертация — только в момент
+  форматирования; тест `TP-04: многократное переключение` подтверждает
+  отсутствие дрейфа округления.
+- **Консистентность с кодовой базой.** Глобальный неймспейс и
+  классический скрипт соответствуют стилю `app.js`; паттерн
+  `window._updateScaleZoom` зеркалит существующий `window._map`;
+  блок-маркеры `>>> ET-005 ... >>>` повторяют приём ET-002.
+- **Документированность.** JSDoc на всех публичных и приватных функциях
+  со ссылками на ADR/риски; запись в `CHANGELOG.md`; commit —
+  Conventional Commits (`feat(web):`).
+- **Оркестратор аккуратен.** `renderLinkCards()`/`drawScenicRoutes()`
+  идемпотентны (remove-before-add), повторный вызов из `onUnitChange()`
+  не вызывает «layer already exists»; перерисовка связки/«красивого»
+  маршрута огорожена флагами `linkMode`/`scenicMode`.
+
+Замечания — см. findings R-01 (P2), R-02/R-03 (P3).
+
+## 5. Качество тестов
+
+- **Unit (`units.test.js`)** — высокое качество: тесты исполняют
+  **реальный** `src/web/units.js` (сброс `require.cache` + инъекция
+  моков `window`/`document`/`localStorage` перед каждым тестом).
+  Покрыты TP-01..TP-04, AC-2, AC-3, граница 1000 м, недоступный
+  `localStorage`, валидация `setUnit()`, публикация неймспейса.
+- **Python (`test_unit_toggle.py`)** — статические проверки структуры
+  `units.js`/`index.html`/`app.js` (ADR-0001, риски R1/R3/R6/R7) +
+  запуск JS-раннера через `node --test` со `skip` при отсутствии `node`
+  (по аналогии с `test_poi_toggle.py`).
+- e2e TP-05 (mobile responsive) не реализован — Playwright-инфраструктуры
+  в репозитории нет; отклонение задокументировано в шапке
+  `test_unit_toggle.py`. На merge не влияет, относится к этапу
+  тестирования.
+
+## Findings
+
+### R-01 — Переключение единиц сбрасывает выбор варианта связки (P2)
+
+`onUnitChange()` (`app.js:2937-2938`) при активном режиме связки
+вызывает `renderLinkCards(linkRoutes)`. В отличие от
+`drawScenicRoutes(routes, activeIdx)`, функция `renderLinkCards()` **не
+принимает индекс активного варианта** и всегда отрисовывает «Вариант 1»
+как активный: карточке `i===0` присваивается класс `active`, линии
+`link-line-0` — `line-width:5`. В коде нет переменной `activeLinkIdx` —
+выбор существует только как класс в DOM и ширина линии на карте.
+
+Следствие: пользователь, выбравший вариант 2/3 связки через
+`selectLinkRoute()`, при каждом переключении км/мили теряет выбор —
+подсветка карточки и акцент линии откатываются на вариант 1. Требования
+ТЗ (ФТ-3 «пересчёт расстояний») формально выполнены — расстояния
+пересчитываются корректно; это побочный дефект UX, ограниченный режимом
+связки, поэтому P2, а не P1.
+
+**Рекомендация:** ввести `activeLinkIdx` (по аналогии с
+`activeScenicIdx`), обновлять его в `selectLinkRoute()` и после
+повторного `renderLinkCards()` в `onUnitChange()` вызывать
+`selectLinkRoute(activeLinkIdx)` для восстановления подсветки. Можно
+поправить в этом же PR либо вынести в техдолг.
+
+### R-02 — Scale-bar в режиме «mi» использует точку и латиницу (P3)
+
+Масштабная линейка (`updateScaleZoom`, `app.js:1455`) формирует подпись
+как `distance + ' ' + unit`, где `unit` — латинские `'mi'`/`'km'`/`'m'`,
+а суб-единичные значения выводятся стандартным `Number.toString()` с
+точкой (`'0.5 mi'`). Это расходится с `units.js`, который по R4 задаёт
+запятую и русские подписи (`'0,5 ми'`). Митигация R4 в `10-tech-risks.md`
+предписывает «единый разделитель для всех поверхностей».
+
+Замечание ограничено P3: латиница и точка в scale-bar — **поведение,
+существовавшее до ET-005** (в режиме «km» линейка и раньше показывала
+`'30 km'`/`'0.5 km'`); ET-005 лишь расширил тот же стиль на мили и
+консистентен внутри компонента. Полная унификация требует отдельного
+форматирования подписи scale-bar и выходит за scope ET-005.
+**Рекомендация:** при желании привести подпись scale-bar к запятой и
+русским единицам — отдельной косметической правкой/техдолгом.
+
+### R-03 — Слой `app.js` (оркестратор, scale-bar) покрыт только статически (P3)
+
+Поведенческими тестами покрыт изолированный модуль `units.js`. Логика
+`app.js` — `onUnitChange()` (перерисовка поверхностей), unit-aware ветка
+scale-bar — проверяется лишь строковыми ассертами наличия в
+`test_unit_toggle.py` (`test_app_js_*`). Поведение «после переключения
+каждая видимая поверхность пересчиталась» (TP-02) не верифицируется
+автоматически.
+
+Это приемлемо: в репозитории нет DOM/MapLibre-харнесса для `app.js`
+(монолитный non-module скрипт), и подход повторяет принятый в ET-002.
+Сквозную проверку TP-02 закрывает этап тестирования. Фиксируется как
+технический долг — учесть при будущей модуляризации фронтенда.
+
+## Заключение
+
+Реализация ET-005 корректна и полна: все функциональные и
+нефункциональные требования ТЗ выполнены, ADR-0001 (пп.1–9) соблюдён
+полностью, риски R1–R8 отработаны. Архитектурные отклонения от раздела
+«Технический дизайн» ТЗ легитимны и зафиксированы в ADR. Дублирования
+нет, форматирование централизовано, защитное программирование на месте,
+модуль `units.js` тщательно покрыт unit-тестами на реальном коде.
+
+Блокирующих замечаний нет.
+
+**Вердикт: APPROVED.** R-01 (P2) рекомендуется поправить в этом же PR
+либо осознанно вынести в техдолг; R-02/R-03 (P3) — на усмотрение
+разработчика. Ни одно замечание merge не блокирует.