-
Notifications
You must be signed in to change notification settings - Fork 2.2k
enhancement: disabled config when server is running #5058
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.
Important
Looks good to me! 👍
Reviewed everything up to 756cbbf in 58 seconds. Click for details.
- Reviewed
82
lines of code in1
files - Skipped
0
files when reviewing. - Skipped posting
3
draft 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/routes/settings/local-api-server.tsx:100
- Draft comment:
Good use of a derived state variable (isServerRunning) to simplify repeated checks of the server status. - Reason this comment was not posted:
Confidence changes required:0%
<= threshold50%
None
2. web-app/src/routes/settings/local-api-server.tsx:122
- Draft comment:
Using the derived 'isServerRunning' for the button variant and label improves clarity. - Reason this comment was not posted:
Confidence changes required:0%
<= threshold50%
None
3. web-app/src/routes/settings/local-api-server.tsx:147
- Draft comment:
Consistent application of conditional classes using 'cn' to disable inputs is good. Consider extracting the repeated 'opacity-50 pointer-events-none' string into a constant for easier maintenance. - Reason this comment was not posted:
Confidence changes required:33%
<= threshold50%
None
Workflow ID: wflow_eCQ7iepHyITtMwUk
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
This is the build for this pull request. You can download it from the Artifacts section here: Build URL. |
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.
Thanks!
This is the build for this pull request. You can download it from the Artifacts section here: Build URL. |
756cbbf
to
be5f910
Compare
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.
Important
Looks good to me! 👍
Reviewed be5f910 in 55 seconds. Click for details.
- Reviewed
82
lines of code in1
files - Skipped
0
files when reviewing. - Skipped posting
4
draft 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/routes/settings/local-api-server.tsx:105
- Draft comment:
Good use of derived state with 'isServerRunning' to simplify conditional checks. - Reason this comment was not posted:
Confidence changes required:0%
<= threshold50%
None
2. web-app/src/routes/settings/local-api-server.tsx:127
- Draft comment:
The button's variant and label now clearly use 'isServerRunning', enhancing readability. - Reason this comment was not posted:
Confidence changes required:0%
<= threshold50%
None
3. web-app/src/routes/settings/local-api-server.tsx:159
- Draft comment:
Using 'cn' to conditionally add 'opacity-50 pointer-events-none' improves UX by visually disabling settings when the server runs. Consider abstracting these disabled classes into a helper to reduce duplication. - Reason this comment was not posted:
Confidence changes required:0%
<= threshold50%
None
4. web-app/src/routes/settings/local-api-server.tsx:188
- Draft comment:
For accessibility, consider adding 'aria-disabled' attributes to interactive components in addition to CSS-based disabling. - Reason this comment was not posted:
Confidence changes required:33%
<= threshold50%
None
Workflow ID: wflow_CCeATkUCUKfDMhwl
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
This is the build for this pull request. You can download it from the Artifacts section here: Build URL. |
Describe Your Changes
This pull request refactors the
LocalAPIServer
component inweb-app/src/routes/settings/local-api-server.tsx
to improve readability and user experience. The key changes include introducing a derived state variable for server status, simplifying button logic, and disabling certain UI elements when the server is running.Refactoring and Code Simplification:
isServerRunning
, to simplify logic related to the server's running state. This replaces repeated checks ofserverStatus === 'running'
. [1] [2]UI Enhancements:
LocalAPIServer
component to useisServerRunning
for determining itsvariant
and label text, improving code clarity.ServerHostSwitcher
,PortInput
,ApiPrefixInput
, and switches for CORS and verbose logs) when the server is running by applying conditional classes (opacity-50
andpointer-events-none
) using thecn
utility. This ensures a better user experience by preventing changes to settings while the server is active. [1] [2] [3]Utility Import:
cn
utility import from@/lib/utils
to handle conditional class names for UI elements.Fixes Issues
Self Checklist
Important
Refactors
LocalAPIServer
to useisServerRunning
for logic simplification and disables certain UI elements when the server is running.isServerRunning
inLocalAPIServer
to replaceserverStatus === 'running'
checks.isServerRunning
forvariant
and label.ServerHostSwitcher
,PortInput
,ApiPrefixInput
, and switches for CORS and verbose logs when server is running usingcn
utility.cn
utility import from@/lib/utils
for conditional class names.This description was created by
for be5f910. You can customize this summary. It will automatically update as commits are pushed.