#8287: Improve UX for long sidebar open times #8474
Conversation
| } catch (error) { | ||
| throw new Error("The sidebar did not respond in time", { cause: error }); | ||
| } | ||
| await pingSidebar(); |
There was a problem hiding this comment.
This try/catch was redundant
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #8474 +/- ##
==========================================
- Coverage 73.47% 73.42% -0.05%
==========================================
Files 1334 1352 +18
Lines 41259 41572 +313
Branches 7686 7801 +115
==========================================
+ Hits 30316 30526 +210
- Misses 10943 11046 +103 ☔ View full report in Codecov by Sentry. |
@grahamlangford you should be able to replicate it by setting a delay in the open sidebar method? (Or by setting a breakpoint in that method) |
|
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. |
| { | ||
| retries: 3, | ||
| onFailedAttempt({ attemptNumber }) { | ||
| if (attemptNumber === 1) { |
There was a problem hiding this comment.
@grahamlangford you need to save the handle so you can clear the message once the sidebar is open
- FIXME: clear the sidebar loading toast once the sidebar load
There was a problem hiding this comment.
I'd have the notification show indefinitely until either: 1) the sidebar loads, or 2) the load times out. Otherwise there can be a situation where the message disappears but the sidebar has not loaded yet
There was a problem hiding this comment.
Set it to 30 seconds, which should timeout well before. But I can alway set it to Number.POSITIVE_INFINITY for disabling auto-dismiss. That seems to be the way the library is intended to work
There was a problem hiding this comment.
Will dismiss on success or timeout
There was a problem hiding this comment.
Set it to 30 seconds, which should timeout well before. But I can alway set it to Number.POSITIVE_INFINITY for disabling auto-dismiss. That seems to be the way the library is intended to work
I'd recommend turning off auto-dismiss
Will dismiss on success or timeout
Where is the logic for hiding on timeout? Don't you need to also hide here when the error message will be shown?: https://github.com/pixiebrix/pixiebrix-extension/pull/8474/files/9fb06597a57a6ebca77e1adac133cf9096f78aff..22a882006f7d91d0c6767bbbd306c631dd557a23#diff-401596be27525397ebb55206c26230337d2badc594398f9033a2e6e1ff4a9ee8R119
IIRC ping sidebar throws on a failed ping, so the logic to hide it won't run on the final failed retry?
There was a problem hiding this comment.
Where is the logic for hiding on timeout?
I think you must be looking at an earlier commit, because I hide before I throw in the catch block:
pixiebrix-extension/src/contentScript/sidebarController.tsx
Lines 123 to 131 in 8d97195
There was a problem hiding this comment.
I'd recommend turning off auto-dismiss
I don't think that's possible other than setting the timeout to Number.POSITIVE_INFINITY. I wouldn't want to max out the timeout and make the toast not dismissible
You know, that's blindingly obvious in retrospect. Doing that now |
Playwright test results - MV2
Details
Flaky testschromeSetup › auth.setup.ts › authenticate Skipped testschrome › tests/extensionConsoleActivation.spec.ts › can activate a mod with built-in integration |
Playwright test results - MV3
Details
|
grahamlangford
deleted the
8287-make-the-sidebar-open-timeout-experience-better
branch
What does this PR do?
Reviewer Tips
Discussion
I've not been able to get the original timeout to throw on my machine, so this isn't tested beyond verifying it doesn't break typical open/close behaviorChecklist