Extract Resources filtering to ResourcesViewModel#11653
Closed
Conversation
Co-authored-by: google-labs-jules[bot] <161369871+google-labs-jules[bot]@users.noreply.github.com>
Co-authored-by: google-labs-jules[bot] <161369871+google-labs-jules[bot]@users.noreply.github.com>
Contributor
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
❌ 13 blocking issues (13 total)
|
![qltysh[bot]](https://avatars.githubusercontent.com/in/890766?s=60&v=4){kind=small}
| return !(subjects.isEmpty() && languages.isEmpty() | ||
| && mediums.isEmpty() && levels.isEmpty() | ||
| && searchTags.isEmpty() && searchText.isEmpty()) | ||
| return !(resourcesViewModel.getSubjects().isEmpty() && resourcesViewModel.getLanguages().isEmpty() |
Okuro3499
requested changes
Mar 3, 2026
Collaborator
Okuro3499
left a comment
Okuro3499
left a comment
There was a problem hiding this comment.
1. Realm objects stored inside StateFlow / ViewModel
_allLibraryItems: MutableStateFlow<List<RealmMyLibrary>> and _tagsMap: MutableStateFlow<Map<String, List<RealmTag>>> hold live managed RealmObject instances. The ViewModel
survives configuration changes; the Realm instance that backs those objects does not. On any rotation or fragment recreation the Realm bound to those objects may be closed, making
every field access throw IllegalStateException: Realm instance has already been closed. The correct fix is to convert to unmanaged copies (realm.copyFromRealm()) or to the
already-existing ResourceItem/TagItem DTOs before handing data to the ViewModel.
2. Eight-way combine with wall-to-wall unchecked casts
Kotlin's combine only has type-safe overloads up to 5 flows. The 8-flow call falls back to the Array<*> overload, so every argument is cast with @Suppress("UNCHECKED_CAST"):
val items = args[0] as List<RealmMyLibrary>
val query = args[1] as String
val tags = args[2] as List<RealmTag>
// …and so on for all 8
If the order of flows in the combine call is ever changed (very easy to do), you get a silent ClassCastException at runtime, not a compile error. Replace the eight individual
StateFlows with a single data class FilterState(…) held in one MutableStateFlow<FilterState>. The combine collapses to a single _filterState.map { applyAll(it) }.
Co-authored-by: google-labs-jules[bot] <161369871+google-labs-jules[bot]@users.noreply.github.com>
Co-authored-by: google-labs-jules[bot] <161369871+google-labs-jules[bot]@users.noreply.github.com>
Okuro3499
approved these changes
Mar 3, 2026
Okuro3499
requested changes
Mar 4, 2026
Collaborator
Okuro3499
left a comment
Okuro3499
left a comment
There was a problem hiding this comment.
java.lang.ClassCastException: kotlin.collections.EmptySet cannot be cast to kotlin.collections.MutableSet
at org.ole.planet.myplanet.ui.resources.ResourcesFilterFragment$initList$1.invokeSuspend(ResourcesFilterFragment.kt:113)
at kotlin.coroutines.jvm.internal.BaseContinuationImpl.resumeWith(ContinuationImpl.kt:34)
at kotlinx.coroutines.internal.DispatchedContinuationKt.resumeCancellableWith(DispatchedContinuation.kt:375)
at kotlinx.coroutines.intrinsics.CancellableKt.startCoroutineCancellable(Cancellable.kt:26)
at kotlinx.coroutines.CoroutineStart.invoke(CoroutineStart.kt:358)
at kotlinx.coroutines.AbstractCoroutine.start(AbstractCoroutine.kt:134)
at kotlinx.coroutines.BuildersKt__Builders_commonKt.launch(Builders.common.kt:53)
at kotlinx.coroutines.BuildersKt.launch(Unknown Source:1)
at kotlinx.coroutines.BuildersKt__Builders_commonKt.launch$default(Builders.common.kt:44)
at kotlinx.coroutines.BuildersKt.launch$default(Unknown Source:1)
at org.ole.planet.myplanet.ui.resources.ResourcesFilterFragment.initList(ResourcesFilterFragment.kt:107)
at org.ole.planet.myplanet.ui.resources.ResourcesFilterFragment.onViewCreated(ResourcesFilterFragment.kt:93)
at androidx.fragment.app.Fragment.performViewCreated(Fragment.java:3147)
at androidx.fragment.app.FragmentStateManager.createView(FragmentStateManager.java:588)
at androidx.fragment.app.FragmentStateManager.moveToExpectedState(FragmentStateManager.java:272)
at androidx.fragment.app.FragmentManager.executeOpsTogether(FragmentManager.java:1943)
at androidx.fragment.app.FragmentManager.removeRedundantOperationsAndExecute(FragmentManager.java:1845)
at androidx.fragment.app.FragmentManager.execPendingActions(FragmentManager.java:1782)
at androidx.fragment.app.FragmentManager$5.run(FragmentManager.java:565)
at android.os.Handler.handleCallback(Handler.java:958)
at android.os.Handler.dispatchMessage(Handler.java:99)
at android.os.Looper.loopOnce(Looper.java:205)
at android.os.Looper.loop(Looper.java:294)
at android.app.ActivityThread.main(ActivityThread.java:8177)
at java.lang.reflect.Method.invoke(Native Method)
at com.android.internal.os.RuntimeInit$MethodAndArgsCaller.run(RuntimeInit.java:552)
at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:971)
Suppressed: kotlinx.coroutines.internal.DiagnosticCoroutineContextException: [StandaloneCoroutine{Cancelling}@c6b552b, Dispatchers.Main.immediate]
Co-authored-by: google-labs-jules[bot] <161369871+google-labs-jules[bot]@users.noreply.github.com>
…ps://github.com/open-learning-exchange/myplanet into extract-resources-viewmodel-9620540927375388035
Okuro3499
requested changes
Mar 5, 2026
Collaborator
Okuro3499
left a comment
Okuro3499
left a comment
There was a problem hiding this comment.
1. Dual state — maintainability risk (must-fix)
subjects, languages, mediums, levels, and searchTags are still maintained as local fragment vars and mirrored into the ViewModel. Every mutation site has to update both:
// ResourcesFragment.kt – clearTagsButton()
levels.clear()
mediums.clear()
subjects.clear()
languages.clear()
resourcesViewModel.setFilters(emptySet(), emptySet(), emptySet(), emptySet())
They're kept in sync in the current code, but the pattern invites divergence as soon as anyone touches one without the other. The local vars should be deleted and the ViewModel treated as the sole source of truth.
2. No debounce on text search — performance risk (must-fix for large libraries)
// ResourcesFragment.kt – searchTextWatcher
resourcesViewModel.setSearchQuery(s.toString().trim())
Every keystroke triggers _filterState update → full pipeline re-execution on viewModelScope. The old code did the same but at least did it inside a lifecycleScope.launch. Adding a 300 ms debounce with debounce() on the flow in the ViewModel init block is a one-liner fix.
3. Initial render gap — UI inconsistency risk (must-fix)
getAdapter() now returns the adapter without calling checkList(), showNoData(), or changeButtonStatus(). Those are only called inside the filteredLibraryItems collector. The collector won't emit until the ViewModel receives items via setAllLibraryItems, which happens asynchronously after loadDataAsync(). On slow devices or large datasets there will be a window where the empty-state message is shown before the list loads, or the button state is wrong.
4. ResourcesFilterFragment cast fix — correct (no action needed)
// Before (ClassCastException risk)
selectedLvls = filterListener?.getSelectedFilter()?.get("levels") as MutableSet<String>
// After (safe)
selectedLvls = filterListener?.getSelectedFilter()?.get("levels")?.toMutableSet() ?: mutableSetOf()
This is a genuine crash fix. The original cast would throw at runtime if the underlying Set was not already a MutableSet. Approved as-is.
Co-authored-by: google-labs-jules[bot] <161369871+google-labs-jules[bot]@users.noreply.github.com>
| feedback?.isUploaded = true | ||
| feedback?.item = JsonUtils.getString("item", act) | ||
| feedback?.state = JsonUtils.getString("state", act) | ||
| feedback?._rev = JsonUtils.getString("_rev", act) |
| newsObject.addProperty("updatedDate", news.newsUpdatedDate) | ||
| newsObject.addProperty("sharedBy", news.sharedBy) | ||
| `object`.add("news", newsObject) | ||
| return `object` |
| } | ||
| } | ||
| val jsonConcatenatedLinks = JsonUtils.gson.toJson(existingConcatenatedLinks) | ||
| settings.edit { putString("concatenated_links", jsonConcatenatedLinks) } |
| settings.edit { putString("concatenated_links", jsonConcatenatedLinks) } | ||
| } | ||
| } | ||
| } |
| } | ||
| } catch (e: Exception) { | ||
| e.printStackTrace() | ||
| } |
| } | ||
| } catch (e: Exception) { | ||
| e.printStackTrace() | ||
| } |
|
|
||
| override fun onSaveConfigAndContinue(dialog: MaterialDialog, binding: DialogServerUrlBinding, defaultUrl: String) { | ||
| saveConfigAndContinue(dialog, binding, "", false, defaultUrl) | ||
| private fun handleConfigurationSuccess(id: String, code: String, url: String, defaultUrl: String, isAlternativeUrl: Boolean, callerActivity: String) { |
| } | ||
| } | ||
| ) | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Extract
applyFilter,isValidFilter, and related filter states out ofBaseRecyclerFragmentinto a new@HiltViewModelnamedResourcesViewModel.ResourcesFragmentnow observes aStateFlowfrom this ViewModel instead of calling the base methods directly.PR created automatically by Jules for task 9620540927375388035 started by @dogi