fix: show aggregation errors in Chart.svelte instead of an infinite spinner

[eminemead]

• Failed aggregations left the chart on an infinite diamond spinner.
• Chart.svelte checked isFetching before the query error, so a failed request never left the loading state.
• Render the aggregation error first; keep the spinner only when there is no error.

Checklist:

• Covered by tests
• Ran it and it works as intended
• Reviewed the diff before requesting a review
• Checked for unhandled edge cases
• Linked the issues it closes
• Checked if the docs need to be updated. If so, create a separate Linear DOCS issue
• Intend to cherry-pick into the release branch
• I'm proud of this work!

---

Developed in collaboration with Claude Code

[nishantmonu51]

1. isFetching was not what swallowed the error; measures.length === 0 was, and that path still spins forever.

The query client caps retries (globalQueryClient.ts: retry only for network errors, max two), so once an aggregation fails, isFetching is false and the old {:else if error} branch was already reachable. The condition that actually keeps the chart on the spinner is measures.length === 0 in the same #if: getMeasuresForMetricView returns [] whenever the metrics view resource is missing or has no validSpec, and nothing ever changes that. The reorder does fix the reported bug, because a chart pointed at a missing or invalid metrics view both errors and has empty measures — but the residual case is worth handling: an empty measures with no error (metrics view absent, or still not in the resource list) is still an indefinite spinner. Consider distinguishing "measures not loaded yet" from "metrics view has no measures" rather than folding the latter into the loading branch.

2. The tests do not cover the scenario that produced the bug.

Every case passes measures = [measure], and the first case pairs isFetching: true with an error, a combination the retry configuration makes unlikely in practice. The regression this PR fixes is measures: [] plus an error, and that is the case worth pinning:

renderChart(chartData({ isFetching: false, error: new Error("...") }), []);

Also, expect(container.querySelector(".status")).toBeNull() passes vacuously if Spinner.svelte ever renames its class, so the absence assertions are weaker than they look.

3. The vitest-setup.ts localStorage mock appears unnecessary.

Chart.spec.ts passes without it on both Node 20 and Node 22 (npx -y node@22 ../node_modules/vitest/vitest.mjs run src/features/components/charts/Chart.spec.ts with the setup file reverted to main). The test.projects config declares only the jsdom environment, where jsdom supplies localStorage, so the guard's branch is not taken. If it is needed for a case I have not reproduced, the PR should say which one; otherwise dropping those 34 lines keeps an unrelated global change out of a targeted bug fix. If it stays, note that clearMocks: true does not reset the backing Map, so storage written by one test is visible to the next.

4. Minor: a stale error now suppresses the spinner on refetch.

With error checked first, a query that failed and is being refetched (a filter or time-range change after a failure) shows the previous error until the new response lands, where it used to show the spinner. That is a reasonable trade against an infinite spinner, but if the empty-measures case in point 1 is handled separately, isFetching && !error becomes unnecessary and the ordering question goes away.