From 0e7a75dfdbc062eb7e418cfe325f84b65b891fcd Mon Sep 17 00:00:00 2001 From: chrisnojima Date: Wed, 23 Sep 2026 12:47:43 -0400 Subject: [PATCH 1/4] fix(js): keep attachment urls and repoint retried images while the http server restarts --- .../thread-message-state.test.tsx | 57 +++++++++++++++++++ .../conversation/thread-message-state.tsx | 30 +++++++++- shared/common-adapters/image.tsx | 10 ++-- shared/common-adapters/localhost-src.test.ts | 44 ++++++++++++++ shared/common-adapters/localhost-src.tsx | 32 +++++++++++ 5 files changed, 165 insertions(+), 8 deletions(-) create mode 100644 shared/common-adapters/localhost-src.test.ts create mode 100644 shared/common-adapters/localhost-src.tsx diff --git a/shared/chat/conversation/thread-message-state.test.tsx b/shared/chat/conversation/thread-message-state.test.tsx index 8ba2bfb62629..5ffcf90b4bbd 100644 --- a/shared/chat/conversation/thread-message-state.test.tsx +++ b/shared/chat/conversation/thread-message-state.test.tsx @@ -673,3 +673,60 @@ 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: on master 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 reactionOrdinal = ordinal + 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: 'emoji src', + make: (url: string) => + makeTextMessage({reactions: new Map([[':party:', {decorated: url, users: [{timestamp: 1, username: 'testuser'}]}]])}), + read: (state: WritableConversationThreadMessageState) => + (state.messageMap.get(reactionOrdinal) as T.Chat.MessageText).reactions?.get(':party:')?.decorated, + }, + ])('$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) + }) +}) diff --git a/shared/chat/conversation/thread-message-state.tsx b/shared/chat/conversation/thread-message-state.tsx index a2c575bb253b..dec6da309890 100644 --- a/shared/chat/conversation/thread-message-state.tsx +++ b/shared/chat/conversation/thread-message-state.tsx @@ -143,6 +143,23 @@ 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: on master 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 mergeReactions = ( + cur: Map, + val: Map +) => { + for (const [emoji, incoming] of val) { + const existing = cur.get(emoji) + cur.set(emoji, {...incoming, decorated: keepUrl(incoming.decorated, existing?.decorated) ?? incoming.decorated}) + } +} + const mergeMessage = ( existing: WritableDraft, incoming: WritableDraft @@ -164,12 +181,21 @@ const mergeMessage = ( ;(cur as Map).delete(k) } } - for (const [k, v] of val as Map) { - ;(cur as Map).set(k, v) + if (key === 'reactions') { + mergeReactions(cur as Map, val as Map) + } else { + for (const [k, v] of val as Map) { + ;(cur as Map).set(k, v) + } } } 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 } diff --git a/shared/common-adapters/image.tsx b/shared/common-adapters/image.tsx index 26d22748be6b..ca3bc603e85f 100644 --- a/shared/common-adapters/image.tsx +++ b/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,9 +53,6 @@ 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) => { @@ -63,6 +61,7 @@ const NativeImage = (p: Props) => { const [lastSrc, setLastSrc] = React.useState(src) const [attempt, setAttempt] = React.useState(0) const retryable = isLocalhostSrc(src) + const httpSrv = useConfigState(s => s.httpSrv) const failedRef = React.useRef(false) const triesRef = React.useRef(0) const timerRef = React.useRef>(undefined) @@ -130,9 +129,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 = retryable && attempt > 0 ? retryLocalhostSrc(src, attempt, httpSrv) : src const recyclingKey = typeof src === 'string' ? src : Array.isArray(src) ? src[0]?.uri : String(src) return ( diff --git a/shared/common-adapters/localhost-src.test.ts b/shared/common-adapters/localhost-src.test.ts new file mode 100644 index 000000000000..c0d324d5d1dc --- /dev/null +++ b/shared/common-adapters/localhost-src.test.ts @@ -0,0 +1,44 @@ +/// +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' + ) +}) diff --git a/shared/common-adapters/localhost-src.tsx b/shared/common-adapters/localhost-src.tsx new file mode 100644 index 000000000000..f2420f422777 --- /dev/null +++ b/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}` +} From 4d269413ac9b64d287ca97971889d006f983a9d6 Mon Sep 17 00:00:00 2001 From: chrisnojima Date: Wed, 23 Sep 2026 12:55:52 -0400 Subject: [PATCH 2/4] fix(js): drop the reactions keepUrl guard, decorated is never a bare url --- .../conversation/thread-message-state.test.tsx | 8 -------- .../chat/conversation/thread-message-state.tsx | 18 ++---------------- 2 files changed, 2 insertions(+), 24 deletions(-) diff --git a/shared/chat/conversation/thread-message-state.test.tsx b/shared/chat/conversation/thread-message-state.test.tsx index 5ffcf90b4bbd..fc76d49247a5 100644 --- a/shared/chat/conversation/thread-message-state.test.tsx +++ b/shared/chat/conversation/thread-message-state.test.tsx @@ -680,7 +680,6 @@ describe('addMessagesToThreadState', () => { // field only ever takes an incoming value that actually looks like one. describe('local server urls', () => { const attachmentOrdinal = T.Chat.numberToOrdinal(201) - const reactionOrdinal = ordinal const validUrl = 'http://127.0.0.1:1234/at?key=abc' const garbageUrl = '&prev=false&noanim=true' @@ -706,13 +705,6 @@ describe('local server urls', () => { read: (state: WritableConversationThreadMessageState) => (state.messageMap.get(attachmentOrdinal) as T.Chat.MessageAttachment).previewURL, }, - { - field: 'emoji src', - make: (url: string) => - makeTextMessage({reactions: new Map([[':party:', {decorated: url, users: [{timestamp: 1, username: 'testuser'}]}]])}), - read: (state: WritableConversationThreadMessageState) => - (state.messageMap.get(reactionOrdinal) as T.Chat.MessageText).reactions?.get(':party:')?.decorated, - }, ])('$field: empty or garbage keeps the existing value, a real url replaces it', ({make, read}) => { const empty = makeThreadState([]) addMessagesToThreadState(empty, [make(validUrl)], {}) diff --git a/shared/chat/conversation/thread-message-state.tsx b/shared/chat/conversation/thread-message-state.tsx index dec6da309890..143540e0ac72 100644 --- a/shared/chat/conversation/thread-message-state.tsx +++ b/shared/chat/conversation/thread-message-state.tsx @@ -150,16 +150,6 @@ const maybeGetOrdinalByMessageID = ( const keepUrl = (next: string | undefined, prev: string | undefined) => next?.startsWith('http://') ? next : prev ?? next -const mergeReactions = ( - cur: Map, - val: Map -) => { - for (const [emoji, incoming] of val) { - const existing = cur.get(emoji) - cur.set(emoji, {...incoming, decorated: keepUrl(incoming.decorated, existing?.decorated) ?? incoming.decorated}) - } -} - const mergeMessage = ( existing: WritableDraft, incoming: WritableDraft @@ -181,12 +171,8 @@ const mergeMessage = ( ;(cur as Map).delete(k) } } - if (key === 'reactions') { - mergeReactions(cur as Map, val as Map) - } else { - for (const [k, v] of val as Map) { - ;(cur as Map).set(k, v) - } + for (const [k, v] of val as Map) { + ;(cur as Map).set(k, v) } } else { existingRecord[key] = val From 2d1fe13d77a0e1c224cf40a978c9eae26c7a2c7c Mon Sep 17 00:00:00 2001 From: chrisnojima Date: Wed, 23 Sep 2026 15:31:59 -0400 Subject: [PATCH 3/4] perf(js): resolve an image's retried src when the retry is scheduled Each image no longer subscribes to the config store's httpSrv; the retry reads it once and keeps the rewritten src in state. --- shared/common-adapters/image.tsx | 23 +++++++++++++++-------- 1 file changed, 15 insertions(+), 8 deletions(-) diff --git a/shared/common-adapters/image.tsx b/shared/common-adapters/image.tsx index ca3bc603e85f..5256088bd107 100644 --- a/shared/common-adapters/image.tsx +++ b/shared/common-adapters/image.tsx @@ -59,9 +59,9 @@ 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 httpSrv = useConfigState(s => s.httpSrv) const failedRef = React.useRef(false) const triesRef = React.useRef(0) const timerRef = React.useRef>(undefined) @@ -75,9 +75,18 @@ 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 failedRef.current = false @@ -89,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) @@ -110,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,7 +137,7 @@ const NativeImage = (p: Props) => { }, [retryable]) // recyclingKey stays on the original src so the view isn't blanked by retries - const srcToUse = retryable && attempt > 0 ? retryLocalhostSrc(src, attempt, httpSrv) : src + const srcToUse = retry.src ?? src const recyclingKey = typeof src === 'string' ? src : Array.isArray(src) ? src[0]?.uri : String(src) return ( From a11347b7e865f868e6f5ac689e955f99c5cdd40a Mon Sep 17 00:00:00 2001 From: chrisnojima Date: Wed, 23 Sep 2026 15:31:59 -0400 Subject: [PATCH 4/4] docs(chat): describe the keepUrl guard without referring to master --- shared/chat/conversation/thread-message-state.test.tsx | 4 ++-- shared/chat/conversation/thread-message-state.tsx | 6 +++--- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/shared/chat/conversation/thread-message-state.test.tsx b/shared/chat/conversation/thread-message-state.test.tsx index fc76d49247a5..ac22c72ee69c 100644 --- a/shared/chat/conversation/thread-message-state.test.tsx +++ b/shared/chat/conversation/thread-message-state.test.tsx @@ -675,8 +675,8 @@ describe('addMessagesToThreadState', () => { }) // The http server can be stopped (BACKGROUND) or mid-restart (a fresh port/token) when an update -// carrying a stale GetURL lands: on master 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 +// 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) diff --git a/shared/chat/conversation/thread-message-state.tsx b/shared/chat/conversation/thread-message-state.tsx index 143540e0ac72..da18e4485828 100644 --- a/shared/chat/conversation/thread-message-state.tsx +++ b/shared/chat/conversation/thread-message-state.tsx @@ -144,9 +144,9 @@ 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: on master 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. +// 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