Skip to content

Commit ca11667

Browse files
authored
fix(chat): resolve a new chat's history entry to the chat route on Back, and say when an effort save fails (#8703)
1 parent db30e1a commit ca11667

7 files changed

Lines changed: 244 additions & 11 deletions

File tree

‎apps/sim/app/o/[organizationId]/home/organization-home.tsx‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -35,6 +35,7 @@ import {
3535
useChatResourcePanel,
3636
useResourcePanelController,
3737
} from '@/app/workspace/[workspaceId]/home/hooks/use-resource-panel'
38+
import { useRestoredChatEntry } from '@/app/workspace/[workspaceId]/home/hooks/use-restored-chat-entry'
3839
import { resolveWorkspaceResourceRef } from '@/app/workspace/[workspaceId]/home/resolve-resource-ref'
3940
import { searchFiltersFromParams } from '@/app/workspace/[workspaceId]/home/search-params'
4041
import type {
@@ -66,9 +67,10 @@ export function OrganizationHome(props: OrganizationHomeProps) {
6667
const { organization, searchAccess, canBuild, mothershipAvailable } = useOrganizationContext()
6768
const { data: session } = useSession()
6869
const isClient = useSyncExternalStore(subscribeToClient, clientSnapshot, serverSnapshot)
70+
const isRestoredChatEntry = useRestoredChatEntry({ chatId: props.chatId })
6971
if (!mothershipAvailable || (!canBuild && !searchAccess.memberScoped)) return null
7072
/** Preferences are browser-persisted and keyed by user; never paint a guessed mode first. */
71-
if (!isClient || !session?.user?.id) return <HomeFallback />
73+
if (isRestoredChatEntry || !isClient || !session?.user?.id) return <HomeFallback />
7274
return (
7375
<OrganizationHomeContent
7476
key={`${session.user.id}:${organization.id}:${props.chatId ?? 'new'}`}

‎apps/sim/app/workspace/[workspaceId]/home/home.tsx‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -24,10 +24,12 @@ import { persistImportedWorkflow } from '@/lib/workflows/operations/import-expor
2424
import { ChatResourcePanel } from '@/app/workspace/[workspaceId]/home/components/chat-resource-panel'
2525
import { RESOURCE_HEADER_CLASSES } from '@/app/workspace/[workspaceId]/home/components/mothership-view/components/resource-tabs/resource-tab-controls'
2626
import { SuggestedActions } from '@/app/workspace/[workspaceId]/home/components/suggested-actions'
27+
import { HomeFallback } from '@/app/workspace/[workspaceId]/home/home-fallback'
2728
import {
2829
useChatResourcePanel,
2930
useResourcePanelController,
3031
} from '@/app/workspace/[workspaceId]/home/hooks/use-resource-panel'
32+
import { useRestoredChatEntry } from '@/app/workspace/[workspaceId]/home/hooks/use-restored-chat-entry'
3133
import { resolveWorkspaceResourceRef } from '@/app/workspace/[workspaceId]/home/resolve-resource-ref'
3234
import { PermissionAccessBoundary } from '@/ee/access-requests/components/permission-access-boundary'
3335
import { useMarkMothershipChatRead } from '@/hooks/queries/mothership-chats'
@@ -58,6 +60,8 @@ interface HomeProps {
5860
}
5961

6062
export function Home(props: HomeProps) {
63+
const isRestoredChatEntry = useRestoredChatEntry({ chatId: props.chatId })
64+
if (isRestoredChatEntry) return <HomeFallback />
6165
return (
6266
<PermissionAccessBoundary configKey='hideCopilot'>
6367
<HomeContent {...props} />
Lines changed: 83 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,83 @@
1+
/** @vitest-environment jsdom */
2+
3+
import { act, StrictMode } from 'react'
4+
import { nextNavigationMock, nextNavigationMockFns } from '@sim/testing/mocks/next-navigation.mock'
5+
import { createRoot, type Root } from 'react-dom/client'
6+
import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'
7+
8+
vi.mock('next/navigation', () => nextNavigationMock)
9+
10+
import { useRestoredChatEntry } from '@/app/workspace/[workspaceId]/home/hooks/use-restored-chat-entry'
11+
12+
interface SurfaceProps {
13+
chatId?: string
14+
}
15+
16+
let isRestoredEntry: boolean | undefined
17+
function Surface({ chatId }: SurfaceProps) {
18+
isRestoredEntry = useRestoredChatEntry({ chatId })
19+
return null
20+
}
21+
22+
let root: Root
23+
function at(url: string) {
24+
const { pathname, search } = new URL(url, 'http://localhost')
25+
nextNavigationMockFns.mockUsePathname.mockReturnValue(pathname)
26+
nextNavigationMockFns.mockUseSearchParams.mockReturnValue(new URLSearchParams(search))
27+
}
28+
function render(props: SurfaceProps = {}) {
29+
act(() =>
30+
root.render(
31+
<StrictMode>
32+
<Surface {...props} />
33+
</StrictMode>
34+
)
35+
)
36+
}
37+
38+
beforeEach(() => {
39+
vi.stubGlobal('IS_REACT_ACT_ENVIRONMENT', true)
40+
isRestoredEntry = undefined
41+
root = createRoot(document.createElement('div'))
42+
})
43+
44+
afterEach(() => {
45+
act(() => root.unmount())
46+
})
47+
48+
describe('useRestoredChatEntry', () => {
49+
it('hands a new-chat surface restored at a chat URL to the router, query included', () => {
50+
at('/workspace/w/chat/c?resource=x')
51+
render()
52+
53+
expect(isRestoredEntry).toBe(true)
54+
expect(nextNavigationMockFns.router.replace).toHaveBeenLastCalledWith(
55+
'/workspace/w/chat/c?resource=x',
56+
{ scroll: false }
57+
)
58+
})
59+
60+
it('keeps rendering the surface that moved its own URL to the chat during a turn', () => {
61+
at('/workspace/w/home')
62+
render()
63+
at('/workspace/w/chat/c')
64+
render()
65+
66+
expect(isRestoredEntry).toBe(false)
67+
expect(nextNavigationMockFns.router.replace).not.toHaveBeenCalled()
68+
})
69+
70+
it('leaves a chat surface and the home URL alone', () => {
71+
at('/workspace/w/chat/c')
72+
render({ chatId: 'c' })
73+
expect(isRestoredEntry).toBe(false)
74+
75+
act(() => root.unmount())
76+
root = createRoot(document.createElement('div'))
77+
at('/o/o/home')
78+
render()
79+
expect(isRestoredEntry).toBe(false)
80+
81+
expect(nextNavigationMockFns.router.replace).not.toHaveBeenCalled()
82+
})
83+
})
Lines changed: 39 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,39 @@
1+
import { useEffect, useState } from 'react'
2+
import { usePathname, useRouter, useSearchParams } from 'next/navigation'
3+
4+
const CHAT_PATH = /^\/(?:workspace|o)\/[^/]+\/chat\/[^/]+$/
5+
6+
interface UseRestoredChatEntryProps {
7+
/** The chat the surface was opened for; the new-chat surface has none. */
8+
chatId: string | undefined
9+
}
10+
11+
/**
12+
* Hands a restored new-chat history entry back to the router.
13+
*
14+
* A new chat moves its URL from the home route to `/chat/<id>` in place, through
15+
* `history.replaceState`, so the turn streaming on that surface stays mounted. Next keeps
16+
* the home route's tree in that history entry, so Back or Forward to it mounts the home
17+
* route at the chat's URL. Replacing the entry through the router resolves the chat route
18+
* and stores its tree, so later visits to the entry render the chat directly. Only the URL
19+
* at mount counts: the surface that moved its own URL keeps rendering.
20+
*
21+
* @returns Whether this mount is a restored entry; the caller renders its fallback until
22+
* the chat route replaces it.
23+
*/
24+
export function useRestoredChatEntry({ chatId }: UseRestoredChatEntryProps): boolean {
25+
const router = useRouter()
26+
const pathname = usePathname()
27+
const searchParams = useSearchParams()
28+
const [restoredChatUrl] = useState(() => {
29+
if (chatId || !CHAT_PATH.test(pathname)) return null
30+
const search = searchParams.toString()
31+
return search ? `${pathname}?${search}` : pathname
32+
})
33+
34+
useEffect(() => {
35+
if (restoredChatUrl) router.replace(restoredChatUrl, { scroll: false })
36+
}, [restoredChatUrl, router])
37+
38+
return restoredChatUrl !== null
39+
}
Lines changed: 99 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,99 @@
1+
/** @vitest-environment jsdom */
2+
3+
import { act, useEffect } from 'react'
4+
import { jsonResponse } from '@sim/testing/helpers/http'
5+
import { nextNavigationMock } from '@sim/testing/mocks/next-navigation.mock'
6+
import { sleep } from '@sim/utils/helpers'
7+
import { QueryClient, QueryClientProvider } from '@tanstack/react-query'
8+
import { createRoot, type Root } from 'react-dom/client'
9+
import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'
10+
11+
vi.mock('next/navigation', () => nextNavigationMock)
12+
13+
import { ToastProvider } from '@sim/emcn'
14+
import type { MothershipEffort } from '@/lib/mothership/model-options'
15+
import { useSetMothershipChatEffort } from '@/hooks/queries/mothership-chats'
16+
import { useMothershipEffortStore } from '@/stores/mothership-effort/store'
17+
18+
const FAILURE_NOTICE = "Couldn't change reasoning effort"
19+
20+
interface EffortPickerProps {
21+
onReady: (pick: (effort: MothershipEffort) => Promise<void>) => void
22+
}
23+
24+
function EffortPicker({ onReady }: EffortPickerProps) {
25+
const { mutateAsync } = useSetMothershipChatEffort('chat-1')
26+
useEffect(
27+
() => onReady((effort) => mutateAsync(effort).catch(() => undefined)),
28+
[onReady, mutateAsync]
29+
)
30+
return null
31+
}
32+
33+
let root: Root
34+
let pick: (effort: MothershipEffort) => Promise<void>
35+
36+
beforeEach(async () => {
37+
vi.stubGlobal('IS_REACT_ACT_ENVIRONMENT', true)
38+
vi.stubGlobal(
39+
'ResizeObserver',
40+
class {
41+
observe() {}
42+
unobserve() {}
43+
disconnect() {}
44+
}
45+
)
46+
vi.stubGlobal('fetch', vi.fn())
47+
useMothershipEffortStore.getState().reset()
48+
const container = document.createElement('div')
49+
document.body.appendChild(container)
50+
root = createRoot(container)
51+
await act(async () => {
52+
root.render(
53+
<QueryClientProvider client={new QueryClient()}>
54+
<ToastProvider>
55+
<EffortPicker
56+
onReady={(next) => {
57+
pick = next
58+
}}
59+
/>
60+
</ToastProvider>
61+
</QueryClientProvider>
62+
)
63+
})
64+
})
65+
66+
afterEach(() => {
67+
act(() => root.unmount())
68+
document.body.innerHTML = ''
69+
})
70+
71+
describe('chat effort save failures', () => {
72+
it('tells the user when a failed save rolls their pick back', async () => {
73+
vi.mocked(fetch).mockResolvedValueOnce(new Response('save failed', { status: 500 }))
74+
75+
await act(() => pick('xhigh'))
76+
77+
expect(useMothershipEffortStore.getState().chatEfforts['chat-1']).toBeUndefined()
78+
expect(document.body.textContent).toContain(FAILURE_NOTICE)
79+
})
80+
81+
it('stays quiet when a newer pick already replaced the one that failed', async () => {
82+
const saves = [Promise.withResolvers<Response>(), Promise.withResolvers<Response>()]
83+
for (const save of saves) vi.mocked(fetch).mockReturnValueOnce(save.promise)
84+
85+
let outcomes: Promise<void>[] = []
86+
await act(async () => {
87+
outcomes = [pick('low'), pick('high')]
88+
await sleep(1)
89+
})
90+
await act(async () => {
91+
saves[0].resolve(new Response('save failed', { status: 500 }))
92+
saves[1].resolve(jsonResponse({ success: true }))
93+
await Promise.all(outcomes)
94+
})
95+
96+
expect(useMothershipEffortStore.getState().chatEfforts['chat-1']?.effort).toBe('high')
97+
expect(document.body.textContent).not.toContain(FAILURE_NOTICE)
98+
})
99+
})

‎apps/sim/hooks/queries/mothership-chats.ts‎

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,4 @@
1+
import { toast } from '@sim/emcn'
12
import { toError } from '@sim/utils/errors'
23
import { isRecordLike } from '@sim/utils/object'
34
import {
@@ -663,8 +664,10 @@ function chatEffortMutationOptions(queryClient: QueryClient, chatId: string | un
663664
return { pick: useMothershipEffortStore.getState().setChatEffort(chatId, effort) }
664665
},
665666
onError: (_error, _effort, context) => {
666-
if (chatId && context)
667-
useMothershipEffortStore.getState().dropChatEffort(chatId, context.pick)
667+
if (!chatId || !context) return
668+
if (useMothershipEffortStore.getState().dropChatEffort(chatId, context.pick)) {
669+
toast.error("Couldn't change reasoning effort")
670+
}
668671
},
669672
onSuccess: (_data, effort) => {
670673
queryClient.setQueryData<MothershipChatHistory>(

‎apps/sim/stores/mothership-effort/store.ts‎

Lines changed: 11 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -27,8 +27,11 @@ interface MothershipEffortState {
2727
chatEfforts: Record<string, ChatEffortPick>
2828
/** Records a pick and returns its token for {@link MothershipEffortState.dropChatEffort}. */
2929
setChatEffort: (chatId: string, effort: MothershipEffort) => number
30-
/** Drops a pick whose save failed, unless a newer pick replaced it, even one of the same value. */
31-
dropChatEffort: (chatId: string, pick: number) => void
30+
/**
31+
* Drops a pick whose save failed, unless a newer pick replaced it, even one of the same value.
32+
* Returns whether it dropped the pick, which rolls the chat back to its saved effort.
33+
*/
34+
dropChatEffort: (chatId: string, pick: number) => boolean
3235
/** Moves the new-chat pick onto the chat its first send created. */
3336
adoptNewChatEffort: (chatId: string, effort: MothershipEffort) => void
3437
reset: () => void
@@ -55,7 +58,7 @@ function withModelSelection(
5558
export const useMothershipEffortStore = create<MothershipEffortState>()(
5659
devtools(
5760
persist(
58-
(set) => ({
61+
(set, get) => ({
5962
...initialState,
6063
setFastMode: (fastMode) =>
6164
set((state) => withModelSelection({ ...state.modelSelection, fastMode })),
@@ -67,11 +70,11 @@ export const useMothershipEffortStore = create<MothershipEffortState>()(
6770
set((state) => ({ chatEfforts: { ...state.chatEfforts, [chatId]: { effort, pick } } }))
6871
return pick
6972
},
70-
dropChatEffort: (chatId, pick) =>
71-
set((state) => {
72-
if (state.chatEfforts[chatId]?.pick !== pick) return state
73-
return { chatEfforts: omit(state.chatEfforts, [chatId]) }
74-
}),
73+
dropChatEffort: (chatId, pick) => {
74+
if (get().chatEfforts[chatId]?.pick !== pick) return false
75+
set((state) => ({ chatEfforts: omit(state.chatEfforts, [chatId]) }))
76+
return true
77+
},
7578
adoptNewChatEffort: (chatId, effort) => {
7679
const pick = ++lastChatEffortPick
7780
set((state) => ({

0 commit comments

Comments
 (0)