[Android] Fix CollectionView with an EmptyView or EmptyViewTemplate gets potentially corrupted after being cleared and repopulated a number of times#34208
Conversation
… and ensured that the corresponding integer ViewType variables do not collide with each other
|
Hey there @@davidlilwall! Thank you so much for your PR! Someone from the team will get assigned to your PR shortly and we'll get it reviewed. |
|
@dotnet-policy-service agree |
🤖 AI Summary📊 Expand Full Review —
|
| # | Source | Approach | Test Result | Files Changed | Notes |
|---|---|---|---|---|---|
| PR | PR #34208 | Avoid redundant view-type churn by short-circuiting unchanged header/footer/empty assignments and regenerate each special view type from the current max value to prevent collisions. | ⏳ PENDING (Gate) | src/Controls/src/Core/Handlers/Items/Android/Adapters/EmptyViewAdapter.cs |
Original PR; no test changes in the PR. |
Issue: #34207 - [Android] CollectionView with an EmptyView or EmptyViewTemplate gets potentially corrupted after being cleared and repopulated a number of times
PR: #34208 - [Android] Fix CollectionView with an EmptyView or EmptyViewTemplate gets potentially corrupted after being cleared and repopulated a number of times
Platforms Affected: Android
Files Changed: 1 implementation, 0 test
Key Findings
- The linked issue is Android-only and reproduces after repeated clear/repopulate cycles, with corruption visible around the 9th reload.
- PR [Android] Fix CollectionView with an EmptyView or EmptyViewTemplate gets potentially corrupted after being cleared and repopulated a number of times #34208 changes only
src/Controls/src/Core/Handlers/Items/Android/Adapters/EmptyViewAdapter.csand adds no automated regression coverage. - The PR approach adds equality guards to avoid unnecessary view-type churn and reassigns special view-type IDs from the current max value + 1 to avoid internal collisions among header/footer/empty slots.
- A prior agent review exists on the PR and recommended an alternative bounded-range strategy; the author pushed back that simple toggling would reuse stale IDs and collide with RecyclerView caching.
- No inline review comments exist; only PR discussion and the linked issue provide edge-case context.
Fix Candidates
| # | Source | Approach | Test Result | Files Changed | Notes |
|---|---|---|---|---|---|
| PR | PR #34208 | Add guards for unchanged special-view assignments and allocate each special view type from the current max special value + 1. | ⏳ PENDING (Gate) | src/Controls/src/Core/Handlers/Items/Android/Adapters/EmptyViewAdapter.cs |
Original PR; no test changes included. |
🚦 Gate — Test Verification
Gate Result: ⚠️ SKIPPED
Platform: android
Mode: Full Verification
- Tests FAIL without fix: ❌
- Tests PASS with fix: ❌
Notes
- PR [Android] Fix CollectionView with an EmptyView or EmptyViewTemplate gets potentially corrupted after being cleared and repopulated a number of times #34208 does not include any UI, unit, or device test changes.
- No repo test exists for issue [Android] CollectionView with an EmptyView or EmptyViewTemplate gets potentially corrupted after being cleared and repopulated a number of times #34207 specifically, so there is no bug-targeted test to run through
verify-tests-fail-without-fix. - Existing related Android UI coverage (for example
Issue11896) exercises CollectionView header/footer/empty-state transitions, but it is not the reproduction for this exact collision bug. - Result: gate skipped due to missing issue-specific regression coverage. This weakens confidence in the PR despite the narrow code change.
Gate Result: ⚠️ SKIPPED
Platform: android
Mode: Full Verification
- Tests FAIL without fix: ❌
- Tests PASS with fix: ❌
Notes
- PR [Android] Fix CollectionView with an EmptyView or EmptyViewTemplate gets potentially corrupted after being cleared and repopulated a number of times #34208 adds no new UI, unit, or device tests.
- The repository has related Android CollectionView EmptyView coverage such as
Issue11896, but no regression test that reproduces issue [Android] CollectionView with an EmptyView or EmptyViewTemplate gets potentially corrupted after being cleared and repopulated a number of times #34207 specifically. - Because there is no issue-specific test to run through
verify-tests-fail-without-fix, full gate verification is skipped rather than overstated.
🔧 Fix — Analysis & Comparison
Gate Result: ⚠️ SKIPPED
Platform: android
Mode: Full Verification
- Tests FAIL without fix: ❌
- Tests PASS with fix: ❌
Notes
- PR [Android] Fix CollectionView with an EmptyView or EmptyViewTemplate gets potentially corrupted after being cleared and repopulated a number of times #34208 does not include any UI, unit, or device test changes.
- No repo test exists for issue [Android] CollectionView with an EmptyView or EmptyViewTemplate gets potentially corrupted after being cleared and repopulated a number of times #34207 specifically, so there is no bug-targeted test to run through
verify-tests-fail-without-fix. - Existing related Android UI coverage (for example
Issue11896) exercises CollectionView header/footer/empty-state transitions, but it is not the reproduction for this exact collision bug. - Result: gate skipped due to missing issue-specific regression coverage. This weakens confidence in the PR despite the narrow code change.
Fix Candidates
| # | Source | Approach | Test Result | Files Changed | Notes |
|---|---|---|---|---|---|
| 1 | try-fix (claude-opus-4.6) | Use a generation-based allocator where header/empty/footer view types are derived from one monotonically increasing generation (gen*3+slot) instead of independent max tracking. |
✅ PASS (proxy: Issue11896) |
1 file | Single-file alternative; uniqueness is structural rather than recomputed. |
| 2 | try-fix (claude-sonnet-4.6) | Install a private RecycledViewPool for EmptyViewAdapter and use bounded A/B slot toggles to refresh empty/header/footer holders without unbounded growth. |
✅ PASS (proxy: Issue11896) |
2 files | Architectural isolation from shared pool, but it expands public API surface via public overrides. |
| 3 | try-fix (gpt-5.3-codex) | Allocate fresh special view types from a monotonically decreasing negative-ID namespace so they never overlap positive item-template types. | ✅ PASS (proxy: Issue11896) |
1 file | Small and localized, but still unbounded and relies on assumptions about item-template view-type sign/range. |
| 4 | try-fix (gemini-3-pro-preview) | Derive stable special view types from content/template identity hashes plus high-bit tags. | ✅ PASS (proxy: Issue11896) |
1 file | Eliminates churn, but hash-based identity can collide and may vary with object identity semantics. |
| 5 | try-fix (cross idea: non-recyclable holders) | Keep fixed constants and mark EmptyViewAdapter holders non-recyclable so they never enter the shared recycled pool. | ✅ PASS (proxy: Issue11896) |
1 file | Simplest robust alternative: one file, no API changes, no unbounded ID strategy, and at most three holders so the allocation cost is negligible. |
| 6 | try-fix (cross idea: stable container) | Use stable wrapper/container holders with fixed view-type constants and swap dynamic content inside the holder during bind. | ✅ PASS (proxy: Issue11896) |
1 file | Avoids ID churn entirely, but significantly increases implementation complexity. |
| 7 | try-fix (cross idea: pool eviction) | Keep fixed constants and explicitly evict stale special-type holders from the shared pool when special content changes. | ✅ PASS (proxy: Issue11896) |
1 file | More direct than dynamic IDs, but still depends on delicate pool-management timing. |
| 8 | try-fix (cross idea: ConcatAdapter isolation) | Split special slots into isolated ConcatAdapter sub-adapters so RecyclerView remaps view-type namespaces automatically. |
✅ PASS (proxy: Issue11896) |
2 files | Structurally strong, but much larger than necessary for a targeted bug fix. |
| PR | PR #34208 | Add guards for unchanged special-view assignments and allocate each special view type from the current max special value + 1. | src/Controls/src/Core/Handlers/Items/Android/Adapters/EmptyViewAdapter.cs |
Original PR; no issue-specific regression test in PR/repo. |
Cross-Pollination
| Model | Round | New Ideas? | Details |
|---|---|---|---|
| claude-opus-4.6 | 2 | Yes | Make special holders non-recyclable so they never enter the shared pool. |
| claude-sonnet-4.6 | 2 | Yes | Use a stable container holder and swap dynamic content inside it. |
| gpt-5.3-codex | 2 | Yes | Evict cached holders for fixed view types from the recycled pool on content change. |
| gemini-3-pro-preview | 2 | Yes | Fixed constants + recycled-pool eviction on property changes. |
| claude-opus-4.6 | 3 | Yes | Use ConcatAdapter with isolated view types for special-slot sub-adapters. |
| claude-sonnet-4.6 | 3 | No | NO NEW IDEAS |
| gpt-5.3-codex | 3 | Yes | Variant of ConcatAdapter with isolated view types and adapter replacement. |
| gemini-3-pro-preview | 3 | No | NO NEW IDEAS |
Exhausted: Yes
Selected Fix: Candidate #5 — It is the best-tested balance of simplicity, locality, and robustness. Unlike the PR, it removes dynamic view-type growth entirely. Unlike the larger alternatives, it stays in one file, avoids API changes, and the non-recyclable-holder cost is bounded because EmptyViewAdapter can only materialize header, empty, and footer holders.
📋 Report — Final Recommendation
Gate Result: ⚠️ SKIPPED
Platform: android
Mode: Full Verification
- Tests FAIL without fix: ❌
- Tests PASS with fix: ❌
Notes
- PR [Android] Fix CollectionView with an EmptyView or EmptyViewTemplate gets potentially corrupted after being cleared and repopulated a number of times #34208 does not include any UI, unit, or device test changes.
- No repo test exists for issue [Android] CollectionView with an EmptyView or EmptyViewTemplate gets potentially corrupted after being cleared and repopulated a number of times #34207 specifically, so there is no bug-targeted test to run through
verify-tests-fail-without-fix. - Existing related Android UI coverage (for example
Issue11896) exercises CollectionView header/footer/empty-state transitions, but it is not the reproduction for this exact collision bug. - Result: gate skipped due to missing issue-specific regression coverage. This weakens confidence in the PR despite the narrow code change.
⚠️ Final Recommendation: REQUEST CHANGES
Phase Status
| Phase | Status | Notes |
|---|---|---|
| Pre-Flight | ✅ COMPLETE | Issue #34207 is Android-only; PR #34208 changes one adapter file and adds no tests. |
| Gate | No issue-specific regression test exists in the PR or repo for #34207, so full fail-without-fix/pass-with-fix verification could not be run. | |
| Try-Fix | ✅ COMPLETE | 8 candidates were executed; all 8 passed the available Android proxy test (Issue11896). Candidate #5 was selected as the best alternative. |
| Report | ✅ COMPLETE |
Summary
PR #34208 targets the right area (EmptyViewAdapter) and addresses the observed Android corruption by reducing unnecessary special-view-type churn. But the PR still depends on a dynamic max + 1 allocation strategy, and it arrives without an issue-specific regression test, so the review cannot prove the reported bug is caught end-to-end.
During the mandatory try-fix exploration, multiple alternative fixes passed the closest existing Android proxy coverage. The strongest of those alternatives is Candidate #5: keep special slot view types fixed and mark EmptyViewAdapter holders non-recyclable so they never re-enter the shared RecyclerView pool. That approach is simpler than the PR, stays localized to one file, avoids unbounded ID growth entirely, and directly addresses stale-holder reuse.
Root Cause
The bug stems from EmptyViewAdapter treating header, footer, and empty slots as special RecyclerView view types whose IDs keep changing as CollectionView state flips between empty and populated. Over time those special IDs can overlap with the item adapter's type space or otherwise interact badly with shared RecyclerView recycling, which lets the wrong holder/template reappear in the list.
Fix Quality
The PR is directionally correct because it adds equality guards and prevents the three special IDs from colliding with each other, but it still uses an ever-growing allocator and is unverified against a reproduction test for issue #34207. The selected alternative removes the need for dynamic special-ID allocation altogether while keeping the change small and behaviorally easy to reason about.
MauiBot
added
s/agent-changes-requested
kubaflo
left a comment
kubaflo
left a comment
There was a problem hiding this comment.
Could you please review the AI's summary?
|
The AI recommended approach of just toggling the values rather than incrementing them seems to be a poor suggestion. These values are effectively Ids and are presumably being used for caching (which is why they are being changed in the first place). If the Ids are just toggled then after the first toggle, every other toggle is going to collide with a previously used Id resulting in stale versions of the views potentially being used. |
Multimodal Code Review — Codex + Gemini🟡 Codex (GPT-5.1-Codex): 1 issue (false positive)
🔴 Gemini (Gemini 3 Pro): 3 issues1. Unbounded ViewType growth → RecycledViewPool fragmentation (High)
Example:
2. Reference equality fails for strings and boxed value types (Medium) if (_headerView == value) return;This is reference equality. If the consumer sets
3. Consensus
Overall: The fix correctly prevents view type collisions, which was the original bug. However, it introduces a new concern: unbounded view type growth causing RecycledViewPool fragmentation and defeating view recycling. The reference equality guards also won't work for string/value-type Header/Footer content. Consider using stable view type IDs with proper bind-time updates instead of ever-increasing IDs. |
|
The fix does not introduce the new concern of 'unbounded view type growth' - that was a concern which very obviously existed in the original code. The fix mitigates this concern rather than introduces it. 'Using stable view type IDs with proper binding-time updates instead of ever-increasing IDs' is of course the preferred solution but that would involve changing the code in other files as well. This fix simply fixes two bugs in the EmptyViewAdapter so that the code works in the way the original coder presumably intended it to. Consequently it fixes the Issue that the pull-request is linked to. However it does not seek to fix more general issues with the code. |
3 participants
Description of Change
Added guards to the property setters for HeaderView, EmptyView etc in EmptyViewAdapter to prevent _headerViewType, _emptyItemViewType, and _footerViewType getting repeatedly unnecessarily incremented. Also ensured that the corresponding integer ViewType variables do not collide with each other.
Issues Fixed
Fixes #34207