fix: remove outdated import/export jobs#6080
Conversation
🐳 Docker image sizes
|
There was a problem hiding this comment.
Pull request overview
Removes stale dataset import tracking entries (stored client-side) when the referenced backend job no longer exists, addressing “old import messages reappearing” scenarios (Issue #5919).
Changes:
- Extend
useImportJobStatus’sonErrorcallback to receive the underlying error object. - In
LoadingImportDataset, delete the local import entry when the job query fails with an “invalid job” error. - Update
LoadingImportDatasettests to reflect the new “delete entry on invalid job” behavior.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| application/ui/src/hooks/api/jobs/use-import-job-status.hook.tsx | Passes error object into onError and adjusts effect deps around error handling. |
| application/ui/src/components/loading-import-dataset/loading-import-dataset.component.tsx | Deletes stale import entries on invalid job errors; refactors success flow around staged-dataset deletion. |
| application/ui/src/components/loading-import-dataset/loading-import-dataset.test.tsx | Updates assertions to expect entry deletion on invalid job query failures and renames tests for clarity. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
📊 Test coverage report
|
| }; | ||
|
|
||
| it('renders failed job state and displays job message and error', async () => { | ||
| it('displays job message, error, and does not delete entry when job fails', async () => { |
There was a problem hiding this comment.
Why we don't remove the entry (staged-file) when import job fails? When that entry is removed?
There was a problem hiding this comment.
The requirement is to remove it when the job is invalid (line 95). For any other errors, we should display the error information so the user knows what happened. When they manually close the card, the staged file is removed.
There was a problem hiding this comment.
I understand that but the question is why we don't remove the entry (staged-file) when import job fails?
There was a problem hiding this comment.
To avoid having the local storage entry point to a staged file that no longer exists, it could be removed when an error occurs. However, I think it should remain until all of its references have been cleared.
camiloHimura
force-pushed
the
ccolora11/5919-import-errors
branch
from
cb6260a to
377c2ef
Compare
camiloHimura
force-pushed
the
ccolora11/5919-import-errors
branch
from
377c2ef to
134c226
Compare
| }); | ||
| }, | ||
| onError: (error) => { | ||
| if (isInvalidJob(error)) { |
There was a problem hiding this comment.
if we're doing it onError and onsuccess, can we not do it on onSettled?
There was a problem hiding this comment.
It’s possible, but I’d prefer not to. Under the hood, we’re using useEffect to trigger those events, and I want to avoid adding more than strictly necessary.
There was a problem hiding this comment.
i meant using onSettled: () => {
...the 2 lines that are repeated
}
wont it work?
There was a problem hiding this comment.
Yes, but we’d also need to add an extra event and additional validation, since deleteEntry and deleteStagedFileMutation.mutate are not called for every error
Summary
closes #5919
This PR removes import/export LS job entries when the ID is no longer valid.
How to test
Checklist