Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -64,17 +64,53 @@ describe('MessageRenderBoundary', () => {
spy.mockRestore()
})

it('re-throws unrelated errors so real bugs still surface', () => {
it('contains unrelated errors to an inline fallback instead of unwinding to the root', () => {
const spy = vi.spyOn(console, 'error').mockImplementation(() => undefined)

expect(() =>
render(
<MessageRenderBoundary resetKey="a">
<Boom error={new Error('genuine render bug')} />
</MessageRenderBoundary>
)
).toThrow('genuine render bug')
render(
<MessageRenderBoundary resetKey="a">
<Boom error={new Error('genuine render bug')} />
</MessageRenderBoundary>
)

expect(screen.getByRole('alert').textContent).toBe('This message failed to render.')
expect(spy).toHaveBeenCalledWith(
'[message-render-boundary]',
expect.objectContaining({ message: 'genuine render bug' })
)
spy.mockRestore()
})

it('does not tag the transient lookup race in the console', () => {
const spy = vi.spyOn(console, 'error').mockImplementation(() => undefined)

render(
<MessageRenderBoundary resetKey="a">
<Boom error={lookupError} />
</MessageRenderBoundary>
)

const tagged = spy.mock.calls.filter(call => call[0] === '[message-render-boundary]')
expect(tagged).toEqual([])
spy.mockRestore()
})

it('recovers from a contained error when resetKey changes', () => {
const spy = vi.spyOn(console, 'error').mockImplementation(() => undefined)

const { rerender } = render(
<MessageRenderBoundary resetKey="a">
<Boom error={new Error('genuine render bug')} />
</MessageRenderBoundary>
)

rerender(
<MessageRenderBoundary resetKey="b">
<div>recovered</div>
</MessageRenderBoundary>
)

expect(screen.getByText('recovered')).toBeTruthy()
spy.mockRestore()
})
})
Original file line number Diff line number Diff line change
@@ -1,17 +1,32 @@
import { Component, type ReactNode } from 'react'

import { useI18n } from '@/i18n'

// `@assistant-ui/store`'s index-keyed child-scope lookup (`tapClientLookup`)
// throws — rather than returning undefined — when a subscriber reads an index
// that the message/parts list no longer has. This races during high-frequency
// store replacement (session switch mid-stream, gateway reconnect replay): a
// subscriber from the previous, longer list is still in React's notification
// queue and reads one slot past the new, shorter array before it can unmount.
// The throw is transient and self-heals on the next consistent snapshot, but
// without a local boundary it unwinds to the root and blanks the whole app.
// The throw is transient and self-heals on the next consistent snapshot, so it
// is swallowed silently (no fallback flash, no log spam).
// Upstream-tracked: assistant-ui/assistant-ui#4051, #3652.
const isTransientLookupError = (error: unknown): boolean =>

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Current main's matcher includes useClientLookup as well as legacy tapClient* signatures (commit 673a61edc after this PR base). Keep that branch when salvaging this change or the current dependency's transient race will again escape this boundary.

error instanceof Error && /tapClient(Lookup|Resource).*out of bounds/.test(error.message)

function MessageRenderFallback() {
const { t } = useI18n()

return (
<p
className="rounded-md border border-border/65 bg-(--composer-fill) px-3 py-2 text-xs text-muted-foreground"
role="alert"
>
{t.assistant.thread.messageRenderFailed}
</p>
)
}

interface Props {
// Changes whenever the message list mutates; remounting clears the caught
// error so the next consistent render recovers silently.
Expand All @@ -26,6 +41,16 @@ export class MessageRenderBoundary extends Component<Props, { error: Error | nul
return { error }
}

componentDidCatch(error: Error) {
// One malformed message must not unwind to the root boundary and blank
// the whole app (sidebar, composer, every other session). Contain it here
// and keep the cause in the console pipeline that main persists to
// desktop.log so real render bugs still surface.
if (!isTransientLookupError(error)) {
console.error('[message-render-boundary]', error)
}
}

componentDidUpdate(prev: Props) {
if (this.state.error && prev.resetKey !== this.props.resetKey) {
this.setState({ error: null })
Expand All @@ -34,13 +59,7 @@ export class MessageRenderBoundary extends Component<Props, { error: Error | nul

render() {
if (this.state.error) {
// Only swallow the transient store race; re-throw anything else so real
// bugs still reach the root error boundary.
if (!isTransientLookupError(this.state.error)) {
throw this.state.error
}

return null
return isTransientLookupError(this.state.error) ? null : <MessageRenderFallback />
}

return this.props.children
Expand Down
31 changes: 31 additions & 0 deletions apps/desktop/src/components/react-root-error-logging.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
import { afterEach, describe, expect, it, vi } from 'vitest'

import { reactRootErrorOptions } from './react-root-error-logging'

describe('reactRootErrorOptions', () => {
afterEach(() => {
vi.restoreAllMocks()
})

it('logs the wrapped cause and component stack for recoverable errors', () => {
const spy = vi.spyOn(console, 'error').mockImplementation(() => undefined)
const cause = new TypeError("Cannot read properties of undefined (reading 'refText')")
const wrapper = new Error('Minified React error #520', { cause })

reactRootErrorOptions().onRecoverableError?.(wrapper, { componentStack: '\n at Thread' })

expect(spy).toHaveBeenCalledWith('[react:recoverable]', wrapper, 'cause:', cause, '\n at Thread')
})

it('logs caught and uncaught errors without a cause', () => {
const spy = vi.spyOn(console, 'error').mockImplementation(() => undefined)
const error = new Error('boom')
const options = reactRootErrorOptions()

options.onCaughtError?.(error, { componentStack: '\n at App' })
options.onUncaughtError?.(error, {})

expect(spy).toHaveBeenNthCalledWith(1, '[react:caught]', error, '\n at App')
expect(spy).toHaveBeenNthCalledWith(2, '[react:uncaught]', error, '')
})
})
28 changes: 28 additions & 0 deletions apps/desktop/src/components/react-root-error-logging.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
import type { RootOptions } from 'react-dom/client'

// createRoot error hooks that keep minified production errors identifiable.
// React's defaults report only the wrapper (e.g. "Minified React error #520",
// the recoverable concurrent-render wrapper) — the real culprit rides in
// `error.cause` and the component stack, which the defaults drop. Logging via
// console.error lands in the console-message pipeline Electron main persists
// to desktop.log.
function logRootError(tag: string, error: unknown, errorInfo: { componentStack?: string }): void {
const cause = error instanceof Error ? error.cause : undefined

if (cause !== undefined) {
console.error(tag, error, 'cause:', cause, errorInfo.componentStack ?? '')
} else {
console.error(tag, error, errorInfo.componentStack ?? '')
}
}

export function reactRootErrorOptions(): Pick<
RootOptions,
'onCaughtError' | 'onRecoverableError' | 'onUncaughtError'
> {
return {
onRecoverableError: (error, errorInfo) => logRootError('[react:recoverable]', error, errorInfo),
onCaughtError: (error, errorInfo) => logRootError('[react:caught]', error, errorInfo),
onUncaughtError: (error, errorInfo) => logRootError('[react:uncaught]', error, errorInfo)
}
}
75 changes: 74 additions & 1 deletion apps/desktop/src/hermes.test.ts
Original file line number Diff line number Diff line change
@@ -1,16 +1,28 @@
import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'

import {
createCronJob,
deleteCronJob,
getCronJob,
getCronJobRuns,
getCronJobs,
getGlobalModelInfo,
getGlobalModelOptions,
getHermesConfig,
getHermesConfigDefaults,
getMessagingPlatforms,
getProfiles,
getSessionMessages,
getStatus,
listAllProfileSessions,
listSessions
listSessions,
pauseCronJob,
resumeCronJob,
setApiRequestProfile,
testMessagingPlatform,
triggerCronJob,
updateCronJob,
updateMessagingPlatform
} from './hermes'
import { refreshActiveProfile } from './store/profile'

Expand Down Expand Up @@ -134,4 +146,65 @@ describe('Hermes REST session helpers', () => {
profile: 'xiaoxuxu'
})
})

describe('profile scoping of cron and messaging wrappers', () => {
afterEach(() => {
setApiRequestProfile(null)
})

it('tags every cron call with the active profile so jobs never land in "default"', async () => {
api.mockResolvedValue({})
setApiRequestProfile('xiaoxuxu')

const cronCalls: [() => Promise<unknown>, string][] = [
[getCronJobs, '/api/cron/jobs'],
[() => getCronJob('job-1'), '/api/cron/jobs/job-1'],
[() => getCronJobRuns('job-1'), '/api/cron/jobs/job-1/runs?limit=20'],
[() => createCronJob({ name: 'n', prompt: 'p', schedule: '* * * * *' }), '/api/cron/jobs'],
[() => updateCronJob('job-1', {}), '/api/cron/jobs/job-1'],
[() => pauseCronJob('job-1'), '/api/cron/jobs/job-1/pause'],
[() => resumeCronJob('job-1'), '/api/cron/jobs/job-1/resume'],
[() => triggerCronJob('job-1'), '/api/cron/jobs/job-1/trigger'],
[() => deleteCronJob('job-1'), '/api/cron/jobs/job-1']
]

for (const [call, path] of cronCalls) {
api.mockClear()
await call()
expect(api).toHaveBeenCalledWith(expect.objectContaining({ path, profile: 'xiaoxuxu' }))
}
})

it('tags messaging platform calls with the active profile', async () => {
api.mockResolvedValue({})
setApiRequestProfile('xiaoxuxu')

const messagingCalls: [() => Promise<unknown>, string][] = [
[getMessagingPlatforms, '/api/messaging/platforms'],
[
() => updateMessagingPlatform('telegram', { clear_env: [], env: {} }),
'/api/messaging/platforms/telegram'
],
[() => testMessagingPlatform('telegram'), '/api/messaging/platforms/telegram/test']
]

for (const [call, path] of messagingCalls) {
api.mockClear()
await call()
expect(api).toHaveBeenCalledWith(expect.objectContaining({ path, profile: 'xiaoxuxu' }))
}
})

it('leaves cron and messaging unscoped for the primary profile', async () => {
api.mockResolvedValue({})
api.mockClear()

await getCronJobs()
await getMessagingPlatforms()

for (const [request] of api.mock.calls) {
expect(request).not.toHaveProperty('profile')
}
})
})
})
18 changes: 18 additions & 0 deletions apps/desktop/src/hermes.ts
Original file line number Diff line number Diff line change
Expand Up @@ -725,6 +725,7 @@ export function grantComputerUsePermissions(): Promise<ActionResponse> {

export function getMessagingPlatforms(): Promise<MessagingPlatformsResponse> {
return window.hermesDesktop.api<MessagingPlatformsResponse>({
...profileScoped(),
path: '/api/messaging/platforms'
})
}
Expand All @@ -734,6 +735,7 @@ export function updateMessagingPlatform(
body: MessagingPlatformUpdate
): Promise<{ ok: boolean; platform: string }> {
return window.hermesDesktop.api<{ ok: boolean; platform: string }>({
...profileScoped(),
path: `/api/messaging/platforms/${encodeURIComponent(platformId)}`,
method: 'PUT',
body
Expand All @@ -742,26 +744,36 @@ export function updateMessagingPlatform(

export function testMessagingPlatform(platformId: string): Promise<MessagingPlatformTestResponse> {
return window.hermesDesktop.api<MessagingPlatformTestResponse>({
...profileScoped(),
path: `/api/messaging/platforms/${encodeURIComponent(platformId)}/test`,
method: 'POST'
})
}

// Cron routes must be profile-scoped like config/env/skills: the backend
// defaults POST /api/cron/jobs to profile "default" (a job created while a
// non-default profile is active would silently land in the wrong profile) and
// per-job routes resolve the owning profile by job-id scan, which mismatches
// on id collisions. Unscoped (primary profile active) GET keeps the backend's
// profile="all" default, so single-profile users see the same list as before.
export function getCronJobs(): Promise<CronJob[]> {
return window.hermesDesktop.api<CronJob[]>({
...profileScoped(),
path: '/api/cron/jobs',
timeoutMs: STARTUP_REQUEST_TIMEOUT_MS
})
}

export function getCronJob(jobId: string): Promise<CronJob> {
return window.hermesDesktop.api<CronJob>({
...profileScoped(),
path: `/api/cron/jobs/${encodeURIComponent(jobId)}`
})
}

export async function getCronJobRuns(jobId: string, limit = 20): Promise<SessionInfo[]> {
const { runs } = await window.hermesDesktop.api<{ runs: SessionInfo[] }>({
...profileScoped(),
path: `/api/cron/jobs/${encodeURIComponent(jobId)}/runs?limit=${limit}`
})

Expand All @@ -770,6 +782,7 @@ export async function getCronJobRuns(jobId: string, limit = 20): Promise<Session

export function createCronJob(body: CronJobCreatePayload): Promise<CronJob> {
return window.hermesDesktop.api<CronJob>({
...profileScoped(),
path: '/api/cron/jobs',
method: 'POST',
body
Expand All @@ -778,6 +791,7 @@ export function createCronJob(body: CronJobCreatePayload): Promise<CronJob> {

export function updateCronJob(jobId: string, updates: CronJobUpdates): Promise<CronJob> {
return window.hermesDesktop.api<CronJob>({
...profileScoped(),
path: `/api/cron/jobs/${encodeURIComponent(jobId)}`,
method: 'PUT',
body: { updates }
Expand All @@ -786,27 +800,31 @@ export function updateCronJob(jobId: string, updates: CronJobUpdates): Promise<C

export function pauseCronJob(jobId: string): Promise<CronJob> {
return window.hermesDesktop.api<CronJob>({
...profileScoped(),
path: `/api/cron/jobs/${encodeURIComponent(jobId)}/pause`,
method: 'POST'
})
}

export function resumeCronJob(jobId: string): Promise<CronJob> {
return window.hermesDesktop.api<CronJob>({
...profileScoped(),
path: `/api/cron/jobs/${encodeURIComponent(jobId)}/resume`,
method: 'POST'
})
}

export function triggerCronJob(jobId: string): Promise<CronJob> {
return window.hermesDesktop.api<CronJob>({
...profileScoped(),
path: `/api/cron/jobs/${encodeURIComponent(jobId)}/trigger`,
method: 'POST'
})
}

export function deleteCronJob(jobId: string): Promise<{ ok: boolean }> {
return window.hermesDesktop.api<{ ok: boolean }>({
...profileScoped(),
path: `/api/cron/jobs/${encodeURIComponent(jobId)}`,
method: 'DELETE'
})
Expand Down
1 change: 1 addition & 0 deletions apps/desktop/src/i18n/en.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2217,6 +2217,7 @@ export const en: Translations = {
thread: {
loadingSession: 'Loading session',
showEarlier: 'Show earlier messages',
messageRenderFailed: 'This message failed to render.',
loadingResponse: 'Hermes is loading a response',
resumeWhenBackgroundDone: count =>
count === 1
Expand Down
Loading