ui/_docs/03_implementation/reviews/batch_11_review.md

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.