ScrollView's ScrollToAsync doesn't complete when called thrice with the same value#27300
ScrollView's ScrollToAsync doesn't complete when called thrice with the same value#27300KarthikRajaKalaimani wants to merge 3 commits intodotnet:mainfrom
Conversation
|
@dotnet-policy-service agree company="Syncfusion, Inc." |
|
/azp run |
|
Azure Pipelines successfully started running 3 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 3 pipeline(s). |
|
Hi guys, Any update on this PR? Thank you. |
|
/rebase |
306ee52 to
60d08d1
Compare
|
/azp run |
|
Azure Pipelines successfully started running 3 pipeline(s). |
60d08d1 to
11a48e6
Compare
11a48e6 to
44d1865
Compare
|
/rebase |
…with the same value Fixed ScrollView's ScrollToAsync doesn't complete when called thrice with the same value.
ScrollView's ScrollToAsync doesn't complete when called thrice with the same value
Test case enabled for this issue
44d1865 to
ce1d567
Compare
🤖 AI Summary📊 Expand Full Review🔍 Pre-Flight — Context & Validation📝 Review Session — Test case enabled for this issue ·
|
| Phase | Status | Details |
|---|---|---|
| Revert Fix | ✅ | 2 fix files reverted to merge-base state |
| Test WITHOUT Fix | ✅ FAILED | Tests fail as expected - TimeoutException waiting for "completed" element |
| Restore Fix | ✅ | 2 fix files restored from HEAD |
| Test WITH Fix | ✅ PASSED | Tests pass with the fix applied (17 seconds) |
Result: VERIFICATION PASSED ✅
🔧 Fix — Analysis & Comparison
📝 Review Session — Test case enabled for this issue · ce1d567
Status: ✅ COMPLETE
| # | Source | Approach | Test Result | Files Changed | Notes |
|---|---|---|---|---|---|
| PR | PR #27300 | Post-dispatch check - call ScrollFinished() after native scroll if at target | ✅ PASS (Gate) | 2 files (handlers) | Original PR |
| 1 | try-fix | Pre-flight check in handlers BEFORE native scroll | ✅ PASS | 2 files (handlers) | Check before ChangeView/SetContentOffset |
| 2 | try-fix | Shared control layer pre-check in ScrollView.cs | ✅ PASS | 1 file (shared) | SIMPLEST: 7 lines, single file |
| 3 | try-fix | Use ChangeView() return value (Windows) | ✅ PASS | 1 file (Windows only) | Windows-specific only |
| 4 | try-fix | Delayed fallback dispatch with tolerance | ❌ FAIL | 1 file (shared) | Timeout - too late to complete |
| 5 | try-fix | Post-dispatch immediate position check | ✅ PASS | 2 files (handlers) | Same as PR's approach |
| 6 | try-fix | Coalesce duplicate requests with caching | ✅ PASS | 1 file (shared) | Complex - 33+ lines added |
Cross-Pollination Results (Round 2):
| Model | Response |
|---|---|
| claude-sonnet-4.5 | NO NEW IDEAS |
| claude-opus-4.5 | NO NEW IDEAS |
| gpt-5.2 | NEW IDEA: Jiggle scroll (tested - not worth pursuing) |
| gpt-5.2-codex | NEW IDEA: Coalesce requests (tested as #6 - ✅ PASS) |
| gemini-3-pro-preview | NEW IDEA: Timeout wrapper (similar to failed #4) |
Exhausted: Yes - 6 approaches tested, 5 passing, remaining ideas are less clean than existing solutions
Selected Fix: PR's fix (same as #5)
Rationale for Selection:
While Attempt #2 (shared layer pre-check) is the simplest (7 lines, single file), the PR's approach is:
- Already implemented and tested across all affected platforms
- Follows the existing code pattern (post-dispatch check)
- More defensive - checks actual platform state after native call
- Has been reviewed and rebased multiple times
The PR's fix is correct, minimal, and follows established patterns. No changes requested.
📋 Report — Final Recommendation
📝 Review Session — Test case enabled for this issue · ce1d567
Status: ✅ COMPLETE
Root Cause
When ScrollToAsync is called with coordinates that match the current scroll position, the native platform controls (ScrollViewer.ChangeView on Windows, UIScrollView.SetContentOffset on iOS) treat this as a no-op and don't fire their scroll completion events. Since MAUI's ScrollToAsync relies on these events to signal completion via ScrollFinished(), the returned Task never completes.
Fix Analysis
The PR correctly addresses this by checking if the scroll position matches the target position and explicitly calling ScrollFinished() when no scroll will occur:
- Windows: After calling
ChangeView(), checks if offsets match and callsScrollFinished() - iOS: Extends the existing
request.Instantcondition to also check if already at target position
Alternative Approaches Tested
| Approach | Result | Comparison to PR |
|---|---|---|
| Pre-flight check in handlers | ✅ PASS | Similar complexity, less defensive |
| Shared layer check in ScrollView.cs | ✅ PASS | Simpler (7 lines, 1 file) but different pattern |
| ChangeView() return value | ✅ PASS | Windows-only, not cross-platform |
| Request coalescing | ✅ PASS | More complex (33+ lines) |
Recommendation
✅ APPROVE - The PR's fix is correct, minimal, and follows established patterns in the codebase. While a simpler alternative exists (shared layer check), the PR's approach is more defensive by checking actual platform state after the native call.
Code Quality
- ✅ Minimal changes (5 lines total across 2 files)
- ✅ Test coverage uses existing
ScrollToYTwicetest - ✅ Follows existing code patterns in handlers
- ✅ No breaking API changes
…s extraction (#33813) <!-- 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! ## Summary This PR significantly enhances the PR agent workflow with improvements across **two development phases**: ### Phase 1: Original Enhancements (Commits 1-8) 1. **Multi-model try-fix exploration** - Uses 5 different AI models to explore alternative solutions 2. **Environment blocker handling** - Strict rules to stop and ask user when environment issues occur 3. **Review automation script** - PowerShell script that invokes Copilot CLI directly for PR reviews 4. **Branch safety rules** - Prevents agent from switching branches during reviews 5. **Template formatting rules** - Exact template adherence for downstream script compatibility ### Phase 2: Consolidation & Simplification (Commit 9) After multi-model review (5 models: Claude Sonnet 4.5, Claude Opus 4.5, GPT-5.2, GPT-5.2-Codex, Gemini 3 Pro), the following improvements were made: 6. **Shared rules extraction** - Created `SHARED-RULES.md` to eliminate duplication across files 7. **Simplified git policy** - Agent never runs git commands; always assumes correct branch 8. **State file handling** - Changed "commit" to "save" (state files are gitignored) 9. **Reduced verbosity** - Compressed cross-pollination section, converted PLAN-TEMPLATE to checklist --- ## Commits ### 1. `80d7e412c2` - Update pr agent with multi-model try-fix workflow **Why:** The original PR agent only used a single model for exploring fixes. Different AI models have different strengths and may find solutions others miss. **Changes:** - Added Phase 4 multi-model workflow using 5 models: - `claude-sonnet-4.5`, `claude-opus-4.5`, `gpt-5.2`, `gpt-5.2-codex`, `gemini-3-pro-preview` - Cross-pollination loop: Share results between models to spark new ideas - Continue until all models confirm "no more approaches to explore" --- ### 2. `69cc6af403` - Address Copilot review suggestions **Why:** Initial PR review feedback suggested improvements. **Changes:** - Minor formatting and clarity improvements to pr.md --- ### 3. `fe55c3fd21` - Add rules for template formatting and skill script usage **Why:** Downstream scripts depend on exact regex patterns in state files. Agents were "improving" templates by adding attributes like `open` which broke parsing. **Changes:** - Added "Follow Templates EXACTLY" rule - no adding attributes, no improving formats - Added "Use Skills' Scripts" rule - run provided PowerShell scripts, don't bypass with manual commands --- ### 4. `debbee608e` - Add 'Stop on Environment Blockers' rule to PR agent **Why:** Agent was continuing through phases when environment issues (missing Appium, WinAppDriver errors) prevented completion, leading to incomplete reviews. **Changes:** - Added explicit blocker handling section to pr.md - Common blockers: Appium drivers, WinAppDriver, Xcode, emulators, port conflicts - Must STOP, report the blocker, and ask user how to proceed - Never mark phase as blocked and continue to next phase --- ### 5. `ad29f6a796` - Add PR review plan template and Review-PR.ps1 script **Why:** Need a reusable template for consistent PR reviews and a script to automate invocation. **Changes:** - Created `.github/agents/pr/PLAN-TEMPLATE.md` - Reusable 5-phase review plan - Created `.github/scripts/Review-PR.ps1` - Script to prepare environment and invoke Copilot CLI --- ### 6. `886ea2aa8e` - Improve blocker handling and fix Review-PR.ps1 for Copilot CLI **Why:** During PR #27300 review, agent spent 10+ tool calls troubleshooting WinAppDriver instead of stopping after first failure. **Changes:** - Added strict retry limits table: | Blocker Type | Max Retries | Action | |--------------|-------------|--------| | Server errors (500, timeout) | 0 | STOP immediately | | Missing tools | 1 install | STOP and ask | | Port conflicts | 1 kill | STOP and ask | | WinAppDriver errors | 0 | STOP immediately | - Added "What I tried" section to blocker report template - New prohibitions: Never spend more than 2-3 tool calls on same blocker --- ### 7. `d67da75e85` - Update Review-PR.ps1 to invoke Copilot CLI directly **Why:** Initially thought Copilot CLI was interactive-only. Discovered it supports `-i <prompt>` and `-p <prompt>` for programmatic invocation. **Changes:** - Script now invokes `copilot --agent pr -i "<prompt>"` directly - Validates both `gh` CLI and `copilot` CLI are installed - New `-NoInteractive` switch for `-p` mode (exits after completion) - Dry run mode shows exactly what would be invoked --- ### 8. `ed74c574a5` - Add 'Do NOT Switch Branches' rule to pr agent **Why:** During PR review testing, the pr agent ran `git checkout`, `git stash`, and other branch-switching commands, causing loss of local changes and confusion about which code was being reviewed. **Changes:** - Added explicit "Do NOT Switch Branches" rule to both pr.md and PLAN-TEMPLATE.md - Forbidden commands: `git checkout`, `git switch`, `gh pr checkout`, `git stash` - Agent must work on current branch as-is, using `git diff` or `gh pr diff` to see PR changes - Fixed variable expansion in Review-PR.ps1 prompt (double backticks for here-strings) --- ### 9. `632bfb7155` - Extract shared rules, simplify git policy, reduce duplication **Why:** Multi-model review (5 AI models) identified significant duplication (~200 lines) across files, conflicting "commit" terminology, and overly verbose sections. The git checkout prohibition also conflicted with workflow steps that mentioned git checkout. **Changes:** - **Created `SHARED-RULES.md`** (167 lines) - Single source of truth for: - Phase Completion Protocol - Follow Templates EXACTLY - No Direct Git Commands (absolute - agent never runs git) - Use Skills' Scripts - Stop on Environment Blockers (with retry limits) - Multi-Model Configuration (5 models list) - Platform Selection guidance - **Simplified git policy** - Agent is ALWAYS on correct branch, never runs git commands: - Removed git fetch/checkout from Phase 1 Pre-Flight - Phase 5: User handles commit/push/PR creation - Changed all "State file committed" → "State file saved" (gitignored files) - **Compressed content**: - Cross-pollination ASCII box: 51 → 20 lines - PLAN-TEMPLATE.md: Full docs → Pure checklist (226 → 112 lines) - pr.md: 662 → 535 lines (-19%) - post-gate.md: 403 → 302 lines (-25%) - Total reduction: 1291 → 1116 lines (-14%) - **Eliminated duplication**: - Blocker handling was in 3 files → now in SHARED-RULES.md only - Phase Completion Protocol was in 2 files → now in SHARED-RULES.md only - Model list was in 3 files → now in SHARED-RULES.md only --- ## Files Changed | File | Purpose | |------|---------| | `.github/agents/pr.md` | Main PR agent instructions (Phases 1-3) | | `.github/agents/pr/post-gate.md` | Phase 4-5 instructions (multi-model try-fix) | | `.github/agents/pr/PLAN-TEMPLATE.md` | **NEW** - Reusable 5-phase review checklist | | `.github/agents/pr/SHARED-RULES.md` | **NEW** - Extracted shared rules (single source of truth) | | `.github/scripts/Review-PR.ps1` | **NEW** - Script to invoke Copilot CLI for PR review | --- ## Usage ```powershell # Interactive mode (default) - stays open for follow-up pwsh .github/scripts/Review-PR.ps1 -PRNumber 33687 # Non-interactive mode - exits when done pwsh .github/scripts/Review-PR.ps1 -PRNumber 33687 -NoInteractive # Specific platform pwsh .github/scripts/Review-PR.ps1 -PRNumber 33687 -Platform ios # Skip merge if already on branch pwsh .github/scripts/Review-PR.ps1 -PRNumber 33687 -SkipMerge # Dry run to preview pwsh .github/scripts/Review-PR.ps1 -PRNumber 33687 -DryRun ``` --- ## Multi-Model Validation The final changes (commit 9) were validated by 5 AI models: | Model | Verdict | Key Feedback | |-------|---------|--------------| | Claude Sonnet 4.5 | ✅ READY TO MERGE | "All git command instructions successfully removed" | | Claude Opus 4.5 | ✅ READY TO MERGE | "Excellent refactoring, no conflicting guidance" | | GPT-5.2 | ✅ READY TO MERGE | "Progressive disclosure maintained" | | GPT-5.2-Codex | ✅ READY TO MERGE | "No instructions to run git commands remain" | | Gemini 3 Pro | ✅ READY TO MERGE | "Agent is instructed to STOP and ask user for commits" | --- ## Testing Tested by reviewing PR #27300 (ScrollView ScrollToAsync fix): - Pre-Flight phase completed successfully - Tests phase verified test files exist - Gate phase encountered WinAppDriver blocker → agent correctly stopped and asked - Blocker handling rules validated through real-world usage - **Branch safety verified**: Agent stayed on branch instead of switching --------- Co-authored-by: Jakub Florkowski <kubaflo123@gmail.com>
…s extraction (#33813) <!-- 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! ## Summary This PR significantly enhances the PR agent workflow with improvements across **two development phases**: ### Phase 1: Original Enhancements (Commits 1-8) 1. **Multi-model try-fix exploration** - Uses 5 different AI models to explore alternative solutions 2. **Environment blocker handling** - Strict rules to stop and ask user when environment issues occur 3. **Review automation script** - PowerShell script that invokes Copilot CLI directly for PR reviews 4. **Branch safety rules** - Prevents agent from switching branches during reviews 5. **Template formatting rules** - Exact template adherence for downstream script compatibility ### Phase 2: Consolidation & Simplification (Commit 9) After multi-model review (5 models: Claude Sonnet 4.5, Claude Opus 4.5, GPT-5.2, GPT-5.2-Codex, Gemini 3 Pro), the following improvements were made: 6. **Shared rules extraction** - Created `SHARED-RULES.md` to eliminate duplication across files 7. **Simplified git policy** - Agent never runs git commands; always assumes correct branch 8. **State file handling** - Changed "commit" to "save" (state files are gitignored) 9. **Reduced verbosity** - Compressed cross-pollination section, converted PLAN-TEMPLATE to checklist --- ## Commits ### 1. `80d7e412c2` - Update pr agent with multi-model try-fix workflow **Why:** The original PR agent only used a single model for exploring fixes. Different AI models have different strengths and may find solutions others miss. **Changes:** - Added Phase 4 multi-model workflow using 5 models: - `claude-sonnet-4.5`, `claude-opus-4.5`, `gpt-5.2`, `gpt-5.2-codex`, `gemini-3-pro-preview` - Cross-pollination loop: Share results between models to spark new ideas - Continue until all models confirm "no more approaches to explore" --- ### 2. `69cc6af403` - Address Copilot review suggestions **Why:** Initial PR review feedback suggested improvements. **Changes:** - Minor formatting and clarity improvements to pr.md --- ### 3. `fe55c3fd21` - Add rules for template formatting and skill script usage **Why:** Downstream scripts depend on exact regex patterns in state files. Agents were "improving" templates by adding attributes like `open` which broke parsing. **Changes:** - Added "Follow Templates EXACTLY" rule - no adding attributes, no improving formats - Added "Use Skills' Scripts" rule - run provided PowerShell scripts, don't bypass with manual commands --- ### 4. `debbee608e` - Add 'Stop on Environment Blockers' rule to PR agent **Why:** Agent was continuing through phases when environment issues (missing Appium, WinAppDriver errors) prevented completion, leading to incomplete reviews. **Changes:** - Added explicit blocker handling section to pr.md - Common blockers: Appium drivers, WinAppDriver, Xcode, emulators, port conflicts - Must STOP, report the blocker, and ask user how to proceed - Never mark phase as blocked and continue to next phase --- ### 5. `ad29f6a796` - Add PR review plan template and Review-PR.ps1 script **Why:** Need a reusable template for consistent PR reviews and a script to automate invocation. **Changes:** - Created `.github/agents/pr/PLAN-TEMPLATE.md` - Reusable 5-phase review plan - Created `.github/scripts/Review-PR.ps1` - Script to prepare environment and invoke Copilot CLI --- ### 6. `886ea2aa8e` - Improve blocker handling and fix Review-PR.ps1 for Copilot CLI **Why:** During PR #27300 review, agent spent 10+ tool calls troubleshooting WinAppDriver instead of stopping after first failure. **Changes:** - Added strict retry limits table: | Blocker Type | Max Retries | Action | |--------------|-------------|--------| | Server errors (500, timeout) | 0 | STOP immediately | | Missing tools | 1 install | STOP and ask | | Port conflicts | 1 kill | STOP and ask | | WinAppDriver errors | 0 | STOP immediately | - Added "What I tried" section to blocker report template - New prohibitions: Never spend more than 2-3 tool calls on same blocker --- ### 7. `d67da75e85` - Update Review-PR.ps1 to invoke Copilot CLI directly **Why:** Initially thought Copilot CLI was interactive-only. Discovered it supports `-i <prompt>` and `-p <prompt>` for programmatic invocation. **Changes:** - Script now invokes `copilot --agent pr -i "<prompt>"` directly - Validates both `gh` CLI and `copilot` CLI are installed - New `-NoInteractive` switch for `-p` mode (exits after completion) - Dry run mode shows exactly what would be invoked --- ### 8. `ed74c574a5` - Add 'Do NOT Switch Branches' rule to pr agent **Why:** During PR review testing, the pr agent ran `git checkout`, `git stash`, and other branch-switching commands, causing loss of local changes and confusion about which code was being reviewed. **Changes:** - Added explicit "Do NOT Switch Branches" rule to both pr.md and PLAN-TEMPLATE.md - Forbidden commands: `git checkout`, `git switch`, `gh pr checkout`, `git stash` - Agent must work on current branch as-is, using `git diff` or `gh pr diff` to see PR changes - Fixed variable expansion in Review-PR.ps1 prompt (double backticks for here-strings) --- ### 9. `632bfb7155` - Extract shared rules, simplify git policy, reduce duplication **Why:** Multi-model review (5 AI models) identified significant duplication (~200 lines) across files, conflicting "commit" terminology, and overly verbose sections. The git checkout prohibition also conflicted with workflow steps that mentioned git checkout. **Changes:** - **Created `SHARED-RULES.md`** (167 lines) - Single source of truth for: - Phase Completion Protocol - Follow Templates EXACTLY - No Direct Git Commands (absolute - agent never runs git) - Use Skills' Scripts - Stop on Environment Blockers (with retry limits) - Multi-Model Configuration (5 models list) - Platform Selection guidance - **Simplified git policy** - Agent is ALWAYS on correct branch, never runs git commands: - Removed git fetch/checkout from Phase 1 Pre-Flight - Phase 5: User handles commit/push/PR creation - Changed all "State file committed" → "State file saved" (gitignored files) - **Compressed content**: - Cross-pollination ASCII box: 51 → 20 lines - PLAN-TEMPLATE.md: Full docs → Pure checklist (226 → 112 lines) - pr.md: 662 → 535 lines (-19%) - post-gate.md: 403 → 302 lines (-25%) - Total reduction: 1291 → 1116 lines (-14%) - **Eliminated duplication**: - Blocker handling was in 3 files → now in SHARED-RULES.md only - Phase Completion Protocol was in 2 files → now in SHARED-RULES.md only - Model list was in 3 files → now in SHARED-RULES.md only --- ## Files Changed | File | Purpose | |------|---------| | `.github/agents/pr.md` | Main PR agent instructions (Phases 1-3) | | `.github/agents/pr/post-gate.md` | Phase 4-5 instructions (multi-model try-fix) | | `.github/agents/pr/PLAN-TEMPLATE.md` | **NEW** - Reusable 5-phase review checklist | | `.github/agents/pr/SHARED-RULES.md` | **NEW** - Extracted shared rules (single source of truth) | | `.github/scripts/Review-PR.ps1` | **NEW** - Script to invoke Copilot CLI for PR review | --- ## Usage ```powershell # Interactive mode (default) - stays open for follow-up pwsh .github/scripts/Review-PR.ps1 -PRNumber 33687 # Non-interactive mode - exits when done pwsh .github/scripts/Review-PR.ps1 -PRNumber 33687 -NoInteractive # Specific platform pwsh .github/scripts/Review-PR.ps1 -PRNumber 33687 -Platform ios # Skip merge if already on branch pwsh .github/scripts/Review-PR.ps1 -PRNumber 33687 -SkipMerge # Dry run to preview pwsh .github/scripts/Review-PR.ps1 -PRNumber 33687 -DryRun ``` --- ## Multi-Model Validation The final changes (commit 9) were validated by 5 AI models: | Model | Verdict | Key Feedback | |-------|---------|--------------| | Claude Sonnet 4.5 | ✅ READY TO MERGE | "All git command instructions successfully removed" | | Claude Opus 4.5 | ✅ READY TO MERGE | "Excellent refactoring, no conflicting guidance" | | GPT-5.2 | ✅ READY TO MERGE | "Progressive disclosure maintained" | | GPT-5.2-Codex | ✅ READY TO MERGE | "No instructions to run git commands remain" | | Gemini 3 Pro | ✅ READY TO MERGE | "Agent is instructed to STOP and ask user for commits" | --- ## Testing Tested by reviewing PR #27300 (ScrollView ScrollToAsync fix): - Pre-Flight phase completed successfully - Tests phase verified test files exist - Gate phase encountered WinAppDriver blocker → agent correctly stopped and asked - Blocker handling rules validated through real-world usage - **Branch safety verified**: Agent stayed on branch instead of switching --------- Co-authored-by: Jakub Florkowski <kubaflo123@gmail.com>
Issue Details:
When calling ScrollToAsync thrice with the same Y value on a ScrollView, the scrolling operation does not complete on iOS, Windows, and Catalyst platforms. This behavior is inconsistent with Android, where the scrolling operation completes as expected.
Root Cause:
The ScrollToAsync method does not mark the task as completed because the scroll did not occur due to the target position already being the current position. As a result, the ScrollToAsync method does not return control to the calling method.
Description of Change:
To resolve the issue, i called the ScrollFinished method if the target scroll position and current scroll position is same because the ScrollFinished method will mark the task as completed.
Tested the behavior in the following platforms.
Reference:
N/A
Issues Fixed:
Fixes #27250
Screenshots
Before_27250.mov
After_27250.mov