Fix/UI ux improvements eid wallet #584
Conversation
WalkthroughBumps eID Wallet to v0.5.0, adds a Toast component and an eName copy action, queues/guards deep-link processing until app readiness, hardens scan‑QR deep‑link parsing/handling (auth/sign/reveal/poll/blind‑vote), and adjusts several auth UI flows and deeplink-login fallbacks. Changes
Sequence Diagram(s)sequenceDiagram
participant OS as External deep link
participant App as App Entrypoint (Tauri/Browser)
participant Layout as +layout.svelte
participant Storage as sessionStorage
participant Scan as scan-qr Page
participant Logic as scanLogic.ts
participant UI as Drawers / User UI
OS->>App: open deep link (payload)
App->>Layout: emit deep-link event
Layout->>Layout: if !isAppReady -> enqueue pendingDeepLinks
Layout->>Storage: persist pendingDeepLinks
alt app not ready
Layout-->>App: ack queued
else app ready
Layout->>Scan: dispatch deep-link event
Scan->>Logic: handleDeepLinkData(payload)
Logic->>Logic: guard duplicate in-flight requests
alt auth flow
Logic->>Logic: parse host/session/redirect -> set auth drawer/state
else sign flow
Logic->>Logic: validate/resolve platform_url -> fetch poll -> set signing state
else reveal flow
Logic->>Logic: validate pollId -> set reveal state
end
Logic-->>Scan: open drawer / update UI
Scan-->>UI: render drawer and user interactions
end
Note right of Layout: when isAppReady becomes true, pendingDeepLinks are processed
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings, 1 inconclusive)
✨ 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: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
platforms/blabsy/src/pages/deeplink-login.tsx (1)
111-117: Stricter response validation may require coordinated backend changesRequiring both
data.tokenanddata.useris a good safety check, but it’s a behavior change: any auth endpoint that previously returned only a token will now surfaceInvalid response from serverand block login. Please confirm that all relevant backends already includeuser, or consider temporarily accepting responses with justtoken(while logging or tracking missinguser) to avoid breaking existing deeplinks during rollout.infrastructure/eid-wallet/src/routes/(auth)/verify/+page.svelte (1)
438-456: Clean up CTA button markup and stray>There is an extra
>rendered after the Back button and the button markup is a bit cramped, which can lead to visual glitches.- <div class="flex w-full items-center gap-3 pt-4"> - {#if $status !== "declined"} - <ButtonAction - variant="soft" - class="flex-1" - callback={() => goto("/onboarding")}>Back</ButtonAction - > - <ButtonAction - class="flex-1" - callback={handleContinue} - color="primary" - >{$status === "approved" - ? "Continue" - : $status === "duplicate" - ? "Claim old eVault" - : "Retry"}</ButtonAction - > - {/if} - </div> + <div class="flex w-full items-center gap-3 pt-4"> + {#if $status !== "declined"} + <ButtonAction + variant="soft" + class="flex-1" + callback={() => goto("/onboarding")} + > + Back + </ButtonAction> + <ButtonAction + class="flex-1" + callback={handleContinue} + color="primary" + > + {$status === "approved" + ? "Continue" + : $status === "duplicate" + ? "Claim old eVault" + : "Retry"} + </ButtonAction> + {/if} + </div>
🧹 Nitpick comments (13)
platforms/group-charter-manager/src/app/deeplink-login/page.tsx (2)
47-63: Consider consolidating redundant parsing attempts.The fallback logic (lines 49-56) re-parses
window.location.searchandwindow.location.hash, but lines 13-38 already extract parameters from these same sources. Both attempts parse identical inputs, creating duplication.While the defensive approach adds resilience, it increases maintenance complexity. Consider refactoring to a single, more robust parsing pass.
Example consolidation:
- // Try parsing from search string first - let params: URLSearchParams; - let searchString = window.location.search; - - // If search is empty, try parsing from hash or full URL - if (!searchString || searchString === '') { - const hash = window.location.hash; - if (hash && hash.includes('?')) { - searchString = hash.substring(hash.indexOf('?')); - } else { - try { - const fullUrl = new URL(window.location.href); - searchString = fullUrl.search; - } catch (e) { - // Ignore parsing errors - } - } - } - - // Remove leading ? if present - if (searchString.startsWith('?')) { - searchString = searchString.substring(1); - } - - // Parse the search string - params = new URLSearchParams(searchString); - - let ename = params.get('ename'); - let session = params.get('session'); - let signature = params.get('signature'); - const appVersion = params.get('appVersion'); - - // Only set error after all parsing attempts have failed - // Try additional parsing methods if params are missing - if (!ename || !session || !signature) { - // Try one more time with window.location directly - const directParams = new URLSearchParams(window.location.search); - const hashParams = window.location.hash.includes('?') - ? new URLSearchParams(window.location.hash.substring(window.location.hash.indexOf('?') + 1)) - : null; - - ename = ename || directParams.get('ename') || hashParams?.get('ename') || null; - session = session || directParams.get('session') || hashParams?.get('session') || null; - signature = signature || directParams.get('signature') || hashParams?.get('signature') || null; - - if (!ename || !session || !signature) { - setError("Missing required authentication parameters"); - setIsLoading(false); - return; - } - } + // Parse from search and hash in a single pass + const searchParams = new URLSearchParams(window.location.search); + const hashParams = window.location.hash.includes('?') + ? new URLSearchParams(window.location.hash.substring(window.location.hash.indexOf('?') + 1)) + : new URLSearchParams(); + + const ename = searchParams.get('ename') || hashParams.get('ename'); + const session = searchParams.get('session') || hashParams.get('session'); + const signature = searchParams.get('signature') || hashParams.get('signature'); + const appVersion = searchParams.get('appVersion') || hashParams.get('appVersion'); + + if (!ename || !session || !signature) { + setError("Missing required authentication parameters"); + setIsLoading(false); + return; + }
54-56: Optional: Trailing|| nullis redundant.The
|| nullat the end of each fallback chain is technically unnecessary—if all previous values are falsy, the result will already benullorundefined. However, it does make the intent explicit, which may aid readability.platforms/eReputation/client/src/pages/deeplink-login.tsx (1)
39-61: Robust deeplink param fallback; consider tiny polish on defaults/duplicationThe new multi-step parsing (initial
params, thendirectParams+hashParams) is sound and should make deeplink handling much more resilient. Optional follow‑ups:
- You construct
URLSearchParamsfromwindow.location.searchtwice (initialparamsviasearchString, thendirectParams). You could slightly simplify by reusingparamsas one of the fallback sources, but this is purely cosmetic.- Given the PR bumps the app version to
0.5.0, you may want to align the default here:- const requestBody = { ename, session, signature, appVersion: appVersion || '0.4.0' }; + const requestBody = { ename, session, signature, appVersion: appVersion || '0.5.0' };If the
'0.4.0'default is intentional for backward-compat behavior, keeping it as-is is fine.infrastructure/eid-wallet/src/routes/(app)/scan-qr/scanLogic.ts (1)
1235-1248: Consider validating platform URL before opening the drawer.The drawer is opened at lines 1237 before the platform URL validation at lines 1243-1248. If validation fails, users may briefly see the drawer open before an error state. Moving the UI state changes after validation would provide a smoother experience.
- isBlindVotingRequest.set(true); - selectedBlindVoteOption.set(null); - signingDrawerOpen.set(true); - blindVoteError.set(null); - const platformUrlCandidate = parsedSigningData?.platformUrl?.trim(); if (!platformUrlCandidate) { const errorMessage = "Missing platform URL in signing data and no PUBLIC_PLATFORM_URL configured."; blindVoteError.set(errorMessage); throw new Error(errorMessage); } let platformUrl: string; try { // Validate the URL and normalize trailing slashes for consistent API calls. const validatedUrl = new URL( platformUrlCandidate, ); platformUrl = validatedUrl .toString() .replace(/\/+$/, ""); } catch (error) { const message = error instanceof Error ? error.message : String(error); const errorMessage = `Invalid platform URL "${platformUrlCandidate}": ${message}`; blindVoteError.set( "Invalid platform URL in signing request.", ); throw new Error(errorMessage); } + + // Open drawer only after validation succeeds + isBlindVotingRequest.set(true); + selectedBlindVoteOption.set(null); + blindVoteError.set(null); + signingDrawerOpen.set(true);platforms/blabsy/src/pages/deeplink-login.tsx (1)
41-82: Robust multi-source param parsing; consider consolidating to reduce duplicationThe fallback logic makes the deeplink handling much more resilient, but it now parses
window.location.search/hashmultiple times (once viasearchString→params, then again asdirectParams/hashParams). Extracting a small helper (e.g.getAuthParam(name)that tries search + hash once) or reusing the existingparamswhere possible would simplify the flow and make future debugging/maintenance easier without changing behavior.infrastructure/eid-wallet/src/routes/(app)/settings/+page.svelte (1)
143-143: Version string format inconsistency.The version displays as
v0.5.0.0(4-part) whilepackage.jsonandtauri.conf.jsonuse0.5.0(3-part semver). Consider aligning for consistency:- Version v0.5.0.0 + Version v0.5.0infrastructure/eid-wallet/src/routes/(auth)/register/+page.svelte (1)
6-11: Remove unused imports.
Buttonnamespace andArrowLeft01Iconare imported but not used in this file.-import * as Button from "$lib/ui/Button"; -import { - ArrowLeft01Icon, - CircleLock01Icon, - FaceIdIcon, -} from "@hugeicons/core-free-icons"; +import { CircleLock01Icon, FaceIdIcon } from "@hugeicons/core-free-icons";infrastructure/eid-wallet/src/routes/+layout.svelte (2)
237-269: Extract globalState readiness wait logic to reduce duplication.The retry loop pattern (wait for globalState with polling) is duplicated three times for
auth,sign, andrevealactions. Consider extracting it:async function waitForGlobalState(maxRetries = 10, delayMs = 100): Promise<GlobalState | null> { let retries = 0; while (!globalState && retries < maxRetries) { await new Promise(resolve => setTimeout(resolve, delayMs)); retries++; } return globalState ?? null; }This would simplify each
checkAuthfunction significantly.
192-325: Significant code duplication across deep link action handlers.The
checkAuthlogic forauth(lines 217-320),sign(lines 351-454), andreveal(lines 474-577) actions are nearly identical (~100 lines each). They all:
- Check
isOnAuthenticatedRoute- Wait for
globalState- Check vault authentication
- Dispatch events and navigate
Consider refactoring into a shared helper function that accepts the
deepLinkDataand action-specific parameters. This would reduce ~200 lines of duplicate code and make maintenance easier.infrastructure/eid-wallet/src/lib/ui/index.ts (1)
5-5: Minor: prefer exporting from the Toast barrel instead of the .svelte fileSince you added
lib/ui/Toast/index.ts, it’s a bit cleaner to export from the directory barrel rather than the component file directly.-export { default as Toast } from "./Toast/Toast.svelte"; +export { default as Toast } from "./Toast";infrastructure/eid-wallet/src/routes/(app)/main/+page.svelte (1)
5-6: Copy-to-clipboard + Toast flow is good; consider handling missing eName explicitlyThe
copyEName+ Toast wiring is correct and handles clipboard failures well. One minor UX gap: ifenameisn’t loaded yet, the handler returns silently and the user gets no feedback.You could surface a short error toast in that case:
async function copyEName() { - if (!ename) return; - try { + if (!ename) { + toastMessage = "eName not available yet"; + showToast = true; + return; + } + try { await navigator.clipboard.writeText(ename); toastMessage = "eName copied to clipboard!"; showToast = true; } catch (error) { console.error("Failed to copy eName:", error); toastMessage = "Failed to copy eName"; showToast = true; } }The rest of the integration (IdentityCard
copyBtnand conditional<Toast>rendering) looks good.Also applies to: 27-29, 35-50, 155-160, 219-221
infrastructure/eid-wallet/src/lib/ui/Toast/Toast.svelte (1)
11-24: Enable actual fade-out animation by removing the{#if isVisible}wrapperRight now
{#if isVisible}removes the DOM node as soon asisVisible = false, so theopacity-0transition never plays and the toast just disappears abruptly.You can keep the fade behavior and still call
onCloseafter 300ms by dropping the{#if}and relying on the opacity class:- {#if isVisible} - <div - {...restProps} - class="fixed bottom-[30px] left-1/2 -translate-x-1/2 z-50 w-[90%] bg-primary-500 text-white px-6 py-3 rounded-lg shadow-lg transition-opacity duration-300 {isVisible - ? 'opacity-100' - : 'opacity-0'}" - role="alert" - > - <div class="flex items-center gap-2"> - <svg - class="w-5 h-5" - fill="none" - stroke="currentColor" - viewBox="0 0 24 24" - > - <path - stroke-linecap="round" - stroke-linejoin="round" - stroke-width="2" - d="M5 13l4 4L19 7" - /> - </svg> - <span class="font-medium">{message}</span> - </div> - </div> -{/if} +<div + {...restProps} + class="fixed bottom-[30px] left-1/2 -translate-x-1/2 z-50 w-[90%] bg-primary-500 text-white px-6 py-3 rounded-lg shadow-lg transition-opacity duration-300 {isVisible + ? 'opacity-100' + : 'opacity-0'}" + role="alert" +> + <div class="flex items-center gap-2"> + <svg + class="w-5 h-5" + fill="none" + stroke="currentColor" + viewBox="0 0 24 24" + > + <path + stroke-linecap="round" + stroke-linejoin="round" + stroke-width="2" + d="M5 13l4 4L19 7" + /> + </svg> + <span class="font-medium">{message}</span> + </div> +</div>With this, the toast fades out over 300ms and then
onClosecan flip the parent’sshowToastto unmount it.Also applies to: 27-52
infrastructure/eid-wallet/src/routes/(auth)/onboarding/+page.svelte (1)
191-199: Pre-verification + demo name flow is sound; you can DRY the readiness checkThe new requirement that both
verificationIdanddemoNamebe present before continuing is correct, and resetting both on failure makes the flow clearer.The readiness condition is currently duplicated:
- In
handleContinue:
- guards on
!verificationId || !demoName || verificationId.length === 0 || demoName.length === 0- In the button props:
variant={verificationId.length === 0 || demoName.length === 0 ? "soft" : "solid"}disabled={verificationId.length === 0 || demoName.length === 0}To avoid future drift, you could centralize this into a helper:
const isPreVerificationReady = () => verificationId.length > 0 && demoName.length > 0; handleContinue = async () => { if (!isPreVerificationReady()) { return; } // ... };<ButtonAction variant={isPreVerificationReady() ? "solid" : "soft"} disabled={!isPreVerificationReady()} class="w-full" callback={handleContinue} > Next </ButtonAction>Functionally you’re in a good place; this just keeps the logic in sync.
Also applies to: 236-239, 362-369, 382-388, 390-393
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
infrastructure/eid-wallet/src-tauri/gen/android/app/arm64/release/app-arm64-release.aabis excluded by!**/gen/**
📒 Files selected for processing (19)
infrastructure/eid-wallet/package.json(1 hunks)infrastructure/eid-wallet/src-tauri/tauri.conf.json(1 hunks)infrastructure/eid-wallet/src/lib/fragments/IdentityCard/IdentityCard.svelte(4 hunks)infrastructure/eid-wallet/src/lib/ui/Toast/Toast.svelte(1 hunks)infrastructure/eid-wallet/src/lib/ui/Toast/index.ts(1 hunks)infrastructure/eid-wallet/src/lib/ui/index.ts(1 hunks)infrastructure/eid-wallet/src/routes/(app)/main/+page.svelte(4 hunks)infrastructure/eid-wallet/src/routes/(app)/scan-qr/+page.svelte(2 hunks)infrastructure/eid-wallet/src/routes/(app)/scan-qr/scanLogic.ts(1 hunks)infrastructure/eid-wallet/src/routes/(app)/settings/+page.svelte(2 hunks)infrastructure/eid-wallet/src/routes/(auth)/e-passport/+page.svelte(2 hunks)infrastructure/eid-wallet/src/routes/(auth)/onboarding/+page.svelte(5 hunks)infrastructure/eid-wallet/src/routes/(auth)/register/+page.svelte(5 hunks)infrastructure/eid-wallet/src/routes/(auth)/verify/+page.svelte(2 hunks)infrastructure/eid-wallet/src/routes/+layout.svelte(11 hunks)infrastructure/eid-wallet/src/routes/+page.svelte(0 hunks)platforms/blabsy/src/pages/deeplink-login.tsx(2 hunks)platforms/eReputation/client/src/pages/deeplink-login.tsx(1 hunks)platforms/group-charter-manager/src/app/deeplink-login/page.tsx(1 hunks)
💤 Files with no reviewable changes (1)
- infrastructure/eid-wallet/src/routes/+page.svelte
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-11-14T17:54:47.719Z
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.719Z
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)/register/+page.svelte
📚 Learning: 2025-11-21T15:49:12.904Z
Learnt from: coodos
Repo: MetaState-Prototype-Project/prototype PR: 458
File: platforms/eReputation-api/src/services/ReferenceSigningSessionService.ts:60-60
Timestamp: 2025-11-21T15:49:12.904Z
Learning: In platforms/eReputation-api/src/services/ReferenceSigningSessionService.ts, the VITE_EREPUTATION_BASE_URL environment variable intentionally has no fallback value. The service should fail if this environment variable is not set, following a fail-fast design pattern.
Applied to files:
platforms/eReputation/client/src/pages/deeplink-login.tsx
⏰ 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 (18)
platforms/group-charter-manager/src/app/deeplink-login/page.tsx (1)
40-43: LGTM—mutable variables enable fallback parsing.Changing these to
letallows the fallback logic to reassign missing values, which is necessary for the resilient parameter extraction approach.infrastructure/eid-wallet/src/routes/(app)/scan-qr/scanLogic.ts (4)
1154-1160: Good addition of globalState guard.The defensive check prevents processing deep links before the application state is ready.
1162-1179: Good safeguards against duplicate in-flight requests.The guards properly check for active drawers before processing. Note there's a theoretical race condition if two identical deep links arrive simultaneously before the drawer state is set, but this is an edge case and the current implementation is a reasonable improvement.
1181-1203: LGTM - Auth deep link handling is well structured.Good approach: closing other modals first, handling URL parsing errors gracefully, and marking the request origin.
1324-1345: Good reveal deep link handling with proper validation.The flow correctly validates the pollId and provides user feedback via
revealErrorwhen missing.infrastructure/eid-wallet/src-tauri/tauri.conf.json (1)
4-4: Version bump looks good, but verify Android versionCode.The version is correctly updated to 0.5.0 to match
package.json. However, theandroid.versionCode(line 29) remains at 12. If this release is intended for the Play Store, the versionCode typically needs to be incremented as well.infrastructure/eid-wallet/package.json (1)
3-3: LGTM!Version bump to 0.5.0 is consistent with
tauri.conf.json.infrastructure/eid-wallet/src/routes/(app)/settings/+page.svelte (1)
48-52: LGTM!Making
cancelDeleteasync and navigating to/mainafter closing confirmation dialogs provides a consistent UX flow, ensuring users return to the main screen after cancellation.infrastructure/eid-wallet/src/routes/(app)/scan-qr/+page.svelte (2)
101-105: LGTM!Simplifying the decline handler to always navigate to
/mainprovides a predictable user experience. The previous conditional logic for restarting scanning vs. navigating is removed in favor of a consistent cancel-to-main flow.
122-126: LGTM!Consistent with
handleAuthDrawerDecline- the signing drawer decline now follows the same pattern of closing and navigating to/main.infrastructure/eid-wallet/src/routes/(auth)/register/+page.svelte (3)
152-165: LGTM!The two-button layout (Back + Confirm) improves navigation UX by providing a clear escape path to onboarding. The flex layout with equal-width buttons is consistent with other auth flows in the PR.
221-228: Back button closes drawer but doesn't navigate.The Back button at line 225 sets
showDrawer = falsebut doesn't navigate anywhere. This leaves the user on the same page with the drawer closed but PIN already set. Consider whether this is intentional—if the user closes this drawer, they might be confused about next steps.
273-282: Same concern for biometrics Back button.Similar to the previous drawer, this Back button (line 277) closes the drawer without navigation after biometrics are successfully added. The user would be left seeing the underlying page with their state partially complete. Verify this is the intended behavior.
infrastructure/eid-wallet/src/routes/+layout.svelte (1)
21-22: LGTM!Good addition of app readiness state and deep link queue to prevent processing deep links before the app is fully initialized.
infrastructure/eid-wallet/src/lib/ui/Toast/index.ts (1)
1-1: Toast barrel re-export is fineThe directory-level default re-export is idiomatic and matches how other UI components are structured.
infrastructure/eid-wallet/src/routes/(auth)/verify/+page.svelte (1)
12-16: New imports are consistent with patterns elsewhereAdding the Button namespace and Arrow icon imports aligns with other auth routes and doesn’t affect existing behavior here.
infrastructure/eid-wallet/src/routes/(auth)/e-passport/+page.svelte (1)
7-8: Imports align with shared button/icon usageThe added Button namespace and Arrow icon imports match patterns used in other auth screens; no concerns here.
infrastructure/eid-wallet/src/lib/fragments/IdentityCard/IdentityCard.svelte (1)
6-8: IdentityCard copy button integration looks solidThe new
copyBtnprop andCopy01Iconwiring for the eName variant are consistent with existing Button.Icon usage and cleanly scoped behind{#if copyBtn}so the UI degrades gracefully when no handler is passed.Also applies to: 20-21, 31-32, 74-80
| try { | ||
| const pollResponse = await fetch( | ||
| `${platformUrl}/api/polls/${get(signingData)?.pollId}`, | ||
| ); | ||
| if (pollResponse.ok) { | ||
| const pollDetails = | ||
| await pollResponse.json(); | ||
| signingData.update((prev) => | ||
| prev | ||
| ? { | ||
| ...prev, | ||
| pollDetails, | ||
| } | ||
| : prev, | ||
| ); | ||
| console.log( | ||
| "✅ Poll details fetched:", | ||
| pollDetails, | ||
| ); | ||
| } | ||
| } catch (error) { | ||
| console.error( | ||
| "Failed to fetch poll details:", | ||
| error, | ||
| ); | ||
| blindVoteError.set( | ||
| "Failed to load poll details", | ||
| ); | ||
| } |
There was a problem hiding this comment.
Missing error handling for non-ok poll response.
If pollResponse.ok is false (e.g., 404, 500), no error is set and signingData.pollDetails remains as "Loading poll details..." indefinitely. The catch block only handles network errors, not HTTP error statuses.
try {
const pollResponse = await fetch(
`${platformUrl}/api/polls/${get(signingData)?.pollId}`,
);
if (pollResponse.ok) {
const pollDetails =
await pollResponse.json();
signingData.update((prev) =>
prev
? {
...prev,
pollDetails,
}
: prev,
);
console.log(
"✅ Poll details fetched:",
pollDetails,
);
+ } else {
+ console.error(
+ `Failed to fetch poll details: ${pollResponse.status} ${pollResponse.statusText}`,
+ );
+ blindVoteError.set(
+ "Failed to load poll details. Please try again.",
+ );
}
} catch (error) {🤖 Prompt for AI Agents
in infrastructure/eid-wallet/src/routes/(app)/scan-qr/scanLogic.ts around lines
1283 to 1311: the current code only handles network exceptions but ignores HTTP
error statuses when pollResponse.ok is false, leaving signingData.pollDetails
stuck as "Loading poll details..."; update the logic to handle non-ok responses
by reading the response body (if available) or response.status/text, log a
descriptive error including status and body, and set blindVoteError to a clear
message (e.g., "Failed to load poll details: <status or message>");
additionally, ensure signingData is updated to a final state (null or an error
marker) instead of leaving the "Loading..." placeholder so upstream UI stops
waiting.
| } catch (error) { | ||
| console.error("Error in handleDeepLinkData:", error); | ||
| } |
There was a problem hiding this comment.
Outer catch block swallows errors without user feedback.
If an unexpected error occurs, it's only logged to console. Users receive no indication that something went wrong. Consider setting a generic error state to inform users.
} catch (error) {
console.error("Error in handleDeepLinkData:", error);
+ // Set a generic error based on the most recent context
+ // The specific error stores (authError, signingError, blindVoteError, revealError)
+ // may already be set by inner handlers, but if not, provide generic feedback
+ if (data.type === "auth" && !get(authError)) {
+ authError.set("Failed to process authentication request. Please try again.");
+ } else if (data.type === "sign" && !get(signingError) && !get(blindVoteError)) {
+ signingError.set("Failed to process signing request. Please try again.");
+ } else if (data.type === "reveal" && !get(revealError)) {
+ revealError.set("Failed to process reveal request. Please try again.");
+ }
}Committable suggestion skipped: line range outside the PR's diff.
| <div class="flex items-center gap-3"> | ||
| <ButtonAction | ||
| variant="soft" | ||
| class="flex-1" | ||
| callback={() => goto("/register")}>Back</ButtonAction | ||
| > | ||
| <ButtonAction class="flex-1" callback={handleFinish}>Finish</ButtonAction> | ||
| </div> |
There was a problem hiding this comment.
Fix extra > and improve CTA button markup
There’s an extra > after the Back button which will render as text, and the inline child text makes the markup harder to read.
- <div class="flex items-center gap-3">
- <ButtonAction
- variant="soft"
- class="flex-1"
- callback={() => goto("/register")}>Back</ButtonAction
- >
- <ButtonAction class="flex-1" callback={handleFinish}>Finish</ButtonAction>
- </div>
+ <div class="flex items-center gap-3">
+ <ButtonAction
+ variant="soft"
+ class="flex-1"
+ callback={() => goto("/register")}
+ >
+ Back
+ </ButtonAction>
+ <ButtonAction class="flex-1" callback={handleFinish}>
+ Finish
+ </ButtonAction>
+ </div>📝 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.
| <div class="flex items-center gap-3"> | |
| <ButtonAction | |
| variant="soft" | |
| class="flex-1" | |
| callback={() => goto("/register")}>Back</ButtonAction | |
| > | |
| <ButtonAction class="flex-1" callback={handleFinish}>Finish</ButtonAction> | |
| </div> | |
| <div class="flex items-center gap-3"> | |
| <ButtonAction | |
| variant="soft" | |
| class="flex-1" | |
| callback={() => goto("/register")} | |
| > | |
| Back | |
| </ButtonAction> | |
| <ButtonAction class="flex-1" callback={handleFinish}> | |
| Finish | |
| </ButtonAction> | |
| </div> |
🤖 Prompt for AI Agents
in infrastructure/eid-wallet/src/routes/(auth)/e-passport/+page.svelte around
lines 51 to 58, there is an extra stray ">" after the Back ButtonAction closing
tag and the button label is inline which reduces readability; remove the stray
">" so it no longer renders as text, and move the button label onto its own
child line (or use a label prop/slot consistently) so the ButtonAction markup is
clean and readable; ensure both ButtonAction components follow the same
formatting style and keep the callback attributes as-is.
| // Helper function to check if user is on an authenticated route | ||
| function isAuthenticatedRoute(pathname: string): boolean { | ||
| // Authenticated routes are those under (app)/ which are protected by the auth guard | ||
| const authenticatedRoutes = ["/main", "/scan-qr", "/settings"]; | ||
| return authenticatedRoutes.includes(pathname); | ||
| } |
There was a problem hiding this comment.
Authenticated routes list may be incomplete.
The isAuthenticatedRoute helper only checks exact matches for /main, /scan-qr, and /settings. Routes like /settings/language, /settings/pin, and /settings/privacy (used in this same PR) won't match. Consider using startsWith for prefix matching:
function isAuthenticatedRoute(pathname: string): boolean {
- const authenticatedRoutes = ["/main", "/scan-qr", "/settings"];
- return authenticatedRoutes.includes(pathname);
+ const authenticatedPrefixes = ["/main", "/scan-qr", "/settings"];
+ return authenticatedPrefixes.some(prefix => pathname.startsWith(prefix));
}🤖 Prompt for AI Agents
in infrastructure/eid-wallet/src/routes/+layout.svelte around lines 149-154, the
isAuthenticatedRoute helper only checks exact pathname matches so nested routes
like /settings/language, /settings/pin, /settings/privacy (and any future
subroutes) won't be considered authenticated; update the function to use prefix
matching by defining authenticated route prefixes (e.g. "/main", "/scan-qr",
"/settings") and return true if pathname === prefix or
pathname.startsWith(prefix + "/") (or simply pathname.startsWith(prefix)
depending on desired behavior) so all subroutes under /settings are correctly
treated as authenticated.
| // Process queued deep links | ||
| if (pendingDeepLinks.length > 0 && globalState) { | ||
| console.log("Processing", pendingDeepLinks.length, "queued deep links"); | ||
| for (const deepLink of pendingDeepLinks) { | ||
| try { | ||
| handleDeepLink(deepLink); | ||
| } catch (error) { | ||
| console.error("Error processing queued deep link:", error); | ||
| } | ||
| } | ||
| pendingDeepLinks = []; | ||
| } |
There was a problem hiding this comment.
Queued deep links processed without awaiting.
handleDeepLink contains async operations (checkAuth), but the loop processes links synchronously without awaiting. This could cause race conditions if multiple deep links are queued:
if (pendingDeepLinks.length > 0 && globalState) {
console.log("Processing", pendingDeepLinks.length, "queued deep links");
for (const deepLink of pendingDeepLinks) {
try {
- handleDeepLink(deepLink);
+ await handleDeepLink(deepLink);
} catch (error) {
console.error("Error processing queued deep link:", error);
}
}
pendingDeepLinks = [];
}Note: This would also require making handleDeepLink return a Promise or be declared as async.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
in infrastructure/eid-wallet/src/routes/+layout.svelte around lines 601 to 612,
the loop processes queued deep links by calling handleDeepLink synchronously
even though it performs async work (e.g., checkAuth); update the code to await
each call and ensure handleDeepLink is async/returns a Promise — replace the for
loop with a for..of that does await handleDeepLink(deepLink) inside the
try/catch (or collect promises and await Promise.all if parallel processing is
desired), and update handleDeepLink signature to async (or return a Promise) so
callers can await it.
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 (1)
platforms/eReputation/client/src/pages/deeplink-login.tsx (1)
70-70: Update default appVersion to match wallet version.The default
appVersionis set to'0.4.0', but the PR summary indicates the eID wallet is being bumped to v0.5.0. Update the default to maintain version consistency.Apply this diff:
- const requestBody = { ename, session, signature, appVersion: appVersion || '0.4.0' }; + const requestBody = { ename, session, signature, appVersion: appVersion || '0.5.0' };
🧹 Nitpick comments (2)
platforms/group-charter-manager/src/app/deeplink-login/page.tsx (1)
45-62: Redundant fallback parsing logic.The extended parsing in lines 49-56 re-parses the same sources (
window.location.searchandwindow.location.hash) that were already parsed in lines 13-30. If the initial parsing didn't extract the parameters, parsing the same sources again won't yield different results.If this is defensive coding for a specific edge case (e.g., browser quirks, timing issues), please add a comment explaining when this fallback is needed. Otherwise, consider simplifying:
- let ename = params.get('ename'); - let session = params.get('session'); - let signature = params.get('signature'); + const ename = params.get('ename'); + const session = params.get('session'); + const signature = params.get('signature'); const appVersion = params.get('appVersion'); - // Only set error after all parsing attempts have failed - // Try additional parsing methods if params are missing if (!ename || !session || !signature) { - // Try one more time with window.location directly - const directParams = new URLSearchParams(window.location.search); - const hashParams = window.location.hash.includes('?') - ? new URLSearchParams(window.location.hash.substring(window.location.hash.indexOf('?') + 1)) - : null; - - ename = ename || directParams.get('ename') || hashParams?.get('ename') || null; - session = session || directParams.get('session') || hashParams?.get('session') || null; - signature = signature || directParams.get('signature') || hashParams?.get('signature') || null; - - if (!ename || !session || !signature) { - setError("Missing required authentication parameters"); - setIsLoading(false); - return; - } + setError("Missing required authentication parameters"); + setIsLoading(false); + return; }platforms/blabsy/src/pages/deeplink-login.tsx (1)
78-78: Consider making error message more specific.The generic error message doesn't indicate which parameters (ename, session, or signature) are missing, which could help with debugging deep link issues.
Example improvement:
const missingParams = []; if (!ename) missingParams.push('ename'); if (!session) missingParams.push('session'); if (!signature) missingParams.push('signature'); setError(`Missing required parameters: ${missingParams.join(', ')}`);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
platforms/blabsy/src/pages/deeplink-login.tsx(2 hunks)platforms/eReputation/client/src/pages/deeplink-login.tsx(2 hunks)platforms/group-charter-manager/src/app/deeplink-login/page.tsx(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-21T15:49:12.904Z
Learnt from: coodos
Repo: MetaState-Prototype-Project/prototype PR: 458
File: platforms/eReputation-api/src/services/ReferenceSigningSessionService.ts:60-60
Timestamp: 2025-11-21T15:49:12.904Z
Learning: In platforms/eReputation-api/src/services/ReferenceSigningSessionService.ts, the VITE_EREPUTATION_BASE_URL environment variable intentionally has no fallback value. The service should fail if this environment variable is not set, following a fail-fast design pattern.
Applied to files:
platforms/eReputation/client/src/pages/deeplink-login.tsx
⏰ 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 (5)
platforms/group-charter-manager/src/app/deeplink-login/page.tsx (1)
79-89: LGTM - Redirect improvement.The change from reload to
window.location.href = "/"is a good improvement. It navigates to the root path after successful authentication rather than reloading the deeplink page. Usingwindow.location.hrefinstead of Next.js router is acceptable here since a full navigation ensures the auth state is cleanly initialized.platforms/eReputation/client/src/pages/deeplink-login.tsx (2)
39-41: LGTM! Variable declarations support fallback parsing.Changing these to
letappropriately enables the fallback parameter parsing implemented below.
80-88: LGTM! Robust response validation.The additional check for both
data.tokenanddata.userprevents storing incomplete credentials and aligns with patterns used in other modules. This defensive approach improves authentication reliability.platforms/blabsy/src/pages/deeplink-login.tsx (2)
41-43: Variable reassignment enabled for fallback parsing.Changing these from
consttoletcorrectly supports the fallback parameter extraction logic that follows.
109-117: Token-based authentication flow looks correct.The response handling properly checks for the presence of
data.tokenand falls back to an error state if absent. The comment clarifies the expected API response format.
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 (2)
platforms/eReputation/client/src/pages/deeplink-login.tsx (1)
76-87: Good validation, but remove unnecessary navigation delay.The addition of validation for
data.tokenanddata.userbefore storing credentials is good defensive programming.However, the 100ms
setTimeoutbefore navigation is unnecessary.localStorage.setItemis a synchronous operation in browsers—the values are immediately persisted before the next line executes. The delay adds artificial latency without providing any real benefit.Apply this diff to remove the unnecessary delay:
if (data.token && data.user) { localStorage.setItem("ereputation_token", data.token); localStorage.setItem("ereputation_user_id", data.user.id); - // Use setTimeout to ensure localStorage is written before navigation - setTimeout(() => { - window.location.href = "/"; - }, 100); + window.location.href = "/"; } else {If you're experiencing issues with localStorage not being available after navigation, the problem likely lies elsewhere (e.g., timing of when the target page reads localStorage, or session storage isolation in certain scenarios). If that's the case, please clarify the specific issue you're addressing.
platforms/pictique/src/routes/(auth)/deeplink-login/+page.svelte (1)
12-130: Remove extensive debug logging before production deployment.The file contains 20+
console.logandconsole.errorstatements that log the authentication flow, including truncated sensitive parameters (ename, session, signature, token). While truncation is better than full values, production code should not log authentication credentials in any form.Recommended approach:
- Remove all debug logging statements for production
- If diagnostics are needed, use conditional logging based on environment:
const DEBUG = import.meta.env.DEV; // or use a feature flag if (DEBUG) { console.log('[PICTIQUE DEEPLINK] Starting deeplink login'); }
- For production error monitoring, log non-sensitive metadata only:
// Good: no credentials console.error('[PICTIQUE DEEPLINK] Authentication failed', { hasEname: !!ename, hasSession: !!session, hasSignature: !!signature, errorCode: response.status }); // Bad: contains credential fragments console.log('ename:', ename ? `${ename.substring(0, 10)}...` : 'MISSING');Alternatively, integrate with a proper observability tool (e.g., Sentry, DataDog) that can sanitize sensitive data automatically.
♻️ Duplicate comments (4)
platforms/eReputation/client/src/pages/deeplink-login.tsx (1)
44-58: Remove arbitrary delay and investigate root cause of missing parameters.This fallback block introduces several issues:
Arbitrary 500ms delay: Blocks execution when parameters are missing, creating poor UX. The delay suggests an underlying race condition or timing issue that should be fixed at the root cause, not masked with a delay.
Duplicate parsing: Re-parses
window.location.searchand hash despite lines 12-37 already performing comprehensive URL parsing. A previous review flagged this duplicate logic as addressed in commit ca2429d, but it reappears here.Unclear necessity: Why would parameters be missing initially but present after 500ms? This suggests the component may be mounting before the URL is fully populated, which indicates an architectural issue upstream.
Recommended approach:
- If deep-link parameters arrive asynchronously, implement proper event-driven handling or state management rather than polling with delays.
- If the issue is platform-specific (e.g., iOS/Android deep-link timing), document the behavior and handle it explicitly.
- Remove the duplicate parsing and rely on the initial parse (lines 12-37).
Apply this diff to remove the delay and duplicate parsing:
- if (!ename || !session || !signature) { - // Add a small delay to allow URL to fully parse before showing error - await new Promise(resolve => setTimeout(resolve, 500)); - // Re-check one more time after delay - const finalParams = new URLSearchParams(window.location.search || (window.location.hash.includes('?') ? window.location.hash.substring(window.location.hash.indexOf('?') + 1) : '') || ''); - ename = finalParams.get('ename') || ename; - session = finalParams.get('session') || session; - signature = finalParams.get('signature') || signature; - - if (!ename || !session || !signature) { - setError("Missing required authentication parameters"); - setIsLoading(false); - return; - } + if (!ename || !session || !signature) { + setError("Missing required authentication parameters"); + setIsLoading(false); + return; }Then change lines 39-41 back to
const:- let ename = params.get('ename'); - let session = params.get('session'); - let signature = params.get('signature'); + const ename = params.get('ename'); + const session = params.get('session'); + const signature = params.get('signature');platforms/dreamSync/client/src/pages/deeplink-login.tsx (1)
39-58: Same 500ms delay workaround as eCurrency platform.This file contains identical parameter retry logic as
platforms/eCurrency/client/src/pages/deeplink-login.tsx. See the review comment on that file for detailed analysis and refactoring suggestions.platforms/blabsy/src/pages/deeplink-login.tsx (1)
43-62: Same 500ms delay workaround pattern (duplicated).This implements the identical arbitrary delay retry logic as the other platform files. A previous review comment on this file (lines 46-82) suggested simplifying the fallback parsing to eliminate redundancy by parsing both sources upfront—this PR takes the opposite approach by adding delays.
See the review comment on
platforms/eCurrency/client/src/pages/deeplink-login.tsx(lines 39-58) for a detailed refactoring proposal using a centralized utility.platforms/group-charter-manager/src/app/deeplink-login/page.tsx (1)
40-59: Same 500ms delay workaround pattern (duplicated).Identical to the other platform files.
See the review comment on
platforms/eCurrency/client/src/pages/deeplink-login.tsx(lines 39-58) for detailed analysis and refactoring suggestions.
🧹 Nitpick comments (1)
platforms/pictique/src/routes/(auth)/deeplink-login/+page.svelte (1)
64-72: LGTM—improved error messaging.Building an explicit list of missing parameters (lines 64-68) and including them in the error message (line 69) provides much clearer feedback than the generic "Missing required authentication parameters" used in other files.
Consider adopting this pattern in the other platform deeplink-login files for consistency.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
platforms/blabsy/src/pages/deeplink-login.tsx(4 hunks)platforms/dreamSync/client/src/pages/deeplink-login.tsx(1 hunks)platforms/eCurrency/client/src/pages/deeplink-login.tsx(1 hunks)platforms/eReputation/client/src/pages/deeplink-login.tsx(2 hunks)platforms/group-charter-manager/src/app/deeplink-login/page.tsx(2 hunks)platforms/pictique/src/routes/(auth)/deeplink-login/+page.svelte(3 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-21T15:49:12.904Z
Learnt from: coodos
Repo: MetaState-Prototype-Project/prototype PR: 458
File: platforms/eReputation-api/src/services/ReferenceSigningSessionService.ts:60-60
Timestamp: 2025-11-21T15:49:12.904Z
Learning: In platforms/eReputation-api/src/services/ReferenceSigningSessionService.ts, the VITE_EREPUTATION_BASE_URL environment variable intentionally has no fallback value. The service should fail if this environment variable is not set, following a fail-fast design pattern.
Applied to files:
platforms/eReputation/client/src/pages/deeplink-login.tsx
🧬 Code graph analysis (1)
platforms/blabsy/src/pages/deeplink-login.tsx (4)
platforms/dreamSync/client/src/pages/deeplink-login.tsx (1)
DeeplinkLogin(4-132)platforms/eCurrency/client/src/pages/deeplink-login.tsx (1)
DeeplinkLogin(4-132)platforms/eReputation/client/src/pages/deeplink-login.tsx (1)
DeeplinkLogin(4-136)platforms/eVoting/src/app/deeplink-login/page.tsx (1)
DeeplinkLogin(6-129)
⏰ 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
| // Navigation will happen via auth state change, but ensure we navigate | ||
| router.push('/home').catch(() => { | ||
| // If push fails, try replace | ||
| router.replace('/home').catch(() => { | ||
| window.location.href = '/home'; | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Potential double-navigation race between effects.
Two separate code paths can trigger navigation to '/home':
- Lines 94-100: Immediately after
signInWithCustomTokensucceeds - Lines 128-136: When the
userstate becomes truthy (triggered by the auth state change fromsignInWithCustomToken)
Since signInWithCustomToken at line 93 likely updates the user state in the auth context, both navigations may fire in quick succession, potentially causing UI flashing or unexpected behavior.
Consider one of these approaches:
Option 1: Remove explicit navigation after login (rely on auth state)
if (data.token) {
await signInWithCustomToken(data.token);
- // Navigation will happen via auth state change, but ensure we navigate
- router.push('/home').catch(() => {
- // If push fails, try replace
- router.replace('/home').catch(() => {
- window.location.href = '/home';
- });
- });
+ // Navigation handled by user effect below
} else {Option 2: Remove the separate user effect (handle in login flow only)
- // If user is authenticated, navigate to home
- useEffect(() => {
- if (user) {
- router.push('/home').catch(() => {
- router.replace('/home').catch(() => {
- window.location.href = '/home';
- });
- });
- }
- }, [user, router]);Option 1 is preferred if other pages also need the automatic user-based redirect.
Also applies to: 128-136
| let ename = params.get('ename'); | ||
| let session = params.get('session'); | ||
| let signature = params.get('signature'); | ||
| const appVersion = params.get('appVersion'); | ||
|
|
||
| if (!ename || !session || !signature) { | ||
| setError("Missing required authentication parameters"); | ||
| setIsLoading(false); | ||
| return; | ||
| // Add a small delay to allow URL to fully parse before showing error | ||
| await new Promise(resolve => setTimeout(resolve, 500)); | ||
| // Re-check one more time after delay | ||
| const finalParams = new URLSearchParams(window.location.search || (window.location.hash.includes('?') ? window.location.hash.substring(window.location.hash.indexOf('?') + 1) : '') || ''); | ||
| ename = finalParams.get('ename') || ename; | ||
| session = finalParams.get('session') || session; | ||
| signature = finalParams.get('signature') || signature; | ||
|
|
||
| if (!ename || !session || !signature) { | ||
| setError("Missing required authentication parameters"); | ||
| setIsLoading(false); | ||
| return; | ||
| } | ||
| } |
There was a problem hiding this comment.
Reconsider the 500ms delay workaround—address the root cause instead.
The arbitrary 500ms delay added to retry parameter parsing is a fragile timing assumption that blocks the UI, offers no guarantee of success, and doesn't address why parameters aren't available on first parse. This pattern is duplicated across multiple platform files (eCurrency, dreamSync, blabsy, group-charter-manager).
Consider these alternatives:
- Event-driven approach: Listen for URL changes or app-ready events rather than polling with arbitrary delays.
- Synchronous URL access: Ensure the app waits for deep-link intent/URL to be fully resolved before rendering this component.
- Centralized utility: Extract this logic into a shared
parseDeeplinkParamsutility that all platforms can use, eliminating the duplication across 5+ files.
If the delay is truly necessary due to platform limitations, document why 500ms was chosen and add a maximum retry count to prevent indefinite blocking.
Example refactor to centralized utility:
// shared/utils/deeplink.ts
export async function parseDeeplinkParams(
maxRetries = 2,
retryDelay = 500
): Promise<{ ename: string; session: string; signature: string; appVersion?: string } | null> {
for (let attempt = 0; attempt <= maxRetries; attempt++) {
const params = extractParams(); // extract from search/hash
if (params.ename && params.session && params.signature) {
return params;
}
if (attempt < maxRetries) {
await new Promise(resolve => setTimeout(resolve, retryDelay));
}
}
return null;
}Then use it consistently:
-let ename = params.get('ename');
-let session = params.get('session');
-let signature = params.get('signature');
-const appVersion = params.get('appVersion');
-
-if (!ename || !session || !signature) {
- await new Promise(resolve => setTimeout(resolve, 500));
- const finalParams = new URLSearchParams(window.location.search || (window.location.hash.includes('?') ? window.location.hash.substring(window.location.hash.indexOf('?') + 1) : '') || '');
- ename = finalParams.get('ename') || ename;
- session = finalParams.get('session') || session;
- signature = finalParams.get('signature') || signature;
-
- if (!ename || !session || !signature) {
- setError("Missing required authentication parameters");
- setIsLoading(false);
- return;
- }
-}
+const authParams = await parseDeeplinkParams();
+if (!authParams) {
+ setError("Missing required authentication parameters");
+ setIsLoading(false);
+ return;
+}
+const { ename, session, signature, appVersion } = authParams;| // Use setTimeout to ensure tokens are written before navigation | ||
| setTimeout(() => { | ||
| window.location.href = "/"; | ||
| }, 100); |
There was a problem hiding this comment.
Remove unnecessary 100ms navigation delay.
The 100ms setTimeout before navigation (to "ensure tokens are written") is likely unnecessary. setAuthToken and setAuthId appear to be synchronous localStorage operations based on the naming and usage pattern. localStorage writes are synchronous and blocking—they complete before the next line executes.
Remove the setTimeout wrapper:
if (data.token && data.user) {
setAuthId(data.user.id);
setAuthToken(data.token);
- // Use setTimeout to ensure tokens are written before navigation
- setTimeout(() => {
- window.location.href = "/";
- }, 100);
+ window.location.href = "/";
} else {If there's evidence that tokens aren't available after navigation, investigate the actual root cause rather than adding arbitrary delays.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In platforms/group-charter-manager/src/app/deeplink-login/page.tsx around lines
81 to 84, remove the unnecessary 100ms setTimeout wrapper around navigation;
instead set window.location.href = "/" immediately after calling
setAuthToken/setAuthId (or their equivalents) so navigation occurs synchronously
after the token writes; if token availability issues remain, create a separate
follow-up to investigate the root cause rather than reintroducing artificial
delays.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
infrastructure/eid-wallet/src/routes/(app)/scan-qr/scanLogic.ts (1)
1221-1354: Deep-link sign/blind-vote error handling still leaves users without feedback in several failure modesThe new
handleDeepLinkDatalogic is a big improvement (duplicate guards, clearer state transitions, blind-vote support), but a few important UX/error-handling gaps remain:
Missing user feedback when sign deep-link params are invalid
If
signingSessionId,base64Data, orredirectUriare missing, theif (get(signingSessionId) && base64Data && redirectUri)block is skipped and the function exits silently. Similarly, ifatob/JSON.parsefails, the innercatchonly logs to console and returns.That leaves the user with no visible error or UI change, which is the same underlying issue called out in the previous review.
Consider surfacing a clear
signingErrorand logging details when validation/decoding fails, e.g.:
if (get(signingSessionId) && base64Data && redirectUri) {
if (get(signingSessionId) && base64Data && redirectUri) { redirect.set(redirectUri); signingError.set(null); // Clear any previous signing errors@@
} catch (error) {console.error("Error decoding signing data:", error);return;}}
} catch (error) {console.error("Error decoding signing data:", error);signingError.set("Failed to decode signing data. The link may be invalid.",);return;}} else {console.error("Invalid signing deep link parameters:", {signingSessionId: get(signingSessionId),hasData: !!base64Data,hasRedirectUri: !!redirectUri,});signingError.set("Invalid signing request. Missing required parameters.",);return;}
Blind-vote via sign deep link: missing pollId guard and non-ok poll response handling
In the blind-vote branch,
pollIdis pulled fromparsedSigningData?.pollIdbut not validated before constructing the poll-details URL. IfpollIdis missing or empty, you could end up requesting/api/polls/undefined. Also, non-ok HTTP statuses are ignored; only network exceptions setblindVoteError. That matches the previous concern wherepollDetailsstays as “Loading…” forever without an error.You can tighten this up by:
- Guarding against missing
pollId.- Handling
!pollResponse.okwith a user-facing error.For example:
signingData.set({pollId: parsedSigningData?.pollId,
const pollId = parsedSigningData?.pollId;if (!pollId) {const errorMessage ="Missing poll ID in blind-vote signing data.";blindVoteError.set(errorMessage);throw new Error(errorMessage);}signingData.set({pollId, sessionId: parsedSigningData?.sessionId, platform_url: platformUrl, redirect: redirectUri,@@
try {const pollResponse = await fetch(`${platformUrl}/api/polls/${get(signingData)?.pollId}`,);if (pollResponse.ok) {
try {const pollResponse = await fetch(`${platformUrl}/api/polls/${pollId}`,);if (pollResponse.ok) { const pollDetails = await pollResponse.json();@@
console.log("✅ Poll details fetched:",pollDetails,);}
console.log("✅ Poll details fetched:",pollDetails,);} else {console.error(`Failed to fetch poll details: ${pollResponse.status} ${pollResponse.statusText}`,);blindVoteError.set("Failed to load poll details. Please try again.",);} } catch (error) { console.error( "Failed to fetch poll details:", error, ); blindVoteError.set(
"Failed to load poll details",
"Failed to load poll details", ); }
Outer catch still only logs — no generic user-facing fallback
The outer
catchat the bottom ofhandleDeepLinkDatastill just logs the error. If something unexpected goes wrong before a specific error store is set, the user sees nothing. You can preserve the log while providing a generic per-type fallback:
} catch (error) {console.error("Error in handleDeepLinkData:", error);}
} catch (error) {console.error("Error in handleDeepLinkData:", error);// Provide generic user feedback if specific errors were not setif (data.type === "auth" && !get(authError)) {authError.set("Failed to process authentication request. Please try again.",);} else if (data.type === "sign" &&!get(signingError) &&!get(blindVoteError)) {signingError.set("Failed to process signing request. Please try again.",);} else if (data.type === "reveal" && !get(revealError)) {revealError.set("Failed to process reveal request. Please try again.",);}}
Minor copy nit: PUBLIC_PLATFORM_URL mention
The message:
const errorMessage = "Missing platform URL in signing data and no PUBLIC_PLATFORM_URL configured.";still references
PUBLIC_PLATFORM_URL, but the code no longer consults any such fallback. Consider rephrasing to something like"Missing platform URL in signing data."to better reflect the current fail-fast behavior. Based on learnings, the fail-fast pattern itself (no implicit base URL fallback) is consistent with how similar services are configured elsewhere in the repo.
🧹 Nitpick comments (3)
platforms/eReputation/client/src/pages/deeplink-login.tsx (2)
39-52: Async fallback re-parse works but consider unmount safety and optionalappVersionrefreshThe 500 ms delayed re-parse is reasonable for late-arriving deeplink params, but because
handleDeeplinkLoginisasyncand the effect has no cleanup,setError/setIsLoadingcan still run after the component unmounts (e.g., user navigates away quickly), which can trigger React warnings. A small cancellation flag in the effect cleanup would avoid that.Also, the fallback only re-reads
ename,session, andsignature. IfappVersionis also injected late into the URL, you’ll still send the originalappVersion(likelynull) and fall back to the hard-coded default below. If you intend the server to see the real wallet version, you may want to mirror the re-parse forappVersionas well.Example sketch:
useEffect(() => { - const handleDeeplinkLogin = async () => { + let cancelled = false; + + const handleDeeplinkLogin = async () => { try { … - let ename = params.get('ename'); - let session = params.get('session'); - let signature = params.get('signature'); - const appVersion = params.get('appVersion'); + let ename = params.get('ename'); + let session = params.get('session'); + let signature = params.get('signature'); + let appVersion = params.get('appVersion'); … if (!ename || !session || !signature) { await new Promise(resolve => setTimeout(resolve, 500)); - const finalParams = new URLSearchParams(…); - ename = finalParams.get('ename') || ename; - session = finalParams.get('session') || session; - signature = finalParams.get('signature') || signature; + if (cancelled) return; + const finalParams = new URLSearchParams(…); + ename = finalParams.get('ename') || ename; + session = finalParams.get('session') || session; + signature = finalParams.get('signature') || signature; + appVersion = finalParams.get('appVersion') || appVersion; } … } catch (error) { - console.error('Login request failed:', error); - setError("Failed to connect to server"); - setIsLoading(false); + if (!cancelled) { + console.error('Login request failed:', error); + setError("Failed to connect to server"); + setIsLoading(false); + } } }; handleDeeplinkLogin(); - }, []); + return () => { + cancelled = true; + }; + }, []);
76-83: Stricter success contract looks good; optionally guarduser.idand re-evaluate need forsetTimeoutRequiring both
data.tokenanddata.userbefore treating the login as successful is a solid improvement. Two small follow‑ups you might consider:
If
data.useris present but doesn’t have anid, you’ll store"undefined"inereputation_user_id. If that matters downstream, you could folddata.user.idinto the success predicate:if (data.token && data.user && data.user.id) { localStorage.setItem("ereputation_token", data.token); localStorage.setItem("ereputation_user_id", data.user.id); setTimeout(() => { window.location.href = "/"; }, 100); } else { … }
localStorage.setItemis synchronous, so thesetTimeoutis likely unnecessary from a persistence standpoint. If the delay is only to smooth UX or avoid race conditions elsewhere, consider adding a brief comment about that intent (or reducing/removing it) so future readers understand why it’s there.infrastructure/eid-wallet/src/routes/(auth)/register/+page.svelte (1)
179-190: Second-step Back/Confirm pairing is consistent with new flowThe new
div.flexwrapping Back (to/onboarding) and the primary Confirm action, withflex-1on both buttons, is structurally sound and matches the first-step layout pattern. No functional issues spotted here.If the intention is to go “one step back” rather than exit the PIN flow, you might consider toggling
firstStep = trueinstead of navigating to/onboarding, but that’s purely UX-dependent.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (6)
infrastructure/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.apkis excluded by!**/gen/**infrastructure/eid-wallet/src-tauri/gen/android/app/arm64/release/baselineProfiles/0/app-arm64-release.dmis excluded by!**/gen/**infrastructure/eid-wallet/src-tauri/gen/android/app/arm64/release/baselineProfiles/1/app-arm64-release.dmis excluded by!**/gen/**infrastructure/eid-wallet/src-tauri/gen/android/app/arm64/release/output-metadata.jsonis excluded by!**/gen/**
📒 Files selected for processing (8)
infrastructure/eid-wallet/src/routes/(app)/scan-qr/+page.svelte(2 hunks)infrastructure/eid-wallet/src/routes/(app)/scan-qr/scanLogic.ts(2 hunks)infrastructure/eid-wallet/src/routes/(auth)/register/+page.svelte(5 hunks)platforms/blabsy/src/pages/deeplink-login.tsx(4 hunks)platforms/dreamSync/client/src/pages/deeplink-login.tsx(1 hunks)platforms/eCurrency/client/src/pages/deeplink-login.tsx(1 hunks)platforms/eReputation/client/src/pages/deeplink-login.tsx(2 hunks)platforms/group-charter-manager/src/app/deeplink-login/page.tsx(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- platforms/dreamSync/client/src/pages/deeplink-login.tsx
- infrastructure/eid-wallet/src/routes/(app)/scan-qr/+page.svelte
- platforms/group-charter-manager/src/app/deeplink-login/page.tsx
- platforms/eCurrency/client/src/pages/deeplink-login.tsx
- platforms/blabsy/src/pages/deeplink-login.tsx
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-11-21T15:49:12.904Z
Learnt from: coodos
Repo: MetaState-Prototype-Project/prototype PR: 458
File: platforms/eReputation-api/src/services/ReferenceSigningSessionService.ts:60-60
Timestamp: 2025-11-21T15:49:12.904Z
Learning: In platforms/eReputation-api/src/services/ReferenceSigningSessionService.ts, the VITE_EREPUTATION_BASE_URL environment variable intentionally has no fallback value. The service should fail if this environment variable is not set, following a fail-fast design pattern.
Applied to files:
platforms/eReputation/client/src/pages/deeplink-login.tsxinfrastructure/eid-wallet/src/routes/(app)/scan-qr/scanLogic.ts
📚 Learning: 2025-11-14T17:54:47.719Z
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.719Z
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)/register/+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 (5)
platforms/eReputation/client/src/pages/deeplink-login.tsx (1)
66-67: ConfirmappVersiondefault ('0.4.0') is still correct and consider making it less brittleThe request body currently hard‑codes a default:
const requestBody = { ename, session, signature, appVersion: appVersion || '0.4.0' };Given this PR is about eID Wallet UX and (per the PR summary) bumps the wallet to v0.5.0, it’s worth double‑checking whether
'0.4.0'is still the right fallback. If the server logic depends on this version, misreporting could affect feature flags or compatibility checks.If you expect this to change frequently, you might also consider centralising the “current wallet version” in a single constant or deriving it from build metadata/env to avoid future hard‑coded mismatches.
infrastructure/eid-wallet/src/routes/(auth)/register/+page.svelte (3)
6-11: Icon imports and Button namespace look consistentThe additional
@hugeiconsimports andButtonnamespace import fit the existing icon usage patterns in this file; no issues from a correctness standpoint.
221-228: Drawer PIN-set Back/Next actions behave coherentlyUsing a soft Back button to close the drawer (
showDrawer = false) and a primary Next button to advance to the biometrics screen viahandleNextis consistent with the stated UX goals. Markup and callbacks look correct.
273-282: Drawer biometrics-added Back/Continue actions are wired correctlyIn the “You’re all set!” state, the Back button cleanly closes the drawer while Continue calls
handleBiometricsAddedto mark onboarding complete and navigate to review. Layout matches the other two-button groups and the logic is sound.infrastructure/eid-wallet/src/routes/(app)/scan-qr/scanLogic.ts (1)
335-338: Non-blocking navigation before opening external deeplink looks goodTriggering
goto("/main")in a fire-and-forget way with an attached.catchavoids blocking the auth flow while still preventing unhandled rejections. This aligns well with the “navigate home before opening external deeplink” requirement without risking a hard failure if navigation itself breaks.
| <div class="flex items-center gap-3"> | ||
| <ButtonAction | ||
| variant="soft" | ||
| class="flex-1" | ||
| callback={() => goto("/onboarding")}>Back</ButtonAction | ||
| > | ||
| <ButtonAction | ||
| class="w-full" | ||
| class="flex-1" | ||
| variant={btnVariant} | ||
| callback={handleFirstStep} | ||
| > | ||
| Confirm | ||
| </ButtonAction> | ||
| </div> |
There was a problem hiding this comment.
Fix malformed closing tag for the first-step Confirm button
The ButtonAction wrapping the “Confirm” label is missing the terminating > on its closing tag (</ButtonAction), which will cause a Svelte compilation error.
You can align this with the surrounding pattern by splitting the closing tag over two lines:
- <ButtonAction
- class="flex-1"
- variant={btnVariant}
- callback={handleFirstStep}
- >
- Confirm
- </ButtonAction
- </div>
+ <ButtonAction
+ class="flex-1"
+ variant={btnVariant}
+ callback={handleFirstStep}
+ >
+ Confirm
+ </ButtonAction
+ >
+ </div>📝 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.
| <div class="flex items-center gap-3"> | |
| <ButtonAction | |
| variant="soft" | |
| class="flex-1" | |
| callback={() => goto("/onboarding")}>Back</ButtonAction | |
| > | |
| <ButtonAction | |
| class="w-full" | |
| class="flex-1" | |
| variant={btnVariant} | |
| callback={handleFirstStep} | |
| > | |
| Confirm | |
| </ButtonAction> | |
| </div> | |
| <div class="flex items-center gap-3"> | |
| <ButtonAction | |
| variant="soft" | |
| class="flex-1" | |
| callback={() => goto("/onboarding")}>Back</ButtonAction> | |
| > | |
| <ButtonAction | |
| class="flex-1" | |
| variant={btnVariant} | |
| callback={handleFirstStep} | |
| > | |
| Confirm | |
| </ButtonAction> | |
| </div> |
🤖 Prompt for AI Agents
infrastructure/eid-wallet/src/routes/(auth)/register/+page.svelte around lines
152 to 165: the closing tag for the Confirm ButtonAction is malformed (missing
the trailing '>') causing a Svelte compile error; fix it by correcting the
closing tag to "</ButtonAction>" (match the surrounding pattern and split across
two lines if desired so the opening tag, inner text, and full closing tag are
properly formed) and ensure indentation and spacing match the adjacent Back
button.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
platforms/eReputation/client/src/pages/references.tsx (1)
59-67: Align unauthorized revoke flow with the new clearAuth + /auth patternOn unauthorized errors you currently toast and redirect to
/api/loginwithout clearing client auth state. Given that explicit logout is nowclearAuth()+ redirect to/auth, it would be cleaner and less error‑prone to do the same here so stale tokens don’t linger and the user sees a consistent flow.A possible adjustment:
if (isUnauthorizedError(error)) { toast({ title: "Unauthorized", description: "You are logged out. Logging in again...", variant: "destructive", }); - setTimeout(() => { - window.location.href = "/api/login"; - }, 500); + setTimeout(() => { + clearAuth(); + window.location.href = "/auth"; + }, 500); return; }platforms/eReputation/client/src/pages/dashboard.tsx (1)
4-4: Dashboard logout matches new clearAuth pattern; consider centralizing itThe updated logout (call
clearAuth()then redirect to/auth) is consistent with the new auth flow and looks correct here.Since the same pattern now appears in multiple pages (e.g., Dashboard and References), you might consider exposing a shared
useLogout()hook or an exportedlogout()helper from your auth utilities, so that future changes to the logout behavior stay consistent across the app.Also applies to: 75-78
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
platforms/eReputation/client/src/pages/dashboard.tsx(2 hunks)platforms/eReputation/client/src/pages/references.tsx(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
platforms/eReputation/client/src/pages/dashboard.tsx (1)
platforms/eReputation/client/src/lib/authUtils.ts (1)
clearAuth(31-34)
platforms/eReputation/client/src/pages/references.tsx (1)
platforms/eReputation/client/src/lib/authUtils.ts (1)
clearAuth(31-34)
⏰ 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). (2)
- GitHub Check: build
- GitHub Check: lint
🔇 Additional comments (1)
platforms/eReputation/client/src/pages/references.tsx (1)
4-4: Logout pattern looks good – confirmed stateless JWT design
clearAuth()removes the client-side token/user ID from localStorage and the redirect to/authis the correct logout flow for your stateless JWT architecture. No server-side logout call is needed since the backend validates tokens on each request without maintaining session or refresh-token state.This pattern is already consistent across the eReputation platform (no
/api/logoutusage anywhere), so no changes required.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
platforms/eReputation/client/src/pages/deeplink-login.tsx (1)
112-120: Duplicate issue: Silent redirect hides connection failures.This exhibits the same problem as lines 101-106—silently redirecting users when an existing token is found, even though the connection or authentication attempt failed. When users click a deeplink, they expect feedback about the outcome, not a silent redirect that masks the failure.
Apply the same fix as suggested for lines 101-109:
- // If token already exists, silently continue to home - const existingToken = localStorage.getItem("ereputation_token"); - if (existingToken) { - window.location.href = "/"; - return; - } setError("Failed to connect to server"); setIsLoading(false);
🧹 Nitpick comments (1)
platforms/eReputation/client/src/pages/deeplink-login.tsx (1)
86-89: The setTimeout delay is unnecessary.
localStorage.setItemis synchronous and completes immediately. The 100ms delay before navigation doesn't serve a functional purpose and only adds unnecessary latency to the user experience.Apply this diff to remove the unnecessary delay:
- // Use setTimeout to ensure localStorage is written before navigation - setTimeout(() => { - window.location.href = "/"; - }, 100); + window.location.href = "/";
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
platforms/eReputation/client/src/pages/deeplink-login.tsx(3 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-21T15:49:12.904Z
Learnt from: coodos
Repo: MetaState-Prototype-Project/prototype PR: 458
File: platforms/eReputation-api/src/services/ReferenceSigningSessionService.ts:60-60
Timestamp: 2025-11-21T15:49:12.904Z
Learning: In platforms/eReputation-api/src/services/ReferenceSigningSessionService.ts, the VITE_EREPUTATION_BASE_URL environment variable intentionally has no fallback value. The service should fail if this environment variable is not set, following a fail-fast design pattern.
Applied to files:
platforms/eReputation/client/src/pages/deeplink-login.tsx
⏰ 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
| // If token already exists, silently continue to home | ||
| const existingToken = localStorage.getItem("ereputation_token"); | ||
| if (existingToken) { | ||
| window.location.href = "/"; | ||
| return; | ||
| } | ||
| setError(errorData.error || "Authentication failed"); | ||
| setIsLoading(false); | ||
| } |
There was a problem hiding this comment.
Silent redirect hides authentication failures from users.
When deeplink authentication fails but an existing token is found, the code silently redirects to home without indicating that the authentication attempt failed. This creates a confusing user experience—users who clicked a deeplink expecting to authenticate receive no feedback about the failure.
Additionally, the code doesn't validate whether the existing token is still valid or expired. While this pattern may follow similar implementations elsewhere in the codebase, it risks masking legitimate issues and degrading the user experience.
Consider either:
- Removing the silent redirect and allowing the error to display normally
- Adding user-visible feedback (e.g., a toast notification) indicating "Already authenticated, redirecting..."
- Validating the existing token before deciding to redirect
- // If token already exists, silently continue to home
- const existingToken = localStorage.getItem("ereputation_token");
- if (existingToken) {
- window.location.href = "/";
- return;
- }
setError(errorData.error || "Authentication failed");
setIsLoading(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.
| // If token already exists, silently continue to home | |
| const existingToken = localStorage.getItem("ereputation_token"); | |
| if (existingToken) { | |
| window.location.href = "/"; | |
| return; | |
| } | |
| setError(errorData.error || "Authentication failed"); | |
| setIsLoading(false); | |
| } | |
| setError(errorData.error || "Authentication failed"); | |
| setIsLoading(false); | |
| } |
🤖 Prompt for AI Agents
In platforms/eReputation/client/src/pages/deeplink-login.tsx around lines
101-109, the current logic silently redirects to "/" when an existing token is
present, hiding authentication failures; change this to validate the existing
token before redirecting: call the existing token validation or user-info
endpoint (or a validateToken helper) and if the token is valid show a brief
user-visible message (toast or banner) like "Already authenticated,
redirecting…" then redirect, otherwise clear the invalid token from
localStorage, allow the error to display (setError / setIsLoading(false)), and
avoid the silent redirect; ensure all branches update isLoading and error state
appropriately.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
platforms/group-charter-manager/src/app/deeplink-login/page.tsx (1)
100-103: Arbitrary delay added despite prior feedback.A previous review flagged removing unnecessary navigation delays, noting that
setAuthToken/setAuthIdare synchronous localStorage operations. This change introduces a 300ms delay and switches towindow.location.reload().If
AuthProviderhas a race condition reading tokens on mount, that should be fixed in theAuthProvideritself (e.g., by reading tokens synchronously during initialization or using a state flag). Arbitrary timeouts are fragile and create unpredictable behavior across different devices/network conditions.platforms/eReputation/client/src/pages/deeplink-login.tsx (2)
39-64: 500ms retry + silent redirect still degrade UX and hide deeplink errors.This block re-introduces a delayed re-parse of the URL and then silently redirects home when
ename/session/signatureare missing. URL state is synchronous and stable, so a 500ms delay is unlikely to fix missing parameters but always slows the flow, and the silent redirect means a user who just tapped a login deeplink gets no feedback that the link was invalid.Consider removing the delay and fallback re-parsing entirely and surfacing a clear error instead:
- if (!ename || !session || !signature) { - // Add a small delay to allow URL to fully parse before deciding - await new Promise(resolve => setTimeout(resolve, 500)); - // Re-check one more time after delay - const finalParams = new URLSearchParams( - window.location.search || - (window.location.hash.includes('?') - ? window.location.hash.substring(window.location.hash.indexOf('?') + 1) - : '') || - '' - ); - ename = finalParams.get('ename') || ename; - session = finalParams.get('session') || session; - signature = finalParams.get('signature') || signature; - - if (!ename || !session || !signature) { - // If still missing, silently redirect to home/login to avoid flashing error - window.location.href = "/"; - return; - } - } + if (!ename || !session || !signature) { + setError("Missing required authentication parameters"); + setIsLoading(false); + return; + }This aligns with the existing full‑screen error UI and avoids hiding deeplink failures behind a redirect.
115-120: Silent redirects on existing tokens hide deeplink-auth failures.In both the non‑OK response branch and the catch block, finding an
ereputation_tokencauses an immediate redirect to/without surfacing that the deeplink authentication the user just initiated actually failed. This matches earlier feedback and still leads to confusing UX, especially given the rest of the PR’s focus on explicit user feedback (toasts, full‑screen errors).Instead, let the error UI show, or explicitly communicate “Already authenticated, redirecting…”:
- // If token already exists, silently continue to home - const existingToken = localStorage.getItem("ereputation_token"); - if (existingToken) { - window.location.href = "/"; - return; - } setError(errorData.error || "Authentication failed"); setIsLoading(false);and
- // If token already exists, silently continue to home - const existingToken = localStorage.getItem("ereputation_token"); - if (existingToken) { - window.location.href = "/"; - return; - } setError("Failed to connect to server"); setIsLoading(false);If you want to treat “already authenticated” as success, consider validating the token first and then showing an explicit toast/banner before redirecting.
Also applies to: 126-131
🧹 Nitpick comments (3)
platforms/group-charter-manager/src/app/deeplink-login/page.tsx (2)
87-91: Use a utility function or constant for the localStorage key.Direct access to
localStorage.getItem("group_charter_auth_token")is inconsistent with usingsetAuthToken/setAuthIdutilities elsewhere. If the key name changes, these direct accesses will break.Consider adding a
getAuthToken()utility function in@/lib/authUtilsand using it here and at lines 115-116 and 125-126.+import { setAuthToken, setAuthId, getAuthToken } from "@/lib/authUtils"; ... - const existingToken = localStorage.getItem("group_charter_auth_token"); + const existingToken = getAuthToken();
125-129: Add logging for network failure fallback.Redirecting home on network failure when a token exists is reasonable for resilience. However, silent redirects make debugging production issues difficult.
const existingToken = localStorage.getItem("group_charter_auth_token"); if (existingToken) { + console.warn("Deeplink login network failure, falling back to existing token", error); window.location.href = "/"; return; }platforms/eReputation/client/src/pages/deeplink-login.tsx (1)
81-103: JSON-parse fallback using existing token can mask server issues and wrong-identity logins.On a 200 OK response, if
response.json()throws, the code falls back toexistingTokenand silently redirects home. That can hide broken server responses (schema regressions, HTML error pages, etc.) and also prevent a user from realizing that a new deeplink login (possibly for a different account) failed and they are still on the old token.A simpler, more predictable behavior would be to always treat a JSON parse failure as an error, regardless of any existing token:
- let data: any; - try { - data = await response.json(); - } catch (parseError) { - console.error("Failed to parse auth response JSON:", parseError); - // If token already exists, proceed; otherwise surface error - const existingToken = localStorage.getItem("ereputation_token"); - if (existingToken) { - window.location.href = "/"; - return; - } - setError("Invalid response from server"); - setIsLoading(false); - return; - } + let data: any; + try { + data = await response.json(); + } catch (parseError) { + console.error("Failed to parse auth response JSON:", parseError); + setError("Invalid response from server"); + setIsLoading(false); + return; + }If you still want to special‑case “already authenticated”, consider explicitly validating the existing token elsewhere and showing a brief “Already authenticated, redirecting…” message before navigating, rather than treating a parse failure as success.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
platforms/eReputation/client/src/lib/apiClient.ts(0 hunks)platforms/eReputation/client/src/pages/deeplink-login.tsx(3 hunks)platforms/group-charter-manager/src/app/deeplink-login/page.tsx(3 hunks)
💤 Files with no reviewable changes (1)
- platforms/eReputation/client/src/lib/apiClient.ts
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-21T15:49:12.904Z
Learnt from: coodos
Repo: MetaState-Prototype-Project/prototype PR: 458
File: platforms/eReputation-api/src/services/ReferenceSigningSessionService.ts:60-60
Timestamp: 2025-11-21T15:49:12.904Z
Learning: In platforms/eReputation-api/src/services/ReferenceSigningSessionService.ts, the VITE_EREPUTATION_BASE_URL environment variable intentionally has no fallback value. The service should fail if this environment variable is not set, following a fail-fast design pattern.
Applied to files:
platforms/eReputation/client/src/pages/deeplink-login.tsx
⏰ 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)
platforms/group-charter-manager/src/app/deeplink-login/page.tsx (1)
40-42: LGTM!The change from
consttoletis appropriate to support the re-parsing fallback logic below.
| // Add a small delay to allow URL to fully parse before deciding | ||
| await new Promise(resolve => setTimeout(resolve, 500)); | ||
| // Re-check one more time after delay | ||
| const finalParams = new URLSearchParams( | ||
| window.location.search || | ||
| (window.location.hash.includes('?') | ||
| ? window.location.hash.substring(window.location.hash.indexOf('?') + 1) | ||
| : '') || | ||
| '', | ||
| ); | ||
| ename = finalParams.get('ename') || ename; | ||
| session = finalParams.get('session') || session; | ||
| signature = finalParams.get('signature') || signature; | ||
|
|
||
| if (!ename || !session || !signature) { | ||
| // If still missing, silently redirect to home/login to avoid flashing error | ||
| window.location.href = "/"; | ||
| return; | ||
| } |
There was a problem hiding this comment.
Investigate root cause rather than adding arbitrary delays.
URLs are synchronously available in JavaScript—they don't require time to "parse." This 500ms delay appears to be a workaround for an undiagnosed timing issue, possibly related to how the mobile app delivers deep link parameters.
Concerns:
- The silent redirect to
"/"without any logging makes debugging difficult when parameters genuinely fail to arrive. - The re-parsing logic duplicates the earlier parsing block (lines 14-38).
Consider adding a console.warn before the silent redirect to aid debugging, and investigate the actual root cause (e.g., is the eID Wallet updating the URL after navigation?).
if (!ename || !session || !signature) {
// If still missing, silently redirect to home/login to avoid flashing error
+ console.warn("Deeplink login: Required params missing after retry, redirecting home");
window.location.href = "/";
return;
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In platforms/group-charter-manager/src/app/deeplink-login/page.tsx around lines
46 to 64, remove the arbitrary 500ms timeout and the duplicated parsing logic;
instead extract the URL parsing into a single helper used both places and call
it again deterministically (or subscribe to hashchange/popstate if deep-links
may arrive after navigation), and before performing the silent redirect keep the
redirect but add a console.warn (or process logger) that includes
window.location.href, window.location.search, window.location.hash and the
parsed ename/session/signature so missing-param cases are recorded for
debugging.
| const existingToken = localStorage.getItem("group_charter_auth_token"); | ||
| if (existingToken) { | ||
| window.location.href = "/"; | ||
| return; | ||
| } |
There was a problem hiding this comment.
Token-based redirect may mask server-side validation failures.
If the server returns a non-OK response (e.g., 401/403 indicating invalid credentials or revoked session), silently redirecting based on an existing local token could leave the user in an inconsistent state—they have a locally stored token that the server may no longer accept.
Consider at minimum logging this scenario, or validating whether the existing token is still valid before redirecting:
const existingToken = localStorage.getItem("group_charter_auth_token");
if (existingToken) {
+ console.warn("Deeplink login failed but existing token found, redirecting home", { status: response.status });
window.location.href = "/";
return;
}📝 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 existingToken = localStorage.getItem("group_charter_auth_token"); | |
| if (existingToken) { | |
| window.location.href = "/"; | |
| return; | |
| } | |
| const existingToken = localStorage.getItem("group_charter_auth_token"); | |
| if (existingToken) { | |
| console.warn("Deeplink login failed but existing token found, redirecting home", { status: response.status }); | |
| window.location.href = "/"; | |
| return; | |
| } |
🤖 Prompt for AI Agents
In platforms/group-charter-manager/src/app/deeplink-login/page.tsx around lines
115-119, the code unconditionally redirects when a token exists in localStorage
which can mask server-side invalidation; change this to validate the token
before redirecting by calling the relevant auth/validate or user/me endpoint
with the token, only navigate to "/" if the response is OK, and if the response
is not OK remove the token (or log the failure) and let the page proceed to show
the login flow; ensure network errors are handled (treat as invalid or retry)
and surface/log the validation failure for debugging.
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)
infrastructure/evault-core/src/controllers/VerificationController.ts (1)
231-239: Address non-deterministic selection when multiple duplicate verifications exist.The
find()at line 231 returns the first verification with alinkedEName, but if multiple verifications exist for the samedocumentIdwith differentlinkedENamevalues (sincefindManyAndCountreturns multiple matches), the selection is non-deterministic depending on the order returned by the database query.Also, the property name
linkedENameis semantically misleading—it stores eitherw3idvalues (during provisioning, see line 238) oreNamevalues (during device registration in NotificationService). Consider renaming this property to better reflect its dual purpose (e.g.,linkedIdentifierorlinkedW3ID), or separating these concerns into distinct properties.infrastructure/eid-wallet/src/routes/(auth)/login/+page.svelte (1)
1-335: Run the formatter locally to fix formatting issuesThe GitHub Actions "Check Code" job is failing because formatting checks detected issues in this file. Run the project's formatting pipeline locally (
pnpm run formatin theinfrastructure/eid-walletdirectory, or at the root) and commit the resulting changes to fix the CI failure.
♻️ Duplicate comments (6)
platforms/group-charter-manager/src/app/deeplink-login/page.tsx (4)
45-65: Arbitrary delay and missing logging (duplicate concern).This 500ms delay and retry logic was previously flagged. URLs are synchronously available—the delay suggests an undiagnosed timing issue with how the mobile app delivers parameters. The silent redirect without logging makes debugging impossible when parameters genuinely fail to arrive.
As recommended previously, add logging before the silent redirect:
if (!ename || !session || !signature) { // If still missing, silently redirect to home/login to avoid flashing error + console.warn("Deeplink login: Required params missing after retry", { + url: window.location.href, + search: window.location.search, + hash: window.location.hash, + ename, + session, + signature + }); window.location.href = "/"; return; }
98-100: Arbitrary 300ms reload delay remains unnecessary (duplicate concern).The previous review flagged the 100ms navigation delay as unnecessary. This 300ms delay before
reload()likely suffers from the same issue—setAuthTokenandsetAuthIdappear to be synchronous localStorage operations that complete immediately.Remove the arbitrary delay:
setAuthId(data.user.id); setAuthToken(data.token); - setTimeout(() => { - window.location.reload(); - }, 300); + window.location.reload();If tokens aren't available after reload, investigate the actual root cause in the authentication flow.
109-116: Token-based redirect masks authentication failures (duplicate concern).This was previously flagged: when the server returns non-OK (401/403), silently redirecting based on an existing token can leave the user in an inconsistent state. The local token may no longer be valid on the server.
As recommended previously, add logging and consider token validation:
const existingToken = localStorage.getItem("group_charter_auth_token"); if (existingToken) { + console.warn("Deeplink login failed but existing token found, redirecting home", { + status: response.status, + error: errorData.error + }); window.location.href = "/"; return; }
121-125: Token-based redirect masks network errors (duplicate concern).Similar to the previous concerns: when a network error occurs, silently redirecting based on an existing token masks the actual failure. This makes it impossible to distinguish between network issues, server errors, and authentication failures.
Add logging to capture the actual error:
} catch (error) { + console.error("Deeplink login: Network or request error", { error }); const existingToken = localStorage.getItem("group_charter_auth_token"); if (existingToken) { + console.warn("Falling back to existing token due to error"); window.location.href = "/"; return; }platforms/eReputation/client/src/pages/deeplink-login.tsx (2)
40-63: Avoid arbitrary delay + silent redirect when deeplink params are missing.The 500 ms delay plus second URL parse and silent redirect to
/hides real deeplink failures and makes the flow feel flaky. Users who clicked a login link get no indication that anything went wrong.Consider:
- Keeping at most a short, well‑documented retry (if you truly see late URL updates), and
- Always surfacing an explicit error when
ename/session/signatureare still missing instead of silently redirecting.A concrete adjustment that preserves the retry but avoids the silent failure:
- if (!ename || !session || !signature) { - // Add a small delay to allow URL to fully parse before deciding - await new Promise(resolve => setTimeout(resolve, 500)); - // Re-check one more time after delay - const finalParams = new URLSearchParams( - window.location.search || - (window.location.hash.includes('?') - ? window.location.hash.substring(window.location.hash.indexOf('?') + 1) - : '') || - '' - ); - ename = finalParams.get('ename') || ename; - session = finalParams.get('session') || session; - signature = finalParams.get('signature') || signature; - - if (!ename || !session || !signature) { - // If still missing, silently redirect to home/login to avoid flashing error - window.location.href = "/"; - return; - } - } + if (!ename || !session || !signature) { + // Optional: brief retry window in case URL params are populated asynchronously + await new Promise((resolve) => setTimeout(resolve, 500)); + const finalParams = new URLSearchParams( + window.location.search || + (window.location.hash.includes("?") + ? window.location.hash.substring(window.location.hash.indexOf("?") + 1) + : "") || + "" + ); + ename = finalParams.get("ename") || ename; + session = finalParams.get("session") || session; + signature = finalParams.get("signature") || signature; + } + + if (!ename || !session || !signature) { + setError("Missing required authentication parameters"); + setIsLoading(false); + return; + }
87-101: Silent redirect on existing token masks deeplink auth failures.Both the inner and outer
catchblocks redirect to/whenereputation_tokenexists, without validating it or informing the user. That can:
- Hide actual deeplink authentication problems.
- Leave users confused when a deeplink “does nothing”.
A minimal change is to remove the silent redirect and always surface the error; you can later enhance this to show a toast like “Already authenticated, redirecting…” if you add token validation:
- } catch (err) { - // If token already exists, silently continue to home - const existingToken = localStorage.getItem("ereputation_token"); - if (existingToken) { - window.location.href = "/"; - return; - } + } catch (err) { const axiosErr = err as any; const msg = axiosErr?.response?.data?.error || axiosErr?.message || "Failed to connect to server"; setError(msg); setIsLoading(false); } } catch (error) { - // If token already exists, silently continue to home - const existingToken = localStorage.getItem("ereputation_token"); - if (existingToken) { - window.location.href = "/"; - return; - } setError("Failed to connect to server"); setIsLoading(false); }If you do want to keep a redirect-on-valid-token flow, consider validating the token first and showing a brief user-visible message before navigating.
Also applies to: 103-108
🧹 Nitpick comments (5)
platforms/group-charter-manager/src/app/deeplink-login/page.tsx (1)
40-42: Consider extracting parameter parsing into a helper function.The mutation from
consttoletis necessary for the retry logic but indicates duplicated parsing code. Extract the parameter extraction logic (lines 14-38 and 49-55) into a reusable helper function to improve maintainability and reduce duplication.Example refactor:
+function parseDeeplinkParams(): { ename: string | null; session: string | null; signature: string | null } { + let searchString = window.location.search; + if (!searchString || searchString === '') { + const hash = window.location.hash; + if (hash && hash.includes('?')) { + searchString = hash.substring(hash.indexOf('?')); + } else { + try { + const fullUrl = new URL(window.location.href); + searchString = fullUrl.search; + } catch (e) { + // Ignore parsing errors + } + } + } + if (searchString.startsWith('?')) { + searchString = searchString.substring(1); + } + const params = new URLSearchParams(searchString); + return { + ename: params.get('ename'), + session: params.get('session'), + signature: params.get('signature') + }; +} + export default function DeeplinkLogin() {Then use it consistently:
- let ename = params.get('ename'); - let session = params.get('session'); - let signature = params.get('signature'); + let { ename, session, signature } = parseDeeplinkParams();platforms/eReputation/client/src/pages/deeplink-login.tsx (1)
2-2: ReuseapiClientbaseURL and make config/version behavior explicit.You now import
apiClientbut still readVITE_EREPUTATION_BASE_URLdirectly to buildloginUrl. GivenapiClientis already configured with a base URL inplatforms/eReputation/client/src/lib/apiClient.ts, this duplicates configuration and increases the chance of drift.You can simplify and centralize config by relying on the client’s baseURL:
- // Make POST request to login endpoint using axios (auto JSON parsing) - const apiBaseUrl = import.meta.env.VITE_EREPUTATION_BASE_URL; - const loginUrl = `${apiBaseUrl}/api/auth`; - const requestBody = { ename, session, signature, appVersion: appVersion || '0.4.0' }; + // Make POST request to login endpoint using shared apiClient + const requestBody = { ename, session, signature, appVersion: appVersion || "0.4.0" }; try { - const { data } = await apiClient.post(loginUrl, requestBody); + const { data } = await apiClient.post("/api/auth", requestBody);Additionally, two small follow-ups you might want to confirm:
- If
VITE_EREPUTATION_BASE_URLis considered mandatory on the API side (fail-fast pattern), consider mirroring that on the client by surfacing a clear “client misconfigured” error if the baseURL is missing, rather than a generic connection failure. Based on learnings, this keeps misconfigurations obvious.- With eID Wallet bumped to v0.5.0, double-check that the fallback
appVersion || '0.4.0'is still the right default (or update it to the new minimum supported version).Also applies to: 70-77, 73-73
infrastructure/eid-wallet/src/routes/(auth)/login/+page.svelte (3)
153-157: Avoid potentially calling handlePinInput twice for the same PIN entry
handlePinInput(pin)is invoked both from the$effectreacting topinchanges (Lines 153–157) and via theonchangeprop on<InputPin>(Line 290). Depending on howInputPinfiresonchange, this can result in duplicated verification and post-auth work (e.g., two health checks and twogoto()calls) for a single 4‑digit PIN entry.Consider centralizing the trigger so that exactly one path calls
handlePinInputper user action—for example, either:
- rely solely on the
$effectthat watchespin, or- rely solely on
onchangefromInputPinand remove the$effect.Please verify how
InputPincurrently uses theonchangeprop to avoid unintended duplicate calls.Also applies to: 290-290
100-103: Reset loading state when eVault is deleted to avoid misleading “Logging you in...” UIIn both PIN and biometric flows, when
healthCheck.deletedis true you setisDeletedVaultModalOpen = trueand thenreturn(Lines 100–103, 185–188). BecauseisPostAuthLoadingremainstrue, the background login UI continues to show the loading spinner and “Logging you in...” subtitle (Lines 247–251, 255–260) even though the user is now blocked on the non-dismissible “eVault Has Been Deleted” Drawer.Functionally this works, but the messaging is slightly misleading. Consider explicitly resetting the loading flag when you show the deleted‑vault modal, e.g.:
- if (healthCheck.deleted) { - isDeletedVaultModalOpen = true; - return; // Don't continue to app - } + if (healthCheck.deleted) { + isDeletedVaultModalOpen = true; + isPostAuthLoading = false; + return; // Don't continue to app + }…and apply the same change in the biometric branch. That keeps the visual state aligned with the fact that the login flow is no longer progressing.
Also applies to: 185-188, 247-251, 255-260
85-147: Consider extracting shared post-auth flow (health check + deep-link + navigation)The PIN and biometric branches both perform essentially the same steps after successful auth: eVault health check, optional public-key sync, pending deep-link processing, and final navigation to
/scan-qror/main. The duplication is correct but makes it easy for the two paths to drift over time.If you end up revisiting this area, consider factoring that sequence into a shared helper (e.g.,
await completePostAuth(globalState)), which can encapsulateisDeletedVaultModalOpenhandling, deep-link routing, and final navigation in one place.Also applies to: 170-232
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
infrastructure/eid-wallet/src-tauri/gen/android/app/arm64/release/app-arm64-release.apkis excluded by!**/gen/**
📒 Files selected for processing (4)
infrastructure/eid-wallet/src/routes/(auth)/login/+page.svelte(6 hunks)infrastructure/evault-core/src/controllers/VerificationController.ts(1 hunks)platforms/eReputation/client/src/pages/deeplink-login.tsx(2 hunks)platforms/group-charter-manager/src/app/deeplink-login/page.tsx(3 hunks)
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-11-21T15:49:12.904Z
Learnt from: coodos
Repo: MetaState-Prototype-Project/prototype PR: 458
File: platforms/eReputation-api/src/services/ReferenceSigningSessionService.ts:60-60
Timestamp: 2025-11-21T15:49:12.904Z
Learning: In platforms/eReputation-api/src/services/ReferenceSigningSessionService.ts, the VITE_EREPUTATION_BASE_URL environment variable intentionally has no fallback value. The service should fail if this environment variable is not set, following a fail-fast design pattern.
Applied to files:
platforms/eReputation/client/src/pages/deeplink-login.tsx
📚 Learning: 2025-11-14T17:54:47.719Z
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.719Z
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
🧬 Code graph analysis (2)
platforms/eReputation/client/src/pages/deeplink-login.tsx (2)
platforms/eReputation/client/src/lib/apiClient.ts (1)
apiClient(5-10)platforms/eCurrency/client/src/lib/apiClient.ts (1)
apiClient(5-10)
infrastructure/evault-core/src/controllers/VerificationController.ts (1)
platforms/dreamSync/shared/schema.ts (1)
matches(109-117)
🪛 GitHub Actions: Check Code
infrastructure/eid-wallet/src/routes/(auth)/login/+page.svelte
[error] 1-1: Build/check process failed due to formatter errors detected by svelte-check. Command 'pnpm run check' exited with code 1.
⏰ 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: test
- GitHub Check: build
- GitHub Check: test-web3-adapter-integration
🔇 Additional comments (1)
infrastructure/eid-wallet/src/routes/(auth)/login/+page.svelte (1)
16-16: isPostAuthLoading reentrancy guard and UX wiring look solidThe new
isPostAuthLoadingflag is consistently used to prevent re-entry during post-auth work (PIN + biometrics), swap the PIN UI for a clear “Logging you in...” state, and hide the Clear PIN debug action while navigation is in flight. I don’t see logical gaps or race issues in how this flag is set/reset across the flows; this is a good UX and correctness improvement. Keeping Clear PIN one-click but only when not loading also matches the intended debug-only nature of that action. Based on learnings, this aligns with prior guidance on the Clear PIN button behavior.Also applies to: 65-69, 71-76, 81-82, 148-151, 168-169, 235-236, 247-251, 255-260, 261-295, 296-300
| const [matches] = | ||
| await this.verificationService.findManyAndCount({ | ||
| documentId: | ||
| body.data.verification.document.number | ||
| .value, | ||
| }); | ||
| console.log("matched", verificationMatch); | ||
| const verificationMatch = matches.find( | ||
| (v) => !!v.linkedEName, | ||
| ); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, find the verification service to understand findManyAndCount implementation
fd -e "ts" -e "js" | grep -i "verification.*service" | head -20Repository: MetaState-Prototype-Project/prototype
Length of output: 292
🏁 Script executed:
# Search for findManyAndCount method definition
rg -n "findManyAndCount" --type ts -A 10Repository: MetaState-Prototype-Project/prototype
Length of output: 3987
🏁 Script executed:
# Check the controller method context around lines 225-233 to see if id is accessible
sed -n '200,240p' infrastructure/evault-core/src/controllers/VerificationController.tsRepository: MetaState-Prototype-Project/prototype
Length of output: 1833
🏁 Script executed:
# Verify the exact context around line 220 where id is available
sed -n '215,245p' infrastructure/evault-core/src/controllers/VerificationController.tsRepository: MetaState-Prototype-Project/prototype
Length of output: 1543
🏁 Script executed:
# Check if there's any other filtering logic that might exclude the current verification
rg -n "documentId" infrastructure/evault-core/src/controllers/VerificationController.ts -B 5 -A 5Repository: MetaState-Prototype-Project/prototype
Length of output: 1206
🏁 Script executed:
# Double-check if there are any other places where the current verification id might be excluded
rg -n "findManyAndCount" infrastructure/evault-core/src/controllers/VerificationController.ts -B 3 -A 10Repository: MetaState-Prototype-Project/prototype
Length of output: 875
Exclude the current verification from duplicate detection to prevent self-matching.
The duplicate detection queries for all verifications with matching documentId but does not exclude the current verification. If the current verification already has a linkedEName (from previous processing), it will be included in the matches array and matched against itself, causing a false positive where it marks itself as a duplicate.
Filter the results to exclude the current verification by id:
const verificationMatch = matches.find(
- (v) => !!v.linkedEName,
+ (v) => v.id !== id && !!v.linkedEName,
);Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
in infrastructure/evault-core/src/controllers/VerificationController.ts around
lines 225-233, the duplicate-detection code queries verifications by documentId
but does not exclude the current verification, so it can self-match; update the
logic to exclude the current verification by id (either add an exclusion
condition to the findManyAndCount query using body.data.verification.id or
immediately filter the returned matches with matches = matches.filter(m => m.id
!== currentVerificationId)) and then perform the .find(v => !!v.linkedEName) on
the filtered list; also add a defensive check that body.data.verification.id
exists before excluding.
| let data: any; | ||
| try { | ||
| data = await response.json(); | ||
| } catch { | ||
| const existingToken = localStorage.getItem("group_charter_auth_token"); | ||
| if (existingToken) { | ||
| window.location.href = "/"; | ||
| return; | ||
| } | ||
| setError("Invalid response from server"); | ||
| setIsLoading(false); | ||
| return; | ||
| } |
There was a problem hiding this comment.
Token-based redirect masks JSON parse errors from successful responses.
When the server returns 200 OK but invalid JSON, that indicates a server-side bug. Silently redirecting based on an existing token masks this error, making it impossible to detect and diagnose server issues.
Add logging to capture parse failures:
} catch {
+ console.error("Deeplink login: JSON parse failed on successful response", {
+ status: response.status,
+ contentType: response.headers.get('content-type')
+ });
const existingToken = localStorage.getItem("group_charter_auth_token");
if (existingToken) {
+ console.warn("Falling back to existing token due to parse error");
window.location.href = "/";
return;
}🤖 Prompt for AI Agents
In platforms/group-charter-manager/src/app/deeplink-login/page.tsx around lines
82 to 94, the JSON parse catch silently redirects to "/" when a token exists,
which masks server-side parse errors; update the catch to capture and log the
parsing error (e.g., console.error or send to telemetry) including the response
status and body when possible, and then decide flow: either setError("Invalid
response from server") and stop loading so the error is visible, or if you must
keep the existing redirect fallback still perform the redirect only after
logging the detailed parse error so server issues can be diagnosed.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (5)
infrastructure/eid-wallet/src/routes/(app)/scan-qr/scanLogic.ts (3)
1295-1310: Missing error handling for non-ok poll response.This issue was flagged in a previous review. If
pollResponse.okis false (e.g., 404, 500), no error is set andsigningData.pollDetailsremains as "Loading poll details..." indefinitely.
1355-1357: Outer catch block swallows errors without user feedback.This issue was flagged in a previous review. If an unexpected error occurs, it's only logged to console. Users receive no indication that something went wrong.
1224-1226: Missing error feedback when sign parameters are invalid.This issue was flagged in a previous review. If the condition at line 1224 evaluates to false (missing session, data, or redirect_uri), the function silently does nothing and the user receives no feedback.
platforms/blabsy/src/pages/deeplink-login.tsx (2)
102-108: Potential double-navigation race.As flagged in a previous review, this navigation block may race with the
usereffect (lines 136-144) sincesignInWithCustomTokenupdates auth state. Consider removing this explicit navigation and relying solely on the user effect, or vice versa.
135-144: Part of the double-navigation race.This effect triggers navigation when auth state changes, which occurs immediately after the navigation at lines 102-108. Consider consolidating to a single navigation approach as suggested in the previous review.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
infrastructure/eid-wallet/src/routes/(app)/scan-qr/+page.svelte(2 hunks)infrastructure/eid-wallet/src/routes/(app)/scan-qr/scanLogic.ts(2 hunks)platforms/blabsy/src/pages/deeplink-login.tsx(4 hunks)platforms/pictique/src/routes/(auth)/deeplink-login/+page.svelte(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- infrastructure/eid-wallet/src/routes/(app)/scan-qr/+page.svelte
- platforms/pictique/src/routes/(auth)/deeplink-login/+page.svelte
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-21T15:49:12.904Z
Learnt from: coodos
Repo: MetaState-Prototype-Project/prototype PR: 458
File: platforms/eReputation-api/src/services/ReferenceSigningSessionService.ts:60-60
Timestamp: 2025-11-21T15:49:12.904Z
Learning: In platforms/eReputation-api/src/services/ReferenceSigningSessionService.ts, the VITE_EREPUTATION_BASE_URL environment variable intentionally has no fallback value. The service should fail if this environment variable is not set, following a fail-fast design pattern.
Applied to files:
infrastructure/eid-wallet/src/routes/(app)/scan-qr/scanLogic.ts
🧬 Code graph analysis (1)
infrastructure/eid-wallet/src/routes/(app)/scan-qr/scanLogic.ts (1)
platforms/pictique/src/lib/stores/posts.ts (1)
error(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). (3)
- GitHub Check: build
- GitHub Check: test-web3-adapter-integration
- GitHub Check: test
🔇 Additional comments (5)
infrastructure/eid-wallet/src/routes/(app)/scan-qr/scanLogic.ts (4)
335-341: LGTM!Non-blocking navigation to home before triggering external deep links is a reasonable UX pattern. The fire-and-forget approach with error logging is appropriate since the external URL opening is the primary action.
1154-1187: Good defensive additions.The globalState availability check prevents runtime errors, and the duplicate request guards using drawer state are a sensible approach to prevent overlapping requests from causing UI inconsistencies.
1189-1211: LGTM!The auth deep link handling properly closes other modals first, has defensive hostname parsing with a fallback, and correctly distinguishes between scan and deep link origins.
1332-1354: LGTM!The reveal deep link handling properly closes other modals, validates the pollId, and provides appropriate error feedback when the pollId is missing.
platforms/blabsy/src/pages/deeplink-login.tsx (1)
7-11: LGTM on component setup.The component correctly uses
useRouterfromnext/routerfor Pages Router and destructures the necessary auth context values.
| } catch (error) { | ||
| console.error("Error decoding signing data:", error); | ||
| return; | ||
| } |
There was a problem hiding this comment.
Silent failure on decode error leaves user without feedback.
When base64 decoding or JSON parsing fails at line 1328, the function logs the error and returns without setting signingError. The user sees no indication of why the signing request failed.
} catch (error) {
console.error("Error decoding signing data:", error);
+ signingError.set(
+ "Failed to decode signing data. The deep link may be invalid.",
+ );
return;
}🤖 Prompt for AI Agents
in infrastructure/eid-wallet/src/routes/(app)/scan-qr/scanLogic.ts around lines
1327-1330, the catch block currently logs the decode/parse error and returns
silently leaving the UI without feedback; update the catch to set the
component/state variable signingError with a user-friendly message (e.g.,
"Failed to decode signing request — invalid data"), optionally include the
original error for developer logs (processLogger.error or console.error), and
ensure any cleanup/state resets are done before returning so the UI shows the
error to the user.
| if (!ename || !session || !signature) { | ||
| setError('Missing required authentication parameters'); | ||
| setIsLoading(false); | ||
| return; | ||
| // Add a small delay to allow URL to fully parse before showing error | ||
| await new Promise((resolve) => setTimeout(resolve, 500)); | ||
| // Re-check one more time after delay | ||
| const finalParams = new URLSearchParams( | ||
| window.location.search || | ||
| (window.location.hash.includes('?') | ||
| ? window.location.hash.substring( | ||
| window.location.hash.indexOf('?') + 1 | ||
| ) | ||
| : '') || | ||
| '' | ||
| ); | ||
| ename = finalParams.get('ename') || ename; | ||
| session = finalParams.get('session') || session; | ||
| signature = finalParams.get('signature') || signature; | ||
|
|
||
| if (!ename || !session || !signature) { | ||
| setError('Missing required authentication parameters'); | ||
| setIsLoading(false); | ||
| return; | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Avoid hardcoded delay; use router.isReady instead.
The 500ms delay is an unreliable timing-based workaround. In Next.js Pages Router, the router may not be hydrated on initial render, which is likely the root cause of the missing parameters. Use router.isReady to guard parameter access.
- useEffect(() => {
+ useEffect(() => {
+ // Wait for router to be ready before accessing URL params
+ if (!router.isReady) return;
+
const handleDeeplinkLogin = async () => {Then remove the delay-based fallback logic:
if (!ename || !session || !signature) {
- // Add a small delay to allow URL to fully parse before showing error
- await new Promise((resolve) => setTimeout(resolve, 500));
- // Re-check one more time after delay
- const finalParams = new URLSearchParams(
- window.location.search ||
- (window.location.hash.includes('?')
- ? window.location.hash.substring(
- window.location.hash.indexOf('?') + 1
- )
- : '') ||
- ''
- );
- ename = finalParams.get('ename') || ename;
- session = finalParams.get('session') || session;
- signature = finalParams.get('signature') || signature;
-
- if (!ename || !session || !signature) {
- setError('Missing required authentication parameters');
- setIsLoading(false);
- return;
- }
+ setError('Missing required authentication parameters');
+ setIsLoading(false);
+ return;
}And update the dependency array:
- }, [signInWithCustomToken, router]);
+ }, [signInWithCustomToken, router, router.isReady]);Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In platforms/blabsy/src/pages/deeplink-login.tsx around lines 48 to 70, replace
the hardcoded 500ms setTimeout retry logic with a guard that waits for Next.js
router to be ready: remove the delay and the manual URLSearchParams re-check,
and instead only read ename/session/signature after router.isReady is true (or
return early from the effect until router.isReady). Also update the useEffect
dependency array to include router.isReady (and router or router.query as
appropriate) so the effect re-runs when the router becomes ready.
Description of change
Issue Number
Type of change
How the change has been tested
Change checklist
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Chores
✏️ Tip: You can customize this high-level summary in your review settings.