Repository navigation
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces full-width rendering of chart artifacts on the work unit page. On the backend, the experiment state server now derives plot specifications (summary boxes and scatter plots) from metric names, limits the metrics to prevent expensive fan-out, and records the assigned chart back to the artifact. On the frontend, a new ChartArtifactView component is added to fetch and display these plots. Feedback on the changes highlights a critical issue in ChartArtifactView where using toSignal on an asynchronous observable chain without an initialValue will cause a runtime crash (NG0203) when initialized with an already-provisioned chart; passing { initialValue: undefined } is recommended to resolve this.
| // Record the chart the service assigned. It is the only link from the artifact to | ||
| // its plots: without the server-assigned resource name a reader holds a list of | ||
| // metric names and no way to find the chart that draws them. | ||
| cArtifact.Chart = created |
There was a problem hiding this comment.
[Blocker] Detach context before Spanner status update
ctxBg is scoped to a 2-minute timeout (ctxBg, cancel := context.WithTimeout(ctxBg, 2*time.Minute)). If CreateChart times out or is cancelled, ctxBg.Err() is non-nil (context.DeadlineExceeded / context.Canceled).
Calling o.storage.UpdateArtifact(ctxBg, ...) below with this expired context causes the Spanner transaction to fail immediately with deadline exceeded. The failure status (cArtifact.Status = FAILED) is never persisted to Spanner, stranding the artifact in PROVISIONING permanently in the database and causing the UI to display "The chart for this artifact is still being provisioned. Reload in a moment." indefinitely.
Suggested Fix:
storeCtx, storeCancel := context.WithTimeout(context.WithoutCancel(ctxBg), 15*time.Second)
defer storeCancel()
if _, err := o.storage.UpdateArtifact(storeCtx, &storage.ArtifactUpdate{
AID: aid,
Artifact: artifactToProvision,
UpdateMask: &fieldmaskpb.FieldMask{Paths: []string{"payload"}},
}); err != nil {
logging.Error(storeCtx, "Failed to update ChartArtifact status in Spanner", "error", err)
}There was a problem hiding this comment.
Agreed, and taken — thank you, this is the one that would actually have stranded users.
To be clear about scope for anyone reading: this is pre-existing rather than something this PR introduced (both the two-minute context.WithTimeout and the UpdateArtifact(ctxBg, ...) come from #441), but this PR is what makes the stuck state visible in the UI, so it belongs here.
Fixed as you suggested: context.WithoutCancel(ctxBg) with its own 15-second deadline. Test added that drives OnArtifactCreated with an already-finished context and asserts the write still arrives at storage with a live context and the FAILED status persists — it fails against the old code.
| type dashboardObserver struct { | ||
| client *toollibs.DashboardToolClient | ||
| storage storage.Storage | ||
| wg *sync.WaitGroup | ||
| } | ||
|
|
||
| // uniquePlotID slugifies a metric name into a plot ID, disambiguating names that slugify to | ||
| // the same string ("train/loss" and "train_loss") with a counter suffix. seen is mutated. | ||
| func uniquePlotID(metric string, seen map[string]int) string { |
There was a problem hiding this comment.
[Major] Slug collision on metric names with pre-existing numeric suffixes
uniquePlotID tracks counts of base slugs via seen[slug]++. If a work unit logs metric names that already carry numeric suffixes (e.g. ["foo", "foo_2", "foo"]):
- Metric 1 (
"foo"):seen["foo"] = 1-> returns"foo" - Metric 2 (
"foo_2"):seen["foo_2"] = 1-> returns"foo_2" - Metric 3 (
"foo"):seen["foo"] = 2-> returns"foo_2"(Collision!)
Both Metric 2 and 3 receive PlotId: "foo_2_latest". In the Angular UI, plotDataMap keys by plotId, so one metric overwrites the other, and @for track plot.plotId triggers duplicate key errors.
Suggested Fix:
Use an existence set and loop until an unused slug candidate is found:
func uniquePlotID(metric string, seen map[string]bool) string {
slug := strings.Trim(nonPlotIDChars.ReplaceAllString(strings.ToLower(metric), "_"), "_")
if slug == "" {
slug = "metric"
}
candidate := slug
for i := 2; seen[candidate]; i++ {
candidate = fmt.Sprintf("%s_%d", slug, i)
}
seen[candidate] = true
return candidate
}There was a problem hiding this comment.
Good catch, and it was worse than a cosmetic duplicate — fixed.
You are right that counting occurrences of a slug is the bug: ["foo", "foo_2", "foo"] hands out foo_2 twice, because the generated suffix collides with a metric that already ends in a number. The consequence is not just a duplicate ID: the UI keys plot data by plot ID (plotDataMap in chart_artifact_view.ts), so both plots would have drawn the same series.
uniquePlotID now takes a map[string]bool of IDs already handed out and increments past any that are taken, as you suggest. Regression test added covering exactly your ["foo", "foo_2", "foo"] case, asserting both the expected IDs and that no ID repeats.
| // Call Dashboard service. | ||
| _, err := o.client.CreateChart(ctxBg, &dashboard_pb.CreateChartRequest{ | ||
| created, err := o.client.CreateChart(ctxAuth, &dashboard_pb.CreateChartRequest{ |
There was a problem hiding this comment.
[Major] Pass deterministic ChartId for idempotency
CreateChartRequest omits ChartId. When omitted, Dashboard Service generates a random UUID (uuid.New().String()). If CreateChart succeeds but the subsequent Spanner write fails or the observer is retried, the initial chart is permanently orphaned in the Dashboard Service database without an artifact linking to it.
Suggested Fix:
Derive ChartId deterministically from the artifact ID (per AIP-133):
created, err := o.client.CreateChart(ctxAuth, &dashboard_pb.CreateChartRequest{
Parent: fmt.Sprintf("experiments/%d/workUnits/%d", xid, wid),
ChartId: fmt.Sprintf("art-%d", aid),
Chart: &dashboard_apipb.Chart{ ... },
})There was a problem hiding this comment.
Deferring this one, but not dismissing it — a deterministic ID is the right shape and AIP-133 is the right reference.
Two reasons it is not in this PR. First, it is pre-existing: origin/main omits ChartId on this same call, so it is not something introduced here.
Second, and more to the point, I do not think it fixes the orphan on its own. There is no retry path in this goroutine — if the Spanner write fails we log and return, and nothing comes back to try again. A deterministic ID makes a future retry idempotent, which is genuinely valuable, but the chart is equally orphaned either way until that retry exists. So the fix for the orphaning is a retry, and the deterministic ID is what makes that retry safe to write.
The related failure — the status write being refused because it inherited the CreateChart deadline — is fixed in this PR (thread above), and that was the one actually stranding artifacts in PROVISIONING.
Happy to do the deterministic ID plus a retry together as a follow-up. One thing to settle there: art-%d needs checking against whatever the Dashboard Service validates chart IDs with, since the server currently mints UUIDs for this field.
| * A ChartArtifact is provisioned asynchronously, so it spends its first moments naming | ||
| * metrics it cannot yet plot. Saying which of those states it is in beats a blank panel. | ||
| */ | ||
| protected readonly notice = computed<string | null>(() => { |
There was a problem hiding this comment.
[Major] Support proto numeric enum tags for status
While isSummaryBoxPlot accounts for numeric tags ((plot.type as unknown) === 1), notice only checks string literals:
if (payload.status === "FAILED") { ... }
if (!this.chartName()) {
return payload.status === "SUCCESS" ? ... : "... still being provisioned ...";
}When deserialized as numeric enum values (FAILED = 3), payload.status === "FAILED" evaluates to false. It falls through to !this.chartName() where status === "SUCCESS" is also false, resulting in "The chart for this artifact is still being provisioned. Reload in a moment.", completely masking the server error message.
Suggested Fix:
const isFailed = payload.status === "FAILED" || (payload.status as unknown) === 3;
const isSuccess = payload.status === "SUCCESS" || (payload.status as unknown) === 2;
const isProvisioning = payload.status === "PROVISIONING" || (payload.status as unknown) === 1;
if (isFailed) {
return payload.errorMessage || "The server could not provision a chart for this artifact.";
}
if (isProvisioning) {
return "The chart for this artifact is still being provisioned. Reload in a moment.";
}
if (!this.chartName()) {
return "This artifact has no provisioned chart to draw.";
}
if (!this.plots().length) {
return "The provisioned chart declares no plots, so there is nothing to draw.";
}There was a problem hiding this comment.
Agreed and fixed — this was the worst of the four UI ones, because the failure mode is silent.
Added chartStatusName, which normalises ChartArtifact.Status from either spelling to its proto JSON name, mirroring what isSummaryBoxPlot already does for PlotSpec.type two functions up. notice() now compares against the normalised value.
Two tests: one asserting the notice surfaces errorMessage when the status arrives as the tag 3, and a unit test over the mapping including an unknown tag (which stays undefined rather than being coerced to a wrong status). The first fails against the old code with the notice reading "still being provisioned" while the payload carried "dashboard service unavailable" — which is exactly the swallowed error you described.
| private readonly dashboardService = inject(DashboardService); | ||
| private readonly refreshTrigger$ = new BehaviorSubject<void>(undefined); | ||
|
|
||
| protected readonly isLoading = signal<boolean>(false); |
There was a problem hiding this comment.
[Major] Prevent initial render double-mounting & canvas flicker
toObservable(this.chartName) emits asynchronously in a microtask, while isLoading starts initialized to signal(false).
On initial mount:
- The template evaluates
@elseand mounts all<xmc-plot-card>components withplotData = undefined. - In the microtask,
toObservablefires, settingisLoading = true. Angular tears down the plot cards and mounts<mat-spinner>. - When data arrives,
isLoading = false, tearing down the spinner and mounting the plot cards a second time.
Suggested Fix:
Initialize isLoading to true when a chart name is present and has plots:
protected readonly isLoading = signal<boolean>(
!!this.artifact().payload.chart?.name && (this.artifact().payload.chart?.plots?.length ?? 0) > 0,
);There was a problem hiding this comment.
I tried to take this one and then backed it out, because I could not demonstrate the problem — flagging that rather than shipping the change on faith.
I did build it: a showSpinner computed that also went true whenever the data on hand was not the data for the current chart, so it did not depend on isLoading having been set yet. Then I instrumented the component to print both signals around change detection, and got:
PROBE before CD: isLoading=false showSpinner=false
PROBE after CD: isLoading=true showSpinner=true
toObservable's effect flushes within the same change-detection pass, so by the end of the first pass isLoading is already true and the two signals never diverge. I also checked the DOM directly: with a never-emitting source, the first render shows the spinner and zero plot cards — against the unmodified code. So there is no frame in which plot cards mount over empty data, at least not one I can produce.
That left me adding a second piece of state and a second computed to fix something I could not reproduce, so I
10000
reverted it. If you have seen the flicker in a real browser I will happily dig again — a zoneless or OnPush-with-manual-CD path might well schedule differently from TestBed, and that would be the interesting difference. But I did not want to claim a fix I could not back up.
The other five findings on this review are all fixed.
| // GetChartData with an empty name would only turn the wait into an error message. | ||
| if (!name) { | ||
| this.isLoading.set(false); | ||
| return of(undefined); |
There was a problem hiding this comment.
[Minor] Skip network RPC when chart has no plots
When an artifact has a chart name but chart.plots is empty (plots: []), notice() computes "The provisioned chart declares no plots...", but switchMap still executes getChartData({ name }) over HTTP.
Suggested Fix:
switchMap(([name]) => {
if (!name || this.plots().length === 0) {
this.isLoading.set(false);
return of(undefined);
}
return this.dashboardService.getChartData({ name }).pipe(...);
})There was a problem hiding this comment.
Taken — fixed. Added a fetchTarget computed that yields '' when the chart declares no plots, so the pipeline never reaches getChartData. Worth more than the one round trip, since the data engine runs a query per plot behind it.
Test added that swaps in a plotless chart under a different resource name, so the absent plots are the only thing standing between it and a request — with the name held constant the computed never re-emits and the assertion passes vacuously either way, which caught me out on the first attempt.
| } | ||
|
|
||
| .chart-artifact-title { | ||
| margin: 0; |
There was a problem hiding this comment.
[Minor] Add white-space: nowrap for ellipsis truncation
.chart-artifact-title specifies overflow: hidden; text-overflow: ellipsis; without white-space: nowrap;. Under CSS specifications, text will wrap to multiple lines rather than truncating with an ellipsis.
Suggested Fix:
.chart-artifact-title {
white-space: nowrap;
overflow: hidden;
text-overflow: ellipsis;
}There was a problem hiding this comment.
Correct — fixed, white-space: nowrap added. Without it the other two properties were dead: the heading wrapped instead, so the ellipsis never appeared.
48931ca to
817a30d
Compare
A ChartArtifact showed up on the Artifacts tab as a narrow card listing its
metric names and a JSON dump of its chart field. Nothing was plotted, for two
reasons that had to be fixed on both sides of the wire.
Experiment State Server:
- `dashboardObserver` provisioned the Chart with only a title, an xid and a
wid. `FetchChartData` iterates `chart.GetPlots()`, so a chart with no plots
returns no data however much the work unit logged. It now derives plot specs
from the artifact's `metric_names`: per metric, a SUMMARY_BOX for the latest
value and a step-indexed SCATTER curve, laid out on a two-column grid. The
number of metrics expanded is capped, because the list is client-supplied
and the data engine runs a query per plot.
- The observer discarded the `CreateChart` response, so the artifact never
recorded which chart was provisioned for it. It now stores the created Chart
on the artifact payload, which is what gives a reader the server-assigned
resource name to fetch data by.
UI:
- New `xmc-chart-artifact-view`: a full-width band above the three-column
pipeline on the work unit Artifacts tab, one per chart artifact, drawing the
plot specs the artifact now carries with data from `GetChartData`. The
narrow card stays in its pipeline column as the index entry. When there is
nothing to draw the band says which state the artifact is in -- still
provisioning, provisioning failed, or provisioned with no plots -- rather
than showing an empty panel.
- The band is rendered on work unit pages only. An experiment lists the
artifacts of all its work units, and each band costs one GetChartData.
- `ChartArtifactPayload.chart` is typed as `Chart` instead of
`Record<string, unknown>`.
Verification: `ng test` passes (558 specs, 9 of them new), as do tsc, ESLint and
Prettier. The Go change is covered by new unit tests in
`dashboard_observer_test.go` but could not be compiled or run here -- this
checkout's Go import paths are Bazel-mapped and Bazel is not available -- so
only gofmt and golangci-lint were run against it.
TAG=agy
CONV=96ac94bd-c2b9-4753-b54f-b0b6b02d801f
Background chart provisioning has never been authenticated. The Dashboard Service connection is dialed insecure with no per-RPC credentials, and grpc-go does not carry incoming metadata onto outgoing calls, so the CreateChart the observer issues arrives with no token. The service rejects it with Unauthenticated, the observer stores that in the artifact's error_message, and every ChartArtifact ends up FAILED with no chart. An Experiment State Server identity would not fix it either: CreateChart authorizes the *user's* WRITER role on the experiment, so the user's own token is the one that has to make the trip. Copy the credential headers off the incoming context onto the outgoing one, and warn when there are none to copy rather than failing the artifact -- whether an anonymous call is acceptable is the Dashboard Service's decision to make. This was invisible until the full-width chart view began rendering error_message; the narrow artifact card never showed it. TAG=agy CONV=96ac94bd-c2b9-4753-b54f-b0b6b02d801f
Six review findings on #710, four taken and two answered on the threads. ESS: - uniquePlotID counted occurrences of a slug, so a metric name that already ends in a number could collide with a generated suffix: ["foo", "foo_2", "foo"] handed out foo_2 twice. Plot IDs are the key the UI looks plot data up by, so both plots would have drawn the same series. It now skips IDs already claimed. - The status write reused the context governing CreateChart. If CreateChart is what spent those two minutes, the write failed before reaching Spanner, so FAILED never persisted and the artifact sat in PROVISIONING for good -- the UI reporting a chart still being provisioned by a goroutine that had long exited. It now writes under context.WithoutCancel with its own short deadline. UI: - notice() matched ChartArtifact.Status against its proto JSON names only. A serializer that emits the enum tag instead sent FAILED through as a healthy chart, swallowing the error message the payload carried. chartStatusName normalises both spellings, the way isSummaryBoxPlot already did for PlotSpec.type. - A chart declaring no plots no longer triggers GetChartData: nothing would display the response, and the data engine runs a query per plot behind it. - .chart-artifact-title had text-overflow: ellipsis with no white-space: nowrap, so it never ellipsised. Not taken, with reasoning left on the threads: the suggested initialValue for toSignal (it defaults to undefined without one -- only requireSync throws), and initialising isLoading from the payload to avoid a first-render double mount (toObservable's effect flushes within the same change-detection pass, so isLoading is already true on the first painted frame; a probe found no frame where the two differ). TAG=agy CONV=96ac94bd-c2b9-4753-b54f-b0b6b02d801f
9ba7627 to
a794a5b
Compare
There was a problem hiding this comment.
Please split this PR into multiple ones - it goes over the agreed upon limits.
Size. Over the 400-line threshold. Splitting the ESS fixes from the UI component was
considered, but the UI half alone renders nothing (unauthenticated call, no chart on the artifact, no plots on the chart) and the ESS half alone is unobservable, so neither half is independently reviewable or testable. Happy to split if you would rather.
If that's a concern - consider first returning mocked/fake data on the backend and building the UI based on that data and later properly generating the backend data.
|
@janzak-dev — split done, closing this in favour of four stacked PRs.
Each is under the 400/1000 thresholds on its own base ( Nothing under review changed. The cut is by functional depth rather than by layer, but I want to flag the one place it is
The three unresolved items from round 2 (deterministic #713 has been re-parented onto #676, since its one-rule TAG=agy |
What
Stacked on #676 (base branch
feat/chart-artifact-example).The chart artifact example from #676 works end to end, but the chart it produces is never
drawn. On the work unit Artifacts tab a
ChartArtifactrenders as a narrow card in oneof the three pipeline columns, listing its metric names and dumping its
chartfield asJSON. This PR makes it plot.
Three defects had to be fixed, two on the server side and one on the client.
Experiment State Server (
dashboard_observer.go)1. The provisioned chart had no plots.
OnArtifactCreatedcalledCreateChartwith onlyTitle,XidandWid.dataengine.FetchChartDataiterateschart.GetPlots(), so a chartwith no plots yields no data no matter how many metrics the work unit logged — the chart is
not empty-looking, it is unqueryable.
The observer now derives plot specs from the artifact's
metric_names: per metric, aSUMMARY_BOXfor the latest value and a step-indexedSCATTERcurve, on a two-columnGRIDlayout. Metric names are path-like (
chart_demo/loss), so they are slugified into plot IDs,with a counter suffix disambiguating names that slugify alike (
train/lossvstrain_loss).The number of metrics expanded is capped at 20 — the list is client-supplied and the data
engine runs one query per plot, so an unbounded list turns a single
GetChartDatainto aproportionally expensive fan-out.
2. The artifact never recorded which chart was provisioned for it. The
CreateChartresponse was discarded, so
ChartArtifact.chartstayed empty and a reader was left holding alist of metric names with no way to find the chart that draws them. The observer now stores
the created
Chart(which carries the server-assigned resource name) on the artifact payloadbefore the Spanner write.
The alternative considered and rejected was a UI-side
ListChartson the work unit parent —that is a by-parent heuristic, whereas this gives artifact → chart identity, and it is what
the
OUTPUT_ONLY chartfield in the proto was there for.3. The
CreateChartcall was never authenticated. Found during manual testing of thisPR: every chart artifact came back
FAILEDwithThe Dashboard Service connection is dialed insecure with no per-RPC credentials
(
prod.go:initDashboardClient), and grpc-go does not carry incoming metadata onto outgoingcalls, so the observer's
CreateChartarrived with no token at all andcreatechart.gorejected it. The observer stored that inerror_messageand marked theartifact
FAILED.An Experiment State Server identity would not have fixed it:
CreateChartcallsauthorizeExperimentWrite(ctx, user, xid), authorizing the user's WRITER role on theexperiment, so the user's own token is the one that has to make the trip.
withForwardedAuthcopies
x-goog-iap-jwt-assertion/authorizationoff the incoming context onto the outgoingone. When there is nothing to copy it logs a warning and calls anyway — whether an anonymous
call is acceptable is the Dashboard Service's decision, not the observer's.
This defect predates the PR; it was simply invisible, because the narrow artifact card never
rendered
error_message. The full-width view below does.UI
New standalone component
xmc-chart-artifact-viewundercmd/ui/app/common/components/artifacts_list/chart_artifact_view/:band per chart artifact. The narrow card stays in its pipeline column as the index entry.
:hostdeliberately has nomax-width, unlikechart_card.scss— the whole point is touse the width the pipeline column cannot give it. Summary boxes go side by side across the
top; curves stack, so each gets the full width for its x axis.
the series data from
DashboardService.getChartData, reusingxmc-plot-cardand thePlotDataPipealready exported bychart_card.ts.provisioning failed (surfacing the server's
error_message), or provisioned with no plots —rather than showing an empty panel. A chart artifact is provisioned asynchronously, so it
spends its first moments naming metrics it cannot yet plot.
artifacts_list.tsgates onparseWorkUnitName). Anexperiment page lists the artifacts of every one of its work units and each band costs one
GetChartData, so drawing them all there would fan out to a request per work unit on a pagethat never asked for plots.
ChartArtifactPayload.chartis retyped fromRecord<string, unknown>toChart.Verification
Run locally, all green:
ng test --watch=false— 558 specs pass, 9 of them new(
chart_artifact_view_test.ts).tsc -p tsconfig.json --noEmit— clean.eslintandprettier --checkon the changed files — clean../.githooks/pre-commit --check— buildifier, gofmt, golangci-lint, addlicense and ruff allpass. It still exits non-zero, but only because mypy/terraform/tflint/trivy are absent from
this machine and the hook counts a skipped tool as a failure.
Unauthenticateddefect surfaced.Not verified locally: the Go change could not be compiled or run. This checkout's Go
import paths are Bazel-mapped (
xmanager_cloud/dashboard_service/proto/api_go_protoandfriends have no on-disk Go package) and Bazel is not installed here, so
go build/go testfail on every import. The new tests in
dashboard_observer_test.gocover the derived plotIDs, types, metrics and layout; that the artifact records the created chart; the empty
metric_namescase; blank-name skipping; slug collisions; the placeholder slug for a namewith no usable characters; the cap; credential forwarding for both header names; that
non-credential headers are not forwarded; and that a call with no credentials is still issued.
CI is their first real run.
Also not verified: the rendered appearance. The band's look has not been seen in a browser.
Notes for the reviewer
considered, but the UI half alone renders nothing (unauthenticated call, no chart on the
artifact, no plots on the chart) and the ESS half alone is unobservable, so neither half is
independently reviewable or testable. Happy to split if you would rather.
isSummaryBoxPlotduplicates the plot-type filtering inchart_card.ts. Both accept theproto JSON name and the integer tag. Left duplicated rather than refactoring
chart_card.tsin a PR that is already over the size threshold; worth folding into a shared helper later.
PerRPCCredentialsor interceptor patternexists anywhere in this repo to reuse, and only one outbound call needs it. If more
server-to-server hops appear this belongs in a client interceptor instead.
artifact_card.ng.htmlstill shows the raw chartJSON in a collapsible panel. Now that the payload actually carries a chart, that panel has
real content in it; it seemed better to leave the raw-data view alone than to change two
surfaces at once.
artifacts_list. That component loadsartifacts_list.css, which is alicense-header-only stub, while
artifacts_list.scssis the internal source — so stylesadded there would not apply in the OSS build. The new component carries its own spacing
instead.
Screenshot
Review round 2
Six findings from review, four taken and two answered on the threads.
UpdateArtifactreuses the context that boundedCreateChartCreateChartspent the two minutes, the write failed before reaching Spanner, so FAILED never persisted and the artifact sat in PROVISIONING for good. Now writes undercontext.WithoutCancelwith its own 15s deadline. Pre-existing (from #441), taken here because this PR is what makes the stuck state visible.uniquePlotIDcollides on names ending in a number["foo", "foo_2", "foo"]handed outfoo_2twice; the UI keys plot data by ID, so both plots drew the same series. Now skips IDs already claimed.notice()matches only the status' JSON nameschartStatusNamenormalises both spellings, asisSummaryBoxPlotalready did forPlotSpec.type.getChartDatacalled for a chart with no plots.chart-artifact-titleellipsis never fireswhite-space: nowrapadded; without it the other two properties were dead.ChartIdonCreateCharttoSignalneedsinitialValueor it throwsNG0203requireSync: truethrows, and withNG0601; without ittoSignalreturnsSignal<T | undefined>. The existingchartData()?.plotsDatatyping already relies on that.isLoadingfrom the payload to avoid a first-render double mounttoObservable's effect flushes within the same change-detection pass, soisLoadingis already true by the end of the first pass and the two signals never diverge. Checked the DOM directly too: with a never-emitting source the first render shows the spinner and zero plot cards, against the unmodified code. Reverted rather than ship state I could not justify. Details on the thread.Three tests added, each checked to fail against the old code: the
["foo", "foo_2", "foo"]collision, the status write surviving a finished context, and the notice surfacingerrorMessagewhen the status arrives as the tag3. Plus a unit test over the status mapping. Full Angular suite green at 561 specs.TAG=agy
CONV=96ac94bd-c2b9-4753-b54f-b0b6b02d801f