refactor: completed state refactor moving major context to their dedicated Zustand stores#393
refactor: completed state refactor moving major context to their dedicated Zustand stores#393reachrazamair merged 4 commits intomainfrom
Conversation
… store Replace ToastContext (React Context) with notificationStore (Zustand) for toast notification state. notifyToast() is now a stable module-level function callable from React and non-React code, eliminating the addToastRef pattern. - Create notificationStore with toast queue, config, and notifyToast() wrapper - Wire Toast.tsx, DebugPackageModal, QuickActionsModal, useAgentListeners, App.tsx - Remove ToastProvider from main.tsx, delete ToastContext.tsx - 74 store tests + 26 component tests (100 total), full suite green (19,097 tests)
…th Zustand store Final store migration (11/11). Replaces the 2,088-line useSettings hook with a Zustand store containing 65 state fields, 80+ actions, and a batch loadAllSettings() function. The useSettings adapter preserves the UseSettingsReturn interface unchanged so zero consumer changes are needed. Eliminates 3 useRef + 3 sync useEffects by using get() in store actions. Adds 119 new store tests and store reset to existing test files for Zustand singleton compatibility.
…on and hover bugs Merge 112 commits from main into code-refactor. Resolve useSettings.ts conflict (3 regions) preserving thin Zustand adapter pattern. Integrate 3 new settings (directorNotesSettings, wakatimeApiKey, wakatimeEnabled) into settingsStore. Convert incoming addToast call to notifyToast for notificationStore consistency. Fix unused React imports from incoming History components. Additionally fix two file explorer bugs: - Selection now tracks item identity (fullPath) instead of positional index, so expanding/collapsing folders no longer jumps selection to an unrelated item - Hover highlighting works by removing inline backgroundColor override that had higher specificity than Tailwind's hover pseudo-class
Code Review - PR #393: Zustand Store RefactorComprehensive review of the state management refactor moving ToastContext and useSettings internals to Zustand stores. Executive SummaryThis refactor significantly improves the architecture by consolidating React hooks into centralized Zustand stores, reducing the Overall Assessment: ⭐⭐⭐⭐ (4/5) - Approve with required changes Critical Issues 🔴1. Memory Leak in notificationStore.ts (Lines 259-263)Problem: Auto-dismiss timeouts are never cleaned up if toast is manually dismissed. // Current implementation
if (!toastsDisabled && durationMs > 0) {
setTimeout(() => {
useNotificationStore.getState().removeToast(id);
}, durationMs);
}Required Fix: // Store timeout IDs and clear on remove
const timeoutIds = new Map<string, NodeJS.Timeout>();
export function notifyToast(toast: Omit<Toast, 'id' | 'timestamp'>): string {
// ... existing code ...
if (!toastsDisabled && durationMs > 0) {
const timeoutId = setTimeout(() => {
timeoutIds.delete(id);
useNotificationStore.getState().removeToast(id);
}, durationMs);
timeoutIds.set(id, timeoutId);
}
return id;
}
// Update removeToast action
removeToast: (id) => {
const timeoutId = timeoutIds.get(id);
if (timeoutId) {
clearTimeout(timeoutId);
timeoutIds.delete(id);
}
set((s) => ({ toasts: s.toasts.filter((t) => t.id !== id) }));
},2. Async Setter Race Conditions in settingsStore.tsProblem: State updates immediately but persistence could fail, leaving store/disk out of sync. Affected methods (lines 749, 754, 759):
Required Fix: setPreventSleepEnabled: async (value) => {
try {
await window.maestro.settings.set('preventSleepEnabled', value);
await window.maestro.power.setEnabled(value);
set({ preventSleepEnabled: value }); // Only update on success
} catch (error) {
console.error('[Settings] Failed to set preventSleep:', error);
throw error; // Let Sentry capture
}
},3. Port Validation Logic Flaw (Lines 654-661)Problems:
Current implementation: setWebInterfaceCustomPort: (value) => {
set({ webInterfaceCustomPort: value }); // Allows invalid values
if (value >= 1024 && value <= 65535) {
window.maestro.settings.set('webInterfaceCustomPort', value);
}
},Required Fix - Always clamp: setWebInterfaceCustomPort: (value) => {
const clamped = Math.max(1024, Math.min(65535, value));
set({ webInterfaceCustomPort: clamped });
window.maestro.settings.set('webInterfaceCustomPort', clamped);
},4. Migration Applied Multiple Times (Lines 1451-1464)Problem: Required Fix: if (!allSettings['concurrentAutoRunTimeMigrationApplied'] && stats.cumulativeTimeMs > 0) {
// Write flag FIRST before stats to prevent duplicate runs
await window.maestro.settings.set('concurrentAutoRunTimeMigrationApplied', true);
const THREE_HOURS_MS = 3 * 60 * 60 * 1000;
stats = {
...stats,
cumulativeTimeMs: stats.cumulativeTimeMs + THREE_HOURS_MS,
};
window.maestro.settings.set('autoRunStats', stats);
}Strengths ✅notificationStore.ts
settingsStore.ts
useSettings.ts
Toast.tsx
Medium Priority Issues 🟡Error Handling in IPC Calls (settingsStore.ts)Lines 830-838 and similar patterns update store state regardless of IPC success. Add try/catch or let errors bubble to Sentry: try {
window.maestro.settings.set('usageStats', updated);
set({ usageStats: updated });
} catch (error) {
console.error('[Settings] Failed to persist usageStats:', error);
// Optionally rollback state or notify user
}Security Considerations 🔒Good Practices ✅
Verify:
Performance Analysis 🚀Improvements
ConsiderationLarge store size: settingsStore is ~1,700 lines. Consider future split into domain-specific stores (e.g., Testing Recommendations 🧪Critical Test Cases (verify these are covered)
Test files exist at:
Action: Confirm critical scenarios above are covered. Nice to Have 💡
RecommendationApprove with required changes. The refactor is well-executed with significant architectural and performance improvements. The critical issues are straightforward to fix and don't undermine the core design. Once timeout cleanup, async setter handling, port validation, and migration atomicity are addressed, this is merge-ready. Scores:
|
… to setPreventSleepEnabled Address PR review feedback: - notificationStore: track auto-dismiss timeouts in a Map, cancel on removeToast/clearToasts to avoid dangling timers - settingsStore: setPreventSleepEnabled now rolls back optimistic state update if the power IPC call fails, keeping UI in sync with OS state
Code Review: Zustand State RefactoringI've completed a comprehensive review of this PR. Overall, this is an excellent refactoring that significantly improves the codebase architecture. The migration from Context-based patterns to Zustand stores is well-executed with thorough testing. Summary✅ 9,800 additions, 7,606 deletions What Was ReviewedArchitecture & Design ⭐⭐⭐⭐⭐
Key Strengths
Code Quality AssessmentnotificationStore.ts ⭐⭐⭐⭐⭐Excellent implementation:
Side effects properly handled: // All side effects in wrapper function, not store reducers
export async function notifyToast(opts) {
// Generate ID, add to queue
store.getState().addToast(toast);
// Side effects AFTER state update
await logToast(toast);
await speakNotification(toast);
await showOsNotification(toast);
scheduleAutoDismiss(toast);
}settingsStore.ts ⭐⭐⭐⭐½Very strong implementation:
Excellent features: setPreventSleepEnabled: async (value) => {
const prev = get().preventSleepEnabled;
set({ preventSleepEnabled: value });
try {
await window.maestro.settings.set('preventSleepEnabled', value);
await window.maestro.power.setEnabled(value);
} catch (error) {
// Rollback on failure - UI stays in sync
set({ preventSleepEnabled: prev });
throw error; // Let Sentry capture
}
}Issues & Recommendations🟡 Minor Issues (Not Blocking)
✅ No Critical IssuesAll identified issues are:
TestingTest Execution✅ 19,054 tests passing (per PR description) Manual Testing Recommendations
Security Considerations✅ No security concerns identified:
Performance ImpactPositive Changes✅ Startup performance: Reduced from ~60 IPC calls to 1 batch load Potential Optimizations (Follow-up)
Comparison to CLAUDE.md Guidelines✅ Error handling: Allows exceptions to bubble to Sentry (except where explicitly caught) Final Verdict✅ APPROVED - Ready to MergeThis is a high-quality refactoring that represents a significant architectural improvement:
Recommended Next Steps
Great work on this refactoring! The codebase is significantly improved, and the migration is exemplary in its thoroughness and attention to detail. Review conducted by Claude Code with comprehensive analysis of:
|
1 participant
Summary
ToastContext. Consolidates toast queue management, notification configuration, and side effects (logging, audio TTS, OS notifications, auto-dismiss) into a module-levelnotifyToast()wrapper. Eliminates theaddToastRefpattern from App.tsx (~47 call sites replaced with directnotifyToast()calls).ToastContext.tsxdeleted.useSettingshook. All 100+ settings now live in Zustand withgetState()/setState()access outside React. TheuseSettingshook is retained as a thin adapter that delegates to the store, preserving the existing API contract for all consumers. No component changes required.Details
notificationStore
notifyToast()exported at module level — handles side effects outside React lifecycleToast.tsx,DebugPackageModal.tsx,QuickActionsModal.tsx,useAgentListeners.ts,App.tsxSessionProviderwrapper removed frommain.tsxsettingsStore
useSettingshook reduced to a thin adapter (~80 lines) that subscribes to the storeuseSettingsStore.getState()Test plan
npx tsc --noEmit)