Uh oh!
There was an error while loading. Please reload this page.
test(plugin-detail): serve the record-level explain probe from a double, not the network - #4105
Merged
Merged
Conversation
…le, not the network (#3339) DetailView mounts `useRecordEditable` twice (update + delete) for any schema carrying `objectName` + `resourceId`. With no `SchemaRendererProvider` in the tree the hook has no host `apiFetch` and falls back to the GLOBAL fetch — by design, for standalone embeds. Under happy-dom that global fetch is a REAL request to the default origin, so the suite fired 36 live `POST http://localhost:3000/api/v1/security/explain` calls per run. They failed fire-and-forget: the hook swallows the rejection in order to fail open, so the suite stayed green while stderr filled with 72 lines of `connect ECONNREFUSED 127.0.0.1:3000` (two per escaped request). Answer the probe from an injected double in the two files that escape — `DetailView.test.tsx` (28 calls) and `DetailView.invalidation.test.tsx` (8). `visible: true` reproduces the old observable behaviour exactly, since a failed request already failed open, so no existing assertion changes meaning. Product code is untouched: the global-fetch fallback is deliberate, documented and pinned by `useRecordEditable.test.tsx`. The escaped request was previously asserted by nobody, so its shape was invisible. Pin it: DetailView's own wiring — recordId sourced from `resourceId`, and one probe each for the update and the delete CTA — which `useRecordEditable.test.tsx` cannot see. The double records every URL it is handed, so an escape to another endpoint fails that assertion rather than vanishing into a swallowed rejection. No global error swallowing. Fixes#3339
The latest updates on your projects. Learn more about Vercel for GitHub. |
Contributor
✅ Console Performance Budget
📦 Bundle Size Report
Size Limits
|
This was referenced Aug 10, 2026
yinlianghui
marked this pull request as ready for review
August 10, 2026 09:57
Uh oh!
There was an error while loading. Please reload this page.
This was referenced Aug 10, 2026
Merged
Merged
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#3339
Test-only. Product code is untouched.
Premise: reproduced, still valid
The card was filed @
a41568462and explicitly asked whether the escape had moved or been fixed. It has not. Onorigin/main@c29ceffb8, the card's own repro command still produces the noise:The bisect: one call site, two files, 100% of the noise
Rather than bisect file-by-file, I instrumented
fetch/XMLHttpRequest/EventSourcein the root setup files and attributed every escape to its test file. Result — the escapes are not scattered, they are a single call site:packages/plugin-detail/src/__tests__/DetailView.test.tsxpackages/plugin-detail/src/__tests__/DetailView.invalidation.test.tsxAll 36 are
POST /api/v1/security/explain; 36 requests x 2 = the 72ECONNREFUSEDlines exactly, so these two files account for all of it. The two fields/components files named in the repro command escape nothing — they are observation context, as the card said.Every escape comes from
packages/plugin-detail/src/useRecordEditable.ts:74:DetailViewmounts this hook twice (update + delete) for any schema carryingobjectName+resourceId. With noSchemaRendererProviderin the tree there is no hostapiFetch, so it falls back to the globalfetch— which under happy-dom is a real request to the default originhttp://localhost:3000. The hook swallows the rejection in order to fail open, which is exactly why the suite stayed green while stderr filled up: fire-and-forget, as the card described.Why the fix is test-side
The global-fetch fallback is deliberate product behaviour, documented in the hook's header ("a standalone
detail:viewembed has no host fetch and must still degrade to the global one") and pinned by an existing test —falls back to the global fetch in a standalone embed (no provider). Changing it to silence test noise would be changing the product to suit the tests, so the fix injects the dependency at the two call sites instead.Route chosen: injected test double, not MSW. The card allowed either. MSW is a declared devDependency of
plugin-form/plugin-gridbut is imported nowhere in this repo — there is nosetupServerharness at all, so the MSW route would mean standing up the repo's first MSW test infrastructure inside a hygiene fix. The double also matches whatuseRecordEditable.test.tsxalready does one level down.visible: truereproduces the old observable behaviour exactly — a failed request already failed open — so no existing assertion changes meaning.No global error swallowing, per the card's explicit ruling and the triage comment that reinforced it. The double is scoped to the two files, answers only the explain endpoint, and records every URL it is handed, so an escape to some other endpoint becomes a failing assertion rather than silence.
Keeping the assertions meaningful
The escaped request was asserted by nobody, so its shape was invisible. Per the card, it is now pinned — and this is genuinely new coverage, being
DetailView's own wiring rather than the hook's: that the recordId comes fromresourceId, and that both the update and the delete CTA are gated, one probe each.useRecordEditable.test.tsxcannot see any of that.Verification
Acceptance bar — the card's full repro command, stderr grepped:
Reverse verification, both directions predicted before running:
git checkout origin/main --on both test files) — noise returns: 72ECONNREFUSED, and the suite is still green at 34 tests. Confirms the double is what stops it, and that the escape never had a test watching it.DetailView's delete gate probe'update') turns it red with a precise diff:Restored afterwards;
git diff origin/main -- packages/plugin-detail/src/DetailView.tsxis empty.Gates:
pnpm --filter @object-ui/plugin-detail type-checkgreen (afterpnpm --filter '@object-ui/plugin-detail^...' build— the fresh-worktree dependency closure, without which every@object-ui/*import reads as TS2307),eslinton both files 0 errors, control-byte self-scan clean.scripts/check-changeset-presence.mjsarbitrated and required the empty-frontmatter changeset for a test-only change ("declared as releasing nothing" — its sanctioned pass, not a workaround);check-changeset-no-major.mjsgreen.Deliberately not in this PR
plugin-charts/plugin-dashboardshow the same noise per the issue's 08-09 comment, but they have a different root cause — neither package reachesuseRecordEditableorDetailViewat all. Their escape still needs its own bisect and is out of this card'splugin-detailscope.app-shell'sRecordDetailViewalso callsuseRecordEditable, so I checked rather than assumed:RecordDetailView.headerRefresh.test.tsxruns with 0ECONNREFUSED. Nothing to do there.packages/plugin-detail/vitest.setup.tsnever runs under the canonical repo-root invocation (the root config'sdomproject usesvitest.setup.dom-light.tsx), so a guard added there would protect nothing. That dead-config finding is attached to the card that owns it, objectui#3240 — it turned out to be a concrete hole inassertCanonicalVitestInvocation, whose docstring claims a coverage property that 11 packages break.Generated by Claude Code