225 lines
18 KiB
Markdown
225 lines
18 KiB
Markdown
---
|
||
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 не блокирует.
|