@claude Review the latest code.

Claude finished @from2001's task in 1m 33s —— View job

Code Review Complete ✓

Reviewing PR #290: feat: update transform sync settings

Summary

This PR successfully refactors network synchronization settings to improve encapsulation and Unity Editor usability. The changes are well-structured and maintain consistency between the Unity client and Python server. However, there are a few issues that need to be addressed.

Issues Found

🔴 Critical Issue: Missing Property Setter Validation

File: STYLY-NetSync-Unity/Packages/com.styly.styly-netsync/Runtime/NetSyncManager.cs:310-314

The ServerDiscoveryPort property setter lacks validation and doesn't propagate changes to the discovery manager. This is inconsistent with the TransformSendRate property which properly validates and updates its manager.

public int ServerDiscoveryPort
{
    get => _serverDiscoveryPort;
    set => _serverDiscoveryPort = value;  // ❌ No validation, no propagation
}

Impact:

• If set at runtime with an invalid value (< 1), the validation is bypassed
• Changes won't take effect until StartDiscovery() is called
• Inconsistent with the [Min(1)] attribute on the backing field

Recommendation: Add validation matching the field attribute and propagate to the discovery manager:

public int ServerDiscoveryPort
{
    get => _serverDiscoveryPort;
    set
    {
        var clamped = Mathf.Max(1, value);
        if (_serverDiscoveryPort == clamped)
        {
            return;
        }

        _serverDiscoveryPort = clamped;

        if (_discoveryManager != null)
        {
            _discoveryManager.Port = _serverDiscoveryPort;
        }
    }
}

Note: The copilot reviewer also identified this same issue.

Fix this issue →

⚠️ Minor Issue: Inconsistent Range Attributes

File: STYLY-NetSync-Unity/Packages/com.styly.styly-netsync/Runtime/NetSyncManager.cs:44

The _transformSendRate field has [Range(1, 60)] but the property setter uses Mathf.Clamp(value, 1f, 60f). While functionally correct, this creates a maintenance burden if the range needs to change.

Recommendation: Consider extracting the range constants:

private const float MIN_TRANSFORM_SEND_RATE = 1f;
private const float MAX_TRANSFORM_SEND_RATE = 60f;

[SerializeField, Range(MIN_TRANSFORM_SEND_RATE, MAX_TRANSFORM_SEND_RATE), Tooltip("...")]
private float _transformSendRate = 10f;

public float TransformSendRate
{
    get => _transformSendRate;
    set
    {
        _transformSendRate = Mathf.Clamp(value, MIN_TRANSFORM_SEND_RATE, MAX_TRANSFORM_SEND_RATE);
        // ...
    }
}

This ensures the range is consistent between the Inspector and the property setter.

Positive Observations

1. Excellent Encapsulation: Converting public fields to private serialized fields with public properties follows Unity best practices.

2. Good Documentation: Tooltips and XML comments are clear and helpful for users.

3. Consistent Naming: The naming convention (_transformSendRate field → TransformSendRate property) is clean and consistent.

4. Server-Client Consistency: The Python server configuration correctly mirrors the Unity client changes:

   • Removed base_broadcast_interval and dirty_threshold
   • Added transform_broadcast_rate with proper Hz-to-interval conversion
   • Updated validation to check 1-60 Hz range
   • Updated tests comprehensively

5. Editor Improvements: The custom label "Transform Send Rate (Hz)" in NetSyncManagerEditor.cs improves clarity.

6. No Null Propagation Operators: The code correctly avoids using ?. with Unity objects, following the CLAUDE.md guidelines.

7. Code Cleanup: Removed the unused _sendRate field to avoid confusion.

Testing Recommendations

Before merging, please verify:

1. Unity Inspector: Test that the new advanced settings display correctly and validate ranges in the Inspector
2. Runtime Changes: Test setting TransformSendRate and ServerDiscoveryPort at runtime to ensure they work as expected
3. Python Tests: Run the Python test suite to verify all config changes work correctly:

   cd STYLY-NetSync-Server
   pytest

4. Integration Test: Verify that Unity client and Python server work together with the new transform broadcast rate settings

Files Reviewed

• ✅ NetSyncManager.cs - Main Unity changes (1 critical issue, 1 minor issue)
• ✅ NetSyncManagerEditor.cs - Editor UI changes (looks good)
• ✅ config.py - Python config refactor (looks good)
• ✅ default.toml - Default config values (looks good)
• ✅ server.py - Server initialization changes (looks good)
• ✅ test_config.py - Test updates (comprehensive)

|