fix: apply code review fixes — double layout, SSE leaks, interval leak, conflict values

- App.tsx: remove double-wrapped MainLayout from /notifications route
- NotificationBell.tsx: decouple SSE from popover open state using ref
- useNotifications.ts: fix interval leak in onerror via ref in cleanup
- notifications.controller.ts: unsubscribe on req.on('close') for SSE Subject
- schedules.controller.ts: use existing schedule values in conflict catch
This commit is contained in:
2026-07-05 23:31:21 +08:00
parent 7020e86809
commit 9e6c136ef0
5 changed files with 19 additions and 17 deletions

View File

@@ -232,13 +232,7 @@ const App: React.FC = () => {
} }
/> />
<Route path="/notifications" element={ <Route path="notifications" element={<NotificationsPage />} />
<PrivateRoute>
<MainLayout />
</PrivateRoute>
}>
<Route index element={<NotificationsPage />} />
</Route>
</Route> </Route>
</Routes> </Routes>
</BrowserRouter> </BrowserRouter>

View File

@@ -1,4 +1,4 @@
import React, { useState, useEffect } from 'react'; import React, { useState, useEffect, useRef } from 'react';
import { Badge, Popover, Button, List, Typography, Empty } from 'antd'; import { Badge, Popover, Button, List, Typography, Empty } from 'antd';
import { BellOutlined } from '@ant-design/icons'; import { BellOutlined } from '@ant-design/icons';
import { useNavigate } from 'react-router-dom'; import { useNavigate } from 'react-router-dom';
@@ -56,7 +56,10 @@ const NotificationBell: React.FC = () => {
setUnreadCount(data.count); setUnreadCount(data.count);
} catch { /* ignore */ } } catch { /* ignore */ }
}; };
const openRef = useRef(open);
openRef.current = open;
// SSE connection — decoupled from popover open state
useEffect(() => { useEffect(() => {
fetchUnread(); fetchUnread();
const token = localStorage.getItem('token'); const token = localStorage.getItem('token');
@@ -66,16 +69,15 @@ const NotificationBell: React.FC = () => {
try { try {
JSON.parse(event.data); JSON.parse(event.data);
setUnreadCount((c) => c + 1); setUnreadCount((c) => c + 1);
if (open) fetchNotifications(); if (openRef.current) fetchNotifications();
} catch { /* ignore */ } } catch { /* ignore */ }
}; };
es.onerror = () => { es.onerror = () => {
es.close(); es.close();
const interval = setInterval(fetchUnread, 60_000); setInterval(fetchUnread, 60_000);
return () => clearInterval(interval);
}; };
return () => es.close(); return () => es.close();
}, [open]); }, []);
const handleOpen = (visible: boolean) => { const handleOpen = (visible: boolean) => {
setOpen(visible); setOpen(visible);

View File

@@ -1,4 +1,4 @@
import { useState, useEffect, useCallback } from 'react'; import { useState, useEffect, useCallback, useRef } from 'react';
import api from '../api'; import api from '../api';
interface Notification { interface Notification {
@@ -41,16 +41,18 @@ export function useNotifications() {
} }
}; };
const pollRef = { current: undefined as number | undefined };
es.onerror = () => { es.onerror = () => {
es.close(); es.close();
const interval = setInterval(() => { pollRef.current = setInterval(() => {
fetchUnreadCount(); fetchUnreadCount();
}, 60_000); }, 60_000);
return () => clearInterval(interval);
}; };
return () => { return () => {
es.close(); es.close();
if (pollRef.current !== undefined) clearInterval(pollRef.current);
}; };
}, [fetchUnreadCount]); }, [fetchUnreadCount]);

View File

@@ -48,6 +48,9 @@ export class NotificationsController {
@Sse('stream') @Sse('stream')
stream(@Req() req: AuthenticatedRequest): Observable<MessageEvent> { stream(@Req() req: AuthenticatedRequest): Observable<MessageEvent> {
const userId = req.user.id; const userId = req.user.id;
req.on('close', () => {
this.service.unsubscribe(userId);
});
return this.service.subscribe(userId).pipe( return this.service.subscribe(userId).pipe(
map((notification) => ({ map((notification) => ({
data: JSON.stringify({ data: JSON.stringify({

View File

@@ -108,6 +108,7 @@ export class SchedulesController {
@Request() req: { user?: { id: number; username: string }; headers?: Record<string, string> }, @Request() req: { user?: { id: number; username: string }; headers?: Record<string, string> },
) { ) {
const { ipAddress, userAgent } = extractRequestInfo(req); const { ipAddress, userAgent } = extractRequestInfo(req);
const existing = await this.service.findOne(+id);
try { try {
const result = await this.service.update(+id, dto); const result = await this.service.update(+id, dto);
await this.logService.log({ await this.logService.log({
@@ -126,7 +127,7 @@ export class SchedulesController {
if (error instanceof ConflictException) { if (error instanceof ConflictException) {
try { try {
const conflicts = await this.service.checkConflict( const conflicts = await this.service.checkConflict(
dto.classroomId ?? 0, dto.weekDay ?? 0, dto.startTime ?? '', dto.endTime ?? '', dto.startDate ?? '', dto.endDate ?? '', existing.classroomId, existing.weekDay, existing.startTime, existing.endTime, existing.startDate, existing.endDate,
); );
const teacherIds = [...new Set(conflicts.map(c => c.teacherId).filter(Boolean))]; const teacherIds = [...new Set(conflicts.map(c => c.teacherId).filter(Boolean))];
if (teacherIds.length > 0) { if (teacherIds.length > 0) {
@@ -134,7 +135,7 @@ export class SchedulesController {
recipientIds: teacherIds, recipientIds: teacherIds,
type: NotificationType.SCHEDULE_CONFLICT, type: NotificationType.SCHEDULE_CONFLICT,
title: '排课冲突', title: '排课冲突',
content: `教室${dto.classroomId}${dto.weekDay} ${dto.startTime}-${dto.endTime} (更新) 与已有排课冲突`, content: `教室${existing.classroomId}${existing.weekDay} ${existing.startTime}-${existing.endTime} (更新) 与已有排课冲突`,
}); });
} }
} catch {} } catch {}