From ea24316c41ed897191c6a4087823371d1da43ea3 Mon Sep 17 00:00:00 2001 From: Daniel Falbel Date: Mon, 29 Jul 2024 19:53:19 -0300 Subject: [PATCH] Forward errors when performing View actions (#4122) This PR addresses https://github.com/posit-dev/positron/issues/3653 by forwarding the error when opening the Viewer to the user (using a notification) and suggesting to restart the session. It will also forward possible errors when performing `view` on Connections objects. --- .../tests/test_variables.py | 40 +++++++++++++++++++ .../positron/positron_ipykernel/variables.py | 20 ++++++++-- .../browser/components/variableItem.tsx | 10 ++++- .../browser/positronVariablesState.tsx | 2 + .../browser/positronVariablesView.tsx | 5 ++- 5 files changed, 71 insertions(+), 6 deletions(-) diff --git a/extensions/positron-python/python_files/positron/positron_ipykernel/tests/test_variables.py b/extensions/positron-python/python_files/positron/positron_ipykernel/tests/test_variables.py index e6121103d2c..eabc17758d7 100644 --- a/extensions/positron-python/python_files/positron/positron_ipykernel/tests/test_variables.py +++ b/extensions/positron-python/python_files/positron/positron_ipykernel/tests/test_variables.py @@ -696,6 +696,46 @@ def test_view_error(variables_comm: DummyComm) -> None: ] +def test_view_error_when_pandas_not_loaded( + shell: PositronShell, variables_comm: DummyComm, mock_dataexplorer_service: Mock +) -> None: + # regression test for https://github.com/posit-dev/positron/issues/3653 + shell.user_ns["x"] = pd.DataFrame({"a": [0]}) + + # Cases where the object has a viewer action, but no service reports it as + # supported. + def not_supported(value): + return False + + mock_dataexplorer_service.is_supported = not_supported + + path = _encode_path(["x"]) + msg = json_rpc_request("view", {"path": path}, comm_id="dummy_comm_id") + variables_comm.handle_msg(msg, raise_errors=False) + + assert variables_comm.messages == [ + json_rpc_error( + JsonRpcErrorCode.INTERNAL_ERROR, + f"Error opening viewer for variable at '{path}'. Object not supported. Try restarting the session.", + ) + ] + + # Case where the object has a viewer, but somehting wrong happens when checking + # if the object is supported. + def fail_is_supported(value): + raise TypeError("Not supported") + + mock_dataexplorer_service.is_supported = fail_is_supported + variables_comm.handle_msg(msg, raise_errors=False) + + assert [variables_comm.messages[-1]] == [ + json_rpc_error( + JsonRpcErrorCode.INTERNAL_ERROR, + f"Error opening viewer for variable at '{path}'. Try restarting the session.", + ) + ] + + def test_unknown_method(variables_comm: DummyComm) -> None: msg = json_rpc_request("unknown_method", comm_id="dummy_comm_id") variables_comm.handle_msg(msg, raise_errors=False) diff --git a/extensions/positron-python/python_files/positron/positron_ipykernel/variables.py b/extensions/positron-python/python_files/positron/positron_ipykernel/variables.py index ce817ec392c..1bd26085c37 100644 --- a/extensions/positron-python/python_files/positron/positron_ipykernel/variables.py +++ b/extensions/positron-python/python_files/positron/positron_ipykernel/variables.py @@ -576,10 +576,22 @@ def _perform_view_action(self, path: List[str]) -> None: f"Cannot find variable at '{path}' to view", ) - if self.kernel.connections_service.object_is_supported(value): - self._open_connections_pane(path, value) - elif self.kernel.data_explorer_service.is_supported(value): - self._open_data_explorer(path, value) + try: + if self.kernel.connections_service.object_is_supported(value): + self._open_connections_pane(path, value) + elif self.kernel.data_explorer_service.is_supported(value): + self._open_data_explorer(path, value) + else: + self._send_error( + JsonRpcErrorCode.INTERNAL_ERROR, + f"Error opening viewer for variable at '{path}'. Object not supported. Try restarting the session.", + ) + except Exception as err: + self._send_error( + JsonRpcErrorCode.INTERNAL_ERROR, + f"Error opening viewer for variable at '{path}'. Try restarting the session.", + ) + logger.error(err, exc_info=True) def _open_data_explorer(self, path: List[str], value: Any) -> None: """Opens a DataExplorer comm for the variable at the requested diff --git a/src/vs/workbench/contrib/positronVariables/browser/components/variableItem.tsx b/src/vs/workbench/contrib/positronVariables/browser/components/variableItem.tsx index 33c2633b903..439a8f2806b 100644 --- a/src/vs/workbench/contrib/positronVariables/browser/components/variableItem.tsx +++ b/src/vs/workbench/contrib/positronVariables/browser/components/variableItem.tsx @@ -123,7 +123,15 @@ export const VariableItem = (props: VariableItemProps) => { instance.requestFocus(); } else { // Open a viewer for the variable item. - const viewerId = await item.view(); + let viewerId: string | undefined; + try { + viewerId = await item.view(); + } catch (err) { + positronVariablesContext.notificationService.error(localize( + 'positron.variables.viewerError', + "An error occurred while opening the viewer. Try restarting your session." + )); + } // If a binding was returned, save the binding between the viewer and the variable item. if (viewerId) { diff --git a/src/vs/workbench/contrib/positronVariables/browser/positronVariablesState.tsx b/src/vs/workbench/contrib/positronVariables/browser/positronVariablesState.tsx index 0eb6d643f2c..d2f0fa89af7 100644 --- a/src/vs/workbench/contrib/positronVariables/browser/positronVariablesState.tsx +++ b/src/vs/workbench/contrib/positronVariables/browser/positronVariablesState.tsx @@ -13,6 +13,7 @@ import { IPositronVariablesInstance } from 'vs/workbench/services/positronVariab import { PositronActionBarServices } from 'vs/platform/positronActionBar/browser/positronActionBarState'; import { IRuntimeSessionService } from 'vs/workbench/services/runtimeSession/common/runtimeSessionService'; import { IPositronDataExplorerService } from 'vs/workbench/services/positronDataExplorer/browser/interfaces/positronDataExplorerService'; +import { INotificationService } from 'vs/platform/notification/common/notification'; /** * PositronVariablesServices interface. @@ -24,6 +25,7 @@ export interface PositronVariablesServices extends PositronActionBarServices { readonly positronVariablesService: IPositronVariablesService; readonly reactComponentContainer: IReactComponentContainer; readonly dataExplorerService: IPositronDataExplorerService; + readonly notificationService: INotificationService; } /** diff --git a/src/vs/workbench/contrib/positronVariables/browser/positronVariablesView.tsx b/src/vs/workbench/contrib/positronVariables/browser/positronVariablesView.tsx index 3dc04176dcf..6dd6134602c 100644 --- a/src/vs/workbench/contrib/positronVariables/browser/positronVariablesView.tsx +++ b/src/vs/workbench/contrib/positronVariables/browser/positronVariablesView.tsx @@ -29,6 +29,7 @@ import { IRuntimeSessionService } from 'vs/workbench/services/runtimeSession/com import { PositronViewPane } from 'vs/workbench/browser/positronViewPane/positronViewPane'; import { IHoverService } from 'vs/platform/hover/browser/hover'; import { IPositronDataExplorerService } from 'vs/workbench/services/positronDataExplorer/browser/interfaces/positronDataExplorerService'; +import { INotificationService } from 'vs/platform/notification/common/notification'; /** * PositronVariablesViewPane class. @@ -198,7 +199,8 @@ export class PositronVariablesViewPane extends PositronViewPane implements IReac @ITelemetryService telemetryService: ITelemetryService, @IThemeService themeService: IThemeService, @IViewDescriptorService viewDescriptorService: IViewDescriptorService, - @IWorkbenchLayoutService private readonly _layoutService: IWorkbenchLayoutService + @IWorkbenchLayoutService private readonly _layoutService: IWorkbenchLayoutService, + @INotificationService private readonly _notificationService: INotificationService ) { // Call the base class's constructor. super( @@ -268,6 +270,7 @@ export class PositronVariablesViewPane extends PositronViewPane implements IReac contextKeyService={this.contextKeyService} contextMenuService={this.contextMenuService} dataExplorerService={this._dataExplorerService} + notificationService={this._notificationService} hoverService={this.hoverService} keybindingService={this.keybindingService} runtimeSessionService={this._runtimeSessionService}