Cap channel CIDs at 100 when calling getSyncHistory#6420
Open
VelikovPetar wants to merge 1 commit intodevelopfrom
Open
Cap channel CIDs at 100 when calling getSyncHistory#6420VelikovPetar wants to merge 1 commit intodevelopfrom
getSyncHistory#6420VelikovPetar wants to merge 1 commit intodevelopfrom
Conversation
Co-Authored-By: Claude <noreply@anthropic.com>
Contributor
PR checklist ✅All required conditions are satisfied:
🎉 Great job! This PR is ready for review. |
VelikovPetar
changed the title
getSyncHistory
Contributor
SDK Size Comparison 📏
|
WalkthroughThe PR introduces a hard limit of 100 channel IDs during sync operations. ChangesSync Channel ID Capping
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
stream-chat-android-client/src/test/java/io/getstream/chat/android/client/internal/state/internal/SyncManagerTest.kt (1)
227-246: ⚡ Quick winCover the raw-last-sync branch in the cap test too.
This test proves capping for the
Dateoverload, but not for theString(rawLastSyncedAt) overload that was also changed.Proposed test addition
+ `@Test` + fun `performSync caps channel_cids at 100 when rawLastSyncedAt exists`() = runTest(testDispatcher) { + /* Given */ + val nowDate = Date(currentTime) + val rawNowDate = streamDateFormatter.format(nowDate) + _syncState.value = SyncState( + userId = user.id, + activeChannelIds = emptyList(), + lastSyncedAt = nowDate, + rawLastSyncedAt = rawNowDate, + markedAllReadAt = nowDate, + ) + val cids = (1..150).map { "messaging:channel-$it" } + val expectedCids = cids.take(100) + + whenever(chatClient.getSyncHistory(any(), any<String>())) doReturn TestCall( + Result.Success(emptyList()), + ) + + val syncManager = buildSyncManager() + + /* When */ + syncManager.performSync(cids = cids) + + /* Then */ + verify(chatClient).getSyncHistory(eq(expectedCids), eq(rawNowDate)) + }🤖 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 `@stream-chat-android-client/src/test/java/io/getstream/chat/android/client/internal/state/internal/SyncManagerTest.kt` around lines 227 - 246, Add a sibling test that verifies capping when calling the String overload of performSync (rawLastSyncedAt) — create the same 150 cids, take the first 100 as expectedCids, mock chatClient.getSyncHistory to return a TestCall(Result.Success(emptyList())), call syncManager.performSync(cids = cids, rawLastSyncedAt = "<some-timestamp-string>") and verify chatClient.getSyncHistory was invoked with eq(expectedCids) and eq("<some-timestamp-string>"); reuse the existing setup (null sync state, repositoryFacade.selectSyncState returning null, buildSyncManager()) and mirror the existing test named `performSync caps channel_cids at 100` but target the rawLastSyncedAt overload.stream-chat-android-client/src/main/java/io/getstream/chat/android/client/internal/state/sync/internal/SyncManager.kt (1)
307-311: ⚡ Quick winEmit a warning when
channel_cidsare truncated.The cap is correct, but truncation is currently silent; a warning here helps explain missing sync coverage when users have >100 active channels.
Proposed tweak
val cappedCids = cids.take(SYNC_MAX_CIDS) + if (cids.size > SYNC_MAX_CIDS) { + logger.w { + "[performSync] cids truncated: requested=${cids.size}, capped=${cappedCids.size}" + } + } val result = if (rawLastSyncAt != null) { chatClient.getSyncHistory(cappedCids, rawLastSyncAt).await() } else { chatClient.getSyncHistory(cappedCids, lastSyncAt).await() }🤖 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 `@stream-chat-android-client/src/main/java/io/getstream/chat/android/client/internal/state/sync/internal/SyncManager.kt` around lines 307 - 311, The code silently truncates cids to SYNC_MAX_CIDS before calling getSyncHistory (cappedCids vs cids), so add a warning when cids.size > SYNC_MAX_CIDS: emit a log.warn in SyncManager (near the cappedCids calculation) that includes the original cids size, SYNC_MAX_CIDS, and optionally the truncated cids (cappedCids) to make missing sync coverage visible; keep existing call to chatClient.getSyncHistory(cappedCids, rawLastSyncAt ?: lastSyncAt).await().
🤖 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.
Nitpick comments:
In
`@stream-chat-android-client/src/main/java/io/getstream/chat/android/client/internal/state/sync/internal/SyncManager.kt`:
- Around line 307-311: The code silently truncates cids to SYNC_MAX_CIDS before
calling getSyncHistory (cappedCids vs cids), so add a warning when cids.size >
SYNC_MAX_CIDS: emit a log.warn in SyncManager (near the cappedCids calculation)
that includes the original cids size, SYNC_MAX_CIDS, and optionally the
truncated cids (cappedCids) to make missing sync coverage visible; keep existing
call to chatClient.getSyncHistory(cappedCids, rawLastSyncAt ?:
lastSyncAt).await().
In
`@stream-chat-android-client/src/test/java/io/getstream/chat/android/client/internal/state/internal/SyncManagerTest.kt`:
- Around line 227-246: Add a sibling test that verifies capping when calling the
String overload of performSync (rawLastSyncedAt) — create the same 150 cids,
take the first 100 as expectedCids, mock chatClient.getSyncHistory to return a
TestCall(Result.Success(emptyList())), call syncManager.performSync(cids = cids,
rawLastSyncedAt = "<some-timestamp-string>") and verify
chatClient.getSyncHistory was invoked with eq(expectedCids) and
eq("<some-timestamp-string>"); reuse the existing setup (null sync state,
repositoryFacade.selectSyncState returning null, buildSyncManager()) and mirror
the existing test named `performSync caps channel_cids at 100` but target the
rawLastSyncedAt overload.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: b37245ad-02f6-468e-b8bb-9891953cddfb
📒 Files selected for processing (2)
stream-chat-android-client/src/main/java/io/getstream/chat/android/client/internal/state/sync/internal/SyncManager.ktstream-chat-android-client/src/test/java/io/getstream/chat/android/client/internal/state/internal/SyncManagerTest.kt
|
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.
Goal
The
getSyncHistoryendpoint enforces a server-side limit on the number ofchannel_cidsit accepts. When users have a large number of active channels, the SDK was forwarding the full list, which could lead to failed sync requests.Note: Aligns with the iOS limit set in: GetStream/stream-chat-swift#3863
Implementation
In
SyncManager.performSync, cap thecidslist at 100 (SYNC_MAX_CIDS) before passing it tochatClient.getSyncHistory. Both code paths (with and withoutrawLastSyncAt) use the capped list.UI Changes
No UI changes.
Testing
performSync caps channel_cids at 100inSyncManagerTestverifies that when 150 cids are passed in, only the first 100 reachgetSyncHistory.Summary by CodeRabbit
Bug Fixes
Tests