fix(notifications): mark only the notifications the filter shows as read

app/Actions/UpdateNotificationsReadAt.php

@@ -9,13 +9,14 @@
 
 final readonly class UpdateNotificationsReadAt
 {
-    public function handle(User $user): CarbonImmutable
+    public function handle(User $user, string $type): CarbonImmutable
     {
         $readAt = CarbonImmutable::now()->startOfSecond();
 
         $user->notifications()
             ->relevant()
             ->unread()
+            ->ofType($type)
             ->update(['read_at' => $readAt]);
 
         return $readAt;

app/Http/Controllers/Api/NotificationController.php

@@ -19,14 +19,15 @@
 final readonly class NotificationController
 {
     /**
-     * The notifications grouped and filtered by ?type= like the web does it. As on the web, listing them marks every
-     * notification as read, whatever the filter, and read_at says when, so the ones read since then are the new ones.
-     * A commit comes with its author and the commit it replies to, when the user may see it.
+     * The notifications grouped and filtered by ?type= like the web does it. As on the web, listing them marks the
+     * notifications the filter shows as read, so the rest is still new under another filter, and read_at says when, so
+     * the ones read since then are the new ones. A commit comes with its author and the commit it replies to, when the
+     * user may see it.
      */
     public function index(ListNotificationsRequest $request, #[CurrentUser] User $user, NotificationGroupsQuery $query, UpdateNotificationsReadAt $action): AnonymousResourceCollection
     {
-        $readAt = $action->handle($user);
         $type = $request->type($user);
+        $readAt = $action->handle($user, $type);
 
         $notifications = $query->paginate($user, $type, $request->through());
 

app/Http/Controllers/NotificationController.php

@@ -17,8 +17,8 @@
 {
     public function index(ListNotificationsRequest $request, #[CurrentUser] User $user, NotificationGroupsQuery $query, UpdateNotificationsReadAt $action): Response
     {
-        $readAt = $request->loadsNotifications() ? $action->handle($user) : null;
         $type = $request->type($user);
+        $readAt = $request->loadsNotifications() ? $action->handle($user, $type) : null;
 
         return Inertia::render('notification/index', [
             'notifications' => Inertia::scroll(fn (): CursorPaginator => $query->withStarredCommitImages($query->withQuotedCommits($user, $query->paginate($user, $type, $request->through()))))->matchOn('data.group_id'),

app/Http/Requests/ListNotificationsRequest.php

@@ -15,9 +15,14 @@ final class ListNotificationsRequest extends FormRequest
 {
     public const string TYPE_RULE = 'regex:/\A(?:all|none|(?:replies|quotes|stars|mentions|follows|moderation)(?:,(?:replies|quotes|stars|mentions|follows|moderation))*)\z/';
 
+    /**
+     * The filter the browser keeps, including one this response is about to set, so the unread badge counts under the
+     * same filter the inbox just marked read.
+     */
     public static function rememberedType(Request $request, User $user): string
     {
-        $type = $request->cookie('notification_types_'.$user->id, 'all');
+        $cookie = 'notification_types_'.$user->id;
+        $type = Cookie::queued($cookie)?->getValue() ?? $request->cookie($cookie, 'all');
 
         return is_string($type) && preg_match(mb_substr(self::TYPE_RULE, 6), $type) === 1 ? $type : 'all';
     }

app/Queries/NotificationBuilder.php

@@ -42,6 +42,23 @@ public function unread(): self
         return $this->whereNull('read_at');
     }
 
+    /**
+     * Narrows to the types the inbox filter shows: a comma-separated list of its categories, 'all' for every type, and
+     * 'none' for no type at all.
+     */
+    public function ofType(string $type): self
+    {
+        return $this->when($type !== 'all', fn (self $query): self => $query->whereIn('type', array_merge(...array_map(fn (string $category): array => match ($category) {
+            'replies' => [NotificationType::Replied],
+            'quotes' => [NotificationType::Quoted],
+            'stars' => [NotificationType::Starred],
+            'mentions' => [NotificationType::Mentioned],
+            'follows' => [NotificationType::Followed],
+            'moderation' => [NotificationType::Approved, NotificationType::Rejected],
+            default => [],
+        }, explode(',', $type)))));
+    }
+
     /**
      * Leaves out what actors the recipient mutes did, and anything from across a block. Moderation
      * notifications are their own actor, and nobody can mute or block themselves.

app/Queries/NotificationGroupsQuery.php

@@ -90,7 +90,7 @@ public function unreadCount(User $user, string $type = 'all'): int
             return 0;
         }
 
-        $oldestUnread = $this->visible($user)->unread()->oldest()->first();
+        $oldestUnread = $this->visible($user)->unread()->ofType($type)->oldest()->first();
 
         return $oldestUnread instanceof Notification
             ? $this->groupsSince($user, $oldestUnread->created_at, $type)->where('group_rank', 1)->where('unread_count', '>', 0)->count()
@@ -132,15 +132,7 @@ private function group(NotificationBuilder $visible, string $type = 'all', ?Noti
                 'identified.group_id',
                 fn (QueryBuilder $query): QueryBuilder => $query->from('identified')->where('id', $member->id)->select('group_id'),
             ))
-            ->when($type !== 'all', fn (NotificationBuilder $query): NotificationBuilder => $query->whereIn('type', array_merge(...array_map(fn (string $category): array => match ($category) {
-                'replies' => ['replied'],
-                'quotes' => ['quoted'],
-                'stars' => ['starred'],
-                'mentions' => ['mentioned'],
-                'follows' => ['followed'],
-                'moderation' => ['approved', 'rejected'],
-                default => [],
-            }, explode(',', $type)))));
+            ->ofType($type);
 
         $gap = match (DB::connection($visible->getModel()->getConnectionName())->getDriverName()) {
             'mysql', 'mariadb' => 'TIMESTAMPDIFF(SECOND, previous_match_at, created_at)',

tests/Feature/Controllers/Api/NotificationControllerTest.php

@@ -273,15 +273,16 @@
     'moderation' => ['moderation', ['rejected', 'approved']],
 ]);
 
-it('marks every notification as read whatever the filter, as the web does', function (): void {
+it('marks only what the filter shows as read, as the web does', function (): void {
     $followed = Notification::factory()->for($this->user, 'recipient')->create();
     $replied = Notification::factory()->for($this->user, 'recipient')->replied()->create();
 
     signedApiRequest($this->keyPair, 'GET', route('api-notification.index', ['type' => 'follows']))
         ->assertOk()
         ->assertJsonPath('data.*.id', [$followed->id]);
 
-    expect($replied->refresh()->read_at)->not->toBeNull();
+    expect($followed->refresh()->read_at)->not->toBeNull()
+        ->and($replied->refresh()->read_at)->toBeNull();
 });
 
 it('validates the filter before marking anything as read', function (array $query, string $field): void {

tests/Feature/NotificationInboxTest.php

@@ -77,6 +77,24 @@
     'partially invalid saved filter' => ['replies,invalid', 4],
 ]);
 
+it('marks only what the filter shows as read, so the rest is still new under another filter', function (): void {
+    $user = User::factory()->create();
+    $reply = Notification::factory()->for($user, 'recipient')->replied()->create();
+    $follow = Notification::factory()->for($user, 'recipient')->create();
+
+    $this->actingAs($user)->get(route('notification.index', ['type' => 'replies']))->assertOk();
+
+    expect($reply->refresh()->read_at)->not->toBeNull()
+        ->and($follow->refresh()->read_at)->toBeNull();
+
+    $this->travel(1)->minute();
+
+    $this->get(route('notification.index', ['type' => 'all']))->assertOk()
+        ->assertInertia(fn ($page) => $page->where('readAt', $follow->refresh()->read_at?->toJSON()));
+
+    expect($reply->refresh()->read_at)->toBeLessThan($follow->read_at);
+});
+
 it('refreshes the entire loaded inbox window without losing older pages', function (): void {
     $user = User::factory()->create();
     $notices = collect();
@@ -147,7 +165,7 @@
         ->assertJsonPath('props.auth.unreadNotificationsCount', 0);
 
     expect($latest->refresh()->read_at)->not->toBeNull()
-        ->and($follow->refresh()->read_at)->not->toBeNull()
+        ->and($follow->refresh()->read_at)->toBeNull()
         ->and($other->refresh()->read_at)->toBeNull();
 });
 
@@ -371,7 +389,7 @@
     expect($one->refresh()->read_at->equalTo($readAt))->toBeTrue();
 });
 
-it('marks all relevant activity read beyond the first page and selected type', function (): void {
+it('marks all relevant activity of the selected type read beyond the first page', function (): void {
     $user = User::factory()->create();
     $root = Commit::factory()->create();
     $events = collect();
@@ -391,8 +409,8 @@
     $this->actingAs($user)->get(route('notification.index', ['type' => 'replies']))
         ->assertOk()->assertInertia(fn ($page) => $page->has('notifications.data', 20)
         ->where('auth.unreadNotificationsCount', 0));
-    expect($user->notifications()->relevant()->unread()->count())->toBe(0)
-        ->and($follow->refresh()->read_at)->not->toBeNull()
+    expect($user->notifications()->relevant()->unread()->ofType('replies')->count())->toBe(0)
+        ->and($follow->refresh()->read_at)->toBeNull()
         ->and($events->last()->refresh()->read_at)->not->toBeNull()
         ->and($hidden->refresh()->read_at)->toBeNull()
         ->and($other->refresh()->read_at)->toBeNull();
@@ -410,6 +428,9 @@
     expect($latest->refresh()->read_at)->toBeNull();
     $this->get(route('notification.index', ['type' => 'follows']))->assertInertia(fn ($page) => $page
         ->where('auth.unreadNotificationsCount', 0));
+    expect($latest->refresh()->read_at)->toBeNull();
+    $this->get(route('notification.index', ['type' => 'all']))->assertInertia(fn ($page) => $page
+        ->where('auth.unreadNotificationsCount', 0));
     expect($latest->refresh()->read_at)->not->toBeNull();
 });
 

tests/Unit/Actions/UpdateNotificationsReadAtTest.php

@@ -13,7 +13,7 @@
 
     $action = resolve(UpdateNotificationsReadAt::class);
 
-    $action->handle($user);
+    $action->handle($user, 'all');
 
     foreach ($notifications as $notification) {
         expect($notification->refresh()->read_at)->not->toBeNull();
@@ -25,7 +25,7 @@
 it('returns the moment it marked notifications read', function (): void {
     $notification = Notification::factory()->create();
 
-    $readAt = resolve(UpdateNotificationsReadAt::class)->handle($notification->recipient);
+    $readAt = resolve(UpdateNotificationsReadAt::class)->handle($notification->recipient, 'all');
 
     expect($readAt->toJSON())->toBe(now()->startOfSecond()->toJSON())
         ->and($notification->refresh()->read_at->equalTo($readAt))->toBeTrue();
@@ -36,7 +36,7 @@
 
     $action = resolve(UpdateNotificationsReadAt::class);
 
-    $action->handle($notification->recipient);
+    $action->handle($notification->recipient, 'all');
 
     expect($notification->refresh()->read_at->toDateTimeString())->toBe(now()->subDay()->toDateTimeString());
 });
@@ -49,7 +49,7 @@
     $otherCommit = Notification::factory()->for($starred->recipient, 'recipient')->starred()->create();
     $follow = Notification::factory()->for($starred->recipient, 'recipient')->create();
 
-    resolve(UpdateNotificationsReadAt::class)->handle($starred->recipient);
+    resolve(UpdateNotificationsReadAt::class)->handle($starred->recipient, 'all');
 
     expect($sameCommit->refresh()->read_at)->not->toBeNull()
         ->and($replied->refresh()->read_at)->toBeNull()
@@ -61,7 +61,28 @@
 it('marks nothing for a user without notifications', function (): void {
     $notification = Notification::factory()->create();
 
-    resolve(UpdateNotificationsReadAt::class)->handle(User::factory()->create());
+    resolve(UpdateNotificationsReadAt::class)->handle(User::factory()->create(), 'all');
 
     expect($notification->refresh()->read_at)->toBeNull();
 });
+
+it('marks only the types the filter shows, and nothing under none', function (): void {
+    $user = User::factory()->create();
+    $reply = Notification::factory()->for($user, 'recipient')->replied()->create();
+    $star = Notification::factory()->for($user, 'recipient')->starred()->create();
+    $follow = Notification::factory()->for($user, 'recipient')->create();
+
+    $action = resolve(UpdateNotificationsReadAt::class);
+
+    $action->handle($user, 'none');
+
+    expect($reply->refresh()->read_at)->toBeNull()
+        ->and($star->refresh()->read_at)->toBeNull()
+        ->and($follow->refresh()->read_at)->toBeNull();
+
+    $action->handle($user, 'replies,stars');
+
+    expect($reply->refresh()->read_at)->not->toBeNull()
+        ->and($star->refresh()->read_at)->not->toBeNull()
+        ->and($follow->refresh()->read_at)->toBeNull();
+});