Skip to content

fix: resolve infinite re-render loop by optimizing Zustand selectors - #1248

Merged
LiorAgnin merged 5 commits into
qafrom
hotfix/homepage-infinite-rerender-loop
Jan 15, 2026
Merged

fix: resolve infinite re-render loop by optimizing Zustand selectors#1248
LiorAgnin merged 5 commits into
qafrom
hotfix/homepage-infinite-rerender-loop

Conversation

@LiorAgnin

@LiorAgnin LiorAgnin commented Jan 15, 2026

Copy link
Copy Markdown
Contributor
  • Replace shallow with useShallow hook wrapper (Zustand v5 requirement)
  • Select primitives instead of arrays/objects where possible:
    • TurnkeyProvider: select credentialId string only
    • app/_layout: select hasSelectedUser boolean
    • useCardStatus: select userId string only
  • Fix optional chaining in useActivity.ts (transactionsRef.current?.withdraws)
  • Remove anti-pattern ref mutation in usePostSignupInit useMemo

Root cause: Immer (used by Zustand persist) creates new object references
on every state update, causing React to re-render components that select
arrays/objects without proper equality functions.

LiorAgnin and others added 2 commits January 15, 2026 10:57
Apply the same ref + JSON.stringify pattern to prevent infinite
re-renders caused by React Query returning new object references
even when data is identical.

This is an additional fix to address the remaining React error #185
(Maximum update depth exceeded) that was not fully resolved by the
previous fix in useFinancial.ts and useDepositCalculations.ts.

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Replace shallow with useShallow hook wrapper (Zustand v5 requirement)
- Select primitives instead of arrays/objects where possible:
  - TurnkeyProvider: select credentialId string only
  - app/_layout: select hasSelectedUser boolean
  - useCardStatus: select userId string only
- Fix optional chaining in useActivity.ts (transactionsRef.current?.withdraws)
- Remove anti-pattern ref mutation in usePostSignupInit useMemo

Root cause: Immer (used by Zustand persist) creates new object references
on every state update, causing React to re-render components that select
arrays/objects without proper equality functions.
@vercel

vercel Bot commented Jan 15, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

2 Skipped Deployments
Project Deployment Review Updated (UTC)
solid-app Ignored Ignored Preview Jan 15, 2026 1:16pm
solid-app-staging Ignored Ignored Preview Jan 15, 2026 1:16pm

@LiorAgnin LiorAgnin changed the title Hotfix/homepage infinite rerender loop fix: resolve infinite re-render loop by optimizing Zustand selectors Jan 15, 2026
@claude

claude Bot commented Jan 15, 2026

Copy link
Copy Markdown

Pull Request Review: Fix Infinite Re-render Loop

Summary

This PR addresses a critical performance issue caused by Zustand + Immer creating new object references on every state update, triggering unnecessary re-renders. The solution correctly migrates to useShallow and optimizes selectors to extract primitives instead of objects/arrays.


Strengths

1. Root Cause Analysis

The PR description clearly identifies the problem: Immer (used by Zustand persist) creates new object references, breaking React shallow equality checks. This is excellent documentation.

2. Correct Use of useShallow

All migrations to useShallow are properly implemented:

  • TurnkeyProvider.tsx: Extracts credentialId string only
  • app/_layout.tsx: Uses hasSelectedUser boolean instead of user object
  • app/signup/creating.tsx: Selects hasUsers boolean and methods
  • app/(protected)/_layout.tsx: Derives usersCount instead of array

3. Smart Stabilization Pattern in useActivity.ts

The ref-based stabilization using JSON comparison (lines 109-120) is a clever solution for unstable React Query references. This prevents infinite loops while maintaining data freshness.

4. Anti-pattern Removal

Removing the ref mutation in usePostSignupInit.ts useMemo is correct - refs should not be mutated during render.


Issues and Concerns

1. CRITICAL: Verify Zustand v5 Migration Complete

The PR mentions Zustand v5 requirement. Verify no other files still use the old v4 shallow import pattern:

  • v4: import { shallow } from zustand/shallow
  • v5: import { useShallow } from zustand/react/shallow

Fixed in: useUser.ts, TurnkeyProvider.tsx, app/(protected)/_layout.tsx, app/signup/creating.tsx

2. Potential Bug: Optional Chaining Inconsistency (useActivity.ts:196)

Implementation is inconsistent with optional chaining. If transactionsRef.current can be undefined, always use optional chaining consistently throughout.

3. Performance: JSON.stringify in Hot Path (useActivity.ts:113)

Using JSON.stringify on potentially large transaction objects in a useMemo dependency can be expensive. Monitor performance with realistic data volumes. Consider using a hash function or deep-equal library if performance becomes an issue.

4. Test Coverage Gap

No test changes accompany this critical performance fix. Recommended additions:

  • Unit test verifying selectors return stable references for unchanged data
  • Integration test ensuring no infinite render loops
  • Test for the ref stabilization pattern in useActivity.ts

Security

No security concerns identified. Changes are performance optimizations without security implications.


Performance Impact

Expected Improvements:

  • Eliminates infinite re-render loops
  • Reduces unnecessary component re-renders
  • More efficient selector memoization

Potential Regressions:

  • JSON.stringify overhead (monitor in production)

Recommendations

High Priority:

  1. Verify no remaining v4 shallow imports across the codebase
  2. Fix optional chaining consistency in useActivity.ts
  3. Add tests for selector stability and re-render prevention

Medium Priority:
4. Profile JSON.stringify performance with realistic data volumes
5. Document the ref stabilization pattern - its non-obvious and valuable

Low Priority:
6. Consider extracting the ref stabilization pattern into a custom hook for reuse


Final Verdict: Approve with Minor Fixes

This is a well-researched and correctly implemented fix for a critical performance issue. The changes demonstrate strong understanding of React rendering, Zustand internals, and Immer behavior.

Required before merge:

  • Fix optional chaining consistency in useActivity.ts
  • Verify no remaining v4 shallow imports

Recommended before merge:

  • Add basic test coverage for selector stability

Great work identifying and solving this subtle but impactful bug!

@claude

claude Bot commented Jan 15, 2026

Copy link
Copy Markdown

Pull Request Review: Fix infinite re-render loop by optimizing Zustand selectors

Overview

This PR addresses a critical performance issue - infinite re-render loops caused by unstable object/array references in Zustand selectors when using Immer with the persist middleware. The approach is solid and follows React best practices.

✅ Strengths

1. Root Cause Analysis

The PR description correctly identifies that Immer (used by Zustand persist) creates new object references on every state update, causing components selecting arrays/objects to re-render unnecessarily. This is a well-understood performance issue.

2. Correct Use of useShallow

The implementation properly uses useShallow from zustand/react/shallow to prevent re-renders when selecting objects/arrays:

  • app/(protected)/_layout.tsx:34-35: Selects usersCount (primitive) instead of users array
  • components/TurnkeyProvider.tsx:19-22: Selects only credentialId string instead of entire user object
  • hooks/useUser.ts:77: Uses useShallow with .find() to stabilize the selected user reference

3. Smart Optimizations

  • TurnkeyProvider (components/TurnkeyProvider.tsx:19-22): Selecting only credentialId is excellent - this component only needs this single primitive value, not the entire user object
  • useCardStatus (hooks/useCardStatus.ts:18): Selecting only selectedUserId instead of the entire user object is a good optimization
  • Protected Layout (app/(protected)/_layout.tsx:34-35): Computing usersCount in the selector is smart - the component only cares about the length for conditional rendering

4. Innovative Solution in useActivity

The useActivity.ts changes (lines 113-122) show a creative approach to stabilize React Query data:

const transactionsRef = useRef(userTransactions);
const transactionsKey = useMemo(() => JSON.stringify(userTransactions ?? null), [userTransactions]);
useEffect(() => {
  transactionsRef.current = userTransactions;
}, [transactionsKey, userTransactions]);

This prevents infinite loops when React Query returns new object references with identical data.

⚠️ Issues & Concerns

1. Critical: Unrelated Analytics Change (Blocker)

File: lib/analytics.ts:65

- const sampleRate = 1; // Record all user sessions
+ const sampleRate = 0.2;

