⚡ Rule pages - Do not render a page again when its content did not change

__tests__/app/revalidateRoute.test.ts

@@ -0,0 +1,103 @@
+/**
+ * @jest-environment node
+ */
+import { revalidatePath, revalidateTag } from "next/cache";
+import { POST } from "@/app/api/revalidate/route";
+import { GLOBAL_SETTINGS_TAG } from "@/lib/services/global";
+
+jest.mock("next/cache", () => ({ revalidatePath: jest.fn(), revalidateTag: jest.fn(), unstable_cache: (fn: unknown) => fn }));
+jest.mock("@/tina/__generated__/client", () => ({ __esModule: true, default: { queries: {} } }));
+
+const SECRET = "test-secret";
+
+const send = (body: unknown, secret: string = SECRET) =>
+  POST(
+    new Request("http://localhost/api/revalidate", {
+      method: "POST",
+      headers: { "content-type": "application/json", "x-revalidate-secret": secret },
+      body: JSON.stringify(body),
+    })
+  );
+
+const paths = () => (revalidatePath as jest.Mock).mock.calls.map(([path]) => path);
+const tags = () => (revalidateTag as jest.Mock).mock.calls.map(([tag]) => tag);
+
+describe("POST /api/revalidate", () => {
+  const savedSecret = process.env.TINA_WEBHOOK_SECRET;
+
+  beforeEach(() => {
+    jest.clearAllMocks();
+    process.env.TINA_WEBHOOK_SECRET = SECRET;
+  });
+  afterAll(() => {
+    process.env.TINA_WEBHOOK_SECRET = savedSecret;
+  });
+
+  it("rejects a request that has the wrong secret", async () => {
+    const res = await send({ type: "content.modified", paths: ["global/index.json"] }, "wrong");
+
+    expect(res.status).toBe(401);
+    expect(revalidatePath).not.toHaveBeenCalled();
+    expect(revalidateTag).not.toHaveBeenCalled();
+  });
+
+  it("ignores an event type that is not a content change", async () => {
+    const res = await send({ type: "content.removed", paths: ["public/uploads/rules/a-rule/rule.mdx"] });
+
+    expect(await res.json()).toMatchObject({ revalidated: false, ignored: true });
+    expect(revalidatePath).not.toHaveBeenCalled();
+  });
+
+  it("revalidates only the page and the card of an edited rule", async () => {
+    const res = await send({ type: "content.modified", paths: ["public/uploads/rules/3-steps-to-a-pbi/rule.mdx"] });
+
+    expect(await res.json()).toEqual({ revalidated: true, routes: ["/3-steps-to-a-pbi", "/3-steps-to-a-pbi/opengraph-image"] });
+    expect(paths()).toEqual(["/3-steps-to-a-pbi", "/3-steps-to-a-pbi/opengraph-image"]);
+    expect(revalidateTag).not.toHaveBeenCalled();
+  });
+
+  it("also revalidates the rule lists and the rule count for a new rule", async () => {
+    await send({ type: "content.added", paths: ["public/uploads/rules/a-new-rule/rule.mdx"] });
+
+    expect(paths()).toEqual(["/a-new-rule", "/a-new-rule/opengraph-image", "/api/rules"]);
+    expect(revalidateTag).toHaveBeenCalledWith("latest-rules", { expire: 0 });
+    expect(revalidateTag).toHaveBeenCalledWith("rule-count", { expire: 0 });
+  });
+
+  it("revalidates the category page, the home page and the category data for an edited category", async () => {
+    await send({ type: "content.modified", paths: ["categories/communication/rules-to-better-email.mdx"] });
+
+    expect(paths()).toEqual(["/opengraph-image", "/", "/rules-to-better-email", "/rules-to-better-email/opengraph-image"]);
+    expect(tags()).toEqual(["category-rule-data"]);
+  });
+
+  it("does not revalidate a category page for a top category index file", async () => {
+    await send({ type: "content.modified", paths: ["categories/communication/index.mdx"] });
+
+    expect(paths()).toEqual(["/opengraph-image", "/"]);
+  });
+
+  // The root layout reads the global settings, so no path identifies the pages. The tag is on each page.
+  it("marks all pages stale for a change of the global settings", async () => {
+    const res = await send({ type: "content.modified", paths: ["global/index.json"] });
+
+    expect(await res.json()).toEqual({ revalidated: true, routes: [] });
+    expect(revalidateTag).toHaveBeenCalledTimes(1);
+    expect(revalidateTag).toHaveBeenCalledWith(GLOBAL_SETTINGS_TAG, "max");
+    expect(revalidatePath).not.toHaveBeenCalled();
+  });
+
+  it("does not use the global settings tag for a rule or a category change", async () => {
+    await send({ type: "content.added", paths: ["public/uploads/rules/a-new-rule/rule.mdx", "categories/communication/rules-to-better-email.mdx"] });
+
+    expect(tags()).not.toContain(GLOBAL_SETTINGS_TAG);
+  });
+
+  it("revalidates nothing for a file that no page reads through this route", async () => {
+    const res = await send({ type: "content.modified", paths: ["README.md"] });
+
+    expect(await res.json()).toEqual({ revalidated: true, routes: [] });
+    expect(revalidatePath).not.toHaveBeenCalled();
+    expect(revalidateTag).not.toHaveBeenCalled();
+  });
+});

__tests__/lib/giscusDiscussions.test.ts

@@ -0,0 +1,57 @@
+/**
+ * @jest-environment node
+ */
+import { unstable_cache } from "next/cache";
+import { getDiscussionNumberForGuid } from "@/lib/services/github/giscusDiscussions.service";
+
+jest.mock("next/cache", () => ({ unstable_cache: jest.fn((fn: unknown) => fn) }));
+jest.mock("@/lib/services/github/github.utils", () => ({ getGitHubAppToken: jest.fn(async () => "token") }));
+
+// The module calls unstable_cache one time, when it loads
+const [, , cacheOptions] = (unstable_cache as jest.Mock).mock.calls[0];
+
+const discussionsPage = (nodes: { number: number; title: string; comments: { totalCount: number } }[]) =>
+  new Response(JSON.stringify({ data: { repository: { discussions: { pageInfo: { hasNextPage: false, endCursor: null }, nodes } } } }), { status: 200 });
+
+describe("giscus discussion numbers", () => {
+  const savedEnv = { ...process.env };
+  const savedFetch = global.fetch;
+
+  beforeEach(() => {
+    process.env.NEXT_PUBLIC_GITHUB_ORG = "SSWConsulting";
+    process.env.NEXT_PUBLIC_GISCUS_REPO_NAME = "SSW.Rules.Content";
+  });
+  afterEach(() => {
+    process.env = { ...savedEnv };
+    global.fetch = savedFetch;
+  });
+
+  // Each rule page calls the cached function, so a number here makes Next.js render each rule page again after that time
+  it("gives the rule pages no revalidation period", () => {
+    expect(cacheOptions.revalidate).toBeUndefined();
+    expect(cacheOptions.tags).toEqual(["giscus-discussion-numbers"]);
+  });
+
+  it("resolves a rule GUID to the discussion that has the most comments", async () => {
+    global.fetch = jest.fn(async () =>
+      discussionsPage([
+        { number: 12, title: "ABC-123", comments: { totalCount: 0 } },
+        { number: 7, title: "abc-123 ", comments: { totalCount: 4 } },
+        { number: 9, title: "other-guid", comments: { totalCount: 1 } },
+      ])
+    ) as unknown as typeof fetch;
+
+    await expect(getDiscussionNumberForGuid(" ABC-123 ")).resolves.toBe(7);
+    await expect(getDiscussionNumberForGuid("no-discussion-yet")).resolves.toBeNull();
+  });
+
+  // Keep this test last: after a failure, the service does not call GitHub for some minutes
+  it("returns null when GitHub fails, so the page falls back to the search mapping", async () => {
+    const warn = jest.spyOn(console, "warn").mockImplementation(() => {});
+    global.fetch = jest.fn(async () => new Response("", { status: 502, statusText: "Bad Gateway" })) as unknown as typeof fetch;
+
+    await expect(getDiscussionNumberForGuid("abc-123")).resolves.toBeNull();
+    expect(warn).toHaveBeenCalled();
+    warn.mockRestore();
+  });
+});

__tests__/lib/globalSettings.test.ts

@@ -0,0 +1,51 @@
+/**
+ * @jest-environment node
+ */
+import { unstable_cache } from "next/cache";
+import { GLOBAL_SETTINGS_TAG, getGlobalSettings } from "@/lib/services/global";
+import client from "@/tina/__generated__/client";
+
+jest.mock("next/cache", () => ({ unstable_cache: jest.fn((fn: unknown) => fn) }));
+jest.mock("@/tina/__generated__/client", () => ({ __esModule: true, default: { queries: { global: jest.fn() } } }));
+
+const globalQuery = (client as any).queries.global as jest.Mock;
+// The module calls unstable_cache one time, when it loads
+const [, , cacheOptions] = (unstable_cache as jest.Mock).mock.calls[0];
+
+beforeEach(() => globalQuery.mockReset());
+
+describe("getGlobalSettings", () => {
+  // The root layout calls it, so a number here makes Next.js render each page of the site again after that time
+  it("gives the pages no revalidation period", () => {
+    expect(cacheOptions.revalidate).toBeUndefined();
+  });
+
+  it("puts the global settings tag on the pages", () => {
+    expect(cacheOptions.tags).toEqual([GLOBAL_SETTINGS_TAG]);
+  });
+
+  it("returns the global settings document", async () => {
+    const settings = { theme: { color: "blue" } };
+    globalQuery.mockResolvedValue({ data: { global: settings } });
+
+    await expect(getGlobalSettings()).resolves.toEqual(settings);
+    expect(globalQuery).toHaveBeenCalledWith({ relativePath: "index.json" }, {});
+  });
+
+  // unstable_cache stores only a result. An error must stay an error, or the cache keeps a page without settings.
+  it("tries a throttled request again, then rejects", async () => {
+    jest.useFakeTimers();
+    const warn = jest.spyOn(console, "warn").mockImplementation(() => {});
+    globalQuery.mockRejectedValue(new Error("Throughput exceeds the current capacity of your table or index"));
+
+    try {
+      const result = expect(getGlobalSettings()).rejects.toThrow("Throughput exceeds");
+      await jest.runAllTimersAsync();
+      await result;
+      expect(globalQuery).toHaveBeenCalledTimes(5);
+    } finally {
+      warn.mockRestore();
+      jest.useRealTimers();
+    }
+  });
+});

app/api/revalidate/route.ts

@@ -1,5 +1,6 @@
 import { revalidatePath, revalidateTag } from "next/cache";
 import { NextResponse } from "next/server";
