feat(credentials): nested match expressions table

[hareetd]

Fixes #466

Includes some minor fixes to the nested table implementation in the AllTargetsArchvedRecordingsTable.

[andrewazores]

Just looks like the test snapshot needs updating.

[hareetd]

Ah, I forgot to update/add tests, thanks.

[andrewazores]

Oh, I see this is still a draft. Carry on :-)

[andrewazores]

I notice that after first adding a new credential from directly within the Security view, the new row gets added to the table but the count badge is missing. If I navigate away to some other view and then back to Security then the badge appears with the expected number.

[andrewazores]

Also the badge doesn't seem to live-update with target discovery notifications while I'm on the Security view - at least not "LOST" notifications, which are all I tried.

[hareetd]

Sounds good, thanks for the feedback.

[hareetd]

I realized while writing tests that the notification handling for multiple deleted credentials in StoreJmxCredentials was broken. Specifically, the notifications would come through but not all of the corresponding credentials would be removed from the table. My first thought was that the rendering was having issues due to improper state handling.

Considering I have a lot of state objects (credentials, expandedCredentials, checkedCredentials, isHeaderChecked, and counts) that are related to each other and as a result, are frequently updated at the same time, I decided to convert their handling from useState to useReducer. Unfortunately, this didn't fix the original problem.

I then realized since the synchronous notification handling for deleted credentials is more involved than usual (relative to our other components) some of the asynchronous notifications from the Subscription were being missed and going unprocessed. So I decided to use concatMap in conjunction with of to ensure each notification is fully processed before proceeding to the next:

 React.useEffect(() => {
    addSubscription(
      context.notificationChannel.messages(NotificationCategory.CredentialsDeleted)
      .pipe(concatMap(v => of(dispatch({ type: Actions.HANDLE_CREDENTIALS_DELETED_NOTIFICATION, payload: { credential: v.message }}))))
      .subscribe(() => {} /* do nothing - dispatch will have already handled updating state */)
    );
  }, [addSubscription, context, context.notificationChannel]);

I wonder how many of our other components also have this same issue but we haven't realized since the type of notifications they handle don't typically get spammed when testing?

[hareetd]

Taking a look at the ArchivedRecordingsTable, it looks like the notification handling for deleted recordings is fairly involved but there is no such issue present there.

I wonder if my issue is due to the nature of setting up a nested table using useEffects with dependencies (credentials, counts, etc.) that are frequently updated at the same time (at least, at a more frequent rate than our more typical components) or whether my design approach is incorrect?

[andrewazores]

I'm not sure - this is a tough thing to track down. useReducer was a good idea to try to tackle the problem.

It might be as simple as a state hook not updating, though. IIRC the elements within the dependencies array are only compared by reference, not by value, and not by any deep comparison. So for example, if you have a piece of state that is actually an array or a Map etc. and you mutate that object, the reference remains the same and the hook will not update.

But your hunch that it could be timing related also seems completely possible.

[andrewazores]

Last commit looks like it solved it :-)

[hareetd]

Yeah, looks like it was a timing issue. I've modified the AllTargetsArchivedRecordingsTable to ensure synchronous target-related notification handling there as well, just in case.