145 lines
9.6 KiB
Markdown
145 lines
9.6 KiB
Markdown
---
|
||
type: review
|
||
work_item_id: ET-002
|
||
title: "Code Review: Чекбокс показа/скрытия POI в попапе рельефа"
|
||
version: 2
|
||
status: approved
|
||
verdict: APPROVED
|
||
created_at: 2026-05-21
|
||
authors:
|
||
- "agent:reviewer"
|
||
---
|
||
|
||
# Code Review — ET-002
|
||
|
||
## Вердикт
|
||
|
||
**APPROVED** — найдены только замечания уровня P3 (nice-to-have).
|
||
Блокеров (P0) и must-fix (P1) нет.
|
||
|
||
## Объём ревью
|
||
|
||
Проверены изменения в `src/web/app.js` и `src/web/index.html` против:
|
||
|
||
- `docs/work-items/ET-002/02-trz.md` (ТЗ, v1, approved)
|
||
- `docs/work-items/ET-002/03-acceptance-criteria.md` (AC, v1, approved)
|
||
- `docs/work-items/ET-002/06-adr/adr-0001-poi-visibility-client-side.md`
|
||
- `docs/work-items/ET-002/04-test-plan.yaml`
|
||
- `CLAUDE.md`
|
||
|
||
Дополнительно учтён юнит-тест `tests/unit/poi_toggle.test.js`.
|
||
|
||
---
|
||
|
||
## 1. Соответствие ТЗ
|
||
|
||
| Требование | Статус | Где реализовано |
|
||
|------------|--------|-----------------|
|
||
| REQ-F-01 — чекбокс «POI» в `terrain-popup` после `trails-path-cb`, отделён `<hr>` | ✅ | `index.html:56-60` |
|
||
| REQ-F-02 — состояние по умолчанию: checked, POI видимы | ✅ | `index.html:58` (атрибут `checked`); `app.js:2848-2852` (`stored === null → poiOn = true`) |
|
||
| REQ-F-03 — снятие чекбокса → `visibility: 'none'` для `poi-circles`, `poi-labels` | ✅ | `app.js:2833-2837` → `applyPoiVisibility(false)` → `app.js:2815-2825` |
|
||
| REQ-F-04 — установка чекбокса → `visibility: 'visible'` | ✅ | те же функции, ветка `visible` |
|
||
| REQ-F-05 — сохранение в `localStorage['poi-visible']` (`'1'`/`'0'`) | ✅ | `app.js:2835` |
|
||
| REQ-F-06 — восстановление при загрузке (`'0'` скрыть / `'1'`\|null показать) | ✅ | `app.js:2847-2853` (`restorePoiState`) |
|
||
| REQ-F-07 — синхронизация `layerState.poi` | ✅ | `app.js:2816` (`layerState.poi = visible` в общем хелпере) |
|
||
| REQ-NF-01 — переключение < 50 мс, без перезагрузки тайлов | ✅ | используется `setLayoutProperty`, источник слоёв не трогается |
|
||
| REQ-NF-02 — совместимость с MapLibre GL JS | ✅ | штатные API MapLibre, без экзотики |
|
||
| REQ-NF-03 — мобильный touch target ≥ 44px | ⚠️ см. P3-2 | переиспользован класс `terrain-checkbox` (как предписано ТЗ §3) |
|
||
| REQ-NF-04 — отсутствие регрессий | ✅ | существующие чекбоксы и их обработчики не затронуты |
|
||
|
||
UI-спецификация ТЗ §3 (разметка, `id="poi-visible-cb"`, `onchange="onPoiCheckbox()"`,
|
||
класс `terrain-checkbox`) воспроизведена в `index.html` дословно.
|
||
|
||
**Вывод:** все функциональные требования выполнены.
|
||
|
||
## 2. Соответствие ADR (adr-0001)
|
||
|
||
| Пункт решения | Статус | Комментарий |
|
||
|---------------|--------|-------------|
|
||
| 1. Видимость через `map.setLayoutProperty(... 'visibility' ...)` | ✅ | `app.js:2822` |
|
||
| 2. Персистентность — `localStorage['poi-visible']`, `'1'`/`'0'` | ✅ | соответствует конвенции проекта (`trails-track`, `terrain-*`) |
|
||
| 3. Источник истины — `layerState.poi`; `onPoiCheckbox()` синхронно обновляет `layerState`, `localStorage`, `layout.visibility` | ✅ | `onPoiCheckbox` → `setItem` + `applyPoiVisibility` (правит `layerState` и оба слоя) |
|
||
| 4. Без дублирования: общий приватный хелпер, переиспользование `layerGroups.poi` | ✅ | выделен `applyPoiVisibility(visible)`, итерирует `layerGroups.poi`; используется и `onPoiCheckbox`, и `restorePoiState` — ровно как предлагает ADR |
|
||
| 5. Backend/БД/API/инфраструктура без изменений | ✅ | изменения чисто клиентские |
|
||
|
||
Выбран и реализован Вариант A. Решение полностью соответствует ADR.
|
||
Замечание по консистентности с `toggleLayer('poi')` — см. P3-1.
|
||
|
||
## 3. Качество кода
|
||
|
||
Сильные стороны:
|
||
|
||
- Изменения локализованы в явно размеченном блоке
|
||
`>>> ET-002 POI visibility block ... <<<` (`app.js:2800-2854`) — удобно
|
||
для ревью и для тест-харнеса.
|
||
- JSDoc на всех трёх функциях, есть ссылка на ADR в шапке блока.
|
||
- `applyPoiVisibility` имеет guard `if (!map) return` и проверку
|
||
`map.getLayer(id)` перед `setLayoutProperty` — устойчиво к раннему вызову
|
||
и к смене стиля.
|
||
- `restorePoiState` корректно интегрирован в существующую цепочку
|
||
восстановления: `style.load → onMapStyleLoad → rebuildMapOverlays`
|
||
(`app.js:131`) — POI восстанавливается и при первой загрузке, и при
|
||
переключении темы. Паттерн идентичен `restoreTrailsState`.
|
||
- `restorePoiState` не пишет в `localStorage` (восстановление не должно
|
||
иметь побочных эффектов) — поведение задокументировано в JSDoc и
|
||
покрыто тестом TP-03.
|
||
- Разделение ответственности: персистентность — только в `onPoiCheckbox`,
|
||
применение видимости — в общем хелпере.
|
||
|
||
Замечаний P0/P1/P2 нет.
|
||
|
||
## 4. Качество тестов
|
||
|
||
`tests/unit/poi_toggle.test.js` исполняет **реальный** код из `app.js`
|
||
(блок извлекается по маркерам ET-002 и оборачивается через `new Function`
|
||
с мок-зависимостями) — это покрывает риск рассинхрона тестов и продакшн-кода.
|
||
|
||
- TP-01 — снятие чекбокса: скрытие слоёв + `setItem('poi-visible','0')` + `layerState.poi=false` ✅
|
||
- TP-02 — установка чекбокса: показ слоёв + `setItem('1')` + `layerState.poi=true` ✅
|
||
- TP-03 — `restorePoiState()` при `'0'`: скрытие + чекбокс снят + без записи в `localStorage` ✅
|
||
- TP-04 — `restorePoiState()` без ключа: дефолт «видимы» ✅
|
||
- Доп.: значение `'1'`, изоляция чужих слоёв (дух TP-08), синхронизация
|
||
`layerState` без слоёв на карте.
|
||
|
||
TP-05..TP-07, TP-09 (e2e) и TP-08 (integration) по своей природе не
|
||
покрываются юнит-тестом — это ожидаемо и не является замечанием к данному PR.
|
||
|
||
## Findings
|
||
|
||
### P3-1 (nice-to-have) — рассинхрон `toggleLayer('poi')` с чекбоксом
|
||
|
||
`toggleLayer(group)` (`app.js:386-396`) меняет `layerState.poi`, но не
|
||
обновляет ни чекбокс `poi-visible-cb`, ни `localStorage`. ADR-0001 п.4
|
||
указывает, что состояние кнопки и чекбокса не должно расходиться.
|
||
|
||
Смягчающие факты: `toggleLayer` не имеет ни одного вызова в кодовой базе,
|
||
элемента `btn-poi` в `index.html` нет (вызов `toggleLayer('poi')` упал бы
|
||
на `btn.classList`). Фактически это недостижимый код, расхождение
|
||
пользователю не наблюдаемо. ADR сам относит унификацию тулбар-кнопок и
|
||
popup-чекбоксов к будущему техдолгу (раздел «Технический долг»).
|
||
|
||
Рекомендация (вне scope ET-002): удалить мёртвый `toggleLayer` либо
|
||
оформить unified-контроллер слоёв отдельной задачей.
|
||
|
||
### P3-2 (nice-to-have) — REQ-NF-03 не проверяется по диффу
|
||
|
||
Touch target ≥ 44px зависит от CSS-класса `terrain-checkbox` в `app.css`,
|
||
который в рамках ET-002 не менялся (ТЗ §3 предписывает переиспользовать
|
||
существующий класс). Замечания к коду нет — отметка для приёмки:
|
||
подтвердить REQ-NF-03 прогоном e2e-теста TP-09.
|
||
|
||
### P3-3 (nice-to-have) — неровный отступ в IIFE `initTerrain`
|
||
|
||
В `app.js:2949-2950` и `2962-2963` вызовы `restoreTrailsState()` /
|
||
`restorePoiState()` имеют отступ, не совпадающий с окружающим блоком.
|
||
Это предсуществующая стилевая мелочь, которую ET-002 лишь продолжил.
|
||
Косметика; рекомендуется выровнять при ближайшем касании файла.
|
||
|
||
## Итог
|
||
|
||
Реализация полностью соответствует ТЗ и ADR-0001, код аккуратен, юнит-тесты
|
||
исполняют реальный код и покрывают TP-01..TP-04. Найдены только три
|
||
замечания уровня P3, ни одно из которых не блокирует мерж.
|
||
|
||
**Вердикт: APPROVED.**
|