Skip to content

⚡ Bolt: optimize SearchInterface with useCallback for event handlers #336

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
daggerstuff wants to merge 1 commit into
base: staging
Choose a base branch
from
Open

⚡ Bolt: optimize SearchInterface with useCallback for event handlers #336

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 17 additions & 7 deletions src/components/research/SearchFilters.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,13 +1,23 @@
import { render, screen, fireEvent } from '@testing-library/react'
import React from 'react'
// @vitest-environment jsdom
/**
* @vitest-environment jsdom
*/
import { describe, it, expect, vi } from 'vitest'

import SearchFilters, { type SearchFiltersState } from './SearchFilters'

// Setup Mock for onChange
const mockOnChange = vi.fn()

import { afterEach } from 'vitest'
import { cleanup } from '@testing-library/react'

afterEach(() => {
cleanup()
vi.clearAllMocks()
})

const defaultFilters: SearchFiltersState = {
topics: [],
minRelevance: 0,
Expand All @@ -26,11 +36,11 @@ describe('SearchFilters', () => {
it('renders all filter sections', () => {
render(<SearchFilters filters={defaultFilters} onChange={mockOnChange} />)

expect(screen.getByText('Advanced Filters')).toBeInTheDocument()
expect(screen.getByLabelText('Year From')).toBeInTheDocument()
expect(screen.getByLabelText('Year To')).toBeInTheDocument()
expect(screen.getByText('Therapeutic Topics')).toBeInTheDocument()
expect(screen.getByText('Min Relevance Score')).toBeInTheDocument()
expect(screen.getByText('Advanced Filters')).not.toBeNull()
expect(screen.getByLabelText('Year From')).not.toBeNull()
expect(screen.getByLabelText('Year To')).not.toBeNull()
expect(screen.getByText('Therapeutic Topics')).not.toBeNull()
expect(screen.getByText('Min Relevance Score')).not.toBeNull()
Comment on lines 37 to +43
Copy link
Copy Markdown

@charliecreates charliecreates bot Mar 31, 2026

Choose a reason for hiding this comment

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

These assertions are redundant and (as written) weaker than the previous jest-dom versions:

  • getByText/getByLabelText already throw if not found, so expect(...).not.toBeNull() adds no coverage.
  • The swap from toHaveAttribute/toBeInTheDocument reduces readability and intent.

If jest-dom matchers were removed to “make the test pass”, that’s treating the symptom—better to fix the test setup so you can keep expressive assertions.

Suggestion

Either remove the redundant expectations entirely, or restore the stronger jest-dom matchers by ensuring @testing-library/jest-dom/vitest is loaded in your Vitest setup.

Option A (remove redundancy):

render(<SearchFilters filters={defaultFilters} onChange={mockOnChange} />)

screen.getByText('Advanced Filters')
screen.getByLabelText('Year From')
screen.getByLabelText('Year To')
screen.getByText('Therapeutic Topics')
screen.getByText('Min Relevance Score')

Reply with "@CharlieHelps yes please" if you’d like me to add a commit with this suggestion.

})

it('toggles topics correctly', () => {
Expand All @@ -43,7 +53,7 @@ describe('SearchFilters', () => {
// AND handleApply calls onChange. But wait, toggleTopic updates local state.
// We verify the button indicates it is pressed or selected visually (class check or aria-pressed).
// After clicking, it should be pressed (true)
expect(topicButton).toHaveAttribute('aria-pressed', 'true')
expect(topicButton.getAttribute('aria-pressed')).toBe('true')
})

it('calls onChange with new filters when Apply is clicked', () => {
Expand Down
124 changes: 67 additions & 57 deletions src/components/research/SearchInterface.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import { motion } from 'framer-motion'
import React, { useState } from 'react'
import React, { useState, useCallback } from 'react'

import { researchAPI, type BookMetadata } from '@/lib/api/research'

Expand Down Expand Up @@ -30,62 +30,72 @@ export default function SearchInterface() {
// Export State
const [showExport, setShowExport] = useState(false)

const executeSearch = async (
currentQuery: string,
currentSources: string[],
currentFilters: SearchFiltersState,
) => {
if (!currentQuery.trim()) return

setLoading(true)
setHasSearched(true)
setError(null)
setResults([])

try {
// Track search event
void researchAPI.trackEvent('search_literature', {
query: currentQuery,
sources: currentSources,
filter_count:
(currentFilters.topics.length || 0) +
(currentFilters.yearFrom ? 1 : 0) +
(currentFilters.yearTo ? 1 : 0),
})

const data = await researchAPI.searchLiterature({
q: currentQuery,
limit: 12,
sources: currentSources.includes('all') ? undefined : currentSources,
year_from: currentFilters.yearFrom,
year_to: currentFilters.yearTo,
min_relevance: currentFilters.minRelevance,
topics: currentFilters.topics,
sort_by: currentFilters.sortBy,
})

setResults(data.results)
} catch (err: any) {
console.error('Search error:', err)
setError(err.message || 'Failed to fetch results.')
} finally {
setLoading(false)
}
}

const handleSearch = (e: React.FormEvent) => {
e.preventDefault()
void executeSearch(query, selectedSources, filters)
}

const handleFilterChange = (newFilters: SearchFiltersState) => {
setFilters(newFilters)
setShowFilters(false)
// Auto-refresh search if we already have a query
if (query.trim()) {
void executeSearch(query, selectedSources, newFilters)
}
}
// ⚡ Bolt: wrap executeSearch and related event handlers in useCallback to prevent unnecessary re-creations and re-renders of child components
const executeSearch = useCallback(
async (
currentQuery: string,
currentSources: string[],
currentFilters: SearchFiltersState,
) => {
if (!currentQuery.trim()) return

setLoading(true)
setHasSearched(true)
setError(null)
setResults([])

try {
// Track search event
void researchAPI.trackEvent('search_literature', {
query: currentQuery,
sources: currentSources,
filter_count:
(currentFilters.topics.length || 0) +
(currentFilters.yearFrom ? 1 : 0) +
(currentFilters.yearTo ? 1 : 0),
})

const data = await researchAPI.searchLiterature({
q: currentQuery,
limit: 12,
sources: currentSources.includes('all') ? undefined : currentSources,
year_from: currentFilters.yearFrom,
year_to: currentFilters.yearTo,
min_relevance: currentFilters.minRelevance,
topics: currentFilters.topics,
sort_by: currentFilters.sortBy,
})

setResults(data.results)
} catch (err: any) {
console.error('Search error:', err)
setError(err.message || 'Failed to fetch results.')
} finally {
Comment on lines +70 to +73
Copy link
Copy Markdown

@charliecreates charliecreates bot Mar 31, 2026

Choose a reason for hiding this comment

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

The catch (err: any) pattern is an unsafe escape hatch, and this refactor was a good opportunity to tighten it. Even if the API usually throws an Error, any makes it easy to accidentally rely on non-existent properties in the future.

Suggestion

Use unknown and extract a message safely.

} catch (err: unknown) {
  console.error('Search error:', err)
  const message = err instanceof Error ? err.message : 'Failed to fetch results.'
  setError(message)
} finally {
  setLoading(false)
}

Reply with "@CharlieHelps yes please" if you’d like me to add a commit with this suggestion.

setLoading(false)
}
},
[],
)

const handleSearch = useCallback(
(e: React.FormEvent) => {
e.preventDefault()
void executeSearch(query, selectedSources, filters)
},
[executeSearch, query, selectedSources, filters],
)

const handleFilterChange = useCallback(
(newFilters: SearchFiltersState) => {
setFilters(newFilters)
setShowFilters(false)
// Auto-refresh search if we already have a query
if (query.trim()) {
void executeSearch(query, selectedSources, newFilters)
}
},
[executeSearch, query, selectedSources],
)
Comment on lines +88 to +98
Copy link
Copy Markdown

@charliecreates charliecreates bot Mar 31, 2026

Choose a reason for hiding this comment

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

handleFilterChange is still recreated on every query change because it captures query (and selectedSources). If SearchFilters is memoized, this largely defeats the stated goal (“fewer renders of SearchFilters when the query state updates”), since the onChange prop identity will still churn while typing.

Suggestion

Make the callback stable by not capturing query/selectedSources directly. One pragmatic approach is to keep refs to the latest values and depend only on executeSearch:

const queryRef = useRef(query)
useEffect(() => {
  queryRef.current = query
}, [query])

const sourcesRef = useRef(selectedSources)
useEffect(() => {
  sourcesRef.current = selectedSources
}, [selectedSources])

const handleFilterChange = useCallback(
  (newFilters: SearchFiltersState) => {
    setFilters(newFilters)
    setShowFilters(false)
    if (queryRef.current.trim()) {
      void executeSearch(queryRef.current, sourcesRef.current, newFilters)
    }
  },
  [executeSearch],
)

Reply with "@CharlieHelps yes please" if you’d like me to add a commit with this suggestion.


return (
<motion.div className='mx-auto w-full max-w-6xl p-4'>
Expand Down
Loading