Fix Apple Notes permission flow#8807
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 98fdc556cc
ℹ️ 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".
| guard let storeURL = candidates.first(where: { fm.fileExists(atPath: $0.path) }) else { | ||
| throw AppleNotesReaderError.invalidSelectedFolder(path: selectedFolderPath) | ||
| } |
There was a problem hiding this comment.
Resolve stored parent Notes folders before failing
When onboardingAppleNotesFolderPath contains ~/Library/Group Containers (the onboarding picker still writes selectedURL.path directly in selectAppleNotesFolderAndSync, and the Apps page treats that parent as a valid selection), this new guard throws invalid_selected_folder before adding the normal home-directory candidates. Previously the reader fell through to ~/Library/Group Containers/group.com.apple.notes/NoteStore.sqlite, so those users now cannot sync Notes until they pick the exact group.com.apple.notes folder; resolve the remembered path or only fail after the default candidates are checked.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
2 issues found across 5 files
Confidence score: 3/5
- In
desktop/macos/Desktop/Sources/AppleNotesReaderService.swift,locateNotesStoreURLcan fail to resolve a previously savedselectedFolderPathwhen it points to the parent~/Library/Group Containersdirectory, so existing users may lose a working import path after updating and be blocked from reading notes — add fallback candidate handling for the legacy parent-folder selection before merging. - In
desktop/macos/Desktop/Sources/MainWindow/Pages/AppsPage.swift, Apple Notes import appears to reopen folder selection for allAppleNotesReaderErrorcases, including schema/read errors that folder re-selection cannot fix, which can trap users in a retry loop and hide the real failure cause — gate the picker prompt to path-related errors and surface non-path errors directly.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="desktop/macos/Desktop/Sources/MainWindow/Pages/AppsPage.swift">
<violation number="1" location="desktop/macos/Desktop/Sources/MainWindow/Pages/AppsPage.swift:1264">
P2: Apple Notes import currently reopens folder selection for every AppleNotesReaderError, including schema/read failures that are not fixed by choosing a different folder. This can send users into a retry loop instead of surfacing the real failure; consider prompting folder selection only for path/permission errors and returning the error directly for `.schemaUnavailable`/`.storeReadFailed`.</violation>
</file>
<file name="desktop/macos/Desktop/Sources/AppleNotesReaderService.swift">
<violation number="1" location="desktop/macos/Desktop/Sources/AppleNotesReaderService.swift:396">
P1: When `locateNotesStoreURL` reads a stored `selectedFolderPath` from UserDefaults that is the parent `~/Library/Group Containers` directory (which was a valid selection in the prior flow), the candidates built here won't include the nested `group.com.apple.notes/NoteStore.sqlite` path. The guard then throws `invalidSelectedFolder`, breaking Notes sync for those users.
`resolveSelectedFolder` handles this exact parent-folder inference, but `locateNotesStoreURL` never calls it. Consider either calling `resolveSelectedFolder` on the stored path before building candidates, or adding `selectedFolderURL/group.com.apple.notes/NoteStore.sqlite` to the candidates list so existing users aren't broken.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| .appendingPathComponent("NoteStore.sqlite", isDirectory: false) | ||
| ) | ||
|
|
||
| guard let storeURL = candidates.first(where: { fm.fileExists(atPath: $0.path) }) else { |
There was a problem hiding this comment.
P1: When locateNotesStoreURL reads a stored selectedFolderPath from UserDefaults that is the parent ~/Library/Group Containers directory (which was a valid selection in the prior flow), the candidates built here won't include the nested group.com.apple.notes/NoteStore.sqlite path. The guard then throws invalidSelectedFolder, breaking Notes sync for those users.
resolveSelectedFolder handles this exact parent-folder inference, but locateNotesStoreURL never calls it. Consider either calling resolveSelectedFolder on the stored path before building candidates, or adding selectedFolderURL/group.com.apple.notes/NoteStore.sqlite to the candidates list so existing users aren't broken.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At desktop/macos/Desktop/Sources/AppleNotesReaderService.swift, line 396:
<comment>When `locateNotesStoreURL` reads a stored `selectedFolderPath` from UserDefaults that is the parent `~/Library/Group Containers` directory (which was a valid selection in the prior flow), the candidates built here won't include the nested `group.com.apple.notes/NoteStore.sqlite` path. The guard then throws `invalidSelectedFolder`, breaking Notes sync for those users.
`resolveSelectedFolder` handles this exact parent-folder inference, but `locateNotesStoreURL` never calls it. Consider either calling `resolveSelectedFolder` on the stored path before building candidates, or adding `selectedFolderURL/group.com.apple.notes/NoteStore.sqlite` to the candidates list so existing users aren't broken.</comment>
<file context>
@@ -262,6 +392,11 @@ actor AppleNotesReaderService {
.appendingPathComponent("NoteStore.sqlite", isDirectory: false)
)
+
+ guard let storeURL = candidates.first(where: { fm.fileExists(atPath: $0.path) }) else {
+ throw AppleNotesReaderError.invalidSelectedFolder(path: selectedFolderPath)
+ }
</file context>
| } catch let error as AppleNotesReaderError { | ||
| switch error { | ||
| case .storeNotFound, .storeUnavailable: | ||
| case .storeNotFound, .authorizationDenied, .invalidSelectedFolder, .schemaUnavailable, .storeReadFailed: |
There was a problem hiding this comment.
P2: Apple Notes import currently reopens folder selection for every AppleNotesReaderError, including schema/read failures that are not fixed by choosing a different folder. This can send users into a retry loop instead of surfacing the real failure; consider prompting folder selection only for path/permission errors and returning the error directly for .schemaUnavailable/.storeReadFailed.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At desktop/macos/Desktop/Sources/MainWindow/Pages/AppsPage.swift, line 1264:
<comment>Apple Notes import currently reopens folder selection for every AppleNotesReaderError, including schema/read failures that are not fixed by choosing a different folder. This can send users into a retry loop instead of surfacing the real failure; consider prompting folder selection only for path/permission errors and returning the error directly for `.schemaUnavailable`/`.storeReadFailed`.</comment>
<file context>
@@ -1269,7 +1261,7 @@ private final class ImportConnectorSheetModel: ObservableObject {
} catch let error as AppleNotesReaderError {
switch error {
- case .storeNotFound, .storeUnavailable:
+ case .storeNotFound, .authorizationDenied, .invalidSelectedFolder, .schemaUnavailable, .storeReadFailed:
break
}
</file context>
Summary
apple_notes_read_probeautomation action plus focused reader tests for selected folders, permission errors, schema errors, and limit clampingVerification
xcrun swift test --package-path desktop/macos/Desktop --filter AppleNotesReaderServiceTests/Applications/omi-apple-notes-auth.app(com.omi.omi-apple-notes-auth) on automation port 47998:classification=readable,noteCount=1,firstTitle=Agent verification notemaxResults=-7probe returned readable via clamped limit/tmpreturnedclassification=invalid_selected_folder