Crash: IllegalArgumentException in LazyList SubcomposeLayout recurs on Android 16 (Samsung Galaxy A15) (#4427)#4434
Conversation
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds a stable dateKey to transaction group UI models and replaces index-based keys with content-derived keys in Lazy lists/grids; previews updated and transaction groups cached via remember before rendering. ChangesCompose Key Stability
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/src/main/java/com/vultisig/wallet/ui/components/v2/tokenitem/TokenSelectionList.kt (1)
159-169:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
hashCode()can produce duplicate keys within a group — same crash as the one being fixed.
GridTokenUiModelis adata class-based sealed type; itshashCode()derives from field values and is not guaranteed unique. If any two items in the same group collide on hash (statistically unlikely but non-zero for large token lists), Compose sees duplicate keys and throws the identicalIllegalArgumentExceptionthis PR targets.The cross-group prefix
"${title ?: "group$groupIndex"}"prevents collisions between groups but gives no protection within a group.Minimum fix — add
itemIndexas a uniqueness tiebreaker while keeping content-based sorting:🔧 Proposed fix
- key = { _, item -> - "${title ?: "group$groupIndex"}-${item.hashCode()}" - }, + key = { itemIndex, item -> + "${title ?: "group$groupIndex"}-${item.hashCode()}_$itemIndex" + },Ideal long-term fix: add a
keyExtractor: (GridTokenUiModel<T>) -> Anyparameter so callers can supply a domain-level stable ID (e.g.coin.id).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/src/main/java/com/vultisig/wallet/ui/components/v2/tokenitem/TokenSelectionList.kt` around lines 159 - 169, The current itemsIndexed key uses item.hashCode() which can collide for different GridTokenUiModel instances and cause duplicate-key crashes; update the key lambda passed to itemsIndexed (the key = { index, item -> ... } parameter) to include the item index (or other stable per-item tiebreaker) together with the content-derived value so keys are unique within a group while preserving content-based identity (e.g., combine the group prefix/title, item.hashCode(), and the index), and consider adding a long-term improvement by introducing a keyExtractor parameter so callers can supply a stable domain ID for GridTokenUiModel.
🧹 Nitpick comments (1)
app/src/main/java/com/vultisig/wallet/ui/screens/transaction/TransactionHistoryScreen.kt (1)
217-223: 💤 Low value
remember(groups) { groups }is a no-op — consider removing it.
remember(key) { block }returnsblock()whenkeychanges and the cached value otherwise. In both branchessnapshotholds exactly the same reference asgroups. It does not create a defensive snapshot of the list during SubcomposeLayout measurement, and it cannot prevent a recomposition triggered by thegroupsStateFlow emission. The actual crash fix is the stabledateKeyon the sticky header; this wrapper only adds confusion.♻️ Proposed simplification
- val snapshot = remember(groups) { groups } LazyColumn( modifier = modifier, contentPadding = PaddingValues(horizontal = 16.dp, vertical = 8.dp), verticalArrangement = Arrangement.spacedBy(8.dp), ) { - snapshot.forEach { group -> + groups.forEach { group ->🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/src/main/java/com/vultisig/wallet/ui/screens/transaction/TransactionHistoryScreen.kt` around lines 217 - 223, Remove the redundant remember wrapper: delete the line declaring val snapshot = remember(groups) { groups } and update the LazyColumn body to iterate directly over groups (replace snapshot.forEach { ... } with groups.forEach { ... }); keep the rest of the LazyColumn (modifier, contentPadding, verticalArrangement, headers) unchanged so the code uses the original groups list directly.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In
`@app/src/main/java/com/vultisig/wallet/ui/components/v2/tokenitem/TokenSelectionList.kt`:
- Around line 159-169: The current itemsIndexed key uses item.hashCode() which
can collide for different GridTokenUiModel instances and cause duplicate-key
crashes; update the key lambda passed to itemsIndexed (the key = { index, item
-> ... } parameter) to include the item index (or other stable per-item
tiebreaker) together with the content-derived value so keys are unique within a
group while preserving content-based identity (e.g., combine the group
prefix/title, item.hashCode(), and the index), and consider adding a long-term
improvement by introducing a keyExtractor parameter so callers can supply a
stable domain ID for GridTokenUiModel.
---
Nitpick comments:
In
`@app/src/main/java/com/vultisig/wallet/ui/screens/transaction/TransactionHistoryScreen.kt`:
- Around line 217-223: Remove the redundant remember wrapper: delete the line
declaring val snapshot = remember(groups) { groups } and update the LazyColumn
body to iterate directly over groups (replace snapshot.forEach { ... } with
groups.forEach { ... }); keep the rest of the LazyColumn (modifier,
contentPadding, verticalArrangement, headers) unchanged so the code uses the
original groups list directly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 6994bca6-4ac3-4490-85fb-10dfec7ba807
📒 Files selected for processing (4)
app/src/main/java/com/vultisig/wallet/ui/components/v2/tokenitem/TokenSelectionList.ktapp/src/main/java/com/vultisig/wallet/ui/models/TransactionHistoryViewModel.ktapp/src/main/java/com/vultisig/wallet/ui/screens/ImportFileScreen.ktapp/src/main/java/com/vultisig/wallet/ui/screens/transaction/TransactionHistoryScreen.kt
|
Picked up CodeRabbit feedback (actionable issues only, skipping nitpicks). Working on it. |
Add itemIndex as tiebreaker in TokenSelectionList grid key to guard against hashCode collisions within a group. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Pushed coderabbit fixes in 88a5007. Ready for re-review. Addressed
Declined
Note: both review findings were posted as "outside diff range" comments embedded in the review body (no inline comment IDs were created), so per-thread inline replies aren't possible — this summary covers both. |
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@app/src/main/java/com/vultisig/wallet/ui/components/v2/tokenitem/TokenSelectionList.kt`:
- Around line 161-163: The key generation for items in TokenSelectionList
currently omits groupIndex when title is non-null, causing potential key
collisions; update the key lambda (the key = { itemIndex, item -> ... } in
TokenSelectionList) to always include groupIndex as a prefix (e.g., start the
key with groupIndex followed by the title or a default) so each group's items
are globally unique across groups even when titles repeat.
In `@app/src/main/java/com/vultisig/wallet/ui/screens/ImportFileScreen.kt`:
- Line 206: The LazyColumn call uses itemsIndexed(zipOutputs, key = { _,
zipOutput -> zipOutput.name }) which can throw if names are duplicated; update
the key so it is unique (for example use the index as a tiebreaker or the index
alone) — change the key lambda in the itemsIndexed call to include the index
(e.g. key = { index, zipOutput -> "${index}_${zipOutput.name}" } or key = {
index, _ -> index }) to guarantee uniqueness for itemsIndexed and avoid
IllegalArgumentException.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 7b5747db-125a-4529-93cc-371361f1bdf0
📒 Files selected for processing (4)
app/src/main/java/com/vultisig/wallet/ui/components/v2/tokenitem/TokenSelectionList.ktapp/src/main/java/com/vultisig/wallet/ui/models/TransactionHistoryViewModel.ktapp/src/main/java/com/vultisig/wallet/ui/screens/ImportFileScreen.ktapp/src/main/java/com/vultisig/wallet/ui/screens/transaction/TransactionHistoryScreen.kt
|
Picked up CodeRabbit feedback (actionable issues only, skipping nitpicks). Working on it. |
Make LazyList keys collision-proof: - TokenSelectionList: always prefix item key with groupIndex so duplicate group titles cannot produce identical keys. - ImportFileScreen: include the index as a tiebreaker for zip entry names in case a ZIP contains duplicate entry names. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Addressed CodeRabbit review feedback in 3017147. Comments fixed (2/2):
Comments declined: none. |
|
Picked up CodeRabbit feedback (actionable issues only, skipping nitpicks). Working on it. |
|
Only nitpick feedback remaining — no action taken. Latest CodeRabbit review (run b1bcee63) reports
The only remaining flag is the global Docstring Coverage pre-merge warning (0%). The diff adds a single |
|
CI failure detected. Working on a fix. |
- TokenSelectionList: revert to "$groupIndex-$itemIndex"; the previous key was already collision-free since both indices come from positional iterators, and the hashCode/title segments added no uniqueness benefit. - FileHelper.fileContent: rethrow CancellationException before the generic Exception catch so coroutine cancellation propagates, matching VaultAccountsViewModel/DepositFormViewModel. - ImportFileViewModel.saveFileToAppDir: when fileContent returns null, surface the import_file_not_supported error instead of silently storing null and bailing out of parseFileContent. - ImportFileScreen.ZipOutput: drop the index from the LazyColumn key so it is just zipOutput.name; keeping the index left it identical to the previous key while shifting indices on filtered removals.
|
Addressed @rkokhatskyi's review feedback in e667284. Fixed (4/4 inline comments)
Declined: none. Verification: |
Fixes #4427
Changes
TokenSelectionList.kt: ChangeditemsIndexedkey in the grouped grid from index-based"$groupIndex-$itemIndex"to content-based"${title ?: "group$groupIndex"}-${item.hashCode()}"to avoid duplicate keys across groups.TransactionHistoryViewModel.kt: AddeddateKey: Stringfield toTransactionHistoryGroupUiModeland populated it withdate.toString()when constructing groups.TransactionHistoryScreen.kt: Replaced thestickyHeaderkey from"header_${group.dateSuffix}"to"header_${group.dateKey}"(stable across UiText changes), and wrappedgroupsinremember(groups) { groups }snapshot before iteration; updated all three preview composables to pass the newdateKey.ImportFileScreen.kt: SimplifiedZipOutput'sitemsIndexedkey from"$index:${zipOutput.name}"to justzipOutput.name, dropping the index from the key.Checklist
Summary by CodeRabbit