fix(user_defaults): return canonical JSON for NSNumber(BOOL) values (#9041)#9708
Conversation
|
Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have the users @MatthewJamisonJS on file. In order for us to review and merge your code, each contributor must visit https://cla.warp.dev to read and agree to our CLA. Once you have done so, please comment |
|
I'm starting a first review of this pull request. You can view the conversation on Warp. I reviewed this pull request and requested human review from: @DanielPeng. Comment Powered by Oz |
github-actions
Bot
added
the
external-contributor
![oz-for-oss[bot]](https://avatars.githubusercontent.com/in/3450139?s=60&v=4){kind=small}
There was a problem hiding this comment.
Overview
This PR updates the macOS NSUserDefaults-backed preferences reader to detect NSNumber-backed BOOL values before falling back to stringForKey:, returning canonical JSON booleans for defaults written with -bool.
Concerns
- No blocking correctness or security concerns found in the changed diff.
Verdict
Found: 0 critical, 0 important, 0 suggestions
Approve
Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).
Powered by Oz
|
Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have the users @MatthewJamisonJS on file. In order for us to review and merge your code, each contributor must visit https://cla.warp.dev to read and agree to our CLA. Once you have done so, please comment |
|
@cla-bot check |
|
The cla-bot has been summoned, and re-checked this pull request! |
|
/oz-review |
|
I'm re-reviewing this pull request in response to a review request. You can view the conversation on Warp. I reviewed this pull request and requested human review from: @DanielPeng. Comment Powered by Oz |
There was a problem hiding this comment.
Overview
This PR updates macOS NSUserDefaults preference reads to recognize NSNumber-backed booleans and return canonical JSON boolean strings before falling back to stringForKey:.
Concerns
- The boolean detection uses Objective-C type encodings, so char-backed NSNumber values (
objCType == "c") would also be interpreted as booleans. This is a narrow compatibility edge for externally-written defaults and does not block the fix.
Verdict
Found: 0 critical, 0 important, 1 suggestions
Approve with nits
Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).
Powered by Oz
| let obj_c_type: *const std::os::raw::c_char = msg_send![raw, objCType]; | ||
| let encoding = std::ffi::CStr::from_ptr(obj_c_type).to_bytes(); | ||
| // 'c' = signed char (BOOL on x86_64); 'B' = C++ bool (BOOL on arm64). | ||
| if encoding == b"c" || encoding == b"B" { |
There was a problem hiding this comment.
💡 [SUGGESTION] objCType == b"c" also matches char-backed NSNumbers, not only BOOL; prefer identifying CFBoolean directly if this path may read defaults written by other code.
grawish
left a comment
grawish
left a comment
There was a problem hiding this comment.
Reviewed with a focus on correctness and regression risk.
| let obj_c_type: *const std::os::raw::c_char = msg_send![raw, objCType]; | ||
| let encoding = std::ffi::CStr::from_ptr(obj_c_type).to_bytes(); | ||
| // 'c' = signed char (BOOL on x86_64); 'B' = C++ bool (BOOL on arm64). | ||
| if encoding == b"c" || encoding == b"B" { |
There was a problem hiding this comment.
This check may also match non-boolean NSNumber values: Objective-C type encoding c is signed char, and NSNumber can use that encoding for numeric char values as well as BOOL on some runtimes. If a future/default key stores a char-valued number, this path would coerce it to JSON true/false instead of preserving the existing stringForKey: behavior. Consider checking CoreFoundation boolean identity instead (for example CFGetTypeID(raw as _) == CFBooleanGetTypeID()) before returning canonical JSON booleans.
There was a problem hiding this comment.
Thank you for that insight!
Truly appreciate you taking time to point me in the right direction 🙂🖖🏿
2 participants
Description
Refs #9041.
CopyOnSelect=falseset viadefaults write -bool falseis silently ignored. Root cause:UserDefaultsPreferencesStorage::read_valueusesstringForKey:, which coerces anNSNumber(BOOL)to"0"or"1".serde_json::from_str::<bool>("0")returnsErr; the setting falls back to its defaulttrue; drag-to-select copies regardless of the user's preference.Fix: call
objectForKey:first. If the stored value is anNSNumberwith a boolean ObjC type encoding (con x86_64,Bon arm64), return"true"or"false"directly. All other value types (NSString, non-boolean NSNumber) fall through to the originalstringForKey:path — no behavior change for values written without-bool.A second conditional path where post-migration
defaults writeedits are stranded whenFeatureFlag::SettingsFileis enabled was also identified during the audit of this issue. That scenario has design tradeoffs and is tracked separately.Linked Issue
ready-to-implement.Screenshots / Videos
Not applicable — no user-visible UI change; the fix restores an existing preference to having effect.
Testing
cargo clippy -p warpui_extras --all-targets -- -D warningscleandefaults write dev.warp.Warp-Stable CopyOnSelect -bool false, launch Warp, drag-select text — clipboard should not be written. Toggle back with-bool true, verify drag-select copies again.Agent Mode
CHANGELOG-BUG-FIX: Fix CopyOnSelect=false being ignored when the preference was set via
defaults write -bool false