⚡ Bolt: Make CartSummary a data class to prevent unnecessary recompositions#83
⚡ Bolt: Make CartSummary a data class to prevent unnecessary recompositions#83himattm wants to merge 1 commit into
Conversation
Changed CartSummary to be a data class to prevent Jetpack Compose from performing unnecessary recompositions on the CartBanner due to identity equality checks. Tests updated to reflect correct behavior. Co-authored-by: himattm <6266621+himattm@users.noreply.github.com>
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request converts the CartSummary class to a data class to ensure structural equality, which prevents unnecessary recompositions in Jetpack Compose. Additionally, the RecompositionRegressionTest has been updated to assert stable behavior and specific recomposition counts. The review feedback points out several locations where existing code comments have become stale or inaccurate as a result of these changes and recommends updating or removing them to maintain documentation accuracy.
| // causing recomposition even when the logical content is the same. | ||
| // Making this a `data class` would fix the problem. | ||
| class CartSummary(val itemCount: Int, val totalPrice: String) | ||
| data class CartSummary(val itemCount: Int, val totalPrice: String) |
There was a problem hiding this comment.
The comments on lines 46-48 (and the detailed explanation in the file header at lines 35-44) are now outdated. They describe CartSummary as an unstable class and suggest converting it to a data class, which has now been done. These comments should be removed or updated to reflect the current state of the code.
|
|
||
| // ISSUE: CartBanner recomposes despite no logical change to the cart | ||
| composeTestRule.onNodeWithTag("cart_banner").assertRecompositions(atLeast = 1) | ||
| composeTestRule.onNodeWithTag("cart_banner").assertStable() |
There was a problem hiding this comment.
The surrounding comments (line 118 and lines 121-124) are now stale. They describe a recomposition issue and suggest a fix that has already been applied in this line. Additionally, the comment on line 124 mentions assertFirstComposition(), which differs from the actual implementation assertStable(). These comments should be cleaned up.
| // recompositions (2 from selects + 3 from refreshes). | ||
| // Budget: at most 5 -- bounded at 1:1 with parent recompositions. | ||
| composeTestRule.onNodeWithTag("cart_banner").assertRecompositions(atMost = 5) | ||
| composeTestRule.onNodeWithTag("cart_banner").assertRecompositions(exactly = 2) |
There was a problem hiding this comment.
The comments on lines 162-165 and 168-170 are now outdated. They refer to the "current behavior" as an issue and describe the data class fix as a future step. Since the fix is now implemented, these comments should be updated or removed.
| // FIX: With `data class CartSummary`, only the 2 selects would cause | ||
| // recomposition. The fixed assertion would be: assertRecomposesExactly(2) | ||
| composeTestRule.onNodeWithTag("cart_banner").assertRecompositions(atLeast = 3) | ||
| composeTestRule.onNodeWithTag("cart_banner").assertRecompositions(exactly = 2) |
There was a problem hiding this comment.
The comments on lines 200-203 are now stale as they describe the recomposition issue that this PR resolves. They should be removed or updated to match the current stable behavior.
1 participant
💡 What: Changed
CartSummaryfrom a regularclassto adata class.🎯 Why: Regular classes in Kotlin are compared by reference. When
ProductListScreenrecomposed (e.g. from an unrelated refresh count change), a newCartSummaryinstance was created. Compose saw a new reference and unnecessarily recomposedCartBanner, even when the cart items and total price were identical. By making it adata class, Compose can compare the contents structurally.📊 Impact: Reduces
CartBannerrecompositions. Wasted recompositions on unrelated parent state changes (like refreshing) drop to 0, bounding recompositions tightly to 1:1 with actual cart content updates.🔬 Measurement:
demo/src/androidTest/java/demo/app/RecompositionRegressionTest.ktwas updated to assert the fixed expected behavior (assertStable()instead ofatLeast = 1for refresh actions, exactly 2 instead of at least 3 for combined operations).PR created automatically by Jules for task 1562027274511002186 started by @himattm