feat: userSettings for package #3235

astandrik · 2025-12-17T10:42:06Z

CI Results

Test Status: ❌ FAILED

Total	Passed	Failed	Flaky	Skipped
384	379	1	2	2

Test Changes Summary ⏭️2

⏭️ Skipped Tests (2)

Scroll to row, get shareable link, navigate to URL and verify row is scrolled into view (tenant/diagnostics/tabs/queries.test.ts)
Copy result button copies to clipboard (tenant/queryEditor/queryEditor.test.ts)

Bundle Size: 🔺

Current: 62.56 MB | Main: 62.54 MB
Diff: +0.03 MB (0.04%)

⚠️ Bundle size increased. Please review.

ℹ️ CI Information

Test recordings for failed tests are available in the full report.
Bundle size is measured for the entire 'dist' directory.
📊 indicates links to detailed reports.
🔺 indicates increase, 🔽 decrease, and ✅ no change in bundle size.

astandrik · 2025-12-17T11:13:48Z

@copilot review
@codex review
@cursor review
@greptile review

Copilot · 2025-12-17T11:13:57Z

@astandrik I've opened a new pull request, #3237, to work on those changes. Once the pull request is ready, I'll request review from you.

greptile-apps

Additional Comments (1)

src/components/SplitPane/SplitPane.tsx, line 75-91 (link)

logic: Missing dependencies in useEffect hooks cause stale closure issues. Both effects call setDefaultSizePane which uses saveSizesStringDebounced, but neither includes it in their dependency arrays. This violates React hooks rules and can cause unexpected behavior.

_{22 files reviewed, 6 comments}

_{Edit Code Review Agent Settings | Greptile}

src/components/SplitPane/SplitPane.tsx

src/store/reducers/settings/utils.ts

src/store/reducers/settings/api.ts

src/store/reducers/settings/settings.ts

Copilot

Pull request overview

This PR introduces remote user settings support for package mode by adding a settingsBackend configuration to the UIFactory. The changes migrate from direct localStorage access to a Redux-based settings management system that can optionally sync with a remote settings service.

Key changes:

Adds settingsBackend configuration to UIFactory with getEndpoint() and getUserId() methods for remote settings
Refactors settings management to use Redux store as source of truth with optional localStorage and remote sync
Introduces debounced remote writes with retry logic and optimistic updates
Adds SettingsBootstrap component to preload settings on app startup

Reviewed changes

Copilot reviewed 22 out of 22 changed files in this pull request and generated 11 comments.

Show a summary per file

File	Description
src/utils/hooks/useSetting.ts	Refactored to use new store-based `useSetting` hook instead of directly dispatching to Redux
src/utils/hooks/useSelectedColumns.ts	Migrated from `settingsManager` to `useSetting` hook for column preferences
src/uiFactory/uiFactory.ts	Added `settingsBackend` property to base factory configuration
src/uiFactory/types.ts	Added `settingsBackend` interface with endpoint and user ID getters
src/types/api/settings.ts	Simplified API types to support new JSON-string-based settings contract
src/store/reducers/settings/utils.ts	Added `shouldSyncSettingToLS` helper and modified `stringifySettingValue` to handle undefined
src/store/reducers/settings/useSetting.ts	Completely refactored to support remote settings with fallback to localStorage
src/store/reducers/settings/settings.ts	Changed initial state to empty object, added conditional localStorage sync
src/store/reducers/settings/effects.ts	New file with handlers for remote setting results and optimistic writes
src/store/reducers/settings/api.ts	Major refactor with debounced remote writes, retry logic, and dual client support
src/store/reducers/cluster/cluster.ts	Removed localStorage access from cluster tab initialization
src/store/configureStore.ts	Added metaSettingsBaseUrl configuration from UIFactory
src/services/api/metaSettings.ts	Added baseUrl override support for custom settings endpoints
src/services/api/index.ts	Added `settingsService` API instance for custom backend
src/containers/Tenant/utils/paneVisibilityToggleHelpers.tsx	Removed localStorage writes, parameter now unused
src/containers/Tenant/Tenant.tsx	Migrated pane collapse state to useSetting hook
src/containers/Tenant/ObjectSummary/ObjectSummary.tsx	Migrated pane collapse state to useSetting hook
src/containers/Cluster/Cluster.tsx	Migrated default cluster tab to useSetting hook with validation
src/containers/App/SettingsBootstrap.tsx	New component to preload settings when remote backend is available
src/containers/App/Content.tsx	Wrapped children in SettingsBootstrap component
src/components/SplitPane/SplitPane.tsx	Migrated pane sizes to useSetting hook with debounced saves
src/components/Drawer/Drawer.tsx	Migrated drawer width to useSetting hook with debounced saves

