Skip to content

Commit eac0909

Browse files
Fix question prompt not rendering for active session (#325)
* Fix question prompt not rendering for active session currentQuestion was the first pending question globally across all sessions. When another session had the first question in the flat list, the QuestionPrompt would not render for the session being viewed even though it had its own pending question. Add getForSession() to the questions context and use it in SessionDetail to look up questions scoped to the current session. * test(frontend): cover session-scoped question prompt resolution The SessionDetail useQuestions mocks did not expose getForSession, so every page test threw on the new session-scoped lookup. Add it to each mock and lock in the behaviour with regression coverage: the context helper resolves questions per session while another session owns the global current question, and SessionDetail renders the prompt for the session being viewed. --------- Co-authored-by: Chris Scott <99081550+chriswritescode-dev@users.noreply.github.com>
1 parent 6cc50a0 commit eac0909

8 files changed

Lines changed: 313 additions & 3 deletions

frontend/src/contexts/EventContext.test.tsx

Lines changed: 21 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -94,7 +94,7 @@ const pendingPermission: PermissionRequest = {
9494
}
9595

9696
function Harness() {
97-
const { current, pendingCount, syncForSession, navigateToCurrent, reject, reply } = useQuestions()
97+
const { current, pendingCount, syncForSession, navigateToCurrent, reject, reply, getForSession } = useQuestions()
9898
const permissions = usePermissions()
9999
const permissionForCall = permissions.getForCallID('call-1', 'session-1')
100100
const location = useLocation()
@@ -103,6 +103,9 @@ function Harness() {
103103
<div>
104104
<div data-testid="count">{pendingCount}</div>
105105
<div data-testid="current">{current?.id ?? 'none'}</div>
106+
<div data-testid="for-session-1">{getForSession('session-1')?.id ?? 'none'}</div>
107+
<div data-testid="for-session-2">{getForSession('session-2')?.id ?? 'none'}</div>
108+
<div data-testid="for-session-unknown">{getForSession('session-unknown')?.id ?? 'none'}</div>
106109
<div data-testid="permission-count">{permissions.pendingCount}</div>
107110
<div data-testid="permission-current">{permissions.current?.id ?? 'none'}</div>
108111
<div data-testid="permission-call">{permissionForCall?.id ?? 'none'}</div>
@@ -239,6 +242,23 @@ describe('EventProvider questions', () => {
239242
})
240243
})
241244

245+
it('resolves pending questions per session independently of the global current', async () => {
246+
mocks.listPendingQuestions.mockResolvedValue([pendingQuestion, secondPendingQuestion])
247+
248+
render(<Harness />, { wrapper: createWrapper() })
249+
250+
await userEvent.click(screen.getByRole('button', { name: 'Sync' }))
251+
252+
await waitFor(() => {
253+
expect(screen.getByTestId('count')).toHaveTextContent('2')
254+
})
255+
256+
expect(screen.getByTestId('current')).toHaveTextContent('question-1')
257+
expect(screen.getByTestId('for-session-1')).toHaveTextContent('question-1')
258+
expect(screen.getByTestId('for-session-2')).toHaveTextContent('question-2')
259+
expect(screen.getByTestId('for-session-unknown')).toHaveTextContent('none')
260+
})
261+
242262
it('reconciles stale pending questions after reconnect', async () => {
243263
mocks.listRepos.mockResolvedValue([{ id: 123, fullPath: '/repo' }])
244264
mocks.listPendingQuestions

frontend/src/contexts/EventContext.tsx

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -158,6 +158,7 @@ interface EventContextValue {
158158
reject: (requestID: string) => Promise<void>
159159
dismiss: (requestID: string, sessionID?: string) => void
160160
getForCallID: (callID: string, sessionID: string) => QuestionRequest | null
161+
getForSession: (sessionID: string) => QuestionRequest | null
161162
hasForSession: (sessionID: string) => boolean
162163
navigateToCurrent: () => void
163164
syncForSession: (directory: string, sessionID: string) => Promise<void>
@@ -404,6 +405,10 @@ export function EventProvider({ children }: { children: React.ReactNode }) {
404405
return (permissionsBySession[sessionID]?.length ?? 0) > 0
405406
}, [permissionsBySession])
406407

408+
const getQuestionForSession = useCallback((sessionID: string): QuestionRequest | null => {
409+
return questionsBySession[sessionID]?.[0] ?? null
410+
}, [questionsBySession])
411+
407412
const hasQuestionsForSession = useCallback((sessionID: string): boolean => {
408413
return (questionsBySession[sessionID]?.length ?? 0) > 0
409414
}, [questionsBySession])
@@ -597,6 +602,7 @@ export function EventProvider({ children }: { children: React.ReactNode }) {
597602
reject: rejectQuestion,
598603
dismiss: removeQuestion,
599604
getForCallID: getQuestionForCallID,
605+
getForSession: getQuestionForSession,
600606
hasForSession: hasQuestionsForSession,
601607
navigateToCurrent: navigateToCurrentQuestion,
602608
syncForSession: syncQuestionsForSession,
@@ -622,6 +628,7 @@ export function EventProvider({ children }: { children: React.ReactNode }) {
622628
rejectQuestion,
623629
removeQuestion,
624630
getQuestionForCallID,
631+
getQuestionForSession,
625632
hasQuestionsForSession,
626633
navigateToCurrentQuestion,
627634
syncQuestionsForSession,

frontend/src/pages/SessionDetail.tsx

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -173,7 +173,8 @@ export function SessionDetail() {
173173
const { isEnabled: ttsEnabled } = useTTS();
174174
const sessionStatus = useSessionStatusForSession(sessionId);
175175
const { syncForSession: syncPermissionsForSession } = usePermissions();
176-
const { current: currentQuestion, reply: replyToQuestion, reject: rejectQuestion, syncForSession: syncQuestionsForSession } = useQuestions();
176+
const { getForSession: getQuestionForSession, reply: replyToQuestion, reject: rejectQuestion, syncForSession: syncQuestionsForSession } = useQuestions();
177+
const currentQuestion = sessionId ? getQuestionForSession(sessionId) : null;
177178

178179
const lastAssistantMessage = messages?.filter(m => m.info.role === 'assistant').at(-1);
179180
const lastAssistantText = getAssistantText(lastAssistantMessage);
@@ -568,7 +569,7 @@ export function SessionDetail() {
568569
onDismiss={() => rejectQuestion(minimizedQuestion.id)}
569570
/>
570571
)}
571-
{!minimizedQuestion && currentQuestion && currentQuestion.sessionID === sessionId && (
572+
{!minimizedQuestion && currentQuestion && (
572573
<QuestionPrompt
573574
key={currentQuestion.id}
574575
question={currentQuestion}

frontend/src/pages/__tests__/SessionDetail.assistant-loading.test.tsx

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -185,6 +185,7 @@ describe('SessionDetail assistant loading at repoId=0', () => {
185185
})
186186
mocks.useQuestions.mockReturnValue({
187187
current: null,
188+
getForSession: vi.fn(() => null),
188189
pendingCount: 0,
189190
hasQuestionsForSession: vi.fn(() => false),
190191
reply: vi.fn(),

frontend/src/pages/__tests__/SessionDetail.polling.test.tsx

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -164,6 +164,7 @@ describe('SessionDetail pending-actions polling gating', () => {
164164
})
165165
mocks.useQuestions.mockReturnValue({
166166
current: null,
167+
getForSession: vi.fn(() => null),
167168
pendingCount: 0,
168169
hasQuestionsForSession: vi.fn(() => false),
169170
reply: vi.fn(),
Lines changed: 278 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,278 @@
1+
import { describe, it, expect, vi, beforeEach } from 'vitest'
2+
import { render, screen, waitFor } from '@testing-library/react'
3+
import { QueryClient, QueryClientProvider } from '@tanstack/react-query'
4+
import { MemoryRouter, Route, Routes } from 'react-router-dom'
5+
import type { QuestionRequest } from '@/api/types'
6+
import { SessionDetail } from '../SessionDetail'
7+
8+
const mocks = vi.hoisted(() => ({
9+
useSession: vi.fn(),
10+
useMessages: vi.fn(),
11+
useSSE: vi.fn(),
12+
useRepoActivity: vi.fn(),
13+
usePermissions: vi.fn(),
14+
useQuestions: vi.fn(),
15+
useSSEHealth: vi.fn(),
16+
useConfig: vi.fn(),
17+
useOpenCodeClient: vi.fn(),
18+
useMobile: vi.fn(),
19+
useAutoScroll: vi.fn(),
20+
useDialogParam: vi.fn(),
21+
useSidebarAction: vi.fn(),
22+
useSessionStatusForSession: vi.fn(),
23+
}))
24+
25+
vi.mock('@/config', () => ({
26+
OPENCODE_API_ENDPOINT: 'http://localhost:5551/api/opencode',
27+
API_BASE_URL: 'http://localhost:5551',
28+
SERVER_PORT: 5003,
29+
OPENCODE_PORT: 5551,
30+
FILE_LIMITS: {},
31+
DEFAULTS: {},
32+
ALLOWED_MIME_TYPES: [],
33+
GIT_PROVIDERS: [],
34+
}))
35+
36+
vi.mock('@/hooks/useOpenCode', () => ({
37+
useSession: mocks.useSession,
38+
useAbortSession: vi.fn(() => ({ mutate: vi.fn() })),
39+
useUpdateSession: vi.fn(() => ({ mutate: vi.fn() })),
40+
useCreateSession: vi.fn(() => ({ mutateAsync: vi.fn() })),
41+
useMessages: mocks.useMessages,
42+
useConfig: mocks.useConfig,
43+
useSendPrompt: vi.fn(() => ({ mutate: vi.fn() })),
44+
useSendShell: vi.fn(() => ({ mutate: vi.fn() })),
45+
useAgents: vi.fn(() => ({ data: [] })),
46+
useOpenCodeClient: mocks.useOpenCodeClient,
47+
}))
48+
49+
vi.mock('@/hooks/useModelSelection', () => ({
50+
useModelSelection: vi.fn(() => ({ model: null, modelString: null })),
51+
}))
52+
53+
vi.mock('@/hooks/useTTS', () => ({
54+
useTTS: vi.fn(() => ({ isEnabled: false })),
55+
}))
56+
57+
vi.mock('@/hooks/useSettings', () => ({
58+
useSettings: vi.fn(() => ({
59+
preferences: { expandToolCalls: false },
60+
updateSettings: vi.fn(),
61+
})),
62+
}))
63+
64+
vi.mock('@/hooks/useSettingsDialog', () => ({
65+
useSettingsDialog: vi.fn(() => ({ open: vi.fn() })),
66+
}))
67+
68+
vi.mock('@/hooks/useMobile', () => ({
69+
useMobile: mocks.useMobile,
70+
useSwipeBack: vi.fn(() => ({ ref: vi.fn() })),
71+
}))
72+
73+
vi.mock('@/hooks/useVisualViewport', () => ({
74+
useVisualViewport: vi.fn(() => ({ keyboardHeight: 0 })),
75+
}))
76+
77+
vi.mock('@/hooks/useKeyboardShortcuts', () => ({
78+
useKeyboardShortcuts: vi.fn(() => ({ leaderActive: false })),
79+
}))
80+
81+
vi.mock('@/hooks/useAutoScroll', () => ({
82+
useAutoScroll: mocks.useAutoScroll,
83+
}))
84+
85+
vi.mock('@/hooks/useDialogParam', () => ({
86+
useDialogParam: vi.fn(() => [false, vi.fn()]),
87+
}))
88+
89+
vi.mock('@/hooks/useSidebarAction', () => ({
90+
useSidebarAction: vi.fn(() => {}),
91+
}))
92+
93+
vi.mock('@/hooks/useAutoPlayLastResponse', () => ({
94+
getAssistantText: vi.fn(() => ''),
95+
getLatestPlayableAssistantMessage: vi.fn(() => null),
96+
useAutoPlayLastResponse: vi.fn(() => {}),
97+
}))
98+
99+
vi.mock('@/stores/uiStateStore', () => ({
100+
useUIState: vi.fn((selector?: (state: Record<string, unknown>) => unknown) =>
101+
typeof selector === 'function'
102+
? selector({ isEditingMessage: false, setActivePromptFileBasePath: vi.fn() })
103+
: false
104+
),
105+
}))
106+
107+
vi.mock('@/stores/sessionStatusStore', () => ({
108+
useSessionStatus: vi.fn(() => ({ setStatus: vi.fn() })),
109+
useSessionStatusForSession: mocks.useSessionStatusForSession,
110+
}))
111+
112+
vi.mock('@/hooks/useSSE', () => ({
113+
useSSE: mocks.useSSE,
114+
}))
115+
116+
vi.mock('@/hooks/useRepoActivity', () => ({
117+
useRepoActivity: mocks.useRepoActivity,
118+
}))
119+
120+
vi.mock('@/contexts/EventContext', async (importOriginal) => {
121+
const actual = await importOriginal()
122+
return {
123+
...(actual as object),
124+
usePermissions: mocks.usePermissions,
125+
useQuestions: mocks.useQuestions,
126+
useSSEHealth: mocks.useSSEHealth,
127+
}
128+
})
129+
130+
vi.mock('@/api/repos', () => ({
131+
getRepo: vi.fn(() => Promise.resolve({
132+
id: 1,
133+
repoUrl: 'https://github.com/test/repo',
134+
localPath: '/test/repo',
135+
sourcePath: null,
136+
fullPath: '/test/repo',
137+
branch: 'main',
138+
currentBranch: 'main',
139+
fullSlug: 'test/repo',
140+
repoType: 'github' as const,
141+
})),
142+
initializeAssistantMode: vi.fn(() => Promise.resolve({ directory: '/test/repo' })),
143+
}))
144+
145+
vi.mock('@/components/model/ModelSelectDialog', () => ({
146+
ModelSelectDialog: vi.fn(() => null),
147+
}))
148+
149+
vi.mock('@/components/session/SessionList', () => ({
150+
SessionList: vi.fn(() => null),
151+
}))
152+
153+
vi.mock('@/components/file-browser/FileBrowserSheet', () => ({
154+
FileBrowserSheet: vi.fn(() => null),
155+
}))
156+
157+
vi.mock('@/components/repo/RepoMcpDialog', () => ({
158+
RepoMcpDialog: vi.fn(() => null),
159+
}))
160+
161+
vi.mock('@/components/repo/ResetPermissionsDialog', () => ({
162+
ResetPermissionsDialog: vi.fn(() => null),
163+
}))
164+
165+
vi.mock('@/components/repo/RepoLspDialog', () => ({
166+
RepoLspDialog: vi.fn(() => null),
167+
}))
168+
169+
vi.mock('@/components/repo/RepoSkillsDialog', () => ({
170+
RepoSkillsDialog: vi.fn(() => null),
171+
}))
172+
173+
vi.mock('@/components/source-control', () => ({
174+
SourceControlPanel: vi.fn(() => null),
175+
}))
176+
177+
vi.mock('@/components/session/QuestionPrompt', () => ({
178+
QuestionPrompt: ({ question }: { question: QuestionRequest }) => (
179+
<div data-testid="question-prompt">{question.id}</div>
180+
),
181+
}))
182+
183+
vi.mock('@/components/session/MinimizedQuestionIndicator', () => ({
184+
MinimizedQuestionIndicator: vi.fn(() => null),
185+
}))
186+
187+
vi.mock('@/components/notifications/PendingActionsGroup', () => ({
188+
PendingActionsGroup: vi.fn(() => null),
189+
}))
190+
191+
vi.mock('@/components/message/PromptInput', () => ({
192+
PromptInput: vi.fn(() => <div>MockedPromptInput</div>),
193+
}))
194+
195+
const VIEWED_SESSION_ID = 'viewed-session'
196+
197+
function createQuestion(id: string, sessionID: string): QuestionRequest {
198+
return {
199+
id,
200+
sessionID,
201+
questions: [
202+
{
203+
question: 'Continue?',
204+
header: 'Confirm',
205+
options: [{ label: 'Yes', description: 'Continue' }],
206+
multiple: false,
207+
},
208+
],
209+
}
210+
}
211+
212+
const viewedSessionQuestion = createQuestion('question-viewed', VIEWED_SESSION_ID)
213+
const otherSessionQuestion = createQuestion('question-other', 'other-session')
214+
215+
describe('SessionDetail question prompt session scoping', () => {
216+
beforeEach(() => {
217+
vi.clearAllMocks()
218+
219+
mocks.useSession.mockReturnValue({ data: undefined, isLoading: false })
220+
mocks.useMessages.mockReturnValue({ data: [], isLoading: false })
221+
mocks.useSSE.mockReturnValue({ isConnected: true, isReconnecting: false })
222+
mocks.useRepoActivity.mockReturnValue(undefined)
223+
mocks.usePermissions.mockReturnValue({
224+
pendingCount: 0,
225+
syncForSession: vi.fn(),
226+
})
227+
mocks.useSSEHealth.mockReturnValue({ isHealthy: true })
228+
mocks.useConfig.mockReturnValue({ data: undefined, isLoading: false })
229+
mocks.useOpenCodeClient.mockReturnValue({})
230+
mocks.useMobile.mockReturnValue(false)
231+
mocks.useAutoScroll.mockReturnValue({ scrollToBottom: vi.fn() })
232+
mocks.useDialogParam.mockReturnValue([false, vi.fn()])
233+
mocks.useSidebarAction.mockReturnValue(undefined)
234+
mocks.useSessionStatusForSession.mockReturnValue({ type: 'idle' })
235+
})
236+
237+
const renderWithQuestions = (questionsBySession: Record<string, QuestionRequest>, current: QuestionRequest | null) => {
238+
mocks.useQuestions.mockReturnValue({
239+
current,
240+
getForSession: vi.fn((sessionID: string) => questionsBySession[sessionID] ?? null),
241+
pendingCount: Object.keys(questionsBySession).length,
242+
reply: vi.fn(),
243+
reject: vi.fn(),
244+
syncForSession: vi.fn(),
245+
})
246+
247+
return render(
248+
<MemoryRouter initialEntries={[`/repos/1/sessions/${VIEWED_SESSION_ID}`]}>
249+
<QueryClientProvider client={new QueryClient({ defaultOptions: { queries: { retry: false } } })}>
250+
<Routes>
251+
<Route path="/repos/:id/sessions/:sessionId" element={<SessionDetail />} />
252+
</Routes>
253+
</QueryClientProvider>
254+
</MemoryRouter>
255+
)
256+
}
257+
258+
it('renders the viewed session question when another session owns the globally current question', async () => {
259+
renderWithQuestions(
260+
{
261+
[VIEWED_SESSION_ID]: viewedSessionQuestion,
262+
'other-session': otherSessionQuestion,
263+
},
264+
otherSessionQuestion
265+
)
266+
267+
await waitFor(() => {
268+
expect(screen.getByTestId('question-prompt')).toHaveTextContent('question-viewed')
269+
})
270+
})
271+
272+
it('renders no question prompt when only another session has a pending question', async () => {
273+
renderWithQuestions({ 'other-session': otherSessionQuestion }, otherSessionQuestion)
274+
275+
await waitFor(() => expect(screen.getByText('MockedPromptInput')).toBeInTheDocument())
276+
expect(screen.queryByTestId('question-prompt')).not.toBeInTheDocument()
277+
})
278+
})

frontend/src/pages/__tests__/SessionDetail.scroll-floating.test.tsx

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -214,6 +214,7 @@ describe('SessionDetail scroll floating button', () => {
214214
})
215215
mocks.useQuestions.mockReturnValue({
216216
current: null,
217+
getForSession: vi.fn(() => null),
217218
pendingCount: 0,
218219
hasQuestionsForSession: vi.fn(() => false),
219220
reply: vi.fn(),

0 commit comments

Comments
 (0)