Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #581 +/- ##
==========================================
+ Coverage 72.79% 73.55% +0.76%
==========================================
Files 56 57 +1
Lines 2286 2401 +115
==========================================
+ Hits 1664 1766 +102
- Misses 473 481 +8
- Partials 149 154 +5
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
Adds Safari multi-profile support on macOS 14+ by introducing profile discovery and per-profile source path resolution, while keeping shared data (Keychain passwords, bookmarks) attributed to the default profile and filtering shared downloads per entry.
Changes:
- Discover named Safari profiles via
SafariTabs.db(authoritative) with aSafari/Profiles/ReadDir fallback when the DB is unreadable. - Resolve per-profile source paths for History/Cookies and filter shared
Downloads.plistentries byDownloadEntryProfileUUIDStringKey. - Add extensive tests for profile discovery, source building, and per-profile download filtering.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| browser/safari/testutil_test.go | Adds a helper to create SafariTabs.db fixtures for profile discovery tests. |
| browser/safari/source.go | Refactors Safari source path resolution to be profile-aware (default vs named). |
| browser/safari/safari.go | Returns one Browser per profile; routes extraction/counting accordingly and filters shared downloads. |
| browser/safari/safari_test.go | Adds multi-profile NewBrowsers test and updates resolveSourcePaths tests for new API. |
| browser/safari/profiles.go | Implements profile discovery, name sanitization, UUID normalization, and disambiguation. |
| browser/safari/profiles_test.go | New test suite covering discovery behavior, fallback rules, naming, and source building. |
| browser/safari/extract_download.go | Filters downloads by profile ownership and updates plist field mappings. |
| browser/safari/extract_download_test.go | Updates/expands tests for per-profile download filtering and counting. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| continue | ||
| } | ||
| out = append(out, newNamedProfile(externalUUID.String, title.String)) | ||
| } |
There was a problem hiding this comment.
readNamedProfilesFromDB iterates rows but never checks rows.Err() before returning. That can silently drop query/IO errors mid-iteration and treat the DB as authoritative with partial results. Please add a rows.Err() check (and return an error) before returning out.
| } | |
| } | |
| if err := rows.Err(); err != nil { | |
| return nil, fmt.Errorf("iterate SafariTabs.db rows: %w", err) | |
| } |
There was a problem hiding this comment.
Good catch — fixed in 5d5ec1c. Added rows.Err() check after the iteration so mid-iteration IO errors no longer get swallowed as an authoritative empty result.
| sources := buildSources(profileContext{legacyHome: dir}) | ||
| resolved := resolveSourcePaths(sources) |
There was a problem hiding this comment.
This test builds sources with profileContext{legacyHome: dir} but leaves profileContext.container empty. defaultSources will then generate a relative container cookie path ("Cookies/Cookies.binarycookies"), so resolveSourcePaths may accidentally stat a real file from the current working directory and make the test flaky. Set container explicitly (deriveContainerRoot(dir)) or adjust buildSources/defaultSources to derive container when empty.
There was a problem hiding this comment.
Fixed in 5d5ec1c — both tests now pass container: deriveContainerRoot(dir) so defaultSources gets absolute paths instead of relative candidates against cwd.
| sources := buildSources(profileContext{legacyHome: t.TempDir()}) | ||
| assert.Empty(t, resolveSourcePaths(sources)) |
There was a problem hiding this comment.
Same as above: container is empty here, so buildSources/defaultSources can produce relative candidate paths (e.g., "Cookies/Cookies.binarycookies"), making the assertion dependent on the process working directory. Provide container (deriveContainerRoot) or make buildSources fill it in.
There was a problem hiding this comment.
Same fix as above in 5d5ec1c — TestResolveSourcePaths_Empty now sets container explicitly.
| // namedSources omits shared categories (Bookmark, Download) — those are attributed to the default profile. | ||
| // | ||
| // LocalStorage slot for a follow-up PR: | ||
| // | ||
| // file(filepath.Join(p.container, "WebKit/WebsiteDataStore", p.uuidLower, "LocalStorage")) | ||
| func namedSources(p profileContext) map[types.Category][]sourcePath { | ||
| profileDir := filepath.Join(p.container, "Safari", "Profiles", p.uuidUpper) | ||
| webkitStore := filepath.Join(p.container, "WebKit", "WebsiteDataStore", p.uuidLower) | ||
|
|
||
| return map[types.Category][]sourcePath{ | ||
| types.History: {file(filepath.Join(profileDir, "History.db"))}, | ||
| types.Cookie: {file(filepath.Join(webkitStore, "Cookies", "Cookies.binarycookies"))}, | ||
| types.Download: {file(filepath.Join(p.legacyHome, "Downloads.plist"))}, |
There was a problem hiding this comment.
The comment says namedSources omits shared categories "(Bookmark, Download)", but the function actually includes types.Download. This is misleading for future maintenance—either update the comment to reflect that Downloads.plist is shared but still included (and filtered per-profile), or remove Download from namedSources if the intent is truly default-only attribution.
There was a problem hiding this comment.
Stale comment — fixed in 5d5ec1c. Updated to explain that Download is included but filtered by DownloadEntryProfileUUIDStringKey in extractDownloads, while Bookmark stays default-only because it has no per-entry profile tag.
Summary
SafariTabs.db(authoritative) with aSafari/Profiles/ReadDir fallback; always includes the default profile.History/Cookieare per-profile — default reads from~/Library/Safari/+~/Library/Containers/.../Cookies/, named profiles read fromSafari/Profiles/<UUID>/+WebKit/WebsiteDataStore/<uuid>/Cookies/.Downloadis a shared plist filtered per-entry viaDownloadEntryProfileUUIDStringKey(empty = pre-profile Safari → default).Bookmarkhas no per-entry profile tag, so it stays attributed to the default profile only (avoids duplicating the same entries per profile).Password(macOS Keychain) is user-scope; also attributed to default only.Addresses the multi-profile item of #565; PR5 (LocalStorage) is the next step and has a hook point left in
buildSources.Test plan
go test ./browser/safari/... -count=1— new profile-discovery tests (default-only, named+default, title fallback, orphan UUID, missing-DB ReadDir fallback, duplicate names, UUID case, empty-DB authoritative) and per-profile download filtering.golangci-lint run ./browser/safari/— 0 issues.GOOS=windows GOARCH=amd64 go build/GOOS=linux GOARCH=amd64 go build— cross-compile clean.default+profile 2):history.json: 8815 / 12 split across profilescookie.json: 66 / 46 split across profilesdownload.json: 2 / 2 correctly attributed byDownloadEntryProfileUUIDStringKeypassword.json: 17 (default only, no duplication)bookmark.json: 28 (default only, no duplication)