Resolve revision and change request URL correctly (#98)

* Add tests for getURLLookupAlternatives

* Match correctly revisions and changes URLs

* Use revision and changerequets ID passed from lookup

* Prevent indexing these urls

* Start

* Start rendering toolbar

* Update api client

* Format

* Format

* Mock resolve of snippet

* Add test for revision

package.json

@@ -15,7 +15,7 @@
     },
     "dependencies": {
         "@geist-ui/icons": "^1.0.2",
-        "@gitbook/api": "^0.25.0",
+        "@gitbook/api": "^0.26.0",
         "@radix-ui/react-checkbox": "^1.0.4",
         "@radix-ui/react-popover": "^1.0.7",
         "@readme/openapi-parser": "^2.5.0",

src/app/[spaceId]/(content)/layout.tsx

@@ -3,6 +3,7 @@ import { Metadata, Viewport } from 'next';
 import Script from 'next/script';
 import React from 'react';
 
+import { AdminToolbar } from '@/components/AdminToolbar';
 import { CookiesToast } from '@/components/Cookies';
 import { SpaceLayout } from '@/components/SpaceLayout';
 import { getContentSecurityPolicyNonce } from '@/lib/csp';
@@ -57,6 +58,10 @@ export default async function ContentLayout(props: {
                     <CookiesToast privacyPolicy={customization.privacyPolicy.url} />
                 </React.Suspense>
             ) : null}
+
+            {content.revisionId || content.changeRequestId ? (
+                <AdminToolbar content={content} />
+            ) : null}
         </>
     );
 }

src/app/[spaceId]/(core)/sitemap.xml/route.ts

@@ -5,7 +5,7 @@ import { NextRequest } from 'next/server';
 import { getRevisionPages } from '@/lib/api';
 import { pageHref } from '@/lib/links';
 
-import { SpaceParams } from '../../fetch';
+import { SpaceParams, getContentPointer } from '../../fetch';
 
 export const runtime = 'edge';
 
@@ -13,7 +13,7 @@ export const runtime = 'edge';
  * Generate a sitemap.xml for the current space.
  */
 export async function GET(req: NextRequest, { params }: { params: SpaceParams }) {
-    const rootPages = await getRevisionPages(params);
+    const rootPages = await getRevisionPages(getContentPointer(params));
     const pages = flattenPages(rootPages);
     const urls = pages.map(({ page, depth }) => {
         // Decay priority with depth

src/app/[spaceId]/fetch.ts

@@ -1,4 +1,5 @@
 import { ContentVisibility, RevisionPage, Space } from '@gitbook/api';
+import { headers } from 'next/headers';
 
 import {
     getCollectionSpaces,
@@ -10,7 +11,9 @@ import {
 } from '@/lib/api';
 import { resolvePagePath, resolvePageId } from '@/lib/pages';
 
-export type SpaceParams = ContentPointer;
+export interface SpaceParams {
+    spaceId: string;
+}
 
 export interface PagePathParams extends SpaceParams {
     pathname?: string[];
@@ -20,16 +23,25 @@ export interface PageIdParams extends SpaceParams {
     pageId?: string;
 }
 
+/**
+ * Get the current content pointer from the params.
+ */
+export function getContentPointer(params: PagePathParams | PageIdParams) {
+    const headerSet = headers();
+    const content: ContentPointer = {
+        spaceId: params.spaceId,
+        revisionId: headerSet.get('x-gitbook-content-revision') ?? undefined,
+        changeRequestId: headerSet.get('x-gitbook-content-changerequest') ?? undefined,
+    };
+
+    return content;
+}
+
 /**
  * Fetch all the data needed to render the space layout.
  */
 export async function fetchSpaceData(params: PagePathParams | PageIdParams) {
-    const content: ContentPointer = {
-        spaceId: params.spaceId,
-        changeRequestId: params.changeRequestId,
-        revisionId: params.revisionId,
-    };
-
+    const content = getContentPointer(params);
     const { space, pages, customization, scripts } = await getSpaceContent(content);
     const collection = await fetchParentCollection(space);
 
@@ -49,12 +61,7 @@ export async function fetchSpaceData(params: PagePathParams | PageIdParams) {
  * Optimized to fetch in parallel as much as possible.
  */
 export async function fetchPageData(params: PagePathParams | PageIdParams) {
-    const content: ContentPointer = {
-        spaceId: params.spaceId,
-        changeRequestId: params.changeRequestId,
-        revisionId: params.revisionId,
-    };
-
+    const content = getContentPointer(params);
     const { space, pages, customization, scripts } = await getSpaceContent(content);
 
     const page = await resolvePage(pages, content, params);
@@ -120,5 +127,5 @@ async function fetchParentCollection(space: Space) {
  */
 export function getPathnameParam(params: PagePathParams): string {
     const { pathname } = params;
-    return pathname ? pathname.map(part => decodeURIComponent(part)).join('/') : '';
+    return pathname ? pathname.map((part) => decodeURIComponent(part)).join('/') : '';
 }

src/components/AdminToolbar/AdminToolbar.tsx

@@ -0,0 +1,95 @@
+import React from 'react';
+
+import { ContentPointer, getChangeRequest, getRevision, getRevisionPages } from '@/lib/api';
+import { tcls } from '@/lib/tailwind';
+
+interface AdminToolbarProps {
+    content: ContentPointer;
+}
+
+/**
+ * Toolbar with information for the content admin when previewing a revision or change-request.
+ */
+export function AdminToolbar(props: AdminToolbarProps) {
+    const { content } = props;
+
+    return (
+        <div
+            className={tcls(
+                'fixed',
+                'bottom-5',
+                'left-1/2',
+                'z-50',
+                'transform',
+                '-translate-x-1/2',
+                'rounded-full',
+                'bg-slate-950',
+                'shadow-lg',
+                'min-h-10',
+                'min-w-40',
+                'p-2',
+                'max-w-md',
+                'border-slate-300',
+            )}
+        >
+            <React.Suspense fallback={null}>
+                {content.changeRequestId ? (
+                    <ChangeRequestToolbar
+                        spaceId={content.spaceId}
+                        changeRequestId={content.changeRequestId}
+                    />
+                ) : null}
+                {content.revisionId ? (
+                    <RevisionToolbar spaceId={content.spaceId} revisionId={content.revisionId} />
+                ) : null}
+            </React.Suspense>
+        </div>
+    );
+}
+
+async function ChangeRequestToolbar(props: { spaceId: string; changeRequestId: string }) {
+    const { spaceId, changeRequestId } = props;
+
+    const changeRequest = await getChangeRequest(spaceId, changeRequestId);
+
+    return (
+        <ToolbarButton href={changeRequest.urls.app}>
+            Change request #{changeRequest.number}: {changeRequest.subject ?? 'No subject'}
+        </ToolbarButton>
+    );
+}
+
+async function RevisionToolbar(props: { spaceId: string; revisionId: string }) {
+    const { spaceId, revisionId } = props;
+
+    const revision = await getRevision(spaceId, revisionId);
+
+    return (
+        <ToolbarButton href={revision.urls.app}>
+            Revision created on {new Date(revision.createdAt).toLocaleDateString()}
+        </ToolbarButton>
+    );
+}
+
+function ToolbarButton(props: { href: string; children: React.ReactNode }) {
+    const { href, children } = props;
+
+    return (
+        <a
+            href={href}
+            className={tcls(
+                'block',
+                'text-sm',
+                'px-4',
+                'py-1',
+                'text-slate-400',
+                'rounded-full',
+                'hover:bg-slate-800',
+                'hover:text-white',
+                'truncate',
+            )}
+        >
+            {children}
+        </a>
+    );
+}

src/components/AdminToolbar/index.ts

@@ -0,0 +1 @@
+export * from './AdminToolbar';

src/components/Search/SearchScopeToggle.tsx

@@ -18,7 +18,7 @@ export function SearchScopeToggle(props: { spaceTitle: string }) {
 
     return (
         <div
-            aria-role="toolbar"
+            role="toolbar"
             aria-orientation="horizontal"
             className={tcls('flex', 'flex-row', 'gap-3', 'py-3', 'px-4', 'pt-0')}
         >
@@ -53,6 +53,8 @@ function ToggleButton(props: { onClick: () => void; children: React.ReactNode; a
 
     return (
         <button
+            role="tab"
+            type="button"
             aria-selected={active}
             onClick={onClick}
             className={tcls(

src/components/TableOfContents/ToggleableLinkItem.tsx

@@ -77,7 +77,7 @@ export function ToggleableLinkItem(props: {
         else {
             animate('& > ul > li', { opacity: 0 });
         }
-    }, [isVisible, hasDescendants]);
+    }, [isVisible, hasDescendants, animate, scope]);
 
     // Track if the component is mounted.
     const mountedRef = React.useRef(false);

src/lib/api.ts

@@ -156,6 +156,25 @@ export const getSpace = cache('api.getSpace', async (spaceId: string) => {
     });
 });
 
+/**
+ * Get a change request by its ID.
+ */
+export const getChangeRequest = cache(
+    'api.getChangeRequest',
+    async (spaceId: string, changeRequestId: string) => {
+        const response = await api().spaces.getChangeRequestById(spaceId, changeRequestId, {
+            ...noCacheFetchOptions,
+        });
+        return cacheResponse(response, {
+            tags: [],
+        });
+    },
+    {
+        // We don't cache for long s we currently don't invalidate change-request cache
+        defaultTtl: 60 * 60,
+    },
+);
+
 /**
  * List the scripts to load for the space.
  */
@@ -174,6 +193,22 @@ export const getSpaceIntegrationScripts = cache(
     },
 );
 
+/**
+ * Get a revision by its ID.
+ */
+export const getRevision = cache('api.getRevision', async (spaceId: string, revisionId: string) => {
+    const response = await api().spaces.getRevisionById(spaceId, revisionId, {
+        ...noCacheFetchOptions,
+    });
+
+    return cacheResponse(response, {
+        data: response.data,
+        tags: [
+            // Revision are immutable so we don't cache
+        ],
+    });
+});
+
 /**
  * Get all the pages in the space.
  */
@@ -186,7 +221,7 @@ export const getRevisionPages = cache('api.getRevisionPages', async (pointer: Co
         }
 
         if (pointer.changeRequestId) {
-            return api().spaces.listPagesInChangeRequest(spaceId, pointer.changeRequestId, {
+            return api().spaces.listPagesInChangeRequest(pointer.spaceId, pointer.changeRequestId, {
                 ...noCacheFetchOptions,
             });
         }

src/lib/cache/cache.ts

@@ -41,6 +41,9 @@ export function cache<Args extends any[], Result>(
     options: {
         /** Filter the arguments that should be taken into consideration for cachine */
         extractArgs?: (args: Args) => any[];
+
+        /** Default ttl (in seconds) */
+        defaultTtl?: number;
     } = {},
 ): CacheFunction<Args, Result> {
     const revalidate = async (key: string, ...args: Args) => {
@@ -55,7 +58,7 @@ export function cache<Args extends any[], Result>(
             meta: {
                 cache: cacheName,
                 tags: result.tags ?? [],
-                expiresAt: Date.now() + (result.ttl ?? 60 * 60 * 24) * 1000,
+                expiresAt: Date.now() + (result.ttl ?? options.defaultTtl ?? 60 * 60 * 24) * 1000,
                 args,
                 hits: 1,
             },

src/lib/middleware.test.ts

@@ -0,0 +1,67 @@
+import { describe, it, expect } from 'bun:test';
+
+import { getURLLookupAlternatives } from './middleware';
+
+describe('getURLLookupAlternatives', () => {
+    it('should return all URLs up to the root', () => {
+        expect(getURLLookupAlternatives(new URL('https://docs.mycompany.com/a/b/c'))).toEqual([
+            {
+                extraPath: 'a/b/c',
+                url: 'https://docs.mycompany.com/',
+            },
+            {
+                extraPath: 'b/c',
+                url: 'https://docs.mycompany.com/a',
+            },
+            {
+                extraPath: '',
+                url: 'https://docs.mycompany.com/a/b/c',
+            },
+        ]);
+    });
+
+    it('should not match before the variant for a variant url', () => {
+        expect(
+            getURLLookupAlternatives(new URL('https://test.gitbook.io/v/variant/space')),
+        ).toEqual([
+            {
+                url: 'https://test.gitbook.io/v/variant',
+                extraPath: 'space',
+            },
+            {
+                url: 'https://test.gitbook.io/v/variant/space',
+                extraPath: '',
+            },
+        ]);
+    });
+
+    it('should not match before a revision ID', () => {
+        expect(
+            getURLLookupAlternatives(new URL('https://docs.mycompany.com/~/revisions/id/hello')),
+        ).toEqual([
+            {
+                extraPath: 'hello',
+                url: 'https://docs.mycompany.com/~/revisions/id',
+            },
+            {
+                extraPath: '',
+                url: 'https://docs.mycompany.com/~/revisions/id/hello',
+            },
+        ]);
+    });
+
+    it('should not match before a revision ID', () => {
+        expect(
+            getURLLookupAlternatives(new URL('https://docs.mycompany.com/~/changes/id/hello')),
+        ).toEqual([
+            {
+                extraPath: 'hello',
+                url: 'https://docs.mycompany.com/~/changes/id',
+            },
+            {
+                extraPath: '',
+                url: 'https://docs.mycompany.com/~/changes/id/hello',
+            },
+        ]);
+    });
+});

src/lib/middleware.ts

@@ -0,0 +1,87 @@
+/**
+ * For a given GitBook URL, return a list of alternative URLs that could be matched against to lookup the content.
+ * The approach is optimized to aim at reusing cached lookup results as much as possible.
+ */
+export function getURLLookupAlternatives(url: URL) {
+    const alternatives: Array<{ url: string; extraPath: string }> = [];
+
+    const pushAlternative = (url: URL, extraPath: string) => {
+        const existing = alternatives.find((alt) => alt.url === url.toString());
+        if (existing) {
+            if (existing.extraPath !== extraPath) {
+                throw new Error(
+                    `Invalid extraPath ${extraPath} for url ${url.toString()}, already set to ${
+                        existing.extraPath
+                    }`,
+                );
+            }
+            return;
+        }
+
+        alternatives.push({
+            url: url.toString(),
+            extraPath,
+        });
+    };
+
+    const pathSegments = url.pathname.slice(1).split('/');
+
+    // URL looks like a collection url (with /v/ in the path)
+    // We only start matching after the /v/ segment and we ignore everything before it
+    // to avoid potentially matching as a page not found under the default space in the collection
+    if (pathSegments.includes('v')) {
+        const collectionURL = new URL(url);
+        const vIndex = pathSegments.indexOf('v');
+        collectionURL.pathname = pathSegments.slice(0, vIndex + 2).join('/');
+
+        pushAlternative(collectionURL, pathSegments.slice(vIndex + 2).join('/'));
+    }
+
+    // URL looks like a specific content url (with ~/revisions/ or ~/changes/ in the path)
+    // We only start matching after the ~/revisions/ or ~/changes/ segment and we ignore everything before it
+    else if (
+        pathSegments.includes('~') &&
+        (pathSegments.includes('revisions') || pathSegments.includes('changes'))
+    ) {
+        const contentURL = new URL(url);
+        const tildeIndex = pathSegments.indexOf('~');
+        const revisionIndex = pathSegments.indexOf('revisions');
+        const changeIndex = pathSegments.indexOf('changes');
+        const revisionOrChangeIndex = Math.max(revisionIndex, changeIndex);
+
+        contentURL.pathname = pathSegments
+            .slice(0, tildeIndex + revisionOrChangeIndex + 2)
+            .join('/');
+
+        pushAlternative(
+            contentURL,
+            pathSegments.slice(tildeIndex + revisionOrChangeIndex + 2).join('/'),
+        );
+    } else {
+        // Match only with the host, if it can be a custom hostname
+        // It should cover most cases of custom domains, and with caching, it should be fast.
+        if (!url.hostname.includes('.gitbook.io')) {
+            const noPathURL = new URL(url);
+            noPathURL.pathname = '/';
+
+            pushAlternative(noPathURL, url.pathname.slice(1));
+        }
+
+        // Otherwise match with only the first segment of the path
+        // as it could potentially a space in an organization or collection domain
+        // or a space using a share link secret
+        if (pathSegments.length > 0) {
+            const shortURL = new URL(url);
+            shortURL.pathname = pathSegments[0];
+
+            pushAlternative(shortURL, pathSegments.slice(1).join('/'));
+        }
+    }
+
+    // Always try with the full URL
+    if (!alternatives.some((alt) => alt.url === url.toString())) {
+        pushAlternative(url, '');
+    }
+
+    return alternatives;
+}

src/lib/references.ts

@@ -111,5 +111,13 @@ export async function resolveContentRef(
         };
     }
 
+    if (contentRef.kind === 'snippet') {
+        return {
+            href: gitbookAppHref(`/o/${contentRef.organization}/snippet/${contentRef.snippet}`),
+            text: 'snippet',
+            active: false,
+        };
+    }
+
     assertNever(contentRef);
 }

src/lib/seo.ts

@@ -20,10 +20,17 @@ export function shouldIndexSpace({
         return false;
     }
 
+    // Prevent indexation of preview of revisions / change-requests
+    if (
+        headerSet.get('x-gitbook-content-revision') ||
+        headerSet.get('x-gitbook-content-changerequest')
+    ) {
+        return false;
+    }
+
     if (space.visibility === ContentVisibility.InCollection) {
         return collection ? shouldIndexVisibility(collection.visibility) : false;
     }
-
     return shouldIndexVisibility(space.visibility);
 }
 

src/middleware.ts

@@ -12,6 +12,8 @@ import {
 } from '@/lib/api';
 import { createContentSecurityPolicyNonce, getContentSecurityPolicy } from '@/lib/csp';
 
+import { getURLLookupAlternatives } from './lib/middleware';
+
 export const config = {
     matcher: '/((?!_next/static|_next/image|~gitbook/revalidate|~gitbook/image).*)',
     skipTrailingSlashRedirect: true,
@@ -91,7 +93,9 @@ export async function middleware(request: NextRequest) {
     }
 
     // Because of how Next will encode, we need to encode ourselves the pathname before reriting to it.
-    const rewritePathname = `/${resolved.space}${normalizePathname(encodePathname(resolved.pathname))}`;
+    const rewritePathname = `/${resolved.space}${normalizePathname(
+        encodePathname(resolved.pathname),
+    )}`;
 
     console.log(`${request.method} ${rewritePathname}`);
 
@@ -122,6 +126,13 @@ export async function middleware(request: NextRequest) {
     headers.set('x-gitbook-token', resolved.apiToken);
     headers.set('x-gitbook-origin-basepath', originBasePath);
     headers.set('x-gitbook-basepath', joinPath(originBasePath, resolved.basePath));
+    if (resolved.revision) {
+        headers.set('x-gitbook-content-revision', resolved.revision);
+    }
+    if (resolved.changeRequest) {
+        headers.set('x-gitbook-content-changerequest', resolved.changeRequest);
+    }
+
     if (apiEndpoint) {
         headers.set('x-gitbook-api', apiEndpoint);
     }
@@ -358,7 +369,7 @@ async function lookupSpaceByAPI(
     url: URL,
     visitorAuthToken: string | undefined,
 ): Promise<LookupResult | null> {
-    const lookupAlternatives = computeLookupAlternatives(stripURLSearch(url));
+    const lookupAlternatives = getURLLookupAlternatives(stripURLSearch(url));
 
     console.log(
         `lookup content for url "${url.toString()}", with ${
@@ -387,6 +398,8 @@ async function lookupSpaceByAPI(
                 abort.abort();
                 return {
                     space: data.space,
+                    changeRequest: data.changeRequest,
+                    revision: data.revision,
                     basePath: data.basePath,
                     pathname: joinPath(data.pathname, alternative.extraPath),
                     apiToken: data.apiToken,
@@ -407,68 +420,6 @@ async function lookupSpaceByAPI(
     return matches.find((match) => match !== null) ?? null;
 }
 
-function computeLookupAlternatives(url: URL) {
-    const alternatives: Array<{ url: string; extraPath: string }> = [];
-
-    const pushAlternative = (url: URL, extraPath: string) => {
-        const existing = alternatives.find((alt) => alt.url === url.toString());
-        if (existing) {
-            if (existing.extraPath !== extraPath) {
-                throw new Error(
-                    `Invalid extraPath ${extraPath} for url ${url.toString()}, already set to ${
-                        existing.extraPath
-                    }`,
-                );
-            }
-            return;
-        }
-
-        alternatives.push({
-            url: url.toString(),
-            extraPath,
-        });
-    };
-
-    // Match only with the host, if it can be a custom hostname
-    // It should cover most cases of custom domains, and with caching, it should be fast.
-    if (!url.hostname.includes('.gitbook.io')) {
-        const noPathURL = new URL(url);
-        noPathURL.pathname = '/';
-
-        pushAlternative(noPathURL, url.pathname.slice(1));
-    }
-
-    const pathSegments = url.pathname.slice(1).split('/');
-
-    // URL looks like a collection url (with /v/ in the path)
-    // We only start matching after the /v/ segment and we ignore everything before it
-    // to avoid potentially matching as a page not found under the default space in the collection
-    if (pathSegments.includes('v')) {
-        const collectionURL = new URL(url);
-        const vIndex = pathSegments.indexOf('v');
-        collectionURL.pathname = pathSegments.slice(0, vIndex + 2).join('/');
-
-        pushAlternative(collectionURL, pathSegments.slice(vIndex + 2).join('/'));
-    }
-
-    // Otherwise match with only the first segment of the path
-    // as it could potentially a space in an organization or collection domain
-    // or a space using a share link secret
-    else if (pathSegments.length > 0) {
-        const shortURL = new URL(url);
-        shortURL.pathname = pathSegments[0];
-
-        pushAlternative(shortURL, pathSegments.slice(1).join('/'));
-    }
-
-    // Always try with the full URL
-    if (!alternatives.some((alt) => alt.url === url.toString())) {
-        pushAlternative(url, '');
-    }
-
-    return alternatives;
-}
-
 function joinPath(...parts: string[]): string {
     return parts.join('/').replace(/\/+/g, '/');
 }
@@ -512,4 +463,4 @@ function stripURLSearch(url: URL): URL {
 
 function encodePathname(pathname: string): string {
     return pathname.split('/').map(encodeURIComponent).join('/');
-}
+}

tests/visual-testing.ts

@@ -58,12 +58,16 @@ const testCases: TestsCase[] = [
             },
             {
                 name: 'Encoded URL',
-                url: 'scan-using-snyk/supported-languages-and-frameworks/c-c++'
+                url: 'scan-using-snyk/supported-languages-and-frameworks/c-c++',
             },
             {
                 name: 'Redirect',
-                url: 'products/snyk-open-source/use-snyk-open-source-from-the-cli'
-            }
+                url: 'products/snyk-open-source/use-snyk-open-source-from-the-cli',
+            },
+            {
+                name: 'Revision',
+                url: '~/revisions/H41VQ6cIvd5hyUwcnwbC',
+            },
         ],
     },
     {