feat(inbox): page through an inbox larger than one screenful - #292
Merged
Conversation
The inbox asked the server for fifty notifications and dropped the cursor it handed back, so the fifty first oldest were simply the end of the inbox as far as the reader was concerned. Nothing said there was more, and nothing could reach it. listInbox already paged, so this is mostly wiring: a GET on /api/notifications hands back a page and its cursor, the server component passes the first cursor down, and the list offers the older ones at its end. The offer loads itself when it scrolls into view, so reading down the list keeps going, and it stays a button so a reader who never scrolls it into view, or who is on a keyboard, can still ask. The cursor lives in a ref rather than only in state. Two things can ask to load at once, the observer and the click, and a request that started against an older cursor would otherwise finish last and put that older cursor back, leaving the offer up forever. Reading the cursor at call time means the second caller sees what the first one already recorded. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
github-actions
Bot
added
tests
📝 WalkthroughWalkthroughThe inbox now supports cursor-based notification pagination. The API returns mapped items and cursors. ChangesInbox pagination
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant InboxPage
participant InboxRealtime
participant InboxView
participant NotificationsAPI
InboxPage->>InboxRealtime: pass nextCursor
InboxRealtime->>InboxView: forward nextCursor
InboxView->>NotificationsAPI: request next page
NotificationsAPI-->>InboxView: return items and nextCursor
InboxView->>InboxView: append unique items and render state
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
A failed page left its message on screen even after the next attempt succeeded, so the inbox went on saying it could not reach the older notifications while showing them. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Both complaints shared one slot, so clearing the paging message on a successful page also wiped a read state save that had failed and rolled back, and the reader lost the only sign their change had not stuck. Paging keeps its own message now, and it sits under the offer at the end of the list rather than in the header, which is where the reader was looking when they asked for more. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
apps/web/src/features/inbox/inbox-view.tsx (1)
222-222: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse a shared interaction helper.
Line 222 adds raw
transition-colorsandhover:text-mutedclasses. Replace them with the applicable shared interaction helper so hover and focus motion uses the required tokens and reduced-motion behavior.As per coding guidelines, "hover/focus state color transitions must be implemented only via the shared motion/color helpers."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/web/src/features/inbox/inbox-view.tsx` at line 222, Replace the raw transition-colors and hover:text-muted classes on the inbox view element with the applicable shared motion/color interaction helper, preserving the existing muted hover appearance while inheriting the required hover/focus tokens and reduced-motion behavior.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/web/src/app/api/notifications/route.ts`:
- Around line 7-18: Add direct tests for the GET route that invoke the exported
GET handler rather than mocking fetch. Cover cursor parsing and forwarding,
enforcement of the INBOX_PAGE_SIZE limit, and mapping the listInbox result into
notifications with the returned nextCursor.
In `@apps/web/src/features/inbox/inbox-view.tsx`:
- Around line 644-657: Update the empty filtered-state branch around visible to
retain the LoadMoreRow when cursor is non-null, rather than bypassing pagination
with EmptyState alone. Reuse the existing loadMore, loadingMore, pagingError,
and inbox-paging-error behavior, and add a regression test covering an empty
first filtered page that can load older matching rows.
- Around line 390-417: Replace the component-owned pagination flow in loadMore
with a TanStack Query infinite query for notifications, using query pages and
page parameters as the source of truth instead of rows and cursorRef. Update the
rendered list from the infinite-query data, preserve deduplication and
loading/error states, and integrate realtime updates by patching or invalidating
the query cache without triggering a full refetch of the viewed list.
---
Nitpick comments:
In `@apps/web/src/features/inbox/inbox-view.tsx`:
- Line 222: Replace the raw transition-colors and hover:text-muted classes on
the inbox view element with the applicable shared motion/color interaction
helper, preserving the existing muted hover appearance while inheriting the
required hover/focus tokens and reduced-motion behavior.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: d15b523e-b9c8-40b9-8409-14ca2c2265d7
📒 Files selected for processing (9)
apps/web/src/app/(app)/inbox/page.tsxapps/web/src/app/api/notifications/route.tsapps/web/src/features/inbox/data.tsapps/web/src/features/inbox/inbox-realtime.tsxapps/web/src/features/inbox/inbox-view.tsxapps/web/tests/components/keyboard-hints.test.tsxapps/web/tests/features/inbox/inbox-issue.test.tsxapps/web/tests/features/inbox/inbox-open.test.tsxapps/web/tests/features/inbox/inbox-pagination.test.tsx
A tab whose matches all sat in pages the reader had not fetched yet showed inbox zero and no way forward: the empty state replaced the list, and the offer to load older notifications went with it. Unread was the easy way to hit it, fifty read notifications deep. Inbox zero now means the inbox really is empty, nothing loaded and nothing left to fetch. While the server still has pages, the list stands with the offer in it even when the filter matches none of what is loaded. The route gets its own tests as well: the tests that came with it mocked fetch, so nothing exercised the cursor, the page size cap or the shape the inbox reads. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The inbox asked for fifty notifications and threw away the cursor the
server handed back. Past fifty, the rest of the inbox was unreachable,
and nothing on the screen admitted it existed.
listInboxalready paged, so most of this is wiring. AGETon/api/notificationsreturns a page and its cursor, the servercomponent passes the first cursor down with the first page, and the
list offers the older ones at its end. The offer loads itself when it
scrolls into view, so reading down the list keeps going, and it stays
a real button so a reader who never scrolls it into view, or who is on
a keyboard, can still ask for more. It retires when the server stops
handing back a cursor.
The one thing that was not wiring
The cursor lives in a ref, not only in state. Two things can ask to
load at the same moment, the observer and a click, and a request that
started against an older cursor would otherwise resolve last and write
that older cursor back, so the offer never retired and the same page
was fetched again. Reading the cursor at call time means the second
caller sees what the first already recorded. There is an in flight
guard too, but on its own it is not enough: it clears before React has
re-rendered, so the next caller would still hold the stale cursor.
Verified
Against a real inbox of 126 notifications: 50, then 100, then 126,
the offer retires on the last page, and no notification appears twice.
Eight unit tests cover the offer appearing and retiring, the cursor
actually asked for, appending under what is already read, paging more
than once, dropping a duplicate that a shifting page can return, and
saying so without losing the offer when a page fails.
Elsewhere
Issue lists and the Slack channel picker already page. Three surfaces
still truncate silently, each returning a cursor the client drops:
issue comments and doc comments both stop at fifty, and issue activity
returns an
activityCursornothing consumes. They are the same shapeof gap as this one, and each wants its own change since loading older
comments reads from the top of a thread rather than the bottom.
Greptile Summary
The PR adds cursor-based inbox pagination while keeping pagination failures separate from read-state failures.
Confidence Score: 5/5
The PR appears safe to merge.
No blocking failure remains; the successful-retry path clears only the pagination complaint, while successful pagination leaves an unrelated failed read-state message intact.
Important Files Changed
Sequence Diagram
sequenceDiagram participant Page as InboxPage participant View as InboxView participant API as GET /api/notifications participant Service as listInbox Page->>Service: Request initial page Service-->>Page: Items and nextCursor Page->>View: Initial items and cursor View->>API: Request page with cursor API->>Service: Request bounded next page Service-->>API: Older items and nextCursor API-->>View: Serialized page View->>View: Deduplicate and append items alt More pages remain View->>View: Retain load-more offer else Last page View->>View: Retire load-more offer endReviews (5): Last reviewed commit: "fix(inbox): keep older pages reachable w..." | Re-trigger Greptile