Skip to content

Commit 1235b22

Browse files
committed
improvement(ui): remove React anti-patterns, fix CSP violations
1 parent 9f41736 commit 1235b22

8 files changed

Lines changed: 205 additions & 115 deletions

File tree

.claude/commands/you-might-not-need-a-callback.md

Lines changed: 25 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -16,17 +16,34 @@ User arguments: $ARGUMENTS
1616
Read before analyzing:
1717
1. https://react.dev/reference/react/useCallback — official docs on when useCallback is actually needed
1818

19+
## The one rule that matters
20+
21+
`useCallback` is only useful when **something observes the reference**. Ask: does anything care if this function gets a new identity on re-render?
22+
23+
Observers that care about reference stability:
24+
- A `useEffect` that lists the function in its deps array
25+
- A `useMemo` that lists the function in its deps array
26+
- Another `useCallback` that lists the function in its deps array
27+
- A child component wrapped in `React.memo` that receives the function as a prop
28+
29+
If none of those apply — if the function is only called inline, or passed to a non-memoized child, or assigned to a native element event — the reference is unobserved and `useCallback` adds overhead with zero benefit.
30+
1931
## Anti-patterns to detect
2032

21-
1. **useCallback on functions not passed as props or deps**: No benefit if only called within the same component.
22-
2. **useCallback with deps that change every render**: Memoization is wasted.
23-
3. **useCallback on handlers passed to native elements**: `<button onClick={fn}>` doesn't benefit from stable references.
24-
4. **useCallback wrapping functions that return new objects/arrays**: Memoization at the wrong level.
25-
5. **useCallback with empty deps when deps are needed**: Stale closures.
26-
6. **Pairing useCallback + React.memo unnecessarily**: Only optimize when you've measured a problem.
27-
7. **useCallback in hooks that don't need stable references**: Not every hook return needs memoization.
33+
1. **No observer tracks the reference**: The function is only called inline in the same component, or passed to a non-memoized child, or used as a native element handler (`<button onClick={fn}>`). Nothing re-runs or bails out based on reference identity. Remove `useCallback`.
34+
2. **useCallback with deps that change every render**: If a dep is a plain object/array created inline, or state that changes on every interaction, memoization buys nothing — the function gets a new identity anyway.
35+
3. **useCallback on handlers passed only to native elements**: `<button onClick={fn}>` — React never does reference equality on native element props. No benefit.
36+
4. **useCallback wrapping functions that return new objects/arrays**: Stable function identity, unstable return value — memoization is at the wrong level. Use `useMemo` on the return value instead, or restructure.
37+
5. **useCallback with empty deps when deps are needed**: Stale closure — reads initial values forever. This is a correctness bug, not just a performance issue.
38+
6. **Pairing useCallback + React.memo on trivially cheap renders**: If the child renders in < 1ms and re-renders rarely, the memo infrastructure costs more than it saves.
39+
40+
## Patterns that ARE correct — do not flag
2841

29-
Note: This codebase uses a ref pattern for stable callbacks (`useRef` + empty deps). That pattern is correct — don't flag it.
42+
- `useCallback` whose result is in a `useEffect` dep array — prevents the effect from re-running on every render
43+
- `useCallback` whose result is in a `useMemo` dep array — prevents the memo from recomputing on every render
44+
- `useCallback` whose result is a dep of another `useCallback` — stabilises a callback chain
45+
- `useCallback` passed to a `React.memo`-wrapped child — the whole point of the pattern
46+
- This codebase's ref pattern: `useRef` + callback with empty deps that reads the ref inside — correct, do not flag
3047

3148
## Steps
3249

apps/sim/app/workspace/[workspaceId]/components/message-actions/message-actions.tsx

Lines changed: 23 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
'use client'
22

