Fix For loading buttons #46
Conversation
Reviewer's Guide by SourceryThis pull request addresses an issue related to the loading state of buttons on the TradePage. The initial loading state was incorrectly set to true, which has been corrected to false. Additionally, error and reconnection checks have been temporarily disabled, and the button payout value has been adjusted for demo purposes. These changes are intended to facilitate testing and demonstration while the API is not fully connected. No diagrams generated as the changes look simple and do not need a visual representation. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.OpenSSF Scorecard
Scanned Manifest Files |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @ahmadtaimoor-deriv - I've reviewed your changes - here's some feedback:
Overall Comments:
- Please remove the console.log statement before merging.
- Avoid commenting out code, instead remove the code and create a new PR to re-add it if necessary.
Here's what I looked at during the review
- 🟡 General issues: 2 issues found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| @@ -182,6 +182,7 @@ export const TradeFormController: React.FC<TradeFormControllerProps> = ({ | |||
| } | |||
| }, [trade_type, config]) | |||
|
|
|||
| console.log("", buttonStates) | |||
There was a problem hiding this comment.
suggestion: Remove or disable console.log before production deployment.
Console logs can clutter the output and may expose internal state details. Consider removing or gating this log under a debugging flag before merging to production.
| console.log("", buttonStates) | |
| if (process.env.NODE_ENV !== "production") { | |
| console.log("", buttonStates) | |
| } |
| // added for demo proposes will change it to 0 once api is connected | ||
| buttonStates[button.actionName]?.payout || 10 |
There was a problem hiding this comment.
suggestion (bug_risk): Use of hardcoded demo fallback for payout value.
While the demo value is acceptable temporarily, consider isolating demo-specific logic (perhaps via a feature flag or config flag) to ensure it doesn't inadvertently make it to production.
Suggested implementation:
import React from 'react';
// other existing imports
// new code to check the demo mode flag (assuming using env variable for simplicity)
const isDemoMode = process.env.REACT_APP_DEMO_MODE === "true";
const fallbackPayout = isDemoMode ? 10 : 0; buttonStates[button.actionName]?.payout || fallbackPayoutIf your project has a centralized configuration or feature flag system, consider importing the demo mode flag from that module instead of reading from process.env directly. Replace the custom logic accordingly if a config flag is already defined in your codebase.
Deploying champion-trader with
|
| Latest commit: |
c079765
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://ef08e191.champion-trader.pages.dev |
| Branch Preview URL: | https://f-ui-fixes.champion-trader.pages.dev |
Summary by Sourcery
Fixes an issue where trade buttons were stuck in a loading state on initial render. Also, temporarily modifies the payout value to 10 for demo purposes, and disables the error and reconnecting states for the buttons, until the API is connected.
Bug Fixes:
Enhancements: