fix shared model manager warning

[scytacki]

afterAttachToDocument is called before the shared model manager is ready.
This then causes a warning when a shared model is accessed by a tile using afterAttachToDocument.

The specific warning this prevents is:
shared-model-document-manager.ts:162 getTileSharedModels has no document

This warning was harmless because the reaction in graph tile would just get called a second time when the shared model manager was ready. But it seems better, to just wait until the shared model manager is ready before trying to access the data.

Also note that the tile will always be attached to the document when the shared model manager is ready, so there isn't a need for the afterAttachToDocument.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (57fb2c3) 79.76% compared to head (f292868) 81.94%.
Report is 28 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #2044      +/-   ##
==========================================
+ Coverage   79.76%   81.94%   +2.17%     
==========================================
  Files         649      649              
  Lines       32084    32089       +5     
  Branches     8401     8405       +4     
==========================================
+ Hits        25592    26295     +703     
+ Misses       6136     5487     -649     
+ Partials      356      307      -49     

Flag	Coverage Δ
cypress-regression	70.55% <100.00%> (+4.00%)	⬆️
cypress-smoke	39.78% <ø> (-0.04%)	⬇️
jest	51.04% <0.00%> (-0.01%)	⬇️

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.

[cypress]

Passing run #9181 ↗︎

0

1

0

0

0

Details:

WIP notes about simplifying shared model sync
Project: collaborative-learning	Commit: f29286860a
Status: Passed	Duration: 01:46 💡
Started: Nov 13, 2023 10:41 PM	Ended: Nov 13, 2023 10:43 PM

Review all test suite changes for PR #2044 ↗︎

[bgoldowsky]

This code has completely changed in PRs #2028 / #2037 so this will certainly cause conflicts there. However, it answers one of the big questions that I had since taking some actions after shared model manager is ready is still important. So, we can merge this to fix the error on master and then make similar changes in the multi-dataset branch.

[scytacki]

Thanks @bgoldowsky. I think it would be good to see if @kswenson has comments on this, but Boris if it makes your work easier go ahead and merge it and we can address any issues from Kirk in another PR.

[kswenson]

I think this issue has gotten a bit muddled. The reason I added support for afterAttachToDocument() was to simplify things for clients, notably around the issue of references, as my comment suggests. The tile content's afterAttach() is called at a time when reference resolution will fail, whereas afterAttachToDocument() is called at a time when reference resolution can succeed. At the time that I added that code originally it was certainly the case that code that failed (i.e. threw a reference resolution exception) before the change succeeded after I added afterAttachToDocument().

Part of the muddle is that it is almost certainly true that the reference in question was to a shared model, but I don't know whether it's necessarily true that all references from tile content to other MST models will be to shared models. 🤷 In any case, I take your main point to be that since the shared model manager isn't guaranteed to be ready in afterAttachToDocument() that a MobX reaction is required to wait for it in any case, so every tile should have its own MobX reaction and that reaction can be in each tile's afterAttach() rather than afterAttachToDocument().

My inclination would be to go the other way -- why should every tile have to implement the same subtly confusing MobX reaction? Why not put the reaction in TileModel itself and then call afterAttachToDocument() when the shared model manager is ready, passing whatever arguments would be useful to the client? In other words, rather than abandoning afterAttachToDocument() we can make it more useful by delaying it until the shared model manager is ready. It seems to me that would simplify things for clients and mean that the subtly confusing reaction could be written in one place in the TileModel rather than requiring that every tile have its own version of it.

I'm also confused by this comment in the PR description:

Also note that the tile will always be attached to the document when the shared model manager is ready, so there isn't a need for the afterAttachToDocument.

which is clearly not correct and, in fact, belied by the rest of the PR.

[scytacki]

I agree that we should try to simplify things for clients (tiles), but I didn't find a simple way to do that since most tiles need to observe different things not just the sharedModelManager and whether it is ready.

Regarding the "Also note that..." comment: here is a more precise version of it:

In a reaction that is observing a tile's sharedModelManager and whether it is ready, when the sharedModelManager is ready, then it is guaranteed that the tile is attached to the document.

This is because the sharedModelManager is available via the document's environment. So when the tile content model is just attached to the tile but the tile is not attached to the document, there won't be a shared model manager available.

[scytacki]

@bgoldowsky since this conflicts with your other changes, and this PR needs more discussion, don't let it hold up your other work. After your other work is merged in I can fix this up.

@kswenson for reference for our conversation, here are the afterAttach dependencies for tiles:

• geometry: { sharedModelManager, sharedDataSets, links: self.links }
• table: { sharedModelManager, sharedDataSet, tileSharedModels }
• data-card: { sharedModelManager, sharedDataSet, tileSharedModels }
• data-flow: { sharedModelManager, sharedDataSet, firstDocumentVariableSharedModel, tileSharedModels }
• diagram: {sharedModelManager, firstDocumentVariableSharedModel, tileSharedModels}
• graph: { sharedModelManagerReady, data } in this PR
• simulator: {sharedModelManager, firstDocumentVariableSharedModel, tileSharedModels}

An interesting note is that we are using the "default" built in comparer: https://mobx.js.org/computeds.html#built-in-comparers so the lists of dependencies I provided above are not actually right. In all of these reactions we are re-creating the "value" object, so this new object will be different every time. Therefore any change to a MobX observable accessed in the value function will cause the reaction to run again.

[bgoldowsky]

It is not holding up my work. I have this afterAttach in my branch now. It can potentially be simplified in the future if you make the changes under discussion.

afterAttach() {
      if (self.layers.length === 1 && !self.layers[0].config.dataset && !self.layers[0].config.isEmpty) {
        // Non-empty dataset lacking a dataset reference = legacy data needing a one-time fix.
        // We can't do that fix until the SharedModelManager is ready, though.
        addDisposer(self, reaction(
          () => {
            return self.tileEnv?.sharedModelManager?.isReady;
          },
          (ready) => {
            if (!ready) return;
            this.setDataConfigurationReferences();
          }
        ));
      }
    },

[scytacki]

After discussing this with this with Kirk and doing more brainstorming and evaluation, I'm going to put this PR on hold. It now includes some of my notes in shared-models.md. They aren't really organized yet, so if you try to read them they might cause more confusion. Below are some hopefully more readable notes:

• I think we could provide a single API perhaps named syncWithSharedModels that would be called on tile content models. This would replace the reactions currently in afterAttach, and replace the updateAfterSharedModelChanges API. It would basically be a managed autorun, so anything that was accessed by syncWithSharedModels would become observed and the function would be called again if that thing changed.
• A problem with the current afterAttach+reaction approach is that it can mess up the undo stack. If a new shared model is added to the document many of these tile reactions will run and in some cases they will modify the tile or shared model. Those changes result in a second event added to the undo stack. The new approach above would prevent this, in a similar way to how updateAfterSharedModelChanges does: the function is called by our manager instead defining its own reaction or autorun.
• being able to leverage MobX's reaction system while at the same time making sure syncWithSharedModels is not called again and again when multiple history entries are replayed is tricky but seems do-able
• being able to leverage MobX's reaction system while at the same time grouping its changes to the tile and shared models into a single undo entry with the original change is still unknown. I'm pretty sure it is possible, using the same technique that is used for updateAfterSharedModelChanges, but I haven't fully verified that yet. A complication is that it means we are going to be making something like an "action": a chunk of code which changes the state. And we want this chunk of code to also track its observable access and be re-run if they change. MobX and MST both treat "actions" as untracked, so this is breaking that pattern. We are basically already doing this, so this breaking of the pattern isn't new, but trying record these action-like functions will be new.
• an incremental step toward all of this is to fix updateSharedDataSetColors. This function is called in several tile afterAttach+reaction functions. It seems to be updating a global data set color map, not one specific to a document. That means the colors would be shared by all of the documents open by CLUE. And it seems like it would be run once for each relevant tile. And when it runs it scans each relevant tile. So that means it will scan tileNum^2 tiles. This can be fixed by moving it out of the tile reactions to the document level, but we don't have a mechanism for a shared model "plugin" to add a reaction like this at the document level when the first one is added to the document. This is related to the above work because this scanning of the shared models start to be tracked in the new approach so lots of extra calls to syncWithSharedModels would happen.

[kswenson]

• an incremental step toward all of this is to fix updateSharedDataSetColors. This function is called in several tile afterAttach+reaction functions. It seems to be updating a global data set color map, not one specific to a document. That means the colors would be shared by all of the documents open by CLUE. And it seems like it would be run once for each relevant tile. And when it runs it scans each relevant tile. So that means it will scan tileNum^2 tiles. This can be fixed by moving it out of the tile reactions to the document level, but we don't have a mechanism for a shared model "plugin" to add a reaction like this at the document level when the first one is added to the document. This is related to the above work because this scanning of the shared models start to be tracked in the new approach so lots of extra calls to syncWithSharedModels would happen.

It should be within the document rather than global.