-
Notifications
You must be signed in to change notification settings - Fork 73
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
Handle timeouts in monitor queries in the UI #5519
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
fides Run #11118
Run Properties:
|
Project |
fides
|
Branch Review |
refs/pull/5519/merge
|
Run status |
Passed #11118
|
Run duration | 00m 38s |
Commit |
d24aef54ad ℹ️: Merge f439b365d6389927f88594cb5997fc8b95d782c9 into 4702281527d3524aa2d3f3a01322...
|
Committer | jpople |
View all properties for this run ↗︎ |
Test results | |
---|---|
Failures |
0
|
Flaky |
0
|
Pending |
0
|
Skipped |
0
|
Passing |
4
|
Upgrade your plan to view test results. | |
View all changes introduced in this branch ↗︎ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approved with one minor suggestion.
@@ -58,6 +76,10 @@ const ConfigureMonitorDatabasesForm = ({ | |||
|
|||
const saveIsDisabled = !allSelected && selected.length === 0; | |||
|
|||
if (initialIsLoading) { | |||
return <FidesSpinner my={12} />; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this meant to center it? If so, wouldn't using auto margins be a safer bet there?
return <FidesSpinner my={12} />; | |
return <FidesSpinner my="auto" />; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh man. My bad. I confused my x
and y
lol.
fides Run #11119
Run Properties:
|
Project |
fides
|
Branch Review |
main
|
Run status |
Passed #11119
|
Run duration | 00m 40s |
Commit |
34802667ec: Handle timeouts in monitor queries in the UI (#5519)
|
Committer | jpople |
View all properties for this run ↗︎ |
Test results | |
---|---|
Failures |
0
|
Flaky |
0
|
Pending |
0
|
Skipped |
0
|
Passing |
4
|
Upgrade your plan to view test results. | |
View all changes introduced in this branch ↗︎ |
Closes HJ-188
Description Of Changes
Adds handling and feedback for when async monitor queries take longer than 5s to resolve:
databases: []
(scoped to all projects)Screen recs (note that these have the timeout delay adjusted as described in the testing steps)
Screen.Recording.2024-11-19.at.23.59.21.mov
Screen.Recording.2024-11-20.at.00.00.38.mov
Steps to Confirm
This is tricky to test-- I've had best results by setting my browser throttling to "GPRS" and temporarily adjusting the timeout delays to be shorter (e.g. 500 ms).
Saving a monitor
TIMEOUT_DELAY
of 5000 ms inConfigureMonitorModal.tsx
to something shorter and slow down your connectionProject list
TIMEOUT_DELAY
inuseCumulativeGetDatabases.ts
and slow down your connectionRTK will cache the results of the project list query when it's called on the main "Monitors" tab and then it'll load instantly inside the modal, so we want to temporarily disable this to make it take longer to load:
ConfigureMonitorModal.tsx
comment out lines 44-50:databasesAvailable
prop totrue
in the ConfigureMonitorDatabasesForm (line 102):databases: []
and a toast should appear explaining what happenedPre-Merge Checklist
CHANGELOG.md
updatedmain
downgrade()
migration is correct and works