[HackDay][Horizon] Refactor refresh logic

[domonkosadam]

No description provided.

[github-actions]

📊 Code Coverage Report

✅ Student

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

✅ Teacher

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

✅ Pandautils

• PR Coverage: 22.52%
• Master Coverage: 22.51%
• Delta: +0.00%

---

📈 Overall Average

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

[inst-danger]

Student Install Page

[andrasmaczak]

Looks better, but I think it could be improved to only refresh the widgets that needs to be refreshed. For example if the user selects an announcement, we only need to refresh that widget, don't need to refresh every other widget.

[inst-danger]

Student Install Page

[inst-danger]

Student Install Page

[claude]

Code Review: Refactor Refresh Logic

Summary

This PR refactors the refresh logic in the Horizon module by introducing event-based communication using event handlers instead of relying on Navigation's SavedStateHandle. This is a solid architectural improvement that decouples components and follows better practices for state management.

Positive Aspects ✅

Architecture Improvements

• Event-driven design: The introduction of DashboardEventHandler, InboxEventHandler, and LearnEventHandler provides a cleaner, more maintainable communication pattern between components
• Reduced coupling: Removing SavedStateHandle dependencies for cross-screen communication is a significant improvement
• Singleton pattern: Event handlers are properly scoped as @Singleton, ensuring consistent event delivery across the app
• SharedFlow usage: Using MutableSharedFlow with replay = 0 is appropriate for event streams where events shouldn't be replayed

Code Quality

• Comprehensive test coverage: Added tests for event handling in ViewModels (DashboardViewModelTest, HorizonInboxListViewModelTest, LearnViewModelTest)
• Proper cleanup: Removed unused constants and SavedStateHandle logic from screens
• Consistent patterns: The event handler pattern is consistently applied across Dashboard, Inbox, and Learn features

Areas for Improvement 🔍

1. Race Condition Risk in DashboardViewModel:46-62

