[fix]: Prevent dropped selections and fix cross-type count in metadata table#1070
[fix]: Prevent dropped selections and fix cross-type count in metadata table#1070deeonwuli wants to merge 9 commits into
Conversation
BundleMonNo change in files bundle size Groups updated (1)
Final result: ✅ View report in BundleMon website ➡️ |
gqcorneby
left a comment
gqcorneby
left a comment
There was a problem hiding this comment.
Thanks @deeonwuli! Works much better now without the long delay 👍
Just a few minor comments
| const sync: GenericSyncUseCase = this.compositionRoot.sync[rule.type](rule.toBuilder()); | ||
|
|
||
| const aggregateDataExchanges = rule.aggregatedDataExchanges?.map(ade => ade.id) || []; | ||
| // unused |
There was a problem hiding this comment.
Looks like Domain and Data layer changes are all just clean up 👍
I just did a quic chec on this but I think we can safely remove it instead of commenting it out.
Initially updated here #1039 then orphaned here #1047
getSourceData could also be cleaned up, I think. Can be done in a separate cleanup task
| }, [compositionRoot.metadata, metadataModelsSyncAll, remoteInstance, syncRule.metadataIds]); | ||
|
|
||
| let cancelled = false; | ||
| compositionRoot.metadata.getByIds(onlyMetadataIds, remoteInstance, "id").then(metadata => { |
There was a problem hiding this comment.
Should we add a .catch here? This sets the rule's metadata types + aggregation flag, so a silent failure leaves the rule in a stale state. Probably worth surfacing via the existing snackbar like the other effects.
| .then(pkg => setOtherTypeCountFallback(countPackageOtherType(pkg, collectionName))); | ||
| }, [collectionName, selectedIds, compositionRoot, remoteInstance]); | ||
| let cancelled = false; | ||
| compositionRoot.metadata.getByIds(selectedIds, remoteInstance, "id").then(pkg => { |
There was a problem hiding this comment.
Whenever a filter is active this re-resolves the whole selectedIds set via getByIds on every selection change, even though only one id is new per click. I'm not sure how large selections get in normal use here. If it's small it's probably fine, but the cost scales with the total count (server resolution + payload + parsing), not just the request count, so even if ids are < 50 it's doing more work than the one changed id needs.
Is it worth caching type-resolution by id and only resolving newly-added ids? I think there's a similar implementation in dhis_tally_sheets useDataSets. Let me know what you think! If it's too much work then maybe it's not wroth the effort.
There was a problem hiding this comment.
Got it and implemented now, only new IDs are resolved per selection change instead of the full set each time. 👍
gqcorneby
left a comment
gqcorneby
left a comment
There was a problem hiding this comment.
Thanks @deeonwuli! Just some nit-picks but all good!
| // When `ids` is not the full id list for the current type (filter active, or collection | ||
| // never populates ids like organisationUnits), set-difference miscounts same-type | ||
| // selections outside the filter as cross-type. Fall back to resolving types via the API. | ||
| const useApiCrossTypeCount = !idsAreComplete; |
There was a problem hiding this comment.
[nit] use is kind of a special word that implies a hook? Maybe we can rename it to resolveCrossTypeViaApi or something
| let cancelled = false; | ||
| compositionRoot.metadata.getByIds(selectedIds, remoteInstance, "id").then(pkg => { | ||
| if (!cancelled) setOtherTypeCountFallback(countPackageOtherType(pkg, collectionName)); | ||
| compositionRoot.metadata.getByIds(uncachedIds, remoteInstance, "id").then(pkg => { |
There was a problem hiding this comment.
[nit] pkg is a bit cryptic. Maybe metadataByType?
|
@deeonwuli Can you please merge with the development branch, please? |
…ynchronization into fix/metadata-selection-race-condition
…election-race-condition
4 participants
📌 References
📝 Implementation
getByIdscall, so rapid clicks merged off stale state and overwrote each other. Selection now updates synchronously viauseLatestRef.Pre-existing issues flagged by in PR #1067.
📹 Screenshots/Screen capture
Screen.Recording.2026-06-10.at.19.54.00.mov
🔥 Is there anything the reviewer should know to test it?
📑 Others
#869dm6r9a