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

Poll for pending records in UI until active or error #355

Merged
merged 1 commit into from
Mar 17, 2023

Conversation

humphd
Copy link
Contributor

@humphd humphd commented Mar 15, 2023

Fixes #316.

This change removes the manual update logic for a user's DNS Records, replacing it with automated polling based on the status of the records.

When we display the table, we check to see if any of the records are status === 'pending' (i.e., vs. active or error). If any are pending, we start an interval that runs every 5s, and recalls the loader to check for updates. Once the pending status is gone, we stop the interval.

To test this:

  1. load the app on localhost:8080
  2. login
  3. go to the DNS Records
  4. click "Create new domain" button
  5. fill out the form (e.g., name=issue316, type=A, value=192.168.2.1)
  6. notice the record is displayed in the table with a pending icon
  7. notice that it automatically changes to a green checkmark when complete

I also cleaned up the ordering of the imports.

@humphd humphd added this to the Milestone 0.6 milestone Mar 15, 2023
@humphd humphd self-assigned this Mar 15, 2023
@humphd humphd requested a review from Eakam1007 March 17, 2023 12:34
revalidator.revalidate();
},
pending ? 5_000 : null
);

return (
<Container maxW="contianer.xl">
Copy link
Contributor

@SerpentBytes SerpentBytes Mar 17, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not related to this PR, but we should file an issue to fix the spelling error in the value for maxW, so CSS rules can be applied as intended.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice. I'll fix it here, since I'm in this file.

@Ririio
Copy link
Contributor

Ririio commented Mar 17, 2023

In this PR, will the newly created record ever change to 'active'?

@humphd
Copy link
Contributor Author

humphd commented Mar 17, 2023

It should. When that happens depends on how long it takes for the DNS to finish.

@Ririio
Copy link
Contributor

Ririio commented Mar 17, 2023

image

Shouldn't it be issue-315.user1.starchart.com?

@humphd
Copy link
Contributor Author

humphd commented Mar 17, 2023

image

Shouldn't it be issue-315.user1.starchart.com?

#359

Genne23v
Genne23v previously approved these changes Mar 17, 2023
Copy link
Contributor

@Genne23v Genne23v left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

const domains = useTypedLoaderData<typeof loader>();
const pending = domains.some((domain) => domain.status === 'pending');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would be useMemo here applicable to avoid unnecessary UI re-renders?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we need useMemo here, since we always want to be in sync with the value of `domains, and the cost of the calculation is cheap.

But I'll add and we can remove if it i's not working.

Myrfion
Myrfion previously approved these changes Mar 17, 2023
Copy link
Contributor

@Myrfion Myrfion left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Genne23v
Genne23v previously approved these changes Mar 17, 2023
@humphd
Copy link
Contributor Author

humphd commented Mar 17, 2023

I had to rebase to update pacakge-lock.json. Extending reviews forward and landing.

@humphd humphd merged commit 7486a7f into DevelopingSpace:main Mar 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Poll for DNS Record updates when any records are pending
6 participants