diff --git a/docs/work-items/ET-005/12-review.md b/docs/work-items/ET-005/12-review.md new file mode 100644 index 0000000..5356906 --- /dev/null +++ b/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, отделён `
` | +| ФТ-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` с разделителем `
`; `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 не блокирует.