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
16 changes: 16 additions & 0 deletions frontend/packages/shared/src/models/__tests__/entity.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import {act} from 'react-dom/test-utils'
import {afterEach, beforeEach, describe, expect, it, vi} from 'vitest'
import {UniversalAppContext} from '../../routing'
import {hmId} from '../../utils/entity-id-url'
import {queryKeys} from '../query-keys'
import {useResource} from '../entity'

const docA = hmId('uid1', {path: ['old-name']})
Expand Down Expand Up @@ -105,6 +106,7 @@ function renderUseResource(params: {
container.remove()
},
client,
queryClient,
}
}

Expand All @@ -131,6 +133,20 @@ describe('useResource redirects', () => {
vi.clearAllMocks()
})

it('caches resolved redirected resources under source and target query keys', async () => {
const onRedirectOrDeleted = vi.fn()
const rendered = renderUseResource({id: docA, onRedirectOrDeleted})

await waitForCondition(() => onRedirectOrDeleted.mock.calls.length === 1)

const oldKey = [queryKeys.ENTITY, docA.id, docA.version || undefined, docA.latest || false]
const newKey = [queryKeys.ENTITY, docB.id, docB.version || undefined, docB.latest || false]
expect(rendered.queryClient.getQueryData(oldKey)).toMatchObject({type: 'document', id: docB})
expect(rendered.queryClient.getQueryData(newKey)).toMatchObject({type: 'document', id: docB})

rendered.cleanup()
})

