Console history not accessible with up arrow

[bojidar-bg]

p5.js version

1.11.1

What is your operating system?

Linux

Web browser and version

Firefox Version: 131.0.3 / 20241014102019

Actual Behavior

Pressing the up arrow key does not bring old code into the console's input field.

Expected Behavior

Pressing the up and down arrow keys cycles through history, so that we don't have to retype any of the code we've just entered
This used to be the case until recently; not sure at what point the regression happened, however.

Steps to reproduce

Steps:

1. Enter some code into the console's input field, e.g. 1. Press enter to execute it.
2. Press the up arrow key.
3. Observe that nothing happens.
4. Input some more text, without pressing enter.
5. Press up arrow again.
6. Observe that your just-entered code gets deleted.

Snippet:

N/A (Default sketch works just fine for reproducing.)

[bojidar-bg]

Looking at the component tree with the React devtools addon and comparing with the code in ConsoleInput.js:12-14, I can see that the commandHistory state manages to only store one line of code, while commandCursor remains at -1 at all times.

I'm going to guess that the problem comes from the fact that useEffect on line 18 and event on line 27 captures the initial value of commandHistory which then renders line 48 ineffective: (and probably likewise for commandCursor)

p5.js-web-editor/client/modules/IDE/components/ConsoleInput.jsx

Line 48 in 686f9e5

setCommandHistory([value, ...commandHistory]);

CC @nahbee10 because it would seem that the previous code, which correctly used state actions for that, was replaced in a466640#diff-5ea6bdfa3c08ded9faeb06a44e8ca992e31c3b5f94c816c390d1cc91266b96ffL45-R48

[raclim]

Thanks for raising this @bojidar-bg! I'm currently working on the Console at the moment, and can add this to the next set of changes to it! Hopefully it should be out by sometime next week.

[ujjwaldubey1]

@raclim I'll Feel very happy to work on this issue

[raclim]

Thanks for the interest @ujjwaldubey1! I did end up finding a solution to it and am currently in the midst of testing and making a few other edits to the Console, so I think we might be covered here. I'll check out the fix you just submitted for the other issue soon though!

[ujjwaldubey1]

Okay!!

[lindapaiste]

@raclim when I reworked this component I found that I had to store both the line number and the array of commands in the same useState hook. That way you can access both items in a prevState callback, allowing you to change the line without needing to introduce a dependency on any state.

That was for v6 so the actual code will be a little different, but the principle still holds.

p5.js-web-editor/client/modules/IDE/components/ConsoleInput.jsx

Lines 76 to 79 in 750994d

	setState((prevState) => ({
	commandCursor: -1,
	commandHistory: [value, ...prevState.commandHistory]
	}));

(I actually feel that it's very bad practice to induce a side effect from within a setState so I kinda hate these two)

p5.js-web-editor/client/modules/IDE/components/ConsoleInput.jsx

Lines 93 to 101 in 750994d

	setState((prevState) => {
	const newCursor = Math.min(
	prevState.commandCursor + 1,
	prevState.commandHistory.length - 1
	);
	replaceContent(view, prevState.commandHistory[newCursor] || '');
	const cursorPos = view.state.doc.lineAt(1).length - 1;
	view.dispatch({ selection: { anchor: cursorPos } });
	return { ...prevState, commandCursor: newCursor };

p5.js-web-editor/client/modules/IDE/components/ConsoleInput.jsx

Lines 115 to 122 in 750994d

	setState((prevState) => {
	const newCursor = Math.max(prevState.commandCursor - 1, -1);
	replaceContent(view, prevState.commandHistory[newCursor] || '');
	const newLineCount = view.state.doc.lines;
	const newLine = view.state.doc.line(newLineCount);
	view.dispatch({ selection: { anchor: newLine.to } });
	return { ...prevState, commandCursor: newCursor };
	});

[lindapaiste]

Looking at the component tree with the React devtools addon and comparing with the code in ConsoleInput.js:12-14, I can see that the commandHistory state manages to only store one line of code, while commandCursor remains at -1 at all times.

I'm going to guess that the problem comes from the fact that useEffect on line 18 and event on line 27 captures the initial value of commandHistory which then renders line 48 ineffective: (and probably likewise for commandCursor)

p5.js-web-editor/client/modules/IDE/components/ConsoleInput.jsx

Line 48 in 686f9e5

setCommandHistory([value, ...commandHistory]);

CC @nahbee10 because it would seem that the previous code, which correctly used state actions for that, was replaced in a466640#diff-5ea6bdfa3c08ded9faeb06a44e8ca992e31c3b5f94c816c390d1cc91266b96ffL45-R48

Thanks for identifying the relevant commit. I can see that we actually were using prevState based callbacks in the previous version and then it was changed to use the state variables directly. But the dependency array for this hook does not contain any of those variables. This means that the value of the commandHistory variable, when accessed inside the useEffect, will always be []. 100% of the time. The body of this function gets the values of all of its variables on the very first render and they are never re-evaluated, due to the empty dependency array. @nahbee10 this is called a "stale closure" and it's what I was warning you about in the WIP PR

You have to be very careful that any function which you provide to the CodeMirror editor will be called with the right variables at the time of execution.

Stale closures are a major issue when dealing with these CodeMirror components, and it's why I've rejected so many previous attempts to convert this component. There is a lot of explanation and suggestions in my review comments on #2409 that you should look at.

[raclim]

@lindapaiste I just attached the PR I was working on—in it I continued with some of the newer changes and broke the useEffect hook down into separate ones and added in the relevant variables (commandHistory, commandCursor, dispatchConsoleEvent) into their dependency arrays, which I think is handling the stale closures.

I found it to work, but it's a bit lengthy and I guess I'm not sure if it's the best approach?