Aum / GRWT-5204 / stake proposal stream #72
Conversation
Reviewer's Guide by SourceryThis pull request implements a stream of stake proposals based on the selected trade parameters and updates the UI with the received data. It introduces a new hook, Sequence diagram for proposal stream subscriptionsequenceDiagram
participant User
participant TradeFormController
participant useProposalStream
participant subscribeToProposalStream
participant Server
User->>TradeFormController: Selects trade parameters
TradeFormController->>useProposalStream: Passes proposalParams, isLoggedIn, account_uuid
useProposalStream->>subscribeToProposalStream: Subscribes to proposal stream with params
activate Server
Server-->>subscribeToProposalStream: Sends proposal data
deactivate Server
subscribeToProposalStream-->>useProposalStream: Returns proposal data
useProposalStream-->>TradeFormController: Updates proposalData state
TradeFormController->>TradeFormController: Updates button states with payout information
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 |
Deploying champion-trader with
|
| Latest commit: |
8a55f22
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://8fa3ec07.champion-trader.pages.dev |
| Branch Preview URL: | https://aum-grwt-5204-stake-proposal.champion-trader.pages.dev |
![devloai[bot]](https://avatars.githubusercontent.com/in/886113?s=60&v=4){kind=small}
There was a problem hiding this comment.
I've completed a review of the stake proposal stream implementation. The code introduces a well-structured SSE connection for real-time proposal data, following the project's patterns. I've identified several areas for improvement including type inconsistencies, potential bugs in state management, and opportunities to enhance error handling and reconnection logic. Overall, the implementation is solid but would benefit from addressing these issues before merging. Feel free to provide any feedback or instructions for future reviews or tasks related to this repository.
src/hooks/useProposalStream.ts
Outdated
| duration_unit: string; | ||
| /** Whether equals option is allowed */ | ||
| allow_equals: boolean; | ||
| /** Stake amount */ |
There was a problem hiding this comment.
The type for allow_equals in the interface is a boolean, but it's being converted to a string in the params on line 70. This could cause type confusion. Consider either making the interface parameter a string or handling the conversion more explicitly in the hook.
src/hooks/useProposalStream.ts
Outdated
| setLoading(true); | ||
| setError(null); | ||
|
|
||
| const cleanup = createSSEConnection<ProposalStreamResponse>({ |
There was a problem hiding this comment.
When a connection is established, loading state isn't explicitly reset if there are no messages received. Consider adding a timeout to set loading to false after a certain period if no message is received, to prevent the UI from showing a loading state indefinitely.
| } | ||
|
|
||
| const data = JSON.parse(jsonString) as ContractPriceResponse; | ||
| const data = JSON.parse(jsonString) as T; |
There was a problem hiding this comment.
The generic type T is correctly applied to the function signature, but there's no runtime type validation here. Consider adding a validation step or schema checking to ensure the parsed data matches the expected type structure, especially for critical data like contract proposals.
| // Reset loading states when duration or trade type changes | ||
| // Update button states based on proposal stream data | ||
| useEffect(() => { | ||
| if (data) { |
There was a problem hiding this comment.
This effect doesn't properly handle the case where the variants array might not include all possible button types. Consider adding a fallback that sets error states for buttons that don't have corresponding variants in the response data.
| jest.clearAllMocks(); | ||
| (createSSEConnection as jest.Mock).mockReturnValue(mockCleanup); | ||
| }); | ||
|
|
There was a problem hiding this comment.
The test verifies that the hook is called with correct parameters, but it doesn't test the automatic reconnection behavior when the network fails. Consider adding a test that simulates a network failure and checks if the connection is reestablished correctly.
| if (loading) { | ||
| // Set loading state for all buttons without referencing previous state directly | ||
| setButtonStates((prev) => { | ||
| const newState = { ...prev }; |
There was a problem hiding this comment.
You're creating a new state object with newState = { ...prev } but then modifying it directly in the forEach loop. This mutative approach could lead to subtle bugs. Consider using a more immutable pattern:
setButtonStates((prev) => {
const newState = {};
tradeTypeConfigs[trade_type].buttons.forEach((button) => {
newState[button.actionName] = {
...prev[button.actionName],
loading: false,
error: {
error: error instanceof Error ? error.message : String(error),
} as WebSocketError,
};
});
return { ...prev, ...newState };
});
src/services/api/sse/types.ts
Outdated
| account: null; | ||
| transaction: null; | ||
| } | ||
|
|
There was a problem hiding this comment.
The pricing_tick_id field appears as required in the response type, but it's marked as optional in the request type on line 9. This mismatch could cause confusion. Consider either making it consistent in both places or documenting when/why it might be optional in the request but always present in the response.
src/hooks/useProposalStream.ts
Outdated
|
|
||
| useEffect(() => { | ||
| // Skip connection if essential parameters are missing | ||
| if (!params.product_id || !params.instrument_id || !params.stake) { |
There was a problem hiding this comment.
The hook doesn't handle reconnection attempts for network issues. The createSSEConnection function has reconnection logic built in, but the state in this hook doesn't reflect reconnection attempts. Consider adding a reconnecting state that users of this hook can display to users.
aum-deriv
force-pushed
the
aum/GRWT-5204/stake-proposal-stream
branch
from
9a1f1a2 to
acb5839
Compare
aum-deriv
force-pushed
the
aum/GRWT-5204/stake-proposal-stream
branch
from
acb5839 to
da9c314
Compare
|
Unable to perform a code review. You have run out of credits 😔 |
|
@sourcery-ai summary |
aum-deriv
changed the title
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @aum-deriv - I've reviewed your changes - here's some feedback:
Overall Comments:
- Consider adding a cancellation mechanism to the
useProposalStreamhook to prevent memory leaks when the component unmounts. - The
parseDurationfunction could be improved by using a more robust regular expression or a dedicated library for parsing duration strings.
Here's what I looked at during the review
- 🟢 General issues: all looks good
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟡 Complexity: 1 issue found
- 🟢 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.
| payout: prevStates[button.actionName]?.payout || 0, | ||
| reconnecting: false, | ||
| }; | ||
| }); |
There was a problem hiding this comment.
issue (complexity): Consider extracting the button state update logic into a helper function to avoid duplicated loops over config.buttons in multiple useEffect blocks, improving code maintainability and readability by centralizing state management..
Consider extracting and centralizing the button state update logic into a helper or custom hook so that you can remove the duplicated loops over config.buttons in multiple useEffect blocks. For example, you could define a helper that returns the initial or updated states and then call it from your effects.
Create a helper:
const getButtonStates = (
buttons: any[],
prevStates: ButtonStates = {},
overrides: Partial<ButtonState> = {}
): ButtonStates =>
buttons.reduce((acc, button) => {
acc[button.actionName] = {
loading: overrides.loading ?? false,
error: null,
payout: prevStates[button.actionName]?.payout || 0,
reconnecting: false,
...overrides,
};
return acc;
}, {} as ButtonStates);Usage in your effects:
// In the trade_type update effect:
useEffect(() => {
setButtonStates(getButtonStates(config.buttons, buttonStates));
}, [trade_type, config.buttons]);
// In the proposal update effect:
useEffect(() => {
if (!isLoggedIn || !account_uuid) return;
if (!proposalData && !proposalError) {
setButtonStates(getButtonStates(config.buttons, buttonStates, { loading: true }));
return;
}
if (proposalData) {
const updatedStates: ButtonStates = {};
config.buttons.forEach((button: any) => {
const variantType = button.actionName === "buy_rise" ? "rise" : "fall";
const variant = proposalData.data.variants.find(
(v: any) => v.variant === variantType
);
updatedStates[button.actionName] = {
loading: false,
error: null,
payout: variant ? Number(variant.contract_details.payout) : 0,
reconnecting: false,
};
});
setButtonStates(updatedStates);
}
if (proposalError) {
setButtonStates((prevStates) =>
getButtonStates(config.buttons, prevStates, {
loading: false,
reconnecting: true,
error: proposalError instanceof Error ? { error: proposalError.message } : proposalError,
})
);
}
}, [proposalData, proposalError, trade_type, isLoggedIn, account_uuid, config.buttons]);This refactoring reduces duplication and helps isolate the button state logic, making the code more maintainable while keeping all features intact.
|
@sourcery summary |
3 participants
Summary by Sourcery
Subscribes to a stream of stake proposals based on the selected trade parameters and updates the UI with the received data. This includes fetching proposal data, handling loading and error states, and updating the payout values for the trade buttons.
New Features:
Screen.Recording.2025-03-14.at.12.03.43.PM.mov
Summary by Sourcery
Subscribes to a stream of stake proposals based on the selected trade parameters and updates the UI with the received data, including payout values for the trade buttons. It also handles loading and error states, and includes contract details in the proposal response.
New Features: