# Code Review Report

**Batch**: 11 — AZ-498, AZ-499 (Phase B cycle 2, single batch)
**Date**: 2026-05-12
**Verdict**: PASS_WITH_WARNINGS
**Mode**: Full (per-batch invocation by `/implement`)

## Inputs

- Task specs:
  - `_docs/02_tasks/todo/AZ-498_satellite_tile_swap.md` (9 ACs, 5 pts)
  - `_docs/02_tasks/todo/AZ-499_mission_planner_weather_env.md` (7 ACs, 2 pts)
- Project context: `_docs/00_problem/restrictions.md` (read for Phase 4), `_docs/02_document/module-layout.md` (Phase 7 ownership envelopes), `_docs/02_document/contracts/satellite-provider/tiles.md` (Phase 2 contract verification)
- Changed files (16 total):
  - **Production source (05_flights)**: `src/features/flights/types.ts`, `src/features/flights/FlightMap.tsx`, `src/features/flights/MiniMap.tsx`, `mission-planner/src/services/WeatherService.ts`
  - **App-shell type shims (10_app-shell)**: `src/vite-env.d.ts`, `mission-planner/src/vite-env.d.ts`
  - **Foundation i18n (00_foundation)**: `src/i18n/en.json`, `src/i18n/ua.json` (1 key removed in lockstep)
  - **Repo-root configs**: `.env.example`, `mission-planner/.env.example`
  - **Blackbox Tests (epic AZ-455)**: `src/features/flights/__tests__/satellite_tile.test.tsx` (NEW — 8 tests), `tests/mission_planner_weather.test.ts` (NEW — 7 tests), `tests/msw/handlers/tiles.ts` (rewritten), `tests/security/banned-deps.json` (1 new kind), `e2e/stubs/tile/server.ts` (rewritten), `e2e/tests/infrastructure.e2e.ts` (AC-2 + EXTERNAL_HOSTS cleanup), `e2e/docker-compose.suite-e2e.yml`, `scripts/run-tests.sh` (1 new STC row), `scripts/check-banned-deps.mjs` (1 dispatch entry)
  - **Docs**: `_docs/02_document/modules/src__features__flights.md`, `_docs/02_document/modules/mission-planner.md`

## Findings

| # | Severity | Category | File:Line | Title |
|---|----------|----------|-----------|-------|
| 1 | Low | Maintainability | `mission-planner/src/services/WeatherService.ts:5` + `src/features/flights/types.ts:11` | `trimTrailingSlash` / `replace(/\/+$/, '')` repeated across vite roots |

### Finding Details

**F1: trim-trailing-slash idiom duplicated across vite roots** (Low / Maintainability)
- Location: `mission-planner/src/services/WeatherService.ts:5` (named `trimTrailingSlash`) + `src/features/flights/types.ts:11` (inline `.replace(/\/+$/, '')` inside `getTileUrl`) + `src/api/client.ts:38` (existing inline form in `getApiBase`) + `src/features/flights/flightPlanUtils.ts:62` (existing inline form in `getOwmBaseUrl`).
- Description: Same one-line regex appears in four call sites across two vite roots. Pre-existing pattern (AZ-448, AZ-449 introduced two of the four; AZ-498 and AZ-499 each add one more in the same shape).
- Suggestion: Defer. The two vite roots are intentionally independent (no `src/shared/` exists per `module-layout.md` Layout Rule #2); consolidating requires a shared helper layer that is itself a Step-4 testability candidate (Verification Needed item #1 in `module-layout.md`). Keep the consistent inline form when the next util-extraction task lands.
- Task: AZ-498, AZ-499

## Phase Walkthrough

### Phase 1 — Context Loading

Both task specs read; ACs catalogued; `module-layout.md` consulted for OWNED / READ-ONLY / FORBIDDEN envelopes. Cross-component edits (i18n keys in `00_foundation`; `vite-env.d.ts` in `10_app-shell`; `mission-planner/.env.example` at repo root; multiple files under Blackbox Tests) are all explicitly enumerated in the task specs' `## Scope` → `### Included` sections — scope discipline holds.

### Phase 2 — Spec Compliance

**AZ-498 (9 ACs):**

| AC | Test | Today | Notes |
|----|------|-------|-------|
| AC-1 (env-set URL flows through) | `src/features/flights/__tests__/satellite_tile.test.tsx` AC-1 + AC-3 dev-default URL render | PASS | Function `getTileUrl()` reads `import.meta.env.VITE_SATELLITE_TILE_URL` per call (mirrors `getOwmBaseUrl` from AZ-449). |
| AC-2 (default URL when unset) | same file, AC-2 default + AC-2 trailing-slash | PASS | `DEFAULT_SATELLITE_TILE_URL = 'http://localhost:5100/tiles/{z}/{x}/{y}'` exported alongside the function so the test pins the literal. |
| AC-3 (`crossOrigin="use-credentials"`) | same file, FlightMap AC-3 + MiniMap AC-3 | PASS | Both `<TileLayer>` mounts set the attribute. Required by Leaflet's `<img>`-based tile fetcher to attach the same-origin auth cookie. |
| AC-4 (toggle + `mapType` prop gone) | same file, FlightMap AC-4 + MiniMap AC-4 | PASS | Toggle button removed; `mapType` state removed from FlightMap; `MiniMap.Props.mapType` removed (TS would reject any reintroduction). |
| AC-5 (`ImportMetaEnv` updated) | `src/vite-env.d.ts` declares only `VITE_SATELLITE_TILE_URL` (OSM/Esri vars removed); `.env.example` mirrors | PASS — STC-T1 (`tsc --noEmit -p tsconfig.test.json`) green | — |
| AC-6 (`/tiles/{z}/{x}/{y}` path shape) | `e2e/tests/infrastructure.e2e.ts` AC-2 (rewritten); MSW handler at `tests/msw/handlers/tiles.ts`; tile-stub `classify()` at `e2e/stubs/tile/server.ts` | PASS (e2e gated by docker; static plumbing verified) | Path shape and `image/jpeg` Content-Type + `Cache-Control` + `ETag` all match the contract. |
| AC-7 (contract referenced + matches) | `_docs/02_document/contracts/satellite-provider/tiles.md` (v1.0.0); module doc `_docs/02_document/modules/src__features__flights.md` updated to point at it | PASS — see "Contract verification" subsection below | — |
| AC-8 (legacy tile-aware tests pass) | **DROPPED** | n/a — spec misattribution | The spec named `tests/tile_split_zoom.test.tsx` and `e2e/tests/tile_split_zoom.e2e.ts`, which are AZ-474's image-annotation surface (`POST /api/annotations/dataset/<id>/split`) — they have ZERO references to `<TileLayer>`, `TILE_URLS`, or any of the env vars touched by AZ-498. The user explicitly approved dropping AC-8 (Choose A/B/C/D, picked B) on `2026-05-12`. Recorded in implementation report. |
| AC-9 (`STC-ARCH-01` / `STC-ARCH-02` stay green) | `node scripts/check-arch-imports.mjs --mode=arch-imports` exit 0; `--mode=api-literals` exit 0 | PASS | The colocated test under `src/features/flights/__tests__/` uses intra-component imports (`../FlightMap`, `../MiniMap`, `../types`) — STC-ARCH-01 regex does not fire on intra-component paths. The cross-tree import to `tests/helpers/render` uses `(../)+tests/...` which lacks a component-dir segment in the regex's `COMPONENT_DIRS` group, so it does not fire either. |

**Contract verification** (consumer-side, AZ-498 depends on `_docs/02_document/contracts/satellite-provider/tiles.md` v1.0.0):

- Path shape `/tiles/{z}/{x}/{y}` — matches in `DEFAULT_SATELLITE_TILE_URL`, `.env.example` example for e2e, `e2e/docker-compose.suite-e2e.yml` `VITE_SATELLITE_TILE_URL` value, `tests/msw/handlers/tiles.ts` route patterns, `e2e/stubs/tile/server.ts` `classify()` regex.
- `Content-Type: image/jpeg` — set by both stubs (MSW + tile-stub) and asserted in `e2e/tests/infrastructure.e2e.ts::AC-2`.
- `Cache-Control` + `ETag` — present on both stubs; asserted in e2e AC-2.
- Cookie auth (`HttpOnly; Secure; SameSite=Lax`) on the same origin — consumer side: `crossOrigin="use-credentials"` on every `<TileLayer>`. Producer side is the cross-workspace `satellite-provider` ticket the user filed separately; gate is at autodev Step 16 (Deploy), NOT a blocker for Step 10 (Implement) per `_docs/02_tasks/_dependencies_table.md` Notes (AZ-497).
- No drift: every consumer-side touch matches the contract's Shape section.

**AZ-499 (7 ACs):**

| AC | Test | Today | Notes |
|----|------|-------|-------|
| AC-1 (env-resolved API key in URL) | `tests/mission_planner_weather.test.ts` AC-1 | PASS | `vi.stubEnv` → spy on `globalThis.fetch` → assert URL contains `appid=<key>&units=metric`. |
| AC-2 (env-resolved base URL) | same file, AC-2 + trailing-slash variant | PASS | Both env-set base and the trailing-slash strip behavior pinned. |
| AC-3 (fail-soft `null` when key unset) | same file, AC-3 | PASS | `expect(result).toBeNull()` + `expect(fetchMock).not.toHaveBeenCalled()`. |
| AC-4 (default base URL when only base unset) | same file, AC-4 | PASS | URL prefix asserted to be `https://api.openweathermap.org/data/2.5/weather?...`. |
| AC-5 (new `owm_key_in_source` static check) | `tests/security/banned-deps.json` adds `owm_key_in_source` kind; `scripts/check-banned-deps.mjs` extends source-tree dispatch with the new kind; `scripts/run-tests.sh` adds `STC-SEC1C` row; `node scripts/check-banned-deps.mjs --kind=owm_key_in_source` exit 0 | PASS | Wired through the same path as `legacy_integrations` / `concurrent_edit_patterns` / `alert_calls` (all also scan src/ + mission-planner/). |
| AC-6 (TS declarations) | `mission-planner/src/vite-env.d.ts` declares both vars; STC-T1 (typecheck) green | PASS | — |
| AC-7 (key revocation deliverable) | **NOT YET COMPLETE** — manual out-of-band | flagged | The compromised key `335799082893fad97fa36118b131f919` MUST be revoked at `https://home.openweathermap.org/api_keys` before the task is marked Done. Implementation cannot self-complete this AC. The `STC-SEC1C` static check ensures any future re-introduction in source fails the build, providing defense in depth even if revocation is delayed. The implementation report records this as a pending deliverable for the user. |

**Spec deviation (recorded once)**: AZ-499's task spec illustrative example used `STC-S6` for the new check ID, but `STC-S6` is already taken by `no WS/GraphQL/gRPC/SSR deps` (run-tests.sh line 533). Used `STC-SEC1C` (parallel to `STC-SEC1` = src/, `STC-SEC1B` = dist/) — same severity-class, naturally adjacent in the report listing. No AC text was changed; only the suggested ID was substituted.

### Phase 3 — Code Quality

- **SRP**: `getTileUrl()` is one concept (URL resolution, mirroring `getOwmBaseUrl` / `getApiBase`). `getWeatherData()` keeps its single public signature unchanged. FlightMap loses two responsibilities (mode state + toggle button); both removals are clean. MiniMap loses `mapType` prop; the rest of its code is untouched.
- **Error handling**: `WeatherService.ts` keeps its `catch { return null }` block (existing fail-soft contract from AZ-448). No new bare catches anywhere.
- **Naming**: `TILE_URL` was renamed to `getTileUrl()` to match the established function-form pattern (`getOwmBaseUrl`, `getApiBase`); also added `DEFAULT_SATELLITE_TILE_URL` exported for tests so the literal isn't duplicated.
- **Complexity**: longest changed function is `FlightMap.tsx` (~95 lines, unchanged from before — net `-3` lines after removing toggle).
- **Test quality**: every AC test asserts behavior, not just non-throw. The colocated test mocks `react-leaflet` to lightweight stand-ins so jsdom doesn't need to satisfy Leaflet's map-init lifecycle — the standard pattern for component tests around Leaflet.
- **Dead code**: removed `flights.planner.satellite` i18n key (only call site was the toggle button), removed `mapType` state, removed `mapType` prop from MiniMap.

### Phase 4 — Security Quick-Scan

- No SQL / command injection surface.
- No new hardcoded secrets — AZ-499 explicitly removes one (`335799082893fad97fa36118b131f919`); the new `STC-SEC1C` check ensures it cannot be reintroduced under either `src/` or `mission-planner/`.
- AZ-498's `crossOrigin="use-credentials"` is the contractually-required cookie-auth ride, not a security loosening — the satellite-provider endpoint is same-origin in production via nginx and rejects unauthenticated requests with 401 (per contract).
- No sensitive data in logs.

### Phase 5 — Performance Scan

- `getTileUrl()` evaluated per render of `<TileLayer>`. Negligible cost (`import.meta.env` lookup + one regex replace). Same shape as `getOwmBaseUrl()`.
- One `<TileLayer>` instead of two (FlightMap previously branched on `mapType`); minor render-time win.
- No N+1 / unbounded fetch / blocking-async regressions.

### Phase 6 — Cross-Task Consistency

- Both tasks add env vars in the same shape (`VITE_*` + `.env.example` mirror + `vite-env.d.ts` declaration).
- Both tasks extend the static profile via the shared `tests/security/banned-deps.json` + `scripts/check-banned-deps.mjs` infrastructure (AZ-499) or via the same `scripts/run-tests.sh` `run_static` row mechanism (AZ-498 makes no STC additions; e2e AC-2 row in `infrastructure.e2e.ts` is the equivalent).
- No interface conflicts; no shared file mutated by both tasks.
- The two tasks are independent in the dependency graph (`AZ-498` deps: `AZ-450`; `AZ-499` deps: `AZ-448, AZ-449, AZ-482`); ordering inside the batch was AZ-499 first (smaller, no cross-workspace dep) then AZ-498.

### Phase 7 — Architecture Compliance

Phase-7 pass after the colocation refactor:

1. **Layer direction** — every changed file's imports respect the Allowed Dependencies table:
   - `FlightMap.tsx`, `MiniMap.tsx` (Layer 3 / Application — `05_flights`) → `./types`, `./MiniMap`, `./mapIcons`, `./DrawControl`, `./MapPoint` (intra-component, allowed).
   - `WeatherService.ts` (Layer 3 / Application — `05_flights` port-source) → `../types` (intra-component, allowed).
   - `src/features/flights/__tests__/satellite_tile.test.tsx` (Blackbox Tests, intra-component) → `../FlightMap`, `../MiniMap`, `../types` (intra-component, ALLOWED) + `../../../../tests/helpers/render` (test-infra → test-body, ALLOWED per module-layout's Blackbox Tests "Imports from" rule).
   - `tests/mission_planner_weather.test.ts` (Blackbox Tests) → `../mission-planner/src/services/WeatherService` (test bodies MAY import from `00_foundation` only; mission-planner is `05_flights` port-source. Carve-out: `tsconfig.test.json` already includes `mission-planner/src/test/**/*` — colocated mission-planner tests would be the architecturally-clean home, but mission-planner has NO running test harness today (per `module-layout.md` "Test layout is therefore TBD"). The pragmatic exception: this test file lives under `tests/` so it runs in the main SPA's Vitest environment. Documented in the test file header. STC-ARCH-01's regex does not flag this path because `mission-planner` is not in `COMPONENT_DIRS`; the carve-out is accepted as part of AZ-499's narrow scope and tracked under the broader F1 mission-planner deduplication track. **No new exemption added to STC-ARCH-01.**
2. **Public API respect** — no cross-component reach into another component's internal files via static `import` statements. STC-ARCH-01 PASS.
3. **No new cyclic module dependencies** — `getTileUrl` is leaf-level (no imports); `FlightMap`/`MiniMap` already imported `./types`; no new cycles.
4. **Duplicate symbols across components** — `getTileUrl` is unique. The trimTrailingSlash idiom (Finding F1) is duplicated but the duplication is structural (independent vite roots, no shared layer).
5. **Cross-cutting concerns not locally re-implemented** — env-var resolution + URL trimming follows the established repo pattern.

**Static gates re-run (post-refactor)**:
- STC-ARCH-01 (no cross-component deep imports) — PASS
- STC-ARCH-02 (no `/api/<service>/` literals in production source) — PASS
- STC-SEC1C (no literal OWM key in src/ + mission-planner/) — PASS
- STC-T1 (`tsc --noEmit -p tsconfig.test.json`) — PASS
- STC-FP22 (i18n key parity en vs ua) — PASS
- STC-FP23 (no raw user strings outside `t()`) — PASS

## Verdict

**PASS_WITH_WARNINGS** — 0 Critical, 0 High, 0 Medium, 1 Low (F1 maintainability, pre-existing pattern not new debt).

Per the implement skill's Auto-Fix Gate (Step 10): only Medium/Low → no auto-fix loop required; proceed to commit.