From b012c5cf3f3dc0cb55a777e55e4ca9e774199655 Mon Sep 17 00:00:00 2001 From: Adam Setch Date: Sun, 1 Nov 2026 15:01:17 -0500 Subject: [PATCH 1/3] feat(renderer): adopt data router navigation Replace the declarative HashRouter/Routes tree with a single createHashRouter route table and RouterProvider. Move the window chrome, global shortcuts, and sidebar into a root layout route, and the login guard into a protected layout route. Share sidebar destinations and shortcut routes through nav.ts, render route destinations as NavLinks, and return to the requested destination after login. Add a catch-all route for unknown hashes. Upgrade react-router-dom@7.18.5 to react-router@8.5.0, update imports, test helpers, mocks, and the packaged-dependency check. --- ARCHITECTURE.md | 5 +- package.json | 2 +- pnpm-lock.yaml | 60 ++++----- scripts/verifyPackagedDependencies.js | 2 +- src/renderer/App.test.tsx | 121 ++++++++++++++++++ src/renderer/App.tsx | 102 +-------------- src/renderer/__helpers__/test-utils.tsx | 2 +- src/renderer/__helpers__/visual-utils.tsx | 2 +- src/renderer/__helpers__/vitest.setup.ts | 4 +- src/renderer/components/Oops.tsx | 2 +- src/renderer/components/Sidebar.test.tsx | 21 +-- src/renderer/components/Sidebar.tsx | 56 ++++++-- src/renderer/components/layout/AppLayout.tsx | 4 +- .../__snapshots__/AppLayout.test.tsx.snap | 17 ++- .../LoginWithPersonalAccessTokenForm.tsx | 2 +- src/renderer/components/primitives/Header.tsx | 2 +- .../components/settings/InboxSettings.tsx | 2 +- src/renderer/hooks/useShortcutActions.ts | 25 ++-- src/renderer/nav.ts | 55 ++++++++ src/renderer/router.test.ts | 29 +++++ src/renderer/router.tsx | 68 ++++++++++ src/renderer/routes/AccountScopes.test.tsx | 4 +- src/renderer/routes/AccountScopes.tsx | 2 +- src/renderer/routes/Accounts.tsx | 2 +- src/renderer/routes/Login.tsx | 13 +- src/renderer/routes/ProtectedLayout.tsx | 19 +++ src/renderer/routes/RootLayout.tsx | 19 +++ .../routes/github/DeviceFlowAuthorize.tsx | 2 +- .../routes/github/LoginWithCustomOAuthApp.tsx | 2 +- .../routes/github/LoginWithGitHubCLI.tsx | 2 +- 30 files changed, 459 insertions(+), 189 deletions(-) create mode 100644 src/renderer/App.test.tsx create mode 100644 src/renderer/nav.ts create mode 100644 src/renderer/router.test.ts create mode 100644 src/renderer/router.tsx create mode 100644 src/renderer/routes/ProtectedLayout.tsx create mode 100644 src/renderer/routes/RootLayout.tsx diff --git a/ARCHITECTURE.md b/ARCHITECTURE.md index 023f2f2ad..9430bb3a1 100644 --- a/ARCHITECTURE.md +++ b/ARCHITECTURE.md @@ -36,7 +36,7 @@ The renderer is layered. **Runtime imports flow in one direction only** — a mo | routes screens | - App.tsx / index.tsx composition root - may import any layer + App.tsx / router.tsx / index.tsx composition root - may import any layer ``` - `utils/` is split by concern: `api`, `auth`, `core`, `forges`, `notifications`, `system`, `ui`. @@ -45,7 +45,8 @@ 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`, `router.tsx`, `index.tsx`) is the only place allowed to wire every layer together. +- `router.tsx` owns the single route table and creates the hash data router. A root layout route renders the window chrome, global shortcuts, and sidebar around an ``; a nested protected layout route owns the login redirect for every authenticated screen. Destinations shared with the sidebar and shortcuts live in `nav.ts`. ## 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..07e5048ce --- /dev/null +++ b/src/renderer/App.test.tsx @@ -0,0 +1,121 @@ +import { act, cleanup, render, screen, waitFor } from '@testing-library/react'; +import userEvent from '@testing-library/user-event'; +import { createMemoryRouter, RouterProvider } from 'react-router'; + +import { QueryClient, QueryClientProvider } from '@tanstack/react-query'; + +import { mockGitHubCloudAccount } from './__mocks__/account-mocks'; +import { mockSettings } from './__mocks__/state-mocks'; + +import { useAccountsStore, useSettingsStore } from './stores'; + +import type { Account } from './types'; + +import { routes } from './router'; + +// 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'); + +function renderAt(path: string, { accounts = [] }: { accounts?: Account[] } = {}) { + useAccountsStore.setState({ accounts }); + useSettingsStore.setState(mockSettings); + + const router = createMemoryRouter(routes, { initialEntries: [path] }); + const queryClient = new QueryClient({ + defaultOptions: { queries: { refetchOnWindowFocus: false, retry: false } }, + }); + + const result = render( + + + , + ); + + return { ...result, router }; +} + +afterEach(() => { + cleanup(); +}); + +describe('renderer/App routing', () => { + it('redirects an unauthenticated protected route to login and preserves the destination', async () => { + const { router } = renderAt('/settings'); + + expect(await screen.findByTestId('login-scroll-container')).toBeInTheDocument(); + await waitFor(() => expect(router.state.location.pathname).toBe('/login')); + expect(router.state.location.state).toEqual({ + from: expect.objectContaining({ pathname: '/settings' }), + }); + }); + + it('returns to the requested destination after login', async () => { + const { router } = renderAt('/settings'); + await screen.findByTestId('login-scroll-container'); + + act(() => { + useAccountsStore.setState({ accounts: [mockGitHubCloudAccount] }); + }); + + await waitFor(() => expect(router.state.location.pathname).toBe('/settings')); + expect(await screen.findByTestId('settings')).toBeInTheDocument(); + }); + + it('lands on the default route when login is opened directly', async () => { + const { router } = renderAt('/login'); + await screen.findByTestId('login-scroll-container'); + + act(() => { + useAccountsStore.setState({ accounts: [mockGitHubCloudAccount] }); + }); + + await waitFor(() => expect(router.state.location.pathname).toBe('/')); + expect(await screen.findByText('No new notifications')).toBeInTheDocument(); + }); + + it('resolves an unknown route to the default screen', async () => { + const { router } = renderAt('/unknown', { accounts: [mockGitHubCloudAccount] }); + + await waitFor(() => expect(router.state.location.pathname).toBe('/')); + expect(await screen.findByText('No new notifications')).toBeInTheDocument(); + }); + + it('marks the active sidebar destination with aria-current', async () => { + renderAt('/filters', { accounts: [mockGitHubCloudAccount] }); + + expect(await screen.findByTestId('filters')).toBeInTheDocument(); + expect(screen.getByRole('link', { name: 'Filters' })).toHaveAttribute('aria-current', 'page'); + expect(screen.getByRole('link', { name: 'Settings' })).not.toHaveAttribute('aria-current'); + }); + + it('keeps back navigation working', async () => { + const { router } = renderAt('/', { accounts: [mockGitHubCloudAccount] }); + + await userEvent.click(screen.getByRole('link', { name: 'Settings' })); + await waitFor(() => expect(router.state.location.pathname).toBe('/settings')); + + await act(async () => { + await router.navigate(-1); + }); + + await waitFor(() => expect(router.state.location.pathname).toBe('/')); + expect(await screen.findByText('No new notifications')).toBeInTheDocument(); + }); + + it('settles on the last route for rapid navigation', async () => { + const { router } = renderAt('/', { accounts: [mockGitHubCloudAccount] }); + + const settings = screen.getByRole('link', { name: 'Settings' }); + const home = screen.getByRole('link', { name: 'Gitify' }); + + act(() => { + settings.click(); + home.click(); + }); + + await waitFor(() => expect(router.state.location.pathname).toBe('/')); + expect(screen.queryByTestId('settings')).not.toBeInTheDocument(); + expect(screen.getByRole('link', { name: 'Settings' })).not.toHaveFocus(); + }); +}); diff --git a/src/renderer/App.tsx b/src/renderer/App.tsx index e72a9992c..cd0325887 100644 --- a/src/renderer/App.tsx +++ b/src/renderer/App.tsx @@ -1,43 +1,19 @@ import { useEffect } from 'react'; -import { Navigate, Route, HashRouter as Router, Routes, useLocation } from 'react-router-dom'; import { BaseStyles } from '@primer/react'; import { ThemeProvider } from '@primer/react/next'; import { QueryClientProvider } from '@tanstack/react-query'; +import { RouterProvider } from 'react-router/dom'; -import { AccountsRoute } from './routes/Accounts'; -import { AccountScopesRoute } from './routes/AccountScopes'; -import { BitbucketLoginWithPersonalAccessTokenRoute } from './routes/bitbucket/LoginWithPersonalAccessToken'; -import { FiltersRoute } from './routes/Filters'; -import { GiteaLoginWithPersonalAccessTokenRoute } from './routes/gitea/LoginWithPersonalAccessToken'; -import { GitHubLoginWithCustomOAuthAppRoute } from './routes/github/LoginWithCustomOAuthApp'; -import { GitHubLoginWithDeviceFlowRoute } from './routes/github/LoginWithDeviceFlow'; -import { GitHubLoginWithCLIRoute } from './routes/github/LoginWithGitHubCLI'; -import { GitHubLoginWithPersonalAccessTokenRoute } from './routes/github/LoginWithPersonalAccessToken'; -import { GitLabLoginWithPersonalAccessTokenRoute } from './routes/gitlab/LoginWithPersonalAccessToken'; -import { LoginRoute } from './routes/Login'; -import { NotificationsRoute } from './routes/Notifications'; -import { SettingsRoute } from './routes/Settings'; -import { useAccountsStore } from './stores'; import { initializeStoreSubscriptions } from './stores/subscriptions'; import { GlobalEffects } from './components/GlobalEffects'; -import { GlobalShortcuts } from './components/GlobalShortcuts'; +import { router } from './router'; import './App.css'; -import { AppLayout } from './components/layout/AppLayout'; - import { queryClient } from './utils/api/queryClient'; -function RequireAuth({ children }: { children: React.ReactNode }) { - const location = useLocation(); - - const isLoggedIn = useAccountsStore((s) => s.isLoggedIn()); - - return isLoggedIn ? children : ; -} - export function App() { // Initialize store subscriptions with proper cleanup useEffect(() => { @@ -50,79 +26,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/__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/Oops.tsx b/src/renderer/components/Oops.tsx index e302676ca..498355bba 100644 --- a/src/renderer/components/Oops.tsx +++ b/src/renderer/components/Oops.tsx @@ -1,5 +1,5 @@ import { useMemo } from 'react'; -import { useNavigate } from 'react-router-dom'; +import { useNavigate } from 'react-router'; import { Button } from '@primer/react'; diff --git a/src/renderer/components/Sidebar.test.tsx b/src/renderer/components/Sidebar.test.tsx index 4be7f15ba..f165f17f9 100644 --- a/src/renderer/components/Sidebar.test.tsx +++ b/src/renderer/components/Sidebar.test.tsx @@ -29,18 +29,19 @@ describe('renderer/components/Sidebar.tsx', () => { }); for (const name of [ - new RegExp(`^${APPLICATION.NAME}`), /^Notifications/, /^Toggle focused mode/, - /^Filters/, /^My issues/, /^My pull requests/, /^Refresh/, - /^Settings/, ]) { expect(screen.getByRole('button', { name })).toBeInTheDocument(); } + for (const name of [new RegExp(`^${APPLICATION.NAME}`), /^Filters/, /^Settings/]) { + expect(screen.getByRole('link', { name })).toBeInTheDocument(); + } + expect( screen.queryByRole('button', { name: new RegExp(`^Quit ${APPLICATION.NAME}`) }), ).not.toBeInTheDocument(); @@ -53,7 +54,7 @@ describe('renderer/components/Sidebar.tsx', () => { }); expect( - screen.getByRole('button', { name: new RegExp(`^${APPLICATION.NAME}`) }), + screen.getByRole('link', { name: new RegExp(`^${APPLICATION.NAME}`) }), ).toBeInTheDocument(); expect( screen.getByRole('button', { name: new RegExp(`^Quit ${APPLICATION.NAME}`) }), @@ -84,7 +85,7 @@ describe('renderer/components/Sidebar.tsx', () => { it('navigates home when clicking logo', async () => { renderWithProviders(); - await userEvent.click(screen.getByRole('button', { name: new RegExp(`^${APPLICATION.NAME}`) })); + await userEvent.click(screen.getByRole('link', { name: new RegExp(`^${APPLICATION.NAME}`) })); expect(navigateMock).toHaveBeenCalledTimes(1); expect(navigateMock).toHaveBeenCalledWith('/', { replace: true }); @@ -172,7 +173,7 @@ describe('renderer/components/Sidebar.tsx', () => { accounts: [mockGitHubCloudAccount], }); - await userEvent.click(screen.getByRole('button', { name: /^Filters/ })); + await userEvent.click(screen.getByRole('link', { name: /^Filters/ })); expect(navigateMock).toHaveBeenCalledTimes(1); expect(navigateMock).toHaveBeenCalledWith('/filters'); @@ -184,7 +185,7 @@ describe('renderer/components/Sidebar.tsx', () => { accounts: [mockGitHubCloudAccount], }); - await userEvent.click(screen.getByRole('button', { name: /^Filters/ })); + await userEvent.click(screen.getByRole('link', { name: /^Filters/ })); expect(navigateMock).toHaveBeenCalledTimes(1); expect(navigateMock).toHaveBeenCalledWith('/', { replace: true }); @@ -197,7 +198,7 @@ describe('renderer/components/Sidebar.tsx', () => { filters: { reasons: ['assign'] }, }); - expect(screen.getByRole('button', { name: /^Filters/ })).toHaveAttribute( + expect(screen.getByRole('link', { name: /^Filters/ })).toHaveAttribute( 'data-variant', 'primary', ); @@ -302,7 +303,7 @@ describe('renderer/components/Sidebar.tsx', () => { accounts: [mockGitHubCloudAccount], }); - await userEvent.click(screen.getByRole('button', { name: /^Settings/ })); + await userEvent.click(screen.getByRole('link', { name: /^Settings/ })); expect(navigateMock).toHaveBeenCalledTimes(1); expect(navigateMock).toHaveBeenCalledWith('/settings'); @@ -315,7 +316,7 @@ describe('renderer/components/Sidebar.tsx', () => { fetchNotifications: fetchNotificationsMock, }); - await userEvent.click(screen.getByRole('button', { name: /^Settings/ })); + await userEvent.click(screen.getByRole('link', { name: /^Settings/ })); expect(fetchNotificationsMock).toHaveBeenCalledTimes(1); expect(navigateMock).toHaveBeenCalledTimes(1); diff --git a/src/renderer/components/Sidebar.tsx b/src/renderer/components/Sidebar.tsx index 22be2463d..9b5a3a250 100644 --- a/src/renderer/components/Sidebar.tsx +++ b/src/renderer/components/Sidebar.tsx @@ -1,4 +1,6 @@ +import type { MouseEvent } from 'react'; import { useTranslation } from 'react-i18next'; +import { NavLink } from 'react-router'; import { BellIcon, @@ -20,6 +22,14 @@ import { useShortcutActions } from '../hooks/useShortcutActions'; import { useAccountsStore, useFiltersStore, useSettingsStore } from '../stores'; import { LogoIcon } from '../icons/LogoIcon'; +import { getNavItem } from '../nav'; + +/** + * Primer styles a link-rendered button as `inline-flex`, which squeezes the + * icon into the button's content box. Keep the icon-button grid so icons keep + * their intrinsic size. + */ +const NAV_LINK_STYLE = { display: 'inline-grid' } as const; export function Sidebar() { const { t } = useTranslation(); @@ -36,6 +46,23 @@ export function Sidebar() { const isLoading = status === 'loading'; + const home = getNavItem('home'); + const filters = getNavItem('filters'); + const settings = getNavItem('settings'); + + /** + * Keep the existing toggle behavior for a destination that is already shown, + * while letting modified clicks use the link's native navigation. + */ + const navigateFromControl = (event: MouseEvent, action: () => void) => { + if (event.button !== 0 || event.metaKey || event.ctrlKey || event.shiftKey || event.altKey) { + return; + } + + event.preventDefault(); + action(); + }; + return ( } keybindingHint={shortcuts.home.key} - onClick={() => shortcuts.home.action()} + onClick={(event) => navigateFromControl(event, shortcuts.home.action)} size="small" + style={NAV_LINK_STYLE} + to={home.path} tooltipDirection="e" + type={undefined} variant="invisible" /> @@ -91,13 +123,17 @@ export function Sidebar() { /> shortcuts.filters.action()} + onClick={(event) => navigateFromControl(event, shortcuts.filters.action)} size="small" + style={NAV_LINK_STYLE} + to={filters.path} tooltipDirection="e" + type={undefined} variant={hasFilters ? 'primary' : 'invisible'} /> @@ -141,13 +177,17 @@ export function Sidebar() { /> shortcuts.settings.action()} + onClick={(event) => navigateFromControl(event, shortcuts.settings.action)} size="small" + style={NAV_LINK_STYLE} + to={settings.path} tooltipDirection="e" + type={undefined} variant="invisible" /> diff --git a/src/renderer/components/layout/AppLayout.tsx b/src/renderer/components/layout/AppLayout.tsx index 1faeab219..b81542072 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..541883fc6 100644 --- a/src/renderer/components/layout/__snapshots__/AppLayout.test.tsx.snap +++ b/src/renderer/components/layout/__snapshots__/AppLayout.test.tsx.snap @@ -30,16 +30,19 @@ exports[`renderer/components/layout/AppLayout.tsx > renders itself & its childre data-padding="normal" data-wrap="nowrap" > - + -
Test -
+ `; diff --git a/src/renderer/components/login/LoginWithPersonalAccessTokenForm.tsx b/src/renderer/components/login/LoginWithPersonalAccessTokenForm.tsx index d952fa98d..74ce94bfb 100644 --- a/src/renderer/components/login/LoginWithPersonalAccessTokenForm.tsx +++ b/src/renderer/components/login/LoginWithPersonalAccessTokenForm.tsx @@ -1,6 +1,6 @@ import { type ReactNode, useCallback, useState } from 'react'; import { useTranslation } from 'react-i18next'; -import { useLocation, useNavigate } from 'react-router-dom'; +import { useLocation, useNavigate } from 'react-router'; import { BookIcon, EyeClosedIcon, EyeIcon, KeyIcon, SignInIcon } from '@primer/octicons-react'; import { Banner, Button, FormControl, Stack, Text, TextInput, Tooltip } from '@primer/react'; diff --git a/src/renderer/components/primitives/Header.tsx b/src/renderer/components/primitives/Header.tsx index 421fcb6e6..443942854 100644 --- a/src/renderer/components/primitives/Header.tsx +++ b/src/renderer/components/primitives/Header.tsx @@ -1,5 +1,5 @@ import { useTranslation } from 'react-i18next'; -import { useNavigate } from 'react-router-dom'; +import { useNavigate } from 'react-router'; import { ArrowLeftIcon, type Icon } from '@primer/octicons-react'; import { IconButton, Stack } from '@primer/react'; diff --git a/src/renderer/components/settings/InboxSettings.tsx b/src/renderer/components/settings/InboxSettings.tsx index a10110f43..2da7995af 100644 --- a/src/renderer/components/settings/InboxSettings.tsx +++ b/src/renderer/components/settings/InboxSettings.tsx @@ -1,6 +1,6 @@ import type { MouseEvent } from 'react'; import { Trans, useTranslation } from 'react-i18next'; -import { useNavigate } from 'react-router-dom'; +import { useNavigate } from 'react-router'; import { CheckIcon, diff --git a/src/renderer/hooks/useShortcutActions.ts b/src/renderer/hooks/useShortcutActions.ts index 082e961c4..c35c7bac1 100644 --- a/src/renderer/hooks/useShortcutActions.ts +++ b/src/renderer/hooks/useShortcutActions.ts @@ -1,8 +1,9 @@ import { useMemo } from 'react'; -import { useLocation, useNavigate } from 'react-router-dom'; +import { useLocation, useNavigate } from 'react-router'; import { useAccountsStore, useSettingsStore } from '../stores'; +import { getNavItem } from '../nav'; import { quitApp } from '../utils/system/comms'; import { openHostIssues, openHostNotifications, openHostPulls } from '../utils/system/links'; import { useNotifications } from './useNotifications'; @@ -50,12 +51,16 @@ export function useShortcutActions(): { shortcuts: ShortcutConfigs } { const isOnSettingsRoute = location.pathname.startsWith('/settings'); const isLoading = status === 'loading'; + const home = getNavItem('home'); + const filters = getNavItem('filters'); + const settings = getNavItem('settings'); + const shortcuts: ShortcutConfigs = useMemo(() => { return { home: { - key: 'h', + key: home.shortcutKey, isAllowed: true, - action: () => navigate('/', { replace: true }), + action: () => navigate(home.path, { replace: true }), }, myNotifications: { key: 'n', @@ -68,13 +73,13 @@ export function useShortcutActions(): { shortcuts: ShortcutConfigs } { action: () => updateSetting('participating', !participating), }, filters: { - key: 'f', + key: filters.shortcutKey, isAllowed: isLoggedIn, action: () => { if (isOnFiltersRoute) { - void navigate('/', { replace: true }); + void navigate(home.path, { replace: true }); } else { - void navigate('/filters'); + void navigate(filters.path); } }, }, @@ -95,19 +100,19 @@ export function useShortcutActions(): { shortcuts: ShortcutConfigs } { if (isLoading) { return; } - void navigate('/', { replace: true }); + void navigate(home.path, { replace: true }); void fetchNotifications(); }, }, settings: { - key: 's', + key: settings.shortcutKey, isAllowed: isLoggedIn, action: () => { if (isOnSettingsRoute) { - void navigate('/', { replace: true }); + void navigate(home.path, { replace: true }); void fetchNotifications(); } else { - void navigate('/settings'); + void navigate(settings.path); } }, }, diff --git a/src/renderer/nav.ts b/src/renderer/nav.ts new file mode 100644 index 000000000..b78357794 --- /dev/null +++ b/src/renderer/nav.ts @@ -0,0 +1,55 @@ +export type NavDestinationId = 'home' | 'filters' | 'settings'; + +export interface NavDestination { + /** Stable identifier used to map presentation details such as the icon. */ + id: NavDestinationId; + /** Route the destination resolves to. */ + path: '/' | '/filters' | '/settings'; + /** Translation key for the destination's accessible name. */ + labelKey: string; + /** Translation key for the tooltip description. */ + descriptionKey: string; + /** Keyboard key that activates the destination. */ + shortcutKey: string; + /** Whether the destination only matches its exact path. */ + exact?: boolean; +} + +/** + * Destinations shared by the sidebar and the keyboard shortcuts. + * Plain data only: presentation (icons, labels) lives with the renderer. + */ +export const NAV_ITEMS: readonly NavDestination[] = [ + { + id: 'home', + path: '/', + labelKey: 'sidebar.home', + descriptionKey: 'sidebar.home', + shortcutKey: 'h', + exact: true, + }, + { + id: 'filters', + path: '/filters', + labelKey: 'sidebar.filters', + descriptionKey: 'sidebar.filter_notifications', + shortcutKey: 'f', + }, + { + id: 'settings', + path: '/settings', + labelKey: 'sidebar.settings', + descriptionKey: 'settings.title', + shortcutKey: 's', + }, +]; + +export function getNavItem(id: NavDestinationId): NavDestination { + const item = NAV_ITEMS.find((destination) => destination.id === id); + + if (!item) { + throw new Error(`Unknown navigation destination: ${id}`); + } + + return item; +} diff --git a/src/renderer/router.test.ts b/src/renderer/router.test.ts new file mode 100644 index 000000000..ca38beec4 --- /dev/null +++ b/src/renderer/router.test.ts @@ -0,0 +1,29 @@ +import { createMemoryRouter } from 'react-router'; + +import { routes } from './router'; + +const ROUTE_CASES: Array<[path: string, expectedLeaf: string]> = [ + ['/', 'index'], + ['/filters', 'filters'], + ['/settings', 'settings'], + ['/accounts', 'accounts'], + ['/account-scopes', 'account-scopes'], + ['/login', 'login'], + ['/login/github/device-flow', 'login/github/device-flow'], + ['/login/github/cli', 'login/github/cli'], + ['/login/github/personal-access-token', 'login/github/personal-access-token'], + ['/login/github/custom-oauth-app', 'login/github/custom-oauth-app'], + ['/login/gitea/personal-access-token', 'login/gitea/personal-access-token'], + ['/login/bitbucket/personal-access-token', 'login/bitbucket/personal-access-token'], + ['/login/gitlab/personal-access-token', 'login/gitlab/personal-access-token'], + ['/unknown', '*'], +]; + +describe('renderer/router.tsx', () => { + it.each(ROUTE_CASES)('resolves %s', (path, expectedLeaf) => { + const router = createMemoryRouter(routes, { initialEntries: [path] }); + const leaf = router.state.matches.at(-1)?.route; + + expect(leaf?.path ?? (leaf?.index ? 'index' : undefined)).toBe(expectedLeaf); + }); +}); diff --git a/src/renderer/router.tsx b/src/renderer/router.tsx new file mode 100644 index 000000000..847e0c95e --- /dev/null +++ b/src/renderer/router.tsx @@ -0,0 +1,68 @@ +import { createHashRouter, Navigate, type RouteObject } from 'react-router'; + +import { AccountsRoute } from './routes/Accounts'; +import { AccountScopesRoute } from './routes/AccountScopes'; +import { BitbucketLoginWithPersonalAccessTokenRoute } from './routes/bitbucket/LoginWithPersonalAccessToken'; +import { FiltersRoute } from './routes/Filters'; +import { GiteaLoginWithPersonalAccessTokenRoute } from './routes/gitea/LoginWithPersonalAccessToken'; +import { GitHubLoginWithCustomOAuthAppRoute } from './routes/github/LoginWithCustomOAuthApp'; +import { GitHubLoginWithDeviceFlowRoute } from './routes/github/LoginWithDeviceFlow'; +import { GitHubLoginWithCLIRoute } from './routes/github/LoginWithGitHubCLI'; +import { GitHubLoginWithPersonalAccessTokenRoute } from './routes/github/LoginWithPersonalAccessToken'; +import { GitLabLoginWithPersonalAccessTokenRoute } from './routes/gitlab/LoginWithPersonalAccessToken'; +import { LoginRoute } from './routes/Login'; +import { NotificationsRoute } from './routes/Notifications'; +import { ProtectedLayout } from './routes/ProtectedLayout'; +import { RootLayout } from './routes/RootLayout'; +import { SettingsRoute } from './routes/Settings'; + +/** + * The renderer's route table. Exported separately from the router instance so + * tests can build a memory router from the same routes. + */ +export const routes: RouteObject[] = [ + { + element: , + children: [ + { element: , path: 'login' }, + { + element: , + path: 'login/github/device-flow', + }, + { element: , path: 'login/github/cli' }, + { + element: , + path: 'login/github/personal-access-token', + }, + { + element: , + path: 'login/github/custom-oauth-app', + }, + { + element: , + path: 'login/gitea/personal-access-token', + }, + { + element: , + path: 'login/bitbucket/personal-access-token', + }, + { + element: , + path: 'login/gitlab/personal-access-token', + }, + { + element: , + children: [ + { element: , index: true }, + { element: , path: 'filters' }, + { element: , path: 'settings' }, + { element: , path: 'accounts' }, + { element: , path: 'account-scopes' }, + ], + }, + { element: , path: '*' }, + ], + }, +]; + +export const router = createHashRouter(routes); 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..896c2417d 100644 --- a/src/renderer/routes/Accounts.tsx +++ b/src/renderer/routes/Accounts.tsx @@ -1,6 +1,6 @@ import { type FC, useCallback, useRef, useState } from 'react'; import { useTranslation } from 'react-i18next'; -import { useNavigate } from 'react-router-dom'; +import { useNavigate } from 'react-router'; import { AlertFillIcon, diff --git a/src/renderer/routes/Login.tsx b/src/renderer/routes/Login.tsx index 606d61c54..4fe1bae5f 100644 --- a/src/renderer/routes/Login.tsx +++ b/src/renderer/routes/Login.tsx @@ -1,6 +1,6 @@ import { useCallback, useEffect, useLayoutEffect, useMemo, useRef, useState } from 'react'; import { useTranslation } from 'react-i18next'; -import { useNavigate } from 'react-router-dom'; +import { type Location, useLocation, useNavigate } from 'react-router'; import { Heading, Stack } from '@primer/react'; @@ -23,6 +23,7 @@ const PANEL_TRANSITION_MS = 280; export function LoginRoute() { const { t } = useTranslation(); const navigate = useNavigate(); + const location = useLocation(); const adapters = listAdapters(); const isLoggedIn = useAccountsStore((s) => s.isLoggedIn()); @@ -111,9 +112,15 @@ export function LoginRoute() { useEffect(() => { if (isLoggedIn) { showWindow(); - void navigate('/', { replace: true }); + + const from = (location.state as { from?: Location } | null)?.from; + const destination = from + ? { hash: from.hash, pathname: from.pathname, search: from.search } + : '/'; + + void navigate(destination, { replace: true }); } - // oxlint-disable-next-line react/exhaustive-deps -- navigate is stable + // oxlint-disable-next-line react/exhaustive-deps -- navigate is stable; read the entry location once at the login transition }, [isLoggedIn]); return ( diff --git a/src/renderer/routes/ProtectedLayout.tsx b/src/renderer/routes/ProtectedLayout.tsx new file mode 100644 index 000000000..8733c0ea9 --- /dev/null +++ b/src/renderer/routes/ProtectedLayout.tsx @@ -0,0 +1,19 @@ +import { Navigate, Outlet, useLocation } from 'react-router'; + +import { useAccountsStore } from '../stores'; + +/** + * Single guard for every authenticated route. Reading the account store here + * keeps the redirect reactive to login and logout without a router + * invalidation. + */ +export function ProtectedLayout() { + const isLoggedIn = useAccountsStore((s) => s.isLoggedIn()); + const location = useLocation(); + + if (!isLoggedIn) { + return ; + } + + return ; +} diff --git a/src/renderer/routes/RootLayout.tsx b/src/renderer/routes/RootLayout.tsx new file mode 100644 index 000000000..3453541ef --- /dev/null +++ b/src/renderer/routes/RootLayout.tsx @@ -0,0 +1,19 @@ +import { Outlet } from 'react-router'; + +import { GlobalShortcuts } from '../components/GlobalShortcuts'; +import { AppLayout } from '../components/layout/AppLayout'; + +/** + * Root layout for the application shell. The sidebar and window controls stay + * outside the routed content, so a route change never remounts them. + */ +export function RootLayout() { + return ( + <> + + + + + + ); +} diff --git a/src/renderer/routes/github/DeviceFlowAuthorize.tsx b/src/renderer/routes/github/DeviceFlowAuthorize.tsx index b2ffd706c..87ea385a4 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, useNavigate } from 'react-router'; import { CheckCircleFillIcon, 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..48ac2c6b2 100644 --- a/src/renderer/routes/github/LoginWithGitHubCLI.tsx +++ b/src/renderer/routes/github/LoginWithGitHubCLI.tsx @@ -1,6 +1,6 @@ import { useCallback, useState } from 'react'; import { Trans, useTranslation } from 'react-i18next'; -import { useLocation, useNavigate } from 'react-router-dom'; +import { useLocation, useNavigate } from 'react-router'; import { SignInIcon, TerminalIcon } from '@primer/octicons-react'; import { Banner, Button, FormControl, Stack, Text, TextInput } from '@primer/react'; From 548a57179a3441a1a9f016e56bc4faf8eaedf786 Mon Sep 17 00:00:00 2001 From: Adam Setch Date: Sun, 1 Nov 2026 15:06:04 -0500 Subject: [PATCH 2/3] fix(renderer): keep sidebar destination labels extractable Restore literal t() keys for the sidebar copy so removeUnusedKeys keeps them, and leave nav.ts as paths and shortcuts only. --- src/renderer/components/Sidebar.tsx | 25 ++++++++++++++++++++----- src/renderer/nav.ts | 10 ---------- 2 files changed, 20 insertions(+), 15 deletions(-) diff --git a/src/renderer/components/Sidebar.tsx b/src/renderer/components/Sidebar.tsx index 9b5a3a250..c22d23a78 100644 --- a/src/renderer/components/Sidebar.tsx +++ b/src/renderer/components/Sidebar.tsx @@ -50,6 +50,21 @@ export function Sidebar() { const filters = getNavItem('filters'); const settings = getNavItem('settings'); + // Literal t() keys keep the labels discoverable by the i18n extractor. + const copy = { + filters: { + label: t('sidebar.filters', 'Filters'), + description: t('sidebar.filter_notifications', 'Filter notifications'), + }, + home: { + description: t('sidebar.home', 'Home'), + }, + settings: { + label: t('sidebar.settings', 'Settings'), + description: t('settings.title', 'Settings'), + }, + }; + /** * Keep the existing toggle behavior for a destination that is already shown, * while letting modified clicks use the link's native navigation. @@ -73,7 +88,7 @@ export function Sidebar() { } keybindingHint={shortcuts.home.key} @@ -123,9 +138,9 @@ export function Sidebar() { /> navigateFromControl(event, shortcuts.filters.action)} @@ -177,9 +192,9 @@ export function Sidebar() { /> navigateFromControl(event, shortcuts.settings.action)} diff --git a/src/renderer/nav.ts b/src/renderer/nav.ts index b78357794..90d8da292 100644 --- a/src/renderer/nav.ts +++ b/src/renderer/nav.ts @@ -5,10 +5,6 @@ export interface NavDestination { id: NavDestinationId; /** Route the destination resolves to. */ path: '/' | '/filters' | '/settings'; - /** Translation key for the destination's accessible name. */ - labelKey: string; - /** Translation key for the tooltip description. */ - descriptionKey: string; /** Keyboard key that activates the destination. */ shortcutKey: string; /** Whether the destination only matches its exact path. */ @@ -23,23 +19,17 @@ export const NAV_ITEMS: readonly NavDestination[] = [ { id: 'home', path: '/', - labelKey: 'sidebar.home', - descriptionKey: 'sidebar.home', shortcutKey: 'h', exact: true, }, { id: 'filters', path: '/filters', - labelKey: 'sidebar.filters', - descriptionKey: 'sidebar.filter_notifications', shortcutKey: 'f', }, { id: 'settings', path: '/settings', - labelKey: 'sidebar.settings', - descriptionKey: 'settings.title', shortcutKey: 's', }, ]; From 2eec8fd9367f2f8a772c23e3414e825a55723394 Mon Sep 17 00:00:00 2001 From: Adam Setch Date: Sun, 1 Nov 2026 15:13:29 -0500 Subject: [PATCH 3/3] refactor(renderer): keep sidebar navigation as buttons Expose the active destination with aria-current instead of rendering route controls as links. This avoids the Primer link-specific icon sizing workaround and keeps the existing toggle behavior without click interception. --- src/renderer/App.test.tsx | 12 ++--- src/renderer/components/Sidebar.test.tsx | 16 +++--- src/renderer/components/Sidebar.tsx | 49 +++++-------------- .../__snapshots__/AppLayout.test.tsx.snap | 10 ++-- 4 files changed, 30 insertions(+), 57 deletions(-) diff --git a/src/renderer/App.test.tsx b/src/renderer/App.test.tsx index 07e5048ce..6e40ee172 100644 --- a/src/renderer/App.test.tsx +++ b/src/renderer/App.test.tsx @@ -85,14 +85,14 @@ describe('renderer/App routing', () => { renderAt('/filters', { accounts: [mockGitHubCloudAccount] }); expect(await screen.findByTestId('filters')).toBeInTheDocument(); - expect(screen.getByRole('link', { name: 'Filters' })).toHaveAttribute('aria-current', 'page'); - expect(screen.getByRole('link', { name: 'Settings' })).not.toHaveAttribute('aria-current'); + expect(screen.getByRole('button', { name: 'Filters' })).toHaveAttribute('aria-current', 'page'); + expect(screen.getByRole('button', { name: 'Settings' })).not.toHaveAttribute('aria-current'); }); it('keeps back navigation working', async () => { const { router } = renderAt('/', { accounts: [mockGitHubCloudAccount] }); - await userEvent.click(screen.getByRole('link', { name: 'Settings' })); + await userEvent.click(screen.getByRole('button', { name: 'Settings' })); await waitFor(() => expect(router.state.location.pathname).toBe('/settings')); await act(async () => { @@ -106,8 +106,8 @@ describe('renderer/App routing', () => { it('settles on the last route for rapid navigation', async () => { const { router } = renderAt('/', { accounts: [mockGitHubCloudAccount] }); - const settings = screen.getByRole('link', { name: 'Settings' }); - const home = screen.getByRole('link', { name: 'Gitify' }); + const settings = screen.getByRole('button', { name: 'Settings' }); + const home = screen.getByRole('button', { name: 'Gitify' }); act(() => { settings.click(); @@ -116,6 +116,6 @@ describe('renderer/App routing', () => { await waitFor(() => expect(router.state.location.pathname).toBe('/')); expect(screen.queryByTestId('settings')).not.toBeInTheDocument(); - expect(screen.getByRole('link', { name: 'Settings' })).not.toHaveFocus(); + expect(screen.getByRole('button', { name: 'Settings' })).not.toHaveFocus(); }); }); diff --git a/src/renderer/components/Sidebar.test.tsx b/src/renderer/components/Sidebar.test.tsx index f165f17f9..4cb2fd714 100644 --- a/src/renderer/components/Sidebar.test.tsx +++ b/src/renderer/components/Sidebar.test.tsx @@ -39,7 +39,7 @@ describe('renderer/components/Sidebar.tsx', () => { } for (const name of [new RegExp(`^${APPLICATION.NAME}`), /^Filters/, /^Settings/]) { - expect(screen.getByRole('link', { name })).toBeInTheDocument(); + expect(screen.getByRole('button', { name })).toBeInTheDocument(); } expect( @@ -54,7 +54,7 @@ describe('renderer/components/Sidebar.tsx', () => { }); expect( - screen.getByRole('link', { name: new RegExp(`^${APPLICATION.NAME}`) }), + screen.getByRole('button', { name: new RegExp(`^${APPLICATION.NAME}`) }), ).toBeInTheDocument(); expect( screen.getByRole('button', { name: new RegExp(`^Quit ${APPLICATION.NAME}`) }), @@ -85,7 +85,7 @@ describe('renderer/components/Sidebar.tsx', () => { it('navigates home when clicking logo', async () => { renderWithProviders(); - await userEvent.click(screen.getByRole('link', { name: new RegExp(`^${APPLICATION.NAME}`) })); + await userEvent.click(screen.getByRole('button', { name: new RegExp(`^${APPLICATION.NAME}`) })); expect(navigateMock).toHaveBeenCalledTimes(1); expect(navigateMock).toHaveBeenCalledWith('/', { replace: true }); @@ -173,7 +173,7 @@ describe('renderer/components/Sidebar.tsx', () => { accounts: [mockGitHubCloudAccount], }); - await userEvent.click(screen.getByRole('link', { name: /^Filters/ })); + await userEvent.click(screen.getByRole('button', { name: /^Filters/ })); expect(navigateMock).toHaveBeenCalledTimes(1); expect(navigateMock).toHaveBeenCalledWith('/filters'); @@ -185,7 +185,7 @@ describe('renderer/components/Sidebar.tsx', () => { accounts: [mockGitHubCloudAccount], }); - await userEvent.click(screen.getByRole('link', { name: /^Filters/ })); + await userEvent.click(screen.getByRole('button', { name: /^Filters/ })); expect(navigateMock).toHaveBeenCalledTimes(1); expect(navigateMock).toHaveBeenCalledWith('/', { replace: true }); @@ -198,7 +198,7 @@ describe('renderer/components/Sidebar.tsx', () => { filters: { reasons: ['assign'] }, }); - expect(screen.getByRole('link', { name: /^Filters/ })).toHaveAttribute( + expect(screen.getByRole('button', { name: /^Filters/ })).toHaveAttribute( 'data-variant', 'primary', ); @@ -303,7 +303,7 @@ describe('renderer/components/Sidebar.tsx', () => { accounts: [mockGitHubCloudAccount], }); - await userEvent.click(screen.getByRole('link', { name: /^Settings/ })); + await userEvent.click(screen.getByRole('button', { name: /^Settings/ })); expect(navigateMock).toHaveBeenCalledTimes(1); expect(navigateMock).toHaveBeenCalledWith('/settings'); @@ -316,7 +316,7 @@ describe('renderer/components/Sidebar.tsx', () => { fetchNotifications: fetchNotificationsMock, }); - await userEvent.click(screen.getByRole('link', { name: /^Settings/ })); + await userEvent.click(screen.getByRole('button', { name: /^Settings/ })); expect(fetchNotificationsMock).toHaveBeenCalledTimes(1); expect(navigateMock).toHaveBeenCalledTimes(1); diff --git a/src/renderer/components/Sidebar.tsx b/src/renderer/components/Sidebar.tsx index c22d23a78..685f8a844 100644 --- a/src/renderer/components/Sidebar.tsx +++ b/src/renderer/components/Sidebar.tsx @@ -1,6 +1,5 @@ -import type { MouseEvent } from 'react'; import { useTranslation } from 'react-i18next'; -import { NavLink } from 'react-router'; +import { useLocation } from 'react-router'; import { BellIcon, @@ -24,15 +23,9 @@ import { useAccountsStore, useFiltersStore, useSettingsStore } from '../stores'; import { LogoIcon } from '../icons/LogoIcon'; import { getNavItem } from '../nav'; -/** - * Primer styles a link-rendered button as `inline-flex`, which squeezes the - * icon into the button's content box. Keep the icon-button grid so icons keep - * their intrinsic size. - */ -const NAV_LINK_STYLE = { display: 'inline-grid' } as const; - export function Sidebar() { const { t } = useTranslation(); + const location = useLocation(); const { status, notificationCount, hasUnreadNotifications, isFetching } = useNotifications(); const { shortcuts } = useShortcutActions(); @@ -65,18 +58,10 @@ export function Sidebar() { }, }; - /** - * Keep the existing toggle behavior for a destination that is already shown, - * while letting modified clicks use the link's native navigation. - */ - const navigateFromControl = (event: MouseEvent, action: () => void) => { - if (event.button !== 0 || event.metaKey || event.ctrlKey || event.shiftKey || event.altKey) { - return; - } - - event.preventDefault(); - action(); - }; + const isActive = (path: string, exact = false) => + exact + ? location.pathname === path + : location.pathname === path || location.pathname.startsWith(`${path}/`); return ( } keybindingHint={shortcuts.home.key} - onClick={(event) => navigateFromControl(event, shortcuts.home.action)} + onClick={() => shortcuts.home.action()} size="small" - style={NAV_LINK_STYLE} - to={home.path} tooltipDirection="e" - type={undefined} variant="invisible" /> @@ -138,17 +119,14 @@ export function Sidebar() { /> navigateFromControl(event, shortcuts.filters.action)} + onClick={() => shortcuts.filters.action()} size="small" - style={NAV_LINK_STYLE} - to={filters.path} tooltipDirection="e" - type={undefined} variant={hasFilters ? 'primary' : 'invisible'} /> @@ -192,17 +170,14 @@ export function Sidebar() { /> navigateFromControl(event, shortcuts.settings.action)} + onClick={() => shortcuts.settings.action()} size="small" - style={NAV_LINK_STYLE} - to={settings.path} tooltipDirection="e" - type={undefined} variant="invisible" /> diff --git a/src/renderer/components/layout/__snapshots__/AppLayout.test.tsx.snap b/src/renderer/components/layout/__snapshots__/AppLayout.test.tsx.snap index 541883fc6..7eb5ed6f2 100644 --- a/src/renderer/components/layout/__snapshots__/AppLayout.test.tsx.snap +++ b/src/renderer/components/layout/__snapshots__/AppLayout.test.tsx.snap @@ -30,19 +30,17 @@ exports[`renderer/components/layout/AppLayout.tsx > renders itself & its childre data-padding="normal" data-wrap="nowrap" > - - +