Repository navigation
Extract browser-search settings into CmuxSettings (Wave-2 store, no namespace enum) - #6143
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughMoves browser search settings types ( ChangesBrowser Search Settings Extraction into CmuxSettings
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 20 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (20 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
Greptile SummaryThis PR extracts the browser-search settings domain from
Confidence Score: 5/5Safe to merge; this is a mechanical extraction with no behavioral changes to production search URL rendering, defaults keys, or default values (the catalog defaultValue delta is noted in a prior thread). The change is a well-scoped extraction: keys, defaults, and URL-rendering logic are moved verbatim, the build and test suite pass, and existing tests cover every behavioral path that was migrated. The open design questions about store allocation inside BrowserSearchConfiguration and BrowserSearchEngine are already captured in earlier review threads and do not affect correctness today. Packages/CmuxSettings/Sources/CmuxSettings/Values/BrowserSearchConfiguration.swift and BrowserSearchEngine.swift — see prior thread comments about the store-allocation pattern inside those value types. Important Files Changed
Class Diagram%%{init: {'theme': 'neutral'}}%%
classDiagram
class BrowserSearchSettingsReading {
<<protocol, Sendable>>
+currentSearchEngine: BrowserSearchEngine
+currentConfiguration: BrowserSearchConfiguration
+currentSearchSuggestionsEnabled: Bool
+configuration(engineRaw:customName:customURLTemplate:) BrowserSearchConfiguration
+normalizedCustomSearchEngineName(raw:) String?
+isValidSearchURLTemplate(raw:) Bool
+searchURL(fromTemplate:query:) URL?
}
class BrowserSearchSettingsStore {
<<struct, Sendable>>
+static searchEngineKey: String
+static customSearchEngineNameKey: String
+static customSearchEngineURLTemplateKey: String
+static searchSuggestionsEnabledKey: String
+static defaultSearchEngine: BrowserSearchEngine
+static defaultCustomSearchEngineName: String
+static defaultCustomSearchEngineURLTemplate: String
+static defaultSearchSuggestionsEnabled: Bool
-nonisolated(unsafe) defaults: UserDefaults
+init(defaults: UserDefaults)
}
class BrowserSearchConfiguration {
<<struct, Equatable, Sendable>>
+engine: BrowserSearchEngine
+customName: String
+customURLTemplate: String
+displayName: String
+remoteSuggestionsEngine: BrowserSearchEngine?
+searchURL(query:) URL?
}
class BrowserSearchEngine {
<<enum, RawRepresentable, Identifiable, Sendable>>
+google, duckduckgo, bing, kagi, startpage
+brave, perplexity, exa, yahoo, ecosia
+qwant, mojeek, wikipedia, github, baidu, yandex, custom
+displayName: String
+searchURLTemplate: String?
+supportsRemoteSuggestions: Bool
+searchURL(query:) URL?
}
BrowserSearchSettingsStore ..|> BrowserSearchSettingsReading
BrowserSearchSettingsStore ..> BrowserSearchConfiguration : produces
BrowserSearchConfiguration --> BrowserSearchEngine : contains
BrowserSearchConfiguration ..> BrowserSearchSettingsStore : creates (standard defaults)
BrowserSearchEngine ..> BrowserSearchSettingsStore : creates (standard defaults)
Reviews (2): Last reviewed commit: "Refresh browser search test length budge..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| public var displayName: String { | ||
| guard engine == .custom else { return engine.displayName } | ||
| return BrowserSearchSettingsStore().normalizedCustomSearchEngineName(customName) | ||
| ?? engine.displayName | ||
| } | ||
|
|
||
| /// Built-in engine to query for remote suggestions, if supported. | ||
| public var remoteSuggestionsEngine: BrowserSearchEngine? { | ||
| guard engine.supportsRemoteSuggestions else { return nil } | ||
| return engine | ||
| } | ||
|
|
||
| /// Renders a search URL for the given query. | ||
| /// | ||
| /// - Parameter query: The raw search query. | ||
| /// - Returns: An allowed `http` or `https` URL, or `nil` when the | ||
| /// configured template cannot produce one. | ||
| public func searchURL(query: String) -> URL? { | ||
| if engine == .custom { | ||
| return BrowserSearchSettingsStore().searchURL(fromTemplate: customURLTemplate, query: query) | ||
| } | ||
| return engine.searchURL(query: query) | ||
| } |
There was a problem hiding this comment.
Both displayName and searchURL(query:) construct a BrowserSearchSettingsStore() using .standard defaults, regardless of which suite the outer store was initialized with. The helpers they delegate to — normalizedCustomSearchEngineName and searchURL(fromTemplate:query:) — are pure string functions that don't read defaults today, so tests pass and production is correct. The danger is forward-looking: any developer who later adds a setting read inside those helpers (e.g., a scheme allow-list or a locale-aware template) will not realize that BrowserSearchConfiguration always escapes to .standard, silently making the injected suite invisible to these two paths. Since customName and customURLTemplate are already stored fields on the struct, both helpers could be called as static methods or free functions without involving the store at all.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| public func searchURL(query: String) -> URL? { | ||
| guard let template = searchURLTemplate else { return nil } | ||
| return BrowserSearchSettingsStore().searchURL(fromTemplate: template, query: query) | ||
| } |
There was a problem hiding this comment.
BrowserSearchEngine.searchURL(query:) creates a BrowserSearchSettingsStore() solely to invoke searchURL(fromTemplate:query:), which is a stateless URL-rendering function. This inverts the dependency: a plain value enum in the same package should not depend on the settings store. Because searchURLTemplate is already a computed property on the enum, the URL construction from that string could be done with the same inline logic as the store uses, without the BrowserSearchSettingsStore() allocation. This also means every built-in engine search from BrowserSearchConfiguration.searchURL(query:) goes through two store allocations — one constructed by BrowserSearchConfiguration and one by the engine — each doing nothing with defaults.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| public let customSearchEngineURLTemplate = DefaultsKey<String>( | ||
| id: "browser.customSearchEngineURLTemplate", | ||
| defaultValue: "", | ||
| userDefaultsKey: "browserCustomSearchEngineURLTemplate" | ||
| defaultValue: BrowserSearchSettingsStore.defaultCustomSearchEngineURLTemplate, | ||
| userDefaultsKey: BrowserSearchSettingsStore.customSearchEngineURLTemplateKey | ||
| ) | ||
|
|
There was a problem hiding this comment.
defaultValue for customSearchEngineURLTemplate changed from "" to the Google URL
The old catalog entry had defaultValue: "" while BrowserSearchSettings.defaultCustomSearchEngineURLTemplate was always "https://www.google.com/search?q={query}". Those were inconsistent; this PR aligns them. However, the PR description only claims "Defaults key strings are preserved byte-identically" without calling out this value change. Any catalog consumer that reads BrowserCatalogSection().customSearchEngineURLTemplate.defaultValue directly — for example, a settings-export path that renders the fallback default before the user has set a value — will now see the Google URL instead of an empty string. This is probably the right behavior (it now matches what register(defaults:) installs via normalizeBrowserDefaults), but it should be explicitly confirmed rather than left as an implicit side effect of the refactor.
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Sources/Panels/BrowserPanel.swift`:
- Around line 4077-4080: In the BrowserPanel.swift defaults registration around
the searchEngineKey assignment, the search engine value is not being
canonicalized during bootstrap like other settings in the same registration
block. Resolve the engine value by instantiating a BrowserSearchSettingsStore
with the defaults dictionary being built, then extract the canonicalized
rawValue from that resolved store instance and write it back to the
BrowserSearchSettingsStore.searchEngineKey entry instead of using the raw
default directly. This ensures that stale or invalid engine values do not
persist and keeps AppStorage consumers synchronized with the navigation
fallback.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: fd1b6791-b5f7-47f3-9829-7781d9299ee2
⛔ Files ignored due to path filters (1)
.github/swift-file-length-budget.tsvis excluded by!**/*.tsv
📒 Files selected for processing (15)
Packages/CmuxSettings/Sources/CmuxSettings/Keys/BrowserCatalogSection.swiftPackages/CmuxSettings/Sources/CmuxSettings/Stores/BrowserSearchSettingsReading.swiftPackages/CmuxSettings/Sources/CmuxSettings/Stores/BrowserSearchSettingsStore.swiftPackages/CmuxSettings/Sources/CmuxSettings/Values/BrowserSearchConfiguration.swiftPackages/CmuxSettings/Sources/CmuxSettings/Values/BrowserSearchEngine.swiftPackages/CmuxSettings/Tests/CmuxSettingsTests/BrowserSearchSettingsStoreTests.swiftSources/CommandPalette/CommandPaletteSettingsToggle.swiftSources/KeyboardShortcutSettingsFileStore+Template.swiftSources/KeyboardShortcutSettingsFileStore.swiftSources/Panels/BrowserPanel.swiftSources/Panels/BrowserPanelView.swiftSources/TerminalController.swiftcmuxTests/BrowserConfigTests.swiftcmuxTests/GhosttyConfigTests.swiftcmuxTests/KeyboardShortcutSettingsFileStoreStartupTests.swift
| BrowserSearchSettingsStore.searchEngineKey: BrowserSearchSettingsStore.defaultSearchEngine.rawValue, | ||
| BrowserSearchSettingsStore.customSearchEngineNameKey: BrowserSearchSettingsStore.defaultCustomSearchEngineName, | ||
| BrowserSearchSettingsStore.customSearchEngineURLTemplateKey: BrowserSearchSettingsStore.defaultCustomSearchEngineURLTemplate, | ||
| BrowserSearchSettingsStore.searchSuggestionsEnabledKey: BrowserSearchSettingsStore.defaultSearchSuggestionsEnabled, |
There was a problem hiding this comment.
Write back the canonical search-engine value during bootstrap.
register(defaults:) won’t overwrite an existing invalid browserSearchEngine, so this migration leaves stale raw values persisted while other settings in this same method are canonicalized. Resolve the engine through BrowserSearchSettingsStore(defaults:) and write the resolved rawValue back here so AppStorage/settings consumers stay in sync with the navigation fallback.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Sources/Panels/BrowserPanel.swift` around lines 4077 - 4080, In the
BrowserPanel.swift defaults registration around the searchEngineKey assignment,
the search engine value is not being canonicalized during bootstrap like other
settings in the same registration block. Resolve the engine value by
instantiating a BrowserSearchSettingsStore with the defaults dictionary being
built, then extract the canonicalized rawValue from that resolved store instance
and write it back to the BrowserSearchSettingsStore.searchEngineKey entry
instead of using the raw default directly. This ensures that stale or invalid
engine values do not persist and keeps AppStorage consumers synchronized with
the navigation fallback.
This branch was successfully deployed
1 participant
Summary
Extracts the browser-search settings domain from
Sources/Panels/BrowserPanel.swiftintoPackages/CmuxSettingsusing the Wave-2 settings-store shape:BrowserSearchEnginenow lives inCmuxSettingswith the same cases, display names, built-in templates, remote-suggestion support, and search URL rendering behavior.BrowserSearchConfigurationnow lives inCmuxSettingsas its own value type.BrowserSearchSettingsReadingandBrowserSearchSettingsStore, with constructor-injectedUserDefaultsand instance methods for reads, configuration construction, URL-template validation/rendering, and custom-name normalization.BrowserSearchSettingsStoreconstants or constructed stores.UserDefaultssuites.Defaults / wire format
Defaults key strings are preserved byte-identically:
browserSearchEnginebrowserCustomSearchEngineNamebrowserCustomSearchEngineURLTemplatebrowserSearchSuggestionsEnabledLegacy default values are preserved in
BrowserSearchSettingsStore:googlehttps://www.google.com/search?q={query}trueDestination decision
Destination is
CmuxSettingsbecause this code owns settings keys/defaults, defaults-backed reads, and settings-file validation behavior. That matches the prior Wave-2 settings cutover. If owners prefer a future browser-domain package for runtime-only search behavior, this PR keeps the settings storage seam isolated enough to split later without changing the persisted wire format.BrowserPanel.swift reduction
BrowserPanel.swiftbudget was ratcheted from 13,595 to 13,358 lines: net -237 lines. The commit diff for that file is 6 additions and 243 deletions.Validation
scripts/lint-ios-package-conventions.sh-> OK, no newlint:allowsuppressions.swift buildinPackages/CmuxSettings-> passed.swift testinPackages/CmuxSettings-> passed, 133 tests total including 4 newBrowserSearchSettingsStoretests.xcodebuild -project cmux.xcodeproj -scheme cmux -configuration Debug -destination 'platform=macOS' -derivedDataPath /tmp/cmux-browsersearch build->** BUILD SUCCEEDED **.xcodebuild -project cmux.xcodeproj -scheme cmux-unit -configuration Debug -destination 'platform=macOS' -derivedDataPath /tmp/cmux-browsersearch-unit build->** BUILD SUCCEEDED **.python3 scripts/swift_file_length_budget.py-> budget respected.settings.browser.searchEngine.*keys; verified each hasenandjaentries inResources/Localizable.xcstrings.Do not merge; leaving this for owner review.
Need help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.Summary by cubic
Extracted browser-search settings into
CmuxSettingswith a Wave-2 store and protocol, preserving behavior, keys, and defaults. This isolates persistence and URL rendering, reducesBrowserPanel.swiftby 237 lines, and improves testability.Refactors
BrowserSearchEngineandBrowserSearchConfigurationtoCmuxSettings; same cases, display names, templates, suggestion support, and URL rendering;BrowserSearchEngineis nowIdentifiable.BrowserSearchSettingsReadingandBrowserSearchSettingsStore(injectedUserDefaults) with current engine/config/suggestions and helpers for config factory, template validation/rendering, and custom-name normalization.BrowserSearchSettingsStoreconstants/APIs acrossBrowserPanel,BrowserPanelView,TerminalController, command-palette toggles, settings-file import, and catalog defaults.BrowserSearchEnginetoCmuxSettings; added Swift tests inCmuxSettingsfor config factory, template validation/rendering, and normalization.Migration
CmuxSettings, depend onBrowserSearchSettingsReading, and replaceBrowserSearchSettings.*calls withBrowserSearchSettingsStore.Written for commit a4970eb. Summary will update on new commits.
Summary by CodeRabbit