enduro-trails/docs/work-items/ET-002/09-review.md

type, work_item_id, title, version, status, verdict, created_at, authors

type	work_item_id	title	version	status	verdict	created_at	authors
review	ET-002	Code Review: Чекбокс показа/скрытия POI в попапе рельефа	2	approved	APPROVED	2026-05-21	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.