From 8bae321294e09d0240fd3660d49c25d3801ca169 Mon Sep 17 00:00:00 2001 From: Val Redchenko Date: Wed, 25 Mar 2026 21:32:37 +0000 Subject: [PATCH 1/6] feat: add Keycloak authentication to smartem app Add keycloak-js auth infrastructure with AuthProvider, useAuth() hook, and automatic token refresh. Wire tokens into the shared Axios interceptor so all API calls include Bearer headers when authenticated. Replace the prototyping RoleSwitcher in the Header with real auth controls (sign in / account menu / sign out). Auth is disabled by default in dev/mock mode. --- apps/smartem/.env.example | 4 + apps/smartem/package.json | 2 + apps/smartem/src/auth/AuthGate.tsx | 12 ++ apps/smartem/src/auth/AuthProvider.tsx | 123 +++++++++++++++++++ apps/smartem/src/auth/config.ts | 14 +++ apps/smartem/src/auth/index.ts | 3 + apps/smartem/src/auth/types.ts | 17 +++ apps/smartem/src/components/shell/Header.tsx | 95 +++++++------- apps/smartem/src/main.tsx | 5 +- package-lock.json | 11 ++ packages/api/src/index.ts | 2 +- packages/api/src/mutator.ts | 13 ++ 12 files changed, 247 insertions(+), 54 deletions(-) create mode 100644 apps/smartem/.env.example create mode 100644 apps/smartem/src/auth/AuthGate.tsx create mode 100644 apps/smartem/src/auth/AuthProvider.tsx create mode 100644 apps/smartem/src/auth/config.ts create mode 100644 apps/smartem/src/auth/index.ts create mode 100644 apps/smartem/src/auth/types.ts diff --git a/apps/smartem/.env.example b/apps/smartem/.env.example new file mode 100644 index 00000000..75534e0a --- /dev/null +++ b/apps/smartem/.env.example @@ -0,0 +1,4 @@ +VITE_KEYCLOAK_URL=https://identity.diamond.ac.uk +VITE_KEYCLOAK_REALM=master +VITE_KEYCLOAK_CLIENT_ID=smartem-frontend +VITE_AUTH_ENABLED=false diff --git a/apps/smartem/package.json b/apps/smartem/package.json index b83f1759..c36275a6 100644 --- a/apps/smartem/package.json +++ b/apps/smartem/package.json @@ -15,7 +15,9 @@ "@smartem/ui": "*", "@emotion/react": "^11.14.0", "@emotion/styled": "^11.14.0", + "@mui/icons-material": "^7.3.9", "@mui/material": "^7.0.2", + "keycloak-js": "^26.1.0", "@tanstack/react-query": "^5.100.6", "@tanstack/react-router": "^1.168.3", "@tanstack/router-devtools": "^1.166.11", diff --git a/apps/smartem/src/auth/AuthGate.tsx b/apps/smartem/src/auth/AuthGate.tsx new file mode 100644 index 00000000..fe5089ae --- /dev/null +++ b/apps/smartem/src/auth/AuthGate.tsx @@ -0,0 +1,12 @@ +import { setAuthToken } from '@smartem/api' +import type { PropsWithChildren } from 'react' +import { AuthProvider } from './AuthProvider' +import { isAuthEnabled } from './config' + +export const AuthGate = ({ children }: PropsWithChildren) => { + if (!isAuthEnabled()) { + return <>{children} + } + + return {children} +} diff --git a/apps/smartem/src/auth/AuthProvider.tsx b/apps/smartem/src/auth/AuthProvider.tsx new file mode 100644 index 00000000..2218ce0b --- /dev/null +++ b/apps/smartem/src/auth/AuthProvider.tsx @@ -0,0 +1,123 @@ +import Keycloak, { type KeycloakError } from 'keycloak-js' +import { + createContext, + type PropsWithChildren, + useContext, + useEffect, + useRef, + useState, +} from 'react' +import { keycloakConfig } from './config' +import type { Auth, AuthUser } from './types' + +const MIN_SECONDS_BEFORE_EXPIRY = 10 +const MIN_REFRESH_INTERVAL_SECONDS = 5 + +const defaultAuth: Auth = { + initialised: false, + authenticated: false, + login: () => {}, + logout: () => {}, + getToken: () => '', +} + +const AuthContext = createContext(defaultAuth) + +export const useAuth = () => useContext(AuthContext) + +interface AuthProviderProps extends PropsWithChildren { + onTokenChange?: (token: string) => void +} + +function buildAuth(keycloak: Keycloak): Auth { + const auth: Auth = { + initialised: keycloak.didInitialize, + authenticated: keycloak.authenticated ?? false, + login: () => void keycloak.login({}), + logout: () => void keycloak.logout({}), + getToken: () => keycloak.token ?? '', + } + + if (keycloak.authenticated && keycloak.idTokenParsed) { + auth.user = { + name: keycloak.idTokenParsed.name ?? '', + givenName: keycloak.idTokenParsed.given_name ?? '', + familyName: keycloak.idTokenParsed.family_name ?? '', + fedId: keycloak.idTokenParsed.fedId ?? '', + email: keycloak.idTokenParsed.email ?? '', + } satisfies AuthUser + } + + return auth +} + +export const AuthProvider = ({ children, onTokenChange }: AuthProviderProps) => { + const [auth, setAuth] = useState(defaultAuth) + const refreshTimer = useRef>(undefined) + const onTokenChangeRef = useRef(onTokenChange) + onTokenChangeRef.current = onTokenChange + + useEffect(() => { + const keycloak = new Keycloak(keycloakConfig) + + const emitToken = () => { + const token = keycloak.authenticated && keycloak.token ? keycloak.token : '' + onTokenChangeRef.current?.(token) + } + + const scheduleRefresh = () => { + clearTimeout(refreshTimer.current) + + if (!keycloak.idTokenParsed?.exp || !keycloak.idTokenParsed?.iat) return + + const tokenLifetime = keycloak.idTokenParsed.exp - keycloak.idTokenParsed.iat + let refreshIn = tokenLifetime - MIN_SECONDS_BEFORE_EXPIRY + if (refreshIn < MIN_REFRESH_INTERVAL_SECONDS) { + refreshIn = MIN_REFRESH_INTERVAL_SECONDS + } + + refreshTimer.current = setTimeout(() => { + keycloak.updateToken(-1).catch((err: KeycloakError) => { + console.error('Token refresh failed:', err) + setAuth((prev) => ({ ...prev, error: 'Token refresh failed' })) + }) + }, refreshIn * 1000) + } + + keycloak.onAuthSuccess = () => { + setAuth(buildAuth(keycloak)) + emitToken() + scheduleRefresh() + } + + keycloak.onAuthRefreshSuccess = () => { + emitToken() + scheduleRefresh() + } + + keycloak.onAuthLogout = () => { + clearTimeout(refreshTimer.current) + setAuth(buildAuth(keycloak)) + emitToken() + } + + keycloak.onAuthError = (error: KeycloakError) => { + console.error('Keycloak auth error:', error) + setAuth((prev) => ({ ...prev, error: `Auth error: ${error.error}` })) + } + + keycloak + .init({ onLoad: 'check-sso' }) + .then(() => setAuth(buildAuth(keycloak))) + .catch((err) => { + console.error('Keycloak init failed:', err) + setAuth({ ...defaultAuth, initialised: true, error: 'Failed to connect to Keycloak' }) + }) + + return () => { + clearTimeout(refreshTimer.current) + } + }, []) + + return {children} +} diff --git a/apps/smartem/src/auth/config.ts b/apps/smartem/src/auth/config.ts new file mode 100644 index 00000000..785c52e0 --- /dev/null +++ b/apps/smartem/src/auth/config.ts @@ -0,0 +1,14 @@ +import type { KeycloakServerConfig } from 'keycloak-js' + +export const isAuthEnabled = (): boolean => { + if (import.meta.env.VITE_ENABLE_MOCKS === 'true') { + return import.meta.env.VITE_AUTH_ENABLED === 'true' + } + return import.meta.env.VITE_AUTH_ENABLED !== 'false' +} + +export const keycloakConfig: KeycloakServerConfig = { + url: import.meta.env.VITE_KEYCLOAK_URL || 'https://identity.diamond.ac.uk', + realm: import.meta.env.VITE_KEYCLOAK_REALM || 'master', + clientId: import.meta.env.VITE_KEYCLOAK_CLIENT_ID || 'smartem-frontend', +} diff --git a/apps/smartem/src/auth/index.ts b/apps/smartem/src/auth/index.ts new file mode 100644 index 00000000..c5bdb5dc --- /dev/null +++ b/apps/smartem/src/auth/index.ts @@ -0,0 +1,3 @@ +export { AuthGate } from './AuthGate' +export { AuthProvider, useAuth } from './AuthProvider' +export type { Auth, AuthUser } from './types' diff --git a/apps/smartem/src/auth/types.ts b/apps/smartem/src/auth/types.ts new file mode 100644 index 00000000..4f290028 --- /dev/null +++ b/apps/smartem/src/auth/types.ts @@ -0,0 +1,17 @@ +export type AuthUser = { + name: string + givenName: string + familyName: string + fedId: string + email: string +} + +export interface Auth { + initialised: boolean + authenticated: boolean + user?: AuthUser + login: () => void + logout: () => void + getToken: () => string + error?: string +} diff --git a/apps/smartem/src/components/shell/Header.tsx b/apps/smartem/src/components/shell/Header.tsx index e2de80d1..519b0e45 100644 --- a/apps/smartem/src/components/shell/Header.tsx +++ b/apps/smartem/src/components/shell/Header.tsx @@ -1,7 +1,7 @@ +import { AccountCircle, Login } from '@mui/icons-material' import { AppBar, Box, - Button, Divider, IconButton, InputBase, @@ -14,7 +14,8 @@ import { Typography, } from '@mui/material' import { Link, useNavigate, useRouterState } from '@tanstack/react-router' -import { useCallback, useMemo, useRef, useState } from 'react' +import { useMemo, useState } from 'react' +import { useAuth } from '~/auth' import { type CommandGroup, CommandPalette } from '~/components/widgets/CommandPalette' import { sessions } from '~/data/mock-dashboard' import { gray } from '~/theme' @@ -53,22 +54,6 @@ function NavLink({ label, to }: { label: string; to: string }) { ) } -type UserRole = 'visitor' | 'staff' | 'admin' - -const ROLE_KEY = 'smartem-user-role' - -const ROLES: { key: UserRole; label: string; short: string }[] = [ - { key: 'visitor', label: 'Visitor user', short: 'Visitor' }, - { key: 'staff', label: 'Facility staff', short: 'Staff' }, - { key: 'admin', label: 'System admin', short: 'Admin' }, -] - -function readRole(): UserRole { - const v = localStorage.getItem(ROLE_KEY) - if (v === 'visitor' || v === 'staff' || v === 'admin') return v - return 'staff' -} - export function Header() { const navigate = useNavigate() const [paletteOpen, setPaletteOpen] = useState(false) @@ -234,7 +219,7 @@ export function Header() { - + @@ -349,46 +334,52 @@ function SettingsMenu() { ) } -function RoleSwitcher() { - const [role, setRole] = useState(readRole) - const [open, setOpen] = useState(false) - const anchorRef = useRef(null) +function AuthControls() { + const auth = useAuth() + const [anchorEl, setAnchorEl] = useState(null) - const pick = useCallback((r: UserRole) => { - setRole(r) - localStorage.setItem(ROLE_KEY, r) - setOpen(false) - }, []) + if (!auth.initialised) return null - const current = ROLES.find((r) => r.key === role) ?? ROLES[1] + if (!auth.authenticated) { + return ( + + auth.login()} sx={{ color: 'text.secondary' }}> + + + + ) + } return ( <> - + + setAnchorEl(e.currentTarget)} + sx={{ color: 'text.secondary' }} + > + + + setOpen(false)} - slotProps={{ paper: { sx: { mt: 0.5, minWidth: 160 } } }} + anchorEl={anchorEl} + open={Boolean(anchorEl)} + onClose={() => setAnchorEl(null)} + slotProps={{ paper: { sx: { mt: 0.5, minWidth: 180 } } }} > - {ROLES.map((r) => ( - pick(r.key)} - sx={{ fontSize: '0.8125rem' }} - > - {r.label} - - ))} + + {auth.user?.name || auth.user?.email} + + + { + setAnchorEl(null) + auth.logout() + }} + sx={{ fontSize: '0.8125rem' }} + > + Sign out + ) diff --git a/apps/smartem/src/main.tsx b/apps/smartem/src/main.tsx index b53f9c6a..d8d10fa0 100644 --- a/apps/smartem/src/main.tsx +++ b/apps/smartem/src/main.tsx @@ -1,6 +1,7 @@ import { createRouter, RouterProvider } from '@tanstack/react-router' import { StrictMode } from 'react' import { createRoot } from 'react-dom/client' +import { AuthGate } from './auth' import { routeTree } from './routeTree.gen' import './app.css' @@ -30,7 +31,9 @@ if (rootElement && !rootElement.innerHTML) { const root = createRoot(rootElement) root.render( - + + + ) }) diff --git a/package-lock.json b/package-lock.json index 4d5117d1..d1c75ecf 100644 --- a/package-lock.json +++ b/package-lock.json @@ -57,12 +57,14 @@ "dependencies": { "@emotion/react": "^11.14.0", "@emotion/styled": "^11.14.0", + "@mui/icons-material": "^7.3.9", "@mui/material": "^7.0.2", "@smartem/api": "*", "@smartem/ui": "*", "@tanstack/react-query": "^5.100.6", "@tanstack/react-router": "^1.168.3", "@tanstack/router-devtools": "^1.166.11", + "keycloak-js": "^26.1.0", "react": "^19.2.4", "react-dom": "^19.2.4" }, @@ -4883,6 +4885,15 @@ "node": ">=0.10.0" } }, + "node_modules/keycloak-js": { + "version": "26.2.3", + "resolved": "https://registry.npmjs.org/keycloak-js/-/keycloak-js-26.2.3.tgz", + "integrity": "sha512-widjzw/9T6bHRgEp6H/Se3NCCarU7u5CwFKBcwtu7xfA1IfdZb+7Q7/KGusAnBo34Vtls8Oz9vzSqkQvQ7+b4Q==", + "license": "Apache-2.0", + "workspaces": [ + "test" + ] + }, "node_modules/lefthook": { "version": "2.1.6", "resolved": "https://registry.npmjs.org/lefthook/-/lefthook-2.1.6.tgz", diff --git a/packages/api/src/index.ts b/packages/api/src/index.ts index b1915c16..1cf0466b 100644 --- a/packages/api/src/index.ts +++ b/packages/api/src/index.ts @@ -9,7 +9,7 @@ export * as mswHandlers from './generated/default/default.msw' // Generated TypeScript models/types export * from './generated/models' // Axios mutator and helpers -export { ApiError, AXIOS_INSTANCE, apiUrl, customInstance } from './mutator' +export { ApiError, AXIOS_INSTANCE, apiUrl, customInstance, setAuthToken } from './mutator' // API version checking export { checkApiVersion, diff --git a/packages/api/src/mutator.ts b/packages/api/src/mutator.ts index ca8f9fe8..84bb763f 100644 --- a/packages/api/src/mutator.ts +++ b/packages/api/src/mutator.ts @@ -14,6 +14,19 @@ export const AXIOS_INSTANCE = Axios.create({ baseURL: apiUrl(), }) +let authToken: string | null = null + +export const setAuthToken = (token: string | null) => { + authToken = token || null +} + +AXIOS_INSTANCE.interceptors.request.use((config) => { + if (authToken) { + config.headers.Authorization = `Bearer ${authToken}` + } + return config +}) + export class ApiError extends Error { constructor( message: string, From f95be8833eda9f83d4e29bb44a2b28e4392d429c Mon Sep 17 00:00:00 2001 From: Val Redchenko Date: Wed, 25 Mar 2026 21:58:56 +0000 Subject: [PATCH 2/6] chore: delete enhancements.md legacy analysis Content recycled to smartem-frontend #39 (tier 3 code examples and re-evaluation comment) and smartem-devtools PR #160 (NFR suggestions). The file analysed the legacy app and is superseded by the curated roadmap in issue #39. --- enhancements.md | 2202 ----------------------------------------------- 1 file changed, 2202 deletions(-) delete mode 100644 enhancements.md diff --git a/enhancements.md b/enhancements.md deleted file mode 100644 index 2a2ed090..00000000 --- a/enhancements.md +++ /dev/null @@ -1,2202 +0,0 @@ -# SmartEM Frontend - Comprehensive Improvement Analysis - -**Analysis Date:** 2025-10-07 -**Framework:** React 19 + React Router 7 + TypeScript -**Current State:** Production-ready application with modern tooling - ---- - -## Executive Summary - -The SmartEM frontend is a well-architected React application using modern technologies including React 19, React Router 7, TanStack Query, and Material-UI. The codebase demonstrates good practices in several areas, particularly in API client generation and type safety. However, there are significant opportunities for improvement in code organization, component architecture, testing, and developer tooling. - -**Overall Assessment:** 7/10 - Solid foundation with room for modernization and refinement. - ---- - -## 1. Project Structure and Organization - -### Current State - -``` -app/ -├── api/ -│ ├── generated/ # Auto-generated (300+ files) -│ ├── mutator.ts -│ ├── version-check.ts -│ └── openapi.json -├── components/ # 6 components, mixed purposes -├── hooks/ # Empty directory -├── mocks/ # MSW setup (2 files) -├── routes/ # 10 route components (94-432 lines each) -├── utils/ # Empty directory -├── routes.ts -├── root.tsx -└── entry.client.tsx -``` - -### Issues Identified - -1. **Flat component structure** - All components in single directory without categorization -2. **Large route components** - Some routes have 400+ lines (atlas.tsx: 432 lines, squareLR.tsx: 396 lines) -3. **Empty directories** - `hooks/` and `utils/` directories exist but are empty -4. **Mixed responsibilities** - Components contain business logic, API calls, and presentation -5. **No feature-based organization** - Everything organized by technical type rather than domain - -### Recommended Improvements - -#### 1.1 Adopt Feature-Based Structure - -**Effort:** Medium | **Priority:** High | **Impact:** High - -Reorganize code by domain features: - -``` -app/ -├── features/ -│ ├── acquisitions/ -│ │ ├── components/ -│ │ ├── hooks/ -│ │ ├── routes/ -│ │ └── types.ts -│ ├── atlas/ -│ │ ├── components/ -│ │ ├── hooks/ -│ │ └── routes/ -│ ├── grids/ -│ └── predictions/ -├── shared/ -│ ├── components/ # Navbar, ApiVersionCheck, etc. -│ ├── hooks/ -│ ├── utils/ -│ └── theme/ -├── api/ -└── core/ # Root-level app setup -``` - -**Benefits:** - -- Better code discoverability -- Easier to understand feature scope -- Reduced coupling between features -- Easier onboarding for new developers -- Clearer ownership boundaries - -#### 1.2 Split Large Route Components - -**Effort:** Medium | **Priority:** High | **Impact:** High - -Break down large route files (atlas.tsx, squareLR.tsx, grid.tsx) into smaller, focused components: - -**Current:** `app/routes/atlas.tsx` (432 lines) -**Proposed:** - -``` -app/features/atlas/ -├── routes/ -│ └── AtlasRoute.tsx (50-100 lines) -├── components/ -│ ├── AtlasViewer.tsx -│ ├── LatentSpacePanel.tsx -│ ├── PredictionControls.tsx -│ └── GridSquareOverlay.tsx -├── hooks/ -│ ├── useAtlasData.ts -│ ├── usePredictions.ts -│ └── useLatentRepresentation.ts -└── types.ts -``` - -**Benefits:** - -- Improved testability -- Better code reuse -- Easier to understand and maintain -- Clearer separation of concerns - -#### 1.3 Create Proper Barrel Exports - -**Effort:** Low | **Priority:** Medium | **Impact:** Medium - -Add index.ts files for cleaner imports: - -```typescript -// app/features/atlas/index.ts -export { AtlasRoute } from './routes/AtlasRoute' -export * from './types' - -// Usage -import { AtlasRoute } from '~/features/atlas' -``` - -**Benefits:** - -- Cleaner import statements -- Better encapsulation -- Easier refactoring - ---- - -## 2. TypeScript Usage and Type Safety - -### Current State - -**Strengths:** - -- TypeScript strict mode enabled (`tsconfig.json`: `"strict": true`) -- Auto-generated types from OpenAPI spec -- Comprehensive API type coverage -- Proper type exports from generated code - -**Issues:** - -1. **4 instances of `@ts-ignore` or `any` types** in application code -2. **Inconsistent type imports** - Mix of `import type` and regular imports -3. **Missing custom types** - Domain types not formalized -4. **Inline type definitions** - Types defined in components instead of separate files - -### Recommended Improvements - -#### 2.1 Remove All Type Suppressions - -**Effort:** Low | **Priority:** High | **Impact:** Medium - -Current issues in `/home/vredchenko/dev/DLS/smartem-frontend/app/api/mutator.ts`: - -```typescript -// @ts-ignore -promise.cancel = () => { - source.cancel('Query was cancelled') -} -``` - -**Solution:** - -```typescript -type CancellablePromise = Promise & { - cancel: () => void -} - -export const customInstance = ( - config: AxiosRequestConfig, - options?: AxiosRequestConfig -): CancellablePromise => { - const source = Axios.CancelToken.source() - const promise = AXIOS_INSTANCE({ - ...config, - ...options, - cancelToken: source.token, - }).then(({ data }) => data) as CancellablePromise - - promise.cancel = () => { - source.cancel('Query was cancelled') - } - - return promise -} -``` - -**Benefits:** - -- Full type safety -- Better IDE autocomplete -- Catch errors at compile time - -#### 2.2 Consolidate Types into Domain Files - -**Effort:** Medium | **Priority:** Medium | **Impact:** Medium - -**Current:** Types scattered across route files - -```typescript -// In atlas.tsx -type GridSquare = GridSquareResponse -type PredictionModel = QualityPredictionModelResponse -type Coords = { x: number; y: number; index: number } -``` - -**Proposed:** - -```typescript -// app/features/atlas/types.ts -export type GridSquare = GridSquareResponse -export type PredictionModel = QualityPredictionModelResponse -export type Coords = { x: number; y: number; index: number } -export type AtlasViewState = { - selectedSquare: string - showPredictions: boolean - selectionFrozen: boolean -} -``` - -**Benefits:** - -- Single source of truth for types -- Easier to share types across files -- Better type documentation - -#### 2.3 Use Consistent Type Import Syntax - -**Effort:** Low | **Priority:** Low | **Impact:** Low - -Standardize on `import type` for all type-only imports: - -```typescript -// Before (inconsistent) -import { GridSquareResponse } from '../api/generated/models' -import type { SelectChangeEvent } from '@mui/material' - -// After (consistent) -import type { GridSquareResponse } from '../api/generated/models' -import type { SelectChangeEvent } from '@mui/material' -``` - -**Benefits:** - -- Clearer intent -- Better tree-shaking -- Faster compilation - -#### 2.4 Enable Additional TypeScript Strict Checks - -**Effort:** Low | **Priority:** Medium | **Impact:** Medium - -Update `tsconfig.json`: - -```json -{ - "compilerOptions": { - "strict": true, - "noUncheckedIndexedAccess": true, - "noImplicitOverride": true, - "exactOptionalPropertyTypes": true - } -} -``` - -**Benefits:** - -- Catch more potential runtime errors -- Better null/undefined handling -- Stronger type guarantees - ---- - -## 3. React Patterns and Best Practices - -### Current State - -**Good Practices:** - -- Using React 19 features -- Functional components throughout -- React.StrictMode enabled -- Using hooks appropriately - -**Issues Identified:** - -1. **72 instances of `React.useState`, `React.useEffect`** - Unnecessary namespace usage -2. **No custom hooks** - Business logic embedded in components -3. **Prop drilling** - No context usage for shared state -4. **Mixed concerns** - Components handle data fetching, state, and UI -5. **Inline styles** - Heavy use of `style={}` props (78 instances) -6. **No component composition patterns** - Monolithic components -7. **Missing error boundaries** - Only root-level error boundary -8. **No loading states abstraction** - Repeated loading/error patterns - -### Recommended Improvements - -#### 3.1 Remove React Namespace from Hooks - -**Effort:** Low | **Priority:** Medium | **Impact:** Low - -**Current:** - -```typescript -import React from 'react' -const [state, setState] = React.useState(false) -``` - -**Proposed:** - -```typescript -import { useState } from 'react' -const [state, setState] = useState(false) -``` - -**Benefits:** - -- Cleaner code -- Modern React conventions -- Smaller bundle (tree-shaking) - -#### 3.2 Extract Custom Hooks - -**Effort:** Medium | **Priority:** High | **Impact:** High - -**Current:** Business logic in components - -```typescript -// In atlas.tsx (432 lines) -export default function Atlas({ loaderData, params }: Route.ComponentProps) { - const [predictions, setPredictions] = React.useState>() - const [latentRep, setLatentRep] = React.useState>() - - const handleChange = async (event: SelectChangeEvent) => { - setPredictionModel(event.target.value) - const preds = await getPredictions(event.target.value, params.gridId) - setPredictions(preds) - } - // ... 400 more lines -} -``` - -**Proposed:** - -```typescript -// app/features/atlas/hooks/usePredictions.ts -export function usePredictions(gridId: string) { - const [predictions, setPredictions] = useState>() - const [model, setModel] = useState('') - const [isLoading, setIsLoading] = useState(false) - - const loadPredictions = useCallback( - async (modelName: string) => { - setIsLoading(true) - setModel(modelName) - try { - const preds = await getPredictions(modelName, gridId) - setPredictions(preds) - } finally { - setIsLoading(false) - } - }, - [gridId] - ) - - return { predictions, model, isLoading, loadPredictions } -} - -// app/features/atlas/routes/AtlasRoute.tsx -export default function Atlas({ loaderData, params }: Route.ComponentProps) { - const { predictions, loadPredictions } = usePredictions(params.gridId) - const { latentRep, loadLatentRep } = useLatentRepresentation(params.gridId) - // Component now 100 lines, focused on UI -} -``` - -**Benefits:** - -- Testable business logic -- Reusable across components -- Easier to understand component purpose -- Better separation of concerns - -#### 3.3 Create Shared UI Component Abstractions - -**Effort:** Medium | **Priority:** High | **Impact:** High - -**Pattern identified:** Repeated loading/error/data pattern - -```typescript -// Appears in multiple routes -{isLoading ? ( - - - -) : error ? ( - Error: {error.message} -) : ( - // render data -)} -``` - -**Proposed:** - -```typescript -// app/shared/components/QueryStateHandler.tsx -export function QueryStateHandler({ - isLoading, - error, - data, - children, - loadingMessage = 'Loading...', - emptyMessage = 'No data found' -}: QueryStateHandlerProps) { - if (isLoading) return - if (error) return - if (!data || (Array.isArray(data) && data.length === 0)) { - return - } - return <>{children(data)} -} - -// Usage - - {(data) => } - -``` - -**Benefits:** - -- DRY principle -- Consistent UX -- Easier to update loading/error states globally - -#### 3.4 Replace Inline Styles with Styled Components or Theme - -**Effort:** High | **Priority:** Medium | **Impact:** Medium - -**Current:** 78 instances of inline styles - -```typescript - - -``` - -**Proposed Option 1 - MUI sx prop:** - -```typescript - - -``` - -**Proposed Option 2 - styled components:** - -```typescript -const StyledContainer = styled(Container)(({ theme }) => ({ - width: '100%', - paddingTop: theme.spacing(6.25), -})) -``` - -**Benefits:** - -- Theme-aware styling -- Type-safe style props -- Better performance (no style recalculation) -- Easier to maintain - -#### 3.5 Implement Compound Components Pattern - -**Effort:** Medium | **Priority:** Medium | **Impact:** Medium - -For complex components like Atlas viewer: - -```typescript -// app/features/atlas/components/AtlasViewer/index.tsx -export const AtlasViewer = { - Root: AtlasViewerRoot, - Image: AtlasImage, - Overlay: GridSquareOverlay, - Controls: AtlasControls, - LatentPanel: LatentSpacePanel -} - -// Usage - - - - - - - - {showLatent && } - -``` - -**Benefits:** - -- Better component composition -- Clearer component hierarchy -- More flexible layouts -- Self-documenting API - -#### 3.6 Add Error Boundaries per Feature - -**Effort:** Low | **Priority:** Medium | **Impact:** Medium - -**Current:** Only root error boundary - -**Proposed:** - -```typescript -// app/shared/components/ErrorBoundary.tsx -export class FeatureErrorBoundary extends Component { - static getDerivedStateFromError(error: Error) { - return { hasError: true, error } - } - - componentDidCatch(error: Error, errorInfo: ErrorInfo) { - logErrorToService(error, errorInfo) - } - - render() { - if (this.state.hasError) { - return this.props.fallback || - } - return this.props.children - } -} - -// Usage in routes - - - -``` - -**Benefits:** - -- Graceful degradation -- Better error isolation -- Improved UX -- Error tracking - ---- - -## 4. Build Tooling and Configuration - -### Current State - -**Configuration Files:** - -- `vite.config.ts` - Minimal config (18 lines) -- `react-router.config.ts` - Very basic (7 lines) -- `tsconfig.json` - Good strict settings -- `orval.config.ts` - Well configured - -**Strengths:** - -- Modern Vite setup -- Fast HMR -- Good proxy configuration -- TypeScript path aliases configured - -**Issues:** - -1. **No bundle analysis** - Can't analyze bundle size -2. **No environment validation** - No schema for env vars -3. **No source maps configuration** - Debug experience could be better -4. **No build optimization config** - Using defaults -5. **Missing performance budgets** - -### Recommended Improvements - -#### 4.1 Add Bundle Analysis - -**Effort:** Low | **Priority:** Medium | **Impact:** Medium - -```typescript -// vite.config.ts -import { visualizer } from 'rollup-plugin-visualizer' - -export default defineConfig(({ mode }) => ({ - plugins: [ - tailwindcss(), - reactRouter(), - tsconfigPaths(), - mode === 'analyze' && - visualizer({ - open: true, - filename: 'bundle-analysis.html', - gzipSize: true, - brotliSize: true, - }), - ].filter(Boolean), -})) -``` - -Add script to `package.json`: - -```json -{ - "scripts": { - "analyze": "vite build --mode analyze" - } -} -``` - -**Benefits:** - -- Identify large dependencies -- Optimize bundle size -- Find code splitting opportunities - -#### 4.2 Add Environment Variable Validation - -**Effort:** Low | **Priority:** High | **Impact:** Medium - -```typescript -// app/config/env.ts -import { z } from 'zod' - -const envSchema = z.object({ - VITE_API_ENDPOINT: z.string().url().optional(), - VITE_ENABLE_MOCKS: z - .string() - .transform((val) => val === 'true') - .optional(), - MODE: z.enum(['development', 'production', 'test']), - DEV: z.boolean(), - PROD: z.boolean(), -}) - -export const env = envSchema.parse({ - VITE_API_ENDPOINT: import.meta.env.VITE_API_ENDPOINT, - VITE_ENABLE_MOCKS: import.meta.env.VITE_ENABLE_MOCKS, - MODE: import.meta.env.MODE, - DEV: import.meta.env.DEV, - PROD: import.meta.env.PROD, -}) - -// Usage -import { env } from '~/config/env' -const apiUrl = env.VITE_API_ENDPOINT || 'http://localhost:8000' -``` - -**Benefits:** - -- Type-safe environment variables -- Fail fast on misconfiguration -- Clear documentation of required env vars - -#### 4.3 Optimize Build Configuration - -**Effort:** Medium | **Priority:** Medium | **Impact:** Medium - -```typescript -// vite.config.ts -export default defineConfig({ - plugins: [tailwindcss(), reactRouter(), tsconfigPaths()], - - build: { - target: 'es2022', - minify: 'terser', - terserOptions: { - compress: { - drop_console: true, // Remove console.logs in production - drop_debugger: true, - }, - }, - rollupOptions: { - output: { - manualChunks: { - 'vendor-react': ['react', 'react-dom', 'react-router'], - 'vendor-mui': ['@mui/material', '@mui/icons-material'], - 'vendor-query': ['@tanstack/react-query'], - 'vendor-charts': ['@mui/x-charts', 'd3-array'], - }, - }, - }, - sourcemap: true, - chunkSizeWarningLimit: 1000, - }, - - server: { - proxy: { - '/api': { - target: 'http://localhost:8000', - changeOrigin: true, - rewrite: (path) => path.replace(/^\/api/, ''), - }, - }, - }, -}) -``` - -**Benefits:** - -- Better caching -- Faster initial load -- Smaller bundle sizes -- Better debugging - -#### 4.4 Add Performance Budgets - -**Effort:** Low | **Priority:** Low | **Impact:** Low - -```json -// package.json -{ - "budgets": { - "client/index.html": "500kb", - "client/assets/*.js": "200kb", - "client/assets/*.css": "50kb" - } -} -``` - -**Benefits:** - -- Prevent bundle bloat -- Maintain performance standards -- Early warning system - ---- - -## 5. Testing Setup and Coverage - -### Current State - -**Issues:** - -- **NO testing framework configured** (no vitest, jest, or playwright) -- **NO test files** found in the codebase -- **NO test scripts** in package.json -- **0% test coverage** -- NO CI/CD pipeline detected - -This is the **most critical gap** in the codebase. - -### Recommended Improvements - -#### 5.1 Set Up Vitest for Unit/Integration Tests - -**Effort:** Medium | **Priority:** CRITICAL | **Impact:** High - -**Install dependencies:** - -```json -{ - "devDependencies": { - "@testing-library/react": "^16.0.0", - "@testing-library/jest-dom": "^6.1.5", - "@testing-library/user-event": "^14.5.1", - "@vitest/ui": "^2.0.0", - "vitest": "^2.0.0", - "happy-dom": "^12.10.3" - } -} -``` - -**Create config:** - -```typescript -// vitest.config.ts -import { defineConfig } from 'vitest/config' -import react from '@vitejs/plugin-react' -import tsconfigPaths from 'vite-tsconfig-paths' - -export default defineConfig({ - plugins: [react(), tsconfigPaths()], - test: { - globals: true, - environment: 'happy-dom', - setupFiles: ['./app/test/setup.ts'], - coverage: { - provider: 'v8', - reporter: ['text', 'html', 'json'], - exclude: ['app/api/generated/**', '**/*.config.{ts,js}', '**/types.ts'], - thresholds: { - lines: 70, - functions: 70, - branches: 70, - statements: 70, - }, - }, - }, -}) -``` - -**Add scripts:** - -```json -{ - "scripts": { - "test": "vitest", - "test:ui": "vitest --ui", - "test:coverage": "vitest --coverage" - } -} -``` - -**Benefits:** - -- Catch bugs early -- Safer refactoring -- Documentation through tests -- Better code quality - -#### 5.2 Create Test Structure - -**Effort:** Low | **Priority:** High | **Impact:** Medium - -``` -app/ -├── test/ -│ ├── setup.ts -│ ├── utils/ -│ │ ├── test-utils.tsx # Custom render with providers -│ │ ├── mocks/ # Mock data factories -│ │ └── fixtures.ts -│ └── __mocks__/ # Global mocks -└── features/ - └── atlas/ - ├── components/ - │ ├── AtlasViewer.tsx - │ └── __tests__/ - │ └── AtlasViewer.test.tsx - └── hooks/ - ├── usePredictions.ts - └── __tests__/ - └── usePredictions.test.ts -``` - -#### 5.3 Write Tests for Critical Paths - -**Effort:** High | **Priority:** High | **Impact:** High - -**Example hook test:** - -```typescript -// app/features/atlas/hooks/__tests__/usePredictions.test.ts -import { renderHook, waitFor } from '@testing-library/react' -import { QueryClient, QueryClientProvider } from '@tanstack/react-query' -import { usePredictions } from '../usePredictions' - -describe('usePredictions', () => { - it('should load predictions when model changes', async () => { - const { result } = renderHook(() => usePredictions('grid-123'), { - wrapper: createQueryWrapper(), - }) - - expect(result.current.predictions).toBeUndefined() - - await act(() => result.current.loadPredictions('model-1')) - - await waitFor(() => { - expect(result.current.predictions).toBeDefined() - expect(result.current.isLoading).toBe(false) - }) - }) -}) -``` - -**Example component test:** - -```typescript -// app/shared/components/__tests__/QueryStateHandler.test.tsx -import { render, screen } from '@testing-library/react' -import { QueryStateHandler } from '../QueryStateHandler' - -describe('QueryStateHandler', () => { - it('shows loading state', () => { - render( - - {() =>
Data
} -
- ) - expect(screen.getByRole('progressbar')).toBeInTheDocument() - }) - - it('shows error state', () => { - const error = new Error('Failed to load') - render( - - {() =>
Data
} -
- ) - expect(screen.getByText(/failed to load/i)).toBeInTheDocument() - }) -}) -``` - -**Priority test coverage:** - -1. Custom hooks (100% coverage goal) -2. Shared components (90% coverage goal) -3. API utilities (80% coverage goal) -4. Route components (60% coverage goal - mainly integration tests) - -#### 5.4 Add E2E Testing with Playwright - -**Effort:** Medium | **Priority:** Medium | **Impact:** High - -```typescript -// playwright.config.ts -import { defineConfig } from '@playwright/test' - -export default defineConfig({ - testDir: './e2e', - fullyParallel: true, - forbidOnly: !!process.env.CI, - retries: process.env.CI ? 2 : 0, - use: { - baseURL: 'http://localhost:5174', - trace: 'on-first-retry', - }, - webServer: { - command: 'npm run dev', - url: 'http://localhost:5174', - reuseExistingServer: !process.env.CI, - }, -}) -``` - -**Example E2E test:** - -```typescript -// e2e/acquisition-flow.spec.ts -import { test, expect } from '@playwright/test' - -test('user can view and select acquisition', async ({ page }) => { - await page.goto('/') - - // Should show acquisitions table - await expect(page.getByRole('table')).toBeVisible() - - // Click on first acquisition - await page.getByRole('row').first().click() - - // Should navigate to grids page - await expect(page).toHaveURL(/\/acquisitions\/.*/) - await expect(page.getByRole('heading', { name: /grids/i })).toBeVisible() -}) -``` - -**Benefits:** - -- Catch UI regressions -- Test user workflows -- Confidence in deployments - ---- - -## 6. Code Quality Tools - -### Current State - -**Configured:** - -- Prettier (`.prettierrc`) - Basic configuration -- TypeScript (strict mode enabled) - -**Missing:** - -- ESLint -- Husky (git hooks) -- lint-staged -- Commitlint -- Import sorting - -### Recommended Improvements - -#### 6.1 Add ESLint Configuration - -**Effort:** Low | **Priority:** High | **Impact:** Medium - -```javascript -// eslint.config.js -import js from '@eslint/js' -import typescript from '@typescript-eslint/eslint-plugin' -import tsParser from '@typescript-eslint/parser' -import react from 'eslint-plugin-react' -import reactHooks from 'eslint-plugin-react-hooks' -import jsxA11y from 'eslint-plugin-jsx-a11y' -import importPlugin from 'eslint-plugin-import' - -export default [ - js.configs.recommended, - { - files: ['**/*.{ts,tsx}'], - plugins: { - '@typescript-eslint': typescript, - react: react, - 'react-hooks': reactHooks, - 'jsx-a11y': jsxA11y, - import: importPlugin, - }, - languageOptions: { - parser: tsParser, - parserOptions: { - ecmaVersion: 'latest', - sourceType: 'module', - ecmaFeatures: { jsx: true }, - }, - }, - rules: { - // TypeScript - '@typescript-eslint/no-explicit-any': 'error', - '@typescript-eslint/no-unused-vars': [ - 'error', - { - argsIgnorePattern: '^_', - varsIgnorePattern: '^_', - }, - ], - '@typescript-eslint/explicit-function-return-type': 'off', - '@typescript-eslint/consistent-type-imports': 'error', - - // React - 'react/react-in-jsx-scope': 'off', // Not needed in React 19 - 'react-hooks/rules-of-hooks': 'error', - 'react-hooks/exhaustive-deps': 'warn', - 'react/prop-types': 'off', // Using TypeScript - - // Accessibility - 'jsx-a11y/alt-text': 'error', - 'jsx-a11y/aria-props': 'error', - - // Imports - 'import/order': [ - 'error', - { - groups: [ - 'builtin', - 'external', - 'internal', - 'parent', - 'sibling', - 'index', - ], - 'newlines-between': 'always', - alphabetize: { order: 'asc' }, - }, - ], - 'import/no-duplicates': 'error', - }, - }, -] -``` - -**Add scripts:** - -```json -{ - "scripts": { - "lint": "eslint . --ext .ts,.tsx", - "lint:fix": "eslint . --ext .ts,.tsx --fix" - } -} -``` - -**Benefits:** - -- Catch common errors -- Enforce code style -- Improve code quality -- Better team consistency - -#### 6.2 Set Up Git Hooks with Husky - -**Effort:** Low | **Priority:** Medium | **Impact:** High - -```bash -npm install -D husky lint-staged @commitlint/cli @commitlint/config-conventional -npx husky init -``` - -```json -// package.json -{ - "lint-staged": { - "*.{ts,tsx}": ["eslint --fix", "prettier --write", "vitest related --run"], - "*.{json,md,css}": "prettier --write" - } -} -``` - -```javascript -// .husky/pre-commit -npm run lint-staged -``` - -```javascript -// .husky/commit-msg -npx --no -- commitlint --edit $1 -``` - -```javascript -// commitlint.config.js -export default { - extends: ['@commitlint/config-conventional'], - rules: { - 'type-enum': [ - 2, - 'always', - [ - 'feat', - 'fix', - 'docs', - 'style', - 'refactor', - 'test', - 'chore', - 'perf', - 'ci', - 'build', - ], - ], - }, -} -``` - -**Benefits:** - -- Prevent bad commits -- Enforce code quality -- Consistent commit messages -- Automated quality checks - -#### 6.3 Add Import Sorting and Organization - -**Effort:** Low | **Priority:** Low | **Impact:** Low - -```json -// .prettierrc -{ - "trailingComma": "es5", - "tabWidth": 2, - "semi": false, - "singleQuote": true, - "importOrder": ["^react", "^@?\\w", "^~/", "^[./]"], - "importOrderSeparation": true, - "importOrderSortSpecifiers": true, - "plugins": ["@trivago/prettier-plugin-sort-imports"] -} -``` - -**Benefits:** - -- Cleaner imports -- Easier to scan -- Prevent merge conflicts - ---- - -## 7. Dependencies and Package Management - -### Current State - -**Dependencies (16):** - -- React 19.1.1 -- React Router 7.9.2 -- TanStack Query 5.90.2 -- Material-UI 7.3.2 -- Axios 1.12.2 -- TypeScript 5.9.2 - -**DevDependencies (13):** - -- Vite 5.4.20 -- Orval 7.13.0 -- MSW 2.11.3 -- Tailwind CSS 4.1.13 - -**Issues:** - -1. **No lockfile validation** in CI -2. **No dependency update automation** -3. **No security audit automation** -4. **Missing peer dependencies warnings** -5. **No bundle size tracking** - -### Recommended Improvements - -#### 7.1 Add Dependency Update Automation - -**Effort:** Low | **Priority:** Medium | **Impact:** Medium - -**Create Renovate config:** - -```json -// renovate.json -{ - "$schema": "https://docs.renovatebot.com/renovate-schema.json", - "extends": ["config:base"], - "packageRules": [ - { - "matchUpdateTypes": ["minor", "patch"], - "matchCurrentVersion": "!/^0/", - "automerge": true - }, - { - "groupName": "React ecosystem", - "matchPackagePatterns": ["^react", "^@types/react"] - }, - { - "groupName": "MUI ecosystem", - "matchPackagePatterns": ["^@mui/"] - }, - { - "matchDepTypes": ["devDependencies"], - "automerge": true - } - ], - "vulnerabilityAlerts": { - "enabled": true - } -} -``` - -**Benefits:** - -- Keep dependencies updated -- Automatic security patches -- Reduce maintenance burden - -#### 7.2 Add Security Audit Script - -**Effort:** Low | **Priority:** High | **Impact:** Medium - -```json -{ - "scripts": { - "audit": "npm audit --audit-level=moderate", - "audit:fix": "npm audit fix" - } -} -``` - -**Add to CI:** - -```yaml -# .github/workflows/security.yml -name: Security Audit -on: - schedule: - - cron: '0 0 * * 0' # Weekly - pull_request: - -jobs: - audit: - runs-on: ubuntu-latest - steps: - - uses: actions/checkout@v3 - - run: npm audit --audit-level=moderate -``` - -**Benefits:** - -- Early security vulnerability detection -- Automated fixes -- Compliance - -#### 7.3 Evaluate Heavy Dependencies - -**Effort:** Medium | **Priority:** Medium | **Impact:** Medium - -**Current bundle (estimated):** - -- node_modules: 662MB -- Largest dependencies: MUI, React Router, TanStack Query - -**Recommendations:** - -1. **Consider lighter alternatives:** - - Material-UI (large) → Consider Radix UI + custom styling - - d3-array → Consider native Array methods where possible - -2. **Use selective imports:** - -```typescript -// Before -import { Box, Container, Stack } from '@mui/material' - -// After (if needed for bundle optimization) -import Box from '@mui/material/Box' -import Container from '@mui/material/Container' -import Stack from '@mui/material/Stack' -``` - -3. **Lazy load heavy features:** - -```typescript -const ChartsPanel = lazy(() => import('./components/ChartsPanel')) -``` - -**Benefits:** - -- Smaller bundle size -- Faster initial load -- Better performance - -#### 7.4 Add Package Size Limit - -**Effort:** Low | **Priority:** Low | **Impact:** Low - -```json -{ - "devDependencies": { - "size-limit": "^11.0.0", - "@size-limit/preset-app": "^11.0.0" - }, - "scripts": { - "size": "size-limit" - } -} -``` - -```javascript -// .size-limit.js -export default [ - { - name: 'Client bundle', - path: 'build/client/**/*.js', - limit: '500 KB', - }, -] -``` - -**Benefits:** - -- Prevent bundle bloat -- Performance budget enforcement - ---- - -## 8. Performance Optimizations - -### Current State - -**Strengths:** - -- React Router 7 with SSR enabled -- TanStack Query for caching (5-minute stale time) -- Vite for fast builds - -**Issues:** - -1. **No code splitting** beyond route-level -2. **No lazy loading** for heavy components -3. **No image optimization** -4. **No memoization** in expensive computations -5. **Fetch waterfalls** in some routes -6. **No prefetching** of likely next pages - -### Recommended Improvements - -#### 8.1 Implement Component Code Splitting - -**Effort:** Medium | **Priority:** High | **Impact:** High - -```typescript -// app/routes/atlas.tsx -import { lazy, Suspense } from 'react' - -const LatentSpacePanel = lazy(() => - import('../features/atlas/components/LatentSpacePanel') -) -const PredictionControls = lazy(() => - import('../features/atlas/components/PredictionControls') -) - -export default function Atlas() { - return ( - - - - }> - {showLatent && } - - - ) -} -``` - -**Benefits:** - -- Smaller initial bundle -- Faster initial load -- Better caching - -#### 8.2 Add React.memo for Expensive Components - -**Effort:** Low | **Priority:** Medium | **Impact:** Medium - -```typescript -// app/features/atlas/components/GridSquareOverlay.tsx -import { memo } from 'react' - -export const GridSquareOverlay = memo(({ - squares, - predictions, - onSquareClick -}: Props) => { - return ( - - {squares.map(square => ( - - ))} - - ) -}, (prev, next) => { - // Custom comparison - return prev.squares === next.squares && - prev.predictions === next.predictions -}) -``` - -**Benefits:** - -- Reduce unnecessary re-renders -- Better performance with large lists -- Smoother interactions - -#### 8.3 Implement Virtual Scrolling for Large Lists - -**Effort:** Medium | **Priority:** Medium | **Impact:** High - -For grid squares or large tables: - -```typescript -import { useVirtualizer } from '@tanstack/react-virtual' - -function GridSquareList({ squares }: Props) { - const parentRef = useRef(null) - - const virtualizer = useVirtualizer({ - count: squares.length, - getScrollElement: () => parentRef.current, - estimateSize: () => 50, - overscan: 10 - }) - - return ( -
-
- {virtualizer.getVirtualItems().map(virtualRow => ( - - ))} -
-
- ) -} -``` - -**Benefits:** - -- Handle thousands of items -- Constant performance regardless of list size -- Better UX - -#### 8.4 Optimize TanStack Query Configuration - -**Effort:** Low | **Priority:** Medium | **Impact:** Medium - -```typescript -// app/root.tsx -const queryClient = new QueryClient({ - defaultOptions: { - queries: { - staleTime: 1000 * 60 * 5, // 5 minutes - gcTime: 1000 * 60 * 10, // 10 minutes (was cacheTime) - retry: 1, - refetchOnWindowFocus: false, // Prevent unnecessary refetches - refetchOnReconnect: true, - }, - mutations: { - retry: 0, - onError: (error) => { - // Global error handling - console.error('Mutation error:', error) - }, - }, - }, -}) -``` - -**Add prefetching:** - -```typescript -// app/routes/home.tsx -import { useQueryClient } from '@tanstack/react-query' - -export default function Home() { - const queryClient = useQueryClient() - const { data: acquisitions } = useGetAcquisitionsAcquisitionsGet() - - const handleRowHover = (acqId: string) => { - // Prefetch grids data - queryClient.prefetchQuery({ - queryKey: getGetAcquisitionGridsQueryKey(acqId), - queryFn: () => getAcquisitionGrids(acqId) - }) - } - - return ( - handleRowHover(acq.uuid)}> - {/* ... */} - - ) -} -``` - -**Benefits:** - -- Better caching strategy -- Faster perceived performance -- Reduced API calls - -#### 8.5 Implement Image Optimization - -**Effort:** Medium | **Priority:** Medium | **Impact:** Medium - -```typescript -// app/components/OptimizedImage.tsx -export function OptimizedImage({ - src, - alt, - width, - height -}: Props) { - return ( - {alt} { - e.currentTarget.src = '/placeholder.png' - }} - /> - ) -} - -// Usage in atlas - -``` - -**Benefits:** - -- Faster page loads -- Better UX -- Reduced bandwidth - ---- - -## 9. Developer Experience Improvements - -### Current State - -**Strengths:** - -- Good README documentation -- Mock API mode for development -- Fast HMR with Vite -- TypeScript for better DX - -**Issues:** - -1. **No component documentation** (Storybook, etc.) -2. **No dev tools extensions** configured -3. **No debugging configuration** -4. **Limited error messages** -5. **No development utilities** - -### Recommended Improvements - -#### 9.1 Add Storybook for Component Documentation - -**Effort:** Medium | **Priority:** Medium | **Impact:** High - -```bash -npx storybook@latest init -``` - -```typescript -// .storybook/main.ts -export default { - stories: ['../app/**/*.stories.@(ts|tsx)'], - addons: [ - '@storybook/addon-essentials', - '@storybook/addon-interactions', - '@storybook/addon-a11y', - ], - framework: '@storybook/react-vite', -} -``` - -**Example story:** - -```typescript -// app/shared/components/QueryStateHandler.stories.tsx -import type { Meta, StoryObj } from '@storybook/react' -import { QueryStateHandler } from './QueryStateHandler' - -const meta = { - title: 'Shared/QueryStateHandler', - component: QueryStateHandler, - parameters: { - layout: 'centered', - }, - tags: ['autodocs'], -} satisfies Meta - -export default meta -type Story = StoryObj - -export const Loading: Story = { - args: { - isLoading: true, - error: null, - data: null, - children: () =>
Data
- } -} - -export const Error: Story = { - args: { - isLoading: false, - error: new Error('Failed to load'), - data: null, - children: () =>
Data
- } -} -``` - -**Benefits:** - -- Component documentation -- Visual testing -- Isolated development -- Better collaboration - -#### 9.2 Add React Query DevTools - -**Effort:** Low | **Priority:** Low | **Impact:** Medium - -```typescript -// app/root.tsx -import { ReactQueryDevtools } from '@tanstack/react-query-devtools' - -export default function App() { - return ( - - - - {import.meta.env.DEV && ( - - )} - - ) -} -``` - -**Benefits:** - -- Debug query state -- Inspect cache -- Better development experience - -#### 9.3 Add VS Code Debug Configuration - -**Effort:** Low | **Priority:** Low | **Impact:** Low - -```json -// .vscode/launch.json -{ - "version": "0.2.0", - "configurations": [ - { - "type": "chrome", - "request": "launch", - "name": "Launch Chrome against localhost", - "url": "http://localhost:5174", - "webRoot": "${workspaceFolder}" - }, - { - "type": "node", - "request": "launch", - "name": "Vitest: Current File", - "program": "${workspaceFolder}/node_modules/vitest/vitest.mjs", - "args": ["--run", "${relativeFile}"], - "smartStep": true, - "console": "integratedTerminal" - } - ] -} -``` - -**Benefits:** - -- Better debugging -- Faster development -- Easier troubleshooting - -#### 9.4 Add Development Utilities - -**Effort:** Low | **Priority:** Low | **Impact:** Medium - -```typescript -// app/utils/dev.ts -export const devLog = { - info: (...args: any[]) => { - if (import.meta.env.DEV) { - console.log('[INFO]', ...args) - } - }, - warn: (...args: any[]) => { - if (import.meta.env.DEV) { - console.warn('[WARN]', ...args) - } - }, - error: (...args: any[]) => { - console.error('[ERROR]', ...args) - }, -} - -export const devAssert = (condition: boolean, message: string) => { - if (import.meta.env.DEV && !condition) { - console.error(`Assertion failed: ${message}`) - } -} -``` - -**Benefits:** - -- Better logging -- Easier debugging -- Development-only code - ---- - -## 10. Modern Web Standards and Patterns - -### Current State - -**Strengths:** - -- Using React 19 (latest) -- ES2022 target -- Modern bundler (Vite) - -**Issues:** - -1. **No Web Vitals tracking** -2. **No PWA support** -3. **No service worker** (except MSW for mocks) -4. **No dark mode implementation** (despite theme setup) -5. **Limited accessibility features** - -### Recommended Improvements - -#### 10.1 Add Web Vitals Monitoring - -**Effort:** Low | **Priority:** Medium | **Impact:** Medium - -```typescript -// app/utils/vitals.ts -import { onCLS, onFID, onLCP, onFCP, onTTFB } from 'web-vitals' - -export function reportWebVitals(onPerfEntry?: (metric: any) => void) { - if (onPerfEntry && onPerfEntry instanceof Function) { - onCLS(onPerfEntry) - onFID(onPerfEntry) - onLCP(onPerfEntry) - onFCP(onPerfEntry) - onTTFB(onPerfEntry) - } -} - -// app/entry.client.tsx -import { reportWebVitals } from './utils/vitals' - -reportWebVitals((metric) => { - // Send to analytics - console.log(metric) -}) -``` - -**Benefits:** - -- Track real user performance -- Identify performance issues -- Data-driven optimizations - -#### 10.2 Implement Dark Mode Properly - -**Effort:** Medium | **Priority:** Low | **Impact:** Medium - -```typescript -// app/hooks/useTheme.ts -import { useState, useEffect } from 'react' - -export function useThemeMode() { - const [mode, setMode] = useState<'light' | 'dark'>(() => { - if (typeof window === 'undefined') return 'dark' - const stored = localStorage.getItem('theme-mode') - if (stored) return stored as 'light' | 'dark' - return window.matchMedia('(prefers-color-scheme: dark)').matches - ? 'dark' - : 'light' - }) - - useEffect(() => { - localStorage.setItem('theme-mode', mode) - document.documentElement.classList.toggle('dark', mode === 'dark') - }, [mode]) - - const toggleMode = () => setMode((m) => (m === 'light' ? 'dark' : 'light')) - - return { mode, toggleMode } -} - -// Update theme.tsx -export function useAppTheme() { - const { mode } = useThemeMode() - - return createTheme({ - palette: { - mode, - // ... rest of theme - }, - }) -} -``` - -**Benefits:** - -- Better UX -- Reduced eye strain -- Modern user expectation - -#### 10.3 Add Accessibility Improvements - -**Effort:** Medium | **Priority:** High | **Impact:** High - -**Current issues:** - -- Missing aria labels -- Keyboard navigation not always clear -- Focus management issues - -**Improvements:** - -```typescript -// app/features/atlas/components/GridSquareOverlay.tsx - { - if (e.key === 'Enter' || e.key === ' ') { - handleSelectionClick(gridSquare.uuid) - } - }} - onClick={() => handleSelectionClick(gridSquare.uuid)} -/> -``` - -**Add skip links:** - -```typescript -// app/root.tsx - - - Skip to main content - - {children} - -``` - -**Benefits:** - -- Better accessibility -- Legal compliance -- Wider user reach - -#### 10.4 Add Offline Support - -**Effort:** High | **Priority:** Low | **Impact:** Medium - -```typescript -// vite.config.ts -import { VitePWA } from 'vite-plugin-pwa' - -export default defineConfig({ - plugins: [ - tailwindcss(), - reactRouter(), - tsconfigPaths(), - VitePWA({ - registerType: 'autoUpdate', - workbox: { - globPatterns: ['**/*.{js,css,html,ico,png,svg}'], - runtimeCaching: [ - { - urlPattern: /^https:\/\/api\./, - handler: 'NetworkFirst', - options: { - cacheName: 'api-cache', - expiration: { - maxEntries: 100, - maxAgeSeconds: 60 * 60 * 24, // 1 day - }, - }, - }, - ], - }, - }), - ], -}) -``` - -**Benefits:** - -- Work offline -- Better reliability -- Faster loads on repeat visits - ---- - -## 11. Additional Recommendations - -### 11.1 API Client Improvements - -**Current:** Good setup with Orval - -**Enhancements:** - -1. **Add request/response logging:** - -```typescript -// app/api/mutator.ts -if (import.meta.env.DEV) { - AXIOS_INSTANCE.interceptors.request.use((request) => { - console.log('API Request:', request.method?.toUpperCase(), request.url) - return request - }) -} -``` - -2. **Add retry logic:** - -```typescript -import axiosRetry from 'axios-retry' - -axiosRetry(AXIOS_INSTANCE, { - retries: 3, - retryDelay: axiosRetry.exponentialDelay, - retryCondition: (error) => { - return ( - axiosRetry.isNetworkOrIdempotentRequestError(error) || - error.response?.status === 429 - ) - }, -}) -``` - -3. **Add request cancellation:** - -```typescript -export function useApiQuery(queryKey: QueryKey, queryFn: () => Promise) { - return useQuery({ - queryKey, - queryFn: ({ signal }) => { - // Orval already handles this with CancelToken - return queryFn() - }, - }) -} -``` - -### 11.2 State Management Considerations - -**Current:** TanStack Query for server state only - -**Potential additions:** - -For complex client state, consider: - -- Zustand (lightweight) -- Jotai (atomic state) -- React Context + useReducer (built-in) - -**Not needed yet**, but worth considering if: - -- Sharing state across many components -- Complex state interactions -- Need for state persistence - -### 11.3 Form Management - -**Current:** Manual form handling - -**Recommendation:** Add React Hook Form for complex forms - -```typescript -import { useForm } from 'react-hook-form' -import { zodResolver } from '@hookform/resolvers/zod' - -const schema = z.object({ - name: z.string().min(1), - description: z.string() -}) - -function CreateModelForm() { - const { register, handleSubmit, formState } = useForm({ - resolver: zodResolver(schema) - }) - - return ( -
- - {formState.errors.name && {formState.errors.name.message}} -
- ) -} -``` - -**Benefits:** - -- Better validation -- Less boilerplate -- Type-safe forms - -### 11.4 Documentation Improvements - -**Add:** - -1. Architecture Decision Records (ADRs) -2. Component usage examples in README -3. API integration guide -4. Contributing guidelines -5. Deployment guide - ---- - -## 12. Priority Implementation Roadmap - -### Phase 1: Critical (Week 1-2) - -**Focus: Testing & Quality** - -1. Set up Vitest + Testing Library -2. Add ESLint configuration -3. Write tests for critical paths (hooks, shared components) -4. Set up git hooks (Husky + lint-staged) -5. Add environment variable validation - -**Effort:** ~40 hours -**Impact:** Prevents bugs, improves code quality - -### Phase 2: High Priority (Week 3-4) - -**Focus: Code Organization & Architecture** - -1. Extract custom hooks from route components -2. Create shared UI component abstractions -3. Implement feature-based folder structure -4. Split large route components -5. Remove React namespace from hooks -6. Fix TypeScript `@ts-ignore` issues - -**Effort:** ~60 hours -**Impact:** Better maintainability, easier development - -### Phase 3: Medium Priority (Week 5-6) - -**Focus: Performance & DX** - -1. Implement code splitting -2. Add React.memo for expensive components -3. Optimize TanStack Query configuration -4. Add Storybook -5. Set up bundle analysis -6. Add React Query DevTools - -**Effort:** ~40 hours -**Impact:** Better performance, better DX - -### Phase 4: Enhancement (Week 7-8) - -**Focus: Polish & Modern Features** - -1. Implement dark mode properly -2. Add accessibility improvements -3. Set up E2E testing with Playwright -4. Add Web Vitals monitoring -5. Improve error boundaries -6. Add dependency update automation - -**Effort:** ~30 hours -**Impact:** Better UX, modern standards - ---- - -## 13. Metrics and Success Criteria - -### Code Quality Metrics - -| Metric | Current | Target | Timeline | -| ---------------------- | -------- | ------ | -------- | -| Test Coverage | 0% | 70% | 4 weeks | -| ESLint Errors | Unknown | 0 | 2 weeks | -| TypeScript `any` usage | 4 | 0 | 2 weeks | -| Bundle Size | ~1.3MB | <800KB | 6 weeks | -| Lighthouse Score | Unknown | >90 | 8 weeks | -| Component Count | 6 | 30+ | 6 weeks | -| Hook Count | 0 custom | 15+ | 4 weeks | - -### Developer Experience Metrics - -| Metric | Current | Target | Timeline | -| ------------------------------ | ------- | ------ | -------- | -| Build Time | ~5s | <3s | 4 weeks | -| HMR Speed | Fast | Faster | 4 weeks | -| Time to First Meaningful Paint | Unknown | <1.5s | 6 weeks | -| Storybook Stories | 0 | 25+ | 6 weeks | - ---- - -## 14. Conclusion - -The SmartEM frontend is a solid React application with good foundations. The most critical gaps are: - -1. **Testing** (0% coverage) - Highest priority -2. **Component organization** - Large, monolithic components -3. **Code quality tooling** - Missing ESLint -4. **Type safety** - Some type suppressions - -The project would benefit most from: - -- Immediate focus on testing infrastructure -- Gradual refactoring to extract hooks and split components -- Adding ESLint and git hooks -- Improving component reusability - -Overall, with the recommended improvements, this codebase can evolve from a good application to an excellent, maintainable, and scalable frontend architecture. - ---- - -## Appendix A: Useful Resources - -- [React Router 7 Documentation](https://reactrouter.com/) -- [TanStack Query Best Practices](https://tanstack.com/query/latest/docs/react/guides/best-practices) -- [Testing Library Best Practices](https://testing-library.com/docs/guiding-principles/) -- [Web Vitals](https://web.dev/vitals/) -- [TypeScript Deep Dive](https://basarat.gitbook.io/typescript/) - ---- - -**Generated:** 2025-10-07 -**Analyzer:** Claude Code -**Next Review:** 2025-11-07 From 4c3b1241c13e6f1e8772446980a012f00c5e2ccb Mon Sep 17 00:00:00 2001 From: Val Redchenko Date: Tue, 12 May 2026 12:23:00 +0100 Subject: [PATCH 3/6] config: use real Keycloak realm and client ID Helpdesk UASHD-4189 registered the SmartEM app against the dls realm on identity.diamond.ac.uk (prod) and identity-test.diamond.ac.uk (test) with client ID SmartEM. Replaces the placeholder master / smartem-frontend values from initial scaffolding. .env.example defaults to the test environment for local dev; the in-code fallback in config.ts stays on prod so a production build without env vars set doesn't silently point at the test realm. --- apps/smartem/.env.example | 6 +++--- apps/smartem/src/auth/config.ts | 4 ++-- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/apps/smartem/.env.example b/apps/smartem/.env.example index 75534e0a..50baf546 100644 --- a/apps/smartem/.env.example +++ b/apps/smartem/.env.example @@ -1,4 +1,4 @@ -VITE_KEYCLOAK_URL=https://identity.diamond.ac.uk -VITE_KEYCLOAK_REALM=master -VITE_KEYCLOAK_CLIENT_ID=smartem-frontend +VITE_KEYCLOAK_URL=https://identity-test.diamond.ac.uk +VITE_KEYCLOAK_REALM=dls +VITE_KEYCLOAK_CLIENT_ID=SmartEM VITE_AUTH_ENABLED=false diff --git a/apps/smartem/src/auth/config.ts b/apps/smartem/src/auth/config.ts index 785c52e0..4602747f 100644 --- a/apps/smartem/src/auth/config.ts +++ b/apps/smartem/src/auth/config.ts @@ -9,6 +9,6 @@ export const isAuthEnabled = (): boolean => { export const keycloakConfig: KeycloakServerConfig = { url: import.meta.env.VITE_KEYCLOAK_URL || 'https://identity.diamond.ac.uk', - realm: import.meta.env.VITE_KEYCLOAK_REALM || 'master', - clientId: import.meta.env.VITE_KEYCLOAK_CLIENT_ID || 'smartem-frontend', + realm: import.meta.env.VITE_KEYCLOAK_REALM || 'dls', + clientId: import.meta.env.VITE_KEYCLOAK_CLIENT_ID || 'SmartEM', } From 557edf67cae59896605fdaf13683a8ac5716bff9 Mon Sep 17 00:00:00 2001 From: Val Redchenko Date: Wed, 13 May 2026 16:43:39 +0100 Subject: [PATCH 4/6] fix: use hidden iframe for silent SSO and fix init failure no-op login Add silent-check-sso.html so keycloak-js performs the session check in a hidden iframe instead of a top-level redirect, preventing the redirect storm caused by React StrictMode double-mounting the init effect. Use the live keycloak instance in the init error path so login() still triggers a full-page redirect even when the silent-SSO handshake fails. --- apps/smartem/public/silent-check-sso.html | 4 ++++ apps/smartem/src/auth/AuthProvider.tsx | 12 ++++++++++-- 2 files changed, 14 insertions(+), 2 deletions(-) create mode 100644 apps/smartem/public/silent-check-sso.html diff --git a/apps/smartem/public/silent-check-sso.html b/apps/smartem/public/silent-check-sso.html new file mode 100644 index 00000000..66da34c6 --- /dev/null +++ b/apps/smartem/public/silent-check-sso.html @@ -0,0 +1,4 @@ + + diff --git a/apps/smartem/src/auth/AuthProvider.tsx b/apps/smartem/src/auth/AuthProvider.tsx index 2218ce0b..94e0213b 100644 --- a/apps/smartem/src/auth/AuthProvider.tsx +++ b/apps/smartem/src/auth/AuthProvider.tsx @@ -107,11 +107,19 @@ export const AuthProvider = ({ children, onTokenChange }: AuthProviderProps) => } keycloak - .init({ onLoad: 'check-sso' }) + .init({ + onLoad: 'check-sso', + silentCheckSsoRedirectUri: `${window.location.origin}/silent-check-sso.html`, + pkceMethod: 'S256', + }) .then(() => setAuth(buildAuth(keycloak))) .catch((err) => { console.error('Keycloak init failed:', err) - setAuth({ ...defaultAuth, initialised: true, error: 'Failed to connect to Keycloak' }) + setAuth({ + ...buildAuth(keycloak), + initialised: true, + error: 'Failed to connect to Keycloak', + }) }) return () => { From 6b02a4a477ee6c180cedbfe9053c0c4f868cbdae Mon Sep 17 00:00:00 2001 From: Val Redchenko Date: Thu, 14 May 2026 11:35:04 +0100 Subject: [PATCH 5/6] fix(auth): disable iframe SSO checks to break redirect loop MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adding `silentCheckSsoRedirectUri` covered the initial check-sso, but keycloak-js still defaults `checkLoginIframe: true` - a hidden iframe that polls Keycloak every 5 seconds for session state. In modern browsers third- party cookies are blocked, so that iframe can never read Keycloak's session cookie; it reports "session changed" each tick, which keycloak-js translates into a top-level redirect. Result: the SPA bounces between `/` and `/#error=login_required` every five seconds. The companion option `silentCheckSsoFallback` (default true) made the same thing happen for the initial check: if the silent iframe couldn't see the session cookie, keycloak-js fell back to a top-level redirect. Combined with React StrictMode's double-mount of the AuthProvider effect, the fallback produced an immediate two-redirect storm even before the polling iframe kicked in. Disable both: - `silentCheckSsoFallback: false` keeps the initial check confined to the iframe; if that fails, the user just sees the sign-in button and has to click it (rather than getting redirect-stormed). - `checkLoginIframe: false` stops the post-init polling entirely. We lose cross-tab logout detection from this path, but the token-refresh timer already detects an invalidated session on its next tick. Verified end-to-end against the local Keycloak mock: 0 spurious redirects during settle, click → KC login → return → authenticated state with the account menu visible. --- apps/smartem/src/auth/AuthProvider.tsx | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/apps/smartem/src/auth/AuthProvider.tsx b/apps/smartem/src/auth/AuthProvider.tsx index 94e0213b..513a21ab 100644 --- a/apps/smartem/src/auth/AuthProvider.tsx +++ b/apps/smartem/src/auth/AuthProvider.tsx @@ -111,6 +111,16 @@ export const AuthProvider = ({ children, onTokenChange }: AuthProviderProps) => onLoad: 'check-sso', silentCheckSsoRedirectUri: `${window.location.origin}/silent-check-sso.html`, pkceMethod: 'S256', + // Don't fall back to a top-level redirect when the silent iframe can't + // read Keycloak's session cookie (third-party cookies are blocked by + // default in modern browsers). The fallback turns "not logged in" into + // a full page navigation, which in dev's StrictMode double-mount or + // alongside the polling iframe below produces a redirect loop. + silentCheckSsoFallback: false, + // The default post-init polling iframe (5s interval) hits the same + // third-party-cookie wall and keeps firing top-level logins. Disable + // it; cross-tab logout is handled via the token-refresh path. + checkLoginIframe: false, }) .then(() => setAuth(buildAuth(keycloak))) .catch((err) => { From 2ee26bd965fe0a996b8f1963d24dedd3ca741c37 Mon Sep 17 00:00:00 2001 From: Val Redchenko Date: Thu, 14 May 2026 11:41:00 +0100 Subject: [PATCH 6/6] feat(auth): block app contents until user is authenticated `AuthGate` was previously a pure pass-through wrapper around `AuthProvider` - it set up the auth context but always rendered children, regardless of auth state. Unauthenticated users could see the entire dashboard with just a "Sign in" button in the header. Add an `AuthBoundary` inner component that gates rendering on the resolved auth state: - `!auth.initialised`: render nothing. Keycloak init is in flight; better to show a brief blank than flash either the sign-in screen or the app. - `!auth.authenticated`: render `SignInScreen` - a centred MUI Box with the app title, a one-line prompt, and a contained "Sign in" button that calls `auth.login()`. If `auth.error` is populated (init failed, refresh failed, etc.) it is surfaced beneath the button in `error.main` colour. - authenticated: render children as before. When `VITE_AUTH_ENABLED=false` the gate is a no-op (the early return at the top of `AuthGate` is unchanged), so mock-mode UI work is unaffected. Verified end-to-end against the local Keycloak mock: pre-login the only visible content is the sign-in screen; clicking through to Keycloak and back unlocks the dashboard. --- apps/smartem/src/auth/AuthGate.tsx | 54 ++++++++++++++++++++++++++++-- 1 file changed, 52 insertions(+), 2 deletions(-) diff --git a/apps/smartem/src/auth/AuthGate.tsx b/apps/smartem/src/auth/AuthGate.tsx index fe5089ae..0ceab228 100644 --- a/apps/smartem/src/auth/AuthGate.tsx +++ b/apps/smartem/src/auth/AuthGate.tsx @@ -1,6 +1,8 @@ +import { Login } from '@mui/icons-material' +import { Box, Button, Typography } from '@mui/material' import { setAuthToken } from '@smartem/api' import type { PropsWithChildren } from 'react' -import { AuthProvider } from './AuthProvider' +import { AuthProvider, useAuth } from './AuthProvider' import { isAuthEnabled } from './config' export const AuthGate = ({ children }: PropsWithChildren) => { @@ -8,5 +10,53 @@ export const AuthGate = ({ children }: PropsWithChildren) => { return <>{children} } - return {children} + return ( + + {children} + + ) +} + +function AuthBoundary({ children }: PropsWithChildren) { + const auth = useAuth() + + // Keycloak init is in flight - render nothing rather than flash either the + // sign-in screen or the app contents. + if (!auth.initialised) return null + + if (!auth.authenticated) return + + return <>{children} +} + +function SignInScreen({ onSignIn, error }: { onSignIn: () => void; error?: string }) { + return ( + + + SmartEM + + + Sign in with your Diamond account to continue. + + + {error && ( + + {error} + + )} + + ) }