Skip to content

fix: dedup node lists to prevent LazyColumn duplicate-key crash#6193

Merged
jamesarich merged 2 commits into
mainfrom
claude/beautiful-napier-487d5e
Jul 10, 2026
Merged

fix: dedup node lists to prevent LazyColumn duplicate-key crash#6193
jamesarich merged 2 commits into
mainfrom
claude/beautiful-napier-487d5e

Conversation

@jamesarich

@jamesarich jamesarich commented Jul 10, 2026

Copy link
Copy Markdown
Collaborator

Why

Crashlytics issue 159e25351be7042db517f3af684684db is an umbrella for IllegalArgumentException: Key "X" was already used crashes in LazyColumn. Two variants are already fixed (contactKey via #6169, tcp-discovered via #6189). This closes out the third, untriaged variant: bare-numeric keys — observed "0" on 2.8.0-internal.4 and "2322" on 2.7.13.

Both come from lists keyed on a raw Int node num:

  • NodeListScreenitems(nodes, key = { it.num })
  • map ClusterItemsListDialogitems(items, key = { it.node.num })

num = 0 is the unknown-node sentinel, and a brief double-emission during an active-DB switch (SwitchingNodeInfoReadDataSource's flatMapLatest) can momentarily surface two entries with the same num, tripping Compose's unique-key requirement. The DB itself can't hold duplicate nums (num is the @PrimaryKey), so this is a transient render-boundary collision.

🐛 Fix

Dedup with distinctBy { it.num } at both render sites.

Deliberately not the "${num}_$index" composite-key pattern used elsewhere in the codebase for timestamp keys: NodeListScreen uses Modifier.animateItem() and the list reorders (sort by last-heard/distance). Index-in-key changes when an item moves, which turns move animations into remove+insert. Dedup keeps key = { it.num } stable and unique, preserving the animation.

Testing Performed

No regression test added — the fix is a stdlib distinctBy one-liner at the UI boundary with no custom list logic to unit-test (the collision only manifests through Compose's items key path). Baseline verification passed locally: spotlessCheck detekt assembleDebug, :androidApp:testGoogleDebugUnitTest, and :feature:node:allTests all green.

🤖 Generated with Claude Code

Summary by CodeRabbit

  • Bug Fixes
    • Prevented duplicate entries from appearing in the map’s cluster items list by deduplicating nodes by their numeric ID.
    • Improved the node list by deduplicating temporarily repeated node IDs so the app bar count and displayed rows remain consistent.
    • Preserved smooth list reordering animations by keeping stable item keys based on the node numeric ID.

The bare-numeric variant of the LazyColumn duplicate-key crash (Crashlytics
159e25351be7042db517f3af684684db; observed keys "0" and "2322") comes from two
lists keyed on a raw Int node num. num=0 is the unknown-node sentinel and a
brief double-emission during an active-DB switch can momentarily surface two
entries with the same num, tripping Compose's unique-key requirement.

Dedup with distinctBy at both render sites rather than switching to a
"num_index" composite key: NodeListScreen uses Modifier.animateItem() and
reorders, and index-based keys would break move animations (a moved item gets a
new key and is treated as remove+insert). Dedup keeps key = { it.num } stable
and unique.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@github-actions github-actions Bot added the bugfix PR tag label Jul 10, 2026
@coderabbitai

coderabbitai Bot commented Jul 10, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: dac50bb5-adda-4ed4-bde8-1a374a46333b

📥 Commits

Reviewing files that changed from the base of the PR and between a873f64 and 4d2312c.

📒 Files selected for processing (1)
  • feature/node/src/commonMain/kotlin/org/meshtastic/feature/node/list/NodeListScreen.kt
🚧 Files skipped from review as they are similar to previous changes (1)
  • feature/node/src/commonMain/kotlin/org/meshtastic/feature/node/list/NodeListScreen.kt

📝 Walkthrough

Walkthrough

ClusterItemsListDialog and NodeListScreen now deduplicate rendered entries by node number before passing them to keyed LazyColumn lists. Existing node-number keys are preserved, and NodeListScreen documents temporary duplicate upstream entries.

Changes

Node list rendering

Layer / File(s) Summary
Deduplicate keyed node items
androidApp/src/google/kotlin/org/meshtastic/app/map/component/ClusterItemsListDialog.kt, feature/node/src/commonMain/kotlin/org/meshtastic/feature/node/list/NodeListScreen.kt
Map cluster items and node list entries use distinctBy on node numbers before rendering, while retaining node-number keys. NodeListScreen documents duplicate upstream emissions and the continued use of node-number keys.

Estimated code review effort: 2 (Simple) | ~10 minutes

🚥 Pre-merge checks | ✅ 4
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly describes the main change: deduplicating node lists to prevent LazyColumn duplicate-key crashes.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

Comment @coderabbitai help to get the list of available commands.

@jamesarich
jamesarich enabled auto-merge July 10, 2026 13:13

@coderabbitai coderabbitai Bot 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.

Actionable comments posted: 1

🤖 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
`@feature/node/src/commonMain/kotlin/org/meshtastic/feature/node/list/NodeListScreen.kt`:
- Around line 238-242: Deduplicate the node list once before rendering so the
displayed count matches the rows. In NodeListScreen, compute a shared list using
distinctBy { it.num }, pass its size to MainAppBar, and reuse that list in the
LazyColumn items call while preserving key = { it.num } and animateItem()
behavior.
🪄 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 Plus

Run ID: ce379c78-f3ee-45cb-b6b2-7b969266909b

📥 Commits

Reviewing files that changed from the base of the PR and between 0a3e000 and a873f64.

📒 Files selected for processing (2)
  • androidApp/src/google/kotlin/org/meshtastic/app/map/component/ClusterItemsListDialog.kt
  • feature/node/src/commonMain/kotlin/org/meshtastic/feature/node/list/NodeListScreen.kt

Hoist distinctBy { it.num } out of the LazyColumn items() call into a
remember(allNodes) val so the deduped list is the single source for the app-bar
count (nodes.size), the rendered rows, and the empty-state check — they can no
longer disagree — and it's computed per emission rather than per composition.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@jamesarich
jamesarich disabled auto-merge July 10, 2026 13:19
@jamesarich
jamesarich enabled auto-merge July 10, 2026 13:19
@jamesarich
jamesarich added this pull request to the merge queue Jul 10, 2026
Merged via the queue into main with commit 228fa8a Jul 10, 2026
17 checks passed
@jamesarich
jamesarich deleted the claude/beautiful-napier-487d5e branch July 10, 2026 13:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bugfix PR tag

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant