satellite-provider/_docs/03_implementation/reviews/batch_23_review.md

Code Review Report

Batch: 23 Tasks: AZ-376 (C23), AZ-378 (C25), AZ-379 (C26), AZ-380 (C27) Date: 2026-05-11 Verdict: PASS_WITH_WARNINGS

Findings

#	Severity	Category	File:Line	Title
1	Low	Maintainability	SatelliteProvider.DataAccess/Repositories/RouteRepository.cs:38	route_points column list left inline (single use)
2	Low	Spec-Gap	SatelliteProvider.DataAccess/Repositories/TileRepository.cs:69	EARTH_CIRCUMFERENCE_METERS, TILE_SIZE_PIXELS, 111000.0 literals still in GetTilesByRegionAsync
3	Low	Spec-Gap	task ACs reference "37 unit + 5 smoke"	Stale test count in task ACs

Finding Details

F1: route_points column list left inline (single use) (Low / Maintainability)

• Location: SatelliteProvider.DataAccess/Repositories/RouteRepository.cs:38-46
• Description: GetRoutePointsAsync keeps its 8-column list inline rather than extracting a second const. AZ-379 task spec is explicit ("Per repository: extract the column list once" — singular) and the column list is used at exactly one site, so extracting would not reduce duplication.
• Suggestion: leave as-is. If a future SELECT also needs the route_points columns, extract RoutePointColumnList then.
• Task: AZ-379

F2: Earth-related literals still in GetTilesByRegionAsync (Low / Spec-Gap)

• Location: SatelliteProvider.DataAccess/Repositories/TileRepository.cs:58-67, 90
• Description: EARTH_CIRCUMFERENCE_METERS, TILE_SIZE_PIXELS, and 111000.0 are still local constants/literals. These are the explicit target of AZ-377 (C24), which is in batch 24 and has dependency AZ-371 ✓. Out of scope for batch 23.
• Suggestion: addressed in batch 24 by AZ-377.
• Task: AZ-377 (out of scope here)

F3: Stale test count in task ACs (Low / Spec-Gap)

• Location: all four task spec files (AZ-376_*.md, AZ-378_*.md, AZ-379_*.md, AZ-380_*.md) — AC-3
• Description: each AC-3 quotes "37 unit + 5 smoke tests stay green". Pre-/document-era figure; current count is 190 unit tests after this batch (was 181 before). Same finding as F1 of batch_22_review (carried over).
• Suggestion: defer to refactor Phase 7 documentation sweep (batch 22 review already noted this).
• Task: shared

Phase Notes

Phase 1 — Context loading: read _docs/04_refactoring/03-code-quality-refactoring/list-of-changes.md (C23, C25, C26, C27), four task specs, and the current source under review.

Phase 2 — Spec compliance: every AC has a corresponding test in RepositoryRefactorTests.cs or GeoUtilsRefactorTests.cs:

• AZ-376 AC-1 ↔ TileRepository_DoesNotExposeFindExistingTileAsync_AZ376_AC1
• AZ-376 AC-2/AC-3 ↔ build + 190/190 unit run (covers AC-2 build success and AC-3 tests-green)
• AZ-378 AC-1 ↔ TileRepository_KeepsAndUsesLogger_AZ378_AC1
• AZ-378 AC-2 ↔ RegionRepository_HasNoUnusedLoggerParameter_AZ378_AC2 + RouteRepository_HasNoUnusedLoggerParameter_AZ378_AC2
• AZ-378 AC-3 ↔ 190/190 unit run
• AZ-379 AC-1 ↔ *_DefinesColumnListConstantOnce_AZ379_AC1 (one per repo)
• AZ-379 AC-2 ↔ RepositoryColumnLists_ContainExpectedColumns_AZ379_AC2 (column tokens preserved). Strict byte-for-byte SQL is structurally guaranteed by const interpolation; smoke run (deferred to test-run skill) will validate end-to-end DB interaction.
• AZ-379 AC-3 ↔ 190/190 unit run
• AZ-380 AC-1 ↔ GeoUtils_DoesNotExposeCalculatePolygonDiagonalDistance_AZ380_AC1
• AZ-380 AC-2/AC-3 ↔ build + 190/190 unit run

Phase 3 — Code quality:

• New SlowQueryThresholdMs = 500 constant has a brief one-line use site; tunable later via config (deferred to a separate ticket if telemetry becomes formal).
• Stopwatch.StartNew() + log on threshold breach is the cheapest visible-only-when-slow pattern; no perf overhead in the hot path beyond a long comparison.
• private const string ColumnList placed at the top of each repository class; $@"..." interpolation requires C# 10+ const-interpolated strings (project targets net8.0, fully supported).
• Removed using Microsoft.Extensions.Logging from RegionRepository.cs and RouteRepository.cs after dropping the field.

Phase 4 — Security quick-scan:

• ColumnList interpolation is private const string defined in source code (not user input). Same trust boundary as the original inline const string sql. Dapper parameterization preserved for all actual values. No SQL-injection vector introduced.
• Slow-query log emits latitude, longitude, sizeMeters, zoomLevel — geographic coordinates, not PII; consistent with existing API logging.
• No hardcoded secrets. No new external input paths.

Phase 5 — Performance scan:

• Net positive: GetTilesByRegionAsync now self-reports slow queries. Hot path adds one Stopwatch.GetTimestamp start + one comparison + the existing await; negligible.
• ColumnList interpolation is compile-time; zero runtime cost.
• AZ-376 deletes a database round-trip method (no callers, but if reflection ever found it, it's gone now). Minor surface reduction.

Phase 6 — Cross-task consistency:

• AZ-376 (delete method) and AZ-378 (logger field) and AZ-379 (SELECT constants) all touch TileRepository.cs; applied in topological order avoiding interaction. Final state merges cleanly.
• DI registrations in Program.cs updated atomically with RegionRepository and RouteRepository constructor changes — no DI breakage.
• No conflicting patterns: all repos still use Dapper + raw SQL; constructor shape now varies (TileRepository keeps logger, others don't), justified by per-repo logger-usage decision in AZ-378.

Phase 7 — Architecture compliance:

• All edits stay within SatelliteProvider.DataAccess/** (DataAccess Owns) and SatelliteProvider.Common/Utils/GeoUtils.cs (Common Owns). Program.cs is WebApi Owns and is allowed to import DataAccess.
• No new cross-component imports. No new cyclic deps. No duplicate symbols introduced.
• module-layout.md Public API surface for ITileRepository changed (one method removed). All external consumers (none use the deleted method per grep) unaffected.

Baseline Delta

_docs/02_document/architecture_compliance_baseline.md exists. No new Architecture findings introduced; F2 from baseline (logger fields not used) is partially Resolved for RegionRepository and RouteRepository (deleted) and Carried over as a deliberate-use case for TileRepository.

Status	Finding
Resolved	F-baseline: RegionRepository._logger unused — deleted (AZ-378)
Resolved	F-baseline: RouteRepository._logger unused — deleted (AZ-378)
Carried over (now justified)	F-baseline: TileRepository._logger unused — now used by GetTilesByRegionAsync slow-query warning (AZ-378)

Verdict

PASS_WITH_WARNINGS — three Low findings, all informational or out-of-scope; zero Critical / High / Medium.