Ios track downloaded splits#258
Merged
balaganesh-juspay merged 1 commit intomainfrom Mar 26, 2026
Merged
Conversation
|
Review changes with |
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughThese changes enhance lazy-splits download handling across Android and iOS SDKs. Android introduces conditional lazy-splits download logic based on release fetch outcomes. iOS adds centralized tracking of downloaded splits via a new internal set and exposes query methods for downloaded asset paths, including retry mechanisms for failed downloads. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
airborne_sdk_android/airborne/src/main/java/in/juspay/airborne/ota/UpdateTask.kt (1)
239-259:⚠️ Potential issue | 🟡 Minor
newReleaseFailedis incorrectlytrueforUpdate.Package.NA, causing a misleading log
installPackageUpdatereturnsfalseforUpdate.Package.NA(lines 450–458), which means!didPackageUpdateistruefor that case. The combined condition on lines 239–240 therefore setsnewReleaseFailed = truewhen there is simply no new version available — not an actual failure.Downstream effect: when
updateTimedOut = falseandpresult is Update.Package.NA,shouldDownloadNewLazyevaluates tofalse(becausepresult is Update.Package.Finishedis also false), and execution falls into theelse if (newReleaseFailed)branch on line 257, logging "Skipping new release lazy splits download due to download failure." — a misleading message during normal operation.The simplest fix is to scope
newReleaseFailedto actual download/install failures, excluding NA:🐛 Proposed fix
- newReleaseFailed = !updateTimedOut.get() && - (presult is Update.Package.Failed || !didPackageUpdate) + newReleaseFailed = !updateTimedOut.get() && + (presult is Update.Package.Failed || + (presult is Update.Package.Finished && !didPackageUpdate))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@airborne_sdk_android/airborne/src/main/java/in/juspay/airborne/ota/UpdateTask.kt` around lines 239 - 259, The newReleaseFailed flag is currently set using "!didPackageUpdate" which makes Update.Package.NA be treated as a failure; change the assignment of newReleaseFailed (and any logic that uses it) to only consider actual failure states—e.g., base it on presult being Update.Package.Failed (and still respect updateTimedOut.get()) rather than "!didPackageUpdate" or explicitly exclude presult == Update.Package.NA; update the references to newReleaseFailed and the shouldDownloadNewLazy computation (which checks presult is Update.Package.Finished) so NA does not trigger the "Skipping new release lazy splits download due to download failure." branch and only real failures set newReleaseFailed.
🧹 Nitpick comments (3)
airborne_sdk_android/airborne/src/main/java/in/juspay/airborne/ota/UpdateTask.kt (1)
231-231: Redundant!!after a null-guard on a localvar
presultis a local variable and is not mutated between thepresult != nullguard on the same line and thesaveDownloadedPackagescall. Kotlin's data-flow analysis smart-casts localvars in this scenario, making!!unnecessary. Prefer an explicit smart-cast orletfor clarity:♻️ Proposed refactor
- } else if (presult != null && fetched.pkg.lazy.isEmpty()) { - saveDownloadedPackages(presult!!, fetched.pkg) + } else if (presult != null && fetched.pkg.lazy.isEmpty()) { + saveDownloadedPackages(presult, fetched.pkg)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@airborne_sdk_android/airborne/src/main/java/in/juspay/airborne/ota/UpdateTask.kt` at line 231, The call uses a redundant non-null assertion "presult!!" after a local null-check; remove the "!!" and rely on Kotlin's smart-cast (or wrap the use in presult?.let { ... }) so the compiler treats presult as non-null when calling saveDownloadedPackages; update the call site in UpdateTask (saveDownloadedPackages and fetched.pkg usage) to either call saveDownloadedPackages(presult, fetched.pkg) after the null guard or enclose the call in presult?.let { saveDownloadedPackages(it, fetched.pkg) } for clarity.airborne_sdk_iOS/hyper-ota/Airborne/AirborneObjC/ApplicationManager/AJPApplicationManager.m (1)
1362-1452:retryFailedLazyDownloadsis a copy-paste ofretryFailedLazyDownloadsWithCompletion:The two methods are ~40 lines of near-identical code.
retryFailedLazyDownloadscan be collapsed to a one-liner that delegates to the new variant.♻️ Proposed refactor
- (void)retryFailedLazyDownloads { - NSMutableArray<AJPLazyResource *> *failedDownloads = [NSMutableArray array]; - `@synchronized`(self.package) { - for (AJPLazyResource *resource in self.package.lazy) { - if (!resource.isDownloaded) { - [failedDownloads addObject:resource]; - } - } - } - if (failedDownloads.count > 0) { - [self.tracker trackInfo:@"retrying_failed_lazy_downloads" value:[@{@"count": @(failedDownloads.count)} mutableCopy]]; - __weak AJPApplicationManager *weakSelf = self; - [self downloadLazyPackageResources:failedDownloads version:self.package.version singleDownloadHandler:^(BOOL status, AJPResource *resource) { - if (status && [resource isKindOfClass:[AJPLazyResource class]]) { - if (weakSelf) { - __strong AJPApplicationManager* strongSelf = weakSelf; - AJPLazyResource *lazyResource = (AJPLazyResource *)resource; - [strongSelf moveLazyPackageFromTempToMain:lazyResource]; - } - } - [[NSNotificationCenter defaultCenter] postNotificationName:LAZY_PACKAGE_NOTIFICATION - object:nil - userInfo:@{ - @"lazyDownloadsComplete": `@NO`, - @"downloadStatus": @(status), - @"url": resource.url, - @"filePath": resource.filePath - }]; - } downloadCompletion:^{ - [[NSNotificationCenter defaultCenter] postNotificationName:LAZY_PACKAGE_NOTIFICATION - object:nil - userInfo:@{@"lazyDownloadsComplete": `@YES`}]; - }]; - } else { - [self.tracker trackInfo:@"no_failed_lazy_downloads" value:[@{} mutableCopy]]; - } + [self retryFailedLazyDownloadsWithCompletion:nil]; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@airborne_sdk_iOS/hyper-ota/Airborne/AirborneObjC/ApplicationManager/AJPApplicationManager.m` around lines 1362 - 1452, The method retryFailedLazyDownloads duplicates retryFailedLazyDownloadsWithCompletion:; replace the entire implementation of retryFailedLazyDownloads to simply call [self retryFailedLazyDownloadsWithCompletion:nil] (delegate to the completion-taking variant) to remove the copy-pasted logic and keep a single source of truth (ensure the selector name retryFailedLazyDownloadsWithCompletion: and the parameter nil are used exactly as in the existing code).airborne_sdk_iOS/hyper-ota/Airborne/AirborneObjC/include/AJPApplicationManager.h (1)
178-192: LGTM — optional: align parameter names between header and implementationThe
_Nullableannotation and doc comments are correct. The only nit is that the header declaresfilePathwhile the implementation usesresourcePathfor the same parameter. Obj-C doesn't require them to match, but consistency aids readability.🔧 Optional: align parameter name
- - (NSString * _Nullable)getPathForAssetsInReleaseConfig:(NSString *)filePath; + - (NSString * _Nullable)getPathForAssetsInReleaseConfig:(NSString *)resourcePath;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@airborne_sdk_iOS/hyper-ota/Airborne/AirborneObjC/include/AJPApplicationManager.h` around lines 178 - 192, The header and implementation use different parameter names for the same method: normalize the parameter name for - (NSString * _Nullable)getPathForAssetsInReleaseConfig:(NSString *)filePath; so it matches the implementation (or vice versa) — e.g., rename the parameter in the implementation of getPathForAssetsInReleaseConfig: from resourcePath to filePath (or rename the header param to resourcePath) and keep the _Nullable annotation and existing comments unchanged to improve readability/consistency.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@airborne_sdk_android/airborne/src/main/java/in/juspay/airborne/ota/UpdateTask.kt`:
- Around line 251-256: The condition computing shouldDownloadNewLazy can become
true when updateTimedOut is true even if presult is Update.Package.NA, causing
duplicate lazy-split download attempts; update the logic in UpdateTask.kt so
shouldDownloadNewLazy only becomes true when fetched != null, newReleaseFailed
is false, and presult is not an Update.Package.NA (i.e., add a guard like
presult !is Update.Package.NA) before calling downloadLazySplits(tw,
fetched.pkg, ...), ensuring downloadLazySplits is not invoked for the NA case
and avoiding unnecessary TempWriter allocation/callbacks.
In
`@airborne_sdk_iOS/hyper-ota/Airborne/AirborneObjC/ApplicationManager/AJPApplicationManager.m`:
- Around line 259-271: The _downloadedSplits set is populated from self.package
allImportantSplits without verifying files exist on disk; update the bootstrap
so that for each AJPResource returned by [self.package allImportantSplits] you
check disk existence (use NSFileManager fileExistsAtPath: on resource.filePath)
before adding to _downloadedSplits, and similarly ensure AJPLazyResource entries
in self.package.lazy are only added when lazy.isDownloaded is true and the file
actually exists; reference AJPApplicationManager's _downloadedSplits
initialization, AJPResource.filePath, AJPLazyResource, self.package.lazy and
self.resources.resources when making the change (or alternatively adjust the
comment to state the manifest-trust assumption if you intentionally skip the
check).
- Around line 686-709: Replace all `@synchronized`(self->_downloadedSplits) /
`@synchronized`(strongSelf->_downloadedSplits) usages with the shared
collectionsLock to make access to _downloadedSplits consistent with other
guarded collections: wrap reads and writes to _downloadedSplits in
[self.collectionsLock lock] / [self.collectionsLock unlock] (or equivalent)
instead of synchronized blocks so methods like getPathForAssetsInReleaseConfig
and getDownloadedSplits and mutation sites (moveLazyPackageFromTempToMain,
moveResourceToMainAndUpdate, downloadResourcesWithCurrentResources, and the
rebuild block in tryDownloadingUpdate) use the same lock as
_availableLazySplits, _availableResources, and _downloadedApplicationManifest.
- Around line 800-839: The rebuild clears _downloadedSplits after
downloadResourcesWithCurrentResources may have already added unchanged
resources, causing a race; fix by adding the unchanged resources into the
rebuild itself: inside the `@synchronized`(strongSelf->_downloadedSplits) block
(in AJPApplicationManager where you call getResourcesFrom:filtering: to compute
toDownload and pendingLazyPaths), after adding allImportantSplits and the
downloadedLazy entries, also iterate the same source of current/installed
resources (the set used by downloadResourcesWithCurrentResources — e.g.,
strongSelf.currentResources/currentLazy or the collection returned from
getResourcesFrom:filtering: logic) and add any resource.filePath that is not in
pendingLazyPaths to _downloadedSplits so unchanged resources are present
regardless of concurrency with downloadResourcesWithCurrentResources; keep the
rest of the moveLazyPackageFromTempToMain and notification flow intact.
---
Outside diff comments:
In
`@airborne_sdk_android/airborne/src/main/java/in/juspay/airborne/ota/UpdateTask.kt`:
- Around line 239-259: The newReleaseFailed flag is currently set using
"!didPackageUpdate" which makes Update.Package.NA be treated as a failure;
change the assignment of newReleaseFailed (and any logic that uses it) to only
consider actual failure states—e.g., base it on presult being
Update.Package.Failed (and still respect updateTimedOut.get()) rather than
"!didPackageUpdate" or explicitly exclude presult == Update.Package.NA; update
the references to newReleaseFailed and the shouldDownloadNewLazy computation
(which checks presult is Update.Package.Finished) so NA does not trigger the
"Skipping new release lazy splits download due to download failure." branch and
only real failures set newReleaseFailed.
---
Nitpick comments:
In
`@airborne_sdk_android/airborne/src/main/java/in/juspay/airborne/ota/UpdateTask.kt`:
- Line 231: The call uses a redundant non-null assertion "presult!!" after a
local null-check; remove the "!!" and rely on Kotlin's smart-cast (or wrap the
use in presult?.let { ... }) so the compiler treats presult as non-null when
calling saveDownloadedPackages; update the call site in UpdateTask
(saveDownloadedPackages and fetched.pkg usage) to either call
saveDownloadedPackages(presult, fetched.pkg) after the null guard or enclose the
call in presult?.let { saveDownloadedPackages(it, fetched.pkg) } for clarity.
In
`@airborne_sdk_iOS/hyper-ota/Airborne/AirborneObjC/ApplicationManager/AJPApplicationManager.m`:
- Around line 1362-1452: The method retryFailedLazyDownloads duplicates
retryFailedLazyDownloadsWithCompletion:; replace the entire implementation of
retryFailedLazyDownloads to simply call [self
retryFailedLazyDownloadsWithCompletion:nil] (delegate to the completion-taking
variant) to remove the copy-pasted logic and keep a single source of truth
(ensure the selector name retryFailedLazyDownloadsWithCompletion: and the
parameter nil are used exactly as in the existing code).
In
`@airborne_sdk_iOS/hyper-ota/Airborne/AirborneObjC/include/AJPApplicationManager.h`:
- Around line 178-192: The header and implementation use different parameter
names for the same method: normalize the parameter name for - (NSString *
_Nullable)getPathForAssetsInReleaseConfig:(NSString *)filePath; so it matches
the implementation (or vice versa) — e.g., rename the parameter in the
implementation of getPathForAssetsInReleaseConfig: from resourcePath to filePath
(or rename the header param to resourcePath) and keep the _Nullable annotation
and existing comments unchanged to improve readability/consistency.
Yash02Rajput
force-pushed
the
ios-track-downloaded-splits
branch
2 times, most recently
from
36d8177 to
01a3003
Compare
Yash02Rajput
force-pushed
the
ios-track-downloaded-splits
branch
2 times, most recently
from
c70cc0a to
0578957
Compare
balaganesh-juspay
previously approved these changes
Mar 3, 2026
Yash02Rajput
force-pushed
the
ios-track-downloaded-splits
branch
from
0578957 to
414eb55
Compare
Yash02Rajput
force-pushed
the
ios-track-downloaded-splits
branch
from
414eb55 to
eb6f5dd
Compare
balaganesh-juspay
approved these changes
Mar 26, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Bug Fixes
Improvements