feat: better deeplink login context handling #567
Conversation
|
Warning Rate limit exceeded@coodos has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 13 minutes and 4 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (3)
WalkthroughAdds an Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
infrastructure/eid-wallet/src/routes/(app)/scan-qr/scanLogic.ts (2)
297-297: Extract hardcoded app version to a constant.The app version
"0.4.0"is hardcoded in two places. Consider extracting it to a named constant at the module level for easier maintenance and consistency.Apply this diff to extract the version:
+const APP_VERSION = "0.4.0"; + export interface SigningData {Then update the usage:
const payload = { ename: vault.ename, session: get(session) as string, signature: signature, - appVersion: "0.4.0", + appVersion: APP_VERSION, };loginUrl.searchParams.set("ename", vault.ename); loginUrl.searchParams.set("session", get(session) as string); loginUrl.searchParams.set("signature", signature); - loginUrl.searchParams.set("appVersion", "0.4.0"); + loginUrl.searchParams.set("appVersion", APP_VERSION);Also applies to: 331-331
303-315: Add timeout to fetch request.The fetch request to submit authentication has no timeout, which could cause the request to hang indefinitely if the server is unresponsive. Consider adding a timeout using
AbortControlleror request configuration.Example implementation:
+ const controller = new AbortController(); + const timeoutId = setTimeout(() => controller.abort(), 10000); // 10 second timeout + const response = await fetch(redirectUrl, { method: "POST", headers: { "Content-Type": "application/json", }, body: JSON.stringify(payload), + signal: controller.signal, }); + + clearTimeout(timeoutId);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
infrastructure/eid-wallet/src/routes/(app)/scan-qr/scanLogic.ts(6 hunks)
⏰ 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 (4)
infrastructure/eid-wallet/src/routes/(app)/scan-qr/scanLogic.ts (4)
82-82: LGTM: Store declaration and initialization.The
isFromScanstore is properly declared in the interface, initialized tofalse, and exported in the stores object, consistent with other boolean flags in the file.Also applies to: 147-147, 1440-1440
1192-1192: LGTM: Correctly marks deeplink-origin auth.The flag is properly set to
falsefor deeplink-origin authentication requests, ensuring the correct flow is taken inhandleAuth.
291-323: Verify intentional bypass of sessionStorage deeplink processing.The scan flow returns early at line 323, skipping the sessionStorage deeplink processing logic (lines 342-419) that handles navigation after auth completion. This means scan-origin authentications don't process any pending deeplink data. Confirm this is intentional, as it changes the navigation behavior for scan-based auth compared to the previous implementation.
If scan-origin auth should also process pending deeplinks, the early return should be moved after the sessionStorage processing block.
460-460: No action required—flag state management is correct.The
isFromScanflag is properly managed across authentication flows. It is explicitly set totruefor scan-origin requests (line 460) and tofalsefor deeplink-origin requests (line 1192). Each new auth request establishes the correct flag state, preventing any state carryover issues between scan and deeplink authentications.
coodos
force-pushed
the
feat/tauri-deeplink-issue
branch
from
93a92c5 to
5552f33
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
infrastructure/eid-wallet/src/routes/(app)/scan-qr/scanLogic.ts (1)
82-82:isFromScanorigin tracking is wired consistently; consider resetting on teardownThe
isFromScanstore is added toScanStores, initialized, toggled inhandleAuthRequest(scan →true) andhandleDeepLinkDataforauth(deeplink →false), and exported viastores, sohandleAuthand any consumers can reliably distinguish origin. Functionally this is sound.If you expect external consumers to treat
isFromScanas “origin of the current auth flow” rather than “origin of the last auth flow”, consider resetting it (e.g. tofalse) in teardown paths likecloseDrawer,handleSuccessOkay, and at the end ofhandleAuth(both branches). That would avoid any chance of staletruelingering when no auth is active.Also applies to: 147-147, 451-462, 1171-1193, 1440-1440
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
infrastructure/eid-wallet/src/routes/(app)/scan-qr/scanLogic.ts(6 hunks)
⏰ 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)
infrastructure/eid-wallet/src/routes/(app)/scan-qr/scanLogic.ts (1)
289-337: Scan vs. deeplink branching inhandleAuthlooks correct and robustThe new
fromScanbranch cleanly separates scan‑origin auth (server POST with JSON, early return) from deeplink‑origin auth (construct/deeplink-loginURL and open it), while reusing the same signedsessionpayload and keeping existing error handling. Early‑returning for the scan case avoids accidentally running the deep‑link redirect logic with staledeepLinkData, which matches the PR’s stated intent.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
infrastructure/evault-core/src/core/http/server.ts (1)
174-183: Consider simplifying redundant logging.The individual warnings for missing
registryUrl,sharedSecret, or no keys are redundant with the configuration status already logged at lines 130-131. Consider removing or consolidating these to reduce log noise.Apply this diff to remove redundant warnings:
- } else { - if (!registryUrl) { - console.warn("[WHOIS] Cannot generate certificates: Registry URL not configured"); - } - if (!sharedSecret) { - console.warn("[WHOIS] Cannot generate certificates: Shared secret not configured"); - } - if (publicKeys.length === 0) { - console.log("[WHOIS] No public keys to generate certificates for"); - } - } + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
infrastructure/eid-wallet/src/lib/global/controllers/evault.ts(5 hunks)infrastructure/evault-core/src/core/http/server.ts(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- infrastructure/eid-wallet/src/lib/global/controllers/evault.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: test
- GitHub Check: build
🔇 Additional comments (4)
infrastructure/evault-core/src/core/http/server.ts (4)
97-99: LGTM! Clear request/response framing.The visual separators and consistent
[WHOIS]prefix make it easy to trace individual requests in logs.Also applies to: 190-191
106-123: LGTM! Comprehensive database retrieval logging.The logging provides good visibility into key retrieval outcomes without exposing full keys. Error handling remains sound.
130-131: LGTM! Configuration status visibility.Logging config presence without exposing values is good practice for troubleshooting.
136-166: LGTM! Per-certificate progress tracking.The loop change from
for-ofto indexedforenables clear per-iteration logging. Error handling correctly continues processing remaining keys when individual certificate generation fails.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
infrastructure/eid-wallet/src/lib/global/controllers/evault.ts(1 hunks)infrastructure/evault-core/src/core/http/server.ts(1 hunks)
⏰ 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: test-web3-adapter-integration
- GitHub Check: build
🔇 Additional comments (1)
infrastructure/eid-wallet/src/lib/global/controllers/evault.ts (1)
122-123: LGTM! Correctly consumes the updated API schema.The code correctly retrieves
keyBindingCertificatesas an array from the whois response, which aligns with the schema change ininfrastructure/evault-core/src/core/http/server.ts.
Description of change
Issue Number
Type of change
How the change has been tested
Change checklist
Summary by CodeRabbit
New Features
Bug Fixes
Chores
✏️ Tip: You can customize this high-level summary in your review settings.