Phase 1 of #2126. Six hooks move out of `clients/web/src/App.tsx`, taking
its component body from 4,891 to 4,267 lines with no behavior change:
- useThemeToggle — the light/dark toggle
- useTabUiState — the nine per-screen `ui` objects, the active tab,
the pinned-Protocol ids, and the session reset
- useInspectorStores — the twelve state managers as one create/destroy
lifecycle, their `core/react` hooks, and the
pagination-mode selection
- useExportActions — the eleven Clear / Export / Replay handlers
- useProgressToasts — `notifications/progress` toasts
- useTaskToasts — task-status toasts, the cancel confirmation, the
taskId -> progress map, and the in-flight
tool-call taskId ref
Holding the stores in one slot collapses `setupClientForServer`'s and
`onConfirmRemove`'s dependency arrays from twelve entries to two stable
callbacks; `destroyStores` reads the live set through a ref mirror rather
than the state it also writes.
All six hooks are at 100% on all four coverage dimensions, no `v8 ignore`
needed. Also refreshes the now-stale "~4.5k-line" description of App.tsx
in AGENTS.md and the coverage `include` comment.
Signed-off-by: cliffhall <cliff@futurescale.com>
- Move REPLAYABLE_PROTOCOL_METHODS / isReplayableProtocolMethod out of
components/groups/protocolUtils.ts into utils/replayableProtocolMethods.ts.
It is a pure predicate over a string set, so by the project's own lib-vs-utils
rule it never belonged under components/ — and keeping it there was what made
lib/protocolReplay.ts import 'up' into the component graph. Both consumers now
import from utils/; the predicate keeps its own test.
- Give replayProtocolRequest an exported ReplayClient = Pick<InspectorClient, ...>
naming the nine methods it calls, so a caller (and the test) can satisfy the
real contract instead of casting a stub into a whole client.
- Rebuild the protocolReplay fixtures as plain typed MessageEntry values. The
double cast had been hiding a real defect: MessageEntry.timestamp is a Date
and the fixture passed a number, so the assertion confirmed a shape
messagesToLogEntries cannot produce.
- Replace the window casts in authToken.test.ts with Reflect.set /
Reflect.deleteProperty. No 'as unknown as' remains in either test.
- Drop the stale 'floats top-right' sentence on ReAuthBannerBar, which moved
verbatim and arrived contradicting the paragraph below it.
Declined: re-homing ReAuthBannerBar's transform/zIndex as a theme variant.
'styles' is the Styles API rather than an inline style, Mantine exposes neither
property as a prop, and a styling change is out of scope for a phase whose
safety argument is that nothing was edited.
Signed-off-by: cliffhall <cliff@futurescale.com>
Phase 0 of #2126. Lines 239-668 of clients/web/src/App.tsx were already at
module scope — helpers, formatters, constants, and the toast subcomponents,
none of which close over component state — so the whole block moves verbatim
into files. App.tsx drops from 5,303 to 4,892 lines with no behavior change.
Placement follows the lib-vs-utils rule in AGENTS.md: pure computation to
utils/, anything touching the environment to lib/.
EMPTY_SETTINGS and the two loose toast ids deliberately do NOT land in a new
top-level src/constants.ts, which the issue suggested: the web coverage
include is a whitelist naming components/hooks/theme/lib/utils/server, so a
module there would have fallen out of the >=90 gate silently. They go under
utils/ instead.
ReAuthBannerBar is in the block but not in the issue's table — it was added
after the issue was written (#2108). It moves to components/groups/ReAuthBanner/,
beside the banner it wraps.
Every new module is at 100% on all four coverage dimensions. Two v8 ignores,
each justified in place: getAuthToken's typeof-window guard (a happy-dom
inherent path) and replayProtocolRequest's provably-dead default case.
Signed-off-by: cliffhall <cliff@futurescale.com>
Making every pinned field read-only left an optional one unreachable: the
schema is equally happy with or without it, but the form displayed a value the
user could neither send nor edit. An optional `const` is a yes/no rather than
a fixed answer, so it renders as its single choice, clearable — opting in sends
the schema's own typed value, opting out leaves the property absent. A required
one is still displayed read-only, since there is nothing to decide.
Signed-off-by: cliffhall <cliff@futurescale.com>
- `const` constrains a property that is PRESENT; it neither requires the
property nor acts as a default. It is now supplied automatically only where
the schema also requires the field — in `collectSchemaDefaults`,
`applySchemaConstants` and the TUI's initial values alike — so an optional
`dryRun: { const: true }` no longer turns a valid `{}` call into one that
asks the server to do something. A value that IS present is still corrected
to the constant, whatever supplied it, and the field stays read-only.
- Offerability is judged on the base+member merge rather than the raw member,
so `anyOf: [{ required: ["email"] }, { required: ["phone"] }]` over
root-declared properties is offered. It was declined, which left the gate
checking the base alone and accepting `{}` — which that schema rejects.
- The TUI's branch sections keep the schema's own `required` for the seeding
decision while still rendering every branch field non-required, which is what
keeps a static form submittable.
Signed-off-by: cliffhall <cliff@futurescale.com>
A schema may require a property it never declares: `{ type: "object",
required: ["token"] }` is legal and the tool plainly takes an argument.
declaresAnyFields counted only `properties`, so an App tool shaped that way —
including nested under a composition — was reported input-free and
auto-invoked with `{}`.
Signed-off-by: cliffhall <cliff@futurescale.com>
- branchesKey is built from the resolved alternatives themselves, not from
labels and field names: two branches can share both while pinning different
discriminators or typing a field differently, and a reorder of those went
unnoticed — the form then displayed one branch and submitted another.
- InspectorView gains a deep-link test with root-union appArgs: they name the
second branch, and the assertions pin the picker, the read-only
discriminator, THAT branch's default (not the first branch's), and the
absence of the other branch's field. The old shallow spread fails it.
Signed-off-by: cliffhall <cliff@futurescale.com>
The cross-branch agreement check ignored `const`, so two branches pinned to
`1` and `"1"` both stated no `type`, both matched the text `1`, and
the first one's typed constant was sent. Textually indistinguishable constants
of different types are ambiguous, and the raw string the user typed is the
honest answer.
Signed-off-by: cliffhall <cliff@futurescale.com>
- The seeding effect re-applies constants as well as filling absences: an
in-place schema change can move a `const` the user cannot edit, and the
read-only field then displayed the new value while `values` still held the
old one — which is what would be submitted. Ordinary defaults are still only
ever added, so an edited field keeps what the user put there.
- hasMissingIn checks own-property presence: a required argument legally named
`constructor` resolved to the inherited one and read as supplied, enabling
a submit the schema rejects.
- The TUI keeps the declared name as a branch field's fallback title. The
prefix is an internal field NAME, and buildFields labels from its map key —
so the showcase displayed `__b0__address` where the schema says
`address`.
Signed-off-by: cliffhall <cliff@futurescale.com>
`branchIndex` is a position, and a tool refreshed in place keeps its
`resetKey` — so a union reordered or rewritten with the same number of
branches left the index pointing at a different shape than the one the values
describe: the picker showed SMS while Execute submitted the email arguments.
The selection is now re-derived from the values whenever the alternatives
themselves change, keyed by their labels and declared fields.
Signed-off-by: cliffhall <cliff@futurescale.com>
- The seeding effect is keyed by the fixed values themselves as well as the
entity and branch. A tool refreshed in place keeps its `resetKey` and its
branch while its schema changes underneath, so a newly pinned field was
rendered read-only and never seeded — leaving a required one permanently
unsubmittable. The schema object's identity says nothing (callers rebuild it
every render), which is why the key is derived from what has to be seeded.
- branchLabel stringifies every primitive constant. `hasDiscriminator`
accepts distinct booleans and nulls, so a `true`/`false` union was
discriminated perfectly well and still labelled "Option 1"/"Option 2".
Signed-off-by: cliffhall <cliff@futurescale.com>
- A `const` may be an object or an array, with or without a `type`, and
`String({...})` is "[object Object]" — which no CLI argument can equal, so
the only value such a schema accepts never matched. Supplied text is parsed
and compared structurally through the resolver's own comparison (exported as
`sameJsonValue` so the two cannot disagree), for both branch selection and
the substitution.
- The TUI's one-option const control is never marked required: its option may
legitimately be the empty string, which ink-form's required gate can never
accept, so submission would not even reach `decodeFormValues`. The value is
fixed by the schema, and `missingRequiredFields` still validates the call.
Signed-off-by: cliffhall <cliff@futurescale.com>
The carry filter dropped a name whose value was `undefined`, but clearing a
number or JSON field leaves exactly that: the name present with no value, which
is the user's answer rather than an absence. The incoming branch's defaults
then put the field's default back and undid the clear. Own-property presence
alone is now the test.
Signed-off-by: cliffhall <cliff@futurescale.com>
- coercionProperties collapses each declaration before comparing types. A
nullable schema written as an `anyOf` states no top-level `type`, so
`number | null` and `boolean | null` both read as "no type" and counted
as agreeing — and `value=true` then came back as `NaN` through the first
branch's number.
- admitsNull reads an `enum` offering null, under the same sibling conditions
a `const: null` answers to. Without it `{ enum: [null] }` reported that it
rejects null, and a required read-only field spelled that way stayed
permanently invalid.
Signed-off-by: cliffhall <cliff@futurescale.com>
- convertToolParameters defines its result keys rather than assigning them, so
a discriminator legally named `__proto__` reaches the call instead of the
prototype setter. It also collapses a nullable declaration before dispatching,
so `type: ["number","null"]` coerces as a number rather than being sent
as text.
- typeNameOf sorts an array `type`: JSON Schema reads it as a SET, so
`["number","null"]` and `["null","number"]` are one declaration
and must not read as branches disagreeing.
- nullableUnion's non-asserting keyword set covers `$defs`, `$id`,
`$schema`, `$anchor`, `$dynamicAnchor` and `$vocabulary` — they
identify or declare rather than assert. `$ref` stays out: it applies
whatever it points at.
Signed-off-by: cliffhall <cliff@futurescale.com>
Rendering every `const` read-only assumed the caller had already seeded it,
which SchemaForm never did itself: `InlineElicitationRequest` passes its
values straight through and gates submit on hasMissingRequiredFields, so a
required `const` — a root union's discriminator among them — was displayed
and never supplied, leaving Submit disabled with no way to fix it.
The form now reports the fixed values upward once per entity, in an effect (the
parent's `onChange` cannot be called during our render), adding only names
absent from `values` — so a caller that already seeds sees no call at all and
the report cannot loop.
Signed-off-by: cliffhall <cliff@futurescale.com>
One conflict, in SchemaForm's `resetKey` reset: #2138 added the enlarge-caret
Map to clear there while this branch added the union branch selection. Both
belong in it, so the merged block clears the carets and re-derives the branch
from the incoming values.
Signed-off-by: cliffhall <cliff@futurescale.com>
smoke-web-elicitation.mjs told the reader to run
`SMOKE_BROWSER=webkit npm run smoke:web:app` -- the App smoke, not the
elicitation smoke the file documents. Following it exercises a different
path entirely, and it does not fail while doing so: a real smoke runs and
passes, so nothing signals that you never tested what you were reading
about.
This is a direct cost of my own decision two rounds ago to make the two
App headers word-for-word identical so they could not drift. The shared
PROSE should be identical; the example command is the one line that must
not be, and copy-paste does not distinguish them. Both headers now name
their own smoke, and both point at npm run smoke:web:webkit for all three.
Guarded, since the class is invisible at runtime by construction:
run-engine-smokes.test.mjs now asserts no smoke header tells the reader to
run a sibling's command, and that every `npm run` it does name is a real
script. Both mutation-verified -- restoring the exact bug fails the first,
and naming a nonexistent script fails the second.
Addresses the suppressed comment on Copilot review 5031065042 of #2133.
Signed-off-by: cliffhall <cliff@futurescale.com>
Step 1 of the v2.4.0 release: the bump lands on v2/main before the
milestone merge, so v2/main is never left behind main afterwards.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_013hzwzS8UvBsr4yZm7o7sev
Signed-off-by: cliffhall <cliff@futurescale.com>
The README claimed both showcase tools rendered nothing but the Execute Tool
button, which the PR's own screenshots contradict: `echo` carries a root
`message` and rendered that one field, so it was callable with half its
arguments; only `get_weather`, whose fields live entirely on its `oneOf`,
rendered no controls at all. The two now show the two halves deliberately.
Signed-off-by: cliffhall <cliff@futurescale.com>
Two more from review, both about field data the schema controls.
The caret record was a bare object keyed by field name, and field names
come from a server's schema. A field legitimately named `constructor` or
`toString` read back an inherited function rather than undefined, so the
documented end-of-value fallback never fired and that value reached
setSelectionRange. Use a Map, which has no such keys.
JSON Schema counts maxLength in Unicode code points; String.length counts
UTF-16 code units. A field holding one emoji measured 2, so a maxLength:2
field with room for another character was treated as full and refused its
newline. Count code points. The maxLength handed to the DOM input still
carries the HTML attribute's own UTF-16 counting; that predates this and
is left alone.
Signed-off-by: cliffhall <cliff@futurescale.com>
`anyOfAdmitsNull` sent every non-object member to `toBranch` and refused it,
so `{ const: null, anyOf: [true] }` read as non-nullable — a required field
seeded and rendered read-only as `null` and reported missing forever. `true`
is the unconstrained schema and admits every value, `null` among them;
`false` admits none and still does not.
Signed-off-by: cliffhall <cliff@futurescale.com>
- anyOfAdmitsNull recognizes a branch that constrains nothing (`{}`, or
annotations only): it admits every value, null among them, so a field pinned
to `const: null` beside such a union stays submittable. `admitsNull`
cannot say that on its own — an unconstrained schema tells it nothing either
way and it declines rather than guesses — but here the question is narrower.
- convertToolParameters sends a matched `const` as the schema's own typed
value: `kind=2` selects a branch pinned to `const: 2` and was then sent
`"2"`, which that same branch rejects. Only an exact match is
substituted; anything else is the user's input and is left alone.
Signed-off-by: cliffhall <cliff@futurescale.com>
Two faults in the Enter path, both found in review.
The split read `values[fieldName] as string`, a cast over a
Record<string, unknown> fed by whatever a server's schema declares. A
string field holding a non-string default renders as text and then threw
on `.slice`, so one keystroke crashed the panel. Read the control's own
value instead: always a string, and the exact string the selection
offsets index into.
The caret was recorded only when the newline fit, so pressing Enter mid
value in a field at its maxLength enlarged without one and threw the
caret to the end — a second surprise on top of the newline that did not
arrive. Record it on both paths, `start + 1` after an insertion and
`start` otherwise.
Signed-off-by: cliffhall <cliff@futurescale.com>
JSON Schema's boolean form is legal, but only `true` is harmless: it
constrains nothing, while `false` admits no value at all — so a field
declared with it can never be filled, and a required one makes the whole
branch unsatisfiable. Offering it contradicted every other faithfulness check
in the resolver, so such a branch is declined.
Signed-off-by: cliffhall <cliff@futurescale.com>
isComposing covers browsers that fire compositionend after the
committing keydown. WebKit is reported to fire it before, which leaves
isComposing already false and the CJK user back to a stray newline on
every committed word.
Also reject keyCode 229, the pre-isComposing sentinel for a keydown the
IME consumed, which such an event still carries. Deprecated, and kept
anyway: nothing non-deprecated distinguishes that event, and it cannot
swallow a real Enter, which reports 13 — tested both ways so the
sentinel is provably not eating ordinary keystrokes.
The ordering claim is second-hand; the guard is taken on its own merits,
since its downside is bounded at zero and it covers any browser whose
composition events land too late rather than one vendor.
Signed-off-by: cliffhall <cliff@futurescale.com>
Three findings, all from Copilot's suppressed block.
1. The tests never checked that loadBrowser launches the engine it was
asked for. The failure-path stand-in rejects identically for every
engine, so loadBrowser(root, "firefox") could have called
chromium.launch() and still produced a correctly Firefox-labelled
error. The smokes could not catch it either: the Firefox gate runs
after Chromium is already installed, so a mis-dispatched launch would
SUCCEED and report Firefox coverage that never happened.
Added a success-path test with distinct per-engine spies asserting the
requested engine is invoked, that nothing else is, and that its handle
is what comes back. Mutation-verified: hard-coding playwright.chromium
fails it.
2. The launch-failure message told the reader to run
`npx playwright install --with-deps <engine>`, which is wrong now that
the npm scripts run from the repo root rather than clients/web -- I
introduced that when I removed the `cd`. Playwright is pinned in
clients/web, so a bare root-level npx can fetch a different version and
install a browser revision the pinned one still cannot launch. It now
points at `npm run smoke:web:<engine>` first, and scopes the raw
command to clients/web with the reason.
That message named smoke:web:webkit, which did not exist -- only the
chromium and firefox scripts did. Added it, and a test asserting every
supported engine has one, since a remedy nobody can run is worse than
no remedy. Mutation-verified.
3. AGENTS.md said npm run ci "mirrors" the workflow and then, two
sentences later, that it is "a superset, not a mirror". It now leads
with strict superset, which is the topology this PR is documenting.
The README and AGENTS.md remediation wording is corrected the same way as
the runtime message, so a reader gets the same advice wherever they find
it.
Addresses Copilot review 5030668906 on #2133.
Signed-off-by: cliffhall <cliff@futurescale.com>
- selectBranchIndex asks the supplied NAMES first and falls back to a lone
agreeing constant only when they settle nothing. A matching `const` does
not identify a branch while another candidate leaves that property unpinned:
`{ kind: "email", phone: "555" }` agrees with an email branch whose
`address` is missing while satisfying a phone branch outright.
- isOfferable requires every property VALUE to be readable — JSON Schema's
boolean form is fine, a `null` or an array is not a schema at all and the
web form dereferences one on the way to choosing a widget. Such a branch is
declined rather than handed on to crash a tool panel that would otherwise
have rendered.
Signed-off-by: cliffhall <cliff@futurescale.com>
The handler carried four guards, a preventDefault and a state
transition inline in the TextInput's props, which is what AGENTS.md
rules out ("NEVER use inline code; instead extract to functions in the
same file"). Name it handleEnlargeKeyDown and put it beside
enlargeWithNewline, where the comment can enumerate why each condition
is there rather than sitting between two JSX props.
No behavior change.
Signed-off-by: cliffhall <cliff@futurescale.com>
A distinct `const` does not make `oneOf` alternatives mutually exclusive
unless the property carrying it is required: two branches pinning an OPTIONAL
`kind` both match `{}`, so arguments omitting it satisfy more than one
alternative — exactly what `oneOf` forbids. The property must now be required
by the root or by every branch.
Also makes `propertiesOf` total (an unreadable `properties` reads as empty,
with a separate predicate for the one caller that must tell them apart), which
removes a row of unreachable `?? {}` fallbacks, and covers the value/type
comparisons the const checks rest on.
Signed-off-by: cliffhall <cliff@futurescale.com>
Enter appended its newline to the end of the value regardless of where
the caret was, so pressing it in the middle of `abc|def` produced
`abcdef` with a trailing blank line rather than splitting the text, and
a selected range was kept rather than replaced. That is a rewritten
value, not just a misplaced caret, and no editor behaves that way.
Split at the field's own selection and replace any selected range, then
mount the text area with the caret just after the inserted newline —
EnlargedStringField takes an explicit caretAt for this. Pointer
activation passes none and keeps falling back to end-of-value, since a
click carries no meaningful position. The maxLength check now prices the
resulting value, so replacing a selection can free the room the newline
needs.
The two maxLength tests were asserting against a stub onChange, which
froze the value and made "it did not change" pass whatever the component
did. They now run on a seeded stateful harness, with a no-maxLength
control so the constraint is shown doing the work.
Signed-off-by: cliffhall <cliff@futurescale.com>
I claimed ENGINE_SMOKES was the single home for "which smokes are
engine-sensitive". It was not: npm run smoke still hard-coded the same
three, so a fourth entry would have reached the Firefox and on-demand runs
and silently skipped the DEFAULT Chromium run -- which is the one GitHub
CI executes. The exact drift the list was introduced to remove, left in
the one tier that matters most.
npm run smoke now ends in smoke:web:chromium, which goes through the
runner like the other two tiers.
Two things fall out of that:
- The Chromium tier passes its engine as a literal argument, so an
ambient SMOKE_BROWSER can no longer redirect it. Without that,
SMOKE_BROWSER=firefox npm run ci would have run Firefox twice and never
exercised Chromium at all -- a gate silently testing something other
than what it claims.
- run-engine-smokes.test.mjs now asserts that no tier names an individual
smoke, that both gated tiers name their engine explicitly, and that the
on-demand entry point is the one that follows the environment. This is
the durable half: a tier running fewer smokes is invisible at runtime,
because it still passes. Mutation-verified -- restoring the old hard-
coded chain fails the test.
Docs updated to state the wider claim rather than the one that was true
of only two tiers.
Addresses the suppressed comment on Copilot review 5026921914 of #2133.
Signed-off-by: cliffhall <cliff@futurescale.com>
Enter is also how a Japanese, Chinese or Korean input method commits the
candidate being composed. That keystroke means "accept this word", not
"new line", so the shortcut fired on every word such a user finished —
enlarging the field and inserting a stray newline each time.
Guard on event.nativeEvent.isComposing. The test pairs the composing case
with a negative control dispatching the same event with the flag off, so
a test that stopped reaching the handler cannot pass for the wrong reason.
Signed-off-by: cliffhall <cliff@futurescale.com>
- matchesConstants checks own-property presence before reading a supplied
value, so an absent argument legally named `constructor` no longer reads the
inherited one and rules out every branch pinning that name.
- anyOfAdmitsNull asks `admitsNull` about each composition-free branch instead
of testing `type` alone, so a branch spelling its nullability as
`{ const: null }` or `{ nullable: true }` is recognized. Refusing composed
branches is what bounds the recursion.
- Drop a double cast from the malformed-applicator fixture.
Signed-off-by: cliffhall <cliff@futurescale.com>
The App deep link seeded defaults and spread its `appArgs` over them, which
replaces a nested object wholesale: `{ config: { kind: "sms" } }` discarded
the nested SMS branch's own defaults, which the form then displayed while the
submitted arguments omitted them. New seedSchemaValues merges the two per
level, picking each nested branch from the nested values, and the supplied
value still wins wherever the two meet.
Signed-off-by: cliffhall <cliff@futurescale.com>
collectSchemaDefaults recursed only when a nested object declared `properties`
directly, so one keeping its fields on a composition branch was skipped — while
SchemaForm rendered that branch and displayed its read-only discriminator. The
constant was shown and never submitted, and the server rejected the call. The
recursion now gates on declaresAnyFields.
Signed-off-by: cliffhall <cliff@futurescale.com>
- rootUnion reads every composition keyword through `membersOf` and every
`required` through `requiredOf`. These schemas describe the wire and each
member arrives as `unknown`, so `anyOf: {}` or `required: "x"` really
can arrive — and reading them as lists threw, taking all three clients down
rather than declining one malformed tool. Both web and TUI required-field
checks are guarded the same way.
- admitsNull evaluates a sibling `anyOf` beside a `const: null` instead of
refusing outright: the union decides in both directions, and
`{ const: null, anyOf: [{ type: "null" }] }` admits null as plainly as an
all-string union rejects it. Refusing it left a read-only required field
seeded with the only value it accepts and submit permanently disabled.
Signed-off-by: cliffhall <cliff@futurescale.com>
hasMissingRequiredFields treated every empty string as missing, so a required
field the schema pins to `""` — rendered read-only, and seeded with the only
value it accepts — left Execute and Open App permanently disabled. It now
matches the TUI check: an empty string counts as supplied when the property's
own `const` pins it there, and an ordinary blank required string still does
not.
Signed-off-by: cliffhall <cliff@futurescale.com>
- TUI missingRequiredFields treats an empty string as supplied when the
property's own `const` pins it there. The one-option control cannot produce
anything else, so reporting the seeded value as missing made such a branch
permanently uncallable.
- A nested SchemaForm is reset by the branch-aware key too. Two outer
alternatives can both carry the same nested object field, and a nested form
left mounted across an outer switch kept displaying the inner branch chosen
for the other one while the newly seeded values described another shape.
Signed-off-by: cliffhall <cliff@futurescale.com>
