fix(lib): address review comments on useTags hook

- Fix stale tagCount in setTagApi: use result.length instead of
  captured tagCount to determine allTagsLoaded (removes stale closure)
- Fix duplicate API calls in addTag: early-return on existing tags and
  eagerly update tagsRef to prevent same-tick duplicate createItem calls
- Fix act() warnings in tests: use await act() for async setTagApi
  calls so microtasks flush within act boundary
- Remove now-unused eslint-disable for react-hooks/exhaustive-deps
This commit is contained in:
mahula 2026-02-06 15:46:24 +01:00
parent 51879265e0
commit 4f4810b3b7
2 changed files with 35 additions and 49 deletions

View File

@ -92,16 +92,16 @@ describe('useTags - Hashtag Auto-Creation Feature', () => {
}) })
describe('API Integration', () => { describe('API Integration', () => {
it('persists new tags via API.createItem', () => { it('persists new tags via API.createItem', async () => {
const { api, createItem } = createMockApi() const { api, createItem } = createMockApi()
const { result } = renderHook(() => ({ addTag: useAddTag(), setTagApi: useSetTagApi() }), { const { result } = renderHook(() => ({ addTag: useAddTag(), setTagApi: useSetTagApi() }), {
wrapper: createWrapper([]), wrapper: createWrapper([]),
}) })
act(() => { // setTagApi is async internally — use async act to flush microtasks
result.current.setTagApi(api) // eslint-disable-next-line @typescript-eslint/no-confusing-void-expression, @typescript-eslint/await-thenable
}) await act(() => result.current.setTagApi(api))
act(() => { act(() => {
result.current.addTag({ id: 'new-1', name: 'permaculture', color: '#84cc16' }) result.current.addTag({ id: 'new-1', name: 'permaculture', color: '#84cc16' })
@ -110,16 +110,15 @@ describe('useTags - Hashtag Auto-Creation Feature', () => {
expect(createItem).toHaveBeenCalledWith(expect.objectContaining({ name: 'permaculture' })) expect(createItem).toHaveBeenCalledWith(expect.objectContaining({ name: 'permaculture' }))
}) })
it('does not call API for existing tags', () => { it('does not call API for existing tags', async () => {
const { api, createItem } = createMockApi() const { api, createItem } = createMockApi()
const { result } = renderHook(() => ({ addTag: useAddTag(), setTagApi: useSetTagApi() }), { const { result } = renderHook(() => ({ addTag: useAddTag(), setTagApi: useSetTagApi() }), {
wrapper: createWrapper(existingTags), wrapper: createWrapper(existingTags),
}) })
act(() => { // eslint-disable-next-line @typescript-eslint/no-confusing-void-expression, @typescript-eslint/await-thenable
result.current.setTagApi(api) await act(() => result.current.setTagApi(api))
})
act(() => { act(() => {
result.current.addTag({ id: 'dup-1', name: 'nature', color: '#000000' }) result.current.addTag({ id: 'dup-1', name: 'nature', color: '#000000' })
@ -128,7 +127,7 @@ describe('useTags - Hashtag Auto-Creation Feature', () => {
expect(createItem).not.toHaveBeenCalled() expect(createItem).not.toHaveBeenCalled()
}) })
it('uses current API ref, not stale closure', () => { it('uses current API ref, not stale closure', async () => {
const { api: oldApi, createItem: oldCreateItem } = createMockApi() const { api: oldApi, createItem: oldCreateItem } = createMockApi()
const { api: newApi, createItem: newCreateItem } = createMockApi() const { api: newApi, createItem: newCreateItem } = createMockApi()
@ -136,12 +135,10 @@ describe('useTags - Hashtag Auto-Creation Feature', () => {
wrapper: createWrapper([]), wrapper: createWrapper([]),
}) })
act(() => { // eslint-disable-next-line @typescript-eslint/no-confusing-void-expression, @typescript-eslint/await-thenable
result.current.setTagApi(oldApi) await act(() => result.current.setTagApi(oldApi))
}) // eslint-disable-next-line @typescript-eslint/no-confusing-void-expression, @typescript-eslint/await-thenable
act(() => { await act(() => result.current.setTagApi(newApi))
result.current.setTagApi(newApi)
})
act(() => { act(() => {
result.current.addTag({ id: '1', name: 'test', color: '#ff0000' }) result.current.addTag({ id: '1', name: 'test', color: '#ff0000' })
@ -153,7 +150,7 @@ describe('useTags - Hashtag Auto-Creation Feature', () => {
}) })
describe('Hashtag Detection & Auto-Creation (processItemsTags)', () => { describe('Hashtag Detection & Auto-Creation (processItemsTags)', () => {
it('extracts hashtags from item text and creates tags', () => { it('extracts hashtags from item text and creates tags', async () => {
const { api, createItem } = createMockApi() const { api, createItem } = createMockApi()
const { result } = renderHook( const { result } = renderHook(
@ -165,9 +162,8 @@ describe('useTags - Hashtag Auto-Creation Feature', () => {
{ wrapper: createWrapper([]) }, { wrapper: createWrapper([]) },
) )
act(() => { // eslint-disable-next-line @typescript-eslint/no-confusing-void-expression, @typescript-eslint/await-thenable
result.current.setTagApi(api) await act(() => result.current.setTagApi(api))
})
act(() => { act(() => {
result.current.processItemsTags([mockItem('Join our #gardening workshop this weekend!')]) result.current.processItemsTags([mockItem('Join our #gardening workshop this weekend!')])
@ -178,7 +174,7 @@ describe('useTags - Hashtag Auto-Creation Feature', () => {
expect(createItem).toHaveBeenCalledTimes(1) expect(createItem).toHaveBeenCalledTimes(1)
}) })
it('skips hashtags that already exist as tags', () => { it('skips hashtags that already exist as tags', async () => {
const { api, createItem } = createMockApi() const { api, createItem } = createMockApi()
const { result } = renderHook( const { result } = renderHook(
@ -190,9 +186,8 @@ describe('useTags - Hashtag Auto-Creation Feature', () => {
{ wrapper: createWrapper(existingTags) }, { wrapper: createWrapper(existingTags) },
) )
act(() => { // eslint-disable-next-line @typescript-eslint/no-confusing-void-expression, @typescript-eslint/await-thenable
result.current.setTagApi(api) await act(() => result.current.setTagApi(api))
})
act(() => { act(() => {
result.current.processItemsTags([mockItem('Love #nature and #community events')]) result.current.processItemsTags([mockItem('Love #nature and #community events')])
@ -202,7 +197,7 @@ describe('useTags - Hashtag Auto-Creation Feature', () => {
expect(createItem).not.toHaveBeenCalled() expect(createItem).not.toHaveBeenCalled()
}) })
it('handles multiple hashtags in one item', () => { it('handles multiple hashtags in one item', async () => {
const { api, createItem } = createMockApi() const { api, createItem } = createMockApi()
const { result } = renderHook( const { result } = renderHook(
@ -214,9 +209,8 @@ describe('useTags - Hashtag Auto-Creation Feature', () => {
{ wrapper: createWrapper([]) }, { wrapper: createWrapper([]) },
) )
act(() => { // eslint-disable-next-line @typescript-eslint/no-confusing-void-expression, @typescript-eslint/await-thenable
result.current.setTagApi(api) await act(() => result.current.setTagApi(api))
})
act(() => { act(() => {
result.current.processItemsTags([ result.current.processItemsTags([
@ -231,7 +225,7 @@ describe('useTags - Hashtag Auto-Creation Feature', () => {
expect(createItem).toHaveBeenCalledTimes(3) expect(createItem).toHaveBeenCalledTimes(3)
}) })
it('deduplicates same hashtag across multiple items', () => { it('deduplicates same hashtag across multiple items', async () => {
const { api, createItem } = createMockApi() const { api, createItem } = createMockApi()
const { result } = renderHook( const { result } = renderHook(
@ -243,9 +237,8 @@ describe('useTags - Hashtag Auto-Creation Feature', () => {
{ wrapper: createWrapper([]) }, { wrapper: createWrapper([]) },
) )
act(() => { // eslint-disable-next-line @typescript-eslint/no-confusing-void-expression, @typescript-eslint/await-thenable
result.current.setTagApi(api) await act(() => result.current.setTagApi(api))
})
act(() => { act(() => {
result.current.processItemsTags([ result.current.processItemsTags([
@ -260,7 +253,7 @@ describe('useTags - Hashtag Auto-Creation Feature', () => {
expect(createItem).toHaveBeenCalledTimes(1) expect(createItem).toHaveBeenCalledTimes(1)
}) })
it('handles items without text gracefully', () => { it('handles items without text gracefully', async () => {
const { api, createItem } = createMockApi() const { api, createItem } = createMockApi()
const { result } = renderHook( const { result } = renderHook(
@ -272,9 +265,8 @@ describe('useTags - Hashtag Auto-Creation Feature', () => {
{ wrapper: createWrapper([]) }, { wrapper: createWrapper([]) },
) )
act(() => { // eslint-disable-next-line @typescript-eslint/no-confusing-void-expression, @typescript-eslint/await-thenable
result.current.setTagApi(api) await act(() => result.current.setTagApi(api))
})
act(() => { act(() => {
result.current.processItemsTags([ result.current.processItemsTags([
@ -288,7 +280,7 @@ describe('useTags - Hashtag Auto-Creation Feature', () => {
expect(createItem).not.toHaveBeenCalled() expect(createItem).not.toHaveBeenCalled()
}) })
it('assigns color to auto-created tags', () => { it('assigns color to auto-created tags', async () => {
const { api } = createMockApi() const { api } = createMockApi()
const { result } = renderHook( const { result } = renderHook(
@ -300,9 +292,8 @@ describe('useTags - Hashtag Auto-Creation Feature', () => {
{ wrapper: createWrapper([]) }, { wrapper: createWrapper([]) },
) )
act(() => { // eslint-disable-next-line @typescript-eslint/no-confusing-void-expression, @typescript-eslint/await-thenable
result.current.setTagApi(api) await act(() => result.current.setTagApi(api))
})
act(() => { act(() => {
result.current.processItemsTags([mockItem('New #colorful tag')]) result.current.processItemsTags([mockItem('New #colorful tag')])

View File

@ -66,7 +66,7 @@ function useTagsManager(initialTags: Tag[]): {
apiRef.current = api apiRef.current = api
const result = await api.getItems() const result = await api.getItems()
setTagCount(result.length) setTagCount(result.length)
if (tagCount === 0) setallTagsLoaded(true) if (result.length === 0) setallTagsLoaded(true)
if (result) { if (result) {
result.map((tag) => { result.map((tag) => {
// tag.name = tag.name.toLocaleLowerCase(); // tag.name = tag.name.toLocaleLowerCase();
@ -74,8 +74,6 @@ function useTagsManager(initialTags: Tag[]): {
return null return null
}) })
} }
// eslint-disable-next-line react-hooks/exhaustive-deps
}, []) }, [])
const setTagData = useCallback((data: Tag[]) => { const setTagData = useCallback((data: Tag[]) => {
@ -87,18 +85,15 @@ function useTagsManager(initialTags: Tag[]): {
}, []) }, [])
const addTag = useCallback((tag: Tag) => { const addTag = useCallback((tag: Tag) => {
// Check against current tags using ref to avoid stale closure
const tagExists = tagsRef.current.some( const tagExists = tagsRef.current.some(
(t) => t.name.toLocaleLowerCase() === tag.name.toLocaleLowerCase(), (t) => t.name.toLocaleLowerCase() === tag.name.toLocaleLowerCase(),
) )
if (tagExists) return
dispatch({ tagsRef.current = [...tagsRef.current, tag]
type: 'ADD', dispatch({ type: 'ADD', tag })
tag,
})
// Only create in API if tag doesn't already exist if (apiRef.current.createItem) {
if (!tagExists && apiRef.current.createItem) {
apiRef.current.createItem(tag) apiRef.current.createItem(tag)
} }
}, []) }, [])