DM-55470: Add execution_error to Times Square client schemas

[jonathansick]

Summary

• Adopt the additive Times Square execution_error contract (DM-55470) in @lsst-sqre/times-square-client: a new ExecutionErrorSchema (code, title, message) plus the field on HtmlStatusSchema and HtmlEventSchema.

• Stay backward-compatible with pre-DM-55470 Times Square deployments — execution_error is optional-nullable and defaults to null, so payloads that omit the key entirely still parse and normalize to null. code is parsed as a plain string rather than an enum for forward compatibility, with the build-time-known values (timeout, jupyter_error, unknown, result_unavailable) exported as the EXECUTION_ERROR_CODES const.

• Add failed-state mock fixtures (mockExecutionError, mockHtmlStatusFailed, mockHtmlEventFailed) for the later slices that build the terminal UI on top.

• Treat a reported execution_error as terminal in both data paths. The html-status query option builders derive refetchInterval from the cached status and stop polling once the error is non-null (useHtmlStatus surfaces it as a new executionError field), and subscribeToHtmlEvents now auto-aborts on an event carrying a non-null execution_error as well as on a successful complete event — a failed run reports complete with no html_hash, so the old success-only condition left the SSE stream open forever. The terminal event still reaches onEvent before onComplete fires, and autoAbortOnComplete: false disables both conditions.

• Give subscribeToHtmlEvents optional bounded-reconnect options (maxReconnectAttempts, reconnectBackoffMs): once the configured number of consecutive connection failures is reached the stream is aborted and a terminal SseConnectionFailedError is reported through onError, so a consumer can drive a connection-failed UI state and a once-only Sentry capture. Both options default to unset, leaving today's unbounded retry behavior unchanged.

• Add the way out of that terminal state: deletePageHtml / deleteHtmlByUrl issue Times Square's DELETE /v1/pages/{page}/html soft delete, and the package's first react-query mutation (rerunPageMutationOptions, plus the useRerunPage hook) wraps them. On success it invalidates the ['times-square', 'html-status'] key prefix — covering both the htmlStatusForPage and htmlStatusByUrl key shapes — so the cached execution_error is dropped and polling resumes at its normal cadence.

• Move Squareone's TimesSquareHtmlEventsProvider onto that package transport: the client provider drops its inline HtmlEvent type and direct fetchEventSource usage in favor of subscribeToHtmlEvents + createHtmlEventsUrl with the bounded-reconnect options (5 attempts, 1 s linear backoff), so SSE events are schema-validated in one place. The connectionFailed alert banner and the once-per-subscription Sentry capture (tagged site: times-square-sse) behave as before, now driven by the package's terminal SseConnectionFailedError, and the events context gains an executionError field for the terminal-UI slices to consume. @microsoft/fetch-event-source is no longer an app dependency.

• Unify both re-run paths on that one mutation: the page panel's Recompute action drops its raw DELETE fetch for useRerunPage's rerunPageAsync, so it too sends a credentialed request and invalidates the html-status cache. The failure alert and its Sentry capture (tagged site: times-square-recompute) are unchanged — the mutation is deliberately Sentry-agnostic, so the app reports from the call site — and the button is now disabled while a request is in flight. No raw DELETE calls remain in the app.

• Reach the terminal UI state the contract was added for: when useHtmlStatus reports a non-null execution_error, TimesSquareNotebookViewerClient (which serves both the notebook viewer and the GitHub page-panel paths) stops showing an endless "Loading…" and renders a new NotebookExecutionError panel carrying the API's own title and message. Squareone authors no per-code copy of its own — code only selects the panel's icon and accent tone, and a code from a newer Times Square falls back to a generic treatment. The panel's Re-run notebook button soft-deletes the page instance through useRerunPage, targeting the page's html_url with this instance's notebook parameters and display settings; the mutation's invalidation drops the cached failure, so the viewer returns to its loading state, polling resumes, and the next successful render displays the HTML. A failed re-run surfaces an inline message and is captured in Sentry (tagged site: times-square-rerun).

