feat: 进度百分比 + 下载重试 + 降级链测试 + Cresc 测试 + globals.d.ts#583
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:
📝 WalkthroughWalkthroughThe PR adds computed download progress, retry-aware update downloads, fallback-chain tests, an ambient boolean declaration for ChangesClient download flow and support types
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 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 |
398f57f to
ebd47be
Compare
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/__tests__/client.test.ts (1)
373-406: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
maxRetriesdestructured here is never used.
setupDownloadMocksacceptsmaxRetriesbut never forwards it; the actual retry count is controlled solely bynew Pushy({ ..., maxRetries })in each test. The parameter (passed at Lines 472, 495, 527) is dead and misleading—drop it or wire it into the constructed client to avoid implying it has an effect.🤖 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/__tests__/client.test.ts` around lines 373 - 406, The setupDownloadMocks helper in client.test.ts destructures maxRetries but never uses it, so the parameter is misleading and dead. Remove maxRetries from the helper signature and its destructuring in setupDownloadMocks, or alternatively thread it through to the Pushy client setup if the tests are meant to control retry behavior; use the setupDownloadMocks and Pushy test setup as the places to update.src/client.ts (1)
528-607: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winRetries fire immediately with no backoff.
On persistent failures the loop re-runs the full diff→pdiff→full chain back-to-back (up to
maxRetries + 1times) with no delay. For server/network-induced failures this hammers the backend instantly and is unlikely to recover. Consider adding a short (ideally exponential) backoff between attempts.♻️ Example backoff between attempts
for (let attempt = 0; attempt <= maxRetries; attempt++) { if (attempt > 0) { log(`retry attempt ${attempt}/${maxRetries}`); + await new Promise(r => setTimeout(r, Math.min(1000 * 2 ** (attempt - 1), 10000))); errorMessages.length = 0; lastError = undefined; succeeded = ''; }🤖 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 528 - 607, The retry loop in client.ts for the update download flow retries immediately with no delay, which can hammer the backend on repeated failures. Add a short backoff between attempts inside the for-loop around the diff/pdiff/full download chain, ideally exponential and only between failed attempts, while keeping the existing success break behavior and preserving the current logging/error handling in the retry path.
🤖 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 `@src/type.ts`:
- Around line 36-37: The `progress` field in `RCTPushyDownloadProgress` is being
treated as always present, but that is not true across all emission paths.
Update the `type.ts` definition and the related progress flow in
`downloadAndInstallApk`/`onDownloadProgress` so the runtime contract matches
reality: either make `progress` optional in the shared type or ensure every
producer, including the APK download path and any native
`RCTPushyDownloadProgress` payloads, computes and sets it consistently like
`wrapProgress` does in `downloadUpdate`.
---
Nitpick comments:
In `@src/__tests__/client.test.ts`:
- Around line 373-406: The setupDownloadMocks helper in client.test.ts
destructures maxRetries but never uses it, so the parameter is misleading and
dead. Remove maxRetries from the helper signature and its destructuring in
setupDownloadMocks, or alternatively thread it through to the Pushy client setup
if the tests are meant to control retry behavior; use the setupDownloadMocks and
Pushy test setup as the places to update.
In `@src/client.ts`:
- Around line 528-607: The retry loop in client.ts for the update download flow
retries immediately with no delay, which can hammer the backend on repeated
failures. Add a short backoff between attempts inside the for-loop around the
diff/pdiff/full download chain, ideally exponential and only between failed
attempts, while keeping the existing success break behavior and preserving the
current logging/error handling in the retry path.
🪄 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: eb1e47da-d9d4-4940-8b63-bdbabb7aedd9
📒 Files selected for processing (6)
src/__tests__/client.test.tssrc/__tests__/utils.test.tssrc/client.tssrc/globals.d.tssrc/type.tssrc/utils.ts
…c tests, globals.d.ts Features: - Add progress percentage (0-100) to ProgressData via computeProgress utility - Add maxRetries option for automatic download retry on failure - Download progress callbacks now include computed progress field Tests: - Add comprehensive downloadUpdate fallback chain tests (diff→pdiff→full) - Add retry mechanism tests (success on retry, exhaust retries) - Add Cresc class tests (endpoints, locale, instanceof, custom server) - Add computeProgress unit tests Developer Experience: - Add src/globals.d.ts for __DEV__ type declaration - Type-hint progressData callbacks with ProgressData type
ebd47be to
7df1727
Compare
|
All 3 CodeRabbit findings addressed:
Lint ✅ | 66 tests pass ✅ |
…b/index.js CLI refactored in ebd5a45 moved command dispatch from index.ts to bin.ts, but e2e script still pointed at lib/index.js which only re-exports. pushy bundle exited with code 0 without producing .ppk files. Read the bin field from the CLI's package.json for forward-compatibility.
The prepare step was exiting with code 0 but never generating diffs or writing manifest.json. Added step-by-step logging to diagnose: - 'Generating ppk diff...' / 'Ppk diff generated.' - 'Generating package diff...' / 'Package diff generated.' - 'Manifest written to ...' - 'prepare-local-update-artifacts completed successfully.' Inlined diff calls (removed generatePpkDiff/generateAndroidPackageDiff wrapper functions) for more direct error propagation.
The in-process require of lib/diff.js + calling diffCommands.hdiff() was hanging because yauzl's async entry callbacks don't keep the Node.js event loop alive properly when called from a script that has no other active handles. Switched to running diff commands as CLI subprocesses via runPushy(), matching the pattern already used for bundleTo(). Each diff command runs in its own node process with a proper event loop. Also removed the DiffCommandRunner type and lib/diff.js require since they're no longer needed.
The CLI's loadModule('node-hdiffpatch') searches from process.cwd(),
which is the project root when running as a subprocess. Install the
module to the project's node_modules instead of the CLI's so it can
be found.
This ensures pushy hdiff/hdiffFromApk commands can resolve the native
diff module when run as CLI subprocesses.
macOS 26 runner requires explicit tap trust for Homebrew security. Fixes: Refusing to load formula wix/brew/applesimutils from untrusted tap
The local e2e server appears to start (health check passes) but returns 404 for artifact files. Added: - stdout/stderr capture from bun server subprocess - spawn error handler - HTTP response status logging in waitForRequestReady
Root cause was CLI's loadModule not checking NODE_PATH dirs. Fixed in react-native-update-cli@151ea0f.
- prepare script: verify ppk.patch and apk.patch exist after generation - server: log 404 responses with resolved path and existence check - server: add /debug/artifacts endpoint listing all files - globalSetup: capture server stdout/stderr to .server.log - globalSetup: list artifacts dir and query debug endpoint before warmServer
Root cause: loadModule('node-hdiffpatch') in CLI's lib/diff.js uses
require.resolve with paths=['.', ...NODE_PATH]. The '.' resolves to
cliRoot/lib/, and NODE_PATH only had cliRoot/node_modules/. But
node-hdiffpatch was installed in projectRoot/node_modules/, which was
not in the search path. The module silently failed to load, the diff
command exited without error (or with a swallowed error), and the
output file was never created.
Fix: add projectRoot/node_modules to NODE_PATH so the CLI can find
node-hdiffpatch installed in the project's node_modules.
pushy hdiff exits 0 but doesn't create the output file. Capture and log all stdout/stderr from the CLI subprocess to understand why.
…rectly pushy hdiff exits 0 in CI but doesn't create output files. The root cause is that the CLI's bin.ts flow (loadSession, argument parsing) interferes with the hdiff command somehow - the handler either never runs or silently fails. Fix: use a standalone wrapper script (run-hdiff-wrapper.js) that requires the CLI's lib/diff module directly and calls the hdiff/hdiffFromApk handlers with the correct arguments. This bypasses the bin.ts flow entirely while reusing the same diff logic. Also adds projectRoot/node_modules to NODE_PATH so the CLI's loadModule can find node-hdiffpatch installed in the project.
- Use createRequire from node:module for reliable module loading - Add extensive logging to diagnose wrapper execution in CI - Validate input files exist before calling diff handlers - Verify output file creation after diff completes
__dirname in TS-compiled scripts points to .e2e-artifacts/.ts-build/scripts/, but run-hdiff-wrapper.js lives in the source scripts/ directory and is not copied to the build output. Use projectRoot + 'scripts' to resolve correctly.
The CLI's loadModule uses require.resolve with paths that only include the CLI's own lib/ and node_modules/. It cannot find node-hdiffpatch installed in the project's node_modules. Fix: pre-load node-hdiffpatch from project root in the wrapper script and pass it as options.customHdiffModule to the diff handler. This bypasses the loadModule resolution entirely.
变更概览
新功能
测试补充
开发者体验
改动文件
验证
Summary by CodeRabbit
__DEV__flag.