src/utils/hooks/useSetting.ts

src/containers/App/SettingsBootstrap.tsx

src/components/SplitPane/SplitPane.tsx

src/containers/Cluster/Cluster.tsx

src/containers/Tenant/utils/paneVisibilityToggleHelpers.tsx

src/store/reducers/settings/api.ts

src/components/SplitPane/SplitPane.tsx

src/types/api/settings.ts

src/store/reducers/settings/api.ts

astandrik · 2025-12-17T13:57:13Z

@cursor review
@codex review

cursor · 2025-12-17T13:57:16Z

Skipping Bugbot: Bugbot is disabled for this repository

astandrik · 2025-12-17T13:59:13Z

@greptile review

greptile-apps

_{22 files reviewed, 7 comments}

_{Edit Code Review Agent Settings | Greptile}

src/containers/Tenant/Tenant.tsx

src/containers/App/SettingsBootstrap.tsx

src/utils/hooks/useSetting.ts

src/components/Drawer/Drawer.tsx

src/containers/Tenant/ObjectSummary/ObjectSummary.tsx

Copilot

Pull request overview

Copilot reviewed 22 out of 22 changed files in this pull request and generated 12 comments.

Comments suppressed due to low confidence (2)

src/containers/Tenant/ObjectSummary/ObjectSummary.tsx:81

Superfluous argument passed to function paneVisibilityToggleReducerCreator.

        paneVisibilityToggleReducerCreator(DEFAULT_IS_TENANT_COMMON_INFO_COLLAPSED),

src/containers/Tenant/Tenant.tsx:47

Superfluous argument passed to function paneVisibilityToggleReducerCreator.

        paneVisibilityToggleReducerCreator(DEFAULT_IS_TENANT_SUMMARY_COLLAPSED),

src/uiFactory/types.ts

src/store/configureStore.ts

Copilot · 2025-12-17T14:23:19Z

src/store/reducers/settings/settings.ts

+            return {
+                ...state,
+                userSettings: {
+                    ...state.userSettings,
+                    [action.payload.name]: action.payload.value,
+                },
+            };


The reducer is manually returning a new state object with spread operators to maintain immutability. However, Redux Toolkit's create.reducer uses Immer internally, which allows direct mutation of the draft state. You can simplify this to state.userSettings[action.payload.name] = action.payload.value; without the return statement, which is more idiomatic for RTK and potentially more performant.

Copilot · 2025-12-17T14:23:19Z

src/store/reducers/settings/api.ts

+                    const status = data?.status;
+
+                    if (status && status !== 'SUCCESS') {
                        throw new Error('Cannot set setting - status is not SUCCESS');
                    }


When the status is not 'SUCCESS', an error is thrown with a generic message. However, the actual status value (which might provide useful debugging information) is not included in the error message. Consider including the actual status in the error: new Error(\Cannot set setting - status is ${status}, expected SUCCESS`)` to aid debugging.

Copilot · 2025-12-17T14:23:19Z

src/containers/Tenant/ObjectSummary/ObjectSummary.tsx

+        false,
+    );
    const [commonInfoVisibilityState, dispatchCommonInfoVisibilityState] = React.useReducer(
        paneVisibilityToggleReducerCreator(DEFAULT_IS_TENANT_COMMON_INFO_COLLAPSED),


The function paneVisibilityToggleReducerCreator is called with an argument DEFAULT_IS_TENANT_COMMON_INFO_COLLAPSED, but the updated function signature no longer accepts any parameters. The parameter was removed in the refactoring, but the call site was not updated.

Copilot · 2025-12-17T14:23:20Z

src/containers/Tenant/Tenant.tsx

+        false,
+    );
    const [summaryVisibilityState, dispatchSummaryVisibilityAction] = React.useReducer(
        paneVisibilityToggleReducerCreator(DEFAULT_IS_TENANT_SUMMARY_COLLAPSED),


The function paneVisibilityToggleReducerCreator is called with an argument DEFAULT_IS_TENANT_SUMMARY_COLLAPSED, but the updated function signature no longer accepts any parameters. The parameter was removed in the refactoring, but the call site was not updated.

Copilot · 2025-12-17T14:23:21Z

src/services/api/metaSettings.ts

+function joinBaseUrlAndPath(baseUrl: string, path: string) {
+    const normalizedBaseUrl = baseUrl.endsWith('/') ? baseUrl.slice(0, -1) : baseUrl;
+    const normalizedPath = path.startsWith('/') ? path : `/${path}`;
+    return `${normalizedBaseUrl}${normalizedPath}`;
+}


The joinBaseUrlAndPath helper is duplicating logic that likely exists elsewhere in the codebase. Consider extracting this to a shared utility module to avoid code duplication and ensure consistent URL handling across the application.

Copilot · 2025-12-17T14:23:21Z

src/components/SplitPane/SplitPane.tsx

+    const defaultSizePane = React.useMemo(() => {
        if (initialSizes) {
            return initialSizes;
        }
-        const sizes = localStorage.getItem(defaultSizePaneKey)?.split(',').map(Number);
-        return sizes || defaultSizes;
-    };
-    const setDefaultSizePane = (sizes: number[]) => {
-        const {defaultSizePaneKey} = props;
-        localStorage.setItem(defaultSizePaneKey, sizes.join(','));
-    };
+        const sizes = savedSizesString?.split(',').map(Number);
+        return sizes || defaultSizesProp || sizesDefaultInner;
+    }, [defaultSizesProp, initialSizes, savedSizesString]);


The defaultSizePane is computed in a useMemo from savedSizesString, which means it will update when savedSizesString loads asynchronously. However, if the component has already set innerSizes via user interaction before the remote setting loads, the remote value might override the user's choice. Consider tracking whether the user has manually adjusted sizes and preserving that over late-loading remote settings.

Copilot · 2025-12-17T14:23:21Z

src/store/reducers/settings/api.ts

+async function withRetries<T>(fn: () => Promise<T>, attempts: number, delay: number) {
+    let lastError: unknown;
+    for (let attempt = 0; attempt < attempts; attempt++) {
+        try {
+            return await fn();
+        } catch (error) {
+            lastError = error;
+            if (attempt < attempts - 1) {
+                await delayMs(delay);
+            }
+        }
+    }
+    throw lastError;
+}


The retry logic is hardcoded with 3 attempts and 200ms delay. Consider extracting these as named constants (e.g., REMOTE_WRITE_RETRY_ATTEMPTS, REMOTE_WRITE_RETRY_DELAY_MS) at the top of the file for easier configuration and consistency with REMOTE_WRITE_DEBOUNCE_MS.

Copilot · 2025-12-17T14:23:22Z

src/components/Drawer/Drawer.tsx

+    const [savedWidthString, setSavedWidthString] = useSetting<string | undefined>(storageKey);
+    const [drawerWidth, setDrawerWidth] = React.useState<number | undefined>(() => {
+        return isNumeric(savedWidthString) ? Number(savedWidthString) : defaultWidth;
    });


The drawer width is initialized from savedWidthString in the useState initializer, but when savedWidthString is loaded asynchronously (e.g., from remote settings), the component won't update its drawerWidth state. This means the drawer will always use the default width until the user manually resizes it. Consider adding a useEffect that updates setDrawerWidth when savedWidthString changes and the current width is still undefined or the default.

astandrik added 3 commits December 17, 2025 13:41

feat: userSettings for package

bd02dba

Merge branch 'main' into astandrik.useSettings

fd49f3d

fix: debounced write

78cab23

Copilot AI mentioned this pull request Dec 17, 2025

Code review response: address debounce implementation issues #3237

Draft

astandrik requested a review from Copilot December 17, 2025 11:14

Copilot started reviewing on behalf of astandrik December 17, 2025 11:21 View session

greptile-apps bot reviewed Dec 17, 2025

View reviewed changes

Copilot AI reviewed Dec 17, 2025

View reviewed changes

fix: review fixes

a1c7984

astandrik requested a review from Copilot December 17, 2025 13:56

greptile-apps bot reviewed Dec 17, 2025

View reviewed changes

Copilot started reviewing on behalf of astandrik December 17, 2025 14:17 View session

Copilot AI reviewed Dec 17, 2025

View reviewed changes

astandrik closed this Dec 17, 2025

feat: userSettings for package #3235

feat: userSettings for package #3235

Uh oh!

Conversation

astandrik commented Dec 17, 2025 • edited by github-actions bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

CI Results

Test Status: ❌ FAILED

⏭️ Skipped Tests (2)

Bundle Size: 🔺

Uh oh!

astandrik commented Dec 17, 2025

Uh oh!

Copilot AI commented Dec 17, 2025

Uh oh!

greptile-apps bot left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Additional Comments (1)

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

astandrik commented Dec 17, 2025

Uh oh!

cursor bot commented Dec 17, 2025

Uh oh!

astandrik commented Dec 17, 2025

Uh oh!

greptile-apps bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Copilot AI Dec 17, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 17, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 17, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 17, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 17, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 17, 2025

Choose a reason for hiding this comment

Uh oh!

astandrik commented Dec 17, 2025 •

edited by github-actions bot

Loading

greptile-apps bot left a comment •

edited

Loading