Uh oh!
There was an error while loading. Please reload this page.
fix(data-objectstack): map dataset-query failures by the server's error code, not its status (#5663) - #5723
Merged
os-zhuang merged 2 commits intoAug 22, 2026
Conversation
…or code, not its status (#5663) `POST /api/v1/analytics/dataset/query` answers 404 for two unrelated conditions — the runtime dispatcher's `ROUTE_NOT_FOUND` when the route was never mounted, and the route's own `NOT_FOUND` when `body.datasetName` matches no saved dataset. `queryDataset` tested `res.status === 501 || res.status === 404` and called all of it "the analytics capability is not installed", so every unknown dataset told the operator to install a server plugin. Measured live on a prod tenant, that banner ran on four HotCRM widgets while analytics was installed and answering; the real condition was an installed app at 1.3.0 whose datasets ship in 2.2.2. Branch on the ADR-0112 `code` instead, giving three conditions three answers: `NOT_IMPLEMENTED`/`ROUTE_NOT_FOUND` keep the capability-missing copy, `NOT_FOUND` gets the new `AnalyticsDatasetNotFoundError`, and `401 UNAUTHENTICATED` gets the new `AnalyticsUnauthenticatedError` rather than being read as either. The headline is now a pure function of `code` and the parenthetical a verbatim quote of `message` off the same response, so the banner can no longer quote its subject and contradict it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EuPCi56cnGyykygi3z9w4m
…e, not @link (#5663) `readAnalyticsErrorEnvelope` is module-private, so a `{@link}` to it from an exported class's docstring has no target in the emitted declarations. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EuPCi56cnGyykygi3z9w4m
Contributor
✅ Console Performance Budget
The eager closure is every chunk the entry reaches through static imports — what the browser fetches and parses before the app renders. The entry chunk on its own is a small fraction of it. 📦 Bundle Size Report
Size Limits
|
os-zhuang
marked this pull request as ready for review
August 22, 2026 22:45
Uh oh!
There was an error while loading. Please reload this page.
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 freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Fixes#5663
POST /api/v1/analytics/dataset/queryanswers 404 for two unrelated conditions.ObjectStackAdapter.queryDatasettested the status and could not see the difference:codeROUTE_NOT_FOUND@objectstack/runtimedispatcher-plugin.ts(wrapped envelope)NOT_IMPLEMENTED@objectstack/restregisterAnalyticsEndpointsNOT_FOUNDbody.datasetNamelookup missUNAUTHENTICATEDenforceAuth→ANONYMOUS_DENY_BODY(@objectstack/core)Rows 1 and 3 share a status and nothing else. Every unknown dataset therefore produced the capability-missing banner: on the reported prod tenant, four HotCRM Executive Overview widgets told the operator to install
@objectstack/service-analyticsand mountAnalyticsServicePlugin, while the analytics service was installed and answering the whole time. The real condition was an installedapp.objectstack.hotcrmat 1.3.0 whose datasets ship in 2.2.2 — a package upgrade, the opposite corner of the system from the remedy the banner named.What changed
queryDatasetnow branches on thecodethe framework declares for each condition, and consults the status only as a residual when the answer carries no code at all:NOT_IMPLEMENTED/ROUTE_NOT_FOUND→AnalyticsNotInstalledError, copy unchanged. One remedy, one message.NOT_FOUND→ newAnalyticsDatasetNotFoundError(ANALYTICS_DATASET_NOT_FOUND), which names the dataset and points at the installed app's version.UNAUTHENTICATED→ newAnalyticsUnauthenticatedError(ANALYTICS_UNAUTHENTICATED), which says the request was refused before it ran and therefore says nothing about the capability. Triage ruled this a third branch rather than a shade of either other one; an expired session reported as a missing capability is the same defect wearing a different mask.Dataset query failed: …with its server detail.The status is not a re-entry point here: the residual is reached only after every code branch has declined. A 404 is safe there and unsafe as a primary test for exactly the same reason — the route's own
NOT_FOUNDalways ships a code, so a code-less 404 cannot be the unknown-dataset case.The headline can no longer contradict its own parenthetical
The reported banner printed the server's real message in parentheses —
(server said: Dataset "opportunity_metrics" not found.)— under a headline claiming a missing capability. A diagnostic that quotes its subject and then overrides its meaning is worse than one that says nothing, because it reads as authoritative.That is now structurally impossible rather than merely corrected. The headline is a pure function of
code; the parenthetical is a verbatim quote ofmessage; both are read off the same response by the same reader, andmessagenever feeds classification. If they ever disagree the producer has a bug — whereas before, the consumer did: the headline came from a status two conditions share while the quote came from the one that had actually happened. A parameterised test walks every branch asserting each message carries its own headline and none of the others'.Reading two envelopes on purpose
One url, two possible producers, so
readAnalyticsErrorEnvelopereads two shapes: the flat{ code, message }the route writes itself, and the wrapped{ success: false, error: { code, message } }the dispatcher writes when the route is not mounted. Both are live and sanctioned by ADR-0112's 2026-07-30 amendment.This is not the tolerant
body.error?.code ?? body.errorchain@objectstack/core'sanonymous-deny.tswarns consumers off. The two families are told apart structurally (typeof body.error === 'object'), never by trying one key and falling through to the other, so a producer that regresses its envelope reads as "no code" — the honest answer, which lands in the residual — instead of being quietly absorbed. And which family answered is itself part of the signal: only the wrappedROUTE_NOT_FOUNDcan mean the route is absent, and only the route's own flat envelope can mean the dataset is.Scope notes
AnalyticsNotInstalledErrorkeeps itscode, its copy and its constructor arity (it gains an optional thirdserverCodeargument plus aserverCodefield). Consumers matchingANALYTICS_NOT_INSTALLED— includingmetadata-admin'sDatasetPreview, whose test constructs the capability-missing string as a fixture — are untouched, so nothing underpackages/app-shell/**needed editing.CUBE_NOT_FOUND(404) used to land in the capability-missing branch by the same status collision; it now keeps its server detail. Pinned.classifyAnalyticsFailure(same file, the/analytics/queryface) has the same shape —status === 404is tested before the code and short-circuits it, so aCUBE_NOT_FOUNDis warned as a missing capability and silently degraded to a client-side aggregate. That is a different route face with a different envelope family, and correcting it moves degradation behaviour rather than copy, so it needs its own measurement:classifyAnalyticsFailurereads the status before the code, so/analytics/query'sCUBE_NOT_FOUNDis warned as a missing capability and silently degraded #5721.@object-ui/data-objectstackis a transport adapter with no@object-ui/i18ndependency and not()call sites; both live renderers (plugin-dashboard'sDatasetWidget,app-shell'sDatasetPreview) printerror.messageverbatim. Localising this copy means moving the mapping into a renderer, which is a different change in a different package. Adding pack keys nothing reads would be dead keys on arrival. Called out for the maintainer in the issue report.Verification
All at
5c2376387, the branch head.pnpm exec vitest run --maxWorkers=2 packages/data-objectstack/→Test Files 42 passed (42) · Tests 576 passed (576)(545 before; 31 new)pnpm --filter @object-ui/data-objectstack type-check→tsc --noEmit, exit 0pnpm exec eslint .inpackages/data-objectstack→✖ 368 problems (0 errors, 368 warnings), all pre-existingno-explicit-anycheck:control-bytes→✅ OK (scanned 4768 tracked text file(s); skipped 85 binary)check:i18n-keys→✅ Every in-scope call-site key resolves against the en pack (2924 keys)…check:i18n-drift→✅ No en value changed in this range.check:phantom-deps,check:self-import,check:esm-specifiers→ exit 0check-changeset-presence.mjs→✅ 2 source file(s) of 1 released package(s) changed, and this change declares 1 changeset(s)check-changeset-no-major.mjs→✅ No changeset declares a major bump.Ablation. Restoring the pre-fix
res.status === 501 || res.status === 404mapping (mutation confirmed on disk by counting both the injected marker, 1, and the deleted branch text, 0) turns 9 of the 31 new tests red: all four branch pins, the incident replay, the dataset-unknown and not-signed-in halves of the headline/parenthetical sweep, the bare-401 residual, and the unrecognised-404 pin. The route-absent headline pin correctly stays green — the ablation does not change that branch's verdict, only itsserverCodeprovenance. No rebuild was needed: the test imports./indexby relative source path, sodiststaleness cannot make the ablation falsely green. The restore leg ran under anEXIT INT TERMtrap and was verified byte-identical to the commit (git diff --stat HEADempty), then re-measured green at 576/576.Cross-package type check. Against the rebuilt
dist/index.d.ts: a consumer calling the pre-existing two-argumentnew AnalyticsNotInstalledError(surface, detail)still compiles (exit 0), and the reverse leg — passingdatasetName: 404— fails withTS2322: Type 'number' is not assignable to type 'string', confirming the declarations being read are the rebuilt ones and not a cache.Declared narrowing. Repo-wide
pnpm test/pnpm type-check/pnpm lint(allturbo run …) are CI's runs and are not duplicated here. The lint narrowing is a measurement, not a skip: the population comes from eslint's own config resolution viaeslint .in the package — the exact commandturbo run lintinvokes for it — the file count (48) is read from--format json, andeslint.config.jsenables no type-aware linting (tseslint.configs.recommended, noparserOptions.project/projectService), so every file's verdict is a function of its own text plus the shared config and this diff cannot move a verdict in any file it does not edit. The diff touches two files, both inside this one package.Generated by Claude Code