feat(scribing): migrate from fabric.js v5 to v7#8287
feat(scribing): migrate from fabric.js v5 to v7#8287adi-herwana-nus wants to merge 1 commit intomasterfrom
Conversation
- ported top-level components and Redux state reducer from JS to TS - minor improvements to component CSS - implemented new CSS zoom-based canvas scaling - fixed LayersComponent - fixed toggling between fill / no fill for existing shapes
adi-herwana-nus
force-pushed
the
adi/scribing-library-updates
branch
from
2d2762d to
542c249
Compare
There was a problem hiding this comment.
Pull request overview
Migrates the course assessment submission “scribing” feature from fabric.js v5 to v7, refactors the main scribing UI components from class-based JSX to functional TSX, and modernizes scribing state management to a Redux Toolkit slice to support the new Fabric v7 async/object model and reduce dependency vulnerabilities.
Changes:
- Upgrade
fabricdependency to v7.2.0 and update scribing canvas/toolbar implementations for Fabric v7 APIs and event ordering. - Replace legacy scribing reducer/action patterns with a Redux Toolkit slice (
scribingActions) and wire explicit initialization into submission fetch. - Convert scribing UI components (Canvas/Toolbar/Layers/Fields) to TSX functional components and introduce a shared imperative
ScribingCanvasRef.
Reviewed changes
Copilot reviewed 22 out of 23 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| client/tailwind.config.ts | Adds a custom Tailwind utility used by the new scribing layout. |
| client/package.json | Pins fabric.js to v7.2.0. |
| client/app/types/course/assessment/submission/answer/scribing.ts | Introduces explicit TS interfaces for scribing answer payloads. |
| client/app/bundles/course/assessment/submission/reducers/scribing/index.ts | New RTK slice for scribing state + actions. |
| client/app/bundles/course/assessment/submission/reducers/scribing.js | Removes legacy Immer-based scribing reducer. |
| client/app/bundles/course/assessment/submission/propTypes.js | Removes legacy scribing PropTypes shapes (now TS-driven). |
| client/app/bundles/course/assessment/submission/constants.ts | Reworks scribing constants into typed as const arrays + union types. |
| client/app/bundles/course/assessment/submission/components/ScribingView/ScribingToolbar.tsx | New TSX functional toolbar using hooks + canvas ref. |
| client/app/bundles/course/assessment/submission/components/ScribingView/ScribingToolbar.jsx | Removes legacy class-based toolbar. |
| client/app/bundles/course/assessment/submission/components/ScribingView/ScribingCanvas.tsx | New TSX functional canvas with Fabric v7 async hydration + imperative handle. |
| client/app/bundles/course/assessment/submission/components/ScribingView/ScribingCanvas.jsx | Removes legacy class-based canvas. |
| client/app/bundles/course/assessment/submission/components/ScribingView/LayersComponent.tsx | New TSX functional layers popover component. |
| client/app/bundles/course/assessment/submission/components/ScribingView/LayersComponent.jsx | Removes legacy class-based layers component. |
| client/app/bundles/course/assessment/submission/components/ScribingView/index.tsx | Lifts and shares the canvas imperative ref; lazy-loads new TSX components. |
| client/app/bundles/course/assessment/submission/components/ScribingView/index.jsx | Removes legacy ScribingView component wrapper. |
| client/app/bundles/course/assessment/submission/components/ScribingView/fields/ShapeField.tsx | Converts shape selector field to TSX + typed shape unions. |
| client/app/bundles/course/assessment/submission/components/ScribingView/fields/ShapeField.jsx | Removes legacy JSX shape field. |
| client/app/bundles/course/assessment/submission/components/ScribingView/fields/ColorPickerField.jsx | Fixes “no fill” toggle to correctly restore alpha on uncheck. |
| client/app/bundles/course/assessment/submission/components/ScribingView/test/index.test.tsx | Updates test to use new scribingActions API. |
| client/app/bundles/course/assessment/submission/actions/index.js | Dispatches scribingActions.initialize during submission fetch. |
| client/app/bundles/course/assessment/submission/actions/answers/scribing.ts | New thunk for updating scribing answer via API using RTK slice status flags. |
| client/app/bundles/course/assessment/submission/actions/answers/scribing.js | Removes legacy scribing action creators module. |
Comments suppressed due to low confidence (1)
client/app/bundles/course/assessment/submission/actions/index.js:104
scribingActions.initializeis dispatched onfetchSubmission, but the scribing state was previously also re-initialized on other submission lifecycle successes (e.g., finalise/unsubmit) when the server returns a freshdata.answerspayload. Consider dispatchingscribingActions.initialize({ answers: data.answers })in those success paths too, otherwise the scribing slice can become stale relative toFETCH_SUBMISSION_SUCCESSpayloads.
dispatch(
historyActions.initSubmissionHistory({
submissionId: data.submission.id,
questionHistories: data.history.questions,
questions: data.questions,
}),
);
dispatch(scribingActions.initialize({ answers: data.answers }));
dispatch(initiateAnswerFlagsForAnswers({ answers: data.answers }));
dispatch(
initiateLiveFeedbackChatPerQuestion({
answerIds: data.answers.map((answer) => answer.id),
}),
);
})
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const ScribingToolbar: FC<ScribingToolbarProps> = ({ answerId, canvasRef }) => { | ||
| const scribings = useAppSelector( | ||
| (state) => state.assessments.submission.scribing, | ||
| ); | ||
| const scribing = scribings[answerId]; | ||
| const dispatch = useAppDispatch(); | ||
| const { t } = useTranslation(); |
There was a problem hiding this comment.
scribing can be undefined on initial render (e.g., before scribingActions.initialize runs). This component dereferences scribing.* throughout (e.g., zoom handlers) without a guard, which will throw. Add an early return/loading state when scribing is missing (or provide a selector default).
| const [popoverAnchor, setPopoverAnchor] = useState<HTMLElement | null>(null); | ||
| const [, forceUpdate] = useReducer((x: number) => x + 1, 0); | ||
|
|
||
| useEffect(() => canvasRef?.onSelectionChange(forceUpdate), [canvasRef]); |
There was a problem hiding this comment.
The selection-change subscription created in useEffect isn't cleaned up. onSelectionChange returns an unsubscribe function, but the effect currently ignores it, which can leak listeners and cause duplicate forceUpdate calls if canvasRef changes or the toolbar unmounts. Return the unsubscribe function from the effect.
| useEffect(() => canvasRef?.onSelectionChange(forceUpdate), [canvasRef]); | |
| useEffect(() => { | |
| const unsubscribe = canvasRef?.onSelectionChange(forceUpdate); | |
| return unsubscribe; | |
| }, [canvasRef]); |
| // Create layer for each user's scribble | ||
| // Scribbles in layers have selection disabled | ||
| if (scribble.creator_id !== userId) { | ||
| const scribbleGroup = new Group(fabricObjs); |
There was a problem hiding this comment.
getFabricObjectsFromJson can return undefined, but new Group(fabricObjs) is called without a fallback. If a scribble has empty/invalid content, this will throw. Default fabricObjs to an empty array (or skip creating the layer) before constructing the Group.
| const scribbleGroup = new Group(fabricObjs); | |
| const scribbleGroup = new Group(fabricObjs ?? []); |
| container.tabIndex = 1000; | ||
| container.addEventListener('keydown', onKeyDown, false); | ||
|
|
||
| let rafId: number; |
There was a problem hiding this comment.
A keydown listener is added to container but never removed in the cleanup function. This can leak handlers across mounts and cause duplicate keyboard handling. Remove the event listener in the effect cleanup (and ensure the same handler reference is used).
| const scribblesAsJson = ( | ||
| canvas: Canvas, | ||
| scribing: ScribingAnswerState, | ||
| ): string => { | ||
| // Remove non-user scribings in canvas | ||
| layers.current.forEach((layer) => { | ||
| if (layer.creator_id !== scribing.answer.user_id) { | ||
| layer.scribbleGroup.set({ visible: false }); | ||
| } | ||
| }); | ||
| canvas.renderAll(); | ||
|
|
||
| // Only save rescaled user scribings | ||
| const objects = canvas.getObjects(); | ||
| objects.forEach((obj) => { | ||
| normaliseScribble(canvas, obj); | ||
| }); | ||
| const json = JSON.stringify(objects); | ||
|
|
There was a problem hiding this comment.
scribblesAsJson only hides non-user layer groups (visible: false) but still serializes canvas.getObjects() with JSON.stringify. Hidden layer groups will still be included in the saved JSON because they remain in the objects array. To ensure only the current user's objects are persisted, exclude layer groups from export (e.g., set excludeFromExport on layer groups) or filter them out of the serialized list.
| dispatch({ | ||
| type: actionTypes.FETCH_SUBMISSION_SUCCESS, | ||
| payload: mockSubmission, | ||
| }); | ||
|
|
||
| const loaded = true; | ||
| const url = `/courses/${global.courseId}/assessments/${assessmentId}/submissions/${submissionId}/edit`; | ||
|
|
||
| dispatch(setCanvasLoaded(answerId, loaded)); | ||
| dispatch(scribingActions.setCanvasLoaded({ answerId, loaded })); | ||
|
|
||
| const page = render(<ScribingView answerId={answerId} />, { at: [url] }); |
There was a problem hiding this comment.
This test dispatches FETCH_SUBMISSION_SUCCESS directly, but the new scribing slice is initialized via scribingActions.initialize (dispatched by fetchSubmission), not by handling FETCH_SUBMISSION_SUCCESS. As a result, setCanvasLoaded is a no-op because state.scribing[answerId] doesn't exist. Initialize the scribing slice in the test before calling setCanvasLoaded (or dispatch fetchSubmission with a mocked API response).
| @@ -76,52 +82,11 @@ export const scribingTools = mirrorCreator([ | |||
| 'ZOOM_IN', | |||
| 'ZOOM_OUT', | |||
| 'DELETE', | |||
| ]); | |||
| ] as const; | |||
| export type ScribingTool = (typeof SCRIBING_TOOLS)[number]; | |||
|
|
|||
| export const scribingShapes = mirrorCreator(['RECT', 'ELLIPSE']); | |||
|
|
|||
| export const canvasActionTypes = mirrorCreator([ | |||
| 'SET_CANVAS', | |||
| 'SET_CANVAS_LOADED', | |||
| 'SET_TOOL_SELECTED', | |||
| 'SET_FONT_FAMILY', | |||
| 'SET_FONT_SIZE', | |||
| 'SET_LINE_STYLE_CHIP', | |||
| 'SET_COLORING_TOOL_COLOR', | |||
| 'SET_TOOL_THICKNESS', | |||
| 'SET_SELECTED_SHAPE', | |||
| 'SET_NO_FILL', | |||
| 'SET_CANVAS_LOADED', | |||
| 'OPEN_POPOVER', | |||
| 'CLOSE_POPOVER', | |||
| 'ADD_LAYER', | |||
| 'SET_LAYER_DISPLAY', | |||
| 'SET_CANVAS_PROPERTIES', | |||
| 'SET_DRAWING_MODE', | |||
| 'SET_CANVAS_CURSOR', | |||
| 'SET_CURRENT_STATE_INDEX', | |||
| 'SET_CANVAS_STATES', | |||
| 'UPDATE_CANVAS_STATE', | |||
| 'SET_ACTIVE_OBJECT', | |||
| 'SET_CANVAS_ZOOM', | |||
| 'RESET_CHANGE_TOOL', | |||
| 'DELETE_CANVAS_OBJECT', | |||
| 'RESET_CANVAS_DELETE', | |||
| 'SET_DISABLE_OBJECT_SELECTION', | |||
| 'RESET_DISABLE_OBJECT_SELECTION', | |||
| 'SET_ENABLE_OBJECT_SELECTION', | |||
| 'RESET_ENABLE_OBJECT_SELECTION', | |||
| 'SET_ENABLE_TEXT_SELECTION', | |||
| 'RESET_ENABLE_TEXT_SELECTION', | |||
| 'SET_CANVAS_DIRTY', | |||
| 'RESET_CANVAS_DIRTY', | |||
| 'SET_CANVAS_SAVE', | |||
| 'RESET_CANVAS_SAVE', | |||
| 'SET_UNDO', | |||
| 'RESET_UNDO', | |||
| 'SET_REDO', | |||
| 'RESET_REDO', | |||
| ]); | |||
| const SCRIBING_SHAPES = ['RECT', 'ELLIPSE'] as const; | |||
| export type ScribingShape = (typeof SCRIBING_SHAPES)[number]; | |||
|
|
|||
There was a problem hiding this comment.
This change removes/renames the previously exported scribing constant objects (e.g., scribingTools, scribingToolColor, scribingToolThickness, scribingToolLineStyle, scribingShapes, canvasActionTypes). There are still references in the repo (e.g., components/ScribingView/__test__/ScribingToolbar.test.jsx) that will break until updated (or until equivalent runtime exports are restored).
Migration of scribing question components from fabric.js v5.3.0 to v7.2.0. This addresses several vulnerabilities associated with the
node-tarpackage (fabric v7 removes this from the dependency tree entirely).Associated Redux state reducer and several top-level components (
ScribingCanvas,ScribingToolbar,LayersComponent) also migrated from JSX class components to TSX functional components.Sample of new submission with Fabric v7.2.0 objects:
Database view of above:
Sample of existing submission with Fabric v5.3.0 objects opened with Fabric 7.2.0 frontend:
Note that no database migrations were needed to maintain backward compatibility.
ScribingCanvas.jsx → ScribingCanvas.tsx
constructoruseRefhooks for shape/flag trackingcurrentStateIndexgetterscribingRef.current.currentStateIndexcanvasStatesgetterscribingRef.current.canvasStatescomponentDidMountuseEffect([answerId])shouldComponentUpdateuseEffecthooksdeleteActiveObjectsonKeyDownonMouseDownCanvasscribingRef.current = scribingState[answerId]during render)onMouseMoveCanvasonMouseOutonMouseOveronMouseUpCanvasonObjectMovingCanvasgetBoundingRect()offset — works for anyoriginX/originYonObjectSelectedonSelectionClearedonTextChangedscribingRef.currentmutation to fix Fabric v7 event ordering (text:editing:exitednow fires beforemouse:down)getCanvasPointgetFabricObjectsFromJsonfromObjectreturns a Promise)getMousePointscribblesAsJsongetterlayer.scribbleGroup.set({ visible })setCopiedCanvasObjectPositionrehydrateCanvasbackgroundImageacrosscanvas.clear()setCanvasStateAndUpdateAnswercloneTextdenormaliseScribbledisableObjectSelectionenableObjectSelectionselectableon existing canvas objects — avoids JSON round-trip that caused stale re-render when closing style popoversgenerateMouseDragPropertiesinitializeCanvasuseEffect;canvas.dispose()nowawaited — Fabric v7dispose()is async (fixesinsertBeforeDOM error on first load)initializeScribblesAndBackgroundnormaliseScribbleupdateAnswersaveScribblesisSavingScribblesre-entrant guard — Fabric v7obj.set()firesobject:modified, causing an infinite loop not present in old componentscaleCanvaszoomon canvas wrapper viaResizeObserver— scales dynamically without re-encoding canvas objectsundoredorenderScribingToolbar.jsx → ScribingToolbar.tsx
initializeColorDropdownsinitializePopoversconstructoruseStatehooks;forceUpdateviauseReducerfor selection and layer changesonChangeCompleteColoronChangeFontFamilyonChangeFontSizeonChangeSliderThicknessonClickColorPickeronClickDeleteonClickDrawingModeonClickLineModeonClickLineStyleChiponClickMoveModeonClickPopoveronClickRedoonClickSelectionModeonClickShapeModeonClickTypingChevrononClickTypingIcononClickTypingModeonClickUndoonClickZoomInonClickZoomOutonRequestCloseColorPickeronRequestClosePopovergetActiveObjectSelectedLineStylegetSavingStatussetSelectedShapesetToSelectToolrenderOther Significant Changes
Fabric v7 shifts the default
originX,originYvalues fromleft,toptocenter,centerrespectively.There are proposals to deprecate these fields entirely, so we are making the shift now to make future transitions to Fabric v8+ easier.
activeObjectno longer tracked in Redux. Exposed directly viaScribingCanvasRef.getActiveObject(), re-read on each render triggered byonSelectionChange.selection:created/updated/clearedevents drive a pub/subonSelectionChangecallback onScribingCanvasRef, which the toolbar subscribes to viauseEffectto trigger aforceUpdate.layerstracking moved from Redux state to an internaluseRefinScribingCanvas.Lifted to
index.tsx.ScribingCanvasusesforwardRef+useImperativeHandle, exposingScribingCanvasRef(getActiveObject,getCanvasWidth,getLayers,setLayerDisplay,onSelectionChange).All 13 canvas interaction-tracking variables (
mouseDownFlag,line,rect, etc.) converted fromuseState→useRef. Redux state accessed via sync-ref pattern (scribingRef.current = scribingState[answerId]during render).withCanvasHOFGuards every canvas event handler and
useEffectagainst missingcanvas/scribing. Returns a no-op if either is absent. AllowsuseEffect(withCanvas(...), [deps])directly.CSS
zoomon canvas wrapper div, updated by aResizeObserver+requestAnimationFramedebounce (avoids "ResizeObserver loop" errors). ReplacesscaleCanvas(). This allows scribing element to resize dynamically.forceUpdateviauseReducerin toolbar triggers re-read ofcanvasRef.getLayers()aftersetLayerDisplay.ColorPickerField)Now restores fill alpha to 1 when "no fill" is unchecked (previously only set alpha to 0 on check). Existing-shape
ShapePopoverderivesnoFillValuefromactiveObject.fillregex rather than the stalescribing.hasNoFillflag.Remaining Work