[TASK-16189] feat: add browser not supported modal on invites page#1397
[TASK-16189] feat: add browser not supported modal on invites page#1397Zishan-7 merged 5 commits intopeanut-wallet-devfrom
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
WalkthroughAdds a PasskeySupport context/provider and a Jest mock for simplewebauthn; refactors UnsupportedBrowserModal to consume the new context (with loading state and timeout cleanup), and renders the modal (non-closable) inside InvitesPageLayout. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/components/Invites/InvitesPage.tsx(3 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: Zishan-7
Repo: peanutprotocol/peanut-ui PR: 1072
File: src/app/(setup)/setup/page.tsx:173-175
Timestamp: 2025-08-07T12:53:50.946Z
Learning: In the peanut-ui setup flow at `src/app/(setup)/setup/page.tsx`, when handling unsupported scenarios, both device not supported and browser not supported cases should show the same "Unsupported browser" message using the `UnsupportedBrowserModal` component, rather than having distinct messaging for each scenario.
📚 Learning: 2025-08-07T12:53:50.946Z
Learnt from: Zishan-7
Repo: peanutprotocol/peanut-ui PR: 1072
File: src/app/(setup)/setup/page.tsx:173-175
Timestamp: 2025-08-07T12:53:50.946Z
Learning: In the peanut-ui setup flow at `src/app/(setup)/setup/page.tsx`, when handling unsupported scenarios, both device not supported and browser not supported cases should show the same "Unsupported browser" message using the `UnsupportedBrowserModal` component, rather than having distinct messaging for each scenario.
Applied to files:
src/components/Invites/InvitesPage.tsx
🧬 Code graph analysis (1)
src/components/Invites/InvitesPage.tsx (1)
src/hooks/usePasskeySupport.ts (1)
usePasskeySupport(21-102)
⏰ 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: Deploy-Preview
🔇 Additional comments (1)
src/components/Invites/InvitesPage.tsx (1)
18-19: LGTM! Imports are correct.The necessary imports for browser support gating are properly added.
|
@coderabbitai review in depth. Evaluate:
Also, write a very concise testing guide for QA |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 17355 @Hugo0, I've completed an in-depth review of this PR. Here's my analysis: 🔴 Risk Assessment: MEDIUM-HIGHCritical Issues:
Non-Breaking Changes:✅ Existing ⚡ Performance Impact: LOW-MEDIUMConcerns:
Latency:
Recommendation: Lift the hook to a context provider and cache results globally: // Create PasskeySupportProvider
const PasskeySupportContext = createContext<PasskeySupportResult | null>(null)
export const PasskeySupportProvider = ({ children }) => {
const support = usePasskeySupport()
return <PasskeySupportContext.Provider value={support}>{children}</PasskeySupportContext.Provider>
}
export const usePasskeySupportContext = () => {
const context = useContext(PasskeySupportContext)
if (!context) throw new Error('usePasskeySupportContext must be used within PasskeySupportProvider')
return context
}🎨 Code Quality & DRY: NEEDS IMPROVEMENTDRY Violations:
Recommendations:Option A - Self-Contained Modal (Preferred): // InvitesPage.tsx - Remove usePasskeySupport
<UnsupportedBrowserModal allowClose={false} />
// Modal handles detection internally - no visible prop neededOption B - Parent-Controlled Modal: // Remove usePasskeySupport from UnsupportedBrowserModal
// Rely only on visible prop from parent
<UnsupportedBrowserModal allowClose={false} visible={!isCheckingPasskeySupport && !isPasskeySupported} />I recommend Option A - keeps the modal self-contained and reduces parent complexity. Positive Aspects:✅ Proper 🧪 QA Testing GuideTest Scenarios:1. Supported Browser (Desktop)
2. Unsupported Browser
3. In-App Browser
4. Copy Link Functionality
5. With redirect_uri Parameter
6. Loading State
7. Already Logged-In User with App Access
8. Invalid Invite Code
📋 Summary
Recommendation: Refactor to eliminate duplicate hook calls before merging. The functionality works but has technical debt that should be addressed. 🧠 Learnings used |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/utils/__mocks__/simplewebauthn-browser.ts (1)
1-32: Consider adding explicit return types for better type safety.The mocks use simplified data types (strings instead of
ArrayBuffer/Uint8Array) which is acceptable for testing, but adding explicit TypeScript return types would improve type safety and make the mock contract clearer. Since these arejest.fn()wrappers, individual tests can still override these implementations for error scenarios.Example for improved type safety:
import type { RegistrationResponseJSON, AuthenticationResponseJSON } from '@simplewebauthn/browser' export const startRegistration = jest.fn((): Promise<RegistrationResponseJSON> => Promise.resolve({ id: 'mock-credential-id', rawId: 'mock-raw-id', response: { clientDataJSON: 'mock-client-data', attestationObject: 'mock-attestation', }, type: 'public-key', } as RegistrationResponseJSON) )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
package.json(1 hunks)src/utils/__mocks__/simplewebauthn-browser.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: Zishan-7
Repo: peanutprotocol/peanut-ui PR: 1072
File: src/app/(setup)/setup/page.tsx:173-175
Timestamp: 2025-08-07T12:53:50.946Z
Learning: In the peanut-ui setup flow at `src/app/(setup)/setup/page.tsx`, when handling unsupported scenarios, both device not supported and browser not supported cases should show the same "Unsupported browser" message using the `UnsupportedBrowserModal` component, rather than having distinct messaging for each scenario.
⏰ 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: Deploy-Preview
🔇 Additional comments (3)
package.json (1)
152-152: LGTM! Jest mock mapping correctly configured.The moduleNameMapper entry follows the established pattern and correctly points to the new mock implementation.
src/utils/__mocks__/simplewebauthn-browser.ts (2)
34-40: Encoding/decoding mocks are appropriately simplified for testing.These utility functions return dummy values rather than performing actual encoding/decoding, which is acceptable for most test scenarios. Tests requiring specific encoded values can override these mocks.
42-53: LGTM! Error and service classes properly implemented.The
WebAuthnErrorclass correctly extendsErrorwith proper name assignment. TheWebAuthnAbortServiceuses a realAbortControllerrather than a mock, which is acceptable sinceAbortControlleris a standard browser API available in the test environment.
2 participants
No description provided.