#2902: Throw BusinessError on runBrick if tab is closed or has no access
#2917
Conversation
| @@ -87,7 +103,7 @@ export async function requestRunInBroadcast( | |||
| } | |||
|
|
|||
| try { | |||
| const response = runBrick({ tabId: tab.id }, subRequest); | |||
There was a problem hiding this comment.
It doesn't matter as much here, but I suppose wrapping the error still makes sense.
src/background/executor.ts
Outdated
|
|
||
| // The caught error isn't "The tab was closed" because of: | ||
| // https://github.com/pixiebrix/pixiebrix-extension/issues/2902#issuecomment-1062658248 | ||
| throw new BusinessError("The tab was closed"); |
There was a problem hiding this comment.
As the comment mentions, I think we can safely assume that this was the reason at this point.
The demo video included the previous error message.
There was a problem hiding this comment.
I think we can safely assume that this was the reason at this point
The other case is that PixieBrix doesn't have access to the other page (i.e., it's on a different domain that PixieBrix doesn't have access to). What's the error in that case?
There was a problem hiding this comment.
Yeah in that case we get the same error because the "receiving end did not respond". We can change this error to "PixieBrix doesn't have access to the tab or it was closed"
Separately I suppose we can find a solution to detect this lack of permissions, probably in the background before calling runBrick:
- if target exists and we don't have access to it, throw error
- else runBrick(target)
There was a problem hiding this comment.
We can change this error to "PixieBrix doesn't have access to the tab or it was closed"
I'm good doing this for now in the PR
Separately I suppose we can find a solution to detect this lack of permissions, probably in the background before calling runBrick
👍
There was a problem hiding this comment.
I ended up implementing a better logic to tell the two errors apart:
- call
canAccessTab(it will runtrue;on the tab) - call runBrick + attach an "on tab closed" handler
- if the handler resolves first, the tab was closed
You can see that the "tab closed" error appears immediately, even if the messenger keeps retrying (we can stop the retries too, after sindresorhus/p-retry#53)
Screen.Recording.1.mov
And here you can see the specific error when PB doesn't have access:
Screen.Recording.2.mov
fregante
changed the title
runBrick if tab is closed or has no access
| @@ -138,3 +138,13 @@ export async function setReduxStorage<T extends JsonValue = JsonValue>( | |||
| ): Promise<void> { | |||
| await browser.storage.local.set({ [storageKey]: JSON.stringify(value) }); | |||
| } | |||
|
|
|||
| export async function onTabClose(watchedTabId: number): Promise<void> { | |||
| await new Promise<void>((resolve) => { | |||
There was a problem hiding this comment.
Do we need to unsubscribe the listener somehow after?
After the tab is closed, the timeout continues and then it throws a BusinessError.
Screen.Recording.mov