Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: updated the logic for variable update queue #6586

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
+800 −109
Open

feat: updated the logic for variable update queue #6586

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
645bb17
feat: updated the logic for variable update queue
SagarRajput-7 Dec 4, 2024
af840fb
feat: added API limiting to reduce unnecessary api call for dashboard…
SagarRajput-7 Dec 17, 2024
5097375
feat: add jest test cases for new logic's utils, functions and proces…
SagarRajput-7 Dec 17, 2024
0c3282e
Merge branch 'main' into variable-update-queue
srikanthccv Dec 24, 2024
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
69 changes: 43 additions & 26 deletions ...end/src/container/NewDashboard/DashboardVariablesSelection/DashboardVariableSelection.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,15 @@
import { Row } from 'antd';
import { isNull } from 'lodash-es';
import { useDashboard } from 'providers/Dashboard/Dashboard';
import { memo, useEffect, useState } from 'react';
import { memo, useEffect, useRef, useState } from 'react';
import { IDashboardVariable } from 'types/api/dashboard/getAll';

import {
buildDependencies,
buildDependencyGraph,
buildParentDependencyGraph,
onUpdateVariableNode,
VariableGraph,
} from './util';
import VariableItem from './VariableItem';

function DashboardVariableSelection(): JSX.Element | null {
Expand All @@ -21,6 +27,12 @@ function DashboardVariableSelection(): JSX.Element | null {

const [variablesTableData, setVariablesTableData] = useState<any>([]);

const [dependencyData, setDependencyData] = useState<{
order: string[];
graph: VariableGraph;
Copy link
Collaborator

@vikrantgupta25 vikrantgupta25 Dec 17, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

extract out to independent type , IDependencyData

parentDependencyGraph: VariableGraph;
} | null>(null);

useEffect(() => {
if (variables) {
const tableRowData = [];
Expand All @@ -43,35 +55,28 @@ function DashboardVariableSelection(): JSX.Element | null {
}
}, [variables]);

const onVarChanged = (name: string): void => {
/**
* this function takes care of adding the dependent variables to current update queue and removing
* the updated variable name from the queue
*/
const dependentVariables = variablesTableData
?.map((variable: any) => {
if (variable.type === 'QUERY') {
const re = new RegExp(`\\{\\{\\s*?\\.${name}\\s*?\\}\\}`); // regex for `{{.var}}`
const queryValue = variable.queryValue || '';
const dependVarReMatch = queryValue.match(re);
if (dependVarReMatch !== null && dependVarReMatch.length > 0) {
return variable.name;
}
}
return null;
})
.filter((val: string | null) => !isNull(val));
setVariablesToGetUpdated((prev) => [
...prev.filter((v) => v !== name),
...dependentVariables,
]);
};
const initializationRef = useRef(false);

useEffect(() => {
if (variablesTableData.length > 0 && !initializationRef.current) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

why is this required ?

  • since the initial state of dependency data is null, a null check would be sufficient to know the first load ?
  • on similar lines we do not want to re-calculate the deps graph / order when variablesTableData is updated ? when adding or removing variables from the dashboard ?

const depGrp = buildDependencies(variablesTableData);
const { order, graph } = buildDependencyGraph(depGrp);
const parentDependencyGraph = buildParentDependencyGraph(graph);
setDependencyData({
order,
graph,
parentDependencyGraph,
});
initializationRef.current = true;
}
}, [variablesTableData]);

const onValueUpdate = (
name: string,
id: string,
value: IDashboardVariable['selectedValue'],
allSelected: boolean,
isMountedCall?: boolean,
// eslint-disable-next-line sonarjs/cognitive-complexity
): void => {
if (id) {
Expand Down Expand Up @@ -111,7 +116,18 @@ function DashboardVariableSelection(): JSX.Element | null {
});
}

onVarChanged(name);
if (dependencyData && !isMountedCall) {
const updatedVariables: string[] = [];
onUpdateVariableNode(
name,
dependencyData.graph,
dependencyData.order,
(node) => updatedVariables.push(node),
);
setVariablesToGetUpdated(updatedVariables.filter((v) => v !== name));
} else if (isMountedCall) {
setVariablesToGetUpdated((prev) => prev.filter((v) => v !== name));
}
}
};

Expand Down Expand Up @@ -139,6 +155,7 @@ function DashboardVariableSelection(): JSX.Element | null {
onValueUpdate={onValueUpdate}
variablesToGetUpdated={variablesToGetUpdated}
setVariablesToGetUpdated={setVariablesToGetUpdated}
dependencyData={dependencyData}
/>
))}
</Row>
Expand Down
191 changes: 108 additions & 83 deletions frontend/src/container/NewDashboard/DashboardVariablesSelection/VariableItem.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ import { commaValuesParser } from 'lib/dashbaordVariables/customCommaValuesParse
import sortValues from 'lib/dashbaordVariables/sortVariableValues';
import { debounce, isArray, isString } from 'lodash-es';
import map from 'lodash-es/map';
import { ChangeEvent, memo, useEffect, useMemo, useState } from 'react';
import { ChangeEvent, memo, useEffect, useMemo, useRef, useState } from 'react';
import { useQuery } from 'react-query';
import { useSelector } from 'react-redux';
import { AppState } from 'store/reducers';
Expand All @@ -35,12 +35,15 @@ import { popupContainer } from 'utils/selectPopupContainer';

import { variablePropsToPayloadVariables } from '../utils';
import { SelectItemStyle } from './styles';
import { areArraysEqual } from './util';
import {
areArraysEqual,
checkAPIInvocation,
onUpdateVariableNode,
VariableGraph,
} from './util';

const ALL_SELECT_VALUE = '__ALL__';

const variableRegexPattern = /\{\{\s*?\.([^\s}]+)\s*?\}\}/g;

enum ToggleTagValue {
Only = 'Only',
All = 'All',
Expand All @@ -54,9 +57,15 @@ interface VariableItemProps {
id: string,
arg1: IDashboardVariable['selectedValue'],
allSelected: boolean,
isMountedCall?: boolean,
) => void;
variablesToGetUpdated: string[];
setVariablesToGetUpdated: React.Dispatch<React.SetStateAction<string[]>>;
dependencyData: {
order: string[];
graph: VariableGraph;
parentDependencyGraph: VariableGraph;
} | null;
}

const getSelectValue = (
Expand All @@ -79,6 +88,7 @@ function VariableItem({
onValueUpdate,
variablesToGetUpdated,
setVariablesToGetUpdated,
dependencyData,
}: VariableItemProps): JSX.Element {
const [optionsData, setOptionsData] = useState<(string | number | boolean)[]>(
[],
Expand All @@ -88,59 +98,28 @@ function VariableItem({
(state) => state.globalTime,
);

useEffect(() => {
if (variableData.allSelected && variableData.type === 'QUERY') {
setVariablesToGetUpdated((prev) => {
const variablesQueue = [...prev.filter((v) => v !== variableData.name)];
if (variableData.name) {
variablesQueue.push(variableData.name);
}
return variablesQueue;
});
}
// eslint-disable-next-line react-hooks/exhaustive-deps
}, [minTime, maxTime]);

const [errorMessage, setErrorMessage] = useState<null | string>(null);
// logic to detect if its a rerender or a new render/mount
const isMounted = useRef(false);
Copy link
Collaborator

Choose a reason for hiding this comment

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

why is onMount handling different ? onMount should be handled via the root triggers ?


const getDependentVariables = (queryValue: string): string[] => {
const matches = queryValue.match(variableRegexPattern);
useEffect(() => {
isMounted.current = true;
}, []);

// Extract variable names from the matches array without {{ . }}
return matches
? matches.map((match) => match.replace(variableRegexPattern, '$1'))
: [];
const validVariableUpdate = (): boolean => {
if (!variableData.name) {
return false;
}
if (!isMounted.current) {
// variableData.name is present as the top element or next in the queue - variablesToGetUpdated
return Boolean(
variablesToGetUpdated.length &&
variablesToGetUpdated[0] === variableData.name,
);
}
return variablesToGetUpdated.includes(variableData.name);
};

const getQueryKey = (variableData: IDashboardVariable): string[] => {
let dependentVariablesStr = '';

const dependentVariables = getDependentVariables(
variableData.queryValue || '',
);

const variableName = variableData.name || '';

dependentVariables?.forEach((element) => {
const [, variable] =
Object.entries(existingVariables).find(
([, value]) => value.name === element,
) || [];

dependentVariablesStr += `${element}${variable?.selectedValue}`;
});

const variableKey = dependentVariablesStr.replace(/\s/g, '');

// added this time dependency for variables query as API respects the passed time range now
return [
REACT_QUERY_KEY.DASHBOARD_BY_ID,
variableName,
variableKey,
`${minTime}`,
`${maxTime}`,
];
};
const [errorMessage, setErrorMessage] = useState<null | string>(null);

// eslint-disable-next-line sonarjs/cognitive-complexity
const getOptions = (variablesRes: VariableResponseProps | null): void => {
Expand Down Expand Up @@ -184,9 +163,7 @@ function VariableItem({
if (
variableData.type === 'QUERY' &&
variableData.name &&
(variablesToGetUpdated.includes(variableData.name) ||
valueNotInList ||
variableData.allSelected)
(validVariableUpdate() || valueNotInList || variableData.allSelected)
) {
let value = variableData.selectedValue;
let allSelected = false;
Expand All @@ -200,7 +177,16 @@ function VariableItem({
}

if (variableData && variableData?.name && variableData?.id) {
onValueUpdate(variableData.name, variableData.id, value, allSelected);
onValueUpdate(
variableData.name,
variableData.id,
value,
allSelected,
isMounted.current,
);
setVariablesToGetUpdated((prev) =>
Copy link
Collaborator

Choose a reason for hiding this comment

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

onValueUpdate should be responsible to remove the variable from variablesToGetUpdated ?

prev.filter((name) => name !== variableData.name),
);
}
}

Expand All @@ -224,36 +210,75 @@ function VariableItem({
}
};

const { isLoading } = useQuery(getQueryKey(variableData), {
enabled: variableData && variableData.type === 'QUERY',
queryFn: () =>
dashboardVariablesQuery({
query: variableData.queryValue || '',
variables: variablePropsToPayloadVariables(existingVariables),
}),
refetchOnWindowFocus: false,
onSuccess: (response) => {
getOptions(response.payload);
},
onError: (error: {
details: {
error: string;
};
}) => {
const { details } = error;

if (details.error) {
let message = details.error;
if (details.error.includes('Syntax error:')) {
message =
'Please make sure query is valid and dependent variables are selected';
const { isLoading } = useQuery(
[
REACT_QUERY_KEY.DASHBOARD_BY_ID,
variableData.name || '',
`${minTime}`,
`${maxTime}`,
],
{
enabled:
variableData &&
variableData.type === 'QUERY' &&
checkAPIInvocation(
variablesToGetUpdated,
variableData,
dependencyData?.parentDependencyGraph,
),
queryFn: () =>
dashboardVariablesQuery({
query: variableData.queryValue || '',
variables: variablePropsToPayloadVariables(existingVariables),
}),
refetchOnWindowFocus: false,
onSuccess: (response) => {
getOptions(response.payload);
if (
dependencyData?.parentDependencyGraph[variableData.name || ''].length === 0
) {
const updatedVariables: string[] = [];
onUpdateVariableNode(
variableData.name || '',
dependencyData.graph,
dependencyData.order,
(node) => updatedVariables.push(node),
);
setVariablesToGetUpdated((prev) => [
...prev,
...updatedVariables.filter((v) => v !== variableData.name),
]);
}
setErrorMessage(message);
}
},
onError: (error: {
details: {
error: string;
};
}) => {
const { details } = error;

if (details.error) {
let message = details.error;
if (details.error.includes('Syntax error:')) {
message =
'Please make sure query is valid and dependent variables are selected';
}
setErrorMessage(message);
}
},
},
});
);

const handleChange = (value: string | string[]): void => {
// if value is equal to selected value then return
if (
value === variableData.selectedValue ||
(Array.isArray(value) &&
Array.isArray(variableData.selectedValue) &&
areArraysEqual(value, variableData.selectedValue))
) {
return;
}
if (variableData.name) {
if (
value === ALL_SELECT_VALUE ||
Expand Down
Loading
Loading