Conversation
Reviewer's GuideThis PR adds a full Browse/BrowseNext feature to the OPC DA extension by extending the server interface, introducing domain models and filter mappings, wiring up both real and mock implementations, and covering the workflow with a new unit test. Sequence diagram for OPC DA Browse workflowsequenceDiagram
participant Client
participant IOpcDaServer
participant OpcDaServer
participant OPC_DA_Server
Client->>IOpcDaServer: Browse(name, filters, out position)
IOpcDaServer->>OpcDaServer: Browse(name, filters, out position)
OpcDaServer->>OPC_DA_Server: server.Browse(ItemIdentifier, filters, out pos)
OPC_DA_Server-->>OpcDaServer: BrowseElement[]
OpcDaServer-->>IOpcDaServer: OpcBrowseElement[]
IOpcDaServer-->>Client: OpcBrowseElement[]
Note over Client,IOpcDaServer: position is returned for BrowseNext
Sequence diagram for OPC DA BrowseNext workflowsequenceDiagram
participant Client
participant IOpcDaServer
participant OpcDaServer
participant OPC_DA_Server
Client->>IOpcDaServer: BrowseNext(position)
IOpcDaServer->>OpcDaServer: BrowseNext(position)
OpcDaServer->>OPC_DA_Server: server.BrowseNext(ref pos)
OPC_DA_Server-->>OpcDaServer: BrowseElement[]
OpcDaServer-->>IOpcDaServer: OpcBrowseElement[]
IOpcDaServer-->>Client: OpcBrowseElement[]
Class diagram for new OPC DA Browse featureclassDiagram
class IOpcDaServer {
+HashSet<OpcWriteItem> Write(HashSet<OpcWriteItem> items)
+OpcBrowseElement[] Browse(string name, OpcBrowseFilters filters, out OpcBrowsePosition? position)
+OpcBrowseElement[] BrowseNext(OpcBrowsePosition position)
}
class OpcDaServer {
-Opc.Da.Server? _server
+OpcBrowseElement[] Browse(string name, OpcBrowseFilters filters, out OpcBrowsePosition? position)
+OpcBrowseElement[] BrowseNext(OpcBrowsePosition position)
}
class MockOpcDaServer {
+OpcBrowseElement[] Browse(string name, OpcBrowseFilters filters, out OpcBrowsePosition? position)
+OpcBrowseElement[] BrowseNext(OpcBrowsePosition position)
}
class OpcBrowseElement {
+string Name
+string ItemName
+bool IsItem
+bool HasChildren
+OpcBrowseElement()
+OpcBrowseElement(BrowseElement element)
}
class OpcBrowseFilters {
+int MaxElementsReturned
+string? ElementNameFilter
+bool ReturnAllProperties
+bool ReturnPropertyValues
+OpcBrowseFilterType BrowseFilter
}
class OpcBrowsePosition {
+OpcBrowsePosition(Opc.Da.BrowsePosition? position)
-Opc.Da.BrowsePosition? Position
}
class OpcBrowseFilterType {
<<enum>>
All
Branch
Item
}
IOpcDaServer <|.. OpcDaServer
IOpcDaServer <|.. MockOpcDaServer
OpcDaServer o-- OpcBrowseElement
OpcDaServer o-- OpcBrowseFilters
OpcDaServer o-- OpcBrowsePosition
MockOpcDaServer o-- OpcBrowseElement
MockOpcDaServer o-- OpcBrowseFilters
MockOpcDaServer o-- OpcBrowsePosition
OpcBrowseFilters --> OpcBrowseFilterType
OpcBrowseElement --> BrowseElement
OpcBrowsePosition --> Opc.Da.BrowsePosition
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @ArgoZhang - I've reviewed your changes - here's some feedback:
- Use C# array initializer syntax (e.g., new OpcBrowseElement[] { ... }) instead of JavaScript-style [ ... ] in your Browse and BrowseNext implementations.
- Your unit test asserts 3 elements for "Channel1" but the mock returns only one; either update the mock data or adjust the assertion to match the expected results.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Use C# array initializer syntax (e.g., new OpcBrowseElement[] { ... }) instead of JavaScript-style [ ... ] in your Browse and BrowseNext implementations.
- Your unit test asserts 3 elements for "Channel1" but the mock returns only one; either update the mock data or adjust the assertion to match the expected results.
## Individual Comments
### Comment 1
<location> `src/extensions/BootstrapBlazor.OpcDa/Mock/MockOpcDaServer.cs:74` </location>
<code_context>
+ /// <param name="filters"></param>
+ /// <param name="position"></param>
+ /// <returns></returns>
+ public OpcBrowseElement[] Browse(string name, OpcBrowseFilters filters, out OpcBrowsePosition? position)
+ {
+ position = null;
</code_context>
<issue_to_address>
Consider refactoring the Browse method to use a static tree structure and a data-driven lookup instead of multiple hard-coded if statements.
Here is a quick data‐driven refactor that keeps the same behavior but collapses all of those hard-coded branches into a single tree lookup.
1. Define a simple node model:
```csharp
class BrowseNode
{
public string Name { get; init; }
public string ItemName { get; init; }
public bool IsItem { get; init; }
public BrowseNode[] Children { get; init; } = Array.Empty<BrowseNode>();
}
```
2. Build a static tree once:
```csharp
static readonly BrowseNode[] _tree = new[]
{
new BrowseNode
{
Name = "Channel1",
ItemName = "Channel1",
Children = new[]
{
new BrowseNode
{
Name = "Device1",
ItemName = "Channel1.Device1",
Children = new[]
{
new BrowseNode { Name = "Tag1", ItemName = "Channel1.Device1.Tag1", IsItem = true },
new BrowseNode { Name = "Tag2", ItemName = "Channel1.Device1.Tag2", IsItem = true }
}
}
}
},
new BrowseNode { Name = "Channel2", ItemName = "Channel2" }
};
```
3. Rewrite `Browse` to just walk that tree:
```csharp
public OpcBrowseElement[] Browse(string name, OpcBrowseFilters filters, out OpcBrowsePosition? position)
{
position = null;
// find the node (or use root set if name is empty)
IEnumerable<BrowseNode> list = string.IsNullOrEmpty(name)
? _tree
: _tree.SelectMany(n => Flatten(n))
.FirstOrDefault(n => n.ItemName == name)?
.Children
?? Array.Empty<BrowseNode>();
return list.Select(n => new OpcBrowseElement
{
Name = n.Name,
ItemName = n.ItemName,
IsItem = n.IsItem,
HasChildren = n.Children.Length > 0
}).ToArray();
}
// helper to flatten subtree including the root node
IEnumerable<BrowseNode> Flatten(BrowseNode node)
{
yield return node;
foreach (var c in node.Children)
foreach (var x in Flatten(c))
yield return x;
}
```
This:
- Eliminates all the `if(name == "...")` blocks
- Centralizes your structure in one place for easy extension
- Keeps the exact same output of channels, devices, and tags
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
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
1 participant
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 #519
Summary By Copilot
Regression?
Risk
Verification
Packaging changes reviewed?
☑️ Self Check before Merge
Summary by Sourcery
Add browsing capabilities to the BootstrapBlazor OPC DA extension, enabling clients to enumerate channels, devices, and tags on an OPC DA server.
New Features:
Enhancements:
Tests: