Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Pass site url as context to listSiteSpaces API #2339

Merged
merged 12 commits into from
Jun 14, 2024
17 changes: 13 additions & 4 deletions src/app/(space)/fetch.ts
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ export function getContentPointer(): ContentPointer | SiteContentPointer {
if (siteId) {
const organizationId = headerSet.get('x-gitbook-content-organization');
const siteSpaceId = headerSet.get('x-gitbook-content-site-space');
const siteUrl = headerSet.get('x-gitbook-content-url');
if (!organizationId) {
throw new Error('Missing site content headers');
}
Expand All @@ -48,6 +49,7 @@ export function getContentPointer(): ContentPointer | SiteContentPointer {
siteId,
spaceId,
siteSpaceId: siteSpaceId ?? undefined,
siteUrl: siteUrl ?? undefined,
organizationId,
revisionId: headerSet.get('x-gitbook-content-revision') ?? undefined,
changeRequestId: headerSet.get('x-gitbook-content-changerequest') ?? undefined,
Expand All @@ -71,7 +73,10 @@ export async function fetchSpaceData() {

const [{ space, contentTarget, pages, customization, scripts }, parentSite] = await Promise.all(
'siteId' in content
? [getCurrentSiteData(content), fetchParentSite(content.organizationId, content.siteId)]
? [
getCurrentSiteData(content),
fetchParentSite(content.organizationId, content.siteId, content.siteUrl),
]
: [getSpaceData(content)],
);

Expand Down Expand Up @@ -102,7 +107,7 @@ export async function fetchPageData(params: PagePathParams | PageIdParams) {
const page = await resolvePage(contentTarget, pages, params);
const [parent, document] = await Promise.all([
'siteId' in content
? fetchParentSite(content.organizationId, content.siteId)
? fetchParentSite(content.organizationId, content.siteId, content.siteUrl)
: fetchParentCollection(space),
page?.page.documentId ? getDocument(space.id, page.page.documentId) : null,
]);
Expand Down Expand Up @@ -173,10 +178,14 @@ async function fetchParentCollection(space: Space) {
return { parent: collection, spaces };
}

async function fetchParentSite(organizationId: string, siteId: string) {
async function fetchParentSite(
organizationId: string,
siteId: string,
siteUrl: string | undefined,
) {
const [site, siteSpaces] = await Promise.all([
getSite(organizationId, siteId),
getSiteSpaces(organizationId, siteId),
getSiteSpaces(organizationId, siteId, siteUrl ?? null),
]);

const spaces: Record<string, Space> = {};
Expand Down
2 changes: 1 addition & 1 deletion src/components/Search/server-actions.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,7 @@ export async function searchParentContent(
api.searchParentContent(parent.id, query),
parent.object === 'collection' ? api.getCollectionSpaces(parent.id) : null,
parent.object === 'site' && 'organizationId' in pointer
? api.getSiteSpaces(pointer.organizationId, parent.id)
? api.getSiteSpaces(pointer.organizationId, parent.id, pointer.siteUrl ?? null)
: null,
]);

Expand Down
34 changes: 29 additions & 5 deletions src/lib/api.ts
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,11 @@ export interface SiteContentPointer extends ContentPointer {
* ID of the siteSpace can be undefined when rendering in multi-id mode (for site previews)
*/
siteSpaceId: string | undefined;
/**
* URL of the site content that was used for lookup. Only set for `multi` and `multi-path` modes
* where an URL is involved in the lookup/resolution
*/
siteUrl: string | undefined;
}

/**
Expand Down Expand Up @@ -126,6 +131,8 @@ export type PublishedContentWithCache =
| ((PublishedContentLookup | PublishedSiteContentLookup) & {
cacheMaxAge?: number;
cacheTags?: string[];
/** Published content URL that was used for lookup */
contentUrl?: string;
})
| {
error: {
Expand Down Expand Up @@ -235,6 +242,7 @@ export const getPublishedContentByUrl = cache(
...response.data,
cacheMaxAge: parsed.ttl,
cacheTags: parsed.tags,
contentUrl: url,
};
return {
tags: [
Expand Down Expand Up @@ -736,12 +744,28 @@ export const getSite = cache(
*/
export const getSiteSpaces = cache(
'api.getSiteSpaces',
async (organizationId: string, siteId: string, options: CacheFunctionOptions) => {
async (
organizationId: string,
siteId: string,
/**
* Additional site URL that can be used as context to resolve site space published urls
*/
siteUrlContext: string | null,
Copy link
Contributor

Choose a reason for hiding this comment

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

By passing as an arg here it means it'll form part of the cache key so multiple requests with the same orgId/siteId will not be cached:

getSiteSpaces(':orgId', ':siteId', 'https://docs.gitbook.com') => CACHE MISS
getSiteSpaces(':orgId', ':siteId', 'https://docs.gitbook.com/getting-started') => CACHE MISS, even though data is the same

I'm not sure the best solution for it. If we don't include it in the cache key it could mean caching the same content between different share links, but including it will mean effectively no cache.

Copy link
Contributor

Choose a reason for hiding this comment

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

It's not good. We have to fix that.

options: CacheFunctionOptions,
) => {
const response = await getAll((params) =>
api().orgs.listSiteSpaces(organizationId, siteId, params, {
...noCacheFetchOptions,
signal: options.signal,
}),
api().orgs.listSiteSpaces(
organizationId,
siteId,
{
...params,
context: siteUrlContext ?? undefined,
},
{
...noCacheFetchOptions,
signal: options.signal,
},
),
);

return cacheResponse(response, {
Expand Down
4 changes: 4 additions & 0 deletions src/middleware.ts
Original file line number Diff line number Diff line change
Expand Up @@ -209,6 +209,9 @@ export async function middleware(request: NextRequest) {
headers.set('x-gitbook-content-site-space', resolved.siteSpace);
}
}
if (resolved.contentUrl) {
headers.set('x-gitbook-content-url', resolved.contentUrl);
}
if (resolved.revision) {
headers.set('x-gitbook-content-revision', resolved.revision);
}
Expand Down Expand Up @@ -659,6 +662,7 @@ async function lookupSpaceByAPI(
apiToken: data.apiToken,
cacheMaxAge: data.cacheMaxAge,
cacheTags: data.cacheTags,
contentUrl: data.contentUrl,
...('site' in data
? { site: data.site, siteSpace: data.siteSpace, organization: data.organization }
: {}),
Expand Down
Loading