fix: prevent double loading during PayrollConfiguration pagination #928
Conversation
There was a problem hiding this comment.
Pull request overview
This PR fixes a double-loading issue during pagination in PayrollConfiguration where users would see employee names update before their corresponding compensation data, causing a visual data mismatch. The fix ensures both datasets are synchronized before updating the displayed data.
Key changes:
- Switch from Suspense-based employee data fetching to regular fetching with
keepPreviousData - Add synchronization logic to keep displaying previous page data until both employee and compensation datasets match
- Combine loading states to show a single spinner until both fetches complete
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| return employeeUuids.every(uuid => preparedUuids.has(uuid)) | ||
| }, [preparedPayroll?.employeeCompensations, employeeUuids]) | ||
|
|
||
| const [syncedEmployeeData, setSyncedEmployeeData] = useState(employeeData) |
There was a problem hiding this comment.
Initial state is set to employeeData which may be undefined on first render since useEmployeesList no longer uses Suspense. This could cause displayedEmployees to be an empty array even when data exists. Initialize with undefined or null instead, and update the useEffect dependency to handle the initial data load.
| const [syncedEmployeeData, setSyncedEmployeeData] = useState(employeeData) | |
| const [syncedEmployeeData, setSyncedEmployeeData] = | |
| useState<typeof employeeData | undefined>(undefined) |
| } | ||
| }, [isDataInSync, employeeData]) | ||
|
|
||
| const displayedEmployees = syncedEmployeeData?.showEmployees || [] |
There was a problem hiding this comment.
This fallback to an empty array could mask the difference between 'no data yet' and 'legitimately empty page'. Consider using a more explicit null check pattern that distinguishes between loading states and empty results.
| const displayedEmployees = syncedEmployeeData?.showEmployees || [] | |
| const displayedEmployees = syncedEmployeeData?.showEmployees ?? [] |
jeffredodd
force-pushed
the
jdj/fix-payroll-config-double-load
branch
from
63afefe to
fa1f6d7
Compare
jeffredodd
requested review from
dmortal,
krisxcrash,
serikjensen and
shermango
| if (!employeeData) { | ||
| return <Loading /> | ||
| } |
There was a problem hiding this comment.
Shouldn't we base this on isFetching instead? could we have no value for employee data here but not actually be loading?
| useEffect(() => { | ||
| if (isDataInSync && employeeData) { | ||
| setSyncedEmployeeData(employeeData) | ||
| } | ||
| }, [isDataInSync, employeeData]) |
There was a problem hiding this comment.
Recommendation here to figure out if we can do this via callback for the data fetching rather than running it through an effect, given employeeData is a large object that is compared here in the dep array via referential equality
| const isDataInSync = useMemo(() => { | ||
| if (!preparedPayroll?.employeeCompensations || employeeUuids.length === 0) return false | ||
| const preparedUuids = new Set(preparedPayroll.employeeCompensations.map(c => c.employeeUuid)) | ||
| return employeeUuids.every(uuid => preparedUuids.has(uuid)) | ||
| }, [preparedPayroll?.employeeCompensations, employeeUuids]) |
There was a problem hiding this comment.
I have similar concerns here memoizing this where the employeeUuids array is being compared via referential equality here in the dep array since it's an array of strings. Same with employee compensations. Wondering if we manage to make the change below for the effect if we can just run this when we execute that rather than placing it in a memo
jeffredodd
force-pushed
the
jdj/fix-payroll-config-double-load
branch
from
2e23cca to
6b58d24
Compare
serikjensen
left a comment
serikjensen
left a comment
There was a problem hiding this comment.
Let's chat about this when you're back in, we've extracted employeeData into a ref, but i'm unsure if that actually resolves the concerns i raised.
jeffredodd
force-pushed
the
jdj/fix-payroll-config-double-load
branch
3 times, most recently
from
f2286c3 to
d5c109b
Compare
| beforeEach(() => { | ||
| server.use(getPaySchedules, getPaySchedulePreview, createPaySchedule, updatePaySchedule) | ||
| }) | ||
|
|
There was a problem hiding this comment.
Any reason we needed to add this here?
There was a problem hiding this comment.
The beforeEach is required because the "pay schedule preview functionality" tests depend on the getPaySchedulePreview endpoint returning data to render the calendar preview component (role="application").
While setupApiTestMocks() does include PayScheduleHandlers, other tests in the file override specific handlers (e.g., line 34-40 overrides getPaySchedules to return an empty array). MSW's handler precedence means the most recently added handler takes priority, and this can affect subsequent tests if handlers aren't explicitly reset.
- Switch from useEmployeesListSuspense to useEmployeesList with keepPreviousData - Add isDataInSync to detect when prepared payroll matches current employees - Add syncedEmployeeData state that only updates when both datasets sync - Use displayedEmployees for rendering to prevent mismatched data flash - Combined loading state shows spinner until both fetches complete - Add null check for initial load
- Initialize syncedEmployeeData with undefined instead of potentially undefined employeeData - Use nullish coalescing (??) instead of || for displayedEmployees fallback
… state - Add isPaginating flag to distinguish initial load from pagination - Use isPaginating for pagination fetching indicator instead of full isLoading - Add staleTime: Infinity to paySchedule query (doesn't change during pagination) - Memoize employeeUuidsKey to prevent unnecessary effect re-runs - Result: old data stays visible during page changes instead of loading spinner
…onDataReady Callback is memoized by the caller, so the defensive ref pattern isn't needed.
…s-payroll_id-prepare.json
…DeductionsForm tests
jeffredodd
force-pushed
the
jdj/fix-payroll-config-double-load
branch
from
7182849 to
22443c2
Compare
Problem
When paginating in PayrollConfiguration, users see two loading states instead of one:
During the transition, the table renders with mismatched data - new employee names with old compensation data - before the second fetch completes.
Solution
Keep showing previous page data until BOTH datasets are synchronized.
Changes
useEmployeesListSuspensetouseEmployeesListwithkeepPreviousDataisDataInSyncto detect when prepared payroll matches current employeessyncedEmployeeDatastate that only updates when both datasets syncdisplayedEmployeesfor rendering to prevent mismatched data flashResult
isDataInSync = falseisDataInSync = trueProof of Functionality
Before
Screen.Recording.2026-01-08.at.9.44.58.AM.mov
After
Screen.Recording.2026-01-08.at.8.48.46.AM.mov