CMM-150: Scroll to restored blog post after undoing block site#22532
CMM-150: Scroll to restored blog post after undoing block site#22532
Conversation
When a user blocks a site and then taps "Undo", the blog posts are restored but previously there was no visual indication. Now the list automatically scrolls to show the first post from the restored blog. Changes: - Add scrollToSiteId LiveData to ReaderPostCardActionsHandler - Forward the event through ReaderPostListViewModel - Observe in ReaderPostListFragment and scroll after data loads - Add getPositionOfFirstPostFromBlog() to ReaderPostAdapter Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Generated by 🚫 Danger |
|
Claude finished @nbradbury's task in 1m 23s —— View job Code Review Complete
SummaryThis is a well-implemented feature that adds scroll-to-restored-blog functionality after undoing a site block action. The implementation follows established patterns in the codebase and integrates cleanly with the existing architecture. Overall, the code quality is good with no critical issues found. Positive Aspects✅ Clean Architecture: The implementation properly follows the MVVM pattern with clear separation of concerns across the layers Code Quality Issues1. Potential Race Condition/Timing IssueLocation: There's a potential timing issue where Suggestion: Consider adding a check to verify the blog's posts exist in the adapter: pendingScrollToBlogId?.let { blogId ->
val position = getPostAdapter().getPositionOfFirstPostFromBlog(blogId)
if (position > -1) {
scrollToFirstPostFromBlog(blogId)
} else {
// Posts not loaded yet, keep pending for next data load
return@let
}
pendingScrollToBlogId = null
}2. Missing Edge Case HandlingLocation: The Suggestion: Add logging when the blog is not found to aid debugging: public int getPositionOfFirstPostFromBlog(long blogId) {
if (mPosts == null) {
AppLog.w(T.READER, "getPositionOfFirstPostFromBlog: mPosts is null");
return -1;
}
for (int i = 0; i < mPosts.size(); i++) {
if (mPosts.get(i).blogId == blogId) {
// ... existing code
}
}
AppLog.d(T.READER, "getPositionOfFirstPostFromBlog: blog " + blogId + " not found");
return -1;
}3. DocumentationLocation: While the comment is helpful, it could be more descriptive about when and why this event is emitted. Suggestion: Expand the comment: // Emits a blog ID to scroll to after undo block action. This provides visual feedback
// to the user that the undo was successful by scrolling to show the restored posts.
// The event is consumed by ReaderPostListFragment after posts are refreshed.
private val _scrollToSiteId = MediatorLiveData<Event<Long>>()Performance Considerations✅ Efficient Search: The
✅ No Memory Leaks: Properly uses LiveData observers that are lifecycle-aware Security✅ No security concerns identified. The feature only deals with UI state and doesn't involve:
TestingRecommendation: Add tests for:
Example test structure: @Test
fun `scrollToSiteId emits blog ID after undo block action`() = runTest {
// Given a blocked blog
val blogId = 123L
// When undo is triggered
// Then scrollToSiteId should emit the blog ID
}Documentation✅ PR description clearly explains the changes and provides testing instructions Minor Observations
Recommendations SummaryMust Fix: None - the code is functional as-is Should Fix:
Nice to Have:
Overall Assessment: ✅ Approved - This is a solid implementation that adds valuable user feedback. The code is clean, follows established patterns, and doesn't introduce any critical issues. The main gap is test coverage. |
Verifies that scrollToSiteId LiveData emits the blog ID when user clicks undo after blocking a site, enabling the UI to scroll to the restored post. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
|
| App Name | WordPress | |
| Flavor | Jalapeno | |
| Build Type | Debug | |
| Version | pr22532-44fc46c | |
| Commit | 44fc46c | |
| Direct Download | wordpress-prototype-build-pr22532-44fc46c.apk |
|
| App Name | Jetpack | |
| Flavor | Jalapeno | |
| Build Type | Debug | |
| Version | pr22532-44fc46c | |
| Commit | 44fc46c | |
| Direct Download | jetpack-prototype-build-pr22532-44fc46c.apk |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## trunk #22532 +/- ##
=======================================
Coverage 38.87% 38.87%
=======================================
Files 2202 2202
Lines 107052 107057 +5
Branches 15103 15103
=======================================
+ Hits 41618 41623 +5
Misses 61972 61972
Partials 3462 3462 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| private val _scrollToSiteId = MediatorLiveData<Event<Long>>() | ||
| val scrollToSiteId: LiveData<Event<Long>> = _scrollToSiteId |
There was a problem hiding this comment.
❓ Can we use a Flow Instead of a LiveData here?
|
|
||
| private val _scrollToSiteId = MediatorLiveData<Event<Long>>() | ||
| val scrollToSiteId: LiveData<Event<Long>> = _scrollToSiteId | ||
|
|
There was a problem hiding this comment.
Same questions about not using LiveData
Refactored _scrollToSiteId in ReaderPostCardActionsHandler to use MutableSharedFlow instead of MediatorLiveData. Updated the ViewModel to directly expose the handler's flow and updated the Fragment to collect the flow using repeatOnLifecycle. Updated tests to use backgroundScope for flow collection. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
|
4 participants
Description
Fixes CMM-150
When a user blocks a site in the Reader and then taps "Undo", the blog posts are restored but previously there was no visual indication that the action was undone. Now the list automatically scrolls to show the first post from the restored blog, providing a clear visual cue.
Changes:
scrollToSiteIdLiveData toReaderPostCardActionsHandlerthat emits the blog ID after undoReaderPostListViewModelReaderPostListFragmentand scroll to the restored blog after data loadsgetPositionOfFirstPostFromBlog()method toReaderPostAdapterTesting instructions
Block site undo scroll:
block.mp4