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

Resolves #3220 - show heartbeat status in top bar #3252

Merged
merged 21 commits into from
Jun 7, 2024
Merged

Conversation

alexcottner
Copy link
Contributor

@alexcottner alexcottner commented May 23, 2024

Resolves #3220 . Show the heartbeat endpoint status in top bar. Status icon is also a link that will take the user to the __heartbeat__ endpoint. Also resolves a brief error that users see during redirect to auth.

  • Add check to top bar (first draft done)
  • Add unit tests
  • Fill in missing unit tests (don't have tests for SessionInfoBar yet)

Screenshots of error (red !) vs success (green circle):
image
Screenshot from 2024-05-28 10-18-53

@alexcottner alexcottner marked this pull request as draft May 23, 2024 20:58
src/actions/session.ts Outdated Show resolved Hide resolved
@alexcottner alexcottner changed the title WIP - Resolves #3220 - show heartbeat status in top bar Resolves #3220 - show heartbeat status in top bar May 28, 2024
@alexcottner alexcottner marked this pull request as ready for review May 28, 2024 19:48
src/actions/session.ts Outdated Show resolved Hide resolved
src/actions/session.ts Outdated Show resolved Hide resolved
src/components/SessionInfoBar.tsx Outdated Show resolved Hide resolved
src/components/SessionInfoBar.tsx Outdated Show resolved Hide resolved
src/components/SessionInfoBar.tsx Outdated Show resolved Hide resolved
src/components/SessionInfoBar.tsx Outdated Show resolved Hide resolved
src/components/SessionInfoBar.tsx Outdated Show resolved Hide resolved
Copy link
Contributor

@grahamalama grahamalama left a comment

Choose a reason for hiding this comment

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

Thanks for updating this to use Redux + Sagas. I spun up this issue for us to track the conversation for adopting new state management / data fetching patterns.

For this PR:

  • I see you elected to add a new heartbeat slice of the store. Any reason you didn't opt too add a property in the session slice, e.g. session.heartbeat? We are adding this to the SessionInfoBar, after all
  • Adding a few saga tests (see these examples for session sagas) might be a good idea

@alexcottner
Copy link
Contributor Author

I see you elected to add a new heartbeat slice of the store. Any reason you didn't opt too add a property in the session slice, e.g. session.heartbeat?

Mainly because the session slice is written to localStorage, and that felt a little awkward. Do you think that's a reasonable place for it though?

@alexcottner
Copy link
Contributor Author

Added a few tests for the saga.

Copy link
Contributor

@grahamalama grahamalama left a comment

Choose a reason for hiding this comment

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

Do you think that's a reasonable place for it though?

It does feel more reasonable to have this in the session slice, but yeah, doesn't make sense to save to local storage. Maybe we could update saveSession / loadSession to account for that.

However, seeing as this is blocking a Remote Settings release. Maybe we can call this PR good for now and create a follow on issue.

I think all we need to call this PR done is 1 test that demonstrates that the heartbeat is re-fetched after the timeout winds down.

src/components/SessionInfoBar.tsx Show resolved Hide resolved
@alexcottner alexcottner merged commit a30b4d1 into master Jun 7, 2024
6 checks passed
@alexcottner alexcottner deleted the issue-3220 branch June 7, 2024 17:11
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.

Show heartbeat status in top bar
2 participants