fix: ensure verification finishes in background once app says it is safe to close #1629
Conversation
Previously, notification would appear saying registration is finished but the document would not finish registering. Ensure that once the data is sent to the TEE to do the proof, the document will finish registering even with app closed.
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 Walkthrough📝 Walkthrough🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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: 3
🤖 Fix all issues with AI agents
In `@app/src/navigation/index.tsx`:
- Around line 246-256: runCheck currently awaits
checkPendingRegistrationWithRetry() without catching rejections which can cause
unhandled promise rejections; wrap that await in a try/catch inside runCheck
(keeping the existing finally that resets isCheckingRef.current) and handle/log
the error (e.g., with console.error or the app's logger) so failures from
checkPendingRegistrationWithRetry() are absorbed and do not break the foreground
retry loop; reference runCheck, checkPendingRegistrationWithRetry,
isCheckingRef, and selfClient when updating the function.
In `@app/src/services/registration/pendingRegistrationService.ts`:
- Around line 40-54: The loop handling RETRY_DELAYS_MS may exit with an
unhandled rejection if updateDocumentRegistrationState throws; wrap the call to
updateDocumentRegistrationState (inside the branch where checkStatus(...) ===
STATUS_SUCCESS and pendingRegistrationDocumentId is set) in a try/catch, log the
error including pendingRegistrationDocumentId and the thrown error, do not call
clearPendingRegistration on failure, and continue the retry/return logic only on
successful update; keep other actions (clearPendingRegistration, console.log,
and return { completed: true, documentId: pendingRegistrationDocumentId })
inside the try block so they run only when the update succeeds.
- Around line 95-101: The status listener on socket (socket.on('status', ...))
can throw on JSON.parse and leave the promise unresolved and the socket open;
wrap the parsing/handling logic in a try-catch inside the 'status' handler, on
success call done(status) and on any parse/error path call done(null), ensure
you still clearTimeout(timeout) and remove/close the socket (or remove the
'status' listener) in both branches so the promise never hangs and the socket
does not leak; reference the existing socket.on('status', (msg: unknown) => {
... }), the done callback, and the timeout variable when implementing this.
🧹 Nitpick comments (1)
app/src/services/registration/pendingRegistrationService.ts (1)
13-16: Use exponential backoff with jitter for retries.
Fixed delays can synchronize clients and spike relayer load; jittered backoff reduces retry storms. Based on learnings, please apply exponential backoff with jitter.♻️ Suggested refactor
-const RETRY_DELAYS_MS = [5000, 10000, 20000, 30000, 30000]; // 5 retries with increasing delays +const RETRY_BASE_MS = 5000; +const RETRY_COUNT = 5; +const jitter = (ms: number) => Math.floor(ms * (0.8 + Math.random() * 0.4)); +const RETRY_DELAYS_MS = Array.from({ length: RETRY_COUNT }, (_, i) => + jitter(RETRY_BASE_MS * 2 ** i), +);
| const runCheck = async () => { | ||
| if (isCheckingRef.current || !selfClient) return; | ||
| const { pendingRegistrationUuid } = useSettingStore.getState(); | ||
| if (!pendingRegistrationUuid) return; | ||
|
|
||
| isCheckingRef.current = true; | ||
| try { | ||
| await checkPendingRegistrationWithRetry(); | ||
| } finally { | ||
| isCheckingRef.current = false; | ||
| } |
There was a problem hiding this comment.
Handle errors from pending registration checks.
checkPendingRegistrationWithRetry() is awaited without a catch; a rejection will surface as an unhandled promise and can disrupt the foreground retry loop. As per coding guidelines, wrap async calls with try/catch.
✅ Suggested fix
- try {
- await checkPendingRegistrationWithRetry();
- } finally {
- isCheckingRef.current = false;
- }
+ try {
+ await checkPendingRegistrationWithRetry();
+ } catch {
+ console.warn('Pending registration check failed');
+ } finally {
+ isCheckingRef.current = false;
+ }📝 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.
| const runCheck = async () => { | |
| if (isCheckingRef.current || !selfClient) return; | |
| const { pendingRegistrationUuid } = useSettingStore.getState(); | |
| if (!pendingRegistrationUuid) return; | |
| isCheckingRef.current = true; | |
| try { | |
| await checkPendingRegistrationWithRetry(); | |
| } finally { | |
| isCheckingRef.current = false; | |
| } | |
| const runCheck = async () => { | |
| if (isCheckingRef.current || !selfClient) return; | |
| const { pendingRegistrationUuid } = useSettingStore.getState(); | |
| if (!pendingRegistrationUuid) return; | |
| isCheckingRef.current = true; | |
| try { | |
| await checkPendingRegistrationWithRetry(); | |
| } catch { | |
| console.warn('Pending registration check failed'); | |
| } finally { | |
| isCheckingRef.current = false; | |
| } | |
| } |
🤖 Prompt for AI Agents
In `@app/src/navigation/index.tsx` around lines 246 - 256, runCheck currently
awaits checkPendingRegistrationWithRetry() without catching rejections which can
cause unhandled promise rejections; wrap that await in a try/catch inside
runCheck (keeping the existing finally that resets isCheckingRef.current) and
handle/log the error (e.g., with console.error or the app's logger) so failures
from checkPendingRegistrationWithRetry() are absorbed and do not break the
foreground retry loop; reference runCheck, checkPendingRegistrationWithRetry,
isCheckingRef, and selfClient when updating the function.
| for (let i = 0; i < RETRY_DELAYS_MS.length; i++) { | ||
| const status = await checkStatus(wsUrl, pendingRegistrationUuid); | ||
|
|
||
| if (status === STATUS_SUCCESS && pendingRegistrationDocumentId) { | ||
| await updateDocumentRegistrationState( | ||
| pendingRegistrationDocumentId, | ||
| true, | ||
| ); | ||
| clearPendingRegistration(); | ||
| console.log( | ||
| 'Completed pending registration:', | ||
| pendingRegistrationDocumentId, | ||
| ); | ||
| return { completed: true, documentId: pendingRegistrationDocumentId }; | ||
| } |
There was a problem hiding this comment.
Guard registration state update failures.
If updateDocumentRegistrationState throws, the promise rejects and the loop exits with an unhandled rejection. As per coding guidelines, wrap async calls with try/catch.
✅ Suggested fix
- if (status === STATUS_SUCCESS && pendingRegistrationDocumentId) {
- await updateDocumentRegistrationState(
- pendingRegistrationDocumentId,
- true,
- );
- clearPendingRegistration();
- console.log(
- 'Completed pending registration:',
- pendingRegistrationDocumentId,
- );
- return { completed: true, documentId: pendingRegistrationDocumentId };
- }
+ if (status === STATUS_SUCCESS && pendingRegistrationDocumentId) {
+ try {
+ await updateDocumentRegistrationState(
+ pendingRegistrationDocumentId,
+ true,
+ );
+ clearPendingRegistration();
+ console.log(
+ 'Completed pending registration:',
+ pendingRegistrationDocumentId,
+ );
+ return { completed: true, documentId: pendingRegistrationDocumentId };
+ } catch {
+ console.warn('Pending registration update failed');
+ return { completed: false, documentId: null };
+ }
+ }📝 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.
| for (let i = 0; i < RETRY_DELAYS_MS.length; i++) { | |
| const status = await checkStatus(wsUrl, pendingRegistrationUuid); | |
| if (status === STATUS_SUCCESS && pendingRegistrationDocumentId) { | |
| await updateDocumentRegistrationState( | |
| pendingRegistrationDocumentId, | |
| true, | |
| ); | |
| clearPendingRegistration(); | |
| console.log( | |
| 'Completed pending registration:', | |
| pendingRegistrationDocumentId, | |
| ); | |
| return { completed: true, documentId: pendingRegistrationDocumentId }; | |
| } | |
| for (let i = 0; i < RETRY_DELAYS_MS.length; i++) { | |
| const status = await checkStatus(wsUrl, pendingRegistrationUuid); | |
| if (status === STATUS_SUCCESS && pendingRegistrationDocumentId) { | |
| try { | |
| await updateDocumentRegistrationState( | |
| pendingRegistrationDocumentId, | |
| true, | |
| ); | |
| clearPendingRegistration(); | |
| console.log( | |
| 'Completed pending registration:', | |
| pendingRegistrationDocumentId, | |
| ); | |
| return { completed: true, documentId: pendingRegistrationDocumentId }; | |
| } catch { | |
| console.warn('Pending registration update failed'); | |
| return { completed: false, documentId: null }; | |
| } | |
| } |
🤖 Prompt for AI Agents
In `@app/src/services/registration/pendingRegistrationService.ts` around lines 40
- 54, The loop handling RETRY_DELAYS_MS may exit with an unhandled rejection if
updateDocumentRegistrationState throws; wrap the call to
updateDocumentRegistrationState (inside the branch where checkStatus(...) ===
STATUS_SUCCESS and pendingRegistrationDocumentId is set) in a try/catch, log the
error including pendingRegistrationDocumentId and the thrown error, do not call
clearPendingRegistration on failure, and continue the retry/return logic only on
successful update; keep other actions (clearPendingRegistration, console.log,
and return { completed: true, documentId: pendingRegistrationDocumentId })
inside the try block so they run only when the update succeeds.
| socket.on('status', (msg: unknown) => { | ||
| clearTimeout(timeout); | ||
| const status = | ||
| typeof msg === 'string' | ||
| ? (JSON.parse(msg) as { status: number }).status | ||
| : (msg as { status: number }).status; | ||
| done(status); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cd app && find . -name "pendingRegistrationService.ts" -type fRepository: selfxyz/self
Length of output: 113
🏁 Script executed:
cat -n app/src/services/registration/pendingRegistrationService.ts | head -150Repository: selfxyz/self
Length of output: 4210
🏁 Script executed:
cat -n app/src/services/registration/pendingRegistrationService.ts | tail -50Repository: selfxyz/self
Length of output: 1644
Add error handling in status listener to prevent promise hanging and socket leaks.
If JSON.parse throws while parsing the status message, the promise never resolves and the socket stays open indefinitely. Wrap the parsing logic in try-catch to ensure done(null) is called on parse errors.
Suggested fix
socket.on('status', (msg: unknown) => {
clearTimeout(timeout);
- const status =
- typeof msg === 'string'
- ? (JSON.parse(msg) as { status: number }).status
- : (msg as { status: number }).status;
- done(status);
+ try {
+ const status =
+ typeof msg === 'string'
+ ? (JSON.parse(msg) as { status: number }).status
+ : (msg as { status: number }).status;
+ done(status);
+ } catch {
+ done(null);
+ }
});📝 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.
| socket.on('status', (msg: unknown) => { | |
| clearTimeout(timeout); | |
| const status = | |
| typeof msg === 'string' | |
| ? (JSON.parse(msg) as { status: number }).status | |
| : (msg as { status: number }).status; | |
| done(status); | |
| socket.on('status', (msg: unknown) => { | |
| clearTimeout(timeout); | |
| try { | |
| const status = | |
| typeof msg === 'string' | |
| ? (JSON.parse(msg) as { status: number }).status | |
| : (msg as { status: number }).status; | |
| done(status); | |
| } catch { | |
| done(null); | |
| } | |
| }); |
🤖 Prompt for AI Agents
In `@app/src/services/registration/pendingRegistrationService.ts` around lines 95
- 101, The status listener on socket (socket.on('status', ...)) can throw on
JSON.parse and leave the promise unresolved and the socket open; wrap the
parsing/handling logic in a try-catch inside the 'status' handler, on success
call done(status) and on any parse/error path call done(null), ensure you still
clearTimeout(timeout) and remove/close the socket (or remove the 'status'
listener) in both branches so the promise never hangs and the socket does not
leak; reference the existing socket.on('status', (msg: unknown) => { ... }), the
done callback, and the timeout variable when implementing this.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 3 potential issues.
This is the final PR Bugbot will review for you during this billing cycle
Your free Bugbot reviews will reset on February 17
Details
Your team is on the Bugbot Free tier. On this plan, Bugbot will review limited PRs each billing cycle for each member of your team.
To receive Bugbot reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial.
| pendingRegistrationDocumentId, | ||
| ); | ||
| return { completed: true, documentId: pendingRegistrationDocumentId }; | ||
| } |
There was a problem hiding this comment.
Pending registration not cleared on server success
Medium Severity
When checkPendingRegistrationWithRetry receives STATUS_SUCCESS (4) from the server but pendingRegistrationDocumentId is null, the pending registration is never cleared. The condition on line 43 requires both success status AND documentId to be truthy, but the failure handler on line 56 only matches statuses 3 or 5 (not 4). This leaves the pending registration persisted indefinitely, causing repeated WebSocket checks (~95 seconds of delays) on every app foreground event.
| ? (JSON.parse(msg) as { status: number }).status | ||
| : (msg as { status: number }).status; | ||
| done(status); | ||
| }); |
There was a problem hiding this comment.
Promise hangs if status message parsing fails
Medium Severity
In the status event handler, clearTimeout(timeout) is called before attempting to parse the message. If JSON.parse(msg) throws (e.g., malformed JSON from server), the exception is unhandled, done() is never called, and the Promise never resolves. Since the timeout was already cleared, there's no safety net. The await checkStatus(...) in checkPendingRegistrationWithRetry will hang indefinitely, blocking all subsequent registration checks.
| pendingRegistrationDocumentId, | ||
| true, | ||
| ); | ||
| clearPendingRegistration(); |
There was a problem hiding this comment.
Race condition clears wrong pending registration
Medium Severity
checkPendingRegistrationWithRetry captures pendingRegistrationUuid at the start but the retry loop can run for 95+ seconds. If a new registration starts during this time (calling setPendingRegistration with a new UUID), the old retry loop's clearPendingRegistration() call at lines 48 or 57 clears the CURRENT store state—wiping out the newer registration's pending data. The user could then close the app believing their new registration is safe, but it won't resume on next foreground.
Additional Locations (1)
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@app/src/stores/settingStore.ts`:
- Around line 161-183: The pendingRegistrationTimestamp field is set in
setPendingRegistration but never used, so add a staleness check in
pendingRegistrationService to avoid retrying very old registrations: in
settingStore.ts keep pendingRegistrationTimestamp and ensure
pendingRegistrationService (the logic that reads
pendingRegistrationUuid/documentId) verifies timestamp freshness (e.g., compare
Date.now() - pendingRegistrationTimestamp to a TTL like 24 hours) and calls
clearPendingRegistration if stale; alternatively, if you prefer removal, delete
pendingRegistrationTimestamp and all references and adjust
setPendingRegistration/clearPendingRegistration accordingly—pick one approach
and update both settingStore.ts
(setPendingRegistration/clearPendingRegistration) and pendingRegistrationService
to be consistent.
🧹 Nitpick comments (1)
app/src/providers/selfClientProvider.tsx (1)
226-248: Verify error propagation behavior is intentional.The catch block at lines 243-247 swallows all errors with only a warning log. While graceful degradation is reasonable here (registration can still proceed without persistence tracking), ensure this aligns with the expected behavior — if persisting pending registration fails, the "safe to close" feature won't work as intended for that session.
Consider whether a more prominent log level (e.g.,
console.error) or analytics event would help diagnose issues in production where users report registration not completing after closing.💡 Optional: Add analytics tracking for failure cases
} catch { - console.warn( - 'Cannot save pending registration: failed to load catalog', - ); + console.warn('Cannot save pending registration: failed to load catalog'); + trackEvent('PENDING_REGISTRATION_SAVE_FAILED', { + reason: 'catalog_load_failure', + }); }
| // Pending registration tracking - for resuming registration after app restart | ||
| pendingRegistrationUuid: null, | ||
| pendingRegistrationDocumentId: null, | ||
| pendingRegistrationTimestamp: null, | ||
| pendingRegistrationIsMock: false, | ||
| setPendingRegistration: ( | ||
| uuid: string, | ||
| documentId: string, | ||
| isMock: boolean, | ||
| ) => | ||
| set({ | ||
| pendingRegistrationUuid: uuid, | ||
| pendingRegistrationDocumentId: documentId, | ||
| pendingRegistrationTimestamp: Date.now(), | ||
| pendingRegistrationIsMock: isMock, | ||
| }), | ||
| clearPendingRegistration: () => | ||
| set({ | ||
| pendingRegistrationUuid: null, | ||
| pendingRegistrationDocumentId: null, | ||
| pendingRegistrationTimestamp: null, | ||
| pendingRegistrationIsMock: false, | ||
| }), |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
fd -t f "pendingRegistrationService" --exec cat {}Repository: selfxyz/self
Length of output: 3405
🏁 Script executed:
rg "pendingRegistrationTimestamp" -A 3 -B 3Repository: selfxyz/self
Length of output: 1992
🏁 Script executed:
rg "pendingRegistration" -t ts -t tsx --exec cat {} \;Repository: selfxyz/self
Length of output: 464
🏁 Script executed:
rg "pendingRegistrationTimestamp" -A 2 -B 2Repository: selfxyz/self
Length of output: 1474
🏁 Script executed:
rg "24.*hour|stale|old.*registration" -i -t ts -t tsxRepository: selfxyz/self
Length of output: 83
🏁 Script executed:
rg "pendingRegistration" -t js -t ts | head -30Repository: selfxyz/self
Length of output: 2643
🏁 Script executed:
find app/src -name "*.tsx" -o -name "*.ts" | xargs grep -l "pendingRegistration" | head -10Repository: selfxyz/self
Length of output: 175
🏁 Script executed:
cat app/src/navigation/index.tsx | grep -A 10 -B 10 "pendingRegistration"Repository: selfxyz/self
Length of output: 1690
🏁 Script executed:
sed -n '1,150p' app/src/navigation/index.tsx | grep -A 5 -B 5 "checkPendingRegistrationWithRetry"Repository: selfxyz/self
Length of output: 643
Remove unused pendingRegistrationTimestamp or implement staleness checks.
The pendingRegistrationTimestamp is collected in settingStore.ts but never used. The pendingRegistrationService.ts has no staleness checks—it retries for ~95 seconds, then gives up. If the device was offline for days and the app restarts, the service will retry the stale registration without any timeout logic. Either implement a staleness check (e.g., clear registrations older than 24 hours) using this timestamp, or remove the unused field to avoid confusion.
🤖 Prompt for AI Agents
In `@app/src/stores/settingStore.ts` around lines 161 - 183, The
pendingRegistrationTimestamp field is set in setPendingRegistration but never
used, so add a staleness check in pendingRegistrationService to avoid retrying
very old registrations: in settingStore.ts keep pendingRegistrationTimestamp and
ensure pendingRegistrationService (the logic that reads
pendingRegistrationUuid/documentId) verifies timestamp freshness (e.g., compare
Date.now() - pendingRegistrationTimestamp to a TTL like 24 hours) and calls
clearPendingRegistration if stale; alternatively, if you prefer removal, delete
pendingRegistrationTimestamp and all references and adjust
setPendingRegistration/clearPendingRegistration accordingly—pick one approach
and update both settingStore.ts
(setPendingRegistration/clearPendingRegistration) and pendingRegistrationService
to be consistent.
Description
Previously it would tell users it is safe to close the app before it was actually safe to close. Closing at this point would result in newly added documents not being registered. Firstly, changed to only show the screen 'safe to close' after the 2nd user authentication input, and the mobile has sent the payload to the TEE for the proof. Secondly, if a user does close the app after that point, it will actually finish registration. It used to show a notification saying the document was finished and ready, even when it was still unregistered.
Tested
Tested on local mobile build closing the mobile app after it says it is safe to close. Safe to close was displayed at correct spot in the registration process and it would finish registering even when app was closed.
How to QA
Replicate testing in local mobile build.
Note
Ensures registration completes even if the app is closed after the second auth, and tightens when the UI indicates it's safe to close.
pendingRegistrationServiceto poll db-relayer via WebSocket with retries, finalizing registration viaupdateDocumentRegistrationStateand clearing state on success/failureuuid,documentId,isMock, timestamp) insettingStorewithsetPendingRegistration/clearPendingRegistrationPROVING_BEGIN_GENERATION(register circuit) and clears it onPROVING_ACCOUNT_VERIFIED_SUCCESSorPROVING_REGISTER_ERROR_OR_FAILUREAppStateandcheckPendingRegistrationWithRetrycanCloseAppwhen inproving/post_proving/completedANDcircuitType === 'register'Written by Cursor Bugbot for commit 426e975. This will update automatically on new commits. Configure here.
Summary by CodeRabbit
Release Notes
✏️ Tip: You can customize this high-level summary in your review settings.