[Task] ArchivedRecordingsTable does not properly update view state using notifications

[hareetd]

When a notification is issued, before the table state is updated we check whether the current target in context matches the one described in the notification (see here). However, unlike the active recordings notifications, the archived recordings notifications do not contain a target field for matching the connectUrl of the current target so the state is never updated.

Known cases:

1. Archived Recording Deleted notifications do not update the table state [1, 2]
2. Unable to differentiate between uploaded vs. saved archived recordings, resulting in bad table state in feat(archives): Redesign "All Archives" view #431 [1]

[andrewazores]

This only happens on the archived recordings table under the Recordings view. The Archives view for all targets behaves as expected.

[maxcao13]

When a notification is issued, before the table state is updated we check whether the current target in context matches the one described in the notification

I'm not quite familiar enough with this repo or React even but why do we check this in the ArchivedRecordingsTable and not the AllArchivedRecordingsTable?

[hareetd]

The ArchivedRecordingsTable is target-specific so when we receive a notification from the backend, such as "Archived Recording Deleted", we need to make sure that the target to which the deleted recording belongs to is the same target that's currently selected in the view (i.e. the ArchivedRecordingsTable instance we're looking at in the web client belongs to the same target from the notification).

The AllArchivedRecordingsTable, on the other hand, is not target-specific. It lists the archived recordings belonging to all targets, meaning, for example, when an "Archived Recording Deleted" notification is received, we can simply remove the recording from the AllArchivedRecordingsTable without having to check which target it belongs to.

[maxcao13]

@hareetd Hey sorry, I am sort of lost on how to fix this... I've tried tracing the problem back upwards but I get stuck at NotificationChannel.messages where the function takes a string category and filters it in someway. I'm confused on how the NotificationCategory.[notification] changes the output of the function.

Since the archived recording Notification doesn't include a target, isn't that the backend's fault for not providing it? Please let me know what I am missing and which way I should be pointed towards.

[andrewazores]

Since the archived recording Notification doesn't include a target, isn't that the backend's fault for not providing it? Please let me know what I am missing and which way I should be pointed towards.

Correct, the fix will require a patch to the backend to emit this information as well as a patch to the frontend to handle it accordingly.

NotificationChannel.messages where the function takes a string category and filters it in someway.

The filtering performed is just that the notification's category matches the provided argument. Components don't generally need to subscribe to every kind of notification, only ones with one or two specific categories. In this case there's a notification category for when a recording is copied to archives, and another when a recording is deleted from archives, for example.

[maxcao13]

Correct, the fix will require a patch to the backend to emit this information as well as a patch to the frontend to handle it accordingly.

So does this mean I have to file a new issue in [cryostat] to patch this then?

[andrewazores]

Yep.

[maxcao13]

How do I recreate this bug for case 2? I tried checking out Hareet's repo and doing it that way but the archive view was loading a while and I get errors.

I'm also asking about a bit more clarification about case 2. Is it a problem that the UploadedRecordings don't have a target, or is it the ArchivedRecordings that get uploaded and sent to the AllArchivesView that don't have a target?

[andrewazores]

Hareet's frontend changes require corresponding backend changes. You'll need to run both of them together, otherwise API requests will fail and result in the ever-loading views and errors you see.

[hareetd]

I'm also asking about a bit more clarification about case 2. Is it a problem that the UploadedRecordings don't have a target, or is it the ArchivedRecordings that get uploaded and sent to the AllArchivesView that don't have a target?

There are two separate actions here: saving active recordings to the archives vs. uploading recordings directly to the archives. The former issues ActiveRecordingSaved notifications to the front end while the latter issues ArchivedRecordingCreated notifications. Find the separate handlers responsible for each of these two actions and you'll see which notifications are missing a target field. Off the top of my head, I'm pretty sure the ActiveRecordingSaved notification already has a target field since it's a target-related action.

As for the actual problem, we need both notification types to contain a target field in order to differentiate between saved vs. uploaded archived recordings. My Archives view PR will need this change because it has two tables, one for archived recordings linked to targets (i.e. created from saving active recordings on these targets) and one for uploads directly to archives. However, both tables use the ArchivedRecordingsTable component to house the data. Since we currently can't differentiate between the two types of archived recordings using the target field in the notifications, the Uploads table view adds target-related archived recordings upon receiving ActiveRecordingSaved notifications, when it should only consist of uploaded recordings.

[hareetd]

I could also just listen separately for ActiveRecordingSaved and ArchivedRecordingCreated notifications, depending on which "type" of ArchivedRecordingsTable the view is currently showing (i.e. target-related or uploads), which would eliminate the need for a target field. However, since you've already added a target field to the ArchivedRecordingDeleted notification, might as well do it for the others as well; it would be nice to have the target-source information in case we need it for future changes too.

[maxcao13]

I'm having trouble finding the actual targetId the uploaded recordings come from. I'm rather certain that RecordingsPostHandler has the notifications that are being emitted, but unlike the Case 1 where the parentPath is the encoded serviceUri for the recordings, these uploaded recordings have no such parentPath and they just get put in the unlabelled folder of the archives.

Then is it okay to have target: "unlabelled" or I am missing something?

[hareetd]

Yep, that's it. The target: "unlabelled" approach is actually what I had in mind for the solution as well. You just need something in the target field to help process the ActiveRecordingSaved and ArchivedRecordingCreated notifications when they're subscribed to at the same time on the frontend.

[maxcao13]

Now that https://github.com/cryostatio/cryostat/issues/986 is fixed, sorry I think I asked this before but what is there to do now for the front-end?

[hareetd]

Known cases:

1. Archived Recording Deleted notifications do not update the table state [https://github.com/feat(archives): Redesign "All Archives" view #431#issuecomment-1149070884, https://github.com/feat(archives): Redesign "All Archives" view #431#issuecomment-1149071976]

2. Unable to differentiate between uploaded vs. saved archived recordings, resulting in a bad table state in feat(archives): Redesign "All Archives" view #431 [https://github.com/feat(archives): Redesign "All Archives" view #431#issuecomment-1149067098]

You just have to take care of case 1. I'll use your backend changes to address case 2 in my All Archives PR.

[maxcao13]

Forgive me if I'm looking at this wrong but since the original problem was that

the archived recordings notifications do not contain a target field for matching the connectUrl of the current target so the state is never updated

but now it does, isn't the problem solved? I tried doing a ArchivedRecordingsDelete and it gets past the match now

[hareetd]

Ah right, I was thinking of the old ArchivedRecordingsTable which included both target-related and uploaded recordings.

@andrewazores In that case, since Max's backend PR takes care of case 1 and case 2 (indirectly), should it simply close this issue?

[andrewazores]

@maxcao13 @hareetd yep looks to me like the issue is solved. The frontend was already expecting the notification to have this format in both the views, so now that the backend is emitting the proper notifications this bug is solved.