feat: Improve codebase Onboarding #3867
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
4 Skipped Deployments
|
WalkthroughThe pull request includes modifications to the Changes
Possibly related PRs
Suggested reviewers
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
🧹 Outside diff range and nitpick comments (8)
packages/server/src/queries/coingecko/index.ts (2)
9-11: LGTM: Conditional assignment of PRICES_API_URLThe conditional assignment of
PRICES_API_URLeffectively implements the fallback mechanism to the free Coingecko API when the Osmosis API key is not available. This change aligns well with the PR objective of improving the onboarding process.For improved clarity, consider adding a brief comment explaining the purpose of this conditional assignment:
// Use Osmosis API if COINGECKO_API_KEY is available, otherwise fallback to free Coingecko API export const PRICES_API_URL = process.env.COINGECKO_API_KEY ? "https://prices.osmosis.zone" : COINGECKO_API_URL;
12-14: LGTM: Conditional assignment of DETAILS_API_URLThe conditional assignment of
DETAILS_API_URLfollows the same pattern asPRICES_API_URL, providing a consistent fallback mechanism to the free Coingecko API when the Osmosis API key is not available. This change further supports the PR objective of improving the onboarding process.For consistency and improved clarity, consider adding a brief comment here as well:
// Use Osmosis API if COINGECKO_API_KEY is available, otherwise fallback to free Coingecko API export const DETAILS_API_URL = process.env.COINGECKO_API_KEY ? "https://coingecko.osmosis.zone" : COINGECKO_API_URL;packages/web/hooks/use-feature-flags.ts (3)
48-55: LGTM! Consider updating documentation.The changes to the default feature flags align with the PR objectives to reflect the current production UI. This update will improve the onboarding experience for new developers.
Consider updating the README or relevant documentation to reflect these new default feature flag values, ensuring new contributors are aware of the expected default behavior.
107-111: LGTM! Consider a minor refactor for readability.The new logic for the
oneClickTradingflag improves control over the feature's availability and aligns with the PR objectives. It correctly handles development mode without a client ID and ensures the feature is only enabled under specific conditions.Consider refactoring the condition for better readability:
oneClickTrading: isDevModeWithoutClientID ? defaultFlags.oneClickTrading : !isMobile && launchdarklyFlags.swapToolSimulateFee && launchdarklyFlags.oneClickTrading,This refactor maintains the same logic while making it slightly more concise and easier to read.
114-121: LGTM! Consider a minor optimization.The new logic for the
limitOrdersflag successfully implements geoblocking functionality and improves the development experience. It aligns well with the PR objectives.Consider this minor optimization to reduce nesting and improve readability:
limitOrders: isDevModeWithoutClientID ? defaultFlags.limitOrders : isInitialized && launchdarklyFlags.limitOrders && (LIMIT_ORDER_COUNTRY_CODES.length === 0 || LIMIT_ORDER_COUNTRY_CODES.includes(levanaGeoblock?.countryCode ?? ""))This refactor maintains the same logic while slightly reducing nesting and improving readability.
README.md (3)
54-60: Improved setup instructions for contributors.The addition of the build step is crucial for ensuring all necessary artifacts are created before running the local server. This change significantly improves the onboarding process for new contributors.
Consider adding a brief explanation of why the build step is necessary. For example:
2. Run an initial build to create packages build artifacts (this step is crucial for setting up the monorepo structure): ```bash yarn build--- Line range hint `62-89`: **Comprehensive testnet development instructions added.** The new testnet section provides valuable information for both public and local testnet development. The inclusion of environment variable examples and the note about updating IBC assets are particularly helpful for new contributors. Consider adding a brief explanation of the difference between the public testnet and a local testnet, and when a developer might choose one over the other. This could help new contributors make informed decisions about their development environment. --- Line range hint `91-93`: **Important release status update added.** The new note in the "Releases" section provides critical information about the suspension of releases during the refactor. This warning is essential for preventing potential issues for users of the packages. Consider adding an estimated timeline for the completion of the refactor, if possible. This could help users plan their development accordingly. For example: ```markdown > Note: releases are suspended until the refactor is complete (estimated completion: Q4 2023). Please avoid importing packages from this repo until further notice. We will announce when releases resume on our official communication channels.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- README.md (1 hunks)
- packages/server/src/queries/coingecko/index.ts (1 hunks)
- packages/web/hooks/use-feature-flags.ts (2 hunks)
🔇 Additional comments (5)
packages/server/src/queries/coingecko/index.ts (2)
7-8: LGTM: Introduction of COINGECKO_API_URL constantThe addition of the
COINGECKO_API_URLconstant is a good approach. It centralizes the Coingecko API base URL, making it easier to maintain and update if needed in the future. This change aligns well with the PR objective of providing a fallback to the free Coingecko API when the Osmosis API key is not available.
7-14: Summary: Effective implementation of API fallback mechanismThe changes in this file successfully implement a fallback mechanism to the free Coingecko API when the Osmosis API key is not available. This aligns perfectly with the PR objective of improving the onboarding process for new contributors.
Key points:
- Introduction of
COINGECKO_API_URLconstant for centralized management of the Coingecko API base URL.- Conditional assignments for both
PRICES_API_URLandDETAILS_API_URLto use either the Osmosis API or fall back to the Coingecko API.These changes should effectively resolve the issues encountered during the initial build when environment variables are not configured, making it easier for new contributors to get started with the project.
To further enhance the code:
- Consider adding brief comments explaining the purpose of the conditional assignments for both
PRICES_API_URLandDETAILS_API_URL.Overall, these changes are well-implemented and should significantly improve the onboarding experience.
packages/web/hooks/use-feature-flags.ts (1)
Line range hint
1-134: Overall, the changes look good and align with the PR objectives.The modifications to the
useFeatureFlagshook successfully improve the onboarding process for new developers and add more granular control over feature availability. The changes to default flag values and the introduction of development mode checks enhance the initial setup experience.Key improvements:
- Updated default feature flags to reflect the current production UI.
- Implemented checks for development mode without a client ID.
- Added geoblocking functionality for limit orders.
These changes will facilitate easier onboarding and contribute to a more streamlined development process.
README.md (2)
Line range hint
5-7: Excellent addition of context for the ongoing refactor.The new note effectively communicates the current state of the codebase and sets appropriate expectations for contributors. This aligns well with the PR objective of improving the onboarding process.
Line range hint
1-100: Overall excellent improvements to documentation and onboarding process.The changes made to the README.md file significantly enhance the onboarding process for new contributors and provide clearer instructions for various development scenarios. The additions and updates align well with the PR objectives, addressing the following key points:
- Improved clarity on the current state of the codebase and ongoing refactor.
- Enhanced setup instructions, including the crucial build step.
- Comprehensive guidance for testnet development, both public and local.
- Important information about the suspension of releases during the refactor.
These changes will greatly benefit new contributors and existing developers working on the Osmosis Frontend project.
What is the purpose of the change:
This PR improves the onboarding process by addressing issues related to the initial build when environment variables have not been set up. Specifically, we replaced the hardcoded Osmosis API URL, which requires an API key, with a fallback to the free Coingecko API. While the Coingecko API has rate limits, it is sufficient for initial onboarding and short-term contributions to the code base.
Additionally, this PR updates the default feature flags to display a more up-to-date UI that aligns closely with the current production version. The README has also been updated to streamline the first installation and dev server setup. With these improvements, setting up the code base is now as simple as following the included instructions.
Linear Task
https://linear.app/osmosis/issue/FE-1146/improve-osmosis-fe-onboarding
Brief Changelog