CMM-883 support Odie bot conversation pagination #22316
Conversation
|
Claude finished @adalpari's task —— View job Code Review Complete ✅I've reviewed the pagination implementation for the AI Bot conversation feature. Here's my comprehensive feedback: Critical Issues🔴 1. Debug TODO Left in Production CodeLocation: The page size is set to 4 for testing purposes, but this must be changed before merging. A page size of 4 will cause excessive API calls in production. Action Required: Change Design & Architecture Issues🟡 2. Race Condition in Message LoadingLocation: The guard clause checking Recommendation: Use a 🟡 3. Page State ManagementLocation: The Recommendation: Consider storing pagination state per conversation ID using a map. Code Quality Issues🟡 4. Message Deduplication MissingLocation: When prepending older messages, there's no deduplication logic. If the API returns overlapping messages or pagination boundaries shift, duplicates could appear. Recommendation: Add 🟡 5. Hardcoded Threshold ValueLocation: The pagination trigger threshold is hardcoded as 4, matching the debug page size. Recommendation: Extract to a named constant for better maintainability. 🟢 6. Message ID GenerationLocation: Using Recommendation: Use Performance Considerations🟢 7. No Caching StrategyLocation: Each conversation load makes a fresh API call with no caching of previously loaded pages. Suggestion: Consider implementing local caching to reduce network calls and improve offline support. Testing & Edge Cases🟡 8. Empty Response HandlingLocation: The code assumes empty messages mean "no more pages," but this could also indicate an API error. Recommendation: Add validation to distinguish between legitimate end-of-data and error conditions. 🟡 9. Missing Test CoverageNo test files were added or modified in this PR. Recommendation: Add unit tests for pagination state management, edge cases, and concurrent loading scenarios. Positive Observations 👍
Summary & Action ItemsThe pagination implementation is generally solid with good architectural patterns. However, there are critical issues that must be addressed before merging: Must Fix Before Merge:
Strongly Recommended: Nice to Have: Overall, the code demonstrates good understanding of Compose, coroutines, and MVVM patterns. With the critical fixes applied, this will be a solid implementation of pagination. Great job on the UX considerations! 🎉 |
| params = GetBotConversationParams() | ||
| params = GetBotConversationParams( | ||
| pageNumber = pageNumber.toULong(), | ||
| // TODO: this is set to 4 for testing purpose |
There was a problem hiding this comment.
This TODO is preventing the PR from passing. itemsPerPage will be changed to a higher number
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
...Press/src/main/java/org/wordpress/android/support/aibot/repository/AIBotSupportRepository.kt
Outdated
Show resolved
Hide resolved
WordPress/src/main/java/org/wordpress/android/support/aibot/ui/AIBotSupportViewModel.kt
Show resolved
Hide resolved
WordPress/src/main/java/org/wordpress/android/support/aibot/ui/AIBotSupportViewModel.kt
Outdated
Show resolved
Hide resolved
WordPress/src/main/java/org/wordpress/android/support/aibot/ui/AIBotConversationDetailScreen.kt
Outdated
Show resolved
Hide resolved
This is on purpose and explained here: #22316 (comment) |
Mutex and constant
|
| App Name | WordPress | |
| Flavor | Jalapeno | |
| Build Type | Debug | |
| Version | pr22316-e066a74 | |
| Commit | e066a74 | |
| Direct Download | wordpress-prototype-build-pr22316-e066a74.apk |
|
| App Name | Jetpack | |
| Flavor | Jalapeno | |
| Build Type | Debug | |
| Version | pr22316-e066a74 | |
| Commit | e066a74 | |
| Direct Download | jetpack-prototype-build-pr22316-e066a74.apk |
Mutex and constant
… of https://github.com/wordpress-mobile/WordPress-Android into feat/CMM-883-support-Oddie-bot-conversation-pagination
adalpari
changed the base branch from
feat/CMM-884-support-Iterate-over-the-whole-flow-and-style
to
trunk
adalpari
changed the base branch from
trunk
to
feat/CMM-884-support-Iterate-over-the-whole-flow-and-style
There was a problem hiding this comment.
Changes look good. Tested well for me.
I did experience one moment where the chat detail via seemingly crashed and disappeared, revealing the Help & Support view. I was unable to reproduce it. I believe I was quickly scrolling upwards in a long, paginated conversation while testing the pagination loading of older messages.
Thank you, I'll keep an eye! |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## feat/CMM-884-support-Iterate-over-the-whole-flow-and-style #22316 +/- ##
==============================================================================================
- Coverage 39.38% 39.33% -0.05%
==============================================================================================
Files 2195 2198 +3
Lines 105232 105394 +162
Branches 14963 14983 +20
==============================================================================================
+ Hits 41446 41460 +14
- Misses 60297 60442 +145
- Partials 3489 3492 +3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
* Put ConversationListView in common between bots and HE * Empty and error state * Skip site capitalization * Adding a11c labels * Adding headings labels * adding accessible labels to chat bubbles * detekt * Fixing tests * PR suggestion about bot chat bubble * Fixing tests * Fixing TalkBack duplication
Generated by 🚫 Danger |
|
adalpari
merged commit d0122ff
into
feat/CMM-884-support-Iterate-over-the-whole-flow-and-style
adalpari
deleted the
feat/CMM-883-support-Oddie-bot-conversation-pagination
branch
* Adding basic UI * Renaming * Some styling * Renaming and dummy data * Using proper "new conversation icon" * Conversation details screen * Creating the reply bottomsheet * Linking to the support screen * bottomsheet fix * Mov navigation form activity to viewmodel * Adding create ticket screen * More screen adjustments * Extracting common code * Margin fix * detekt * Style * New ticket check * Creating tests * Creating repository and load conversations function * Adding createConversation function * Creating loadConversation func * Loading conversations form the viewmodel * Adding loading spinner * Pull to refresh * Proper ionitialization * Adding empty screen * Handling send new conversation * Show loading when sending * New ticket creation fix * Using snackbar for errors * Error handling * Answering conversation * Adding some test to the repository * More tests! * Compile fixes * Similarities improvements * Using snackbar in bots activity * Extracting EmptyConversationsView * Renaming * Extracting VM and UI common code * Extracting navigation common code * Renaming VMs for clarification * More refactor * Capitalise text fields * Updating rs library * Loading conversation UX * Style fix * Fixing scaffolds paddings * userID fix * Fixing the padding problem in bot chat when the keyboard is opened * Apply padding to create ticket screen when the keyboard is opened * Fixing scroll state in reply bottomsheet * Adding tests for the new common viewmodel * Fixing AIBotSupportViewModel tests * detekt * Improvements int he conversation interaction * Adding tests for HE VM * Saving draft state * Properly navigating when a ticket is selected * Error parsing improvement * accessToken suggestion improvements * General suggestions * Send message error UX improvement * Fixing tests * Converting the UI to more AndroidMaterial style * Bots screen renaming * Bots screens renaming * Make NewTicket screen more Android Material theme as well * Adding preview for EmptyConversationsView * Button fix * detekt * Ticket selection change * Supporting markdown text * detekt * Improving MarkdownUtils * Formatting text in the repository layer instead the ui * Renaming * Fixing tests * Parsing markdown more exhaustively * New links support * Detekt * CMM-883 support Odie bot conversation pagination (#22316) * Support pagination * Triggering in the 4th element * detekt * TODO for debug purposes * Claude PR suggestions Mutex and constant * Support pagination * Triggering in the 4th element * detekt * TODO for debug purposes * Claude PR suggestions Mutex and constant * Detekt * Removing testing code * CMM-894 new support general improvements (#22320) * Put ConversationListView in common between bots and HE * Empty and error state * Skip site capitalization * Adding a11c labels * Adding headings labels * adding accessible labels to chat bubbles * detekt * Fixing tests * PR suggestion about bot chat bubble * Fixing tests * Fixing TalkBack duplication
Description
This PR is adding pagination support to the bot conversations.
Testing instructions
Note: the page size is set to 4, but this is set on purpose to be able to properly test it. There's a TODO preventing the PR from passing, to remember changing it to a higher number before merge.
Screen_recording_20251027_135041.mp4