Repository navigation
Conversation
…ifications Each inbox page grouped the recipient's whole notification history, so a page cost time in proportion to that history. NotificationGroupsQuery now groups a window of recent notifications and widens the window until it provably holds the groups the whole history would give. It falls back to the whole history after four attempts. Signed-off-by: Younes <git@younes.io>
|
Benchmark numbers for this PR. The script served
On page 1 the SQL time fell from 348 ms to 14 ms. About 32 ms of PHP outside SQL is now the limit. Type filters on local-dev page 1, from an earlier run: follows 304 to 63 ms, replies 276 to 55 ms, stars 318 to 14 ms. The query then had 2 to 4 more round trips than the old one. For some small cases that cost more than it saved. dev_2 replies went from 25 ms to 27 ms, and local-dev mentions stayed level. The last page of a sparse filter still groups the whole history, for example 202 ms for local-dev follows page 10. |
…ons-window Replies now form one group each (dev-null-sh#911), so the straddling-conversation test seeds mentions, which still group by thread. Signed-off-by: Younes <git@younes.io>
|
No actionable comments were generated in the recent review.
 |
| Layer / File(s) | Summary |
|---|---|
Group start metadata app/Models/Notification.php, app/Queries/NotificationGroupsQuery.php |
The model casts and hides group_started_at. The grouping query selects each group’s earliest creation time. |
Bounded-window pagination app/Queries/NotificationGroupsQuery.php |
The query resolves the cursor, selects and widens notification windows, checks group completeness, includes related starred notifications, maps categories to notification types, and paginates the selected groups. |
Pagination regression tests tests/Unit/Queries/NotificationGroupsQueryTest.php |
Timestamped fixtures and comparison helpers test mention, star, and follow groups across windows and pages. Query-count expectations are updated. |
Priority:
Estimated code review effort: 3 (Moderate) | ~25 minutes
Change: Refactor
Sequence Diagram(s)
sequenceDiagram
participant Client
participant NotificationGroupsQuery
participant Paginator
participant NotificationQuery
Client->>NotificationGroupsQuery: Request a notification group page
NotificationGroupsQuery->>Paginator: Resolve the request cursor
Paginator-->>NotificationGroupsQuery: Return the cursor
NotificationGroupsQuery->>NotificationQuery: Fetch and widen notification windows
NotificationQuery-->>NotificationGroupsQuery: Return notification rows
NotificationGroupsQuery->>Paginator: Cursor-paginate selected groups
Paginator-->>Client: Return the group page
Possibly related PRs
- dev-null-sh/dev-null.sh#1078: Enriches paginated notification groups with commit star data.
- dev-null-sh/dev-null.sh#1168: Changes the visible notification rows used by the window strategy.
- dev-null-sh/dev-null.sh#678: Changes category filtering and read behavior in the notification pipeline.
Suggested reviewers: xheaven, hbenabdallah
Merge Risk: 
617fa
-
No identified pagination issue remains that needs resolution before merging; proceed with normal checks.
Security architecture risk: 
617fa
-
The optimization preserves recipient-scoped notification reads and includes widening and fallback paths when a window cannot establish completeness. No new security boundary violation was identified. The remaining risk is preserving grouping behavior across cursor and live-update conditions.
Retained concerns
No architecture-level concerns identified.Security review details
-
Security Blast Radius
- inferred — The changed algorithm affects both web and API inbox reads. Notification-row selection remains recipient-scoped, while computational cost can still grow with that recipient’s history because star expansion and fallback are not hard row limits.
Trust Boundaries and Controls
- observed — The inspected production callers obtain identity through CurrentUser rather than a request-supplied recipient. Notification types are validated against a fixed category grammar and mapped to known notification kinds; request cursors influence window boundaries without replacing recipient ownership.
Resilience and Maintainability Implications
- observed — Window widening is capped at four attempts with growth factors between two and eight. This bounds retry rounds, not total database work: unsuccessful attempts precede the fallback query.
-
Pre-merge checks: 
-
Check name Status Explanation Resolution Docstring Coverage Docstring coverage is 64.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 25 functions across 3 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 names the main change: the notifications inbox grouping now runs over a window of recent notifications instead of the full history, and it uses the perf() type prefix. The raw summary and ob… 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 names the main change: the notifications inbox grouping now runs over a window of recent notifications instead of the full history, and it uses the perf() type prefix. The raw summary and objectives confirm this is the core change, with the bounded-window strategy in NotificationGroupsQuery::paginate and supporting test updates. The title is concise, specific, and accurate, so it passes.
-
-
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.
…ons-window Signed-off-by: Younes <git@younes.io> # Conflicts: # tests/Unit/Queries/NotificationGroupsQueryTest.php
Why
Each inbox page regrouped the user's whole notification history, so
/notificationsgot slower as an account aged. For an account with 46k notifications, one query took 348 of the 380 ms.How it works
A page shows 20 groups, and they come from the most recent notifications. The query now groups only a recent slice, starting at about 32 notifications per group on the page. Two rules let it prove the slice gives the same groups as the whole history:
If a group on the page fails that check, the query widens the slice two to eight times, depending on how many groups it found, and tries again. After four tries it groups the whole history, as before.
Results
Medians of 5 alternating rounds against
production, with identical response bytes. A small account gains less because its history was already short. The type filters, the method, and two small regressions are in the benchmark comment.Scope
Blast radius
Every inbox page goes through this path, and
group_id,event_count, andunread_countmust come out exactly as before. A grouping bug would show as a split or merged group.Verification
paginateon 25 users, 8 type filters, and every page. All 4,453 pages matched. With either rule above removed, it found 46 and 2 mismatches.NotificationGroupsQueryTestcover an old commit getting a new star, a reply thread across the slice edge, and follows around an hour gap. The 946 notification tests pass.Summary by CodeRabbit