Add loading state spinner for job and workflow list views #9095
Conversation
📝 WalkthroughWalkthroughReplaced manual polling and local loading state with react-query ( Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20–30 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
frontend/javascripts/admin/voxelytics/workflow_list_view.tsx (1)
4-4: Spinner integration looks good; consider conditionalemptyTextand polling UXWrapping the Table in
<Spin spinning={isLoading} size="large">and importingSpincorrectly ties the loading indicator to the existingisLoadingstate, so the list now shows a proper spinner during fetches instead of “no data”.Two small follow‑ups to consider:
locale={{ emptyText: null }}suppresses the “no data” message even after loading completes and there truly are no workflows, which can make the view look broken. You can keep the “no data” message once loading is done while still hiding it during loading by makingemptyTextconditional onisLoading, e.g.:locale={{ emptyText: null }}
locale={isLoading ? { emptyText: null } : undefined}
- Because
usePollingcallsloadData(), the spinner will also show on every polling refresh. IfVX_POLLING_INTERVALis short and requests are slow, this can briefly block the table on each poll. If that turns out to be distracting, consider a separate “silent” refresh function for polling that doesn’t toggleisLoading, similar to how the jobs view’s interval fetch keeps the spinner for initial/manual loads only.Also applies to: 187-279
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
frontend/javascripts/admin/job/job_list_view.tsx(1 hunks)frontend/javascripts/admin/voxelytics/workflow_list_view.tsx(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
frontend/javascripts/admin/job/job_list_view.tsx (1)
frontend/javascripts/admin/api/jobs.ts (1)
getJobs(47-56)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build-smoketest-push
- GitHub Check: backend-tests
🔇 Additional comments (1)
frontend/javascripts/admin/job/job_list_view.tsx (1)
147-153: Loading state handling aroundfetchDatais solidWrapping
getJobs()in atry/finallythat togglesisLoadingensures the existing<Spin spinning={isLoading}>now accurately reflects initial load and any explicit refreshes (e.g., after cancel/retry) without affecting the lightweight interval updates.
|
@hotzenklotz I already did this in #9068 🙈 |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
frontend/javascripts/admin/voxelytics/workflow_list_view.tsx (2)
173-175: Minor: simplify refresh handler and optionally reflect loading on the buttonThe Refresh button wiring is fine, but you can tighten it up and improve UX slightly:
- Call
refetchdirectly instead of wrapping in an arrow.- Optionally, set
loading={isFetching}(ordisabled={isFetching}) on the button so users get clearer feedback when a manual refresh is in flight.For example:
- <Button onClick={() => refetch()} style={{ marginRight: 20 }}> - <SyncOutlined spin={isFetching} /> Refresh - </Button> + <Button onClick={refetch} loading={isFetching} style={{ marginRight: 20 }}> + <SyncOutlined spin={isFetching} /> Refresh + </Button>
185-276: Empty state now fully suppressed; consider conditional empty text once loading is doneWrapping the table in
<Spin>tied toisLoadingnicely fixes the “no data during load” issue, andlocale={{ emptyText: null }}ensures AntD doesn’t show its default empty text while the list is loading.However, with
emptyText: null, users will also see a completely blank table when there are genuinely zero workflows (after loading or an error), which may be confusing.If you still want an explicit empty state outside of the loading phase, consider making
emptyTextconditional on the query state, e.g.:- <Table + <Table ... - locale={{ emptyText: null }} + locale={{ + emptyText: + isLoading || isFetching || renderRuns.length > 0 + ? null + : "No workflows found", + }} />This keeps the table clean while loading, but restores a clear “no data” message when appropriate.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
frontend/javascripts/admin/job/job_list_view.tsx(5 hunks)frontend/javascripts/admin/voxelytics/workflow_list_view.tsx(4 hunks)unreleased_changes/9095.md(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- unreleased_changes/9095.md
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/javascripts/admin/job/job_list_view.tsx
🧰 Additional context used
🧬 Code graph analysis (1)
frontend/javascripts/admin/voxelytics/workflow_list_view.tsx (3)
frontend/javascripts/admin/rest_api.ts (1)
getVoxelyticsWorkflows(2390-2392)frontend/javascripts/admin/voxelytics/utils.ts (1)
VX_POLLING_INTERVAL(5-5)frontend/javascripts/libs/format_utils.ts (3)
formatCountToDataAmountUnit(333-346)formatNumber(540-542)formatDateMedium(451-453)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: frontend-tests
- GitHub Check: backend-tests
- GitHub Check: build-smoketest-push
🔇 Additional comments (1)
frontend/javascripts/admin/voxelytics/workflow_list_view.tsx (1)
77-94: Incorrect technical claim about TanStack Query refetchIntervalThe TanStack Query documentation states that the default refetchInterval is 0, which disables polling refetching. The review comment claims that
refetchInterval: VX_POLLING_INTERVAL ?? 0results in "tight-loop polling" that will "hammer the workflows endpoint," but this is factually incorrect. Setting refetchInterval to 0 disables periodic refetching, not enables it.Since
VX_POLLING_INTERVALisnull(as confirmed infrontend/javascripts/admin/voxelytics/utils.tsline 5), the current code safely falls back to 0 and does not poll. Changing tofalseis a stylistic improvement for clarity, but the code does not present the performance hazard described.The error handling concern about Toast appearing multiple times with default retries is a separate valid point, but the core performance claim is incorrect.
Likely an incorrect or invalid review comment.
philippotto
left a comment
philippotto
left a comment
There was a problem hiding this comment.
great 👍 only see my one comment. didn't test because you already did that.
| throw err; | ||
| } | ||
| }, | ||
| refetchInterval: VX_POLLING_INTERVAL ?? 0, |
There was a problem hiding this comment.
according to the docs, this should be: refetchInterval: number | false | ((query: Query) => number | false | undefined). so, passing 0 as a fallback sounds a bit dangerous. can you use ?? false?
This PR fixes an issues with the VX Worfklow view. After a while or on page refresh it would show the following error message: <img width="1207" height="613" alt="Screenshot 2025-12-01 at 10 56 04" src="https://github.com/user-attachments/assets/db2cb363-72b4-4401-8687-bc6c3641b55a" /> 1. The error indicates that the workflow list date was not properly transformed, especially the string timestamps were not transformed into JS `Date` objects. 2. Upon closer inspection I noticed that this workflow data is being displayed (and failing) before the request to WK backend is even completed (or started). Hence, the data must be restored from cache. 3. In PR #9095, I introduced the use of `react-query` for fetching the workflow. React-query is configured to cache all requests in the [`localStorage`](https://vscode.dev/github/scalableminds/webknossos/blob/master/frontend/javascripts/main.tsx#L50-L51). In general, the cached data is serialized as JSON strings. 4. Is suspect, that the workflow list view data including the JS Date objects, is serialized as JSON strings when being cached. Upon restoring the cached JS Date objects are parsed/restored as regular strings not `Date` objects. 🤦 5. I fixed it by applying the workflow data timestamps to `Date` transformations in a separate `select` (=transform) step in the `react-query` runtime that is being executing both after fetching the data from the server and restoring from the cache. See docs: https://tanstack.com/query/v5/docs/framework/react/reference/useQuery ``` This option can be used to transform or select a part of the data returned by the query function. It affects the returned data value, but does not affect what gets stored in the query cache. The select function will only run if data changed, or if the reference to the select function itself changes. To optimize, wrap the function in useCallback. ``` ### Steps to test: I have not found a way to manual trigger the above error yet. :-/ To test the propose solution: - Clear your browser `localStorage` - Open the VX workflow list and make sure that it is populated correctly - Refresh page to trigger a cache response ### Issues: - fixes a regression from #9095 - fixes https://scm.slack.com/archives/C5AKLAV0B/p1764323131933529 ------ (Please delete unneeded items, merge only when none are left open) - [x] Added changelog entry (create a `$PR_NUMBER.md` file in `unreleased_changes` or use `./tools/create-changelog-entry.py`) - [ ] Added migration guide entry if applicable (edit the same file as for the changelog) - [ ] Updated [documentation](../blob/master/docs) if applicable - [ ] Adapted [wk-libs python client](https://github.com/scalableminds/webknossos-libs/tree/master/webknossos/webknossos/client) if relevant API parts change - [ ] Removed dev-only changes like prints and application.conf edits - [ ] Considered [common edge cases](../blob/master/.github/common_edge_cases.md) - [ ] Needs datastore update after deployment
### Description - Unified display of ids, allowing single-click copy. - Added link to workflow report to job list (superusers only for now). - Speed up loading of job list. - ~Fixed loading indicators for job and workflow lists.~ already done in #9095 - Refactored APIJob type in frontend and backend to make them simpler and make them match ### Steps to test: - `yarn enable jobs` - start a few jobs - check that workflow list view looks as expected - check that id rendering in job list view, annotation list view, etc is pretty. ### TODOs: - [x] Fix Loading indicator - [x] also on workflow list view (loading indicator in topright is there, but table still says No Data) - [x] Workflow link for superusers if available - [x] Fix rendering of infer instances job - [x] Speed up loading (old code ca 5s on scm orga) - [x] complex query in list view - [x] adapt types - [x] get single job json should be superset of compact list json - [x] test ### Issues: - fixes #9051 ------ - [x] Added changelog entry (create a `$PR_NUMBER.md` file in `unreleased_changes` or use `./tools/create-changelog-entry.py`) - [x] Removed dev-only changes like prints and application.conf edits - [x] Considered [common edge cases](../blob/master/.github/common_edge_cases.md)
I fixed the loading state spinner for the job list view and added one for the VX workflow list view too. I also took the opportunity to replace the loading and polling code with standardized react-query hooks.
Steps to test:
Issues:
(Please delete unneeded items, merge only when none are left open)
$PR_NUMBER.mdfile inunreleased_changesor use./tools/create-changelog-entry.py)