Conversation
…line TEXT/IMAGE subtasks - Enhanced routing to support both TEXT and IMAGE content types in PlanTextScreen. - Updated comments and documentation to reflect the new inlineImage content type. - Improved navigation logic to seamlessly handle transitions between different content types. - Adjusted UI components to render inline images appropriately alongside text content.
… day details - Enhanced PlanDaysModel and UserPlanDayDetailResponse to include thumbnailUrl and shareableImageUrl fields. - Updated DayCompletionBottomSheet to handle sharing of images and display thumbnails. - Refactored plan completion logic to utilize new image fields for improved user experience.
…meters and introducing a dedicated navigation utility. the recitation text reader screen was inconsistent from the home and practice screen paths.
…ment handling in plan list view
…ent deep link generation for sharing text IDs
…nality with share text
… and synchronization
…Version Chore/plans bottom sheet sharable version
feat: add deep link support for series sharing and enhance routing
fix: bug fixes
…_more_bottom_sheet.dart Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com>
Feat/chants share
feat : acumulation leaderboard and changes in the group page
…ent-apis Refactor/series detail enrollment apis
Feat/verse of day deep link share
…updates in MalaSyncManager
Feature/mala screen layout
Confidence Score: 4/5Safe to merge; all findings are non-blocking quality concerns. The core sync, routing, and UI logic is well-structured and mirrors existing patterns. The identified issues are: a duplicate protected-route entry (harmless to auth), a Riverpod ref.watch-in-callback anti-pattern (functionally works but unclean), stale counts in the group accumulations sheet (narrow real-world window), and an incomplete leaderboard when paginating with the today sort (UX limitation, no data loss). group_accumulator_screen.dart (leaderboard sort + pagination interaction) and plan_list_view.dart (ref.watch in tap callback) deserve a second look before the next iteration. Reviews (1): Last reviewed commit: "Merge pull request #480 from OpenPecha/f..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| // Group accumulator counts (submit requires auth). | ||
| '/group-accumulators/', // Catch-all: POST count, GET detail, etc. | ||
|
|
||
| // Group join / follow | ||
| '/author/groups/{groupId}/join', | ||
| '/author/groups/{groupId}/follow', | ||
|
|
||
| // Group accumulators (group prayer accumulations) | ||
| '/group-accumulators/', |
There was a problem hiding this comment.
/group-accumulators/ is listed twice — once under "Group accumulator counts" and again under "Group accumulators". The duplicate has no functional impact (the route is protected either way) but creates maintenance confusion about which comment applies and suggests copy-paste residue.
| // Group accumulator counts (submit requires auth). | |
| '/group-accumulators/', // Catch-all: POST count, GET detail, etc. | |
| // Group join / follow | |
| '/author/groups/{groupId}/join', | |
| '/author/groups/{groupId}/follow', | |
| // Group accumulators (group prayer accumulations) | |
| '/group-accumulators/', | |
| // Group accumulator counts / detail (submit requires auth). | |
| '/group-accumulators/', // Catch-all: POST count, GET detail, join, etc. | |
| // Group join / follow | |
| '/author/groups/{groupId}/join', | |
| '/author/groups/{groupId}/follow', |
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!
| final id = seriesId; | ||
| if (id == null) { | ||
| _navigateToPlan(context, plan, null); | ||
| _navigateToPlan( | ||
| context, | ||
| plan: plan, | ||
| isEnrolled: _isPlanEnrolledFromMyData(ref, plan.id), | ||
| enrolledInfo: _getEnrolledInfoFromMyPlans(ref, plan.id), | ||
| ); | ||
| return; | ||
| } |
There was a problem hiding this comment.
_isPlanEnrolledFromMyData and _getEnrolledInfoFromMyPlans both call ref.watch internally, but they are being called here inside a tap callback rather than during build. In Riverpod, ref.watch outside of build behaves like ref.read (no subscription is set up), but it is still an anti-pattern that can cause unexpected behavior. The values isPlanEnrolled and enrolledInfo are already computed during build and can be used directly here via closure capture.
| final id = seriesId; | |
| if (id == null) { | |
| _navigateToPlan(context, plan, null); | |
| _navigateToPlan( | |
| context, | |
| plan: plan, | |
| isEnrolled: _isPlanEnrolledFromMyData(ref, plan.id), | |
| enrolledInfo: _getEnrolledInfoFromMyPlans(ref, plan.id), | |
| ); | |
| return; | |
| } | |
| final id = seriesId; | |
| if (id == null) { | |
| _navigateToPlan( | |
| context, | |
| plan: plan, | |
| isEnrolled: isPlanEnrolled, | |
| enrolledInfo: enrolledInfo, | |
| ); | |
| return; | |
| } |
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!
| child: Text( | ||
| context.l10n.mala_group_accumulations, | ||
| style: Theme.of(context).textTheme.titleMedium?.copyWith( | ||
| fontWeight: FontWeight.w700, |
There was a problem hiding this comment.
countsNotifier is obtained via ref.read, so the sheet never re-subscribes when new taps are recorded in the background. In practice this window is narrow (the sheet overlays the mala bead area), but if a tap fires via some other code path while the sheet is open, the displayed number will remain stale. Switching to watching the counts map directly would keep the displayed totals live.
| ); | ||
|
|
||
| return ListView.builder( | ||
| controller: _scrollController, | ||
| padding: const EdgeInsets.fromLTRB(16, 16, 16, 32), | ||
| itemCount: | ||
| 1 + |
There was a problem hiding this comment.
sortAccumulatorMembers re-orders the already-loaded list client-side, but loadInitial and loadMore always pass GroupAccumulatorMemberSort.total to the API. When a user switches to "Today" and then scrolls to trigger loadMore, the next page arrives sorted by total and is appended before the client re-sorts. Members with high today-counts but low total-counts will never appear, so the "Today" leaderboard is incomplete once more than one page is loaded. Consider disabling loadMore when sort is today, or re-fetching from page 0 when the sort changes.
3 participants
Uh oh!
There was an error while loading. Please reload this page.