fix refresh crashing app #4296
fix refresh crashing app #4296
Conversation
…fore its available
…uting to avoid protocol errors in electron
| break; | ||
|
|
||
| // Use React Router navigation if available, otherwise fallback to hash manipulation | ||
| if (navigateRef.current) { |
There was a problem hiding this comment.
When is it not available?
ui/desktop/src/main.ts
Outdated
| const configStr = JSON.stringify(windowConfig).replace(/'/g, "\\'"); | ||
| mainWindow.webContents | ||
| .executeJavaScript( | ||
| ` |
There was a problem hiding this comment.
A bit outside the scope of this PR, but why do we need to manually call executeJavaScript on this block?
Could we not move it to some renderer side ts?
|
@alexhancock I can see how this would be confusing so I added comments to both of these. Let me know if it makes sense or not. |
* 'main' of github.com:block/goose: fix slow typing with long sessions (#4291) docs: improvements to release docs (#4317) blog: mcp-ui recap blog (#4146) added more logging for recipe creation failures (#4299) Remove a bit from pre-commit that husky says we should (#4286) feat: autovisualiser of structured data with mcp-ui (#4153) docs: Plan tutorial (#4309) Extensions Modal Improvements (#4293) docs: fixed cicd tutorial pipeline in docs (#4223) Read oltp config from config and env (#4292) # Conflicts: # ui/desktop/src/App.tsx
DOsinga
left a comment
DOsinga
left a comment
There was a problem hiding this comment.
we should maybe discuss this some more, this does feel like we're trying to fix stuff on top of shaky stuff maybe?
| onNavigateReady={(navigate) => { | ||
| navigateRef.current = navigate; | ||
| }} | ||
| /> |
There was a problem hiding this comment.
you'll have to explain to me why we need this.
There was a problem hiding this comment.
Basically setView was being called before react router was ready during direct navigation or refresh so now it will capture it and use it once its ready. We prefer to use react router navigation anyway so it should improve things. That being said I can leave this part out for now and only do the local storage changes to see if that is enough to fix the refresh issue.
| const injectionScript = localStorageInjectionScript('gooseConfig', configStr); | ||
| mainWindow.webContents.executeJavaScript(injectionScript).catch((error) => { | ||
| console.error('Failed to execute localStorage script:', error); | ||
| }); |
There was a problem hiding this comment.
why again do we want this to be injected into local storage? we seem to be loading this with:
getConfig: () => {
// Add fallback to localStorage if config from preload is empty or missing
if (!config || Object.keys(config).length === 0) {
try {
if (window.localStorage) {
const storedConfig = localStorage.getItem('gooseConfig');
if (storedConfig) {
return JSON.parse(storedConfig);
}
}
} catch (e) {
console.warn('Failed to parse stored config from localStorage:', e);
}
}
return config;
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?
There was a problem hiding this comment.
Agreed this can probably be improved with some exposeInMainWorld logic but unfortunately would require more time so I think we should come back to that in a bigger refactor. IPC would require async calls that happen further down the chain and could cause race conditions. This is just a way to get it called as early as possible and have retries to fix the main error that localstorage isn't available to the browser yet when it tries to access it.
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.
|
Appreciate the feedback! Closing this in favor of a more targeted fix #4355 |
Made the fallbacks for checking local storage more resilient. Also added a backup mechanism for navigation since electron can have issues with hash based routing on load.
fixes #4255