fix(ios): stop package install from wiping unchanged nested assets - #373
Open
yuvrajjsingh0 wants to merge 1 commit into
Open
yuvrajjsingh0 wants to merge 1 commit into
yuvrajjsingh0 wants to merge 1 commit into
Conversation
`moveAllPackagesFromTempToMain` listed the temp directory non-recursively and
handed each top-level entry to `movePackageFromTempToMain`, which deletes the
destination before moving. For a package whose splits live in subdirectories
that meant `main/assets` was replaced wholesale by whatever the update happened
to stage in temp — and since `getResourcesFrom` only queues splits whose URL or
checksum changed, every unchanged file in that directory was deleted and never
re-fetched. `isAppInstalled` then failed on the first missing file ("package
copy failed"), so the manifest was never updated even though the new index
bundle had already been moved into place: the app booted the new bundle against
a half-deleted asset tree, and stayed pinned to the old version on every
subsequent launch.
Enumerate temp recursively so every entry moved is a file, which is what
`handleTempPackageInstallation` already does when installing a package staged
after a boot timeout, and what the Android SDK does via `TempWriter.copyToMain`.
Intermediate directories are created by `fullPathInStorageForFilePath`, so
nested paths need nothing further.
Also make the download filter self-healing: an important split missing from
`main/` is queued even when both manifests agree on it. Without this, devices
already in the broken state stay stuck indefinitely, because the manifest diff
never consults the filesystem. Lazy splits keep the manifest-only comparison —
absence from disk is their normal state until they are downloaded.
`file_moved_to_main` is now emitted once with a count rather than once per file,
since a package can carry hundreds of nested assets and this runs on the boot
path. Move failures are still logged individually.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Review changes with Changed Files |
WalkthroughOTA resource selection now optionally verifies files in ChangesOTA resource and package updates
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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: 1
🧹 Nitpick comments (1)
airborne_sdk_iOS/hyper-ota/AirborneTestAppTests/AJPApplicationManagerUtilsTests.swift (1)
308-317: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAlso test
.jsastored as its mapped.jsfilename.The current test covers
filesInMain.contains(filePath)only; add abundle.jsasplit withbundle.json disk to protect thejsFileName(for:)branch.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@airborne_sdk_iOS/hyper-ota/AirborneTestAppTests/AJPApplicationManagerUtilsTests.swift` around lines 308 - 317, Extend testGetResourcesFrom_requiringPresence_jsaSplitStoredUnderRawName_skips to also create the split resource on disk under its mapped bundle.js filename, then assert the same installed-resource behavior through the jsFileName(for:) branch. Preserve the existing raw-name case and empty-result expectation.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@airborne_sdk_iOS/hyper-ota/Airborne/AirborneSwift/AJPApplicationManagerUtils.swift`:
- Around line 250-263: Update movePackageFromTempToMain to create the
destination parent directory with intermediate directories before replacing or
moving the package file. Derive the parent from the destination path so nested
entries such as main/assets/... succeed even when absent, while preserving the
existing replacement and move behavior.
---
Nitpick comments:
In
`@airborne_sdk_iOS/hyper-ota/AirborneTestAppTests/AJPApplicationManagerUtilsTests.swift`:
- Around line 308-317: Extend
testGetResourcesFrom_requiringPresence_jsaSplitStoredUnderRawName_skips to also
create the split resource on disk under its mapped bundle.js filename, then
assert the same installed-resource behavior through the jsFileName(for:) branch.
Preserve the existing raw-name case and empty-result expectation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: fa7ea651-5be8-40d1-8b49-aee59ee8fa00
📒 Files selected for processing (3)
airborne_sdk_iOS/hyper-ota/Airborne/AirborneSwift/AJPApplicationManager.swiftairborne_sdk_iOS/hyper-ota/Airborne/AirborneSwift/AJPApplicationManagerUtils.swiftairborne_sdk_iOS/hyper-ota/AirborneTestAppTests/AJPApplicationManagerUtilsTests.swift
Comment on lines
+250
to
+263
| // Enumerated recursively so every entry handed to `movePackageFromTempToMain` is a file. | ||
| // Moving a top-level entry instead would hand it a *directory*, and that call deletes the | ||
| // destination before moving: `main/assets` would lose every file this update's delta did | ||
| // not re-download. This is the enumeration `handleTempPackageInstallation` already uses | ||
| // when installing a package staged after a boot timeout. | ||
| let tempFiles = getAllFilesInDirectory(AJPApplicationConstants.JUSPAY_PACKAGE_DIR, | ||
| subFolder: AJPApplicationConstants.JUSPAY_TEMP_DIR, | ||
| includeSubfolders: true) | ||
|
|
||
| var movedCount = 0 | ||
| for fileName in tempFiles { | ||
| do { | ||
| try movePackageFromTempToMain(fileName) | ||
| let map = NSMutableDictionary() | ||
| map["file"] = fileName | ||
| tracker.trackInfo("file_moved_to_main", value: map) | ||
| movedCount += 1 |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
ast-grep outline airborne_sdk_iOS/hyper-ota/Airborne/AirborneSwift/AJPApplicationManagerUtils.swift \
--match movePackageFromTempToMain --view expanded
rg -n -C3 'moveItem\(atPath:.*toPath:|createDirectory\(atPath:' \
airborne_sdk_iOS/hyper-ota/Airborne/AirborneSwift/AJPApplicationManagerUtils.swiftRepository: juspay/airborne
Length of output: 935
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="airborne_sdk_iOS/hyper-ota/Airborne/AirborneSwift/AJPApplicationManagerUtils.swift"
echo "## relevant outline"
ast-grep outline "$file" \
--match movePackageFromTempToMain --view expanded || true
echo "## inspect function and surrounding call site"
sed -n '200,320p' "$file" | cat -n
echo "## constants and directory helper usages"
rg -n 'AJPApplicationConstants\.JUSPAY_(PACKAGE_DIR|TEMP_DIR)|movePackageFromTempToMain|getAllFilesInDirectory|main/assets|assets' "$file"Repository: juspay/airborne
Length of output: 9070
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "## moveAllPackagesFromTempToMain with line numbers"
file="airborne_sdk_i iOS/hyper-ota/Airborne/AirborneSwift/AJPApplicationManagerUtils.swift"
sed -n '50,85p' "$file" | cat -n
echo "## directory and file manager helpers"
rg -n 'func removeItem(atPath:|func createDirectory|createDirectory\(atPath:|fullPathInStorageForFilePath|cleanupEntireDirectory|getAllFilesInDirectory' airborne_sdk_iOS/hyper-ota/Airborne/AirborneSwift
echo "## behavioral probe for swift FileManager.moveItem without intermediate parent"
python3 - <<'PY'
import os, tempfile
with tempfile.TemporaryDirectory() as d:
root = os.path.join(d, "root")
os.makedirs(root)
src_parent = os.path.join(root, "src")
os.makedirs(src_parent)
src = os.path.join(src_parent, "file.txt")
with open(src, "w") as f: f.write("x")
dst = os.path.join(root, "nested", "assets", "file.txt")
with open(src, "w") as f: f.write("x")
for attempt in range(2):
print("attempt", attempt, "exists_parent", os.path.exists(os.path.join(root, "nested")))
try:
os.rename(src, dst)
print("successful")
break
except FileNotFoundError as e:
print("FileNotFoundError", e)
PYRepository: juspay/airborne
Length of output: 332
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="airborne_sdk_iOS/hyper-ota/Airborne/AirborneSwift/AJPApplicationManagerUtils.swift"
echo "## inspect movePackageFromTempToMain"
sed -n '210,235p' "$file" | cat -n
echo "## inspect moveAllPackagesFromTempToMain"
sed -n '242,278p' "$file" | cat -n
echo "## directory and filesystem helpers"
rg -n 'func removeItem\(atPath:|func createDirectory|createDirectory\(atPath:|fullPathInStorageForFilePath|cleanupEntireDirectory|getAllFilesInDirectory' airborne_sdk_iOS/hyper-ota/Airborne/AirborneSwift || true
echo "## behavioral probe for moveItem without intermediate parent"
python3 - <<'PY'
import os, tempfile
with tempfile.TemporaryDirectory() as d:
root = os.path.join(d, "root")
os.makedirs(root)
os.makedirs(os.path.join(root, "src"))
src = os.path.join(root, "src", "file.txt")
with open(src, "w") as f: f.write("x")
dst = os.path.join(root, "nested", "assets", "file.txt")
print("exists_parent", os.path.exists(os.path.join(root, "nested")))
try:
os.rename(src, dst)
print("moved")
except FileNotFoundError as e:
print("FileNotFoundError", repr(str(e)))
PYRepository: juspay/airborne
Length of output: 10373
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "## cleanupEntireDirectory implementation"
rg -n -C5 'func cleanupEntireDirectory' airborne_sdk_iOS/hyper-ota/Airborne/AirborneSwift
echo "## full move operations candidates"
rg -n -C4 'func move|let fileManager = FileManager.default|createDirectory|removeItem|moveItem' airborne_sdk_iOS/hyper-ota/Airborne/AirborneSwift/AJPApplicationManagerUtils.swiftRepository: juspay/airborne
Length of output: 194
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "## fileUtil references"
rg -n -C3 '\.fileUtil|fileUtil\s+:' airborne_sdk_iOS/hyper-ota/Airborne/AirborneSwift || true
echo "## cleanupEntireDirectory implementation with file extension expansion"
rg -n -C5 'cleanupEntireDirectory' airborne_sdk_iOS || true
echo "## full move operation area from Utils"
sed -n '370,455p' airborne_sdk_iOS/hyper-ota/Airborne/AirborneSwift/AJPApplicationManagerUtils.swift | cat -nRepository: juspay/airborne
Length of output: 14410
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "## AJPFileUtil.cleanupEntireDirectory implementation"
sed -n '250,340p' airborne_sdk_iOS/hyper-ota/Airborne/AirborneSwiftCore/AJPFileUtil.swift | cat -n
echo "## AJPFileUtil.fileUtil storage path constructor"
rg -n -C5 'func fullPathInStorageForFilePath|fullPathInStorageForFilePath' airborne_sdk_iOS/hyper-ota/Airborne/AirborneSwiftCore/AJPFileUtil.swift | head -n 120
echo "## constants JUSPAY_MAIN_DIR and JUSPAY_TEMP_DIR"
rg -n -C3 'JUSPAY_(MAIN_DIR|TEMP_DIR|PACKAGE_DIR)' airborne_sdk_iOSRepository: juspay/airborne
Length of output: 50371
Create destination parent directories before moving nested package files.
movePackageFromTempToMain() removes mainPath only when it already exists, then runs moveItem(atPath:toPath:) without creating JuspayPackages/main/assets/... first, so a new first-time nested asset fails this path. Add createDirectory(atPath:withIntermediateDirectories:attributes:) for the destination directory before the replace/move.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@airborne_sdk_iOS/hyper-ota/Airborne/AirborneSwift/AJPApplicationManagerUtils.swift`
around lines 250 - 263, Update movePackageFromTempToMain to create the
destination parent directory with intermediate directories before replacing or
moving the package file. Derive the parent from the destination path so nested
entries such as main/assets/... succeed even when absent, while preserving the
existing replacement and move behavior.
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
1 participant
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.
Symptom
An OTA update downloads successfully, then fails to install, and the app stays pinned to its old package version — permanently. From a production device:
Root cause
Two pieces that contradict each other:
getResourcesFromonly queues splits whose URL or checksum changed. Unchanged files are expected to already be on disk.moveAllPackagesFromTempToMainlisted temp withcontentsOfDirectory(non-recursive), so for a package whose splits live in subdirectories it handedmovePackageFromTempToMaina directory — and that call deletes the destination before moving.main/assetswas replaced wholesale by whatever the delta happened to stage, deleting every unchanged file under it.isAppInstalledthen failed on the first missing file, so the manifest was never updated — but the new index bundle had already been moved into place. The app booted the new bundle against a half-deleted asset tree while the SDK believed it was still on the old version, and since the delta filter never consults the filesystem, the deleted files were never re-fetched. Every later launch repeated the same failure.This is not a regression — the shape is unchanged since the original iOS import (
ff00b45); the Swift migration (92b7ed0) was a faithful port. What changed recently is the app-version wipe added in9ec317b/5a9b1b4, which resets state on every app update and so hides the bug until the first delta OTA after it.Fix
handleTempPackageInstallationalready does on the boot-timeout path, and what the Android SDK does viaTempWriter.copyToMain. Intermediate directories are created byfullPathInStorageForFilePath, so nested paths need nothing further.main/is queued even when both manifests agree on it. Without this, devices already broken stay broken. Lazy splits keep the manifest-only comparison (absence from disk is their normal state), via a defaultedrequiringPresenceInMainparameter passed only at the important-splits call site.Behavior change to be aware of
file_moved_to_mainis now emitted once with acountrather than once per file — a package can carry hundreds of nested assets and this runs on the boot path. Any log query keyed on itsfilefield needs updating. Move failures are still logged per file.Testing
Unit — 11 tests added to
AJPApplicationManagerUtilsTests(6 for the move, 5 for the presence filter). The core regression test was verified as a genuine negative control: it fails against the old enumeration. Full suite otherwise green apart from three failures that pre-exist on a clean tree (two network-dependent suites plustestGetCurrentApplicationManifest_configBootTimeout).End-to-end — driven through a real RN app on the simulator against a mock release-config server, two important splits nested at
assets/assets/images/,rev=aoriginal content andrev=bupdated:The pre-fix run reproduced the production log line for line, including
file_moved_to_main | file: assetsand the samefile_missingpath. Two notes from the matrix:toDownload.isEmptyearly return skips the install gate entirely, so the version bumped to v2 with the file still missing — silent corruption, nothing logged.Not addressed
tryDownloadingUpdateshort-circuits before the filter runs, so nothing heals. Recovery still needs a new release or an app-version bump.main.jsbundlein place rather than staging the package atomically..zipsplits are saved but never extracted;.jsasplits are written under their raw name while the install gate looks for the.jsform.No docs impact: no public API, CLI, env-var, or dashboard surface, and these tracker events are not referenced in
airborne_docs/.🤖 Generated with Claude Code
Summary by CodeRabbit
.jsand.jsaresource files when checking installed content.