Skip to content

Conversation

@tamaskozmer
Copy link
Contributor

@tamaskozmer tamaskozmer commented Oct 29, 2025

Summary

Implements the complete To Do List feature UI for the Student app using Jetpack Compose. The feature includes:

  • List view with planner items grouped by date
  • Sticky date headers with smooth scroll animation
  • Item cards showing title, context (course), date/time, type icon, and completion status
  • "Today" badge indicator for current date
  • Empty state with panda illustrations
  • Pull-to-refresh functionality
  • Complete data fetching and mapping from Canvas API
  • Comprehensive unit test coverage

Note: UI tests are intentionally not included as there are multiple follow-up stories that will continue to build on this feature. E2E tests will be added once the feature is more complete.

Test plan

  1. Enable the todo_redesign feature flag
  2. Navigate to the To Do List screen from the bottom navigation
  3. Verify planner items are displayed grouped by date
  4. Verify sticky headers appear and animate smoothly while scrolling
  5. Verify "Today" badge appears on today's date header
  6. Verify item cards show correct information (title, course, time, icon, checkbox state)
  7. Verify empty state shows panda illustrations when no items exist
  8. Test pull-to-refresh functionality
  9. Test in different screen sizes and orientations
  10. Run unit tests to verify all business logic

Manual Testing

  • Tested with various planner item types (assignments, quizzes, discussions, calendar events, planner notes)
  • Tested sticky header behavior during scrolling
  • Tested "Today" badge positioning and styling
  • Tested empty state with panda illustrations
  • Tested pull-to-refresh functionality
  • All 61 unit tests passing:
    • ToDoListViewModelTest: 20 tests
    • ToDoListRepositoryTest: 15 tests
    • PlannerItemExtensionsTest: 26 tests

🤖 Generated with Claude Code

Co-Authored-By: Claude [email protected]

refs: MBL-19399
affects: Student
release note: Added new To Do List view for students to see upcoming assignments and tasks

Checklist

  • Follow-up e2e test ticket created or not needed
  • Tested in dark mode
  • Tested in light mode
  • Test in landscape mode and/or tablet
  • A11y checked
  • Approve from product

@inst-danger
Copy link
Contributor

Parent Install Page

@inst-danger
Copy link
Contributor

Parent Install Page

@inst-danger
Copy link
Contributor

Parent Install Page

# Conflicts:
#	libs/pandares/src/main/res/values/strings.xml
@tamaskozmer
Copy link
Contributor Author

@claude

@claude
Copy link

claude bot commented Oct 29, 2025

Claude finished @tamaskozmer's task —— View job


Code Review Complete

I've conducted a comprehensive review of the To Do List UI implementation. Overall, this is a well-structured and thoughtfully implemented feature with excellent test coverage. Here are my findings:


✅ Strengths

  1. Excellent Architecture

    • Clean separation of concerns (Screen/ViewModel/Repository)
    • Proper use of MVVM pattern with Kotlin Flows
    • Well-structured UI state management
    • Router pattern correctly implemented following project conventions
  2. Comprehensive Test Coverage

    • 61 unit tests covering ViewModel (20), Repository (15), and Extensions (26)
    • Tests are well-written, isolated, and properly use Mockk
    • Good coverage of edge cases and error scenarios
  3. Code Reusability

    • Excellent refactoring of duplicate code from ToDoWidgetUpdater into shared extension functions in PlannerItemExtensions.kt (lines 34-111)
    • Proper code sharing between pandautils and student modules
  4. Feature Flag Integration

    • Clean feature flag implementation using RemoteConfigParam.TODO_REDESIGN
    • Graceful fallback to old implementation (OldToDoListFragment)
    • Navigation behavior properly adapts based on feature flag
  5. UI Implementation

    • Well-structured Compose components with proper state management
    • Sticky header animation implementation is sophisticated
    • Good use of derivedStateOf for performance optimization
    • Preview functions provided for development

🔍 Issues & Suggestions

High Priority

  1. Hardcoded Magic Numbers in Sticky Header Calculation (ToDoListScreen.kt:536-546)

    val textHeightPx = with(density) { 22.sp.toPx() }
    val circleHeightPx = with(density) { 32.dp.toPx() }
    val paddingPx = with(density) { 8.dp.toPx() }
    • Issue: These values are hardcoded separately from the actual DateBadge component dimensions
    • Risk: If DateBadge layout changes, the sticky header animation will break
    • Recommendation: Extract these as constants or calculate them dynamically from the component
  2. Memory Consideration in ToDoListContent (ToDoListScreen.kt:190-192)

    val itemPositions = remember { mutableStateMapOf<String, Float>() }
    val itemSizes = remember { mutableStateMapOf<String, Int>() }
    • Issue: With large lists, these maps could grow significantly
    • Recommendation: Consider limiting map size or implementing a cleanup strategy for off-screen items
  3. Date Formatting Not Locale-Safe (ToDoListScreen.kt:414-415)

    SimpleDateFormat("EEE", Locale.getDefault()).format(date)
    SimpleDateFormat("MMM", Locale.getDefault()).format(date)
    • Issue: These patterns may not work well for all locales
    • Recommendation: Consider using DateHelper utility or more robust date formatting
  4. TODO Count Filtering Not Implemented (CallbackActivity.kt:223)

    // TODO Implement correct filtering in MBL-19401
    • Issue: Badge count may be inaccurate until filtering is implemented
    • Impact: Users may see incorrect count in bottom navigation badge
    • Recommendation: Ensure follow-up ticket MBL-19401 is prioritized

