Skip to content

feat(gallery): add map view with geo-tagged photo markers#83

Merged
joaquimscosta merged 11 commits intomainfrom
feat/gallery-map-view
Feb 27, 2026
Merged

feat(gallery): add map view with geo-tagged photo markers#83
joaquimscosta merged 11 commits intomainfrom
feat/gallery-map-view

Conversation

@joaquimscosta
Copy link
Copy Markdown
Contributor

Summary

  • Backend: Add hasGeo query parameter to GET /api/v1/gallery endpoint for server-side geo-filtering
  • Shared primitives: Extract reusable BaseMap component and useMapClustering hook from BravaMap, refactor MapCanvas to use them
  • Gallery map: Build GalleryMapCanvas with clustered photo markers, popup cards, empty states, and auto-fit bounds
  • Integration: Add map view toggle (grid/timeline/map), lazy-loaded via next/dynamic, URL sync with ?view=map, "Show on Map" button in lightbox
  • Tests: 7 backend integration tests + 14 frontend component tests + 10 geo-adapter unit tests
  • Accessibility: ARIA roles, keyboard navigation, 44px touch targets, semantic design tokens

Spec

plan/arkhe/specs/026-gallery-map-view/ — 12 tasks across 4 waves, all completed. Verification report: 95% overall confidence (PASS).

Key files

Area Files
Backend GalleryController.kt, GalleryService.kt, GalleryControllerGeoFilterTest.kt
Shared map features/map/shared/base-map.tsx, use-map-clustering.ts, types.ts
Gallery map components/gallery/gallery-map-canvas.tsx, gallery-map-marker.tsx, gallery-map-popup.tsx
Integration gallery-content.tsx, image-lightbox.tsx, gallery-mappers.ts
Tests gallery-map-canvas.test.tsx, gallery-mappers.test.ts

Test plan

  • Backend: ./gradlew test — 7 geo-filter integration tests pass
  • Frontend: pnpm run test:unit — 270/270 tests pass (14 new component tests)
  • TypeScript: npx tsc --noEmit — clean
  • Manual: Navigate to /gallery?view=map, verify markers, clustering, popup, empty states
  • Manual: Open lightbox → "Show on Map" → verify fly-to animation
  • Manual: Test on mobile viewport (responsive layout, touch targets)

…y results

Add optional hasGeo boolean parameter to GET /api/v1/gallery that filters
results to only UserUploadedMedia items with non-null latitude and longitude.
Combines with existing category, decade, and search filters via AND logic.
Includes integration tests for all filter combinations.
Create BaseMap and useMapClustering as reusable shared primitives
extracted from BravaMap's MapCanvas. Refactor MapCanvas to consume
these primitives, preparing for GalleryMapCanvas reuse.

- BaseMap: thin wrapper around react-map-gl with token, defaults, ref forwarding
- useMapClustering: generic hook wrapping Supercluster with expandCluster helper
- Shared types: ClusterPointProperties, GeoPointFeature, MapBounds
- MapCanvas: replaced direct Map/useSupercluster with shared primitives
Wave 3: Build GalleryMapCanvas with markers, popup, and geo adapter.

- GalleryMapCanvas: flat 2D map using shared BaseMap + useMapClustering
- GalleryMapMarker: 48x48px photo thumbnail markers with pin nub
- GalleryMapPopup: preview card with title, location, date, CTA
- mediaItemToGeoFeature(): converts MediaItem to GeoJSON point
- hasGeo parameter threaded through API client and query hook
- Empty state for zero geo-tagged photos
… a11y

Wave 4: Wire gallery map into the gallery page with full integration.

- GalleryView type extended to "grid" | "timeline" | "map"
- Map toggle button with MapPin icon and geo-tagged photo count tooltip
- GalleryMapCanvas lazy-loaded via next/dynamic (ssr: false)
- Photo selection from map opens lightbox
- "Show on Map" button in lightbox desktop sidebar and mobile bottom sheet
- Lightbox → map: closes lightbox, switches to map view, flies to location
- Filtered empty state with "Clear Filters" action
- aria-pressed on all view toggle buttons
- Photo interface extended with latitude/longitude for lightbox
…ctedPhotoId

Move flyToCoords handler from render body to useEffect to prevent
DOM mutations during render. Wire selectedPhotoId prop so the map
highlights the photo currently open in the lightbox.
- Fix deep-link navigation: ?view=map now correctly restores map view on page load
- Wire hasGeo=true to API: Map view now fetches only geo-tagged photos from backend
- Add auto-fit bounds: Maps with 1-3 markers auto-fit to show all locations in view
- Update error message: Change error text to match spec ("Map unavailable")
- Add frontend tests: 10 Vitest tests for mediaItemToGeoFeature (valid/null/edge coords)

