Skip to content

Conversation

@WA11AX
Copy link
Owner

@WA11AX WA11AX commented Aug 13, 2025

Summary

  • type WebSocket messages with Tournament and strict event union
  • track reconnection attempts via useRef

Testing

  • npm run type-check
  • npm run lint:check
  • npm test

https://chatgpt.com/codex/tasks/task_e_689d085d8ccc8327bc09bfbcfab8927a

Copilot AI review requested due to automatic review settings August 13, 2025 22:44
Copy link
Contributor

Copilot AI left a 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 by extracting them from lib utilities into dedicated service modules with React context providers, while improving type safety for WebSocket messages.

  • Moved WebSocket and Telegram functionality from lib/ to services/ with React context providers
  • Enhanced WebSocket message types with strict Tournament typing and union types
  • Improved reconnection handling by tracking attempts via useRef instead of closure variables

Reviewed Changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 6 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 Updated to use new WebSocket service context
client/src/pages/tournament-detail.tsx Updated to use new Telegram service context
client/src/lib/websocket.ts Converted to re-export from services
client/src/lib/telegram.ts Converted to re-export from services
client/src/App.tsx Added service providers and removed direct initialization
client/README.md Updated documentation for new service 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) => {
Copy link

Copilot AI Aug 13, 2025

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. This should match the type used in the websocketCallbacks ref for consistency.

Suggested change
const addCallback = (callback: (message: any) => void) => {
// Extend the message type to include system events
export type WebSocketEvent =
| 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: WebSocketEvent) => void)[]>([]);
const addCallback = (callback: (message: WebSocketEvent) => void) => {

Copilot uses AI. Check for mistakes.
}
});
useEffect(() => {
return addCallback((message: WebSocketMessage) => {
Copy link

Copilot AI Aug 13, 2025

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 will receive internal WebSocket events like { type: 'connected' } which don't match the WebSocketMessage type.

Copilot uses AI. Check for mistakes.
WA11AX and others added 4 commits August 14, 2025 01:48
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants