Skip to content
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
7 changes: 7 additions & 0 deletions .changeset/heavy-donkeys-greet.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
---
"@knocklabs/react-core": patch
"@knocklabs/client": patch
"@knocklabs/react": patch
---

[Guides] Use `readyToTarget` to coordinate fetching guides in guide toolbar v2
17 changes: 17 additions & 0 deletions packages/client/src/clients/guide/client.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import {
formatFilters,
formatGroupStage,
formatState,
getToolbarRunConfigFromUrl,
mockDefaultGroup,
newUrl,
predicateUrlPatterns,
Expand Down Expand Up @@ -600,6 +601,12 @@ export class KnockGuideClient {

setDebug(debugOpts?: Omit<DebugState, "debugging">) {
this.knock.log("[Guide] .setDebug()");

if (!this.knock.isAuthenticated()) {
this.knock.log("[Guide] Not authenticated, cannot start debugging");
return;
}

const shouldRefetch = !this.store.state.debug?.debugging;

// Clear the stage before updating the store state to let the next render
Expand Down Expand Up @@ -636,11 +643,17 @@ export class KnockGuideClient {

this.store.setState((state) => ({ ...state, debug: undefined }));

if (!this.knock.isAuthenticated()) {
this.knock.log("[Guide] Not authenticated, will not refetch");
return;
}

if (shouldRefetch) {
this.knock.log(
`[Guide] Stop debugging, refetching guides and resubscribing to the websocket channel`,
);
this.fetch({ force: true });
// TODO: Manage (re)subscribe/unsubscribe in V2 rather than here.
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done in #943.

this.subscribe();
}
}
Expand Down Expand Up @@ -1480,4 +1493,8 @@ export class KnockGuideClient {
this.replaceStateFn = undefined;
}
}

static getToolbarRunConfigFromUrl() {
return getToolbarRunConfigFromUrl();
}
}
38 changes: 38 additions & 0 deletions packages/client/src/clients/guide/helpers.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import {
GuideActivationUrlRuleData,
GuideData,
GuideGroupData,
KnockGuide,
KnockGuideActivationUrlPattern,
SelectFilterParams,
StoreState,
Expand Down Expand Up @@ -225,3 +226,40 @@ export const predicateUrlPatterns = (
}
}, predicateDefault);
};

// Use this param to start Toolbar v2 and enter into a debugging session when
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Just moved, not new.

// it is present and set to true.
const TOOLBAR_QUERY_PARAM = "knock_guide_toolbar";

// Optional, when present pin/focus on this guide.
const GUIDE_KEY_PARAM = "focused_guide_key";

export type ToolbarV2RunConfig = {
isVisible: boolean;
focusedGuideKeys?: Record<KnockGuide["key"], true>;
};

export const getToolbarRunConfigFromUrl = (): ToolbarV2RunConfig => {
const fallback = { isVisible: false };

if (typeof window === "undefined" || !window.location) {
return fallback;
}

const urlSearchParams = new URLSearchParams(window.location.search);
const toolbarParamValue = urlSearchParams.get(TOOLBAR_QUERY_PARAM);
const guideKeyParamValue = urlSearchParams.get(GUIDE_KEY_PARAM);

if (toolbarParamValue === null) {
return fallback;
}

const config: ToolbarV2RunConfig = {
isVisible: toolbarParamValue === "true",
};
if (guideKeyParamValue) {
config.focusedGuideKeys = { [guideKeyParamValue]: true };
}

return config;
};
1 change: 1 addition & 0 deletions packages/client/src/clients/guide/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ export {
checkActivatable,
} from "./client";
export { checkStateIfThrottled } from "./helpers";
export type { ToolbarV2RunConfig } from "./helpers";
export type {
KnockGuide,
KnockGuideStep,
Expand Down
35 changes: 35 additions & 0 deletions packages/client/test/clients/guide/guide.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4806,6 +4806,21 @@ describe("KnockGuideClient", () => {

expect(client.store.state.debug!.ignoreDisplayInterval).toBe(false);
});

test("returns early without setting debug when not authenticated", () => {
vi.mocked(mockKnock.isAuthenticated).mockReturnValue(false);
const client = new KnockGuideClient(mockKnock, channelId);
client.store.state.debug = undefined;

const fetchSpy = vi
.spyOn(client, "fetch")
.mockImplementation(() => Promise.resolve({ status: "ok" }));

client.setDebug();

expect(client.store.state.debug).toBe(undefined);
expect(fetchSpy).not.toHaveBeenCalled();
});
});

describe("unsetDebug", () => {
Expand Down Expand Up @@ -4868,6 +4883,26 @@ describe("KnockGuideClient", () => {
expect(subscribeSpy).not.toHaveBeenCalled();
});

test("clears debug state but does not fetch when not authenticated", () => {
const client = new KnockGuideClient(mockKnock, channelId);
client.store.state.debug = { debugging: true };

vi.mocked(mockKnock.isAuthenticated).mockReturnValue(false);

const fetchSpy = vi
.spyOn(client, "fetch")
.mockImplementation(() => Promise.resolve({ status: "ok" }));
const subscribeSpy = vi
.spyOn(client, "subscribe")
.mockImplementation(() => {});

client.unsetDebug();

expect(client.store.state.debug).toBe(undefined);
expect(fetchSpy).not.toHaveBeenCalled();
expect(subscribeSpy).not.toHaveBeenCalled();
});

test("does not call fetch and subscribe when debug exists but debugging is false", () => {
const client = new KnockGuideClient(mockKnock, channelId);
client.store.state.debug = { debugging: false };
Expand Down
58 changes: 57 additions & 1 deletion packages/client/test/clients/guide/helpers.test.ts
Original file line number Diff line number Diff line change
@@ -1,8 +1,9 @@
import { describe, expect, test } from "vitest";
import { describe, expect, test, vi, afterEach } from "vitest";
import { URLPattern } from "urlpattern-polyfill";

import {
evaluateUrlRule,
getToolbarRunConfigFromUrl,
predicateUrlRules,
predicateUrlPatterns,
} from "../../../src/clients/guide/helpers";
Expand Down Expand Up @@ -679,3 +680,58 @@ describe("predicateUrlPatterns", () => {
});
});
});

describe("getToolbarRunConfigFromUrl", () => {
const stubWindow = (search: string) => {
vi.stubGlobal("window", { location: { search } });
};

afterEach(() => {
vi.unstubAllGlobals();
vi.restoreAllMocks();
});

test("returns isVisible true when URL param is 'true'", () => {
stubWindow("?knock_guide_toolbar=true");

const config = getToolbarRunConfigFromUrl();

expect(config).toEqual({ isVisible: true });
});

test("returns isVisible false when URL param is 'false'", () => {
stubWindow("?knock_guide_toolbar=false");

const config = getToolbarRunConfigFromUrl();

expect(config).toEqual({ isVisible: false });
});

test("returns default config when URL param is absent", () => {
stubWindow("");

const config = getToolbarRunConfigFromUrl();

expect(config).toEqual({ isVisible: false });
});

test("includes focusedGuideKeys when focused_guide_key URL param is present", () => {
stubWindow("?knock_guide_toolbar=true&focused_guide_key=my_guide");

const config = getToolbarRunConfigFromUrl();

expect(config).toEqual({
isVisible: true,
focusedGuideKeys: { my_guide: true },
});
});

test("does not include focusedGuideKeys when focused_guide_key param is absent", () => {
stubWindow("?knock_guide_toolbar=true");

const config = getToolbarRunConfigFromUrl();

expect(config).toEqual({ isVisible: true });
expect(config).not.toHaveProperty("focusedGuideKeys");
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,11 @@ export const KnockGuideProvider: React.FC<
]);

React.useEffect(() => {
if (readyToTarget) {
// When the toolbar v2 is visible, defer fetch/subscribe to the toolbar
// so it can drive the debugging session lifecycle.
const toolbarRunConfig = KnockGuideClient.getToolbarRunConfigFromUrl();

if (readyToTarget && !toolbarRunConfig.isVisible) {
knockGuideClient.fetch();
if (listenForUpdates) knockGuideClient.subscribe();
}
Expand Down
30 changes: 29 additions & 1 deletion packages/react-core/test/guide/KnockGuideProvider.test.tsx
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import { renderHook } from "@testing-library/react";
import React from "react";
import { describe, expect, it, vi } from "vitest";
import { beforeEach, describe, expect, it, vi } from "vitest";

import { KnockGuideProvider } from "../../src/modules/guide/context/KnockGuideProvider";
import { useGuideContext } from "../../src/modules/guide/hooks/useGuideContext";
Expand Down Expand Up @@ -29,14 +29,19 @@ var subscribeMock: ReturnType<typeof vi.fn>;
// eslint-disable-next-line no-var
var cleanupMock: ReturnType<typeof vi.fn>;
// eslint-disable-next-line no-var
var getToolbarRunConfigFromUrlMock: ReturnType<typeof vi.fn>;
// eslint-disable-next-line no-var
var MockKnockGuideClient: new () => unknown;

vi.mock("@knocklabs/client", () => {
fetchMock = vi.fn();
subscribeMock = vi.fn();
cleanupMock = vi.fn();

getToolbarRunConfigFromUrlMock = vi.fn(() => ({ isVisible: false }));

MockKnockGuideClient = class {
static getToolbarRunConfigFromUrl = getToolbarRunConfigFromUrlMock;
fetch = fetchMock;
subscribe = subscribeMock;
cleanup = cleanupMock;
Expand All @@ -50,6 +55,13 @@ describe("KnockGuideProvider", () => {
// Deliberately omitting negative-case test that asserts throwing without KnockProvider
// as React swallows errors thrown during rendering, making the assertion unreliable

beforeEach(() => {
fetchMock.mockClear();
subscribeMock.mockClear();
cleanupMock.mockClear();
getToolbarRunConfigFromUrlMock.mockReturnValue({ isVisible: false });
});

it("provides context and triggers fetch/subscribe when ready", () => {
const wrapper: React.FC<{ children: React.ReactNode }> = ({ children }) =>
React.createElement(
Expand All @@ -65,4 +77,20 @@ describe("KnockGuideProvider", () => {
expect(fetchMock).toHaveBeenCalled();
expect(subscribeMock).toHaveBeenCalled();
});

it("defers fetch/subscribe to toolbar v2 when toolbar is visible", () => {
getToolbarRunConfigFromUrlMock.mockReturnValue({ isVisible: true });

const wrapper: React.FC<{ children: React.ReactNode }> = ({ children }) =>
React.createElement(
KnockGuideProvider,
{ channelId: "feed", readyToTarget: true },
children,
);

renderHook(() => useGuideContext(), { wrapper });

expect(fetchMock).not.toHaveBeenCalled();
expect(subscribeMock).not.toHaveBeenCalled();
});
});
17 changes: 12 additions & 5 deletions packages/react/src/modules/guide/components/Toolbar/V2/V2.tsx
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import { KnockGuideClient } from "@knocklabs/client";
import { useGuideContext } from "@knocklabs/react-core";
import { Button } from "@telegraph/button";
import { Icon } from "@telegraph/icon";
Expand All @@ -22,7 +23,7 @@ import "../styles.css";
import { FocusChin } from "./FocusChin";
import { GuideContextDetails } from "./GuideContextDetails";
import { GuideRow } from "./GuideRow";
import { DisplayOption, getRunConfig, sharedTooltipProps } from "./helpers";
import { DisplayOption, sharedTooltipProps } from "./helpers";
import { useDraggable } from "./useDraggable";
import {
InspectionResultOk,
Expand Down Expand Up @@ -84,12 +85,18 @@ const filterGuides = (
});
};

export const V2 = () => {
type Props = {
readyToTarget: boolean;
};

export const V2 = ({ readyToTarget }: Props) => {
const { client } = useGuideContext();

const [displayOption, setDisplayOption] =
React.useState<DisplayOption>("only-active");
const [runConfig, setRunConfig] = React.useState(() => getRunConfig());
const [runConfig, setRunConfig] = React.useState(() =>
KnockGuideClient.getToolbarRunConfigFromUrl(),
);
const [isCollapsed, setIsCollapsed] = React.useState(false);
const [isContextPanelOpen, setIsContextPanelOpen] = React.useState(false);

Expand All @@ -104,7 +111,7 @@ export const V2 = () => {
React.useEffect(() => {
const { isVisible = false, focusedGuideKeys = {} } = runConfig || {};
const isDebugging = client.store.state.debug?.debugging;
if (isVisible && !isDebugging) {
if (isVisible && !isDebugging && readyToTarget) {
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This is the main change: We check if readyToTarget is true before making the fetch call.

client.setDebug({ focusedGuideKeys });

// If focused, switch to all guides so you can see in the list.
Expand All @@ -116,7 +123,7 @@ export const V2 = () => {
return () => {
client.unsetDebug();
};
}, [runConfig, client, setDisplayOption]);
}, [readyToTarget, runConfig, client, setDisplayOption]);

// Toggle collapsed state with Ctrl + .
React.useEffect(() => {
Expand Down
Loading
Loading