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
8 changes: 4 additions & 4 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -14,10 +14,10 @@
"lint": "turbo lint",
"format": "turbo format",
"format:check": "turbo format:check",
"test": "vitest run --config=./vitest/config.ts --workspace=./vitest/workspaces.ts",
"test:watch": "vitest run --config=./vitest/config.ts --workspace=./vitest/workspaces.ts --watch",
"test:coverage": "vitest run --config=./vitest/config.ts --workspace=./vitest/workspaces.ts --coverage",
"test:ci": "vitest run --config=./vitest/config.ts --workspace=./vitest/workspaces.ts --silent --coverage --reporter=junit --outputFile=test-report.junit.xml",
"test": "vitest run --config=./vitest/config.ts",
"test:watch": "vitest run --config=./vitest/config.ts --watch",
"test:coverage": "vitest run --config=./vitest/config.ts --coverage",
"test:ci": "vitest run --config=./vitest/config.ts --silent --coverage --reporter=junit --outputFile=test-report.junit.xml",
"test:integration": "yarn test:integration:react-18 && yarn test:integration:react-19",
"test:integration:react-18": "./integration/run-integration.sh 18.2.0",
"test:integration:react-19": "./integration/run-integration.sh 19.1.0",
Expand Down
7 changes: 7 additions & 0 deletions packages/client/src/clients/guide/client.ts
Original file line number Diff line number Diff line change
Expand Up @@ -217,6 +217,13 @@ const predicate = (
return false;
}

return checkActivatable(guide, location);
};

export const checkActivatable = (
guide: KnockGuide,
location: string | undefined,
) => {
const url = location ? newUrl(location) : undefined;

const urlRules = guide.activation_url_rules || [];
Expand Down
6 changes: 5 additions & 1 deletion packages/client/src/clients/guide/index.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,8 @@
export { KnockGuideClient, DEBUG_QUERY_PARAMS } from "./client";
export {
KnockGuideClient,
DEBUG_QUERY_PARAMS,
checkActivatable,
} from "./client";
export type {
KnockGuide,
KnockGuideStep,
Expand Down
Original file line number Diff line number Diff line change
@@ -1,9 +1,15 @@
import { Button } from "@telegraph/button";
import { Stack } from "@telegraph/layout";
import { Box, Stack } from "@telegraph/layout";
import { Tag } from "@telegraph/tag";
import { Tooltip } from "@telegraph/tooltip";
import { Text } from "@telegraph/typography";
import { CheckCircle2, CircleDashed, Eye, UserCircle2 } from "lucide-react";
import {
CheckCircle2,
CircleDashed,
Eye,
LocateFixed,
UserCircle2,
} from "lucide-react";
import * as React from "react";

import { GuideHoverCard } from "./GuideHoverCard";
Expand Down Expand Up @@ -43,6 +49,30 @@ export const GuideRow = ({ guide, orderIndex }: Props) => {
</Stack>

<Stack justify="flex-end">
{!isUnknownGuide(guide) && (
Copy link
Contributor

Choose a reason for hiding this comment

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

Uber nit: Feels like these could be grouped together w/ a single !isUnknownGuide(guide) check

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep you are right, will update.

<>
<Stack gap="1">
<Tooltip
label={
guide.annotation.activatable.status
? "This guide can be activated at the current location"
: "This guide cannot be activated at the current location"
}
>
<Button
px="1"
size="1"
variant="soft"
color={guide.annotation.activatable.status ? "green" : "red"}
leadingIcon={{ icon: LocateFixed, alt: "Target" }}
/>
</Tooltip>
</Stack>
<Stack px="2" align="center">
<Box h="3" borderLeft="px" borderColor="gray-6" />
</Stack>
</>
)}
<Stack gap="1">
{!isUnknownGuide(guide) && (
<>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,11 +22,9 @@ export const GuidesListDisplaySelect = ({ value, onChange }: Props) => {
style: { zIndex: TOOLBAR_Z_INDEX },
}}
>
{/* NOTE: Commented out until we can show relevant indicators for this opt
<Select.Option size="1" value="current-page">
Displayable on current page
</Select.Option>
*/}
<Select.Option size="1" value="current-page">
Displayable on current page
</Select.Option>
<Select.Option size="1" value="all-eligible">
All eligible guides for user
</Select.Option>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,12 @@ const GuidesList = ({
displayOption: DisplayOption;
}) => {
return guides.map((guide, idx) => {
if (
displayOption === "current-page" &&
(!guide.annotation.isEligible || !guide.annotation.isQualified)
) {
Copy link

Choose a reason for hiding this comment

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

Current-page filter overstates displayable guides

Medium Severity

The new current-page branch filters by guide.annotation.isEligible and guide.annotation.isQualified, but those annotations do not include the guide.steps.every((s) => !!s.message.archived_at) exclusion used by selection logic. This can show guides as “Displayable on current page” even when all steps are archived and they cannot actually render.

Additional Locations (1)

Fix in Cursor Fix in Web

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not an issue in this PR, as it will be captured in isEligible. It is a valid point for any new engagement interactions during a debug/toolbar session, but I want to tackle that as a separate PR.

return null;
}
if (displayOption === "all-eligible" && !guide.annotation.isEligible) {
return null;
}
Expand All @@ -39,7 +45,7 @@ export const V2 = () => {
const { client } = useGuideContext();

const [guidesListDisplayOption, setGuidesListDisplayOption] =
React.useState<DisplayOption>("all-eligible");
React.useState<DisplayOption>("current-page");

const [isVisible, setIsVisible] = React.useState(detectToolbarParam());
const [isCollapsed, setIsCollapsed] = React.useState(true);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import {
KnockGuide,
KnockGuideClientStoreState,
KnockGuideIneligibilityMarker,
checkActivatable,
} from "@knocklabs/client";
import { useGuideContext, useStore } from "@knocklabs/react-core";

Expand All @@ -28,6 +29,10 @@ type TargetableStatusFalse = {
};
type TargetableStatus = TargetableStatusTrue | TargetableStatusFalse;

type ActivatableStatus = {
status: boolean;
};

// Archived: `false` status = good
type ArchivedStatus = {
status: boolean;
Expand All @@ -38,12 +43,18 @@ type AnnotatedStatuses = {
active: ActiveStatus;
targetable: TargetableStatus;
archived: ArchivedStatus;
// Individual qualified statuses:
activatable: ActivatableStatus;
};

type GuideAnnotation = AnnotatedStatuses & {
// Resolved eligibility based on active, targetable and archived statuses,
// which are backend driven evaluation results that are exposed for debugging.
isEligible: boolean;

// Resolved display qualification based on an activatable status, which
// informs "when" and "where" an eligible guide can be displayed to user.
isQualified: boolean;
};

export type AnnotatedGuide = KnockGuide & {
Expand All @@ -64,6 +75,7 @@ export type UnknownGuide = {
bypass_global_group_limit: false;
annotation: {
isEligible: false;
isQualified: false;
};
};

Expand Down Expand Up @@ -116,28 +128,37 @@ const resolveIsEligible = ({
return true;
};

export const resolveIsQualified = ({ activatable }: AnnotatedStatuses) => {
if (!activatable.status) return false;
return true;
};

type StoreStateSnapshot = Pick<
KnockGuideClientStoreState,
"guides" | "guideGroups" | "ineligibleGuides" | "debug"
"location" | "guides" | "guideGroups" | "ineligibleGuides" | "debug"
>;

const annotateGuide = (
guide: KnockGuide,
snapshot: StoreStateSnapshot,
): AnnotatedGuide => {
const { ineligibleGuides } = snapshot;
const { ineligibleGuides, location } = snapshot;
const marker = ineligibleGuides[guide.key];
const ineligiblity = marker ? toIneligibilityStatus(marker) : undefined;

const statuses: AnnotatedStatuses = {
// isEligible:
active: ineligiblity?.active || { status: true },
targetable: ineligiblity?.targetable || { status: true },
archived: ineligiblity?.archived || { status: false },
// isQualified:
activatable: { status: checkActivatable(guide, location) },
};

const annotation: GuideAnnotation = {
...statuses,
isEligible: resolveIsEligible(statuses),
isQualified: resolveIsQualified(statuses),
};

return {
Expand All @@ -154,15 +175,17 @@ const newUnknownGuide = (key: KnockGuide["key"]) =>
bypass_global_group_limit: false,
annotation: {
isEligible: false,
isQualified: false,
},
}) as UnknownGuide;

export const useInspectGuideClientStore = (): InspectionResult | undefined => {
const { client } = useGuideContext();

// Extract a snapshot of the client store state for debugging.
const snapshot = useStore(client.store, (state) => {
const snapshot: StoreStateSnapshot = useStore(client.store, (state) => {
return {
location: state.location,
guides: state.guides,
guideGroups: state.guideGroups,
ineligibleGuides: state.ineligibleGuides,
Expand Down
Loading
Loading