Skip to content

feat: overhaul of overlay UI #5441

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 11 commits into
base: master
Choose a base branch
from
Open

Conversation

negimox
Copy link
Contributor

@negimox negimox commented Mar 22, 2025

  • This is a bugfix
  • This is a feature
  • This is a code refactor
  • This is a test update
  • This is a docs update
  • This is a metadata update

For Bugs and Features; did you add new tests?

No ( Already exists )

Motivation / Use-Case

Modified and improved the overlay UI according to #3689

Breaking Changes

Additional Info

webpack_dev_feat_overlay-2025-03-23_03.09.36.mp4

@negimox
Copy link
Contributor Author

negimox commented Mar 22, 2025

Hello, @alexander-akait @snitin315 can you check if this change suits issue #3689 ?

Copy link

codecov bot commented Mar 22, 2025

Codecov Report

Attention: Patch coverage is 77.19298% with 39 lines in your changes missing coverage. Please review.

Project coverage is 82.87%. Comparing base (6afffac) to head (b1e9134).

Files with missing lines Patch % Lines
client-src/overlay.js 77.19% 32 Missing and 7 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #5441      +/-   ##
==========================================
- Coverage   83.40%   82.87%   -0.54%     
==========================================
  Files          13       13              
  Lines        2031     2149     +118     
  Branches      749      769      +20     
==========================================
+ Hits         1694     1781      +87     
- Misses        303      327      +24     
- Partials       34       41       +7     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Copy link
Member

@alexander-akait alexander-akait left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's add a test cases.

You remove:

containerElement.innerHTML = overlayTrustedTypesPolicy
        ? overlayTrustedTypesPolicy.createHTML("")
        : "";

But in some part of code you use something.innerHTML= "html", you need to use the same logic as you remove, otherwise trusted police will be broken

Also does it work for warnings too?

@alexander-akait
Copy link
Member

Anyway code looks good, let's fix the problems above

@negimox
Copy link
Contributor Author

negimox commented Mar 28, 2025

Hello, @alexander-akait. I have modified the code according to the above suggestions. can you verify if it is sufficient?

@negimox negimox requested a review from alexander-akait April 23, 2025 18:12
Copy link
Member

@alexander-akait alexander-akait left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's add the same test but with TrustedTypesPolicy to ensure we don't break something and we can merge, thank you for your work

@negimox negimox requested a review from alexander-akait April 26, 2025 21:19
@negimox
Copy link
Contributor Author

negimox commented Apr 26, 2025

Hello, @alexander-akait I have added another test cases with same functionality for TrustedTypesPolicy could you please check and verify if its suitable implementation for it?
Thank you.

P.S.
The lint check is failing for security audit with log:

npm warn config production Use `--omit=dev` instead.
# npm audit report

http-proxy-middleware  <2.0.[9](https://github.com/webpack/webpack-dev-server/actions/runs/14685205670/job/41212922379?pr=5441#step:8:10)
Severity: moderate
http-proxy-middleware allows fixRequestBody to proceed even if bodyParser has failed - https://github.com/advisories/GHSA-9gqv-wp59-fq42
fix available via `npm audit fix`
node_modules/http-proxy-middleware

1 moderate severity vulnerability

To address all issues, run:
  npm audit fix

Iam unsure on what steps, if any I should take for it.

@alexander-akait
Copy link
Member

@negimox Just make rebase, already fixed in master/main

@negimox
Copy link
Contributor Author

negimox commented May 4, 2025

Hello @alexander-akait, I’ve rebased this branch onto the latest changes. Let me know if there’s anything else you’d like me to address here.

Have a great weekend!

@alexander-akait
Copy link
Member

@negimox And also can you update snapshots, because in our test we still have old HTML output, thank you

Copy link
Member

@alexander-akait alexander-akait left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's fix a lint problem and does it work with warnings too?

@alexander-akait
Copy link
Member

Sorry for delay

@negimox
Copy link
Contributor Author

negimox commented Jun 21, 2025

Hey @alexander-akait 😅 — I'm also quite late to this, my apologies!

@negimox And also can you update snapshots, because in our test we still have old HTML output, thank you

Got it — I’ll update the snapshots accordingly.

Let's fix a lint problem and does it work with warnings too?

Yes, it should work with warnings as well, but I’ll double-check and verify that to be sure.

@alexander-akait
Copy link
Member

@negimox Feel free update and we can merge

@negimox
Copy link
Contributor Author

negimox commented Jun 29, 2025

Hello @alexander-akait, hope you are doing well.
I've resolved all lint errors related to code formatting and style, but it's currently failing under lint for the reason:

Validate PR commits with commitlint
BREAKING CHANGE: cross origin requests are not allowed unless allowed by `Access-Control-Allow-Origin` header
✖   subject may not be empty [subject-empty]
✖   type may not be empty [type-empty]

✖   found 2 problems, 0 warnings
ⓘ   Get help: https://github.com/conventional-changelog/commitlint/#what-is-commitlint

⧗   input: Merge commit from fork

BREAKING CHANGE: requests with an IP addresses Origin header are not allowed to connect to WebSocket server unless configured by `allowedHosts`
✖   subject may not be empty [subject-empty]
✖   type may not be empty [type-empty]

✖   found 2 problems, 0 warnings
ⓘ   Get help: https://github.com/conventional-changelog/commitlint/#what-is-commitlint

⧗   input: feat: overhaul of overlay UI
✔   found 0 problems, 0 warnings
Error: Process completed with exit code 1.

Could you point me as to how can I resolve this error?


Additionally, I've made improvements to the UI to better align with the reference images and ensure better responsiveness on smaller screen sizes.
Notably,

  • Small changes to the styling to better match the reference issue.
  • Smaller dimensions support

P.S. I tested and it is working with warning as well.

Video.06-29-2025.23.18.22.mp4

Let me know if there are still any changes that need to be made.

@negimox negimox requested a review from alexander-akait June 29, 2025 17:51
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.

2 participants