-
Notifications
You must be signed in to change notification settings - Fork 3.4k
Fix: Resolve invisible optimistic updates in Knowledge feature #717
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.
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
Closed
jonahgabriel
wants to merge
14
commits into
coleam00:main
from
jonahgabriel:fix/invisible-optimistic-updates-todo
Closed
Changes from 9 commits
Commits
Show all changes
14 commits
Select commit
Hold shift + click to select a range
32d8ccb
fix: resolve invisible optimistic updates TODO with KnowledgeFilterCo…
9e4e662
cleanup: remove .claude-flow metrics files
jonahgabriel ee757ff
fix: replace hardcoded pagination values in optimistic updates
jonahgabriel 43a8b21
revert: remove pagination fixes to focus on invisible optimistic upda…
jonahgabriel 35632c1
Add comprehensive test coverage for invisible optimistic updates fix
jonahgabriel bb23458
Fix KnowledgeFilterContext tests with proper API usage and act() wrap…
jonahgabriel 5bcce4c
Merge remote-tracking branch 'origin/main' into fix/invisible-optimis…
jonahgabriel 2416475
fix: align Knowledge optimistic updates with established patterns
jonahgabriel 3d67aaa
Resolve merge conflicts from upstream/main
jonahgabriel 7a90fd6
Address PR #717 reviewer feedback: remove context layer and align wit…
jonahgabriel ed846e4
refactor: address CodeRabbit nitpick comments
jonahgabriel cfa53cc
fix: address all CodeRabbit nitpick comments
jonahgabriel a07676c
refactor: address CodeRabbit review feedback in useKnowledgeQueries
jonahgabriel 6301444
fix: address CodeRabbit review comments for PR #717
jonahgabriel File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
86 changes: 86 additions & 0 deletions
86
archon-ui-main/src/features/knowledge/context/KnowledgeFilterContext.tsx
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,86 @@ | ||
| /** | ||
| * Knowledge Filter Context | ||
| * Provides shared filter state between KnowledgeView components and hooks | ||
| * | ||
| * This solves the optimistic updates issue where mutations need to know | ||
| * the current filter state to update the correct cached query. | ||
| */ | ||
|
|
||
| import React, { createContext, useContext, useMemo, useState } from "react"; | ||
| import type { KnowledgeItemsFilter } from "../types"; | ||
|
|
||
| interface KnowledgeFilterContextType { | ||
| // Current filter state | ||
| searchQuery: string; | ||
| typeFilter: "all" | "technical" | "business"; | ||
|
|
||
| // Filter state setters | ||
| setSearchQuery: (query: string) => void; | ||
| setTypeFilter: (type: "all" | "technical" | "business") => void; | ||
|
|
||
| // Computed filter object for API queries | ||
| currentFilter: KnowledgeItemsFilter; | ||
| } | ||
|
|
||
| const KnowledgeFilterContext = createContext<KnowledgeFilterContextType | null>(null); | ||
|
|
||
| interface KnowledgeFilterProviderProps { | ||
| children: React.ReactNode; | ||
| } | ||
|
|
||
| /** | ||
| * Provider component that manages knowledge filter state | ||
| */ | ||
| export function KnowledgeFilterProvider({ children }: KnowledgeFilterProviderProps) { | ||
| const [searchQuery, setSearchQuery] = useState(""); | ||
| const [typeFilter, setTypeFilter] = useState<"all" | "technical" | "business">("all"); | ||
|
|
||
| // Build filter object for API - memoize to prevent recreating on every render | ||
| const currentFilter = useMemo<KnowledgeItemsFilter>(() => { | ||
| const filter: KnowledgeItemsFilter = { | ||
| page: 1, | ||
| per_page: 100, | ||
| }; | ||
|
|
||
| if (searchQuery) { | ||
| filter.search = searchQuery; | ||
| } | ||
|
|
||
| if (typeFilter !== "all") { | ||
| filter.knowledge_type = typeFilter; | ||
| } | ||
|
|
||
| return filter; | ||
| }, [searchQuery, typeFilter]); | ||
|
|
||
| const value = useMemo<KnowledgeFilterContextType>( | ||
| () => ({ | ||
| searchQuery, | ||
| typeFilter, | ||
| setSearchQuery, | ||
| setTypeFilter, | ||
| currentFilter, | ||
| }), | ||
| [searchQuery, typeFilter, currentFilter] | ||
| ); | ||
|
|
||
| return ( | ||
| <KnowledgeFilterContext.Provider value={value}> | ||
| {children} | ||
| </KnowledgeFilterContext.Provider> | ||
| ); | ||
| } | ||
|
|
||
| /** | ||
| * Hook to access knowledge filter context | ||
| * @throws Error if used outside of KnowledgeFilterProvider | ||
| */ | ||
| export function useKnowledgeFilter() { | ||
| const context = useContext(KnowledgeFilterContext); | ||
|
|
||
| if (!context) { | ||
| throw new Error("useKnowledgeFilter must be used within a KnowledgeFilterProvider"); | ||
| } | ||
|
|
||
| return context; | ||
| } | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,6 @@ | ||
| /** | ||
| * Knowledge Context Module | ||
| * Exports all context providers and hooks | ||
| */ | ||
|
|
||
| export { KnowledgeFilterProvider, useKnowledgeFilter } from "./KnowledgeFilterContext"; |
200 changes: 200 additions & 0 deletions
200
archon-ui-main/src/features/knowledge/context/tests/KnowledgeFilterContext.test.tsx
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,200 @@ | ||
| import { describe, it, expect } from 'vitest'; | ||
| import { render, screen, act } from '@testing-library/react'; | ||
| import { userEvent } from '@testing-library/user-event'; | ||
| import { KnowledgeFilterProvider, useKnowledgeFilter } from '../KnowledgeFilterContext'; | ||
| import type { KnowledgeFilter } from '../../types'; | ||
|
|
||
| // Test component that uses the context | ||
| function TestComponent() { | ||
| const { currentFilter, searchQuery, typeFilter, setSearchQuery, setTypeFilter } = useKnowledgeFilter(); | ||
|
|
||
| return ( | ||
| <div> | ||
| <div data-testid="current-filter">{JSON.stringify(currentFilter)}</div> | ||
| <div data-testid="search-query">{searchQuery}</div> | ||
| <div data-testid="type-filter">{typeFilter}</div> | ||
| <button | ||
| data-testid="update-search" | ||
| onClick={() => setSearchQuery('test search')} | ||
| > | ||
| Update Search | ||
| </button> | ||
| <button | ||
| data-testid="update-type" | ||
| onClick={() => setTypeFilter('technical')} | ||
| > | ||
| Update Type | ||
| </button> | ||
| </div> | ||
| ); | ||
| } | ||
|
|
||
| function TestComponentWithoutProvider() { | ||
| const hook = useKnowledgeFilter(); | ||
| return <div data-testid="hook-result">{JSON.stringify(hook)}</div>; | ||
| } | ||
|
|
||
| describe('KnowledgeFilterContext', () => { | ||
| describe('KnowledgeFilterProvider', () => { | ||
| it('should provide default filter state', () => { | ||
| render( | ||
| <KnowledgeFilterProvider> | ||
| <TestComponent /> | ||
| </KnowledgeFilterProvider> | ||
| ); | ||
|
|
||
| const filterDisplay = screen.getByTestId('current-filter'); | ||
| const defaultFilter = JSON.parse(filterDisplay.textContent || '{}'); | ||
|
|
||
| expect(defaultFilter).toEqual({ | ||
| page: 1, | ||
| per_page: 100 | ||
| }); | ||
|
|
||
| const searchQuery = screen.getByTestId('search-query'); | ||
| const typeFilter = screen.getByTestId('type-filter'); | ||
|
|
||
| expect(searchQuery.textContent).toBe(''); | ||
| expect(typeFilter.textContent).toBe('all'); | ||
| }); | ||
|
|
||
| it('should update search query when setSearchQuery is called', async () => { | ||
| const user = userEvent.setup(); | ||
|
|
||
| render( | ||
| <KnowledgeFilterProvider> | ||
| <TestComponent /> | ||
| </KnowledgeFilterProvider> | ||
| ); | ||
|
|
||
| const updateButton = screen.getByTestId('update-search'); | ||
|
|
||
| await act(async () => { | ||
| await user.click(updateButton); | ||
| }); | ||
|
|
||
| const filterDisplay = screen.getByTestId('current-filter'); | ||
| const updatedFilter = JSON.parse(filterDisplay.textContent || '{}'); | ||
| const searchQuery = screen.getByTestId('search-query'); | ||
|
|
||
| expect(searchQuery.textContent).toBe('test search'); | ||
| expect(updatedFilter).toEqual({ | ||
| search: 'test search', | ||
| page: 1, | ||
| per_page: 100 | ||
| }); | ||
| }); | ||
|
|
||
| it('should update type filter when setTypeFilter is called', async () => { | ||
| const user = userEvent.setup(); | ||
|
|
||
| render( | ||
| <KnowledgeFilterProvider> | ||
| <TestComponent /> | ||
| </KnowledgeFilterProvider> | ||
| ); | ||
|
|
||
| const updateButton = screen.getByTestId('update-type'); | ||
|
|
||
| await act(async () => { | ||
| await user.click(updateButton); | ||
| }); | ||
|
|
||
| const filterDisplay = screen.getByTestId('current-filter'); | ||
| const updatedFilter = JSON.parse(filterDisplay.textContent || '{}'); | ||
| const typeFilter = screen.getByTestId('type-filter'); | ||
|
|
||
| expect(typeFilter.textContent).toBe('technical'); | ||
| expect(updatedFilter).toEqual({ | ||
| knowledge_type: 'technical', | ||
| page: 1, | ||
| per_page: 100 | ||
| }); | ||
| }); | ||
|
|
||
| it('should merge search and type updates correctly', async () => { | ||
| const user = userEvent.setup(); | ||
|
|
||
| render( | ||
| <KnowledgeFilterProvider> | ||
| <TestComponent /> | ||
| </KnowledgeFilterProvider> | ||
| ); | ||
|
|
||
| // Update search first | ||
| await act(async () => { | ||
| await user.click(screen.getByTestId('update-search')); | ||
| }); | ||
|
|
||
| // Then update type | ||
| await act(async () => { | ||
| await user.click(screen.getByTestId('update-type')); | ||
| }); | ||
|
|
||
| const filterDisplay = screen.getByTestId('current-filter'); | ||
| const finalFilter = JSON.parse(filterDisplay.textContent || '{}'); | ||
|
|
||
| expect(finalFilter).toEqual({ | ||
| search: 'test search', | ||
| knowledge_type: 'technical', | ||
| page: 1, | ||
| per_page: 100 | ||
| }); | ||
| }); | ||
| }); | ||
|
|
||
| describe('useKnowledgeFilter', () => { | ||
| it('should throw error when used outside provider', () => { | ||
| // Suppress console.error for this test | ||
| const originalError = console.error; | ||
| console.error = () => {}; | ||
|
|
||
| expect(() => { | ||
| render(<TestComponentWithoutProvider />); | ||
| }).toThrow('useKnowledgeFilter must be used within a KnowledgeFilterProvider'); | ||
|
|
||
| console.error = originalError; | ||
| }); | ||
|
|
||
| it('should maintain filter state across re-renders', async () => { | ||
| let renderCount = 0; | ||
|
|
||
| const TestRerenderComponent = () => { | ||
| renderCount++; | ||
| const { currentFilter, setSearchQuery } = useKnowledgeFilter(); | ||
|
|
||
| return ( | ||
| <div> | ||
| <div data-testid="render-count">{renderCount}</div> | ||
| <div data-testid="filter-state">{JSON.stringify(currentFilter)}</div> | ||
| <button | ||
| data-testid="trigger-update" | ||
| onClick={() => setSearchQuery('persistent')} | ||
| > | ||
| Update | ||
| </button> | ||
| </div> | ||
| ); | ||
| }; | ||
|
|
||
| const user = userEvent.setup(); | ||
|
|
||
| render( | ||
| <KnowledgeFilterProvider> | ||
| <TestRerenderComponent /> | ||
| </KnowledgeFilterProvider> | ||
| ); | ||
|
|
||
| // Update the filter | ||
| await act(async () => { | ||
| await user.click(screen.getByTestId('trigger-update')); | ||
| }); | ||
|
|
||
| const filterState = screen.getByTestId('filter-state'); | ||
| const currentFilter = JSON.parse(filterState.textContent || '{}'); | ||
|
|
||
| expect(currentFilter.search).toBe('persistent'); | ||
| expect(renderCount).toBeGreaterThan(1); | ||
| }); | ||
| }); | ||
| }); |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
🧩 Analysis chain
Hard-coded pagination defaults can drift from API/query keys
Page/per_page are fixed to 1/100 here. If server/query-key defaults differ elsewhere, caches fragment and optimistic updates won’t land where expected. Extract shared defaults and import them, or read them from the query-key builder used by useKnowledgeSummaries.
🏁 Script executed:
Length of output: 10803
Centralize pagination defaults (page / per_page) across the knowledge feature
Hard-coded defaults appear in many places (most use page: 1, per_page: 100) but at least one test uses per_page: 20 — extract a single shared DEFAULT_PAGE/DEFAULT_PER_PAGE (or read from the query-key builder/knowledgeKeys) and import it everywhere to avoid cache/query-key mismatches and brittle tests.
Relevant locations:
🤖 Prompt for AI Agents