Add protobuf serialization infrastructure

[TimPurdum]

Summary

This PR introduces the foundational infrastructure for protobuf-based binary serialization in GeoBlazor Core. This is the first in a series of PRs to incrementally merge changes from the feature/478-multipart-operator branch.

What's Included

• Serialization base classes: MapComponentSerializationRecord and MapComponentCollectionSerializationRecord provide the foundation for all protobuf serialization records
• IProtobufSerializable interface: Defines the contract for types that can be serialized to protobuf format
• Custom attributes: ProtobufSerializableAttribute and SerializedMethodAttribute for marking types and methods for serialization
• JsSyncManager stub: Placeholder for JS interop with protobuf support (will be expanded in future PRs)
• ProtobufSourceGenerator: Skeleton source generator that detects protobuf-attributed types (generation logic to be added later)
• SourceGenerator.Shared project: Shared utilities for source generators including ProcessHelper for PowerShell script execution
• Build configuration updates: Directory.Build.props now includes CoreProjectPath for source generators

Why Protobuf Serialization?

Binary serialization via protobuf-net offers significant performance improvements over JSON for Blazor Server scenarios:

• Reduced payload size: Binary format is more compact than JSON
• Faster serialization/deserialization: Protobuf is optimized for speed
• Streaming support: Enables efficient transfer of large datasets (graphics, geometries)

Relationship to Larger Changes

This PR is part of a planned series to merge the feature/478-multipart-operator branch incrementally. The full change set includes:

1. Protobuf Serialization Infrastructure ← This PR
2. LogicComponent Refactoring & Null Safety
3. GeometryEngine Enhancements (multipart geometry operators)
4. Custom Popup Content Support
5. Search Widget Enhancements
6. Additional Widget & Component Updates

By merging these changes incrementally, we can:

• Review each feature area in isolation
• Ensure stability between merges
• Make it easier to identify and fix any issues

Test Plan

• Verified Core project builds successfully
• Run existing unit tests to ensure no regressions
• Verify source generators execute without errors during build

🤖 Generated with Claude Code

[claude]

Claude finished @TimPurdum's task —— View job

---

PR Review: Add protobuf serialization infrastructure

Todo List:

• Read and analyze all changed files
• Review protobuf serialization base classes and interfaces
• Examine source generator implementations
• Check build configuration changes
• Evaluate PowerShell script additions
• Provide comprehensive feedback with specific recommendations

Review Summary

This PR introduces the foundational infrastructure for protobuf-based binary serialization in GeoBlazor Core. While the implementation is largely skeletal (as intended for this incremental approach), there are several important code quality and design issues that should be addressed.

Critical Issues

1. Missing Assembly Reference (GeoBlazorMetaData.cs:11)

Assembly proAssembly = Assembly.Load("dymaptic.GeoBlazor.Pro");

Problem: Missing using System.Reflection; statement will cause compilation failure.

Fix: Add the missing using statement at the top of the file.

2. Unsafe Null Handling (ProtobufDefinitionsGenerator.cs:56)

return ProtoDefinitions!; // Null-forgiving operator without validation

Problem: Using null-forgiving operator without ensuring the dictionary is actually populated. If ExtractProtobufDefinitions returns an empty dictionary, this could lead to issues.

Recommendation: Add proper null checking:

return ProtoDefinitions ?? new Dictionary<string, ProtoMessageDefinition>();

3. PowerShell Script Security Concerns (ProcessHelper.cs:16-17)

string shellArguments = $"-NoProfile -ExecutionPolicy ByPass -File \"{
    Path.Combine(workingDirectory, powershellScriptName)}\" {arguments}";

Security Risk: Using -ExecutionPolicy ByPass completely disables PowerShell's execution policy, which is a security mechanism.

Recommendation: Consider using -ExecutionPolicy RemoteSigned or handle execution policy more securely.

4. Inconsistent Error Handling (ProcessHelper.cs:87-92)

if (exitCode != 0)
{
    logBuilder.AppendLine(outputBuilder.ToString()); // Duplicate logging
    throw new Exception($"{processName}: Error executing command '{shellArguments}' for process {processId}. Exit code: {exitCode}");
}

Problem: The output is logged twice (line 75 and 89), and using generic Exception instead of specific exception types.

Fix: Remove duplicate logging and use more specific exceptions like InvalidOperationException.

Code Quality Issues

5. Incomplete Implementation Pattern

Multiple classes have empty or placeholder implementations:

• SerializationExtensions.cs - Completely empty partial class
• JsSyncManager.cs - Placeholder methods that don't use protobuf serialization yet

Recommendation: Consider adding TODO comments or XML documentation explaining when these will be implemented.

6. String Concatenation Performance (ProtobufDefinitionsGenerator.cs:32-35)

string encoded = typescriptContent
    .Replace("\"", "\\\"")
    .Replace("\r\n", "\\r\\n")
    .Replace("\n", "\\n");

Issue: Multiple string replacements create intermediate strings.

Optimization: Use StringBuilder or consider regex for better performance with large content.

7. Hardcoded Magic Numbers

• Line 14: [ProtoMember(1000)] - Magic number without explanation
• Line 39: [ProtoMember(1000)] - Same magic number reused

Recommendation: Use const fields or document why 1000 is chosen.

Architecture and Design Concerns

8. Source Generator Complexity

The ProtobufDefinitionsGenerator class is doing too many things:

• Extracting protobuf definitions
• Generating proto file content
• Writing to TypeScript files via PowerShell
• Managing state with static dictionaries

Recommendation: Consider splitting into separate responsibilities for better maintainability.

9. Global State Management

public static Dictionary<string, ProtoMessageDefinition>? ProtoDefinitions;

Issue: Mutable global state in source generators can lead to inconsistent behavior across compilation runs.

Recommendation: Consider passing state through the generator pipeline instead of using static fields.

10. Exception Swallowing (GeoBlazorMetaData.cs:15-18)

catch
{
    // GeoBlazor.Pro not available
}

Problem: Catching all exceptions without logging specific failure reasons.

Fix: At minimum catch specific exceptions and consider logging the attempt.

Performance Considerations

11. Reflection Performance (GeoBlazorMetaData.cs:7)

GeoblazorTypes = typeof(GeoBlazorMetaData).Assembly.GetTypes();

Issue: GetTypes() is expensive and called in static constructor, affecting startup time.

Recommendation: Consider lazy initialization or caching strategies.

12. LINQ Performance in Generators (ProtobufDefinitionsGenerator.cs:105-106)

Multiple LINQ operations in hot paths within source generators can impact compilation performance.

Recommendation: Consider more efficient enumeration patterns where possible.

Documentation and Testing

13. Missing XML Documentation

Several public APIs lack proper documentation:

• ProtoMessageDefinition class and its properties
• ProcessHelper.Execute method parameters
• Exception scenarios in public methods

14. No Test Coverage

The PR adds significant infrastructure but no corresponding unit tests for:

• Protobuf definition extraction logic
• Source generator behavior
• PowerShell script execution error handling

Positive Aspects

✅ Good separation of concerns with the shared source generator project
✅ Proper use of incremental generators for performance
✅ Consistent naming conventions throughout the codebase
✅ Appropriate use of records for immutable data structures
✅ Clear intent with the infrastructure-first approach

Recommendations for Next Steps

1. Address compilation issues (missing using statements)
2. Improve error handling patterns throughout
3. Add comprehensive unit tests before adding more functionality
4. Review security implications of PowerShell execution bypassing policies
5. Consider performance impact of reflection and LINQ in generators
6. Add proper logging/diagnostics for debugging source generation issues

The overall architecture looks sound for the intended incremental approach, but the implementation needs refinement before adding the full protobuf functionality in subsequent PRs.

[claude]

Claude finished @TimPurdum's task —— View job

---

PR Review: Add protobuf serialization infrastructure

Todo List:

• Check git history to understand changes since previous review
• Read and analyze all changed files in current PR state
• Compare against previous Claude review to identify new issues
• Provide comprehensive feedback focusing on code quality, bugs, performance, security
• Update comment with final review findings

Review of Changes Since Previous Claude Review

The author addressed many of the issues from the previous review in commit c6db4ad. Here's what was fixed and what still needs attention:

✅ Issues Addressed

1. Added comprehensive TODO comments - All placeholder implementations now have clear documentation about future work
2. Documented magic number 1000 - ProtoMember(1000) usage is now properly explained as reserved for base class properties
3. Improved exception handling - GeoBlazorMetaData.cs now catches FileNotFoundException specifically instead of bare catch
4. Added XML documentation - Key classes now have proper documentation
5. Fixed duplicate logging in ProcessHelper - Removed the redundant logging that was making error diagnostics empty

❌ Critical Issues Still Remaining

1. COMPILATION FAILURE - Missing Using Statement (GeoBlazorMetaData.cs:16)

Assembly proAssembly = Assembly.Load("dymaptic.GeoBlazor.Pro");

Problem: Still missing using System.Reflection; - this will cause compilation failure.
Severity: Critical - blocks compilation
Fix Required: Add using System.Reflection; at the top of the file.

2. Unsafe Null Handling (ProtobufDefinitionsGenerator.cs:56)

return ProtoDefinitions!; // Still using null-forgiving operator without validation

Problem: No guarantee ExtractProtobufDefinitions populates the dictionary. If it returns empty, runtime issues could occur.
Fix: return ProtoDefinitions ?? new Dictionary<string, ProtoMessageDefinition>();

3. Security Risk Still Present (ProcessHelper.cs:16-17)

string shellArguments = $"-NoProfile -ExecutionPolicy ByPass -File \"{
    Path.Combine(workingDirectory, powershellScriptName)}\" {arguments}";

Issue: -ExecutionPolicy ByPass completely disables PowerShell security policies.
Recommendation: Use -ExecutionPolicy RemoteSigned or implement proper script signing.

🔍 Newly Identified Issues

4. Performance: Multiple String Replacements (ProtobufDefinitionsGenerator.cs:32-35)

string encoded = typescriptContent
    .Replace("\"", "\\\"")
    .Replace("\r\n", "\\r\\n")
    .Replace("\n", "\\n");

Issue: Creates intermediate string objects with each replacement.
Fix: Use StringBuilder for better performance.

5. Resource Management: Missing Task ConfigureAwait (ProtobufDefinitionsGenerator.cs:44)

ProcessHelper.RunPowerShellScript("Copy Protobuf Definitions",
    corePath, scriptPath, $"-Content \"{encoded}\"",
    logBuilder, context.CancellationToken).GetAwaiter().GetResult();

Issues:

• Synchronous wait on async operation (blocks thread)
• Missing ConfigureAwait(false) (though GetAwaiter().GetResult() doesn't capture context)
  Recommendation: Consider making the calling method async or document why sync is required.

6. Error Handling: Generic Exception Type (ProcessHelper.cs:87)

throw new Exception($"{processName}: Error executing command '{shellArguments}' for process {processId}. Exit code: {exitCode}");

Issue: Using generic Exception instead of specific exception types.
Fix: Use InvalidOperationException or create custom exception type.

7. String Interpolation in Hot Path (ProtobufDefinitionsGenerator.cs:43)

$"-Content \"{encoded}\""

Issue: When encoded is large (complex TypeScript), string interpolation creates performance overhead.
Recommendation: Pre-format argument string.

🏗️ Architecture Concerns (Lower Priority)

8. Global Mutable State (ProtobufDefinitionsGenerator.cs:14)

public static Dictionary<string, ProtoMessageDefinition>? ProtoDefinitions;

Issue: Global mutable state in source generators can cause inconsistencies across compilation runs.
Impact: Medium - may cause intermittent generation issues.

9. Source Generator Complexity

The ProtobufDefinitionsGenerator class has multiple responsibilities:

• Extracting protobuf definitions
• Generating proto content
• Writing TypeScript via PowerShell
• Managing global state

Recommendation: Consider splitting responsibilities as the implementation grows.

🛡️ Security Assessment

The PowerShell execution bypass remains the main security concern. While this is development tooling, consider:

1. Document why bypass is required
2. Consider alternative approaches (direct file I/O vs PowerShell)
3. Validate script paths to prevent injection

⚡ Performance Considerations

1. Reflection Cost (GeoBlazorMetaData.cs:10) - GetTypes() in static constructor affects startup
2. String Operations - Multiple replacements and interpolation in hot paths
3. Async Blocking - Synchronous waits on async operations

📋 Missing Test Coverage

The PR still lacks unit tests for:

• Protobuf definition extraction logic
• Source generator behavior with various input scenarios
• PowerShell script execution error handling
• Edge cases in metadata extraction

✅ Positive Aspects

• Excellent documentation improvements - TODOs and XML docs are comprehensive
• Proper exception specificity in GeoBlazorMetaData
• Clear architectural intent for incremental implementation
• Good separation of concerns with shared generator project

Summary and Recommendations

Immediate Actions Required (Pre-Merge)

1. Fix compilation issue - Add missing using System.Reflection;
2. Address null safety - Handle null ProtoDefinitions properly
3. Review PowerShell security - Document or change execution policy approach

Follow-up Improvements

1. Add comprehensive unit tests
2. Optimize string operations for performance
3. Consider architecture improvements as functionality expands
4. Add proper async patterns or document sync requirements

The infrastructure foundation is solid, but the compilation issue must be resolved before merge. The other issues can be addressed in subsequent PRs as the functionality is built out.

[TimPurdum]

The comment about missing usings is incorrect, Claude is missing the GlobalUsings.cs file consistently.