chore: harden codex and swift setup#4
Conversation
|
Important Review skippedToo many files! This PR contains 222 files, which is 72 over the limit of 150. To get a review, narrow the scope: ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (33)
📒 Files selected for processing (222)
You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ 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. Comment |
![codereviewbot-ai[bot]](https://avatars.githubusercontent.com/in/309638?s=60&v=4){kind=small}
| if ! command -v swiftc >/dev/null 2>&1; then echo "swiftc not found" >&2; exit 1; fi | ||
|
|
||
| echo "[browser] building $APP" | ||
| rm -rf "$APP" |
There was a problem hiding this comment.
Potential Data Loss Risk
The command rm -rf "$APP" will recursively delete the directory specified by $APP. If $APP is unset or set to an unintended value, this could result in accidental deletion of important files or directories. Recommendation: Add a sanity check to ensure $APP is set and points to the expected application bundle before performing the deletion. For example:
if [[ -z "$APP" || "$APP" == "/" ]]; then
echo "[ERROR] APP variable is not set correctly. Aborting." >&2
exit 1
fi| cp "$HERE/Info.plist" "$APP/Contents/Info.plist" | ||
| printf 'APPL????' > "$APP/Contents/PkgInfo" | ||
| if command -v codesign >/dev/null 2>&1; then | ||
| codesign --force --sign - --deep "$APP" 2>/dev/null || echo "[browser] codesign skipped" |
There was a problem hiding this comment.
Error Suppression in Codesign
The codesign command redirects all errors to /dev/null and always prints '[browser] codesign skipped' on failure. This can mask genuine signing errors, making troubleshooting difficult. Recommendation: Remove the error redirection and provide more informative error handling. For example:
if ! codesign --force --sign - --deep "$APP"; then
echo "[browser] codesign failed" >&2
fi| MainActor.assumeIsolated { | ||
| runWebViewCompanion(WebViewCompanionConfig( | ||
| title: "Detour Browser", | ||
| initialURL: detourReactURL(view: "browser"), | ||
| frameAutosaveName: "DetourBrowserWindow", | ||
| defaultSize: CGSize(width: 1280, height: 820), | ||
| )) | ||
| } |
There was a problem hiding this comment.
Potential misuse of MainActor.assumeIsolated:
The use of MainActor.assumeIsolated is non-standard for ensuring main-thread execution. Typically, MainActor.run { ... } or DispatchQueue.main.async { ... } is used. Misuse may lead to thread safety issues or undefined behavior.
Recommendation:
Replace with:
MainActor.run {
runWebViewCompanion(...)
}or use DispatchQueue.main.async if appropriate.
| runWebViewCompanion(WebViewCompanionConfig( | ||
| title: "Detour Browser", | ||
| initialURL: detourReactURL(view: "browser"), | ||
| frameAutosaveName: "DetourBrowserWindow", | ||
| defaultSize: CGSize(width: 1280, height: 820), | ||
| )) |
There was a problem hiding this comment.
Lack of error handling for initialization:
The call to runWebViewCompanion does not include any error handling or validation. If initialization fails, the failure will be silent, potentially leading to a non-responsive UI or application crash.
Recommendation:
Wrap the initialization in a do-catch block or check for errors returned by runWebViewCompanion (if it throws or returns a result), and handle them appropriately.
| if command -v codesign >/dev/null 2>&1; then | ||
| codesign --force --sign - --deep "$APP" 2>/dev/null || echo "[chat] codesign skipped" | ||
| fi |
There was a problem hiding this comment.
The codesign step suppresses all error output and always prints a generic message ('codesign skipped') if signing fails. This can obscure genuine signing issues and make debugging difficult.
Recommendation: Capture and print the actual error output from codesign to aid in troubleshooting. For example:
if command -v codesign >/dev/null 2>&1; then
if ! codesign --force --sign - --deep "$APP"; then
echo "[chat] codesign failed" >&2
exit 1
fi
fi| func relativeTimeAgo() -> String { | ||
| let delta = Date().timeIntervalSince1970 - (self / 1000) | ||
| if delta < 60 { return "just now" } | ||
| if delta < 3600 { return "\(Int(delta / 60))m ago" } | ||
| if delta < 86400 { return "\(Int(delta / 3600))h ago" } | ||
| return "\(Int(delta / 86400))d ago" | ||
| } |
There was a problem hiding this comment.
Edge Case: Negative or Future Timestamps in relativeTimeAgo
The relativeTimeAgo() extension assumes the Double is a valid Unix-ms timestamp. If the value is negative or in the future, the function may return misleading results (e.g., negative minutes or days ago).
Recommendation:
Add handling for negative or future timestamps to ensure the output is always meaningful.
let delta = Date().timeIntervalSince1970 - (self / 1000)
if delta < 0 { return "in the future" }
if delta < 60 { return "just now" }
// ...| func localAI(tier: String, action: String, preset: String? = nil) async { | ||
| var req = URLRequest(url: baseURL.appendingPathComponent("api/local-ai/\(tier)/\(action)")) | ||
| req.httpMethod = "POST" | ||
| req.addValue("application/json", forHTTPHeaderField: "content-type") | ||
| if let preset, !preset.isEmpty { | ||
| req.httpBody = try? JSONSerialization.data(withJSONObject: ["preset": preset]) | ||
| } else { | ||
| req.httpBody = "{}".data(using: .utf8) | ||
| } | ||
| _ = try? await URLSession.shared.data(for: req) | ||
| await MainActor.run { self.poll() } | ||
| } |
There was a problem hiding this comment.
Silent Error Handling in localAI
The localAI method ignores errors from the POST request by using try? and does not surface any failure to the caller or log the error. This can result in silent failures that are difficult to diagnose.
Recommendation:
- Consider capturing and logging errors, or returning a
Bool/Resultto indicate success or failure. For example:
do {
_ = try await URLSession.shared.data(for: req)
} catch {
NSLog("[DetourClient] localAI failed: \(error.localizedDescription)")
}| func getJSON<T: Decodable>(_ path: String, query: [String: String] = [:], as: T.Type) async -> T? { | ||
| var components = URLComponents(url: baseURL.appendingPathComponent(path), resolvingAgainstBaseURL: false)! | ||
| if !query.isEmpty { | ||
| components.queryItems = query.map { URLQueryItem(name: $0.key, value: $0.value) } | ||
| } | ||
| guard let url = components.url else { return nil } | ||
| var req = URLRequest(url: url, timeoutInterval: 5.0) | ||
| req.httpMethod = "GET" | ||
| do { | ||
| let (data, _) = try await URLSession.shared.data(for: req) | ||
| return try JSONDecoder().decode(T.self, from: data) | ||
| } catch { | ||
| return nil | ||
| } | ||
| } | ||
|
|
||
| /// Eval-token-aware GET — endpoints under /api/eval/* require a | ||
| /// bearer token from .env. The companion reads it once at startup | ||
| /// and reuses it. Returns nil if the token isn't available. | ||
| private let evalToken: String? = readEvalToken() | ||
|
|
||
| func getEvalJSON<T: Decodable>(_ path: String, query: [String: String] = [:], as: T.Type) async -> T? { | ||
| guard let token = evalToken else { return nil } | ||
| var components = URLComponents(url: baseURL.appendingPathComponent(path), resolvingAgainstBaseURL: false)! | ||
| if !query.isEmpty { | ||
| components.queryItems = query.map { URLQueryItem(name: $0.key, value: $0.value) } | ||
| } | ||
| guard let url = components.url else { return nil } | ||
| var req = URLRequest(url: url, timeoutInterval: 8.0) | ||
| req.httpMethod = "GET" | ||
| req.addValue(token, forHTTPHeaderField: "x-detour-eval-token") | ||
| do { | ||
| let (data, _) = try await URLSession.shared.data(for: req) | ||
| return try JSONDecoder().decode(T.self, from: data) | ||
| } catch { | ||
| return nil | ||
| } | ||
| } |
There was a problem hiding this comment.
Lack of Error Differentiation in getJSON and getEvalJSON
Both getJSON and getEvalJSON return nil for any error, making it impossible for the caller to distinguish between network errors, decoding errors, or missing tokens. This reduces observability and complicates debugging.
Recommendation:
- Consider returning a
Result<T, Error>or at least logging errors internally to provide more insight into failures. For example:
catch {
NSLog("[DetourClient] getJSON failed: \(error.localizedDescription)")
return nil
}| func refresh() { | ||
| loading = true | ||
| error = nil | ||
| let root = mediaRoot | ||
| DispatchQueue.global(qos: .userInitiated).async { [weak self] in | ||
| guard let self else { return } | ||
| var collected: [GalleryItem] = [] | ||
| if let enumerator = FileManager.default.enumerator( | ||
| at: root, | ||
| includingPropertiesForKeys: [.creationDateKey, .isRegularFileKey, .typeIdentifierKey], | ||
| options: [.skipsHiddenFiles], | ||
| ) { | ||
| for case let url as URL in enumerator { | ||
| let vals = try? url.resourceValues(forKeys: [.isRegularFileKey, .creationDateKey, .typeIdentifierKey]) | ||
| if vals?.isRegularFile != true { continue } | ||
| let ext = url.pathExtension.lowercased() | ||
| let kind: GalleryKind | ||
| if ["png", "jpg", "jpeg", "webp", "gif", "heic", "tiff"].contains(ext) { kind = .image } | ||
| else if ["mp4", "mov", "webm", "m4v"].contains(ext) { kind = .video } | ||
| else if ["mp3", "wav", "m4a", "aiff"].contains(ext) { kind = .audio } | ||
| else { kind = .other } | ||
| let createdAt = vals?.creationDate ?? Date.distantPast | ||
| collected.append(GalleryItem( | ||
| id: url.path, url: url, | ||
| createdAt: createdAt, kind: kind, | ||
| )) | ||
| } | ||
| } | ||
| collected.sort { $0.createdAt > $1.createdAt } | ||
| Task { @MainActor in | ||
| if !FileManager.default.fileExists(atPath: root.path) { | ||
| self.error = "~/.detour/media doesn't exist yet — the agent hasn't generated any media." | ||
| } | ||
| self.items = collected | ||
| self.loading = false | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Insufficient error handling for missing media directory
In GalleryViewModel.refresh(), the code attempts to enumerate files in the media directory without first checking if the directory exists. If the directory does not exist, the enumerator will be nil, and the user will see an empty gallery with no clear indication of the problem until after enumeration. This can lead to confusion and a poor user experience.
Recommended solution:
Check for the existence of the media directory before attempting enumeration, and set the error state immediately if it does not exist:
let root = mediaRoot
if !FileManager.default.fileExists(atPath: root.path) {
DispatchQueue.main.async { [weak self] in
self?.error = "~/.detour/media doesn't exist yet — the agent hasn't generated any media."
self?.items = []
self?.loading = false
}
return
}
// Proceed with enumeration...| private func loadThumb() { | ||
| if item.kind == .image { | ||
| DispatchQueue.global(qos: .userInitiated).async { | ||
| let img = NSImage(contentsOf: item.url) | ||
| DispatchQueue.main.async { self.thumb = img } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
No error handling for failed thumbnail loading
In GalleryThumbnail.loadThumb(), if NSImage(contentsOf:) fails (e.g., due to a corrupt or unsupported image file), thumb will be set to nil, resulting in the placeholder icon being shown. However, there is no indication to the user that the image failed to load, nor is there any logging or fallback mechanism. This can make debugging and user experience more difficult.
Recommended solution:
Consider adding error handling or logging for failed image loads, and optionally display a distinct error placeholder:
let img = NSImage(contentsOf: item.url)
if img == nil {
// Optionally log or set a specific error state
}
DispatchQueue.main.async { self.thumb = img }![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 91e45457f8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| function extractLastUserText(args: DynamicPromptArgs): string { | ||
| const recent = args.state?.values?.recentMessages; | ||
| if (typeof recent !== "string" || recent.length === 0) return ""; | ||
| const lines = recent.split("\n"); | ||
| // Walk backwards looking for a "User:" entry (eliza format) and grab the text after the colon. | ||
| for (let i = lines.length - 1; i >= 0; i -= 1) { | ||
| for (let i = lines.length - 1; i -= 1, i >= 0;) { |
There was a problem hiding this comment.
The loop condition decrements i before the first iteration (i -= 1, i >= 0), so it always skips the most recent line and returns "" for single-line histories. This means companion pre-pass can miss the latest user turn (or all user text), reducing persona framing/compression quality exactly when the newest message should drive it.
Useful? React with 👍 / 👎.
| const hook = (this as unknown as { cancelHook?: () => void }).cancelHook; | ||
| hook?.(); |
There was a problem hiding this comment.
start() stores cancelHook on the controller, but cancel() reads this.cancelHook from the stream source object, so the hook is never found on normal client disconnects. In that case unsubscribe() and clearInterval(heartbeat) are skipped, leaking broadcast subscribers/timers across reconnects.
Useful? React with 👍 / 👎.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces significant infrastructure updates, including new Git hooks, configuration for OpenCode/Antigravity, and the initial Swift-based shell (Swiftun) for native macOS window hosting. My review identified a hardcoded path in the post-commit hook that breaks portability, a shell word-splitting issue in the pre-push hook, and code duplication regarding token resolution logic that should be centralized for better maintainability.
| elif [[ -x /Users/home/.local/bin/xh ]]; then | ||
| /Users/home/.local/bin/xh _record-commit "${GIT_DIR:-.git}" || true | ||
| /Users/home/.local/bin/xh backup --bg || true |
There was a problem hiding this comment.
The script contains a hardcoded user path /Users/home/.local/bin/xh. This will only work for the user named home and will fail for any other developer on their machine. This makes the hook not portable.
To fix this, you should use a variable like $HOME or the ~ tilde expansion to refer to the current user's home directory. This will ensure the hook works correctly for all users.
elif [[ -x "$HOME/.local/bin/xh" ]]; then
"$HOME/.local/bin/xh" _record-commit "${GIT_DIR:-.git}" || true
"$HOME/.local/bin/xh" backup --bg || true
| TESTS=$(find src -name "*.test.ts" -not -path "*/node_modules/*") | ||
| if [[ -z "$TESTS" ]]; then | ||
| echo "pre-push found no src tests" >&2 | ||
| exit 1 | ||
| fi | ||
|
|
||
| bun test $TESTS |
There was a problem hiding this comment.
The current method of collecting test files with TESTS=$(find ...) and then passing them unquoted to bun test $TESTS is not robust against filenames containing spaces. The shell will perform word splitting on the $TESTS variable, causing the command to fail.
Using mapfile (or readarray) to read the find output into a bash array is a safer and more modern approach that correctly handles all filenames.
mapfile -t test_files < <(find src -name "*.test.ts" -not -path "*/node_modules/*")
if [ ${#test_files[@]} -eq 0 ]; then
echo "pre-push found no src tests" >&2
exit 1
fi
bun test "${test_files[@]}"
| private func readEvalTokenForIntent() -> String? { | ||
| if let env = ProcessInfo.processInfo.environment["DETOUR_EVAL_TOKEN"], !env.isEmpty { | ||
| return env | ||
| } | ||
| let path = NSString(string: "~/.detour/.env").expandingTildeInPath | ||
| guard let text = try? String(contentsOfFile: path, encoding: .utf8) else { return nil } | ||
| for line in text.split(separator: "\n") { | ||
| let t = line.trimmingCharacters(in: .whitespaces) | ||
| if t.hasPrefix("DETOUR_EVAL_TOKEN=") { | ||
| var v = String(t.dropFirst("DETOUR_EVAL_TOKEN=".count)) | ||
| if (v.hasPrefix("\"") && v.hasSuffix("\"")) || (v.hasPrefix("'") && v.hasSuffix("'")) { | ||
| v = String(v.dropFirst().dropLast()) | ||
| } | ||
| return v.isEmpty ? nil : v | ||
| } | ||
| } | ||
| return nil | ||
| } |
There was a problem hiding this comment.
The logic to read the DETOUR_EVAL_TOKEN from environment variables or the .env file is duplicated in several places, including ChatSurface.swift, NotificationManager.swift, and DetourClient.swift.
This duplication can lead to inconsistencies if the token resolution logic needs to be updated in the future. It would be better to centralize this logic into a single utility function or a shared helper, and then call it from all the places where the token is needed. This will improve maintainability and reduce code redundancy.
| let sinkQueue = DispatchQueue(label: "ai.detour.tts.sink") | ||
| synth.write(utterance) { buf in | ||
| collectSpeechBuffer(buf, sink: sink, queue: sinkQueue) | ||
| } | ||
| try await waitForSpeechCompletion(sink: sink, queue: sinkQueue) |
There was a problem hiding this comment.
Potential Deadlock with Synchronous Queue Access
The use of DispatchQueue.sync in both collectSpeechBuffer and waitForSpeechCompletion can lead to deadlocks if the queue is ever accessed recursively or from the main thread. This is especially risky if future modifications introduce such access patterns.
Recommendation:
Consider using DispatchQueue.async for non-blocking updates, or ensure that all queue accesses are strictly from background threads. Alternatively, use an actor or another concurrency-safe structure to manage shared state.
| let totalFrames = buffers.reduce(AVAudioFrameCount(0)) { $0 + $1.frameLength } | ||
| let outURL = FileManager.default.temporaryDirectory | ||
| .appendingPathComponent("detour-tts-\(UUID().uuidString).aiff") | ||
| defer { try? FileManager.default.removeItem(at: outURL) } |
There was a problem hiding this comment.
Temporary File Cleanup May Not Always Occur
The use of defer { try? FileManager.default.removeItem(at: outURL) } in writeAiff is generally safe, but if the process crashes or is terminated before the defer block executes, temporary files may be left behind, leading to resource leaks over time.
Recommendation:
Consider using a dedicated temporary file management utility or periodically cleaning up old temporary files to mitigate potential accumulation.
| func load() async { | ||
| struct Wrap: Decodable { let models: ModelConfigWire } | ||
| if let w: Wrap = await client.getEvalJSON("api/eval/models", as: Wrap.self) { | ||
| await MainActor.run { self.models = w.models } | ||
| } | ||
| struct PinnedWrap: Decodable { let tier: String } | ||
| if let p: PinnedWrap = await client.getEvalJSON("api/eval/planner-tier", as: PinnedWrap.self) { | ||
| await MainActor.run { self.plannerTier = p.tier } | ||
| } | ||
| } | ||
|
|
||
| func setPlannerTier(_ tier: String) async { | ||
| await client.postEval("api/eval/planner-tier", body: ["tier": tier]) | ||
| await MainActor.run { self.plannerTier = tier; self.status = tier.isEmpty ? "Planner uses default tier cascade." : "Planner pinned to \(tier)." } | ||
| } | ||
|
|
||
| func save() async { | ||
| guard let m = models else { return } | ||
| await MainActor.run { saving = true } | ||
| let body: [String: Any] = [ | ||
| "codexLarge": m.codexLarge, "codexSmall": m.codexSmall, "codexImage": m.codexImage, | ||
| "openRouterTextLarge": m.openRouterTextLarge, | ||
| "openRouterTextSmall": m.openRouterTextSmall, | ||
| "openRouterEmbedding": m.openRouterEmbedding, | ||
| "openRouterImage": m.openRouterImage, | ||
| "openRouterVideo": m.openRouterVideo, | ||
| "openRouterVision": m.openRouterVision, | ||
| "elizaCloudLarge": m.elizaCloudLarge, | ||
| "elizaCloudMedium": m.elizaCloudMedium, | ||
| "elizaCloudSmall": m.elizaCloudSmall, | ||
| "elizaCloudNano": m.elizaCloudNano, | ||
| "elizaCloudMega": m.elizaCloudMega, | ||
| "elizaCloudResponseHandler": m.elizaCloudResponseHandler, | ||
| "elizaCloudImage": m.elizaCloudImage, | ||
| "elizaCloudVideo": m.elizaCloudVideo, | ||
| ] | ||
| let ok = await client.postEval("api/eval/models", body: body) | ||
| await MainActor.run { | ||
| saving = false | ||
| status = ok ? "Saved — planner will use new routing on next call." : "Save failed." | ||
| } | ||
| } |
There was a problem hiding this comment.
Missing Error Handling in Async Network Calls
The load, setPlannerTier, and save methods in RoutingViewModel perform async network operations but do not handle errors or exceptions. If the network call fails, the UI may not reflect this, and the user may not be informed of the failure. This can lead to silent failures and poor user experience.
Recommended Solution:
Add error handling to these methods, such as using do-catch blocks or checking for nil/false returns, and update the status property to inform the user of any errors. For example:
func load() async {
do {
struct Wrap: Decodable { let models: ModelConfigWire }
if let w: Wrap = try await client.getEvalJSON("api/eval/models", as: Wrap.self) {
await MainActor.run { self.models = w.models }
} else {
await MainActor.run { self.status = "Failed to load models." }
}
// ...
} catch {
await MainActor.run { self.status = "Error: \(error.localizedDescription)" }
}
}| private struct CustomModelIdSheet: View { | ||
| let initial: String | ||
| let apply: (String) -> Void | ||
| let cancel: () -> Void | ||
| @State private var text: String = "" | ||
| var body: some View { | ||
| VStack(alignment: .leading, spacing: 12) { | ||
| Text("Custom model id").font(.headline) | ||
| Text("Paste a provider-specific model id. Use the exact string the provider expects (\"claude-sonnet-4-6\", \"openai/gpt-5\", etc.).") | ||
| .font(.caption).foregroundStyle(.secondary) | ||
| TextField("model id", text: $text) | ||
| .textFieldStyle(.roundedBorder) | ||
| HStack { | ||
| Spacer() | ||
| Button("Cancel", action: cancel) | ||
| Button("Apply") { apply(text) } | ||
| .buttonStyle(.borderedProminent) | ||
| .disabled(text.trimmingCharacters(in: .whitespaces).isEmpty) | ||
| } | ||
| } | ||
| .padding(20) | ||
| .frame(width: 460) | ||
| .onAppear { text = initial } | ||
| } | ||
| } |
There was a problem hiding this comment.
Lack of Input Validation for Custom Model ID
The CustomModelIdSheet allows users to input arbitrary strings for the model id, but there is no validation or sanitization. If the backend expects specific formats or has security requirements, this could lead to errors or vulnerabilities.
Recommended Solution:
Add input validation to ensure the model id conforms to expected formats before applying it. For example, use a regex or provider-specific validation logic, and display an error message if the input is invalid.
| private func fetchInitialSnapshot() async { | ||
| do { | ||
| let data = try await RPCClient.shared.call("tray.snapshot") | ||
| if let snap = try? JSONDecoder().decode(TraySnapshot.self, from: data) { | ||
| self.snapshot = snap | ||
| self.rebuildMenu() | ||
| } | ||
| } catch { | ||
| // First call often races bun's startup; the RPC client | ||
| // reconnects automatically. The push subscription will | ||
| // deliver the next update either way. | ||
| NSLog("[TrayController] initial snapshot fetch failed: \(error.localizedDescription)") | ||
| } | ||
| } |
There was a problem hiding this comment.
Silent error handling in async snapshot fetch
In fetchInitialSnapshot, errors during the RPC call or JSON decoding are either ignored or only logged. This can make it difficult to detect and diagnose failures, especially if the menu does not update as expected. Consider surfacing errors to the user (e.g., via a menu item or notification) or providing more robust logging and fallback behavior.
Recommended solution:
- Add user-visible error feedback if the initial snapshot cannot be fetched.
- Log decoding errors explicitly, not just RPC call failures.
- Optionally, retry the fetch after a delay if it fails.
| private func openDetourURL(_ url: String) { | ||
| guard let _ = URL(string: url) else { return } | ||
| let dispatchURL = URL(string: "http://127.0.0.1:2138/api/url-scheme/dispatch")! | ||
| var req = URLRequest(url: dispatchURL, timeoutInterval: 3.0) | ||
| req.httpMethod = "POST" | ||
| req.setValue("application/json", forHTTPHeaderField: "Content-Type") | ||
| let body = try? JSONSerialization.data(withJSONObject: ["url": url]) | ||
| req.httpBody = body | ||
| let task = URLSession.shared.dataTask(with: req) { _, response, _ in | ||
| if let http = response as? HTTPURLResponse, http.statusCode == 200 { return } | ||
| if let u = URL(string: url) { | ||
| DispatchQueue.main.async { NSWorkspace.shared.open(u) } | ||
| } | ||
| } | ||
| task.resume() | ||
| } |
There was a problem hiding this comment.
Insufficient error handling in openDetourURL
The openDetourURL function posts to a local endpoint and falls back to NSWorkspace.open if the HTTP status is not 200. However, it does not handle network errors, timeouts, or log failures, which could result in silent failures if the local server is unreachable or returns an unexpected response.
Recommended solution:
- Add error logging in the URLSession completion handler for network errors.
- Consider surfacing failures to the user if the fallback also fails.
- Ensure that all error cases are handled explicitly to avoid silent failures.
| wv.frame = host.bounds | ||
| wv.autoresizingMask = [.width, .height] | ||
| } | ||
| wv.load(URLRequest(url: url)) |
There was a problem hiding this comment.
Lack of Error Handling for WebView Loading
The wv.load(URLRequest(url: url)) call does not handle errors that may occur during the loading process. If the URL fails to load (e.g., due to network issues or invalid URLs), the user receives no feedback, and the window remains open in an unusable state.
Recommendation:
Implement WKNavigationDelegate for the WKWebView and handle webView(_:didFail:withError:) and webView(_:didFailProvisionalNavigation:withError:) to provide user feedback or close the window if loading fails.
| private func observeClose(for window: NSWindow, key: String) { | ||
| if let oldObserver = closeObservers.removeValue(forKey: key) { | ||
| NotificationCenter.default.removeObserver(oldObserver) | ||
| } | ||
| let observer = NotificationCenter.default.addObserver( | ||
| forName: NSWindow.willCloseNotification, | ||
| object: window, | ||
| queue: .main, | ||
| ) { [weak self] _ in | ||
| Task { @MainActor in | ||
| self?.windows.removeValue(forKey: key) | ||
| if let observer = self?.closeObservers.removeValue(forKey: key) { | ||
| NotificationCenter.default.removeObserver(observer) | ||
| } | ||
| } | ||
| } | ||
| closeObservers[key] = observer | ||
| } |
There was a problem hiding this comment.
Potential Memory Leak in Observer Management
While observers are removed when windows are closed, there is a risk of memory leaks if observers are not properly cleaned up in all scenarios (e.g., if the WindowFactory is deallocated or if windows are closed in non-standard ways).
Recommendation:
Consider using NotificationCenter.default.addObserver(forName:object:queue:using:) with a weak reference to self, as is done, but also ensure that all observers are removed in the WindowFactory's deinitializer (deinit) to guarantee cleanup in all cases.
1 participant
Summary
Verification