core(image-size-responsive): use real mimeType to exclude svg #16443

rkodey · 2025-04-12T21:40:31Z

Summary

bugfix

The current audit relies on guessing MimeType based on file extension. This can lead to inaccurate results for dynamically generated files since file extension is not guaranteed. This commit retrieves the real mimeType, while preserving the existing guess approach as a fallback.

This change will be transparent for existing SVG images, and only improves handling of images claiming to be SVG with alternate file extensions.

Unit tests have also been updated to accommodate the async audit, and simulated network records.

Thanks!!

Related Issues/PRs
Fixes #12018

The current audit relies on guessing MimeType based on file extension. This can lead to inaccurate results for dynamically generated files since file extension is not guaranteed. This commit retrieves the real mimeType, while preserving the existing guess approach.

connorjclark

Looks good, thanks!

BTW this audit will be removed in the near future, but the replacement insight audit image-delivery-insight is already good in this respect.

connorjclark · 2025-04-14T21:56:00Z

I couldn't push to your branch - can you please run this to fix the tests?

yarn update:sample-json && yarn mocha api-test-pptr -u

yarn update:sample-json yarn mocha api-test-pptr -u

rkodey · 2025-04-14T22:24:47Z

Sure - I guess I should commit the resulting changes to my branch... hopefully that's the right step.

Note the yarn update:sample-json ran clean.

However, the yarn mocha api-test-pptr -u produced an error message even though it's showing all 6 tests passed.

yarn run v1.22.22
$ node --loader=testdouble core/test/scripts/run-mocha-tests.js api-test-pptr -u
(node:3989452) ExperimentalWarning: `--experimental-loader` may be removed in the future; instead use `register()`:
--import 'data:text/javascript,import { register } from "node:module"; import { pathToFileURL } from "node:url"; register("testdouble", pathToFileURL("./"));'
(Use `node --trace-warnings ...` to show where the warning was created)
(node:3989452) UnsupportedWarning: `globalPreload` has been removed; use `initialize` instead.
(Use `node --trace-warnings ...` to show where the warning was created)
applied test filters: [
  "api-test-pptr"
]
running 1 test files


  Individual modes API
    snapshot
      ✔ should compute accessibility results on the page as-is (3636ms)
    startTimespan
      ✔ should compute ConsoleMessage results across a span of time (3907ms)
      ✔ should compute results from timespan after page load (3138ms)
      ✔ should know target type of network requests from frames created before timespan (33433ms)
    navigation
      ✔ should compute both snapshot & timespan results (6156ms)
LanternError: Invalid dependency graph created, cycle detected
    at PageDependencyGraph.createGraph (file:///home/rkodey/dev/lighthouse/node_modules/@paulirish/trace_engine/models/trace/lantern/graph/PageDependencyGraph.js?__quibble=0:481:19)
    at Module.createGraph (file:///home/rkodey/dev/lighthouse/node_modules/@paulirish/trace_engine/models/trace/LanternComputationData.js?__quibble=0:366:46)
    at #createLanternContext (file:///home/rkodey/dev/lighthouse/node_modules/@paulirish/trace_engine/models/trace/Processor.js?__quibble=0:255:46)
    at #computeInsights (file:///home/rkodey/dev/lighthouse/node_modules/@paulirish/trace_engine/models/trace/Processor.js?__quibble=0:458:53)
    at TraceProcessor.parse (file:///home/rkodey/dev/lighthouse/node_modules/@paulirish/trace_engine/models/trace/Processor.js?__quibble=0:122:38)
    at async TraceEngineResult.runTraceEngine (file:///home/rkodey/dev/lighthouse/core/computed/trace-engine-result.js?__quibble=0:38:5)
    at async TraceEngineResult.compute_ (file:///home/rkodey/dev/lighthouse/core/computed/trace-engine-result.js?__quibble=0:245:7)
      ✔ should compute results with callback requestor (6204ms)


  6 passing (57s)

Tests passed
Done in 59.11s.

rkodey · 2025-04-14T22:30:01Z

core/test/scenarios/__snapshots__/api-test-pptr.js.snap

  "image-alt",
  "image-aspect-ratio",
  "image-redundant-alt",
-  "image-size-responsive",


I'm a bit concerned with this diff... but I also don't fully grasp what this step is doing.

It seems image-size-responsive has been removed from the snapshot?
Is that expected?

Thanks for calling this out.

This snapshot comes from this test, which does a snapshot Lighthouse run.

What happened is related to devtoolsLogs being added as a required artifact. Snapshot runs have no devtoolsLogs, since that's a moment-in-time and that artifact does not support snapshot mode. So this change currently omits this audit from running. Since it's an optional enhancement, that is unfortunate.

The fix is to place devtoolsLogs in meta.__internalOptionalArtifacts instead, and then only use it / create NetworkRecords if present on artifacts.

connorjclark · 2025-04-16T00:30:57Z

dunno why I can't push to this branch given I think you allowed for it:

Anyhow, today I fixed some failing tests we had in main w/ the latest Chromium. If you update your branch w/ main and then run yarn lint --fix and yarn update:sample-json, CI should then pass.

rkodey · 2025-04-16T01:29:46Z

Cool - I merged, linted, and updated the sample json.

I didn't do anything with the snapshot comments above... is that needed as well?
( That would take quite a bit more time for me to pull together I think. )

Hopefully this little fix isn't becoming a pain - I figured it was an easy patch!! 😃

rkodey · 2025-04-16T13:42:58Z

Alright - I see the CI failed due to the missing audit test.
I'll follow your suggestion on using the __internalOptionalArtifacts -- I found examples in the code that I can pull from I think.

rkodey · 2025-04-16T14:05:16Z

Ok, that was an easy add to the code. __internalOptionalArtifacts + checks for missing devtoolsLogs
** no commit yet **

However, the devtoolsLogs are now consistently undefined when running the CLI.
I tried adding an artificial await, butno devtoolsLogs showed up, so I guess it's not a simple async issue.

I found this comment that seems to suggest exactly what I'm seeing

lighthouse/core/audits/dobetterweb/dom-size.js

Lines 85 to 94 in cd200f4

    
           const {GatherContext, devtoolsLogs, traces} = artifacts; 
        
           if (GatherContext.gatherMode !== 'navigation') { 
        
             return undefined; 
        
           } 
        
           // Since the artifacts are optional, it's still possible for them to be missing in navigation mode. 
        
           // Navigation mode does compute TBT so we should surface a numerical savings of 0. 
        
           if (!devtoolsLogs?.[Audit.DEFAULT_PASS] || !traces?.[Audit.DEFAULT_PASS]) { 
        
             return 0; 
        
           }

rkodey · 2025-04-16T14:51:52Z

I think I have a working fix!

Found this blog ref in the changelog related to DevtoolsLog vs devtoolsLogs
It appears DevtoolsLog is provided as an optional artifact.
And, the test snaphot has re-added image-size-responsive

Sweet. I'll convert, test, and re-commit later today.

devtoolsLogs wasn't loading as optional. But, found a reference in the changelogs indicating DevtoolsLog is preferred. Testing shows DevtoolsLog is available in navigation mode.

The image-size-responsive test was added back in.

rkodey · 2025-04-16T16:04:12Z

I think this commit will pass the CI

connorjclark · 2025-04-16T16:46:34Z

Thanks for the improvement, and for sticking through the additional changes!

rkodey requested a review from a team as a code owner April 12, 2025 21:40

rkodey requested review from connorjclark and removed request for a team April 12, 2025 21:40

vercel bot deployed to Preview April 12, 2025 21:41 View deployment

rkodey changed the title ~~core(image-size-responsive): Use real mimeType in addition to guessMimeType~~ core(image-size-responsive): use real mimeType in addition to guessMimeType Apr 12, 2025

connorjclark approved these changes Apr 14, 2025

View reviewed changes

connorjclark changed the title ~~core(image-size-responsive): use real mimeType in addition to guessMimeType~~ core(image-size-responsive): use real mimeType to exclude svg Apr 14, 2025

Update generated tests

25c96fb

yarn update:sample-json yarn mocha api-test-pptr -u

Merge branch 'main' into rk-fix-12018

2028537

vercel bot deployed to Preview April 14, 2025 22:25 View deployment

rkodey commented Apr 14, 2025

View reviewed changes

rkodey added 2 commits April 15, 2025 21:13

Merge branch 'main' into rk-fix-12018

b33ea20

Update sample json and linor lint change after merge

e9cfb49

vercel bot deployed to Preview April 16, 2025 01:27 View deployment

rkodey added 3 commits April 16, 2025 11:27

Update audit to use DevtoolsLog as optional artifact

64e6083

devtoolsLogs wasn't loading as optional. But, found a reference in the changelogs indicating DevtoolsLog is preferred. Testing shows DevtoolsLog is available in navigation mode.

Update to use artifacts.DevtoolsLog for consistency

250d9ab

Update test snap after refactor

9012ca9

The image-size-responsive test was added back in.

vercel bot deployed to Preview April 16, 2025 16:03 View deployment

connorjclark merged commit e7f1afc into GoogleChrome:main Apr 16, 2025
24 checks passed

core(image-size-responsive): use real mimeType to exclude svg #16443

core(image-size-responsive): use real mimeType to exclude svg #16443

Conversation

rkodey commented Apr 12, 2025

Uh oh!

connorjclark left a comment

Choose a reason for hiding this comment

Uh oh!

connorjclark commented Apr 14, 2025

Uh oh!

rkodey commented Apr 14, 2025

Uh oh!

rkodey Apr 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

connorjclark Apr 16, 2025

Choose a reason for hiding this comment

Uh oh!

connorjclark commented Apr 16, 2025

Uh oh!

rkodey commented Apr 16, 2025

Uh oh!

rkodey commented Apr 16, 2025

Uh oh!

rkodey commented Apr 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rkodey commented Apr 16, 2025

Uh oh!

rkodey commented Apr 16, 2025

Uh oh!

Uh oh!

connorjclark commented Apr 16, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

rkodey Apr 14, 2025 •

edited

Loading

rkodey commented Apr 16, 2025 •

edited

Loading