Get unexpected results when clicking run after loading a document for the first time

[wlach]

A colleague of mine loaded this notebook and clicked on the play button:

https://extremely-alpha.iodide.io/notebooks/139/

And then got this error:

ReferenceError: pyodide is not defined

It seems like when you first load a document and press play, it evaluates the last cell by default? I guess because that's where the caret position is by default. Here's a simplified notebook that demonstrates the problem:

https://definitely-staging.iodide.io/notebooks/8/

Should we disable the play button when not in the text area? Or set the caret position to 0 by default? Or something else entirely? I think the current interaction model isn't at all clear for someone unfamiliar with iodide who just gets a link to a notebook like the above.

[hamilton]

Great catch. We should be defaulting the caret position to 0 by default, I think. @bcolloran thoughts there?

[bcolloran]

ah yes, we should definitely be defaulting the cursor pos to 0.

longer term/more correct solution: we should move the current cursor position into the redux store and fire an action to update it whenever the cursor moves, instead of just checking the cursor position inside of the evaluateText action. this will more cleanly decouple the actions from the internals of the editor, which will (1) push this into our state tree, where we have more kinds of sanity checking and thus bugs like this are easier to to catch in advance, and (2) make it easier to migrate to Monaco down the line. (I don't know why i didn't do this to begin with -- this was my bad during the jsmd migration)

[wlach]

ah yes, we should definitely be defaulting the cursor pos to 0.

longer term/more correct solution: we should move the current cursor position into the redux store and fire an action to update it whenever the cursor moves, instead of just checking the cursor position inside of the evaluateText action. this will more cleanly decouple the actions from the internals of the editor, which will (1) push this into our state tree, where we have more kinds of sanity checking and thus bugs like this are easier to to catch in advance, and (2) make it easier to migrate to Monaco down the line. (I don't know why i didn't do this to begin with -- this was my bad during the jsmd migration)

Yup this sounds like the right solution looking at the code. Until we have time to do that, I have an interim patch in #1582 which does the simple thing we talk about above.

[hamilton]

@wlach @bcolloran would you argue that we've fixed the immediate issue here? I understand there is a better medium-term fix to be done but at least we've fixed this rough edge for now.

[hamilton]

(and maybe should close this)

[wlach]

@wlach @bcolloran would you argue that we've fixed the immediate issue here? I understand there is a better medium-term fix to be done but at least we've fixed this rough edge for now.

We could file a new issue with @bcolloran's suggestions above, though I'm not sure what the priority should be. I'll defer to him.

[bcolloran]

i'll file a new issue. i plan to look at this soon.