Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 7 commits into from
Jul 11, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion editor.planx.uk/src/@planx/components/Section/Public.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
6 changes: 2 additions & 4 deletions editor.planx.uk/src/@planx/components/shared/Preview/Card.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ const Card: React.FC<Props> = ({
...props
}) => {
const theme = useTheme();
const [path, currentCard] = useStore((state) => [
const [path, visibleNode] = useStore((state) => [
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Throughout this PR I've tired to maintain existing variable names when deconstructing state from the store. I think this minimises the number of changes, and hopefully maintains context within the component. For example, sometimes the currentCard is referred to as the node, visibleNode or currentCard across differing components.

state.path,
state.currentCard,
]);
Expand All @@ -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);
}, []);

Expand Down
9 changes: 4 additions & 5 deletions editor.planx.uk/src/components/Header.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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 &&
Expand Down
3 changes: 1 addition & 2 deletions editor.planx.uk/src/lib/feedback.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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(),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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(() => {
Expand Down Expand Up @@ -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!);

Expand All @@ -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!);

Expand Down
17 changes: 11 additions & 6 deletions editor.planx.uk/src/pages/FlowEditor/lib/__tests__/filters.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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", () => {
Expand Down Expand Up @@ -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");
});
});

Expand All @@ -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",
Expand Down Expand Up @@ -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",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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 = {
Expand Down Expand Up @@ -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);
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ import { vanillaStore } from "../../store";

const { getState, setState } = vanillaStore;

const { resetPreview, previousCard, currentCard } = getState();
const { resetPreview, previousCard, getCurrentCard } = getState();

beforeEach(() => {
resetPreview();
Expand All @@ -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", () => {
Expand All @@ -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", () => {
Expand All @@ -45,6 +45,6 @@ describe("store.previousCard is", () => {
},
});

expect(previousCard(currentCard())).toEqual("a");
expect(previousCard(getCurrentCard())).toEqual("a");
});
});
Loading
Loading