---
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 не блокирует.