Skip to content

refactor: centralize app settings via typed RPC and shared provider (4)#1230

Merged
arnestrickmann merged 3 commits intogeneralaction:mainfrom
Davidknp:improve-app-settings
Mar 2, 2026
Merged

refactor: centralize app settings via typed RPC and shared provider (4)#1230
arnestrickmann merged 3 commits intogeneralaction:mainfrom
Davidknp:improve-app-settings

Conversation

@Davidknp
Copy link
Collaborator

@Davidknp Davidknp commented Mar 2, 2026

Summary

  • Introduce a typed RPC layer for app settings — replaces the old settings:get / settings:update IPC channels with a createRPCController handler (appSettings.get / appSettings.update) whose return types flow end-to-end from settings.ts through to the renderer with no any casts.
  • Create AppSettingsProvider — a single React context backed by TanStack Query that owns fetching, caching, optimistic updates, and rollback for app settings. All 15+ settings components now call useAppSettings() instead of each managing their own useState + useEffect + rpc call.
  • Introduce DeepPartial<AppSettings> / AppSettingsUpdate — typed deep-partial update shape so callers only pass the fields they're changing; the server-side deepMerge does the rest, eliminating the bug where components spread stale cached values on every save.
  • Remove 272 lines from electron-api.d.ts — the hand-maintained getSettings / updateSettings declarations and their duplicated types (ProviderCustomConfig, etc.) are deleted; the ground truth lives in settings.ts now.
  • Net result: −1,132 lines, +353 lines across 34 files.

Why this was necessary

Every settings component had its own copy of the same pattern:

const [value, setValue] = useState(DEFAULT);
useEffect(() => { rpc.appSettings.get().then(setValue) }, []);
const save = async (next) => { setSaving(true); await rpc.appSettings.update(...); setSaving(false); };

This caused several concrete problems:

  1. Stale data bugs — each component held its own in-memory snapshot; toggling one setting could silently overwrite another field changed elsewhere.
  2. No optimistic updates — the UI lagged behind user input while waiting for the IPC round-trip.
  3. settings:get / settings:update errors — several components called the old window.electronAPI.getSettings() API that no longer had a registered handler after a prior IPC refactor, causing visible console errors on load.
  4. Duplicated typesProviderCustomConfig, AppSettings sub-types, and related shapes were declared in both settings.ts and electron-api.d.ts, drifting out of sync.
  5. any return types — the @/shared/ipc/rpc import in settingsIpc.ts used the @/* alias which resolves differently under the renderer's tsconfig.json, causing the entire RpcRouter type to collapse to any in the renderer.

@vercel
Copy link

vercel bot commented Mar 2, 2026

@Davidknp is attempting to deploy a commit to the General Action Team on Vercel.

A member of the Team first needs to authorize it.

@Davidknp Davidknp changed the title refactor: centralize app settings via typed RPC and shared provider refactor: centralize app settings via typed RPC and shared provider (4) Mar 2, 2026
@Davidknp Davidknp marked this pull request as ready for review March 2, 2026 19:05
@greptile-apps
Copy link
Contributor

greptile-apps bot commented Mar 2, 2026

Greptile Summary

This PR successfully centralizes app settings management by introducing a typed RPC layer and React context provider. The refactor eliminates 1,132 lines of duplicated state management logic and replaces it with a single source of truth backed by TanStack Query.

Key improvements

  • Type safety end-to-end: RPC controller types flow from settings.ts through to the renderer with no any casts, eliminating the type-collapse issue mentioned in the description
  • Eliminated duplication: Removed 272 lines of duplicate type definitions from electron-api.d.ts and consolidated settings access through useAppSettings() hook
  • Simplified components: Settings cards reduced from 50-100 lines each to ~20-30 lines by removing local state, useEffect, and IPC calls
  • Migration consistency: All 15+ settings components successfully migrated to the new pattern

Issues found

  • Critical logic bug in NotificationSettingsCard.tsx:31 — condition inverted, disables sub-settings when notifications are enabled instead of disabled
  • Code duplicationdeepMerge implemented in both src/main/settings.ts and src/renderer/lib/utils.ts
  • Missing optimistic updates — PR description claims to fix "no optimistic updates" but AppSettingsProvider doesn't implement onMutate for optimistic UI updates; still waits for IPC round-trip

Confidence Score: 3/5

  • Safe to merge after fixing the critical logic bug in NotificationSettingsCard
  • Score reflects one critical logic bug that breaks notification sub-settings, plus minor issues with code duplication and missing optimistic updates. The architectural changes are solid and well-executed across 53 files, but the inverted boolean logic is a showstopper that needs immediate fixing.
  • Pay close attention to src/renderer/components/NotificationSettingsCard.tsx (critical bug on line 31)

Important Files Changed

Filename Overview
src/renderer/components/NotificationSettingsCard.tsx Critical logic bug - sub-settings disabled when notifications are enabled instead of disabled
src/renderer/contexts/AppSettingsProvider.tsx New provider created but lacks optimistic updates despite PR claiming to fix this
src/main/ipc/settingsIpc.ts Successfully migrated from manual IPC handlers to typed RPC controller
src/main/settings.ts Added DeepPartial type and AppSettingsUpdate, duplicated deepMerge in renderer utils
src/renderer/types/electron-api.d.ts Removed 272 lines of duplicate type definitions, now sourced from settings.ts
src/renderer/lib/utils.ts Added deepMerge utility but duplicates logic from main/settings.ts

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[Settings Components] -->|useAppSettings| B[AppSettingsProvider]
    B -->|TanStack Query| C[Query Cache]
    B -->|rpc.appSettings.get| D[RPC Client]
    B -->|rpc.appSettings.update| D
    D -->|IPC invoke| E[Main Process]
    E -->|appSettings.get| F[appSettingsController]
    E -->|appSettings.update| F
    F -->|getAppSettings| G[settings.ts]
    F -->|updateAppSettings| G
    G -->|Read/Write| H[settings.json]
    B -->|invalidateQueries| C
    
    subgraph Renderer Process
        A
        B
        C
        D
    end
    
    subgraph Main Process
        E
        F
        G
        H
    end
Loading

Last reviewed commit: 5cc2b73

Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

53 files reviewed, 1 comment

Edit Code Review Agent Settings | Greptile

@arnestrickmann
Copy link
Contributor

sick refactor

@arnestrickmann arnestrickmann merged commit 62221b0 into generalaction:main Mar 2, 2026
2 of 3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants