chore: update simple chart with real data #13
Merged
Conversation
This file contains 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
Reviewer's Guide by SourceryThis pull request introduces a new Chart component that displays market data using WebSocket, replacing the old WebSocket implementation in the App component. It also includes dependency updates and test cleanup. Sequence diagram for Chart component WebSocket interactionsequenceDiagram
participant C as Chart Component
participant WS as WebSocket Hook
participant S as Market Server
participant CH as Chart Library
C->>WS: Initialize with R_100 instrument
WS->>S: Connect
S-->>WS: Connection established
WS-->>C: isConnected = true
loop Real-time updates
S->>WS: Price update
WS->>C: onPrice callback
C->>CH: Update chart data
C->>C: Update current price display
end
Note over C,S: Connection can be lost
S-xWS: Connection lost
WS-->>C: isConnected = false
C->>C: Show disconnected message
Class diagram for Chart component structureclassDiagram
class Chart {
+className?: string
-chartContainerRef: RefObject
-chartRef: IChartApi
-seriesRef: BaselineSeries
-currentPrice: number
-currentTime: string
-priceHistory: ChartData[]
+render()
}
class ChartData {
+time: UTCTimestamp
+value: number
}
class useMarketWebSocket {
+isConnected: boolean
+onConnect(): void
+onError(error: Error): void
+onPrice(price: Price): void
}
Chart --> ChartData: uses
Chart --> useMarketWebSocket: uses
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Deploying champion-trader with
|
| Latest commit: |
0255f8a
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://ab32639f.champion-trader.pages.dev |
| Branch Preview URL: | https://shafin-championtrader-feat-c.champion-trader.pages.dev |
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.OpenSSF Scorecard
Scanned Manifest Filespackage-lock.jsonpackage.json
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @shafin-deriv - I've reviewed your changes - here's some feedback:
Overall Comments:
- Tests for the Chart component were removed without replacement. Please add new tests before merging to maintain code quality and prevent regressions.
- The Chart component has a hardcoded instrument ID 'R_100'. Consider making this configurable via props to improve component reusability.
Here's what I looked at during the review
- 🟡 General issues: 3 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.
|
|
||
| const { isConnected } = useMarketWebSocket("R_100", { | ||
| onConnect: () => console.log("Market WebSocket Connected in Chart"), | ||
| onError: (err) => console.log("Market WebSocket Error in Chart:", err), |
There was a problem hiding this comment.
suggestion (bug_risk): Consider implementing proper error handling for WebSocket errors
Instead of just logging to console, consider showing an error state to users and implementing reconnection logic.
| } | ||
| }; | ||
|
|
||
| window.addEventListener("resize", handleResize); |
There was a problem hiding this comment.
suggestion (performance): Consider debouncing the resize handler to improve performance
Frequent resize events could impact performance. Consider using a debounced version of the handler.
Suggested implementation:
// Import at the top of the file
import debounce from 'lodash/debounce';
// Handle window resize with debouncing
const handleResize = debounce(() => { return () => {
window.removeEventListener("resize", handleResize);
handleResize.cancel(); // Cancel any pending debounced callsYou'll need to:
- Install lodash if not already installed:
npm install lodashoryarn add lodash - Optionally adjust the debounce wait time by passing a second argument to debounce if needed, e.g.,
debounce(() => {...}, 250)
| time: Math.floor(timestamp.getTime() / 1000) as UTCTimestamp, | ||
| value: price.ask, | ||
| }; | ||
| setPriceHistory((prev) => [...prev, newPrice]); |
There was a problem hiding this comment.
suggestion (performance): Consider implementing a limit on price history to prevent unbounded growth
The price history array will grow indefinitely. Consider implementing a maximum length and removing older entries.
Suggested implementation:
const MAX_PRICE_HISTORY = 1000; // Keep last 1000 price points
const { isConnected } = useMarketWebSocket("R_100", { setPriceHistory((prev) => {
const updatedHistory = [...prev, newPrice];
return updatedHistory.length > MAX_PRICE_HISTORY
? updatedHistory.slice(-MAX_PRICE_HISTORY)
: updatedHistory;
});| }); | ||
|
|
||
| useEffect(() => { | ||
| if (!chartContainerRef.current) return; |
There was a problem hiding this comment.
suggestion (code-quality): Use block braces for ifs, whiles, etc. (use-braces)
Suggested change
| if (!chartContainerRef.current) return; | |
| if (!chartContainerRef.current) { |
Explanation
It is recommended to always use braces and create explicit statement blocks.Using the allowed syntax to just write a single statement can lead to very confusing
situations, especially where subsequently a developer might add another statement
while forgetting to add the braces (meaning that this wouldn't be included in the condition).
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
This pull request includes several changes to the codebase, primarily focusing on the integration of a new chart component that displays market data using WebSocket, and the removal of the old WebSocket implementation from the
App. Below are the most important changes:Integration of Chart Component:
src/components/Chart/Chart.tsx: Introduced theChartcomponent which displays market data using WebSocket integration. This component handles real-time price updates, connection status, and error handling.src/components/Chart/README.md: Added documentation for theChartcomponent, detailing its usage, props, implementation details, and future enhancements.Dependency Updates:
package.json: Added thelightweight-chartslibrary as a new dependency to support the charting functionality.Removal of Old WebSocket Implementation:
src/App.tsx: Removed the old WebSocket implementation (useMarketWebSocket) from theAppcomponent. [1] [2]src/App.test.tsx: Removed tests related to the old WebSocket implementation. [1] [2] [3] [4] [5]Structural Documentation Update:
src/STRUCTURE.md: Updated the project structure documentation to include the newChartcomponent under reusable UI components.Test Cleanup:
src/components/Chart/__tests__/Chart.test.tsx: Removed the outdated tests for theChartcomponent, which will be replaced with new tests reflecting the updated implementation.These changes collectively enhance the application's ability to display real-time market data and streamline the code by removing redundant WebSocket implementations.
Summary by Sourcery
Introduce a new Chart component to display real-time market data using a WebSocket connection. Remove the old WebSocket implementation from the App component and update tests accordingly.
New Features:
Tests: