-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Settings screen implementation #2934
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
This commit introduces new settings pages for Appearance, Advanced, Indexer, Library, Privacy, and Services. It also adds core operations for getting and updating application and library configurations. Co-authored-by: ijamespine <[email protected]>
|
Cursor Agent can help with this pull request. Just |
|
@cursor resolve the merge conflicts |
|
Perfect! The merge conflicts have been successfully resolved. Let me create a summary of what was done: SummaryI've successfully resolved the merge conflicts in the PR. Here's what was done: Conflicts Resolved:
Result:
Learn more about Cursor Agents |
Resolved merge conflicts: - Updated settings index.tsx import path to reference ../../Settings/pages - Accepted main branch version of bun.lockb
This commit introduces several improvements to the settings interface, including the addition of device name and slug fields in the GeneralSettings component. It also refactors the settings structure to utilize a more consistent context for state management. The UI has been updated with animations and improved styling for a better user experience. Additionally, the app now supports logging configuration updates and integrates React Query Devtools for enhanced debugging capabilities.
PR SummaryImplements a functional Settings experience with backend support for configuration.
Written by Cursor Bugbot for commit f6a68d3. Configure here. |
apps/tauri/src/App.tsx
Outdated
| SpacedriveProvider, | ||
| ServerProvider, | ||
| } from "@sd/interface"; | ||
| import { ReactQueryDevtools } from "@tanstack/react-query-devtools"; |
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.
This import is unused - ReactQueryDevtools is not rendered in this file.
apps/tauri/src/App.tsx
Outdated
|
|
||
| // Route to different UIs based on window type | ||
| if (route === "/settings") { | ||
| console.log("[App] Rendering Settings route"); |
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.
These debug console.log statements should be removed before merging.
| AdvancedSettings, | ||
| AboutSettings, | ||
| } from "../../Settings/pages"; | ||
| import { ReactQueryDevtools } from "@tanstack/react-query-devtools"; |
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.
This import is unused here - ReactQueryDevtools is imported but the component using it was removed during refactoring.
| buttonPosition="bottom-right" | ||
| /> | ||
| </> | ||
| ); |
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.
These debug console.log statements should be removed before merging.
| console.log("[GeneralSettings] Status query:", { | ||
| data: statusQuery.data, | ||
| isLoading: statusQuery.isLoading, | ||
| error: statusQuery.error, |
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.
Debug logging should be removed before merging. This entire useEffect block with console.log statements appears to be debugging code.
| }, | ||
| }); | ||
|
|
||
| const onDeviceSubmit = deviceForm.handleSubmit(async (data) => { |
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.
Consider validating form state before submission. The handleSubmit wrapper should prevent submission while pending, but the button's disabled state relies on updateDevice.isPending which may not cover all edge cases (e.g., during validation).
| </p> | ||
| </div> | ||
|
|
||
| <form onSubmit={onSubmit} className="space-y-4"> |
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.
Using alert() for user notifications isn't ideal for a polished UI. Consider using a toast notification or modal component for a better user experience.
| onSuccess: (result) => { | ||
| alert( | ||
| result.message || "Data has been reset. Please restart the application." | ||
| ); |
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.
Using window.confirm() and alert() for user interactions isn't consistent with a modern application UX. Consider using a custom modal/dialog component from your UI library instead.
| const { data: config, refetch } = useCoreQuery({ type: "config.app.get", input: null as any }); | ||
| const updateConfig = useCoreMutation("config.app.update"); | ||
|
|
||
| const form = useForm<AdvancedSettingsForm>({ |
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.
The input type passed to useCoreQuery uses null as any which bypasses type safety. Consider using the proper input type from the generated types, or if the input is truly null/empty, use the appropriate type definition.
| const libraryId = client.getCurrentLibraryId(); | ||
| const { data: config, refetch, isLoading } = useLibraryQuery( | ||
| { type: "config.library.get", input: null as any }, | ||
| { enabled: !!libraryId } |
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.
Same issue here - null as any bypasses type checking. The generated types should have proper input type definitions for these queries.
| // Validate language (basic ISO 639-1 check) | ||
| if let Some(ref lang) = self.input.language { | ||
| if lang.len() != 2 || !lang.chars().all(|c| c.is_ascii_lowercase()) { | ||
| return Err(ActionError::Validation { |
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.
The language validation is restrictive to exactly 2-character ISO 639-1 codes. However, some locales use extended tags like zh-CN or pt-BR for regional variants. Consider whether this validation should be relaxed or if these cases will be handled differently.
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.
Cursor Bugbot has reviewed your changes and found 3 potential issues.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.
| const client = useSpacedriveClient(); | ||
| const statusQuery = useCoreQuery({ type: "core.status", input: null as any }); | ||
| const configQuery = useCoreQuery({ type: "config.app.get", input: null as any }); | ||
| const updateDevice = useCoreMutation("device.update"); |
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.
Device update action missing from generated TypeScript bindings
High Severity
The device.update action is registered in the backend (UpdateDeviceAction with register_core_action!) but the corresponding TypeScript types and wire method mappings were not generated. GeneralSettings.tsx uses useCoreMutation("device.update") which will fail at runtime because WIRE_METHODS.coreActions["device.update"] is undefined. The @ts-nocheck directive in useMutation.ts prevents TypeScript from catching this at compile time. This breaks the device name/slug editing functionality in the General Settings page.
Additional Locations (1)
…nt structure This commit cleans up the GeneralSettings and Settings components by removing redundant logging statements and the use of the Spacedrive client where it is no longer needed. Additionally, it updates the DeviceInfo type to include a slug field and introduces new types for device configuration updates, enhancing the overall structure and clarity of the codebase.
This PR implements the initial phase of the settings screen, providing a functional interface for configuring core application and library-specific settings.
It introduces new backend operations for
AppConfigandLibraryConfigto fetch and update settings, alongside 8 new frontend pages: General, Appearance, Library, Indexer, Services, Privacy, Advanced, and About. These pages integrate with the new backend operations, allowing users to manage various aspects of Spacedrive's behavior, from UI preferences to daemon services and indexing rules.This lays the groundwork for future advanced configuration features.
Closes #(issue)