Skip to content

feat: integrate RecitationFirstSegmentModel into routine items and enhance PracticeChantListTile for improved recitation handling#495

Merged
tentamdin merged 1 commit into
developfrom
feat/routine-chant-list-tile-ui
Jul 3, 2026
Merged

feat: integrate RecitationFirstSegmentModel into routine items and enhance PracticeChantListTile for improved recitation handling#495
tentamdin merged 1 commit into
developfrom
feat/routine-chant-list-tile-ui

Conversation

@tentamdin

@tentamdin tentamdin commented Jul 3, 2026

Copy link
Copy Markdown
Member
  • update practice screen recitation UI

…hance PracticeChantListTile for improved recitation handling
@greptile-apps

greptile-apps Bot commented Jul 3, 2026

Copy link
Copy Markdown
Contributor

Confidence Score: 4/5

The 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

Comment on lines +9 to +16
RecitationModel recitationModelFromRoutineItem(RoutineItem item) {
return RecitationModel(
textId: item.id,
title: item.title,
language: item.language,
firstSegment: item.firstSegment,
);
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 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!

Comment on lines +162 to +179
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,
),
),
);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 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!

Comment thread pubspec.lock
Comment on lines +224 to 229
sha256: f71061c654a3380576a52b451dd5532377954cf9dbd272a78fc8479606670803
url: "https://pub.dev"
source: hosted
version: "1.4.1"
version: "1.4.0"
checked_yaml:
dependency: transitive

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Transitive package downgrades

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.

@tentamdin tentamdin merged commit c27beeb into develop Jul 3, 2026
1 of 2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant