Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughUpdates backup completion handling: job status responses use a boolean Changes
Sequence Diagram(s)sequenceDiagram
participant UI as "Points UI\n(app/src/components/navbar/Points.tsx)"
participant Events as "PointEvent Store\n(job events)"
participant JobSvc as "JobStatus Service\n(app/src/services/points/jobStatus.ts)"
participant Settings as "SettingStore\n(app/src/stores/settingStore.ts)"
UI->>Events: subscribe / derive getBackupState
Events-->>UI: emits pending/completed flags
UI->>JobSvc: request job status (poll)
JobSvc-->>UI: returns { success, message }
UI->>UI: compute backupState (pending/completed/started)
alt no pending/completed && hasCompletedBackupForPoints
UI->>Settings: setBackupForPointsCompleted(false)
Settings-->>UI: updated flag persisted
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes Areas requiring attention (medium+ priority):
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
seshanthS
changed the title
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
app/src/utils/points/jobStatus.ts (1)
7-10: Update JobStatusResponse type to match the actual API response.The type definition still references
status: 'complete' | 'failed', but the code at lines 36-44 now checksdata.success(boolean) anddata.message(string). This is a critical type mismatch that breaks type safety.Update the type definition to reflect the actual API response structure:
export type JobStatusResponse = { job_id: string; - status: 'complete' | 'failed'; + success: boolean; + message?: string; };app/src/stores/settingStore.ts (1)
34-34: Update the type declaration to match the implementation signature.The
PersistedSettingsStateinterface declaressetBackupForPointsCompleted: () => voidbut the implementation at lines 110-111 now accepts an optional boolean parameter(value: boolean = true). This type mismatch breaks type safety.Update the interface to reflect the new signature:
hasCompletedBackupForPoints: boolean; - setBackupForPointsCompleted: () => void; + setBackupForPointsCompleted: (value?: boolean) => void; resetBackupForPoints: () => void;
🧹 Nitpick comments (1)
app/src/components/NavBar/Points.tsx (1)
90-93: Fix typos and clarify the comment.The comment has grammatical errors ("ccan be delayed") and could be clearer about the purpose of this effect.
- //we show the backup success message immediately. This flips the flag to false undo the action - //and to show the backup button again. - //Another way is to show success modal here, but this ccan be delayed (as polling can be upto 32 seconds) + // We show the backup success message immediately. This effect resets the flag to false + // to undo that action and show the backup button again once there are no pending or completed backups. + // Note: Showing the success modal here would be delayed (polling can take up to 32 seconds). useEffect(() => {
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
app/src/components/NavBar/Points.tsx(1 hunks)app/src/screens/account/settings/CloudBackupScreen.tsx(3 hunks)app/src/stores/settingStore.ts(1 hunks)app/src/utils/points/jobStatus.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{js,ts,tsx,jsx,sol,nr}
📄 CodeRabbit inference engine (.cursorrules)
**/*.{js,ts,tsx,jsx,sol,nr}: NEVER log sensitive data including PII (names, DOB, passport numbers, addresses), credentials, tokens, API keys, private keys, or session identifiers.
ALWAYS redact/mask sensitive fields in logs using consistent patterns (e.g.,***-***-1234for passport numbers,J*** D***for names).
Files:
app/src/components/NavBar/Points.tsxapp/src/screens/account/settings/CloudBackupScreen.tsxapp/src/stores/settingStore.tsapp/src/utils/points/jobStatus.ts
app/**/*.{ts,tsx}
📄 CodeRabbit inference engine (app/AGENTS.md)
Type checking must pass before PRs (yarn types)
Files:
app/src/components/NavBar/Points.tsxapp/src/screens/account/settings/CloudBackupScreen.tsxapp/src/stores/settingStore.tsapp/src/utils/points/jobStatus.ts
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/components/NavBar/Points.tsxapp/src/screens/account/settings/CloudBackupScreen.tsxapp/src/stores/settingStore.tsapp/src/utils/points/jobStatus.ts
🧠 Learnings (1)
📚 Learning: 2025-09-22T11:10:22.019Z
Learnt from: CR
Repo: selfxyz/self PR: 0
File: .cursorrules:0-0
Timestamp: 2025-09-22T11:10:22.019Z
Learning: Applies to src/screens/**/*.{js,ts,tsx,jsx} : Lazy load screens using `React.lazy()`, organize screens by feature modules.
Applied to files:
app/src/screens/account/settings/CloudBackupScreen.tsx
🧬 Code graph analysis (1)
app/src/components/NavBar/Points.tsx (1)
app/src/stores/pointEventStore.ts (1)
usePointEventStore(53-334)
⏰ 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). (5)
- GitHub Check: e2e-ios
- GitHub Check: android-build-test
- GitHub Check: analyze-android
- GitHub Check: build-deps
- GitHub Check: analyze-ios
🔇 Additional comments (1)
app/src/utils/points/jobStatus.ts (1)
39-42: Clarify the business logic for "Address already verified" special case.When
data.success === falsebut the message is "Address already verified", the status returns 'completed' instead of 'failed'. This special-case handling is counterintuitive—why does a failed response with this specific message indicate completion?Consider documenting this business logic with a comment explaining why this particular failure message should be treated as success. For example:
// Special case: If verification fails because address is already verified, // treat as completed since the end goal (verified address) is achieved if (data.message && data.message === 'Address already verified') { return 'completed'; }
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/src/components/navbar/Points.tsx(1 hunks)app/src/services/points/jobStatus.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (13)
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (.cursorrules)
**/*.{js,jsx,ts,tsx}: NEVER log sensitive data including PII (names, DOB, passport numbers, addresses), credentials, tokens, API keys, private keys, or session identifiers.
ALWAYS redact/mask sensitive fields in logs using consistent patterns (e.g.,***-***-1234for passport numbers,J*** D***for names).
Files:
app/src/services/points/jobStatus.tsapp/src/components/navbar/Points.tsx
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.cursorrules)
**/*.{ts,tsx,js,jsx}: Use React Navigation withcreateStaticNavigationfor type-safe navigation in React Native applications.
Implement platform-specific handling withPlatform.OS === 'ios' ? 'iOS' : 'Android'checks before platform-specific code in React Native.
Initialize native modules withinitializeNativeModules()before any native operations in React Native.
Implement lazy loading for screens usingReact.lazy()in React Native applications.
Implement custom modal system withuseModalhook and callback registry in React Native.
Integrate haptic feedback usinguseHapticNavigationhook in React Native navigation.
Use platform-specific initial routes: web uses 'Home', mobile uses 'Splash' in React Navigation.
Use Zustand for global state management in React Native applications.
Use custom hooks for complex state (useModal,useHapticNavigation) instead of inline logic.
Use AsyncStorage for simple data, SQLite for complex data, and Keychain for sensitive data in React Native.
Use@/alias for src imports and@tests/alias for test imports in TypeScript/JavaScript files.
Use conditional rendering with Platform.OS for platform-specific code in React Native.
Use Tamagui for UI components in React Native applications.
Do not log sensitive data in production, including identity verification and passport information.
Use Keychain for secure storage of sensitive data in React Native.
Implement proper cleanup of sensitive data after use.
Implement certificate validation for passport data verification.
Always use try-catch for async operations in React Native and TypeScript code.
Implement graceful degradation when native modules fail in React Native.
Provide user-friendly error messages in UI and error handlers.
Lazy load screens and components to optimize bundle size in React Native.
Prevent memory leaks in native modules in React Native.
Files:
app/src/services/points/jobStatus.tsapp/src/components/navbar/Points.tsx
**/*.{tsx,jsx,ts,js}
📄 CodeRabbit inference engine (.cursorrules)
Implement proper cleanup in useEffect and component unmount hooks in React.
Files:
app/src/services/points/jobStatus.tsapp/src/components/navbar/Points.tsx
**/{mobile,client,app,time,verification}/**/*.{ts,tsx,js,swift,kt}
📄 CodeRabbit inference engine (.cursor/rules/compliance-verification.mdc)
Use server-signed time tokens or chain block timestamps for trusted time in mobile clients, do not trust device wall-clock alone
Files:
app/src/services/points/jobStatus.tsapp/src/components/navbar/Points.tsx
**/{mobile,client,app,proof,zk}/**/*.{ts,tsx,js,swift,kt}
📄 CodeRabbit inference engine (.cursor/rules/compliance-verification.mdc)
**/{mobile,client,app,proof,zk}/**/*.{ts,tsx,js,swift,kt}: Include trusted time anchor in proof generation and verify time anchor authenticity before proof generation in mobile implementations
Achieve proof generation in <60 seconds on mid-tier mobile devices
Files:
app/src/services/points/jobStatus.tsapp/src/components/navbar/Points.tsx
app/src/**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (.cursor/rules/mobile-sdk-migration.mdc)
Use module mapping
@/→src/and@tests/→tests/src/in app Jest configuration
Files:
app/src/services/points/jobStatus.tsapp/src/components/navbar/Points.tsx
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (.cursor/rules/mobile-sdk-migration.mdc)
**/*.{ts,tsx,js}: Never log PII, credentials, or private keys in production code; use DEBUG_SECRETS_TOKEN flag for debug-level secrets
Use consistent redaction patterns for sensitive fields in logs and test data
Files:
app/src/services/points/jobStatus.tsapp/src/components/navbar/Points.tsx
app/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/mobile-sdk-migration.mdc)
Update app to consume mobile-sdk-alpha modules after migration and validate all existing app tests pass
Files:
app/src/services/points/jobStatus.tsapp/src/components/navbar/Points.tsx
app/**/*.{ts,tsx,js,jsx,json,yml,yaml}
📄 CodeRabbit inference engine (app/AGENTS.md)
Ensure
yarn nicepasses (fixes linting and formatting) before creating a PR
Files:
app/src/services/points/jobStatus.tsapp/src/components/navbar/Points.tsx
app/**/*.{ts,tsx}
📄 CodeRabbit inference engine (app/AGENTS.md)
Ensure
yarn typespasses (TypeScript validation) before creating a PR
Files:
app/src/services/points/jobStatus.tsapp/src/components/navbar/Points.tsx
app/**/*.{ts,tsx,js,jsx,swift,kt,java}
📄 CodeRabbit inference engine (app/AGENTS.md)
app/**/*.{ts,tsx,js,jsx,swift,kt,java}: Flag security-sensitive operations and note performance implications in code comments
Ensure no sensitive data (PII, credentials, tokens) is present in logs
Files:
app/src/services/points/jobStatus.tsapp/src/components/navbar/Points.tsx
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/services/points/jobStatus.tsapp/src/components/navbar/Points.tsx
**/*.{tsx,jsx}
📄 CodeRabbit inference engine (.cursorrules)
Implement comprehensive error boundaries in React components.
Files:
app/src/components/navbar/Points.tsx
⏰ 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). (7)
- GitHub Check: test-common
- GitHub Check: type-check
- GitHub Check: build-deps
- GitHub Check: e2e-ios
- GitHub Check: android-build-test
- GitHub Check: analyze-android
- GitHub Check: analyze-ios
🔇 Additional comments (1)
app/src/services/points/jobStatus.ts (1)
7-11: Job status mapping tosuccessflag and special-case looks soundThe new
JobStatusResponseshape and the 200-handling usingdata.success(with the"Address already verified"idempotent-success override) align with the described backend behavior and avoid falsely treating that case as a failure. I don’t see correctness or security concerns in this mapping.Also applies to: 37-45
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
app/src/components/navbar/Points.tsx (1)
79-88: Zustand selector returns a new object reference on every store update, causing unnecessary re-renders.This selector creates a fresh
{ pending, completed, started }object on everypointEventStorestate change. Since Zustand defaults toObject.isequality, any store mutation (not just backup-related events) will trigger a re-render of the entirePointscomponent.Use
useShallowfromzustand/react/shallowor split into individual primitive selectors.Additionally,
startedis computed but never referenced anywhere in this file.♻️ Option A: use useShallow
+import { useShallow } from 'zustand/react/shallow'; ... - const getBackupState = usePointEventStore(state => { - const backups = state.events.filter(e => e.type === 'backup'); - const isPending = backups.some(e => e.status === 'pending'); - const isCompleted = backups.some(e => e.status === 'completed'); - return { - pending: isPending, - completed: isCompleted, - started: isPending || isCompleted, - }; - }); + const { pending: isBackupPending, completed: isBackupCompleted } = + usePointEventStore( + useShallow(state => { + const backups = state.events.filter(e => e.type === 'backup'); + return { + pending: backups.some(e => e.status === 'pending'), + completed: backups.some(e => e.status === 'completed'), + }; + }), + );♻️ Option B: separate primitive selectors (zero extra deps)
- const getBackupState = usePointEventStore(state => { - const backups = state.events.filter(e => e.type === 'backup'); - const isPending = backups.some(e => e.status === 'pending'); - const isCompleted = backups.some(e => e.status === 'completed'); - return { - pending: isPending, - completed: isCompleted, - started: isPending || isCompleted, - }; - }); + const isBackupPending = usePointEventStore(state => + state.events.some(e => e.type === 'backup' && e.status === 'pending'), + ); + const isBackupCompleted = usePointEventStore(state => + state.events.some(e => e.type === 'backup' && e.status === 'completed'), + );
|
@seshanthS merged with |
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.
| started: isPending || isCompleted, | ||
| isLoading: state.isLoading, | ||
| }; | ||
| }); |
There was a problem hiding this comment.
Zustand selector creates new object causing excessive re-renders
Low Severity
The getBackupState selector returns a new object on every store change, and Zustand's default Object.is comparison treats each new object as "changed", triggering a re-render even when the derived boolean values (pending, completed, isLoading) haven't changed. This is compounded by usePointEventStore() without a selector on line 64, which subscribes to the entire store. Since pointEventStore is highly dynamic (event polling, points refreshing, incoming points updates), this causes the Points component to re-render much more frequently than the previous useSettingStore() approach. The selector needs useShallow from zustand/react/shallow, or the individual values could be selected separately.
Additional Locations (1)
3 participants
Changes
Previous Flow
New Flow
Other changes
/job/{}/statuscheckdata.success === trueinstead ofcompleteddata.message.Summary by CodeRabbit
Note
Medium Risk
Touches points/backup reward state and persistence plus job-status polling semantics; mistakes could incorrectly hide/show the backup CTA or misclassify event processing results.
Overview
Improves the backup-for-points flow by moving
hasCompletedBackupForPointsout ofsettingStoreand intopointEventStore, persisting it separately in AsyncStorage (with migration from existing completed backup events) and resetting it when no backup event is pending/completed so the backup widget can reappear after backend failure.Updates points job polling to interpret
/job/{id}/statusviasuccess/message(treating"Address already verified"as success) rather than astatusstring, and wires dev “danger zone” reset/clear actions to the new backup flag storage. Also fixes a modal navigationsetTimeoutinCloudBackupScreento be tracked/cleared on unmount to avoid lingering timers.Written by Cursor Bugbot for commit 995b7e6. This will update automatically on new commits. Configure here.