The One-Line "Cleanup" That Quietly Broke CLS for Everyone

Someone deleted two arrow functions to make a JavaScript bundle a few bytes smaller. Each deletion was harmless on its own. Together they made web-vitals — the library Google ships for measuring Core Web Vitals — quietly send analytics a fake reading every time a visitor pressed the Back button.

It shipped in v5.3.0 and nobody noticed through the entire v6 line. Here's how I found it, how I nearly convinced myself I was wrong, and how the person who wrote the original bug turned up in my issue thread and made my fix better.

Two words first: CLS and bfcache

CLS is Cumulative Layout Shift — a score for how much a page jumps around while it loads. You're halfway through a sentence, an image finishes loading above it, the text shoves down, you lose your place. CLS adds those jumps up. Lower is better, Google uses it as a ranking signal, so real businesses genuinely watch this number.

bfcache is the back/forward cache. When you press Back, the browser usually doesn't rebuild the page — it kept the whole thing frozen in memory and just un-pauses it, like resuming a paused game instead of restarting the level. It's why Back feels instant.

Where they meet: when a page comes back from bfcache, web-vitals treats it as a fresh page view. It resets CLS to zero and starts measuring again. That reset is correct and deliberate. The bug lived in what happened two animation frames after it.

The bug in one sentence

Every time someone pressed Back onto your site, web-vitals told your analytics "this page view's CLS is 0" — before the page had had any chance to shift at all.

One page load, one Back navigation, and this is what actually landed in the beacon log:

{"value":0.022345870537622232,"delta":0.0223458705,"navigationType":"navigate"}
{"value":0,"delta":0,"entries":0,"navigationType":"back-forward-cache"}

First line real. Second line fiction.

Zero is the best possible CLS score, so those fake samples quietly drag your average down toward "good". A site with a genuine layout-shift problem and a lot of Back-button traffic would look healthier than it actually is. And this wasn't some exotic config — it was the exact snippet in the README, onCLS(sendToAnalytics), in its default mode. Teams that de-duplicate by metric ID were fine. Teams that count or average every callback, which is the obvious way to do it, were not.

Getting into the repo was harder than finding the bug

I'd never opened this codebase before. It's small — a few thousand lines — but dense. Almost every strange-looking line is strange for a reason, with an issue number attached to it. So your first reaction to odd code has to be "I don't understand this yet," not "this is wrong." Learning to tell those two apart is basically the whole job.

Before I got anywhere near the interesting logic, the setup fought me. The repo's .nvmrc pins Node 18, and on Node 18 npm run build dies with ReferenceError: crypto is not defined somewhere deep inside rollup. Lint and formatting pass happily though — just enough success to convince you your setup is fine. Node 22 builds it. npm ci fails where npm install works. The end-to-end tests need three terminals running at once, and if you forget one you get failures that look exactly like product bugs.

Then the reading. A big soft-navigation rewrite had recently landed, so a chunk of the code was new even to people who knew the library. And the repo is tightly maintained — around eleven open issues at the time. There's no pile of neglected low-hanging fruit here. If something obvious were broken, it'd already be fixed.

So I did the slow thing. Read every entry point end to end, then the shared helpers, and wrote down everything that looked even slightly off — not to file it, just to force myself to explain each one out loud. Twelve candidates. Most dissolved the moment I understood the surrounding code properly. Three survived. This was the cleanest of them.

Honestly, that notes file was the real output of the week, not the PR. The candidates that died taught me more about the library than the one that lived.

The thing that looked wrong

In src/onCLS.ts, the bfcache handler resets the metric and then schedules a report two animation frames later:

onBFCacheRestore(() => {
  initNewCLSMetric('back-forward-cache', /* ... */);

  doubleRAF(report);      // <-- this line
});

Nothing about that screams at you. What made me stop was comparing it to its siblings. The same "wait two frames, then report" pattern exists in onFCP and onLCP, and both of them write it differently:

doubleRAF(() => report(true));   // onFCP, onLCP
doubleRAF(report);              // onCLS  <-- the odd one out

Four call sites of the same pattern, one written differently. That proves nothing. It's just the cheapest possible hint about where to spend the next two hours.

Why passing a function by name is a trap

This is the heart of it, and it's worth understanding properly, because this trap exists in every JavaScript codebase alive.

report isn't a plain no-argument function. Its real signature is (forceReport?: boolean) => void. It takes one optional argument, meaning "report even if nothing changed?"

And doubleRAF is just requestAnimationFrame(() => requestAnimationFrame(cb)).

Here's the collision. The browser doesn't call animation-frame callbacks with nothing — it passes them a timestamp, something like 4213.7. That's the whole point of the API; it's how animations know how much time has passed. So doubleRAF(report) means the browser calls report(4213.7), and that timestamp lands in the forceReport slot. Any non-zero number is truthy. So forceReport isn't "undefined, so no" — it's "4213.7, which means yes, absolutely, force it."

The way I picture it: the function is a doorman told "only let people in if they show you a pass." The browser walks up and hands him a stopwatch. He doesn't look at what it is. It's a thing, things mean yes, in they go.

And what does it force out? The metric had just been reset, so its value is 0, nothing has ever been reported for it, and the delta is 0. The guard inside the reporter lets anything through if no previous value exists — which is there for a good reason, since a page that gets hidden with a real CLS of zero genuinely should report that zero. So out goes a CLS of 0.

The detail that turned suspicion into certainty was the line right underneath: setTimeout(report). Identical shape, completely safe — because setTimeout doesn't pass its callback any arguments. Same code, opposite outcome, purely because of what the scheduler hands you. At that point I knew I wasn't misreading somebody's clever design. Both lines couldn't be intentional.

The test suite said everything was fine

This is the part that nearly stopped me.

The suite has extensive bfcache coverage for CLS. It passed on the broken code. It passed with my fix. Same tests, same result, both times. If I'd trusted it, I'd have decided I was imagining things and moved on.

Two things were hiding the bug, neither of them anyone's mistake. The helper that reads beacons only returns the ones sharing the most recent metric ID — a sensible default that stops one test's leftovers polluting the next, and which quietly sorted my fake beacon out of the picture. And the fake beacon was slow: two animation frames, then sendBeacon, then a write to the test server's log. By the time it arrived, the assertions had run and the cleanup had already deleted the log file. The evidence was being destroyed milliseconds before it showed up.

What broke it open was abandoning the helpers entirely — drive a real forward-and-back in a real Chrome, sit there doing nothing for a few seconds, then just cat the raw beacon log. Two lines where there should have been one. That output is what I pasted into the issue, and it's why the report was taken seriously in hours instead of argued about for weeks.

A passing test suite is evidence about the tests, not about the code. Any helper that filters or de-duplicates can erase the exact anomaly you're hunting.

git blame, and the surprise

Before filing, I wanted to know when this started — "was it ever correct?" is the fastest way to tell a bug from a decision. It traced back to a PR titled "Reduce bundle size by refactoring." One commit, two hunks:

// src/lib/doubleRAF.ts
-  requestAnimationFrame(() => requestAnimationFrame(() => cb()));
+  requestAnimationFrame(() => requestAnimationFrame(cb));

// src/onCLS.ts
-          doubleRAF(() => report());
+          doubleRAF(report);

Look at that carefully, because it's the most useful thing in this whole story. Either change alone is completely harmless. Keep the wrapper inside doubleRAF and it doesn't matter what the call site passes. Keep it at the call site and it doesn't matter what doubleRAF does. Only removing both, in the same commit, opens a clean path from the browser's timestamp straight into forceReport. Reviewed hunk by hunk, it really did look like a no-op cleanup.

TypeScript didn't complain either, and never would have. A function taking fewer parameters is assignable to a type expecting more — that's deliberate, it's what lets you write arr.map(x => x * 2) without declaring the index and array params. It's also the rule behind the classic ['1','2','3'].map(parseInt) returning [1, NaN, NaN]. Types won't save you from an argument you didn't know was coming.

The plot twist

I wrote the issue the way the repo's better reports are written: summary, impact, steps to reproduce, expected versus actual, environment. I front-loaded everything a maintainer would otherwise have to rediscover — the exact line, the chain from timestamp to truthy flag to fired callback, the comparison against the sibling files, the archaeology, the raw two-beacon output, and an explanation of why the existing tests don't catch it, so nobody would run the suite and conclude I was wrong.

I also mentioned an optional extra: instead of patching the onCLS call site, you could harden doubleRAF itself so no future caller could ever be bitten the same way. I filed that as an afterthought. It turned out to be the important part.

A maintainer confirmed the analysis and asked for tests covering both configurations. Then the author of the original refactor showed up. They owned it straight away — it was meant as a pure bundle-size change, and they hadn't realised removing both wrappers together would change behaviour — and then argued my optional hardening should be the primary fix, precisely because either wrapper alone would have prevented this. Fixing the helper restores the guarantee for every call site, not just the one that happened to get caught. They even offered to open the PR themselves, since it came from their change, while noting it was mine if I wanted it. I was already halfway through, so I kept the PR and took their fix over my own.

That exchange is why I write detailed issues now. If I'd filed three lines — "onCLS reports 0 after bfcache, here's a one-liner" — I'd have gotten a merged one-liner on the call site, and the trap would still be sitting in doubleRAF waiting for the next person.

The fix

export const doubleRAF = (cb: () => unknown) => {
-  requestAnimationFrame(() => requestAnimationFrame(cb));
+  requestAnimationFrame(() => requestAnimationFrame(() => cb()));
};

One arrow function. onCLS.ts wasn't touched at all.

In plain English: the wrapper is a filter. Hand the browser cb directly and the browser hands cb a timestamp. Hand it () => cb() and the browser hands that the timestamp — and it ignores the thing completely and calls cb() with nothing. The doorman takes the stopwatch, pockets it, and asks for the pass.

The bit I had to be careful not to break: when someone explicitly opts into reportAllChanges: true, that zero-value report after a restore is correct and expected. It still fires, because the check is forceReport || reportAllChanges and the second half carries it now. The fix removes an accidental force, not a deliberate one.

So there are two new tests, one for each side of that line. The regression test does a forward-and-back with default options, waits well past the double frame, and asserts there's still exactly one beacon — it fails on unfixed code through both retries, and passes with the fix. The second asserts the zero-value report does still arrive when reportAllChanges is on, and passes with and without the fix. That's the point of it: a test that only fails before your change proves the bug was real, and a test that must keep passing proves you didn't fix it by breaking something else.

I also slipped in a drive-by. An existing test was reading the beacon file without waiting for the beacon to arrive, while its sibling test had exactly that wait — a pre-existing race from years earlier that happened to bite me locally. I added the missing await, called it out in the PR description as unrelated, and offered to split it into its own PR. Say the quiet part out loud rather than smuggling unrelated changes past a reviewer.

Then the wait. Filed 15 August, PR the same day, merged on the 24th, released in v6.2.0 as one line in a changelog: "Prevent spurious CLS report of 0 after bfcache restore." (One admin note for future me: Google projects need their CLA signed before anything merges. Sign it early, not while a maintainer is waiting.)

What I want to remember

1. Two individually-safe refactors can add up to a bug. Reviewing a diff hunk by hunk isn't the same as reviewing it by behaviour.

2. foo(cb) and foo(() => cb()) aren't stylistic alternatives. The first is only safe if you know exactly what arguments the caller supplies. rAF gives you a timestamp, setTimeout gives you nothing, .map() gives you three things, event listeners give you an event. That wrapper is a contract, not noise for a bundler to shave off.

3. A green suite is evidence about the tests. Read the raw output by hand at least once before you conclude you were wrong.

4. Write the issue as if someone else will fix it. Mine got a better fix than the one I proposed, because the detail let someone with more context see I was patching the wrong layer.

5. And be generous when someone else's commit caused your bug. The author owned it immediately and improved my fix. Nobody needed telling off, and the code came out better than either of us would've managed alone.

The whole change is one arrow function. Working out which arrow function is the job.