fix: show inline errors for secondary data instead of spinning#1069
fix: show inline errors for secondary data instead of spinning#1069igboyes wants to merge 1 commit into
Conversation
Secondary (Tier 2) React Query consumers used `if (isPending || !data)`, which on a failed request fell into the loading branch and spun forever instead of surfacing the error. Add a shared `@base/QueryError` (a red `Alert` taking a `noun`) and gate it on `isError && !data` across lists, sidebar widgets, dialogs, banners, and the wrapper hooks that compose queries. The `&& !data` is load-bearing: in React Query v5 `error` and `data` coexist, so a bare `if (isError)` would blank an already-loaded view the moment a background refetch fails. Erroring only when there is nothing to show fixes the initial-load spin while keeping stale data on screen through background errors, and still narrows `data` for the success branch. Also fixes AnalysisDetail, which never checked its analysis query's error and would fall through to `analysis.ready` and crash. Update docs/queries.md and AGENTS.md to document the QueryError component and the isError && !data rule.
Summary by CodeRabbitRelease Notes
WalkthroughThis PR introduces inline error handling for secondary (Tier 2) React Query data fetches by creating a new Comment |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- There is now a lot of repeated
if (isError && !data) return <QueryError ...>; if (isPending) ...logic across components; consider extracting a small helper or wrapper hook for Tier 2 queries so the error/loading pattern is implemented in one place and harder to get subtly wrong in future changes. - In views backed by multiple queries (eg.
OtuList,FileManager,CurrentOtuContextProvider, analysis create flows), you now short-circuit to a top-level<QueryError>when any of the queries error with missing data; it may be worth revisiting which pieces of these views can still render with partial/stale data and localizingQueryErrorto just the failing section instead of blanking the whole view. - For places like
IndexDetailyou’re still mapping a broadisError(including non-404 failures) to<NotFound />; consider distinguishing genuine not-found from transient/network errors and usingQueryErrorfor the latter so users don’t see a 404-equivalent when the backend is temporarily unavailable.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- There is now a lot of repeated `if (isError && !data) return <QueryError ...>; if (isPending) ...` logic across components; consider extracting a small helper or wrapper hook for Tier 2 queries so the error/loading pattern is implemented in one place and harder to get subtly wrong in future changes.
- In views backed by multiple queries (eg. `OtuList`, `FileManager`, `CurrentOtuContextProvider`, analysis create flows), you now short-circuit to a top-level `<QueryError>` when any of the queries error with missing data; it may be worth revisiting which pieces of these views can still render with partial/stale data and localizing `QueryError` to just the failing section instead of blanking the whole view.
- For places like `IndexDetail` you’re still mapping a broad `isError` (including non-404 failures) to `<NotFound />`; consider distinguishing genuine not-found from transient/network errors and using `QueryError` for the latter so users don’t see a 404-equivalent when the backend is temporarily unavailable.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (4)
apps/web/src/analyses/components/AnalysisDetail.tsx (1)
37-39: ⚖️ Poor tradeoffImprecise error condition for dual-query scenario.
The condition
(isError || isSampleError) && (!analysis || !sample)can false-positive when one query has errored with stale data while the other is still pending. For example, if the analysis background refetch fails (leaving stale data) while the sample is on its initial load, this would show an error instead of the loading placeholder.The pattern should check that each errored query specifically lacks data:
if ((isError && !analysis) || (isSampleError && !sample)) { return <QueryError noun="analysis" />; }This ensures error UI appears only when a query has failed and has no data to display, aligning with the React Query v5
isError && !datapattern and avoiding false positives during mixed pending/error states.🔍 Suggested fix for precise dual-query error handling
- if ((isError || isSampleError) && (!analysis || !sample)) { + if ((isError && !analysis) || (isSampleError && !sample)) { return <QueryError noun="analysis" />; }apps/web/src/analyses/components/Create/CreateIimi.tsx (1)
52-54: ⚡ Quick winIncomplete
isError && !datapattern for mlModels query.The error condition checks
isErrorIndexes || isErrorMlModelswithout verifying that data is actually missing. Per the PR'sisError && !datapattern, this should also check!mlModelsto distinguish between "initial load failed" and "background refetch failed with stale data." The current check will show an error even whenmlModelshas stale data that could still be displayed.Note: The
indexesportion cannot implement the pattern fully becauseuseCompatibleIndexesalways returns an array (viadata ?? []), making it impossible to distinguish missing data from an empty result. That's a hook design constraint, butmlModelsdoes not have this limitation.♻️ Recommended fix to align with the pattern
- if (isErrorIndexes || isErrorMlModels) { + if (isErrorIndexes || (isErrorMlModels && !mlModels)) { return <QueryError noun="analysis options" />; }apps/web/src/analyses/components/Create/CreateNuvs.tsx (1)
58-60: ⚡ Quick winIncomplete
isError && !datapattern (same issue as CreateIimi).The error condition
isErrorIndexes || isErrorSubtractionschecks only the error flags without verifying missing data. This deviates from the PR'sisError && !datapattern and will show errors even when stale data could be displayed.The hooks' design (both return derived arrays/objects rather than exposing raw
data) makes implementing the full pattern difficult here, but the check is overly conservative compared to the documented pattern.apps/web/src/analyses/components/Create/CreatePathoscope.tsx (1)
58-60: ⚡ Quick winIncomplete
isError && !datapattern (same issue as CreateIimi and CreateNuvs).The error condition checks only
isErrorIndexes || isErrorSubtractionswithout verifying that data is missing. This deviates from the PR'sisError && !datapattern and will show error UI even when stale data from successful prior fetches could be displayed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 7364826a-a823-41de-8139-c7f5fb4f12e8
📒 Files selected for processing (41)
AGENTS.mdapps/web/src/account/components/AccountProfile.tsxapps/web/src/account/components/ApiKeyPermissions.tsxapps/web/src/account/components/ApiKeys.tsxapps/web/src/administration/components/Banners.tsxapps/web/src/administration/components/ServerSettings.tsxapps/web/src/analyses/components/AnalysisDetail.tsxapps/web/src/analyses/components/Create/CreateIimi.tsxapps/web/src/analyses/components/Create/CreateNuvs.tsxapps/web/src/analyses/components/Create/CreatePathoscope.tsxapps/web/src/analyses/components/Create/QuickAnalyze.tsxapps/web/src/analyses/hooks.tsapps/web/src/base/QueryError.tsxapps/web/src/groups/components/Groups.tsxapps/web/src/hmm/components/HmmInstall.tsxapps/web/src/hmm/components/HmmList.tsxapps/web/src/indexes/components/IndexDetail.tsxapps/web/src/indexes/components/Indexes.tsxapps/web/src/indexes/components/RebuildAlert.tsxapps/web/src/jobs/components/JobList.tsxapps/web/src/labels/components/Labels.tsxapps/web/src/otus/components/Detail/OtuSection.tsxapps/web/src/otus/components/Detail/Schema/Schema.tsxapps/web/src/otus/components/OtuList.tsxapps/web/src/otus/queries.tsxapps/web/src/references/components/Detail/AddReferenceGroup.tsxapps/web/src/references/components/Detail/AddReferenceUser.tsxapps/web/src/references/components/Detail/ReferenceManager.tsxapps/web/src/references/components/ReferenceList.tsxapps/web/src/routes/_authenticated/samples/$sampleId/general.tsxapps/web/src/routes/_authenticated/samples/create.tsxapps/web/src/routes/_authenticated/samples/index.tsxapps/web/src/samples/components/SamplesList.tsxapps/web/src/samples/components/Sidebar/DefaultSubtractions.tsxapps/web/src/samples/hooks.tsapps/web/src/subtraction/components/SubtractionCreate.tsxapps/web/src/subtraction/components/SubtractionList.tsxapps/web/src/uploads/components/FileManager.tsxapps/web/src/users/components/UserGroups.tsxapps/web/src/users/components/UsersList.tsxdocs/queries.md
| type UseCompatibleIndexesResult = { | ||
| indexes: IndexMinimal[]; | ||
| isPending: boolean; | ||
| isError: boolean; | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Add JSDoc comment for exported type.
Per coding guidelines, every exported type must have a one-line JSDoc comment. UseCompatibleIndexesResult is missing its documentation.
📝 Proposed fix
+/** Result of useCompatibleIndexes hook providing ready, non-archived indexes grouped by reference. */
type UseCompatibleIndexesResult = {
indexes: IndexMinimal[];
isPending: boolean;
isError: boolean;
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| type UseCompatibleIndexesResult = { | |
| indexes: IndexMinimal[]; | |
| isPending: boolean; | |
| isError: boolean; | |
| }; | |
| /** Result of useCompatibleIndexes hook providing ready, non-archived indexes grouped by reference. */ | |
| type UseCompatibleIndexesResult = { | |
| indexes: IndexMinimal[]; | |
| isPending: boolean; | |
| isError: boolean; | |
| }; |
Source: Coding guidelines
| type UseSubtractionOptionsResult = { | ||
| defaultSubtractions: SubtractionOption[]; | ||
| subtractions: SubtractionOption[]; | ||
| isPending: boolean; | ||
| isError: boolean; | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Add JSDoc comment for exported type.
Per coding guidelines, every exported type must have a one-line JSDoc comment. UseSubtractionOptionsResult is missing its documentation.
📝 Proposed fix
+/** Result of useSubtractionOptions hook providing available subtractions and defaults for sample analysis. */
type UseSubtractionOptionsResult = {
defaultSubtractions: SubtractionOption[];
subtractions: SubtractionOption[];
isPending: boolean;
isError: boolean;
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| type UseSubtractionOptionsResult = { | |
| defaultSubtractions: SubtractionOption[]; | |
| subtractions: SubtractionOption[]; | |
| isPending: boolean; | |
| isError: boolean; | |
| }; | |
| /** Result of useSubtractionOptions hook providing available subtractions and defaults for sample analysis. */ | |
| type UseSubtractionOptionsResult = { | |
| defaultSubtractions: SubtractionOption[]; | |
| subtractions: SubtractionOption[]; | |
| isPending: boolean; | |
| isError: boolean; | |
| }; |
Source: Coding guidelines
| type QueryErrorProps = { | ||
| noun: string; | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Add JSDoc comment for exported type.
Per coding guidelines, every exported type must have a one-line JSDoc comment. QueryErrorProps is missing its documentation.
📝 Proposed fix
+/** Props for rendering an inline query error message. */
type QueryErrorProps = {
noun: string;
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| type QueryErrorProps = { | |
| noun: string; | |
| }; | |
| /** Props for rendering an inline query error message. */ | |
| type QueryErrorProps = { | |
| noun: string; | |
| }; |
Source: Coding guidelines
| const { data: selectedGroup } = useFetchGroup(selectedGroupId ?? 0); | ||
|
|
||
| if (isPendingGroups || !groups || (groups.length && !selectedGroup)) { | ||
| if (isErrorGroups && !groups) { | ||
| return <QueryError noun="groups" />; | ||
| } | ||
|
|
||
| if (isPendingGroups || (groups.length && !selectedGroup)) { | ||
| return <LoadingPlaceholder className="mt-32" />; |
There was a problem hiding this comment.
Handle selected-group query errors to avoid infinite loading.
When the selected group request fails with no cached data, Line 49 keeps the component in LoadingPlaceholder forever because there is no isError branch for useFetchGroup.
💡 Proposed fix
- const { data: selectedGroup } = useFetchGroup(selectedGroupId ?? 0);
+ const {
+ data: selectedGroup,
+ isPending: isPendingSelectedGroup,
+ isError: isErrorSelectedGroup,
+ } = useFetchGroup(selectedGroupId ?? 0);
if (isErrorGroups && !groups) {
return <QueryError noun="groups" />;
}
- if (isPendingGroups || (groups.length && !selectedGroup)) {
+ if (isErrorSelectedGroup && groups.length && !selectedGroup) {
+ return <QueryError noun="selected group" />;
+ }
+
+ if (isPendingGroups || (groups.length && isPendingSelectedGroup)) {
return <LoadingPlaceholder className="mt-32" />;
}| } = useFetchReference(refId); | ||
|
|
||
| if (isError) { | ||
| if ((isError || isErrorReference) && (!index || !reference)) { |
There was a problem hiding this comment.
Refine the NotFound condition to pair each query's error with its own data check.
The current condition (isError || isErrorReference) && (!index || !reference) can show NotFound when one query has cached data (from a previous successful fetch) but is in an error state due to a failed background refetch, while the other query's data is missing for an unrelated reason. This doesn't match the React Query v5 pattern where error and data can coexist.
The correct pattern pairs each query's error state with its own data: if the index query errors and has no data, OR if the reference query errors and has no data, then show NotFound.
🔧 Proposed fix
- if ((isError || isErrorReference) && (!index || !reference)) {
+ if ((isError && !index) || (isErrorReference && !reference)) {
return <NotFound />;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if ((isError || isErrorReference) && (!index || !reference)) { | |
| if ((isError && !index) || (isErrorReference && !reference)) { | |
| return <NotFound />; | |
| } |
| const { isPending: isPendingReference, isError: isErrorReference } = | ||
| useFetchReference(refId); | ||
| const { | ||
| data: otu, | ||
| isPending: isPendingOTU, | ||
| isError: isErrorOTU, | ||
| } = useFetchOTU(otuId); | ||
|
|
||
| if (isPendingReference || isPendingOTU || !otu) { | ||
| if ((isErrorReference || isErrorOTU) && !otu) { | ||
| return <QueryError noun="OTU" />; | ||
| } |
There was a problem hiding this comment.
Root cause: cross-query predicate coupling causes false error UI.
These branches combine one query’s isError with another query’s missing data, which can display <QueryError /> for mixed states (for example, one query in refetch-error with stale data while the other is just pending). Use independent checks per query: (isErrorX && !dataX) || (isErrorY && !dataY).
| if (isErrorAccount || isErrorSample) { | ||
| return { hasPermission: false, isPending: false }; | ||
| } | ||
|
|
||
| if (isPendingSample || isPendingAccount) { | ||
| return { hasPermission: false, isPending: true }; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "isErrorAccount|isErrorSample|hasPermission|isPendingSample|isPendingAccount" apps/web/src/samples/hooks.ts
sed -n '1,120p' apps/web/src/samples/hooks.tsRepository: virtool/virtool-ui
Length of output: 1606
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "export function useFetchAccount|useFetchAccount\\(" apps/web/src -S
rg -n "export function useFetchSample|useFetchSample\\(" apps/web/src -S
fd -t f "hooks.ts" apps/web/src/samples -aRepository: virtool/virtool-ui
Length of output: 1987
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "---- apps/web/src/account/queries.ts ----"
sed -n '1,200p' apps/web/src/account/queries.ts
echo "---- apps/web/src/samples/queries.ts ----"
sed -n '1,220p' apps/web/src/samples/queries.tsRepository: virtool/virtool-ui
Length of output: 10610
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "useFetchAccount\\(\\)" apps/web/src/account apps/web/src -S
rg -n "const \\{[^}]*isError[^}]*\\} = useFetchAccount" apps/web/src -S
rg -n "isError.*data" apps/web/src -S
rg -n "isRefetchError" apps/web/src -SRepository: virtool/virtool-ui
Length of output: 3182
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,120p' apps/web/src/users/components/UsersList.tsx
sed -n '1,120p' apps/web/src/account/components/ApiKeys.tsx
sed -n '1,120p' apps/web/src/account/components/AccountProfile.tsx
sed -n '1,120p' apps/web/src/subtraction/components/SubtractionList.tsxRepository: virtool/virtool-ui
Length of output: 6040
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "useCheckCanEditSample\\(" apps/web/src -S
rg -n "hasPermission" apps/web/src/samples -SRepository: virtool/virtool-ui
Length of output: 1049
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,120p' apps/web/src/routes/_authenticated/samples/\$sampleId.tsx
sed -n '1,120p' apps/web/src/analyses/components/AnalysisList.tsx
sed -n '1,120p' apps/web/src/samples/components/SamplesToolbar.tsx
sed -n '1,120p' apps/web/src/samples/components/Detail/SampleRights.tsxRepository: virtool/virtool-ui
Length of output: 9712
Fix permission gating to not deny on isError when cached data exists
apps/web/src/samples/hooks.ts:23-29 currently returns hasPermission: false when isErrorAccount || isErrorSample even if account/sample is still present. This codebase typically gates errors like this: if (isError && !data) { ... }, so permission checks should follow the same isError && !data pattern.
Suggested fix
- if (isErrorAccount || isErrorSample) {
+ if ((isErrorAccount && !account) || (isErrorSample && !sample)) {
return { hasPermission: false, isPending: false };
}
- if (isPendingSample || isPendingAccount) {
+ if (isPendingSample || isPendingAccount || !account || !sample) {
return { hasPermission: false, isPending: true };
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (isErrorAccount || isErrorSample) { | |
| return { hasPermission: false, isPending: false }; | |
| } | |
| if (isPendingSample || isPendingAccount) { | |
| return { hasPermission: false, isPending: true }; | |
| } | |
| if ((isErrorAccount && !account) || (isErrorSample && !sample)) { | |
| return { hasPermission: false, isPending: false }; | |
| } | |
| if (isPendingSample || isPendingAccount || !account || !sample) { | |
| return { hasPermission: false, isPending: true }; | |
| } |
| if ((isErrorAccount || isErrorFiles) && (!account || !files)) { | ||
| return <QueryError noun="files" />; | ||
| } |
There was a problem hiding this comment.
Decouple error guards per query to prevent false error UI.
The current combined predicate can return QueryError when only one query errored and the other is just pending/missing, which incorrectly short-circuits loading/success paths.
Suggested fix
-if ((isErrorAccount || isErrorFiles) && (!account || !files)) {
- return <QueryError noun="files" />;
-}
+if (isErrorAccount && !account) {
+ return <QueryError noun="account" />;
+}
+
+if (isErrorFiles && !files) {
+ return <QueryError noun="files" />;
+}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if ((isErrorAccount || isErrorFiles) && (!account || !files)) { | |
| return <QueryError noun="files" />; | |
| } | |
| if (isErrorAccount && !account) { | |
| return <QueryError noun="account" />; | |
| } | |
| if (isErrorFiles && !files) { | |
| return <QueryError noun="files" />; | |
| } |
1 participant
Summary
if (isPending || !data)anti-pattern in secondary (Tier 2) React Query consumers — lists, sidebar widgets, dialogs, banners, and the wrapper hooks that compose queries — so a failed request shows a contained inline error instead of spinning forever (VIR-2537).@base/QueryError(a redAlerttaking anoun, the Tier 2 counterpart toRouteError) and gate it onisError && !data— error only when there's nothing to show. In React Query v5erroranddatacoexist, so a bareif (isError)would blank an already-loaded view the moment a background refetch fails;&& !datafixes the initial-load spin while keeping stale data on screen through background errors.AnalysisDetail, which never checked its analysis query's error and would fall through toanalysis.ready.docs/queries.mdandAGENTS.mdto document theQueryErrorcomponent and theisError && !datarule.