fix: DIA-754: [FE] Search option does not reflect actual queries state

[yyassi-heartex]

PR fulfills these requirements

• Commit message(s) and PR title follows the format [fix|feat|ci|chore|doc]: TICKET-ID: Short description of change made ex. fix: DEV-XXXX: Removed inconsistent code usage causing intermittent errors
• Tests for the changes have been added/updated (for bug fixes/features)
• Docs have been added/updated (for bug fixes/features)
• Best efforts were made to ensure docs/code are concise and coherent (checked for spelling/grammatical errors, commented out code, debug logs etc.)
• Self-reviewed and ran all changes on a local instance (for bug fixes/features)

Change has impacts in these area(s)

(check all that apply)

• Product design
• Frontend

Describe the reason for change

api requests can now check if request is already in flight - if already in flight cancel first promise and use the new one. this prevents a race condition where second request resolves before the first causing the first to override changes applied by the second (where second one has the correct outcome due to having more up to date params)

[bmartel]

Is this apiCall the same one as the DM/LSF sdk uses all over? I only ask because that one already employs a AbortController concept. This does look like a different set of code though.

[yyassi-heartex]

i saw abort controller in LSP, not in DM

[bmartel]

Where does this call get made from? I realize there are differing calls now that I look, some are from the useAPI hook which underlying uses an api client, and those I believe have a similar but different method and signature to this one. Overall I think this change should be fine, the only concern I have is if this conflicts and breaks all the existing calls and hooks we have that use abort controllers.

[yyassi-heartex]

valid concern! from within datamanager apiCall is used in many places, annotations, tasks, tab, comments, etc basically we can safely assume any where we call a request to the api this is the function that handles it - i just saw that NotificationsProvider in LSP also calls it.

i did have that in mind though which is why i updated const requestBody = { signal, ...(apiTransform?.body?.(body) ?? body) }; like this, with the intent that signal can be overridden by any one using apiCall with their own signal

[bmartel]

Thanks for the clarity, and the foresight to handle it that way. Ok, I feel much better now about this given it has intent to be supportive of the various use cases. Last question, when an abort signal is sent, it produces an error which will be thrown and inevitably caught in the error handler (which produces a modal with the error) if not filtered out by the caller using the call options. Will this produce error modals in cases where the abort signal rightfully is working to ensure consistency on calls made?

[yyassi-heartex]

no i dont expect any modals to be generated here - i think it should be caught specifically by the calling function as it may need to be handled in different ways - the triggering function just needs this to cancel silently since there is a new request that we need to pay attention to and we're aiming to prevent race conditions that come up from following requests resolving after requests that are already in flight

[bmartel]

i think it should be caught specifically by the calling function as it may need to be handled in different ways

I agree, but my question is more to the effect of this change on all existing api calls, and whether this might end up producing modals for all unhandled cases where there are these abort signals.

[yyassi-heartex]

good point, alot of testing will be required to double check but from my testing, i didnt see any of these modals come up. i'll take a deeper look and mark the LSE pr ready for review to do some checking on FB aswell

[bmartel]

I'm thinking we might want to do something similar to what you have already here, and append to the errorFilter a check to remove the abort signal calls in every case there ends up being a signal (which is every time now). Because the one thing we do know, is regardless if someone supplied an errorFilter or not, we never want the abort signal to throw. In fact this would alleviate a lot of cases of having to remember to provide this to the api calls in general, because it isn't an error to be caught ever upstream.

[yyassi-heartex]

the only time cancelation trigger was on tabUpdate and tasks for both projects and datasets, this is a good thing since both these requests can easily be hammered just by repeatedly toggling a column. the most recent changes should alleviate this significantly and with the response now including isCanceled we can safely let the calling function deal with it (ie prevent continuing to the next step)