Fix two latent sync-dispatch bugs; implement updateService - #29
Conversation
Triage before wiring repositories into the sync queue turned up two defects
in SyncManager's dispatch, plus a wrong number in the roadmap.
**The roadmap's framing is wrong.** Track D says "wire queueOperation into
~34 OfflineException sites across 7 write repos". There are 25 such sites,
and only 11 are writes — the other 14 are cache-miss reads ("Bookings not
available offline"), which cannot be queued because there is nothing to
replay. Corrected in ROADMAP with the write sites enumerated.
**SERVICE/UPDATE replayed as a create.** `dispatchService` shared one branch
between CREATE and UPDATE, both calling `backendService.createService(dto)`.
A queued service edit — a price change, say — would have appended a second
listing rather than editing the first. The reason it shared a branch is that
`IBackendService` had no `updateService` at all, so this adds one (mirroring
`updateProvider` from #26) and splits the branches. That also lets
`ServiceRepository.updateService` stop returning
`Result.Error(Exception("Not implemented in backend service"))`, which is
the same hard-stub class as `updateProvider` before #26.
**PROVIDER_PROFILE would have silently truncated.** `dispatchProfile`
decodes every PROFILE payload as a `ClientDto`, and `json` is configured
with `ignoreUnknownKeys = true` — so a queued *provider* update would decode
successfully, drop bio/rating/serviceRadius/specializations/employer, and
write that truncated record to the clients collection. A new
`EntityType.PROVIDER_PROFILE` keeps them apart; `dispatchToBackend`'s `when`
is an expression, so the compiler required the new branch.
Neither bug is reachable today because nothing enqueues anything — which is
exactly why they survived. Both had to be fixed before wiring repositories
in, since wiring is what would make them reachable.
Also gave the stub a `serviceStore`, matching `providerStore`/`clientStore`:
`createService` returned its input without storing it, so a service created
in-session was invisible to every later read, and `createBooking` resolved
its price against the seed list only.
Tests: SERVICE/UPDATE reaches updateService and never createService;
PROVIDER_PROFILE round-trips the provider-only fields and never touches
updateClient; PROVIDER_PROFILE/CREATE joins the non-representable set.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01G6LvbheYzc1mN9toTuuPR5
CI failure on #29: adding EntityType.PROVIDER_PROFILE broke an exhaustive `when` in :ui that I did not check before pushing. e: ui/.../sync/SyncStatusScreen.kt:456:59 'when' expression must be exhaustive, add necessary 'PROVIDER_PROFILE' branch I verified the new constant against :core and :data and stopped there. The compiler caught it in :ui — which is the exhaustive `when` doing exactly its job, and the reason SyncManager's dispatch uses expression-form `when` in the first place. A whole-repo sweep now confirms both sites (the dispatcher and this icon map) cover all nine constants. Note :data:testDebugUnitTest ran and passed in the failing build, so the dispatch-routing tests added in be320c7 are green; only :ui:compileDebugKotlin failed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01G6LvbheYzc1mN9toTuuPR5
There was a problem hiding this comment.
Pull request overview
This PR fixes two latent sync-queue dispatch bugs in SyncManager (incorrect service update replay and provider profile payload truncation) and completes the backend/service write path by adding updateService plus consistent in-session service storage in the stub backend, as groundwork for wiring offline-write queue operations (Track A / decision 3A).
Changes:
- Add
IBackendService.updateServiceand implement it in both Firebase and stub backends; updateServiceRepository.updateServiceto call it and cache the result. - Fix sync dispatch routing: split
SERVICE/CREATEvsSERVICE/UPDATE, and introduceEntityType.PROVIDER_PROFILE+ dispatch path to prevent provider payloads decoding asClientDto. - Update roadmap framing and add tests covering
SERVICE/UPDATEdispatch andPROVIDER_PROFILEdispatch.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| ui/src/main/java/com/example/diamonds/ui/sync/SyncStatusScreen.kt | Adds UI icon mapping for the new PROVIDER_PROFILE entity type. |
| ROADMAP.md | Corrects Track A framing and enumerates queue-able write sites with replay status. |
| data/src/test/java/com/example/diamonds/data/sync/SyncManagerTest.kt | Adds test coverage for service-update dispatch and provider-profile dispatch routing. |
| data/src/main/java/com/example/diamonds/data/sync/SyncManager.kt | Fixes SERVICE/UPDATE dispatch and adds PROVIDER_PROFILE dispatch to avoid provider payload truncation. |
| data/src/main/java/com/example/diamonds/data/repository/ServiceRepository.kt | Implements updateService by delegating to backend and updating the local cache. |
| data/src/main/java/com/example/diamonds/data/remote/backend/IBackendService.kt | Introduces the updateService(ServiceDto) backend contract. |
| data/src/main/java/com/example/diamonds/data/remote/backend/FirebaseBackendService.kt | Implements updateService via Firestore .set() on the service document. |
| data/src/main/java/com/example/diamonds/data/remote/backend/BackendServiceStub.kt | Adds a mutable serviceStore and implements updateService; createService now stores created services. |
| data/src/main/java/com/example/diamonds/data/mapper/Mappers.kt | Adds Service.toDto() mapper for the new service update write path. |
| core/src/main/java/com/example/diamonds/domain/repository/Repositories.kt | Adds EntityType.PROVIDER_PROFILE with documentation explaining why it must be distinct from PROFILE. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Copilot review on #29, and correct on both halves. `BackendServiceStub.createService` appended unconditionally and returned its input unchanged, while `FirebaseBackendService.createService` generates an id when one is blank and overwrites the document when an id is supplied. The blank-id half is the sharper one. Handing back a DTO whose id is still "" means the caller maps it with `ServiceDto.toDomain()`, which as of #28 rejects a blank id outright — so the debug backend would fail a create that the Firebase backend completes. A behavioural divergence between the two backends is exactly what the stub exists to avoid, and this one only became reachable when the mapper validation landed. The duplicate half matters too: re-creating with an existing id showed the same service twice in search, where Firebase would have overwritten it. Tests: id generation when blank, upsert-not-append for a supplied id, and updateService editing in place — each asserted through a subsequent read rather than the return value, so the store is what is under test. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01G6LvbheYzc1mN9toTuuPR5
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated no new comments.
Suppressed comments (1)
data/src/main/java/com/example/diamonds/data/remote/backend/BackendServiceStub.kt:348
createServicegenerates ids usingSystem.currentTimeMillis(), which can collide if two creates occur within the same millisecond (possible in tests or fast repeated UI actions). A collision would overwrite the earlier service inserviceStore, producing hard-to-debug behavior. Using a UUID (or a monotonic counter) avoids this class of bug while still staying close to Firebase’s random document ids.
val saved =
if (service.id.isBlank()) service.copy(id = "s${System.currentTimeMillis()}") else service
Groundwork for wiring the sync queue (decision 3A). Triaging the write sites first turned up two defects in
SyncManager's dispatch — plus a wrong number in the roadmap.The roadmap's framing was wrong
Track A says "wire
queueOperation()into ~34OfflineExceptionsites across 7 write repos". There are 25 such sites, and only 11 are writes. The other 14 are cache-miss reads ("Bookings not available offline") — nothing to replay, nothing to queue. ROADMAP now carries the corrected list with per-site replay status.Two latent bugs, neither reachable today
Nothing enqueues anything, which is exactly why these survived. Both had to be fixed before wiring repositories in, since wiring is what makes them reachable.
1.
SERVICE/UPDATEreplayed as a create.dispatchServiceshared one branch betweenCREATEandUPDATE, both callingbackendService.createService(dto). A queued service edit — a price change, say — would have appended a second listing instead of editing the first.The reason it shared a branch:
IBackendServicehad noupdateServiceat all. So this adds one (mirroringupdateProviderfrom #26) and splits the branches. That also letsServiceRepository.updateServicestop returningResult.Error(Exception("Not implemented in backend service"))— the same hard-stub class asupdateProviderbefore #26.2.
PROVIDER_PROFILEwould have silently truncated.dispatchProfiledecodes everyPROFILEpayload as aClientDto, andjsonis configured withignoreUnknownKeys = true— so a queued provider update would decode successfully, dropbio/rating/serviceRadius/specializations/employer fields, and write that truncated record to theclientscollection. A newEntityType.PROVIDER_PROFILEkeeps them apart.dispatchToBackend'swhenis an expression, so the compiler required the new branch rather than letting it fall through.Stub consistency
createServicereturned its input without storing it, so a service created in-session was invisible to every later read, andcreateBookingresolved price against the seed list only. Now backed by aserviceStore, matching theproviderStore/clientStorepattern from #26.Tests
SERVICE/UPDATEreachesupdateServiceand nevercreateService;PROVIDER_PROFILEround-trips the provider-only fields and never touchesupdateClient;PROVIDER_PROFILE/CREATEjoins the existing non-representable set.What's next
The five idempotent write sites (
updateBookingStatus,cancelBooking,updateClient,updateProvider,updateService) are ready to wire once this lands. Three sites stay blocked for reasons worth stating plainly:createBooking— the server assigns the id and the queue has no way to reconcile it back into the optimistic local row, which would keep a fabricated id forever.createPayment— would tell a user their payment succeeded while offline with no processor contacted. That compounds the existing client-sideSUCCEEDEDfabrication rather than fixing it.createReview/createService— at-least-once replay can double-post a visible artefact. These need an idempotency key first.Verification
No Android SDK in this environment, so CI (
assembleDebug,allUnitTests,assembleDebugAndroidTest) is the compile signal — please treat a red run as blocking. Basedevelop@2a3e09b.🤖 Generated with Claude Code
https://claude.ai/code/session_01G6LvbheYzc1mN9toTuuPR5
Generated by Claude Code