diff --git a/ARCHITECTURE.md b/ARCHITECTURE.md index 023f2f2ad..9aabd9064 100644 --- a/ARCHITECTURE.md +++ b/ARCHITECTURE.md @@ -45,7 +45,7 @@ The renderer is layered. **Runtime imports flow in one direction only** — a mo - `stores/index.ts` is the public store facade for `hooks`, `components`, and `routes`. Lower layers (`utils`) import the specific store module instead (for example `stores/useFiltersStore`): the barrel re-exports every store and its dependencies, which would pull the account/auth/forge graph into leaf utilities and create cycles. - `utils` and `stores` are **peers** — they may reference each other. Neither may import `hooks`, `components`, or `routes`. - `components/` is grouped by feature (`notifications`, `filters`, `settings`, `login`, `avatars`, `layout`, `metrics`, `primitives`). -- The composition root (`App.tsx`, `index.tsx`) is the only place allowed to wire every layer together. +- The composition root (`App.tsx`, `index.tsx`) is the only place allowed to wire every layer together. `App.tsx` owns the router tree and route composition, including the logged-out redirect. `AppLayout` wraps only the routed content in the route `ViewTransition`; the sidebar and window chrome stay outside it. `GlobalEffects` remains outside the router so it is still the single side-effect host. ## State ownership diff --git a/package.json b/package.json index 51128275b..4edc3f31e 100644 --- a/package.json +++ b/package.json @@ -139,7 +139,7 @@ "react-devtools": "8.0.0", "react-dom": "19.3.1", "react-i18next": "17.1.9", - "react-router-dom": "7.18.5", + "react-router": "8.5.0", "rimraf": "6.1.3", "semver": "7.8.5", "tailwindcss": "4.3.3", diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index 83a578664..ac1fee7d4 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -358,9 +358,9 @@ importers: react-i18next: specifier: 17.1.9 version: 17.1.9(i18next@26.5.11(typescript@7.0.2))(react-dom@19.3.1(react@19.3.1))(react@19.3.1)(typescript@7.0.2) - react-router-dom: - specifier: 7.18.5 - version: 7.18.5(react-dom@19.3.1(react@19.3.1))(react@19.3.1) + react-router: + specifier: 8.5.0 + version: 8.5.0(@types/react-dom@19.3.0(@types/react@19.3.0))(@types/react@19.3.0)(react-dom@19.3.1(react@19.3.1))(react@19.3.1) rimraf: specifier: 6.1.3 version: 6.1.3 @@ -1541,6 +1541,9 @@ packages: '@types/react-is': optional: true + '@remix-run/route-pattern@0.22.1': + resolution: {integrity: sha512-czdGJWh09Lapvcqt7Vb09KlrU2PhRJ8xQaI+AItTuD9aDJ5MAgxnS38lnys6Ll+6RJEqPiaUqTwIQhVLwFvv+w==} + '@repeaterjs/repeater@3.1.0': resolution: {integrity: sha512-TaoVksZRSx2KWYYpyLQtMQXXeS98VsgZImzW65xmiVgbYhXLk+aEsmzPLirqVuE4/XuUapH2iMtxUzaBNDzdSQ==} @@ -2612,9 +2615,8 @@ packages: convert-source-map@2.0.0: resolution: {integrity: sha512-Kvp459HrV2FEJ1CAsi1Ku+MY3kasH19TFykTz2xWmMeq6bk2NU3XXvfJ+Q61m0xktWwt+1HSYf3JZsTms3aRJg==} - cookie@1.1.1: - resolution: {integrity: sha512-ei8Aos7ja0weRpFzJnEA9UHJ/7XQmqglbRwnf2ATjcB9Wq874VKH9kfjjirM6UhU2/E5fFYadylyhFldcqSidQ==} - engines: {node: '>=18'} + cookie-es@3.1.1: + resolution: {integrity: sha512-UaXxwISYJPTr9hwQxMFYZ7kNhSXboMXP+Z3TRX6f1/NyaGPfuNUZOWP1pUEb75B2HjfklIYLVRfWiFZJyC6Npg==} core-util-is@1.0.3: resolution: {integrity: sha512-ZQBvi1DcpJ4GDqanjucZ2Hj3wEO5pZDS89BWbkcrvdxksJorwUDDZamX9ldFkp9aw2lmBDLgkObEA4DWNJ9FYQ==} @@ -4133,20 +4135,19 @@ packages: react-is@19.3.1: resolution: {integrity: sha512-RRieF6IyxQBkcK6pP8xOHprLTPW6QjRkbGdImtqNme+/hYVwCZkLQLdlZGOhUqXwkS9IEynaGvPJtv6XB/cz0Q==} - react-router-dom@7.18.5: - resolution: {integrity: sha512-9HcfDMU6JJ/fp6gJZYGwNE6RknVGS4kBjH1HkNw959mCJi70TVwdipQWv2FEol9uGpi+SZTFtVpJPtEQfayeOw==} - engines: {node: '>=20.0.0'} - peerDependencies: - react: '>=18' - react-dom: '>=18' - - react-router@7.18.5: - resolution: {integrity: sha512-dTgvcXDOrLPrlLHrcZAqWveiK1hMSqN+rGAJzegP8HtmNkfRxOoshGWBNX3ZJ0cZjOHkRdlUJtaIhZMLLyGyPw==} - engines: {node: '>=20.0.0'} + react-router@8.5.0: + resolution: {integrity: sha512-9nEd/deifcTJIIcUaNIyQGkyXD0D6QVB9XD2O5GgHpP3xu3U5RXm87YQ8++Wn8JQWgMBM/5+PjWPM/7n7O7WHA==} + engines: {node: '>=22.22.0'} peerDependencies: - react: '>=18' - react-dom: '>=18' + '@types/react': '>=19.2.0' + '@types/react-dom': '>=19.2.0' + react: '>=19.2.7' + react-dom: '>=19.2.7' peerDependenciesMeta: + '@types/react': + optional: true + '@types/react-dom': + optional: true react-dom: optional: true @@ -4296,9 +4297,6 @@ packages: resolution: {integrity: sha512-8I8TjW5KMOKsZQTvoxjuSIa7foAwPWGOts+6o7sgjz41/qMD9VQHEDxi6PBvK2l0MXUmqZyNpUK+T2tQaaElvw==} engines: {node: '>=10'} - set-cookie-parser@2.7.3: - resolution: {integrity: sha512-irZHtTqurLna57KQG/s+fzOwv/J6XSH4cs8M/vsrY3uCaQ3S6+QJguAH0WdN47SXlk82voMZ4naWMQf8fdEoIg==} - shebang-command@2.0.0: resolution: {integrity: sha512-kHxr2zZpYtdmrN1qDjrrX/Z1rR1kG8Dx+gkpK1G4eXmvXswmcE1hTWBWYUzlraYw1/yZp6YuDY77YtvbN0dmDA==} engines: {node: '>=8'} @@ -6116,6 +6114,8 @@ snapshots: '@types/react': 19.3.0 '@types/react-dom': 19.3.0(@types/react@19.3.0) + '@remix-run/route-pattern@0.22.1': {} + '@repeaterjs/repeater@3.1.0': {} '@rolldown/pluginutils@1.0.1': {} @@ -7011,7 +7011,7 @@ snapshots: convert-source-map@2.0.0: {} - cookie@1.1.1: {} + cookie-es@3.1.1: {} core-util-is@1.0.3: {} @@ -8535,18 +8535,14 @@ snapshots: react-is@19.3.1: {} - react-router-dom@7.18.5(react-dom@19.3.1(react@19.3.1))(react@19.3.1): + react-router@8.5.0(@types/react-dom@19.3.0(@types/react@19.3.0))(@types/react@19.3.0)(react-dom@19.3.1(react@19.3.1))(react@19.3.1): dependencies: + '@remix-run/route-pattern': 0.22.1 + cookie-es: 3.1.1 react: 19.3.1 - react-dom: 19.3.1(react@19.3.1) - react-router: 7.18.5(react-dom@19.3.1(react@19.3.1))(react@19.3.1) - - react-router@7.18.5(react-dom@19.3.1(react@19.3.1))(react@19.3.1): - dependencies: - cookie: 1.1.1 - react: 19.3.1 - set-cookie-parser: 2.7.3 optionalDependencies: + '@types/react': 19.3.0 + '@types/react-dom': 19.3.0(@types/react@19.3.0) react-dom: 19.3.1(react@19.3.1) react@19.3.1: {} @@ -8678,8 +8674,6 @@ snapshots: type-fest: 0.13.1 optional: true - set-cookie-parser@2.7.3: {} - shebang-command@2.0.0: dependencies: shebang-regex: 3.0.0 diff --git a/scripts/verifyPackagedDependencies.js b/scripts/verifyPackagedDependencies.js index 0ee86633f..bfbb411d7 100755 --- a/scripts/verifyPackagedDependencies.js +++ b/scripts/verifyPackagedDependencies.js @@ -27,7 +27,7 @@ const FORBIDDEN_PACKAGES = [ 'react', 'react-dom', 'react-i18next', - 'react-router-dom', + 'react-router', ]; const BUILD_ONLY_PACKAGES = [ 'electron', diff --git a/src/renderer/App.test.tsx b/src/renderer/App.test.tsx new file mode 100644 index 000000000..fc3a13484 --- /dev/null +++ b/src/renderer/App.test.tsx @@ -0,0 +1,148 @@ +import { act, cleanup, fireEvent, render, screen, waitFor } from '@testing-library/react'; +import userEvent from '@testing-library/user-event'; + +import { mockGitHubCloudAccount, mockPersonalAccessTokenAccount } from './__mocks__/account-mocks'; +import { mockSettings } from './__mocks__/state-mocks'; + +import { useAccountsStore, useSettingsStore } from './stores'; + +import { App } from './App'; + +// Route resolution is the subject of these tests, so use the real router +// instead of the global `useNavigate` mock from vitest.setup.ts. +vi.unmock('react-router'); + +const PROTECTED_PATHS = ['/', '/settings', '/filters', '/accounts', '/account-scopes']; + +function setHash(path: string) { + window.history.replaceState(null, '', `#${path}`); +} + +async function renderApp(path: string) { + useAccountsStore.setState({ + accounts: PROTECTED_PATHS.includes(path) ? [mockGitHubCloudAccount] : [], + }); + useSettingsStore.setState(mockSettings); + setHash(path); + + await act(async () => { + render(); + }); +} + +afterEach(() => { + cleanup(); + setHash('/'); +}); + +describe('renderer/App.tsx routing', () => { + it('redirects an unauthenticated protected route to login and preserves the destination', async () => { + setHash('/settings'); + render(); + + expect(await screen.findByTestId('login-scroll-container')).toBeInTheDocument(); + expect(window.location.hash).toBe('#/login'); + expect(window.history.state?.usr).toEqual({ + from: expect.objectContaining({ pathname: '/settings' }), + }); + }); + + it('resolves the notifications route at the hash root', async () => { + await renderApp('/'); + + expect(await screen.findByText('No new notifications')).toBeInTheDocument(); + expect(window.location.hash).toBe('#/'); + }); + + it.each([ + ['/settings', 'settings'], + ['/filters', 'filters'], + ['/accounts', 'accounts'], + ['/login', 'login-scroll-container'], + ['/login/github/device-flow', 'Login With Device Flow'], + ['/login/github/cli', 'Login With GitHub CLI'], + ['/login/github/personal-access-token', 'Login With Personal Access Token'], + ['/login/github/custom-oauth-app', 'Login With Custom OAuth App'], + ['/login/gitea/personal-access-token', 'Login With Personal Access Token'], + ['/login/bitbucket/personal-access-token', 'Login With Personal Access Token'], + ['/login/gitlab/personal-access-token', 'Login With Personal Access Token'], + ])('resolves %s', async (path, testId) => { + await renderApp(path); + + expect(await screen.findByTestId(testId)).toBeInTheDocument(); + expect(window.location.hash).toBe(`#${path}`); + }); + + it('preserves the account-scopes destination using router state', async () => { + useAccountsStore.setState({ accounts: [mockPersonalAccessTokenAccount] }); + window.history.replaceState( + { usr: { account: mockPersonalAccessTokenAccount }, key: 'account-scopes', idx: 0 }, + '', + '#/account-scopes', + ); + render(); + + expect(await screen.findByTestId('account-scopes')).toBeInTheDocument(); + }); + + it('keeps browser back and forward navigation working', async () => { + await renderApp('/'); + + await userEvent.click(screen.getByRole('button', { name: 'Settings' })); + expect(await screen.findByTestId('settings')).toBeInTheDocument(); + + await act(async () => { + window.history.back(); + }); + await waitFor(() => expect(window.location.hash).toBe('#/')); + expect(await screen.findByText('No new notifications')).toBeInTheDocument(); + + await act(async () => { + window.history.forward(); + }); + await waitFor(() => expect(window.location.hash).toBe('#/settings')); + expect(await screen.findByTestId('settings')).toBeInTheDocument(); + }); + + it('navigates between settings and the notification list through app controls', async () => { + await renderApp('/'); + + await userEvent.click(screen.getByRole('button', { name: 'Settings' })); + expect(await screen.findByTestId('settings')).toBeInTheDocument(); + expect(window.location.hash).toBe('#/settings'); + + await userEvent.click(screen.getByRole('button', { name: 'Go back' })); + await waitFor(() => expect(window.location.hash).toBe('#/')); + expect(await screen.findByText('No new notifications')).toBeInTheDocument(); + }); + + it('settles on the last route for rapid navigation without stale content or trapped focus', async () => { + await renderApp('/'); + + const [settings, home] = [ + screen.getByRole('button', { name: 'Settings' }), + screen.getByRole('button', { name: 'Gitify' }), + ]; + + act(() => { + fireEvent.click(settings); + fireEvent.click(home); + }); + + await waitFor(() => expect(window.location.hash).toBe('#/')); + expect(await screen.findByText('No new notifications')).toBeInTheDocument(); + expect(screen.queryByTestId('settings')).not.toBeInTheDocument(); + expect(screen.getByRole('button', { name: 'Settings' })).not.toHaveFocus(); + + await userEvent.click(screen.getByRole('button', { name: 'Settings' })); + expect(await screen.findByTestId('settings')).toBeInTheDocument(); + }); + + it('keeps an unknown route on the same empty layout fallback', async () => { + await renderApp('/unknown'); + + expect(await screen.findByLabelText('initial focus')).toBeInTheDocument(); + expect(screen.queryByTestId('notifications')).not.toBeInTheDocument(); + expect(window.location.hash).toBe('#/unknown'); + }); +}); diff --git a/src/renderer/App.tsx b/src/renderer/App.tsx index e72a9992c..83000b6c3 100644 --- a/src/renderer/App.tsx +++ b/src/renderer/App.tsx @@ -1,5 +1,5 @@ import { useEffect } from 'react'; -import { Navigate, Route, HashRouter as Router, Routes, useLocation } from 'react-router-dom'; +import { Navigate, Route, HashRouter as Router, Routes, useLocation } from 'react-router'; import { BaseStyles } from '@primer/react'; import { ThemeProvider } from '@primer/react/next'; @@ -38,6 +38,76 @@ function RequireAuth({ children }: { children: React.ReactNode }) { return isLoggedIn ? children : ; } +function AppRoutes() { + return ( + + + + + } + path="/" + /> + + + + } + path="/filters" + /> + + + + } + path="/settings" + /> + + + + } + path="/accounts" + /> + + + + } + path="/account-scopes" + /> + } path="/login" /> + } path="/login/github/device-flow" /> + } path="/login/github/cli" /> + } + path="/login/github/personal-access-token" + /> + } + path="/login/github/custom-oauth-app" + /> + } + path="/login/gitea/personal-access-token" + /> + } + path="/login/bitbucket/personal-access-token" + /> + } + path="/login/gitlab/personal-access-token" + /> + + ); +} + export function App() { // Initialize store subscriptions with proper cleanup useEffect(() => { @@ -53,74 +123,7 @@ export function App() { - - - - - } - path="/" - /> - - - - } - path="/filters" - /> - - - - } - path="/settings" - /> - - - - } - path="/accounts" - /> - - - - } - path="/account-scopes" - /> - } path="/login" /> - } - path="/login/github/device-flow" - /> - } path="/login/github/cli" /> - } - path="/login/github/personal-access-token" - /> - } - path="/login/github/custom-oauth-app" - /> - } - path="/login/gitea/personal-access-token" - /> - } - path="/login/bitbucket/personal-access-token" - /> - } - path="/login/gitlab/personal-access-token" - /> - + diff --git a/src/renderer/AppNavigation.visual.test.tsx b/src/renderer/AppNavigation.visual.test.tsx new file mode 100644 index 000000000..85ae17e4b --- /dev/null +++ b/src/renderer/AppNavigation.visual.test.tsx @@ -0,0 +1,60 @@ +import { render, screen } from '@testing-library/react'; +import userEvent from '@testing-library/user-event'; + +import { mockGitHubCloudAccount } from './__mocks__/account-mocks'; +import { mockSettings } from './__mocks__/state-mocks'; + +import { useAccountsStore, useSettingsStore } from './stores'; + +import { App } from './App'; + +vi.mock('./hooks/useAccounts', () => ({ + useAccounts: vi.fn(), +})); + +vi.mock('./hooks/notifications/useNotificationsHost', async () => { + const { getMockedNotificationsState } = await import('./__helpers__/hook-mocks'); + return { + useNotificationsHost: () => getMockedNotificationsState(), + }; +}); + +function renderAtRoot() { + useAccountsStore.setState({ accounts: [mockGitHubCloudAccount] }); + useSettingsStore.setState(mockSettings); + window.history.replaceState(null, '', '#/'); + + return render(); +} + +function routeContent() { + return screen.getByTestId('route-content'); +} + +describe('renderer/App route changes', () => { + it('keeps sidebar controls above the routed content', async () => { + renderAtRoot(); + + const settings = screen.getByRole('button', { name: 'Settings' }); + const rect = settings.getBoundingClientRect(); + const hit = document.elementFromPoint(rect.left + rect.width / 2, rect.top + rect.height / 2); + + expect(settings.contains(hit)).toBe(true); + + await userEvent.click(settings); + expect(await screen.findByTestId('settings')).toBeInTheDocument(); + }); + + it('swaps routes immediately and leaves the destination interactive', async () => { + renderAtRoot(); + + await userEvent.click(screen.getByRole('button', { name: 'Settings' })); + expect(await screen.findByTestId('settings')).toBeInTheDocument(); + expect(routeContent().getAnimations()).toHaveLength(0); + expect(routeContent()).not.toContainElement(screen.getByRole('button', { name: 'Gitify' })); + + await userEvent.click(screen.getByRole('button', { name: 'Go back' })); + expect(await screen.findByText('No new notifications')).toBeInTheDocument(); + expect(routeContent().getAnimations()).toHaveLength(0); + }); +}); diff --git a/src/renderer/__helpers__/test-utils.tsx b/src/renderer/__helpers__/test-utils.tsx index 333e35ab0..49b2ded96 100644 --- a/src/renderer/__helpers__/test-utils.tsx +++ b/src/renderer/__helpers__/test-utils.tsx @@ -1,6 +1,6 @@ import { render } from '@testing-library/react'; import type { ReactElement } from 'react'; -import { type InitialEntry, MemoryRouter } from 'react-router-dom'; +import { type InitialEntry, MemoryRouter } from 'react-router'; import { QueryClient, QueryClientProvider } from '@tanstack/react-query'; diff --git a/src/renderer/__helpers__/visual-utils.tsx b/src/renderer/__helpers__/visual-utils.tsx index 1da4438a6..4bc2ca3b7 100644 --- a/src/renderer/__helpers__/visual-utils.tsx +++ b/src/renderer/__helpers__/visual-utils.tsx @@ -1,6 +1,6 @@ import { render, waitFor } from '@testing-library/react'; import type { ReactElement } from 'react'; -import { type InitialEntry, MemoryRouter } from 'react-router-dom'; +import { type InitialEntry, MemoryRouter } from 'react-router'; import { BaseStyles } from '@primer/react'; import { ThemeProvider } from '@primer/react/next'; diff --git a/src/renderer/__helpers__/vitest.setup.ts b/src/renderer/__helpers__/vitest.setup.ts index 7448f0f13..ce3e2fd37 100644 --- a/src/renderer/__helpers__/vitest.setup.ts +++ b/src/renderer/__helpers__/vitest.setup.ts @@ -16,8 +16,8 @@ import useSettingsStore from '../stores/useSettingsStore'; * Shared navigate mock — import from test-utils in any test that needs to assert on navigation */ export const navigateMock = vi.fn(); -vi.mock('react-router-dom', async () => ({ - ...(await vi.importActual('react-router-dom')), +vi.mock('react-router', async () => ({ + ...(await vi.importActual('react-router')), useNavigate: () => navigateMock, })); diff --git a/src/renderer/components/GlobalEffects.test.tsx b/src/renderer/components/GlobalEffects.test.tsx index 9f83546f7..ab5598eda 100644 --- a/src/renderer/components/GlobalEffects.test.tsx +++ b/src/renderer/components/GlobalEffects.test.tsx @@ -33,6 +33,19 @@ describe('renderer/components/GlobalEffects.tsx', () => { expect(setTrayIconColorAndTitleSpy).toHaveBeenCalledWith(-1, true); }); + it('refreshes the tray when a tray setting changes', async () => { + await act(async () => { + renderWithProviders(, { status: 'success', notificationCount: 3 }); + }); + setTrayIconColorAndTitleSpy.mockClear(); + + act(() => { + useSettingsStore.getState().updateSetting('showNotificationsCountInTray', false); + }); + + expect(setTrayIconColorAndTitleSpy).toHaveBeenCalledWith(3, true); + }); + it('resets all stores when the app is reset', async () => { await act(async () => { renderWithProviders(, { diff --git a/src/renderer/components/Oops.tsx b/src/renderer/components/Oops.tsx index e302676ca..eab8b0ed3 100644 --- a/src/renderer/components/Oops.tsx +++ b/src/renderer/components/Oops.tsx @@ -1,8 +1,9 @@ import { useMemo } from 'react'; -import { useNavigate } from 'react-router-dom'; import { Button } from '@primer/react'; +import { useAppNavigate } from '../hooks/useAppNavigate'; + import { EmojiSplash } from './layout/EmojiSplash'; import type { GitifyError } from '../types'; @@ -16,7 +17,7 @@ interface OopsProps { } export function Oops({ error, fullHeight = true }: Readonly) { - const navigate = useNavigate(); + const navigate = useAppNavigate(); const err = error ?? getErrorDetails('UNKNOWN'); diff --git a/src/renderer/components/layout/AppLayout.test.tsx b/src/renderer/components/layout/AppLayout.test.tsx index ac5609e81..05207e5f0 100644 --- a/src/renderer/components/layout/AppLayout.test.tsx +++ b/src/renderer/components/layout/AppLayout.test.tsx @@ -1,3 +1,5 @@ +import { screen } from '@testing-library/react'; + import { renderWithProviders } from '../../__helpers__/test-utils'; import { AppLayout } from './AppLayout'; @@ -8,4 +10,12 @@ describe('renderer/components/layout/AppLayout.tsx', () => { expect(tree.container).toMatchSnapshot(); }); + + it('keeps the sidebar outside the route transition boundary', () => { + renderWithProviders(Test); + + const routeContent = screen.getByTestId('route-content'); + expect(routeContent).toHaveTextContent('Test'); + expect(routeContent).not.toContainElement(screen.getByRole('button', { name: 'Gitify' })); + }); }); diff --git a/src/renderer/components/layout/AppLayout.tsx b/src/renderer/components/layout/AppLayout.tsx index 1faeab219..af48644d6 100644 --- a/src/renderer/components/layout/AppLayout.tsx +++ b/src/renderer/components/layout/AppLayout.tsx @@ -31,7 +31,9 @@ export function AppLayout({ children }: Readonly) { -
{children}
+
+ {children} +
); } diff --git a/src/renderer/components/layout/__snapshots__/AppLayout.test.tsx.snap b/src/renderer/components/layout/__snapshots__/AppLayout.test.tsx.snap index 794b5b692..a47e7e6e1 100644 --- a/src/renderer/components/layout/__snapshots__/AppLayout.test.tsx.snap +++ b/src/renderer/components/layout/__snapshots__/AppLayout.test.tsx.snap @@ -228,6 +228,7 @@ exports[`renderer/components/layout/AppLayout.tsx > renders itself & its childre
Test
diff --git a/src/renderer/components/login/LoginWithPersonalAccessTokenForm.tsx b/src/renderer/components/login/LoginWithPersonalAccessTokenForm.tsx index d952fa98d..e77961606 100644 --- a/src/renderer/components/login/LoginWithPersonalAccessTokenForm.tsx +++ b/src/renderer/components/login/LoginWithPersonalAccessTokenForm.tsx @@ -1,10 +1,11 @@ import { type ReactNode, useCallback, useState } from 'react'; import { useTranslation } from 'react-i18next'; -import { useLocation, useNavigate } from 'react-router-dom'; +import { useLocation } from 'react-router'; import { BookIcon, EyeClosedIcon, EyeIcon, KeyIcon, SignInIcon } from '@primer/octicons-react'; import { Banner, Button, FormControl, Stack, Text, TextInput, Tooltip } from '@primer/react'; +import { useAppNavigate } from '../../hooks/useAppNavigate'; import { useLogins } from '../../hooks/useLogins'; import { Contents } from '../layout/Contents'; @@ -79,7 +80,7 @@ export function LoginWithPersonalAccessTokenForm({ children, }: Readonly) { const { t } = useTranslation(); - const navigate = useNavigate(); + const navigate = useAppNavigate(); const location = useLocation(); const { account: reAuthAccount } = (location.state ?? {}) as LocationState; @@ -127,7 +128,7 @@ export function LoginWithPersonalAccessTokenForm({ ...(data.username ? { username: data.username } : {}), forge, }); - void navigate('/'); + navigate('/'); } catch (err) { rendererLogError('loginWithPersonalAccessToken', 'Failed to login with PAT', toError(err)); setErrors({ diff --git a/src/renderer/components/primitives/Header.tsx b/src/renderer/components/primitives/Header.tsx index 421fcb6e6..5478157da 100644 --- a/src/renderer/components/primitives/Header.tsx +++ b/src/renderer/components/primitives/Header.tsx @@ -1,10 +1,10 @@ import { useTranslation } from 'react-i18next'; -import { useNavigate } from 'react-router-dom'; import { ArrowLeftIcon, type Icon } from '@primer/octicons-react'; import { IconButton, Stack } from '@primer/react'; import { useNotificationActions } from '../../hooks/notifications/useNotificationActions'; +import { useAppNavigate } from '../../hooks/useAppNavigate'; import { SectionHeading } from './SectionHeading'; @@ -16,7 +16,7 @@ interface HeaderProps { export function Header(props: Readonly) { const { t } = useTranslation(); - const navigate = useNavigate(); + const navigate = useAppNavigate(); const { refetchNotifications: fetchNotifications } = useNotificationActions(); @@ -27,7 +27,7 @@ export function Header(props: Readonly) { aria-label={t('header.go_back', 'Go back')} icon={ArrowLeftIcon} onClick={() => { - void navigate(-1); + navigate(-1); if (props.fetchOnBack) { void fetchNotifications(); } diff --git a/src/renderer/components/settings/InboxSettings.tsx b/src/renderer/components/settings/InboxSettings.tsx index a10110f43..534cbf4a0 100644 --- a/src/renderer/components/settings/InboxSettings.tsx +++ b/src/renderer/components/settings/InboxSettings.tsx @@ -1,6 +1,5 @@ import type { MouseEvent } from 'react'; import { Trans, useTranslation } from 'react-i18next'; -import { useNavigate } from 'react-router-dom'; import { CheckIcon, @@ -29,6 +28,7 @@ import { APPLICATION } from '../../../shared/constants'; import { Constants } from '../../constants'; +import { useAppNavigate } from '../../hooks/useAppNavigate'; import { useAccountsStore, useSettingsStore } from '../../stores'; import { SectionHeading } from '../primitives/SectionHeading'; @@ -47,7 +47,7 @@ import { SettingsGroupHeading } from './SettingsGroupHeading'; export function InboxSettings() { const { t } = useTranslation(); - const navigate = useNavigate(); + const navigate = useAppNavigate(); // Account store values const accounts = useAccountsStore((s) => s.accounts); diff --git a/src/renderer/hooks/notifications/useNotificationEffects.ts b/src/renderer/hooks/notifications/useNotificationEffects.ts index 898fd506d..cec0a2e53 100644 --- a/src/renderer/hooks/notifications/useNotificationEffects.ts +++ b/src/renderer/hooks/notifications/useNotificationEffects.ts @@ -1,4 +1,4 @@ -import { useCallback, useEffect, useRef } from 'react'; +import { useCallback, useEffect, useEffectEvent, useRef } from 'react'; import { focusManager, type QueryKey, useQueryClient } from '@tanstack/react-query'; @@ -75,6 +75,30 @@ export const useNotificationEffects = ({ const showSystemNotifications = useSettingsStore((s) => s.showNotifications); const notificationVolume = useSettingsStore((s) => s.notificationVolume); + const announceNewNotifications = useEffectEvent( + (filteredDiffNotifications: GitifyNotification[]) => { + if (playSoundNewNotifications) { + void raiseSoundNotification(notificationVolume); + } + + if (showSystemNotifications) { + void Promise.resolve(raiseNativeNotification(filteredDiffNotifications)) + .then((published) => { + for (const payload of published ?? []) { + publishedIdsRef.current.add(payload.id); + } + }) + .catch((error) => { + rendererLogError( + 'useNotificationEffects', + 'Failed to publish native notifications', + toError(error), + ); + }); + } + }, + ); + useEffect(() => { actionsRef.current = actions; }, [actions]); @@ -213,40 +237,12 @@ export const useNotificationEffects = ({ ) as GitifyNotification[]; if (filteredDiffNotifications.length > 0) { - if (playSoundNewNotifications) { - void raiseSoundNotification(notificationVolume); - } - - if (showSystemNotifications) { - void Promise.resolve(raiseNativeNotification(filteredDiffNotifications)) - .then((published) => { - for (const payload of published ?? []) { - publishedIdsRef.current.add(payload.id); - } - }) - .catch((error) => { - rendererLogError( - 'useNotificationEffects', - 'Failed to publish native notifications', - toError(error), - ); - }); - } + announceNewNotifications(filteredDiffNotifications); } } previousNotificationsRef.current = unfilteredNotifications; - }, [ - enabled, - notifications, - isLoading, - isError, - playSoundNewNotifications, - showSystemNotifications, - notificationVolume, - queryClient, - notificationsQueryKey, - ]); + }, [enabled, notifications, isLoading, isError, queryClient, notificationsQueryKey]); // Remove published notifications when they become read, leave the cache, // or when no unread notifications remain for the summary. diff --git a/src/renderer/hooks/useAppNavigate.test.ts b/src/renderer/hooks/useAppNavigate.test.ts new file mode 100644 index 000000000..8fb167acb --- /dev/null +++ b/src/renderer/hooks/useAppNavigate.test.ts @@ -0,0 +1,45 @@ +import { readdirSync, readFileSync, statSync } from 'node:fs'; +import { join, relative } from 'node:path'; + +import { act, renderHook } from '@testing-library/react'; + +import { navigateMock } from '../__helpers__/test-utils'; + +import { useAppNavigate } from './useAppNavigate'; + +const RENDERER_ROOT = join(process.cwd(), 'src/renderer'); +const WRAPPER = join(RENDERER_ROOT, 'hooks/useAppNavigate.ts'); + +function sourceFiles(directory: string): string[] { + return readdirSync(directory).flatMap((entry) => { + const path = join(directory, entry); + if (statSync(path).isDirectory()) { + return sourceFiles(path); + } + return /\.(ts|tsx)$/.test(entry) && !/\.test\.(ts|tsx)$/.test(entry) ? [path] : []; + }); +} + +describe('useAppNavigate', () => { + it('is the only renderer module that imports the raw navigation hook', () => { + const offenders = sourceFiles(RENDERER_ROOT).filter( + (path) => + path !== WRAPPER && + /import\s*\{[^}]*\buseNavigate\b[^}]*\}\s*from\s*['"]react-router['"]/.test( + readFileSync(path, 'utf8'), + ), + ); + + expect(offenders.map((path) => relative(process.cwd(), path))).toEqual([]); + }); + + it('delegates navigation with the destination and options', () => { + const { result } = renderHook(() => useAppNavigate()); + + act(() => { + result.current('/settings', { replace: true }); + }); + + expect(navigateMock).toHaveBeenCalledWith('/settings', { replace: true }); + }); +}); diff --git a/src/renderer/hooks/useAppNavigate.ts b/src/renderer/hooks/useAppNavigate.ts new file mode 100644 index 000000000..407a883c5 --- /dev/null +++ b/src/renderer/hooks/useAppNavigate.ts @@ -0,0 +1,26 @@ +import { useCallback } from 'react'; +import { type NavigateOptions, type To, useNavigate } from 'react-router'; + +export type AppNavigate = (to: To | number, options?: NavigateOptions) => void; + +/** + * The renderer's shared navigation entry point. Keeping navigation behind one + * hook gives tests and future navigation work a single call site, while routes + * swap immediately with no page-level animation. + */ +export function useAppNavigate(): AppNavigate { + const navigate = useNavigate(); + + return useCallback( + (to, options) => { + if (typeof to === 'number') { + void navigate(to); + } else if (options === undefined) { + void navigate(to); + } else { + void navigate(to, options); + } + }, + [navigate], + ); +} diff --git a/src/renderer/hooks/useNotifications.test.tsx b/src/renderer/hooks/useNotifications.test.tsx index bd15e5f9a..319ea9897 100644 --- a/src/renderer/hooks/useNotifications.test.tsx +++ b/src/renderer/hooks/useNotifications.test.tsx @@ -658,6 +658,47 @@ describe('renderer/hooks/useNotifications.ts', () => { expect(raiseNativeNotificationSpy).toHaveBeenCalledTimes(1); }); + it('uses the latest alert settings for the next alert without re-registering the effect', async () => { + useSettingsStore.setState({ + playSound: true, + showNotifications: false, + notificationVolume: 20 as Percentage, + }); + getAllNotificationsMock.mockResolvedValueOnce(mockSingleAccountNotifications); + + const { result } = renderNotificationsHook(); + await waitFor(() => expect(result.current.hasNotifications).toBe(true)); + await waitFor(() => expect(raiseSoundNotificationSpy).toHaveBeenCalledWith(20)); + expect(raiseNativeNotificationSpy).not.toHaveBeenCalled(); + + act(() => { + useSettingsStore.setState({ + notificationVolume: 60 as Percentage, + showNotifications: true, + }); + }); + getAllNotificationsMock.mockResolvedValueOnce([ + { + account: mockGitHubCloudAccount, + notifications: [ + mockGitifyNotification, + mockGitHubCloudGitifyNotifications[1], + ] as AccountNotifications['notifications'], + error: null, + }, + ]); + + await act(async () => { + await result.current.refetchNotifications(); + }); + + await waitFor(() => { + expect(raiseSoundNotificationSpy).toHaveBeenCalledTimes(2); + expect(raiseSoundNotificationSpy).toHaveBeenLastCalledWith(60); + expect(raiseNativeNotificationSpy).toHaveBeenCalledTimes(1); + }); + }); + it('does not raise sound or native notifications when disabled', async () => { useSettingsStore.setState({ playSound: false, diff --git a/src/renderer/hooks/useShortcutActions.ts b/src/renderer/hooks/useShortcutActions.ts index 082e961c4..db6d7ae25 100644 --- a/src/renderer/hooks/useShortcutActions.ts +++ b/src/renderer/hooks/useShortcutActions.ts @@ -1,10 +1,11 @@ import { useMemo } from 'react'; -import { useLocation, useNavigate } from 'react-router-dom'; +import { useLocation } from 'react-router'; import { useAccountsStore, useSettingsStore } from '../stores'; import { quitApp } from '../utils/system/comms'; import { openHostIssues, openHostNotifications, openHostPulls } from '../utils/system/links'; +import { useAppNavigate } from './useAppNavigate'; import { useNotifications } from './useNotifications'; type ShortcutName = @@ -35,7 +36,7 @@ type ShortcutConfigs = Record; * Used by both the global shortcuts component and UI buttons to avoid duplication. */ export function useShortcutActions(): { shortcuts: ShortcutConfigs } { - const navigate = useNavigate(); + const navigate = useAppNavigate(); const location = useLocation(); const { refetchNotifications: fetchNotifications, status } = useNotifications(); @@ -72,9 +73,9 @@ export function useShortcutActions(): { shortcuts: ShortcutConfigs } { isAllowed: isLoggedIn, action: () => { if (isOnFiltersRoute) { - void navigate('/', { replace: true }); + navigate('/', { replace: true }); } else { - void navigate('/filters'); + navigate('/filters'); } }, }, @@ -95,7 +96,7 @@ export function useShortcutActions(): { shortcuts: ShortcutConfigs } { if (isLoading) { return; } - void navigate('/', { replace: true }); + navigate('/', { replace: true }); void fetchNotifications(); }, }, @@ -104,10 +105,10 @@ export function useShortcutActions(): { shortcuts: ShortcutConfigs } { isAllowed: isLoggedIn, action: () => { if (isOnSettingsRoute) { - void navigate('/', { replace: true }); + navigate('/', { replace: true }); void fetchNotifications(); } else { - void navigate('/settings'); + navigate('/settings'); } }, }, diff --git a/src/renderer/routes/AccountScopes.test.tsx b/src/renderer/routes/AccountScopes.test.tsx index d3c56bb1b..b7264d891 100644 --- a/src/renderer/routes/AccountScopes.test.tsx +++ b/src/renderer/routes/AccountScopes.test.tsx @@ -23,8 +23,8 @@ let mockLocationAccount: Account = { scopes: ['read:user', 'notifications', 'repo'], }; -vi.mock('react-router-dom', async (importActual) => { - const actual = await importActual(); +vi.mock('react-router', async (importActual) => { + const actual = await importActual(); return { ...actual, useNavigate: () => navigateMock, diff --git a/src/renderer/routes/AccountScopes.tsx b/src/renderer/routes/AccountScopes.tsx index e6bca76be..1663bb55c 100644 --- a/src/renderer/routes/AccountScopes.tsx +++ b/src/renderer/routes/AccountScopes.tsx @@ -1,5 +1,5 @@ import { useTranslation } from 'react-i18next'; -import { useLocation } from 'react-router-dom'; +import { useLocation } from 'react-router'; import { AlertFillIcon, LinkExternalIcon, ShieldCheckIcon } from '@primer/octicons-react'; import { Button, Label, Stack, Text } from '@primer/react'; diff --git a/src/renderer/routes/Accounts.tsx b/src/renderer/routes/Accounts.tsx index a64e9fb75..22bbb47e0 100644 --- a/src/renderer/routes/Accounts.tsx +++ b/src/renderer/routes/Accounts.tsx @@ -1,6 +1,5 @@ import { type FC, useCallback, useRef, useState } from 'react'; import { useTranslation } from 'react-i18next'; -import { useNavigate } from 'react-router-dom'; import { AlertFillIcon, @@ -20,6 +19,7 @@ import { ActionList, ActionMenu, Button, IconButton, Stack, Text } from '@primer import { cn } from 'cn'; +import { useAppNavigate } from '../hooks/useAppNavigate'; import { useLogins } from '../hooks/useLogins'; import { useNotifications } from '../hooks/useNotifications'; import { useAccountsStore } from '../stores'; @@ -67,7 +67,7 @@ function getScopeIcon(account: Account): FC { export function AccountsRoute() { const { t } = useTranslation(); - const navigate = useNavigate(); + const navigate = useAppNavigate(); const { logoutFromAccount } = useLogins(); const { notifications } = useNotifications(); @@ -94,7 +94,7 @@ export function AccountsRoute() { useAccountsStore.setState({ accounts: [account, ...accounts.filter((a) => a !== account)], }); - void navigate('/accounts', { replace: true }); + navigate('/accounts', { replace: true }); }; const handleRefresh = async (account: Account) => { @@ -116,7 +116,7 @@ export function AccountsRoute() { })); } - void navigate('/accounts', { replace: true }); + navigate('/accounts', { replace: true }); /** * Typically the above refresh API call completes very quickly, @@ -153,7 +153,7 @@ export function AccountsRoute() { return; } - void navigate(loginMethod.route, { + navigate(loginMethod.route, { replace: true, state: { account }, }); @@ -398,7 +398,7 @@ export function AccountsRoute() { keepMenuOpenRef.current = true; setSelectedForge(adapter); } else { - void navigate(adapter.loginMethods[0].route, { replace: true }); + navigate(adapter.loginMethods[0].route, { replace: true }); } }} > diff --git a/src/renderer/routes/Login.tsx b/src/renderer/routes/Login.tsx index 606d61c54..610be45b1 100644 --- a/src/renderer/routes/Login.tsx +++ b/src/renderer/routes/Login.tsx @@ -1,11 +1,11 @@ import { useCallback, useEffect, useLayoutEffect, useMemo, useRef, useState } from 'react'; import { useTranslation } from 'react-i18next'; -import { useNavigate } from 'react-router-dom'; import { Heading, Stack } from '@primer/react'; import { cn } from 'cn'; +import { useAppNavigate } from '../hooks/useAppNavigate'; import { useAccountsStore } from '../stores'; import { LoginPrimaryAction, LoginSecondaryMethods } from '../components/login/LoginMethods'; @@ -22,7 +22,7 @@ const PANEL_TRANSITION_MS = 280; export function LoginRoute() { const { t } = useTranslation(); - const navigate = useNavigate(); + const navigate = useAppNavigate(); const adapters = listAdapters(); const isLoggedIn = useAccountsStore((s) => s.isLoggedIn()); @@ -111,7 +111,7 @@ export function LoginRoute() { useEffect(() => { if (isLoggedIn) { showWindow(); - void navigate('/', { replace: true }); + navigate('/', { replace: true }); } // oxlint-disable-next-line react/exhaustive-deps -- navigate is stable }, [isLoggedIn]); diff --git a/src/renderer/routes/github/DeviceFlowAuthorize.tsx b/src/renderer/routes/github/DeviceFlowAuthorize.tsx index b2ffd706c..aca2d9339 100644 --- a/src/renderer/routes/github/DeviceFlowAuthorize.tsx +++ b/src/renderer/routes/github/DeviceFlowAuthorize.tsx @@ -1,6 +1,6 @@ import { type ReactNode, useCallback, useEffect, useState } from 'react'; import { Trans, useTranslation } from 'react-i18next'; -import { useLocation, useNavigate } from 'react-router-dom'; +import { useLocation } from 'react-router'; import { CheckCircleFillIcon, @@ -14,6 +14,7 @@ import { Banner, Button, Heading, IconButton, Stack, Text } from '@primer/react' import { Constants } from '../../constants'; +import { useAppNavigate } from '../../hooks/useAppNavigate'; import { useLogins } from '../../hooks/useLogins'; import { Contents } from '../../components/layout/Contents'; @@ -51,7 +52,7 @@ export function GitHubDeviceFlowAuthorize({ title, }: Readonly) { const { t } = useTranslation(); - const navigate = useNavigate(); + const navigate = useAppNavigate(); const location = useLocation(); const { account: reAuthAccount } = (location.state ?? {}) as LocationState; @@ -132,7 +133,7 @@ export function GitHubDeviceFlowAuthorize({ if (token && isActive) { await loginWithDeviceFlowComplete(forge, token, session.hostname, clientId); - void navigate('/'); + navigate('/'); return; } diff --git a/src/renderer/routes/github/LoginWithCustomOAuthApp.tsx b/src/renderer/routes/github/LoginWithCustomOAuthApp.tsx index 4ccc645cd..b1a05495b 100644 --- a/src/renderer/routes/github/LoginWithCustomOAuthApp.tsx +++ b/src/renderer/routes/github/LoginWithCustomOAuthApp.tsx @@ -1,6 +1,6 @@ import { useState } from 'react'; import { Trans, useTranslation } from 'react-i18next'; -import { useLocation } from 'react-router-dom'; +import { useLocation } from 'react-router'; import { BookIcon, PersonIcon } from '@primer/octicons-react'; import { Button, FormControl, Stack, Text, TextInput, Tooltip } from '@primer/react'; diff --git a/src/renderer/routes/github/LoginWithGitHubCLI.tsx b/src/renderer/routes/github/LoginWithGitHubCLI.tsx index 3979a6d2a..6512d953b 100644 --- a/src/renderer/routes/github/LoginWithGitHubCLI.tsx +++ b/src/renderer/routes/github/LoginWithGitHubCLI.tsx @@ -1,12 +1,13 @@ import { useCallback, useState } from 'react'; import { Trans, useTranslation } from 'react-i18next'; -import { useLocation, useNavigate } from 'react-router-dom'; +import { useLocation } from 'react-router'; import { SignInIcon, TerminalIcon } from '@primer/octicons-react'; import { Banner, Button, FormControl, Stack, Text, TextInput } from '@primer/react'; import { Constants } from '../../constants'; +import { useAppNavigate } from '../../hooks/useAppNavigate'; import { useLogins } from '../../hooks/useLogins'; import { Contents } from '../../components/layout/Contents'; @@ -27,7 +28,7 @@ interface LocationState { export function GitHubLoginWithCLIRoute() { const { t } = useTranslation(); - const navigate = useNavigate(); + const navigate = useAppNavigate(); const location = useLocation(); const { account: reAuthAccount } = (location.state ?? {}) as LocationState; @@ -50,7 +51,7 @@ export function GitHubLoginWithCLIRoute() { try { await loginWithCli('github', hostname); - void navigate('/'); + navigate('/'); } catch (err) { rendererLogError('loginWithCli', 'Failed to login with GitHub CLI', toError(err)); setError( diff --git a/src/renderer/routes/routes.visual.test.tsx b/src/renderer/routes/routes.visual.test.tsx index 9caa6ab6a..7e135056e 100644 --- a/src/renderer/routes/routes.visual.test.tsx +++ b/src/renderer/routes/routes.visual.test.tsx @@ -1,6 +1,6 @@ import type { ReactElement } from 'react'; -import { page } from 'vite-plus/test/browser'; +import { cdp, page } from 'vite-plus/test/browser'; import { renderRoute, type VisualRenderOptions } from '../__helpers__/visual-utils'; import { @@ -39,6 +39,20 @@ const AUTHENTICATED: RouteState = { accounts: [mockGitHubCloudAccount, mockGitHubEnterpriseServerAccount], }; +const cdpSession = cdp() as unknown as { + send: (method: string, params: unknown) => Promise; +}; + +/** A leftover pointer can hover a row in the next route's screenshot. */ +async function parkPointer() { + await cdpSession.send('Input.dispatchMouseEvent', { + type: 'mouseMoved', + x: 0, + y: 0, + buttons: 0, + }); +} + /** Every route reachable from `App.tsx`, in the order they are declared there. */ const ROUTES: RouteCase[] = [ { @@ -148,6 +162,7 @@ async function captureRoute( designLanguage: DesignLanguage, { name = route.name, settings }: CaptureOptions = {}, ) { + await parkPointer(); renderRoute(route.element, { ...route.state, theme,