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(AutoRefresh): reflect default value selection when auto refresh i… #7139

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

vsnegi039
Copy link

@vsnegi039 vsnegi039 commented Feb 17, 2025

Summary

Ensure the default value of auto-refresh (30s) is reflected when the user selects it

Related Issues / PR's

#7048

Screenshots

screen-capture.12.webm

Affected Areas and Manually Tested Areas

frontend/src/container/TopNav/AutoRefreshV2/index.tsx


Important

Fix default value selection for auto-refresh in AutoRefresh component to reflect '30s' when enabled.

  • Behavior:
    • In AutoRefresh component, onChangeAutoRefreshHandler now sets selectedOption to '30s' when auto-refresh is enabled.
    • Ensures default value is reflected when user selects auto-refresh.
  • Affected Areas:
    • frontend/src/container/TopNav/AutoRefreshV2/index.tsx

This description was created by Ellipsis for ff5fb49. It will automatically update as commits are pushed.

@vsnegi039 vsnegi039 requested a review from YounixM as a code owner February 17, 2025 19:14
@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.


vsnegi039 seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
You have signed the CLA already but the status is still pending? Let us recheck it.

Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

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

❌ Changes requested. Reviewed everything up to ff5fb49 in 1 minute and 58 seconds

More details
  • Looked at 15 lines of code in 1 files
  • Skipped 0 files when reviewing.
  • Skipped posting 1 drafted comments based on config settings.
1. frontend/src/container/TopNav/AutoRefreshV2/index.tsx:133
  • Draft comment:
    Avoid hardcoding the default option value. Instead of '30s', use options[0].key or a constant so that defaults update consistently.
  • Reason this comment was not posted:
    Marked as duplicate.

Workflow ID: wflow_pI56yvKQRRHg4YoY


Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

setSelectedOption('off'); // Ensure selectedOption reflects the removal
} else {
// Ensure default option reflects
setSelectedOption('30s');
Copy link
Contributor

Choose a reason for hiding this comment

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

When enabling auto-refresh (checked true), the default interval is set in state as '30s', but it isn’t persisted to localStorage. This may lead to inconsistencies (e.g., after a page reload, the default won’t be retained). Consider adding a localStorage update (e.g., using set() with the new value) similar to the unchecked branch. Additionally, avoid using the magic string '30s' directly—use a constant or derive it from the default option in your config.

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