Skip to content

FIREFLY-1935: Alert Viewer#1917

Open
kpuriIpac wants to merge 11 commits intodevfrom
FIREFLY-1935-alert-viewer
Open

FIREFLY-1935: Alert Viewer#1917
kpuriIpac wants to merge 11 commits intodevfrom
FIREFLY-1935-alert-viewer

Conversation

@kpuriIpac
Copy link
Contributor

@kpuriIpac kpuriIpac commented Mar 3, 2026

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

  • Implement Alert Viewer template
  • Improved server side logic by adding a search processor based on feedback
  • The processor retrieves & downloads the file, and then uses the existing AlertAnalyzer, then returns the parts (2 tables and 3 images) back to client as JSON
    • each JSON entry also includes the file location, in case we support multiple (more than one) files later
  • This in turn simplifies the client-side code inside AlertUploadPanel
  • Using the same viewerId and only one call to MultiImageViewer for images now
  • ToDo: some basic cleanup left to do including fixing heights of the 2 tables
  • Note: The search processor currently only supports URLs, but I have some commented out code (marked with a todo) ready to accept Alert IDs as well. This should be pretty easy to enable once we have working examples of Alert IDs (and the service to call with those IDs).

Test build: Alert Viewer: https://fireflydev.ipac.caltech.edu/firefly-1935-alert-viewer/firefly/alertviewer

@kpuriIpac kpuriIpac self-assigned this Mar 3, 2026
@kpuriIpac kpuriIpac force-pushed the FIREFLY-1935-alert-viewer branch 6 times, most recently from 41c2af3 to cf5a5a3 Compare March 11, 2026 22:27
@kpuriIpac kpuriIpac requested a review from loitly March 11, 2026 22:30
Copy link
Contributor

@loitly loitly left a comment

Choose a reason for hiding this comment

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

The UI still needs some polishing, which is expected. Moving the logic to the server side is a good improvement. I think we should talk offline to fully flesh out what the new interface should look like.
@robyww should review the multiProduct changes.

<title>Alert Viewer</title>

<script>
var pOpts = window.firefly && window.firefly.options || {};
Copy link
Contributor

Choose a reason for hiding this comment

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

Not used.

}}>

{/* Top: tables */}
<Box sx={{display: 'flex', flex: '1 1 0%', gap: 1, p: 1, overflow: 'hidden', minHeight: 0, minWidth: 0,
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is Box with display=flex used throughout? Would it be easier to use Stack?

isFromURL={true}
canDragDrop={false}
fileAnalysis={onLoading}
uploadParams={{analyzerId: 'alert'}} //triggers server alert analyzer
Copy link
Contributor

Choose a reason for hiding this comment

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

Not exactly what I had in mind, but I could be wrong. Let's talk offline.

onChange: (ev) => onUrlChange(ev, viewProps, fireValueChange),
value: viewProps.displayValue,
onUrlAnalysis: (value) => doUrlAnalysis(value, fireValueChange, fileType, viewProps.fileAnalysis)
onUrlAnalysis: (value) => doUrlAnalysis(value, fireValueChange, fileType, viewProps.fileAnalysis, viewProps.uploadParams)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it could be done without having to change FileUpload.

@kpuriIpac kpuriIpac force-pushed the FIREFLY-1935-alert-viewer branch 3 times, most recently from ddc105d to 97be20a Compare March 19, 2026 20:32
@kpuriIpac kpuriIpac marked this pull request as ready for review March 19, 2026 20:33
@kpuriIpac kpuriIpac requested a review from robyww March 19, 2026 20:55
@robyww
Copy link
Contributor

robyww commented Mar 20, 2026

@kpuriIpac I brought up the test. The alert tab does not appear to be enabled.

@kpuriIpac
Copy link
Contributor Author

Copy link
Contributor

@robyww robyww left a comment

Choose a reason for hiding this comment

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

(Mostly UI feedback with a couple of code comments. I will look at the code more on Monday.)

UI Feedback:

  • I think you should use the DockLayoutPanel for the three views. I think it is pretty each to use since it does layout this way by default. See line 67 of ResultsPanel.jsx
  • pad the right side table down so it better aligns with the left side table
  • remove Scroll Images: Create a prompt on MultiViewStandardToolbar to disable this features. The toolbar get all the props passed to MultiImageViewer.

Comment on lines +51 to +52
? {flex: '1 1 0%', minHeight: 0, height: '100%', maxWidth: '100%', overflow: 'hidden'}
: {width: '100%', height: '100%'};
Copy link
Contributor

Choose a reason for hiding this comment

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

I am surprise this change was necessary.

)}
</Box>

<Box sx={{flex: '1 1 0%', overflow: 'hidden', minHeight: 0, minWidth: 0}}>
Copy link
Contributor

Choose a reason for hiding this comment

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

also I wee you adding minHeight:0, minWidth: 0. I don't see why this is necessary.


return (
<Stack width={1} alignItems='center'>
<Stack ml={0} mt={4} spacing={3} sx={{position: 'relative'}}>
Copy link
Contributor

Choose a reason for hiding this comment

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

why ml=0?
also since youu are declaring a sx you should put the style in there.

viewerId={ALERT.IMG_VIEWER}
insideFlex={true}
forceRowSize={1}
Toolbar={MultiViewStandardToolbar}
Copy link
Contributor

Choose a reason for hiding this comment

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

add the prop for MultiViewStandardToolbar here

@kpuriIpac kpuriIpac force-pushed the FIREFLY-1935-alert-viewer branch 2 times, most recently from a841da8 to 66af329 Compare March 24, 2026 00:40
@kpuriIpac kpuriIpac force-pushed the FIREFLY-1935-alert-viewer branch 3 times, most recently from 3aa49cd to 518377d Compare March 24, 2026 00:43
@kpuriIpac kpuriIpac force-pushed the FIREFLY-1935-alert-viewer branch from 518377d to 0a657fa Compare March 24, 2026 01:06
@kpuriIpac
Copy link
Contributor Author

@robyww I implemented the feedback, using DockLayoutPanel now, and cleaned up the styling a lot (unnecessary use of Box, some minHeight / minWidth - lot of it left over from my initial work & testing to make the layout work, but not really required).

One UI bug that I tried to fix today but couldn't: When you expand images (full screen) and Close the expanded mode, the images don't render (but you still see the images toolbar). If you interact with either the toolbar or either one of the tables, the images show up again. Trying to put in a fix for this soon. But otherwise the code is ready for review. Test build updated.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants