refactor(messagesearch): replace FlexibleAdapter with ListAdapter - #6789
rapterjet2004 wants to merge 1 commit into
Conversation
Message search was the only remaining FlexibleAdapter user. Replace it with a plain ListAdapter/DiffUtil implementation, swap FlexibleUtils' null-safe lowercase in TalkSpecificViewThemeUtils, and drop the dependency along with dead items (ContactItem, SpacerItem, header items) and their layouts. Assisted-by: Claude Code:claude-opus-5-5 Signed-off-by: rapterjet2004 <juliuslinus1@gmail.com>
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. 📝 WalkthroughWalkthroughMessage search now uses MessageSearchAdapter, which binds result and load-more rows and receives their click events. The activity submits results and adds a load-more row when more results are available. FlexibleAdapter dependencies, related adapter items, and layouts were removed. Text highlighting now uses default-locale lowercase conversion. Priority: ⬇️ Low Merge Risk: 🔵 Low · up to Search results without message IDs may show incorrect row transitions when results update. The issue is bounded, but the identity comparison should be corrected before merging if practical. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to The change does not appear to expand access to messages or introduce a new external entry point. Search-result handling remains in the existing account and search flow. Some edge-case guarantees for result identity and text highlighting remain unverified. Retained concerns Security review detailsSecurity Blast Radius
Trust Boundaries and Controls
Resilience and Maintainability Implications
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
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}
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Advanced
Run ID: 18a28150-2811-4a6d-8c37-2a095fbb6b4b
📒 Files selected for processing (15)
app/build.gradle.ktsapp/src/main/java/com/nextcloud/talk/adapters/items/ContactItem.ktapp/src/main/java/com/nextcloud/talk/adapters/items/FlexibleItemViewType.ktapp/src/main/java/com/nextcloud/talk/adapters/items/GenericTextHeaderItem.ktapp/src/main/java/com/nextcloud/talk/adapters/items/LoadMoreResultsItem.ktapp/src/main/java/com/nextcloud/talk/adapters/items/MessageResultItem.ktapp/src/main/java/com/nextcloud/talk/adapters/items/MessagesTextHeaderItem.ktapp/src/main/java/com/nextcloud/talk/adapters/items/SpacerItem.ktapp/src/main/java/com/nextcloud/talk/messagesearch/MessageSearchActivity.ktapp/src/main/java/com/nextcloud/talk/messagesearch/MessageSearchAdapter.ktapp/src/main/java/com/nextcloud/talk/ui/theme/TalkSpecificViewThemeUtils.ktapp/src/main/res/layout/item_spacer.xmlapp/src/main/res/layout/rv_item_contact.xmlapp/src/main/res/layout/rv_item_title_header.xmlgradle/verification-metadata.xml
💤 Files with no reviewable changes (12)
- app/src/main/res/layout/rv_item_title_header.xml
- app/src/main/res/layout/rv_item_contact.xml
- app/src/main/java/com/nextcloud/talk/adapters/items/MessagesTextHeaderItem.kt
- app/src/main/res/layout/item_spacer.xml
- app/build.gradle.kts
- app/src/main/java/com/nextcloud/talk/adapters/items/LoadMoreResultsItem.kt
- app/src/main/java/com/nextcloud/talk/adapters/items/FlexibleItemViewType.kt
- app/src/main/java/com/nextcloud/talk/adapters/items/GenericTextHeaderItem.kt
- gradle/verification-metadata.xml
- app/src/main/java/com/nextcloud/talk/adapters/items/MessageResultItem.kt
- app/src/main/java/com/nextcloud/talk/adapters/items/SpacerItem.kt
- app/src/main/java/com/nextcloud/talk/adapters/items/ContactItem.kt
Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 3 remain after this review.
| private object ItemCallback : DiffUtil.ItemCallback<Item>() { | ||
| override fun areItemsTheSame(oldItem: Item, newItem: Item): Boolean = | ||
| when { | ||
| oldItem is Item.Result && newItem is Item.Result -> | ||
| oldItem.entry.conversationToken == newItem.entry.conversationToken && | ||
| oldItem.entry.messageId == newItem.entry.messageId | ||
| else -> oldItem == newItem | ||
| } | ||
|
|
||
| override fun areContentsTheSame(oldItem: Item, newItem: Item): Boolean = oldItem == newItem | ||
| } | ||
|
|
||
| companion object { | ||
| private const val VIEW_TYPE_RESULT = 0 | ||
| private const val VIEW_TYPE_LOAD_MORE = 1 | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🔎 Supported by static analysis
🏁 Script executed:
sed -n '75,100p' app/src/main/java/com/nextcloud/talk/messagesearch/MessageSearchAdapter.kt
rg -n 'SearchMessageEntry\(' app/src/main/java/com/nextcloud/talkRepository: nextcloud/talk-android
Length of output: 1332
🏁 Script executed:
set -eu
printf '%s\n' '--- adapter ---'
cat -n app/src/main/java/com/nextcloud/talk/messagesearch/MessageSearchAdapter.kt
printf '%s\n' '--- model ---'
cat -n app/src/main/java/com/nextcloud/talk/models/domain/SearchMessageEntry.kt
printf '%s\n' '--- producer ---'
cat -n app/src/main/java/com/nextcloud/talk/repositories/unifiedsearch/UnifiedSearchRepositoryImpl.kt
printf '%s\n' '--- result references ---'
rg -n -C 4 'MessageSearchAdapter|submitList|SearchMessageEntry|messageId|conversationToken' app/src/main/java/com/nextcloud/talk/messagesearch app/src/main/java/com/nextcloud/talk/repositories/unifiedsearch app/src/main/java/com/nextcloud/talk/models/domain app/src/test app/src/androidTest 2>/dev/null || trueRepository: nextcloud/talk-android
Length of output: 41890
🏁 Script executed:
set -eu
printf '%s\n' '--- helper ---'
cat -n app/src/main/java/com/nextcloud/talk/messagesearch/MessageSearchHelper.kt
printf '%s\n' '--- activity relevant ---'
sed -n '110,150p' app/src/main/java/com/nextcloud/talk/messagesearch/MessageSearchActivity.kt | cat -n
printf '%s\n' '--- view model selection ---'
rg -n -C 6 'selectMessage|FinishedState|messageEntry.messageId' app/src/main/java/com/nextcloud/talk/messagesearch
printf '%s\n' '--- DTO ---'
rg -n -C 5 'data class UnifiedSearchEntryDto|class UnifiedSearchEntryDto|attributes' app/src/main/java/com/nextcloud/talk/models/json/unifiedsearch app/src/test/java/com/nextcloud/talk/messagesearch
printf '%s\n' '--- relevant diff ---'
git diff 5c28bcbce6bc377698a5593e66201a2ba75bddd4 6ebf1f52e5c98c58cfa88e8999a22ffe9bfa8051 -- app/src/main/java/com/nextcloud/talk/messagesearch/MessageSearchAdapter.ktRepository: nextcloud/talk-android
Length of output: 21983
Do not treat null messageId values as shared result identities.
messageId is nullable, and the search pipeline keeps entries without it. Two entries from the same conversation can therefore satisfy the current identity comparison. This violates DiffUtil.ItemCallback’s unique-item identity contract and can produce incorrect row matching or animations. Changed content is still compared separately, so stale content or click selection is not established as the consequence.
Suggested fix
oldItem is Item.Result && newItem is Item.Result ->
- oldItem.entry.conversationToken == newItem.entry.conversationToken &&
+ oldItem.entry.messageId != null &&
+ oldItem.entry.conversationToken == newItem.entry.conversationToken &&
oldItem.entry.messageId == newItem.entry.messageId📝 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.
| private object ItemCallback : DiffUtil.ItemCallback<Item>() { | |
| override fun areItemsTheSame(oldItem: Item, newItem: Item): Boolean = | |
| when { | |
| oldItem is Item.Result && newItem is Item.Result -> | |
| oldItem.entry.conversationToken == newItem.entry.conversationToken && | |
| oldItem.entry.messageId == newItem.entry.messageId | |
| else -> oldItem == newItem | |
| } | |
| override fun areContentsTheSame(oldItem: Item, newItem: Item): Boolean = oldItem == newItem | |
| } | |
| companion object { | |
| private const val VIEW_TYPE_RESULT = 0 | |
| private const val VIEW_TYPE_LOAD_MORE = 1 | |
| } | |
| } | |
| private object ItemCallback : DiffUtil.ItemCallback<Item>() { | |
| override fun areItemsTheSame(oldItem: Item, newItem: Item): Boolean = | |
| when { | |
| oldItem is Item.Result && newItem is Item.Result -> | |
| oldItem.entry.messageId != null && | |
| oldItem.entry.conversationToken == newItem.entry.conversationToken && | |
| oldItem.entry.messageId == newItem.entry.messageId | |
| else -> oldItem == newItem | |
| } | |
| override fun areContentsTheSame(oldItem: Item, newItem: Item): Boolean = oldItem == newItem | |
| } | |
| companion object { | |
| private const val VIEW_TYPE_RESULT = 0 | |
| private const val VIEW_TYPE_LOAD_MORE = 1 | |
| } | |
| } |
📱 QA build
The QA build installs alongside a released Nextcloud app, so you can keep Downloading the file requires a GitHub account, so open this link on the |
1 participant
Message search was the only remaining FlexibleAdapter user. Replace it with a plain ListAdapter/DiffUtil implementation, swap FlexibleUtils' null-safe lowercase in TalkSpecificViewThemeUtils, and drop the dependency along with dead items (ContactItem, SpacerItem, header items) and their layouts.
Assisted-by: Claude Code:claude-opus-5-5
🏁 Checklist
/backport to stable-xx.x🤖 AI (if applicable)