chore: Refactor settings handling API#1148
Conversation
There was a problem hiding this comment.
Pull request overview
This PR refactors the /appium/settings handling in WebDriverAgent by centralizing “get current settings” and “apply settings” logic into a dedicated FBSettingsHandler utility, reducing duplication in FBSessionCommands.
Changes:
- Added
FBSettingsHandlerto provide a single source of truth for settings getters/setters and settings application/validation flow. - Updated
FBSessionCommandssettings routes to delegate toFBSettingsHandler. - Registered the new handler in the Xcode project; also added a new Ruby gem dependency (
multi_json).
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| WebDriverAgentLib/Utilities/FBSettingsHandler.m | New centralized implementation of settings getters/setters and application logic. |
| WebDriverAgentLib/Utilities/FBSettingsHandler.h | Public interface for applying settings and retrieving current settings. |
| WebDriverAgentLib/Commands/FBSessionCommands.m | Routes now call into FBSettingsHandler instead of inline settings logic. |
| WebDriverAgent.xcodeproj/project.pbxproj | Adds the new handler files to build phases/headers. |
| Gemfile | Adds multi_json gem dependency. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| source "https://rubygems.org" | ||
|
|
||
| gem "fastlane", '~> 2.229' | ||
| gem "multi_json" |
There was a problem hiding this comment.
Hm, is multi_json required outside of git managed code?
There was a problem hiding this comment.
I had to add it to resolve integration tests execution, which suddenly started to fail. I would be happy to tune that if there is a better fix. Failing job example: https://github.com/appium/WebDriverAgent/actions/runs/27012566145/job/79719509941
| FBSettingApplyBlock handler = settersMap[key]; | ||
| if (nil == handler) { | ||
| continue; | ||
| } | ||
| FBCommandStatus *status = handler(session, FBNormalizedSettingValue(settings[key])); |
| map[FB_SETTING_ACCEPT_ALERT_BUTTON_SELECTOR] = ^id(FBSession *session) { | ||
| return FBConfiguration.acceptAlertButtonSelector; | ||
| }; |
There was a problem hiding this comment.
the current behaviour is ok
| map[FB_SETTING_DISMISS_ALERT_BUTTON_SELECTOR] = ^id(FBSession *session) { | ||
| return FBConfiguration.dismissAlertButtonSelector; | ||
| }; |
There was a problem hiding this comment.
the current behaviour is ok
| map[FB_SETTING_AUTO_CLICK_ALERT_SELECTOR] = ^id(FBSession *session) { | ||
| return FBConfiguration.autoClickAlertSelector; | ||
| }; |
There was a problem hiding this comment.
the current behaviour is ok
| NSDictionary<NSString *, FBSettingApplyBlock> *settersMap = [self settersMap]; | ||
| NSSet<NSString *> *nilClearableKeys = FBNilClearableSettingKeys(); | ||
| for (NSString *key in settings) { | ||
| FBSettingApplyBlock handler = settersMap[key]; | ||
| if (nil == handler) { | ||
| continue; | ||
| } | ||
| id value = FBNormalizedSettingValue(settings[key]); | ||
| if (nil == value && ![nilClearableKeys containsObject:key]) { | ||
| continue; | ||
| } | ||
| FBCommandStatus *status = handler(session, value); | ||
| if (status.hasError) { | ||
| return status; | ||
| } | ||
| } | ||
| return nil; |
There was a problem hiding this comment.
no crash is going to happen, unrecognised selector would just error out the API request with an unknown error
| + (nullable FBCommandStatus *)applySettings:(NSDictionary *)settings toSession:(FBSession *)session | ||
| { | ||
| NSDictionary<NSString *, FBSettingApplyBlock> *settersMap = [self settersMap]; | ||
| NSSet<NSString *> *nilClearableKeys = FBNilClearableSettingKeys(); | ||
| for (NSString *key in settings) { |
There was a problem hiding this comment.
this would be an overkill. I'm happy with the current state
| source "https://rubygems.org" | ||
|
|
||
| gem "fastlane", '~> 2.229' | ||
| gem "multi_json" |
There was a problem hiding this comment.
Hm, is multi_json required outside of git managed code?
## [13.2.1](v13.2.0...v13.2.1) (2026-06-06) ### Miscellaneous Chores * Refactor settings handling API ([#1148](#1148)) ([ff7ac36](ff7ac36))
|
🎉 This PR is included in version 13.2.1 🎉 The release is available on: Your semantic-release bot 📦🚀 |
No description provided.