Repository navigation
fix: address browser import follow-up review comments - #1638
Closed
lawrencecchen wants to merge 5 commits into
Closed
lawrencecchen wants to merge 5 commits into
lawrencecchen wants to merge 5 commits into
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
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}
Contributor
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.
📝 WalkthroughWalkthroughAdds guarded profile removal to BrowserProfileStore (removes data, history, updates lastUsedProfileID), changes default profile-selection fallback to treat all profiles as potential defaults, introduces buffered Settings navigation (pendingTarget) and a new auxiliary window identifier, centralizes temporary test profile tracking/teardown, and tightens import-wizard UI-test synchronization. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant SettingsNavigationRequest
participant SettingsWindowController
participant SettingsView
Note over SettingsNavigationRequest,SettingsWindowController: pendingTarget buffered until window is key
User->>SettingsNavigationRequest: post(navigationTarget)
SettingsNavigationRequest->>SettingsNavigationRequest: store pendingTarget
SettingsWindowController->>SettingsWindowController: windowDidBecomeKey
SettingsWindowController->>SettingsNavigationRequest: postPendingNavigationTargetIfPossible()
SettingsNavigationRequest->>SettingsView: Notification(with target)
SettingsView->>SettingsNavigationRequest: consumePendingTarget()
SettingsView->>SettingsView: perform scroll/navigation (animated or immediate)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
cmuxTests/CmuxWebViewKeyEquivalentTests.swift (1)
82-87: Make cleanup failures visible to avoid hidden test pollution.
cleanup()ignoresremoveProfilefailures, so leaked profiles can silently affect later tests. Please surface failures in teardown.💡 Suggested patch
`@MainActor` private final class TemporaryBrowserProfileTracker { private var createdProfileIDs: [UUID] = [] @@ - func cleanup() { + `@discardableResult` + func cleanup() -> [UUID] { + var failedProfileIDs: [UUID] = [] for profileID in createdProfileIDs.reversed() { - _ = BrowserProfileStore.shared.removeProfile(id: profileID) + if !BrowserProfileStore.shared.removeProfile(id: profileID) { + failedProfileIDs.append(profileID) + } } createdProfileIDs.removeAll() + return failedProfileIDs } }override func tearDown() { - temporaryProfileTracker.cleanup() + XCTAssertTrue( + temporaryProfileTracker.cleanup().isEmpty, + "Temporary browser profiles failed to clean up" + ) super.tearDown() }override func tearDown() { - temporaryProfileTracker.cleanup() + XCTAssertTrue( + temporaryProfileTracker.cleanup().isEmpty, + "Temporary browser profiles failed to clean up" + ) super.tearDown() }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmuxTests/CmuxWebViewKeyEquivalentTests.swift` around lines 82 - 87, The cleanup() helper currently swallows failures from BrowserProfileStore.shared.removeProfile(id:) when iterating createdProfileIDs, which can silently leak profiles; update cleanup() to capture and surface errors (e.g., assert, XCTFail, or rethrow) when removeProfile returns failure or throws, and ensure you still attempt removal for remaining profile IDs (iterate all IDs even on error) before clearing createdProfileIDs so test pollution is visible and teardown remains robust; refer to cleanup(), BrowserProfileStore.shared.removeProfile(id:), and createdProfileIDs when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@cmuxTests/CmuxWebViewKeyEquivalentTests.swift`:
- Around line 82-87: The cleanup() helper currently swallows failures from
BrowserProfileStore.shared.removeProfile(id:) when iterating createdProfileIDs,
which can silently leak profiles; update cleanup() to capture and surface errors
(e.g., assert, XCTFail, or rethrow) when removeProfile returns failure or
throws, and ensure you still attempt removal for remaining profile IDs (iterate
all IDs even on error) before clearing createdProfileIDs so test pollution is
visible and teardown remains robust; refer to cleanup(),
BrowserProfileStore.shared.removeProfile(id:), and createdProfileIDs when making
the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e3a6e0b1-17f5-4083-82aa-3d1489d4f109
📒 Files selected for processing (4)
Sources/Panels/BrowserPanel.swiftSources/cmuxApp.swiftcmuxTests/CmuxWebViewKeyEquivalentTests.swiftcmuxUITests/BrowserImportProfilesUITests.swift
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
cmuxUITests/BrowserImportProfilesUITests.swift (2)
210-215: Consider addingisHittablecheck to matchwaitForImportWizardpattern.Tests immediately click the checkbox after this wait (e.g., lines 100-102). Checking only
existswithoutisHittablecould lead to flaky failures if the checkbox appears in the view hierarchy but isn't yet interactable.private func waitForDataTypesStep(_ app: XCUIApplication) { let dataTypesReady = browserImportPollUntil(timeout: 5.0) { - app.checkBoxes["BrowserImportCookiesCheckbox"].exists + let checkbox = app.checkBoxes["BrowserImportCookiesCheckbox"] + return checkbox.exists && checkbox.isHittable } XCTAssertTrue(dataTypesReady, "Expected the import data-type step to finish loading") }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmuxUITests/BrowserImportProfilesUITests.swift` around lines 210 - 215, The waitForDataTypesStep helper currently polls only for existence of the cookies checkbox which can be visible but not yet interactable; update the closure used by browserImportPollUntil inside waitForDataTypesStep to assert both existence and hittability (e.g., checkBoxes["BrowserImportCookiesCheckbox"].exists && checkBoxes["BrowserImportCookiesCheckbox"].isHittable) so callers (and subsequent immediate taps) match the waitForImportWizard pattern and avoid flaky interactions.
203-208: Consider addingisHittablecheck for consistency.
waitForImportWizardnow checks bothexistsandisHittable, but this helper only checksexists. For consistency and to ensure the checkbox is interactable before proceeding, consider adding anisHittablecheck:private func waitForSourceProfilesStep(_ app: XCUIApplication) { let sourceProfilesReady = browserImportPollUntil(timeout: 5.0) { - app.checkBoxes["You"].exists || app.staticTexts["Step 2 of 3"].exists + let checkbox = app.checkBoxes["You"] + return (checkbox.exists && checkbox.isHittable) || app.staticTexts["Step 2 of 3"].exists } XCTAssertTrue(sourceProfilesReady, "Expected the source-profile step to finish loading") }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmuxUITests/BrowserImportProfilesUITests.swift` around lines 203 - 208, The helper waitForSourceProfilesStep currently waits only for existence; update its poll predicate (inside browserImportPollUntil) to require both existence and hittability for the "You" checkbox (app.checkBoxes["You"].exists && app.checkBoxes["You"].isHittable) or similarly require app.staticTexts["Step 2 of 3"].exists && app.staticTexts["Step 2 of 3"].isHittable, mirroring the waitForImportWizard behavior so the step is interactable before proceeding; modify the predicate in waitForSourceProfilesStep accordingly.
🤖 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/Panels/BrowserPanel.swift`:
- Around line 423-437: The history store is being removed by setting
historyStores[id] = nil while its debounced save Task may still be running; call
the store's clearHistory() to cancel its saveTask and perform clean deletion
before nil'ing the reference and removing files: locate where
profiles.remove(at: index) and historyStores[id] = nil are used and invoke
historyStores[id]?.clearHistory() (or the equivalent clear method on the store)
prior to setting historyStores[id] = nil and before removing the history file
returned by historyFileURL(for: id).
---
Nitpick comments:
In `@cmuxUITests/BrowserImportProfilesUITests.swift`:
- Around line 210-215: The waitForDataTypesStep helper currently polls only for
existence of the cookies checkbox which can be visible but not yet interactable;
update the closure used by browserImportPollUntil inside waitForDataTypesStep to
assert both existence and hittability (e.g.,
checkBoxes["BrowserImportCookiesCheckbox"].exists &&
checkBoxes["BrowserImportCookiesCheckbox"].isHittable) so callers (and
subsequent immediate taps) match the waitForImportWizard pattern and avoid flaky
interactions.
- Around line 203-208: The helper waitForSourceProfilesStep currently waits only
for existence; update its poll predicate (inside browserImportPollUntil) to
require both existence and hittability for the "You" checkbox
(app.checkBoxes["You"].exists && app.checkBoxes["You"].isHittable) or similarly
require app.staticTexts["Step 2 of 3"].exists && app.staticTexts["Step 2 of
3"].isHittable, mirroring the waitForImportWizard behavior so the step is
interactable before proceeding; modify the predicate in
waitForSourceProfilesStep accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7f4c3d02-9efd-4c42-8509-834f0a5e1385
📒 Files selected for processing (3)
Sources/Panels/BrowserPanel.swiftSources/cmuxApp.swiftcmuxUITests/BrowserImportProfilesUITests.swift
Comment on lines
+423
to
+437
| profiles.remove(at: index) | ||
| dataStores[id] = nil | ||
| historyStores[id] = nil | ||
|
|
||
| if let historyFileURL = historyFileURL(for: id) { | ||
| try? FileManager.default.removeItem(at: historyFileURL) | ||
| let profileDirectory = historyFileURL.deletingLastPathComponent() | ||
| if let remainingContents = try? FileManager.default.contentsOfDirectory( | ||
| at: profileDirectory, | ||
| includingPropertiesForKeys: nil | ||
| ), | ||
| remainingContents.isEmpty { | ||
| try? FileManager.default.removeItem(at: profileDirectory) | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "1) removeProfile implementation (check whether clearHistory is called before historyStores[id] eviction):"
rg -n -C4 'func removeProfile\(id: UUID\) -> Bool|historyStores\[id\]|clearHistory\(' Sources/Panels/BrowserPanel.swift
echo
echo "2) BrowserHistoryStore.clearHistory implementation (check cancellation semantics):"
rg -n -C4 'func clearHistory\(\)|saveTask\?\.cancel\(\)|saveTask = nil' Sources/Panels/BrowserPanel.swiftRepository: manaflow-ai/cmux
Length of output: 1998
Cancel pending history saves before evicting the profile history store
On line 425, historyStores[id] = nil removes the reference to the store without canceling its debounced save task. The underlying Task.detached continues independently and can rewrite the history file after deletion, reintroducing stale state.
Call clearHistory() before nil assignment. This method explicitly cancels saveTask (line 1273) and deletes the file properly.
Suggested fix
@@
- profiles.remove(at: index)
- dataStores[id] = nil
- historyStores[id] = nil
+ profiles.remove(at: index)
+ // Ensure pending debounced writes are cancelled before dropping the store.
+ historyStores[id]?.clearHistory()
+ historyStores[id] = nil
+ dataStores[id] = nil📝 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
| profiles.remove(at: index) | |
| dataStores[id] = nil | |
| historyStores[id] = nil | |
| if let historyFileURL = historyFileURL(for: id) { | |
| try? FileManager.default.removeItem(at: historyFileURL) | |
| let profileDirectory = historyFileURL.deletingLastPathComponent() | |
| if let remainingContents = try? FileManager.default.contentsOfDirectory( | |
| at: profileDirectory, | |
| includingPropertiesForKeys: nil | |
| ), | |
| remainingContents.isEmpty { | |
| try? FileManager.default.removeItem(at: profileDirectory) | |
| } | |
| } | |
| profiles.remove(at: index) | |
| // Ensure pending debounced writes are cancelled before dropping the store. | |
| historyStores[id]?.clearHistory() | |
| historyStores[id] = nil | |
| dataStores[id] = nil | |
| if let historyFileURL = historyFileURL(for: id) { | |
| try? FileManager.default.removeItem(at: historyFileURL) | |
| let profileDirectory = historyFileURL.deletingLastPathComponent() | |
| if let remainingContents = try? FileManager.default.contentsOfDirectory( | |
| at: profileDirectory, | |
| includingPropertiesForKeys: nil | |
| ), | |
| remainingContents.isEmpty { | |
| try? FileManager.default.removeItem(at: profileDirectory) | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Sources/Panels/BrowserPanel.swift` around lines 423 - 437, The history store
is being removed by setting historyStores[id] = nil while its debounced save
Task may still be running; call the store's clearHistory() to cancel its
saveTask and perform clean deletion before nil'ing the reference and removing
files: locate where profiles.remove(at: index) and historyStores[id] = nil are
used and invoke historyStores[id]?.clearHistory() (or the equivalent clear
method on the store) prior to setting historyStores[id] = nil and before
removing the history file returned by historyFileURL(for: id).
There was a problem hiding this comment.
🧹 Nitpick comments (1)
Sources/cmuxApp.swift (1)
2517-2544: Consider makingSettingsWindowControllerexplicitly@MainActorfor isolation symmetry.
SettingsNavigationRequestisolates its navigation state to the main actor (lines 2517, 2519, 2534, 2540), butSettingsWindowController's mutablependingNavigationTargetstate lacks compile-time isolation. Adding@MainActorto the class enforces that future modifications remain main-thread-safe and aligns with the patterns established inSettingsNavigationRequest.♻️ Proposed refactor
-final class SettingsWindowController: NSWindowController, NSWindowDelegate { +@MainActor +final class SettingsWindowController: NSWindowController, NSWindowDelegate {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/cmuxApp.swift` around lines 2517 - 2544, The SettingsWindowController class should be annotated with `@MainActor` to match the isolation used by SettingsNavigationRequest and protect its mutable pendingNavigationTarget state; update the class declaration to include `@MainActor` and ensure any initializers or methods referencing pendingNavigationTarget (and any call sites creating or accessing SettingsWindowController) are executed or awaited from the main actor context so the compiler knows those accesses are main-thread-safe.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@Sources/cmuxApp.swift`:
- Around line 2517-2544: The SettingsWindowController class should be annotated
with `@MainActor` to match the isolation used by SettingsNavigationRequest and
protect its mutable pendingNavigationTarget state; update the class declaration
to include `@MainActor` and ensure any initializers or methods referencing
pendingNavigationTarget (and any call sites creating or accessing
SettingsWindowController) are executed or awaited from the main actor context so
the compiler knows those accesses are main-thread-safe.
lawrencecchen
added
the
stale-revisit
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
2 participants
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
Testing
Related
Summary by cubic
Fixes browser import wizard flakiness by preselecting all source profiles and waiting for real step readiness in tests. Also makes Settings navigation to the Browser Import section reliable and exposes it to accessibility, and ensures the browser profile popover debug window owns Cmd+W.
Bug Fixes
cmux.browserProfilePopoverDebug; added a regression test.Refactors
removeProfile(id:)toBrowserProfileStoreto delete non-default profiles, clean history files/directories, and reset last-used when needed.TemporaryBrowserProfileTrackerin tests to create and clean up profiles reliably.Written for commit 8d4186b. Summary will update on new commits.
Summary by CodeRabbit
New Features
Behavior Changes
Tests