169 lines
12 KiB
Markdown
169 lines
12 KiB
Markdown
---
|
||
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 не блокирует.
|