SELF-1175: Use Job status instead for backup status #1411
Conversation
WalkthroughDerives backup pending/started/completed flags from the point-events store, changes setBackupForPointsCompleted to accept an optional boolean, and refactors job status parsing to use a boolean Changes
Sequence Diagram(s)sequenceDiagram
participant Points as NavBar/Points.tsx
participant Store as pointEventStore
participant Settings as settingStore
rect rgb(230,245,255)
note right of Store: Derived flags: pending, started, completed
end
Store->>Points: expose { pending, started, completed }
activate Points
Points->>Points: useEffect watches derived flags + hasCompletedBackupForPoints
alt no pending && no completed && hasCompletedBackupForPoints
Points->>Settings: setBackupForPointsCompleted(false)
end
deactivate Points
sequenceDiagram
participant Client as jobStatus fn
participant API as /job/:id
participant Parser as jobStatus.ts logic
Client->>API: GET job status
API-->>Client: 200 { job_id, success: true/false, message? }
Client->>Parser: parse response
alt success === true
Parser-->>Client: "completed"
else success === false and message == "Address already verified"
Parser-->>Client: "completed"
else success === false
Parser-->>Client: "failed"
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🧰 Additional context used📓 Path-based instructions (3)**/*.{js,ts,tsx,jsx,sol,nr}📄 CodeRabbit inference engine (.cursorrules)
Files:
app/**/*.{ts,tsx}📄 CodeRabbit inference engine (app/AGENTS.md)
Files:
app/src/**/*.{ts,tsx,js,jsx}⚙️ CodeRabbit configuration file
Files:
🔇 Additional comments (2)
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'; }
| setTimeout(() => { | ||
| navigation.navigate(params.returnToScreen); | ||
| }, 500); |
There was a problem hiding this comment.
Cleanup missing setTimeout timers to prevent memory leaks and stale navigation.
The three setTimeout calls delay navigation by 500ms but don't store the timer ID for cleanup. If the component unmounts during this delay, the navigation call will still execute, potentially causing errors or navigating from an unmounted component.
Store and clean up the timeout:
+ const timeoutRef = React.useRef<NodeJS.Timeout | null>(null);
+
+ React.useEffect(() => {
+ return () => {
+ if (timeoutRef.current) {
+ clearTimeout(timeoutRef.current);
+ }
+ };
+ }, []);
// In handleICloudBackup:
if (params?.returnToScreen) {
- setTimeout(() => {
+ timeoutRef.current = setTimeout(() => {
navigation.navigate(params.returnToScreen);
}, 500);
}Apply similar changes to the other two setTimeout calls at lines 181-183 and 195-197.
Also consider documenting why the 500ms delay is necessary—this appears related to backup state synchronization but the rationale isn't clear from the code.
Also applies to: 181-183, 195-197
🤖 Prompt for AI Agents
In app/src/screens/account/settings/CloudBackupScreen.tsx around lines 132-134,
181-183 and 195-197, the setTimeout calls that delay navigation by 500ms are not
storing the timer IDs and therefore cannot be cleared on unmount; capture each
setTimeout return value into a const (e.g. const timeoutId = setTimeout(...)),
keep them in component scope (or state/ref) and clear them in the component
cleanup (useEffect return or componentWillUnmount) via clearTimeout(timeoutId);
apply this pattern to all three timeouts and add a short comment noting the
500ms rationale if still required.
| removeSubscribedTopic: (topic: string) => void; | ||
| hasCompletedBackupForPoints: boolean; | ||
| setBackupForPointsCompleted: () => void; | ||
| setBackupForPointsCompleted: (value?: boolean) => void; |
There was a problem hiding this comment.
rather than keeping this here and have to derive it from data in the poinEvents store. what about adding a function to pointEvents store hasCompletedBackupForPoints
this would mean less state to worry about
Changes
Previous Flow
New Flow
Other changes
/job/{}/statuscheckdata.success === trueinstead ofcompleteddata.message.Summary by CodeRabbit