FIREFLY-1954: React Split Pane version upgrade by kpuriIpac · Pull Request #1977 · Caltech-IPAC/firefly

kpuriIpac · 2026-07-06T17:56:05Z

Ticket: FIREFLY-1954
IFE PR: https://github.com/IPAC-SW/irsa-ife/pull/472

This PR upgrades Firefly from react-split-pane v0.1.92 to v3.2.0
- Using the migration doc and readme for reference
The core changes are in DockLayoutPanel. v0 allowed sizing props to be applied at the parent SplitPane level, but v3 moves that to the individual Pane components.
- collapsible split panel logic is also slightly altered now to handle v3's pane sizing requirement.
Changed several consumers of SplitPane (using react-split-pane directly) to use our SplitPanel wrapper instead
AdvancedADQL and TabPanel include timing fixes for react-split-pane's v3 onwards ResizeObserver-based layout
- In AdvancedADQL in particular, earlier the code assumed DOM was present when the useEffect for Prism.highlighAll ran), but with v3.2.0 of react-split-pane using ResizeObserver for layout measurements, I noticed the Pane content can become available after the useEffect where we try to initialize Prism
  - This led to crashes and/or the SQL examples not being highlighted
  - So the new retry logic aims to fix this race issue. Could possibly be polished further by extracting the logic into a small separate hook. Open to other feedback as well.
The dockerfile change to Node 20 from Node 18 was needed because react-split-pane@3.2.0 declares node >= 20 and my Jenkins builds were failing without this change. I will talk to @loitly about this before deciding if we need to keep this.

Testing:
Firefly, Euclid, Spherex, Timeseries

Confirm visually in TAP, Advanced ADQL (Edit ADQL), tri view and bi view results, collapsible panels (spherex spectrophotometry) and in Timeseries that the results look good.
- Split Pane is used in several places, like the TAP screen as well. Test visually and compare against dev/ops if needed. Move around the panes to make sure there's no bugs there.
Dev links, to compare the test builds against: Firefly Dev, Euclid Dev, Spherex Dev, Timeseries Dev

robyww

This looks good. It works in my testing. There are some style things that I don't really understand that I assume is because of the component.

my big question is: why vertical to horizontal ?

robyww · 2026-07-09T16:55:22Z

            <Stack id='chart-pinned-sideBySide' overflow='hidden' flexGrow={1}>
                <Stack flexGrow={1} position='relative'>
-                    <SplitPanel split='vertical' defaultSize={400} style={{display: 'inline-flex'}} pKey='chart-sideBySide'>
+                    <SplitPanel direction='horizontal' defaultSize={400} style={{display: 'inline-flex'}} pKey='chart-sideBySide'>


This strikes me as unexpected. did the layout change?

See below, layout remains the same. Just the meaning of vertical/horizontal is swapped.

robyww · 2026-07-09T21:18:49Z


    return (
-        <SplitPane split='vertical' defaultSize={200} style={{display: 'inline-flex', ...style}}>
+        <SplitPanel direction='horizontal' defaultSize={200} style={{display: 'inline-flex', ...style}} pKey='filter-editor'>


again, when did vertical become horizontal

Yes so this is a big (but easy) change from v0.x to v3.x, they swapped vertical and horizontal to match CSS flex direction. Hence all of these changes.

robyww · 2026-07-09T21:38:26Z

                }
                setVal(adqlKey, `SELECT TOP 1000 * FROM ${maybeQuote(insertTname,true)}`);
-                window.setTimeout( () => prismLiveRef.current.syncStyles?.(), 10);
+                window.setTimeout( () => prismLiveRef.current?.syncStyles?.(), 10);


You should define a function at the top of the component syncStyles that is

const syncStyles= window.setTimeout( () => prismLiveRef.current?.syncStyles?.(), 10);

loitly

I’ve tested it, and it worked fine. However, I have a suggestion that you should consider.

loitly · 2026-07-10T17:44:24Z

+                        <Pane
+                            key={idx}
+                            {...paneProps[idx]}
+                            style={paneStyle}
+                        >
+                            {child}
+                        </Pane>


