Skip to content

FE-1226: Split the optimizations provider into policy, errors, and storage modules - #9151

Draft
kube wants to merge 1 commit into
mainfrom
cf/fe-1226-provider-simplification
Draft

FE-1226: Split the optimizations provider into policy, errors, and storage modules#9151
kube wants to merge 1 commit into
mainfrom
cf/fe-1226-provider-simplification

Conversation

@kube

@kube kube commented Aug 3, 2026

Copy link
Copy Markdown
Collaborator

🌟 What is the purpose of this PR?

Make the detached-run optimization provider readable. #9066 left it at 874 lines: about half was framework-free logic that happened to live next to the React state using it, and its attach loop was a single 258-line callback whose failure handling interleaved reconnect policy with state effects.

This is a pure refactor — no behavioural change, no public API change. The 17 provider tests from #9066 pass untouched, which is the guarantee that nothing moved semantically.

🔗 Related links

  • FE-1226 — the reconnect work this cleans up after
  • FE-1217 — parent hardening issue

🚫 Blocked by

🔍 What does this change?

Three modules extracted from provider.tsx, all internal (nothing is added to the @hashintel/petrinaut/react entry point):

  • reconnect-policy.ts — the backoff constants, reconnectDelayMs, abortableDelay, and a new pure decideAttachFailure that maps one failure to cancelled / settled / expired / reconnect / giveUp. The attach loop's 75-line catch cascade — six sequential ifs, each ending in a return, with the reconnect predicate computed inline in the middle — becomes a switch over that decision. The policy's ordering constraints are now stated where the policy lives, and testable without driving a provider through fake streams.
  • transport-errors.ts — the duck-typed classifyError, buildErrorMessage, and isAbortError, plus one new isAbortFailure predicate. Run creation and the attach loop each carried their own copy of the same three-part abort check (signal.aborted || isAbortError(error) || classified?.category === "aborted"); they now share one, so they cannot drift.
  • active-run-storage.ts — the sessionStorage bookkeeping for runs a reload may re-attach to. ACTIVE_RUNS_STORAGE_KEY is now imported by the tests instead of the string literal being repeated eight times.

Net effect: provider.tsx 874 → 656 lines, and runAttachLoop 258 → 163.

Also corrected in passing: the comment on computeRunningBest claimed the service "no longer knows the objective direction after the creating request ends". That was true of the old connection-scoped design; with detached runs the Optuna study — and its direction — lives for the run's lifetime. The reason the provider computes the running best is that cursors make any single attachment partial, not that the service has forgotten anything.

Pre-Merge Checklist 🚀

🚢 Has this modified a publishable library?

This PR:

  • modifies an npm-publishable library and I have added a changeset file(s)

📜 Does this require a change to the docs?

The changes in this PR:

  • are internal and do not require a docs change

🕸️ Does this require a change to the Turbo Graph?

The changes in this PR:

  • do not affect the execution graph

⚠️ Known issues

One item from the review that produced this PR is deliberately not done: createOptimization and the reload re-attach effect duplicate ~20 lines of lifecycle (mint id → AbortController → register in two refs → prepend record → run the loop → clear both refs). Merging them needs a nullable-runId branch, because creation only learns its run id after an await while re-attach knows it upfront — the resulting helper reads worse than the duplication. Left as is.

🛡 What tests cover this?

  • The 17 existing provider tests pass with no modification other than importing the storage key — the behaviour-preservation guard for the whole change.
  • 10 new tests in reconnect-policy.test.ts pin the decision function: the backoff sequence and its cap, which categories reconnect, that a definitive http status never does, that cancellation outranks every other outcome, that a failure after a terminal event is settled, and that a 404 only means expired on a stored run's first attachment.

26 test files / 197 tests green in @hashintel/petrinaut; eslint and oxfmt clean.

❓ How to test this?

  1. cd libs/@hashintel/petrinaut && yarn vitest --run src/react/optimizations
  2. Confirm git diff on provider.tsx is only deletions plus the rewired imports and the switch.

🤖 Generated with Claude Code

@vercel

vercel Bot commented Aug 3, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
hash Ready Ready Preview Aug 4, 2026 8:31am
petrinaut Ready Ready Preview Aug 4, 2026 8:31am
1 Skipped Deployment
Project Deployment Actions Updated (UTC)
hashdotdesign-tokens Ignored Ignored Preview Aug 4, 2026 8:31am

@github-actions github-actions Bot added area/infra Relates to version control, CI, CD or IaC (area) area/libs Relates to first-party libraries/crates/packages (area) type/eng > frontend Owned by the @frontend team labels Aug 3, 2026
Base automatically changed from cf/fe-1226-petrinaut-optimization-reconnect to main August 4, 2026 07:16
@github-actions github-actions Bot added area/apps > hash* Affects HASH (a `hash-*` app) type/eng > backend Owned by the @backend team area/tests New or updated tests area/apps area/apps > hash.design Affects the `hash.design` design site (app) labels Aug 4, 2026
…orage modules

The detached-run provider had grown to 874 lines, half of them framework-free
logic sitting next to the React state it happens to be used by, and a
258-line attach loop whose failure handling interleaved policy with effects.

- `reconnect-policy.ts`: the backoff constants and delay, plus a pure
  `decideAttachFailure` that maps a failure to one of cancelled / settled /
  expired / reconnect / giveUp. The loop's 75-line `catch` cascade becomes a
  switch over that decision, and the policy is now unit-testable directly.
- `transport-errors.ts`: duck-typed error classification and message
  building, plus a single `isAbortFailure` predicate replacing the
  three-part abort check that run creation and the attach loop each had
  their own copy of.
- `active-run-storage.ts`: the sessionStorage bookkeeping for runs a reload
  may re-attach to. Its key is now used by the tests instead of a literal
  repeated eight times.

Pure refactor: the 17 existing provider tests pass unchanged, and 10 new
tests cover the extracted decision function's ordering and cap.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@kube
kube force-pushed the cf/fe-1226-provider-simplification branch from e84d4e9 to ffd2115 Compare August 4, 2026 08:23
@github-actions github-actions Bot removed area/apps > hash* Affects HASH (a `hash-*` app) type/eng > backend Owned by the @backend team area/tests New or updated tests area/apps area/apps > hash.design Affects the `hash.design` design site (app) labels Aug 4, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/infra Relates to version control, CI, CD or IaC (area) area/libs Relates to first-party libraries/crates/packages (area) type/eng > frontend Owned by the @frontend team

Development

Successfully merging this pull request may close these issues.

1 participant