-
Notifications
You must be signed in to change notification settings - Fork 5
feat: health check #437
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: health check #437
Conversation
|
Warning Rate limit exceeded@coodos has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 10 minutes and 22 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (5)
WalkthroughAdds Argon2 browser packages and bumps multiple Tauri JS/Rust dependencies; introduces VaultController.checkHealth with timeouts and 404-as-deleted semantics; exposes SecurityController.setOnboardingPin; makes Drawer dismissibility configurable; integrates eVault health checks into login to show a non-dismissible deletion modal and a nukeWallet flow; increments Android versionCode. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant LoginPage as Login Page
participant Vault as VaultController
participant Registry as Registry
participant Modal as Deletion Modal
participant Global as Global State
User->>LoginPage: Enter PIN or use biometric
LoginPage->>Vault: checkHealth(w3id) (3s timeout)
Vault->>Registry: resolve(w3id) (axios 5s timeout)
Registry-->>Vault: response (success / 404 / error)
alt 404 (deleted)
Vault-->>LoginPage: { healthy: false, deleted: true }
LoginPage->>Modal: Open non-dismissible Drawer
User->>Modal: Click "Delete Local Data"
LoginPage->>Global: nukeWallet (clear local + global state)
LoginPage->>LoginPage: Redirect to onboarding
else unhealthy (other error)
Vault-->>LoginPage: { healthy: false, error: "..." }
LoginPage->>LoginPage: Log error, continue to app
else healthy
Vault-->>LoginPage: { healthy: true, uri: "..." }
LoginPage->>LoginPage: Navigate to app
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
infrastructure/eid-wallet/src/routes/(auth)/login/+page.svelte (1)
79-98: Extract duplicated health check logic.The health check logic after successful login is duplicated identically in both the PIN verification path (lines 79-98) and the biometric authentication path (lines 145-164). This violates the DRY principle and makes maintenance harder.
Extract the health check into a shared function:
+async function performHealthCheck() { + try { + const vault = await globalState?.vaultController.vault; + if (vault?.ename) { + const healthCheck = await globalState.vaultController.checkHealth(vault.ename); + if (!healthCheck.healthy) { + console.warn("eVault health check failed:", healthCheck.error); + + // If eVault was deleted (404), show modal + if (healthCheck.deleted) { + isDeletedVaultModalOpen = true; + return false; // Prevent navigation + } + // For other errors, continue to app - non-blocking + } + } + } catch (error) { + console.error("Error during eVault health check:", error); + // Continue to app even if health check fails - non-blocking + } + return true; // Allow navigation +}Then use it in both locations:
if (!check) { isError = true; return; } - // Check eVault health after successful login - try { - const vault = await globalState?.vaultController.vault; - if (vault?.ename) { - const healthCheck = await globalState.vaultController.checkHealth(vault.ename); - if (!healthCheck.healthy) { - console.warn("eVault health check failed:", healthCheck.error); - - // If eVault was deleted (404), show modal - if (healthCheck.deleted) { - isDeletedVaultModalOpen = true; - return; // Don't continue to app - } - // For other errors, continue to app - non-blocking - } - } - } catch (error) { - console.error("Error during eVault health check:", error); - // Continue to app even if health check fails - non-blocking - } + const shouldContinue = await performHealthCheck(); + if (!shouldContinue) return;And similarly in the biometric path.
Also applies to: 145-164
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (8)
infrastructure/eid-wallet/src-tauri/Cargo.lockis excluded by!**/*.lockinfrastructure/eid-wallet/src-tauri/gen/android/.idea/gradle.xmlis excluded by!**/gen/**infrastructure/eid-wallet/src-tauri/gen/android/.idea/kotlinc.xmlis excluded by!**/gen/**infrastructure/eid-wallet/src-tauri/gen/android/app/arm64/release/app-arm64-release.aabis excluded by!**/gen/**infrastructure/eid-wallet/src-tauri/gen/android/app/src/main/AndroidManifest.xmlis excluded by!**/gen/**infrastructure/eid-wallet/src-tauri/gen/android/build.gradle.ktsis excluded by!**/gen/**infrastructure/eid-wallet/src-tauri/gen/android/buildSrc/build.gradle.ktsis excluded by!**/gen/**pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (7)
infrastructure/eid-wallet/package.json(2 hunks)infrastructure/eid-wallet/src-tauri/Cargo.toml(2 hunks)infrastructure/eid-wallet/src-tauri/tauri.conf.json(1 hunks)infrastructure/eid-wallet/src/lib/global/controllers/evault.ts(2 hunks)infrastructure/eid-wallet/src/lib/ui/Drawer/Drawer.svelte(3 hunks)infrastructure/eid-wallet/src/routes/(auth)/login/+page.svelte(6 hunks)platforms/eReputation/client/src/pages/references-page.tsx(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/(auth)/login/+page.svelte
🧬 Code graph analysis (1)
infrastructure/eid-wallet/src/lib/global/controllers/evault.ts (1)
platforms/pictique/src/env.d.ts (1)
PUBLIC_REGISTRY_URL(7-7)
⏰ 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
🔇 Additional comments (12)
platforms/eReputation/client/src/pages/references-page.tsx (1)
459-459: LGTM!Trailing newline added for consistent file formatting.
infrastructure/eid-wallet/src-tauri/tauri.conf.json (2)
21-21: LGTM!Formatting consolidation of the capabilities array maintains the same content.
29-29: Verify the Android versionCode skip from 10 to 12.The versionCode jumped from 10 to 12, skipping 11. While not necessarily problematic, Android versionCodes typically increment sequentially. Ensure this skip is intentional and won't cause issues with app distribution or updates.
infrastructure/eid-wallet/src/lib/ui/Drawer/Drawer.svelte (2)
12-12: LGTM!The new
dismissibleprop with a default oftruemaintains backward compatibility while enabling non-dismissible drawer behavior when needed.Also applies to: 22-22
38-53: LGTM!The drawer UI appropriately adjusts based on dismissibility: stronger backdrop, no bottom-close gesture, no draggable handle, and backdrop taps are blocked when non-dismissible.
infrastructure/eid-wallet/src/lib/global/controllers/evault.ts (2)
130-165: LGTM!The
checkHealthmethod provides a clean health check API with appropriate timeout (3s), handles the deleted state (404) correctly, and returns structured error information for different failure scenarios.
181-183: LGTM!Adding a 5-second timeout to the resolve endpoint call improves resilience against slow or unresponsive registry services.
infrastructure/eid-wallet/src/routes/(auth)/login/+page.svelte (3)
36-37: LGTM!Context accessors retrieved for global state management in the vault reset flow.
39-46: LGTM!The
nukeWalletfunction properly resets the global state, closes the modal, and navigates to onboarding.
248-278: LGTM!The non-dismissible modal with clear deletion guidance provides appropriate UX for the critical state where the vault has been deleted from the registry.
infrastructure/eid-wallet/src-tauri/Cargo.toml (1)
27-27: Verification passed: both Tauri plugin versions are valid and secure.Both
tauri-plugin-store 2.4.1andtauri-plugin-biometric 2.3.2exist on crates.io and have no known security vulnerabilities.infrastructure/eid-wallet/package.json (1)
28-34: Now I need to check if the eid-wallet project has a Vite configuration with the insecure pattern:Remove security advisory verification—not applicable to this package version update.
The flagged advisory documents a commonly used misconfiguration in Tauri documentation's insecure Vite example configuration, not a vulnerability in Tauri code itself. The issue occurs only with the deprecated pattern
envPrefix: ['VITE_', 'TAURI_']in vite.config.ts. The correct configuration usesenvPrefix: ['VITE_']with desired TAURI variables manually added.The advisory notes that values are automatically bundled into debug builds, but production builds are not embedded as Vite statically replaces these values, reducing the amount of affected applications to a very small amount. Critically, the root cause is not in Tauri itself, so the issue is not fixed by updating Tauri.
The version @tauri-apps/cli@2.9.4 is beyond any affected version ranges. This advisory documents a configuration best practice, not a version-specific issue. If eid-wallet has the insecure Vite pattern, that's a separate configuration concern for vite.config.ts, independent of the @tauri-apps/cli version.
Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 4
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (2)
infrastructure/eid-wallet/package.json(2 hunks)infrastructure/eid-wallet/src/lib/global/controllers/security.ts(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- infrastructure/eid-wallet/package.json
⏰ 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
🔇 Additional comments (1)
infrastructure/eid-wallet/src/lib/global/controllers/security.ts (1)
48-49: Consider security implications of moving hashing to the frontend.Moving PIN hashing from the Rust backend to the JavaScript frontend reduces the security posture since frontend code is easier to inspect, debug, and potentially extract data from memory. While this may be acceptable for local app authentication (vs. network credentials), ensure this aligns with your security requirements and threat model.
Also applies to: 94-95, 129-133
infrastructure/eid-wallet/src/lib/global/controllers/security.ts
Outdated
Show resolved
Hide resolved
infrastructure/eid-wallet/src/lib/global/controllers/security.ts
Outdated
Show resolved
Hide resolved
infrastructure/eid-wallet/src/lib/global/controllers/security.ts
Outdated
Show resolved
Hide resolved
infrastructure/eid-wallet/src/lib/global/controllers/security.ts
Outdated
Show resolved
Hide resolved
268e468 to
1d6ee27
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
infrastructure/eid-wallet/src/lib/global/controllers/security.ts(10 hunks)infrastructure/eid-wallet/src/routes/(auth)/register/+page.svelte(2 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/lib/global/controllers/security.ts
⏰ 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). (3)
- GitHub Check: build
- GitHub Check: lint
- GitHub Check: lint
🔇 Additional comments (3)
infrastructure/eid-wallet/src/lib/global/controllers/security.ts (1)
5-5: LGTM!Author tag updates across methods are correctly updated to reflect the new organization domain.
Also applies to: 32-32, 54-54, 69-69, 79-79, 96-96, 160-160, 176-176, 201-201, 221-221
infrastructure/eid-wallet/src/routes/(auth)/register/+page.svelte (2)
77-83: LGTM!The try/catch wrapper around
checkStatus()provides graceful error handling. SettingisBiometricsAvailableto false on failure ensures the UI remains functional even if biometric detection fails, which is appropriate defensive programming.
99-99: LGTM!Using
setOnboardingPininstead ofupdatePinis semantically clearer for the onboarding flow and correctly reflects the intent of this page. The error handling (lines 101-104) appropriately catches and logs any failures.
| /** | ||
| * @author SoSweetHam <soham@auvo.io> | ||
| * @author SoSweetHam <soham@ensombl.io> | ||
| * @memberof SecurityController | ||
| * @description Set the onboarding pin for the app | ||
| * @param pin - The pin in plain text | ||
| * @param confirmPin - The confirm pin in plain text | ||
| * @returns void | ||
| * @throws Error if the pins are not the same | ||
| * @example | ||
| * ```ts | ||
| * const globalState = await GlobalState.create(); | ||
| * globalState.securityController.setOnboardingPin("1234", "1234"); | ||
| * ``` | ||
| */ | ||
| async setOnboardingPin(pin: string, confirmPin: string) { | ||
| if (pin !== confirmPin) { | ||
| throw new Error("Pins are not the same!"); | ||
| } | ||
| return await this.#setPin(pin); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add guard to prevent PIN overwrites outside onboarding.
The setOnboardingPin method is public and doesn't verify that no PIN currently exists. This creates a security risk: if called after onboarding, it would overwrite the existing PIN without requiring the old PIN, bypassing the protection in updatePin (lines 124-135).
Apply this diff to add a guard:
async setOnboardingPin(pin: string, confirmPin: string) {
+ const existingPin = await this.#getPin();
+ if (existingPin) {
+ throw new Error("PIN already set. Use updatePin to change it.");
+ }
if (pin !== confirmPin) {
throw new Error("Pins are not the same!");
}
return await this.#setPin(pin);
}Alternatively, if routing guards already prevent access to the register page after onboarding, document this assumption clearly in the method's JSDoc to prevent misuse.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
in infrastructure/eid-wallet/src/lib/global/controllers/security.ts around lines
138 to 157, the public setOnboardingPin currently overwrites any existing PIN;
add a guard that first checks whether a PIN already exists (via the controller's
PIN storage accessor or existing internal getter) and if one exists throw an
error (e.g., "Onboarding PIN cannot overwrite existing PIN") so setOnboardingPin
only sets when no PIN is present; alternatively, if the app routing guarantees
this method is only called during onboarding, add a clear JSDoc note stating
that precondition instead of changing behavior.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nack, you know there reasons why we had to implement it this way because an amazing OS named android decided that it might be a fun idea to keep stale files around, so this causes some phones to have an error with onboarding because android, being the amazing OS it is just messes everything up.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@coodos, thanks for the explanation! I understand now—this is a deliberate workaround for Android's storage cleanup issues where stale files can persist and cause onboarding errors. The ability to overwrite is necessary to handle those edge cases.
If it's not already captured in the code, you might consider adding a brief comment explaining this Android-specific behavior near the method to help future maintainers understand why the guard isn't present. But I'll leave that to your discretion.
✏️ Learnings added
Learnt from: coodos
Repo: MetaState-Prototype-Project/prototype PR: 437
File: infrastructure/eid-wallet/src/lib/global/controllers/security.ts:138-157
Timestamp: 2025-11-14T17:54:47.711Z
Learning: The `setOnboardingPin` method in `infrastructure/eid-wallet/src/lib/global/controllers/security.ts` is intentionally designed to allow overwriting existing PINs without checking if a PIN already exists. This is a workaround for Android keeping stale files around during app reinstallation or onboarding, which causes errors on some devices. Do not suggest adding guards to prevent PIN overwrites in this method.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: sosweetham
Repo: MetaState-Prototype-Project/prototype PR: 97
File: infrastructure/eid-wallet/src/lib/global/controllers/security.ts:86-93
Timestamp: 2025-04-24T06:00:34.296Z
Learning: Rate limiting for PIN verification is not part of the design requirements for the SecurityController in the eid-wallet application, and should not be suggested as an enhancement.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🧹 Nitpick comments (3)
infrastructure/eid-wallet/src/routes/(auth)/login/+page.svelte (1)
36-50: Consolidate duplicategetContextcalls.The
getContext("globalState")call appears twice: once at line 36 and again at line 50 insideonMount. Consider consolidating these to avoid redundant context lookups.Apply this diff to consolidate:
-const getGlobalState = getContext<() => GlobalState>("globalState"); const setGlobalState = getContext<(value: GlobalState) => void>("setGlobalState"); // ... nukeWallet function ... onMount(async () => { - globalState = getContext<() => GlobalState>("globalState")(); + globalState = getGlobalState();infrastructure/eid-wallet/src/lib/global/controllers/evault.ts (2)
149-155: Consider edge case: what ifresponse.dataexists buturiis explicitlynullor empty string?Line 149 checks
if (response.data?.uri), which will returnfalsefor empty strings or other falsy values. While this is likely intentional, it means an empty URI from the registry would be treated as unhealthy rather than invalid.If the registry could return
{ uri: "" }or{ uri: null }, you might want more explicit validation.- if (response.data?.uri) { + if (response.data?.uri && typeof response.data.uri === 'string' && response.data.uri.length > 0) { console.log(`✅ eVault is healthy, URI: ${response.data.uri}`); return { healthy: true, uri: response.data.uri }; } else { console.warn(`⚠️ Registry responded but no URI found`); - return { healthy: false, error: "No URI in registry response" }; + return { + healthy: false, + error: response.data?.uri === null + ? "Registry returned null URI" + : "No URI in registry response" + }; }
130-174: Timeout difference is intentional; optional refactor to extract constants recommended.The 3-second timeout for
checkHealthversus 5-second forresolveEndpointis intentional and follows a consistent pattern across the codebase: health checks use shorter timeouts to fail fast for diagnostics, while regular registry operations use 5 seconds. Other registry operations in the codebase (GraphQL server, maintenance banner) also use 5-second timeouts, confirming this is a deliberate strategy.Extracting these timeout values as named constants (as suggested in the diff) would improve maintainability without changing behavior.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
infrastructure/eid-wallet/src/lib/global/controllers/evault.ts(2 hunks)infrastructure/eid-wallet/src/routes/(auth)/login/+page.svelte(6 hunks)infrastructure/eid-wallet/src/routes/(auth)/register/+page.svelte(2 hunks)platforms/blabsy/src/components/chat/chat-window.tsx(1 hunks)platforms/pictique/src/routes/(protected)/messages/[id]/+page.svelte(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- platforms/pictique/src/routes/(protected)/messages/[id]/+page.svelte
- platforms/blabsy/src/components/chat/chat-window.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- infrastructure/eid-wallet/src/routes/(auth)/register/+page.svelte
🧰 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/(auth)/login/+page.svelte
🧬 Code graph analysis (1)
infrastructure/eid-wallet/src/lib/global/controllers/evault.ts (1)
platforms/pictique/src/env.d.ts (1)
PUBLIC_REGISTRY_URL(7-7)
⏰ 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
🔇 Additional comments (3)
infrastructure/eid-wallet/src/routes/(auth)/login/+page.svelte (2)
40-47: Early return innukeWalletis safe.The early return at line 41 correctly guards against undefined
globalState. This prevents potential runtime errors during wallet reset.
261-291: Non-dismissible modal correctly enforces critical user action.The non-dismissible Drawer (
dismissible={false}) appropriately forces the user to acknowledge and act on the deleted eVault state. This prevents users from being stuck in an inconsistent state where the backend vault is deleted but local data remains.infrastructure/eid-wallet/src/lib/global/controllers/evault.ts (1)
134-174: Health check implementation looks solid.The
checkHealthmethod correctly:
- Implements a reasonable timeout for fast failure
- Distinguishes between 404 (deleted) and other errors
- Returns a well-structured response object
- Uses appropriate logging with emoji indicators for easy scanning
- Treats health checks as non-blocking for non-404 errors (as consumed by the login flow)
|
Dear Future Developer, Archaeologist, or Poor Soul Reading This, Let me tell you a bedtime story about ANDROID — that majestic, chaos-powered But no. No no no. Android, in its infinite cosmic wisdom, decided: You uninstall the app? Android: “Cute. I’ll pretend I didn’t see that.” And so, fast-forward to 2025 where I'm here implementing setOnboardingPin, Yes, rabbit. Yes. I HAD a whole system for that. I built updatePin BUT THEN ANDROID, THE DIGITAL POSSUM LIVING IN MY WALLS, Meaning: So yeah, CodeRabbit, my dude, my chrome-plated Peter Cottontail, tl;dr — This method stays as-is because the OS is a feral raccoon, P.S. — Dear Android: P.P.S. — I’m tired. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
♻️ Duplicate comments (1)
infrastructure/eid-wallet/src/routes/(auth)/login/+page.svelte (1)
80-105: Extract duplicated health check logic into a reusable helper.The eVault health check logic at lines 80-105 (PIN flow) and lines 157-181 (biometric flow) is nearly identical. Extracting this into a helper function would reduce maintenance burden and prevent inconsistencies.
Also applies to: 157-181
🧹 Nitpick comments (2)
infrastructure/eid-wallet/src/routes/(auth)/login/+page.svelte (2)
102-105: Consider user feedback for non-blocking health check failures.When the health check fails with a non-404 error, the code silently continues to the app. Users have no indication their eVault might be unhealthy until they attempt to use it. Consider displaying a non-intrusive warning (toast or banner) to inform users of the health check failure.
Also applies to: 178-181
269-301: Consider adding a support contact option in the deletion modal.The non-dismissible modal forces users to delete their local data with no alternative. Users might want to contact support before taking this irreversible action, especially if the vault deletion was unexpected or might be recoverable.
Consider adding a "Contact Support" button or help text:
<Drawer bind:isPaneOpen={isDeletedVaultModalOpen} dismissible={false}> <div class="text-center"> <h4 class="mt-[2.3svh] mb-[0.5svh] text-red-600"> 🗑️ eVault Has Been Deleted </h4> <p class="text-black-700 mb-4"> Your eVault has been deleted from the registry and is no longer accessible. </p> + <div class="bg-blue-50 border border-blue-200 rounded-md p-4 mb-4"> + <p class="text-blue-800"> + <strong>Need help?</strong> Contact support at support@example.com + before deleting your data. + </p> + </div> <div class="bg-red-50 border border-red-200 rounded-md p-4 mb-6"> <p class="text-red-800 font-medium"> To continue using the app, you need to delete your local account data and start fresh. </p> </div> <!-- ... rest of modal ... --> </div> </Drawer>
📜 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/lib/ui/Drawer/Drawer.svelte(1 hunks)infrastructure/eid-wallet/src/routes/(auth)/login/+page.svelte(2 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-11-14T17:54:47.711Z
Learnt from: coodos
Repo: MetaState-Prototype-Project/prototype PR: 437
File: infrastructure/eid-wallet/src/lib/global/controllers/security.ts:138-157
Timestamp: 2025-11-14T17:54:47.711Z
Learning: The `setOnboardingPin` method in `infrastructure/eid-wallet/src/lib/global/controllers/security.ts` is intentionally designed to allow overwriting existing PINs without checking if a PIN already exists. This is a workaround for Android keeping stale files around during app reinstallation or onboarding, which causes errors on some devices. Do not suggest adding guards to prevent PIN overwrites in this method.
Applied to files:
infrastructure/eid-wallet/src/routes/(auth)/login/+page.svelte
📚 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/(auth)/login/+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
🔇 Additional comments (3)
infrastructure/eid-wallet/src/lib/ui/Drawer/Drawer.svelte (1)
8-48: LGTM! Dismissible prop is well-integrated.The new
dismissibleprop is properly typed, defaults totruefor backward compatibility, and logically controls the pane's behavior (backdrop opacity, bottom-close, and draggable handle). The higher backdrop opacity (0.8) for non-dismissible modals effectively emphasizes their importance.infrastructure/eid-wallet/src/routes/(auth)/login/+page.svelte (2)
14-38: LGTM! State and context setup is clean.The state declarations, biometric authentication options, and global state context retrieval are properly structured and appropriate for the login flow.
40-47: LGTM! Wallet reset logic is sound.The
nukeWalletfunction correctly resets the global state, updates the context, closes the modal, and navigates to onboarding in the proper sequence.
Description of change
Type of change
How the change has been tested
Change checklist
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Chores