fix: Reduce React re-renders when checking currentCard() (#3398)

theopensystemslab · Jul 11, 2024 · 495bb41 · 495bb41
1 parent a06557d
commit 495bb41
Show file tree

Hide file tree

Showing 16 changed files with 126 additions and 127 deletions.
diff --git a/editor.planx.uk/src/@planx/components/Section/Public.tsx b/editor.planx.uk/src/@planx/components/Section/Public.tsx
@@ -33,7 +33,7 @@ export default function Component(props: Props) {
     state.breadcrumbs,
     state.cachedBreadcrumbs,
     state.changeAnswer,
-    state.currentCard(),
+    state.currentCard,
     state.currentSectionIndex,
     state.flow,
     state.flowName,

diff --git a/editor.planx.uk/src/@planx/components/shared/Preview/Card.tsx b/editor.planx.uk/src/@planx/components/shared/Preview/Card.tsx
@@ -48,7 +48,7 @@ const Card: React.FC<Props> = ({
   ...props
 }) => {
   const theme = useTheme();
-  const [path, currentCard] = useStore((state) => [
+  const [path, visibleNode] = useStore((state) => [
     state.path,
     state.currentCard,
   ]);
@@ -57,9 +57,7 @@ const Card: React.FC<Props> = ({
   const { track } = useAnalyticsTracking();
 
   useEffect(() => {
-    // The Card component is only rendered when there's content the user will
-    // see
-    const visibleNode = currentCard();
+    // The Card component is only rendered when there's content the user will see
     if (visibleNode?.id) track(visibleNode?.id);
   }, []);
 

diff --git a/editor.planx.uk/src/components/Header.tsx b/editor.planx.uk/src/components/Header.tsx
@@ -259,20 +259,19 @@ const Breadcrumbs: React.FC = () => {
 };
 
 const NavBar: React.FC = () => {
-  const [index, sectionCount, title, hasSections, saveToEmail, path] = useStore(
-    (state) => [
+  const [index, sectionCount, title, hasSections, saveToEmail, path, node] =
+    useStore((state) => [
       state.currentSectionIndex,
       state.sectionCount,
       state.currentSectionTitle,
       state.hasSections,
       state.saveToEmail,
       state.path,
-    ],
-  );
+      state.currentCard,
+    ]);
   const isSaveAndReturnLandingPage =
     path !== ApplicationPath.SingleSession && !saveToEmail;
   const isContentPage = useCurrentRoute()?.data?.isContentPage;
-  const { node } = useAnalyticsTracking();
   const isSectionCard = node?.type == TYPES.Section;
   const isVisible =
     hasSections &&

diff --git a/editor.planx.uk/src/lib/feedback.ts b/editor.planx.uk/src/lib/feedback.ts
@@ -23,13 +23,12 @@ export type FeedbackMetadata = {
 export async function getInternalFeedbackMetadata(): Promise<FeedbackMetadata> {
   const {
     breadcrumbs,
-    currentCard,
+    currentCard: node,
     computePassport,
     fetchCurrentTeam,
     id: flowId,
   } = useStore.getState();
   const { id: teamId } = await fetchCurrentTeam();
-  const node = currentCard();
   const userData = {
     breadcrumbs: breadcrumbs,
     passport: computePassport(),

diff --git a/editor.planx.uk/src/pages/FlowEditor/lib/__tests__/clones.test.ts b/editor.planx.uk/src/pages/FlowEditor/lib/__tests__/clones.test.ts
@@ -3,7 +3,7 @@ const { getState, setState } = vanillaStore;
 import forwardsFlow from "./mocks/flowWithClones.json";
 import reverseFlow from "./mocks/flowWithReverseClones.json";
 
-const { record, previousCard, currentCard, upcomingCardIds, resetPreview } =
+const { record, previousCard, getCurrentCard, upcomingCardIds, resetPreview } =
   getState();
 
 beforeEach(() => {
@@ -54,14 +54,14 @@ describe("Clone order in flow (backwards)", () => {
     // Traverse forward to final node
     record("question", { answers: ["leftChoice"] });
     record("clone", { answers: ["finalNode"] });
-    expect(currentCard()?.id).toBe("finalNode");
+    expect(getCurrentCard()?.id).toBe("finalNode");
 
     // Traverse back one-by-one to first node
-    let previous = previousCard(currentCard());
+    let previous = previousCard(getCurrentCard());
     expect(previous).toBe("clone");
     record(previous!);
 
-    previous = previousCard(currentCard());
+    previous = previousCard(getCurrentCard());
     expect(previous).toBe("question");
     record(previous!);
 
@@ -79,18 +79,18 @@ describe("Clone order in flow (backwards)", () => {
     record("question", { answers: ["rightChoice"] });
     record("rightNotice", { answers: ["clone"] });
     record("clone", { answers: ["finalNode"] });
-    expect(currentCard()?.id).toBe("finalNode");
+    expect(getCurrentCard()?.id).toBe("finalNode");
 
     // Traverse back one-by-one to first node
-    let previous = previousCard(currentCard());
+    let previous = previousCard(getCurrentCard());
     expect(previous).toBe("clone");
     record(previous!);
 
-    previous = previousCard(currentCard());
+    previous = previousCard(getCurrentCard());
     expect(previous).toBe("rightNotice");
     record(previous!);
 
-    previous = previousCard(currentCard());
+    previous = previousCard(getCurrentCard());
     expect(previous).toBe("question");
     record(previous!);
 

diff --git a/editor.planx.uk/src/pages/FlowEditor/lib/__tests__/filters.test.ts b/editor.planx.uk/src/pages/FlowEditor/lib/__tests__/filters.test.ts
@@ -4,8 +4,13 @@ import flowWithBranchingFilters from "./mocks/flowWithBranchingFilters.json";
 import flowWithRootFilter from "./mocks/flowWithRootFilter.json";
 
 const { getState, setState } = vanillaStore;
-const { upcomingCardIds, resetPreview, record, currentCard, collectedFlags } =
-  getState();
+const {
+  upcomingCardIds,
+  resetPreview,
+  record,
+  getCurrentCard,
+  collectedFlags,
+} = getState();
 
 // https://i.imgur.com/k0kkKox.png
 describe("A filter on the root of the graph", () => {
@@ -74,9 +79,9 @@ describe("A filter on a branch", () => {
     record("fork", { answers: ["filter2"] });
 
     // XXX: Test fails here
-    // The currentCard returns as "immunityFlag2" which we should not land on -
+    // getCurrentCard returns as "immunityFlag2" which we should not land on -
     // the flags on the first filter are skipped, we go direct from "immunityPath1" to "fork"
-    expect(currentCard()?.id).toBe("immunityPath2");
+    expect(getCurrentCard()?.id).toBe("immunityPath2");
   });
 });
 
@@ -97,7 +102,7 @@ describe("Nodes on a filter path should only be auto-answered when the path matc
     record("TiIuAVIXsV", { answers: ["hdaeOVIXsV"], auto: false });
 
     // land on the correct result component
-    expect(currentCard()?.id).toBe("seN42VIXsV");
+    expect(getCurrentCard()?.id).toBe("seN42VIXsV");
     expect(getState().resultData()["Planning permission"]).toHaveProperty(
       "flag.value",
       "PLANNING_PERMISSION_REQUIRED",
@@ -131,7 +136,7 @@ describe("Nodes on a filter path should only be auto-answered when the path matc
     upcomingCardIds();
 
     // land on the correct result component
-    expect(currentCard()?.id).toBe("seN42VIXsV");
+    expect(getCurrentCard()?.id).toBe("seN42VIXsV");
     expect(getState().resultData()["Planning permission"]).toHaveProperty(
       "flag.value",
       "PLANNING_PERMISSION_REQUIRED",

diff --git a/editor.planx.uk/src/pages/FlowEditor/lib/__tests__/preview/canGoBack.test.ts b/editor.planx.uk/src/pages/FlowEditor/lib/__tests__/preview/canGoBack.test.ts
@@ -3,6 +3,8 @@ import { ComponentType as TYPES } from "@opensystemslab/planx-core/types";
 import { Store, vanillaStore } from "../../store";
 
 const { getState, setState } = vanillaStore;
+const { canGoBack, getCurrentCard, resetPreview, record, changeAnswer } =
+  getState();
 
 // https://imgur.com/VFV64ax
 const flow: Store.flow = {
@@ -58,98 +60,77 @@ const flow: Store.flow = {
 };
 
 beforeEach(() => {
-  getState().resetPreview();
+  resetPreview();
   setState({
     flow,
   });
 });
 
 describe("can go back if", () => {
   test("the previous component was manually answered", () => {
-    setState({
-      breadcrumbs: {
-        Question: {
-          auto: false,
-          answers: ["NoFeeAnswerPath"],
-        },
-      },
+    record("Question", {
+      auto: false,
+      answers: ["NoFeeAnswerPath"],
     });
-    expect(getState().canGoBack(getState().currentCard())).toStrictEqual(true);
+
+    expect(canGoBack(getCurrentCard())).toStrictEqual(true);
   });
 
   test("the user skipped the payment component", () => {
-    setState({
-      breadcrumbs: {
-        Question: {
-          auto: false,
-          answers: ["NoFeeAnswerPath"],
-        },
-        Pay: {
-          auto: true,
-        },
-      },
+    record("Question", {
+      auto: false,
+      answers: ["NoFeeAnswerPath"],
     });
-    expect(getState().canGoBack(getState().currentCard())).toStrictEqual(true);
+    record("Pay", { auto: true });
+
+    expect(canGoBack(getCurrentCard())).toStrictEqual(true);
   });
 });
 
 describe("cannot go back if", () => {
   test("it's the very first component", () => {
-    expect(getState().canGoBack(getState().currentCard())).toStrictEqual(false);
+    expect(canGoBack(getCurrentCard())).toStrictEqual(false);
   });
 
   test("the only previous component was auto-answered", () => {
-    setState({
-      breadcrumbs: {
-        Question: {
-          auto: true,
-          answers: ["NoFeeAnswerPath"],
-        },
-      },
+    record("Question", {
+      auto: true,
+      answers: ["NoFeeAnswerPath"],
     });
-    expect(getState().canGoBack(getState().currentCard())).toStrictEqual(false);
+
+    expect(canGoBack(getCurrentCard())).toStrictEqual(false);
   });
 
   test("the applicant made a payment", () => {
-    setState({
-      breadcrumbs: {
-        Question: {
-          auto: false,
-          answers: ["FeeAnswerPath"],
-        },
-        Calculate: {
-          auto: true,
-          data: {
-            fee: 10,
-          },
-        },
-        Pay: {
-          auto: false,
-        },
+    record("Question", {
+      auto: false,
+      answers: ["NoFeeAnswerPath"],
+    });
+    record("Calculate", {
+      auto: true,
+      data: {
+        fee: 10,
       },
     });
-    expect(getState().canGoBack(getState().currentCard())).toStrictEqual(false);
+    record("Pay", { auto: false });
+
+    expect(canGoBack(getCurrentCard())).toStrictEqual(false);
   });
 
   test("changing a component's answer", () => {
-    setState({
-      breadcrumbs: {
-        Question: {
-          auto: false,
-          answers: ["FeeAnswerPath"],
-        },
-        Calculate: {
-          auto: true,
-          data: {
-            fee: 10,
-          },
-        },
-        Pay: {
-          auto: false,
-        },
+    record("Question", {
+      auto: false,
+      answers: ["FeeAnswerPath"],
+    });
+    record("Calculate", {
+      auto: true,
+      data: {
+        fee: 10,
       },
-      changedNode: "Confirmation",
     });
-    expect(getState().canGoBack(getState().currentCard())).toStrictEqual(false);
+    record("Pay", { auto: false });
+    changeAnswer("Confirmation");
+
+    expect(canGoBack(getCurrentCard())).toStrictEqual(false);
   });
 });
diff --git a/editor.planx.uk/src/pages/FlowEditor/lib/__tests__/preview/overrideAnswer.test.ts b/editor.planx.uk/src/pages/FlowEditor/lib/__tests__/preview/overrideAnswer.test.ts
@@ -2,7 +2,7 @@ import { vanillaStore } from "../../store";
 
 const { getState, setState } = vanillaStore;
 
-const { overrideAnswer, currentCard, upcomingCardIds, record } = getState();
+const { overrideAnswer, getCurrentCard, upcomingCardIds, record } = getState();
 
 test("it clears the correct breadcrumb and navigates back to the right node", async () => {
   // set up initial state, confirm our passport computes as expected
@@ -38,7 +38,7 @@ test("it clears the correct breadcrumb and navigates back to the right node", as
   });
 
   // confirm we've navigated back to the right node, and that PropertyInformation is queued up again in upcoming cards
-  expect(currentCard()?.id).toEqual("FirstPropertyTypeQuestionNodeId");
+  expect(getCurrentCard()?.id).toEqual("FirstPropertyTypeQuestionNodeId");
   expect(upcomingCardIds()).toContain("PropertyInformationNodeId");
 
   // select a new answer, confirm our passport has updated

diff --git a/editor.planx.uk/src/pages/FlowEditor/lib/__tests__/preview/previousCard.test.ts b/editor.planx.uk/src/pages/FlowEditor/lib/__tests__/preview/previousCard.test.ts
@@ -2,7 +2,7 @@ import { vanillaStore } from "../../store";
 
 const { getState, setState } = vanillaStore;
 
-const { resetPreview, previousCard, currentCard } = getState();
+const { resetPreview, previousCard, getCurrentCard } = getState();
 
 beforeEach(() => {
   resetPreview();
@@ -24,7 +24,7 @@ const setup = (args = {}) =>
 describe("store.previousCard is", () => {
   test("undefined when there are no breadcrumbs", () => {
     setup();
-    expect(previousCard(currentCard())).toBeUndefined();
+    expect(previousCard(getCurrentCard())).toBeUndefined();
   });
 
   test("undefined when cards were automatically answered", () => {
@@ -34,7 +34,7 @@ describe("store.previousCard is", () => {
         b: { auto: true },
       },
     });
-    expect(previousCard(currentCard())).toBeUndefined();
+    expect(previousCard(getCurrentCard())).toBeUndefined();
   });
 
   test("the most recent human-answered card id", () => {
@@ -45,6 +45,6 @@ describe("store.previousCard is", () => {
       },
     });
 
-    expect(previousCard(currentCard())).toEqual("a");
+    expect(previousCard(getCurrentCard())).toEqual("a");
   });
 });