Skip to content

improvement(react): replace unnecessary useEffect patterns with better React primitives#3675

Merged
waleedlatif1 merged 7 commits intostagingfrom
waleedlatif1/fix-changecolor-ref-detach
Mar 19, 2026
Merged

improvement(react): replace unnecessary useEffect patterns with better React primitives#3675
waleedlatif1 merged 7 commits intostagingfrom
waleedlatif1/fix-changecolor-ref-detach

Conversation

@waleedlatif1
Copy link
Collaborator

Summary

  • Fix ColorGrid focus-stealing bug in color picker (hex input lost focus on every keystroke)
  • Replace ~25 unnecessary useEffects across the codebase with better React patterns:
    • Derived state via useMemo/inline computation (auth forms, document tags, combobox)
    • Event handler callbacks instead of effects (chat scroll reset, modal resets, sidebar nav, voice interface ref sync)
    • Render-time state adjustment for prop-to-state sync (date picker, time picker, credentials, profile picture)
    • Lazy initializers for initial state from URL params (login, signup)
  • Audited all changes with 10 parallel agents for correctness and safety

Type of Change

  • Bug fix
  • Refactor

Testing

Tested manually. TypeScript passes with 0 errors. Lint passes clean.

Checklist

  • Code follows project style guidelines
  • Self-reviewed my changes
  • Tests added/updated and passing
  • No new warnings introduced
  • I confirm that I have read and agree to the terms outlined in the Contributor License Agreement (CLA)

@vercel
Copy link

vercel bot commented Mar 19, 2026

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

1 Skipped Deployment
Project Deployment Actions Updated (UTC)
docs Skipped Skipped Mar 19, 2026 7:36pm

Request Review

@cursor
Copy link

cursor bot commented Mar 19, 2026

PR Summary

Medium Risk
Mostly refactors to remove useEffect-driven derived state, but it touches auth redirect handling and several interactive UI components (modals, sidebar navigation, voice/chat input), which can introduce subtle behavior regressions.

Overview
Reduces reliance on useEffect for derived state across auth and UI components by switching to lazy state initializers, useMemo, and refs/callback helpers (e.g., login/signup URL param handling, reset-password token error display).

Fixes several interaction edge cases: chat input now resizes on change and focuses via requestAnimationFrame, voice interface keeps state/mute refs in sync via updateState/updateIsMuted, and multiple modals/menus reset state more deterministically (upload modal close guard, help modal reset helper, create-workspace autofocus).

Improves menu UX/accessibility by preventing color picker focus from stealing input, adding max-height constraints to dropdown content, and hardening Popover open/close state resets while forwarding onOpenChange.

Written by Cursor Bugbot for commit 4860ff8. Configure here.

@greptile-apps
Copy link
Contributor

greptile-apps bot commented Mar 19, 2026

Greptile Summary

This PR replaces ~25 useEffect patterns across 16 files with more appropriate React primitives. The majority of changes are correct and represent real improvements: lazy initializers for URL param state, useMemo for derived values, useCallback wrappers to keep state and refs in sync atomically, and the React-recommended render-time state adjustment pattern for prop-to-state sync in date-picker and time-picker. The headline bug fix (ColorGrid focus stealing on hex input) is correctly solved by making the submenu uncontrolled and scoping the focus effect to component mount.

Key observations:

  • permissions-table.tsx: The useRef mutation in the render body (hasLoadedOnceRef.current = true) is unsafe in concurrent mode — React can invoke and discard renders, causing the ref to be set from a non-committed pass. The original useEffect correctly ran post-commit.
  • login-form.tsx: logger.warn(...) is called directly in the render body, which is a side effect that can fire from discarded/aborted renders in concurrent mode. The invalidCallbackRef guard mitigates duplicate logs but doesn't solve the timing issue.
  • chat/input.tsx: Moving adjustTextareaHeight() into handleInputChange runs it before React commits the new inputValue to the DOM. For user typing this is typically fine, but pasting or programmatic content updates may yield a stale height until the next interaction.
  • popover.tsx: The change from open === false to !open in the reset effect causes it to fire on initial mount when open is undefined; while currently a no-op, it is a semantic shift from the original intent.

Confidence Score: 4/5

  • Safe to merge with minor follow-up recommended for the concurrent-mode ref mutation in permissions-table.
  • The large majority of changes are correct refactors that improve performance and readability. The three flagged issues (ref mutation during render, logger side effect in render, textarea height timing) are subtle and unlikely to cause visible regressions under normal usage, but the permissions-table pattern is technically incorrect for React concurrent mode and should be addressed.
  • apps/sim/app/workspace/[workspaceId]/w/components/sidebar/components/workspace-header/components/invite-modal/components/permissions-table.tsx — ref mutation in render body; apps/sim/app/(auth)/login/login-form.tsx — logger.warn side effect during render

Important Files Changed

Filename Overview
apps/sim/app/(auth)/login/login-form.tsx Replaces useEffect for searchParams reading with lazy initializers and inline derivation; adds a logger.warn call directly in the render body guarded by a ref, which is a minor side-effect-during-render concern in concurrent mode.
apps/sim/app/chat/components/input/input.tsx Removes useEffect for textarea height adjustment, moving adjustTextareaHeight() into handleInputChange (pre-render) and requestAnimationFrame in handleActivate; height adjustment now runs before the DOM reflects the latest state for some input scenarios.
apps/sim/app/chat/components/voice-interface/voice-interface.tsx Introduces updateState/updateIsMuted callbacks to keep refs in sync when state changes; all setState call sites updated correctly, eliminating two useEffect syncs without introducing stale closures.
apps/sim/app/workspace/[workspaceId]/w/components/sidebar/components/workspace-header/components/invite-modal/components/permissions-table.tsx Swaps useState+useEffect "has loaded once" flag for a ref mutation in the render body; this pattern is unsafe in concurrent mode since React can invoke and discard renders, leaving the ref set from a non-committed pass.
apps/sim/components/emcn/components/popover/popover.tsx Extracts resetState callback and adds handleOpenChange to reset hover state on open; subtle behavioral change from open === false to !open causes resetState() to fire on initial mount when open is undefined, though this is a no-op in practice.
apps/sim/app/workspace/[workspaceId]/w/components/sidebar/components/workflow-list/components/context-menu/context-menu.tsx Fixes the focus-stealing bug by lifting buttonRefs out of ColorGrid, making the submenu uncontrolled, and running the focus-on-mount effect once (which is correct since ColorGrid unmounts/remounts with each submenu open).

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[useEffect Pattern] --> B{Pattern Type}

    B -->|Derived from props/params| C[Lazy initializer / useMemo / inline]
    B -->|Sync ref with state| D[updateState / updateIsMuted callbacks]
    B -->|Prop-to-state sync| E[Render-time state adjustment\nprevValue sentinel]
    B -->|Event-driven reset| F[useCallback reset + onOpenChange]
    B -->|Focus management| G[requestAnimationFrame /\nonOpenAutoFocus prop]
    B -->|Track loaded-once flag| H[useRef mutation in render body]

    C -->|login, signup, reset-password,\ncombobox| C1[✅ Clean: runs at correct time,\nno extra renders]
    D -->|voice-interface| D1[✅ Ref always in sync;\nall setState call sites updated]
    E -->|date-picker, time-picker| E1[✅ React-recommended pattern;\nbatched state updates]
    F -->|popover, add-documents-modal,\nhelp-modal| F1[⚠️ open === false → !open\nfires on undefined too]
    G -->|chat/input, create-workspace-modal| G1[⚠️ adjustTextareaHeight runs\nbefore DOM update]
    H -->|permissions-table| H1[⚠️ Concurrent mode: discarded\nrenders can set ref prematurely]
Loading

Comments Outside Diff (1)

  1. apps/sim/components/emcn/components/popover/popover.tsx, line 923-925 (link)

    !open changes reset behaviour when open is undefined

    The original condition was open === false, which meant the reset only ran in strictly controlled mode after an explicit close. The new !open is falsy for both false and undefined. In uncontrolled usage (where open is never passed), open starts as undefined on mount, so resetState() is called immediately on the first effect run. While resetState() is a no-op when state is already at default values (all primitives/null), the condition change is a semantic shift that could cause unexpected resets if the component is ever used in a partially-controlled way where open is undefined transiently.

    If the intent is to reset only on explicit close, restore the strict equality check:

Last reviewed commit: "fix(add-documents-mo..."

@waleedlatif1
Copy link
Collaborator Author

@cursor review

@waleedlatif1
Copy link
Collaborator Author

@greptile

- Modals (create-workspace, rename-document, edit-knowledge-base): restore
  useEffect watching `open` prop for form reset on programmatic open, since
  Radix onOpenChange doesn't fire for parent-driven prop changes
- Popover: add useEffect watching `open` for programmatic close reset
- Chat scroll: restore useEffect watching `isStreamingResponse` so the 1s
  suppression timer starts when streaming begins, not before the fetch
- Credentials manager: revert render-time pattern to useEffect for initial
  sync from cached React Query data (useRef captures initial value, making
  the !== check always false on mount)
@waleedlatif1
Copy link
Collaborator Author

@cursor review

@waleedlatif1
Copy link
Collaborator Author

@greptile

…reset

- Help modal: restore useEffect watching `open` for form reset on
  programmatic open (same Radix onOpenChange pattern as other modals)
- Invite modal: restore useEffect watching `open` to clear error on
  programmatic open
- Combobox: restore useEffect to reset highlightedIndex when filtered
  options shrink (prevents stale index from reappearing when options grow)
- Remove no-op handleOpenChange wrappers in rename-document and
  edit-knowledge-base modals (now pure pass-throughs after useEffect fix)
@waleedlatif1
Copy link
Collaborator Author

@greptile

@waleedlatif1
Copy link
Collaborator Author

@cursor review

…ove no-op wrapper in create-workspace-modal

- ColorGrid: replaced setTimeout with requestAnimationFrame for initial
  focus to wait for submenu paint completion
- create-workspace-modal: removed handleOpenChange pass-through wrapper,
  use onOpenChange directly
@waleedlatif1
Copy link
Collaborator Author

@greptile

@waleedlatif1
Copy link
Collaborator Author

@cursor review

… refetch

The useEffect that sets previewMode should only run when selectedFileId
changes, not when files array reference changes from React Query refetch.
Restores the filesRef pattern to read latest files without triggering
the effect — prevents overriding user's manual mode selection.
…, fix combobox dep array

- add-documents-modal: handleOpenChange(true) is dead code in Radix
  controlled mode — restored useEffect watching open for reset-on-open
- combobox: depend on filteredOptions array (not .length) so highlight
  resets when items change even with same count
@waleedlatif1
Copy link
Collaborator Author

Addressing review concerns (4860ff8)

1. add-documents-modal.tsx — Fixed (real bug)

The handleOpenChange(true) branch was dead code. In Radix controlled Dialog, onOpenChange only fires for user-initiated interactions (Escape, overlay click) — never with true when the parent programmatically sets open={true}. Restored a useEffect watching open for the reset-on-open logic, kept handleOpenChange solely for the close-with-upload-guard path.

2. files.tsx — Already fixed (7dc680e)

The filesRef pattern was restored in the previous round. The effect only runs when selectedFileId changes — background React Query refetches no longer override the user's manual preview/editor mode selection.

3. combobox.tsx — Fixed (real bug)

Changed the dependency from filteredOptions.length back to filteredOptions (the array reference). With .length only, when the user types and the filtered items change to a different set with the same count, the effect wouldn't fire — leaving the keyboard highlight silently pointing at a different item at the same index position.

@waleedlatif1
Copy link
Collaborator Author

@cursor review

@waleedlatif1
Copy link
Collaborator Author

@greptile

Copy link

@cursor cursor bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes and found 2 potential issues.

Fix All in Cursor

Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

@waleedlatif1 waleedlatif1 merged commit c3c22e4 into staging Mar 19, 2026
12 checks passed
@waleedlatif1 waleedlatif1 deleted the waleedlatif1/fix-changecolor-ref-detach branch March 19, 2026 19:57
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