refactor: move webview JSON field reading to Swift - #929
BrandonStalnaker merged 3 commits into
Conversation
🐦 Swift Migration ProgressProduction implementation code at
Objective-C retained by design: Core SDK 5,802 · SDK kit infrastructure 2,041 · Standalone kits 0. This PR's code movement
How this is measured
Generated with |
📦 SDK Size Impact ReportMeasures how much the SDK adds to an app's size (with-SDK minus without-SDK).
➡️ SDK size impact change is minimal. Raw measurementsTarget branch (workstation/swift-migration): {"baseline_app_size_kb":84,"baseline_executable_size_bytes":75464,"with_sdk_app_size_kb":2616,"with_sdk_executable_size_bytes":76312,"sdk_impact_kb":2532,"sdk_executable_impact_bytes":848,"xcframework_size_kb":6984}This PR: {"baseline_app_size_kb":84,"baseline_executable_size_bytes":75464,"with_sdk_app_size_kb":2620,"with_sdk_executable_size_bytes":76312,"sdk_impact_kb":2536,"sdk_executable_impact_bytes":848,"xcframework_size_kb":6988} |
e9b571e to
b1e001a
Compare
PR SummaryMedium Risk Overview
Behavior fix: promotion and transaction attribute setters are only invoked when a value is actually present, so invalid/all-null payloads do not touch lazy attribute dictionaries and still yield nil Adds Swift unit tests for Reviewed by Cursor Bugbot for commit 8c3eea8. Bugbot is set up for automated code reviews on this repo. Configure here. |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit b1e001a. Configure here.
b1e001a to
49ef53a
Compare
Continues the "move migrated types over" work after #918. MPConvertJS is the webview bridge's JSON-to-SDK-type converter; every one of its public methods returns an SDK type, so construction stays in the .m per docs/swift-migration/CONVERSION-RECIPE.md and only the reading moves. MPConvertJS.m goes from 307 to 262 sloc. Moved to Swift: - +commerceEventActionForJSValue:. MPJSCommerceEventAction and MPCommerceEventAction do not share an order, so this is a genuine remap, not a cast: AddToWishList is 9 on the way in and 2 on the way out, and Checkout is 3 in and 4 out. Every pair is asserted individually rather than trusted. Swift returns nil for a value the bridge does not define — including its own Unknown (0) — so the .m keeps the MPILogError and the AddToCart fallback at the same call site the switch's default: had. - promotionFieldsFromJSON: and transactionAttributesFieldsFromJSON:, which replace twelve -isKindOfClass: guarded reads. The parameter of the action mapping is Any? rather than NSNumber* on purpose: the ObjC declared NSNumber* but read the value with -intValue and nothing enforced the type, so a string-encoded action worked. That is preserved through MPJSONCoercion and pinned by a test. The .m now assigns every field unconditionally. That is safe because each target setter removes its key when handed nil (MPPromotion.m:176, MPTransactionAttributes.m:186 and friends) and the keys are absent on a freshly initialised instance, so assigning nil and not assigning are the same thing. Checked rather than assumed — all twelve properties have custom setters. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
49ef53a to
4b44d2d
Compare
BrandonStalnaker
left a comment
BrandonStalnaker
left a comment
There was a problem hiding this comment.
MPPromotion’s setters always go through the lazy attributes / beautifiedAttributes getters. Those allocate empty dictionaries, then remove a key that was never there. dictionaryRepresentation returns _attributes as-is (it does not treat empty as nil):
Before this PR, a promotion with no valid string fields left _attributes nil, so [promotion dictionaryRepresentation] was nil and MPPromotionContainer skipped it. After, it is @{}. The container then ships that empty object (and the same for beautified output):
MPPromotion and MPTransactionAttributes both lazily allocate their attributes/beautifiedAttributes dictionaries on first touch, including from a setter's nil-removal branch. Assigning every field unconditionally still triggers that allocation when the value is nil, so a promotion or transaction with no valid fields ended up with an empty (non-nil) dictionaryRepresentation instead of nil -- which MPPromotionContainer no longer skips, shipping an empty promotion where it previously shipped nothing. Restores the per-field nil guard removed when this method moved to Swift-backed field reading, and adds a regression test for both types. Addresses Brandon's review on this PR.
|
Good catch, @BrandonStalnaker — fixed. Restored the per-field
Added a regression test for each asserting |
if (fields.shipping) on an NSNumber* pointer trips the static analyzer's osx.NumberObjectConversion check, same as the string-field guards fixed in b8f17ca. Use an explicit != nil check for the three NSNumber fields.
ae9a889
into
workstation/swift-migration
2 participants
Background
Continues the wrapper-deletion audit from #918: no further dead files or privatization candidates remain, so the series continues via extraction.
MPConvertJS.m(webview→SDK-type JSON converter) was the best-shaped untouched candidate.What Has Changed
MPConvertJSFields.swiftreadingcommerceEventActionForJSValue:,promotionFieldsFromJSON:,transactionAttributesFieldsFromJSON:. SDK-type construction (MPCommerceEvent,MPPromotion, etc.) stays in the.m. 307 → 262 SLOC.nilfor unknown/undefined values so the.mkeeps itsMPILogError+AddToCartfallback.actionparameter typedAny?(notNSNumber *) since the original read a string-encoded value via-intValuewith no enforced type..mnow assigns every field unconditionally — safe because every target setter already removes its key on nil.Validation
abi-guardPASSEDtrunk checkclean (caught 6 trailing-comma issues in new tests, fixed)Stack
Depends on #927.
Checklist