feat: 进度百分比 + 下载重试 + 降级链测试 + Cresc 测试 + globals.d.ts#583
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (5)
📝 WalkthroughWalkthroughThe PR adds computed download progress, retry-aware update downloads, fallback-chain tests, and an ambient boolean declaration for ChangesClient download flow and support types
Sequence Diagram(s)sequenceDiagram
participant downloadUpdate
participant downloadPatchFromPpk
participant downloadPatchFromPackage
participant downloadFullUpdate
loop attempt 0..maxRetries
downloadUpdate->>downloadPatchFromPpk: try diff download
alt diff succeeds
downloadPatchFromPpk-->>downloadUpdate: success
else diff fails
downloadUpdate->>downloadPatchFromPackage: try pdiff download
alt pdiff succeeds
downloadPatchFromPackage-->>downloadUpdate: success
else pdiff fails
downloadUpdate->>downloadFullUpdate: try full update
downloadFullUpdate-->>downloadUpdate: success or error
end
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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
src/__tests__/client.test.tsOops! Something went wrong! :( ESLint: 8.57.1 Error: .eslintrc.js » src/__tests__/utils.test.tsOops! Something went wrong! :( ESLint: 8.57.1 Error: .eslintrc.js » src/client.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 |
sunnylqm
force-pushed
the
feat/progress-retry-strict-tests
branch
from
398f57f to
ebd47be
Compare
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
![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 (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
sunnylqm
force-pushed
the
feat/progress-retry-strict-tests
branch
from
ebd47be to
7df1727
Compare
|
All 3 CodeRabbit findings addressed:
Lint ✅ | 66 tests pass ✅ |
1 participant
变更概览
新功能
测试补充
开发者体验
改动文件
验证
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Documentation/Types
__DEV__global type declaration for React Native development mode.