highperfocused/multica
1
0
Fork 0
mirror of https://github.com/multica-ai/multica.git synced 2026-07-05 13:29:44 +02:00
Code Issues Packages Projects Releases Wiki Activity

fix(comments): suppress recent+since cursor when head thread past since (MUL-2387)

Browse Source 
Previous suppression only tripped when the `since` filter emptied the
page. That missed the mixed case Elon flagged in #2787's second review:
the page keeps rows from fresher threads but the head (oldest-active)
thread already sits at or before `since`, so every older page is
guaranteed empty too. Predicating on `headLast <= since` covers both
cases.

Add a recent=2 + since fixture that pins the mixed scenario: root1
(last_activity = base+3m) is filtered out, root2 stays, and the cursor
is suppressed even though the body is non-empty.

Co-authored-by: multica-agent <github@multica.ai>
This commit is contained in:
Jiang Bohan
2026-05-18 19:56:57 +08:00
parent 669f259947
commit bb49820146
2 changed files with 40 additions and 10 deletions
Download Patch File Download Diff File Expand all files Collapse all files

20
server/internal/handler/comment.go
View File

@@ -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 {

30
server/internal/handler/comment_list_test.go
View File

@@ -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