⚡ Bolt: [performance improvement] Change CartSummary to a data class to prevent unnecessary recompositions#79
Conversation
This prevents unnecessary recompositions of CartBanner on unrelated state changes by enabling structural equality. Tests have been updated. 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 improves Compose stability by converting the CartSummary class to a data class and updating the corresponding regression tests to reflect the expected stable behavior. Review feedback highlights the need to refine the documentation regarding data class stability, as it depends on the stability of its properties, and to update or remove several outdated comments in the source code and tests that still describe the previous unstable implementation.
| @@ -0,0 +1,3 @@ | |||
| ## 2024-05-19 - [Compose Stability] | |||
| **Learning:** Passing a regular class as a parameter in Compose causes unnecessary recompositions because Compose uses `Object.equals` (identity-based) for non-data classes, while `data class` enables structural equality and skipping. | |||
| **Action:** Always use `data class` for state objects passed as parameters to ensure structural equality and prevent unnecessary recompositions. | |||
There was a problem hiding this comment.
The recommendation to always use a data class is slightly over-generalized. A data class only ensures stability in Compose if all its properties are also stable (e.g., primitives, String, or other stable types). If it contains unstable types like a standard List, it won't prevent unnecessary recompositions. It's better to specify that this applies to state objects with stable properties.
| // 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 are now outdated as they describe the class as unstable and suggest a fix that has already been implemented. Please update or remove them 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 comments surrounding this assertion (lines 118 and 121-123) still describe the 'unstable class' issue and the proposed fix. Since the fix has been applied, these comments are now misleading and should be updated to describe the expected stable behavior.
| // 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 outdated. They refer to the 'current behavior' as having 5 recompositions, but the code now correctly asserts 2. These comments should be updated to reflect the optimized behavior.
| // 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 outdated and describe the issue as if it still exists. Additionally, the comment on line 203 mentions assertRecomposesExactly(2), which is inconsistent with the actual method name assertRecompositions(exactly = 2) used in the code.
1 participant
💡 What: Changed
CartSummaryfrom a regular class to adata classinProductList.ktand updated the related assertions inRecompositionRegressionTest.kt.🎯 Why: A regular class uses identity-based equality (
Object.equals), meaning each parent recomposition creates a new instance that forcesCartBannerto recompose, even when logical values (e.g., total price and item count) are exactly the same. Making it adata classallows Compose to use structural equality and skip recomposition.📊 Impact: Reduces unneeded
CartBannerrecompositions. It drops interactions that don't logically update the cart fromatLeast = 1unneeded recompositions down to0(Stable). In mixed tests, recompositions dropped from 5 to 2.🔬 Measurement: Verify by running
RecompositionRegressionTest.ktassertions which now enforce stability (assertStable()) when only unrelated content changes.PR created automatically by Jules for task 13876448279272267603 started by @himattm