feat(performance): add performance monitoring and optimization utilities#54
Conversation
- Add PerformanceMonitor class for tracking render times, mount times, and FPS - Add performance hooks: useDebouncedValue, useThrottledCallback, useDebouncedCallback - Add useStableCallback, usePrevious, useLazyValue for stable references - Add useStableArray, useStableObject for memoized collections - Add useIntersectionObserver, useWindowDimensions for lazy loading - Add useRenderTime, useMountTime, usePerformanceTracking for component metrics - Add OptimizedList component as performance-optimized FlatList wrapper - Add createMemoizedRenderItem and withListItemMemo helpers - Add comprehensive test suite for PerformanceMonitor Closes #17 Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
📝 WalkthroughWalkthroughAdds a new performance module (monitor, hooks, OptimizedList) with tests, exports the performance API from the library, refactors FileTree and editor theming by centralizing helpers, and tightens typing in error handling. No public API removals. Changes
Sequence Diagram(s)sequenceDiagram
participant Component as React Component
participant Hook as useRenderTime / useMountTime
participant Monitor as PerformanceMonitor
participant Logger as Logger
Component->>Hook: mount / render
Hook->>Monitor: trackMount(componentName, duration)
Hook->>Monitor: trackRender(componentName, duration)
Monitor->>Monitor: record metrics & update componentStats
Monitor->>Monitor: evaluate slow threshold
alt slow component detected
Monitor->>Logger: warn about slow component (details)
end
Monitor->>Monitor: accumulate metrics
Note over Monitor: startReporting() triggers periodic reportMetrics()
Monitor->>Logger: info(report summary)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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 |
- Add 26 tests for performance hooks (useRenderTime, useMountTime, useDebouncedValue, useThrottledCallback, useDebouncedCallback, usePrevious, useStableCallback, useLazyValue, useStableArray, useStableObject, useWindowDimensions) - Add 15 tests for OptimizedList component and helper functions - Increase test coverage for new performance module Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
- Remove unused imports (React, waitFor) - Fix import ordering - Apply Biome formatting Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
- Add tests for startReporting/stopReporting - Add tests for disabled monitor behavior - Add tests for slow render and mount logging - Add tests for FPS metrics in getSummary - Improve coverage from 78% to target 80%+ Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Refactor EditorSettingsScreen to use theme color helper function reducing cognitive complexity from 33 to under 15 - Extract FileTreeNode helper components to reduce complexity from 22 to under 15 - Fix any type usage in error-handler.ts using unknown cast - Fix test file formatting issues Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/lib/performance/hooks.ts`:
- Around line 239-252: The effect currently depends on a potentially new inline
options object each render, causing reruns; fix by stabilizing options before
useEffect: either memoize the incoming options inside the hook (e.g., create
stableOptions with useMemo or normalize via useRef) and use stableOptions in the
dependency array of the useEffect that creates the IntersectionObserver
(referencing elementRef, setIsVisible, and options), or change the API to
accept/extract individual primitive option fields (threshold, root, rootMargin)
and list those primitives in the effect deps so the observer only resets when
actual option values change; ensure observer.observe(element) and
observer.disconnect() still run against the stabilized options.
In `@src/lib/performance/OptimizedList.tsx`:
- Around line 22-23: The isLoadingMore prop on the OptimizedList component is
declared but unused; either remove it from the component's props/interface or
implement the footer loading UI: inside the OptimizedList functional component
(search for OptimizedList and its props/interface), wire isLoadingMore to the
FlatList/List's ListFooterComponent (or renderFooter function) so when
isLoadingMore is true the footer shows a loading indicator (ActivityIndicator or
similar) and when false it returns null; if you prefer to remove the API
surface, delete isLoadingMore from the props/interface and any related
docs/tests.
♻️ Duplicate comments (1)
src/lib/__tests__/performance-hooks.test.ts (1)
424-425: Remove unused variables instead of underscore-prefixing.The
_originalInnerWidthand_originalInnerHeightvariables are declared but never used. The restoration logic usesoriginalWindowonly (line 446-450). Underscore-prefixing suppresses lint warnings but doesn't address the underlying issue of dead code.♻️ Suggested fix
const originalWindow = global.window; - const _originalInnerWidth = global.window?.innerWidth; - const _originalInnerHeight = global.window?.innerHeight;
🧹 Nitpick comments (11)
app/settings/editor.tsx (1)
114-114: Consider typingthemestate asThemeto avoid repeated casts.The
themestate is typed asstringbut repeatedly cast toThemewhen callinggetThemeColor. Typing the state properly would eliminate the casts and provide compile-time safety.♻️ Suggested improvement
- const [theme, setTheme] = useState('dark'); + const [theme, setTheme] = useState<Theme>('dark');Then remove the
as Themecasts throughout the preview section.Also applies to: 284-306
src/lib/__tests__/OptimizedList.test.tsx (1)
58-77: Consider strengthening the keyExtractor assertion.The test verifies
customKeyExtractorwas called but doesn't confirm it was called for each item. A more precise assertion would verify the call count.♻️ Suggested improvement
// keyExtractor should be called for each item - expect(customKeyExtractor).toHaveBeenCalled(); + expect(customKeyExtractor).toHaveBeenCalledTimes(mockData.length);src/lib/__tests__/performance.test.ts (1)
67-82: Busy-wait timing test may be flaky in CI environments.The spin-wait loop (
while (Date.now() - startTime < 10)) can be unreliable under CPU contention. Consider usingjest.useFakeTimers()withjest.advanceTimersByTime()if the implementation supports it, or increase the tolerance.♻️ Alternative approach if flakiness occurs
If tests become flaky, consider mocking
Date.now()orperformance.now()to control timing deterministically:it('should return a function that records duration', () => { const mockNow = jest.spyOn(Date, 'now'); mockNow.mockReturnValueOnce(1000).mockReturnValueOnce(1015); const stopTiming = monitor.startTiming('timedOperation'); const duration = stopTiming(); expect(duration).toBe(15); mockNow.mockRestore(); });src/lib/performance/monitor.ts (2)
61-69: Consider restarting reporting when config changes.If
configure()is called while reporting is active, the newreportIntervalwon't take effect untilstopReporting()/startReporting()is called. This might be intentional, but consider documenting this behavior or auto-restarting the interval when config changes.
218-219: Fragile name-based filtering for render metrics.Using
m.name.includes('render')to filter render metrics is brittle—it would match unrelated metrics like'prerender'or'renderer-config'. Consider using a dedicatedtypefield onPerformanceMetricor a specific naming convention (e.g., prefix withrender:) that you control.src/lib/performance/OptimizedList.tsx (3)
71-81: Simplify viewabilityConfig initialization.Using
useRef(...).currentfor a constant object is slightly awkward. Consider usinguseMemofor consistency, or just define the config outside the component if it's truly static.Using useMemo
- // Viewability config for optimization - const viewabilityConfig = useRef({ + // Viewability config for optimization + const viewabilityConfig = useMemo(() => ({ itemVisiblePercentThreshold: 50, minimumViewTime: 300, - }).current; + }), []);
107-118: Document usage constraint.This helper creates a new
MemoizedComponenton each call. If called during render, it would create a new component type each render, breaking React's reconciliation. Consider adding a JSDoc note that this should be called at module level or inuseMemo:/** * Create a memoized render item function. * * `@note` Call this at module level or in useMemo, not during render. */
133-142: Consider comparingindexas well.This comparison function ignores the
indexparameter. If a list item's position changes (sameid, differentindex), the component won't re-render. This could cause issues if rendering depends on index (e.g., alternating row styles, "first"/"last" indicators).Include index in comparison
export function defaultListItemPropsAreEqual<T extends { id?: string | number }>( prevProps: { item: T; index: number }, nextProps: { item: T; index: number } ): boolean { + if (prevProps.index !== nextProps.index) { + return false; + } // Compare by id if available, otherwise by reference if (prevProps.item.id !== undefined && nextProps.item.id !== undefined) { return prevProps.item.id === nextProps.item.id; } return prevProps.item === nextProps.item; }src/lib/performance/hooks.ts (3)
94-101: Potential stale closure for pending timeouts.If
callbackchanges while a timeout is pending (lines 95-101), the scheduled execution will use the newcallbackreference (due to theuseCallbackdependency), but the pending timeout was created with the old closure. This could cause unexpected behavior.Consider storing the callback in a ref (similar to
useStableCallback) to always use the latest reference:Use ref for latest callback
export function useThrottledCallback<T extends (...args: unknown[]) => unknown>( callback: T, delay: number ): T { const lastCall = useRef(0); const lastArgs = useRef<unknown[] | null>(null); const timeoutId = useRef<ReturnType<typeof setTimeout> | null>(null); + const callbackRef = useRef(callback); + + useEffect(() => { + callbackRef.current = callback; + }, [callback]); const throttled = useCallback( (...args: unknown[]): unknown => { const now = Date.now(); const remaining = delay - (now - lastCall.current); lastArgs.current = args; if (remaining <= 0) { lastCall.current = now; if (timeoutId.current) { clearTimeout(timeoutId.current); timeoutId.current = null; } - return callback(...args); + return callbackRef.current(...args); } if (!timeoutId.current) { timeoutId.current = setTimeout(() => { lastCall.current = Date.now(); timeoutId.current = null; if (lastArgs.current) { - callback(...lastArgs.current); + callbackRef.current(...lastArgs.current); } }, remaining); } return undefined; }, - [callback, delay] + [delay] ) as T;
130-141: Same stale closure issue asuseThrottledCallback.If
callbackchanges while a timeout is pending, the scheduled execution will call the oldcallbackfrom the previous closure. Apply the same ref pattern as suggested foruseThrottledCallback.
211-228: Render-time ref mutation is a React anti-pattern.Mutating
ref.currentduring the render phase (line 224) can cause issues with React's concurrent features. TheuseMemousage here is also unusual—it computes a boolean but the intent is side-effect (comparison), not memoization.Consider using
useEffectfor the update or restructuring to use a proper memoization pattern:Safer implementation
export function useStableObject<T extends Record<string, unknown>>(obj: T): T { - const ref = useRef<T>(obj); - - const isEqual = useMemo(() => { - const keys1 = Object.keys(ref.current); - const keys2 = Object.keys(obj); - - if (keys1.length !== keys2.length) return false; - - return keys1.every((key) => ref.current[key] === obj[key]); - }, [obj]); - - if (!isEqual) { - ref.current = obj; - } - - return ref.current; + const isEqual = (a: T, b: T): boolean => { + const keys1 = Object.keys(a); + const keys2 = Object.keys(b); + if (keys1.length !== keys2.length) return false; + return keys1.every((key) => a[key] === b[key]); + }; + + const ref = useRef<T>(obj); + + // Use useMemo to derive the stable value + return useMemo(() => { + if (isEqual(ref.current, obj)) { + return ref.current; + } + ref.current = obj; + return obj; + }, [obj]); }
| useEffect(() => { | ||
| const element = elementRef.current; | ||
| if (!element) return; | ||
|
|
||
| const observer = new IntersectionObserver(([entry]) => { | ||
| setIsVisible(entry.isIntersecting); | ||
| }, options); | ||
|
|
||
| observer.observe(element); | ||
|
|
||
| return () => { | ||
| observer.disconnect(); | ||
| }; | ||
| }, [options]); |
There was a problem hiding this comment.
Potential infinite loop with inline options object.
If options is passed inline (e.g., useIntersectionObserver({ threshold: 0.5 })), a new object reference is created each render, causing the effect to re-run infinitely.
Consider stabilizing the options or documenting that callers must memoize them:
Option 1: Stabilize internally
export function useIntersectionObserver(
options: IntersectionObserverInit = {}
): [React.RefObject<Element | null>, boolean] {
const elementRef = useRef<Element | null>(null);
const [isVisible, setIsVisible] = useState(false);
+
+ // Stabilize options to prevent infinite loops
+ const stableOptions = useStableObject(options as Record<string, unknown>) as IntersectionObserverInit;
useEffect(() => {
const element = elementRef.current;
if (!element) return;
const observer = new IntersectionObserver(([entry]) => {
setIsVisible(entry.isIntersecting);
- }, options);
+ }, stableOptions);
observer.observe(element);
return () => {
observer.disconnect();
};
- }, [options]);
+ }, [stableOptions]);Option 2: Extract individual options as dependencies
export function useIntersectionObserver(
- options: IntersectionObserverInit = {}
+ options: IntersectionObserverInit = {},
): [React.RefObject<Element | null>, boolean] {
+ const { root, rootMargin, threshold } = options;
const elementRef = useRef<Element | null>(null);
const [isVisible, setIsVisible] = useState(false);
useEffect(() => {
const element = elementRef.current;
if (!element) return;
const observer = new IntersectionObserver(([entry]) => {
setIsVisible(entry.isIntersecting);
- }, options);
+ }, { root, rootMargin, threshold });
observer.observe(element);
return () => {
observer.disconnect();
};
- }, [options]);
+ }, [root, rootMargin, threshold]);🤖 Prompt for AI Agents
In `@src/lib/performance/hooks.ts` around lines 239 - 252, The effect currently
depends on a potentially new inline options object each render, causing reruns;
fix by stabilizing options before useEffect: either memoize the incoming options
inside the hook (e.g., create stableOptions with useMemo or normalize via
useRef) and use stableOptions in the dependency array of the useEffect that
creates the IntersectionObserver (referencing elementRef, setIsVisible, and
options), or change the API to accept/extract individual primitive option fields
(threshold, root, rootMargin) and list those primitives in the effect deps so
the observer only resets when actual option values change; ensure
observer.observe(element) and observer.disconnect() still run against the
stabilized options.
| /** Whether to show footer loading indicator */ | ||
| isLoadingMore?: boolean; |
There was a problem hiding this comment.
Unused isLoadingMore prop.
The isLoadingMore prop is declared in the interface but never used in the component implementation. Either remove it or implement the loading indicator logic (e.g., via ListFooterComponent).
Option 1: Remove unused prop
export interface OptimizedListProps<T> extends Omit<FlatListProps<T>, 'getItemLayout'> {
/** Height of each item (required for optimization) */
itemHeight: number;
/** Number of items to render above/below viewport */
windowSize?: number;
/** Threshold for onEndReached callback */
endReachedThreshold?: number;
- /** Whether to show footer loading indicator */
- isLoadingMore?: boolean;
/** Key extractor override */
keyExtractor?: (item: T, index: number) => string;📝 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.
| /** Whether to show footer loading indicator */ | |
| isLoadingMore?: boolean; | |
| export interface OptimizedListProps<T> extends Omit<FlatListProps<T>, 'getItemLayout'> { | |
| /** Height of each item (required for optimization) */ | |
| itemHeight: number; | |
| /** Number of items to render above/below viewport */ | |
| windowSize?: number; | |
| /** Threshold for onEndReached callback */ | |
| endReachedThreshold?: number; | |
| /** Key extractor override */ | |
| keyExtractor?: (item: T, index: number) => string; |
🤖 Prompt for AI Agents
In `@src/lib/performance/OptimizedList.tsx` around lines 22 - 23, The
isLoadingMore prop on the OptimizedList component is declared but unused; either
remove it from the component's props/interface or implement the footer loading
UI: inside the OptimizedList functional component (search for OptimizedList and
its props/interface), wire isLoadingMore to the FlatList/List's
ListFooterComponent (or renderFooter function) so when isLoadingMore is true the
footer shows a loading indicator (ActivityIndicator or similar) and when false
it returns null; if you prefer to remove the API surface, delete isLoadingMore
from the props/interface and any related docs/tests.
- Add tests for useIntersectionObserver hook (observe, visibility, disconnect) - Add test for useWindowDimensions resize handler - Add test for useThrottledCallback cleanup on unmount - Add test for low FPS detection and warning in PerformanceMonitor - Add test for performance reporting with slow components - Remove unused variables from test file - Coverage on new code should now exceed 80% threshold Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
|
1 participant
Summary
PerformanceMonitorclass for tracking render times, mount times, frame rates, and general performance metricsuseDebouncedValue/useDebouncedCallback- delay execution until after wait perioduseThrottledCallback- limit function execution frequencyuseStableCallback- stable reference for callbacks to prevent unnecessary re-rendersusePrevious- track previous value for comparisonuseLazyValue- defer expensive computationsuseStableArray/useStableObject- memoized collectionsuseIntersectionObserver- lazy loading supportuseWindowDimensions- responsive design supportuseRenderTime/useMountTime/usePerformanceTracking- component performance trackingOptimizedListcomponent as a performance-optimized FlatList wrapper with:createMemoizedRenderItem,withListItemMemo)Test plan
Closes #17
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Improvements
Tests
✏️ Tip: You can customize this high-level summary in your review settings.