-
Notifications
You must be signed in to change notification settings - Fork 20
Fix feed fidget settings resetting when switching fidgets #1646
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: canary
Are you sure you want to change the base?
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 📝 WalkthroughWalkthroughThe changes introduce an activation gate mechanism to prevent stale updates in multi-instance FidgetSettingsEditor scenarios, add a default-value merging helper function, and simplify a feed data effect's dependency array by removing settings. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/common/components/organisms/FidgetSettingsEditor.tsx (3)
181-193: Consider more robust type handling infillWithDefaults.The current logic for determining whether a value exists treats all non-string types uniformly (line 190). This means:
- Empty arrays
[]or empty objects{}are preserved instead of falling back to defaults- Boolean
falseand number0are correctly preserved (likely intended)If field values can be arrays or objects, consider adding specific checks for empty collections, or ensure this behavior is intentional.
🔎 Example: Enhanced type handling
const fillWithDefaults = (input: FidgetSettings) => properties.fields.reduce((acc, field) => { const value = input && typeof input === "object" ? (input as any)[field.fieldName] : undefined; const hasValue = value !== undefined && value !== null && - (typeof value !== "string" || value.trim() !== ""); + (typeof value !== "string" || value.trim() !== "") && + (!Array.isArray(value) || value.length > 0) && + (typeof value !== "object" || Object.keys(value).length > 0 || typeof value === "boolean"); acc[field.fieldName] = hasValue ? value : field.default ?? ""; return acc; }, {} as FidgetSettings);Note: Adjust based on your field types and intended behavior for empty collections.
199-201: Potential unnecessary re-renders fromproperties.fieldsdependency.The effect depends on
properties.fields, which could trigger re-renders if the parent component doesn't memoize thepropertiesobject. While this doesn't break functionality, it may cause the editor to re-sync state more frequently than necessary.Consider requesting that parent components memoize the
propertiesprop, or use a deep comparison forproperties.fieldsif this becomes a performance concern.
135-136: LGTM! Guards effectively prevent cross-instance state pollution.The
isActiveguard at the point of state update ensures that only the currently active editor instance can modify state. The inline callback creates a new function on each render, but the performance impact is negligible for this simple comparison.Optional: Memoize isActive callback
If you prefer to avoid creating new functions on every render:
+ const isActive = useCallback( + () => activeIdRef.current === fidgetId, + [fidgetId] + ); <FidgetSettingsGroup ... - isActive={() => activeIdRef.current === fidgetId} + isActive={isActive} />Also applies to: 143-143, 155-155, 264-265, 275-276, 287-288
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/common/components/organisms/FidgetSettingsEditor.tsxsrc/fidgets/farcaster/Feed.tsx
🧰 Additional context used
🧬 Code graph analysis (1)
src/common/components/organisms/FidgetSettingsEditor.tsx (2)
src/common/fidgets/index.d.ts (1)
FidgetSettings(15-17)src/common/lib/hooks/useUIColors.ts (1)
useUIColors(15-49)
🔇 Additional comments (2)
src/common/components/organisms/FidgetSettingsEditor.tsx (1)
207-219: LGTM! Effective guard mechanism against stale updates.The
safeOnSavewrapper and the guard in_onSaveproperly prevent updates from inactive editor instances. Combined withfillWithDefaults, this ensures that only the active editor's state—with all defaults applied—is saved to the parent.src/fidgets/farcaster/Feed.tsx (1)
452-457: LGTM! Correctly removes unnecessary dependency.The effect body doesn't reference
settings, so removing it from the dependency array is correct. This prevents the thread stack from being cleared and the initial hash from being re-pushed when settings change, which aligns with the PR objective of preventing feed settings from resetting when switching fidgets.
…lidation and settings management - Introduced filter validation in FidgetSettingsEditor to ensure valid feed filters before saving. - Added normalization for filter types to default to 'Users' if an invalid type is provided. - Updated FidgetWrapper to maintain local settings overrides and synchronize settings panel with the latest configurations. - Refactored onSave logic to handle local settings and unselection more effectively.
Summary by CodeRabbit
Release Notes
✏️ Tip: You can customize this high-level summary in your review settings.