fix(frontend): Fix critical type safety issues from code review
Address all Critical and High Priority issues identified in frontend code review report: Critical Issues Fixed: - Created unified logger utility (lib/utils/logger.ts) to replace all console.log statements - Consolidated User type definitions - removed duplicate from authStore, using single source from types/user.ts - Eliminated 'any' types in API client - added proper generic types with AxiosRequestConfig - Fixed SignalR ConnectionManager - replaced 'any' with generic types <T> - Created API error types (lib/types/errors.ts) with ApiError and getErrorMessage helper - Fixed IssueCard component - removed all type assertions, created discriminated union types for Kanban items - Added React.memo to IssueCard for performance optimization - Added proper ARIA labels and accessibility attributes to IssueCard High Priority Issues Fixed: - Fixed hardcoded user ID in CreateProjectDialog - now uses actual user from authStore - Added useCallback to CreateProjectDialog onSubmit handler - Fixed error handlers in use-epics.ts - replaced 'any' with ApiError type - Updated all error handling to use logger and getErrorMessage Type Safety Improvements: - Created KanbanItem discriminated union (KanbanEpic | KanbanStory | KanbanTask) with proper type guards - Added 'never' types to prevent invalid property access - Fixed User interface to include all required fields (createdAt, updatedAt) - Maintained backward compatibility with LegacyKanbanBoard for existing code Files Changed: - lib/utils/logger.ts - New centralized logging utility - lib/types/errors.ts - New API error types and helpers - types/user.ts - Consolidated User type with TenantRole - types/kanban.ts - New discriminated union types for type-safe Kanban items - components/features/kanban/IssueCard.tsx - Type-safe with React.memo - components/features/projects/CreateProjectDialog.tsx - Fixed hardcoded user ID, added useCallback - lib/api/client.ts - Eliminated 'any', added proper generics - lib/signalr/ConnectionManager.ts - Replaced console.log, added generics - lib/hooks/use-epics.ts - Fixed error handler types - stores/authStore.ts - Removed duplicate User type - lib/hooks/useAuth.ts - Added createdAt field to User TypeScript compilation: ✅ All type checks passing (0 errors) 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
This commit is contained in:
@@ -1,5 +1,6 @@
|
||||
import axios, { AxiosError, InternalAxiosRequestConfig } from 'axios';
|
||||
import axios, { AxiosError, AxiosRequestConfig, InternalAxiosRequestConfig } from 'axios';
|
||||
import { API_BASE_URL } from './config';
|
||||
import { logger } from '@/lib/utils/logger';
|
||||
|
||||
// Create axios instance
|
||||
export const apiClient = axios.create({
|
||||
@@ -134,30 +135,42 @@ apiClient.interceptors.response.use(
|
||||
}
|
||||
);
|
||||
|
||||
// API helper functions
|
||||
// API helper functions with proper typing
|
||||
export const api = {
|
||||
get: async <T>(url: string, config?: any): Promise<T> => {
|
||||
const response = await apiClient.get(url, config);
|
||||
get: async <T>(url: string, config?: AxiosRequestConfig): Promise<T> => {
|
||||
const response = await apiClient.get<T>(url, config);
|
||||
return response.data;
|
||||
},
|
||||
|
||||
post: async <T>(url: string, data?: any, config?: any): Promise<T> => {
|
||||
const response = await apiClient.post(url, data, config);
|
||||
post: async <T, D = unknown>(
|
||||
url: string,
|
||||
data?: D,
|
||||
config?: AxiosRequestConfig
|
||||
): Promise<T> => {
|
||||
const response = await apiClient.post<T>(url, data, config);
|
||||
return response.data;
|
||||
},
|
||||
|
||||
put: async <T>(url: string, data?: any, config?: any): Promise<T> => {
|
||||
const response = await apiClient.put(url, data, config);
|
||||
put: async <T, D = unknown>(
|
||||
url: string,
|
||||
data?: D,
|
||||
config?: AxiosRequestConfig
|
||||
): Promise<T> => {
|
||||
const response = await apiClient.put<T>(url, data, config);
|
||||
return response.data;
|
||||
},
|
||||
|
||||
patch: async <T>(url: string, data?: any, config?: any): Promise<T> => {
|
||||
const response = await apiClient.patch(url, data, config);
|
||||
patch: async <T, D = unknown>(
|
||||
url: string,
|
||||
data?: D,
|
||||
config?: AxiosRequestConfig
|
||||
): Promise<T> => {
|
||||
const response = await apiClient.patch<T>(url, data, config);
|
||||
return response.data;
|
||||
},
|
||||
|
||||
delete: async <T>(url: string, config?: any): Promise<T> => {
|
||||
const response = await apiClient.delete(url, config);
|
||||
delete: async <T>(url: string, config?: AxiosRequestConfig): Promise<T> => {
|
||||
const response = await apiClient.delete<T>(url, config);
|
||||
return response.data;
|
||||
},
|
||||
};
|
||||
|
||||
@@ -1,6 +1,6 @@
|
||||
import { api } from './client';
|
||||
import type { Project, CreateProjectDto, UpdateProjectDto } from '@/types/project';
|
||||
import type { KanbanBoard } from '@/types/kanban';
|
||||
import type { LegacyKanbanBoard } from '@/types/kanban';
|
||||
|
||||
export const projectsApi = {
|
||||
getAll: async (page = 1, pageSize = 20): Promise<Project[]> => {
|
||||
@@ -23,7 +23,7 @@ export const projectsApi = {
|
||||
return api.delete(`/api/v1/projects/${id}`);
|
||||
},
|
||||
|
||||
getKanban: async (id: string): Promise<KanbanBoard> => {
|
||||
getKanban: async (id: string): Promise<LegacyKanbanBoard> => {
|
||||
return api.get(`/api/v1/projects/${id}/kanban`);
|
||||
},
|
||||
};
|
||||
|
||||
@@ -2,19 +2,21 @@ import { useQuery, useMutation, useQueryClient } from '@tanstack/react-query';
|
||||
import { epicsApi } from '@/lib/api/pm';
|
||||
import type { Epic, CreateEpicDto, UpdateEpicDto, WorkItemStatus } from '@/types/project';
|
||||
import { toast } from 'sonner';
|
||||
import { logger } from '@/lib/utils/logger';
|
||||
import { ApiError, getErrorMessage } from '@/lib/types/errors';
|
||||
|
||||
// ==================== Query Hooks ====================
|
||||
export function useEpics(projectId?: string) {
|
||||
return useQuery<Epic[]>({
|
||||
queryKey: ['epics', projectId],
|
||||
queryFn: async () => {
|
||||
console.log('[useEpics] Fetching epics...', { projectId });
|
||||
logger.debug('[useEpics] Fetching epics', { projectId });
|
||||
try {
|
||||
const result = await epicsApi.list(projectId);
|
||||
console.log('[useEpics] Fetch successful:', result);
|
||||
logger.debug('[useEpics] Fetch successful', result);
|
||||
return result;
|
||||
} catch (error) {
|
||||
console.error('[useEpics] Fetch failed:', error);
|
||||
logger.error('[useEpics] Fetch failed', error);
|
||||
throw error;
|
||||
}
|
||||
},
|
||||
@@ -46,9 +48,9 @@ export function useCreateEpic() {
|
||||
|
||||
toast.success('Epic created successfully!');
|
||||
},
|
||||
onError: (error: any) => {
|
||||
console.error('[useCreateEpic] Error:', error);
|
||||
toast.error(error.response?.data?.detail || 'Failed to create epic');
|
||||
onError: (error: ApiError) => {
|
||||
logger.error('[useCreateEpic] Error', error);
|
||||
toast.error(getErrorMessage(error));
|
||||
},
|
||||
});
|
||||
}
|
||||
@@ -74,15 +76,15 @@ export function useUpdateEpic() {
|
||||
|
||||
return { previousEpic };
|
||||
},
|
||||
onError: (error: any, variables, context) => {
|
||||
console.error('[useUpdateEpic] Error:', error);
|
||||
onError: (error: ApiError, variables, context) => {
|
||||
logger.error('[useUpdateEpic] Error', error);
|
||||
|
||||
// Rollback
|
||||
if (context?.previousEpic) {
|
||||
queryClient.setQueryData(['epics', variables.id], context.previousEpic);
|
||||
}
|
||||
|
||||
toast.error(error.response?.data?.detail || 'Failed to update epic');
|
||||
toast.error(getErrorMessage(error));
|
||||
},
|
||||
onSuccess: (updatedEpic) => {
|
||||
toast.success('Epic updated successfully!');
|
||||
@@ -104,9 +106,9 @@ export function useDeleteEpic() {
|
||||
queryClient.removeQueries({ queryKey: ['epics', id] });
|
||||
toast.success('Epic deleted successfully!');
|
||||
},
|
||||
onError: (error: any) => {
|
||||
console.error('[useDeleteEpic] Error:', error);
|
||||
toast.error(error.response?.data?.detail || 'Failed to delete epic');
|
||||
onError: (error: ApiError) => {
|
||||
logger.error('[useDeleteEpic] Error', error);
|
||||
toast.error(getErrorMessage(error));
|
||||
},
|
||||
});
|
||||
}
|
||||
@@ -129,14 +131,14 @@ export function useChangeEpicStatus() {
|
||||
|
||||
return { previousEpic };
|
||||
},
|
||||
onError: (error: any, variables, context) => {
|
||||
console.error('[useChangeEpicStatus] Error:', error);
|
||||
onError: (error: ApiError, variables, context) => {
|
||||
logger.error('[useChangeEpicStatus] Error', error);
|
||||
|
||||
if (context?.previousEpic) {
|
||||
queryClient.setQueryData(['epics', variables.id], context.previousEpic);
|
||||
}
|
||||
|
||||
toast.error(error.response?.data?.detail || 'Failed to change epic status');
|
||||
toast.error(getErrorMessage(error));
|
||||
},
|
||||
onSuccess: () => {
|
||||
toast.success('Epic status changed successfully!');
|
||||
@@ -159,9 +161,9 @@ export function useAssignEpic() {
|
||||
queryClient.invalidateQueries({ queryKey: ['epics'] });
|
||||
toast.success('Epic assigned successfully!');
|
||||
},
|
||||
onError: (error: any) => {
|
||||
console.error('[useAssignEpic] Error:', error);
|
||||
toast.error(error.response?.data?.detail || 'Failed to assign epic');
|
||||
onError: (error: ApiError) => {
|
||||
logger.error('[useAssignEpic] Error', error);
|
||||
toast.error(getErrorMessage(error));
|
||||
},
|
||||
});
|
||||
}
|
||||
|
||||
@@ -1,10 +1,10 @@
|
||||
import { useQuery, useMutation, useQueryClient } from '@tanstack/react-query';
|
||||
import { projectsApi } from '@/lib/api/projects';
|
||||
import type { KanbanBoard } from '@/types/kanban';
|
||||
import type { LegacyKanbanBoard } from '@/types/kanban';
|
||||
import { api } from '@/lib/api/client';
|
||||
|
||||
export function useKanbanBoard(projectId: string) {
|
||||
return useQuery<KanbanBoard>({
|
||||
return useQuery<LegacyKanbanBoard>({
|
||||
queryKey: ['projects', projectId, 'kanban'],
|
||||
queryFn: () => projectsApi.getKanban(projectId),
|
||||
enabled: !!projectId,
|
||||
|
||||
@@ -38,6 +38,8 @@ export function useLogin() {
|
||||
tenantName: data.user.tenantName,
|
||||
role: data.user.role,
|
||||
isEmailVerified: data.user.isEmailVerified,
|
||||
createdAt: data.user.createdAt || new Date().toISOString(),
|
||||
updatedAt: data.user.updatedAt,
|
||||
});
|
||||
|
||||
router.push('/dashboard');
|
||||
|
||||
@@ -1,6 +1,7 @@
|
||||
import * as signalR from '@microsoft/signalr';
|
||||
import { tokenManager } from '@/lib/api/client';
|
||||
import { SIGNALR_CONFIG } from './config';
|
||||
import { logger } from '@/lib/utils/logger';
|
||||
|
||||
export type ConnectionState =
|
||||
| 'disconnected'
|
||||
@@ -23,13 +24,13 @@ export class SignalRConnectionManager {
|
||||
this.connection &&
|
||||
this.connection.state === signalR.HubConnectionState.Connected
|
||||
) {
|
||||
console.log('[SignalR] Already connected');
|
||||
logger.debug('[SignalR] Already connected');
|
||||
return;
|
||||
}
|
||||
|
||||
const token = tokenManager.getAccessToken();
|
||||
if (!token) {
|
||||
console.warn('[SignalR] No access token found, cannot connect');
|
||||
logger.warn('[SignalR] No access token found, cannot connect');
|
||||
return;
|
||||
}
|
||||
|
||||
@@ -52,11 +53,11 @@ export class SignalRConnectionManager {
|
||||
try {
|
||||
this.notifyStateChange('connecting');
|
||||
await this.connection.start();
|
||||
console.log(`[SignalR] Connected to ${this.hubUrl}`);
|
||||
logger.info(`[SignalR] Connected to ${this.hubUrl}`);
|
||||
this.notifyStateChange('connected');
|
||||
this.reconnectAttempt = 0;
|
||||
} catch (error) {
|
||||
console.error('[SignalR] Connection error:', error);
|
||||
logger.error('[SignalR] Connection error', error);
|
||||
this.notifyStateChange('disconnected');
|
||||
this.scheduleReconnect();
|
||||
}
|
||||
@@ -67,17 +68,17 @@ export class SignalRConnectionManager {
|
||||
await this.connection.stop();
|
||||
this.connection = null;
|
||||
this.notifyStateChange('disconnected');
|
||||
console.log('[SignalR] Disconnected');
|
||||
logger.info('[SignalR] Disconnected');
|
||||
}
|
||||
}
|
||||
|
||||
on(methodName: string, callback: (...args: any[]) => void): void {
|
||||
on<T = unknown>(methodName: string, callback: (data: T) => void): void {
|
||||
if (this.connection) {
|
||||
this.connection.on(methodName, callback);
|
||||
}
|
||||
}
|
||||
|
||||
off(methodName: string, callback?: (...args: any[]) => void): void {
|
||||
off<T = unknown>(methodName: string, callback?: (data: T) => void): void {
|
||||
if (this.connection && callback) {
|
||||
this.connection.off(methodName, callback);
|
||||
} else if (this.connection) {
|
||||
@@ -85,7 +86,7 @@ export class SignalRConnectionManager {
|
||||
}
|
||||
}
|
||||
|
||||
async invoke(methodName: string, ...args: any[]): Promise<any> {
|
||||
async invoke<T = unknown>(methodName: string, ...args: unknown[]): Promise<T> {
|
||||
if (
|
||||
!this.connection ||
|
||||
this.connection.state !== signalR.HubConnectionState.Connected
|
||||
@@ -93,7 +94,7 @@ export class SignalRConnectionManager {
|
||||
throw new Error('SignalR connection is not established');
|
||||
}
|
||||
|
||||
return await this.connection.invoke(methodName, ...args);
|
||||
return await this.connection.invoke<T>(methodName, ...args);
|
||||
}
|
||||
|
||||
onStateChange(listener: (state: ConnectionState) => void): () => void {
|
||||
@@ -109,18 +110,18 @@ export class SignalRConnectionManager {
|
||||
if (!this.connection) return;
|
||||
|
||||
this.connection.onclose((error) => {
|
||||
console.log('[SignalR] Connection closed', error);
|
||||
logger.info('[SignalR] Connection closed', error);
|
||||
this.notifyStateChange('disconnected');
|
||||
this.scheduleReconnect();
|
||||
});
|
||||
|
||||
this.connection.onreconnecting((error) => {
|
||||
console.log('[SignalR] Reconnecting...', error);
|
||||
logger.info('[SignalR] Reconnecting...', error);
|
||||
this.notifyStateChange('reconnecting');
|
||||
});
|
||||
|
||||
this.connection.onreconnected((connectionId) => {
|
||||
console.log('[SignalR] Reconnected', connectionId);
|
||||
logger.info('[SignalR] Reconnected', connectionId);
|
||||
this.notifyStateChange('connected');
|
||||
this.reconnectAttempt = 0;
|
||||
});
|
||||
@@ -128,14 +129,14 @@ export class SignalRConnectionManager {
|
||||
|
||||
private scheduleReconnect(): void {
|
||||
if (this.reconnectAttempt >= SIGNALR_CONFIG.RECONNECT_DELAYS.length) {
|
||||
console.error('[SignalR] Max reconnect attempts reached');
|
||||
logger.error('[SignalR] Max reconnect attempts reached');
|
||||
return;
|
||||
}
|
||||
|
||||
const delay = SIGNALR_CONFIG.RECONNECT_DELAYS[this.reconnectAttempt];
|
||||
this.reconnectAttempt++;
|
||||
|
||||
console.log(
|
||||
logger.info(
|
||||
`[SignalR] Scheduling reconnect in ${delay}ms (attempt ${this.reconnectAttempt})`
|
||||
);
|
||||
|
||||
|
||||
43
lib/types/errors.ts
Normal file
43
lib/types/errors.ts
Normal file
@@ -0,0 +1,43 @@
|
||||
import { AxiosError } from 'axios';
|
||||
|
||||
/**
|
||||
* Standard API error response structure
|
||||
*/
|
||||
export interface ApiErrorResponse {
|
||||
message: string;
|
||||
errors?: Record<string, string[]>;
|
||||
statusCode: number;
|
||||
timestamp?: string;
|
||||
}
|
||||
|
||||
/**
|
||||
* Type-safe API error type
|
||||
*/
|
||||
export type ApiError = AxiosError<ApiErrorResponse>;
|
||||
|
||||
/**
|
||||
* Type guard to check if error is an API error
|
||||
*/
|
||||
export function isApiError(error: unknown): error is ApiError {
|
||||
return (
|
||||
typeof error === 'object' &&
|
||||
error !== null &&
|
||||
'isAxiosError' in error &&
|
||||
(error as AxiosError).isAxiosError === true
|
||||
);
|
||||
}
|
||||
|
||||
/**
|
||||
* Extract user-friendly error message from API error
|
||||
*/
|
||||
export function getErrorMessage(error: unknown): string {
|
||||
if (isApiError(error)) {
|
||||
return error.response?.data?.message || error.message || 'An unexpected error occurred';
|
||||
}
|
||||
|
||||
if (error instanceof Error) {
|
||||
return error.message;
|
||||
}
|
||||
|
||||
return 'An unexpected error occurred';
|
||||
}
|
||||
88
lib/utils/logger.ts
Normal file
88
lib/utils/logger.ts
Normal file
@@ -0,0 +1,88 @@
|
||||
/**
|
||||
* Unified logging utility for ColaFlow
|
||||
* Provides type-safe logging with environment-aware behavior
|
||||
*/
|
||||
|
||||
type LogLevel = 'debug' | 'info' | 'warn' | 'error';
|
||||
|
||||
interface LoggerConfig {
|
||||
isDevelopment: boolean;
|
||||
enableDebug: boolean;
|
||||
enableInfo: boolean;
|
||||
}
|
||||
|
||||
class Logger {
|
||||
private config: LoggerConfig;
|
||||
|
||||
constructor() {
|
||||
this.config = {
|
||||
isDevelopment: process.env.NODE_ENV === 'development',
|
||||
enableDebug: process.env.NODE_ENV === 'development',
|
||||
enableInfo: process.env.NODE_ENV === 'development',
|
||||
};
|
||||
}
|
||||
|
||||
/**
|
||||
* Debug level logging - only in development
|
||||
*/
|
||||
debug(message: string, data?: unknown): void {
|
||||
if (this.config.enableDebug) {
|
||||
console.log(`[DEBUG] ${message}`, data !== undefined ? data : '');
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* Info level logging - only in development
|
||||
*/
|
||||
info(message: string, data?: unknown): void {
|
||||
if (this.config.enableInfo) {
|
||||
console.info(`[INFO] ${message}`, data !== undefined ? data : '');
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* Warning level logging - always logged
|
||||
*/
|
||||
warn(message: string, data?: unknown): void {
|
||||
console.warn(`[WARN] ${message}`, data !== undefined ? data : '');
|
||||
}
|
||||
|
||||
/**
|
||||
* Error level logging - always logged
|
||||
* In production, this should integrate with error tracking services
|
||||
*/
|
||||
error(message: string, error?: unknown): void {
|
||||
console.error(`[ERROR] ${message}`, error !== undefined ? error : '');
|
||||
|
||||
// In production, send to error tracking service
|
||||
if (!this.config.isDevelopment) {
|
||||
// TODO: Integrate with Sentry/DataDog/etc
|
||||
// errorTracker.captureException(error, { message });
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* Log with context information for better debugging
|
||||
*/
|
||||
logWithContext(level: LogLevel, message: string, context?: Record<string, unknown>): void {
|
||||
const contextString = context ? JSON.stringify(context) : '';
|
||||
|
||||
switch (level) {
|
||||
case 'debug':
|
||||
this.debug(message, context);
|
||||
break;
|
||||
case 'info':
|
||||
this.info(message, context);
|
||||
break;
|
||||
case 'warn':
|
||||
this.warn(message, context);
|
||||
break;
|
||||
case 'error':
|
||||
this.error(message, context);
|
||||
break;
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
// Export singleton instance
|
||||
export const logger = new Logger();
|
||||
Reference in New Issue
Block a user