Fix: Resolve invisible optimistic updates in Knowledge feature#717
Fix: Resolve invisible optimistic updates in Knowledge feature#717jonahgabriel wants to merge 14 commits intocoleam00:mainfrom
Conversation
…ntext - Create KnowledgeFilterContext to share filter state between components - Add useCurrentKnowledgeFilter helper for accessing context in hooks - Update KnowledgeView to use context instead of local state - Enhance useCrawlUrl and useUploadDocument with smart optimistic updates - Prioritize updating currently viewed filter for immediate user feedback - Add search query matching to filter logic for accurate updates - Remove TODO comment and replace with FIXED documentation Fixes the issue where users didn't see immediate feedback when adding knowledge items unless the filter happened to match what was being viewed. Users now see optimistic updates regardless of current filter state.
Remove accidentally committed Claude Flow metrics files from the optimistic updates fix branch. These files should not be part of the version control.
|
Thanks for this @jonahgabriel! Adding @Wirasm as a reviewer |
- Replace hardcoded page: 1 and per_page: 100 with dynamic values from filters - Use currentFilter.page ?? 1 and currentFilter.per_page ?? 100 for current filter updates - Use filter?.page ?? 1 and filter?.per_page ?? 100 for cached filter updates - Prevents cache key mismatches when users have different pagination settings This ensures optimistic updates target the correct cached queries regardless of the user's current page size or page number settings.
…tes issue Reverted dynamic pagination values back to hardcoded values to match the original scope of the invisible optimistic updates issue. The pagination improvements can be addressed in a separate PR. Keeping this PR focused on the KnowledgeFilterContext solution only.
- Add complete KnowledgeFilterContext test suite (8/8 tests) - Context provider functionality - Hook error handling outside provider - Filter state updates and merging - Deep equality checks and state persistence - Enhance useKnowledgeQueries tests (14/14 tests) - Filter-aware optimistic updates for crawling and uploads - Priority filter matching (current vs other cached filters) - Context integration and proper mock configuration - Fix filter type mismatches (knowledge_type vs type) All 22 tests now passing, meeting contributing guidelines for comprehensive test coverage.
…ping - Fixed import to use useKnowledgeFilter instead of non-existent useCurrentKnowledgeFilter - Updated test expectations to match actual context API (searchQuery, typeFilter, setSearchQuery, setTypeFilter) - Simplified tests to focus on actual context functionality - Wrapped all state updates in act() to eliminate React warnings - All 20 knowledge tests now pass cleanly (6 context + 14 hooks) - Tests complete in ~1s without hanging or timeouts
Wirasm
left a comment
Wirasm
left a comment
There was a problem hiding this comment.
Hi @jonahgabriel,
First off, thank you so much for taking the time to tackle this issue! 🙏 I really appreciate you diving into those TODO comments and working on fixing the invisible optimistic updates. Your comprehensive test coverage is excellent, and the solution definitely works.
After reviewing the PR, I noticed some deviation from the established patterns documented in PRPs/ai_docs/optimistic_updates.md that would help simplify this implementation significantly. The current approach introduces a context layer that adds complexity, while our existing patterns in the Tasks and Projects features achieve the same result more simply.
A few specific points based on our patterns:
- Use our standard utilities: We should use replaceOptimisticEntity() and removeDuplicateEntities() from our shared optimistic utilities instead of manual mapping. These handle race conditions safely.
- Avoid the context layer: Looking at how Tasks (useTaskQueries.ts) and Projects (useProjectQueries.ts) handle this, we can achieve the same filter-awareness without adding a context. Perhaps passing the current filter as a parameter to the mutation hooks would be simpler?
- Avoid dynamic require(): The dynamic CommonJS import could cause bundling issues - standard ESM imports would be more reliable.
Would you mind taking a look at PRPs/ai_docs/optimistic_updates.md and aligning the implementation with those patterns? The documented approach should actually reduce the code needed while maintaining all the functionality you've built.
Again, really grateful for your contribution here, the core insight of making mutations filter-aware is spot on! Just want to make sure we follow our established patterns to keep the codebase consistent and maintainable.
Let me know if you have any questions about the patterns or need any clarification!
Yep. Will do. |
- Replace manual mapping with replaceOptimisticEntity() and removeDuplicateEntities() - Remove context layer and use filter-aware parameter approach - Replace dynamic require() with standard ESM imports - Pass current filter to mutations for filter-aware optimistic updates Addresses PR feedback to align with Tasks/Projects patterns from PRPs/ai_docs/optimistic_updates.md
WalkthroughAdds filter-aware optimistic flows: useCrawlUrl and useUploadDocument now accept an optional KnowledgeItemsFilter and prioritize optimistic updates for that filter; AddKnowledgeDialog accepts and forwards currentFilter; KnowledgeView derives and supplies currentFilter from local search/type state; tests extended to validate filter-targeted cache updates. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant View as KnowledgeView
participant Dialog as AddKnowledgeDialog
participant Hook as useCrawlUrl / useUploadDocument
participant RQ as React Query Cache
participant API as Backend
User->>View: Update search/type filters
View->>View: Derive currentFilter
View->>Dialog: Render with currentFilter
User->>Dialog: Submit crawl/upload
Dialog->>Hook: mutate(payload, currentFilter)
rect rgba(230,245,255,0.6)
note over Hook,RQ: Optimistic update — target current filter first
Hook->>RQ: Update summaries(currentFilter) if itemMatchesFilter
Hook->>RQ: Update other summaries caches (skip current if handled)
end
Hook->>API: POST crawl/upload
API-->>Hook: Success (real IDs)
rect rgba(235,255,235,0.6)
note over Hook,RQ: Reconcile success
Hook->>RQ: replaceOptimisticEntity(realID) & removeDuplicateEntities()
Hook->>RQ: Update progress/operations (crawl)
end
Hook-->>Dialog: onSuccess
Dialog-->>User: UI shows confirmed item
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
archon-ui-main/src/features/knowledge/components/AddKnowledgeDialog.tsx (2)
86-89: Avoid double-closing the dialog; rely on onSuccess to closeKnowledgeView’s onSuccess already closes and refetches. Dropping this local close prevents redundant state updates.
resetForm(); onSuccess(); - onOpenChange(false);
203-204: Prefer Tailwind utility over inline color styleFor consistency with feature styling, swap the inline style for a utility class.
- <div className="absolute inset-y-0 left-0 pl-3 flex items-center pointer-events-none"> - <Globe className="h-5 w-5" style={{ color: "#0891b2" }} /> + <div className="absolute inset-y-0 left-0 pl-3 flex items-center pointer-events-none"> + <Globe className="h-5 w-5 text-cyan-600" />archon-ui-main/src/features/knowledge/hooks/useKnowledgeQueries.ts (1)
196-198: Avoid JSON.stringify for query key comparisonUsing
JSON.stringifyfor deep equality comparison of query keys could be fragile if the key structure changes or contains non-serializable values.Consider using a more robust comparison method:
-if (currentFilter && JSON.stringify(qk) === JSON.stringify(knowledgeKeys.summaries(currentFilter))) { +// Check if this is the current filter's query key by comparing the filter object +const queryFilter = qk[qk.length - 1] as KnowledgeItemsFilter | undefined; +if (currentFilter && queryFilter && + queryFilter.knowledge_type === currentFilter.knowledge_type && + queryFilter.search === currentFilter.search && + queryFilter.page === currentFilter.page && + queryFilter.per_page === currentFilter.per_page) { continue; }Also applies to: 422-424
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
archon-ui-main/src/features/knowledge/components/AddKnowledgeDialog.tsx(2 hunks)archon-ui-main/src/features/knowledge/context/KnowledgeFilterContext.tsx(1 hunks)archon-ui-main/src/features/knowledge/context/index.ts(1 hunks)archon-ui-main/src/features/knowledge/context/tests/KnowledgeFilterContext.test.tsx(1 hunks)archon-ui-main/src/features/knowledge/hooks/tests/useKnowledgeQueries.test.ts(4 hunks)archon-ui-main/src/features/knowledge/hooks/useKnowledgeQueries.ts(8 hunks)archon-ui-main/src/features/knowledge/index.ts(1 hunks)archon-ui-main/src/features/knowledge/views/KnowledgeView.tsx(2 hunks)archon-ui-main/src/features/knowledge/views/KnowledgeViewWithBoundary.tsx(1 hunks)
🧰 Additional context used
📓 Path-based instructions (6)
archon-ui-main/src/features/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
archon-ui-main/src/features/**/*.{ts,tsx}: Use TanStack Query for all data fetching (no prop drilling); use smart HTTP polling (no WebSockets)
Biome formatting in features: 120-character line length, double quotes, and trailing commas
Apply Tron-inspired glassmorphism styling with Tailwind for feature UIUse Biome in features: 120 character line length, double quotes, and trailing commas
Files:
archon-ui-main/src/features/knowledge/context/KnowledgeFilterContext.tsxarchon-ui-main/src/features/knowledge/context/index.tsarchon-ui-main/src/features/knowledge/index.tsarchon-ui-main/src/features/knowledge/views/KnowledgeView.tsxarchon-ui-main/src/features/knowledge/components/AddKnowledgeDialog.tsxarchon-ui-main/src/features/knowledge/hooks/useKnowledgeQueries.tsarchon-ui-main/src/features/knowledge/context/tests/KnowledgeFilterContext.test.tsxarchon-ui-main/src/features/knowledge/views/KnowledgeViewWithBoundary.tsxarchon-ui-main/src/features/knowledge/hooks/tests/useKnowledgeQueries.test.ts
archon-ui-main/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Disallow implicit any in TypeScript
archon-ui-main/src/**/*.{ts,tsx}: Frontend TypeScript must use strict mode with no implicit any
Use TanStack Query for all data fetching; avoid prop drilling
Use database values directly in the frontend; avoid mapping layers between BE and FE types
Files:
archon-ui-main/src/features/knowledge/context/KnowledgeFilterContext.tsxarchon-ui-main/src/features/knowledge/context/index.tsarchon-ui-main/src/features/knowledge/index.tsarchon-ui-main/src/features/knowledge/views/KnowledgeView.tsxarchon-ui-main/src/features/knowledge/components/AddKnowledgeDialog.tsxarchon-ui-main/src/features/knowledge/hooks/useKnowledgeQueries.tsarchon-ui-main/src/features/knowledge/context/tests/KnowledgeFilterContext.test.tsxarchon-ui-main/src/features/knowledge/views/KnowledgeViewWithBoundary.tsxarchon-ui-main/src/features/knowledge/hooks/tests/useKnowledgeQueries.test.ts
archon-ui-main/src/**/*.{ts,tsx,py}
📄 CodeRabbit inference engine (CLAUDE.md)
In code comments, avoid meta terms like SIMPLIFIED, ENHANCED, LEGACY, CHANGED, REMOVED; comment only on functionality and reasoning (do not mention beta/global rules)
Files:
archon-ui-main/src/features/knowledge/context/KnowledgeFilterContext.tsxarchon-ui-main/src/features/knowledge/context/index.tsarchon-ui-main/src/features/knowledge/index.tsarchon-ui-main/src/features/knowledge/views/KnowledgeView.tsxarchon-ui-main/src/features/knowledge/components/AddKnowledgeDialog.tsxarchon-ui-main/src/features/knowledge/hooks/useKnowledgeQueries.tsarchon-ui-main/src/features/knowledge/context/tests/KnowledgeFilterContext.test.tsxarchon-ui-main/src/features/knowledge/views/KnowledgeViewWithBoundary.tsxarchon-ui-main/src/features/knowledge/hooks/tests/useKnowledgeQueries.test.ts
archon-ui-main/src/features/*/components/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Place new UI components under the feature’s components directory
Place new UI components under src/features/[feature]/components
Files:
archon-ui-main/src/features/knowledge/components/AddKnowledgeDialog.tsx
archon-ui-main/src/features/*/hooks/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Place TanStack Query hooks in the feature’s hooks directory
Use feature-scoped TanStack Query hooks in src/features/[feature]/hooks
Files:
archon-ui-main/src/features/knowledge/hooks/useKnowledgeQueries.tsarchon-ui-main/src/features/knowledge/hooks/tests/useKnowledgeQueries.test.ts
archon-ui-main/src/**/*.test.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Write frontend tests with Vitest and React Testing Library
Files:
archon-ui-main/src/features/knowledge/context/tests/KnowledgeFilterContext.test.tsxarchon-ui-main/src/features/knowledge/hooks/tests/useKnowledgeQueries.test.ts
🧠 Learnings (2)
📚 Learning: 2025-09-19T10:31:54.277Z
Learnt from: CR
PR: coleam00/Archon#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-19T10:31:54.277Z
Learning: Applies to archon-ui-main/src/**/*.test.{ts,tsx} : Write frontend tests with Vitest and React Testing Library
Applied to files:
archon-ui-main/src/features/knowledge/context/tests/KnowledgeFilterContext.test.tsx
📚 Learning: 2025-09-19T10:32:55.557Z
Learnt from: CR
PR: coleam00/Archon#0
File: AGENTS.md:0-0
Timestamp: 2025-09-19T10:32:55.557Z
Learning: Applies to archon-ui-main/src/features/*/hooks/**/*.{ts,tsx} : Use feature-scoped TanStack Query hooks in src/features/[feature]/hooks
Applied to files:
archon-ui-main/src/features/knowledge/hooks/tests/useKnowledgeQueries.test.ts
🧬 Code graph analysis (6)
archon-ui-main/src/features/knowledge/context/KnowledgeFilterContext.tsx (2)
archon-ui-main/src/features/knowledge/types/knowledge.ts (1)
KnowledgeItemsFilter(127-134)archon-ui-main/src/features/knowledge/context/index.ts (2)
KnowledgeFilterProvider(6-6)useKnowledgeFilter(6-6)
archon-ui-main/src/features/knowledge/views/KnowledgeView.tsx (3)
archon-ui-main/src/features/knowledge/context/KnowledgeFilterContext.tsx (1)
useKnowledgeFilter(78-86)archon-ui-main/src/features/knowledge/types/knowledge.ts (1)
KnowledgeItem(26-39)archon-ui-main/src/features/knowledge/hooks/useKnowledgeQueries.ts (1)
useKnowledgeSummaries(791-836)
archon-ui-main/src/features/knowledge/components/AddKnowledgeDialog.tsx (2)
archon-ui-main/src/features/knowledge/types/knowledge.ts (1)
KnowledgeItemsFilter(127-134)archon-ui-main/src/features/knowledge/hooks/useKnowledgeQueries.ts (2)
useCrawlUrl(101-330)useUploadDocument(335-549)
archon-ui-main/src/features/knowledge/hooks/useKnowledgeQueries.ts (2)
archon-ui-main/src/features/knowledge/types/knowledge.ts (3)
KnowledgeItemsFilter(127-134)KnowledgeItemsResponse(98-103)KnowledgeItem(26-39)archon-ui-main/src/features/shared/optimistic.ts (1)
removeDuplicateEntities(65-74)
archon-ui-main/src/features/knowledge/views/KnowledgeViewWithBoundary.tsx (2)
archon-ui-main/src/features/knowledge/context/KnowledgeFilterContext.tsx (1)
KnowledgeFilterProvider(34-72)archon-ui-main/src/features/knowledge/views/KnowledgeView.tsx (1)
KnowledgeView(18-188)
archon-ui-main/src/features/knowledge/hooks/tests/useKnowledgeQueries.test.ts (3)
archon-ui-main/src/features/knowledge/services/knowledgeService.ts (1)
knowledgeService(23-220)archon-ui-main/src/features/knowledge/types/knowledge.ts (1)
KnowledgeItemsResponse(98-103)archon-ui-main/src/features/knowledge/hooks/useKnowledgeQueries.ts (3)
knowledgeKeys(27-47)useCrawlUrl(101-330)useUploadDocument(335-549)
🔇 Additional comments (15)
archon-ui-main/src/features/knowledge/context/index.ts (1)
6-6: LGTM: focused re-exportStraightforward barrel export; no issues.
archon-ui-main/src/features/knowledge/context/KnowledgeFilterContext.tsx (2)
78-86: Throw-on-missing-provider is goodClear failure mode when used outside the provider; no changes needed.
49-54: Confirm filter completeness for optimistic matchingOnly search and knowledge_type are propagated. If summaries are filterable by source_type or tags in other views, optimistic visibility may still mismatch. Consider including those when relevant to keep cache keys and optimistic placement aligned.
archon-ui-main/src/features/knowledge/views/KnowledgeViewWithBoundary.tsx (1)
3-3: Align with repo’s optimistic-update pattern: avoid adding a filter context layer herePer PR feedback and the patterns in useTaskQueries/useProjectQueries, prefer deriving/passing the current filter into hooks (and leveraging shared optimistic utilities) instead of introducing a new Provider boundary. This keeps KnowledgeView self-contained and avoids an extra context to maintain.
Apply after migrating KnowledgeView back to local filter derivation:
-import { KnowledgeFilterProvider } from "../context"; ... - <KnowledgeFilterProvider> - <KnowledgeView /> - </KnowledgeFilterProvider> + <KnowledgeView />Run the script below to confirm Knowledge uses the same optimistic utilities and filter-derivation approach as Tasks/Projects before removing the Provider.
Also applies to: 11-13
archon-ui-main/src/features/knowledge/components/AddKnowledgeDialog.tsx (2)
1-20: Sanity check: no dynamic require/import patternsPrior feedback requested avoiding dynamic CommonJS imports. This file is clean. Run a repo scan to confirm no remaining dynamic require usage under Knowledge.
36-37: Pass-through of currentFilter looks correct; ensure hooks use shared optimistic helpersGood that the filter scopes optimistic placement. Verify useCrawlUrl/useUploadDocument delegate to replaceOptimisticEntity/removeDuplicateEntities to avoid race conditions.
archon-ui-main/src/features/knowledge/index.ts (1)
13-14: Avoid public surface expansion until the pattern is settledRe-exporting the context from the feature index encourages external usage and makes later removal harder. Keep it internal to the feature (import from ./context where needed) until we finalize the “no-context” direction.
-// Context -export * from "./context";archon-ui-main/src/features/knowledge/context/tests/KnowledgeFilterContext.test.tsx (1)
1-200: Well-structured test coverage for the context!The test suite comprehensively covers the KnowledgeFilterContext with appropriate test cases for default state, updates, error handling, and state persistence. The implementation follows Vitest and React Testing Library patterns correctly.
archon-ui-main/src/features/knowledge/views/KnowledgeView.tsx (3)
6-6: Good migration to context-based filter state!The removal of local state in favor of the KnowledgeFilterContext correctly centralizes filter management.
Also applies to: 13-13
19-27: Clean context integration!The consumption of filter state from the context is properly implemented. This provides centralized filter management as intended.
37-37: Consider refactoring to follow established patternsBased on reviewer feedback, this implementation deviates from established patterns documented in
PRPs/ai_docs/optimistic_updates.md. The reviewer suggests:
- Using shared optimistic utilities (
replaceOptimisticEntity()andremoveDuplicateEntities()) instead of manual mapping- Avoiding the context layer - existing patterns in
useTaskQueries.tsanduseProjectQueries.tsachieve filter-awareness by passing the filter as a parameterConsider aligning with these patterns for consistency and reduced complexity.
Also applies to: 164-164
archon-ui-main/src/features/knowledge/hooks/tests/useKnowledgeQueries.test.ts (1)
253-384: Comprehensive test coverage for filter-aware optimistic updates!The tests thoroughly verify that optimistic updates target the current filter and handle both matching and non-matching scenarios appropriately.
Also applies to: 430-529
archon-ui-main/src/features/knowledge/hooks/useKnowledgeQueries.ts (3)
8-8: Good use of shared optimistic utilities!The import and usage of
removeDuplicateEntitiesaligns with the reviewer's recommendation to use established utilities.
162-189: Filter matching logic is correctly implemented!The implementation properly checks if optimistic items match the current filter based on type, tags, and search criteria before updating the cache. This provides immediate visual feedback for the active filter as intended.
Also applies to: 388-415
101-101: Consider simplifying without the context layerPer reviewer feedback, the established pattern passes filters as parameters rather than using a context layer. The current implementation with optional
currentFilterparameter is a step in the right direction, but consider whether the KnowledgeFilterContext is needed at all.The reviewer suggests looking at how
useTaskQueries.tsanduseProjectQueries.tsachieve filter-awareness without introducing a new context. Let me verify the existing patterns:Also applies to: 335-335
| const currentFilter = useMemo<KnowledgeItemsFilter>(() => { | ||
| const filter: KnowledgeItemsFilter = { | ||
| page: 1, | ||
| per_page: 100, | ||
| }; |
There was a problem hiding this comment.
🛠️ 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.
- const currentFilter = useMemo<KnowledgeItemsFilter>(() => {
- const filter: KnowledgeItemsFilter = {
- page: 1,
- per_page: 100,
- };
+ const DEFAULT_PAGE = 1;
+ const DEFAULT_PER_PAGE = 100;
+ const currentFilter = useMemo<KnowledgeItemsFilter>(() => {
+ const filter: KnowledgeItemsFilter = {
+ page: DEFAULT_PAGE,
+ per_page: DEFAULT_PER_PAGE,
+ };🏁 Script executed:
#!/bin/bash
# Ensure pagination defaults match across feature
rg -nP '\b(page|per_page)\s*:\s*\d+' archon-ui-main/src/features/knowledge --type=ts -C2Length 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:
- archon-ui-main/src/features/knowledge/context/KnowledgeFilterContext.tsx (current 1/100)
- archon-ui-main/src/features/knowledge/hooks/useKnowledgeQueries.ts (optimistic updates use 1/100)
- archon-ui-main/src/features/knowledge/hooks/tests/useKnowledgeQueries.test.ts (uses per_page: 20)
🤖 Prompt for AI Agents
In archon-ui-main/src/features/knowledge/context/KnowledgeFilterContext.tsx
around lines 39-43, the pagination defaults (page: 1, per_page: 100) are
hard-coded; extract shared constants (e.g., DEFAULT_PAGE and DEFAULT_PER_PAGE)
into a single module (or reuse the query-key builder/knowledgeKeys export if
available), replace the inline literals here with those constants, update
useKnowledgeQueries.ts to reference the same constants for optimistic updates,
and modify the test useKnowledgeQueries.test.ts to use the shared
DEFAULT_PER_PAGE (or update its expected value to match the centralized
constant) so all places import the same source of truth and avoid mismatched
cache/query keys and brittle tests.
- Resolved conflict in useKnowledgeQueries.ts to keep utility function usage (deduplicatedItems) - Resolved conflict in KnowledgeView.tsx to include both useMemo and useToast imports - Maintains PR coleam00#717 implementation using shared utilities per feedback
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
archon-ui-main/src/features/knowledge/components/AddKnowledgeDialog.tsx(2 hunks)archon-ui-main/src/features/knowledge/hooks/tests/useKnowledgeQueries.test.ts(4 hunks)archon-ui-main/src/features/knowledge/hooks/useKnowledgeQueries.ts(8 hunks)archon-ui-main/src/features/knowledge/views/KnowledgeView.tsx(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- archon-ui-main/src/features/knowledge/components/AddKnowledgeDialog.tsx
🧰 Additional context used
📓 Path-based instructions (5)
archon-ui-main/src/features/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
archon-ui-main/src/features/**/*.{ts,tsx}: Use TanStack Query for all data fetching (no prop drilling); use smart HTTP polling (no WebSockets)
Biome formatting in features: 120-character line length, double quotes, and trailing commas
Apply Tron-inspired glassmorphism styling with Tailwind for feature UIUse Biome in features: 120 character line length, double quotes, and trailing commas
Files:
archon-ui-main/src/features/knowledge/views/KnowledgeView.tsxarchon-ui-main/src/features/knowledge/hooks/tests/useKnowledgeQueries.test.tsarchon-ui-main/src/features/knowledge/hooks/useKnowledgeQueries.ts
archon-ui-main/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Disallow implicit any in TypeScript
archon-ui-main/src/**/*.{ts,tsx}: Frontend TypeScript must use strict mode with no implicit any
Use TanStack Query for all data fetching; avoid prop drilling
Use database values directly in the frontend; avoid mapping layers between BE and FE types
Files:
archon-ui-main/src/features/knowledge/views/KnowledgeView.tsxarchon-ui-main/src/features/knowledge/hooks/tests/useKnowledgeQueries.test.tsarchon-ui-main/src/features/knowledge/hooks/useKnowledgeQueries.ts
archon-ui-main/src/**/*.{ts,tsx,py}
📄 CodeRabbit inference engine (CLAUDE.md)
In code comments, avoid meta terms like SIMPLIFIED, ENHANCED, LEGACY, CHANGED, REMOVED; comment only on functionality and reasoning (do not mention beta/global rules)
Files:
archon-ui-main/src/features/knowledge/views/KnowledgeView.tsxarchon-ui-main/src/features/knowledge/hooks/tests/useKnowledgeQueries.test.tsarchon-ui-main/src/features/knowledge/hooks/useKnowledgeQueries.ts
archon-ui-main/src/**/*.test.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Write frontend tests with Vitest and React Testing Library
Files:
archon-ui-main/src/features/knowledge/hooks/tests/useKnowledgeQueries.test.ts
archon-ui-main/src/features/*/hooks/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Place TanStack Query hooks in the feature’s hooks directory
Use feature-scoped TanStack Query hooks in src/features/[feature]/hooks
Files:
archon-ui-main/src/features/knowledge/hooks/tests/useKnowledgeQueries.test.tsarchon-ui-main/src/features/knowledge/hooks/useKnowledgeQueries.ts
🧠 Learnings (3)
📚 Learning: 2025-09-19T10:32:55.557Z
Learnt from: CR
PR: coleam00/Archon#0
File: AGENTS.md:0-0
Timestamp: 2025-09-19T10:32:55.557Z
Learning: Applies to archon-ui-main/src/features/*/hooks/**/*.{ts,tsx} : Use feature-scoped TanStack Query hooks in src/features/[feature]/hooks
Applied to files:
archon-ui-main/src/features/knowledge/hooks/tests/useKnowledgeQueries.test.ts
📚 Learning: 2025-08-28T13:50:10.499Z
Learnt from: Wirasm
PR: coleam00/Archon#514
File: archon-ui-main/src/services/pollingService.ts:38-45
Timestamp: 2025-08-28T13:50:10.499Z
Learning: The pollingService.ts file is being deprecated in favor of the usePolling hook from usePolling.ts. The team has decided to consolidate on the usePolling hook pattern rather than maintaining separate polling services, as it provides better React integration and reduces code duplication.
Applied to files:
archon-ui-main/src/features/knowledge/hooks/useKnowledgeQueries.ts
📚 Learning: 2025-08-29T08:57:47.558Z
Learnt from: Wirasm
PR: coleam00/Archon#514
File: archon-ui-main/src/services/progressService.ts:0-0
Timestamp: 2025-08-29T08:57:47.558Z
Learning: The progressService.ts file was removed from the codebase in PR #514 as part of the consolidation effort to use the usePolling hook pattern from usePolling.ts instead of maintaining separate polling services. This aligns with the architectural decision to deprecate individual polling services in favor of the centralized hook-based approach.
Applied to files:
archon-ui-main/src/features/knowledge/hooks/useKnowledgeQueries.ts
🧬 Code graph analysis (3)
archon-ui-main/src/features/knowledge/views/KnowledgeView.tsx (3)
archon-ui-main/src/features/knowledge/context/KnowledgeFilterContext.tsx (1)
useKnowledgeFilter(78-86)archon-ui-main/src/features/knowledge/types/knowledge.ts (1)
KnowledgeItem(26-39)archon-ui-main/src/features/knowledge/hooks/useKnowledgeQueries.ts (1)
useKnowledgeSummaries(788-833)
archon-ui-main/src/features/knowledge/hooks/tests/useKnowledgeQueries.test.ts (4)
archon-ui-main/src/features/knowledge/services/knowledgeService.ts (1)
knowledgeService(23-220)archon-ui-main/src/features/knowledge/types/knowledge.ts (1)
KnowledgeItemsResponse(98-103)archon-ui-main/src/features/shared/config/queryClient.ts (1)
queryClient(13-48)archon-ui-main/src/features/knowledge/hooks/useKnowledgeQueries.ts (3)
knowledgeKeys(27-47)useCrawlUrl(101-329)useUploadDocument(334-546)
archon-ui-main/src/features/knowledge/hooks/useKnowledgeQueries.ts (2)
archon-ui-main/src/features/knowledge/types/knowledge.ts (3)
KnowledgeItemsFilter(127-134)KnowledgeItemsResponse(98-103)KnowledgeItem(26-39)archon-ui-main/src/features/shared/utils/optimistic.ts (1)
removeDuplicateEntities(65-74)
🔇 Additional comments (7)
archon-ui-main/src/features/knowledge/hooks/useKnowledgeQueries.ts (2)
123-126: FIXED: Comments updated to better clarify the optimistic update strategyThe comments appropriately describe the resolution to the optimistic updates issue. The implementation now correctly targets the currently viewed filter for immediate user feedback.
268-277: Consider using replaceOptimisticEntity utility as suggestedWhile the manual mapping works correctly, the shared
replaceOptimisticEntityutility would be more consistent with established patterns in other features.archon-ui-main/src/features/knowledge/views/KnowledgeView.tsx (2)
19-27: Clean migration to context-based filteringThe removal of local filter state in favor of consuming from
KnowledgeFilterContextis well-executed and follows the context provider pattern correctly.
164-164: Good prop threading for filter consistencyPassing
currentFiltertoAddKnowledgeDialogensures optimistic updates target the correct filter cache, maintaining UI consistency.archon-ui-main/src/features/knowledge/hooks/tests/useKnowledgeQueries.test.ts (3)
254-310: Well-structured test for optimistic updates with current filterThe test effectively validates that optimistic updates target the current filter's cache when the filter context is available.
531-593: Comprehensive filter integration test coverageThe test properly validates the core enhancement - that optimistic updates prioritize the current filter cache over other cached filters, and correctly filters by knowledge_type.
60-79: Remove dynamic require() - potential bundling issue!The dynamic CommonJS import could cause issues with bundlers. The vi.mock at lines 52-58 should be sufficient for mocking the context module.
Remove the entire globalThis.require override and rely on the existing ESM mock:
-// Mock the require function used in dynamic imports -const originalRequire = globalThis.require; -beforeAll(() => { - globalThis.require = vi.fn().mockImplementation((module: string) => { - if (module === "../context") { - return { - useKnowledgeFilter: () => ({ - currentFilter: mockCurrentFilter, - updateFilter: mockUpdateFilter, - isCurrentFilter: mockIsCurrentFilter, - }), - }; - } - return originalRequire?.(module); - }); -}); - -afterAll(() => { - globalThis.require = originalRequire; -});
…lign with Tasks/Projects patterns - Remove KnowledgeFilterContext entirely per reviewer feedback - Update KnowledgeView to use local state like Projects/Tasks features - Update mutations to accept optional currentFilter parameter - Remove context provider from KnowledgeViewWithBoundary - Update tests to use parameter-based approach instead of context - Remove context exports from knowledge index - All 14 Knowledge tests passing ✅ Addresses Wirasm's feedback: 1. ✅ Use shared utilities (replaceOptimisticEntity, removeDuplicateEntities) 2. ✅ Remove context layer and follow Tasks/Projects patterns 3. ✅ Remove dynamic require() usage Implementation now perfectly matches Tasks/Projects pattern with local state management.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
archon-ui-main/src/features/knowledge/views/KnowledgeView.tsx (4)
22-29: Minor polish: avoid magic number and trim search string.Keep the memo (stable queryKey), but extract page size and trim search to reduce noisy cache keys.
Apply this diff within the block:
- const currentFilter: KnowledgeItemsFilter = useMemo(() => ({ + const currentFilter: KnowledgeItemsFilter = useMemo(() => ({ page: 1, - per_page: 100, - ...(searchQuery && { search: searchQuery }), + per_page: PAGE_SIZE, + ...(searchQuery.trim() && { search: searchQuery.trim() }), ...(typeFilter && { knowledge_type: typeFilter }), }), [searchQuery, typeFilter]);Add this near the imports:
const PAGE_SIZE = 100;
38-40: Fix misleading comment (not using context here).The code uses the derived currentFilter, not a context-provided filter.
Apply this diff:
- // Fetch knowledge summaries using filter from context + // Fetch knowledge summaries using the derived currentFilter const { data, isLoading, error, refetch, setActiveCrawlIds, activeOperations } = useKnowledgeSummaries(currentFilter);
63-81: Show success toast when an op disappears (not only when last status was "completed"); refetch once.If an operation vanishes while last seen as "processing"/"storing", success isn’t shown. Also, multiple refetch() calls per render.
Apply this diff:
- // Show toast for each finished operation - finishedOps.forEach((op) => { - // Check if it was an error or success - if (op.status === "error" || op.status === "failed") { - // Show error message with details - const errorMessage = op.message || op.error || "Operation failed"; - showToast(`❌ ${errorMessage}`, "error", 7000); - } else if (op.status === "completed") { - // Show success message - const message = op.message || "Operation completed"; - showToast(`✅ ${message}`, "success", 5000); - } - - // Remove from active crawl IDs - setActiveCrawlIds((prev) => prev.filter((id) => id !== op.operation_id)); - - // Refetch summaries after any completion - refetch(); - }); + // Show toast for each finished operation and refetch once after + const hadFinished = finishedOps.length > 0; + finishedOps.forEach((op) => { + if (op.status === "error" || op.status === "failed") { + const errorMessage = op.message || op.error || "Operation failed"; + showToast(`❌ ${errorMessage}`, "error", 7000); + } else { + const message = op.message || "Operation completed"; + showToast(`✅ ${message}`, "success", 5000); + } + setActiveCrawlIds((prev) => prev.filter((id) => id !== op.operation_id)); + }); + if (hadFinished) refetch();
18-21: Debounce KnowledgeHeader search inputonChange currently calls onSearchChange directly (archon-ui-main/src/features/knowledge/components/KnowledgeHeader.tsx:54) and no debounce/useDeferredValue or debounce util was found in the repo — wrap onSearchChange with a small debounce or useDeferredValue to avoid firing a query on every keypress.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
archon-ui-main/src/features/knowledge/hooks/tests/useKnowledgeQueries.test.ts(4 hunks)archon-ui-main/src/features/knowledge/views/KnowledgeView.tsx(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- archon-ui-main/src/features/knowledge/hooks/tests/useKnowledgeQueries.test.ts
🧰 Additional context used
📓 Path-based instructions (3)
archon-ui-main/src/features/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
archon-ui-main/src/features/**/*.{ts,tsx}: Use TanStack Query for all data fetching (no prop drilling); use smart HTTP polling (no WebSockets)
Biome formatting in features: 120-character line length, double quotes, and trailing commas
Apply Tron-inspired glassmorphism styling with Tailwind for feature UIUse Biome in features: 120 character line length, double quotes, and trailing commas
Files:
archon-ui-main/src/features/knowledge/views/KnowledgeView.tsx
archon-ui-main/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Disallow implicit any in TypeScript
archon-ui-main/src/**/*.{ts,tsx}: Frontend TypeScript must use strict mode with no implicit any
Use TanStack Query for all data fetching; avoid prop drilling
Use database values directly in the frontend; avoid mapping layers between BE and FE types
Files:
archon-ui-main/src/features/knowledge/views/KnowledgeView.tsx
archon-ui-main/src/**/*.{ts,tsx,py}
📄 CodeRabbit inference engine (CLAUDE.md)
In code comments, avoid meta terms like SIMPLIFIED, ENHANCED, LEGACY, CHANGED, REMOVED; comment only on functionality and reasoning (do not mention beta/global rules)
Files:
archon-ui-main/src/features/knowledge/views/KnowledgeView.tsx
🧬 Code graph analysis (1)
archon-ui-main/src/features/knowledge/views/KnowledgeView.tsx (2)
archon-ui-main/src/features/knowledge/types/knowledge.ts (2)
KnowledgeItemsFilter(127-134)KnowledgeItem(26-39)archon-ui-main/src/features/knowledge/hooks/useKnowledgeQueries.ts (1)
useKnowledgeSummaries(788-833)
🔇 Additional comments (1)
archon-ui-main/src/features/knowledge/views/KnowledgeView.tsx (1)
166-166: Approve — currentFilter prop and hooks verified
AddKnowledgeDialogProps declares currentFilter?: KnowledgeItemsFilter and both useCrawlUrl and useUploadDocument accept an optional currentFilter; tests exercise both cases.
- Extract PAGE_SIZE constant instead of magic number 100 - Fix misleading comment about 'filter from context' - Improve toast notification logic for completed operations - Add search input debouncing for better UX All 38 Knowledge tests passing.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
archon-ui-main/src/features/knowledge/views/KnowledgeView.tsx (5)
17-19: Confirm PAGE_SIZE alignment with API caps.Verify that 100 matches the backend’s max page size to avoid silent server-side clamping or partial pages. If there’s a shared page-size constant for other features, reuse it for consistency.
35-42: Stabilize filter object shape for deterministic query keys.Building the object with conditional spreads can reorder keys and create different query hashes across transitions. Prefer explicit keys with undefined for absent fields.
- const currentFilter: KnowledgeItemsFilter = useMemo(() => ({ - page: 1, - per_page: PAGE_SIZE, - ...(debouncedSearchQuery && { search: debouncedSearchQuery }), - ...(typeFilter && { knowledge_type: typeFilter }), - }), [debouncedSearchQuery, typeFilter]); + const currentFilter: KnowledgeItemsFilter = useMemo( + () => ({ + page: 1, + per_page: PAGE_SIZE, + search: debouncedSearchQuery || undefined, + knowledge_type: typeFilter ?? undefined, + }), + [debouncedSearchQuery, typeFilter], + );
93-95: Refetch once after processing all finished ops.Calling
refetch()inside the loop can trigger N requests. Move it after the loop.- finishedOps.forEach((op) => { + finishedOps.forEach((op) => { ... - refetch(); - }); + }); + if (finishedOps.length > 0) refetch();
169-173: Deduplicate tracked progress IDs.Prevent duplicate IDs if
onRefreshStartedfires more than once for the same op.- onRefreshStarted={(progressId) => { - // Add the progress ID to track it - setActiveCrawlIds((prev) => [...prev, progressId]); - }} + onRefreshStarted={(progressId) => { + // Add the progress ID to track it (deduped) + setActiveCrawlIds((prev) => + prev.includes(progressId) ? prev : [...prev, progressId] + ); + }}
26-34: Extract debounce logic into a reusableuseDebouncedValuehook
Inarchon-ui-main/src/features/knowledge/views/KnowledgeView.tsx, replace the inlineuseEffectdebounce with:const debouncedSearchQuery = useDebouncedValue(searchQuery, SEARCH_DEBOUNCE_MS);If it doesn’t yet exist, add it under
src/features/shared/hooks/useDebouncedValue.tsas:import { useEffect, useState } from "react"; export function useDebouncedValue<T>(value: T, delayMs: number) { const [debounced, setDebounced] = useState(value); useEffect(() => { const id = setTimeout(() => setDebounced(value), delayMs); return () => clearTimeout(id); }, [value, delayMs]); return debounced; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
archon-ui-main/src/features/knowledge/views/KnowledgeView.tsx(3 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
archon-ui-main/src/features/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
archon-ui-main/src/features/**/*.{ts,tsx}: Use TanStack Query for all data fetching (no prop drilling); use smart HTTP polling (no WebSockets)
Biome formatting in features: 120-character line length, double quotes, and trailing commas
Apply Tron-inspired glassmorphism styling with Tailwind for feature UIUse Biome in features: 120 character line length, double quotes, and trailing commas
Files:
archon-ui-main/src/features/knowledge/views/KnowledgeView.tsx
archon-ui-main/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Disallow implicit any in TypeScript
archon-ui-main/src/**/*.{ts,tsx}: Frontend TypeScript must use strict mode with no implicit any
Use TanStack Query for all data fetching; avoid prop drilling
Use database values directly in the frontend; avoid mapping layers between BE and FE types
Files:
archon-ui-main/src/features/knowledge/views/KnowledgeView.tsx
archon-ui-main/src/**/*.{ts,tsx,py}
📄 CodeRabbit inference engine (CLAUDE.md)
In code comments, avoid meta terms like SIMPLIFIED, ENHANCED, LEGACY, CHANGED, REMOVED; comment only on functionality and reasoning (do not mention beta/global rules)
Files:
archon-ui-main/src/features/knowledge/views/KnowledgeView.tsx
🧬 Code graph analysis (1)
archon-ui-main/src/features/knowledge/views/KnowledgeView.tsx (2)
archon-ui-main/src/features/knowledge/types/knowledge.ts (2)
KnowledgeItemsFilter(127-134)KnowledgeItem(26-39)archon-ui-main/src/features/knowledge/hooks/useKnowledgeQueries.ts (1)
useKnowledgeSummaries(788-833)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Backend Tests (Python + pytest)
🔇 Additional comments (3)
archon-ui-main/src/features/knowledge/views/KnowledgeView.tsx (3)
21-25: LGTM: Local filter state matches Tasks/Projects pattern.Nice move away from the context layer here; keeps the view self-contained while still passing the filter where needed.
51-53: LGTM: Query is correctly scoped to currentFilter with smart polling upstream.
180-180: LGTM: Passing currentFilter into AddKnowledgeDialog.Aligns with the “no context” pattern and enables targeted optimistic updates downstream.
| } else { | ||
| // Show success message for any completed operation (not just "completed" status) | ||
| const operationType = op.operation_type || "Operation"; | ||
| const successMessage = op.message || `${operationType} completed successfully`; | ||
| showToast(`✅ ${successMessage}`, "success", 5000); | ||
| } |
There was a problem hiding this comment.
Avoid false “success” toasts when an op disappears due to polling gaps.
Treating any non-error final state as success can misfire if the op temporarily drops from activeOperations. Gate success on explicit terminal success states only; otherwise skip or use a neutral toast.
- } else {
- // Show success message for any completed operation (not just "completed" status)
- const operationType = op.operation_type || "Operation";
- const successMessage = op.message || `${operationType} completed successfully`;
- showToast(`✅ ${successMessage}`, "success", 5000);
- }
+ } else if (op.status === "completed" || op.status === "document_storage") {
+ const operationType = op.operation_type || "Operation";
+ const successMessage = op.message || `${operationType} completed successfully`;
+ showToast(`✅ ${successMessage}`, "success", 5000);
+ }
+ // else: skip toast to avoid false positives when polling briefly drops the op📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| } else { | |
| // Show success message for any completed operation (not just "completed" status) | |
| const operationType = op.operation_type || "Operation"; | |
| const successMessage = op.message || `${operationType} completed successfully`; | |
| showToast(`✅ ${successMessage}`, "success", 5000); | |
| } | |
| } else if (op.status === "completed" || op.status === "document_storage") { | |
| const operationType = op.operation_type || "Operation"; | |
| const successMessage = op.message || `${operationType} completed successfully`; | |
| showToast(`✅ ${successMessage}`, "success", 5000); | |
| } | |
| // else: skip toast to avoid false positives when polling briefly drops the op |
🤖 Prompt for AI Agents
In archon-ui-main/src/features/knowledge/views/KnowledgeView.tsx around lines
83-88, the current else branch shows a success toast for any non-error final
state which can produce false positives if an op vanishes due to polling gaps;
change the logic to only show a success toast when the operation has an explicit
terminal-success status (e.g., op.status === 'completed' or other defined
success statuses), otherwise do not show a success toast (or show a neutral/info
toast if you prefer). Implement the check against explicit success states before
building the successMessage and calling showToast, and leave other
terminal/non-present cases silent or neutral.
- Remove redundant onOpenChange(false) call from crawl handler to avoid double-closing dialog - Replace inline color style with Tailwind utility class (text-cyan-600) - Fix JSON.stringify query key comparisons with proper deep equality checks - Update context types and use proper optimistic entity utilities - Remove outdated context-related comments and update with current implementation All 38 Knowledge tests passing. Implementation now fully complies with established patterns and CodeRabbit feedback.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
archon-ui-main/src/features/knowledge/hooks/useKnowledgeQueries.ts (5)
166-171: DRY the filter-matching logicmatchesType/matchesTags/matchesSearch are duplicated in four places. Extract a small helper to ensure consistent semantics and reduce risk of drift.
Add this helper near the top (outside these ranges):
function itemMatchesFilter(item: KnowledgeItem & Partial<{ metadata: { tags?: string[] } }>, filter?: KnowledgeItemsFilter) { if (!filter) return true; const tags = item.metadata?.tags ?? []; return ( (!filter.knowledge_type || item.knowledge_type === filter.knowledge_type) && (!filter.source_type || item.source_type === filter.source_type) && (!filter.tags || filter.tags.every((t) => tags.includes(t))) && (!filter.search || item.title.toLowerCase().includes(filter.search.toLowerCase())) ); }Then replace the repeated checks with:
-const matchesType = ... -const matchesTags = ... -const matchesSearch = ... -if (matchesType && matchesTags && matchesSearch) { +if (itemMatchesFilter(optimisticItem, currentFilter)) {and similarly where filter is derived from qk.
Also applies to: 214-221, 412-415, 459-466
174-179: Use existing per_page when seeding cache entriesHardcoding per_page: 100 can desync pagination. Prefer currentData?.per_page or filter?.per_page fallback.
- queryClient.setQueryData<KnowledgeItemsResponse>(currentQueryKey, { + queryClient.setQueryData<KnowledgeItemsResponse>(currentQueryKey, { items: [optimisticItem], total: 1, page: 1, - per_page: 100, + per_page: currentData?.per_page ?? currentFilter?.per_page ?? 100, });and
- queryClient.setQueryData<KnowledgeItemsResponse>(qk, { + queryClient.setQueryData<KnowledgeItemsResponse>(qk, { items: [optimisticItem], total: 1, page: 1, - per_page: 100, + per_page: old?.per_page ?? (filter?.per_page ?? 100), });Also applies to: 221-227, 419-423, 468-472
274-277: Remove shadowed/unused serverItemThis outer serverItem is unused and shadowed by the inner declaration; safe to delete.
- // Create the server entity with real progress ID - const serverItem: Partial<KnowledgeItem> = { - source_id: response.progressId, - };
110-115: Tighten mutation context type: drop unused previousKnowledgepreviousKnowledge is not set in onMutate nor read later. Remove to keep the context lean.
- previousKnowledge?: KnowledgeItem[]; previousSummaries?: Array<[readonly unknown[], KnowledgeItemsResponse | undefined]>; previousOperations?: ActiveOperationsResponse; tempProgressId: string; optimisticId: string;
278-301: Consistency note: deduplication uses id, not source_id
- Verified: removeDuplicateEntities deduplicates by entity.id (archon-ui-main/src/features/shared/utils/optimistic.ts). useKnowledgeQueries calls replaceOptimisticEntity(..., context.optimisticId, serverItem) then removeDuplicateEntities(...) (archon-ui-main/src/features/knowledge/hooks/useKnowledgeQueries.ts — ~lines 294–296 and 533–535).
- Impact: if the backend assigns a different id than the optimistic id, a duplicate can persist until the next fetch.
- Recommendation: if server ids are stable, no change required; otherwise either set the replaced entity’s id to the server-returned id before deduplication or dedupe by source_id (or by id+source_id) during reconciliation.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
archon-ui-main/src/features/knowledge/components/AddKnowledgeDialog.tsx(3 hunks)archon-ui-main/src/features/knowledge/hooks/useKnowledgeQueries.ts(10 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- archon-ui-main/src/features/knowledge/components/AddKnowledgeDialog.tsx
🧰 Additional context used
📓 Path-based instructions (4)
archon-ui-main/src/features/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
archon-ui-main/src/features/**/*.{ts,tsx}: Use TanStack Query for all data fetching (no prop drilling); use smart HTTP polling (no WebSockets)
Biome formatting in features: 120-character line length, double quotes, and trailing commas
Apply Tron-inspired glassmorphism styling with Tailwind for feature UIUse Biome in features: 120 character line length, double quotes, and trailing commas
Files:
archon-ui-main/src/features/knowledge/hooks/useKnowledgeQueries.ts
archon-ui-main/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Disallow implicit any in TypeScript
archon-ui-main/src/**/*.{ts,tsx}: Frontend TypeScript must use strict mode with no implicit any
Use TanStack Query for all data fetching; avoid prop drilling
Use database values directly in the frontend; avoid mapping layers between BE and FE types
Files:
archon-ui-main/src/features/knowledge/hooks/useKnowledgeQueries.ts
archon-ui-main/src/features/*/hooks/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Place TanStack Query hooks in the feature’s hooks directory
Use feature-scoped TanStack Query hooks in src/features/[feature]/hooks
Files:
archon-ui-main/src/features/knowledge/hooks/useKnowledgeQueries.ts
archon-ui-main/src/**/*.{ts,tsx,py}
📄 CodeRabbit inference engine (CLAUDE.md)
In code comments, avoid meta terms like SIMPLIFIED, ENHANCED, LEGACY, CHANGED, REMOVED; comment only on functionality and reasoning (do not mention beta/global rules)
Files:
archon-ui-main/src/features/knowledge/hooks/useKnowledgeQueries.ts
🧠 Learnings (2)
📚 Learning: 2025-08-28T13:50:10.499Z
Learnt from: Wirasm
PR: coleam00/Archon#514
File: archon-ui-main/src/services/pollingService.ts:38-45
Timestamp: 2025-08-28T13:50:10.499Z
Learning: The pollingService.ts file is being deprecated in favor of the usePolling hook from usePolling.ts. The team has decided to consolidate on the usePolling hook pattern rather than maintaining separate polling services, as it provides better React integration and reduces code duplication.
Applied to files:
archon-ui-main/src/features/knowledge/hooks/useKnowledgeQueries.ts
📚 Learning: 2025-08-29T08:57:47.558Z
Learnt from: Wirasm
PR: coleam00/Archon#514
File: archon-ui-main/src/services/progressService.ts:0-0
Timestamp: 2025-08-29T08:57:47.558Z
Learning: The progressService.ts file was removed from the codebase in PR #514 as part of the consolidation effort to use the usePolling hook pattern from usePolling.ts instead of maintaining separate polling services. This aligns with the architectural decision to deprecate individual polling services in favor of the centralized hook-based approach.
Applied to files:
archon-ui-main/src/features/knowledge/hooks/useKnowledgeQueries.ts
🧬 Code graph analysis (1)
archon-ui-main/src/features/knowledge/hooks/useKnowledgeQueries.ts (2)
archon-ui-main/src/features/knowledge/types/knowledge.ts (3)
KnowledgeItemsFilter(127-134)KnowledgeItemsResponse(98-103)KnowledgeItem(26-39)archon-ui-main/src/features/shared/utils/optimistic.ts (2)
replaceOptimisticEntity(48-59)removeDuplicateEntities(65-74)
🔇 Additional comments (3)
archon-ui-main/src/features/knowledge/hooks/useKnowledgeQueries.ts (3)
10-10: Good: standardized optimistic utilities adoptedImporting replaceOptimisticEntity and removeDuplicateEntities aligns with the shared optimistic pattern and addresses earlier feedback.
101-116: API shape aligns with feature patterns (filter passed to hooks)Accepting currentFilter in the hook (vs. adding a global context here) matches our Tasks/Projects pattern and reduces coupling.
854-862: Good: smart polling integratedUsing useSmartPolling based on active operations aligns with the deprecation of pollingService and the consolidated hook pattern noted in prior learnings.
- Add itemMatchesFilter helper to DRY repeated filter logic - Use dynamic per_page values instead of hardcoding 100 - Remove unused serverItem variable and previousKnowledge context field - Maintain existing deduplication by id approach (confirmed correct) All 38 Knowledge feature tests passing. Addresses CodeRabbit nitpick comments from latest review.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
archon-ui-main/src/features/knowledge/hooks/useKnowledgeQueries.ts (3)
26-38: Broaden filter matching (search across URL/description; case‑insensitive tags).Improves parity with typical backend search and reduces surprising misses.
Apply:
-function itemMatchesFilter(item: KnowledgeItem & Partial<{ metadata: { tags?: string[] } }>, filter?: KnowledgeItemsFilter) { - if (!filter) return true; - const tags = item.metadata?.tags ?? []; - return ( - (!filter.knowledge_type || item.knowledge_type === filter.knowledge_type) && - (!filter.source_type || item.source_type === filter.source_type) && - (!filter.tags || filter.tags.every((t) => tags.includes(t))) && - (!filter.search || item.title.toLowerCase().includes(filter.search.toLowerCase())) - ); -} +function itemMatchesFilter( + item: KnowledgeItem & Partial<{ metadata: { tags?: string[]; description?: string } }>, + filter?: KnowledgeItemsFilter, +) { + if (!filter) return true; + const itemTags = (item.metadata?.tags ?? []).map((t) => t.toLowerCase()); + const search = filter.search?.toLowerCase(); + const title = item.title?.toLowerCase() ?? ""; + const url = item.url?.toLowerCase() ?? ""; + const description = item.metadata?.description?.toLowerCase() ?? ""; + const matchesSearch = !search || title.includes(search) || url.includes(search) || description.includes(search); + const matchesTags = !filter.tags || filter.tags.every((t) => itemTags.includes(t.toLowerCase())); + return ( + (!filter.knowledge_type || item.knowledge_type === filter.knowledge_type) && + (!filter.source_type || item.source_type === filter.source_type) && + matchesTags && + matchesSearch + ); +}
136-139: Remove meta keyword in comments per guidelines.Avoid “FIXED:” in comments.
- // FIXED: Optimistic updates now target the currently viewed filter - // Optimistic updates target the currently viewed filter by - // checking if the new item matches the filter passed to the hook. + // Optimistic updates target the currently viewed filter by checking if the new item matches the filter passed in.
205-220: Optional: de-duplicate the filter equality logic via a small helper.Reduces copy/paste and future drift.
Add near itemMatchesFilter:
function filtersEqual(a?: KnowledgeItemsFilter, b?: KnowledgeItemsFilter): boolean { if (!a && !b) return true; if (!a || !b) return false; const eqTags = (a.tags?.length ?? 0) === (b.tags?.length ?? 0) && (a.tags ?? []).every((t, i) => t === (b.tags ?? [])[i]); return ( a.page === b.page && a.per_page === b.per_page && a.search === b.search && a.knowledge_type === b.knowledge_type && a.source_type === b.source_type && eqTags ); }Then replace the inlined deep-compare with
filtersEqual(qkFilter, currentFilterPart).Also applies to: 436-451
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
archon-ui-main/src/features/knowledge/hooks/useKnowledgeQueries.ts(11 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
archon-ui-main/src/features/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
archon-ui-main/src/features/**/*.{ts,tsx}: Use TanStack Query for all data fetching (no prop drilling); use smart HTTP polling (no WebSockets)
Biome formatting in features: 120-character line length, double quotes, and trailing commas
Apply Tron-inspired glassmorphism styling with Tailwind for feature UIUse Biome in features: 120 character line length, double quotes, and trailing commas
Files:
archon-ui-main/src/features/knowledge/hooks/useKnowledgeQueries.ts
archon-ui-main/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Disallow implicit any in TypeScript
archon-ui-main/src/**/*.{ts,tsx}: Frontend TypeScript must use strict mode with no implicit any
Use TanStack Query for all data fetching; avoid prop drilling
Use database values directly in the frontend; avoid mapping layers between BE and FE types
Files:
archon-ui-main/src/features/knowledge/hooks/useKnowledgeQueries.ts
archon-ui-main/src/features/*/hooks/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Place TanStack Query hooks in the feature’s hooks directory
Use feature-scoped TanStack Query hooks in src/features/[feature]/hooks
Files:
archon-ui-main/src/features/knowledge/hooks/useKnowledgeQueries.ts
archon-ui-main/src/**/*.{ts,tsx,py}
📄 CodeRabbit inference engine (CLAUDE.md)
In code comments, avoid meta terms like SIMPLIFIED, ENHANCED, LEGACY, CHANGED, REMOVED; comment only on functionality and reasoning (do not mention beta/global rules)
Files:
archon-ui-main/src/features/knowledge/hooks/useKnowledgeQueries.ts
🧠 Learnings (2)
📚 Learning: 2025-08-28T13:50:10.499Z
Learnt from: Wirasm
PR: coleam00/Archon#514
File: archon-ui-main/src/services/pollingService.ts:38-45
Timestamp: 2025-08-28T13:50:10.499Z
Learning: The pollingService.ts file is being deprecated in favor of the usePolling hook from usePolling.ts. The team has decided to consolidate on the usePolling hook pattern rather than maintaining separate polling services, as it provides better React integration and reduces code duplication.
Applied to files:
archon-ui-main/src/features/knowledge/hooks/useKnowledgeQueries.ts
📚 Learning: 2025-08-29T08:57:47.558Z
Learnt from: Wirasm
PR: coleam00/Archon#514
File: archon-ui-main/src/services/progressService.ts:0-0
Timestamp: 2025-08-29T08:57:47.558Z
Learning: The progressService.ts file was removed from the codebase in PR #514 as part of the consolidation effort to use the usePolling hook pattern from usePolling.ts instead of maintaining separate polling services. This aligns with the architectural decision to deprecate individual polling services in favor of the centralized hook-based approach.
Applied to files:
archon-ui-main/src/features/knowledge/hooks/useKnowledgeQueries.ts
🧬 Code graph analysis (1)
archon-ui-main/src/features/knowledge/hooks/useKnowledgeQueries.ts (2)
archon-ui-main/src/features/knowledge/types/knowledge.ts (3)
KnowledgeItem(26-39)KnowledgeItemsFilter(127-134)KnowledgeItemsResponse(98-103)archon-ui-main/src/features/shared/utils/optimistic.ts (2)
replaceOptimisticEntity(48-59)removeDuplicateEntities(65-74)
🔇 Additional comments (7)
archon-ui-main/src/features/knowledge/hooks/useKnowledgeQueries.ts (7)
10-10: Good: consolidated optimistic utils with ESM imports.This aligns with shared utilities and the reviewer guidance.
353-367: API surface change looks good (filter-aware upload).Consistent with reviewer guidance to pass filter to hooks rather than using context.
405-427: LGTM: prioritized update of current filter cache.Good UX: immediate feedback in the active view.
110-110: No-op change.Blank line; nothing to review.
205-220: Fix skip-check: include tags and source_type in filter equality (prevents missed optimistic updates).This can falsely “skip” updating the current cache when only tags/source_type differ.
- if (currentQueryKey && qk.length === currentQueryKey.length && - qk.every((part, index) => { + if (currentQueryKey && qk.length === currentQueryKey.length && + qk.every((part, index) => { if (index < qk.length - 1) return part === currentQueryKey[index]; // For the filter object, do a deep comparison const qkFilter = part as KnowledgeItemsFilter | undefined; const currentFilterPart = currentQueryKey[index] as KnowledgeItemsFilter | undefined; if (!qkFilter && !currentFilterPart) return true; if (!qkFilter || !currentFilterPart) return false; - return ( - qkFilter.page === currentFilterPart.page && - qkFilter.per_page === currentFilterPart.per_page && - qkFilter.search === currentFilterPart.search && - qkFilter.knowledge_type === currentFilterPart.knowledge_type - ); + const eqTags = + (qkFilter.tags?.length ?? 0) === (currentFilterPart.tags?.length ?? 0) && + (qkFilter.tags ?? []).every((t, i) => t === (currentFilterPart.tags ?? [])[i]); + return ( + qkFilter.page === currentFilterPart.page && + qkFilter.per_page === currentFilterPart.per_page && + qkFilter.search === currentFilterPart.search && + qkFilter.knowledge_type === currentFilterPart.knowledge_type && + qkFilter.source_type === currentFilterPart.source_type && + eqTags + ); })) { continue; }
436-451: Fix skip-check (upload path): include tags and source_type.Mirror the crawl fix to avoid false skips here too.
- return ( - qkFilter.page === currentFilterPart.page && - qkFilter.per_page === currentFilterPart.per_page && - qkFilter.search === currentFilterPart.search && - qkFilter.knowledge_type === currentFilterPart.knowledge_type - ); + const eqTags = + (qkFilter.tags?.length ?? 0) === (currentFilterPart.tags?.length ?? 0) && + (qkFilter.tags ?? []).every((t, i) => t === (currentFilterPart.tags ?? [])[i]); + return ( + qkFilter.page === currentFilterPart.page && + qkFilter.per_page === currentFilterPart.per_page && + qkFilter.search === currentFilterPart.search && + qkFilter.knowledge_type === currentFilterPart.knowledge_type && + qkFilter.source_type === currentFilterPart.source_type && + eqTags + );
844-852: Confirm: useSmartPolling is canonical.
useSmartPolling is implemented at archon-ui-main/src/features/shared/hooks/useSmartPolling.ts (exported via src/features/shared/hooks/index.ts) and is imported across the codebase (knowledge, projects, progress, settings, etc.); no usePolling references found.
| const filter = qk[qk.length - 1] as KnowledgeItemsFilter | undefined; | ||
| const matchesType = !filter?.knowledge_type || optimisticItem.knowledge_type === filter.knowledge_type; | ||
| const matchesTags = | ||
| !filter?.tags || filter.tags.every((t) => (optimisticItem.metadata?.tags ?? []).includes(t)); | ||
| if (!(matchesType && matchesTags)) continue; | ||
| if (!itemMatchesFilter(optimisticItem, filter)) continue; | ||
| if (!old) { | ||
| queryClient.setQueryData<KnowledgeItemsResponse>(qk, { | ||
| items: [optimisticItem], | ||
| total: 1, | ||
| page: 1, | ||
| per_page: 100, | ||
| per_page: filter?.per_page ?? 100, | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
🧩 Analysis chain
Confirm per_page default.
Using 100 as a fallback may diverge from API defaults and skew pagination counts.
🏁 Script executed:
#!/bin/bash
# Find the default per_page used by knowledge summaries on the client and service
rg -nP --type=ts -C2 '\bper_page\b' archon-ui-main | sed -n '1,120p'
rg -nP --type=ts -C3 'getKnowledgeSummaries\s*\(' archon-ui-mainLength of output: 16990
Confirm per_page default — don't hardcode 100
Optimistic-update fallbacks use "?? 100" while the UI view uses PAGE_SIZE and the service only appends per_page when provided; this can mismatch the API default and skew pagination totals. Confirm the server's default per_page and make client fallbacks consistent (use the shared PAGE_SIZE constant or omit per_page to rely on the API).
Locations:
- archon-ui-main/src/features/knowledge/hooks/useKnowledgeQueries.ts — per_page fallback occurrences at lines ~186, ~230, ~417, ~461.
- archon-ui-main/src/features/knowledge/views/KnowledgeView.tsx — currentFilter sets per_page: PAGE_SIZE (line ~38).
- archon-ui-main/src/features/knowledge/services/knowledgeService.ts — getKnowledgeSummaries only appends per_page when filter?.per_page (lines ~31-33).
🤖 Prompt for AI Agents
In archon-ui-main/src/features/knowledge/hooks/useKnowledgeQueries.ts around
lines 223 to 231 (and similarly at ~186, ~417, ~461), the optimistic-update uses
a hardcoded per_page fallback of 100 which can diverge from the UI PAGE_SIZE and
server default; change these fallbacks to use the shared PAGE_SIZE constant
(import it from the module where it's defined) or remove per_page from the
optimistic payload to let the API apply its default, and update all listed
occurrences so optimistic responses and pagination remain consistent with
KnowledgeView and knowledgeService behavior.
| // Replace temporary IDs with real ones from the server using shared utilities | ||
| if (context) { | ||
| // Update summaries cache with real progress ID | ||
| // Update summaries cache using replaceOptimisticEntity and removeDuplicateEntities | ||
| queryClient.setQueriesData<KnowledgeItemsResponse>({ queryKey: knowledgeKeys.summariesPrefix() }, (old) => { | ||
| if (!old) return old; | ||
|
|
||
| // Find the optimistic item to create server entity with updated data | ||
| const optimisticItem = old.items.find(item => item._localId === context.optimisticId); | ||
| if (!optimisticItem) return old; | ||
|
|
||
| const serverItem: KnowledgeItem = { | ||
| ...optimisticItem, | ||
| source_id: response.progressId, | ||
| _optimistic: false, | ||
| _localId: undefined, | ||
| } as KnowledgeItem; | ||
|
|
||
| // Replace the optimistic entity with server data | ||
| const replacedItems = replaceOptimisticEntity(old.items, context.optimisticId, serverItem); | ||
| // Remove any duplicates that might have been created | ||
| const deduplicatedItems = removeDuplicateEntities(replacedItems); | ||
|
|
||
| return { | ||
| ...old, | ||
| items: old.items.map((item) => { | ||
| if (item.source_id === context.tempProgressId) { | ||
| return { | ||
| ...item, | ||
| source_id: response.progressId, | ||
| }; | ||
| } | ||
| return item; | ||
| }), | ||
| items: deduplicatedItems, | ||
| }; | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
🧩 Analysis chain
LGTM on shared optimistic reconciliation; consider secondary dedupe by source_id.
If server returns a different id but identical source_id, you can still end up with duplicates after polling.
// Remove any duplicates that might have been created
- const deduplicatedItems = removeDuplicateEntities(replacedItems);
+ const deduplicatedItems = removeDuplicateEntities(replacedItems);
+ // Secondary dedupe by source_id to guard against id != source_id during reconciliation
+ const seenSourceIds = new Set<string>();
+ const dedupBySourceId = deduplicatedItems.filter((i) => {
+ if (seenSourceIds.has(i.source_id)) return false;
+ seenSourceIds.add(i.source_id);
+ return true;
+ });
return {
...old,
- items: deduplicatedItems,
+ items: dedupBySourceId,
};To verify whether id can differ from source_id in this feature’s data, run:
Also applies to: 508-531
🏁 Script executed:
#!/bin/bash
# Inspect usages to confirm which field is treated as canonical
rg -nP -C2 '\b(source_id|id)\b' archon-ui-main/src/features/knowledge | sed -n '1,200p'Length of output: 18606
Add secondary dedupe by source_id after optimistic reconciliation
Codebase treats source_id as the canonical identifier in many places (KnowledgeList / KnowledgeTable / KnowledgeView / getActiveOperationForItem); dedupe by source_id to avoid duplicates when server id != source_id.
Location: archon-ui-main/src/features/knowledge/hooks/useKnowledgeQueries.ts lines 275-301 (also apply same change at 508-531).
// Remove any duplicates that might have been created
- const deduplicatedItems = removeDuplicateEntities(replacedItems);
+ const deduplicatedItems = removeDuplicateEntities(replacedItems);
+ // Secondary dedupe by source_id to guard against id != source_id during reconciliation
+ const seenSourceIds = new Set<string>();
+ const dedupBySourceId = deduplicatedItems.filter((i) => {
+ if (seenSourceIds.has(i.source_id)) return false;
+ seenSourceIds.add(i.source_id);
+ return true;
+ });
return {
...old,
- items: deduplicatedItems,
+ items: dedupBySourceId,
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Replace temporary IDs with real ones from the server using shared utilities | |
| if (context) { | |
| // Update summaries cache with real progress ID | |
| // Update summaries cache using replaceOptimisticEntity and removeDuplicateEntities | |
| queryClient.setQueriesData<KnowledgeItemsResponse>({ queryKey: knowledgeKeys.summariesPrefix() }, (old) => { | |
| if (!old) return old; | |
| // Find the optimistic item to create server entity with updated data | |
| const optimisticItem = old.items.find(item => item._localId === context.optimisticId); | |
| if (!optimisticItem) return old; | |
| const serverItem: KnowledgeItem = { | |
| ...optimisticItem, | |
| source_id: response.progressId, | |
| _optimistic: false, | |
| _localId: undefined, | |
| } as KnowledgeItem; | |
| // Replace the optimistic entity with server data | |
| const replacedItems = replaceOptimisticEntity(old.items, context.optimisticId, serverItem); | |
| // Remove any duplicates that might have been created | |
| const deduplicatedItems = removeDuplicateEntities(replacedItems); | |
| return { | |
| ...old, | |
| items: old.items.map((item) => { | |
| if (item.source_id === context.tempProgressId) { | |
| return { | |
| ...item, | |
| source_id: response.progressId, | |
| }; | |
| } | |
| return item; | |
| }), | |
| items: deduplicatedItems, | |
| }; | |
| }); | |
| // Replace temporary IDs with real ones from the server using shared utilities | |
| if (context) { | |
| // Update summaries cache using replaceOptimisticEntity and removeDuplicateEntities | |
| queryClient.setQueriesData<KnowledgeItemsResponse>({ queryKey: knowledgeKeys.summariesPrefix() }, (old) => { | |
| if (!old) return old; | |
| // Find the optimistic item to create server entity with updated data | |
| const optimisticItem = old.items.find(item => item._localId === context.optimisticId); | |
| if (!optimisticItem) return old; | |
| const serverItem: KnowledgeItem = { | |
| ...optimisticItem, | |
| source_id: response.progressId, | |
| _optimistic: false, | |
| _localId: undefined, | |
| } as KnowledgeItem; | |
| // Replace the optimistic entity with server data | |
| const replacedItems = replaceOptimisticEntity(old.items, context.optimisticId, serverItem); | |
| // Remove any duplicates that might have been created | |
| const deduplicatedItems = removeDuplicateEntities(replacedItems); | |
| // Secondary dedupe by source_id to guard against id != source_id during reconciliation | |
| const seenSourceIds = new Set<string>(); | |
| const dedupBySourceId = deduplicatedItems.filter((i) => { | |
| if (seenSourceIds.has(i.source_id)) return false; | |
| seenSourceIds.add(i.source_id); | |
| return true; | |
| }); | |
| return { | |
| ...old, | |
| items: dedupBySourceId, | |
| }; | |
| }); |
🤖 Prompt for AI Agents
In archon-ui-main/src/features/knowledge/hooks/useKnowledgeQueries.ts around
lines 275 to 301, the optimistic reconciliation replaces temporary items but may
still leave duplicates when the server-created entity has a different source_id;
update the dedupe step to perform a secondary deduplication keyed by source_id
(after removeDuplicateEntities) so any items sharing the same source_id are
collapsed to a single canonical entry (prefer the serverItem for conflicts), and
apply the same change at the other reconciliation site around lines 508 to 531.
- Broaden filter matching to search title, URL, description with case-insensitive tags - Remove 'FIXED:' meta keyword from comments per guidelines - Add filtersEqual() helper function to reduce duplication - Fix skip-check logic to include tags and source_type in both crawl and upload paths Resolves all technical CodeRabbit nitpick comments while maintaining test compatibility.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
archon-ui-main/src/features/knowledge/hooks/useKnowledgeQueries.ts (5)
26-53: Tag matching likely too permissive; use equality (case‑insensitive) instead of substring.Substring matching for tags can cause false positives and show items under filters they shouldn’t match. Prefer exact tag equality (case‑insensitive).
Apply this diff:
-function itemMatchesFilter(item: KnowledgeItem & Partial<{ metadata: { tags?: string[] } }>, filter?: KnowledgeItemsFilter) { +function itemMatchesFilter(item: KnowledgeItem & Partial<{ metadata: { tags?: string[] } }>, filter?: KnowledgeItemsFilter) { if (!filter) return true; - const tags = item.metadata?.tags ?? []; + const itemTags = (item.metadata?.tags ?? []).map((t) => t.toLowerCase()); + const filterTags = (filter.tags ?? []).map((t) => t.toLowerCase()); // Check basic filter criteria const basicMatch = ( (!filter.knowledge_type || item.knowledge_type === filter.knowledge_type) && (!filter.source_type || item.source_type === filter.source_type) && - (!filter.tags || filter.tags.every((t) => tags.some(tag => tag.toLowerCase().includes(t.toLowerCase())))) + (!filter.tags || filterTags.every((t) => itemTags.includes(t))) ); // Enhanced search across title, URL, description, and metadata if (filter.search && basicMatch) { const searchTerm = filter.search.toLowerCase(); const title = item.title?.toLowerCase() || ''; const url = item.url?.toLowerCase() || ''; const description = item.metadata?.description?.toLowerCase() || '';
58-74: filtersEqual should compare tags ignoring order and case.Order-sensitive comparison can cause false skips/misses across caches. Normalize and sort before comparison.
Apply this diff:
function filtersEqual(filter1?: KnowledgeItemsFilter, filter2?: KnowledgeItemsFilter): boolean { if (!filter1 && !filter2) return true; if (!filter1 || !filter2) return false; - // Compare tags arrays - const tagsEqual = (filter1.tags?.length || 0) === (filter2.tags?.length || 0) && - (filter1.tags || []).every((tag, index) => tag === (filter2.tags || [])[index]); + // Compare tags arrays (order- and case-insensitive) + const norm1 = [...(filter1.tags ?? [])].map((t) => t.toLowerCase()).sort(); + const norm2 = [...(filter2.tags ?? [])].map((t) => t.toLowerCase()).sort(); + const tagsEqual = norm1.length === norm2.length && norm1.every((t, i) => t === norm2[i]); return ( filter1.page === filter2.page && filter1.per_page === filter2.per_page && filter1.search === filter2.search && filter1.knowledge_type === filter2.knowledge_type && filter1.source_type === filter2.source_type && tagsEqual ); }
303-329: Add secondary dedupe by source_id to avoid duplicates when server returns a different id.removeDuplicateEntities dedupes by id; when polling inserts a server entity (different id, same source_id), duplicates remain. Add a second pass keyed by source_id.
Apply this diff:
// Replace the optimistic entity with server data const replacedItems = replaceOptimisticEntity(old.items, context.optimisticId, serverItem); // Remove any duplicates that might have been created - const deduplicatedItems = removeDuplicateEntities(replacedItems); + const deduplicatedItems = removeDuplicateEntities(replacedItems); + // Secondary dedupe by source_id + const seenSourceIds = new Set<string>(); + const dedupBySourceId = deduplicatedItems.filter((i) => { + if (seenSourceIds.has(i.source_id)) return false; + seenSourceIds.add(i.source_id); + return true; + }); return { ...old, - items: deduplicatedItems, + items: dedupBySourceId, };
526-551: Mirror source_id dedupe in upload success path.Same duplication risk exists here; apply the same secondary dedupe.
Apply this diff:
// Replace the optimistic entity with server data const replacedItems = replaceOptimisticEntity(old.items, context.optimisticId, serverItem); // Remove any duplicates that might have been created - const deduplicatedItems = removeDuplicateEntities(replacedItems); + const deduplicatedItems = removeDuplicateEntities(replacedItems); + // Secondary dedupe by source_id + const seenSourceIds = new Set<string>(); + const dedupBySourceId = deduplicatedItems.filter((i) => { + if (seenSourceIds.has(i.source_id)) return false; + seenSourceIds.add(i.source_id); + return true; + }); return { ...old, - items: deduplicatedItems, + items: dedupBySourceId, };
209-231: Avoid hardcoding per_page = 100 — use currentFilter?.per_page or a shared PAGE_SIZEMultiple optimistic-cache fallbacks use
?? 100in src/features/knowledge/hooks/useKnowledgeQueries.ts (around lines ~221, ~258, ~445, ~482). KnowledgeView.tsx definesPAGE_SIZE = 100locally but there is no canonical exported PAGE_SIZE. Replace the literal withcurrentFilter?.per_pagewhen available, or import/use a single exported PAGE_SIZE (or omit per_page to let the API default) to avoid pagination mismatches.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
archon-ui-main/src/features/knowledge/hooks/useKnowledgeQueries.ts(11 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
archon-ui-main/src/features/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
archon-ui-main/src/features/**/*.{ts,tsx}: Use TanStack Query for all data fetching (no prop drilling); use smart HTTP polling (no WebSockets)
Biome formatting in features: 120-character line length, double quotes, and trailing commas
Apply Tron-inspired glassmorphism styling with Tailwind for feature UIUse Biome in features: 120 character line length, double quotes, and trailing commas
Files:
archon-ui-main/src/features/knowledge/hooks/useKnowledgeQueries.ts
archon-ui-main/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Disallow implicit any in TypeScript
archon-ui-main/src/**/*.{ts,tsx}: Frontend TypeScript must use strict mode with no implicit any
Use TanStack Query for all data fetching; avoid prop drilling
Use database values directly in the frontend; avoid mapping layers between BE and FE types
Files:
archon-ui-main/src/features/knowledge/hooks/useKnowledgeQueries.ts
archon-ui-main/src/features/*/hooks/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Place TanStack Query hooks in the feature’s hooks directory
Use feature-scoped TanStack Query hooks in src/features/[feature]/hooks
Files:
archon-ui-main/src/features/knowledge/hooks/useKnowledgeQueries.ts
archon-ui-main/src/**/*.{ts,tsx,py}
📄 CodeRabbit inference engine (CLAUDE.md)
In code comments, avoid meta terms like SIMPLIFIED, ENHANCED, LEGACY, CHANGED, REMOVED; comment only on functionality and reasoning (do not mention beta/global rules)
Files:
archon-ui-main/src/features/knowledge/hooks/useKnowledgeQueries.ts
🧠 Learnings (2)
📚 Learning: 2025-08-28T13:50:10.499Z
Learnt from: Wirasm
PR: coleam00/Archon#514
File: archon-ui-main/src/services/pollingService.ts:38-45
Timestamp: 2025-08-28T13:50:10.499Z
Learning: The pollingService.ts file is being deprecated in favor of the usePolling hook from usePolling.ts. The team has decided to consolidate on the usePolling hook pattern rather than maintaining separate polling services, as it provides better React integration and reduces code duplication.
Applied to files:
archon-ui-main/src/features/knowledge/hooks/useKnowledgeQueries.ts
📚 Learning: 2025-08-29T08:57:47.558Z
Learnt from: Wirasm
PR: coleam00/Archon#514
File: archon-ui-main/src/services/progressService.ts:0-0
Timestamp: 2025-08-29T08:57:47.558Z
Learning: The progressService.ts file was removed from the codebase in PR #514 as part of the consolidation effort to use the usePolling hook pattern from usePolling.ts instead of maintaining separate polling services. This aligns with the architectural decision to deprecate individual polling services in favor of the centralized hook-based approach.
Applied to files:
archon-ui-main/src/features/knowledge/hooks/useKnowledgeQueries.ts
🧬 Code graph analysis (1)
archon-ui-main/src/features/knowledge/hooks/useKnowledgeQueries.ts (2)
archon-ui-main/src/features/knowledge/types/knowledge.ts (3)
KnowledgeItem(26-39)KnowledgeItemsFilter(127-134)KnowledgeItemsResponse(98-103)archon-ui-main/src/features/shared/utils/optimistic.ts (2)
replaceOptimisticEntity(48-59)removeDuplicateEntities(65-74)
🔇 Additional comments (2)
archon-ui-main/src/features/knowledge/hooks/useKnowledgeQueries.ts (2)
10-10: Good: using shared optimistic utilities.Adopting createOptimisticId/createOptimisticEntity and replaceOptimisticEntity/removeDuplicateEntities aligns with our shared patterns.
238-249: Smart skip-check for current query key looks solid.Deep-equality via filtersEqual prevents double-updating the current cache while still updating other caches.
Consider extracting this comparison into a tiny helper to reuse between crawl/upload paths for consistency.
|
🔄 This repository is being replaced by a new version of Archon. The original Python/MCP codebase is being archived to the This PR is being closed as part of the migration. Thank you for your contribution! |
The test 'should call generateAndSetTitle with workflow name and user message' was failing with 'Cannot create worktree: not in a git repository' because two mocks were inconsistent with the code path being exercised. Changes: - Add `configureIsolation` to the `@archon/isolation` mock (was imported but unmocked) - Fix `findCodebaseByDefaultCwd` mock to return a codebase object instead of null so the isolation block is entered and `generateAndSetTitle` is reached Fixes #717 Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
…m00#728) The test 'should call generateAndSetTitle with workflow name and user message' was failing with 'Cannot create worktree: not in a git repository' because two mocks were inconsistent with the code path being exercised. Changes: - Add `configureIsolation` to the `@archon/isolation` mock (was imported but unmocked) - Fix `findCodebaseByDefaultCwd` mock to return a codebase object instead of null so the isolation block is entered and `generateAndSetTitle` is reached Fixes coleam00#717 Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
…m00#728) The test 'should call generateAndSetTitle with workflow name and user message' was failing with 'Cannot create worktree: not in a git repository' because two mocks were inconsistent with the code path being exercised. Changes: - Add `configureIsolation` to the `@archon/isolation` mock (was imported but unmocked) - Fix `findCodebaseByDefaultCwd` mock to return a codebase object instead of null so the isolation block is entered and `generateAndSetTitle` is reached Fixes coleam00#717 Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
3 participants
Summary
Resolves the issue where optimistic updates were not visible to users when adding knowledge items (web crawling or document upload). Users now see immediate visual feedback when adding content.
Problem Fixed
Solution
Technical Changes
KnowledgeFilterContext.tsxfor centralized filter stateuseKnowledgeQueries.tswith current filter awareness in optimistic updatesKnowledgeView.tsxto use context instead of local stateKnowledgeViewWithBoundary.tsxwith filter provider.claude-flowmetrics filesTest Plan
Automated Tests ✅
Integration Tests ✅
Manual Testing
Files Changed
archon-ui-main/src/features/knowledge/context/KnowledgeFilterContext.tsx(new)archon-ui-main/src/features/knowledge/context/index.ts(new)archon-ui-main/src/features/knowledge/context/tests/KnowledgeFilterContext.test.tsx(new)archon-ui-main/src/features/knowledge/hooks/useKnowledgeQueries.ts(enhanced)archon-ui-main/src/features/knowledge/hooks/tests/useKnowledgeQueries.test.ts(enhanced)archon-ui-main/src/features/knowledge/views/KnowledgeView.tsx(refactored)archon-ui-main/src/features/knowledge/views/KnowledgeViewWithBoundary.tsx(enhanced)Impact
Summary by CodeRabbit
New Features
Bug Fixes
UI
Tests