Fix event hooks not persisting across server syncs#799
Fix event hooks not persisting across server syncs#799gsxdsm merged 2 commits intoAutoMaker-Org:v0.15.0rcfrom
Conversation
📝 WalkthroughWalkthroughAdds an explicit Changes
Sequence DiagramsequenceDiagram
actor User
participant Root as Root Component
participant Store as App Store
participant API as Server API
User->>Root: Open app / navigate
Root->>Store: Trigger fast-hydrate
Store->>API: Fetch cached/final settings
API-->>Store: Return finalSettings (may include eventHooks)
Store-->>Root: Hydrated state
Root->>Root: Compare serverHooks vs storeHooks (JSON.stringify)
alt serverHooks non-empty AND differ
Root->>Store: set eventHooks from server
else server empty AND local has hooks
Root->>Store: keep local eventHooks (migration preserves)
end
User->>Store: Call setEventHooks(newHooks)
Store->>Store: Update local state immediately
Store->>API: POST updateGlobal({ eventHooks: newHooks, allowEmptyEventHooks: ... })
API-->>Store: Success or error (error logged)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Suggested labels
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Summary of ChangesHello @gsxdsm, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly improves the robustness and persistence of event hooks across client-server synchronizations. It addresses potential data loss scenarios by safeguarding against empty array overwrites, prioritizing local settings during merges, and intelligently reconciling server-side changes during hydration. The changes ensure that event hook updates are reliably saved to the server, enhancing the overall stability of user configurations. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request aims to fix persistence issues for event hooks by adding server-side guards, migration logic, and making the store action sync with the server. The changes are logical and follow existing patterns. However, I've identified a significant issue where the new guards prevent users from intentionally deleting all their event hooks, leading to a confusing user experience where deletions are silently reverted. I've left two high-severity comments on the server-side and client-side code that contribute to this issue, with suggestions for resolution. The rest of the changes look good.
| ignoreEmptyArrayOverwrite('mcpServers'); | ||
| ignoreEmptyArrayOverwrite('enabledCursorModels'); | ||
| ignoreEmptyArrayOverwrite('claudeApiProfiles'); | ||
| ignoreEmptyArrayOverwrite('eventHooks'); |
There was a problem hiding this comment.
While adding eventHooks to ignoreEmptyArrayOverwrite prevents accidental data loss from an uninitialized client, it also introduces a bug where a user cannot intentionally delete all their event hooks. An attempt to set eventHooks to an empty array will be ignored by the server, and the hooks will reappear on the next application load.
Given that other parts of the client-side logic (like use-settings-migration) seem to handle data preservation during initialization, this server-side guard might be too aggressive.
Consider removing this line to allow users to clear all their event hooks. A similar property, claudeCompatibleProviders, is intentionally not guarded for this reason, as noted in the comment on line 575.
| JSON.stringify(serverHooks) !== JSON.stringify(currentHooks) && | ||
| serverHooks.length > 0 |
There was a problem hiding this comment.
In conjunction with my comment on settings-service.ts, this serverHooks.length > 0 check should be removed. It prevents the client from syncing an empty eventHooks array from the server (e.g., when a user deletes all hooks on another device). This can lead to data inconsistencies where this client retains stale hooks and may even re-sync them back to the server later.
Removing this check will ensure that deleting all hooks is correctly propagated across clients.
JSON.stringify(serverHooks) !== JSON.stringify(currentHooks)
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/server/src/services/settings-service.ts (1)
519-575:⚠️ Potential issue | 🟠 MajorAllow intentional clearing of eventHooks (Line 574).
With this guard in place, any update that setseventHooks: []is discarded when existing hooks are present, so users can’t delete all hooks (they’ll reappear on the next sync). Consider an explicit “allow empty” escape hatch so intentional clears can pass through.💡 Example approach (explicit clear flag)
+ const allowEmptyEventHooks = + (updates as Record<string, unknown>).__allowEmptyEventHooks === true; + if (allowEmptyEventHooks) { + delete (sanitizedUpdates as Record<string, unknown>).__allowEmptyEventHooks; + } - const ignoreEmptyArrayOverwrite = <K extends keyof GlobalSettings>(key: K): void => { + const ignoreEmptyArrayOverwrite = <K extends keyof GlobalSettings>( + key: K, + opts: { allowEmpty?: boolean } = {} + ): void => { const nextVal = sanitizedUpdates[key] as unknown; const curVal = current[key] as unknown; if ( - Array.isArray(nextVal) && + !opts.allowEmpty && + Array.isArray(nextVal) && nextVal.length === 0 && Array.isArray(curVal) && curVal.length > 0 ) { delete sanitizedUpdates[key]; } }; - ignoreEmptyArrayOverwrite('eventHooks'); + ignoreEmptyArrayOverwrite('eventHooks', { allowEmpty: allowEmptyEventHooks });Then, when the UI intentionally clears hooks (e.g.,
setEventHooks([])), pass__allowEmptyEventHooks: truein the update.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/services/settings-service.ts` around lines 519 - 575, The empty-array guard (ignoreEmptyArrayOverwrite) prevents intentional clearing of eventHooks by deleting sanitizedUpdates.eventHooks when current hooks exist; modify the update flow to honor an explicit escape hatch: treat a boolean flag like sanitizedUpdates.__allowEmptyEventHooks as permission to allow an empty eventHooks array (i.e., skip calling ignoreEmptyArrayOverwrite('eventHooks') when that flag is true), and ensure you remove/delete the __allowEmptyEventHooks flag from sanitizedUpdates before persisting so it isn’t stored. Reference: ignoreEmptyArrayOverwrite, sanitizedUpdates, eventHooks, and the new __allowEmptyEventHooks flag.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/ui/src/store/types/state-types.ts`:
- Line 637: Update the four handlers in
apps/ui/src/components/views/settings-view/event-hooks/event-hooks-section.tsx
(handleDeleteHook, handleToggleHook, handleSaveHook) so they properly await and
handle the Promise returned by setEventHooks (now typed as () => Promise<void>):
either mark each handler async and use await setEventHooks(...) inside try/catch
to surface/log errors, or keep them synchronous but chain
setEventHooks(...).catch(...) to handle failures; ensure you reference the
setEventHooks calls in those handler functions and add consistent error
handling/logging.
---
Outside diff comments:
In `@apps/server/src/services/settings-service.ts`:
- Around line 519-575: The empty-array guard (ignoreEmptyArrayOverwrite)
prevents intentional clearing of eventHooks by deleting
sanitizedUpdates.eventHooks when current hooks exist; modify the update flow to
honor an explicit escape hatch: treat a boolean flag like
sanitizedUpdates.__allowEmptyEventHooks as permission to allow an empty
eventHooks array (i.e., skip calling ignoreEmptyArrayOverwrite('eventHooks')
when that flag is true), and ensure you remove/delete the __allowEmptyEventHooks
flag from sanitizedUpdates before persisting so it isn’t stored. Reference:
ignoreEmptyArrayOverwrite, sanitizedUpdates, eventHooks, and the new
__allowEmptyEventHooks flag.
…d improve error handling in UI
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
apps/ui/src/components/views/settings-view/event-hooks/event-hooks-section.tsx (2)
51-66: Optionally usetoast.promisefor save to provide loading feedback.
handleSaveHookinvolves a server round-trip that may have noticeable latency. Replacing the manual try/catch withtoast.promisegives the user a loading indicator at no extra code cost:✨ Optional refactor using toast.promise
const handleSaveHook = async (hook: EventHook) => { - try { - if (editingHook) { - await setEventHooks(eventHooks.map((h) => (h.id === hook.id ? hook : h))); - } else { - await setEventHooks([...eventHooks, hook]); - } - setDialogOpen(false); - setEditingHook(null); - } catch (error) { - logger.error('Failed to save event hook:', error); - toast.error('Failed to save event hook'); - } + const savePromise = editingHook + ? setEventHooks(eventHooks.map((h) => (h.id === hook.id ? hook : h))) + : setEventHooks([...eventHooks, hook]); + + toast.promise(savePromise, { + loading: 'Saving hook…', + success: () => { + setDialogOpen(false); + setEditingHook(null); + return 'Event hook saved'; + }, + error: (err) => { + logger.error('Failed to save event hook:', err); + return 'Failed to save event hook'; + }, + }); };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/src/components/views/settings-view/event-hooks/event-hooks-section.tsx` around lines 51 - 66, Replace the manual try/catch in handleSaveHook with a toast.promise wrapper to surface loading/success/failure UI states; call toast.promise around the async setEventHooks(...) call(s) (both the update path in handleSaveHook and the add-new path) passing promises that perform setEventHooks, provide appropriate messages for pending/success/error, and keep the existing state updates (setDialogOpen(false); setEditingHook(null)) after the awaited promise resolves; ensure logger.error and toast.error are still invoked on rejection if you need extra logging.
33-49: Consider adding an in-flight guard to prevent concurrent stale-closure writes.All three handlers derive the new state by transforming the
eventHooksvalue captured in the render-time closure. With no loading/pending gate, a second action fired before the firstawait setEventHooks(...)resolves operates on a stale snapshot and will overwrite the first write. For example, a rapid delete-then-toggle can silently resurrect the deleted hook.A simple
isPendingflag that disables the delete/toggle controls is the lightest fix:♻️ Suggested guard pattern
+ const [isPending, setIsPending] = useState(false); const handleDeleteHook = async (hookId: string) => { + if (isPending) return; + setIsPending(true); try { await setEventHooks(eventHooks.filter((h) => h.id !== hookId)); } catch (error) { logger.error('Failed to delete event hook:', error); toast.error('Failed to delete event hook'); + } finally { + setIsPending(false); } }; const handleToggleHook = async (hookId: string, enabled: boolean) => { + if (isPending) return; + setIsPending(true); try { await setEventHooks(eventHooks.map((h) => (h.id === hookId ? { ...h, enabled } : h))); } catch (error) { logger.error('Failed to toggle event hook:', error); toast.error('Failed to update event hook'); + } finally { + setIsPending(false); } };Pass
isPendingintoHookCardand disable the action buttons while pending:<HookCard key={hook.id} hook={hook} onEdit={() => handleEditHook(hook)} onDelete={() => handleDeleteHook(hook.id)} onToggle={(enabled) => handleToggleHook(hook.id, enabled)} + disabled={isPending} />🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/src/components/views/settings-view/event-hooks/event-hooks-section.tsx` around lines 33 - 49, The handlers handleDeleteHook and handleToggleHook operate on a render-closed eventHooks snapshot and can cause stale-overwrites when calls overlap; introduce an isPending (or similar) state, set it true before awaiting setEventHooks and false after, pass isPending into HookCard to disable action buttons while pending, and also switch setEventHooks calls to the functional updater form (prev => prev.filter(...) or prev.map(...)) so updates derive from the latest state if multiple requests happen concurrently.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@apps/ui/src/components/views/settings-view/event-hooks/event-hooks-section.tsx`:
- Around line 33-66: The component handlers (handleDeleteHook, handleToggleHook,
handleSaveHook) call setEventHooks which currently performs an optimistic update
without rollback; add a proper rollback by saving the previous eventHooks state
before calling setEventHooks and restoring it if the server call fails, or
implement the rollback inside setEventHooks itself by capturing get().eventHooks
into a local variable, applying the optimistic set, calling
httpApi.settings.updateGlobal, and on catch restoring the saved previous state
and logging the error (ensure you reference setEventHooks in app-store.ts for
this change).
---
Nitpick comments:
In
`@apps/ui/src/components/views/settings-view/event-hooks/event-hooks-section.tsx`:
- Around line 51-66: Replace the manual try/catch in handleSaveHook with a
toast.promise wrapper to surface loading/success/failure UI states; call
toast.promise around the async setEventHooks(...) call(s) (both the update path
in handleSaveHook and the add-new path) passing promises that perform
setEventHooks, provide appropriate messages for pending/success/error, and keep
the existing state updates (setDialogOpen(false); setEditingHook(null)) after
the awaited promise resolves; ensure logger.error and toast.error are still
invoked on rejection if you need extra logging.
- Around line 33-49: The handlers handleDeleteHook and handleToggleHook operate
on a render-closed eventHooks snapshot and can cause stale-overwrites when calls
overlap; introduce an isPending (or similar) state, set it true before awaiting
setEventHooks and false after, pass isPending into HookCard to disable action
buttons while pending, and also switch setEventHooks calls to the functional
updater form (prev => prev.filter(...) or prev.map(...)) so updates derive from
the latest state if multiple requests happen concurrently.
| const handleDeleteHook = async (hookId: string) => { | ||
| try { | ||
| await setEventHooks(eventHooks.filter((h) => h.id !== hookId)); | ||
| } catch (error) { | ||
| logger.error('Failed to delete event hook:', error); | ||
| toast.error('Failed to delete event hook'); | ||
| } | ||
| }; | ||
|
|
||
| const handleToggleHook = (hookId: string, enabled: boolean) => { | ||
| setEventHooks(eventHooks.map((h) => (h.id === hookId ? { ...h, enabled } : h))); | ||
| const handleToggleHook = async (hookId: string, enabled: boolean) => { | ||
| try { | ||
| await setEventHooks(eventHooks.map((h) => (h.id === hookId ? { ...h, enabled } : h))); | ||
| } catch (error) { | ||
| logger.error('Failed to toggle event hook:', error); | ||
| toast.error('Failed to update event hook'); | ||
| } | ||
| }; | ||
|
|
||
| const handleSaveHook = (hook: EventHook) => { | ||
| if (editingHook) { | ||
| // Update existing | ||
| setEventHooks(eventHooks.map((h) => (h.id === hook.id ? hook : h))); | ||
| } else { | ||
| // Add new | ||
| setEventHooks([...eventHooks, hook]); | ||
| const handleSaveHook = async (hook: EventHook) => { | ||
| try { | ||
| if (editingHook) { | ||
| // Update existing | ||
| await setEventHooks(eventHooks.map((h) => (h.id === hook.id ? hook : h))); | ||
| } else { | ||
| // Add new | ||
| await setEventHooks([...eventHooks, hook]); | ||
| } | ||
| setDialogOpen(false); | ||
| setEditingHook(null); | ||
| } catch (error) { | ||
| logger.error('Failed to save event hook:', error); | ||
| toast.error('Failed to save event hook'); | ||
| } | ||
| setDialogOpen(false); | ||
| setEditingHook(null); | ||
| }; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find app-store.ts
fd -t f 'app-store' --extension tsRepository: AutoMaker-Org/automaker
Length of output: 97
🏁 Script executed:
# Search for setEventHooks implementation
rg -n 'setEventHooks' --type ts -A 30 -B 2 | head -100Repository: AutoMaker-Org/automaker
Length of output: 9928
Implement rollback in setEventHooks on server failure, or restore prior state in each handler.
The setEventHooks implementation in app-store.ts (lines 1418–1426) uses an optimistic-update pattern: it mutates the Zustand store immediately (set({ eventHooks: hooks })) before awaiting the server call (httpApi.settings.updateGlobal). If the server rejects the change, the catch block only logs the error and does not revert the local state.
This means if the network fails or the server returns an error, the UI permanently reflects the mutation (hook deleted/toggled/added) while the server retains the original state—a divergence that persists until the next sync.
Option 1: Implement rollback in the store:
setEventHooks: async (hooks) => {
const previous = get().eventHooks; // Save prior state
set({ eventHooks: hooks });
try {
const httpApi = getHttpApiClient();
await httpApi.settings.updateGlobal({ eventHooks: hooks });
} catch (error) {
logger.error('Failed to sync event hooks:', error);
set({ eventHooks: previous }); // Revert on error
}
},
Option 2: Add explicit rollback in each component handler:
const handleDeleteHook = async (hookId: string) => {
+ const previousHooks = eventHooks;
try {
await setEventHooks(eventHooks.filter((h) => h.id !== hookId));
} catch (error) {
logger.error('Failed to delete event hook:', error);
toast.error('Failed to delete event hook');
+ await setEventHooks(previousHooks).catch(() => {});
}
};🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@apps/ui/src/components/views/settings-view/event-hooks/event-hooks-section.tsx`
around lines 33 - 66, The component handlers (handleDeleteHook,
handleToggleHook, handleSaveHook) call setEventHooks which currently performs an
optimistic update without rollback; add a proper rollback by saving the previous
eventHooks state before calling setEventHooks and restoring it if the server
call fails, or implement the rollback inside setEventHooks itself by capturing
get().eventHooks into a local variable, applying the optimistic set, calling
httpApi.settings.updateGlobal, and on catch restoring the saved previous state
and logging the error (ensure you reference setEventHooks in app-store.ts for
this change).
* Changes from fix/event-hook-persistence * feat: Add explicit permission escape hatch for clearing eventHooks and improve error handling in UI
1 participant
Summary
setEventHooksasync to ensure server persistence when hooks are updatedChanges
eventHooksto theignoreEmptyArrayOverwriteguard to prevent empty arrays from overwriting existing hooks during settings updatessetEventHooksto be async and automatically persist changes to the server via HTTP API, with error logging on failuresetEventHookstype signature fromvoidtoPromise<void>to reflect async behavioreventHooks: []toDEFAULT_GLOBAL_SETTINGSto ensure proper default initializationSummary by CodeRabbit
Bug Fixes
Improvements
New Features
Defaults