Add proxy URL settings UI and config handling#188
Add proxy URL settings UI and config handling#188chelishchev wants to merge 1 commit intoautomazeio:mainfrom
Conversation
📝 WalkthroughWalkthroughAdds proxy URL configuration and persistence (UI + ServerManager), merges proxy settings into a generated YAML config when needed, updates server restart/hot-reload behavior when config changes, and renames an NSWindowDelegate callback from Changes
Sequence Diagram(s)sequenceDiagram
participant User as User
participant SettingsView as SettingsView
participant ServerManager as ServerManager
participant UserDefaults as UserDefaults
participant FileSystem as FileSystem/Disk
participant Server as Server
User->>SettingsView: Enter proxy URL & click Apply
SettingsView->>ServerManager: setProxyURL(url)
ServerManager->>UserDefaults: save proxyURL
ServerManager->>ServerManager: applyConfigUpdate()
ServerManager->>FileSystem: write merged-config.yaml (with proxy override)
alt config path changed while running
ServerManager->>Server: restart with new config
else config applied for next start
ServerManager->>ServerManager: update activeConfigPath for next run
end
Note over ServerManager,UserDefaults: On launch or SettingsView appear
ServerManager->>UserDefaults: load proxyURL
SettingsView->>ServerManager: read proxyURL on appear
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Important Action Needed: IP Allowlist UpdateIf your organization protects your Git platform with IP whitelisting, please add the new CodeRabbit IP address to your allowlist:
Failure to add the new IP will result in interrupted reviews. 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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/Sources/ServerManager.swift`:
- Around line 59-64: The code is persisting proxy credentials in plaintext by
saving proxyURL in UserDefaults (see the proxyURL property didSet that calls
UserDefaults.standard.set); stop storing URLs that may contain
usernames/passwords in UserDefaults—either (a) validate proxyURL in the setter
to reject or strip embedded credentials and prompt the user to enter credentials
separately, or (b) move credential storage to the macOS Keychain (use a Keychain
helper/KeychainAccess and store only the credential blob keyed to the proxy
identifier while keeping the non-sensitive host/port in proxyURL). Also update
any other places that call UserDefaults.standard.set for connection info
(referenced around lines 98-100) to follow the same approach. Ensure the
proxyURL didSet no longer writes credentials to UserDefaults and that credential
saving/retrieval uses the Keychain API or prompts the user.
In `@src/Sources/SettingsView.swift`:
- Around line 249-290: The DisclosureGroup labeled "Network" is being toggled
both by its built-in binding isNetworkExpanded and by an extra .onTapGesture
that calls isNetworkExpanded.toggle(), causing double-toggles; remove the
explicit .onTapGesture block from the label (the HStack label containing
Text("Network") / Text("Configured")) so the DisclosureGroup uses only its
isExpanded: $isNetworkExpanded binding to control expansion.
| @Published var proxyURL: String = "" { | ||
| didSet { | ||
| UserDefaults.standard.set(proxyURL, forKey: "proxyURL") | ||
| } | ||
| } | ||
| private var activeConfigPath: String = "" |
There was a problem hiding this comment.
Avoid persisting proxy credentials in UserDefaults.
Proxy URLs may embed usernames/passwords; storing them in UserDefaults is plaintext. Consider Keychain storage or explicitly disallow credentials in the URL and prompt the user accordingly.
Also applies to: 98-100
🤖 Prompt for AI Agents
In `@src/Sources/ServerManager.swift` around lines 59 - 64, The code is persisting
proxy credentials in plaintext by saving proxyURL in UserDefaults (see the
proxyURL property didSet that calls UserDefaults.standard.set); stop storing
URLs that may contain usernames/passwords in UserDefaults—either (a) validate
proxyURL in the setter to reject or strip embedded credentials and prompt the
user to enter credentials separately, or (b) move credential storage to the
macOS Keychain (use a Keychain helper/KeychainAccess and store only the
credential blob keyed to the proxy identifier while keeping the non-sensitive
host/port in proxyURL). Also update any other places that call
UserDefaults.standard.set for connection info (referenced around lines 98-100)
to follow the same approach. Ensure the proxyURL didSet no longer writes
credentials to UserDefaults and that credential saving/retrieval uses the
Keychain API or prompts the user.
| DisclosureGroup(isExpanded: $isNetworkExpanded) { | ||
| VStack(alignment: .leading, spacing: 8) { | ||
| HStack(alignment: .center, spacing: 8) { | ||
| Text("Proxy URL") | ||
| .frame(width: 80, alignment: .leading) | ||
| TextField("", text: $proxyURLInput) | ||
| .textFieldStyle(.roundedBorder) | ||
| .frame(maxWidth: .infinity) | ||
| .multilineTextAlignment(.leading) | ||
| } | ||
| HStack(spacing: 8) { | ||
| Button("Apply") { | ||
| applyProxyURL() | ||
| } | ||
| .disabled(trimmedProxyURLInput == serverManager.proxyURL) | ||
| Button("Clear") { | ||
| clearProxyURL() | ||
| } | ||
| .disabled(proxyURLInput.isEmpty && serverManager.proxyURL.isEmpty) | ||
| Spacer() | ||
| } | ||
| Text("Leave empty to disable. Example: http://proxy.local:8080 or socks5://127.0.0.1:1080") | ||
| .font(.caption) | ||
| .foregroundColor(.secondary) | ||
| } | ||
| .frame(maxWidth: .infinity, alignment: .leading) | ||
| .padding(.top, 6) | ||
| } label: { | ||
| HStack { | ||
| Text("Network") | ||
| Spacer() | ||
| if !serverManager.proxyURL.isEmpty { | ||
| Text("Configured") | ||
| .font(.caption) | ||
| .foregroundColor(.secondary) | ||
| } | ||
| } | ||
| .contentShape(Rectangle()) | ||
| .onTapGesture { | ||
| isNetworkExpanded.toggle() | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
SwiftUI DisclosureGroup isExpanded binding onTapGesture double toggle conflict
💡 Result:
When you pass isExpanded: $expanded and also do expanded.toggle() in an onTapGesture, a single tap can flip the binding twice (once by DisclosureGroup’s built‑in tap handling, once by your gesture), so it looks like it “does nothing” (open then immediately close).
Fix options
- Don’t toggle in the tap gesture; observe instead
UseDisclosureGroup’s own interaction to changeexpanded, and react viaonChange:
DisclosureGroup("Title", isExpanded: $expanded) {
Text("Content")
}
.onChange(of: expanded) { newValue in
// do side effects here
}(Using the binding externally to control expansion is the intended model.) [1]
-
If you want full control of tapping, use a custom
DisclosureGroupStyle(iOS 16+)
Renderconfiguration.labelyourself and toggleconfiguration.isExpandedonly there, avoiding the default toggle behavior. [2] -
If your goal is “bigger tap target”, don’t put the toggle on the label
Instead, make the row tappable and toggle once (typical pattern uses.contentShape(Rectangle())on the container). [3]
Sources: manual control via isExpanded binding [1]; custom DisclosureGroupStyle approach [2]; container tap target pattern using contentShape + single toggle [3].
Remove the manual toggle from the DisclosureGroup label.
Adding onTapGesture with toggle() to a DisclosureGroup label that already has isExpanded: $binding causes the binding to toggle twice on a single tap (once by DisclosureGroup's built-in tap handling, once by the explicit gesture), making the control appear unresponsive. The DisclosureGroup already handles the label tap through the binding; the manual toggle is redundant and breaks that behavior.
Fix
} label: {
HStack {
Text("Network")
Spacer()
if !serverManager.proxyURL.isEmpty {
Text("Configured")
.font(.caption)
.foregroundColor(.secondary)
}
}
.contentShape(Rectangle())
- .onTapGesture {
- isNetworkExpanded.toggle()
- }
}🤖 Prompt for AI Agents
In `@src/Sources/SettingsView.swift` around lines 249 - 290, The DisclosureGroup
labeled "Network" is being toggled both by its built-in binding
isNetworkExpanded and by an extra .onTapGesture that calls
isNetworkExpanded.toggle(), causing double-toggles; remove the explicit
.onTapGesture block from the label (the HStack label containing Text("Network")
/ Text("Configured")) so the DisclosureGroup uses only its isExpanded:
$isNetworkExpanded binding to control expansion.
There was a problem hiding this comment.
Pull request overview
This PR adds network proxy configuration functionality to the VibeProxy application, allowing users to configure HTTP/SOCKS5 proxy settings through the UI. The changes enable proxy URL configuration with proper persistence and automatic server management when proxy settings change.
Changes:
- Added Network settings section in the UI with proxy URL input field, Apply/Clear buttons, and status indicator
- Implemented proxy URL persistence in UserDefaults and config file merging with automatic server restart when needed
- Fixed window delegate warning by changing from
windowDidClosetowindowWillClose
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 10 comments.
| File | Description |
|---|---|
| src/Sources/SettingsView.swift | Adds proxy URL UI components with input validation state and button logic; increases window height to accommodate new section |
| src/Sources/ServerManager.swift | Implements proxy URL storage, config merging with YAML escaping, intelligent server restart/hot-reload logic, and cleanup of merged configs |
| src/Sources/AppDelegate.swift | Fixes window delegate method from windowDidClose to proper windowWillClose |
Comments suppressed due to low confidence (1)
src/Sources/ServerManager.swift:550
- The getConfigPath function is called in multiple places (lines 126, 175, 304) but it can fail silently by returning an empty string when resourcePath is unavailable. When this happens, the calling code checks for empty strings, but this pattern could lead to inconsistent behavior. Consider throwing an error or using a Result type instead of returning empty strings to make error handling more explicit and robust.
func getConfigPath() -> String {
guard let resourcePath = Bundle.main.resourcePath else {
return ""
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| value | ||
| .replacingOccurrences(of: "\\", with: "\\\\") | ||
| .replacingOccurrences(of: "\"", with: "\\\"") | ||
| .replacingOccurrences(of: "\n", with: "\\n") | ||
| .replacingOccurrences(of: "\t", with: "\\t") |
There was a problem hiding this comment.
The YAML escaping implementation is incomplete. While it handles common escape sequences (backslash, double quote, newline, tab), it doesn't handle other special characters that need escaping in YAML double-quoted strings, such as carriage return (\r), bell (\u{07}), and other control characters. Additionally, Unicode escape sequences should be considered. While this may not be critical for typical proxy URLs, it could cause YAML parsing errors if users input URLs with unusual characters.
| value | |
| .replacingOccurrences(of: "\\", with: "\\\\") | |
| .replacingOccurrences(of: "\"", with: "\\\"") | |
| .replacingOccurrences(of: "\n", with: "\\n") | |
| .replacingOccurrences(of: "\t", with: "\\t") | |
| var escaped = "" | |
| escaped.reserveCapacity(value.count) | |
| for scalar in value.unicodeScalars { | |
| switch scalar { | |
| case "\\": | |
| escaped.append("\\\\") | |
| case "\"": | |
| escaped.append("\\\"") | |
| case "\n": | |
| escaped.append("\\n") | |
| case "\r": | |
| escaped.append("\\r") | |
| case "\t": | |
| escaped.append("\\t") | |
| case "\u{0}": | |
| escaped.append("\\0") | |
| case "\u{7}": | |
| escaped.append("\\a") | |
| case "\u{8}": | |
| escaped.append("\\b") | |
| case "\u{B}": | |
| escaped.append("\\v") | |
| case "\u{C}": | |
| escaped.append("\\f") | |
| default: | |
| // Escape other non-printable/control characters using \uXXXX | |
| if scalar.value < 0x20 || (scalar.value >= 0x7F && scalar.value <= 0x9F) { | |
| let codeUnit = scalar.value | |
| let hex = String(format: "%04X", codeUnit) | |
| escaped.append("\\u\(hex)") | |
| } else { | |
| escaped.unicodeScalars.append(scalar) | |
| } | |
| } | |
| } | |
| return escaped |
| private func applyProxyURLOverride(to content: String, proxyURL: String) -> String { | ||
| let escapedURL = escapeYAMLDoubleQuoted(proxyURL) | ||
| let newLine = "proxy-url: \"\(escapedURL)\"" | ||
| var didReplace = false | ||
| let lines = content.split(omittingEmptySubsequences: false, whereSeparator: \.isNewline) | ||
| let updated = lines.map { line -> String in | ||
| let trimmed = String(line).trimmingCharacters(in: .whitespaces) | ||
| if trimmed.hasPrefix("proxy-url:") { | ||
| didReplace = true | ||
| let indent = line.prefix { $0 == " " || $0 == "\t" } | ||
| return "\(indent)\(newLine)" | ||
| } | ||
| return String(line) | ||
| } | ||
| var result = updated.joined(separator: "\n") | ||
| if !didReplace { | ||
| result += "\n\n# Network proxy (auto-added by VibeProxy)\n\(newLine)\n" | ||
| } | ||
| return result |
There was a problem hiding this comment.
The applyProxyURLOverride function modifies lines that start with "proxy-url:" but doesn't preserve comments that might be on the same line. If the bundled config has a line like "proxy-url: "" # comment", this will be replaced with just the proxy-url line without the comment. While this may not be critical, it could remove useful documentation from the config file.
| @Published var proxyURL: String = "" { | ||
| didSet { | ||
| UserDefaults.standard.set(proxyURL, forKey: "proxyURL") | ||
| } |
There was a problem hiding this comment.
The proxyURL property's didSet handler directly writes to UserDefaults synchronously. Since UserDefaults operations can be slow on older systems or when the defaults database is large, this could potentially cause UI lag when the user clicks Apply. Consider making the UserDefaults write asynchronous, though this is a minor performance concern and unlikely to be noticeable in practice.
| let currentPath = activeConfigPath.isEmpty ? newConfigPath : activeConfigPath | ||
| if currentPath != newConfigPath { |
There was a problem hiding this comment.
The logic at line 130 sets currentPath to newConfigPath when activeConfigPath is empty. This means on first config change after server start, if activeConfigPath wasn't properly initialized, the comparison currentPath != newConfigPath will always be false (since both are newConfigPath), preventing a restart even when one might be needed. The activeConfigPath is set in start() at line 222, so this should normally be fine, but if start() fails before reaching line 222, activeConfigPath remains empty and subsequent config changes won't trigger restarts. Consider ensuring activeConfigPath is always properly initialized or add additional validation.
|
|
||
| if !shouldAddProxy && !shouldAddProviders && !shouldAddZai { | ||
| if FileManager.default.fileExists(atPath: mergedConfigPath.path) { | ||
| try? FileManager.default.removeItem(at: mergedConfigPath) |
There was a problem hiding this comment.
The merged config cleanup at lines 587-589 silently ignores errors when removing the file. If the file removal fails (e.g., due to permissions), the code will still return the bundled config path, but the stale merged config will remain on disk. This could lead to confusion if the merged config is accidentally used later. Consider logging the error or handling the failure case more explicitly.
| try? FileManager.default.removeItem(at: mergedConfigPath) | |
| do { | |
| try FileManager.default.removeItem(at: mergedConfigPath) | |
| } catch { | |
| NSLog("Failed to remove merged config at %@: %@", mergedConfigPath.path, String(describing: error)) | |
| } |
| Button("Clear") { | ||
| clearProxyURL() | ||
| } | ||
| .disabled(proxyURLInput.isEmpty && serverManager.proxyURL.isEmpty) |
There was a problem hiding this comment.
The Clear button is disabled when both proxyURLInput.isEmpty and serverManager.proxyURL.isEmpty are true. However, if the user types whitespace-only input, proxyURLInput will not be empty, but the actual saved proxy URL will be empty (due to trimming). In this case, the Clear button will be enabled but clicking it will have no visible effect since setProxyURL will detect no change (empty == empty after trimming). Consider trimming the input before checking isEmpty for better UX consistency.
| .disabled(proxyURLInput.isEmpty && serverManager.proxyURL.isEmpty) | |
| .disabled(trimmedProxyURLInput.isEmpty && serverManager.proxyURL.isEmpty) |
| .onTapGesture { | ||
| isNetworkExpanded.toggle() | ||
| } |
There was a problem hiding this comment.
The DisclosureGroup label has a custom onTapGesture handler that toggles isNetworkExpanded. This creates duplicate tap handling since DisclosureGroup already handles taps to expand/collapse. The custom tap gesture might interfere with the default behavior or create accessibility issues. Consider removing the onTapGesture and letting DisclosureGroup handle the interaction natively, or verify that this doesn't cause double-toggling issues.
| .onTapGesture { | |
| isNetworkExpanded.toggle() | |
| } |
| didSet { | ||
| UserDefaults.standard.set(proxyURL, forKey: "proxyURL") | ||
| } | ||
| } |
There was a problem hiding this comment.
The proxy URL is stored in plain text in UserDefaults (line 61) and written to the config file without encryption. If the proxy URL contains credentials (e.g., http://user:password@proxy.com:8080), these would be stored insecurely. Consider warning users not to include credentials in the proxy URL, or implement secure storage using Keychain for proxy URLs with embedded authentication credentials.
| didSet { | |
| UserDefaults.standard.set(proxyURL, forKey: "proxyURL") | |
| } | |
| } | |
| didSet { | |
| // Avoid persisting proxy URLs that contain embedded credentials | |
| if ServerManager.containsCredentials(in: proxyURL) { | |
| NSLog("Warning: Proxy URL contains embedded credentials and will not be stored in UserDefaults for security reasons.") | |
| return | |
| } | |
| UserDefaults.standard.set(proxyURL, forKey: "proxyURL") | |
| } | |
| } | |
| private static func containsCredentials(in urlString: String) -> Bool { | |
| guard let url = URL(string: urlString), | |
| let components = URLComponents(url: url, resolvingAgainstBaseURL: false) else { | |
| return false | |
| } | |
| let hasUser = components.user?.isEmpty == false | |
| let hasPassword = components.password?.isEmpty == false | |
| return hasUser || hasPassword | |
| } | |
| let outputData = outputPipe.fileHandleForReading.readDataToEndOfFile() | ||
| let errorData = errorPipe.fileHandleForReading.readDataToEndOfFile() | ||
|
|
||
| var output = String(data: outputData ?? Data(), encoding: .utf8) ?? "" | ||
| var output = String(data: outputData, encoding: .utf8) ?? "" | ||
| if output.isEmpty { output = capture.text } | ||
| let error = String(data: errorData ?? Data(), encoding: .utf8) ?? "" | ||
| let error = String(data: errorData, encoding: .utf8) ?? "" |
There was a problem hiding this comment.
The removal of try? when reading pipe data could cause crashes if reading fails. The fileHandleForReading.readDataToEndOfFile() method can throw an error if the file handle is closed or there's an I/O error. The original try? implementation safely handled these cases by returning nil, which was then converted to an empty Data() object. Without error handling, this could crash the authentication flow.
| private func applyProxyURL() { | ||
| serverManager.setProxyURL(proxyURLInput) | ||
| proxyURLInput = serverManager.proxyURL | ||
| } |
There was a problem hiding this comment.
The proxy URL input lacks validation. Users can enter invalid URLs (e.g., "not-a-url", "ftp://invalid", etc.) that would be accepted and written to the config file. Consider adding URL validation to ensure the proxy URL follows expected formats (http://, https://, socks5://) before allowing the user to apply it. This would prevent configuration errors and improve user experience.
chelishchev
force-pushed
the
proxy-config-ui
branch
from
9cca03f to
ca540b5
Compare
1 participant
Summary
Summary by CodeRabbit
Release Notes
New Features
Refactor