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

fix: ds-438 add refetch interval to query helper #485

Merged
merged 1 commit into from
Oct 17, 2024

Conversation

nicolearagao
Copy link
Member

@nicolearagao nicolearagao commented Oct 14, 2024

Add the parameter refetchInterval provided by react-query and set it to one minute.

Notes

  • fix to [Mirek] Scans page refreshes automatically every 15 minutes. That seems a bit high - every 5 minutes would probably be enough.
    2024-10-02 Is this a timer issue, or is this a browser issue? The timer should be 1 minute to be consistent with the old UI. Mirek suggests that in some cases the browser may pause background tabs.
    If the issue is in our code having the “wait” number too high, please lower the number.
    Future idea: configurable refresh time.
    [Mirek] 2024-10-09 I found this JIRA that might be related: DISCOVERY-443
  • This doesn't solve the issue raised on DISCOVERY-443, we can always lower the interval in whch we refresh the page for scans, but according to the acceptance criteria this needs to happen when a few pre-requisites are met. The solution for this problem requires a more complex implementation, therefore I would recommend attempting to solve this in a second iteration.

How to test

Coverage and basic unit test check

  1. update the NPM packages with $ npm install
  2. $ npm test
  3. confirm tests come back clean

Local run check

  1. update the NPM packages with $ npm install
  2. $ npm run start
  3. confirm connections display as intended

Example

Updates issue/story

DISCOVERY-438

@nicolearagao nicolearagao marked this pull request as ready for review October 14, 2024 18:06
@nicolearagao nicolearagao requested review from a team October 14, 2024 18:06
Copy link
Member

@cdcabrera cdcabrera left a comment

Choose a reason for hiding this comment

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

it would be good to leverage the left-over dotenv parameter here instead of hardcoding this value

Copy link
Member

@cdcabrera cdcabrera left a comment

Choose a reason for hiding this comment

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

close! one change, comment about legacy behavior, and an optional

.env Show resolved Hide resolved
src/helpers/queryHelpers.ts Outdated Show resolved Hide resolved
Copy link
Member

@cdcabrera cdcabrera left a comment

Choose a reason for hiding this comment

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

optional recommended clean up work... otherwise works!

src/helpers/queryHelpers.ts Show resolved Hide resolved
Copy link
Member

@cdcabrera cdcabrera left a comment

Choose a reason for hiding this comment

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

github is acting odd, my 2nd approval ha

@nicolearagao nicolearagao enabled auto-merge (squash) October 17, 2024 01:45
@nicolearagao nicolearagao merged commit 21d441f into main Oct 17, 2024
7 checks passed
@nicolearagao nicolearagao deleted the add-automatic-refresh-time branch October 17, 2024 01:45
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.

2 participants