Closes #6571: Reduce the processing on not-cached page when checking the LCP/ATF data

[Miraeld]

Description

Fixes #6571

Documentation

User documentation

It won't try to check if LCP data are already existing for non-cached URLs.

Technical documentation

We check if the page is a cached version, if it isn't we will bailout before sending an AJAX call to check if LCP datas are already existing for the URL.

Type of change

• Bug fix (non-breaking change which fixes an issue).
• Enhancement (non-breaking change which improves an existing functionality).

New dependencies

N/a

Risks

N/a

Checklists

Feature validation

• I validated all the Acceptance Criteria. If possible, provide sreenshots or videos.
• I triggered all changed lines of code at least once without new errors/warnings/notices.
• I implemented built-in tests to cover the new/changed code.

Documentation

• I prepared the user documentation for the feature/enhancement and shared it in the PR or the GitHub issue.
• The user documentation covers new/changed entry points (endpoints, WP hooks, configuration files, ...).
• I prepared the technical documentation if needed, and shared it in the PR or the GitHub issue.

Code style

• I wrote self-explanatory code about what it does.
• I wrote comments to explain why it does it.
• I named variables and functions explicitely.
• I protected entry points against unexpected inputs.
• I did not introduce unecessary complexity.
• I listed the introduced external dependencies explicitely on the PR.
• I validated the repo-specific guidelines from CONTRIBUTING.md.

[codacy-production]

Coverage summary from Codacy

See diff coverage on Codacy

Coverage variation	Diff coverage
✅ +0.00% (target: -0.10%)	✅ ∅ (target: 50.00%)

Coverage variation details

	Coverable lines	Covered lines	Coverage
Common ancestor commit (74186d3)	37122	14325	38.59%
Head commit (24e6a11)	37122 (+0)	14325 (+0)	38.59% (+0.00%)

Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: <coverage of head commit> - <coverage of common ancestor commit>

Diff coverage details

	Coverable lines	Covered lines	Diff coverage
Pull request (#6610)	0	0	∅ (not applicable)

Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: <covered lines added or modified>/<coverable lines added or modified> * 100%

See your quality gate settings    Change summary preferences

_{Codacy will stop sending the deprecated coverage status from June 5th, 2024. Learn more}

[jeawhanlee]

I think we should check for something more reliable maybe like the whole sentence as there are some cases where we do not generate timestamp in the footprint.

[Miraeld]

I used the same condition as on WPR Inspector (https://github.com/wp-media/wpr-inspector/blob/bd71796388de3c9cc0cff45488de23ce920df6d1/wpr-inspector/inspector.js#L25)

[wordpressfan]

We need to bear in mind that in some hosts our cache is disabled by default also the signature can be removed in cases like when having the whitelabel constant or so.

I need to understand more how do u detect the cached page in depth.

[Miraeld]

@wordpressfan for these cases, I guess we won't have the choice to do the DB request we want to avoid which is the one checking if LCP data does exists for the page while it isn't even cached yet.

We can't check through the DB as it would add another DB Request, which is the opposite of what we want, and there are no other way to determine if a page is cached or not. Except if there is something else I'm missing.

The way the current detection work is through the signature, where if the page is cached, we get the following text: Debug: cached@1714733510
Where 1714733510 is a timestamp.

[Miraeld]

@jeawhanlee & @wordpressfan Are you okay with the latest changes ?

[jeawhanlee]

For me I have issues with how reliable it can be detecting a cached page with our footprint signature, what if we tried checking the directory if the cache file exists and pass the result (boolean) to the script.

[jeawhanlee]

For me I have issues with how reliable it can be detecting a cached page with our footprint signature, what if we tried checking the directory if the cache file exists and pass the result (boolean) to the script.

The above won't work, when the page is cached, it would bail out and render the cache.

In my opinion, If we would use the signature, we should use parts that must always be present instead of just Debug: cached
Here is an example of a page without the timestamp:

[MathieuLamiot]

@Miraeld Looking at wp-rocket's code, there are versions of the code that don't have the timestamp in the signature I think. Typically in case of 3rd party hosting, white label and if time is empty.
In that case, it would make sense indeed to look for:

• '<!-- Optimized for great performance'
  OR
• '<!-- This website is like a Rocket, isn't it? Performance optimized by '

Then, of course, it is not perfect because sometimes, comments are removed from cached files. But better done than perfect.

WDYT of matching on those two rather than Debug + timestamp? In any cases, better done than perfect: Can we just change the matching to those 2 strings, move forward with this PR and eventually open a follow-up GH issue if there are edge cases you still foresee?

[Miraeld]

So based on our conversation yesterday, I guess we have to merge this PR and prepare another issue to enhance it,
@wordpressfan as you had the idea for the enhancement, can you create the new issue, and when it's done can you merge this PR ?

[MathieuLamiot]

@wordpressfan @jeawhanlee Since @Miraeld will be off end of this week, can you make sure to close this? Thanks 🙏

[wordpressfan]

Approved with very minor enhancement

[codacy-production]

Coverage summary from Codacy

See diff coverage on Codacy

Coverage variation	Diff coverage
✅ +0.01% (target: -0.10%)	✅ ∅ (target: 50.00%)

Coverage variation details

	Coverable lines	Covered lines	Coverage
Common ancestor commit (74186d3)	37122	14325	38.59%
Head commit (02032cd)	37174 (+52)	14349 (+24)	38.60% (+0.01%)

Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: <coverage of head commit> - <coverage of common ancestor commit>

Diff coverage details

	Coverable lines	Covered lines	Diff coverage
Pull request (#6610)	0	0	∅ (not applicable)

Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: <covered lines added or modified>/<coverable lines added or modified> * 100%

See your quality gate settings    Change summary preferences

_{Codacy will stop sending the deprecated coverage status from June 5th, 2024. Learn more}

[MathieuLamiot]

Expected behavior and observation from QA team discussed here: https://wp-media.slack.com/archives/CUT7FLHF1/p1716202320587489

[Mai-Saad]

The issue mentioned here #6610 (comment) is fixed, but on PR, nothing is added to the database with warmup (neither after a fresh install, nor after clear critical images)

steps:
1-fresh install of PR => check ATF table, nothing added although WPR debug contains some URLs to add to queue
2- clear critical images => check ATF, still nothing is added to database

Note: related test plan https://wpmediaqa.testrail.io/index.php?/runs/view/856&group_by=cases:section_id&group_order=asc

[wordpressfan]

All jobs from that site in SaaS are returning timeout error, I'm checking with Mostafa now what is going on there.

[wordpressfan]

This was because in SaaS we load the not cached page which doesn't include the signature and this means that the following line returns js error in the console:

wp-rocket/assets/js/lcp-beacon.js

Line 51 in a6b7622

const signature = document.documentElement.nextSibling.data ? document.documentElement.nextSibling.data : '';

because document.documentElement.nextSibling is null in this case, pushing a fix now.

[Mai-Saad]

Working as expected
testrail-report-605.pdf