[Bug]: DeliveriesTable calls DeliveryFunction twice when sorting

[arnejduranovic]

Contact Details

No response

What happened?

DeliveriesTable calls DeliveryFunction twice when sorting. It should only call it once. This is observable in the browser Network tab.

Steps to repro:

1. Log in to ReportStream
2. Go Daily data
3. Open network tab in browser
4. Click "Available" column header to sort
5. Check Network tab, should see two identical calls were made

Background

It looks like there's an issue with the usePagination hook: on every sort-by-column operation, there are three actions dispatched:

RESET PROCESS_RESULTS PROCESS_RESULTS

The first PROCESS_RESULTS has an empty results payload and a different cursor.

Should just be DeliveriesTable and SubmissionTable (we think). It may require a bit more rework to make sure the dispatched events are coming through correctly.

Version

1.0.2 (Default)

What browsers are you seeing the problem on?

Firefox, Chrome, Safari, Microsoft Edge

Relevant log output

No response

Acceptance Criteria

• [ ] When sorting by column on the DeliveriesTable, we should only fire one fetch request
• [ ] When sorting by column on the SubmissionTable, we should only fire one fetch request

Code of Conduct

• [X] I agree to follow this project's Code of Conduct

[hannasage]

Thanks for this @arnejduranovic! Will triage this next week and find a spot for it in our queue.

[hannasage]

Useful docs for whomever picks this up: Fetching w/ Effects Prevent effect from firing too often

[chris-kuryak]

Per @stephenkao There are two delivery requests, but one has a cursor of 2000 and the other of 3000, so they are not identical. But we shouldn't be sending two requests anyway.

[chris-kuryak]

@stephenkao When you have finished your investigation, come in and fix up this ticket and let me know. And then we'll put it up for backlog refinement.

[stephenkao]

Just did a bit of investigation, and it looks like there's an issue with the usePagination hook: on every sort-by-column operation, there are three actions dispatched:

• RESET
• PROCESS_RESULTS
• PROCESS_RESULTS

The potential issues I see here are that 1) we should only be firing one event and 2) the first PROCESS_RESULTS has an empty results payload and a different cursor.

I'm going to mark this as a 3 because it's affecting anything that uses the usePagination hook (should just be DeliveriesTable and SubmissionTable I think?) and it may require a bit more rework to make sure the dispatched events are coming through correctly.

[chris-kuryak]

@stephenkao Check out the acceptance criteria I added. Should there be anything more than that?

[stephenkao]

@chris-kuryak Ah, I just added a bit more detail to make sure we cover both use cases in production. Thanks for calling this out!

[etanb]

@stephenkao would you be able to provide some more insight into this code in UsePagination.ts:

// Reset the state if any of the hook props change.
    useEffect(() => {
        dispatch({
            type: PaginationActionType.RESET,
            payload: {
                startCursor,
                isCursorInclusive,
                pageSize,
                extractCursor,
            },
        });
    }, [fetchResults, pageSize, startCursor, extractCursor, isCursorInclusive]);

With that dependency array full, useDeepCompareEffect gets called multiple times with the reset cursor 2000 then 3000 but when I dump the dependency array in the above useEffect, useDeepCompareEffect gets called once which is what we're looking for.

My gut says that the above reset function should be called directly when it's needed, and not EVERY time fetchResults, pageSize, startCursor, extractCursor, or isCursorInclusive changes.

[stephenkao]

@stephenkao would you be able to provide some more insight into this code in UsePagination.ts:

// Reset the state if any of the hook props change.
    useEffect(() => {
        dispatch({
            type: PaginationActionType.RESET,
            payload: {
                startCursor,
                isCursorInclusive,
                pageSize,
                extractCursor,
            },
        });
    }, [fetchResults, pageSize, startCursor, extractCursor, isCursorInclusive]);

With that dependency array full, useDeepCompareEffect gets called multiple times with the reset cursor 2000 then 3000 but when I dump the dependency array in the above useEffect, useDeepCompareEffect gets called once which is what we're looking for.

My gut says that the above reset function should be called directly when it's needed, and not EVERY time fetchResults, pageSize, startCursor, extractCursor, or isCursorInclusive changes.

@etanb I'm as new to this project as you are, so I don't have any additional context over the code :) But I'm happy to pair on this with you -- just let me know!

One thing to note is that our endpoints only return arrays as opposed to objects that would contain helpful metadata specifically about contained entity types and pagination. SO there's some wonky logic around fetching the first 5-6 pages to render the page buttons in the paginator and then fetching individual pages after that. This logic may be used to work around that, but I can do some digging later today or tomorrow. I'll also add a card to propose a payload structure change -- that'd help simplify a lot of this weird pagination logic in the future.