I would expect Pane to be the wrapper used in SplitContent and not injected here. This container should translate the out-dated 'maxSize/minSize' props into minSize defaultSize, than pass-along to the children, which should be Pane since we use SplitContent in all of our split panes.

+1. This way you won't need to define and maintain paneProps=[{}, {}] dedicatedly here and pass-through props will allow consumers of SplitContent to leverage new/better API of Pane if they decide to.

jaladh-singhal

@kpuriIpac this is thorough! I tested Firefly and spherex primarily and they are working great (and better than before esp since there's no flicker on dragging the split pane resizer).

One minor issue in collapsible panel that appears in SP results: when you collapse it, it's still 2-4px in height as opposed to 0px height on dev. But it's not a dealbreaker since collapsible panel needs work in future anyways to make it handle both vertical and horizontal cases.

jaladh-singhal · 2026-07-10T23:39:42Z

    "react-router-dom": "^6.14.0",
    "react-slick": "~0.31",
-    "react-split-pane": "~0.1",
+    "react-split-pane": "^3.2.0",


We generally pin the minor version too since some packages introduce breaking change in minor version too (not sure about react-split-pane, looks stable enough) and hence use ~ instead of ^. We had a long discussion over this last time I had to update package.json 😅 and I added that to our dev documentation.

jaladh-singhal · 2026-07-10T23:45:22Z

-        const textArea = adqlEl.current?.querySelector('textarea');
-        // adopt textArea
-        prismLiveRef.current = new Prism.Live(textArea);
+        let cancelled = false;


Might be worth abstracting this out as a hook since above useEffect follows same pattern and it obscures the two-line business logic.

jaladh-singhal · 2026-07-10T23:59:39Z

+                        <Pane
+                            key={idx}
+                            {...paneProps[idx]}
+                            style={paneStyle}
+                        >
+                            {child}
+                        </Pane>


+1. This way you won't need to define and maintain paneProps=[{}, {}] dedicatedly here and pass-through props will allow consumers of SplitContent to leverage new/better API of Pane if they decide to.

jaladh-singhal · 2026-07-11T00:03:48Z

 function CollapsibleSplitContent({sx={}, panelTitle, children, ...props}) {
    const {isOpen, onToggle} = useContext(SplitContext);
-
-    // TODO: dynamically calculate styles based on the position of toggle button and split direction (passed as props)


This is an important TODO that I'd advise against removing since this component isn't mature enough to handle both vertical and horizontal splits. If you want to remove it, we should at least add this note to docstring of this component and mark it @experimental

jaladh-singhal · 2026-07-11T00:05:06Z

-            onDragStarted: () => setIsSplitterDragging(true),
-            onDragFinished: (currentSize) => {
-                setIsSplitterDragging(false);
-                setIsCollapsibleOpen(currentSize > collapsedSize); // update the open state after dragging


again these code comments (and below) were helpful to understand the complex layout manipulation code.

kpuriIpac added 6 commits July 2, 2026 16:25

buggy first pass

0d0f3e2

more changes to support v3

f4fea1e

more fine tuning and fixed

d6d6552

fix bug and bloated lock file

6a54f2c

try a dockerfile node version fix

aaaf13f

cleanup

616c440

kpuriIpac added this to the 2026.2 milestone Jul 6, 2026

kpuriIpac self-assigned this Jul 6, 2026

kpuriIpac requested review from jaladh-singhal, loitly and robyww July 6, 2026 17:56

robyww approved these changes Jul 9, 2026

View reviewed changes

loitly approved these changes Jul 10, 2026

View reviewed changes

jaladh-singhal reviewed Jul 11, 2026

View reviewed changes

Uh oh!

Conversation

kpuriIpac commented Jul 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

robyww left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

loitly left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jaladh-singhal left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jaladh-singhal Jul 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

kpuriIpac commented Jul 6, 2026 •

edited

Loading

jaladh-singhal Jul 11, 2026 •

edited

Loading