-
Notifications
You must be signed in to change notification settings - Fork 0
fix: refine websocket service types #35
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR refactors WebSocket and Telegram services from simple utility modules into React context providers with improved type safety. The refactoring extracts service logic from lib/ into services/ directory and provides proper TypeScript definitions for WebSocket messages and tournament data.
- Converts WebSocket and Telegram utilities to React context providers for better state management
- Improves TypeScript type safety with strict
WebSocketMessageunion types usingTournamentschema - Centralizes reconnection logic using
useReffor tracking attempts
Reviewed Changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| client/src/services/websocket.tsx | New WebSocket service with context provider and improved type definitions |
| client/src/services/telegram.tsx | New Telegram service with context provider and error handling improvements |
| client/src/pages/tournaments.tsx | Updates to use new WebSocket service context |
| client/src/pages/tournament-detail.tsx | Updates to use new Telegram service context |
| client/src/lib/websocket.ts | Replaced with re-export from services |
| client/src/lib/telegram.ts | Replaced with re-export from services |
| client/src/App.tsx | Wraps app with new service providers |
| client/README.md | Documents new services architecture |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| const reconnectTimeoutRef = useRef<number>(); | ||
| const websocketCallbacks = useRef<((message: any) => void)[]>([]); | ||
|
|
||
| const addCallback = (callback: (message: any) => void) => { |
Copilot
AI
Aug 13, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The callback parameter type any weakens type safety. Consider creating a specific type for internal callback messages that includes both WebSocketMessage and internal connection state messages like { type: 'connected' | 'disconnected' | 'error' | 'message', data?: WebSocketMessage, error?: string }.
| const addCallback = (callback: (message: any) => void) => { | |
| // Union type for all possible callback messages | |
| export type WebSocketCallbackMessage = | |
| | WebSocketMessage | |
| | { type: 'connected' } | |
| | { type: 'disconnected' } | |
| | { type: 'error'; error: string } | |
| | { type: 'message'; data: WebSocketMessage }; | |
| function useWebSocket(onMessage?: (message: WebSocketMessage) => void) { | |
| const [isConnected, setIsConnected] = useState(false); | |
| const wsRef = useRef<WebSocket | null>(null); | |
| const reconnectTimeoutRef = useRef<number>(); | |
| const websocketCallbacks = useRef<((message: WebSocketCallbackMessage) => void)[]>([]); | |
| const addCallback = (callback: (message: WebSocketCallbackMessage) => void) => { |
| const reconnectTimeoutRef = useRef<number>(); | ||
| const websocketCallbacks = useRef<((message: any) => void)[]>([]); | ||
|
|
||
| const addCallback = (callback: (message: any) => void) => { |
Copilot
AI
Aug 13, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The callback parameter type any should be replaced with a more specific type that matches the internal message structure used in the websocket callbacks.
| const addCallback = (callback: (message: any) => void) => { | |
| // Extend WebSocketMessage with internal messages for callbacks | |
| type InternalWebSocketMessage = | |
| | WebSocketMessage | |
| | { type: 'connected' } | |
| | { type: 'error'; error: string }; | |
| function useWebSocket(onMessage?: (message: WebSocketMessage) => void) { | |
| const [isConnected, setIsConnected] = useState(false); | |
| const wsRef = useRef<WebSocket | null>(null); | |
| const reconnectTimeoutRef = useRef<number>(); | |
| const websocketCallbacks = useRef<((message: InternalWebSocketMessage) => void)[]>([]); | |
| const addCallback = (callback: (message: InternalWebSocketMessage) => void) => { |
| interface WebSocketContextValue { | ||
| isConnected: boolean; | ||
| sendMessage: (message: WebSocketMessage) => void; | ||
| addCallback: (callback: (message: any) => void) => () => void; |
Copilot
AI
Aug 13, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The addCallback interface uses any type which reduces type safety. This should match the callback type used in the implementation for consistency.
| addCallback: (callback: (message: any) => void) => () => void; | |
| addCallback: (callback: (message: WebSocketMessage) => void) => () => void; |
| } | ||
| }); | ||
| useEffect(() => { | ||
| return addCallback((message: WebSocketMessage) => { |
Copilot
AI
Aug 13, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Type mismatch: addCallback expects (message: any) => void but receives (message: WebSocketMessage) => void. The callback should handle the internal message structure that includes connection state messages, not just WebSocketMessage.
Summary
Tournamentand strict event unionuseRefsendMessagein WebSocket contextTesting
npm run type-checknpm run lint:checknpm testhttps://chatgpt.com/codex/tasks/task_e_689d085d8ccc8327bc09bfbcfab8927a