refactor: useEffect audit and best practices improvements#15
Conversation
- Replace prop→state sync with useControllableState in EditableName - Add loading/error states to useSiteInfo hook - Replace defaultValues + useEffect reset pattern with `values` prop in 11 form files (react-hook-form best practice) - Add justification comments to legitimate useEffect usages - Fix double-reset bug in GeneralSettings form Affected forms: - orders/ui-settings-form.tsx - logs/ui-settings-form.tsx - reports/ui-settings-form.tsx - pos/cart/ui-settings-form.tsx - customers/ui-settings-form.tsx - settings/tax.tsx - settings/barcode-scanning/settings.tsx - settings/general.tsx - orders/edit/form.tsx - pos/cart/edit-cart-customer.tsx - pos/cart/buttons/edit-order-meta/form.tsx
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 WalkthroughWalkthroughRefactors multiple forms to bind external data via Changes
Sequence DiagramssequenceDiagram
participant User as User
participant Form as React Hook Form
participant Hook as useFormChangeHandler
participant Debounce as Debounce Handler
participant Persist as onChange Callback
rect rgba(150, 100, 200, 0.5)
Note over User,Form: User edits a form field
end
User->>Form: Edit field value
Form->>Hook: watch subscription emits field change
alt Programmatic reset (isResettingRef = true)
Hook->>Hook: Ignore change
else User-initiated change
Hook->>Hook: Build changes object for changed path
alt debounceMs > 0
Hook->>Debounce: Schedule debounced call (debounceMs)
Debounce->>Persist: onChange(changes) after debounce
else
Hook->>Debounce: Flush pending debounced calls
Hook->>Persist: onChange(changes) immediately
end
end
sequenceDiagram
participant Component as Consumer
participant Hook as useSiteInfo
participant HTTP as HTTP Client
participant API as Backend API
participant Site as Site Store
rect rgba(100, 150, 200, 0.5)
Note over Component,Hook: Hook invoked on mount / site change
end
Component->>Hook: useSiteInfo({ site })
Hook->>Hook: setIsLoading(true), setError(null)
rect rgba(100, 200, 150, 0.5)
Note over Hook,API: Fetch site info using wpApiUrl
end
Hook->>HTTP: http.get(wpApiUrl)
HTTP->>API: GET /wp-json/wcpos/v1/system-info
API-->>HTTP: Response (status, data)
HTTP-->>Hook: Response received
alt 2xx & valid data
Hook->>Site: site.incrementalPatch({ wp_version, wc_version, ... })
else invalid status or data
Hook->>Hook: setError(errorMsg)
end
Hook->>Hook: setIsLoading(false)
Hook-->>Component: return { isLoading, error }
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 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. 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 |
useControllableState wasn't suitable here because this component has special semantics: - Maintains internal editing buffer - Only notifies parent on submit (blur/enter), not every keystroke - Needs to sync from prop only when NOT editing
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/core/src/screens/main/settings/tax.tsx (1)
133-135: Replace console.error with project logger.Use
@wcpos/utils/loggerfor structured logging and consistency. As per coding guidelines, please avoid console usage.🛠️ Proposed fix
import { View } from 'react-native'; +import log from '@wcpos/utils/logger'; } catch (error) { - console.error(error); + log.error('Failed to restore server settings', { context: { error } }); } finally {packages/components/src/form/use-form-change-handler.ts (1)
68-91: WraporiginalReset.apply()in try/finally to ensure flag clears even if reset throws.If
form.resetthrows an error beforequeueMicrotaskis called,isResettingRef.currentremains true permanently, blocking all futureonChangeevents from firing (see line 100 in the watch subscription). This prevents form changes from persisting.Proposed fix
form.reset = (...args) => { isResettingRef.current = true; - const result = originalReset.apply(form, args); - // Reset the flag after a microtask to ensure watch has fired - queueMicrotask(() => { - isResettingRef.current = false; - }); - return result; + try { + return originalReset.apply(form, args); + } finally { + // Reset the flag after a microtask to ensure watch has fired + queueMicrotask(() => { + isResettingRef.current = false; + }); + } };
Summary
Comprehensive
useEffectaudit following React best practices from the codebase conventions.Changes
useControllableStatehookisLoadinganderrorreturn values for better UXdefaultValues+useEffect(() => form.reset())withvaluesprop (react-hook-form best practice) in 11 form filesuseEffectusagesWhy
valuesinstead ofdefaultValues+ reset?defaultValuesonly initializes once - async data arrives after initializationvaluesprop makes the form reactive to external data changes@FIXMEremoved)Test Plan
Settings Forms (highest risk)
UI Settings Forms
Order/Customer Forms
Component Changes
Edge Cases
Files Changed
input/index.tsx,loader/index.tsx,textarea/index.tsx,use-form-change-handler.tsuse-site-info.ts,use-online-status.web.tsx,use-query.tseditable-name.tsx,add-customer.tsx, 6xui-settings-form.tsx,edit-cart-customer.tsx,edit/form.tsx,edit-order-meta/form.tsx,general.tsx,tax.tsx,barcode-scanning/settings.tsxSummary by CodeRabbit
New Features
Improvements
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.