3-
import { useCallback, useEffect, useRef, useState } from 'react'
3+
import { memo, useEffect, useRef, useState } from 'react'
44
import {
55
Button,
66
Check,
@@ -50,7 +50,12 @@ interface MessageActionsProps {
5050
requestId?: string
5151
}
5252

53-
export function MessageActions({ content, chatId, userQuery, requestId }: MessageActionsProps) {
53+
export const MessageActions = memo(function MessageActions({
54+
content,
55+
chatId,
56+
userQuery,
57+
requestId,
58+
}: MessageActionsProps) {
5459
const [copied, setCopied] = useState(false)
5560
const [copiedRequestId, setCopiedRequestId] = useState(false)
5661
const [pendingFeedback, setPendingFeedback] = useState<'up' | 'down' | null>(null)
@@ -70,7 +75,7 @@ export function MessageActions({ content, chatId, userQuery, requestId }: Messag
7075
}
7176
}, [])
7277

73-
const copyToClipboard = useCallback(async () => {
78+
const copyToClipboard = async () => {
7479
if (!content) return
7580
const text = toPlainText(content)
7681
if (!text) return
@@ -84,9 +89,9 @@ export function MessageActions({ content, chatId, userQuery, requestId }: Messag
8489
} catch {
8590
/* clipboard unavailable */
8691
}
87-
}, [content])
92+
}
8893

89-
const copyRequestId = useCallback(async () => {
94+
const copyRequestId = async () => {
9095
if (!requestId) return
9196
try {
9297
await navigator.clipboard.writeText(requestId)
@@ -98,20 +103,17 @@ export function MessageActions({ content, chatId, userQuery, requestId }: Messag
98103
} catch {
99104
/* clipboard unavailable */
100105
}
101-
}, [requestId])
106+
}
102107

103-
const handleFeedbackClick = useCallback(
104-
(type: 'up' | 'down') => {
105-
if (chatId && userQuery) {
106-
setPendingFeedback(type)
107-
setFeedbackText('')
108-
setCopiedRequestId(false)
109-
}
110-
},
111-
[chatId, userQuery]
112-
)
108+
const handleFeedbackClick = (type: 'up' | 'down') => {
109+
if (chatId && userQuery) {
110+
setPendingFeedback(type)
111+
setFeedbackText('')
112+
setCopiedRequestId(false)
113+
}
114+
}
113115

114-
const handleSubmitFeedback = useCallback(() => {
116+
const handleSubmitFeedback = () => {
115117
if (!pendingFeedback || !chatId || !userQuery) return
116118
const text = feedbackText.trim()
117119
if (!text) {
@@ -128,15 +130,15 @@ export function MessageActions({ content, chatId, userQuery, requestId }: Messag
128130
})
129131
setPendingFeedback(null)
130132
setFeedbackText('')
131-
}, [pendingFeedback, chatId, userQuery, content, feedbackText])
133+
}
132134

133-
const handleModalClose = useCallback((open: boolean) => {
135+
const handleModalClose = (open: boolean) => {
134136
if (!open) {
135137
setPendingFeedback(null)
136138
setFeedbackText('')
137139
setCopiedRequestId(false)
138140
}
139-
}, [])
141+
}
140142

141143
if (!content) return null
142144

@@ -224,4 +226,4 @@ export function MessageActions({ content, chatId, userQuery, requestId }: Messag
224226
</Modal>
225227
</>
226228
)
227-
}
229+
})

apps/sim/app/workspace/[workspaceId]/components/resource/components/resource-header/resource-header.tsx

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -40,6 +40,7 @@ export interface HeaderAction {
4040
icon?: React.ElementType
4141
onClick: () => void
4242
disabled?: boolean
43+
active?: boolean
4344
}
4445

4546
export interface CreateAction {
@@ -103,7 +104,12 @@ export const ResourceHeader = memo(function ResourceHeader({
103104
onClick={action.onClick}
104105
disabled={action.disabled}
105106
variant='subtle'
106-
className='px-2 py-1 text-caption'
107+
className={cn(
108+
'px-2 py-1 text-caption',
109+
action.active === true &&
110+
'bg-[var(--surface-active)] hover-hover:bg-[var(--surface-active)]',
111+
action.active === false && 'hover-hover:bg-[var(--surface-hover)]'
112+
)}
107113
>
108114
{ActionIcon && (
109115
<ActionIcon

0 commit comments

Comments
 (0)