Conversation
WalkthroughAdds WebSocket-based real-time broadcasting: new WebSocketManager and client provider, MarketDataService subscription API and types, container wiring and config for WS/interval, client-side WS provider + fallback, dependency and docker/dev updates, and docs/readme changes. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant WS as WebSocketManager
participant MDS as MarketDataService
participant PriceSim as PriceSimulator
Client->>WS: Open WS connection
WS-->>Client: Send clientId (welcome)
WS->>Client: Ping (heartbeat)
Client->>WS: Pong
Client->>WS: {"type":"subscribe","payload":"SYM"}
WS->>MDS: subscribeToTicker("SYM", cb)
MDS-->>WS: initial ticker payload
WS-->>Client: {"type":"data","payload":ticker}
PriceSim->>MDS: update price for SYM
MDS->>MDS: notifySubscribers(updatedTicker)
MDS->>WS: (via cb) updatedTicker
WS-->>Client: {"type":"data","payload":updatedTicker}
Client->>WS: {"type":"unsubscribe","payload":"SYM"}
WS->>MDS: unsubscribeFromTicker("SYM", cbId)
Client->>WS: Close
WS->>WS: cleanup client subscriptions
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
server/market-trading-service/src/core/services/MarketDataService.ts (1)
25-33: Critical: Subscribers are never notified of price updates.The
startSimulationmethod updates the repository but never callsnotifySubscribersto inform WebSocket clients of the changes. This means subscribers won't receive real-time updates.Apply this diff to notify subscribers:
async startSimulation(): Promise<void> { const tickers = await this.repository.findAll(); tickers.forEach((ticker) => { this.priceSimulator.start(ticker, (updatedTicker) => { this.repository.update(updatedTicker); + this.notifySubscribers(updatedTicker); }); }); }
🧹 Nitpick comments (4)
server/market-trading-service/src/core/types/index.ts (1)
88-98: Consider more specific typing for WSMessage payload.The
payloadfield usesany, which reduces type safety. Consider using a union of specific payload types or generic constraints.Example refactor:
export interface WSMessage<T = unknown> { type: "subscribe" | "unsubscribe" | "ping" | "pong" | "error" | "data"; payload?: T; }server/market-trading-service/src/container/Container.ts (1)
66-68: Consider registering WebSocketManager in the services map for better lifecycle management.The
createWebSocketManagermethod creates a new instance each time it's called but doesn't register it in the services map. This makes it difficult to manage the WebSocket server's lifecycle (e.g., shutdown). Consider registering it as a singleton service.Example refactor:
createWebSocketManager(port: number): WebSocketManager { if (!this.services.has('WebSocketManager')) { const wsManager = new WebSocketManager(port, this.getMarketDataService()); this.services.set('WebSocketManager', wsManager); } return this.services.get('WebSocketManager'); }server/market-trading-service/src/core/services/MarketDataService.ts (1)
45-59: Improve callback ID generation to prevent collisions.Using
Math.random().toString(36).substring(7)can produce collisions. Consider using a more robust approach like UUID or a counter.Example using a counter:
private callbackIdCounter = 0; subscribeToTicker( symbol: string, callback: (ticker: ITicker) => void ): string { symbol = symbol.toUpperCase(); if (!this.subscribers.has(symbol)) { this.subscribers.set(symbol, new Map()); } const callbackId = `callback_${++this.callbackIdCounter}`; this.subscribers.get(symbol)!.set(callbackId, callback); return callbackId; }server/market-trading-service/src/infrastructure/websocket/WebSocketManager.ts (1)
148-150: Improve client ID generation to prevent collisions.Using
Math.random()can produce collisions in high-traffic scenarios. Consider using a more robust approach like UUID.Example using timestamp + counter:
private clientIdCounter = 0; private generateClientId(): string { return `client_${Date.now()}_${++this.clientIdCounter}`; }Or install the
uuidpackage and use:import { v4 as uuidv4 } from 'uuid'; private generateClientId(): string { return uuidv4(); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
server/market-trading-service/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (8)
docker-compose.yml(1 hunks)server/market-trading-service/package.json(2 hunks)server/market-trading-service/src/container/Container.ts(2 hunks)server/market-trading-service/src/core/services/MarketDataService.ts(2 hunks)server/market-trading-service/src/core/types/index.ts(3 hunks)server/market-trading-service/src/index.ts(1 hunks)server/market-trading-service/src/infrastructure/config/Config.ts(1 hunks)server/market-trading-service/src/infrastructure/websocket/WebSocketManager.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
server/market-trading-service/src/infrastructure/websocket/WebSocketManager.ts (1)
server/market-trading-service/src/core/types/index.ts (4)
WSClient(93-98)IMarketDataService(42-50)WSMessage(88-91)Ticker(4-15)
server/market-trading-service/src/core/types/index.ts (1)
server/market-trading-service/src/core/entities/Ticker.ts (1)
Ticker(3-59)
server/market-trading-service/src/container/Container.ts (1)
server/market-trading-service/src/infrastructure/websocket/WebSocketManager.ts (1)
WebSocketManager(4-164)
🔇 Additional comments (5)
server/market-trading-service/src/core/types/index.ts (2)
1-2: LGTM!The WebSocket type import is correctly aliased to avoid conflicts with browser WebSocket types.
56-57: LGTM!The new configuration fields are appropriately added to support WebSocket functionality.
server/market-trading-service/src/container/Container.ts (1)
1-11: LGTM!The migration to aliased import paths improves consistency and maintainability.
server/market-trading-service/src/core/services/MarketDataService.ts (1)
10-11: LGTM!The subscribers map structure is appropriate for managing per-symbol callbacks.
server/market-trading-service/package.json (1)
23-23: ws version 8.18.3 has no known security vulnerabilities. The previously identified CVE-2024-37890 (high-severity DoS) affected versions <8.17.1 and was fixed in 8.17.1; 8.18.3 is a later release and is not vulnerable.
| stopSimulation(): void { | ||
| return this.priceSimulator.stopAll(); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Clean up subscribers when stopping simulation.
The stopSimulation method should clear all subscribers to prevent memory leaks and stale subscriptions.
Apply this diff:
stopSimulation(): void {
+ this.subscribers.clear();
return this.priceSimulator.stopAll();
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| stopSimulation(): void { | |
| return this.priceSimulator.stopAll(); | |
| } | |
| stopSimulation(): void { | |
| this.subscribers.clear(); | |
| return this.priceSimulator.stopAll(); | |
| } |
🧰 Tools
🪛 Biome (2.1.2)
[error] 79-79: The function should not return a value because its return type is void.
The function is here:
'void' signals the absence of value. The returned value is likely to be ignored by the caller.
(lint/correctness/noVoidTypeReturn)
🤖 Prompt for AI Agents
In server/market-trading-service/src/core/services/MarketDataService.ts around
lines 78 to 80, stopSimulation currently just calls
this.priceSimulator.stopAll(); update it to also remove/cleanup all subscribers
to avoid memory leaks: iterate and unsubscribe/clear the subscriber collection
owned by MarketDataService (or call the simulator's provided method to remove
all subscribers) before or immediately after stopping the simulator, and ensure
the subscriber store is reset (e.g., emptied or reinitialized) so no stale
references remain.
| // Start WebSocket server | ||
| container.createWebSocketManager(config.wsPort); | ||
| console.log(`WebSocket Server running on port ${config.wsPort}`); |
There was a problem hiding this comment.
Critical: WebSocket server lifecycle not managed properly.
The WebSocketManager instance created on line 84 is not stored, preventing proper cleanup during shutdown. The shutdown handler (lines 87-106) should close WebSocket connections gracefully. Additionally, there's no error handling if WebSocket server creation fails.
Apply this diff to properly manage the WebSocket server lifecycle:
// Start price simulation
const marketDataService = container.getMarketDataService();
try {
await marketDataService.startSimulation();
console.log("✅ Price simulation started");
} catch (error) {
console.error("❌ Failed to start price simulation:", error);
throw error;
}
// Start WebSocket server
- container.createWebSocketManager(config.wsPort);
- console.log(`WebSocket Server running on port ${config.wsPort}`);
+ const wsManager = container.createWebSocketManager(config.wsPort);
+ console.log(`✅ WebSocket Server running on port ${config.wsPort}`);
const shutdown = (signal: string) => {
console.log(`\n⚠️ ${signal} received, shutting down gracefully...`);
// Close HTTP server
httpServer.close(() => {
console.log("🛑 HTTP server closed");
});
+
+ // Close WebSocket server
+ if (wsManager) {
+ wsManager.shutdown();
+ console.log("🛑 WebSocket server closed");
+ }
// In production, would also:
// - Stop price simulationsNote: You'll need to implement a shutdown() method in WebSocketManager to properly close all connections.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In server/market-trading-service/src/index.ts around lines 83 to 85, the
WebSocketManager instance returned by container.createWebSocketManager is not
stored and there is no error handling for creation; update the call to capture
the instance in a variable (e.g., const wsManager = await
container.createWebSocketManager(config.wsPort)) inside a try/catch so creation
errors are logged and cause a controlled shutdown/exit, add a call to await
wsManager.shutdown() (or wsManager.close()) in the existing shutdown handler so
connections are closed gracefully, and ensure shutdown errors are caught and
logged; also implement a shutdown() method on WebSocketManager that stops
accepting new connections and closes all existing sockets before resolving.
| wsPort: parseInt(process.env.WS_PORT || "8080"), | ||
| port: parseInt(process.env.PORT || "3005"), | ||
| env: process.env.NODE_ENV || "development", | ||
| priceUpdateInterval: parseInt( | ||
| process.env.PRICE_UPDATE_INTERVAL || "2000" | ||
| ), |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add validation for parsed integer values.
The parseInt calls don't validate the result, which could lead to NaN values if environment variables contain invalid numeric strings. This could cause runtime errors when these values are used.
Apply this diff to add validation:
export class Config {
static get(): AppConfig {
+ const wsPort = parseInt(process.env.WS_PORT || "8080");
+ const port = parseInt(process.env.PORT || "3005");
+ const priceUpdateInterval = parseInt(process.env.PRICE_UPDATE_INTERVAL || "2000");
+
+ if (isNaN(wsPort) || isNaN(port) || isNaN(priceUpdateInterval)) {
+ throw new Error("Invalid configuration: port values must be valid numbers");
+ }
+
return {
- wsPort: parseInt(process.env.WS_PORT || "8080"),
- port: parseInt(process.env.PORT || "3005"),
+ wsPort,
+ port,
env: process.env.NODE_ENV || "development",
- priceUpdateInterval: parseInt(
- process.env.PRICE_UPDATE_INTERVAL || "2000"
- ),
+ priceUpdateInterval,
};
}
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In server/market-trading-service/src/infrastructure/config/Config.ts around
lines 9 to 14, the parseInt calls can yield NaN if env vars are invalid; update
the code to parse each env var into a number, check isNaN for each parsed value,
and if NaN replace it with the intended default (e.g., 8080 for WS_PORT, 3005
for PORT, 2000 for PRICE_UPDATE_INTERVAL) or throw a clear startup error; ensure
the final config property is a validated number (not NaN) before exporting.
| private initialize(): void { | ||
| this.wss.on("connection", (ws: WebSocket) => { | ||
| const clientId = this.generateClientId(); | ||
| const client: WSClient = { | ||
| id: clientId, | ||
| ws: ws as any, | ||
| subscriptions: new Set(), | ||
| isAlive: true, | ||
| }; | ||
|
|
||
| this.clients.set(clientId, client); | ||
| console.log(`Client connected: ${clientId}`); | ||
|
|
||
| ws.send( | ||
| JSON.stringify({ | ||
| type: "connected", | ||
| payload: { clientId }, | ||
| }) | ||
| ); | ||
|
|
||
| ws.on("message", (data: WebSocket.Data) => { | ||
| this.handleMessage(client, data.toString()); | ||
| }); | ||
|
|
||
| ws.on("close", () => { | ||
| this.handleDisconnect(clientId); | ||
| }); | ||
|
|
||
| ws.on("error", (error) => { | ||
| console.error(`WebSocket error for client ${clientId}:`, error); | ||
| }); | ||
|
|
||
| ws.on("pong", () => { | ||
| client.isAlive = true; | ||
| }); | ||
| }); | ||
|
|
||
| // Heartbeat | ||
| setInterval(() => { | ||
| this.clients.forEach((client) => { | ||
| if (!client.isAlive) { | ||
| client.ws.terminate(); | ||
| this.clients.delete(client.id); | ||
| return; | ||
| } | ||
| client.isAlive = false; | ||
| client.ws.ping(); | ||
| }); | ||
| }, 30000); | ||
| } |
There was a problem hiding this comment.
Critical: Heartbeat interval is never cleared.
The setInterval on line 53 is never stored or cleared, creating a resource leak that will continue running even if the WebSocket server shuts down.
Apply this diff to properly manage the heartbeat interval:
export class WebSocketManager {
private wss: WebSocketServer;
private clients: Map<string, WSClient> = new Map();
private marketDataService: IMarketDataService;
+ private heartbeatInterval: NodeJS.Timeout | null = null;
constructor(port: number, marketDataService: IMarketDataService) {
this.marketDataService = marketDataService;
this.wss = new WebSocketServer({ port });
this.initialize();
}
private initialize(): void {
this.wss.on("connection", (ws: WebSocket) => {
// ... connection handling ...
});
// Heartbeat
- setInterval(() => {
+ this.heartbeatInterval = setInterval(() => {
this.clients.forEach((client) => {
if (!client.isAlive) {
client.ws.terminate();
this.clients.delete(client.id);
return;
}
client.isAlive = false;
client.ws.ping();
});
}, 30000);
}
+
+ shutdown(): void {
+ if (this.heartbeatInterval) {
+ clearInterval(this.heartbeatInterval);
+ }
+ this.clients.forEach(client => client.ws.terminate());
+ this.clients.clear();
+ this.wss.close();
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| private initialize(): void { | |
| this.wss.on("connection", (ws: WebSocket) => { | |
| const clientId = this.generateClientId(); | |
| const client: WSClient = { | |
| id: clientId, | |
| ws: ws as any, | |
| subscriptions: new Set(), | |
| isAlive: true, | |
| }; | |
| this.clients.set(clientId, client); | |
| console.log(`Client connected: ${clientId}`); | |
| ws.send( | |
| JSON.stringify({ | |
| type: "connected", | |
| payload: { clientId }, | |
| }) | |
| ); | |
| ws.on("message", (data: WebSocket.Data) => { | |
| this.handleMessage(client, data.toString()); | |
| }); | |
| ws.on("close", () => { | |
| this.handleDisconnect(clientId); | |
| }); | |
| ws.on("error", (error) => { | |
| console.error(`WebSocket error for client ${clientId}:`, error); | |
| }); | |
| ws.on("pong", () => { | |
| client.isAlive = true; | |
| }); | |
| }); | |
| // Heartbeat | |
| setInterval(() => { | |
| this.clients.forEach((client) => { | |
| if (!client.isAlive) { | |
| client.ws.terminate(); | |
| this.clients.delete(client.id); | |
| return; | |
| } | |
| client.isAlive = false; | |
| client.ws.ping(); | |
| }); | |
| }, 30000); | |
| } | |
| private initialize(): void { | |
| this.wss.on("connection", (ws: WebSocket) => { | |
| const clientId = this.generateClientId(); | |
| const client: WSClient = { | |
| id: clientId, | |
| ws: ws as any, | |
| subscriptions: new Set(), | |
| isAlive: true, | |
| }; | |
| this.clients.set(clientId, client); | |
| console.log(`Client connected: ${clientId}`); | |
| ws.send( | |
| JSON.stringify({ | |
| type: "connected", | |
| payload: { clientId }, | |
| }) | |
| ); | |
| ws.on("message", (data: WebSocket.Data) => { | |
| this.handleMessage(client, data.toString()); | |
| }); | |
| ws.on("close", () => { | |
| this.handleDisconnect(clientId); | |
| }); | |
| ws.on("error", (error) => { | |
| console.error(`WebSocket error for client ${clientId}:`, error); | |
| }); | |
| ws.on("pong", () => { | |
| client.isAlive = true; | |
| }); | |
| }); | |
| // Heartbeat | |
| this.heartbeatInterval = setInterval(() => { | |
| this.clients.forEach((client) => { | |
| if (!client.isAlive) { | |
| client.ws.terminate(); | |
| this.clients.delete(client.id); | |
| return; | |
| } | |
| client.isAlive = false; | |
| client.ws.ping(); | |
| }); | |
| }, 30000); | |
| } | |
| shutdown(): void { | |
| if (this.heartbeatInterval) { | |
| clearInterval(this.heartbeatInterval); | |
| } | |
| this.clients.forEach(client => client.ws.terminate()); | |
| this.clients.clear(); | |
| this.wss.close(); | |
| } |
🤖 Prompt for AI Agents
In
server/market-trading-service/src/infrastructure/websocket/WebSocketManager.ts
around lines 15 to 64, the heartbeat setInterval is started but its timer id is
never stored or cleared, leaking resources when the server shuts down; fix by
adding a class property (e.g., private heartbeatTimer?: NodeJS.Timeout), assign
the returned interval id to it when calling setInterval, and clear it with
clearInterval(this.heartbeatTimer) in the WebSocketManager shutdown/close method
(and set the property to undefined after clearing); also ensure any existing
timer is cleared before creating a new one to avoid duplicates and use the
correct NodeJS.Timeout type for the property.
| this.wss.on("connection", (ws: WebSocket) => { | ||
| const clientId = this.generateClientId(); | ||
| const client: WSClient = { | ||
| id: clientId, | ||
| ws: ws as any, | ||
| subscriptions: new Set(), | ||
| isAlive: true, | ||
| }; |
There was a problem hiding this comment.
Remove unsafe type assertion.
Line 20 uses ws as any to bypass TypeScript's type checking. This is unsafe and unnecessary since the WebSocket type from the ws library should match the WsWebSocket type alias.
Apply this diff:
const client: WSClient = {
id: clientId,
- ws: ws as any,
+ ws: ws,
subscriptions: new Set(),
isAlive: true,
};If there are type compatibility issues, verify that the WsWebSocket type import in types/index.ts correctly references the ws library's WebSocket type.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| this.wss.on("connection", (ws: WebSocket) => { | |
| const clientId = this.generateClientId(); | |
| const client: WSClient = { | |
| id: clientId, | |
| ws: ws as any, | |
| subscriptions: new Set(), | |
| isAlive: true, | |
| }; | |
| this.wss.on("connection", (ws: WebSocket) => { | |
| const clientId = this.generateClientId(); | |
| const client: WSClient = { | |
| id: clientId, | |
| ws: ws, | |
| subscriptions: new Set(), | |
| isAlive: true, | |
| }; |
| private handleMessage(client: WSClient, message: string): void { | ||
| try { | ||
| const msg: WSMessage = JSON.parse(message); | ||
|
|
||
| switch (msg.type) { | ||
| case "subscribe": | ||
| this.handleSubscribe(client, msg.payload?.symbols || []); | ||
| break; | ||
| case "unsubscribe": | ||
| this.handleUnsubscribe(client, msg.payload?.symbols || []); | ||
| break; | ||
| case "ping": | ||
| client.ws.send(JSON.stringify({ type: "pong" })); | ||
| break; | ||
| } | ||
| } catch (error) { | ||
| client.ws.send( | ||
| JSON.stringify({ | ||
| type: "error", | ||
| payload: { message: "Invalid message format" }, | ||
| }) | ||
| ); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add error handling for WebSocket send operations.
The ws.send calls (lines 78, 82) can throw errors if the connection is closed. These should be wrapped in try-catch blocks.
Apply this diff:
private handleMessage(client: WSClient, message: string): void {
try {
const msg: WSMessage = JSON.parse(message);
switch (msg.type) {
case "subscribe":
this.handleSubscribe(client, msg.payload?.symbols || []);
break;
case "unsubscribe":
this.handleUnsubscribe(client, msg.payload?.symbols || []);
break;
case "ping":
- client.ws.send(JSON.stringify({ type: "pong" }));
+ try {
+ client.ws.send(JSON.stringify({ type: "pong" }));
+ } catch (error) {
+ console.error(`Failed to send pong to client ${client.id}:`, error);
+ }
break;
}
} catch (error) {
- client.ws.send(
- JSON.stringify({
- type: "error",
- payload: { message: "Invalid message format" },
- })
- );
+ try {
+ client.ws.send(
+ JSON.stringify({
+ type: "error",
+ payload: { message: "Invalid message format" },
+ })
+ );
+ } catch (sendError) {
+ console.error(`Failed to send error message to client ${client.id}:`, sendError);
+ }
}
}🤖 Prompt for AI Agents
In
server/market-trading-service/src/infrastructure/websocket/WebSocketManager.ts
around lines 66 to 89, the ws.send calls inside the "ping" case (and any other
send points) can throw when the socket is closed; wrap each client.ws.send
invocation in a try-catch, and in the catch log the error (including client
id/context) and terminate/close the socket or remove the client from your
registry to avoid leaking state; ensure the try-catch is applied consistently to
future sends and do not attempt to send an error message back over the same
failed socket.
server/market-trading-service/src/infrastructure/websocket/WebSocketManager.ts
Show resolved
Hide resolved
server/market-trading-service/src/infrastructure/websocket/WebSocketManager.ts
Show resolved
Hide resolved
… to REST API; update environment variables and documentation.
… to REST API; update environment variables and documentation.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
server/market-trading-service/src/core/services/MarketDataService.ts (1)
81-83: Critical: Fix return statement and clean up subscribers.Two issues here:
- Return statement with void: Line 82 returns the result of
stopAll(), but the method signature declaresvoidreturn type. This is a type error.- Memory leak: Subscribers are not cleared when stopping simulation, which can cause stale subscriptions and memory leaks (previously flagged).
Apply this diff to fix both issues:
stopSimulation(): void { + this.subscribers.clear(); - return this.priceSimulator.stopAll(); + this.priceSimulator.stopAll(); }
🧹 Nitpick comments (5)
server/market-trading-service/src/core/services/MarketDataService.ts (1)
48-62: Consider using a more robust ID generation strategy.Using
Math.random().toString(36)for callback IDs can lead to collisions in high-frequency scenarios and is not cryptographically secure. Consider using a counter or UUID library for guaranteed uniqueness.Example using a counter:
export class MarketDataService implements IMarketDataService { private subscribers: Map<string, Map<string, (ticker: ITicker) => void>> = new Map(); + private callbackIdCounter = 0; constructor( private repository: ITickerRepository, private priceSimulator: IPriceSimulator ) {} // ... rest of code ... subscribeToTicker( symbol: string, callback: (ticker: ITicker) => void ): string { symbol = symbol.toUpperCase(); if (!this.subscribers.has(symbol)) { this.subscribers.set(symbol, new Map()); } - const callbackId = Math.random().toString(36).substring(7); + const callbackId = `cb-${++this.callbackIdCounter}`; this.subscribers.get(symbol)!.set(callbackId, callback); return callbackId; }Or install and use the
uuidpackage for cryptographically secure IDs.server/market-trading-service/src/core/types/index.ts (1)
88-91: Replaceanywithunknownfor better type safety.The
payloadproperty typed asanybypasses TypeScript's type checking. Useunknownto maintain type safety while allowing flexibility, or define specific payload types for each message type.Apply this diff:
export interface WSMessage { type: "subscribe" | "unsubscribe" | "ping" | "pong" | "error" | "data"; - payload?: any; + payload?: unknown; }Alternatively, for stronger typing, define discriminated unions:
export type WSMessage = | { type: "subscribe"; payload: { symbol: string } } | { type: "unsubscribe"; payload: { symbol: string } } | { type: "ping" | "pong"; payload?: never } | { type: "error"; payload: { message: string } } | { type: "data"; payload: Ticker };client/trading-dashboard/components/TickerGrid.tsx (1)
9-20: Rename function to match its purpose.The function
getPostsfetches tickers, not posts. This naming inconsistency may cause confusion.Apply this diff to fix the naming:
-async function getPosts(): Promise<Ticker[]> { +async function getTickers(): Promise<Ticker[]> { const res = await fetch(`${API_BASE_URL}/tickers`); if (!res.ok) { - throw new Error("Failed to fetch posts"); + throw new Error("Failed to fetch tickers"); }And update the query configuration:
const { data, error } = useQuery<Ticker[], Error>({ queryKey: ["tickers"], - queryFn: getPosts, + queryFn: getTickers, refetchInterval: useFallback ? 1000 : false,client/trading-dashboard/providers/WebSocketProvider.tsx (2)
163-173: Potential stale closure risk with subscribe/unsubscribe functions.The
subscribeandunsubscribefunctions are not included in theuseMemodependency array (Line 172), which could lead to stale closures ifsendJsonMessagechanges between renders. Whilereact-use-websockettypically provides a stablesendJsonMessagereference, explicitly including these functions in the dependencies or wrapping them inuseCallbackwould make the behavior more predictable and safer.Consider wrapping the functions in
useCallback:+ const subscribe = useCallback((symbols: string[]) => { + const list = symbols.map((s) => s.toUpperCase()); + list.forEach((s) => subscribed.current.add(s)); + sendJsonMessage({ type: "subscribe", payload: { symbols: list } }); + }, [sendJsonMessage]); + + const unsubscribe = useCallback((symbols: string[]) => { + const list = symbols.map((s) => s.toUpperCase()); + list.forEach((s) => subscribed.current.delete(s)); + sendJsonMessage({ type: "unsubscribe", payload: { symbols: list } }); + }, [sendJsonMessage]); + - const subscribe = (symbols: string[]) => { - const list = symbols.map((s) => s.toUpperCase()); - list.forEach((s) => subscribed.current.add(s)); - sendJsonMessage({ type: "subscribe", payload: { symbols: list } }); - }; - - const unsubscribe = (symbols: string[]) => { - const list = symbols.map((s) => s.toUpperCase()); - list.forEach((s) => subscribed.current.delete(s)); - sendJsonMessage({ type: "unsubscribe", payload: { symbols: list } }); - }; const value = useMemo<WebSocketContextValue>( () => ({ connected, clientId, tickers, priceStatus, subscribe, unsubscribe, }), - [connected, clientId, tickers, priceStatus] + [connected, clientId, tickers, priceStatus, subscribe, unsubscribe] );
182-198: Consider input validation in normalizeTicker.The
normalizeTickerfunction performs type coercion but doesn't validate that the input contains the expected fields or that coerced values are valid (e.g., non-NaN numbers). While the try-catch in the message handler (Line 144) provides some protection, explicit validation here could prevent subtle bugs from malformed server data.Consider adding basic validation:
function normalizeTicker(t: any): Ticker { + if (!t || typeof t !== 'object') { + throw new Error('Invalid ticker: expected object'); + } + if (!t.symbol || !t.name) { + throw new Error('Invalid ticker: missing required fields'); + } + return { symbol: String(t.symbol), name: String(t.name), price: Number(t.price), previousClose: Number(t.previousClose ?? t.price), change: Number(t.change), changePercent: Number(t.changePercent), volume: Number(t.volume), high24h: Number(t.high24h), low24h: Number(t.low24h), lastUpdate: typeof t.lastUpdate === "string" ? t.lastUpdate : new Date(t.lastUpdate).toISOString(), } as Ticker; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
.github/instructions/snyk_rules.instructions.md(0 hunks)README.md(1 hunks)client/trading-dashboard/app/(dashboard)/dashboard/page.tsx(1 hunks)client/trading-dashboard/app/Providers.tsx(2 hunks)client/trading-dashboard/components/Footer.tsx(0 hunks)client/trading-dashboard/components/TickerGrid.tsx(3 hunks)client/trading-dashboard/constants/index.ts(1 hunks)client/trading-dashboard/example.env(1 hunks)client/trading-dashboard/hooks/useTradingData.ts(4 hunks)client/trading-dashboard/providers/WebSocketProvider.tsx(1 hunks)server/market-trading-service/src/core/services/MarketDataService.ts(3 hunks)server/market-trading-service/src/core/types/index.ts(3 hunks)server/market-trading-service/src/infrastructure/websocket/WebSocketManager.ts(1 hunks)
💤 Files with no reviewable changes (2)
- client/trading-dashboard/components/Footer.tsx
- .github/instructions/snyk_rules.instructions.md
✅ Files skipped from review due to trivial changes (2)
- client/trading-dashboard/app/(dashboard)/dashboard/page.tsx
- README.md
🚧 Files skipped from review as they are similar to previous changes (1)
- server/market-trading-service/src/infrastructure/websocket/WebSocketManager.ts
🧰 Additional context used
🧬 Code graph analysis (5)
client/trading-dashboard/app/Providers.tsx (1)
client/trading-dashboard/providers/WebSocketProvider.tsx (1)
WebSocketProvider(30-180)
client/trading-dashboard/providers/WebSocketProvider.tsx (2)
server/market-trading-service/src/core/types/index.ts (1)
Ticker(4-15)client/trading-dashboard/constants/index.ts (3)
WS_URL(7-7)API_BASE_URL(4-6)MOCK_TICKERS(9-118)
server/market-trading-service/src/core/types/index.ts (1)
server/market-trading-service/src/core/entities/Ticker.ts (1)
Ticker(3-59)
client/trading-dashboard/components/TickerGrid.tsx (3)
client/trading-dashboard/providers/WebSocketProvider.tsx (1)
useWebSocketContext(200-213)client/trading-dashboard/types/index.ts (1)
Ticker(1-12)client/trading-dashboard/constants/index.ts (1)
MOCK_TICKERS(9-118)
client/trading-dashboard/hooks/useTradingData.ts (3)
server/market-trading-service/src/core/types/index.ts (1)
Ticker(4-15)client/trading-dashboard/types/index.ts (1)
Ticker(1-12)client/trading-dashboard/providers/WebSocketProvider.tsx (1)
useWebSocketContext(200-213)
🪛 GitHub Actions: CI
client/trading-dashboard/providers/WebSocketProvider.tsx
[error] 22-22: Cannot find module 'react-use-websocket' imported in WebSocketProvider.tsx. Install the dependency or mock it for tests.
🪛 GitHub Actions: E2E (Cypress)
client/trading-dashboard/providers/WebSocketProvider.tsx
[error] 11-11: Build failed during 'npm run build'. Turbopack: Module not found: Can't resolve 'react-use-websocket' (imported at line 11). Ensure the dependency is installed and available to the build.
🔇 Additional comments (17)
server/market-trading-service/src/core/services/MarketDataService.ts (4)
10-11: LGTM! Well-structured subscriber management.The nested Map structure efficiently manages per-symbol callbacks with unique IDs, enabling proper cleanup and preventing interference between symbols.
30-33: LGTM! Proper integration of notification flow.The persistence followed by subscriber notification ensures consistent state propagation to WebSocket clients.
64-72: LGTM! Proper cleanup with empty map removal.The unsubscribe logic correctly removes the callback and cleans up empty symbol maps to prevent memory leaks.
74-79: LGTM! Clean notification implementation.The notification logic efficiently broadcasts updates to all registered callbacks for a symbol.
server/market-trading-service/src/core/types/index.ts (4)
1-2: LGTM! Correct WebSocket type import.The type-only import from the
wspackage is properly declared for TypeScript typing.
48-49: LGTM! Interface signature now matches implementation.The
subscribeToTickermethod now correctly returnsstring(the callbackId), aligning with the implementation and enabling proper cleanup viaunsubscribeFromTicker.
56-57: LGTM! Sensible configuration extensions.The
priceUpdateIntervalandwsPortadditions properly support WebSocket functionality and configurable update cadence.
93-98: LGTM! Well-defined WebSocket client contract.The
WSClientinterface properly encapsulates client state with ID, connection, subscriptions set, and liveness tracking.client/trading-dashboard/constants/index.ts (1)
7-7: LGTM! Clean environment variable configuration.The WebSocket URL constant is correctly configured with a sensible fallback value that aligns with the example.env file.
client/trading-dashboard/example.env (1)
3-3: LGTM! Environment variable properly documented.The WebSocket URL environment variable is correctly added to the example configuration file.
client/trading-dashboard/app/Providers.tsx (1)
6-6: LGTM! WebSocket context properly integrated.The WebSocketProvider is correctly imported and integrated into the provider tree, enabling WebSocket context throughout the application.
Also applies to: 23-23
client/trading-dashboard/components/TickerGrid.tsx (1)
22-34: Clean WebSocket fallback implementation.The fallback logic correctly prioritizes WebSocket data when available and seamlessly falls back to REST polling when needed. The conditional query enabling prevents unnecessary API calls when WebSocket is active.
client/trading-dashboard/providers/WebSocketProvider.tsx (2)
63-107: Bootstrap logic is well-structured with proper cleanup.The initial data fetch with REST fallback and WebSocket subscription setup is well-implemented. The cancellation token prevents state updates after unmount, and the mock data fallback ensures the UI remains functional even when the API is unavailable.
110-147: Message handling is robust and defensive.The WebSocket message parsing correctly handles both connection acknowledgment and data updates, with appropriate error handling for malformed messages and proper price direction tracking.
client/trading-dashboard/hooks/useTradingData.ts (3)
9-9: Excellent refactor to derive selectedTicker from symbol.Storing
selectedSymbolas a string and deriving the fullselectedTickerviauseMemoensures the selected ticker object always reflects the latest data from either WebSocket or REST source. This prevents stale ticker data when prices update.Also applies to: 76-80, 106-106
15-42: Clean WebSocket integration with REST fallback.The fallback detection logic properly prioritizes WebSocket data and only enables REST polling when WebSocket data is unavailable. This ensures efficient data fetching while maintaining reliability.
44-67: Proper price status handling for both data sources.The effect correctly uses WebSocket-provided price status when available and falls back to computing it from REST data. The dependency array is complete and accurate.
…update README and architecture documentation
…parajuli/multi-bank-coding-challenge into feature/ws-implementation
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
client/trading-dashboard/components/TickerGrid.tsx (1)
55-57: Consider removing redundant null check.The
tickersToShow &&check is unnecessary sincetickersToShowis always defined (it falls back toMOCK_TICKERSwhen other sources are unavailable). The length check alone is sufficient.Apply this diff to simplify:
- {tickersToShow && - tickersToShow.length > 0 && + {tickersToShow.length > 0 && tickersToShow.map((ticker) => (README.md (3)
36-38: Wrap bare URLs in markdown link syntax or code blocks.Lines 36–38 contain bare URLs flagged by markdown linting. Wrap them in backticks or link syntax for proper markdown formatting:
- Frontend (dev server): http://localhost:3000 - Backend API (dev): http://localhost:3005 - WebSocket (dev): ws://localhost:8080 + Frontend (dev server): `http://localhost:3000` + Backend API (dev): `http://localhost:3005` + WebSocket (dev): `ws://localhost:8080`
78-78: Capitalize "GitHub" correctly.Line 78 uses lowercase "github"; the official name is "GitHub".
- - CI/CD via github actions to run test on PR + - CI/CD via GitHub Actions to run tests on PR
102-102: Fix spelling and grammar in line 102."time constrain" is incorrect. Use "time constraints" or "tight timeline".
- - Some of the Bonus features aren't covered due to time constrain. + - Some bonus features aren't covered due to time constraints.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
client/trading-dashboard/package-lock.jsonis excluded by!**/package-lock.jsonserver/market-trading-service/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (7)
README.md(2 hunks)client/trading-dashboard/components/TickerGrid.tsx(3 hunks)client/trading-dashboard/constants/index.ts(1 hunks)client/trading-dashboard/package.json(1 hunks)docker-compose.dev.yml(1 hunks)docker-compose.yml(2 hunks)docs/architecture.md(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- docker-compose.yml
- client/trading-dashboard/constants/index.ts
🧰 Additional context used
🧬 Code graph analysis (1)
client/trading-dashboard/components/TickerGrid.tsx (3)
client/trading-dashboard/providers/WebSocketProvider.tsx (1)
useWebSocketContext(200-213)client/trading-dashboard/types/index.ts (1)
Ticker(1-12)client/trading-dashboard/constants/index.ts (1)
MOCK_TICKERS(17-126)
🪛 LanguageTool
README.md
[uncategorized] ~78-~78: The official name of this software platform is spelled with a capital “H”.
Context: ...nitial bootstrap/fallback. - CI/CD via github actions to run test on PR - Used Code ...
(GITHUB)
[grammar] ~102-~102: Ensure spelling is correct
Context: ...nus features aren't covered due to time constrain. License This project is available un...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🪛 markdownlint-cli2 (0.18.1)
README.md
36-36: Bare URL used
(MD034, no-bare-urls)
37-37: Bare URL used
(MD034, no-bare-urls)
docs/architecture.md
33-33: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🔇 Additional comments (7)
docs/architecture.md (2)
70-71: Documentation updates are accurate and beneficial.The minor text refinements (React Query polling clarification, state management stance, Recharts/D3.js integration details) improve clarity and align well with the actual implementation patterns described. These changes accurately reflect the architecture decisions.
Also applies to: 75-75
13-13: Backend port alignment confirmed across all configuration files.The verification shows that the documentation update is accurate and properly aligned with the actual backend configuration:
- Backend REST API port 3005 is consistently configured across docker-compose files, environment configurations (.env), and HTTP test files (api.http)
- WebSocket service port 8080 is configured separately and correctly referenced in client environment variables
- docs/architecture.md line 13 correctly displays
localhost:3005for the Node.js Backend- All related configuration files are in sync with no discrepancies
No action required.
client/trading-dashboard/components/TickerGrid.tsx (2)
26-28: LGTM! Efficient query configuration.The conditional
refetchIntervalandenabledsettings correctly prevent redundant API polling when WebSocket data is available, while ensuring continuous updates in fallback mode.
31-33: LGTM! Clear data source fallback chain.The data selection logic correctly prioritizes WebSocket tickers and provides a clean fallback to API data or mock data, ensuring the component always has data to render.
docker-compose.dev.yml (1)
1-34: Docker Compose development setup looks well-structured.The service configuration properly wires both server and client with correct port mappings (3005 for REST, 8080 for WebSocket, 3000 for frontend), environment variables, and dependency ordering. Volume setup with anonymous /app/node_modules mount prevents host-dependency conflicts and supports hot reload.
README.md (1)
64-102: Comprehensive assumptions and trade-offs documentation.The new "Assumptions & Trade-offs" section clearly articulates design decisions, constraints, and rationale behind the WebSocket implementation and development choices. This is valuable for onboarding and future maintenance decisions.
client/trading-dashboard/package.json (1)
26-26: ---react-use-websocket@4.13.0 is compatible with React 19.2.0 and actively maintained.
Package targets React 18+ with peerDependencies "react": ">=18.0.0", compatible with React 19.2.0, and v4.13.0 was published approximately 6–7 months ago with an active GitHub repository. The dependency is sound as written.
| const { tickers: wsTickers } = useWebSocketContext(); | ||
| const useFallback = !wsTickers || wsTickers.length === 0; |
There was a problem hiding this comment.
Fallback logic ignores WebSocket connection status.
The useFallback condition only checks whether wsTickers has data, not whether the WebSocket is still connected. If the WebSocket disconnects after receiving initial data, the component will continue showing stale tickers without falling back to API polling.
Apply this diff to incorporate connection status:
- const { tickers: wsTickers } = useWebSocketContext();
- const useFallback = !wsTickers || wsTickers.length === 0;
+ const { tickers: wsTickers, connected } = useWebSocketContext();
+ const useFallback = !connected || !wsTickers || wsTickers.length === 0;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const { tickers: wsTickers } = useWebSocketContext(); | |
| const useFallback = !wsTickers || wsTickers.length === 0; | |
| const { tickers: wsTickers, connected } = useWebSocketContext(); | |
| const useFallback = !connected || !wsTickers || wsTickers.length === 0; |
🤖 Prompt for AI Agents
In client/trading-dashboard/components/TickerGrid.tsx around lines 21-22, the
current useFallback only checks wsTickers presence and ignores the WebSocket
connection state; update the code to also read the connection flag (e.g.,
wsConnected or isConnected) from useWebSocketContext and set useFallback to true
when there are no tickers OR when the socket is not connected (for example:
useFallback = !wsTickers || wsTickers.length === 0 || !wsConnected), ensuring
you safely destructure the connection boolean from the context and handle
undefined values.
|
|
||
| ## Server Architecture | ||
|
|
||
| ``` |
There was a problem hiding this comment.
Specify language for fenced code block.
The fenced code block at line 33 is missing a language specification. Since it contains a file tree structure, consider marking it as ```text or ```bash for proper markdown rendering and linting compliance.
-```
+```text
├── server/market-trading-service/
├── src/This aligns with markdown best practices and resolves the static analysis warning (MD040, fenced-code-language).
🧰 Tools
🪛 markdownlint-cli2 (0.18.1)
33-33: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
In docs/architecture.md around line 33, the fenced code block is missing a
language specifier causing markdown lint MD040; update the opening fence to
include an appropriate language such as "text" or "bash" (e.g., change ``` to
```text) so the file-tree snippet is rendered and linted correctly.
1 participant
Summary by CodeRabbit
New Features
Chores
Documentation