Skip to content

Commit 784e53a

Browse files
iHiDclaude
andauthored
Speed up reputation/notifications header dropdowns (#9400)
* Speed up reputation/notifications header dropdowns Fixes N+1 queries on track/exercise, stops the header dropdowns from eagerly refetching their full list on every page load (badge counts are inlined server-side instead, list is fetched lazily on first open), and fixes a staleTime unit bug (30ms instead of 30s) that made the reputation dropdown refetch far more aggressively than intended. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Ekpu7NEZTWCghctyM7XjTW * Fix CI: badge props/websocket live-updates broke by lazy dropdown loading - internal.tsx passed data.defaultUnreadCount but the raw JSON prop key is snake_case (default_unread_count), like the other dropdowns already do — the badge was rendering blank. - The websocket-triggered background refresh (used to keep the badge live while the dropdown is closed, even before it's ever been opened) was accidentally gated behind hasOpenedOnce in both dropdowns. Restored the original "refetch while closed" condition; refetch() works fine even while the query is enabled:false pre-first-open. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Ekpu7NEZTWCghctyM7XjTW * Fix flaky notifications dropdown system test The list now fetches lazily on first open instead of eagerly on mount, so the initial (empty) fetch can race a notification created immediately after opening. Wait for that fetch to resolve before creating the notification, rather than relying on timing. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Ekpu7NEZTWCghctyM7XjTW --------- Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
1 parent c593385 commit 784e53a

10 files changed

Lines changed: 82 additions & 18 deletions

File tree

app/assemblers/assemble_notifications_list.rb

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -39,7 +39,7 @@ def header_notifications
3939

4040
ids = ids.to_a # Sets don't have `.index`
4141

42-
notifications = User::Notification.where(id: ids).
42+
notifications = User::Notification.where(id: ids).includes(:track, :exercise).
4343
sort_by { |n| ids.index(n.id) }[0, 5]
4444

4545
Kaminari.paginate_array(notifications, total_count: notifications.size).page(1).per(5)

app/assemblers/assemble_reputation_tokens.rb

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -49,7 +49,7 @@ def header_tokens
4949

5050
ids = ids.to_a # Sets don't have `.index`
5151

52-
tokens = User::ReputationToken.where(id: ids).
52+
tokens = User::ReputationToken.where(id: ids).includes(:track, :exercise).
5353
sort_by { |rt| ids.index(rt.id) }[0, 5]
5454

5555
Kaminari.paginate_array(tokens, total_count: tokens.size).page(1).per(5)

app/commands/user/notification/retrieve.rb

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -32,7 +32,7 @@ def call
3232
end
3333

3434
def setup!
35-
@notifications = user.notifications.visible
35+
@notifications = user.notifications.visible.includes(:track, :exercise)
3636
end
3737

3838
def sort!

app/commands/user/reputation_token/search.rb

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -21,7 +21,7 @@ def initialize(user, criteria: nil, category: nil, page: nil, per: nil, order: n
2121
end
2222

2323
def call
24-
@tokens = user.reputation_tokens
24+
@tokens = user.reputation_tokens.includes(:track, :exercise)
2525

2626
filter_criteria!
2727
filter_category!

app/helpers/react_components/dropdowns/notifications.rb

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,10 +1,15 @@
11
module ReactComponents
22
module Dropdowns
33
class Notifications < ReactComponent
4+
initialize_with :user
5+
46
def to_s
57
super(
68
"dropdowns-notifications",
7-
{ endpoint: Exercism::Routes.api_notifications_url(for_header: true) },
9+
{
10+
default_unread_count: user.notifications.unread.count,
11+
endpoint: Exercism::Routes.api_notifications_url(for_header: true)
12+
},
813
persistent: true
914
)
1015
end

app/helpers/view_components/site_header.rb

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -127,7 +127,7 @@ def signed_in_section
127127
[
128128
new_testimonial_icon,
129129
new_badge_icon,
130-
ReactComponents::Dropdowns::Notifications.new.to_s,
130+
ReactComponents::Dropdowns::Notifications.new(current_user).to_s,
131131
render(ReactComponents::Dropdowns::Reputation.new(current_user)),
132132
render(ViewComponents::UserMenu.new)
133133
]

app/javascript/components/dropdowns/Notifications.tsx

Lines changed: 43 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -85,22 +85,30 @@ export const NOTIFICATIONS_CACHE_KEY = 'notifications'
8585

8686
export default function Notifications({
8787
endpoint,
88+
defaultUnreadCount,
8889
}: {
8990
endpoint: string
91+
defaultUnreadCount: number
9092
}): JSX.Element {
9193
const queryClient = useQueryClient()
94+
const [unreadCount, setUnreadCount] = useState(defaultUnreadCount)
95+
// The badge is seeded from defaultUnreadCount (rendered server-side), so we
96+
// don't need to fetch the notification list until the user actually opens
97+
// the dropdown.
98+
const [hasOpenedOnce, setHasOpenedOnce] = useState(false)
9299
const {
93100
data: resolvedData,
94101
error,
95102
status,
103+
refetch,
96104
} = usePaginatedRequestQuery<APIResponse, unknown>(
97105
[NOTIFICATIONS_CACHE_KEY],
98106
{
99107
endpoint: endpoint,
100108
query: { per_page: MAX_NOTIFICATIONS },
101109
options: {
102110
staleTime: 30 * 1000,
103-
refetchOnMount: true,
111+
enabled: hasOpenedOnce,
104112
},
105113
}
106114
)
@@ -113,32 +121,58 @@ export default function Notifications({
113121
} = useNotificationDropdown(resolvedData)
114122

115123
const connectionRef = useRef<NotificationsChannel | null>(null)
124+
const hiddenRef = useRef(listAttributes.hidden)
125+
const refetchRef = useRef(refetch)
126+
hiddenRef.current = listAttributes.hidden
127+
refetchRef.current = refetch
128+
129+
useEffect(() => {
130+
if (!resolvedData) {
131+
return
132+
}
133+
134+
setUnreadCount(resolvedData.meta.unreadCount)
135+
}, [resolvedData])
116136

117137
useEffect(() => {
118138
if (!connectionRef.current) {
119139
connectionRef.current = new NotificationsChannel((message) => {
120140
if (!message) return
121141

122-
if (message.type === 'notifications.changed' && listAttributes.hidden) {
123-
queryClient.invalidateQueries({ queryKey: [NOTIFICATIONS_CACHE_KEY] })
142+
// Refetch (which also refreshes the badge count) whenever the
143+
// dropdown is closed, even if it's never been opened yet. `refetch`
144+
// works regardless of the query's `enabled` state. While the
145+
// dropdown is open we leave the visible list alone so it doesn't
146+
// shift under the user.
147+
if (message.type === 'notifications.changed' && hiddenRef.current) {
148+
refetchRef.current()
124149
}
125150
})
126151
}
127152

128-
if (!listAttributes.hidden) {
129-
queryClient.refetchQueries({ queryKey: [NOTIFICATIONS_CACHE_KEY] })
130-
}
131-
132153
return () => {
133154
connectionRef.current?.disconnect()
134155
connectionRef.current = null
135156
}
136-
}, [listAttributes.hidden, queryClient])
157+
}, [])
158+
159+
useEffect(() => {
160+
if (listAttributes.hidden) {
161+
return
162+
}
163+
164+
if (!hasOpenedOnce) {
165+
setHasOpenedOnce(true)
166+
return
167+
}
168+
169+
queryClient.refetchQueries({ queryKey: [NOTIFICATIONS_CACHE_KEY] })
170+
}, [listAttributes.hidden, hasOpenedOnce, queryClient])
137171

138172
return (
139173
<React.Fragment>
140174
<NotificationsIcon
141-
count={resolvedData?.meta?.unreadCount || 0}
175+
count={unreadCount}
142176
aria-label="Open notifications"
143177
{...buttonAttributes}
144178
/>

app/javascript/components/dropdowns/Reputation.tsx

Lines changed: 18 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -104,6 +104,10 @@ export default function Reputation({
104104
const [isStale, setIsStale] = useState(false)
105105
const [reputation, setReputation] = useState(defaultReputation)
106106
const [isSeen, setIsSeen] = useState(defaultIsSeen)
107+
// The badge itself is seeded from defaultReputation/defaultIsSeen (rendered
108+
// server-side), so we don't need to fetch the token list until the user
109+
// actually opens the dropdown.
110+
const [hasOpenedOnce, setHasOpenedOnce] = useState(false)
107111

108112
const {
109113
data: resolvedData,
@@ -114,8 +118,8 @@ export default function Reputation({
114118
endpoint: endpoint,
115119
query: { per_page: MAX_TOKENS },
116120
options: {
117-
staleTime: 30,
118-
refetchOnMount: true,
121+
staleTime: 30 * 1000,
122+
enabled: hasOpenedOnce,
119123
},
120124
})
121125

@@ -159,6 +163,18 @@ export default function Reputation({
159163
setIsSeen(resolvedData.meta.unseenTotal === 0)
160164
}, [resolvedData])
161165

166+
useEffect(() => {
167+
if (listAttributes.hidden || hasOpenedOnce) {
168+
return
169+
}
170+
171+
setHasOpenedOnce(true)
172+
}, [listAttributes.hidden, hasOpenedOnce])
173+
174+
// Keep the badge (reputation/isSeen) fresh in the background whenever the
175+
// dropdown is closed and a websocket event marks it stale — regardless of
176+
// whether the list has ever been opened. `refetch` works even while the
177+
// query is disabled (pre-first-open).
162178
useEffect(() => {
163179
if (!listAttributes.hidden || !isStale) {
164180
return

app/javascript/packs/internal.tsx

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -597,7 +597,10 @@ initReact({
597597
),
598598
'dropdowns-notifications': (data: any) => (
599599
<Suspense fallback={<NotificationsDropdownSkeleton />}>
600-
<NotificationsDropdown endpoint={data.endpoint} />
600+
<NotificationsDropdown
601+
endpoint={data.endpoint}
602+
defaultUnreadCount={data.default_unread_count}
603+
/>
601604
</Suspense>
602605
),
603606
'dropdowns-reputation': (data: any) => (

test/system/flows/notifications/dropdown_test.rb

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -58,6 +58,12 @@ class DropdownTest < ApplicationSystemTestCase
5858
visit dashboard_path
5959
find(".c-notification").click
6060

61+
# The list is now fetched lazily on first open, so wait for that
62+
# initial (empty) fetch to resolve before creating a notification -
63+
# otherwise it can race the in-flight request and appear before
64+
# the dropdown is closed and reopened.
65+
assert_link "See all your notifications"
66+
6167
create :mentor_started_discussion_notification, user:, params: { discussion: }, status: :unread
6268
NotificationsChannel.broadcast_changed!(user)
6369
wait_for_websockets

0 commit comments

Comments
 (0)