-
-
Notifications
You must be signed in to change notification settings - Fork 866
fix #2629: avoid multiple rerenders when tags is an array #2624
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: main
Are you sure you want to change the base?
fix #2629: avoid multiple rerenders when tags is an array #2624
Conversation
|
WalkthroughThe change modifies the Estimated code review effortπ― 2 (Simple) | β±οΈ ~10 minutes Pre-merge checks and finishing touchesβ Passed checks (3 passed)
β¨ 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 |
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 (2)
packages/react-hooks/src/hooks/useRealtime.ts (2)
431-433: Stable tag normalization looks good!The approach of sorting and stringifying array tags creates a deterministic dependency key that prevents re-renders when tag order changes. For non-array tags, the value passes through unchanged.
Optional: Consider adding a comment for clarity.
A brief comment explaining why
stableTagis needed would help future maintainers understand this pattern:+ // Normalize array tags to prevent re-renders when order changes. + // Arrays with the same elements in different orders produce the same stableTag. const stableTag = Array.isArray(tag) ? JSON.stringify([...tag].sort()) : tag;
434-465: Consider includingtagin the dependency array to avoid stale closures.The callback uses
tagat line 444 but doesn't include it in the dependency array. While the current implementation works (assuming the API treats tag arrays as unordered sets), includingtagin the dependency array would follow React best practices and avoid potential stale closure issues.Apply this change to include
tagin the dependency array:- }, [stableTag, mutateRuns, runsRef, abortControllerRef, apiClient, setError]); + }, [tag, stableTag, mutateRuns, runsRef, abortControllerRef, apiClient, setError]);How this works together:
triggerRequestgets recreated whenevertagchanges (by reference), ensuring it always has the current value- The effect (line 477) still uses
stableTag, so it only runs when the set of tags changes, not when the order changes- This prevents infinite re-renders while avoiding stale closures
π Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
π Files selected for processing (1)
packages/react-hooks/src/hooks/useRealtime.ts(3 hunks)
π§° Additional context used
π Path-based instructions (1)
**/*.{ts,tsx}
π CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{ts,tsx}: Always prefer using isomorphic code like fetch, ReadableStream, etc. instead of Node.js specific code
For TypeScript, we usually use types over interfaces
Avoid enums
No default exports, use function declarations
Files:
packages/react-hooks/src/hooks/useRealtime.ts
π Additional comments (1)
packages/react-hooks/src/hooks/useRealtime.ts (1)
467-477: Excellent fix for the infinite re-render issue!Using
stableTaginstead oftagin the effect dependency array is the key change that prevents unnecessary re-subscriptions when tag arrays change by reference but contain the same elements (possibly in different order). The effect will only re-run when the actual set of tags changes, not when the array reference changes.
|
NOTE: temporary fixed it by memoizing my tags. Altho it works I dont think it should be the real fix as it is forcing your users to unnecessarily memoize their tags |
β Checklist
π§ͺ Testing
I ran the project locally and confirmed that infinite re-renders are no longer happening.
The subscription now initializes once and remains stable, even when using tag arrays.
π§Ύ Changelog
Fixed an issue causing infinite re-renders by ensuring the
tagdependency in theuseRealtimeRunsWithTaghook is stable.React was previously interpreting a new array reference each render (due to shallow comparison), which triggered unnecessary re-subscriptions.
πΈ Screenshots
No visual changes β purely internal stability improvement.
π―
fixes #2629