Repository navigation
Conversation
…read Signed-off-by: Younes <git@younes.io>
Signed-off-by: Younes <git@younes.io>
…ns-read-scope Signed-off-by: Younes <git@younes.io> # Conflicts: # app/Http/Controllers/NotificationController.php
|
No actionable comments were generated in the recent review.
 |
| Layer / File(s) | Summary |
|---|---|
Apply filters to notification queries app/Http/Requests/ListNotificationsRequest.php, app/Queries/NotificationBuilder.php, app/Queries/NotificationGroupsQuery.php |
The request checks the queued filter cookie before the request cookie. NotificationBuilder::ofType maps categories to notification types. Grouped results and unread counts now apply the selected filter. |
Mark filtered notifications as read app/Actions/UpdateNotificationsReadAt.php, app/Http/Controllers/Api/NotificationController.php, app/Http/Controllers/NotificationController.php, tests/Feature/Controllers/Api/NotificationControllerTest.php, tests/Feature/NotificationInboxTest.php, tests/Unit/Actions/UpdateNotificationsReadAtTest.php |
The controllers pass the resolved type to the read-timestamp action. The action updates only unread relevant notifications of that type. Tests cover filtered and unfiltered views, including notifications beyond the first page. |
Priority:
Estimated code review effort: 3 (Moderate) | ~20 minutes
Change: Bug fix
Possibly related PRs
- dev-null-sh/dev-null.sh#997: Adds
EventCancellednotifications to the relevant notification flow that this PR filters when updating read timestamps.
Suggested reviewers: xheaven, simonhamp
Merge Risk: 
026eb
-
Filtered inbox views leave hidden notifications unread, and no merge-blocking issue remains after normal checks.
Security architecture risk: 026eb
-
The change narrows read acknowledgments to the selected notification categories while preserving recipient ownership and existing visibility controls. No material security risk introduced or worsened by this change was identified.
Retained concerns
No architecture-level concerns identified.Security review details
-
Security Blast Radius
- inferred — Through the inspected entrypoints, caller-controlled filters select categories within the current user's relevant notifications; they do not select another recipient or increase update authority. The affected persistent state is the read timestamp of matching unread notifications.
Security Findings and Attack Paths
- inferred — No introduced cross-recipient read mutation or visibility bypass was identified in the inspected paths. Compared with the base, the new predicate narrows acknowledgment while retaining the ownership and visibility predicates used before this PR.
Trust Boundaries and Controls
- observed — The request type is constrained to named categories, all, or none. Remembered cookie values pass the same allowlist and fall back to all when invalid. Category values are mapped into a whereIn predicate rather than interpreted as query structure.
Resilience and Maintainability Implications
- observed — Acknowledgment still precedes pagination and rendering, without a surrounding transaction in these methods. A later request failure can therefore leave notifications acknowledged without successful delivery. This ordering predates the PR; the change limits the affected categories rather than introducing or broadening that failure window.
-
Pre-merge checks: 
-
Check name Status Explanation Resolution Docstring Coverage Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 8 functions across 9 files. Write docstrings for the functions missing them to satisfy the coverage threshold. -
Check name Status Explanation Description Check Check skipped - CodeRabbit’s high-level summary is enabled. Title check The title "fix(notifications): mark only the notifications the filter shows as read" matches the main change. The PR changes the web inbox and GET /api/notifications so opening them marks only the not… Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request. Full details: Title check
-
Explanation
The title "fix(notifications): mark only the notifications the filter shows as read" matches the main change. The PR changes the web inbox and GET /api/notifications so opening them marks only the notification types shown by the active filter as read. Hidden types stay unread. The title is specific, uses the conventional commit prefix, and names the behavior change. It is not vague, and it does not describe anything absent from the changeset.
-
-
Finishing Touches
-
🧪 Generate unit tests (beta)
-
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Comment @coderabbitai help to get the list of available commands.
Why
Opening the inbox marks every notification as read, whatever filter is on. If you filter to Replies, your unseen stars and follows are marked read too. You never see them as new, and the badge drops them. Now the inbox marks as read only the types the filter shows.
What changed
UpdateNotificationsReadAttakes the active filter and marks only the matching types as read, on the web and in the API.NotificationBuilder::ofType(), so grouping, the unread count and marking read use one rule.rememberedType()reads a filter cookie queued in the same response, so the badge counts under the filter the inbox just applied.Scope
GET /api/notifications.Blast Radius
The change touches only the notification read path and the badge count. With no filter (
all), the behavior is the same as before.Verification
TomTest, which fails the same way onproduction. Pint, Rector and PHPStan pass.Summary by CodeRabbit