refactor(trips): spool trip cover upload through storage

server/src/nest/trips/trips.controller.ts

@@ -20,11 +20,10 @@ import { FileInterceptor } from '@nestjs/platform-express';
 import { isDemoWriteBlocked, DEMO_WRITE_ERROR } from '../common/demo-write';
 import { RuntimeEnvService } from '../app-config/runtime-env.service';
 import type { Request, Response } from 'express';
-import { diskStorage } from 'multer';
+import type { Options } from 'multer';
 import path from 'path';
-import fs from 'fs';
-import { v4 as uuidv4 } from 'uuid';
 import type { ActiveTripResponse } from '@trek/shared';
+import { StorageService } from '../storage/storage.service';
 import type { User } from '../../types';
 import { TripsService } from './trips.service';
 import { JwtAuthGuard } from '../auth/jwt-auth.guard';
@@ -38,23 +37,17 @@ import { UnsplashService } from '../unsplash/unsplash.service';
 import { CalendarService } from '../calendar/calendar.service';
 import { TripReadModelService } from '../trip-read-model/trip-read-model.service';
 
-const MAX_COVER_SIZE = 20 * 1024 * 1024;
+export const MAX_COVER_SIZE = 20 * 1024 * 1024;
+// Still needed by the Unsplash cover download (a raw-fs writer until the
+// caches/downloads slice); the multer destination moved to the storage spool.
 const coversDir = path.join(__dirname, '../../../uploads/covers');
