test(guards): scan wrapper buttons for dead affordances and name the primary-action inventory - #2663
Conversation
PaperHLBtn, TdButton and TdIconButton each render a native <button> root and
declare click as an emit, so the component file passes the native-button scan
while an unbound call site ships an enabled control that does nothing. Nothing
scanned the ~150 call sites, and ESLint's click-events-have-key-events cannot
fire on markup with no click handler at all.
findDeadWrapperButtons reports a call site unless it binds a click, uses v-on
object syntax, carries a static type=submit with a proven form owner, is
permanently disabled, or is a marked visual specimen. Form ownership reuses
formIds and hasFormOwner rather than restating the proof.
The submit clause is deliberately not a copy of the native one: all three
components default the type prop to 'button', so an untyped wrapper inside a
form submits nothing, unlike a native button.
directiveOnlyMarkup now keeps a bare v-on alongside v-on:x so object syntax is
read through the same attribute-boundary parse that rejects an inert
data-note='v-on="{ click: run }"'. No existing finding changes.
The repo-wide assertion is red at this commit with the seven style-guide
specimens the next commit marks.
The wrapper scan's repo-wide red output named exactly seven offenders, all in PaperStyleGuideView.vue: the five PaperHLBtn variant specimens and the two opposite-theme preview specimens. Both rows exist to show what a variant looks like, so there is no intended handler to restore and no product control is affected. They carry the same narrow data-dead-affordance-exempt marker the four native .pbtn specimens in the same view already use. No product offender was reported, so nothing outside the style guide changed. The attribute lands on PaperHLBtn's rendered <button> root as an ordinary fallthrough data attribute; there is no behaviour change.
… preconditions The registry proved two controls and said nothing about its own scope, so a reader could not tell whether an absent control was safe or merely unexamined. The header now defines a primary action as one whose activation writes or enqueues, lists every covered surface with the preconditions each can be blocked by, and names what is deliberately out: PaperReviewView's approve, reject and execute while GH-2629 holds those files, the native form submits, and every other writing control. Six registrations back the inventory: PaperTriageRowEdit save - empty, unchanged, busyElsewhere (its three saveBlock states, so a change that keeps the button enabled for one cannot pass by satisfying the others). PaperCaptureNib submit - empty draft. PaperTriageTable accept-on-board - loadFailed and viewOnly, joining the noBoard registration already here. Each keeps the branch-pinning disabled assertion and adds a data-reason assertion, so a test cannot silently drift onto a different blocked state: saveBlock reports busyElsewhere before empty, and boardPickBlock reports noBoard before viewOnly. The viewOnly path models write access being revoked on an already picked board, because the picker disables a read-only option and the state is otherwise unreachable from the UI. No production file changes.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Review record (alpha product-trust lane, review-and-ship round 1 at head b3484f0). Reviewer: one fresh-context independent reviewer subagent (read-only), input = merge-base..head diff plus the worktree at the head. Verdict: SHIP, no CRITICAL or HIGH: no false green or false red constructible on markup that exists in src today, and the shared-parser change (keeping a bare v-on) is relax-only and cannot redden any existing finding. Confirmed clean: no wrapper component has a link mode; the $attrs and component-is fallthrough classes are empty today; the tokenizer handles quoted > and multi-line tags and mirrors the native form-depth tracking; the repo-wide scan is green by construction (every unbound wrapper is one of the seven marked style-guide specimens or a type="submit" inside a same-file form); the specimen marker lands as an inert data attribute; the AC3 registry mounts nothing from PaperReviewView; each registration asserts the disabled branch and the multi-state ones also assert data-reason; none of the six registrations depends on text the other session's #2654 label change touches. Triage, all taken in one round-2 commit (guards and comments only; no production change):
Merge gate: ci-required green at the round-2 head, aged three minutes, then merge commit; no further review round because round 2 changes guard rules and comments only, re-proved with the four guard specs and npm run lint. |
…ntory
MEDIUM 1. The AC3 header claimed to list every precondition each control can
be blocked by, but three controls have more than the registrations cover:
composer submit is a four-conjunct canSubmit, row-edit save is saveBlock ||
saving, and accept-on-board is boardPickBlock plus a six-way isActionDisabled.
Each bullet now enumerates every disjunct or conjunct the binding reads and
marks it REGISTERED or not-registered with a reason, and the header states that
an unmarked entry means unexamined rather than safe.
MEDIUM 2. hasVOnObjectBinding accepted any non-empty v-on object, so
v-on="{ mouseenter: showHint }" was redeemed while @mouseenter on the same tag
was reported. It now requires a spelled-out click key in any of the four JS
spellings, including the { click } shorthand. An expression that never spells
the key - v-on="listeners", or a spread-only object - is REPORTED rather than
accepted: source text cannot say what it resolves to, and the compiler-backed
scan already treats that shape on a native control as unproven. The former
listeners pin is inverted and a new block covers mouseenter-only, dblclick,
clickOutside and onclick as reported shapes.
LOW 3. TdIconButton declares no type prop and hard-codes type=button on its
root, so the old comment was wrong for it. Corrected, and TdIconButton is now
excluded from the submit clause via SUBMIT_CAPABLE_WRAPPERS rather than resting
the exemption on unverified fallthrough merge precedence.
LOW 4. Added the kebab spellings paper-h-l-btn, td-button and td-icon-button,
matching how the shared native list already carries router-link beside
routerlink. The subset test compares on the de-hyphenated name, and a second
loop pins SUBMIT_CAPABLE_WRAPPERS as a subset of the scanned set.
LOW 5. The vacuity canary counted FILES while being named and described as call
sites, so it would have passed with one call site left per file. It now counts
tag matches through WRAPPER_CALL_SITE, with the threshold raised from 20 to 100
against a real 156.
LOW 6. The loadFailed registration's comment said the accept click triggers the
failing load. onMounted primes it and onAccept re-triggers only from idle, so
the load has already failed by then. Comment corrected and an explicit flush
added after mount so the described sequence is the one that runs.
No production file changes in this commit.
Summary
PaperHLBtn,TdButtonandTdIconButtoneach render a real native<button>root, so the component files always passed the existing dead-affordance scan. Their call sites did not pass anything, because nothing scanned them. An enabled<PaperHLBtn label="Apply" />with no@clickshipped a focusable, enabled control that does nothing on activation, and because that markup contains no click handler at all, ESLint'svuejs-accessibility/click-events-have-key-eventsnever fired on it either. That is the #1941 defect in component clothing, across roughly 150 unscanned call sites in 35 SFCs.This PR adds a call-site scan for those three components and wires it into a repo-wide assertion, then disposes of every offender the scan found. It also makes the AC3 registry state its own scope: the registry proved two controls and said nothing about which controls it had examined, so a reader could not tell whether an absent control was safe or merely unlooked-at.
No production behaviour changes. The only production edit is seven
data-dead-affordance-exempt="visual-specimen"attributes on style-guide specimens, which land on the rendered<button>as ordinary fallthrough data attributes.Refs #1949.
Closed by this PR: AC1's wrapper-component half (call sites of the three wrapper button components are now scanned and clean) and AC3's stated-inventory gap (the registry now names its covered surfaces, every precondition each of their disabled bindings reads, and what is deliberately out).
Still open on #1949 after this: AC4 route-walking runtime coverage, and the parser LOWs already disclosed in the guard header (HTML-entity scheme obfuscation, runtime-assembled hrefs, dynamic event names,
:typeresolution).PaperReviewView's approve, reject and execute remain unregistered while #2629 holds those files.Changes
ddf486298test(guards): scan wrapper button call sites for dead affordancessrc/tests/guards/deadAnchors.spec.ts. AddsWRAPPER_BUTTON_COMPONENTS(the button-shaped subset of the existingNATIVE_INTERACTIVE_COMPONENTS) andfindDeadWrapperButtons. A call site is reported unless it binds a click, usesv-onobject syntax with a click key, carries a statictype="submit"with a provable form owner on a component that declares atypeprop, is permanently disabled, or carries the specimen marker. Form ownership reuses the existingformIdsandhasFormOwnerrather than restating the proof, and form depth is tracked across the same match stream as the wrapper tags so document order is respected.Two deliberate departures from the native-button clause, both documented in the code:
type="submit". A native<button>inside a form defaults to submit, butPaperHLBtnandTdButtondeclaretypewith a'button'default, so an untyped wrapper inside a form submits nothing. Treating a missing type as submit here would have silently exempted a real dead control.v-on="{ click: fn }"redeems a wrapper call site, unlike on a native control where the compiler-backed scan reports it as unproven. On these componentsclickis the declared emit, so object syntax with a spelled-out key genuinely wires activation.directiveOnlyMarkupnow keeps a barev-onalongsidev-on:x, so object syntax is read through the same attribute-boundary parse that already rejects an inertdata-note='@click="run"'. Nothing else keys off a barev-on, and no existing finding changes.The header's disclosure list is updated: the "component tags are not scanned" bullet is replaced with what is now scanned and what still is not, including the three live-at-runtime shapes that are reported anyway.
d4ec44c66test(guards): mark the seven inert PaperHLBtn style-guide specimenssrc/views/PaperStyleGuideView.vue. The only production file touched, and only to add the marker attribute plus two explanatory comments.b3484f06etest(guards): state the AC3 primary-action inventory and register six preconditionssrc/tests/guards/primaryActionGuards.spec.ts. Header defines a primary action as one whose activation writes or enqueues, and lists what is covered and what is out. Six new registrations back it. No change toguardedPrimaryAction.ts; its contract was sufficient as-is.4ad6519f0test(guards): address round-2 review on the wrapper scan and AC3 inventoryTwo MEDIUMs and four LOWs, one commit, no production files.
canSubmit(non-blank body,!submitting,selectedBoardIsWritable,!transcriptTooLong); row-edit save issaveBlock !== null || saving; accept-on-board isboardPickBlock !== null || isActionDisabled(item)with a six-wayisActionDisabled. Each bullet now enumerates every disjunct or conjunct the binding reads and marks it REGISTERED or not-registered with a reason (transient in-flight state, parent-owned prop, or out of this slice). The header states outright that an unmarked entry means unexamined, never safe, and that registering one precondition proves only that the control honours its binding at all.hasVOnObjectBindingaccepted any non-emptyv-onobject, sov-on="{ mouseenter: showHint }"was redeemed while@mouseenteron the same tag was reported. It now requires a spelled-outclickkey in any of the four JS spellings ({ click: fn },{ 'click': fn },{ "click": fn }, and the{ click }shorthand). Rule chosen for the unprovable case: report it.v-on="listeners"and a spread-only object are now reported, not accepted, because source text cannot say what they resolve to and the compiler-backed scan already treats that exact shape on a native control as unproven action evidence; accepting it on a wrapper would have been an inconsistency. The oldlistenerspin is inverted accordingly, and a new test block covers mouseenter-only,dblclick,clickOutsideandonclickas reported shapes alongside the accepted spellings. Nov-oncall site of any kind exists today, so the strictness costs nothing now.TdIconButtondeclares notypeprop and hard-codestype="button"on its root, so the comment claiming all three declare atypeprop with a'button'default was wrong for it. Comment corrected, andTdIconButtonis now excluded from the submit clause via a newSUBMIT_CAPABLE_WRAPPERSset rather than keeping an exemption that would rest on unverified fallthrough merge precedence over an explicit root attribute. A test pins<form><TdIconButton type="submit">as reported and<form><TdButton type="submit">as clean.paper-h-l-btn,td-buttonandtd-icon-button, matching how the shared native list already carriesrouter-linkbesiderouterlink. The subset test now compares on the de-hyphenated name, and a second loop pinsSUBMIT_CAPABLE_WRAPPERSas a subset of the scanned set. A new test block covers both spellings reaching the same verdict, and that<td-button-group>is still not a prefix match.WRAPPER_CALL_SITEregex built from the same set, with the threshold raised from 20 to 100 against a real 156.loadFailedregistration's comment said the accept click triggers the failing load.onMountedprimes it andonAcceptre-triggers only fromidle, so the load has already failed by the time the picker opens. Comment corrected and an explicit flush added after mount so the described sequence is the one that runs.Test plan
Run from
frontend/taskdeck-webafternpm ci.Verified at head
4ad6519f0npx vitest --run --maxWorkers=2 src/tests/guards/deadAnchors.spec.ts src/tests/guards/primaryActionGuards.spec.ts src/tests/guards/shortcutLedgerTruth.spec.ts src/tests/guards/shortcutNotation.spec.ts— 4 files passed, 47 tests passed (45 at round 1; the two new blocks are thev-onclick-key rule and the kebab-spelling coverage).deadAnchorswent from 18 at base to 26;primaryActionGuardsfrom 5 to 11.npx vitest --run --maxWorkers=2 src/tests/views/paper/inbox/PaperTriageTable.spec.ts src/tests/views/paper/inbox/PaperTriageRowEdit.spec.ts src/tests/views/paper/inbox/PaperCaptureNib.spec.ts— 3 files passed, 95 tests passed. These are the specs of the three components the registrations mount.npm run typecheck— exit 0.npx eslint src/tests/guards/deadAnchors.spec.ts src/tests/guards/primaryActionGuards.spec.ts— clean.npm run lint(the job CI runs,eslint . --max-warnings=20) — 0 errors, 8 warnings, under budget. All 8 are pre-existing and in files this PR does not touch:TdDateField.vue,AgentRunDetailView.vue,AgentRunsView.vue,AgentsView.vue,PaperCaptureComposer.vue.npm run build— succeeded, PWA precache 142 entries.git diff --check— clean.Red-first evidence (recorded at round 1, still the evidence for the detector)
Repo-wide scan, recorded at
ddf486298before any marker landed.never ships a wrapper button call site without an actionfailed with exactly 7 offenders, and the other 23 tests in the file stayed green (so wideningdirectiveOnlyMarkupchanged no existing finding):Mutation A, detector blinded (
findDeadWrapperButtonsreturns[]unconditionally). 6 failed, 18 passed: the live-detector canary plus all five detector blocks that assert a shape IS reported. Reverted.Mutation B, every exemption clause disabled (report every wrapper tag). 4 failed, 20 passed: all three blocks that assert a shape is CLEAN, plus the repo-wide assertion. Reverted.
Together these two runs prove the detector can fail in both directions rather than being a green no-op. The round-2 rules are pinned by assertions in both directions in the same style:
v-on="{ click: apply }"clean againstv-on="{ mouseenter: showHint }"reported,<form><TdButton type="submit">clean against<form><TdIconButton type="submit">reported.Registry vacuity check. With
isDisabledinguardedPrimaryAction.tstemporarily forced tofalse, 7 of the 8 registrations failed with "Enabled-and-silent primary action", proving each one really locates and evaluates its trigger. The one that still passed is theloadFailedregistration, because that state renders its reason withrole="alert"and therefore satisfies both branches of the contract, not just the disabled one. Reverted;guardedPrimaryAction.tsis byte-identical to the base and appears in no commit on this branch.Repo-wide offender list and disposition
The scan reported 7 offenders. All 7 are in
src/views/PaperStyleGuideView.vue, and all 7 are intentional visual specimens: the fivePaperHLBtnvariant swatches in the primitives section, and the two in the opposite-theme preview frame. Both rows exist to show what a variant looks like, so there is no intended handler to restore. Disposition: marked withdata-dead-affordance-exempt="visual-specimen", the same narrow opt-out the four native.pbtnspecimens a few lines above in the same file already carry.Zero product offenders. Of the 156 wrapper call sites the scan walks, 142 bind
@click, 7 aretype="submit"inside a real<form>(BoardsListView,IntegrationsView,NotificationPreferencesView,PaperCardComposer,PaperBoardViewtwice,ProfileSettingsView), and 7 were the specimens above. Nothing needed a handler added, so the "obvious missing handler" carve-out was not used and no production logic changed.NOT verified
PaperStyleGuideViewrenders correctly in a browser. That view has no spec anywhere in the repo, sonpm run build,npm run typecheckand the guard are its only coverage here. The change is additive data attributes on a component that already spreads fallthrough attrs onto its<button>root.type="submit"overrideTdIconButton's hard-coded roottype="button". Deliberately not relied on and not measured — the scan now reports that shape instead of exempting it.Boundaries and risks
Held files, not edited.
PaperTriageTable.vueandPaperCaptureComposer.vueare held by #2654;PaperReviewView.vueand its spec by #2629. None of them is in this diff. The three files changed here aresrc/tests/guards/deadAnchors.spec.ts,src/tests/guards/primaryActionGuards.spec.tsandsrc/views/PaperStyleGuideView.vue.Registrations that mount a held file. The registry mounts
PaperTriageTableandPaperCaptureComposer, as it already did before this PR. The two newPaperTriageTableregistrations depend on this markup:button[data-action="accept"],button[data-action="accept-on-board"],button[data-action="retry-board-load"],[data-testid="board-pick-reason"]with itsdata-reason, the board<select>and its:value="board.id"options, and thecanWritegating behindboardPickBlock.Checked against #2654's head as of this PR: that PR is a two-line i18n change replacing the hard-coded text of two placeholder
<option>labels witht(...)calls. It touches none of the hooks above, so these registrations survive it as it stands. If #2654's head moves and changes any of that markup, the two newPaperTriageTableregistrations are the ones to drop; the other four new registrations touch onlyPaperTriageRowEditandPaperCaptureNib, which nothing holds.Risk: the
directiveOnlyMarkupwidening. It is a one-token change to shared parser code used by the anchor, native-button and aria-label finders. Nothing else reads a barev-on, and the fulldeadAnchorsfile was green on the same commit that introduced it, including the existing test that a native<button v-on="{ click: run }">is still reported. A detector case pins that an inertdata-note='v-on="{ click: run }"'is still reported, so the attribute-boundary parse did not loosen.Risk: the guard is stricter than the runtime in three places. A wrapper call site bound only by
@mousedown, one bound by av-onexpression that never spells theclickkey, and a<TdIconButton type="submit">inside a form are all reported despite being potentially live. Each is deliberate, documented at its constant, and costs nothing today because no such call site exists. The cost would be a future false positive, and the header tells the next agent what each one means.Not a risk this PR takes on. The scan is source-text only. It does not compiler-expand the wrapper components, so it still cannot see a handler passed through
v-bind="attrs"or assembled at runtime. The header says so rather than implying the call sites are now fully proven.Worktree.
.worktrees/codex-1949-wrapper-actionability, ready for plaingit worktree removeonce this is merged.git status --porcelain --ignoredlists onlyfrontend/taskdeck-web/dist/andfrontend/taskdeck-web/node_modules/, both regenerable build artifacts. Nothing was copied out because nothing needed to survive.