Add mobile duplicate push payload guards#491
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces local deduplication and preflight validation for push payloads in SyncManager to prevent duplicate key errors during portal synchronization. It adds dedupeWorkoutSessionsById to filter out duplicate workout sessions and rejectDuplicatePushPayloadKeys to fail the sync early if duplicate nested keys are detected. Feedback recommends making the workout session deduplication case-insensitive to align with the preflight validation, extending duplicate checks to routines and cycles, and updating unit tests to verify case-insensitive deduplication.
There was a problem hiding this comment.
Pull request overview
Adds defensive guards to prevent duplicate IDs from producing invalid portal sync push payloads, and ensures repeated WorkoutSession.id rows don’t cause duplicate exercise IDs or duplicate post-push stamping.
Changes:
- Deduplicate
WorkoutSessionrows byidbefore building portal push payloads and before post-push timestamp stamping. - Add a local preflight that detects (case-insensitive) duplicate IDs across push payload tables (
workout_sessions,exercises,sets,rep_summaries,rep_telemetry) and fails fast withPortalApiException. - Add regression tests covering duplicate session IDs, nested duplicate exercise IDs, and stable exercise ID derivation.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| shared/src/commonMain/kotlin/com/devil/phoenixproject/data/sync/SyncManager.kt | Adds session deduping and push-payload duplicate-key preflight before network push. |
| shared/src/commonTest/kotlin/com/devil/phoenixproject/testutil/FakeSyncRepository.kt | Tracks post-push stamping calls to assert deduped stamping behavior in tests. |
| shared/src/commonTest/kotlin/com/devil/phoenixproject/data/sync/SyncManagerTest.kt | Adds sync regression tests for deduping and local duplicate-key rejection. |
| shared/src/commonTest/kotlin/com/devil/phoenixproject/data/sync/PortalSyncAdapterTest.kt | Adds test ensuring exercise IDs remain stable (derived from workout session ID). |
| shared/src/commonTest/kotlin/com/devil/phoenixproject/data/sync/PortalPushLimitsTest.kt | Adds test for case-insensitive duplicate detection across all push payload tables. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 357d57c3b1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| val pushedSessions = dedupeWorkoutSessionsById( | ||
| syncRepository.getWorkoutSessionsModifiedSince( | ||
| prePushLastSync, | ||
| activeProfileId, | ||
| ), | ||
| context = "Post-push stamping", | ||
| ) |
There was a problem hiding this comment.
Stamp dropped duplicate session aliases
When the duplicate differs only by case (the new test even uses id and id.uppercase()), this dedupes the post-push list before calling updateSessionTimestamp, so only the kept casing is marked synced. The SQL update is an exact WHERE id = ?, while the delta query keeps returning rows with updatedAt IS NULL; therefore the dropped alias is selected on the next sync and can be pushed as a standalone payload, defeating the duplicate guard and potentially overwriting the portal row you meant to suppress.
Useful? React with 👍 / 👎.
Code Review Roast 🔥Verdict: No New Issues Found | Recommendation: Merge Oh wait, this PR is actually clean. I need to sit down. I had my flamethrower warmed up and everything. The duplicate detection logic is solid, the tests are comprehensive, and the code follows existing patterns. The existing comment about locale-sensitive 📊 Overall: Like finding a unicorn in production — I didn't think clean PRs existed anymore, but here we are. Files Reviewed (4 files)
Reviewed by laguna-m.1-20260312:free · 1,451,782 tokens |
Summary
WorkoutSession.idvalues before building portal push payloads and before post-push timestamp stamping.workout_sessions,exercises,sets,rep_summaries, andrep_telemetrythat fails fast withPortalApiExceptioninstead of sending a doomed request.Testing
:shared:testAndroidHostTest --tests com.devil.phoenixproject.data.sync.SyncManagerTest:shared:testAndroidHostTest --tests com.devil.phoenixproject.data.sync.PortalPushLimitsTest:shared:validateSchemaManifest :shared:testAndroidHostTest