diff --git a/docs/work-items/ET-007/12-review.md b/docs/work-items/ET-007/12-review.md index 5bb469d..5de0195 100644 --- a/docs/work-items/ET-007/12-review.md +++ b/docs/work-items/ET-007/12-review.md @@ -1,416 +1,355 @@ --- type: code-review work_item_id: ET-007 -title: "Review: Спутниковая карта (Схема / Спутник) — артефакты анализа и архитектуры" -version: 1 -status: REQUEST_CHANGES +title: "Review v2: Спутниковая карта (Схема / Спутник) — артефакты + код" +version: 2 +status: APPROVED_WITH_COMMENTS created_at: 2026-05-31 updated_at: 2026-05-31 authors: - "agent:reviewer" branch: feature/ET-007-et-005 -stage_reviewed: analysis + architecture +stage_reviewed: analysis + architecture + development (code + tests) +previous_review: v1 (REQUEST_CHANGES, 6 P1 / 6 P2 / 3 P3) --- -# Code Review — ET-007: Спутниковая карта (Схема / Спутник) +# Code Review v2 — ET-007: Спутниковая карта (Схема / Спутник) ## Вердикт -**REQUEST_CHANGES** — найдено 6 P1-блокирующих findings и 6 P2/3 -nice-to-have. Кода в ветке ещё нет (только документация анализа и -архитектуры); цель ревью — не пропустить эти расхождения в -разработку, иначе разработчик будет вынужден угадывать намерения -или возвращаться за уточнениями. +**APPROVED with comments.** -Сами решения (Esri World Imagery; ленивый raster-source; восстановление -через `rebuildMapOverlays()`; гибридное halo) — обоснованы и -непротиворечивы внутри ADR-004. Проблема в нестыковках между ТЗ / -ADR / Data / Tech-Risks и в нескольких ссылках на сущности в коде -(`trails-grade1..5`, `paths-bridleway`, baseline-цвета), которых в -реальном `src/web/` нет. +Все 6 P1-блокеров из v1 закрыты в спецификации **и** в коде. Реализация +корректна, прошли все 22 pytest-проверки (`tests/unit/test_base_layer.py`) +и все 33 поведенческих JS-теста (`tests/unit/base_layer.test.js`), +запускаемых под `node --test`. Архитектурные решения (Esri, M-A, S-B, +O-A, H-B) соблюдены в коде один-в-один; никаких отклонений от ADR-004 +не обнаружено. + +Остаются 4 не-блокирующих замечания P2 (часть из них — несвёрнутые +концы v1 P2) и 4 nice-to-have P3, включая мелкие расхождения между +текстом TRZ и фактической реализацией (код в одной точке делает чуть +больше, чем требует ТЗ — добавляет защитный `beforeId`). Эти правки +рекомендуется внести следующим коммитом, но они не препятствуют +переходу в `testing` / merge. ## Что проверено -- `docs/work-items/ET-007/01-brd.md` (v1, status draft) -- `docs/work-items/ET-007/02-trz.md` (v1, status draft) -- `docs/work-items/ET-007/03-acceptance-criteria.md` (v1, status draft) -- `docs/work-items/ET-007/04-test-plan.yaml` (v1) -- `docs/work-items/ET-007/04b-ui-test-cases.md` (v1) — просмотрено -- `docs/work-items/ET-007/06-adr/ADR-004-satellite-base-layer.md` (accepted) -- `docs/work-items/ET-007/07-infra-requirements.md` (approved) -- `docs/work-items/ET-007/08-data-requirements.md` (approved) -- `docs/work-items/ET-007/10-tech-risks.md` (approved) -- `docs/architecture/README.md` (обновлён ET-007) -- `docs/architecture/adr/README.md` (добавлен ADR-004) -- Diff `origin/main...HEAD`: 12 файлов, 2020 вставок, 1 удаление — - только `docs/`. Кода в ветке нет; ревью на этапе разработки - понадобится повторно. -- Сверка с фактическим кодом: `src/web/app.js` (3132 строки), - `src/web/style.json` (136), `src/web/style-dark.json` (136). -- CLAUDE.md +### Артефакты (вторая итерация) +- `01-brd.md` v2 — P1-3 закрыт (риск №4: «авто-выключение hillshade + не вводится»). +- `02-trz.md` v2 — P1-1, P1-2, P1-4, P1-5, P1-6 закрыты; добавлены + §5.6 (контракт с `layerState.basemap`) и §5.7 (синхронизация halo). +- `03-acceptance-criteria.md` v2 — добавлены сценарии под P1-2/P1-5/P1-6. +- `06-adr/ADR-004-satellite-base-layer.md` (accepted) — добавлены + §8 и §9 под P1-5 и P1-6; §5 переписан под реальные halo-id; + §6 единая константа `#2a2a2a`; baseline dark исправлен `#1a1a2e`. +- `08-data-requirements.md` v2 — таблицы 5.1 / 5.2 (baseline POI per- + theme и satellite-значения), `_savedBasemapState`, исправлен dark + baseline на `#1a1a2e`. +- `10-tech-risks.md` v2 — R-1 переписан под реальные id; R-5 + обновлён с baseline `#1a1a2e`. +- `04-test-plan.yaml`, `04b-ui-test-cases.md`, `07-infra-requirements.md` + — без изменений (v1 не требовали правок по тем findings). + +### Код (новое в ветке) +- `src/web/index.html` (+11 строк) — блок `#base-seg` в `#terrain-popup`. +- `src/web/app.css` (+30 строк) — стили `.terrain-base-row`, + `.base-seg`, CSS-hook `body.satellite-active #btn-basemap`. +- `src/web/app.js` (+368 строк) — блок ET-007, хук в + `rebuildMapOverlays()`, синхронизация halo в `onTrailsCheckbox()` / + `restoreTrailsState()`, инициализация в обеих ветках IIFE. +- `src/web/style.json` (+30) / `src/web/style-dark.json` (+30) — два + halo-underlay-слоя `trails-track-halo-satellite` и + `trails-path-bridleway-halo-satellite`, оба с `visibility: none` и + размещены **перед** соответствующим базовым trails-слоем (z-order). + +### Тесты +- `tests/unit/test_base_layer.py` (+301 строк) — 22 статических теста + (HTML/CSS/JS-структура, halo-слои в обоих стилях, порядок halo + перед базовым слоем, `restoreBaseLayerState()` первым в + `rebuildMapOverlays()`, ≥4 вызова в init-путях) + сабпроцесс + `node --test` для JS-suite. +- `tests/unit/base_layer.test.js` (+468 строк) — 33 поведенческих + unit-теста через `new Function`-загрузку блока ET-007, с мок-DOM, + мок-localStorage и мок-картой; покрывают U-01..U-05, U-10..U-11, + I-01..I-04, I-07, halo, POI text-color/halo (P1-2), background P1-4 + обе темы, валидацию входа, отсутствие window._map, недоступный + localStorage, z-order. + +### Прогон +``` +$ python -m pytest tests/unit/test_base_layer.py -v +22 passed in 0.16s + +$ node --test tests/unit/base_layer.test.js +# tests 33, pass 33, fail 0 +``` +Полный `pytest tests/` падает только на `tests/unit/test_health.py` +из-за отсутствующего `shapely` в окружении — это инфраструктурная +проблема, не относится к ET-007. --- -## P1 — must-fix перед началом разработки +## P1 — must-fix -### P1-1 — TRZ / ADR / Data / Tech-Risks ссылаются на несуществующие слои +**Нет.** Все 6 P1-findings из v1 закрыты. -**Где:** -- TRZ §1 REQ-F-04 таблица — строки «Trails (grade1..5)» и «Paths/bridleway». -- ADR-004 §5 пункт 1 — «Для линий **grade1..5** и paths/bridleway… парные - underlay-слои `*-halo-satellite`». -- Data §6 таблица — `trails-grade1`, `trails-grade2`, `paths-bridleway`, - и т.д. -- Tech-Risks R-1 — «массив `['trails-grade1', 'trails-grade2', ...]` с - производным правилом `-halo-satellite`». +| v1 ID | Категория | Где закрыто | +|-------|-----------|-------------| +| P1-1 | Несуществующие слои grade1..5 | TRZ §1 REQ-F-04, ADR-004 §5, Data §6, Tech-Risks R-1 — реальные id `trails-track-halo-satellite`, `trails-path-bridleway-halo-satellite` | +| P1-2 | POI text-color на спутнике | TRZ §1 REQ-F-04-POI, ADR-004 §5, Data §5.2, код `_applyPoiSatellitePaint()`, JS-тесты «P1-2» | +| P1-3 | BRD vs TRZ hillshade | BRD §5 риск 4 переписан под TRZ/ADR/AC: авто-выключение не вводится | +| P1-4 | background-color 3 источника | ADR-004 §6, TRZ §1 REQ-F-03, Data §5 — единая `#2a2a2a` для обеих тем; baseline dark `#1a1a2e`; JS-тесты обе темы | +| P1-5 | Контракт с `layerState.basemap` | TRZ §5.6, AC-02/AC-03 новые сценарии, ADR-004 §8, код `_savedBasemapState` + `_setBodyClass('satellite-active', …)`, CSS `body.satellite-active #btn-basemap { display:none !important }` | +| P1-6 | halo не синхронизирован с чекбоксами | TRZ §5.7, AC-04 новые сценарии, ADR-004 §9, код `_applyTrailHaloVisibility(map, base)` + хуки в `onTrailsCheckbox()` и `restoreTrailsState()` | -**Что в коде:** в `src/web/style.json` (стр. 42–92) и `style-dark.json` -определены только три line-слоя поверх OSM: - -| id в style.json | filter | как обрабатываются grade | -|---|---|---| -| `trails-asphalt` | `highway in primary/secondary/tertiary/residential` | без grade | -| `trails-track` | `highway == track` | **одно** paint-выражение `match` по `tracktype` (`grade1..5`) — см. style.json:64–72 | -| `trails-path-bridleway` | `highway in path/bridleway/footway` | без grade | - -Слоёв `trails-grade1`, `trails-grade2`, …, `trails-grade5`, -`paths-bridleway` **нет** ни в одном из стилей. Поэтому: - -- Halo-стратегия H-B (пара underlay-слоёв на каждый base-слой) на - практике даёт 2–3 пары (`trails-track-halo-satellite`, - `trails-path-bridleway-halo-satellite`, опционально - `trails-asphalt-halo-satellite`), а **не** 5+ пар, как написано. -- Чтобы получить «разный halo по grade», halo-слой `trails-track-halo-satellite` - должен сам содержать `match` по `tracktype` (если halo должен - зависеть от grade) — либо halo единого цвета на весь `trails-track`. - ADR не оговаривает. -- Tech-Risks R-1 митигация «массив `['trails-grade1', …, 'trails-grade5']`» - введёт разработчика в заблуждение и приведёт либо к `addLayer` от - несуществующих base, либо к молчаливому `getLayer === undefined`. - -**Действие:** обновить TRZ §1 REQ-F-04, ADR-004 §5, Data §6, Tech-Risks -R-1, чтобы они оперировали реальными id (`trails-track`, -`trails-path-bridleway`, при необходимости `trails-asphalt`) и явно -зафиксировали, нужен ли halo, чувствительный к `tracktype`, или -единый halo на весь `trails-track`. - -### P1-2 — Контраст POI labels на спутнике спецификацией сломан - -**Где:** -- TRZ §1 REQ-F-04 таблица, строка «POI labels» — «`text-halo-color: #000000`, - `text-halo-width: 2px` для читаемости на спутнике». -- ADR-004 §5 пункт 2 — «`text-halo-color` (`#000000` на спутнике …) и - `text-halo-width` (`2` на спутнике …) — через `setPaintProperty`». -- TestPlan I-24 — «`text-halo-color` === `#000000`». - -**Что в коде:** `style.json:128–133` для `poi-labels`: -``` -"text-color": "#333333", -"text-halo-color": "#ffffff", -"text-halo-width": 1.5 -``` -В `style-dark.json` аналогично, `text-color` тёмный. - -Меняем halo с белого на чёрный, при этом текст остаётся `#333333` — -**тёмный текст на тёмном halo не читаем**. Это прямо нарушает цель -требования («для читаемости на спутнике»). - -**Действие:** в TRZ/ADR явно поменять на спутнике **`text-color`** -тоже (например, `#ffffff` или `#f0ede6`) или зафиксировать другую -схему контраста (инвертированная пара). Если на схеме `text-color` -должен оставаться `#333333` — это нужно вернуть в `applyBaseLayer('schematic')`, -т.е. baseline POI text-color должен попасть в перечень «исходных -значений» Data §5. - -### P1-3 — Прямое противоречие BRD ↔ TRZ/ADR/AC насчёт hillshade - -**Где:** -- BRD §5 риск 4 (hillshade): «По умолчанию hillshade **отключается** - при включении спутника — поведение фиксируется в TRZ». -- TRZ §1 REQ-F-04 таблица, строка «Hillshade»: «Включается/выключается - чекбоксом как раньше; **по умолчанию НЕ авто-выключается**». -- ADR-004 §«Открытые вопросы» / Раздел «контекст / 1.5»: «оставить - как есть». -- AC-04 сценарий «Hillshade поверх спутника»: ожидается одновременный - показ обоих слоёв (т.е. поведение без авто-выключения). - -BRD говорит «отключать», все нижестоящие документы — «не отключать». -Развилка проектная, нужно выбрать одну версию. - -**Действие:** обновить BRD §5 риск 4, чтобы он совпадал с TRZ/ADR/AC -(митигация = «hillshade продолжает работать поверх спутника как и -поверх схемы; визуальную проверку даёт UI-тест AC-04»). Если же -правильное решение всё-таки «авто-выключать» — нужно -синхронно править TRZ, ADR-004, AC-04 и TestPlan. - -### P1-4 — Цвет фона `background-color` в режиме «Спутник» — три расходящихся источника - -**Где:** -- TRZ §1 REQ-F-03: «цвет фона `#2a2a2a` для тёмной темы и **`#1a1a1a` - для светлой темы** в режиме «Спутник»» — две константы, причём - светлая получает **более тёмный** фон (`#1a1a1a` < `#2a2a2a`), что - выглядит как опечатка / перестановка. -- ADR-004 §6: одна константа `#2a2a2a` для обеих тем - (`setPaintProperty('background', 'background-color', '#2a2a2a')`). -- Data §5 строка «baseline-значения `background-color`»: `#f0ede6` - (light), **`#1a1a1a`** (dark) — но фактическое значение в - `style-dark.json:28` — `"#1a1a2e"` (а не `#1a1a1a`). - -Итого: четыре цвета упомянуто, два не совпадают с фактическим стилем -и сам ADR противоречит ТЗ по количеству констант. - -**Действие:** -1. Выбрать модель: «одна satellite-bg-константа на обе темы» (ADR-вариант, - проще) или «по константе на тему» (TRZ-вариант, эстетичнее под тёмные - снимки). Зафиксировать в TRZ и ADR одинаково. -2. Если TRZ-вариант — проверить, что светлая тема не получает более - тёмный фон, чем тёмная (исправить вероятную опечатку - `#1a1a1a`/`#2a2a2a`). -3. В Data §5 baseline тёмной темы заменить `#1a1a1a` → `#1a1a2e` - (фактическое значение из `style-dark.json:28`). - -### P1-5 — В `app.js:380–386` уже есть `layerState.basemap: ['osm-base']` и `toggleLayer('basemap')`; контракт взаимодействия не описан - -**Где (код, app.js:380):** -```js -const layerState = { tracks: true, paths: true, poi: true, basemap: true }; -const layerGroups = { …, basemap: ['osm-base'] }; -function toggleLayer(group) { …setLayoutProperty('osm-base', 'visibility', …) } -``` -Это существующий UI-механизм, способный включать/выключать `osm-base` -независимо от подложки. Ни TRZ, ни ADR-004 его не упоминают. - -**Открытые вопросы (не отвечены ни в одном артефакте):** -- При `applyBaseLayer('satellite')` мы выставляем `osm-base.visibility=none`. - Что делать с `layerState.basemap`? Сбрасывать в `false` (тогда после - возврата на «Схему» пользователь обнаружит, что схема скрыта) или - оставлять `true` (тогда `layerState` рассинхронизирован с реальной - видимостью)? -- При возврате на «Схему» (`applyBaseLayer('schematic')`) мы безусловно - выставляем `osm-base.visibility=visible` (TRZ §5.2 шаг 3.1). Если у - пользователя `layerState.basemap === false`, это нарушит его выбор. -- В UI существует ли кнопка `btn-basemap` (упоминается в `toggleLayer`)? - Что с ней делает «Спутник»: скрыть, заблокировать, оставить как - «выключатель схемы поверх спутника»? - -**Действие:** в TRZ §5.2 (алгоритм `applyBaseLayer`) и в AC-02/AC-03 -добавить раздел «Взаимодействие с существующим toggleLayer('basemap')». -Рекомендация — на спутнике переключатель «Базовая карта» либо скрыт -из UI, либо означает «показывать схему поверх спутника» (гибрид) — -но это уже out of scope (BRD §3). Самый простой контракт: при входе -в «Спутник» запомнить `layerState.basemap`, при выходе — восстановить. - -### P1-6 — `restoreTrailsState()` и обработчики «Грунтовки»/«Тропы» не управляют halo-слоями - -**Где (код, app.js:2798–2821):** `restoreTrailsState()` выставляет -`visibility` только для `trails-track` и `trails-path-bridleway`. -Обработчики чекбоксов «Грунтовки» (`onTrailsTrackCb` / аналогично) — тоже. - -В режиме «Спутник» при выключении чекбокса «Грунтовки» базовый слой -скрывается, а **halo-underlay-слой `trails-track-halo-satellite` -останется видимым** (его никто не трогает). Получим «фантом» белой -линии на спутнике без основной линии сверху. - -Ни TRZ, ни ADR-004 не правят `restoreTrailsState()` и обработчики -чекбоксов. Контракт неполный. - -**Действие:** в TRZ §5 (либо отдельный пункт «синхронизация halo с -чекбоксами слоёв») задать, что: -- `restoreTrailsState()` должен учитывать текущий base layer и - одновременно править видимость пары halo-слоя; -- обработчики чекбоксов «Грунтовки» / «Тропы» — то же самое; -- источник истины: «halo видим ⇔ (base === satellite) AND (соответствующий - чекбокс ON)». - -То же замечание про POI: при выключении чекбокса «POI» в «Спутник» -динамические правки halo текста (P1-2) тоже не нужны — но это будет -автоматически, потому что меняется `visibility` всей группы; явно -зафиксировать в TRZ. +Спецификация и реализация на уровне поведения согласованы. Z-order +проверен JS-тестом «Z-order: satellite-base вставляется beforeId=первый +terrain/trails/poi слой» и Python-тестом +`test_halo_layers_below_real_trails`. --- ## P2 — should-fix -### P2-1 — TRZ §5.2 алгоритм противоречит решению ADR-004 «Вариант O» +### P2-1 — `00-business-request.md` остался с `TBD` и неверным заголовком -**Где:** TRZ §5.2 шаг 2.2: +**Где:** `docs/work-items/ET-007/00-business-request.md`: +``` +# Business Request: ET-005: Спутниковая карта (Схема/Спутник) +Work Item ID: ET-007 +## Description +TBD +``` -> 2.2. Если layer 'satellite-base' отсутствует — addLayer (см. 4.2), -> вставлять **beforeId** = id первого слоя trails-* или terrain-* -> (первый из существующих) — чтобы спутник оказался под terrain и trails. +v1 пункты P2-4 и P2-5 не закрыты. Заголовок всё ещё «ET-005» +(пересекается с фактической ET-005 «единицы измерения»), Description +— пустой. BRD/TRZ/AC уже содержат целевую формулировку, поэтому +блокировать поставку этим нельзя, но формальное основание для +возврата остаётся. -В ADR-004 §«Вариант O» именно вариант **O-B** («через явный `beforeId` -первого trails-слоя») **отклонён** как ненадёжный («terrain/trails -добавляются асинхронно; использовать `beforeId` слоёв, которых ещё -нет, нельзя»). Выбран **O-A** — «вызвать `restoreBaseLayerState()` -первым в `rebuildMapOverlays()`», без `beforeId`. +**Действие:** заменить заголовок на «Business Request: ET-007: +Спутниковая карта (Схема / Спутник)»; в Description вставить 2–3 +предложения из BRD §1. -Алгоритм в TRZ остаётся в варианте O-B. Разработчик возьмёт TRZ. +### P2-2 — Tech-Risks R-2 митигация частично противоречит BRD F-02 -**Действие:** в TRZ §5.2 шаг 2.2 убрать `beforeId` или поменять -формулировку на «addLayer без beforeId; порядок гарантируется тем, -что restoreBaseLayerState вызывается первым в rebuildMapOverlays -(ADR-004 §«Вариант O», O-A)». +**Где:** `10-tech-risks.md` R-2 «Альтернативные провайдеры … +быстрый switch на следующего по приоритету — Mapbox или MapTiler — +потребует только введения переменной окружения для API-ключа (это +уже инфра-изменение, выходящее за scope ET-007)». -### P2-2 — Tech-Risks R-2 митигация противоречит BRD F-02 / ADR §«Вариант P» +Caveat «выходящее за scope ET-007» добавлен в v2 — это улучшение. +Но формулировка «потребует только введения переменной окружения для +API-ключа» по-прежнему противоречит BRD F-02 («без API-ключа»), +ADR-004 §«Вариант P» (Mapbox/MapTiler отклонены именно по этому +критерию) и описанию out-of-scope в BRD §3. -**Где:** Tech-Risks R-2: «быстрый switch на следующего по приоритету — -Mapbox или MapTiler — потребует только введения переменной окружения -для API-ключа». +**Действие:** в R-2 переписать митигацию честно: «при деградации Esri +без-API-ключевых публичных альтернатив с глобальным покрытием в +момент написания нет; реакция требует либо пересмотра BRD F-02 +(возврат в Анализ), либо архитектурного решения о self-hosted +satellite tiles (новый ADR)». -Но BRD F-02 явно требует «без API-ключа», ADR-004 §«Вариант P» -отклоняет Mapbox/MapTiler именно по этому критерию. Значит, такой -switch — **не** «один объект source-spec», а пересмотр функциональных -требований BRD (и инфра-требований: введение переменной окружения, -секретов, ротации). +### P2-3 — TRZ §3.2 оставлено двусмысленное указание про позицию блока -**Действие:** в R-2 заменить митигацию на честную: «при деградации -Esri единственный безапи-ключевой кандидат — отсутствует; реакция -требует пересмотра BRD F-02 (вход в Анализ) либо архитектурного -решения о self-hosted-тайлах». +**Где:** TRZ §3.2: «в начале `#terrain-popup`, **сразу после** +`
Эндуро
` ИЛИ выше него — по +выбору разработчика». -### P2-3 — Baseline `background-color` тёмной темы в Data §5 неверный +Реализация выбрала «выше заголовка» (совпадает с диаграммой §3.1 и +закреплено тестом `test_base_toggle_placed_at_top_of_terrain_popup`). +ТЗ нужно привести в соответствие, иначе следующая итерация фичи +может молча вернуть блок ниже заголовка. -**Где:** Data §5: «`#1a1a1a` (dark) — задублированы из `style*.json`». -**Что в коде:** `style-dark.json:28` — `"#1a1a2e"`. +**Действие:** в §3.2 убрать ветку «сразу после» и оставить только +«в самом верху попапа, выше заголовка «Эндуро»». -(Дубль с P1-4 в части цифры, но это узкая правка — поправить значение -в Data §5.) +### P2-4 — Отсутствует автотест для CSS-hook `body.satellite-active #btn-basemap` и для контракта halo-синхронизации с чекбоксами -### P2-4 — `00-business-request.md` содержит только `TBD` +**Где:** `tests/unit/test_base_layer.py` и `base_layer.test.js`. -**Где:** `00-business-request.md` — секция Description пустая (TBD). -По формальным правилам репозитория BR должен содержать первичное -формулирование запроса; всё остальное (BRD, TRZ, AC) уже опирается -на это как на «источник правды». Анализ-этап де-факто содержит -правду в BRD §1, но BR оставлен незаполненным. +Покрытие P1-5 и P1-6 на уровне поведения присутствует в JS-suite +**только** для частей, лежащих внутри блока ET-007 (применение +satellite-active класса делегировано на `_setBodyClass`, который в +мок-DOM деградирует в no-op — см. `_setBodyClass` lines 2989–2997). +В итоге не тестируется: -**Действие:** заполнить BR двумя-тремя предложениями (можно -конспектом из BRD §1) — это снимет одно из формальных оснований -для возврата. +1. **CSS-rule** `body.satellite-active #btn-basemap { display:none }` + — Python-тест `test_base_toggle_styles_defined` проверяет только + `.terrain-base-row` / `.terrain-base-label` / `.base-seg`. AC-02 + сценарий «Кнопка «Базовая карта» скрывается на спутнике (P1-5)» + формально не покрыт автоматическим тестом. +2. **Вызов `_applyTrailHaloVisibility(...)` из `onTrailsCheckbox`** — + функция `onTrailsCheckbox` живёт ВНЕ блока ET-007 и в JS-suite не + подгружается через `new Function`. Python-сторона лишь проверяет + присутствие id `trails-*-halo-satellite` в `app.js`, но не сам + вызов хука. AC-04 сценарии «Выключение «Грунтовки» скрывает и halo» + формально не покрыты регресс-тестом. +3. **`_savedBasemapState` save/restore цикл** — поведение + реализовано, но в JS-suite нет теста, который бы выставил + `layerState.basemap = false` до перехода в спутник и проверил, + что после возврата `osm-base.visibility === 'none'`. AC-02/03 + сценарий «Запоминание выбора Базовая карта» формально не покрыт. -### P2-5 — Несогласованный заголовок: «ET-005» при `work_item_id: ET-007` +Гэп ровно по тем границам, по которым были претензии v1 (P1-5/P1-6). +Код корректен (проверено вручную при ревью), но без регрессионных +тестов будущий рефакторинг `onTrailsCheckbox` или `applyBaseLayer` +может молча сломать контракт. -**Где:** `00-business-request.md` — `# Business Request: ET-005: Спутниковая карта (Схема/Спутник)` / `Work Item ID: ET-007`. Аналогично `.task.md` title. +**Действие (минимум):** +- Python-тест: assert `'body.satellite-active'` и `'#btn-basemap'` в + `app.css`. +- Python-тест: внутри `function onTrailsCheckbox(` тело содержит + `_applyTrailHaloVisibility`; то же для `restoreTrailsState`. +- JS-тест: один кейс на цикл «schematic (basemap=false) → satellite + → schematic», проверяющий, что `osm-base` остаётся `none` после + возврата. Сейчас в moc-окружении нет `layerState` (он вне блока + ET-007) — потребуется либо экспортировать `_savedBasemapState`, + либо добавить тонкую заглушку `layerState` в `loadBaseLayerModule`. -ET-005 — это уже сделанная задача про единицы измерения (km/mi, -ADR-0001). Двойное использование «ET-005» в заголовке этой задачи -сбивает с толку. - -**Действие:** заменить заголовок на «ET-007: Спутниковая карта -(Схема / Спутник)» — как и в BRD/TRZ/AC. - -### P2-6 — TRZ §3.2 даёт две взаимоисключающие инструкции про позицию блока - -**Где:** -- §3.1 диаграмма: «Подложка» в **самом верху** попапа (над «Эндуро»). -- §3.2 текст: «в начале `#terrain-popup`, **сразу после** - `
Эндуро
` ИЛИ **выше него** — - по выбору разработчика». - -Диаграмма и текст не совпадают. AC-01 не уточняет позицию. - -**Действие:** в §3.2 зафиксировать одну позицию (рекомендуется «выше -заголовка Эндуро», что соответствует §3.1). +Эту работу можно сделать одним PR. Не блокирует merge ET-007, но +блокирует «зрелость» теста. --- ## P3 — nice-to-have -### P3-1 — TRZ §5.5: избыточная защита `typeof restoreBaseLayerState === 'function'` +### P3-1 — Расхождение TRZ §5.2 vs код в части `beforeId` -**Где:** TRZ §5.5 показывает пример встраивания: +TRZ v2 §5.2 шаг 2.2: «addLayer (см. 4.2) **без beforeId**. Корректный +z-order гарантируется тем, что restoreBaseLayerState вызывается +ПЕРВЫМ в rebuildMapOverlays». + +Код `app.js` `applyBaseLayer()`: +```js +const before = _firstOverlayLayerId(map); +map.addLayer({ id: SATELLITE_LAYER_ID, ... }, before); +``` + +Код **дополнительно** вычисляет `beforeId` через `_firstOverlayLayerId` +(ищет первый слой с префиксом `terrain-` / `trails-` / `poi-`). Это +защита от случая, когда `restoreBaseLayerState` вызван не первым (на +старте приложения, например). Поведение **более устойчивое**, чем +требует ТЗ, и тесты I-02 и Z-order это подтверждают. Но формально +spec ↔ code расходятся. + +**Действие:** либо обновить TRZ §5.2 шаг 2.2 (добавить «с +опциональным `beforeId` от `_firstOverlayLayerId(map)` как +страховкой — не обязателен, поскольку порядок гарантирован O-A»), +либо снять защиту из кода и положиться чисто на O-A. Первый вариант +проще и оставляет код более устойчивым. + +### P3-2 — Мёртвая константа `SATELLITE_HALO_LAYER_IDS` + +`app.js:2914–2917`: +```js +const SATELLITE_HALO_LAYER_IDS = [ + 'trails-track-halo-satellite', + 'trails-path-bridleway-halo-satellite', +]; +``` + +Константа объявлена и экспортируется из фабрики юнит-тестов, но в +рантайме не используется ни единого раза — `_applyTrailHaloVisibility` +работает по локальному `pairs`. Либо использовать константу +(`pairs.forEach((p) => …)` → читать halo-id из неё), либо удалить. + +### P3-3 — Мёртвая функция `_toggleSatelliteHalo` + +`app.js:3107–3110`: +```js +function _toggleSatelliteHalo(map, enabled) { + _applyTrailHaloVisibility(map, enabled ? 'satellite' : 'schematic'); +} +``` + +Комментарий заявляет «обратная совместимость для существующих +unit-тестов», но в `tests/unit/base_layer.test.js` функция нигде не +вызывается и из фабрики не экспортируется. Если она использовалась в +промежуточной версии тестов — её можно безопасно удалить. + +### P3-4 — Избыточная защита `typeof restoreBaseLayerState === 'function'` в `rebuildMapOverlays()` + +`app.js:127–131`: ```js if (typeof restoreBaseLayerState === 'function') { restoreBaseLayerState(); } ``` -Эта защита оправдана для `rebuildGpxOverlays` (ET-006), потому что -GPX-фича живёт в отдельном файле `gpx.js`, который теоретически может -не подгрузиться. ADR-004 §2 явно решает, что фича остаётся в `app.js` -— значит функция всегда определена в том же файле, и `typeof === 'function'` + +ADR-004 §2 явно решает, что фича остаётся в `app.js` — функция всегда +определена в том же файле. Защита оправдана для `rebuildGpxOverlays` +(ET-006), где функция живёт в отдельно подгружаемом `gpx.js`. Здесь ничего не защищает. -Не блокирует, но запутывает читателя кода. Уточнить намерение или -просто вызвать функцию напрямую. - -### P3-2 — AC-08 пороги без метода замера - -**Где:** AC-08 — «первая спутниковая плитка отображается в течение -≤ 500 мс», «смена визуально мгновенная (≤ 100 мс)». Тест-план не -указывает, как именно мерить (Performance API, throttling сети, -`map.once('idle')`). 500 мс ≈ длина сетевого RTT — пограничный -порог, который без чёткого метода замера сорвётся то в одну, то в -другую сторону. - -**Действие:** в TestPlan TP-Performance (если он есть в e2e suite, -сейчас в `04-test-plan.yaml` есть только e2e-base-layer-workflow без -явного НФТ-кейса) добавить процедуру замера и условия сети. - -### P3-3 — `rebuildMapOverlays()` уже вызывает `restoreTrailsState`/`restorePoiState` после `restoreTerrainState` — порядок halo-инициализации в «Спутник» нетривиален - -Маленькая ремарка к ADR-004 §«Вариант O»: правило «satellite-base -первым» гарантирует только z-order **базы** относительно terrain/trails. -Но halo-underlay-слои тоже должны лечь **под** соответствующие base-line-слои. -Если halo-слои добавлены прямо в `style.json` (как предполагает ADR §5), -их порядок относительно `trails-track` определяется порядком в массиве -`layers` стиля, а не порядком восстановления. Стоит явно отметить в -ADR §5 / Data §6, что в `style.json` halo-слой ставится **непосредственно -перед** соответствующим базовым (выше `osm-base`, ниже `trails-track`), -иначе halo окажется поверх базовой линии и испортит вид и на схеме, и -на спутнике (включён только тогда halo не виден из-за `visibility:none`, -но как только включится — проблема). +Не блокирует, но запутывает читателя кода. То же относится к двум +вхождениям в init-IIFE (там защита тоже стоит). --- ## Сводка -| ID | Severity | Категория | Действие | -|-------|----------|---------------------------------|-----------------| -| P1-1 | P1 | Несуществующие слои grade1..5 | TRZ/ADR/Data/Risks — переписать под реальные id | -| P1-2 | P1 | POI labels: тёмный текст + чёрный halo | TRZ/ADR — добавить смену text-color | -| P1-3 | P1 | BRD vs TRZ — hillshade авто-выкл| Обновить BRD §5 | -| P1-4 | P1 | background-color: 3 источника | Свести к одному; исправить опечатку и baseline | -| P1-5 | P1 | Контракт с layerState.basemap не задан | TRZ §5.2 + AC | -| P1-6 | P1 | halo не синхронизирован с чекбоксами Грунтовки/Тропы | TRZ §5 | -| P2-1 | P2 | TRZ §5.2 beforeId vs ADR O-A | Поправить TRZ §5.2 | -| P2-2 | P2 | Tech-Risks R-2 митигация противоречит BRD F-02 | Переформулировать R-2 | -| P2-3 | P2 | Baseline dark `#1a1a1a` ≠ `#1a1a2e` в стиле | Data §5 | -| P2-4 | P2 | BR.md — `TBD` | Заполнить | -| P2-5 | P2 | Заголовок «ET-005» при id ET-007| Переименовать | -| P2-6 | P2 | TRZ §3.2 двусмысленность позиции| Зафиксировать одну | -| P3-1 | P3 | Избыточный `typeof === 'function'`| Уточнить или убрать | -| P3-2 | P3 | AC-08 без метода замера | Добавить процедуру в TestPlan | -| P3-3 | P3 | Порядок halo в style.json не оговорён | Уточнить в ADR §5 | +| ID | Severity | Категория | +|-------|----------|----------------------------------------------| +| P2-1 | P2 | BR.md: TBD и заголовок «ET-005» | +| P2-2 | P2 | Tech-Risks R-2 митигация частично vs BRD F-02| +| P2-3 | P2 | TRZ §3.2 — двусмысленное указание про позицию| +| P2-4 | P2 | Нет автотестов для CSS-hook и hook'ов halo (граница AC-02/04 P1-5/P1-6) | +| P3-1 | P3 | TRZ §5.2 «без beforeId» vs код с `_firstOverlayLayerId` | +| P3-2 | P3 | Мёртвая константа `SATELLITE_HALO_LAYER_IDS` | +| P3-3 | P3 | Мёртвая функция `_toggleSatelliteHalo` | +| P3-4 | P3 | Избыточный `typeof === 'function'` | + +--- ## Что хорошо -- ADR-004 — добротный: явное обоснование выбора по 5 осям (P/M/S/O/H) - с таблицей альтернатив, явная классификация «minor change», - раздел «Технический долг» с конкретным сценарием миграции в - `basemap.js`. -- Архитектурный реестр `docs/architecture/adr/README.md` и - `docs/architecture/README.md` обновлены синхронно с ET-007 (раздел - «Внешние тайл-провайдеры»). -- Tech-Risks (R-1..R-10) — полное покрытие, со ссылками на конкретные - митигации (UI-тесты, идемпотентные паттерны, точки расширения). - R-2 — единственный «высокий», но честно зафиксирован. -- Data §7 (PII) и Infra §3 (внешние сетевые вызовы) чётко описывают - расширение круга третьих сторон и сохраняют «приватный по умолчанию» - через ленивое создание source. -- TestPlan по структуре богатый (unit + integration + e2e + error - handling). AC покрывает 10 сценариев, включая mobile (AC-09) и - регресс существующего функционала (AC-10). +- **Покрытие P1 v1 — 6/6 в коде и в спецификации одновременно.** Это + редкий случай: чаще в одном из двух фронтов остаются концы. +- **JS-suite 33 теста** через `new Function`-загрузку блока — изящный + способ исполнить реальный production-код в Node без бандлера и без + переписывания app.js в ES-модуль. Покрытие включает edge-cases + (private mode → localStorage недоступен, `window._map` отсутствует, + невалидное stored-значение, повторный toggle, dark/light темы, + z-order при пустом наборе overlay'ев). +- **Маркеры блока `// >>> ET-007 base layer toggle block` / + `// <<<`** — позволяют как читать блок целиком (поиск 30+ функций + в 3132-строчном `app.js` — боль), так и подгружать его в тестах. То + же решение применил ET-002 для POI; единый паттерн. +- **Декларативные halo-underlay-слои в `style.json` / `style-dark.json`, + расположенные перед соответствующими `trails-*`** — z-order + явно зафиксирован в стиле и закреплён регресс-тестом + `test_halo_layers_below_real_trails` (оба файла стиля). Любое + будущее «перенесём слой» сломает тест немедленно. +- **`_savedBasemapState` + CSS-class hook** — корректное и минимально + инвазивное решение P1-5: модуль ET-007 не правит `layerState` + существующего модуля, не редактирует `toggleLayer()`, не лезет в + его обработчики; контракт реализован через одну приватную + переменную и одну CSS-зависимость. Это самый низкий blast radius, + который можно было выбрать. +- **Idempotent `addSource/addLayer`** через `if (!map.getSource(…))` / + `if (!map.getLayer(…))` (R-6) — соответствует паттерну + `terrain` / `trails` / `poi`. +- **Документация и changelog** во всех артефактах v2 точно указывают, + какие P-findings из v1 ими закрыты (`changelog: "v2 ... P1-1..P1-6"`). + Это резко упрощает повторное ревью. ## Что делать дальше -1. Исправить P1 (см. Действие в каждом пункте) — обновить TRZ, ADR, - BRD, Data, Tech-Risks. После правок поднять `version` затронутых - документов до v2. -2. Желательно — закрыть P2-1…P2-6 в том же раунде. -3. P3 — на усмотрение Owner/Architect. -4. После правок — повторное ревью артефактов (этот же документ, v2). -5. **Только после этого** — переход к stage `development`. - -Кода в ветке нет; ревью кода будет отдельным шагом после -реализации. +1. **APPROVED → merge в main.** Никаких P0/P1 нет, регресс не + обнаружен, тесты зелёные. +2. **Параллельно или одним PR** закрыть P2-1, P2-2, P2-3, P2-4. P2-4 + — самое полезное (закрывает регрессионный риск для P1-5/P1-6 + логики). Остальные — гигиена документации. +3. **P3 — на усмотрение** разработчика/Owner. Самый полезный из них — + P3-1: либо синхронизировать TRZ с кодом (рекомендуется), либо + снять защитный `beforeId` (тогда поведение строго совпадает с + ADR §O-A). +4. После merge — переход в `testing` stage. Браузерные кейсы + (E-01..E-10, TC-UI-01..14) требуют Playwright-инфраструктуры, + которой в репозитории пока нет (см. ET-002 ADR-0001, 07-infra- + requirements.md). До её появления покрытие AC обеспечивается + связкой статических Python-тестов + JS-unit + ручной приёмки + на test-стенде.