From eed9fec8bed850822487e10e63056cb517f78696 Mon Sep 17 00:00:00 2001 From: Boris Goldowsky Date: Thu, 19 Sep 2024 13:01:30 -0400 Subject: [PATCH 1/3] Fix history bugs --- src/models/history/tree-manager.test.ts | 59 +++++++++++++++++++ src/models/history/tree-manager.ts | 14 +++-- src/models/history/tree-monitor.ts | 59 +++++++++++-------- src/models/history/undo-store.test.ts | 56 +++++++++--------- src/models/history/undo-store.ts | 15 +++-- .../data-card/components/case-attribute.tsx | 12 +++- .../components/data-card-toolbar-buttons.tsx | 2 +- src/plugins/data-card/data-card-content.ts | 29 ++++++--- src/plugins/data-card/data-card-tile.tsx | 27 +++++---- src/plugins/data-card/data-card-toolbar.tsx | 2 +- 10 files changed, 187 insertions(+), 88 deletions(-) diff --git a/src/models/history/tree-manager.test.ts b/src/models/history/tree-manager.test.ts index 423769e302..2f4d3992dd 100644 --- a/src/models/history/tree-manager.test.ts +++ b/src/models/history/tree-manager.test.ts @@ -226,6 +226,54 @@ const action4 = { undoable: true }; +const sharedModelChange = { + "action": "/content/sharedModelMap/sm1/sharedModel/setValue", + "created": expect.any(Number), + "id": expect.any(String), + "records": [ + { + "action": "/content/sharedModelMap/sm1/sharedModel/setValue", + "inversePatches": [ + { + "op": "replace", + "path": "/content/sharedModelMap/sm1/sharedModel/value", + "value": undefined, + }, + ], + "patches": [ + { + "op": "replace", + "path": "/content/sharedModelMap/sm1/sharedModel/value", + "value": "shared value", + }, + ], + "tree": "test", + }, + { + "action": "/handleSharedModelChanges", + "inversePatches": [ + { + "op": "replace", + "path": "/content/tileMap/t1/content/text", + }, + ], + "patches": [ + { + "op": "replace", + "path": "/content/tileMap/t1/content/text", + "value": "shared value-tile", + }, + ], + "tree": "test", + } + ], + "state": "complete", + "tree": "test", + "undoable": true, + }; + + + /** * Remove the Jest `expect.any(Number)` on created, and provide a real id. * @param entry @@ -295,6 +343,17 @@ it("can replay the history entries", async () => { expect(getSnapshot(manager.document.history)).toEqual(history); }); +it("records tile model changes in response to shared model changes", async () => { + const {tileContent, manager, sharedModel} = setupDocument(); + sharedModel.setValue("shared value"); + await expectEntryToBeComplete(manager, 1); + expect(tileContent.text).toBe("shared value-tile"); + expect(tileContent.updateCount).toBe(1); + + const changeDocument = manager.document as Instance; + expect(getSnapshot(changeDocument.history)).toEqual([sharedModelChange]); +}); + // TODO: it would nicer to use a custom Jest matcher here so we can // provide a better error message when it fails async function expectEntryToBeComplete(manager: Instance, length: number) { diff --git a/src/models/history/tree-manager.ts b/src/models/history/tree-manager.ts index 667e88ce8a..ea0c8ee9f7 100644 --- a/src/models/history/tree-manager.ts +++ b/src/models/history/tree-manager.ts @@ -543,13 +543,17 @@ export const TreeManager = types const startingIndex = direction === 1 ? self.numHistoryEventsApplied : self.numHistoryEventsApplied - 1; const endingIndex = direction === 1 ? newHistoryPosition : newHistoryPosition - 1; for (let i=startingIndex; i !== endingIndex; i=i+direction) { - const entry = self.document.history.at(i); - for (const treeEntry of (entry?.records || [])) { - const patches = treePatches[treeEntry.tree]; + const historyEntry = self.document.history.at(i); + let records = historyEntry ? [ ...historyEntry.records] : []; + if (direction === -1) { + records = records.reverse(); + } + for (const entryRecord of records) { + const patches = treePatches[entryRecord.tree]; if (newHistoryPosition > self.numHistoryEventsApplied) { - patches?.push(...treeEntry.getPatches(HistoryOperation.Redo)); + patches?.push(...entryRecord.getPatches(HistoryOperation.Redo)); } else { - patches?.push(...treeEntry.getPatches(HistoryOperation.Undo)); + patches?.push(...entryRecord.getPatches(HistoryOperation.Undo)); } } } diff --git a/src/models/history/tree-monitor.ts b/src/models/history/tree-monitor.ts index 939fcd6bad..365f4e572c 100644 --- a/src/models/history/tree-monitor.ts +++ b/src/models/history/tree-monitor.ts @@ -220,6 +220,7 @@ export class TreeMonitor { // TODO: If there are multiple shared model changes, we might want // to send them all to the tree at the same time, that way it can // inform the tiles of all changes at the same time. + const exchangesToProcess: { id: string, path: string }[] = []; for (const [sharedModelPath, numModifications] of Object.entries(sharedModelModifications)) { if (numModifications > 0) { // If a shared model has been deleted, we can't run these callbacks without errors, @@ -251,32 +252,7 @@ export class TreeMonitor { await this.manager.startExchange(historyEntryId, sharedModelChangesExchangeId, "recordAction.sharedModelChanges"); - // Recursion: handleSharedModelChanges is an action on the - // tree, and we are currently in a middleware that is - // monitoring actions on that tree. At this point in the - // middleware we are finishing a different action. Calling - // handleSharedModelChanges starts a new top level action: - // an action with no parent actions. This is what we want so - // we can record any changes made to the tree as part of the - // undo entry. I don't know if calling an action from a - // middleware is an officially supported or tested approach. - // It is working now. If it stops working we could delay the - // call to handleSharedModelChanges with a setTimeout. - // - // It is recursive because we will end up back in this - // recordAction function. Because we are awaiting - // handleSharedModelChanges that second recursive - // recordAction will get kicked off before this call to - // handleSharedModelChanges returns. The tree's - // implementation of handleSharedModelChanges should not - // modify the shared model itself or we could get into an - // infinite loop. - // - // Within this recursive call to recordAction, - // addTreePatchRecord will be called. This is how the - // startExchange above is closed out. - await this.tree.handleSharedModelChanges(historyEntryId, sharedModelChangesExchangeId, - call, sharedModelPath, initialSharedModelMap); + exchangesToProcess.push({ id: sharedModelChangesExchangeId, path: sharedModelPath }); } } @@ -301,5 +277,36 @@ export class TreeMonitor { inversePatches, }; this.manager.addTreePatchRecord(historyEntryId, exchangeId, record); + + for (const exchange of exchangesToProcess) { + // Now that the patches have been recorded, we can process the shared model changes + // Recursion: handleSharedModelChanges is an action on the + // tree, and we are currently in a middleware that is + // monitoring actions on that tree. At this point in the + // middleware we are finishing a different action. Calling + // handleSharedModelChanges starts a new top level action: + // an action with no parent actions. This is what we want so + // we can record any changes made to the tree as part of the + // undo entry. I don't know if calling an action from a + // middleware is an officially supported or tested approach. + // It is working now. If it stops working we could delay the + // call to handleSharedModelChanges with a setTimeout. + // + // It is recursive because we will end up back in this + // recordAction function. Because we are awaiting + // handleSharedModelChanges that second recursive + // recordAction will get kicked off before this call to + // handleSharedModelChanges returns. The tree's + // implementation of handleSharedModelChanges should not + // modify the shared model itself or we could get into an + // infinite loop. + // + // Within this recursive call to recordAction, + // addTreePatchRecord will be called. This is how the + // startExchange above is closed out. + await this.tree.handleSharedModelChanges(historyEntryId, exchange.id, + call, exchange.path, initialSharedModelMap); + } + } } diff --git a/src/models/history/undo-store.test.ts b/src/models/history/undo-store.test.ts index e73b3654e3..6d70119e06 100644 --- a/src/models/history/undo-store.test.ts +++ b/src/models/history/undo-store.test.ts @@ -610,21 +610,21 @@ const initialSharedModelUpdateEntry = { created: expect.any(Number), id: expect.any(String), records: [ - { action: "/handleSharedModelChanges", + { action: "/content/sharedModelMap/sm1/sharedModel/setValue", inversePatches: [ - { op: "replace", path: "/content/tileMap/t1/content/text", value: undefined} + { op: "replace", path: "/content/sharedModelMap/sm1/sharedModel/value", value: undefined} ], patches: [ - { op: "replace", path: "/content/tileMap/t1/content/text", value: "something-tile"} + { op: "replace", path: "/content/sharedModelMap/sm1/sharedModel/value", value: "something"} ], tree: "test" }, - { action: "/content/sharedModelMap/sm1/sharedModel/setValue", + { action: "/handleSharedModelChanges", inversePatches: [ - { op: "replace", path: "/content/sharedModelMap/sm1/sharedModel/value", value: undefined} + { op: "replace", path: "/content/tileMap/t1/content/text", value: undefined} ], patches: [ - { op: "replace", path: "/content/sharedModelMap/sm1/sharedModel/value", value: "something"} + { op: "replace", path: "/content/tileMap/t1/content/text", value: "something-tile"} ], tree: "test" } @@ -710,19 +710,19 @@ const redoSharedModelEntry = { records: [ { action: "/applyPatchesFromManager", inversePatches: [ - { op: "replace", path: "/content/tileMap/t1/content/text", value: undefined} + { op: "replace", path: "/content/sharedModelMap/sm1/sharedModel/value", value: undefined} ], patches: [ - { op: "replace", path: "/content/tileMap/t1/content/text", value: "something-tile"} + { op: "replace", path: "/content/sharedModelMap/sm1/sharedModel/value", value: "something"} ], tree: "test" }, { action: "/applyPatchesFromManager", inversePatches: [ - { op: "replace", path: "/content/sharedModelMap/sm1/sharedModel/value", value: undefined} + { op: "replace", path: "/content/tileMap/t1/content/text", value: undefined} ], patches: [ - { op: "replace", path: "/content/sharedModelMap/sm1/sharedModel/value", value: "something"} + { op: "replace", path: "/content/tileMap/t1/content/text", value: "something-tile"} ], tree: "test" } @@ -821,24 +821,6 @@ it("can track the addition of a new shared model", async () => { created: expect.any(Number), id: expect.any(String), records: [ - { - tree: "test", - action: "/handleSharedModelChanges", - patches: [ - { - op: "replace", - path: "/content/tileMap/t1/content/text", - value: "new model-tile" - } - ], - inversePatches: [ - { - op: "replace", - path: "/content/tileMap/t1/content/text", - value: undefined - } - ] - }, { action: "/content/addSharedModel", inversePatches: [ @@ -859,6 +841,24 @@ it("can track the addition of a new shared model", async () => { } ], tree: "test" + }, + { + tree: "test", + action: "/handleSharedModelChanges", + patches: [ + { + op: "replace", + path: "/content/tileMap/t1/content/text", + value: "new model-tile" + } + ], + inversePatches: [ + { + op: "replace", + path: "/content/tileMap/t1/content/text", + value: undefined + } + ] } ], state: "complete", diff --git a/src/models/history/undo-store.ts b/src/models/history/undo-store.ts index 5abc122133..ebfd911772 100644 --- a/src/models/history/undo-store.ts +++ b/src/models/history/undo-store.ts @@ -65,16 +65,20 @@ export const UndoStore = types const uniqueTreeIds = [...new Set(treeIds)]; // first disable shared model syncing in each tree + // Order of calls does not matter for this operation. const startPromises = uniqueTreeIds.map(treeId => { const startExchangeId = nanoid(); manager.startExchange(historyEntryId, startExchangeId, "UndoStore.applyPatchesToTrees.start"); - return manager.trees[treeId].startApplyingPatchesFromManager(historyEntryId, startExchangeId); }); yield Promise.all(startPromises); - // apply the patches to all trees - const applyPromises = treePatchRecords.map(treePatchRecord => { + // apply the patches to all trees, in reverse order if we are undoing changes. + const undoRecords = [ ...treePatchRecords ]; + if (opType === HistoryOperation.Undo) { + undoRecords.reverse(); + } + for (const treePatchRecord of undoRecords) { // console.log(`send tile entry to ${opType} to the tree`, getSnapshot(treeEntry)); // If there are multiple trees, and a patch is applied to shared model @@ -91,10 +95,9 @@ export const UndoStore = types manager.startExchange(historyEntryId, applyExchangeId, "UndoStore.applyPatchesToTrees.apply"); const tree = manager.trees[treePatchRecord.tree]; - return tree.applyPatchesFromManager(historyEntryId, applyExchangeId, + yield tree.applyPatchesFromManager(historyEntryId, applyExchangeId, treePatchRecord.getPatches(opType)); - }); - yield Promise.all(applyPromises); + } // finish the patch application // diff --git a/src/plugins/data-card/components/case-attribute.tsx b/src/plugins/data-card/components/case-attribute.tsx index 1d245f9f46..2008c3517c 100644 --- a/src/plugins/data-card/components/case-attribute.tsx +++ b/src/plugins/data-card/components/case-attribute.tsx @@ -1,9 +1,11 @@ import React, { useCallback, useEffect, useState } from "react"; import { observer } from "mobx-react"; +import { isAlive } from "mobx-state-tree"; import escapeStringRegexp from "escape-string-regexp"; import { useCombobox } from "downshift"; import { uniq } from "lodash"; import { VisuallyHidden } from "@chakra-ui/react"; +import classNames from "classnames"; import { gImageMap } from "../../../models/image-map"; import { ITileModel } from "../../../models/tiles/tile-model"; import { DataCardContentModelType } from "../data-card-content"; @@ -25,7 +27,6 @@ import NumberTypeIcon from "../assets/id-type-number.svg"; import ExpandDownIcon from "../assets/expand-more-icon.svg"; import './single-card-data-area.scss'; -import classNames from "classnames"; const typeIcons = { "date": , @@ -57,13 +58,16 @@ export const CaseAttribute: React.FC = observer(props => { model, caseId, attrKey, currEditAttrId, currEditFacet, setCurrEditFacet, setCurrEditAttrId, readOnly } = props; + if (!isAlive(model)) { + console.log("rendering unalive model", model); + } const content = model.content as DataCardContentModelType; const dataSet = content.dataSet; const cell = { attributeId: attrKey, caseId: caseId ?? "" }; const isLinked = useIsLinked(); const getName = useCallback(() => { - return content.dataSet.attrFromID(attrKey).name; + return content.dataSet.attrFromID(attrKey)?.name; }, [attrKey, content.dataSet]); const getValue = useCallback(() => { @@ -92,7 +96,9 @@ export const CaseAttribute: React.FC = observer(props => { }, [editingValue, valueCandidate.length]); useEffect(() => { - const nameLines = measureTextLines(getName(), kFieldWidthFactor); + const name = getName(); + if (!name) return; + const nameLines = measureTextLines(name, kFieldWidthFactor); const nameCandidateLines = measureTextLines(nameCandidate, kFieldWidthFactor); const nameLinesNeeded = Math.max(nameLines, nameCandidateLines); const valueLines = measureTextLines(valueStr, kFieldWidthFactor); diff --git a/src/plugins/data-card/components/data-card-toolbar-buttons.tsx b/src/plugins/data-card/components/data-card-toolbar-buttons.tsx index 7a71569335..fbd89871e7 100644 --- a/src/plugins/data-card/components/data-card-toolbar-buttons.tsx +++ b/src/plugins/data-card/components/data-card-toolbar-buttons.tsx @@ -53,7 +53,7 @@ export const DeleteAttrButton = () => { const isEditingValue = !!context?.currEditAttrId && context?.currEditFacet === "value"; const handleClick = () => { - const thisCaseId = content?.dataSet.caseIDFromIndex(content.caseIndex); + const thisCaseId = content?.dataSet.caseIDFromIndex(content.caseIndexNumber); if (thisCaseId && context){ content?.setAttValue(thisCaseId, context.currEditAttrId, ""); } diff --git a/src/plugins/data-card/data-card-content.ts b/src/plugins/data-card/data-card-content.ts index 58af9ad881..bdc1a38916 100644 --- a/src/plugins/data-card/data-card-content.ts +++ b/src/plugins/data-card/data-card-content.ts @@ -36,7 +36,7 @@ export const DataCardContentModel = TileContentModel .named("DataCardTool") .props({ type: types.optional(types.literal(kDataCardTileType), kDataCardTileType), - caseIndex: 0, + caseIndex: types.maybe(types.number), selectedSortAttributeId: types.maybe(types.string) }) .volatile(self => ({ @@ -45,6 +45,9 @@ export const DataCardContentModel = TileContentModel emptyDataSet: DataSet.create() })) .views(self => ({ + get caseIndexNumber() { + return self.caseIndex || 0; + }, get sharedModel() { const sharedModelManager = self.tileEnv?.sharedModelManager; // Perhaps we should pass the type to getTileSharedModel, so it can return the right value @@ -132,7 +135,7 @@ export const DataCardContentModel = TileContentModel })) .views(self => ({ get caseId() { - return self.dataSet.caseIDFromIndex(self.caseIndex); + return self.caseIndex ? self.dataSet.caseIDFromIndex(self.caseIndex) : undefined; } })) .views(self => ({ @@ -207,11 +210,6 @@ export const DataCardContentModel = TileContentModel }, {name: "sharedModelSetup", fireImmediately: true})); }, - updateAfterSharedModelChanges(sharedModel?: SharedModelType) { - if (self.caseIndex >= self.totalCases && self.totalCases > 0) { - this.setCaseIndex(self.totalCases - 1); - } - }, setCaseIndex(caseIndex: number) { // current case is serialized, but navigation is not undoable withoutUndo(); @@ -259,8 +257,23 @@ export const DataCardContentModel = TileContentModel } })) .actions(self => ({ + updateAfterSharedModelChanges(sharedModel?: SharedModelType) { + const dataSet = self.dataSet; + if (!dataSet) return; + // Select the card of the first selected case + const selectedCaseId = dataSet.firstSelectedCaseId + ? dataSet.firstSelectedCaseId : dataSet.firstSelectedCell?.caseId; + if (selectedCaseId && dataSet.caseIndexFromID(selectedCaseId) !== self.caseIndex) { + self.setCaseIndex(dataSet.caseIndexFromID(selectedCaseId)); + } else if (self.caseIndex === undefined) { + self.setCaseIndex(0); + } else if (self.caseIndex >= self.totalCases && self.totalCases > 0) { + // Make sure case index is in range if number of cases has changed + self.setCaseIndex(self.totalCases - 1); + } + }, duplicateCard() { - const originalCaseIndex = self.caseIndex; + const originalCaseIndex = self.caseIndexNumber; const copyableCase = self.caseByIndex(originalCaseIndex); if (copyableCase) { // strip __id__ so a new id will be generated on insertion diff --git a/src/plugins/data-card/data-card-tile.tsx b/src/plugins/data-card/data-card-tile.tsx index c07afb1c92..12f058307e 100644 --- a/src/plugins/data-card/data-card-tile.tsx +++ b/src/plugins/data-card/data-card-tile.tsx @@ -1,6 +1,7 @@ import classNames from "classnames"; import { observer } from "mobx-react"; -import React, { useEffect, useRef, useState } from "react"; +import { isAlive } from "mobx-state-tree"; +import React, { useRef, useState, useEffect } from "react"; import { ITileProps } from "../../components/tiles/tile-component"; import { useUIStore } from "../../hooks/use-stores"; import { DataCardContentModelType } from "./data-card-content"; @@ -23,6 +24,11 @@ import "./data-card-tile.scss"; export const DataCardToolComponent: React.FC = observer(function DataCardToolComponent(props) { const { documentId, model, readOnly, documentContent, tileElt, onRegisterTileApi, scale, onUnregisterTileApi, height, onRequestRowHeight } = props; + // Doing this check lets mobx know that it shouldn't try to render a model that has been deleted + if (!isAlive(model)) { + console.log("rendering unalive model", model); + } + const backgroundRef = useRef(null); const content = model.content as DataCardContentModelType; @@ -41,12 +47,13 @@ export const DataCardToolComponent: React.FC = observer(function Dat const displaySingle = !content.selectedSortAttributeId; const shouldShowAddField = !readOnly && isTileSelected && displaySingle; const attrIdsNames = content.existingAttributesWithNames(); - const cardOf = `Card ${content.caseIndex + 1 } of `; + const cardOf = `Card ${content.caseIndexNumber + 1 } of `; // When a highlighted case or cell is set, show it const selectedCaseId = dataSet.firstSelectedCaseId ? dataSet.firstSelectedCaseId : dataSet.firstSelectedCell?.caseId; useEffect(() => { - if (selectedCaseId) { + if (content.caseIndex === undefined) return; + if (selectedCaseId && dataSet.caseIndexFromID(selectedCaseId) !== content.caseIndex) { content.setCaseIndex(dataSet.caseIndexFromID(selectedCaseId)); } }, [content, dataSet, selectedCaseId]); @@ -64,8 +71,8 @@ export const DataCardToolComponent: React.FC = observer(function Dat }); function nextCase() { - if (content.caseIndex < content.totalCases - 1) { - content.setCaseIndex(content.caseIndex + 1); + if (content.caseIndexNumber < content.totalCases - 1) { + content.setCaseIndex(content.caseIndexNumber + 1); } } @@ -75,8 +82,8 @@ export const DataCardToolComponent: React.FC = observer(function Dat }; function previousCase() { - if (content.caseIndex > 0){ - content.setCaseIndex(content.caseIndex - 1); + if (content.caseIndexNumber > 0){ + content.setCaseIndex(content.caseIndexNumber - 1); } } @@ -134,11 +141,11 @@ export const DataCardToolComponent: React.FC = observer(function Dat const previousButtonClasses = classNames( "card-nav", "previous", - content.caseIndex > 0 ? "active" : "disabled", + content.caseIndexNumber > 0 ? "active" : "disabled", ); const nextButtonClasses = classNames( "card-nav", "next", - content.caseIndex < content.totalCases - 1 ? "active" : "disabled", + content.caseIndexNumber < content.totalCases - 1 ? "active" : "disabled", ); const addCardClasses = classNames("add-card", "teal-bg", { hidden: !shouldShowAddCase }); const removeCardClasses = classNames("remove-card", { hidden: !shouldShowDeleteCase }); @@ -234,7 +241,7 @@ export const DataCardToolComponent: React.FC = observer(function Dat
{ content.totalCases > 0 && = observer(function DataCardToolb onIsEnabled, setImageUrlToAdd, ...others }: IProps) { const content = model.content as DataCardContentModelType; - const { caseIndex, dataSet, totalCases } = content; + const { caseIndexNumber: caseIndex, dataSet, totalCases } = content; const currentCaseId = caseIndex >= 0 && caseIndex < totalCases ? dataSet.caseIDFromIndex(caseIndex) : undefined ; const enabled = onIsEnabled(); //"enabled" is the visibility of the toolbar at lower left const location = useFloatingToolbarLocation({ From c9a5a943b6ac384f1f3a1247ca3804ab1cb8f376 Mon Sep 17 00:00:00 2001 From: Boris Goldowsky Date: Thu, 19 Sep 2024 16:23:02 -0400 Subject: [PATCH 2/3] Don't do init steps on all dataset changes. Take more care with undefined vs. falsy --- src/plugins/data-card/data-card-content.ts | 19 +++++++++++-------- src/plugins/data-card/data-card-tile.tsx | 5 +++-- 2 files changed, 14 insertions(+), 10 deletions(-) diff --git a/src/plugins/data-card/data-card-content.ts b/src/plugins/data-card/data-card-content.ts index bdc1a38916..8e30543350 100644 --- a/src/plugins/data-card/data-card-content.ts +++ b/src/plugins/data-card/data-card-content.ts @@ -135,7 +135,7 @@ export const DataCardContentModel = TileContentModel })) .views(self => ({ get caseId() { - return self.caseIndex ? self.dataSet.caseIDFromIndex(self.caseIndex) : undefined; + return self.caseIndex !== undefined ? self.dataSet.caseIDFromIndex(self.caseIndex) : undefined; } })) .views(self => ({ @@ -260,13 +260,16 @@ export const DataCardContentModel = TileContentModel updateAfterSharedModelChanges(sharedModel?: SharedModelType) { const dataSet = self.dataSet; if (!dataSet) return; - // Select the card of the first selected case - const selectedCaseId = dataSet.firstSelectedCaseId - ? dataSet.firstSelectedCaseId : dataSet.firstSelectedCell?.caseId; - if (selectedCaseId && dataSet.caseIndexFromID(selectedCaseId) !== self.caseIndex) { - self.setCaseIndex(dataSet.caseIndexFromID(selectedCaseId)); - } else if (self.caseIndex === undefined) { - self.setCaseIndex(0); + // At initialization, caseIndex will be undefined. + // Set it to the first selected case, or the first card if there's no selection. + if (self.caseIndex === undefined) { + const selectedCaseId = dataSet.firstSelectedCaseId !== undefined + ? dataSet.firstSelectedCaseId : dataSet.firstSelectedCell?.caseId; + if (selectedCaseId !== undefined) { + self.setCaseIndex(dataSet.caseIndexFromID(selectedCaseId)); + } else { + self.setCaseIndex(0); + } } else if (self.caseIndex >= self.totalCases && self.totalCases > 0) { // Make sure case index is in range if number of cases has changed self.setCaseIndex(self.totalCases - 1); diff --git a/src/plugins/data-card/data-card-tile.tsx b/src/plugins/data-card/data-card-tile.tsx index 12f058307e..9d63c3cc36 100644 --- a/src/plugins/data-card/data-card-tile.tsx +++ b/src/plugins/data-card/data-card-tile.tsx @@ -50,10 +50,11 @@ export const DataCardToolComponent: React.FC = observer(function Dat const cardOf = `Card ${content.caseIndexNumber + 1 } of `; // When a highlighted case or cell is set, show it - const selectedCaseId = dataSet.firstSelectedCaseId ? dataSet.firstSelectedCaseId : dataSet.firstSelectedCell?.caseId; + const selectedCaseId = dataSet.firstSelectedCaseId !== undefined + ? dataSet.firstSelectedCaseId : dataSet.firstSelectedCell?.caseId; useEffect(() => { if (content.caseIndex === undefined) return; - if (selectedCaseId && dataSet.caseIndexFromID(selectedCaseId) !== content.caseIndex) { + if (selectedCaseId !== undefined && dataSet.caseIndexFromID(selectedCaseId) !== content.caseIndex) { content.setCaseIndex(dataSet.caseIndexFromID(selectedCaseId)); } }, [content, dataSet, selectedCaseId]); From 5ad11671dbc16b460a19ef569659e570060e7b7d Mon Sep 17 00:00:00 2001 From: Boris Goldowsky Date: Fri, 20 Sep 2024 12:12:06 -0400 Subject: [PATCH 3/3] Add comment. --- src/plugins/data-card/data-card-tile.tsx | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/plugins/data-card/data-card-tile.tsx b/src/plugins/data-card/data-card-tile.tsx index 9d63c3cc36..da1697ab25 100644 --- a/src/plugins/data-card/data-card-tile.tsx +++ b/src/plugins/data-card/data-card-tile.tsx @@ -53,6 +53,10 @@ export const DataCardToolComponent: React.FC = observer(function Dat const selectedCaseId = dataSet.firstSelectedCaseId !== undefined ? dataSet.firstSelectedCaseId : dataSet.firstSelectedCell?.caseId; useEffect(() => { + // caseIndex is undefined when the tile is first created. + // The initial setting of this field must be handled `updateAfterSharedModelChanges`, + // so that it is part of the same history entry as the tile creation. Setting it here would mean + // creating a second history entry that might be out of order. if (content.caseIndex === undefined) return; if (selectedCaseId !== undefined && dataSet.caseIndexFromID(selectedCaseId) !== content.caseIndex) { content.setCaseIndex(dataSet.caseIndexFromID(selectedCaseId));