A failed instrument preview replaces the whole Manage Instruments page with the error page instead of showing the dialog's error message

When an instrument's bundle cannot be loaded, "Preview Form" in the admin instrument preview dialog replaces the whole Manage Instruments page with the error page. The dialog is meant to show an inline "Failed to load instrument preview." instead. InstrumentPreviewDialog renders that message when bundleQuery.isError. But useInstrumentBundle is a plain useQuery, and the app's QueryClient sets throwOnError: true for every query (src/services/react-query.ts; apps/web/AGENTS.md: "Errors reach the router error boundary, so do not write if (query.isError) UI"). On failure, the hook throws during render before the branch is reached, and the router's AppErrorComponent takes over the page. The unit test only passes because it mocks the hook to return { isError: true }, a state that cannot occur in the app. So an admin previewing a broken or unavailable instrument loses the page they were working on, when the dialog was built to contain the failure.

Where

apps/web/src/components/InstrumentPreviewDialog/InstrumentPreviewDialog.tsx:61 and :83-91:

const bundleQuery = useInstrumentBundle(showForm ? item.id : null);
// ...
{bundleQuery.isError && (
  <p className="text-destructive py-4 text-center text-sm">
    {t({
      en: 'Failed to load instrument preview.',
      // ...
    })}
  </p>
)}

apps/web/src/hooks/useInstrumentBundle.ts:9-25 (a useQuery with no throwOnError override) and apps/web/src/services/react-query.ts:13 (throwOnError: true).

Reproduce

As an admin, open Admin → Instruments and click an instrument to open its preview dialog.
Make GET /v1/instruments/bundle/<id> fail (for example, an instrument whose stored bundle fails $InstrumentBundleContainer parsing, or a 500), then click "Preview Form".

Actual: the dialog and the Manage Instruments page are replaced by the app's error page (or the "Connection Problem" screen for a network error).
Expected: the dialog stays open and shows "Failed to load instrument preview.", and the rest of the page is unaffected.

Tests

apps/web/src/components/InstrumentPreviewDialog/__tests__/InstrumentPreviewDialog.test.tsx: it('should show the preview error inside the dialog when the bundle request fails, so a broken instrument does not take down the page'). Use a real QueryClient with throwOnError: true and a rejecting axios mock, rather than mocking the hook's return value.
testing/src/specs/admin-instruments.spec.ts: route **/v1/instruments/bundle/** to a 500, click "Preview Form", and assert the inline message is visible and the instruments table is still on the page.

Suggested fix

Let this one caller opt out of throwing: add an options parameter to useInstrumentBundle (for example { throwOnError?: boolean }) and pass throwOnError: false from the dialog, so the existing isError branch actually renders. Keep the default for the render route, where the error boundary is the intended behavior.

Still reproduces on main (d86740e). No open PR touches the dialog, useInstrumentBundle or services/react-query.ts.

Manage Group loses unsaved edits

The same dialog renders on /group/manage (group/manage.tsx:593-595), inside ManageGroupForm. That form holds unsaved state: the instrument checkboxes (selectedIds, :558) and the settings Form (:671). When a preview fails there, the router error component unmounts the form and those edits are lost. This is the worst of the three call sites. The other two are admin/instruments.tsx:213 and :349.

A correction to the reproduce steps: a single click on an Admin → Instruments row does nothing (instruments.tsx:208-210). Open the dialog with the "Preview" row action. A double-click also opens it in the forms view, but in the series view a double-click archives or unarchives the row (:341-345).

Two things the suggested fix misses

Pair the opt-out with meta: { disableDefaultErrorNotification: true } on the GET, as useUpdateGroupMutation.ts:30 does. Without it, a 500 shows the inline message and also the "HTTP Request Failed" toast from services/axios.ts:197-205. The meta table in .agents/docs/playbooks/add-web-data-hook.md:44 already lists that flag for "The caller shows its own error message".
Update apps/web/AGENTS.md:77-78 in the same commit. It says "do not write if (query.isError) UI", and every throwOnError: false on main today is on a mutation, so this is the first query to opt out. Document the exception there and in step 10 of the playbook, which covers only mutations: a query whose failure the component shows inline sets both flags. The only fix that follows the rule as written is to delete the unreachable branch at InstrumentPreviewDialog.tsx:83-91, and that keeps the Manage Group data loss.

Shape: useInstrumentBundle(id, { throwOnError = true } = {}), passing throwOnError to useQuery and meta: { disableDefaultErrorNotification: !throwOnError } to axios.get, the same pattern as useUpdateGroupMutation. useInstrument and render/$id.tsx keep the default. No local error boundary is needed. Interpretation failures already render inline (ScalarInstrumentRenderer.tsx:96-114, SeriesInstrumentRenderer.tsx:134-147), so the query is what reaches the router boundary.

The global throwOnError: (error, query) => query.state.data === undefined proposed in #1535 would not fix this. The bundle has no data on its first fetch, so it would still throw. This issue needs the per-hook opt-out however #1535 is decided.

Tests

apps/web/src/hooks/__tests__/useInstrumentBundle.test.ts: build the client with the app defaults (retry: false, throwOnError: true; renderBundleQuery at :20 sets only retry) and reject mockAxios.get. Assert that { throwOnError: false } settles to isError without throwing and sends disableDefaultErrorNotification: true. The exact-arguments assertion at :37-39 fails once meta is added, so update it.
InstrumentPreviewDialog.test.tsx mocks the hook for the whole file (:18-20), so a real QueryClient does not fit there. Instead, change :95 and :209 (toHaveBeenLastCalledWith(null) and ('instrument-1')), which fail once a second argument is passed, to expect { throwOnError: false } as well. With the hook test, that covers the path the mocked isError test at :225-230 assumes.
testing/src/specs/group-manage.spec.ts, next to the preview test at :31-41: fill subjectIdDisplayLengthInput, route **/v1/instruments/bundle/** to a 500, open a preview with instrumentPreviewButton(title) and click "Preview Form". Assert that the inline message is visible and no "HTTP Request Failed" toast is, then close the dialog and assert that the input still holds its value. Keep the 500: services/axios.ts:36 retries 502, 503 and 504 for up to 15 seconds. The admin-instruments.spec.ts case in the issue is a fine second test.

_{Triage note written with Claude Code. Code references point at main @ d86740e.}

Navigation Menu

Repository navigation

A failed instrument preview replaces the whole Manage Instruments page with the error page instead of showing the dialog's error message #1687

Where

Reproduce

Tests

Suggested fix

Manage Group loses unsaved edits

Two things the suggested fix misses

Tests

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

A failed instrument preview replaces the whole Manage Instruments page with the error page instead of showing the dialog's error message #1687

Description

Where

Reproduce

Tests

Suggested fix

Activity

joshunrau commented on Oct 26, 2026

Manage Group loses unsaved edits

Two things the suggested fix misses

Tests

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions