diff --git a/frontend/src/components/session/ChangesWalkthroughSheet.test.tsx b/frontend/src/components/session/ChangesWalkthroughSheet.test.tsx index 221a26cf8..9b58ef338 100644 --- a/frontend/src/components/session/ChangesWalkthroughSheet.test.tsx +++ b/frontend/src/components/session/ChangesWalkthroughSheet.test.tsx @@ -73,13 +73,14 @@ describe('ChangesWalkthroughSheet', () => { Element.prototype.setPointerCapture ??= () => {} Element.prototype.releasePointerCapture ??= () => {} Element.prototype.scrollIntoView ??= () => {} + Element.prototype.scrollTo ??= () => {} }) beforeEach(() => { vi.clearAllMocks() }) - it('generates a walkthrough and renders the summary and first stop', async () => { + it('generates a walkthrough and opens on the overview page', async () => { const user = userEvent.setup() let generated = false mocks.getChangeWalkthrough.mockImplementation(async () => @@ -98,33 +99,73 @@ describe('ChangesWalkthroughSheet', () => { expect(mocks.generateChangeWalkthrough).toHaveBeenCalledWith('ses_1', {}) }) expect(await screen.findByText('This change adds a greeting.')).toBeInTheDocument() - expect(screen.getByText('Stop 1 of 2')).toBeInTheDocument() - expect(screen.getByText('src/greet.ts')).toBeInTheDocument() - expect(screen.getByText('const a = 2')).toBeInTheDocument() - expect(screen.queryByText('src/main.ts')).not.toBeInTheDocument() + expect(screen.getByText('Overview')).toBeInTheDocument() + expect(screen.getByRole('button', { name: '1. Add the greeting' })).toBeInTheDocument() + expect(screen.getByRole('button', { name: '2. Wire it up' })).toBeInTheDocument() + expect(screen.queryByText('src/greet.ts')).not.toBeInTheDocument() expect(screen.getByText('assets/logo.png — binary file')).toBeInTheDocument() }) - it('moves through the stops with the navigator in order', async () => { + it('opens a stop from the overview list without the summary or list', async () => { const user = userEvent.setup() mocks.getChangeWalkthrough.mockResolvedValue(state()) renderSheet() - expect(await screen.findByText('Stop 1 of 2')).toBeInTheDocument() - expect(screen.getByText('src/greet.ts')).toBeInTheDocument() + await user.click(await screen.findByRole('button', { name: '2. Wire it up' })) + + expect(await screen.findByText('2 of 2')).toBeInTheDocument() + expect(screen.getByText('src/main.ts')).toBeInTheDocument() + expect(screen.queryByText('This change adds a greeting.')).not.toBeInTheDocument() + expect(screen.queryByRole('button', { name: '1. Add the greeting' })).not.toBeInTheDocument() + }) + + it('jumps between pages from the header dropdown', async () => { + const user = userEvent.setup() + mocks.getChangeWalkthrough.mockResolvedValue(state()) + renderSheet() + + await user.click(await screen.findByRole('button', { name: '1. Add the greeting' })) + expect(await screen.findByText('1 of 2')).toBeInTheDocument() + + await user.click(screen.getByRole('button', { name: 'Jump to page' })) + await user.click(await screen.findByRole('menuitemradio', { name: '2. Wire it up' })) + expect(await screen.findByText('2 of 2')).toBeInTheDocument() + expect(screen.getByText('src/main.ts')).toBeInTheDocument() + + await user.click(screen.getByRole('button', { name: 'Jump to page' })) + await user.click(await screen.findByRole('menuitemradio', { name: 'Overview' })) + expect(await screen.findByText('This change adds a greeting.')).toBeInTheDocument() + }) + + it('moves through the overview and stops with the navigator in order', async () => { + const user = userEvent.setup() + mocks.getChangeWalkthrough.mockResolvedValue(state()) + renderSheet() + + expect(await screen.findByText('Overview')).toBeInTheDocument() expect(screen.getByRole('button', { name: /previous/i })).toBeDisabled() await user.click(screen.getByRole('button', { name: /next/i })) - expect(await screen.findByText('Stop 2 of 2')).toBeInTheDocument() + expect(await screen.findByText('1 of 2')).toBeInTheDocument() + expect(screen.getByText('src/greet.ts')).toBeInTheDocument() + expect(screen.getByText('const a = 2')).toBeInTheDocument() + expect(screen.queryByText('This change adds a greeting.')).not.toBeInTheDocument() + + await user.click(screen.getByRole('button', { name: /next/i })) + + expect(await screen.findByText('2 of 2')).toBeInTheDocument() expect(screen.getByText('src/main.ts')).toBeInTheDocument() expect(screen.queryByText('src/greet.ts')).not.toBeInTheDocument() expect(screen.getByText('truncated')).toBeInTheDocument() expect(screen.getByRole('button', { name: /next/i })).toBeDisabled() await user.click(screen.getByRole('button', { name: /previous/i })) + expect(await screen.findByText('1 of 2')).toBeInTheDocument() - expect(await screen.findByText('Stop 1 of 2')).toBeInTheDocument() + await user.click(screen.getByRole('button', { name: /previous/i })) + expect(await screen.findByText('Overview')).toBeInTheDocument() + expect(screen.getByText('This change adds a greeting.')).toBeInTheDocument() }) it('shows the stale warning and regenerates', async () => { @@ -150,7 +191,7 @@ describe('ChangesWalkthroughSheet', () => { mocks.generateChangeWalkthrough.mockResolvedValue(state()) renderSheet() - await screen.findByText('Stop 1 of 2') + await screen.findByText('Overview') await user.click(screen.getByRole('button', { name: 'Regenerate walkthrough' })) await waitFor(() => { @@ -158,6 +199,27 @@ describe('ChangesWalkthroughSheet', () => { }) }) + it('scrolls back to the top of the overview when a walkthrough is regenerated', async () => { + const user = userEvent.setup() + const scrollTo = vi.spyOn(Element.prototype, 'scrollTo') + let createdAt = 1 + mocks.getChangeWalkthrough.mockImplementation(async () => state({ walkthrough: { ...walkthrough, createdAt } })) + mocks.generateChangeWalkthrough.mockImplementation(async () => { + createdAt = 2 + return state({ walkthrough: { ...walkthrough, createdAt } }) + }) + renderSheet() + + await user.click(await screen.findByRole('button', { name: '1. Add the greeting' })) + expect(await screen.findByText('1 of 2')).toBeInTheDocument() + + scrollTo.mockClear() + await user.click(screen.getByRole('button', { name: 'Regenerate walkthrough' })) + + await waitFor(() => expect(scrollTo).toHaveBeenCalledWith({ top: 0 })) + expect(await screen.findByText('Overview')).toBeInTheDocument() + }) + it('shows progress while the server is generating instead of the generate button', async () => { mocks.getChangeWalkthrough.mockResolvedValue(state({ walkthrough: null, generating: true })) renderSheet() @@ -237,9 +299,12 @@ describe('ChangesWalkthroughSheet', () => { }, }), ) + const user = userEvent.setup() const { container } = renderSheet() expect(await screen.findByText(/Summary/)).toBeInTheDocument() + expect(container.querySelector('iframe')).toBeNull() + await user.click(screen.getByRole('button', { name: /next/i })) expect(await screen.findByText(/Explanation/)).toBeInTheDocument() expect(container.querySelector('iframe')).toBeNull() expect(container.querySelector('script')).toBeNull() diff --git a/frontend/src/components/session/ChangesWalkthroughSheet.tsx b/frontend/src/components/session/ChangesWalkthroughSheet.tsx index 44ac695ba..ab1abf1a6 100644 --- a/frontend/src/components/session/ChangesWalkthroughSheet.tsx +++ b/frontend/src/components/session/ChangesWalkthroughSheet.tsx @@ -1,14 +1,21 @@ import { createContext, memo, useCallback, useContext, useEffect, useRef, useState } from 'react' import type { ReactNode, RefObject } from 'react' -import { ChevronLeft, ChevronRight, Loader2, RefreshCw } from 'lucide-react' +import { ChevronDown, ChevronLeft, ChevronRight, Loader2, RefreshCw } from 'lucide-react' import { SideDrawer, SideDrawerHeader } from '@/components/ui/side-drawer' import { Button } from '@/components/ui/button' +import { + DropdownMenu, + DropdownMenuContent, + DropdownMenuRadioGroup, + DropdownMenuRadioItem, + DropdownMenuSeparator, + DropdownMenuTrigger, +} from '@/components/ui/dropdown-menu' import { Badge } from '@/components/ui/badge' import { DiffLines } from '@/components/file-browser/DiffLines' import { ScheduleRunMarkdown } from '@/components/schedules/ScheduleRunMarkdown' import { useChangeWalkthrough, useGenerateChangeWalkthrough } from '@/hooks/useChangeWalkthrough' import { GIT_STATUS_COLORS, GIT_STATUS_LABELS } from '@/lib/git-status-styles' -import { cn } from '@/lib/utils' import { WalkthroughOmittedFileSchema } from '@opencode-manager/shared/schemas' import type { ChangeWalkthrough, @@ -80,11 +87,11 @@ interface ChangesWalkthroughContextValue { generate: () => void regenerate: () => void stops: WalkthroughStop[] - stopIndex: number + stopIndex: number | null selectedStop: WalkthroughStop | null selectedHunks: WalkthroughHunk[] - selectStop: (index: number) => void - detailRef: RefObject + selectStop: (index: number | null) => void + scrollRef: RefObject } const ChangesWalkthroughContext = createContext(null) @@ -106,8 +113,8 @@ export function ChangesWalkthroughProvider({ sessionId, active, children }: Chan const stateQuery = useChangeWalkthrough(sessionId, active) const generate = useGenerateChangeWalkthrough(sessionId) const resetGenerate = generate.reset - const [stopIndex, setStopIndex] = useState(0) - const detailRef = useRef(null) + const [stopIndex, setStopIndex] = useState(null) + const scrollRef = useRef(null) const state = stateQuery.data const walkthrough = state?.walkthrough ?? null @@ -121,20 +128,21 @@ export function ChangesWalkthroughProvider({ sessionId, active, children }: Chan }, [active, sessionId, resetGenerate]) useEffect(() => { - setStopIndex(0) + setStopIndex(null) + scrollRef.current?.scrollTo?.({ top: 0 }) }, [active, sessionId, walkthrough?.createdAt]) const hunksById = new Map(walkthrough?.hunks.map((hunk) => [hunk.id, hunk]) ?? []) const stops = walkthrough?.stops ?? [] - const clampedIndex = stops.length > 0 ? Math.min(stopIndex, stops.length - 1) : 0 - const selectedStop = stops[clampedIndex] ?? null + const clampedIndex = stopIndex === null || stops.length === 0 ? null : Math.min(stopIndex, stops.length - 1) + const selectedStop = clampedIndex === null ? null : stops[clampedIndex] ?? null const selectedHunks = (selectedStop?.hunkIds ?? []) .map((id) => hunksById.get(id)) .filter((hunk): hunk is WalkthroughHunk => hunk !== undefined) - const selectStop = useCallback((index: number) => { + const selectStop = useCallback((index: number | null) => { setStopIndex(index) - detailRef.current?.scrollIntoView?.({ block: 'start', behavior: 'smooth' }) + scrollRef.current?.scrollTo?.({ top: 0 }) }, []) const value: ChangesWalkthroughContextValue = { @@ -151,13 +159,13 @@ export function ChangesWalkthroughProvider({ sessionId, active, children }: Chan selectedStop, selectedHunks, selectStop, - detailRef, + scrollRef, } return {children} } -/** Previous/next stop controls for the walkthrough chrome; renders nothing until a walkthrough has stops. */ +/** Previous/next page controls (overview, then each stop) for the walkthrough chrome; renders nothing until a walkthrough has stops. */ export const ChangesWalkthroughNav = memo(function ChangesWalkthroughNav() { const { stops, stopIndex, selectStop } = useChangesWalkthrough() @@ -169,20 +177,39 @@ export const ChangesWalkthroughNav = memo(function ChangesWalkthroughNav() { variant="outline" size="icon-sm" aria-label="Previous stop" - onClick={() => selectStop(stopIndex - 1)} - disabled={stopIndex === 0} + onClick={() => selectStop(stopIndex === null || stopIndex === 0 ? null : stopIndex - 1)} + disabled={stopIndex === null} > - - Stop {stopIndex + 1} of {stops.length} - + + + + + + selectStop(value === 'overview' ? null : Number(value))} + > + Overview + + {stops.map((stop, index) => ( + + {index + 1}. {stop.title} + + ))} + + + @@ -221,15 +248,14 @@ export const ChangesWalkthroughView = memo(function ChangesWalkthroughView() { generate, regenerate, stops, - stopIndex, selectedStop, selectedHunks, selectStop, - detailRef, + scrollRef, } = useChangesWalkthrough() return ( -
+
{error ? (

{walkthroughErrorMessage(error)}

@@ -271,35 +297,9 @@ export const ChangesWalkthroughView = memo(function ChangesWalkthroughView() {
) : null} - - - {stops.length > 0 ? ( -
    - {stops.map((stop, index) => ( -
  1. - -
  2. - ))} -
- ) : null} - {selectedStop ? ( -
-

- {stopIndex + 1}. {selectedStop.title} -

+
+

{selectedStop.title}

{selectedHunks.map((hunk) => (
@@ -318,9 +318,29 @@ export const ChangesWalkthroughView = memo(function ChangesWalkthroughView() {
))}
- ) : null} - - {walkthrough.omittedFiles.length > 0 ? : null} + ) : ( + <> + + + {stops.length > 0 ? ( +
    + {stops.map((stop, index) => ( +
  1. + +
  2. + ))} +
+ ) : null} + + {walkthrough.omittedFiles.length > 0 ? : null} + + )}
)}