From 3ee8f64e39332488d376f2bb3d7f1db2e55d267f Mon Sep 17 00:00:00 2001 From: Horacio Herrera Date: Fri, 12 Jun 2026 16:17:35 +0200 Subject: [PATCH 1/2] fix(frontend): wait for redirect route replacement Return an error when resource redirects exceed the max depth instead of leaking a redirect resource, avoid carrying source versions onto redirect targets, and keep redirected resource data cached under both source and target query keys. --- .../src/models/__tests__/entity.test.tsx | 16 +++++++++ .../src/models/__tests__/queries.test.ts | 32 ++++++++++++----- .../packages/shared/src/models/queries.ts | 3 ++ .../__tests__/resource-page-common.test.ts | 34 +++++++++++++++++++ .../packages/ui/src/resource-page-common.tsx | 21 ++++++++++++ 5 files changed, 98 insertions(+), 8 deletions(-) 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..d16100c50 100644 --- a/frontend/packages/ui/src/__tests__/resource-page-common.test.ts +++ b/frontend/packages/ui/src/__tests__/resource-page-common.test.ts @@ -5,6 +5,7 @@ import { hasUnpublishedDraftForResourceState, shouldSuppressMainCommentEditor, shouldUseDraftForRenderedDocument, + shouldWaitForRedirectRouteReplace, } from '../resource-page-common' describe('shouldSuppressMainCommentEditor', () => { @@ -189,3 +190,36 @@ describe('hasUnpublishedDraftForResourceState', () => { ).toBe(false) }) }) + +describe('shouldWaitForRedirectRouteReplace', () => { + 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('waits when the fetched document resolved to a different id before any document loaded', () => { + expect(shouldWaitForRedirectRouteReplace(oldId, redirectedDocument, false)).toBe(true) + }) + + it('does not wait when the resolved document matches the requested id', () => { + expect(shouldWaitForRedirectRouteReplace(newId, redirectedDocument, false)).toBe(false) + }) + + it('does not wait after a document has already loaded', () => { + expect(shouldWaitForRedirectRouteReplace(oldId, redirectedDocument, true)).toBe(false) + }) +}) diff --git a/frontend/packages/ui/src/resource-page-common.tsx b/frontend/packages/ui/src/resource-page-common.tsx index 808209aef..a69ff8aeb 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,15 @@ type CommentDraftTarget = { quotingRangeEnd?: number } +/** Returns true while a redirected document is waiting for the route to be replaced. */ +export function shouldWaitForRedirectRouteReplace( + requestedId: UnpackedHypermediaId | null | undefined, + resourceData: HMResource | null | undefined, + hasEverLoaded: boolean, +): boolean { + return !!requestedId && !hasEverLoaded && resourceData?.type === 'document' && resourceData.id.id !== requestedId.id +} + function extractQuotingRange(blockRange?: BlockRange | null): {start: number; end: number} | undefined { if (!blockRange) return undefined if ( @@ -598,6 +608,17 @@ export function ResourcePage({ ) } + if (shouldWaitForRedirectRouteReplace(resourceFetchId, resource.data, hasEverLoadedRef.current)) { + return ( + +
+ +
+ {pageFooter} + + ) + } + // Handle not-found if ((!resource.data || resource.data.type === 'not-found') && !hasUnpublishedDraft && !hasEverLoadedRef.current) { return ( From 422d1e8a6dc7f6f2fac16b08289c8554daf94be3 Mon Sep 17 00:00:00 2001 From: Horacio Herrera Date: Fri, 12 Jun 2026 17:24:27 +0200 Subject: [PATCH 2/2] fix(frontend): render redirected document ids immediately --- .../__tests__/resource-page-common.test.ts | 16 +++---- .../packages/ui/src/resource-page-common.tsx | 42 +++++++------------ 2 files changed, 20 insertions(+), 38 deletions(-) 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 d16100c50..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,8 +4,8 @@ import { getCommentReplyPanelRoute, hasUnpublishedDraftForResourceState, shouldSuppressMainCommentEditor, + getRenderedDocumentId, shouldUseDraftForRenderedDocument, - shouldWaitForRedirectRouteReplace, } from '../resource-page-common' describe('shouldSuppressMainCommentEditor', () => { @@ -191,7 +191,7 @@ describe('hasUnpublishedDraftForResourceState', () => { }) }) -describe('shouldWaitForRedirectRouteReplace', () => { +describe('getRenderedDocumentId', () => { const oldId = hmId('uid1', {path: ['old-name']}) const newId = hmId('uid1', {path: ['new-name']}) const redirectedDocument = { @@ -211,15 +211,11 @@ describe('shouldWaitForRedirectRouteReplace', () => { }, } - it('waits when the fetched document resolved to a different id before any document loaded', () => { - expect(shouldWaitForRedirectRouteReplace(oldId, redirectedDocument, false)).toBe(true) + it('uses the resolved document id when a redirect returned a different document', () => { + expect(getRenderedDocumentId(oldId, redirectedDocument)).toEqual(newId) }) - it('does not wait when the resolved document matches the requested id', () => { - expect(shouldWaitForRedirectRouteReplace(newId, redirectedDocument, false)).toBe(false) - }) - - it('does not wait after a document has already loaded', () => { - expect(shouldWaitForRedirectRouteReplace(oldId, redirectedDocument, true)).toBe(false) + 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 a69ff8aeb..a3253455a 100644 --- a/frontend/packages/ui/src/resource-page-common.tsx +++ b/frontend/packages/ui/src/resource-page-common.tsx @@ -135,13 +135,9 @@ type CommentDraftTarget = { quotingRangeEnd?: number } -/** Returns true while a redirected document is waiting for the route to be replaced. */ -export function shouldWaitForRedirectRouteReplace( - requestedId: UnpackedHypermediaId | null | undefined, - resourceData: HMResource | null | undefined, - hasEverLoaded: boolean, -): boolean { - return !!requestedId && !hasEverLoaded && resourceData?.type === 'document' && resourceData.id.id !== requestedId.id +/** 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 { @@ -608,17 +604,6 @@ export function ResourcePage({ ) } - if (shouldWaitForRedirectRouteReplace(resourceFetchId, resource.data, hasEverLoadedRef.current)) { - return ( - -
- -
- {pageFooter} - - ) - } - // Handle not-found if ((!resource.data || resource.data.type === 'not-found') && !hasUnpublishedDraft && !hasEverLoadedRef.current) { return ( @@ -725,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 @@ -753,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 @@ -765,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, @@ -779,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, }} @@ -796,7 +782,7 @@ export function ResourcePage({ >