Avoid duplicate startup refresh in .NET MAUI mobile template - #36687
Conversation
Remove the refresh from InitData because Appearing refreshes immediately after initialization. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 888e9ad7-deb0-4ead-a335-3e597afaf597
|
🚀 Dogfood this PR with:
curl -fsSL https://raw.githubusercontent.com/dotnet/maui/main/eng/scripts/get-maui-pr.sh | bash -s -- 36687Or
iex "& { $(irm https://raw.githubusercontent.com/dotnet/maui/main/eng/scripts/get-maui-pr.ps1) } 36687" |
|
Azure Pipelines: Successfully started running 1 pipeline(s). There may be pipelines that require an authorized user to comment /azp run to run. |
There was a problem hiding this comment.
Pull request overview
This PR optimizes the .NET MAUI maui-mobile template (sample-content) startup path by removing a redundant data refresh during initial seeding, so the initial load occurs only once.
Changes:
- Removed the
await Refresh();call fromMainPageModel.InitData()to prevent a duplicate refresh at startup. - Kept the existing refresh behavior in
Appearing()as the single place responsible for the initial load.
AI Review Summary
🗂️ Review Sessions — click to expand🚦 Gate — Test Before & After FixGate Result:
|
| # | Source | Approach | Test Result | Files Changed | Notes |
|---|---|---|---|---|---|
| PR | PR #36687 | Remove await Refresh(); from InitData() so first Appearing() refreshes once |
src/Templates/src/templates/maui-mobile/PageModels/MainPageModel.cs |
Original PR; simplest candidate |
🔬 Code Review — Deep Analysis
Code Review — PR #36687
Independent Assessment
What this changes: Removes one duplicate initial Refresh() call from MainPageModel.InitData(). First page appearance still seeds, marks _dataLoaded, then refreshes once.
Inferred motivation: Avoid loading and measuring the mobile sample-content startup UI twice.
Reconciliation with PR Narrative
Author claims: Startup data was loaded twice; this keeps Appearing() as the single initial refresh point.
Agreement/disagreement: Agrees with code. The remaining Appearing() refresh preserves initial and subsequent navigation refresh behavior.
Prior Review Reconciliation
No prior ❌ Error findings found. Checked top-level reviews, inline review comments, and issue comments via GitHub REST.
Blast Radius Assessment
- Runs for all instances: yes, but only the sample-content mobile template
MainPageModel. - Startup impact: yes; reduces first-load work.
- Static/shared state: no new static/shared state.
CI Status
- Required-check result: undetermined —
gh pr checks --requiredfailed because GitHub CLI is unauthenticated. - Classification: supplemental REST check-runs for head SHA show
maui-prand visible checks succeeded; required-check set still not verifiable throughgh. - Action taken: confidence capped low; no
LGTMper skill rules.
External Output Contract
| Consumer token/pattern | Producer location | Producer emission condition | Consumer assumption | Ordinary negative case | Downstream effect |
|---|---|---|---|---|---|
| N/A | N/A | No regex/string external-output classifier changed | N/A | N/A | N/A |
Findings
No code findings.
Failure-Mode Probing
- First launch after no seed preference:
InitData()seeds, sets preference, thenAppearing()refreshes once. - Already seeded launch:
InitData()completes without seeding;Appearing()still refreshes. - Navigation away/back: existing
_isNavigatedTopath still refreshes when returning. - Null/default UI state: unchanged; lists still default to empty collections.
Verdict: NEEDS_DISCUSSION
Confidence: low
Summary: The code change itself looks sound and removes redundant startup work. However, required CI status could not be verified with gh pr checks --required, so the skill rules prohibit LGTM.
🛠️ Fix — Analysis & Comparison
Fix Candidates
| # | Source | Approach | Test Result | Files Changed | Notes |
|---|---|---|---|---|---|
| 1 | try-fix | Move initial refresh ownership to InitData(); remove duplicate first-appearance refresh |
✅ PASS (dotnet build src/Templates/src/Microsoft.Maui.Templates.csproj) |
1 file | Builds, but not better than PR because InitData() keeps a hidden UI-refresh side effect |
| 2 | try-fix | Separate initial load from pull-to-refresh with LoadDataSafe() |
✅ PASS (dotnet build src/Templates/src/Microsoft.Maui.Templates.csproj) |
1 file | Builds and is semantically distinct, but adds code and is only better if startup must avoid IsRefreshing semantics |
| PR | PR #36687 | Remove await Refresh(); from InitData() so first Appearing() refreshes once |
1 file | Simplest and clearest fix; no tests detected |
Cross-Pollination
| Model | Round | New Ideas? | Details |
|---|---|---|---|
| maui-expert-reviewer / gpt-5.5 | 1 | Yes | Candidate 1: move initial refresh ownership to InitData() |
| maui-expert-reviewer / gpt-5.5 | 2 | Yes | Candidate 2: separate startup load from refresh semantics via LoadDataSafe() |
| maui-expert-reviewer / gpt-5.5 | 3 | No | Remaining viable fixes reduce to choosing one of the two startup call sites or adding trivial guards/flags |
| claude-opus-4.6 | Cross-pollination | No | Remaining approaches are trivial guards, flags, or reordering around the same two call sites |
| claude-opus-4.7 | Cross-pollination | No | Remaining variants reduce to the same two-call-site restructuring already covered |
| gpt-5.3-codex | Cross-pollination | No | Remaining options are variants of deduping/guarding the same refresh paths |
| gpt-5.5 | Cross-pollination | No | All meaningful fixes reduce to choosing which startup call site owns refresh |
Exhausted: Yes
Selected Fix: PR #36687 — The PR is the simplest and clearest passing approach. Candidate 2 is a viable alternative if the template intentionally wants startup load to avoid IsRefreshing, but absent that requirement it is extra complexity. Candidate 1 is equivalent but worse because InitData() continues to refresh UI data.
Candidate Details
Candidate 1 — Move Initial Refresh Ownership to InitData()
See ../try-fix-1/content.md and attempt-1/ artifacts.
Candidate 2 — Separate Initial Load from Pull-to-Refresh
See ../try-fix-2/content.md and attempt-2/ artifacts.
🏁 Report — Final Recommendation
Comparative Fix Report — PR #36687
Candidates Compared
| Rank | Candidate | Result | Assessment |
|---|---|---|---|
| 1 | pr |
Best candidate. Removes the duplicate startup refresh at the source by making InitData() seed-only and leaving Appearing() responsible for the one initial refresh. Smallest correct change. |
|
| 2 | pr-plus-reviewer |
Functionally identical to pr because the expert reviewer found no actionable changes. Ranked behind pr only because it is not materially distinct. |
|
| 3 | try-fix-2 |
✅ Build passed | Viable, but adds a new LoadDataSafe() abstraction and changes first startup to avoid IsRefreshing. That may be useful if startup must not use refresh semantics, but no such requirement is established. |
| 4 | try-fix-1 |
✅ Build passed; behavioral concern | Not selected. Its prose says it moves initial refresh ownership to InitData(), but the stored diff adds a second await Refresh(); inside InitData() while removing the Appearing() refresh, so the candidate still performs two startup refreshes and does not satisfy the regression intent. |
Detailed Comparison
pr
The submitted PR removes await Refresh(); from InitData() and leaves the first Appearing() branch to call Refresh() once after seeding. This directly fixes the observed duplicate startup refresh while preserving later appearance refreshes and pull-to-refresh behavior. It also improves separation of concerns: InitData() seeds data and updates the seed preference; Refresh() remains the data-loading UI refresh path.
pr-plus-reviewer
The expert reviewer produced no inline findings and recommended no changes. Therefore this candidate is identical to pr; it has the same behavior, risk profile, and validation gap.
try-fix-2
This candidate removes the refresh from InitData(), adds LoadDataSafe(), calls that helper on first appearance, and makes Refresh() wrap LoadDataSafe() while toggling IsRefreshing. It is build-passing and semantically coherent, but it is a broader behavior split than needed. It changes startup from a refresh-shaped load to a non-refresh load without evidence that the IsRefreshing distinction is required for the template.
try-fix-1
This candidate is not acceptable as stored. Although the attempt analysis says the candidate preserves exactly one refresh by leaving Refresh() in InitData() and removing the Appearing() refresh, the actual stored diff shows two consecutive await Refresh(); calls in InitData(). That keeps the duplicate startup refresh behavior and must rank below candidates that actually remove the duplicate.
Winner
Winning candidate: pr
The PR fix is the smallest correct candidate, the expert reviewer found no actionable issues, and the alternatives are either equivalent with no added value (pr-plus-reviewer), unnecessarily broader (try-fix-2), or behaviorally incorrect as stored (try-fix-1).
🧭 Next Steps — review latest findings
No alternative fix was selected for this run. Review the session findings and CI results before merging.
<!-- 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 of Change `MainPageModel.InitData()` called `Refresh()`, and `Appearing()` called it again immediately after initialization. This caused the .NET MAUI mobile sample-content template to load its startup data twice. This change removes the `Refresh()` call from `InitData()`. `Appearing()` remains responsible for the initial refresh, so startup loads data once while later appearances continue to refresh as before. Template layout and shimmer markup are unchanged. #### Profiling results | Metric | Baseline | Single refresh | Change | | --- | ---: | ---: | ---: | | `LoadData` occurrences | 2 | 1 | -50% | | `ComputeDesiredSize` | 1.54s | 1.17s | -24% | | Grid first measure | 596ms | 442ms | -26% | | Syncfusion measurement | 339ms | 248ms | -27% | | Pull-to-refresh measurement | 266ms | 211ms | -21% | | Item binding | 194ms | 160ms | -17% | | Trace events | Baseline | Single refresh | -19% | #### Testing - Built `src/Templates/src/Microsoft.Maui.Templates.csproj` successfully with no warnings or errors. - Existing `--sample-content` integration cases cover generated mobile-template compilation; no behavioral page-model test harness is available. ### Issues Fixed N/A Copilot-Session: 888e9ad7-deb0-4ead-a335-3e597afaf597
<!-- 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 of Change `MainPageModel.InitData()` called `Refresh()`, and `Appearing()` called it again immediately after initialization. This caused the .NET MAUI mobile sample-content template to load its startup data twice. This change removes the `Refresh()` call from `InitData()`. `Appearing()` remains responsible for the initial refresh, so startup loads data once while later appearances continue to refresh as before. Template layout and shimmer markup are unchanged. #### Profiling results | Metric | Baseline | Single refresh | Change | | --- | ---: | ---: | ---: | | `LoadData` occurrences | 2 | 1 | -50% | | `ComputeDesiredSize` | 1.54s | 1.17s | -24% | | Grid first measure | 596ms | 442ms | -26% | | Syncfusion measurement | 339ms | 248ms | -27% | | Pull-to-refresh measurement | 266ms | 211ms | -21% | | Item binding | 194ms | 160ms | -17% | | Trace events | Baseline | Single refresh | -19% | #### Testing - Built `src/Templates/src/Microsoft.Maui.Templates.csproj` successfully with no warnings or errors. - Existing `--sample-content` integration cases cover generated mobile-template compilation; no behavioral page-model test harness is available. ### Issues Fixed N/A Copilot-Session: 888e9ad7-deb0-4ead-a335-3e597afaf597
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 from this PR and let us know in a comment if this change resolves your issue. Thank you!
Description of Change
MainPageModel.InitData()calledRefresh(), andAppearing()called it again immediately after initialization. This caused the .NET MAUI mobile sample-content template to load its startup data twice.This change removes the
Refresh()call fromInitData().Appearing()remains responsible for the initial refresh, so startup loads data once while later appearances continue to refresh as before.Template layout and shimmer markup are unchanged.
Profiling results
LoadDataoccurrencesComputeDesiredSizeTesting
src/Templates/src/Microsoft.Maui.Templates.csprojsuccessfully with no warnings or errors.--sample-contentintegration cases cover generated mobile-template compilation; no behavioral page-model test harness is available.Issues Fixed
N/A