diff --git a/.oxlintrc.json b/.oxlintrc.json index 42fc478ed9..349f3eea59 100644 --- a/.oxlintrc.json +++ b/.oxlintrc.json @@ -121,7 +121,8 @@ "ignoreRestSiblings": true } ], - "react/rules-of-hooks": "error" + "react/rules-of-hooks": "error", + "react-hooks/exhaustive-deps": "error" }, "plugins": ["eslint", "oxc", "react", "typescript", "import"] }, diff --git a/app/components/Collection/CollectionForm.tsx b/app/components/Collection/CollectionForm.tsx index 124d050d53..6f433eeedf 100644 --- a/app/components/Collection/CollectionForm.tsx +++ b/app/components/Collection/CollectionForm.tsx @@ -54,6 +54,9 @@ const useIconColor = (collection?: Collection) => { (hasMultipleCollections && collectionColors.length === 1 ? collectionColors[0] : randomElement(colorPalette)), + // Deliberately only keyed on the collection color so the randomly picked + // fallback stays stable while the form is open. + // eslint-disable-next-line react-hooks/exhaustive-deps [collection?.color] ); return iconColor; diff --git a/app/components/DocumentViews.tsx b/app/components/DocumentViews.tsx index 820a66304f..cb90456780 100644 --- a/app/components/DocumentViews.tsx +++ b/app/components/DocumentViews.tsx @@ -8,6 +8,7 @@ import type User from "~/models/User"; import { Avatar, AvatarSize } from "~/components/Avatar"; import ListItem from "~/components/List/Item"; import PaginatedList from "~/components/PaginatedList"; +import { useComputed } from "~/hooks/useComputed"; import useCurrentUser from "~/hooks/useCurrentUser"; import useStores from "~/hooks/useStores"; @@ -20,23 +21,26 @@ function DocumentViews({ document }: Props) { const { views, presence } = useStores(); const user = useCurrentUser(); const locale = dateLocale(user.language); - const documentPresence = presence.get(document.id); - const documentPresenceArray = documentPresence - ? Array.from(documentPresence.values()) - : []; - - // Use Set for O(1) lookups and stable references - const presentIds = useMemo( - () => new Set(documentPresenceArray.map((p) => p.userId)), - [documentPresenceArray] - ); - const editingIds = useMemo( - () => - new Set( - documentPresenceArray.filter((p) => p.isEditing).map((p) => p.userId) - ), - [documentPresenceArray] - ); + // Use Set for O(1) lookups, computed so the identity is only replaced when + // the observable presence for the document actually changes. + const presentIds = useComputed(() => { + const documentPresence = presence.get(document.id); + return new Set( + documentPresence + ? Array.from(documentPresence.values()).map((p) => p.userId) + : [] + ); + }, [presence, document.id]); + const editingIds = useComputed(() => { + const documentPresence = presence.get(document.id); + return new Set( + documentPresence + ? Array.from(documentPresence.values()) + .filter((p) => p.isEditing) + .map((p) => p.userId) + : [] + ); + }, [presence, document.id]); // ensure currently present via websocket are always ordered first const documentViews = useMemo( diff --git a/app/components/Lightbox.tsx b/app/components/Lightbox.tsx index ac447f9909..9834a3716c 100644 --- a/app/components/Lightbox.tsx +++ b/app/components/Lightbox.tsx @@ -15,6 +15,7 @@ import { useContext, useEffect, useMemo, + useReducer, useRef, useState, } from "react"; @@ -65,28 +66,13 @@ import { useDocumentContext } from "./DocumentContext"; import LightboxComments from "~/scenes/Document/components/Comments/LightboxComments"; import { PortalContext } from "./Portal"; import useHideElement from "~/hooks/useHideElement"; - -export enum LightboxStatus { - READY_TO_OPEN, - OPENING, - OPENED, - READY_TO_CLOSE, - CLOSING, - CLOSED, -} - -export enum ImageStatus { - LOADING, - ERROR, - LOADED, - MIN_ZOOM, - MAX_ZOOM, - ZOOMED, -} -type Status = { - lightbox: LightboxStatus | null; - image: ImageStatus | null; -}; +import type { Status } from "./LightboxState"; +import { + ImageStatus, + LightboxStatus, + initialStatus, + reducer, +} from "./LightboxState"; type Animation = { fadeIn?: { apply: () => Keyframes; duration: number }; @@ -98,6 +84,35 @@ type Animation = { const ANIMATION_DURATION = 0.3 * Second.ms; +/** + * Converts an SVG data URL, as produced by the diagram and mermaid embeds, into + * a blob that can be downloaded. + * + * @param dataURL The data URL to convert. + * @returns The blob, or undefined if the URL is not an SVG data URL. + */ +const svgDataURLToBlob = (dataURL: string) => { + // Match the SVG data URL format (with or without charset) + const match = dataURL.match( + /^data:image\/svg\+xml(?:;charset=utf-8)?,(.*)$/i + ); + if (!match) { + return; + } + + const encodedSVGData = match[1]; + const decodedSVGData = decodeURIComponent(encodedSVGData); + + // Convert string to Uint8Array + const uint8 = new Uint8Array(decodedSVGData.length); + for (let i = 0; i < decodedSVGData.length; ++i) { + uint8[i] = decodedSVGData.charCodeAt(i); + } + + // Create and return the Blob + return new Blob([uint8], { type: "image/svg+xml" }); +}; + /** * Stops a React synthetic event from propagating to ancestor handlers, including * Radix Dialog's outside-interaction detection and the editor's own click @@ -243,7 +258,7 @@ function Lightbox({ images, activeImage, onUpdate, onClose, readOnly }: Props) { const imgRef = useRef(null); const overlayRef = useRef(null); const contentRef = useRef(null); - const [status, setStatus] = useState({ lightbox: null, image: null }); + const [status, dispatch] = useReducer(reducer, initialStatus); const [commentsOpen, setCommentsOpen] = useState(false); const [commentsRendered, setCommentsRendered] = useState(false); const [commentsVisible, setCommentsVisible] = useState(false); @@ -267,111 +282,39 @@ function Lightbox({ images, activeImage, onUpdate, onClose, readOnly }: Props) { (img) => img.pos === activeImage.pos ); - // Debugging status changes - // useEffect(() => { - // console.log( - // `lstat:${status.lightbox === null ? status.lightbox : LightboxStatus[status.lightbox]}, istat:${status.image === null ? status.image : ImageStatus[status.image]}` - // ); - // }, [status]); - - useEffect( - () => () => { - if (status.lightbox === LightboxStatus.CLOSED) { - onClose(); - } - }, - [status.lightbox] + const handleImageLoading = useCallback( + () => dispatch({ type: "imageLoading" }), + [] + ); + const handleImageLoad = useCallback( + () => dispatch({ type: "imageLoaded" }), + [] + ); + const handleImageError = useCallback( + () => dispatch({ type: "imageErrored" }), + [] + ); + const handleMinZoom = useCallback( + () => dispatch({ type: "zoomChanged", zoom: ImageStatus.MIN_ZOOM }), + [] + ); + const handleZoom = useCallback( + () => dispatch({ type: "zoomChanged", zoom: ImageStatus.ZOOMED }), + [] + ); + const handleMaxZoom = useCallback( + () => dispatch({ type: "zoomChanged", zoom: ImageStatus.MAX_ZOOM }), + [] ); + // Keep the latest callbacks in a ref so the close sequence can fire them + // without re-running when the caller passes new function identities. + const callbacks = useRef({ onUpdate, onClose }); useEffect(() => { - setStatus({ - lightbox: LightboxStatus.READY_TO_OPEN, - image: status.image, - }); - }, []); + callbacks.current = { onUpdate, onClose }; + }); - useEffect(() => { - if (status.image === ImageStatus.LOADED) { - rememberImagePosition(); - } - }, [status.image]); - - useEffect(() => { - if ( - (status.image === ImageStatus.ERROR || - status.image === ImageStatus.LOADED) && - status.lightbox === LightboxStatus.READY_TO_OPEN - ) { - setupFadeIn(); - setupZoomIn(); - setStatus({ - lightbox: LightboxStatus.OPENING, - image: status.image, - }); - } - }, [status.image, status.lightbox]); - - useEffect(() => { - if ( - status.lightbox === LightboxStatus.OPENED && - status.image === ImageStatus.LOADED - ) { - setStatus({ - lightbox: LightboxStatus.OPENED, - image: ImageStatus.MIN_ZOOM, - }); - } - }, [status.lightbox, status.image]); - - useEffect(() => { - if (status.lightbox === LightboxStatus.READY_TO_CLOSE) { - setupFadeOut(); - setupZoomOut(); - setStatus({ - lightbox: LightboxStatus.CLOSING, - image: status.image, - }); - } - }, [status.lightbox]); - - useEffect(() => { - if (status.lightbox === LightboxStatus.CLOSED) { - onUpdate(null); - } - }, [status.lightbox]); - - useEffect(() => { - if (commentsOpen) { - setCommentsRendered(true); - const frame = window.requestAnimationFrame(() => - setCommentsVisible(true) - ); - return () => window.cancelAnimationFrame(frame); - } - setCommentsVisible(false); - const timer = window.setTimeout(() => setCommentsRendered(false), 200); - return () => window.clearTimeout(timer); - }, [commentsOpen]); - - useEffect(() => { - if (status.image === ImageStatus.MIN_ZOOM) { - // It was observed that focus went to `body` as the zoom out button was disabled - // upon clicking it. This stopped navigating to next/previous image using arrow keys. - // So focusing the content div here to restore the functionality. - contentRef.current?.focus(); - } - }, [status.image]); - - // Hide the inline image in the editor while the lightbox zoom transition is - // active, otherwise a duplicate is visible behind the fading overlay. - useHideElement( - activeImage.getElement(), - status.lightbox !== null && - status.lightbox !== LightboxStatus.READY_TO_OPEN && - status.lightbox !== LightboxStatus.CLOSED - ); - - const rememberImagePosition = () => { + const rememberImagePosition = useCallback(() => { if (imgRef.current) { const lightboxImgDOMRect = imgRef.current.getBoundingClientRect(); const { @@ -389,9 +332,9 @@ function Lightbox({ images, activeImage, onUpdate, onClose, readOnly }: Props) { height: lightboxImgHeight, }; } - }; + }, []); - const setupZoomIn = () => { + const setupZoomIn = useCallback(() => { if (imgRef.current) { // in editor const editorImageEl = activeImage.getElement(); @@ -452,9 +395,9 @@ function Lightbox({ images, activeImage, onUpdate, onClose, readOnly }: Props) { zoomIn: { apply: zoomIn, duration: ANIMATION_DURATION }, }; } - }; + }, [activeImage]); - const setupFadeIn = () => { + const setupFadeIn = useCallback(() => { const fadeIn = () => keyframes` from { opacity: 0; } to { opacity: 1; } @@ -464,9 +407,9 @@ function Lightbox({ images, activeImage, onUpdate, onClose, readOnly }: Props) { fadeIn: { apply: fadeIn, duration: ANIMATION_DURATION }, fadeOut: undefined, }; - }; + }, []); - const setupFadeOut = () => { + const setupFadeOut = useCallback(() => { const fadeOut = () => keyframes` from { opacity: ${overlayRef.current ? window.getComputedStyle(overlayRef.current).opacity : 1}; } to { opacity: 0; } @@ -481,9 +424,9 @@ function Lightbox({ images, activeImage, onUpdate, onClose, readOnly }: Props) { : ANIMATION_DURATION, }, }; - }; + }, []); - const setupZoomOut = () => { + const setupZoomOut = useCallback(() => { if ( imgRef.current && !( @@ -584,7 +527,98 @@ function Lightbox({ images, activeImage, onUpdate, onClose, readOnly }: Props) { }, }; } - }; + }, [activeImage, status.image, rememberImagePosition]); + + useEffect(() => { + dispatch({ type: "mounted" }); + }, []); + + // Notify the caller once a completed close unmounts the lightbox. The cleanup + // captures the status at the time it was registered, so it only fires when + // the lightbox is torn down having already reached CLOSED. + useEffect( + () => () => { + if (status.lightbox === LightboxStatus.CLOSED) { + callbacks.current.onClose(); + } + }, + [status.lightbox] + ); + + // The image position and the opening keyframes are measured after the loaded + // image has been laid out, so they cannot be folded into the reducer. + useEffect(() => { + // MIN_ZOOM as well as LOADED, as the status settles straight to MIN_ZOOM + // when navigating to another image with the lightbox already open. Both + // mean the image is resting at its natural size, which is what is cached. + if ( + status.image === ImageStatus.LOADED || + status.image === ImageStatus.MIN_ZOOM + ) { + rememberImagePosition(); + } + + if ( + (status.image === ImageStatus.ERROR || + status.image === ImageStatus.LOADED) && + status.lightbox === LightboxStatus.READY_TO_OPEN + ) { + setupFadeIn(); + setupZoomIn(); + dispatch({ type: "openAnimationPrepared" }); + } + }, [ + status.image, + status.lightbox, + rememberImagePosition, + setupFadeIn, + setupZoomIn, + ]); + + useEffect(() => { + if (status.lightbox === LightboxStatus.READY_TO_CLOSE) { + setupFadeOut(); + setupZoomOut(); + dispatch({ type: "closeAnimationPrepared" }); + } + }, [status.lightbox, setupFadeOut, setupZoomOut]); + + useEffect(() => { + if (status.lightbox === LightboxStatus.CLOSED) { + callbacks.current.onUpdate(null); + } + }, [status.lightbox]); + + useEffect(() => { + if (commentsOpen) { + setCommentsRendered(true); + const frame = window.requestAnimationFrame(() => + setCommentsVisible(true) + ); + return () => window.cancelAnimationFrame(frame); + } + setCommentsVisible(false); + const timer = window.setTimeout(() => setCommentsRendered(false), 200); + return () => window.clearTimeout(timer); + }, [commentsOpen]); + + useEffect(() => { + if (status.image === ImageStatus.MIN_ZOOM) { + // It was observed that focus went to `body` as the zoom out button was disabled + // upon clicking it. This stopped navigating to next/previous image using arrow keys. + // So focusing the content div here to restore the functionality. + contentRef.current?.focus(); + } + }, [status.image]); + + // Hide the inline image in the editor while the lightbox zoom transition is + // active, otherwise a duplicate is visible behind the fading overlay. + useHideElement( + activeImage.getElement(), + status.lightbox !== null && + status.lightbox !== LightboxStatus.READY_TO_OPEN && + status.lightbox !== LightboxStatus.CLOSED + ); const prev = () => { if ( @@ -614,76 +648,49 @@ function Lightbox({ images, activeImage, onUpdate, onClose, readOnly }: Props) { } }; - const close = () => { - if ( - status.lightbox === LightboxStatus.OPENING || - status.lightbox === LightboxStatus.OPENED - ) { - setStatus({ - lightbox: LightboxStatus.READY_TO_CLOSE, - image: status.image, - }); - } - }; + const close = useCallback(() => { + dispatch({ type: "closeRequested" }); + }, []); - const svgDataURLToBlob = (dataURL: string) => { - // Match the SVG data URL format (with or without charset) - const match = dataURL.match( - /^data:image\/svg\+xml(?:;charset=utf-8)?,(.*)$/i - ); - if (!match) { - return; - } + const downloadImage = useCallback( + async (src: string, saveAs: string) => { + let imageBlob; + if (isInternalUrl(src)) { + const image = await fetch(src); + imageBlob = await image.blob(); + } else { + // Assuming it's a mermaid svg + imageBlob = svgDataURLToBlob(src); + } - const encodedSVGData = match[1]; - const decodedSVGData = decodeURIComponent(encodedSVGData); + if (!imageBlob) { + toast.error(t("Unable to download image")); + return; + } - // Convert string to Uint8Array - const uint8 = new Uint8Array(decodedSVGData.length); - for (let i = 0; i < decodedSVGData.length; ++i) { - uint8[i] = decodedSVGData.charCodeAt(i); - } + const imageURL = URL.createObjectURL(imageBlob); + const name = saveAs || "image"; + const extension = imageBlob.type.split(/\/|\+/g)[1]; - // Create and return the Blob - return new Blob([uint8], { type: "image/svg+xml" }); - }; + // create a temporary link node and click it with our image data + const link = document.createElement("a"); + link.href = imageURL; + link.download = `${name}.${extension}`; + document.body.appendChild(link); + link.click(); - const downloadImage = async (src: string, saveAs: string) => { - let imageBlob; - if (isInternalUrl(src)) { - const image = await fetch(src); - imageBlob = await image.blob(); - } else { - // Assuming it's a mermaid svg - imageBlob = svgDataURLToBlob(src); - } - - if (!imageBlob) { - toast.error(t("Unable to download image")); - return; - } - - const imageURL = URL.createObjectURL(imageBlob); - const name = saveAs || "image"; - const extension = imageBlob.type.split(/\/|\+/g)[1]; - - // create a temporary link node and click it with our image data - const link = document.createElement("a"); - link.href = imageURL; - link.download = `${name}.${extension}`; - document.body.appendChild(link); - link.click(); - - // cleanup - document.body.removeChild(link); - URL.revokeObjectURL(imageURL); - }; + // cleanup + document.body.removeChild(link); + URL.revokeObjectURL(imageURL); + }, + [t] + ); const handleDownload = useCallback(() => { if (activeImage && status.lightbox === LightboxStatus.OPENED) { void downloadImage(activeImage.src, activeImage.alt); } - }, [activeImage, status.lightbox]); + }, [activeImage, status.lightbox, downloadImage]); const handleKeyDown = (ev: React.KeyboardEvent) => { // Don't intercept keys while typing into an input, textarea, or editor. @@ -733,26 +740,20 @@ function Lightbox({ images, activeImage, onUpdate, onClose, readOnly }: Props) { fadeIn: undefined, startTime: undefined, }; - setStatus({ - lightbox: LightboxStatus.OPENED, - image: status.image, - }); + dispatch({ type: "openAnimationEnded" }); } else if (animation.current?.fadeOut) { - setStatus({ - lightbox: LightboxStatus.CLOSED, - image: null, - }); + dispatch({ type: "closeAnimationEnded" }); } }; const handleEditDiagram = () => { - const { state, dispatch } = editor.view; + const { state, dispatch: dispatchTransaction } = editor.view; // Select the node at the position const tr = state.tr.setSelection( NodeSelection.create(state.doc, activeImage.pos) ); - dispatch(tr); + dispatchTransaction(tr); editor.commands.editDiagram(); }; @@ -918,48 +919,18 @@ function Lightbox({ images, activeImage, onUpdate, onClose, readOnly }: Props) { ref={imgRef} src={activeImage.src} alt={activeImage.alt} - onLoading={() => - setStatus({ - lightbox: status.lightbox, - image: ImageStatus.LOADING, - }) - } - onLoad={() => - setStatus({ - lightbox: status.lightbox, - image: ImageStatus.LOADED, - }) - } - onError={() => - setStatus({ - lightbox: status.lightbox, - image: ImageStatus.ERROR, - }) - } + onLoading={handleImageLoading} + onLoad={handleImageLoad} + onError={handleImageError} onSwipeRight={prev} onSwipeLeft={next} onSwipeUp={close} onSwipeDown={close} status={status} animation={animation.current} - onMinZoom={() => { - setStatus({ - lightbox: status.lightbox, - image: ImageStatus.MIN_ZOOM, - }); - }} - onZoom={() => - setStatus({ - lightbox: status.lightbox, - image: ImageStatus.ZOOMED, - }) - } - onMaxZoom={() => - setStatus({ - lightbox: status.lightbox, - image: ImageStatus.MAX_ZOOM, - }) - } + onMinZoom={handleMinZoom} + onZoom={handleZoom} + onMaxZoom={handleMaxZoom} /> {currentImageIndex < images.length - 1 && @@ -1073,14 +1044,13 @@ const Image = forwardRef(function Image_( useEffect(() => { onLoading(); - }, [src]); + }, [src, onLoading]); + // Anything past loading means there is something to show. This deliberately + // does not test for LOADED specifically, as the status can settle straight to + // MIN_ZOOM when the lightbox is already open and a new image is navigated to. useEffect(() => { - if (status.image === null || status.image === ImageStatus.LOADING) { - setHidden(true); - } else if (status.image === ImageStatus.LOADED) { - setHidden(false); - } + setHidden(status.image === null || status.image === ImageStatus.LOADING); }, [status.image]); return status.image === ImageStatus.ERROR ? ( diff --git a/app/components/LightboxState.test.ts b/app/components/LightboxState.test.ts new file mode 100644 index 0000000000..dfc5dbf5fe --- /dev/null +++ b/app/components/LightboxState.test.ts @@ -0,0 +1,145 @@ +import type { Action } from "./LightboxState"; +import { + ImageStatus, + LightboxStatus, + initialStatus, + reducer, +} from "./LightboxState"; + +/** + * Applies a sequence of actions to the initial status. + * + * @param actions The actions to apply in order. + * @returns The resulting status. + */ +const run = (...actions: Action[]) => actions.reduce(reducer, initialStatus); + +describe("Lightbox state", () => { + it("opens once the image has loaded and the animation has run", () => { + const status = run( + { type: "mounted" }, + { type: "imageLoading" }, + { type: "imageLoaded" }, + { type: "openAnimationPrepared" }, + { type: "openAnimationEnded" } + ); + + expect(status).toEqual({ + lightbox: LightboxStatus.OPENED, + image: ImageStatus.MIN_ZOOM, + }); + }); + + it("opens when the image fails to load", () => { + const status = run( + { type: "mounted" }, + { type: "imageLoading" }, + { type: "imageErrored" }, + { type: "openAnimationPrepared" }, + { type: "openAnimationEnded" } + ); + + expect(status).toEqual({ + lightbox: LightboxStatus.OPENED, + image: ImageStatus.ERROR, + }); + }); + + it("settles at minimum zoom when the image loads after opening", () => { + const opened = run( + { type: "mounted" }, + { type: "imageLoading" }, + { type: "imageLoaded" }, + { type: "openAnimationPrepared" }, + { type: "openAnimationEnded" } + ); + + // Navigating to another image while already open. + const loading = reducer(opened, { type: "imageLoading" }); + expect(loading.image).toBe(ImageStatus.LOADING); + + const loaded = reducer(loading, { type: "imageLoaded" }); + expect(loaded).toEqual({ + lightbox: LightboxStatus.OPENED, + image: ImageStatus.MIN_ZOOM, + }); + }); + + it("preserves the lightbox status when the zoom changes", () => { + const opened = run( + { type: "mounted" }, + { type: "imageLoading" }, + { type: "imageLoaded" }, + { type: "openAnimationPrepared" }, + { type: "openAnimationEnded" } + ); + + const zoomed = reducer(opened, { + type: "zoomChanged", + zoom: ImageStatus.MAX_ZOOM, + }); + + expect(zoomed).toEqual({ + lightbox: LightboxStatus.OPENED, + image: ImageStatus.MAX_ZOOM, + }); + }); + + it("closes through to a final closed status", () => { + const opened = run( + { type: "mounted" }, + { type: "imageLoading" }, + { type: "imageLoaded" }, + { type: "openAnimationPrepared" }, + { type: "openAnimationEnded" } + ); + + const requested = reducer(opened, { type: "closeRequested" }); + expect(requested.lightbox).toBe(LightboxStatus.READY_TO_CLOSE); + + const closing = reducer(requested, { type: "closeAnimationPrepared" }); + expect(closing.lightbox).toBe(LightboxStatus.CLOSING); + + expect(reducer(closing, { type: "closeAnimationEnded" })).toEqual({ + lightbox: LightboxStatus.CLOSED, + image: null, + }); + }); + + it("ignores a close requested before the lightbox has opened", () => { + const readyToOpen = reducer(initialStatus, { type: "mounted" }); + + expect(reducer(readyToOpen, { type: "closeRequested" })).toBe(readyToOpen); + }); + + it("ignores a repeated close request while already closing", () => { + const closing = run( + { type: "mounted" }, + { type: "imageLoading" }, + { type: "imageLoaded" }, + { type: "openAnimationPrepared" }, + { type: "openAnimationEnded" }, + { type: "closeRequested" }, + { type: "closeAnimationPrepared" } + ); + + expect(reducer(closing, { type: "closeRequested" })).toBe(closing); + expect(reducer(closing, { type: "closeAnimationPrepared" })).toBe(closing); + }); + + it("does not reopen when a slow image load lands after closing started", () => { + const closing = run( + { type: "mounted" }, + { type: "imageLoading" }, + { type: "imageLoaded" }, + { type: "openAnimationPrepared" }, + { type: "openAnimationEnded" }, + { type: "closeRequested" } + ); + + const late = reducer(closing, { type: "imageLoaded" }); + + expect(late.lightbox).toBe(LightboxStatus.READY_TO_CLOSE); + expect(reducer(late, { type: "openAnimationPrepared" })).toBe(late); + }); +}); diff --git a/app/components/LightboxState.ts b/app/components/LightboxState.ts new file mode 100644 index 0000000000..9f6e4dbbbd --- /dev/null +++ b/app/components/LightboxState.ts @@ -0,0 +1,97 @@ +export enum LightboxStatus { + READY_TO_OPEN, + OPENING, + OPENED, + READY_TO_CLOSE, + CLOSING, + CLOSED, +} + +export enum ImageStatus { + LOADING, + ERROR, + LOADED, + MIN_ZOOM, + MAX_ZOOM, + ZOOMED, +} + +export type Status = { + lightbox: LightboxStatus | null; + image: ImageStatus | null; +}; + +export type Action = + /** The lightbox mounted and is ready to begin its opening animation. */ + | { type: "mounted" } + /** A new image started loading, either the first one or after navigating. */ + | { type: "imageLoading" } + | { type: "imageLoaded" } + | { type: "imageErrored" } + /** The zoom level changed, either by the user or by resetting the transform. */ + | { type: "zoomChanged"; zoom: ImageStatus } + /** The opening fade and zoom keyframes have been measured and applied. */ + | { type: "openAnimationPrepared" } + | { type: "openAnimationEnded" } + /** The user asked to close, e.g. via Escape, the close button, or a swipe. */ + | { type: "closeRequested" } + /** The closing fade and zoom keyframes have been measured and applied. */ + | { type: "closeAnimationPrepared" } + | { type: "closeAnimationEnded" }; + +export const initialStatus: Status = { lightbox: null, image: null }; + +/** + * Settles the image at minimum zoom once the lightbox has finished opening and + * the image has loaded, whichever of the two happens last. + * + * @param status The status to settle. + * @returns The settled status. + */ +function settle(status: Status): Status { + return status.lightbox === LightboxStatus.OPENED && + status.image === ImageStatus.LOADED + ? { ...status, image: ImageStatus.MIN_ZOOM } + : status; +} + +/** + * Drives the lightbox open and close choreography. Transitions are guarded so + * that events arriving out of order — a slow image load completing after the + * user has already started closing, for example — cannot move it backwards. + * + * @param status The current status. + * @param action The action to apply. + * @returns The next status. + */ +export function reducer(status: Status, action: Action): Status { + switch (action.type) { + case "mounted": + return { ...status, lightbox: LightboxStatus.READY_TO_OPEN }; + case "imageLoading": + return settle({ ...status, image: ImageStatus.LOADING }); + case "imageLoaded": + return settle({ ...status, image: ImageStatus.LOADED }); + case "imageErrored": + return settle({ ...status, image: ImageStatus.ERROR }); + case "zoomChanged": + return { ...status, image: action.zoom }; + case "openAnimationPrepared": + return status.lightbox === LightboxStatus.READY_TO_OPEN + ? { ...status, lightbox: LightboxStatus.OPENING } + : status; + case "openAnimationEnded": + return settle({ ...status, lightbox: LightboxStatus.OPENED }); + case "closeRequested": + return status.lightbox === LightboxStatus.OPENING || + status.lightbox === LightboxStatus.OPENED + ? { ...status, lightbox: LightboxStatus.READY_TO_CLOSE } + : status; + case "closeAnimationPrepared": + return status.lightbox === LightboxStatus.READY_TO_CLOSE + ? { ...status, lightbox: LightboxStatus.CLOSING } + : status; + case "closeAnimationEnded": + return { lightbox: LightboxStatus.CLOSED, image: null }; + } +} diff --git a/app/components/PaginatedList.tsx b/app/components/PaginatedList.tsx index 084e7665c7..3bb0347f39 100644 --- a/app/components/PaginatedList.tsx +++ b/app/components/PaginatedList.tsx @@ -217,6 +217,9 @@ const PaginatedList = ({ if (fetch) { void fetchResults(); } + // `fetchResults` changes identity as pagination advances, depending on it + // here would re-run the initial fetch for every page. + // eslint-disable-next-line react-hooks/exhaustive-deps }, [fetch]); // Handle updates to fetch or options diff --git a/app/components/Sharing/Document/AccessControlList.tsx b/app/components/Sharing/Document/AccessControlList.tsx index 84bafd41a8..890f57ce1e 100644 --- a/app/components/Sharing/Document/AccessControlList.tsx +++ b/app/components/Sharing/Document/AccessControlList.tsx @@ -247,15 +247,17 @@ const CollectionSquircle = ({ collection }: { collection: Collection }) => { function useUsersInCollection(collection?: Collection) { const { users, memberships } = useStores(); - const { request } = useRequest(() => - memberships.fetchPage({ limit: 1, id: collection!.id }) + const fetchMemberships = React.useCallback( + () => memberships.fetchPage({ limit: 1, id: collection!.id }), + [memberships, collection] ); + const { request } = useRequest(fetchMemberships); React.useEffect(() => { if (collection && !collection.permission) { void request(); } - }, [collection]); + }, [collection, request]); return collection ? collection.permission diff --git a/app/components/Sharing/components/GroupMembersPopover.tsx b/app/components/Sharing/components/GroupMembersPopover.tsx index 2abbb3e39e..645a7a00cb 100644 --- a/app/components/Sharing/components/GroupMembersPopover.tsx +++ b/app/components/Sharing/components/GroupMembersPopover.tsx @@ -28,8 +28,12 @@ export const GroupMembersPopover = observer(({ group, children }: Props) => { const { groupUsers } = useStores(); const [open, setOpen] = React.useState(false); + // `orderedData` is an observable dependency, it is what recomputes the list + // when group membership changes in the store. + // eslint-disable-next-line react-hooks/exhaustive-deps const members = React.useMemo( () => groupUsers.inGroup(group.id), + // eslint-disable-next-line react-hooks/exhaustive-deps [groupUsers.orderedData, group.id] ); diff --git a/app/components/Sharing/components/Suggestions.tsx b/app/components/Sharing/components/Suggestions.tsx index e42f1eaae7..5d348e3e04 100644 --- a/app/components/Sharing/components/Suggestions.tsx +++ b/app/components/Sharing/components/Suggestions.tsx @@ -116,6 +116,9 @@ export const Suggestions = observer( : []), ...filtered, ]; + // The store collections listed below are observable dependencies, they + // are what recompute the suggestions when the underlying data changes. + // eslint-disable-next-line react-hooks/exhaustive-deps }, [ getSuggestionForEmail, users, diff --git a/app/components/Sidebar/Shared.tsx b/app/components/Sidebar/Shared.tsx index 6f665a1d5f..f80ec05b0f 100644 --- a/app/components/Sidebar/Shared.tsx +++ b/app/components/Sidebar/Shared.tsx @@ -57,6 +57,8 @@ function SharedSidebar({ share }: Props) { useEffect(() => { ui.tocVisible = share.showTOC; + // Only seed the initial visibility, the user can toggle it afterwards. + // eslint-disable-next-line react-hooks/exhaustive-deps }, []); if (!rootNode?.children.length) { diff --git a/app/components/Sidebar/components/ArchiveLink.tsx b/app/components/Sidebar/components/ArchiveLink.tsx index 9a5b7dd940..7d95a32b31 100644 --- a/app/components/Sidebar/components/ArchiveLink.tsx +++ b/app/components/Sidebar/components/ArchiveLink.tsx @@ -43,7 +43,7 @@ function ArchiveLink() { if (disclosure && isUndefined(expanded)) { setExpanded(false); } - }, [disclosure]); + }, [disclosure, expanded]); useEffect(() => { if (expanded) { diff --git a/app/components/Sidebar/components/SharedWithMe.tsx b/app/components/Sidebar/components/SharedWithMe.tsx index ef600a2841..d8d2724a8a 100644 --- a/app/components/Sidebar/components/SharedWithMe.tsx +++ b/app/components/Sidebar/components/SharedWithMe.tsx @@ -92,6 +92,9 @@ function SharedWithMe() { }) ); } + // `history` is read imperatively, the sidebar context should only be + // recalculated when the active document or memberships change. + // eslint-disable-next-line react-hooks/exhaustive-deps }, [ ui.activeDocumentId, locationSidebarContext, diff --git a/app/editor/components/FloatingToolbar.tsx b/app/editor/components/FloatingToolbar.tsx index 40f6b265ec..07ec83f30b 100644 --- a/app/editor/components/FloatingToolbar.tsx +++ b/app/editor/components/FloatingToolbar.tsx @@ -49,7 +49,9 @@ function usePosition({ const [menuWidth, setMenuWidth] = React.useState(0); const menuHeight = 36; - // Measure the menu width after DOM updates to ensure accurate positioning + // Measure the menu width after DOM updates to ensure accurate positioning. + // Runs after every render by design, the width comparison prevents a loop. + // eslint-disable-next-line react-hooks/exhaustive-deps React.useLayoutEffect(() => { if (menuRef.current) { const width = menuRef.current.offsetWidth; diff --git a/app/editor/components/SelectionToolbar.tsx b/app/editor/components/SelectionToolbar.tsx index ed2637bb0f..0b2f9ec518 100644 --- a/app/editor/components/SelectionToolbar.tsx +++ b/app/editor/components/SelectionToolbar.tsx @@ -113,6 +113,9 @@ export function SelectionToolbar(props: Props) { } else if (selection.empty) { setActiveToolbar(null); } + // `activeToolbar` is read to decide whether the link toolbar should stay + // open, re-running when it changes would fight the user's own selection. + // eslint-disable-next-line react-hooks/exhaustive-deps }, [ readOnly, isActive, @@ -127,7 +130,7 @@ export function SelectionToolbar(props: Props) { if (autoFocusLinkInput && activeToolbar !== Toolbar.Link) { setAutoFocusLinkInput(false); } - }, [activeToolbar]); + }, [activeToolbar, autoFocusLinkInput]); const prevActiveToolbar = React.useRef(activeToolbar); React.useLayoutEffect(() => { @@ -183,7 +186,7 @@ export function SelectionToolbar(props: Props) { return () => { window.removeEventListener("mouseup", handleClickOutside); }; - }, [isActive, readOnly, view]); + }, [isActive, readOnly, view, extensions]); useEventListener( "keydown", diff --git a/app/editor/components/StickyBlockToolbar.tsx b/app/editor/components/StickyBlockToolbar.tsx index b230b789bf..d40dddf1f4 100644 --- a/app/editor/components/StickyBlockToolbar.tsx +++ b/app/editor/components/StickyBlockToolbar.tsx @@ -94,7 +94,9 @@ const StickyBlockToolbar = React.forwardRef(function StickyBlockToolbar_( const element = getBlockElement(view); - // Measure the block relative to the portal's offset parent. + // Measure the block relative to the portal's offset parent. Runs after every + // render by design, the rect comparison prevents a loop. + // eslint-disable-next-line react-hooks/exhaustive-deps React.useLayoutEffect(() => { const track = trackRef.current; if (!element || !track) { diff --git a/app/editor/components/ToolbarMenu.tsx b/app/editor/components/ToolbarMenu.tsx index c103ae0743..34d5d155b3 100644 --- a/app/editor/components/ToolbarMenu.tsx +++ b/app/editor/components/ToolbarMenu.tsx @@ -55,6 +55,9 @@ function ToolbarDropdown(props: ToolbarDropdownProps) { return resolvedItemChildren ? mapMenuItems(resolvedItemChildren, commands, view, state) : []; + // Menu items are resolved against the editor state at the moment the menu + // opens, recomputing on every transaction would rebuild the open menu. + // eslint-disable-next-line react-hooks/exhaustive-deps }, [isOpen, commands]); const handleCloseAutoFocus = useCallback((ev: Event) => { diff --git a/app/hooks/useComputed.ts b/app/hooks/useComputed.ts index 187fb8742b..c68a6dfe02 100644 --- a/app/hooks/useComputed.ts +++ b/app/hooks/useComputed.ts @@ -11,6 +11,8 @@ export function useComputed( callback: () => T, inputs: DependencyList = [] ): T { + // The dependency list is supplied by the caller so it cannot be verified here. + // eslint-disable-next-line react-hooks/exhaustive-deps const value = useMemo(() => computed(callback), inputs); return value.get(); } diff --git a/app/hooks/usePaginatedRequest.ts b/app/hooks/usePaginatedRequest.ts index 0bea24fc43..1d27f4faea 100644 --- a/app/hooks/usePaginatedRequest.ts +++ b/app/hooks/usePaginatedRequest.ts @@ -81,6 +81,9 @@ export default function usePaginatedRequest( }) ); } + // `params` is intentionally omitted, callers pass an object literal which + // changes identity on every render. + // eslint-disable-next-line react-hooks/exhaustive-deps }, [offset, fetchLimit, requestFn]); const next = useCallback(() => { @@ -100,6 +103,9 @@ export default function usePaginatedRequest( limit: fetchLimit, }) ); + // `params` and `fetchLimit` are intentionally omitted, resetting pagination + // should only be driven by the request function changing. + // eslint-disable-next-line react-hooks/exhaustive-deps }, [requestFn]); return { data, next, loading, error, page, offset, end }; diff --git a/app/hooks/usePinnedDocuments.ts b/app/hooks/usePinnedDocuments.ts index 90a8ccf2dd..62295a065c 100644 --- a/app/hooks/usePinnedDocuments.ts +++ b/app/hooks/usePinnedDocuments.ts @@ -1,4 +1,4 @@ -import { useEffect } from "react"; +import { useCallback, useEffect } from "react"; import usePersistedState from "~/hooks/usePersistedState"; import useStores from "./useStores"; @@ -13,13 +13,15 @@ export function usePinnedDocuments(urlId: UrlId, collectionId?: string) { 0 ); - function getPins() { - return urlId === "home" - ? pins.home - : collectionId - ? pins.inCollection(collectionId) - : []; - } + const getPins = useCallback( + () => + urlId === "home" + ? pins.home + : collectionId + ? pins.inCollection(collectionId) + : [], + [urlId, collectionId, pins] + ); useEffect(() => { void pins @@ -27,7 +29,7 @@ export function usePinnedDocuments(urlId: UrlId, collectionId?: string) { .then(() => { setPinsCacheCount(getPins().length); }); - }, [collectionId, pins]); + }, [urlId, collectionId, pins, getPins, setPinsCacheCount]); return { count: pinsCacheCount, diff --git a/app/hooks/useRequest.ts b/app/hooks/useRequest.ts index 9061c2b812..fa9fbd768a 100644 --- a/app/hooks/useRequest.ts +++ b/app/hooks/useRequest.ts @@ -59,6 +59,9 @@ export default function useRequest( if (makeRequestOnMount) { void request(); } + // Only ever request on mount, later changes to the request function are + // surfaced through the returned `request` for the caller to invoke. + // eslint-disable-next-line react-hooks/exhaustive-deps }, []); return { data, loading, loaded, error, request }; diff --git a/app/scenes/Collection/index.tsx b/app/scenes/Collection/index.tsx index f16e4b362b..c6d73e0349 100644 --- a/app/scenes/Collection/index.tsx +++ b/app/scenes/Collection/index.tsx @@ -115,6 +115,9 @@ const CollectionScene = observer(function CollectionScene_() { } void fetchData(); + // Fetched once on mount, the slug in `id` also changes when the collection + // is renamed which must not trigger a refetch. + // eslint-disable-next-line react-hooks/exhaustive-deps }, []); useEffect(() => { diff --git a/app/scenes/Developer/Changesets.tsx b/app/scenes/Developer/Changesets.tsx index b3ef21b7c3..11d36e3e84 100644 --- a/app/scenes/Developer/Changesets.tsx +++ b/app/scenes/Developer/Changesets.tsx @@ -14,7 +14,7 @@ import useStores from "~/hooks/useStores"; import usePersistedState from "~/hooks/usePersistedState"; import Scrollable from "~/components/Scrollable"; import Switch from "~/components/Switch"; -import { action } from "mobx"; +import { runInAction } from "mobx"; import { ChangesetHelper } from "@shared/editor/lib/ChangesetHelper"; /** @@ -39,8 +39,8 @@ function Changesets() { * This ensures that MobX reactions in RevisionViewer and the model computed properties * (like `changeset`) are triggered correctly. */ - React.useEffect( - action(() => { + React.useEffect(() => { + runInAction(() => { stores.revisions.data.clear(); stores.documents.data.clear(); @@ -82,9 +82,8 @@ function Changesets() { createdAt: "2024-01-02T12:00:00.000Z", data: selectedExample.after, }); - }), - [selectedExample, id] - ); + }); + }, [selectedExample, id]); const mockDocument = stores.documents.get("mock-document-id"); const mockDiffRevision = stores.revisions.get("mock-diff-revision-" + id); diff --git a/app/scenes/Document/components/KeyboardShortcutsButton.tsx b/app/scenes/Document/components/KeyboardShortcutsButton.tsx index bf83634f17..4467b45b7b 100644 --- a/app/scenes/Document/components/KeyboardShortcutsButton.tsx +++ b/app/scenes/Document/components/KeyboardShortcutsButton.tsx @@ -29,6 +29,9 @@ function KeyboardShortcutsButton() { if (shortcutsQuery !== null) { handleOpenKeyboardShortcuts(shortcutsQuery); } + // Only the query param should open the guide, re-running when the handler + // identity changes would reopen an already dismissed dialog. + // eslint-disable-next-line react-hooks/exhaustive-deps }, [shortcutsQuery]); return ( diff --git a/app/scenes/Document/components/MultiplayerEditor.tsx b/app/scenes/Document/components/MultiplayerEditor.tsx index 2f630f55b0..60672a65e5 100644 --- a/app/scenes/Document/components/MultiplayerEditor.tsx +++ b/app/scenes/Document/components/MultiplayerEditor.tsx @@ -231,6 +231,9 @@ function MultiplayerEditor( setRemoteProvider(undefined); ui.setMultiplayerStatus(undefined, undefined); }; + // `token` is intentionally omitted, it is only read when establishing the + // connection and a refreshed token must not tear down the provider. + // eslint-disable-next-line react-hooks/exhaustive-deps }, [ history, t, diff --git a/app/scenes/Search/Search.tsx b/app/scenes/Search/Search.tsx index 8a62b3199d..07a9e8f218 100644 --- a/app/scenes/Search/Search.tsx +++ b/app/scenes/Search/Search.tsx @@ -68,9 +68,16 @@ function Search() { const userId = params.get("userId") ?? ""; const documentId = params.get("documentId") ?? undefined; const dateFilter = (params.get("dateFilter") as TDateFilter) ?? ""; - const statusFilter = params.getAll("statusFilter")?.length - ? (params.getAll("statusFilter") as TStatusFilter[]) - : [TStatusFilter.Published, TStatusFilter.Draft]; + // Keyed on the serialized value so the array keeps a stable identity between + // renders and can be used directly as a dependency. + const statusFilterKey = params.getAll("statusFilter").join(","); + const statusFilter = React.useMemo( + () => + statusFilterKey + ? (statusFilterKey.split(",") as TStatusFilter[]) + : [TStatusFilter.Published, TStatusFilter.Draft], + [statusFilterKey] + ); const titleFilter = isTruthyQueryValue(params.get("titleFilter")); const sort = (params.get("sort") as TSortFilter) ?? ""; const direction = (params.get("direction") as TDirectionFilter) ?? ""; @@ -103,7 +110,7 @@ function Search() { }), [ query, - JSON.stringify(statusFilter), + statusFilter, collectionId, userId, dateFilter, diff --git a/app/scenes/Settings/Application.tsx b/app/scenes/Settings/Application.tsx index 55db759f7c..f98829e4ca 100644 --- a/app/scenes/Settings/Application.tsx +++ b/app/scenes/Settings/Application.tsx @@ -42,13 +42,17 @@ const LoadingState = observer(function LoadingState() { const { id } = useParams<{ id: string }>(); const { oauthClients } = useStores(); const oauthClient = oauthClients.get(id); - const { request } = useRequest(() => oauthClients.fetch(id)); + const fetchOAuthClient = useCallback( + () => oauthClients.fetch(id), + [oauthClients, id] + ); + const { request } = useRequest(fetchOAuthClient); useEffect(() => { if (!oauthClient) { void request(); } - }, [oauthClient]); + }, [oauthClient, request]); if (!oauthClient) { return ; diff --git a/shared/editor/components/Embed.tsx b/shared/editor/components/Embed.tsx index 81a46665b2..7d42a2eea6 100644 --- a/shared/editor/components/Embed.tsx +++ b/shared/editor/components/Embed.tsx @@ -22,25 +22,14 @@ const Embed = (props: Props) => { const naturalHeight = 400; const isResizable = !!onChangeSize && !embedsDisabled; - const { width, height, setSize, handlePointerDown, dragging } = useDragResize( - { - width: node.attrs.width ?? naturalWidth, - height: node.attrs.height ?? naturalHeight, - naturalWidth, - naturalHeight, - onChangeSize, - ref, - } - ); - - React.useEffect(() => { - if (node.attrs.height && node.attrs.height !== height) { - setSize({ - width: node.attrs.width, - height: node.attrs.height, - }); - } - }, [node.attrs.height]); + const { width, height, handlePointerDown, dragging } = useDragResize({ + width: node.attrs.width ?? naturalWidth, + height: node.attrs.height ?? naturalHeight, + naturalWidth, + naturalHeight, + onChangeSize, + ref, + }); const style: React.CSSProperties = { width: width || "100%", diff --git a/shared/editor/components/Image.tsx b/shared/editor/components/Image.tsx index 3885ed2a38..1de2cbbb31 100644 --- a/shared/editor/components/Image.tsx +++ b/shared/editor/components/Image.tsx @@ -94,21 +94,15 @@ const Image = (props: Props) => { const [naturalHeight, setNaturalHeight] = React.useState(node.attrs.height); const lastTapTimeRef = React.useRef(0); const ref = React.useRef(null); - const { - width, - height, - setSize, - handlePointerDown, - handleDoubleClick, - dragging, - } = useDragResize({ - width: node.attrs.width ?? naturalWidth, - height: node.attrs.height ?? naturalHeight, - naturalWidth, - naturalHeight, - onChangeSize, - ref, - }); + const { width, height, handlePointerDown, handleDoubleClick, dragging } = + useDragResize({ + width: node.attrs.width ?? naturalWidth, + height: node.attrs.height ?? naturalHeight, + naturalWidth, + naturalHeight, + onChangeSize, + ref, + }); const isFullWidth = layoutClass === "full-width"; const isInlineIcon = isInlineImageIcon({ layoutClass, width, error }); @@ -117,15 +111,6 @@ const Image = (props: Props) => { const className = imageClassName({ layoutClass, width, error }); - React.useEffect(() => { - if (node.attrs.width && node.attrs.width !== width) { - setSize({ - width: node.attrs.width, - height: node.attrs.height, - }); - } - }, [node.attrs.width]); - const sanitizedSrc = sanitizeImageSrc(src); const linkMarkType = props.view.state.schema.marks.link; const imgLink = @@ -261,16 +246,11 @@ const Image = (props: Props) => { // seen and is not sized to 0px const nw = (ev.target as HTMLImageElement).naturalWidth || 300; const nh = (ev.target as HTMLImageElement).naturalHeight; + // When no width is set on the node the natural size is what the + // image is displayed at, so it feeds straight into useDragResize. setNaturalWidth(nw); setNaturalHeight(nh); setLoaded(true); - - if (!node.attrs.width) { - setSize((state) => ({ - ...state, - width: nw, - })); - } }} onClick={handleImageClick} onTouchStart={handleImageTouchStart} diff --git a/shared/editor/components/PDF.tsx b/shared/editor/components/PDF.tsx index 4ca414d975..34656fd4e6 100644 --- a/shared/editor/components/PDF.tsx +++ b/shared/editor/components/PDF.tsx @@ -34,7 +34,7 @@ export default function PdfViewer(props: Props) { const embedRef = useRef(null); const debounceTimerRef = useRef(null); - const { width, setSize, handlePointerDown, dragging } = useDragResize({ + const { width, handlePointerDown, dragging } = useDragResize({ width: node.attrs.width, height: node.attrs.height, naturalWidth, @@ -43,16 +43,6 @@ export default function PdfViewer(props: Props) { ref, }); - useEffect(() => { - if (node.attrs.width && node.attrs.width !== width) { - setSize({ - width: node.attrs.width, - height: node.attrs.height, - }); - } - // eslint-disable-next-line react-hooks/exhaustive-deps - }, [node.attrs.width]); - // force embed to reload, so the content fits the new size. useEffect(() => { // firefox handles resizing on its own diff --git a/shared/editor/components/Video.tsx b/shared/editor/components/Video.tsx index 11ab7ec3b6..95a331692c 100644 --- a/shared/editor/components/Video.tsx +++ b/shared/editor/components/Video.tsx @@ -18,30 +18,15 @@ export default function Video(props: Props) { const ref = React.useRef(null); const isResizable = !!onChangeSize; - const { - width, - height, - setSize, - handlePointerDown, - handleDoubleClick, - dragging, - } = useDragResize({ - width: node.attrs.width ?? naturalWidth, - height: node.attrs.height ?? naturalHeight, - naturalWidth, - naturalHeight, - onChangeSize, - ref, - }); - - React.useEffect(() => { - if (node.attrs.width && node.attrs.width !== width) { - setSize({ - width: node.attrs.width, - height: node.attrs.height, - }); - } - }, [node.attrs.width]); + const { width, height, handlePointerDown, handleDoubleClick, dragging } = + useDragResize({ + width: node.attrs.width ?? naturalWidth, + height: node.attrs.height ?? naturalHeight, + naturalWidth, + naturalHeight, + onChangeSize, + ref, + }); const style: React.CSSProperties = { width: width || "auto", diff --git a/shared/editor/components/hooks/useDragResize.ts b/shared/editor/components/hooks/useDragResize.ts index 76dd7ad77c..f2f83136a3 100644 --- a/shared/editor/components/hooks/useDragResize.ts +++ b/shared/editor/components/hooks/useDragResize.ts @@ -46,8 +46,6 @@ type ReturnValue = { ) => (event: React.PointerEvent) => void; /** Event handler for double-click event on the resize handle. */ handleDoubleClick: () => void; - /** Handler to set the new size of the element from outside. */ - setSize: React.Dispatch>; /** Whether the element is currently being resized. */ dragging: DragDirection | undefined; /** The current width of the element. */ @@ -59,9 +57,9 @@ type ReturnValue = { type Params = { /** Callback triggered when the image is resized */ onChangeSize?: undefined | ((size: SizeState) => void); - /** The initial width of the element. */ + /** The committed width of the element, this is the source of truth. */ width: number; - /** The initial height of the element. */ + /** The committed height of the element, this is the source of truth. */ height: number; /** The natural width of the element. */ naturalWidth: number; @@ -88,16 +86,39 @@ export default function useDragResize(props: Params): ReturnValue { isCentered = true, } = props; - const [size, setSize] = React.useState({ + // The committed size passed in through props is the source of truth. `draft` + // holds the size being previewed while resizing and is released once the + // committed size arrives back through props, so the element never flashes at + // its previous size between the drag ending and the change round-tripping. + const [draft, setDraft] = React.useState(null); + const [committed, setCommitted] = React.useState({ width: props.width, height: props.height, }); const [maxWidth, setMaxWidth] = React.useState(Infinity); const [offset, setOffset] = React.useState({ x: 0, y: 0 }); - const [sizeAtDragStart, setSizeAtDragStart] = React.useState(size); + const [sizeAtDragStart, setSizeAtDragStart] = React.useState({ + width: props.width, + height: props.height, + }); const [dragging, setDragging] = React.useState(); const isResizable = !!onChangeSize; + // Release the draft whenever the committed size changes, whether that's this + // element's own resize landing or an external change such as undo, a reset, + // or a collaborator resizing the same node. Skipped while dragging so a + // remote change cannot yank the element out from under the pointer. + if ( + !dragging && + (!Object.is(committed.width, props.width) || + !Object.is(committed.height, props.height)) + ) { + setCommitted({ width: props.width, height: props.height }); + setDraft(null); + } + + const size = draft ?? { width: props.width, height: props.height }; + // Mirror the latest size into a ref so handlePointerUp can read it without // re-binding listeners on every pointermove that updates size. const sizeRef = React.useRef(size); @@ -175,7 +196,7 @@ export default function useDragResize(props: Params): ReturnValue { : undefined : sizeAtDragStart.height; - setSize({ + setDraft({ width: nextWidth, height: nextHeight, }); @@ -197,21 +218,23 @@ export default function useDragResize(props: Params): ReturnValue { const heightOnGrid = Math.round(newHeight / gridHeight) * gridHeight; const nextHeight = Math.max(heightOnGrid, minHeight ?? 50); - setSize((state) => { - const nextState = { - ...state, - height: nextHeight, - }; - window.dispatchEvent( - new CustomEvent("media-drag-resize", { - detail: { - ...nextState, - isDragging: true, - }, - }) - ); - return nextState; - }); + // Vertical-only drags never adjust the width, so it is carried over + // from the current size rather than recomputed. + const nextState = { + width: sizeRef.current.width, + height: nextHeight, + }; + + setDraft(nextState); + + window.dispatchEvent( + new CustomEvent("media-drag-resize", { + detail: { + ...nextState, + isDragging: true, + }, + }) + ); } }, [ @@ -224,6 +247,7 @@ export default function useDragResize(props: Params): ReturnValue { naturalHeight, minHeight, constrainWidth, + isCentered, ] ); @@ -254,7 +278,7 @@ export default function useDragResize(props: Params): ReturnValue { event.preventDefault(); event.stopPropagation(); - setSize(sizeAtDragStart); + setDraft(sizeAtDragStart); setDragging(undefined); window.dispatchEvent( @@ -280,7 +304,7 @@ export default function useDragResize(props: Params): ReturnValue { width: naturalWidth, height: naturalHeight, }; - setSize(newSize); + setDraft(newSize); onChangeSize?.(newSize); }; @@ -346,7 +370,6 @@ export default function useDragResize(props: Params): ReturnValue { handlePointerDown, handleDoubleClick, dragging, - setSize, width: size.width, height: size.height, };