fix: browser import profile follow-up regressions#1582
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (10)
📝 WalkthroughWalkthroughAdds a browser import hint UI and import-wizard dynamic sizing/presentation, centralizes WebView navigation delegate callbacks, implements source-panel-aware browser profile inheritance for new browser surfaces, expands localization entries, and adds unit and UI tests for import/profile behaviors. No API signature removals; some enums/structs and debug hooks were added. Changes
Sequence Diagram(s)sequenceDiagram
participant App as AppDelegate
participant Workspace
participant ProfileStore as BrowserProfileStore
participant Panel as BrowserPanel
participant PanelView as BrowserPanelView
participant WebView
participant ImportCoord as BrowserDataImportCoordinator
App->>Workspace: launch / configure (UI-test import-hint env)
Workspace->>ProfileStore: resolveNewBrowserProfileID(sourcePanelId?)
ProfileStore-->>Workspace: profileID
Workspace->>Panel: create panel (profileID, sourcePanelId)
Panel->>PanelView: render (import hint state)
Panel->>WebView: bind & configure navigation callbacks
WebView-->>Panel: didFinish / didFail navigation
Panel->>Panel: recordVisit (if profile matches)
PanelView->>ImportCoord: presentImportDialog() / open settings
ImportCoord-->>PanelView: show import-wizard (dynamic sizing / steps)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![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 8 files
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="cmuxUITests/BrowserImportProfilesUITests.swift">
<violation number="1" location="cmuxUITests/BrowserImportProfilesUITests.swift:141">
P2: Do one final capture-file read before returning `nil`; otherwise writes that occur near the timeout boundary can be missed and make the UI test flaky.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cmuxUITests/BrowserImportProfilesUITests.swift (1)
131-141:⚠️ Potential issue | 🟡 MinorRestore the final file read after the poll loop.
This now misses captures written during the last sleep interval, so the test can fail even when the JSON lands before the timeout expires.
💡 Suggested fix
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)) } + if let data = try? Data(contentsOf: url), + let object = try? JSONSerialization.jsonObject(with: data) as? [String: Any] { + 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 - 141, The poll loop in waitForCapturedSelection(timeout:) can miss a capture written during the last sleep interval because it returns nil immediately after the loop; add one final attempt to read and parse the file at capturePath (same logic using Data(contentsOf:) and JSONSerialization.jsonObject(with:) as used inside the loop) after the while loop but before returning nil so captures that arrive right at the deadline are picked up.
🧹 Nitpick comments (3)
Sources/Workspace.swift (1)
2389-2396: Harden source-profile inheritance with the same validation as the other branches.The new
sourcePanelIdpath flows throughresolvedNewBrowserProfileID(...), but that resolver only validatespreferredProfileIDandpreferredBrowserProfileID. If the selected source browser is still carrying a deleted profile, this path will propagate a stale UUID into the new surface instead of falling back to a valid profile.🔧 Suggested hardening in `resolvedNewBrowserProfileID`
private func resolvedNewBrowserProfileID( preferredProfileID: UUID? = nil, sourcePanelId: UUID? = nil ) -> UUID { if let preferredProfileID, BrowserProfileStore.shared.profileDefinition(id: preferredProfileID) != nil { return preferredProfileID } if let sourcePanelId, let sourceBrowserPanel = browserPanel(for: sourcePanelId), BrowserProfileStore.shared.profileDefinition(id: sourceBrowserPanel.profileID) != nil { return sourceBrowserPanel.profileID } if let preferredBrowserProfileID, BrowserProfileStore.shared.profileDefinition(id: preferredBrowserProfileID) != nil { return preferredBrowserProfileID } return BrowserProfileStore.shared.effectiveLastUsedProfileID }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/Workspace.swift` around lines 2389 - 2396, The selected-source path is not validated: update resolvedNewBrowserProfileID to accept sourcePanelId and validate the source panel's profile the same way as other branches; specifically, inside resolvedNewBrowserProfileID preserve the existing checks for preferredProfileID and preferredBrowserProfileID, then if sourcePanelId is non-nil call browserPanel(for: sourcePanelId) and verify BrowserProfileStore.shared.profileDefinition(id: sourceBrowserPanel.profileID) != nil before returning sourceBrowserPanel.profileID, otherwise fall back to BrowserProfileStore.shared.effectiveLastUsedProfileID; update the call site (where effectiveSelectedPanelId(inPane:) is used to compute sourcePanelId for BrowserPanel init) to pass that sourcePanelId into resolvedNewBrowserProfileID.cmuxTests/GhosttyConfigTests.swift (1)
2031-2033: Consider attaching the failing path to the thrown write error.The fail-fast behavior is great; adding
NSFilePathErrorKeywill make failures easier to diagnose in CI logs.Proposed tweak
- guard FileManager.default.createFile(atPath: url.path, contents: contents) else { - throw CocoaError(.fileWriteUnknown) + 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 2031 - 2033, Guard that creates the file currently throws a CocoaError(.fileWriteUnknown) without context; modify the throw to include a userInfo dictionary with NSFilePathErrorKey set to url.path so failures show the path (e.g., throw CocoaError(.fileWriteUnknown, userInfo: [NSFilePathErrorKey: url.path])) and keep the FileManager.default.createFile(atPath:contents:) check and behavior otherwise unchanged.cmuxTests/CmuxWebViewKeyEquivalentTests.swift (1)
6280-6286: Consider deduplicatingmakeProfile(named:)across the two new test classes.A single file-private fixture helper would reduce maintenance drift for profile test setup.
Also applies to: 6456-6462
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmuxTests/CmuxWebViewKeyEquivalentTests.swift` around lines 6280 - 6286, The helper makeProfile(named:) is duplicated between two test classes; extract it as a single file-private fixture function (e.g., file-private func makeProfile(named: String) throws -> BrowserProfileDefinition) and have both test classes call this shared helper instead of redefining it; use the existing logic that calls BrowserProfileStore.shared.createProfile(named: "\(prefix)-\(UUID().uuidString)") and wrap with try XCTUnwrap to preserve current behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@cmuxUITests/BrowserImportProfilesUITests.swift`:
- Around line 131-141: The poll loop in waitForCapturedSelection(timeout:) can
miss a capture written during the last sleep interval because it returns nil
immediately after the loop; add one final attempt to read and parse the file at
capturePath (same logic using Data(contentsOf:) and
JSONSerialization.jsonObject(with:) as used inside the loop) after the while
loop but before returning nil so captures that arrive right at the deadline are
picked up.
---
Nitpick comments:
In `@cmuxTests/CmuxWebViewKeyEquivalentTests.swift`:
- Around line 6280-6286: The helper makeProfile(named:) is duplicated between
two test classes; extract it as a single file-private fixture function (e.g.,
file-private func makeProfile(named: String) throws -> BrowserProfileDefinition)
and have both test classes call this shared helper instead of redefining it; use
the existing logic that calls BrowserProfileStore.shared.createProfile(named:
"\(prefix)-\(UUID().uuidString)") and wrap with try XCTUnwrap to preserve
current behavior.
In `@cmuxTests/GhosttyConfigTests.swift`:
- Around line 2031-2033: Guard that creates the file currently throws a
CocoaError(.fileWriteUnknown) without context; modify the throw to include a
userInfo dictionary with NSFilePathErrorKey set to url.path so failures show the
path (e.g., throw CocoaError(.fileWriteUnknown, userInfo: [NSFilePathErrorKey:
url.path])) and keep the FileManager.default.createFile(atPath:contents:) check
and behavior otherwise unchanged.
In `@Sources/Workspace.swift`:
- Around line 2389-2396: The selected-source path is not validated: update
resolvedNewBrowserProfileID to accept sourcePanelId and validate the source
panel's profile the same way as other branches; specifically, inside
resolvedNewBrowserProfileID preserve the existing checks for preferredProfileID
and preferredBrowserProfileID, then if sourcePanelId is non-nil call
browserPanel(for: sourcePanelId) and verify
BrowserProfileStore.shared.profileDefinition(id: sourceBrowserPanel.profileID)
!= nil before returning sourceBrowserPanel.profileID, otherwise fall back to
BrowserProfileStore.shared.effectiveLastUsedProfileID; update the call site
(where effectiveSelectedPanelId(inPane:) is used to compute sourcePanelId for
BrowserPanel init) to pass that sourcePanelId into resolvedNewBrowserProfileID.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6de5a2ae-956b-4541-8f99-ca314fa2205f
📒 Files selected for processing (8)
Resources/Localizable.xcstringsSources/Panels/BrowserPanel.swiftSources/Panels/BrowserPanelView.swiftSources/Workspace.swiftcmuxTests/CmuxWebViewKeyEquivalentTests.swiftcmuxTests/GhosttyConfigTests.swiftcmuxTests/SessionPersistenceTests.swiftcmuxUITests/BrowserImportProfilesUITests.swift
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Addressed the remaining cubic and CodeRabbit follow-up comments in aac8a41. This restores the final timeout-boundary capture read in |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/CmuxWebViewKeyEquivalentTests.swift`:
- Around line 68-75: The helper makeTemporaryBrowserProfile currently creates
profiles via BrowserProfileStore.shared.createProfile but never removes them;
update tests to record the returned BrowserProfileDefinition (or its identifier)
and ensure teardown removes the created profile using
BrowserProfileStore.shared.removeProfile (or the store's deletion API) — e.g.,
capture the created profile inside makeTemporaryBrowserProfile or return an
opaque handle and register it for removal in the test's tearDown method so each
profile created by makeTemporaryBrowserProfile is deleted after the test.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9733e021-d92d-46d2-b84c-7e1f6b3c947e
📒 Files selected for processing (4)
Sources/Workspace.swiftcmuxTests/CmuxWebViewKeyEquivalentTests.swiftcmuxTests/GhosttyConfigTests.swiftcmuxUITests/BrowserImportProfilesUITests.swift
| @MainActor | ||
| private func makeTemporaryBrowserProfile(named prefix: String) throws -> BrowserProfileDefinition { | ||
| try XCTUnwrap( | ||
| BrowserProfileStore.shared.createProfile( | ||
| named: "\(prefix)-\(UUID().uuidString)" | ||
| ) | ||
| ) | ||
| } |
There was a problem hiding this comment.
Add teardown cleanup for temporary browser profiles.
This helper creates profiles in the shared store but never removes them. Over time, repeated test runs can accumulate persistent profiles and destabilize profile-dependent tests. Please clean up created profiles in teardown using the profile-store removal API.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cmuxTests/CmuxWebViewKeyEquivalentTests.swift` around lines 68 - 75, The
helper makeTemporaryBrowserProfile currently creates profiles via
BrowserProfileStore.shared.createProfile but never removes them; update tests to
record the returned BrowserProfileDefinition (or its identifier) and ensure
teardown removes the created profile using
BrowserProfileStore.shared.removeProfile (or the store's deletion API) — e.g.,
capture the created profile inside makeTemporaryBrowserProfile or return an
opaque handle and register it for removal in the test's tearDown method so each
profile created by makeTemporaryBrowserProfile is deleted after the test.
Tighten browser import sheet UI
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
…ollowups # Conflicts: # Sources/Workspace.swift
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
…ort-followups fix: browser import profile follow-up regressions
1 participant
Summary
Testing
Summary by cubic
Fixes browser profile handling and refines the browser‑import UI for a smoother setup. Adds import hint variants, debug controls, and a clear menu entry (with settings deep‑link) to start imports.
Bug Fixes
UI Improvements
Written for commit f5d610e. Summary will update on new commits.
Summary by CodeRabbit
Localization
New Features
Bug Fixes
Tests