From fac4e1ba1aafacd881ad369aed86070b9255cd69 Mon Sep 17 00:00:00 2001 From: Antti Kettunen Date: Sat, 2 May 2026 16:37:21 +0300 Subject: [PATCH] Sync issues routing and shell URLs - Move issue detail selection into route search so the modal is deep-linkable and back-button friendly. - Normalize issue category and detail params before they reach the loader. - Keep the legacy shell URL in sync for React-owned home pages. - Preserve the legacy issues-tour hooks on the React issues page. - Add Escape handling, focus trapping, and focus restore to the issue detail modal. - Add route and helper coverage for the new search-state behavior. --- .../src/routes/issues/-issues.helpers.test.ts | 35 +++++++ webui/src/routes/issues/-issues.helpers.ts | 26 ++++- webui/src/routes/issues/-issues.types.ts | 1 + webui/src/routes/issues/-route.test.tsx | 46 ++++++++- .../routes/issues/-ui/issue-detail-modal.tsx | 96 +++++++++++++++++- webui/src/routes/issues/-ui/issues-page.tsx | 99 ++++++++++++------- webui/src/routes/issues/route.tsx | 10 +- webui/static/search.js | 5 +- 8 files changed, 268 insertions(+), 50 deletions(-) create mode 100644 webui/src/routes/issues/-issues.helpers.test.ts diff --git a/webui/src/routes/issues/-issues.helpers.test.ts b/webui/src/routes/issues/-issues.helpers.test.ts new file mode 100644 index 00000000..9001b470 --- /dev/null +++ b/webui/src/routes/issues/-issues.helpers.test.ts @@ -0,0 +1,35 @@ +import { describe, expect, it } from 'vite-plus/test'; + +import { normalizeIssuesSearch } from './-issues.helpers'; + +describe('normalizeIssuesSearch', () => { + it('falls back to all for unknown categories', () => { + expect(normalizeIssuesSearch({ category: 'not_real' })).toEqual({ + status: 'open', + category: 'all', + }); + }); + + it('preserves known categories', () => { + expect(normalizeIssuesSearch({ category: 'wrong_metadata' })).toEqual({ + status: 'open', + category: 'wrong_metadata', + }); + }); + + it('drops invalid issue ids', () => { + expect(normalizeIssuesSearch({ issueId: 'abc123' })).toEqual({ + status: 'open', + category: 'all', + issueId: undefined, + }); + }); + + it('normalizes numeric issue ids', () => { + expect(normalizeIssuesSearch({ issueId: '7' })).toEqual({ + status: 'open', + category: 'all', + issueId: 7, + }); + }); +}); diff --git a/webui/src/routes/issues/-issues.helpers.ts b/webui/src/routes/issues/-issues.helpers.ts index 762916f0..4dec9db2 100644 --- a/webui/src/routes/issues/-issues.helpers.ts +++ b/webui/src/routes/issues/-issues.helpers.ts @@ -89,6 +89,12 @@ export const ISSUE_CATEGORY_META: Record< }, }; +const ISSUE_CATEGORY_KEYS = new Set(Object.keys(ISSUE_CATEGORY_META)); + +export type NormalizedIssuesSearch = Required> & { + issueId?: number; +}; + export const ISSUE_STATUS_META: Record = { open: { label: 'Open', className: 'is-open' }, in_progress: { label: 'In Progress', className: 'is-progress' }, @@ -113,9 +119,10 @@ export function createDefaultIssueTitle(category: string, entityName: string): s return `${label}: ${entityName || 'Unknown'}`; } -export function normalizeIssuesSearch(search: IssuesSearch | undefined): Required { +export function normalizeIssuesSearch(search: IssuesSearch | undefined): NormalizedIssuesSearch { const status = search?.status; const category = search?.category; + const issueId = search?.issueId; return { status: @@ -126,7 +133,15 @@ export function normalizeIssuesSearch(search: IssuesSearch | undefined): Require status === 'dismissed' ? status : DEFAULT_ISSUES_SEARCH.status, - category: typeof category === 'string' && category.length > 0 ? category : 'all', + category: typeof category === 'string' && ISSUE_CATEGORY_KEYS.has(category) + ? category + : 'all', + issueId: + (typeof issueId === 'number' && Number.isInteger(issueId) && issueId > 0) + ? issueId + : (typeof issueId === 'string' && /^[1-9]\d*$/.test(issueId) + ? Number(issueId) + : undefined), }; } @@ -144,7 +159,7 @@ export async function fetchIssueCounts(profileId: number): Promise export async function fetchIssueList( profileId: number, - search: Required, + search: Pick, ): Promise { const params = new URLSearchParams(); params.set('limit', String(DEFAULT_LIMIT)); @@ -240,7 +255,10 @@ export function issueCountsQueryOptions(profileId: number) { }); } -export function issueListQueryOptions(profileId: number, search: Required) { +export function issueListQueryOptions( + profileId: number, + search: Pick, +) { return queryOptions({ queryKey: ['issues', 'list', profileId, search.status, search.category], queryFn: () => fetchIssueList(profileId, search), diff --git a/webui/src/routes/issues/-issues.types.ts b/webui/src/routes/issues/-issues.types.ts index 9c315521..53a6c1b6 100644 --- a/webui/src/routes/issues/-issues.types.ts +++ b/webui/src/routes/issues/-issues.types.ts @@ -84,6 +84,7 @@ export interface IssueCountsResponse { export interface IssuesSearch { category?: string; + issueId?: string | number; status?: IssueStatus | 'all'; } diff --git a/webui/src/routes/issues/-route.test.tsx b/webui/src/routes/issues/-route.test.tsx index 3257c3f3..4cb6d42f 100644 --- a/webui/src/routes/issues/-route.test.tsx +++ b/webui/src/routes/issues/-route.test.tsx @@ -28,12 +28,16 @@ function createShellBridge(overrides: Partial = {}): ShellBridge { }; } -function renderIssuesRoute() { +function renderIssuesRoute(initialEntries = ['/issues']) { const queryClient = createAppQueryClient(); - const history = createMemoryHistory({ initialEntries: ['/issues'] }); + const history = createMemoryHistory({ initialEntries }); const router = createAppRouter({ history, queryClient }); - return render(); + return { + history, + router, + ...render(), + }; } const workflowActions = { @@ -136,6 +140,11 @@ describe('issues route', () => { expect(await screen.findByTestId('issue-card-7')).toHaveTextContent('Bad tags'); }); + it('loads the detail modal from the route search state', async () => { + renderIssuesRoute(['/issues?issueId=7']); + await waitFor(() => expect(screen.getByRole('dialog')).toHaveTextContent('Issue #7')); + }); + it('stores filters in route search state', async () => { renderIssuesRoute(); const status = await screen.findByRole('combobox', { name: /status/i }); @@ -144,11 +153,40 @@ describe('issues route', () => { }); it('opens and closes the detail modal', async () => { - renderIssuesRoute(); + const { history } = renderIssuesRoute(); fireEvent.click(await screen.findByTestId('issue-card-7')); await waitFor(() => expect(screen.getByRole('dialog')).toHaveTextContent('Issue #7')); + await waitFor(() => expect(history.location.search).toContain('issueId=7')); fireEvent.click(screen.getByRole('button', { name: /close issue detail/i })); await waitFor(() => expect(screen.queryByRole('dialog')).not.toBeInTheDocument()); + await waitFor(() => expect(history.location.search).toBe('?status=open&category=all')); + }); + + it('closes the detail modal with Escape', async () => { + const { history } = renderIssuesRoute(); + fireEvent.click(await screen.findByTestId('issue-card-7')); + await waitFor(() => expect(screen.getByRole('dialog')).toHaveTextContent('Issue #7')); + await waitFor(() => expect(history.location.search).toContain('issueId=7')); + + fireEvent.keyDown(document, { key: 'Escape' }); + + await waitFor(() => expect(screen.queryByRole('dialog')).not.toBeInTheDocument()); + await waitFor(() => expect(history.location.search).toBe('?status=open&category=all')); + }); + + it('traps focus inside the detail modal', async () => { + renderIssuesRoute(); + fireEvent.click(await screen.findByTestId('issue-card-7')); + + const closeButton = await screen.findByRole('button', { name: /close issue detail/i }); + const deleteButton = await screen.findByRole('button', { name: /delete/i }); + + deleteButton.focus(); + expect(deleteButton).toHaveFocus(); + + fireEvent.keyDown(document, { key: 'Tab' }); + + expect(closeButton).toHaveFocus(); }); it('invokes the shared workflow adapter for admin downloads', async () => { diff --git a/webui/src/routes/issues/-ui/issue-detail-modal.tsx b/webui/src/routes/issues/-ui/issue-detail-modal.tsx index c311f863..ae8c5258 100644 --- a/webui/src/routes/issues/-ui/issue-detail-modal.tsx +++ b/webui/src/routes/issues/-ui/issue-detail-modal.tsx @@ -1,5 +1,5 @@ import { useMutation } from '@tanstack/react-query'; -import { useEffect, useMemo, useState } from 'react'; +import { useEffect, useMemo, useRef, useState } from 'react'; import { Button, FormField, TextArea } from '@/components/form'; import { @@ -40,11 +40,88 @@ export function IssueDetailModal({ profileId: number; }) { const [adminResponse, setAdminResponse] = useState(''); + const modalRef = useRef(null); + const previouslyFocusedElementRef = useRef(null); + const isOpen = Boolean(issue || isLoading || error); useEffect(() => { setAdminResponse(issue?.admin_response || ''); }, [issue?.admin_response, issue?.id]); + useEffect(() => { + if (!isOpen) { + return; + } + + previouslyFocusedElementRef.current = document.activeElement instanceof HTMLElement + ? document.activeElement + : null; + + const focusModal = () => { + const modal = modalRef.current; + if (!modal) return; + + const focusable = getFocusableElements(modal); + (focusable[0] || modal).focus(); + }; + + const handleKeyDown = (event: KeyboardEvent) => { + if (event.key === 'Escape') { + event.preventDefault(); + event.stopPropagation(); + onClose(); + return; + } + + if (event.key !== 'Tab') return; + + const modal = modalRef.current; + if (!modal) return; + + const focusable = getFocusableElements(modal); + if (focusable.length === 0) { + event.preventDefault(); + modal.focus(); + return; + } + + const first = focusable[0]; + const last = focusable[focusable.length - 1]; + const activeElement = document.activeElement; + + if (event.shiftKey) { + if (activeElement === first || !modal.contains(activeElement)) { + event.preventDefault(); + last.focus(); + } + } else if (activeElement === last) { + event.preventDefault(); + first.focus(); + } + }; + + const onFocusIn = (event: FocusEvent) => { + const modal = modalRef.current; + if (!modal) return; + if (event.target instanceof Node && !modal.contains(event.target)) { + const focusable = getFocusableElements(modal); + (focusable[0] || modal).focus(); + } + }; + + const raf = requestAnimationFrame(focusModal); + document.addEventListener('keydown', handleKeyDown); + document.addEventListener('focusin', onFocusIn); + + return () => { + cancelAnimationFrame(raf); + document.removeEventListener('keydown', handleKeyDown); + document.removeEventListener('focusin', onFocusIn); + previouslyFocusedElementRef.current?.focus?.(); + previouslyFocusedElementRef.current = null; + }; + }, [isOpen, onClose]); + const updateMutation = useMutation({ mutationFn: async (payload: { issueId: number; status: string; adminResponse: string }) => { await updateIssue(profileId, payload.issueId, { @@ -197,6 +274,8 @@ export function IssueDetailModal({ >
event.stopPropagation()} >
@@ -524,6 +603,21 @@ export function IssueDetailModal({ ); } +function getFocusableElements(container: HTMLElement) { + return Array.from( + container.querySelectorAll( + [ + 'button:not([disabled])', + '[href]', + 'input:not([disabled])', + 'select:not([disabled])', + 'textarea:not([disabled])', + '[tabindex]:not([tabindex="-1"])', + ].join(','), + ), + ).filter((element) => element.tabIndex >= 0); +} + function getStatusClassName(status: string) { if (status === 'in_progress') return styles.issueStatusProgress; if (status === 'resolved') return styles.issueStatusResolved; diff --git a/webui/src/routes/issues/-ui/issues-page.tsx b/webui/src/routes/issues/-ui/issues-page.tsx index 4a9c3e83..4dbe29e8 100644 --- a/webui/src/routes/issues/-ui/issues-page.tsx +++ b/webui/src/routes/issues/-ui/issues-page.tsx @@ -1,6 +1,6 @@ import { useQuery, useQueryClient } from '@tanstack/react-query'; import { useNavigate } from '@tanstack/react-router'; -import { useEffect, useState } from 'react'; +import { useEffect } from 'react'; import { Select } from '@/components/form'; import { getShellProfileContext } from '@/platform/shell/bridge'; @@ -30,24 +30,39 @@ import { Route } from '../route'; import { IssueDetailModal } from './issue-detail-modal'; import styles from './issues-page.module.css'; +type NavigateFunction = ReturnType; + +function clearIssueSelection(navigate: NavigateFunction) { + void navigate({ + search: (prev) => normalizeIssuesSearch({ ...prev, issueId: undefined }), + replace: true, + }); +} + export function IssuesPage() { const bridge = useReactPageShell('issues'); const queryClient = useQueryClient(); const navigate = useNavigate({ from: Route.fullPath }); const search = Route.useSearch(); const normalizedSearch = normalizeIssuesSearch(search); - const [selectedIssueId, setSelectedIssueId] = useState(null); + const selectedIssueId = normalizedSearch.issueId ? Number(normalizedSearch.issueId) : null; const profile = getShellProfileContext(bridge); const profileId = profile?.profileId ?? 0; + const openIssue = (issueId: number) => { + void navigate({ + search: (prev) => normalizeIssuesSearch({ ...prev, issueId }), + }); + }; + useEffect(() => { const handleRefresh = () => { void queryClient.invalidateQueries({ queryKey: ['issues'] }); }; const handleClose = () => { - setSelectedIssueId(null); + clearIssueSelection(navigate); }; window.addEventListener(REFRESH_EVENT, handleRefresh); @@ -56,7 +71,7 @@ export function IssuesPage() { window.removeEventListener(REFRESH_EVENT, handleRefresh); window.removeEventListener(CLOSE_EVENT, handleClose); }; - }, [queryClient]); + }, [navigate, queryClient]); const countsQuery = useQuery({ ...issueCountsQueryOptions(profileId), @@ -86,14 +101,22 @@ export function IssuesPage() { issuesLoading={issuesQuery.isLoading} onCategoryChange={(category) => void navigate({ - search: (prev) => normalizeIssuesSearch({ ...prev, category }), + search: (prev) => + normalizeIssuesSearch({ + ...prev, + category, + }), replace: true, }) } - onIssueSelect={setSelectedIssueId} + onIssueSelect={openIssue} onStatusChange={(status) => void navigate({ - search: (prev) => normalizeIssuesSearch({ ...prev, status }), + search: (prev) => + normalizeIssuesSearch({ + ...prev, + status, + }), replace: true, }) } @@ -104,9 +127,9 @@ export function IssuesPage() { issue={selectedIssueQuery.data ?? null} isLoading={selectedIssueQuery.isLoading} error={selectedIssueQuery.error} - onClose={() => setSelectedIssueId(null)} + onClose={() => clearIssueSelection(navigate)} onMutationSuccess={() => { - setSelectedIssueId(null); + clearIssueSelection(navigate); dispatchIssuesRefreshEvent(); }} profileId={profile.profileId} @@ -148,7 +171,7 @@ function IssueBoard({ return (
-
+

Issues

@@ -201,7 +224,7 @@ function IssueBoard({

-
+
{safeCounts.open}
Open
@@ -224,33 +247,33 @@ function IssueBoard({
- {issuesLoading ? ( -
-
- Loading issues... -
- ) : issuesError ? ( -
-
Failed to load issues
-
- {issuesError instanceof Error ? issuesError.message : 'Unknown error'} +
+ {issuesLoading ? ( +
+
+ Loading issues...
-
- ) : issues.length === 0 ? ( -
- - )} + }) + )} +
); } diff --git a/webui/src/routes/issues/route.tsx b/webui/src/routes/issues/route.tsx index 0546dad8..735805f4 100644 --- a/webui/src/routes/issues/route.tsx +++ b/webui/src/routes/issues/route.tsx @@ -4,6 +4,7 @@ import { getProfileHomePath, getShellProfileContext } from '@/platform/shell/bri import { issueCountsQueryOptions, + issueDetailQueryOptions, issueListQueryOptions, normalizeIssuesSearch, } from './-issues.helpers'; @@ -17,7 +18,11 @@ export const Route = createFileRoute('/issues')({ throw redirect({ href: getProfileHomePath(bridge), replace: true }); } }, - loaderDeps: ({ search }) => search, + loaderDeps: ({ search }) => ({ + status: search.status, + category: search.category, + issueId: search.issueId ?? null, + }), loader: async ({ context, deps }) => { const profile = getShellProfileContext(context.platform.getShellBridge()); if (!profile) return; @@ -25,6 +30,9 @@ export const Route = createFileRoute('/issues')({ await Promise.all([ context.queryClient.ensureQueryData(issueCountsQueryOptions(profile.profileId)), context.queryClient.ensureQueryData(issueListQueryOptions(profile.profileId, deps)), + deps.issueId + ? context.queryClient.ensureQueryData(issueDetailQueryOptions(profile.profileId, deps.issueId)) + : Promise.resolve(), ]); }, component: IssuesPage, diff --git a/webui/static/search.js b/webui/static/search.js index 1a7c21f4..9c6dff9e 100644 --- a/webui/static/search.js +++ b/webui/static/search.js @@ -1178,14 +1178,15 @@ async function loadInitialData() { : homePage; // Always apply the target page to the legacy shell chrome. - // When the router is present, skipRouteChange keeps the URL stable - // while still syncing the active page/nav state for direct loads. const router = getWebRouter(); const route = router?.routeManifest?.find((entry) => entry.pageId === targetPage); if (route?.kind === 'react') { showReactHost(targetPage); setActivePageChrome(targetPage); + if (window.location.pathname !== route.path) { + history.replaceState({ page: targetPage }, '', route.path); + } return; }