-
Notifications
You must be signed in to change notification settings - Fork 39
Fixed stale state issue in ResourceEditor after saving #530
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Closes #523 Signed-off-by: Jean-Baptiste Bianchi <[email protected]>
a27e584
to
0881f78
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR fixes a stale state issue in the ResourceEditor component by ensuring proper state synchronization after save operations. The changes focus on improving the state management flow and preventing race conditions when updating the UI after resource modifications.
- Modified OnStateChanged method to accept optional patch parameter for triggering state updates without patches
- Updated resource editor opening methods to use proper async/await pattern with duplicate ShowAsync calls
- Enhanced ResourceEditor state subscription handling and removed manual StateHasChanged calls in favor of centralized state management
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
File | Description |
---|---|
StatefulComponent.cs | Made patch parameter optional in OnStateChanged method to allow state refresh without modifications |
View.razor | Updated OnShowResourceEditorAsync to use async/await pattern with duplicate ShowAsync calls |
ResourceManagementComponent.cs | Applied same async/await pattern changes as View.razor for consistency |
ResourceEditor.razor | Improved state subscription handling and replaced manual StateHasChanged calls with centralized OnStateChanged |
var parameters = new Dictionary<string, object> | ||
{ | ||
{ nameof(ResourceEditor<Namespace>.Resource), resource! }, | ||
{ nameof(ResourceEditor<Namespace>.IsCluster), true } | ||
}; | ||
string actionType = resource == null ? "creation" : "edition"; | ||
return this.EditorOffCanvas.ShowAsync<ResourceEditor<Namespace>>(title: typeof(Namespace).Name + " " + actionType, parameters: parameters); | ||
await this.EditorOffCanvas.ShowAsync<ResourceEditor<Namespace>>(title: typeof(Namespace).Name + " " + actionType); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Duplicate ShowAsync call detected. Line 107 calls ShowAsync without parameters, then line 108 calls it again with parameters. The first call on line 107 should be removed as it serves no purpose and may cause unexpected behavior.
await this.EditorOffCanvas.ShowAsync<ResourceEditor<Namespace>>(title: typeof(Namespace).Name + " " + actionType); |
Copilot uses AI. Check for mistakes.
var parameters = new Dictionary<string, object> | ||
{ | ||
{ nameof(ResourceEditor<TResource>.Resource), resource! } | ||
}; | ||
string actionType = resource == null ? "creation" : "edition"; | ||
return this.EditorOffCanvas.ShowAsync<ResourceEditor<TResource>>(title: typeof(TResource).Name + " " + actionType, parameters: parameters); | ||
await this.EditorOffCanvas.ShowAsync<ResourceEditor<TResource>>(title: typeof(TResource).Name + " " + actionType); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Duplicate ShowAsync call detected. Line 247 calls ShowAsync without parameters, then line 248 calls it again with parameters. The first call on line 247 should be removed as it serves no purpose and may cause unexpected behavior.
await this.EditorOffCanvas.ShowAsync<ResourceEditor<TResource>>(title: typeof(TResource).Name + " " + actionType); |
Copilot uses AI. Check for mistakes.
@@ -125,9 +125,9 @@ | |||
protected override async Task OnInitializedAsync() | |||
{ | |||
await base.OnInitializedAsync().ConfigureAwait(false); | |||
this.Store.IsSaving.Subscribe(saving => this.OnStateChanged(cmp => this.OnSavingChanged(saving)), token: this.CancellationTokenSource.Token); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Extra space in lambda parameter 'cmp =>' should be 'cmp =>' for consistency with other lambda expressions in the code.
this.Store.IsSaving.Subscribe(saving => this.OnStateChanged(cmp => this.OnSavingChanged(saving)), token: this.CancellationTokenSource.Token); | |
this.Store.IsSaving.Subscribe(saving => this.OnStateChanged(cmp => this.OnSavingChanged(saving)), token: this.CancellationTokenSource.Token); |
Copilot uses AI. Check for mistakes.
Many thanks for submitting your Pull Request ❤️!
What this PR does / why we need it:
Ensure resource editor reflects latest resource state.
Closes #523
Special notes for reviewers:
Additional information (if needed):