Repository navigation
feat(ui): explain and recover from ChartArtifact states that cannot be drawn - #812
Merged
AntonTyb merged 3 commits intoSep 11, 2026
Merged
Conversation
There was a problem hiding this comment.
Code Review
This pull request enhances the chart artifact view by adding support for asynchronous provisioning states, error handling, and manual data refreshing. It introduces a notice container to explain why a chart cannot be drawn (such as when it is still provisioning or has failed), normalizes status representations from both string and numeric enum tags, and adds a retry mechanism on failure. The reviewer's feedback suggests making the error-catching logic more robust by handling cases where the caught error is a plain string rather than an object with a message property.
AntonTyb
force-pushed
the
feat/ui-chart-artifact-view-states
branch
from
September 8, 2026 08:35
9e6b16c to
6b7d418
Compare
AntonTyb
marked this pull request as ready for review
September 8, 2026 08:44
andrzejfudro
previously approved these changes
Sep 8, 2026
AntonTyb
dismissed
andrzejfudro’s stale review
September 8, 2026 15:59
The merge-base changed after approval.
AntonTyb
force-pushed
the
feat/ui-chart-artifact-view-states
branch
from
September 8, 2026 15:59
6b7d418 to
3526ae9
Compare
AntonTyb
force-pushed
the
feat/ui-chart-artifact-view-states
branch
from
September 9, 2026 06:59
3526ae9 to
c8ad690
Compare
andrzejfudro
previously approved these changes
Sep 9, 2026
AntonTyb
force-pushed
the
feat/ui-chart-artifact-view-states
branch
from
September 9, 2026 07:51
c8ad690 to
3677c83
Compare
There was a problem hiding this comment.
🤖 Incremental PR Approval
Clean rebase detected with zero semantic patch lines changed.
Preserving prior approval by @andrzejfudro on commit c8ad690.
…e drawn ChartArtifactView assumed a provisioned, renderable chart. An artifact still being provisioned, one whose provisioning failed, or one whose chart carries no plots all fell through to an empty panel with no indication of why -- and a failed GetChartData call left the same blank space behind. Each of those states now says what happened in place of the plots, carrying the server's error message through when there is one, and a refresh button retries a fetch that did not arrive. Two of them also stop a pointless round trip: an unprovisioned artifact has no chart name to ask about, and a chart with no plots has nothing to display the response. Statuses are normalised through chartStatusName so that a serializer emitting ChartArtifact.Status as its enum tag rather than its proto JSON name cannot slip a FAILED artifact past the notice and render it as healthy. TAG=agy CONV=96ac94bd-c2b9-4753-b54f-b0b6b02d801f
The spec threw an Error whose message was the component's own fallback string,
so `expect(error()).toBe('Failed to load chart metrics')` held whether the
message was read off the error or not -- replacing the whole expression with
the literal fallback still passed it.
Throw a distinct message and assert that, which fails against a component that
ignores the error. The fallback keeps its own spec, with an Error carrying no
message.
TAG=agy
CONV=96ac94bd-c2b9-4753-b54f-b0b6b02d801f
Review round 3 on #812. The hand-rolled error container is replaced by xmc-error-banner, which already carries the retry affordance and the styling the rest of the app uses for the same state. Its own refresh button goes with it, and the local .error-container rule is no longer needed. The two failure specs now assert through ErrorBannerHarness instead of reading the component's private signals, and a third covers the retry re-fetching. The banner only renders once loading has finished, so reaching it also covers the loading flag clearing that the old spec checked directly. TAG=agy CONV=96ac94bd-c2b9-4753-b54f-b0b6b02d801f
AntonTyb
force-pushed
the
feat/ui-chart-artifact-view-states
branch
from
September 9, 2026 14:05
3677c83 to
107a655
Compare
There was a problem hiding this comment.
🤖 Incremental PR Approval
Clean rebase detected with zero semantic patch lines changed.
Preserving prior approval by @andrzejfudro on commit c8ad690.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Stacked on #811 (base branch
feat/ui-chart-artifact-view). Slice 4 of 4 from the split of#710, requested in review.
Slice 3 assumed a provisioned, renderable chart. Everything else fell through to an empty
panel with no indication of why. A chart artifact is provisioned asynchronously, so it spends
its first moments naming metrics it cannot yet plot — the blank state is the one a user is
most likely to hit first.
Each unrenderable state now says what happened in place of the plots:
chart name to ask the Dashboard Service about; calling
GetChartDatawith an empty namewould only turn the wait into an error message.
error_message. This is the surface thatmade the unauthenticated-
CreateChartdefect fixed in fix(ess): authenticate and durably record ChartArtifact provisioning #809 visible at all.response and the data engine runs a query per plot behind it.
Statuses are normalised through
chartStatusNamebefore matching. A serializer that did notapply the proto JSON mapping emits
ChartArtifact.Statusas its enum tag, and matching onlythe names let a numeric
FAILEDslip past the notice and render as a healthy chart,swallowing the error message the payload was carrying.
isSummaryBoxPlotalready did the samefor
PlotSpec.type. An unrecognised tag is left unmatched rather than mapped to a wrongstatus.
Testing
Full Angular suite green at 585 specs — the pre-split baseline;
tsc --noEmitand prettierclean.
Seven specs added to
chart_artifact_view_test.ts, covering each notice, both statusspellings, the two suppressed requests, the failed fetch, and
chartStatusName's mappingincluding the unknown-tag case. The enum-tag
FAILEDspec was checked to fail againstname-only matching.
Notes for the reviewer
git diffbetween this branch andfeat(ui): render ChartArtifact plots full-width on the work unit page #710's head is empty. feat(ui): render ChartArtifact plots full-width on the work unit page #710 is closed in favour of the four slices.
Stack
feat/chart-artifact-example(#676)fix/ess-chart-provisioning-lifecycle(#809)feat/ess-chart-artifact-plot-specs(#810)feat/ui-chart-artifact-view(#811)feat/ui-chart-artifact-view-statesTAG=agy
CONV=96ac94bd-c2b9-4753-b54f-b0b6b02d801f