Repository navigation
Conversation
Notification rows and other action-only leaf components called `useNotifications()`, so each mounted row subscribed to the notifications query as its own observer. Every cache write then re-ran the query's `select` and its structural-sharing walk once per mounted row, which is what froze the renderer for ~8.5s when marking a notification on a 1,274-notification inbox. Add `useNotificationActions()`, which exposes the three mutations plus `removeAccountNotifications` and `refetchNotifications` without calling `useQuery`, and move the action-only consumers onto it: `NotificationRow`, `RepositoryHeader`, `primitives/Header` and `useLogins`. The notifications query key is extracted into `useNotificationsQueryKey()` so the action surface targets the same cache entry as the readers without an observer. Readers keep `useNotifications()`, which now composes the reader and action surfaces, and structural sharing stays at its default. An unchanged poll therefore remains a no-op for observers. Alternative to #3320, which fixes the same freeze by disabling structural sharing for the query and so also drops the poll-path reference stability.
The visual setup mirrors the app-hook mocks from vitest.setup.ts, but the new
useNotificationActions mock was only added to vitest.setup.ts. Visual tests
therefore rendered NotificationRow and RepositoryHeader against the real actions
hook, so `setNotificationsOverrides({ markNotificationsAsRead })` never applied:
the row stayed out of its pending/exit state and the real mutation attempted a
network request that the visual setup rejects.
Add the matching mock so action-only consumers resolve mocked actions in the
visual project too.
|
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Alternative to #3320 for the same freeze: marking a notification stalls the renderer for ~8.5s on a 1,274-notification inbox.
The dominant cost is the deep structural-sharing walk TanStack Query runs on every notification cache write, and it is multiplied once per mounted observer. Every
NotificationRow(and other action-only leaf components) calleduseNotifications(), so each mounted row subscribed to the notifications query as its own observer; a write then re-ranselectand the structural-sharing walk once per row.#3320 removes the walk by setting
structuralSharing: false. That also removes the per-observer select-result dedup, so an unchanged poll stops being a no-op: every poll re-runsselect, rebuilds the list items and re-renders consumers. This PR removes the amplification at its source instead and leaves structural sharing on.Changes
useNotificationActions()— exposesmarkNotificationsAsRead,markNotificationsAsDone,unsubscribeNotification,removeAccountNotificationsandrefetchNotificationswithout callinguseQuery.useNotificationsQueryKey()— extracts the query-key derivation from the reader so the action surface targets the same cache entry without an observer.NotificationRow,RepositoryHeader,primitives/Header,useLogins.useNotifications()now composes the reader and the action surfaces; readers (route, sidebar, filters, accounts, shortcuts) are unchanged.removeAccountNotificationsandrefetchNotificationsmoved out of the reader hook.ARCHITECTURE.mddocuments the subscription rule.Verification
pnpm test— 192 files / 1916 tests pass (28 new).pnpm checkandpnpm format:checkclean.Note
Merge one of this and #3320, not both. The matching spec change is gitify-app/gitify-specs#6 (now archived and synced into
notification-state-composition).