docs(ET-002): code review APPROVED - no P0/P1 findings

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

@@ -0,0 +1,144 @@
+---
+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.**

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

@@ -0,0 +1,168 @@
+---
+type: review
+work_item_id: ET-002
+title: "Code Review: Чекбокс показа/скрытия POI в попапе рельефа"
+version: 1
+status: approved
+verdict: APPROVED
+created_at: 2026-05-21
+authors:
+  - "agent:reviewer"
+---
+
+# Code Review — ET-002
+
+## Вердикт
+
+**APPROVED** (с комментариями).
+
+P0/P1-findings нет. Зафиксировано 1×P2 и 3×P3 — все некритичные, не
+блокируют merge. P2 относится к рассогласованию апстрим-артефактов
+(test-plan vs infra-requirements), а не к дефекту реализации.
+
+## Объект ревью
+
+- Ветка: `feature/ET-002-poi-toggle`
+- Базовая точка: `main` (832099c3) — ветка на 4 коммита впереди
+- Код-коммит: `8c17a4f` `feat(web): add POI visibility checkbox to terrain popup`
+- Изменённые файлы кода:
+  - `src/web/index.html` (+5 строк)
+  - `src/web/app.js` (+58 строк)
+  - `tests/unit/poi_toggle.test.js` (новый, 167 строк)
+  - `tests/unit/test_poi_toggle.py` (новый, 162 строки)
+- Прочитано: `02-trz.md`, `03-acceptance-criteria.md`, `04-test-plan.yaml`,
+  `06-adr/adr-0001-poi-visibility-client-side.md`, `07-infra-requirements.md`,
+  `CLAUDE.md`.
+
+## 1. Соответствие ТЗ
+
+| Требование | Статус | Комментарий |
+|------------|--------|-------------|
+| REQ-F-01 Чекбокс в попапе после «Тропы», отделён `<hr>` | ✅ | `index.html:56-60` — `<hr>` + `label.terrain-checkbox`, идёт сразу после `trails-path-cb` |
+| REQ-F-02 Состояние по умолчанию — checked | ✅ | Атрибут `checked` в HTML; `restorePoiState()` при отсутствии ключа даёт `poiOn = true` |
+| REQ-F-03 Скрытие → `visibility: 'none'` обоих слоёв | ✅ | `applyPoiVisibility(false)` → `setLayoutProperty` для `layerGroups.poi` |
+| REQ-F-04 Показ → `visibility: 'visible'` | ✅ | `applyPoiVisibility(true)` |
+| REQ-F-05 Сохранение в `localStorage` `poi-visible` `'1'`/`'0'` | ✅ | `onPoiCheckbox()` → `localStorage.setItem('poi-visible', …)` |
+| REQ-F-06 Восстановление при загрузке | ⚠️ | Реализовано; нештатные значения ключа трактуются как «скрыть» — см. R-02 (P3) |
+| REQ-F-07 Синхронизация `layerState.poi` | ✅ | `applyPoiVisibility()` пишет `layerState.poi` первой строкой |
+| REQ-NF-01 Производительность < 50 мс | ✅ | Только `setLayoutProperty`, тайлы не перезапрашиваются |
+| REQ-NF-02 Совместимость браузеров | ✅ | Стандартные API (`localStorage`, `setLayoutProperty`) |
+| REQ-NF-03 Мобильная доступность ≥ 44px | ✅ | Переиспользован общий класс `terrain-checkbox` (как у соседних чекбоксов) |
+| REQ-NF-04 Отсутствие регрессий | ✅ | Изменения аддитивные; точки восстановления зеркалят `restoreTrailsState()` |
+
+Все функциональные и нефункциональные требования ТЗ выполнены.
+
+## 2. Соответствие ADR-0001
+
+| Пункт решения ADR | Статус | Комментарий |
+|-------------------|--------|-------------|
+| п.1 Видимость через `setLayoutProperty` | ✅ | Подтверждено; `removeLayer`/`addLayer` не используются (Вариант C отвергнут корректно) |
+| п.2 Персистентность `localStorage` `poi-visible` `'1'`/`'0'` | ✅ | Соответствует конвенции проекта |
+| п.3 Источник истины — `layerState.poi`; `onPoiCheckbox()` синхронно обновляет state + storage + visibility | ✅ | Выполнено |
+| п.4 Без дублирования; переиспользование `layerGroups.poi`; общий хелпер | ✅ | Создан `applyPoiVisibility(visible)` — буквально имя, предложенное в ADR; используется и `onPoiCheckbox()`, и `restorePoiState()` |
+| п.4 Консистентность `toggleLayer('poi')` ↔ `onPoiCheckbox()` через `layerState.poi` | ✅ | Обе функции читают/пишут `layerState.poi`. POI-кнопки в тулбаре нет (`btn-poi` отсутствует, `toggleLayer` нигде не вызывается) — визуальный рассинхрон невозможен |
+| п.5 Backend/БД/API/инфраструктура без изменений | ✅ | Затронут только `src/web/` |
+
+Реализация полностью соответствует принятому ADR. Отдельно отмечается
+точное следование п.4 — выделен ровно тот приватный хелпер, который ADR
+описывал как рекомендуемый.
+
+## 3. Acceptance Criteria
+
+| AC | Покрытие |
+|----|----------|
+| AC-01 Чекбокс в попапе после «Тропы», отделён линией | ✅ `test_poi_checkbox_placed_after_trails_separated_by_hr` |
+| AC-02 POI включены по умолчанию | ✅ `test_poi_checkbox_checked_by_default`, JS `TP-04` |
+| AC-03 Скрытие POI | ✅ JS `TP-01` |
+| AC-04 Показ POI | ✅ JS `TP-02` |
+| AC-05 Состояние сохраняется после перезагрузки | ✅ JS `TP-03` (restore при `poi-visible=0`) |
+| AC-06 Восстановление включённого состояния | ✅ JS `restorePoiState() при poi-visible=1` |
+| AC-07 Не ломает существующие чекбоксы | ✅ JS `onPoiCheckbox() меняет только poi-circles/poi-labels`; изменения HTML аддитивны |
+| AC-08 Синхронизация с `layerState` | ✅ JS `TP-01`/`TP-02` (`layerState.poi`), `applyPoiVisibility()` без слоёв |
+
+Все 8 критериев имеют поведенческое покрытие.
+
+## 4. Качество кода
+
+Сильные стороны:
+
+- **Нет дублирования.** Логика видимости группы вынесена в единый
+  `applyPoiVisibility()`; карта слоёв `layerGroups.poi` не продублирована.
+- **Консистентность с кодовой базой.** `restorePoiState()` подключён в
+  тех же трёх точках, что и `restoreTrailsState()` (`rebuildMapOverlays`
+  + обе ветки `initTerrain`), используется `window._map`, паттерн
+  `localStorage` `'1'`/`'0'` — всё единообразно с существующим кодом.
+- **Защитное программирование.** Проверки `if (!map)`, `if (map.getLayer(id))`,
+  `if (cb)`; `layerState.poi` обновляется даже когда карта/слои ещё не
+  готовы (порядок инициализации безопасен).
+- **Документированность.** JSDoc на всех трёх функциях, блок-маркеры с
+  ссылкой на ADR, запись в `CHANGELOG.md`.
+- **Коммиты.** Conventional Commits соблюдён (`feat(web):`, `docs(ET-002):`).
+
+Замечания — см. findings R-02, R-03 (P3).
+
+## 5. Качество тестов
+
+- **Unit (`poi_toggle.test.js`)** — высокое качество: тесты исполняют
+  **реальный** код из `app.js` (блок извлекается по маркерам и
+  оборачивается через `new Function` с инъекцией моков), а не его копию.
+  Покрыты TP-01..TP-04 + 3 дополнительных кейса (значение `'1'`,
+  изоляция чужих слоёв, синхронизация state без слоёв на карте).
+- **Python (`test_poi_toggle.py`)** — статические проверки структуры
+  HTML/JS (REQ-F-01, REQ-F-02, ADR-0001) + запуск JS-раннера через
+  `node --test`, со `skip` при отсутствии `node` (по аналогии с
+  существующим `test_lua_syntax`/`luac`).
+- Браузерные e2e TP-05..TP-09 не реализованы — см. R-01 (P2).
+
+## Findings
+
+### R-01 — Отклонение от test-plan: e2e TP-05..TP-09 не реализованы (P2)
+
+`04-test-plan.yaml` определяет TP-05..TP-09 как `type: e2e`, однако
+`07-infra-requirements.md` §6 явно запрещает новые npm-пакеты, а
+Playwright-инфраструктуры в репозитории нет. Это конфликт между двумя
+**approved**-артефактами, а не дефект разработчика: реализовать e2e «как
+написано» означало бы нарушить инфра-требования. Поведение всех
+сценариев покрыто статическими + unit-тестами, отклонение подробно
+задокументировано в шапке `test_poi_toggle.py`.
+**Рекомендация:** Analyst — согласовать `04-test-plan.yaml` с
+`07-infra-requirements.md` (пометить TP-05..09 как покрытые альтернативно
+либо завести отдельную инфра-задачу на Playwright). На merge ET-002 не
+влияет.
+
+### R-02 — `restorePoiState()`: нештатное значение ключа скрывает POI (P3)
+
+`const poiOn = stored === null || stored === '1';` — любое значение,
+отличное от `'1'`/`null` (например мусор в `localStorage`), трактуется
+как «скрыть». REQ-F-06 описывает только `'0'` и `'1'`/`null`, а дефолт
+фичи (REQ-F-02) — POI включены. Надёжнее: `const poiOn = stored !== '0';`
+— тогда повреждённое значение деградирует к дефолту «показать».
+Крайний кейс (ключ пишет только это приложение), отсюда P3.
+
+### R-03 — Непоследовательные отступы в `initTerrain` (P3)
+
+`app.js:2948-2950` и `2961-2963`: `restoreTrailsState()`/`restorePoiState()`
+смещены относительно `restoreTerrainState()`. Дефект унаследован из
+существующего кода — новые строки повторяют локальный (сломанный) отступ.
+Косметика; при желании выровнять блок целиком.
+
+### R-04 — Хрупкая связанность тестов с исходником (P3)
+
+Тесты зависят от комментариев-маркеров `>>> ET-002 POI visibility block`
+в `app.js` и от точного совпадения пробелов в
+`test_poi_logic_reuses_layer_state_and_groups`
+(`"poi:     ['poi-circles', 'poi-labels']"`). Для монолитного non-module
+`app.js` это прагматичное и задокументированное решение, но хрупкое при
+рефакторинге/прогоне линтера. Технический долг — учесть при будущей
+модуляризации фронтенда.
+
+## Заключение
+
+Реализация ET-002 корректна, полна и точно следует ТЗ и ADR-0001.
+Дублирования нет, регрессионные риски закрыты повторением проверенного
+паттерна `restoreTrailsState()`. Тесты исполняют реальный код и
+покрывают все acceptance-критерии. Блокирующих замечаний нет.
+
+**Вердикт: APPROVED.** R-02/R-03/R-04 — на усмотрение разработчика
+(можно поправить в этом же PR или вынести в техдолг). R-01 — действие на
+стороне Analyst, merge не блокирует.