Skip to content

Fix article jumping under the reader after scrolling stops - #76

Open
jonocodes wants to merge 2 commits into
mainfrom
investigate-mobile-scroll-bounce
Open

jonocodes wants to merge 2 commits into
mainfrom
investigate-mobile-scroll-bounce

Conversation

@jonocodes

@jonocodes jonocodes commented Oct 2, 2026 •

Copy link
Copy Markdown
Owner

Fixes the mobile report: "when I scroll an article and stop, after a second or two it bounces up or down a bit."

What was happening

The reading-progress save fires 1s after scrolling stops. It re-emits the article through the Dexie liveQuery, re-rendering ArticleScreen — and that re-render rebuilt the entire article DOM.

React 19 diffs dangerouslySetInnerHTML by object identity, not by the __html string (React ≤18 compared the string), and then assigns innerHTML unconditionally. ArticleComponent passed a fresh { __html: sanitize(...) } literal every render, so every re-render tore down and recreated every node.

Text was recreated at identical heights, so the rebuild was invisible. Images weren't: each <img> came back new and unloaded, and web.css styles article images as max-width: 100%; height: auto with no width/height attributes — so they collapsed to zero height until they decoded again. The page shrank, then sprang back. That's the bounce, and it's why it only happened on articles with images.

The same rebuild also discarded in-flight image downloads and restarted them from zero, which is why it was worst on a bad connection: images that are still fetched over the network had their downloads cancelled on every reading pause and could never finish. Partly self-inflicted.

Evidence

MutationObserver on the article container, across one progress save:

childList mutations image requests (4 images, 3 pauses)
before [{removed: 243, added: 243}] 8
after [] 4

Measured as where the reader's line of text sits on screen: −307px → 0px.

Changes

  • src/components/ArticleComponent.tsx — the fix. Memoize the sanitized {__html} (now via Show inline video embeds (YouTube, Vimeo) in the reader #80's sanitizeArticleHtml) so its identity is stable and React leaves the DOM alone. Also stops re-sanitizing the whole article on every render, and stops video embeds reloading their player on every save.
  • lib/src/ingestion.ts — record each image's real dimensions as width/height attributes, so the browser reserves the correct box before decode. resizeImage now reports the canvas's own (truncated) dimensions so the aspect ratio is exact. Stored image bytes are unchanged — only the reported numbers and the saved HTML's attributes.
  • src/components/ArticleScreen.tsx — remove 5 leftover console.log calls that ran on every save in production.

Tests

tests/e2e/scroll-stability.spec.ts (9) and lib/__tests__/ingestion-image-dimensions.test.ts (3). Each new test was confirmed to fail without its fix rather than pass vacuously.

Coverage includes the root cause (no DOM rebuild on a progress save), the symptom (text must not move when a late image loads), no image re-downloads across reading pauses, plus fresh/resumed articles, live sync, mobile fling under 6× CPU throttle, viewport resize, and a second device writing conflicting progress mid-read.

That last one also pins something worth stating: the reading position is not re-applied mid-read. The restore is guarded to run once. A sync write moving progress from 45 to 12 under an active reader must leave the viewport untouched.

Full run: 310 unit tests, 46 e2e (articles/TTS/edit/ingestion/smoke), 9 scroll-stability. Typecheck clean; lint 0 errors.

Notes

🤖 Generated with Claude Code

The reading-progress save fires 1s after scrolling stops. It re-emits the
article through the Dexie liveQuery, which re-renders ArticleScreen — and
that re-render rebuilt the entire article DOM.

React 19 diffs dangerouslySetInnerHTML by object identity rather than by
the __html string (React <=18 compared the string), then assigns innerHTML
unconditionally. ArticleComponent passed a fresh `{ __html: sanitize(...) }`
literal every render, so every re-render tore down and recreated every node.
Text was recreated at identical heights so the rebuild was invisible, but
each <img> was new and unloaded — and web.css gives article images
`height: auto` with no width/height attributes, so they collapsed to zero
height until they decoded again. The page shrank, then sprang back.

The rebuild also discarded in-flight image downloads and restarted them.
Images whose download failed at ingest keep their original remote URL, so
on a slow connection they were cancelled and restarted on every reading
pause and could never finish. Measured at 8 requests for 4 images across
3 pauses; now 4. The remaining half of that — those images being remote at
all — is tracked in #75.

Also:
- Record each image's real dimensions as width/height attributes at ingest,
  so the browser reserves the correct box before decode instead of laying
  out a zero-height box and reflowing when it arrives. resizeImage now
  reports the canvas's own (truncated) dimensions so the aspect ratio is
  exact; the stored image bytes are unchanged.
- Remove leftover scroll-progress debug logging that ran in production.

Note the reading position is NOT re-applied mid-read: the restore is
guarded to run once, which the two-device test pins — a sync write moving
progress from 45 to 12 under an active reader must not move the viewport.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@netlify

netlify Bot commented Oct 2, 2026 •

Copy link
Copy Markdown

✅ Deploy Preview for savrlist ready!

Name Link
🔨 Latest commit 5fce191
🔍 Latest deploy log https://app.netlify.com/projects/savrlist/deploys/6ac1e14de88c90000841510c
😎 Deploy Preview https://deploy-preview-76--savrlist.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify project configuration.

Conflicts:
- ArticleComponent.tsx: #80 moved sanitising into sanitizeArticleHtml
  (video-embed iframe allow-list) but still passed a fresh `{ __html }`
  literal each render, so the DOM-rebuild bug remained on main. Keep #80's
  sanitiser and this branch's memoization: useMemo(() => ({ __html:
  sanitizeArticleHtml(html) }), [html]).
- CHANGELOG.md: keep both; this branch's entries move under #80's
  2026-10-03 heading since they land after it.

With embeds now rendered the rebuild also reloaded every player on each
save: on main's code 3 embeds were loaded 18 times over 3 reading pauses.
Added a scroll-stability test pinning one load per embed (fails on main's
version of the line, passes with the memoization).

Adjusted for #79, which swaps failed images for a local placeholder:
- The failed-download dimensions test now expects the placeholder and
  data-orig-src, and still asserts no width/height is claimed.
- The shared jest mock of ~/utils/article/tools gains a FetchError class.
  ingestion.ts now does `e instanceof FetchError` on a failed image, and
  without it the check throws; no existing test ran a failed download
  through ingestHtml, so the gap was latent.
- Comment and changelog wording: only articles saved before #79 still
  hold remote URLs for failed images.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@jonocodes

Copy link
Copy Markdown
Owner Author

Merged main to resolve conflicts with #79 and #80.

The bug was still on main, and #80 made it worse. #80 moved sanitizing into sanitizeArticleHtml and allowed video-embed <iframe>s, but it still passed a new { __html } object on every render. So every progress save also recreated every embed and reloaded its player. On main's version of that line, 3 embeds were loaded 18 times across 3 reading pauses. The resolution keeps #80's sanitizer and memoizes its result, which brings it to 3 loads. A new scroll-stability test, progress save does not reload video embeds, covers this. It fails on main's version of the line and passes with the fix.

Adjusted for #79 (failed images become a local placeholder):

  • The failed-download dimensions test now expects the placeholder and data-orig-src, and still checks that no width/height is written for an image whose size we never learned.
  • The shared Jest mock of ~/utils/article/tools now includes a FetchError class. ingestion.ts checks e instanceof FetchError after a failed image download, and without the class that check throws. No existing test ran a failed download through ingestHtml, so this hadn't shown up. It only affects tests; in the app FetchError is a real class.
  • Comments and changelog wording now say that only articles saved before Add a fetch governor: dedupe, negative-cache and back off repeated proxy fetches #79 still hold remote URLs for failed images.

Checked after the merge: 351 unit tests and 57 e2e tests (scroll-stability, #79's failed-image ingest, ingest, reader, edit, TTS, smoke) pass; typecheck is clean.

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.

1 participant