it('dispatches the same redirect again after navigating away and back', async () => {
const onRedirectOrDeleted = vi.fn()
const rendered = renderUseResource({id: docA, onRedirectOrDeleted})
Expand Down
32 changes: 24 additions & 8 deletions frontend/packages/shared/src/models/__tests__/queries.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -142,8 +142,7 @@ describe('queryResource', () => {
expect(result).toMatchObject({type: 'tombstone', id: docB})
})

test('stops following redirects after max depth (5)', async () => {
// Create a chain of 6 redirects — should stop after 5
test('returns an error after redirect max depth instead of leaking a redirect resource', async () => {
const ids = Array.from({length: 7}, (_, i) => hmId('uid1', {path: [`doc-${i}`]}))
const client = createMockClient((_key, input) => {
const idx = ids.findIndex((id) => id.id === input.id)
Expand All @@ -152,15 +151,32 @@ describe('queryResource', () => {
}
return documentResponse(ids[idx]!)
})
const query = queryResource(client, ids[0]!)
const result = await query.queryFn!()
// After 5 redirects, we're at ids[5] which still redirects to ids[6],
// but we've hit the limit. The result is the redirect response itself.
expect(result).toMatchObject({type: 'redirect'})
// 1 initial + 5 follows = 6 total requests

const result = await queryResource(client, ids[0]!).queryFn!()

expect(result).toMatchObject({
type: 'error',
id: ids[0],
message: 'Too many redirects while resolving resource',
})
expect(client.request).toHaveBeenCalledTimes(6)
})

test('does not copy source version onto redirect target', async () => {
const versionedDocA = hmId('uid1', {path: ['old-name'], version: 'v123'})
const client = createMockClient((_key, input) => {
if (input.id === versionedDocA.id) return redirectResponse(versionedDocA, docB)
if (input.id === docB.id) return documentResponse(docB)
throw new Error(`Unexpected request: ${input.id}`)
})

const result = await queryResource(client, versionedDocA).queryFn!()

expect(result).toMatchObject({type: 'document', id: docB})
expect(result?.id.version).toBeNull()
expect(client.request).toHaveBeenNthCalledWith(2, 'Resource', docB, {signal: undefined})
})

test('returns null for null id', async () => {
const client = createMockClient(() => {
throw new Error('Should not be called')
Expand Down
3 changes: 3 additions & 0 deletions frontend/packages/shared/src/models/queries.ts
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,9 @@ export function queryResource(client: UniversalClient, id: UnpackedHypermediaId
}
res = await client.request('Resource', nextTarget, {signal})
}
if (res?.type === 'redirect') {
return {type: 'error', id, message: 'Too many redirects while resolving resource'}
}
const parsed = HMResourceSchema.parse(res)
if (republishSourceId && (parsed.type === 'document' || parsed.type === 'comment') && !id.hostname) {
return {
Expand Down
30 changes: 30 additions & 0 deletions frontend/packages/ui/src/__tests__/resource-page-common.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import {
getCommentReplyPanelRoute,
hasUnpublishedDraftForResourceState,
shouldSuppressMainCommentEditor,
getRenderedDocumentId,
shouldUseDraftForRenderedDocument,
} from '../resource-page-common'

Expand Down Expand Up @@ -189,3 +190,32 @@ describe('hasUnpublishedDraftForResourceState', () => {
).toBe(false)
})
})

describe('getRenderedDocumentId', () => {
const oldId = hmId('uid1', {path: ['old-name']})
const newId = hmId('uid1', {path: ['new-name']})
const redirectedDocument = {
type: 'document' as const,
id: newId,
document: {
version: 'v1',
account: 'uid1',
path: '/new-name',
authors: [],
content: [],
metadata: {},
genesis: 'genesis1',
visibility: 'PUBLIC' as const,
createTime: '',
updateTime: '',
},
}

it('uses the resolved document id when a redirect returned a different document', () => {
expect(getRenderedDocumentId(oldId, redirectedDocument)).toEqual(newId)
})

it('keeps the route document id when the resource is not a document', () => {
expect(getRenderedDocumentId(oldId, {type: 'not-found', id: oldId})).toEqual(oldId)
})
})
27 changes: 17 additions & 10 deletions frontend/packages/ui/src/resource-page-common.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import {
HMComment,
HMDocument,
HMExistingDraft,
HMResource,
UnpackedHypermediaId,
} from '@seed-hypermedia/client/hm-types'
import {
Expand Down Expand Up @@ -134,6 +135,11 @@ type CommentDraftTarget = {
quotingRangeEnd?: number
}

/** Returns the document ID that should back the rendered document content. */
export function getRenderedDocumentId(routeDocId: UnpackedHypermediaId, resourceData: HMResource | null | undefined) {
return resourceData?.type === 'document' ? resourceData.id : routeDocId
}

function extractQuotingRange(blockRange?: BlockRange | null): {start: number; end: number} | undefined {
if (!blockRange) return undefined
if (
Expand Down Expand Up @@ -704,7 +710,8 @@ export function ResourcePage({
// exists, fabricate a placeholder so DocumentBody / the document machine
// can transition to "loaded" → editing for the new-document case.
let document: HMDocument
if (resourceFetchId && resource.data?.type === 'document' && resource.data.id.id === resourceFetchId.id) {
const renderedDocId = getRenderedDocumentId(docId, resource.data)
if (resourceFetchId && resource.data?.type === 'document') {
document = resource.data.document
} else if (lastGoodDocumentRef.current) {
// Transient refetch failure / not-found / discovery flap — keep showing the last
Expand Down Expand Up @@ -732,9 +739,9 @@ export function ResourcePage({
)
}

const shouldUseDraft = shouldUseDraftForRenderedDocument({docId, existingDraft, isLatest})
const shouldUseDraft = shouldUseDraftForRenderedDocument({docId: renderedDocId, existingDraft, isLatest})
const effectiveCanEdit =
(canEdit || (resourceFetchId === null && !!existingDraft)) && (!docId.version || isLatest || shouldUseDraft)
(canEdit || (resourceFetchId === null && !!existingDraft)) && (!renderedDocId.version || isLatest || shouldUseDraft)
const effectiveExistingDraft = shouldUseDraft ? existingDraft : false
const effectiveExistingDraftVisibility = shouldUseDraft ? existingDraftVisibility : undefined
const effectiveExistingDraftContent = shouldUseDraft ? existingDraftContent : undefined
Expand All @@ -744,7 +751,7 @@ export function ResourcePage({
const effectiveExistingDraftDeps = shouldUseDraft ? existingDraftDeps : undefined
const draftVersionEntry = existingDraft
? {
docId,
docId: renderedDocId,
draftId: existingDraft.id,
deps: existingDraftDeps,
metadata: existingDraft.metadata,
Expand All @@ -758,15 +765,15 @@ export function ResourcePage({
// path changes (e.g. after first publish from `-${draftId}` → real slug).
// Without this, useActorRef keeps the original actor instance and its
// context still references the old documentId/editPath.
key={`${docId.id}@${docId.version ?? 'latest'}`}
key={`${renderedDocId.id}@${renderedDocId.version ?? 'latest'}`}
input={{
documentId: docId,
documentId: renderedDocId,
canEdit: effectiveCanEdit,
isLatest,
deps: effectiveExistingDraftDeps,
reservedDraftId: reservedDraftId ?? undefined,
editUid: docId.uid,
editPath: docId.path ?? undefined,
editUid: renderedDocId.uid,
editPath: renderedDocId.path ?? undefined,
signingAccountId,
publishAccountUid,
}}
Expand All @@ -775,15 +782,15 @@ export function ResourcePage({
>
<PageWrapper
siteHomeId={siteHomeId}
docId={docId}
docId={renderedDocId}
headerData={headerData}
document={document}
rightActions={rightActions}
editNavPane={editNavPane}
transientResourceError={transientResourceError}
>
<DocumentBody
docId={docId}
docId={renderedDocId}
document={document}
activeView={getActiveView(route.key)}
isLatest={isLatest}
Expand Down
Loading