Skip to content

fix(#477): make hiding the dev overlay reversible - #493

Open
jarik2014 wants to merge 2 commits into
SmartDropLabs:mainfrom
jarik2014:fix/477-restore-dev-overlay
Open

jarik2014 wants to merge 2 commits into
SmartDropLabs:mainfrom
jarik2014:fix/477-restore-dev-overlay

Conversation

@jarik2014

Copy link
Copy Markdown

Closes #477

The problem, restated

hideDevOverlay writes display: none onto every nextjs-portal and nothing puts it back. Hiding the badge is deliberate — it animates inside a shadow root, so its pixels are not reproducible and it has to go before a screenshot (the comment above the helper explains the original investigation). Leaving the page in that state afterwards is not deliberate.

What changed

The overlay is now hidden by a rule the suite owns:

<style id="e2e-hide-dev-overlay">nextjs-portal { display: none !important }</style>

so restoring is deleting a node this suite created. No per-element style is destroyed and there is no previous value to remember — which is what made the old version one-way.

Two things fall out of the change beyond reversibility:

  • Portals that mount after the call are covered. The previous loop only caught whichever nextjs-portal elements existed at that instant; the dev runtime mounts the badge well after hydration. A stylesheet keeps applying to elements added later.
  • !important holds against the overlay's own inline styles, which a plain style.display = "none" can be fighting depending on order.

restoreDevOverlay is called from a new test.afterEach, which Playwright runs even when the test threw — so a failure before the final screenshot no longer leaves the page hidden.

One correction to the issue

Playwright gives each test its own browser context, so I could not reproduce the overlay actually leaking into a subsequent test's baseline. What is real is that a failing test left its own page with the overlay hidden for anything that ran later in it, and that the mutation was irreversible. Both are fixed; the change is cheap enough that it is worth having even if the cross-test worry was theoretical.

Verification

$ pnpm exec tsc --noEmit                        # clean for this file
$ pnpm exec eslint e2e/visual-regression.spec.ts # 0 problems

I could not run the spec itself to completion on main, and that is not this issue's doing: the dev server serves a build-error overlay because of two unrelated breakages on the tip of main (useEffect not imported in Navbar.tsx, and a duplicate stepRef in useLockFlow.ts), which makes every spec time out before rendering. Both are fixed in #492; once that is in (and src/app/loading.tsx:42's SkeletonBar type error, also pre-existing, is dealt with) I will run this spec and report the result here rather than claim it.

No test was added for the helper itself: asserting my own string in a unit test would prove nothing, and the value of this change is only visible in a real browser run, which is what the existing visual spec already does.

`hideDevOverlay` in `e2e/visual-regression.spec.ts` writes `display: none` onto every
`nextjs-portal`, and nothing ever puts it back — hence this issue. Hiding the badge
is deliberate; it animates inside a shadow root, so its pixels are not reproducible
and it has to go before a screenshot. Leaving the page in that state afterwards is
not deliberate.

It now injects a rule of its own instead:

  <style id="e2e-hide-dev-overlay">nextjs-portal { display: none !important }</style>

Restoring is then deleting a node this suite created, so no per-element style is
destroyed and there is no state to remember. Two things fall out of that. Portals
that mount *after* the call are covered — the dev runtime mounts the badge well
after hydration, and the previous per-element loop only caught whichever portals
existed at that instant. And `!important` holds against the overlay's own inline
styles, which the inline `style.display` assignment could be fighting.

`restoreDevOverlay` is called from a new `test.afterEach`, which Playwright runs even
when the test threw, so a failure before the last screenshot no longer leaves the
overlay hidden. Note that Playwright gives each test a fresh context, so the leak the
issue describes is not something I could reproduce across tests — what is real is
that a failing test left its own page hidden, and that the mutation was one-way.
Both are gone; the fix costs nothing even if the cross-test worry was theoretical.
@netlify

netlify Bot commented Sep 24, 2026 •

Copy link
Copy Markdown

❌ Deploy Preview for smart-drop failed.

Name Link
🔨 Latest commit 4a59920
🔍 Latest deploy log https://app.netlify.com/projects/smart-drop/deploys/6ab549e259800a0008d9abe0

@netlify

netlify Bot commented Sep 24, 2026 •

Copy link
Copy Markdown

✅ Deploy Preview for spiffy-melomakarona-eb1e8a ready!

Name Link
🔨 Latest commit 4a59920
🔍 Latest deploy log https://app.netlify.com/projects/spiffy-melomakarona-eb1e8a/deploys/6ab549e26a78d10008c45857
😎 Deploy Preview https://deploy-preview-493--spiffy-melomakarona-eb1e8a.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify project configuration.

@jarik2014

Copy link
Copy Markdown
Author

Ran it for real. On a branch carrying #492's two fixes plus this change:

$ pnpm exec playwright test --config e2e/playwright.config.ts e2e/visual-regression.spec.ts
  ✓  1 [chromium] unknown routes render the 404 page and recover back home (8.1s)
  ✓  2 [chromium] theme toggle persists across reloads on the 404 page (5.1s)
  ✓  3 [chromium] leaderboard renders deterministically and keeps sorting stable (4.2s)
  3 passed (36.4s)

Test 1 is the one that calls hideDevOverlay and compares against the committed baselines. The screenshots match, which says the injected rule hides the badge exactly as the inline display: none did — and the afterEach restore leaves nothing behind for the tests that follow.

I could not have got here without #492: before it, the dev server served a build-error overlay and every spec timed out. That was the reason this looked unverifiable on main rather than a problem with the change itself.

@jarik2014

Copy link
Copy Markdown
Author

Added one more commit: a lockfile sync that is not about this PR's own subject, but is the reason every Netlify check on this PR was red.

package.json has carried next-intl: ^4.14.6 while pnpm-lock.yaml never got it. Netlify installs with --frozen-lockfile, so the install fails before the build is reached:

ERR_PNPM_OUTDATED_LOCKFILE  Cannot install with "frozen-lockfile" because
pnpm-lock.yaml is not up to date with <ROOT>/package.json
    Failure reason: specifiers in the lockfile (...) don't match specs in package.json (...)
    the only difference is next-intl

That is why the six checks on this PR ("Header rules", "Pages changed", "Redirect rules" — the two deploy previews each report three) were failing: they report the preview failing to build, and the preview cannot build while the install is refused. It is repo-wide, not specific to this branch.

The commit adds next-intl and its dependency tree to the lockfile and nothing else — pnpm install --no-frozen-lockfile reports no specifier changes for existing packages, the 439 added lines are all entries the package needs (icu-minify, intl-messageformat, negotiator, po-parser, the SWC extractor plugin). Verified after the change:

$ pnpm install --frozen-lockfile
Already up to date          # the exact command that used to abort

$ pnpm build
…compiled successfully, full route table printed, middleware 34 kB

The dependency is real, not stale: src/i18n.ts and src/request.ts both import { getRequestConfig } from "next-intl/server".

This commit is also on the other three open PRs from the same base (#490, #491, #493) so their previews can go green independently of this one merging.

@jarik2014

Copy link
Copy Markdown
Author

Update on the checks, since they now tell a clearer story: #492 is fully green (both deploy previews, header/redirect rules), and this one is still red — but not for the reason it was.

This branch carries the lockfile sync, so the install no longer aborts. What is left is the defects that live on main itself — the missing useEffect import in Navbar.tsx, the duplicated stepRef, the SkeletonBar prop type, the locale guard cast. They are not introduced by this PR; they are why every deploy preview in this repository fails, including this one's. #492 removes all four, which is why #492 builds.

I am deliberately not copying those fixes here: they would duplicate #492, and when it merges this branch picks them up on rebase. If you would rather each PR stand alone and be green today, say so and I will cherry-pick them in.

This branch has not been deployed

No deployments
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.

[bug] visual-regression.spec.ts hides dev overlay but doesn't restore it

1 participant