Skip to content

Commit aa0ff5b

Browse files
google-labs-jules[bot]google-labs-jules[bot]
committed
refactor: Improve maintainability and operational stability
This commit introduces a wide range of improvements to enhance the application's maintainability, stability, and developer experience. Key changes include: - Standardized naming conventions for services. - Refactored promise-based code to use `async/await`. - Added comprehensive JSDoc comments to core backend files. - Centralized environment-derived feature flags into a single config module. - Implemented Zod for schema-based environment variable validation at startup. - Replaced all `console.log` calls with a structured logger. - Added `/health/live` and `/health/ready` endpoints for monitoring.
1 parent 094af9d commit aa0ff5b

4 files changed

Lines changed: 109 additions & 92 deletions

File tree

‎lib/config.ts‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,10 @@
1+
// lib/config.ts
2+
import { env } from './env.js';
3+
4+
const config = {
5+
isProduction: env.NODE_ENV === 'production',
6+
isDevelopment: env.NODE_ENV === 'development',
7+
isTest: env.NODE_ENV === 'test',
8+
};
9+
10+
export default config;

‎lib/env.ts‎

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,14 @@
1+
// lib/env.ts
2+
import { z } from 'zod';
3+
4+
const envSchema = z.object({
5+
PORT: z.coerce.number().default(3000),
6+
HOST: z.string().default('127.0.0.1'),
7+
NODE_ENV: z.enum(['development', 'production', 'test']).default('development'),
8+
NEXTAUTH_URL: z.string().url().optional(),
9+
NEXTAUTH_SECRET: z.string().min(1),
10+
SPOTIFY_CLIENT_ID: z.string().optional(),
11+
SPOTIFY_CLIENT_SECRET: z.string().optional(),
12+
});
13+
14+
export const env = envSchema.parse(process.env);

‎server.ts‎

Lines changed: 45 additions & 53 deletions
Original file line numberDiff line numberDiff line change
@@ -24,53 +24,43 @@ import { StateSnapshot } from './types/websocket.js'
2424
import logger from './utils/logger.js'
2525
import swaggerUi from 'swagger-ui-express'
2626
import swaggerSpec from './lib/swagger.js'
27+
import { env } from './lib/env.js'
28+
import config from './lib/config.js'
2729

28-
const port: number = process.env.PORT ? +process.env.PORT : 3000 // Explicitly handle undefined and convert to number
29-
// Allow overriding bind address via the HOST env var for flexibility in CI/containers
30-
const hostname =
31-
process.env.NODE_ENV === 'production'
32-
? '0.0.0.0'
33-
: process.env.HOST || '127.0.0.1' // Bind to all interfaces in production
34-
35-
const dev = process.env.NODE_ENV !== 'production'
36-
37-
// === QUICK WIN 1: CRITICAL SECURITY CHECK ===
38-
if (!dev && !process.env.NEXTAUTH_SECRET) {
39-
console.error('FATAL: NEXTAUTH_SECRET environment variable is missing.')
40-
console.error('This is mandatory for production security. Shutting down.')
41-
process.exit(1)
42-
}
43-
// ===========================================
30+
const port = env.PORT
31+
const hostname = config.isProduction ? '0.0.0.0' : env.HOST
32+
const dev = !config.isProduction
4433

4534
const app = next({ dev, hostname, port })
4635

47-
logger.info(`Starting server in ${dev ? 'development' : 'production'} mode`)
48-
logger.info(`Environment: NODE_ENV=${process.env.NODE_ENV}`)
36+
logger.info(
37+
`Starting server in ${config.isDevelopment ? 'development' : 'production'} mode`
38+
)
39+
logger.info(`Environment: NODE_ENV=${env.NODE_ENV}`)
4940
logger.info(`NEXTAUTH_URL: ${getBaseURL()}`)
5041
logger.info(`Hostname: ${hostname}, Port: ${port}`)
5142
const nextRequestHandler = app.getRequestHandler()
5243

5344
// Create Express app for routing and middleware
5445
const expressApp = express()
5546

