From 2fd311c521bfa1491da6ef196d9aed0d741d1d5c Mon Sep 17 00:00:00 2001
From: Phoomparin Mano <poom@metabase.com>
Date: Fri, 25 Oct 2024 12:54:27 +0700
Subject: [PATCH] fix(sdk): static question should cancel requests on component
unmount (#48808)
* cancel request on unmount in static question
* use deferred promise
* remove logs
* alias cancelled to deferred.promise
* extract useLoadStaticQuestion hook
* add tests for request cancellation
* change updateQuestion method
* add sanity check with fetchMock
---
.../public/StaticQuestion/StaticQuestion.tsx | 74 ++---------------
.../StaticQuestion.unit.spec.tsx | 18 ++++-
.../hooks/private/use-load-static-question.ts | 81 +++++++++++++++++++
.../embedding-sdk/lib/load-static-question.ts | 18 +++--
4 files changed, 114 insertions(+), 77 deletions(-)
create mode 100644 enterprise/frontend/src/embedding-sdk/hooks/private/use-load-static-question.ts
diff --git a/enterprise/frontend/src/embedding-sdk/components/public/StaticQuestion/StaticQuestion.tsx b/enterprise/frontend/src/embedding-sdk/components/public/StaticQuestion/StaticQuestion.tsx
index f53b5f1dc84..12ce4fe6bc6 100644
--- a/enterprise/frontend/src/embedding-sdk/components/public/StaticQuestion/StaticQuestion.tsx
+++ b/enterprise/frontend/src/embedding-sdk/components/public/StaticQuestion/StaticQuestion.tsx
@@ -1,5 +1,4 @@
import cx from "classnames";
-import { useEffect, useState } from "react";
import { t } from "ttag";
import {
@@ -7,11 +6,10 @@ import {
SdkLoader,
withPublicComponentWrapper,
} from "embedding-sdk/components/private/PublicComponentWrapper";
+import { useLoadStaticQuestion } from "embedding-sdk/hooks/private/use-load-static-question";
import { getDefaultVizHeight } from "embedding-sdk/lib/default-height";
-import { loadStaticQuestion } from "embedding-sdk/lib/load-static-question";
import CS from "metabase/css/core/index.css";
import { useValidatedEntityId } from "metabase/lib/entity-id/hooks/use-validated-entity-id";
-import type { GenericErrorResponse } from "metabase/lib/errors";
import { getResponseErrorMessage } from "metabase/lib/errors";
import { useSelector } from "metabase/lib/redux";
import QueryVisualization from "metabase/query_builder/components/QueryVisualization";
@@ -23,7 +21,7 @@ import { getMetadata } from "metabase/selectors/metadata";
import { Box, Group } from "metabase/ui";
import { PublicMode } from "metabase/visualizations/click-actions/modes/PublicMode";
import Question from "metabase-lib/v1/Question";
-import type { Card, CardEntityId, CardId, Dataset } from "metabase-types/api";
+import type { CardEntityId, CardId, Dataset } from "metabase-types/api";
export type StaticQuestionProps = {
questionId: CardId | CardEntityId;
@@ -32,13 +30,6 @@ export type StaticQuestionProps = {
parameterValues?: Record<string, string | number>;
};
-type State = {
- loading: boolean;
- card: Card | null;
- result: Dataset | null;
- error: GenericErrorResponse | null;
-};
-
type StaticQuestionVisualizationSelectorProps = {
question: Question;
result: Dataset | null;
@@ -87,63 +78,8 @@ const StaticQuestionInner = ({
const metadata = useSelector(getMetadata);
- const [{ loading, card, result, error }, setState] = useState<State>({
- loading: false,
- card: null,
- result: null,
- error: null,
- });
-
- useEffect(() => {
- async function loadCardData() {
- setState(prevState => ({
- ...prevState,
- loading: true,
- }));
-
- if (!questionId) {
- return;
- }
-
- try {
- const { card, result } = await loadStaticQuestion({
- questionId,
- parameterValues,
- });
-
- setState(prevState => ({
- ...prevState,
- card,
- result,
- loading: false,
- error: null,
- }));
- } catch (error) {
- if (typeof error === "object") {
- setState(prevState => ({
- ...prevState,
- result: null,
- card: null,
- loading: false,
- error,
- }));
- } else {
- console.error("error loading static question", error);
- }
- }
- }
-
- loadCardData();
- }, [questionId, parameterValues]);
-
- const changeVisualization = (newQuestion: Question) => {
- setState({
- card: newQuestion.card(),
- result: result,
- loading: false,
- error: null,
- });
- };
+ const { card, loading, result, error, updateQuestion } =
+ useLoadStaticQuestion(questionId, parameterValues);
const isLoading = loading || (!result && !error) || isValidatingEntityId;
@@ -173,7 +109,7 @@ const StaticQuestionInner = ({
<StaticQuestionVisualizationSelector
question={question}
result={result}
- onUpdateQuestion={changeVisualization}
+ onUpdateQuestion={updateQuestion}
/>
)}
<QueryVisualization
diff --git a/enterprise/frontend/src/embedding-sdk/components/public/StaticQuestion/StaticQuestion.unit.spec.tsx b/enterprise/frontend/src/embedding-sdk/components/public/StaticQuestion/StaticQuestion.unit.spec.tsx
index 9901398f3f3..2a595bb5bc2 100644
--- a/enterprise/frontend/src/embedding-sdk/components/public/StaticQuestion/StaticQuestion.unit.spec.tsx
+++ b/enterprise/frontend/src/embedding-sdk/components/public/StaticQuestion/StaticQuestion.unit.spec.tsx
@@ -1,3 +1,4 @@
+import { act } from "@testing-library/react";
import userEvent from "@testing-library/user-event";
import fetchMock from "fetch-mock";
@@ -85,7 +86,7 @@ const setup = ({
setupCardQueryEndpoints(card, TEST_DATASET);
- renderWithProviders(
+ return renderWithProviders(
<StaticQuestion
questionId={TEST_QUESTION_ID}
showVisualizationSelector={showVisualizationSelector}
@@ -174,4 +175,19 @@ describe("StaticQuestion", () => {
value: 1024,
});
});
+
+ it("should cancel the request when the component unmounts", async () => {
+ const abortSpy = jest.spyOn(AbortController.prototype, "abort");
+
+ const { unmount } = setup();
+
+ await act(async () => unmount());
+
+ expect(abortSpy).toHaveBeenCalled();
+ abortSpy.mockRestore();
+
+ // sanity check that the two requests were made initially
+ expect(fetchMock.calls(`path:/api/card/1`).length).toBe(1);
+ expect(fetchMock.calls(`path:/api/card/1/query`).length).toBe(1);
+ });
});
diff --git a/enterprise/frontend/src/embedding-sdk/hooks/private/use-load-static-question.ts b/enterprise/frontend/src/embedding-sdk/hooks/private/use-load-static-question.ts
new file mode 100644
index 00000000000..3abe084469d
--- /dev/null
+++ b/enterprise/frontend/src/embedding-sdk/hooks/private/use-load-static-question.ts
@@ -0,0 +1,81 @@
+import { useEffect, useState } from "react";
+
+import { loadStaticQuestion } from "embedding-sdk/lib/load-static-question";
+import type { GenericErrorResponse } from "metabase/lib/errors";
+import { defer } from "metabase/lib/promise";
+import type Question from "metabase-lib/v1/Question";
+import type { Card, Dataset } from "metabase-types/api";
+
+type QuestionState = {
+ loading: boolean;
+ card: Card | null;
+ result: Dataset | null;
+ error: GenericErrorResponse | null;
+};
+
+export function useLoadStaticQuestion(
+ questionId: number | null,
+ parameterValues?: Record<string, string | number>,
+) {
+ const [questionState, setQuestionState] = useState<QuestionState>({
+ loading: false,
+ card: null,
+ result: null,
+ error: null,
+ });
+
+ const updateQuestion = (newQuestion: Question) =>
+ setQuestionState(state => ({
+ ...state,
+ card: newQuestion.card(),
+ loading: false,
+ error: null,
+ }));
+
+ useEffect(() => {
+ const cancelDeferred = defer();
+
+ async function loadCardData() {
+ setQuestionState(state => ({ ...state, loading: true }));
+
+ if (!questionId) {
+ return;
+ }
+
+ try {
+ const { card, result } = await loadStaticQuestion({
+ questionId,
+ parameterValues,
+ cancelDeferred,
+ });
+
+ setQuestionState({
+ card,
+ result,
+ loading: false,
+ error: null,
+ });
+ } catch (error) {
+ if (typeof error === "object") {
+ setQuestionState({
+ result: null,
+ card: null,
+ loading: false,
+ error,
+ });
+ } else {
+ console.error("error loading static question", error);
+ }
+ }
+ }
+
+ loadCardData();
+
+ return () => {
+ // cancel pending requests upon unmount
+ cancelDeferred.resolve();
+ };
+ }, [questionId, parameterValues]);
+
+ return { ...questionState, updateQuestion };
+}
diff --git a/enterprise/frontend/src/embedding-sdk/lib/load-static-question.ts b/enterprise/frontend/src/embedding-sdk/lib/load-static-question.ts
index ab4bd190444..d29c7da86f5 100644
--- a/enterprise/frontend/src/embedding-sdk/lib/load-static-question.ts
+++ b/enterprise/frontend/src/embedding-sdk/lib/load-static-question.ts
@@ -1,24 +1,28 @@
+import type { Deferred } from "metabase/lib/promise";
import { CardApi } from "metabase/services";
import type { Card, Dataset, ParameterQueryObject } from "metabase-types/api";
interface Options {
questionId: number;
parameterValues?: Record<string, string | number>;
+ cancelDeferred?: Deferred;
}
type ParameterQueryInput = { id: string } & ParameterQueryObject;
export async function loadStaticQuestion(options: Options) {
- const { questionId, parameterValues } = options;
+ const { questionId, parameterValues, cancelDeferred } = options;
let card: Card | null;
let result: Dataset | null;
+ const cancelled = cancelDeferred?.promise;
+
[card, result] = await Promise.all([
- CardApi.get({ cardId: questionId }),
+ CardApi.get({ cardId: questionId }, { cancelled }),
// Query the card in parallel when no parameters are provided.
- !parameterValues && CardApi.query({ cardId: questionId }),
+ !parameterValues && CardApi.query({ cardId: questionId }, { cancelled }),
]);
if (parameterValues && card?.parameters) {
@@ -31,10 +35,10 @@ export async function loadStaticQuestion(options: Options) {
value: parameterValues[parameter.slug],
}));
- result = await CardApi.query({
- cardId: questionId,
- parameters,
- });
+ result = await CardApi.query(
+ { cardId: questionId, parameters },
+ { cancelled },
+ );
}
return { card, result };
--
GitLab