Skip to content

[WOOMOB-3442] Fix crash when toggling the unread reviews filter#16260

Merged
irfano merged 3 commits into
trunkfrom
issue/WOOMOB-3442_reviews-filter-crash
Jul 16, 2026
Merged

[WOOMOB-3442] Fix crash when toggling the unread reviews filter#16260
irfano merged 3 commits into
trunkfrom
issue/WOOMOB-3442_reviews-filter-crash

Conversation

@AdamGrzybkowski

@AdamGrzybkowski AdamGrzybkowski commented Jul 14, 2026

Copy link
Copy Markdown
Contributor

Description

Fixes WOOMOB-3442

Root cause: The Reviews list is a Compose LazyColumn keyed by review.remoteId, which requires globally unique keys. The unread-filter path resolves review ids from the selected site's notifications and reads them back via ProductReviewsDao.getProductReviews(ids) — the only query in that DAO that filtered solely by remoteProductReviewId IN (:ids), without scoping to localSiteId (every sibling query is site-scoped). Because WordPress review/comment ids are per-site, a merchant with more than one store can have the same review id cached under two localSiteIds. The un-scoped query then returned both rows, producing two reviews with the same remoteId → duplicate LazyColumn key → crash.

Fix: Scope getProductReviews(ids) to localSiteId (matching the pattern of every other query in the DAO) and thread the selected site through the single call chain:

Test Steps

Requires an account with two connected stores whose product reviews have overlapping ids (natural for fresh test stores, which number reviews 1, 2, 3…), and at least one unread review on the selected store (so the "Unread reviews only" switch and the mark-all-read ✓ appear).

  1. Open the app on the store that has unread reviews (Jetpack-connected, push not taken over by Woo).
  2. Go to Menu → Reviews.
  3. Toggle the "Unread reviews only" switch several times.
  4. Verify the list keeps rendering unread reviews and the app no longer crashes.

Images/gif

N/A

  • I have considered if this change warrants release notes and have added them to RELEASE-NOTES.txt if necessary. Use the "[Internal]" label for non-user-facing changes.

@AdamGrzybkowski AdamGrzybkowski added this to the 25.3 milestone Jul 14, 2026
@AdamGrzybkowski AdamGrzybkowski added category: unit tests Related to unit testing. feature: review list Related to the product reviews list. type: crash The worst kind of bug. labels Jul 14, 2026
@wpmobilebot

wpmobilebot commented Jul 14, 2026

Copy link
Copy Markdown
Collaborator

App Icon📲 You can test the changes from this Pull Request in WooCommerce Android by scanning the QR code below to install the corresponding build.

App NameWooCommerce Android
Platform📱 Mobile
FlavorJalapeno
Build TypeDebug
Build Number772
Version25.2-rc-1
Application IDcom.woocommerce.android.prealpha
Commit843656b
Installation URL117trubpo5ov8
Automatticians: You can use our internal self-serve MC tool to give yourself access to those builds if needed.

@codecov-commenter

codecov-commenter commented Jul 14, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 0% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 41.16%. Comparing base (c6abb0c) to head (843656b).
⚠️ Report is 13 commits behind head on trunk.

Files with missing lines Patch % Lines
...ommerce/android/ui/reviews/ReviewListRepository.kt 0.00% 1 Missing ⚠️
...rg/wordpress/android/fluxc/store/WCProductStore.kt 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff            @@
##              trunk   #16260   +/-   ##
=========================================
  Coverage     41.16%   41.16%           
  Complexity    13501    13501           
=========================================
  Files          2503     2503           
  Lines        146456   146456           
  Branches      21760    21760           
=========================================
  Hits          60283    60283           
  Misses        80076    80076           
  Partials       6097     6097           

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@AdamGrzybkowski
AdamGrzybkowski force-pushed the issue/WOOMOB-3442_reviews-filter-crash branch from 94e0179 to bfcd628 Compare July 14, 2026 18:11

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR fixes a crash in the Reviews screen when toggling the “Unread reviews only” filter for merchants with multiple connected stores by ensuring cached review lookups are properly scoped to the selected site (preventing duplicate Compose LazyColumn keys due to cross-site ID collisions).

Changes:

  • Scope ProductReviewsDao.getProductReviews(ids) by localSiteId to avoid returning reviews from other sites with the same remote review ID.
  • Thread the selected SiteModel through ReviewListRepository → WCProductStore → ProductReviewsDao for the unread-review cached lookup path.
  • Add a regression test that validates cross-site duplicate review IDs do not produce duplicate results when querying a single site.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.

File Description
WooCommerce/src/main/kotlin/com/woocommerce/android/ui/reviews/ReviewListRepository.kt Passes the selected site into the cached unread-reviews lookup to keep results site-scoped.
libs/fluxc-plugin/src/main/kotlin/org/wordpress/android/fluxc/store/WCProductStore.kt Updates the store API to require site when fetching reviews by review ID, enforcing correct scoping.
libs/fluxc-plugin/src/main/kotlin/org/wordpress/android/fluxc/persistence/dao/ProductReviewsDao.kt Fixes the underlying Room query to filter by localSiteId when selecting by review IDs.
libs/fluxc-plugin/src/test/java/org/wordpress/android/fluxc/persistence/dao/ProductReviewsDaoTest.kt Adds a test covering duplicate review IDs across sites to prevent regressions.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@AdamGrzybkowski
AdamGrzybkowski marked this pull request as ready for review July 14, 2026 18:50

@claude claude Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AI Code Review - No issues found. The changes look good.

The fix scopes the getProductReviews(ids) Room query by localSiteId, which is consistent with every other query in ProductReviewsDao (all of which are already site-scoped). The site parameter is threaded correctly through WCProductStore.getProductReviewsByReviewId and ReviewListRepository.getCachedUnreadProductReviews, and the only caller in the repo is updated. The added regression test correctly exercises the cross-site duplicate-ID scenario (same review IDs cached under two localSiteIds) and asserts no cross-site duplicates leak through — which is exactly the condition that produced the duplicate LazyColumn keys crash.


Automatic review · claude-opus-4-8 · Workflow run

How to reply to a finding

Reply on this review (or inline at the line the finding refers to) with one of:

  • @claude addressed - I made the change. Bot verifies against the next diff before marking resolved.
  • @claude rejected: <reason> - Will not fix; reason gets quoted on the next review.
  • @claude not-applicable - Finding does not apply (wrong file, already covered elsewhere, etc.).

The bot honours these on the next review pass.

@irfano irfano self-assigned this Jul 15, 2026

@irfano irfano left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! 👍🏻

@irfano
irfano enabled auto-merge July 16, 2026 20:43
@irfano
irfano merged commit db0528c into trunk Jul 16, 2026
16 checks passed
@irfano
irfano deleted the issue/WOOMOB-3442_reviews-filter-crash branch July 16, 2026 21:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

category: unit tests Related to unit testing. feature: review list Related to the product reviews list. type: crash The worst kind of bug.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants