fix(harmony): prepend file:// scheme to getURL() to enable local image loading from hot update bundle#597
Conversation
…e loading from hot update bundle
📝 WalkthroughWalkthroughThe getURL() method in PushyFileJSBundleProvider.ets was modified to normalize the bundle URL path by prefixing it with "file://" when the resolved path does not already have that prefix, rather than returning the raw path directly. ChangesURL Normalization
Estimated code review effort: 1 (Trivial) | ~2 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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: 1
🧹 Nitpick comments (1)
harmony/pushy/src/main/ets/PushyFileJSBundleProvider.ets (1)
19-23: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueInconsistent path normalization between
getURL()andgetBundle().
getURL()now returns afile://-prefixed path, butgetBundle()(lines 27-37) continues to use the raw path fromupdateContext.getBundleUrl()for filesystem operations (fs.access) and the returnedfilePath. This is likely intentional since@ohos.file.fsAPIs expect raw sandbox paths, not URI schemes. However, this creates a subtle contract where two methods on the same provider return different path formats for the same logical resource.Consider adding a clarifying code comment in
getURL()explaining whygetBundle()deliberately does not prefix, to prevent future maintainers from "fixing" the inconsistency and breaking filesystem access.getURL(): string { // AssetSourceResolver expects file:// URIs for local asset resolution, // while fs.access in getBundle() requires raw sandbox paths. const path = this.updateContext.getBundleUrl(); ... }🤖 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 `@harmony/pushy/src/main/ets/PushyFileJSBundleProvider.ets` around lines 19 - 23, The path format is intentionally different between getURL() and getBundle(), so add a clarifying comment in PushyFileJSBundleProvider.ets around getURL() explaining that it returns a file:// URI for asset resolution while getBundle() must keep using the raw updateContext.getBundleUrl() value for fs.access and filePath handling. Use the getURL() and getBundle() methods as the anchor points so future maintainers do not “normalize” both paths and break filesystem access.
🤖 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 `@harmony/pushy/src/main/ets/PushyFileJSBundleProvider.ets`:
- Around line 18-24: getURL() may return undefined despite being declared as
returning string, because PushyFileJSBundleProvider.getURL falls through to
return path when updateContext.getBundleUrl() is empty or undefined. Update
getURL() to explicitly handle the falsy path case by returning a valid string,
or change the method signature to string | undefined if that is the intended
contract, while keeping the existing file:// prefix logic intact.
---
Nitpick comments:
In `@harmony/pushy/src/main/ets/PushyFileJSBundleProvider.ets`:
- Around line 19-23: The path format is intentionally different between getURL()
and getBundle(), so add a clarifying comment in PushyFileJSBundleProvider.ets
around getURL() explaining that it returns a file:// URI for asset resolution
while getBundle() must keep using the raw updateContext.getBundleUrl() value for
fs.access and filePath handling. Use the getURL() and getBundle() methods as the
anchor points so future maintainers do not “normalize” both paths and break
filesystem access.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: acf21678-1939-4bec-be2f-8ec346c6b789
📒 Files selected for processing (1)
harmony/pushy/src/main/ets/PushyFileJSBundleProvider.ets
| getURL(): string { | ||
| return this.updateContext.getBundleUrl(); | ||
| const path = this.updateContext.getBundleUrl(); | ||
| if (path && !path.startsWith('file://')) { | ||
| return 'file://' + path; | ||
| } | ||
| return path; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
getURL() can return undefined violating its string return type.
this.updateContext.getBundleUrl() may return undefined (or empty string), and when path is falsy, the method falls through to return path (line 23), returning undefined despite the string type annotation. This could cause runtime issues for consumers expecting a valid URL string.
Add explicit handling for the undefined/empty case, or adjust the return type to string | undefined if that's the intended contract.
getURL(): string | undefined {
const path = this.updateContext.getBundleUrl();
if (!path) {
return undefined;
}
if (!path.startsWith('file://')) {
return 'file://' + path;
}
return path;
}
Or if empty path should never happen, throw or return empty string with a warning.
🤖 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 `@harmony/pushy/src/main/ets/PushyFileJSBundleProvider.ets` around lines 18 -
24, getURL() may return undefined despite being declared as returning string,
because PushyFileJSBundleProvider.getURL falls through to return path when
updateContext.getBundleUrl() is empty or undefined. Update getURL() to
explicitly handle the falsy path case by returning a valid string, or change the
method signature to string | undefined if that is the intended contract, while
keeping the existing file:// prefix logic intact.
1 participant
This PR fixes local image assets (imported via
require('./image.png')) not displaying when loaded from a hot-updated bundle on React Native OpenHarmony.Root Cause
AssetSourceResolver.harmony.ts, local assets resolve relative to the bundle URL if and only if the bundle URL starts with thefile://scheme.PushyFileJSBundleProvider.ets'sgetURL()returned the raw absolute path (/data/storage/.../bundle.harmony.js) without thefile://prefix.AssetSourceResolverto resolve the asset's URI asasset://assets/.../image.png, which ArkUI'sRNImage.etscompiles into a compile-time$rawfile(...)macro. Since hot-updated assets reside in the sandbox at runtime, this lookup failed, resulting in blank/invisible images.file://togetURL()causesAssetSourceResolverto resolve local assets asfile:///data/storage/.../assets/image.png, which ArkUI'sImage()component can load directly from the device's sandbox.Summary by CodeRabbit
file://format.file://paths could be returned without normalization, helping ensure more reliable app loading behavior.