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

Open
kpuriIpac wants to merge 5 commits into
base: dev
Choose a base branch
from
Open

FIREFLY-1943: React Compiler bugs #1922

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
07b0882
react compiler bug fixes and improvements
kpuriIpac Mar 17, 2026
b233af3
more fixes
kpuriIpac Mar 17, 2026
6b4b61a
tap dropdown and user entered urls bug fixes
kpuriIpac Mar 18, 2026
9b0cb5f
some more cleanup
kpuriIpac Mar 27, 2026
6ff9f62
revert changes for additional bugs
kpuriIpac Mar 31, 2026
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
2 changes: 1 addition & 1 deletion buildScript/webpack.config.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ export default function makeWebpackConfig(config) {
const MIN_CHROME_VERSION= '130';
const MIN_FIREFOX_VERSION= '134';
const MIN_EDGE_VERSION= '130';
const useReactCompiler = false;
const useReactCompiler = true;

if (!process.env.NODE_ENV) {
process.env.NODE_ENV = ['local', 'dev'].includes(BUILD_ENV) ? 'development' : 'production';
Expand Down
39 changes: 22 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,23 @@ 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


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 +29,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)));
}

export function getTapUploadSchemaEntry(tapBrowserState) {
Expand All @@ -38,26 +52,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);
}
if (!constraintObj?.constraintErrors?.length) {
adqlConstraints.push(...getAdqlConstraintsList(constraintObj));
} else {
adqlConstraintErrorsArray.push(constraintObj.constraintErrors);
}
if (!constraintObj.constraintErrors?.length) {
if (constraintObj.siaConstraints?.length > 0) {
siaConstraints.push(...constraintObj.siaConstraints);
}
} 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
71 changes: 49 additions & 22 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 @@ -28,12 +28,7 @@ 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,
import {DebugObsCore, getPanelPrefix, makeCollapsibleCheckHeader, makeFieldErrorList,
} from './TableSearchHelpers.jsx';
import {showUploadTableChooser} from '../UploadTableChooser.js';
import {
Expand Down Expand Up @@ -134,16 +129,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 +228,52 @@ 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 targetWp = getVal(ServerParams.USER_TARGET_WORLD_PT);
const spatialRegOp = getVal(SpatialRegOp);
const spatialType = getVal(SPATIAL_TYPE);
const spatialMethodVal = getVal(SpatialMethod);
const radiusVal = getVal(RadiusSize);
const polygonCornersVal = getVal(PolygonCorners);
const centerLonVal = getVal(CenterLonColumns);
const centerLatVal = getVal(CenterLatColumns);
const uploadCenterLonVal = getVal(UploadCenterLonColumns);
const uploadCenterLatVal = getVal(UploadCenterLatColumns);
const cornerCalcTypeVal = getVal(cornerCalcType);
const closestVal = getVal(Closest);

const isSpatialPanelActive = checkHeaderCtl?.isPanelActive();

const constraintResult = React.useMemo(() => {

//when spatial panel is inactive, it should not contribute constraints or errors
if (!isSpatialPanelActive) {
return {
adqlConstraintsAry: [],
constraintErrors: [],
simpleError: '',
};
}

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

return {
...constraints,
constraintErrors: [...(constraints.errAry ?? [])],
simpleError: constraints.errAry?.[0] ?? '',
};
}, [isSpatialPanelActive, columnsModel, obsCoreEnabled, uploadInfo, tableName, canUpload,
useSIAv2, targetWp, spatialRegOp, spatialType, spatialMethodVal,
radiusVal, polygonCornersVal, centerLonVal, centerLatVal,
uploadCenterLonVal, uploadCenterLatVal, cornerCalcTypeVal, closestVal,
]);

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 @@ -321,13 +350,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 = useFieldGroupValue(SPATIAL_TYPE)[0]() ?? SINGLE;
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.

This maybe just preference issue, I find useStoreConnector cleaner for these cases. I consider useFieldGroupValue more for cases where I want a getter function that I'll call later.

Suggested change
const spacialType = useFieldGroupValue(SPATIAL_TYPE)[0]() ?? SINGLE;
const spacialType = useStoreConnector(() => getFieldVal(groupKey, SPATIAL_TYPE, SINGLE));

You can get groupKey by doing const {groupKey} = useContext(FieldGroupCtx);

Copy link
Copy Markdown
Contributor

@robyww robyww Apr 2, 2026

Choose a reason for hiding this comment

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

I would prefer to stay consistent with useFieldGroupValue. Maybe we should make is support a default

Copy link
Copy Markdown
Contributor

@robyww robyww Apr 2, 2026

Choose a reason for hiding this comment

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

You could defined this in SimpleComponent. I think it would work. There are many uses.

export function useFieldValueOnly(fieldKey, def, gk) {
    return useFieldGroupValue(fieldKey,gk)[0]() ?? def;
}

const spatialMethod = useFieldGroupValue(SpatialMethod)[0]() ?? CONE_CHOICE_KEY;
const closest = useFieldGroupValue(Closest)[0]() ?? '';
const cornerCalcTypeValue = useFieldGroupValue(cornerCalcType)[0]() ?? 'image';
const spatialRegOpValue = useFieldGroupValue(SpatialRegOp)[0]() ?? SpatialRegOpType.CONTAINS_POINT;
Comment on lines +353 to +357
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. I think I went down the wrong path using getVal here.

const layoutMode= getSpacialLayoutMode(spacialType,obsCoreEnabled,capabilities?.canUpload);
const isCone= spatialMethod === CONE_CHOICE_KEY;
const containsPoint= spatialRegOpValue === SpatialRegOpType.CONTAINS_POINT;
Expand Down
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