Move finding search results to default dispatcher#1046
Merged
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughHighlighting moved off the main thread: highlightSearchResults is suspend, occurrence detection runs on Dispatchers.Default, lifecycleScope.launch orchestrates calls, highlight APIs switched to bulk highlight/select, and findAllOccurrences was rewritten to an indexOf loop with a result cap. ChangesSearch highlighting background execution
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/src/main/java/com/philkes/notallyx/presentation/activity/note/EditListActivity.kt (1)
167-184:⚠️ Potential issue | 🟠 Major | ⚡ Quick winKeep checked-list refresh state separate from the main list.
Lines 172-184 reuse one
alreadyNotifiedItemPosset for both adapters and then send the checked-section fallback refresh throughadapter. If index0is highlighted in the main list, checked item0is treated as already refreshed too, and any non-highlighted checked rows are notified on the wrong adapter. That leaves stale highlights in the checked section.Proposed fix
override suspend fun highlightSearchResults(search: String): Int { val resultPosCounter = AtomicInteger(0) - val alreadyNotifiedItemPos = mutableSetOf<Int>() + val alreadyNotifiedMainItemPos = mutableSetOf<Int>() + val alreadyNotifiedCheckedItemPos = mutableSetOf<Int>() adapter?.clearHighlights() adapterChecked?.clearHighlights() val amount = - items.highlightSearch(search, adapter, resultPosCounter, alreadyNotifiedItemPos) + + items.highlightSearch(search, adapter, resultPosCounter, alreadyNotifiedMainItemPos) + (itemsChecked?.highlightSearch( search, adapterChecked, resultPosCounter, - alreadyNotifiedItemPos, + alreadyNotifiedCheckedItemPos, ) ?: 0) items.indices - .filter { !alreadyNotifiedItemPos.contains(it) } + .filter { !alreadyNotifiedMainItemPos.contains(it) } .forEach { adapter?.notifyItemChanged(it) } itemsChecked ?.indices - ?.filter { !alreadyNotifiedItemPos.contains(it) } - ?.forEach { adapter?.notifyItemChanged(it) } + ?.filter { !alreadyNotifiedCheckedItemPos.contains(it) } + ?.forEach { adapterChecked?.notifyItemChanged(it) } return amount }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/src/main/java/com/philkes/notallyx/presentation/activity/note/EditListActivity.kt` around lines 167 - 184, The shared alreadyNotifiedItemPos set is used for both main and checked lists causing index collisions and the final notify loop for the checked section calls the wrong adapter; split the state and notifications so each list tracks its own refreshed indexes: create a separate alreadyNotifiedItemPosChecked (or similar) and pass it into the itemsChecked.highlightSearch call, then notify changes for the main list with adapter.notifyItemChanged(...) and for the checked list with adapterChecked.notifyItemChanged(...), ensuring highlightSearch is invoked with the correct adapter/sets (references: highlightSearch, items, itemsChecked, adapter, adapterChecked, resultPosCounter).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@app/src/main/java/com/philkes/notallyx/presentation/activity/note/EditActivity.kt`:
- Around line 424-438: The updateSearchResults function currently launches a new
lifecycleScope.launch for each invocation causing concurrent searches; fix this
by introducing a cancellable Job field (e.g., a private var searchJob: Job?) at
the class level, cancel the existing searchJob if active before starting a new
one, then assign the new lifecycleScope.launch to searchJob and run
highlightSearchResults inside it (keeping the existing Dispatchers.Default usage
inside highlightSearchResults). Ensure you still update search.results.value and
search.resultPos.value only from the latest job so canceled jobs do not
overwrite newer results.
In `@app/src/main/java/com/philkes/notallyx/utils/SortedListExtensions.kt`:
- Around line 13-19: The mapIndexedSuspended extensions iterate live collections
and may resume after the underlying data changes; snapshot first, then suspend
over the snapshot. In SortedList<R>.mapIndexedSuspended, create an immutable
list of elements (and their order) from the current SortedList before any
suspending calls, then iterate that snapshot with indices to invoke transform.
In List<R>.mapIndexedSuspended, create an immutable copy of the list and iterate
that copy with indices to invoke transform. Ensure no access uses this[...] or
size() from the live collection during iteration; rely solely on the snapshot.
---
Outside diff comments:
In
`@app/src/main/java/com/philkes/notallyx/presentation/activity/note/EditListActivity.kt`:
- Around line 167-184: The shared alreadyNotifiedItemPos set is used for both
main and checked lists causing index collisions and the final notify loop for
the checked section calls the wrong adapter; split the state and notifications
so each list tracks its own refreshed indexes: create a separate
alreadyNotifiedItemPosChecked (or similar) and pass it into the
itemsChecked.highlightSearch call, then notify changes for the main list with
adapter.notifyItemChanged(...) and for the checked list with
adapterChecked.notifyItemChanged(...), ensuring highlightSearch is invoked with
the correct adapter/sets (references: highlightSearch, items, itemsChecked,
adapter, adapterChecked, resultPosCounter).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 73bd2e29-694b-4b38-b1a3-fda97d8febb3
📒 Files selected for processing (5)
app/src/main/java/com/philkes/notallyx/presentation/activity/note/EditActivity.ktapp/src/main/java/com/philkes/notallyx/presentation/activity/note/EditListActivity.ktapp/src/main/java/com/philkes/notallyx/presentation/activity/note/EditNoteActivity.ktapp/src/main/java/com/philkes/notallyx/presentation/activity/note/EditTextPlainActivity.ktapp/src/main/java/com/philkes/notallyx/utils/SortedListExtensions.kt
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/src/main/java/com/philkes/notallyx/presentation/view/misc/highlightableview/HighlightableEditText.kt (1)
139-145:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
unselectHighlight()now wipes every other highlight.The new
highlight(newOccurrences, ...)first clearshighlightedSpansand removes all existing spans (Lines 68-70) before rebuilding only from the passed list. Sinceunselect()callshighlight(listOf(Pair(previousHighlightedStartIdx, previousHighlightedEndIdx)), -1), deselecting a result (e.g.EditNoteActivity.selectSearchResult(resultPos < 0)) removes all other search highlights, leaving just the single previously-selected range. This is a regression from the old single-rangehighlightsemantics.Revert just this span to a dimmed highlight instead of going through the bulk API:
🐛 Proposed fix
private fun CharacterStyle.unselect() { - val (previousHighlightedStartIdx, previousHighlightedEndIdx) = getSpanRange(this) - if (previousHighlightedStartIdx != -1) { - removeSpan(this) - highlight(listOf(Pair(previousHighlightedStartIdx, previousHighlightedEndIdx)), -1) - } + val (start, end) = getSpanRange(this) + if (start != -1) { + val editable = text ?: return + editable.removeSpan(this) + highlightedSpans.remove(this) + if (this == selectedHighlightedSpan) selectedHighlightedSpan = null + val dimmedSpan = HighlightSpan(highlightColor.withAlpha(0.1f)) + editable.setSpan(dimmedSpan, start, end, Spanned.SPAN_EXCLUSIVE_EXCLUSIVE) + highlightedSpans.add(dimmedSpan) + } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/src/main/java/com/philkes/notallyx/presentation/view/misc/highlightableview/HighlightableEditText.kt` around lines 139 - 145, CharacterStyle.unselect() currently calls highlight(listOf(Pair(previousHighlightedStartIdx, previousHighlightedEndIdx)), -1) which resets highlightedSpans and removes all other spans; instead, after removeSpan(this) re-apply only a dimmed span for the same range so other highlights remain. Concretely: inside CharacterStyle.unselect() use getSpanRange(this) as before, call removeSpan(this), then add a dimmed variant for (previousHighlightedStartIdx, previousHighlightedEndIdx) (e.g., by creating/setting the appropriate CharacterStyle or calling the existing add/set-span helper instead of highlight(...)), and update highlightedSpans to include that single dimmed range without touching other entries.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@app/src/main/java/com/philkes/notallyx/presentation/view/misc/highlightableview/HighlightableEditText.kt`:
- Line 12: The constant SEARCH_HIGHLIGHT_LIMIT (1_000) used in
HighlightableEditText.highlight() diverges from the 20_000 cap used by
findAllOccurrences, causing the UI to only render 1,000 highlights while
searchResultIndices can report up to 20,000 matches; update the code so the two
limits are consistent by either increasing SEARCH_HIGHLIGHT_LIMIT to match
findAllOccurrences' cap (20_000) or reduce/centralize the cap so both
findAllOccurrences and highlight() reference the same shared limit constant, and
ensure any UI code that relies on searchResultIndices (e.g., select or
navigation code) handles the unified cap accordingly.
---
Outside diff comments:
In
`@app/src/main/java/com/philkes/notallyx/presentation/view/misc/highlightableview/HighlightableEditText.kt`:
- Around line 139-145: CharacterStyle.unselect() currently calls
highlight(listOf(Pair(previousHighlightedStartIdx, previousHighlightedEndIdx)),
-1) which resets highlightedSpans and removes all other spans; instead, after
removeSpan(this) re-apply only a dimmed span for the same range so other
highlights remain. Concretely: inside CharacterStyle.unselect() use
getSpanRange(this) as before, call removeSpan(this), then add a dimmed variant
for (previousHighlightedStartIdx, previousHighlightedEndIdx) (e.g., by
creating/setting the appropriate CharacterStyle or calling the existing
add/set-span helper instead of highlight(...)), and update highlightedSpans to
include that single dimmed range without touching other entries.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b542e83e-ee05-452d-9440-d5b94e525727
📒 Files selected for processing (6)
app/src/main/java/com/philkes/notallyx/presentation/activity/note/EditActivity.ktapp/src/main/java/com/philkes/notallyx/presentation/activity/note/EditNoteActivity.ktapp/src/main/java/com/philkes/notallyx/presentation/activity/note/EditTextPlainActivity.ktapp/src/main/java/com/philkes/notallyx/presentation/view/misc/highlightableview/HighlightableEditText.ktapp/src/main/java/com/philkes/notallyx/presentation/view/note/listitem/adapter/ListItemVH.ktapp/src/main/java/com/philkes/notallyx/utils/MiscExtensions.kt
🚧 Files skipped from review as they are similar to previous changes (3)
- app/src/main/java/com/philkes/notallyx/presentation/activity/note/EditTextPlainActivity.kt
- app/src/main/java/com/philkes/notallyx/presentation/activity/note/EditActivity.kt
- app/src/main/java/com/philkes/notallyx/presentation/activity/note/EditNoteActivity.kt
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
1 participant
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.
Fixes #1033
Summary by CodeRabbit
Bug Fixes
New Features