doc: add SageModeler v3 compatibility requirements spec [CODAP-1127]#2431
doc: add SageModeler v3 compatibility requirements spec [CODAP-1127]#2431dougmartin wants to merge 1 commit intomainfrom
Conversation
Document the spike findings for running SageModeler with CODAP v3. The spec covers URL parameters, plugin state persistence, undo/redo integration (the sole blocker), CFM communication, DI API surface, and deployment routing. R7 (CloudFront) and R8 (dev proxy) have been implemented in: - concord-consortium/sage-modeler-site#211 - concord-consortium/sage-modeler-site#210
daecbaf to
5f7193e
Compare
There was a problem hiding this comment.
Pull request overview
This PR adds a comprehensive technical specification document for enabling CODAP v3 to run within the SageModeler iframe stack. The spec documents the findings from a spike investigation into SageModeler compatibility requirements.
Changes:
- Added requirements specification covering URL parameters, plugin state persistence, undo/redo integration, CFM communication, Data Interactive API usage, and deployment routing
- Documented that the
undoChangeNoticehandler is the primary blocker for full SageModeler compatibility - Included detailed technical notes on implementation constraints, particularly around standalone undo mode and autosave behavior
Comments suppressed due to low confidence (1)
v3/docs/plans/CODAP-1127-sagemodeler-in-v3/requirements.md:168
- Line 168 contains an extremely long paragraph (over 1000 characters) that is difficult to read. Consider breaking this critical implementation constraint into separate paragraphs or using bullet points for the different concepts: (1) the per-tile suppression mechanism, (2) the autosave hazard explanation, and (3) the solution using withoutUndo(). This would improve readability for future developers implementing this requirement.
**Critical implementation constraint**: In standalone undo mode, all DI API requests from the standalone plugin's webview tile must be treated as non-undoable by CODAP, even if the underlying handler would normally mark them undoable. The scope is per-tile, keyed by the tile's `interactiveFrame` ID: CODAP knows which tile returned `standaloneUndoModeAvailable: true` in its `interactiveFrame` response, and suppresses undo recording for DI requests originating from that tile. (Every DI handler already receives the originating tile via `resources.interactiveFrame`, populated by `resolveResources()` in the request pipeline — no new plumbing is needed to identify the source tile.) Without this, when Building Models undoes "add node" (issuing a DI DELETE), TreeManager would record that DELETE as a new undoable action, creating an incoherent undo stack. This suppression is scoped to DI-sourced mutations only — any direct CODAP UI edits (e.g., editing table cells, resizing components) should remain undoable through CODAP's normal undo system if they are possible in this mode. **Autosave hazard**: `treeManager.revisionId` — the sole trigger for dirty state and autosave — is only updated inside `completeHistoryEntry()`, which is part of the undo/history recording pipeline (`tree-manager.ts:179-182`). This means `revisionId` is **coupled** to history entry creation, not to MST tree changes directly. If the suppression mechanism bypasses the history pipeline entirely (e.g., raw MST mutations without `applyModelChange`), then `revisionId` won't increment, dirty state won't propagate, and **autosave will silently break**. The safe path is v3's existing `withoutUndo()` mechanism: it creates a non-undoable history entry that still completes normally.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| - **Production deployment**: CloudFront routing and SageModeler URL construction (R7) have been updated — see [sage-modeler-site PR #211](https://github.com/concord-consortium/sage-modeler-site/pull/211) | ||
| - **Dev tooling**: A v3-compatible dev proxy (R8) has been implemented — see [sage-modeler-site PR #210](https://github.com/concord-consortium/sage-modeler-site/pull/210) |
There was a problem hiding this comment.
There is a significant discrepancy between the PR description and the document content regarding sage-modeler-site PRs #211 and #210. The PR description states that #211 is about "Log remoteEndpoint in logLaraData" and #210 is about "Fix number toggle bugs", but this document describes #211 as CloudFront routing changes (R7) and #210 as dev proxy implementation (R8). Please verify that the correct PR numbers are referenced, or update the PR description to accurately reflect what these PRs contain.
codap-v3
|
||||||||||||||||||||||||||||
| Project |
codap-v3
|
| Branch Review |
CODAP-1127-sagemodeler-in-v3
|
| Run status |
|
| Run duration | 02m 12s |
| Commit |
|
| Committer | Doug Martin |
| View all properties for this run ↗︎ | |
| Test results | |
|---|---|
|
|
0
|
|
|
0
|
|
|
0
|
|
|
0
|
|
|
4
|
| View all changes introduced in this branch ↗︎ | |
Codecov Report✅ All modified and coverable lines are covered by tests.
Additional details and impacted files@@ Coverage Diff @@
## main #2431 +/- ##
===========================================
- Coverage 85.50% 69.24% -16.26%
===========================================
Files 757 757
Lines 42044 42044
Branches 10410 10007 -403
===========================================
- Hits 35948 29112 -6836
- Misses 6081 12926 +6845
+ Partials 15 6 -9
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
scytacki
left a comment
There was a problem hiding this comment.
The undo section doesn't seem to accurately handle the interleaved requirement. The document Kirk create does address it, but see my comment on that PR: #2388
Based on that comment, I think undo will be pretty broken without some more complex fix so CODAP changes can be replayed on top of plugin changes. However perhaps if we identify when a CODAP change can't be replayed, and just not replay it and clear any undo/redo entries after it that will be good enough for Dan.
kswenson
left a comment
There was a problem hiding this comment.
👍 Great analysis -- very detailed and thought-provoking. I think the back-and-forth on the undo/redo question has been very informative, and forced me to think much more carefully about it. Having done so, however, I come to the conclusion that we can/should follow the V2 implementation rather than the shadow counter approach proposed here. The reasons are summarized below and also discussed in the recent updates to PR #2388. I'm not sure what the best disposition of this PR is, but maybe just acknowledge that the undo/redo discussion continued beyond what is proposed here, so this should not be taken as the last word on the subject. The rest of this review was developed in conjunction with Claude Code 🤖.
PR Review Summary
Changes: 1 file, +384 / -0 lines (new document)
What it does
Adds a comprehensive requirements specification for running SageModeler with CODAP v3. This documents the findings from a spike investigation (CODAP-1127) covering the full iframe stack: SageModeler → CFM → CODAP v3 → Building Models plugin. The spec catalogs 8 requirement areas (URL params, plugin state, undo/redo, v2 compat, CFM communication, DI API surface, CloudFront routing, dev proxy), identifies the sole blocker (undo/redo integration via undoChangeNotice), and provides detailed implementation guidance for fixing it.
The approach
This is a docs-only PR — a single new markdown file at v3/docs/plans/CODAP-1127-sagemodeler-in-v3/requirements.md. The spec is well-organized with an executive summary, project owner overview, background, 8 numbered requirements (R1–R8), technical notes, an E2E smoke test, and resolved open questions.
The R3 section (undo/redo) is the most substantial and where the main discussion point lies.
Assessment
The spec is thorough and well-researched. All code references are accurate (verified against current source). The non-undo sections (R1, R2, R4–R8) are excellent — clear status assessments, precise file/line references, and good separation of what's implemented vs what needs work.
The R3 undo/redo section contains a detailed analysis, but a deeper look at V2's actual implementation and V3's existing infrastructure suggests that V3 can successfully follow the V2 implementation rather than introducing shadow counters as a new mechanism.
R3 Discussion: V3 Can Follow V2's Proven Approach
The spec proposes shadow counters for standalone mode and rejects TreeManager integration, arguing that "there would be two systems competing over the same state." However, a closer analysis (documented in PR #2388) reveals that this concern doesn't arise in practice, and that V2's simple, proven approach translates directly to V3.
V2's approach is mode-agnostic and straightforward. V2's handleUndoChangeNotice has no standalone-vs-external branching. When a plugin sends undoableActionPerformed, CODAP creates a DG.Command with an empty execute() and callbacks that send undoAction/redoAction back to the plugin. That's it. The same code path handles both modes.
The "two systems competing" concern doesn't arise. The spec worries that if plugin undo entries go on CODAP's stack, TreeManager would also record undo entries for the DI API mutations that Building Models re-issues during undo/redo. But V3 DI handlers already call applyModelChange() without undo strings, which routes through withoutUndo(). Plugin-triggered data mutations do not create undo entries. This is functionally equivalent to V2's retainUndo = true behavior — it's already solved.
V3 has the building blocks for the V2 approach. The withCustomUndoRedo mechanism supports registering non-patch-based undo/redo entries — exactly what's needed to create entries whose undo/redo callbacks simply send messages to the plugin iframe. No shadow counters, no separate bookkeeping, no mode-specific branching.
The interleaving of plugin and CODAP entries on a shared stack is chronologically correct undo. The most recent action is undone first, regardless of source. This is the same behavior V2 has always had.
As Scott Cytacki notes in his comment, there are scenarios where interleaved undo/redo can lead to inconsistent state (e.g., a plugin redoing "create graph" produces a new tile ID, breaking a subsequent CODAP undo entry that references the old ID). But these edge cases are equally true for V2 — they're inherent to the relay model where CODAP doesn't understand the plugin's internal state. The shadow counter approach doesn't solve them either; it just avoids them by making CODAP tile changes non-undoable in standalone mode, which is a real tradeoff.
Recommendation: The R3 section should be revised to acknowledge that the undo/redo discussion has continued beyond what is proposed here (see PR #2388, for instance), so this should not be taken as the last word on the matter. The spec's analysis of the withoutUndo()/noDirty mechanism, the dirtyDocument metadata distinction, the response contract, and the acceptance criteria are all solid and should be retained.
Other Notes
- The
undoButtonPressvsundoButtonPresseddiscrepancy callout (line 183) is a good catch and should be preserved. - The autosave hazard analysis around
revisionIdcoupling is valuable context, though with the V2-style approach it becomes less critical sincewithCustomUndoRedoalready goes through the normal history pipeline. - Copilot's suggestion to break up the long paragraph at line 168 into bullets is reasonable for readability.
|
Sound fine to me. There is a reference to this PR from the other one, so that should help us find it or remember it. |
Document the spike findings for running SageModeler with CODAP v3. The spec covers URL parameters, plugin state persistence, undo/redo integration (the sole blocker), CFM communication, DI API surface, and deployment routing. R7 (CloudFront) and R8 (dev proxy) have been implemented in: