Skip to content

CODAP-1118: plugin undo/redo analysis document#2388

Open
kswenson wants to merge 7 commits intomainfrom
CODAP-1118-plugin-undo-redo
Open

CODAP-1118: plugin undo/redo analysis document#2388
kswenson wants to merge 7 commits intomainfrom
CODAP-1118-plugin-undo-redo

Conversation

@kswenson
Copy link
Member

@kswenson kswenson commented Feb 13, 2026

Summary

  • Add analysis document (v3/doc/plugin-undo-redo.md) covering the V2 plugin undo/redo implementation and a V3 implementation plan for the undoChangeNotice API
  • Re-analyze V2 implementation in light of CODAP-1127 spec comparison, with 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
    • Flag all-cases-handler undo strings as a likely bug
  • Identify the interleaving problem: user-triggered CODAP actions (graph changes, table edits) share the undo stack with plugin entries, so undoButtonPress can undo the wrong action in standalone mode
  • Recommend two code paths: shadow counters for standalone mode (separate from CODAP's stack), CODAP undo entries for external mode (where interleaving is intentional)

Refs CODAP-1118

Test plan

  • Documentation only -- no code changes to test

Generated with Claude Code

kswenson and others added 2 commits February 13, 2026 09:39
V2 implementation analysis and V3 implementation plan for the
undoChangeNotice plugin API (CODAP-1118).

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@kswenson kswenson added the v3 CODAP v3 label Feb 13, 2026
Code PRs start as drafts; docs-only PRs skip draft status.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@cypress
Copy link

cypress bot commented Feb 13, 2026

codap-v3    Run #10457

Run Properties:  status check passed Passed #10457  •  git commit a6c1e3c6b8: Verify withCustomUndoRedo supports pure side-effect undo entries
Project codap-v3
Branch Review CODAP-1118-plugin-undo-redo
Run status status check passed Passed #10457
Run duration 02m 19s
Commit git commit a6c1e3c6b8: Verify withCustomUndoRedo supports pure side-effect undo entries
Committer Kirk Swenson
View all properties for this run ↗︎

Test results
Tests that failed  Failures 0
Tests that were flaky  Flaky 0
Tests that did not run due to a developer annotating a test with .skip  Pending 0
Tests that did not run due to a failure in a mocha hook  Skipped 0
Tests that passed  Passing 4
View all changes introduced in this branch ↗︎

@kswenson kswenson requested a review from bfinzer February 13, 2026 17:48
Copy link
Contributor

@bfinzer bfinzer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍🏻LGTM

@kswenson kswenson requested a review from scytacki February 17, 2026 02:05
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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
@codecov
Copy link

codecov bot commented Feb 23, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 69.24%. Comparing base (2a3224a) to head (a6c1e3c).
⚠️ Report is 97 commits behind head on main.

❗ There is a different number of reports uploaded between BASE (2a3224a) and HEAD (a6c1e3c). Click for more details.

HEAD has 15 uploads less than BASE
Flag BASE (2a3224a) HEAD (a6c1e3c)
cypress 16 1
Additional details and impacted files
@@             Coverage Diff             @@
##             main    #2388       +/-   ##
===========================================
- Coverage   85.45%   69.24%   -16.21%     
===========================================
  Files         753      753               
  Lines       41500    41500               
  Branches    10235    10235               
===========================================
- Hits        35462    28736     -6726     
- Misses       6028    12758     +6730     
+ Partials       10        6        -4     
Flag Coverage Δ
cypress 39.63% <ø> (-29.93%) ⬇️
jest 56.90% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

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 <noreply@anthropic.com>
Copy link
Member

@scytacki scytacki left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The part missing from this analysis is the potential issue with id or other state that probably won't be maintained properly when the plugin undoes or replays its actions.

For example if the plugin creates the graph and then the user modifies the graph. If the graph change is undone and then the plugin's creation of the graph is undone. If the plugin's change is then redone, it will very likely create a tile with a new id instead of using the previous id. Now when the graph change is applied it will fail.

Perhaps this kind of failure is OK, but it should be noted here. Since many of the actions plugins take are creating graphs and tables this seems like it will make the undo support not very useful.

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 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

v3 CODAP v3

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants