From 4f4810b3b7bfe8fa25b7127357b19262bb71e01f Mon Sep 17 00:00:00 2001 From: mahula Date: Fri, 6 Feb 2026 15:46:24 +0100 Subject: [PATCH] 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 --- lib/src/Components/Map/hooks/useTags.spec.tsx | 69 ++++++++----------- lib/src/Components/Map/hooks/useTags.tsx | 15 ++-- 2 files changed, 35 insertions(+), 49 deletions(-) diff --git a/lib/src/Components/Map/hooks/useTags.spec.tsx b/lib/src/Components/Map/hooks/useTags.spec.tsx index 55a9c095..eca28144 100644 --- a/lib/src/Components/Map/hooks/useTags.spec.tsx +++ b/lib/src/Components/Map/hooks/useTags.spec.tsx @@ -92,16 +92,16 @@ describe('useTags - Hashtag Auto-Creation Feature', () => { }) describe('API Integration', () => { - it('persists new tags via API.createItem', () => { + it('persists new tags via API.createItem', async () => { const { api, createItem } = createMockApi() const { result } = renderHook(() => ({ addTag: useAddTag(), setTagApi: useSetTagApi() }), { wrapper: createWrapper([]), }) - act(() => { - result.current.setTagApi(api) - }) + // setTagApi is async internally — use async act to flush microtasks + // eslint-disable-next-line @typescript-eslint/no-confusing-void-expression, @typescript-eslint/await-thenable + await act(() => result.current.setTagApi(api)) act(() => { 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' })) }) - it('does not call API for existing tags', () => { + it('does not call API for existing tags', async () => { const { api, createItem } = createMockApi() const { result } = renderHook(() => ({ addTag: useAddTag(), setTagApi: useSetTagApi() }), { wrapper: createWrapper(existingTags), }) - act(() => { - 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(() => { result.current.addTag({ id: 'dup-1', name: 'nature', color: '#000000' }) @@ -128,7 +127,7 @@ describe('useTags - Hashtag Auto-Creation Feature', () => { 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: newApi, createItem: newCreateItem } = createMockApi() @@ -136,12 +135,10 @@ describe('useTags - Hashtag Auto-Creation Feature', () => { wrapper: createWrapper([]), }) - act(() => { - result.current.setTagApi(oldApi) - }) - act(() => { - result.current.setTagApi(newApi) - }) + // eslint-disable-next-line @typescript-eslint/no-confusing-void-expression, @typescript-eslint/await-thenable + await act(() => result.current.setTagApi(oldApi)) + // eslint-disable-next-line @typescript-eslint/no-confusing-void-expression, @typescript-eslint/await-thenable + await act(() => result.current.setTagApi(newApi)) act(() => { 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)', () => { - it('extracts hashtags from item text and creates tags', () => { + it('extracts hashtags from item text and creates tags', async () => { const { api, createItem } = createMockApi() const { result } = renderHook( @@ -165,9 +162,8 @@ describe('useTags - Hashtag Auto-Creation Feature', () => { { wrapper: createWrapper([]) }, ) - act(() => { - 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(() => { result.current.processItemsTags([mockItem('Join our #gardening workshop this weekend!')]) @@ -178,7 +174,7 @@ describe('useTags - Hashtag Auto-Creation Feature', () => { 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 { result } = renderHook( @@ -190,9 +186,8 @@ describe('useTags - Hashtag Auto-Creation Feature', () => { { wrapper: createWrapper(existingTags) }, ) - act(() => { - 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(() => { result.current.processItemsTags([mockItem('Love #nature and #community events')]) @@ -202,7 +197,7 @@ describe('useTags - Hashtag Auto-Creation Feature', () => { expect(createItem).not.toHaveBeenCalled() }) - it('handles multiple hashtags in one item', () => { + it('handles multiple hashtags in one item', async () => { const { api, createItem } = createMockApi() const { result } = renderHook( @@ -214,9 +209,8 @@ describe('useTags - Hashtag Auto-Creation Feature', () => { { wrapper: createWrapper([]) }, ) - act(() => { - 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(() => { result.current.processItemsTags([ @@ -231,7 +225,7 @@ describe('useTags - Hashtag Auto-Creation Feature', () => { expect(createItem).toHaveBeenCalledTimes(3) }) - it('deduplicates same hashtag across multiple items', () => { + it('deduplicates same hashtag across multiple items', async () => { const { api, createItem } = createMockApi() const { result } = renderHook( @@ -243,9 +237,8 @@ describe('useTags - Hashtag Auto-Creation Feature', () => { { wrapper: createWrapper([]) }, ) - act(() => { - 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(() => { result.current.processItemsTags([ @@ -260,7 +253,7 @@ describe('useTags - Hashtag Auto-Creation Feature', () => { expect(createItem).toHaveBeenCalledTimes(1) }) - it('handles items without text gracefully', () => { + it('handles items without text gracefully', async () => { const { api, createItem } = createMockApi() const { result } = renderHook( @@ -272,9 +265,8 @@ describe('useTags - Hashtag Auto-Creation Feature', () => { { wrapper: createWrapper([]) }, ) - act(() => { - 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(() => { result.current.processItemsTags([ @@ -288,7 +280,7 @@ describe('useTags - Hashtag Auto-Creation Feature', () => { expect(createItem).not.toHaveBeenCalled() }) - it('assigns color to auto-created tags', () => { + it('assigns color to auto-created tags', async () => { const { api } = createMockApi() const { result } = renderHook( @@ -300,9 +292,8 @@ describe('useTags - Hashtag Auto-Creation Feature', () => { { wrapper: createWrapper([]) }, ) - act(() => { - 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(() => { result.current.processItemsTags([mockItem('New #colorful tag')]) diff --git a/lib/src/Components/Map/hooks/useTags.tsx b/lib/src/Components/Map/hooks/useTags.tsx index b0eeb377..6b72103d 100644 --- a/lib/src/Components/Map/hooks/useTags.tsx +++ b/lib/src/Components/Map/hooks/useTags.tsx @@ -66,7 +66,7 @@ function useTagsManager(initialTags: Tag[]): { apiRef.current = api const result = await api.getItems() setTagCount(result.length) - if (tagCount === 0) setallTagsLoaded(true) + if (result.length === 0) setallTagsLoaded(true) if (result) { result.map((tag) => { // tag.name = tag.name.toLocaleLowerCase(); @@ -74,8 +74,6 @@ function useTagsManager(initialTags: Tag[]): { return null }) } - - // eslint-disable-next-line react-hooks/exhaustive-deps }, []) const setTagData = useCallback((data: Tag[]) => { @@ -87,18 +85,15 @@ function useTagsManager(initialTags: Tag[]): { }, []) const addTag = useCallback((tag: Tag) => { - // Check against current tags using ref to avoid stale closure const tagExists = tagsRef.current.some( (t) => t.name.toLocaleLowerCase() === tag.name.toLocaleLowerCase(), ) + if (tagExists) return - dispatch({ - type: 'ADD', - tag, - }) + tagsRef.current = [...tagsRef.current, tag] + dispatch({ type: 'ADD', tag }) - // Only create in API if tag doesn't already exist - if (!tagExists && apiRef.current.createItem) { + if (apiRef.current.createItem) { apiRef.current.createItem(tag) } }, [])