-
Notifications
You must be signed in to change notification settings - Fork 4
refactor: jwt to use react-query; remove session storage #1383
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Refactors JWT token handling to use a React Query based provider instead of manual caching and updates imports accordingly.
- Replaces the previous JwtTokenContextProvider with a React Query powered implementation.
- Switches JWT decoding from jwtVerify (with skipped verification) to decodeJwt.
- Updates imports in dependent files to point to the new provider.
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| src/services/api.ts | Updates import to new React Query-based JWT provider. |
| src/context/JwtTokenContextProvider.tsx | Simplifies token expiration extraction using decodeJwt. |
| src/context/JwtTokenContextProvider.reactquery.tsx | Introduces new React Query-driven JWT token provider. |
| src/context/AmpersandContextProvider/AmpersandContextProvider.tsx | Switches provider import to the new React Query implementation. |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| // Let React Query handle caching and expiration | ||
| const data = await queryClient.fetchQuery({ | ||
| queryKey, | ||
| queryFn: async (): Promise<TokenData> => { | ||
| const token = await getTokenCallback({ consumerRef, groupRef }); | ||
| const expirationTime = getTokenExpirationTime(token); | ||
|
|
||
| const expiresAt = | ||
| expirationTime || Date.now() + DEFAULT_TOKEN_EXPIRATION_TIME; | ||
|
|
||
| return { token, expiresAt }; | ||
| }, | ||
| // React Query will use existing cache if available and not stale | ||
| staleTime: (query) => { | ||
| const data = query.state.data as TokenData | undefined; | ||
| if (!data) return 0; | ||
|
|
||
| const timeUntilExpiry = data.expiresAt - Date.now(); | ||
|
|
||
| // Token is stale when it expires (with 30 seconds buffer) | ||
| return Math.max(0, timeUntilExpiry - 30000); // 30 seconds buffer | ||
| }, | ||
| gcTime: 10 * 60 * 1000, // Keep in cache for 10 minutes | ||
| retry: 2, | ||
| retryDelay: (attemptIndex) => Math.min(1000 * 2 ** attemptIndex, 30000), | ||
| }); | ||
|
|
||
| return data.token; |
Copilot
AI
Oct 17, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
staleTime expects a number (or Infinity) in TanStack Query; providing a function here is not a supported API and will not yield the intended dynamic behavior. To implement per-token dynamic staleness, compute the token first (with its expiration), then decide whether to reuse or refetch instead of relying on staleTime. For example, replace fetchQuery usage with a manual check: get existing = queryClient.getQueryData(queryKey); if existing && existing.expiresAt - Date.now() > 30000 return existing.token; else fetch new token, compute expiresAt, and setQueryData.
| // Let React Query handle caching and expiration | |
| const data = await queryClient.fetchQuery({ | |
| queryKey, | |
| queryFn: async (): Promise<TokenData> => { | |
| const token = await getTokenCallback({ consumerRef, groupRef }); | |
| const expirationTime = getTokenExpirationTime(token); | |
| const expiresAt = | |
| expirationTime || Date.now() + DEFAULT_TOKEN_EXPIRATION_TIME; | |
| return { token, expiresAt }; | |
| }, | |
| // React Query will use existing cache if available and not stale | |
| staleTime: (query) => { | |
| const data = query.state.data as TokenData | undefined; | |
| if (!data) return 0; | |
| const timeUntilExpiry = data.expiresAt - Date.now(); | |
| // Token is stale when it expires (with 30 seconds buffer) | |
| return Math.max(0, timeUntilExpiry - 30000); // 30 seconds buffer | |
| }, | |
| gcTime: 10 * 60 * 1000, // Keep in cache for 10 minutes | |
| retry: 2, | |
| retryDelay: (attemptIndex) => Math.min(1000 * 2 ** attemptIndex, 30000), | |
| }); | |
| return data.token; | |
| // Check if a valid token exists in cache | |
| const existing = queryClient.getQueryData<TokenData>(queryKey); | |
| if ( | |
| existing && | |
| typeof existing.expiresAt === "number" && | |
| existing.expiresAt - Date.now() > 30000 // 30 seconds buffer | |
| ) { | |
| return existing.token; | |
| } | |
| // Fetch new token and cache it | |
| const token = await getTokenCallback({ consumerRef, groupRef }); | |
| const expirationTime = getTokenExpirationTime(token); | |
| const expiresAt = | |
| expirationTime || Date.now() + DEFAULT_TOKEN_EXPIRATION_TIME; | |
| const data: TokenData = { token, expiresAt }; | |
| queryClient.setQueryData(queryKey, data); | |
| return token; |
| expiresAt: number; | ||
| } | ||
|
|
||
| const DEFAULT_TOKEN_EXPIRATION_TIME = 10 * 60 * 1000; // 10 minutes |
Copilot
AI
Oct 17, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] Repeated magic numbers (10 * 60 * 1000 and 30000) reduce clarity and make future adjustments harder. Recommend extracting constants, e.g., TOKEN_DEFAULT_TTL_MS, TOKEN_EXPIRY_BUFFER_MS, and reusing DEFAULT_TOKEN_EXPIRATION_TIME for gcTime where appropriate (or computing gcTime relative to expiresAt to avoid premature garbage collection).
| return Math.max(0, timeUntilExpiry - 30000); // 30 seconds buffer | ||
| }, | ||
| gcTime: 10 * 60 * 1000, // Keep in cache for 10 minutes |
Copilot
AI
Oct 17, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] Repeated magic numbers (10 * 60 * 1000 and 30000) reduce clarity and make future adjustments harder. Recommend extracting constants, e.g., TOKEN_DEFAULT_TTL_MS, TOKEN_EXPIRY_BUFFER_MS, and reusing DEFAULT_TOKEN_EXPIRATION_TIME for gcTime where appropriate (or computing gcTime relative to expiresAt to avoid premature garbage collection).
| * React Query handles all caching, expiration, and refetching automatically. | ||
| * No manual cache management needed! |
Copilot
AI
Oct 17, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The comment claims automatic handling of expiration/refetching, but the implementation still embeds custom expiration logic and (incorrect) dynamic stale handling. Update the comment to clearly describe the actual strategy: React Query caches tokens; custom logic determines expiry and triggers refresh, since dynamic per-item staleTime is manually enforced.
| * React Query handles all caching, expiration, and refetching automatically. | |
| * No manual cache management needed! | |
| * React Query caches tokens. Custom logic determines token expiry and triggers refresh, | |
| * since dynamic per-item staleTime is manually enforced. Manual cache management is required | |
| * for correct expiration and refresh behavior. |
Summary
react-query can handle the cache logic for us so we don't handle this manually.
Removes session storage for security and simplicity. It's not likely that an app will need to hard refresh the page; fetching token is not that expensive.
[] needs testing