Fix/wallet restoration not syncing to graphql due to messy state #432
Conversation
WalkthroughAdds public async clear() methods to VaultController, KeyService, SecurityController, and UserController; introduces GlobalState.reset() to call those clears and remove init/onboarding flags; updates settings UI to asynchronously call reset and added a context setter to expose setGlobalState in layout. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant Settings as Settings Page
participant Layout as App Layout
participant GlobalState
participant Controllers as Controllers
participant Store as Internal Store
User->>Settings: click "nuke wallet"
Settings->>GlobalState: call nukeWallet() → await reset()
GlobalState->>Controllers: call clear() on Vault, Key, Security, User
Controllers->>Store: delete respective keys / clear caches
Store-->>Controllers: deletions acknowledged
Controllers-->>GlobalState: clear() complete
GlobalState->>Store: delete initialized / onboarding flags
Store-->>GlobalState: deletions acknowledged
GlobalState-->>Layout: return new GlobalState instance
Layout->>Settings: setGlobalState(newState)
Settings->>Settings: navigate to /onboarding
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ 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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
infrastructure/eid-wallet/src/lib/global/controllers/key.ts (1)
54-60: Code duplication:reset()andclear()methods are nearly identical.Both
reset()(lines 54-60) and the newly addedclear()(lines 216-222) perform the same operations. This duplication increases maintenance burden. Consider whether both methods are needed, or if one should delegate to the other.If both methods are intended to remain, consider this pattern:
async reset(): Promise<void> { + await this.clear(); + } + + async clear() { this.#managerCache.clear(); this.#contexts.clear(); await this.#store.delete(CONTEXTS_KEY); await this.#store.delete(READY_KEY); this.#ready = false; } - - async clear() { - this.#managerCache.clear(); - this.#contexts.clear(); - this.#store.delete(CONTEXTS_KEY); - this.#store.delete(READY_KEY); - this.#ready = false; - }Also applies to: 216-222
🧹 Nitpick comments (1)
infrastructure/eid-wallet/src/lib/global/controllers/evault.ts (1)
372-374: Consider resetting internal state fields.The
clear()method only deletes the store entry but doesn't reset the internal state fields#client,#endpoint, and#profileCreationStatus. If the VaultController instance is reused after clearing (which happens in the reset flow), these stale values could cause unexpected behavior.Apply this diff to reset internal state:
async clear() { + this.#client = null; + this.#endpoint = null; + this.#profileCreationStatus = "idle"; await this.#store.delete("vault"); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
infrastructure/eid-wallet/src/lib/global/controllers/evault.ts(1 hunks)infrastructure/eid-wallet/src/lib/global/controllers/key.ts(1 hunks)infrastructure/eid-wallet/src/lib/global/controllers/security.ts(1 hunks)infrastructure/eid-wallet/src/lib/global/controllers/user.ts(1 hunks)infrastructure/eid-wallet/src/lib/global/state.ts(1 hunks)infrastructure/eid-wallet/src/routes/(app)/settings/+page.svelte(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-04-24T05:54:12.858Z
Learnt from: sosweetham
Repo: MetaState-Prototype-Project/prototype PR: 97
File: infrastructure/eid-wallet/src/routes/+page.svelte:31-35
Timestamp: 2025-04-24T05:54:12.858Z
Learning: The "Clear PIN" button on the root page of the eID wallet application is intended for debugging purposes only, not for production use, so it doesn't require confirmation dialogs or detailed user guidance.
Applied to files:
infrastructure/eid-wallet/src/routes/(app)/settings/+page.svelte
🔇 Additional comments (2)
infrastructure/eid-wallet/src/lib/global/controllers/security.ts (1)
220-223: LGTM!The
clear()method correctly deletes both security-related store entries with proper async/await handling.infrastructure/eid-wallet/src/lib/global/controllers/user.ts (1)
147-151: LGTM!The
clear()method correctly deletes all user-related store entries with proper async/await handling for each operation.
| async reset() { | ||
| await this.securityController.clear(); | ||
| await this.userController.clear(); | ||
| await this.vaultController.clear(); | ||
| await this.keyService.clear(); | ||
| await this.#store.delete("initialized"); | ||
| await this.#store.delete("isOnboardingComplete"); | ||
| } |
There was a problem hiding this comment.
Consider adding error handling to prevent partial reset state.
The reset() method correctly orchestrates clearing all controllers sequentially. However, if any clear() operation fails midway, the application could be left in an inconsistent state with some data cleared and others not. Consider wrapping the operations in a try-catch block or implementing a rollback mechanism.
Example with error handling:
async reset() {
+ try {
await this.securityController.clear();
await this.userController.clear();
await this.vaultController.clear();
await this.keyService.clear();
await this.#store.delete("initialized");
await this.#store.delete("isOnboardingComplete");
+ } catch (error) {
+ console.error("Failed to reset global state:", error);
+ throw new Error("Incomplete reset - application may be in inconsistent state");
+ }
}📝 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.
| async reset() { | |
| await this.securityController.clear(); | |
| await this.userController.clear(); | |
| await this.vaultController.clear(); | |
| await this.keyService.clear(); | |
| await this.#store.delete("initialized"); | |
| await this.#store.delete("isOnboardingComplete"); | |
| } | |
| async reset() { | |
| try { | |
| await this.securityController.clear(); | |
| await this.userController.clear(); | |
| await this.vaultController.clear(); | |
| await this.keyService.clear(); | |
| await this.#store.delete("initialized"); | |
| await this.#store.delete("isOnboardingComplete"); | |
| } catch (error) { | |
| console.error("Failed to reset global state:", error); | |
| throw new Error("Incomplete reset - application may be in inconsistent state"); | |
| } | |
| } |
🤖 Prompt for AI Agents
In infrastructure/eid-wallet/src/lib/global/state.ts around lines 113-120, the
reset() sequence can leave the app in a partially-reset state if any clear()
fails; wrap the sequence in a try/catch, take a lightweight snapshot/backup of
each controller/store state before performing clears, perform the clears
sequentially inside the try, and if any operation fails run a rollback that
restores controllers/stores from the snapshot, log the original error and
rethrow or return a structured failure; ensure snapshot cleanup on success and
make the function return/throw a clear error so callers can act.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
infrastructure/eid-wallet/src/lib/global/controllers/key.ts (1)
216-222: Consider removing duplicate logic betweenclear()andreset()methods.The
clear()method (lines 216-222) is identical to the existingreset()method (lines 54-60). Both perform the same operations in the same order. This duplication creates maintenance overhead and API confusion.Consider one of these approaches:
- If
clear()is the new standard API across controllers, deprecate or removereset()and migrate callers toclear()- If both methods serve different purposes, add documentation explaining when to use each
- If
reset()should remain for backward compatibility, haveclear()delegate toreset()(or vice versa)Additionally, add an explicit return type to
clear()for consistency:-async clear() { +async clear(): Promise<void> {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
infrastructure/eid-wallet/src/lib/global/controllers/key.ts(1 hunks)infrastructure/eid-wallet/src/lib/global/state.ts(1 hunks)infrastructure/eid-wallet/src/routes/(app)/settings/+page.svelte(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- infrastructure/eid-wallet/src/lib/global/state.ts
- infrastructure/eid-wallet/src/routes/(app)/settings/+page.svelte
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
coodos
changed the title
coodos
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
infrastructure/eid-wallet/src/lib/global/state.ts(1 hunks)infrastructure/eid-wallet/src/routes/(app)/settings/+page.svelte(2 hunks)infrastructure/eid-wallet/src/routes/+layout.svelte(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- infrastructure/eid-wallet/src/lib/global/state.ts
- infrastructure/eid-wallet/src/routes/(app)/settings/+page.svelte
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
| setContext("setGlobalState", (value: GlobalState | undefined) => { | ||
| globalState = value; | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
Request verification: Potential race conditions during state reset.
Exposing a setter that allows external components to directly mutate globalState could lead to race conditions if components access globalState while it's being reset to undefined or a new instance. Consider these scenarios:
- Component A reads
globalStatevia the getter context - Component B calls
setGlobalState(undefined)orsetGlobalState(newState) - Component A attempts to access properties on what it thought was a valid
GlobalStateobject
To verify this is safe, run the following script to check how globalState and setGlobalState are used together:
Consider these improvements:
- Add guards: Check if operations are in progress before allowing mutation
- Clarify intent: Rename to
setContext("resetGlobalState", ...)to signal this is for reset operations only - Alternative pattern: Expose a
resetGlobalState()method that encapsulates the full reset logic rather than a generic setter:
-setContext("setGlobalState", (value: GlobalState | undefined) => {
- globalState = value;
-});
+setContext("resetGlobalState", async () => {
+ if (globalState) {
+ await globalState.reset();
+ }
+ globalState = await GlobalState.create();
+});This would provide better encapsulation and prevent misuse while still supporting the wallet reset flow.
🏁 Script executed:
#!/bin/bash
# Check usage patterns of getGlobalState and setGlobalState contexts
echo "=== Finding setGlobalState usage ==="
rg -n --type=ts --type=svelte -C 5 'getContext.*setGlobalState'
echo -e "\n=== Finding globalState getter usage ==="
rg -n --type=ts --type=svelte -C 5 'getContext.*globalState.*\(\)'
echo -e "\n=== Finding reset() calls ==="
rg -n --type=ts --type=svelte -C 5 '\.reset\(\)'Length of output: 202
🏁 Script executed:
#!/bin/bash
# Check usage patterns - corrected for .svelte files
echo "=== Finding setGlobalState usage (no type filter) ==="
rg -n -C 5 'setGlobalState' --type-add 'svelte:*.svelte' --type svelte --type ts
echo -e "\n=== Finding globalState context getter usage ==="
rg -n -C 5 "getContext.*['\"]globalState" --type-add 'svelte:*.svelte' --type svelte --type ts
echo -e "\n=== Finding reset\(\) method calls ==="
rg -n -C 5 '\.reset\s*\(' --type-add 'svelte:*.svelte' --type svelte --type ts
echo -e "\n=== Finding setContext calls in layout ==="
rg -n -C 5 'setContext' infrastructure/eid-wallet/src/routes/+layout.svelteLength of output: 26473
Fix type mismatch in setGlobalState context setter.
The setter's type signature doesn't match its usage. The layout exports (value: GlobalState | undefined), but settings/+page.svelte expects (value: GlobalState). Update the type in settings/+page.svelte line 18 to accept undefined:
const setGlobalState = getContext<(value: GlobalState | undefined) => void>("setGlobalState");Regarding the race condition concern: the single-threaded nature of JavaScript and immediate navigation after reset (line 43) significantly reduce practical risk. Components holding stale references won't run after the navigation occurs. However, for clarity and future maintainability, consider renaming this to resetGlobalState() to signal its specific purpose rather than a generic state setter.
🤖 Prompt for AI Agents
in infrastructure/eid-wallet/src/routes/+layout.svelte lines 23-25 and
settings/+page.svelte line 18, the setGlobalState context setter is declared to
accept GlobalState | undefined but settings/+page.svelte types it as (value:
GlobalState) — update the getContext call in settings/+page.svelte to use
(value: GlobalState | undefined) => void so the types match; optionally consider
renaming the setter to resetGlobalState if its intent is to clear state for
clarity.
Description of change
Fixes delete account logic to properly reset application state to help start new onboarding properly
Issue Number
Type of change
How the change has been tested
Manually
Change checklist
Summary by CodeRabbit