Medium Priority

  1. Repository Methods Unused (ToDoListRepository.kt:56-80)

    • updatePlannerOverride() and createPlannerOverride() are defined but never called
    • Recommendation: Either remove if not needed yet, or add TODO comments linking to follow-up stories
  2. Empty ViewModel Action Handlers (ToDoListViewModel.kt:159-165)

    is ToDoListActionHandler.ToggleItemChecked -> {
        // TODO: Implement toggle checked - will be implemented in future story
    }
    • Issue: Filter and toggle functionality are stubbed out
    • Recommendation: Add TODO comments with ticket numbers for follow-up work
  3. Router Method Not Implemented (StudentToDoListRouter.kt:40-42)

    override fun openToDoItem(itemId: String) {
        // TODO: Implement navigation to specific to-do item based on item type
    }
    • Issue: Clicking items won't navigate anywhere
    • Impact: Core user interaction is incomplete
    • Recommendation: Should this be implemented before merging?
  4. Context Name Logic Discrepancy (PlannerItemExtensions.kt:77-98)

    • New implementation uses course.originalName check for nickname detection (line 79)
    • Widget implementation was removed but behavior differs slightly from new implementation
    • Widget used courseCode, new implementation uses course.name if nickname exists
    • Recommendation: Verify this behavior change is intentional

Low Priority

  1. Panda Illustration Space Calculation (ToDoListScreen.kt:214-219)

    val minSpaceForPandasPx = with(density) { 140.dp.toPx() }
    • The 140.dp threshold seems arbitrary
    • Recommendation: Document why this specific value was chosen
  2. Navigation Behavior Property Access (NavigationBehavior.kt:48-54)

    val todoFragmentClass: Class<out Fragment>
        get() {
            return if (RemoteConfigUtils.getBoolean(RemoteConfigParam.TODO_REDESIGN)) {
    • Issue: This property is accessed multiple times per navigation event, calling RemoteConfig each time
    • Performance: Not critical but could be optimized with caching
    • Recommendation: Consider caching the result during session
  3. Copyright Year Inconsistency

    • New files use "2025" but existing files use "2024"
    • Not an issue, just an observation

📝 Best Practices Observed

  • ✅ Proper use of Dagger Hilt for dependency injection
  • ✅ Follows CLAUDE.md guidelines (companion objects at bottom, no inline comments)
  • ✅ Consistent code style with existing codebase
  • ✅ Good error handling with try-catch in ViewModel
  • ✅ Proper use of collectOneOffEvents for ViewModel actions
  • ✅ Preview functions for both light and dark modes
  • ✅ Accessibility considerations (content descriptions, A11y checked in checklist)

🎯 Recommendations Before Merge

Critical:

  1. ✅ Verify sticky header animation works correctly with different date badge sizes
  2. ❓ Decide if openToDoItem() navigation should be implemented now or later
  3. ✅ Ensure follow-up tickets are created for TODOs (filter, toggle, navigation)

Nice to Have:
4. Consider adding inline documentation for the complex sticky header calculation logic
5. Add guards against potential memory leaks in large item lists


Summary

This is high-quality work that follows the project's architectural patterns and coding standards well. The implementation is clean, well-tested, and properly feature-flagged. The main concerns are around incomplete navigation/interaction features (which may be intentional per your follow-up story plan) and some hardcoded values in the sticky header animation.

Recommendation:Approve with minor suggestions - The issues identified are manageable and don't block merging if they're tracked in follow-up tickets.

Great job on the comprehensive test coverage and clean architecture! 🎉


| Branch: MBL-19399-to-do-list-ui

@inst-danger
Copy link
Contributor

Parent Install Page

@inst-danger
Copy link
Contributor

Teacher Install Page

@inst-danger
Copy link
Contributor

Student Install Page

@github-actions
Copy link

📊 Code Coverage Report

⚠️ Student

  • PR Coverage: 43.88%
  • Master Coverage: 44.06%
  • Delta: -0.18%

✅ Teacher

  • PR Coverage: 25.58%
  • Master Coverage: 25.58%
  • Delta: +0.00%

⚠️ Pandautils

  • PR Coverage: 22.43%
  • Master Coverage: 22.53%
  • Delta: -0.10%

📈 Overall Average

  • PR Coverage: 30.63%
  • Master Coverage: 30.72%
  • Delta: -0.09%

@inst-danger
Copy link
Contributor

Parent Install Page

@inst-danger
Copy link
Contributor

Teacher Install Page

@inst-danger
Copy link
Contributor

Student Install Page

@Composable
fun ToDoListScreen(
uiState: ToDoListUiState,
actionHandler: (ToDoListActionHandler) -> Unit,
Copy link
Contributor

Choose a reason for hiding this comment

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

instead of ToDoListActionHandler we could have functions in ToDoListUiState for the actions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is it okay if I refactor the architecture in the next ticket? I made quite a lot of changes and don't want to create unnecessary conflicts.

Copy link
Contributor

Choose a reason for hiding this comment

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

That’s fine, thank you.

Copy link
Contributor

@kristofnemere kristofnemere left a comment

Choose a reason for hiding this comment

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

QA+1

(A minor issue occurred once with the Panda illustration, but I wasn’t able to reproduce it. We’ll fix it later.)

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.

5 participants