Reimplement GWT assay plate designer in React

[labkey-jeckels]

Rationale

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

Related Pull Requests

• https://github.com/LabKey/limsModules/pull/2155

Changes

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

Tasks 📍

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

[labkey-adam]

👍

[labkey-alan]

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]

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

[labkey-jeckels]

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

[labkey-tchad]

Manual testing looks mostly good. Just some problems with the buttons at the top.

• The "Save & Close" and "Cancel" buttons trigger a dirty page alert if there are changes.
• The "Save & Close" button is enabled when there aren't any changes
• None of the buttons match existing LabKey button styling

[labkey-tchad]

I just noticed a functionality downgrade in the new component.
There isn't any visual connection between the plate grid and the list of well groups other than their color. This is passable for a small number of groups on a smaller plate but it is pretty unintelligible on a large plate with a lot of groups (such as the non-empty 384 well NAb templates).

In the GWT designer, we display the well and group names next to the cancel button when you mouse over a well:

The GWT designer also highlights corresponding plate grid gutters when you mouse over a group:

The new designer already applies a class (template-grid__cell--active) to any wells associated with the currently selected group so it would just require us to assign some styling to that class for the plate grid to highlight the currently selected group.

[labkey-jeckels]

Manual testing looks mostly good. Just some problems with the buttons at the top.

• The "Save & Close" and "Cancel" buttons trigger a dirty page alert if there are changes.
• The "Save & Close" button is enabled when there aren't any changes
• None of the buttons match existing LabKey button styling

Save & Close has typically been enabled even when the page isn't dirty. That's retained from GWT.

Other button issues should be fixed - I matched the normal SDMS styling.

[labkey-jeckels]

I just noticed a functionality downgrade in the new component. There isn't any visual connection between the plate grid and the list of well groups other than their color. This is passable for a small number of groups on a smaller plate but it is pretty unintelligible on a large plate with a lot of groups (such as the non-empty 384 well NAb templates).

In the GWT designer, we display the well and group names next to the cancel button when you mouse over a well:

The GWT designer also highlights corresponding plate grid gutters when you mouse over a group:

The new designer already applies a class (template-grid__cell--active) to any wells associated with the currently selected group so it would just require us to assign some styling to that class for the plate grid to highlight the currently selected group.

I didn't bother with the gutter highlighting, but there's now feedback between the selected group and its wells and vice versa.

[labkey-tchad]

The button behavior doesn't seem to have changed at all. The "Save & Close" and "Cancel" buttons still alert you about unsaved changes.
Whether or not a "Cancel" button should alert you about unsaved changes is arguable, I suppose, but "Save & Close" definitely shouldn't.

[labkey-jeckels]

The button behavior doesn't seem to have changed at all. The "Save & Close" and "Cancel" buttons still alert you about unsaved changes. Whether or not a "Cancel" button should alert you about unsaved changes is arguable, I suppose, but "Save & Close" definitely shouldn't.

Strange, I don't know why I couldn't repro the Save & Close before. I removed the prompt from both buttons. The GWT designer confirmed on Cancel from a dirty page, but from spot checking our other React UIs we aren't doing it elsewhere.

[labkey-alan]

I've left some feedback. Overall it looks pretty good, but there are several changes that I think we should make.

One general piece of feedback is that function components should be typed as FC, and they should all have their displayName set. Most (all?) components are defined as function MyComponent(props: MyPropsType): JSX.Element, which is not a pattern we use at LabKey, and I think is worse than the const MyComponent: FC<MyPropsType> = () => {} pattern.

Another piece of feedback is that there is very little consistency in how we handle Events. Sometimes useCallback is used, but even then sometimes it's used in a way that doesn't matter (it's wrapped in a function that is regenerated on every render). Other times we create named functions for event handlers, but they're not memoized with useCallback. Often we just have inline event handlers. While techincally speaking you don't need to memoize callbacks handed to native DOM elements, we tend to do that at LabKey. The consistency is nice, but also because refactors or new features happen reasonably often it tends to be safer to memoize it today, because tomorrow's usage may benefit from it. There's very little overhead to useCallback, and we've seen real performance issues from omitting it.

[labkey-alan]

The TabButton component here will re-render on every render cycle because we are always passing a fresh onClick function to it. Instead, you should pass onTabChange and type as props, then in TabButton you should define onClick as const onClick = useCallback(() => onTabChange(type), [onTabChange, type]). Then, React can skip rendering these TabButton components unless onTabChange or type changes, which is unlikely.

[labkey-jeckels]

Extracted.

[labkey-alan]

This component is incredibly confusing because it defines quite a few handlers above via useCallback, which is what I would recommend, but then in the TSX here and below we define a lot of inline event handlers. Generally speaking I recommend defining the event handlers above, with named functions. This accomplishes two things: There's less stuff in your TSX, and your functions are named, which can help increase readability.

[labkey-jeckels]

Extracted.

[labkey-alan]

This whole React.Fragment smells like a component to me.

[labkey-jeckels]

Extracted but it doesn't feel great due to the large number of properties. I did some reduction but open to further ideas.

[labkey-alan]

Suggested change

	export function WarningPanel({ warnings }: WarningPanelProps): JSX.Element {
	return (
	<div className="warning-panel">
	{warnings.length === 0 ? (
	<div className="warning-panel__none">No warnings.</div>
	) : (
	<ul className="warning-panel__list">
	{warnings.map((w) => (
	<li key={w} className="warning-panel__item">{w}</li>
	))}
	</ul>
	)}
	</div>
	);
	}
	export const WarningPanel: FC<WarningPanelProps> = ({ warnings }) => (
	<div className="warning-panel">
	{warnings.length === 0 ? (
	<div className="warning-panel__none">No warnings.</div>
	) : (
	<ul className="warning-panel__list">
	{warnings.map((w) => (
	<li key={w} className="warning-panel__item">{w}</li>
	))}
	</ul>
	)}
	</div>
	);
	WarningPanel.displayName = 'WarningPanel';

[labkey-alan]

This comment is unnecessary.

[labkey-jeckels]

FWIW, I asked Claude to generate these and other comments to help me understand the structure. I'm OK to delete or simplify them if they're not useful to others. If you have a pointer to an example that's been useful in other code, I'd be happy to adapt to it.

[labkey-alan]

This formatting is incredibly weird. I will push a change that adds our eslint-config as a dependency. You'll want to run the npm run lint-fix command after I push, and commit the changes.