From 1ca0dafbe73255834997dd447cb69bbfeaeec626 Mon Sep 17 00:00:00 2001 From: PARTH J ROHIT Date: Sun, 26 Jul 2026 21:16:53 +0100 Subject: [PATCH 1/2] fix(frontend): stop analysis poll loop stalling before terminal state The poll loop rebuilt its interval on every progress tick (repository object identity changed each response) and kept scheduling another request even after a completed/failed/cancelled response, leaving the "Analysing Repository" page stuck until a manual reload. The loop now stops itself on a terminal status and the effect only restarts on an actual status/error change, not every progress percentage. --- .../AnalysisPipelinePage.integration.test.tsx | 150 ++++++++++++++++++ .../hooks/useAnalysisPipeline.test.ts | 31 ++++ .../analysis/hooks/useAnalysisPipeline.ts | 19 ++- 3 files changed, 194 insertions(+), 6 deletions(-) create mode 100644 apps/frontend/src/app/pages/AnalysisPipelinePage.integration.test.tsx diff --git a/apps/frontend/src/app/pages/AnalysisPipelinePage.integration.test.tsx b/apps/frontend/src/app/pages/AnalysisPipelinePage.integration.test.tsx new file mode 100644 index 0000000..8df122d --- /dev/null +++ b/apps/frontend/src/app/pages/AnalysisPipelinePage.integration.test.tsx @@ -0,0 +1,150 @@ +import { act, render, screen } from '@testing-library/react'; +import { MemoryRouter, Route, Routes } from 'react-router-dom'; +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; +import { useAppStore } from '@/app/store/useAppStore'; +import { backendService } from '@/shared/services/backend'; +import { NetworkError } from '@/shared/services/api'; +import type { Repository } from '@/shared/types'; +import { AnalysisPipelinePage } from './AnalysisPipelinePage'; + +// Exercises the real hook wired to the real store, unlike the sibling +// AnalysisPipelinePage.test.tsx which mocks the hook entirely -- #175 was a +// gap between a hook that transitions correctly in isolation and a page that +// never actually saw the transition land. +vi.mock('@/features/repositories/hooks/useRepository', () => ({ + useRepository: () => ({ repositories: useAppStore((state) => state.repositories) }), +})); + +const repository: Repository = { + id: 'repo-1', + name: 'sample', + source: 'github', + size: 10, + fileCount: 1, + status: 'analysing', + analysisStage: null, + analysisProgress: 0, + uploadedAt: '2026-07-22T08:00:00Z', + meta: null, + fileTree: [], +}; + +function renderPage() { + return render( + + + } /> + Repo Detail Page} /> + + , + ); +} + +describe('AnalysisPipelinePage integration with the real polling hook', () => { + beforeEach(() => { + useAppStore.setState({ repositories: [repository], analysisRunning: true }); + }); + + afterEach(() => { + vi.restoreAllMocks(); + vi.useRealTimers(); + }); + + it('navigates to the analysed repository as soon as the backend reports completed, without a reload', async () => { + vi.useFakeTimers(); + vi.spyOn(backendService, 'startAnalysis').mockResolvedValue({ + repositoryId: repository.id, + status: 'queued', + jobId: 'job-1', + }); + vi.spyOn(backendService, 'fetchAnalysisStatus').mockResolvedValue({ + repositoryId: repository.id, + status: 'completed', + jobId: 'job-1', + stage: 'completed', + progress: 100, + startedAt: '2026-07-22T08:00:01Z', + completedAt: '2026-07-22T08:00:02Z', + error: null, + }); + + // MemoryRouter navigation is a client-side state transition, not a + // browser reload -- this test proves the SPA route swap happens on its + // own, with no manual reload step anywhere in the flow. + renderPage(); + + await act(async () => { + await vi.advanceTimersByTimeAsync(0); + }); + + expect(screen.getByText('Repo Detail Page')).toBeInTheDocument(); + }); + + it('renders the Analysis Failed terminal state when the job fails server-side', async () => { + vi.useFakeTimers(); + vi.spyOn(backendService, 'fetchAnalysisStatus').mockResolvedValue({ + repositoryId: repository.id, + status: 'failed', + jobId: 'job-1', + stage: 'extracting-modules', + progress: 35, + startedAt: '2026-07-22T08:00:01Z', + completedAt: '2026-07-22T08:00:02Z', + error: 'Extraction crashed.', + }); + + renderPage(); + + await act(async () => { + await vi.advanceTimersByTimeAsync(0); + }); + + expect(screen.getByText('Analysis Failed')).toBeInTheDocument(); + expect(screen.getByText('Extraction crashed.')).toBeInTheDocument(); + }); + + it('recovers from a transient poll error into the completed terminal state, without ever showing Analysis Failed', async () => { + vi.useFakeTimers(); + const fetchStatus = vi + .spyOn(backendService, 'fetchAnalysisStatus') + .mockRejectedValueOnce(new NetworkError('/analysis/repo-1/status')) + .mockResolvedValueOnce({ + repositoryId: repository.id, + status: 'running', + jobId: 'job-1', + stage: 'extracting-modules', + progress: 35, + startedAt: '2026-07-22T08:00:01Z', + completedAt: null, + error: null, + }) + .mockResolvedValue({ + repositoryId: repository.id, + status: 'completed', + jobId: 'job-1', + stage: 'completed', + progress: 100, + startedAt: '2026-07-22T08:00:01Z', + completedAt: '2026-07-22T08:00:02Z', + error: null, + }); + + renderPage(); + + await act(async () => { + await vi.advanceTimersByTimeAsync(0); + }); + expect(screen.getByText('Connection lost — retrying…')).toBeInTheDocument(); + expect(screen.queryByText('Analysis Failed')).not.toBeInTheDocument(); + + await act(async () => { + await vi.advanceTimersByTimeAsync(1000); + }); + await act(async () => { + await vi.advanceTimersByTimeAsync(1500); + }); + + expect(screen.getByText('Repo Detail Page')).toBeInTheDocument(); + expect(fetchStatus.mock.calls.length).toBeGreaterThanOrEqual(3); + }); +}); diff --git a/apps/frontend/src/features/analysis/hooks/useAnalysisPipeline.test.ts b/apps/frontend/src/features/analysis/hooks/useAnalysisPipeline.test.ts index 5dc4fc7..e57462a 100644 --- a/apps/frontend/src/features/analysis/hooks/useAnalysisPipeline.test.ts +++ b/apps/frontend/src/features/analysis/hooks/useAnalysisPipeline.test.ts @@ -382,5 +382,36 @@ describe('useAnalysisPipeline', () => { expect(hook.result.current.error).toBe('Extraction crashed.'); expect(hook.result.current.connectionStatus).toBe('connected'); }); + + it('stops polling once a terminal status is observed instead of scheduling another request', async () => { + vi.useFakeTimers(); + const fetchStatus = vi.spyOn(backendService, 'fetchAnalysisStatus').mockResolvedValue({ + repositoryId: repository.id, + status: 'completed', + jobId: 'job-1', + stage: 'completed', + progress: 100, + startedAt: '2026-07-22T08:00:01Z', + completedAt: '2026-07-22T08:00:02Z', + error: null, + }); + + const hook = renderHook(() => useAnalysisPipeline(repository.id)); + + await act(async () => { + await vi.advanceTimersByTimeAsync(0); + }); + expect(hook.result.current.jobStatus).toBe('completed'); + const callsAtCompletion = fetchStatus.mock.calls.length; + + // Advancing well past the poll interval must not trigger another + // request -- the loop stops itself on a terminal status rather than + // relying on the surrounding effect to tear it down. + await act(async () => { + await vi.advanceTimersByTimeAsync(1500 * 5); + }); + expect(fetchStatus.mock.calls.length).toBe(callsAtCompletion); + }); + }); }); diff --git a/apps/frontend/src/features/analysis/hooks/useAnalysisPipeline.ts b/apps/frontend/src/features/analysis/hooks/useAnalysisPipeline.ts index a78c152..1b02f77 100644 --- a/apps/frontend/src/features/analysis/hooks/useAnalysisPipeline.ts +++ b/apps/frontend/src/features/analysis/hooks/useAnalysisPipeline.ts @@ -43,6 +43,7 @@ export function useAnalysisPipeline(repositoryId: string | undefined) { const repository = repositories.find((repo) => repo.id === repositoryId) || null; const repositoryStatus = repository?.status; + const repositoryErrorMessage = repository?.errorMessage; useEffect(() => { pollingGenerationRef.current += 1; @@ -99,21 +100,20 @@ export function useAnalysisPipeline(repositoryId: string | undefined) { setRefreshKey((key) => key + 1); }, []); - // Known limitation: repository identity changes on each poll, so the interval is recreated. useEffect(() => { - if (!repositoryId || !repository) { + if (!repositoryId || !repositoryStatus) { setStatus('empty'); return; } - if (repository.status === 'completed') { + if (repositoryStatus === 'completed') { setStatus('success'); return; } - if (repository.status === 'error') { + if (repositoryStatus === 'error') { setStatus('error'); - setError(repository.errorMessage || 'Analysis failed.'); + setError(repositoryErrorMessage || 'Analysis failed.'); return; } @@ -176,6 +176,13 @@ export function useAnalysisPipeline(repositoryId: string | undefined) { // A job the API itself reports as failed is a real, terminal outcome -- // render it immediately, distinct from a dropped connection below. applyJobResponse(response); + + // Stop the loop here instead of relying on the surrounding effect to + // tear it down on the next render: the poll loop owns its own + // lifecycle, so a terminal outcome never schedules another request. + if (response.status === 'completed' || response.status === 'failed' || response.status === 'cancelled') { + return; + } } catch (caught) { if (cancelled) return; @@ -215,7 +222,7 @@ export function useAnalysisPipeline(repositoryId: string | undefined) { applyJobResponse, failAnalysis, refreshKey, - repository, + repositoryErrorMessage, repositoryId, repositoryStatus, jobStatus, From 6cb240160b9473a2ee7efb3b8d5b844c16d473cc Mon Sep 17 00:00:00 2001 From: PARTH J ROHIT Date: Sun, 26 Jul 2026 21:19:38 +0100 Subject: [PATCH 2/2] fix(frontend): stop deep-linked routes from crashing with a raw 404 Direct navigation to /dashboard or /analysis/{id}/architecture hit react-router's default error boundary ("Unexpected Application Error! 404 Not Found") because no route matched them and there was no catch-all. /dashboard now aliases the Dashboard surface at /, the architecture deep link selects that repository and hands off to /architecture, and any other unmatched path renders a NotFound page instead of crashing. The auth guard still gates all of this. --- .../pages/AnalysisArchitectureRedirect.tsx | 20 ++++++ apps/frontend/src/app/pages/NotFoundPage.tsx | 20 ++++++ apps/frontend/src/app/routes/router.test.tsx | 69 +++++++++++++++++++ apps/frontend/src/app/routes/router.tsx | 26 ++++++- 4 files changed, 134 insertions(+), 1 deletion(-) create mode 100644 apps/frontend/src/app/pages/AnalysisArchitectureRedirect.tsx create mode 100644 apps/frontend/src/app/pages/NotFoundPage.tsx create mode 100644 apps/frontend/src/app/routes/router.test.tsx diff --git a/apps/frontend/src/app/pages/AnalysisArchitectureRedirect.tsx b/apps/frontend/src/app/pages/AnalysisArchitectureRedirect.tsx new file mode 100644 index 0000000..237eb85 --- /dev/null +++ b/apps/frontend/src/app/pages/AnalysisArchitectureRedirect.tsx @@ -0,0 +1,20 @@ +import { useEffect } from 'react'; +import { useNavigate, useParams } from 'react-router-dom'; +import { useAppStore } from '@/app/store/useAppStore'; + +// A repository-scoped deep link into Architecture (#179): the surface itself +// is only ever mounted at the top-level `/architecture` route and reads the +// globally selected repository, so this selects the requested repository +// first and hands off to that route rather than duplicating the surface. +export function AnalysisArchitectureRedirect() { + const { id } = useParams<{ id: string }>(); + const navigate = useNavigate(); + const setActiveRepositoryId = useAppStore((state) => state.setActiveRepositoryId); + + useEffect(() => { + if (id) setActiveRepositoryId(id); + navigate('/architecture', { replace: true }); + }, [id, navigate, setActiveRepositoryId]); + + return null; +} diff --git a/apps/frontend/src/app/pages/NotFoundPage.tsx b/apps/frontend/src/app/pages/NotFoundPage.tsx new file mode 100644 index 0000000..2a7c620 --- /dev/null +++ b/apps/frontend/src/app/pages/NotFoundPage.tsx @@ -0,0 +1,20 @@ +import { useNavigate } from 'react-router-dom'; +import { Compass } from 'lucide-react'; +import { EmptyState } from '@/shared/components/ui/EmptyState'; +import { PageHeader } from '@/shared/components/ui/PageHeader'; + +export function NotFoundPage() { + const navigate = useNavigate(); + + return ( +
+ + navigate('/') }} + /> +
+ ); +} diff --git a/apps/frontend/src/app/routes/router.test.tsx b/apps/frontend/src/app/routes/router.test.tsx new file mode 100644 index 0000000..f10f4a4 --- /dev/null +++ b/apps/frontend/src/app/routes/router.test.tsx @@ -0,0 +1,69 @@ +import { render, screen } from '@testing-library/react'; +import { Outlet, RouterProvider } from 'react-router-dom'; +import { afterEach, describe, expect, it, vi } from 'vitest'; +import { useAppStore } from '@/app/store/useAppStore'; + +// Bypasses auth/repository-fetch plumbing the same way productSurfaces.test.tsx +// and accessibility.test.tsx do, so these tests exercise real route matching +// (#179) without needing a live backend. +vi.mock('./RequireAuth', () => ({ RequireAuth: () => })); +vi.mock('@/shared/components/layout/MainLayout', () => ({ MainLayout: () => })); +vi.mock('@/features/repositories/hooks/useRepository', () => ({ + useRepository: () => ({ + repositories: [], + completedRepositories: [], + activeRepository: null, + loading: false, + error: null, + selectRepository: () => undefined, + removeRepository: async () => undefined, + refresh: () => undefined, + retry: () => undefined, + }), +})); + +import { createAppRouter } from './router'; + +const ROUTE_RENDER_TIMEOUT_MS = 3_000; +let activeRouter: ReturnType | null = null; + +function renderFreshRoute(path: string) { + window.history.replaceState({}, '', path); + activeRouter = createAppRouter(); + return render(); +} + +describe('deep-linked route reachability (#179)', () => { + afterEach(() => { + activeRouter?.dispose(); + activeRouter = null; + window.history.replaceState({}, '', '/'); + useAppStore.setState({ activeRepositoryId: null }); + }); + + it('renders Dashboard directly at /dashboard instead of 404ing', async () => { + renderFreshRoute('/dashboard'); + + expect( + await screen.findByRole('heading', { name: 'Dashboard', level: 1 }, { timeout: ROUTE_RENDER_TIMEOUT_MS }), + ).toBeInTheDocument(); + }); + + it('renders Architecture directly at /analysis/{id}/architecture, selecting that repository', async () => { + renderFreshRoute('/analysis/repo-42/architecture'); + + expect( + await screen.findByRole('heading', { name: 'Architecture', level: 1 }, { timeout: ROUTE_RENDER_TIMEOUT_MS }), + ).toBeInTheDocument(); + expect(useAppStore.getState().activeRepositoryId).toBe('repo-42'); + }); + + it('renders a NotFound page instead of crashing on an unknown route', async () => { + renderFreshRoute('/this-route-does-not-exist'); + + expect( + await screen.findByRole('heading', { name: 'Page not found', level: 1 }, { timeout: ROUTE_RENDER_TIMEOUT_MS }), + ).toBeInTheDocument(); + expect(screen.queryByText(/Unexpected Application Error/i)).not.toBeInTheDocument(); + }); +}); diff --git a/apps/frontend/src/app/routes/router.tsx b/apps/frontend/src/app/routes/router.tsx index 4c1243b..a6450e9 100644 --- a/apps/frontend/src/app/routes/router.tsx +++ b/apps/frontend/src/app/routes/router.tsx @@ -1,4 +1,4 @@ -import { createBrowserRouter } from 'react-router-dom'; +import { createBrowserRouter, Navigate } from 'react-router-dom'; import { MainLayout } from '@/shared/components/layout/MainLayout'; import { RequireAuth } from './RequireAuth'; import { productSurfaceRoutes } from './productSurfaces'; @@ -25,6 +25,12 @@ export function createAppRouter() { { element: , children: [ + { + // Dashboard's registered path is `/` (see productSurfaces); this + // alias keeps the plainer, commonly-guessed URL from 404ing. + path: '/dashboard', + element: , + }, { path: '/repositories/:id', lazy: async () => { @@ -39,7 +45,25 @@ export function createAppRouter() { return { Component: AnalysisPipelinePage }; }, }, + { + path: '/analysis/:id/architecture', + lazy: async () => { + const { AnalysisArchitectureRedirect } = await import('@/app/pages/AnalysisArchitectureRedirect'); + return { Component: AnalysisArchitectureRedirect }; + }, + }, ...productSurfaceRoutes, + { + // Catch-all: an unmatched path previously fell through to react-router's + // default error boundary ("Unexpected Application Error! 404 Not Found") + // instead of a page. This still sits behind RequireAuth, so an + // unauthenticated visitor to a bogus path is redirected to /login first. + path: '*', + lazy: async () => { + const { NotFoundPage } = await import('@/app/pages/NotFoundPage'); + return { Component: NotFoundPage }; + }, + }, ], }, ],