-
Notifications
You must be signed in to change notification settings - Fork 2.4k
fix: update default GPU toggle, and simplify state #5937
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 ed8442e in 1 minute and 3 seconds. Click for details.
- Reviewed
417lines of code in6files - Skipped
0files when reviewing. - Skipped posting
5draft 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__/useLlamacppDevices.test.ts:80
- Draft comment:
The 'should set devices directly' test passes a device object without an explicit 'activated' property. Since the new type requires 'activated: boolean', consider updating the test fixture to include it (e.g. activated: false) for consistency. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
2. web-app/src/hooks/useLlamacppDevices.ts:95
- Draft comment:
In toggleDevice, updateSettings is awaited without error handling. Consider wrapping the updateSettings call in a try/catch block to handle potential provider update failures gracefully. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
3. web-app/src/routes/settings/hardware.tsx:87
- Draft comment:
Removal of manual activatedDevices initialization simplifies the component. Ensure that fetchDevices reliably initializes the activation state for all devices as intended. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
4. web-app/src/routes/settings/providers/$providerName.tsx:369
- Draft comment:
Replacing setActivatedDevices with a call to fetchDevices for resetting activation state is appropriate. If rapid backend changes occur, consider debouncing the fetchDevices call to avoid excessive updates. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
5. web-app/src/services/hardware.ts:10
- Draft comment:
The DeviceList interface now includes an 'activated' property. Ensure that the llamacpp extension’s getDevices() either supplies this property or that the hook correctly assigns it during mapping. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
Workflow ID: wflow_Xj4vomFYT1QbHBes
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.
LGTM and works great!
Describe Your Changes
This pull request refactors the
useLlamacppDeviceshook to simplify device activation management by incorporating the activation status directly into thedevicesarray. It removes the separateactivatedDevicesstate and updates related components and tests accordingly.Refactor of device activation management:
web-app/src/hooks/useLlamacppDevices.ts: Removed theactivatedDevicesstate and added anactivatedproperty to each device in thedevicesarray. Updated thefetchDevicesandtoggleDevicemethods to handle activation status directly within thedevicesarray. [1] [2] [3]web-app/src/services/hardware.ts: Updated theDeviceListinterface to include anactivatedproperty and ensured that devices fetched from the llamacpp extension include their activation status. [1] [2]Updates to tests:
web-app/src/hooks/__tests__/useLlamacppDevices.test.ts: Updated tests to reflect the new activation status handling. Removed references toactivatedDevicesand validated theactivatedproperty within thedevicesarray. [1] [2] [3]Component updates:
web-app/src/routes/settings/hardware.tsx: Removed logic for initializingactivatedDevicesand updated UI bindings to use theactivatedproperty of devices. [1] [2] [3]web-app/src/routes/system-monitor.tsx: Simplified the component by removing initialization logic foractivatedDevicesand updated UI elements to reflect the newactivatedproperty. [1] [2] [3] [4]Other updates:
web-app/src/routes/settings/providers/$providerName.tsx: Adjusted llamacpp device activation reset logic to use the updatedfetchDevicesmethod instead ofsetActivatedDevices.Fixes Issues
Self Checklist
Important
Refactor
useLlamacppDevicesto manage device activation withindevicesarray, updating components and tests accordingly.useLlamacppDevicesinuseLlamacppDevices.tsto includeactivatedstatus indevicesarray, removingactivatedDevicesstate.fetchDevicesandtoggleDeviceto manage activation withindevices.DeviceListinhardware.tsto includeactivatedproperty.useLlamacppDevices.test.tsto validateactivatedproperty indevicesand removeactivatedDevicesreferences.hardware.tsxandsystem-monitor.tsxto useactivatedproperty for UI logic.$providerName.tsxto usefetchDevices.This description was created by
for ed8442e. You can customize this summary. It will automatically update as commits are pushed.