fix: reset-pin #417
fix: reset-pin #417
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 22 minutes and 10 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughAdds context retrieval and onMount initialization in the PIN settings page; strengthens PIN change handler with input length validation, match checks, and try/catch around securityController.updatePin; reintroduces an effect to set the header title. Also adjusts a UI label's markup, pins React packages via pnpm overrides, and adds a CI workaround step plus Node 20 in the GitHub Actions build workflow. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Form as PIN Form
participant Validator as Input Validator
participant Controller as Security Controller
participant UI as UI State
User->>Form: Submit (currentPin, newPin, repeatPin)
Form->>Validator: Validate lengths (current, new, repeat)
alt Validation fails
Validator-->>Form: validation error
Form->>UI: set isError = true
UI-->>User: show validation feedback
else Validation passes
Validator-->>Form: OK
Form->>Form: check newPin === repeatPin
alt Mismatch
Form->>UI: set isError = true
UI-->>User: show mismatch error
else Match
Form->>Controller: updatePin(currentPin, newPin)
alt Success
Controller-->>Form: success
Form->>UI: clear isError, show success Drawer
UI-->>User: show success message
else Failure
Controller-->>Form: error
Form->>UI: log error, set isError = true
UI-->>User: show error feedback
end
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (3 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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
infrastructure/eid-wallet/src/routes/(app)/settings/pin/+page.svelte (1)
62-62: Fix typo in user-facing text.Line 62 contains a grammatical error: "Enter you current PIN" should be "Enter your current PIN".
- <p class="mb-[1svh]">Enter you current PIN</p> + <p class="mb-[1svh]">Enter your current PIN</p>
🧹 Nitpick comments (4)
infrastructure/eid-wallet/src/routes/(app)/settings/pin/+page.svelte (4)
22-29: Consider adding numeric-only validation for PIN inputs.The current validation only checks PIN length but doesn't verify that the inputs contain only numeric characters. While the
InputPincomponent may handle this, explicit validation here would improve robustness and provide clearer error messages if non-numeric input is somehow entered.Add numeric validation:
const handleChangePIN = async () => { + const isNumeric = (pin: string) => /^\d+$/.test(pin); + + if (!isNumeric(currentPin) || !isNumeric(newPin) || !isNumeric(repeatPin)) { + errorMessage = "PIN must contain only numbers."; + return; + } + if ( newPin.length < 4 || repeatPin.length < 4 || currentPin.length < 4 ) {
36-43: Add loading state to prevent duplicate submissions.The async
updatePincall has no loading indicator, allowing users to repeatedly click "Change PIN" during the operation. This could result in multiple concurrent update attempts.Add a loading state:
+ let isLoading = $state(false); ... const handleChangePIN = async () => { + if (isLoading) return; + if ( newPin.length < 4 || repeatPin.length < 4 || currentPin.length < 4 ) { isError = true; return; } if (newPin !== repeatPin) { isError = true; return; } try { + isLoading = true; await globalState?.securityController.updatePin(currentPin, newPin); isError = false; showDrawer = true; } catch (err) { console.error("Failed to update PIN:", err); isError = true; + } finally { + isLoading = false; } };Then disable the button during loading:
- <ButtonAction class="w-full" callback={handleChangePIN} + <ButtonAction class="w-full" callback={handleChangePIN} disabled={isLoading} >Change PIN</ButtonAction >
46-49: Error clearing logic is overly specific and may not handle all cases.The
$effectclearsisErroronly whenrepeatPinis exactly 4 characters and matchesnewPin. This doesn't clear errors when:
- The user corrects the
currentPinlength after an initial validation failure- The user is entering a PIN longer than 4 characters and they match
- A previous error was from a failed
updatePincall (server-side error)If you implement the
errorMessageapproach suggested earlier, consider clearing the error message as users type, providing real-time feedback:$effect(() => { runtime.header.title = "Change PIN"; - if (repeatPin.length === 4 && newPin === repeatPin) isError = false; + // Clear error when user fixes the validation issues + if (currentPin.length >= 4 && newPin.length >= 4 && repeatPin.length >= 4) { + if (newPin === repeatPin && errorMessage.includes("match")) { + errorMessage = ""; + } else if (errorMessage.includes("at least 4 characters")) { + errorMessage = ""; + } + } });
36-43: Consider clearing PIN fields after successful update.After a successful PIN change, the input fields retain their values. If the user dismisses the success drawer, the old and new PINs remain visible in the form. For better security and UX, consider clearing all fields after a successful update.
try { await globalState?.securityController.updatePin(currentPin, newPin); isError = false; showDrawer = true; + currentPin = ""; + newPin = ""; + repeatPin = ""; } catch (err) { console.error("Failed to update PIN:", err); isError = true; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
infrastructure/eid-wallet/src/routes/(app)/settings/pin/+page.svelte(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: sosweetham
Repo: MetaState-Prototype-Project/prototype PR: 97
File: infrastructure/eid-wallet/src/lib/global/controllers/security.ts:86-93
Timestamp: 2025-04-24T06:00:34.296Z
Learning: Rate limiting for PIN verification is not part of the design requirements for the SecurityController in the eid-wallet application, and should not be suggested as an enhancement.
⏰ 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
infrastructure/eid-wallet/src/routes/(app)/settings/pin/+page.svelte
Outdated
Show resolved
Hide resolved
Description of change
Fixed reset pin
Issue Number
closes #408
Type of change
How the change has been tested
Manual
Change checklist
Summary by CodeRabbit
Bug Fixes
Style
Chores