Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughSimplified completion state handling in TunnelProvingScreen by consolidating two-phase transitions into single-path navigation. Removed conditional phase-based logic, registration re-initialization call, and associated error handling. Added analytics event and 5-second navigation delay. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ce5e2c4e-1bd0-4215-8b2c-832ce527fb9e
📒 Files selected for processing (1)
packages/webview-app/src/screens/tunnel/TunnelProvingScreen.tsx
| } else if (currentState === 'completed') { | ||
| analytics.trackEvent('tunnel_proving_registration_complete', { previousPhase: phase }); | ||
| // Brief delay to allow tree reader to index the on-chain commitment | ||
| // before disclose fetches the identity tree. | ||
| setTimeout(() => navigate('/tunnel/proof/disclose', { replace: true }), 5000); | ||
| } | ||
| }, [currentState, initDone, phase, client, init, analytics, haptic, navigate, errorCode, reason, navigateToError]); | ||
| }, [currentState, initDone, phase, analytics, haptic, navigate, errorCode, reason, navigateToError]); |
There was a problem hiding this comment.
Missing cleanup for setTimeout causes navigation after unmount.
The 5-second setTimeout at line 111 isn't cleared if the component unmounts before it fires. This will trigger navigation on an unmounted component, causing React warnings and unexpected navigation behavior if the user has already left this screen.
Proposed fix
} else if (currentState === 'completed') {
analytics.trackEvent('tunnel_proving_registration_complete', { previousPhase: phase });
// Brief delay to allow tree reader to index the on-chain commitment
// before disclose fetches the identity tree.
- setTimeout(() => navigate('/tunnel/proof/disclose', { replace: true }), 5000);
+ const timeoutId = setTimeout(() => navigate('/tunnel/proof/disclose', { replace: true }), 5000);
+ return () => clearTimeout(timeoutId);
}
-}, [currentState, initDone, phase, analytics, haptic, navigate, errorCode, reason, navigateToError]);
+ }, [currentState, initDone, phase, analytics, haptic, navigate, errorCode, reason, navigateToError]);📝 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.
| } else if (currentState === 'completed') { | |
| analytics.trackEvent('tunnel_proving_registration_complete', { previousPhase: phase }); | |
| // Brief delay to allow tree reader to index the on-chain commitment | |
| // before disclose fetches the identity tree. | |
| setTimeout(() => navigate('/tunnel/proof/disclose', { replace: true }), 5000); | |
| } | |
| }, [currentState, initDone, phase, client, init, analytics, haptic, navigate, errorCode, reason, navigateToError]); | |
| }, [currentState, initDone, phase, analytics, haptic, navigate, errorCode, reason, navigateToError]); | |
| useEffect(() => { | |
| // ... existing code ... | |
| let timeoutId: NodeJS.Timeout | undefined; | |
| if (currentState === 'completed') { | |
| analytics.trackEvent('tunnel_proving_registration_complete', { previousPhase: phase }); | |
| // Brief delay to allow tree reader to index the on-chain commitment | |
| // before disclose fetches the identity tree. | |
| timeoutId = setTimeout(() => navigate('/tunnel/proof/disclose', { replace: true }), 5000); | |
| } | |
| return () => { | |
| if (timeoutId) clearTimeout(timeoutId); | |
| }; | |
| }, [currentState, initDone, phase, analytics, haptic, navigate, errorCode, reason, navigateToError]); |
| state: currentState, | ||
| }); | ||
| navigateToError(reason ?? errorCode ?? currentState); | ||
| } else if (currentState === 'completed' && phase === 'dsc') { |
There was a problem hiding this comment.
Removing this because the dsc to register process happens inside the provingMachine. We don't need to start init provingMachine with register.
For the KYC flow, the currentState will transition to completed once the register step is completed.
For the mock passport flow, the currentState will transition to completed when both the dsc and register steps are completed.
1 participant
Summary
Fixes - wrongly navigating to
Recovery requiredscreen in mock passport flow.Test plan
Native Consolidation Checklist
cd app && yarn jest:run/yarn workspace @selfxyz/rn-sdk-test-app test)Summary by CodeRabbit