-const COVER_UPLOAD = {
-  storage: diskStorage({
-    destination: (_req, _file, cb) => {
-      if (!fs.existsSync(coversDir)) fs.mkdirSync(coversDir, { recursive: true });
-      cb(null, coversDir);
-    },
-    filename: (_req, file, cb) => cb(null, `${uuidv4()}${path.extname(file.originalname)}`),
-  }),
-  limits: { fileSize: MAX_COVER_SIZE },
-  fileFilter: (_req: Request, file: Express.Multer.File, cb: (err: Error | null, accept: boolean) => void) => {
-    const ext = path.extname(file.originalname).toLowerCase();
-    const allowed = ['.jpg', '.jpeg', '.png', '.gif', '.webp'];
-    if (file.mimetype.startsWith('image/') && !file.mimetype.includes('svg') && allowed.includes(ext)) cb(null, true);
-    else cb(new Error('Only jpg, png, gif, webp images allowed'), false);
-  },
+// Consumed by trips.module.ts's MulterModule factory. Quirk preserved on
+// purpose: a plain Error without statusCode maps to 500, not 400 (parity).
+export const TRIP_COVER_FILE_FILTER: Options['fileFilter'] = (_req, file, cb) => {
+  const ext = path.extname(file.originalname).toLowerCase();
+  const allowed = ['.jpg', '.jpeg', '.png', '.gif', '.webp'];
+  if (file.mimetype.startsWith('image/') && !file.mimetype.includes('svg') && allowed.includes(ext)) cb(null, true);
+  else cb(new Error('Only jpg, png, gif, webp images allowed'));
 };
 
 const toDateStr = (d: Date) => d.toISOString().slice(0, 10);
@@ -74,7 +67,7 @@ const addDays = (d: Date, n: number) => { const r = new Date(d); r.setDate(r.get
 export class TripsController {
   // calendar last: the hand-wired construction sites in the tests stay positional,
   // so a new dependency does not touch the ones that never reach the ICS route.
-  constructor(private readonly trips: TripsService, private readonly audit: AuditService, private readonly env: RuntimeEnvService, private readonly unsplash: UnsplashService, private readonly calendar: CalendarService, private readonly readModel: TripReadModelService) {}
+  constructor(private readonly trips: TripsService, private readonly audit: AuditService, private readonly env: RuntimeEnvService, private readonly unsplash: UnsplashService, private readonly calendar: CalendarService, private readonly readModel: TripReadModelService, private readonly storage: StorageService) {}
 
   @Get()
   list(@CurrentUser() user: User, @Query('archived') archived?: string) {
@@ -194,8 +187,8 @@ export class TripsController {
   }
 
   @Post(':id/cover')
-  @UseInterceptors(FileInterceptor('cover', COVER_UPLOAD))
-  cover(@CurrentUser() user: User, @Param('id') id: string, @UploadedFile() file: Express.Multer.File | undefined) {
+  @UseInterceptors(FileInterceptor('cover'))
+  async cover(@CurrentUser() user: User, @Param('id') id: string, @UploadedFile() file: Express.Multer.File | undefined) {
     if (isDemoWriteBlocked(this.env, user.email)) {
       throw new HttpException(DEMO_WRITE_ERROR, 403);
     }
@@ -213,6 +206,9 @@ export class TripsController {
     if (!file) {
       throw new HttpException({ error: 'No image uploaded' }, 400);
     }
+    // Commit the spooled upload to its final storage location (atomic
+    // same-volume rename) before anything references the final path.
+    await this.storage.put('covers', file.filename, { tmpPath: file.path });
     this.trips.deleteOldCover(trip.cover_image);
     const coverUrl = `/uploads/covers/${file.filename}`;
     this.trips.updateCoverImage(id, coverUrl);

server/src/nest/trips/trips.module.ts

@@ -27,11 +27,28 @@ import { TripMembersModule } from '../trip-members/trip-members.module';
 import { TripReadModelModule } from '../trip-read-model/trip-read-model.module';
 import { AddonsModule } from '../addons/addons.module';
 import { McpSharedModule } from '../mcp-shared/mcp-shared.module';
+import { MulterModule } from '@nestjs/platform-express';
+import { StorageModule } from '../storage/storage.module';
+import { StorageService } from '../storage/storage.service';
+import { buildStorageUploadOptions } from '../storage/storage-upload.factory';
+import { MAX_COVER_SIZE, TRIP_COVER_FILE_FILTER } from './trips.controller';
 
 /** Trips aggregate root (C1 — Phase 3). Uses exact strangler prefixes so it does
  *  not capture the nested sub-domain mounts (collab, files, ...). */
 @Module({
-  imports: [McpSharedModule, TodoModule, PackingModule, FilesModule, ReservationsModule, DaysModule, PermissionsModule, AuditModule, BudgetModule, CollabModule, VacayModule, PlacesModule, AuthModule, AppConfigModule, UnsplashModule, RealtimeModule, PluginGuardsModule, AddonsModule, TripMembershipModule, CalendarModule, AccommodationsModule, TripMembersModule, TripReadModelModule],
+  imports: [
+    MulterModule.registerAsync({
+      imports: [StorageModule],
+      inject: [StorageService],
+      useFactory: (storage: StorageService) =>
+        buildStorageUploadOptions(storage, {
+          category: 'covers',
+          maxSize: MAX_COVER_SIZE,
+          fileFilter: TRIP_COVER_FILE_FILTER,
+        }),
+    }),
+    StorageModule,
+    McpSharedModule, TodoModule, PackingModule, FilesModule, ReservationsModule, DaysModule, PermissionsModule, AuditModule, BudgetModule, CollabModule, VacayModule, PlacesModule, AuthModule, AppConfigModule, UnsplashModule, RealtimeModule, PluginGuardsModule, AddonsModule, TripMembershipModule, CalendarModule, AccommodationsModule, TripMembersModule, TripReadModelModule],
   controllers: [TripsController],
   providers: [TripsService, TripsMcp, TripPromptsMcp, TripsRpc],
   // Exported for FeedsModule (ICS feeds) and PluginsModule (RPC host injection).

server/tests/integration/trips.test.ts

@@ -1279,3 +1279,84 @@ describe('Trip bundle', () => {
     expect(memberView.body.packingItems.map((i: { name: string }) => i.name)).toEqual(['Tent']);
   });
 });
+
+// ─────────────────────────────────────────────────────────────────────────────
+// Cover upload parity (TRIP-P01…P05) — written BEFORE the storage-upload swap.
+// ─────────────────────────────────────────────────────────────────────────────
+
+describe('Trip cover upload parity', () => {
+  const fsMod = require('fs') as typeof import('fs');
+  const pathMod = require('path') as typeof import('path');
+  const FIXTURE_IMG = pathMod.join(__dirname, '../fixtures/small-image.jpg');
+  const coversDir = pathMod.join(__dirname, '../../uploads/covers');
+
+  afterAll(() => {
+    fsMod.rmSync(coversDir, { recursive: true, force: true });
+  });
+
+  it('TRIP-P01 — cover upload returns /uploads/covers/<uuid> and writes the file', async () => {
+    const { user } = createUser(testDb);
+    const trip = createTrip(testDb, user.id);
+
+    const res = await request(app)
+      .post(`/api/trips/${trip.id}/cover`)
+      .set('Cookie', authCookie(user.id))
+      .attach('cover', FIXTURE_IMG, 'cover.png');
+    expect(res.status).toBe(201);
+    expect(res.body.cover_image).toMatch(/^\/uploads\/covers\/[0-9a-f-]{36}\.png$/);
+    const diskName = res.body.cover_image.replace('/uploads/covers/', '');
+    expect(fsMod.existsSync(pathMod.join(coversDir, diskName))).toBe(true);
+  });
+
+  it('TRIP-P02 — re-upload deletes the previous cover file', async () => {
+    const { user } = createUser(testDb);
+    const trip = createTrip(testDb, user.id);
+
+    const first = await request(app)
+      .post(`/api/trips/${trip.id}/cover`)
+      .set('Cookie', authCookie(user.id))
+      .attach('cover', FIXTURE_IMG, 'one.jpg');
+    const firstName = first.body.cover_image.replace('/uploads/covers/', '');
+    expect(fsMod.existsSync(pathMod.join(coversDir, firstName))).toBe(true);
+
+    const second = await request(app)
+      .post(`/api/trips/${trip.id}/cover`)
+      .set('Cookie', authCookie(user.id))
+      .attach('cover', FIXTURE_IMG, 'two.jpg');
+    expect(second.status).toBe(201);
+    expect(fsMod.existsSync(pathMod.join(coversDir, firstName))).toBe(false);
+  });
+
+  it('TRIP-P03 — no file → 400 "No image uploaded"', async () => {
+    const { user } = createUser(testDb);
+    const trip = createTrip(testDb, user.id);
+
+    const res = await request(app)
+      .post(`/api/trips/${trip.id}/cover`)
+      .set('Cookie', authCookie(user.id));
+    expect(res.status).toBe(400);
+    expect(res.body.error).toBe('No image uploaded');
+  });
+
+  it('TRIP-P04 — non-image upload is 500 (plain-Error filter quirk — pinned, do not "fix")', async () => {
+    const { user } = createUser(testDb);
+    const trip = createTrip(testDb, user.id);
+
+    const res = await request(app)
+      .post(`/api/trips/${trip.id}/cover`)
+      .set('Cookie', authCookie(user.id))
+      .attach('cover', Buffer.from('plain text'), { filename: 'doc.txt', contentType: 'text/plain' });
+    expect(res.status).toBe(500);
+  });
+
+  it('TRIP-P05 — unknown trip → 404 "Trip not found"', async () => {
+    const { user } = createUser(testDb);
+
+    const res = await request(app)
+      .post('/api/trips/999999/cover')
+      .set('Cookie', authCookie(user.id))
+      .attach('cover', FIXTURE_IMG);
+    expect(res.status).toBe(404);
+    expect(res.body.error).toBe('Trip not found');
+  });
+});

server/tests/unit/nest/trips.controller.test.ts

@@ -2,9 +2,6 @@ import { describe, it, expect, vi, beforeEach } from 'vitest';
 import type { UnsplashService } from '../../../src/nest/unsplash/unsplash.service';
 import { RuntimeEnvService } from '../../../src/nest/app-config/runtime-env.service';
 import { HttpException } from '@nestjs/common';
-import { INTERCEPTORS_METADATA } from '@nestjs/common/constants';
-import fs from 'fs';
-import path from 'path';
 import type { Request } from 'express';
 
 vi.mock('../../../src/nest/audit/client-ip', () => ({ getClientIp: vi.fn(() => '1.2.3.4') }));
@@ -19,7 +16,7 @@ const unsplashStub = {
   saveUnsplashCover: vi.fn().mockResolvedValue('mock-cover.jpg'),
 } as unknown as UnsplashService;
 
-import { TripsController } from '../../../src/nest/trips/trips.controller';
+import { TripsController, MAX_COVER_SIZE, TRIP_COVER_FILE_FILTER } from '../../../src/nest/trips/trips.controller';
 import type { AuditService } from '../../../src/nest/audit/audit.service';
 import type { TripsService } from '../../../src/nest/trips/trips.service';
 import type { CalendarService } from '../../../src/nest/calendar/calendar.service';
@@ -39,8 +36,9 @@ const audit = { writeAudit } as unknown as AuditService;
 // they were; the two that do pass a stub.
 // calendar and readModel are optional so the call sites that reach neither the
 // ICS route nor the bundle stay as they were; the ones that do pass a stub.
+const storageStub = { put: vi.fn().mockResolvedValue(undefined) } as unknown as import('../../../src/nest/storage/storage.service').StorageService;
 const tc = (s: TripsService, calendar?: Partial<CalendarService>, readModel?: Partial<TripReadModelService>) =>
-  new TripsController(s, audit, new RuntimeEnvService(), unsplashStub, (calendar ?? {}) as CalendarService, (readModel ?? {}) as TripReadModelService);
+  new TripsController(s, audit, new RuntimeEnvService(), unsplashStub, (calendar ?? {}) as CalendarService, (readModel ?? {}) as TripReadModelService, storageStub);
 
 function svc(o: Partial<TripsService> = {}): TripsService {
   return {
@@ -307,24 +305,25 @@ describe('TripsController (parity with the legacy /api/trips route)', () => {
 
   describe('POST /:id/cover', () => {
     const file = { filename: 'abc.jpg' } as Express.Multer.File;
-    it('404 without access, 403 without permission, 404 raw trip, 400 no file, else returns url', () => {
-      expect(thrown(() => tc(svc({ canAccessTrip: vi.fn().mockReturnValue(undefined) })).cover(user, '9', file))).toEqual({ status: 404, body: { error: 'Trip not found' } });
-      expect(thrown(() => tc(svc({ can: vi.fn().mockReturnValue(false) })).cover(user, '9', file))).toEqual({ status: 403, body: { error: 'No permission to change the cover image' } });
-      expect(thrown(() => tc(svc({ getRaw: vi.fn().mockReturnValue(undefined) } as Partial<TripsService>)).cover(user, '9', file))).toEqual({ status: 404, body: { error: 'Trip not found' } });
-      expect(thrown(() => tc(svc({ getRaw: vi.fn().mockReturnValue({ cover_image: null }) } as Partial<TripsService>)).cover(user, '9', undefined))).toEqual({ status: 400, body: { error: 'No image uploaded' } });
+    it('404 without access, 403 without permission, 404 raw trip, 400 no file, else commits + returns url', async () => {
+      expect(await thrownAsync(() => tc(svc({ canAccessTrip: vi.fn().mockReturnValue(undefined) })).cover(user, '9', file))).toEqual({ status: 404, body: { error: 'Trip not found' } });
+      expect(await thrownAsync(() => tc(svc({ can: vi.fn().mockReturnValue(false) })).cover(user, '9', file))).toEqual({ status: 403, body: { error: 'No permission to change the cover image' } });
+      expect(await thrownAsync(() => tc(svc({ getRaw: vi.fn().mockReturnValue(undefined) } as Partial<TripsService>)).cover(user, '9', file))).toEqual({ status: 404, body: { error: 'Trip not found' } });
+      expect(await thrownAsync(() => tc(svc({ getRaw: vi.fn().mockReturnValue({ cover_image: null }) } as Partial<TripsService>)).cover(user, '9', undefined))).toEqual({ status: 400, body: { error: 'No image uploaded' } });
       const deleteOldCover = vi.fn(); const updateCoverImage = vi.fn();
       const s = svc({ getRaw: vi.fn().mockReturnValue({ cover_image: '/old.jpg' }), deleteOldCover, updateCoverImage } as Partial<TripsService>);
-      expect(tc(s).cover(user, '9', file)).toEqual({ cover_image: '/uploads/covers/abc.jpg' });
+      expect(await tc(s).cover(user, '9', file)).toEqual({ cover_image: '/uploads/covers/abc.jpg' });
+      expect(storageStub.put).toHaveBeenCalledWith('covers', 'abc.jpg', { tmpPath: undefined });
       expect(deleteOldCover).toHaveBeenCalledWith('/old.jpg');
       expect(updateCoverImage).toHaveBeenCalledWith('9', '/uploads/covers/abc.jpg');
     });
 
-    it('403 in demo mode for a demo account', () => {
+    it('403 in demo mode for a demo account', async () => {
       const prev = process.env.DEMO_MODE;
       process.env.DEMO_MODE = 'true';
       isDemoEmail.mockReturnValueOnce(true);
       try {
-        expect(thrown(() => tc(svc()).cover(user, '9', file))).toEqual({
+        expect(await thrownAsync(() => tc(svc()).cover(user, '9', file))).toEqual({
           status: 403, body: { error: 'Uploads are disabled in demo mode. Self-host TREK for full functionality.' },
         });
       } finally {
@@ -334,69 +333,15 @@ describe('TripsController (parity with the legacy /api/trips route)', () => {
     });
 
     /**
-     * The multer options are a module-private const wired into the route through
-     * @UseInterceptors, so the handler tests above never reach them: a broken
-     * destination/filename/filter would still let every case in this file pass.
-     * Pulling the interceptor back off the route and reading the multer instance
-     * it built (multer keeps storage, limits and fileFilter verbatim) exercises
-     * the real callbacks instead of asserting an object literal.
+     * The multer options moved into trips.module.ts's MulterModule factory
+     * (storage engine + spool destination are covered by the storage-upload
+     * factory unit suite and TRIP-P01/P02 integration parity); the trips-owned
+     * piece that remains testable here is the exported cover fileFilter.
      */
-    type StorageCb = (err: Error | null, value: string) => void;
-    type CoverUpload = {
-      storage: {
-        getDestination: (req: unknown, file: Express.Multer.File, cb: StorageCb) => void;
-        getFilename: (req: unknown, file: Express.Multer.File, cb: StorageCb) => void;
-      };
-      limits: { fileSize: number };
-      fileFilter: (req: unknown, file: Express.Multer.File, cb: (err: Error | null, accept: boolean) => void) => void;
-    };
-    function coverUpload(): CoverUpload {
-      const [Interceptor] = Reflect.getMetadata(INTERCEPTORS_METADATA, TripsController.prototype.cover) as Array<new () => { multer: CoverUpload }>;
-      return new Interceptor().multer;
-    }
-    const coversDir = path.join('uploads', 'covers');
-
-    it('creates uploads/covers on the first upload and reuses it afterwards', () => {
-      // A fresh container has no uploads/covers, and multer does not create the
-      // destination itself, so without the mkdir the very first cover upload on
-      // a new instance fails with ENOENT.
-      const exists = vi.spyOn(fs, 'existsSync').mockReturnValue(false);
-      const mkdir = vi.spyOn(fs, 'mkdirSync').mockImplementation(() => undefined);
-      try {
-        const { getDestination } = coverUpload().storage;
-        const cb = vi.fn();
-        getDestination(req, file, cb);
-        expect(mkdir).toHaveBeenCalledWith(expect.stringContaining(coversDir), { recursive: true });
-        expect(cb).toHaveBeenCalledWith(null, expect.stringContaining(coversDir));
-        exists.mockReturnValue(true);
-        mkdir.mockClear();
-        getDestination(req, file, cb);
-        expect(mkdir).not.toHaveBeenCalled();
-        expect(cb).toHaveBeenLastCalledWith(null, expect.stringContaining(coversDir));
-      } finally {
-        exists.mockRestore();
-        mkdir.mockRestore();
-      }
-    });
-
-    it('gives every stored cover a fresh name and keeps the original extension', () => {
-      const { getFilename } = coverUpload().storage;
-      const cb = vi.fn();
-      const upload = { originalname: 'sunset.JPG' } as Express.Multer.File;
-      getFilename(req, upload, cb);
-      getFilename(req, upload, cb);
-      const [first, second] = cb.mock.calls.map((c) => c[1] as string);
-      // Keeping the client's name would let one trip's upload overwrite another's;
-      // dropping the extension leaves the static handler with no mime to serve.
-      expect(first).toMatch(/^[0-9a-f-]{36}\.JPG$/);
-      expect(second).not.toBe(first);
-    });
-
     it('accepts the four allowed image types and rejects svg, a faked extension and a non-image mime', () => {
-      const { fileFilter, limits } = coverUpload();
       const verdict = (originalname: string, mimetype: string) => {
         const cb = vi.fn();
-        fileFilter(req, { originalname, mimetype } as Express.Multer.File, cb);
+        TRIP_COVER_FILE_FILTER!(req, { originalname, mimetype } as Express.Multer.File, cb);
         return cb.mock.calls[0];
       };
       expect(verdict('a.jpg', 'image/jpeg')).toEqual([null, true]);
@@ -404,11 +349,14 @@ describe('TripsController (parity with the legacy /api/trips route)', () => {
       expect(verdict('a.WEBP', 'image/webp')).toEqual([null, true]);
       // svg passes the image/* prefix but runs script when the browser opens the
       // stored cover, so it is rejected even under an allowed extension.
-      expect(verdict('logo.png', 'image/svg+xml')).toEqual([expect.any(Error), false]);
+      expect(verdict('logo.png', 'image/svg+xml')).toEqual([expect.any(Error)]);
       // A binary renamed to an image mime must still fail on the extension.
-      expect(verdict('payload.exe', 'image/png')).toEqual([expect.any(Error), false]);
-      expect(verdict('a.png', 'application/octet-stream')).toEqual([expect.any(Error), false]);
-      expect(limits.fileSize).toBe(20 * 1024 * 1024);
+      expect(verdict('payload.exe', 'image/png')).toEqual([expect.any(Error)]);
+      expect(verdict('a.png', 'application/octet-stream')).toEqual([expect.any(Error)]);
+      // The quirk stays quirky: the rejection error carries NO statusCode, so
+      // the route answers 500 (pinned by TRIP-P04), not 400.
+      expect((verdict('logo.png', 'image/svg+xml')[0] as Error & { statusCode?: number }).statusCode).toBeUndefined();
+      expect(MAX_COVER_SIZE).toBe(20 * 1024 * 1024);
     });
   });