iOS: apply bsdiff patches during package install - #48
Conversation
c9f5d13 to
5aefe55
Compare
5aefe55 to
2a740c0
Compare
2a740c0 to
316a439
Compare
316a439 to
221514a
Compare
221514a to
f86f490
Compare
| NSArray *deletedFiles = manifestJSON[@"deletedFiles"]; | ||
| for (NSString *deletedFileName in deletedFiles) { | ||
| NSString *absoluteDeletedFilePath = [newUpdateFolderPath stringByAppendingPathComponent:deletedFileName]; | ||
| if (error) { |
There was a problem hiding this comment.
Bug: unhandled error from [NSJSONSerialization JSONObjectWithData] :)
f86f490 to
80a906d
Compare
80a906d to
813d75f
Compare
There was a problem hiding this comment.
🟡 Changes recommended
Duplicate Xcode object IDs, a breaking public selector change, and missing install-orchestration coverage must be addressed.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds opt-in iOS support for applying bsdiff patches during package installation.
Changes:
- Parses and validates diff manifests before safely applying binary patches.
- Adds the
CodePushEnableDeltaUpdatesconfiguration flag. - Updates iOS project wiring and setup documentation.
File summaries
| File | Description |
|---|---|
ios/CodePush/CodePushPackage.m |
Integrates manifest validation and binary patch application. |
ios/CodePush/CodePushConfig.m |
Reads the delta-update flag from Info.plist. |
ios/CodePush/CodePush.m |
Passes the configuration into package downloads. |
ios/CodePush/CodePush.h |
Exposes the flag and updated download method. |
ios/CodePush.xcodeproj/project.pbxproj |
Adds binary patcher test-target references. |
docs/setup-ios.md |
Documents enabling delta updates. |
Review details
Suppressed comments (1)
ios/CodePush.xcodeproj/project.pbxproj:204
- This PBXFileReference uses the same object ID as the existing definition at line 207. Duplicate PBX object keys can be collapsed or rejected by Xcode tooling; remove the duplicate definition so the test file has one reference object.
F42FA68BF21AED765F55E71A /* CodePushBinaryDiffPatcherTests.swift */ = {isa = PBXFileReference; includeInIndex = 1; lastKnownFileType = sourcecode.swift; path = CodePushBinaryDiffPatcherTests.swift; sourceTree = "<group>"; };
- Files reviewed: 6/6 changed files
- Comments generated: 3
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
813d75f to
92fe5c4
Compare
92fe5c4 to
2b598e6
Compare
| { | ||
| BOOL enableDeltaUpdates = [[CodePushConfig current] enableDeltaUpdates]; | ||
|
|
||
| if (diffManifest.version > 2 || diffManifest.version < 1) { |
There was a problem hiding this comment.
The version and flag validation depend on nothing, but the parsed manifest. They run after the copies have been done. You could split the method and run validations right after manifestFromJSON making rejections happen before the copies are done.
There was a problem hiding this comment.
Rejecting an invalid manifest before any of the package copy operations is a way bigger refactor (see below), so I'd rather not include that in this PR. But I agree about splitting the validation code into its own function, so I just did that.
miklosboros
left a comment
There was a problem hiding this comment.
A few minor / nit-level notes on the implementation details, separate from the larger points.
| @interface CodePushPackage () | ||
|
|
||
| + (BOOL)validateAndApplyDiffManifest:(CodePushDiffManifest *)diffManifest | ||
| currentPackageFolder:(NSString *)currentPackageFolderPath | ||
| unzippedFolder:(NSString *)unzippedFolderPath | ||
| newUpdateFolder:(NSString *)newUpdateFolderPath | ||
| error:(NSError **)error; | ||
|
|
||
| @end |
There was a problem hiding this comment.
This extension isn't needed: the definition at line 38 precedes the only call site at line 294, so the compiler already has the declaration by then. Dropping it also removes a second copy of the signature to keep in sync — the parameter alignment has already drifted a space from the definition.
There was a problem hiding this comment.
Nice, happy coincidence. Removing it
| // The patches folder must not stay in the installed package: it is | ||
| // not part of the released contents, so it changes the folder hash | ||
| // and surfaces later as a misleading integrity-check failure. |
There was a problem hiding this comment.
The invariant this relies on — patch files living under __hcp_patches — isn't actually enforced anywhere. CodePushPatchedFileEntry.patch documents it as "under the reserved patches folder prefix", but resolveWithin(unzippedFolder, entry.patch, …) in the patcher only checks containment, not the prefix.
So a manifest that puts a patch file anywhere else leaves it in the installed package and produces exactly the misleading integrity-check failure this comment is guarding against. verifyFolderHash still catches it, so it fails safe — but validating the prefix in manifestFromJSON: would close the gap properly. Same gap on Android, so this could be a follow-up rather than a blocker.
There was a problem hiding this comment.
Fully agree, good find. Because it needs to be fixed on both platforms, here's a follow-up ticket, I'll open a separate PR: https://bitrise.atlassian.net/browse/RA-4925
| - (BOOL)enableDeltaUpdates | ||
| { | ||
| return _enableDeltaUpdates; | ||
| } |
There was a problem hiding this comment.
@property (readonly) BOOL enableDeltaUpdates would autosynthesize both the _enableDeltaUpdates ivar and this getter, so the explicit ivar on line 6 and this method can both go — just assign in init.
Separately: this is the only config value stored outside _configDictionary, which makes it the only one absent from configuration (line 76) — the dictionary CodePush.m:959 hands to JS. Deliberate? publicKey is also setter-less but still lives in the dictionary, and it carries a comment explaining why there's no setter; worth the same one-liner here.
There was a problem hiding this comment.
About autosynthesis: thanks, fixed!
Regarding _configDictionary: I didn't want to propage that existing pattern because it reads and writes a NSMutableDictionary, so there isn't any type safety for the values we store inside.
I didn't notice how that dictionary is exposed to the JS side, but in this specific case, I think it's completely okay not to expose this new field: this feature flag can only be defined statically in Info.plist, so I don't see any reason why an app would want to query this at runtime (unlike the other config values, which can be overridden from the JS side)
0e59454 to
6043a6b
Compare
8428205 to
9cb8a25
Compare
9cb8a25 to
93c37a6
Compare
Context
iOS counterpart of #42 and #43. Builds on top of #44 and the rest of the PR stack.
Changes
[CodePushBinaryDiffPatcher applyBinaryDiffPatchesFromManifest:]from the main package install flow.CodePushEnableDeltaUpdatesand handle its different states in combination with the manifest version value.Decisions
Tests: unlike #53 for Android, the
CodePushPackage.mchanges can't be unit tested at the moment because of the transitive dependency onReact(singleCodePush.himport, which we started breaking up in #56, #59 and #58). Adding new E2E test cases is an option, but the test harness would have to be extended with bsdiff patch generation and v2 manifest handling. It would also increase our already bad test run times.