Skip to content

Reimplement GWT assay plate designer in React#7623

Open
labkey-jeckels wants to merge 11 commits intodevelopfrom
fb_reactPlateDesigner
Open

Reimplement GWT assay plate designer in React#7623
labkey-jeckels wants to merge 11 commits intodevelopfrom
fb_reactPlateDesigner

Conversation

@labkey-jeckels
Copy link
Copy Markdown
Contributor

@labkey-jeckels labkey-jeckels commented Apr 27, 2026

Rationale

Our assay plate designer is our final GWT UI. This replaces it with a React-based version.

Changes

  • New React app that matches the GWT version 1:1
  • Improve generics in PlateManager and related callers

Tasks 📍

  • Code review @labkey-nicka
  • Manual Testing @labkey-tchad
  • Update existing Selenium tests (NAb and Elispot)
  • Add unit tests
  • Add endpoint tests

@labkey-adam
Copy link
Copy Markdown
Contributor

👍

Copy link
Copy Markdown
Contributor

@labkey-alan labkey-alan left a comment

Choose a reason for hiding this comment

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

I did not do the most thorough review on the React code, since I was only asked to take a cursory look at the code, and the GitHub outage is preventing me from viewing all of the files at this time. I do have two pieces of feedback though:

  1. Many of the components have really large chunks of TSX code (e.g. GroupTypesPanel). Lots of nested loops and conditional statements. This is a readability mess, and it also makes it significantly harder to test, since we can't test in smaller units of functionality. My recommendation is to split these large chunks of TSX into smaller components.
  2. There are zero unit tests. For a set of components this size, and this complex, we really should have as much unit test coverage as is reasonable.

Additionally I do have a concern about introducing this code more generally. While I understand the motivation to get rid of the last GWT component, and I would love to see us accomplish that goal, I am not sure that this is the best path forward. We already have a lot of plate related code in our apps, and while this code helps us get rid of GWT, it does not change the fact that we have two different implementations of a UI that is meant to accomplish the same thing. I think we would benefit most from having one UI for our plate code, not two different implementations that are both made with React.

I can take a more thorough look at the code later if wanted.

@labkey-jeckels
Copy link
Copy Markdown
Contributor Author

---------------------------------------------|---------|----------|---------|---------|------------------------------------------------------------------------------------------------
File                                         | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s                                                                              
---------------------------------------------|---------|----------|---------|---------|------------------------------------------------------------------------------------------------
All files                                    |   73.37 |    74.48 |   68.29 |   76.86 |                                                                                                
 src/client/PlateTemplateDesigner            |   47.63 |    39.39 |   29.57 |   53.49 |                                                                                                
  PlateTemplateDesigner.tsx                  |   43.22 |    33.33 |   28.57 |   48.86 | ...283,287-289,293-301,305-319,324,328-329,339-346,352-365,370-377,382,386-387,393-395,416-418 
  models.ts                                  |     100 |      100 |     100 |     100 |                                                                                                
 src/client/PlateTemplateDesigner/components |   98.35 |    96.65 |   97.84 |    99.2 |                                                                                                
  GroupTypesPanel.tsx                        |   96.62 |    95.18 |   94.28 |   97.29 | 280,329                                                                                        
  MultiCreateDialog.tsx                      |   98.36 |    94.11 |     100 |     100 | 24,35                                                                                          
  RightPanel.tsx                             |     100 |      100 |     100 |     100 |                                                                                                
  ShiftPanel.tsx                             |     100 |      100 |     100 |     100 |                                                                                                
  StatusBar.tsx                              |     100 |      100 |     100 |     100 |                                                                                                
  TemplateGrid.tsx                           |     100 |      100 |     100 |     100 |                                                                                                
  WarningPanel.tsx                           |     100 |      100 |     100 |     100 |                                                                                                
  WellGroupProperties.tsx                    |   95.83 |    92.85 |     100 |     100 | 46                                                                                             
 test/js                                     |     100 |      100 |     100 |     100 |                                                                                                
  fileMock.js                                |     100 |      100 |     100 |     100 |                                                                                                
---------------------------------------------|---------|----------|---------|---------|------------------------------------------------------------------------------------------------


@RequiresAnyOf({InsertPermission.class, DesignAssayPermission.class})
public class DesignerAction extends SimpleViewAction<DesignerForm>
public class GetTemplateDefinitionAction extends ReadOnlyApiAction<DesignerForm>
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Consider adding endpoint tests in PlateController.ispec.ts for both plate-getTemplateDefinition.api and plate-saveTemplate.api introduced in this PR.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

private static final int MAX_BODY_BYTES = 10 * 1024 * 1024; // 10 MB

@Override
protected BaseApiAction.FormAndErrors<SaveTemplateForm> populateJacksonForm() throws Exception
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Missing @NotNull annotation

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I found a better way to accomplish this. Separate Platform PR coming soon.

}

@RequiresAnyOf({InsertPermission.class, DesignAssayPermission.class})
public static class SaveTemplateAction extends MutatingApiAction<SaveTemplateForm>
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why is populateJacksonForm override necessary here? Just to limit to 10MB? Could you annotate with @Marshal(Marshaller.JSONObject) to achieve this instead?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yes, just to provide a limit and protect against DOS. We should support this more centrally. Platform PR coming soon.

}

@RequiresAnyOf({InsertPermission.class, DesignAssayPermission.class})
public static class SaveTemplateAction extends MutatingApiAction<SaveTemplateForm>
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

It would be nice to distinguish these endpoints somehow from our preferred endpoints so future engineers/users don't get them mixed up. Consider naming these GetDesignerTemplateDefinitionAction and SaveDesignerTemplateAction.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Updated to getDesignerTemplate and saveDesignerTemplate. Also refactored to share more with createPlate

@labkey-jeckels
Copy link
Copy Markdown
Contributor Author

I did not do the most thorough review on the React code, since I was only asked to take a cursory look at the code, and the GitHub outage is preventing me from viewing all of the files at this time. I do have two pieces of feedback though:

  1. Many of the components have really large chunks of TSX code (e.g. GroupTypesPanel). Lots of nested loops and conditional statements. This is a readability mess, and it also makes it significantly harder to test, since we can't test in smaller units of functionality. My recommendation is to split these large chunks of TSX into smaller components.
  2. There are zero unit tests. For a set of components this size, and this complex, we really should have as much unit test coverage as is reasonable.

Additionally I do have a concern about introducing this code more generally. While I understand the motivation to get rid of the last GWT component, and I would love to see us accomplish that goal, I am not sure that this is the best path forward. We already have a lot of plate related code in our apps, and while this code helps us get rid of GWT, it does not change the fact that we have two different implementations of a UI that is meant to accomplish the same thing. I think we would benefit most from having one UI for our plate code, not two different implementations that are both made with React.

I can take a more thorough look at the code later if wanted.

  1. Refactored. See what you think now.
  2. Significant unit testing added, along with endpoint testing (with help from Nick).

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants