From 8189e95d573682fb0ab881e6f69a70026b15f248 Mon Sep 17 00:00:00 2001 From: Kirk Swenson Date: Fri, 13 Feb 2026 09:39:00 -0800 Subject: [PATCH 1/7] Add plugin undo/redo analysis document V2 implementation analysis and V3 implementation plan for the undoChangeNotice plugin API (CODAP-1118). Co-Authored-By: Claude Opus 4.6 --- v3/doc/plugin-undo-redo.md | 159 +++++++++++++++++++++++++++++++++++++ 1 file changed, 159 insertions(+) create mode 100644 v3/doc/plugin-undo-redo.md diff --git a/v3/doc/plugin-undo-redo.md b/v3/doc/plugin-undo-redo.md new file mode 100644 index 0000000000..f79c375480 --- /dev/null +++ b/v3/doc/plugin-undo-redo.md @@ -0,0 +1,159 @@ +# Plugin Undo/Redo: V2 Analysis & V3 Implementation Plan + +## V2 Plugin Undo/Redo: How It Works + +### Overview + +V2's undo system is built around `DG.Command` objects pushed onto `DG.UndoHistory`'s undo/redo stacks. Each command has `execute()`, `undo()`, and `redo()` methods. For plugin undo/redo, CODAP acts as a **relay** — it doesn't know what the plugin did, it just records that *something undoable happened* and stores callbacks that tell the plugin to undo/redo its own state. + +### The Protocol + +**Plugin → CODAP (3 operations via `notify` on `undoChangeNotice`):** + +| Operation | Meaning | +|---|---| +| `undoableActionPerformed` | "I just did something undoable" (with optional `logMessage`) | +| `undoButtonPress` | "The user clicked my undo button — please undo" | +| `redoButtonPress` | "The user clicked my redo button — please redo" | + +**CODAP → Plugin (sent during undo/redo execution):** + +| Operation | Meaning | +|---|---| +| `undoAction` | "Please undo your last action" | +| `redoAction` | "Please redo your last undone action" | +| `clearUndo` | "The undo stack was cleared" (with `canUndo`/`canRedo`) | +| `clearRedo` | "The redo stack was cleared" (with `canUndo`/`canRedo`) | + +Every response to `undoChangeNotice` includes `{ canUndo, canRedo }` so the plugin can update its own UI. + +### The Core Mechanism + +When a plugin sends `undoableActionPerformed`, CODAP's `registerUndoChangeNotice` creates a `DG.Command` and pushes it onto the undo stack: + +- **`execute()`**: Empty — the plugin already performed the action +- **`undo()`**: Sends `{operation: "undoAction"}` back to the originating plugin via iframe-phone +- **`redo()`**: Sends `{operation: "redoAction"}` back to the originating plugin via iframe-phone + +The plugin must respond with `{success: true}` or `{success: false}`. On failure, CODAP shows an error alert. + +### Key Files (V2) + +- `apps/dg/models/command.js` — `DG.Command` template +- `apps/dg/controllers/undo_history.js` — `DG.UndoHistory` singleton (stacks, execute/undo/redo, notifications) +- `apps/dg/components/data_interactive/data_interactive_phone_handler.js` — `handleUndoChangeNotice` (lines 2298–2356), including `registerUndoChangeNotice` +- `apps/dg/components/game/game_controller.js` — `DG.sendCommandToDI` broadcasts to all plugins + +### Known V2 Limitations + +1. **No async support**: Undo does not support asynchronous changes. If execute/undo/redo trigger async code that dirties the document, the undo stacks are cleared. +2. **Plugin removal**: If a plugin component is removed and re-added via undo, calling undo/redo on that component will likely fail because the plugin's undo stack would have been cleared. +3. **Fire-and-forget with error reporting**: If the plugin fails to undo/redo, an error alert is shown, but the undo history state has already been modified. There is no rollback mechanism. + +--- + +## V3 Current State + +### What Exists + +- **Full undo/redo infrastructure** via `TreeManager`/`UndoStore`/`HistoryEntry` using MST JSON patches +- **`withCustomUndoRedo`** mechanism for registering non-patch-based undo/redo (used for things like sort) +- **`undoChangeNotice` handler is registered but stubbed** — returns `"not implemented (yet)"` +- **`externalUndoAvailable`** is already reported to plugins in `interactiveFrame` GET responses +- **Translation strings** for `DG.Undo/Redo.interactiveUndoableAction` exist in all locales +- **`broadcastMessage`** infrastructure works for sending messages to specific plugins via `targetTileId` + +### What's Missing + +1. **The `undoChangeNotice` handler implementation** — all three operations are stubbed +2. **Custom undo/redo entry creation** for plugin actions +3. **Sending `undoAction`/`redoAction` messages back** to the originating plugin during undo/redo +4. **Proactive `clearUndo`/`clearRedo` notifications** when the undo stack changes +5. **Returning `canUndo`/`canRedo`** in handler responses + +### Key V3 Files + +| File | Role | +|---|---| +| `src/data-interactive/handlers/undo-change-notice-handler.ts` | Stubbed handler | +| `src/models/history/tree-manager.ts` | Central undo/redo coordinator | +| `src/models/history/undo-store.ts` | Undo/redo stack management | +| `src/models/history/with-custom-undo-redo.ts` | Custom (non-patch) undo/redo registration | +| `src/models/history/apply-model-change.ts` | `applyModelChange` action | +| `src/models/document/document.ts` | `canUndo`/`canRedo` views, `undoLastAction`/`redoLastAction` | +| `src/components/web-view/web-view-model.ts` | `broadcastMessage` to plugin iframe | +| `src/models/document/document-content.ts` | `broadcastMessage` across all tiles | +| `src/data-interactive/handlers/interactive-frame-handler.ts` | Reports `externalUndoAvailable` | + +--- + +## V3 Implementation Plan + +### Phase 1: Core `undoChangeNotice` Handler + +#### 1a. Implement `undoButtonPress` / `redoButtonPress` (simplest, no custom undo needed) + +In `src/data-interactive/handlers/undo-change-notice-handler.ts`: + +- Access the document model (via `appState.document` or through the resources) +- For `undoButtonPress`: call `document.undoLastAction()` +- For `redoButtonPress`: call `document.redoLastAction()` +- Return `{ success: true, values: { canUndo, canRedo } }` + +#### 1b. Implement `undoableActionPerformed` (the main feature) + +This is the most complex piece. When a plugin says it did something undoable: + +1. Identify the originating plugin's tile ID from `resources.interactiveFrame` +2. Use `withCustomUndoRedo` (or a similar mechanism) to register a history entry with: + - **Custom undo handler**: Uses `broadcastMessage` with `targetTileId` to send `{action: 'notify', resource: 'undoChangeNotice', values: {operation: 'undoAction'}}` to the originating plugin + - **Custom redo handler**: Same but with `{operation: 'redoAction'}` +3. The entry should use the existing `DG.Undo.interactiveUndoableAction` / `DG.Redo.interactiveUndoableAction` strings +4. Return `{ success: true, values: { canUndo, canRedo } }` + +**Key challenge**: `withCustomUndoRedo` currently works within the context of an `applyModelChange` call. Plugin undo entries don't modify MST state — they're purely side-effect-based (send a message to an iframe). The implementation needs to either: + +- Create a no-op `applyModelChange` that produces no patches but registers custom undo/redo, OR +- Extend the history system to support purely custom entries + +#### 1c. Wire up response values + +All three operations should return `{ canUndo, canRedo }` from the document model. + +### Phase 2: Proactive Notifications + +#### 2a. Observe undo/redo state changes + +Add a MobX reaction (or similar) that watches `document.canUndo` and `document.canRedo`. When either changes (particularly when stacks are cleared), broadcast `{action: 'notify', resource: 'undoChangeNotice', values: {operation: 'clearUndo'|'clearRedo', canUndo, canRedo}}` to all plugins. + +This could be implemented: + +- In the `DocumentModel` or `TreeManager` as a side effect +- Or as a standalone observer set up during document initialization + +### Phase 3: Testing & Edge Cases + +- **Plugin removed and re-added**: If the plugin tile is destroyed, undo entries referencing it should handle the missing tile gracefully (show error or skip) +- **Multiple plugins**: Each plugin's undo entries should only send messages to the originating plugin, not all plugins +- **Async concerns**: Plugin responses to `undoAction`/`redoAction` are async (callback-based). Need to handle failures gracefully +- **Plugin response handling**: If a plugin responds with `{success: false}`, show an error notification (matching V2 behavior) + +### Files to Modify + +| File | Changes | +|---|---| +| `src/data-interactive/handlers/undo-change-notice-handler.ts` | Full handler implementation | +| `src/models/history/tree-manager.ts` or `undo-store.ts` | May need extension for purely custom entries | +| `src/models/document/document.ts` | Access to `canUndo`/`canRedo` for responses | +| `src/models/history/with-custom-undo-redo.ts` | May need adaptation for no-MST-patch entries | +| New: observer for undo state change notifications | Proactive `clearUndo`/`clearRedo` broadcasts | + +--- + +## Open Questions + +1. **Should plugin-triggered data mutations (create/update/delete cases via API) also be undoable?** Currently most handler actions call `applyModelChange` without undo strings. This is a separate concern from the `undoChangeNotice` mechanism but related to the overall plugin undo story. + +2. **Should `withCustomUndoRedo` be extended, or should a new mechanism be created** for purely side-effect-based undo entries that produce no MST patches? + +3. **How should the proactive notifications be scoped?** V2 broadcasts `clearUndo`/`clearRedo` to all plugins. Should V3 do the same, or only to plugins that have registered undoable actions? From 8b6da577ea496759797c29fc8f83a9fe3f688852 Mon Sep 17 00:00:00 2001 From: Kirk Swenson Date: Fri, 13 Feb 2026 09:39:52 -0800 Subject: [PATCH 2/7] Require branch name approval before creation Co-Authored-By: Claude Opus 4.6 --- CLAUDE.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CLAUDE.md b/CLAUDE.md index 3a62671dbc..43ce46fe26 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -27,6 +27,7 @@ When the user says "Let's start work on CODAP-XXX" (or similar): - Example: `CODAP-1027-inbounds-url-param` - Use lowercase kebab-case for the description - Keep it concise but descriptive + - **Always propose the branch name and get user approval before creating it** 3. **Update Jira status**: Transition the story to "In Progress" From bd53818942ed54ca88b0f9943ecbc819de154925 Mon Sep 17 00:00:00 2001 From: Kirk Swenson Date: Fri, 13 Feb 2026 09:43:47 -0800 Subject: [PATCH 3/7] Distinguish draft vs non-draft PR creation Code PRs start as drafts; docs-only PRs skip draft status. Co-Authored-By: Claude Opus 4.6 --- CLAUDE.md | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 43ce46fe26..0a779b069f 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -35,14 +35,16 @@ When the user says "Let's start work on CODAP-XXX" (or similar): When the work is ready for initial CI validation: -1. **Create a draft PR**: +1. **Create a PR**: - Title format: `{JIRA-ID}: {description}` (e.g., `CODAP-1027: add inbounds URL parameter`) - Description should include: - Summary of the changes - Reference to the Jira story (e.g., `Fixes CODAP-1027` or link to the story) so Jira links it automatically - Apply the `v3` label + - **For PRs with code changes**: Create as a draft PR. Mark as "Ready for Review" only after CI passes. + - **For docs-only PRs** (no code changes): Create as a regular (non-draft) PR — no need for CI validation. -2. **Initial CI validation**: +2. **Initial CI validation** (code PRs only): - CI runs automatically on PRs (lint, build, limited Cypress tests) - Review CI results and fix any issues From 28da88133727eb158228c5b51835ac1b4eef4125 Mon Sep 17 00:00:00 2001 From: Kirk Swenson Date: Mon, 23 Feb 2026 11:26:35 -0800 Subject: [PATCH 4/7] Update plugin undo/redo analysis with revised conclusions Re-analyze V2 implementation in light of comparison with CODAP-1127 spec. Key findings: - V2's handleUndoChangeNotice is mode-agnostic (no standalone vs external branching) - V3 DI handlers already bypass undo (applyModelChange without undo strings routes through withoutUndo), matching V2's retainUndo behavior - V3 can use a single code path for both modes, same as V2 - Shadow counter approach (from CODAP-1127 spec) is not needed - Flag all-cases-handler undo strings as a bug Co-Authored-By: Claude Opus 4.6 --- v3/doc/plugin-undo-redo.md | 92 ++++++++++++++++++++++++++++++++++---- 1 file changed, 83 insertions(+), 9 deletions(-) diff --git a/v3/doc/plugin-undo-redo.md b/v3/doc/plugin-undo-redo.md index f79c375480..eb908310f7 100644 --- a/v3/doc/plugin-undo-redo.md +++ b/v3/doc/plugin-undo-redo.md @@ -37,12 +37,28 @@ When a plugin sends `undoableActionPerformed`, CODAP's `registerUndoChangeNotice The plugin must respond with `{success: true}` or `{success: false}`. On failure, CODAP shows an error alert. +### Standalone vs External Undo Modes + +V2 supports two undo modes, controlled by the `?standalone=` URL parameter: + +- **Standalone mode** (`?standalone=SageModeler`): CODAP reports `standaloneUndoModeAvailable: true` to the plugin. The CODAP toolbar is hidden. The plugin owns the undo UX — only the plugin triggers undo/redo via `undoButtonPress`/`redoButtonPress`. +- **External/embedded mode** (default): CODAP reports `externalUndoAvailable: true`. CODAP's toolbar is visible and its undo/redo buttons are functional. + +**Critically, `handleUndoChangeNotice` has no mode-specific branching.** The same code path handles both modes. This works because: + +1. **DI API mutations don't interfere with the undo stack.** Plugin-triggered data mutations (create/update/delete cases) go through `context.applyChange()` directly — they are NOT wrapped in `DG.Command` objects. The `shouldRetainUndo = true` flag on data context operations prevents `DG.UndoHistory.documentWasChanged()` from clearing the undo stacks. + +2. **The only undo entry is from `registerUndoChangeNotice`.** The `DG.Command` created for `undoableActionPerformed` has an empty `execute()` and message-sending callbacks for undo/redo. There are no competing entries from DI API mutations. + +3. **In standalone mode, CODAP's undo UI is inaccessible.** The toolbar height is 0 (`kToolbarHeight = DG.STANDALONE_MODE ? 0 : 66`), so the user cannot accidentally trigger CODAP undo from the UI. Whether plugin actions sit on CODAP's undo stack is invisible to the user. + ### Key Files (V2) - `apps/dg/models/command.js` — `DG.Command` template - `apps/dg/controllers/undo_history.js` — `DG.UndoHistory` singleton (stacks, execute/undo/redo, notifications) - `apps/dg/components/data_interactive/data_interactive_phone_handler.js` — `handleUndoChangeNotice` (lines 2298–2356), including `registerUndoChangeNotice` - `apps/dg/components/game/game_controller.js` — `DG.sendCommandToDI` broadcasts to all plugins +- `apps/dg/core.js` — `STANDALONE_MODE` and `isStandaloneComponent()` detection ### Known V2 Limitations @@ -58,15 +74,42 @@ The plugin must respond with `{success: true}` or `{success: false}`. On failure - **Full undo/redo infrastructure** via `TreeManager`/`UndoStore`/`HistoryEntry` using MST JSON patches - **`withCustomUndoRedo`** mechanism for registering non-patch-based undo/redo (used for things like sort) +- **`withoutUndo`** mechanism for applying changes without creating undo entries - **`undoChangeNotice` handler is registered but stubbed** — returns `"not implemented (yet)"` -- **`externalUndoAvailable`** is already reported to plugins in `interactiveFrame` GET responses +- **`externalUndoAvailable`** and **`standaloneUndoModeAvailable`** are already reported to plugins in `interactiveFrame` GET responses (driven by `uiState.standaloneMode`) - **Translation strings** for `DG.Undo/Redo.interactiveUndoableAction` exist in all locales - **`broadcastMessage`** infrastructure works for sending messages to specific plugins via `targetTileId` +### DI API Mutations Already Bypass Undo + +A critical finding for the implementation: V3 DI API handlers call `applyModelChange()` without providing `undoStringKey`/`redoStringKey`. When undo strings are omitted, `AppHistoryService.handleApplyModelChange()` automatically calls `withoutUndo()`: + +```typescript +// app-history-service.ts +if (undoStringKey != null && redoStringKey != null) { + withUndoRedoStrings(undoStringKey, redoStringKey) +} else { + withoutUndo({ noDirty }) +} +``` + +This means plugin-triggered data mutations (create cases, update cases, delete cases, etc.) do NOT create undo entries. The mutations are applied and persisted in the document history (for autosave), but they don't appear on the undo stack. This is functionally equivalent to V2's `retainUndo = true` behavior. + +**Note:** `all-cases-handler.ts` (delete all cases) is the sole exception — it passes undo strings. This appears to be a bug, inconsistent with the established pattern for DI handlers. It should be fixed to omit undo strings like every other DI handler. + +### Why a Single Code Path Works + +Because V3 DI API mutations already bypass undo (just like V2), and because CODAP's undo UI is hidden in standalone mode (just like V2), the V3 implementation does **not** need separate code paths for standalone vs external mode. The same handler logic works for both: + +- `undoableActionPerformed` creates an undo entry with plugin message callbacks +- `undoButtonPress` triggers undo (which pops the entry and messages the plugin) +- DI API mutations stay off the undo stack regardless of mode +- In standalone mode, the user never sees CODAP's undo stack, so plugin entries on it are harmless + ### What's Missing 1. **The `undoChangeNotice` handler implementation** — all three operations are stubbed -2. **Custom undo/redo entry creation** for plugin actions +2. **Custom undo/redo entry creation** for plugin actions (history entries with no MST patches, only custom callbacks) 3. **Sending `undoAction`/`redoAction` messages back** to the originating plugin during undo/redo 4. **Proactive `clearUndo`/`clearRedo` notifications** when the undo stack changes 5. **Returning `canUndo`/`canRedo`** in handler responses @@ -80,10 +123,12 @@ The plugin must respond with `{success: true}` or `{success: false}`. On failure | `src/models/history/undo-store.ts` | Undo/redo stack management | | `src/models/history/with-custom-undo-redo.ts` | Custom (non-patch) undo/redo registration | | `src/models/history/apply-model-change.ts` | `applyModelChange` action | +| `src/models/history/app-history-service.ts` | Routes `applyModelChange` to undo or `withoutUndo` based on undo strings | +| `src/models/history/without-undo.ts` | `withoutUndo` — suppresses undo recording | | `src/models/document/document.ts` | `canUndo`/`canRedo` views, `undoLastAction`/`redoLastAction` | | `src/components/web-view/web-view-model.ts` | `broadcastMessage` to plugin iframe | | `src/models/document/document-content.ts` | `broadcastMessage` across all tiles | -| `src/data-interactive/handlers/interactive-frame-handler.ts` | Reports `externalUndoAvailable` | +| `src/data-interactive/handlers/interactive-frame-handler.ts` | Reports `externalUndoAvailable` / `standaloneUndoModeAvailable` | --- @@ -100,9 +145,11 @@ In `src/data-interactive/handlers/undo-change-notice-handler.ts`: - For `redoButtonPress`: call `document.redoLastAction()` - Return `{ success: true, values: { canUndo, canRedo } }` +**Note on operation name:** The existing TODO in the handler references `undoButtonPressed` (with trailing "d"), but V2 and Building Models use `undoButtonPress` (no "d"). The implementation should accept the correct V2 name. + #### 1b. Implement `undoableActionPerformed` (the main feature) -This is the most complex piece. When a plugin says it did something undoable: +When a plugin says it did something undoable: 1. Identify the originating plugin's tile ID from `resources.interactiveFrame` 2. Use `withCustomUndoRedo` (or a similar mechanism) to register a history entry with: @@ -111,7 +158,7 @@ This is the most complex piece. When a plugin says it did something undoable: 3. The entry should use the existing `DG.Undo.interactiveUndoableAction` / `DG.Redo.interactiveUndoableAction` strings 4. Return `{ success: true, values: { canUndo, canRedo } }` -**Key challenge**: `withCustomUndoRedo` currently works within the context of an `applyModelChange` call. Plugin undo entries don't modify MST state — they're purely side-effect-based (send a message to an iframe). The implementation needs to either: +**Key implementation challenge**: `withCustomUndoRedo` currently works within the context of an `applyModelChange` call. Plugin undo entries don't modify MST state — they're purely side-effect-based (send a message to an iframe). The implementation needs to either: - Create a no-op `applyModelChange` that produces no patches but registers custom undo/redo, OR - Extend the history system to support purely custom entries @@ -138,6 +185,10 @@ This could be implemented: - **Async concerns**: Plugin responses to `undoAction`/`redoAction` are async (callback-based). Need to handle failures gracefully - **Plugin response handling**: If a plugin responds with `{success: false}`, show an error notification (matching V2 behavior) +### Phase 4: Bug Fix + +- **Remove undo strings from `all-cases-handler.ts`**: The `delete` operation passes `undoStringKey`/`redoStringKey`, making it the only DI handler that creates undo entries for plugin-triggered mutations. This is inconsistent with the established pattern and should be fixed. + ### Files to Modify | File | Changes | @@ -146,14 +197,37 @@ This could be implemented: | `src/models/history/tree-manager.ts` or `undo-store.ts` | May need extension for purely custom entries | | `src/models/document/document.ts` | Access to `canUndo`/`canRedo` for responses | | `src/models/history/with-custom-undo-redo.ts` | May need adaptation for no-MST-patch entries | +| `src/data-interactive/handlers/all-cases-handler.ts` | Remove undo strings from `delete` operation | | New: observer for undo state change notifications | Proactive `clearUndo`/`clearRedo` broadcasts | --- -## Open Questions +## Resolved Questions + +1. **Should plugin-triggered data mutations (create/update/delete cases via API) also be undoable?** **No.** V3 DI handlers already call `applyModelChange` without undo strings, which routes through `withoutUndo()`. This is the correct behavior — plugin-triggered mutations are one half of a logical action whose undo is managed by the plugin itself. Recording them separately would break the undo model. This matches V2's `retainUndo = true` behavior. + +2. **Does the implementation need separate code paths for standalone vs external mode?** **No.** V2 uses a single code path for both modes. V3 can do the same because: (a) DI API mutations already bypass undo in both modes, and (b) in standalone mode, CODAP's undo stack is hidden from the user, so plugin entries on it are harmless. The mode distinction only affects what flags the plugin receives (`standaloneUndoModeAvailable` vs `externalUndoAvailable`) and whether CODAP's toolbar is visible — neither is the handler's concern. + +## Considered Alternative: Shadow Counters + +An alternative approach was proposed in the CODAP-1127 spec (R3) for standalone mode: instead of creating CODAP undo entries, CODAP would maintain two integers (undo depth, redo depth) as "shadow counters." On `undoableActionPerformed`, increment the undo counter. On `undoButtonPress`, send `undoAction` directly to the plugin and adjust counters — no CODAP undo system involvement at all. -1. **Should plugin-triggered data mutations (create/update/delete cases via API) also be undoable?** Currently most handler actions call `applyModelChange` without undo strings. This is a separate concern from the `undoChangeNotice` mechanism but related to the overall plugin undo story. +This approach was motivated by two concerns: + +1. **DI API mutations would corrupt the undo stack** by creating interleaved undo entries alongside plugin entries. +2. **Plugin actions should not appear on CODAP's undo stack** in standalone mode. + +Both concerns turned out to be unfounded: + +1. V3 DI handlers already call `applyModelChange` without undo strings, which routes through `withoutUndo()`. Plugin-triggered mutations do not create undo entries. This is equivalent to V2's `retainUndo = true` behavior. +2. In standalone mode, CODAP's undo UI is hidden and inaccessible. Whether plugin entries sit on CODAP's undo stack has no user-visible effect. + +The shadow counter approach would also introduce a second code path (branching on standalone vs external mode) that V2 does not need. V2 uses a single mode-agnostic handler, and V3 can do the same. + +For these reasons, the recommended approach follows V2: use CODAP's undo system for both modes, with `withCustomUndoRedo` (or equivalent) to register plugin undo/redo callbacks. The shadow counter approach is not needed. + +## Open Questions -2. **Should `withCustomUndoRedo` be extended, or should a new mechanism be created** for purely side-effect-based undo entries that produce no MST patches? +1. **Should `withCustomUndoRedo` be extended, or should a new mechanism be created** for purely side-effect-based undo entries that produce no MST patches? -3. **How should the proactive notifications be scoped?** V2 broadcasts `clearUndo`/`clearRedo` to all plugins. Should V3 do the same, or only to plugins that have registered undoable actions? +2. **How should the proactive notifications be scoped?** V2 broadcasts `clearUndo`/`clearRedo` to all plugins. Should V3 do the same, or only to plugins that have registered undoable actions? From 1b4f29635f26a2c6361ec79aa3f5ab1e3ffc2072 Mon Sep 17 00:00:00 2001 From: Kirk Swenson Date: Mon, 23 Feb 2026 11:52:19 -0800 Subject: [PATCH 5/7] Address interleaving of user-triggered CODAP undo with plugin undo User-triggered CODAP actions (graph changes, table edits) create undo entries on the same stack as plugin entries. In standalone mode, this causes undoButtonPress to potentially undo the wrong action. Revise analysis to recommend shadow counters for standalone mode (keeping plugin undo separate from CODAP stack) while retaining CODAP undo entries for external mode (where interleaving is intentional). Co-Authored-By: Claude Opus 4.6 --- v3/doc/plugin-undo-redo.md | 81 +++++++++++++++++++++++++------------- 1 file changed, 53 insertions(+), 28 deletions(-) diff --git a/v3/doc/plugin-undo-redo.md b/v3/doc/plugin-undo-redo.md index eb908310f7..a1756381f4 100644 --- a/v3/doc/plugin-undo-redo.md +++ b/v3/doc/plugin-undo-redo.md @@ -97,14 +97,29 @@ This means plugin-triggered data mutations (create cases, update cases, delete c **Note:** `all-cases-handler.ts` (delete all cases) is the sole exception — it passes undo strings. This appears to be a bug, inconsistent with the established pattern for DI handlers. It should be fixed to omit undo strings like every other DI handler. -### Why a Single Code Path Works +### The Interleaving Problem in Standalone Mode -Because V3 DI API mutations already bypass undo (just like V2), and because CODAP's undo UI is hidden in standalone mode (just like V2), the V3 implementation does **not** need separate code paths for standalone vs external mode. The same handler logic works for both: +While DI API mutations don't create undo entries, **user-triggered CODAP actions do**. In standalone mode, the CODAP toolbar is hidden, but the user can still interact directly with CODAP tiles (graphs, tables, etc.). These interactions create normal undo entries with MST patches on CODAP's undo stack. -- `undoableActionPerformed` creates an undo entry with plugin message callbacks -- `undoButtonPress` triggers undo (which pops the entry and messages the plugin) -- DI API mutations stay off the undo stack regardless of mode -- In standalone mode, the user never sees CODAP's undo stack, so plugin entries on it are harmless +If plugin undo entries share CODAP's undo stack, they interleave with user-triggered entries. For example: + +1. Plugin performs an undoable action → plugin entry pushed to CODAP stack +2. User changes a graph setting → CODAP entry pushed to stack +3. Plugin sends `undoButtonPress` → `document.undoLastAction()` pops the **graph change**, not the plugin action + +The plugin never receives `undoAction` — the wrong thing gets undone. This interleaving problem exists in V2 as well (V2's `handleUndoChangeNotice` shares the same `DG.UndoHistory` stack), but it likely goes unnoticed in practice because SageModeler controls most of the UX and direct user manipulation of CODAP tiles is rare. + +For V3, the **shadow counter approach** solves this cleanly for standalone mode by keeping plugin undo tracking completely separate from CODAP's undo stack (see [Standalone vs External Mode: Two Code Paths](#standalone-vs-external-mode-two-code-paths) below). + +In **external/embedded mode**, interleaving is the correct behavior — plugin actions and CODAP actions share a unified undo stack, and the user interacts with undo through CODAP's toolbar. + +### Standalone vs External Mode: Two Code Paths + +Despite V2 using a single code path, V3 should use **two code paths** to correctly handle the interleaving problem: + +- **Standalone mode** (`uiState.standaloneMode`): Use shadow counters. Plugin undo entries are NOT placed on CODAP's undo stack. `undoButtonPress` sends `undoAction` directly to the plugin and adjusts counters. User-triggered CODAP actions remain on CODAP's stack independently. The two undo histories never interfere with each other. + +- **External/embedded mode**: Use CODAP's undo stack (the approach from the original analysis). `undoableActionPerformed` creates a custom undo entry on CODAP's stack. `undoButtonPress` calls `document.undoLastAction()`. Interleaving of plugin and CODAP entries is intentional in this mode — they share a unified undo experience. ### What's Missing @@ -136,20 +151,26 @@ Because V3 DI API mutations already bypass undo (just like V2), and because CODA ### Phase 1: Core `undoChangeNotice` Handler -#### 1a. Implement `undoButtonPress` / `redoButtonPress` (simplest, no custom undo needed) - -In `src/data-interactive/handlers/undo-change-notice-handler.ts`: +The handler should branch on the undo mode early: -- Access the document model (via `appState.document` or through the resources) -- For `undoButtonPress`: call `document.undoLastAction()` -- For `redoButtonPress`: call `document.redoLastAction()` -- Return `{ success: true, values: { canUndo, canRedo } }` +``` +if (standalone mode for this plugin's tile) → shadow counter path +else → CODAP undo entry path +``` **Note on operation name:** The existing TODO in the handler references `undoButtonPressed` (with trailing "d"), but V2 and Building Models use `undoButtonPress` (no "d"). The implementation should accept the correct V2 name. -#### 1b. Implement `undoableActionPerformed` (the main feature) +#### 1a. Standalone mode: Shadow counters + +Maintain per-plugin undo/redo depth counters (two integers per plugin tile). No CODAP undo stack involvement. + +- **`undoableActionPerformed`**: Increment undo counter, reset redo counter. Return `{ success: true, values: { canUndo: true, canRedo: false } }`. +- **`undoButtonPress`**: Send `{action: 'notify', resource: 'undoChangeNotice', values: {operation: 'undoAction'}}` directly to the originating plugin via `broadcastMessage` with `targetTileId`. Wait for the plugin's callback. Decrement undo counter, increment redo counter. Return updated `{ canUndo, canRedo }`. +- **`redoButtonPress`**: Same as above but with `{operation: 'redoAction'}`. Increment undo counter, decrement redo counter. -When a plugin says it did something undoable: +#### 1b. External mode: CODAP undo entries + +Use CODAP's undo system. When a plugin says it did something undoable: 1. Identify the originating plugin's tile ID from `resources.interactiveFrame` 2. Use `withCustomUndoRedo` (or a similar mechanism) to register a history entry with: @@ -158,6 +179,10 @@ When a plugin says it did something undoable: 3. The entry should use the existing `DG.Undo.interactiveUndoableAction` / `DG.Redo.interactiveUndoableAction` strings 4. Return `{ success: true, values: { canUndo, canRedo } }` +For `undoButtonPress`/`redoButtonPress`: +- Call `document.undoLastAction()` / `document.redoLastAction()` +- Return `{ success: true, values: { canUndo, canRedo } }` + **Key implementation challenge**: `withCustomUndoRedo` currently works within the context of an `applyModelChange` call. Plugin undo entries don't modify MST state — they're purely side-effect-based (send a message to an iframe). The implementation needs to either: - Create a no-op `applyModelChange` that produces no patches but registers custom undo/redo, OR @@ -165,7 +190,7 @@ When a plugin says it did something undoable: #### 1c. Wire up response values -All three operations should return `{ canUndo, canRedo }` from the document model. +All operations should return `{ canUndo, canRedo }`. In standalone mode, these come from the shadow counters. In external mode, they come from the document model. ### Phase 2: Proactive Notifications @@ -206,25 +231,25 @@ This could be implemented: 1. **Should plugin-triggered data mutations (create/update/delete cases via API) also be undoable?** **No.** V3 DI handlers already call `applyModelChange` without undo strings, which routes through `withoutUndo()`. This is the correct behavior — plugin-triggered mutations are one half of a logical action whose undo is managed by the plugin itself. Recording them separately would break the undo model. This matches V2's `retainUndo = true` behavior. -2. **Does the implementation need separate code paths for standalone vs external mode?** **No.** V2 uses a single code path for both modes. V3 can do the same because: (a) DI API mutations already bypass undo in both modes, and (b) in standalone mode, CODAP's undo stack is hidden from the user, so plugin entries on it are harmless. The mode distinction only affects what flags the plugin receives (`standaloneUndoModeAvailable` vs `externalUndoAvailable`) and whether CODAP's toolbar is visible — neither is the handler's concern. - -## Considered Alternative: Shadow Counters +2. **Does the implementation need separate code paths for standalone vs external mode?** **Yes.** Although V2 uses a single code path, that approach has an interleaving flaw: user-triggered CODAP actions (e.g., graph changes) create undo entries on the same stack as plugin entries. When the plugin sends `undoButtonPress`, `document.undoLastAction()` may pop a CODAP entry instead of the plugin's entry. V2 gets away with this because direct user manipulation of CODAP tiles is rare in SageModeler, but it's not architecturally correct. V3 should use shadow counters for standalone mode (keeping plugin undo separate from CODAP's stack) and CODAP undo entries for external mode (where interleaving is intentional). -An alternative approach was proposed in the CODAP-1127 spec (R3) for standalone mode: instead of creating CODAP undo entries, CODAP would maintain two integers (undo depth, redo depth) as "shadow counters." On `undoableActionPerformed`, increment the undo counter. On `undoButtonPress`, send `undoAction` directly to the plugin and adjust counters — no CODAP undo system involvement at all. +## Design Rationale: Why Shadow Counters for Standalone Mode -This approach was motivated by two concerns: +An earlier version of this analysis concluded that V3 could use a single code path for both modes, matching V2. That conclusion was revised after identifying the **interleaving problem**: in standalone mode, user-triggered CODAP actions (graph changes, table edits, etc.) create undo entries on the same stack as plugin entries. When the plugin sends `undoButtonPress`, `document.undoLastAction()` may pop a user-triggered CODAP entry instead of the plugin's entry, undoing the wrong thing. -1. **DI API mutations would corrupt the undo stack** by creating interleaved undo entries alongside plugin entries. -2. **Plugin actions should not appear on CODAP's undo stack** in standalone mode. +The shadow counter approach (from the CODAP-1127 spec) solves this by keeping plugin undo tracking completely separate from CODAP's undo stack: -Both concerns turned out to be unfounded: +- Plugin actions are tracked by per-plugin integer counters, not CODAP undo entries +- `undoButtonPress` sends `undoAction` directly to the plugin and adjusts counters — CODAP's undo stack is never touched +- User-triggered CODAP actions remain on CODAP's own stack independently +- The two undo histories cannot interfere with each other -1. V3 DI handlers already call `applyModelChange` without undo strings, which routes through `withoutUndo()`. Plugin-triggered mutations do not create undo entries. This is equivalent to V2's `retainUndo = true` behavior. -2. In standalone mode, CODAP's undo UI is hidden and inaccessible. Whether plugin entries sit on CODAP's undo stack has no user-visible effect. +Note that two earlier concerns about the shared-stack approach turned out to be unfounded and are **not** the reason for choosing shadow counters: -The shadow counter approach would also introduce a second code path (branching on standalone vs external mode) that V2 does not need. V2 uses a single mode-agnostic handler, and V3 can do the same. +1. ~~DI API mutations would corrupt the undo stack~~ — V3 DI handlers already call `applyModelChange` without undo strings, which routes through `withoutUndo()`. Plugin-triggered mutations do not create undo entries. +2. ~~Plugin actions should not appear on CODAP's undo stack~~ — In standalone mode, CODAP's undo UI is hidden. Plugin entries on the stack would be invisible to the user. -For these reasons, the recommended approach follows V2: use CODAP's undo system for both modes, with `withCustomUndoRedo` (or equivalent) to register plugin undo/redo callbacks. The shadow counter approach is not needed. +The actual reason is the interleaving of plugin entries with user-triggered CODAP entries on a shared stack. In **external mode**, this interleaving is correct and intentional (unified undo experience). In **standalone mode**, it causes the wrong action to be undone. ## Open Questions From 71848d72155c117f7a7d12116ca9d71e6d53cfb7 Mon Sep 17 00:00:00 2001 From: Kirk Swenson Date: Mon, 23 Feb 2026 14:34:45 -0800 Subject: [PATCH 6/7] Revise analysis: shared-stack interleaving is chronologically correct MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The interleaving of plugin and CODAP undo entries on a shared stack is actually correct behavior — the most recent action is undone first. Shift recommendation from shadow counters to single code path (shared stack) for both standalone and external modes, with shadow counters as a fallback only if plugin testing reveals issues. Co-Authored-By: Claude Opus 4.6 --- v3/doc/plugin-undo-redo.md | 133 ++++++++++++++++++++++++------------- 1 file changed, 85 insertions(+), 48 deletions(-) diff --git a/v3/doc/plugin-undo-redo.md b/v3/doc/plugin-undo-redo.md index a1756381f4..ea3c7f7fa1 100644 --- a/v3/doc/plugin-undo-redo.md +++ b/v3/doc/plugin-undo-redo.md @@ -97,29 +97,42 @@ This means plugin-triggered data mutations (create cases, update cases, delete c **Note:** `all-cases-handler.ts` (delete all cases) is the sole exception — it passes undo strings. This appears to be a bug, inconsistent with the established pattern for DI handlers. It should be fixed to omit undo strings like every other DI handler. -### The Interleaving Problem in Standalone Mode +### The Interleaving Question in Standalone Mode While DI API mutations don't create undo entries, **user-triggered CODAP actions do**. In standalone mode, the CODAP toolbar is hidden, but the user can still interact directly with CODAP tiles (graphs, tables, etc.). These interactions create normal undo entries with MST patches on CODAP's undo stack. -If plugin undo entries share CODAP's undo stack, they interleave with user-triggered entries. For example: +If plugin undo entries share CODAP's undo stack, they interleave with user-triggered entries. Consider this scenario where the user clicks undo in the plugin: -1. Plugin performs an undoable action → plugin entry pushed to CODAP stack -2. User changes a graph setting → CODAP entry pushed to stack -3. Plugin sends `undoButtonPress` → `document.undoLastAction()` pops the **graph change**, not the plugin action +1. Plugin does something → plugin entry pushed (stack: [P1]) +2. User changes a graph setting → CODAP entry pushed (stack: [P1, C1]) +3. Plugin does something → plugin entry pushed (stack: [P1, C1, P2]) +4. User clicks undo in plugin → `undoButtonPress` → pops P2, sends `undoAction` to plugin +5. User clicks undo again → `undoButtonPress` → pops C1, undoes graph change +6. User clicks undo again → `undoButtonPress` → pops P1, sends `undoAction` to plugin -The plugin never receives `undoAction` — the wrong thing gets undone. This interleaving problem exists in V2 as well (V2's `handleUndoChangeNotice` shares the same `DG.UndoHistory` stack), but it likely goes unnoticed in practice because SageModeler controls most of the UX and direct user manipulation of CODAP tiles is rare. +**This is chronologically correct undo behavior.** The most recent action gets undone first, regardless of whether it was a plugin or CODAP action. From the user's perspective, this is exactly right. -For V3, the **shadow counter approach** solves this cleanly for standalone mode by keeping plugin undo tracking completely separate from CODAP's undo stack (see [Standalone vs External Mode: Two Code Paths](#standalone-vs-external-mode-two-code-paths) below). +The shared stack approach works well for the user experience. The real concerns are about **plugin-side handling**: -In **external/embedded mode**, interleaving is the correct behavior — plugin actions and CODAP actions share a unified undo stack, and the user interacts with undo through CODAP's toolbar. +1. **Plugin internal state sync.** If the plugin maintains its own internal undo stack and eagerly pops from it every time it sends `undoButtonPress` (before knowing the result), it will pop a plugin entry at step 5 when CODAP actually undid a graph change. The plugin's internal state gets out of sync. This only matters if the plugin eagerly pops its own stack rather than waiting for `undoAction`. -### Standalone vs External Mode: Two Code Paths +2. **No `undoAction` callback for CODAP entries.** At step 5, the plugin sent `undoButtonPress` and gets back `{success: true, canUndo, canRedo}`, but it does NOT receive an `undoAction` callback (because a CODAP entry was undone, not a plugin entry). If the plugin assumes every `undoButtonPress` will result in an `undoAction` callback, it will be confused. -Despite V2 using a single code path, V3 should use **two code paths** to correctly handle the interleaving problem: +3. **`canUndo`/`canRedo` semantics.** The shared stack's `canUndo` reflects ALL actions (plugin + CODAP). The plugin might show its undo button as enabled when the only remaining undo entry is a CODAP graph change, not a plugin action. This is arguably correct (the user CAN undo something) but might surprise a plugin that thinks `canUndo` means "I have plugin actions to undo." -- **Standalone mode** (`uiState.standaloneMode`): Use shadow counters. Plugin undo entries are NOT placed on CODAP's undo stack. `undoButtonPress` sends `undoAction` directly to the plugin and adjusts counters. User-triggered CODAP actions remain on CODAP's stack independently. The two undo histories never interfere with each other. +**If the plugin handles these cases correctly, the shared stack approach works and shadow counters are unnecessary.** If it doesn't, shadow counters paper over the problem but at the cost of making CODAP tile changes permanently non-undoable in standalone mode (since user-triggered CODAP actions would sit on a stack with no UI to access them). -- **External/embedded mode**: Use CODAP's undo stack (the approach from the original analysis). `undoableActionPerformed` creates a custom undo entry on CODAP's stack. `undoButtonPress` calls `document.undoLastAction()`. Interleaving of plugin and CODAP entries is intentional in this mode — they share a unified undo experience. +In **external/embedded mode**, interleaving is unambiguously correct — plugin actions and CODAP actions share a unified undo stack, and the user interacts with undo through CODAP's toolbar. + +### Standalone vs External Mode: One or Two Code Paths? + +V2 uses a single code path for both modes. V3 can likely do the same, **provided the plugin correctly handles the interleaving edge cases** described above. + +- **Single code path (shared stack)**: Both modes use CODAP's undo stack. `undoableActionPerformed` creates a custom undo entry. `undoButtonPress`/`redoButtonPress` call `document.undoLastAction()`/`document.redoLastAction()`. In standalone mode, the user interacts with undo through the plugin's UI; in external mode, through CODAP's toolbar. The interleaving of plugin and CODAP entries is chronologically correct in both modes. + +- **Two code paths (shadow counters for standalone)**: Standalone mode uses per-plugin shadow counters, keeping plugin undo completely separate from CODAP's stack. External mode uses the shared stack. This avoids the plugin-side handling concerns but introduces a tradeoff: user-triggered CODAP tile changes become permanently non-undoable in standalone mode, since they sit on a CODAP undo stack that has no accessible UI. + +**Recommendation**: Start with the single code path (shared stack) approach for both modes. This is simpler to implement, matches V2 behavior, and provides chronologically correct undo. If plugin testing reveals that the plugin doesn't handle the interleaving edge cases correctly, the shadow counter approach can be added as a fallback for standalone mode. The key question to resolve is how Building Models handles `undoButtonPress` responses when a non-plugin action was undone (see [Open Questions](#open-questions)). ### What's Missing @@ -151,26 +164,13 @@ Despite V2 using a single code path, V3 should use **two code paths** to correct ### Phase 1: Core `undoChangeNotice` Handler -The handler should branch on the undo mode early: - -``` -if (standalone mode for this plugin's tile) → shadow counter path -else → CODAP undo entry path -``` +Implement a single code path using CODAP's undo stack for both standalone and external modes. **Note on operation name:** The existing TODO in the handler references `undoButtonPressed` (with trailing "d"), but V2 and Building Models use `undoButtonPress` (no "d"). The implementation should accept the correct V2 name. -#### 1a. Standalone mode: Shadow counters - -Maintain per-plugin undo/redo depth counters (two integers per plugin tile). No CODAP undo stack involvement. +#### 1a. `undoableActionPerformed`: Create CODAP undo entry -- **`undoableActionPerformed`**: Increment undo counter, reset redo counter. Return `{ success: true, values: { canUndo: true, canRedo: false } }`. -- **`undoButtonPress`**: Send `{action: 'notify', resource: 'undoChangeNotice', values: {operation: 'undoAction'}}` directly to the originating plugin via `broadcastMessage` with `targetTileId`. Wait for the plugin's callback. Decrement undo counter, increment redo counter. Return updated `{ canUndo, canRedo }`. -- **`redoButtonPress`**: Same as above but with `{operation: 'redoAction'}`. Increment undo counter, decrement redo counter. - -#### 1b. External mode: CODAP undo entries - -Use CODAP's undo system. When a plugin says it did something undoable: +When a plugin says it did something undoable: 1. Identify the originating plugin's tile ID from `resources.interactiveFrame` 2. Use `withCustomUndoRedo` (or a similar mechanism) to register a history entry with: @@ -179,18 +179,31 @@ Use CODAP's undo system. When a plugin says it did something undoable: 3. The entry should use the existing `DG.Undo.interactiveUndoableAction` / `DG.Redo.interactiveUndoableAction` strings 4. Return `{ success: true, values: { canUndo, canRedo } }` -For `undoButtonPress`/`redoButtonPress`: -- Call `document.undoLastAction()` / `document.redoLastAction()` -- Return `{ success: true, values: { canUndo, canRedo } }` - **Key implementation challenge**: `withCustomUndoRedo` currently works within the context of an `applyModelChange` call. Plugin undo entries don't modify MST state — they're purely side-effect-based (send a message to an iframe). The implementation needs to either: - Create a no-op `applyModelChange` that produces no patches but registers custom undo/redo, OR - Extend the history system to support purely custom entries -#### 1c. Wire up response values +#### 1b. `undoButtonPress` / `redoButtonPress`: Use CODAP's undo stack + +- Call `document.undoLastAction()` / `document.redoLastAction()` +- Return `{ success: true, values: { canUndo, canRedo } }` + +Note: if the top of the undo stack is a CODAP entry (not a plugin entry), the CODAP action will be undone and the plugin will NOT receive an `undoAction` callback. This is chronologically correct undo behavior. Whether the plugin handles this correctly is an open question (see [Open Questions](#open-questions)). -All operations should return `{ canUndo, canRedo }`. In standalone mode, these come from the shadow counters. In external mode, they come from the document model. +#### 1c. Fallback: Shadow counters for standalone mode (if needed) + +If plugin testing reveals that the plugin doesn't handle the shared-stack edge cases (no `undoAction` callback when a CODAP entry is undone, `canUndo`/`canRedo` reflecting all actions), add a standalone-mode-specific code path: + +- Maintain per-plugin undo/redo depth counters (two integers per plugin tile). No CODAP undo stack involvement. +- **`undoableActionPerformed`**: Increment undo counter, reset redo counter. +- **`undoButtonPress`**: Send `undoAction` directly to the plugin via `broadcastMessage`. Decrement undo counter, increment redo counter. +- **`redoButtonPress`**: Send `redoAction` directly. Increment undo counter, decrement redo counter. +- **Tradeoff**: User-triggered CODAP tile changes become permanently non-undoable in standalone mode. + +#### 1d. Wire up response values + +All operations should return `{ canUndo, canRedo }` from the document model. If shadow counters are added later for standalone mode, standalone responses would use the counters instead. ### Phase 2: Proactive Notifications @@ -218,7 +231,7 @@ This could be implemented: | File | Changes | |---|---| -| `src/data-interactive/handlers/undo-change-notice-handler.ts` | Full handler implementation | +| `src/data-interactive/handlers/undo-change-notice-handler.ts` | Full handler implementation (single code path for both modes) | | `src/models/history/tree-manager.ts` or `undo-store.ts` | May need extension for purely custom entries | | `src/models/document/document.ts` | Access to `canUndo`/`canRedo` for responses | | `src/models/history/with-custom-undo-redo.ts` | May need adaptation for no-MST-patch entries | @@ -231,28 +244,52 @@ This could be implemented: 1. **Should plugin-triggered data mutations (create/update/delete cases via API) also be undoable?** **No.** V3 DI handlers already call `applyModelChange` without undo strings, which routes through `withoutUndo()`. This is the correct behavior — plugin-triggered mutations are one half of a logical action whose undo is managed by the plugin itself. Recording them separately would break the undo model. This matches V2's `retainUndo = true` behavior. -2. **Does the implementation need separate code paths for standalone vs external mode?** **Yes.** Although V2 uses a single code path, that approach has an interleaving flaw: user-triggered CODAP actions (e.g., graph changes) create undo entries on the same stack as plugin entries. When the plugin sends `undoButtonPress`, `document.undoLastAction()` may pop a CODAP entry instead of the plugin's entry. V2 gets away with this because direct user manipulation of CODAP tiles is rare in SageModeler, but it's not architecturally correct. V3 should use shadow counters for standalone mode (keeping plugin undo separate from CODAP's stack) and CODAP undo entries for external mode (where interleaving is intentional). +2. **Does the implementation need separate code paths for standalone vs external mode?** **Probably not.** When plugin and CODAP entries interleave on a shared stack, the undo order is chronologically correct — the most recent action is undone first, regardless of source. The interleaving is not a flaw from the user's perspective. The potential issues are plugin-side: whether the plugin handles `undoButtonPress` responses where no `undoAction` is sent back (because a CODAP entry was undone), and whether it correctly interprets `canUndo`/`canRedo` as reflecting all actions. V3 should start with a single code path matching V2, and only add shadow counters for standalone mode if plugin testing reveals problems. + +## Design Rationale: Shared Stack vs Shadow Counters + +### Evolution of this analysis + +This analysis went through several revisions: + +1. **First version**: Concluded V3 could use a single code path (shared stack) for both modes, matching V2. +2. **Second revision**: Identified the "interleaving problem" — plugin and CODAP entries share a stack, so `undoButtonPress` might undo a CODAP entry instead of a plugin entry. Concluded shadow counters were needed for standalone mode. +3. **Third revision (current)**: Realized the interleaving is actually **chronologically correct undo behavior**. The most recent action is undone first, regardless of source. The real concerns are plugin-side, not CODAP-side. + +### The interleaving is not a problem for the user -## Design Rationale: Why Shadow Counters for Standalone Mode +When plugin and CODAP actions interleave on a shared undo stack, the undo order is chronological: the most recent action is always undone first. This is correct. A user who changes a graph setting after a plugin action would expect "undo" to revert the graph change first, then the plugin action — which is exactly what happens. -An earlier version of this analysis concluded that V3 could use a single code path for both modes, matching V2. That conclusion was revised after identifying the **interleaving problem**: in standalone mode, user-triggered CODAP actions (graph changes, table edits, etc.) create undo entries on the same stack as plugin entries. When the plugin sends `undoButtonPress`, `document.undoLastAction()` may pop a user-triggered CODAP entry instead of the plugin's entry, undoing the wrong thing. +### The real concerns are plugin-side -The shadow counter approach (from the CODAP-1127 spec) solves this by keeping plugin undo tracking completely separate from CODAP's undo stack: +The shared stack raises three concerns about how the **plugin** handles edge cases: -- Plugin actions are tracked by per-plugin integer counters, not CODAP undo entries -- `undoButtonPress` sends `undoAction` directly to the plugin and adjusts counters — CODAP's undo stack is never touched -- User-triggered CODAP actions remain on CODAP's own stack independently -- The two undo histories cannot interfere with each other +1. **Plugin internal state sync**: If the plugin eagerly pops its own internal undo stack on every `undoButtonPress` (before receiving the result), it will get out of sync when CODAP undoes a CODAP entry instead of sending `undoAction`. -Note that two earlier concerns about the shared-stack approach turned out to be unfounded and are **not** the reason for choosing shadow counters: +2. **Missing `undoAction` callback**: When `undoButtonPress` causes a CODAP entry to be undone, the plugin does NOT receive an `undoAction` callback. If the plugin assumes every `undoButtonPress` results in `undoAction`, it will be confused. + +3. **`canUndo`/`canRedo` scope**: The shared stack's `canUndo`/`canRedo` reflect all actions (plugin + CODAP). The plugin might show its undo button as enabled when the only remaining entry is a CODAP change. + +### Shadow counters as a fallback + +If the plugin (specifically Building Models/SageModeler) doesn't handle these cases correctly, shadow counters can be added as a standalone-mode-specific fallback. However, shadow counters have their own tradeoff: user-triggered CODAP tile changes become permanently non-undoable in standalone mode, since they sit on a CODAP undo stack with no accessible UI. + +### Debunked concerns + +Two earlier concerns about the shared-stack approach turned out to be unfounded: 1. ~~DI API mutations would corrupt the undo stack~~ — V3 DI handlers already call `applyModelChange` without undo strings, which routes through `withoutUndo()`. Plugin-triggered mutations do not create undo entries. 2. ~~Plugin actions should not appear on CODAP's undo stack~~ — In standalone mode, CODAP's undo UI is hidden. Plugin entries on the stack would be invisible to the user. -The actual reason is the interleaving of plugin entries with user-triggered CODAP entries on a shared stack. In **external mode**, this interleaving is correct and intentional (unified undo experience). In **standalone mode**, it causes the wrong action to be undone. - ## Open Questions -1. **Should `withCustomUndoRedo` be extended, or should a new mechanism be created** for purely side-effect-based undo entries that produce no MST patches? +1. **How does Building Models handle `undoButtonPress` when a non-plugin action is undone?** This is the critical question for deciding between shared stack and shadow counters. Specifically: + - Does Building Models eagerly pop its own internal undo stack on `undoButtonPress`, or does it wait for `undoAction`? + - Does it expect an `undoAction` callback on every `undoButtonPress`, or does it handle the case where no callback arrives? + - Does it use `canUndo`/`canRedo` from the response, or does it track these independently? + + If Building Models handles these cases correctly (or if interleaved CODAP actions are rare enough in practice), the shared stack approach works and shadow counters are unnecessary. + +2. **Should `withCustomUndoRedo` be extended, or should a new mechanism be created** for purely side-effect-based undo entries that produce no MST patches? -2. **How should the proactive notifications be scoped?** V2 broadcasts `clearUndo`/`clearRedo` to all plugins. Should V3 do the same, or only to plugins that have registered undoable actions? +3. **How should the proactive notifications be scoped?** V2 broadcasts `clearUndo`/`clearRedo` to all plugins. Should V3 do the same, or only to plugins that have registered undoable actions? From a6c1e3c6b8e1d22e6542549dbffd541096edd4c4 Mon Sep 17 00:00:00 2001 From: Kirk Swenson Date: Wed, 25 Feb 2026 16:27:25 -0800 Subject: [PATCH 7/7] Verify withCustomUndoRedo supports pure side-effect undo entries MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add test confirming that an MST action producing zero patches but calling withCustomUndoRedo creates a valid undoable history entry with working undo/redo callbacks. This resolves the open question about whether the history system needs extension for plugin undo/redo — it does not. Update analysis document accordingly. Co-Authored-By: Claude Opus 4.6 --- v3/doc/plugin-undo-redo.md | 20 ++++--- v3/src/models/history/undo-store.test.ts | 75 ++++++++++++++++++++++++ 2 files changed, 86 insertions(+), 9 deletions(-) diff --git a/v3/doc/plugin-undo-redo.md b/v3/doc/plugin-undo-redo.md index ea3c7f7fa1..5be3d1a23e 100644 --- a/v3/doc/plugin-undo-redo.md +++ b/v3/doc/plugin-undo-redo.md @@ -137,7 +137,7 @@ V2 uses a single code path for both modes. V3 can likely do the same, **provided ### What's Missing 1. **The `undoChangeNotice` handler implementation** — all three operations are stubbed -2. **Custom undo/redo entry creation** for plugin actions (history entries with no MST patches, only custom callbacks) +2. **Custom undo/redo entry creation** for plugin actions (using `withCustomUndoRedo` — the mechanism already supports this; see [Resolved Questions](#resolved-questions)) 3. **Sending `undoAction`/`redoAction` messages back** to the originating plugin during undo/redo 4. **Proactive `clearUndo`/`clearRedo` notifications** when the undo stack changes 5. **Returning `canUndo`/`canRedo`** in handler responses @@ -179,10 +179,13 @@ When a plugin says it did something undoable: 3. The entry should use the existing `DG.Undo.interactiveUndoableAction` / `DG.Redo.interactiveUndoableAction` strings 4. Return `{ success: true, values: { canUndo, canRedo } }` -**Key implementation challenge**: `withCustomUndoRedo` currently works within the context of an `applyModelChange` call. Plugin undo entries don't modify MST state — they're purely side-effect-based (send a message to an iframe). The implementation needs to either: +**Implementation note**: `withCustomUndoRedo` already supports purely side-effect-based undo entries with no MST state changes. A test in `undo-store.test.ts` ("supports undo/redo of pure side-effect actions") confirms that an MST action which produces zero patches but calls `withCustomUndoRedo` correctly: +- Creates a history entry (the tree manager preserves entries with custom patches even when there are no standard patches) +- Is automatically marked undoable (`undoable: undoable || !!customPatches?.length` in `tree-manager.ts`) +- Invokes the registered custom undo handler on `undoStore.undo()` +- Invokes the registered custom redo handler on `undoStore.redo()` -- Create a no-op `applyModelChange` that produces no patches but registers custom undo/redo, OR -- Extend the history system to support purely custom entries +No extensions to the history system are needed. The implementation simply needs to call `withCustomUndoRedo` from within any MST action on the tile content model, and register a custom undo/redo patcher that sends messages to the plugin iframe #### 1b. `undoButtonPress` / `redoButtonPress`: Use CODAP's undo stack @@ -232,10 +235,9 @@ This could be implemented: | File | Changes | |---|---| | `src/data-interactive/handlers/undo-change-notice-handler.ts` | Full handler implementation (single code path for both modes) | -| `src/models/history/tree-manager.ts` or `undo-store.ts` | May need extension for purely custom entries | | `src/models/document/document.ts` | Access to `canUndo`/`canRedo` for responses | -| `src/models/history/with-custom-undo-redo.ts` | May need adaptation for no-MST-patch entries | | `src/data-interactive/handlers/all-cases-handler.ts` | Remove undo strings from `delete` operation | +| New: custom undo/redo patcher registration for plugin actions | Register handlers that send `undoAction`/`redoAction` to plugin iframes | | New: observer for undo state change notifications | Proactive `clearUndo`/`clearRedo` broadcasts | --- @@ -246,6 +248,8 @@ This could be implemented: 2. **Does the implementation need separate code paths for standalone vs external mode?** **Probably not.** When plugin and CODAP entries interleave on a shared stack, the undo order is chronologically correct — the most recent action is undone first, regardless of source. The interleaving is not a flaw from the user's perspective. The potential issues are plugin-side: whether the plugin handles `undoButtonPress` responses where no `undoAction` is sent back (because a CODAP entry was undone), and whether it correctly interprets `canUndo`/`canRedo` as reflecting all actions. V3 should start with a single code path matching V2, and only add shadow counters for standalone mode if plugin testing reveals problems. +3. **Does `withCustomUndoRedo` need to be extended for purely side-effect-based undo entries?** **No.** A test (`undo-store.test.ts`: "supports undo/redo of pure side-effect actions") confirms that an MST action producing zero patches but calling `withCustomUndoRedo` works correctly out of the box. The tree manager preserves entries that have custom patches even when there are no standard patches (`!entry.records.length && !entry.customPatches?.length` guard), and automatically marks them undoable. The custom undo/redo handlers are invoked correctly during undo and redo. No new mechanism is needed — the implementation simply registers a custom patcher that sends messages to the plugin iframe. + ## Design Rationale: Shared Stack vs Shadow Counters ### Evolution of this analysis @@ -290,6 +294,4 @@ Two earlier concerns about the shared-stack approach turned out to be unfounded: If Building Models handles these cases correctly (or if interleaved CODAP actions are rare enough in practice), the shared stack approach works and shadow counters are unnecessary. -2. **Should `withCustomUndoRedo` be extended, or should a new mechanism be created** for purely side-effect-based undo entries that produce no MST patches? - -3. **How should the proactive notifications be scoped?** V2 broadcasts `clearUndo`/`clearRedo` to all plugins. Should V3 do the same, or only to plugins that have registered undoable actions? +2. **How should the proactive notifications be scoped?** V2 broadcasts `clearUndo`/`clearRedo` to all plugins. Should V3 do the same, or only to plugins that have registered undoable actions? diff --git a/v3/src/models/history/undo-store.test.ts b/v3/src/models/history/undo-store.test.ts index 9870a1d9f0..a25d82ab61 100644 --- a/v3/src/models/history/undo-store.test.ts +++ b/v3/src/models/history/undo-store.test.ts @@ -115,11 +115,26 @@ const TestTile = TileContentModel }), setChildValue(_value: string) { self.child?.setValueWithoutUndo(_value) + }, + // Pure side-effect action: no MST state changes, only custom patches. + // Models the plugin undo/redo case where the action sends a message to an iframe. + performPureSideEffect(actionId: string) { + withCustomUndoRedo({ + type: "TestTile.performPureSideEffect", + data: { id: self.id, actionId } + }) } })) interface TestTileType extends Instance {} +const mockPureSideEffectUndo = jest.fn() +const mockPureSideEffectRedo = jest.fn() + registerCustomUndoRedo({ + "TestTile.performPureSideEffect": { + undo: mockPureSideEffectUndo, + redo: mockPureSideEffectRedo + }, "TestTile.setVolatileValueWithCustomPatch": { undo: (node: IAnyStateTreeNode, patch: ICustomPatch, entry: HistoryEntryType) => { const testTile = resolveIdentifier(TestTile, node, patch.data.id) @@ -979,6 +994,66 @@ it("can track the addition of a new shared model", async () => { ]) }) +it("supports undo/redo of pure side-effect actions (no MST state changes)", async () => { + const {tileContent, manager, undoStore} = setupDocument() + + mockPureSideEffectUndo.mockClear() + mockPureSideEffectRedo.mockClear() + + // Perform an action that produces no MST patches — only a custom patch + tileContent.performPureSideEffect("plugin-action-1") + + // Wait for the entry to be recorded + let timedOut = false + try { + await when( + () => manager.activeHistoryEntries.length === 0, + {timeout: 100}) + } catch (e) { + timedOut = true + } + expect(timedOut).toBe(false) + + // A history entry should exist with the custom patch + expect(manager.document.history.length).toBe(1) + const entry = getSnapshot(manager.document.history[0]) + expect(entry.customPatches).toEqual([{ + type: "TestTile.performPureSideEffect", + data: { id: tileContent.id, actionId: "plugin-action-1" } + }]) + // No standard patches — records may contain an entry with empty patches + const totalPatches = entry.records.reduce((sum, r) => sum + r.patches.length, 0) + expect(totalPatches).toBe(0) + + // Should be undoable + expect(undoStore.canUndo).toBe(true) + expect(undoStore.canRedo).toBe(false) + expect(mockPureSideEffectUndo).not.toHaveBeenCalled() + expect(mockPureSideEffectRedo).not.toHaveBeenCalled() + + // Undo should invoke the custom undo handler + undoStore.undo() + expect(mockPureSideEffectUndo).toHaveBeenCalledTimes(1) + expect(mockPureSideEffectUndo).toHaveBeenCalledWith( + expect.anything(), + { type: "TestTile.performPureSideEffect", data: { id: tileContent.id, actionId: "plugin-action-1" } }, + expect.anything() + ) + expect(undoStore.canUndo).toBe(false) + expect(undoStore.canRedo).toBe(true) + + // Redo should invoke the custom redo handler + undoStore.redo() + expect(mockPureSideEffectRedo).toHaveBeenCalledTimes(1) + expect(mockPureSideEffectRedo).toHaveBeenCalledWith( + expect.anything(), + { type: "TestTile.performPureSideEffect", data: { id: tileContent.id, actionId: "plugin-action-1" } }, + expect.anything() + ) + expect(undoStore.canUndo).toBe(true) + expect(undoStore.canRedo).toBe(false) +}) + async function expectUpdateToBeCalledTimes(testTile: TestTileType, times: number) { const updateCalledTimes = when(() => testTile.updateCount === times, {timeout: 100}) return expect(updateCalledTimes).resolves.toBeUndefined()