From 002cfc55d72189d9dde5868d2a329a314e915465 Mon Sep 17 00:00:00 2001 From: Adam Setch Date: Sun, 1 Nov 2026 10:36:38 -0500 Subject: [PATCH 1/8] fix(notifications): bound notification exit animation Finish the row exit within a fixed 500ms budget instead of holding its layout space until the mutation resolves. The slide/fade remains 350ms; the grid track then collapses over 150ms so slow networks cannot leave an invisible blank row. Group actions collapse the header and rows together, failures restore the layout, and Reduce Motion collapses immediately. Cache updates remain pessimistic. --- .../notifications/ExitCollapse.test.tsx | 53 +++ .../components/notifications/ExitCollapse.tsx | 49 +++ .../NotificationList.visual.test.tsx | 354 +++++++++++++++++- .../notifications/NotificationRow.test.tsx | 45 ++- .../notifications/NotificationRow.tsx | 195 +++++----- .../notifications/RepositoryHeader.test.tsx | 46 ++- .../notifications/RepositoryHeader.tsx | 123 +++--- 7 files changed, 706 insertions(+), 159 deletions(-) create mode 100644 src/renderer/components/notifications/ExitCollapse.test.tsx create mode 100644 src/renderer/components/notifications/ExitCollapse.tsx diff --git a/src/renderer/components/notifications/ExitCollapse.test.tsx b/src/renderer/components/notifications/ExitCollapse.test.tsx new file mode 100644 index 000000000..c070dba5e --- /dev/null +++ b/src/renderer/components/notifications/ExitCollapse.test.tsx @@ -0,0 +1,53 @@ +import { renderWithProviders } from '../../__helpers__/test-utils'; + +import { ExitCollapse } from './ExitCollapse'; + +describe('renderer/components/notifications/ExitCollapse.tsx', () => { + const renderCollapse = (isExiting: boolean) => + renderWithProviders( + +
Notification
+ , + ); + + it('renders its content without exit markers', () => { + const tree = renderCollapse(false); + + expect(tree.getByTestId('content')).toBeInTheDocument(); + expect(tree.container.querySelector('[data-exit-motion]')).toBeNull(); + expect(tree.container.querySelector('[data-exit-collapse]')).toBeNull(); + expect(tree.container.firstElementChild?.firstElementChild).toHaveClass('grid-rows-[1fr]'); + }); + + it('marks both layers and applies the slide and collapse classes while exiting', () => { + const tree = renderCollapse(true); + + expect(tree.container.querySelector('[data-exit-motion]')).toHaveClass( + 'translate-x-full', + 'opacity-0', + 'overflow-hidden', + 'duration-350', + 'motion-reduce:transition-none', + ); + expect(tree.container.querySelector('[data-exit-collapse]')).toHaveClass( + 'grid-rows-[0fr]', + 'duration-150', + 'delay-350', + 'motion-reduce:transition-none', + ); + }); + + it('clears the exit markers and classes when the exit stops', () => { + const tree = renderCollapse(true); + + tree.rerender( + +
Notification
+ , + ); + + expect(tree.container.querySelector('[data-exit-motion]')).toBeNull(); + expect(tree.container.querySelector('[data-exit-collapse]')).toBeNull(); + expect(tree.container.firstElementChild).not.toHaveClass('translate-x-full'); + }); +}); diff --git a/src/renderer/components/notifications/ExitCollapse.tsx b/src/renderer/components/notifications/ExitCollapse.tsx new file mode 100644 index 000000000..eae1541ed --- /dev/null +++ b/src/renderer/components/notifications/ExitCollapse.tsx @@ -0,0 +1,49 @@ +import type { ReactNode } from 'react'; + +import { cn } from 'cn'; + +export interface ExitCollapseProps { + isExiting: boolean; + children: ReactNode; +} + +/** + * Two-layer exit animation for a notification row or repository header. + * + * The outer layer slides and fades the content out over 350ms. Tailwind v4 + * emits `translate-x-full` as the `translate` property, so the transition list + * must include `translate` as well as `transform`; transitioning only + * `transform` makes the row jump right instead of sliding. The inner grid + * layer then collapses the content's layout space over 150ms, so a slow or + * hanging request cannot leave an invisible row holding its height. The + * collapse starts after the slide via the matching `delay-350`, bounding the + * whole exit at 500ms. + * + * The transition classes stay mounted so a failed action can slide back in + * smoothly, while the collapse transition only exists while exiting so the + * layout snaps back as soon as the row is restored. Reduced-motion users skip + * both transitions and collapse immediately. + */ +export function ExitCollapse({ isExiting, children }: Readonly) { + return ( +
+
+
{children}
+
+
+ ); +} diff --git a/src/renderer/components/notifications/NotificationList.visual.test.tsx b/src/renderer/components/notifications/NotificationList.visual.test.tsx index 438bc9493..08b0773fc 100644 --- a/src/renderer/components/notifications/NotificationList.visual.test.tsx +++ b/src/renderer/components/notifications/NotificationList.visual.test.tsx @@ -1,6 +1,6 @@ import { act, waitFor } from '@testing-library/react'; -import { page } from 'vite-plus/test/browser'; +import { cdp, page } from 'vite-plus/test/browser'; import { getMockedNotificationsState, @@ -14,6 +14,7 @@ import { import { mockGitifyNotification } from '../../__mocks__/notifications-mocks'; import { NotificationsRoute } from '../../routes/Notifications'; +import { getNotificationFailureKey, useNotificationActionFailuresStore } from '../../stores'; import { type AccountNotifications, @@ -23,6 +24,10 @@ import { Theme, } from '../../types'; +type CdpSession = { + send: (method: string, params?: object) => Promise; +}; + const account = mockGitHubCloudAccount; const fixture = (count: number) => Array.from({ length: count }, (_, index) => ({ @@ -78,7 +83,41 @@ async function scrollToStart() { }); } +/** + * The visual setup zeroes every transition and delay for screenshot stability. + * Timing-sensitive exit tests remove that override so the real budget runs. + */ +function enableAnimations() { + for (const style of document.querySelectorAll('style')) { + if (style.textContent?.includes('animation-duration: 0s !important')) { + style.remove(); + } + } +} + +/** The row's parent is the collapsing grid item. */ +function rowWrapperHeight(id: string): number { + return document.getElementById(id)?.parentElement?.getBoundingClientRect().height ?? -1; +} + +/** The button's parent is the header root, which sits inside the collapsing grid item. */ +function repositoryWrapperHeight(): number { + return ( + document + .querySelector('[data-testid="open-repository"]') + ?.parentElement?.parentElement?.getBoundingClientRect().height ?? -1 + ); +} + +async function waitForRowCollapsed(id: string) { + await waitFor(() => expect(rowWrapperHeight(id)).toBeLessThan(2), { timeout: 2000 }); +} + describe('notification list in a browser', () => { + afterEach(() => { + useNotificationActionFailuresStore.getState().reset(); + }); + it.each([GroupBy.DATE, GroupBy.REPOSITORY])( 'renders grouping %s and reaches every row in a 2000-notification inbox', async (groupBy) => { @@ -324,4 +363,317 @@ describe('notification list in a browser', () => { false, ); }); + + it('collapses a pending row within the budget without removing it from the cache', async () => { + enableAnimations(); + + let finish: (value: boolean) => void = () => {}; + const pending = new Promise((resolve) => { + finish = resolve; + }); + setNotificationsOverrides({ markNotificationsAsRead: vi.fn(() => pending) }); + const tree = await renderList(fixture(2), { + delayNotificationState: false, + fetchReadNotifications: false, + }); + const original = getMockedNotificationsState().notifications; + const secondTopBefore = document.getElementById('notification-1')!.getBoundingClientRect().top; + + await page.getByTestId('notification-mark-as-read').first().click(); + await waitFor(() => + expect(document.getElementById('notification-0')?.hasAttribute('data-animating-exit')).toBe( + true, + ), + ); + + const motionLayer = document + .getElementById('notification-0') + ?.closest('[data-exit-motion]'); + expect(motionLayer).not.toBeNull(); + const motionStyle = getComputedStyle(motionLayer!); + expect(motionStyle.transitionProperty).toContain('translate'); + expect(motionStyle.transitionProperty).toContain('opacity'); + expect(motionStyle.transitionDuration).not.toBe('0s'); + + await waitForRowCollapsed('notification-0'); + expect(document.getElementById('notification-1')!.getBoundingClientRect().top).toBeLessThan( + secondTopBefore - 20, + ); + expect(document.querySelector('[data-testid="open-repository"]')?.textContent).toContain('(2)'); + + const remaining = original.map((entry) => ({ + ...entry, + notifications: entry.notifications.filter( + (notification) => notification.id !== 'notification-0', + ), + })); + await act(async () => { + setNotificationsOverrides({ notifications: remaining }); + tree.rerender(); + finish(true); + await pending; + }); + await waitFor(() => expect(document.getElementById('notification-0')).toBeNull()); + }); + + it.each(['before', 'after'])( + 'restores a row when a pending action fails %s the collapse budget', + async (phase) => { + enableAnimations(); + + let finish: (value: boolean) => void = () => {}; + const pending = new Promise((resolve) => { + finish = resolve; + }); + setNotificationsOverrides({ markNotificationsAsRead: vi.fn(() => pending) }); + await renderList(fixture(2), { + delayNotificationState: false, + fetchReadNotifications: false, + }); + + await page.getByTestId('notification-mark-as-read').first().click(); + await waitFor(() => + expect(document.getElementById('notification-0')?.hasAttribute('data-animating-exit')).toBe( + true, + ), + ); + + if (phase === 'after') { + await waitForRowCollapsed('notification-0'); + } + + const collapsedBeforeFailure = rowWrapperHeight('notification-0') < 2; + expect(collapsedBeforeFailure).toBe(phase === 'after'); + + await act(async () => { + finish(false); + await pending; + }); + + await waitFor(() => { + expect(rowWrapperHeight('notification-0')).toBeGreaterThan(20); + expect(document.getElementById('notification-0')?.hasAttribute('data-animating-exit')).toBe( + false, + ); + }); + expect( + document + .getElementById('notification-0') + ?.querySelector('[data-testid="notification-mark-as-read"]'), + ).not.toBeNull(); + + useNotificationActionFailuresStore + .getState() + .setFailure(getNotificationFailureKey(account, 'notification-0'), { + action: 'markAsRead', + error: { title: 'Action Forbidden', descriptions: [], emojis: [] }, + }); + await waitFor(() => { + expect( + document + .getElementById('notification-0') + ?.querySelector('[data-testid="notification-mark-as-read"]') + ?.getAttribute('title'), + ).toContain('Action Forbidden'); + }); + }, + ); + + it('collapses a repository group as one unit and restores a partial failure', async () => { + enableAnimations(); + + let finish: (value: boolean) => void = () => {}; + const pending = new Promise((resolve) => { + finish = resolve; + }); + setNotificationsOverrides({ markNotificationsAsRead: vi.fn(() => pending) }); + const tree = await renderList(fixture(2), { + delayNotificationState: false, + fetchReadNotifications: false, + }); + const original = getMockedNotificationsState().notifications; + + await page.getByTestId('repository-mark-as-read').first().click(); + await waitFor(() => + expect(document.getElementById('notification-0')?.hasAttribute('data-animating-exit')).toBe( + true, + ), + ); + + await waitFor( + () => { + expect(repositoryWrapperHeight()).toBeLessThan(2); + expect(rowWrapperHeight('notification-0')).toBeLessThan(2); + expect(rowWrapperHeight('notification-1')).toBeLessThan(2); + }, + { timeout: 2000 }, + ); + + const remaining = original.map((entry) => ({ + ...entry, + notifications: entry.notifications.filter( + (notification) => notification.id !== 'notification-0', + ), + })); + await act(async () => { + setNotificationsOverrides({ notifications: remaining }); + tree.rerender(); + finish(false); + await pending; + }); + + await waitFor(() => { + expect(document.getElementById('notification-0')).toBeNull(); + expect(repositoryWrapperHeight()).toBeGreaterThan(20); + expect(rowWrapperHeight('notification-1')).toBeGreaterThan(20); + }); + expect(document.getElementById('notification-1')?.hasAttribute('data-animating-exit')).toBe( + false, + ); + expect(document.querySelector('[data-testid="open-repository"]')?.textContent).toContain('(1)'); + }); + + it('mounts a pending exiting row already collapsed after a remount', async () => { + enableAnimations(); + + let finish: (value: boolean) => void = () => {}; + const pending = new Promise((resolve) => { + finish = resolve; + }); + setNotificationsOverrides({ markNotificationsAsRead: vi.fn(() => pending) }); + await renderList(fixture(300), { + delayNotificationState: false, + fetchReadNotifications: false, + }); + + await page.getByTestId('notification-mark-as-read').first().click(); + await waitFor(() => + expect(document.getElementById('notification-0')?.hasAttribute('data-animating-exit')).toBe( + true, + ), + ); + await waitForRowCollapsed('notification-0'); + + await scrollToEnd(); + await waitFor(() => expect(document.getElementById('notification-0')).toBeNull()); + await scrollToStart(); + await waitFor(() => expect(document.getElementById('notification-0')).not.toBeNull()); + + expect(document.getElementById('notification-0')?.hasAttribute('data-animating-exit')).toBe( + true, + ); + expect(rowWrapperHeight('notification-0')).toBeLessThan(2); + + await act(async () => { + finish(false); + await pending; + }); + }); + + it('collapses without the slide when reduced motion is preferred', async () => { + enableAnimations(); + const session = cdp() as unknown as CdpSession; + + let finish: (value: boolean) => void = () => {}; + const pending = new Promise((resolve) => { + finish = resolve; + }); + + try { + await session.send('Emulation.setEmulatedMedia', { + features: [{ name: 'prefers-reduced-motion', value: 'reduce' }], + }); + + setNotificationsOverrides({ markNotificationsAsRead: vi.fn(() => pending) }); + await renderList(fixture(2), { + delayNotificationState: false, + fetchReadNotifications: false, + }); + + await page.getByTestId('notification-mark-as-read').first().click(); + await waitFor(() => + expect(document.getElementById('notification-0')?.hasAttribute('data-animating-exit')).toBe( + true, + ), + ); + + await waitFor(() => expect(rowWrapperHeight('notification-0')).toBeLessThan(2), { + timeout: 300, + }); + + const motionLayer = document + .getElementById('notification-0') + ?.closest('[data-exit-motion]'); + expect(motionLayer).not.toBeNull(); + expect(getComputedStyle(motionLayer!).transitionProperty).toBe('none'); + + await act(async () => { + finish(false); + await pending; + }); + } finally { + await session.send('Emulation.setEmulatedMedia', { + features: [{ name: 'prefers-reduced-motion', value: 'no-preference' }], + }); + } + }); + + it('keeps the visible anchor when an exiting row above the viewport collapses', async () => { + enableAnimations(); + + // Give the test time to scroll and capture the anchor before the collapse. + const delayStyle = document.createElement('style'); + delayStyle.textContent = '.delay-350 { transition-delay: 1000ms !important; }'; + document.head.appendChild(delayStyle); + + let finish: (value: boolean) => void = () => {}; + const pending = new Promise((resolve) => { + finish = resolve; + }); + + try { + setNotificationsOverrides({ markNotificationsAsRead: vi.fn(() => pending) }); + await renderList(fixture(20), { + delayNotificationState: false, + fetchReadNotifications: false, + }); + + await act(async () => { + scrollContainer().scrollTop = 5 * 58; + }); + await waitFor(() => expect(document.getElementById('notification-5')).not.toBeNull()); + + const action = document + .getElementById('notification-5') + ?.querySelector('[data-testid="notification-mark-as-read"]'); + if (!action) { + throw new Error('Notification action is missing'); + } + await page.elementLocator(action).click(); + await waitFor(() => + expect(document.getElementById('notification-5')?.hasAttribute('data-animating-exit')).toBe( + true, + ), + ); + + await act(async () => { + scrollContainer().scrollTop += 80; + }); + await waitFor(() => expect(document.getElementById('notification-8')).not.toBeNull()); + const anchorTopBefore = document + .getElementById('notification-8')! + .getBoundingClientRect().top; + expect(rowWrapperHeight('notification-5')).toBeGreaterThan(20); + + await waitForRowCollapsed('notification-5'); + const anchorTopAfter = document.getElementById('notification-8')!.getBoundingClientRect().top; + expect(Math.abs(anchorTopAfter - anchorTopBefore)).toBeLessThan(4); + } finally { + delayStyle.remove(); + await act(async () => { + finish(false); + await pending; + }); + } + }); }); diff --git a/src/renderer/components/notifications/NotificationRow.test.tsx b/src/renderer/components/notifications/NotificationRow.test.tsx index d9654ae43..7c502058a 100644 --- a/src/renderer/components/notifications/NotificationRow.test.tsx +++ b/src/renderer/components/notifications/NotificationRow.test.tsx @@ -61,6 +61,46 @@ describe('renderer/components/notifications/NotificationRow.tsx', () => { expect(screen.getByRole('button', { name: 'Unsubscribe from thread' })).toBeInTheDocument(); }); + it('marks the exit wrapper while animating out', () => { + renderWithProviders( + , + ); + + expect(document.querySelector('[data-exit-motion]')).toBeInTheDocument(); + expect(document.querySelector('[data-exit-collapse]')).toBeInTheDocument(); + expect(screen.queryByTestId('notification-mark-as-read')).not.toBeInTheDocument(); + }); + + it('clears the exit wrapper and restores actions when the action settles', async () => { + let resolveAction: () => void = () => {}; + const markNotificationsAsReadMock = vi.fn().mockImplementation( + () => + new Promise((resolve) => { + resolveAction = resolve; + }), + ); + + renderWithProviders( + , + { + settings: { ...mockSettings, delayNotificationState: false, fetchReadNotifications: false }, + markNotificationsAsRead: markNotificationsAsReadMock, + }, + ); + + await userEvent.click(screen.getByTestId('notification-mark-as-read')); + + expect(document.querySelector('[data-exit-collapse]')).toBeInTheDocument(); + expect(screen.queryByTestId('notification-mark-as-read')).not.toBeInTheDocument(); + + await act(async () => { + resolveAction(); + }); + + expect(document.querySelector('[data-exit-collapse]')).toBeNull(); + expect(screen.getByTestId('notification-mark-as-read')).toBeInTheDocument(); + }); + it('renders the reference when grouped by repository', () => { const props: NotificationRowProps = { notification: mockGitifyNotification, @@ -513,7 +553,9 @@ describe('renderer/components/notifications/NotificationRow.tsx', () => { await userEvent.click(screen.getByTestId('notification-mark-as-read')); // While the retry is still in flight, the row is animating out again - - // its hover actions are hidden, exactly like the very first attempt. + // its hover actions are hidden and its exit wrapper is active, exactly + // like the very first attempt. + expect(document.querySelector('[data-exit-collapse]')).toBeInTheDocument(); expect(screen.queryByTestId('notification-mark-as-read')).not.toBeInTheDocument(); // The retry fails again; the store still has a (new) failure entry for @@ -523,6 +565,7 @@ describe('renderer/components/notifications/NotificationRow.tsx', () => { resolveRetry(); }); + expect(document.querySelector('[data-exit-collapse]')).toBeNull(); await screen.findByTestId('notification-mark-as-read'); }); }); diff --git a/src/renderer/components/notifications/NotificationRow.tsx b/src/renderer/components/notifications/NotificationRow.tsx index f81a4bd43..fd565607f 100644 --- a/src/renderer/components/notifications/NotificationRow.tsx +++ b/src/renderer/components/notifications/NotificationRow.tsx @@ -24,6 +24,7 @@ import { import { isGroupByDate } from '../../utils/notifications/group'; import { shouldRemoveNotificationsFromState } from '../../utils/notifications/remove'; import { openNotification } from '../../utils/system/links'; +import { ExitCollapse } from './ExitCollapse'; import { NotificationFooter } from './NotificationFooter'; import { NotificationHeader } from './NotificationHeader'; import { NotificationTitle } from './NotificationTitle'; @@ -99,108 +100,110 @@ export function NotificationRow({ const isNotificationRead = !notification.unread; return ( -
- - - - - - - + +
+ + + + - - {/* Hidden (keeping its space) while the row is hovered, so the + + + + + {/* Hidden (keeping its space) while the row is hovered, so the full-height action rail can never leave the number partially rendered. */} - - {notification.display.identifier} - + + {notification.display.identifier} + + + + - - - - - {!isAnimatingExit && ( - - - - - - - - )} -
+ + {!isAnimatingExit && ( + + + + + + + + )} +
+ ); } diff --git a/src/renderer/components/notifications/RepositoryHeader.test.tsx b/src/renderer/components/notifications/RepositoryHeader.test.tsx index 37b8da748..b72c0e135 100644 --- a/src/renderer/components/notifications/RepositoryHeader.test.tsx +++ b/src/renderer/components/notifications/RepositoryHeader.test.tsx @@ -1,5 +1,6 @@ -import { screen } from '@testing-library/react'; +import { act, screen } from '@testing-library/react'; import userEvent from '@testing-library/user-event'; +import { useState } from 'react'; import { renderWithProviders } from '../../__helpers__/test-utils'; import { mockGitHubCloudGitifyNotifications } from '../../__mocks__/notifications-mocks'; @@ -22,6 +23,21 @@ describe('renderer/components/notifications/RepositoryHeader.tsx', () => { onAnimateExit: vi.fn(), }; + function ControlledRepositoryHeader() { + const [isAnimatingExit, setAnimatingExit] = useState(false); + + return ( + + ); + } + it('renders the repository and its actions', () => { renderWithProviders(); @@ -132,6 +148,34 @@ describe('renderer/components/notifications/RepositoryHeader.tsx', () => { ).not.toBeInTheDocument(); }); + it('marks the exit wrapper while a group action is pending and clears it on failure', async () => { + let resolveAction: () => void = () => {}; + const markNotificationsAsReadMock = vi.fn().mockImplementation( + () => + new Promise((resolve) => { + resolveAction = () => resolve(false); + }), + ); + + renderWithProviders(, { + settings: { ...mockSettings }, + markNotificationsAsRead: markNotificationsAsReadMock, + }); + + await userEvent.click(screen.getByRole('button', { name: 'Mark repository as read' })); + + expect(document.querySelector('[data-exit-motion]')).toBeInTheDocument(); + expect(document.querySelector('[data-exit-collapse]')).toBeInTheDocument(); + + await act(async () => { + resolveAction(); + }); + + expect(document.querySelector('[data-exit-motion]')).toBeNull(); + expect(document.querySelector('[data-exit-collapse]')).toBeNull(); + expect(screen.getByRole('button', { name: 'Mark repository as read' })).toBeInTheDocument(); + }); + it('clears the group exit animation when the action settles', async () => { const onAnimateExit = vi.fn(); diff --git a/src/renderer/components/notifications/RepositoryHeader.tsx b/src/renderer/components/notifications/RepositoryHeader.tsx index 0945dbee7..6cdbc27aa 100644 --- a/src/renderer/components/notifications/RepositoryHeader.tsx +++ b/src/renderer/components/notifications/RepositoryHeader.tsx @@ -19,6 +19,7 @@ import { openRepository } from '../../utils/system/links'; import { getChevronDetails } from '../../utils/ui/display'; import { getChevronIcon } from '../../utils/ui/icons'; import { AvatarWithFallback } from '../avatars/AvatarWithFallback'; +import { ExitCollapse } from './ExitCollapse'; export interface RepositoryHeaderProps { repoNotifications: GitifyNotification[]; @@ -58,67 +59,69 @@ export function RepositoryHeader({ const chevron = getChevronDetails(true, !isCollapsed, 'repository'); return ( - - - - {!isAnimatingExit && ( - - runGroupAction(() => markNotificationsAsRead(repoNotifications))} - enabled={!areAllRepoNotificationsRead} - icon={ReadIcon} - label={t('repository.mark_as_read', 'Mark repository as read')} - testid="repository-mark-as-read" + + + {!isAnimatingExit && ( + + runGroupAction(() => markNotificationsAsRead(repoNotifications))} + enabled={!areAllRepoNotificationsRead} + icon={ReadIcon} + label={t('repository.mark_as_read', 'Mark repository as read')} + testid="repository-mark-as-read" + /> + + runGroupAction(() => markNotificationsAsDone(repoNotifications))} + enabled={ + isMarkAsDoneFeatureSupported(repoNotifications[0].account) && + !areAllRepoNotificationsRead + } + icon={CheckIcon} + label={t('repository.mark_as_done', 'Mark repository as done')} + testid="repository-mark-as-done" + /> + + + + )} + + ); } From 241b61853caa0886cac0d80b60c64ea43cef9272 Mon Sep 17 00:00:00 2001 From: Adam Setch Date: Sun, 1 Nov 2026 10:47:33 -0500 Subject: [PATCH 2/8] fix(notifications): snap notification exit layout Close the row's layout space at the end of the 350ms slide instead of animating a further 150ms height transition. This matches main's instant vertical reflow while keeping the gap bounded on slow requests. --- .../components/notifications/ExitCollapse.test.tsx | 2 +- src/renderer/components/notifications/ExitCollapse.tsx | 9 ++++----- 2 files changed, 5 insertions(+), 6 deletions(-) diff --git a/src/renderer/components/notifications/ExitCollapse.test.tsx b/src/renderer/components/notifications/ExitCollapse.test.tsx index c070dba5e..fd2b0271b 100644 --- a/src/renderer/components/notifications/ExitCollapse.test.tsx +++ b/src/renderer/components/notifications/ExitCollapse.test.tsx @@ -31,7 +31,7 @@ describe('renderer/components/notifications/ExitCollapse.tsx', () => { ); expect(tree.container.querySelector('[data-exit-collapse]')).toHaveClass( 'grid-rows-[0fr]', - 'duration-150', + 'duration-0', 'delay-350', 'motion-reduce:transition-none', ); diff --git a/src/renderer/components/notifications/ExitCollapse.tsx b/src/renderer/components/notifications/ExitCollapse.tsx index eae1541ed..d1aa91f38 100644 --- a/src/renderer/components/notifications/ExitCollapse.tsx +++ b/src/renderer/components/notifications/ExitCollapse.tsx @@ -14,10 +14,9 @@ export interface ExitCollapseProps { * emits `translate-x-full` as the `translate` property, so the transition list * must include `translate` as well as `transform`; transitioning only * `transform` makes the row jump right instead of sliding. The inner grid - * layer then collapses the content's layout space over 150ms, so a slow or - * hanging request cannot leave an invisible row holding its height. The - * collapse starts after the slide via the matching `delay-350`, bounding the - * whole exit at 500ms. + * layer then snaps the layout closed when the slide finishes, so a slow or + * hanging request cannot leave an invisible row holding its height. The fixed + * budget is the 350ms slide. * * The transition classes stay mounted so a failed action can slide back in * smoothly, while the collapse transition only exists while exiting so the @@ -37,7 +36,7 @@ export function ExitCollapse({ isExiting, children }: Readonly Date: Sun, 1 Nov 2026 10:58:53 -0500 Subject: [PATCH 3/8] fix(notifications): keep long notification titles inside the list The exit wrapper's grid needs an explicit single column and min-width constraints so an unwrapped long title cannot widen the row and drag the hover action rail offscreen. Add browser coverage for the row width and rail alignment. --- .../components/notifications/ExitCollapse.tsx | 10 ++++-- .../NotificationList.visual.test.tsx | 33 +++++++++++++++++++ .../notifications/NotificationRow.tsx | 7 ++-- .../notifications/RepositoryHeader.tsx | 2 +- 4 files changed, 47 insertions(+), 5 deletions(-) diff --git a/src/renderer/components/notifications/ExitCollapse.tsx b/src/renderer/components/notifications/ExitCollapse.tsx index d1aa91f38..770746d2a 100644 --- a/src/renderer/components/notifications/ExitCollapse.tsx +++ b/src/renderer/components/notifications/ExitCollapse.tsx @@ -18,6 +18,12 @@ export interface ExitCollapseProps { * hanging request cannot leave an invisible row holding its height. The fixed * budget is the 350ms slide. * + * `grid-cols-1` is load-bearing: a single implicit auto column would size to + * the row's max-content width, so a long, unwrapped notification title would + * widen the row past the list viewport and drag the absolutely positioned + * action rail off with it. The explicit `minmax(0, 1fr)` track keeps the row at + * the list width so the title truncates/wraps and the rail stays put. + * * The transition classes stay mounted so a failed action can slide back in * smoothly, while the collapse transition only exists while exiting so the * layout snaps back as soon as the row is restored. Reduced-motion users skip @@ -34,14 +40,14 @@ export function ExitCollapse({ isExiting, children }: Readonly
-
{children}
+
{children}
); diff --git a/src/renderer/components/notifications/NotificationList.visual.test.tsx b/src/renderer/components/notifications/NotificationList.visual.test.tsx index 08b0773fc..4271436c5 100644 --- a/src/renderer/components/notifications/NotificationList.visual.test.tsx +++ b/src/renderer/components/notifications/NotificationList.visual.test.tsx @@ -212,6 +212,39 @@ describe('notification list in a browser', () => { }, ); + it('keeps a long unwrapped title and its action rail inside the list', async () => { + const longTitle = `https://example.com/${'a'.repeat(300)}`; + await renderList([], { wrapNotificationTitle: false }, [ + { + account, + notifications: [ + { + ...mockGitifyNotification, + id: 'long-title', + subject: { ...mockGitifyNotification.subject, title: longTitle }, + }, + ], + error: null, + }, + ]); + + const row = document.getElementById('long-title'); + if (!row) { + throw new Error('Long-title notification row is missing'); + } + + const rowBox = row.getBoundingClientRect(); + expect(rowBox.width).toBeLessThanOrEqual(scrollContainer().getBoundingClientRect().width); + + await page.getByTestId('notification-row').first().hover(); + const rail = row.querySelector('.gitify-hover-actions'); + if (!rail) { + throw new Error('Action rail is missing from the long-title row'); + } + await waitFor(() => expect(getComputedStyle(rail).opacity).toBe('1')); + expect(rail.getBoundingClientRect().right).toBeCloseTo(rowBox.right, 0); + }); + it.each(['notification', 'repository'])( 'retains a pending %s action across remounts and restores it on failure', async (kind) => { diff --git a/src/renderer/components/notifications/NotificationRow.tsx b/src/renderer/components/notifications/NotificationRow.tsx index fd565607f..6814a300b 100644 --- a/src/renderer/components/notifications/NotificationRow.tsx +++ b/src/renderer/components/notifications/NotificationRow.tsx @@ -106,7 +106,7 @@ export function NotificationRow({ 'group relative border-b', 'pl-2.75 pr-1 py-0.75', 'text-gitify-font border-gitify-notification-border hover:bg-gitify-notification-hover', - 'min-h-0', + 'min-h-0 min-w-0', isNotificationRead && Opacity.READ, )} data-animating-exit={isAnimatingExit || undefined} @@ -124,7 +124,10 @@ export function NotificationRow({ Date: Sun, 1 Nov 2026 11:39:03 -0500 Subject: [PATCH 4/8] feat(renderer): adopt React Router 8 navigation Upgrade the renderer to react-router 8.5.0 and add a shared navigation wrapper that starts route transitions for React view transitions. Add route enter/exit CSS with reduced-motion handling, retain the notifications route with Activity, and refresh notification alerts through useEffectEvent. Keep the tray effect unchanged because it has no resubscription work to remove. Add route, transition, rapid-navigation, reduced-motion, retention, and Electron smoke coverage. Document the deferred change-02 focus-intent dependency. --- ARCHITECTURE.md | 2 +- package.json | 2 +- pnpm-lock.yaml | 60 ++--- scripts/verifyPackagedDependencies.js | 2 +- src/renderer/App.css | 47 ++++ src/renderer/App.test.tsx | 156 ++++++++++++ src/renderer/App.tsx | 147 +++++------ src/renderer/AppNavigation.visual.test.tsx | 238 ++++++++++++++++++ src/renderer/__helpers__/test-utils.tsx | 2 +- src/renderer/__helpers__/visual-utils.tsx | 2 +- src/renderer/__helpers__/vitest.setup.ts | 4 +- .../components/GlobalEffects.test.tsx | 13 + src/renderer/components/Oops.tsx | 5 +- .../components/layout/AppLayout.test.tsx | 10 + src/renderer/components/layout/AppLayout.tsx | 8 +- .../__snapshots__/AppLayout.test.tsx.snap | 1 + .../LoginWithPersonalAccessTokenForm.tsx | 7 +- src/renderer/components/primitives/Header.tsx | 6 +- .../components/settings/InboxSettings.tsx | 4 +- .../notifications/useNotificationEffects.ts | 58 ++--- src/renderer/hooks/useAppNavigate.test.ts | 45 ++++ src/renderer/hooks/useAppNavigate.ts | 29 +++ src/renderer/hooks/useNotifications.test.tsx | 41 +++ src/renderer/hooks/useShortcutActions.ts | 15 +- src/renderer/routes/AccountScopes.test.tsx | 4 +- src/renderer/routes/AccountScopes.tsx | 2 +- src/renderer/routes/Accounts.tsx | 12 +- src/renderer/routes/Login.tsx | 6 +- .../routes/github/DeviceFlowAuthorize.tsx | 7 +- .../routes/github/LoginWithCustomOAuthApp.tsx | 2 +- .../routes/github/LoginWithGitHubCLI.tsx | 7 +- 31 files changed, 765 insertions(+), 179 deletions(-) create mode 100644 src/renderer/App.test.tsx create mode 100644 src/renderer/AppNavigation.visual.test.tsx create mode 100644 src/renderer/hooks/useAppNavigate.test.ts create mode 100644 src/renderer/hooks/useAppNavigate.ts diff --git a/ARCHITECTURE.md b/ARCHITECTURE.md index 023f2f2ad..1785fd4cc 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: `AppRoutes` retains the notifications route in a hidden React `Activity` while authenticated, keeps every other screen on ``, and preserves the logged-out redirect on `/`. `AppLayout` wraps only the routed content in the route `ViewTransition`; the sidebar and window chrome stay outside it. `GlobalEffects` remains outside the router and the retained route 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.css b/src/renderer/App.css index 6c36f546f..2b83391a0 100644 --- a/src/renderer/App.css +++ b/src/renderer/App.css @@ -565,6 +565,53 @@ body, } } +/** + * Route changes use React's `` boundary. Only the + * routed content participates; the sidebar and window chrome stay put. + */ +@keyframes gitify-route-out { + from { + opacity: 1; + transform: translateY(0); + } + to { + opacity: 0; + transform: translateY(-4px); + } +} + +@keyframes gitify-route-in { + from { + opacity: 0; + transform: translateY(6px); + } + to { + opacity: 1; + transform: translateY(0); + } +} + +::view-transition-old(route) { + animation: gitify-route-out 120ms ease-in both; +} + +::view-transition-new(route) { + animation: gitify-route-in 180ms cubic-bezier(0.22, 1, 0.36, 1) both; +} + +@media (prefers-reduced-motion: reduce) { + ::view-transition-group(root), + ::view-transition-old(root), + ::view-transition-new(root), + ::view-transition-image-pair(root), + ::view-transition-group(route), + ::view-transition-old(route), + ::view-transition-new(route), + ::view-transition-image-pair(route) { + animation: none; + } +} + .gitify-scroll-fade { --gitify-scroll-fade-size: 1.5rem; diff --git a/src/renderer/App.test.tsx b/src/renderer/App.test.tsx new file mode 100644 index 000000000..ea6f6f331 --- /dev/null +++ b/src/renderer/App.test.tsx @@ -0,0 +1,156 @@ +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('retains the notifications route in a hidden Activity on settings', async () => { + await renderApp('/'); + + await userEvent.click(screen.getByRole('button', { name: 'Settings' })); + expect(await screen.findByTestId('settings')).toBeInTheDocument(); + expect(screen.getByText('No new notifications')).not.toBeVisible(); + }); + + 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..05ebcc113 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 { Activity, useEffect } from 'react'; +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,80 @@ function RequireAuth({ children }: { children: React.ReactNode }) { return isLoggedIn ? children : ; } +function AppRoutes() { + const location = useLocation(); + const isLoggedIn = useAccountsStore((s) => s.isLoggedIn()); + const isNotificationsRoute = location.pathname === '/'; + + return ( + <> + {isLoggedIn && ( + + + + )} + + {null}} 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 +127,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..3c50ad89f --- /dev/null +++ b/src/renderer/AppNavigation.visual.test.tsx @@ -0,0 +1,238 @@ +import { act, render, screen, waitFor } from '@testing-library/react'; +import userEvent from '@testing-library/user-event'; + +import { cdp } from 'vite-plus/test/browser'; + +import { getMockedNotificationsState, setNotificationsOverrides } from './__helpers__/hook-mocks'; +import { mockGitHubCloudAccount } from './__mocks__/account-mocks'; +import { mockGitifyNotification } from './__mocks__/notifications-mocks'; +import { mockSettings } from './__mocks__/state-mocks'; + +import { useAccountsStore, useSettingsStore } from './stores'; + +import { type AccountNotifications, type GitifyNotification, GroupBy } from './types'; + +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(), + }; +}); + +interface TransitionRecord { + pseudoElements: Set; +} + +function getPseudoElement(animation: Animation): string | undefined { + return (animation.effect as KeyframeEffect | null)?.pseudoElement ?? undefined; +} + +function spyOnViewTransitions() { + const original = document.startViewTransition.bind(document); + const startViewTransition = vi.spyOn(document, 'startViewTransition'); + const records: TransitionRecord[] = []; + + startViewTransition.mockImplementation((update) => { + const transition = original(update); + void transition.ready + .then(() => { + const pseudoElements = new Set( + document + .getAnimations() + .map(getPseudoElement) + .filter( + (pseudoElement): pseudoElement is string => + pseudoElement?.startsWith('::view-transition') ?? false, + ), + ); + records.push({ pseudoElements }); + }) + .catch(() => { + // A skipped transition still leaves the route change complete. + }); + return transition; + }); + + return { startViewTransition, records }; +} + +function hasAnimatedViewTransition() { + return document.getAnimations().some((animation) => { + if (!getPseudoElement(animation)?.startsWith('::view-transition')) { + return false; + } + const duration = animation.effect?.getComputedTiming().duration; + return typeof duration === 'number' && duration > 0; + }); +} + +function viewTransitionAnimations() { + return document + .getAnimations() + .filter((animation) => getPseudoElement(animation)?.startsWith('::view-transition')); +} + +const notificationFixture = (count: number) => + Array.from({ length: count }, (_, index) => ({ + ...mockGitifyNotification, + id: `notification-${index}`, + order: index, + subject: { ...mockGitifyNotification.subject, title: `Notification ${index}` }, + })); + +function renderAtRoot( + notifications: GitifyNotification[] = getMockedNotificationsState().notifications.flatMap( + (entry) => entry.notifications, + ), +) { + const accountNotifications: AccountNotifications[] = [ + { + account: mockGitHubCloudAccount, + notifications, + error: null, + }, + ]; + + useAccountsStore.setState({ accounts: [mockGitHubCloudAccount] }); + useSettingsStore.setState({ ...mockSettings, groupBy: GroupBy.REPOSITORY }); + setNotificationsOverrides({ + notifications: accountNotifications, + notificationCount: notifications.length, + hasNotifications: notifications.length > 0, + }); + window.history.replaceState(null, '', '#/'); + + return render(); +} + +function scrollContainer() { + const element = document.querySelector('.overflow-y-auto'); + if (!element) { + throw new Error('Notification scroll container is missing'); + } + return element; +} + +async function setReducedMotion(value: 'reduce' | 'no-preference') { + const session = cdp() as unknown as { + send: (method: string, params: unknown) => Promise; + }; + await session.send('Emulation.setEmulatedMedia', { + features: [{ name: 'prefers-reduced-motion', value }], + }); +} + +afterEach(async () => { + vi.restoreAllMocks(); + await setReducedMotion('no-preference'); +}); + +describe('renderer/App route transitions', () => { + it('transitions between the notification list and settings and leaves the destination interactive', async () => { + const { startViewTransition, records } = spyOnViewTransitions(); + + renderAtRoot(); + + await userEvent.click(screen.getByRole('button', { name: 'Settings' })); + expect(await screen.findByTestId('settings')).toBeInTheDocument(); + expect(startViewTransition).toHaveBeenCalled(); + + await userEvent.click(screen.getByRole('button', { name: 'Go back' })); + expect(await screen.findByText('No new notifications')).toBeInTheDocument(); + + await userEvent.click(screen.getByRole('button', { name: 'Settings' })); + expect(await screen.findByTestId('settings')).toBeInTheDocument(); + + await waitFor(() => { + expect(records.length).toBeGreaterThanOrEqual(2); + }); + const pseudoElements = new Set(records.flatMap((record) => [...record.pseudoElements])); + expect(pseudoElements).toContain('::view-transition-old(route)'); + expect(pseudoElements).toContain('::view-transition-new(route)'); + }); + + it('swaps the route without animation when reduced motion is active', async () => { + await setReducedMotion('reduce'); + expect(window.matchMedia('(prefers-reduced-motion: reduce)').matches).toBe(true); + + const { startViewTransition } = spyOnViewTransitions(); + + renderAtRoot(); + + await userEvent.click(screen.getByRole('button', { name: 'Settings' })); + expect(await screen.findByTestId('settings')).toBeInTheDocument(); + + expect(startViewTransition).toHaveBeenCalled(); + await new Promise((resolve) => setTimeout(resolve, 50)); + expect(hasAnimatedViewTransition()).toBe(false); + }); + + it('preserves the notification scroll offset across a settings round trip', async () => { + renderAtRoot(notificationFixture(20)); + + await waitFor(() => expect(document.getElementById('notification-0')).not.toBeNull()); + await act(async () => { + scrollContainer().scrollTop = 5 * 58; + }); + await waitFor(() => expect(document.getElementById('notification-5')).not.toBeNull()); + const scrollTopBefore = scrollContainer().scrollTop; + expect(scrollTopBefore).toBeGreaterThan(0); + + await userEvent.click(screen.getByRole('button', { name: 'Settings' })); + expect(await screen.findByTestId('settings')).toBeInTheDocument(); + + await userEvent.click(screen.getByRole('button', { name: 'Go back' })); + await waitFor(() => expect(document.getElementById('notification-5')).not.toBeNull()); + expect(scrollContainer().scrollTop).toBeCloseTo(scrollTopBefore, 0); + }); + + it('preserves a collapsed repository across a settings round trip', async () => { + renderAtRoot(notificationFixture(4)); + + await waitFor(() => + expect(screen.getByRole('button', { name: 'Hide repository notifications' })).toBeVisible(), + ); + await userEvent.click(screen.getByTestId('repository-toggle')); + expect( + await screen.findByRole('button', { name: 'Show repository notifications' }), + ).toBeInTheDocument(); + + await userEvent.click(screen.getByRole('button', { name: 'Settings' })); + expect(await screen.findByTestId('settings')).toBeInTheDocument(); + + await userEvent.click(screen.getByRole('button', { name: 'Go back' })); + expect( + await screen.findByRole('button', { name: 'Show repository notifications' }), + ).toBeInTheDocument(); + }); + + it('applies a runtime reduced-motion change to the next route change', async () => { + await setReducedMotion('no-preference'); + + const { startViewTransition, records } = spyOnViewTransitions(); + + renderAtRoot(); + + await userEvent.click(screen.getByRole('button', { name: 'Settings' })); + expect(await screen.findByTestId('settings')).toBeInTheDocument(); + await waitFor(() => { + expect(records).toHaveLength(1); + }); + expect(records[0].pseudoElements.size).toBeGreaterThan(0); + await Promise.all(viewTransitionAnimations().map((animation) => animation.finished)); + + await setReducedMotion('reduce'); + + await userEvent.click(screen.getByRole('button', { name: 'Go back' })); + expect(await screen.findByText('No new notifications')).toBeInTheDocument(); + expect(startViewTransition).toHaveBeenCalledTimes(1); + await new Promise((resolve) => setTimeout(resolve, 50)); + expect(hasAnimatedViewTransition()).toBe(false); + }); +}); 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..78c11a0bd 100644 --- a/src/renderer/components/layout/AppLayout.tsx +++ b/src/renderer/components/layout/AppLayout.tsx @@ -1,4 +1,4 @@ -import type { ReactNode } from 'react'; +import { type ReactNode, ViewTransition } from 'react'; import { useTranslation } from 'react-i18next'; import { Sidebar } from '../Sidebar'; @@ -31,7 +31,11 @@ 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..3937ec1f2 --- /dev/null +++ b/src/renderer/hooks/useAppNavigate.ts @@ -0,0 +1,29 @@ +import { startTransition, useCallback, addTransitionType } from 'react'; +import { type NavigateOptions, type To, useNavigate } from 'react-router'; + +export type AppNavigate = (to: To | number, options?: NavigateOptions) => void; + +/** + * Navigates inside a React transition so the routed content can participate in + * a view transition. This is the only supported way to navigate within the + * renderer; use it instead of the raw router hook. + */ +export function useAppNavigate(): AppNavigate { + const navigate = useNavigate(); + + return useCallback( + (to, options) => { + startTransition(() => { + addTransitionType('route'); + 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( From 84fc5e552569b53fa016f28a4e29c0d16828a868 Mon Sep 17 00:00:00 2001 From: Adam Setch Date: Sun, 1 Nov 2026 12:53:15 -0500 Subject: [PATCH 5/8] fix(renderer): stabilize route transitions Keep the sidebar out of the view-transition tree and use opaque route snapshots with a short slide instead of a full-page crossfade. Revert the retained Activity notifications route because keeping it mounted slowed switches to heavier views such as Filters. Park stray pointer hover before route screenshots so high-contrast visual baselines are deterministic. --- ARCHITECTURE.md | 2 +- src/renderer/App.css | 26 ++-- src/renderer/App.test.tsx | 8 -- src/renderer/App.tsx | 136 +++++++++--------- src/renderer/AppNavigation.visual.test.tsx | 103 +++---------- src/renderer/components/layout/AppLayout.tsx | 2 +- .../__snapshots__/AppLayout.test.tsx.snap | 2 +- src/renderer/routes/routes.visual.test.tsx | 17 ++- 8 files changed, 120 insertions(+), 176 deletions(-) diff --git a/ARCHITECTURE.md b/ARCHITECTURE.md index 1785fd4cc..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. `App.tsx` owns the router tree and route composition: `AppRoutes` retains the notifications route in a hidden React `Activity` while authenticated, keeps every other screen on ``, and preserves the logged-out redirect on `/`. `AppLayout` wraps only the routed content in the route `ViewTransition`; the sidebar and window chrome stay outside it. `GlobalEffects` remains outside the router and the retained route so it is still the single side-effect host. +- 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/src/renderer/App.css b/src/renderer/App.css index 2b83391a0..6e48b8d54 100644 --- a/src/renderer/App.css +++ b/src/renderer/App.css @@ -571,39 +571,41 @@ body, */ @keyframes gitify-route-out { from { - opacity: 1; transform: translateY(0); } to { - opacity: 0; - transform: translateY(-4px); + transform: translateY(-3px); } } @keyframes gitify-route-in { from { - opacity: 0; - transform: translateY(6px); + transform: translateY(4px); } to { - opacity: 1; transform: translateY(0); } } +/* Cross-fading full-page snapshots shows the outgoing and incoming content at + * partial opacity. Keep both opaque and slide them instead, so the window + * background and the old route never show through the new one. */ ::view-transition-old(route) { - animation: gitify-route-out 120ms ease-in both; + animation: gitify-route-out 120ms ease-out both; } ::view-transition-new(route) { - animation: gitify-route-in 180ms cubic-bezier(0.22, 1, 0.36, 1) both; + animation: gitify-route-in 120ms ease-out both; +} + +/* Only the route layer participates in a view transition. Leaving the sidebar + * and window chrome out of the transition tree keeps them as live DOM instead + * of static snapshots that flicker. */ +:root { + view-transition-name: none; } @media (prefers-reduced-motion: reduce) { - ::view-transition-group(root), - ::view-transition-old(root), - ::view-transition-new(root), - ::view-transition-image-pair(root), ::view-transition-group(route), ::view-transition-old(route), ::view-transition-new(route), diff --git a/src/renderer/App.test.tsx b/src/renderer/App.test.tsx index ea6f6f331..fc3a13484 100644 --- a/src/renderer/App.test.tsx +++ b/src/renderer/App.test.tsx @@ -104,14 +104,6 @@ describe('renderer/App.tsx routing', () => { expect(await screen.findByTestId('settings')).toBeInTheDocument(); }); - it('retains the notifications route in a hidden Activity on settings', async () => { - await renderApp('/'); - - await userEvent.click(screen.getByRole('button', { name: 'Settings' })); - expect(await screen.findByTestId('settings')).toBeInTheDocument(); - expect(screen.getByText('No new notifications')).not.toBeVisible(); - }); - it('navigates between settings and the notification list through app controls', async () => { await renderApp('/'); diff --git a/src/renderer/App.tsx b/src/renderer/App.tsx index 05ebcc113..83000b6c3 100644 --- a/src/renderer/App.tsx +++ b/src/renderer/App.tsx @@ -1,4 +1,4 @@ -import { Activity, useEffect } from 'react'; +import { useEffect } from 'react'; import { Navigate, Route, HashRouter as Router, Routes, useLocation } from 'react-router'; import { BaseStyles } from '@primer/react'; @@ -39,76 +39,72 @@ function RequireAuth({ children }: { children: React.ReactNode }) { } function AppRoutes() { - const location = useLocation(); - const isLoggedIn = useAccountsStore((s) => s.isLoggedIn()); - const isNotificationsRoute = location.pathname === '/'; - return ( - <> - {isLoggedIn && ( - - - - )} - - {null}} 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" - /> - - + + + + + } + 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 index 3c50ad89f..a5c998473 100644 --- a/src/renderer/AppNavigation.visual.test.tsx +++ b/src/renderer/AppNavigation.visual.test.tsx @@ -1,17 +1,13 @@ -import { act, render, screen, waitFor } from '@testing-library/react'; +import { render, screen, waitFor } from '@testing-library/react'; import userEvent from '@testing-library/user-event'; import { cdp } from 'vite-plus/test/browser'; -import { getMockedNotificationsState, setNotificationsOverrides } from './__helpers__/hook-mocks'; import { mockGitHubCloudAccount } from './__mocks__/account-mocks'; -import { mockGitifyNotification } from './__mocks__/notifications-mocks'; import { mockSettings } from './__mocks__/state-mocks'; import { useAccountsStore, useSettingsStore } from './stores'; -import { type AccountNotifications, type GitifyNotification, GroupBy } from './types'; - import { App } from './App'; vi.mock('./hooks/useAccounts', () => ({ @@ -27,12 +23,18 @@ vi.mock('./hooks/notifications/useNotificationsHost', async () => { interface TransitionRecord { pseudoElements: Set; + rootAnimated: boolean; } function getPseudoElement(animation: Animation): string | undefined { return (animation.effect as KeyframeEffect | null)?.pseudoElement ?? undefined; } +function isAnimated(animation: Animation): boolean { + const duration = animation.effect?.getComputedTiming().duration; + return typeof duration === 'number' && duration > 0; +} + function spyOnViewTransitions() { const original = document.startViewTransition.bind(document); const startViewTransition = vi.spyOn(document, 'startViewTransition'); @@ -42,16 +44,24 @@ function spyOnViewTransitions() { const transition = original(update); void transition.ready .then(() => { + const animations = document.getAnimations(); const pseudoElements = new Set( - document - .getAnimations() + animations .map(getPseudoElement) .filter( (pseudoElement): pseudoElement is string => pseudoElement?.startsWith('::view-transition') ?? false, ), ); - records.push({ pseudoElements }); + records.push({ + pseudoElements, + rootAnimated: animations.some( + (animation) => + getPseudoElement(animation)?.includes('(root)') && + getPseudoElement(animation)?.startsWith('::view-transition') && + isAnimated(animation), + ), + }); }) .catch(() => { // A skipped transition still leaves the route change complete. @@ -78,47 +88,14 @@ function viewTransitionAnimations() { .filter((animation) => getPseudoElement(animation)?.startsWith('::view-transition')); } -const notificationFixture = (count: number) => - Array.from({ length: count }, (_, index) => ({ - ...mockGitifyNotification, - id: `notification-${index}`, - order: index, - subject: { ...mockGitifyNotification.subject, title: `Notification ${index}` }, - })); - -function renderAtRoot( - notifications: GitifyNotification[] = getMockedNotificationsState().notifications.flatMap( - (entry) => entry.notifications, - ), -) { - const accountNotifications: AccountNotifications[] = [ - { - account: mockGitHubCloudAccount, - notifications, - error: null, - }, - ]; - +function renderAtRoot() { useAccountsStore.setState({ accounts: [mockGitHubCloudAccount] }); - useSettingsStore.setState({ ...mockSettings, groupBy: GroupBy.REPOSITORY }); - setNotificationsOverrides({ - notifications: accountNotifications, - notificationCount: notifications.length, - hasNotifications: notifications.length > 0, - }); + useSettingsStore.setState(mockSettings); window.history.replaceState(null, '', '#/'); return render(); } -function scrollContainer() { - const element = document.querySelector('.overflow-y-auto'); - if (!element) { - throw new Error('Notification scroll container is missing'); - } - return element; -} - async function setReducedMotion(value: 'reduce' | 'no-preference') { const session = cdp() as unknown as { send: (method: string, params: unknown) => Promise; @@ -155,6 +132,7 @@ describe('renderer/App route transitions', () => { const pseudoElements = new Set(records.flatMap((record) => [...record.pseudoElements])); expect(pseudoElements).toContain('::view-transition-old(route)'); expect(pseudoElements).toContain('::view-transition-new(route)'); + expect(records.every((record) => !record.rootAnimated)).toBe(true); }); it('swaps the route without animation when reduced motion is active', async () => { @@ -173,45 +151,6 @@ describe('renderer/App route transitions', () => { expect(hasAnimatedViewTransition()).toBe(false); }); - it('preserves the notification scroll offset across a settings round trip', async () => { - renderAtRoot(notificationFixture(20)); - - await waitFor(() => expect(document.getElementById('notification-0')).not.toBeNull()); - await act(async () => { - scrollContainer().scrollTop = 5 * 58; - }); - await waitFor(() => expect(document.getElementById('notification-5')).not.toBeNull()); - const scrollTopBefore = scrollContainer().scrollTop; - expect(scrollTopBefore).toBeGreaterThan(0); - - await userEvent.click(screen.getByRole('button', { name: 'Settings' })); - expect(await screen.findByTestId('settings')).toBeInTheDocument(); - - await userEvent.click(screen.getByRole('button', { name: 'Go back' })); - await waitFor(() => expect(document.getElementById('notification-5')).not.toBeNull()); - expect(scrollContainer().scrollTop).toBeCloseTo(scrollTopBefore, 0); - }); - - it('preserves a collapsed repository across a settings round trip', async () => { - renderAtRoot(notificationFixture(4)); - - await waitFor(() => - expect(screen.getByRole('button', { name: 'Hide repository notifications' })).toBeVisible(), - ); - await userEvent.click(screen.getByTestId('repository-toggle')); - expect( - await screen.findByRole('button', { name: 'Show repository notifications' }), - ).toBeInTheDocument(); - - await userEvent.click(screen.getByRole('button', { name: 'Settings' })); - expect(await screen.findByTestId('settings')).toBeInTheDocument(); - - await userEvent.click(screen.getByRole('button', { name: 'Go back' })); - expect( - await screen.findByRole('button', { name: 'Show repository notifications' }), - ).toBeInTheDocument(); - }); - it('applies a runtime reduced-motion change to the next route change', async () => { await setReducedMotion('no-preference'); diff --git a/src/renderer/components/layout/AppLayout.tsx b/src/renderer/components/layout/AppLayout.tsx index 78c11a0bd..046dd3101 100644 --- a/src/renderer/components/layout/AppLayout.tsx +++ b/src/renderer/components/layout/AppLayout.tsx @@ -32,7 +32,7 @@ export function AppLayout({ children }: Readonly) { -
+
{children}
diff --git a/src/renderer/components/layout/__snapshots__/AppLayout.test.tsx.snap b/src/renderer/components/layout/__snapshots__/AppLayout.test.tsx.snap index a47e7e6e1..0bd4c9a6e 100644 --- a/src/renderer/components/layout/__snapshots__/AppLayout.test.tsx.snap +++ b/src/renderer/components/layout/__snapshots__/AppLayout.test.tsx.snap @@ -227,7 +227,7 @@ exports[`renderer/components/layout/AppLayout.tsx > renders itself & its childre
Test 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, From 85e09303a26affdd15c189e6c26d384d2b87dd74 Mon Sep 17 00:00:00 2001 From: Adam Setch Date: Sun, 1 Nov 2026 13:02:31 -0500 Subject: [PATCH 6/8] fix(renderer): keep sidebar stable during route transitions Give the sidebar its own static view-transition snapshot so the root crossfade cannot fade it during route changes. --- src/renderer/App.css | 24 +++++++++++++++++++++--- 1 file changed, 21 insertions(+), 3 deletions(-) diff --git a/src/renderer/App.css b/src/renderer/App.css index 6e48b8d54..8e71cc48e 100644 --- a/src/renderer/App.css +++ b/src/renderer/App.css @@ -598,13 +598,31 @@ body, animation: gitify-route-in 120ms ease-out both; } -/* Only the route layer participates in a view transition. Leaving the sidebar - * and window chrome out of the transition tree keeps them as live DOM instead - * of static snapshots that flicker. */ +/* Only the route layer participates in a view transition. The sidebar gets its + * own static snapshot so the root crossfade cannot fade it. */ :root { view-transition-name: none; } +.gitify-sidebar { + view-transition-name: sidebar; +} + +::view-transition-group(sidebar), +::view-transition-image-pair(sidebar) { + animation: none; +} + +::view-transition-old(sidebar) { + animation: none; + opacity: 0; +} + +::view-transition-new(sidebar) { + animation: none; + opacity: 1; +} + @media (prefers-reduced-motion: reduce) { ::view-transition-group(route), ::view-transition-old(route), From e59cf42b80d54159cd46a205044fff7273137a45 Mon Sep 17 00:00:00 2001 From: Adam Setch Date: Sun, 1 Nov 2026 13:16:30 -0500 Subject: [PATCH 7/8] fix(renderer): remove view transitions from route changes Browser view transitions repeatedly captured and faded the sidebar despite named layers. Replace them with a route-only CSS slide and remove the route background that covered the fixed sidebar padding. --- src/renderer/App.css | 52 +------- src/renderer/AppNavigation.visual.test.tsx | 112 ++++-------------- src/renderer/components/layout/AppLayout.tsx | 16 ++- .../__snapshots__/AppLayout.test.tsx.snap | 2 +- 4 files changed, 36 insertions(+), 146 deletions(-) diff --git a/src/renderer/App.css b/src/renderer/App.css index 8e71cc48e..589b4958c 100644 --- a/src/renderer/App.css +++ b/src/renderer/App.css @@ -566,18 +566,9 @@ body, } /** - * Route changes use React's `` boundary. Only the - * routed content participates; the sidebar and window chrome stay put. + * Route changes animate only the routed content. The sidebar is outside this + * element, so it is never captured, re-rendered, or faded by the animation. */ -@keyframes gitify-route-out { - from { - transform: translateY(0); - } - to { - transform: translateY(-3px); - } -} - @keyframes gitify-route-in { from { transform: translateY(4px); @@ -587,47 +578,12 @@ body, } } -/* Cross-fading full-page snapshots shows the outgoing and incoming content at - * partial opacity. Keep both opaque and slide them instead, so the window - * background and the old route never show through the new one. */ -::view-transition-old(route) { - animation: gitify-route-out 120ms ease-out both; -} - -::view-transition-new(route) { +.gitify-route-enter { animation: gitify-route-in 120ms ease-out both; } -/* Only the route layer participates in a view transition. The sidebar gets its - * own static snapshot so the root crossfade cannot fade it. */ -:root { - view-transition-name: none; -} - -.gitify-sidebar { - view-transition-name: sidebar; -} - -::view-transition-group(sidebar), -::view-transition-image-pair(sidebar) { - animation: none; -} - -::view-transition-old(sidebar) { - animation: none; - opacity: 0; -} - -::view-transition-new(sidebar) { - animation: none; - opacity: 1; -} - @media (prefers-reduced-motion: reduce) { - ::view-transition-group(route), - ::view-transition-old(route), - ::view-transition-new(route), - ::view-transition-image-pair(route) { + .gitify-route-enter { animation: none; } } diff --git a/src/renderer/AppNavigation.visual.test.tsx b/src/renderer/AppNavigation.visual.test.tsx index a5c998473..df3318953 100644 --- a/src/renderer/AppNavigation.visual.test.tsx +++ b/src/renderer/AppNavigation.visual.test.tsx @@ -21,79 +21,26 @@ vi.mock('./hooks/notifications/useNotificationsHost', async () => { }; }); -interface TransitionRecord { - pseudoElements: Set; - rootAnimated: boolean; -} - -function getPseudoElement(animation: Animation): string | undefined { - return (animation.effect as KeyframeEffect | null)?.pseudoElement ?? undefined; -} - -function isAnimated(animation: Animation): boolean { - const duration = animation.effect?.getComputedTiming().duration; - return typeof duration === 'number' && duration > 0; -} - -function spyOnViewTransitions() { - const original = document.startViewTransition.bind(document); - const startViewTransition = vi.spyOn(document, 'startViewTransition'); - const records: TransitionRecord[] = []; - - startViewTransition.mockImplementation((update) => { - const transition = original(update); - void transition.ready - .then(() => { - const animations = document.getAnimations(); - const pseudoElements = new Set( - animations - .map(getPseudoElement) - .filter( - (pseudoElement): pseudoElement is string => - pseudoElement?.startsWith('::view-transition') ?? false, - ), - ); - records.push({ - pseudoElements, - rootAnimated: animations.some( - (animation) => - getPseudoElement(animation)?.includes('(root)') && - getPseudoElement(animation)?.startsWith('::view-transition') && - isAnimated(animation), - ), - }); - }) - .catch(() => { - // A skipped transition still leaves the route change complete. - }); - return transition; - }); +function renderAtRoot() { + useAccountsStore.setState({ accounts: [mockGitHubCloudAccount] }); + useSettingsStore.setState(mockSettings); + window.history.replaceState(null, '', '#/'); - return { startViewTransition, records }; + return render(); } -function hasAnimatedViewTransition() { - return document.getAnimations().some((animation) => { - if (!getPseudoElement(animation)?.startsWith('::view-transition')) { - return false; - } - const duration = animation.effect?.getComputedTiming().duration; - return typeof duration === 'number' && duration > 0; - }); +function routeContent() { + return screen.getByTestId('route-content'); } -function viewTransitionAnimations() { - return document +function routeAnimations() { + return routeContent() .getAnimations() - .filter((animation) => getPseudoElement(animation)?.startsWith('::view-transition')); + .filter((animation) => (animation as CSSAnimation).animationName === 'gitify-route-in'); } -function renderAtRoot() { - useAccountsStore.setState({ accounts: [mockGitHubCloudAccount] }); - useSettingsStore.setState(mockSettings); - window.history.replaceState(null, '', '#/'); - - return render(); +async function waitForRouteAnimation() { + await waitFor(() => expect(routeAnimations().length).toBeGreaterThan(0)); } async function setReducedMotion(value: 'reduce' | 'no-preference') { @@ -106,72 +53,55 @@ async function setReducedMotion(value: 'reduce' | 'no-preference') { } afterEach(async () => { - vi.restoreAllMocks(); await setReducedMotion('no-preference'); }); describe('renderer/App route transitions', () => { - it('transitions between the notification list and settings and leaves the destination interactive', async () => { - const { startViewTransition, records } = spyOnViewTransitions(); - + it('animates the routed content and leaves the destination interactive', async () => { renderAtRoot(); await userEvent.click(screen.getByRole('button', { name: 'Settings' })); expect(await screen.findByTestId('settings')).toBeInTheDocument(); - expect(startViewTransition).toHaveBeenCalled(); + expect(routeAnimations().length).toBeGreaterThan(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(); + await waitForRouteAnimation(); await userEvent.click(screen.getByRole('button', { name: 'Settings' })); expect(await screen.findByTestId('settings')).toBeInTheDocument(); - - await waitFor(() => { - expect(records.length).toBeGreaterThanOrEqual(2); - }); - const pseudoElements = new Set(records.flatMap((record) => [...record.pseudoElements])); - expect(pseudoElements).toContain('::view-transition-old(route)'); - expect(pseudoElements).toContain('::view-transition-new(route)'); - expect(records.every((record) => !record.rootAnimated)).toBe(true); }); it('swaps the route without animation when reduced motion is active', async () => { await setReducedMotion('reduce'); expect(window.matchMedia('(prefers-reduced-motion: reduce)').matches).toBe(true); - const { startViewTransition } = spyOnViewTransitions(); - renderAtRoot(); await userEvent.click(screen.getByRole('button', { name: 'Settings' })); expect(await screen.findByTestId('settings')).toBeInTheDocument(); - expect(startViewTransition).toHaveBeenCalled(); await new Promise((resolve) => setTimeout(resolve, 50)); - expect(hasAnimatedViewTransition()).toBe(false); + expect(routeAnimations()).toHaveLength(0); }); it('applies a runtime reduced-motion change to the next route change', async () => { await setReducedMotion('no-preference'); - const { startViewTransition, records } = spyOnViewTransitions(); - renderAtRoot(); await userEvent.click(screen.getByRole('button', { name: 'Settings' })); expect(await screen.findByTestId('settings')).toBeInTheDocument(); - await waitFor(() => { - expect(records).toHaveLength(1); - }); - expect(records[0].pseudoElements.size).toBeGreaterThan(0); - await Promise.all(viewTransitionAnimations().map((animation) => animation.finished)); + await waitForRouteAnimation(); + await Promise.all(routeAnimations().map((animation) => animation.finished)); await setReducedMotion('reduce'); await userEvent.click(screen.getByRole('button', { name: 'Go back' })); expect(await screen.findByText('No new notifications')).toBeInTheDocument(); - expect(startViewTransition).toHaveBeenCalledTimes(1); + await new Promise((resolve) => setTimeout(resolve, 50)); - expect(hasAnimatedViewTransition()).toBe(false); + expect(routeAnimations()).toHaveLength(0); }); }); diff --git a/src/renderer/components/layout/AppLayout.tsx b/src/renderer/components/layout/AppLayout.tsx index 046dd3101..fa9650bdd 100644 --- a/src/renderer/components/layout/AppLayout.tsx +++ b/src/renderer/components/layout/AppLayout.tsx @@ -1,5 +1,6 @@ -import { type ReactNode, ViewTransition } from 'react'; +import type { ReactNode } from 'react'; import { useTranslation } from 'react-i18next'; +import { useLocation } from 'react-router'; import { Sidebar } from '../Sidebar'; @@ -13,6 +14,7 @@ interface AppLayoutProps { */ export function AppLayout({ children }: Readonly) { const { t } = useTranslation(); + const location = useLocation(); const setFocusRef = (el: HTMLButtonElement | null) => { if (el) { el.focus(); @@ -31,11 +33,13 @@ 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 0bd4c9a6e..6d3576d0b 100644 --- a/src/renderer/components/layout/__snapshots__/AppLayout.test.tsx.snap +++ b/src/renderer/components/layout/__snapshots__/AppLayout.test.tsx.snap @@ -227,7 +227,7 @@ exports[`renderer/components/layout/AppLayout.tsx > renders itself & its childre
Test From 00484c7e468087fd8c16bdc0128b4661cbbe69d9 Mon Sep 17 00:00:00 2001 From: Adam Setch Date: Sun, 1 Nov 2026 13:24:57 -0500 Subject: [PATCH 8/8] fix(renderer): remove route animations Route changes now swap immediately. This keeps the sidebar stable and interactive and removes the animation timing that made heavy views feel slow. --- src/renderer/App.css | 23 ------ src/renderer/AppNavigation.visual.test.tsx | 71 ++++--------------- src/renderer/components/layout/AppLayout.tsx | 8 +-- .../__snapshots__/AppLayout.test.tsx.snap | 2 +- src/renderer/hooks/useAppNavigate.ts | 25 +++---- 5 files changed, 25 insertions(+), 104 deletions(-) diff --git a/src/renderer/App.css b/src/renderer/App.css index 589b4958c..6c36f546f 100644 --- a/src/renderer/App.css +++ b/src/renderer/App.css @@ -565,29 +565,6 @@ body, } } -/** - * Route changes animate only the routed content. The sidebar is outside this - * element, so it is never captured, re-rendered, or faded by the animation. - */ -@keyframes gitify-route-in { - from { - transform: translateY(4px); - } - to { - transform: translateY(0); - } -} - -.gitify-route-enter { - animation: gitify-route-in 120ms ease-out both; -} - -@media (prefers-reduced-motion: reduce) { - .gitify-route-enter { - animation: none; - } -} - .gitify-scroll-fade { --gitify-scroll-fade-size: 1.5rem; diff --git a/src/renderer/AppNavigation.visual.test.tsx b/src/renderer/AppNavigation.visual.test.tsx index df3318953..85ae17e4b 100644 --- a/src/renderer/AppNavigation.visual.test.tsx +++ b/src/renderer/AppNavigation.visual.test.tsx @@ -1,8 +1,6 @@ -import { render, screen, waitFor } from '@testing-library/react'; +import { render, screen } from '@testing-library/react'; import userEvent from '@testing-library/user-event'; -import { cdp } from 'vite-plus/test/browser'; - import { mockGitHubCloudAccount } from './__mocks__/account-mocks'; import { mockSettings } from './__mocks__/state-mocks'; @@ -33,75 +31,30 @@ function routeContent() { return screen.getByTestId('route-content'); } -function routeAnimations() { - return routeContent() - .getAnimations() - .filter((animation) => (animation as CSSAnimation).animationName === 'gitify-route-in'); -} - -async function waitForRouteAnimation() { - await waitFor(() => expect(routeAnimations().length).toBeGreaterThan(0)); -} - -async function setReducedMotion(value: 'reduce' | 'no-preference') { - const session = cdp() as unknown as { - send: (method: string, params: unknown) => Promise; - }; - await session.send('Emulation.setEmulatedMedia', { - features: [{ name: 'prefers-reduced-motion', value }], - }); -} - -afterEach(async () => { - await setReducedMotion('no-preference'); -}); - -describe('renderer/App route transitions', () => { - it('animates the routed content and leaves the destination interactive', async () => { +describe('renderer/App route changes', () => { + it('keeps sidebar controls above the routed content', async () => { renderAtRoot(); - await userEvent.click(screen.getByRole('button', { name: 'Settings' })); - expect(await screen.findByTestId('settings')).toBeInTheDocument(); - expect(routeAnimations().length).toBeGreaterThan(0); - expect(routeContent()).not.toContainElement(screen.getByRole('button', { name: 'Gitify' })); + 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); - await userEvent.click(screen.getByRole('button', { name: 'Go back' })); - expect(await screen.findByText('No new notifications')).toBeInTheDocument(); - await waitForRouteAnimation(); - - await userEvent.click(screen.getByRole('button', { name: 'Settings' })); - expect(await screen.findByTestId('settings')).toBeInTheDocument(); - }); - - it('swaps the route without animation when reduced motion is active', async () => { - await setReducedMotion('reduce'); - expect(window.matchMedia('(prefers-reduced-motion: reduce)').matches).toBe(true); - - renderAtRoot(); + expect(settings.contains(hit)).toBe(true); - await userEvent.click(screen.getByRole('button', { name: 'Settings' })); + await userEvent.click(settings); expect(await screen.findByTestId('settings')).toBeInTheDocument(); - - await new Promise((resolve) => setTimeout(resolve, 50)); - expect(routeAnimations()).toHaveLength(0); }); - it('applies a runtime reduced-motion change to the next route change', async () => { - await setReducedMotion('no-preference'); - + 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(); - await waitForRouteAnimation(); - await Promise.all(routeAnimations().map((animation) => animation.finished)); - - await setReducedMotion('reduce'); + 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(); - - await new Promise((resolve) => setTimeout(resolve, 50)); - expect(routeAnimations()).toHaveLength(0); + expect(routeContent().getAnimations()).toHaveLength(0); }); }); diff --git a/src/renderer/components/layout/AppLayout.tsx b/src/renderer/components/layout/AppLayout.tsx index fa9650bdd..af48644d6 100644 --- a/src/renderer/components/layout/AppLayout.tsx +++ b/src/renderer/components/layout/AppLayout.tsx @@ -1,6 +1,5 @@ import type { ReactNode } from 'react'; import { useTranslation } from 'react-i18next'; -import { useLocation } from 'react-router'; import { Sidebar } from '../Sidebar'; @@ -14,7 +13,6 @@ interface AppLayoutProps { */ export function AppLayout({ children }: Readonly) { const { t } = useTranslation(); - const location = useLocation(); const setFocusRef = (el: HTMLButtonElement | null) => { if (el) { el.focus(); @@ -33,11 +31,7 @@ export function AppLayout({ children }: Readonly) { -
+
{children}
diff --git a/src/renderer/components/layout/__snapshots__/AppLayout.test.tsx.snap b/src/renderer/components/layout/__snapshots__/AppLayout.test.tsx.snap index 6d3576d0b..a47e7e6e1 100644 --- a/src/renderer/components/layout/__snapshots__/AppLayout.test.tsx.snap +++ b/src/renderer/components/layout/__snapshots__/AppLayout.test.tsx.snap @@ -227,7 +227,7 @@ exports[`renderer/components/layout/AppLayout.tsx > renders itself & its childre
Test diff --git a/src/renderer/hooks/useAppNavigate.ts b/src/renderer/hooks/useAppNavigate.ts index 3937ec1f2..407a883c5 100644 --- a/src/renderer/hooks/useAppNavigate.ts +++ b/src/renderer/hooks/useAppNavigate.ts @@ -1,28 +1,25 @@ -import { startTransition, useCallback, addTransitionType } from 'react'; +import { useCallback } from 'react'; import { type NavigateOptions, type To, useNavigate } from 'react-router'; export type AppNavigate = (to: To | number, options?: NavigateOptions) => void; /** - * Navigates inside a React transition so the routed content can participate in - * a view transition. This is the only supported way to navigate within the - * renderer; use it instead of the raw router hook. + * 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) => { - startTransition(() => { - addTransitionType('route'); - if (typeof to === 'number') { - void navigate(to); - } else if (options === undefined) { - void navigate(to); - } else { - void navigate(to, options); - } - }); + if (typeof to === 'number') { + void navigate(to); + } else if (options === undefined) { + void navigate(to); + } else { + void navigate(to, options); + } }, [navigate], );