fix(js): keep attachment urls and repoint images across http restarts

shared/chat/conversation/thread-message-state.test.tsx

@@ -673,3 +673,52 @@ describe('addMessagesToThreadState', () => {
     expect(merged?.type === 'text' && merged.text.stringValue()).toBe('edited')
   })
 })
+
+// The http server can be stopped (BACKGROUND) or mid-restart (a fresh port/token) when an update
+// carrying a stale GetURL lands: that comes back as '' or as a base-less URL with only query
+// params appended (e.g. "&prev=false&noanim=true"), never as a garbage http:// value. A url
+// field only ever takes an incoming value that actually looks like one.
+describe('local server urls', () => {
+  const attachmentOrdinal = T.Chat.numberToOrdinal(201)
+  const validUrl = 'http://127.0.0.1:1234/at?key=abc'
+  const garbageUrl = '&prev=false&noanim=true'
+
+  test('a new non-empty url replaces the old one', () => {
+    const state = makeThreadState([])
+    addMessagesToThreadState(state, [makeAttachmentMessage({fileURL: 'http://127.0.0.1:5000/f'})], {})
+    addMessagesToThreadState(state, [makeAttachmentMessage({fileURL: 'http://127.0.0.1:6000/f'})], {})
+    expect((state.messageMap.get(attachmentOrdinal) as T.Chat.MessageAttachment).fileURL).toBe(
+      'http://127.0.0.1:6000/f'
+    )
+  })
+
+  test.each([
+    {
+      field: 'fileURL',
+      make: (url: string) => makeAttachmentMessage({fileURL: url}),
+      read: (state: WritableConversationThreadMessageState) =>
+        (state.messageMap.get(attachmentOrdinal) as T.Chat.MessageAttachment).fileURL,
+    },
+    {
+      field: 'previewURL',
+      make: (url: string) => makeAttachmentMessage({previewURL: url}),
+      read: (state: WritableConversationThreadMessageState) =>
+        (state.messageMap.get(attachmentOrdinal) as T.Chat.MessageAttachment).previewURL,
+    },
+  ])('$field: empty or garbage keeps the existing value, a real url replaces it', ({make, read}) => {
+    const empty = makeThreadState([])
+    addMessagesToThreadState(empty, [make(validUrl)], {})
+    addMessagesToThreadState(empty, [make('')], {})
+    expect(read(empty)).toBe(validUrl)
+
+    const garbage = makeThreadState([])
+    addMessagesToThreadState(garbage, [make(validUrl)], {})
+    addMessagesToThreadState(garbage, [make(garbageUrl)], {})
+    expect(read(garbage)).toBe(validUrl)
+
+    const replaced = makeThreadState([])
+    addMessagesToThreadState(replaced, [make('http://127.0.0.1:5000/f')], {})
+    addMessagesToThreadState(replaced, [make(validUrl)], {})
+    expect(read(replaced)).toBe(validUrl)
+  })
+})

shared/chat/conversation/thread-message-state.tsx

@@ -143,6 +143,13 @@ const maybeGetOrdinalByMessageID = (
 ) =>
   getOrdinalForMessageID(state.messageMap, state.pendingOutboxToOrdinal, messageID, state.messageIDToOrdinal)
 
+// The service's local http server can be stopped or mid-restart when an update carrying a
+// GetURL-derived value lands: that value can come back as '' or as a base-less URL with only
+// query params appended, never as a garbage http:// value. Keep whatever was already rendering
+// until a real replacement arrives.
+const keepUrl = (next: string | undefined, prev: string | undefined) =>
+  next?.startsWith('http://') ? next : prev ?? next
+
 const mergeMessage = (
   existing: WritableDraft<T.Chat.Message>,
   incoming: WritableDraft<T.Chat.Message>
@@ -170,6 +177,11 @@ const mergeMessage = (
       } else {
         existingRecord[key] = val
       }
+    } else if (key === 'fileURL' || key === 'previewURL') {
+      const next = keepUrl(val as string | undefined, cur as string | undefined)
+      if (cur !== next) {
+        existingRecord[key] = next
+      }
     } else if (cur !== val) {
       existingRecord[key] = val
     }

shared/common-adapters/image.tsx

@@ -3,6 +3,7 @@ import * as Styles from '@/styles'
 import type {ImageLoadEventData, ImageErrorEventData} from 'expo-image'
 import {Image as ExpoImage} from 'expo-image'
 import LoadingStateView from './loading-state-view'
+import {isLocalhostSrc, retryLocalhostSrc} from './localhost-src'
 import type {StylesCrossPlatform} from '@/styles'
 import {useConfigState} from '@/stores/config'
 import {useShellState} from '@/stores/shell'
@@ -52,16 +53,14 @@ const DesktopImage = (p: Props) => {
 // on background/inactive and restarts it (new token, possibly new port) on foreground, so a
 // load racing the restart gets connection refused. Those are worth retrying; remote srcs keep
 // the old fail-once behavior.
-const isLocalhostSrc = (src: Props['src']): src is string =>
-  typeof src === 'string' && src.startsWith('http://127.0.0.1:')
-
 const maxRetries = 3
 
 const NativeImage = (p: Props) => {
   const {showLoadingStateUntilLoaded, src, onLoad, onError, style, contentFit = 'contain', allowDownscaling} = p
   const [loading, setLoading] = React.useState(!showLoadingStateUntilLoaded)
   const [lastSrc, setLastSrc] = React.useState(src)
-  const [attempt, setAttempt] = React.useState(0)
+  // the retried src is resolved against the http server when the retry is scheduled
+  const [retry, setRetry] = React.useState<{attempt: number; src?: string}>({attempt: 0})
   const retryable = isLocalhostSrc(src)
   const failedRef = React.useRef(false)
   const triesRef = React.useRef(0)
@@ -76,8 +75,17 @@ const NativeImage = (p: Props) => {
   if (lastSrc !== src) {
     setLastSrc(src)
     setLoading(true)
-    setAttempt(0)
+    setRetry({attempt: 0})
+  }
+
+  const scheduleRetry = () => {
+    if (!isLocalhostSrc(src)) return
+    setRetry(r => ({
+      attempt: r.attempt + 1,
+      src: retryLocalhostSrc(src, r.attempt + 1, useConfigState.getState().httpSrv),
+    }))
   }
+  const scheduleRetryEvent = React.useEffectEvent(scheduleRetry)
 
   React.useEffect(() => {
     triesRef.current = 0
@@ -90,9 +98,7 @@ const NativeImage = (p: Props) => {
     if (retryable && triesRef.current < maxRetries) {
       triesRef.current++
       clearTimeout(timerRef.current)
-      timerRef.current = setTimeout(() => {
-        setAttempt(a => a + 1)
-      }, 1000 * 2 ** (triesRef.current - 1))
+      timerRef.current = setTimeout(scheduleRetry, 1000 * 2 ** (triesRef.current - 1))
       return
     }
     setLoading(false)
@@ -111,7 +117,7 @@ const NativeImage = (p: Props) => {
       failedRef.current = false
       triesRef.current = 0
       setLoading(true)
-      setAttempt(a => a + 1)
+      scheduleRetryEvent()
     }
     const unsubConfig = useConfigState.subscribe((s, prev) => {
       if (s.httpSrv.address !== prev.httpSrv.address || s.httpSrv.token !== prev.httpSrv.token) {
@@ -130,9 +136,8 @@ const NativeImage = (p: Props) => {
     }
   }, [retryable])
 
-  // cache-buster forces expo-image to actually refetch; recyclingKey stays on the original
-  // src so the view isn't blanked by retries
-  const srcToUse = retryable && attempt > 0 ? `${src}${src.includes('?') ? '&' : '?'}kbRetry=${attempt}` : src
+  // recyclingKey stays on the original src so the view isn't blanked by retries
+  const srcToUse = retry.src ?? src
   const recyclingKey = typeof src === 'string' ? src : Array.isArray(src) ? src[0]?.uri : String(src)
 
   return (

shared/common-adapters/localhost-src.test.ts

@@ -0,0 +1,44 @@
+/// <reference types="jest" />
+import {isLocalhostSrc, retryLocalhostSrc} from './localhost-src'
+
+const httpSrv = {address: '127.0.0.1:61234', token: 'newtoken'}
+
+test('only local service srcs are retryable', () => {
+  expect(isLocalhostSrc('http://127.0.0.1:5000/av?name=testuser')).toBe(true)
+  expect(isLocalhostSrc('https://keybase.io/images/testuser.png')).toBe(false)
+  expect(isLocalhostSrc(3)).toBe(false)
+})
+
+test('a retry points a baked attachment url at the current server port', () => {
+  const src = 'http://127.0.0.1:5000/at?key=abc&prev=true&noanim=false&isemoji=false'
+  expect(retryLocalhostSrc(src, 1, httpSrv)).toBe(
+    'http://127.0.0.1:61234/at?key=abc&prev=true&noanim=false&isemoji=false&kbRetry=1'
+  )
+})
+
+test('a retry replaces the token param when there is one', () => {
+  const src = 'http://127.0.0.1:5000/av?typ=user&name=testuser&token=oldtoken&count=0'
+  expect(retryLocalhostSrc(src, 2, httpSrv)).toBe(
+    'http://127.0.0.1:61234/av?typ=user&name=testuser&token=newtoken&count=0&kbRetry=2'
+  )
+})
+
+test('a service restart on the same port still carries the new token', () => {
+  const src = 'http://127.0.0.1:61234/av?typ=user&name=testuser&token=oldtoken&count=0'
+  expect(retryLocalhostSrc(src, 1, {address: '127.0.0.1:61234', token: 'newtoken'})).toBe(
+    'http://127.0.0.1:61234/av?typ=user&name=testuser&token=newtoken&count=0&kbRetry=1'
+  )
+})
+
+test('a retry keeps the baked address when the current one is unknown', () => {
+  const src = 'http://127.0.0.1:5000/at?key=abc'
+  expect(retryLocalhostSrc(src, 1, {address: '', token: ''})).toBe('http://127.0.0.1:5000/at?key=abc&kbRetry=1')
+})
+
+test('a kbfs src keeps its own server and token', () => {
+  // kbfs runs a second local server on its own port with its own token
+  const src = 'http://127.0.0.1:7000/files/private/testuser/cat.png?token=kbfstoken&viewTypeInvariance=1'
+  expect(retryLocalhostSrc(src, 1, httpSrv)).toBe(
+    'http://127.0.0.1:7000/files/private/testuser/cat.png?token=kbfstoken&viewTypeInvariance=1&kbRetry=1'
+  )
+})

shared/common-adapters/localhost-src.tsx

@@ -0,0 +1,32 @@
+const localhostPrefix = /^http:\/\/127\.0\.0\.1:\d+/
+
+// The service's own endpoints: "at" (go/chat/attachment_httpsrv.go), "av" (go/avatars/srv.go) and
+// "map" (go/chat/maps/srv.go). KBFS serves /files/ from a second local server with its own port and
+// its own token, so a src has to be matched against these before anything is repointed.
+const serviceSrc = /^http:\/\/127\.0\.0\.1:\d+\/(?:at|av|map)(?=[?#]|$)/
+
+export const isLocalhostSrc = (src: unknown): src is string =>
+  typeof src === 'string' && localhostPrefix.test(src)
+
+// The service can restart its http server on a new port, but chat bakes the address into
+// attachment and emoji URLs, so a retry points the src at wherever the server is now. A service
+// process restart draws a fresh port and mints a new per-process token (see
+// go/kbhttp/manager/manager.go), and a reconnect alone doesn't refetch already-rendered thread
+// data, so the token also needs rewriting or a stale token= keeps failing forever. The
+// cache-buster forces expo-image to actually refetch, and is all a non-service src gets.
+export const retryLocalhostSrc = (
+  src: string,
+  attempt: number,
+  httpSrv: {address: string; token: string}
+) => {
+  let next = src
+  if (serviceSrc.test(src)) {
+    if (httpSrv.address) {
+      next = next.replace(localhostPrefix, `http://${httpSrv.address}`)
+    }
+    if (httpSrv.token) {
+      next = next.replace(/([?&]token=)[^&#]*/, `$1${httpSrv.token}`)
+    }
+  }
+  return `${next}${next.includes('?') ? '&' : '?'}kbRetry=${attempt}`
+}