diff --git a/src/context/AppContext.test.tsx b/src/context/AppContext.test.tsx index 84ba303f..cce31e2e 100644 --- a/src/context/AppContext.test.tsx +++ b/src/context/AppContext.test.tsx @@ -1,11 +1,12 @@ -import React, { ReactNode } from 'react'; +import React, { ReactNode, useLayoutEffect, useRef } from 'react'; import { renderHook, act } from '@testing-library/react'; import { describe, it, expect, vi, beforeEach } from 'vitest'; import { renderWithRouter } from '../test-utils/renderWithProviders'; -import { AppProvider, useApp, useRoster, useSettings } from './AppContext'; +import { AppProvider, useApp, useRoster, useSettings, useAuthoring } from './AppContext'; import * as storage from '../store/storage'; import { storageSync } from '../services/database'; import type { Rubric, GradeScale } from '../types'; +import { DEFAULT_FORMAT } from '../types'; // vi.hoisted so this spy can be created before vi.mock('../hooks/useToast', ...) runs // (vi.mock factories are hoisted above regular imports/consts) and still be referenced @@ -688,4 +689,165 @@ describe('AppContext', () => { expect(rosterRenders.length).toBeGreaterThan(1); // roster consumer re-rendered expect(settingsRenders.length).toBe(1); // settings consumer did NOT }); + + it('roster consumers do not re-render when authoring data (rubrics) changes', () => { + // Cross-domain action stability: createStudentRubric / createGroupStudentRubrics read + // rubrics via currentStateRef, so their identity (and thus rosterValue) must not change + // when the authoring rubrics collection changes. + const rosterRenders: number[] = []; + let triggerAddRubric: (() => void) | null = null; + + function RosterProbe() { + const { students } = useRoster(); + rosterRenders.push(students.length); + return
{students.length}
; + } + + function TriggerProbe() { + const { addRubric } = useAuthoring(); + triggerAddRubric = () => + addRubric({ + name: 'New Rubric', + subject: '', + description: '', + criteria: [], + gradeScaleId: 'default-scale', + format: DEFAULT_FORMAT, + attachmentIds: [], + totalMaxPoints: 100, + scoringMode: 'weighted-percentage', + }); + return null; + } + + renderWithRouter( + <> + + + , + { withAppProvider: true } + ); + + expect(rosterRenders.length).toBe(1); + + act(() => { + triggerAddRubric!(); + }); + + expect(rosterRenders.length).toBe(1); // roster consumer did NOT re-render + }); + + it('settings consumers do not re-render when authoring data (grade scales) changes', () => { + // getActiveGradeScale reads gradeScales via currentStateRef, so its identity (and thus + // settingsValue) must not change when the authoring grade-scales collection changes. + const settingsRenders: string[] = []; + let triggerAddGradeScale: (() => void) | null = null; + + function SettingsProbe() { + const { settings } = useSettings(); + settingsRenders.push(settings.language); + return
{settings.language}
; + } + + function TriggerProbe() { + const { addGradeScale } = useAuthoring(); + triggerAddGradeScale = () => addGradeScale({ name: 'New Scale', type: 'points', ranges: [] }); + return null; + } + + renderWithRouter( + <> + + + , + { withAppProvider: true } + ); + + expect(settingsRenders.length).toBe(1); + + act(() => { + triggerAddGradeScale!(); + }); + + expect(settingsRenders.length).toBe(1); // settings consumer did NOT re-render + }); + + it('actions invoked from a layout effect see the latest rubric state', () => { + // Regression for currentStateRef sync timing: the provider must refresh the ref in the + // layout phase (parent layout effects fire before descendants'), so an action called from + // a descendant's useLayoutEffect — e.g. pre-filling a just-created rubric — reads the + // fresh snapshot instead of the previous render's. + const entryCounts: number[] = []; + const rubricIdRef: { current: string } = { current: '' }; + + function LayoutEffectProbe() { + const { createStudentRubric, createGroupStudentRubrics } = useRoster(); + const { addRubric } = useAuthoring(); + const runRef = useRef(0); + + useLayoutEffect(() => { + runRef.current += 1; + if (runRef.current === 1) { + const rubric = addRubric({ + name: 'Layout Rubric', + subject: '', + description: '', + gradeScaleId: 'default-scale', + format: DEFAULT_FORMAT, + attachmentIds: [], + totalMaxPoints: 30, + scoringMode: 'weighted-percentage', + criteria: [ + { id: 'c1', title: 'C1', description: '', weight: 34, levels: [] }, + { id: 'c2', title: 'C2', description: '', weight: 33, levels: [] }, + { id: 'c3', title: 'C3', description: '', weight: 33, levels: [] }, + ], + }); + rubricIdRef.current = rubric.id; + } else if (runRef.current === 2) { + const single = createStudentRubric(rubricIdRef.current, 'student-1'); + entryCounts.push(single.entries.length); + const group = createGroupStudentRubrics(rubricIdRef.current, ['student-1', 'student-2']); + entryCounts.push(group[0].entries.length); + } + }); + + return
; + } + + renderWithRouter(, { withAppProvider: true }); + act(() => {}); // flush the re-render scheduled by the first layout effect + + // Each created StudentRubric must mirror the 3 criteria of the rubric added in run 1. + expect(entryCounts).toEqual([3, 3]); + }); + + it('getActiveGradeScale invoked from a layout effect sees the latest grade-scale state', () => { + const observedNames: string[] = []; + + function LayoutEffectScaleProbe() { + const { getActiveGradeScale, updateSettings } = useSettings(); + const { addGradeScale } = useAuthoring(); + const runRef = useRef(0); + + useLayoutEffect(() => { + runRef.current += 1; + if (runRef.current === 1) { + const scale = addGradeScale({ name: 'Layout Scale', type: 'points', ranges: [] }); + updateSettings({ defaultGradeScaleId: scale.id }); + } else if (runRef.current === 2) { + observedNames.push(getActiveGradeScale().name); + } + }); + + return
; + } + + renderWithRouter(, { withAppProvider: true }); + act(() => {}); // flush the re-render scheduled by the first layout effect + + // With the fix the ref is refreshed in the layout phase, so the action sees the scale + // added in run 1; with a passive-effect sync it would still resolve the old default. + expect(observedNames).toEqual(['Layout Scale']); + }); }); diff --git a/src/context/AppContext.tsx b/src/context/AppContext.tsx index 989f4633..6aea77a0 100644 --- a/src/context/AppContext.tsx +++ b/src/context/AppContext.tsx @@ -1419,7 +1419,14 @@ async function flushToLocalStorage(merged: StoreData, changedKeys?: Set { - currentStateRef.current = state; - }, [state]); - // 'checking' while we detect session; 'show' = show landing; 'hide' = in app const [landingState, setLandingState] = useState<'checking' | 'show' | 'hide'>('checking'); // Ref so the OTP handler ([] deps effect) can read current state without @@ -1918,6 +1920,11 @@ export function AppProvider({ children }: { children: ReactNode }) { [] ); + // These actions read state via currentStateRef instead of closing over a slice so their + // identity stays stable: a closure over e.g. `state.rubrics` would re-identify the action + // (and thus re-render every roster consumer) whenever rubrics change, even though rubrics + // belong to the authoring domain. Read-only helpers built from fresh state keep each domain + // value changing only when the slices it exposes change. const createStudentRubric = useCallback((rubricId: string, studentId: string): StudentRubric => { const rubric = currentStateRef.current.rubrics.find((r) => r.id === rubricId); const entries: ScoreEntry[] = (rubric?.criteria ?? []).map((c) => ({ @@ -1943,41 +1950,38 @@ export function AppProvider({ children }: { children: ReactNode }) { * Grading any member through the normal single-student flow then duplicates its scores to the * rest of the group via the SAVE_STUDENT_RUBRIC reducer case — no separate group grading UI. */ - const createGroupStudentRubrics = useCallback( - (rubricId: string, studentIds: string[]): StudentRubric[] => { - const rubric = currentStateRef.current.rubrics.find((r) => r.id === rubricId); - const entries: ScoreEntry[] = (rubric?.criteria ?? []).map((c) => ({ - criterionId: c.id, - levelId: null, - comment: '', - checkedSubItems: [], - })); - const groupId = nanoid(); - const srs = studentIds.map((studentId): StudentRubric => { - const existing = state.studentRubrics.find( - (sr) => sr.rubricId === rubricId && sr.studentId === studentId && !sr.isPeerReview - ); - return { - ...existing, - id: existing?.id ?? nanoid(), - rubricId, - studentId, - entries: entries.map((e) => ({ ...e })), - overallComment: '', - isPeerReview: false, - groupId, - gradedBy: undefined, - gradedAt: undefined, - submittedAt: undefined, - notHandedIn: undefined, - round: undefined, - }; - }); - srs.forEach((sr) => dispatch({ type: 'SAVE_STUDENT_RUBRIC', payload: sr })); - return srs; - }, - [state.rubrics, state.studentRubrics] - ); + const createGroupStudentRubrics = useCallback((rubricId: string, studentIds: string[]): StudentRubric[] => { + const rubric = currentStateRef.current.rubrics.find((r) => r.id === rubricId); + const entries: ScoreEntry[] = (rubric?.criteria ?? []).map((c) => ({ + criterionId: c.id, + levelId: null, + comment: '', + checkedSubItems: [], + })); + const groupId = nanoid(); + const srs = studentIds.map((studentId): StudentRubric => { + const existing = currentStateRef.current.studentRubrics.find( + (sr) => sr.rubricId === rubricId && sr.studentId === studentId && !sr.isPeerReview + ); + return { + ...existing, + id: existing?.id ?? nanoid(), + rubricId, + studentId, + entries: entries.map((e) => ({ ...e })), + overallComment: '', + isPeerReview: false, + groupId, + gradedBy: undefined, + gradedAt: undefined, + submittedAt: undefined, + notHandedIn: undefined, + round: undefined, + }; + }); + srs.forEach((sr) => dispatch({ type: 'SAVE_STUDENT_RUBRIC', payload: sr })); + return srs; + }, []); const deleteStudentRubric = useCallback((id: string, scope: 'student' | 'group') => { logAuditEvent('grade', 'student_rubric_delete', 'student_rubric', id, { scope }); @@ -2011,6 +2015,9 @@ export function AppProvider({ children }: { children: ReactNode }) { [] ); + // Read via currentStateRef for the same reason as createStudentRubric: gradeScales belongs to + // the authoring domain, so closing over it would re-render every settings consumer on any + // grade-scale change. const getActiveGradeScale = useCallback((): GradeScale => { const { gradeScales, settings } = currentStateRef.current; return gradeScales.find((gs) => gs.id === settings.defaultGradeScaleId) ?? gradeScales[0];