Skip to content

FIREFLY-1943: React Compiler bugs #1922

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

Merged
kpuriIpac merged 1 commit into from
Apr 8, 2026
Merged

FIREFLY-1943: React Compiler bugs #1922

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
11 changes: 11 additions & 0 deletions src/firefly/js/ui/SimpleComponent.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -152,6 +152,17 @@ export function useFieldGroupValue(fieldKey, gk) {
return [ getter, setter];
}

/**
* Convenience hook for components that only need the current field value and not the setter.
*
* @param {String} fieldKey - field key to read
* @param {*} def - default value to return when the field value is nullish
* @param {String} [gk] - optional group key; defaults to the current FieldGroup context
* @return {*} current field value or the provided default
*/
export function useFieldValueOnly(fieldKey, def, gk) {
return useFieldGroupValue(fieldKey, gk)[0]() ?? def;
}


/**
Expand Down
43 changes: 26 additions & 17 deletions src/firefly/js/ui/tap/Constraints.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,10 +3,27 @@ import React from 'react';

export const ConstraintContext = React.createContext({});

export function hasAdqlConstraint(constraintObj) {
return Boolean(
constraintObj?.adqlConstraint ||
constraintObj?.adqlConstraintsAry?.length
);
}

function getAdqlConstraintsList(constraintObj) {
if (constraintObj?.adqlConstraintsAry?.length) return constraintObj.adqlConstraintsAry;
if (constraintObj?.adqlConstraint) return [constraintObj.adqlConstraint];
return [];
}
Comment on lines +7 to +17
Copy link
Copy Markdown
Contributor

@robyww robyww Apr 1, 2026

Choose a reason for hiding this comment

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

good! getAdqlConstraintsList is a real improvement.

Copy link
Copy Markdown
Contributor

@robyww robyww Apr 1, 2026

Choose a reason for hiding this comment

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

You should add some sort of hasErrors to work with constraintObj.constraintErrors?.length or call is isContraintsValid


function isConstraintsValid(constraintObj) {
return !constraintObj?.constraintErrors?.length;
}

export function isTapUpload(tapBrowserState) {
const {constraintFragments}= tapBrowserState;
return [...constraintFragments.values()]
.some( (c) => Boolean(c.uploadFile && c.TAP_UPLOAD && c.adqlConstraint));
.some((c) => Boolean(c.uploadFile && c.TAP_UPLOAD && hasAdqlConstraint(c)));
}

/**
Expand All @@ -16,7 +33,8 @@ export function isTapUpload(tapBrowserState) {
*/
export function getUploadConstraint(tapBrowserState) {
const {constraintFragments}= tapBrowserState;
return [...constraintFragments.values()].find( (c) => Boolean(c.uploadFile && c.TAP_UPLOAD && c.adqlConstraint));
return [...constraintFragments.values()]
.find((c) => Boolean(c.uploadFile && c.TAP_UPLOAD && hasAdqlConstraint(c)));
}
Comment thread
kpuriIpac marked this conversation as resolved.

export function getTapUploadSchemaEntry(tapBrowserState) {
Expand All @@ -38,26 +56,17 @@ export function getHelperConstraints(tapBrowserState) {
const {constraintFragments}= tapBrowserState;
const adqlConstraints = [];
const adqlConstraintErrorsArray = [];
const siaConstraints = [];
// adqlComponents can apparently be modified during iteration in the forEach...

Array.from(constraintFragments.values()).forEach((constraintObj) => {
if (!constraintObj.adqlConstraintErrors?.length) {
if (constraintObj.adqlConstraint) {
adqlConstraints.push(constraintObj.adqlConstraint);
}
} else {
adqlConstraintErrorsArray.push(constraintObj.constraintErrors);
}
if (!constraintObj.constraintErrors?.length) {
if (constraintObj.siaConstraints?.length > 0) {
siaConstraints.push(...constraintObj.siaConstraints);
}
if (isConstraintsValid(constraintObj)) {
adqlConstraints.push(...getAdqlConstraintsList(constraintObj));
} else {
adqlConstraintErrorsArray.push(constraintObj.constraintErrors);
adqlConstraintErrorsArray.push(...constraintObj.constraintErrors);
}
});

return {
valid: adqlConstraintErrorsArray?.length === 0,
valid: adqlConstraintErrorsArray.length === 0,
messages: adqlConstraintErrorsArray,
where: adqlConstraints.join('\n AND ')
};
Expand Down
48 changes: 23 additions & 25 deletions src/firefly/js/ui/tap/SpatialSearch.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,19 +22,15 @@ import {calcCornerString, PolygonDataArea} from '../CatalogSearchMethodType.jsx'
import {FieldGroupCtx, ForceFieldGroupValid} from '../FieldGroup.jsx';
import {ListBoxInputField} from '../ListBoxInputField.jsx';
import {RadioGroupInputField} from '../RadioGroupInputField.jsx';
import {useFieldGroupRerender, useFieldGroupValue, useFieldGroupWatch} from '../SimpleComponent.jsx';
import {useFieldGroupRerender, useFieldGroupValue, useFieldGroupWatch,
useFieldValueOnly} from '../SimpleComponent.jsx';
import {SizeInputFields} from '../SizeInputField.jsx';
import {DEF_TARGET_PANEL_KEY} from '../TargetPanel.jsx';
import {ConstraintContext} from './Constraints.js';
import {ROW_POSITION, SEARCH_POSITION} from './Cutout';
import {getDataServiceOption} from './DataServicesOptions';
import {
DebugObsCore,
getPanelPrefix,
makeCollapsibleCheckHeader,
makeFieldErrorList,
makePanelStatusUpdater,
} from './TableSearchHelpers.jsx';
import {DebugObsCore, getPanelPrefix, makeCollapsibleCheckHeader, makeConstraintEntry, makeEmptyConstraints,
makeFieldErrorList} from './TableSearchHelpers.jsx';
import {showUploadTableChooser} from '../UploadTableChooser.js';
import {
getAsEntryForTableName, getColumnAttribute, getTapServiceByURL, makeUploadSchema, maybeQuote,
Expand Down Expand Up @@ -134,16 +130,13 @@ export function SpatialSearch({sx, cols, serviceUrl, serviceLabel, serviceId, co

const {setConstraintFragment}= useContext(ConstraintContext);
const {setVal,getVal,makeFldObj}= useContext(FieldGroupCtx);
const [constraintResult, setConstraintResult] = useState({});
const [getUploadInfo, setUploadInfo]= useFieldGroupValue('uploadInfo');
const [posDefaultOpenMsg, setPosDefaultOpenMsg]= useState(true);

useFieldGroupRerender([...fldListAry, ...collapsibleCheckHeaderKeys]); // force rerender on any change

const uploadInfo= getUploadInfo() || undefined;

const updatePanelStatus= makePanelStatusUpdater(checkHeaderCtl.isPanelActive(), Spatial);

useEffect(() => {
if (!canUpload) setVal(SPATIAL_TYPE,SINGLE);
}, [serviceUrl,canUpload]);
Expand Down Expand Up @@ -236,15 +229,23 @@ export function SpatialSearch({sx, cols, serviceUrl, serviceLabel, serviceId, co

useFieldGroupWatch([cornerCalcType], () => onChangeToPolygonMethod());

useEffect(() => {
const constraints= makeSpatialConstraints(columnsModel, obsCoreEnabled, makeFldObj(fldListAry), uploadInfo, tableName, canUpload,useSIAv2);
updatePanelStatus(constraints, constraintResult, setConstraintResult,useSIAv2);
});

const isSpatialPanelActive = checkHeaderCtl?.isPanelActive();

const constraintResult = React.useMemo(() => {
if (!isSpatialPanelActive) return makeEmptyConstraints();

const constraints = makeSpatialConstraints(
columnsModel, obsCoreEnabled, makeFldObj(fldListAry),
uploadInfo, tableName, canUpload, useSIAv2
);

return makeConstraintEntry(constraints);
}, [...fldListAry.map((v) => getVal(v))]);

Copy link
Copy Markdown
Contributor

@robyww robyww Apr 1, 2026

Choose a reason for hiding this comment

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

I really like this approach. It is much clean and easier to follow. However, I think it is to verbose and hard to consistently reuse. So here are a couple of ideas.

this will help a lot

fldListAry.map

Idea 1

define in TableSearchHelpers.js

function makeConstrainstHelper(active, make)  {
        if (!active) return { adqlConstraintsAry: [], constraintErrors: [], simpleError: '', };

        const constraints = make();
        return {
            ...constraints,
            constraintErrors: [...(constraints.errAry ?? [])],
            simpleError: constraints.errAry?.[0] ?? '',
        };
 }

then your use Memo function

    const constraintResult = React.useMemo(() => {
        return doContrains(
            isSpatialPanelActive,
            () => makeSpatialConstraints( columnsModel, obsCoreEnabled, makeFldObj(fldListAry),
                uploadInfo, tableName, canUpload, useSIAv2)
            );
    }, [...fldListAry.map((v)=> getVal(v)) ]);

Idea 2

define in TableSearchHelpers.js the short functions makeEmptyConstrains() and a makeConstrainstEntry(constraints)

then

   const constraintResult1 = React.useMemo(() => {
        if (!isSpatialPanelActive) return makeEmptyConstrains();

        const constraints = makeSpatialConstraints(
            columnsModel, obsCoreEnabled, makeFldObj(fldListAry),
            uploadInfo, tableName, canUpload, useSIAv2
        );

        return makeConstrainstEntry(constraints);
    }, [...fldListAry.map((v)=> getVal(v)) ]);

Copy link
Copy Markdown
Member

@jaladh-singhal jaladh-singhal Apr 2, 2026

Choose a reason for hiding this comment

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

I vote for idea 2

useEffect(() => {
setConstraintFragment(panelPrefix, constraintResult);
return () => setConstraintFragment(panelPrefix, '');
}, [constraintResult]);
}, [panelPrefix, setConstraintFragment, constraintResult]);

if (disablePanel) {
return (
Expand Down Expand Up @@ -320,14 +321,11 @@ function getSpacialLayoutMode(spacialType, obsCoreEnabled, canUpload) {

const SpatialSearchLayout = ({initArgs, obsCoreEnabled, uploadInfo, setUploadInfo, serviceLabel, serviceId,
hipsUrl, centerWP, fovDeg, capabilities, embeddedInHiPS, slotProps}) => {

const {getVal}= useContext(FieldGroupCtx);

const spacialType= getVal(SPATIAL_TYPE) ?? SINGLE;
const spatialMethod= getVal(SpatialMethod)??CONE_CHOICE_KEY;
const closest= getVal(Closest)??'';
const cornerCalcTypeValue= getVal(cornerCalcType)??'image';
const spatialRegOpValue= getVal(SpatialRegOp) ?? SpatialRegOpType.CONTAINS_POINT;
const spacialType = useFieldValueOnly(SPATIAL_TYPE, SINGLE);
const spatialMethod = useFieldValueOnly(SpatialMethod, CONE_CHOICE_KEY);
const closest = useFieldValueOnly(Closest, '');
const cornerCalcTypeValue = useFieldValueOnly(cornerCalcType, 'image');
const spatialRegOpValue = useFieldValueOnly(SpatialRegOp, SpatialRegOpType.CONTAINS_POINT);
const layoutMode= getSpacialLayoutMode(spacialType,obsCoreEnabled,capabilities?.canUpload);
const isCone= spatialMethod === CONE_CHOICE_KEY;
const containsPoint= spatialRegOpValue === SpatialRegOpType.CONTAINS_POINT;
Expand Down
18 changes: 17 additions & 1 deletion src/firefly/js/ui/tap/TableSearchHelpers.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,22 @@ function getPanelAdqlConstraint(panelActive, panelTitle,constraintsValid,adqlCon
}
}

export function makeEmptyConstraints() {
return {
adqlConstraintsAry: [],
constraintErrors: [],
simpleError: '',
};
}

export function makeConstraintEntry(constraints = {}) {
return {
...constraints,
constraintErrors: [...(constraints.errAry ?? [])],
simpleError: constraints.errAry?.[0] ?? '',
};
}

/**
*
* @param {boolean} panelActive
Expand Down Expand Up @@ -393,4 +409,4 @@ export function TapTitleCustomizeButton({groupKey, tapBrowserState, selectBy}) {
* @prop {Array.<String>} uploadColumns
* @prop {Object} TAP_UPLOAD
* @prop {String} uploadFile
*/
*/
8 changes: 4 additions & 4 deletions src/firefly/js/ui/tap/TapSearchSubmit.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ import {
getHelperConstraints,
getTapUploadSchemaEntry,
getUploadServerFile,
getUploadTableName,
getUploadTableName, hasAdqlConstraint,
isTapUpload
} from 'firefly/ui/tap/Constraints';
import {makeTblRequest, setNoCache} from 'firefly/tables/TableRequestUtil';
Expand Down Expand Up @@ -105,9 +105,9 @@ export function onTapSearchSubmit({request, serviceUrl, tapBrowserState, additio

function getCutoutType(tapBrowserState) {
const spatial= tapBrowserState?.constraintFragments?.get('spatial');
if (spatial?.adqlConstraint?.length) return spatial.cutoutType;
if (hasAdqlConstraint(spatial)) return spatial.cutoutType;
const location= tapBrowserState?.constraintFragments?.get('location');
if (location?.adqlConstraint?.length) return location.cutoutType;
if (hasAdqlConstraint(location)) return location.cutoutType;
return ROW_POSITION;
}

Expand All @@ -129,7 +129,7 @@ export function getAdqlQuery(tapBrowserState, additionalClauses, allowColumnCons
if (isUpload) { //check for more than one upload file (in Spatial and in ObjectID col) - should this be a utility function in constraints.js?
const { constraintFragments } = tapBrowserState;
const entries = [...constraintFragments.values()];
const matchingEntries = entries.filter((c) => Boolean(c.uploadFile && c.TAP_UPLOAD && c.adqlConstraint));
const matchingEntries = entries.filter((c) => Boolean(c.uploadFile && c.TAP_UPLOAD && hasAdqlConstraint(c)));
if (matchingEntries.length > 1) {
if (showErrors) showInfoPopup('We currently do not support searches with more than one uploaded table.', 'Error');
return;
Expand Down