Error handling improvements, ITERATION 2#459
Conversation
…iagnostics; enhance error capturing with detailed diagnostics data.
ar9708
added
bug
There was a problem hiding this comment.
Pull Request Overview
Improves robustness and observability of error handling, session/auth refresh logic, and diagnostics, plus adds localized strings and styling flexibility for error actions.
- Adds richer error propagation (collection errors, Axios diagnostics) and passes error states to UI components.
- Enhances auth token refresh and 401 handling plus diagnostic event payloads.
- Introduces a test-oriented diagnostics trigger and customizable ErrorPage button styling.
Reviewed Changes
Copilot reviewed 25 out of 26 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| screens/Playlists/index.tsx | Incorporates collection error into displayed error details. |
| screens/Playlists/components/PlaylistVideosView.tsx | Suppresses null return when an actual error state exists. |
| screens/Playlist/index.tsx | Propagates video query error state to VideoGrid. |
| screens/ChannelsScreen/index.tsx | Centralizes error rendering and includes collection errors. |
| screens/ChannelCategoryScreen/index.tsx | Adds error flags and forwards them to VideoGrid. |
| screens/CategoryScreen/index.tsx | Adds category/video error handling to grid. |
| screens/CategoriesScreen/index.tsx | Includes collection error in error key resolution. |
| public/featured-instances.json5 | Enables username/password login for specific instance. |
| locales/*.json | Adds new translation keys for copy feedback and press limit error. |
| hooks/useAuthSessionSync.tsx | Awaits session selection before marking load complete. |
| hooks/useAppStateDiagnostics.ts | Safely optional-chains subscription removal. |
| diagnostics/useCustomDiagnosticEvents.ts | Removes unsafe any cast on capture. |
| components/shared/Button.tsx | Adds configurable text color and refactors internal color calc. |
| components/ErrorPage.tsx | Supports custom button colors. |
| components/ErrorBoundary.tsx | Supplies explicit styling for reload button. |
| components/DeviceCapabilities/DeviceCapabilities.tsx | Adds copy feedback, press limit test trigger, and improved cleanup. |
| components/ChannelView.tsx | Avoids hiding content when an error (not emptiness) occurred. |
| components/CategoryView.tsx | Same error-aware visibility + variant prop. |
| app/_layout.tsx | Moves ErrorBoundary higher in tree for broader coverage. |
| api/helpers.ts | Adds aggregated error array and Axios diagnostics parser. |
| api/errorHandler.ts | Captures structured Axios diagnostics with error type. |
| api/axiosInstance.ts | Hardens token refresh logic, adds 401 interceptor, and refined expiry calculations. |
Files not reviewed (1)
- OwnTube.tv/package-lock.json: Language not supported
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 25 out of 26 changed files in this pull request and generated 4 comments.
Files not reviewed (1)
- OwnTube.tv/package-lock.json: Language not supported
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
|
@claude Please review! |
|
Claude finished @mblomdahl's task —— View job Code Review Complete ✅I've reviewed the error handling improvements in this PR. Overall, this is a solid improvement to the application's error handling and diagnostics infrastructure. Below are my findings: 🎯 Strengths
🐛 Issues Found1. Typo in ErrorBoundary (OwnTube.tv/components/ErrorBoundary.tsx:30)There's a typo: "occurred" is misspelled as "occured" // Line 30 - Should be "occurred" not "occured" error occurred${...`}2. Potential null pointer in collection error handlingSeveral screens access
Recommendation: Use optional chaining consistently: const { title, description } = getErrorTextKeys(channelsError || channelsCollectionError?.[0] || null);3. Token refresh timing issue (OwnTube.tv/api/axiosInstance.ts:103-104)The token refresh is triggered when const halfway = accessIssued && accessExpiresInNum ? accessIssued + accessExpiresInNum * 0.5 : 0;
if ((now > halfway && refreshToken && shouldAttachAccessToken) || (!accessTokenValid && refreshTokenValid)) {Issue: If we're past halfway but the access token is still valid ( Recommendation: Only refresh if we're past 80-90% of the token's lifetime OR if the token has expired: const refreshThreshold = accessIssued && accessExpiresInNum ? accessIssued + accessExpiresInNum * 0.8 : 0;
if ((now > refreshThreshold && refreshToken && shouldAttachAccessToken && accessTokenValid) ||
(!accessTokenValid && refreshTokenValid)) {4. Missing dependency in useEffect (OwnTube.tv/hooks/useAuthSessionSync.tsx:40)The Recommendation: Either include the function in dependencies or use const handleCheckSessionExistence = useCallback(async () => {
if (backend) {
await selectSession(backend);
} else {
clearSession();
}
}, [backend, selectSession, clearSession]);💡 Suggestions for Improvement5. Consider debouncing the token refresh (OwnTube.tv/api/axiosInstance.ts)While you have a lock mechanism ( const REFRESH_PROMISES: Record<string, Promise<any> | null> = {};
const refreshAccessToken = async (backend: string, refreshToken: string) => {
if (REFRESH_PROMISES[backend]) {
return REFRESH_PROMISES[backend]; // Wait for existing refresh
}
const promise = (async () => {
try {
// ... existing refresh logic
} finally {
REFRESH_PROMISES[backend] = null;
}
})();
REFRESH_PROMISES[backend] = promise;
return promise;
};6. Button text color calculation (OwnTube.tv/components/shared/Button.tsx:57-58)The text color logic is a bit complex. Consider extracting it to a separate function: const getTextColor = useCallback(() => {
if (textColor) return textColor;
if (disabled) return colors.themeDesaturated500;
return contrast === "high" ? colors.white94 : colors.theme900;
}, [textColor, disabled, contrast, colors]);7. Error boundary placement (OwnTube.tv/app/_layout.tsx:192)Good move placing ErrorBoundary higher in the tree! This ensures errors in providers are caught. Consider adding error logging to track which component caused the error. 🔒 Security Notes✅ Good job on sanitizing error data - The ✅ Token handling looks secure - Tokens are never logged or exposed in error messages 📝 Code Style
🧪 Testing RecommendationsSince this PR touches critical authentication and error handling:
✅ VerdictApprove with minor fixes recommended. The core improvements are solid and will significantly improve error handling. The issues identified are mostly minor (except the token refresh timing, which could cause performance issues under heavy load). Priority fixes:
Nice-to-have: Great work on improving the diagnostics infrastructure! 🎉 Job run: https://github.com/OwnTube-tv/web-client/actions/runs/ |
|
@claude I think the issue no. 1 about the "occurred" typo is already fixed. Is it not? |
|
Claude finished @mblomdahl's task —— View job Verification Complete ✅You're absolutely right, @mblomdahl! I apologize for the confusion. I just checked ```typescript error occurred${...`} |
mblomdahl
left a comment
mblomdahl
left a comment
There was a problem hiding this comment.
@mykhailodanilenko Please assess the PR review from Claude and correct the things you think are valid issues + nice-to-haves. Tag me for re-review after!
…; improve code readability
|
@claude About issue 3 that you mentioned, "token refresh timing issue", the idea is to minimize the risk of intermittent users finding themselves with expired refresh tokens. We don't control the token expiration time, but it's usually 2 weeks, and if a user is visiting randomly a few times per month we want to maximize for them still being logged in on next visit. Does that make sense? How can this code be made more understandable to humans and AI agents alike? |
|
Claude finished @mblomdahl's task —— View job Response to Token Refresh Timing Question
|
|
@claude About issue 7 from your initial review, please elaborate. How can we do this, "Consider adding error logging to track which component caused the error."? Propose solution. |
|
Claude finished @mblomdahl's task —— View job Elaborating on Error Boundary Component Tracking ✅
Solution: Enhanced Error Boundary with Component TrackingGreat question! Currently, the ErrorBoundary at React's Proposed ImplementationHere's how to enhance the ErrorBoundary to track which component caused errors: 1. Add a new exception type to diagnostics constantsAdd to export enum CustomPostHogExceptions {
OutOfMemory = "out_of_memory",
VideoPlayerError = "video_player_error",
HttpRequestError = "http_request_error",
ChromecastError = "chromecast_error",
TokenError = "token_error",
RateLimitError = "rate_limit_error",
ComponentRenderError = "component_render_error", // NEW
}2. Parse the component stack to extract meaningful infoAdd a helper function to extract the most relevant component from the stack: // OwnTube.tv/components/ErrorBoundary.tsx
/**
* Extracts the component name that likely caused the error from React's component stack.
* Returns the first custom component (non-React internal) in the stack.
*
* @example
* Input: " at VideoGrid (src/components/VideoGrid.tsx:45)\n at CategoryScreen..."
* Output: { componentName: "VideoGrid", filePath: "src/components/VideoGrid.tsx", lineNumber: 45 }
*/
const parseComponentStack = (componentStack?: string): {
componentName?: string;
filePath?: string;
lineNumber?: number;
fullStack?: string;
} => {
if (!componentStack) {
return {};
}
// Split by lines and find the first non-React internal component
const lines = componentStack.split('\n').filter(line => line.trim());
for (const line of lines) {
// Match patterns like: "at ComponentName (path/to/file.tsx:123)"
const match = line.match(/at\s+(\w+)\s+\((.+?):(\d+)(?::\d+)?\)/);
if (match) {
const [, componentName, filePath, lineNumber] = match;
// Skip React internals and common wrappers
if (!['ErrorBoundary', 'Suspense', 'Fragment'].includes(componentName)) {
return {
componentName,
filePath,
lineNumber: parseInt(lineNumber, 10),
fullStack: componentStack,
};
}
}
}
// Fallback: return the full stack if we couldn't parse it
return { fullStack: componentStack };
};3. Update componentDidCatch to capture enriched dataReplace the current implementation in componentDidCatch(error: unknown, info: { componentStack?: string }) {
this.setState({ ...this.state, error, info });
// Parse component stack to identify the failing component
const componentInfo = parseComponentStack(info.componentStack);
// Log to console for development debugging
console.error('ErrorBoundary caught error:', {
error,
component: componentInfo.componentName,
location: componentInfo.filePath
? `${componentInfo.filePath}:${componentInfo.lineNumber}`
: 'unknown',
});
console.error('Full component stack:', info.componentStack);
// Capture to PostHog with structured metadata
postHogInstance.captureException(error, {
errorType: CustomPostHogExceptions.ComponentRenderError,
componentName: componentInfo.componentName,
filePath: componentInfo.filePath,
lineNumber: componentInfo.lineNumber,
componentStack: componentInfo.fullStack,
errorMessage: (error as Error)?.message,
errorName: (error as Error)?.name,
});
}4. Add TypeScript type for the error metadata (optional but recommended)Create a type for consistency: interface ComponentErrorMetadata {
errorType: CustomPostHogExceptions.ComponentRenderError;
componentName?: string;
filePath?: string;
lineNumber?: number;
componentStack?: string;
errorMessage?: string;
errorName?: string;
}Benefits of This Approach
Example PostHog EventAfter implementation, when a component crashes, PostHog will capture: {
"event": "$exception",
"properties": {
"errorType": "component_render_error",
"componentName": "VideoGrid",
"filePath": "OwnTube.tv/components/VideoGrid/index.tsx",
"lineNumber": 67,
"errorMessage": "Cannot read property 'map' of undefined",
"errorName": "TypeError",
"componentStack": " at VideoGrid (src/components/VideoGrid/index.tsx:67)\n at CategoryScreen..."
}
}Testing the ImplementationYou can test this by intentionally throwing an error in a component: // In any component for testing
useEffect(() => {
if (testErrorBoundary) {
throw new Error('Test error boundary tracking');
}
}, [testErrorBoundary]);Then check PostHog to verify the component name, file path, and line number are captured correctly. Alternative: Use Error Boundary per RouteFor even more granular tracking, you could wrap individual routes/screens with named error boundaries: const NamedErrorBoundary = ({ name, children }: { name: string; children: ReactNode }) => {
// Similar implementation but includes `name` in PostHog capture
};
// Then in your routes:
<NamedErrorBoundary name="VideoScreen">
<VideoScreen />
</NamedErrorBoundary>This approach would tell you exactly which screen failed, even if the component stack is unclear. SummaryThe key improvements are:
Would you like me to implement these changes, or do you have questions about the approach? Job run: https://github.com/OwnTube-tv/web-client/actions/runs/18336375395 |
Replaces #457, to get nice AI-assisted reviews from Claude. (As shown in #458, it does not work with cross-repo PRs yet, so we need to pause using forks as the source.)
🚀 Description
This PR improves the error handling mechanisms and logging across the application, while fixing some of the errors related to refresh token handling. Also, a small test for error handling is added to the "copy diagnostic info" button, press it 5 times in a row and observe the result.
📄 Motivation and Context
#451
🧪 How Has This Been Tested?
📦 Types of changes
✅ Checklist (copied from README)
npx eslint .andnpx prettier --check ../from the./OwnTube.tv/directory (without errors/warnings)