perspective-workspace: safer isVisible() callbacks#3076
perspective-workspace: safer isVisible() callbacks#3076tomjakubowski wants to merge 1 commit intoperspective-dev:masterfrom
Conversation
I've been seeing "cannot read properties of null" errors in these isVisible() callbacks while developing a React application which wraps `<perspective-workspace>`. The error message details suggest getWidgetByName() is returning null at some point. Make these isVisible() callbacks safer with the null coalescing operator, to avoid null 'dereferences' when `getWidgetByName()` returns null. In this case, the command is considered not visible. I didn't pin down reliable repro steps. Although from a look at this code's context, it might be related to a viewer widget being deleted from the workspace, or perhaps the viewer being renamed, while at the same time Lumino is rendering workspace commands (such as in the context menu). Signed-off-by: Tom Jakubowski <tom@prospective.dev>
|
Thanks for the PR!
@timbess can cherry-pick this change into #3044, but given the amount of work we're putting in to trying to fix
We should start by adding tests for these cases for sure. Neither viewer widget deletion nor renaming have tests, and there are multiple independent methods by which these actions may occur (By calling |
I've been seeing "cannot read properties of null" errors in these isVisible() callbacks while developing a React application which wraps
<perspective-workspace>. The error message details suggest getWidgetByName() is returning null at some point.Make these isVisible() callbacks safer with the null coalescing operator, to avoid null 'dereferences' when
getWidgetByName()returns null. In this case, the command is considered not visible.I didn't pin down reliable repro steps. Although from a look at this code's context, it might be related to a viewer widget being deleted from the workspace, or perhaps the viewer being renamed, while at the same time Lumino is rendering workspace commands (such as in the context menu).
Pull Request Checklist
Discussions, this PR applies to.