Introduce React Native OTA updates with zephyr-ota-plugin#293
Introduce React Native OTA updates with zephyr-ota-plugin#293HigorAlves wants to merge 14 commits intomainfrom
Conversation
e3f4848 to
518bdba
Compare
…pdates - Added core implementation of the `zephyr-ota-plugin` library. - Integrated API client for resolving remote metadata from Zephyr Cloud. - Included React hooks and provider for easy integration into React Native applications. - Supported periodic update checks, app reloads, and version tracking. - Implemented utilities for dependency parsing and platform detection. - Added comprehensive unit tests for key utilities and API integration. - Configured ESLint, Jest, and TypeScript for the library. - Provided documentation, examples, and default configurations.
518bdba to
b948e30
Compare
|
| Command | Status | Duration | Result |
|---|---|---|---|
nx run e2e-deployment:e2e-test -- --passWithNoT... |
❌ Failed | 14s | View ↗ |
nx affected -t build --skipRemoteCache |
✅ Succeeded | 3m 23s | View ↗ |
nx affected -t test |
✅ Succeeded | 3m 6s | View ↗ |
nx affected -t lint |
✅ Succeeded | 8s | View ↗ |
☁️ Nx Cloud last updated this comment at 2025-12-23 12:40:57 UTC
# Conflicts: # pnpm-lock.yaml
|
Some of them are nitpicks and you can just ignore and some are valid questions. Nice work btw |
|
@claude please review |
|
Claude finished @HigorAlves's task in 2m 1s —— View job Code Review Progress
Code Review for zephyr-ota-pluginGreat work on implementing this OTA plugin! The overall architecture is solid, with good separation of concerns between the API client, storage, service layer, and React integration. Here's my detailed feedback: Critical Issues1. Security: Platform.OS Type Safety (libs/zephyr-ota-plugin/src/lib/utils/platform.ts:12)The cast Recommendation: Add validation: export function getBuildTarget(): BuildTarget {
const os = Platform.OS;
if (os !== 'ios' && os !== 'android') {
throw new Error(`Unsupported platform: ${os}. OTA updates only work on iOS and Android.`);
}
return os;
}2. Potential API Bombardment (libs/zephyr-ota-plugin/src/lib/core/ZephyrOTAService.ts:171)As @arthurfiorette noted, with 100 remotes, this could hammer the API with 100 parallel requests. While your response about OTA needing to check all remotes is valid, consider implementing:
Recommendation: Use a promise queue with concurrency limit: async function resolveWithConcurrency<T>(
items: T[],
fn: (item: T) => Promise<any>,
concurrency: number
): Promise<any[]> {
const results: any[] = [];
for (let i = 0; i < items.length; i += concurrency) {
const batch = items.slice(i, i + concurrency);
results.push(...await Promise.all(batch.map(fn)));
}
return results;
}3. Missing Error Handling in Cache Clearing (libs/zephyr-ota-plugin/src/lib/core/ZephyrOTAService.ts:254-273)The cache clearing logic silently catches errors but continues. If cache clearing fails for some remotes, the user might not get the updates after reload. Recommendation: Collect failures and warn the user, or retry the failed operations. High Priority Issues4. Unused Function - createZephyrOTAService (libs/zephyr-ota-plugin/src/lib/core/ZephyrOTAService.ts:385)As @arthurfiorette mentioned, this function is redundant. Users can just call Recommendation: Remove this function or make it do something useful (like validation, setup, etc.). 5. Unused Factory Function - OTAStorage (libs/zephyr-ota-plugin/src/lib/core/storage.ts)Similarly, there's no need for a factory function if it's just calling the constructor. Recommendation: Remove the factory function if it exists. 6. Inconsistent Logging Structure (libs/zephyr-ota-plugin/src/lib/utils/logger.ts:40)As @arthurfiorette noted, the logging here uses a custom implementation while other Zephyr plugins use the Recommendation: Refactor to use the shared logging structure from zephyr-agent for consistency:
7. URL Construction Readability (libs/zephyr-ota-plugin/src/lib/core/api-client.ts:49-53)As @arthurfiorette mentioned, using URL object throughout would be more readable. You already use it here, which is good! However, line 116 uses string concatenation instead: const url = `${defaultUrl}/__get_version_info__`;Recommendation: Use URL object consistently for all URL construction. 8. useZephyrOTA Hook Naming (libs/zephyr-ota-plugin/src/lib/react/ZephyrOTAProvider.tsx:375)As @arthurfiorette asked, why not call it Counterpoint: Verdict: I'd keep the current name as it's more user-friendly. 9. Large File - Extract Types (libs/zephyr-ota-plugin/src/lib/react/ZephyrOTAProvider.tsx)@arthurfiorette suggested extracting types to a separate file. You already did this in 10. Questionable useMemo (libs/zephyr-ota-plugin/src/lib/react/ZephyrOTAProvider.tsx:126-140)@arthurfiorette noted the useMemo might not have benefits. Looking at the dependencies Issue: Recommendation: Advise users to memoize the config prop, or use 11. Package.json Exports Field (libs/zephyr-ota-plugin/package.json:13)As @arthurfiorette mentioned, RN bundlers sometimes have issues with exports field. Consider adding: "exports": {
".": {
"types": "./dist/index.d.ts",
"require": "./dist/index.js",
"default": "./dist/index.js"
},
"./*": "./*",
"./package.json": "./package.json"
}12. API Base URL Configuration (libs/zephyr-ota-plugin/src/lib/types/config.ts:37)@arthurfiorette suggested using env vars as default values like edge-contract does. This would be helpful for consistency. Recommendation: Import from edge-contract or add similar env var support: import { ZE_API_ENDPOINT } from 'zephyr-edge-contract';
export const DEFAULT_OTA_CONFIG: Required<ZephyrOTAConfig> = {
authToken: process.env.ZE_AUTH_TOKEN || '',
apiBaseUrl: ZE_API_ENDPOINT(),
// ...
};Note: Ensure this works in React Native context (env vars might need special handling). Medium Priority Issues13. Parallel Updates in Service (libs/zephyr-ota-plugin/src/lib/core/ZephyrOTAService.ts:120)@arthurfiorette asked if this can work inside Promise.all. Yes, it can! The 14. Missing Jsdoc for Public APIsWhile the code has some inline comments, public APIs could benefit from more comprehensive JSDoc comments with examples. 15. Rate Limiting Logic (libs/zephyr-ota-plugin/src/lib/core/ZephyrOTAService.ts:134-138)The rate limiting only checks time since last check, but doesn't account for concurrent checks. If multiple components call Recommendation: Add a lock mechanism or return the in-flight promise. 16. DevSettings Availability (libs/zephyr-ota-plugin/src/lib/core/ZephyrOTAService.ts:299-306)DevSettings might not be available in production builds. The code handles this gracefully, but consider documenting this limitation and providing an alternative reload mechanism. Low Priority / Nitpicks17. Unused onReloadRequested Prop (libs/zephyr-ota-plugin/src/lib/react/types.ts:98)The Recommendation: Either implement it or remove it. 18. Test CoverageGood that you included tests! However, consider adding:
19. TypeScript StrictnessConsider enabling stricter TypeScript options:
20. Storage Error HandlingAll storage methods catch errors and log warnings, but don't propagate them. This could hide issues. Consider:
Positive Highlights
SummaryThis is a solid implementation with good architectural decisions. The main concerns are:
I'd recommend addressing the critical and high-priority issues before merging. The rest can be handled in follow-up PRs. |
ryok90
left a comment
There was a problem hiding this comment.
As a ReactProvider this makes sense since it allows the user to input behavior into the OTA responses.
Since this is tightly related to ReactNative and Metro, it would be interesting to see this into metro plugin or repack plugin (if it supports and with a reexport in metro) instead of a separate NPM package.
One thing I noticed on metro plugin is the verbose declaration of options in the withZephyr when we do have all of that in zephyr-engine being derived from users config in MF plugin and other configs such as, target, remotes, name, and all of that. We don't need such parameters since we are already grabbing that info from configs. OTAProvider also won't need those configs if we have access to zephyr-manifest.json.
All of these changes can be done after this PR is merged but for now what is really important is that we reduce user's boilerplate into making OTA work, try and move this Provider into metro plugin, we can document and work on other changes on the next iteration.
…alidation, and imports - Added `mapWithConcurrency` helper to limit concurrent API requests. - Introduced platform validation to restrict usage to iOS and Android. - Simplified imports and code structure by removing unused exports and redundant functions. - Updated `apiBaseUrl` to dynamically resolve from `zephyr-edge-contract`. - Enhanced package export definitions in `package.json`.
|
@claude please review |
|
Claude finished @HigorAlves's task in 1m 45s —— View job Code Review Progress
Review of zephyr-ota-plugin PRI've thoroughly reviewed this PR and the previous feedback from @arthurfiorette and the earlier Claude review. Here's my assessment: SummaryThis is a well-architected implementation of OTA updates for React Native with Module Federation. The code demonstrates good separation of concerns, thoughtful API design, and solid test coverage. Most of the critical issues raised in previous reviews have been addressed. What's Been Fixed ✅
Remaining Issues & RecommendationsHigh Priority1. Logging Inconsistency As @arthurfiorette noted, this plugin uses a custom logger (libs/zephyr-ota-plugin/src/lib/utils/logger.ts) while other Zephyr packages use the Impact: Inconsistent debugging experience across the Zephyr ecosystem. Users familiar with enabling debug output in other Zephyr tools won't have the same experience here. Recommendation:
2. Unused Factory Functions The codebase no longer exports 3. URL Construction Inconsistency Good use of URL object in api-client.ts:49-53, but line 116 uses string concatenation: const url = `${defaultUrl}/__get_version_info__`;Minor: Could use URL object for consistency, but string concatenation is fine here since we're just appending a path. Medium Priority4. Storage Error Handling Strategy The storage module (libs/zephyr-ota-plugin/src/lib/core/storage.ts) catches all errors and logs warnings without propagating. This could hide critical issues like:
Recommendation: Consider providing an error callback or event for storage failures so users can implement their own fallback/monitoring strategies. 5. useZephyrOTA Hook Name @arthurfiorette questioned why not call it 6. useMemo in Provider @arthurfiorette questioned the useMemo at ZephyrOTAProvider.tsx:126-140. The memo is appropriate since:
Recommendation: Document in README that users should memoize/stabilize the config object if passing as a prop to avoid unnecessary service recreations. 7. Cache Clearing Error Handling In ZephyrOTAService.ts:276-302, cache clearing catches errors but continues. If cache clearing fails for multiple remotes, the user might reload without getting the updates. Suggestion: Consider collecting failures and:
Low Priority / Nitpicks8. DevSettings Availability The code gracefully handles DevSettings not being available (ZephyrOTAService.ts:327-334). This is well done! Consider documenting this limitation in the README for production builds. 9. Missing onReloadRequested Implementation The types define 10. Test Coverage Gaps Good unit test coverage for utilities! Consider adding:
Can be follow-up work. Architecture Strengths 🌟
Addressing @ryok90's Architectural Comments@ryok90 makes an excellent point about potentially moving this into metro/repack plugin to reduce boilerplate. This is a great idea for future iteration but shouldn't block this PR: Pros of current approach:
Future improvement:
Final VerdictRecommendation: Approve with minor follow-ups This is high-quality code that's ready to merge. The critical issues (platform validation, concurrency) have been addressed. The remaining items are:
The implementation demonstrates solid engineering practices and will serve as a strong foundation for OTA updates in the Zephyr ecosystem. Testing Notes:
|
# Conflicts: # pnpm-lock.yaml
…ug logging - Introduced `StorageError` and `StorageErrorHandler` types for robust error reporting in storage operations. - Added `reportError` utility for centralized error handling across storage methods. - Integrated `debug` package for improved, scoped logging with pre-defined namespaces. - Updated async storage operations with error reporting callbacks. - Enhanced `logger` utility to support scoped loggers and dynamic debug enabling/disabling. - Updated dependencies to include `debug` and `@types/debug`.
…tch resolve API - Introduced `BatchResolveRequest` and `BatchResolveResponse` types. - Implemented `resolveRemotesBatch` method in the API client for handling batch resolve requests. - Replaced `mapWithConcurrency` logic with batch resolve in version tracking and remote resolution. - Improved error handling with fallback to individual requests when batch resolve fails.
…ing utilities - Deleted `detect-remotes` and `parse-dependencies` modules along with their tests. - Removed unused exports and related types from the library. - Updated `ZephyrOTAProvider` to adopt the new manifest-based update detection flow. - Streamlined the library API to align with the updated architecture.

What's added in this PR?
This PR introduces the initial implementation of the
zephyr-ota-pluginlibrary for managing OTA (Over-The-Air) updates in React Native applications. Key features include:Screenshots
What's the issues or discussion related to this PR ?
This library was created to streamline the integration of OTA updates into React Native apps, simplifying version management and minimizing user disruption during updates.
What are the steps to test this PR?
Documentation update for this PR (if applicable)?
The library's setup guide and usage examples are included in the documentation. Please include this PR into zephyr-documentation.
(Optional) What's left to be done for this PR?
Fine-tuning and feedback-based iterations may be required after wider usage.
(Optional) What's the potential risk and how to mitigate it?
Potential configuration issues or platform-specific edge cases. Mitigation includes thorough testing across platforms and providing fallback mechanisms.
(Required) Pre-PR/Merge checklist