Skip to content

fix(chat-deploy): fix launch chat popup and auth persistence, clean up React anti-patterns#3380

Merged
waleedlatif1 merged 5 commits intostagingfrom
waleedlatif1/fix-chat-deploy-bugs
Feb 28, 2026
Merged

fix(chat-deploy): fix launch chat popup and auth persistence, clean up React anti-patterns#3380
waleedlatif1 merged 5 commits intostagingfrom
waleedlatif1/fix-chat-deploy-bugs

Conversation

@waleedlatif1
Copy link
Collaborator

Summary

  • Fix "Launch Chat" button being blocked by popup blockers by pre-opening window before async mutation
  • Fix auth type changes (password → public) not persisting by refetching before form re-initialization
  • Replace useEffect anti-patterns with adjust-state-during-render pattern per React docs
  • Remove duplicate cache invalidations, dead code, and fix timeout memory leaks in deploy modal
  • Fix stale closure in useChatDeploymentInfo refetch callback
  • Rename greenhouse tool files from hyphen-case to snake_case

Type of Change

  • Bug fix

Testing

Tested manually

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 Feb 28, 2026

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

1 Skipped Deployment
Project Deployment Actions Updated (UTC)
docs Skipped Skipped Feb 28, 2026 7:50pm

Request Review

@waleedlatif1 waleedlatif1 force-pushed the waleedlatif1/fix-chat-deploy-bugs branch from 48bef35 to d0dfa43 Compare February 28, 2026 18:52
@waleedlatif1
Copy link
Collaborator Author

@greptile

@waleedlatif1
Copy link
Collaborator Author

@cursor review

@greptile-apps
Copy link
Contributor

greptile-apps bot commented Feb 28, 2026

Greptile Summary

This PR fixes several important bugs related to chat deployment, memory leaks, and React anti-patterns, while also standardizing greenhouse tool file naming.

Key Changes:

  • Popup blocker fix: Pre-opens browser window before async chat creation to prevent popup blockers (only for new chats, not updates)
  • Auth persistence fix: Added formInitCounter to force AuthSelector remount after refetch, ensuring fresh data initialization
  • Stale closure fix: Fixed useChatDeploymentInfo refetch callback to use fresh data instead of stale closure
  • Memory leak fixes: Properly cleaned up setTimeout calls in both copyToClipboard and handleChatDeployed using useEffect cleanup and refs
  • Code cleanup: Removed redundant isDeleting state, simplified form submission logic, and removed duplicate cache invalidations
  • File naming: Standardized greenhouse tool files to snake_case for consistency

Behavior Changes:

  • When updating an existing chat, a new tab is no longer automatically opened (only happens for new chat creation). This appears intentional and improves UX by not opening unnecessary tabs during updates.

Confidence Score: 5/5

  • This PR is safe to merge with minimal risk
  • All changes are bug fixes and code quality improvements with clear intent. The popup blocker fix, memory leak fixes, and stale closure fix are well-implemented. The formInitCounter pattern, while unconventional, correctly forces component remounting to ensure fresh state. No breaking changes or security concerns identified.
  • No files require special attention

Important Files Changed

Filename Overview
apps/sim/app/workspace/[workspaceId]/w/[workflowId]/components/panel/components/deploy/components/deploy-modal/components/chat/chat.tsx Fixed popup blocker issue by pre-opening window, fixed timeout memory leak in copy functionality, and added formInitCounter to force AuthSelector remount after refetch
apps/sim/app/workspace/[workspaceId]/w/[workflowId]/components/panel/components/deploy/components/deploy-modal/deploy-modal.tsx Fixed timeout memory leak by properly cleaning up chatSuccess timeout with useRef, removed dead code from handleChatFormSubmit, and cleaned up cache invalidations
apps/sim/hooks/queries/deployments.ts Fixed stale closure bug in refetch callback by using useCallback and awaiting fresh status result before conditional detailQuery refetch
apps/sim/tools/greenhouse/index.ts Updated import paths to use snake_case file names (get_application, get_candidate, etc.) instead of hyphen-case

Last reviewed commit: 37cd395

Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

16 files reviewed, 4 comments

Edit Code Review Agent Settings | Greptile

@waleedlatif1
Copy link
Collaborator Author

@greptile

@waleedlatif1
Copy link
Collaborator Author

@cursor review

@greptile-apps
Copy link
Contributor

greptile-apps bot commented Feb 28, 2026

Additional Comments (3)

apps/sim/app/workspace/[workspaceId]/w/[workflowId]/components/panel/components/deploy/components/deploy-modal/components/chat/chat.tsx, line 643
setTimeout without cleanup in AuthSelector

The copyToClipboard function schedules a setTimeout but never stores a ref to cancel it. If the component unmounts within 2 seconds of clicking copy (e.g., user closes the modal), the callback attempts to call setCopySuccess on an unmounted component. While React 18 suppresses the warning, this is still a memory leak and the ref is left dangling.

The deploy-modal.tsx already demonstrates the correct pattern with chatSuccessTimeoutRef. Apply the same here:

// Add a ref alongside the copySuccess state:
const copyTimeoutRef = useRef<ReturnType<typeof setTimeout> | null>(null)

const copyToClipboard = (text: string) => {
  navigator.clipboard.writeText(text)
  if (copyTimeoutRef.current) clearTimeout(copyTimeoutRef.current)
  setCopySuccess(true)
  copyTimeoutRef.current = setTimeout(() => setCopySuccess(false), 2000)
}

// cleanup in a useEffect:
useEffect(() => () => { if (copyTimeoutRef.current) clearTimeout(copyTimeoutRef.current) }, [])

apps/sim/hooks/queries/deployments.ts, line 225
refetch wrapper recreated on every render

The refetch function is defined inline in the returned plain object (not memoized), so it gets a new reference on every render of any component consuming useChatDeploymentInfo. In deploy-modal.tsx, handleRefetchChat is wrapped in useCallback([refetchChatInfo]), but since refetchChatInfo is a new reference each render, the useCallback is invalidated on every render, making the memoization effectively a no-op.

Consider memoizing the refetch callback with useCallback inside the hook so its reference is stable:

const refetch = useCallback(async () => {
  const statusResult = await statusQuery.refetch()
  if (statusResult.data?.deployment?.id) {
    await detailQuery.refetch()
  }
}, [statusQuery, detailQuery])

return {
  // ...
  refetch,
}

Since statusQuery.refetch and detailQuery.refetch are stable references provided by TanStack Query, this useCallback will produce a stable function reference.


apps/sim/app/workspace/[workspaceId]/w/[workflowId]/components/panel/components/deploy/components/deploy-modal/components/chat/chat.tsx, line 632
Stale emailItems state after form re-initialization

AuthSelector's internal emailItems state is initialized once from the emails prop via lazy useState, but is never re-synchronized when the emails prop changes after a form re-initialization.

After a successful submit, setHasInitializedForm(false) is called and onRefetchChat() runs. The useEffect resets formData.emails to the server's fresh value — but emailItems inside AuthSelector stays stale because the component isn't remounted (the key prop stays the same for existing chats since the ID hasn't changed).

Concrete scenario:

  1. Existing chat: authType = 'email', emails has entries
  2. User changes to authType = 'public' and saves → form re-initializes with empty formData.emails
  3. User switches back to authType = 'email'
  4. emailItems still shows old entries (stale) but formData.emails is empty
  5. Validation fails with "At least one email is required" even though the tag input appears populated — confusing UX

Consider incorporating a version counter into the AuthSelector key that's bumped whenever setHasInitializedForm(false) is called, forcing the component to remount and reinitialize its internal state from the fresh emails prop.

@waleedlatif1
Copy link
Collaborator Author

@greptile

@waleedlatif1
Copy link
Collaborator Author

@cursor review

@waleedlatif1
Copy link
Collaborator Author

@cursor review

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.

✅ Bugbot reviewed your changes and found no new issues!

Comment @cursor review or bugbot run to trigger another review on this PR

@waleedlatif1 waleedlatif1 merged commit 40bab77 into staging Feb 28, 2026
12 checks passed
@waleedlatif1 waleedlatif1 deleted the waleedlatif1/fix-chat-deploy-bugs branch February 28, 2026 20:01
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