Skip to content

Commit

Permalink
Fix a couple of bugs related to reportError (#8123)
Browse files Browse the repository at this point in the history
  • Loading branch information
fregante authored Apr 4, 2024
1 parent ae5ba4c commit c1c3b4b
Show file tree
Hide file tree
Showing 9 changed files with 29 additions and 38 deletions.
7 changes: 2 additions & 5 deletions src/background/contextMenus.ts
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@
import pTimeout from "p-timeout";
import { type Menus, type Tabs } from "webextension-polyfill";
import chromeP from "webext-polyfill-kinda";
import reportError from "@/telemetry/reportError";
import { handleMenuAction, notify } from "@/contentScript/messenger/strict/api";
import { waitForContentScript } from "@/background/contentScript";
import { expectContext } from "@/utils/expectContext";
Expand Down Expand Up @@ -87,11 +86,9 @@ async function dispatchMenu(
args: info,
});
} catch (error) {
// Handle internal/messenger errors here. The real error handling occurs in the contextMenu extension point
reportError(error);
notify.error(target, {
message: "Error handling context menu action",
reportError: false,
// Handle internal/messenger errors here. The real error handling occurs in the contextMenu extension point
error: new Error("Error handling context menu action", { cause: error }),
});
}
}
Expand Down
12 changes: 3 additions & 9 deletions src/bricks/transformers/ephemeralForm/EphemeralForm.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@
* along with this program. If not, see <http://www.gnu.org/licenses/>.
*/

import React, { useEffect } from "react";
import React from "react";
import {
cancelForm,
getFormDefinition,
Expand All @@ -24,12 +24,12 @@ import Loader from "@/components/Loader";
import { getErrorMessage } from "@/errors/errorHelpers";
import { type Target } from "@/types/messengerTypes";
import { validateUUID } from "@/types/helpers";
import reportError from "@/telemetry/reportError";
import { TOP_LEVEL_FRAME_ID } from "@/domConstants";
import useAsyncState from "@/hooks/useAsyncState";
import { EphemeralFormContent } from "./EphemeralFormContent";
import EmotionShadowRoot from "@/components/EmotionShadowRoot";
import ErrorBoundary from "@/components/ErrorBoundary";
import useReportError from "@/hooks/useReportError";

const ModalLayout: React.FC = ({ children }) => (
// Don't use React Bootstrap's Modal because we want to customize the classes in the layout
Expand Down Expand Up @@ -66,13 +66,7 @@ const EphemeralForm: React.FC = () => {
error,
} = useAsyncState(async () => getFormDefinition(target, nonce), [nonce]);

// Report error once
useEffect(() => {
if (error) {
// TODO: https://github.com/pixiebrix/pixiebrix-extension/issues/2769
reportError(error);
}
}, [error]);
useReportError(error);

if (isLoading) {
return (
Expand Down
12 changes: 3 additions & 9 deletions src/bricks/transformers/temporaryInfo/EphemeralPanel.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@
*/

import cx from "classnames";
import React, { useEffect } from "react";
import React from "react";
import { Button, Modal, Popover } from "react-bootstrap";
import {
cancelTemporaryPanel,
Expand All @@ -26,7 +26,6 @@ import Loader from "@/components/Loader";
import { getErrorMessage } from "@/errors/errorHelpers";
import { type Target } from "@/types/messengerTypes";
import { validateUUID } from "@/types/helpers";
import reportError from "@/telemetry/reportError";
import ErrorBoundary from "@/components/ErrorBoundary";
import PanelBody from "@/sidebar/PanelBody";
import useTemporaryPanelDefinition from "@/bricks/transformers/temporaryInfo/useTemporaryPanelDefinition";
Expand All @@ -35,6 +34,7 @@ import { startCase } from "lodash";
import { type PanelButton } from "@/types/sidebarTypes";
import { ClosePanelAction } from "@/bricks/errors";
import styles from "./EphemeralPanel.module.scss";
import useReportError from "@/hooks/useReportError";

type Mode = "modal" | "popover";

Expand Down Expand Up @@ -98,13 +98,7 @@ const EphemeralPanel: React.FC = () => {
initialNonce,
);

// Report error once
useEffect(() => {
if (error) {
// TODO: https://github.com/pixiebrix/pixiebrix-extension/issues/2769
reportError(error);
}
}, [error]);
useReportError(error);

if (isLoading) {
return (
Expand Down
6 changes: 6 additions & 0 deletions src/errors/contextInvalidated.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
* along with this program. If not, see <http://www.gnu.org/licenses/>.
*/

import { isPageEditor } from "@/utils/expectContext";
import { getErrorMessage, getRootCause } from "./errorHelpers";
import { CONTEXT_INVALIDATED_ERROR } from "@/errors/knownErrorMessages";

Expand All @@ -30,6 +31,11 @@ const CONTEXT_INVALIDATED_NOTIFICATION_DURATION_MS = 20_000;
* all communication becomes impossible.
*/
export async function notifyContextInvalidated(): Promise<void> {
if (isPageEditor()) {
// It's one of the few contexts that stay open after invalidation, but it has its own InvalidatedContextGate
return;
}

// Lazily import React component. Also avoids a `webext-messenger` transitive dependency.
// https://github.com/pixiebrix/pixiebrix-extension/pull/6234
// https://github.com/pixiebrix/pixiebrix-extension/issues/4058#issuecomment-1217391772
Expand Down
2 changes: 1 addition & 1 deletion src/starterBricks/quickBarExtension.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -183,7 +183,7 @@ export abstract class QuickBarStarterBrickABC extends StarterBrickABC<QuickBarCo
try {
await this.registerExtensionAction(extension);
} catch (error) {
reportError(error, selectEventData(extension));
reportError(error, { context: selectEventData(extension) });
throw error;
}
});
Expand Down
4 changes: 2 additions & 2 deletions src/starterBricks/quickBarProviderExtension.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,6 @@ import {
import { castArray, cloneDeep, isEmpty } from "lodash";
import { checkAvailable, testMatchPatterns } from "@/bricks/available";
import reportError from "@/telemetry/reportError";
import { selectEventData } from "@/telemetry/deployments";
import { selectExtensionContext } from "@/starterBricks/helpers";
import { type BrickConfig, type BrickPipeline } from "@/bricks/types";
import { collectAllBricks } from "@/bricks/util";
Expand Down Expand Up @@ -64,6 +63,7 @@ import { allSettled } from "@/utils/promiseUtils";
import type { PlatformCapability } from "@/platform/capabilities";
import type { PlatformProtocol } from "@/platform/platformProtocol";
import { propertiesToSchema } from "@/utils/schemaUtils";
import { selectEventData } from "@/telemetry/deployments";

export type QuickBarProviderConfig = {
/**
Expand Down Expand Up @@ -223,7 +223,7 @@ export abstract class QuickBarProviderStarterBrickABC extends StarterBrickABC<Qu
try {
await this.registerActionProvider(extension);
} catch (error) {
reportError(error, selectEventData(extension));
reportError(error, { context: selectEventData(extension) });
throw error;
}
});
Expand Down
6 changes: 3 additions & 3 deletions src/telemetry/BackgroundLogger.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,11 +17,10 @@

import { type Logger, type MessageContext } from "@/types/loggerTypes";
import { type JsonObject } from "type-fest";
import { isBackground } from "webext-detect-page";
import { notifyContextInvalidated } from "@/errors/contextInvalidated";
import { wasContextInvalidated } from "webext-events";
import { recordLog } from "@/background/messenger/strict/api";
import { expectContext, isPageEditor } from "@/utils/expectContext";
import { expectContext } from "@/utils/expectContext";
import reportError from "@/telemetry/reportError";

/**
Expand Down Expand Up @@ -74,8 +73,9 @@ class BackgroundLogger implements Logger {
}

async error(error: unknown, data: JsonObject): Promise<void> {
if (wasContextInvalidated() && !isBackground() && !isPageEditor()) {
if (wasContextInvalidated()) {
void notifyContextInvalidated();
return;
}

console.error("BackgroundLogger:error", {
Expand Down
15 changes: 9 additions & 6 deletions src/telemetry/deployments.ts
Original file line number Diff line number Diff line change
@@ -1,13 +1,14 @@
import { type ModComponentBase } from "@/types/modComponentTypes";
import { type JsonObject } from "type-fest";
import type { Nullishable } from "@/utils/nullishUtils";
import { type MessageContext } from "@/types/loggerTypes";
import { isRegistryId } from "@/types/helpers";

/**
* Select data to report to the team admins for the deployment
*/
export function selectEventData(
modComponent: Nullishable<ModComponentBase>,
): JsonObject {
): MessageContext {
if (modComponent == null) {
return {};
}
Expand All @@ -19,9 +20,11 @@ export function selectEventData(
label: modComponent.label,
extensionId: modComponent.id,
deploymentId: modComponent._deployment?.id,
extensionPointId: modComponent.extensionPointId,
blueprintId: modComponent._recipe?.id ?? null,
blueprintVersion: modComponent._recipe?.version ?? null,
extensionPointId: isRegistryId(modComponent.extensionPointId)
? modComponent.extensionPointId
: undefined,
blueprintId: modComponent._recipe?.id,
blueprintVersion: modComponent._recipe?.version,
};
}

Expand All @@ -30,7 +33,7 @@ export function selectEventData(
label: modComponent.label,
extensionId: modComponent.id,
blueprintId: modComponent._recipe?.id,
blueprintVersion: modComponent._recipe?.version ?? null,
blueprintVersion: modComponent._recipe?.version,
};
}

Expand Down
3 changes: 0 additions & 3 deletions src/utils/modUtils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,6 @@ import {
import { type RegistryId } from "@/types/registryTypes";
import { type UUID } from "@/types/stringTypes";
import { InvalidTypeError } from "@/errors/genericErrors";
import reportError from "@/telemetry/reportError";
import { assertNotNullish } from "./nullishUtils";
import {
minimalSchemaFactory,
Expand Down Expand Up @@ -222,8 +221,6 @@ export function getSharingSource({
{ mod, organization, scope, installedExtensions },
);

reportError(error);

throw error;
}

Expand Down

0 comments on commit c1c3b4b

Please sign in to comment.