Issue: This changes the Amplitude session replay sampling rate from 100% to 20%. This is completely unrelated to the infinite re-render fix and should not be in this PR.

Impact:

  • This is a significant product/analytics decision, not a bug fix
  • It changes user session recording behavior in production
  • It should have its own PR with proper discussion about why sampling is being reduced

Recommendation: Remove this change from the PR. If sampling rate adjustment is needed, create a separate PR with justification.

2. Security: Removed Safe Address Sync Logic (High Priority)

File: hooks/usePostSignupInit.ts:31-47 (deleted)

Issue: The PR removes an entire section that syncs the safe address to the backend:

// Removed code:
if (user.safeAddress && !safeAddressSynced[user.userId]) {
  await withRefreshToken(() => updateSafeAddress(user.safeAddress));
  markSafeAddressSynced(user.userId);
}

Questions:

  1. Why was this removed? Is safe address syncing happening elsewhere now?
  2. The PR description mentions "Remove anti-pattern ref mutation in usePostSignupInit useMemo" but the removed code isn't in a useMemo
  3. Is safeAddressSynced state still needed in useUserStore.ts:14?

Recommendation:

  • Clarify why this was removed and ensure safe address syncing still happens
  • If this logic is moved elsewhere, add a code comment explaining where
  • Consider removing safeAddressSynced from the store if no longer needed

3. Potential Bug: ESLint Disable Without Full Justification

Files: hooks/useActivity.ts:242-243

// eslint-disable-next-line react-hooks/exhaustive-deps -- transactionsKey is intentional: stable JSON key replaces unstable object reference
}, [events, user?.userId, transactionsKey]);

Issue: The comment explains transactionsKey but transactionsRef is also used in the dependency array's closure (line 191, 197). The ref pattern is correct, but the justification could be clearer.

Recommendation: Update the comment to explain both the transactionsKey AND the ref pattern:

// eslint-disable-next-line react-hooks/exhaustive-deps -- Using transactionsKey (stable JSON) instead of userTransactions (unstable ref) + accessing via ref to avoid dependency

4. Code Quality: Inconsistent Selector Patterns

File: app/welcome.tsx:22

const users = useUserStore(state => state.users);

Issue: This file still selects the full users array directly without useShallow. While users are needed for rendering, this creates inconsistency with other files that were optimized.

Question: Does welcome.tsx need the full array for rendering the user list, or could it select a stable primitive (like user count) and derive the list?

Recommendation:

  • If the full array is needed for mapping over users, add a comment explaining why useShallow isn't used
  • Consider whether this could be optimized similarly to other files

5. Missing Information: Test Coverage

Issue: The PR doesn't show test updates or additions.

Recommendations:

  1. Add a test that verifies components don't re-render unnecessarily when Zustand state updates
  2. Test that the useActivity hook doesn't cause infinite loops with React Query updates
  3. Add integration test for the selector optimizations

Example test approach:

it('should not re-render when unrelated Zustand state changes', () => {
  const { result, rerender } = renderHook(() => useCardStatus());
  const renderCount = useRef(0);
  renderCount.current++;
  // Update unrelated user state
  act(() => useUserStore.getState().storeUser(differentUser));
  expect(renderCount.current).toBe(1); // Should not re-render
});

📊 Performance Considerations

Positive Impacts:

  1. Reduced re-renders: Components will only re-render when their specific selected data changes
  2. Memory efficiency: Not creating closures for entire objects when only primitives are needed
  3. Stability: The useActivity stabilization prevents React Query from triggering cascading updates

Potential Concerns:

  1. JSON.stringify overhead (useActivity.ts:115): Serializing potentially large transaction objects on every render could be expensive
    • Recommendation: Consider using a stable hash or deep-equal comparison library if userTransactions is large

🔒 Security Considerations

  1. Removed backend sync: The removal of safe address syncing logic (point design card screens #2 above) needs verification
  2. No security vulnerabilities introduced: The selector optimizations themselves don't introduce security issues
  3. Data integrity: Ensure that selecting primitives doesn't lose important user data references needed for authorization checks

🎯 Recommendations Summary

Must Fix (Blocking):

  1. Remove the unrelated sampleRate change from lib/analytics.ts
  2. Clarify why safe address syncing was removed and ensure it happens elsewhere

Should Fix (High Priority):

  1. ⚠️ Add test coverage for re-render prevention
  2. ⚠️ Clarify the ESLint disable comment in useActivity.ts
  3. ⚠️ Document why welcome.tsx doesn't use the optimization pattern

Consider:

  1. 💭 Evaluate JSON.stringify performance impact with large datasets
  2. 💭 Remove safeAddressSynced from store if no longer needed
  3. 💭 Add performance monitoring to track re-render improvements

📝 Code Review Checklist

  • ✅ Code quality and best practices: Good use of hooks and selectors
  • ✅ Logic is correct: Selector optimizations are sound
  • ❌ No unrelated changes: Analytics change should be removed
  • ⚠️ Security concerns: Safe address sync removal needs explanation
  • ❌ Test coverage: Tests needed for re-render prevention
  • ✅ Performance: Significant improvement expected
  • ✅ Documentation: PR description is thorough

Overall Assessment

Status: ⚠️ Needs Changes Before Merge

The core solution is excellent and will significantly improve performance. However, the unrelated analytics change must be removed, and the safe address sync removal needs clarification. Once these issues are addressed, this will be a solid improvement to the codebase.

Estimated Impact: This fix should eliminate the infinite re-render loops and noticeably improve app responsiveness, especially on the homepage and activity screens.

Add useShallow wrapper to prevent infinite re-renders (React error #185)
caused by Zustand returning new object references on every state update.

Fixed components:
- Modal providers (8 files): currentModal, transaction objects
- Token selectors (4 files): selectedToken object
- BankTransfer components (7 instances): kyc, bankTransfer objects
- Hooks: useSendOption, useWithdrawOption with multiple objects
- Other: country_selection, CheckConnectionWrapper, DepositDirectly
@claude

claude Bot commented Jan 15, 2026

Copy link
Copy Markdown

PR Review: Fix infinite re-render loop by optimizing Zustand selectors

Summary

This PR addresses React Error #185 (Maximum update depth exceeded) by optimizing Zustand selectors to prevent unnecessary re-renders caused by Immer creating new object references on every state update. The changes are well-targeted and follow best practices for Zustand v5.


✅ Code Quality & Best Practices

Strengths:

  1. Consistent pattern application: The useShallow wrapper is applied consistently across all 36 affected files with clear explanatory comments
  2. Strategic primitive selection: Where possible, the PR selects primitives instead of objects (e.g., credentialId string only in TurnkeyProvider.tsx:19-23, hasSelectedUser boolean in app/_layout.tsx:34, userId string in useCardStatus.ts)
  3. Excellent commit messages: Clear, detailed explanations of the root cause and specific fixes applied
  4. Backward compatibility: Changes maintain existing functionality while fixing the performance issue

Suggestions for improvement:

  1. Import organization: Consider grouping all zustand imports together for consistency:
    // Good pattern seen in most files
    import { useShallow } from 'zustand/react/shallow';
  2. Comment consistency: Some files have verbose comments while others are minimal. Consider standardizing:
    // Preferred: Brief and consistent
    // Use useShallow to prevent re-renders from Immer object references

🐛 Potential Issues

Minor concerns:

  1. hooks/useActivity.ts:99-101 - The useShallow usage for array comparison is correct, but the comment on line 258 could be clearer:

    // Current comment is verbose - consider simplifying to:
    // eslint-disable-next-line react-hooks/exhaustive-deps -- withdrawsKey prevents unnecessary re-renders
  2. hooks/usePostSignupInit.ts - Excellent cleanup! The removal of ref mutation anti-pattern (lines 17-18 using useRef + lastUserId) is the correct approach. However, verify that hasInitialized.current on line 29 properly prevents duplicate runs across component remounts.

  3. components/TurnkeyProvider.tsx:19-23 - Good optimization selecting only credentialId, but the inline selector logic could be extracted for readability:

    const selectedCredentialId = useUserStore(state => {
      const selectedUser = state.users.find(u => u.selected) ?? 
        (state.users.length === 1 ? state.users[0] : undefined);
      return selectedUser?.credentialId;
    });

    Consider: Moving this logic to a separate selector function in the store for reusability and testability.

  4. app/_layout.tsx:34 - Selecting usersCount: state.users.length instead of the users array is smart, but line 147 still checks usersCount && !user. Ensure user from useUser() hook is stable to avoid re-render loops.


⚡ Performance Considerations

Excellent improvements:

  1. hooks/useActivity.ts:122-137 - The lightweight withdrawsKey approach is much better than JSON.stringify for large objects. The sampling strategy (first 3 transactions) is smart for change detection.

  2. Reduced re-render cascade: By selecting primitives and using useShallow, the PR prevents cascading re-renders across the 20+ components that use these stores.

  3. Memory efficiency: The changes reduce memory pressure by preventing unnecessary object allocations during renders.

Suggestions:

  1. lib/analytics.ts:65 - Reducing Amplitude session sample rate from 100% to 20% will significantly reduce analytics costs and network usage. ✅ Good operational decision. Consider:

    • Documenting this decision in code comments or a separate analytics configuration file
    • Adding environment-based configuration (e.g., 100% for staging, 20% for production)
  2. Consider memoization: For components with complex derived state, consider using useMemo in addition to useShallow:

    const derivedValue = useMemo(() => {
      // expensive computation
    }, [stableRef]);

🔒 Security Concerns

No significant security issues identified. The changes are purely performance-related and don't introduce new attack vectors.

Minor note:

  • Ensure that the useShallow implementation correctly handles sensitive data (like credentials) without exposing it in different contexts. Current implementation appears safe.

🧪 Test Coverage

Critical gap identified:

  1. No automated tests: The repository has minimal test coverage (only __tests__/sanity.test.ts and 4 Playwright e2e specs). This PR would benefit from:

    • Unit tests for the new selector patterns
    • Integration tests verifying the re-render fix
    • Performance regression tests to catch future issues
  2. Recommended test additions:

    // Example test for useShallow selector
    describe('useUserStore with useShallow', () => {
      it('should not trigger re-render when irrelevant state changes', () => {
        // Test that selecting credentialId doesn't re-render when other user props change
      });
    });
  3. E2E verification: Manually verify in browser DevTools that:

    • React DevTools Profiler shows reduced render counts
    • No "Maximum update depth exceeded" errors in console
    • User flows (login, transactions, etc.) work correctly

📋 Additional Recommendations

  1. Documentation: Add a section to the codebase documenting the Zustand + Immer pattern and when to use useShallow:

    ## Zustand State Management Guidelines
    
    - Always use `useShallow` when selecting objects/arrays from persisted stores
    - Prefer selecting primitives over objects when possible
    - Use inline selectors for simple cases, extract to store for complex logic
  2. ESLint rule: Consider adding an ESLint rule to catch missing useShallow usage:

    // Custom rule to warn when selecting objects without useShallow
  3. Performance monitoring: Add Sentry performance monitoring to track render counts and catch future regression:

    import * as Sentry from '@sentry/react-native';
    
    // In critical components
    Sentry.addBreadcrumb({
      category: 'render',
      message: 'Component rendered',
      level: 'info',
    });
  4. Type safety: Consider creating a typed useShallowStore helper:

    export const useShallowStore = <T, U>(
      store: UseBoundStore<StoreApi<T>>,
      selector: (state: T) => U
    ) => store(useShallow(selector));

✨ Summary

Verdict: ✅ Approve with minor suggestions

This is a high-quality PR that systematically addresses a critical performance issue. The changes are:

  • Well-researched and targeted
  • Consistently applied across the codebase
  • Properly documented in commit messages
  • Following React and Zustand best practices

Before merging:

  1. ✅ Verify manually that infinite re-render loops are resolved
  2. ⚠️ Add basic tests for critical selector patterns (recommended)
  3. ✅ Confirm analytics sample rate change is intentional
  4. ✅ Update documentation with new patterns

Estimated impact:

Great work on identifying and systematically fixing this issue! 🎉


Files reviewed: 36 files changed (+389/-112)
Review focus: Code quality, performance, security, test coverage
Reviewer: Claude Code Assistant

…nders

Wrap all Zustand store object destructuring with useShallow to fix React
error #185 (infinite re-render loops). When destructuring like
const { a, b } = useStore(), a new object reference is created on every
state change, causing components to re-render infinitely.

Changes:
- Add useShallow wrapper to ~55 files (components, hooks, app routes)
- Simplify single-property selectors to direct selectors
- Add ESLint rule to enforce useShallow for object-returning selectors
- Add useRenderMonitor hook for production re-render detection
- Export selector functions from useUserStore for reusability
@LiorAgnin
LiorAgnin merged commit 1b6db1a into qa Jan 15, 2026
5 checks passed
@LiorAgnin
LiorAgnin deleted the hotfix/homepage-infinite-rerender-loop branch January 15, 2026 13:17
@claude

claude Bot commented Jan 15, 2026

Copy link
Copy Markdown

PR Review: Fix Infinite Re-render Loop by Optimizing Zustand Selectors

Overview

This PR addresses a critical performance issue (React error #185 - infinite re-render loops) by properly implementing Zustand v5's useShallow wrapper for object/array selectors. The root cause is that Immer (used by Zustand persist) creates new object references on every state update, causing unnecessary re-renders.

✅ Strengths

1. Correct Root Cause Analysis

  • The PR correctly identifies that Immer creates new references, requiring shallow equality checks
  • The fix aligns with Zustand v5 best practices for preventing unnecessary re-renders

2. Comprehensive Coverage

  • 55+ files updated systematically
  • Covers components, hooks, and app routes
  • Consistent application of the pattern across the codebase

3. Performance Monitoring

  • Added useRenderMonitor hook for production render tracking via Sentry
  • Proactive monitoring prevents future regressions
  • Includes configurable thresholds and cooldown periods

4. ESLint Rule Addition

  • Smart addition of ESLint rule to catch future violations
  • Warning level is appropriate (not blocking)
  • Clear error message guides developers

5. Selector Function Exports

  • Good addition of reusable selectors like selectSelectedUser and selectSelectedCredentialId
  • Promotes consistency and reduces duplication

🔍 Issues & Concerns

CRITICAL: useRenderMonitor Implementation Bug

Location: hooks/useRenderMonitor.ts:44-46

if (isActive) {
  renderCountRef.current += 1;
}

Issue: Mutating refs during render phase is an anti-pattern and violates React's rules. While refs are mutable, incrementing during render can cause issues with:

  • React's concurrent rendering
  • Strict mode double-invocations
  • Potential race conditions

Fix: Move the increment to useEffect:

useEffect(() => {
  if (!isActive) return;
  renderCountRef.current += 1;
});

SECURITY: Missing Input Validation

Location: hooks/useRenderMonitor.ts:31-35

The componentName parameter should be validated to prevent Sentry abuse:

export const useRenderMonitor = ({
  componentName,
  warnThreshold = 10,
  trackInDev = false,
}: RenderMonitorOptions) => {
  // Add validation
  if (!componentName || typeof componentName !== 'string') {
    console.error('[useRenderMonitor] Invalid componentName');
    return;
  }

MODERATE: ESLint Rule Limitations

Location: eslint.config.js:34-40

The current ESLint rule only catches inline object expressions:

selector: 'CallExpression[callee.name=/^use.*Store$/] > ArrowFunctionExpression > ObjectExpression'

Misses:

  1. Multi-line selectors where object is returned without explicit return
  2. Selectors that return arrays: useStore(state => [state.a, state.b])
  3. Destructuring: const { a, b } = useStore(selector) where selector returns object

Recommendation: Consider adding additional rules or document these edge cases.

MODERATE: Incomplete Migration

Location: hooks/useActivity.ts:99-100

const userEventsFromStore = useActivityStore(
  useShallow(state => (user?.userId ? state.events[user.userId] : undefined)),

Issue: Selecting arrays with useShallow still causes re-renders if the array contents change (new references). The PR description mentions using refs + JSON.stringify pattern in some places but not here consistently.

Question: Should this follow the same pattern as in the commit 94c2c6f ("stabilize userTransactions reference")?

MINOR: Inconsistent Pattern Usage

Some files use primitives (optimal):

// ✅ Good - app/_layout.tsx:34-35
const { usersCount, _hasHydrated } = useUserStore(
  useShallow(state => ({ usersCount: state.users.length, _hasHydrated: state._hasHydrated })),
);

Others use objects even for single properties:

// ⚠️ Could be simpler - components/coins/[id].tsx:44-50
const { selectedTime, selectedPrice, selectedPriceChange } = useCoinStore(
  useShallow(state => ({
    selectedTime: state.selectedTime,
    selectedPrice: state.selectedPrice,
    selectedPriceChange: state.selectedPriceChange,
  })),
);

Recommendation: Document when to use useShallow vs direct selection in a style guide.

MINOR: usePostSignupInit Refactor

Location: hooks/usePostSignupInit.ts:1-87

Good removal of the useMemo anti-pattern! However:

useEffect(() => {
  if (!user || user.userId === lastUserId.current) return;
  lastUserId.current = user.userId;
  // ...
}, [user?.userId]);

Issue: The dependency array [user?.userId] is correct, but setting lastUserId.current = user.userId at the start means if the effect runs twice quickly, the second run will skip due to the ref check, even if needed.

Consider: Moving ref update to end of async operation or using a proper loading flag.

🧪 Testing Concerns

Missing Tests

  1. ❌ No unit tests for useRenderMonitor hook
  2. ❌ No tests verifying the re-render fixes work
  3. ❌ No tests for the ESLint rule

Recommendation: Add tests for:

  • useRenderMonitor render counting accuracy
  • Sentry integration mocking
  • ESLint rule pattern matching

Manual Testing

  • ✅ E2E tests exist (playwright)
  • ❓ Has the infinite re-render been manually verified as fixed?
  • ❓ Has performance been measured before/after?

📊 Performance Considerations

Positive Impact

  • Eliminating infinite re-renders is a major performance win
  • Proper selector usage reduces unnecessary component updates
  • useRenderMonitor overhead is minimal (only in production by default)

Potential Concerns

  • useShallow adds a shallow comparison on every selector call (negligible cost)
  • 55+ files changed increases bundle size slightly (more function wrappers)
  • useRenderMonitor creates 3 refs per monitored component

🔒 Security Assessment

Low Risk

  • No XSS, SQL injection, or authentication vulnerabilities introduced
  • No sensitive data exposure
  • Sentry integration properly scoped

Considerations

  • Ensure Sentry rate limiting is configured server-side
  • useRenderMonitor could theoretically spam Sentry if exploited (see input validation above)

📝 Best Practices

Follows:

  • ✅ Zustand v5 migration guide recommendations
  • ✅ React hooks rules (mostly - see useRenderMonitor issue)
  • ✅ Consistent code style
  • ✅ Detailed commit messages

Could Improve:

  • Add JSDoc comments to complex selectors
  • Create a migration guide document for team reference
  • Add performance benchmarks in commit message

📋 Recommendations

Before Merge (Required):

  1. Fix useRenderMonitor ref mutation - Move increment to useEffect
  2. Add input validation to useRenderMonitor
  3. Verify manual testing - Confirm infinite re-render is resolved
  4. Document ESLint rule limitations - Add comment about edge cases

Before Merge (Nice to Have):

  1. Consider adding tests for useRenderMonitor
  2. Document the useShallow pattern in a style guide
  3. Review arrays in useActivity.ts for consistency with ref pattern

Post-Merge:

  1. Monitor Sentry for render warnings in production
  2. Create follow-up ticket for comprehensive test coverage
  3. Consider TypeScript utility type for enforcing shallow wrappers

🎯 Overall Assessment

Recommendation: APPROVE with minor changes required

This is a well-executed fix for a critical performance issue. The systematic approach, monitoring addition, and ESLint rule make this a solid foundation. The useRenderMonitor bug must be fixed before merge, but otherwise this is production-ready.

Impact: 🟢 High positive impact - Fixes critical UX issue
Risk: 🟡 Low-Medium - One critical bug to fix, otherwise safe
Code Quality: 🟢 Good - Systematic, consistent, well-documented


Great work on tackling this complex refactor! The addition of proactive monitoring shows excellent engineering judgment. 🚀

Generated with Claude Code

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant