Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Bug] Agent probe notification handler should check current target #893

Closed
tthvo opened this issue Feb 22, 2023 · 1 comment · Fixed by #904
Closed

[Bug] Agent probe notification handler should check current target #893

tthvo opened this issue Feb 22, 2023 · 1 comment · Fixed by #904
Assignees
Labels
bug Something isn't working good first issue Good for newcomers needs-triage Needs thorough attention from code reviewers

Comments

@tthvo
Copy link
Member

tthvo commented Feb 22, 2023

Current Behavior:

Agent Live Configuration table is updating with incorrect data (belonging to other targets).

Expected Behavior:

Agent Live Configuration table should just update its content on notification if the selected target is matched.

Source code

 React.useEffect(() => {
    addSubscription(
      // This will just apply without any knowledge of currently selected target
      context.notificationChannel.messages(NotificationCategory.ProbeTemplateApplied).subscribe((e) => {
        setProbes((old) => {
          const probes = e.message.events as EventProbe[];
          const probeIds = probes.map((p) => p.id);
          if (probes?.length > 0) {
            return [...old.filter((probe) => !probeIds.includes(probe.id)), ...probes];
          }
          return old;
        });
      })
    );
  }, [addSubscription, context, context.notificationChannel, setProbes]);

  React.useEffect(() => {
    addSubscription(
     // same here
      context.notificationChannel.messages(NotificationCategory.ProbesRemoved).subscribe((_) => setProbes([]))
    );
  }, [addSubscription, context, context.notificationChannel, setProbes]);

Others

Related to #709 (previously missing this check).

@tthvo tthvo added bug Something isn't working needs-triage Needs thorough attention from code reviewers labels Feb 22, 2023
@tthvo tthvo moved this to Todo in 2.3.0 release Feb 22, 2023
@tthvo tthvo added the good first issue Good for newcomers label Feb 22, 2023
@tthvo tthvo linked a pull request Feb 23, 2023 that will close this issue
7 tasks
@tthvo tthvo removed a link to a pull request Feb 23, 2023
7 tasks
@tthvo
Copy link
Member Author

tthvo commented Mar 7, 2023

While here, using only combineLatest as below, for example in active recording table:

  React.useEffect(() => {
    addSubscription(
      combineLatest([
        context.target.target(),
        merge(
          context.notificationChannel.messages(NotificationCategory.ActiveRecordingCreated),
          context.notificationChannel.messages(NotificationCategory.SnapshotCreated)
        ),
      ]).subscribe(([currentTarget, event]) => {
        if (currentTarget.connectUrl != event.message.target) {
          return;
        }
        setRecordings((old) => old.concat([event.message.recording]));
      })
    );
  }, [addSubscription, context, context.notificationChannel, setRecordings]);

This is not quite safe. If the context.target.target emits another value (changing global target selection), then outer observable will emit it along with the old event message.

I think we can fix with switchMap but its not too serious thanks to fetching new recordings (take some time). The state is correctly overwritten later on. Just intermediate states are incorrect.

@tthvo tthvo self-assigned this Mar 9, 2023
@tthvo tthvo moved this from Todo to In Progress in 2.3.0 release Mar 9, 2023
@github-project-automation github-project-automation bot moved this from In Progress to Done in 2.3.0 release Mar 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working good first issue Good for newcomers needs-triage Needs thorough attention from code reviewers
Projects
No open projects
Status: Done
Development

Successfully merging a pull request may close this issue.

1 participant