diff --git a/docs/work-items/ET-002/09-review.md b/docs/work-items/ET-002/09-review.md new file mode 100644 index 0000000..9f4806b --- /dev/null +++ b/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`, отделён `
` | ✅ | `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.** diff --git a/docs/work-items/ET-002/12-review.md b/docs/work-items/ET-002/12-review.md new file mode 100644 index 0000000..dbe9064 --- /dev/null +++ b/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 Чекбокс в попапе после «Тропы», отделён `
` | ✅ | `index.html:56-60` — `
` + `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 не блокирует.