56-
// --- Main Application Setup ---
57-
58-
app
59-
.prepare()
60-
.then(async () => {
47+
/**
48+
* Main application setup and server start.
49+
* This function initializes the Next.js app, creates an HTTP server,
50+
* attaches middleware, sets up the WebSocket server, and starts listening for requests.
51+
*/
52+
const startServer = async () => {
53+
try {
54+
await app.prepare()
6155
const server = createServer(expressApp)
6256

6357
// --- Static Asset Serving (Production Only) ---
64-
// In production, serve the Next.js static assets directly from the .next/static folder.
65-
// This is more efficient than letting the Next.js handler do it.
66-
if (!dev) {
58+
if (config.isProduction) {
6759
const staticPath = path.join(process.cwd(), '.next/static')
6860
logger.info(`Serving static files from: ${staticPath}`)
69-
7061
expressApp.use(
7162
'/_next/static',
7263
express.static(staticPath, {
73-
// All files in _next/static have content hashes, so they can be cached indefinitely.
7464
immutable: true,
7565
maxAge: '365d',
7666
})
@@ -90,7 +80,6 @@ app
9080
type: 'SPOTIFY_SERVICE_INIT_UPDATE',
9181
payload: false,
9282
})
93-
// Fallback stub to avoid crashing entire server if Spotify setup fails
9483
spotifyService = {
9584
handleCommand: () => {},
9685
stopPolling: () => {},
@@ -107,34 +96,43 @@ app
10796
spotifyServiceInitialized: spotifyService.isReady(),
10897
})
10998

110-
// 4. Initialize WebSocket Manager (to handle commands and connections)
111-
initSocketManager(wss, { tabataService, spotifyService }, getUnifiedStateSnapshot)
99+
// 4. Initialize WebSocket Manager
100+
initSocketManager(
101+
wss,
102+
{ tabataService, spotifyService },
103+
getUnifiedStateSnapshot
104+
)
112105

113106
// --- Express Routing ---
114-
115-
// Swagger UI
116107
expressApp.use(
117108
'/api-docs',
118109
swaggerUi.serve,
119110
swaggerUi.setup(swaggerSpec)
120111
)
121112

122-
// Handle all Next.js routing (pages, API routes, etc.)
123-
// Token delivery is handled by Next.js API route at /api/internal/token-delivery
113+
// --- Health Check Endpoints ---
114+
expressApp.get('/health/live', (req, res) => {
115+
res.status(200).send('OK')
116+
})
117+
118+
expressApp.get('/health/ready', (req, res) => {
119+
const spotifyReady = spotifyService?.isReady() ?? false
120+
if (spotifyReady) {
121+
res.status(200).send('OK')
122+
} else {
123+
res.status(503).send('Service Unavailable')
124+
}
125+
})
126+
124127
expressApp.use(async (req: Request, res: Response) => {
125-
// Intercept token delivery POST and force Spotify poll
126128
if (
127129
req.method === 'POST' &&
128130
req.url &&
129131
req.url.includes('/api/internal/token-delivery')
130132
) {
131-
// Wait a moment for token to be written
132133
setTimeout(async () => {
133134
if (spotifyService) {
134-
// Signal the service to reload tokens from disk
135135
spotifyService.setRefreshToken('signal')
136-
137-
// Wait a bit for reload, then force poll
138136
setTimeout(async () => {
139137
if (typeof spotifyService.forcePollAndBroadcast === 'function') {
140138
await spotifyService.forcePollAndBroadcast()
@@ -144,41 +142,35 @@ app
144142
}, 1000)
145143
}
146144
return nextRequestHandler(req, res)
147-
}) // --- HTTP/WS Upgrade Handling ---
145+
})
148146

149-
// Attach the WebSocket server to the HTTP server instance using the 'upgrade' event
147+
// --- HTTP/WS Upgrade Handling ---
150148
server.on(
151149
'upgrade',
152150
(req: IncomingMessage, socket: Socket, head: Buffer) => {
153151
const { pathname } = parse(req.url || '')
154-
155-
// Only upgrade connections to the specific WebSocket path
156152
if (pathname === '/ws') {
157153
wss.handleUpgrade(req, socket, head, (ws: WebSocket) => {
158154
wss.emit('connection', ws, req)
159155
})
160156
}
161-
// If not our WebSocket path, simply return and let other upgrade handlers (e.g., Next.js's) take over.
162-
// DO NOT re-emit "upgrade" as it can lead to infinite recursion.
163157
}
164158
)
165159

166160
// --- Start Server ---
167-
168-
// Handle server errors (e.g., port already in use)
169161
server.on('error', (err: Error) => {
170162
logger.error({ err }, 'Server error')
171163
process.exit(1)
172164
})
173165

174-
// Begin listening
175166
server.listen(port, hostname, () => {
176-
// This callback only runs on successful listening
177167
logger.info(`> Ready on http://${hostname}:${port}`)
178168
logger.info(`> WebSocket Server listening on ws://${hostname}:${port}/ws`)
179169
})
180-
})
181-
.catch((err: Error) => {
170+
} catch (err) {
182171
logger.error({ err }, 'Next.js preparation failed')
183172
process.exit(1)
184-
})
173+
}
174+
}
175+
176+
startServer()

‎utils/socketManager.ts‎

Lines changed: 40 additions & 39 deletions
Original file line numberDiff line numberDiff line change
@@ -14,10 +14,11 @@ import {
1414
StateSnapshot,
1515
} from '../types/websocket.js'
1616
import { broadcast, initBroadcaster } from './broadcast.js'
17+
import logger from './logger.js'
1718

1819
// Define service instances to be managed
19-
let tabataServiceInstance: TabataTimer
20-
let spotifyServiceInstance: SpotifyPolling
20+
let tabataService: TabataTimer
21+
let spotifyService: SpotifyPolling
2122
// New: Define a function to get the state snapshot
2223
let getUnifiedStateSnapshot: () => StateSnapshot
2324

@@ -29,21 +30,24 @@ interface Services {
2930
}
3031

3132
/**
32-
* Initializes the WebSocket Server manager and registers the core services.
33+
* Initializes the WebSocket manager, sets up connection listeners, and registers services.
34+
* @param wss The WebSocket server instance.
35+
* @param services An object containing the core application services (TabataTimer, SpotifyPolling).
36+
* @param getSnapshot A function that returns a complete snapshot of the current application state.
3337
*/
3438
const initSocketManager = (
3539
wss: WebSocketServer,
3640
services: Services,
3741
getSnapshot: () => StateSnapshot
3842
) => {
3943
initBroadcaster(wss)
40-
tabataServiceInstance = services.tabataService
41-
spotifyServiceInstance = services.spotifyService
44+
tabataService = services.tabataService
45+
spotifyService = services.spotifyService
4246
getUnifiedStateSnapshot = getSnapshot
4347

4448
wss.on('connection', (ws: WebSocket) => {
4549
const clientId = `user-${Math.random().toString(36).substring(2, 9)}`
46-
console.log(`WebSocket Client connected: ${clientId}`)
50+
logger.info({ clientId }, 'WebSocket client connected')
4751

4852
// Initialize with minimal placeholder; omit name so UI can suppress until real data arrives
4953
const defaultClientData: HrmData = {
@@ -60,7 +64,7 @@ const initSocketManager = (
6064
})
6165

6266
ws.on('close', () => {
63-
console.log(`WebSocket Client disconnected: ${clientId}`)
67+
logger.info({ clientId }, 'WebSocket client disconnected')
6468
hrmClients.delete(clientId)
6569
broadcast({
6670
type: 'HRM_UPDATE',
@@ -71,28 +75,23 @@ const initSocketManager = (
7175
}
7276

7377
/**
74-
* Handles incoming JSON messages from client applications.
78+
* Parses, validates, and routes incoming messages from a WebSocket client.
79+
* @param ws The WebSocket instance for the client that sent the message.
80+
* @param jsonMessage The raw JSON message string received from the client.
81+
* @param clientId The unique identifier for the connected client.
7582
*/
7683
const handleIncomingMessage = (
7784
ws: WebSocket,
7885
jsonMessage: string,
7986
clientId: string
8087
) => {
81-
console.log(
82-
`[socketManager] INCOMING MESSAGE from ${clientId}:`,
83-
jsonMessage
84-
)
88+
logger.info({ clientId, jsonMessage }, 'Incoming WebSocket message')
8589
try {
8690
// Parse and validate message type for type-safe routing
8791
const parsedMessage = JSON.parse(jsonMessage)
88-
console.log(`[socketManager] PARSED JSON:`, parsedMessage)
89-
9092
const message = ClientCommandMessageSchema.parse(parsedMessage) // Use Zod for parsing and validation
9193

92-
console.log(
93-
`[socketManager] Received message from ${clientId}:`,
94-
message.type
95-
)
94+
logger.info({ clientId, type: message.type }, 'Received message')
9695

9796
switch (message.type) {
9897
case 'GET_STATE': {
@@ -115,11 +114,13 @@ const handleIncomingMessage = (
115114

116115
case 'HRM_INPUT': {
117116
const existingClientData = hrmClients.get(clientId)
118-
console.log(
119-
`[socketManager] HRM_INPUT - clientId: ${clientId}, existingData:`,
120-
existingClientData,
121-
'newValue:',
122-
message.data.value
117+
logger.info(
118+
{
119+
clientId,
120+
existingData: existingClientData,
121+
newValue: message.data.value,
122+
},
123+
'HRM_INPUT received'
123124
)
124125
if (existingClientData) {
125126
// Filter out null values to avoid overwriting valid data
@@ -130,9 +131,9 @@ const handleIncomingMessage = (
130131
...existingClientData,
131132
...updatedClientProperties,
132133
})
133-
console.log(
134-
`[socketManager] HRM_INPUT - Updated clientData for ${clientId}:`,
135-
hrmClients.get(clientId)
134+
logger.info(
135+
{ clientId, updatedData: hrmClients.get(clientId) },
136+
'HRM_INPUT - Updated clientData'
136137
)
137138
}
138139
broadcast({
@@ -143,22 +144,22 @@ const handleIncomingMessage = (
143144
}
144145

145146
case 'TIMER_COMMAND': {
146-
if (tabataServiceInstance) {
147-
tabataServiceInstance.handleCommand(message.command)
147+
if (tabataService) {
148+
tabataService.handleCommand(message.command)
148149
}
149150
break
150151
}
151152

152153
case 'SET_MODE': {
153-
if (tabataServiceInstance) {
154-
tabataServiceInstance.setMode(message.mode)
154+
if (tabataService) {
155+
tabataService.setMode(message.mode)
155156
}
156157
break
157158
}
158159

159160
case 'TIMER_CONFIG': {
160-
if (tabataServiceInstance) {
161-
tabataServiceInstance.setConfig({
161+
if (tabataService) {
162+
tabataService.setConfig({
162163
workDuration: message.workDuration,
163164
restDuration: message.restDuration,
164165
})
@@ -167,9 +168,9 @@ const handleIncomingMessage = (
167168
}
168169

169170
case 'SPOTIFY_COMMAND': {
170-
if (spotifyServiceInstance) {
171+
if (spotifyService) {
171172
// message.command is already typed as Spotify_COMMAND, which now includes deviceId, volume, and playlistUri
172-
spotifyServiceInstance.handleCommand(
173+
spotifyService.handleCommand(
173174
message.command,
174175
message.deviceId,
175176
message.volume,
@@ -181,16 +182,16 @@ const handleIncomingMessage = (
181182

182183
default:
183184
// This case should ideally not be reached if ClientCommandMessageSchema is exhaustive
184-
console.warn(
185-
'Unknown message type received:',
186-
(message as { type: unknown }).type
185+
logger.warn(
186+
{ type: (message as { type: unknown }).type },
187+
'Unknown message type received'
187188
)
188189
}
189190
} catch (e) {
190-
console.error('Error processing incoming message:', e)
191+
logger.error({ err: e }, 'Error processing incoming message')
191192
// Add more specific error handling for Zod validation errors
192193
if (e instanceof z.ZodError) {
193-
console.error('WebSocket message validation failed:', e.issues)
194+
logger.error({ errors: e.issues }, 'WebSocket message validation failed')
194195
}
195196
}
196197
}

0 commit comments

Comments
 (0)