dashboardEventHandler.events.collect { event ->
    when (event) {
        is DashboardEvent.DashboardRefresh -> {
            _uiState.update { it.copy(externalShouldRefresh = true) }
            refresh()
            delay(50) // Give some time for the refresh to start
            _uiState.update { it.copy(externalShouldRefresh = false) }
        }

Issue: The hardcoded delay(50) is a code smell that suggests timing-dependent behavior. This could fail if the UI takes longer than 50ms to observe the state change.

Recommendation: Consider using a state machine or callback-based approach:

is DashboardEvent.DashboardRefresh -> {
    _uiState.update { it.copy(externalShouldRefresh = true) }
    viewModelScope.launch {
        refresh()
        // Reset after refresh completes, not after arbitrary delay
        _uiState.update { it.copy(externalShouldRefresh = false) }
    }
}

2. Error Handling in Event Collectors

Multiple ViewModels collect events but don't handle potential exceptions:

• DashboardViewModel:46-62
• DashboardCourseViewModel:52-60
• HorizonInboxListViewModel:88-100

Recommendation: Wrap event collectors in try-catch or use catch operator on the flow to prevent crashes from unhandled exceptions.

3. Event Handler Testing Gap

While ViewModels are tested with event handlers, the event handlers themselves lack unit tests. Consider adding tests for:

• Thread safety of concurrent event posting
• Behavior with multiple collectors
• Event delivery guarantees

4. Missing newline at end of file

DashboardViewModel.kt:86 - File should end with a newline per Kotlin coding conventions.

5. Potential Memory Leak in DashboardScreen:112-122

LaunchedEffect(uiState.snackbarMessage) {
    if (uiState.snackbarMessage != null) {
        snackbarHostState.showSnackbar(
            message = uiState.snackbarMessage,
        )
        uiState.onSnackbarDismiss()
    }
}

Issue: If onSnackbarDismiss() updates the state while the LaunchedEffect is still running, it could trigger the effect again unnecessarily.

Recommendation: Consider using a consumable event pattern or checking if the snackbar is already dismissed before calling onSnackbarDismiss().

6. Inconsistent Event Naming

• DashboardEvent.DashboardRefresh vs DashboardEvent.ProgressRefresh - redundant prefix
• InboxEvent.RefreshRequested vs LearnEvent.RefreshRequested - better naming

Recommendation: Consider consistent naming like:

sealed interface DashboardEvent {
    data object Refresh : DashboardEvent
    data object RefreshProgress : DashboardEvent
    data class ShowSnackbar(val message: String) : DashboardEvent
}

Security Considerations 🔒

No security concerns identified. The event handlers don't handle sensitive data or expose security vulnerabilities.

Performance Considerations ⚡

• Positive: Event handlers use SharedFlow efficiently without unnecessary replay
• Minor concern: Multiple collectors on the same event handler could cause redundant work if not carefully managed
• Suggestion: Consider adding event filtering at the collector level if multiple components listen to the same events but only need specific ones

Test Coverage 📊

Strengths:

• Good coverage of event handling in ViewModels
• Tests verify state changes after events are posted
• Tests include both single and multiple event scenarios

Gaps:

• No integration tests for end-to-end event flow
• Event handlers themselves lack unit tests
• No tests for error scenarios (network failures during refresh, etc.)

Minor Issues

1. CourseProgressScreen:44-50 - Removed code references deleted constants, good cleanup
2. ProgramDetailsViewModel:360 - Event posting in nested launch could be flattened
3. ModuleItemSequenceViewModel:586-589 - Good use of posting multiple events for cross-feature updates

Recommendations Summary

1. Replace delay(50) with proper state management
2. Add error handling to event collectors
3. Add unit tests for event handlers
4. Fix newline at EOF in DashboardViewModel.kt
5. Review snackbar LaunchedEffect for potential re-triggering
6. Consider more consistent event naming conventions
7. Add integration tests for event flows

Overall Assessment

This is a well-executed refactoring that significantly improves the architecture. The event-driven approach is cleaner and more maintainable than the previous SavedStateHandle pattern. With the minor improvements suggested above, this will be production-ready code.

Recommendation: Approve with minor changes requested, particularly addressing the delay(50) race condition.

---

Review generated by Claude Code - Hackday PR Review

[claude]

Pull Request Review: [HackDay][Horizon] Refactor refresh logic

Summary

This PR successfully refactors the refresh logic from a SavedStateHandle-based approach to an event-driven architecture using dedicated EventHandler classes. This is a solid architectural improvement that eliminates tight coupling between screens and improves code maintainability.

---

✅ Strengths

Architecture & Design

• Event-driven architecture: The introduction of DashboardEventHandler, InboxEventHandler, and LearnEventHandler as @Singleton classes provides a clean, scalable communication pattern
• Separation of concerns: Removes the coupling between screens that was created by SavedStateHandle navigation-based communication
• Type-safe events: Sealed interfaces for events (e.g., DashboardEvent, InboxEvent) provide compile-time safety
• Consistent pattern: The same pattern is applied across Dashboard, Inbox, and Learn features

Code Quality

• Proper licensing: All new files include appropriate Apache 2.0 license headers
• Clean state management: UI state updates are handled through immutable data classes with copy()
• Code removal: Good cleanup of deprecated constants and old refresh mechanisms

Test Coverage

• Comprehensive tests: New test cases cover event handling scenarios
• Event verification: Tests properly verify that events trigger expected behavior (refresh, snackbar display)
• Multiple scenarios: Tests cover single events, multiple events, and state updates

---

⚠️ Issues & Concerns

1. Memory Leak Potential (High Priority)

Location: Multiple ViewModels (DashboardViewModel.kt:49-59, InboxListViewModel.kt:88-99, etc.)

viewModelScope.launch {
    dashboardEventHandler.events.collect { event ->
        // Event handling
    }
}

Issue: These collection jobs are launched in init blocks but never cancelled. Since the EventHandlers are @Singleton, the Flow collections will continue even after the ViewModel is cleared, potentially causing memory leaks.

Recommendation:

• Consider making EventHandlers scoped to their feature (e.g., @ViewModelScoped or custom scope) rather than @Singleton
• Or ensure proper cleanup by storing the Job and cancelling it in onCleared()
• Or use shareIn with appropriate replay and scope parameters

2. Race Condition in DashboardScreen (Medium Priority)

Location: DashboardScreen.kt:108-111

LaunchedEffect(uiState.externalShouldRefresh) {
    if (uiState.externalShouldRefresh) {
        shouldRefresh = true
        uiState.updateExternalShouldRefresh(false)
    }
}

Issue: There's a timing window where externalShouldRefresh is set to true, then immediately to false, which could cause missed refreshes if events arrive rapidly.

Recommendation: Consider using a Channel or SharedFlow with proper buffer configuration to ensure no events are lost.

3. Missing newline at end of file (Low Priority)

Location: DashboardViewModel.kt:112

The file ends without a newline character, which violates common coding standards.

Recommendation: Add a newline at the end of the file.

4. Empty catch blocks (Medium Priority)

Location: Multiple files (e.g., DashboardViewModel.kt:42-44, 87-89)

} catch {
    // Empty or just comment
}

Issue: Silent failure can make debugging difficult and hide potential issues.

Recommendation:

• Log errors at minimum
• Consider showing error state to user for critical operations
• If truly expected to fail silently, document why

5. Inconsistent event handling (Low Priority)

Location: DashboardViewModel.kt:51-57

when (event) {
    is DashboardEvent.DashboardRefresh -> {
        _uiState.update { it.copy(externalShouldRefresh = true) }
        refresh()
    }
    is DashboardEvent.ShowSnackbar -> showSnackbar(event.message)
    else -> { /* No-op */ }
}

Issue: The else branch with a no-op comment is unnecessary with sealed interfaces since all cases are covered.

Recommendation: Remove the else branch to get exhaustive checking benefits from sealed interfaces.

6. Potential snackbar message loss (Medium Priority)

Location: DashboardScreen.kt:115-121

LaunchedEffect(uiState.snackbarMessage) {
    if (uiState.snackbarMessage != null) {
        val result = snackbarHostState.showSnackbar(
            message = uiState.snackbarMessage,
        )
        if (result == SnackbarResult.Dismissed) {
            uiState.onSnackbarDismiss()
        }
    }
}

Issue: If multiple snackbar messages arrive quickly, only the latest will be shown. The previous message will be overwritten before being displayed.

Recommendation: Consider using a queue for snackbar messages or a Channel-based approach to ensure all messages are shown.

---

🔍 Additional Observations

Performance Considerations

• SharedFlow replay: MutableSharedFlow<DashboardEvent>(replay = 0) - This is appropriate for events that shouldn't be replayed to new subscribers. ✅
• Event collection: Multiple ViewModels collecting from singleton EventHandlers could have minor overhead, but acceptable for this use case.

Security Concerns

• No security issues identified. ✅
• Events don't carry sensitive data directly.

Testing

• Good coverage: Event-driven logic is well tested
• Missing edge cases: Consider testing:
  • Rapid event firing
  • ViewModel clearing during event collection
  • Error scenarios in event handlers

---

📝 Suggestions for Future Improvements

1. Documentation: Consider adding KDoc comments to EventHandler classes explaining their purpose and usage patterns
2. Centralized event bus: If more features need similar event handling, consider extracting a common EventBus interface
3. Metrics/Analytics: Consider logging events for debugging and analytics purposes
4. Error handling: Add structured error reporting for failed operations

---

Summary & Recommendation

This is a solid refactoring that improves the architecture significantly. The event-driven approach is cleaner and more maintainable than the SavedStateHandle pattern it replaces.

Blockers: The memory leak potential in #1 should be addressed before merging.

Non-blockers but recommended: Issues #2, #4, and #6 regarding error handling and race conditions.

Overall, this is good work! The pattern is consistent, well-tested, and improves code quality. With the memory leak concern addressed, this is ready to merge. 🚀

---

Review generated with Claude Code

[inst-danger]

Student Install Page