FIREFLY-1943: React Compiler bugs

[kpuriIpac]

Ticket: https://jira.ipac.caltech.edu/browse/FIREFLY-1943

• Fixes bugs introduced in code after enabling react compiler. See ticket for list of bugs.

• Reason for most bugs: before react compiler, incidental re-renders would make some of the logic work, but with react compiler enabled, state renders became more obvious (and would happen only when necessary), often leading to stale values and exposing existing bugs (or rather bad code, because everything still worked fine).

• Bug 1: SIA Search:

  • The issue with constraints is probably related to makePanelStatusUpdater. The way we update constraints here is really complicated, and constraints were not updated properly after enabling react compiler and led to stale values.
    • I simplified it by reading the current field values that we calculate constraints from and depending on those changing to calculate/update constraintResult.
  • if this refactor looks good, I will address the remaining usage of makePanelStatusUpdater in the new compiler ticket (see note below for other bugs found for this ticket). It's used in ObjectIDSearch, ObsCore, and a few other components.
    • While there are no bugs there yet, it'll just make for better code to repeat the above there and not rely on the makePanelStatusUpdater flow.

• Bug 2: Spherex TAP Search:

  • This is also related to constraints. By the time we called getAdqlQuery, the spatial constraint fragment was missing (after enabling react compiler). The above fix (stopped relying on makePanelStatusUpdater) and directly computing and updated constraintResult from the values it depends on fixed this as well
    • (BTW, explicit use of useMemo helps make sure we use the previous constraintResult value of none of the spatial inputs it relies on change, kind of similar to makePanelStatusUpdater logic of checking if new/old values differ)

• Bug 3: fixed by Trey in a previous PR

Note: I found 2 more bugs: The TAP service dropdown and moving between Single and Multi-Object in Spatial Search. The fix for both is likely the same (in TapSearchRootPanel and TapUtil). I attempted fixing them in this PR, but the changes were a bit complex & out of scope for this PR. I will address those 2 in a new React Compiler ticket.

I will turn off React Compiler before merging.

Testing:
Firefly, Euclid and Spherex

• Test bugs found in ticket for Firefly and Spherex
  • TAP search, SIAv2 search, Spectral Image Search on Spherex
• For Euclid, test TAP to do a search and check ADQL in job history to ensure the search was correct
• I tested the apps with react compiler turned off as well, they work as expected with the changes above.

[robyww]

This is good work and good analysis. I like your big solution of useMemo I just want you to make it more reusable.

[robyww]

good! getAdqlConstraintsList is a real improvement.

[robyww]

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

[robyww]

good. I think I went down the wrong path using getVal here.

[robyww]

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)) ]);

[jaladh-singhal]

I vote for idea 2

[jaladh-singhal]

Code looks improved, left some minor comments.

Regarding testing, the spatial constraints are working now but other constraints like spectral constraints no longer get added to constraint fragment list.

1. SIA: make a search with location constraints, check job/table info -> job url, you will see POS param in URL query string. Now make another search with same location constraints and with some spectral coverage constraints, check job url, you will not see BAND param in URL query string. Do this same search again, BAND will now appear.
2. Spectral Image aka SPHEREx ObsTAP: same as above check wavelength params and inspect ADQL query in job info, the constraint on em_min, em_max columns won't appear in the WHERE clause. Same for time constraints

[jaladh-singhal]

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);

[robyww]

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

[robyww]

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;
}

[jaladh-singhal]

I vote for idea 2