[MAIN-IMP] Updating useJobsHook and integrating with the jobsList#97
[MAIN-IMP] Updating useJobsHook and integrating with the jobsList#97
Conversation
Updating useJobs to handle advanced filtering Updating Jobs page to use the useJobs hook for jobs fetching
📝 WalkthroughWalkthroughRefactors job fetching to React Query via an updated Changes
Sequence Diagram(s)sequenceDiagram
participant JobsPage
participant useJobsHook as useJobs
participant ReactQueryCache as Cache
participant JobsService as API
participant ReduxStore as Store
JobsPage->>useJobsHook: mount / call useJobs(params)
useJobsHook->>Cache: check queryKey (limit,offset,sorting,status,advancedFilters,targetServer)
alt cache miss or stale
useJobsHook->>JobsService: fetchJobs(params)
JobsService-->>useJobsHook: jobs[]
useJobsHook->>Cache: store result
end
useJobsHook-->>JobsPage: return { jobs, isJobFetching, forceJobRefresh }
JobsPage->>Store: dispatch setJobsList(jobs)
Note over JobsPage,useJobsHook: On actions (row action, import, confirm)
JobsPage->>useJobsHook: call forceJobRefresh()
useJobsHook->>Cache: invalidate queryKey
Cache->>useJobsHook: refetch
useJobsHook->>JobsService: fetchJobs(updated params)
JobsService-->>useJobsHook: jobs[]
useJobsHook-->>JobsPage: updated jobs
JobsPage->>Store: dispatch setJobsList(updated jobs)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/features/jobsTable/jobsPage.tsx (1)
361-370:⚠️ Potential issue | 🟠 MajorUse
isPendingor separate background-refetch indicator instead ofisFetchingfor the Spinner overlay.
isJobFetchingmaps directly to React Query'sisFetching, which istrueduring every fetch—including background refetches triggered byforceJobRefresh(), window refocus, and reconnect. The LoadingOverlay renders a full backdrop covering the table each timeisFetchingchanges, creating a flashing overlay UX that hides existing data unnecessarily. For the initial loading state, useisPending. For background refetches, either:
- Reserve
isFetchingfor a subtle indicator (e.g., a header badge) instead of a full overlay, or- Use
isRefetching(true only during background refetches when data exists) to avoid covering the table during initial load.Modify
useJobsto exportisPendingfrom the query, or manage these states directly in the component.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/features/jobsTable/jobsPage.tsx` around lines 361 - 370, The Spinner currently uses isJobFetching (which maps to React Query's isFetching) and causes a full overlay on background refetches; update the component to use the query's isPending for the full LoadingOverlay and use isRefetching (or a separate subtle flag) for background refresh UI instead of isFetching: change the hook useJobs to re-export isPending and isRefetching alongside existing values, then replace usage of isJobFetching in the Spinner with the new isPending variable and handle isRefetching elsewhere (e.g., header badge) so forceJobRefresh/window refocus won't trigger the full overlay.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/components/custom/notifications/NotificationServicesPanel.tsx`:
- Line 363: Remove the redundant text-ellipsis utility from the className in the
NotificationServicesPanel component: locate the div using className "text-sm
font-bold text-ellipsis line-clamp-2" inside NotificationServicesPanel and drop
"text-ellipsis" so the element only uses "line-clamp-2" (and other classes) —
line-clamp-2 already provides the overflow and ellipsis behavior.
In `@src/features/jobsTable/jobsPage.tsx`:
- Around line 59-64: The jobsPage useJobs call currently hard-codes limit: 99999
and offset: 0 which bypasses server-side pagination; change the call in
jobsPage.tsx to omit limit and offset (or pass undefined) so you don’t
force-fetch everything (e.g., call useJobs({ sorting, advancedFilters }) ), add
a clear TODO comment explaining this is a temporary client-side full-fetch for
the partial migration and must be revisited when pagination is reintroduced, and
verify/update useJobs to avoid silently dropping limit/offset when
advancedFilters is present so parameters are passed through consistently.
- Around line 113-120: Update the useMemo/useCallback dependency arrays so they
include only the actual stable functions used and include any non-stable
callbacks referenced: for tableEventsMemoized include
[filterAndPaginationChange, forceJobRefresh] (remove selectedRowIds), for
takeJobsAction change deps to [forceJobRefresh], for confirmBatchJobAction
change deps to [takeBatchJobsAction], and for importJobs include forceJobRefresh
in its deps (no longer []). Re-audit other hooks in this file (notably the
blocks around takeJobsAction, confirmBatchJobAction, tableEventsMemoized, and
importJobs) to ensure each dependency array exactly matches the referenced
symbols (e.g., filterAndPaginationChange, forceJobRefresh, takeBatchJobsAction)
to avoid stale closures.
- Line 224: Remove the no-op promise handler: drop the empty .finally(() => {})
from the promise chain in jobsPage.tsx (where the async fetch/update sequence
uses .finally(() => {})), leaving the chain without that redundant call so
behavior is unchanged and code is cleaner.
- Around line 66-68: The effect currently dispatches setJobsList on every
refetch because useQuery returns new array references and also omits dispatch
from deps; change the effect to only run when jobs is defined and include
dispatch in the dependency array (e.g., useEffect(() => { if (jobs !==
undefined) dispatch(setJobsList(jobs)); }, [dispatch, jobs])), and in the query
call remove placeholderData: [] or switch to keepPreviousData to avoid the
initial empty array overwriting store state; update references to the useEffect,
dispatch, setJobsList, jobs, and the query hook accordingly.
In `@src/hooks/useJobs.tsx`:
- Line 33: The local state variable const [loading, setLoading] =
useState<boolean>(false) is misleading because it only tracks the event-fetching
path (getEventsPerJobs/getJobEvents) while the main jobs fetch uses React
Query's isJobFetching; rename loading to isEventsLoading (and setLoading to
setIsEventsLoading) or remove it if not used, then update all references inside
useJobs (including getEventsPerJobs, any callers that check loading, and the
returned object at the bottom around lines ~140-148) so the hook returns a clear
isEventsLoading flag instead of loading and consumers use isJobFetching for the
primary fetch.
- Around line 44-52: The fetchJobs callback drops limit/offset when
advancedFilters is present causing inconsistent paging; update the branch that
calls jobsService.filterJobs from fetchJobs to include limit and offset (and
sorting) in the payload (e.g., merge limit and offset into the advancedFilters
body) so filtered requests respect paging, or alternatively change
jobsService.filterJobs to accept limit/offset as arguments and forward them to
the API; ensure you update references to fetchJobs and the filterJobs call
accordingly to preserve symmetric behavior with getAllJobs.
- Around line 44-52: The useCallback-wrapped fetchJobs is misleading because it
doesn't use any closure variables (only its parameters) and its dependency array
([limit, offset, status, sorting, advancedFilters]) is incorrect; also memoizing
it doesn't stabilize the queryFn passed to React Query. Fix by removing
useCallback and either inline the existing branch directly into the queryFn used
for the query or convert fetchJobs to a plain top-level function that calls
jobsService.filterJobs(...) or jobsService.getAllJobs(...); update any call
sites (the queryFn) to call the new plain function or use the inlined logic so
no stale dependency array or unnecessary memoization remains (referencing
fetchJobs, jobsService.filterJobs, jobsService.getAllJobs, and queryFn).
- Around line 134-138: The useCallback for forceJobRefresh currently omits
queryClient from its dependency array; update the useCallback that defines
forceJobRefresh (the function that calls queryClient.invalidateQueries with
queryKey ["allJobs"]) to include queryClient in its dependencies so React hooks
lint rules are satisfied and the callback stays correct when queryClient changes
(i.e., change the empty deps array to [queryClient]).
---
Outside diff comments:
In `@src/features/jobsTable/jobsPage.tsx`:
- Around line 361-370: The Spinner currently uses isJobFetching (which maps to
React Query's isFetching) and causes a full overlay on background refetches;
update the component to use the query's isPending for the full LoadingOverlay
and use isRefetching (or a separate subtle flag) for background refresh UI
instead of isFetching: change the hook useJobs to re-export isPending and
isRefetching alongside existing values, then replace usage of isJobFetching in
the Spinner with the new isPending variable and handle isRefetching elsewhere
(e.g., header badge) so forceJobRefresh/window refocus won't trigger the full
overlay.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 38e5414d-6db9-4455-84c5-7590528eac32
📒 Files selected for processing (5)
src/components/custom/charts/AllEventStats.tsxsrc/components/custom/notifications/NotificationServicesPanel.tsxsrc/features/dashboard/dashboard.tsxsrc/features/jobsTable/jobsPage.tsxsrc/hooks/useJobs.tsx
💤 Files with no reviewable changes (1)
- src/features/dashboard/dashboard.tsx
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/features/jobsTable/jobsPage.tsx`:
- Around line 59-68: The code duplicates job state between React Query (useJobs
-> jobs) and Redux (setJobsList / JobsList) causing transient mismatch; fix by
consolidating: remove the useEffect that dispatches setJobsList and have
DataTable read directly from the jobs returned by useJobs (use jobs ?? [] as the
DataTable data prop), and delete or stop updating setJobsList for this read path
(leave Redux-only handling for mutation flows if needed) so useJobs is the
single source of truth; alternatively, if you prefer Redux as the single source,
keep the dispatch but remove reliance on useJobs.data and stop using
placeholderData in useJobs—pick one approach and remove the redundant syncing
logic around useJobs, setJobsList, and JobsList.
- Line 363: The Spinner is using isJobFetching which is true for background
refetches; change the Spinner's loading condition to only show when there are no
jobs (e.g., use isJobFetching && !jobs?.length) so background fetches stay
silent; update the Spinner invocation that currently passes isJobFetching to
instead pass a combined condition using the jobs array (refer to Spinner,
isJobFetching, and jobs in jobsPage.tsx).
In `@src/hooks/useJobs.tsx`:
- Around line 54-69: The useQuery call currently destructures only data and
isFetching, so consumers can't tell if an empty jobs array is from a failed
fetch; update the destructuring to also capture the query error (e.g., error:
jobsError) from useQuery and include that jobsError in the hook's returned
object so callers like JobsPage can render error UI; reference the existing
useQuery invocation and the jobs/isFetching identifiers and ensure the hook
return shape exposes jobsError.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 842f8f8d-4ab8-406e-97d6-b7ae8d8e915d
📒 Files selected for processing (3)
src/components/custom/notifications/NotificationServicesPanel.tsxsrc/features/jobsTable/jobsPage.tsxsrc/hooks/useJobs.tsx
| const { jobs, isJobFetching, forceJobRefresh } = useJobs({ | ||
| sorting, | ||
| advancedFilters: advancedFilters, | ||
| offset: 0, | ||
| limit: 99999, | ||
| }) | ||
|
|
||
| const updateTableData = useCallback( | ||
| (sorting?: any, avFilters?: any) => { | ||
| return Promise.all([getRunningJobs(), fetchTableData(sorting, avFilters)]) | ||
| }, | ||
| [sorting], | ||
| ) | ||
| useEffect(() => { | ||
| dispatch(setJobsList(jobs ?? [])) | ||
| }, [jobs]) |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Dual source of truth for jobs (React Query cache + Redux) — consider consolidating.
useJobs already caches jobs in the React Query store, but you then mirror the result into Redux (setJobsList) and <DataTable data={JobsList} /> reads from Redux rather than from jobs returned by the hook. This creates two stores for the same data and introduces an intermediate render where jobs and JobsList disagree (especially relevant given placeholderData: [] dispatches an empty list on first render).
Since the PR description notes this is a partial migration, a follow-up to either:
- render directly from
jobsand dropsetJobsListfrom this flow, or - keep Redux as the single source and drop
useQuery'sdatafrom this path
would eliminate the redundant sync and the transient empty-list state. Noting this for the future-mutations work referenced in the PR description.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/features/jobsTable/jobsPage.tsx` around lines 59 - 68, The code
duplicates job state between React Query (useJobs -> jobs) and Redux
(setJobsList / JobsList) causing transient mismatch; fix by consolidating:
remove the useEffect that dispatches setJobsList and have DataTable read
directly from the jobs returned by useJobs (use jobs ?? [] as the DataTable data
prop), and delete or stop updating setJobsList for this read path (leave
Redux-only handling for mutation flows if needed) so useJobs is the single
source of truth; alternatively, if you prefer Redux as the single source, keep
the dispatch but remove reliance on useJobs.data and stop using placeholderData
in useJobs—pick one approach and remove the redundant syncing logic around
useJobs, setJobsList, and JobsList.
| </div> | ||
| </div> | ||
| <Spinner isLoading={loading} icon={LoaderPinwheelIcon}> | ||
| <Spinner isLoading={isJobFetching} icon={LoaderPinwheelIcon}> |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
@tanstack/react-query v5 useQuery isFetching vs isPending vs isLoading
💡 Result:
In @tanstack/react-query v5 useQuery, the flags isFetching, isPending, and isLoading indicate different states of the query: - isPending: true when the query status is 'pending' (no data in cache yet, first load or enabled=false). Derived directly from status === 'pending'. Use for initial loading when no data exists. Official docs recommend checking this first for safe data access and TypeScript narrowing. - isFetching: true whenever the queryFn is executing (fetchStatus === 'fetching'), including initial fetches and background refetches. Use for showing refresh indicators without blocking stale data display. - isLoading: true only during the first fetch when no cached data exists, equivalent to isPending && isFetching. Less commonly needed since isPending and isFetching cover most cases directly. Key distinctions: - isPending focuses on data availability (status). - isFetching focuses on network activity (fetchStatus). - All combinations possible due to background refetches, pausing (e.g., offline), etc. Example usage: const { data, isPending, isFetching, isError, error } = useQuery({ queryKey: ['todos'], queryFn: fetchTodos, }); if (isPending) return
Citations:
- 1: https://tanstack.com/query/v5/docs/framework/react/guides/queries
- 2: https://tanstack.dev/query/v5/docs/framework/react/reference/useQuery
- 3: https://tanstack.com/query/v5/docs/framework/react/guides/migrating-to-v5
- 4: https://tanstack.com/query/v5/docs/framework/react/guides/background-fetching-indicators
- 5: Why do the docs recommend `isPending` instead of `isLoading`? TanStack/query#6297
🏁 Script executed:
# Find the useJobs hook implementation
fd -t f "useJobs" --type fRepository: moda20/TypeSchedulerUI
Length of output: 88
🏁 Script executed:
# Search for useJobs in the codebase
rg "useJobs" -t tsx -t ts -B 2 -A 5 | head -100Repository: moda20/TypeSchedulerUI
Length of output: 94
🏁 Script executed:
# Find where the hook is defined
rg "export.*useJobs|function useJobs|const useJobs" -t tsx -t ts -A 10Repository: moda20/TypeSchedulerUI
Length of output: 94
🏁 Script executed:
# Read the useJobs hook
cat -n src/hooks/useJobs.tsxRepository: moda20/TypeSchedulerUI
Length of output: 4820
🏁 Script executed:
# Also check the jobsPage.tsx file to understand the Spinner usage context
grep -n "useJobs\|isFetching\|Spinner\|isJobFetching" src/features/jobsTable/jobsPage.tsx | head -30Repository: moda20/TypeSchedulerUI
Length of output: 327
isFetching triggers the spinner on background refetches too.
isFetching is true for the initial load and every background refetch — reconnect (the hook sets refetchOnReconnect: true), window focus (React Query's default), and every forceJobRefresh() triggered by row actions. That's a UX change from the previous manual loading flag, which only flipped around explicit fetches. Users will see the full-table spinner overlay flash after each job action and on tab refocus.
If that isn't desired, use isFetching && !jobs?.length so background refreshes are silent:
💡 Suggested fix
- <Spinner isLoading={isJobFetching} icon={LoaderPinwheelIcon}>
+ <Spinner isLoading={isJobFetching && !jobs?.length} icon={LoaderPinwheelIcon}>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <Spinner isLoading={isJobFetching} icon={LoaderPinwheelIcon}> | |
| <Spinner isLoading={isJobFetching && !jobs?.length} icon={LoaderPinwheelIcon}> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/features/jobsTable/jobsPage.tsx` at line 363, The Spinner is using
isJobFetching which is true for background refetches; change the Spinner's
loading condition to only show when there are no jobs (e.g., use isJobFetching
&& !jobs?.length) so background fetches stay silent; update the Spinner
invocation that currently passes isJobFetching to instead pass a combined
condition using the jobs array (refer to Spinner, isJobFetching, and jobs in
jobsPage.tsx).
| const { data: jobs, isFetching } = useQuery({ | ||
| queryKey: [ | ||
| "allJobs", | ||
| limit, | ||
| offset, | ||
| status, | ||
| sorting, | ||
| advancedFilters, | ||
| savedConfig.targetServer, | ||
| ], | ||
| queryFn: () => | ||
| fetchJobs({ sorting, status, limit, offset, advancedFilters }), | ||
| placeholderData: [], | ||
| refetchOnReconnect: true, | ||
| retry: false, | ||
| }) |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
🧩 Analysis chain
🌐 Web query:
@tanstack/react-query v5 useQuery return fields error isError
💡 Result:
In @tanstack/react-query v5, the useQuery hook returns an object with fields including error and isError. - error: null | TError - The error object for the query if an error was thrown (defaults to null). Type can be customized via generics or global registration. - isError: boolean - True if the query status is 'error', derived from the status for convenience. These are used to check for query failures: const { data, error, isError } = useQuery({ queryKey, queryFn }); if (isError) { return <span>Error: {error.message}</span>; } Note: Errors only trigger if the queryFn throws a rejected promise. Native fetch does not throw on HTTP errors (e.g., 404); use libraries like axios or manual checks.
Citations:
- 1: https://tanstack.dev/query/v5/docs/framework/react/reference/useQuery
- 2: https://tanstack.com/query/v5/docs/framework/react/guides/queries
🏁 Script executed:
# Find the useJobs hook definition and its return statement
fd 'useJobs' --type f
# Find where useJobs is used (likely in JobsPage)
rg 'useJobs' -A 3 -B 1 --type tsxRepository: moda20/TypeSchedulerUI
Length of output: 134
🏁 Script executed:
# Check the full useJobs hook to see what it currently returns
fd 'useJobs.tsx' --type f -x cat -n {}Repository: moda20/TypeSchedulerUI
Length of output: 4820
🏁 Script executed:
rg 'useJobs' --type ts -A 5 -B 2Repository: moda20/TypeSchedulerUI
Length of output: 5945
🏁 Script executed:
# Find JobsPage component
fd -i 'jobspage' --type fRepository: moda20/TypeSchedulerUI
Length of output: 102
🏁 Script executed:
# Check JobsPage to see how it uses jobs and if there's any error handling
cat -n src/features/jobsTable/jobsPage.tsx | head -100Repository: moda20/TypeSchedulerUI
Length of output: 4245
🏁 Script executed:
# Also check if there's any error toast or error UI in JobsPage
rg 'error|Error|toast' src/features/jobsTable/jobsPage.tsx -iRepository: moda20/TypeSchedulerUI
Length of output: 219
Expose error from the query to enable error handling in consumers.
With retry: false and placeholderData: [], a failed fetch leaves consumers with an empty array and no way to distinguish "no jobs" from "fetch failed". Surfacing error from useQuery allows JobsPage (and other consumers) to render error feedback instead of silently showing an empty table.
Suggested change
- const { data: jobs, isFetching } = useQuery({
+ const { data: jobs, isFetching, error: jobsError } = useQuery({
queryKey: [
"allJobs",
limit,
offset,
status,
sorting,
advancedFilters,
savedConfig.targetServer,
],
queryFn: () =>
fetchJobs({ sorting, status, limit, offset, advancedFilters }),
placeholderData: [],
refetchOnReconnect: true,
retry: false,
})Include jobsError in the returned object so consumers can handle fetch failures.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/hooks/useJobs.tsx` around lines 54 - 69, The useQuery call currently
destructures only data and isFetching, so consumers can't tell if an empty jobs
array is from a failed fetch; update the destructuring to also capture the query
error (e.g., error: jobsError) from useQuery and include that jobsError in the
hook's returned object so callers like JobsPage can render error UI; reference
the existing useQuery invocation and the jobs/isFetching identifiers and ensure
the hook return shape exposes jobsError.
1 participant
Improvements :
useJobshook to fetch jobs using advanced filters, via the POST request. Added a jobs refresh function to invalidate thequerycache.JobsPageto useuseJobshook, and refresh on each destructive actionNotes :
useJobshook and the jobs page, w'll see if we need to use mutations for the job actions and if we can integrate them into the custom hookSummary by CodeRabbit
Style
Refactor