Merge pull request #455 from the-luap/fix/photo-dimensions-and-default-fit

fix(import): capture photo dimensions in fileWatcher + s3AutoImporter (#447)

backend/migrations/core/095_add_customer_portal_flag.js

@@ -1,5 +1,5 @@
 /**
- * Migration 094: Add `customerPortal` to feature_flags.
+ * Migration 095: Add `customerPortal` to feature_flags.
  *
  * The customer portal (#354) is the foundation feature for the
  * customer-side UI surface — login, dashboard, profile, password reset,

backend/migrations/core/096_backfill_photo_dimensions_v2.js

@@ -0,0 +1,102 @@
+/**
+ * Migration: Backfill photo dimensions (v2)
+ *
+ * Re-runs the dimension backfill from migration 064 for any rows that are
+ * still NULL. Migration 064 only ran once at upgrade time; new photos
+ * imported via fileWatcher.js or s3AutoImporter.js between then and now
+ * had their width/height columns left NULL because those code paths did
+ * not capture metadata on insert. This PR fixes both writers, but
+ * pre-existing rows still need a backfill — that is what this does.
+ *
+ * Without dimensions, MasonryGalleryLayout falls back to a hard-coded
+ * 800×600 default, which is why every card in masonry mode looks like
+ * the same 4:3 box (#447).
+ *
+ * Local-fs only — S3 deployments cannot read source objects in a
+ * migration without instantiating the storage backend. Those
+ * deployments rely on the writer fix in s3AutoImporter.js for new
+ * photos and can run a one-shot script if a backfill is needed.
+ */
+
+const path = require('path');
+const fs = require('fs');
+
+exports.up = async function(knex) {
+  const hasWidth = await knex.schema.hasColumn('photos', 'width');
+  const hasHeight = await knex.schema.hasColumn('photos', 'height');
+  if (!hasWidth || !hasHeight) {
+    console.log('[Migration 090] width/height columns not present, skipping');
+    return;
+  }
+
+  const backend = (process.env.STORAGE_BACKEND || 'local').toLowerCase();
+  if (backend !== 'local') {
+    console.log(`[Migration 090] STORAGE_BACKEND=${backend} — backfill skipped (S3 deployments not supported in-migration)`);
+    return;
+  }
+
+  const storagePath = process.env.STORAGE_PATH || path.join(__dirname, '../../../storage');
+
+  const photos = await knex('photos')
+    .where(function () {
+      this.whereNull('width').orWhereNull('height');
+    })
+    .andWhere(function () {
+      // Skip videos — sharp can't handle them; they need ffprobe.
+      this.where('media_type', '!=', 'video').orWhereNull('media_type');
+    })
+    .select('id', 'path', 'filename');
+
+  if (photos.length === 0) {
+    console.log('[Migration 090] no photos missing dimensions');
+    return;
+  }
+
+  console.log(`[Migration 090] backfilling ${photos.length} photos`);
+
+  let sharp;
+  try {
+    sharp = require('sharp');
+  } catch (err) {
+    console.error('[Migration 090] sharp unavailable, skipping:', err.message);
+    return;
+  }
+
+  let updated = 0;
+  let failed = 0;
+  for (const photo of photos) {
+    try {
+      if (!photo.path) {
+        failed++;
+        continue;
+      }
+      const fullPath = path.join(storagePath, 'events/active', photo.path);
+      if (!fs.existsSync(fullPath)) {
+        failed++;
+        continue;
+      }
+      const metadata = await sharp(fullPath).metadata();
+      if (metadata.width && metadata.height) {
+        await knex('photos').where('id', photo.id).update({
+          width: metadata.width,
+          height: metadata.height,
+        });
+        updated++;
+        if (updated % 100 === 0) {
+          console.log(`[Migration 090] ${updated}/${photos.length}`);
+        }
+      } else {
+        failed++;
+      }
+    } catch (err) {
+      console.error(`[Migration 090] photo ${photo.id}: ${err.message}`);
+      failed++;
+    }
+  }
+
+  console.log(`[Migration 090] done — ${updated} updated, ${failed} skipped`);
+};
+
+exports.down = async function() {
+  // Data-only migration; no rollback action.
+};

backend/src/routes/customerAuth.js

@@ -48,6 +48,12 @@ const TOKEN_TTL_SECONDS = 24 * 60 * 60; // mirrors admin tokens
 
 // ---- login -------------------------------------------------------------
 
+// The customerPortal feature flag deliberately does NOT gate this route.
+// Flipping the master toggle off in Settings → Features hides the admin
+// UI surface (sidebar entry, /admin/customers page) but does not revoke
+// access for customers who already accepted an invitation. To lock out
+// existing customers, deactivate their accounts individually
+// (customer_accounts.is_active = false) — which IS enforced below.
 router.post('/login', [
   body('email').isEmail().normalizeEmail().withMessage('Valid email is required'),
   body('password').isString().notEmpty(),

backend/src/services/customerAccountsService.js

@@ -220,6 +220,12 @@ async function acceptInvitation({ token, name, password, profile }) {
       country_code: merged.country_code || null,
       password_hash: passwordHash,
       is_active: formatBoolean(true),
+      // must_change_password is decorative today — accept-invite always
+      // sets a customer-chosen password, so this flag is never true and
+      // customerAuth doesn't read it. TODO when we ship an "admin
+      // pre-loads a temporary password" flow: surface a code in the
+      // login response (mirroring adminAuth's MUST_CHANGE_PASSWORD) and
+      // add a /change-password gate to customerAuth.
       must_change_password: formatBoolean(false),
       // Leave password_changed_at NULL on initial accept. Setting it here
       // creates a millisecond/second-rounding race with the JWT issued

backend/src/services/fileWatcher.js

@@ -1,6 +1,7 @@
 const chokidar = require('chokidar');
 const path = require('path');
 const fs = require('fs').promises;
+const sharp = require('sharp');
 const { db } = require('../database/db');
 const { formatBoolean } = require('../utils/dbCompat');
 const { generateThumbnail, generateVideoPlaceholder } = require('./imageProcessor');
@@ -91,7 +92,23 @@ async function processNewPhoto(filePath) {
   // Calculate relative thumbnail path
   const relativeThumbPath = thumbnailPath; // thumbnailPath is already relative to storage root
   const mimeType = detectedMime || (isVideo ? 'video/mp4' : 'image/jpeg');
-  
+
+  // Capture image dimensions so aspect-aware layouts (masonry / mosaic /
+  // justified) can size each card to the photo's real proportions
+  // instead of the 800×600 fallback in MasonryGalleryLayout (#447).
+  // Skip videos — those would need ffprobe.
+  let dimensions = null;
+  if (!isVideo) {
+    try {
+      const metadata = await sharp(filePath).metadata();
+      if (metadata.width && metadata.height) {
+        dimensions = { width: metadata.width, height: metadata.height };
+      }
+    } catch (err) {
+      logger.debug(`Could not read image dimensions for ${filename}: ${err.message}`);
+    }
+  }
+
   // Check if photo already exists (by filename or path, to handle replacements)
   const existingPhoto = await db('photos')
     .where({ event_id: event.id })
@@ -110,7 +127,8 @@ async function processNewPhoto(filePath) {
       thumbnail_path: relativeThumbPath,
       type: isVideo ? 'video' : photoType,
       size_bytes: stats.size,
-      mime_type: mimeType
+      mime_type: mimeType,
+      ...(dimensions && { width: dimensions.width, height: dimensions.height })
     }).returning('id');
     const photoId = insertResult[0]?.id || insertResult[0];
 

backend/src/services/imageProcessor.js

@@ -15,7 +15,13 @@ sharp.concurrency(2); // Limit concurrent operations
 // Default thumbnail settings
 const DEFAULT_THUMBNAIL_WIDTH = 300;
 const DEFAULT_THUMBNAIL_HEIGHT = 300;
-const DEFAULT_THUMBNAIL_FIT = 'cover'; // 'cover' for square crops
+// 'inside' preserves the source aspect ratio (output ≤ width × height).
+// This is the right default for masonry / mosaic / justified layouts —
+// the gallery sizes each card from photo.width/height and renders the
+// thumbnail with object-cover, so a thumb that already matches the
+// source aspect doesn't get re-cropped (#447). Admins who want
+// uniform 1:1 grid tiles can switch to 'cover' in the thumbnail settings.
+const DEFAULT_THUMBNAIL_FIT = 'inside';
 const DEFAULT_THUMBNAIL_QUALITY = 85;
 const DEFAULT_THUMBNAIL_FORMAT = 'jpeg';
 

backend/src/services/s3AutoImporter.js

@@ -17,9 +17,11 @@
 
 const path = require('path');
 const mime = require('mime-types');
+const sharp = require('sharp');
 const { db } = require('../database/db');
 const { formatBoolean } = require('../utils/dbCompat');
 const { getStorage } = require('./storage');
+const { withLocalCopy } = require('./imageProcessor');
 const logger = require('../utils/logger');
 
 const POLL_INTERVAL_MS = parseInt(process.env.STORAGE_AUTO_IMPORT_INTERVAL_MS || `${5 * 60 * 1000}`, 10);
@@ -98,6 +100,26 @@ async function processEvent(event, storage) {
       const isVideo = mimeType.startsWith('video/');
       if (!isImage && !isVideo) continue;
 
+      // Capture image dimensions so aspect-aware layouts (masonry /
+      // mosaic / justified) can size each card to the photo's real
+      // proportions instead of the 800×600 fallback (#447). Materialize
+      // a tmp local copy via withLocalCopy — withLocalCopy handles the
+      // S3 download + cleanup. Skip videos (would need ffprobe).
+      let dimensions = null;
+      if (isImage) {
+        try {
+          dimensions = await withLocalCopy(entry.key, async (localPath) => {
+            const metadata = await sharp(localPath).metadata();
+            if (metadata.width && metadata.height) {
+              return { width: metadata.width, height: metadata.height };
+            }
+            return null;
+          });
+        } catch (err) {
+          logger.debug(`[s3AutoImporter] could not read dimensions for ${entry.key}: ${err.message}`);
+        }
+      }
+
       try {
         const insertResult = await db('photos').insert({
           event_id: event.id,
@@ -110,6 +132,7 @@ async function processEvent(event, storage) {
           mime_type: mimeType,
           source_origin: 'managed',
           uploaded_at: new Date().toISOString(),
+          ...(dimensions && { width: dimensions.width, height: dimensions.height }),
         }).returning('id');
         const photoId = insertResult[0]?.id || insertResult[0];
 

frontend/src/features/settings/tabs/ThumbnailsTab.tsx

@@ -229,6 +229,9 @@ export const ThumbnailsTab: React.FC = () => {
           <p className="text-xs text-neutral-500 dark:text-neutral-400 mt-1">
             {t('settings.thumbnails.fitHelp', 'How images are resized to fit the thumbnail dimensions. "Cover" crops to fill, "Contain" fits within bounds.')}
           </p>
+          <p className="text-xs text-neutral-500 dark:text-neutral-400 mt-2">
+            {t('settings.thumbnails.fitRecommendation', 'Recommendation: use "Inside" for masonry / mosaic / justified layouts (preserves aspect ratio); "Cover" for uniform 1:1 grid tiles.')}
+          </p>
         </div>
       </Card>
 

frontend/src/i18n/locales/de.json

@@ -1246,7 +1246,8 @@
       "fit_contain": "Einpassen (innerhalb)",
       "fit_fill": "Strecken",
       "fit_inside": "Innen (verkleinern)",
-      "fit_outside": "Außen (vergrößern)"
+      "fit_outside": "Außen (vergrößern)",
+      "fitRecommendation": "Empfehlung: „Inside\" für Masonry-/Mosaic-/Justified-Layouts (bewahrt das Seitenverhältnis), „Cover\" für gleichförmige 1:1-Kacheln."
     },
     "categories": {
       "title": "Kategorien",

frontend/src/i18n/locales/en.json

@@ -952,7 +952,8 @@
       "fit_contain": "Contain (fit within)",
       "fit_fill": "Fill (stretch)",
       "fit_inside": "Inside (shrink to fit)",
-      "fit_outside": "Outside (expand to cover)"
+      "fit_outside": "Outside (expand to cover)",
+      "fitRecommendation": "Recommendation: use \"Inside\" for masonry / mosaic / justified layouts (preserves aspect ratio); \"Cover\" for uniform 1:1 grid tiles."
     },
     "seo": {
       "title": "SEO & Robots",

frontend/src/i18n/locales/fr.json

@@ -967,7 +967,8 @@
       "fit_cover": "Recouvrir (recadrer pour remplir)",
       "fit_contain": "Contenir (ajuster dans les limites)",
       "fit_inside": "Intérieur (réduire pour ajuster)",
-      "fit_outside": "Extérieur (agrandir pour recouvrir)"
+      "fit_outside": "Extérieur (agrandir pour recouvrir)",
+      "fitRecommendation": "Recommandation : utilisez « Inside » pour les mises en page masonry/mosaic/justified (préserve les proportions) ; « Cover » pour des tuiles 1:1 uniformes."
     },
     "seo": {
       "title": "SEO et Robots",

frontend/src/i18n/locales/nl.json

@@ -952,7 +952,8 @@
       "fit_contain": "Contain (passend binnen)",
       "fit_fill": "Vullen (uitrekken)",
       "fit_inside": "Binnenkant (verkleinen om te passen)",
-      "fit_outside": "Buitenkant (vergroten om te bedekken)"
+      "fit_outside": "Buitenkant (vergroten om te bedekken)",
+      "fitRecommendation": "Aanbeveling: gebruik \"Inside\" voor masonry/mosaic/justified-lay-outs (behoudt beeldverhouding); \"Cover\" voor uniforme 1:1-tegels."
     },
     "seo": {
       "title": "SEO & Robots",

frontend/src/i18n/locales/pt.json

@@ -969,7 +969,8 @@
       "fit_contain": "Contain (ajustar ao espaço)",
       "fit_fill": "Fill (esticar)",
       "fit_inside": "Inside (encolher para caber)",
-      "fit_outside": "Outside (expandir para cobrir)"
+      "fit_outside": "Outside (expandir para cobrir)",
+      "fitRecommendation": "Recomendação: use \"Inside\" para layouts masonry/mosaic/justified (preserva proporção); \"Cover\" para tiles 1:1 uniformes."
     },
     "seo": {
       "title": "SEO e Robots",

frontend/src/i18n/locales/ru.json

@@ -1027,7 +1027,8 @@
       "fit_contain": "Вписывание (внутри)",
       "fit_fill": "Растягивание",
       "fit_inside": "Внутри (уменьшить)",
-      "fit_outside": "Снаружи (увеличить)"
+      "fit_outside": "Снаружи (увеличить)",
+      "fitRecommendation": "Рекомендация: «Inside» для масонри/мозаики/выровненных раскладок (сохраняет пропорции); «Cover» для одинаковых плиток 1:1."
     },
     "photoDimensions": {
       "title": "Размеры фотографий",

frontend/src/pages/admin/EventsListPage.tsx

@@ -104,17 +104,6 @@ export const EventsListPage: React.FC = () => {
     setPage(1);
   }, [statusFilter, debouncedSearchTerm]);
 
-  // Clamp the active page when the result count shrinks (#442 — bulk
-  // delete of an entire page would leave the user on a now-empty
-  // page=N where N > totalPages, with no auto-correction). Triggers
-  // after each successful refetch when totalPages drops below the
-  // current page (bulk delete, individual delete, archive, anything).
-  useEffect(() => {
-    if (data?.pagination && page > data.pagination.totalPages) {
-      setPage(Math.max(1, data.pagination.totalPages));
-    }
-  }, [data?.pagination, page]);
-
   // Close dropdown when clicking outside
   useEffect(() => {
     const handleClickOutside = (event: MouseEvent) => {
@@ -157,6 +146,20 @@ export const EventsListPage: React.FC = () => {
     placeholderData: (prev) => prev,
   });
 
+  // Clamp the active page when the result count shrinks (#442 — bulk
+  // delete of an entire page would leave the user on a now-empty
+  // page=N where N > totalPages, with no auto-correction). Triggers
+  // after each successful refetch when totalPages drops below the
+  // current page (bulk delete, individual delete, archive, anything).
+  // Must live AFTER the useQuery above so `data` is in scope — the
+  // original placement at the top of the component caused a TDZ
+  // ReferenceError on /admin/events that crashed the page (#454).
+  useEffect(() => {
+    if (data?.pagination && page > data.pagination.totalPages) {
+      setPage(Math.max(1, data.pagination.totalPages));
+    }
+  }, [data?.pagination, page]);
+
   // Aggregate counters come from the dashboard stats endpoint so the cards
   // and the "All (N)" filter button always reflect global totals, not the
   // currently visible page.