e2e: fix MM-T4054 media preview failure on Mattermost 11.10.0-rc2

[yasserfaraazkhan]

Summary

Fixes the hard E2E failure of MM-T4054 (mattermost/media_preview) that started failing on master after CI moved from Mattermost 11.10.0-rc1 → 11.10.0-rc2.

This is not a regression from #3921 (destroyed-object guards). Evidence:

Run	Server	MM-T4054
Master green (5e585bb6)	11.10.0-rc1	passed
PR #3921 / master after merge / unrelated #3922	11.10.0-rc2	failed on all OSes

Root causes (both required):

1. Invalid fixture PNG — the previous base64 PNG was rejected by Mattermost’s decoder (png: invalid format: too much pixel data), so no preview/thumbnail was generated.
2. Webapp click gating in rc2 — MM-69174 / mattermost#37420 makes SizeAwareImage ignore clicks until state.loaded. On rc1, clicking still opened the modal even when the image failed to decode; on rc2 it does not.

Changes in e2e/specs/mattermost/media_preview.test.ts:

• Replace the fixture with a valid 128×128 PNG that the server accepts (has_preview_image: true).
• Wait for a visible, loaded non-placeholder .file-preview__button before opening.
• Prefer clicking that control; never click .image-loading__placeholder.
• Share loaded-image selectors / visibility helpers across wait and open paths (CodeRabbit feedback).

MM-T5891 (Windows-only on the master run) passed on #3921’s own E2E and is treated as an unrelated flake; not changed here.

Ticket Link

N/A — follow-up to master E2E health after #3921 / server rc2 bump.

Checklist

• Added or updated unit tests (required for all new features) — E2E helper/fixture update only
• Has UI changes
• read and understood our Contributing Guidelines
• completed Mattermost Contributor Agreement
• executed npm run lint:js for proper code formatting
• Run E2E tests by adding label E2E/Run

Device Information

This PR was tested on: Linux (Cloud VM), Mattermost 11.10.0-rc2 local server, Playwright --project=linux

Screenshots

N/A (E2E-only). Local verification:

npx playwright test specs/mattermost/media_preview.test.ts --project=linux --workers=1 --retries=0
✓ MM-T4054 Open/Close permanent link media preview (3.4s)
1 passed

Release Note

NONE

  

Change Impact: 🟢 Low

Regression Risk: Limited to a single E2E test file, adjusting only test-side fixture validity and renderer DOM synchronization/click logic for the media preview modal; no production/shared modules are modified.
QA Recommendation: Rely on the existing automated E2E coverage; manual QA can be skipped.
Generated by CodeRabbitAI

[coderabbitai]

📝 Walkthrough

Walkthrough

The Mattermost media preview E2E test replaces an invalid PNG fixture, waits for loaded image controls, expands preview selectors, adds fallback click targets, and extracts file IDs only from loaded non-placeholder images.

Changes

Media preview E2E flow

Layer / File(s)	Summary
Fixture and image readiness e2e/specs/mattermost/media_preview.test.ts	The test uses a valid 128x128 PNG, detects expanded image selectors, waits for visible loaded preview controls, and invokes that wait after posting the attachment.
Preview target selection e2e/specs/mattermost/media_preview.test.ts	Preview opening expands post detection, prefers loaded preview buttons, falls back to eligible visible thumbnails or images, and derives file IDs from non-placeholder image URLs.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Suggested reviewers: nickmisasi

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately summarizes the main change: an e2e fix for the media preview failure on Mattermost 11.10.0-rc2.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch cursor/master-e2e-test-failures-0213

---

_{Comment @coderabbitai help to get the list of available commands.}

[devinbinnie]

Thank you @yasserfaraazkhan :)

[coderabbitai]

🧹 Nitpick comments (1)

e2e/specs/mattermost/media_preview.test.ts (1)

41-81: 📐 Maintainability & Code Quality | 🔵 Trivial

Duplicate selector list/predicate logic across renderer scripts.

The legacy fallback selector string here (.post-image img:not(.image-loading__placeholder), .post--attachment img:not(.image-loading__placeholder), img[src*="/api/v4/files/"]:not(.image-loading__placeholder)) is duplicated verbatim in openImagePreview's clickTargets (Lines 216-218), and the visible/loaded predicate logic here mirrors the isVisible/isLoadedImg helpers defined later in openImagePreview. Since these are separately injected renderer scripts, consider extracting the shared selector strings/predicate bodies into module-level constants and interpolating them into both templates (similar to the ${DOM_UTILS} pattern in e2e/helpers/serverView.ts), so future selector changes stay in sync.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@e2e/specs/mattermost/media_preview.test.ts` around lines 41 - 81, Extract the
shared legacy image selector and visible/loaded image predicate logic used by
waitForLoadedImagePreviewControl and openImagePreview into module-level reusable
constants or script fragments, then interpolate them into both renderer
templates using the existing DOM_UTILS-style pattern. Preserve the current
selectors and behavior while ensuring future changes update both paths
consistently.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@e2e/specs/mattermost/media_preview.test.ts`:
- Around line 41-81: Extract the shared legacy image selector and visible/loaded
image predicate logic used by waitForLoadedImagePreviewControl and
openImagePreview into module-level reusable constants or script fragments, then
interpolate them into both renderer templates using the existing DOM_UTILS-style
pattern. Preserve the current selectors and behavior while ensuring future
changes update both paths consistently.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: c7e2096e-680a-446c-a777-063548f4a581

📥 Commits

Reviewing files that changed from the base of the PR and between 112c82a and b1f2da5.

📒 Files selected for processing (1)

• e2e/specs/mattermost/media_preview.test.ts

[coderabbitai]

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@e2e/specs/mattermost/media_preview.test.ts`:
- Around line 68-85: In e2e/specs/mattermost/media_preview.test.ts:68-85, update
waitForLoadedImagePreviewControl to locate the post containing the expected
e2e-preview.png attachment and poll only that post’s loaded preview control; in
e2e/specs/mattermost/media_preview.test.ts:192-231, reuse the same
post/attachment identity and restrict fallback clicks to that attachment’s
loaded image, preserving deterministic waits and preventing unrelated posts from
satisfying readiness or being clicked.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: 04345d4b-72f1-4259-8ea9-b6ea8a48562d

📥 Commits

Reviewing files that changed from the base of the PR and between b1f2da5 and 8257a5b.

📒 Files selected for processing (1)

• e2e/specs/mattermost/media_preview.test.ts

[coderabbitai]

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

e2e/specs/mattermost/media_preview.test.ts (1)

197-241: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Fallback click targets don't verify the wrapped image is actually loaded.

For HTMLImageElement targets the filter correctly requires isLoadedPreviewImage (loaded + visible). But for container targets (.post-image .image-loaded-container, .post-image .small-image__container, .post-image__image, .file-viewer-touch), the filter (Lines 228-229) only checks visibility plus the presence of a child img:not(.image-loading__placeholder) — it never confirms that child image is complete/naturalWidth > 0. A container could pass while its inner image is still mid-load, which risks reintroducing the exact rc2 click-gating flakiness this PR is fixing, if this last-resort branch is ever reached.

🐛 Proposed fix

             if (target instanceof HTMLImageElement) {
                 return isLoadedPreviewImage(target);
             }
-            return isPreviewControlVisible(target) &&
-                Boolean(target.querySelector?.('img:not(.image-loading__placeholder)'));
+            return isPreviewControlVisible(target) &&
+                isLoadedPreviewImage(target.querySelector?.('img:not(.image-loading__placeholder)'));
         });

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@e2e/specs/mattermost/media_preview.test.ts` around lines 197 - 241, Update
the container-target branch in openImagePreview so it selects the wrapped image
element and validates it with isLoadedPreviewImage, in addition to checking the
container’s visibility. Preserve the existing direct HTMLImageElement validation
and only allow fallback containers whose inner image is loaded and visible
before clicking.

Source: Coding guidelines

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Outside diff comments:
In `@e2e/specs/mattermost/media_preview.test.ts`:
- Around line 197-241: Update the container-target branch in openImagePreview so
it selects the wrapped image element and validates it with isLoadedPreviewImage,
in addition to checking the container’s visibility. Preserve the existing direct
HTMLImageElement validation and only allow fallback containers whose inner image
is loaded and visible before clicking.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: 025db70b-9461-45ed-bbab-9b82d7df5968

📥 Commits

Reviewing files that changed from the base of the PR and between 8257a5b and dd2810b.

📒 Files selected for processing (1)

• e2e/specs/mattermost/media_preview.test.ts