[Windows] Fix for Grouping collection view without data template results in displaying the default string representation of the object#28617
Conversation
|
Hey there @SyedAbdulAzeemSF4852! Thank you so much for your PR! Someone from the team will get assigned to your PR shortly and we'll get it reviewed. |
|
/azp run |
|
Azure Pipelines successfully started running 3 pipeline(s). |
| IsSourceGrouped = true, | ||
| ItemsPath = new Microsoft.UI.Xaml.PropertyPath(nameof(GroupTemplateContext.Items)) | ||
| }; | ||
| return new CollectionViewSource |
There was a problem hiding this comment.
Can encapsulate the logic within separate methods for better readability:
private CollectionViewSource CreateGroupedCollectionViewSource(object itemsSource, object itemTemplate)
{
return new CollectionViewSource
{
Source = TemplatedItemSourceFactory.CreateGrouped(itemsSource, itemTemplate,
ItemsView.GroupHeaderTemplate, ItemsView.GroupFooterTemplate, Element, mauiContext: MauiContext),
IsSourceGrouped = true,
ItemsPath = new Microsoft.UI.Xaml.PropertyPath(nameof(GroupTemplateContext.Items))
};
}
// Creates and returns a grouped CollectionViewSource using itemsSource as the data source when an itemTemplate is not defined.
private CollectionViewSource CreateDefaultGroupedCollectionViewSource(object itemsSource)
{
return new CollectionViewSource
{
Source = itemsSource,
IsSourceGrouped = true,
};
}
return (itemTemplate is not null && itemsSource is not null)
? CreateGroupedCollectionViewSource(itemsSource, itemTemplate)
: CreateDefaultGroupedCollectionViewSource(itemsSource);
There was a problem hiding this comment.
@jsuarezruiz , As suggested have encapsulated the logic within separate methods for better readability.
| public void GroupedCollectionViewWithoutDataTemplate() | ||
| { | ||
| App.WaitForElement("CollectionViewWithoutDataTemplate"); | ||
| VerifyScreenshot(); |
There was a problem hiding this comment.
Pending snapshots already available in the latest build.
Example, iOS:
Could you commit the images?
There was a problem hiding this comment.
@jsuarezruiz , I have committed the snapshots.
|
/azp run |
|
Azure Pipelines successfully started running 3 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 3 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 3 pipeline(s). |
| } | ||
| } | ||
|
|
||
| private CollectionViewSource CreateGroupedCollectionViewSource(IEnumerable itemsSource, DataTemplate itemTemplate) |
There was a problem hiding this comment.
Can we drop private we don t use it in our code base
There was a problem hiding this comment.
@rmarinho , As suggested, I've removed the private modifier.
| } | ||
|
|
||
| // Creates and returns a grouped CollectionViewSource using itemsSource as the data source when an itemTemplate is not defined. | ||
| private CollectionViewSource CreateDefaultGroupedCollectionViewSource(IEnumerable itemsSource) |
There was a problem hiding this comment.
| private CollectionViewSource CreateDefaultGroupedCollectionViewSource(IEnumerable itemsSource) | |
| CollectionViewSource CreateDefaultGroupedCollectionViewSource(IEnumerable itemsSource) |
There was a problem hiding this comment.
@rmarinho , I've made the changes as mentioned.
| } | ||
|
|
||
| // Creates and returns a grouped CollectionViewSource using itemsSource as the data source when an itemTemplate is not defined. | ||
| private CollectionViewSource CreateDefaultGroupedCollectionViewSource(IEnumerable itemsSource) |
There was a problem hiding this comment.
What happens if itemsSource is null?
There was a problem hiding this comment.
|
/azp run MAUI-UITests-public |
|
Azure Pipelines successfully started running 1 pipeline(s). |
e70284c to
2b56ab2
Compare
There was a problem hiding this comment.
Pull request overview
This PR fixes a Windows-specific issue where CollectionView with grouping displays the default string representation (ToString()) of objects when no ItemTemplate is provided. The fix ensures that when an ItemTemplate is not specified, the data is bound directly without applying a default template that causes binding issues.
Key Changes:
- Modified the CollectionViewSource creation logic to handle grouped collections differently based on whether an ItemTemplate is provided
- Added two helper methods to distinguish between templated and non-templated grouped collection view scenarios
Reviewed changes
Copilot reviewed 3 out of 7 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| src/Controls/src/Core/Handlers/Items/GroupableItemsViewHandler.Windows.cs | Refactored CollectionViewSource creation to conditionally apply templates, extracting logic into separate methods for templated vs. non-templated grouped collections |
| src/Controls/tests/TestCases.HostApp/Issues/Issue23293.cs | Added test case demonstrating grouped CollectionView without ItemTemplate using animal groups |
| src/Controls/tests/TestCases.Shared.Tests/Tests/Issues/Issue23293.cs | Added UI test to verify the fix via screenshot validation |
🤖 AI Summary📊 Expand Full Review —
|
| # | Source | Approach | Test Result | Files Changed | Notes |
|---|---|---|---|---|---|
| PR | PR #28617 | When grouping on Windows, use templated grouped source only when both itemTemplate and itemsSource are non-null; otherwise create a grouped CollectionViewSource directly from itemsSource without templates. |
PENDING (Gate) | src/Controls/src/Core/Handlers/Items/GroupableItemsViewHandler.Windows.cs, src/Controls/tests/TestCases.HostApp/Issues/Issue23293.cs, src/Controls/tests/TestCases.Shared.Tests/Tests/Issues/Issue23293.cs, snapshot files |
Original PR |
🚦 Gate — Test Verification
Result: FAILED
Gate Result: FAILED
Platform: windows
Mode: Full Verification
- Tests FAIL without fix:
- Tests PASS with fix:
Notes
- Without the PR fix,
Issue23293.GroupedCollectionViewWithoutDataTemplatefailed as expected with a visual regression against the stored snapshot. - With the PR fix applied, the same verification failed on Windows with a timeout waiting for the target UI element.
- Gate conclusion: the test catches the bug, but the PR's current fix does not make the verification pass on the selected platform.
🔧 Fix — Analysis & Comparison
Fix Candidates
| # | Source | Approach | Test Result | Files Changed | Notes |
|---|---|---|---|---|---|
| 1 | try-fix | Move the no-template decision into TemplatedItemSourceFactory.CreateGrouped, returning raw grouped data when no item template exists, and let the Windows handler set ItemsPath only for GroupedItemTemplateCollection. |
PASS | 3 files | Passing alternative, but broader than candidate 2 because it changes both the factory and handler. |
| 2 | try-fix | Keep grouped wrapper behavior intact, but in GroupedItemTemplateCollection.CreateGroupTemplateContext expose raw group items when _itemTemplate is null instead of wrapping them in ItemTemplateContext. |
PASS | 1 code file + Windows snapshot | Strongest passing candidate: smallest production change and preserves the grouped wrapper pipeline. |
| 3 | try-fix | Change the default projection in TemplatedItemSourceFactory.Create for null templates and suppress wrapper text by overriding GroupTemplateContext.ToString(). |
FAIL | 2 files | Still produced a Windows screenshot mismatch (~4.09% difference). |
| 4 | try-fix | Introduce an interface so GroupTemplateContext can unwrap item-wrapper collections back to their original data source when ItemTemplate is null. |
FAIL | 4 files | Functionally close, but still failed visual verification (~4.09% difference). |
| 5 | try-fix | Inject an internal grouped fallback DataTemplate on Windows when ItemTemplate is null, while keeping grouped wrappers and ItemsPath intact. |
FAIL | 1 file | Avoided null-template data branching, but still produced the same ~4.09% screenshot mismatch. |
| 6 | try-fix | Use native DisplayMemberPath = nameof(ItemTemplateContext.Item) for grouped Windows items without an item template. |
PASS (weakened test) | 1 code file + 1 test file | Functional text-based check passed only after weakening the verification away from screenshot validation. |
| 7 | try-fix | Leave the data pipeline intact and instead route grouped null-template items through ItemContentControl, which renders a native fallback TextBlock when FormsDataTemplate is null. |
PASS | 2 production files + HostApp test data | Passed screenshot verification, but is broader than candidate 2 and reaches deeper into the rendering path. |
| PR | PR #28617 | Special-case grouped Windows CollectionViewSource creation in GroupableItemsViewHandler.Windows so templated grouping is used only when both itemTemplate and itemsSource are non-null, otherwise bind the raw grouped source directly. |
FAIL (Gate) | 3 code/test files + snapshots | Original PR times out on Windows during verification. |
Cross-Pollination
| Model | Round | New Ideas? | Details |
|---|---|---|---|
| claude-opus-4.6 | 1 | Yes | Factory-level grouped-source fallback plus adaptive handler ItemsPath passed. |
| claude-sonnet-4.6 | 1 | Yes | Grouped item materialization fallback inside GroupedItemTemplateCollection passed. |
| gpt-5.3-codex | 1 | Yes | Projection-layer passthrough plus GroupTemplateContext.ToString() failed. |
| gemini-3-pro-preview | 1 | Yes | Wrapper-unwrapping in GroupTemplateContext failed. |
| gpt-5.3-codex | 2 | Yes | Injecting a fallback grouped item template failed. |
| gemini-3-pro-preview | 2 | Yes | Native DisplayMemberPath passed only with weakened verification. |
| claude-sonnet-4.6 | 2 | Yes | Suggested rendering-layer fallback rather than another data-pipeline change. |
| claude-opus-4.6 | 2 | Yes | Suggested routing grouped null-template items through ItemContentControl; that approach passed as candidate 7. |
| gpt-5.3-codex | 3 | Yes | Proposed source-shaping to lightweight bindable objects; not run because stronger passing alternatives already existed. |
| gemini-3-pro-preview | 3 | Yes | Proposed ContainerContentChanging manual unwrapping; not run because stronger passing alternatives already existed. |
| claude-opus-4.6 | 3 | No | No new materially different ideas after comparing all seven attempts; passing strategies already cover the viable layers. |
| claude-sonnet-4.6 | 3 | Blocked | Timed out during final exhaustion query. |
Exhausted: No
Selected Fix: Candidate #2 Best balance of correctness, simplicity, and consistency. It fixes the grouped no-template case at the point where bad wrappers are created, without broad rendering-path changes or weakened verification.
📋 Report — Final Recommendation
Final Recommendation: REQUEST CHANGES
Phase Status
| Phase | Status | Notes |
|---|---|---|
| Pre-Flight | COMPLETE | Linked issue #23293 is Windows-only; PR changes one Windows handler plus UI test assets. |
| Gate | FAILED | Windows verification reproduced the bug without the fix, but the PR fix still failed with a timeout waiting for the target element. |
| Try-Fix | COMPLETE | 7 attempts: 3 strong passes, 1 weakened pass, 3 failures. Best alternative was candidate 2. |
| Report | COMPLETE |
Summary
The PR should not be merged in its current form. The gate showed that the new test catches the bug, but the PR's handler-level fix still fails on Windows. Independent exploration found multiple alternatives that do pass, which means the problem is fixable but the current implementation is not the best path.
Root Cause
The failure is tied to how Windows grouped CollectionView materializes or renders leaf items when ItemTemplate is null. The PR tries to bypass that by branching in GroupableItemsViewHandler.Windows, but verified Windows execution still times out. The strongest alternative fix instead addresses the grouped item materialization point directly by exposing raw group items when no template exists.
Fix Quality
The PR fix is not shippable as written because it fails the Windows gate. Candidate 2 is a better direction: it is smaller, stays within the existing grouped wrapper model, and passed validation without weakening the test. Candidate 7 also passed, but it is broader because it changes the rendering path and required more supporting adjustments.
📋 Expand PR Finalization Review
PR #28617 Finalization Review
Title Review
Current: [Windows] Fix for Grouping collection view without data template results in displaying the default string representation of the object
Assessment: Needs a tighter, more searchable title.
Recommended: [Windows] CollectionView: Fix grouped rendering without ItemTemplate
Why: the current title repeats the issue text verbatim and is harder to scan in git history. The recommended title keeps the platform + component prefix and describes the actual behavioral change.
Description Review
Assessment: Decent foundation, but not merge-ready as written.
What is good:
- Includes a real root-cause section.
- Includes a behavior description and issue link.
- Accurately frames the change as Windows-specific.
What should be updated:
- Add the required NOTE block at the very top from
.github/PULL_REQUEST_TEMPLATE.md. - Prefer the standard
### Description of Change/### Issues Fixedstructure instead of leading with### Issue Details. - Explicitly mention that the PR also adds UI coverage/snapshots for the reproduced scenario.
- If the implementation changes, update the description accordingly: the current text says the grouped source is created directly from
itemsSourcewhenitemTemplateis null, but that fallback currently appears to introduce a grouped header/footer regression risk.
Action: Keep the existing root-cause narrative, prepend the NOTE block, and revise the implementation details before merge.
Code Review Findings
Critical Issues
Grouped header/footer templates are likely regressed in the new Windows fallback path
- File:
src/Controls/src/Core/Handlers/Items/GroupableItemsViewHandler.Windows.cs - Problem: The new
CreateDefaultGroupedCollectionViewSource(IEnumerable itemsSource)path bypassesTemplatedItemSourceFactory.CreateGrouped(...)and returns the raw grouped source directly whenItemTemplateis null. - Why this matters: On Windows, grouped rendering relies on
GroupTemplateContextfor group metadata.GroupedItemTemplateCollectioncreates that wrapper and populatesHeaderItemTemplateContext/FooterItemTemplateContext(src/Controls/src/Core/Platform/Windows/CollectionView/GroupedItemTemplateCollection.cs,GroupTemplateContext.cs). The Windows group header template then binds itsDataContexttoHeaderItemTemplateContext(src/Controls/src/Core/Platform/Windows/CollectionView/ItemsViewStyles.xaml). If the source is raw groups instead ofGroupTemplateContext,GroupHeaderTemplate/GroupFooterTemplatecannot work correctly. - Evidence: Existing UI coverage already validates grouped header/footer behavior (
src/Controls/tests/TestCases.Shared.Tests/Tests/FeatureMatrix/CollectionView_GroupingFeatureTests.cs:250-308). Those scenarios depend on the same Windows group-template plumbing this fallback now skips. - Recommendation: Do not bypass the grouped wrapper. Preserve
TemplatedItemSourceFactory.CreateGrouped(...)/GroupTemplateContextfor grouped sources, and instead fix the no-ItemTemplatecase lower in the pipeline so group headers/footers continue to function.
Suggestions
- Add a targeted Windows test for the combination:
IsGrouped = true,ItemTemplate = null, andGroupHeaderTemplateand/orGroupFooterTemplateset. The new test page only covers the no-template grouped-items scenario, so it would not catch the regression above. - When the description is updated, mention that the fix is Windows handler code plus cross-platform UI snapshots, so reviewers do not misread the added screenshots as cross-platform behavior changes.
Looks Good
- The PR includes a focused reproduction page and a UI test for the reported issue (
Issue23293), which is the right level of coverage for the original bug. - The handler change is narrowly scoped to the Windows grouped CollectionView path.
Overall Assessment
The metadata needs a small cleanup, but the larger concern is the Windows grouped-template regression risk. I would not consider this PR ready to merge until the grouped no-ItemTemplate fix preserves GroupTemplateContext behavior for GroupHeaderTemplate / GroupFooterTemplate.
kubaflo
left a comment
There was a problem hiding this comment.
Could you please apply the AI's suggestions?
🤖 AI Summary📊 Expand Full Review —
|
| # | Source | Approach | Test Result | Files Changed | Notes |
|---|---|---|---|---|---|
| PR | PR #28617 | When grouping on Windows, use templated grouped source only when both itemTemplate and itemsSource are non-null; otherwise create a grouped CollectionViewSource directly from itemsSource without templates. |
PENDING (Gate) | src/Controls/src/Core/Handlers/Items/GroupableItemsViewHandler.Windows.cs, src/Controls/tests/TestCases.HostApp/Issues/Issue23293.cs, src/Controls/tests/TestCases.Shared.Tests/Tests/Issues/Issue23293.cs, snapshot files |
Original PR |
Issue: #23293 - [MAUI] I6 Grouping - 'Grouping for Vertical list without DataTemplates' page loading exception
PR: #28617 - [Windows] Fix for Grouping collection view without data template results in displaying the default string representation of the object
Platforms Affected: Windows (issue), PR claims validation on Windows/Android/iOS/Mac
Files Changed: 1 implementation, 2 test source files, 4 snapshot assets
Key Findings
- The linked issue is a Windows-only CollectionView grouping bug: grouped items without an
ItemTemplateshow the default string representation instead of the animal names. - The implementation change is isolated to
src/Controls/src/Core/Handlers/Items/GroupableItemsViewHandler.Windows.cs; added tests are a HostApp issue page plus a shared UI screenshot test (Issue23293). - Review discussion requested readability refactoring and snapshot commits; both were addressed in follow-up commits.
- A reviewer explicitly asked what happens when
itemsSourceis null, and the author answered that the new fallback path can create a groupedCollectionViewSourcewith a nullSourceif upstream null checks are removed. - A prior agent review exists on the PR and the current labels already indicate a previous
s/agent-reviewed,s/agent-changes-requested,s/agent-gate-failed, ands/agent-fix-winoutcome.
Edge Cases Mentioned
itemsSource == nullon the grouped fallback path remains a discussed edge case in review comments.- The added UI test is screenshot-based and verifies rendered output, not the null-itemsSource behavior.
Fix Candidates
| # | Source | Approach | Test Result | Files Changed | Notes |
|---|---|---|---|---|---|
| PR | PR #28617 | On Windows grouped CollectionView, use the templated grouped source only when both ItemTemplate and ItemsSource are non-null; otherwise create a grouped CollectionViewSource directly from ItemsSource. |
FAILED (Gate) | src/Controls/src/Core/Handlers/Items/GroupableItemsViewHandler.Windows.cs, src/Controls/tests/TestCases.HostApp/Issues/Issue23293.cs, src/Controls/tests/TestCases.Shared.Tests/Tests/Issues/Issue23293.cs, snapshots |
Imported from prior PR agent review summary already posted on the PR. |
🚦 Gate — Test Verification
Result: FAILED
Gate Result: FAILED
Platform: windows
Mode: Full Verification
- Tests FAIL without fix:
- Tests PASS with fix:
Notes
- Without the PR fix,
Issue23293.GroupedCollectionViewWithoutDataTemplatefailed as expected with a visual regression against the stored snapshot. - With the PR fix applied, the same verification failed on Windows with a timeout waiting for the target UI element.
- Gate conclusion: the test catches the bug, but the PR's current fix does not make the verification pass on the selected platform.
Result: FAILED
Gate Result: FAILED
Platform: windows
Mode: Full Verification
- Tests FAIL without fix: Yes
- Tests PASS with fix: No
Notes
- Imported from prior PR agent review summary already posted on PR [Windows] Fix for Grouping collection view without data template results in displaying the default string representation of the object #28617.
- Without the PR fix,
Issue23293.GroupedCollectionViewWithoutDataTemplatefailed as expected with a visual regression against the stored snapshot. - With the PR fix applied, the same verification failed on Windows with a timeout waiting for the target UI element.
- Gate conclusion: the test catches the bug, but the PR's current fix does not make the verification pass on the selected platform.
🔧 Fix — Analysis & Comparison
Result: FAILED
Gate Result: FAILED
Platform: windows
Mode: Full Verification
- Tests FAIL without fix:
- Tests PASS with fix:
Notes
- Without the PR fix,
Issue23293.GroupedCollectionViewWithoutDataTemplatefailed as expected with a visual regression against the stored snapshot. - With the PR fix applied, the same verification failed on Windows with a timeout waiting for the target UI element.
- Gate conclusion: the test catches the bug, but the PR's current fix does not make the verification pass on the selected platform.
Fix Candidates
| # | Source | Approach | Test Result | Files Changed | Notes |
|---|---|---|---|---|---|
| 1 | try-fix | Move the no-template decision into TemplatedItemSourceFactory.CreateGrouped, returning raw grouped data when no item template exists, and let the Windows handler set ItemsPath only for GroupedItemTemplateCollection. |
PASS | 3 files | Passing alternative, but broader than candidate 2 because it changes both the factory and handler. |
| 2 | try-fix | Keep grouped wrapper behavior intact, but in GroupedItemTemplateCollection.CreateGroupTemplateContext expose raw group items when _itemTemplate is null instead of wrapping them in ItemTemplateContext. |
PASS | 1 code file + Windows snapshot | Strongest passing candidate: smallest production change and preserves the grouped wrapper pipeline. |
| 3 | try-fix | Change the default projection in TemplatedItemSourceFactory.Create for null templates and suppress wrapper text by overriding GroupTemplateContext.ToString(). |
FAIL | 2 files | Still produced a Windows screenshot mismatch (~4.09% difference). |
| 4 | try-fix | Introduce an interface so GroupTemplateContext can unwrap item-wrapper collections back to their original data source when ItemTemplate is null. |
FAIL | 4 files | Functionally close, but still failed visual verification (~4.09% difference). |
| 5 | try-fix | Inject an internal grouped fallback DataTemplate on Windows when ItemTemplate is null, while keeping grouped wrappers and ItemsPath intact. |
FAIL | 1 file | Avoided null-template data branching, but still produced the same ~4.09% screenshot mismatch. |
| 6 | try-fix | Use native DisplayMemberPath = nameof(ItemTemplateContext.Item) for grouped Windows items without an item template. |
PASS (weakened test) | 1 code file + 1 test file | Functional text-based check passed only after weakening the verification away from screenshot validation. |
| 7 | try-fix | Leave the data pipeline intact and instead route grouped null-template items through ItemContentControl, which renders a native fallback TextBlock when FormsDataTemplate is null. |
PASS | 2 production files + HostApp test data | Passed screenshot verification, but is broader than candidate 2 and reaches deeper into the rendering path. |
| PR | PR #28617 | Special-case grouped Windows CollectionViewSource creation in GroupableItemsViewHandler.Windows so templated grouping is used only when both itemTemplate and itemsSource are non-null, otherwise bind the raw grouped source directly. |
FAIL (Gate) | 3 code/test files + snapshots | Original PR times out on Windows during verification. |
Cross-Pollination
| Model | Round | New Ideas? | Details |
|---|---|---|---|
| claude-opus-4.6 | 1 | Yes | Factory-level grouped-source fallback plus adaptive handler ItemsPath passed. |
| claude-sonnet-4.6 | 1 | Yes | Grouped item materialization fallback inside GroupedItemTemplateCollection passed. |
| gpt-5.3-codex | 1 | Yes | Projection-layer passthrough plus GroupTemplateContext.ToString() failed. |
| gemini-3-pro-preview | 1 | Yes | Wrapper-unwrapping in GroupTemplateContext failed. |
| gpt-5.3-codex | 2 | Yes | Injecting a fallback grouped item template failed. |
| gemini-3-pro-preview | 2 | Yes | Native DisplayMemberPath passed only with weakened verification. |
| claude-sonnet-4.6 | 2 | Yes | Suggested rendering-layer fallback rather than another data-pipeline change. |
| claude-opus-4.6 | 2 | Yes | Suggested routing grouped null-template items through ItemContentControl; that approach passed as candidate 7. |
| gpt-5.3-codex | 3 | Yes | Proposed source-shaping to lightweight bindable objects; not run because stronger passing alternatives already existed. |
| gemini-3-pro-preview | 3 | Yes | Proposed ContainerContentChanging manual unwrapping; not run because stronger passing alternatives already existed. |
| claude-opus-4.6 | 3 | No | No new materially different ideas after comparing all seven attempts; passing strategies already cover the viable layers. |
| claude-sonnet-4.6 | 3 | Blocked | Timed out during final exhaustion query. |
Exhausted: No
Selected Fix: Candidate #2 Best balance of correctness, simplicity, and consistency. It fixes the grouped no-template case at the point where bad wrappers are created, without broad rendering-path changes or weakened verification.
Provenance
- Imported from prior PR agent review summary already posted on PR [Windows] Fix for Grouping collection view without data template results in displaying the default string representation of the object #28617.
📋 Report — Final Recommendation
Final Recommendation: REQUEST CHANGES
Phase Status
| Phase | Status | Notes |
|---|---|---|
| Pre-Flight | COMPLETE | Linked issue #23293 is Windows-only; PR changes one Windows handler plus UI test assets. |
| Gate | FAILED | Windows verification reproduced the bug without the fix, but the PR fix still failed with a timeout waiting for the target element. |
| Try-Fix | COMPLETE | 7 attempts: 3 strong passes, 1 weakened pass, 3 failures. Best alternative was candidate 2. |
| Report | COMPLETE |
Summary
The PR should not be merged in its current form. The gate showed that the new test catches the bug, but the PR's handler-level fix still fails on Windows. Independent exploration found multiple alternatives that do pass, which means the problem is fixable but the current implementation is not the best path.
Root Cause
The failure is tied to how Windows grouped CollectionView materializes or renders leaf items when ItemTemplate is null. The PR tries to bypass that by branching in GroupableItemsViewHandler.Windows, but verified Windows execution still times out. The strongest alternative fix instead addresses the grouped item materialization point directly by exposing raw group items when no template exists.
Fix Quality
The PR fix is not shippable as written because it fails the Windows gate. Candidate 2 is a better direction: it is smaller, stays within the existing grouped wrapper model, and passed validation without weakening the test. Candidate 7 also passed, but it is broader because it changes the rendering path and required more supporting adjustments.
Final Recommendation: REQUEST CHANGES
Phase Status
| Phase | Status | Notes |
|---|---|---|
| Pre-Flight | COMPLETE | Linked issue #23293 is Windows-only; PR changes one Windows handler plus UI test assets. |
| Gate | FAILED | Windows verification reproduced the bug without the fix, but the PR fix still failed with a timeout waiting for the target element. |
| Try-Fix | COMPLETE | Imported prior results: 7 attempts, 4 passes including 1 weakened pass; best alternative was candidate 2. |
| Report | COMPLETE | Reconstructed from prior PR agent review summary. |
Summary
The PR should not be merged in its current form. The gate showed that the new test catches the bug, but the PR's handler-level fix still fails on Windows. Independent exploration previously found multiple alternatives that do pass, which means the problem is fixable but the current implementation is not the best path.
Root Cause
The failure is tied to how Windows grouped CollectionView materializes or renders leaf items when ItemTemplate is null. The PR tries to bypass that by branching in GroupableItemsViewHandler.Windows, but verified Windows execution still timed out. The strongest alternative fix instead addresses the grouped item materialization point directly by exposing raw group items when no template exists.
Fix Quality
The PR fix is not shippable as written because it fails the Windows gate. Candidate 2 is a better direction: it is smaller, stays within the existing grouped wrapper model, and passed validation without weakening the test. Candidate 7 also passed, but it is broader because it changes the rendering path and required more supporting adjustments.
📋 Expand PR Finalization Review
PR #28617 Finalization Review
Title Review
Current: [Windows] Fix for Grouping collection view without data template results in displaying the default string representation of the object
Assessment: Needs a tighter, more searchable title.
Recommended: [Windows] CollectionView: Fix grouped rendering without ItemTemplate
Why: the current title repeats the issue text verbatim and is harder to scan in git history. The recommended title keeps the platform + component prefix and describes the actual behavioral change.
Description Review
Assessment: Decent foundation, but not merge-ready as written.
What is good:
- Includes a real root-cause section.
- Includes a behavior description and issue link.
- Accurately frames the change as Windows-specific.
What should be updated:
- Add the required NOTE block at the very top from
.github/PULL_REQUEST_TEMPLATE.md. - Prefer the standard
### Description of Change/### Issues Fixedstructure instead of leading with### Issue Details. - Explicitly mention that the PR also adds UI coverage/snapshots for the reproduced scenario.
- If the implementation changes, update the description accordingly: the current text says the grouped source is created directly from
itemsSourcewhenitemTemplateis null, but that fallback currently appears to introduce a grouped header/footer regression risk.
Action: Keep the existing root-cause narrative, prepend the NOTE block, and revise the implementation details before merge.
Code Review Findings
Critical Issues
Grouped header/footer templates are likely regressed in the new Windows fallback path
- File:
src/Controls/src/Core/Handlers/Items/GroupableItemsViewHandler.Windows.cs - Problem: The new
CreateDefaultGroupedCollectionViewSource(IEnumerable itemsSource)path bypassesTemplatedItemSourceFactory.CreateGrouped(...)and returns the raw grouped source directly whenItemTemplateis null. - Why this matters: On Windows, grouped rendering relies on
GroupTemplateContextfor group metadata.GroupedItemTemplateCollectioncreates that wrapper and populatesHeaderItemTemplateContext/FooterItemTemplateContext(src/Controls/src/Core/Platform/Windows/CollectionView/GroupedItemTemplateCollection.cs,GroupTemplateContext.cs). The Windows group header template then binds itsDataContexttoHeaderItemTemplateContext(src/Controls/src/Core/Platform/Windows/CollectionView/ItemsViewStyles.xaml). If the source is raw groups instead ofGroupTemplateContext,GroupHeaderTemplate/GroupFooterTemplatecannot work correctly. - Evidence: Existing UI coverage already validates grouped header/footer behavior (
src/Controls/tests/TestCases.Shared.Tests/Tests/FeatureMatrix/CollectionView_GroupingFeatureTests.cs:250-308). Those scenarios depend on the same Windows group-template plumbing this fallback now skips. - Recommendation: Do not bypass the grouped wrapper. Preserve
TemplatedItemSourceFactory.CreateGrouped(...)/GroupTemplateContextfor grouped sources, and instead fix the no-ItemTemplatecase lower in the pipeline so group headers/footers continue to function.
Suggestions
- Add a targeted Windows test for the combination:
IsGrouped = true,ItemTemplate = null, andGroupHeaderTemplateand/orGroupFooterTemplateset. The new test page only covers the no-template grouped-items scenario, so it would not catch the regression above. - When the description is updated, mention that the fix is Windows handler code plus cross-platform UI snapshots, so reviewers do not misread the added screenshots as cross-platform behavior changes.
Looks Good
- The PR includes a focused reproduction page and a UI test for the reported issue (
Issue23293), which is the right level of coverage for the original bug. - The handler change is narrowly scoped to the Windows grouped CollectionView path.
Overall Assessment
The metadata needs a small cleanup, but the larger concern is the Windows grouped-template regression risk. I would not consider this PR ready to merge until the grouped no-ItemTemplate fix preserves GroupTemplateContext behavior for GroupHeaderTemplate / GroupFooterTemplate.
kubaflo
left a comment
There was a problem hiding this comment.
Looks like the test is failing
#34575) <!-- Please let the below note in for people that find this PR --> > [!NOTE] > Are you waiting for the changes in this PR to be merged? > It would be very helpful if you could [test the resulting artifacts](https://github.com/dotnet/maui/wiki/Testing-PR-Builds) from this PR and let us know in a comment if this change resolves your issue. Thank you! ## Description Adds Windows platform support to the `maui-copilot` CI pipeline (AzDO definition 27723), enabling Copilot PR reviews on Windows-targeted PRs. ### Changes **`eng/pipelines/ci-copilot.yml`** - Add `catalyst` and `windows` to Platform parameter values - Add per-platform pool selection (`androidPool`, `iosPool`, `macPool`, `windowsPool`) - Skip Xcode, Android SDK, simulator setup for Windows/Catalyst - Add Windows-specific "Set screen resolution" step (1920x1080) - Add MacCatalyst-specific "Disable Notification Center" step - Fix `sed` command for `Directory.Build.Override.props` on Windows (Git Bash uses GNU sed) - Handle Copilot CLI PATH detection on Windows vs Unix - Change `script:` steps to `bash:` for cross-platform consistency **`.github/scripts/Review-PR.ps1`** - Add `catalyst` to ValidateSet for Platform parameter **`.github/scripts/BuildAndRunHostApp.ps1`** - Add Windows test assembly directory for artifact collection **`.github/scripts/post-ai-summary-comment.ps1` / `post-pr-finalize-comment.ps1`** - Various improvements for cross-platform comment posting ### Validation Successfully ran the pipeline with `Platform=windows` on multiple Windows-specific PRs: - PR #27713 — ✅ Succeeded - PR #34337 — ✅ Succeeded - PR #26217, #27609, #27880, #28617, #29927, #30068 — Triggered and running --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…plates' page loading exception ).
…plates' page loading exception ).
…plates' page loading exception ).
…plates' page loading exception ).
…plates' page loading exception ).
…plates' page loading exception ).
…te results in displaying the default string representation of the object)
…te results in displaying the default string representation of the object)
…te results in displaying the default string representation of the object)
…te results in displaying the default string representation of the object)
…te results in displaying the default string representation of the object)
…te results in displaying the default string representation of the object)
2b56ab2 to
b3cef0c
Compare
|
🚀 Dogfood this PR with:
curl -fsSL https://raw.githubusercontent.com/dotnet/maui/main/eng/scripts/get-maui-pr.sh | bash -s -- 28617Or
iex "& { $(irm https://raw.githubusercontent.com/dotnet/maui/main/eng/scripts/get-maui-pr.ps1) } 28617" |
…napshots for iOS and Android
@kubaflo, I’ve updated the fix and modified the test case based on the suggestions provided by Mauibot. |
Issue Details
Root Cause
Description of Change
Validated the behaviour in the following platforms
Issues Fixed
Fixes #23293
Output
Before.mp4
After.mp4