Fix project filter synchronization issues in viewer UI (#70)
* feat: Enhance session and summary handling - Update SQL query in SessionStore to exclude null or empty projects. - Add 'investigated' field to Summary interface for better tracking. - Modify App component to handle pagination more efficiently based on current filters. - Update SummaryCard to display the 'investigated' field if present. - Refactor usePagination hook to reset pagination state when filters change. - Adjust mergeAndDeduplicateByProject function to ensure it only merges unfiltered data. * refactor: address PR feedback - remove redundancies and fix dependency cycles Fixes based on PR #70 review feedback: Required: - Fixed useEffect dependency cycle in App.tsx (removed handleLoadMore from deps) Recommended: - Removed redundant filter detection from App.tsx (usePagination handles this) - Removed redundant stateRef update effect from usePagination.ts - Simplified mergeAndDeduplicateByProject by removing defensive validation - Removed unused imports (useRef, useEffect) All changes follow CLAUDE.md principles: DRY, fail-fast, no defensive programming. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com> * fix: Reset paginated data arrays when filter changes Critical fix for data mixing bug identified in PR review follow-up. Problem: When filter changes, usePagination correctly resets its offset to 0, but the paginated state arrays (observations, summaries, prompts) were NOT being reset. This caused data from different projects to mix together because setState appends to the existing array. Example: - User views Project A: [A1, A2, A3] - User switches to Project B - API fetches [B1, B2, B3] - setState does: [...prev, ...new] = [A1, A2, A3, B1, B2, B3] ❌ Solution: Added a separate useEffect that resets all three paginated arrays when currentFilter changes. This happens BEFORE handleLoadMore fetches new data, ensuring clean state for the new filter. Files changed: - src/ui/viewer/App.tsx: Added useEffect to reset arrays on filter change - plugin/ui/viewer-bundle.js: Built UI bundle Testing: 1. Select Project A, verify data loads 2. Switch to Project B 3. Verify ONLY Project B data is shown (no mixing) 4. Switch back to "All Projects" 5. Verify all data appears correctly 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com> * refactor: Combine filter change useEffect hooks for guaranteed order Merged two separate useEffect hooks into one to: 1. Guarantee execution order (reset THEN load) 2. Reduce complexity (one hook instead of two) 3. Make intent clearer with single comment Before: - useEffect #1: handleLoadMore() on filter change - useEffect #2: Reset arrays on filter change - React could run these in any order After: - Single useEffect: Reset arrays THEN handleLoadMore() - Execution order is now guaranteed Files changed: - src/ui/viewer/App.tsx: Combined useEffect hooks - plugin/ui/viewer-bundle.js: Built UI bundle 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com> --------- Co-authored-by: Claude <noreply@anthropic.com>
This commit is contained in:
Alex Newman
+28
-23
@@ -23,28 +23,30 @@ export function App() {
|
||||
const { preference, resolvedTheme, setThemePreference } = useTheme();
|
||||
const pagination = usePagination(currentFilter);
|
||||
|
||||
// Reset paginated data when filter changes
|
||||
useEffect(() => {
|
||||
setPaginatedObservations([]);
|
||||
setPaginatedSummaries([]);
|
||||
setPaginatedPrompts([]);
|
||||
}, [currentFilter]);
|
||||
// When filtering by project: ONLY use paginated data (API-filtered)
|
||||
// When showing all projects: merge SSE live data with paginated data
|
||||
const allObservations = useMemo(() => {
|
||||
if (currentFilter) {
|
||||
// Project filter active: API handles filtering, ignore SSE items
|
||||
return paginatedObservations;
|
||||
}
|
||||
// No filter: merge SSE + paginated, deduplicate by ID
|
||||
return mergeAndDeduplicateByProject(observations, paginatedObservations);
|
||||
}, [observations, paginatedObservations, currentFilter]);
|
||||
|
||||
// Merge real-time data with paginated data, removing duplicates and filtering by project
|
||||
const allObservations = useMemo(
|
||||
() => mergeAndDeduplicateByProject(observations, paginatedObservations, currentFilter),
|
||||
[observations, paginatedObservations, currentFilter]
|
||||
);
|
||||
const allSummaries = useMemo(() => {
|
||||
if (currentFilter) {
|
||||
return paginatedSummaries;
|
||||
}
|
||||
return mergeAndDeduplicateByProject(summaries, paginatedSummaries);
|
||||
}, [summaries, paginatedSummaries, currentFilter]);
|
||||
|
||||
const allSummaries = useMemo(
|
||||
() => mergeAndDeduplicateByProject(summaries, paginatedSummaries, currentFilter),
|
||||
[summaries, paginatedSummaries, currentFilter]
|
||||
);
|
||||
|
||||
const allPrompts = useMemo(
|
||||
() => mergeAndDeduplicateByProject(prompts, paginatedPrompts, currentFilter),
|
||||
[prompts, paginatedPrompts, currentFilter]
|
||||
);
|
||||
const allPrompts = useMemo(() => {
|
||||
if (currentFilter) {
|
||||
return paginatedPrompts;
|
||||
}
|
||||
return mergeAndDeduplicateByProject(prompts, paginatedPrompts);
|
||||
}, [prompts, paginatedPrompts, currentFilter]);
|
||||
|
||||
// Toggle sidebar
|
||||
const toggleSidebar = useCallback(() => {
|
||||
@@ -72,13 +74,16 @@ export function App() {
|
||||
} catch (error) {
|
||||
console.error('Failed to load more data:', error);
|
||||
}
|
||||
}, [pagination.observations, pagination.summaries, pagination.prompts]);
|
||||
}, [currentFilter, pagination.observations, pagination.summaries, pagination.prompts]);
|
||||
|
||||
// Load first page only when filter changes
|
||||
// Reset paginated data and load first page when filter changes
|
||||
useEffect(() => {
|
||||
setPaginatedObservations([]);
|
||||
setPaginatedSummaries([]);
|
||||
setPaginatedPrompts([]);
|
||||
handleLoadMore();
|
||||
// eslint-disable-next-line react-hooks/exhaustive-deps
|
||||
}, [currentFilter]); // Only re-run when filter changes, not when handleLoadMore changes
|
||||
}, [currentFilter]);
|
||||
|
||||
return (
|
||||
<div className="container">
|
||||
|
||||
@@ -20,6 +20,9 @@ export function SummaryCard({ summary }: SummaryCardProps) {
|
||||
{summary.request && (
|
||||
<div className="card-title">Request: {summary.request}</div>
|
||||
)}
|
||||
{summary.investigated && (
|
||||
<div className="card-subtitle">Investigated: {summary.investigated}</div>
|
||||
)}
|
||||
{summary.learned && (
|
||||
<div className="card-subtitle">Learned: {summary.learned}</div>
|
||||
)}
|
||||
|
||||
@@ -1,4 +1,4 @@
|
||||
import { useState, useEffect, useCallback, useRef } from 'react';
|
||||
import { useState, useCallback, useRef } from 'react';
|
||||
import { Observation, Summary, UserPrompt } from '../types';
|
||||
import { UI } from '../constants/ui';
|
||||
import { API_ENDPOINTS } from '../constants/api';
|
||||
@@ -19,42 +19,40 @@ function usePaginationFor(endpoint: string, dataType: DataType, currentFilter: s
|
||||
isLoading: false,
|
||||
hasMore: true
|
||||
});
|
||||
const [offset, setOffset] = useState(0);
|
||||
|
||||
// Use refs to avoid stale closures and prevent infinite loops
|
||||
// Track offset and filter in refs to handle synchronous resets
|
||||
const offsetRef = useRef(0);
|
||||
const lastFilterRef = useRef(currentFilter);
|
||||
const stateRef = useRef(state);
|
||||
const offsetRef = useRef(offset);
|
||||
|
||||
useEffect(() => {
|
||||
stateRef.current = state;
|
||||
}, [state]);
|
||||
|
||||
useEffect(() => {
|
||||
offsetRef.current = offset;
|
||||
}, [offset]);
|
||||
|
||||
// Reset pagination when filter changes
|
||||
useEffect(() => {
|
||||
setOffset(0);
|
||||
setState({
|
||||
isLoading: false,
|
||||
hasMore: true
|
||||
});
|
||||
}, [currentFilter]);
|
||||
|
||||
/**
|
||||
* Load more items from the API
|
||||
* Automatically resets offset to 0 if filter has changed
|
||||
*/
|
||||
const loadMore = useCallback(async (): Promise<DataItem[]> => {
|
||||
// Check if filter changed - if so, reset pagination synchronously
|
||||
const filterChanged = lastFilterRef.current !== currentFilter;
|
||||
|
||||
if (filterChanged) {
|
||||
offsetRef.current = 0;
|
||||
lastFilterRef.current = currentFilter;
|
||||
|
||||
// Reset state both in React state and ref synchronously
|
||||
const newState = { isLoading: false, hasMore: true };
|
||||
setState(newState);
|
||||
stateRef.current = newState; // Update ref immediately to avoid stale checks
|
||||
}
|
||||
|
||||
// Prevent concurrent requests using ref (always current)
|
||||
if (stateRef.current.isLoading || !stateRef.current.hasMore) {
|
||||
// Skip this check if we just reset the filter - we want to load the first page
|
||||
if (!filterChanged && (stateRef.current.isLoading || !stateRef.current.hasMore)) {
|
||||
return [];
|
||||
}
|
||||
|
||||
setState(prev => ({ ...prev, isLoading: true }));
|
||||
|
||||
try {
|
||||
// Build query params using ref (always current)
|
||||
// Build query params using current offset from ref
|
||||
const params = new URLSearchParams({
|
||||
offset: offsetRef.current.toString(),
|
||||
limit: UI.PAGINATION_PAGE_SIZE.toString()
|
||||
@@ -71,7 +69,7 @@ function usePaginationFor(endpoint: string, dataType: DataType, currentFilter: s
|
||||
throw new Error(`Failed to load ${dataType}: ${response.statusText}`);
|
||||
}
|
||||
|
||||
const data = await response.json();
|
||||
const data = await response.json() as { items: DataItem[], hasMore: boolean };
|
||||
|
||||
setState(prev => ({
|
||||
...prev,
|
||||
@@ -79,14 +77,16 @@ function usePaginationFor(endpoint: string, dataType: DataType, currentFilter: s
|
||||
hasMore: data.hasMore
|
||||
}));
|
||||
|
||||
setOffset(prev => prev + UI.PAGINATION_PAGE_SIZE);
|
||||
return data.items as DataItem[];
|
||||
// Increment offset after successful load
|
||||
offsetRef.current += UI.PAGINATION_PAGE_SIZE;
|
||||
|
||||
return data.items;
|
||||
} catch (error) {
|
||||
console.error(`Failed to load ${dataType}:`, error);
|
||||
setState(prev => ({ ...prev, isLoading: false }));
|
||||
return [];
|
||||
}
|
||||
}, [currentFilter, endpoint, dataType]); // Only stable values - no state/offset deps
|
||||
}, [currentFilter, endpoint, dataType]);
|
||||
|
||||
return {
|
||||
...state,
|
||||
|
||||
@@ -21,6 +21,7 @@ export interface Summary {
|
||||
session_id: string;
|
||||
project: string;
|
||||
request?: string;
|
||||
investigated?: string;
|
||||
learned?: string;
|
||||
completed?: string;
|
||||
next_steps?: string;
|
||||
|
||||
@@ -4,25 +4,21 @@
|
||||
*/
|
||||
|
||||
/**
|
||||
* Merge real-time SSE items with paginated items, removing duplicates and filtering by project
|
||||
* @param liveItems - Items from SSE stream
|
||||
* @param paginatedItems - Items from pagination API (already filtered by project)
|
||||
* @param projectFilter - Current project filter (empty string = all projects)
|
||||
* Merge real-time SSE items with paginated items, removing duplicates by ID
|
||||
* NOTE: This should ONLY be used when no project filter is active.
|
||||
* When filtering, use ONLY paginated data (API-filtered).
|
||||
*
|
||||
* @param liveItems - Items from SSE stream (unfiltered)
|
||||
* @param paginatedItems - Items from pagination API
|
||||
* @returns Merged and deduplicated array
|
||||
*/
|
||||
export function mergeAndDeduplicateByProject<T extends { id: number; project?: string }>(
|
||||
liveItems: T[],
|
||||
paginatedItems: T[],
|
||||
projectFilter: string
|
||||
paginatedItems: T[]
|
||||
): T[] {
|
||||
// Filter SSE items by current project (pagination is already filtered)
|
||||
const filteredLive = projectFilter
|
||||
? liveItems.filter(item => item.project === projectFilter)
|
||||
: liveItems;
|
||||
|
||||
// Deduplicate using Set
|
||||
// Deduplicate by ID
|
||||
const seen = new Set<number>();
|
||||
return [...filteredLive, ...paginatedItems].filter(item => {
|
||||
return [...liveItems, ...paginatedItems].filter(item => {
|
||||
if (seen.has(item.id)) return false;
|
||||
seen.add(item.id);
|
||||
return true;
|
||||
|
||||
Reference in New Issue
Block a user