Repository navigation
Add copy-on-select setting - #1326
Closed
lawrencecchen wants to merge 1 commit into
Closed
lawrencecchen wants to merge 1 commit into
lawrencecchen wants to merge 1 commit into
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}
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.
📝 WalkthroughWalkthroughThis PR implements a copy-on-select feature across the application by adding localization strings, introducing a TerminalCopyOnSelectSettings configuration subsystem with multiple modes, integrating settings UI controls with persistent storage, refactoring config injection to use inline loading, and adding corresponding tests. Changes
Sequence DiagramsequenceDiagram
actor User
participant SettingsView
participant AppStorage
participant cmuxApp
participant GhosttyApp
participant GhosttyTerminalView
User->>SettingsView: Select copy-on-select mode
SettingsView->>AppStorage: Save terminalCopyOnSelectMode
AppStorage-->>SettingsView: onChange triggered
SettingsView->>cmuxApp: Normalize & trigger reload
cmuxApp->>GhosttyApp: reloadConfiguration("settings.copy_on_select")
GhosttyApp->>GhosttyTerminalView: Pass config to terminal
GhosttyTerminalView->>GhosttyTerminalView: loadCopyOnSelectOverrideIfNeeded()
GhosttyTerminalView->>GhosttyTerminalView: loadInlineGhosttyConfig() with override line
GhosttyTerminalView-->>User: Apply selected copy-on-select behavior
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
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 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.
Actionable comments posted: 1
🧹 Nitpick comments (1)
cmuxTests/GhosttyConfigTests.swift (1)
541-543: Use unique UserDefaults suite names per test to avoid cross-test coupling.A single shared suite name in this class can make tests flaky when execution overlaps. Prefer generating a UUID-based suite per test instance.
♻️ Proposed isolation tweak
final class TerminalCopyOnSelectSettingsTests: XCTestCase { func testCopyOnSelectSettingDefaultsToInheritWhenUnset() { - let defaults = makeCopyOnSelectDefaults() - defer { defaults.removePersistentDomain(forName: defaultsSuiteName) } + let (defaults, suiteName) = makeCopyOnSelectDefaults() + defer { defaults.removePersistentDomain(forName: suiteName) } XCTAssertEqual(TerminalCopyOnSelectSettings.mode(defaults: defaults), .inherit) XCTAssertNil(TerminalCopyOnSelectSettings.overrideConfigLine(defaults: defaults)) } @@ func testCopyOnSelectSettingReadsStoredClipboardOverride() { - let defaults = makeCopyOnSelectDefaults() - defer { defaults.removePersistentDomain(forName: defaultsSuiteName) } + let (defaults, suiteName) = makeCopyOnSelectDefaults() + defer { defaults.removePersistentDomain(forName: suiteName) } defaults.set( TerminalCopyOnSelectSettings.Mode.clipboard.rawValue, forKey: TerminalCopyOnSelectSettings.modeKey ) @@ - private let defaultsSuiteName = "TerminalCopyOnSelectSettingsTests" - - private func makeCopyOnSelectDefaults() -> UserDefaults { - let defaults = UserDefaults(suiteName: defaultsSuiteName)! - defaults.removePersistentDomain(forName: defaultsSuiteName) - return defaults + private func makeCopyOnSelectDefaults() -> (UserDefaults, String) { + let suiteName = "cmux.tests.copy-on-select.\(UUID().uuidString)" + let defaults = UserDefaults(suiteName: suiteName)! + defaults.removePersistentDomain(forName: suiteName) + return (defaults, suiteName) } }Also applies to: 565-567, 579-584
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmuxTests/GhosttyConfigTests.swift` around lines 541 - 543, The tests use a shared defaultsSuiteName which can cause cross-test coupling; change each test to create a unique suite name (e.g., UUID string) and pass it into makeCopyOnSelectDefaults (or overload/extend makeCopyOnSelectDefaults to accept a suite name) instead of using the global defaultsSuiteName, then update cleanup to call defaults.removePersistentDomain(forName: generatedSuiteName); update all occurrences around makeCopyOnSelectDefaults and the deferred removePersistentDomain calls (including the other instances at the mentioned ranges) to use the per-test generated suite identifier.
🤖 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/GhosttyTerminalView.swift`:
- Around line 1056-1058: Replace the DEBUG-only Self.initLog call with a dlog()
call: inside the existing `#if` DEBUG / `#endif` block where Self.initLog("failed to
write \(logLabel) config: \(error)") is invoked, call dlog(...) instead and pass
the same formatted message (including logLabel and error) so the debug event
uses the standardized dlog function; locate the call by searching for
Self.initLog in the GhosttyTerminalView initialization/config write code and
update that site accordingly.
---
Nitpick comments:
In `@cmuxTests/GhosttyConfigTests.swift`:
- Around line 541-543: The tests use a shared defaultsSuiteName which can cause
cross-test coupling; change each test to create a unique suite name (e.g., UUID
string) and pass it into makeCopyOnSelectDefaults (or overload/extend
makeCopyOnSelectDefaults to accept a suite name) instead of using the global
defaultsSuiteName, then update cleanup to call
defaults.removePersistentDomain(forName: generatedSuiteName); update all
occurrences around makeCopyOnSelectDefaults and the deferred
removePersistentDomain calls (including the other instances at the mentioned
ranges) to use the per-test generated suite identifier.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 62063f02-b93a-4e54-97a9-4c30a2b71bad
📒 Files selected for processing (4)
Resources/Localizable.xcstringsSources/GhosttyTerminalView.swiftSources/cmuxApp.swiftcmuxTests/GhosttyConfigTests.swift
Comment on lines
+1056
to
+1058
| #if DEBUG | ||
| Self.initLog("failed to write \(logLabel) config: \(error)") | ||
| #endif |
There was a problem hiding this comment.
Use dlog() for this DEBUG log call.
Line 1057 logs via Self.initLog(...); new debug call sites in Swift should use dlog().
Suggested patch
} catch {
`#if` DEBUG
- Self.initLog("failed to write \(logLabel) config: \(error)")
+ dlog("failed to write \(logLabel) config: \(error)")
`#endif`
}As per coding guidelines, “All debug events must be logged using the dlog() function … wrapped in #if DEBUG / #endif.”
📝 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
| #if DEBUG | |
| Self.initLog("failed to write \(logLabel) config: \(error)") | |
| #endif | |
| `#if` DEBUG | |
| dlog("failed to write \(logLabel) config: \(error)") | |
| `#endif` |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Sources/GhosttyTerminalView.swift` around lines 1056 - 1058, Replace the
DEBUG-only Self.initLog call with a dlog() call: inside the existing `#if` DEBUG /
`#endif` block where Self.initLog("failed to write \(logLabel) config: \(error)")
is invoked, call dlog(...) instead and pass the same formatted message
(including logLabel and error) so the debug event uses the standardized dlog
function; locate the call by searching for Self.initLog in the
GhosttyTerminalView initialization/config write code and update that site
accordingly.
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
copy-on-selectmodes, with anInherit Ghostty Configdefault so cmux preserves current Ghostty behavior unless the user overrides ittrue,clipboard, andfalsevaluesTesting
jq empty Resources/Localizable.xcstringsxcodebuild -project GhosttyTabs.xcodeproj -scheme cmux-unit -destination 'platform=macOS' -derivedDataPath /tmp/cmux-feat-copy-on-select-option-unit -only-testing:cmuxTests/TerminalCopyOnSelectSettingsTests -only-testing:cmuxTests/GhosttyConfigTests test./scripts/reload.sh --tag feat-copy-on-select-optionIssues
Summary by cubic
Add a Copy on Select preference that mirrors upstream Ghostty modes and preserves current behavior by default. The setting overrides Ghostty config only when changed and applies immediately.
New Features
copy-on-selectvalues: selection →true, clipboard →clipboard, off →false; inherit leaves config unchanged.Refactors
Written for commit 618b1f6. Summary will update on new commits.
Summary by CodeRabbit
New Features
Localization