Fixes all 5 gaps identified in verification report:
- page.tsx initialView parsing now recognizes 'map' value
- gallery-content.tsx filters include hasGeo when activeView === 'map'
- gallery-map-canvas.tsx uses fitBounds for sparse marker sets
- Tests achieve 100% coverage of GeoJSON conversion logic with null safety
…mbnail URL

Add 14 Vitest component tests for GalleryMapCanvas covering populated
state (markers, count badge, ARIA), empty states (zero photos, filtered),
and callback wiring (onViewChange, onClearFilters). Fix stale YouTube
thumbnail URL expectation (maxresdefault → hqdefault) in gallery-mappers
tests.
@chatgpt-codex-connector
Copy link
Copy Markdown

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.

@github-actions
Copy link
Copy Markdown

github-actions bot commented Feb 26, 2026

🚀 PR Validation Results

📁 Components Changed:

  • ℹ️ No core component changes detected

🔍 Validation Results:

⏭️ Backend CI: Skipped (no relevant changes)
Frontend CI: success
⏭️ Infrastructure CI: Skipped (no relevant changes)
Global Security Scan: success

🎉 Status: READY FOR REVIEW

All validation checks have passed! This PR is ready for code review.


Updated: 2026-02-27T00:18:02.444Z | PR: #83

@joaquimscosta
Copy link
Copy Markdown
Contributor Author

Code review

