feat: integrate RecitationFirstSegmentModel into routine items and enhance PracticeChantListTile for improved recitation handling#495
Conversation
…hance PracticeChantListTile for improved recitation handling
Confidence Score: 4/5The feature changes are self-contained and correct; the main thing to verify before merging is whether the pubspec.lock downgrades are intentional. The data-flow additions (SessionDTO → RoutineItem → PracticeChantListTile) are well-guarded and follow existing patterns. The UI refactoring splits item rendering cleanly by type with no observable behavioral regressions for existing item types. The only non-trivial concern is the unexplained downgrade of five transitive packages in pubspec.lock, which could silently diverge the team's dependency graph if merged without review. pubspec.lock (unexplained transitive downgrades) and practice_chant_list_tile.dart (data-mapper function placed in widget layer). Reviews (1): Last reviewed commit: "feat: integrate RecitationFirstSegmentMo..." | Re-trigger Greptile |
| RecitationModel recitationModelFromRoutineItem(RoutineItem item) { | ||
| return RecitationModel( | ||
| textId: item.id, | ||
| title: item.title, | ||
| language: item.language, | ||
| firstSegment: item.firstSegment, | ||
| ); | ||
| } |
There was a problem hiding this comment.
Data mapper placed in a UI file
recitationModelFromRoutineItem is a model-to-model conversion function but lives in practice_chant_list_tile.dart (a widget file). Both routine_filled_state.dart and routine_time_block.dart now import a widget file solely to access this helper, which inverts the typical dependency direction. If the tile is ever refactored or renamed, the import path in those two callers breaks. Moving this function to a utility module (e.g. routine_item_display.dart or a dedicated recitation_mapper.dart) would keep the widget layer free of data-layer consumers.
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!
| Widget _buildItemTile( | ||
| BuildContext context, | ||
| RoutineItem item, | ||
| int index, | ||
| bool isDark, | ||
| ) { | ||
| final dragHandle = ReorderableDragStartListener( | ||
| index: index, | ||
| child: GestureDetector( | ||
| onTapDown: (_) => HapticFeedback.heavyImpact(), | ||
| child: Icon( | ||
| AppAssets.list, | ||
| size: 22, | ||
| color: | ||
| isDark ? AppColors.textTertiaryDark : AppColors.textSecondary, | ||
| ), | ||
| ), | ||
| ); |
There was a problem hiding this comment.
dragHandle widget is always constructed, even for non-recitation items
dragHandle is built unconditionally at the top of _buildItemTile, but for non-recitation items it is immediately discarded — those items get their drag handle from RoutineItemCard via reorderIndex. The extra object allocation is negligible at runtime, but building a ReorderableDragStartListener (which internally looks up its SliverReorderableListState ancestor) in a branch that doesn't insert it into the tree is noisy. Consider moving the dragHandle construction inside the if (item.type == RoutineItemType.recitation) branch to make the intent clearer and avoid any unnecessary ancestor lookups.
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!
| sha256: f71061c654a3380576a52b451dd5532377954cf9dbd272a78fc8479606670803 | ||
| url: "https://pub.dev" | ||
| source: hosted | ||
| version: "1.4.1" | ||
| version: "1.4.0" | ||
| checked_yaml: | ||
| dependency: transitive |
There was a problem hiding this comment.
Several transitive packages were downgraded relative to the base branch: characters 1.4.1 → 1.4.0, matcher 0.12.19 → 0.12.17, material_color_utilities 0.13.0 → 0.11.1, meta 1.17.0 → 1.16.0, and test_api 0.7.10 → 0.7.6. Lock-file changes unrelated to a feature PR usually indicate the author's Flutter/Dart toolchain version differs from the rest of the team. If this branch is merged, CI environments and teammates running flutter pub get after the merge may get these older versions unexpectedly. It is worth confirming whether this was intentional or whether the lock file should be regenerated from the base branch's pubspec.yaml.
Uh oh!
There was an error while loading. Please reload this page.