Conversation
Generated by 🚫 Danger |
|
| } | ||
|
|
||
| if ([self.blog supports:BlogFeatureWPComRESTAPI] && self.blog.isAdmin) { | ||
| if ([self.blog supports:BlogFeatureWpComRESTAPI] && self.blog.isAdmin) { |
There was a problem hiding this comment.
It's automatic conversion from .wpComRESTAPI – should be fine. The ObjC code is legacy code, and it would be ideal to convert it later as well.
|
| App Name | WordPress | |
| Configuration | Release-Alpha | |
| Build Number | 31114 | |
| Version | PR #25296 | |
| Bundle ID | org.wordpress.alpha | |
| Commit | 271a063 | |
| Installation URL | 106rms0n1j2c0 |
|
| App Name | Jetpack | |
| Configuration | Release-Alpha | |
| Build Number | 31114 | |
| Version | PR #25296 | |
| Bundle ID | com.jetpack.alpha | |
| Commit | 271a063 | |
| Installation URL | 4o01u4a7k6cn8 |
🤖 Build Failure AnalysisThis build has failures. Claude has analyzed them - check the build annotations for details. |
jkmassel
left a comment
There was a problem hiding this comment.
Comprehensive Code Review
I did a line-by-line comparison of every BlogFeature case (old ObjC vs new Swift), verified all call sites, and built/tested locally. This is a clean, well-executed conversion. All 45 enum cases are faithfully translated, all removed methods have no external callers, and the @objc bridging names are correct (with the WPComRESTAPI → WpComRESTAPI rename properly propagated to all 6 ObjC call sites).
One behavioral change worth noting
hasRequiredJetpackVersion has a subtle behavioral change when jetpack?.version is nil:
Old ObjC:
[self.jetpack.version compare:requiredJetpackVersion options:NSNumericSearch] != NSOrderedAscendingWhen jetpack or version is nil, ObjC nil-messaging returns NSOrderedSame (0), so NSOrderedSame != NSOrderedAscending → YES. The old code incorrectly returned true for sites with no Jetpack version info.
New Swift:
guard supportsRestAPI, !isHostedAtWPcom,
let version = jetpack?.version else {
return false
}The guard let correctly returns false when version is nil.
This is almost certainly a latent bug fix (a nil version should never satisfy a version requirement), and the real-world impact is limited since most affected features (.contactInfo, .videoPressV5, .facebookEmbed, etc.) have || isHostedAtWPcom fallbacks. Just flagging since the PR description says the conversion is faithful — this is actually more correct than the original.
Verified locally
- WordPress scheme: BUILD SUCCEEDED
- WordPressTest suite: 69 tests, all passed
- WordPressShareExtension: BUILD SUCCEEDED
Nice cleanup! 👍





It's impractical to test it, so I carefully reviewed every line that changed. There are few existing unit tests for the more advanced features as well.
There are still a couple of simplifications that could be made. I plan to address them separately.