fix(frontend): wait for redirect route replacement (#771)

* 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.

* fix(frontend): render redirected document ids immediately

Author: horacioh

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})

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')

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 {

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)
+  })
+})

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,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}