React Animal History - Search By Id

[labkey-martyp]

Rationale

Implements the first phase of React conversion of Animal History. This is currently a hidden view until we get to MVP. Link in the ehrAdmin page. Participant history is available if you have the experimental feature on, you'll get a link at the top of the old participant view which comes up when you click on an Id.

Spec is in this PR

Changes

• Added single/multiple Id search
• Add All Animal search
• Add All Alive at Center search
• Add URL readOnly search for participant history
• Add URL binding
• Add new column for reports that are compatible with non-Id filters

[labkey-jeckels]

Are these requirements or non-requiremnents? I'm unclear on the break between them and the first seven.

[labkey-martyp]

requirements

[labkey-jeckels]

If it's worth capping the limit in "normal" usage (for performance, I guess?), it seems like we shouldn't exempt URL-provided values. But I'm also not really clear on why this is a useful mode.

[labkey-martyp]

This is the same UI as participant history which completely derives the filters from the URL. This is pretty much only used when clicking on links generated in the EHR, so we assume we don't need to validate the parameters. In the normal search we're just trying to scope the amount of Ids that can be filtered, but may need to revisit this as we get more testing done.

[labkey-jeckels]

Seems like a lot of separate hard-codings of these values.

[labkey-martyp]

These were moved to variables.

[labkey-jeckels]

What does this feature flag control? Being able to open the new and old versions without toggling an experimental flag seems like it would be more convenient. Or does the flag control which is the target URL by default for existing UI?

[labkey-martyp]

I updated the spec. This is going to just be hidden with link in admin page for now.

[labkey-danield]

Because IDs can contain special characters the test should try IDs with separator values (e.g. ID123,A).

[labkey-martyp]

added

[labkey-danield]

Are IDs stored as string values? Would another boundary case be an ID that is a very long string, or MAXINT + 1?

[labkey-martyp]

added

[labkey-danield]

It might be worthwhile to call out some explicit special characters to test. For example, any characters that could be interpreted as a comment(// ' #), string(" ') or something could have special meaning in a SQL query or a url (? @ :).
Also worthwhile to consider where in the ID the special character is. An ID that starts with @123, or '123 or an ID that looks like a link / email address ([email protected])

[labkey-martyp]

added

[labkey-danield]

Another boundary case could be 10 IDs that are 1K characters long (or some other ridiculous size) and bookmark that.

[labkey-martyp]

added

[labkey-danield]

I'm not opposed to using sleeps, if they work that's great. Sometimes waiting for an element to go stale, the button or some element to show up can make the test a little more reliable and have a quicker run time. Something like:
shortWait().until(ExpectedConditions.stalenessOf(button));

Not sure if that will be applicable, but just a thought.

[labkey-martyp]

I need to work through a few of these.

[labkey-tchad]

asserts are out of scope for page (and component) classes. Page classes should provide methods to inspect and control the page but test classes should assert functionality.
If these assertions would be useful elsewhere, you could put them into a separate helper class, maybe.

[labkey-martyp]

asserts are out of scope for page (and component) classes. Page classes should provide methods to inspect and control the page but test classes should assert functionality. If these assertions would be useful elsewhere, you could put them into a separate helper class, maybe.

Thanks @labkey-tchad . I believe all your suggestions are implemented now or the code was factored out. Let me know if you have any other feedback.

[labkey-tchad]

Looks like this change is causing some test failures. ONPRC tests are failing folder import.

VALIDATION ERROR: Query study.aliasIdMatches failed validation!
org.labkey.api.query.QueryNotFoundException: Error on line 8: Query or table not found: study.alias

[labkey-alan]

I've left a lot of feedback. But in general we need to ensure that:

1. We are properly memoizing things (especially functions used as callbacks)
2. Our tests are actually testing things. A lot of the tests in this PR are not actually testing anything. I reviewed most tests, but not all, I suspect the tests I did not review are suffering from the same problems as the ones I did review.
3. Interfaces should not be defined inline
4. Prop types should be correct. Props that are always passed should be considered required, even if sometimes they're undefined. If something is sometimes undefined, say that in the type definition. If something is always defined, then we shouldn't treat is as sometimes not being defined (there's a lot of code that looks like foo?.bar?.baz.()). Sometimes it's better to not render a component until all of the necessary props are defined, then you don't need any defensive code in the component, and there are less edge cases to handle.

We also need to rethink the component structure of the ReportTab and related components.

I also think that we shouldn't retain backwards compatibility with the hash URL, it adds a lot of code that we have to maintain, for very little benefit. We should use URL search params as they are intended to be used, not invent our own thing.

I also personally find this UI confusing. It doesn't make a lot of sense to have a text area with three buttons below it, but two of the buttons ignore the textarea. It seems to me like there should be something like a dropdown, or a radio group, where you select the filter mode. We should only render the textarea if the user selected "Search By Ids".

[labkey-alan]

I don't think there is any value in having a models folder with just an index.ts file with models defined. I would either:

1. Get granular and split these model definitions up as needed into separate files in this folder
2. Alternatively, get rid of the models directory, instead move this file up one directory and rename it to models.ts

[labkey-martyp]

Yeah since these are shared across files I just wanted them in a common place. I foresee more models being added in future stories and breaking up this file into multiple files. That's why I have it in a separate folder.

[labkey-alan]

These variables should be memoized, you're creating a new array object every render cycle when filter is called, even if the resolved variable hasn't changed.

[labkey-martyp]

I think memoizing this would actually raise the overhead. This is a pretty small calculation.

Also it's a pretty narrow use case (if at all) that this component would re-render with the same prop values (which would be the dependency on the useMemo). Most likely resolutionResult will have changed and these values will need to be recalculated any way.

[labkey-alan]

This component re-renders, and these values are re-computed, on every keystroke in the textarea, I have manually verified this. So it is actually called with the same value repeatedly, which is why I had suggested to use useMemo here. You could also use memo on the component itself, which should have the same effect as useMemo, since the incoming props would be compared against their previous values and only render when they change.

[labkey-martyp]

Made component a memo

[labkey-alan]

I'm not seeing that, you may need to push, but that is probably the best solution.

[labkey-martyp]

I've left a lot of feedback. But in general we need to ensure that:

1. We are properly memoizing things (especially functions used as callbacks)
2. Our tests are actually testing things. A lot of the tests in this PR are not actually testing anything. I reviewed most tests, but not all, I suspect the tests I did not review are suffering from the same problems as the ones I did review.
3. Interfaces should not be defined inline
4. Prop types should be correct. Props that are always passed should be considered required, even if sometimes they're undefined. If something is sometimes undefined, say that in the type definition. If something is always defined, then we shouldn't treat is as sometimes not being defined (there's a lot of code that looks like foo?.bar?.baz.()). Sometimes it's better to not render a component until all of the necessary props are defined, then you don't need any defensive code in the component, and there are less edge cases to handle.

We also need to rethink the component structure of the ReportTab and related components.

I also think that we shouldn't retain backwards compatibility with the hash URL, it adds a lot of code that we have to maintain, for very little benefit. We should use URL search params as they are intended to be used, not invent our own thing.

I also personally find this UI confusing. It doesn't make a lot of sense to have a text area with three buttons below it, but two of the buttons ignore the textarea. It seems to me like there should be something like a dropdown, or a radio group, where you select the filter mode. We should only render the textarea if the user selected "Search By Ids".

Thanks for the great review @labkey-alan. This is a large PR, I appreciate the time on it. Most of your feedback has been incorporated. I commented on anything that wasn't. For your specific points.

1. I think we should discuss this more, maybe in the frontend meeting. useMemo and useCallback both have their own complexity and performance implications, so using them where not needed can add unnecessary overhead. I commented on your comments below where I thought that would happen if we memoized. However, I understand if there are coding conventions we want to just always follow because the implications of not memoizing is greater.
2. I did do some refactoring in the jest tests. In many cases they are just testing very shallowly. Not going down the component tree at all. I updated some to test more thoroughly, but open to further discussion on that.
3. These should all be fixed and this is added to my code review file.
4. Good to know. These should all be fixed now and this is added to my code review file.

I commented on the structure of ReportTab and related components below. I think given the integration with JS and ExtJS outside the react lifecycle and the setup required to render the non-react reports, this is a clean approach. Some of the components are not typical react components, but it's not a typical react interface as it extends outside the normal lifecycle. The patterns used, render-props and headless components, are all established patterns in react.

On the URL I think you're right. We probably just need to break compatibility. That may be a story all on it's own to ensure we get it all correct and all links in EHR and otherwise updated.

Thanks for the feedback on the UI. I will look into making that more clear in the next story.

[labkey-alan]

I have a bunch of pending review comments, but I wanted to leave a comment at the top level here because it's less likely to get lost in the noise of everything else.

useMemo and useCallback both have their own complexity and performance implications, so using them where not needed can add unnecessary overhead. I commented on your comments below where I thought that would happen if we memoized.

Here is the math I am doing in my head to determine if we should memo/useMemo something:

1. Is the potential performance hit from the overhead of memo/useMemo small enough to be unnoticeable? In my experience (and I've been using React since the first public release), 100% of the time the answer is yes.
2. Is the potential performance gain big enough to be noticeable? Probably, we've had several performance regressions where just typing into a text box has noticeable delay.

Because the performance hit from the overhead is effectively never disruptive to us you are, generally speaking, better off just memoizing the thing. If you are in the habit of doing that, then the likelihood that you'll introduce a performance issue is low. It also saves you the cognitive burden of constantly having to think to yourself "is this case one that could get out of hand and cause issues down the road?".

You can absolutely write your code such that you only ever memoize stuff when you know for a fact that you'll get a benefit that outweighs the (most definitely negligible) overhead cost of memoizing, but in order to write that code you'll need to manually verify every component you write, and I don't really think that is worth the effort, when the cost is so low. You also need to ensure that when you're verifying your code you're doing it with realistic data, which is definitely easier said than done. An example of this: we had a performance issue in the Domain Designer, but it wasn't noticeable until you added something like 15-20 fields to a domain. Once you broke that threshold typing in any input on a Domain Field was noticeably slow. We didn't know about this issue until someone had a local domain with enough fields to reproduce the problem.

[labkey-martyp]

I have a bunch of pending review comments, but I wanted to leave a comment at the top level here because it's less likely to get lost in the noise of everything else.

useMemo and useCallback both have their own complexity and performance implications, so using them where not needed can add unnecessary overhead. I commented on your comments below where I thought that would happen if we memoized.

Here is the math I am doing in my head to determine if we should memo/useMemo something:

1. Is the potential performance hit from the overhead of memo/useMemo small enough to be unnoticeable? In my experience (and I've been using React since the first public release), 100% of the time the answer is yes.
2. Is the potential performance gain big enough to be noticeable? Probably, we've had several performance regressions where just typing into a text box has noticeable delay.

Because the performance hit from the overhead is effectively never disruptive to us you are, generally speaking, better off just memoizing the thing. If you are in the habit of doing that, then the likelihood that you'll introduce a performance issue is low. It also saves you the cognitive burden of constantly having to think to yourself "is this case one that could get out of hand and cause issues down the road?".

You can absolutely write your code such that you only ever memoize stuff when you know for a fact that you'll get a benefit that outweighs the (most definitely negligible) overhead cost of memoizing, but in order to write that code you'll need to manually verify every component you write, and I don't really think that is worth the effort, when the cost is so low. You also need to ensure that when you're verifying your code you're doing it with realistic data, which is definitely easier said than done. An example of this: we had a performance issue in the Domain Designer, but it wasn't noticeable until you added something like 15-20 fields to a domain. Once you broke that threshold typing in any input on a Domain Field was noticeably slow. We didn't know about this issue until someone had a local domain with enough fields to reproduce the problem.

Yeah I understand where you're coming from on that. I added it as a topic in our next front end meeting so we're consistent in that approach. It's a good time to formalize some of these standards across the team.

[labkey-alan]

The code looks better, especially the new useReportTab hook which I think accomplishes a clearer component hierarchy, and is more idiomatic. I left a bunch of specific feedback, but I have some general feedback as well:

• There are a ton of comments in the code. In general I am a fan of code comments, and think that LabKey as a whole (myself included) do a poor job of commenting our code. That said, I think you should consider taking a pass to look at all of the comments across the files in this PR and trim the very obvious ones, and especially trim the ones that say something like "extracted from ", I don't think those are useful at all. I commented on a few of these, but there are a lot more that I didn't comment on.
• There are still a lot of tests that setup what seems like valid scenarios, but then fail to assert anything useful. If we cannot properly assert something, then I struggle to believe the test is useful.

Edit: some of my comments are on outdated code because you pushed changes while I was reviewing, but GitHub then hid those comments from me when it came to review submission time, so I couldn't remove them. I suspect some of those comments are already completely irrelevant.

[labkey-alan]

This type isn't used, so you could remove it. You could also use it on ReportConfigBase by defining reportType: ReportType and the interfaces below it that extend ReportConfigBase would still be valid. I don't have a strong preference, other than unused stuff should be removed.

[labkey-alan]

Need to update these tests to no longer pass the flag now that it's been removed.

[labkey-alan]

Yes and the way to do that within a react app is generally to use a ref and check if ref.current is not undefined. I'm sure this works, it's just not what I would call idiomatic react code.

[labkey-alan]

Hard disagree that it is an established or clean pattern to create components that never render anything. I have never seen that before. I have asked other people, and they have never seen that before. Happy to discuss.

[labkey-alan]

Yes it is a style we have across many of our components, and the advantage is that you don't need to have code that destructures your props on multiple lines, you can condense it into a single line.

[labkey-alan]

I don't think this comment, or any of the ones like it (QueryReportWrapper, JSReportWrapper, probably elsewhere), are useful. It's just noise.

[labkey-alan]

In general I am in favor of comments like this that explain what a hook (or component) does. However, in this particular case I think it's not useful to provide Extracted from the former ReportTab render-prop component.. The rest of it seems appropriate.

[labkey-alan]

Most of the tests in this file (34/55) only rely on await waitForReportsToLoad();, despite all of them having test names implying they're testing something specific, and some even having additional comments implying even more specific behavior is being tested. These two tests in particular (props passed to TabbedReportPanel) have the exact same test body, if you ignore the comments, I don't think that's particularly useful. We should be asserting something different for each test. The passes correct reportNamespace prop test in particular says the thing being tested is verified indirectly by successful render, but surely we could actually assert that the TabbedReportPanel is visible right?

Another example is the test case for readOnly mode, it's not asserting anything different than most of the tests in this file, but if you look at the implementation of the component we could easily assert that the SearchByIdPanel should not be visible, since we hide that in readOnly mode.

The tests in this file (and everywhere else) need to actually assert something. I don't think it is a useful test if the only thing being asserted is that the text "General" has appeared on the page somewhere. The coverage numbers don't matter if you aren't asserting anything in your tests.

If you can't actually assert something in the body of a test, then the test is probably worthless; maybe unit tests aren't appropriate for the particular test case.

[labkey-alan]

This file has a lot of comments that feel overly verbose, but this one in particular I think is not helpful.

[labkey-alan]

Something to consider: these button styles don't match any other button styles within LabKey. I think they look good, but it's odd that we have completely different styles for these buttons. I would consider using the bootstrap styles that align with what you're doing e.g. btn btn-primary, btn btn-default, etc.