Repository navigation
Browser: import data from detected installed browsers - #318
Merged
lawrencecchen merged 8 commits intoMar 17, 2026
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
![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: 9dd66980ff
ℹ️ 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".
| domainMatches(host: host, filters: domainFilters) else { | ||
| return | ||
| } | ||
| let lastVisited = firefoxDate(fromUnixMicroseconds: lastVisitMicros) ?? Date() |
There was a problem hiding this comment.
Rows with missing/zero visit timestamps are currently imported as Date() (here for Firefox, and similarly in Chromium), which makes never-visited or timestamp-less URLs look like they were just visited. In real profiles this can happen for entries with last_visit_date/last_visit_time unset, and it will distort recency ordering and can cause mergeImportedEntries to overwrite existing metadata as if those pages were most recent. These rows should be skipped or given a non-current fallback (for example .distantPast) rather than Date().
Useful? React with 👍 / 👎.
Contributor
Author
There was a problem hiding this comment.
Fixed in dc6bcb2. Firefox and Chromium now fall back to .distantPast instead of Date(), so missing visit timestamps no longer jump to the front.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
📝 WalkthroughWalkthroughAdds a browser data import feature: detects installed browsers, presents a multi-step import wizard (cookies/history with domain filtering), imports via SQLite/binary parsing, merges results into the app’s history/cookie stores, and surfaces UI entry points (menu, settings) plus localization and tests. Changes
Sequence DiagramsequenceDiagram
participant User
participant UI as Import Wizard UI
participant Coord as BrowserDataImportCoordinator
participant Detect as InstalledBrowserDetector
participant Importer as BrowserDataImporter
participant Store as BrowserHistoryStore / CookieStore
User->>UI: Trigger import (menu / settings)
UI->>Detect: detectInstalledBrowsers()
Detect-->>UI: [InstalledBrowserCandidate...]
UI->>User: Present selection & scope
User->>UI: Confirm selection, provide domain filters
UI->>Coord: startImport(selection, scope, filters)
Coord->>Importer: importData(from:selection, scope, filters)
Importer->>Importer: read SQLite / binary cookie files
Importer-->>Coord: BrowserImportOutcome (counts, warnings)
Coord->>Store: mergeImportedEntries(importedHistoryEntries)
Store-->>Coord: mergedCount
Coord->>UI: presentOutcome(outcome, mergedCount)
UI->>User: Show results & warnings
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
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: 7
🧹 Nitpick comments (2)
cmuxTests/GhosttyConfigTests.swift (1)
1627-1663: Missing test coverage for.everythingscope.Per the
fromSelectiondocumentation inBrowserPanel.swift, whenincludeAdditionalDataistrue, the method returns.everything. Consider adding a test case:🧪 Proposed test for `.everything` scope
func testFromSelectionRejectsEmptySelection() { let scope = BrowserImportScope.fromSelection( includeCookies: false, includeHistory: false, includeAdditionalData: false ) XCTAssertNil(scope) } + + func testFromSelectionEverything() { + let scope = BrowserImportScope.fromSelection( + includeCookies: true, + includeHistory: true, + includeAdditionalData: true + ) + XCTAssertEqual(scope, .everything) + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmuxTests/GhosttyConfigTests.swift` around lines 1627 - 1663, Add a test to BrowserImportScopeTests that asserts BrowserImportScope.fromSelection returns .everything when includeAdditionalData is true; specifically, add a new test method (e.g., testFromSelectionEverything) that calls BrowserImportScope.fromSelection(includeCookies: X, includeHistory: Y, includeAdditionalData: true) and XCTAssertEqual(result, .everything) to cover the documented behavior in BrowserPanel.swift for the .everything scope.Sources/Panels/BrowserPanel.swift (1)
5846-5856: Extract duplicated URL canonicalization helper
dedupedCanonicalURLsis duplicated with the same implementation in two places. Please extract to a single file-private helper to avoid drift.Also applies to: 6425-6435
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/Panels/BrowserPanel.swift` around lines 5846 - 5856, The function dedupedCanonicalURLs(_:), duplicated in two places (one at this snippet and another at lines around 6425–6435), should be extracted to a single file-private helper to avoid drift: create a file-private top-level function (e.g., file-private func dedupedCanonicalURLs(_ urls: [URL]) -> [URL]) in this Swift file and replace both local/static implementations with calls to that helper; keep the exact logic (using url.standardizedFileURL.resolvingSymlinksInPath().path and a Set<String> for seen) and update any call sites to reference the new helper so there is only one canonical implementation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Sources/cmuxApp.swift`:
- Around line 3187-3189: The subtitle produced by browserImportSubtitle uses
InstalledBrowserDetector.summaryText(for:) which returns hardcoded English;
update it to use localized strings: either modify
InstalledBrowserDetector.summaryText(for:) to return
String(localized:defaultValue:) variants for all cases (no browsers, single
detected, multiple detected with a localized format that accepts a joined list),
or build the localized subtitle here in browserImportSubtitle by mapping
detectedImportBrowsers to localized browser names, joining them, and passing
that into String(localized:defaultValue:) format tokens (e.g., "Detected: {0}"
and "No supported browsers detected."). Ensure you use the
String(localized:defaultValue:) API for all user-facing text and keep the
existing function/variable names (InstalledBrowserDetector.summaryText(for:) and
browserImportSubtitle) to locate the change.
In `@Sources/Panels/BrowserPanel.swift`:
- Around line 6519-6529: The wizard currently hardcodes includeAdditionalData:
false which prevents emitting the .everything scope; add a UI control (e.g., a
new NSButton checkbox named additionalDataCheckbox) alongside cookiesCheckbox
and historyCheckbox in BrowserPanel.swift, expose its state
(additionalDataCheckbox.state == .on) to the form data, and replace the
hardcoded includeAdditionalData: false at the call site with the checkbox-driven
value so the flow can produce .everything when the user enables additional data.
- Around line 5179-5189: The enum's displayName property is returning hardcoded
user-facing strings (cases .cookiesOnly, .historyOnly, .cookiesAndHistory,
.everything) and other new import-flow strings elsewhere; replace each bare
string literal with the localization API (e.g., String(localized:
"importflow.cookiesOnly", defaultValue: "Cookies only")) using meaningful keys,
update every similar occurrence in the import flow (the other affected blocks
noted in the comment), and add corresponding keys and default English values to
the app's Localizable.strings (or the project's localization resource) so all
user-visible text is localized; keep keys consistent and use the same pattern
for tooltip/dialog/error strings.
In `@Sources/Panels/BrowserPanelView.swift`:
- Around line 1223-1225: The view is calling
InstalledBrowserDetector.detectInstalledBrowsers() synchronously from
refreshEmptyStateImportBrowsers(), blocking the UI; change
refreshEmptyStateImportBrowsers() to perform the scan off the main thread (e.g.,
DispatchQueue.global or an async Task) and only invoke it when the blank-state
overlay is actually eligible to show (check the same condition used to decide
overlay visibility in onAppear/CTA handling), then marshal the result back to
the main thread to assign emptyStateImportBrowsers; ensure you reference and
gate by the overlay eligibility condition so onAppear and non-blank tabs do not
trigger the scan.
- Around line 886-889: The empty-state CTA is being placed in SwiftUI's .overlay
above webView (isWebViewBlank() / emptyBrowserStateOverlay), which can render
behind portal-hosted WKWebView; change this so the CTA is not mounted in the
overlay layer: either move emptyBrowserStateOverlay out to the same
portal/AppKit-hosted container used by the search overlay (so it renders above
portal-hosted web content), or conditionally render it only when
!panel.shouldRenderWebView (i.e., only on the SwiftUI placeholder path). Update
references in BrowserPanelView to stop inserting emptyBrowserStateOverlay inside
the .overlay block and instead mount it via the portal-host/AppKit path or
guarded by !panel.shouldRenderWebView.
- Line 1141: The displayed summary comes from
InstalledBrowserDetector.summaryText(for:) which currently returns hard-coded
English; change this flow to return or produce localized strings and proper
plural forms before feeding Text in BrowserPanelView: update
InstalledBrowserDetector.summaryText(for:) (or add a new
summaryLocalizedKey(for:)) to return a LocalizedStringKey or a localized String
using NSLocalizedString with explicit ".one" and ".other" localization keys for
the "+N more" case, and then replace
Text(InstalledBrowserDetector.summaryText(for: emptyStateImportBrowsers)) with
Text(LocalizedStringKey_or_localizedString) so the UI uses localized keys and
correct plural handling.
---
Nitpick comments:
In `@cmuxTests/GhosttyConfigTests.swift`:
- Around line 1627-1663: Add a test to BrowserImportScopeTests that asserts
BrowserImportScope.fromSelection returns .everything when includeAdditionalData
is true; specifically, add a new test method (e.g., testFromSelectionEverything)
that calls BrowserImportScope.fromSelection(includeCookies: X, includeHistory:
Y, includeAdditionalData: true) and XCTAssertEqual(result, .everything) to cover
the documented behavior in BrowserPanel.swift for the .everything scope.
In `@Sources/Panels/BrowserPanel.swift`:
- Around line 5846-5856: The function dedupedCanonicalURLs(_:), duplicated in
two places (one at this snippet and another at lines around 6425–6435), should
be extracted to a single file-private helper to avoid drift: create a
file-private top-level function (e.g., file-private func dedupedCanonicalURLs(_
urls: [URL]) -> [URL]) in this Swift file and replace both local/static
implementations with calls to that helper; keep the exact logic (using
url.standardizedFileURL.resolvingSymlinksInPath().path and a Set<String> for
seen) and update any call sites to reference the new helper so there is only one
canonical implementation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c27f7763-a0fd-4922-9922-7c9e3a9492b4
📒 Files selected for processing (5)
Resources/Localizable.xcstringsSources/Panels/BrowserPanel.swiftSources/Panels/BrowserPanelView.swiftSources/cmuxApp.swiftcmuxTests/GhosttyConfigTests.swift
Comment on lines
+5949
to
+5955
| if browser.descriptor.id == "safari" { | ||
| return CookieImportResult( | ||
| importedCount: 0, | ||
| skippedCount: 0, | ||
| warnings: [ | ||
| "Safari cookies are stored in Cookies.binarycookies and are not yet supported by this importer." | ||
| ] |
There was a problem hiding this comment.
Cookie import is still incomplete for key browsers
Line 5954 explicitly reports Safari cookies as unsupported, and Lines 6066-6070 skip encrypted Chromium cookies. In practice, this leaves major auth/session cookie coverage incomplete for the core browser targets.
Also applies to: 6066-6070, 6096-6097
Contributor
Author
There was a problem hiding this comment.
Partially fixed in dc6bcb2. Chromium and Helium encrypted cookies now attempt Keychain-backed decryption automatically during import. Safari Cookies.binarycookies is still unsupported in this PR, so I left that warning path explicit instead of pretending coverage is complete.
There was a problem hiding this comment.
@lawrencecchen Thanks for the update. I walked through the new code and it matches what you described:
- Chromium-family encrypted cookies: BrowserDataImporter.importChromiumCookies now tries decryption via ChromiumCookieDecryptor (PBKDF2 “saltysalt”, v10/v11, Keychain lookup for “Storage Key” / “Safe Storage”, with Helium explicitly covered in keychainBaseNames). Skips are limited to true decrypt failures and a single consolidated warning is surfaced.
- Safari: cookie import remains intentionally unsupported with the explicit warning, while WebKit/Safari history import is implemented through History.db.
Targeted follow-ups I’d suggest:
- UX: In the import wizard, add a short note that macOS may prompt for Keychain access and (for some profiles) Full Disk Access might be required to read databases. This can reduce confusion if cookies don’t appear on first try.
- Tests: Add unit tests for BrowserDataImporter.parseDomainFilters and for InstalledBrowserDetector.summaryText formatting (<= limit and “+N more” cases). For Keychain-backed decryption, consider a tiny decryptor protocol so tests can inject a stub without requiring real Keychain access.
- Tracking: Keep the Safari Cookies.binarycookies path as an explicit “not yet supported” warning (good choice), and track it in a follow-up issue.
Would you like me to open a follow-up GitHub issue to track “Safari Cookies.binarycookies import support” with current constraints and proposed approach called out? I can file it and link back to this PR and comment.
✏️ Learnings added
Learnt from: lawrencecchen
Repo: manaflow-ai/cmux PR: 318
File: Sources/Panels/BrowserPanel.swift:7347-7356
Timestamp: 2026-03-17T05:33:49.911Z
Learning: cmux BrowserDataImporter: Encrypted cookies for Chromium-family (including Helium) now attempt automatic Keychain-backed decryption (PBKDF2 'saltysalt', v10/v11) via ChromiumCookieDecryptor; skips are warned when decryption fails.
Learnt from: lawrencecchen
Repo: manaflow-ai/cmux PR: 318
File: Sources/Panels/BrowserPanel.swift:7347-7356
Timestamp: 2026-03-17T05:33:49.911Z
Learning: cmux BrowserDataImporter: Safari Cookies.binarycookies import is currently unsupported by design; importer surfaces an explicit localized warning and only imports Safari/WebKit history from History.db.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: atani
Repo: manaflow-ai/cmux PR: 819
File: Sources/AppDelegate.swift:0-0
Timestamp: 2026-03-04T14:05:42.574Z
Learning: Guideline: In Swift files (cmux project), when handling pluralized strings, prefer using localization keys with the ICU-style plural forms .one and .other. For example, use keys like statusMenu.unreadCount.one for the singular case (1) and statusMenu.unreadCount.other for all other counts, and similarly for statusMenu.tooltip.unread.one/other. Rationale: ensures correct pluralization across locales and makes localization keys explicit. Review code to ensure any unread count strings and related tooltips follow this .one/.other key pattern and verify the correct value is chosen based on the count.
Learnt from: austinywang
Repo: manaflow-ai/cmux PR: 954
File: Sources/TerminalController.swift:0-0
Timestamp: 2026-03-05T22:04:34.712Z
Learning: Adopt the convention: for health/telemetry tri-state values in Swift, prefer Optionals (Bool?) over sentinel booleans. In TerminalController.swift, socketConnectable is Bool? and only set when socketProbePerformed is true; downstream logic must treat nil as 'not probed'. Ensure downstream code checks for nil before using a value and uses explicit non-nil checks to determine state, improving clarity and avoiding misinterpretation of default false.
Learnt from: moyashin63
Repo: manaflow-ai/cmux PR: 1074
File: Sources/AppDelegate.swift:7523-7545
Timestamp: 2026-03-09T01:38:24.337Z
Learning: When the command palette is visible (as in manaflow-ai/cmux Sources/AppDelegate.swift), ensure the shortcut handling consumes most Command shortcuts to protect the palette's text input. Specifically, do not allow UI zoom shortcuts (Cmd+Shift+= / Cmd+Shift+− / Cmd+Shift+0) to trigger while the palette is open. Do not reorder shortcut handlers (e.g., uiZoomShortcutAction(...)) to bypass this guard; users must close the palette before performing zoom actions. This guideline should apply to Swift source files handling global shortcuts within the app.
Learnt from: zlatkoc
Repo: manaflow-ai/cmux PR: 1368
File: Sources/Panels/BrowserPanel.swift:69-69
Timestamp: 2026-03-13T13:46:01.733Z
Learning: Do not wrap engine/brand name literals (e.g., displayName values such as Google, DuckDuckGo, Bing, Kagi, Startpage) in String(localized: ...). These are brand/product names that are not translatable UI text. Localization should apply to generic UI strings (labels, buttons, error messages, etc.). Apply this guideline across Swift source files under Sources/ (notably in BrowserPanel.swift and similar UI/engine-related strings) and flag only brand-name strings that are part of user-facing UI text appropriately for translation scope.
Learnt from: kjb0787
Repo: manaflow-ai/cmux PR: 1461
File: Sources/GhosttyTerminalView.swift:5904-5905
Timestamp: 2026-03-15T19:22:32.330Z
Learning: In Swift files under the Sources directory that manage terminal/scroll behavior, ensure the following: when preserving scroll across workspace switches, save savedScrollRow only if the scrollbar offset is greater than 0 (indicating the user has scrolled up). On restore, call scroll_to_row only if savedScrollRow is non-nil; if it is nil, rely on synchronizeScrollView() to keep bottom-pinned sessions following new output. This pattern should be applied wherever GhosttyTerminalView-like views implement setVisibleInUI(_:) to maintain consistent user scroll state across workspace switches.
Learnt from: MaTriXy
Repo: manaflow-ai/cmux PR: 1460
File: Sources/TerminalController.swift:0-0
Timestamp: 2026-03-16T08:02:06.824Z
Learning: In Swift sources, for any panel_id-only route handling in v2PanelMarkBackground(params:) and v2PanelMarkForeground(params:), first attempt v2ResolveTabManager(params:). Use the manager only if it actually owns the panelId; otherwise fall back to AppDelegate.shared?.locateSurface(surfaceId:) to locate the correct TabManager across windows. Apply this pattern to all panel_id-only routes to avoid active-window bias.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 12 files (changes from recent commits).
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="cmuxTests/GhosttyConfigTests.swift">
<violation number="1" location="cmuxTests/GhosttyConfigTests.swift:1702">
P2: The expected Safari profile order is incorrect; detector sorting puts "Travel" before "Work", so this assertion is flaky/incorrect against current implementation.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| return | ||
| } | ||
|
|
||
| XCTAssertEqual(safari.profiles.map(\.displayName), ["Default", "Work", "Travel"]) |
There was a problem hiding this comment.
P2: The expected Safari profile order is incorrect; detector sorting puts "Travel" before "Work", so this assertion is flaky/incorrect against current implementation.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At cmuxTests/GhosttyConfigTests.swift, line 1702:
<comment>The expected Safari profile order is incorrect; detector sorting puts "Travel" before "Work", so this assertion is flaky/incorrect against current implementation.</comment>
<file context>
@@ -1614,6 +1615,104 @@ final class BrowserInstallDetectorTests: XCTestCase {
+ return
+ }
+
+ XCTAssertEqual(safari.profiles.map(\.displayName), ["Default", "Work", "Travel"])
+ XCTAssertEqual(
+ safari.profiles.map { $0.rootURL.path(percentEncoded: false) }.sorted(),
</file context>
Suggested change
| XCTAssertEqual(safari.profiles.map(\.displayName), ["Default", "Work", "Travel"]) | |
| XCTAssertEqual(safari.profiles.map(\.displayName), ["Default", "Travel", "Work"]) | |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dc6bcb259a
ℹ️ 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".
|
|
||
| // Set up navigation delegate | ||
| let navDelegate = BrowserNavigationDelegate() | ||
| navDelegate.didFinish = { webView in | ||
| BrowserHistoryStore.shared.recordVisit(url: webView.url, title: webView.title) | ||
| Task { @MainActor [weak self] in | ||
| self?.historyStore.recordVisit(url: webView.url, title: webView.title) |
There was a problem hiding this comment.
Record the visit only after confirming this callback belongs to the active WKWebView. Right now didFinish writes to self?.historyStore before any isCurrentWebView check, so if a user switches browser profiles (or the panel swaps webviews) while a navigation is completing, a stale webview completion can write history into the newly selected profile's store and break profile isolation.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Sources/Workspace.swift (1)
2311-2324:⚠️ Potential issue | 🟠 MajorUse the selected tab in
paneIdas the default profile source.
newBrowserSurfacecurrently resolves only from an explicitpreferredProfileIDor the workspace-globalpreferredBrowserProfileID. The.didRequestNewTab(kind: "browser")path on Line 5125 calls this without a profile, so a pane that is already on profile A can still open the new tab under profile B from another pane or restore order.Suggested fix
) -> BrowserPanel? { let shouldFocusNewTab = focus ?? (bonsplitController.focusedPaneId == paneId) + let sourcePanelId = bonsplitController.selectedTab(inPane: paneId) + .flatMap { panelIdFromSurfaceId($0.id) } let browserPanel = BrowserPanel( workspaceId: id, - profileID: resolvedNewBrowserProfileID(preferredProfileID: preferredProfileID), + profileID: resolvedNewBrowserProfileID( + preferredProfileID: preferredProfileID, + sourcePanelId: sourcePanelId + ), initialURL: url, bypassInsecureHTTPHostOnce: bypassInsecureHTTPHostOnce )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/Workspace.swift` around lines 2311 - 2324, newBrowserSurface currently only uses preferredProfileID or workspace preferredBrowserProfileID; update its profile resolution so that when preferredProfileID is nil it first checks the selected tab in the target pane (use bonsplitController to get the pane by paneId and read its selected tab's profileID) and only fallback to resolvedNewBrowserProfileID(preferredProfileID:) or workspace preferredBrowserProfileID if the pane has no selected tab/profile; modify the code path that computes profileID passed into BrowserPanel to prefer the pane's selected tab profile before other defaults.
♻️ Duplicate comments (1)
Sources/Panels/BrowserPanelView.swift (1)
1244-1244:⚠️ Potential issue | 🟠 MajorLocalize the installed-browser summary before rendering it in UI text.
Rendering
InstalledBrowserDetector.summaryText(for:)directly here can still bypass localization/pluralization guarantees unless that helper already returns localized content (including count forms like “+N more”).As per coding guidelines: “All user-facing strings must be localized... Keys must be in
Resources/Localizable.xcstringswith translations for all supported languages...”.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/Panels/BrowserPanelView.swift` at line 1244, InstalledBrowserDetector.summaryText(for:) is being passed raw into Text which may bypass localization/pluralization; update the UI to use a localized string key instead: have InstalledBrowserDetector provide either a LocalizedStringKey or a localization key + format args, then call Text(LocalizedStringKey(key), tableName: nil, bundle: .main) or Text(String(format: NSLocalizedString(key, comment: ""), args...)) so SwiftUI pluralization works; also add the new key(s) to Resources/Localizable.xcstrings and ensure translations are present for all supported languages and update any callers of InstalledBrowserDetector.summaryText(for:) to use the new localized output.
🧹 Nitpick comments (4)
cmuxTests/SessionPersistenceTests.swift (1)
153-157: UseXCTUnwrapfor the fixture UUID to enforce test intent.Right now a malformed UUID literal would silently become
niland reduce assertion value.Suggested tweak
- let profileID = UUID(uuidString: "8F03A658-5A84-428B-AD03-5A6D04692F64") + let profileID = try XCTUnwrap(UUID(uuidString: "8F03A658-5A84-428B-AD03-5A6D04692F64"))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmuxTests/SessionPersistenceTests.swift` around lines 153 - 157, The test uses UUID(uuidString: "...") to create profileID which returns an optional and can silently be nil; change the assignment to assert and unwrap the fixture using XCTUnwrap (e.g., replace the direct UUID(...) call with try XCTUnwrap(UUID(uuidString: "..."))) before passing profileID into SessionBrowserPanelSnapshot so the test fails loudly on malformed UUIDs; update any enclosing test function signature to allow `throws` if necessary so XCTUnwrap can be used.cmuxTests/GhosttyConfigTests.swift (1)
1720-1723: Fail fast if test fixture file creation fails.
FileManager.createFilereturnsfalseon failure; currently that’s silently ignored, which can hide setup issues.Suggested helper fix
private func createFile(at url: URL, contents: Data) throws { try FileManager.default.createDirectory(at: url.deletingLastPathComponent(), withIntermediateDirectories: true) - _ = FileManager.default.createFile(atPath: url.path, contents: contents) + guard FileManager.default.createFile(atPath: url.path, contents: contents) else { + throw CocoaError(.fileWriteUnknown, userInfo: [NSFilePathErrorKey: url.path]) + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmuxTests/GhosttyConfigTests.swift` around lines 1720 - 1723, The test helper createFile(at:contents:) currently ignores FileManager.createFile's Bool return and can silently fail; update the createFile function to check the Bool result and throw an error (or fail the test) when createFile returns false so setup failures are surfaced; i.e., after calling FileManager.default.createFile(atPath: url.path, contents: contents) guard the returned Bool and throw a descriptive NSError (including url.path) or use XCTFail to abort the test, keeping the createFile(at:contents:) signature that throws.cmuxUITests/BrowserImportProfilesUITests.swift (1)
131-147: Optional: Simplify by removing the post-loop check.The
guardblock after the loop (lines 142-146) is nearly unreachable — if the file contained valid JSON, it would have been returned from within the loop. The only scenario where the post-loop check succeeds is a sub-millisecond race where the file appears exactly as the deadline passes.♻️ Simplified helper
private func waitForCapturedSelection(timeout: TimeInterval) -> [String: Any]? { let deadline = Date().addingTimeInterval(timeout) let url = URL(fileURLWithPath: capturePath) while Date() < deadline { if let data = try? Data(contentsOf: url), let object = try? JSONSerialization.jsonObject(with: data) as? [String: Any] { return object } RunLoop.current.run(until: Date().addingTimeInterval(0.05)) } - - guard let data = try? Data(contentsOf: url), - let object = try? JSONSerialization.jsonObject(with: data) as? [String: Any] else { - return nil - } - return object + return nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmuxUITests/BrowserImportProfilesUITests.swift` around lines 131 - 147, The post-loop guard in waitForCapturedSelection is redundant because any valid JSON would have been returned inside the loop; remove the final guard/JSON-parsing block that reads Data(contentsOf: url) and JSONSerialization again, and replace it with a simple return nil at the end of the function so the method only retries inside the loop and returns nil if the deadline passes (referencing function waitForCapturedSelection, variables capturePath and deadline).Sources/Panels/BrowserPanelView.swift (1)
1331-1354: Coalesce or debounce refresh calls to prevent overlapping filesystem scans.The code creates a new
Task.detachedfor eachrefreshEmptyStateImportBrowsers()call. SinceInstalledBrowserDetector.detectInstalledBrowsers()is a synchronous function that performs blockingFileManager.contentsOfDirectory()operations without internal cancellation checks, rapid state changes (e.g., consecutive URL updates inonChange(of: panel.currentURL)) spawn multiple concurrent filesystem scans. Theguard !Task.isCancelledcheck at line 1346 only prevents result processing after the scan completes—it doesn't stop the already-running detached work.While the generation counter prevents stale results from being applied, the redundant scans waste resources. Consider debouncing the refresh trigger or ensuring only one scan runs at a time.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/Panels/BrowserPanelView.swift` around lines 1331 - 1354, refreshEmptyStateImportBrowsers currently spawns a Task.detached for InstalledBrowserDetector.detectInstalledBrowsers which is not cancelled when emptyStateImportBrowserRefreshTask is cancelled, causing overlapping filesystem scans; change the implementation to run detectInstalledBrowsers inside the same cancellable Task (i.e., remove Task.detached and call await InstalledBrowserDetector.detectInstalledBrowsers() directly inside the Task assigned to emptyStateImportBrowserRefreshTask) so cancellation of emptyStateImportBrowserRefreshTask cancels the scan, or alternatively keep a handle to the detached Task and cancel that handle before starting a new scan; update refreshEmptyStateImportBrowsers to use emptyStateImportBrowserRefreshTask, InstalledBrowserDetector.detectInstalledBrowsers, and Task cancellation 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 `@cmuxTests/GhosttyConfigTests.swift`:
- Around line 1702-1703: The test currently asserts exact ordering of discovered
Safari profile names via safari.profiles.map(\.displayName) which can flake;
change the assertion to compare an order-insensitive collection (e.g., wrap the
mapped display names in a Set or sort both sides) so the assertion checks
membership rather than sequence equality—update the XCTAssertEqual line that
compares safari.profiles.map(\.displayName) to use Set(...) or a sorted array
comparison to ensure order-independence.
In `@Resources/Localizable.xcstrings`:
- Around line 4771-4787: The Japanese translation for "browser" is inconsistent;
update the string value for the key browser.import.complete.browser (currently
"ブラウザ: %@") to use the standardized form "ブラウザー: %@" and audit the related keys
menu.view.importFromBrowser, settings.browser.emptyImport.title, and
settings.browser.import (and the ranges noted: 37680-37696, 50060-50076,
50190-50206) to replace any remaining "ブラウザ" occurrences with "ブラウザー" so all
localizations use the same term.
In `@Sources/Panels/BrowserPanelView.swift`:
- Around line 1463-1467: Move the workspace preference update to occur only
after a successful panel switch: call panel.switchToProfile(profileID) first
(handling its return value or thrown error), and only when it succeeds call
owningWorkspace?.setPreferredBrowserProfileID(profileID); keep
isBrowserProfileMenuPresented = false behavior as appropriate (dismiss
regardless or after success per UX). Update applyBrowserProfileSelection to
check panel.switchToProfile's result/throw and avoid persisting the preference
on failure.
In `@Sources/Workspace.swift`:
- Around line 2258-2260: The code currently mutates panels, panelTitles and
calls setPreferredBrowserProfileID for browserPanel before the
splitPane/createTab (bonsplit) call succeeds, which can leave workspace state
inconsistent on failure; change the flow so that you only assign
panels[browserPanel.id] = browserPanel, panelTitles[browserPanel.id] =
browserPanel.displayTitle and call
setPreferredBrowserProfileID(browserPanel.profileID) after the
splitPane/createTab (or bonsplit) returns a non-nil/successful result (the same
place around the existing success-path update at Line 1337), and remove the
redundant eager writes at both the block around panels[...] (2258-2260) and the
similar block at 2327-2329 so state is only mutated on successful attach.
---
Outside diff comments:
In `@Sources/Workspace.swift`:
- Around line 2311-2324: newBrowserSurface currently only uses
preferredProfileID or workspace preferredBrowserProfileID; update its profile
resolution so that when preferredProfileID is nil it first checks the selected
tab in the target pane (use bonsplitController to get the pane by paneId and
read its selected tab's profileID) and only fallback to
resolvedNewBrowserProfileID(preferredProfileID:) or workspace
preferredBrowserProfileID if the pane has no selected tab/profile; modify the
code path that computes profileID passed into BrowserPanel to prefer the pane's
selected tab profile before other defaults.
---
Duplicate comments:
In `@Sources/Panels/BrowserPanelView.swift`:
- Line 1244: InstalledBrowserDetector.summaryText(for:) is being passed raw into
Text which may bypass localization/pluralization; update the UI to use a
localized string key instead: have InstalledBrowserDetector provide either a
LocalizedStringKey or a localization key + format args, then call
Text(LocalizedStringKey(key), tableName: nil, bundle: .main) or
Text(String(format: NSLocalizedString(key, comment: ""), args...)) so SwiftUI
pluralization works; also add the new key(s) to Resources/Localizable.xcstrings
and ensure translations are present for all supported languages and update any
callers of InstalledBrowserDetector.summaryText(for:) to use the new localized
output.
---
Nitpick comments:
In `@cmuxTests/GhosttyConfigTests.swift`:
- Around line 1720-1723: The test helper createFile(at:contents:) currently
ignores FileManager.createFile's Bool return and can silently fail; update the
createFile function to check the Bool result and throw an error (or fail the
test) when createFile returns false so setup failures are surfaced; i.e., after
calling FileManager.default.createFile(atPath: url.path, contents: contents)
guard the returned Bool and throw a descriptive NSError (including url.path) or
use XCTFail to abort the test, keeping the createFile(at:contents:) signature
that throws.
In `@cmuxTests/SessionPersistenceTests.swift`:
- Around line 153-157: The test uses UUID(uuidString: "...") to create profileID
which returns an optional and can silently be nil; change the assignment to
assert and unwrap the fixture using XCTUnwrap (e.g., replace the direct
UUID(...) call with try XCTUnwrap(UUID(uuidString: "..."))) before passing
profileID into SessionBrowserPanelSnapshot so the test fails loudly on malformed
UUIDs; update any enclosing test function signature to allow `throws` if
necessary so XCTUnwrap can be used.
In `@cmuxUITests/BrowserImportProfilesUITests.swift`:
- Around line 131-147: The post-loop guard in waitForCapturedSelection is
redundant because any valid JSON would have been returned inside the loop;
remove the final guard/JSON-parsing block that reads Data(contentsOf: url) and
JSONSerialization again, and replace it with a simple return nil at the end of
the function so the method only retries inside the loop and returns nil if the
deadline passes (referencing function waitForCapturedSelection, variables
capturePath and deadline).
In `@Sources/Panels/BrowserPanelView.swift`:
- Around line 1331-1354: refreshEmptyStateImportBrowsers currently spawns a
Task.detached for InstalledBrowserDetector.detectInstalledBrowsers which is not
cancelled when emptyStateImportBrowserRefreshTask is cancelled, causing
overlapping filesystem scans; change the implementation to run
detectInstalledBrowsers inside the same cancellable Task (i.e., remove
Task.detached and call await InstalledBrowserDetector.detectInstalledBrowsers()
directly inside the Task assigned to emptyStateImportBrowserRefreshTask) so
cancellation of emptyStateImportBrowserRefreshTask cancels the scan, or
alternatively keep a handle to the detached Task and cancel that handle before
starting a new scan; update refreshEmptyStateImportBrowsers to use
emptyStateImportBrowserRefreshTask,
InstalledBrowserDetector.detectInstalledBrowsers, and Task cancellation
consistently.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 328ad552-c9f3-4426-be09-d19ea7dc4ab4
📒 Files selected for processing (12)
GhosttyTabs.xcodeproj/project.pbxprojResources/Localizable.xcstringsSources/AppDelegate.swiftSources/Panels/BrowserPanel.swiftSources/Panels/BrowserPanelView.swiftSources/SessionPersistence.swiftSources/TabManager.swiftSources/Workspace.swiftcmuxTests/BrowserImportMappingTests.swiftcmuxTests/GhosttyConfigTests.swiftcmuxTests/SessionPersistenceTests.swiftcmuxUITests/BrowserImportProfilesUITests.swift
Comment on lines
+1702
to
+1703
| XCTAssertEqual(safari.profiles.map(\.displayName), ["Default", "Work", "Travel"]) | ||
| XCTAssertEqual( |
There was a problem hiding this comment.
Avoid order-coupled assertion for discovered Safari profiles.
This assertion can flake if profile discovery order changes (filesystem/enumeration differences). Compare as a set (or sorted arrays) instead of exact sequence.
Suggested test hardening
- XCTAssertEqual(safari.profiles.map(\.displayName), ["Default", "Work", "Travel"])
+ XCTAssertEqual(
+ Set(safari.profiles.map(\.displayName)),
+ Set(["Default", "Work", "Travel"])
+ )📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| XCTAssertEqual(safari.profiles.map(\.displayName), ["Default", "Work", "Travel"]) | |
| XCTAssertEqual( | |
| XCTAssertEqual( | |
| Set(safari.profiles.map(\.displayName)), | |
| Set(["Default", "Work", "Travel"]) | |
| ) | |
| XCTAssertEqual( |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cmuxTests/GhosttyConfigTests.swift` around lines 1702 - 1703, The test
currently asserts exact ordering of discovered Safari profile names via
safari.profiles.map(\.displayName) which can flake; change the assertion to
compare an order-insensitive collection (e.g., wrap the mapped display names in
a Set or sort both sides) so the assertion checks membership rather than
sequence equality—update the XCTAssertEqual line that compares
safari.profiles.map(\.displayName) to use Set(...) or a sorted array comparison
to ensure order-independence.
Comment on lines
+4771
to
+4787
| "browser.import.complete.browser": { | ||
| "extractionState": "manual", | ||
| "localizations": { | ||
| "en": { | ||
| "stringUnit": { | ||
| "state": "translated", | ||
| "value": "Browser: %@" | ||
| } | ||
| }, | ||
| "ja": { | ||
| "stringUnit": { | ||
| "state": "translated", | ||
| "value": "ブラウザ: %@" | ||
| } | ||
| } | ||
| } | ||
| }, |
There was a problem hiding this comment.
Inconsistent Japanese terminology for "browser".
The Japanese translations mix "ブラウザー" (with long vowel mark, per JIS standard) and "ブラウザ" (short form):
| Key | Value |
|---|---|
browser.import.complete.browser (line 4783) |
"ブラウザ: %@" |
menu.view.importFromBrowser (line 37692) |
"ブラウザから取り込む…" |
settings.browser.emptyImport.title (line 50072) |
"ブラウザデータを取り込む" |
settings.browser.import (line 50202) |
"ブラウザから取り込む" |
Most other keys use "ブラウザー" (e.g., lines 4562, 4664, 4970, 5650). Consider standardizing on one form—"ブラウザー" is the more common choice in macOS Japanese localizations.
Also applies to: 37680-37696, 50060-50076, 50190-50206
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Resources/Localizable.xcstrings` around lines 4771 - 4787, The Japanese
translation for "browser" is inconsistent; update the string value for the key
browser.import.complete.browser (currently "ブラウザ: %@") to use the standardized
form "ブラウザー: %@" and audit the related keys menu.view.importFromBrowser,
settings.browser.emptyImport.title, and settings.browser.import (and the ranges
noted: 37680-37696, 50060-50076, 50190-50206) to replace any remaining "ブラウザ"
occurrences with "ブラウザー" so all localizations use the same term.
Comment on lines
+1463
to
+1467
| private func applyBrowserProfileSelection(_ profileID: UUID) { | ||
| isBrowserProfileMenuPresented = false | ||
| owningWorkspace?.setPreferredBrowserProfileID(profileID) | ||
| _ = panel.switchToProfile(profileID) | ||
| } |
There was a problem hiding this comment.
Persist preferred profile only after a successful panel switch.
setPreferredBrowserProfileID(profileID) currently runs before panel.switchToProfile(profileID). If switching fails, workspace preference can drift from the actual active profile.
Suggested change
private func applyBrowserProfileSelection(_ profileID: UUID) {
isBrowserProfileMenuPresented = false
- owningWorkspace?.setPreferredBrowserProfileID(profileID)
- _ = panel.switchToProfile(profileID)
+ guard panel.switchToProfile(profileID) else { return }
+ owningWorkspace?.setPreferredBrowserProfileID(profileID)
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Sources/Panels/BrowserPanelView.swift` around lines 1463 - 1467, Move the
workspace preference update to occur only after a successful panel switch: call
panel.switchToProfile(profileID) first (handling its return value or thrown
error), and only when it succeeds call
owningWorkspace?.setPreferredBrowserProfileID(profileID); keep
isBrowserProfileMenuPresented = false behavior as appropriate (dismiss
regardless or after success per UX). Update applyBrowserProfileSelection to
check panel.switchToProfile's result/throw and avoid persisting the preference
on failure.
Comment on lines
2258
to
+2260
| panels[browserPanel.id] = browserPanel | ||
| panelTitles[browserPanel.id] = browserPanel.displayTitle | ||
| setPreferredBrowserProfileID(browserPanel.profileID) |
There was a problem hiding this comment.
Don't mutate preferredBrowserProfileID until creation succeeds.
These writes happen before splitPane/createTab succeeds. If the Bonsplit call returns nil, the method still changes the workspace default even though no browser tab was created. Since Line 1337 already updates the preference after a successful attach, these eager writes are redundant and make the failure path stateful.
Suggested fix
panels[browserPanel.id] = browserPanel
panelTitles[browserPanel.id] = browserPanel.displayTitle
- setPreferredBrowserProfileID(browserPanel.profileID)
// Pre-generate the bonsplit tab ID so the mapping exists before the split lands. panels[browserPanel.id] = browserPanel
panelTitles[browserPanel.id] = browserPanel.displayTitle
- setPreferredBrowserProfileID(browserPanel.profileID)
guard let newTabId = bonsplitController.createTab(Also applies to: 2327-2329
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Sources/Workspace.swift` around lines 2258 - 2260, The code currently mutates
panels, panelTitles and calls setPreferredBrowserProfileID for browserPanel
before the splitPane/createTab (bonsplit) call succeeds, which can leave
workspace state inconsistent on failure; change the flow so that you only assign
panels[browserPanel.id] = browserPanel, panelTitles[browserPanel.id] =
browserPanel.displayTitle and call
setPreferredBrowserProfileID(browserPanel.profileID) after the
splitPane/createTab (or bonsplit) returns a non-nil/successful result (the same
place around the existing success-path update at Line 1337), and remove the
redundant eager writes at both the block around panels[...] (2258-2260) and the
similar block at 2327-2329 so state is only mutated on successful attach.
This was referenced Mar 17, 2026
bn-l
pushed a commit
to bn-l/cmux
that referenced
this pull request
Apr 3, 2026
…browser-cookies-history-settings Browser: import data from detected installed browsers
This branch was successfully deployed
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
Verification
xcodebuild -project GhosttyTabs.xcodeproj -scheme cmux -configuration Debug -destination 'platform=macOS' build\n-xcodebuild -project GhosttyTabs.xcodeproj -scheme cmux-unit -destination 'platform=macOS' -only-testing:cmuxTests/BrowserInstallDetectorTests test\n-./scripts/reload.sh --tag issue-122-browser-import\n\nCloses Import cookies, history, and settings from other browsers #122Summary by CodeRabbit
New Features
Localization
Tests