[MAIN-IMP] Updating useJobsHook and integrating with the jobsList#97
Merged
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
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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