Conversation
…ly Streak, enhancing user engagement through shareable links
…te others to join groups via shareable links
…echa/Flutter-Pecha into feature/mala-screen-layout
…into feature/mala-screen-layout
…g duplicate dispatches and enhancing user experience when navigating back to the home screen. tryng to solve the cold start loading issue when clicked back button.
…d UI responsiveness
…TimerCard for cleaner UI
…enhance local state management
…ounts, update documentation and provider logic
…ocal taps and updating related documentation
Task/add deep link to all share
…ultiple languages
Feature/mala screen layout
fix: minor bugs from review and consistency
…for consistent UI styling
…andling in mala feature
Feature/mala screen layout
refactor: enhance MissedDaysBadge widget for improved tap handling an…
Confidence Score: 3/5Safe to merge for the core offline-round and group-reset flows, but the sync-callback design has a real edge case where a second mala screen invalidates the wrong preset's provider. The single-slot
Reviews (1): Last reviewed commit: "Merge pull request #485 from OpenPecha/f..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
|
||
| /// Called after a group session count POST succeeds. Used to refresh lifetime | ||
| /// totals from `GET /accumulators/{id}/groups`. | ||
| void Function(String groupAccumulatorId)? onGroupCountSynced; |
There was a problem hiding this comment.
onGroupCountSynced is a single mutable field on the app-scoped MalaSyncManager. Every GroupAccumulationCountsNotifier constructor writes _sync.onGroupCountSynced = handleGroupCountSynced, so the last notifier to initialise silently overwrites any earlier one. If two mala screens for different preset IDs are simultaneously in the widget tree (e.g. push navigation stacking screens while the previous one is still mounted and watching its providers), the earlier preset's handler is lost. When a group POST completes, _pushGroupTotal calls onGroupCountSynced?.call(groupAccumulatorId), which invokes the surviving handler — invalidating joinedAccumulatorGroupsProvider for the wrong preset. The active preset's group counts would then not refresh after a sync.
The autoDispose + onDispose guard reduces exposure, but the guard only prevents the disposee from clearing the callback when it has already been overwritten — it does not prevent the overwrite itself from causing the wrong handler to fire.
| /// `GroupAccumulatorDetail.user.totalCount`. | ||
| final joinedGroupUserCountsProvider = FutureProvider.autoDispose | ||
| .family<Map<String, int>, String>((ref, presetId) async { | ||
| final groups = await ref.watch( | ||
| joinedAccumulatorGroupsProvider(presetId).future, | ||
| ); | ||
| if (groups.isEmpty) return const {}; | ||
|
|
||
| final repository = ref.watch(groupAccumulatorRepositoryProvider); | ||
| final entries = await Future.wait( | ||
| groups.map((group) async { | ||
| final result = await repository.getGroupAccumulator( | ||
| group.groupAccumulatorId, | ||
| ); | ||
| final count = result.fold( | ||
| (_) => 0, | ||
| (detail) => detail.user?.totalCount ?? 0, | ||
| ); | ||
| return MapEntry(group.groupAccumulatorId, count); | ||
| }), | ||
| ); | ||
| return Map.fromEntries(entries); | ||
| }); |
There was a problem hiding this comment.
joinedGroupUserCountsProvider issues one getGroupAccumulator call per joined group via Future.wait. This provider is re-fetched whenever joinedAccumulatorGroupsProvider is invalidated. The invalidation chain is: round complete → POST group count succeeds → onGroupCountSynced fires → joinedAccumulatorGroupsProvider invalidated (via handleGroupCountSynced) → joinedGroupUserCountsProvider re-watches it → N group-detail GETs fire in parallel.
For a user who completes rounds frequently and belongs to several groups, this generates a burst of N+1 requests on every 108-bead boundary. Consider batching on the server or caching the results with a TTL rather than re-fetching the full set after each sync.
| if (hasDirtyTail) unawaited(_sync.flush(SyncReason.launch)); | ||
| } | ||
|
|
||
| int countFor(String groupAccumulatorId, List<AccumulatorGroup> groups) { | ||
| int countFor(String groupAccumulatorId, [List<AccumulatorGroup>? groups]) { | ||
| final fromState = state[groupAccumulatorId]; | ||
| if (fromState != null) return fromState; | ||
|
|
||
| final userId = _userId; | ||
| if (userId != null) { |
There was a problem hiding this comment.
groups parameter is accepted but never used in countFor, increment, and addRounds
countFor was updated to read exclusively from in-memory state and Hive, dropping the old fallback to group.userTotalCount. The groups parameter is now optional and silently ignored. All three callers — increment, addRounds, and any external call sites — still pass a groups list that is dead code. This creates maintenance risk: future readers may assume the list is actually consulted and pass stale data thinking it matters. The parameter should be removed from countFor, increment, and addRounds to make the new ownership model explicit.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
3 participants
Uh oh!
There was an error while loading. Please reload this page.