SELF-483: Fix and enable recovery prompts #834
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughuseRecoveryPrompts now tracks the last shown login count with a ref, adds modal visibility to the effect dependencies, fetches documents and early-returns if none exist, handles document-fetch failures with an early return, and returns an object exposing Changes
Sequence Diagram(s)sequenceDiagram
participant AppComponent
participant useRecoveryPrompts
participant useModal
participant DocumentStore as getAllDocuments()
AppComponent->>useRecoveryPrompts: call hook (loginCount, hasViewedRecoveryPhrase)
useRecoveryPrompts->>useModal: read `visible`
useRecoveryPrompts->>useRecoveryPrompts: check `lastPromptCount` (ref)
useRecoveryPrompts->>DocumentStore: getAllDocuments() (async)
alt no documents or fetch failed
useRecoveryPrompts-->>AppComponent: return { visible }
else modal not visible and loginCount !== lastPromptCount
useRecoveryPrompts->>useModal: showModal()
useRecoveryPrompts->>useRecoveryPrompts: update lastPromptCount
useRecoveryPrompts-->>AppComponent: return { visible }
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
✨ 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 |
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
transphorm
changed the title
transphorm
force-pushed
the
codex/guard-modal-trigger-in-userecoveryprompts
branch
from
f2573ba to
8ce000a
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
app/src/hooks/useRecoveryPrompts.ts (2)
37-52: Short-circuit before I/O to prevent redundant getAllDocuments calls and reduce prompt churnRight now, getAllDocuments() is called even when we already know we won’t show the modal (e.g., modal currently visible, already prompted for this loginCount, or loginCount isn’t on a prompt schedule). On mobile, avoiding unnecessary I/O and work on every dependency change is important.
Refactor to compute the prompt schedule and visibility/lastPrompt checks first, then bail early before any fetch. Also guard docs for null/undefined to avoid caught TypeErrors.
@@ - if (!cloudBackupEnabled && !hasViewedRecoveryPhrase) { - try { - const docs = await getAllDocuments(); - if (Object.keys(docs).length === 0) { - return; - } - const shouldPrompt = - loginCount > 0 && (loginCount <= 3 || (loginCount - 3) % 5 === 0); - if ( - shouldPrompt && - !visible && - lastPromptCount.current !== loginCount - ) { - showModal(); - lastPromptCount.current = loginCount; - } - } catch { - // Silently fail to avoid breaking the hook - // If we can't get documents, we shouldn't show the prompt - return; - } - } + if (!cloudBackupEnabled && !hasViewedRecoveryPhrase) { + // Fast-path: bail out before any I/O if we shouldn't prompt now, + // the modal is already visible, or we've already prompted for this loginCount. + const shouldPrompt = + loginCount > 0 && (loginCount <= 3 || (loginCount - 3) % 5 === 0); + if (!shouldPrompt || visible || lastPromptCount.current === loginCount) { + return; + } + try { + const docs = await getAllDocuments(); + if (!docs || Object.keys(docs).length === 0) { + return; + } + showModal(); + lastPromptCount.current = loginCount; + } catch { + // Silently fail to avoid breaking the hook + // If we can't get documents, we shouldn't show the prompt + return; + } + }
30-31: Ref-only lastPromptCount resets on remount; persist in store to avoid duplicate prompts across sessionsuseRef only guards within the hook’s current mount. If the hook remounts (navigation reset, hot reload, or app restart), users can be re-prompted for the same loginCount. Persisting this value (e.g., in useSettingStore or AsyncStorage keyed by user/account) avoids repeat prompts and improves UX.
Apply within this file:
- const lastPromptCount = useRef<number | null>(null); + // Persist across remounts by lifting into the settings store (example) + const { + lastRecoveryPromptLoginCount, + setLastRecoveryPromptLoginCount, + } = useSettingStore();And replace usages:
- !visible && - lastPromptCount.current !== loginCount + !visible && + lastRecoveryPromptLoginCount !== loginCount ) { showModal(); - lastPromptCount.current = loginCount; + setLastRecoveryPromptLoginCount(loginCount);Outside this file (example shape; adapt to your store API):
// In settingStore: type SettingState = { // ... lastRecoveryPromptLoginCount: number | null; setLastRecoveryPromptLoginCount: (n: number) => void; }; // Initial state: lastRecoveryPromptLoginCount: null, // Actions: setLastRecoveryPromptLoginCount: (n) => set({ lastRecoveryPromptLoginCount: n }),
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
app/src/hooks/useRecoveryPrompts.ts(4 hunks)app/tests/src/hooks/useRecoveryPrompts.test.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/tests/src/hooks/useRecoveryPrompts.test.ts
🧰 Additional context used
📓 Path-based instructions (1)
app/src/**/*.{ts,tsx,js,jsx}
⚙️ CodeRabbit Configuration File
app/src/**/*.{ts,tsx,js,jsx}: Review React Native TypeScript code for:
- Component architecture and reusability
- State management patterns
- Performance optimizations
- TypeScript type safety
- React hooks usage and dependencies
- Navigation patterns
Files:
app/src/hooks/useRecoveryPrompts.ts
⏰ Context from checks skipped due to timeout of 300000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: analyze-ios
- GitHub Check: analyze-android
- GitHub Check: e2e-ios
🔇 Additional comments (1)
app/src/hooks/useRecoveryPrompts.ts (1)
65-65: Verified: showModal is memoized, no changes required
- In app/src/hooks/useModal.ts (around line 18),
showModalis wrapped inuseCallback, ensuring its identity remains stable and won’t trigger unnecessary effect reruns.
Summary
Testing
yarn lintyarn buildyarn workspace @selfxyz/contracts build(fails: Invalid account: #0 for network: mainnet - Expected string, received undefined)yarn typesyarn workspace @selfxyz/common testyarn workspace @selfxyz/circuits test(fails: Unsupported signature algorithm: undefined)yarn workspace @selfxyz/mobile-app testhttps://chatgpt.com/codex/tasks/task_b_688efe0b2bd8832daff1b8d9d5f0f466
Summary by CodeRabbit
Bug Fixes
New Features
Tests