Skip to content

Commit 666ec05

Browse files
committed
fix: address code review feedback for fallbackToALaCarte feature
- Fix syntax error (stray "the" token) in preferences API route - Fix CLI cache mismatch: use setActivityQueryData instead of TanStack Query - Update chat.tsx to use server-side fallbackToALaCarte instead of local setting - Add disabled state to toggle buttons while mutation is pending - Add query invalidation to web mutation onSettled - Improve CLI mutation error handling to show actual server error - Remove deprecated getAlwaysUseALaCarte/setAlwaysUseALaCarte functions
1 parent ce513ea commit 666ec05

File tree

7 files changed

+56
-64
lines changed

7 files changed

+56
-64
lines changed

cli/src/chat.tsx

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -58,7 +58,7 @@ import { getClaudeOAuthStatus } from './utils/claude-oauth'
5858
import { showClipboardMessage } from './utils/clipboard'
5959
import { readClipboardImage } from './utils/clipboard-image'
6060
import { getInputModeConfig } from './utils/input-modes'
61-
import { getAlwaysUseALaCarte } from './utils/settings'
61+
6262
import {
6363
type ChatKeyboardState,
6464
createDefaultChatKeyboardState,
@@ -1288,12 +1288,13 @@ export const Chat = ({
12881288
// Auto-show subscription limit banner when rate limit becomes active
12891289
const subscriptionLimitShownRef = useRef(false)
12901290
const subscriptionRateLimit = subscriptionData?.hasSubscription ? subscriptionData.rateLimit : undefined
1291+
const fallbackToALaCarte = subscriptionData?.fallbackToALaCarte ?? false
12911292
useEffect(() => {
12921293
const isLimited = subscriptionRateLimit?.limited === true
12931294
if (isLimited && !subscriptionLimitShownRef.current) {
12941295
subscriptionLimitShownRef.current = true
12951296
// Skip showing the banner if user prefers to always fall back to a-la-carte
1296-
if (!getAlwaysUseALaCarte()) {
1297+
if (!fallbackToALaCarte) {
12971298
useChatStore.getState().setInputMode('subscriptionLimit')
12981299
}
12991300
} else if (!isLimited) {
@@ -1302,7 +1303,7 @@ export const Chat = ({
13021303
useChatStore.getState().setInputMode('default')
13031304
}
13041305
}
1305-
}, [subscriptionRateLimit?.limited])
1306+
}, [subscriptionRateLimit?.limited, fallbackToALaCarte])
13061307

13071308
const inputBoxTitle = useMemo(() => {
13081309
const segments: string[] = []

cli/src/components/subscription-limit-banner.tsx

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -132,9 +132,9 @@ export const SubscriptionLimitBanner = () => {
132132
</box>
133133

134134
{hasAlaCarteCredits && (
135-
<Button onClick={handleToggleFallbackToALaCarte}>
135+
<Button onClick={handleToggleFallbackToALaCarte} disabled={updatePreference.isPending}>
136136
<text style={{ fg: theme.muted }}>
137-
{fallbackToALaCarte ? '[x]' : '[ ]'} always use credits if subscription limit is reached
137+
{updatePreference.isPending ? '[...]' : fallbackToALaCarte ? '[x]' : '[ ]'} always use credits if subscription limit is reached
138138
</text>
139139
</Button>
140140
)}

cli/src/components/usage-banner.tsx

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -122,7 +122,7 @@ export const UsageBanner = ({ showTime }: { showTime: number }) => {
122122
subscriptionInfo={subscriptionInfo}
123123
rateLimit={rateLimit}
124124
isLoading={isSubscriptionLoading}
125-
fallbackToALaCarte={activeSubscription.fallbackToALaCarte}
125+
fallbackToALaCarte={activeSubscription.fallbackToALaCarte ?? false}
126126
/>
127127
)}
128128

@@ -271,9 +271,9 @@ const SubscriptionUsageSection: React.FC<SubscriptionUsageSectionProps> = ({
271271
<text style={{ fg: theme.muted }}>
272272
{fallbackToALaCarte ? 'spend credits' : 'pause'}
273273
</text>
274-
<Button onClick={handleToggleFallbackToALaCarte}>
274+
<Button onClick={handleToggleFallbackToALaCarte} disabled={updatePreference.isPending}>
275275
<text style={{ fg: theme.muted, attributes: TextAttributes.UNDERLINE }}>
276-
[{fallbackToALaCarte ? 'switch to pause' : 'switch to spend credits'}]
276+
{updatePreference.isPending ? '[updating...]' : `[${fallbackToALaCarte ? 'switch to pause' : 'switch to spend credits'}]`}
277277
</text>
278278
</Button>
279279
</box>
Lines changed: 40 additions & 37 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,10 @@
1-
import { useMutation, useQueryClient } from '@tanstack/react-query'
1+
import { useCallback, useState } from 'react'
22

3+
import {
4+
getActivityQueryData,
5+
invalidateActivityQuery,
6+
setActivityQueryData,
7+
} from './use-activity-query'
38
import { subscriptionQueryKeys } from './use-subscription-query'
49
import { getApiClient } from '../utils/codebuff-api'
510
import { logger } from '../utils/logger'
@@ -11,51 +16,49 @@ interface UpdatePreferenceParams {
1116
}
1217

1318
export function useUpdatePreference() {
14-
const queryClient = useQueryClient()
19+
const [isPending, setIsPending] = useState(false)
1520

16-
return useMutation({
17-
mutationFn: async (params: UpdatePreferenceParams) => {
18-
const client = getApiClient()
19-
const response = await client.patch('/api/user/preferences', {
20-
body: params,
21-
includeCookie: true,
22-
})
21+
const mutate = useCallback(async (params: UpdatePreferenceParams) => {
22+
const queryKey = subscriptionQueryKeys.current()
2323

24-
if (!response.ok) {
25-
throw new Error('Failed to update preference')
26-
}
24+
// Snapshot the previous value for rollback
25+
const previousData = getActivityQueryData<SubscriptionResponse>(queryKey)
2726

28-
return params
29-
},
30-
onMutate: async (newParams) => {
31-
// Cancel any outgoing refetches
32-
await queryClient.cancelQueries({ queryKey: subscriptionQueryKeys.current() })
27+
// Optimistically update to the new value
28+
if (previousData && params.fallbackToALaCarte !== undefined) {
29+
setActivityQueryData<SubscriptionResponse>(queryKey, {
30+
...previousData,
31+
fallbackToALaCarte: params.fallbackToALaCarte,
32+
})
33+
}
3334

34-
// Snapshot the previous value
35-
const previousData = queryClient.getQueryData<SubscriptionResponse>(
36-
subscriptionQueryKeys.current()
35+
setIsPending(true)
36+
37+
try {
38+
const client = getApiClient()
39+
const response = await client.patch<{ success: boolean; error?: string }>(
40+
'/api/user/preferences',
41+
params as Record<string, unknown>,
42+
{ includeCookie: true },
3743
)
3844

39-
// Optimistically update to the new value
40-
if (previousData && newParams.fallbackToALaCarte !== undefined) {
41-
queryClient.setQueryData<SubscriptionResponse>(
42-
subscriptionQueryKeys.current(),
43-
{ ...previousData, fallbackToALaCarte: newParams.fallbackToALaCarte }
44-
)
45+
if (!response.ok) {
46+
const errorMessage = response.error || 'Failed to update preference'
47+
throw new Error(errorMessage)
4548
}
4649

47-
return { previousData }
48-
},
49-
onError: (err, _newParams, context) => {
50+
// Invalidate to refetch fresh data from server
51+
invalidateActivityQuery(queryKey)
52+
} catch (err) {
5053
// Rollback to previous value on error
51-
if (context?.previousData) {
52-
queryClient.setQueryData(subscriptionQueryKeys.current(), context.previousData)
54+
if (previousData) {
55+
setActivityQueryData(queryKey, previousData)
5356
}
5457
logger.error({ err }, 'Failed to update preference')
55-
},
56-
onSettled: () => {
57-
// Refetch after mutation
58-
queryClient.invalidateQueries({ queryKey: subscriptionQueryKeys.current() })
59-
},
60-
})
58+
} finally {
59+
setIsPending(false)
60+
}
61+
}, [])
62+
63+
return { mutate, isPending }
6164
}

cli/src/utils/settings.ts

Lines changed: 0 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -149,19 +149,3 @@ export const saveModePreference = (mode: AgentMode): void => {
149149
saveSettings({ mode })
150150
}
151151

152-
/**
153-
* Load the "always use a-la-carte" preference
154-
* @deprecated Use server-side fallbackToALaCarte setting via useSubscriptionQuery instead
155-
*/
156-
export const getAlwaysUseALaCarte = (): boolean => {
157-
const settings = loadSettings()
158-
return settings.fallbackToALaCarte ?? settings.alwaysUseALaCarte ?? false
159-
}
160-
161-
/**
162-
* Save the "always use a-la-carte" preference
163-
* @deprecated Use server-side fallbackToALaCarte setting via useUpdatePreference instead
164-
*/
165-
export const setAlwaysUseALaCarte = (value: boolean): void => {
166-
saveSettings({ fallbackToALaCarte: value })
167-
}

web/src/app/api/user/preferences/route.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -55,7 +55,7 @@ export async function PATCH(request: Request) {
5555
.update(schema.user)
5656
.set(updates)
5757
.where(eq(schema.user.id, userId))
58-
the
58+
5959
logger.info({ userId, updates }, 'User preferences updated')
6060

6161
return NextResponse.json({ success: true, ...parsed.data })

web/src/app/profile/components/subscription-section.tsx

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -76,13 +76,17 @@ function SubscriptionActive({ data, email }: { data: ActiveSubscriptionResponse;
7676
old ? { ...old, fallbackToALaCarte: newValue } : old
7777
)
7878
},
79-
onError: (error: Error) => {
79+
onError: (err: Error) => {
8080
toast({
8181
title: 'Error',
82-
description: error.message,
82+
description: err.message,
8383
variant: 'destructive',
8484
})
8585
},
86+
onSettled: () => {
87+
// Refetch to ensure consistency with server
88+
queryClient.invalidateQueries({ queryKey: ['subscription'] })
89+
},
8690
})
8791

8892
const blockRemainingPercent =

0 commit comments

Comments
 (0)