Skip to content

Refactor /sketcher page #2140

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
jacbn wants to merge 1 commit into
base: main
Choose a base branch
from
+43 −34
Draft

Refactor /sketcher page #2140

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
77 changes: 43 additions & 34 deletions src/app/components/pages/GraphSketcher.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,27 +16,44 @@ const GraphSketcherPage = () => {
const user = useAppSelector(selectors.user.orNull);
const [modalVisible, setModalVisible] = useState(false);
const {openModal, closeModalAndReturnToScrollPosition} = useModalWithScroll({setModalVisible});
const [currentAttempt, setCurrentAttempt] = useState<GraphChoiceDTO | undefined>();
const [previewSketch, setPreviewSketch] = useState<GraphSketcher>();
const [initialState, setInitialState] = useState<GraphSketcherState>();
const [currentAttempt, setCurrentAttempt] = useState<GraphChoiceDTO | undefined>(undefined);
const previewSketch = useRef<GraphSketcher | undefined>(undefined);
const previewRef = useRef(null);
const [generateGraphSpec, {data: graphSpec}] = useGenerateAnswerSpecificationMutation();

const onGraphSketcherStateChange = (newState: GraphSketcherState) => {
setInitialState(newState);
setCurrentAttempt({type: 'graphChoice', value: JSON.stringify(GraphSketcher.toExternalState(newState))});
if (previewSketch) {
previewSketch.state = newState;
previewSketch.state.curves = previewSketch.state.curves || [];
const initialModalState: GraphSketcherState | undefined = currentAttempt?.value ? GraphSketcher.toInternalState(JSON.parse(currentAttempt.value)) : undefined;

// loads with previous attempt data in real use
const [pendingAttemptState, setPendingAttemptState] = useState<GraphSketcherState | undefined>(undefined);

const updatePreviewState = useCallback((attempt: GraphChoiceDTO | undefined) => {
// Set the state of the preview box whenever currentAttempt changes
if (previewSketch.current && attempt?.value) {
console.log(previewSketch.current);
const data: GraphSketcherState = JSON.parse(attempt.value);
Copy link
Copy Markdown
Contributor

@sjd210 sjd210 May 22, 2026

Choose a reason for hiding this comment

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

re: Your other comment

I've found the issue! The preview is meant to update when previewSketch updates, and that happens in this callback - however the data being fed into previewSketch is in the wrong format. We need to convert it into the internal state that the graph sketcher can read first:

Suggested change
const data: GraphSketcherState = JSON.parse(attempt.value);
const data: GraphSketcherState = GraphSketcher.toInternalState(JSON.parse(attempt.value));

The reason it was working with initialModalState is that it does make this conversion (up on L24) and so the graph sketcher could be re-initialised with a correct data format and that would allow a rendered curve.

I'm pretty sure this change lets you remove the initialCurves parameter and initialModalState?.curves dependency that has been troubling you.

data.canvasWidth = 1000;
data.canvasHeight = 600;
data.curves = data.curves || [];
previewSketch.current.state = data;
}
};
}, []);

const closeModal = useCallback(async () => {
if (currentAttempt?.value && isStaff(user)) {
await generateGraphSpec({ type: 'graphChoice', value: currentAttempt.value});
if (pendingAttemptState) {
const newAttempt = {type: 'graphChoice', value: JSON.stringify(GraphSketcher.toExternalState(pendingAttemptState))};

setCurrentAttempt(newAttempt);
updatePreviewState(newAttempt);
setPendingAttemptState(undefined);

if (isStaff(user)) {
await generateGraphSpec({ type: 'graphChoice', value: newAttempt.value});
}
}

closeModalAndReturnToScrollPosition();
}, [currentAttempt?.value, user, generateGraphSpec, closeModalAndReturnToScrollPosition]);

}, [pendingAttemptState, closeModalAndReturnToScrollPosition, updatePreviewState, user, generateGraphSpec]);

const handleKeyPress = useCallback(async (ev: KeyboardEvent) => {
if (ev.code === 'Escape') {
Expand All @@ -50,29 +67,21 @@ const GraphSketcherPage = () => {
return () => {
window.removeEventListener('keyup', handleKeyPress);
};
}, [closeModal, handleKeyPress]);
}, [handleKeyPress]);

useEffect(() => {
if (previewSketch) return;
if (makeGraphSketcher && previewRef.current) {
const { sketch } = makeGraphSketcher(previewRef.current || undefined, 1000, 600, { previewMode: true, initialCurves: initialState?.curves });
if (sketch) {
sketch.selectedLineType = LineType.BEZIER;
setPreviewSketch(sketch);
}
const { sketch, p: p5 } = makeGraphSketcher(previewRef.current, 1000, 600, { previewMode: true, initialCurves: initialModalState?.curves });
if (sketch) {
sketch.selectedLineType = LineType.BEZIER;
previewSketch.current = sketch;
}
}, [previewRef, previewSketch]);

useEffect(() => {
// Set the state of the preview box whenever currentAttempt changes
if (previewSketch && currentAttempt?.value) {
const data: GraphSketcherState = JSON.parse(currentAttempt.value);
data.canvasWidth = 1000;
data.canvasHeight = 600;
data.curves = data.curves || [];
previewSketch.state = data;
}
}, [currentAttempt]);
return () => {
// teardown sketcher instance on unmount
previewSketch.current?.teardown();
p5?.remove();
};
}, [initialModalState?.curves]);
Copy link
Copy Markdown
Contributor Author

@jacbn jacbn May 7, 2026

Choose a reason for hiding this comment

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

This UE dependency is the one thing I am completely lost on. It seems to me that the intention was for setting sketch.state (i.e. previewSketch.current.state here) to automatically update the previewRef canvas and everything to just work; with this approach, we'd only ever need to run this UE once, and only adjust sketch.state to edit the preview.

However. This simply doesn't seem to be working. The only way I can get the preview to update is if I insert initialCurves: initialModalState?.curves into the makeGraphSketcher (which shouldn't need to be there because the graph always initialises from empty on this page), and rerun this UE after the modal closes with this extra dependency. I really don't understand how the sketcher question page works without doing this...


return <div>
<Container>
Expand All @@ -84,8 +93,8 @@ const GraphSketcherPage = () => {
{modalVisible && <GraphSketcherModal
user={user}
close={closeModal}
onGraphSketcherStateChange={onGraphSketcherStateChange}
initialState={initialState}
onGraphSketcherStateChange={setPendingAttemptState}
initialState={initialModalState}
/>}
</div>
{graphSpec && graphSpec.map((spec, i) => <pre key={i}>{spec}</pre>)}
Expand Down
Loading