diff --git a/server/internal/handler/comment.go b/server/internal/handler/comment.go index 907514c48..37a848657 100644 --- a/server/internal/handler/comment.go +++ b/server/internal/handler/comment.go @@ -383,18 +383,18 @@ func (h *Handler) fetchCommentsForList(ctx context.Context, args fetchCommentsAr // requested ⇒ the SELECT exhausted matching threads, so there is // no older page to scroll to. // - // Additionally suppress the cursor when `since` is set and filtered - // the page empty. The pagination walks threads in strictly - // decreasing last_activity_at, so if every comment in this page is - // <= since then head.last_activity_at <= since, and every older - // thread (last_activity_at strictly less than head's) is also <= - // since by transitivity — none of them can produce a comment newer - // than `since`. Emitting a cursor in that case would invite the - // caller into a guaranteed-empty walk. Flagged by Elon in #2787's - // second review (MUL-2340 nit). + // Additionally suppress the cursor when `since` is set and the head + // thread's last_activity_at is already <= since. The pagination + // walks threads in strictly decreasing last_activity_at, so every + // older page has last_activity_at strictly less than the head's — + // if the head itself can't satisfy `> since`, no older thread can + // either. Predicating on the head (not on whether `comments` is + // empty) also catches the mixed case where this page keeps rows + // from fresher threads but the head thread is already past `since`. + // Flagged by Elon in #2787's second review (MUL-2340 nit). out := fetchCommentsResult{Comments: comments} emitCursor := len(seenRoot) >= args.RecentN && headRoot.Valid && headLast.Valid - if args.Since.Valid && len(comments) == 0 { + if emitCursor && args.Since.Valid && !headLast.Time.After(args.Since.Time) { emitCursor = false } if emitCursor { diff --git a/server/internal/handler/comment_list_test.go b/server/internal/handler/comment_list_test.go index 1b1e02fe8..6d78e40a0 100644 --- a/server/internal/handler/comment_list_test.go +++ b/server/internal/handler/comment_list_test.go @@ -619,6 +619,36 @@ func TestListComments_RecentWithSinceKeepsCursorWhenPageHasRows(t *testing.T) { } } +// TestListComments_RecentWithSinceSuppressesCursorWhenHeadPastSince covers +// the mixed case from Elon's #2787 second review: the page is full (so the +// legacy `len(seen) >= N` check would emit a cursor) AND the `since` filter +// keeps rows from fresher threads (so the legacy `len(comments) == 0` +// suppression does NOT trip), but the head (oldest-active) thread already +// sits at or before `since`. Older pages walk strictly less-recent threads, +// so none of them can produce a `> since` comment — emitting a cursor here +// would still drag the caller into a guaranteed-empty next-page fetch. +// +// Fixture: root1 last_activity = base+3m, root2 last_activity = base+12m. +// recent=2 ⇒ both threads on the page, head = root1. since = base+5m drops +// everything in root1, keeps root2 entirely. Expect body = root2 thread, +// no cursor. +func TestListComments_RecentWithSinceSuppressesCursorWhenHeadPastSince(t *testing.T) { + if testHandler == nil || testPool == nil { + t.Skip("database not available") + } + fx := newCommentListFixture(t) + + v := url.Values{} + v.Set("recent", "2") + v.Set("since", fx.Base.Add(5*time.Minute).UTC().Format(time.RFC3339Nano)) + w, rows := listComments(t, fx.IssueID, v.Encode()) + eqIDs(t, ids(rows), []string{fx.Root2, fx.R2a, fx.R2b}, "recent=2 + since keeps root2 thread") + nb, nbid := nextThreadCursor(w) + if nb != "" || nbid != "" { + t.Fatalf("head thread (root1, last_activity = base+3m) is already <= since (base+5m); older pages can't beat it. cursor must be suppressed, got before=%q before_id=%q", nb, nbid) + } +} + // TestListComments_ThreadWithSinceFiltersWithinThread proves the allowed // combination from the rules: `thread + since` returns only comments in // that thread newer than `since`. The since filter is applied in-memory