Skip to content
This repository has been archived by the owner on Apr 18, 2024. It is now read-only.

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

Merged
merged 6 commits into from
Dec 6, 2023
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
a0ce76c
fix: DIA-754: [FE] Search option does not reflect actual queries state
yyassi-heartex Dec 1, 2023
92241fb
reduced hammering as a result of cancelations
yyassi-heartex Dec 5, 2023
2686bbc
restricting cancelable requests to use as needed and DE only
yyassi-heartex Dec 5, 2023
042e2b2
tweaking var name
yyassi-heartex Dec 5, 2023
ec0352b
not sure why this is suddenly breaking but it causes issues so making…
yyassi-heartex Dec 5, 2023
f737fc1
reducing craziness on isCanceled
yyassi-heartex Dec 5, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 7 additions & 6 deletions src/mixins/DataStore/DataStore.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -181,12 +181,13 @@ export const DataStore = (
fetch: flow(function* ({ id, query, pageNumber = null, reload = false, interaction, pageSize } = {}) {
let currentViewId, currentViewQuery;
const requestId = self.requestId = guidGenerator();
const root = getRoot(self);

if (id) {
currentViewId = id;
currentViewQuery = query;
} else {
const currentView = getRoot(self).viewsStore.selected;
const currentView = root.viewsStore.selected;

currentViewId = currentView?.id;
currentViewQuery = currentView?.virtual ? currentView?.query : null;
Expand Down Expand Up @@ -226,18 +227,18 @@ export const DataStore = (

if (interaction) Object.assign(params, { interaction });

const data = yield getRoot(self).apiCall(apiMethod, params);
const data = yield root.apiCall(apiMethod, params, {}, { allowToCancel: root.SDK.type === 'DE' });

// We cancel current request processing if request id
// cnhaged during the request. It indicates that something
// changed during the request. It indicates that something
// triggered another request while current one is not yet finished
if (requestId !== self.requestId) {
if (requestId !== self.requestId || data.isCanceled) {
console.log(`Request ${requestId} was cancelled by another request`);
return;
}

const highlightedID = self.highlighted;
const apiMethodSettings = getRoot(self).API.getSettingsByMethodName(apiMethod);
const apiMethodSettings = root.API.getSettingsByMethodName(apiMethod);
const { total, [apiMethod]: list } = data;
let associatedList = [];

Expand All @@ -260,7 +261,7 @@ export const DataStore = (

self.loading = false;

getRoot(self).SDK.invoke('dataFetched', self);
root.SDK.invoke('dataFetched', self);
}),

reload: flow(function* ({ id, query, interaction } = {}) {
Expand Down
29 changes: 24 additions & 5 deletions src/stores/AppStore.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -135,6 +135,7 @@ export const AppStore = types
.volatile(() => ({
needsDataFetch: false,
projectFetch: false,
requestsInFlight: new Map(),
}))
.actions((self) => ({
startPolling() {
Expand Down Expand Up @@ -541,16 +542,34 @@ export const AppStore = types
* @param {string} methodName one of the methods in api-config
* @param {object} params url vars and query string params
* @param {object} body for POST/PATCH requests
* @param {{ errorHandler?: fn }} [options] additional options like errorHandler
* @param {{ errorHandler?: fn, headers?: object, allowToCancel?: boolean }} [options] additional options like errorHandler
*/
apiCall: flow(function* (methodName, params, body, options) {
const isAllowCancel = options?.allowToCancel;
const controller = new AbortController();
const signal = controller.signal;
Comment on lines +549 to +550
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i saw abort controller in LSP, not in DM

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@bmartel bmartel Dec 4, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

@yyassi-heartex yyassi-heartex Dec 4, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@bmartel bmartel Dec 4, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@bmartel bmartel Dec 4, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

const apiTransform = self.SDK.apiTransform?.[methodName];
const requestParams = apiTransform?.params?.(params) ?? params ?? {};
const requestBody = apiTransform?.body?.(body) ?? body ?? undefined;

let result = yield self.API[methodName](requestParams, requestBody);
const requestBody = apiTransform?.body?.(body) ?? body ?? {};
const requestHeaders = apiTransform?.headers?.(options?.headers) ?? options?.headers ?? {};
const requestKey = `${methodName}_${JSON.stringify(params || {})}`;

if (isAllowCancel) {
requestHeaders.signal = signal;
if (self.requestsInFlight.has(requestKey)) {
/* if already in flight cancel the first in favor of new one */
self.requestsInFlight.get(requestKey).abort();
console.log(`Request ${requestKey} canceled`);
}
self.requestsInFlight.set(requestKey, controller);
}
let result = yield self.API[methodName](requestParams, { headers: requestHeaders, body: requestBody.body ?? requestBody });

if (result.error && result.status !== 404) {
if (isAllowCancel) {
result.isCanceled = signal.aborted;
self.requestsInFlight.delete(requestKey);
}
if (result.error && result.status !== 404 && !signal.aborted) {
if (options?.errorHandler?.(result)) {
return result;
}
Expand Down
6 changes: 5 additions & 1 deletion src/stores/Tabs/store.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -302,7 +302,11 @@ export const TabStore = types
const apiMethod =
!view.saved && root.apiVersion === 2 ? "createTab" : "updateTab";

const result = yield root.apiCall(apiMethod, params, body);
const result = yield root.apiCall(apiMethod, params, body, { allowToCancel: root.SDK.type === 'DE' });

if (result.isCanceled) {
return view;
}
const viewSnapshot = getSnapshot(view);
const newViewSnapshot = {
...viewSnapshot,
Expand Down
6 changes: 3 additions & 3 deletions src/stores/Tabs/tab.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -396,11 +396,11 @@ export const Tab = types
}
if (self.virtual) {
yield self.dataStore.reload({ query: self.query, interaction });
} else if (isFF(FF_LOPS_12) && self.root.SDK.type === 'labelops') {
} else if (isFF(FF_LOPS_12) && self.root.SDK?.type === 'labelops') {
yield self.dataStore.reload({ query: self.query, interaction });
}

getRoot(self).SDK.invoke("tabReloaded", self);
getRoot(self).SDK?.invoke?.("tabReloaded", self);
}),

deleteFilter(filter) {
Expand Down Expand Up @@ -439,7 +439,7 @@ export const Tab = types

History.navigate({ tab: self.key }, true);
self.reload({ interaction });
} else if (isFF(FF_LOPS_12) && self.root.SDK.type === 'labelops') {
} else if (isFF(FF_LOPS_12) && self.root.SDK?.type === 'labelops') {
const snapshot = self.serialize();

self.key = self.parent.snapshotToUrl(snapshot);
Expand Down
2 changes: 1 addition & 1 deletion src/utils/api-proxy/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -248,7 +248,7 @@ export class APIProxy {
rawResponse = await fetch(apiCallURL, requestParams);
}

if (raw) return rawResponse;
if (raw || rawResponse.isCanceled) return rawResponse;

responseMeta = {
headers: new Map(Array.from(rawResponse.headers)),
Expand Down