Conversation
Contributor
Reviewer's GuideUpdates the OpcDa sample to use localized UI text, ensures culture is preserved when processing subscription callbacks, and introduces mock OpcDa server/subscription services for testing or non-Windows environments. Sequence diagram for OpcDa subscription callback with culture preservationsequenceDiagram
actor User
participant OpcDaComponent as OpcDa
participant IOpcDaServer
participant Subscription as MockOpcDaSubscription
User->>OpcDaComponent: OnCreateSubscription()
OpcDaComponent->>OpcDaComponent: _culture = CultureInfo.CurrentCulture
OpcDaComponent->>OpcDaComponent: _uiCulture = CultureInfo.CurrentUICulture
OpcDaComponent->>IOpcDaServer: CreateSubscription(Subscription1, 1000, true)
IOpcDaServer-->>OpcDaComponent: IOpcSubscription
OpcDaComponent->>Subscription: AddItems([Tag1, Tag2])
Subscription->>Subscription: DoTask(token)
loop every updateRate
Subscription->>Subscription: UpdateValues()
Subscription-->>OpcDaComponent: DataChanged(items)
OpcDaComponent->>OpcDaComponent: UpdateValues(items)
OpcDaComponent->>OpcDaComponent: InvokeAsync(lambda)
activate OpcDaComponent
OpcDaComponent->>OpcDaComponent: CultureInfo.CurrentCulture = _culture
OpcDaComponent->>OpcDaComponent: CultureInfo.CurrentUICulture = _uiCulture
OpcDaComponent->>OpcDaComponent: StateHasChanged()
deactivate OpcDaComponent
end
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
ArgoZhang
changed the title
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 3 issues, and left some high level feedback:
- In
OpcDa.UpdateValues, you overrideCultureInfo.CurrentCulture/CurrentUICulturewithout restoring the previous values; consider capturing the original cultures and restoring them in atry/finallyto avoid leaking culture changes into unrelated UI callbacks on the same synchronization context. - In
MockOpcDaServer.Disposeyou currently do nothing; if this mock is reused across the app lifetime, consider iterating existing subscriptions and callingCancelSubscriptionto ensure timers/tasks fromMockOpcDaSubscriptionare properly stopped and disposed.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `OpcDa.UpdateValues`, you override `CultureInfo.CurrentCulture`/`CurrentUICulture` without restoring the previous values; consider capturing the original cultures and restoring them in a `try/finally` to avoid leaking culture changes into unrelated UI callbacks on the same synchronization context.
- In `MockOpcDaServer.Dispose` you currently do nothing; if this mock is reused across the app lifetime, consider iterating existing subscriptions and calling `CancelSubscription` to ensure timers/tasks from `MockOpcDaSubscription` are properly stopped and disposed.
## Individual Comments
### Comment 1
<location path="src/BootstrapBlazor.Server/Components/Samples/OpcDa.razor.cs" line_range="68-72" />
<code_context>
private void OnCreateSubscription()
{
_subscribed = true;
+ _culture = CultureInfo.CurrentCulture;
+ _uiCulture = CultureInfo.CurrentUICulture;
_subscription = OpcDaServer.CreateSubscription("Subscription1", 1000, true);
</code_context>
<issue_to_address>
**issue (bug_risk):** Persisting and globally setting cultures in subscription callbacks can have side effects and is not reverted.
The callback captures `CurrentCulture`/`CurrentUICulture` once and then reapplies them on every `UpdateValues` via `InvokeAsync`, without restoring the prior values. Since these are ambient thread-wide settings, this can cause subtle issues if other code changes culture or if the culture changes after the subscription is created. Prefer using the captured cultures only where needed (e.g., `value1.ToString(_culture)`) instead of setting `CurrentCulture`/`CurrentUICulture`, or at least capture and restore the previous culture inside `UpdateValues`.
</issue_to_address>
### Comment 2
<location path="src/BootstrapBlazor.Server/Components/Samples/OpcDa.razor.cs" line_range="84-87" />
<code_context>
private void UpdateValues(List<OpcReadItem> items)
{
- var value1 = items.Find(i => i.Name == Tag1).Value;
- if (value1 != null)
- {
- _tagValue3 = value1.ToString();
- }
- var value2 = items.Find(i => i.Name == Tag2).Value;
- if (value2 != null)
+ _ = InvokeAsync(() =>
</code_context>
<issue_to_address>
**issue (bug_risk):** Potential null dereference when an expected tag is missing from the subscription update list.
`items.Find(i => i.Name == Tag1)` and `items.Find(i => i.Name == Tag2)` may return `null` (e.g., if the server returns only a subset of items or tag names change), leading to a `NullReferenceException` when accessing `.Value`. Please add a null check on the `Find` result before reading `.Value` so the code remains robust if server responses or subscription configuration change.
</issue_to_address>
### Comment 3
<location path="src/BootstrapBlazor.Server/Components/Samples/OpcDa.razor.cs" line_range="33" />
<code_context>
private IOpcSubscription? _subscription;
private bool _subscribed;
+ private CultureInfo? _culture;
+ private CultureInfo? _uiCulture;
</code_context>
<issue_to_address>
**issue (complexity):** Consider simplifying the new culture handling by using non-null captured cultures for formatting instead of mutating global culture and checking nullable fields inside the callback.
The new culture handling adds avoidable complexity via nullable fields and global culture mutation. You can keep the culture-sensitive behavior while simplifying the code and avoiding side effects.
**Key simplifications:**
1. Treat culture as non-null once a subscription exists.
2. Use the stored culture as an `IFormatProvider` instead of mutating `CultureInfo.CurrentCulture` / `CurrentUICulture`.
3. Keep `InvokeAsync` for correct Blazor threading, but only for UI updates and formatting.
### Suggested changes
**1. Make culture non-nullable and capture once**
```csharp
private CultureInfo _culture = CultureInfo.CurrentCulture;
// If you truly need UI culture separately:
private CultureInfo _uiCulture = CultureInfo.CurrentUICulture;
```
Update in `OnCreateSubscription` (no nullable fields, no extra checks later):
```csharp
private void OnCreateSubscription()
{
_subscribed = true;
_culture = CultureInfo.CurrentCulture;
_uiCulture = CultureInfo.CurrentUICulture;
_subscription = OpcDaServer.CreateSubscription("Subscription1", 1000, true);
_subscription.DataChanged += UpdateValues;
_subscription.AddItems([Tag1, Tag2]);
}
```
**2. Use the captured culture only for formatting, no global mutation**
```csharp
private void UpdateValues(List<OpcReadItem> items)
{
// Capture once for thread-safety and clarity
var culture = _culture;
_ = InvokeAsync(() =>
{
var value1 = items.Find(i => i.Name == Tag1).Value;
if (value1 != null)
{
_tagValue3 = Convert.ToString(value1, culture);
}
var value2 = items.Find(i => i.Name == Tag2).Value;
if (value2 != null)
{
_tagValue4 = Convert.ToString(value2, culture);
}
StateHasChanged();
});
}
```
If you specifically need `CurrentUICulture` semantics (e.g., for localized strings), you can use `_uiCulture` the same way:
```csharp
_tagValue3 = Convert.ToString(value1, _uiCulture);
```
**Benefits:**
- Removes global side effects on `CultureInfo.CurrentCulture` / `CurrentUICulture`.
- Eliminates nullable `_culture` / `_uiCulture` and their null checks.
- Keeps the new behavior: values are formatted using the culture captured at subscription time.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+68
to
72
| _culture = CultureInfo.CurrentCulture; | ||
| _uiCulture = CultureInfo.CurrentUICulture; | ||
| _subscription = OpcDaServer.CreateSubscription("Subscription1", 1000, true); | ||
| _subscription.DataChanged += UpdateValues; | ||
| _subscription.AddItems([Tag1, Tag2]); |
Contributor
There was a problem hiding this comment.
issue (bug_risk): Persisting and globally setting cultures in subscription callbacks can have side effects and is not reverted.
The callback captures CurrentCulture/CurrentUICulture once and then reapplies them on every UpdateValues via InvokeAsync, without restoring the prior values. Since these are ambient thread-wide settings, this can cause subtle issues if other code changes culture or if the culture changes after the subscription is created. Prefer using the captured cultures only where needed (e.g., value1.ToString(_culture)) instead of setting CurrentCulture/CurrentUICulture, or at least capture and restore the previous culture inside UpdateValues.
Comment on lines
84
to
+87
|
|
||
| private void UpdateValues(List<OpcReadItem> items) | ||
| { | ||
| var value1 = items.Find(i => i.Name == Tag1).Value; | ||
| if (value1 != null) | ||
| { | ||
| _tagValue3 = value1.ToString(); | ||
| } | ||
| var value2 = items.Find(i => i.Name == Tag2).Value; | ||
| if (value2 != null) | ||
| _ = InvokeAsync(() => |
Contributor
There was a problem hiding this comment.
issue (bug_risk): Potential null dereference when an expected tag is missing from the subscription update list.
items.Find(i => i.Name == Tag1) and items.Find(i => i.Name == Tag2) may return null (e.g., if the server returns only a subset of items or tag names change), leading to a NullReferenceException when accessing .Value. Please add a null check on the Find result before reading .Value so the code remains robust if server responses or subscription configuration change.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #8070 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 766 766
Lines 34155 34155
Branches 4697 4697
=========================================
Hits 34155 34155
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
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.
Link issues
fixes #8069
Summary By Copilot
Regression?
Risk
Verification
Packaging changes reviewed?
☑️ Self Check before Merge
Summary by Sourcery
Localize the OpcDa sample UI and add mock OPC DA server implementations for demonstration and testing.
New Features:
Enhancements: