eddy/weegschaal-app
Watch
1
0
Fork
You've already forked weegschaal-app
 0
Code Issues 12 Pull requests Projects Releases Packages Wiki Activity

[CRITICAL] mergeMeasurements is dode code — syncAll gebruikt eigen inline merge #3

New issue
Closed
opened 2026-08-05 21:11:31 +02:00 by eddy · 2 comments
eddy commented 2026-08-05 21:11:31 +02:00
Owner
Copy link

Probleem

src/sync/syncService.ts bevat twee onafhankelijke merge-implementaties:

  1. mergeMeasurements() (regel 22-57): geëxporteerd, getest met 10+ unit tests in tests/syncService.test.ts
  2. Inline merge in syncAll() (regel 110-157): de daadwerkelijke productiecode die bij elke sync draait

De twee implementaties verschillen in gedrag:

  • mergeMeasurements zet deletedAt op het resultaat bij remote deleted records
  • syncAll's inline merge roept direct removeMeasurement() aan

Gevolg: De unit tests valideren code die nooit in productie draait. Een wijziging in mergeMeasurements zou alle tests laten slagen maar geen effect hebben op runtime-gedrag.

Oplossing

Refactor syncAll() zodat het mergeMeasurements() gebruikt als canonieke merge-functie. De flow wordt:

  1. Pull remote records via API
  2. Haal alle lokale records op (getAllMeasurementsForSync())
  3. Roep mergeMeasurements(local, remote) aan → dit retourneert de gemergde lijst
  4. Loop over het resultaat: records met deletedAt → removeMeasurement(), overige → upsertMeasurement()

Dit elimineert de code-duplicatie en maakt de bestaande tests weer betekenisvol.

NB: mergeMeasurements is een pure functie die alleen data transformeert — de DB-operaties gebeuren pas in syncAll na de merge.

## Probleem `src/sync/syncService.ts` bevat twee onafhankelijke merge-implementaties: 1. **`mergeMeasurements()`** (regel 22-57): geëxporteerd, getest met 10+ unit tests in `tests/syncService.test.ts` 2. **Inline merge in `syncAll()`** (regel 110-157): de daadwerkelijke productiecode die bij elke sync draait De twee implementaties verschillen in gedrag: - `mergeMeasurements` zet `deletedAt` op het resultaat bij remote deleted records - `syncAll`'s inline merge roept direct `removeMeasurement()` aan **Gevolg:** De unit tests valideren code die nooit in productie draait. Een wijziging in `mergeMeasurements` zou alle tests laten slagen maar geen effect hebben op runtime-gedrag. ## Oplossing Refactor `syncAll()` zodat het `mergeMeasurements()` gebruikt als canonieke merge-functie. De flow wordt: 1. Pull remote records via API 2. Haal alle lokale records op (`getAllMeasurementsForSync()`) 3. Roep `mergeMeasurements(local, remote)` aan → dit retourneert de gemergde lijst 4. Loop over het resultaat: records met `deletedAt` → `removeMeasurement()`, overige → `upsertMeasurement()` Dit elimineert de code-duplicatie en maakt de bestaande tests weer betekenisvol. NB: `mergeMeasurements` is een pure functie die alleen data transformeert — de DB-operaties gebeuren pas in `syncAll` na de merge.
eddy commented 2026-08-06 04:43:24 +02:00
Author
Owner
Copy link

Opgelost in PR #19: #19

Wat is er gedaan:

  • Pull-fase van syncAll() gerefactored: loopt nu over het resultaat van mergeMeasurements(local, remote) i.p.v. handmatige inline merge
  • 50 regels inline merge-logica verwijderd, vervangen door 17 regels die de canonieke merge-functie aanroepen
  • Remote deletion LWW-bug opgelost (timestamps worden nu correct vergeleken, lokale recentere records worden niet meer onterecht verwijderd)
  • Remote deletions worden nu als pulled geteld (voorheen deleted in pull-fase)

Verificatie:

  • 16/16 tests ✅ (waarvan 11 mergeMeasurements tests die nu echte productielogica testen)
  • TypeScript: clean
  • Manueel getest op Samsung SM-A266B via expo run:android
Opgelost in PR #19: https://git.eddydevink.nl/eddy/weegschaal-app/pulls/19 **Wat is er gedaan:** - Pull-fase van `syncAll()` gerefactored: loopt nu over het resultaat van `mergeMeasurements(local, remote)` i.p.v. handmatige inline merge - 50 regels inline merge-logica verwijderd, vervangen door 17 regels die de canonieke merge-functie aanroepen - Remote deletion LWW-bug opgelost (timestamps worden nu correct vergeleken, lokale recentere records worden niet meer onterecht verwijderd) - Remote deletions worden nu als `pulled` geteld (voorheen `deleted` in pull-fase) **Verificatie:** - 16/16 tests ✅ (waarvan 11 `mergeMeasurements` tests die nu echte productielogica testen) - TypeScript: clean - Manueel getest op Samsung SM-A266B via `expo run:android`
eddy closed this issue 2026-08-06 04:51:35 +02:00
eddy commented 2026-08-06 04:51:46 +02:00
Author
Owner
Copy link

Opgelost in PR #19: de syncAll pull-fase gebruikt nu mergeMeasurements() als canonieke merge. Dit elimineert de dubbele inline merge-logica en lost de LWW-bug bij remote deletions op.

Opgelost in PR #19: de syncAll pull-fase gebruikt nu mergeMeasurements() als canonieke merge. Dit elimineert de dubbele inline merge-logica en lost de LWW-bug bij remote deletions op.
Sign in to join this conversation.
No Branch/Tag specified
Branches Tags
main
fix/issue-3-mergeMeasurements-refactor
fix/code-review-issues
No results found.
Labels
Clear labels   
bug
Something isn't working

  
duplicate
This issue or pull request already exists

  
enhancement
New feature or request

  
good first issue
Good for newcomers

  
help wanted
Extra attention is needed

  
invalid
This doesn't seem right

  
question
Further information is requested

  
wontfix
This will not be worked on

No labels
bug
duplicate
enhancement
good first issue
help wanted
invalid
question
wontfix
Milestone
Clear milestone
No items
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference
eddy/weegschaal-app#3
No description provided.