chore: upgrade to new RN SDK version #88
Conversation
|
Warning Rate limit exceeded@rodrigopavezi has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 15 minutes and 51 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughThis pull request introduces several updates to the project's dependency management and architecture. The changes primarily focus on updating package versions, adding new Lit Protocol-related dependencies, and modifying the context and provider setup for decryption functionality. The modifications span across multiple files, including Changes
Sequence DiagramsequenceDiagram
participant App as Application
participant Context as AppContext
participant LitProvider as Dynamic Lit Provider
participant LitClient as Lit Node Client
participant CipherProvider as Lit Protocol Cipher Provider
App->>Context: Initialize Context
Context->>LitProvider: Dynamically Import
LitProvider->>LitClient: Initialize Client
LitClient-->>LitProvider: Client Ready
LitProvider->>CipherProvider: Create Provider
CipherProvider-->>LitProvider: Provider Ready
LitProvider->>Context: Trigger onProviderReady Callback
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (4)
utils/litProvider.tsx (2)
28-31: Enhance error handling and loggingThe error handling could be more informative for debugging purposes. Consider adding error details and implementing proper error tracking.
} catch (error) { - console.error('Failed to initialize Cipher Provider:', error); + console.error('Failed to initialize Cipher Provider:', { + error, + network: process.env.NEXT_PUBLIC_LIT_PROTOCOL_NETWORK, + baseURL: process.env.NEXT_PUBLIC_REQUEST_NODE + }); onProviderReady(undefined); }
15-16: Document environment variable defaultsThe code uses environment variables with fallback values. These should be documented for proper configuration management.
Consider adding a comment block explaining the environment variables and their defaults:
/** * Environment Variables: * - NEXT_PUBLIC_LIT_PROTOCOL_NETWORK: The Lit Protocol network to connect to (defaults to 'datil') * - NEXT_PUBLIC_REQUEST_NODE: Base URL for the Request Network node (defaults to 'https://gnosis.gateway.request.network/') */Also applies to: 20-23
utils/context.tsx (2)
62-65: Type safety improvement needed.Consider using a more specific type for the
walletparameter instead ofunknownto ensure type safety.-const initializeRequestNetwork = (wallet: unknown) => { +const initializeRequestNetwork = (wallet: WalletClient) => {
Line range hint
71-156: Refactor suggestion: Simplify chain-specific URL configuration.The nested ternary operators make the code hard to maintain. Consider refactoring to a more maintainable structure.
+const CHAIN_SUBGRAPH_URLS: Record<string, string> = { + 'arbitrum-one': process.env.NEXT_PUBLIC_PAYMENTS_SUBGRAPH_URL_ARBITRUM_ONE || + 'https://subgraph.satsuma-prod.com/e2e4905ab7c8/request-network--434873/request-payments-arbitrum-one/api', + 'avalanche': process.env.NEXT_PUBLIC_PAYMENTS_SUBGRAPH_URL_AVALANCHE || + 'https://subgraph.satsuma-prod.com/e2e4905ab7c8/request-network--434873/request-payments-avalanche/api', + // ... other chains +}; + const paymentsSubgraphUrl = CHAIN_SUBGRAPH_URLS[chain];
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (5)
package.json(2 hunks)pages/index.tsx(1 hunks)types.d.ts(0 hunks)utils/context.tsx(4 hunks)utils/litProvider.tsx(1 hunks)
💤 Files with no reviewable changes (1)
- types.d.ts
🔇 Additional comments (9)
pages/index.tsx (1)
15-15: LGTM! Clean interface simplification
The removal of decryption-related props from context destructuring aligns well with the upgraded lit protocol provider implementation. The change reduces component complexity while maintaining necessary functionality.
package.json (3)
12-13: LGTM! Well-pinned lit-protocol dependencies
The lit-protocol dependencies are correctly pinned to specific versions, which helps ensure consistent behavior across environments.
Also applies to: 38-38
17-22: LGTM! Properly versioned @RequestNetwork packages
The @RequestNetwork package updates are appropriately pinned and follow a consistent versioning pattern.
14-16: Consider pinning remaining dependencies
Several dependencies still use the caret (^) version range, which could lead to unexpected updates. Consider pinning these versions for better reproducibility.
Also applies to: 23-35
utils/context.tsx (5)
64-65: LGTM: Web3SignatureProvider initialization.
The Web3SignatureProvider initialization looks correct with proper error handling.
227-233: LGTM: Clean provider initialization pattern.
The conditional rendering and callback-based provider initialization is well implemented. The dependency chain (walletClient, isConnected, address, chainId) ensures proper initialization order.
13-18: LGTM: Dynamic import setup is appropriate.
The dynamic import with ssr: false is the correct approach for loading the Lit Protocol provider, as it requires browser APIs.
Let's verify the LitProvider implementation:
✅ Verification successful
Dynamic import setup is correctly implemented
The verification confirms that LitProvider is properly exported as a function component from utils/litProvider.tsx, making it compatible with the dynamic import setup in context.tsx. The implementation follows React best practices for client-side only components.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if LitProvider is properly exported from litProvider
rg -t typescript "export.*LitProvider" ./utils/litProvider.tsx
Length of output: 99
Script:
#!/bin/bash
# Let's try different approaches to verify the LitProvider implementation
# First, check if the file exists
fd "litProvider"
# Then check for exports without specifying file type
rg "export.*LitProvider"
# Also check for any React component definition patterns
ast-grep --pattern 'const LitProvider = $_'
Length of output: 202
Line range hint 241-247: LGTM: Context hook follows best practices.
The useAppContext hook implementation is correct with proper error handling.
Let's verify the context usage across the codebase:
✅ Verification successful
Context hook implementation and usage verified successfully
The verification confirms that:
- The context hook is properly implemented in
utils/context.tsxwithcreateContextand error handling - The hook is correctly used across multiple components:
pages/index.tsxpages/create-invoice.tsxcomponents/common/Navbar.tsx
- All usages follow the pattern of destructuring specific values from the context
- No instances of improper usage outside the provider were found
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for proper context usage
rg "useAppContext|Context.Provider" -t typescript
Length of output: 86
Script:
#!/bin/bash
# Check for proper context usage with common TypeScript/JavaScript extensions
rg "useAppContext|Context.Provider" -g "*.{ts,tsx,js,jsx}"
# Also search for any Context-related imports and definitions
rg "createContext|useContext" -g "*.{ts,tsx,js,jsx}"
Length of output: 1000
209-210: Verify potential race condition in useEffect dependencies.
The useEffect dependency on isDecryptionEnabled might cause unnecessary re-initialization. Consider splitting the effects or using a ref to track initial state.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
utils/litProvider.tsx (2)
17-17: ValidateNEXT_PUBLIC_LIT_PROTOCOL_NETWORKbefore castingCasting
process.env.NEXT_PUBLIC_LIT_PROTOCOL_NETWORKtoLIT_NETWORKS_KEYSwithout validation may result in invalid values if the environment variable is incorrectly set. Consider validating the environment variable to ensure it matches one of the allowedLIT_NETWORKS_KEYS.Apply this diff to add validation:
const litNetworkEnv = process.env.NEXT_PUBLIC_LIT_PROTOCOL_NETWORK; - litNetwork: process.env.NEXT_PUBLIC_LIT_PROTOCOL_NETWORK as LIT_NETWORKS_KEYS || LIT_NETWORK.Datil, + litNetwork: Object.values(LIT_NETWORK).includes(litNetworkEnv as LIT_NETWORKS_KEYS) + ? (litNetworkEnv as LIT_NETWORKS_KEYS) + : LIT_NETWORK.Datil,
32-33: Enhance error logging for easier debuggingConsider providing more detailed error information when initialization fails. This could include additional context or the specific parameters used, which can be helpful for troubleshooting.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
utils/litProvider.tsx(1 hunks)
🔇 Additional comments (2)
utils/litProvider.tsx (2)
23-24: Confirm the default baseURL
The default baseURL is set to 'https://gnosis.gateway.request.network/'. Ensure this is the intended default URL for the Request Network node. If not, consider updating it or externalizing it to a configuration file or environment variable.
40-42: 🛠️ Refactor suggestion
Add cleanup for litNodeClient to prevent memory leaks
The cleanup function in useEffect should disconnect the litNodeClient to prevent memory leaks and ensure proper resource management. Currently, only litProviderRef.current?.disconnectClient?.(); is called. Consider storing a reference to litNodeClient and disconnecting it on unmount.
Apply this diff to include cleanup for litNodeClient:
import { useEffect, useRef } from 'react';
+ const litNodeClientRef = useRef<LitNodeClient | null>(null);
const initializeLit = async () => {
try {
const litNodeClient = new LitNodeClient({
litNetwork: process.env.NEXT_PUBLIC_LIT_PROTOCOL_NETWORK as LIT_NETWORKS_KEYS || LIT_NETWORK.Datil,
});
+ litNodeClientRef.current = litNodeClient;
const litCipherProvider = new LitProtocolCipherProvider(
litNodeClient,
{
baseURL:
process.env.NEXT_PUBLIC_REQUEST_NODE ||
'https://gnosis.gateway.request.network/',
headers: {}
},
);
- litCipherProvider.initializeClient();
+ await litCipherProvider.initializeClient();
litProviderRef.current = litCipherProvider;
onProviderReady(litCipherProvider);
} catch (error) {
console.error('Failed to initialize Cipher Provider:', error);
onProviderReady(undefined);
}
};
// Initialize on mount
useEffect(() => {
initializeLit();
return () => {
+ litNodeClientRef.current?.disconnect?.();
litProviderRef.current?.disconnectClient?.();
};
}, []);
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
.github/workflows/build.yml (2)
38-40: Consider optimizing npm installation flagsWhile the changes improve reliability, there are some concerns:
- Forcing cache clean on every run might increase build times unnecessarily
--maxsockets 1could significantly slow down package downloadsConsider this alternative approach:
- npm cache clean --force - npm ci --prefer-offline --no-audit --maxsockets 1 + npm ci --prefer-offline --no-auditOnly clean cache when there are actual cache-related issues.
43-55: Well-configured build optimization settingsThe build configuration changes are well thought out:
- Clearing
.nextprevents stale cache issues- Memory settings are appropriate for large Next.js builds
- Environment variables are properly configured for production builds
Consider monitoring the memory usage during builds to fine-tune the
max-old-space-sizevalue. The current 8GB setting might be more than needed, or in some cases, insufficient depending on the project size.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (4)
.github/workflows/build.yml(1 hunks)next.config.js(1 hunks)package.json(2 hunks)utils/litProvider.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- utils/litProvider.tsx
- package.json
🔇 Additional comments (4)
next.config.js (2)
1-8: LGTM! Well-structured configuration
The configuration is well-structured and properly typed with TypeScript. The module exports follow the standard Next.js configuration pattern.
3-5: Caution: Using experimental Next.js features
The optimizePackageImports is an experimental Next.js feature that may change or be removed in future releases. While it can improve performance by automatically handling package imports more efficiently, be prepared to update this configuration in future Next.js upgrades.
Let's verify the package versions and their compatibility:
✅ Verification successful
The previous output confirmed the packages are installed, but let's verify their actual usage and Next.js version compatibility. I've corrected the ripgrep file type flags to properly include TypeScript and TSX files.
Let's verify the actual usage of these packages and confirm if this experimental feature is officially supported in Next.js 15.1.0. I've corrected the file pattern matching approach for ripgrep.
Experimental feature usage is appropriate and packages are actively used
The configuration is valid as:
- Both packages are properly installed (
@lit-protocol/[email protected]and@requestnetwork/[email protected]) - The packages are actively used in the codebase (found in
utils/litProvider.tsx) - The
optimizePackageImportsfeature is officially supported in Next.js 15.1.0 as an experimental option - This optimization can help improve the application's performance by better handling these specific package imports
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify package versions in package.json and their compatibility
# with the optimizePackageImports feature
# Check if packages are listed in dependencies
jq -r '.dependencies | select(.["@lit-protocol/lit-node-client"] != null or .["@requestnetwork/lit-protocol-cipher"] != null)' package.json
# Search for any import statements of these packages to ensure they're actually used
rg -l '@lit-protocol/lit-node-client|@requestnetwork/lit-protocol-cipher' --type js --type tsx --type ts
Length of output: 1115
Script:
#!/bin/bash
# Search for imports with correct file type flags
rg -l '@lit-protocol/lit-node-client|@requestnetwork/lit-protocol-cipher' -t js -t typescript -t tsx
# Check if Next.js version supports this experimental feature
jq -r '.dependencies.next' package.json
Length of output: 185
Script:
#!/bin/bash
# Search for imports using individual file extensions
rg -l '@lit-protocol/lit-node-client|@requestnetwork/lit-protocol-cipher' -g '*.{ts,tsx,js,jsx}'
# Check if this experimental feature is documented in the Next.js version's release notes
curl -s "https://raw.githubusercontent.com/vercel/next.js/v15.1.0/packages/next/src/server/config-schema.ts" | grep -A 5 "optimizePackageImports"
Length of output: 606
.github/workflows/build.yml (2)
30-36: Great addition of system diagnostics!
Adding system information logging is excellent for CI/CD observability and will help debug potential resource-related build failures.
27-27: Verify dependency compatibility with Node.js 20
The upgrade to Node.js 20 is a good move as it's the latest LTS version. However, we should verify compatibility with all dependencies, especially the new RN SDK version.
✅ Verification successful
Node.js 20 upgrade is compatible with all dependencies
All dependencies are compatible with Node.js 20:
- Most critical dependency
next.jsexplicitly supports Node.js 20 (>= 20.0.0) - Request Network packages require Node.js
>=18.0.0 - Other dependencies have lower version requirements (
>=14.0.0,>=12.4,>=6, etc.) - No dependency has an upper bound that would conflict with Node.js 20
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check package.json for potential Node.js version requirements or conflicts
# Check for explicit Node.js version requirements in package.json
echo "Checking package.json for Node.js version requirements..."
jq '.engines?.node' package.json
# Check dependencies that might have Node.js version constraints
echo "Checking dependencies for Node.js version constraints..."
for pkg in $(jq -r '.dependencies | keys[]' package.json); do
npm info $pkg engines.node
done
Length of output: 2924
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
.github/workflows/build.yml (3)
37-39: Consider adding memory monitoring during buildThe addition of
ulimit -c unlimitedsuggests there might be memory-related issues during the build. While this allows for core dumps, it would be better to monitor memory usage to prevent issues.Consider adding memory monitoring before and after the build:
run: | + free -h ulimit -c unlimited npm run build + free -h🧰 Tools
🪛 yamllint (1.35.1)
[error] 37-37: trailing spaces
(trailing-spaces)
[error] 38-38: trailing spaces
(trailing-spaces)
37-38: Fix trailing whitespaceThere are trailing spaces at the end of these lines.
Apply this diff to fix the formatting:
- run: | - ulimit -c unlimited + run: | + ulimit -c unlimited🧰 Tools
🪛 yamllint (1.35.1)
[error] 37-37: trailing spaces
(trailing-spaces)
[error] 38-38: trailing spaces
(trailing-spaces)
20-22: Consider adding health check for container memory limitsWhile setting a 4GB memory limit is good, it would be beneficial to verify this limit is sufficient throughout the build process.
Consider adding a memory monitoring step or implementing a pre-build check to ensure the container has the required resources. This could help catch potential issues early in the build process.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/build.yml(1 hunks)
🧰 Additional context used
🪛 yamllint (1.35.1)
.github/workflows/build.yml
[error] 37-37: trailing spaces
(trailing-spaces)
[error] 38-38: trailing spaces
(trailing-spaces)
🔇 Additional comments (1)
.github/workflows/build.yml (1)
42-45: LGTM! Optimized build configuration
The added environment variables are well-thought-out:
NODE_OPTIONS: Increases heap size and enables garbage collectionNEXT_TELEMETRY_DISABLED: Disables telemetry for privacyNPM_CONFIG_PRODUCTION: Ensures dev dependencies are installedNODE_ENV: Sets production mode for optimized builds
Problem
There were some issue with the first version of the lit protocol provider from RN SDK
Changes
Upgrade to new version of the RN SDK lit protocol provider
Summary by CodeRabbit
New Features
LitProvidercomponent for improved protocol initialization.DynamicLitProviderfor conditional rendering based on connection status.Updates
Bug Fixes
InvoiceDashboardPropsinterface, streamlining the component's props.Chores