fix: Multi-image mosaic positioning and POI type unification#185
Conversation
Fixes critical issues introduced in PR #182 where the multi-image mosaic was incorrectly rendering inside the Info tab instead of at the top of the sidebar, and media handling was not unified across POI types. **Fixed Issues:** - Mosaic now renders at sidebar top (between header and tabs) for all POIs - Removed duplicate media rendering from ReadOnlyView component - Unified media handling: destinations, linear features, and virtual POIs now use identical Mosaic/Lightbox code path - Added primary image indicators (grey star in mosaic, gold badge in lightbox) - Fixed lightbox navigation when setting primary image (stays on same image) - Added event emission for map marker thumbnail updates - Fixed pending badge async updates with event-driven system - Fixed moderation count badge updates on upload - Standardized button positioning (delete: bottom-left, add/set-primary: bottom-right) - Increased caption length limit from 200 to 2000 characters (migration 017) - Fixed moderation queue to show poi_media table items - Fixed event listener thrashing with stable dependencies **Database:** - Migration 017: Increase poi_media.caption length to 2000 chars **Technical Changes:** - Removed showImage parameter from ReadOnlyView/EditView - Media state management now at Sidebar component level only - Linear features now use same Mosaic component as destinations - Event system: poi-media-updated, poi-updated, moderation-count-changed - Inline refresh logic in event handlers to prevent stale closures **Related:** - Addresses issues discovered during PR #182 review - Creates issue #184 for future POI type unification refactor Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a robust media moderation workflow and a dual-strategy Google OAuth implementation to handle conditional Google Drive access for administrators. Key updates include the addition of media management endpoints (delete, set-primary), the integration of photo submissions into the moderation service, and a refactored frontend featuring a mosaic gallery and an enhanced lightbox with administrative actions. Feedback highlights critical security and stability improvements: specifically, the removal of a hardcoded admin email in favor of environment variables, implementing error handling for JSON parsing of OAuth credentials, and masking internal error details in API responses. Furthermore, it is advised to wrap media deletion logic in a database transaction to maintain referential integrity and to re-enable React.StrictMode to ensure frontend code quality.
I am having trouble creating individual review comments. Click here to see my feedback.
backend/routes/auth.js (7)
Hardcoding a personal email address as a fallback for administrative access is a security risk and violates the repository's configuration standards. This should be strictly loaded from environment variables without a default that points to a specific user. Additionally, relying on email strings for authorization is brittle; consider using the role field from the user record if available.
const ADMIN_EMAIL = process.env.ADMIN_EMAIL || '';
References
- No hardcoded credentials — use environment files or mounted secrets (link)
backend/routes/auth.js (23-24)
Parsing oauth_credentials without a try-catch block is risky. If the database contains an invalid JSON string or an empty string, JSON.parse will throw an exception and crash the request handler. Consider wrapping this in a safe utility function or a try-catch block to ensure application stability.
backend/routes/admin.js (3935)
Exposing error.message directly to the client in a 500 response can leak sensitive information about the backend implementation or database structure. It is safer to log the detailed error on the server and return a generic message to the user.
res.status(500).json({ error: 'Failed to save edits' });
backend/server.js (1273-1299)
The database operations for deleting a media record and updating the POI's has_primary_image flag are not wrapped in a transaction. If the second query fails after the deletion, the POI's state will be inconsistent with its actual media content. Consider using BEGIN and COMMIT to ensure atomicity, similar to the implementation in the set-primary route.
frontend/src/main.jsx (8-12)
The removal of React.StrictMode reduces the ability of the development environment to catch potential issues like side effects or deprecated API usage. Unless there is a specific technical reason for its removal (such as a conflict with a third-party library), it is recommended to keep the application wrapped in StrictMode.
The image navigation buttons (grey chevrons on mobile) were accidentally removed when unifying media handling across POI types. These buttons allow users to navigate between POIs in the list on mobile devices. Fixes failing UI tests: - should navigate POIs using grey chevron buttons - should prevent double navigation on rapid button clicks
Removed redundant comments that restate what the code obviously does. Gourmand correctly flagged these as adding no information beyond the code itself.
Deleted 12 AI-generated status report files that Gourmand correctly identified as providing zero value. These files clutter the repository root and duplicate information already available in git history and the issue tracker. Per Gourmand guidance: - Progress history → git commit messages - Current status → issue tracker - Documentation → README.md and docs/ Files removed: - DEPLOYMENT_GUIDE.md - DEPLOYMENT_VERIFICATION_CHECKLIST.md - EXEC_SUMMARY.md - HANDOFF_SUMMARY.md - NEXT_STEPS.md - PACKAGE_STRUCTURE.md - PROD_FIX_QUICKREF.md - PROD_ISSUE_FLOWCHART.md - PROD_TROUBLESHOOT.md - PRODUCTION_INCIDENT_README.md - README_PRODUCTION.md - TROUBLESHOOTING_PACKAGE_INDEX.md
Fixes all 5 issues identified by Gemini code review: 1. Remove hardcoded admin email fallback (security) - Changed ADMIN_EMAIL fallback from personal email to empty string - Forces proper environment variable configuration 2. Add try-catch for JSON.parse of oauth_credentials (stability) - Prevents crash on invalid JSON in database - Logs error and continues with null credentials 3. Remove internal error details from 500 responses (security) - Prevents leaking implementation details to clients - Still logs full error server-side for debugging 4. Wrap media delete in transaction (data integrity) - Uses BEGIN/COMMIT/ROLLBACK like set-primary endpoint - Ensures has_primary_image flag stays consistent with media state - Prevents orphaned POI state if UPDATE fails after DELETE 5. Re-enable React.StrictMode (code quality) - Restores development-time checks for side effects - Helps catch deprecated API usage and other issues
Addresses Gatehouse advisory about image server deletion happening after DB commit. Now returns honest status when partial success occurs: - 200 OK: Both database and image server deletion succeeded - 202 Accepted: Database updated, image server cleanup pending Changes: - Track image server deletion success/failure - Return 202 with warning when image server delete fails - Log orphaned asset IDs for manual cleanup - Add TODO comment for background cleanup job This accepts eventual consistency as a design decision: - Database (source of truth) is always consistent - Image server failures don't block user operations - Orphaned images can be cleaned up later (manual or automated) - Clients are informed about partial success via 202 status
1 participant
Summary
Fixes critical bugs introduced in PR #182 where the multi-image mosaic was rendering in the wrong location and media handling wasn't unified across POI types.
Fixes from PR #182:
Changes
Core Fixes
UI Improvements
Event System
Database
Bug Fixes
Related Issues
Test Plan
Technical Debt
This PR unifies the media handling across POI types but does not address the root architectural issue (3800+ line Sidebar with duplicate code paths). Issue #184 created to track proper inheritance/composition refactor.
Deployment Notes
🤖 Generated with Claude Code