Sidepanel open check failing in some browsers #8271

grahamlangford · 2024-04-17T15:44:24Z

What does this PR do?

Potentially fixes an issue where the side panel would hang in a loading state with the reserved panels showing

Discussion

I've never been able to reproduce the issue (on Mac or Windows via Parallel)
When the width check ever returns false, side panel hangs
In customer calls, we were able to determine that the sidebar was being reported as not visible

Important

I'm not sure about the performance implications of always pinging
- SIDEBAR_PING is a noop, but we're still increasing the number of messenger calls
- We always ping using the messenger when we think the side panel might be open, though, so it's probably ok
Alternatively, we could try reducing the MINIMUM_SIDEBAR_WIDTH, but as it is already 300, I don't think that would help. It's possible that something is going wrong with the inner and outer width checks

Checklist

Designate a primary reviewer @twschiller

…ome pages

grahamlangford · 2024-04-17T15:45:28Z

src/contentScript/sidebarController.tsx

-/**
- * Determines whether the sidebar is open.
- * @returns false when it's definitely closed or 'unknown' when it cannot be determined,
- * because the extra padding might be caused by the dev tools being open on the side
- * or due to another sidebar
- */
-// The type cannot be `undefined` due to strictNullChecks
-function isSidePanelOpenSync(): false | "unknown" {
-  if (!isMV3()) {
-    throw new Error("isSidePanelOpenSync is only available in MV3");
-  }
-
-  if (!globalThis.window) {
-    return "unknown";
-  }
-
-  return window.outerWidth - window.innerWidth > MINIMUM_SIDEBAR_WIDTH
-    ? "unknown"
-    : false;
-}


Relocated function closer to the call site since it's no longer used by isSidePanelOpenMv3

grahamlangford · 2024-04-17T15:45:58Z

src/contentScript/sidebarController.tsx

-  if (isSidePanelOpenSync() === false) {
-    return false;
-  }
-


The proposed fix. Removing means more messenger calls, but it prevents false negatives due to bad inner/outer width calculations

twschiller · 2024-04-17T16:09:49Z

src/contentScript/sidebarController.tsx

+    return "unknown";
+  }
+
+  return window.outerWidth - window.innerWidth > MINIMUM_SIDEBAR_WIDTH


I don't know much about details of outerWidth/innerWidth. Should we just be saying if there's any difference then it's unknown? What's the benefit to having the minimum difference?

cc @fregante

It does seem like on my machine + chrome, the outerWidth is always equal to the inner width when the sidebar is closed. However, I could imagine that could not be the case on different OS's / Edge. Ex. if you have a persistent scrollbar or not.

We could potentially change to window.outerWidth > window.innerWidth. I'm not 100% confident here since I've never been able to reproduce. I think this failing is far less likely to be problematic when called by sidePanelOnCloseSignal, which is the only remaining usage of isSidePanelOpenSync in this PR

twschiller · 2024-04-17T16:15:16Z

src/contentScript/sidebarController.tsx

@@ -513,6 +488,27 @@ export function getReservedPanelEntries(): {
  };
 }

+/**
+ * Determines whether the sidebar is open.


Provide some comment on when this should be used vs. other ways to determine?

src/contentScript/sidebarController.tsx

fungairino · 2024-04-17T16:25:35Z

Can we ping the w3c folks on this feature request so we don't have to make such a hack? 😅
w3c/webextensions#517

fungairino · 2024-04-17T16:29:12Z

Maybe relevant? https://codereview.stackexchange.com/questions/213760/window-innerwidth-workaround-for-when-it-returns-the-wrong-value

codecov · 2024-04-17T18:20:41Z

Codecov Report

Attention: Patch coverage is 0% with 7 lines in your changes are missing coverage. Please review.

Project coverage is 73.51%. Comparing base (9f3aced) to head (5f87131).
Report is 5 commits behind head on main.

Files	Patch %	Lines
src/contentScript/sidebarController.tsx	0.00%	7 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #8271      +/-   ##
==========================================
+ Coverage   73.49%   73.51%   +0.02%     
==========================================
  Files        1325     1325              
  Lines       40864    40851      -13     
  Branches     7566     7561       -5     
==========================================
  Hits        30032    30032              
+ Misses      10832    10819      -13

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

src/contentScript/sidebarController.tsx

github-actions · 2024-04-17T20:41:13Z

No loom links were found in the first post. Please add one there if you'd like to it to appear on Slack.

Do not edit this comment manually.

fregante · 2024-04-18T00:03:47Z

src/contentScript/sidebarController.tsx

+ * or due to another sidebar
+ */
+// The type cannot be `undefined` due to strictNullChecks
+function isSidePanelOpenSync(): false | "unknown" {


If this function is not reliable, then it should just be dropped. I don’t think there's a scenario in which we need it to be sync, right?

I was avoiding making changes to sidePanelOnCloseSignal, but it's should be safe. I'll drop it.

fregante · 2024-04-18T00:10:08Z

src/contentScript/sidebarController.tsx

-  }
+async function isSidePanelOpenMv3() {
+  return memoizeUntilSettled(
+    throttle(async () => {


These aren't hooks, they're being used incorrectly.

two isSidePanelOpenMv3() will still send two messages

same for throttle

You're probably looking for:

const isSidePanelOpenMv3 = throttle(memoizeUntilSettled(fn));

Now that I look at the file, the pingSidebar function below does the same thing. You're probably looking for:

async function isSidePanelOpenMv3() { try { await pingSidebar() return true } catch { return false } }

This was my initial inclination, but I assumed the original implementation used messenger directly for a reason.

fregante · 2024-04-18T12:22:49Z

src/contentScript/sidebarController.tsx

@@ -113,6 +74,16 @@ const pingSidebar = memoizeUntilSettled(
  }, 1000) as () => Promise<void>,
 );

+// This method is exclusive to the content script, don't export it


Outdated now that it doesn't use the DOM/window, it can be called from anywhere

potentially fixes the side panel never replacing reserved panels on s…

3a35b95

…ome pages

grahamlangford added bug Something isn't working customer Required for a customer projct labels Apr 17, 2024

grahamlangford self-assigned this Apr 17, 2024

grahamlangford requested review from twschiller and fungairino April 17, 2024 15:44

grahamlangford commented Apr 17, 2024

View reviewed changes

twschiller reviewed Apr 17, 2024

View reviewed changes

fungairino reviewed Apr 17, 2024

View reviewed changes

src/contentScript/sidebarController.tsx Outdated Show resolved Hide resolved

adds comment odetailing when to use isSidePanelOpenSync

63ccf7a

vercel bot deployed to Preview April 17, 2024 18:14 View deployment

throttle calls to check if the side panel is open

0488852

vercel bot deployed to Preview April 17, 2024 18:21 View deployment

fungairino reviewed Apr 17, 2024

View reviewed changes

src/contentScript/sidebarController.tsx Outdated Show resolved Hide resolved

fix timeouts

5fc5a01

vercel bot deployed to Preview April 17, 2024 18:30 View deployment

fungairino approved these changes Apr 17, 2024

View reviewed changes

fregante suggested changes Apr 18, 2024

View reviewed changes

PR requested changes

4c45dc2

grahamlangford requested a review from fregante April 18, 2024 12:21

vercel bot deployed to Preview April 18, 2024 12:22 View deployment

fregante approved these changes Apr 18, 2024

View reviewed changes

removed dead coded

2559ccc

vercel bot deployed to Preview April 18, 2024 12:25 View deployment

remove outdated comment

5f87131

grahamlangford enabled auto-merge (squash) April 18, 2024 14:46

vercel bot deployed to Preview April 18, 2024 14:48 View deployment

fungairino approved these changes Apr 18, 2024

View reviewed changes

grahamlangford merged commit f73b84b into main Apr 18, 2024
22 checks passed

grahamlangford deleted the fix-side-panel-never-loading branch April 18, 2024 14:56

grahamlangford mentioned this pull request Apr 19, 2024

fix for lag when opening sidebar using toggle sidebar brick #8295

Merged

1 task

grahamlangford added this to the 1.8.13 milestone Apr 22, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Sidepanel open check failing in some browsers #8271

Sidepanel open check failing in some browsers #8271

grahamlangford commented Apr 17, 2024

grahamlangford Apr 17, 2024

grahamlangford Apr 17, 2024

twschiller Apr 17, 2024 •

edited

Loading

fungairino Apr 17, 2024 •

edited

Loading

grahamlangford Apr 17, 2024

twschiller Apr 17, 2024

fungairino commented Apr 17, 2024

fungairino commented Apr 17, 2024

codecov bot commented Apr 17, 2024 •

edited

Loading

github-actions bot commented Apr 17, 2024

fregante Apr 18, 2024

grahamlangford Apr 18, 2024

fregante Apr 18, 2024

fregante Apr 18, 2024 •

edited

Loading

grahamlangford Apr 18, 2024

fregante Apr 18, 2024

Sidepanel open check failing in some browsers #8271

Sidepanel open check failing in some browsers #8271

Conversation

grahamlangford commented Apr 17, 2024

What does this PR do?

Discussion

Important

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

twschiller Apr 17, 2024 • edited Loading

Choose a reason for hiding this comment

fungairino Apr 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fungairino commented Apr 17, 2024

fungairino commented Apr 17, 2024

codecov bot commented Apr 17, 2024 • edited Loading

Codecov Report

github-actions bot commented Apr 17, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fregante Apr 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

twschiller Apr 17, 2024 •

edited

Loading

fungairino Apr 17, 2024 •

edited

Loading

codecov bot commented Apr 17, 2024 •

edited

Loading

fregante Apr 18, 2024 •

edited

Loading