Debounce the public preview log the way the signed-in one already is

FileThumbnailController::preview() writes at most one FilePreviewed row
per viewer per file per five minutes, because a browser turns one video
into a long tail of Range requests against the same URL. Its docblock
names the anonymous route as the place the same act happens without an
account -- and that route logs unconditionally.

Measured: five requests for the same public file, five
PublicFilePreviewed rows, against one for the signed-in twin. One visitor
watching one clip buries the public half of the activity log, which is
also the half an operator reads to see what the outside world is doing.

The window is now a shared PreviewLog, next to PreviewKind, which the two
preview routes already share for the same reason. Keying is unchanged for
a signed-in viewer; an anonymous one has no account to key on, so the
request IP stands in -- the same substitute the API's rate limiter makes
for an unauthenticated caller. It is a cache key with a five-minute life
and never reaches the log, which keeps its own decision about recording an
IP (ActivityLogger::shouldRecordIp, Setting::DownloadIpLogging).

Three tests: the replay is one row, two visitors are two rows, and the
window is per file. Without the fix the first goes red.

app/Modules/Files/Http/Controllers/FileThumbnailController.php

@@ -6,11 +6,11 @@ namespace App\Modules\Files\Http\Controllers;
 
 use App\Http\Controllers\Controller;
 use App\Modules\Audit\Action;
-use App\Modules\Audit\ActivityLogger;
 use App\Modules\Files\Access\DownloadAllowance;
 use App\Modules\Files\Delivery\StoredFileResponse;
 use App\Modules\Files\Models\File;
 use App\Modules\Files\Preview\PreviewKind;
+use App\Modules\Files\Preview\PreviewLog;
 use App\Modules\Files\Thumbnails\Events\ResolvingImageRendering;
 use App\Modules\Files\Thumbnails\ImageAudience;
 use App\Modules\Files\Thumbnails\ImageRendition;
@@ -22,7 +22,6 @@ use App\Support\ContentDisposition;
 use Illuminate\Http\RedirectResponse;
 use Illuminate\Http\Request;
 use Illuminate\Http\Response;
-use Illuminate\Support\Facades\Cache;
 use Illuminate\Support\Facades\Event;
 use Illuminate\Support\Facades\Gate;
 use Illuminate\Support\Facades\Storage;
@@ -72,7 +71,7 @@ class FileThumbnailController extends Controller
 {
     public function __construct(
         private readonly ThumbnailGenerator $thumbnails,
-        private readonly ActivityLogger $activity,
+        private readonly PreviewLog $previews,
         private readonly DownloadAllowance $allowance,
         private readonly StoredFileResponse $bytes,
         private readonly LocalSourceFile $source,
@@ -144,7 +143,10 @@ class FileThumbnailController extends Controller
         // file.
         abort_unless($this->allowance->allows($file, $request->user()), 403);
 
-        $this->logPreview($file, $request);
+        // Debounced, because a browser turns one video into dozens of
+        // Range requests — see PreviewLog, which the anonymous twin in
+        // PublicGroupsController::preview shares.
+        $this->previews->record(Action::FilePreviewed, $file, $request->user());
 
         if ($kind === PreviewKind::Image) {
             $audience = ImageAudience::forViewer($request->user());
@@ -164,29 +166,6 @@ class FileThumbnailController extends Controller
         return $this->bytes->inline($file);
     }
 
-    /**
-     * One log row per viewer per file per five minutes.
-     *
-     * Watching a video is a single deliberate act that the browser turns
-     * into dozens of Range requests against this route, and each one
-     * arrives here indistinguishable from someone clicking preview again.
-     * Cache::add is the whole mechanism: it writes only if the key is
-     * absent, so the first request through the window logs and the rest
-     * are silent, without a read-then-write race between two of them.
-     *
-     * Keyed by viewer, so one client's playback never suppresses another
-     * person's preview of the same file. Anonymous viewers do not reach
-     * this route at all — see PublicGroupsController::preview.
-     */
-    private function logPreview(File $file, Request $request): void
-    {
-        $key = 'file-preview-logged:'.$file->id.':'.($request->user()->id ?? 'guest');
-
-        if (Cache::add($key, true, now()->addMinutes(5))) {
-            $this->activity->log(Action::FilePreviewed, subject: $file);
-        }
-    }
-
     /**
      * The cached rendition's path on the local disk, generating it first
      * if this is the first time anyone has asked for it. Null only when

app/Modules/Files/Preview/PreviewLog.php

@@ -0,0 +1,52 @@
+<?php
+
+declare(strict_types=1);
+
+namespace App\Modules\Files\Preview;
+
+use App\Models\User;
+use App\Modules\Audit\Action;
+use App\Modules\Audit\ActivityLogger;
+use App\Modules\Files\Models\File;
+use Illuminate\Support\Facades\Cache;
+
+/**
+ * One log row per viewer per file per five minutes, for both preview
+ * routes — FileThumbnailController::preview (signed in) and
+ * PublicGroupsController::preview (anonymous).
+ *
+ * Watching a video is a single deliberate act that the browser turns into
+ * dozens of Range requests, each arriving indistinguishable from someone
+ * clicking preview again. Cache::add is the whole mechanism: it writes
+ * only if the key is absent, so the first request through the window logs
+ * and the rest are silent, without a read-then-write race between two of
+ * them.
+ *
+ * Keyed by viewer, so one person's playback never suppresses another's
+ * view of the same file. An anonymous visitor has no account to key on,
+ * so the request IP stands in — the same substitute the API's rate
+ * limiter makes for an unauthenticated caller. It is a cache key with a
+ * five-minute life and never reaches the log, which keeps its own
+ * decision about recording an IP (see ActivityLogger::shouldRecordIp and
+ * Setting::DownloadIpLogging).
+ *
+ * Shared rather than restated, because the window is the rule: two copies
+ * of "five minutes" are two things to change and one to forget.
+ */
+class PreviewLog
+{
+    private const WINDOW_MINUTES = 5;
+
+    public function __construct(
+        private readonly ActivityLogger $activity,
+    ) {}
+
+    public function record(Action $action, File $file, ?User $viewer): void
+    {
+        $viewerKey = $viewer !== null ? (string) $viewer->id : 'ip:'.request()->ip();
+
+        if (Cache::add('file-preview-logged:'.$file->id.':'.$viewerKey, true, now()->addMinutes(self::WINDOW_MINUTES))) {
+            $this->activity->log($action, subject: $file);
+        }
+    }
+}

app/Modules/Groups/Http/Controllers/PublicGroupsController.php

@@ -14,6 +14,7 @@ use App\Modules\Files\Models\Category;
 use App\Modules\Files\Models\File;
 use App\Modules\Files\Models\Folder;
 use App\Modules\Files\Preview\PreviewKind;
+use App\Modules\Files\Preview\PreviewLog;
 use App\Modules\Files\Thumbnails\ImageAudience;
 use App\Modules\Files\Thumbnails\ImageRendition;
 use App\Modules\Files\Thumbnails\LocalSourceFile;
@@ -77,6 +78,7 @@ class PublicGroupsController extends Controller
     public function __construct(
         private readonly Settings $settings,
         private readonly ActivityLogger $activity,
+        private readonly PreviewLog $previews,
         private readonly DownloadAllowance $allowance,
         private readonly ThumbnailGenerator $thumbnails,
         private readonly PublicThemeRegistry $themes,
@@ -298,7 +300,11 @@ class PublicGroupsController extends Controller
         // nothing.
         abort_unless($this->allowance->allows($file, null), 403);
 
-        $this->activity->log(Action::PublicFilePreviewed, subject: $file);
+        // Debounced exactly as the signed-in twin is, and for the same
+        // reason: a single visitor watching one video arrives here dozens
+        // of times. Without a viewer to key on, PreviewLog keys on the
+        // request IP.
+        $this->previews->record(Action::PublicFilePreviewed, $file, null);
 
         return $this->bytes->inline($file);
     }

tests/Feature/Groups/PublicFilePreviewTest.php

@@ -45,6 +45,45 @@ test('a visitor can preview a public file, and it is logged as a public preview'
         ->and(ActivityLog::query()->where('action', Action::PublicFileDownloaded)->where('subject_id', $file->id)->exists())->toBeFalse();
 });
 
+// The same deliberate act, and the same long tail of Range requests a
+// browser makes of it. The signed-in twin has debounced this since it was
+// written; the anonymous one wrote a row per request.
+test('replaying a public preview logs it once, not once per request', function () {
+    $file = publicListingFile();
+
+    foreach (range(1, 5) as $ignored) {
+        $this->get(route('public.preview', ['public', $file->slug]))->assertOk();
+    }
+
+    expect(ActivityLog::query()->where('action', Action::PublicFilePreviewed)->where('subject_id', $file->id)->count())
+        ->toBe(1);
+});
+
+test('two visitors of the same file are each logged', function () {
+    // Keyed by viewer, and an anonymous visitor's stand-in is their IP —
+    // so one visitor's playback cannot swallow the record of somebody else
+    // looking at the same file.
+    $file = publicListingFile();
+
+    $this->get(route('public.preview', ['public', $file->slug]))->assertOk();
+
+    $this->withServerVariables(['REMOTE_ADDR' => '198.51.100.7'])
+        ->get(route('public.preview', ['public', $file->slug]))->assertOk();
+
+    expect(ActivityLog::query()->where('action', Action::PublicFilePreviewed)->where('subject_id', $file->id)->count())
+        ->toBe(2);
+});
+
+test('the window is per file, so a second public file is still logged', function () {
+    $first = publicListingFile();
+    $second = publicListingFile(['name' => 'second', 'slug' => 'second-report']);
+
+    $this->get(route('public.preview', ['public', $first->slug]))->assertOk();
+    $this->get(route('public.preview', ['public', $second->slug]))->assertOk();
+
+    expect(ActivityLog::query()->where('action', Action::PublicFilePreviewed)->count())->toBe(2);
+});
+
 test('a file that is not public, or has expired, has no preview', function () {
     $private = publicListingFile(['public' => false]);
     $expired = publicListingFile(['expires_at' => now()->subDay()]);