fix: Clear LS after the app closes#6049
Conversation
🐳 Docker image sizes
|
There was a problem hiding this comment.
Pull request overview
Addresses #6029 by preventing stale dataset-import job state from persisting across app restarts.
Changes:
- Added a session-storage JSON parsing helper and switched dataset-import storage from
localStoragetosessionStorage. - Invalidated the dataset items query after a successful dataset import to refresh dependent UI.
- Set the primary button to autofocus in the delete project dialog.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| application/ui/src/hooks/localStorage/utils.ts | Adds getParsedSessionStorage helper for safe JSON parsing/removal on failure. |
| application/ui/src/hooks/localStorage/use-dataset-import-storage.hook.ts | Migrates dataset import persistence from useLocalStorage to useSessionStorage. |
| application/ui/src/features/dataset/import-export/import-jobs-list/import-jobs-list.component.tsx | Invalidates /dataset/items queries after import success to refresh the dataset view. |
| application/ui/src/components/project-dialogs/delete-project-dialog.component.tsx | Ensures destructive action gets focus by default via autoFocusButton='primary'. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
📊 Test coverage report
|
|
|
||
| export const useDatasetImportStorage = <TStep, TEntry extends DatasetImportState<TStep>>(storageKey: string) => { | ||
| const [importEntries, setImportEntries] = useLocalStorage<TEntry[] | null>( | ||
| const [importEntries, setImportEntries] = useSessionStorage<TEntry[] | null>( |
There was a problem hiding this comment.
Why this change? IMO that was a nice feature that user could open different tabs with the app and still see the progress of the import.
There was a problem hiding this comment.
Local Storage doesn’t work well for the desktop version. When the user closes and reopens the app, all jobs are cleared, which leads to this error: #6029
Using session storage instead ensures that all job-related information is cleared automatically.
There was a problem hiding this comment.
IMO with this change we hide the problem instead of solving that properly. If I understand correctly, the issue right now is that when user opened the desktop app again the local storage might contain the outdated job that was removed automatically when closing the desktop app previously. What about clearing job state in the local storage when closing the app?
There was a problem hiding this comment.
I disagree. We need the storage to be cleared when the app closes, and session storage already gives us that behavior. Your approach adds more code to achieve the same result, so to me it doesn’t feel like the right solution.
There was a problem hiding this comment.
If we still need to keep the progress between tabs when using a browser, then the approach suggested by @dwesolow would be more preferable. It comes with some extra code, indeed, but the feature is nice :-)
camiloHimura
force-pushed
the
ccolora11/6029-import-error
branch
2 times, most recently
from
c4de0fe to
af1a238
Compare
| "description": "enables the default permissions", | ||
| "windows": ["main"], | ||
| "permissions": ["core:default"] | ||
| "permissions": ["core:default", "core:window:allow-on-close-requested"] |
There was a problem hiding this comment.
it grants permission to handle window close events, so we can clear the LS
| { params: { path: { project_id: projectId } } }, | ||
| ]), | ||
| }); | ||
| queryClient.invalidateQueries({ |
There was a problem hiding this comment.
why didnt we need this before?
There was a problem hiding this comment.
It was actually missing, this ticket was created because of it: #6078. However, it has its own dedicated PR, so I’ll remove that code from this PR.
| .forEach((key) => localStorage.removeItem(key)); | ||
| }; | ||
|
|
||
| const isTauri = (): boolean => '__TAURI_INTERNALS__' in window; |
There was a problem hiding this comment.
Can we extract Tauri-related code to another folder? We are mixing storage and tauri here
jpggvilaca
left a comment
jpggvilaca
left a comment
There was a problem hiding this comment.
PR title says we're replacing local with session storage but i dont see sessionStorage being used anywhere?
it was the original intention but we move to a different approach, so I'll update the title |
camiloHimura
changed the title
camiloHimura
force-pushed
the
ccolora11/6029-import-error
branch
from
af1a238 to
ba1a65f
Compare
Summary
Fixes #6029.
This PR adds a new Tauri handler that clears the local storage import/export information when the app closes, so no invalid IDs are refetched the next time it opens.
How to test
Checklist