• Make that whole flow exercisable in dev mode without a live Times Square: the mock htmlstatus and htmlevents routes gain a failure case on the existing ?a= magic-parameter convention (a=3 → available: false, html_hash: null, and a realistic timeout execution_error; a=1 and a=2 keep their current behavior), and the mock html route answers DELETE with the spec's DeleteHtmlResponse instead of a 405. A mocked re-run is stateful — it clears the instance's cached outcome for 15 s — so the viewer really does drop back to its loading state and resume polling before the failure returns. The dev page-metadata mocks (/v1/pages/:page, /v1/github/:path, PR preview) had drifted from the Page schema the client parses (string description; missing date_added, uploader_username, html_events_url, github), which left the dev notebook viewer stuck on "Loading…" regardless of ?a=; they now share one schema-valid builder.

• Re-vendor packages/times-square-client/openapi.json at the Times Square 0.25.0 release, the first tagged release carrying the execution_error contract, so the vendored spec documents the API surface these schemas model. The only API-surface change is on htmlstatus: HtmlStatus gains an optional-nullable execution_error backed by the new HtmlExecutionError (code, title, message) and NotebookExecutionErrorCode (timeout, jupyter_error, unknown, result_unavailable) schemas — matching ExecutionErrorSchema and EXECUTION_ERROR_CODES exactly, so no client code changes. The SSE html/events payload is not schema-modeled upstream (the endpoint declares an untyped text/event-stream response), so HtmlEventSchema's execution_error stays client-side only.

• Stop the page panel from reporting a failed run as a success. Times Square reports a failure as execution_status: 'complete' with a non-null execution_error, so ExecStats kept reading "Computed … in N seconds." in the sidebar while the viewer showed the failure panel. It now reads the events context's executionError and, when it is non-null, renders a failure summary carrying the API's own title (the same verbatim-copy policy as the viewer panel — no Squareone-authored per-code wording), followed by when the run finished and no duration phrasing. The check precedes the completed branch and its dateFinished guard, so a failed run that settles without a finish time still gets a summary, just without the timestamp line. The Recompute button stays — it is the recovery path — and with executionError: null the summary is byte-identical to before.

• Make that error text actually render red. Every source usage of --rsd-color-red-900 — the new ExecStats failure/error summaries, the SSE connection alert, the token detail page's delete-error and error panels, and squared's TextInput / TextArea / Select error variants — pointed at a custom property rubin-style-dictionary never defines (its red scale ends at 800), so each color declaration was dropped and the text silently inherited the surrounding body color. All nine now use --rsd-color-red-600 ("Dark red for solid backgrounds and text"), which pairs with the red-500 borders and red-100 backgrounds those rules already carry and clears WCAG AA on both white (7.17:1) and red-100 (6.16:1). Adopting the documented token is the fix; adding a red-900 shade is left as a future design-system decision.

Subsequent PRD #620 tasks append to this PR.

Validation steps

• In a consumer, import HtmlStatusSchema and parse a payload with execution_error present, one with it explicitly null, and one with the key absent — confirm all three parse and the absent case yields execution_error: null.
• Repeat the same three cases against HtmlEventSchema.
• Parse an execution_error carrying a code value not in EXECUTION_ERROR_CODES (e.g. some_future_code) and confirm it is accepted rather than rejected.
• Import mockHtmlStatusFailed / mockHtmlEventFailed from the package root and confirm they reflect the server contract (available: false, html_hash: null).
• With a Times Square page whose htmlstatus response carries an execution_error, watch the network panel and confirm the htmlstatus request stops repeating once the error arrives.
• With a page still executing (and again once it renders successfully), confirm htmlstatus keeps polling about once per second, unchanged.
• With a page whose notebook execution fails, watch the network panel and confirm the html/events SSE connection closes once the failure event arrives instead of staying open, and that the failing event still reached the UI before teardown.
• With a page that renders successfully, confirm the SSE connection still closes on completion exactly as before.
• Subscribe with { autoAbortOnComplete: false } and confirm the stream stays open after both a failed and a successful terminal event.
• Call subscribeToHtmlEvents against an unreachable events URL with { maxReconnectAttempts: 3, reconnectBackoffMs: 1000 } and confirm in devtools that the browser stops retrying after the third failure, with roughly 1 s / 2 s gaps between attempts, and that onError receives a final SseConnectionFailedError.
• Call it against the same unreachable URL with no reconnect options and confirm retries continue indefinitely at the transport's own interval, exactly as before.
• From a consumer, call useRerunPage()'s rerunPage({ pageName, params }) on a parameterized page and confirm in the network panel that a single DELETE /times-square/api/v1/pages/<page>/html?<params> goes out carrying that instance's parameters.
• Call rerunPage({ htmlUrl }) with a fully-formed html_url (the shape ExecStats holds) and confirm the same DELETE is issued against that URL.
• On a page sitting in the terminal error state, trigger a re-run and confirm the htmlstatus request starts repeating again — the invalidated query refetches, the fresh response carries execution_error: null, and the 1 s cadence resumes.
• Trigger a re-run against a page name that does not exist and confirm the hook reports isError with a TimesSquareError rather than silently doing nothing.
• Open a Times Square page with notebook parameters in the URL and confirm the html/events request still carries the full query string (both ts_-prefixed display settings and notebook parameters), and that the exec-stats sidebar fills in as events arrive.
• Stop the Times Square backend while a page is open and confirm the viewer retries a bounded number of times (~1 s, 2 s, 3 s, 4 s gaps), then shows the "Lost the connection…" banner and stops retrying, with a single Sentry event tagged site: times-square-sse.
• On a successfully rendered page, click Recompute in the sidebar and confirm exactly one DELETE goes out against the instance's html_url, that it carries credentials, that the button is disabled while it is in flight, and that the notebook re-executes.
• Make that DELETE fail (stop the backend or return a 500) and confirm the "Failed to request a recompute" alert appears and a single Sentry event tagged site: times-square-recompute is captured; then restore the backend, click Recompute again, and confirm the alert clears on the successful retry.
• Open a Times Square page whose notebook fails to execute and confirm the viewer shows the error panel with the API's own title and message instead of a perpetual "Loading…", and that the htmlstatus request stops repeating in the network panel.
• Click Re-run notebook on that panel and confirm a single DELETE .../html?<params> goes out carrying this page instance's notebook parameters and display settings, the viewer drops back to the loading state, htmlstatus polling resumes, and the notebook renders once execution succeeds.
• Make that re-run request fail (stop the backend or return a 500) and confirm the "Failed to request a re-run" message appears beside the button and a single Sentry event tagged site: times-square-rerun is captured.
• Against a Times Square deployment predating DM-55470 (responses with no execution_error key), open a page that is still executing and one that renders successfully, and confirm the viewer behaves exactly as it does today.
• Run pnpm dev --filter squareone, open /times-square/github/lsst-sqre/times-square-demo/demo?a=3, and confirm the execution-error panel renders with the timeout title and message (the page metadata and notebook tree load normally).
• Click Re-run notebook on that page and confirm a DELETE .../pages/demo/html?a=3&... returns 200, the viewer switches to "Loading…", htmlstatus polling resumes, and the error panel returns about 15 seconds later.
• Open the same page with no ?a= (success) and with ?a=2 (pending) and confirm the notebook iframe renders and the loading state persists, respectively — unchanged from before.
• Run pnpm run check-openapi-drift and confirm times-square-client reports in sync (a version-only difference against the latest docs build, which currently serves a dev-versioned spec) rather than DRIFT.
• Confirm packages/times-square-client/openapi.json reports info.version 0.25.0 and defines execution_error on the HtmlStatus schema.
• Run pnpm dev --filter squareone, open /times-square/github/lsst-sqre/times-square-demo/demo?a=3, and confirm the sidebar summary reads the failure title and when it failed — not "Computed … in N seconds."
• Click Recompute in that failed-state sidebar and confirm the request goes out and the viewer drops back to its loading state, exactly as from a successful run.
• Open the same page with ?a=1 and confirm the sidebar still reads "Computed … in N seconds." and ?a=2 still shows "Computing…".
• On that ?a=3 page, inspect the sidebar failure summary and the SSE connection alert and confirm the text renders dark red (#ad1919) rather than the default body color.
• In Storybook (pnpm storybook --filter @lsst-sqre/squared), open the TextInput / TextArea validation-state stories and the Select "With Form Field Error" story and confirm the error variants' text is dark red, matching their red border, instead of ordinary body text.
• Load a token detail page, trigger a delete failure, and confirm the error panel's text and heading render dark red on the light red background.

References

• Closes Add execution_error to times-square-client schemas and mock data #621
• Closes Add bounded-reconnect options to subscribeToHtmlEvents #622
• Closes Stop useHtmlStatus polling on terminal execution_error #623
• Closes Auto-abort SSE subscription on execution_error events #624
• Closes Add soft-delete client function and re-run mutation #625
• Closes Migrate SSE provider onto package transport with execution-error context #626
• Closes Migrate ExecStats recompute onto the shared soft-delete mutation #627
• Closes Render terminal error panel with Re-run in the notebook viewer #628
• Closes Dev mock API failure states and DELETE handler #629
• Closes Re-vendor Times Square OpenAPI spec at 0.25.0 #630
• Closes ExecStats reflects executionError instead of stale Computed line #634
• Closes Replace undefined red-900 token with red-600 in error styles #635
• PRD: DM-55470: Reach a terminal error state for failed notebook executions in the Times Square UI #620
• Jira: https://rubinobs.atlassian.net/browse/DM-55470

[changeset-bot]

🦋 Changeset detected

Latest commit: 2ccb629

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 3 packages

Name	Type
@lsst-sqre/squared	Patch
squareone	Minor
@lsst-sqre/times-square-client	Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[jonathansick]

Stoker review — PR #631 (review pass 1)

Well-structured adoption of the terminal execution_error contract with strong backward-compatibility handling and thorough tests across the package transport, app providers, viewer UI, and dev mocks; verified against the vendored fetch-event-source semantics. One edge-case robustness bug in the error panel's code-to-presentation lookup and two informational nits — nothing blocking.

Findings

1. [warning / correctness] apps/squareone/src/components/TimesSquareNotebookViewer/NotebookExecutionError.tsx (L45–63): The presentation lookup presentations[executionError.code] ?? genericPresentation uses a plain object literal, so a code equal to an Object.prototype property name (e.g. constructor, toString, valueOf) resolves to the inherited value rather than undefined. The ?? fallback is bypassed, Icon and tone destructure to undefined, and rendering <Icon /> throws — defeating the component's own unknown-code fallback for exactly the forward-compatibility case it documents. The code value is API-supplied, so this is unlikely but real.
   Suggested fix: guard with Object.hasOwn(presentations, executionError.code) (or a null-prototype record / Map), and add a test/story with code constructor to lock in the fallback.

2. [info / maintainability] apps/squareone/src/lib/mocks/timesSquareExecutionStore.ts (L25–37): mockExecutionError duplicates, verbatim, the fixture the package already exports from @lsst-sqre/times-square-client mock-data (the file even imports the ExecutionError type from the package). The two copies can silently drift.
   Suggested fix: import the package's mockExecutionError instead of redefining it, or intentionally diverge and note why.

3. [info / compatibility] packages/times-square-client/src/mutation-options.ts (L10–14): mutationOptions is imported from @tanstack/react-query, but that helper was added partway through the v5 line (present in the installed 5.90.20). The package's peerDependencies still declare ^5, so a consumer on an early 5.x would fail at import.
   Suggested fix: verify the release that introduced mutationOptions and raise the peer range floor accordingly (e.g. ^5.83.0).

{
  "stoker_review_version": 1,
  "pr_number": 631,
  "blocking": false,
  "summary": "Well-structured adoption of the terminal execution_error contract with strong backward-compatibility handling and thorough tests across the package transport, app providers, viewer UI, and dev mocks; verified against the vendored fetch-event-source semantics. One edge-case robustness bug in the error panel's code-to-presentation lookup and two informational nits — nothing blocking.",
  "findings": [
    {
      "id": "f1",
      "severity": "warning",
      "category": "correctness",
      "file": "apps/squareone/src/components/TimesSquareNotebookViewer/NotebookExecutionError.tsx",
      "line_start": 45,
      "line_end": 63,
      "summary": "The presentation lookup `presentations[executionError.code] ?? genericPresentation` uses a plain object literal, so a code equal to an Object.prototype property name (e.g. 'constructor', 'toString', 'valueOf') resolves to the inherited value rather than undefined. The `??` fallback is bypassed, `Icon` and `tone` destructure to undefined, and rendering `<Icon />` throws — defeating the component's own unknown-code fallback for exactly the forward-compatibility case it documents. The code value is API-supplied, so this is unlikely but real.",
      "suggested_fix": "Guard the lookup with an own-property check, e.g. `const presentation = Object.hasOwn(presentations, executionError.code) ? presentations[executionError.code] : genericPresentation;`, or build `presentations` as a `Map` / `Object.create(null)`-based record. Consider adding a story/test case with code 'constructor' to lock in the fallback."
    },
    {
      "id": "f2",
      "severity": "info",
      "category": "maintainability",
      "file": "apps/squareone/src/lib/mocks/timesSquareExecutionStore.ts",
      "line_start": 25,
      "line_end": 37,
      "summary": "`mockExecutionError` here duplicates, verbatim, the fixture the package already exports from `@lsst-sqre/times-square-client` mock-data (the file even imports the ExecutionError type from the package). The two copies can silently drift.",
      "suggested_fix": "Import and re-export the package's `mockExecutionError` instead of redefining it, or intentionally diverge the dev-mock copy and note why."
    },
    {
      "id": "f3",
      "severity": "info",
      "category": "compatibility",
      "file": "packages/times-square-client/src/mutation-options.ts",
      "line_start": 10,
      "line_end": 14,
      "summary": "`mutationOptions` is imported from `@tanstack/react-query`, but that helper was only added partway through the v5 line (present in the installed 5.90.20; believed introduced around 5.83 — needs verification). The package's peerDependencies still declare `^5`, so a consumer on an early 5.x would fail at import. Low stakes while squareone (pinned ^5.90.20) is the only consumer.",
      "suggested_fix": "Verify the release that introduced `mutationOptions` and raise the `@tanstack/react-query` peer range floor accordingly (e.g. `^5.83.0`)."
    }
  ]
}

[jonathansick]

Stoker review — PR #631 (review pass 2, scoped to e8ade4f)

The fixup commit correctly addresses all three findings from the first review: the Object.hasOwn presentation-lookup guard lands with thorough prototype-name tests (constructor, toString, valueOf, __proto__, plus a consistency assertion against ordinary unknown codes); the dev-mock mockExecutionError is deduplicated onto the package's identical fixture export; and the @tanstack/react-query peer floor is raised to ^5.82.0 with a matching devDependency, lockfile entry, and changeset note. No new defects introduced.

{
  "stoker_review_version": 1,
  "pr_number": 631,
  "blocking": false,
  "summary": "The fixup commit correctly addresses all three findings from the first review: the Object.hasOwn presentation-lookup guard lands with thorough prototype-name tests, the dev-mock mockExecutionError is deduplicated onto the package's identical fixture export, and the @tanstack/react-query peer floor is raised to ^5.82.0 with a matching devDependency, lockfile entry, and changeset note. No new defects introduced.",
  "findings": []
}

[jonathansick]

Stoker review — PR #631 (review pass 3, scoped to 6a65631)

The follow-up commit (task #634) is correct and well-tested: the executionError branch precedes the complete branch so a failed run is never reported as computed, the no-finish-time case degrades gracefully, the recompute path is preserved and covered from the failed state, and the stories are now typed against the real context contract. One non-blocking token-hygiene note; mergeable as-is.

Findings

1. [info / clarity] apps/squareone/src/components/TimesSquareGitHubPagePanel/ExecStats.module.css (L14–19): the new .failure class uses var(--rsd-color-red-900), but that custom property is not defined anywhere in the repo — the rubin-style-dictionary red scale tops out at 800, so the color declaration silently falls back to the inherited text color. This mirrors the pre-existing .error class in the same file and other usages across the codebase (TokenDetailsView.module.css, squared TextInput), so it is an established latent token gap rather than a regression introduced here.
   Suggested follow-up (not required for this PR): either switch these usages to the defined --rsd-color-red-600 ("Dark red for solid backgrounds and text") or add a 900 shade to the red scale in rubin-style-dictionary, then sweep the existing red-900 usages.

{
  "stoker_review_version": 1,
  "pr_number": 631,
  "blocking": false,
  "summary": "The follow-up commit 6a656319 (task #634) is correct and well-tested: the executionError branch precedes the 'complete' branch so a failed run is never reported as computed, the no-finish-time case degrades gracefully, the recompute path is preserved and covered from the failed state, and the stories are now typed against the real context contract. One non-blocking token-hygiene note; mergeable as-is.",
  "findings": [
    {
      "id": "f1",
      "severity": "info",
      "category": "clarity",
      "file": "apps/squareone/src/components/TimesSquareGitHubPagePanel/ExecStats.module.css",
      "line_start": 14,
      "line_end": 19,
      "summary": "The new .failure class uses var(--rsd-color-red-900), but that custom property is not defined anywhere in the repo — the rubin-style-dictionary red scale tops out at 800 (packages/rubin-style-dictionary/src/color/red.yaml), so the color declaration silently falls back to the inherited text color and the failure headline is not actually red. This mirrors the pre-existing .error class in the same file and other usages across the codebase (TokenDetailsView.module.css, squared TextInput), so it is an established latent token gap rather than a regression introduced here.",
      "suggested_fix": "As a follow-up (not required for this PR): either switch these usages to the defined --rsd-color-red-600 (documented as 'Dark red for solid backgrounds and text') or add a 900 shade to the red scale in rubin-style-dictionary, then sweep the existing red-900 usages."
    }
  ]
}

[jonathansick]

Stoker review — PR #631 (review pass 4, scoped to 2ccb629)

The red-900 → red-600 sweep (task #635) is complete and correct: all nine source usages of the undefined --rsd-color-red-900 are replaced with the defined --rsd-color-red-600 (#ad1919), a repo-wide grep finds no remaining source references (only the historical planning note and generated storybook-static output), only color declarations were touched (the adjacent red-500 borders and red-100 backgrounds are preserved), and the new @lsst-sqre/squared patch changeset plus the extended app changeset accurately describe the fix. No findings.

{
  "stoker_review_version": 1,
  "pr_number": 631,
  "blocking": false,
  "summary": "The red-900 to red-600 sweep (task #635) is complete and correct: all nine source usages of the undefined --rsd-color-red-900 are replaced with the defined --rsd-color-red-600 (#ad1919), no source references remain, only color declarations were touched (red-500 borders and red-100 backgrounds preserved), and the new squared patch changeset plus the extended app changeset accurately describe the fix.",
  "findings": []
}