-
Notifications
You must be signed in to change notification settings - Fork 2.4k
fix refresh crashing app #4296
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix refresh crashing app #4296
Changes from all commits
7ce21b2
7c62583
68f50bb
3ade8bb
2cf099d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,6 +1,13 @@ | ||
| import { useEffect, useRef, useState } from 'react'; | ||
| import { IpcRendererEvent } from 'electron'; | ||
| import { HashRouter, Routes, Route, useNavigate, useLocation } from 'react-router-dom'; | ||
| import { | ||
| HashRouter, | ||
| Routes, | ||
| Route, | ||
| useNavigate, | ||
| useLocation, | ||
| NavigateFunction, | ||
| } from 'react-router-dom'; | ||
| import { ErrorUI } from './components/ErrorBoundary'; | ||
| import { ExtensionInstallModal } from './components/modals/ExtensionInstallModal'; | ||
| import { useExtensionInstallModal } from './hooks/useExtensionInstallModal'; | ||
|
|
@@ -349,6 +356,21 @@ const ExtensionsRoute = () => { | |
| ); | ||
| }; | ||
|
|
||
| // Component to capture navigate function and provide it via callback | ||
| const NavigateCapture = ({ | ||
| onNavigateReady, | ||
| }: { | ||
| onNavigateReady: (navigate: NavigateFunction) => void; | ||
| }) => { | ||
| const navigate = useNavigate(); | ||
|
|
||
| useEffect(() => { | ||
| onNavigateReady(navigate); | ||
| }, [navigate, onNavigateReady]); | ||
|
|
||
| return null; | ||
| }; | ||
|
|
||
| export default function App() { | ||
| const [fatalError, setFatalError] = useState<string | null>(null); | ||
| const [isLoadingSession, setIsLoadingSession] = useState(false); | ||
|
|
@@ -369,50 +391,67 @@ export default function App() { | |
| const { modalState, modalConfig, dismissModal, confirmInstall } = | ||
| useExtensionInstallModal(addExtension); | ||
|
|
||
| // Create a setView function for useChat hook - we'll use window.history instead of navigate | ||
| // Create a setView function for useChat hook | ||
| const navigateRef = useRef<NavigateFunction | null>(null); | ||
| const setView = (view: View, viewOptions: ViewOptions = {}) => { | ||
| console.log(`Setting view to: ${view}`, viewOptions); | ||
| console.trace('setView called from:'); // This will show the call stack | ||
| // Convert view to route navigation using hash routing | ||
| switch (view) { | ||
| case 'chat': | ||
| window.location.hash = '#/'; | ||
| break; | ||
| case 'pair': | ||
| window.location.hash = '#/pair'; | ||
| break; | ||
| case 'settings': | ||
| window.location.hash = '#/settings'; | ||
| break; | ||
| case 'extensions': | ||
| window.location.hash = '#/extensions'; | ||
| break; | ||
| case 'sessions': | ||
| window.location.hash = '#/sessions'; | ||
| break; | ||
| case 'schedules': | ||
| window.location.hash = '#/schedules'; | ||
| break; | ||
| case 'recipes': | ||
| window.location.hash = '#/recipes'; | ||
| break; | ||
| case 'permission': | ||
| window.location.hash = '#/permission'; | ||
| break; | ||
| case 'ConfigureProviders': | ||
| window.location.hash = '#/configure-providers'; | ||
| break; | ||
| case 'recipeEditor': | ||
| window.location.hash = '#/recipe-editor'; | ||
| break; | ||
| case 'welcome': | ||
| window.location.hash = '#/welcome'; | ||
| break; | ||
| default: | ||
| console.error(`Unknown view: ${view}, not navigating anywhere. This is likely a bug.`); | ||
| console.trace('Invalid setView call stack:'); | ||
| // Don't navigate anywhere for unknown views to avoid unexpected redirects | ||
| break; | ||
|
|
||
| // Use React Router navigation if available, otherwise fallback to hash manipulation | ||
| // navigateRef.current may not be available during: | ||
| // - Initial app startup before React Router is fully initialized | ||
| // - Component unmounting/remounting cycles | ||
| // - Error boundary scenarios where React Router context is lost | ||
| // - When rendered outside of Router context (though this shouldn't happen in our app) | ||
| // - Race conditions during rapid navigation or window creation | ||
| if (navigateRef.current) { | ||
| const navigationHandler = createNavigationHandler(navigateRef.current); | ||
| navigationHandler(view, viewOptions); | ||
| } else { | ||
| // Fallback to hash manipulation for cases where navigate isn't available yet | ||
| // This is a legacy implementation that directly manipulates the URL hash | ||
| // as a backup when React Router navigation is not available | ||
| console.warn('Navigate function not available, using hash fallback'); | ||
| switch (view) { | ||
| case 'chat': | ||
| window.location.hash = '#/'; | ||
| break; | ||
| case 'pair': | ||
| window.location.hash = '#/pair'; | ||
| break; | ||
| case 'settings': | ||
| window.location.hash = '#/settings'; | ||
| break; | ||
| case 'extensions': | ||
| window.location.hash = '#/extensions'; | ||
| break; | ||
| case 'sessions': | ||
| window.location.hash = '#/sessions'; | ||
| break; | ||
| case 'schedules': | ||
| window.location.hash = '#/schedules'; | ||
| break; | ||
| case 'recipes': | ||
| window.location.hash = '#/recipes'; | ||
| break; | ||
| case 'permission': | ||
| window.location.hash = '#/permission'; | ||
| break; | ||
| case 'ConfigureProviders': | ||
| window.location.hash = '#/configure-providers'; | ||
| break; | ||
| case 'recipeEditor': | ||
| window.location.hash = '#/recipe-editor'; | ||
| break; | ||
| case 'welcome': | ||
| window.location.hash = '#/welcome'; | ||
| break; | ||
| default: | ||
| console.error(`Unknown view: ${view}, not navigating anywhere. This is likely a bug.`); | ||
| console.trace('Invalid setView call stack:'); | ||
| // Don't navigate anywhere for unknown views to avoid unexpected redirects | ||
| break; | ||
| } | ||
| } | ||
| }; | ||
|
|
||
|
|
@@ -712,6 +751,11 @@ export default function App() { | |
| onCancel={dismissModal} | ||
| isSubmitting={modalState.isPending} | ||
| /> | ||
| <NavigateCapture | ||
| onNavigateReady={(navigate) => { | ||
| navigateRef.current = navigate; | ||
| }} | ||
| /> | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. you'll have to explain to me why we need this.
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Basically |
||
| <div className="relative w-screen h-screen overflow-hidden bg-background-muted flex flex-col"> | ||
| <div className="titlebar-drag-region" /> | ||
| <Routes> | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -36,6 +36,7 @@ import { | |
| updateEnvironmentVariables, | ||
| updateSchedulingEngineEnvironment, | ||
| } from './utils/settings'; | ||
| import localStorageInjectionScript from './utils/localStorageInjectionScript'; | ||
| import * as crypto from 'crypto'; | ||
| // import electron from "electron"; | ||
| import * as yaml from 'yaml'; | ||
|
|
@@ -655,35 +656,19 @@ const createChat = async ( | |
| // We need to wait for the window to load before we can access localStorage | ||
| mainWindow.webContents.on('did-finish-load', () => { | ||
| const configStr = JSON.stringify(windowConfig).replace(/'/g, "\\'"); | ||
| mainWindow.webContents | ||
| .executeJavaScript( | ||
| ` | ||
| (function() { | ||
| function setConfig() { | ||
| try { | ||
| if (window.localStorage) { | ||
| localStorage.setItem('gooseConfig', '${configStr}'); | ||
| return true; | ||
| } | ||
| } catch (e) { | ||
| console.warn('localStorage access failed:', e); | ||
| } | ||
| return false; | ||
| } | ||
|
|
||
| if (!setConfig()) { | ||
| setTimeout(() => { | ||
| if (!setConfig()) { | ||
| console.error('Failed to set localStorage after retry - continuing without localStorage config'); | ||
| } | ||
| }, 100); | ||
| } | ||
| })(); | ||
| ` | ||
| ) | ||
| .catch((error) => { | ||
| console.error('Failed to execute localStorage script:', error); | ||
| }); | ||
| // This JavaScript is injected into the renderer process to set localStorage with retry logic. | ||
| // The retry mechanism is necessary because localStorage may not be immediately available | ||
| // during Electron renderer initialization, especially on slower systems or during heavy load. | ||
| // We use executeJavaScript from the main process because: | ||
| // 1. The main process needs to pass configuration data to the renderer process | ||
| // 2. This happens during window initialization before the renderer's React app is ready | ||
| // 3. The timing is critical - we need to set config before React components try to read it | ||
| // 4. Direct IPC communication would require the renderer to be fully loaded first | ||
| const injectionScript = localStorageInjectionScript('gooseConfig', configStr); | ||
| mainWindow.webContents.executeJavaScript(injectionScript).catch((error) => { | ||
| console.error('Failed to execute localStorage script:', error); | ||
| }); | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why again do we want this to be injected into local storage? we seem to be loading this with: couldn't we store it locally in the renderer and when it can't be found just do an ipc call back to the electron processs? or expose it using exposeInMainWorld that I just learned about?
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agreed this can probably be improved with some As for why we put config in local storage I'm guessing its so it can be loaded faster from multiple windows but it may be a legacy thing that we could come back to and remove also. |
||
| }); | ||
|
|
||
| // Handle new window creation for links | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,50 @@ | ||
| /** | ||
| * Generates the JavaScript code to inject into the renderer process for setting localStorage | ||
| * with retry logic. This is used by the main process to inject configuration. | ||
| * | ||
| * @param key The localStorage key to set | ||
| * @param value The value to store (will be JSON stringified if not already a string) | ||
| * @returns JavaScript code as a string | ||
| */ | ||
| export default function generateLocalStorageInjectionScript(key: string, value: string): string { | ||
| return ` | ||
| (function() { | ||
| let retryCount = 0; | ||
| const maxRetries = 5; | ||
| const baseDelay = 100; | ||
|
|
||
| function setConfig() { | ||
| try { | ||
| if (window.localStorage && typeof window.localStorage.setItem === 'function') { | ||
| localStorage.setItem('${key}', '${value}'); | ||
| console.log('[Renderer] Successfully set localStorage ${key}'); | ||
| return true; | ||
| } else { | ||
| console.warn('[Renderer] localStorage not available or setItem not a function'); | ||
| } | ||
| } catch (e) { | ||
| console.warn('[Renderer] localStorage access failed:', e); | ||
| } | ||
| return false; | ||
| } | ||
|
|
||
| function retrySetConfig() { | ||
| if (setConfig()) { | ||
| return; // Success, no need to retry | ||
| } | ||
|
|
||
| retryCount++; | ||
| if (retryCount < maxRetries) { | ||
| const delay = baseDelay * Math.pow(2, retryCount - 1); // Exponential backoff | ||
| console.log(\`[Renderer] Retrying localStorage ${key} set (attempt \${retryCount + 1}/\${maxRetries}) in \${delay}ms\`); | ||
| setTimeout(retrySetConfig, delay); | ||
| } else { | ||
| console.error('[Renderer] Failed to set localStorage ${key} after all retries - continuing without localStorage config'); | ||
| } | ||
| } | ||
|
|
||
| // Initial attempt | ||
| retrySetConfig(); | ||
| })(); | ||
| `; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When is it not available?