Skip to content

KM-13513 Sort out app dependencies - #225

Merged
kp-diego-trevisan merged 2 commits into
masterfrom
KM-13513-pia-ios-sort-out-app-dependencies
Jan 5, 2026
Merged

KM-13513 Sort out app dependencies#225
kp-diego-trevisan merged 2 commits into
masterfrom
KM-13513-pia-ios-sort-out-app-dependencies

Conversation

@kp-diego-trevisan

Copy link
Copy Markdown
Collaborator

Summary

  • Move SwiftEntryKit and PopupDialog dependencies from PIALibrary to app project (only needed there)
  • Remove tweetnacl-swiftwrap since it's already a dependency for PIAWireguard
  • Bug 1
  • Bug 2
  • Feature 1
  • Feature 2
  • Breaking changes

Test plan (required)

Code formatting

Closing issues

@kp-diego-trevisan
kp-diego-trevisan force-pushed the KM-13513-pia-ios-sort-out-app-dependencies branch from 12a7258 to c0b2688 Compare December 23, 2025 11:36
@claude

claude Bot commented Dec 23, 2025

Copy link
Copy Markdown

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


Code Review Complete ✅

I've thoroughly analyzed this PR for dependency restructuring. Here's my comprehensive review:


📋 Summary

This PR successfully moves UI-specific dependencies (SwiftEntryKit and PopupDialog) from the shared PIALibrary to the app targets, and removes the redundant tweetnacl-swiftwrap dependency. This is a well-structured dependency optimization.


Positive Aspects

1. Proper Dependency Separation

  • UI dependencies (SwiftEntryKit, PopupDialog) correctly moved from shared library to app-specific targets
  • Maintains clean architecture by keeping UI components out of the core PIALibrary
  • Follows CLAUDE.md principle of having PIALibrary as core logic and separate UI components

2. Dependency Deduplication

  • Removal of tweetnacl-swiftwrap is correct since it's already included via PIAWireguard dependency
  • Package.resolved shows the dependency is still available through the WireGuard package

3. Multi-Platform Support

  • Dependencies properly added to both iOS (PIA VPN) and tvOS (PIA VPN-tvOS) targets
  • Consistent dependency management across platforms

🔍 Code Quality Assessment

Architecture & Best Practices: ✅

  • Follows Swift Package Manager best practices
  • Maintains proper separation of concerns
  • Aligns with repository's architecture guidelines

Performance: ✅

  • Reduces PIALibrary's dependency footprint
  • UI dependencies only loaded where needed (app targets)
  • Should improve build times for PIALibrary tests

Security: ✅

  • No security concerns identified
  • Dependencies use established, well-maintained packages
  • No credential or sensitive data exposure

📝 Specific File Analysis

LocalPackages/PIALibrary/Package.swift

  • ✅ Clean removal of UI dependencies
  • ✅ Core dependencies remain intact
  • ✅ No breaking changes to PIALibrary API

PIA VPN.xcodeproj/project.pbxproj:495-498

  • ✅ SwiftEntryKit and PopupDialog properly added to iOS target frameworks
  • ✅ Consistent framework linking configuration

PIA VPN.xcodeproj/project.pbxproj:747-748

  • ✅ tvOS target correctly includes both dependencies
  • ✅ Proper package references configured

Package.resolved

  • tweetnacl-swiftwrap removed as expected
  • ✅ SwiftEntryKit and PopupDialog versions pinned correctly
  • ✅ All other dependencies maintained

🚨 Potential Issues

Minor Concerns:

  1. GitHub Workflow Changes: The retry logic for iOS platform download looks good, but ensure this doesn't mask underlying infrastructure issues

  2. Missing Test Verification: While the dependency restructuring looks correct, I cannot verify that all UI components still work properly without running tests


🔧 Recommendations

Immediate:

  1. ✅ The PR is ready to merge - no blocking issues found
  2. Ensure CI builds pass for both iOS and tvOS targets
  3. Consider running a quick smoke test on features using SwiftEntryKit/PopupDialog

Future Improvements:

  1. Consider documenting this dependency architecture decision in CLAUDE.md
  2. Add dependency usage guidelines for future contributors

🎯 Test Coverage Assessment

Cannot verify without running tests, but recommend checking:

  • Features using SwiftEntryKit (notifications, overlays)
  • Features using PopupDialog (confirmation dialogs, alerts)
  • PIALibrary builds independently without UI dependencies
  • Both iOS and tvOS apps build successfully

Final Verdict

APPROVED - This is a well-executed dependency refactoring that:

  • ✅ Improves architecture by properly separating UI and core dependencies
  • ✅ Reduces PIALibrary complexity and build dependencies
  • ✅ Maintains multi-platform compatibility
  • ✅ Follows repository best practices and guidelines
  • ✅ No security, memory management, or threading concerns

The changes are minimal, focused, and demonstrate good understanding of Swift Package Manager and Xcode project structure.


@kp-diego-trevisan
kp-diego-trevisan force-pushed the KM-13513-pia-ios-sort-out-app-dependencies branch from c0b2688 to 4323045 Compare December 23, 2025 11:45

@kp-pietro-messineo kp-pietro-messineo left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@kp-diego-trevisan
kp-diego-trevisan merged commit f11a94e into master Jan 5, 2026
3 checks passed
@kp-diego-trevisan
kp-diego-trevisan deleted the KM-13513-pia-ios-sort-out-app-dependencies branch January 5, 2026 10:57
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.

2 participants