fix(harmony): 修复首次加载标记重复消费导致误触发 rollback#586
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a runtime ChangesDynamic getCurrentVersion flow
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
harmony/pushy/src/main/ets/PushyTurboModule.ts (1)
78-85: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winConsume the marker only after fallible constants work succeeds.
Line 78 deletes the first-load marker before
clearRollbackMark()runs. If that later state operation throws,getConstants()will not return to JS, but the marker is already gone, so a retry reportsisFirstTime: false.Proposed fix
- const isFirstTime = this.context.consumeFirstLoadMarker(); const rolledBackVersion = this.context.rolledBackVersion(); const uuid = this.context.getKv('uuid'); const isUsingBundleUrl = this.context.getIsUsingBundleUrl(); if (rolledBackVersion) { this.context.clearRollbackMark(); } + + const isFirstTime = this.context.consumeFirstLoadMarker();🤖 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/PushyTurboModule.ts` around lines 78 - 85, Move the first-load marker consumption in PushyTurboModule.getConstants so it happens only after the fallible state work succeeds, especially after clearRollbackMark() in the rolledBackVersion branch. Use the existing symbols consumeFirstLoadMarker(), clearRollbackMark(), and getConstants() to reorder the logic so a throw leaves the marker intact for retry.
🤖 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/UpdateContext.ts`:
- Around line 29-31: Make `ignoreRollback` process-wide instead of being reset
per `UpdateContext` instance; the current `private ignoreRollback = false` in
`UpdateContext` lets a new context in the same process re-enable rollback
checks. Update the `UpdateContext` class so the flag is shared across instances
(or stored on a process-level singleton/static), and ensure `getBundleUrl()` and
the rollback path around `shouldRollback`/`firstTime` consult and update that
shared state consistently so a second resolution in the same process cannot
trigger rollback again.
---
Outside diff comments:
In `@harmony/pushy/src/main/ets/PushyTurboModule.ts`:
- Around line 78-85: Move the first-load marker consumption in
PushyTurboModule.getConstants so it happens only after the fallible state work
succeeds, especially after clearRollbackMark() in the rolledBackVersion branch.
Use the existing symbols consumeFirstLoadMarker(), clearRollbackMark(), and
getConstants() to reorder the logic so a throw leaves the marker intact for
retry.
🪄 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: 1944a829-970a-4b7c-aec7-56fad44cedaa
📒 Files selected for processing (2)
harmony/pushy/src/main/ets/PushyTurboModule.tsharmony/pushy/src/main/ets/UpdateContext.ts
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/client.ts (1)
478-479: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winAvoid calling
getCurrentVersion()twice for the same check.
getCurrentVersion()is a synchronous native read; invoking it on both the comparison and the log line doubles the bridge call and risks the two reads diverging. Cache it once.♻️ Proposed refactor
- if (hash === getCurrentVersion()) { - log(`current hash ${getCurrentVersion()}, ignored`); + const current = getCurrentVersion(); + if (hash === current) { + log(`current hash ${current}, ignored`); return; }🤖 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 `@src/client.ts` around lines 478 - 479, The version check in the `hash === getCurrentVersion()` path calls `getCurrentVersion()` twice, which repeats the native read and can produce inconsistent values. Cache the result in a local variable before the comparison in `src/client.ts`, then reuse that same value in both the `if` condition and the `log` message so the check and output are based on one read.
🤖 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.
Nitpick comments:
In `@src/client.ts`:
- Around line 478-479: The version check in the `hash === getCurrentVersion()`
path calls `getCurrentVersion()` twice, which repeats the native read and can
produce inconsistent values. Cache the result in a local variable before the
comparison in `src/client.ts`, then reuse that same value in both the `if`
condition and the `log` message so the check and output are based on one read.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 61d1ff0e-b331-4a5b-93e1-7864d825eded
📒 Files selected for processing (5)
harmony/pushy/src/main/ets/PushyTurboModule.tspackage.jsonsrc/NativePushy.tssrc/client.tssrc/core.ts
✅ Files skipped from review due to trivial changes (1)
- package.json
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 @.gitignore:
- Line 64: The commented-out ignore rule for harmony/pushy.har appears to be
unintentional and would allow a generated artifact to be tracked. Re-enable the
existing .gitignore entry unless there is a deliberate decision to version this
file, and if tracking is intended, confirm that the artifact is stable and
documented in the PR description. Use the harmony/pushy.har ignore rule in
.gitignore as the target to locate and restore.
🪄 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: cf4b8b73-a583-49f2-bec7-718f69de6c0a
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (2)
.gitignoreharmony/pushy.har
|
|
||
| harmony/package | ||
| harmony/pushy.har | ||
| #harmony/pushy.har |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Verify this .gitignore change is intentional.
This change disables the ignore rule for harmony/pushy.har, but the PR objectives and summary don't mention tracking this artifact. HAR files are typically build outputs (like Android AARs). If this was commented out temporarily or unintentionally, re-enable the rule to prevent accidentally committing large binaries.
If the intent is to track a stable release artifact, document this in the PR description and ensure the file is meant to be version-controlled.
🤖 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 @.gitignore at line 64, The commented-out ignore rule for harmony/pushy.har
appears to be unintentional and would allow a generated artifact to be tracked.
Re-enable the existing .gitignore entry unless there is a deliberate decision to
version this file, and if tracking is intended, confirm that the artifact is
stable and documented in the PR description. Use the harmony/pushy.har ignore
rule in .gitignore as the target to locate and restore.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/UpdateContext.ts`:
- Around line 157-160: The fallback in UpdateContext’s persistence path
incorrectly calls Preferences.flush() without awaiting it, so the save can
complete before the data is durable and rejected Promises bypass the existing
try/catch. Remove the flush() fallback from the code path in
persistState()/consumeFirstLoadMarker() handling and keep using the synchronous
write method already used for preferences updates, so firstLoadMarked and
currentVersion are only reported after the state is actually written.
🪄 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: 5f3b7e29-9538-48ba-a67d-8fd4f0079712
📒 Files selected for processing (4)
harmony/pushy/oh-package-lock.json5harmony/pushy/src/main/ets/UpdateContext.tssrc/NativePushy.tssrc/core.ts
✅ Files skipped from review due to trivial changes (1)
- harmony/pushy/oh-package-lock.json5
🚧 Files skipped from review as they are similar to previous changes (2)
- src/core.ts
- src/NativePushy.ts
| if (typeof this.preferences.flush === 'function') { | ||
| this.preferences.flush(); | ||
| return; | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
In HarmonyOS / OpenHarmony Preferences API, what are the return type and persistence semantics of Preferences.flush()versusPreferences.flushSync()?
💡 Result:
In the HarmonyOS and OpenHarmony Preferences API, both flush and flushSync are used to persist cached data to a persistent file [1][2][3]. If no data has been modified or the new data matches the cached data, the system will not update the file [1][4][5]. The return types and execution semantics are as follows: flush Return Type: This method returns a Promise [1][4][5]. An alternative overload, flush(callback: AsyncCallback), is also available for callback-based asynchronous handling [1][2][6]. Persistence Semantics: It performs the operation asynchronously [1][2][3]. The data cached in memory is flushed to the persistent storage file without blocking the calling thread [1][7]. flushSync Return Type: This method returns void [1][4][5]. Persistence Semantics: It performs the operation synchronously [1][5]. The cached data is flushed to the persistent file immediately and directly within the calling thread [7]. Because this operation is synchronous, it is recommended to avoid calling it on the main (UI) thread to prevent potential performance degradation or UI lag [7].
Citations:
- 1: https://gitcode.com/openharmony/docs/blob/OpenHarmony-5.0.3-Release/en/application-dev/reference/apis-arkdata/js-apis-data-preferences.md
- 2: https://gitcode.com/openharmony/docs/blob/master/en/application-dev/database/data-persistence-by-preferences.md
- 3: https://gitcode.com/openharmony/docs/blob/OpenHarmony-5.0.3-Release/en/application-dev/database/data-persistence-by-preferences.md
- 4: https://gitcode.com/openharmony/docs/blob/OpenHarmony-5.0.2-Release/en/application-dev/reference/apis-arkdata/js-apis-data-sendablePreferences.md
- 5: https://gitcode.com/openharmony/docs/blob/OpenHarmony-5.0.2-Release/en/application-dev/reference/apis-arkdata/js-apis-data-preferences.md
- 6: https://developer.huawei.com/consumer/cn/doc/harmonyos-guides/data-persistence-by-preferences
- 7: https://dev.to/liu_yang_fc0e605820ac220c/basic-usage-and-tips-of-the-harmonyos-lightweight-database-databasehelper-3oh0
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="harmony/pushy/src/main/ets/UpdateContext.ts"
nl -ba "$FILE" | sed -n '1,240p'Repository: reactnativecn/react-native-update
Length of output: 211
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="harmony/pushy/src/main/ets/UpdateContext.ts"
echo "== around flushPreferences =="
sed -n '130,190p' "$FILE" | cat -n
echo
echo "== callers of flushPreferences/persistState/consumeFirstLoadMarker =="
rg -n "flushPreferences|persistState\\(|consumeFirstLoadMarker\\(|getConstants\\(|getBundleUrl\\(" "$FILE" "harmony/pushy/src/main/ets/PushyTurboModule.ts"Repository: reactnativecn/react-native-update
Length of output: 3529
Don't fall back to flush() here. Preferences.flush() is async, so this returns before the write is durable, and the try/catch won't catch a rejected Promise. That can let persistState() / consumeFirstLoadMarker() report success while firstLoadMarked / currentVersion are still stale on the next launch.
🤖 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 157 - 160, The
fallback in UpdateContext’s persistence path incorrectly calls
Preferences.flush() without awaiting it, so the save can complete before the
data is durable and rejected Promises bypass the existing try/catch. Remove the
flush() fallback from the code path in persistState()/consumeFirstLoadMarker()
handling and keep using the synchronous write method already used for
preferences updates, so firstLoadMarked and currentVersion are only reported
after the state is actually written.
1 participant
Committed at: 2026-06-29 18:46
Committed by: liuqiang
Summary by CodeRabbit