fix(SDK-1291): poll payroll processing without depending on re-renders - #2715
fix(SDK-1291): poll payroll processing without depending on re-renders#2715mariechatfield wants to merge 8 commits into
Conversation
Completion was decided in a useEffect reading rendered query data, with the poll driven by the query's refetchInterval. That makes "did the payroll finish?" a function of whether the query observer notified the component. When it doesn't, the calculation succeeds server-side and the screen never advances -- then a 3-minute failsafe reports a false failure for a payroll that calculated fine. Add usePollingTask: the loop owns its own timer, reads the server directly each tick, and evaluates the terminal state inside its own closure. Callbacks are held in a ref so identity churn can't restart the loop; a run-id token makes double-start a single loop and guarantees no callbacks after stop or unmount; a rejected read is retried on the next tick rather than failing the task; and a terminal result on the deadline tick wins over the deadline. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Same shape as PayrollConfiguration's calculation poll, but worse: the component's main query is non-suspense and gates the entire screen on `!payrollData`, so a dropped notification freezes the very first read, not just a later one -- confirmed live by reproducing the partner's exact "Loading payroll..." screenshot with notifications suppressed. Start the usePollingTask loop unconditionally at mount, not just after Submit. Its first tick forces a render independent of the query's own notification, which is what actually clears the `!payrollData` gate. The same mount-start also picks up a submission already in flight (another tab, another admin) for free, since evaluate() keeps polling for as long as it reads `submitting` regardless of why the loop started. Removes the query's refetchInterval, the old completion effect, and the local isPolling state (now sourced from the hook). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Same shape as the payroll calculation and submission polls: completion was decided in a useEffect reading a query driven by refetchInterval, so a dropped observer notification would leave the screen polling forever behind the "Generating" state with no way out -- this surface had no deadline/failsafe at all, unlike the other two. Converts to usePollingTask and adds a 3-minute deadline consistent with the other two conversions, verifying against the server before reporting failure. The passive useGeneratedDocumentsGet hook is removed entirely -- nothing else in the component rendered off it, so once completion doesn't depend on a render there's nothing left for it to do. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
The deadline path added when converting to usePollingTask had no coverage -- every existing test's mock resolver returned a stable terminal status on the first tick, so the multi-tick/deadline logic never actually ran. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
There was a problem hiding this comment.
On 0.54.1 (or any other version off current main) and when installed in a top-level startTransition island so that no parent or sibling can force a re-render, and when there is something preventing render-time updates from react-query to the component... this is what gets stuck polling forever.
Using the hook (which queries the query client directly, instead of waiting for a render-time observer to notify react that some state has changed and a useEffect needs re-computing), it loads as expected:
| payrollId, | ||
| include: ['taxes', 'benefits', 'deductions', 'payroll_status_meta'], | ||
| }, | ||
| { refetchInterval: isPolling ? 5_000 : false }, |
There was a problem hiding this comment.
Before: we were relying on polling every 5_000 ms to get a new value for payrollData, then waiting for that new value to be "changed" such that a useEffect would re-run
| hasSeenCalculatingRef.current = true | ||
| } | ||
|
|
||
| const { data: blockersData } = usePayrollsGetBlockersSuspense({ |
There was a problem hiding this comment.
This is all just moved from below to here, no actual logic change
| setIsPolling(true) | ||
| pollRunRef.current = { | ||
| baselineCalculatedAt: payrollData.payrollShow?.calculatedAt?.getTime() ?? null, | ||
| sawCalculating: false, | ||
| } | ||
| startCalculationPoll() | ||
| } finally { |
There was a problem hiding this comment.
This is probably the best encapsulation of the change. Instead of setting isPolling to be true (which will then cause the options sent to the suspense query, which will keep loading until something changes, which will then cause a different useEffect to re-fire and set isPolling to false...eventually)...
we actually call a function whose entire job is to manage polling
| </Button> | ||
| )} | ||
| </Flex> | ||
| const emitProcessed = (payroll: PayrollShow | undefined) => { |
There was a problem hiding this comment.
Extract logic from the middle of the useEffect into functions for emitProcessed and emitProcessingFailed so we can consistently set the right alerts / event bodies
… them Pull the evaluate/onDone/onDeadline/fetch closures out of the usePollingTask options literal into named functions, and move the ones with no component state (PrintChecksForm's evaluate, PayrollConfiguration/PayrollOverview's evaluate given the poll-run ref as a param) to module scope. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Move each usePollingTask call site's fetch/evaluate rules into its own hook (useCalculationPoll, useSubmissionPoll, useGenerationPoll). Each hook owns only the polling mechanics and terminal-state rules; the component still owns when to start polling and what to do on completion. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
| const { start: startCalculationPoll, isPolling } = useCalculationPoll({ | ||
| payrollRequest, | ||
| onCalculated: (payroll: PayrollShow | undefined) => { | ||
| onEvent(componentEvents.RUN_PAYROLL_CALCULATED, { | ||
| payrollId, | ||
| alert: { | ||
| type: 'success', | ||
| title: t('alerts.progressSaved'), | ||
| alertKey: 'progressSaved', | ||
| }, | ||
| payPeriod: payroll?.payPeriod, | ||
| }) | ||
| setPayrollBlockers([]) | ||
| }, | ||
| onProcessingFailed: (payroll: PayrollShow | undefined) => { | ||
| onEvent(componentEvents.RUN_PAYROLL_PROCESSING_FAILED) | ||
| // Let prepare run again on retry — but only when there is no calculation for it to wipe. | ||
| if (payroll?.calculatedAt == null) { | ||
| hasSeenCalculatingRef.current = false | ||
| } | ||
| }, | ||
| }) |
There was a problem hiding this comment.
This is the main polling logic. All the actual business logic for "is this payroll still being calculated?" along with how long we should keep polling is in the useCalculationPoll hook -- the component is in charge of
- when to call
startCalculationPoll - what to do while
isPollingis active - what to do when the payroll is either calculated, or fails to calculate in time
| const evaluateCalculationOutcome = ( | ||
| data: PayrollsGetQueryData, | ||
| run: CalculationPollRun | null, | ||
| ): PollTickResult<CalculationOutcome> => { | ||
| const payroll = data.payrollShow | ||
|
|
||
| if (isCalculatingStatus(payroll?.processingRequest)) { | ||
| if (run) run.sawCalculating = true | ||
| return { done: false } | ||
| } | ||
|
|
||
| if (payroll?.processingRequest?.status === PayrollProcessingRequestStatus.ProcessingFailed) { | ||
| return { done: true, value: { type: 'failed', payroll } } | ||
| } | ||
|
|
||
| const calculatedAt = payroll?.calculatedAt | ||
| const isNewCalculation = | ||
| run?.sawCalculating === true || calculatedAt?.getTime() !== run?.baselineCalculatedAt | ||
|
|
||
| if (isNewCalculation && isCalculatedStatus(payroll?.processingRequest, calculatedAt)) { | ||
| return { done: true, value: { type: 'calculated', payroll } } | ||
| } | ||
|
|
||
| return { done: false } | ||
| } |
There was a problem hiding this comment.
All the business logic remains, but now it's in a plain old javascript functions. This is now the source of truth for "given the latest data from the server and our current knowledge about what we've seen so far, can we tell if this payroll is done calculating?"
There was a problem hiding this comment.
Domain-agnostic logic for how to poll on a regular basis, with support for translating any value into "is this a done state" and handlers for running up against a deadline. The three usages so far each call a react-query API for their fetch tasks, but this is purely generic
The three domain hooks each redeclared the same 5s/3min constants with no variation between them. Moving the defaults into usePollingTask lets callers omit them entirely instead of copying literals that only ever needed one value. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…etch useCalculationPoll and useSubmissionPoll each built a second, independent query (via queryClient.fetchQuery + buildPayrollsGetQuery) for the exact queryKey the component's own usePayrollsGet(Suspense) call already observes. Live-reproduced: the submit mutation's global invalidateQueries triggers a background refetch on that same key at nearly the same instant the poll's first tick starts, and TanStack can silently orphan one of the two promises rather than settling it — the poll's `await read(...)` then hangs forever with no error, well before its own deadline. Routing both hooks through the refetch already returned by the mounted query eliminates the second fetch path entirely, so there's nothing left to race. This also drops payrollRequest/useGustoEmbeddedContext/ useQueryClient/buildPayrollsGetQuery from both hooks. useGenerationPoll is intentionally left on fetchQuery: nothing renders generatedDocumentsGet data (it's only ever downloaded), so there's no sibling observer for it to race against and no refetch to reuse. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Summary
PayrollConfiguration,PayrollOverview, andPrintChecksFormeach drove a poll off a query'srefetchIntervaland decided completion in auseEffectreading rendered query data. That makes "is this done?" a function of whether the query observer notifies the component of new data. When it doesn't, the operation finishes correctly server-side and the screen never advances.PayrollOverviewis the worse case: its main query is non-suspense and gates the whole screen on!payrollData, so a dropped notification freezes the very first read, not just a later one.PrintChecksFormhad no deadline/failsafe at all, so a stuck poll would spin indefinitely with no way out.usePollingTask: a hook whose loop owns its own timer, reads the server directly each tick, and evaluates the terminal state inside its own closure — independent of whether the component ever re-renders. Converts all three surfaces to it, adding a 3-minute deadline (verified against the server before reporting failure) toPrintChecksFormfor parity with the other two.useCalculationPoll,useSubmissionPoll,useGenerationPoll) that wrapsusePollingTaskwith the domain's ownevaluate/onDeadlinerules, keeping the three components thin. The interval (5s) and deadline (3min) were identical across all three call sites with no reason to vary, so they're now defaults onusePollingTaskitself (DEFAULT_POLL_INTERVAL_MS/DEFAULT_POLL_DEADLINE_MS) rather than a constant redeclared three times.useCalculationPoll/useSubmissionPollpoll through the render query's ownrefetchinstead of building a second query. Live-repro onsdk-appsurfaced a real hang under this exact fix: each hook built an independent query (viaqueryClient.fetchQuery+buildPayrollsGetQuery) for the same queryKey the component's ownusePayrollsGet(Suspense)call already observes. The submit mutation's globalinvalidateQueries(seecreateSdkQueryClient) triggers a background refetch on that same key at nearly the same instant the poll's first tick starts, and TanStack can silently orphan one of the two promises rather than settling it — the poll'sawait read(...)then hangs forever, no error, well before its own 3-minute deadline. Routing both hooks through the already-mounted query'srefetchremoves the second fetch path entirely, so there's nothing left to race. This also dropspayrollRequest/useGustoEmbeddedContext/useQueryClient/buildPayrollsGetQueryfrom both hooks.useGenerationPollis intentionally left onfetchQuery— nothing rendersgeneratedDocumentsGetdata (it's only ever downloaded), so there's no sibling observer for it to race against and norefetchto reuse.Test plan
usePollingTaskunit suite (10/10): start/stop, retry-on-rejection, deadline-vs-terminal-result precedence, double-start/unmount guaranteesPayrollConfiguration.test.tsx(29/29, incl. 6 new regression tests for the SDK-1291 shape)PayrollOverview.test.tsx(9/9, incl. two tests exercising a real multi-tick pending→processed transition via fake timers)PrintChecksForm.test.tsx(11/11, incl. new coverage for the added deadline path)tsc --noEmitandeslintclean across all touched filessdk-app: reproduced the pre-fix hang on a real submit (payroll reachedsubmit_successserver-side while the poll sat silent past its deadline); re-ran the identical flow post-fix and it resolved to "Payroll submitted" within ~2s