Skip to content

[Image] Pause and play image gifs#3432

Open
catandthemachines wants to merge 44 commits intomainfrom
catjohnson/lems-3735
Open

[Image] Pause and play image gifs#3432
catandthemachines wants to merge 44 commits intomainfrom
catjohnson/lems-3735

Conversation

@catandthemachines
Copy link
Copy Markdown
Member

@catandthemachines catandthemachines commented Mar 31, 2026

Summary:

Functionality to pause and play image gifs. This includes:

  1. Loaded gifs start off paused, until the learner presses play.
  2. Pressing play button plays only one loop of the gif image, and then pauses.
  3. Pausing a gif will pause at the current frame, and resume from that frame until the loop is completed.

Before:

Screen.Recording.2026-03-18.at.4.40.45.PM.mov

After:

Screen.Recording.2026-03-31.at.3.21.47.PM.mov

Issue: LEMS-3735

Test plan:

Highly recommend checking these Storybook pages to confirm the implementation:

  • /?path=/story/widgets-image-widget-demo--gif-image
  • /?path=/story/editors-editorpage--with-all-flags

Here are some test gifs to use:

Also confirm that this experience is not visible when flags are off:

  • /?path=/story/editors-editorpage--demo

@catandthemachines catandthemachines marked this pull request as ready for review March 31, 2026 22:42
Copy link
Copy Markdown

@claude claude bot left a comment

Choose a reason for hiding this comment

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

Claude Code Review

This repository is configured for manual code reviews. Comment @claude review to trigger a review and subscribe this PR to future pushes, or @claude review once for a one-time review.

Tip: disable this comment in your organization's Code Review settings.

@catandthemachines
Copy link
Copy Markdown
Member Author

@claude review once

Comment on lines +239 to +265
React.useEffect(() => {
let mounted = true;

decodeGifFrames(src)
.then((frames) => {
if (!mounted) {
return;
}
framesRef.current = frames;

// Show the first frame on the canvas.
renderFrame(0);

if (latestPropsRef.current.isPlaying) {
play();
}
})
.catch(() => {});

return () => {
mounted = false;
pause();
framesRef.current = [];
};
// Only re-run when src changes.
// eslint-disable-next-line react-hooks/exhaustive-deps
}, [src]);
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 In gif-image.tsx, the .catch(() => {}) on the decodeGifFrames promise chain silently discards all fetch/parse errors, leaving the canvas blank with no user feedback. This is a regression from ImageLoader, which would at least show the browser's broken-image indicator on failure — here users see an empty canvas and a Play button that does nothing.

Extended reasoning...

What the bug is

In gif-image.tsx (the useEffect that loads GIF frames, roughly lines 239–265), the promise chain ends with .catch(() => {}). This empty handler suppresses all rejections from the decodeGifFrames pipeline without logging, setting error state, or providing any fallback rendering.

The specific code path that triggers it

decodeGifFrames already handles HTTP errors gracefully — a non-ok response returns [] rather than throwing. However, two failure modes will throw and be silently eaten:

  1. Network-level failures — DNS resolution failure, connection refused, CORS errors. The await fetch(src) call rejects, and .catch(() => {}) swallows it.
  2. Malformed GIF data — if parseGIF(buffer) or decompressFrames(gif, true) throws on corrupt or invalid GIF bytes, the rejection is discarded.

Why existing code does not prevent it

The empty catch is the only error handler. There is no try/catch in decodeGifFrames around the parse/decompress calls, no error state in the component, and no fallback <img> element. When isGifControlled is true, svg-image.tsx conditionally renders <GifImage> instead of <ImageLoader> (the {!isGifControlled && <ImageLoader .../>} branch is suppressed). So there is no native <img> element in the DOM at all for GIF images.

What the impact would be

On error: framesRef.current stays [], renderFrame() returns early (line ~170: if (frames.length === 0) return), the display canvas is blank, and play() returns early (line ~215: if (framesRef.current.length === 0) return). The Play button remains visible and functional-looking but does nothing when clicked. Users on unreliable connections, or a content author who accidentally sets a non-GIF URL in a GIF widget, see a completely blank canvas with no indication that an error occurred.

Prior to this PR, the same failure would produce a broken-image indicator via the native <img> tag rendered by ImageLoader — a worse-than-nothing fallback that at least signals failure. This PR removes that fallback entirely for GIF images.

Step-by-step proof

  1. User loads a page with a GIF image widget (feature flag enabled).
  2. GifImage mounts; svg-image.tsx skips <ImageLoader> because isGifControlled === true.
  3. decodeGifFrames(src) is called; fetch(src) rejects (e.g., network offline, CORS, typo in URL).
  4. .catch(() => {}) discards the rejection — no console error, no state update.
  5. framesRef.current remains []; the canvas is never sized or painted.
  6. User sees a blank canvas. Clicking Play does nothing (early return at frames.length === 0).
  7. No broken-image indicator, no error message — complete silent failure.

How to fix it

At minimum, the catch should log the error: .catch((err) => { console.error("GifImage: failed to decode", src, err); }). A better fix would set an hasError state and render a fallback <img src={src} alt={alt} /> so the browser's built-in broken-image indicator is shown, matching the pre-existing ImageLoader behavior.

Copy link
Copy Markdown
Member

@benchristel benchristel left a comment

Choose a reason for hiding this comment

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

This is looking good! Thanks for adding gifuct-js. I will come back and do a more thorough review later today — for now, I left some thoughts inline.

}
}

animationIdRef.current = requestAnimationFrame(animate);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

ooh, here's where my React knowledge gets a little murky. animate uses useCallback (good) but refers to itself here, which I'm not sure is okay. useCallback may return a different function instance on each render, but the animate passed to requestAnimationFrame will always be the callback from the first render.

Currently, I think we're in the clear because the dependency drawFrame should never change, so this useCallback will always return the same function. However, this code might break in the future if someone updates drawFrame to have a dependency on a prop or something.

The way I'd prefer to deal with this is to write a hook that manages the animation frame loop:

const {start, stop} = useAnimation((timestamp: number) => {
  // ...
});

It would be great if Wonder Blocks Timing had a hook for this, but it doesn't seem to.

};
// Only re-run when src changes.
// eslint-disable-next-line react-hooks/exhaustive-deps
}, [src]);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We don't list play, pause, and renderFrame as deps here — should we?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I removed those dependencies because we really should only run this useEffect (decode all the GIF frames) on changes to src, if I added those other dependencies it would run this useEffect more often then necessary, and decoding the frames is the more expensive aspect of this code.

I can add them back, but I recommend against it.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Seems like decoding should be done in a separate hook then; perhaps a usePromise hook similar to this one: https://www.npmjs.com/package/react-use-promise

const [frames, error] = usePromise(() => {
  return decodeGifFrames(src)
}, [src]);

React.useEffect(() => {
  if (frames != null) {
    play();
  }

  return pause;
}, [play, pause, frames])

(not suggesting we use that library; we could write our own)

};
// Only re-run when src changes.
// eslint-disable-next-line react-hooks/exhaustive-deps
}, [src]);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Seems like decoding should be done in a separate hook then; perhaps a usePromise hook similar to this one: https://www.npmjs.com/package/react-use-promise

const [frames, error] = usePromise(() => {
  return decodeGifFrames(src)
}, [src]);

React.useEffect(() => {
  if (frames != null) {
    play();
  }

  return pause;
}, [play, pause, frames])

(not suggesting we use that library; we could write our own)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants