Loadable lifecycle improvements: abort handling and unified exception hook#4361
Open
lbwexler wants to merge 37 commits into
Open
Loadable lifecycle improvements: abort handling and unified exception hook#4361lbwexler wants to merge 37 commits into
lbwexler wants to merge 37 commits into
Conversation
Matches the pattern already used in toolbox's .mcp.json. Default of 64342 is preserved for developers without the env var set. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Compare loadNumber against owner.lastRequested?.loadNumber instead of object identity - clones produced by cloneWithSpan() share the same loadNumber but are no longer === to lastRequested, so the previous identity check returned true on every clone.
# Conflicts: # CHANGELOG.md
# Conflicts: # CHANGELOG.md
# Conflicts: # CHANGELOG.md
# Conflicts: # CHANGELOG.md
…spanPrefix - Renamed `Runner.track`/`linkTo` → `withTrack`/`withObserver` and `logInfo`/`logDebug` → `withInfo`/`withDebug` for consistency and to disambiguate from the same-named Promise extensions and `HoistBase` log methods. - Renamed `fetchJson` → `runFetchJson`, added `runPostJson`, and dropped the unused/rare `fetch`, `putJson`, `deleteJson` shortcuts. The `run` prefix marks terminals clearly; raw fetches and other verbs go through `.run(ctx => ...)`. - Renamed `HoistBase.spanPrefix` → `telemetryPrefix` and updated all overrides. - DataAccess: pulled post-fetch `View.fromBlob` transformations inside `.run()` lambdas so they're covered by the runner's span/observability. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…eshHandle # Conflicts: # CHANGELOG.md
## API additions * New `XH.metricsService` — lightweight client-side service recording named timers and counters with optional tags, debouncing batches to the server's `/xh/recordMetrics` endpoint where they flow into the Micrometer registry. Requires hoist-core >= 40.0 (xh/hoist-core#556). * `Runner.timer(name, tags?)` and `Runner.counter(name, tags?)` — record on completion of the wrapped fn and automatically attach an `xh.outcome` tag (`success` | `failure`) based on whether the fn threw. ## Behavior changes (minor) * `Runner.counter()` semantic: previously fired before the wrapped fn ran; now records on completion. --------- Co-authored-by: Anselm McClain <atm@xh.io>
- Fix broken catch ternary that swallowed skipped errors incorrectly; route non-aborted skipped errors through XH.handleException - Skip state updates (lastLoadCompleted/Exception/Succeeded) for skipped loads and capture local `requested` Date so elapsed remains correct for skipped/first-load cases - Tag `lastRequested`, `lastSucceeded`, `target` as @internal and document their roles - Clarify Loadable doc on `lastLoadCompleted`/`lastLoadException` re: skipped loads - Replace lodash `pull` with `omitBy(isNil)` for the debug log payload Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- CHANGELOG: fix stray paren / truncated sentence on `skipStaleLoads` description - LoadAbortedException: reference correct flag name (`skipStaleLoads`, not `abortStaleLoads`) - Loadable: clarify that skipped exceptions are silently swallowed (no alert, no server log), and reference `handleLoadException` (not `handleException`) Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
`HoistModel`/`HoistService.handleLoadException` now calls `XH.handleException(e)` directly. The previous implementation delegated to `LoadSupport.handleLoadException`, which itself called `XH.handleException` - one hop for no reason, since `LoadSupport.doLoadAsync` invokes the target's own `handleLoadException` (not its own). Dropped the now-unused `LoadSupport.handleLoadException`. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Aligns with sibling lifecycle methods (`doLoadAsync` etc.), which are all required with framework-supplied defaults. `HoistModel`/`HoistService` already implement it; added a no-op on `LoadSupport` (since it acts as orchestrator, not target) and a direct delegation on `UrlStore`. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Calls `XH.handleException(e)` instead of being a no-op, matching the `Loadable.handleLoadException` JSDoc and the implementations on `HoistModel`/`HoistService`/`UrlStore`. Easier to reason about (uniform default) and safer if `LoadSupport.handleLoadException` is ever invoked directly. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- LoadSupport: superseded/auto-refresh load errors no longer alert the user, but genuine (non-abort) errors are still logged server-side (pass showAlert: false rather than relying on default opts). - Loadable: corrected skipStaleLoads / skipAutoRefreshErrors docs to match actual behavior (logged-not-alerted, not "no server log"). - CHANGELOG: documented the isFetchAborted -> isAborted rename (Breaking Changes) and handleLoadException becoming a required Loadable member (TS API Adjustments).
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.
Builds on
spanItAll. Reduces per-model boilerplate and makes handling of stale, obsolete, and auto-refresh load errors consistent across theLoadablelifecycle.Additions
LoadAbortedException(isRoutine+isAborted) - thrown when a load is superseded. Raised automatically byFetchServiceafter each fetch (and on demand viaLoadSpec.abortIfNeeded()for non-fetch async work), based onLoadSpec.shouldAbort. Recognized byExceptionHandlerand dropped silently.Loadable.skipStaleLoads(defaulttrue) - skip/abort loads superseded by a newer started request. Loads superseded by a newer completed request are always skipped.Loadable.skipAutoRefreshErrors(defaulttrue) - silence errors raised during an auto-refresh.Loadable.handleLoadException(e, loadSpec)- new hook called only for surface-worthy failures (abort exceptions and, per the flags above, stale/auto-refresh errors are filtered out first). Default delegates toXH.handleException(e). Now a required member of theLoadableinterface;HoistModel/HoistServiceprovide the default.Other changes
isFetchAbortedexception flag toisAborted(now also set byLoadAbortedException). Breaking - update references incatchWhen/catchDefaultWhenpredicates etc.LoadSpec.isStaleto compareloadNumber(stable acrosscloneWithSpan) rather than object identity.if (loadSpec.isStale) return/.catchboilerplate to the new hook.Behavior change to default exception handling — open for discussion
Previously,
LoadSupportcaught an exception fromdoLoadAsynconly to record it and then re-threw it; it never calledXH.handleExceptionitself. For a surface-worthy load that an app did not handle, the error therefore reached the user only if something downstream surfaced it — and the standard refresh lifecycle (loadAllAsyncviaRefreshContextModel) swallows rejections withPromise.allSettled+logError, so the common outcome was a console log, no dialog.Now
LoadSupportroutes such errors tohandleLoadException(default:XH.handleException(e)), so a non-auto-refresh load that throws and isn't handled by the app will surface a standard error dialog by default. (Superseded/auto-refresh failures remain silent — no alert; genuine non-abort errors are still logged server-side.) Note also thatloadAsync()no longer rejects for these errors — it resolves and routes them to the hook.This is arguably the better default (a failed initial load surfaces instead of leaving a blank view), but it is an observable change for apps that relied on the old console-only behavior. If we want to soften the default, we could scale it back to either or both of:
handleLoadExceptioncallsXH.handleException(e, {showAlert: false}), so failures are recorded but not shown to the user.loadAsync()continue to reject), more closely matching the current behavior so existing callers/lifecycle handling stay in control.