refactor(data-objectstack): delete the subsumed CONCURRENT_UPDATE guard and align the predicate's doc - #6422
Merged
Conversation
`normaliseClientError` carried two stacked guards before its re-wrap:
if (e.code !== 'CONCURRENT_UPDATE' && e.httpStatus !== 409) return error;
if (e.code !== 'CONCURRENT_UPDATE') return error;
The first can never decide an outcome — its condition is strictly stronger
than the second's, so every input it would have returned is returned one line
later anyway. Its `httpStatus !== 409` half advertised a second acceptance
path (a bare 409 still being re-wrapped) that does not exist, on the one
function whose whole job is deciding what gets re-wrapped. Deleted, with the
effective rule written where it used to sit.
Also aligned the doc above the exported `isConcurrentUpdateError` with the
predicate below it: the doc named only the wire shape while the code accepts
`name === 'ConcurrentUpdateError'` too. The `name` limb is KEPT — it is the
deliberate cross-realm discriminator that `isViewConfigPermissionDeniedError`
already documents and cites this function as its precedent for — and the doc
now records that instead of leaving it to read as drift.
Both accepted sets are pinned as an explicit truth table in `occ.test.ts`,
including the rows that go red under the other possible deletion.
Cleanup for objectui#6375.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011SfZeFWrhGLHmfq61xbz4q
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
|
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
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 #6375
What changed
1. The subsumed guard is gone.
normaliseClientErrorcarried two stacked guards before itsCONCURRENT_UPDATEre-wrap. Re-derived on this branch againstorigin/main9602dc820(lines 1150-1151 at the time of writing):The first line's condition is strictly stronger than the second's, so it can never be the line that decides an outcome — anything it would have returned is returned one line later anyway. Its
httpStatus !== 409half advertised a second acceptance path (a bare 409 still getting re-wrapped) that does not exist, on the one function whose whole job is deciding what gets re-wrapped. Deleted, with the effective rule — the wirecodeis the sole discriminator — written where the dead line used to sit.2. The doc moved, not the predicate. The charter asked that
isConcurrentUpdateError's doc and itscode || namepredicate stop disagreeing, and said which way to go depends on whether thename ===limb is deliberate. It is deliberate, so the doc is the half that moved. The evidence, in the order it decides the question:The rationale is already written down in this same file, above the sibling
isViewConfigPermissionDeniedError, and it cites this function as its precedent:Deleting the limb would leave that doc citing a predicate that no longer does what it says it does.
A consumer census is the wrong measurement here, and it also comes back empty. The case the limb exists for — a host holding two copies of the class, where
instanceoffails and thenamestring is the only discriminator left — is out of tree by construction. For the record the census was run anyway: no in-repo consumer imports this predicate at all.plugin-detailimports a same-named local copy from./ConcurrentUpdateDialog, not this one. Zero consumers is therefore not evidence against the limb.Two sibling packages carry the same two-limb check, each documenting that it duck-types in order not to depend on the adapter:
packages/plugin-form/src/occSave.tsx:78andpackages/plugin-detail/src/ConcurrentUpdateDialog.tsx:223.plugin-form's own test already pins thenamelimb (occSave.test.tsx:86:expect(isConcurrentUpdateError({ name: 'ConcurrentUpdateError' })).toBe(true)). It is a house pattern with a live pin, not drift.So both limbs stay, and the reason now sits beside the predicate where the next reader meets it. The doc also records the deliberate width difference: the predicate accepts
codeornameand readshttpStatusfor neither, while the re-wrap keys on the wirecodealone.3. The truth table is pinned in
packages/data-objectstack/src/occ.test.ts(see below for why it is not a vacuous pin).The pin is about this deletion, not about the function existing
The deletion is a no-op by construction, so "behaviour unchanged" would pass before and after and prove nothing. What earns these rows their keep is an asymmetry: two of them go red under the other possible deletion — dropping the
code !== 'CONCURRENT_UPDATE'line and keeping the conjunction, under which a 409 whose code says something else falls through and gets re-wrapped as a conflict it never was.Three runs, each with its direction predicted first:
origin/mainsourceTest Files 1 passed (1)/Tests 18 passed (18)Test Files 1 passed (1)/Tests 18 passed (18)Run C's two failures were exactly the predicted rows — "passes through a 409 whose code is NOT CONCURRENT_UPDATE" and "passes through a bare 409 carrying no code at all" — and nothing else moved.
The ablation mutated source only (no
distinvolvement: the test imports./indexinside the package, and vitest resolves it from source), and the mutation was confirmed on disk before the run rather than from an editor exit code: injected textgrep -c= 1, removed textgrep -cx= 0, and the working-treegit hash-objectmoved off theHEADblob hash (fd389f5c…tob019a597…). The restore wasgit checkout HEAD -- <abs path>under atrap … EXIT INT TERM, and is proven by hash rather than by exit code: working tree back tofd389f5c974ef83138d5008495b1b14c87e3d8b7, byte-identical to theHEADblob,git diff HEADempty.Verification — union run on
3d85c0cd9(the final commit)pnpm exec vitest run packages/data-objectstack/—Test Files 47 passed (47)/Tests 640 passed (640)pnpm --filter @object-ui/data-objectstack run type-check—tsc --noEmit, exit 0. Confirmed it actually covers the edited files rather than excluding tests:tsc --listFileslists bothsrc/index.tsandsrc/occ.test.ts.node scripts/check-control-bytes.mjs—OK (scanned 5314 tracked text file(s))node scripts/check-changeset-presence.mjs— declared with an empty frontmatter changeset (this repo's explicit "releases nothing" form; theskip-changesetlabel is the other repo's mechanism and was deliberately not used here). Gate prints: "2 source file(s) of 1 released package(s) changed, and this change declares 1 changeset(s) … Every one of them has an EMPTY frontmatter".node scripts/check-changeset-no-major.mjs—No changeset declares a major bump.node scripts/check-changeset-fixed.mjs— OKnode scripts/check-vi-mock-specifiers.mjs— OKnode scripts/check-shell-escape-residue.mjs— OKTwo declared narrowings, so the difference between measured and not run stays visible:
eslintwas run on the two edited files, not repo-wide.pnpm lintisturbo run lintacross 47 packages — CI runs it either way. Targeted result: 0 errors, 123 warnings, all@typescript-eslint/no-explicit-anyand all pre-existing (the two inocc.test.tsare at lines 29 and 86, both in assertions that predate this branch; the added block introduces noany). This repo's eslint config carries no type-aware linting and no formatting rules, so nothing in this diff can move a verdict on a file it does not touch.check-readme-exportscould not be fully measured locally — prerequisite, not a red gate. It needs every package'sdist/*.d.tson disk and says so itself ("its type entry./dist/index.d.tsis not on disk -- runpnpm buildfirst"). In a fresh worktree 34 of 40 packages are unbuilt. After building the package this PR touches, every unjudged item namingpackages/data-objectstackdisappeared (356 to 348 unjudged — exactly its 8 entries; judged self-imports went 22 to 30 real, 0 wrong-path, 0 fabricated). The remaining 348 belong to the other 33 unbuilt packages. CI builds first and measures the rest.Scope
The guard deletion, the doc alignment, and the pin. This file's other error paths and its 8+
normaliseClientErrorcall sites were not touched.One observation-class item found while reading the boundary was recorded as a separate unassigned card rather than patched here — objectui#6421,
plugin-detail's same-named local predicate narrows to a requiredcode: 'CONCURRENT_UPDATE'while accepting name-only errors that lack it. No consumer reads.codeoff the narrowed result, so there is no symptom today. Out of scope for this PR and intentionally left open: #6421 is not addressed here.Release impact
None. Comment, doc and test changes plus one deleted line that decided nothing; declared as releasing nothing via the empty-frontmatter changeset.
Draft on purpose: the PM lands this one. Not marked ready, no auto-merge, not queued.
Generated by Claude Code
Generated by Claude Code