diff --git a/frontend/packages/shared/src/models/__tests__/entity.test.tsx b/frontend/packages/shared/src/models/__tests__/entity.test.tsx index 4c5faf069..757e1ba3b 100644 --- a/frontend/packages/shared/src/models/__tests__/entity.test.tsx +++ b/frontend/packages/shared/src/models/__tests__/entity.test.tsx @@ -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']}) @@ -105,6 +106,7 @@ function renderUseResource(params: { container.remove() }, client, + queryClient, } } @@ -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}) diff --git a/frontend/packages/shared/src/models/__tests__/queries.test.ts b/frontend/packages/shared/src/models/__tests__/queries.test.ts index 282344995..939a12a32 100644 --- a/frontend/packages/shared/src/models/__tests__/queries.test.ts +++ b/frontend/packages/shared/src/models/__tests__/queries.test.ts @@ -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) @@ -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') diff --git a/frontend/packages/shared/src/models/queries.ts b/frontend/packages/shared/src/models/queries.ts index 65d5fbf64..122d33b6a 100644 --- a/frontend/packages/shared/src/models/queries.ts +++ b/frontend/packages/shared/src/models/queries.ts @@ -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 { diff --git a/frontend/packages/ui/src/__tests__/resource-page-common.test.ts b/frontend/packages/ui/src/__tests__/resource-page-common.test.ts index f7937d236..697408bfe 100644 --- a/frontend/packages/ui/src/__tests__/resource-page-common.test.ts +++ b/frontend/packages/ui/src/__tests__/resource-page-common.test.ts @@ -4,6 +4,7 @@ import { getCommentReplyPanelRoute, hasUnpublishedDraftForResourceState, shouldSuppressMainCommentEditor, + getRenderedDocumentId, shouldUseDraftForRenderedDocument, } from '../resource-page-common' @@ -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) + }) +}) diff --git a/frontend/packages/ui/src/resource-page-common.tsx b/frontend/packages/ui/src/resource-page-common.tsx index 808209aef..a3253455a 100644 --- a/frontend/packages/ui/src/resource-page-common.tsx +++ b/frontend/packages/ui/src/resource-page-common.tsx @@ -4,6 +4,7 @@ import { HMComment, HMDocument, HMExistingDraft, + HMResource, UnpackedHypermediaId, } from '@seed-hypermedia/client/hm-types' import { @@ -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 ( @@ -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 @@ -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 @@ -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, @@ -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, }} @@ -775,7 +782,7 @@ export function ResourcePage({ >