fix(react): UseNavigationOverlayOptions.onRowClick declares the modifier payload it is called with - #9360
Draft
os-tesla wants to merge 3 commits into
Draft
fix(react): UseNavigationOverlayOptions.onRowClick declares the modifier payload it is called with#9360os-tesla wants to merge 3 commits into
os-tesla wants to merge 3 commits into
Conversation
…ier payload it is called with
`useNavigationOverlay` declared the option with one parameter:
onRowClick?: (record: Record<string, unknown>) => void;
and `handleClick`, 126 lines below in the same file, asserted that declaration
away in order to call it with two:
(onRowClick as (r: Record<string, unknown>, e?: HandleClickModifiers) => void)(record, event);
The assertion was the only thing holding the two apart — a declaration that had
lost an argument, not a hook that needed one. The consequence is not a crash: it
is that the modifier payload is invisible on the one line a host reads, so a
host implementing Cmd/Ctrl/middle-click has to discover the second argument from
the implementation and then spell its own parameter optional to stay assignable.
`app-shell`'s `ObjectView` does exactly that at three call sites.
The declaration now names both parameters and the assertion is deleted.
`HandleClickModifiers` is declared and exported in this very file, so naming it
here costs no import and no dependency.
Source-compatible in BOTH directions, measured rather than assumed:
a one-parameter handler is assignable to the widened signature, and a handler
written against the widened signature was already assignable to the narrow one
(its minimum argument count is still one). No consumer changes.
⭐ Which is exactly why the pin is not an assignability assertion: both
spellings satisfy each other, so an `extends` pin is green on the broken tree
and on the repaired one alike. `useNavigationOverlay.onRowClickArity-9357.test.tsx`
uses the two instruments that can separate them — an exact-identity read of
`Parameters<...>` under `tsconfig.test.json`, and a bytes read of the
declaration and the call site off disk — each with controls proving it can fire.
Scope: the hook's own option only. The pass-through props on the view
components that feed it still declare one parameter on their own published
faces; which spelling that family converges on is the open question on
objectui#9357 and is not decided here.
Part of objectui#9357
Claude-Session: https://claude.ai/code/session_01UzHd6hDYatoDn17BuwKxnZ
Co-authored-by: Claude <noreply@anthropic.com>
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
|
Collaborator
Contract reviewReviewed head: Spelling note: generic parameter lists are written in words below ( ① Derived judgments — the accept set and the published faceThe understatement is real, and it is exactly one call site. At the PR head the hook contains one invocation of the option, An optional second parameter is the right spelling, measured on the feeders, not the docblock. Of the nine consumers that feed the option, five call The cast is gone, not survived. The published face names the real type, and that is correct here. The accept set — measured with a reviewer probe, both trees, under the lock. A probe file assigning nine handler shapes to
⇒ The widening narrows the accept set by exactly one class: a handler passed directly to the hook whose second parameter is typed narrower than ② Semver grading against the changeset
Owed edit, blocking: qualify that paragraph. A sufficient form: state that a handler passed directly to ③ Boundary flags and open questions
open_questions:
The code change is the right repair: the arity understatement is real, the cast is deleted rather than relocated, the option names the real payload type where it is nameable, and it agrees with objectui#9356 about the channel. The one defect is a published compatibility sentence that the compiler contradicts for a measured class, in text that ships to consumers and is not correctable after release. FAIL Generated by Claude Code |
Collaborator
ADOPTED — the FAIL at
|
…and pin it
The contract review of this PR verified the repair on every axis and failed it
on one published sentence: the changeset asserted "No caller has to change",
and changeset text ships verbatim into `packages/react/CHANGELOG.md`, where it
cannot be corrected after release.
Measured, that generalisation is false for exactly one class. A handler passed
DIRECTLY to `useNavigationOverlay` whose second parameter is annotated narrower
than `HandleClickModifiers` — React's `MouseEvent` being the shape a host that
discovered the payload from the implementation would write — compiled against
`origin/main` and is refused at this head:
error TS2322: Type '(_record: Record<string, unknown>, _ev?:
ReactMouseEvent) => void' is not assignable to type '(record:
Record<string, unknown>, event?: HandleClickModifiers | undefined) => void'.
… Type 'HandleClickModifiers' is missing the following properties from type
'MouseEvent<Element, MouseEvent>': altKey, buttons, clientX, clientY, and 26
more.
Reproduced in both directions before this edit, same tsconfig, same probe, the
hook file the only variable: refused against this head's declaration (blob
18ddada), accepted against `origin/main`'s (blob 00bb130), with a
one-parameter handler and an exactly-typed handler as controls accepted on both
trees.
Two changes, and nothing else. The changeset's compatibility passage now names
that class, gives the one-line remedy (annotate the parameter
`HandleClickModifiers`, or drop the annotation) and states — without asserting
a count this text cannot re-derive — that no caller inside this repository is
in it. And the pin file's compile-time half gains one `@ts-expect-error` row
holding the boundary, so it is measured rather than described: a directive
whose error stops occurring is itself TS2578, which reds if the option is ever
widened back or the payload respelled `any`.
The code is untouched — the hook, the deleted assertion and every existing pin
assertion stand exactly as reviewed.
Re card objectui#9357.
Claude-Session: https://claude.ai/code/session_01L5xpA5q533BgTTNADibEFt
Co-authored-by: Claude <noreply@anthropic.com>
Sync only, so the checks below run on the tree the merge queue will judge. No conflicts; nothing in the incoming range touches this branch's three files.
Contributor
|
| Step | Outcome |
|---|---|
| Build packages | success |
| Check console performance budget | failure |
Which half objected:
| Eager-closure half | Verdict |
|---|---|
| Aggregate closure ceiling | ✅ pass |
| Per-chunk ceilings | ✅ pass |
| Ceiling sensitivity (headroom) | |
| Ceiling freshness (checkout vs. base branch) | ✅ pass |
⚠️ A broken gauge half is a verdict about the ceiling, not about the bundle: that line has drifted out of range of the regression it exists to catch, or the report behind it cannot be trusted. It does not say anything grew. TheCheck console performance budgetstep log carries the ceiling and the number it was compared against.
Reason: The entry chunk measured 144.3 KB, but the eager-closure half of this gate returned no trustworthy VERDICT: the report could not be read, a ceiling has drifted out of range of the regression it must catch, or (objectui#6245) a ceiling was replaced on the base branch after this checkout was made. The step log says which. This is not a passing budget — and it is not a size regression either.
See the workflow run for details.
📦 Bundle Size Report
| Package | Size | Gzipped |
|---|---|---|
| app-shell (consoleActionDispatch.js) | 0.20KB | 0.19KB |
| app-shell (index.js) | 16.69KB | 6.21KB |
| app-shell (runtime-config.js) | 20.68KB | 7.36KB |
| app-shell (types.js) | 0.01KB | 0.04KB |
| app-shell (urlParams.js) | 10.06KB | 3.86KB |
| auth (ActiveOrganizationStorage.js) | 25.05KB | 9.16KB |
| auth (AuthContext.js) | 0.31KB | 0.24KB |
| auth (AuthGuard.js) | 2.07KB | 1.00KB |
| auth (AuthProvider.js) | 40.18KB | 10.59KB |
| auth (AuthShell.js) | 3.49KB | 1.40KB |
| auth (ForgotPasswordForm.js) | 12.21KB | 3.45KB |
| auth (LoginForm.js) | 18.15KB | 5.39KB |
| auth (PreviewBanner.js) | 0.90KB | 0.50KB |
| auth (RegisterForm.js) | 6.65KB | 2.22KB |
| auth (SocialSignInButtons.js) | 9.61KB | 3.89KB |
| auth (UserMenu.js) | 3.41KB | 1.23KB |
| auth (auth-gate-events.js) | 1.29KB | 0.66KB |
| auth (authStyles.js) | 5.04KB | 1.72KB |
| auth (createAuthClient.js) | 40.21KB | 10.80KB |
| auth (createAuthenticatedFetch.js) | 8.46KB | 3.43KB |
| auth (index.js) | 3.19KB | 1.44KB |
| auth (invitation-status.js) | 1.22KB | 0.70KB |
| auth (org-roles.js) | 6.66KB | 2.78KB |
| auth (phone-identifier.js) | 1.11KB | 0.66KB |
| auth (types.js) | 0.59KB | 0.35KB |
| auth (useAuth.js) | 5.30KB | 1.02KB |
| auth (useWorkspaceAdminStatus.js) | 11.08KB | 4.58KB |
| collaboration (CommentThread.js) | 26.08KB | 7.56KB |
| collaboration (LiveCursors.js) | 3.17KB | 1.27KB |
| collaboration (PresenceAvatars.js) | 6.49KB | 2.64KB |
| collaboration (PresenceProvider.js) | 2.79KB | 1.13KB |
| collaboration (index.js) | 1.68KB | 0.73KB |
| collaboration (useCollaborationTranslation.js) | 6.05KB | 2.52KB |
| collaboration (useCommentSearch.js) | 1.98KB | 0.88KB |
| collaboration (useConflictResolution.js) | 7.75KB | 1.86KB |
| collaboration (useMentionNotifications.js) | 1.81KB | 0.68KB |
| collaboration (usePresence.js) | 6.33KB | 1.84KB |
| collaboration (useRealtimeSubscription.js) | 7.91KB | 2.01KB |
| components (index.js) | 502.02KB | 115.16KB |
| core (index.js) | 8.52KB | 3.41KB |
| create-plugin (index.js) | 27.94KB | 9.51KB |
| data-objectstack (index.js) | 211.58KB | 58.68KB |
| fields (index.js) | 247.89KB | 62.50KB |
| i18n (LocalizationContext.js) | 1.76KB | 0.96KB |
| i18n (builtinAggregateLabels.js) | 0.86KB | 0.49KB |
| i18n (currency.js) | 1.22KB | 0.64KB |
| i18n (fallbackInterpolation.js) | 6.25KB | 2.77KB |
| i18n (i18n.js) | 8.87KB | 3.64KB |
| i18n (index.js) | 5.22KB | 2.26KB |
| i18n (pickLocalized.js) | 9.86KB | 3.95KB |
| i18n (provider.js) | 32.15KB | 10.49KB |
| i18n (useDisplayLocale.js) | 2.85KB | 1.45KB |
| i18n (useObjectLabel.js) | 34.34KB | 9.17KB |
| i18n (useSafeTranslation.js) | 5.60KB | 2.33KB |
| layout (index.js) | 38.83KB | 10.95KB |
| mobile (MobileProvider.js) | 0.92KB | 0.49KB |
| mobile (ResponsiveContainer.js) | 0.94KB | 0.38KB |
| mobile (breakpoints.js) | 1.51KB | 0.70KB |
| mobile (createOfflineDataSource.js) | 5.61KB | 1.75KB |
| mobile (index.js) | 1.99KB | 0.87KB |
| mobile (offlineQueue.js) | 3.91KB | 1.35KB |
| mobile (pwa.js) | 0.97KB | 0.49KB |
| mobile (serviceWorker.js) | 1.48KB | 0.62KB |
| mobile (serviceWorkerSource.js) | 3.41KB | 1.48KB |
| mobile (useBreakpoint.js) | 1.54KB | 0.65KB |
| mobile (useGesture.js) | 6.96KB | 1.98KB |
| mobile (useOfflineSync.js) | 1.99KB | 0.72KB |
| mobile (usePullToRefresh.js) | 2.53KB | 0.85KB |
| mobile (useResponsive.js) | 0.72KB | 0.42KB |
| mobile (useSpecGesture.js) | 4.39KB | 1.66KB |
| mobile (useTouchTarget.js) | 1.01KB | 0.54KB |
| permissions (MePermissionsProvider.js) | 13.52KB | 4.88KB |
| permissions (PermissionContext.js) | 0.31KB | 0.25KB |
| permissions (PermissionGuard.js) | 0.89KB | 0.45KB |
| permissions (PermissionProvider.js) | 6.24KB | 2.16KB |
| permissions (discardProofCache.js) | 1.04KB | 0.55KB |
| permissions (evaluator.js) | 8.39KB | 3.10KB |
| permissions (index.js) | 0.93KB | 0.41KB |
| permissions (store.js) | 0.91KB | 0.42KB |
| permissions (useFieldPermissions.js) | 1.28KB | 0.53KB |
| permissions (usePermissions.js) | 4.83KB | 2.27KB |
| plugin-ai (index.js) | 14.81KB | 3.63KB |
| plugin-calendar (index.js) | 49.25KB | 13.99KB |
| plugin-charts (index.js) | 71.34KB | 19.90KB |
| plugin-chatbot (index.js) | 195.34KB | 46.51KB |
| plugin-dashboard (index.js) | 131.22KB | 34.59KB |
| plugin-designer (index.js) | 215.94KB | 44.33KB |
| plugin-detail (index.js) | 253.46KB | 65.85KB |
| plugin-editor (index.js) | 2.23KB | 1.05KB |
| plugin-form (index.js) | 136.77KB | 34.17KB |
| plugin-gantt (index.js) | 166.95KB | 41.04KB |
| plugin-grid (index.js) | 211.66KB | 57.50KB |
| plugin-kanban (index.js) | 46.00KB | 14.30KB |
| plugin-list (index.js) | 112.58KB | 27.65KB |
| plugin-map (index.js) | 20.64KB | 6.86KB |
| plugin-markdown (index.js) | 13.88KB | 4.80KB |
| plugin-report (index.js) | 43.41KB | 11.93KB |
| plugin-timeline (index.js) | 30.07KB | 8.74KB |
| plugin-tree (index.js) | 9.55KB | 3.32KB |
| plugin-view (index.js) | 84.42KB | 20.79KB |
| providers (DataSourceProvider.js) | 0.75KB | 0.39KB |
| providers (MetadataProvider.js) | 1.37KB | 0.59KB |
| providers (ThemeProvider.js) | 1.90KB | 0.85KB |
| providers (UploadProvider.js) | 11.66KB | 3.50KB |
| providers (index.js) | 0.45KB | 0.23KB |
| providers (types.js) | 0.01KB | 0.04KB |
| react-runtime (index.js) | 5.62KB | 2.34KB |
| react (LazyPluginLoader.js) | 4.47KB | 1.63KB |
| react (SchemaRenderer.js) | 96.00KB | 31.71KB |
| react (data-invalidation.js) | 5.05KB | 2.08KB |
| react (index.js) | 4.63KB | 2.18KB |
| react (schema-input.js) | 4.25KB | 2.04KB |
| react (spec-input.js) | 0.20KB | 0.18KB |
| sdui-parser (codegen.js) | 6.58KB | 2.74KB |
| sdui-parser (dashboard-widget-options.js) | 3.08KB | 1.30KB |
| sdui-parser (index.js) | 5.66KB | 2.50KB |
| sdui-parser (input-type.js) | 2.84KB | 1.40KB |
| sdui-parser (kanban-quick-add.js) | 3.89KB | 1.87KB |
| sdui-parser (parse.js) | 25.28KB | 7.80KB |
| sdui-parser (provenance.js) | 3.66KB | 1.82KB |
| sdui-parser (types.js) | 0.28KB | 0.23KB |
| sdui-parser (validate.js) | 14.82KB | 4.99KB |
| types (ai.js) | 0.20KB | 0.17KB |
| types (api-types.js) | 0.20KB | 0.18KB |
| types (app.js) | 2.87KB | 1.00KB |
| types (base.js) | 0.20KB | 0.18KB |
| types (blocks.js) | 0.20KB | 0.18KB |
| types (complex.js) | 2.93KB | 1.49KB |
| types (crud.js) | 0.20KB | 0.18KB |
| types (dashboard-filter-alias.js) | 6.23KB | 2.74KB |
| types (data-display.js) | 3.75KB | 1.85KB |
| types (data-protocol.js) | 0.20KB | 0.19KB |
| types (data.js) | 0.20KB | 0.18KB |
| types (designer.js) | 1.85KB | 0.85KB |
| types (disclosure.js) | 0.20KB | 0.18KB |
| types (error-code.js) | 1.54KB | 0.88KB |
| types (expression.js) | 0.20KB | 0.18KB |
| types (feedback.js) | 0.20KB | 0.18KB |
| types (field-types.js) | 0.20KB | 0.18KB |
| types (form.js) | 0.20KB | 0.18KB |
| types (http-inflight.js) | 8.87KB | 3.73KB |
| types (http-retry.js) | 4.32KB | 2.02KB |
| types (icon-key-migration.js) | 4.26KB | 1.63KB |
| types (index.js) | 4.74KB | 2.25KB |
| types (layout.js) | 0.20KB | 0.18KB |
| types (managed-by.js) | 0.19KB | 0.18KB |
| types (mobile.js) | 4.73KB | 2.28KB |
| types (navigation.js) | 0.20KB | 0.18KB |
| types (objectql.js) | 0.20KB | 0.18KB |
| types (overlay.js) | 0.20KB | 0.18KB |
| types (permissions.js) | 0.20KB | 0.18KB |
| types (plugin-scope.js) | 0.20KB | 0.18KB |
| types (record-components.js) | 0.20KB | 0.19KB |
| types (record-semantics.js) | 1.28KB | 0.67KB |
| types (registry.js) | 0.20KB | 0.18KB |
| types (reports.js) | 0.20KB | 0.18KB |
| types (select-option.js) | 0.20KB | 0.19KB |
| types (spec-report.js) | 5.05KB | 1.93KB |
| types (spec-ui-namespace.js) | 0.20KB | 0.19KB |
| types (strict-authoring-face.js) | 14.04KB | 5.36KB |
| types (system-fields.js) | 3.33KB | 1.54KB |
| types (theme.js) | 6.28KB | 2.87KB |
| types (ui-action.js) | 8.11KB | 3.32KB |
| types (views.js) | 0.20KB | 0.18KB |
| types (widget.js) | 0.20KB | 0.18KB |
Size Limits
- ✅ Core packages should be < 50KB gzipped
- ✅ Component packages should be < 100KB gzipped
⚠️ Plugin packages should be < 150KB gzipped
Contributor
|
os-dev-report {
"issue": 9357,
"pr": "https://github.com/objectstack-ai/objectui/pull/9360",
"status": "done",
"branch": "claude/issue-9357-onrowclick-arity",
"new_head_sha": "e0c5c14b67f9b3ed4da2cc8362b6e19a94255aee",
"content_commit": "d8325fa79 (the one commit; e0c5c14b6 is the origin/main merge, no content of its own)",
"premise_still_valid": true,
"summary": "Exactly the three things ordered, in one commit, plus the ordered merge of origin/main (b67b53bc0) as a merge commit in the same push. (1) The changeset's unconditional \"Not breaking / No caller has to change\" passage is replaced by one that names the refused class, shows it as a code example, gives the one-line remedy, and states -- as measured and true -- that no caller INSIDE this repository is affected. (2) One @ts-expect-error row was added to the pin file's compile-time half, holding that boundary. (3) Nothing else: the hook, the deleted cast and every pre-existing pin assertion are byte-identical to the reviewed head (blob 18ddadad7fecd2e38341060c0bf1e0251e4ca1d5 for the hook, unchanged from 02b60d8ec). PR left draft; no ready flip, no auto-merge, no label touched, PR body not edited.",
"changeset_wording_landed": {
"file": ".changeset/9357-navigation-overlay-onrowclick-arity.md",
"frontmatter_unchanged": "'@object-ui/react': minor",
"replaced_paragraph": "**Not breaking, in either direction.** A one-parameter handler stays assignable to the widened signature (its extra parameter is optional), and a handler written against the widened signature was already assignable to the old one -- measured on this change, both directions. No caller has to change; what changes is that a caller who wants the modifier payload can now see, from the published type, that it is there.",
"landed_text_verbatim": "**Source-compatible in both directions, with one measured exception.** A\none-parameter handler stays assignable to the widened signature (its extra\nparameter is optional), and a handler written against the widened signature was\nalready assignable to the old one -- measured on this change, both directions.\n\n**The exception, and the one class that has to change.** A handler passed\n*directly* to `useNavigationOverlay` whose second parameter is annotated\n*narrower* than `HandleClickModifiers` no longer type-checks. React's\n`MouseEvent` is the shape this hits in practice, because until now the payload\nwas only discoverable from the implementation, so a host that wanted it wrote\nthe annotation it saw arrive:\n\n(a ts fence follows here in the file: a useNavigationOverlay call whose onRowClick is written `(record, ev?: React.MouseEvent) => { }`, with an inline comment reading \"was accepted; now TS2322 -- HandleClickModifiers is not assignable to React.MouseEvent\")\n\nIt compiled before only because the old declaration had no second parameter to\ncheck the annotation against. The parameter is checked contravariantly, so the\nannotation now has to *admit* `HandleClickModifiers`. **The fix is one line at\nthe call site:** annotate the parameter `HandleClickModifiers` (exported from\n`@object-ui/react`), or drop the annotation and let it be inferred. Either way\nthe handler keeps receiving exactly what it received before -- this is a\ntype-level change only, with no runtime behaviour attached.\n\nNo caller *inside this repository* is in that class, and the repository's own\ntype-check re-derives that on every run rather than this sentence asserting it;\na host that reaches the hook through a view component's `onRowClick` prop is\nunaffected either way, because the prop's own declared type is what gets\nassigned to the option. The boundary is pinned as a `@ts-expect-error` row in\nthis package's `useNavigationOverlay.onRowClickArity-9357` test, so it cannot\nmove without a red check.\n\nWhat the widening buys everyone else: a caller who wants the modifier payload\ncan now see, from the published type, that it is there.",
"note": "The count of in-repo feeders is deliberately NOT written down (AGENTS.md #9): the sentence points at the repository's own type-check as the instrument that re-derives it. The opening and closing paragraphs of the changeset, and the scope note, are unchanged."
},
"pin_row_landed": {
"file": "packages/react/src/hooks/__tests__/useNavigationOverlay.onRowClickArity-9357.test.tsx",
"half": "compile-time (the half tsconfig.test.json executes; vitest erases it)",
"lines_150_to_152_verbatim": [
"type NarrowerSecondParam = (record: RECORD, event?: ReactMouseEvent) => void;",
"// @ts-expect-error a second parameter narrower than `HandleClickModifiers` is refused (TS2322)",
"type _NarrowerSecondParamIsRefused = Expect of (NarrowerSecondParam extends OnRowClick ? true : false)"
],
"spelling_note": "RECORD stands for the record type (Record of string to unknown) and 'Expect of X' for Expect applied to X: GitHub's body sanitiser deletes angle-bracket-shaped spans, code fences included, so generics are written in words here. The file on disk carries the real angle brackets.",
"support_line": "One type-only import added at the top of the same file: import type { MouseEvent as ReactMouseEvent } from 'react'.",
"why_this_form_and_not_the_reviewer_s_assignment_verbatim": "The reviewer's probe was a value assignment (a const of type OnRowClick). Both forms were measured side by side in a throwaway probe before anything was edited, and BOTH fire at the head and BOTH red as TS2578 against origin/main's hook -- they exercise the same assignability relation. The type-level form was landed because the file's own top docblock and its section header both assert that this half is 'erased at runtime' / 'vitest erases every line of it'. A const declaration would have falsified those two published sentences, and repairing them would have been a third change this order does not authorise. The landed form is also the idiom its immediate neighbours (_NarrowIsAssignableToWide, _WideIsAssignableToNarrow) already use. The TS2322 diagnostic the reviewer measured is quoted verbatim in the row's docblock, so the host-visible error is on the record."
},
"reproduction_of_the_refusal_both_directions": {
"method": "One probe file under packages/react/src/hooks/__tests__/ plus a tsconfig extending packages/react/tsconfig.test.json and naming only that file, so the reading is not mixed with the existing pin's output. Same tsconfig on both legs; the hook file was the only variable, swapped by commit sha (never by a moving ref name). Dependency dist was built first (tsconfig.test.json sets paths to empty, so the @object-ui packages resolve through dist); the hook itself is a RELATIVE import from the test file, so no rebuild is needed between legs. Both probe files were deleted afterwards and git status is clean.",
"control_that_the_two_trees_differ": "hook blob at PR head 02b60d8ec = 18ddadad7fecd2e38341060c0bf1e0251e4ca1d5; at the PR base 2e471dc0a AND at origin/main b67b53bc0 = 00bb1307c972dbb27a64476b6594ae2e2e651cd7 (byte-identical base and main, so 'compiles on main' and 'compiled on the base' are the same reading). packages/react/tsconfig.test.json is byte-identical across all three (aa43dc225). On the mutated tree the one-parameter spelling reads 1 occurrence and the assertion 'onRowClick as' reads 1; at the head they read 0.",
"leg_head_refused": {
"tsc_exit": 2,
"diagnostic_verbatim_generics_in_words": "packages/react/src/hooks/__tests__/probe-9357.ts(7,14): error TS2322: Type '(_record: RECORD, _ev?: ReactMouseEvent) => void' is not assignable to type '(record: RECORD, event?: HandleClickModifiers | undefined) => void'.\n Types of parameters '_ev' and 'event' are incompatible.\n Type 'HandleClickModifiers | undefined' is not assignable to type 'MouseEvent of Element | undefined'.\n Type 'HandleClickModifiers' is missing the following properties from type 'MouseEvent of Element': altKey, buttons, clientX, clientY, and 26 more.",
"note": "Matches the reviewer's quoted diagnostic exactly, apart from the probe's own identifier names."
},
"leg_main_accepted": {
"tsc_exit": 0,
"output": "(empty -- the same narrower-annotation handler compiles against origin/main's hook)"
},
"controls_on_both_legs": "A one-parameter handler and a handler annotated exactly HandleClickModifiers were in the same probe file and were ACCEPTED on both trees, so the head leg's single error is the narrowing and not a broken probe.",
"restore_proof": "Mutation and restore under a trap on EXIT INT TERM with absolute paths; restore is 'git checkout HEAD -- PATH' (never a bare checkout), verified by blob-hash equality against the HEAD blob AND an empty 'git diff HEAD', never by an exit code. Both legs restored to 18ddadad7fecd2e38341060c0bf1e0251e4ca1d5."
},
"ablation_of_the_landed_pin_row": {
"run_from": "the committed merged tree, HEAD = e0c5c14b6, so the restore leg has a real restore point",
"command": "pnpm exec tsc -p packages/react/tsconfig.test.json",
"baseline_exit": 0,
"mutation": "git checkout b67b53bc0 -- packages/react/src/hooks/useNavigationOverlay.ts (origin/main's one-parameter declaration)",
"on_disk_proof_before_reading_anything": "blob on disk after the swap = 00bb1307c972dbb27a64476b6594ae2e2e651cd7; injected one-parameter spelling counted with 'grep -o ... | wc -l' = 1; deleted two-parameter spelling = 0. (grep -c counts LINES, so occurrences were counted with grep -o piped to wc -l.)",
"ablated_exit": 2,
"ablated_errors": [
"...onRowClickArity-9357.test.tsx(89,31): error TS2344: Type 'false' does not satisfy the constraint 'true'.",
"...onRowClickArity-9357.test.tsx(92,3): error TS2344: Type 'false' does not satisfy the constraint 'true'.",
"...onRowClickArity-9357.test.tsx(92,26): error TS2493: Tuple type '[record: RECORD]' of length '1' has no element at index '1'.",
"...onRowClickArity-9357.test.tsx(101,3): error TS2344: Type 'false' does not satisfy the constraint 'true'.",
"...onRowClickArity-9357.test.tsx(117,43): error TS2344: Type 'false' does not satisfy the constraint 'true'.",
"...onRowClickArity-9357.test.tsx(151,1): error TS2578: Unused '@ts-expect-error' directive. [THE NEW ROW]"
],
"reading": "The first five are the pre-existing pin's red-first errors, reproduced at +1 line from the reviewer's control leg (88/91/91/100/116 became 89/92/92/101/117) because one import line was added above them -- an independent control that the existing assertions were not disturbed. The sixth, at line 151 column 1, is the new directive and is the ablation result: the row fires when the boundary exists and reds loudly when it does not.",
"restore": "blob back to 18ddadad7...; 'git diff HEAD' empty; restored reading exit 0."
},
"checks": {
"all_run_on": "the merged tree e0c5c14b6 unless marked (pre-merge)",
"locked_runs_verdict_lines_quoted": [
"VERDICT command-exit 0 - held the lock 32s - waited 0s => (pre-merge) pnpm --workspace-concurrency=2 --filter '@object-ui/react^...' build",
"VERDICT command-exit 0 - held the lock 15s - waited 0s => (pre-merge, post-edit) pnpm --filter @object-ui/react type-check (tsc --noEmit && tsc -p tsconfig.test.json)",
"VERDICT command-exit 0 - held the lock 385s (6m25s) - waited 0s => pnpm exec turbo run build --concurrency=2 : Tasks: 44 successful, 44 total",
"VERDICT command-exit 0 - held the lock 371s (6m11s) - waited 0s => pnpm exec turbo run type-check --concurrency=2 : Tasks: 81 successful, 81 total (this is the run that executes packages/react's tsconfig.test.json, i.e. the pin)",
"VERDICT command-exit 0 - held the lock 54s - waited 0s => pnpm exec vitest run packages/react/ : Test Files 84 passed (84), Tests 995 passed (995)",
"VERDICT command-exit 0 - held the lock 1088s (18m08s) - waited 0s => pnpm test --shard=1/4 : Test Files 770 passed | 1 skipped (771), Tests 10455 passed | 2 skipped (10457)",
"VERDICT queue-timeout (exit 99) - never acquired - waited 540s (9m00s) => pnpm test --shard=2/4 : NOT MEASURED",
"VERDICT queue-timeout (exit 99) - never acquired - waited 540s (9m00s) - holder pid 12379, held 263s (a run in objectui-review-9343-merged) => vitest run scripts/ packages/types/ packages/cli/, attempt 1 : NOT MEASURED",
"VERDICT queue-timeout (exit 99) - never acquired - waited 540s (9m00s) - holder pid 12379, held 893s (same holder) => same command, attempt 2 on the resumed slot : NOT MEASURED"
],
"lock_slot": "OS_VERIFY_LOCK_SLOT=objectui-9360, set before the first attempt and resumed on every retry.",
"unlocked_runs_exit_captured_by_redirect_before_any_pipe": [
"pnpm run type-check:scripts exit 0 (the leg the dispatch warns is skipped because scripts/ is not a workspace package)",
"pnpm run type-check:vitest-config exit 0",
"pnpm run type-check:vitest-setup exit 0",
"pnpm run type-check:e2e exit 0",
"pnpm exec turbo run lint --concurrency=2 exit 0 : Tasks: 47 successful, 47 total; 0 errors in every package (warnings only, all pre-existing)",
"pnpm --filter @object-ui/react lint exit 0 : 356 problems (0 errors, 356 warnings)",
"pnpm exec eslint THE-EDITED-TEST-FILE exit 0, no output",
"node scripts/check-changeset-presence.mjs exit 0",
"node scripts/check-changeset-no-major.mjs exit 0",
"node scripts/check-changeset-fixed.mjs exit 0",
"node scripts/check-changeset-overwrite.mjs exit 0",
"node scripts/check-changeset-claims.mjs exit 0",
"node scripts/check-control-bytes.mjs exit 0",
"node scripts/check-comment-mask-corpus.mjs exit 0",
"pnpm run check:new-line-citations exit 0 : 'VERDICT new-cross-file-line-citations: 0 new citation(s), enforcement report-only -> exit 0'",
"node scripts/check-test-path-roots.mjs exit 0",
"node scripts/check-type-check-coverage.mjs exit 0",
"pnpm run check:doc-snippets exit 0 : 'Semantic phase: 649 of 649 block(s) judged, 0 failed.'",
"pnpm run check:skill-examples exit 0 : 'Semantic phase: 14 of 14 ts fence(s) judged, 0 failed.'",
"node scripts/check-governed-queue-guard.mjs --test THE-TWO-CHANGED-PATHS exit 0 : 'NOT GOVERNED -- 2 path(s) checked against 5 governed surface(s); none matched.'",
"pnpm exec vitest run THE-27-CHANGESET-READING-TEST-FILES exit 0 : Test Files 27 passed (27), Tests 960 passed (960)",
"direct control-byte scan over both changed files with grep -naP over the C0/DEL class: 0 hits"
],
"two_previously_red_checks_are_green_on_the_merged_tree": "Doc Snippet Type Check and Skill Example Check were red on 02b60d8ec and the reviewer judged them stale/inherited from main. Both gates now exit 0 locally on the merged tree (figures above), so the merge is expected to clear them on CI as well.",
"narrowings_declared": [
"pnpm test shards 2/4, 3/4 and 4/4 were NOT MEASURED. Shard 2/4 returned exit 99; shard 3/4 sat in the queue 683s behind another seat's run and I stopped it (by recorded pid, never by process name) rather than starve two other queued seats further; shard 4/4 never started. Substituted, and the substitution is argued from reach rather than assumed: my diff is two files. (a) The test file lives in packages/react and nothing imports it -- 'git grep -l onRowClickArity-9357' over every test file in the tree returns 0 hits outside itself -- and the WHOLE packages/react suite ran green (84 files / 995 tests). (b) The changeset file -- 'git grep -l 9357-navigation-overlay' over the same corpus returns 0 hits, so nothing names it; the tests that read the .changeset DIRECTORY at all number 27, and all 27 ran green. On top of that, shard 1/4 of the full suite ran green (771 files), and the FULL build farm (44/44), the FULL type-check farm (81/81) and the FULL lint farm (47/47) all ran green on the merged tree. CI still runs all four shards.",
"The e2e / live-e2e and performance-budget workflows were not run locally at all; they are CI's."
]
},
"measured_false_in_this_order_or_in_my_own_method": [
"Nothing in the order was measured false. The dispatch's compiler fact reproduced exactly, in both directions, with the diagnostic matching the reviewer's record; the blast radius inside the tree is nil as stated; packages/react's type-check really is 'tsc --noEmit && tsc -p tsconfig.test.json' and the second leg really is the one that reads the pin.",
"One refinement, not a contradiction: the dispatch says tsconfig.test.json resolves the @object-ui packages through dist (paths set to empty), so build what it needs first. True, and I did build the dependency closure first -- but the hook under test is reached by a RELATIVE import from the test file, so the two reproduction legs did not need a rebuild between them. Both facts matter: without the dist build the project does not compile at all; with it, swapping the hook source is immediately visible.",
"A method attempt of MINE that measured false: 'pnpm exec vitest list --filesOnly --shard=N/4' IGNORES --shard -- all four values printed the identical 3084-line list. So I could not determine which shard holds the pin file, and I do not claim it. (--shard does work on 'vitest run': shard 1/4 ran 771 of 3084 files.) Reported because I nearly used that output as evidence of coverage.",
"Template conflict, reported rather than resolved: this session's injected attribution template asks for a commit trailer naming a model. The order forbids a model identifier in any repository artifact, and so does my standing contract. I followed the order and the branch's existing convention -- 'Co-authored-by: Claude' with the noreply address, plus 'Claude-Session:' -- and flag the conflict here instead of resolving it with a model name."
],
"mcp_calls": "0 -- every GitHub read and this write went through repo-scoped REST (probed first: GET on the repo returned 200); no MCP GitHub tool was called.",
"open_questions": [],
"out_of_scope_findings": [
"noted, not filed: the PR BODY of #9360 still carries the same unconditional generalisation the changeset carried -- its 'Source compatibility' section ends 'No consumer breaks in either direction, which is why no consumer is edited here.' The order is 'exactly three things ... nothing else', and a body edit is also the one operation GitHub is known to mutate (it appends a second attribution footer and downgrades the session-URL form), so I did not touch it. Successor: the light re-review named in the adoption comment, or the PM, can decide whether the body should be qualified too. Flagged here because the body is what the re-reviewer reads.",
"noted, not filed: nothing else was found. The hook, the deleted cast and the existing pin assertions were read and left byte-identical, as ordered."
],
"landing": "Untouched by me, as ordered: PR #9360 is still draft, auto-merge is null, labels are still exactly 'package: react' and 'tests'. Landing is the PM's."
}Generated by Claude Code |
Contributor
|
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
3 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.
Part of objectui#9357
The defect, re-verified against
origin/mainbefore any editpackages/react/src/hooks/useNavigationOverlay.ts, both halves byte-for-byte, at2e471dc0aa23ff807aa9960618c419ecccdfdcef:premise_still_valid: true— all four lines are exactly where the card says,126 lines apart in one file. The declaration promises one parameter; the call
site asserts the declaration away to pass two. The assertion is the only thing
holding the two apart.
One correction to the dispatch note:
HandleClickModifiersis not importedinto this file — it is declared and exported from this very file, fourteen
lines below the option. Naming it in the option therefore costs no import at
all, which is a stronger version of the same point.
The repair
UseNavigationOverlayOptions.onRowClicknow declares both parameters:the record, and
event?: HandleClickModifiers.handleClicknow callsonRowClick(record, event)straight through the declaration.Consumers, enumerated — and which ones the repair reaches
Thirteen sites call
useNavigationOverlay; nine of them feed the repairedoption. The repair reaches the option itself, so every one of these nine may
now pass a two-parameter handler without an assertion of its own:
packages/plugin-grid/src/ObjectGrid.tsxonRowClickpackages/plugin-list/src/ListView.tsxonRowClickpackages/plugin-list/src/ObjectGallery.tsxprops.onRowClickelseprops.onCardClickpackages/plugin-kanban/src/ObjectKanban.tsxexternalClickpackages/plugin-calendar/src/ObjectCalendar.tsxonRowClickwhen not an overlaypackages/plugin-gantt/src/ObjectGantt.tsxonRowClickwhen not an overlaypackages/plugin-map/src/ObjectMap.tsxonRowClickpackages/plugin-timeline/src/ObjectTimeline.tsxonRowClickelseonItemClickpackages/plugin-tree/src/ObjectTree.tsxonRowClickFour call the hook without the option and are untouched either way:
app-shell'sInterfaceListPage.tsx,ObjectDataPage.tsxandObjectView.tsx, plus the doc example inpackages/components/src/custom/navigation-overlay.tsx.What the repair does NOT reach — each is a published face of its own, and
naming the payload on it is the ruling objectui#9357 leaves open:
ObjectKanbanComponentProps.onRowClickand.onCardClick(plugin-kanban)KanbanRendererProps.schema.onCardClick(plugin-kanban/src/index.tsx)ObjectGallery'sonCardClick/onRowClickpair (plugin-list)onRowClickprop onObjectGrid,ListView,ObjectCalendar,ObjectGantt,ObjectMap,ObjectTimeline,ObjectTreeObjectGridSchema.onRowClickandObjectKanbanSchema.onCardClickin@object-ui/types, where the card records thatHandleClickModifiersisunreachable (phantom dependency plus a cycle) and
event?: anyis theestablished spelling
The three workaround call sites the card names —
app-shell'sObjectView.tsxat lines 2734, 3120 and 3163, each spelling
(record: any, event?: OPT any)—are consumers of those component props, not of the hook option, so they
still need the workaround and are deliberately left alone.
Source compatibility — the prediction, measured here rather than inherited
Both directions hold, and both are now asserted in the pin so they cannot
silently stop holding:
(
_NarrowIsAssignableToWide);one-parameter spelling (
_WideIsAssignableToNarrow) — its minimum argumentcount is still one.
⇒ the prediction is not falsified. No consumer breaks in either direction,
which is why no consumer is edited here.
Why the pin is not an assignability assertion — with the null result to prove it
Because both directions hold,
extendscannot tell the repaired declarationfrom the broken one. Two instruments that can are used instead, each with
controls proving it can fire:
(
Parametersand itslength), run only bypackages/react'stsconfig.test.json;the test file rather than the cwd.
Red-first, on the unmodified tree, verbatim
pnpm --filter @object-ui/react type-check— exit 2:(the TS2493 message's own generic is respelled in words for the reason at the
top; everything else is verbatim.)
pnpm exec vitest runon the pin — exit 1, the four assertions red and theseven controls green:
Ablation — two legs, on-disk proof before any result was read
Mechanism: mutate,
grep -cthe injected text and the deleted text beforereading anything, restore with
git checkout HEAD -- PATHundertrap ... EXIT INT TERM, and verify the restore by blob-hash equality againstthe HEAD blob plus an empty
git diff HEAD— never by an exit code. Bothrestores verified:
18ddadad7fecd2e38341060c0bf1e0251e4ca1d5, diff empty.Leg 1 — the full pre-fix defect put back (narrow declaration + assertion).
tsc --noEmitoversrc: exit 0. ⭐ The defect compiled cleanly. That isthe disease in one number: the assertion made the disagreement legal, so no
type instrument in the tree ever objected.
tsc -p tsconfig.test.json: exit 2 — the five errors above.4 failed | 7 passed.Leg 2 — the NULL RESULT the dispatch predicted, and it is a null result.
In Leg 1's error list the two assignability assertions, at lines 108 and 109,
are absent in both directions: they stayed green while the declaration was
narrowed back. So did
_FirstParamIsTheRecordat line 89, correctly — the firstparameter is identical in both spellings. Reported, not deleted: it is the
measurement that justifies the instrument choice.
Leg 3 — widened declaration kept, the assertion alone put back.
tsc --noEmit: exit 0.tsc -p tsconfig.test.json: exit 0.2 failed | 9 passed, onlyapplies no type assertion to onRowClickandcalls it with both arguments, directly.⇒ the type system is completely blind to a returning assertion once the
declaration is honest. The bytes half is the only thing in this repository that
reds on it, which is why it is in the pin.
Runtime behaviour is unchanged, observed rather than argued. The two runtime
control tests — the hook forwards
(record, event)to the supplied handler, anda one-parameter handler is still called — are green on the broken tree (they are
among the seven that passed in the red-first run and in Leg 1) and green on the
repaired one. A
tsctype assertion is erased at emit, so the call site'ssemantics could not move; this is the measurement that says so.
Dependent-set membership read
Read from the workspace graph, not inferred:
@object-ui/react— 32 workspace packagestransitively depend on it (28 directly). 31 declare a
type-checkscript;@object-ui/example-hello-worlddeclares none..changeset/config.jsonignore—@object-ui/example-*,@object-ui/site,@object-ui/test-support. This excludes packages fromversion bumping and is a different set from the one above. Five members
sit in both (
@object-ui/siteand the fourexample-*packages) and@object-ui/test-supportsits only inignore— so the overlap is partialand neither list may be read off the other.
fixedgroup — one group of 40 packages,@object-ui/reactamongthem.
majoris unavailable by repo rule; the changeset declaresminor.Verification
Every heavy run went through
../objectstack/scripts/pm/os-verify-lock.sh --on a stable slot,
os-dev-9357. VERDICT lines, quoted, never a bare exit code:idle a third time, the remaining work was narrowed, and the narrowing is
declared and measured — not assumed:
The narrowing, and the proof it excludes nothing. The published
.d.tssurface of
@object-ui/reactwas hashed file-by-file before and after therebuild: of 65 emitted declaration files, exactly one moved, and inside it
exactly one declaration line changed (the rest of that hunk is doc comment). So
the only packages whose type-check can move are the ones that read
UseNavigationOverlayOptions. The population was measured, not guessed: twelvepackages outside
packages/reactnameuseNavigationOverlay,UseNavigationOverlayOptionsorHandleClickModifiersanywhere in theirsources. All twelve plus
@object-ui/reactwere type-checked.Unlocked runs — exit codes captured after redirecting to a file, never through a
pipe:
pnpm --filter @object-ui/react builddist completeness: 1 package(s) complete (130 emitted files verified)type-checkover the 13 affected packages,--workspace-concurrency=2app-shellincludedvitest run packages/react/Test Files 83 passed (83),Tests 985 passed (985)vitest run packages/plugin-kanban/ packages/plugin-list/Test Files 130 passed (130),Tests 1263 passed (1263)pnpm --filter @object-ui/react lintGates derived by hand from the root
package.jsonand.github/workflows/(there is no dispatch-gates script in this repository) — all exit 0:
check:control-bytes,check:test-path-roots,check:new-line-citations,check:doc-example-readers,check:handler-key-reads,check:changeset-claims,check:phantom-deps,check:published-tsconfig-exclude,check:comment-mask-corpus,check:doc-examples,check:unreferenced-sources,changeset:check. A directcontrol-byte scan over the three touched files found none.
NOT MEASURED locally, and left to CI rather than claimed: a full
pnpm buildon the post-fix tree (the one that ran green was on the unmodified tree), the
four-shard
pnpm test, the nineteen packages of the dependent set that thedeclaration diff proves cannot be affected,
@object-ui/site's type-check(it needs
next typegen), and the repo-widepnpm lint.Inherited reds — NOT from this change
maincarries two red checks that every PR built on it inherits:Doc Snippet Type Check and Skill Example Check. They arrived with
objectui#9310 and their repairs are with the maintainer (objectui#9352 and
objectui#9308). This branch touches neither
content/docs/**norskills/**and makes no attempt at them.
Acceptance notes
app-shell/src/views/ObjectView.tsxaround line 2191declares an inline structural duplicate of
HandleClickModifiers(
OPT metaKey,OPT ctrlKey,OPT button) rather than importing theexported interface. Cosmetic, one file, and it belongs to whichever PR takes
the component-prop half of objectui#9357 — successor: that PR.
arity in exactly the way this card describes. They are not a separate finding
— objectui#9357's own "What is not decided here" section already carries them,
and picking their spelling is a ruling.
Disjointness against the in-flight PRs
File lists read for objectui#9356, #9343, #9144, #9339, #9351 and #9352: no
path overlap with this branch's three files. Beyond the file faces, the nearest
PR (#9356) asserts on
plugin-kanbanand@object-ui/typessymbols; thisbranch removes only one span of text, the assertion inside
useNavigationOverlay.ts, and a repo-wide search finds no test outside thisbranch that names
UseNavigationOverlayOptionsor reads that span. Theplugin-kanbanandplugin-listsuites were run on this branch and are green.Status
⛔ Draft on purpose. The seat does not flip ready, arm auto-merge or enqueue —
the PM's.
needs:contract-reviewis hung on this PR because the change moves apublished type in
@object-ui/react.Session for this work, as prose so an edit cannot strip it:
https://claude.ai/code/session_01UzHd6hDYatoDn17BuwKxnZGenerated by Claude Code