test explor - #119
Conversation
WalkthroughThis PR introduces comprehensive error handling and performance optimization across the application. A global SequencesequenceDiagram
participant User
participant App
participant ErrorBoundary
participant TaskBoard
participant TaskContext
participant HTTPClient
participant API
User->>App: Loads Application
App->>ErrorBoundary: Wraps entire app
ErrorBoundary->>TaskBoard: Renders components
User->>TaskBoard: Applies filters/search
TaskBoard->>TaskContext: Request filtered tasks
TaskContext->>TaskContext: Sort by priority & time
TaskContext-->>TaskBoard: Return filtered tasks
User->>TaskBoard: Creates/Updates task
TaskBoard->>TaskContext: Optimistic update
TaskContext->>HTTPClient: API request
HTTPClient->>API: Fetch with timeout
alt Success
API-->>HTTPClient: Response
HTTPClient-->>TaskContext: Success
TaskContext-->>TaskBoard: Confirm update
else Network Error
API-->>HTTPClient: Network failure
HTTPClient->>HTTPClient: Detect error type
HTTPClient-->>TaskContext: Error with code
TaskContext->>TaskContext: Rollback state
TaskContext-->>TaskBoard: Show error
TaskBoard-->>User: Display error message
else Runtime Error
TaskBoard->>ErrorBoundary: Error thrown
ErrorBoundary-->>User: Show error UI
User->>ErrorBoundary: Reset/Reload
ErrorBoundary->>App: Recover application
end
Changes
📋 Detailed File Changes📊 Changes by Category (5 groups)🛡️ Error Handling & ResilienceComprehensive error boundary system for React app and enhanced HTTP client error handling with network-specific error management
📋 Task Management FeaturesAdvanced task filtering capabilities (search, status, priority, assignee, tags) and robust error handling with optimistic UI updates
⚡ Performance OptimizationNew performance hooks including useThrottle for rate-limiting, useIdleCallback for deferred execution, and other optimization utilities
✨ Enhance your code reviews with DevzyAi - AI-powered code analysis and suggestions to help your team write better code. Enhance your code reviews with DevzyAi - AI-powered code analysis and suggestions to help your team write better code. 🏛️ Architect ReviewOverall Assessment: ✅ Excellent Detailed AnalysisApproach AssessmentThe PR introduces three distinct architectural improvements: global error handling, performance optimization utilities, and enhanced task management with optimistic updates. Let me evaluate each: Error Handling Architecture: Performance Hooks: Task Management with Optimistic Updates: Filtering System: Scalability Concerns:
Verdict: ACCEPTABLE The approaches are generally sound, but there are opportunities for better abstraction (optimistic updates) and potential performance concerns (filter computation) that should be addressed before the codebase grows significantly. Reuse Recommendations
Specific Recommendation: // core/hooks/useOptimisticUpdate.ts
function useOptimisticUpdate<T>(
optimisticUpdate: () => void,
apiCall: () => Promise<T>,
onSuccess?: (result: T) => void,
onError?: (error: Error) => void
)This would consolidate the repeated try-catch-rollback pattern seen in task operations. Pattern Consistency
Key Deviation Justification: Overall RecommendationThe architectural changes demonstrate good understanding of React patterns and introduce valuable improvements (error boundaries, performance hooks, optimistic updates). However, there are two areas needing attention: (1) abstract the repeated optimistic update pattern into a reusable hook to reduce duplication, and (2) consider splitting the TaskContext to prevent it from becoming a monolithic state manager. These refinements would significantly improve long-term maintainability. Score: Good |
![neatcod-simulator-dev[bot]](https://avatars.githubusercontent.com/u/243576164?s=60&v=4){kind=small}
| task.title.toLowerCase().includes(query) || | ||
| task.description.toLowerCase().includes(query) || | ||
| task.tags.some(tag => tag.toLowerCase().includes(query)) || | ||
| task.assignee?.toLowerCase().includes(query) |
There was a problem hiding this comment.
━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━
Severity: HIGH
This issue appears in 4 locations across 3 files. Similar fix can be applied to all locations.
📍 Affected Locations:
• src/features/tasks/hooks/useTaskFilters.ts:45
• src/features/tasks/hooks/useTaskFilters.ts:65
• src/features/tasks/hooks/useTasks.ts:38
• src/shared/components/ErrorBoundary.tsx:32
💡 General Guidance:
🐛 Bug: Potential runtime error when task.assignee is null or undefined
The code calls .toLowerCase() on task.assignee without null checking. While there's an optional chain on line 45, it only protects the includes() call, not the toLowerCase() call.
src/shared/components/ErrorBoundary.tsx:32
🟡 Medium
Fix: Try again" will immediately trigger the same error again, creating a poor user experience.
━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━
💡 Tip: Fixing this pattern across all locations will improve code quality consistently.
🤖 AI Fix Prompt - Copy this to your AI assistant
Fix the null/undefined check issue in src/features/tasks/hooks/useTaskFilters.ts at line 45
The code is calling toLowerCase() on task.assignee without properly checking if it exists first. The current optional chaining only protects the includes() method but not the toLowerCase() call, which means if task.assignee is null or undefined, the code will throw a runtime error when trying to call toLowerCase() on a non-existent value.
The fix should add proper null/undefined checking before calling toLowerCase(). Change the condition to check if task.assignee exists first, then convert to lowercase, and finally check if it includes the search term. The safest approach is to use optional chaining for the entire chain or add an explicit null check.
This same pattern appears in 3 other locations in the codebase that should be fixed similarly:
Line 65 in the same file (src/features/tasks/hooks/useTaskFilters.ts)
Line 38 in src/features/tasks/hooks/useTasks.ts where updatedAt might be undefined before calling getTime()
Line 32 in src/shared/components/ErrorBoundary.tsx where error handling might access undefined propertiesThe corrected code should follow this pattern: first verify the property exists, then perform string operations. For example, use task.assignee?.toLowerCase()?.includes(filters.assignee.toLowerCase()) or add an explicit check like task.assignee && task.assignee.toLowerCase().includes(filters.assignee.toLowerCase()).
Files that will be affected by this change:
src/features/tasks/hooks/useTaskFilters.ts (primary fix location)
src/features/tasks/components/TaskBoard.tsx (direct dependent that uses this hook)Requirements for the fix:
Ensure no runtime errors occur when task.assignee is null or undefined
Maintain the existing filter logic behavior when values are present
Apply consistent null checking pattern across all similar occurrences
Test with edge cases including null, undefined, and empty string values
Verify the fix works correctly in the TaskBoard component that depends on this hook
| const deleteTask = useCallback((id: string) => { | ||
| const success = taskRepository.delete(id); | ||
| if (success) { | ||
| dispatch({ type: 'DELETE_TASK', payload: id }); | ||
| const taskToDelete = state.tasks.find(t => t.id === id); | ||
| if (!taskToDelete) return; | ||
|
|
||
| dispatch({ type: 'DELETE_TASK', payload: id }); | ||
|
|
||
| try { | ||
| const success = taskRepository.delete(id); | ||
| if (!success) { | ||
| dispatch({ type: 'ADD_TASK', payload: taskToDelete }); | ||
| dispatch({ type: 'SET_ERROR', payload: 'Failed to delete task' }); | ||
| } | ||
| } catch (error) { | ||
| dispatch({ type: 'ADD_TASK', payload: taskToDelete }); | ||
| dispatch({ type: 'SET_ERROR', payload: 'Failed to delete task' }); | ||
| } | ||
| }, []); | ||
| }, [state.tasks]); |
There was a problem hiding this comment.
━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━
Severity: HIGH
This issue appears in 3 locations across 2 files. Similar fix can be applied to all locations.
📍 Affected Locations:
• src/features/tasks/context/TaskContext.tsx:152
• src/features/tasks/context/TaskContext.tsx:178
• src/features/tasks/hooks/useTaskFilters.ts:43
💡 General Guidance:
try {
const success = taskRepository.delete(id);
if (!success) {
-
dispatch({ type: 'ADD_TASK', payload: taskToDelete });
-
// Only rollback if task still doesn't exist -
const stillDeleted = !state.tasks.find(t => t.id === id); -
if (stillDeleted) dispatch({ type
📋 Specific Suggestions for Each File (Click to expand)
src/features/tasks/context/TaskContext.tsx:152
🟠 High
Fix: try {
const success = taskRepository.delete(id);
if (!success) {
-
dispatch({ type: 'ADD_TASK', payload: taskToDelete });
-
// Only rollback if task still doesn't exist -
const stillDeleted = !state.tasks.find(t => t.id === id); -
if (stillDeleted) dispatch({ type
src/features/tasks/context/TaskContext.tsx:178
🟠 High
Fix: try {
const updated = taskRepository.moveTask(taskId, newStatus);
if (updated) {
dispatch({ type: 'UPDATE_TASK', payload: updated });
} else {
-
dispatch({ type: 'UPDATE_TASK', payload: currentTask });
-
const latestTask = getCurrentTask(); -
if (latestTas
src/features/tasks/hooks/useTaskFilters.ts:43
🟠 High
Fix: 🐛 Bug: Potential runtime error when task.description is null or undefined
The code calls .toLowerCase() on task.description without checking if it exists. If a task has no description, this will throw a TypeError.
━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━
💡 Tip: Fixing this pattern across all locations will improve code quality consistently.
🤖 AI Fix Prompt - Copy this to your AI assistant
Fix the race condition bugs in optimistic updates across src/features/tasks/context/TaskContext.tsx at lines 152 and 178, and src/features/tasks/hooks/useTaskFilters.ts at line 43
There are critical race conditions in the task management system where optimistic updates can rollback to stale data. When operations fail and trigger rollbacks, the code doesn't verify whether the state has changed since the optimistic update was applied. This means if another operation modified the same task in the meantime, the rollback will overwrite that newer data with old cached values.
In TaskContext.tsx at line 152, when a task deletion fails, the code blindly re-adds the cached taskToDelete without checking if the task was already re-added by another operation or if the current state has diverged.
In TaskContext.tsx at line 178, when moving a task to a new status fails, the rollback uses the cached currentTask without verifying if that task has been updated by other operations during the async repository call.
In useTaskFilters.ts at line 43, there's an additional bug where task.description.toLowerCase() is called without null checking, which will throw a TypeError if description is null or undefined.
The fix should implement proper state verification before rollback operations:
For line 152 in TaskContext.tsx, before dispatching ADD_TASK on rollback, check if the task still doesn't exist in the current state. Only rollback if the task is genuinely missing, not if it was re-added by another operation.
For line 178 in TaskContext.tsx, instead of using the cached currentTask for rollback, fetch the latest version of the task from the current state at rollback time. Only apply the rollback if the task hasn't been modified by other operations.
For line 43 in useTaskFilters.ts, add optional chaining or explicit null check before calling toLowerCase() on task.description.
The pattern to follow is: capture the optimistic state, perform the async operation, then verify current state before applying any rollback. Use state.tasks.find() to get the latest version of entities rather than relying on closure-captured values.
Files that will be affected by this fix:
src/features/tasks/context/TaskContext.tsx
src/features/tasks/hooks/useTaskFilters.ts
src/features/tasks/hooks/useTasks.ts (indirect impact as it consumes the context)Requirements:
Implement state verification before all rollback operations in optimistic updates
Replace cached entity references with fresh lookups from current state during rollback
Add null safety checks for optional properties like task.description
Ensure the fix maintains the existing dispatch action types and payload structures
Test that concurrent operations don't overwrite each other's changes
Verify that legitimate rollbacks still work when no concurrent modifications occurred
| export function useIdleCallback(callback: () => void, deps: unknown[] = []) { | ||
| const callbackRef = useRef(callback); | ||
|
|
||
| useEffect(() => { | ||
| callbackRef.current = callback; | ||
| }, [callback]); | ||
|
|
||
| useEffect(() => { | ||
| if (typeof requestIdleCallback !== 'undefined') { | ||
| const id = requestIdleCallback(() => { | ||
| callbackRef.current(); | ||
| }); | ||
| return () => cancelIdleCallback(id); | ||
| } else { | ||
| const timeoutId = setTimeout(() => { | ||
| callbackRef.current(); | ||
| }, 1); | ||
| return () => clearTimeout(timeoutId); | ||
| } | ||
| }, deps); | ||
| } |
There was a problem hiding this comment.
━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━
Severity: HIGH
This issue appears in 3 locations across 2 files. Each location requires specific attention based on its context.
📍 Affected Locations:
• src/core/hooks/usePerformance.ts:63
• src/core/hooks/usePerformance.ts:84
• src/features/tasks/components/TaskBoard.tsx:100
💡 General Guidance:
use the effect to re-run on every render if deps is a new array instance (which it will be if passed inline). The effect should use a stable reference or spread the deps.
📋 Specific Suggestions for Each File (Click to expand)
src/core/hooks/usePerformance.ts:63
🟡 Medium
Fix: use the effect to re-run on every render if deps is a new array instance (which it will be if passed inline). The effect should use a stable reference or spread the deps.
src/core/hooks/usePerformance.ts:84
🟡 Medium
Fix: try]) => {
-
callback(entry.isIntersecting); - }, options);
-
callbackRef.current(entry.isIntersecting); -
}, optionsRef.current);
observer.observe(element);
return () => {
observer.disconnect();
};
- }, [elementRef, callback, options]);
- }, [elementRef]);
}
---
#### `src/features/tasks/components/TaskBoard.tsx:100`
🟡 **Medium**
**Fix:** try {
+ clearError();
+ } catch (err) {
+ console.error('Failed to clear error:', err);
+ }
+ }}
aria-label="Dismiss error"
>
</details>
━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━
💡 **Tip:** Fixing this pattern across all locations will improve code quality consistently.
<details>
<summary>🤖 <b>AI Fix Prompt</b> - Copy this to your AI assistant</summary>
<blockquote>
Fix the unstable dependency arrays causing unnecessary hook re-executions in src/core/hooks/usePerformance.ts at lines 43-63, 63-84 and src/features/tasks/components/TaskBoard.tsx at line 100
The problem is that dependency arrays are being passed as inline array literals to useEffect and useCallback hooks. Every time the component renders, a new array instance is created even if the values inside are the same. This causes React to think the dependencies have changed, triggering unnecessary effect re-runs and callback recreations. This defeats the purpose of memoization and can lead to performance issues and infinite render loops.
In usePerformance.ts there are two specific issues:
First issue around line 63 in the useThrottle or useIdleCallback hook - the deps array is likely being passed inline like [deps] or the callback itself is not stable. This needs to be fixed by either using a ref for the callback or ensuring the deps array has a stable reference.
Second issue around line 84 in the useIntersectionObserver hook - both the callback and options object are being passed directly in the dependency array. Since functions and objects are recreated on each render, this causes the effect to run every time. The fix is to use refs to store the callback and options, then access them via callbackRef.current and optionsRef.current inside the effect. The dependency array should only include elementRef.
Third issue in TaskBoard.tsx around line 100 - similar pattern where an inline function or array is causing instability.
The correct pattern for these hooks is:
For callbacks that need to be stable - wrap them in useCallback with proper dependencies or store them in a ref using useRef and update the ref in a separate useEffect
For objects like options - either memoize them with useMemo outside the hook or store them in a ref
For dependency arrays - never pass inline arrays, always define them as a const variable with proper memoization or use the spread operator on a stable reference
Look at how other custom hooks in the codebase handle this pattern. Check if there are examples of useCallback, useMemo, or useRef being used to stabilize dependencies in other hook files.
Files that need changes:
src/core/hooks/usePerformance.ts - fix both the useThrottle/useIdleCallback and useIntersectionObserver implementations
src/features/tasks/components/TaskBoard.tsx - fix the dependency issue in the error handling code
Requirements:
Ensure all callbacks used in effects are either wrapped in useCallback or stored in refs
Ensure all object dependencies like options are memoized or stored in refs
Remove function and object references from dependency arrays and replace with refs
Keep the dependency arrays minimal - only include primitive values or stable refs
Test that the hooks no longer re-run on every render by adding console logs
Verify that the actual functionality still works correctly after the changes
Make sure the fixes follow React hooks best practices and ESLint rules for exhaustive-deps
</blockquote>
</details>
| if (error.message.includes('Failed to fetch') || error.message.includes('NetworkError')) { | ||
| return { | ||
| code: ERROR_CODES.NETWORK_ERROR, | ||
| message: 'Network connection failed. Please check your internet connection.', | ||
| }; | ||
| } |
There was a problem hiding this comment.
🟡 Medium
🐛 Bug: Fragile error detection relies on string matching which is locale-dependent and browser-specific
The error message matching approach is unreliable because:
- Error messages vary across browsers (Chrome: "Failed to fetch", Firefox: "NetworkError when attempting to fetch resource")
- Messages may be localized in non-English browsers
- The check happens AFTER the generic network error check, making it unreachable code
- if (error.message.includes('Failed to fetch') || error.message.includes('NetworkError')) {
+ // Check for network errors by error type/name instead of message
+ if (error.name === 'TypeError' || error instanceof TypeError) {
return {
code: ERROR_CODES.NETWORK_ERROR,
message: 'Network connection failed. Please check your internet connection.',
};
}
return {
code: ERROR_CODES.NETWORK_ERROR,
message: error.message || 'Network error occurred',Note: This code block appears to be unreachable since it comes after a generic return statement for network errors. Consider restructuring the error handling logic to check for specific network error types first, then fall back to the generic handler.
| <ErrorBoundary> | ||
| <ToastProvider> | ||
| <AuthProvider> | ||
| <ProjectProvider> | ||
| <AppLayout /> | ||
| </ProjectProvider> | ||
| </AuthProvider> | ||
| </ToastProvider> | ||
| </ErrorBoundary> |
There was a problem hiding this comment.
🟡 Medium
🐛 Bug: ErrorBoundary placement outside ToastProvider prevents error notifications
The ErrorBoundary wraps the ToastProvider, which means if an error occurs, the toast system won't be available to display error messages. This breaks the user feedback mechanism during errors.
- <ErrorBoundary>
- <ToastProvider>
+ <ToastProvider>
+ <ErrorBoundary>
<AuthProvider>
<ProjectProvider>
<AppLayout />
</ProjectProvider>
</AuthProvider>
- </ToastProvider>
- </ErrorBoundary>
+ </ErrorBoundary>
+ </ToastProvider>Alternative consideration: If ErrorBoundary needs to catch ToastProvider errors, consider having two ErrorBoundaries - one outside for ToastProvider errors (with basic fallback), and one inside for application errors (with toast notifications).
| export function useThrottle<T extends (...args: unknown[]) => unknown>( | ||
| callback: T, | ||
| delay: number | ||
| ): T { | ||
| const lastRun = useRef<number>(0); | ||
| const timeoutRef = useRef<ReturnType<typeof setTimeout> | null>(null); | ||
|
|
||
| const throttledCallback = useCallback( | ||
| ((...args: Parameters<T>) => { | ||
| const now = Date.now(); | ||
| const timeSinceLastRun = now - lastRun.current; | ||
|
|
||
| if (timeSinceLastRun >= delay) { | ||
| lastRun.current = now; | ||
| return callback(...args); | ||
| } | ||
|
|
||
| if (timeoutRef.current) { | ||
| clearTimeout(timeoutRef.current); | ||
| } | ||
|
|
||
| timeoutRef.current = setTimeout(() => { | ||
| lastRun.current = Date.now(); | ||
| callback(...args); | ||
| }, delay - timeSinceLastRun); | ||
| }) as T, | ||
| [callback, delay] | ||
| ); | ||
|
|
||
| useEffect(() => { | ||
| return () => { | ||
| if (timeoutRef.current) { | ||
| clearTimeout(timeoutRef.current); | ||
| } | ||
| }; | ||
| }, []); | ||
|
|
||
| return throttledCallback; | ||
| } |
There was a problem hiding this comment.
🟡 Medium
severity: medium
🐛 Bug: useThrottle implementation has stale closure issue and incorrect throttle behavior
The throttledCallback doesn't include callback in its dependencies properly, and the throttle logic is flawed. When a throttled call is scheduled, it uses the delay from the first call rather than ensuring the callback runs at the end of the throttle window. Additionally, the callback reference can become stale.
-export function useThrottle<T extends (...args: unknown[]) => unknown>(
- callback: T,
- delay: number
-): T {
- const lastRun = useRef<number>(0);
- const timeoutRef = useRef<ReturnType<typeof setTimeout> | null>(null);
-
- const throttledCallback = useCallback(
- ((...args: Parameters<T>) => {
- const now = Date.now();
- const timeSinceLastRun = now - lastRun.current;
-
- if (timeSinceLastRun >= delay) {
- lastRun.current = now;
- return callback(...args);
- }
-
- if (timeoutRef.current) {
- clearTimeout(timeoutRef.current);
- }
-
- timeoutRef.current = setTimeout(() => {
- lastRun.current = Date.now();
- callback(...args);
- }, delay - timeSinceLastRun);
- }) as T,
- [callback, delay]
- );
+export function useThrottle<T extends (...args: unknown[]) => unknown>(
+ callback: T,
+ delay: number
+): T {
+ const lastRun = useRef<number>(0);
+ const timeoutRef = useRef<ReturnType<typeof setTimeout> | null>(null);
+ const callbackRef = useRef(callback);
+
+ useEffect(() => {
+ callbackRef.current = callback;
+ }, [callback]);
+
+ const throttledCallback = useCallback(
+ ((...args: Parameters<T>) => {
+ const now = Date.now();
+ const timeSinceLastRun = now - lastRun.current;
+
+ if (timeSinceLastRun >= delay) {
+ lastRun.current = now;
+ return callbackRef.current(...args);
+ }
+
+ if (timeoutRef.current) {
+ clearTimeout(timeoutRef.current);
+ }
+
+ timeoutRef.current = setTimeout(() => {
+ lastRun.current = Date.now();
+ callbackRef.current(...args);
+ }, delay - timeSinceLastRun);
+ }) as T,
+ [delay]
+ );
| <div className="board-filters"> | ||
| <div className="board-search"> | ||
| <SearchInput | ||
| value={filters.searchQuery} | ||
| onChange={(e) => updateFilter('searchQuery', e.target.value)} | ||
| placeholder="Search tasks... (⌘K)" | ||
| className="task-board-search" | ||
| /> | ||
| </div> | ||
| <div className="board-filter-controls"> | ||
| <Button | ||
| variant="ghost" | ||
| size="sm" | ||
| onClick={() => setShowFilters(!showFilters)} | ||
| > | ||
| <svg width="16" height="16" viewBox="0 0 24 24" fill="none" stroke="currentColor" strokeWidth="2"> | ||
| <polygon points="22 3 2 3 10 12.46 10 19 14 21 14 12.46 22 3" /> | ||
| </svg> | ||
| Filters | ||
| {activeFilterCount > 0 && ( | ||
| <Badge variant="primary" className="filter-badge"> | ||
| {activeFilterCount} | ||
| </Badge> | ||
| )} | ||
| </Button> | ||
| {activeFilterCount > 0 && ( | ||
| <Button variant="ghost" size="sm" onClick={resetFilters}> | ||
| Clear | ||
| </Button> | ||
| )} | ||
| </div> | ||
| </div> | ||
|
|
||
| {showFilters && ( | ||
| <div className="board-filters-panel"> | ||
| <div className="filter-group"> | ||
| <label className="filter-label">Status</label> | ||
| <Select | ||
| value={filters.status} | ||
| onChange={(e) => updateFilter('status', e.target.value as TaskStatus | 'all')} | ||
| options={[ | ||
| { value: 'all', label: 'All Statuses' }, | ||
| { value: 'backlog', label: 'Backlog' }, | ||
| { value: 'in_progress', label: 'In Progress' }, | ||
| { value: 'review', label: 'Review' }, | ||
| { value: 'done', label: 'Done' }, | ||
| ]} | ||
| /> | ||
| </div> | ||
| <div className="filter-group"> | ||
| <label className="filter-label">Priority</label> | ||
| <Select | ||
| value={filters.priority} | ||
| onChange={(e) => updateFilter('priority', e.target.value as TaskPriority | 'all')} | ||
| options={[ | ||
| { value: 'all', label: 'All Priorities' }, | ||
| { value: 'critical', label: 'Critical' }, | ||
| { value: 'high', label: 'High' }, | ||
| { value: 'medium', label: 'Medium' }, | ||
| { value: 'low', label: 'Low' }, | ||
| ]} | ||
| /> | ||
| </div> | ||
| <div className="filter-group"> | ||
| <label className="filter-label">Assignee</label> | ||
| <Select | ||
| value={filters.assignee} | ||
| onChange={(e) => updateFilter('assignee', e.target.value)} | ||
| options={[ | ||
| { value: 'all', label: 'All Assignees' }, | ||
| ...availableAssignees.map(assignee => ({ value: assignee, label: assignee })), | ||
| ]} | ||
| /> | ||
| </div> | ||
| </div> | ||
| )} |
There was a problem hiding this comment.
🟠 High
🐛 Bug: Type casting without validation can cause runtime errors
Lines 166 and 180 cast e.target.value to specific types without validation. If the Select component returns an unexpected value, this could cause type errors downstream when the filter functions expect valid enum values.
<div className="filter-group">
<label className="filter-label">Status</label>
<Select
value={filters.status}
- onChange={(e) => updateFilter('status', e.target.value as TaskStatus | 'all')}
+ onChange={(e) => {
+ const value = e.target.value;
+ if (value === 'all' || ['backlog', 'in_progress', 'review', 'done'].includes(value)) {
+ updateFilter('status', value as TaskStatus | 'all');
+ }
+ }}
options={[
{ value: 'all', label: 'All Statuses' },
{ value: 'backlog', label: 'Backlog' },
{ value: 'in_progress', label: 'In Progress' },
{ value: 'review', label: 'Review' },
{ value: 'done', label: 'Done' },
]}
/>
</div>
<div className="filter-group">
<label className="filter-label">Priority</label>
<Select
value={filters.priority}
- onChange={(e) => updateFilter('priority', e.target.value as TaskPriority | 'all')}
+ onChange={(e) => {
+ const value = e.target.value;
+ if (value === 'all' || ['critical', 'high', 'medium', 'low'].includes(value)) {
+ updateFilter('priority', value as TaskPriority | 'all');
+ }
+ }}
options={[
{ value: 'all', label: 'All Priorities' },
{ value: 'critical', label: 'Critical' },
{ value: 'high', label: 'High' },
{ value: 'medium', label: 'Medium' },
{ value: 'low', label: 'Low' },
]}
/>
</div>🤖 AI Fix Prompt - Copy this to your AI assistant
Fix the type casting bug in src/features/tasks/components/TaskBoard.tsx at lines 166 and 180
The code currently casts e.target.value directly to TaskStatus or TaskPriority union types without validating that the value is actually one of the expected enum values. This creates a runtime type safety issue because if the Select component somehow returns an unexpected value, the cast will succeed at compile time but cause type errors when the filter functions try to process invalid enum values.
The fix should add validation before the type cast to ensure the value is either 'all' or one of the valid enum values. For the status filter at line 166, validate against 'all' and the TaskStatus values: 'backlog', 'in_progress', 'review', 'done'. For the priority filter at line 180, validate against 'all' and the TaskPriority values: 'critical', 'high', 'medium', 'low'.
Replace the inline onChange handlers with proper validation logic that checks if the value matches the expected options before calling updateFilter. Only perform the type cast after confirming the value is valid. If the value is invalid, the handler should either ignore it or log a warning for debugging.
Check if there are type definitions for TaskStatus and TaskPriority in src/features/tasks/types that could be used to make the validation more maintainable. Consider creating a helper function or constant array of valid values that can be reused if similar Select components exist elsewhere in the codebase.
Files that might need changes:
src/features/tasks/components/TaskBoard.tsx (primary fix location)
src/features/tasks/types (check enum definitions)Requirements:
Validate the value before type casting
Handle both status and priority filters
Maintain the existing updateFilter function signature
Ensure the fix doesn't break the existing filter functionality
Consider extracting validation logic if it improves code reusability
Add appropriate error handling or logging for invalid values
| 'mod+k': () => { | ||
| const searchInput = document.querySelector<HTMLInputElement>('.task-board-search input'); | ||
| searchInput?.focus(); |
There was a problem hiding this comment.
🟡 Medium
🐛 Bug: DOM query selector could fail silently
The keyboard shortcut uses a brittle CSS class selector that could break if the className changes. If the element isn't found, the shortcut silently fails with no feedback to the user.
useKeyboardShortcuts({
'mod+k': () => {
- const searchInput = document.querySelector<HTMLInputElement>('.task-board-search input');
- searchInput?.focus();
+ const searchInput = document.querySelector<HTMLInputElement>('.task-board-search input');
+ if (searchInput) {
+ searchInput.focus();
+ } else {
+ console.warn('Search input not found for keyboard shortcut');
+ }
},
| onChange={(e) => updateFilter('assignee', e.target.value)} | ||
| options={[ | ||
| { value: 'all', label: 'All Assignees' }, | ||
| ...availableAssignees.map(assignee => ({ value: assignee, label: assignee })), |
There was a problem hiding this comment.
🟢 Low
💡 Suggestion: Potential empty assignee list edge case
If availableAssignees is empty, the dropdown will only show "All Assignees". Consider adding a disabled placeholder option or hiding the filter when no assignees exist.
<div className="filter-group">
<label className="filter-label">Assignee</label>
<Select
value={filters.assignee}
onChange={(e) => updateFilter('assignee', e.target.value)}
+ disabled={availableAssignees.length === 0}
options={[
{ value: 'all', label: 'All Assignees' },
+ ...(availableAssignees.length === 0
+ ? [{ value: '', label: 'No assignees available', disabled: true }]
+ : []),
...availableAssignees.map(assignee => ({ value: assignee, label: assignee })),
]}
/>
</div>
| try { | ||
| const newTask = taskRepository.create(dto); | ||
| dispatch({ type: 'ADD_TASK', payload: newTask }); | ||
| return newTask; | ||
| } catch (error) { | ||
| dispatch({ type: 'SET_ERROR', payload: 'Failed to create task' }); | ||
| throw error; | ||
| } | ||
| }, []); |
There was a problem hiding this comment.
🟡 Medium
🐛 Bug: Error handling doesn't prevent state corruption when repository throws
The try-catch wraps the dispatch, but if taskRepository.create() throws before returning, the error is caught and re-thrown, but no error state is set because the dispatch never happened. This creates inconsistent error handling compared to other methods.
const createTask = useCallback((dto: CreateTaskDTO): Task => {
try {
const newTask = taskRepository.create(dto);
dispatch({ type: 'ADD_TASK', payload: newTask });
return newTask;
} catch (error) {
dispatch({ type: 'SET_ERROR', payload: 'Failed to create task' });
throw error;
}
}, []);
| const updateTask = useCallback((id: string, dto: UpdateTaskDTO) => { | ||
| const updated = taskRepository.update(id, dto); | ||
| if (updated) { | ||
| dispatch({ type: 'UPDATE_TASK', payload: updated }); | ||
| const currentTask = state.tasks.find(t => t.id === id); | ||
| if (!currentTask) return; | ||
|
|
||
| const optimisticUpdate: Task = { | ||
| ...currentTask, | ||
| ...dto, | ||
| updatedAt: new Date(), | ||
| }; | ||
|
|
||
| dispatch({ type: 'UPDATE_TASK', payload: optimisticUpdate }); | ||
|
|
||
| try { | ||
| const updated = taskRepository.update(id, dto); | ||
| if (updated) { | ||
| dispatch({ type: 'UPDATE_TASK', payload: updated }); | ||
| } else { | ||
| dispatch({ type: 'UPDATE_TASK', payload: currentTask }); | ||
| dispatch({ type: 'SET_ERROR', payload: 'Failed to update task' }); | ||
| } | ||
| } catch (error) { | ||
| dispatch({ type: 'UPDATE_TASK', payload: currentTask }); | ||
| dispatch({ type: 'SET_ERROR', payload: 'Failed to update task' }); | ||
| } | ||
| }, []); | ||
| }, [state.tasks]); |
There was a problem hiding this comment.
🟠 High
🐛 Bug: Race condition in optimistic update - stale task reference
The currentTask is captured at the start, but by the time the rollback happens (lines 127, 131), another update might have modified the task. Rolling back to the stale currentTask will overwrite those changes.
const updateTask = useCallback((id: string, dto: UpdateTaskDTO) => {
- const currentTask = state.tasks.find(t => t.id === id);
+ const getCurrentTask = () => state.tasks.find(t => t.id === id);
+ const currentTask = getCurrentTask();
if (!currentTask) return;
const optimisticUpdate: Task = {
...currentTask,
...dto,
updatedAt: new Date(),
};
dispatch({ type: 'UPDATE_TASK', payload: optimisticUpdate });
try {
const updated = taskRepository.update(id, dto);
if (updated) {
dispatch({ type: 'UPDATE_TASK', payload: updated });
} else {
- dispatch({ type: 'UPDATE_TASK', payload: currentTask });
+ const latestTask = getCurrentTask();
+ if (latestTask) dispatch({ type: 'UPDATE_TASK', payload: latestTask });
dispatch({ type: 'SET_ERROR', payload: 'Failed to update task' });
}
} catch (error) {
- dispatch({ type: 'UPDATE_TASK', payload: currentTask });
+ const latestTask = getCurrentTask();
+ if (latestTask) dispatch({ type: 'UPDATE_TASK', payload: latestTask });
dispatch({ type: 'SET_ERROR', payload: 'Failed to update task' });
}
}, [state.tasks]);🤖 AI Fix Prompt - Copy this to your AI assistant
Fix the race condition in src/features/tasks/context/TaskContext.tsx at lines 110-134
There is a critical race condition in the updateTask function's optimistic update pattern. The problem occurs because currentTask is captured at the beginning of the function (line 110), but when the rollback happens in the catch block (lines 127, 131), the state may have been modified by other concurrent updates. Rolling back to the stale currentTask reference will overwrite any changes that happened between the initial capture and the rollback, causing data loss.
The root cause is that the function closes over the task state at the moment it starts executing, but React state updates are asynchronous and batched. If another update modifies the same task while this update is in flight, the rollback will revert to an outdated version of the task, discarding the intermediate changes.
The fix should create a function getCurrentTask that reads the latest task state from state.tasks at the moment it's called, not when the updateTask function starts. Use this function to get the current task at the beginning for the optimistic update, and call it again during rollback to get the most recent version of the task before reverting. This ensures the rollback always uses the latest state, not a stale closure.
Replace the single currentTask capture with a getCurrentTask function that returns state.tasks.find(t => t.id === id). Call getCurrentTask() at the start to get currentTask for the optimistic update. In both error paths (lines 127 and 131), replace the currentTask rollback with a fresh call to getCurrentTask() and only dispatch if a task is found.
This pattern is similar to how state updates should be handled in React when you need the latest state value rather than a captured closure value. Check if there are similar optimistic update patterns in useTasks.ts or other context files that might have the same issue.
Files that will be affected by this change:
src/features/tasks/context/TaskContext.tsx (direct fix)
src/features/tasks/hooks/useTasks.ts (may need similar fix if it uses this pattern)Requirements:
The getCurrentTask function must read from state.tasks each time it's called, not capture a closure
All rollback operations must use a fresh getCurrentTask() call to avoid stale data
Add null checks after getCurrentTask() in rollback paths since the task might have been deleted
Preserve the existing optimistic update behavior for the success path
Ensure the fix doesn't break the existing dispatch flow or error handling
Test with rapid concurrent updates to the same task to verify no data loss occurs
| const filteredTasks = useMemo(() => { | ||
| let result = [...tasks]; | ||
|
|
||
| if (debouncedSearchQuery) { | ||
| const query = debouncedSearchQuery.toLowerCase(); | ||
| result = result.filter(task => | ||
| task.title.toLowerCase().includes(query) || | ||
| task.description.toLowerCase().includes(query) || | ||
| task.tags.some(tag => tag.toLowerCase().includes(query)) || | ||
| task.assignee?.toLowerCase().includes(query) | ||
| ); | ||
| } | ||
|
|
||
| if (filters.status !== 'all') { | ||
| result = result.filter(task => task.status === filters.status); | ||
| } | ||
|
|
||
| if (filters.priority !== 'all') { | ||
| result = result.filter(task => task.priority === filters.priority); | ||
| } | ||
|
|
||
| if (filters.assignee !== 'all') { | ||
| result = result.filter(task => task.assignee === filters.assignee); | ||
| } | ||
|
|
||
| if (filters.tags.length > 0) { | ||
| result = result.filter(task => | ||
| filters.tags.some(tag => task.tags.includes(tag)) | ||
| ); | ||
| } | ||
|
|
||
| return result; | ||
| }, [tasks, debouncedSearchQuery, filters]); |
There was a problem hiding this comment.
🟢 Low
⚡ Performance: Multiple array iterations could be optimized
The filtering logic performs multiple sequential filter operations, creating intermediate arrays. For large task lists, this could be optimized by combining filters into a single pass.
- let result = [...tasks];
-
- if (debouncedSearchQuery) {
- const query = debouncedSearchQuery.toLowerCase();
- result = result.filter(task =>
- task.title.toLowerCase().includes(query) ||
- task.description?.toLowerCase().includes(query) ||
- task.tags.some(tag => tag.toLowerCase().includes(query)) ||
- task.assignee?.toLowerCase()?.includes(query)
- );
- }
-
- if (filters.status !== 'all') {
- result = result.filter(task => task.status === filters.status);
- }
-
- if (filters.priority !== 'all') {
- result = result.filter(task => task.priority === filters.priority);
- }
-
- if (filters.assignee !== 'all') {
- result = result.filter(task => task.assignee === filters.assignee);
- }
-
- if (filters.tags.length > 0) {
- result = result.filter(task =>
- filters.tags.some(tag => task.tags.includes(tag))
- );
- }
-
- return result;
+ const query = debouncedSearchQuery?.toLowerCase();
+
+ return tasks.filter(task => {
+ // Search query filter
+ if (query && !(
+ task.title.toLowerCase().includes(query) ||
+ task.description?.toLowerCase().includes(query) ||
+ task.tags.some(tag => tag.toLowerCase().includes(query)) ||
+ task.assignee?.toLowerCase()?.includes(query)
+ )) {
+ return false;
+ }
+
+ // Status filter
+ if (filters.status !== 'all' && task.status !== filters.status) {
+ return false;
+ }
+
+ // Priority filter
+ if (filters.priority !== 'all' && task.priority !== filters.priority) {
+ return false;
+ }
+
+ // Assignee filter
+ if (filters.assignee !== 'all' && task.assignee !== filters.assignee) {
+ return false;
+ }
+
+ // Tags filter
+ if (filters.tags.length > 0 && !filters.tags.some(tag => task.tags.includes(tag))) {
+ return false;
+ }
+
+ return true;
+ });
| .sort((a, b) => { | ||
| const priorityDiff = priorityOrder[b.priority] - priorityOrder[a.priority]; | ||
| if (priorityDiff !== 0) return priorityDiff; | ||
| return b.updatedAt.getTime() - a.updatedAt.getTime(); | ||
| }), |
There was a problem hiding this comment.
🟡 Medium
🐛 Bug: Potential runtime error when task priority is undefined or invalid
The code assumes all tasks have a valid priority from the priorityOrder map. If a task has an undefined priority or a priority value not in the map, priorityOrder[b.priority] will return undefined, causing NaN in the subtraction and unpredictable sort behavior.
.sort((a, b) => {
- const priorityDiff = priorityOrder[b.priority] - priorityOrder[a.priority];
+ const priorityA = priorityOrder[a.priority] ?? 0;
+ const priorityB = priorityOrder[b.priority] ?? 0;
+ const priorityDiff = priorityB - priorityA;
if (priorityDiff !== 0) return priorityDiff;
return b.updatedAt.getTime() - a.updatedAt.getTime();
}),
| <Button variant="ghost" onClick={() => window.location.reload()}> | ||
| Reload page | ||
| </Button> |
There was a problem hiding this comment.
🟡 Medium
severity: low
💡 Suggestion: Page reload loses unsaved user data
The "Reload page" button performs a hard refresh which will discard any unsaved form data, drafts, or in-memory state. Consider warning users or attempting to preserve critical state.
- <Button variant="ghost" onClick={() => window.location.reload()}>
+ <Button
+ variant="ghost"
+ onClick={() => {
+ if (confirm('Reloading will discard unsaved changes. Continue?')) {
+ window.location.reload();
+ }
+ }}
+ >
Reload page
</Button>
| {process.env.NODE_ENV === 'development' && this.state.error && ( | ||
| <details className="error-boundary-details"> | ||
| <summary>Error details</summary> | ||
| <pre className="error-boundary-stack"> | ||
| {this.state.error.stack} | ||
| </pre> | ||
| </details> | ||
| )} |
There was a problem hiding this comment.
🟡 Medium
severity: low
💡 Suggestion: Stack trace could expose sensitive information in development builds deployed to staging
While checking NODE_ENV === 'development' is good, development builds are sometimes deployed to staging environments accessible to external users. Stack traces can reveal file paths, internal variable names, and system architecture.
- {process.env.NODE_ENV === 'development' && this.state.error && (
+ {(process.env.NODE_ENV === 'development' || process.env.REACT_APP_SHOW_ERROR_DETAILS === 'true') && this.state.error && (
<details className="error-boundary-details">
<summary>Error details</summary>
<pre className="error-boundary-stack">
{this.state.error.stack}
</pre>
</details>
)}
| import { Component, type ReactNode, type ErrorInfo } from 'react'; | ||
| import { Button } from './Button'; | ||
|
|
||
| interface ErrorBoundaryProps { | ||
| children: ReactNode; | ||
| fallback?: ReactNode; | ||
| onError?: (error: Error, errorInfo: ErrorInfo) => void; | ||
| } | ||
|
|
||
| interface ErrorBoundaryState { | ||
| hasError: boolean; | ||
| error: Error | null; | ||
| } | ||
|
|
||
| export class ErrorBoundary extends Component<ErrorBoundaryProps, ErrorBoundaryState> { | ||
| constructor(props: ErrorBoundaryProps) { | ||
| super(props); | ||
| this.state = { hasError: false, error: null }; | ||
| } | ||
|
|
||
| static getDerivedStateFromError(error: Error): ErrorBoundaryState { | ||
| return { hasError: true, error }; | ||
| } | ||
|
|
||
| componentDidCatch(error: Error, errorInfo: ErrorInfo) { | ||
| console.error('ErrorBoundary caught an error:', error, errorInfo); | ||
| this.props.onError?.(error, errorInfo); | ||
| } | ||
|
|
||
| handleReset = () => { | ||
| this.setState({ hasError: false, error: null }); | ||
| }; | ||
|
|
||
| render() { | ||
| if (this.state.hasError) { | ||
| if (this.props.fallback) { | ||
| return this.props.fallback; | ||
| } | ||
|
|
||
| return ( | ||
| <div className="error-boundary"> | ||
| <div className="error-boundary-content"> | ||
| <svg | ||
| width="64" | ||
| height="64" | ||
| viewBox="0 0 24 24" | ||
| fill="none" | ||
| stroke="currentColor" | ||
| strokeWidth="2" | ||
| className="error-boundary-icon" | ||
| > | ||
| <circle cx="12" cy="12" r="10" /> | ||
| <line x1="12" y1="8" x2="12" y2="12" /> | ||
| <line x1="12" y1="16" x2="12.01" y2="16" /> | ||
| </svg> | ||
| <h2 className="error-boundary-title">Something went wrong</h2> | ||
| <p className="error-boundary-message"> | ||
| {this.state.error?.message || 'An unexpected error occurred'} | ||
| </p> | ||
| <div className="error-boundary-actions"> | ||
| <Button onClick={this.handleReset}>Try again</Button> | ||
| <Button variant="ghost" onClick={() => window.location.reload()}> | ||
| Reload page | ||
| </Button> | ||
| </div> | ||
| {process.env.NODE_ENV === 'development' && this.state.error && ( | ||
| <details className="error-boundary-details"> | ||
| <summary>Error details</summary> | ||
| <pre className="error-boundary-stack"> | ||
| {this.state.error.stack} | ||
| </pre> | ||
| </details> | ||
| )} | ||
| </div> | ||
| </div> | ||
| ); | ||
| } | ||
|
|
||
| return this.props.children; | ||
| } | ||
| } |
There was a problem hiding this comment.
🟡 Medium
severity: low
💡 Suggestion: Missing error reporting integration
The ErrorBoundary logs errors to console but doesn't integrate with error tracking services (Sentry, LogRocket, etc.). The onError callback is optional, so errors might go unreported in production.
Consider adding a default error reporter:
componentDidCatch(error: Error, errorInfo: ErrorInfo) {
console.error('ErrorBoundary caught an error:', error, errorInfo);
// Report to error tracking service
if (process.env.NODE_ENV === 'production') {
// Example: Sentry.captureException(error, { contexts: { react: errorInfo } });
}
this.props.onError?.(error, errorInfo);
}
| import { useEffect, useMemo } from 'react'; | ||
| import { useTaskContext } from '../context/TaskContext'; | ||
| import { COLUMNS_CONFIG, type TaskStatus, type TaskColumn } from '../types'; | ||
| import { COLUMNS_CONFIG, type TaskStatus, type TaskColumn, type TaskPriority } from '../types'; |
There was a problem hiding this comment.
🟡 Medium
⚠️ Potential Duplicate Detected
Symbol: TaskStatus
This symbol appears to already exist in the codebase:
src/features/tasks/types/index.ts:3
Recommendation:
- Consider reusing the existing implementation instead of creating a duplicate
- If this is intentional (e.g., different functionality), consider renaming to avoid confusion
- If consolidating, ensure all existing usages are updated
View existing implementation
src/features/tasks/types/index.ts:3
export type TaskStatus = 'backlog' | 'in_progress' | 'review' | 'done';
| import { useEffect, useMemo } from 'react'; | ||
| import { useTaskContext } from '../context/TaskContext'; | ||
| import { COLUMNS_CONFIG, type TaskStatus, type TaskColumn } from '../types'; | ||
| import { COLUMNS_CONFIG, type TaskStatus, type TaskColumn, type TaskPriority } from '../types'; |
There was a problem hiding this comment.
🟡 Medium
⚠️ Potential Duplicate Detected
Symbol: TaskColumn
This symbol appears to already exist in the codebase:
src/features/tasks/components/TaskColumn.tsx:16, src/features/tasks/components/TaskColumn.tsx:16, src/features/tasks/types/index.ts:18
Recommendation:
- Consider reusing the existing implementation instead of creating a duplicate
- If this is intentional (e.g., different functionality), consider renaming to avoid confusion
- If consolidating, ensure all existing usages are updated
View existing implementation
src/features/tasks/components/TaskColumn.tsx:16
export function TaskColumn({ column, onMoveTask, onDeleteTask, onSelectTask }: TaskColumnProps) {
src/features/tasks/components/TaskColumn.tsx:16
export function TaskColumn({ column, onMoveTask, onDeleteTask, onSelectTask }: TaskColumnProps) {
📋 Additional Findings (Outside Changed Lines)The following issues were detected in areas related to your changes but are outside the diff range. These cannot be added as inline comments but may be relevant to your PR. 📍
|
1 participant
Summary by DevzyAi
Release Notes
New Features
Bug Fixes
Improvements