KM-13582 Remove UI-related code from PIALibrary and modularize UI components

[kp-diego-trevisan]

Summary

• Create PIAUI package with two new libraries: PIAUIKit and PIASwiftUI
• Move PIADesignSystem library inside PIAUI
• Move several UIKit components from PIALibrary and PIA VPN to PIAUIKit and PIASwiftUI
• Extra: Converted legacy Objective-C code to Swift and remove the need for PIALibraryUtilObjC

This PR fixes/implements the following bugs/features

• Bug 1
• Bug 2
• Feature 1
• Feature 2
• Breaking changes

Test plan (required)

Code formatting

Closing issues

[claude]

Claude finished @kp-diego-trevisan's task —— View job

---

📋 Code Review Progress

Reviewing this large-scale refactoring PR that moves UI components from PIALibrary to a new PIAUI package structure.

Todo List:

• Examine new PIAUI package structure and dependencies
• Review Objective-C to Swift conversion (Pinger, Data compression, etc.)
• Check moved UI components for potential issues
• Analyze package dependencies and circular dependency risks
• Review test coverage and identify gaps
• Check for memory management and threading concerns
• Verify security implications
• Provide comprehensive feedback with specific recommendations

---

🔍 Comprehensive Code Review

✅ Positive Aspects

1. Architecture Improvement: The new PIAUI package structure with separate PIAUIKit and PIASwiftUI libraries provides better separation of concerns.
2. Modern Swift: Excellent Objective-C to Swift conversion with proper error handling and memory safety.
3. Test Coverage: Good test coverage for the converted Data compression functionality.
4. Type Safety: Uses modern Swift features like typed throws and proper error enums.

⚠️ Critical Issues & Concerns

1. Network Security Risk in Pinger.swift (High Priority)

File: LocalPackages/PIALibrary/Sources/PIALibrary/Util/Pinger.swift:56

address.sin_addr.s_addr = inet_addr(hostname)

Issue: Using inet_addr() directly without hostname validation creates security vulnerabilities:

• No validation that hostname is a valid IP address
• Potential for buffer overflows or malformed address handling
• Should use getaddrinfo() for proper hostname resolution

Recommendation:

// Replace with proper hostname resolution
var hints = addrinfo()
hints.ai_family = AF_INET
hints.ai_socktype = SOCK_STREAM
var result: UnsafeMutablePointer<addrinfo>?
defer { freeaddrinfo(result) }

guard getaddrinfo(hostname, String(port), &hints, &result) == 0,
      let addr = result?.pointee.ai_addr else {
    return nil
}

2. Memory Management Issues

File: LocalPackages/PIALibrary/Sources/PIALibrary/Util/Data+Compression.swift:39

The throws(CompressionError) syntax is using upcoming Swift 6 typed throws which may not be available in Swift 5.9. This could cause compilation failures.

Recommendation: Use standard throws until Swift 6 is adopted:

func deflated() throws -> Data {

3. Race Condition in LoadingView

File: LocalPackages/PIAUI/Sources/PIASwiftUI/LoadingView.swift:44-46

The @State property isRotating is modified in onAppear which can cause timing issues.

Recommendation: Use onAppear with explicit animation:

.onAppear {
    withAnimation(.linear(duration: 1.0).repeatForever(autoreverses: false)) {
        isRotating = true
    }
}

4. Potential Circular Dependency Risk

Files: LocalPackages/PIAUI/Package.swift:26 and LocalPackages/PIALibrary/Package.swift

The PIAUI package depends on PIALibrary, but this creates potential for circular dependencies if PIALibrary ever needs to import UI components.

Recommendation: Consider making PIALibrary completely UI-agnostic and have the main app handle UI-library coordination.

🔧 Technical Issues

5. Socket Resource Leaks in Pinger

Files: Pinger.swift:109-117

Setting socket timeouts but not checking if setsockopt calls succeed could lead to unexpected behavior.

Recommendation: Add error checking:

if setsockopt(descriptor, SOL_SOCKET, SO_SNDTIMEO, &tv, socklen_t(MemoryLayout<timeval>.size)) != 0 {
    return nil
}

6. Thread Safety Concerns

File: VPNIPAddress.swift:28

The function VPNIPAddressFromInterfaces() accesses system network interfaces without thread safety considerations. If called from multiple threads, it could cause issues.

Recommendation: Add thread safety documentation or synchronization.

7. Missing Input Validation

File: UIViewAutolayout.swift:27

The addConstaintsToSuperview function doesn't validate that the view has a superview before setting up constraints (though it does guard, the function name suggests it will always work).

Recommendation: Consider returning a Bool to indicate success/failure.

📊 Test Coverage Gaps

Missing Tests:

1. Pinger classes: No tests for TCP/UDP ping functionality
2. VPNIPAddress: No tests for network interface enumeration
3. UIViewAutolayout: No tests for constraint setup
4. LoadingView: No tests for SwiftUI component behavior

Test Improvements Needed:

File: DataCompressionTests.swift

The tests are comprehensive but missing error case testing:

• Test malformed data compression
• Test compression failure scenarios
• Test memory pressure scenarios

🛡️ Security Considerations

1. Network Interface Access: VPNIPAddress.swift accesses system network interfaces which could potentially leak sensitive information.
2. Socket Operations: Direct socket manipulation in Pinger.swift needs careful validation.
3. Memory Safety: The compression code handles raw memory correctly with proper defer blocks.

📐 Performance Considerations

1. Compression Block Size: The 16KB block size in compression is reasonable but might need tuning for mobile.
2. Animation Performance: LoadingView animation should perform well but consider testing on older devices.

⚡ Immediate Action Items

1. Fix the Pinger security issue (Critical)
2. Update Swift typed throws syntax for compatibility
3. Add missing test coverage for new Swift utilities
4. Document thread safety requirements for network functions

📝 Minor Issues

• Typo in UIViewAutolayout.swift:27: "addConstaintsToSuperview" should be "addConstraintsToSuperview"
• Some UI components could benefit from accessibility improvements
• Consider using os_log instead of print statements for better debugging

---

Overall Assessment: This is a well-structured refactoring that improves the codebase architecture. The Objective-C to Swift conversion is mostly excellent, but there are some critical security and compatibility issues that need immediate attention before merging.

---

[kp-pietro-messineo]

Why did we get rid of them?

[kp-diego-trevisan]

They are not used in the project at all, only the Regular and Medium variations are :)

[kp-pietro-messineo]

LGTM. Just please double check the other comment about the removal of the font if it wasn't a mistake