Skip to content

fix(plugin-detail): let buildConflict consume the narrowing it is handed - #6585

Merged
os-support-ai merged 2 commits into
mainfrom
claude/issue-6477-buildconflict-narrowed-type
Aug 26, 2026
Merged

fix(plugin-detail): let buildConflict consume the narrowing it is handed#6585
os-support-ai merged 2 commits into
mainfrom
claude/issue-6477-buildconflict-narrowed-type

Conversation

@os-support-ai

Copy link
Copy Markdown
Collaborator

Fixes#6477

InlineEditSaveBar narrows correctly at the call site — if (isConcurrentUpdateError(err) && canAtomic) — and then handed the narrowed value to a callback typed err: any, which discards it. The predicate's return type was inert at its only in-repo consumer, one line after the boundary was drawn.

Step 0: the premise is live

The card was blocked behind #6421, whose PR #6474 landed on this very predicate, so the first question was whether that merge had already fixed the dependant. It had not. On origin/main at 12402a9b8:

packages/plugin-detail/src/InlineEditSaveBar.tsx:125
(draft: Record<string, any>, err: any): ConcurrentUpdateConflict => {
packages/plugin-detail/src/InlineEditSaveBar.tsx:127
const current = (err?.currentRecord ?? null) as Record<string, unknown> | null;

The question was decided by measurement, not preference

Both answers were authorised: take the narrowed type, or keep the any and write down what forces it. buildConflict reads exactly two things off err, and the narrowed type declares both, at types ConcurrentUpdateConflict accepts unchanged:

readnarrowed type declaresconflict payload wantscovered
err?.currentRecordcurrentRecord?: Record<string, unknown> | nullcurrentRecord?: Record<string, unknown> | nullyes
err?.currentVersioncurrentVersion?: stringcurrentVersion?: stringyes

Nothing forced the any, so option one was selected. As the card predicted, the as Record<string, unknown> | null cast then becomes redundant and is deleted — the two shapes agree, so there was no finding to report on that front. exactOptionalPropertyTypes is off repo-wide, so string | undefined flowing into currentVersion?: string needs no ceremony.

The type is derived from the predicate rather than restated:

typeNarrowedBy<F>=Fextends(arg: unknown)=>arg is infer N ? N : never;typeConcurrentUpdateErrorShape=NarrowedBy<typeofisConcurrentUpdateError>;

A hand-copied literal shape would be a second declaration free to drift from the predicate — precisely the disagreement this card exists to rule out. Derived, the compiler re-checks every read against whatever the predicate currently promises.

isConcurrentUpdateError is untouched

PR #6474 deliberately made that narrowed type honest (code?: 'CONCURRENT_UPDATE', optional because the name limb carries no code). Nothing here widens or otherwise edits it — this PR only changes a module-local React.useCallback parameter.

The published surface is unchanged, and that is measured rather than asserted. Building the package before and after and comparing all 52 emitted .js / .cjs / .d.ts artefacts leaves 51 byte-identical, including dist/index.js, dist/index.umd.cjs and dist/index.d.ts. The one delta is dist/InlineEditSaveBar.d.ts, which gains the BuildConflict type the pin test reads. The package's exports map declares only "." (no wildcard subpaths), so that file is unresolvable by any consumer, and src/index.tsx lists published names explicitly and does not re-export it — grep -c BuildConflict dist/index.d.ts is 0. Hence the changeset with empty frontmatter: this releases nothing.

The pins can fail — shown, not claimed

"Type-check is green" is not coverage, so both failure modes were ablated. Each mutation was confirmed on disk before measuring, and each restore confirmed byte-identical to HEAD via git hash-object.

Ablation A — the parameter goes back to any. Mutating the declared error parameter to any:

src/__tests__/InlineEditSaveBar.buildConflictNarrowed-6477.test.tsx(98,33): error TS2344: Type 'false' does not satisfy the constraint 'true'.
src/__tests__/InlineEditSaveBar.buildConflictNarrowed-6477.test.tsx(105,42): error TS2344: Type 'false' does not satisfy the constraint 'true'.
Exit status 2

Those are _ErrParamIsNotAny and _ErrParamIsTheNarrowedType, the two DISAGREES pins.

Ablation B — the narrowed type stops covering a read. Deleting currentVersion?: string from the predicate's narrowed type turns the source itself red:

src/InlineEditSaveBar.tsx(171,32): error TS2339: Property 'currentVersion' does not exist on type '{ code?: "CONCURRENT_UPDATE" | undefined; currentRecord?: ... }'.
src/InlineEditSaveBar.tsx(183,30): error TS2339: Property 'currentVersion' does not exist on type '{ code?: "CONCURRENT_UPDATE" | undefined; currentRecord?: ... }'.
Exit status 2

Under err: any both reads compiled silently and would have been undefined at runtime. That contrast is the whole point of the card.

The pin file is really in the compiler's program — tsc -p tsconfig.test.json --listFiles lists both it and src/InlineEditSaveBar.tsx (as source, not via a built .d.ts), so "type-check is clean" is a statement about these assertions and not a vacuous pass.

Known residual, stated rather than papered over: the compile-time pins bite on the exported BuildConflict type, which the component applies via React.useCallback<BuildConflict>. A revert that deleted the generic and re-annotated the lambda err: any would leave BuildConflict correct but unused and this file green. Nothing reachable from a test can observe a callback local to a component body; the runtime block covers that flank by driving the real component through a real 409.

The runtime half also closes a genuine gap: the existing suite covers the code limb only, so nothing previously proved a code-less (name-limb) error reaches buildConflict at all. The new test asserts both reads arrive in the dialog — the racer's value from currentRecord, and currentVersion in the audit line.

Gates, each quoting its own verdict, on a31b806b0

gateverdict
pnpm --filter @object-ui/plugin-detail type-checkcommand-exit 0 (both tsc --noEmit and tsc -p tsconfig.test.json)
vitest run — 4 affected files, on a31b806b0command-exit 0 · Test Files 4 passed (4) · Tests 17 passed (17)
vitest run packages/plugin-detail/ — whole package, on 14bf45c12command-exit 0 · Test Files 111 passed (111) · Tests 1036 passed (1036)
pnpm --filter @object-ui/plugin-detail lintcommand-exit 0 — 0 errors, 861 warnings (pre-existing baseline)
node scripts/check-changeset-presence.mjsexit 0 — "Every one of them has an EMPTY frontmatter — declared as releasing nothing"
node scripts/check-changeset-fixed.mjsexit 0 — "All workspace packages are in the changeset fixed group"
node scripts/check-changeset-no-major.mjsexit 0 — "No changeset declares a major bump"
node scripts/check-control-bytes.mjsexit 0 — "OK (scanned 5436 tracked text file(s))"
node scripts/check-vi-mock-specifiers.mjsexit 0
node scripts/check-lint-coverage.mjsexit 0 — "46/46 packages linted, 0 with outstanding errors"
node scripts/check-type-check-coverage.mjsexit 0 — "45/46 via type-check" · "41/41 packages compile their tests"
node scripts/check-readme-exports.mjsnot measured locally — needs a full-repo pnpm build; all 297 unjudged self-imports are unbuilt sibling packages ("type entry not on disk"), plugin-detail and BuildConflict appear 0 times. CI builds everything.

The whole-package vitest run is cited against 14bf45c12; the only later commit adds .changeset/*.md, which no test reads. The narrowed 4-file run above was re-run on the final head.

Declared narrowing: repo-wide eslint . is CI's run. Locally the affected package was linted with its own lint script — the same command turbo run lint invokes for it. Population comes from eslint's own config, not a guess: --format json reports 164 files linted, 0 errors. The verdict for untouched files cannot move, because type-aware linting is not configured (no project / projectService in eslint.config.js), so no file outside the diff can change judgement.

Measured side effect worth recording: this deletes one @typescript-eslint/no-explicit-any warning from InlineEditSaveBar.tsx (8 → 7 on the same file, measured both ways), which is exactly the err: any the card names.


Generated by Claude Code

os-support-aiand others added 2 commits August 26, 2026 14:32
InlineEditSaveBar narrows correctly at the call site and then handed the
narrowed value to a callback typed `err: any`, which discards it. The
predicate's return type was inert at its only in-repo consumer.
Measured first: buildConflict reads exactly two things off `err` --
currentRecord and currentVersion -- and the narrowed type declares both, at
types ConcurrentUpdateConflict accepts unchanged. Nothing forced the `any`,
so the parameter now takes the predicate's narrowed type and the
`as Record<string, unknown> | null` cast it made necessary is gone.
The type is DERIVED from the predicate rather than restated, so a future
narrowing that stops covering a read is a red build instead of a fresh `any`.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011SfZeFWrhGLHmfq61xbz4q
Type-only change: emitted JS and the published dist/index.d.ts are
byte-identical before and after, so an empty frontmatter is the honest
declaration rather than a workaround.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011SfZeFWrhGLHmfq61xbz4q
@os-support-aiClaude

Copy link
Copy Markdown
CollaboratorAuthor

Reviewer note on one thing this PR deliberately did not change.

With err now typed as the predicate's narrowed shape, it is non-nullable — the predicate returns false for anything that is not an object — so the ?. in err?.currentRecord and err?.currentVersion can no longer short-circuit. It is statically dead syntax.

I left it. Removing ?. is the one edit in this neighbourhood that would change emitted JS, and the whole diff being type-only is what makes the empty-frontmatter changeset provable (51 of 52 dist artefacts byte-identical, dist/index.js and dist/index.d.ts among them). Trading that for a cosmetic cleanup in a save path with no coverage of the nullish case seemed like the wrong swap, and it is outside what the card asked.

Flagging it rather than leaving a reviewer to wonder why it is still there. Happy to drop the ?. in a follow-up if you would rather have it gone — it would just need its own changeset reasoning.


Generated by Claude Code


Generated by Claude Code

@github-actions

Copy link
Copy Markdown
Contributor

✅ Console Performance Budget

MetricValueBudget
Eager closure (gzip, 52 chunks)3235.1 KB3266.6 KB
Main entry chunk (gzip)157.0 KB350 KB
Entry fileindex-B01-da7x.js
StatusPASS

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

PackageSizeGzipped
app-shell (consoleActionDispatch.js)0.20KB0.19KB
app-shell (index.js)11.71KB4.46KB
app-shell (runtime-config.js)18.10KB6.51KB
app-shell (types.js)0.01KB0.04KB
app-shell (urlParams.js)10.06KB3.86KB
auth (ActiveOrganizationStorage.js)25.05KB9.16KB
auth (AuthContext.js)0.31KB0.24KB
auth (AuthGuard.js)2.07KB1.00KB
auth (AuthProvider.js)40.18KB10.59KB
auth (AuthShell.js)3.49KB1.40KB
auth (ForgotPasswordForm.js)12.21KB3.45KB
auth (LoginForm.js)18.15KB5.39KB
auth (PreviewBanner.js)0.90KB0.50KB
auth (RegisterForm.js)6.65KB2.22KB
auth (SocialSignInButtons.js)9.61KB3.89KB
auth (UserMenu.js)3.41KB1.23KB
auth (auth-gate-events.js)1.29KB0.66KB
auth (authStyles.js)5.04KB1.72KB
auth (createAuthClient.js)40.21KB10.80KB
auth (createAuthenticatedFetch.js)8.46KB3.43KB
auth (index.js)3.19KB1.44KB
auth (invitation-status.js)1.22KB0.70KB
auth (org-roles.js)6.66KB2.78KB
auth (phone-identifier.js)1.11KB0.66KB
auth (types.js)0.59KB0.35KB
auth (useAuth.js)5.30KB1.02KB
auth (useWorkspaceAdminStatus.js)5.13KB2.35KB
collaboration (CommentThread.js)26.08KB7.56KB
collaboration (LiveCursors.js)3.17KB1.27KB
collaboration (PresenceAvatars.js)6.49KB2.64KB
collaboration (PresenceProvider.js)2.79KB1.13KB
collaboration (index.js)1.68KB0.73KB
collaboration (useCollaborationTranslation.js)6.05KB2.52KB
collaboration (useCommentSearch.js)1.98KB0.88KB
collaboration (useConflictResolution.js)7.75KB1.86KB
collaboration (useMentionNotifications.js)1.81KB0.68KB
collaboration (usePresence.js)6.33KB1.84KB
collaboration (useRealtimeSubscription.js)7.91KB2.01KB
components (index.js)506.01KB114.64KB
core (index.js)5.30KB2.13KB
create-plugin (index.js)10.08KB3.26KB
data-objectstack (index.js)173.10KB47.96KB
fields (index.js)238.89KB60.02KB
i18n (LocalizationContext.js)1.76KB0.96KB
i18n (currency.js)1.22KB0.64KB
i18n (fallbackInterpolation.js)6.25KB2.77KB
i18n (i18n.js)4.28KB1.75KB
i18n (index.js)3.44KB1.39KB
i18n (pickLocalized.js)7.62KB3.26KB
i18n (provider.js)26.89KB9.04KB
i18n (useDisplayLocale.js)2.85KB1.45KB
i18n (useObjectLabel.js)33.40KB8.71KB
i18n (useSafeTranslation.js)5.60KB2.33KB
layout (index.js)38.95KB10.97KB
mobile (MobileProvider.js)0.92KB0.49KB
mobile (ResponsiveContainer.js)0.94KB0.38KB
mobile (breakpoints.js)1.51KB0.70KB
mobile (createOfflineDataSource.js)5.61KB1.75KB
mobile (index.js)1.55KB0.62KB
mobile (offlineQueue.js)3.91KB1.35KB
mobile (pwa.js)0.97KB0.49KB
mobile (serviceWorker.js)1.48KB0.62KB
mobile (serviceWorkerSource.js)3.41KB1.48KB
mobile (useBreakpoint.js)1.54KB0.65KB
mobile (useGesture.js)6.96KB1.98KB
mobile (useOfflineSync.js)1.99KB0.72KB
mobile (usePullToRefresh.js)2.53KB0.85KB
mobile (useResponsive.js)0.72KB0.42KB
mobile (useResponsiveConfig.js)1.37KB0.63KB
mobile (useSpecGesture.js)4.32KB1.64KB
mobile (useTouchTarget.js)1.01KB0.54KB
permissions (MePermissionsProvider.js)9.53KB3.38KB
permissions (PermissionContext.js)0.31KB0.25KB
permissions (PermissionGuard.js)0.89KB0.45KB
permissions (PermissionProvider.js)4.64KB1.50KB
permissions (evaluator.js)5.12KB1.74KB
permissions (index.js)0.93KB0.41KB
permissions (store.js)0.91KB0.42KB
permissions (useFieldPermissions.js)1.28KB0.53KB
permissions (usePermissions.js)1.93KB0.88KB
plugin-ai (index.js)15.75KB3.80KB
plugin-calendar (index.js)46.85KB12.89KB
plugin-charts (index.js)64.66KB18.32KB
plugin-chatbot (index.js)188.60KB44.82KB
plugin-dashboard (index.js)133.48KB34.49KB
plugin-designer (index.js)212.80KB43.15KB
plugin-detail (index.js)245.29KB62.39KB
plugin-editor (index.js)2.46KB1.10KB
plugin-form (index.js)131.78KB32.19KB
plugin-gantt (index.js)165.16KB40.33KB
plugin-grid (index.js)201.66KB54.58KB
plugin-kanban (index.js)53.11KB14.62KB
plugin-list (index.js)112.74KB27.50KB
plugin-map (index.js)20.09KB6.62KB
plugin-markdown (index.js)13.72KB4.69KB
plugin-report (index.js)43.51KB11.94KB
plugin-timeline (index.js)26.72KB7.71KB
plugin-tree (index.js)9.26KB3.13KB
plugin-view (index.js)84.85KB20.79KB
providers (DataSourceProvider.js)0.75KB0.39KB
providers (MetadataProvider.js)1.37KB0.59KB
providers (ThemeProvider.js)1.90KB0.85KB
providers (UploadProvider.js)11.66KB3.50KB
providers (index.js)0.45KB0.23KB
providers (types.js)0.01KB0.04KB
react-runtime (index.js)5.62KB2.34KB
react (LazyPluginLoader.js)4.47KB1.63KB
react (SchemaRenderer.js)63.21KB21.05KB
react (data-invalidation.js)5.05KB2.08KB
react (index.js)2.44KB1.21KB
react (schema-input.js)2.32KB1.24KB
react (spec-input.js)0.20KB0.18KB
sdui-parser (codegen.js)5.41KB2.34KB
sdui-parser (dashboard-widget-options.js)3.08KB1.30KB
sdui-parser (index.js)4.93KB2.24KB
sdui-parser (input-type.js)2.84KB1.40KB
sdui-parser (parse.js)12.13KB3.65KB
sdui-parser (provenance.js)3.66KB1.82KB
sdui-parser (types.js)0.28KB0.23KB
sdui-parser (validate.js)7.54KB2.63KB
types (ai.js)0.20KB0.17KB
types (api-types.js)0.20KB0.18KB
types (app.js)2.87KB0.99KB
types (base.js)0.20KB0.18KB
types (blocks.js)0.20KB0.18KB
types (complex.js)2.74KB1.41KB
types (crud.js)0.20KB0.18KB
types (dashboard-filter-alias.js)6.23KB2.74KB
types (data-display.js)3.75KB1.85KB
types (data-protocol.js)0.20KB0.19KB
types (data.js)0.20KB0.18KB
types (designer.js)1.85KB0.85KB
types (disclosure.js)0.20KB0.18KB
types (error-code.js)1.54KB0.88KB
types (feedback.js)0.20KB0.18KB
types (field-types.js)0.20KB0.18KB
types (form.js)0.20KB0.18KB
types (http-inflight.js)8.87KB3.73KB
types (http-retry.js)4.32KB2.02KB
types (icon-key-migration.js)4.26KB1.63KB
types (index.js)4.72KB2.24KB
types (layout.js)0.20KB0.18KB
types (managed-by.js)0.19KB0.18KB
types (mobile.js)2.59KB1.31KB
types (navigation.js)0.20KB0.18KB
types (objectql.js)0.20KB0.18KB
types (overlay.js)0.20KB0.18KB
types (permissions.js)0.20KB0.18KB
types (plugin-scope.js)0.20KB0.18KB
types (record-components.js)0.20KB0.19KB
types (record-semantics.js)1.28KB0.67KB
types (registry.js)0.20KB0.18KB
types (reports.js)0.20KB0.18KB
types (spec-report.js)5.05KB1.93KB
types (spec-ui-namespace.js)0.20KB0.19KB
types (system-fields.js)3.33KB1.54KB
types (theme.js)6.28KB2.87KB
types (ui-action.js)3.40KB1.71KB
types (views.js)0.20KB0.18KB
types (widget.js)0.20KB0.18KB

Size Limits

  • ✅ Core packages should be < 50KB gzipped
  • ✅ Component packages should be < 100KB gzipped
  • ⚠️ Plugin packages should be < 150KB gzipped

@os-support-aiClaude

Copy link
Copy Markdown
CollaboratorAuthor

ACCEPT on the diff. ⏸ Landing held until every check is green — zero failures, 10 still running at a31b806b0397.

Both ⛔ exclusions verified from the file list

Three files: the changeset, InlineEditSaveBar.tsx (+39/−3), and the new pin. Nothing under packages/types, and ConcurrentUpdateDialog.tsx is absent — the predicate whose narrowed type PR #6474 made honest was read, not edited, exactly as the dispatch required.

The answer chosen, and why the mechanism is better than what was asked for

The dispatch authorised both outcomes and required the measurement to pick. It picked take the narrowed type, on the grounds that buildConflict reads exactly two things off err and the narrowed type declares both at types ConcurrentUpdateConflict already accepts — so nothing was load-bearing about the any.

What was not asked for is how the coupling was made:

:86typeNarrowedBy<F>=Fextends(arg: unknown)=>arg is infer N ? N : never;
:87typeConcurrentUpdateErrorShape=NarrowedBy<typeofisConcurrentUpdateError>;
:97err: ConcurrentUpdateErrorShape,
:157constbuildConflict=React.useCallback<BuildConflict>(

The parameter type is inferred from the predicate's own signature rather than restated from it, so the two cannot drift apart. Restating the shape would have re-created a smaller version of the very defect this card is about — two declarations of one thing, free to disagree. This closes it structurally instead.

The as Record<string, unknown> | null cast is gone — 0 occurrences, against a control of 5 live reads of currentRecord, so the zero is a real removal and not a pattern that missed. The card predicted exactly that, and predicted the shapes would agree; both held, so there is no disagreement finding to chase.

The ablation that decides it

Type-level claims are the hard case, because a green tsc proves nothing when the subject is that any made tsc silent. Two legs, and the second is the one that matters:

  • A — parameter reverted to any: type-check RED, exit 2, TS2344 at both guards.
  • B — the narrowed type stops covering a read (currentVersion?: string deleted from the predicate): the source file itself went red, TS2339 at InlineEditSaveBar.tsx(171,32) and (183,30). And the decisive sentence: "under err: any both compiled silently."

That is the proof the change converts a silent hole into a compile error, rather than merely being green. The no-rebuild question was established rather than waved past — tsc -p tsconfig.test.json --listFiles names both the pin and the source file as source, never a built .d.ts.

The empty changeset is earned, not assumed

Published surface unchanged, measured: building the package before and after leaves 51 of 52 dist artefacts byte-identical, including dist/index.js, dist/index.umd.cjs and dist/index.d.ts. The single delta is dist/InlineEditSaveBar.d.ts gaining a type-only BuildConflict, which is unreachable because the exports map declares only "." and index.tsx uses explicit named exports rather than export *. That is a far stronger basis for an empty frontmatter than the usual assertion that a change is internal.

The deliberate non-change is the right call and was flagged rather than taken.err?. is now statically dead, but stripping the ?. is the one edit here that would alter emitted JS — and it would cost exactly the provable type-only property that justifies the empty changeset. Leaving it, and saying why, is worth more than the tidiness.

Three corrections offered, all accepted

  1. A stale count with an intact conclusion. The card recorded 4 grep lines; the same grep now returns 18, and the entire delta is fix(plugin-detail): declare code optional in isConcurrentUpdateError's narrowed type #6474's own pin test (14 references). Excluding it: still one definition, one re-export, one import, one call site. Both of the card's positive controls were re-run rather than trusted — useDetailTranslation returns 54, matching the card exactly, and the second still finds the two deliberate siblings outside scope. Correcting a number by showing what caused the delta and re-firing the controls is the whole method.
  2. pnpm --filter <pkg> test was refused by the repo guard for objectui#3378. It refused rather than lying, so nothing was measured on the bad invocation — the correct root-relative form was used throughout.
  3. One combined run was SIGTERM'd (exit 143) after 8m38s queued for the shared verify lock, with the type-check half already green and the tree clean; tests were re-run narrowed on the same head. Lock contention, not a gate failure — and a useful signal about what running five agents at once costs on the shared lock, which this seat will watch.

Generated by Claude Code

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

finding(plugin-detail): InlineEditSaveBar's buildConflict takes err: any, so isConcurrentUpdateError's narrowing reaches no typed consumer

1 participant

@os-support-ai