Conversation
Co-authored-by: sunnylqm <615282+sunnylqm@users.noreply.github.com>
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
|
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:
📝 WalkthroughWalkthroughConcurrent filesystem operations were introduced in DownloadTask (batched removals and parallel parent-directory creation), tests were added/updated (promiseAny tests; isInRollout tests and setup mock), and Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 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: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/__tests__/utils.test.ts`:
- Around line 50-75: The promiseAny implementation (promiseAny in src/utils.ts)
never settles for an empty array (lines ~23-38), so add a test case in the
utils.test.ts suite that calls promiseAny([]) and expects it to reject (e.g.,
toThrow('error_all_promises_rejected') or the canonical error you choose), and
also update the promiseAny function to immediately reject when input.length ===
0 (throw or return Promise.reject with the same 'error_all_promises_rejected'
error) so the test and callers do not hang.
🪄 Autofix (Beta)
✅ Autofix completed
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 374ff035-c285-4448-a363-b4684af4c593
📒 Files selected for processing (1)
src/__tests__/utils.test.ts
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. Fixes Applied SuccessfullyFixed 2 file(s) based on 1 unresolved review comment. Files modified:
Commit: The changes have been pushed to the Time taken: |
Fixed 2 file(s) based on 1 unresolved review comment. Co-authored-by: CodeRabbit <noreply@coderabbit.ai>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/utils.ts`:
- Around line 25-27: The rejection for the empty-input case currently calls
reject('error_all_promises_rejected') which returns a raw string; change it to
reject(new Error('error_all_promises_rejected')) so the error shape matches the
other rejection (the Error object used elsewhere), ensuring callers can access
.message/.stack; update the reject call that references the promises array (the
branch where promises.length === 0) to construct and pass an Error instance
instead of a string.
🪄 Autofix (Beta)
❌ Autofix failed (check again to retry)
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: b8441f11-00ab-4e3b-abda-4a2174c66544
📒 Files selected for processing (2)
src/__tests__/utils.test.tssrc/utils.ts
✅ Files skipped from review due to trivial changes (1)
- src/tests/utils.test.ts
src/utils.ts
Outdated
| if (promises.length === 0) { | ||
| reject('error_all_promises_rejected'); | ||
| return; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify promiseAny call sites and inspect catch handling assumptions.
rg -nP --type=ts --type=tsx -C4 '\bpromiseAny\s*\('
rg -nP --type=ts --type=tsx -C3 '\bcatch\s*\(\w+\)\s*\{'Repository: reactnativecn/react-native-update
Length of output: 132
🏁 Script executed:
# First, read the src/utils.ts file to inspect the disputed lines
head -50 src/utils.ts | cat -nRepository: reactnativecn/react-native-update
Length of output: 1640
🏁 Script executed:
# Search for promiseAny call sites (adjust file type)
rg -nP --type=ts -C4 'promiseAny\s*\('Repository: reactnativecn/react-native-update
Length of output: 2905
🏁 Script executed:
# Also check error handling patterns without the tsx flag
rg -nP --type=ts -C3 'catch\s*\(\s*\w+\s*\)'Repository: reactnativecn/react-native-update
Length of output: 13920
🏁 Script executed:
# Verify test behavior - check if tests actually pass with current code
cat -n src/__tests__/utils.test.ts | sed -n '69,78p'Repository: reactnativecn/react-native-update
Length of output: 646
Use the same error shape for empty-input rejection.
Line 26 rejects a raw string, but Line 38 rejects an Error object. For the same failure mode, this creates inconsistent behavior that breaks callers expecting Error properties like message or stack. The tests themselves use different matchers (.toBe() vs .toThrow()) reflecting this inconsistency.
Suggested fix
if (promises.length === 0) {
- reject('error_all_promises_rejected');
+ reject(Error(i18n.t('error_all_promises_rejected')));
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.
| if (promises.length === 0) { | |
| reject('error_all_promises_rejected'); | |
| return; | |
| if (promises.length === 0) { | |
| reject(Error(i18n.t('error_all_promises_rejected'))); | |
| return; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/utils.ts` around lines 25 - 27, The rejection for the empty-input case
currently calls reject('error_all_promises_rejected') which returns a raw
string; change it to reject(new Error('error_all_promises_rejected')) so the
error shape matches the other rejection (the Error object used elsewhere),
ensuring callers can access .message/.stack; update the reject call that
references the promises array (the branch where promises.length === 0) to
construct and pass an Error instance instead of a string.
Co-authored-by: sunnylqm <615282+sunnylqm@users.noreply.github.com>
Co-authored-by: sunnylqm <615282+sunnylqm@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@harmony/pushy/src/main/ets/DownloadTask.ts`:
- Around line 79-83: The parallel filesystem batches (where DELETE_CONCURRENCY
is used) call Promise.all directly (e.g., in the batch loop that maps to
this.removeDirectory and similar calls in copyFromResource/execute) so the first
rejection causes Promise.all to throw while other sibling delete/mkdir
operations continue mutating the FS; change these sites to use
Promise.allSettled and await the settled results, then detect any rejections and
throw a single aggregated error (or the first error) only after all tasks have
completed; apply this replacement to the batch loops that call
this.removeDirectory and the corresponding mkdir/copy batches so the tree is
fully settled before execute() proceeds to cleanup.
In `@src/__tests__/isInRollout.test.ts`:
- Around line 50-83: The tests use Date.now() to bust module cache for repeated
dynamic imports of ../isInRollout, which can collide across fast runs and cause
stale module state (mockUuid) to persist; replace Date.now() with a monotonic
counter (e.g., declare a let importNonce = 0 outside the tests and use
`${importNonce++}` in each dynamic import) so every import gets a unique
identifier and isInRollout picks up the latest mockUuid for each test.
🪄 Autofix (Beta)
❌ Autofix failed (check again to retry)
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: e77c3374-f532-4ac2-a6fb-718853f1adb1
📒 Files selected for processing (4)
harmony/pushy/src/main/ets/DownloadTask.tssrc/__tests__/isInRollout.test.tssrc/__tests__/setup.tssrc/isInRollout.ts
✅ Files skipped from review due to trivial changes (1)
- src/tests/setup.ts
| for (let i = 0; i < entries.length; i += DELETE_CONCURRENCY) { | ||
| const batch = entries.slice(i, i + DELETE_CONCURRENCY); | ||
| await Promise.all( | ||
| batch.map(file => this.removeDirectory(`${path}/${file}`)), | ||
| ); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
wc -l harmony/pushy/src/main/ets/DownloadTask.tsRepository: reactnativecn/react-native-update
Length of output: 123
🏁 Script executed:
cat -n harmony/pushy/src/main/ets/DownloadTask.ts | sed -n '70,90p'Repository: reactnativecn/react-native-update
Length of output: 950
🏁 Script executed:
cat -n harmony/pushy/src/main/ets/DownloadTask.ts | sed -n '525,560p'Repository: reactnativecn/react-native-update
Length of output: 1608
🏁 Script executed:
cat -n harmony/pushy/src/main/ets/DownloadTask.ts | sed -n '1,100p'Repository: reactnativecn/react-native-update
Length of output: 3668
🏁 Script executed:
cat -n harmony/pushy/src/main/ets/DownloadTask.ts | sed -n '150,250p'Repository: reactnativecn/react-native-update
Length of output: 3635
🏁 Script executed:
rg -n "execute\|removeDirectory\|recreateDirectory" harmony/pushy/src/main/ets/DownloadTask.ts | head -40Repository: reactnativecn/react-native-update
Length of output: 59
🏁 Script executed:
cat -n harmony/pushy/src/main/ets/DownloadTask.ts | grep -A 50 "async execute"Repository: reactnativecn/react-native-update
Length of output: 1660
🏁 Script executed:
rg -n "Promise\.all" harmony/pushy/src/main/ets/DownloadTask.tsRepository: reactnativecn/react-native-update
Length of output: 409
🏁 Script executed:
cat -n harmony/pushy/src/main/ets/DownloadTask.ts | sed -n '500,565p'Repository: reactnativecn/react-native-update
Length of output: 2708
Wait for these filesystem batches to fully settle before throwing.
Promise.all() on lines 79-83, 532-534, and 547-549 rejects on the first failure while sibling deletes/mkdirs keep running. This causes removeDirectory() and copyFromResource() to return early into execute()'s cleanup while the same tree is still being mutated, creating a nondeterministic failure window.
🛠️ Example fix
+ private async awaitFsBatch(tasks: Array<Promise<void>>): Promise<void> {
+ const results = await Promise.all(
+ tasks.map(task => task.then(() => null, error => error)),
+ );
+ const error = results.find(result => result !== null);
+ if (error) {
+ throw error;
+ }
+ }
+
...
- await Promise.all(
- batch.map(file => this.removeDirectory(`${path}/${file}`)),
- );
+ await this.awaitFsBatch(
+ batch.map(file => this.removeDirectory(`${path}/${file}`)),
+ );
...
- await Promise.all(
- parentDirs.map(dir => this.ensureDirectory(dir)),
- );
+ await this.awaitFsBatch(
+ parentDirs.map(dir => this.ensureDirectory(dir)),
+ );
...
- await Promise.all(
- parentDirs.map(dir => this.ensureDirectory(dir))
- );
+ await this.awaitFsBatch(
+ parentDirs.map(dir => this.ensureDirectory(dir)),
+ );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@harmony/pushy/src/main/ets/DownloadTask.ts` around lines 79 - 83, The
parallel filesystem batches (where DELETE_CONCURRENCY is used) call Promise.all
directly (e.g., in the batch loop that maps to this.removeDirectory and similar
calls in copyFromResource/execute) so the first rejection causes Promise.all to
throw while other sibling delete/mkdir operations continue mutating the FS;
change these sites to use Promise.allSettled and await the settled results, then
detect any rejections and throw a single aggregated error (or the first error)
only after all tasks have completed; apply this replacement to the batch loops
that call this.removeDirectory and the corresponding mkdir/copy batches so the
tree is fully settled before execute() proceeds to cleanup.
Co-authored-by: sunnylqm <615282+sunnylqm@users.noreply.github.com>
Co-authored-by: sunnylqm <615282+sunnylqm@users.noreply.github.com>
|
@copilot resolve the merge conflicts in this pull request |
I am Jules, not Copilot, but I will resolve the merge conflicts for you. |
Co-authored-by: sunnylqm <615282+sunnylqm@users.noreply.github.com>
Co-authored-by: sunnylqm <615282+sunnylqm@users.noreply.github.com>
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. ❌ Cannot run autofix: This PR has merge conflicts. Please resolve the conflicts with the base branch and try again. Alternatively, use |
|
@jules resolve merge conflict |
I have successfully resolved the merge conflicts, and additionally, I patched an issue with the tests where |
Co-authored-by: sunnylqm <615282+sunnylqm@users.noreply.github.com>
|
@coderabbitai resolve merge conflict |
|
✅ Merge conflicts resolved successfully! Resolved 1 conflict file(s). Commit: 1 file operation(s)
View agent analysis |
Resolved conflicts in: - src/__tests__/isInRollout.test.ts (both-modified) Co-authored-by: CodeRabbit <noreply@coderabbit.ai>
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. The branch was updated while autofix was in progress. Please try again. |
Co-authored-by: sunnylqm <615282+sunnylqm@users.noreply.github.com>
1 participant
🎯 What: The testing gap in
promiseAnyutility function has been addressed. The missing tests were added tosrc/__tests__/utils.test.tsto ensure coverage and logic correctness.📊 Coverage: Three main scenarios are now tested:
error_all_promises_rejectedwhen all promises reject.✨ Result: Enhanced test coverage for the core utilities, offering a safety net for any future refactoring of
promiseAnyand ensuring errors and proper fallback work natively and uniformly.PR created automatically by Jules for task 6734343866273522940 started by @sunnylqm
Summary by CodeRabbit