reviewer(ET): auto-commit from reviewer run_id=90
All checks were successful
All checks were successful
This commit is contained in:
221
docs/work-items/ET-014/12-review.md
Normal file
221
docs/work-items/ET-014/12-review.md
Normal file
@@ -0,0 +1,221 @@
|
||||
---
|
||||
type: review
|
||||
work_item_id: ET-014
|
||||
verdict: APPROVED
|
||||
version: 1
|
||||
---
|
||||
|
||||
# Review ET-014 — Z-index конфликт terrain-popup vs sheet-gps-filters
|
||||
|
||||
**Branch:** `feature/ET-014-ui-z-index`
|
||||
**Commit:** `39348f6 fix(ui): terrain-popup закрывается при открытии bottom-sheet (ET-014)`
|
||||
**Reviewer:** agent:reviewer
|
||||
**Date:** 2026-06-04
|
||||
|
||||
## TL;DR
|
||||
|
||||
Реализация **полностью соответствует** ADR-019 (Вариант A): новый
|
||||
helper `closeTerrainPopup()` + один вызов первой строкой в `openSheet()`
|
||||
после null-check. CSS / HTML / backend не затронуты. 8 JS unit-тестов
|
||||
+ 9 Python (статика + node `--test` wrapper) — **все зелёные**.
|
||||
Z-stack `marker-dialog` (500), `search-panel` (600), `ruler-info` (600),
|
||||
`.bottom-sheet` (400), `#sheet-backdrop` (390), `.terrain-popup` (500)
|
||||
без изменений — статический тест это гарантирует.
|
||||
|
||||
P0/P1 не выявлено. Два P2/P3 нита (см. ниже) не блокируют приёмку.
|
||||
|
||||
## Проверенные оси
|
||||
|
||||
| Ось | Статус | Комментарий |
|
||||
|-----|--------|-------------|
|
||||
| Соответствие ТЗ (REQ-F-01..07, REQ-NF-01..05) | ✅ | Все требования закрыты, см. ниже |
|
||||
| Соответствие ADR-019 | ✅ | Реализация байт-в-байт совпадает с §Решение |
|
||||
| Качество кода | ✅ | Стиль файла, комменты, маркеры блока, ссылки на ADR |
|
||||
| Качество тестов | ✅ | 8 поведенческих + 5 статических + 1 wrapper |
|
||||
|
||||
### Соответствие ТЗ (02-trz.md → src/web/app.js)
|
||||
|
||||
| Требование | Покрыто | Где |
|
||||
|------------|---------|-----|
|
||||
| REQ-F-01 (sheet не перекрыт popup'ом) | ✅ | `closeTerrainPopup()` в `openSheet()`; AC-01/02 ⇒ TC-E-01/02 |
|
||||
| REQ-F-02 (`.active` снимается с `#terrain-toggle`) | ✅ | `btn.classList.remove('active')` в helper; covered by TC-U-02 |
|
||||
| REQ-F-03 (закрытие фильтров → возврат к карте) | ✅ | `closeSheet`/`closeAllSheets` не тронуты, ведут себя как раньше |
|
||||
| REQ-F-04 (повторное открытие стабильно) | ✅ | unit test `REQ-F-04` |
|
||||
| REQ-F-05 (terrain-popup для прочих сценариев — без регрессии) | ✅ | `toggleTerrainPopup`/`closeTerrainOnOutside` не изменены (app.js:2787, 2815) |
|
||||
| REQ-F-06 (другие sheets — без регрессии) | ✅ | unit test `REQ-F-06`: для них `closeTerrainPopup` — no-op |
|
||||
| REQ-F-07 (свет/тёмная тема) | ✅ | Логика чисто JS, тема-агностична |
|
||||
| REQ-NF-01 (backend не трогаем) | ✅ | diff пуст в `src/api/` |
|
||||
| REQ-NF-02 (нет тяжёлых обработчиков) | ✅ | helper O(1), вызывается 1 раз на `openSheet` |
|
||||
| REQ-NF-03 (marker-dialog/search-panel/ruler-info без регрессии) | ✅ | статический тест `test_z_index_stack_unchanged_for_affected_widgets` |
|
||||
| REQ-NF-04 (PWA) | ✅ | n/a, JS-логика не зависит от display-mode |
|
||||
| REQ-NF-05 (mobile + desktop) | ✅ | n/a, viewport-агностично |
|
||||
|
||||
### Соответствие ADR-019
|
||||
|
||||
ADR §Решение/1 — функция:
|
||||
```js
|
||||
function closeTerrainPopup() {
|
||||
const popup = document.getElementById('terrain-popup');
|
||||
const btn = document.getElementById('terrain-toggle');
|
||||
if (!popup || popup.style.display === 'none') return;
|
||||
popup.style.display = 'none';
|
||||
if (btn) btn.classList.remove('active');
|
||||
document.removeEventListener('click', closeTerrainOnOutside);
|
||||
}
|
||||
```
|
||||
**Реализация — байт-в-байт совпадает** (`src/web/app.js:211-218`).
|
||||
|
||||
ADR §Решение/2 — вызов первой строкой после null-check:
|
||||
```js
|
||||
function openSheet(id) {
|
||||
const sheet = document.getElementById(id);
|
||||
if (!sheet) return;
|
||||
closeTerrainPopup(); // ← вставлено
|
||||
document.querySelectorAll('.bottom-sheet.open').forEach(...);
|
||||
...
|
||||
}
|
||||
```
|
||||
**Реализация — точно** (`src/web/app.js:220-232`). Порядок проверен статическим
|
||||
тестом `test_open_sheet_calls_close_terrain_popup_first` (null-check →
|
||||
closeTerrainPopup → closeSheet → classList.add).
|
||||
|
||||
ADR §Решение/3 (`closeTerrainOnOutside` не меняется) — подтверждено, `app.js:2815`
|
||||
без изменений. ADR §Решение/4 (`togglePublicTracksFiltersSheet` не меняется) —
|
||||
подтверждено статическим тестом `test_gps_tracks_js_not_touched_by_et014`.
|
||||
|
||||
### Качество кода
|
||||
|
||||
Положительное:
|
||||
- Блок обрамлён маркерами `// >>> ET-014 sheet-popup yield block` / `<<<` —
|
||||
делает блок переиспользуемым для JS unit-тестов через `Function()` факторинг
|
||||
(тот же приём, что в ET-007 `base_layer.test.js`, прецедент закреплён).
|
||||
- Комментарий в `openSheet()` ссылается на ADR-019 — следующий читатель
|
||||
кода не будет гадать, зачем эта строка.
|
||||
- Helper не имеет побочных эффектов сверх документированных в ADR.
|
||||
- Стиль (отступы, кавычки, naming) повторяет окружающий код.
|
||||
|
||||
Замечания: см. P2/P3 ниже.
|
||||
|
||||
### Качество тестов
|
||||
|
||||
`tests/unit/sheet_popup.test.js` (8 node `--test` кейсов):
|
||||
1. TC-U-02 — popup закрывается, `.active` снимается ✓
|
||||
2. REQ-F-04 — повторное открытие стабильно ✓
|
||||
3. REQ-F-06 — для других sheets helper срабатывает (no-op) ✓
|
||||
4. closeTerrainPopup — no-op если popup уже скрыт ✓
|
||||
5. closeTerrainPopup — отписывает `closeTerrainOnOutside` ✓
|
||||
6. closeTerrainPopup — безопасен при отсутствии `#terrain-popup` ✓
|
||||
7. openSheet — ранний выход если sheet не найден ✓
|
||||
8. openSheet — закрывает другие sheets через `closeSheet` ✓
|
||||
|
||||
`tests/unit/test_sheet_popup.py` (9 pytest-кейсов):
|
||||
- 5 статических (маркеры, helper-в-блоке, порядок вызовов в openSheet,
|
||||
z-stack неизменён, gps_tracks.js не тронут)
|
||||
- 1 wrapper (запускает node-тесты)
|
||||
- 2 на `index.html` / порядок-once
|
||||
|
||||
**Все 17 тестов проходят локально**:
|
||||
```
|
||||
node --test: pass 8, fail 0 (73 ms)
|
||||
pytest: 9 passed (0.11 s)
|
||||
```
|
||||
|
||||
E2E (TC-E-01..06, TC-UI-01..08) — Playwright-инфра в репо отсутствует;
|
||||
Python-файл явно документирует skip и поведенчески покрывает суть через
|
||||
JS unit-тесты. Это валидное решение для текущего CI (matched ADR-017 / ET-013
|
||||
precedent).
|
||||
|
||||
## Findings
|
||||
|
||||
### P0 (blocker)
|
||||
|
||||
Нет.
|
||||
|
||||
### P1 (must-fix)
|
||||
|
||||
Нет.
|
||||
|
||||
### P2 (should-fix)
|
||||
|
||||
**F-1 [P2] — Отсутствует запись в CHANGELOG.md под `[Unreleased]`.**
|
||||
|
||||
В проекте есть устойчивая конвенция: ET-008/009/010/012/013 — все имеют
|
||||
`Added`/`Changed`/`Fixed` записи в CHANGELOG под `[Unreleased]` с
|
||||
`Refs: ET-XXX`. У ET-014 — нет. Хотя CLAUDE.md не делает это явным
|
||||
требованием, проектная конвенция говорит «обновлять». Deployer / следующий
|
||||
агент, формирующий тег, не увидит изменение и не сможет включить его в
|
||||
release-note.
|
||||
|
||||
Рекомендация: добавить под `### Fixed` (новая категория, корректная для
|
||||
bug-fix) что-то вроде:
|
||||
|
||||
```
|
||||
### Fixed
|
||||
- ET-014: Панель «Фильтры публичных треков» (#sheet-gps-filters)
|
||||
больше не открывается под панелью слоёв (#terrain-popup).
|
||||
При открытии любого .bottom-sheet через openSheet() popup
|
||||
принудительно закрывается (helper closeTerrainPopup в src/web/app.js).
|
||||
Z-index стек (.bottom-sheet=400, .terrain-popup=500, #marker-dialog=500,
|
||||
#search-panel=600, #ruler-info=600) не изменён — нулевой риск регрессии
|
||||
стека. ADR-019. Refs: ET-014.
|
||||
```
|
||||
|
||||
Severity P2 (не блокирует merge, но желательно поправить до деплоя).
|
||||
|
||||
### P3 (nice-to-have)
|
||||
|
||||
**F-2 [P3] — TD-1 из ADR-019 не закрыт (опционально).**
|
||||
|
||||
ADR-019 §Технический долг/TD-1 предлагает DRY-рефакторинг
|
||||
`closeTerrainOnOutside` на вызов нового `closeTerrainPopup()`:
|
||||
|
||||
```js
|
||||
// Сейчас (src/web/app.js:2815):
|
||||
function closeTerrainOnOutside(e) {
|
||||
const popup = document.getElementById('terrain-popup');
|
||||
const btn = document.getElementById('terrain-toggle');
|
||||
if (!popup.contains(e.target) && e.target !== btn && !btn.contains(e.target)) {
|
||||
popup.style.display = 'none';
|
||||
btn.classList.remove('active');
|
||||
document.removeEventListener('click', closeTerrainOnOutside);
|
||||
}
|
||||
}
|
||||
|
||||
// Можно (тело сжимается до 5 строк):
|
||||
function closeTerrainOnOutside(e) {
|
||||
const popup = document.getElementById('terrain-popup');
|
||||
const btn = document.getElementById('terrain-toggle');
|
||||
if (!popup || !btn) return;
|
||||
if (!popup.contains(e.target) && e.target !== btn && !btn.contains(e.target)) {
|
||||
closeTerrainPopup();
|
||||
}
|
||||
}
|
||||
```
|
||||
|
||||
ADR явно помечает это как opt-in cleanup («обязательного требования нет»).
|
||||
Не блокирует ET-014. Можно сделать отдельным fix-up коммитом сейчас или в
|
||||
будущем work-item. **Не делать в рамках review-loop** — попадёт под общее
|
||||
правило «reviewer не правит код».
|
||||
|
||||
## Definition of Done (по 03-acceptance-criteria.md)
|
||||
|
||||
| DoD-item | Статус |
|
||||
|----------|--------|
|
||||
| AC-01..14 на test-среде | ⏳ Owner-verify (e2e не автоматизирован, см. выше) |
|
||||
| `make test` зелёный | ✅ Локально pytest + node --test зелёные; CI должен подтвердить |
|
||||
| `make lint` зелёный | ⏳ Не проверил локально (нет make), CI проверит |
|
||||
| Playwright UI tests | ⏳ Не запускаются автоматически (нет инфры в репо) — задокументировано |
|
||||
| Owner approve по скриншотам AC-01/02/14 | ⏳ Owner-step |
|
||||
|
||||
Технически Developer закрыл всё что должен. Остальное — этапы CI / Owner.
|
||||
|
||||
## Вердикт
|
||||
|
||||
**APPROVED.**
|
||||
|
||||
Нет P0/P1. Реализация точно соответствует ADR-019. Тесты покрывают
|
||||
поведение и стат-инварианты. Два P2/P3 нита (CHANGELOG, опциональный
|
||||
DRY-рефакторинг `closeTerrainOnOutside`) не блокируют merge.
|
||||
|
||||
Рекомендация перед деплоем: закрыть F-1 (CHANGELOG entry). F-2 — на
|
||||
усмотрение Owner / Developer (можно отложить).
|
||||
Reference in New Issue
Block a user