Decouple storybooks retrieval logic from UI #265
Conversation
WalkthroughThis change introduces a ViewModel-driven architecture for loading and displaying storybooks in the app. New repository and data source abstractions are implemented, with Dagger Hilt modules set up for dependency injection. The UI now observes state from the ViewModel, reflecting asynchronous data loading and success states. Changes
Sequence Diagram(s)sequenceDiagram
participant UI as StoryBooksActivity
participant VM as StoryBookViewModelImpl
participant Repo as StoryBookRepository
participant DS as LocalStoryBooksDataSource
UI->>VM: getAllStoryBooks()
VM->>VM: Emit Loading state
VM->>Repo: getStoryBooks()
Repo->>DS: getStoryBooks()
DS-->>Repo: List<StoryBookGson>
Repo-->>VM: List<StoryBookGson>
VM->>VM: Emit Success state with storybooks
VM-->>UI: uiState emits Success
UI->>UI: showStoryBooks(storyBooks)
Suggested labels
Suggested reviewers
elimu.ai's mission is to build innovative learning software that empowers out-of-school children to teach themselves basic reading📖, writing✍🏽 and math🔢 within 6 months. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (8)
🚧 Files skipped from review as they are similar to previous changes (8)
⏰ Context from checks skipped due to timeout of 90000ms (9)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
app/src/main/java/ai/elimu/vitabu/ui/StoryBooksActivity.kt (1)
99-217:⚠️ Potential issueUnscoped
CoroutineScope(Dispatchers.IO)can leak work past Activity destructionLaunching a new root scope here means:
• Work is not cancelled when the Activity stops/finishes
• Potential memory leaks and wasted battery if the user navigates away quicklyPrefer
lifecycleScope.launch(Dispatchers.IO) { … }, which is automatically cancelled with the Activity, or move the heavy work to the ViewModel.- CoroutineScope(Dispatchers.IO).launch { + lifecycleScope.launch(Dispatchers.IO) {Inside the loop you can replace the nested
CoroutineScope(Dispatchers.IO).launchwithwithContext(Dispatchers.IO)to avoid spawning an unbounded number of coroutines.These tweaks keep the app efficient while it works toward elimu.ai’s mission of empowering learners on resource-constrained devices.
🧹 Nitpick comments (5)
app/src/main/java/ai/elimu/vitabu/viewmodel/StoryBookViewModel.kt (1)
26-29: Clarify contract naming & intentBecause
StoryBookViewModelis an interface, newcomers may assume it’s the concrete AndroidXViewModelsubclass. Consider renaming toStoryBookViewModelContract(or prefixing withI) or keep the name but add KDoc that stresses it is a contract. This avoids confusion and keeps room for a real@HiltViewModelimplementation class.app/src/main/java/ai/elimu/vitabu/data/repository/LocalStoryBooksDataSourceImpl.kt (1)
9-16: Tie heavy I/O to the IO dispatcher & scope
ContentProviderUtil.getAllStoryBookGsons()performs disk / IPC I/O. Although the surrounding API issuspend, nothing here forces the call ontoDispatchers.IO. Make the intention explicit (or inject a dispatcher) so accidentalwithContext(Dispatchers.Main)calls from upper layers don’t block the UI thread.override suspend fun getStoryBooks(): List<StoryBookGson> = withContext(Dispatchers.IO) { ContentProviderUtil.getAllStoryBookGsons( context, BuildConfig.CONTENT_PROVIDER_APPLICATION_ID ) }Also consider annotating the class with
@Singletonand moving the binding toSingletonComponentif its state is stateless and safe to share.app/src/main/java/ai/elimu/vitabu/di/DataModule.kt (1)
13-24: Scope mismatch may cause duplicate instancesThe module is installed in
ViewModelComponent, so each ViewModel instance gets its own repository and data-source. Unless these classes hold per-ViewModel state, that is unnecessary overhead.
Moving the bindings toSingletonComponent(orActivityRetainedComponent) will:• Re-use the same repository across ViewModels
• Avoid needless allocations
• Make it easier to cache data@InstallIn(SingletonComponent::class) abstract class DataModule { ... }If per-ViewModel scope is intentional, please add a comment explaining the rationale.
app/src/main/java/ai/elimu/vitabu/ui/StoryBooksActivity.kt (2)
17-20: Depend on the contract, not the implementationImporting
StoryBookViewModelImplcouples the Activity to one concrete class, defeating the benefit of the interface you just created.
Use Hilt’sby viewModels<StoryBookViewModelImpl>()or request the ViewModel through the interface if you keep manual factories.private val storyBookViewModel: StoryBookViewModel by viewModels()This keeps the Activity agnostic of the implementation and eases testing/replacement.
74-77: Simplify ViewModel retrieval with Hilt helpersManual
ViewModelProviderboilerplate can be replaced by:private val storyBookViewModel: StoryBookViewModelImpl by viewModels()when the implementation is annotated with
@HiltViewModel. This reduces verbosity and prevents accidental retention leaks.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
app/src/main/java/ai/elimu/vitabu/data/repository/LocalStoryBooksDataSource.kt(1 hunks)app/src/main/java/ai/elimu/vitabu/data/repository/LocalStoryBooksDataSourceImpl.kt(1 hunks)app/src/main/java/ai/elimu/vitabu/data/repository/StoryBookRepository.kt(1 hunks)app/src/main/java/ai/elimu/vitabu/data/repository/StoryBooksRepositoryImpl.kt(1 hunks)app/src/main/java/ai/elimu/vitabu/di/DataModule.kt(1 hunks)app/src/main/java/ai/elimu/vitabu/ui/StoryBooksActivity.kt(4 hunks)app/src/main/java/ai/elimu/vitabu/viewmodel/StoryBookViewModel.kt(1 hunks)app/src/main/java/ai/elimu/vitabu/viewmodel/StoryBookViewModelImpl.kt(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: test (28)
- GitHub Check: test (29)
- GitHub Check: build (macos-latest, 17)
- GitHub Check: build (windows-latest, 17)
- GitHub Check: build (macos-latest, 21)
- GitHub Check: build (windows-latest, 21)
- GitHub Check: build (ubuntu-latest, 17)
- GitHub Check: build (ubuntu-latest, 21)
- GitHub Check: lint
🔇 Additional comments (5)
app/src/main/java/ai/elimu/vitabu/data/repository/StoryBookRepository.kt (1)
5-7: Well-designed repository interface.This clean interface provides a clear contract for asynchronous retrieval of storybooks, supporting elimu.ai's mission to build innovative learning software that empowers out-of-school children to teach themselves basic reading📖, writing✍🏽 and math🔢 within 6 months.
The suspend function indicates proper coroutine support for background operations, which is essential for smooth UI experiences when loading educational content.
app/src/main/java/ai/elimu/vitabu/data/repository/LocalStoryBooksDataSource.kt (1)
5-7: Good separation of data source concerns.This interface effectively isolates the local data retrieval logic, supporting elimu.ai's mission to build innovative learning software that empowers out-of-school children to teach themselves basic reading📖, writing✍🏽 and math🔢 within 6 months.
The clean separation between repository and data source layers will make the codebase more maintainable and testable, allowing easier extensions to support different book types or sources in the future.
app/src/main/java/ai/elimu/vitabu/data/repository/StoryBooksRepositoryImpl.kt (1)
6-12: Clean repository implementation with proper DI.This implementation follows good architectural patterns with dependency injection, supporting elimu.ai's mission to build innovative learning software that empowers out-of-school children to teach themselves basic reading📖, writing✍🏽 and math🔢 within 6 months.
The code properly delegates to the injected data source, following the Dependency Inversion Principle and making the component easily testable through mocking.
app/src/main/java/ai/elimu/vitabu/viewmodel/StoryBookViewModelImpl.kt (1)
14-22: Well-structured ViewModel with reactive state handling.The ViewModel implementation properly manages UI state through StateFlow, supporting elimu.ai's mission to build innovative learning software that empowers out-of-school children to teach themselves basic reading📖, writing✍🏽 and math🔢 within 6 months.
The injected IO scope ensures background processing for data operations, keeping the UI responsive for young learners.
app/src/main/java/ai/elimu/vitabu/ui/StoryBooksActivity.kt (1)
82-96: Handle the new Error state to keep the UI responsiveOnce an
Errorstate is added toLoadStoryBooksUiState, extend thiswhenblock to react (e.g., hide the progress bar and show a retry Snackbar). Otherwise the user sees a spinner forever on network/content-provider failures.
| override fun getAllStoryBooks() { | ||
| ioScope.launch { | ||
| _uiState.emit(LoadStoryBooksUiState.Loading) | ||
| val storyBooks = storyBookRepository.getStoryBooks() | ||
| _uiState.emit(LoadStoryBooksUiState.Success(storyBooks)) | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling to improve user experience.
While the implementation is solid, it doesn't handle potential errors during storybook retrieval, which could lead to poor experiences for children using the app to learn reading📖.
Consider adding error handling to ensure the UI properly reflects failure states:
override fun getAllStoryBooks() {
ioScope.launch {
_uiState.emit(LoadStoryBooksUiState.Loading)
- val storyBooks = storyBookRepository.getStoryBooks()
- _uiState.emit(LoadStoryBooksUiState.Success(storyBooks))
+ try {
+ val storyBooks = storyBookRepository.getStoryBooks()
+ _uiState.emit(LoadStoryBooksUiState.Success(storyBooks))
+ } catch (e: Exception) {
+ _uiState.emit(LoadStoryBooksUiState.Error(e.message ?: "Unknown error"))
+ }
}
}Make sure to also add an Error state to your LoadStoryBooksUiState sealed class if it doesn't already exist.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| override fun getAllStoryBooks() { | |
| ioScope.launch { | |
| _uiState.emit(LoadStoryBooksUiState.Loading) | |
| val storyBooks = storyBookRepository.getStoryBooks() | |
| _uiState.emit(LoadStoryBooksUiState.Success(storyBooks)) | |
| } | |
| } | |
| override fun getAllStoryBooks() { | |
| ioScope.launch { | |
| _uiState.emit(LoadStoryBooksUiState.Loading) | |
| try { | |
| val storyBooks = storyBookRepository.getStoryBooks() | |
| _uiState.emit(LoadStoryBooksUiState.Success(storyBooks)) | |
| } catch (e: Exception) { | |
| _uiState.emit(LoadStoryBooksUiState.Error(e.message ?: "Unknown error")) | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
In app/src/main/java/ai/elimu/vitabu/viewmodel/StoryBookViewModelImpl.kt around
lines 23 to 29, the getAllStoryBooks function lacks error handling for potential
failures during storybook retrieval. To fix this, wrap the repository call and
UI state emissions inside a try-catch block within the coroutine. Emit a new
Error state from LoadStoryBooksUiState in the catch block to reflect failure in
the UI. Also, ensure the LoadStoryBooksUiState sealed class includes an Error
state to represent error conditions.
| sealed interface LoadStoryBooksUiState { | ||
| /** | ||
| * The storybooks are still loading. | ||
| */ | ||
| data object Loading : LoadStoryBooksUiState | ||
|
|
||
| /** | ||
| * The storybooks are loaded with the given list of storybooks resources. | ||
| */ | ||
| data class Success( | ||
| /** | ||
| * The list of storybooks resources | ||
| */ | ||
| val storyBooks: List<StoryBookGson>, | ||
| ) : LoadStoryBooksUiState | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add a failure branch to the UI-state sealed interface
At the moment the UI can represent only “Loading” and “Success”. Any I/O or parsing exception thrown by the repository will currently crash the flow or leave the UI stuck in the loading state.
Adding an Error (or Failure) state gives the ViewModel a chance to surface problems gracefully and lets the Activity decide whether to show a retry button or a friendly message – an important usability detail when your users are the out-of-school children that elimu.ai’s mission is championing. 📖✍🏽🔢
sealed interface LoadStoryBooksUiState {
data object Loading : LoadStoryBooksUiState
data class Success(val storyBooks: List<StoryBookGson>) : LoadStoryBooksUiState
+ /**
+ * Loading failed with the given throwable.
+ */
+ data class Error(val throwable: Throwable) : LoadStoryBooksUiState
}🤖 Prompt for AI Agents
In app/src/main/java/ai/elimu/vitabu/viewmodel/StoryBookViewModel.kt between
lines 9 and 24, the sealed interface LoadStoryBooksUiState currently only has
Loading and Success states, which does not handle errors. Add a new data class
Error that takes a Throwable parameter to represent failure states. This will
allow the ViewModel to emit an Error state when loading fails, enabling the UI
to show appropriate error messages or retry options.
There was a problem hiding this comment.
@tuancoltech I see that the changes in this pull request adds 160 lines of code, and another level of abstraction. How will that make it easier for new engineers to understand the code? From my perspective, this change would have the opposite effect.
My recommendation would be to not use Dagger due to its abstraction and complexity. Instead, if we keep things simple, it will be easier for other engineers to understand and modify the code in the future.
@jo-elimu There're a few problems with current codebase: 1. Non-UI logic execution in main thread.Some works (e.g, interaction with database) are being executed in main thread which will cause sluggish app UI, or even freeze UI on some low end devices. Those should be executed in worker thread instead. Below is an example of such logic. 2. Tightly coupled business logic and UI logicUI ( Speaking about understandability.I don't think the newly introduced changes are hard to be understood even for new comers, for 2 reasons:
|
tuancoltech
force-pushed
the
decouple_ui_from_data_layer
branch
from
79a0671 to
77523c7
Compare
@tuancoltech I have a low-end device here, so did a test to see if the loading speed is different:
API_34_Screen_recording_20250526_170346.mp4
API_34_Screen_recording_20250526.mp4The loading speeds looks similar (5-6 seconds), maybe slightly faster with the changes in this PR (0.5 seconds or so). And note that the loading indicator disappeared with the changes in this PR. |
@tuancoltech Different engineers have different perspectives, which is normal. In this case you think the changes are an improvement, while my opinion is different. Either way; As an organizational principle it should be the person doing the actual work that is making the final decision. So since you have been working the most on this app recently, you should also be the person deciding whether or not to merge these changes. |
Decouple storybooks retrieval logic from UI, which eases:
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Refactor