Arvados

Plan is to do a little initial development, then propose one alternative library, and get agreement with Stephen before continuing with the bulk of development.

If it makes development substantially easier, I am open to UI changes as part of this branch—especially minor changes like spacing or button dimensions—but they should get interface-reviewed as normal.

As this ticket is huge, review will be done in stages. Note that intermediate stages may not pass tests.

22381-redux-form-refactor @ 0596ca8629dc153807aaba369192195d7af82e70

developer-run-tests: #5010

Overall looks good! Here's a few suggestions:

• In multiselect-actions, if you filter out undefined results in getResourcesFromCheckedList like so:

.filter((resource): resource is GroupContentsResource => !!resource)

You can avoid having to deal with undefined everywhere that uses it, you can get rid of .filter(res => !!res) in openCollectionCopy and you shouldn't need the ! in the .map

• In collection-copy-actions.ts I noticed that copyCollection catches some errors to show a toast but passes through others to be handled by the caller, I think it would be better to consolidate that by removing the catch block from copyCollection so that all errors end up in the copySingleCollection catch block to handle the UNIQUE_NAME_VIOLATION as well as unknown errors. I have a slight preference for moving the error handling up so that copyCollection is more reusable. This can apply to any other area where the error handling got split up
• In workbench actions copyCollectionRunner and moveCollectionRunner, it would be ideal to group the result toasts - we can probably make a separate ticket but if you're up for it, I previously made a helper to show grouped result toasts showGroupedCommonResourceResultSnackbars used in toggleResourceTrashed - "all" you have to do is map each item to a promise that returns the result of the service action (to resolve the promise to the resource) and hand the helper the settled promises and a map of resource errors to functions that generates the appropriate message. You can .then. off of that and do anything else needed with the results or updated resources.
• isfileOperationLocation It might be good to also validate uuid.length and subpath.length just in case it's possible to get an object with empty string fields somehow
• dialog-collection-partial-copy-to-existing-collection.tsx I'm wondering about the currentUuids and if there's any reason to pass an array containing an empty string vs just an empty array when destination.uuid isn't set

22381-redux-form-refactor @ 1de181bebba16926d98808cfbc19e493b8e6cdcb

Intermediate review of in a not-completely-working state. The project and user profile tests don't pass, and I only have two of the suggestions from the previous review implemented.

Overall looking good, here's a couple things I saw that I think could be improved:

• In resource-properties-form, it seems like an anti-pattern to send signals between neighboring components using state setters, I have an alternate suggestion:
  • The purpose seems to be to allow the Key field when selected and the Add button when pressed to clear the value field
  • An annoying factor is that the value field and the form have their own copies of the value, but this makes sense to encapsulate the validation
  • There are 2 cases where the signal is triggered
    • Key field onSelect
    • Add button submission
    • In both, we are setting the parent field component's value to undefined (at least in the Add button handler it does. When the key field is onSelected, the value field clears itself but doesn't send the updated value to the parent). If both send undefined to the parent, we could have the value field react to the parent field being set to undefined and not need the extra signal.
  • You would just need to create a handleSetKey(key) that sets the key as well as setCurrentValue to undefined like handleAddProperty and change the onFocus handler inside the Key component to call onSelect with undefined to update the parent component instead of sending a signal.
  • Then, instead of passing the signal to the value component, pass the currentValue and check for it being undefined in the useEffect.
• When it comes to using isSubmitting to keep track of submitting state, it seems complicated to use useEffect to detect when submission is done by examining the dialog close state and form error state.
  • I would suggest something more watertight, like chaining .finally() off of the updateFn / moveProjects / whatever dispatchable action is being done. The caveat is that Dispatch apparently doesn't know about async thunks, so you'll need to change the prop type of those mapDispatch to Promise<void> or whatver it's supposed to return in order for the types to understand what's going on - this is kind of a cast since dispatch returns any but at least the prop type for the mapped dispatch will be setting the correct Promise return type.
• The CircularSuspense component seems a bit overengineered for solving the problem of showing a loading indicator, but I'm less inclined to complain since it's self contained. It's unfortunate that only later MUI versions have the loading prop on buttons, which I think shows a loading indicator on a disabled button like redux forms does. I would have gone for something simpler that perhaps uses visibility:hidden and absolute positioning on the indicator to avoid layout shift instead of using a useLayoutEffect and js calculated sizing. Also that file uses tabs for some reason.

22381-redux-form-refactor @ ef194bd6aa0730e2cfe323b6c06897950e2b4dea

developer-run-tests: #5055

• In collection-copy-actions.ts I noticed that copyCollection catches some errors to show a toast...

Fixed

• In workbench actions copyCollectionRunner and moveCollectionRunner, it would be ideal to group the result toasts - we can probably make a separate ticket but if you're up for it.

This is enough work and isolated enough that I believe it should be on its own ticket.

• isfileOperationLocation It might be good to also validate uuid.length and subpath.length...

done

• dialog-collection-partial-copy-to-existing-collection.tsx I'm wondering about the currentUuids...

You're right, fixed

• In resource-properties-form, it seems like an anti-pattern to send signals between neighboring components using state setters...

When I write that I had Go channels in mind, wishing javascript could be more like Go. That said, injecting a pattern from another language just because I think it's cool is not good coding practice, so I've removed in favor of the more traditional approach you recommended.

• When it comes to using isSubmitting to keep track of submitting state, it seems complicated to use useEffect to detect when submission is done...

I'm going to push back here because the proposed solution seems more complicated than the existing solution. It feels simpler to me to have a simple if/then (in the useEffect), and by handling it within the component, managing the disabled/spinner state on the form submit button gets very easy.

• The CircularSuspense component seems a bit overengineered for solving the problem of showing a loading indicator, but I'm less inclined to complain since it's self contained...

Part of the motivation for this refactor was to clear a blocker on #22379, for which the proposed solution includes upgrading MUI to take advantage of the virtualized list support in later MUI versions. By isolating and generalizing CircularSuspense, it will be trivial to drop in a replacement once that MUI upgrade happens.