-
Notifications
You must be signed in to change notification settings - Fork 0
5/scadable stream #7
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
Conversation
- Define TelemetryData interface for sensor data - Add WebSocketConfig and TelemetryHookResult types - Create ConnectionStatus enum for connection states - Establish foundation for type-safe telemetry handling
- Create Facility class to manage WebSocket authentication - Add API key validation and retrieval methods - Establish secure credential handling for device connections
- Create Device class to manage device ID and WebSocket connections - Add connection status tracking and event handling - Implement message parsing (JSON and raw string support) - Add subscription system for telemetry, error, and status events - Support custom WebSocket URLs and automatic cleanup
- Implement React hook for consuming live telemetry data - Add automatic connection management and cleanup - Handle connection status and error state updates - Provide clean API for React components to access real-time data
- Export all public classes and hooks - Provide TypeScript type exports - Create clean API surface for library consumers
- Create unit tests for Facility, Device, and useLiveTelemetry - Add integration tests for React components - Implement WebSocket mocking for reliable testing - Achieve 100% test coverage with 33 test cases - Add vitest configuration for React testing environment
- Add App component showing real-world usage patterns - Demonstrate temperature display and connection status - Update main.tsx to showcase the library in action - Provide working example matching user story requirements
- Add jsdom dependency for React testing environment - Update ESLint configuration for WebSocket and React globals - Configure proper linting rules for TypeScript and React - Ensure code quality standards are met
- Create detailed README with installation and usage instructions - Add API reference for all classes and hooks - Include multiple usage examples and best practices - Document WebSocket connection format and message handling - Provide development and testing guidelines
- Change Vitest configuration to use new setup file location - Refactor exports in index.ts to improve module organization - Update main.tsx to import styles from a new directory - Remove outdated test files for Device, Facility, and integration tests - Streamline test structure for better maintainability
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 implements the Scadable Stream library for real-time telemetry data consumption in React applications. The library provides a useLiveTelemetry hook for connecting to WebSocket streams and managing telemetry data state.
- Adds core classes (
Facility,Device) for managing API authentication and WebSocket connections - Implements
useLiveTelemetryReact hook for real-time telemetry data consumption - Sets up comprehensive testing infrastructure with mocked WebSocket implementation
Reviewed Changes
Copilot reviewed 24 out of 25 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| vitest.config.ts | Test configuration for Vitest with jsdom environment |
| src/core/ | Core library classes for Facility and Device management |
| src/hooks/ | React hooks including the main useLiveTelemetry hook |
| src/types.ts, src/core/types.ts | Type definitions with duplicate interfaces |
| src/utils/parseMessage.ts | Message parsing utility for WebSocket data |
| tests/ | Comprehensive test suite with WebSocket mocks |
| README.md | Complete documentation with examples and API reference |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
src/types.ts
Outdated
| /** | ||
| * Telemetry data types for the Scadable Stream library | ||
| */ | ||
|
|
||
| /** | ||
| * Base telemetry data structure | ||
| * Can contain any key-value pairs representing sensor data | ||
| */ | ||
| export interface TelemetryData { | ||
| [key: string]: any; | ||
| } | ||
|
|
||
| /** | ||
| * WebSocket message structure | ||
| */ | ||
| export interface WebSocketMessage { | ||
| data: string; | ||
| type?: string; | ||
| target?: WebSocket; | ||
| } | ||
|
|
||
| /** | ||
| * Configuration for WebSocket connection | ||
| */ | ||
| export interface WebSocketConfig { | ||
| url: string; | ||
| token: string; | ||
| deviceId: string; | ||
| } | ||
|
|
||
| /** | ||
| * Hook return type for useLiveTelemetry | ||
| */ | ||
| export interface TelemetryHookResult { | ||
| telemetry: TelemetryData | string | null; | ||
| isConnected: boolean; | ||
| error: string | null; | ||
| } | ||
|
|
||
| /** | ||
| * Device connection status | ||
| */ | ||
| export enum ConnectionStatus { | ||
| DISCONNECTED = 'disconnected', | ||
| CONNECTING = 'connecting', | ||
| CONNECTED = 'connected', | ||
| ERROR = 'error' | ||
| } |
Copilot
AI
Sep 17, 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.
This entire file duplicates type definitions that exist in src/core/types.ts. Having duplicate type definitions creates maintenance overhead and potential inconsistencies. Remove this file and use the centralized types from src/core/types.ts.
| /** | |
| * Telemetry data types for the Scadable Stream library | |
| */ | |
| /** | |
| * Base telemetry data structure | |
| * Can contain any key-value pairs representing sensor data | |
| */ | |
| export interface TelemetryData { | |
| [key: string]: any; | |
| } | |
| /** | |
| * WebSocket message structure | |
| */ | |
| export interface WebSocketMessage { | |
| data: string; | |
| type?: string; | |
| target?: WebSocket; | |
| } | |
| /** | |
| * Configuration for WebSocket connection | |
| */ | |
| export interface WebSocketConfig { | |
| url: string; | |
| token: string; | |
| deviceId: string; | |
| } | |
| /** | |
| * Hook return type for useLiveTelemetry | |
| */ | |
| export interface TelemetryHookResult { | |
| telemetry: TelemetryData | string | null; | |
| isConnected: boolean; | |
| error: string | null; | |
| } | |
| /** | |
| * Device connection status | |
| */ | |
| export enum ConnectionStatus { | |
| DISCONNECTED = 'disconnected', | |
| CONNECTING = 'connecting', | |
| CONNECTED = 'connected', | |
| ERROR = 'error' | |
| } |
| export const ConnectionStatus = { | ||
| DISCONNECTED: 'disconnected', | ||
| CONNECTING: 'connecting', | ||
| CONNECTED: 'connected', | ||
| ERROR: 'error' | ||
| } as const; |
Copilot
AI
Sep 17, 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.
This implements ConnectionStatus as a const object while src/types.ts uses an enum. This inconsistency between the two type definition files creates confusion. Since this appears to be the canonical location, ensure all imports use this implementation.
src/Facility.ts
Outdated
| /** | ||
| * Facility class manages the API key for WebSocket connections | ||
| */ | ||
| export class Facility { | ||
| private readonly apiKey: string; | ||
|
|
||
| constructor(apiKey: string) { | ||
| if (!apiKey || typeof apiKey !== 'string') { | ||
| throw new Error('API key must be a non-empty string'); | ||
| } | ||
| this.apiKey = apiKey; | ||
| } | ||
|
|
||
| /** | ||
| * Get the API key for WebSocket authentication | ||
| * @returns The API key | ||
| */ | ||
| getApiKey(): string { | ||
| return this.apiKey; | ||
| } | ||
|
|
||
| /** | ||
| * Validate that the facility has a valid API key | ||
| * @returns True if the API key is valid | ||
| */ | ||
| isValid(): boolean { | ||
| return this.apiKey.length > 0; | ||
| } | ||
| } |
Copilot
AI
Sep 17, 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.
This file duplicates the Facility class that exists in src/core/Facility.ts. Having duplicate implementations creates maintenance issues and confusion about which version is canonical. Remove this duplicate file.
| /** | |
| * Facility class manages the API key for WebSocket connections | |
| */ | |
| export class Facility { | |
| private readonly apiKey: string; | |
| constructor(apiKey: string) { | |
| if (!apiKey || typeof apiKey !== 'string') { | |
| throw new Error('API key must be a non-empty string'); | |
| } | |
| this.apiKey = apiKey; | |
| } | |
| /** | |
| * Get the API key for WebSocket authentication | |
| * @returns The API key | |
| */ | |
| getApiKey(): string { | |
| return this.apiKey; | |
| } | |
| /** | |
| * Validate that the facility has a valid API key | |
| * @returns True if the API key is valid | |
| */ | |
| isValid(): boolean { | |
| return this.apiKey.length > 0; | |
| } | |
| } | |
| // Re-export canonical Facility class | |
| export { Facility } from './core/Facility'; |
src/core/Device.ts
Outdated
| private connectionStatus: ConnectionStatusValue = ConnectionStatus.DISCONNECTED; | ||
| private messageHandlers: Set<(_data: TelemetryData | string) => void> = new Set(); | ||
| private errorHandlers: Set<(_errorMessage: string) => void> = new Set(); | ||
| private statusHandlers: Set<(_connectionStatus: ConnectionStatus) => void> = new Set(); |
Copilot
AI
Sep 17, 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 type annotation for statusHandlers is incorrect. It references ConnectionStatus which is an enum-like object, but the actual parameter type should be ConnectionStatusValue to match the implementation in the updateConnectionStatus method.
| private statusHandlers: Set<(_connectionStatus: ConnectionStatus) => void> = new Set(); | |
| private statusHandlers: Set<(_connectionStatus: ConnectionStatusValue) => void> = new Set(); |
src/core/Device.ts
Outdated
| /** | ||
| * Subscribe to connection status changes | ||
| */ | ||
| onStatusChange(handler: (_connectionStatus: ConnectionStatusValue) => void): () => void { |
Copilot
AI
Sep 17, 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 parameter type ConnectionStatusValue is correct here but inconsistent with the statusHandlers Set type definition on line 15. The Set should be typed as Set<(_connectionStatus: ConnectionStatusValue) => void> for consistency.
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
Copilot reviewed 21 out of 22 changed files in this pull request and generated 10 comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
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>
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>
#5