Skip to content

KM-13709 Fix large widget not activating the connect button as the small one - #237

Merged
kp-diego-trevisan merged 3 commits into
masterfrom
KM-13709-large-widget-does-not-activate-the-connect-button-as-the-small-one-does
Jan 16, 2026
Merged

KM-13709 Fix large widget not activating the connect button as the small one#237
kp-diego-trevisan merged 3 commits into
masterfrom
KM-13709-large-widget-does-not-activate-the-connect-button-as-the-small-one-does

Conversation

@kp-diego-trevisan

@kp-diego-trevisan kp-diego-trevisan commented Jan 13, 2026

Copy link
Copy Markdown
Collaborator

Summary

  • Fix URL called by both widgets so they trigger the connection toggle
  • Fix extra margins added by iOS
  • Improvement: move AppConstants and UserInterface to a shared module (PIALibrary) so they can be referenced from all targets (they are referenced from Widgets target)

@claude

claude Bot commented Jan 13, 2026

Copy link
Copy Markdown

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


Code Review Complete ✅

PR Summary: KM-13709 Fix large widget not activating connect button + architectural improvements

Todo List:

  • Read and understand the PR changes
  • Analyze architectural changes (AppConstants/UserInterface move to PIALibrary)
  • Review widget implementation fixes
  • Check for security concerns in VPN application context
  • Evaluate memory management and threading
  • Assess performance implications
  • Provide comprehensive feedback with specific suggestions

✅ Strengths

Architecture Improvements

  1. Excellent code organization: Moving AppConstants and UserInterface to PIALibrary enables proper code sharing between main app and widget targets
  2. Clean separation of concerns: App-specific constants remain in main app extensions while shared constants are centralized
  3. Proper dependency management: Widget can now access shared constants without duplicating code

Widget Fix Implementation

  1. Correct URL scheme: Both widgets now use AppConstants.Widget.connect ("piavpn:connect") ensuring consistent behavior
  2. UI improvements: Removed unnecessary @ViewBuilder attribute in PIAWidgetView.swift:40 (proper cleanup)

⚠️ Areas for Improvement

Security Concerns

  1. AppConstants+CustomServers.swift:28-66: Custom server parsing lacks input validation:

    // Current implementation is vulnerable to malformed input
    let tokens = line.components(separatedBy: ":")
    guard tokens.count == 6 else { continue } // Good
    
    // Missing validation for:
    // - Empty strings
    // - Malicious hostname/address values
    // - Invalid port ranges (should validate udpPort/tcpPort > 0 && < 65536)

    Recommendation: Add comprehensive input validation and sanitization

  2. Missing server validation: The custom server creation doesn't validate hostnames or IP addresses, potentially allowing malicious entries

Code Quality Issues

  1. PIAWidgetPreview.swift:13-37: Preview data should use realistic values:

    // Current: Uses hardcoded test values
    // Better: Use constants or factory methods for consistent test data
  2. Error handling in AppConstants.swift:86-95: The staging endpoint loading silently fails:

    // Consider adding logging for debugging staging endpoint issues
    guard let content = try? String(contentsOfFile: path) else {
        // Should log the error for debugging
        return nil
    }
  3. Memory considerations: Widget extensions have limited memory - the custom servers array is loaded once but could be large. Consider lazy loading or size limits.

Threading & Performance

  1. WidgetUserDefaultsDatasource.swift: UserDefaults access is synchronous and could block widget rendering on slow devices
  2. Custom server parsing: File I/O happens on main thread during app launch - should consider background processing for large server lists

Test Coverage

⚠️ Critical Gap: No test coverage identified for:

  • Widget URL handling and deep linking
  • Custom server parsing logic
  • AppConstants migration from main app to PIALibrary
  • Widget-specific functionality

📋 Specific Recommendations

High Priority

  1. Add input validation to AppConstants+CustomServers.swift:40-54:

    guard !name.isEmpty, !country.isEmpty, !hostname.isEmpty else { continue }
    guard udpPort > 0 && udpPort < 65536 else { continue }
    // Add hostname/IP validation
  2. Add tests for widget URL schemes and custom server parsing

  3. Consider logging in staging endpoint loading for debugging

Medium Priority

  1. Extract magic numbers: PIAWidgetView.swift:42-43 hardcoded sizes should be constants
  2. Add documentation for the new shared module structure
  3. Consider async loading for custom servers if file becomes large

Low Priority

  1. Consistent preview data in PIAWidgetPreview.swift
  2. Add error boundary around custom server loading

🏗️ Architecture Assessment

The architectural changes are well-executed and follow iOS best practices:

  • ✅ Proper Swift Package Manager usage
  • ✅ Clean module boundaries
  • ✅ Consistent with CLAUDE.md guidelines
  • ✅ Maintains backward compatibility

🔒 Security Assessment

Overall: Acceptable for VPN application with noted improvements needed

  • ✅ No hardcoded credentials exposed
  • ✅ Proper app group usage for IPC
  • ⚠️ Custom server parsing needs input validation
  • ✅ Widget URLs use proper scheme

Final Verdict: APPROVE with Recommendations

This PR successfully fixes the widget connectivity issue and makes valuable architectural improvements. The security concerns are manageable and should be addressed in follow-up work rather than blocking this fix.


@kp-diego-trevisan
kp-diego-trevisan force-pushed the KM-13709-large-widget-does-not-activate-the-connect-button-as-the-small-one-does branch from f1cbd58 to 8889b1b Compare January 13, 2026 13:12
@kp-diego-trevisan
kp-diego-trevisan force-pushed the KM-13709-large-widget-does-not-activate-the-connect-button-as-the-small-one-does branch from 53c4691 to ed2ae01 Compare January 16, 2026 07:44
@kp-diego-trevisan
kp-diego-trevisan merged commit ef815aa into master Jan 16, 2026
3 of 4 checks passed
@kp-diego-trevisan
kp-diego-trevisan deleted the KM-13709-large-widget-does-not-activate-the-connect-button-as-the-small-one-does branch January 16, 2026 08:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants