feat: add braavos mobile #1004
feat: add braavos mobile #1004
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
Warning Rate limit exceeded@Marchand-Nicolas has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 19 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 (2)
WalkthroughThis pull request introduces a new Braavos Mobile wallet connector and updates several configuration files related to wallet integration. The changes involve removing some existing wallet connector imports, updating package dependencies, and adding a new Changes
Possibly related issues
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![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 (3)
utils/braavosMobile.ts (2)
33-37: Consider returning meaningful connection data or error.
Currently,connect()opens a Braavos link in a new tab, but always returnsnullcast toPromise<ConnectorData>. This might confuse consumers expecting an actual payload or an error if the connection fails. If feasible, consider returning some signifier (e.g., a success response or a proper error) after the user completes or cancels a connection attempt.
51-55: ValidateNEXT_PUBLIC_IS_TESTNETenvironment variable.
ThechainId()function correctly switches between Sepolia and mainnet based onNEXT_PUBLIC_IS_TESTNET. However, there is no fallback or error handling if the environment variable is missing or invalid. This could lead to unexpected results in production or other environments.utils/walletConfig.ts (1)
157-157: Consider removing or gating console logs in production.
Logging the connector to the console might be useful in dev but clutters production logs. If that’s not needed in production, consider using a debug flag or removing it altogether.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
app/provider.tsx(1 hunks)package.json(2 hunks)utils/braavosMobile.ts(1 hunks)utils/walletConfig.ts(4 hunks)
🔇 Additional comments (6)
app/provider.tsx (1)
11-11: Removal of unused createTheme import looks clean.
The createTheme import no longer appears to be referenced. This removal helps keep the import list concise and free of unused dependencies.
utils/braavosMobile.ts (2)
39-49: Intentional “not connected” exception design.
These methods (wallet, disconnect, account) throw exceptions since there’s no persistent connection mechanism. Ensure higher-level code handles these exceptions gracefully and informs the user that Braavos Mobile can’t be queried for account details once launched.
57-59: Confirm readiness logic.
ready() always returns true. If the user has not installed Braavos on their mobile device or if prerequisites are unmet, the function still indicates readiness. Ensure your upstream code guards against usage if Braavos Mobile is not actually ready (e.g., user not installed or not opened).
utils/walletConfig.ts (2)
93-104: Accurate “downloads” and “website” references for Braavos Mobile.
The downloads and website fields dynamically build a link using window.location.host. This is a straightforward approach, but confirm that the application is served on a domain accessible to Braavos Mobile. Additionally, ensure that the domain is correct if you have multiple environments (dev, stage, prod).
141-142: Integration of the Braavos Mobile connector.
Adding getBraavosMobile() to the connectors array properly integrates Braavos Mobile into your wallet selection. Ensure that you handle the user’s journey after they open or install Braavos from the external link triggered by the connector’s connect() method.
package.json (1)
33-34: Dependencies updated to support new wallet functionality.
The addition of "get-starknet" and the upgrade of both "get-starknet-core" and "starknetkit" suggest broader changes to wallet integrations. This aligns with using the BraavosMobileConnector. Just verify that these new versions are compatible with other parts of your codebase.
Also applies to: 55-55
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
utils/walletConfig.ts (1)
93-104: Optimize the Braavos Mobile wallet configurationConsider these improvements for better maintainability:
- Use string literals instead of template literals for static URLs
- Extract the repeated URL into a constant to follow DRY principles
+ const BRAAVOS_DAPP_URL = "https://link.braavos.app/dapp/starknet.quest"; { id: "braavosMobile", name: "Braavos Mobile", icon: braavosMobileIcon, downloads: { - chrome: `https://link.braavos.app/dapp/starknet.quest`, - firefox: `https://link.braavos.app/dapp/starknet.quest`, - edge: `https://link.braavos.app/dapp/starknet.quest`, - safari: `https://link.braavos.app/dapp/starknet.quest`, + chrome: BRAAVOS_DAPP_URL, + firefox: BRAAVOS_DAPP_URL, + edge: BRAAVOS_DAPP_URL, + safari: BRAAVOS_DAPP_URL, }, - website: `https://link.braavos.app/dapp/starknet.quest`, + website: BRAAVOS_DAPP_URL, },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
utils/walletConfig.ts(4 hunks)
🔇 Additional comments (2)
utils/walletConfig.ts (2)
141-142: LGTM!
The Braavos Mobile connector is correctly added to the connectors array.
8-8: Verify the Braavos Mobile integration
Please ensure that:
- The
getBraavosMobilefunction is properly exported frombraavosMobile.ts - There are no conflicts between the existing Braavos wallet and the new Braavos Mobile wallet implementations
✅ Verification successful
Braavos Mobile integration is properly implemented with clear separation
The verification shows that:
getBraavosMobileis properly exported frombraavosMobile.ts- There is a clear separation between Braavos and Braavos Mobile implementations:
- Braavos wallet uses the ID "braavos" (line 47 in walletConfig.ts)
- Braavos Mobile uses a distinct ID "braavosMobile" (line 94 in walletConfig.ts)
- They have separate connector configurations and installation links
- No naming conflicts or overlapping functionality found
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the Braavos Mobile implementation and potential conflicts
# Check if getBraavosMobile is properly exported
rg -p "export.*getBraavosMobile"
# Check for any potential naming conflicts between Braavos implementations
rg -p "braavos" --type ts
Length of output: 1946
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
jest.config.js (1)
24-25: Custom transform ignore patterns for specific node modulesIntroducing
transformIgnorePatternsensures that certain libraries are properly transformed. Confirm that these libraries (@starknet-reactand@starknetkit) specifically need to be transpiled and are otherwise causing issues. If you'd like, I can assist with further improvements or additional patterns in this list.app/analytics/page.tsx (3)
20-20: Consider initializing quests to an empty array for clarity.While using
{}as the initial state typecasted toQuestListworks, initializing to an empty array or a more explicit structure may improve readability and reduce confusion about the shape of the data.
42-48: Use a descriptive text color or variant instead of "transparent".Setting the color prop to
"transparent"might hide text if the CSS or the component does not override it later. Make sure the design intention is to rely on background or special styling.
77-77: Check if an extra parenthesis can be removed or clarified.This closing parenthesis on line 77 concludes the mapping logic. Verify that it's aligned with the correct segment to maintain code clarity.
utils/walletConfig.ts (1)
141-144: Ensure that mobile detection meets user expectations.Currently,
window.innerWidth < 768triggers the mobile connector. Check if a tablet user (width slightly above 768) might also need Braavos Mobile. If so, consider using a more nuanced detection approach.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
app/analytics/page.tsx(2 hunks)context/QuestsProvider.tsx(0 hunks)jest.config.js(2 hunks)tests/utils/domainService.test.js(2 hunks)tests/utils/walletConfig.test.js(3 hunks)utils/walletConfig.ts(4 hunks)
💤 Files with no reviewable changes (1)
- context/QuestsProvider.tsx
✅ Files skipped from review due to trivial changes (1)
- tests/utils/walletConfig.test.js
🧰 Additional context used
🪛 Biome (1.9.4)
tests/utils/domainService.test.js
[error] 16-16: A global variable should not be reassigned.
Assigning to a global variable can override essential functionality.
(lint/suspicious/noGlobalAssign)
[error] 35-35: A global variable should not be reassigned.
Assigning to a global variable can override essential functionality.
(lint/suspicious/noGlobalAssign)
🔇 Additional comments (5)
jest.config.js (1)
5-5: Switching to babel-jest for comprehensive Next.js support
This update from ts-jest to the Next.js babel preset is a good change if the project heavily relies on Next.js features and transformations. Verify that you have installed all necessary Babel dependencies for this configuration, and confirm compatibility with your existing Jest test suites.
app/analytics/page.tsx (2)
25-25: Validate response structure from getQuests().
Relying on || {} is reasonable for fallback, but verifying that res meets expected interface (e.g., a known object shape vs. any fallback) would make the code more robust. Otherwise, if getQuests() returns an unexpected structure, subsequent logic might fail silently.
53-75: Ensure proper handling of empty or unexpected quest data.
When quests is an empty object or has keys pointing to non-array values, the code gracefully returns null. Confirm that you don't need to display a fallback UI or messaging instead of silently returning null.
utils/walletConfig.ts (2)
93-104: Confirm Braavos Mobile integration details.
The new wallet entry looks consistent. However, confirm that the custom link (“https://link.braavos.app/dapp/...”) is correct and that any additional required fields are present.
159-159: Remove debugging console.log statement.
This console.log appears to be debugging code and may expose sensitive connector information. Consider removing it before merging to production.
Summary by CodeRabbit
New Features
Bug Fixes
Dependencies
Improvements