fix: backfill only worked for the 1st page of members#194
Conversation
|
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds Paginated message-box sync and member chat MID cache
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![indent-zero[bot]](https://avatars.githubusercontent.com/in/1426746?s=60&v=4){kind=small}
| } | ||
|
|
||
| for _, box := range res.MessageBoxes { | ||
| for _, box := range messageBoxes { |
There was a problem hiding this comment.
Latent: duplicate ChatResync on overlapping pages. fetchAllMessageBoxes uses the last box's ID as the next MinChatID; if LINE treats minChatId as inclusive (a common cursor convention — the protocol isn't documented either way), each page will repeat the previous page's last box, and syncDMChats will emit ChatResync twice for that DM. seenCursors only stops the loop when the cursor doesn't advance at all, so overlap-by-one passes through. The framework should treat ChatResync as idempotent so this is benign today, but prefetchMessages already dedupes via collectStartupBackfillChatMIDs — consider doing the same here for symmetry, or dropping the last box from each non-final page in fetchAllMessageBoxes.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@pkg/connector/sync.go`:
- Around line 65-70: The pagination logic in sync/backfill helpers is treating
an empty or repeated cursor as a successful end-of-data case, which can silently
truncate results. Update the cursor handling in the helper that reads
res.MessageBoxes and tracks seenCursors so that when HasNext is true but
nextCursor is empty or already seen, it returns an error instead of boxes, nil.
Make sure callers can distinguish a broken cursor from a normal completion by
surfacing a clear failure from this pagination path.
🪄 Autofix (Beta)
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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: cc0e8fb9-23b5-4bcf-b59d-f28aa4a5f077
📒 Files selected for processing (3)
pkg/connector/client.gopkg/connector/sync.gopkg/line/structs.go
📜 Review details
⏰ Context from checks skipped due to timeout. (4)
- GitHub Check: Lint with 1.25
- GitHub Check: build-docker
- GitHub Check: build-docker
- GitHub Check: Lint with 1.25
🧰 Additional context used
📓 Path-based instructions (2)
**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
**/*.go: Usego fmtfor code formatting across all Go files
Usegoimportswith-local "github.com/highesttt/matrix-line-messenger"flag to group project-local imports correctly
Usezerologfor logging throughout the codebase
Do not useMsgfin logging; useMsgwith structured fields instead
UseStringerinterface where applicable in Go code
Files:
pkg/connector/client.gopkg/line/structs.gopkg/connector/sync.go
**/!(ltsm)/**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
**/!(ltsm)/**/*.go: Runstaticcheckon all Go files excludingpkg/ltsmpackage (transpiled WASM code)
Rungo veton all Go files excludingpkg/ltsmpackage (transpiled WASM code)
Files:
pkg/connector/client.gopkg/line/structs.gopkg/connector/sync.go
🔇 Additional comments (2)
pkg/line/structs.go (1)
276-281: LGTM!pkg/connector/client.go (1)
37-47: LGTM!Also applies to: 261-263
| nextCursor := res.MessageBoxes[len(res.MessageBoxes)-1].ID | ||
| if nextCursor == "" { | ||
| return boxes, nil | ||
| } | ||
| if _, ok := seenCursors[nextCursor]; ok { | ||
| return boxes, nil |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Don't treat a broken pagination cursor as success.
If HasNext is still true but the API returns an empty or repeated ID, this helper returns boxes, nil and the callers proceed with a silently truncated sync/backfill result. That recreates the “first page only” failure mode without any signal.
Suggested fix
nextCursor := res.MessageBoxes[len(res.MessageBoxes)-1].ID
if nextCursor == "" {
- return boxes, nil
+ return nil, fmt.Errorf("message-box pagination returned empty cursor with hasNext=true")
}
if _, ok := seenCursors[nextCursor]; ok {
- return boxes, nil
+ return nil, fmt.Errorf("message-box pagination cursor %q repeated", nextCursor)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| nextCursor := res.MessageBoxes[len(res.MessageBoxes)-1].ID | |
| if nextCursor == "" { | |
| return boxes, nil | |
| } | |
| if _, ok := seenCursors[nextCursor]; ok { | |
| return boxes, nil | |
| nextCursor := res.MessageBoxes[len(res.MessageBoxes)-1].ID | |
| if nextCursor == "" { | |
| return nil, fmt.Errorf("message-box pagination returned empty cursor with hasNext=true") | |
| } | |
| if _, ok := seenCursors[nextCursor]; ok { | |
| return nil, fmt.Errorf("message-box pagination cursor %q repeated", nextCursor) | |
| } |
🤖 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 `@pkg/connector/sync.go` around lines 65 - 70, The pagination logic in
sync/backfill helpers is treating an empty or repeated cursor as a successful
end-of-data case, which can silently truncate results. Update the cursor
handling in the helper that reads res.MessageBoxes and tracks seenCursors so
that when HasNext is true but nextCursor is empty or already seen, it returns an
error instead of boxes, nil. Make sure callers can distinguish a broken cursor
from a normal completion by surfacing a clear failure from this pagination path.
1 participant
No description provided.