fix: Ensure monotonic progression of chat overlay loading stage#69
Open
google-labs-jules[bot] wants to merge 4 commits into
Open
fix: Ensure monotonic progression of chat overlay loading stage#69google-labs-jules[bot] wants to merge 4 commits into
google-labs-jules[bot] wants to merge 4 commits into
Conversation
The ChatConversationSurfaceStatePresenter previously computed the OverlayLoadingStage completely statelessly. When intermediate transitions occurred (e.g. `isConnecting` falling to false before the overlay was dismissed), the stage could inadvertently regress from `Connecting` (stage 1) to `PreparingSession` (stage 4, the initial state). This violated the lifecycle monotonicity rule, resulting in the status pill text flickering backwards. This change introduces stateful tracking of the maximum attained `LoadingOverlayStage` rank within `ChatConversationSurfaceProjectionCoordinator` over the lifespan of a continuously visible overlay. The fix is safe architecturally because: 1. It is fully contained within the presentation's projection layer and does not mutate any underlying core state mechanisms. 2. It correctly resets its tracking the moment the activation overlay is fully dismissed, ensuring that completely new activations start clean. 3. The dependent `ShouldShowLoadingOverlayStatusPill` and `ShouldShowLoadingOverlayPresenter` boolean values are appropriately recomputed based on the monotonic stage to preserve Uno/WinUI view compatibility. Verification commands run: - dotnet build SalmonEgg.sln --configuration Debug - dotnet test tests/SalmonEgg.Presentation.Core.Tests/SalmonEgg.Presentation.Core.Tests.csproj --configuration Debug --no-restore Remaining unverified GUI-only risks: None identified; the logic is entirely contained within the Core Presentation ViewModel's state projection which drives the XAML View.
Contributor
Author
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Your trial has ended. Reactivate Greptile to resume code reviews.
The ChatConversationSurfaceStatePresenter previously computed the OverlayLoadingStage completely statelessly. When intermediate transitions occurred (e.g. `isConnecting` falling to false before the overlay was dismissed), the stage could inadvertently regress from `Connecting` (stage 1) to `PreparingSession` (stage 4, the initial state). This violated the lifecycle monotonicity rule, resulting in the status pill text flickering backwards. This change introduces stateful tracking of the maximum attained `LoadingOverlayStage` rank within `ChatConversationSurfaceProjectionCoordinator` over the lifespan of a continuously visible overlay. The fix is safe architecturally because: 1. It is fully contained within the presentation's projection layer and does not mutate any underlying core state mechanisms. 2. It correctly resets its tracking the moment the activation overlay is fully dismissed, ensuring that completely new activations start clean. 3. The dependent `ShouldShowLoadingOverlayStatusPill` and `ShouldShowLoadingOverlayPresenter` boolean values are appropriately recomputed based on the monotonic stage to preserve Uno/WinUI view compatibility. Verification commands run: - dotnet build SalmonEgg.sln --configuration Debug - ./tests/SalmonEgg.Presentation.Core.Tests/bin/Debug/net10.0/SalmonEgg.Presentation.Core.Tests Remaining unverified GUI-only risks: None identified; the logic is entirely contained within the Core Presentation ViewModel's state projection which drives the XAML View.
The ChatConversationSurfaceStatePresenter previously computed the OverlayLoadingStage completely statelessly. When intermediate transitions occurred (e.g. `isConnecting` falling to false before the overlay was dismissed), the stage could inadvertently regress from `Connecting` (stage 1) to `PreparingSession` (stage 4, the initial state). This violated the lifecycle monotonicity rule, resulting in the status pill text flickering backwards. This change introduces stateful tracking of the maximum attained `LoadingOverlayStage` rank within `ChatConversationSurfaceProjectionCoordinator` over the lifespan of a continuously visible overlay. The fix is safe architecturally because: 1. It is fully contained within the presentation's projection layer and does not mutate any underlying core state mechanisms. 2. It correctly resets its tracking the moment the activation overlay is fully dismissed, ensuring that completely new activations start clean. 3. The dependent `ShouldShowLoadingOverlayStatusPill` and `ShouldShowLoadingOverlayPresenter` boolean values are appropriately recomputed based on the monotonic stage to preserve Uno/WinUI view compatibility. Additionally, this commit includes a small smoke test fix for WebAssembly gamepad bounds. Verification commands run: - dotnet build SalmonEgg.sln --configuration Debug - ./tests/SalmonEgg.Presentation.Core.Tests/bin/Debug/net10.0/SalmonEgg.Presentation.Core.Tests - bash scripts/gates/run-wasm-smoke-gates.sh Debug Remaining unverified GUI-only risks: None identified; the logic is entirely contained within the Core Presentation ViewModel's state projection which drives the XAML View.
The ChatConversationSurfaceStatePresenter previously computed the OverlayLoadingStage completely statelessly. When intermediate transitions occurred (e.g. `isConnecting` falling to false before the overlay was dismissed), the stage could inadvertently regress from `Connecting` (stage 1) to `PreparingSession` (stage 4, the initial state). This violated the lifecycle monotonicity rule, resulting in the status pill text flickering backwards. This change introduces stateful tracking of the maximum attained `LoadingOverlayStage` rank within `ChatConversationSurfaceProjectionCoordinator` over the lifespan of a continuously visible overlay. The fix is safe architecturally because: 1. It is fully contained within the presentation's projection layer and does not mutate any underlying core state mechanisms. 2. It correctly resets its tracking the moment the activation overlay is fully dismissed, ensuring that completely new activations start clean. 3. The dependent `ShouldShowLoadingOverlayStatusPill` and `ShouldShowLoadingOverlayPresenter` boolean values are appropriately recomputed based on the monotonic stage to preserve Uno/WinUI view compatibility. Additionally, this commit includes a small smoke test fix for WebAssembly gamepad bounds. Verification commands run: - dotnet build SalmonEgg.sln --configuration Debug - ./tests/SalmonEgg.Presentation.Core.Tests/bin/Debug/net10.0/SalmonEgg.Presentation.Core.Tests - bash scripts/gates/run-wasm-smoke-gates.sh Debug Remaining unverified GUI-only risks: None identified; the logic is entirely contained within the Core Presentation ViewModel's state projection which drives the XAML View.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Daily Lifecycle Review: Fix Overlay Loading Stage Regression
Observed Problem:
During rapid session switching and connection lifecycles, the UI loading overlay stage could inappropriately regress. For instance, when
isConnectingfinished but the session switch overlay remained briefly active, theOverlayLoadingStagewould fall back fromConnecting(or higher) to the initialPreparingSessionstage. This caused the UI loading pill and status text to flicker backwards, violating the rule that lifecycle transitions must be monotonic.Root Cause:
ChatConversationSurfaceStatePresenter.ResolveOverlayLoadingStagewas evaluating the stage in a purely stateless manner based on the instantaneous boolean inputs at each frame. As soon as a boolean likeisConnectingwentfalse, it evaluated the next true flag (sessionSwitchOverlayVisible) and returned its lower-ranked stage (PreparingSession).Architectural Safety:
The fix is safe architecturally because it operates purely within the domain of the
ChatConversationSurfaceProjectionCoordinator. It does not bleed view logic into the View model, nor does it create a secondary state machine. It simply acts as a high-water mark for theOverlayLoadingStagewhile an overlay is visible, and safely resets when the overlay is dismissed. It fully honors the existing Single Source of Truth for the connection state.Verification Commands Run:
Note: Due to a test runner issue with
--filter-class, the test executable was run directly which successfully validated the fix.Remaining Unverified GUI-Only Risks:
None. The behavior is well-covered by the new
ChatConversationSurfaceProjectionCoordinatorStageRegressionTestsand the existing presentation core tests. The WinUI/Uno XAML bindings will correctly reflect the now-monotonic projection.PR created automatically by Jules for task 4891367414608074776 started by @YoungSx