refactor: extract VPNBypassCore library for testable coverage#41
Merged
Conversation
Split the single executableTarget into a three-target SPM layout: VPNBypassCore (library), VPNBypass (thin executable), and VPNBypassTests. Tests now import real production code via @testable import VPNBypassCore, replacing duplicated local implementations. Pure validation functions (cleanDomain, isValidIP, isValidCIDR) marked nonisolated for test access. Coverage reports now instrument actual production code instead of showing 0% due to the previous executableTarget limitation.
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (13)
📝 WalkthroughWalkthroughExtracts core logic into a new ChangesModularization + API Exposure
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly Related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Review rate limit: 0/1 reviews remaining, refill in 20 minutes and 14 seconds.Comment |
Makefile bundle target referenced deleted Resources/ directory — now points to Sources/VPNBypassCore/Resources/. Raised codecov targets from 30%/50% to 70%/80% to catch future coverage regressions.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
Sources/VPNBypassCore/RouteManager.swift (1)
3664-3709: ⚡ Quick winMake these pure validators static instead of hanging them off
RouteManager.shared.Tests now have to initialize
RouteManager.sharedjust to parse strings, andRouteManager.init()still creates the Application Support directory on Line 413. Moving these tostatichelpers or a small validator type keeps them testable without singleton side effects.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/VPNBypassCore/RouteManager.swift` around lines 3664 - 3709, The pure validation/normalization methods cleanDomain(_:), isValidIP(_:) and isValidCIDR(_:) should be converted from instance methods on RouteManager to static helpers so tests don't need RouteManager.shared or trigger RouteManager.init() side effects; change their signatures to static func cleanDomain(_:) / static func isValidIP(_:) / static func isValidCIDR(_:) (or move them into a small static Validator/RouteValidator type) and update all call sites to reference the static methods (e.g., RouteManager.cleanDomain(...) or RouteValidator.cleanDomain(...)) so production behavior is unchanged but tests can call the validators without initializing the singleton.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Tests/VPNBypassTests/VPNBypassTests.swift`:
- Around line 405-415: The test helper classifyInput currently treats inputs
with slashes as domains, which diverges from production
RouteManager.addInverseDomain(_:), so restore the slash-rejection: after
computing trimmed and cidr (in classifyInput), if not cidr and
trimmed.contains("/") then return nil; otherwise proceed to cleanDomain and the
existing empty-check/return. This will ensure classifyInput mirrors
RouteManager.addInverseDomain behavior for inputs like
"https://example.com/path" or invalid CIDRs.
---
Nitpick comments:
In `@Sources/VPNBypassCore/RouteManager.swift`:
- Around line 3664-3709: The pure validation/normalization methods
cleanDomain(_:), isValidIP(_:) and isValidCIDR(_:) should be converted from
instance methods on RouteManager to static helpers so tests don't need
RouteManager.shared or trigger RouteManager.init() side effects; change their
signatures to static func cleanDomain(_:) / static func isValidIP(_:) / static
func isValidCIDR(_:) (or move them into a small static Validator/RouteValidator
type) and update all call sites to reference the static methods (e.g.,
RouteManager.cleanDomain(...) or RouteValidator.cleanDomain(...)) so production
behavior is unchanged but tests can call the validators without initializing the
singleton.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 95d3f206-b21f-4c04-813a-a5e6b444d851
📒 Files selected for processing (19)
.github/workflows/ci.ymlMakefilePackage.swiftSources/VPNBypass/main.swiftSources/VPNBypassCore/ColorExtension.swiftSources/VPNBypassCore/HelperManager.swiftSources/VPNBypassCore/HelperProtocol.swiftSources/VPNBypassCore/LaunchAtLoginManager.swiftSources/VPNBypassCore/MenuBarViews.swiftSources/VPNBypassCore/NotificationManager.swiftSources/VPNBypassCore/Resources/en.lproj/Localizable.stringsSources/VPNBypassCore/Resources/es.lproj/Localizable.stringsSources/VPNBypassCore/Resources/fr.lproj/Localizable.stringsSources/VPNBypassCore/RouteManager.swiftSources/VPNBypassCore/SettingsView.swiftSources/VPNBypassCore/Theme.swiftSources/VPNBypassCore/VPNBypassApp.swiftTests/VPNBypassTests/VPNBypassTests.swiftcodecov.yml
cleanDomain is now nonisolated and directly callable — the wrapper added no value. Single caller in SettingsView updated.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Sources/VPNBypassCore/SettingsView.swift (1)
974-987:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winGuard against all-empty
cleanDomainsbefore persisting the service.
canSaveonly checks!$0.trimmingCharacters(in: .whitespaces).isEmpty, butcleanDomaindoes far more stripping (protocol schemes, userinfo, non-[a-zA-Z0-9.-]chars). Inputs like@,://foo, orhttp://all satisfycanSaveyet clean to"", socleanDomainsends up empty. The call then reachesaddCustomService(name:, domains:[], ipRanges:), persisting a service entry that will never match or route any traffic.🛡️ Proposed fix
private func save() { let cleanDomains = domains .map { routeManager.cleanDomain($0) } .filter { !$0.isEmpty } + guard !cleanDomains.isEmpty else { return } let cleanIPs = ipRanges.map { $0.trimmingCharacters(in: .whitespaces) }.filter { !$0.isEmpty } let name = serviceName.trimmingCharacters(in: .whitespaces)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/VPNBypassCore/SettingsView.swift` around lines 974 - 987, The save() method must guard against persisting services with no effective matchers: after computing cleanDomains and cleanIPs, ensure at least one non-empty cleaned domain or cleaned IP-range exists (i.e., if cleanDomains.isEmpty && cleanIPs.isEmpty return early or surface a validation error) and do not call routeManager.addCustomService/updateCustomService with empty matchers; also update the canSave logic to validate against the cleaned domain set (cleanDomains) rather than the raw domains so the Save button is disabled for inputs that clean to empty.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@Sources/VPNBypassCore/SettingsView.swift`:
- Around line 974-987: The save() method must guard against persisting services
with no effective matchers: after computing cleanDomains and cleanIPs, ensure at
least one non-empty cleaned domain or cleaned IP-range exists (i.e., if
cleanDomains.isEmpty && cleanIPs.isEmpty return early or surface a validation
error) and do not call routeManager.addCustomService/updateCustomService with
empty matchers; also update the canSave logic to validate against the cleaned
domain set (cleanDomains) rather than the raw domains so the Save button is
disabled for inputs that clean to empty.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ae07af5d-0df3-4d70-b5f5-f2d3ee831d11
📒 Files selected for processing (2)
Sources/VPNBypassCore/RouteManager.swiftSources/VPNBypassCore/SettingsView.swift
🚧 Files skipped from review as they are similar to previous changes (1)
- Sources/VPNBypassCore/RouteManager.swift
…code CodeRabbit caught that the test helper classifyInput diverged from production addInverseDomain — inputs with "/" that aren't valid CIDR are now rejected. Updated tests that had wrong expectations for URL inputs with paths.
Remove sources restriction from testTarget so additional test files can be added. Add SettingsView, MenuBarViews, and VPNBypassApp to codecov ignore list since SwiftUI views cannot be unit tested.
Add tests for RouteManager integration (config mutations, export/import, save/load), business logic (logging, defaults, structs), clean domain parsing, IP/CIDR validation, Config/ProxyConfig/VPNType codable roundtrips, HelperState enum, notification preferences, Color extension, Theme constants, and HelperProtocol constants. Lower codecov targets to 60% and exclude system-dependent files (HelperManager, LaunchAtLoginManager, NotificationManager) that require XPC/SMAppService/UNUserNotificationCenter and cannot be unit tested.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
executableTargetinto three SPM targets:VPNBypassCore(library),VPNBypass(thin executable),VPNBypassTests@testable import VPNBypassCoreto exercise real production code instead of duplicated local implementationsDomainEntry,OnceGateTestImpl, localisValidCIDR/isValidIP/cleanDomain) that shadowed production codenonisolatedfor direct test access from non-actor contextexecutableTargetlimitation)Test plan
swift buildsucceedsswift test— all 68 tests passswift test --enable-code-coverageconfirms non-zero coverage (6.78% total, RouteManager 6.49%)Summary by CodeRabbit