[Watch + Up Next] Clear replaces properly when performing a login sync#3982
Draft
[Watch + Up Next] Clear replaces properly when performing a login sync#3982
Conversation
Collaborator
Generated by 🚫 Danger |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes a bug where stale Up Next replace actions persist through login sync and are incorrectly sent on subsequent syncs, potentially clearing the server's queue unexpectedly.
Changes:
- Added feature flag
clearPendingUpNextChangesOnLogin(defaulted to false for gradual rollout) - Modified
UpNextSyncTask.process()to clear all pending changes usingInt64.maxduring login sync when feature flag is enabled - Added comprehensive test coverage in
UpNextSyncTaskTeststo verify the fix works and doesn't break existing behavior - Added unit tests in
UpNextChangesDataManagerTeststo demonstrate the bug at the database layer
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| Modules/Server/Sources/PocketCastsServer/Public/Sync/UpNextSyncTask.swift | Implements the fix: uses Int64.max instead of latestActionTime (which is 0) during login sync to clear all pending changes when feature flag is enabled |
| Modules/Server/Tests/PocketCastsServerTests/UpNextSyncTaskTests.swift | New test file with comprehensive integration tests verifying the fix works, the bug exists when disabled, and normal sync behavior is preserved |
| Modules/DataModel/Tests/PocketCastsDataModelTests/UpNextChangesDataManagerTests.swift | Adds unit tests demonstrating the bug at the database layer and verifying the fix approach |
|
|
||
| clearSyncedData(latestActionTime: latestActionTime) | ||
| // For login syncs, clear all pending changes since we've applied server state | ||
| // and any pending changes are stale. See: https://github.com/Automattic/pocket-casts-ios/issues/XXXX |
There was a problem hiding this comment.
The GitHub issue URL contains a placeholder "XXXX" instead of an actual issue number. This should be replaced with the real issue number before merging.
Suggested change
| // and any pending changes are stale. See: https://github.com/Automattic/pocket-casts-ios/issues/XXXX | |
| // and any pending changes are stale after applying the latest server state. |
4aeceb6 to
f51129e
Compare
f51129e to
6473fe7
Compare
Contributor
|
Version |
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
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
clearPendingUpNextChangesOnLoginFeature Flag controls this change.The core bug here is that we were calling
clearSyncedData(latestActionTime: latestActionTime)wherelatestActionTimecould be0(its default value) after syncing during a login, effectively a no-op.The theory is the following:
replacechanges, I think the intent here is that another account could be logged in and send replace events to the wrong account.UpNextSyncTaskmakes a call toclearSyncedDatabut with the defaultlatestActionTimeof0, this leaves unsynced replace changes.Several of the watch logs indicate logouts which could potentially be more likely to trigger a case like this.
To test
Checklist
CHANGELOG.mdif necessary.