Merged
Conversation
Remove com.apple.developer.web-browser entitlement which was incorrectly added in db081d6 and causes provisioning profile conflicts. The restricted entitlement com.apple.developer.web-browser.public-key-credential remains as it is required for passkey support. Also add nookShortcutDetect to the message handler cleanup list in WebViewCoordinator to prevent handler leaks, and switch to stopAllSync in AppDelegate to ensure MCP child processes terminate cleanly. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
|
Caution Review failedThe pull request is closed. 📝 WalkthroughWalkthroughConverts several lifecycle APIs to async/await (with a synchronous termination wrapper), moves AI provider API keys to Keychain, adds MCP request timeouts and transport cancellation, auto-grants media capture for select OAuth origins, registers a new WebKit message handler for shortcut detection, and updates dependency pins. Changes
Sequence Diagram(s)sequenceDiagram
participant App as AppDelegate / NookApp
participant MCP as MCPManager
participant Client as MCPClient
participant Transport as MCPTransport
participant Server as RemoteServer
App->>MCP: call stopAllSync()
MCP->>MCP: spawn Task { await stopAll() }
MCP->>Client: await client.disconnect()
Client->>Transport: request close / cancel receiveTask
Transport->>Server: close connection / cancel streams
Transport-->>Client: closed
Client-->>MCP: disconnected
MCP-->>App: stopAllSync returns or times out (5s)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
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: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
Nook.xcodeproj/project.pbxproj (2)
43-53:⚠️ Potential issue | 🟡 MinorRemove or reinstate SiteSearch.swift—it's unused and excluded from the target.
SiteSearch.swift exists at
Nook/Models/Settings/SiteSearch.swiftbut has no references anywhere in the codebase. Since it's excluded from the Nook target viamembershipExceptionsand not used by any code, either remove the file entirely or clarify if it's intentional legacy code meant to stay in the repository but not compile.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Nook.xcodeproj/project.pbxproj` around lines 43 - 53, SiteSearch.swift is present in the repo but is excluded from the Nook target via the membershipExceptions entry in the PBXFileSystemSynchronizedBuildFileExceptionSet; either delete the file (SiteSearch.swift) from the repository if it's dead code, or reinstate it into the target by removing "SiteSearch.swift" from the membershipExceptions list and adding the necessary references/usages in the codebase (or adding it back to the appropriate target membership) so the compiler includes it; update the PBX entry (remove or modify the membershipExceptions) and commit the corresponding file removal or code changes to ensure consistency between project.pbxproj and the repo content.
579-585:⚠️ Potential issue | 🟠 MajorPin Garnish to a tagged release instead of tracking
main.
Lines 583-584 currently usebranch = main, which breaks reproducible builds. The Garnish package has a stable release (v0.1.0 from Jan 14, 2026) and earlier versions available—use a specific version tag instead. Ifmainis required for a specific feature, document the dependency and establish a clear update policy.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Nook.xcodeproj/project.pbxproj` around lines 579 - 585, The XCRemoteSwiftPackageReference for "Garnish" is pinned to branch = main causing non-reproducible builds; change the requirement block in the XCRemoteSwiftPackageReference "Garnish" entry (the repositoryURL and requirement fields) to reference the specific tag/version for the v0.1.0 release (use a version or exact tag requirement instead of branch) so the package is resolved reproducibly, and if you must keep main add a comment in the file/doc describing why and the update policy.Nook/Managers/WebViewCoordinator/WebViewCoordinator.swift (1)
386-397:⚠️ Potential issue | 🟠 MajorAdd
nookShortcutDetecthandler registration in WebViewCoordinator creation paths.The handler appears in the cleanup list (line 396) but is never registered during WebView creation. Both registration sites in
createWebViewInternal(lines 194–201 and 347–354) omit it, meaning shortcut detection will fail for WebViews created through these paths. While Tab.swift registers it separately (line 637), WebViewCoordinator should also register it for consistency.🛠️ Suggested fix
Add after line 201 and line 354:
newWebView.configuration.userContentController.add(tab, name: "NookIdentity") + newWebView.configuration.userContentController.add(tab, name: "nookShortcutDetect")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Nook/Managers/WebViewCoordinator/WebViewCoordinator.swift` around lines 386 - 397, WebViewCoordinator's createWebViewInternal paths register a set of WKScriptMessageHandlers but omit "nookShortcutDetect" while it is removed in cleanup; update both registration sites inside createWebViewInternal (the blocks that configure controller and define allMessageHandlers) to include "nookShortcutDetect" (use the same message name string as in the cleanup list, e.g., "nookShortcutDetect"), so the controller (webView.configuration.userContentController) adds that handler for the given tabId-backed web view just like the other handlers.Nook/Models/Tab/Tab.swift (1)
2856-2880:⚠️ Potential issue | 🟡 MinorUse the message frame's WebView for robust URL attribution.
_webView?.urlcan be nil or refer to a different window's WebView. Usemessage.frameInfo.webView?.url(with a fallback to_webView?.url) for multi-window/popup cases. (Note:message.webViewis deprecated; preferframeInfo.webView.)🛠️ Suggested update
- guard let shortcutsString = message.body as? String, - let currentURL = _webView?.url?.absoluteString else { return } + guard let shortcutsString = message.body as? String, + let currentURL = (message.frameInfo.webView?.url ?? _webView?.url)?.absoluteString else { return } - // Parse the comma-separated shortcuts - let shortcuts = Set(shortcutsString.split(separator: ",").map { String($0) }) + // Parse the comma-separated shortcuts + let shortcuts = Set( + shortcutsString + .split(separator: ",") + .map { $0.trimmingCharacters(in: .whitespacesAndNewlines) } + .filter { !$0.isEmpty } + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Nook/Models/Tab/Tab.swift` around lines 2856 - 2880, handleShortcutDetection currently reads the current URL from _webView?.url which can be nil or point to a different window; switch to using message.frameInfo.webView?.url with a fallback to _webView?.url to correctly attribute shortcuts to the originating frame. Update the guard in handleShortcutDetection to obtain currentURL from message.frameInfo.webView?.url?.absoluteString ?? _webView?.url?.absoluteString (or return if both are nil) before passing the URL and parsed shortcuts into browserManager?.keyboardShortcutManager?.websiteShortcutDetector.updateJSDetectedShortcuts.
🧹 Nitpick comments (1)
Nook/Models/AI/AIModels.swift (1)
119-207: Set an explicit Keychain accessibility class for consistency and clarity.While the default
kSecAttrAccessibleWhenUnlockedis suitable for foreground-only API key access, setting the policy explicitly is a best practice. Choose based on your app's needs: usekSecAttrAccessibleWhenUnlockedfor normal access (allows backup migration), orkSecAttrAccessibleWhenUnlockedThisDeviceOnlyif you want the key bound to this device only (no cross-device backup restore).♻️ Example using device-bound accessibility
- let attributes: [String: Any] = [kSecValueData as String: data] + let attributes: [String: Any] = [ + kSecValueData as String: data, + kSecAttrAccessible as String: kSecAttrAccessibleWhenUnlockedThisDeviceOnly + ] @@ - insert[kSecValueData as String] = data + insert[kSecValueData as String] = data + insert[kSecAttrAccessible as String] = kSecAttrAccessibleWhenUnlockedThisDeviceOnly🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Nook/Models/AI/AIModels.swift` around lines 119 - 207, Add an explicit Keychain accessibility attribute to stored items to avoid relying on defaults: in AIKeychainStorage (functions saveAPIKey(_:for:) and where new items are inserted or updated) include kSecAttrAccessible with the chosen value (e.g., kSecAttrAccessibleWhenUnlocked or kSecAttrAccessibleWhenUnlockedThisDeviceOnly) — add it to the insert dictionary before SecItemAdd and include it in the attributes passed to SecItemUpdate so both add and update calls set the accessibility class consistently.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Nook.xcodeproj/project.xcworkspace/xcshareddata/swiftpm/Package.resolved`:
- Around line 4-12: Package.resolved shows transitive packages "chronicle"
(Chronicle) and "swift-log" (Logging) that are not imported anywhere; remove
them by updating Swift package dependencies: open the Swift package manifest /
Xcode Swift Package settings and remove Chronicle and swift-log from the
resolved/dependency list (ensure they are not referenced in
XCRemoteSwiftPackageReference), then regenerate Package.resolved (e.g., resolve
packages or run swift package resolve) so the file no longer contains entries
for "chronicle"/Chronicle and "swift-log"/Logging; verify no source imports of
Chronicle or Logging remain before committing.
In `@Nook/Components/MiniWindow/MiniWindowWebView.swift`:
- Around line 388-404: Update the misleading comment in MiniWindowWebView's
media-permission handler: in the function
webView(_:requestMediaCaptureAuthorization:for:initiatedByFrame:decisionHandler:)
remove the claim that this API enables WebAuthn/passkey support and replace it
with an accurate note that this API controls camera/microphone access (used here
to grant camera/mic for OAuth providers that may require getUserMedia during
auth-only MiniWindow flows). While editing, consider adding an origin whitelist
check (using the origin parameter / WKSecurityOrigin) to only grant (.grant) for
known OAuth domains and otherwise call decisionHandler(.deny) to limit future
broad grants.
In `@Nook/Managers/AIManager/AIConfigService.swift`:
- Around line 354-361: After saving migrated API keys to the Keychain in
AIConfigService (where you call AIKeychainStorage.shared.saveAPIKey for "gemini"
and "openrouter"), remove the plaintext entries from UserDefaults by calling
removeObject(forKey:) for "settings.geminiApiKey" and
"settings.openRouterApiKey" after a successful save; ensure you perform the
removal only if the save was attempted (e.g., in the same conditional blocks
that check defaults.string(...) and !isEmpty) so no leftover plaintext secrets
remain in UserDefaults.
In `@Nook/Managers/AIManager/MCP/MCPClient.swift`:
- Around line 183-197: The continuation stored in pendingRequests[id] can leak
when withTimeout cancels the task; wrap the withCheckedThrowingContinuation
block in a withTaskCancellationHandler so that on cancellation you remove the
continuation from pendingRequests (e.g., pendingRequests.removeValue(forKey:
id)) and resume/throw if needed; apply the same pattern to the other similar
block (lines ~200-218) that stores continuations and calls transport.send to
ensure cleanup on timeout/cancellation.
In `@Nook/Managers/AIManager/MCP/MCPManager.swift`:
- Around line 37-46: stopAllSync in MCPManager deadlocks because Task { await
stopAll() } runs on `@MainActor` and semaphore.wait blocks the main thread;
replace the blocking wait with a runloop pump loop that periodically checks the
semaphore and allows the main runloop to process MainActor tasks until signaled
or a timeout elapses. Concretely, keep the Task that calls stopAll() and
semaphore.signal(), then replace _ = semaphore.wait(timeout: .now() + 5) with a
tight loop that checks semaphore.wait(timeout: .now()) for .success and
otherwise calls RunLoop.current.run(mode: .default, before:
Date().addingTimeInterval(0.01)) until success or a 5-second deadline to avoid
deadlock in stopAllSync.
In `@Nook/Managers/AIManager/MCP/MCPTransport.swift`:
- Around line 95-96: The detached receive task reads the shared property
receiveTask creating a data race because close() mutates it; instead, inside the
detached Task use Task.isCancelled to check cancellation (thread-safe) and do
not read self.receiveTask there, and when creating the task assign it to
self.receiveTask only after the Task is created (so construction and assignment
are separated) to avoid concurrent access; update the receive logic in
MCPTransport (the receiveTask property and the detached task block and close()
method) and make the same change in SSETransport (the analogous
receiveTask/receive loop and close method).
- Around line 174-176: The detached receive task currently reads the shared
property receiveTask inside its async loop causing a race when close() mutates
it; fix both StdioTransport.receive()/StdioTransport.close() and
SSETransport.receive()/SSETransport.close() by creating the Task first, checking
Task.isCancelled inside the loop (instead of reading receiveTask), then assign
self.receiveTask = task after creation, and have close() simply call
receiveTask?.cancel() and set receiveTask = nil — this avoids reading the
property from within the task and prevents the data race.
In `@Nook/Managers/AIManager/Providers/GeminiProvider.swift`:
- Around line 158-163: The Gemini functionResponse currently sets the function
name to the call ID because AIToolResult only stores toolCallId; update the code
so the builder can emit functionResponse.name as the original function name and
functionResponse.id as the call ID. Concretely, modify AIToolResult (or the
place that constructs toolCalls in GeminiProvider -> AIToolCall) to also carry
the original function name (e.g., a new property like toolName) when appending
toolCalls in the block that creates AIToolCall from functionCall, and then use
that stored toolName for functionResponse.name while using toolCallId for
functionResponse.id when reconstructing responses. Ensure mappings between
toolCallId and toolName are preserved so the response builder uses the correct
fields.
In `@Nook/Managers/ProfileManager/ProfileManager.swift`:
- Around line 126-139: The removeEphemeralProfile method can hang because
withCheckedContinuation waits forever if WebKit callbacks never fire; modify the
await withCheckedContinuation block (in removeEphemeralProfile) to implement a
timeout and single-resume guard: create a Bool/flag (e.g., resumed) and a
resumeSafely closure that calls continuation.resume() only once, start a
Task/DispatchWorkItem that sleeps for a configurable timeout (e.g., a few
seconds) and calls resumeSafely and logs a timeout, then call
profile.destroyEphemeralDataStore { resumeSafely() } so either the real
completion or the timeout resumes the continuation, and cancel the timeout task
when the real completion runs; keep use of ephemeralProfiles and
destroyEphemeralDataStore intact.
---
Outside diff comments:
In `@Nook.xcodeproj/project.pbxproj`:
- Around line 43-53: SiteSearch.swift is present in the repo but is excluded
from the Nook target via the membershipExceptions entry in the
PBXFileSystemSynchronizedBuildFileExceptionSet; either delete the file
(SiteSearch.swift) from the repository if it's dead code, or reinstate it into
the target by removing "SiteSearch.swift" from the membershipExceptions list and
adding the necessary references/usages in the codebase (or adding it back to the
appropriate target membership) so the compiler includes it; update the PBX entry
(remove or modify the membershipExceptions) and commit the corresponding file
removal or code changes to ensure consistency between project.pbxproj and the
repo content.
- Around line 579-585: The XCRemoteSwiftPackageReference for "Garnish" is pinned
to branch = main causing non-reproducible builds; change the requirement block
in the XCRemoteSwiftPackageReference "Garnish" entry (the repositoryURL and
requirement fields) to reference the specific tag/version for the v0.1.0 release
(use a version or exact tag requirement instead of branch) so the package is
resolved reproducibly, and if you must keep main add a comment in the file/doc
describing why and the update policy.
In `@Nook/Managers/WebViewCoordinator/WebViewCoordinator.swift`:
- Around line 386-397: WebViewCoordinator's createWebViewInternal paths register
a set of WKScriptMessageHandlers but omit "nookShortcutDetect" while it is
removed in cleanup; update both registration sites inside createWebViewInternal
(the blocks that configure controller and define allMessageHandlers) to include
"nookShortcutDetect" (use the same message name string as in the cleanup list,
e.g., "nookShortcutDetect"), so the controller
(webView.configuration.userContentController) adds that handler for the given
tabId-backed web view just like the other handlers.
In `@Nook/Models/Tab/Tab.swift`:
- Around line 2856-2880: handleShortcutDetection currently reads the current URL
from _webView?.url which can be nil or point to a different window; switch to
using message.frameInfo.webView?.url with a fallback to _webView?.url to
correctly attribute shortcuts to the originating frame. Update the guard in
handleShortcutDetection to obtain currentURL from
message.frameInfo.webView?.url?.absoluteString ?? _webView?.url?.absoluteString
(or return if both are nil) before passing the URL and parsed shortcuts into
browserManager?.keyboardShortcutManager?.websiteShortcutDetector.updateJSDetectedShortcuts.
---
Nitpick comments:
In `@Nook/Models/AI/AIModels.swift`:
- Around line 119-207: Add an explicit Keychain accessibility attribute to
stored items to avoid relying on defaults: in AIKeychainStorage (functions
saveAPIKey(_:for:) and where new items are inserted or updated) include
kSecAttrAccessible with the chosen value (e.g., kSecAttrAccessibleWhenUnlocked
or kSecAttrAccessibleWhenUnlockedThisDeviceOnly) — add it to the insert
dictionary before SecItemAdd and include it in the attributes passed to
SecItemUpdate so both add and update calls set the accessibility class
consistently.
Comment on lines
+4
to
+12
| { | ||
| "identity" : "chronicle", | ||
| "kind" : "remoteSourceControl", | ||
| "location" : "https://github.com/Aeastr/Chronicle.git", | ||
| "state" : { | ||
| "revision" : "78f4d0d634c4834f13f7ab1bcca79c150c4c172a", | ||
| "version" : "3.0.2" | ||
| } | ||
| }, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check for imports of Chronicle and Logging in Swift files
find . -name "*.swift" -type f | head -20Repository: nook-browser/Nook
Length of output: 989
🏁 Script executed:
# Search for Chronicle imports
rg -n --type=swift 'import\s+Chronicle' .Repository: nook-browser/Nook
Length of output: 43
🏁 Script executed:
# Search for Logging imports (swift-log)
rg -n --type=swift 'import\s+Logging' .Repository: nook-browser/Nook
Length of output: 43
🏁 Script executed:
# Check the Package.resolved file around the mentioned lines to see both dependencies
cat -n Nook.xcodeproj/project.xcworkspace/xcshareddata/swiftpm/Package.resolved | sed -n '1,20p'Repository: nook-browser/Nook
Length of output: 822
🏁 Script executed:
# Check lines around 130-138 for the swift-log dependency
cat -n Nook.xcodeproj/project.xcworkspace/xcshareddata/swiftpm/Package.resolved | sed -n '125,145p'Repository: nook-browser/Nook
Length of output: 855
🏁 Script executed:
# Check if Package.swift exists and see what dependencies are declared
find . -name "Package.swift" -type fRepository: nook-browser/Nook
Length of output: 43
🏁 Script executed:
# Check Package.swift contents
cat Package.swift 2>/dev/null || echo "No Package.swift in root"Repository: nook-browser/Nook
Length of output: 85
🏁 Script executed:
# Search for any reference to chronicle or logging (case insensitive, broader search)
rg -i "chronicle|logging" --type=swift . | head -30Repository: nook-browser/Nook
Length of output: 185
🏁 Script executed:
# Check if these might be mentioned in comments or documentation
rg -i "chronicle|swift-log" --type=swift --type=md .Repository: nook-browser/Nook
Length of output: 43
🏁 Script executed:
# Look for dependency declarations in the project
find . -name "*.swift" -path "*/Package.swift" -o -name "Package.resolved" | xargs grep -l "identity"Repository: nook-browser/Nook
Length of output: 135
🏁 Script executed:
# Check the Xcode project file for dependency declarations
find . -name "project.pbxproj" -type f | head -1 | xargs grep -i "chronicle\|swift-log" | head -20Repository: nook-browser/Nook
Length of output: 43
🏁 Script executed:
# Look for any Xcode configuration that might show package dependencies
find . -name "*.pbxproj" -o -name "*.xcconfig" | xargs grep -l "SPM\|Package" 2>/dev/null | head -5Repository: nook-browser/Nook
Length of output: 93
🏁 Script executed:
# Check if Package.resolved shows these as direct dependencies or transitive
# by looking for which packages might depend on them
rg -A 5 -B 5 '"identity" : "(chronicle|swift-log)"' Package.resolvedRepository: nook-browser/Nook
Length of output: 117
🏁 Script executed:
# Let me check if there are any other config files that show direct dependencies
find . -type f -name "*.json" -o -name "*.yml" -o -name "*.yaml" | xargs grep -l "chronicle\|swift-log" 2>/dev/nullRepository: nook-browser/Nook
Length of output: 45
🏁 Script executed:
# Check the actual content of project.pbxproj - just grep for the package identifiers
grep -i "chronicle\|swift-log" ./Nook.xcodeproj/project.pbxproj | head -20Repository: nook-browser/Nook
Length of output: 43
🏁 Script executed:
# Look for any SPM package references in the pbxproj
grep -i "XCRemoteSwiftPackageReference\|package-id" ./Nook.xcodeproj/project.pbxproj | head -30Repository: nook-browser/Nook
Length of output: 2081
🏁 Script executed:
# Check the full Package.resolved path and search for dependency relationships
cat ./Nook.xcodeproj/project.xcworkspace/xcshareddata/swiftpm/Package.resolved | grep -A 10 '"identity" : "chronicle"' | head -15Repository: nook-browser/Nook
Length of output: 394
🏁 Script executed:
# Count total pins in Package.resolved and see if these appear to be transitive
wc -l ./Nook.xcodeproj/project.xcworkspace/xcshareddata/swiftpm/Package.resolved
grep '"identity"' ./Nook.xcodeproj/project.xcworkspace/xcshareddata/swiftpm/Package.resolved | wc -lRepository: nook-browser/Nook
Length of output: 142
Remove unused transitive dependencies Chronicle and swift-log.
No imports of Chronicle or Logging found in the Swift codebase. Both packages appear in Package.resolved but are not declared as direct dependencies in the Xcode project (not in XCRemoteSwiftPackageReference), indicating they are transitive dependencies. If they are not required by their parent dependencies, they should be removed to reduce supply-chain surface area.
Also applies to: 130-138
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Nook.xcodeproj/project.xcworkspace/xcshareddata/swiftpm/Package.resolved`
around lines 4 - 12, Package.resolved shows transitive packages "chronicle"
(Chronicle) and "swift-log" (Logging) that are not imported anywhere; remove
them by updating Swift package dependencies: open the Swift package manifest /
Xcode Swift Package settings and remove Chronicle and swift-log from the
resolved/dependency list (ensure they are not referenced in
XCRemoteSwiftPackageReference), then regenerate Package.resolved (e.g., resolve
packages or run swift package resolve) so the file no longer contains entries
for "chronicle"/Chronicle and "swift-log"/Logging; verify no source imports of
Chronicle or Logging remain before committing.
Contributor
Author
|
check on changes made in commit 8d7e417 should have fixed concerns raised in previous comments |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Nook/Managers/WebViewCoordinator/WebViewCoordinator.swift (1)
348-355:⚠️ Potential issue | 🟠 MajorMissing
nookShortcutDetecthandler registration increateWebView.The
nookShortcutDetecthandler was added tocreateWebViewInternal(line 202) but not to thiscreateWebViewmethod. This inconsistency will cause shortcut detection to fail for WebViews created via this path.🐛 Proposed fix
newWebView.configuration.userContentController.add(tab, name: "historyStateDidChange") newWebView.configuration.userContentController.add(tab, name: "NookIdentity") + newWebView.configuration.userContentController.add(tab, name: "nookShortcutDetect") tab.setupThemeColorObserver(for: newWebView)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Nook/Managers/WebViewCoordinator/WebViewCoordinator.swift` around lines 348 - 355, createWebView is missing registration of the "nookShortcutDetect" WKScriptMessageHandler that createWebViewInternal already adds, causing shortcut detection to break for webviews created via createWebView; update createWebView to also call newWebView.configuration.userContentController.add(tab, name: "nookShortcutDetect") (or use the same handler/variable used in createWebViewInternal) so the handler is consistently registered for both createWebView and createWebViewInternal.
🧹 Nitpick comments (4)
Nook/Models/Tab/Tab.swift (1)
3131-3158: MissingnookShortcutDetecthandler in popup WebView setup.Popup WebViews created via
createWebViewWithdon't register thenookShortcutDetecthandler. While popups may not need shortcut detection, the inconsistency with other handler lists could cause issues if the handler list is assumed to be uniform elsewhere.♻️ Add handler for consistency (if needed for popups)
newWebView.configuration.userContentController.removeScriptMessageHandler( forName: "NookIdentity") + newWebView.configuration.userContentController.removeScriptMessageHandler( + forName: "nookShortcutDetect") // Now add the handlers ... newWebView.configuration.userContentController.add(newTab, name: "NookIdentity") + newWebView.configuration.userContentController.add(newTab, name: "nookShortcutDetect")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Nook/Models/Tab/Tab.swift` around lines 3131 - 3158, The popup WebView setup in Tab.swift (the block that removes and then adds script message handlers on newWebView for newTab) omits the "nookShortcutDetect" handler; update that handler list so it is consistent by calling newWebView.configuration.userContentController.removeScriptMessageHandler(forName: "nookShortcutDetect") in the removal section and newWebView.configuration.userContentController.add(newTab, name: "nookShortcutDetect") in the addition section (same place as the other handlers in the createWebViewWith/newWebView setup).Nook/Components/MiniWindow/MiniWindowWebView.swift (1)
402-407: Domain matching could permit unintended origins.Using
origin.host.contains($0)allows matches likeaccounts.google.com.malicious.comto be granted permission. Consider using suffix matching with a leading dot check for tighter security:♻️ Proposed fix with stricter domain matching
let knownOAuthDomains = [ "accounts.google.com", "login.microsoftonline.com", "github.com", "appleid.apple.com", "auth0.com", "okta.com", "auth.cloudflare.com" ] - let isKnownOAuth = knownOAuthDomains.contains { origin.host.contains($0) } + let host = origin.host.lowercased() + let isKnownOAuth = knownOAuthDomains.contains { domain in + host == domain || host.hasSuffix(".\(domain)") + } decisionHandler(isKnownOAuth ? .grant : .deny)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Nook/Components/MiniWindow/MiniWindowWebView.swift` around lines 402 - 407, The current matching uses origin.host.contains($0) which permits hosts like "accounts.google.com.malicious.com"; update the check in MiniWindowWebView.swift to safely normalize origin.host to lowercase (guarding against nil) and perform strict suffix/or-equals matching: replace isKnownOAuth with something like let host = origin.host?.lowercased() ?? "" and then knownOAuthDomains.contains { host == $0 || host.hasSuffix("." + $0) } before calling decisionHandler, so only exact matches or subdomains of the listed OAuth domains are granted.Nook/Managers/AIManager/MCP/MCPManager.swift (1)
73-82: Consider reducing duplication betweendisconnectServeranddisconnectServerAsync.Both methods contain identical logic. You could have
disconnectServercall the async version internally:♻️ Suggested refactor
func disconnectServer(_ serverId: String) { - guard let client = clients[serverId] else { return } - Task { - await client.disconnect() - clients.removeValue(forKey: serverId) - connectionStates[serverId] = .disconnected - refreshTools() + await disconnectServerAsync(serverId) } }Also applies to: 93-100
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Nook/Managers/AIManager/MCP/MCPManager.swift` around lines 73 - 82, The disconnectServer and disconnectServerAsync methods duplicate the same logic; refactor by making disconnectServer simply call/unwrap the async implementation to avoid duplication: move the disconnect logic into one place (keep the existing async Task-based body in disconnectServerAsync), then modify disconnectServer to locate the client (clients[serverId]) and invoke the async method (e.g., by starting a Task that calls disconnectServerAsync(serverId) or by awaiting it if already async), ensuring you still update clients.removeValue(forKey:), connectionStates[serverId] = .disconnected, and call refreshTools() from the single implementation so both paths share the same behavior.Nook/Managers/AIManager/AIProvider.swift (1)
100-109: Consider makingtoolNamerequired to prevent future misuse.
All current call sites passtoolNameexplicitly withtoolCall.name, so the empty default is not currently exploited. However, makingtoolNamerequired would serve as a compile-time safeguard against accidental omission in future code:♻️ Suggested change
- init(toolCallId: String, toolName: String = "", content: String, isError: Bool = false) { + init(toolCallId: String, toolName: String, content: String, isError: Bool = false) { self.toolCallId = toolCallId self.toolName = toolName self.content = content🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Nook/Managers/AIManager/AIProvider.swift` around lines 100 - 109, The AIToolResult initializer currently gives toolName a default empty string which can hide missing values; update the init signature in struct AIToolResult to require toolName (remove the "= \"\""), ensuring callers pass toolName explicitly, and adjust any initializer call sites that rely on the default to pass toolCall.name (or the appropriate name) so compilation enforces presence of toolName.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Nook/Managers/AIManager/AIConfigService.swift`:
- Around line 137-140: updateProviderAPIKey only writes to Keychain via
AIKeychainStorage.shared.saveAPIKey and doesn’t mutate any `@Observable` state, so
bound views won’t refresh; change the implementation to route the update through
the observable updater (call updateProvider(_:apiKey:) with the same providerId
and apiKey) or, after saving, explicitly update the published state (e.g.,
assign activeProvider.apiKey = apiKey or send objectWillChange) so observers of
activeProvider/apiKey are notified.
---
Outside diff comments:
In `@Nook/Managers/WebViewCoordinator/WebViewCoordinator.swift`:
- Around line 348-355: createWebView is missing registration of the
"nookShortcutDetect" WKScriptMessageHandler that createWebViewInternal already
adds, causing shortcut detection to break for webviews created via
createWebView; update createWebView to also call
newWebView.configuration.userContentController.add(tab, name:
"nookShortcutDetect") (or use the same handler/variable used in
createWebViewInternal) so the handler is consistently registered for both
createWebView and createWebViewInternal.
---
Duplicate comments:
In `@Nook/Managers/AIManager/MCP/MCPClient.swift`:
- Around line 183-195: The continuation stored in pendingRequests inside the
withTimeout block can leak when the timeout cancels the task; wrap the
withCheckedThrowingContinuation body in a withTaskCancellationHandler that calls
the existing cancelPendingRequest helper (e.g., cancelPendingRequest(id:)) to
remove the continuation from pendingRequests on cancellation. Concretely, in the
code using withTimeout/requestTimeout and withCheckedThrowingContinuation (where
pendingRequests[id] = continuation and transport.send is started), add
withTaskCancellationHandler { onCancel: { await self.cancelPendingRequest(id:
id) } } around the continuation and ensure the cancellation handler runs before
exiting so late transport.send responses won't resume a timed-out continuation.
In `@Nook/Managers/AIManager/MCP/MCPTransport.swift`:
- Around line 95-96: The property receiveTask is accessed concurrently (written
in the AsyncStream closure and read/written in close()), so protect all accesses
with the existing lock: wrap the assignment to receiveTask inside the
AsyncStream closure and the reads/writes in close() with
lock.lock()/lock.unlock() (or use a scoped lock guard) so both the assignment
where the Task is created (reference receiveTask) and the cancellation/nulling
in close() (method close()) are performed while holding the same lock object;
ensure you only touch receiveTask while holding that lock to eliminate the data
race.
- Around line 174-176: There's a data race on the private var receiveTask in
SSETransport; protect all reads and writes to receiveTask using the existing
lock. Specifically, in SSETransport where you set receiveTask (the assignment at
the site referenced) and where you call receiveTask?.cancel() / nil it out (the
reads/cancel at the other site), wrap those operations in the lock (e.g.,
lock.lock(); defer { lock.unlock() } around the assignment, cancel, and niling)
so that any access to receiveTask is synchronized with the same lock used
elsewhere.
---
Nitpick comments:
In `@Nook/Components/MiniWindow/MiniWindowWebView.swift`:
- Around line 402-407: The current matching uses origin.host.contains($0) which
permits hosts like "accounts.google.com.malicious.com"; update the check in
MiniWindowWebView.swift to safely normalize origin.host to lowercase (guarding
against nil) and perform strict suffix/or-equals matching: replace isKnownOAuth
with something like let host = origin.host?.lowercased() ?? "" and then
knownOAuthDomains.contains { host == $0 || host.hasSuffix("." + $0) } before
calling decisionHandler, so only exact matches or subdomains of the listed OAuth
domains are granted.
In `@Nook/Managers/AIManager/AIProvider.swift`:
- Around line 100-109: The AIToolResult initializer currently gives toolName a
default empty string which can hide missing values; update the init signature in
struct AIToolResult to require toolName (remove the "= \"\""), ensuring callers
pass toolName explicitly, and adjust any initializer call sites that rely on the
default to pass toolCall.name (or the appropriate name) so compilation enforces
presence of toolName.
In `@Nook/Managers/AIManager/MCP/MCPManager.swift`:
- Around line 73-82: The disconnectServer and disconnectServerAsync methods
duplicate the same logic; refactor by making disconnectServer simply call/unwrap
the async implementation to avoid duplication: move the disconnect logic into
one place (keep the existing async Task-based body in disconnectServerAsync),
then modify disconnectServer to locate the client (clients[serverId]) and invoke
the async method (e.g., by starting a Task that calls
disconnectServerAsync(serverId) or by awaiting it if already async), ensuring
you still update clients.removeValue(forKey:), connectionStates[serverId] =
.disconnected, and call refreshTools() from the single implementation so both
paths share the same behavior.
In `@Nook/Models/Tab/Tab.swift`:
- Around line 3131-3158: The popup WebView setup in Tab.swift (the block that
removes and then adds script message handlers on newWebView for newTab) omits
the "nookShortcutDetect" handler; update that handler list so it is consistent
by calling
newWebView.configuration.userContentController.removeScriptMessageHandler(forName:
"nookShortcutDetect") in the removal section and
newWebView.configuration.userContentController.add(newTab, name:
"nookShortcutDetect") in the addition section (same place as the other handlers
in the createWebViewWith/newWebView setup).
Comment on lines
+137
to
+140
| /// Update only the API key for a provider (stored in Keychain) | ||
| func updateProviderAPIKey(_ providerId: String, apiKey: String) { | ||
| AIKeychainStorage.shared.saveAPIKey(apiKey, for: providerId) | ||
| } |
There was a problem hiding this comment.
API-key-only updates may not notify observers.
updateProviderAPIKey writes to Keychain but doesn’t mutate any @Observable state, so views bound to activeProvider.apiKey may not refresh. Consider routing through updateProvider(_, apiKey:) or explicitly bumping observable state after save.
💡 Possible adjustment
func updateProviderAPIKey(_ providerId: String, apiKey: String) {
AIKeychainStorage.shared.saveAPIKey(apiKey, for: providerId)
+ if let idx = config.providers.firstIndex(where: { $0.id == providerId }) {
+ config.providers[idx] = config.providers[idx] // trigger observation
+ }
+ save()
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Nook/Managers/AIManager/AIConfigService.swift` around lines 137 - 140,
updateProviderAPIKey only writes to Keychain via
AIKeychainStorage.shared.saveAPIKey and doesn’t mutate any `@Observable` state, so
bound views won’t refresh; change the implementation to route the update through
the observable updater (call updateProvider(_:apiKey:) with the same providerId
and apiKey) or, after saving, explicitly update the published state (e.g.,
assign activeProvider.apiKey = apiKey or send objectWillChange) so observers of
activeProvider/apiKey are notified.
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 18622 🏁 Script executed: Length of output: 45442
✅ Successfully Addressed
|
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 by CodeRabbit
New Features
Improvements
Bug Fixes
Chores