Found 5 issues:

  1. Backend size cap bypassed when hasGeo=true triggers the in-memory filter path. The pageable is capped at minOf(size, 100) on line 542, but the in-memory branch at lines 567-568 slices with the raw, uncapped size parameter (val start = page * size). A caller can send GET /api/v1/gallery?hasGeo=true&size=10000 and receive all geo-tagged records in a single response, bypassing the 100-item max. (bug: uncapped size in in-memory filter path)

): Page<GalleryMedia> {
val pageable = PageRequest.of(page, minOf(size, 100))
val needsInMemoryFilter = category != null || decade != null || hasGeo == true
return if (needsInMemoryFilter) {
var filtered: List<GalleryMedia> = repository
.findByStatusAndShowInGalleryTrue(GalleryMediaStatus.ACTIVE)
if (category != null) {
filtered = filtered.filter { it.category == category }
}
if (decade != null) {
val yearRange = parseDecadeRange(decade)
if (yearRange != null) {
filtered = filtered.filter { media -> resolveYear(media) in yearRange }
}
}
if (hasGeo == true) {
filtered = filtered.filter { media ->
media is UserUploadedMedia && media.latitude != null && media.longitude != null
}
}
filtered = filtered.sortedBy { it.displayOrder }
val start = page * size
val end = minOf(start + size, filtered.size)
val pageContent = if (start < filtered.size) filtered.subList(start, end) else emptyList()

  1. flyToCoords state is never cleared after "Show on Map" fires, causing stale fly-to on map remount. flyToCoords is set when clicking "Show on Map" but never reset to null. The deduplication guard lastFlyToRef is local to GalleryMapCanvas and resets to null on every mount. When a user switches from map to grid and back, the component remounts, lastFlyToRef is null again, and the stale flyToCoords re-triggers a fly animation to old coordinates. (bug: stale state across unmount/remount cycle)

const [mapLightboxIndex, setMapLightboxIndex] = useState<number | null>(null);
const [flyToCoords, setFlyToCoords] = useState<{
lat: number;
lng: number;
photoId: string;
} | null>(null);

// Handle flyTo from external source (e.g., "Show on Map" button)
useEffect(() => {
if (
!flyToCoords ||
flyToCoords.photoId === lastFlyToRef.current ||
!mapRef.current
) {
return;
}
lastFlyToRef.current = flyToCoords.photoId;
const photo = photoById.get(flyToCoords.photoId);
// eslint-disable-next-line react-hooks/set-state-in-effect -- popup must sync with flyTo animation trigger
if (photo) setPopupPhoto(photo);
mapRef.current.flyTo({
center: [flyToCoords.lng, flyToCoords.lat],
zoom: MAP_CONFIG.LOCATION_ZOOM,
duration: MAP_CONFIG.ANIMATION_DURATION,
});
}, [flyToCoords, photoById]);

  1. getGalleryMedia uses 30-minute ISR cache even when hasGeo=true serves map data. api-contracts.ts defines MAP_DATA: { cache: "no-store" } with the comment "Map data - needs to be dynamic", but the gallery endpoint in backend-api.ts always uses next: { revalidate: 1800 }. Newly uploaded geo-tagged photos would be invisible on the map for up to 30 minutes. (bug: cache policy contradicts documented MAP_DATA contract in api-contracts.ts)

// Use ISR with 30 minute cache for gallery content
const response = await fetch(endpoint, {
next: { revalidate: 1800 }, // 30 minutes
});

// Map data - needs to be dynamic
MAP_DATA: { cache: "no-store" as const },

  1. GalleryMapPopup falls back to photo.url for <Image src>, which for VIDEO items is a YouTube embed/watch URL. .claude/rules/frontend/gallery-media.md says "Never pass an external VIDEO/AUDIO url or embedUrl to next/image." In practice the backend hasGeo filter only returns UserUploadedMedia, but the component accepts generic MediaItem with no type guard — a future caller passing mixed media types would crash the page. (.claude/rules/frontend/gallery-media.md says "Never pass an external VIDEO/AUDIO url or embedUrl to next/image")

}: GalleryMapPopupProps) {
const imageUrl = photo.thumbnailUrl ?? photo.url;

  1. flyTo silently drops on first map load when "Show on Map" triggers lazy-loaded GalleryMapCanvas. When the user clicks "Show on Map", the parent simultaneously sets flyToCoords and switches to map view. Because GalleryMapCanvas is dynamic() loaded, calling mapRef.current.flyTo() before the Mapbox style has loaded fails silently, and lastFlyToRef is updated (line 139), preventing retry. handleMapLoad (line 90) does not check for a pending flyTo. (bug: race condition between lazy component mount and flyTo side-effect)

// Handle flyTo from external source (e.g., "Show on Map" button)
useEffect(() => {
if (
!flyToCoords ||
flyToCoords.photoId === lastFlyToRef.current ||
!mapRef.current
) {
return;
}
lastFlyToRef.current = flyToCoords.photoId;
const photo = photoById.get(flyToCoords.photoId);
// eslint-disable-next-line react-hooks/set-state-in-effect -- popup must sync with flyTo animation trigger
if (photo) setPopupPhoto(photo);
mapRef.current.flyTo({
center: [flyToCoords.lng, flyToCoords.lat],
zoom: MAP_CONFIG.LOCATION_ZOOM,
duration: MAP_CONFIG.ANIMATION_DURATION,
});
}, [flyToCoords, photoById]);

const handleMapLoad = useCallback(() => {
setMapReady(true);
const b = mapRef.current?.getMap().getBounds();
if (b) {

🤖 Generated with Claude Code

- If this code review was useful, please react with 👍. Otherwise, react with 👎.

- Cap in-memory page size in queryActiveMedia (was using raw uncapped size)
- Clear flyToCoords state when switching away from map view (prevents stale flyTo on remount)
- Guard flyTo effect with mapReady to avoid race condition on first map load
- Use CacheConfig.MAP_DATA (no-store) for hasGeo requests instead of 30-min ISR
- Use thumbnailUrl only in GalleryMapPopup (no fallback to url which could be a VIDEO embed)
@joaquimscosta
Copy link
Copy Markdown
Contributor Author

All 5 issues addressed in commit b253cb5:

  1. Backend size capqueryActiveMedia now uses cappedSize (minOf(size, 100)) for the in-memory slicing path, matching searchActiveMedia which was already correct.

  2. Stale flyToCoordshandleViewChange now clears flyToCoords to null when switching away from map view, preventing stale fly-to animations on remount.

  3. ISR cache for map datagetGalleryMedia now uses CacheConfig.MAP_DATA (no-store) when hasGeo=true, and CacheConfig.GALLERY (30-min ISR) otherwise.

  4. GalleryMapPopup VIDEO URL — Changed photo.thumbnailUrl ?? photo.url to photo.thumbnailUrl ?? null, following the gallery-media golden rule. The placeholder image handles the null case.

  5. flyTo race condition — Added mapReady as a dependency and guard condition in the flyTo useEffect. The effect now waits for the Mapbox style to load before attempting flyTo, and re-runs when mapReady transitions to true.

All 270 frontend unit tests and 7 backend geo-filter integration tests pass. TypeScript compilation clean.

…wrappers, extract bounds logic

Simplify gallery map view components and utilities:

- Reorder imports (types before dynamic imports)
- Remove unnecessary arrow wrapper around callback
- Replace nested ternary with parseView() helper
- Extract shared bounds-reading logic into syncBounds callback
- Simplify cluster property casting
- Remove redundant intermediate variables
- Remove dead null fallbacks

All 270 tests pass (56 gallery-specific).
Update plan submodule reference.
@joaquimscosta joaquimscosta merged commit 5216be1 into main Feb 27, 2026
@joaquimscosta joaquimscosta deleted the feat/gallery-map-view branch February 27, 2026 00:17
joaquimscosta added a commit that referenced this pull request Mar 2, 2026
…alidation

Replace legacy force-static/revalidate pattern with "use cache" + cacheLife()
for next-generation on-demand ISR with three cache profiles:
- max: infinite cache (static pages: about, contact, privacy, terms, contribute)
- content: 5min stale, 1hr revalidate, 24hr expire (directory, history, people slugs)
- entry: 1min stale, 30min revalidate, 24hr expire (homepage, gallery, directory detail)
- longLived: 10min stale, 2hr revalidate, 7d expire (history/people index)

Add tag-based cache invalidation for directory/gallery pages. Extend
/api/revalidate to support both path-based and tag-based revalidation:
- revalidateTag("gallery") for all gallery content
- revalidateTag("category:hotels") for category-specific content
- revalidateTag("entry:heritage:chiesa") for individual entries

Update .env.local.example to document REVALIDATE_SECRET configuration for
frontend cache invalidation authentication.

All pages verified with full Playwright sweep. ESLint and TypeScript checks pass.

References: #83
joaquimscosta added a commit that referenced this pull request Mar 2, 2026
…1) (#85)

* perf(web): homepage ISR fix and static asset cache headers

- Replace force-dynamic with revalidate=1800 on homepage (T-02)
- Add immutable Cache-Control header for /_next/static/ assets (T-03)
- Add lightningcss-darwin-x64 for local build compatibility

* perf(web): sharp image optimization with AVIF and Debian slim Docker

- Add sharp as production dependency for native image processing (T-04)
- Switch Dockerfile from node:20-alpine to node:22-slim for glibc compat (T-05)
- Configure AVIF format with WebP fallback for ~50% smaller images (T-06)

* perf(web): enable React Compiler for automatic memoization

- Add babel-plugin-react-compiler as devDependency (T-07)
- Set reactCompiler: true in next.config.ts
- Healthcheck confirmed 439/439 components compatible

* feat(infra): add Cloudflare Terraform provider with DNS and R2 resources

Add Cloudflare provider v5 to manage DNS zone, 17 DNS records, and R2
media bucket as code. All resources imported from existing Cloudflare
dashboard config. CI/CD workflow updated with TF_VAR_cloudflare_* env
vars from GitHub secrets.

* perf(web): migrate to Next.js 16 cacheLife API for granular cache invalidation

Replace legacy force-static/revalidate pattern with "use cache" + cacheLife()
for next-generation on-demand ISR with three cache profiles:
- max: infinite cache (static pages: about, contact, privacy, terms, contribute)
- content: 5min stale, 1hr revalidate, 24hr expire (directory, history, people slugs)
- entry: 1min stale, 30min revalidate, 24hr expire (homepage, gallery, directory detail)
- longLived: 10min stale, 2hr revalidate, 7d expire (history/people index)

Add tag-based cache invalidation for directory/gallery pages. Extend
/api/revalidate to support both path-based and tag-based revalidation:
- revalidateTag("gallery") for all gallery content
- revalidateTag("category:hotels") for category-specific content
- revalidateTag("entry:heritage:chiesa") for individual entries

Update .env.local.example to document REVALIDATE_SECRET configuration for
frontend cache invalidation authentication.

All pages verified with full Playwright sweep. ESLint and TypeScript checks pass.

References: #83

* feat(infra): add revalidation secret infrastructure for frontend cache invalidation

- Create GCP Secret Manager secret (revalidate_secret)
- Grant IAM access to both backend and frontend Cloud Run services
- Inject REVALIDATE_SECRET env var into backend and frontend Cloud Run services
- Backend uses secret to call frontend revalidation endpoint with authentication
- Frontend validates incoming revalidation requests using shared secret

This enables backend services (e.g., gallery media uploads, content updates) to invalidate the frontend's Next.js cache without exposing the cache invalidation endpoint publicly.

* fix(infra,web): add missing IAM depends_on and fix footer imports

- Add missing depends_on for revalidation and resend_api_key IAM bindings on frontend Cloud Run service (prevents race condition during Terraform apply)
- Move COPYRIGHT_YEAR constant after imports in footer component (fixes import ordering)
- Update misleading 'Build-time constant' comment to 'Module-level constant' (reflects actual evaluation timing in use client component)

Addresses code review findings from pragmatic-code-review.

* fix(web,infra): address PR review findings — cacheTag, DNS proxy, generateStaticParams

- Add missing cacheTag(`photo:${id}`) to photo detail page for on-demand
  cache invalidation, matching the pattern in directory entry detail pages
- Fix _domainconnect DNS CNAME record to use proxied=false since Cloudflare
  does not support proxying underscore-prefixed service records
- Restore generateStaticParams to history/[slug] page for build-time
  pre-rendering consistency with people/[slug] page
- Update .claude/rules/frontend/app-router.md to reflect ISR → cacheLife migration

Follow-up: #86 tracks backend tag-based revalidation support

* fix(web): remove generateStaticParams from history/[slug] — no sub-pages exist

The history category has no sub-pages in the Velite content (only _meta.yaml),
so generateStaticParams returns an empty array which Next.js 16 rejects with
EmptyGenerateStaticParamsError when using "use cache". The people category
correctly has generateStaticParams because it has actual sub-pages.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant