diff --git a/packages/client/src/clients/guide/helpers.ts b/packages/client/src/clients/guide/helpers.ts index f6995ff33..49d64837a 100644 --- a/packages/client/src/clients/guide/helpers.ts +++ b/packages/client/src/clients/guide/helpers.ts @@ -146,6 +146,9 @@ export const predicateUrlRules = ( url: URL, urlRules: GuideActivationUrlRuleData[], ) => { + const hasBlockRulesOnly = urlRules.every((r) => r.directive === "block"); + const predicateDefault = hasBlockRulesOnly ? true : undefined; + return urlRules.reduce((acc, urlRule) => { // Any matched block rule prevails so no need to evaluate further // as soon as there is one. @@ -171,13 +174,18 @@ export const predicateUrlRules = ( return matched ? false : acc; } } - }, undefined); + }, predicateDefault); }; export const predicateUrlPatterns = ( url: URL, urlPatterns: KnockGuideActivationUrlPattern[], ) => { + const hasBlockPatternsOnly = urlPatterns.every( + (r) => r.directive === "block", + ); + const predicateDefault = hasBlockPatternsOnly ? true : undefined; + return urlPatterns.reduce((acc, urlPattern) => { // Any matched block rule prevails so no need to evaluate further // as soon as there is one. @@ -203,5 +211,5 @@ export const predicateUrlPatterns = ( return matched ? false : acc; } } - }, undefined); + }, predicateDefault); }; diff --git a/packages/client/test/clients/guide/guide.test.ts b/packages/client/test/clients/guide/guide.test.ts index a6a80d8ab..f3d50f580 100644 --- a/packages/client/test/clients/guide/guide.test.ts +++ b/packages/client/test/clients/guide/guide.test.ts @@ -1098,8 +1098,9 @@ describe("KnockGuideClient", () => { const client = new KnockGuideClient(mockKnock, channelId); const result = client["_selectGuide"](stateWithGuides); - // Should include the guide with allow directive for /dashboard - expect(result!.key).toBe("onboarding"); + // feature_tour has a block pattern for /settings, so it's allowed on /dashboard + // Since feature_tour comes first in display_sequence, it should be selected + expect(result!.key).toBe("feature_tour"); }); test("filters guides by url patterns - block directive", () => { @@ -1152,8 +1153,9 @@ describe("KnockGuideClient", () => { const client = new KnockGuideClient(mockKnock, channelId); const result = client["_selectGuide"](stateWithGuides); - // Should include the guide with allow rule for /dashboard - expect(result!.key).toBe("onboarding"); + // feature_tour still has a block pattern for /settings, so it's allowed on /dashboard + // Since feature_tour comes first in display_sequence, it should be selected + expect(result!.key).toBe("feature_tour"); }); test("filters guides by activation_url_rules - allow directive with contains", () => { @@ -1187,8 +1189,9 @@ describe("KnockGuideClient", () => { const client = new KnockGuideClient(mockKnock, channelId); const result = client["_selectGuide"](stateWithGuides); - // Should include the guide with contains rule matching "dash" - expect(result!.key).toBe("onboarding"); + // feature_tour still has a block pattern for /settings, so it's allowed on /dashboard + // Since feature_tour comes first in display_sequence, it should be selected + expect(result!.key).toBe("feature_tour"); }); test("filters guides by activation_url_rules - block directive with equal_to", () => { @@ -1298,11 +1301,10 @@ describe("KnockGuideClient", () => { const client = new KnockGuideClient(mockKnock, channelId); const result = client["_selectGuide"](stateWithGuides); - // Block rule should prevail even if allow rule also matches - // feature_tour is excluded because it has url_patterns that don't match this location - // onboarding is excluded because block rule prevails - // system_status has no rules and is included - expect(result!.key).toBe("system_status"); + // feature_tour has a block pattern for "/settings" which doesn't match "/admin/settings" + // Since it only has block patterns and doesn't match, it defaults to allowed + // feature_tour comes first in display_sequence, so it gets selected + expect(result!.key).toBe("feature_tour"); }); test("filters guides by activation_url_rules - multiple allow rules (any match allows)", () => { @@ -1342,8 +1344,10 @@ describe("KnockGuideClient", () => { const client = new KnockGuideClient(mockKnock, channelId); const result = client["_selectGuide"](stateWithGuides); - // Should allow the guide when any allow rule matches - expect(result!.key).toBe("onboarding"); + // feature_tour has a block pattern for "/settings" which doesn't match "/settings/profile" + // Since it only has block patterns and doesn't match, it defaults to allowed + // feature_tour comes first in display_sequence, so it gets selected + expect(result!.key).toBe("feature_tour"); }); test("filters guides by activation_url_rules - handles leading slash in arguments", () => { @@ -1377,8 +1381,10 @@ describe("KnockGuideClient", () => { const client = new KnockGuideClient(mockKnock, channelId); const result = client["_selectGuide"](stateWithGuides); - // Should handle argument without leading slash correctly - expect(result!.key).toBe("onboarding"); + // feature_tour has a block pattern for "/settings" which doesn't match "/dashboard" + // Since it only has block patterns and doesn't match, it defaults to allowed + // feature_tour comes first in display_sequence, so it gets selected + expect(result!.key).toBe("feature_tour"); }); test("filters guides by activation_url_rules - no match when url rules don't match", () => { @@ -1412,11 +1418,11 @@ describe("KnockGuideClient", () => { const client = new KnockGuideClient(mockKnock, channelId); const result = client["_selectGuide"](stateWithGuides); - // Should not match the guide when url rules don't match - // feature_tour is excluded because it has url_patterns that don't match this location - // onboarding is excluded because its url_rules don't match - // system_status has no rules and is included - expect(result!.key).toBe("system_status"); + // feature_tour has a block pattern for "/settings" which doesn't match "/dashboard" + // Since it only has block patterns and doesn't match, it defaults to allowed + // onboarding is excluded because its url_rules require "/admin" but location is "/dashboard" + // feature_tour comes first in display_sequence, so it gets selected + expect(result!.key).toBe("feature_tour"); }); test("handles guides without location when location is undefined", () => { diff --git a/packages/client/test/clients/guide/helpers.test.ts b/packages/client/test/clients/guide/helpers.test.ts index caa56f6ab..4c8764529 100644 --- a/packages/client/test/clients/guide/helpers.test.ts +++ b/packages/client/test/clients/guide/helpers.test.ts @@ -1,7 +1,15 @@ import { describe, expect, test } from "vitest"; - -import { evaluateUrlRule } from "../../../src/clients/guide/helpers"; -import type { GuideActivationUrlRuleData } from "../../../src/clients/guide/types"; +import { URLPattern } from "urlpattern-polyfill"; + +import { + evaluateUrlRule, + predicateUrlRules, + predicateUrlPatterns, +} from "../../../src/clients/guide/helpers"; +import type { + GuideActivationUrlRuleData, + KnockGuideActivationUrlPattern, +} from "../../../src/clients/guide/types"; describe("evaluateUrlRule", () => { describe("pathname variable with equal_to operator", () => { @@ -231,3 +239,305 @@ describe("evaluateUrlRule", () => { }); }); }); + +describe("predicateUrlRules", () => { + describe("with only block rules", () => { + test("defaults to true when all rules are block directives", () => { + const blockRules: GuideActivationUrlRuleData[] = [ + { + directive: "block", + variable: "pathname", + operator: "equal_to", + argument: "/admin", + }, + { + directive: "block", + variable: "pathname", + operator: "contains", + argument: "private", + }, + ]; + + // URL that doesn't match any block rules - should default to true + const url = new URL("https://example.com/public/page"); + expect(predicateUrlRules(url, blockRules)).toBe(true); + }); + + test("returns false when URL matches a block rule", () => { + const blockRules: GuideActivationUrlRuleData[] = [ + { + directive: "block", + variable: "pathname", + operator: "equal_to", + argument: "/admin", + }, + ]; + + const url = new URL("https://example.com/admin"); + expect(predicateUrlRules(url, blockRules)).toBe(false); + }); + + test("returns true for empty array of rules", () => { + const emptyRules: GuideActivationUrlRuleData[] = []; + const url = new URL("https://example.com/any/path"); + + // Empty array: every() returns true for empty arrays, so hasBlockRulesOnly is true, defaulting to true + expect(predicateUrlRules(url, emptyRules)).toBe(true); + }); + }); + + describe("with mixed allow and block rules", () => { + test("defaults to undefined when rules contain both allow and block directives", () => { + const mixedRules: GuideActivationUrlRuleData[] = [ + { + directive: "allow", + variable: "pathname", + operator: "equal_to", + argument: "/dashboard", + }, + { + directive: "block", + variable: "pathname", + operator: "equal_to", + argument: "/admin", + }, + ]; + + // URL that doesn't match any rules - should default to undefined + const url = new URL("https://example.com/other"); + expect(predicateUrlRules(url, mixedRules)).toBe(undefined); + }); + + test("returns true when URL matches an allow rule", () => { + const mixedRules: GuideActivationUrlRuleData[] = [ + { + directive: "allow", + variable: "pathname", + operator: "equal_to", + argument: "/dashboard", + }, + { + directive: "block", + variable: "pathname", + operator: "equal_to", + argument: "/admin", + }, + ]; + + const url = new URL("https://example.com/dashboard"); + expect(predicateUrlRules(url, mixedRules)).toBe(true); + }); + + test("returns false when URL matches a block rule even with allow rules present", () => { + const mixedRules: GuideActivationUrlRuleData[] = [ + { + directive: "allow", + variable: "pathname", + operator: "contains", + argument: "/admin", + }, + { + directive: "block", + variable: "pathname", + operator: "equal_to", + argument: "/admin/users", + }, + ]; + + const url = new URL("https://example.com/admin/users"); + expect(predicateUrlRules(url, mixedRules)).toBe(false); + }); + }); + + describe("with only allow rules", () => { + test("defaults to undefined when all rules are allow directives", () => { + const allowRules: GuideActivationUrlRuleData[] = [ + { + directive: "allow", + variable: "pathname", + operator: "equal_to", + argument: "/dashboard", + }, + { + directive: "allow", + variable: "pathname", + operator: "contains", + argument: "public", + }, + ]; + + // URL that doesn't match any allow rules - should default to undefined + const url = new URL("https://example.com/other"); + expect(predicateUrlRules(url, allowRules)).toBe(undefined); + }); + + test("returns true when URL matches an allow rule", () => { + const allowRules: GuideActivationUrlRuleData[] = [ + { + directive: "allow", + variable: "pathname", + operator: "equal_to", + argument: "/dashboard", + }, + ]; + + const url = new URL("https://example.com/dashboard"); + expect(predicateUrlRules(url, allowRules)).toBe(true); + }); + }); +}); + +describe("predicateUrlPatterns", () => { + describe("with only block patterns", () => { + test("defaults to true when all patterns are block directives", () => { + const blockPatterns: KnockGuideActivationUrlPattern[] = [ + { + directive: "block", + pattern: new URLPattern({ pathname: "/admin/*" }), + }, + { + directive: "block", + pattern: new URLPattern({ pathname: "/private/*" }), + }, + ]; + + // URL that doesn't match any block patterns - should default to true + const url = new URL("https://example.com/public/page"); + expect(predicateUrlPatterns(url, blockPatterns)).toBe(true); + }); + + test("returns false when URL matches a block pattern", () => { + const blockPatterns: KnockGuideActivationUrlPattern[] = [ + { + directive: "block", + pattern: new URLPattern({ pathname: "/admin/*" }), + }, + ]; + + const url = new URL("https://example.com/admin/settings"); + expect(predicateUrlPatterns(url, blockPatterns)).toBe(false); + }); + + test("returns true for empty array of patterns", () => { + const emptyPatterns: KnockGuideActivationUrlPattern[] = []; + const url = new URL("https://example.com/any/path"); + + // Empty array: every() returns true for empty arrays, so hasBlockPatternsOnly is true, defaulting to true + expect(predicateUrlPatterns(url, emptyPatterns)).toBe(true); + }); + }); + + describe("with mixed allow and block patterns", () => { + test("defaults to undefined when patterns contain both allow and block directives", () => { + const mixedPatterns: KnockGuideActivationUrlPattern[] = [ + { + directive: "allow", + pattern: new URLPattern({ pathname: "/dashboard/*" }), + }, + { + directive: "block", + pattern: new URLPattern({ pathname: "/admin/*" }), + }, + ]; + + // URL that doesn't match any patterns - should default to undefined + const url = new URL("https://example.com/other"); + expect(predicateUrlPatterns(url, mixedPatterns)).toBe(undefined); + }); + + test("returns true when URL matches an allow pattern", () => { + const mixedPatterns: KnockGuideActivationUrlPattern[] = [ + { + directive: "allow", + pattern: new URLPattern({ pathname: "/dashboard/*" }), + }, + { + directive: "block", + pattern: new URLPattern({ pathname: "/admin/*" }), + }, + ]; + + const url = new URL("https://example.com/dashboard/overview"); + expect(predicateUrlPatterns(url, mixedPatterns)).toBe(true); + }); + + test("returns false when URL matches a block pattern even with allow patterns present", () => { + const mixedPatterns: KnockGuideActivationUrlPattern[] = [ + { + directive: "allow", + pattern: new URLPattern({ pathname: "/admin/*" }), + }, + { + directive: "block", + pattern: new URLPattern({ pathname: "/admin/users" }), + }, + ]; + + const url = new URL("https://example.com/admin/users"); + expect(predicateUrlPatterns(url, mixedPatterns)).toBe(false); + }); + }); + + describe("with only allow patterns", () => { + test("defaults to undefined when all patterns are allow directives", () => { + const allowPatterns: KnockGuideActivationUrlPattern[] = [ + { + directive: "allow", + pattern: new URLPattern({ pathname: "/dashboard/*" }), + }, + { + directive: "allow", + pattern: new URLPattern({ pathname: "/public/*" }), + }, + ]; + + // URL that doesn't match any allow patterns - should default to undefined + const url = new URL("https://example.com/other"); + expect(predicateUrlPatterns(url, allowPatterns)).toBe(undefined); + }); + + test("returns true when URL matches an allow pattern", () => { + const allowPatterns: KnockGuideActivationUrlPattern[] = [ + { + directive: "allow", + pattern: new URLPattern({ pathname: "/dashboard/*" }), + }, + ]; + + const url = new URL("https://example.com/dashboard/stats"); + expect(predicateUrlPatterns(url, allowPatterns)).toBe(true); + }); + }); + + describe("edge cases", () => { + test("handles patterns with wildcards correctly", () => { + const patterns: KnockGuideActivationUrlPattern[] = [ + { + directive: "block", + pattern: new URLPattern({ pathname: "/*/admin" }), + }, + ]; + + const matchingUrl = new URL("https://example.com/users/admin"); + const nonMatchingUrl = new URL("https://example.com/public/page"); + + expect(predicateUrlPatterns(matchingUrl, patterns)).toBe(false); + expect(predicateUrlPatterns(nonMatchingUrl, patterns)).toBe(true); + }); + + test("handles exact URL patterns", () => { + const patterns: KnockGuideActivationUrlPattern[] = [ + { + directive: "block", + pattern: new URLPattern({ pathname: "/specific-page" }), + }, + ]; + + const exactUrl = new URL("https://example.com/specific-page"); + const differentUrl = new URL("https://example.com/other-page"); + + expect(predicateUrlPatterns(exactUrl, patterns)).toBe(false); + expect(predicateUrlPatterns(differentUrl, patterns)).toBe(true); + }); + }); +});