This repository was archived by the owner on Jul 15, 2025. It is now read-only.
feat: handle instrument change for chart#104
Merged
farabi-deriv merged 1 commit intoderiv-com:masterfrom Mar 21, 2025
Merged
Conversation
|
Unable to perform a code review. You have run out of credits 😔 |
Reviewer's Guide by SourceryThis pull request introduces changes to handle instrument changes in the chart components. It includes clearing existing streams, forcing chart re-renders, displaying a loading indicator, and modifying API request handlers to use the effective symbol or instrument when requesting ticks or ticks history. A Sequence diagram for instrument change in TradeChartsequenceDiagram
participant TradeChart
participant tradeStore
participant SmartChart
participant ChartAPI
TradeChart->>tradeStore: instrument changes
activate TradeChart
TradeChart->>TradeChart: setIsChartLoading(true)
TradeChart->>TradeChart: requestForgetStream()
activate TradeChart
TradeChart->>ChartAPI: clearChartCache()
ChartAPI-->>TradeChart:
deactivate TradeChart
TradeChart->>TradeChart: setChartKey(instrument)
TradeChart->>TradeChart: setTimeout(1.5s)
TradeChart->>SmartChart: key=chartKey, symbol=instrument
activate SmartChart
SmartChart->>ChartAPI: requestAPI(ticks_history: instrument)
activate ChartAPI
ChartAPI-->>SmartChart: chart data
deactivate ChartAPI
SmartChart-->>TradeChart: chart rendered
deactivate SmartChart
TradeChart->>TradeChart: setIsChartLoading(false)
deactivate TradeChart
Sequence diagram for instrument change in ContractDetailsChartsequenceDiagram
participant ContractDetailsChart
participant SmartChart
participant ChartAPI
ContractDetailsChart->>ContractDetailsChart: effectiveSymbol changes
activate ContractDetailsChart
ContractDetailsChart->>ContractDetailsChart: setIsChartLoading(true)
ContractDetailsChart->>ContractDetailsChart: clearChartCache()
activate ContractDetailsChart
ContractDetailsChart->>ChartAPI: clearChartCache()
ChartAPI-->>ContractDetailsChart:
deactivate ContractDetailsChart
ContractDetailsChart->>ContractDetailsChart: setChartKey(effectiveSymbol)
ContractDetailsChart->>ContractDetailsChart: setTimeout(1.5s)
ContractDetailsChart->>SmartChart: key=chartKey, symbol=effectiveSymbol
activate SmartChart
SmartChart->>ChartAPI: requestAPI(ticks_history: effectiveSymbol)
activate ChartAPI
ChartAPI-->>SmartChart: chart data
deactivate ChartAPI
SmartChart-->>ContractDetailsChart: chart rendered
deactivate SmartChart
ContractDetailsChart->>ContractDetailsChart: setIsChartLoading(false)
deactivate ContractDetailsChart
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @vinu-deriv - I've reviewed your changes - here's some feedback:
Overall Comments:
- The loading state and forced re-render logic is duplicated in both chart components; consider refactoring into a shared hook.
- The timeout duration for the loading indicator should be configurable.
Here's what I looked at during the review
- 🟡 General issues: 2 issues found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟡 Complexity: 2 issues 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.
| @@ -300,10 +301,20 @@ export const ContractDetailsChart: React.FC<ContractDetailsChartProps> = ({ | |||
|
|
|||
| // API request handlers | |||
There was a problem hiding this comment.
suggestion: Reduce duplication in tick replacement logic in API handlers.
Both the requestAPI and requestSubscribe functions are modifying tick-related properties. If similar logic is required in multiple components, consider extracting a helper function to encapsulate the logic for updating tick or ticks_history with the effective symbol. This can improve maintainability and consistency.
| const [chartKey, setChartKey] = useState(effectiveSymbol); | ||
| const [isChartLoading, setIsChartLoading] = useState(false); | ||
|
|
||
| useEffect(() => { |
There was a problem hiding this comment.
suggestion (performance): Revisit the fixed 1.5s loading timeout in the chart reload effect.
While a 1500ms delay may work in many cases, this hard-coded timeout might not accommodate variable network or rendering delays. If feasible, consider a mechanism to detect when the chart data actually finishes loading so the loading overlay can be removed at the appropriate time.
| }; | ||
| // When effectiveSymbol changes, forget all existing streams and force chart reload | ||
| const [chartKey, setChartKey] = useState(effectiveSymbol); | ||
| const [isChartLoading, setIsChartLoading] = useState(false); |
There was a problem hiding this comment.
issue (complexity): Consider creating a custom hook to encapsulate chart reload logic, which simplifies the component and reduces state duplication..
Consider extracting the chart reload logic into its own custom hook to isolate state handling and side effects. This would remove duplicate state and side effect management from your main component and flatten the logic. For example:
// Create a custom hook in a separate file (e.g., useChartReload.js)
import { useState, useEffect } from "react";
import { clearChartCache } from "@/api/services/chart";
export const useChartReload = (effectiveSymbol) => {
const [chartKey, setChartKey] = useState(effectiveSymbol);
const [isChartLoading, setIsChartLoading] = useState(false);
useEffect(() => {
setIsChartLoading(true);
clearChartCache();
setChartKey(effectiveSymbol);
const loadingTimeout = setTimeout(() => setIsChartLoading(false), 1500);
return () => clearTimeout(loadingTimeout);
}, [effectiveSymbol]);
return { chartKey, isChartLoading };
};Then in your component, simply use the hook:
import { useChartReload } from "@/hooks/useChartReload";
const YourChartComponent = ({ effectiveSymbol, ...props }) => {
const { chartKey, isChartLoading } = useChartReload(effectiveSymbol);
return (
<div className="relative">
{isChartLoading && (
<div className="absolute inset-0 z-10 flex items-center justify-center bg-theme bg-opacity-80 rounded-lg">
<div className="text-center">
<div className="inline-block animate-spin rounded-full h-8 w-8 border-4 border-solid border-blue-500 border-r-transparent"></div>
<p className="mt-4 text-white">Loading chart data...</p>
</div>
</div>
)}
<SmartChart
key={chartKey}
symbol={effectiveSymbol}
// ...other props
/>
</div>
);
};This refactoring preserves functionality while reducing in-component complexity and duplicate state management.
| handleChartForgetStream, | ||
| } from "@/api/services/chart"; | ||
| import { ToolbarWidgets } from "./Components"; | ||
| import { useTradeStore } from "@/stores/tradeStore"; |
There was a problem hiding this comment.
issue (complexity): Consider extracting the instrument change and chart reload logic into a custom hook to reduce component complexity and improve code reusability
Consider extracting the duplicated instrument change and chart reload logic into a custom hook. This approach lets you consolidate the state and effect logic, making each component that uses it much simpler. For example:
// useChartReload.ts
import { useEffect, useState } from "react";
export function useChartReload(instrument: string, requestForgetStream: () => void, delay = 1500) {
const [chartKey, setChartKey] = useState(instrument);
const [isChartLoading, setIsChartLoading] = useState(false);
useEffect(() => {
setIsChartLoading(true);
requestForgetStream();
setChartKey(instrument);
const timer = setTimeout(() => setIsChartLoading(false), delay);
return () => clearTimeout(timer);
}, [instrument, requestForgetStream, delay]);
return { chartKey, isChartLoading };
}Then refactor your component to use the hook:
import { useChartReload } from "@/hooks/useChartReload";
export const TradeChart: React.FC = () => {
// ... other declarations
const instrument = useTradeStore((state) => state.instrument);
// API request handlers
const requestForgetStream = () => handleChartForgetStream();
// Use the custom hook for instrument change reload logic
const { chartKey, isChartLoading } = useChartReload(instrument, requestForgetStream);
// ... component JSX remains mostly unchanged
return (
<div className="flex h-full relative bg-theme border-r text-gray-100">
{isChartLoading && (
<div className="absolute inset-0 z-10 flex items-center justify-center bg-theme bg-opacity-80">
<div className="text-center">
<div className="inline-block animate-spin rounded-full h-8 w-8 border-4 border-solid border-blue-500 border-r-transparent"></div>
<p className="mt-4 text-white">Loading chart data...</p>
</div>
</div>
)}
<Suspense fallback={<div className="flex items-center justify-center h-full w-full">...loading</div>}>
<SmartChart
key={chartKey}
// ... other props
/>
</Suspense>
</div>
);
};This refactoring reduces complexity in your component while preserving all current functionality.
farabi-deriv
approved these changes
Mar 21, 2025
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by Sourcery
Updates the chart components to handle instrument changes by clearing existing streams, forcing a chart reload, and displaying a loading indicator. This ensures the chart accurately reflects the currently selected instrument.
Enhancements: