fix(harmony): safeguard binary version sync against background-thread resource loading failures#591
fix(harmony): safeguard binary version sync against background-thread resource loading failures#591sunnylqm wants to merge 2 commits into
Conversation
…ors and cache buildTime
📝 WalkthroughWalkthroughAdds ChangesiOS Bridging Header Expo Plugin + Example App
Harmony UpdateContext and core cleanup
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
app.plugin.jsOops! Something went wrong! :( ESLint: 8.57.1 Error: .eslintrc.js » harmony/pushy/src/main/ets/UpdateContext.tsOops! Something went wrong! :( ESLint: 8.57.1 Error: .eslintrc.js » 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: 3
🤖 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 `@app.plugin.js`:
- Around line 29-36: Guard the bridging header handling in app.plugin.js so the
iOS prebuild does not fail when `${projectName}-Bridging-Header.h` is missing.
In the logic around `fs.readFileSync(bridgingHeaderPath, "utf8")`, add a check
for file existence first (or create the file if that is the intended behavior)
before reading or writing. Keep the import-injection behavior tied to
`RCT_BRIDGE_IMPORT`, but skip gracefully when the bridging header is absent.
- Around line 4-14: The Expo plugin entry is pointing to a missing local module,
so prebuild cannot resolve the plugin. Update the app.json plugin reference to
an existing plugin module or add the expected file under the project’s plugins
directory, and make sure the plugin exports from app.plugin.js are actually
reachable by the configured entry path. Use the app.plugin.js and
withDangerousMod symbols to locate the plugin wiring.
In `@harmony/pushy/src/main/ets/UpdateContext.ts`:
- Around line 68-73: The fallback in UpdateContext.getBundleVersion is using the
sentinel string 'Unknown', which prevents the new empty-version guard from
detecting a failed bundle lookup. Change the cached package version assignment
so a missing or empty bundleInfo.versionName resolves to an empty string
instead, and keep the logic in getBundleVersion and syncStateWithBinaryVersion
aligned so the latter only runs when a real binary version is available.
🪄 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: 894f49f5-d953-4042-9c26-84d064a58dbe
⛔ Files ignored due to path filters (1)
Example/expoUsePushy/bun.lockis excluded by!**/*.lock
📒 Files selected for processing (5)
Example/expoUsePushy/app.jsonExample/expoUsePushy/package.jsonapp.plugin.jsharmony/pushy/src/main/ets/UpdateContext.tssrc/core.ts
💤 Files with no reviewable changes (1)
- src/core.ts
| function requireConfigPlugins() { | ||
| try { | ||
| return require("@expo/config-plugins"); | ||
| } catch { | ||
| return require(require.resolve("@expo/config-plugins", { | ||
| paths: [process.cwd()], | ||
| })); | ||
| } | ||
| } | ||
|
|
||
| const { withDangerousMod } = requireConfigPlugins(); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Locate the plugin file referenced by app.json
fd -uu -t f 'with-rct-bridge-bridging-header'
fd -uu -t f 'app.plugin.js'Repository: reactnativecn/react-native-update
Length of output: 185
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the example config and plugin entrypoint references
git ls-files 'Example/expoUsePushy/*' 'Example/expoUsePushy/**/*' 'app.plugin.js'
printf '\n--- app.json ---\n'
sed -n '1,220p' Example/expoUsePushy/app.json
printf '\n--- app.plugin.js ---\n'
cat -n app.plugin.js
printf '\n--- plugin path search ---\n'
fd -uu -t f 'with-rct-bridge-bridging-header' Example/expoUsePushy . || true
rg -n --hidden --glob '!**/node_modules/**' --glob '!**/dist/**' --glob '!**/build/**' '"\.\/plugins\/with-rct-bridge-bridging-header"|with-rct-bridge-bridging-header|app\.plugin\.js' Example/expoUsePushy .Repository: reactnativecn/react-native-update
Length of output: 3194
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- example package.json ---'
sed -n '1,220p' Example/expoUsePushy/package.json
printf '\n%s\n' '--- references to app.plugin.js / with-rct-bridge-bridging-header ---'
rg -n --hidden --glob '!**/node_modules/**' --glob '!**/dist/**' --glob '!**/build/**' 'app\.plugin\.js|with-rct-bridge-bridging-header|plugins/\./plugins|plugins"' Example/expoUsePushy .
printf '\n%s\n' '--- files named plugins or plugin entrypoints ---'
fd -uu -t f 'app.plugin.js' .
fd -uu -t d 'plugins' Example/expoUsePushy .Repository: reactnativecn/react-native-update
Length of output: 50389
🌐 Web query:
Expo config plugin relative path app.plugin.js app.json plugins "./plugins/with-rct-bridge-bridging-header" resolution
💡 Result:
In Expo, local config plugins referenced by a relative path in your app.json (e.g., "./plugins/with-rct-bridge-bridging-header") are resolved relative to the project root [1]. When you specify a string path in the plugins array, Expo CLI resolves it using Node's module resolution system [1]. For a local file like "./plugins/with-rct-bridge-bridging-header", Expo will attempt to load the file by appending supported extensions if necessary (such as.js,.ts,.cjs,.mjs, etc., following the latest resolution logic) [2]. Key points regarding this resolution: 1. Local vs. Module Resolution: If the string does not start with a relative path indicator (./ or../) or an absolute path, Expo treats it as an npm package [1]. Using./ ensures it is treated as a local file relative to your project's root directory [1]. 2. Supported Formats: As of May 2026, Expo has aligned config plugin resolution with standard app config resolution, supporting a wider range of formats including.js,.ts,.cjs,.mjs,.cts, and.mts [2]. This allows you to write local plugins in TypeScript or modern JavaScript without requiring manual pre-transpilation in some workflows [2]. 3. Plugin Structure: While npm packages are expected to export the plugin via an app.plugin.js file (or the main field in package.json) [3][4], a local file referenced directly by path is loaded as a module [1]. The exported function from that file will be treated as the config plugin [5][1]. 4. Troubleshooting: If your plugin fails to load, ensure the file exists at the path specified relative to the project root and that it exports the plugin function correctly (usually as the default export or via module.exports) [5][1]. Recent updates have improved error reporting for plugin resolution failures [2].
Citations:
- 1: https://expo-expo.mintlify.app/config-plugins/how-they-work
- 2: feat(config,config-plugins): Align config plugin resolution/loading with app configs expo/expo#45989
- 3: https://docs.expo.dev/config-plugins/development-for-libraries
- 4: [config-plugins] bring back resolution from package.json main field expo/expo#32355
- 5: https://docs.expo.dev/config-plugins/introduction/?redirected=
Point the Expo plugin entry at an existing module
Example/expoUsePushy/app.json points to "./plugins/with-rct-bridge-bridging-header", but there’s no matching module under Example/expoUsePushy/plugins/, and the root app.plugin.js won’t be loaded from that entry. Expo resolves local plugins relative to the project root, so prebuild will fail unless the path is corrected or the plugin file is added.
🧰 Tools
🪛 ast-grep (0.44.0)
[warning] 7-9: Avoid require with non-literal values
Context: require(require.resolve("@expo/config-plugins", {
paths: [process.cwd()],
}))
Note: [CWE-829] Inclusion of Functionality from Untrusted Control Sphere (dynamic require).
(detect-non-literal-require)
🤖 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 `@app.plugin.js` around lines 4 - 14, The Expo plugin entry is pointing to a
missing local module, so prebuild cannot resolve the plugin. Update the app.json
plugin reference to an existing plugin module or add the expected file under the
project’s plugins directory, and make sure the plugin exports from app.plugin.js
are actually reachable by the configured entry path. Use the app.plugin.js and
withDangerousMod symbols to locate the plugin wiring.
| const contents = fs.readFileSync(bridgingHeaderPath, "utf8"); | ||
|
|
||
| if (!contents.includes(RCT_BRIDGE_IMPORT)) { | ||
| fs.writeFileSync( | ||
| bridgingHeaderPath, | ||
| `${contents.trimEnd()}\n\n${RCT_BRIDGE_IMPORT}\n` | ||
| ); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Guard against a missing bridging header before reading.
fs.readFileSync will throw an opaque ENOENT if ${projectName}-Bridging-Header.h doesn't exist (e.g. depending on prebuild ordering or a project without one), failing the entire iOS prebuild. Consider skipping (or creating the file) when it's absent.
🛡️ Proposed guard
+ if (!fs.existsSync(bridgingHeaderPath)) {
+ return config;
+ }
+
const contents = fs.readFileSync(bridgingHeaderPath, "utf8");📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const contents = fs.readFileSync(bridgingHeaderPath, "utf8"); | |
| if (!contents.includes(RCT_BRIDGE_IMPORT)) { | |
| fs.writeFileSync( | |
| bridgingHeaderPath, | |
| `${contents.trimEnd()}\n\n${RCT_BRIDGE_IMPORT}\n` | |
| ); | |
| } | |
| if (!fs.existsSync(bridgingHeaderPath)) { | |
| return config; | |
| } | |
| const contents = fs.readFileSync(bridgingHeaderPath, "utf8"); | |
| if (!contents.includes(RCT_BRIDGE_IMPORT)) { | |
| fs.writeFileSync( | |
| bridgingHeaderPath, | |
| `${contents.trimEnd()}\n\n${RCT_BRIDGE_IMPORT}\n` | |
| ); | |
| } |
🧰 Tools
🪛 ast-grep (0.44.0)
[warning] 31-34: Filesystem path is not a string literal; a request-/variable-derived path can enable path traversal. Validate and normalize the path before use.
Context: fs.writeFileSync(
bridgingHeaderPath,
${contents.trimEnd()}\n\n${RCT_BRIDGE_IMPORT}\n
)
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(detect-non-literal-fs-filename)
🤖 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 `@app.plugin.js` around lines 29 - 36, Guard the bridging header handling in
app.plugin.js so the iOS prebuild does not fail when
`${projectName}-Bridging-Header.h` is missing. In the logic around
`fs.readFileSync(bridgingHeaderPath, "utf8")`, add a check for file existence
first (or create the file if that is the intended behavior) before reading or
writing. Keep the import-injection behavior tied to `RCT_BRIDGE_IMPORT`, but
skip gracefully when the bridging header is absent.
| try { | ||
| const bundleInfo = bundleManager.getBundleInfoForSelfSync( | ||
| this.getBundleFlags(), | ||
| ); | ||
| return bundleInfo?.versionName || 'Unknown'; | ||
| UpdateContext.cachedPackageVersion = bundleInfo?.versionName || 'Unknown'; | ||
| return UpdateContext.cachedPackageVersion; |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Return an empty package version instead of 'Unknown'.
Line 72 turns a failed/empty versionName lookup into a truthy sentinel, so the new guard at Lines 259-261 will not fire. That still lets syncStateWithBinaryVersion() run with a synthetic binary version and can clear persisted update state on a false mismatch.
Proposed fix
try {
const bundleInfo = bundleManager.getBundleInfoForSelfSync(
this.getBundleFlags(),
);
- UpdateContext.cachedPackageVersion = bundleInfo?.versionName || 'Unknown';
+ UpdateContext.cachedPackageVersion = bundleInfo?.versionName || '';
return UpdateContext.cachedPackageVersion;
} catch (error) {
console.error('Failed to get bundle info:', error);
return '';
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| try { | |
| const bundleInfo = bundleManager.getBundleInfoForSelfSync( | |
| this.getBundleFlags(), | |
| ); | |
| return bundleInfo?.versionName || 'Unknown'; | |
| UpdateContext.cachedPackageVersion = bundleInfo?.versionName || 'Unknown'; | |
| return UpdateContext.cachedPackageVersion; | |
| try { | |
| const bundleInfo = bundleManager.getBundleInfoForSelfSync( | |
| this.getBundleFlags(), | |
| ); | |
| UpdateContext.cachedPackageVersion = bundleInfo?.versionName || ''; | |
| return UpdateContext.cachedPackageVersion; |
🤖 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/UpdateContext.ts` around lines 68 - 73, The
fallback in UpdateContext.getBundleVersion is using the sentinel string
'Unknown', which prevents the new empty-version guard from detecting a failed
bundle lookup. Change the cached package version assignment so a missing or
empty bundleInfo.versionName resolves to an empty string instead, and keep the
logic in getBundleVersion and syncStateWithBinaryVersion aligned so the latter
only runs when a real binary version is available.
1 participant
This PR implements a safety guard and caching layer for binary version synchronization to prevent update state wipes during soft reloads:
Background & Issue
During a soft reload, the React Native container initializes its TurboModules (including
Pushy) on a background JS thread. TheUpdateContextconstructor callssyncStateWithBinaryVersion()which tries to readmeta.jsonsynchronously viaresourceManager.getRawFileContentSync('meta.json').In HarmonyOS, synchronous resource manager APIs are not thread-safe and throw exceptions if called from non-UI threads. The exception is silently caught, returning an empty string
"". Because this empty string doesn't match the previously stored build time, the system assumes the binary has been updated and clears the entire update Preferences.Solution
syncStateWithBinaryVersion():packageVersionandbuildTimeinside static class fields during the first resolution (which always runs on the Main/UI thread in the bundle provider).Unit tests pass successfully.
Summary by CodeRabbit
New Features
Bug Fixes