From de4c8a56f5cc882fbfa0fb554d6cd5e02718bd58 Mon Sep 17 00:00:00 2001 From: Duane Nykamp Date: Fri, 14 Aug 2026 19:59:28 -0500 Subject: [PATCH 1/4] Separate document numbering from scored item numbering MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `isDescription` marked a document as unscored and unnumbered, and the question counter, credit extraction, shuffle anchoring, and per-item attempt button already honored it. The item *indexing* did not: `getNumItems` and `getItemSequence` counted descriptions, while `extractActivityItemCredit` excluded them. Those two indexings feed different fields of the same `SPLICE.reportScoreAndState` message, and the host stores per-item state keyed by the scored numbering. With any description present, item state was therefore written to the wrong item. Split the two concepts: - Document sequence (`getDocSequence`/`getNumDocs`, renamed from `getItemSequence`/`getNumItems`, whose doc comment already said "documents") — every rendered document, descriptions included. Drives mounting, pagination, and hide/keep-live. - Scored item sequence (`getScoredItemSequence`/`getNumScoredItems`) — descriptions excluded, in render order. Indexes `doenetStates` and `itemAttemptNumbers`, and produces `item_updated` and `new_doenet_state_idx`. `getScoredItemSequence` filters `getDocSequence` rather than recursing separately, which makes the subsequence invariant structural. A description now reports nothing on state update: it holds no slot in `doenetStates` and its state is not persisted, so emitting a report would cost a save round trip for a row `loadState` ignores. `new_attempt_for_item` is unchanged — it is derived from `item_scores`, so it is already in original order and description-free. Descriptions inside a `select` are rejected rather than silently miscounted: `extractSelectItemCredit` scores a single-document select regardless of `isDescription`, and `propagateStateChangeToRoot` averages over all selected children without filtering. Saved state cannot collide with the new indexing: `createSourceHash` hashes `isDescription`, so a source containing a description necessarily has a different hash and starts fresh. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01K3bUTUym3xhuXLegDyTNZ2 --- package.json | 2 +- src/Activity/Activity.tsx | 18 +- src/Activity/SingleDocActivity.tsx | 2 +- src/Activity/activityState.ts | 66 +++++-- src/Activity/activityStateReducer.ts | 31 +++- src/Activity/selectState.ts | 29 ++- src/Activity/sequenceState.ts | 37 ++-- src/Viewer/Viewer.tsx | 141 ++++++++------ src/test/activityState.test.ts | 117 ++++++++++-- src/test/activityStateReducer.test.ts | 172 ++++++++++++++++++ src/test/testSources/selWithDes.json | 25 +++ .../ActivityViewer.descriptions.cy.tsx | 155 ++++++++++++++++ 12 files changed, 681 insertions(+), 114 deletions(-) create mode 100644 src/test/testSources/selWithDes.json create mode 100644 test/cypress/component/ActivityViewer.descriptions.cy.tsx diff --git a/package.json b/package.json index 2845692..336faa3 100644 --- a/package.json +++ b/package.json @@ -2,7 +2,7 @@ "name": "@doenet/assignment-viewer", "private": false, "description": "View assignments from questions written in DoenetML", - "version": "0.1.0-alpha-17", + "version": "0.1.0-alpha-18", "license": "AGPL-3.0-or-later", "homepage": "https://github.com/Doenet/assignment-viewer#readme", "type": "module", diff --git a/src/Activity/Activity.tsx b/src/Activity/Activity.tsx index 749b630..deb66c0 100644 --- a/src/Activity/Activity.tsx +++ b/src/Activity/Activity.tsx @@ -45,7 +45,8 @@ export type ActivityCommonProps = { ) => void; hasRenderedCallback: (id: string) => void; itemAttemptNumbers: number[]; - itemIndexById: ReadonlyMap; + /** Position of each scored item, i.e. of each document that isn't a description. */ + scoredItemIndexById: ReadonlyMap; itemWord: string; }; @@ -63,10 +64,11 @@ export const Activity = memo(function Activity({ doenetStates, itemAttemptNumbers, answerResponseCountsByItem = [], - itemIndexById, + scoredItemIndexById, ...leafProps } = props; - const itemIdx = itemIndexById.get(state.id) ?? -1; + // A description holds no slot in the per-item arrays. + const itemIdx = scoredItemIndexById.get(state.id) ?? -1; return ( ); } diff --git a/src/Activity/SingleDocActivity.tsx b/src/Activity/SingleDocActivity.tsx index b09cf5e..cbe69d8 100644 --- a/src/Activity/SingleDocActivity.tsx +++ b/src/Activity/SingleDocActivity.tsx @@ -8,7 +8,7 @@ type SingleDocActivityProps = Omit< | "doenetStates" | "itemAttemptNumbers" | "answerResponseCountsByItem" - | "itemIndexById" + | "scoredItemIndexById" > & { state: SingleDocState; /** This item's saved Doenet state (its slice of `doenetStates`). */ diff --git a/src/Activity/activityState.ts b/src/Activity/activityState.ts index 11c2032..7c71cc3 100644 --- a/src/Activity/activityState.ts +++ b/src/Activity/activityState.ts @@ -25,14 +25,16 @@ import { SelectSource, SelectState, SelectStateNoSource, - getNumItemsInSelect, + getNumDocsInSelect, + getNumScoredItemsInSelect, } from "./selectState"; import { addSourceToSequenceState, calcNumVariantsSequence, extractSequenceItemCredit, generateNewSequenceAttempt, - getNumItemsInSequence, + getNumDocsInSequence, + getNumScoredItemsInSequence, initializeSequenceState, isSequenceSource, isSequenceState, @@ -240,8 +242,7 @@ export function initializeActivityAndDoenetState({ restrictToVariantSlice, }); - const numItems = getNumItems(source); - const itemAttemptNumbers = Array(numItems).fill(1); + const itemAttemptNumbers = Array(getNumScoredItems(source)).fill(1); return { activityState, doenetStates: [], @@ -514,8 +515,11 @@ export function extractSourceId(compositeId: string): string { /** * Returns an array of the activity ids of the single document activities, * in the order they will appear. + * + * Includes descriptions, which are rendered like any other document. + * Use `getScoredItemSequence` for the sequence of scored items. */ -export function getItemSequence(state: ActivityState): string[] { +export function getDocSequence(state: ActivityState): string[] { if (state.type === "singleDoc") { return [state.id]; } else { @@ -525,7 +529,7 @@ export function getItemSequence(state: ActivityState): string[] { return []; } else { const prelimResult = state.allChildren.flatMap((a) => - getItemSequence(a), + getDocSequence(a), ); if (state.type === "sequence") { return prelimResult; @@ -535,13 +539,30 @@ export function getItemSequence(state: ActivityState): string[] { } } if (state.type === "sequence") { - return state.orderedChildren.flatMap((a) => getItemSequence(a)); + return state.orderedChildren.flatMap((a) => getDocSequence(a)); } else { - return state.selectedChildren.flatMap((a) => getItemSequence(a)); + return state.selectedChildren.flatMap((a) => getDocSequence(a)); } } } +/** + * Returns an array of the activity ids of the single document activities that count + * as scored items, i.e., `getDocSequence` with the descriptions removed. + * + * The index of an id in this array is its `shuffledOrder` from + * `extractActivityItemCredit` minus one, which is the indexing used by + * `doenetStates` and `itemAttemptNumbers`. + */ +export function getScoredItemSequence(state: ActivityState): string[] { + const allStates = gatherStates(state); + + return getDocSequence(state).filter((id) => { + const docState = allStates[id]; + return docState.type !== "singleDoc" || !docState.source.isDescription; + }); +} + /** * Assuming that `numActivityVariants` contains the number of variants for all single doc activities, * calculate the number of unique variants of the the activity given by `source`. @@ -594,16 +615,39 @@ export function calcNumVariantsFromState( * * Throw an error if a select has options with different numbers of documents. */ -export function getNumItems(source: ActivitySource): number { +export function getNumDocs(source: ActivitySource): number { switch (source.type) { case "singleDoc": { return 1; } case "select": { - return getNumItemsInSelect(source); + return getNumDocsInSelect(source); + } + case "sequence": { + return getNumDocsInSequence(source); + } + } + + throw Error("Invalid activity type"); +} + +/** + * Return the number of documents of this activity that count as scored items, + * i.e., all rendered documents except the descriptions. + * + * Throw an error if a select has options with different numbers of documents, + * or if a select contains a description. + */ +export function getNumScoredItems(source: ActivitySource): number { + switch (source.type) { + case "singleDoc": { + return source.isDescription ? 0 : 1; + } + case "select": { + return getNumScoredItemsInSelect(source); } case "sequence": { - return getNumItemsInSequence(source); + return getNumScoredItemsInSequence(source); } } diff --git a/src/Activity/activityStateReducer.ts b/src/Activity/activityStateReducer.ts index bddf32a..cb489aa 100644 --- a/src/Activity/activityStateReducer.ts +++ b/src/Activity/activityStateReducer.ts @@ -12,8 +12,9 @@ import { gatherStates, generateNewActivityAttempt, generateNewSingleDocSubAttempt, - getItemSequence, - getNumItems, + getDocSequence, + getNumScoredItems, + getScoredItemSequence, initializeActivityState, propagateStateChangeToRoot, pruneActivityStateForSave, @@ -77,7 +78,7 @@ export function activityDoenetStateReducer( const activityState = state.activityState; switch (action.type) { case "initialize": { - const numItems = getNumItems(action.source); + const numScoredItems = getNumScoredItems(action.source); return { activityState: initializeActivityState({ source: action.source, @@ -86,7 +87,7 @@ export function activityDoenetStateReducer( numActivityVariants: action.numActivityVariants, }), doenetStates: [], - itemAttemptNumbers: Array(numItems).fill(1), + itemAttemptNumbers: Array(numScoredItems).fill(1), stateVersion: state.stateVersion + 1, errMsg: null, }; @@ -170,12 +171,17 @@ export function activityDoenetStateReducer( ); } - // The document's position in the item sequence is derived from - // the reducer's own state (callers used to pass it in, which - // required them to track the current sequence). - const doenetStateIdx = getItemSequence(activityState).indexOf( + // The document's position in the scored item sequence is derived + // from the reducer's own state (callers used to pass it in, which + // required them to track the current sequence). Descriptions hold + // no slot in `doenetStates`/`itemAttemptNumbers` and offer no + // attempt button, so there is nothing to regenerate for them. + const doenetStateIdx = getScoredItemSequence(activityState).indexOf( action.docId, ); + if (doenetStateIdx === -1) { + return state; + } let newActivityState; try { @@ -248,7 +254,14 @@ export function activityDoenetStateReducer( // attempt, or after a select re-picked its children. Ignore it: // recording it would corrupt another item's slot, and throwing // would unmount the whole viewer via an error boundary. - const doenetStateIdx = getItemSequence(activityState).indexOf( + if (!getDocSequence(activityState).includes(action.docId)) { + return state; + } + + // Descriptions are not scored, hold no slot in `doenetStates`, and + // their state is not persisted, so there is nothing to record or + // report. Their credit is already excluded from the activity score. + const doenetStateIdx = getScoredItemSequence(activityState).indexOf( action.docId, ); if (doenetStateIdx === -1) { diff --git a/src/Activity/selectState.ts b/src/Activity/selectState.ts index feffafd..94b839b 100644 --- a/src/Activity/selectState.ts +++ b/src/Activity/selectState.ts @@ -13,7 +13,8 @@ import { extractActivityItemCredit, extractSourceId, generateNewActivityAttempt, - getNumItems, + getNumDocs, + getNumScoredItems, initializeActivityState, isActivitySource, isActivityState, @@ -742,16 +743,16 @@ export function calcNumVariantsSelect( * * Throw an error if this select has options with different numbers of documents. */ -export function getNumItemsInSelect(source: SelectSource): number { +export function getNumDocsInSelect(source: SelectSource): number { if (source.items.length === 0) { return 0; } - const numDocumentsPerItem = getNumItems(source.items[0]); + const numDocumentsPerItem = getNumDocs(source.items[0]); if (source.items.length > 1) { for (const item of source.items.slice(1)) { - if (getNumItems(item) !== numDocumentsPerItem) { + if (getNumDocs(item) !== numDocumentsPerItem) { throw Error( "The case where a select has options with different numbers of documents is not implemented", ); @@ -761,3 +762,23 @@ export function getNumItemsInSelect(source: SelectSource): number { return source.numToSelect * numDocumentsPerItem; } + +/** + * Return the number of documents of this select that count as scored items. + * + * Descriptions inside a select are not supported: `extractSelectItemCredit` scores a + * single-document select regardless of `isDescription`, and the select branch of + * `propagateStateChangeToRoot` averages credit over all selected children without + * filtering out descriptions. Rather than silently miscount, throw. + */ +export function getNumScoredItemsInSelect(source: SelectSource): number { + for (const item of source.items) { + if (getNumScoredItems(item) !== getNumDocs(item)) { + throw Error( + "The case where a select contains a description is not implemented", + ); + } + } + + return getNumDocsInSelect(source); +} diff --git a/src/Activity/sequenceState.ts b/src/Activity/sequenceState.ts index 62446e6..f3fd9b1 100644 --- a/src/Activity/sequenceState.ts +++ b/src/Activity/sequenceState.ts @@ -7,7 +7,8 @@ import { extractActivityItemCredit, extractSourceId, generateNewActivityAttempt, - getNumItems, + getNumDocs, + getNumScoredItems, initializeActivityState, isActivitySource, isActivityState, @@ -241,14 +242,14 @@ export function generateNewSequenceAttempt({ const rng = rngClass(rngSeed); - // randomly shuffle `numItems` components of `arr` starting with `startInd` + // randomly shuffle `runLength` components of `arr` starting with `startInd` function shuffle_ids( arr: string[], startInd: number, - numItems: number, + runLength: number, ) { // https://stackoverflow.com/a/12646864 - for (let i = numItems - 1; i > 0; i--) { + for (let i = runLength - 1; i > 0; i--) { const j = Math.floor(rng() * (i + 1)); [arr[startInd + i], arr[startInd + j]] = [ arr[startInd + j], @@ -272,23 +273,23 @@ export function generateNewSequenceAttempt({ } // find the next item that is a description - let numItems = 1; + let runLength = 1; while ( - state.allChildren[startInd + numItems] && - (state.allChildren[startInd + numItems].type !== "singleDoc" || + state.allChildren[startInd + runLength] && + (state.allChildren[startInd + runLength].type !== "singleDoc" || !( - state.allChildren[startInd + numItems] + state.allChildren[startInd + runLength] .source as SingleDocSource ).isDescription) ) { - numItems++; + runLength++; } - if (numItems > 1) { + if (runLength > 1) { // shuffle the group of activities that were found between descriptions - shuffle_ids(childOrder, startInd, numItems); + shuffle_ids(childOrder, startInd, runLength); } - startInd += numItems; + startInd += runLength; } } @@ -451,8 +452,8 @@ export function calcNumVariantsSequence( /** * Return the number of documents that will be rendered by this sequence. */ -export function getNumItemsInSequence(source: SequenceSource): number { - const numDocumentsForEachItem = source.items.map(getNumItems); +export function getNumDocsInSequence(source: SequenceSource): number { + const numDocumentsForEachItem = source.items.map(getNumDocs); const totalNumDocuments = numDocumentsForEachItem.reduce( (a, c) => a + c, @@ -461,3 +462,11 @@ export function getNumItemsInSequence(source: SequenceSource): number { return totalNumDocuments; } + +/** + * Return the number of documents of this sequence that count as scored items, + * i.e., all rendered documents except the descriptions. + */ +export function getNumScoredItemsInSequence(source: SequenceSource): number { + return source.items.map(getNumScoredItems).reduce((a, c) => a + c, 0); +} diff --git a/src/Viewer/Viewer.tsx b/src/Viewer/Viewer.tsx index 17ce97f..c753b0c 100644 --- a/src/Viewer/Viewer.tsx +++ b/src/Viewer/Viewer.tsx @@ -14,13 +14,15 @@ import { ActivitySource, ActivityState, addSourceToActivityState, - getItemSequence, + getDocSequence, validateIds, isExportedActivityState, validateStateAndSource, gatherDocumentStructure, initializeActivityAndDoenetState, - getNumItems, + getNumDocs, + getNumScoredItems, + getScoredItemSequence, createSourceHash, } from "../Activity/activityState"; import type { MountPolicy } from "@doenet/doenetml-iframe"; @@ -93,29 +95,37 @@ export function Viewer({ // Source analysis. A source error is *derived* from the memo (not set // into state), so a later valid `source` self-clears it. - const { numActivityVariants, sourceHash, numItems, sourceErrMsg } = - useMemo(() => { - try { - validateIds(source); - const docStructure = gatherDocumentStructure(source); - const sourceHash = createSourceHash(source); - const numItems = getNumItems(source); - return { - ...docStructure, - sourceHash, - numItems, - sourceErrMsg: null, - }; - } catch (e) { - const message = e instanceof Error ? e.message : ""; - return { - numActivityVariants: {}, - sourceHash: "", - numItems: 0, - sourceErrMsg: `Error in activity source: ${message}`, - }; - } - }, [source]); + const { + numActivityVariants, + sourceHash, + numDocs, + numScoredItems, + sourceErrMsg, + } = useMemo(() => { + try { + validateIds(source); + const docStructure = gatherDocumentStructure(source); + const sourceHash = createSourceHash(source); + const numDocs = getNumDocs(source); + const numScoredItems = getNumScoredItems(source); + return { + ...docStructure, + sourceHash, + numDocs, + numScoredItems, + sourceErrMsg: null, + }; + } catch (e) { + const message = e instanceof Error ? e.message : ""; + return { + numActivityVariants: {}, + sourceHash: "", + numDocs: 0, + numScoredItems: 0, + sourceErrMsg: `Error in activity source: ${message}`, + }; + } + }, [source]); const [activityDoenetState, activityDoenetStateDispatch] = useReducer( activityDoenetStateReducer, @@ -142,27 +152,45 @@ export function Viewer({ const runtimeErrMsg = activityDoenetState.errMsg; // Content-stable: every reducer action (each score report included) - // rebuilds `activityState`, but the sequence of item ids rarely changes. - // Keeping the previous identity when the ids match lets everything - // derived from it (`itemIndexById`, the callbacks, the memoized item - // subtrees) stay stable across reports. - const computedItemSequence = useMemo( - () => getItemSequence(activityState), + // rebuilds `activityState`, but the sequence of document ids rarely + // changes. Keeping the previous identity when the ids match lets + // everything derived from it (`docIndexById`, the callbacks, the memoized + // item subtrees) stay stable across reports. + const computedDocSequence = useMemo( + () => getDocSequence(activityState), + [activityState], + ); + const docSequence = useContentStable( + computedDocSequence, + JSON.stringify(computedDocSequence), + ); + + // Pagination and mounting run over every document, descriptions included. + const docIndexById = useMemo( + () => new Map(docSequence.map((id, idx) => [id, idx])), + [docSequence], + ); + + // Item numbering, per-item attempts, and the indices into `doenetStates` + // run over the scored items only, i.e. the documents that aren't + // descriptions. + const computedScoredItemSequence = useMemo( + () => getScoredItemSequence(activityState), [activityState], ); - const itemSequence = useContentStable( - computedItemSequence, - JSON.stringify(computedItemSequence), + const scoredItemSequence = useContentStable( + computedScoredItemSequence, + JSON.stringify(computedScoredItemSequence), ); - const itemIndexById = useMemo( - () => new Map(itemSequence.map((id, idx) => [id, idx])), - [itemSequence], + const scoredItemIndexById = useMemo( + () => new Map(scoredItemSequence.map((id, idx) => [id, idx])), + [scoredItemSequence], ); - // The index of the current item - const [currentItemIdx, setCurrentItemIdx] = useState(0); - const currentItemId = itemSequence[currentItemIdx]; + // The index of the currently displayed document + const [currentDocIdx, setCurrentDocIdx] = useState(0); + const currentDocId = docSequence[currentDocIdx]; const [itemsRendered, setItemsRendered] = useState([]); @@ -183,23 +211,23 @@ export function Viewer({ if (!paginate || state.type !== "singleDoc") { return false; } - const itemIdx = itemIndexById.get(state.id); + const docIdx = docIndexById.get(state.id); return ( - itemIdx !== undefined && Math.abs(itemIdx - currentItemIdx) <= 1 + docIdx !== undefined && Math.abs(docIdx - currentDocIdx) <= 1 ); }, - [paginate, itemIndexById, currentItemIdx], + [paginate, docIndexById, currentDocIdx], ); const checkHidden = useCallback( (state: ActivityState) => { if (state.type === "singleDoc") { - return paginate && currentItemId !== state.id; + return paginate && currentDocId !== state.id; } else { return false; } }, - [currentItemId, paginate], + [currentDocId, paginate], ); useEffect(() => { @@ -312,10 +340,10 @@ export function Viewer({ ]); function clickNext() { - setCurrentItemIdx((was) => Math.min(numItems - 1, was + 1)); + setCurrentDocIdx((was) => Math.min(numDocs - 1, was + 1)); } function clickPrevious() { - setCurrentItemIdx((was) => Math.max(0, was - 1)); + setCurrentDocIdx((was) => Math.max(0, was - 1)); } const reportScoreAndStateCallback = useCallback( @@ -340,10 +368,10 @@ export function Viewer({ const generateNewItemAttemptPrompt = useCallback( (id: string, initialQuestionCounter: number) => { newItemAttemptInfo.current = { id, initialQuestionCounter }; - setNewAttemptNum((itemIndexById.get(id) ?? 0) + 1); + setNewAttemptNum((scoredItemIndexById.get(id) ?? 0) + 1); dialogRef.current?.showModal(); }, - [itemIndexById], + [scoredItemIndexById], ); function generateNewItemAttempt() { @@ -375,7 +403,7 @@ export function Viewer({ function generateActivityAttempt() { setItemsRendered([]); - setCurrentItemIdx(0); + setCurrentDocIdx(0); activityDoenetStateDispatch({ type: "generateNewActivityAttempt", numActivityVariants, @@ -422,7 +450,8 @@ export function Viewer({ ); const newAttemptDisabled = - numItems === 0 || (maxAttemptsAllowed > 0 && activityAttemptsLeft <= 0); + numScoredItems === 0 || + (maxAttemptsAllowed > 0 && activityAttemptsLeft <= 0); return (
@@ -506,11 +535,11 @@ export function Viewer({ borderRadius: "10px", padding: "5px 20px", }} - disabled={currentItemIdx <= 0} + disabled={currentDocIdx <= 0} > Previous - Page {currentItemIdx + 1} of {numItems} + Page {currentDocIdx + 1} of {numDocs} @@ -566,7 +595,7 @@ export function Viewer({
) : null} diff --git a/src/test/activityState.test.ts b/src/test/activityState.test.ts index 5b72c72..7e410e2 100644 --- a/src/test/activityState.test.ts +++ b/src/test/activityState.test.ts @@ -6,8 +6,11 @@ import { calcNumVariantsFromState, gatherDocumentStructure, generateNewActivityAttempt, - getItemSequence, - getNumItems, + extractActivityItemCredit, + getDocSequence, + getNumDocs, + getNumScoredItems, + getScoredItemSequence, initializeActivityState, pruneActivityStateForSave, validateIds, @@ -23,6 +26,7 @@ import seq0 from "./testSources/seq0.json"; import seq2Sel0 from "./testSources/seq2Sel0.json"; import seqSel0Sel from "./testSources/seqSel0Sel.json"; import seqWithDes from "./testSources/seqWithDes.json"; +import selWithDes from "./testSources/selWithDes.json"; import { SelectSource, @@ -566,12 +570,12 @@ describe("Activity state tests", () => { expect(["doc3", "doc2", "doc1"].includes(docFromSecondSelect)).eq(true); if (state.orderedChildren[0].id === firstSelectState.id) { - expect(getItemSequence(state)).eqls([ + expect(getDocSequence(state)).eqls([ docFromFirstSelect, docFromSecondSelect, ]); } else { - expect(getItemSequence(state)).eqls([ + expect(getDocSequence(state)).eqls([ docFromSecondSelect, docFromFirstSelect, ]); @@ -596,7 +600,7 @@ describe("Activity state tests", () => { parentAttempt: 1, }); - expect(getItemSequence(state)).eqls([]); + expect(getDocSequence(state)).eqls([]); }); it("get item sequence, sequence of 0", () => { @@ -617,7 +621,7 @@ describe("Activity state tests", () => { parentAttempt: 1, }); - expect(getItemSequence(state)).eqls([]); + expect(getDocSequence(state)).eqls([]); }); it("get item sequence, sequence of two selects from 0", () => { @@ -638,7 +642,7 @@ describe("Activity state tests", () => { parentAttempt: 1, }); - expect(getItemSequence(state)).eqls([]); + expect(getDocSequence(state)).eqls([]); }); it("get item sequence, sequence of select from 0 and select", () => { @@ -666,7 +670,7 @@ describe("Activity state tests", () => { const docFromSecondSelect = secondSelectState.selectedChildren[0].id; expect(["doc3", "doc2", "doc1"].includes(docFromSecondSelect)).eq(true); - expect(getItemSequence(state)).eqls([docFromSecondSelect]); + expect(getDocSequence(state)).eqls([docFromSecondSelect]); }); it("error when select multiple from a single doc with selectByVariant=false", () => { @@ -691,14 +695,101 @@ describe("Activity state tests", () => { }); it("return number of documents", () => { - expect(getNumItems(seq2sel as SequenceSource)).eq(2); + expect(getNumDocs(seq2sel as SequenceSource)).eq(2); - expect(getNumItems(selMult2docs as SelectSource)).eq(2); - expect(getNumItems(selMult1doc as SelectSource)).eq(3); + expect(getNumDocs(selMult2docs as SelectSource)).eq(2); + expect(getNumDocs(selMult1doc as SelectSource)).eq(3); // handle cases with no items - expect(getNumItems(sel0 as SelectSource)).eq(0); - expect(getNumItems(seq0 as SequenceSource)).eq(0); + expect(getNumDocs(sel0 as SelectSource)).eq(0); + expect(getNumDocs(seq0 as SequenceSource)).eq(0); + }); + + it("descriptions count as documents but not as scored items", () => { + const source = seqWithDes as SequenceSource; + + // two of the seven documents are descriptions + expect(getNumDocs(source)).eq(7); + expect(getNumScoredItems(source)).eq(5); + + // without descriptions, the two counts agree + expect(getNumScoredItems(seq2sel as SequenceSource)).eq(2); + expect(getNumScoredItems(selMult1doc as SelectSource)).eq(3); + expect(getNumScoredItems(seq0 as SequenceSource)).eq(0); + }); + + it("error when a select contains a description", () => { + expect(() => + getNumScoredItems(selWithDes as SelectSource), + ).toThrowError("select contains a description"); + }); + + it("scored item sequence is the document sequence without the descriptions", () => { + const source = seqWithDes as SequenceSource; + const { numActivityVariants } = gatherDocumentStructure(source); + + let state = initializeActivityState({ + source, + variant: 1, + parentId: null, + numActivityVariants, + }); + + // check over several attempts, as the sequence is shuffled + for (let attempt = 1; attempt <= 4; attempt++) { + ({ state } = generateNewActivityAttempt({ + state, + numActivityVariants, + initialQuestionCounter: 1, + parentAttempt: attempt, + })); + + const docSeq = getDocSequence(state); + const scoredSeq = getScoredItemSequence(state); + + expect(docSeq.length).eq(getNumDocs(source)); + expect(scoredSeq.length).eq(getNumScoredItems(source)); + + expect( + docSeq.filter((id) => !["doc1a", "doc3a"].includes(id)), + ).eqls(scoredSeq); + } + }); + + it("scored item sequence is indexed by shuffledOrder", () => { + // `doenetStates` and `itemAttemptNumbers` are indexed by position in + // the scored item sequence, while the host stores them by the + // `shuffledOrder` reported in `item_scores`. The two must agree. + const source = seqWithDes as SequenceSource; + const { numActivityVariants } = gatherDocumentStructure(source); + + let state = initializeActivityState({ + source, + variant: 1, + parentId: null, + numActivityVariants, + }); + + for (let attempt = 1; attempt <= 4; attempt++) { + ({ state } = generateNewActivityAttempt({ + state, + numActivityVariants, + initialQuestionCounter: 1, + parentAttempt: attempt, + })); + + const scoredSeq = getScoredItemSequence(state); + const itemScores = extractActivityItemCredit(state); + + expect(itemScores.length).eq(scoredSeq.length); + + for (const [idx, id] of scoredSeq.entries()) { + const itemScore = itemScores.find((s) => s.docId === id); + expect(itemScore?.shuffledOrder, `item score for ${id}`).eq( + idx + 1, + ); + } + } }); it("count each document as a question for initialQuestionCounter", () => { diff --git a/src/test/activityStateReducer.test.ts b/src/test/activityStateReducer.test.ts index c88904c..450a412 100644 --- a/src/test/activityStateReducer.test.ts +++ b/src/test/activityStateReducer.test.ts @@ -7,7 +7,12 @@ import { ActivityAndDoenetState, ActivityAndDoenetStateCore, createSourceHash, + extractActivityItemCredit, gatherDocumentStructure, + generateNewActivityAttempt, + getDocSequence, + getNumScoredItems, + getScoredItemSequence, initializeActivityState, pruneActivityStateForSave, } from "../Activity/activityState"; @@ -15,10 +20,12 @@ import { activityDoenetStateReducer } from "../Activity/activityStateReducer"; import seq2sel from "./testSources/seq2sel.json"; import doc from "./testSources/doc.json"; import seqShuf from "./testSources/seqShuf.json"; +import seqWithDes from "./testSources/seqWithDes.json"; import selMult2docs from "./testSources/selMult2docs.json"; import selMult1docNoVariant from "./testSources/selMult1docNoVariant.json"; import { SingleDocSource, SingleDocState } from "../Activity/singleDocState"; import { SelectSource, SelectState } from "../Activity/selectState"; +import { ReportStateMessage } from "../types"; /** * Build a full reducer state from its core fields (the reducer owns the @@ -2518,4 +2525,169 @@ describe("Activity reducer tests", () => { spy, }); }); + + describe("descriptions", () => { + /** A `seqWithDes` state with one attempt generated. */ + function setUp() { + const source = seqWithDes as SequenceSource; + const { numActivityVariants } = gatherDocumentStructure(source); + + const { state: activityState } = generateNewActivityAttempt({ + state: initializeActivityState({ + source, + variant: 1, + parentId: null, + numActivityVariants, + }), + numActivityVariants, + initialQuestionCounter: 1, + parentAttempt: 1, + }); + + return { + source, + numActivityVariants, + sourceHash: createSourceHash(source), + activityState, + state: mkState({ + activityState, + doenetStates: [], + itemAttemptNumbers: Array( + getNumScoredItems(source), + ).fill(1), + }), + }; + } + + it("a report from a description is ignored", () => { + vi.stubGlobal("window", { postMessage: vi.fn(() => null) }); + const spy = vi.spyOn(window, "postMessage"); + + const { state, sourceHash } = setUp(); + + const newState = activityDoenetStateReducer(state, { + type: "updateSingleState", + docId: "doc1a", + doenetState: { some: "state" }, + creditAchieved: 1, + allowSaveState: true, + baseId: "base", + sourceHash, + }); + + // A description holds no slot in `doenetStates`, so there is + // nothing to record and nothing to report. + expect(newState).eq(state); + expect(spy).toHaveBeenCalledTimes(0); + }); + + it("item numbers reported skip the descriptions", () => { + vi.stubGlobal("window", { postMessage: vi.fn(() => null) }); + const spy = vi.spyOn(window, "postMessage"); + + const { state, activityState, sourceHash } = setUp(); + + const docSeq = getDocSequence(activityState); + const scoredSeq = getScoredItemSequence(activityState); + + // pick a scored document that follows a description, so that its + // document index and its item index differ + const docId = scoredSeq.find( + (id) => docSeq.indexOf(id) > scoredSeq.indexOf(id), + )!; + expect(docId).not.eq(undefined); + const scoredIdx = scoredSeq.indexOf(docId); + + const newState = activityDoenetStateReducer(state, { + type: "updateSingleState", + docId, + doenetState: { some: "state" }, + creditAchieved: 1, + allowSaveState: true, + baseId: "base", + sourceHash, + }); + + expect(spy).toHaveBeenCalledTimes(1); + const message = spy.mock.lastCall![0] as ReportStateMessage; + + expect(message.item_updated).eq(scoredIdx + 1); + expect(message.new_doenet_state_idx).eq(scoredIdx); + // the document index is larger, as descriptions precede it + expect(message.item_updated).lessThan(docSeq.indexOf(docId) + 1); + + // this is the indexing the host stores state by + const itemScore = message.item_scores.find( + (s) => s.docId === docId, + ); + expect(itemScore?.shuffledOrder).eq(message.item_updated); + + expect(newState.doenetStates[scoredIdx]).eqls({ some: "state" }); + expect(newState.doenetStates.length).lessThanOrEqual( + scoredSeq.length, + ); + }); + + it("new item attempt uses the scored item indexing", () => { + vi.stubGlobal("window", { postMessage: vi.fn(() => null) }); + const spy = vi.spyOn(window, "postMessage"); + + const { state, activityState, sourceHash, numActivityVariants } = + setUp(); + + const scoredSeq = getScoredItemSequence(activityState); + const docSeq = getDocSequence(activityState); + const docId = scoredSeq.find( + (id) => docSeq.indexOf(id) > scoredSeq.indexOf(id), + )!; + const scoredIdx = scoredSeq.indexOf(docId); + + const newState = activityDoenetStateReducer(state, { + type: "generateSingleDocSubActivityAttempt", + docId, + numActivityVariants, + initialQuestionCounter: 1, + allowSaveState: true, + baseId: "base", + sourceHash, + }); + + // only the one item's attempt number is bumped + expect(newState.itemAttemptNumbers.length).eq(scoredSeq.length); + expect(newState.itemAttemptNumbers).eqls( + scoredSeq.map((_, i) => (i === scoredIdx ? 2 : 1)), + ); + + const message = spy.mock.lastCall![0] as ReportStateMessage; + expect(message.new_doenet_state_idx).eq(scoredIdx); + + // `new_attempt_for_item` is in the *original* (unshuffled) order, + // so it need not equal the shuffled `new_doenet_state_idx + 1` + const originalOrder = + extractActivityItemCredit(activityState).findIndex( + (s) => s.docId === docId || s.id === docId, + ) + 1; + expect(message.new_attempt_for_item).eq(originalOrder); + }); + + it("a new item attempt for a description is ignored", () => { + vi.stubGlobal("window", { postMessage: vi.fn(() => null) }); + const spy = vi.spyOn(window, "postMessage"); + + const { state, sourceHash, numActivityVariants } = setUp(); + + const newState = activityDoenetStateReducer(state, { + type: "generateSingleDocSubActivityAttempt", + docId: "doc3a", + numActivityVariants, + initialQuestionCounter: 1, + allowSaveState: true, + baseId: "base", + sourceHash, + }); + + expect(newState).eq(state); + expect(spy).toHaveBeenCalledTimes(0); + }); + }); }); diff --git a/src/test/testSources/selWithDes.json b/src/test/testSources/selWithDes.json new file mode 100644 index 0000000..98ee397 --- /dev/null +++ b/src/test/testSources/selWithDes.json @@ -0,0 +1,25 @@ +{ + "title": "My select", + "id": "sel", + "type": "select", + "numToSelect": 1, + "selectByVariant": false, + "items": [ + { + "id": "doc1", + "type": "singleDoc", + "isDescription": false, + "doenetML": "Enter z: $a", + "version": "0.7.4", + "numVariants": 1 + }, + { + "id": "doc2a", + "type": "singleDoc", + "isDescription": true, + "doenetML": "Some instructions", + "version": "0.7.4", + "numVariants": 1 + } + ] +} diff --git a/test/cypress/component/ActivityViewer.descriptions.cy.tsx b/test/cypress/component/ActivityViewer.descriptions.cy.tsx new file mode 100644 index 0000000..7e61455 --- /dev/null +++ b/test/cypress/component/ActivityViewer.descriptions.cy.tsx @@ -0,0 +1,155 @@ +import React from "react"; +import { ActivityViewer } from "../../../src/activity-viewer"; +import type { ActivitySource } from "../../../src/Activity/activityState"; +import type { SingleDocSource } from "../../../src/Activity/singleDocState"; +import { IFRAME_READY_TIMEOUT } from "./helpers"; + +// A description is a document that is rendered like any other but is not one +// of the scored items: it gets a page, but no problem number, no per-item +// attempt, and no slot in the item numbering the host stores state by. + +function mkDoc( + id: string, + label: string, + isDescription: boolean, +): SingleDocSource { + return { + id, + type: "singleDoc", + isDescription, + doenetML: isDescription + ? `

${label}

` + : `

${label}:

`, + version: "0.7.24", + numVariants: 1, + }; +} + +// description, problem, problem — so the first problem is on page 2 +const source: ActivitySource = { + type: "sequence", + id: "seq", + title: "with descriptions", + shuffle: false, + items: [ + mkDoc("des1", "Read this first", true), + mkDoc("doc1", "First problem", false), + mkDoc("doc2", "Second problem", false), + ], +} as ActivitySource; + +function itemIframe(id: string, options?: { timeout?: number }) { + return cy.get(`iframe[srcdoc*='"docId":"${id}"']`, options); +} + +/** Assert rendered (script-stripped) iframe content for an item. */ +function assertItemContent(id: string, text: string) { + itemIframe(id) + .its("0.contentDocument.body", { timeout: IFRAME_READY_TIMEOUT }) + .should((body: HTMLElement) => { + const clone = body.cloneNode(true) as HTMLElement; + clone.querySelectorAll("script").forEach((s) => { + s.remove(); + }); + expect(clone.textContent).to.contain(text); + }); +} + +describe("ActivityViewer — descriptions", () => { + it("descriptions get a page but are not scored items", () => { + cy.viewport(900, 700); + cy.mount( + , + ); + + // Pagination counts every document, the description included. + cy.contains("Page 1 of 3"); + assertItemContent("des1", "Read this first"); + + // A description offers no per-item attempt. Every page stays mounted, + // so count the buttons across the activity: one per scored item. + cy.get("[data-test='New Item Attempt']").should("have.length", 2); + cy.get("[data-test='New Item Attempt']:visible").should("not.exist"); + + // Problem numbering skips the description: the first problem, on the + // second page, is numbered 1. + cy.contains("button", "Next").click(); + cy.contains("Page 2 of 3"); + assertItemContent("doc1", "Problem 1"); + + // The new-attempt dialog uses the scored item numbering, so the first + // problem is "problem 1" rather than "problem 2". + cy.get("[data-test='New Item Attempt']:visible").click(); + cy.contains("new version of problem 1"); + cy.get("[data-test='Cancel Create New Attempt']").click(); + + cy.contains("button", "Next").click(); + cy.contains("Page 3 of 3"); + assertItemContent("doc2", "Problem 2"); + }); + + it("only scored items report state to the host", () => { + const reports: { docId: string; itemUpdated?: number }[] = []; + + cy.viewport(900, 700); + cy.mount( + , + ); + + cy.window().then((win) => { + win.addEventListener("message", (event: MessageEvent) => { + const data = event.data as { + subject?: string; + item_updated?: number; + item_scores?: { docId: string }[]; + }; + if (data.subject === "SPLICE.reportScoreAndState") { + reports.push({ + docId: + data.item_scores?.map((s) => s.docId).join(",") ?? + "", + itemUpdated: data.item_updated, + }); + } + }); + }); + + // Answer the first problem, which follows the description. + cy.contains("button", "Next").click(); + assertItemContent("doc1", "Problem 1"); + itemIframe("doc1") + .its("0.contentDocument.body") + .find("input:not([type=checkbox])") + .then(($el) => cy.wrap($el)) + .type("an answer{enter}"); + + cy.then(() => { + expect(reports.length).to.be.greaterThan(0); + // Reported item numbers count scored items only, so the first + // problem is item 1 even though it is the second document. + for (const report of reports) { + expect(report.itemUpdated).to.eq(1); + // The description never appears among the scored items. + expect(report.docId).to.not.contain("des1"); + } + }); + }); +}); From 85fa08440241861768ab902149e782dcb9adb6ed Mon Sep 17 00:00:00 2001 From: Duane Nykamp Date: Fri, 14 Aug 2026 20:08:28 -0500 Subject: [PATCH 2/4] Simplify scored-item indexing guards and tidy doc comments Collapse the redundant document-sequence membership check in `updateSingleState`: a stale id and a description both take the same no-op path through the scored item sequence lookup. Let `Activity.tsx` rely on the out-of-range index falling back rather than branching on the `-1` sentinel twice, and express the select and sequence counters uniformly. Add a test for a new item attempt on a document that is no longer in the activity, and pin the reported `item_scores` doc ids in the Cypress description test. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01K3bUTUym3xhuXLegDyTNZ2 --- src/Activity/Activity.tsx | 13 +++---- src/Activity/activityStateReducer.ts | 32 ++++++++--------- src/Activity/selectState.ts | 14 ++++---- src/Activity/sequenceState.ts | 14 +++----- src/test/activityStateReducer.test.ts | 35 +++++++++++++++++++ .../ActivityViewer.descriptions.cy.tsx | 17 +++++---- 6 files changed, 78 insertions(+), 47 deletions(-) diff --git a/src/Activity/Activity.tsx b/src/Activity/Activity.tsx index deb66c0..26bdb01 100644 --- a/src/Activity/Activity.tsx +++ b/src/Activity/Activity.tsx @@ -67,7 +67,8 @@ export const Activity = memo(function Activity({ scoredItemIndexById, ...leafProps } = props; - // A description holds no slot in the per-item arrays. + // A description holds no slot in the per-item arrays, so it gets + // an out-of-range index and falls back to the defaults below. const itemIdx = scoredItemIndexById.get(state.id) ?? -1; return ( ); } diff --git a/src/Activity/activityStateReducer.ts b/src/Activity/activityStateReducer.ts index cb489aa..0cfe7eb 100644 --- a/src/Activity/activityStateReducer.ts +++ b/src/Activity/activityStateReducer.ts @@ -12,7 +12,6 @@ import { gatherStates, generateNewActivityAttempt, generateNewSingleDocSubAttempt, - getDocSequence, getNumScoredItems, getScoredItemSequence, initializeActivityState, @@ -172,10 +171,11 @@ export function activityDoenetStateReducer( } // The document's position in the scored item sequence is derived - // from the reducer's own state (callers used to pass it in, which - // required them to track the current sequence). Descriptions hold - // no slot in `doenetStates`/`itemAttemptNumbers` and offer no - // attempt button, so there is nothing to regenerate for them. + // from the reducer's own state, so callers need not track the + // current sequence themselves. An id absent from that sequence is + // either stale or a description; descriptions hold no slot in + // `doenetStates`/`itemAttemptNumbers` and offer no attempt button, + // so either way there is nothing to regenerate. const doenetStateIdx = getScoredItemSequence(activityState).indexOf( action.docId, ); @@ -249,18 +249,16 @@ export function activityDoenetStateReducer( }; } case "updateSingleState": { - // A report can arrive from a document that is no longer part of - // the activity — e.g. an in-flight save from a just-regenerated - // attempt, or after a select re-picked its children. Ignore it: - // recording it would corrupt another item's slot, and throwing - // would unmount the whole viewer via an error boundary. - if (!getDocSequence(activityState).includes(action.docId)) { - return state; - } - - // Descriptions are not scored, hold no slot in `doenetStates`, and - // their state is not persisted, so there is nothing to record or - // report. Their credit is already excluded from the activity score. + // Two kinds of report have no slot to be recorded in, and both are + // silently ignored: recording one would corrupt another item's + // slot, and throwing would unmount the whole viewer via an error + // boundary. + // + // 1. A document that is no longer part of the activity — e.g. an + // in-flight save from a just-regenerated attempt, or after a + // select re-picked its children. + // 2. A description: unscored, unpersisted, and already excluded + // from the activity's credit. const doenetStateIdx = getScoredItemSequence(activityState).indexOf( action.docId, ); diff --git a/src/Activity/selectState.ts b/src/Activity/selectState.ts index 94b839b..5a92a50 100644 --- a/src/Activity/selectState.ts +++ b/src/Activity/selectState.ts @@ -772,12 +772,14 @@ export function getNumDocsInSelect(source: SelectSource): number { * filtering out descriptions. Rather than silently miscount, throw. */ export function getNumScoredItemsInSelect(source: SelectSource): number { - for (const item of source.items) { - if (getNumScoredItems(item) !== getNumDocs(item)) { - throw Error( - "The case where a select contains a description is not implemented", - ); - } + if ( + source.items.some( + (item) => getNumScoredItems(item) !== getNumDocs(item), + ) + ) { + throw Error( + "The case where a select contains a description is not implemented", + ); } return getNumDocsInSelect(source); diff --git a/src/Activity/sequenceState.ts b/src/Activity/sequenceState.ts index f3fd9b1..afe104a 100644 --- a/src/Activity/sequenceState.ts +++ b/src/Activity/sequenceState.ts @@ -453,14 +453,7 @@ export function calcNumVariantsSequence( * Return the number of documents that will be rendered by this sequence. */ export function getNumDocsInSequence(source: SequenceSource): number { - const numDocumentsForEachItem = source.items.map(getNumDocs); - - const totalNumDocuments = numDocumentsForEachItem.reduce( - (a, c) => a + c, - 0, - ); - - return totalNumDocuments; + return source.items.reduce((total, item) => total + getNumDocs(item), 0); } /** @@ -468,5 +461,8 @@ export function getNumDocsInSequence(source: SequenceSource): number { * i.e., all rendered documents except the descriptions. */ export function getNumScoredItemsInSequence(source: SequenceSource): number { - return source.items.map(getNumScoredItems).reduce((a, c) => a + c, 0); + return source.items.reduce( + (total, item) => total + getNumScoredItems(item), + 0, + ); } diff --git a/src/test/activityStateReducer.test.ts b/src/test/activityStateReducer.test.ts index 450a412..c6f2d0b 100644 --- a/src/test/activityStateReducer.test.ts +++ b/src/test/activityStateReducer.test.ts @@ -131,6 +131,41 @@ describe("Activity reducer tests", () => { expect(spy).toHaveBeenCalledTimes(0); }); + it("ignores a new item attempt for a document that is not in the activity", () => { + vi.stubGlobal("window", { + postMessage: vi.fn(() => null), + }); + const spy = vi.spyOn(window, "postMessage"); + + const source = doc as SingleDocSource; + const { numActivityVariants } = gatherDocumentStructure(source); + + const state = mkState({ + activityState: initializeActivityState({ + source, + variant: 5, + parentId: null, + numActivityVariants, + }), + doenetStates: [], + itemAttemptNumbers: [1], + }); + + const newState = activityDoenetStateReducer(state, { + type: "generateSingleDocSubActivityAttempt", + docId: "no-longer-present", + numActivityVariants, + initialQuestionCounter: 1, + allowSaveState: true, + baseId: "stale", + sourceHash: createSourceHash(source), + }); + + expect(newState).eq(state); + expect(newState.errMsg).eq(null); + expect(spy).toHaveBeenCalledTimes(0); + }); + it("initialize", () => { const source0 = seq2sel as SequenceSource; const state0 = initializeActivityState({ diff --git a/test/cypress/component/ActivityViewer.descriptions.cy.tsx b/test/cypress/component/ActivityViewer.descriptions.cy.tsx index 7e61455..71dedaf 100644 --- a/test/cypress/component/ActivityViewer.descriptions.cy.tsx +++ b/test/cypress/component/ActivityViewer.descriptions.cy.tsx @@ -99,7 +99,9 @@ describe("ActivityViewer — descriptions", () => { }); it("only scored items report state to the host", () => { - const reports: { docId: string; itemUpdated?: number }[] = []; + // reports of an item update; the initial `new_attempt` report carries + // no `item_updated` and is not collected + const reports: { itemDocIds: string[]; itemUpdated: number }[] = []; cy.viewport(900, 700); cy.mount( @@ -121,11 +123,14 @@ describe("ActivityViewer — descriptions", () => { item_updated?: number; item_scores?: { docId: string }[]; }; - if (data.subject === "SPLICE.reportScoreAndState") { + if ( + data.subject === "SPLICE.reportScoreAndState" && + data.item_updated !== undefined + ) { reports.push({ - docId: - data.item_scores?.map((s) => s.docId).join(",") ?? - "", + itemDocIds: (data.item_scores ?? []).map( + (s) => s.docId, + ), itemUpdated: data.item_updated, }); } @@ -148,7 +153,7 @@ describe("ActivityViewer — descriptions", () => { for (const report of reports) { expect(report.itemUpdated).to.eq(1); // The description never appears among the scored items. - expect(report.docId).to.not.contain("des1"); + expect(report.itemDocIds).to.eql(["doc1", "doc2"]); } }); }); From 0bf262b827fa140f372a1a1d1067a5352a41ce0d Mon Sep 17 00:00:00 2001 From: Duane Nykamp Date: Fri, 14 Aug 2026 20:12:21 -0500 Subject: [PATCH 3/4] Tighten reducer comments on the scored-item guards Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01K3bUTUym3xhuXLegDyTNZ2 --- src/Activity/activityStateReducer.ts | 22 +++++++++------------- 1 file changed, 9 insertions(+), 13 deletions(-) diff --git a/src/Activity/activityStateReducer.ts b/src/Activity/activityStateReducer.ts index 0cfe7eb..0f14fa2 100644 --- a/src/Activity/activityStateReducer.ts +++ b/src/Activity/activityStateReducer.ts @@ -173,9 +173,8 @@ export function activityDoenetStateReducer( // The document's position in the scored item sequence is derived // from the reducer's own state, so callers need not track the // current sequence themselves. An id absent from that sequence is - // either stale or a description; descriptions hold no slot in - // `doenetStates`/`itemAttemptNumbers` and offer no attempt button, - // so either way there is nothing to regenerate. + // either stale or a description, and neither has a slot in + // `doenetStates`/`itemAttemptNumbers` to regenerate. const doenetStateIdx = getScoredItemSequence(activityState).indexOf( action.docId, ); @@ -249,16 +248,13 @@ export function activityDoenetStateReducer( }; } case "updateSingleState": { - // Two kinds of report have no slot to be recorded in, and both are - // silently ignored: recording one would corrupt another item's - // slot, and throwing would unmount the whole viewer via an error - // boundary. - // - // 1. A document that is no longer part of the activity — e.g. an - // in-flight save from a just-regenerated attempt, or after a - // select re-picked its children. - // 2. A description: unscored, unpersisted, and already excluded - // from the activity's credit. + // A report with no slot in the scored item sequence is silently + // ignored: recording it would corrupt another item's slot, and + // throwing would unmount the whole viewer via an error boundary. + // Either the document is no longer part of the activity (an + // in-flight save from a just-regenerated attempt, or a select that + // re-picked its children), or it is a description, which is + // unscored and unpersisted. const doenetStateIdx = getScoredItemSequence(activityState).indexOf( action.docId, ); From 42bc54f1fad858b315990376d1943e4ac964e4cf Mon Sep 17 00:00:00 2001 From: Duane Nykamp Date: Fri, 14 Aug 2026 20:17:19 -0500 Subject: [PATCH 4/4] Align item/document naming and cover description edge cases Rename `itemsRendered` to `docsRendered` in `Viewer` and correct the comments and JSDoc that still said "item" where a document (descriptions included) is meant. Add a test exercising the description/scored split over several shuffled attempts for a bare root description, a sequence of only descriptions, a description as the last child, descriptions on both ends, and a sequence nested in a sequence. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01K3bUTUym3xhuXLegDyTNZ2 --- src/Activity/Activity.tsx | 20 +++--- src/Activity/SingleDocActivity.tsx | 5 +- src/Viewer/Viewer.tsx | 20 +++--- src/test/activityState.test.ts | 107 +++++++++++++++++++++++++++-- 4 files changed, 127 insertions(+), 25 deletions(-) diff --git a/src/Activity/Activity.tsx b/src/Activity/Activity.tsx index 26bdb01..83405fb 100644 --- a/src/Activity/Activity.tsx +++ b/src/Activity/Activity.tsx @@ -27,13 +27,13 @@ export type ActivityCommonProps = { answerResponseCountsByItem?: Record[]; doenetStates: unknown[]; stateVersion: number; - /** The windowed mounting policy every item's viewer registers with. */ + /** The windowed mounting policy every document's viewer registers with. */ mountPolicy: MountPolicy; useSharedCoreWorker?: boolean; reportScoreAndStateCallback: (args: unknown) => void; checkHidden: (state: ActivityState) => boolean; /** - * Whether an item's viewer should stay booted even while hidden or + * Whether a document's viewer should stay booted even while hidden or * off-screen (the paginator marks the current page and its neighbors * so page flips are instant). */ @@ -56,10 +56,10 @@ export const Activity = memo(function Activity({ }: ActivityCommonProps & { state: ActivityState }) { switch (state.type) { case "singleDoc": { - // Extract this item's slice of the per-item arrays so the leaf's - // props only change when *its* data changes: the arrays get a - // fresh identity on every report from any document, which would - // otherwise defeat SingleDocActivity's memo for all N items. + // Extract this document's slice of the per-item arrays so the + // leaf's props only change when *its* data changes: the arrays get + // a fresh identity on every report from any document, which would + // otherwise defeat SingleDocActivity's memo for all N documents. const { doenetStates, itemAttemptNumbers, @@ -69,7 +69,7 @@ export const Activity = memo(function Activity({ } = props; // A description holds no slot in the per-item arrays, so it gets // an out-of-range index and falls back to the defaults below. - const itemIdx = scoredItemIndexById.get(state.id) ?? -1; + const scoredItemIdx = scoredItemIndexById.get(state.id) ?? -1; return ( ); } diff --git a/src/Activity/SingleDocActivity.tsx b/src/Activity/SingleDocActivity.tsx index cbe69d8..a62d33f 100644 --- a/src/Activity/SingleDocActivity.tsx +++ b/src/Activity/SingleDocActivity.tsx @@ -11,7 +11,10 @@ type SingleDocActivityProps = Omit< | "scoredItemIndexById" > & { state: SingleDocState; - /** This item's saved Doenet state (its slice of `doenetStates`). */ + /** + * This document's saved Doenet state (its slice of `doenetStates`). + * Always null for a description, which has no slot in `doenetStates`. + */ doenetState: unknown; itemAttemptNumber: number; answerResponseCounts?: Record; diff --git a/src/Viewer/Viewer.tsx b/src/Viewer/Viewer.tsx index c753b0c..82f564c 100644 --- a/src/Viewer/Viewer.tsx +++ b/src/Viewer/Viewer.tsx @@ -140,9 +140,9 @@ export function Viewer({ const activityState = activityDoenetState.activityState; - // Identifies the state generation items were seeded from: bumped by the - // reducer when the whole activity re-initializes or loads saved state, - // telling items to re-read their initial Doenet state. + // Identifies the state generation documents were seeded from: bumped by + // the reducer when the whole activity re-initializes or loads saved state, + // telling documents to re-read their initial Doenet state. const stateVersion = activityDoenetState.stateVersion; // A runtime (attempt-generation) error leaves the previous activity @@ -155,7 +155,7 @@ export function Viewer({ // rebuilds `activityState`, but the sequence of document ids rarely // changes. Keeping the previous identity when the ids match lets // everything derived from it (`docIndexById`, the callbacks, the memoized - // item subtrees) stay stable across reports. + // document subtrees) stay stable across reports. const computedDocSequence = useMemo( () => getDocSequence(activityState), [activityState], @@ -192,7 +192,7 @@ export function Viewer({ const [currentDocIdx, setCurrentDocIdx] = useState(0); const currentDocId = docSequence[currentDocIdx]; - const [itemsRendered, setItemsRendered] = useState([]); + const [docsRendered, setDocsRendered] = useState([]); const [newAttemptNum, setNewAttemptNum] = useState(0); const dialogRef = useRef(null); @@ -200,7 +200,7 @@ export function Viewer({ const attemptNumber = activityState.attemptNumber; - // Every item's viewer is always mounted; the windowed mounting policy + // Every document's viewer is always mounted; the windowed mounting policy // (`mountPolicy`) decides which of them are actually booted, parking the // rest as placeholders. In paginated mode the current page and its // neighbors are marked `keepLive` so they boot eagerly (hidden pages @@ -385,7 +385,7 @@ export function Viewer({ baseId: activityId, sourceHash, }); - setItemsRendered((was) => { + setDocsRendered((was) => { const idx = was.indexOf(id); if (idx === -1) { return was; @@ -398,11 +398,11 @@ export function Viewer({ } const hasRenderedCallback = useCallback((id: string) => { - setItemsRendered((was) => (was.includes(id) ? was : [...was, id])); + setDocsRendered((was) => (was.includes(id) ? was : [...was, id])); }, []); function generateActivityAttempt() { - setItemsRendered([]); + setDocsRendered([]); setCurrentDocIdx(0); activityDoenetStateDispatch({ type: "generateNewActivityAttempt", @@ -595,7 +595,7 @@ export function Viewer({ ) : null}