+import { GLOBAL_SETTINGS_TAG } from "@/lib/services/global";
 
 enum TINA_CONTENT_CHANGE_TYPE {
   Modified = "content.modified",
@@ -28,10 +29,16 @@ export async function POST(req: Request) {
     let shouldRevalidateCategoryTitles = false;
     let shouldRevalidateLatestRules = false;
     let shouldRevalidateRuleCount = false;
+    let shouldRevalidateGlobalSettings = false;
 
     for (const changedPath of changedPaths) {
       if (typeof changedPath !== "string") continue;
 
+      // Example: global/index.json
+      if (changedPath.startsWith("global/")) {
+        shouldRevalidateGlobalSettings = true;
+      }
+
       // Example: public/uploads/rules/3-steps-to-a-pbi/rule.mdx => /3-steps-to-a-pbi
       if (changedPath.startsWith("public/uploads/rules/")) {
         const slug = changedPath.replace("public/uploads/rules/", "").replace("/rule.mdx", "").replace(/\/+$/, "");
@@ -90,6 +97,13 @@ export async function POST(req: Request) {
       revalidatePath("/opengraph-image");
     }
 
+    if (shouldRevalidateGlobalSettings) {
+      // The tag is on each page. "max" marks the pages stale: a visitor gets the current page and Next.js
+      // renders the new one in the background. With `expire: 0`, the next visitor of each page waits for the
+      // render, and gets an error page if that render fails.
+      revalidateTag(GLOBAL_SETTINGS_TAG, "max");
+    }
+
     for (const route of routesToRevalidate) {
       revalidatePath(route);
     }

components/layout/layout.tsx

@@ -1,7 +1,6 @@
 import React, { PropsWithChildren } from "react";
-import { tinaRetryOptions, withTinaRetry } from "@/lib/tina/tinaRequest";
+import { getGlobalSettings } from "@/lib/services/global";
 import { getMegamenu } from "@/utils/get-mega-menu";
-import client from "../../tina/__generated__/client";
 import { HiddenOnChatPopOut } from "./HiddenOnChatPopOut";
 import { LayoutProvider } from "./layout-context";
 import { MenuProvider } from "./MenuProvider";
@@ -13,28 +12,15 @@ type LayoutProps = PropsWithChildren & {
 };
 
 export default async function Layout({ children, rawPageData }: LayoutProps) {
-  const { data: globalData } = await withTinaRetry("global", (attempt) =>
-    client.queries.global(
-      {
-        relativePath: "index.json",
-      },
-      tinaRetryOptions(attempt, {
-        fetchOptions: {
-          next: {
-            revalidate: 60,
-          },
-        },
-      })
-    )
-  ).catch((error) => {
+  const globalSettings = await getGlobalSettings().catch((error) => {
     console.error("[SiteLayout] global query failed:", error);
     throw error;
   });
 
   const { menuGroups } = await getMegamenu();
 
   return (
-    <LayoutProvider globalSettings={globalData.global} pageData={rawPageData}>
+    <LayoutProvider globalSettings={globalSettings} pageData={rawPageData}>
       <MenuProvider initialMenuGroups={menuGroups}>
         <div className="flex flex-col flex-1 min-h-full">
           <HiddenOnChatPopOut>

lib/services/github/giscusDiscussions.service.ts

@@ -2,9 +2,6 @@ import { unstable_cache } from "next/cache";
 import { getGitHubAppToken } from "./github.utils";
 
 const GITHUB_GRAPHQL_URL = "https://api.github.com/graphql";
-// Only applies when a page re-renders. Rule pages are force-static, so in practice
-// the number refreshes on each deploy, or when a rule edit revalidates its page.
-const SIX_HOURS = 6 * 60 * 60;
 const PAGE_SIZE = 100;
 // Safety stop so a pagination bug can never loop forever. 100 pages = 10,000 discussions.
 const MAX_PAGES = 100;
@@ -123,8 +120,11 @@ async function fetchDiscussionNumbersByGuid(): Promise<Record<string, number>> {
   return Object.fromEntries(Object.entries(best).map(([guid, { number }]) => [guid, number]));
 }
 
+// No `revalidate`: Next.js copies a number to each page that calls this function and renders the page again
+// after that time. Each rule page calls it, and a rule page must stay static until its rule changes.
+// The numbers are read again in each build, and when the Tina webhook revalidates a rule page: that render
+// does not use a cache entry that is older than the revalidation.
 export const getCachedDiscussionNumbersByGuid = unstable_cache(fetchDiscussionNumbersByGuid, ["giscus-discussion-numbers"], {
-  revalidate: SIX_HOURS,
   tags: ["giscus-discussion-numbers"],
 });
 

lib/services/global/globalSettings.service.ts

@@ -0,0 +1,20 @@
+import { unstable_cache } from "next/cache";
+import { tinaRetryOptions, withTinaRetry } from "@/lib/tina/tinaRequest";
+import client from "@/tina/__generated__/client";
+
+// The root layout reads the global settings, so this tag is on each page of the site.
+// A revalidation of the tag renders all pages again.
+export const GLOBAL_SETTINGS_TAG = "global-settings";
+
+async function fetchGlobalSettings() {
+  const { data } = await withTinaRetry("global", (attempt) => client.queries.global({ relativePath: "index.json" }, tinaRetryOptions(attempt)));
+  return data.global;
+}
+
+// - No `revalidate`: Next.js copies a number to each page that calls this function and renders the page again
+//   after that time. The root layout calls it, so a number here becomes the revalidation period of the whole
+//   site. A change gets to the pages through GLOBAL_SETTINGS_TAG.
+// - `unstable_cache`, not `fetchOptions` on the Tina request: in a build, the Tina client answers a repeated
+//   query from its own disk cache and does not call `fetch`. Tags and periods in `fetchOptions` then apply only
+//   to the few pages that send the request.
+export const getGlobalSettings = unstable_cache(fetchGlobalSettings, ["global-settings"], { tags: [GLOBAL_SETTINGS_TAG] });

lib/services/global/index.ts

@@ -0,0 +1 @@
+export * from "./globalSettings.service";