fix oisy and show wallet popup info

[evanmcfarland]

No description provided.

[github-actions]

Pull Request Review

Overview

This PR refactors the authentication UI (login/index.tsx) and fixes the OISY wallet integration by removing the derivationOrigin parameter from the Signer constructor.

Positive Changes

1. Improved UX: Added helpful user guidance with a tip about ensuring wallet extensions are installed, unlocked, and allowed to open pop-ups (lines 192-194 in login/index.tsx)
2. Better Organization: Reorganized wallet options from a complex nested layout to a cleaner, flatter structure
3. Clearer Categorization: Changed "Cross-chain" to "External Wallets" which better describes OISY, Ethereum, Solana, and Arweave
4. Code Simplification: Removed unnecessary e.stopPropagation() calls since the parent clickable card was removed
5. Bug Fix: Removed derivationOrigin from OISY Signer initialization (useOisy.ts:306), which was likely causing authentication issues

Code Quality Observations

Good:

• Consistent formatting and structure
• Removed redundant event handling code
• Better semantic HTML organization

Minor Concerns:

1. Hardcoded URLs: Multiple hardcoded derivationOrigin URLs (https://yj5ba-aiaaa-aaaap-qkmoa-cai.icp0.io) appear throughout (lines 112, 132). Consider extracting to a constant.

2. Duplicate Login Logic: Internet Identity 2.0 and Legacy buttons have nearly identical code with only the identityProvider URL differing. Consider extracting to a helper function.

Potential Issues

1. Environment Variable Safety: No null checks for process.env.CANISTER_ID_INTERNET_IDENTITY when DFX_NETWORK is "local" (lines 113, 133). This could cause runtime errors if the env var is undefined.

2. OISY Categorization: OISY is now under "External Wallets" but it's actually an Internet Computer native wallet (similar to Plug/NFID). Consider whether it belongs in "Native Wallets" section instead.

3. Typo in Alert Message: Line 88 has "msessage" instead of "message":

   • status === 'preparing' - Please wait while we are preparing a secure msessage

Security Considerations

1. derivationOrigin Removal: The removal of derivationOrigin from the OISY Signer (useOisy.ts:306) is significant. While this fixes the issue, ensure this aligns with OISY's security model. The derivationOrigin is typically used for security isolation between applications.

2. Environment Variables in Production: Ensure process.env.DFX_NETWORK and process.env.CANISTER_ID_INTERNET_IDENTITY are properly set in all deployment environments.

Performance

No performance concerns. The changes reduce DOM complexity which could marginally improve render performance.

Test Coverage

Critical Gap: No tests found for the login component or useOisy hook. Given that this PR modifies authentication flows, it would be valuable to add:

1. Unit tests for the login component (wallet button clicks, UI rendering)
2. Integration tests for the OISY authentication flow
3. Tests verifying the environment variable handling

Recommendations

1. High Priority: Fix the typo "msessage" to "message" (line 88)
2. High Priority: Add null checks for environment variables
3. Medium Priority: Extract hardcoded derivation origin to a constant
4. Medium Priority: Refactor duplicate II login logic
5. Medium Priority: Add tests for authentication flows
6. Low Priority: Consider OISY categorization (Native vs External)

Summary

This is a solid refactoring that improves UX and fixes the OISY integration issue. The code quality is good overall, but would benefit from addressing the environment variable safety concerns and the typo before merging. The lack of test coverage for authentication flows is a concern for long-term maintainability.

Recommendation: Request minor changes before merging.

---

Review generated by Claude Code