-
Notifications
You must be signed in to change notification settings - Fork 166
Fetch schedule actions in bulk[MAPS-127] #10378
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
Conversation
FBanfi
left a comment
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.
Looks great to me!! 💯 I added two non-blocking comments
| 'environment.sys.id': sdk.ids.environment, | ||
| 'sys.status[in]': 'scheduled', | ||
| 'order': 'scheduledFor.datetime', | ||
| 'limit': 500 |
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.
Even though there is a 500 limit at creation time for scheduled actions, we are only getting/fetching them here, so I would remove this limit in the query. This would allow us to decouple from the 500 hardcoded value in case in some future it changes!
Since you have already implemented the solution with TanStack, the batch handling would be done automatically if this value some day is higher! 😎
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.
Tested this and unfortunately if you don't specify a limit number, by default the CMA limits it automatically to 100 actions. So, I think we still need to clarify the limit.
| const { data, isFetching, error, refetch } = useQuery<FetchScheduledActionsResult, Error>({ | ||
| queryKey: ['scheduledActions', sdk.ids.space, sdk.ids.environment], |
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.
Just a heads up that if we use TanStack to fetch the scheduled actions as it is right now in the code, we are using the same configuration of the Query Client used for the fetch of the entries (so we are using the same stale time, gcTime, retries and retries delay).
I don't know if we want to have different parameters for the fetch of the scheduled actions. If we are ok using the same ones, just ignore this comment! 😁
// QueryClient
const queryClient = new QueryClient({
defaultOptions: {
queries: {
staleTime: CACHE_CONFIG.STALE_TIME_MS,
gcTime: CACHE_CONFIG.CACHE_TIME_MS, // gcTime replaces cacheTime in v5
refetchOnWindowFocus: CACHE_CONFIG.REFETCH_ON_WINDOW_FOCUS,
retry: 2, // Retry failed requests 2 times
retryDelay: 3000,
},
},
});
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.
Thanks for bringing this up so I can give some though into it! I think it make sense to keep it consistent across queries.
Purpose
Fetching Scheduled Actions
Approach
Testing steps
Created 200+ scheduled actions (entries and assets) with the generating script. Make sure the app can handle it
Grabacion.de.pantalla.2025-12-29.a.las.4.13.15.p.m.mov