-
Notifications
You must be signed in to change notification settings - Fork 2.3k
fix: feature toggle auto updater #6175
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
Conversation
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.
Caution
Changes requested ❌
Reviewed everything up to 388959a in 1 minute and 56 seconds. Click for details.
- Reviewed
81lines of code in2files - Skipped
0files when reviewing. - Skipped posting
1draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. web-app/src/hooks/useAppUpdater.ts:156
- Draft comment:
AUTO_UPDATER_DISABLED is checked again in downloadAndInstallUpdate without explicit definition. Consider abstracting this check (or ensuring a proper import) to avoid duplication and potential runtime issues. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 30% vs. threshold = 50% The comment raises a valid point about code quality - an undefined constant is being used and there's duplicate logic. However, since AUTO_UPDATER_DISABLED isn't defined in this file, it must be defined elsewhere, likely in a config or constants file. Without seeing where it's defined, we can't be certain there's actually an issue. The duplication aspect is minor since it's just a simple guard clause. I might be too quick to dismiss the duplication concern - repeated guard clauses could indicate a need for a higher-level architectural change. Also, using an undefined constant could cause runtime errors. While those are fair concerns, we don't have enough context to know if AUTO_UPDATER_DISABLED is properly defined elsewhere. The duplication is minimal and serves a clear purpose in each function. Delete the comment. Without seeing the full codebase context around AUTO_UPDATER_DISABLED's definition, we can't be certain there's an actual issue here.
Workflow ID: wflow_bzpqueEN4lsGrS5A
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
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.
Caution
Changes requested ❌
Reviewed dcb4617 in 1 minute and 40 seconds. Click for details.
- Reviewed
71lines of code in1files - Skipped
0files when reviewing. - Skipped posting
1draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. web-app/src/hooks/__tests__/useAppUpdater.test.ts:257
- Draft comment:
Replacing direct state mutation with a call to checkForUpdate improves encapsulation. This ensures the hook’s internal logic is exercised rather than bypassed. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
Workflow ID: wflow_BsRXwDKd7EikjXJ2
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
Barecheck - Code coverage reportTotal: 28.69%Your code coverage diff: -0.11% ▾ Uncovered files and lines
|
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.
LGTM
…pdater fix: feature toggle auto updater
This pull request adds logic to disable the auto-updater feature throughout the app when the
AUTO_UPDATER_DISABLEDflag is set. It ensures that update-related actions and UI elements are hidden or prevented from executing when auto-updates are disabled.Auto-updater disabling logic:
AUTO_UPDATER_DISABLEDflag in both thecheckForUpdateanddownloadAndInstallUpdatefunctions withinuseAppUpdater.ts, preventing update logic from running if auto-updates are disabled. [1] [2]UI updates for disabled auto-updater:
Generalsettings page to conditionally render the "Check for Updates" card only when auto-updates are enabled, hiding it ifAUTO_UPDATER_DISABLEDis true. [1] [2]Important
Disable auto-updater feature when
AUTO_UPDATER_DISABLEDflag is set, affecting logic and UI inuseAppUpdater.tsandgeneral.tsx.AUTO_UPDATER_DISABLEDincheckForUpdateanddownloadAndInstallUpdateinuseAppUpdater.ts, preventing execution if true.Generalsettings page ingeneral.tsxto hide "Check for Updates" card whenAUTO_UPDATER_DISABLEDis true.AUTO_UPDATER_DISABLEDinuseAppUpdater.test.tsto test new behavior.checkForUpdateinstead of directly settingupdateInfoinuseAppUpdater.test.ts.This description was created by
for dcb4617. You can customize this summary. It will automatically update as commits are pushed.