Skip to content

Commit 6fd1767

Browse files
authored
improvement(ui): remove React anti-patterns, fix CSP violations (#4203)
* improvement(ui): remove React anti-patterns, fix CSP violations * fix(ui): restore useMemo on existingKeys — it is observed by useAvailableResources * improvement(ui): add RefreshCw icon, update Bell SVG, active state styling for header actions * minor UI improvements
1 parent 6aa6346 commit 6fd1767

File tree

13 files changed

+250
-123
lines changed

13 files changed

+250
-123
lines changed

.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/inline-rename-input.tsx

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -25,14 +25,15 @@ export function InlineRenameInput({ value, onChange, onSubmit, onCancel }: Inlin
2525
ref={inputRef}
2626
type='text'
2727
value={value}
28+
size={Math.max(value.length + 2, 5)}
2829
onChange={(e) => onChange(e.target.value)}
2930
onKeyDown={(e) => {
3031
if (e.key === 'Enter') onSubmit()
3132
if (e.key === 'Escape') onCancel()
3233
}}
3334
onBlur={onSubmit}
3435
onClick={(e) => e.stopPropagation()}
35-
className='min-w-0 flex-1 truncate border-0 bg-transparent p-0 font-medium text-[var(--text-body)] text-sm outline-none focus:outline-none focus:ring-0'
36+
className='min-w-0 border-0 bg-transparent p-0 font-medium text-[var(--text-body)] text-sm outline-none focus:outline-none focus:ring-0'
3637
/>
3738
)
3839
}

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: 8 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,13 @@ 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 !== undefined && 'rounded-lg',
110+
action.active === true &&
111+
'bg-[var(--surface-active)] hover-hover:bg-[var(--surface-active)]',
112+
action.active === false && 'hover-hover:bg-[var(--surface-hover)]'
113+
)}
107114
>
108115
{ActionIcon && (
109116
<ActionIcon

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -222,7 +222,7 @@ const SortDropdown = memo(function SortDropdown({ config }: { config: SortConfig
222222
Sort
223223
</Button>
224224
</DropdownMenuTrigger>
225-
<DropdownMenuContent align='end'>
225+
<DropdownMenuContent align='start'>
226226
{options.map((option) => {
227227
const isActive = active?.column === option.id
228228
const Icon = option.icon

0 commit comments

Comments
 (0)